diff mbox

[1/5] hwmon: (tmp102) Use devm_add_action to register cleanup function

Message ID 1466728108-2512-1-git-send-email-linux@roeck-us.net
State Superseded
Headers show

Commit Message

Guenter Roeck June 24, 2016, 12:28 a.m. UTC
By registering a cleanup function with devm_add_action(), we can
simplify the error path in the probe function and drop the remove
function entirely.

Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
 drivers/hwmon/tmp102.c | 54 ++++++++++++++++++--------------------------------
 1 file changed, 19 insertions(+), 35 deletions(-)

Comments

Nishanth Menon June 24, 2016, 2:13 p.m. UTC | #1
On 06/23/2016 07:28 PM, Guenter Roeck wrote:
> By registering a cleanup function with devm_add_action(), we can
> simplify the error path in the probe function and drop the remove
> function entirely.
>
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>

I dont seem to have a cover letter to reply to... but anyways..

Before: http://pastebin.ubuntu.com/17801376/
After all 5 patches: http://pastebin.ubuntu.com/17801824/

Fails on beagleboard-X15 with:
[   36.781509] tmp102 0-0048: No cache defaults, reading back from HW
[   36.795940] tmp102 0-0048: unexpected config register value

I have'nt bisected down on the specific patch in the series. Have you 
had a chance to test the series?
Guenter Roeck June 24, 2016, 2:30 p.m. UTC | #2
Hi Nishanth,

On 06/24/2016 07:13 AM, Nishanth Menon wrote:
> On 06/23/2016 07:28 PM, Guenter Roeck wrote:
>> By registering a cleanup function with devm_add_action(), we can
>> simplify the error path in the probe function and drop the remove
>> function entirely.
>>
>> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
>
> I dont seem to have a cover letter to reply to... but anyways..
>
> Before: http://pastebin.ubuntu.com/17801376/
> After all 5 patches: http://pastebin.ubuntu.com/17801824/
>
> Fails on beagleboard-X15 with:
> [   36.781509] tmp102 0-0048: No cache defaults, reading back from HW
> [   36.795940] tmp102 0-0048: unexpected config register value
>
> I have'nt bisected down on the specific patch in the series. Have you had a chance to test the series?
>
>

Thanks for testing. Yes, I did test it. Maybe different chip revisions, or different
initial config register values and I messed up there. Can you send me the output
of i2cdump (word wide) ?

Thanks,
Guenter

--
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
Guenter Roeck June 24, 2016, 2:54 p.m. UTC | #3
On 06/24/2016 07:30 AM, Guenter Roeck wrote:
> Hi Nishanth,
>
> On 06/24/2016 07:13 AM, Nishanth Menon wrote:
>> On 06/23/2016 07:28 PM, Guenter Roeck wrote:
>>> By registering a cleanup function with devm_add_action(), we can
>>> simplify the error path in the probe function and drop the remove
>>> function entirely.
>>>
>>> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
>>
>> I dont seem to have a cover letter to reply to... but anyways..
>>
>> Before: http://pastebin.ubuntu.com/17801376/
>> After all 5 patches: http://pastebin.ubuntu.com/17801824/
>>
>> Fails on beagleboard-X15 with:
>> [   36.781509] tmp102 0-0048: No cache defaults, reading back from HW
>> [   36.795940] tmp102 0-0048: unexpected config register value
>>
>> I have'nt bisected down on the specific patch in the series. Have you had a chance to test the series?
>>
>>
>
> Thanks for testing. Yes, I did test it. Maybe different chip revisions, or different
> initial config register values and I messed up there. Can you send me the output
> of i2cdump (word wide) ?
>

Also, beagleboard uses the omap i2c controller, correct ? I'll have to check
if regmap handles endianness conversion properly for controllers supporting
I2C_FUNC_I2C. If some other i2c controller is used, please let me know (or
sene me the output of i2cdetect -l). Recent kernels have a bug in regmap
which affects chips with 16-bit registers if I2C_FUNC_I2C is not supported.

Maybe I should get a beagleboard. The X15 isn't available yet, though. Do the
available boards (eg Rev C) use the same i2c controller ?

Thanks,
Guenter

