Hi Greg,
On Wed, Nov 06, 2024 at 01:42:33PM +0100, Greg KH wrote:
> On Wed, Nov 06, 2024 at 01:34:38PM +0100, Stanislaw Gruszka wrote:
> > For diagnostics purposes read firmware version from device
> > and print it to dmesg during initialization.
>
> No, sorry, when drivers work properly, they are quiet. Think about what
> your kernel log would look like if you did this for every single driver
> in the tree.
>
> >
> > Reviewed-by: Hans de Goede <hdegoede@xxxxxxxxxx>
> > Tested-by: Hans de Goede <hdegoede@xxxxxxxxxx> # ThinkPad X1 Yoga Gen 8, ov2740
> > Signed-off-by: Stanislaw Gruszka <stanislaw.gruszka@xxxxxxxxxxxxxxx>
> > ---
> > drivers/usb/misc/usb-ljca.c | 29 +++++++++++++++++++++++++++++
> > 1 file changed, 29 insertions(+)
> >
> > diff --git a/drivers/usb/misc/usb-ljca.c b/drivers/usb/misc/usb-ljca.c
> > index d9c21f783055..e698a1075a40 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;
> > @@ -695,6 +703,25 @@ static int ljca_reset_handshake(struct ljca_adapter *adap)
> > return 0;
> > }
> >
> > +static void ljca_print_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;
>
> Why not return the error?
An error here would indicate something is a little fishy but doesn't
prevent the device from working as such. I'd think it's fine as-is.
>
> > + }
> > +
> > + dev_info(adap->dev, "Firmware version: %d.%d.%d.%d\n",
> > + version.major, version.minor,
> > + le16_to_cpu(version.patch), le16_to_cpu(version.build));
>
> Again, sorry, but no. Feel free to dump this in a sysfs file if you
> really want to get access to it, but not in the kernel log.
I guess dev_dbg() should do as well.
--
Regards,
Sakari Ailus
