Hello Guenter,
On Wed, Jul 10, 2024 at 07:09:28AM -0700, Guenter Roeck wrote:
> On 7/10/24 01:35, Uwe Kleine-König wrote:
> > The devices supported by this driver report the model name in their
> > register space. The way this is evaluated allows longer strings than the
> > driver's model list. Document this behaviour in a code comment to lessen
> > the surprise for the next reader.
> >
> > Additionally emit the reported model name in case of a mismatch.
> >
> > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@xxxxxxxxxxxx>
> > ---
> > drivers/hwmon/pmbus/ltc4286.c | 12 +++++++++---
> > 1 file changed, 9 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/hwmon/pmbus/ltc4286.c b/drivers/hwmon/pmbus/ltc4286.c
> > index 9e7ceeb7e789..2e5532300eff 100644
> > --- a/drivers/hwmon/pmbus/ltc4286.c
> > +++ b/drivers/hwmon/pmbus/ltc4286.c
> > @@ -95,13 +95,19 @@ static int ltc4286_probe(struct i2c_client *client)
> > "Failed to read manufacturer model\n");
> > }
> > - for (mid = ltc4286_id; mid->name[0]; mid++) {
> > + for (mid = ltc4286_id; mid->name[0]; mid++)
> > + /*
> > + * Note that by limiting the comparison to strlen(mid->name)
> > + * chars, the device reporting "lTc4286chocolade" is accepted,
> > + * too.
> > + */
>
> This is misleading; the desired match is LTC4286 and all its variants (LTC4286[A-Z] and
> whatever else the vendor can come up with), i.e., it is supposed to include all device
> variants, and ignoring case since it is irrelevant. Referring to the odd string just
> makes that look unnecessarily bad. I am not going to apply this patch, sorry.
You're quite an optimist, expecting "whatever the vendor can come up
with" but nothing bad :-)
Anyhow, what about updating the comment to read:
Note that by limiting the comparison to strlen(mid->name) chars,
matching for devices that report their model with a variant
suffix is supported.
While looking at the code again, I spotted a (theoretic) bug: Given that
block_buffer isn't initialized at function entry, it might well contain
"LTC4286something" (which might even be realistic if the driver just
probed on a different bus?). Now if i2c_smbus_read_block_data(...
PMBUS_MFR_MODEL, ...) returned something between 0 and 6, we're looking
at bytes that didn't come from the block read.
Best regards
Uwe
Attachment:
signature.asc
Description: PGP signature
