Re: [PATCH v3 1/4] hwmon: (lm90) split set&show temp as common codes

Re: [PATCH v3 1/4] hwmon: (lm90) split set&show temp as common codes

[Wei Ni]

On 07/12/2013 10:40 PM, Guenter Roeck wrote:
> On Fri, Jul 12, 2013 at 04:30:34PM +0200, Jean Delvare wrote:
>> Hi Guenter,
>>
>> On Fri, 12 Jul 2013 06:50:00 -0700, Guenter Roeck wrote:
>>> On Fri, Jul 12, 2013 at 03:26:15PM +0200, Jean Delvare wrote:
>>>> One thing I am a little worried about (but maybe I'm wrong) is that I
>>>> seem to understand you want to register every LM90-like chip as both a
>>>> hwmon device and two thermal devices. I seem to recall that every
>>>> thermal device is also exposed automatically as a virtual hwmon
>>>> device, is that correct? If so we will be presenting the same values
>>>> twice to libsensors, which would be confusing.
>>>
>>> Not sure if that is a good idea, but if I recall correctly, the thermal folks
>>> plan to remove that path.

Hi, Rui
As Jean said, if we want to register the lm90 as thermal device, it will
have two hwmon devices in the sysfs, one is registered by the lm90
driver, another one is registered by the thermal_zone_device_register(),
this would be confusing.

Do you have any ideas for it?

Thanks.
Wei.

>>
>> If that means that for example the ACPI thermal zone is no longer
>> displayed by "sensors", then I strongly object - unless it is
>> explicitly registered as a separate hwmon device from now on, of course.
>>
> If I recall correctly that was the idea. Of course, in practice that will mean
> that devices will _not_ get exposed as hwmon devices, as implementers won't
> bother doing both.
> 
>> My idea was to make the bridge optional - you decide when you register
>> a thermal device if it should be exposed as hwmon or not.
>>
> Yes, that would be a much better solution.

I think we can decide it in the DT, we can add a dt property in the lm90
device node, such as:
sys-interface = SYS_HWMON;
or
sys-interface = SYS_THERMAL;
So we register it as the hwmon or thermal device

> 
>> I don't have a strong opinion on the implementation, as long as each
>> input is listed by "sensors" once and only once.
>>
> Agreed.
> 
> Guenter
> 

Re: [PATCH v3 1/4] hwmon: (lm90) split set&show temp as common codes

[Jean Delvare]

On Mon, 15 Jul 2013 14:25:29 +0800, Wei Ni wrote:
> On 07/12/2013 10:40 PM, Guenter Roeck wrote:
> > On Fri, Jul 12, 2013 at 04:30:34PM +0200, Jean Delvare wrote:
> >> If that means that for example the ACPI thermal zone is no longer
> >> displayed by "sensors", then I strongly object - unless it is
> >> explicitly registered as a separate hwmon device from now on, of course.
> >
> > If I recall correctly that was the idea. Of course, in practice that will mean
> > that devices will _not_ get exposed as hwmon devices, as implementers won't
> > bother doing both.
> > 
> >> My idea was to make the bridge optional - you decide when you register
> >> a thermal device if it should be exposed as hwmon or not.
> >
> > Yes, that would be a much better solution.
> 
> I think we can decide it in the DT, we can add a dt property in the lm90
> device node, such as:
> sys-interface = SYS_HWMON;
> or
> sys-interface = SYS_THERMAL;
> So we register it as the hwmon or thermal device

This is an option, but please keep in mind that DT is not the only way
to instantiate LM90-like devices, and we should not expose duplicate
inputs by default. It is fine with me if the default is to create only a
HWMON device (as the lm90 driver was doing so far), and only
DT-instantiated devices have the choice.

Another option, as discussed before, would be that the thermal devices
registered by lm90 are specifically tagged as "do not expose as hwmon".
This would avoid the duplicate hwmon inputs in all cases.

Again - no strong opinion on the implementation, as long as it does the
right thing.

Oh, and we'll have to be careful with the Kconfig dependencies. I do
not want the lm90 driver to depend on the thermal framework.

-- 
Jean Delvare

Re: [PATCH v3 1/4] hwmon: (lm90) split set&show temp as common codes

[Wei Ni]

On 07/15/2013 03:24 PM, Jean Delvare wrote:
> On Mon, 15 Jul 2013 14:25:29 +0800, Wei Ni wrote:
>> On 07/12/2013 10:40 PM, Guenter Roeck wrote:
>>> On Fri, Jul 12, 2013 at 04:30:34PM +0200, Jean Delvare wrote:
>>>> If that means that for example the ACPI thermal zone is no longer
>>>> displayed by "sensors", then I strongly object - unless it is
>>>> explicitly registered as a separate hwmon device from now on, of course.
>>>
>>> If I recall correctly that was the idea. Of course, in practice that will mean
>>> that devices will _not_ get exposed as hwmon devices, as implementers won't
>>> bother doing both.
>>>
>>>> My idea was to make the bridge optional - you decide when you register
>>>> a thermal device if it should be exposed as hwmon or not.
>>>
>>> Yes, that would be a much better solution.
>>
>> I think we can decide it in the DT, we can add a dt property in the lm90
>> device node, such as:
>> sys-interface = SYS_HWMON;
>> or
>> sys-interface = SYS_THERMAL;
>> So we register it as the hwmon or thermal device
> 
> This is an option, but please keep in mind that DT is not the only way
> to instantiate LM90-like devices, and we should not expose duplicate
> inputs by default. It is fine with me if the default is to create only a
> HWMON device (as the lm90 driver was doing so far), and only
> DT-instantiated devices have the choice.

Yes, we should not expose duplicate inputs, we may have tree permutation:
1. only hwmon device:
for this items, we just need to call hwmon_device_register().
2. only thermal device + virtual hwmon device:
for this items, we just need to call thermal_zone_device_register().

We can set #1 as the default, and if use DT, we provide option to choice
#1 or #2.

3. hwmon device + thermal device:
for this items, we doesn't need the virtual hwmon which registered by
the thermal fw, how to handle this one? Add flag when register as
thermal device to indicate if want virtual hwmon device or not,
something like your below another option.

> 
> Another option, as discussed before, would be that the thermal devices
> registered by lm90 are specifically tagged as "do not expose as hwmon".
> This would avoid the duplicate hwmon inputs in all cases.

It seems in current thermal fw, it can't be tagged as "do not expose as
hwmon", we need to add flag when register thermal device.

Rui, what do you think for it?

> 
> Again - no strong opinion on the implementation, as long as it does the
> right thing.

I'm working on the Tegra platform, we uses nct1008 as the temperature
sensor, and we want to register it as thermal device, so that we can run
the throttle function. So I prepared these patches to enhance this driver.

> 
> Oh, and we'll have to be careful with the Kconfig dependencies. I do
> not want the lm90 driver to depend on the thermal framework.

Yes, absolutely agree, lm90 should be independent.
Indeed, the thermal folks is trying to restructure the thermal fw, and
in this new fw, it's more easy to add the generic sensors to the thermal
fw. If you interest it, please refer
http://thread.gmane.org/gmane.linux.power-management.general/30692.

Thanks.
Wei.

> 

Re: [PATCH v3 1/4] hwmon: (lm90) split set&show temp as common codes

[Jean Delvare]

Hi Wei,

On Mon, 15 Jul 2013 17:14:07 +0800, Wei Ni wrote:
> On 07/15/2013 03:24 PM, Jean Delvare wrote:
> > On Mon, 15 Jul 2013 14:25:29 +0800, Wei Ni wrote:
> >> I think we can decide it in the DT, we can add a dt property in the lm90
> >> device node, such as:
> >> sys-interface = SYS_HWMON;
> >> or
> >> sys-interface = SYS_THERMAL;
> >> So we register it as the hwmon or thermal device
> > 
> > This is an option, but please keep in mind that DT is not the only way
> > to instantiate LM90-like devices, and we should not expose duplicate
> > inputs by default. It is fine with me if the default is to create only a
> > HWMON device (as the lm90 driver was doing so far), and only
> > DT-instantiated devices have the choice.
> 
> Yes, we should not expose duplicate inputs, we may have tree permutation:
> 1. only hwmon device:
> for this items, we just need to call hwmon_device_register().
> 2. only thermal device + virtual hwmon device:
> for this items, we just need to call thermal_zone_device_register().
> 
> We can set #1 as the default, and if use DT, we provide option to choice
> #1 or #2.

#2 makes little sense IMHO, for a driver which properly implements
hwmon support. The point of the virtual hwmon device created for
thermal zones was to not put an extra burden on thermal driver authors
by asking them to additionally implement the hwmon interface. But the
hwmon interface it richer than the thermal interface in some respects
so native hwmon implementations are preferred when available. Thus I
think your option #3 below is what we want in addition to #1, and we
don't need #2.

> 3. hwmon device + thermal device:
> for this items, we doesn't need the virtual hwmon which registered by
> the thermal fw, how to handle this one? Add flag when register as
> thermal device to indicate if want virtual hwmon device or not,
> something like your below another option.

-- 
Jean Delvare

Re: [PATCH v3 1/4] hwmon: (lm90) split set&show temp as common codes

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 1830 bytes --]

