[PATCH v1 1/3] hwmon: (pwm-fan) Make use of device properties

[PATCH v1 1/3] hwmon: (pwm-fan) Make use of device properties

[Andy Shevchenko]

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(-)

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;
 
-- 
2.35.1

[PATCH v1 2/3] pwm: core: Get rid of unused devm_of_pwm_get()

[Andy Shevchenko]

The devm_of_pwm_get() has recently lost its single user, drop
the dead API as well.

Note, the new code should use either plain pwm_get() or managed
devm_pwm_get() or devm_fwnode_pwm_get() APIs.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 .../driver-api/driver-model/devres.rst        |  1 -
 Documentation/driver-api/pwm.rst              |  3 +-
 drivers/pwm/core.c                            | 30 -------------------
 include/linux/pwm.h                           | 10 -------
 4 files changed, 1 insertion(+), 43 deletions(-)

diff --git a/Documentation/driver-api/driver-model/devres.rst b/Documentation/driver-api/driver-model/devres.rst
index aeb3b2d7cc54..e431f1d746b6 100644
--- a/Documentation/driver-api/driver-model/devres.rst
+++ b/Documentation/driver-api/driver-model/devres.rst
@@ -410,7 +410,6 @@ POWER
 
 PWM
   devm_pwm_get()
-  devm_of_pwm_get()
   devm_fwnode_pwm_get()
 
 REGULATOR
diff --git a/Documentation/driver-api/pwm.rst b/Documentation/driver-api/pwm.rst
index fd26c3d895b6..8c71a2055d27 100644
--- a/Documentation/driver-api/pwm.rst
+++ b/Documentation/driver-api/pwm.rst
@@ -40,8 +40,7 @@ after usage with pwm_free().
 
 New users should use the pwm_get() function and pass to it the consumer
 device or a consumer name. pwm_put() is used to free the PWM device. Managed
-variants of the getter, devm_pwm_get(), devm_of_pwm_get(),
-devm_fwnode_pwm_get(), also exist.
+variants of the getter, devm_pwm_get() and devm_fwnode_pwm_get(), also exist.
 
 After being requested, a PWM has to be configured using::
 
diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
index 0e042410f6b9..dc1b7263a0b0 100644
--- a/drivers/pwm/core.c
+++ b/drivers/pwm/core.c
@@ -1070,36 +1070,6 @@ struct pwm_device *devm_pwm_get(struct device *dev, const char *con_id)
 }
 EXPORT_SYMBOL_GPL(devm_pwm_get);
 
-/**
- * devm_of_pwm_get() - resource managed of_pwm_get()
- * @dev: device for PWM consumer
- * @np: device node to get the PWM from
- * @con_id: consumer name
- *
- * This function performs like of_pwm_get() but the acquired PWM device will
- * automatically be released on driver detach.
- *
- * Returns: A pointer to the requested PWM device or an ERR_PTR()-encoded
- * error code on failure.
- */
-struct pwm_device *devm_of_pwm_get(struct device *dev, struct device_node *np,
-				   const char *con_id)
-{
-	struct pwm_device *pwm;
-	int ret;
-
-	pwm = of_pwm_get(dev, np, con_id);
-	if (IS_ERR(pwm))
-		return pwm;
-
-	ret = devm_add_action_or_reset(dev, devm_pwm_release, pwm);
-	if (ret)
-		return ERR_PTR(ret);
-
-	return pwm;
-}
-EXPORT_SYMBOL_GPL(devm_of_pwm_get);
-
 /**
  * devm_fwnode_pwm_get() - request a resource managed PWM from firmware node
  * @dev: device for PWM consumer
diff --git a/include/linux/pwm.h b/include/linux/pwm.h
index 9429930c5566..572ba92e4206 100644
--- a/include/linux/pwm.h
+++ b/include/linux/pwm.h
@@ -408,8 +408,6 @@ struct pwm_device *of_pwm_get(struct device *dev, struct device_node *np,
 void pwm_put(struct pwm_device *pwm);
 
 struct pwm_device *devm_pwm_get(struct device *dev, const char *con_id);
-struct pwm_device *devm_of_pwm_get(struct device *dev, struct device_node *np,
-				   const char *con_id);
 struct pwm_device *devm_fwnode_pwm_get(struct device *dev,
 				       struct fwnode_handle *fwnode,
 				       const char *con_id);
@@ -517,14 +515,6 @@ static inline struct pwm_device *devm_pwm_get(struct device *dev,
 	return ERR_PTR(-ENODEV);
 }
 
-static inline struct pwm_device *devm_of_pwm_get(struct device *dev,
-						 struct device_node *np,
-						 const char *con_id)
-{
-	might_sleep();
-	return ERR_PTR(-ENODEV);
-}
-
 static inline struct pwm_device *
 devm_fwnode_pwm_get(struct device *dev, struct fwnode_handle *fwnode,
 		    const char *con_id)
-- 
2.35.1

Re: [PATCH v1 2/3] pwm: core: Get rid of unused devm_of_pwm_get()

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 615 bytes --]

Hello,

On Sat, Aug 06, 2022 at 06:25:16PM +0300, Andy Shevchenko wrote:
> The devm_of_pwm_get() has recently lost its single user, drop
> the dead API as well.
> 
> Note, the new code should use either plain pwm_get() or managed
> devm_pwm_get() or devm_fwnode_pwm_get() APIs.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Acked-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

very nice!

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[PATCH v1 3/3] pwm: core: Make of_pwm_get() static

[Andy Shevchenko]

There are no users outside of PWM core of the of_pwm_get().
Make it static.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/pwm/core.c  |  5 ++---
 include/linux/pwm.h | 10 ----------
 2 files changed, 2 insertions(+), 13 deletions(-)

diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
index dc1b7263a0b0..cfe3a0327471 100644
--- a/drivers/pwm/core.c
+++ b/drivers/pwm/core.c
@@ -734,8 +734,8 @@ static struct device_link *pwm_device_link_add(struct device *dev,
  * Returns: A pointer to the requested PWM device or an ERR_PTR()-encoded
  * error code on failure.
  */
