Re: [PATCH] hwmon: (lm93) Return error values on read failure

[Guenter Roeck <linux@roeck-us.net>]

On Thu, Aug 08, 2024 at 07:46:36AM +0530, Abhishek Tamboli wrote:
> On Wed, Aug 07, 2024 at 11:38:34AM -0700, Guenter Roeck wrote:
> Hi Guenter,
> Thank you for your feedback.
> > On 8/7/24 11:17, Abhishek Tamboli wrote:
> > > Fix the issue of lm93_read_byte() and lm93_read_word() return 0 on
> > > read failure after retries, which could be confused with valid data.
> > > 
> > > Address the TODO: what to return in case of error?
> > > 
> > > Signed-off-by: Abhishek Tamboli <abhishektamboli9@gmail.com>
> > > ---
> > >   drivers/hwmon/lm93.c | 10 ++++++----
> > >   1 file changed, 6 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/drivers/hwmon/lm93.c b/drivers/hwmon/lm93.c
> > > index be4853fad80f..b76f3c1c6297 100644
> > > --- a/drivers/hwmon/lm93.c
> > > +++ b/drivers/hwmon/lm93.c
> > > @@ -798,6 +798,7 @@ static unsigned LM93_ALARMS_FROM_REG(struct block1_t b1)
> > >   static u8 lm93_read_byte(struct i2c_client *client, u8 reg)
> > 
> > This is still returning an u8.
> My interpretation of the TODO was to address the error condition while keeping the 
> existing logic of the driver intact. I understand that this driver is 
> old and that changes should be approached with caution.

Those TODOs are, at this point, pretty much pointless. If you want to help
with improving kernel code, it might be better to pick something from the
drivers/staging/ directory and help improve it.

The only thing that would really help for the lm93 driver would be a
complete overhaul, and that would only make sense if someone has real
hardware to test the resulting code; the driver is too complex to just
rely on unit tests. For example, the excessive retries might be because
the chip is really bad with its communication, or it may have been
observed on a system with a bad i2c controller, making it completely
unnecesssary today. Either case, if those retries are really necessary
due to chip issues, they should be hiddden behind regmap (which should
also be used to replace in-driver caching). And so on.

Really, if you want to get into kenrel development, it would be much
better to help improving code which is actually being used, as mentioned
above.

Thanks,
Guenter