2013-08-29 19:38:42

by Eduardo Valentin

[permalink] [raw]
Subject: Re: [PATCHv3 0/8] Thermal Framework Enhancements

Durga,

On 05-02-2013 06:46, Durgadoss R wrote:
> This patch set is a v3 of the previous versions submitted here:
> [v2]: http://lwn.net/Articles/531720/
> [v1]: https://lkml.org/lkml/2012/12/18/108
> [RFC]:https://patchwork.kernel.org/patch/1758921/
>

Long time this work is not moving forward. While writing the device tree
parser, I thought of using your work as baseline to see how the multiple
sensor per zone would look like (also in DT).

But while checking your code again, I realized that you are actually
creating a new API, that is probably why you named it sysfs-api2. While
I understand the motivation (they are pretty different), I believe this
is not a good thing to do. I would suggest making so that we have a
single API.

> This patch set is based on Rui's -next tree, and is
> tested on a Core-i5 and an Atom netbook.
>
> Changes since v2:
> * Reworked the map sysfs attributes in patch [5/8]
> * Dropped configuration for maximum sensors and
> cooling devices, through Kconfig.
> * Added __remove_trip_attr method
> * Renamed __clean_map_entry to __remove_map_entry
> for consistency in naming.
> Changes Since v1:
> * Removed kobject creation for thermal_trip and thermal_map
> nodes as per Greg-KH's comments.
> * Added ABI Documentation under 'testing'.
> * Modified the GET_INDEX macro to be more linux-like, thanks
> to Joe Perches.
> * Added get_[sensor/cdev]_by_name APIs to thermal.h
>
> This series contains 8 patches:
> Patch 1/8: Creates new sensor level APIs
> Patch 2/8: Creates new zone level APIs. The existing tzd structure is
> kept as such for clarity and compatibility purposes.
> Patch 3/8: Creates functions to add/remove a cdev to/from a zone. The
> existing tcd structure need not be modified.
> Patch 4/8: Adds sensorX_trip_[active/passive/hot/critical] sysfs nodes,
> under /sys/class/thermal/zoneY/. This exposes various trip
> points for sensorX present in zoneY.
> Patch 5/8: Adds mapY_* sysfs node. These attributes represent
> the binding relationship between a sensor and a cdev,
> within a zone.
> Patch 6/8: Creates Documentation for the new APIs. A new file is
> created for clarity. Final goal is to merge with the existing
> file or refactor the files, as whatever seems appropriate.
> Patch 7/8: Add ABI documentation for sysfs interfaces introduced in this patch.
> Patch 8/8: A dummy driver that can be used for testing. This is not for merge.
>
> Durgadoss R (8):
> Thermal: Create sensor level APIs
> Thermal: Create zone level APIs
> Thermal: Add APIs to bind cdev to new zone structure
> Thermal: Add trip point sysfs nodes for sensor
> Thermal: Create Thermal map sysfs attributes for a zone
> Thermal: Add Documentation to new APIs
> Thermal: Add ABI Documentation for sysfs interfaces
> Thermal: Dummy driver used for testing
>
> Documentation/ABI/testing/sysfs-class-thermal | 137 ++++
> Documentation/thermal/sysfs-api2.txt | 247 ++++++
> drivers/thermal/Kconfig | 5 +
> drivers/thermal/Makefile | 2 +
> drivers/thermal/thermal_sys.c | 994 +++++++++++++++++++++++++
> drivers/thermal/thermal_test.c | 324 ++++++++
> include/linux/thermal.h | 123 ++-
> 7 files changed, 1831 insertions(+), 1 deletion(-)
> create mode 100644 Documentation/ABI/testing/sysfs-class-thermal
> create mode 100644 Documentation/thermal/sysfs-api2.txt
> create mode 100644 drivers/thermal/thermal_test.c
>


--
You have got to be excited about what you are doing. (L. Lamport)

Eduardo Valentin


Attachments:
signature.asc (295.00 B)
OpenPGP digital signature

2013-08-30 09:20:42

by R, Durgadoss

[permalink] [raw]
Subject: RE: [PATCHv3 0/8] Thermal Framework Enhancements

Hi Eduardo,

