diff mbox series

[2/4] hwmon: (max6639) : Utilise pwm subsystem

Message ID 20240416171720.2875916-2-naresh.solanki@9elements.com (mailing list archive)
State Changes Requested
Headers show
Series [1/4] hwmon (max6639): Use regmap | expand

Commit Message

Naresh Solanki April 16, 2024, 5:17 p.m. UTC
Utilise pwm subsystem for fan pwm handling

Signed-off-by: Naresh Solanki <naresh.solanki@9elements.com>
---
 drivers/hwmon/Kconfig   |   1 +
 drivers/hwmon/max6639.c | 200 ++++++++++++++++++++++++++++++++++++++--
 2 files changed, 191 insertions(+), 10 deletions(-)

Comments

Guenter Roeck April 16, 2024, 9:22 p.m. UTC | #1
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@9elements.com>

That adds a lot of complexity to the driver. I am missing the benefits.
You are supposed to explain why you are making changes, not just that
you are making them.

Why are you making those changes ?

Guenter
Uwe Kleine-König April 17, 2024, 7:04 a.m. UTC | #2
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@9elements.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
Naresh Solanki April 22, 2024, 10:39 a.m. UTC | #3
Hi Guenter,

On Wed, 17 Apr 2024 at 02:52, Guenter Roeck <linux@roeck-us.net> wrote:
>
> 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@9elements.com>
>
> That adds a lot of complexity to the driver. I am missing the benefits.
> You are supposed to explain why you are making changes, not just that
> you are making them.
>
> Why are you making those changes ?
Sure.
This is to align with fan-common.yml wherein chip pwm is exposed.
I'll update commit message

Regards,
Naresh
>
> Guenter
Guenter Roeck April 22, 2024, 12:37 p.m. UTC | #4
On 4/22/24 03:39, Naresh Solanki wrote:
> Hi Guenter,
> 
> On Wed, 17 Apr 2024 at 02:52, Guenter Roeck <linux@roeck-us.net> wrote:
>>
>> 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@9elements.com>
>>
>> That adds a lot of complexity to the driver. I am missing the benefits.
>> You are supposed to explain why you are making changes, not just that
>> you are making them.
>>
>> Why are you making those changes ?
> Sure.
> This is to align with fan-common.yml wherein chip pwm is exposed.
> I'll update commit message
> 

Adding lots of complexity to a driver just to have it match a yaml file ?
I'll want to see a use case. Explain why you need the pwm exposed.
"because the yaml file demands it" is not a use case.

Guenter
Naresh Solanki May 6, 2024, 10:05 a.m. UTC | #5
+Rob Herring

Hi Guenter,


On Mon, 22 Apr 2024 at 18:07, Guenter Roeck <linux@roeck-us.net> wrote:
>
> On 4/22/24 03:39, Naresh Solanki wrote:
> > Hi Guenter,
> >
> > On Wed, 17 Apr 2024 at 02:52, Guenter Roeck <linux@roeck-us.net> wrote:
> >>
> >> 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@9elements.com>
> >>
> >> That adds a lot of complexity to the driver. I am missing the benefits.
> >> You are supposed to explain why you are making changes, not just that
> >> you are making them.
> >>
> >> Why are you making those changes ?
> > Sure.
> > This is to align with fan-common.yml wherein chip pwm is exposed.
> > I'll update commit message
> >
>
> Adding lots of complexity to a driver just to have it match a yaml file ?
> I'll want to see a use case. Explain why you need the pwm exposed.
> "because the yaml file demands it" is not a use case.
The idea behind this was that this approach provides flexibility with
hardware routing i.e., PWM0 might be connected to Fan1 & vise
versa instead of assuming 1:1 mapping.

Regards,
Naresh
>
> Guenter
>
Guenter Roeck May 6, 2024, 1:49 p.m. UTC | #6
On Mon, May 06, 2024 at 03:35:40PM +0530, Naresh Solanki wrote:
> +Rob Herring
> 
> Hi Guenter,
> 
> 
> On Mon, 22 Apr 2024 at 18:07, Guenter Roeck <linux@roeck-us.net> wrote:
> >
> > On 4/22/24 03:39, Naresh Solanki wrote:
> > > Hi Guenter,
> > >
> > > On Wed, 17 Apr 2024 at 02:52, Guenter Roeck <linux@roeck-us.net> wrote:
> > >>
> > >> 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@9elements.com>
> > >>
> > >> That adds a lot of complexity to the driver. I am missing the benefits.
> > >> You are supposed to explain why you are making changes, not just that
> > >> you are making them.
> > >>
> > >> Why are you making those changes ?
> > > Sure.
> > > This is to align with fan-common.yml wherein chip pwm is exposed.
> > > I'll update commit message
> > >
> >
> > Adding lots of complexity to a driver just to have it match a yaml file ?
> > I'll want to see a use case. Explain why you need the pwm exposed.
> > "because the yaml file demands it" is not a use case.
> The idea behind this was that this approach provides flexibility with
> hardware routing i.e., PWM0 might be connected to Fan1 & vise
> versa instead of assuming 1:1 mapping.
> 

The chip does not support such a configuration. Any hardware implementing
this would make automatic fan control using the chip's capabilities
impossible. Also, userspace fan control does not rely on the pwm subsystem.

This would make sense if someone was to use the chip as generic pwm
controller, which would be very unlikely. A "might be" is not sufficient
to warrant such an invasive and (in terms of code size) expensive change.

Thanks,
Guenter
diff mbox series

Patch

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;
+	struct pwm_device *pwmd[MAX6639_NDEV]; /* max6639 has two pwm device */
 };
 
 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);
+
 	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;
+
+	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);
+
+		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);
+	} 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;
+
+		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);
+		err = regmap_write(data->regmap, MAX6639_REG_FAN_CONFIG3(i), value);
+		if (err < 0)
+			goto abort;
+
+		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);
+		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;
 	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);
+		state.period = DIV_ROUND_UP(NSEC_PER_SEC, 25000);
+		state.polarity = PWM_POLARITY_NORMAL;
+		pwm_set_relative_duty_cycle(&state, 0, 255);
+		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)