Re: [PATCH 3/5] hwmon: adt7475: Change to use adt7475_read macro

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

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