> -----Original Message-----
> From: Eduardo Valentin [mailto:[email protected]]
> Sent: Friday, August 30, 2013 1:08 AM
> To: R, Durgadoss
> Cc: Zhang, Rui; [email protected]; [email protected];
> [email protected]; [email protected]; [email protected]
> Subject: Re: [PATCHv3 0/8] Thermal Framework Enhancements
>
> Durga,
>
> On 05-02-2013 06:46, Durgadoss R wrote:
> > This patch set is a v3 of the previous versions submitted here:
> > [v2]: http://lwn.net/Articles/531720/
> > [v1]: https://lkml.org/lkml/2012/12/18/108
> > [RFC]:https://patchwork.kernel.org/patch/1758921/
> >
>
> Long time this work is not moving forward. While writing the device tree

I am trying my best to get to this,
Will try to refresh in a couple of weeks.

> parser, I thought of using your work as baseline to see how the multiple
> sensor per zone would look like (also in DT).
>
> But while checking your code again, I realized that you are actually
> creating a new API, that is probably why you named it sysfs-api2. While
> I understand the motivation (they are pretty different), I believe this
> is not a good thing to do. I would suggest making so that we have a
> single API.

As in the sensor APIs ? or the zone level APIs ?
The sensor level APIs are anyway new; the zone level ones
we have to work out a way for them.

Is this what you are referring to ? Could you confirm/explain a bit more ?

Thanks,
Durga

>
> > This patch set is based on Rui's -next tree, and is
> > tested on a Core-i5 and an Atom netbook.
> >
> > Changes since v2:
> > * Reworked the map sysfs attributes in patch [5/8]
> > * Dropped configuration for maximum sensors and
> > cooling devices, through Kconfig.
> > * Added __remove_trip_attr method
> > * Renamed __clean_map_entry to __remove_map_entry
> > for consistency in naming.
> > Changes Since v1:
> > * Removed kobject creation for thermal_trip and thermal_map
> > nodes as per Greg-KH's comments.
> > * Added ABI Documentation under 'testing'.
> > * Modified the GET_INDEX macro to be more linux-like, thanks
> > to Joe Perches.
> > * Added get_[sensor/cdev]_by_name APIs to thermal.h
> >
> > This series contains 8 patches:
> > Patch 1/8: Creates new sensor level APIs
> > Patch 2/8: Creates new zone level APIs. The existing tzd structure is
> > kept as such for clarity and compatibility purposes.
> > Patch 3/8: Creates functions to add/remove a cdev to/from a zone. The
> > existing tcd structure need not be modified.
> > Patch 4/8: Adds sensorX_trip_[active/passive/hot/critical] sysfs nodes,
> > under /sys/class/thermal/zoneY/. This exposes various trip
> > points for sensorX present in zoneY.
> > Patch 5/8: Adds mapY_* sysfs node. These attributes represent
> > the binding relationship between a sensor and a cdev,
> > within a zone.
> > Patch 6/8: Creates Documentation for the new APIs. A new file is
> > created for clarity. Final goal is to merge with the existing
> > file or refactor the files, as whatever seems appropriate.
> > Patch 7/8: Add ABI documentation for sysfs interfaces introduced in this patch.
> > Patch 8/8: A dummy driver that can be used for testing. This is not for merge.
> >
> > Durgadoss R (8):
> > Thermal: Create sensor level APIs
> > Thermal: Create zone level APIs
> > Thermal: Add APIs to bind cdev to new zone structure
> > Thermal: Add trip point sysfs nodes for sensor
> > Thermal: Create Thermal map sysfs attributes for a zone
> > Thermal: Add Documentation to new APIs
> > Thermal: Add ABI Documentation for sysfs interfaces
> > Thermal: Dummy driver used for testing
> >
> > Documentation/ABI/testing/sysfs-class-thermal | 137 ++++
> > Documentation/thermal/sysfs-api2.txt | 247 ++++++
> > drivers/thermal/Kconfig | 5 +
> > drivers/thermal/Makefile | 2 +
> > drivers/thermal/thermal_sys.c | 994 +++++++++++++++++++++++++
> > drivers/thermal/thermal_test.c | 324 ++++++++
> > include/linux/thermal.h | 123 ++-
> > 7 files changed, 1831 insertions(+), 1 deletion(-)
> > create mode 100644 Documentation/ABI/testing/sysfs-class-thermal
> > create mode 100644 Documentation/thermal/sysfs-api2.txt
> > create mode 100644 drivers/thermal/thermal_test.c
> >
>
>
> --
> You have got to be excited about what you are doing. (L. Lamport)
>
> Eduardo Valentin