On Mon, Jul 15, 2013 at 09:24:15AM +0200, Jean Delvare wrote:
> On Mon, 15 Jul 2013 14:25:29 +0800, Wei Ni wrote:
> > On 07/12/2013 10:40 PM, Guenter Roeck wrote:
> > > On Fri, Jul 12, 2013 at 04:30:34PM +0200, Jean Delvare wrote:
> > >> If that means that for example the ACPI thermal zone is no longer
> > >> displayed by "sensors", then I strongly object - unless it is
> > >> explicitly registered as a separate hwmon device from now on, of course.
> > >
> > > If I recall correctly that was the idea. Of course, in practice that will mean
> > > that devices will _not_ get exposed as hwmon devices, as implementers won't
> > > bother doing both.
> > > 
> > >> My idea was to make the bridge optional - you decide when you register
> > >> a thermal device if it should be exposed as hwmon or not.
> > >
> > > Yes, that would be a much better solution.
> > 
> > I think we can decide it in the DT, we can add a dt property in the lm90
> > device node, such as:
> > sys-interface = SYS_HWMON;
> > or
> > sys-interface = SYS_THERMAL;
> > So we register it as the hwmon or thermal device
> 
> This is an option, but please keep in mind that DT is not the only way
> to instantiate LM90-like devices, and we should not expose duplicate
> inputs by default. It is fine with me if the default is to create only a
> HWMON device (as the lm90 driver was doing so far), and only
> DT-instantiated devices have the choice.

