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@xxxxxxxxx> > > > > --- > > > > 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