--
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
Nishanth Menon June 24, 2016, 3:23 p.m. UTC | #4
On 06/24/2016 09:54 AM, Guenter Roeck wrote:
> On 06/24/2016 07:30 AM, Guenter Roeck wrote:
>> Hi Nishanth,
>>
>> On 06/24/2016 07:13 AM, Nishanth Menon wrote:
>>> On 06/23/2016 07:28 PM, Guenter Roeck wrote:
>>>> By registering a cleanup function with devm_add_action(), we can
>>>> simplify the error path in the probe function and drop the remove
>>>> function entirely.
>>>>
>>>> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
>>>
>>> I dont seem to have a cover letter to reply to... but anyways..
>>>
>>> Before: http://pastebin.ubuntu.com/17801376/
>>> After all 5 patches: http://pastebin.ubuntu.com/17801824/
>>>
>>> Fails on beagleboard-X15 with:
>>> [   36.781509] tmp102 0-0048: No cache defaults, reading back from HW
>>> [   36.795940] tmp102 0-0048: unexpected config register value
>>>
>>> I have'nt bisected down on the specific patch in the series. Have you had a chance to test the series?
>>>
>>>
>>
>> Thanks for testing. Yes, I did test it. Maybe different chip revisions, or different
>> initial config register values and I messed up there. Can you send me the output
>> of i2cdump (word wide) ?
>>
> 
> Before 5 patches:
>> # i2cdump -f 0 0x48 w
>>      0,8  1,9  2,a  3,b  4,c  5,d  6,e  7,f
>> 00: 7912 b062 004b 0050 c018 e006 0000 0000 
>> 08: 7912 b062 004b 0050 c018 e006 0000 0000 
>> 10: 7912 b062 004b 0050 c018 e006 0000 0000 
>> 18: 7912 b062 004b 0050 c018 e006 0000 0000 
>> 20: 7912 b062 004b 0050 c018 e006 0000 0000 
>> 28: 7912 b062 004b 0050 c018 e006 0000 0000 
>> 30: 7912 b062 004b 0050 c018 e006 0000 0000 
>> 38: 7912 b062 004b 0050 c018 e006 0000 0000 
>> 40: 7912 b062 004b 0050 c018 e006 0000 0000 
>> 48: 7912 b062 004b 0050 c018 e006 0000 0000 
>> 50: 7912 b062 004b 0050 c018 e006 0000 0000 
>> 58: 7912 b062 004b 0050 c018 e006 0000 0000 
>> 60: 7912 b062 004b 0050 c018 e006 0000 0000 
>> 68: 7912 b062 004b 0050 c018 e006 0000 0000 
>> 70: 7912 b062 004b 0050 c018 e006 0000 0000 
>> 78: 7912 b062 004b 0050 c018 e006 0000 0000 
>> 80: 7912 b062 004b 0050 c018 e006 0000 0000 
>> 88: 7912 b062 004b 0050 c018 e006 0000 0000 
>> 90: 7912 b062 004b 0050 c018 e006 0000 0000 
>> 98: 7912 b062 004b 0050 c018 e006 0000 0000 
>> a0: 7912 b062 004b 0050 c018 e006 0000 0000 
>> a8: 7912 b062 004b 0050 c018 e006 0000 0000 
>> b0: 7912 b062 004b 0050 c018 e006 0000 0000 
>> b8: 7912 b062 004b 0050 c018 e006 0000 0000 
>> c0: 7912 b062 004b 0050 c018 e006 0000 0000 
>> c8: 7912 b062 004b 0050 c018 e006 0000 0000 
>> d0: 7912 b062 004b 0050 c018 e006 0000 0000 
>> d8: 7912 b062 004b 0050 c018 e006 0000 0000 
>> e0: 7912 b062 004b 0050 c018 e006 0000 0000 
>> e8: 7912 b062 004b 0050 c018 e006 0000 0000 
>> f0: 7912 b062 004b 0050 c018 e006 0000 0000 
>> f8: 7912 b062 004b 0050 c018 e006 0000 0000 
> 
> After 5 patches:
>>  i2cdump -y 0 0x48 w
>>      0,8  1,9  2,a  3,b  4,c  5,d  6,e  7,f
>> 00: 5024 a060 004b 0050 c018 e006 0000 0000 
>> 08: 5024 a060 004b 0050 c018 e006 0000 0000 
>> 10: 5024 a060 004b 0050 c018 e006 0000 0000 
>> 18: 5024 a060 004b 0050 c018 e006 0000 0000 
>> 20: 5024 a060 004b 0050 c018 e006 0000 0000 
>> 28: 5024 a060 004b 0050 c018 e006 0000 0000 
>> 30: 5024 a060 004b 0050 c018 e006 0000 0000 
>> 38: 5024 a060 004b 0050 c018 e006 0000 0000 
>> 40: 5024 a060 004b 0050 c018 e006 0000 0000 
>> 48: 5024 a060 004b 0050 c018 e006 0000 0000 
>> 50: 5024 a060 004b 0050 c018 e006 0000 0000 
>> 58: 5024 a060 004b 0050 c018 e006 0000 0000 
>> 60: 5024 a060 004b 0050 c018 e006 0000 0000 
>> 68: 5024 a060 004b 0050 c018 e006 0000 0000 
>> 70: 5024 a060 004b 0050 c018 e006 0000 0000 
>> 78: 5024 a060 004b 0050 c018 e006 0000 0000 
>> 80: 5024 a060 004b 0050 c018 e006 0000 0000 
>> 88: 5024 a060 004b 0050 c018 e006 0000 0000 
>> 90: 5024 a060 004b 0050 c018 e006 0000 0000 
>> 98: 5024 a060 004b 0050 c018 e006 0000 0000 
>> a0: 5024 a060 004b 0050 c018 e006 0000 0000 
>> a8: 5024 a060 004b 0050 c018 e006 0000 0000 
>> b0: 5024 a060 004b 0050 c018 e006 0000 0000 
>> b8: 5024 a060 004b 0050 c018 e006 0000 0000 
>> c0: 5024 a060 004b 0050 c018 e006 0000 0000 
>> c8: 5024 a060 004b 0050 c018 e006 0000 0000 
>> d0: 5024 a060 004b 0050 c018 e006 0000 0000 
>> d8: 5024 a060 004b 0050 c018 e006 0000 0000 
>> e0: 5024 a060 004b 0050 c018 e006 0000 0000 
>> e8: 5024 a060 004b 0050 c018 e006 0000 0000 
>> f0: 5024 a060 004b 0050 c018 e006 0000 0000 
>> f8: 5024 a060 004b 0050 c018 e006 0000 0000 
> 



