lkml.org 
[lkml]   [2023]   [Jan]   [17]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH 07/19] bus/cdx: add device attributes
    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@amd.com>
    > Signed-off-by: Nipun Gupta <nipun.gupta@amd.com>
    > Signed-off-by: Tarak Reddy <tarak.reddy@amd.com>
    > ---
    > 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@amd.com
    > +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@amd.com
    > +Description:
    > + Device ID for this CDX device

    Same here, be specific.

    > +
    > +What: /sys/bus/cdx/devices/.../reset
    > +Date: January 2023
    > +Contact: nipun.gupta@amd.com
    > +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@amd.com
    > +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

    \
     
     \ /
      Last update: 2023-03-26 23:44    [W:6.253 / U:0.124 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site