| Message ID | 1466728108-2512-1-git-send-email-linux@roeck-us.net (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
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?
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
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
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.
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. >
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
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.
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
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 --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, };
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(-)