> Also, beagleboard uses the omap i2c controller, correct ? I'll have to check
> if regmap handles endianness conversion properly for controllers supporting
> I2C_FUNC_I2C. If some other i2c controller is used, please let me know (or
> sene me the output of i2cdetect -l). Recent kernels have a bug in regmap
> which affects chips with 16-bit registers if I2C_FUNC_I2C is not supported.
> 
> Maybe I should get a beagleboard. The X15 isn't available yet, though. Do the
> available boards (eg Rev C) use the same i2c controller ?

yeah there are variants of omap-i2c controllers, but basically they
work in the same way.

I can try and debug the series once I get some spare time, might be
over the weekend or next week.
Nishanth Menon June 24, 2016, 3:31 p.m. UTC | #5
On 06/24/2016 10:23 AM, Nishanth Menon wrote:
> On 06/24/2016 09:54 AM, Guenter Roeck wrote:
>> On 06/24/2016 07:30 AM, Guenter Roeck wrote:
>>> Hi Nishanth,
>>>
>>> On 06/24/2016 07:13 AM, Nishanth Menon wrote:
>>>> On 06/23/2016 07:28 PM, Guenter Roeck wrote:
>>>>> By registering a cleanup function with devm_add_action(), we can
>>>>> simplify the error path in the probe function and drop the remove
>>>>> function entirely.
>>>>>
>>>>> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
>>>>
>>>> I dont seem to have a cover letter to reply to... but anyways..
>>>>
>>>> Before: http://pastebin.ubuntu.com/17801376/
>>>> After all 5 patches: http://pastebin.ubuntu.com/17801824/
>>>>
>>>> Fails on beagleboard-X15 with:
>>>> [   36.781509] tmp102 0-0048: No cache defaults, reading back from HW
>>>> [   36.795940] tmp102 0-0048: unexpected config register value
>>>>
>>>> I have'nt bisected down on the specific patch in the series. Have you had a chance to test the series?
>>>>
>>>>
>>>
>>> Thanks for testing. Yes, I did test it. Maybe different chip revisions, or different
>>> initial config register values and I messed up there. Can you send me the output
>>> of i2cdump (word wide) ?
>>>
>>
>> Before 5 patches:
>>> # i2cdump -f 0 0x48 w
>>>      0,8  1,9  2,a  3,b  4,c  5,d  6,e  7,f
>>> 00: 7912 b062 004b 0050 c018 e006 0000 0000 
>>> 08: 7912 b062 004b 0050 c018 e006 0000 0000 
>>> 10: 7912 b062 004b 0050 c018 e006 0000 0000 
>>> 18: 7912 b062 004b 0050 c018 e006 0000 0000 
>>> 20: 7912 b062 004b 0050 c018 e006 0000 0000 
>>> 28: 7912 b062 004b 0050 c018 e006 0000 0000 
>>> 30: 7912 b062 004b 0050 c018 e006 0000 0000 
>>> 38: 7912 b062 004b 0050 c018 e006 0000 0000 
>>> 40: 7912 b062 004b 0050 c018 e006 0000 0000 
>>> 48: 7912 b062 004b 0050 c018 e006 0000 0000 
>>> 50: 7912 b062 004b 0050 c018 e006 0000 0000 
>>> 58: 7912 b062 004b 0050 c018 e006 0000 0000 
>>> 60: 7912 b062 004b 0050 c018 e006 0000 0000 
>>> 68: 7912 b062 004b 0050 c018 e006 0000 0000 
>>> 70: 7912 b062 004b 0050 c018 e006 0000 0000 
>>> 78: 7912 b062 004b 0050 c018 e006 0000 0000 
>>> 80: 7912 b062 004b 0050 c018 e006 0000 0000 
>>> 88: 7912 b062 004b 0050 c018 e006 0000 0000 
>>> 90: 7912 b062 004b 0050 c018 e006 0000 0000 
>>> 98: 7912 b062 004b 0050 c018 e006 0000 0000 
>>> a0: 7912 b062 004b 0050 c018 e006 0000 0000 
>>> a8: 7912 b062 004b 0050 c018 e006 0000 0000 
>>> b0: 7912 b062 004b 0050 c018 e006 0000 0000 
>>> b8: 7912 b062 004b 0050 c018 e006 0000 0000 
>>> c0: 7912 b062 004b 0050 c018 e006 0000 0000 
>>> c8: 7912 b062 004b 0050 c018 e006 0000 0000 
>>> d0: 7912 b062 004b 0050 c018 e006 0000 0000 
>>> d8: 7912 b062 004b 0050 c018 e006 0000 0000 
>>> e0: 7912 b062 004b 0050 c018 e006 0000 0000 
>>> e8: 7912 b062 004b 0050 c018 e006 0000 0000 
>>> f0: 7912 b062 004b 0050 c018 e006 0000 0000 
>>> f8: 7912 b062 004b 0050 c018 e006 0000 0000 
>>
>> After 5 patches:
>>>  i2cdump -y 0 0x48 w
>>>      0,8  1,9  2,a  3,b  4,c  5,d  6,e  7,f
>>> 00: 5024 a060 004b 0050 c018 e006 0000 0000 
>>> 08: 5024 a060 004b 0050 c018 e006 0000 0000 
>>> 10: 5024 a060 004b 0050 c018 e006 0000 0000 
>>> 18: 5024 a060 004b 0050 c018 e006 0000 0000 
>>> 20: 5024 a060 004b 0050 c018 e006 0000 0000 
>>> 28: 5024 a060 004b 0050 c018 e006 0000 0000 
>>> 30: 5024 a060 004b 0050 c018 e006 0000 0000 
>>> 38: 5024 a060 004b 0050 c018 e006 0000 0000 
>>> 40: 5024 a060 004b 0050 c018 e006 0000 0000 
>>> 48: 5024 a060 004b 0050 c018 e006 0000 0000 
>>> 50: 5024 a060 004b 0050 c018 e006 0000 0000 
>>> 58: 5024 a060 004b 0050 c018 e006 0000 0000 
>>> 60: 5024 a060 004b 0050 c018 e006 0000 0000 
>>> 68: 5024 a060 004b 0050 c018 e006 0000 0000 
>>> 70: 5024 a060 004b 0050 c018 e006 0000 0000 
>>> 78: 5024 a060 004b 0050 c018 e006 0000 0000 
>>> 80: 5024 a060 004b 0050 c018 e006 0000 0000 
>>> 88: 5024 a060 004b 0050 c018 e006 0000 0000 
>>> 90: 5024 a060 004b 0050 c018 e006 0000 0000 
>>> 98: 5024 a060 004b 0050 c018 e006 0000 0000 
>>> a0: 5024 a060 004b 0050 c018 e006 0000 0000 
>>> a8: 5024 a060 004b 0050 c018 e006 0000 0000 
>>> b0: 5024 a060 004b 0050 c018 e006 0000 0000 
>>> b8: 5024 a060 004b 0050 c018 e006 0000 0000 
>>> c0: 5024 a060 004b 0050 c018 e006 0000 0000 
>>> c8: 5024 a060 004b 0050 c018 e006 0000 0000 
>>> d0: 5024 a060 004b 0050 c018 e006 0000 0000 
>>> d8: 5024 a060 004b 0050 c018 e006 0000 0000 
>>> e0: 5024 a060 004b 0050 c018 e006 0000 0000 
>>> e8: 5024 a060 004b 0050 c018 e006 0000 0000 
>>> f0: 5024 a060 004b 0050 c018 e006 0000 0000 
>>> f8: 5024 a060 004b 0050 c018 e006 0000 0000 
>>
> 
> 
> 
>> Also, beagleboard uses the omap i2c controller, correct ? I'll have to check
>> if regmap handles endianness conversion properly for controllers supporting
>> I2C_FUNC_I2C. If some other i2c controller is used, please let me know (or
>> sene me the output of i2cdetect -l). Recent kernels have a bug in regmap
>> which affects chips with 16-bit registers if I2C_FUNC_I2C is not supported.
Forgot to send you -l output:
root@BeagleBoard-X15:~# i2cdetect -l
i2c-0   i2c             OMAP I2C adapter                        I2C
adapter
i2c-2   i2c             OMAP I2C adapter                        I2C
adapter
root@BeagleBoard-X15:~# i2cdetect -F 0
Functionalities implemented by /dev/i2c-0:
I2C                              yes
SMBus Quick Command              no
SMBus Send Byte                  yes
SMBus Receive Byte               yes
SMBus Write Byte                 yes
SMBus Read Byte                  yes
SMBus Write Word                 yes
SMBus Read Word                  yes
SMBus Process Call               yes
SMBus Block Write                yes
SMBus Block Read                 no
SMBus Block Process Call         no
SMBus PEC                        yes
I2C Block Write                  yes
I2C Block Read                   yes


