Re: [PATCH] drivers/thermal/thermal_of: Add change_mode ops support for thermal_of sensor

[Manaf Meethalavalappu Pallikunhi]

On 3/8/2022 11:28 PM, Daniel Lezcano wrote:
> On 08/03/2022 16:20, Manaf Meethalavalappu Pallikunhi wrote:
> > The sensor driver which register through thermal_of interface doesn't
> > have an option to get thermal zone mode change notification from
> > thermal core.
> >
> > Add support for change_mode ops in thermal_of interface so that sensor
> > driver can use this ops for mode change notification.
> >
> > Signed-off-by: Manaf Meethalavalappu Pallikunhi 
> > <quic_manafm@xxxxxxxxxxx>
> > ---
> >   drivers/thermal/thermal_of.c | 14 ++++++++++++++
> >   include/linux/thermal.h      |  3 +++
> >   2 files changed, 17 insertions(+)
> >
> > diff --git a/drivers/thermal/thermal_of.c b/drivers/thermal/thermal_of.c
> > index 9233f7e..a4e3820 100644
> > --- a/drivers/thermal/thermal_of.c
> > +++ b/drivers/thermal/thermal_of.c
> > @@ -203,6 +203,17 @@ static int of_thermal_get_trend(struct 
> > thermal_zone_device *tz, int trip,
> >       return data->ops->get_trend(data->sensor_data, trip, trend);
> >   }
> >   +static int of_thermal_change_mode(struct thermal_zone_device *tz,
> > +                enum thermal_device_mode mode)
> > +{
> > +    struct __thermal_zone *data = tz->devdata;
> > +
> > +    if (!data->ops || !data->ops->change_mode)
> > +        return -EINVAL;
>
>
> If this function is called it is because below in the bind function 
> ops->change_mode was true, so the condition above is always true, no ?

I agree, it is reduntant. I just followed other ops similar sanity 
check. I will remove it in next revision.

I think I missed to clear tzd->ops->change_mode  in 
tz_of_sensor_unregister() path. I will update that as well

> > +    return data->ops->change_mode(data->sensor_data, mode);
> > +}
> > +
> >   static int of_thermal_bind(struct thermal_zone_device *thermal,
> >                  struct thermal_cooling_device *cdev)
> >   {
> > @@ -408,6 +419,9 @@ thermal_zone_of_add_sensor(struct device_node *zone,
> >       if (ops->set_emul_temp)
> >           tzd->ops->set_emul_temp = of_thermal_set_emul_temp;
> >   +    if (ops->change_mode)
> > +        tzd->ops->change_mode = of_thermal_change_mode;
> > +
> >       mutex_unlock(&tzd->lock);
> >         return tzd;
> > diff --git a/include/linux/thermal.h b/include/linux/thermal.h
> > index c3148939..365733b 100644
> > --- a/include/linux/thermal.h
> > +++ b/include/linux/thermal.h
> > @@ -299,6 +299,8 @@ struct thermal_zone_params {
> >    *           temperature.
> >    * @set_trip_temp: a pointer to a function that sets the trip 
> > temperature on
> >    *           hardware.
> > + * @change_mode: a pointer to a function that notifies the thermal zone
> > + *           mode change.
> >    */
> >   struct thermal_zone_of_device_ops {
> >       int (*get_temp)(void *, int *);
> > @@ -306,6 +308,7 @@ struct thermal_zone_of_device_ops {
> >       int (*set_trips)(void *, int, int);
> >       int (*set_emul_temp)(void *, int);
> >       int (*set_trip_temp)(void *, int, int);
> > +    int (*change_mode) (void *, enum thermal_device_mode);
> >   };
> >     /* Function declarations */