I don't think this information belongs in the device tree. Whether the
device is exposed as hwmon or thermal device (or both) is entirely a
software issue whereas DT is a means to describe the hardware.

It seems to me that the earlier proposal of communicating to the bridge
whether or not a device should be exposed as hwmon device is the right
thing to do here.

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH v3 1/4] hwmon: (lm90) split set&show temp as common codes

[Guenter Roeck]

On Wed, Jul 17, 2013 at 06:26:20AM +0200, Thierry Reding wrote:
> On Mon, Jul 15, 2013 at 09:24:15AM +0200, Jean Delvare wrote:
> > On Mon, 15 Jul 2013 14:25:29 +0800, Wei Ni wrote:
> > > On 07/12/2013 10:40 PM, Guenter Roeck wrote:
> > > > On Fri, Jul 12, 2013 at 04:30:34PM +0200, Jean Delvare wrote:
> > > >> If that means that for example the ACPI thermal zone is no longer
> > > >> displayed by "sensors", then I strongly object - unless it is
> > > >> explicitly registered as a separate hwmon device from now on, of course.
> > > >
> > > > If I recall correctly that was the idea. Of course, in practice that will mean
> > > > that devices will _not_ get exposed as hwmon devices, as implementers won't
> > > > bother doing both.
> > > > 
> > > >> My idea was to make the bridge optional - you decide when you register
> > > >> a thermal device if it should be exposed as hwmon or not.
> > > >
> > > > Yes, that would be a much better solution.
> > > 
> > > I think we can decide it in the DT, we can add a dt property in the lm90
> > > device node, such as:
> > > sys-interface = SYS_HWMON;
> > > or
> > > sys-interface = SYS_THERMAL;
> > > So we register it as the hwmon or thermal device
> > 
> > This is an option, but please keep in mind that DT is not the only way
> > to instantiate LM90-like devices, and we should not expose duplicate
> > inputs by default. It is fine with me if the default is to create only a
> > HWMON device (as the lm90 driver was doing so far), and only
> > DT-instantiated devices have the choice.
> 
> I don't think this information belongs in the device tree. Whether the
> device is exposed as hwmon or thermal device (or both) is entirely a
> software issue whereas DT is a means to describe the hardware.
> 
Correct; this is exactly the type of information which does _not_ belong int
devicetree.