2013-08-30 12:34:28

by Eduardo Valentin

[permalink] [raw]
Subject: Re: [PATCHv3 0/8] Thermal Framework Enhancements

On 30-08-2013 05:20, R, Durgadoss wrote:
> Hi Eduardo,
>
>> -----Original Message-----
>> From: Eduardo Valentin [mailto:[email protected]]
>> Sent: Friday, August 30, 2013 1:08 AM
>> To: R, Durgadoss
>> Cc: Zhang, Rui; [email protected]; [email protected];
>> [email protected]; [email protected]; [email protected]
>> Subject: Re: [PATCHv3 0/8] Thermal Framework Enhancements
>>
>> Durga,
>>
>> On 05-02-2013 06:46, Durgadoss R wrote:
>>> This patch set is a v3 of the previous versions submitted here:
>>> [v2]: http://lwn.net/Articles/531720/
>>> [v1]: https://lkml.org/lkml/2012/12/18/108
>>> [RFC]:https://patchwork.kernel.org/patch/1758921/
>>>
>>
>> Long time this work is not moving forward. While writing the device tree
>
> I am trying my best to get to this,
> Will try to refresh in a couple of weeks.
>
>> parser, I thought of using your work as baseline to see how the multiple
>> sensor per zone would look like (also in DT).
>>
>> But while checking your code again, I realized that you are actually
>> creating a new API, that is probably why you named it sysfs-api2. While
>> I understand the motivation (they are pretty different), I believe this
>> is not a good thing to do. I would suggest making so that we have a
>> single API.
>
> As in the sensor APIs ? or the zone level APIs ?
> The sensor level APIs are anyway new; the zone level ones
> we have to work out a way for them.
>
> Is this what you are referring to ? Could you confirm/explain a bit more ?

OK. Let's assume that maybe I didn't quite get your proposal then. Let
me step back and ask you what is the relation between existing
thermal_zone_device and thermal_zone? Would existing drivers be
complaint with new API?

>
> Thanks,
> Durga
>
>>
>>> This patch set is based on Rui's -next tree, and is
>>> tested on a Core-i5 and an Atom netbook.
>>>
>>> Changes since v2:
>>> * Reworked the map sysfs attributes in patch [5/8]
>>> * Dropped configuration for maximum sensors and
>>> cooling devices, through Kconfig.
>>> * Added __remove_trip_attr method
>>> * Renamed __clean_map_entry to __remove_map_entry
>>> for consistency in naming.
>>> Changes Since v1:
>>> * Removed kobject creation for thermal_trip and thermal_map
>>> nodes as per Greg-KH's comments.
>>> * Added ABI Documentation under 'testing'.
>>> * Modified the GET_INDEX macro to be more linux-like, thanks
>>> to Joe Perches.
>>> * Added get_[sensor/cdev]_by_name APIs to thermal.h
>>>
>>> This series contains 8 patches:
>>> Patch 1/8: Creates new sensor level APIs
>>> Patch 2/8: Creates new zone level APIs. The existing tzd structure is
>>> kept as such for clarity and compatibility purposes.
>>> Patch 3/8: Creates functions to add/remove a cdev to/from a zone. The
>>> existing tcd structure need not be modified.
>>> Patch 4/8: Adds sensorX_trip_[active/passive/hot/critical] sysfs nodes,
>>> under /sys/class/thermal/zoneY/. This exposes various trip
>>> points for sensorX present in zoneY.
>>> Patch 5/8: Adds mapY_* sysfs node. These attributes represent
>>> the binding relationship between a sensor and a cdev,
>>> within a zone.
>>> Patch 6/8: Creates Documentation for the new APIs. A new file is
>>> created for clarity. Final goal is to merge with the existing
>>> file or refactor the files, as whatever seems appropriate.
>>> Patch 7/8: Add ABI documentation for sysfs interfaces introduced in this patch.
>>> Patch 8/8: A dummy driver that can be used for testing. This is not for merge.
>>>
>>> Durgadoss R (8):
>>> Thermal: Create sensor level APIs
>>> Thermal: Create zone level APIs
>>> Thermal: Add APIs to bind cdev to new zone structure
>>> Thermal: Add trip point sysfs nodes for sensor
>>> Thermal: Create Thermal map sysfs attributes for a zone
>>> Thermal: Add Documentation to new APIs
>>> Thermal: Add ABI Documentation for sysfs interfaces
>>> Thermal: Dummy driver used for testing
>>>
>>> Documentation/ABI/testing/sysfs-class-thermal | 137 ++++
>>> Documentation/thermal/sysfs-api2.txt | 247 ++++++
>>> drivers/thermal/Kconfig | 5 +
>>> drivers/thermal/Makefile | 2 +
>>> drivers/thermal/thermal_sys.c | 994 +++++++++++++++++++++++++
>>> drivers/thermal/thermal_test.c | 324 ++++++++
>>> include/linux/thermal.h | 123 ++-
>>> 7 files changed, 1831 insertions(+), 1 deletion(-)
>>> create mode 100644 Documentation/ABI/testing/sysfs-class-thermal
>>> create mode 100644 Documentation/thermal/sysfs-api2.txt
>>> create mode 100644 drivers/thermal/thermal_test.c
>>>
>>
>>
>> --
>> You have got to be excited about what you are doing. (L. Lamport)
>>
>> Eduardo Valentin
>
>
>


