Re: [PATCH v1 01/17] thermal/core: Add a thermal zone 'devdata' accessor

[Niklas Söderlund]

On 2023-02-19 18:07:36 +0100, Daniel Lezcano wrote:
> On 19/02/2023 16:07, Niklas Söderlund wrote:
> > Hi Daniel,
> > 
> > Thanks for your work.
> > 
> > On 2023-02-19 15:36:41 +0100, Daniel Lezcano wrote:
> > > The thermal zone device structure is exposed to the different drivers
> > > and obviously they access the internals while that should be
> > > restricted to the core thermal code.
> > > 
> > > In order to self-encapsulate the thermal core code, we need to prevent
> > > the drivers accessing directly the thermal zone structure and provide
> > > accessor functions to deal with.
> > > 
> > > Provide an accessor to the 'devdata' structure and make use of it in
> > > the different drivers.
> > > 
> > > No functional changes intended.
> > > 
> > > Signed-off-by: Daniel Lezcano <daniel.lezcano@xxxxxxxxxx>
> > > ---
> > 
> > ...
> > 
> > >   drivers/thermal/rcar_gen3_thermal.c              |  4 ++--
> > >   drivers/thermal/rcar_thermal.c                   |  3 +--
> > 
> > For R-Car,
> > 
> > Reviewed-by: Niklas Söderlund <niklas.soderlund+renesas@xxxxxxxxxxxx>
> > 
> > ...
> > 
> > 
> > > diff --git a/include/linux/thermal.h b/include/linux/thermal.h
> > > index 2bb4bf33f4f3..724b95662da9 100644
> > > --- a/include/linux/thermal.h
> > > +++ b/include/linux/thermal.h
> > > @@ -365,6 +365,8 @@ thermal_zone_device_register_with_trips(const char *, struct thermal_trip *, int
> > >   					void *, struct thermal_zone_device_ops *,
> > >   					struct thermal_zone_params *, int, int);
> > > +void *thermal_zone_device_get_data(struct thermal_zone_device *tzd);
> > > +
> > 
> > bikeshedding:
> > 
> > Would it make sens to name this thermal_zone_device_get_priv_data(),
> > thermal_zone_device_get_priv() or something like that? To make it more
> > explicitly when reading the driver code this fetches the drivers private
> > data, and not some data belonging to the zone itself.
> 
> In the headers files, there are more occurrences with _name_priv():
> 
> # _name_priv()
> git grep priv include/linux/ | grep "priv(" | grep -v get | wc -l
> 52
> 
> # _name_private()
> git grep priv include/linux/ | grep "private(" | grep -v get | wc -l
> 33
> 
> # _name_get_private()
> git grep priv include/linux/ | grep "private(" | grep get | wc -l
> 12
> 
> # _name_get_priv()
> git grep priv include/linux/ | grep "priv(" | grep get | wc -l
> 4
> 
> 
> What about thermal_zone_device_priv() ?

Looks good to me.

> 
> 
> 
> 
> 
> 
> -- 
> <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs
> 
> Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
> <http://twitter.com/#!/linaroorg> Twitter |
> <http://www.linaro.org/linaro-blog/> Blog
> 

-- 
Kind Regards,
Niklas Söderlund