>>
>> Maybe I should get a beagleboard. The X15 isn't available yet, though. Do the
>> available boards (eg Rev C) use the same i2c controller ?
> 
> yeah there are variants of omap-i2c controllers, but basically they
> work in the same way.
> 
> I can try and debug the series once I get some spare time, might be
> over the weekend or next week.
>
Guenter Roeck June 24, 2016, 4:36 p.m. UTC | #6
On Fri, Jun 24, 2016 at 10:23:10AM -0500, Nishanth Menon wrote:
> On 06/24/2016 09:54 AM, Guenter Roeck wrote:
> > On 06/24/2016 07:30 AM, Guenter Roeck wrote:
> >> Hi Nishanth,
> >>
> >> On 06/24/2016 07:13 AM, Nishanth Menon wrote:
> >>> On 06/23/2016 07:28 PM, Guenter Roeck wrote:
> >>>> By registering a cleanup function with devm_add_action(), we can
> >>>> simplify the error path in the probe function and drop the remove
> >>>> function entirely.
> >>>>
> >>>> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> >>>
> >>> I dont seem to have a cover letter to reply to... but anyways..
> >>>
> >>> Before: http://pastebin.ubuntu.com/17801376/
> >>> After all 5 patches: http://pastebin.ubuntu.com/17801824/
> >>>
> >>> Fails on beagleboard-X15 with:
> >>> [   36.781509] tmp102 0-0048: No cache defaults, reading back from HW
> >>> [   36.795940] tmp102 0-0048: unexpected config register value
> >>>
> >>> I have'nt bisected down on the specific patch in the series. Have you had a chance to test the series?
> >>>
> >>>
> >>
> >> Thanks for testing. Yes, I did test it. Maybe different chip revisions, or different
> >> initial config register values and I messed up there. Can you send me the output
> >> of i2cdump (word wide) ?
> >>
> > 
> > Before 5 patches:
> >> # i2cdump -f 0 0x48 w
> >>      0,8  1,9  2,a  3,b  4,c  5,d  6,e  7,f
> >> 00: 7912 b062 004b 0050 c018 e006 0000 0000 