> It seems to me that the earlier proposal of communicating to the bridge
> whether or not a device should be exposed as hwmon device is the right
> thing to do here.
> 
Agreed..

Guenter

Re: [PATCH v3 1/4] hwmon: (lm90) split set&show temp as common codes

[Wei Ni]

On 07/17/2013 01:14 PM, Guenter Roeck wrote:
> On Wed, Jul 17, 2013 at 06:26:20AM +0200, Thierry Reding wrote:
>> On Mon, Jul 15, 2013 at 09:24:15AM +0200, Jean Delvare wrote:
>>> On Mon, 15 Jul 2013 14:25:29 +0800, Wei Ni wrote:
>>>> On 07/12/2013 10:40 PM, Guenter Roeck wrote:
>>>>> On Fri, Jul 12, 2013 at 04:30:34PM +0200, Jean Delvare wrote:
>>>>>> If that means that for example the ACPI thermal zone is no longer
>>>>>> displayed by "sensors", then I strongly object - unless it is
>>>>>> explicitly registered as a separate hwmon device from now on, of course.
>>>>>
>>>>> If I recall correctly that was the idea. Of course, in practice that will mean
>>>>> that devices will _not_ get exposed as hwmon devices, as implementers won't
>>>>> bother doing both.
>>>>>
>>>>>> My idea was to make the bridge optional - you decide when you register
>>>>>> a thermal device if it should be exposed as hwmon or not.
>>>>>
>>>>> Yes, that would be a much better solution.
>>>>
>>>> I think we can decide it in the DT, we can add a dt property in the lm90
>>>> device node, such as:
>>>> sys-interface = SYS_HWMON;
>>>> or
>>>> sys-interface = SYS_THERMAL;
>>>> So we register it as the hwmon or thermal device
>>>
>>> This is an option, but please keep in mind that DT is not the only way
>>> to instantiate LM90-like devices, and we should not expose duplicate
>>> inputs by default. It is fine with me if the default is to create only a
>>> HWMON device (as the lm90 driver was doing so far), and only
>>> DT-instantiated devices have the choice.
>>
>> I don't think this information belongs in the device tree. Whether the
>> device is exposed as hwmon or thermal device (or both) is entirely a
>> software issue whereas DT is a means to describe the hardware.
>>
> Correct; this is exactly the type of information which does _not_ belong int
> devicetree.
> 
>> It seems to me that the earlier proposal of communicating to the bridge
>> whether or not a device should be exposed as hwmon device is the right
>> thing to do here.
>>
> Agreed..

Sorry, what's the "bridge" mean, does it mean we need to add a flag in
the thermal_zone_device_register() to indicate if we want to register
virtual hwmon device or not?

Thanks.
Wei.

> 
> Guenter
> 

Re: [PATCH v3 1/4] hwmon: (lm90) split set&show temp as common codes

[Jean Delvare]

On Wed, 17 Jul 2013 14:26:54 +0800, Wei Ni wrote:
> On 07/17/2013 01:14 PM, Guenter Roeck wrote:
> > On Wed, Jul 17, 2013 at 06:26:20AM +0200, Thierry Reding wrote:
> >> On Mon, Jul 15, 2013 at 09:24:15AM +0200, Jean Delvare wrote:
> >>> On Mon, 15 Jul 2013 14:25:29 +0800, Wei Ni wrote:
> >>>> I think we can decide it in the DT, we can add a dt property in the lm90
> >>>> device node, such as:
> >>>> sys-interface = SYS_HWMON;
> >>>> or
> >>>> sys-interface = SYS_THERMAL;
> >>>> So we register it as the hwmon or thermal device
> >>>
> >>> This is an option, but please keep in mind that DT is not the only way
> >>> to instantiate LM90-like devices, and we should not expose duplicate
> >>> inputs by default. It is fine with me if the default is to create only a
> >>> HWMON device (as the lm90 driver was doing so far), and only
> >>> DT-instantiated devices have the choice.
> >>
> >> I don't think this information belongs in the device tree. Whether the
> >> device is exposed as hwmon or thermal device (or both) is entirely a
> >> software issue whereas DT is a means to describe the hardware.
> >>
> > Correct; this is exactly the type of information which does _not_ belong int
> > devicetree.
> > 
> >> It seems to me that the earlier proposal of communicating to the bridge
> >> whether or not a device should be exposed as hwmon device is the right
> >> thing to do here.
> >
> > Agreed..
> 
> Sorry, what's the "bridge" mean,