--
You have got to be excited about what you are doing. (L. Lamport)

Eduardo Valentin


Attachments:
signature.asc (295.00 B)
OpenPGP digital signature

2013-08-30 13:21:14

by R, Durgadoss

[permalink] [raw]
Subject: RE: [PATCHv3 0/8] Thermal Framework Enhancements

> -----Original Message-----
> From: [email protected] [mailto:linux-pm-
> [email protected]] On Behalf Of Eduardo Valentin
> Sent: Friday, August 30, 2013 6:04 PM
> To: R, Durgadoss
> Cc: Eduardo Valentin; Zhang, Rui; [email protected]; linux-
> [email protected]; [email protected]; [email protected]
> Subject: Re: [PATCHv3 0/8] Thermal Framework Enhancements
>
> On 30-08-2013 05:20, R, Durgadoss wrote:
> > Hi Eduardo,
> >
> >> -----Original Message-----
> >> From: Eduardo Valentin [mailto:[email protected]]
> >> Sent: Friday, August 30, 2013 1:08 AM
> >> To: R, Durgadoss
> >> Cc: Zhang, Rui; [email protected]; [email protected];
> >> [email protected]; [email protected]; [email protected]
> >> Subject: Re: [PATCHv3 0/8] Thermal Framework Enhancements
> >>
> >> Durga,
> >>
> >> On 05-02-2013 06:46, Durgadoss R wrote:
> >>> This patch set is a v3 of the previous versions submitted here:
> >>> [v2]: http://lwn.net/Articles/531720/
> >>> [v1]: https://lkml.org/lkml/2012/12/18/108
> >>> [RFC]:https://patchwork.kernel.org/patch/1758921/
> >>>
> >>
> >> Long time this work is not moving forward. While writing the device tree
> >
> > I am trying my best to get to this,
> > Will try to refresh in a couple of weeks.
> >
> >> parser, I thought of using your work as baseline to see how the multiple
> >> sensor per zone would look like (also in DT).
> >>
> >> But while checking your code again, I realized that you are actually
> >> creating a new API, that is probably why you named it sysfs-api2. While
> >> I understand the motivation (they are pretty different), I believe this
> >> is not a good thing to do. I would suggest making so that we have a
> >> single API.
> >
> > As in the sensor APIs ? or the zone level APIs ?
> > The sensor level APIs are anyway new; the zone level ones
> > we have to work out a way for them.
> >
> > Is this what you are referring to ? Could you confirm/explain a bit more ?
>
> OK. Let's assume that maybe I didn't quite get your proposal then. Let
> me step back and ask you what is the relation between existing
> thermal_zone_device and thermal_zone? Would existing drivers be