[ ... ]
> > 
> > After 5 patches:
> >>  i2cdump -y 0 0x48 w
> >>      0,8  1,9  2,a  3,b  4,c  5,d  6,e  7,f
> >> 00: 5024 a060 004b 0050 c018 e006 0000 0000 

[ .... ]

> I can try and debug the series once I get some spare time, might be
> over the weekend or next week.

The register map, at least the initial one, is pretty much the same as mine
and as expected. The configuration register in the second map is messed up,
possible because of a write with wrong endianness.

I bet the regmap_read() of the configuration register returns 0xa060 (or
0xb062) instead of 0x60a0 / 0x62b0 on your system. If you find the time,
it would be great if you can confirm by printing the register value with
the "unexpected config register value" message (guess I should have left
that in place - I used it during testing ;-).

If that is the case, I'll probably have to drop the regmap changes, at least
for now. It would mean that regmap is broken for chips like the LM75 (ie
for all chips with 16-bit registers) on controllers supporting I2C_FUNC_I2C.

Thanks,
Guenter
--
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
Nishanth Menon June 24, 2016, 6:02 p.m. UTC | #7
On 06/24/2016 11:36 AM, Guenter Roeck wrote:
> On Fri, Jun 24, 2016 at 10:23:10AM -0500, Nishanth Menon wrote:
>> On 06/24/2016 09:54 AM, Guenter Roeck wrote:
>>> On 06/24/2016 07:30 AM, Guenter Roeck wrote:
>>>> Hi Nishanth,
>>>>
>>>> On 06/24/2016 07:13 AM, Nishanth Menon wrote:
>>>>> On 06/23/2016 07:28 PM, Guenter Roeck wrote:
>>>>>> By registering a cleanup function with devm_add_action(), we can
>>>>>> simplify the error path in the probe function and drop the remove
>>>>>> function entirely.
>>>>>>
>>>>>> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
>>>>>
>>>>> I dont seem to have a cover letter to reply to... but anyways..
>>>>>
>>>>> Before: http://pastebin.ubuntu.com/17801376/
>>>>> After all 5 patches: http://pastebin.ubuntu.com/17801824/
>>>>>
>>>>> Fails on beagleboard-X15 with:
>>>>> [   36.781509] tmp102 0-0048: No cache defaults, reading back from HW
>>>>> [   36.795940] tmp102 0-0048: unexpected config register value
>>>>>
>>>>> I have'nt bisected down on the specific patch in the series. Have you had a chance to test the series?
>>>>>
>>>>>
>>>>
>>>> Thanks for testing. Yes, I did test it. Maybe different chip revisions, or different
>>>> initial config register values and I messed up there. Can you send me the output
>>>> of i2cdump (word wide) ?
>>>>
>>>
>>> Before 5 patches:
>>>> # i2cdump -f 0 0x48 w
>>>>      0,8  1,9  2,a  3,b  4,c  5,d  6,e  7,f
>>>> 00: 7912 b062 004b 0050 c018 e006 0000 0000 
> 
> [ ... ]
>>>
>>> After 5 patches:
>>>>  i2cdump -y 0 0x48 w
>>>>      0,8  1,9  2,a  3,b  4,c  5,d  6,e  7,f
>>>> 00: 5024 a060 004b 0050 c018 e006 0000 0000 
> 
> [ .... ]
> 
>> I can try and debug the series once I get some spare time, might be
>> over the weekend or next week.
> 
> The register map, at least the initial one, is pretty much the same as mine
> and as expected. The configuration register in the second map is messed up,
> possible because of a write with wrong endianness.

Got a few mins skipping lunch.. ;)

I did a quick bisect, and it is indeed the patch #5 that breaks.
added http://pastebin.ubuntu.com/17812044/ and got:

tmp102 0-0048: regval = 0x0000ffff

That was weird. Does'nt look like endian-ness swap thingy

So, did the following hack to see all messages flowing in and out from
0x48 at bus controller driver level: http://pastebin.ubuntu.com/17813093/
/ # dmesg|grep XXX
/ #

Before patch #5 (and on next-20160624):
the same diff gave:
http://pastebin.ubuntu.com/17813303/



> I bet the regmap_read() of the configuration register returns 0xa060 (or
> 0xb062) instead of 0x60a0 / 0x62b0 on your system. If you find the time,
> it would be great if you can confirm by printing the register value with
> the "unexpected config register value" message (guess I should have left
> that in place - I used it during testing ;-).
> 
> If that is the case, I'll probably have to drop the regmap changes, at least
> for now. It would mean that regmap is broken for chips like the LM75 (ie
> for all chips with 16-bit registers) on controllers supporting I2C_FUNC_I2C.

