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

[Abhishek Tamboli <abhishektamboli9@gmail.com>]

On Thu, Aug 08, 2024 at 09:54:40AM -0700, Guenter Roeck wrote:
> 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.

Hi Guenter,
Thank you for the feedback. I'll look into the drivers/staging directory
and see where I might be able to contribute effectively.

Regards,
Abhishek