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