Re: [PATCH v4 3/3] usb: misc: ljca: add firmware version sysfs attribute

[Greg Kroah-Hartman <gregkh@linuxfoundation.org>]

On Tue, Nov 12, 2024 at 08:55:14AM +0100, Stanislaw Gruszka wrote:
> For diagnostics purpose read firmware version during probe and add
> sysfs attribute to make that information available.
> 
> Acked-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> Signed-off-by: Stanislaw Gruszka <stanislaw.gruszka@linux.intel.com>
> ---
> v3->v4: use sysfs attribute to provide firmware version,
> 	drop tags from Hans since the code changed
> v2->v3: use dev_dbg() for printing the firmware version
> v1->v2: fix 80-chars line wrap
> 
>  .../ABI/testing/sysfs-bus-usb-devices-ljca    |  9 ++++
>  drivers/usb/misc/usb-ljca.c                   | 53 +++++++++++++++++++
>  2 files changed, 62 insertions(+)
>  create mode 100644 Documentation/ABI/testing/sysfs-bus-usb-devices-ljca
> 
> diff --git a/Documentation/ABI/testing/sysfs-bus-usb-devices-ljca b/Documentation/ABI/testing/sysfs-bus-usb-devices-ljca
> new file mode 100644
> index 000000000000..f5eb38bf99a8
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-bus-usb-devices-ljca
> @@ -0,0 +1,9 @@
> +What:		/sys/bus/usb/devices/.../ljca_version
> +Date:		November 2024
> +KernelVersion:	6.13
> +Contact:	Sakari Ailus <sakari.ailus@linux.intel.com>
> +Description:
> +		Provides the firmware version of LJCA device.
> +		The format is Major.Minor.Patch.Build, where
> +		Major, Minor, Patch, and Build are decimal numbers.
> +		For example: 1.0.0.256
> diff --git a/drivers/usb/misc/usb-ljca.c b/drivers/usb/misc/usb-ljca.c
> index d9c21f783055..4b5bd5b27b2a 100644
> --- a/drivers/usb/misc/usb-ljca.c
> +++ b/drivers/usb/misc/usb-ljca.c
> @@ -43,6 +43,7 @@ enum ljca_client_type {
>  
>  /* MNG client commands */
>  enum ljca_mng_cmd {
> +	LJCA_MNG_GET_VERSION = 1,
>  	LJCA_MNG_RESET = 2,
>  	LJCA_MNG_ENUM_GPIO = 4,
>  	LJCA_MNG_ENUM_I2C = 5,
> @@ -68,6 +69,13 @@ struct ljca_msg {
>  	u8 data[] __counted_by(len);
>  } __packed;
>  
> +struct ljca_fw_version {
> +	u8 major;
> +	u8 minor;
> +	__le16 patch;
> +	__le16 build;
> +} __packed;
> +
>  struct ljca_i2c_ctr_info {
>  	u8 id;
>  	u8 capacity;
> @@ -152,8 +160,10 @@ struct ljca_adapter {
>  	struct mutex mutex;
>  
>  	struct list_head client_list;
> +	struct ljca_fw_version fw_version;
>  
>  	bool disconnect;
> +	bool fw_version_valid;
>  
>  	u32 reset_id;
>  };
> @@ -740,6 +750,24 @@ static int ljca_enumerate_clients(struct ljca_adapter *adap)
>  	return ret;
>  }
>  
> +static void ljca_read_fw_version(struct ljca_adapter *adap)
> +{
> +	struct ljca_fw_version version;
> +	int ret;
> +
> +	ret = ljca_send(adap, LJCA_CLIENT_MNG, LJCA_MNG_GET_VERSION, NULL, 0,
> +			(u8 *)&version, sizeof(version), true,
> +			LJCA_WRITE_ACK_TIMEOUT_MS);
> +
> +	if (ret != sizeof(version)) {
> +		dev_err(adap->dev, "Get version failed, ret: %d\n", ret);
> +		return;
> +	}
> +
> +	adap->fw_version = version;
> +	adap->fw_version_valid = true;
> +}
> +
>  static int ljca_probe(struct usb_interface *interface,
>  		      const struct usb_device_id *id)
>  {
> @@ -811,6 +839,8 @@ static int ljca_probe(struct usb_interface *interface,
>  	if (ret)
>  		goto err_free;
>  
> +	ljca_read_fw_version(adap);
> +
>  	/*
>  	 * This works around problems with ov2740 initialization on some
>  	 * Lenovo platforms. The autosuspend delay, has to be smaller than
> @@ -874,6 +904,28 @@ static int ljca_resume(struct usb_interface *interface)
>  	return usb_submit_urb(adap->rx_urb, GFP_KERNEL);
>  }
>  
> +static ssize_t ljca_version_show(struct device *dev,
> +				 struct device_attribute *attr, char *buf)
> +{
> +	struct usb_interface *intf = to_usb_interface(dev);
> +	struct ljca_adapter *adap = usb_get_intfdata(intf);
> +	struct ljca_fw_version ver = adap->fw_version;
> +
> +	if (!adap->fw_version_valid)
> +		return -ENODATA;

Why are you showing this sysfs attribute if there is no firmware
version?  Please only create the file if it is possible to read a valid
value from it.

thanks,

greg k-h