It does look like everything is getting cached out and no actual reads
are actually getting through to the bus controller driver even.

I tested on next-20160624 and used omap2plus_defconfig for the test.
Guenter Roeck June 24, 2016, 6:18 p.m. UTC | #8
On Fri, Jun 24, 2016 at 01:02:32PM -0500, Nishanth Menon wrote:
> On 06/24/2016 11:36 AM, Guenter Roeck wrote:
> > On Fri, Jun 24, 2016 at 10:23:10AM -0500, Nishanth Menon wrote:
> >> On 06/24/2016 09:54 AM, Guenter Roeck wrote:
> >>> On 06/24/2016 07:30 AM, Guenter Roeck wrote:
> >>>> Hi Nishanth,
> >>>>
> >>>> On 06/24/2016 07:13 AM, Nishanth Menon wrote:
> >>>>> On 06/23/2016 07:28 PM, Guenter Roeck wrote:
> >>>>>> By registering a cleanup function with devm_add_action(), we can
> >>>>>> simplify the error path in the probe function and drop the remove
> >>>>>> function entirely.
> >>>>>>
> >>>>>> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> >>>>>
> >>>>> I dont seem to have a cover letter to reply to... but anyways..
> >>>>>
> >>>>> Before: http://pastebin.ubuntu.com/17801376/
> >>>>> After all 5 patches: http://pastebin.ubuntu.com/17801824/
> >>>>>
> >>>>> Fails on beagleboard-X15 with:
> >>>>> [   36.781509] tmp102 0-0048: No cache defaults, reading back from HW
> >>>>> [   36.795940] tmp102 0-0048: unexpected config register value
> >>>>>
> >>>>> I have'nt bisected down on the specific patch in the series. Have you had a chance to test the series?
> >>>>>
> >>>>>
> >>>>
> >>>> Thanks for testing. Yes, I did test it. Maybe different chip revisions, or different
> >>>> initial config register values and I messed up there. Can you send me the output
> >>>> of i2cdump (word wide) ?
> >>>>
> >>>
> >>> Before 5 patches:
> >>>> # i2cdump -f 0 0x48 w
> >>>>      0,8  1,9  2,a  3,b  4,c  5,d  6,e  7,f
> >>>> 00: 7912 b062 004b 0050 c018 e006 0000 0000 
> > 
> > [ ... ]
> >>>
> >>> After 5 patches:
> >>>>  i2cdump -y 0 0x48 w
> >>>>      0,8  1,9  2,a  3,b  4,c  5,d  6,e  7,f
> >>>> 00: 5024 a060 004b 0050 c018 e006 0000 0000 
> > 
> > [ .... ]
> > 
> >> I can try and debug the series once I get some spare time, might be
> >> over the weekend or next week.
> > 
> > The register map, at least the initial one, is pretty much the same as mine
> > and as expected. The configuration register in the second map is messed up,
> > possible because of a write with wrong endianness.
> 
> Got a few mins skipping lunch.. ;)
> 
> I did a quick bisect, and it is indeed the patch #5 that breaks.
> added http://pastebin.ubuntu.com/17812044/ and got:
> 
> tmp102 0-0048: regval = 0x0000ffff
> 
> That was weird. Does'nt look like endian-ness swap thingy
> 
> So, did the following hack to see all messages flowing in and out from
> 0x48 at bus controller driver level: http://pastebin.ubuntu.com/17813093/
> / # dmesg|grep XXX
> / #
> 
> Before patch #5 (and on next-20160624):
> the same diff gave:
> http://pastebin.ubuntu.com/17813303/
> 
> 
> 
> > I bet the regmap_read() of the configuration register returns 0xa060 (or
> > 0xb062) instead of 0x60a0 / 0x62b0 on your system. If you find the time,
> > it would be great if you can confirm by printing the register value with
> > the "unexpected config register value" message (guess I should have left
> > that in place - I used it during testing ;-).
> > 
> > If that is the case, I'll probably have to drop the regmap changes, at least
> > for now. It would mean that regmap is broken for chips like the LM75 (ie
> > for all chips with 16-bit registers) on controllers supporting I2C_FUNC_I2C.
> 
> It does look like everything is getting cached out and no actual reads
> are actually getting through to the bus controller driver even.
> 
Yes, that is really weird. It also seems to fill the cache with 0xffff,
which is even more weird.

Ok, looks like converting drivers to regmap isn't a good idea. I'll need
to get a system with an adapter supporting I2C_FUNC_I2C and do some more
testing.

> I tested on next-20160624 and used omap2plus_defconfig for the test.
> 

Thanks a lot for the information, and for testing this with your system.

