RE: [PATCH 16/22] xlink-ipc: Add xlink ipc driver

[Kelly, Seamus]

From: Joe Perches <joe@xxxxxxxxxxx> 
Sent: Monday, December 7, 2020 2:33 AM
To: mgross@xxxxxxxxxxxxxxx; markgross@xxxxxxxxxx; arnd@xxxxxxxx; bp@xxxxxxx; damien.lemoal@xxxxxxx; dragan.cvetic@xxxxxxxxxx; gregkh@xxxxxxxxxxxxxxxxxxx; corbet@xxxxxxx; leonard.crestez@xxxxxxx; palmerdabbelt@xxxxxxxxxx; paul.walmsley@xxxxxxxxxx; peng.fan@xxxxxxx; robh+dt@xxxxxxxxxx; shawnguo@xxxxxxxxxx
Cc: linux-kernel@xxxxxxxxxxxxxxx; Kelly, Seamus <seamus.kelly@xxxxxxxxx>; linux-doc@xxxxxxxxxxxxxxx; Ryan Carnaghi <ryan.r.carnaghi@xxxxxxxxx>
Subject: Re: [PATCH 16/22] xlink-ipc: Add xlink ipc driver

On Tue, 2020-12-01 at 14:35 -0800, mgross@xxxxxxxxxxxxxxx wrote:
> From: Seamus Kelly <seamus.kelly@xxxxxxxxx>
> 
> Add xLink driver, which interfaces the xLink Core driver with the Keem 
> Bay VPU IPC driver, thus enabling xLink to control and communicate 
> with the VPU IP present on the Intel Keem Bay SoC.

Trivial style comments:

> diff --git a/drivers/misc/xlink-ipc/xlink-ipc.c 
> b/drivers/misc/xlink-ipc/xlink-ipc.c

[]

> +/*
> + * xlink_reserved_memory_init() - Initialize reserved memory for the device.
> + *
> + * @xlink_dev:	[in] The xlink ipc device the reserved memory is allocated to.
> + *
> + * Return: 0 on success, negative error code otherwise.
> + */
> +static int xlink_reserved_memory_init(struct xlink_ipc_dev 
> +*xlink_dev) {
> +	struct device *dev = &xlink_dev->pdev->dev;
> +
> +	xlink_dev->local_xlink_mem.dev = init_xlink_reserved_mem_dev(dev,
> +								     "xlink_local_reserved",
> +								     LOCAL_XLINK_IPC_BUFFER_IDX);
> +	if (!xlink_dev->local_xlink_mem.dev)
> +		return -ENOMEM;

This sort of code, with a repeated struct dereference, generally reads better using a temporary and is also less prone to typo use.

	struct device *dev = &xlink_dev->pdev->dev;
	struct xlink_buf_mem *lxm = &xlink_dev->local_xlink_mem;

	lxm->dev = init_xlink_reserved_mem_dev(dev, "xlink_local_reserved",
					       LOCAL_XLINK_IPC_BUFFER_IDX);
	if (!lxm->dev)
		return -ENOMEM;

> +	xlink_dev->local_xlink_mem.size = get_xlink_reserved_mem_size(dev,
> +								      LOCAL_XLINK_IPC_BUFFER_IDX);

	lxm->size = get_xlink_reserved_mem_size(dev, LOCAL_XLINK_IPC_BUFFER_IDX);

etc...

[Kelly, Seamus] Thank you!  will do.



--------------------------------------------------------------
Intel Research and Development Ireland Limited
Registered in Ireland
Registered Office: Collinstown Industrial Park, Leixlip, County Kildare
Registered Number: 308263


This e-mail and any attachments may contain confidential material for the sole
use of the intended recipient(s). Any review or distribution by others is
strictly prohibited. If you are not the intended recipient, please contact the
sender and delete all copies.