Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Sender: Guenter Roeck <groeck7@gmail.com>
Date:   Thu, 18 Aug 2022 16:20:40 -0700
From:   Guenter Roeck <linux@roeck-us.net>
To:     Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc:     Uwe =?iso-8859-1?Q?Kleine-K=F6nig?= 
        <u.kleine-koenig@pengutronix.de>, linux-doc@vger.kernel.org,
        linux-kernel@vger.kernel.org, linux-pwm@vger.kernel.org,
        linux-hwmon@vger.kernel.org, Jonathan Corbet <corbet@lwn.net>,
        Thierry Reding <thierry.reding@gmail.com>,
        Jean Delvare <jdelvare@suse.com>,
        Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>
Subject: Re: [PATCH v1 1/3] hwmon: (pwm-fan) Make use of device properties
Message-ID: <20220818232040.GA3505561@roeck-us.net>
References: <20220806152517.78159-1-andriy.shevchenko@linux.intel.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <20220806152517.78159-1-andriy.shevchenko@linux.intel.com>
Precedence: bulk

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