Re: [RFC PATCH 01/26] thermal: Introduce thermal_zone_device_register() and params structure

[AngeloGioacchino Del Regno]

Il 15/01/24 13:39, Daniel Lezcano ha scritto:
> On 21/12/2023 13:48, AngeloGioacchino Del Regno wrote:
> > In preparation for extending the thermal zone devices to actually have
> > a name and disambiguation of thermal zone types/names, introduce a new
> > thermal_zone_device_params structure which holds all of the parameters
> > that are necessary to register a thermal zone device, then add a new
> > function thermal_zone_device_register().
> >
> > The latter takes as parameter the newly introduced structure and is
> > made to eventually replace all usages of the now deprecated function
> > thermal_zone_device_register_with_trips() and of
> > thermal_tripless_zone_device_register().
> >
> > Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@xxxxxxxxxxxxx>
> > ---
> >   drivers/thermal/thermal_core.c | 27 +++++++++++++++++++++++++++
> >   include/linux/thermal.h        | 33 +++++++++++++++++++++++++++++++++
> >   2 files changed, 60 insertions(+)
> >
> > diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c
> > index e5434cdbf23b..6be508eb2d72 100644
> > --- a/drivers/thermal/thermal_core.c
> > +++ b/drivers/thermal/thermal_core.c
> > @@ -1235,6 +1235,8 @@ EXPORT_SYMBOL_GPL(thermal_zone_get_crit_temp);
> >    *           whether trip points have been crossed (0 for interrupt
> >    *           driven systems)
> >    *
> > + * This function is deprecated. See thermal_zone_device_register().
> > + *
> >    * This interface function adds a new thermal zone device (sensor) to
> >    * /sys/class/thermal folder as thermal_zone[0-*]. It tries to bind all the
> >    * thermal cooling devices registered at the same time.
> > @@ -1409,6 +1411,7 @@ thermal_zone_device_register_with_trips(const char *type, 
> > struct thermal_trip *t
> >   }
> >   EXPORT_SYMBOL_GPL(thermal_zone_device_register_with_trips);
> > +/* This function is deprecated. See thermal_zone_device_register(). */
> >   struct thermal_zone_device *thermal_tripless_zone_device_register(
> >                       const char *type,
> >                       void *devdata,
> > @@ -1420,6 +1423,30 @@ struct thermal_zone_device 
> > *thermal_tripless_zone_device_register(
> >   }
> >   EXPORT_SYMBOL_GPL(thermal_tripless_zone_device_register);
> > +/**
> > + * thermal_zone_device_register() - register a new thermal zone device
> > + * @tzdp:    Parameters of the new thermal zone device
> > + *        See struct thermal_zone_device_register.
> > + *
> > + * This interface function adds a new thermal zone device (sensor) to
> > + * /sys/class/thermal folder as thermal_zone[0-*]. It tries to bind all the
> > + * thermal cooling devices registered at the same time.
> > + * thermal_zone_device_unregister() must be called when the device is no
> > + * longer needed. The passive cooling depends on the .get_trend() return value.
> > + *
> > + * Return: a pointer to the created struct thermal_zone_device or an
> > + * in case of error, an ERR_PTR. Caller must check return value with
> > + * IS_ERR*() helpers.
> > + */
> > +struct thermal_zone_device *thermal_zone_device_register(struct 
> > thermal_zone_device_params *tzdp)
> > +{
> > +    return thermal_zone_device_register_with_trips(tzdp->type, tzdp->trips, 
> > tzdp->num_trips,
> > +                               tzdp->mask, tzdp->devdata, tzdp->ops,
> > +                               &tzdp->tzp, tzdp->passive_delay,
> > +                               tzdp->polling_delay);
> > +}
> > +EXPORT_SYMBOL_GPL(thermal_zone_device_register);
> > +
> >   void *thermal_zone_device_priv(struct thermal_zone_device *tzd)
> >   {
> >       return tzd->devdata;
> > diff --git a/include/linux/thermal.h b/include/linux/thermal.h
> > index 98957bae08ff..c6ed33a7e468 100644
> > --- a/include/linux/thermal.h
> > +++ b/include/linux/thermal.h
> > @@ -258,6 +258,33 @@ struct thermal_zone_params {
> >       int offset;
> >   };
> > +/**
> > + * struct thermal_zone_device_params - parameters for a thermal zone device
> > + * @type:        the thermal zone device type
> > + * @tzp:        thermal zone platform parameters
> > + * @ops:        standard thermal zone device callbacks
> > + * @devdata:        private device data
> > + * @trips:        a pointer to an array of thermal trips, if any
> > + * @num_trips:        the number of trip points the thermal zone support
> > + * @mask:        a bit string indicating the writeablility of trip points
> > + * @passive_delay:    number of milliseconds to wait between polls when
> > + *            performing passive cooling
> > + * @polling_delay:    number of milliseconds to wait between polls when checking
> > + *            whether trip points have been crossed (0 for interrupt
> > + *            driven systems)
> > + */
> > +struct thermal_zone_device_params {
> > +    const char *type;
> > +    struct thermal_zone_params tzp;
> > +    struct thermal_zone_device_ops *ops;
> > +    void *devdata;
> > +    struct thermal_trip *trips;
> > +    int num_trips;
> > +    int mask;
> > +    int passive_delay;
> > +    int polling_delay;
> > +};
>
>  From my POV, this "struct thermal_zone_params" has been always a inadequate and 
> catch-all structure. It will confuse with thermal_zone_device_params
>
> I suggest to cleanup a bit that by sorting the parameters in the right structures 
> where the result could be something like:
>
> eg.
>
> struct thermal_zone_params {
>
>      const char *type;
>      struct thermal_zone_device_ops *ops;
>      struct thermal_trip *trips;
>      int num_trips;
>
>      int passive_delay;
>      int polling_delay;
>
>      void *devdata;
>          bool no_hwmon;
> };
>
> struct thermal_governor_ipa_params {
>          u32 sustainable_power;
>          s32 k_po;
>          s32 k_pu;
>          s32 k_i;
>          s32 k_d;
>          s32 integral_cutoff;
>          int slope;
>          int offset;
> };
>
> struct thermal_governor_params {
>      char governor_name[THERMAL_NAME_LENGTH];
>      union {
>          struct thermal_governor_ipa_params ipa_params;
>      };
> };
>
> struct thermal_zone_device_params {
>      struct thermal_zone_params *tzp;
>      struct thermal_governor_params *tgp;
> }
>
> No functional changes just code reorg, being a series to be submitted before the 
> rest on these RFC changes (2->26)

Could work. It's true that thermal_zone_params is a catch-all structure, and it's
not really the best... but I also haven't checked how complex and/or how much time
would your proposed change take.

Shouldn't take much as far as I can foresee, but I really have to check a bit.
If I'm right as in it's not something huge, the next series will directly have
this stuff sorted - if not, I'll reach to you.

Cheers,
Angelo

> >   /* Function declarations */
> >   #ifdef CONFIG_THERMAL_OF
> >   struct thermal_zone_device *devm_thermal_of_zone_register(struct device *dev, 
> > int id, void *data,
> > @@ -310,6 +337,8 @@ struct thermal_zone_device 
> > *thermal_tripless_zone_device_register(
> >                       struct thermal_zone_device_ops *ops,
> >                       const struct thermal_zone_params *tzp);
> > +struct thermal_zone_device *thermal_zone_device_register(struct 
> > thermal_zone_device_params *tzdp);
> > +
> >   void thermal_zone_device_unregister(struct thermal_zone_device *tz);
> >   void *thermal_zone_device_priv(struct thermal_zone_device *tzd);
> > @@ -372,6 +401,10 @@ static inline struct thermal_zone_device 
> > *thermal_tripless_zone_device_register(
> >                       const struct thermal_zone_params *tzp)
> >   { return ERR_PTR(-ENODEV); }
> > +static inline struct thermal_zone_device *thermal_zone_device_register(
> > +                    struct thermal_zone_device_params *tzdp)
> > +{ return ERR_PTR(-ENODEV); }
> > +
> >   static inline void thermal_zone_device_unregister(struct thermal_zone_device *tz)
> >   { }