| Message ID | 20230221180710.2781027-7-daniel.lezcano@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Self-encapsulate the thermal zone device structure | expand |
Il 21/02/23 19:07, Daniel Lezcano ha scritto: > Some drivers are directly using the thermal zone's 'device' structure > field. > > Use the driver device pointer instead of the thermal zone device when > it is available. > > Remove the traces when they are duplicate with the traces in the core > code. > > Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org> > Reviewed-by: Balsam CHIHI <bchihi@baylibre.com> #Mediatek LVTS Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com> #MediaTek LVTS
On Tue, Feb 21, 2023 at 7:07 PM Daniel Lezcano <daniel.lezcano@linaro.org> wrote: > > Some drivers are directly using the thermal zone's 'device' structure > field. > > Use the driver device pointer instead of the thermal zone device when > it is available. > > Remove the traces when they are duplicate with the traces in the core > code. > > Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org> > Reviewed-by: Balsam CHIHI <bchihi@baylibre.com> #Mediatek LVTS > --- > drivers/thermal/imx_thermal.c | 37 ++++--------------- > drivers/thermal/mediatek/lvts_thermal.c | 4 +- > drivers/thermal/thermal_hwmon.c | 4 +- > .../ti-soc-thermal/ti-thermal-common.c | 2 +- > 4 files changed, 13 insertions(+), 34 deletions(-) > > diff --git a/drivers/thermal/imx_thermal.c b/drivers/thermal/imx_thermal.c > index a0b8de269986..c3136978adee 100644 > --- a/drivers/thermal/imx_thermal.c > +++ b/drivers/thermal/imx_thermal.c > @@ -285,13 +285,13 @@ static int imx_get_temp(struct thermal_zone_device *tz, int *temp) > if (data->alarm_temp == trips[IMX_TRIP_CRITICAL].temperature && > *temp < trips[IMX_TRIP_PASSIVE].temperature) { > imx_set_alarm_temp(data, trips[IMX_TRIP_PASSIVE].temperature); > - dev_dbg(&tz->device, "thermal alarm off: T < %d\n", > + dev_dbg(data->dev, "thermal alarm off: T < %d\n", > data->alarm_temp / 1000); > } > } > > if (*temp != data->last_temp) { > - dev_dbg(&tz->device, "millicelsius: %d\n", *temp); > + dev_dbg(data->dev, "millicelsius: %d\n", *temp); > data->last_temp = *temp; > } > > @@ -367,36 +367,16 @@ static int imx_set_trip_temp(struct thermal_zone_device *tz, int trip, > static int imx_bind(struct thermal_zone_device *tz, > struct thermal_cooling_device *cdev) > { > - int ret; > - > - ret = thermal_zone_bind_cooling_device(tz, IMX_TRIP_PASSIVE, cdev, > - THERMAL_NO_LIMIT, > - THERMAL_NO_LIMIT, > - THERMAL_WEIGHT_DEFAULT); > - if (ret) { > - dev_err(&tz->device, > - "binding zone %s with cdev %s failed:%d\n", > - tz->type, cdev->type, ret); > - return ret; > - } > - > - return 0; > + return thermal_zone_bind_cooling_device(tz, IMX_TRIP_PASSIVE, cdev, > + THERMAL_NO_LIMIT, > + THERMAL_NO_LIMIT, > + THERMAL_WEIGHT_DEFAULT); > } > > static int imx_unbind(struct thermal_zone_device *tz, > struct thermal_cooling_device *cdev) > { > - int ret; > - > - ret = thermal_zone_unbind_cooling_device(tz, IMX_TRIP_PASSIVE, cdev); > - if (ret) { > - dev_err(&tz->device, > - "unbinding zone %s with cdev %s failed:%d\n", > - tz->type, cdev->type, ret); > - return ret; > - } > - > - return 0; > + return thermal_zone_unbind_cooling_device(tz, IMX_TRIP_PASSIVE, cdev); > } > > static struct thermal_zone_device_ops imx_tz_ops = { > @@ -558,8 +538,7 @@ static irqreturn_t imx_thermal_alarm_irq_thread(int irq, void *dev) > { > struct imx_thermal_data *data = dev; > > - dev_dbg(&data->tz->device, "THERMAL ALARM: T > %d\n", > - data->alarm_temp / 1000); > + dev_dbg(data->dev, "THERMAL ALARM: T > %d\n", data->alarm_temp / 1000); > > thermal_zone_device_update(data->tz, THERMAL_EVENT_UNSPECIFIED); > > diff --git a/drivers/thermal/mediatek/lvts_thermal.c b/drivers/thermal/mediatek/lvts_thermal.c > index fb4b1b4db245..beb835d644e2 100644 > --- a/drivers/thermal/mediatek/lvts_thermal.c > +++ b/drivers/thermal/mediatek/lvts_thermal.c > @@ -305,7 +305,7 @@ static int lvts_set_trips(struct thermal_zone_device *tz, int low, int high) > * 14-0 : Raw temperature for threshold > */ > if (low != -INT_MAX) { > - dev_dbg(&tz->device, "Setting low limit temperature interrupt: %d\n", low); > + pr_debug("%s: Setting low limit temperature interrupt: %d\n", tz->type, low); > writel(raw_low, LVTS_H2NTHRE(base)); > } > > @@ -318,7 +318,7 @@ static int lvts_set_trips(struct thermal_zone_device *tz, int low, int high) > * > * 14-0 : Raw temperature for threshold > */ > - dev_dbg(&tz->device, "Setting high limit temperature interrupt: %d\n", high); > + pr_debug("%s: Setting high limit temperature interrupt: %d\n", tz->type, high); > writel(raw_high, LVTS_HTHRE(base)); > > return 0; > diff --git a/drivers/thermal/thermal_hwmon.c b/drivers/thermal/thermal_hwmon.c > index 964db7941e31..bc02095b314c 100644 > --- a/drivers/thermal/thermal_hwmon.c > +++ b/drivers/thermal/thermal_hwmon.c > @@ -228,14 +228,14 @@ void thermal_remove_hwmon_sysfs(struct thermal_zone_device *tz) > hwmon = thermal_hwmon_lookup_by_type(tz); > if (unlikely(!hwmon)) { > /* Should never happen... */ > - dev_dbg(&tz->device, "hwmon device lookup failed!\n"); > + dev_dbg(hwmon->device, "hwmon device lookup failed!\n"); > return; > } > > temp = thermal_hwmon_lookup_temp(hwmon, tz); > if (unlikely(!temp)) { > /* Should never happen... */ > - dev_dbg(&tz->device, "temperature input lookup failed!\n"); > + dev_dbg(hwmon->device, "temperature input lookup failed!\n"); > return; > } > > diff --git a/drivers/thermal/ti-soc-thermal/ti-thermal-common.c b/drivers/thermal/ti-soc-thermal/ti-thermal-common.c > index 216b29068b08..060f46cea5ff 100644 > --- a/drivers/thermal/ti-soc-thermal/ti-thermal-common.c > +++ b/drivers/thermal/ti-soc-thermal/ti-thermal-common.c > @@ -43,7 +43,7 @@ static void ti_thermal_work(struct work_struct *work) > > thermal_zone_device_update(data->ti_thermal, THERMAL_EVENT_UNSPECIFIED); > > - dev_dbg(&data->ti_thermal->device, "updated thermal zone %s\n", > + dev_dbg(data->bgp->dev, "updated thermal zone %s\n", > data->ti_thermal->type); The code before the change is more consistent, because it refers to the same object in both instances. It looks like a type field accessor is needed, eg. thermal_zone_device_type()? Or move the debug message to thermal_zone_device_update()? > } > > -- > 2.34.1 >
On 22/02/2023 20:43, Rafael J. Wysocki wrote: > On Tue, Feb 21, 2023 at 7:07 PM Daniel Lezcano > <daniel.lezcano@linaro.org> wrote: >> >> Some drivers are directly using the thermal zone's 'device' structure >> field. >> >> Use the driver device pointer instead of the thermal zone device when >> it is available. >> >> Remove the traces when they are duplicate with the traces in the core >> code. >> >> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org> >> Reviewed-by: Balsam CHIHI <bchihi@baylibre.com> #Mediatek LVTS >> --- [ ... ] >> thermal_zone_device_update(data->ti_thermal, THERMAL_EVENT_UNSPECIFIED); >> >> - dev_dbg(&data->ti_thermal->device, "updated thermal zone %s\n", >> + dev_dbg(data->bgp->dev, "updated thermal zone %s\n", >> data->ti_thermal->type); > > The code before the change is more consistent, because it refers to > the same object in both instances. > > It looks like a type field accessor is needed, eg. thermal_zone_device_type()? > > Or move the debug message to thermal_zone_device_update()? Actually it is done on purpose because the patch 9 replaces the accesses to 'type' by 'id', the thermal_zone_device_type() accessor won't be needed.
On Wed, Feb 22, 2023 at 9:00 PM Daniel Lezcano <daniel.lezcano@linaro.org> wrote: > > On 22/02/2023 20:43, Rafael J. Wysocki wrote: > > On Tue, Feb 21, 2023 at 7:07 PM Daniel Lezcano > > <daniel.lezcano@linaro.org> wrote: > >> > >> Some drivers are directly using the thermal zone's 'device' structure > >> field. > >> > >> Use the driver device pointer instead of the thermal zone device when > >> it is available. > >> > >> Remove the traces when they are duplicate with the traces in the core > >> code. > >> > >> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org> > >> Reviewed-by: Balsam CHIHI <bchihi@baylibre.com> #Mediatek LVTS > >> --- > > [ ... ] > > >> thermal_zone_device_update(data->ti_thermal, THERMAL_EVENT_UNSPECIFIED); > >> > >> - dev_dbg(&data->ti_thermal->device, "updated thermal zone %s\n", > >> + dev_dbg(data->bgp->dev, "updated thermal zone %s\n", > >> data->ti_thermal->type); > > > > The code before the change is more consistent, because it refers to > > the same object in both instances. > > > > It looks like a type field accessor is needed, eg. thermal_zone_device_type()? > > > > Or move the debug message to thermal_zone_device_update()? > > Actually it is done on purpose because the patch 9 replaces the accesses > to 'type' by 'id', the thermal_zone_device_type() accessor won't be needed. Cool. However, this is a change in behavior (albeit small) which doesn't appear to be necessary. What would be wrong with having a tz->type accessor too?
On 22/02/2023 21:06, Rafael J. Wysocki wrote: > On Wed, Feb 22, 2023 at 9:00 PM Daniel Lezcano > <daniel.lezcano@linaro.org> wrote: >> >> On 22/02/2023 20:43, Rafael J. Wysocki wrote: >>> On Tue, Feb 21, 2023 at 7:07 PM Daniel Lezcano >>> <daniel.lezcano@linaro.org> wrote: >>>> >>>> Some drivers are directly using the thermal zone's 'device' structure >>>> field. >>>> >>>> Use the driver device pointer instead of the thermal zone device when >>>> it is available. >>>> >>>> Remove the traces when they are duplicate with the traces in the core >>>> code. >>>> >>>> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org> >>>> Reviewed-by: Balsam CHIHI <bchihi@baylibre.com> #Mediatek LVTS >>>> --- >> >> [ ... ] >> >>>> thermal_zone_device_update(data->ti_thermal, THERMAL_EVENT_UNSPECIFIED); >>>> >>>> - dev_dbg(&data->ti_thermal->device, "updated thermal zone %s\n", >>>> + dev_dbg(data->bgp->dev, "updated thermal zone %s\n", >>>> data->ti_thermal->type); >>> >>> The code before the change is more consistent, because it refers to >>> the same object in both instances. >>> >>> It looks like a type field accessor is needed, eg. thermal_zone_device_type()? >>> >>> Or move the debug message to thermal_zone_device_update()? >> >> Actually it is done on purpose because the patch 9 replaces the accesses >> to 'type' by 'id', the thermal_zone_device_type() accessor won't be needed. > > Cool. > > However, this is a change in behavior (albeit small) which doesn't > appear to be necessary. > > What would be wrong with having a tz->type accessor too? I can add the 'type' accessor but from my point of view it is not correct because the information belongs to the thermal framework and it is used to export the information in the sysfs which is along with the directory name giving the id of the thermal zone. Actually, the useful information is the id of the thermal zone, not the type. This one can be duplicate, for instance: cat /sys/class/thermal/thermal_zone*/type acpitz acpitz Given there are few places where 'type' is used in the drivers, I prefer to directly change that to 'id' in the next patch instead of creating the accessor for 'type', then send another series removing it.
On Thu, Feb 23, 2023 at 10:56 AM Daniel Lezcano <daniel.lezcano@linaro.org> wrote: > > On 22/02/2023 21:06, Rafael J. Wysocki wrote: > > On Wed, Feb 22, 2023 at 9:00 PM Daniel Lezcano > > <daniel.lezcano@linaro.org> wrote: > >> > >> On 22/02/2023 20:43, Rafael J. Wysocki wrote: > >>> On Tue, Feb 21, 2023 at 7:07 PM Daniel Lezcano > >>> <daniel.lezcano@linaro.org> wrote: > >>>> > >>>> Some drivers are directly using the thermal zone's 'device' structure > >>>> field. > >>>> > >>>> Use the driver device pointer instead of the thermal zone device when > >>>> it is available. > >>>> > >>>> Remove the traces when they are duplicate with the traces in the core > >>>> code. > >>>> > >>>> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org> > >>>> Reviewed-by: Balsam CHIHI <bchihi@baylibre.com> #Mediatek LVTS > >>>> --- > >> > >> [ ... ] > >> > >>>> thermal_zone_device_update(data->ti_thermal, THERMAL_EVENT_UNSPECIFIED); > >>>> > >>>> - dev_dbg(&data->ti_thermal->device, "updated thermal zone %s\n", > >>>> + dev_dbg(data->bgp->dev, "updated thermal zone %s\n", > >>>> data->ti_thermal->type); > >>> > >>> The code before the change is more consistent, because it refers to > >>> the same object in both instances. > >>> > >>> It looks like a type field accessor is needed, eg. thermal_zone_device_type()? > >>> > >>> Or move the debug message to thermal_zone_device_update()? > >> > >> Actually it is done on purpose because the patch 9 replaces the accesses > >> to 'type' by 'id', the thermal_zone_device_type() accessor won't be needed. > > > > Cool. > > > > However, this is a change in behavior (albeit small) which doesn't > > appear to be necessary. > > > > What would be wrong with having a tz->type accessor too? > > I can add the 'type' accessor but from my point of view it is not > correct because the information belongs to the thermal framework and it > is used to export the information in the sysfs which is along with the > directory name giving the id of the thermal zone. I'm not sure what you mean here. Surely, the 'type' is provided by whoever registers the thermal zone, so I'm not sure in what way it "belongs" to the framework. > Actually, the useful information is the id of the thermal zone, not the > type. This one can be duplicate, for instance: > > cat /sys/class/thermal/thermal_zone*/type > acpitz > acpitz That's correct, but in the particular case of DT-based systems it comes from the DT (AFAICT) and so it allows to connect the kernel message with the DT contents. The id could be used for that, but that involves an extra sysfs lookup. > Given there are few places where 'type' is used in the drivers, I prefer > to directly change that to 'id' in the next patch instead of creating > the accessor for 'type', then send another series removing it. So you are going to change the behavior of those few places with the only reason being aesthetics AFAICS. Is this really a good enough reason to do that?
On 23/02/2023 12:43, Rafael J. Wysocki wrote: > On Thu, Feb 23, 2023 at 10:56 AM Daniel Lezcano > <daniel.lezcano@linaro.org> wrote: >> >> On 22/02/2023 21:06, Rafael J. Wysocki wrote: >>> On Wed, Feb 22, 2023 at 9:00 PM Daniel Lezcano >>> <daniel.lezcano@linaro.org> wrote: >>>> >>>> On 22/02/2023 20:43, Rafael J. Wysocki wrote: >>>>> On Tue, Feb 21, 2023 at 7:07 PM Daniel Lezcano >>>>> <daniel.lezcano@linaro.org> wrote: >>>>>> >>>>>> Some drivers are directly using the thermal zone's 'device' structure >>>>>> field. >>>>>> >>>>>> Use the driver device pointer instead of the thermal zone device when >>>>>> it is available. >>>>>> >>>>>> Remove the traces when they are duplicate with the traces in the core >>>>>> code. >>>>>> >>>>>> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org> >>>>>> Reviewed-by: Balsam CHIHI <bchihi@baylibre.com> #Mediatek LVTS >>>>>> --- >>>> >>>> [ ... ] >>>> >>>>>> thermal_zone_device_update(data->ti_thermal, THERMAL_EVENT_UNSPECIFIED); >>>>>> >>>>>> - dev_dbg(&data->ti_thermal->device, "updated thermal zone %s\n", >>>>>> + dev_dbg(data->bgp->dev, "updated thermal zone %s\n", >>>>>> data->ti_thermal->type); >>>>> >>>>> The code before the change is more consistent, because it refers to >>>>> the same object in both instances. >>>>> >>>>> It looks like a type field accessor is needed, eg. thermal_zone_device_type()? >>>>> >>>>> Or move the debug message to thermal_zone_device_update()? >>>> >>>> Actually it is done on purpose because the patch 9 replaces the accesses >>>> to 'type' by 'id', the thermal_zone_device_type() accessor won't be needed. >>> >>> Cool. >>> >>> However, this is a change in behavior (albeit small) which doesn't >>> appear to be necessary. >>> >>> What would be wrong with having a tz->type accessor too? >> >> I can add the 'type' accessor but from my point of view it is not >> correct because the information belongs to the thermal framework and it >> is used to export the information in the sysfs which is along with the >> directory name giving the id of the thermal zone. > > I'm not sure what you mean here. > > Surely, the 'type' is provided by whoever registers the thermal zone, > so I'm not sure in what way it "belongs" to the framework. I meant the goal of 'type' is to be exported to sysfs, nothing else. That is the reason why I used the word 'belongs', because it was introduced to stay in the scope of the thermal framework, but then its usage has been diverted to a name. Anyway, from my POV having traces in the ops is not a good thing, so I'll propose later to remove them and add a single message in the call sites. Meanwhile, I'll provide the accessor for 'type' and hopefully we do not end up with a plethora of accessors to be used in the core code.
diff --git a/drivers/thermal/imx_thermal.c b/drivers/thermal/imx_thermal.c index a0b8de269986..c3136978adee 100644 --- a/drivers/thermal/imx_thermal.c +++ b/drivers/thermal/imx_thermal.c @@ -285,13 +285,13 @@ static int imx_get_temp(struct thermal_zone_device *tz, int *temp) if (data->alarm_temp == trips[IMX_TRIP_CRITICAL].temperature && *temp < trips[IMX_TRIP_PASSIVE].temperature) { imx_set_alarm_temp(data, trips[IMX_TRIP_PASSIVE].temperature); - dev_dbg(&tz->device, "thermal alarm off: T < %d\n", + dev_dbg(data->dev, "thermal alarm off: T < %d\n", data->alarm_temp / 1000); } } if (*temp != data->last_temp) { - dev_dbg(&tz->device, "millicelsius: %d\n", *temp); + dev_dbg(data->dev, "millicelsius: %d\n", *temp); data->last_temp = *temp; } @@ -367,36 +367,16 @@ static int imx_set_trip_temp(struct thermal_zone_device *tz, int trip, static int imx_bind(struct thermal_zone_device *tz, struct thermal_cooling_device *cdev) { - int ret; - - ret = thermal_zone_bind_cooling_device(tz, IMX_TRIP_PASSIVE, cdev, - THERMAL_NO_LIMIT, - THERMAL_NO_LIMIT, - THERMAL_WEIGHT_DEFAULT); - if (ret) { - dev_err(&tz->device, - "binding zone %s with cdev %s failed:%d\n", - tz->type, cdev->type, ret); - return ret; - } - - return 0; + return thermal_zone_bind_cooling_device(tz, IMX_TRIP_PASSIVE, cdev, + THERMAL_NO_LIMIT, + THERMAL_NO_LIMIT, + THERMAL_WEIGHT_DEFAULT); } static int imx_unbind(struct thermal_zone_device *tz, struct thermal_cooling_device *cdev) { - int ret; - - ret = thermal_zone_unbind_cooling_device(tz, IMX_TRIP_PASSIVE, cdev); - if (ret) { - dev_err(&tz->device, - "unbinding zone %s with cdev %s failed:%d\n", - tz->type, cdev->type, ret); - return ret; - } - - return 0; + return thermal_zone_unbind_cooling_device(tz, IMX_TRIP_PASSIVE, cdev); } static struct thermal_zone_device_ops imx_tz_ops = { @@ -558,8 +538,7 @@ static irqreturn_t imx_thermal_alarm_irq_thread(int irq, void *dev) { struct imx_thermal_data *data = dev; - dev_dbg(&data->tz->device, "THERMAL ALARM: T > %d\n", - data->alarm_temp / 1000); + dev_dbg(data->dev, "THERMAL ALARM: T > %d\n", data->alarm_temp / 1000); thermal_zone_device_update(data->tz, THERMAL_EVENT_UNSPECIFIED); diff --git a/drivers/thermal/mediatek/lvts_thermal.c b/drivers/thermal/mediatek/lvts_thermal.c index fb4b1b4db245..beb835d644e2 100644 --- a/drivers/thermal/mediatek/lvts_thermal.c +++ b/drivers/thermal/mediatek/lvts_thermal.c @@ -305,7 +305,7 @@ static int lvts_set_trips(struct thermal_zone_device *tz, int low, int high) * 14-0 : Raw temperature for threshold */ if (low != -INT_MAX) { - dev_dbg(&tz->device, "Setting low limit temperature interrupt: %d\n", low); + pr_debug("%s: Setting low limit temperature interrupt: %d\n", tz->type, low); writel(raw_low, LVTS_H2NTHRE(base)); } @@ -318,7 +318,7 @@ static int lvts_set_trips(struct thermal_zone_device *tz, int low, int high) * * 14-0 : Raw temperature for threshold */ - dev_dbg(&tz->device, "Setting high limit temperature interrupt: %d\n", high); + pr_debug("%s: Setting high limit temperature interrupt: %d\n", tz->type, high); writel(raw_high, LVTS_HTHRE(base)); return 0; diff --git a/drivers/thermal/thermal_hwmon.c b/drivers/thermal/thermal_hwmon.c index 964db7941e31..bc02095b314c 100644 --- a/drivers/thermal/thermal_hwmon.c +++ b/drivers/thermal/thermal_hwmon.c @@ -228,14 +228,14 @@ void thermal_remove_hwmon_sysfs(struct thermal_zone_device *tz) hwmon = thermal_hwmon_lookup_by_type(tz); if (unlikely(!hwmon)) { /* Should never happen... */ - dev_dbg(&tz->device, "hwmon device lookup failed!\n"); + dev_dbg(hwmon->device, "hwmon device lookup failed!\n"); return; } temp = thermal_hwmon_lookup_temp(hwmon, tz); if (unlikely(!temp)) { /* Should never happen... */ - dev_dbg(&tz->device, "temperature input lookup failed!\n"); + dev_dbg(hwmon->device, "temperature input lookup failed!\n"); return; } diff --git a/drivers/thermal/ti-soc-thermal/ti-thermal-common.c b/drivers/thermal/ti-soc-thermal/ti-thermal-common.c index 216b29068b08..060f46cea5ff 100644 --- a/drivers/thermal/ti-soc-thermal/ti-thermal-common.c +++ b/drivers/thermal/ti-soc-thermal/ti-thermal-common.c @@ -43,7 +43,7 @@ static void ti_thermal_work(struct work_struct *work) thermal_zone_device_update(data->ti_thermal, THERMAL_EVENT_UNSPECIFIED); - dev_dbg(&data->ti_thermal->device, "updated thermal zone %s\n", + dev_dbg(data->bgp->dev, "updated thermal zone %s\n", data->ti_thermal->type); }