Hi Guenter-san, Thank you so much for your help. > A driver should return the error codes it receives from lower level > drivers, ie > in this case the error code from the i2c controller driver. It should > not modify > error return codes. In fact, if a driver does modify error return codes, > static analyzers will complain. Noted about this I will try to consider to do this. Regards, Ikegami > -----Original Message----- > From: Guenter Roeck [mailto:groeck7@xxxxxxxxx] On Behalf Of Guenter > Roeck > Sent: Thursday, July 12, 2018 11:06 PM > To: IKEGAMI Tokunori; Jean Delvare > Cc: PACKHAM Chris; linux-hwmon@xxxxxxxxxxxxxxx > Subject: Re: [PATCH 3/5] hwmon: adt7475: Change to use adt7475_read macro > > On 07/12/2018 06:47 AM, IKEGAMI Tokunori wrote: > > Hi Guenter-san, > > > > Sorry but let me consult one more thing. > > > > Since I have just remembered that there are -EAGAIN and -ETIMEDOUT > possible to be returned by I2C driver. > > Those were mentioned to be able to be checked by the hwmon adt7475 driver > in our company not long ago. > > Also I have just confirmed the Documentation/i2c/fault-codes so it > seems still correct. > > > > How do you think about this? > > Still those errors also should be just returned to user space? > > > > A driver should return the error codes it receives from lower level > drivers, ie > in this case the error code from the i2c controller driver. It should > not modify > error return codes. In fact, if a driver does modify error return codes, > static analyzers will complain. > > Hope this helps, > Guenter > > > Sorry for many times asking you but if you have a time please let me > know your opinion. > > > > Regards, > > Ikegami > > > >> -----Original Message----- > >> From: IKEGAMI Tokunori > >> Sent: Thursday, July 12, 2018 3:23 PM > >> To: 'Guenter Roeck'; Jean Delvare > >> Cc: PACKHAM Chris; linux-hwmon@xxxxxxxxxxxxxxx > >> Subject: RE: [PATCH 3/5] hwmon: adt7475: Change to use adt7475_read > macro > >> > >> Hi Guenter-san, > >> > >> Thank you so much for the explanation. > >> I could understand enough it. > >> I will abandon these patches for now. > >> > >>> What is an "I2C error unit" ? > >> > >> The error unit is caused by the I2C isolator failure device then > bad > >> I2C signals causes the error. > >> Our products have been replaced the device to other new device for > the > >> manufacturing. > >> But still there are many units using the old device and it is > possible > >> to be caused the failure behavior in future. > >> We tried to change the I2C clock speed on the error unit but the > issue > >> was not able to be resolved. > >> By the way we have fixed our original user space driver for other > >> feature to retry for the error. > >> > >>> Yes, the driver should check for errors, but it should report all > errors > >>> to user space and neither retry nor silently hide/ignore errors > (unless > >>> a problem is known to be a chip problem). > >> > >> I will do consider this in future. > >> But at first I will investigate the cause of another voltage > warning > >> issue at first. > >> It is not caused by the I2C error but sometimes detected the voltage > >> warning actually. > >> > >> Regards, > >> Ikegami > >> > >>> -----Original Message----- > >>> From: Guenter Roeck [mailto:groeck7@xxxxxxxxx] On Behalf Of Guenter > >>> Roeck > >>> Sent: Thursday, July 12, 2018 1:23 PM > >>> To: IKEGAMI Tokunori; Jean Delvare > >>> Cc: PACKHAM Chris; linux-hwmon@xxxxxxxxxxxxxxx > >>> Subject: Re: [PATCH 3/5] hwmon: adt7475: Change to use adt7475_read > >> macro > >>> > >>> On 07/11/2018 09:01 PM, IKEGAMI Tokunori wrote: > >>>> Hi Guenter-san, > >>>> > >>>> Thank you so much for your comments. > >>>> > >>>> Oh I see and actually we have some I2C error units in our products. > >>>> The patch is for the error case mainly. > >>> > >>> What is an "I2C error unit" ? > >>> > >>>> But I think that sometimes the I2C error is still caused on the normal > >>> units also actually. > >>>> I am not sure about that the normal units error behavior is caused > >> by > >>> the board (I2C controller) or the chip ADT7476A but probably the > former > >>> but not sure. > >>>> So should these patches not be applied to the adt7475 driver as you > >>> mentioned? > >>>> > >>> > >>> Drivers are not responsible for handling bad board behavior, so > retries > >>> due to controller or board issues should not be handled by the chip > >>> driver. > >>> Otherwise we would end up having to sprinkle retries into all i2c > >> drivers > >>> in the kernel. > >>> > >>> If the problem is caused by the controller, it should be handled in > >> the > >>> controller driver. If the problem is caused by bad i2c signals, it > >> should > >>> be handled by hardware (or maybe by selecting a different clock > speed). > >>> > >>>> At first I thought that the I2C SMBus APIs return error codes but > >> adt7475 > >>> driver does not check so it should be checked the error codes. > >>>> Is this not correct? > >>>> > >>> > >>> Yes, the driver should check for errors, but it should report all > errors > >>> to user space and neither retry nor silently hide/ignore errors > (unless > >>> a problem is known to be a chip problem). > >>> > >>> Guenter > >>> > >>>> To make sure let me confirm above. > >>>> > >>>> Regards, > >>>> Ikegami > >>>> > >>>>> -----Original Message----- > >>>>> From: linux-hwmon-owner@xxxxxxxxxxxxxxx > >>>>> [mailto:linux-hwmon-owner@xxxxxxxxxxxxxxx] On Behalf Of Guenter > >>> Roeck > >>>>> Sent: Thursday, July 12, 2018 12:42 PM > >>>>> To: IKEGAMI Tokunori; Jean Delvare > >>>>> Cc: PACKHAM Chris; linux-hwmon@xxxxxxxxxxxxxxx > >>>>> Subject: Re: [PATCH 3/5] hwmon: adt7475: Change to use adt7475_read > >>> macro > >>>>> > >>>>> On 07/11/2018 07:52 PM, IKEGAMI Tokunori wrote: > >>>>>> Hi Guenter-san, > >>>>>> > >>>>>> Thank you so much for your comments. > >>>>>> Okay now I am thinking to change the adt7475_read macro to a > function > >>>>> to repeat for the error case. > >>>>>> If you have any comment about this please let me know. > >>>>> > >>>>> Yes - we should only do this if it is known to be a chip problem. > >>>>> Patching the chip driver for a board (or i2c controller) problem > >>>>> is not appropriate. > >>>>> > >>>>> Guenter > >>>>> > >>>>>> Anyway I will do send v2 version patches later. > >>>>>> > >>>>>> Regards, > >>>>>> Ikegami > >>>>>> > >>>>>>> -----Original Message----- > >>>>>>> From: Guenter Roeck [mailto:groeck7@xxxxxxxxx] On Behalf Of > >> Guenter > >>>>>>> Roeck > >>>>>>> Sent: Thursday, July 12, 2018 10:50 AM > >>>>>>> To: IKEGAMI Tokunori; Jean Delvare > >>>>>>> Cc: PACKHAM Chris; linux-hwmon@xxxxxxxxxxxxxxx > >>>>>>> Subject: Re: [PATCH 3/5] hwmon: adt7475: Change to use > >> adt7475_read > >>>>> macro > >>>>>>> > >>>>>>> On 07/11/2018 06:04 PM, Tokunori Ikegami wrote: > >>>>>>>> It shoudl be same as with other functions to use adt7475_read. > >>>>>>>> So change to use it instead of i2c_smbus_read_byte_data. > >>>>>>>> > >>>>>>> > >>>>>>> I don't see a point in this change. Replacing a function name > >> doesn't > >>>>>>> make > >>>>>>> the code easier to read. If anything, you could consider dropping > >>>>>>> adt7475_read > >>>>>>> and calling i2c_smbus_read_byte_data() directly. > >>>>>>> > >>>>>>> Guenter > >>>>>>> > >>>>>>>> Signed-off-by: Tokunori Ikegami > <ikegami@xxxxxxxxxxxxxxxxxxxx> > >>>>>>>> Cc: Chris Packham <chris.packham@xxxxxxxxxxxxxxxxxxx> > >>>>>>>> Cc: linux-hwmon@xxxxxxxxxxxxxxx > >>>>>>>> --- > >>>>>>>> drivers/hwmon/adt7475.c | 2 +- > >>>>>>>> 1 file changed, 1 insertion(+), 1 deletion(-) > >>>>>>>> > >>>>>>>> diff --git a/drivers/hwmon/adt7475.c > b/drivers/hwmon/adt7475.c > >>>>>>>> index a40eb62ee6b1..bad250729e99 100644 > >>>>>>>> --- a/drivers/hwmon/adt7475.c > >>>>>>>> +++ b/drivers/hwmon/adt7475.c > >>>>>>>> @@ -1012,7 +1012,7 @@ static ssize_t > >>>>>>> pwm_use_point2_pwm_at_crit_store(struct device *dev, > >>>>>>>> return -EINVAL; > >>>>>>>> > >>>>>>>> mutex_lock(&data->lock); > >>>>>>>> - data->config4 = i2c_smbus_read_byte_data(client, > >>>>>>> REG_CONFIG4); > >>>>>>>> + data->config4 = adt7475_read(REG_CONFIG4); > >>>>>>>> if (val) > >>>>>>>> data->config4 |= CONFIG4_MAXDUTY; > >>>>>>>> else > >>>>>>>> > >>>>>> > >>>>> > >>>>> -- > >>>>> To unsubscribe from this list: send the line "unsubscribe > >> linux-hwmon" > >>>>> in > >>>>> the body of a message to majordomo@xxxxxxxxxxxxxxx > >>>>> More majordomo info at > >> http://vger.kernel.org/majordomo-info.html > > ��.n��������+%������w��{.n�����{�� &��^n�r������&��z�ޗ�zf���h���~����������_��+v���)ߣ�
