On Tue, Jan 17, 2023 at 07:11:39PM +0530, Nipun Gupta wrote:
> Create sysfs entry for CDX devices.
>
> Sysfs entries provided in each of the CDX device detected by
> the CDX controller
> - vendor id
> - device id
> - remove
> - reset of the device.
> - driver override
>
> Signed-off-by: Puneet Gupta <puneet.gupta@xxxxxxx>
> Signed-off-by: Nipun Gupta <nipun.gupta@xxxxxxx>
> Signed-off-by: Tarak Reddy <tarak.reddy@xxxxxxx>
> ---
> Documentation/ABI/testing/sysfs-bus-cdx | 34 +++++
> drivers/bus/cdx/cdx.c | 144 ++++++++++++++++++++
> drivers/bus/cdx/controller/cdx_controller.c | 19 +++
> drivers/bus/cdx/controller/mcdi_functions.c | 14 ++
> drivers/bus/cdx/controller/mcdi_functions.h | 11 ++
> include/linux/cdx/cdx_bus.h | 23 ++++
> 6 files changed, 245 insertions(+)
>
> diff --git a/Documentation/ABI/testing/sysfs-bus-cdx b/Documentation/ABI/testing/sysfs-bus-cdx
> index 8c2425fdb6d9..1e0fdce18cde 100644
> --- a/Documentation/ABI/testing/sysfs-bus-cdx
> +++ b/Documentation/ABI/testing/sysfs-bus-cdx
> @@ -10,3 +10,37 @@ Description:
> For example::
>
> # echo 1 > /sys/bus/cdx/rescan
> +
> +What: /sys/bus/cdx/devices/.../vendor
> +Date: January 2023
> +Contact: nipun.gupta@xxxxxxx
> +Description:
> + Vendor ID for this CDX device
What format is this in? How big is it? Examples?
> +
> +What: /sys/bus/cdx/devices/.../device
> +Date: January 2023
> +Contact: nipun.gupta@xxxxxxx
> +Description:
> + Device ID for this CDX device
Same here, be specific.
> +
> +What: /sys/bus/cdx/devices/.../reset
> +Date: January 2023
> +Contact: nipun.gupta@xxxxxxx
> +Description:
> + Writing a non-zero value to this file would reset the
> + CDX device
> +
> + For example::
> +
> + # echo 1 > /sys/bus/cdx/.../reset
Will that remove the device from the driver too?
Again, more documentation please.
> +
> +What: /sys/bus/cdx/devices/.../remove
> +Date: January 2023
> +Contact: tarak.reddy@xxxxxxx
> +Description:
> + Writing a non-zero value to this file would remove the
> + corrosponding device from the CDX bus
> +
> + For example::
> +
> + # echo 1 > /sys/bus/cdx/devices/.../remove
Why would you want to remove a device from the bus like this?
> diff --git a/drivers/bus/cdx/cdx.c b/drivers/bus/cdx/cdx.c
> index 2737470f08d3..1372d8dcaa4c 100644
> --- a/drivers/bus/cdx/cdx.c
> +++ b/drivers/bus/cdx/cdx.c
> @@ -72,6 +72,38 @@ static DECLARE_BITMAP(cdx_controller_id_map, MAX_CDX_CONTROLLERS);
> /* List of CDX controllers registered with the CDX bus */
> static LIST_HEAD(cdx_controllers);
>
> +/**
> + * reset_cdx_device - Reset a CDX device
> + * @dev: CDX device
> + * @data: This is always passed as NULL, and is not used in this API,
> + * but is required here as the bus_for_each_dev() API expects
> + * the passed function (reset_cdx_device) to have this
> + * as an argument.
> + *
> + * @return: -errno on failure, 0 on success.
I recommend this function actually be the one without the data pointer,
that way you don't get confused here.
> + */
> +static int reset_cdx_device(struct device *dev, void *data)
> +{
> + struct cdx_device *cdx_dev = to_cdx_device(dev);
> + struct cdx_controller *cdx = cdx_dev->cdx;
> + struct cdx_device_config dev_config;
> + int ret;
> +
> + dev_config.type = CDX_DEV_RESET_CONF;
> + ret = cdx->ops->dev_configure(cdx, cdx_dev->bus_num,
> + cdx_dev->dev_num, &dev_config);
> + if (ret)
> + dev_err(dev, "cdx device reset failed\n");
> +
> + return ret;
> +}
> +
> +int cdx_dev_reset(struct device *dev)
> +{
> + return reset_cdx_device(dev, NULL);
> +}
> +EXPORT_SYMBOL_GPL(cdx_dev_reset);
Remove the indirection as mentioned above please.
Otherwise, very minor comments, nice work.
greg k-h
