Message-ID: <xu4eo5qnkg6ziahv34od3tgem5cjheraus5cly2kchmpeoo3aw@veuoqgiqps6k>
Date: Wed, 17 Apr 2024 09:04:49 +0200
From: Uwe Kleine-König <u.kleine-koenig@...gutronix.de>
To: Naresh Solanki <naresh.solanki@...ements.com>
Cc: Guenter Roeck <linux@...ck-us.net>, krzysztof.kozlowski+dt@...aro.org, 
	Jean Delvare <jdelvare@...e.com>, linux-hwmon@...r.kernel.org, linux-kernel@...r.kernel.org, 
	linux-pwm@...r.kernel.org
Subject: Re: [PATCH 2/4] hwmon: (max6639) : Utilise pwm subsystem

Hello,

On Tue, Apr 16, 2024 at 10:47:15PM +0530, Naresh Solanki wrote:
> Utilise pwm subsystem for fan pwm handling
> 
> Signed-off-by: Naresh Solanki <naresh.solanki@...ements.com>
> ---
>  drivers/hwmon/Kconfig   |   1 +
>  drivers/hwmon/max6639.c | 200 ++++++++++++++++++++++++++++++++++++++--
>  2 files changed, 191 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> index 257ec5360e35..c9cc74f8c807 100644
> --- a/drivers/hwmon/Kconfig
> +++ b/drivers/hwmon/Kconfig
> @@ -1224,6 +1224,7 @@ config SENSORS_MAX6639
>  	tristate "Maxim MAX6639 sensor chip"
>  	depends on I2C
>  	select REGMAP_I2C
> +	depends on PWM
>  	help
>  	  If you say yes here you get support for the MAX6639
>  	  sensor chips.
> diff --git a/drivers/hwmon/max6639.c b/drivers/hwmon/max6639.c
> index 1af93fc53cb5..f37fdd161154 100644
> --- a/drivers/hwmon/max6639.c
> +++ b/drivers/hwmon/max6639.c
> @@ -20,6 +20,7 @@
>  #include <linux/err.h>
>  #include <linux/mutex.h>
>  #include <linux/platform_data/max6639.h>
> +#include <linux/pwm.h>
>  #include <linux/regmap.h>
>  
>  /* Addresses to scan */
> @@ -55,6 +56,9 @@ static const unsigned short normal_i2c[] = { 0x2c, 0x2e, 0x2f, I2C_CLIENT_END };
>  #define MAX6639_GCONFIG_PWM_FREQ_HI		0x08
>  
>  #define MAX6639_FAN_CONFIG1_PWM			0x80
> +#define MAX6639_REG_FAN_CONFIG2a_PWM_POL	0x02
> +#define MAX6639_FAN_CONFIG3_FREQ_MASK		0x03
> +#define MAX6639_REG_TARGTDUTY_SLOT		120
>  
>  #define MAX6639_FAN_CONFIG3_THERM_FULL_SPEED	0x40
>  
> @@ -62,6 +66,10 @@ static const unsigned short normal_i2c[] = { 0x2c, 0x2e, 0x2f, I2C_CLIENT_END };
>  
>  static const int rpm_ranges[] = { 2000, 4000, 8000, 16000 };
>  
> +/* Supported PWM frequency */
> +static const unsigned int freq_table[] = { 20, 33, 50, 100, 5000, 8333, 12500,
> +					   25000 };
> +
>  #define FAN_FROM_REG(val, rpm_range)	((val) == 0 || (val) == 255 ? \
>  				0 : (rpm_ranges[rpm_range] * 30) / (val))
>  #define TEMP_LIMIT_TO_REG(val)	clamp_val((val) / 1000, 0, 255)
> @@ -93,6 +101,9 @@ struct max6639_data {
>  
>  	/* Optional regulator for FAN supply */
>  	struct regulator *reg;
> +	/* max6639 pwm chip */
> +	struct pwm_chip chip;

That won't work with the recent changes to the pwm framework. Please
test your patch on top of next.

> +	struct pwm_device *pwmd[MAX6639_NDEV]; /* max6639 has two pwm device */

s/device/devices/

>  };
>  
>  static struct max6639_data *max6639_update_device(struct device *dev)
> @@ -271,8 +282,11 @@ static ssize_t pwm_show(struct device *dev, struct device_attribute *dev_attr,
>  {
>  	struct sensor_device_attribute *attr = to_sensor_dev_attr(dev_attr);
>  	struct max6639_data *data = dev_get_drvdata(dev);
> +	struct pwm_state state;
> +
> +	pwm_get_state(data->pwmd[attr->index], &state);
>  
> -	return sprintf(buf, "%d\n", data->pwm[attr->index] * 255 / 120);
> +	return sprintf(buf, "%d\n", pwm_get_relative_duty_cycle(&state, 255));
>  }
>  
>  static ssize_t pwm_store(struct device *dev,
> @@ -281,6 +295,7 @@ static ssize_t pwm_store(struct device *dev,
>  {
>  	struct sensor_device_attribute *attr = to_sensor_dev_attr(dev_attr);
>  	struct max6639_data *data = dev_get_drvdata(dev);
> +	struct pwm_state state;
>  	unsigned long val;
>  	int res;
>  
> @@ -290,10 +305,10 @@ static ssize_t pwm_store(struct device *dev,
>  
>  	val = clamp_val(val, 0, 255);
>  
> -	mutex_lock(&data->update_lock);
> -	data->pwm[attr->index] = (u8)(val * 120 / 255);
> -	regmap_write(data->regmap, MAX6639_REG_TARGTDUTY(attr->index), data->pwm[attr->index]);
> -	mutex_unlock(&data->update_lock);
> +	pwm_get_state(data->pwmd[attr->index], &state);
> +	pwm_set_relative_duty_cycle(&state, val, 255);
> +	pwm_apply_state(data->pwmd[attr->index], &state);

I'm not a big fan of that pwm_get_state() + modify duty_cycle +
pwm_apply_state(). IMHO it's better to keep a struct pwm_state around in
the consumer and so have complete control in each step.

> +
>  	return count;
>  }
>  
> @@ -373,6 +388,158 @@ static struct attribute *max6639_attrs[] = {
>  };
>  ATTRIBUTE_GROUPS(max6639);
>  
> +static struct max6639_data *to_max6639_pwm(struct pwm_chip *chip)
> +{
> +	return container_of(chip, struct max6639_data, chip);
> +}
> +
> +static int max6639_pwm_get_state(struct pwm_chip *chip,
> +				 struct pwm_device *pwm,
> +				 struct pwm_state *state)
> +{
> +	struct max6639_data *data = to_max6639_pwm(chip);
> +	int value, i = pwm->hwpwm, x, err;
> +	unsigned int freq;
> +
> +	mutex_lock(&data->update_lock);
> +
> +	err = regmap_read(data->regmap, MAX6639_REG_FAN_CONFIG1(i), &value);
> +	if (err < 0)
> +		goto abort;

I don't know if the hwmon maintainers like that, but taking a mutex for
the whole function's runtime can also be expressed by:

	guard(mutex)(&data->update_lock);

then all the goto abort below can be replaced by return err.

> +
> +	if (value & MAX6639_FAN_CONFIG1_PWM) {
> +		state->enabled = true;
> +
> +		/* Determine frequency from respective registers */
> +		err = regmap_read(data->regmap, MAX6639_REG_FAN_CONFIG3(i), &value);
> +		if (err < 0)
> +			goto abort;
> +		x = value & MAX6639_FAN_CONFIG3_FREQ_MASK;
> +
> +		err = regmap_read(data->regmap, MAX6639_REG_GCONFIG, &value);
> +		if (err < 0)
> +			goto abort;
> +		if (value & MAX6639_GCONFIG_PWM_FREQ_HI)
> +			x |= 0x4;
> +		x &= 0x7;
> +		freq = freq_table[x];
> +
> +		state->period = DIV_ROUND_UP(NSEC_PER_SEC, freq);
> +
> +		err = regmap_read(data->regmap, MAX6639_REG_TARGTDUTY(i), &value);
> +		if (err < 0)
> +			goto abort;
> +		/* max6639 supports 120 slots only */
> +		state->duty_cycle = mul_u64_u32_div(state->period, value, 120);

MAX6639_REG_TARGTDUTY_SLOT instead of 120 here.

> +		err = regmap_read(data->regmap, MAX6639_REG_FAN_CONFIG2a(i), &value);
> +		if (err < 0)
> +			goto abort;
> +		value &= MAX6639_REG_FAN_CONFIG2a_PWM_POL;
> +		state->polarity = (value != 0);

Please don't hardcode PWM_POLARITY_* values here. Please use:

	state->polarity = (value != 0) ? PWM_POLARITY_INVERSED : PWM_POLARITY_NORMAL;

(or an if statement).

> +	} else {
> +		state->enabled = false;
> +	}
> +
> +abort:
> +	mutex_unlock(&data->update_lock);
> +	return value;
> +}
> +
> +static int max6639_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> +			     const struct pwm_state *state)
> +{
> +	struct max6639_data *data = to_max6639_pwm(chip);
> +	int value, i = pwm->hwpwm, x, err;
> +	unsigned int freq;
> +	struct pwm_state cstate;
> +
> +	cstate = pwm->state;
> +
> +	mutex_lock(&data->update_lock);
> +
> +	if (state->period != cstate.period) {
> +		/* Configure frequency */
> +		freq = DIV_ROUND_UP_ULL(NSEC_PER_SEC, state->period);
> +
> +		/* Chip supports limited number of frequency */
> +		for (x = 0; x < sizeof(freq_table); x++)
> +			if (freq <= freq_table[x])
> +				break;

If you store the periods instead of the frequencies in the global table
you can save several divisions and so simplify the code.

> +		err = regmap_read(data->regmap, MAX6639_REG_FAN_CONFIG3(i), &value);
> +		if (err < 0)
> +			goto abort;
> +
> +		value &= ~MAX6639_FAN_CONFIG3_FREQ_MASK;
> +		value |= (x & MAX6639_FAN_CONFIG3_FREQ_MASK);

You're using implicitly that there is no shift involved in
MAX6639_FAN_CONFIG3_FREQ_MASK. Better use FIELD_PREP().

Also MAX6639_FAN_CONFIG3_FREQ_MASK is 3, but x ranges in [0 ... 7].
That's a bug, isn't it?

> +		err = regmap_write(data->regmap, MAX6639_REG_FAN_CONFIG3(i), value);
> +		if (err < 0)
> +			goto abort;

How does the hardware behave here? Does it emit the new period already
with the old duty cycle and polarity setting? Please document that,
ideally in a paragraph captured "Limitations:" and a format matching
what several other pwm drivers do, to make that easily greppable.

> +		err = regmap_read(data->regmap, MAX6639_REG_GCONFIG, &value);
> +		if (err < 0)
> +			goto abort;
> +
> +		if (x >> 2)
> +			value &= ~MAX6639_GCONFIG_PWM_FREQ_HI;
> +		else
> +			value |= MAX6639_GCONFIG_PWM_FREQ_HI;
> +		err = regmap_write(data->regmap, MAX6639_REG_GCONFIG, value);
> +		if (err < 0)
> +			goto abort;
> +	}
> +
> +	/* Configure dutycycle */
> +	if (state->duty_cycle != cstate.duty_cycle ||
> +	    state->period != cstate.period) {
> +		value = DIV_ROUND_DOWN_ULL(state->duty_cycle * MAX6639_REG_TARGTDUTY_SLOT,
> +					   state->period);

The multiplication might overflow, better use mul_u64_u64_div_u64()
here. Also you're loosing precision here because the real period might
be lower than state->period. Consider:

	state->period = 9999999 [ns]
	state->duty_cycle = 123456 [ns]

With the possible frequencies you have to pick freq = 5000 [Hz] giving
you a period of 200000 ns. You're then configuring 123456 * 120 / 9999999
= 1 as duty_cycle count giving you 1666 ns as duty cycle. However you're
supposed to configure 74 giving 123333 ns.

> +		err = regmap_write(data->regmap, MAX6639_REG_TARGTDUTY(i), value);
> +		if (err < 0)
> +			goto abort;
> +	}
> +
> +	/* Configure polarity */
> +	if (state->polarity != cstate.polarity) {
> +		err = regmap_read(data->regmap, MAX6639_REG_FAN_CONFIG2a(i), &value);
> +		if (err < 0)
> +			goto abort;
> +		if (state->polarity == PWM_POLARITY_NORMAL)
> +			value |= MAX6639_REG_FAN_CONFIG2a_PWM_POL;
> +		else
> +			value &= ~MAX6639_REG_FAN_CONFIG2a_PWM_POL;
> +		err = regmap_write(data->regmap, MAX6639_REG_FAN_CONFIG2a(i), value);
> +		if (err < 0)
> +			goto abort;
> +	}
> +
> +	if (state->enabled != cstate.enabled) {
> +		err = regmap_read(data->regmap, MAX6639_REG_FAN_CONFIG1(i), &value);
> +		if (err < 0)
> +			goto abort;
> +		if (state->enabled)
> +			value |= MAX6639_FAN_CONFIG1_PWM;
> +		else
> +			value &= ~MAX6639_FAN_CONFIG1_PWM;
> +
> +		err = regmap_write(data->regmap, MAX6639_REG_FAN_CONFIG1(i), value);
> +		if (err < 0)
> +			goto abort;
> +	}
> +	value = 0;
> +
> +abort:
> +	mutex_unlock(&data->update_lock);
> +
> +	return err;
> +}
> +
> +static const struct pwm_ops max6639_pwm_ops = {
> +	.apply = max6639_pwm_apply,
> +	.get_state = max6639_pwm_get_state,
> +};
> +
>  /*
>   *  returns respective index in rpm_ranges table
>   *  1 by default on invalid range
> @@ -396,6 +563,7 @@ static int max6639_init_client(struct i2c_client *client,
>  		dev_get_platdata(&client->dev);
>  	int i;
>  	int rpm_range = 1; /* default: 4000 RPM */
> +	struct pwm_state state;

state could have a more local scope.

>  	int err;
>  
>  	/* Reset chip to default values, see below for GCONFIG setup */
> @@ -459,11 +627,15 @@ static int max6639_init_client(struct i2c_client *client,
>  		if (err)
>  			goto exit;
>  
> -		/* PWM 120/120 (i.e. 100%) */
> -		data->pwm[i] = 120;
> -		err = regmap_write(data->regmap, MAX6639_REG_TARGTDUTY(i), data->pwm[i]);
> -		if (err)
> -			goto exit;
> +		dev_dbg(&client->dev, "Using chip default PWM");
> +		data->pwmd[i] = pwm_request_from_chip(&data->chip, i, NULL);
> +		if (IS_ERR(data->pwmd[i]))
> +			return PTR_ERR(data->pwmd[i]);
> +		pwm_get_state(data->pwmd[i], &state);

What I said above about the pwm_get_state() + modify + pwm_apply_state()
approach applies still more to the initial configuration. Here you're
keeping some HW state set previously by an earlier incarnation of the
driver, or the boot loader or the reset default value, which might
involve state.enabled = false.

> +		state.period = DIV_ROUND_UP(NSEC_PER_SEC, 25000);
> +		state.polarity = PWM_POLARITY_NORMAL;
> +		pwm_set_relative_duty_cycle(&state, 0, 255);

This involves a division. Using

	state.duty_cycle = 0;

would be more efficient here.

> +		pwm_apply_state(data->pwmd[i], &state);
>  	}
>  	/* Start monitoring */
>  	err = regmap_write(data->regmap, MAX6639_REG_GCONFIG,
> @@ -540,6 +712,14 @@ static int max6639_probe(struct i2c_client *client)
>  				     PTR_ERR(data->regmap),
>  				     "regmap initialization failed\n");
>  
> +	/* Add PWM controller of max6639 */
> +	data->chip.dev = dev;
> +	data->chip.ops = &max6639_pwm_ops;
> +	data->chip.npwm = MAX6639_NDEV;
> +	err = devm_pwmchip_add(dev, &data->chip);
> +	if (err < 0)
> +		return dev_err_probe(dev, err, "failed to add PWM chip\n");
> +
>  	data->reg = devm_regulator_get_optional(dev, "fan");
>  	if (IS_ERR(data->reg)) {
>  		if (PTR_ERR(data->reg) != -ENODEV)

Do I understand right that the driver itself is expected to be the only
consumer of this PWM? I'm not sure that using the PWM stuff is a useful
improvement then. You're just gaining some debug possibilies for the
overhead. Probably it's easier to just inspect the device's registers
directly to debug and stick to the old abstraction without a struct
pwm_chip?!

Best regards
Uwe

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

Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)

Powered by blists - more mailing lists