v4.10-rc4 to v4.10-rc5: battery regression on Nokia N900

[Guenter Roeck]

On 01/23/2017 11:06 PM, Zhang Rui wrote:
> On Mon, Jan 23, 2017 at 03:49:12PM -0800, Guenter Roeck wrote:
>> On Tue, Jan 24, 2017 at 12:26:54AM +0100, Pavel Machek wrote:
>>> Hi!
>>>
>>>>>> I'll try to revert it on the top of v4.10-rc5 now... and yes, it fixes
>>>>>> the issue.
>>>>>>
>>>>>> Any idea what went wrong and how to fix that?
>>>>>>
>>>>>> Anyway as we are at -rc5 and this is warning fix that caused a
>>>>>> regression on different hardware... it should be reverted.
>>>>>>
>>>>> Agreed.
>>>>>
>>>>> What exactly does "stopped working" mean ? That might help understanding
>>>>> what went wrong.
>>>>
>>>> /sys files related to battery no longer appear. I beieve this has
>>>> something to do with it:
>>>>
>>>> [    2.374877] of_get_named_gpiod_flags: parsed 'reset-gpios' property
>>>> of node '/ocp at 68000000/spi at 48098000/tsc2005 at 0[0]' - status (0)
>>>> [    2.375946] input: TSC2005 touchscreen as
>>>> /devices/platform/68000000.ocp/48098000.spi/spi_master/spi1/spi1.0/input/input5
>>>> [    2.392120] rx51-battery: probe of n900-battery failed with error
>>>> -22
>>>
>>> Mystery solved:
>>>
>>> diff --git a/drivers/hwmon/hwmon.c b/drivers/hwmon/hwmon.c
>>> index 3932f92..fe5ec82 100644
>>> --- a/drivers/hwmon/hwmon.c
>>> +++ b/drivers/hwmon/hwmon.c
>>> @@ -545,8 +545,10 @@ __hwmon_device_register(struct device *dev, const char *name, void *drvdata,
>>>  	int i, j, err, id;
>>>
>>>  	/* Do not accept invalid characters in hwmon name attribute */
>>> -	if (name && (!strlen(name) || strpbrk(name, "-* \t\n")))
>>> +	if (name && (!strlen(name) || strpbrk(name, "-* \t\n"))) {
>>> +		printk("hwmon: Invalid character detected: %s\n", name);
>>>  		return ERR_PTR(-EINVAL);
>>> +	}
>>>
>>>  	id = ida_simple_get(&hwmon_ida, 0, 0, GFP_KERNEL);
>>>  	if (id < 0)
>>>
>>>
>>> pavel at n900:~$ dmesg | grep -5 Invalid
>>> [    0.829650] of_get_named_gpiod_flags: parsed 'gpio-reset' property
>>> of node '/ocp at 68000000/i2c at 48072000/tlv320aic3x at 19[0]' - status (0)
>>> [    0.833831] tsl2563 2-0029: model 7, rev. 0
>>> [    0.837768] of_get_named_gpiod_flags: parsed 'enable-gpio' property
>>> of node '/ocp at 68000000/i2c at 48072000/lp5523 at 32[0]' - status (0)
>>> [    1.921417] omap_i2c 48072000.i2c: controller timed out
>>> [    2.056823] lp5523x 2-0032: lp5523 Programmable led chip found
>>> [    2.064147] hwmon: Invalid character detected: bq27200-0
>>
>> So the problem is really that the thermal driver needs to create a valid name.
>>
> Right.
>
> Before reverting, can you please try if this patch works or not?
>
> thanks,
> rui
>
>>From 79320ea3721314ec29ed6cdd6987ff922b11d6f9 Mon Sep 17 00:00:00 2001
> From: Zhang Rui <rui.zhang at intel.com>
> Date: Tue, 24 Jan 2017 14:11:03 +0800
> Subject: [PATCH] thermal: fix parameter when registering hwmon
>
> commit 7611fb68062f ("thermal: thermal_hwmon: Convert to
> hwmon_device_register_with_info()") converts thermal core to use
> hwmon_device_register_with_info() to register to hwmon instead of
> deprecated hwmon_device_register().
> But at the same time, the name of the hwmon device created is changed to
> the thermal zone device name in this commit, which may contain
> incompatible characters for hwmon.
>
> Fixes it by using exactly the same parameters as before this commit.
>
> Fixes: 7611fb68062f ("thermal: thermal_hwmon: Convert to hwmon_devce_register_with_info()")
> Reported-by: Pavel Machek <pavel at ucw.cz>
> Signed-off-by: Zhang Rui <rui.zhang at intel.com>
> ---
>  drivers/thermal/thermal_hwmon.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/thermal/thermal_hwmon.c b/drivers/thermal/thermal_hwmon.c
> index c4a508a..0c6789f 100644
> --- a/drivers/thermal/thermal_hwmon.c
> +++ b/drivers/thermal/thermal_hwmon.c
> @@ -157,8 +157,8 @@ int thermal_add_hwmon_sysfs(struct thermal_zone_device *tz)
>
>  	INIT_LIST_HEAD(&hwmon->tz_list);
>  	strlcpy(hwmon->type, tz->type, THERMAL_NAME_LENGTH);
> -	hwmon->device = hwmon_device_register_with_info(NULL, hwmon->type,
> -							hwmon, NULL, NULL);
> +	hwmon->device = hwmon_device_register_with_info(NULL, NULL, NULL,
> +							NULL, NULL);

This is wrong. The name should be converted to a valid name. Otherwise you would
still have to provide the name attribute yourself, and the resulting name would
still be invalid. A simple fix would be to add
	strreplace(hwmon->type, '-', '_');
after strlcpy().

Guenter

>  	if (IS_ERR(hwmon->device)) {
>  		result = PTR_ERR(hwmon->device);
>  		goto free_mem;
>