The existing thermal_zone_device would be a new thermal zone
with one sensor in it.

> complaint with new API?

Okay got it. This is what myself & Rui discussed a while ago to address
this.

When these patches are merged, they will co-exist for some time.
Rui had an idea to write a wrapper on top of these, which will
make the existing drivers complaint with new API.
This wrapper can be a Kconfig option, to select on what we need;
new/existing APIs.

Thanks,
Durga

> >
> >>
> >>> This patch set is based on Rui's -next tree, and is
> >>> tested on a Core-i5 and an Atom netbook.
> >>>
> >>> Changes since v2:
> >>> * Reworked the map sysfs attributes in patch [5/8]
> >>> * Dropped configuration for maximum sensors and
> >>> cooling devices, through Kconfig.
> >>> * Added __remove_trip_attr method
> >>> * Renamed __clean_map_entry to __remove_map_entry
> >>> for consistency in naming.
> >>> Changes Since v1:
> >>> * Removed kobject creation for thermal_trip and thermal_map
> >>> nodes as per Greg-KH's comments.
> >>> * Added ABI Documentation under 'testing'.
> >>> * Modified the GET_INDEX macro to be more linux-like, thanks
> >>> to Joe Perches.
> >>> * Added get_[sensor/cdev]_by_name APIs to thermal.h
> >>>
> >>> This series contains 8 patches:
> >>> Patch 1/8: Creates new sensor level APIs
> >>> Patch 2/8: Creates new zone level APIs. The existing tzd structure is
> >>> kept as such for clarity and compatibility purposes.
> >>> Patch 3/8: Creates functions to add/remove a cdev to/from a zone. The
> >>> existing tcd structure need not be modified.
> >>> Patch 4/8: Adds sensorX_trip_[active/passive/hot/critical] sysfs nodes,
> >>> under /sys/class/thermal/zoneY/. This exposes various trip
> >>> points for sensorX present in zoneY.
> >>> Patch 5/8: Adds mapY_* sysfs node. These attributes represent
> >>> the binding relationship between a sensor and a cdev,
> >>> within a zone.
> >>> Patch 6/8: Creates Documentation for the new APIs. A new file is
> >>> created for clarity. Final goal is to merge with the existing
> >>> file or refactor the files, as whatever seems appropriate.
> >>> Patch 7/8: Add ABI documentation for sysfs interfaces introduced in this
> patch.
> >>> Patch 8/8: A dummy driver that can be used for testing. This is not for merge.
> >>>
> >>> Durgadoss R (8):
> >>> Thermal: Create sensor level APIs
> >>> Thermal: Create zone level APIs
> >>> Thermal: Add APIs to bind cdev to new zone structure
> >>> Thermal: Add trip point sysfs nodes for sensor
> >>> Thermal: Create Thermal map sysfs attributes for a zone
> >>> Thermal: Add Documentation to new APIs
> >>> Thermal: Add ABI Documentation for sysfs interfaces
> >>> Thermal: Dummy driver used for testing
> >>>
> >>> Documentation/ABI/testing/sysfs-class-thermal | 137 ++++
> >>> Documentation/thermal/sysfs-api2.txt | 247 ++++++
> >>> drivers/thermal/Kconfig | 5 +
> >>> drivers/thermal/Makefile | 2 +
> >>> drivers/thermal/thermal_sys.c | 994
> +++++++++++++++++++++++++
> >>> drivers/thermal/thermal_test.c | 324 ++++++++
> >>> include/linux/thermal.h | 123 ++-
> >>> 7 files changed, 1831 insertions(+), 1 deletion(-)
> >>> create mode 100644 Documentation/ABI/testing/sysfs-class-thermal
> >>> create mode 100644 Documentation/thermal/sysfs-api2.txt
> >>> create mode 100644 drivers/thermal/thermal_test.c
> >>>
> >>
> >>
> >> --
> >> You have got to be excited about what you are doing. (L. Lamport)
> >>
> >> Eduardo Valentin
> >
> >
> >
>
>
> --
> You have got to be excited about what you are doing. (L. Lamport)
>
> Eduardo Valentin