The code which creates a virtual hwmon input when a new thermal zone is
registered (this code is in thermal_core.c.)

> does it mean we need to add a flag in
> the thermal_zone_device_register() to indicate if we want to register
> virtual hwmon device or not?

I think so, yes.

Alternatively the flag could be added to struct
thermal_zone_device_ops, so that you don't have to update all the
callers. But I admit it's a hack as the flag doesn't really belong
there, so I suppose we don't really want to do that.

I have been thinking of an automatic approach, based on comparing the
type string passed to thermal_zone_device_register() with already
registered hwmon devices, but I couldn't come up with something good
and robust enough, so let's forget about it.

-- 
Jean Delvare

Re: [PATCH v3 1/4] hwmon: (lm90) split set&show temp as common codes

[Wei Ni]

On 07/17/2013 05:11 PM, Jean Delvare wrote:
> On Wed, 17 Jul 2013 14:26:54 +0800, Wei Ni wrote:
>> On 07/17/2013 01:14 PM, Guenter Roeck wrote:
>>> On Wed, Jul 17, 2013 at 06:26:20AM +0200, Thierry Reding wrote:
>>>> On Mon, Jul 15, 2013 at 09:24:15AM +0200, Jean Delvare wrote:
>>>>> On Mon, 15 Jul 2013 14:25:29 +0800, Wei Ni wrote:
>>>>>> I think we can decide it in the DT, we can add a dt property in the lm90
>>>>>> device node, such as:
>>>>>> sys-interface = SYS_HWMON;
>>>>>> or
>>>>>> sys-interface = SYS_THERMAL;
>>>>>> So we register it as the hwmon or thermal device
>>>>>
>>>>> This is an option, but please keep in mind that DT is not the only way
>>>>> to instantiate LM90-like devices, and we should not expose duplicate
>>>>> inputs by default. It is fine with me if the default is to create only a
>>>>> HWMON device (as the lm90 driver was doing so far), and only
>>>>> DT-instantiated devices have the choice.
>>>>
>>>> I don't think this information belongs in the device tree. Whether the
>>>> device is exposed as hwmon or thermal device (or both) is entirely a
>>>> software issue whereas DT is a means to describe the hardware.
>>>>
>>> Correct; this is exactly the type of information which does _not_ belong int
>>> devicetree.
>>>
>>>> It seems to me that the earlier proposal of communicating to the bridge
>>>> whether or not a device should be exposed as hwmon device is the right
>>>> thing to do here.
>>>
>>> Agreed..
>>
>> Sorry, what's the "bridge" mean,
> 
> The code which creates a virtual hwmon input when a new thermal zone is
> registered (this code is in thermal_core.c.)
> 
>> does it mean we need to add a flag in
>> the thermal_zone_device_register() to indicate if we want to register
>> virtual hwmon device or not?
> 
> I think so, yes.
> 
> Alternatively the flag could be added to struct
> thermal_zone_device_ops, so that you don't have to update all the
> callers. But I admit it's a hack as the flag doesn't really belong
> there, so I suppose we don't really want to do that.

I think it's better to add it to struct thermal_zone_params, the
thermal_zone_device_ops is for the callback functions.
And I ask it with thermal fw engineers in
http://thread.gmane.org/gmane.linux.power-management.general/35874.
May be you can look it.

> 
> I have been thinking of an automatic approach, based on comparing the
> type string passed to thermal_zone_device_register() with already
> registered hwmon devices, but I couldn't come up with something good
> and robust enough, so let's forget about it.
>