| Message ID | 20180712010450.10586-4-ikegami@allied-telesis.co.jp (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
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@allied-telesis.co.jp> > Cc: Chris Packham <chris.packham@alliedtelesis.co.nz> > Cc: linux-hwmon@vger.kernel.org > --- > 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. Anyway I will do send v2 version patches later. Regards, Ikegami > -----Original Message----- > From: Guenter Roeck [mailto:groeck7@gmail.com] On Behalf Of Guenter > Roeck > Sent: Thursday, July 12, 2018 10:50 AM > To: IKEGAMI Tokunori; Jean Delvare > Cc: PACKHAM Chris; linux-hwmon@vger.kernel.org > 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@allied-telesis.co.jp> > > Cc: Chris Packham <chris.packham@alliedtelesis.co.nz> > > Cc: linux-hwmon@vger.kernel.org > > --- > > 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 > >
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@gmail.com] On Behalf Of Guenter >> Roeck >> Sent: Thursday, July 12, 2018 10:50 AM >> To: IKEGAMI Tokunori; Jean Delvare >> Cc: PACKHAM Chris; linux-hwmon@vger.kernel.org >> 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@allied-telesis.co.jp> >>> Cc: Chris Packham <chris.packham@alliedtelesis.co.nz> >>> Cc: linux-hwmon@vger.kernel.org >>> --- >>> 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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@vger.kernel.org >> [mailto:linux-hwmon-owner@vger.kernel.org] On Behalf Of Guenter Roeck >> Sent: Thursday, July 12, 2018 12:42 PM >> To: IKEGAMI Tokunori; Jean Delvare >> Cc: PACKHAM Chris; linux-hwmon@vger.kernel.org >> 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@gmail.com] On Behalf Of Guenter >>>> Roeck >>>> Sent: Thursday, July 12, 2018 10:50 AM >>>> To: IKEGAMI Tokunori; Jean Delvare >>>> Cc: PACKHAM Chris; linux-hwmon@vger.kernel.org >>>> 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@allied-telesis.co.jp> >>>>> Cc: Chris Packham <chris.packham@alliedtelesis.co.nz> >>>>> Cc: linux-hwmon@vger.kernel.org >>>>> --- >>>>> 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@vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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@vger.kernel.org >> 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@gmail.com] On Behalf Of Guenter >>> Roeck >>> Sent: Thursday, July 12, 2018 1:23 PM >>> To: IKEGAMI Tokunori; Jean Delvare >>> Cc: PACKHAM Chris; linux-hwmon@vger.kernel.org >>> 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@vger.kernel.org >>>>> [mailto:linux-hwmon-owner@vger.kernel.org] On Behalf Of Guenter >>> Roeck >>>>> Sent: Thursday, July 12, 2018 12:42 PM >>>>> To: IKEGAMI Tokunori; Jean Delvare >>>>> Cc: PACKHAM Chris; linux-hwmon@vger.kernel.org >>>>> 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@gmail.com] On Behalf Of >> Guenter >>>>>>> Roeck >>>>>>> Sent: Thursday, July 12, 2018 10:50 AM >>>>>>> To: IKEGAMI Tokunori; Jean Delvare >>>>>>> Cc: PACKHAM Chris; linux-hwmon@vger.kernel.org >>>>>>> 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@allied-telesis.co.jp> >>>>>>>> Cc: Chris Packham <chris.packham@alliedtelesis.co.nz> >>>>>>>> Cc: linux-hwmon@vger.kernel.org >>>>>>>> --- >>>>>>>> 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@vger.kernel.org >>>>> More majordomo info at >> http://vger.kernel.org/majordomo-info.html > -- To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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@gmail.com] On Behalf Of Guenter > Roeck > Sent: Thursday, July 12, 2018 11:06 PM > To: IKEGAMI Tokunori; Jean Delvare > Cc: PACKHAM Chris; linux-hwmon@vger.kernel.org > 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@vger.kernel.org > >> 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@gmail.com] On Behalf Of Guenter > >>> Roeck > >>> Sent: Thursday, July 12, 2018 1:23 PM > >>> To: IKEGAMI Tokunori; Jean Delvare > >>> Cc: PACKHAM Chris; linux-hwmon@vger.kernel.org > >>> 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@vger.kernel.org > >>>>> [mailto:linux-hwmon-owner@vger.kernel.org] On Behalf Of Guenter > >>> Roeck > >>>>> Sent: Thursday, July 12, 2018 12:42 PM > >>>>> To: IKEGAMI Tokunori; Jean Delvare > >>>>> Cc: PACKHAM Chris; linux-hwmon@vger.kernel.org > >>>>> 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@gmail.com] On Behalf Of > >> Guenter > >>>>>>> Roeck > >>>>>>> Sent: Thursday, July 12, 2018 10:50 AM > >>>>>>> To: IKEGAMI Tokunori; Jean Delvare > >>>>>>> Cc: PACKHAM Chris; linux-hwmon@vger.kernel.org > >>>>>>> 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@allied-telesis.co.jp> > >>>>>>>> Cc: Chris Packham <chris.packham@alliedtelesis.co.nz> > >>>>>>>> Cc: linux-hwmon@vger.kernel.org > >>>>>>>> --- > >>>>>>>> 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@vger.kernel.org > >>>>> More majordomo info at > >> http://vger.kernel.org/majordomo-info.html > >
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
It shoudl be same as with other functions to use adt7475_read. So change to use it instead of i2c_smbus_read_byte_data. Signed-off-by: Tokunori Ikegami <ikegami@allied-telesis.co.jp> Cc: Chris Packham <chris.packham@alliedtelesis.co.nz> Cc: linux-hwmon@vger.kernel.org --- drivers/hwmon/adt7475.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)