| Message ID | 20220806152517.78159-1-andriy.shevchenko@linux.intel.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v1,1/3] hwmon: (pwm-fan) Make use of device properties | expand |
Hello,
[dropped Bartlomiej Zolnierkiewicz from Cc:, the address bounced in the
past]
at a quick glance this looks nice. I wonder if it makes sense to split
the patch. For example the change
- ctx->pwm = devm_of_pwm_get(dev, dev->of_node, NULL);
+ ctx->pwm = devm_pwm_get(dev, NULL);
could stand alone. Also I think this change is the relevant part in
patch #1 that makes patches #2 and #3 possible.
When this patch doesn't get split, the series needs some coordination,
as patch #1 is for hwmon and patches #2 and #3 are for pwm.
Splitting the series into:
hwmon: (pwm-fan) Use of devm_pwm_get() instead of devm_of_pwm_get()
pwm: core: Get rid of unused devm_of_pwm_get()
pwm: core: Make of_pwm_get() static
for pwm and the remainder of this patch for hwmon might make application
of the changes here easier to coordinate.
But still: Thanks for your effort and
Acked-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
Best regards
Uwe
On Sun, Aug 7, 2022 at 10:38 AM Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote: > at a quick glance this looks nice. I wonder if it makes sense to split > the patch. For example the change > > - ctx->pwm = devm_of_pwm_get(dev, dev->of_node, NULL); > + ctx->pwm = devm_pwm_get(dev, NULL); > > could stand alone. Also I think this change is the relevant part in > patch #1 that makes patches #2 and #3 possible. True. > When this patch doesn't get split, the series needs some coordination, > as patch #1 is for hwmon and patches #2 and #3 are for pwm. > > Splitting the series into: > > hwmon: (pwm-fan) Use of devm_pwm_get() instead of devm_of_pwm_get() > pwm: core: Get rid of unused devm_of_pwm_get() > pwm: core: Make of_pwm_get() static > > for pwm and the remainder of this patch for hwmon might make application > of the changes here easier to coordinate. Either way it will need the hwmon maintainer ACKs or alike. Since we have (plenty of) time I will wait a bit for hwmon maintainers to react. Guenter, what would you prefer? > But still: Thanks for your effort and > Acked-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> Thanks for looking into the series related to PWM core clean up!
On 8/7/22 02:20, Andy Shevchenko wrote: > On Sun, Aug 7, 2022 at 10:38 AM Uwe Kleine-König > <u.kleine-koenig@pengutronix.de> wrote: > >> at a quick glance this looks nice. I wonder if it makes sense to split >> the patch. For example the change >> >> - ctx->pwm = devm_of_pwm_get(dev, dev->of_node, NULL); >> + ctx->pwm = devm_pwm_get(dev, NULL); >> >> could stand alone. Also I think this change is the relevant part in >> patch #1 that makes patches #2 and #3 possible. > > True. > >> When this patch doesn't get split, the series needs some coordination, >> as patch #1 is for hwmon and patches #2 and #3 are for pwm. >> >> Splitting the series into: >> >> hwmon: (pwm-fan) Use of devm_pwm_get() instead of devm_of_pwm_get() >> pwm: core: Get rid of unused devm_of_pwm_get() >> pwm: core: Make of_pwm_get() static >> >> for pwm and the remainder of this patch for hwmon might make application >> of the changes here easier to coordinate. > > Either way it will need the hwmon maintainer ACKs or alike. > Since we have (plenty of) time I will wait a bit for hwmon maintainers > to react. Guenter, what would you prefer? > I have a substantial number of patches pending for the pwm-fan driver. Some of those will conflict with this patch. I'll have to spend more time to be able to understand the implications. Guenter
On Sat, Aug 06, 2022 at 06:25:15PM +0300, Andy Shevchenko wrote: > Convert the module to be property provider agnostic and allow > it to be used on non-OF platforms. > > Add mod_devicetable.h include. > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > Acked-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> I had another look at this patch. A substantial part of the changes is because device properties don't support of_property_read_u32_index(), reworking the code to use device_property_read_u32_array() instead. Sorry, I don't like it, it results in a substantial number of unnecessary changes. Device properties should support the equivalent of of_property_read_u32_index() instead to simplify conversions. Guenter > --- > drivers/hwmon/Kconfig | 2 +- > drivers/hwmon/pwm-fan.c | 50 +++++++++++++++++++++-------------------- > 2 files changed, 27 insertions(+), 25 deletions(-) > > diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig > index e70d9614bec2..58912a5c5de8 100644 > --- a/drivers/hwmon/Kconfig > +++ b/drivers/hwmon/Kconfig > @@ -1613,7 +1613,7 @@ source "drivers/hwmon/pmbus/Kconfig" > > config SENSORS_PWM_FAN > tristate "PWM fan" > - depends on (PWM && OF) || COMPILE_TEST > + depends on PWM || COMPILE_TEST > depends on THERMAL || THERMAL=n > help > If you say yes here you get support for fans connected to PWM lines. > diff --git a/drivers/hwmon/pwm-fan.c b/drivers/hwmon/pwm-fan.c > index 6c08551d8d14..9ce9f2543861 100644 > --- a/drivers/hwmon/pwm-fan.c > +++ b/drivers/hwmon/pwm-fan.c > @@ -9,10 +9,11 @@ > > #include <linux/hwmon.h> > #include <linux/interrupt.h> > +#include <linux/mod_devicetable.h> > #include <linux/module.h> > #include <linux/mutex.h> > -#include <linux/of.h> > #include <linux/platform_device.h> > +#include <linux/property.h> > #include <linux/pwm.h> > #include <linux/regulator/consumer.h> > #include <linux/sysfs.h> > @@ -25,7 +26,6 @@ struct pwm_fan_tach { > int irq; > atomic_t pulses; > unsigned int rpm; > - u8 pulses_per_revolution; > }; > > struct pwm_fan_ctx { > @@ -36,6 +36,7 @@ struct pwm_fan_ctx { > > int tach_count; > struct pwm_fan_tach *tachs; > + u32 *pulses_per_revolution; > ktime_t sample_start; > struct timer_list rpm_timer; > > @@ -73,7 +74,7 @@ static void sample_timer(struct timer_list *t) > pulses = atomic_read(&tach->pulses); > atomic_sub(pulses, &tach->pulses); > tach->rpm = (unsigned int)(pulses * 1000 * 60) / > - (tach->pulses_per_revolution * delta); > + (ctx->pulses_per_revolution[i] * delta); > } > > ctx->sample_start = ktime_get(); > @@ -229,16 +230,14 @@ static const struct thermal_cooling_device_ops pwm_fan_cooling_ops = { > .set_cur_state = pwm_fan_set_cur_state, > }; > > -static int pwm_fan_of_get_cooling_data(struct device *dev, > - struct pwm_fan_ctx *ctx) > +static int pwm_fan_get_cooling_data(struct device *dev, struct pwm_fan_ctx *ctx) > { > - struct device_node *np = dev->of_node; > int num, i, ret; > > - if (!of_find_property(np, "cooling-levels", NULL)) > + if (!device_property_present(dev, "cooling-levels")) > return 0; > > - ret = of_property_count_u32_elems(np, "cooling-levels"); > + ret = device_property_count_u32(dev, "cooling-levels"); > if (ret <= 0) { > dev_err(dev, "Wrong data!\n"); > return ret ? : -EINVAL; > @@ -250,8 +249,8 @@ static int pwm_fan_of_get_cooling_data(struct device *dev, > if (!ctx->pwm_fan_cooling_levels) > return -ENOMEM; > > - ret = of_property_read_u32_array(np, "cooling-levels", > - ctx->pwm_fan_cooling_levels, num); > + ret = device_property_read_u32_array(dev, "cooling-levels", > + ctx->pwm_fan_cooling_levels, num); > if (ret) { > dev_err(dev, "Property 'cooling-levels' cannot be read!\n"); > return ret; > @@ -302,7 +301,7 @@ static int pwm_fan_probe(struct platform_device *pdev) > > mutex_init(&ctx->lock); > > - ctx->pwm = devm_of_pwm_get(dev, dev->of_node, NULL); > + ctx->pwm = devm_pwm_get(dev, NULL); > if (IS_ERR(ctx->pwm)) > return dev_err_probe(dev, PTR_ERR(ctx->pwm), "Could not get PWM\n"); > > @@ -370,6 +369,20 @@ static int pwm_fan_probe(struct platform_device *pdev) > if (!fan_channel_config) > return -ENOMEM; > ctx->fan_channel.config = fan_channel_config; > + > + ctx->pulses_per_revolution = devm_kmalloc_array(dev, > + ctx->tach_count, > + sizeof(*ctx->pulses_per_revolution), > + GFP_KERNEL); > + if (!ctx->pulses_per_revolution) > + return -ENOMEM; > + > + /* Setup default pulses per revolution */ > + memset32(ctx->pulses_per_revolution, 2, ctx->tach_count); > + > + device_property_read_u32_array(dev, "pulses-per-revolution", > + ctx->pulses_per_revolution, > + ctx->tach_count); > } > > channels = devm_kcalloc(dev, channel_count + 1, > @@ -381,7 +394,6 @@ static int pwm_fan_probe(struct platform_device *pdev) > > for (i = 0; i < ctx->tach_count; i++) { > struct pwm_fan_tach *tach = &ctx->tachs[i]; > - u32 ppr = 2; > > tach->irq = platform_get_irq(pdev, i); > if (tach->irq == -EPROBE_DEFER) > @@ -397,20 +409,10 @@ static int pwm_fan_probe(struct platform_device *pdev) > } > } > > - of_property_read_u32_index(dev->of_node, > - "pulses-per-revolution", > - i, > - &ppr); > - tach->pulses_per_revolution = ppr; > - if (!tach->pulses_per_revolution) { > - dev_err(dev, "pulses-per-revolution can't be zero.\n"); > - return -EINVAL; > - } > - > fan_channel_config[i] = HWMON_F_INPUT; > > dev_dbg(dev, "tach%d: irq=%d, pulses_per_revolution=%d\n", > - i, tach->irq, tach->pulses_per_revolution); > + i, tach->irq, ctx->pulses_per_revolution[i]); > } > > if (ctx->tach_count > 0) { > @@ -430,7 +432,7 @@ static int pwm_fan_probe(struct platform_device *pdev) > return PTR_ERR(hwmon); > } > > - ret = pwm_fan_of_get_cooling_data(dev, ctx); > + ret = pwm_fan_get_cooling_data(dev, ctx); > if (ret) > return ret; >
On Fri, Aug 19, 2022 at 2:41 AM Guenter Roeck <linux@roeck-us.net> wrote: > On Sat, Aug 06, 2022 at 06:25:15PM +0300, Andy Shevchenko wrote: > > Convert the module to be property provider agnostic and allow > > it to be used on non-OF platforms. > > > > Add mod_devicetable.h include. > > > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > > Acked-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> > > I had another look at this patch. A substantial part of the changes > is because device properties don't support of_property_read_u32_index(), > reworking the code to use device_property_read_u32_array() instead. > Sorry, I don't like it, it results in a substantial number of unnecessary > changes. Device properties should support the equivalent of > of_property_read_u32_index() instead to simplify conversions. Not all (device property) providers can have such API available. Are you suggesting to a) alloc memory for entire array; b) cache one for a given index; c) free a memory; d) loop as many times as index op is called. Sorry, this is way too far and non-optimal in comparison to the substantial number of unnecessary changes (two or three small refactorings?). Another way is to provide a pwm-fan-acpi, which will be the copy of the driver after this patch is applied. I don't think it's a very bright idea either.
On Fri, Aug 19, 2022 at 12:56 PM Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > On Fri, Aug 19, 2022 at 2:41 AM Guenter Roeck <linux@roeck-us.net> wrote: > > On Sat, Aug 06, 2022 at 06:25:15PM +0300, Andy Shevchenko wrote: > > > Convert the module to be property provider agnostic and allow > > > it to be used on non-OF platforms. > > > > > > Add mod_devicetable.h include. > > > > > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > > > Acked-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> > > > > I had another look at this patch. A substantial part of the changes > > is because device properties don't support of_property_read_u32_index(), > > reworking the code to use device_property_read_u32_array() instead. > > Sorry, I don't like it, it results in a substantial number of unnecessary > > changes. Device properties should support the equivalent of > > of_property_read_u32_index() instead to simplify conversions. > > Not all (device property) providers can have such API available. Are > you suggesting to > a) alloc memory for entire array; > b) cache one for a given index; > c) free a memory; > d) loop as many times as index op is called. > > Sorry, this is way too far and non-optimal in comparison to the > substantial number of unnecessary changes (two or three small > refactorings?). > > Another way is to provide a pwm-fan-acpi, which will be the copy of > the driver after this patch is applied. I don't think it's a very > bright idea either. That said, I will split a change for PWM cleaning up series and leave the rest on the hwmon maintainers to reconsider.
On Fri, Aug 19, 2022 at 12:56:42PM +0300, Andy Shevchenko wrote: > On Fri, Aug 19, 2022 at 2:41 AM Guenter Roeck <linux@roeck-us.net> wrote: > > On Sat, Aug 06, 2022 at 06:25:15PM +0300, Andy Shevchenko wrote: > > > Convert the module to be property provider agnostic and allow > > > it to be used on non-OF platforms. > > > > > > Add mod_devicetable.h include. > > > > > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > > > Acked-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> > > > > I had another look at this patch. A substantial part of the changes > > is because device properties don't support of_property_read_u32_index(), > > reworking the code to use device_property_read_u32_array() instead. > > Sorry, I don't like it, it results in a substantial number of unnecessary > > changes. Device properties should support the equivalent of > > of_property_read_u32_index() instead to simplify conversions. > > Not all (device property) providers can have such API available. Are > you suggesting to > a) alloc memory for entire array; > b) cache one for a given index; > c) free a memory; > d) loop as many times as index op is called. > > Sorry, this is way too far and non-optimal in comparison to the > substantial number of unnecessary changes (two or three small > refactorings?). > > Another way is to provide a pwm-fan-acpi, which will be the copy of > the driver after this patch is applied. I don't think it's a very > bright idea either. > An alternative might be to split the patch in two parts, one replacing of_property_read_u32_index() with of_property_read_u32_array() as preparation, with the above rationale and a note that this is to prepare for the switch to device properties, and then the actual device property switch. Some context showing how other conversions handled this problem would also be nice, though not necessary. Thanks, Guenter
On Fri, Aug 19, 2022 at 4:09 PM Guenter Roeck <linux@roeck-us.net> wrote: > > On Fri, Aug 19, 2022 at 12:56:42PM +0300, Andy Shevchenko wrote: > > On Fri, Aug 19, 2022 at 2:41 AM Guenter Roeck <linux@roeck-us.net> wrote: > > > On Sat, Aug 06, 2022 at 06:25:15PM +0300, Andy Shevchenko wrote: > > > > Convert the module to be property provider agnostic and allow > > > > it to be used on non-OF platforms. > > > > > > > > Add mod_devicetable.h include. > > > > > > > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > > > > Acked-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> > > > > > > I had another look at this patch. A substantial part of the changes > > > is because device properties don't support of_property_read_u32_index(), > > > reworking the code to use device_property_read_u32_array() instead. > > > Sorry, I don't like it, it results in a substantial number of unnecessary > > > changes. Device properties should support the equivalent of > > > of_property_read_u32_index() instead to simplify conversions. > > > > Not all (device property) providers can have such API available. Are > > you suggesting to > > a) alloc memory for entire array; > > b) cache one for a given index; > > c) free a memory; > > d) loop as many times as index op is called. > > > > Sorry, this is way too far and non-optimal in comparison to the > > substantial number of unnecessary changes (two or three small > > refactorings?). > > > > Another way is to provide a pwm-fan-acpi, which will be the copy of > > the driver after this patch is applied. I don't think it's a very > > bright idea either. > > > An alternative might be to split the patch in two parts, one replacing > of_property_read_u32_index() with of_property_read_u32_array() as > preparation, with the above rationale and a note that this is to > prepare for the switch to device properties, and then the actual device > property switch. Some context showing how other conversions handled this > problem would also be nice, though not necessary. Thanks for the idea, I like it and it would indeed simplify the understanding of the changes made.
diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig index e70d9614bec2..58912a5c5de8 100644 --- a/drivers/hwmon/Kconfig +++ b/drivers/hwmon/Kconfig @@ -1613,7 +1613,7 @@ source "drivers/hwmon/pmbus/Kconfig" config SENSORS_PWM_FAN tristate "PWM fan" - depends on (PWM && OF) || COMPILE_TEST + depends on PWM || COMPILE_TEST depends on THERMAL || THERMAL=n help If you say yes here you get support for fans connected to PWM lines. diff --git a/drivers/hwmon/pwm-fan.c b/drivers/hwmon/pwm-fan.c index 6c08551d8d14..9ce9f2543861 100644 --- a/drivers/hwmon/pwm-fan.c +++ b/drivers/hwmon/pwm-fan.c @@ -9,10 +9,11 @@ #include <linux/hwmon.h> #include <linux/interrupt.h> +#include <linux/mod_devicetable.h> #include <linux/module.h> #include <linux/mutex.h> -#include <linux/of.h> #include <linux/platform_device.h> +#include <linux/property.h> #include <linux/pwm.h> #include <linux/regulator/consumer.h> #include <linux/sysfs.h> @@ -25,7 +26,6 @@ struct pwm_fan_tach { int irq; atomic_t pulses; unsigned int rpm; - u8 pulses_per_revolution; }; struct pwm_fan_ctx { @@ -36,6 +36,7 @@ struct pwm_fan_ctx { int tach_count; struct pwm_fan_tach *tachs; + u32 *pulses_per_revolution; ktime_t sample_start; struct timer_list rpm_timer; @@ -73,7 +74,7 @@ static void sample_timer(struct timer_list *t) pulses = atomic_read(&tach->pulses); atomic_sub(pulses, &tach->pulses); tach->rpm = (unsigned int)(pulses * 1000 * 60) / - (tach->pulses_per_revolution * delta); + (ctx->pulses_per_revolution[i] * delta); } ctx->sample_start = ktime_get(); @@ -229,16 +230,14 @@ static const struct thermal_cooling_device_ops pwm_fan_cooling_ops = { .set_cur_state = pwm_fan_set_cur_state, }; -static int pwm_fan_of_get_cooling_data(struct device *dev, - struct pwm_fan_ctx *ctx) +static int pwm_fan_get_cooling_data(struct device *dev, struct pwm_fan_ctx *ctx) { - struct device_node *np = dev->of_node; int num, i, ret; - if (!of_find_property(np, "cooling-levels", NULL)) + if (!device_property_present(dev, "cooling-levels")) return 0; - ret = of_property_count_u32_elems(np, "cooling-levels"); + ret = device_property_count_u32(dev, "cooling-levels"); if (ret <= 0) { dev_err(dev, "Wrong data!\n"); return ret ? : -EINVAL; @@ -250,8 +249,8 @@ static int pwm_fan_of_get_cooling_data(struct device *dev, if (!ctx->pwm_fan_cooling_levels) return -ENOMEM; - ret = of_property_read_u32_array(np, "cooling-levels", - ctx->pwm_fan_cooling_levels, num); + ret = device_property_read_u32_array(dev, "cooling-levels", + ctx->pwm_fan_cooling_levels, num); if (ret) { dev_err(dev, "Property 'cooling-levels' cannot be read!\n"); return ret; @@ -302,7 +301,7 @@ static int pwm_fan_probe(struct platform_device *pdev) mutex_init(&ctx->lock); - ctx->pwm = devm_of_pwm_get(dev, dev->of_node, NULL); + ctx->pwm = devm_pwm_get(dev, NULL); if (IS_ERR(ctx->pwm)) return dev_err_probe(dev, PTR_ERR(ctx->pwm), "Could not get PWM\n"); @@ -370,6 +369,20 @@ static int pwm_fan_probe(struct platform_device *pdev) if (!fan_channel_config) return -ENOMEM; ctx->fan_channel.config = fan_channel_config; + + ctx->pulses_per_revolution = devm_kmalloc_array(dev, + ctx->tach_count, + sizeof(*ctx->pulses_per_revolution), + GFP_KERNEL); + if (!ctx->pulses_per_revolution) + return -ENOMEM; + + /* Setup default pulses per revolution */ + memset32(ctx->pulses_per_revolution, 2, ctx->tach_count); + + device_property_read_u32_array(dev, "pulses-per-revolution", + ctx->pulses_per_revolution, + ctx->tach_count); } channels = devm_kcalloc(dev, channel_count + 1, @@ -381,7 +394,6 @@ static int pwm_fan_probe(struct platform_device *pdev) for (i = 0; i < ctx->tach_count; i++) { struct pwm_fan_tach *tach = &ctx->tachs[i]; - u32 ppr = 2; tach->irq = platform_get_irq(pdev, i); if (tach->irq == -EPROBE_DEFER) @@ -397,20 +409,10 @@ static int pwm_fan_probe(struct platform_device *pdev) } } - of_property_read_u32_index(dev->of_node, - "pulses-per-revolution", - i, - &ppr); - tach->pulses_per_revolution = ppr; - if (!tach->pulses_per_revolution) { - dev_err(dev, "pulses-per-revolution can't be zero.\n"); - return -EINVAL; - } - fan_channel_config[i] = HWMON_F_INPUT; dev_dbg(dev, "tach%d: irq=%d, pulses_per_revolution=%d\n", - i, tach->irq, tach->pulses_per_revolution); + i, tach->irq, ctx->pulses_per_revolution[i]); } if (ctx->tach_count > 0) { @@ -430,7 +432,7 @@ static int pwm_fan_probe(struct platform_device *pdev) return PTR_ERR(hwmon); } - ret = pwm_fan_of_get_cooling_data(dev, ctx); + ret = pwm_fan_get_cooling_data(dev, ctx); if (ret) return ret;
Convert the module to be property provider agnostic and allow it to be used on non-OF platforms. Add mod_devicetable.h include. Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> --- drivers/hwmon/Kconfig | 2 +- drivers/hwmon/pwm-fan.c | 50 +++++++++++++++++++++-------------------- 2 files changed, 27 insertions(+), 25 deletions(-)