Guenter
--
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
Nishanth Menon June 24, 2016, 6:21 p.m. UTC | #9
On 06/24/2016 01:18 PM, Guenter Roeck wrote:
> On Fri, Jun 24, 2016 at 01:02:32PM -0500, Nishanth Menon wrote:
>> On 06/24/2016 11:36 AM, Guenter Roeck wrote:
>>> On Fri, Jun 24, 2016 at 10:23:10AM -0500, Nishanth Menon wrote:
>>>> On 06/24/2016 09:54 AM, Guenter Roeck wrote:
>>>>> On 06/24/2016 07:30 AM, Guenter Roeck wrote:
>>>>>> Hi Nishanth,
>>>>>>
>>>>>> On 06/24/2016 07:13 AM, Nishanth Menon wrote:
>>>>>>> On 06/23/2016 07:28 PM, Guenter Roeck wrote:
>>>>>>>> By registering a cleanup function with devm_add_action(), we can
>>>>>>>> simplify the error path in the probe function and drop the remove
>>>>>>>> function entirely.
>>>>>>>>
>>>>>>>> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
>>>>>>>
>>>>>>> I dont seem to have a cover letter to reply to... but anyways..
>>>>>>>
>>>>>>> Before: http://pastebin.ubuntu.com/17801376/
>>>>>>> After all 5 patches: http://pastebin.ubuntu.com/17801824/
>>>>>>>
>>>>>>> Fails on beagleboard-X15 with:
>>>>>>> [   36.781509] tmp102 0-0048: No cache defaults, reading back from HW
>>>>>>> [   36.795940] tmp102 0-0048: unexpected config register value
>>>>>>>
>>>>>>> I have'nt bisected down on the specific patch in the series. Have you had a chance to test the series?
>>>>>>>
>>>>>>>
>>>>>>
>>>>>> Thanks for testing. Yes, I did test it. Maybe different chip revisions, or different
>>>>>> initial config register values and I messed up there. Can you send me the output
>>>>>> of i2cdump (word wide) ?
>>>>>>
>>>>>
>>>>> Before 5 patches:
>>>>>> # i2cdump -f 0 0x48 w
>>>>>>      0,8  1,9  2,a  3,b  4,c  5,d  6,e  7,f
>>>>>> 00: 7912 b062 004b 0050 c018 e006 0000 0000 
>>>
>>> [ ... ]
>>>>>
>>>>> After 5 patches:
>>>>>>  i2cdump -y 0 0x48 w
>>>>>>      0,8  1,9  2,a  3,b  4,c  5,d  6,e  7,f
>>>>>> 00: 5024 a060 004b 0050 c018 e006 0000 0000 
>>>
>>> [ .... ]
>>>
>>>> I can try and debug the series once I get some spare time, might be
>>>> over the weekend or next week.
>>>
>>> The register map, at least the initial one, is pretty much the same as mine
>>> and as expected. The configuration register in the second map is messed up,
>>> possible because of a write with wrong endianness.
>>
>> Got a few mins skipping lunch.. ;)
>>
>> I did a quick bisect, and it is indeed the patch #5 that breaks.
>> added http://pastebin.ubuntu.com/17812044/ and got:
>>
>> tmp102 0-0048: regval = 0x0000ffff
>>
>> That was weird. Does'nt look like endian-ness swap thingy
>>
>> So, did the following hack to see all messages flowing in and out from
>> 0x48 at bus controller driver level: http://pastebin.ubuntu.com/17813093/
>> / # dmesg|grep XXX
>> / #
>>
>> Before patch #5 (and on next-20160624):
>> the same diff gave:
>> http://pastebin.ubuntu.com/17813303/
>>
>>
>>
>>> I bet the regmap_read() of the configuration register returns 0xa060 (or
>>> 0xb062) instead of 0x60a0 / 0x62b0 on your system. If you find the time,
>>> it would be great if you can confirm by printing the register value with
>>> the "unexpected config register value" message (guess I should have left
>>> that in place - I used it during testing ;-).
>>>
>>> If that is the case, I'll probably have to drop the regmap changes, at least
>>> for now. It would mean that regmap is broken for chips like the LM75 (ie
>>> for all chips with 16-bit registers) on controllers supporting I2C_FUNC_I2C.
>>
>> It does look like everything is getting cached out and no actual reads
>> are actually getting through to the bus controller driver even.
>>
> Yes, that is really weird. It also seems to fill the cache with 0xffff,
> which is even more weird.
> 
> Ok, looks like converting drivers to regmap isn't a good idea. I'll need
> to get a system with an adapter supporting I2C_FUNC_I2C and do some more
> testing.
> 
>> I tested on next-20160624 and used omap2plus_defconfig for the test.
>>
> 
> Thanks a lot for the information, and for testing this with your system.


here is more:
http://pastebin.ubuntu.com/17814261/

Looks like return for is_writable etc should probably be true or false

[   32.609935] at24 0-0050: 4096 byte 24c32 EEPROM, writable, 1
bytes/write
[   32.751560] rtc-ds1307 2-006f: SET TIME!
[   32.857593] palmas_pwrbutton
48070000.i2c:tps659038@58:tps659038_pwr_button: h/w controlled
shutdown duration=12 s
econds
[   33.047265] rtc-ds1307 2-006f: rtc core: registered mcp7941x as rtc0
[   33.135774] tmp102 0-0048: No cache defaults, reading back from HW
[   33.538655] rtc-ds1307 2-006f: 64 bytes nvram
[   34.202107] omap_rng 48090000.rng: _od_fail_runtime_resume: FIXME:
missing hwmod/omap_dev info
[   34.211191] omap_rng 48090000.rng: Failed to runtime_get device: -19
[   34.217869] omap_rng 48090000.rng: initialization failed.
[   34.229190] omap_rtc 48838000.rtc: rtc core: registered
48838000.rtc as rtc2
[   34.371375] omap_i2c 48070000.i2c: XXX MSG[0]: add=0x0048, len: 1,
flags: 0x0
[   34.378893] omap_i2c 48070000.i2c: XXX:[0] 0x00
/ # [   34.457000] omap_i2c 48070000.i2c: XXX MSG[1]: add=0x0048, len:
8, flags: 0x1
[   34.464476] omap_i2c 48070000.i2c: XXX:[0] 0x23
[   34.469255] omap_i2c 48070000.i2c: XXX:[1] 0xa0
[   34.473999] omap_i2c 48070000.i2c: XXX:[2] 0xff
[   34.478775] omap_i2c 48070000.i2c: XXX:[3] 0xff
[   34.483518] omap_i2c 48070000.i2c: XXX:[4] 0xff
[   34.488282] omap_i2c 48070000.i2c: XXX:[5] 0xff
[   34.493022] omap_i2c 48070000.i2c: XXX:[6] 0xff
[   34.497788] omap_i2c 48070000.i2c: XXX:[7] 0xff