-struct pwm_device *of_pwm_get(struct device *dev, struct device_node *np,
-			      const char *con_id)
+static struct pwm_device *of_pwm_get(struct device *dev, struct device_node *np,
+				     const char *con_id)
 {
 	struct pwm_device *pwm = NULL;
 	struct of_phandle_args args;
@@ -797,7 +797,6 @@ struct pwm_device *of_pwm_get(struct device *dev, struct device_node *np,
 
 	return pwm;
 }
-EXPORT_SYMBOL_GPL(of_pwm_get);
 
 /**
  * acpi_pwm_get() - request a PWM via parsing "pwms" property in ACPI
diff --git a/include/linux/pwm.h b/include/linux/pwm.h
index 572ba92e4206..d70c6e5a839d 100644
--- a/include/linux/pwm.h
+++ b/include/linux/pwm.h
@@ -403,8 +403,6 @@ struct pwm_device *of_pwm_single_xlate(struct pwm_chip *pc,
 				       const struct of_phandle_args *args);
 
 struct pwm_device *pwm_get(struct device *dev, const char *con_id);
-struct pwm_device *of_pwm_get(struct device *dev, struct device_node *np,
-			      const char *con_id);
 void pwm_put(struct pwm_device *pwm);
 
 struct pwm_device *devm_pwm_get(struct device *dev, const char *con_id);
@@ -495,14 +493,6 @@ static inline struct pwm_device *pwm_get(struct device *dev,
 	return ERR_PTR(-ENODEV);
 }
 
-static inline struct pwm_device *of_pwm_get(struct device *dev,
-					    struct device_node *np,
-					    const char *con_id)
-{
-	might_sleep();
-	return ERR_PTR(-ENODEV);
-}
-
 static inline void pwm_put(struct pwm_device *pwm)
 {
 	might_sleep();
-- 
2.35.1

Re: [PATCH v1 3/3] pwm: core: Make of_pwm_get() static

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 475 bytes --]

Hello,

On Sat, Aug 06, 2022 at 06:25:17PM +0300, Andy Shevchenko wrote:
> There are no users outside of PWM core of the of_pwm_get().
> Make it static.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

Acked-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v1 1/3] hwmon: (pwm-fan) Make use of device properties

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 1141 bytes --]

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

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v1 1/3] hwmon: (pwm-fan) Make use of device properties

[Andy Shevchenko]

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!

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 1/3] hwmon: (pwm-fan) Make use of device properties

[Guenter Roeck]

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

Re: [PATCH v1 1/3] hwmon: (pwm-fan) Make use of device properties

[Guenter Roeck]

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;
>  

Re: [PATCH v1 1/3] hwmon: (pwm-fan) Make use of device properties

[Andy Shevchenko]

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.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 1/3] hwmon: (pwm-fan) Make use of device properties

[Andy Shevchenko]

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.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 1/3] hwmon: (pwm-fan) Make use of device properties

[Guenter Roeck]

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

Re: [PATCH v1 1/3] hwmon: (pwm-fan) Make use of device properties

[Andy Shevchenko]

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.

-- 
With Best Regards,
Andy Shevchenko