On Thu, Apr 18, 2019 at 03:54:32PM -0400, Jean-Francois Dagenais wrote:
>
>
> > On Apr 18, 2019, at 14:06, Guenter Roeck <linux@xxxxxxxxxxxx> wrote:
> >
> > On Thu, Apr 18, 2019 at 02:02:48PM -0400, Jean-Francois Dagenais wrote:
> >>
> >>> On Apr 18, 2019, at 13:38, Guenter Roeck <linux@xxxxxxxxxxxx> wrote:
> >>>
> >>>> +#if IS_ENABLED(CONFIG_THERMAL)
> >>>
> >>> This will result in missing symbols if THERMAL is built as module
> >>> and this driver is built into the kernel. You'll have to adjust
> >>> Kconfig dependencies accordingly. See other drivers for examples.
> >>
> >> Right! Was not a problem for me, but I do remember seing the "funny"
> >> ifdefs around.
> >>
> >
> > I know, it is annoying. There was an effort to make THERMAL boolean
> > instead of tristate, but it looks like that has stalled or was
> > rejected.
>
> Ok, so I had used pwm-fan.c as a reference and simply forgotten to copy over the
> Kconfig bit which makes sure if the thermal is a module, then so is the chip
> driver. I will put in v3.
>
> >
> >>>
> >>>> + data->cooling_dev =
> >>>> + thermal_of_cooling_device_register(client->dev.of_node,
> >>>> + id->name, data,
> >>>> + &max6650_cooling_ops);
> >>>> + if (IS_ERR(data->cooling_dev)) {
> >>>> + err = PTR_ERR(data->cooling_dev);
> >>>> + dev_err(&client->dev,
> >>>> + "Failed to register as cooling device (%d)\n", err);
> >>>> + return err;
> >>>
> >>> Why would it be fatal for the driver if this fails ? It wasn't
> >>> fatal before.
> >>
> >> Mmmh, you are right. This assumes that all users of max6650 would now require to
> >> be referred to in some thermal zone. Again, this was not a problem for my test
> >> environment.
>
> And about this bit... I had confused the thermal cooling register with the
> thermal_of_sensor register. So if my static analysis of the code in
> __thermal_cooling_device_register is correct, the function cannot really fail
> unless something is terribly wrong in the kernel... like a malloc failure or
> something.
>
That explains why the other drivers don't generate an error message.
You might want to reconsider the dev_err() above; it appears it adds zero
value.
> If I am wrong, then pwm-fan.c should also be visited since my max6650_probe code
> was copy-pasted from it.
>
You are correct, that should not fail either. But two wrongs don't make it right.
>
> On a related note, I am worried about everyone having to progressively add
> thermal-zone support to each fan chip in drivers/hwmon. Would it not be possible
> for hwmon to detect, through `enum hwmon_sensor_types` set to `hwmon_fan` in
> `struct hwmon_channel_info` to be registered as cooling device in thermal ? Of
> course for max665x to work, it would have to be modified to use
> `devm_hwmon_device_register_with_info` instead of
> `devm_hwmon_device_register_with_groups`
>
Yes, that would be desirable. Patches welcome. And, yes, the driver would have
to be reworked to use the new API.
> Another possiblity would be to create a new driver in drivers/thermal for
> max665x as a thermal cooling device. Or even a
Only if we also drop the existing driver from hwmon. Fine with me if others
agree and let you do it (one less hwmon driver to maintain).
Note that I just submitted a patch series to introduce
devm_thermal_of_cooling_device_register(). We'll see if the thermal maintainers
accept it.
Guenter
> drivers/thermal/thermal-generic-hwmon-fan.c or something, where DTS would
> instanciate such a cooling device which references a hwmon device's specific
> pwm channel.
>