I probably have to stop now, since I am behind on an internal
deadline. Do let me know if you want me to dig further, I can try at a
later time..
diff mbox

Patch

diff --git a/drivers/hwmon/tmp102.c b/drivers/hwmon/tmp102.c
index f1e96fd7f445..befd06b6f3a5 100644
--- a/drivers/hwmon/tmp102.c
+++ b/drivers/hwmon/tmp102.c
@@ -52,7 +52,6 @@ 
 
 struct tmp102 {
 	struct i2c_client *client;
-	struct device *hwmon_dev;
 	struct mutex lock;
 	u16 config_orig;
 	unsigned long last_update;
@@ -173,6 +172,15 @@  static const struct thermal_zone_of_device_ops tmp102_of_thermal_ops = {
 	.get_temp = tmp102_read_temp,
 };
 
+static void tmp102_restore_config(void *data)
+{
+	struct tmp102 *tmp102 = data;
+	struct i2c_client *client = tmp102->client;
+
+	i2c_smbus_write_word_swapped(client, TMP102_CONF_REG,
+				     tmp102->config_orig);
+}
+
 static int tmp102_probe(struct i2c_client *client,
 				  const struct i2c_device_id *id)
 {
@@ -201,66 +209,43 @@  static int tmp102_probe(struct i2c_client *client,
 		return status;
 	}
 	tmp102->config_orig = status;
+
+	devm_add_action(dev, tmp102_restore_config, tmp102);
+
 	status = i2c_smbus_write_word_swapped(client, TMP102_CONF_REG,
 					      TMP102_CONFIG);
 	if (status < 0) {
 		dev_err(dev, "error writing config register\n");
-		goto fail_restore_config;
+		return status;
 	}
 	status = i2c_smbus_read_word_swapped(client, TMP102_CONF_REG);
 	if (status < 0) {
 		dev_err(dev, "error reading config register\n");
-		goto fail_restore_config;
+		return status;
 	}
 	status &= ~TMP102_CONFIG_RD_ONLY;
 	if (status != TMP102_CONFIG) {
 		dev_err(dev, "config settings did not stick\n");
-		status = -ENODEV;
-		goto fail_restore_config;
+		return -ENODEV;
 	}
 	tmp102->last_update = jiffies;
 	/* Mark that we are not ready with data until conversion is complete */
 	tmp102->first_time = true;
 	mutex_init(&tmp102->lock);
 
-	hwmon_dev = hwmon_device_register_with_groups(dev, client->name,
-						      tmp102, tmp102_groups);
+	hwmon_dev = devm_hwmon_device_register_with_groups(dev, client->name,
+							   tmp102,
+							   tmp102_groups);
 	if (IS_ERR(hwmon_dev)) {
 		dev_dbg(dev, "unable to register hwmon device\n");
-		status = PTR_ERR(hwmon_dev);
-		goto fail_restore_config;
+		return PTR_ERR(hwmon_dev);
 	}
-	tmp102->hwmon_dev = hwmon_dev;
 	devm_thermal_zone_of_sensor_register(hwmon_dev, 0, hwmon_dev,
 					     &tmp102_of_thermal_ops);
 
 	dev_info(dev, "initialized\n");
 
 	return 0;
-
-fail_restore_config:
-	i2c_smbus_write_word_swapped(client, TMP102_CONF_REG,
-				     tmp102->config_orig);
-	return status;
-}
-
-static int tmp102_remove(struct i2c_client *client)
-{
-	struct tmp102 *tmp102 = i2c_get_clientdata(client);
-
-	hwmon_device_unregister(tmp102->hwmon_dev);
-
-	/* Stop monitoring if device was stopped originally */
-	if (tmp102->config_orig & TMP102_CONF_SD) {
-		int config;
-
-		config = i2c_smbus_read_word_swapped(client, TMP102_CONF_REG);
-		if (config >= 0)
-			i2c_smbus_write_word_swapped(client, TMP102_CONF_REG,
-						     config | TMP102_CONF_SD);
-	}
-
-	return 0;
 }
 
 #ifdef CONFIG_PM_SLEEP
@@ -303,7 +288,6 @@  static struct i2c_driver tmp102_driver = {
 	.driver.name	= DRIVER_NAME,
 	.driver.pm	= &tmp102_dev_pm_ops,
 	.probe		= tmp102_probe,
-	.remove		= tmp102_remove,
 	.id_table	= tmp102_id,
 };