On Tue, Apr 23, 2019 at 06:33:07AM -0700, Guenter Roeck wrote:
> Convert driver to use devm_hwmon_device_register_with_info to simplify
> the code and to reduce its size.
>
To keep everyone up to date, I have made the changes indicated below
to the patch. Thanks to 0day and Julia Lawall for reporting the problems.
Guenter
> Cc: Jean-Francois Dagenais <jeff.dagenais@xxxxxxxxx>
> Signed-off-by: Guenter Roeck <linux@xxxxxxxxxxxx>
> ---
> drivers/hwmon/max6650.c | 505 +++++++++++++++++++++++-------------------------
> 1 file changed, 244 insertions(+), 261 deletions(-)
>
> diff --git a/drivers/hwmon/max6650.c b/drivers/hwmon/max6650.c
> index c02694d70eee..301184172ab1 100644
> --- a/drivers/hwmon/max6650.c
> +++ b/drivers/hwmon/max6650.c
> @@ -114,7 +114,6 @@ module_param(clock, int, 0444);
>
> struct max6650_data {
> struct i2c_client *client;
> - const struct attribute_group *groups[3];
> struct thermal_cooling_device *cooling_dev;
> struct mutex update_lock;
> int nr_fans;
> @@ -230,26 +229,6 @@ static int max6650_set_operating_mode(struct max6650_data *data, u8 mode)
> return 0;
> }
>
> -static ssize_t fan_show(struct device *dev, struct device_attribute *devattr,
> - char *buf)
> -{
> - struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
> - struct max6650_data *data = max6650_update_device(dev);
> - int rpm;
> -
> - /*
> - * Calculation details:
> - *
> - * Each tachometer counts over an interval given by the "count"
> - * register (0.25, 0.5, 1 or 2 seconds). This module assumes
> - * that the fans produce two pulses per revolution (this seems
> - * to be the most common).
> - */
> -
> - rpm = ((data->tach[attr->index] * 120) / DIV_FROM_REG(data->count));
> - return sprintf(buf, "%d\n", rpm);
> -}
> -
> /*
> * Set the fan speed to the specified RPM (or read back the RPM setting).
> * This works in closed loop mode only. Use pwm1 for open loop speed setting.
> @@ -291,26 +270,6 @@ static ssize_t fan_show(struct device *dev, struct device_attribute *devattr,
> * controlled.
> */
>
> -static ssize_t fan1_target_show(struct device *dev,
> - struct device_attribute *devattr, char *buf)
> -{
> - struct max6650_data *data = max6650_update_device(dev);
> - int kscale, ktach, rpm;
> -
> - /*
> - * Use the datasheet equation:
> - *
> - * FanSpeed = KSCALE x fCLK / [256 x (KTACH + 1)]
> - *
> - * then multiply by 60 to give rpm.
> - */
> -
> - kscale = DIV_FROM_REG(data->config);
> - ktach = data->speed;
> - rpm = 60 * kscale * clock / (256 * (ktach + 1));
> - return sprintf(buf, "%d\n", rpm);
> -}
> -
> static int max6650_set_target(struct max6650_data *data, unsigned long rpm)
> {
> int kscale, ktach;
> @@ -339,183 +298,8 @@ static int max6650_set_target(struct max6650_data *data, unsigned long rpm)
> data->speed);
> }
>
> -static ssize_t fan1_target_store(struct device *dev,
> - struct device_attribute *devattr,
> - const char *buf, size_t count)
> -{
> - struct max6650_data *data = dev_get_drvdata(dev);
> - unsigned long rpm;
> - int err;
> -
> - err = kstrtoul(buf, 10, &rpm);
> - if (err)
> - return err;
> -
> - mutex_lock(&data->update_lock);
> -
> - err = max6650_set_target(data, rpm);
> -
> - mutex_unlock(&data->update_lock);
> -
> - if (err < 0)
> - return err;
> -
> - return count;
> -}
> -
> -/*
> - * Get/set the fan speed in open loop mode using pwm1 sysfs file.
> - * Speed is given as a relative value from 0 to 255, where 255 is maximum
> - * speed. Note that this is done by writing directly to the chip's DAC,
> - * it won't change the closed loop speed set by fan1_target.
> - * Also note that due to rounding errors it is possible that you don't read
> - * back exactly the value you have set.
> - */
> -
> -static ssize_t pwm1_show(struct device *dev, struct device_attribute *devattr,
> - char *buf)
> -{
> - struct max6650_data *data = max6650_update_device(dev);
> -
> - return sprintf(buf, "%d\n", dac_to_pwm(data->dac,
> - data->config & MAX6650_CFG_V12));
> -}
> -
> -static ssize_t pwm1_store(struct device *dev,
> - struct device_attribute *devattr, const char *buf,
> - size_t count)
> -{
> - struct max6650_data *data = dev_get_drvdata(dev);
> - struct i2c_client *client = data->client;
> - unsigned long pwm;
> - int err;
> - u8 dac;
> -
> - err = kstrtoul(buf, 10, &pwm);
> - if (err)
> - return err;
> -
> - pwm = clamp_val(pwm, 0, 255);
> -
> - mutex_lock(&data->update_lock);
> - dac = pwm_to_dac(pwm, data->config & MAX6650_CFG_V12);
> - err = i2c_smbus_write_byte_data(client, MAX6650_REG_DAC, dac);
> - if (!err)
> - data->dac = dac;
> - mutex_unlock(&data->update_lock);
> -
> - return err < 0 ? err : count;
> -}
> -
> -/*
> - * Get/Set controller mode:
> - * Possible values:
> - * 0 = Fan always on
> - * 1 = Open loop, Voltage is set according to speed, not regulated.
> - * 2 = Closed loop, RPM for all fans regulated by fan1 tachometer
> - * 3 = Fan off
> - */
> -static ssize_t pwm1_enable_show(struct device *dev,
> - struct device_attribute *devattr, char *buf)
> -{
> - struct max6650_data *data = max6650_update_device(dev);
> - int mode = (data->config & MAX6650_CFG_MODE_MASK) >> 4;
> - int sysfs_modes[4] = {0, 3, 2, 1};
> -
> - return sprintf(buf, "%d\n", sysfs_modes[mode]);
> -}
> -
> -static ssize_t pwm1_enable_store(struct device *dev,
> - struct device_attribute *devattr,
> - const char *buf, size_t count)
> -{
> - struct max6650_data *data = dev_get_drvdata(dev);
> - unsigned long mode;
> - int err;
> - const u8 max6650_modes[] = {
> - MAX6650_CFG_MODE_ON,
> - MAX6650_CFG_MODE_OPEN_LOOP,
> - MAX6650_CFG_MODE_CLOSED_LOOP,
> - MAX6650_CFG_MODE_OFF,
> - };
> -
> - err = kstrtoul(buf, 10, &mode);
> - if (err)
> - return err;
> -
> - if (mode >= ARRAY_SIZE(max6650_modes))
> - return -EINVAL;
> -
> - mutex_lock(&data->update_lock);
> -
> - max6650_set_operating_mode(data, max6650_modes[mode]);
> -
> - mutex_unlock(&data->update_lock);
> -
> - return count;
> -}
> -
> -/*
> - * Read/write functions for fan1_div sysfs file. The MAX6650 has no such
> - * divider. We handle this by converting between divider and counttime:
> - *
> - * (counttime == k) <==> (divider == 2^k), k = 0, 1, 2, or 3
> - *
> - * Lower values of k allow to connect a faster fan without the risk of
> - * counter overflow. The price is lower resolution. You can also set counttime
> - * using the module parameter. Note that the module parameter "prescaler" also
> - * influences the behaviour. Unfortunately, there's no sysfs attribute
> - * defined for that. See the data sheet for details.
> - */
> -
> -static ssize_t fan1_div_show(struct device *dev,
> - struct device_attribute *devattr, char *buf)
> -{
> - struct max6650_data *data = max6650_update_device(dev);
> -
> - return sprintf(buf, "%d\n", DIV_FROM_REG(data->count));
> -}
> -
> -static ssize_t fan1_div_store(struct device *dev,
> - struct device_attribute *devattr,
> - const char *buf, size_t count)
> -{
> - struct max6650_data *data = dev_get_drvdata(dev);
> - struct i2c_client *client = data->client;
> - unsigned long div;
> - int err;
> -
> - err = kstrtoul(buf, 10, &div);
> - if (err)
> - return err;
> -
> - mutex_lock(&data->update_lock);
> - switch (div) {
> - case 1:
> - data->count = 0;
> - break;
> - case 2:
> - data->count = 1;
> - break;
> - case 4:
> - data->count = 2;
> - break;
> - case 8:
> - data->count = 3;
> - break;
> - default:
> - mutex_unlock(&data->update_lock);
> - return -EINVAL;
> - }
> -
> - i2c_smbus_write_byte_data(client, MAX6650_REG_COUNT, data->count);
> - mutex_unlock(&data->update_lock);
> -
> - return count;
> -}
> -
> /*
> - * Get alarm stati:
> + * Get gpio alarm status:
> * Possible values:
> * 0 = no alarm
> * 1 = alarm
> @@ -538,17 +322,6 @@ static ssize_t alarm_show(struct device *dev,
> return sprintf(buf, "%d\n", alarm);
> }
>
> -static SENSOR_DEVICE_ATTR_RO(fan1_input, fan, 0);
> -static SENSOR_DEVICE_ATTR_RO(fan2_input, fan, 1);
> -static SENSOR_DEVICE_ATTR_RO(fan3_input, fan, 2);
> -static SENSOR_DEVICE_ATTR_RO(fan4_input, fan, 3);
> -static DEVICE_ATTR_RW(fan1_target);
> -static DEVICE_ATTR_RW(fan1_div);
> -static DEVICE_ATTR_RW(pwm1_enable);
> -static DEVICE_ATTR_RW(pwm1);
> -static SENSOR_DEVICE_ATTR_RO(fan1_max_alarm, alarm, MAX6650_ALRM_MAX);
> -static SENSOR_DEVICE_ATTR_RO(fan1_min_alarm, alarm, MAX6650_ALRM_MIN);
> -static SENSOR_DEVICE_ATTR_RO(fan1_fault, alarm, MAX6650_ALRM_TACH);
> static SENSOR_DEVICE_ATTR_RO(gpio1_alarm, alarm, MAX6650_ALRM_GPIO1);
> static SENSOR_DEVICE_ATTR_RO(gpio2_alarm, alarm, MAX6650_ALRM_GPIO2);
>
> @@ -564,11 +337,8 @@ static umode_t max6650_attrs_visible(struct kobject *kobj, struct attribute *a,
> */
>
> devattr = container_of(a, struct device_attribute, attr);
> - if (devattr == &sensor_dev_attr_fan1_max_alarm.dev_attr
> - || devattr == &sensor_dev_attr_fan1_min_alarm.dev_attr
> - || devattr == &sensor_dev_attr_fan1_fault.dev_attr
> - || devattr == &sensor_dev_attr_gpio1_alarm.dev_attr
> - || devattr == &sensor_dev_attr_gpio2_alarm.dev_attr) {
> + if (devattr == &sensor_dev_attr_gpio1_alarm.dev_attr ||
> + devattr == &sensor_dev_attr_gpio2_alarm.dev_attr) {
> if (!(data->alarm_en & to_sensor_dev_attr(devattr)->index))
> return 0;
> }
> @@ -577,14 +347,6 @@ static umode_t max6650_attrs_visible(struct kobject *kobj, struct attribute *a,
> }
>
> static struct attribute *max6650_attrs[] = {
> - &sensor_dev_attr_fan1_input.dev_attr.attr,
> - &dev_attr_fan1_target.attr,
> - &dev_attr_fan1_div.attr,
> - &dev_attr_pwm1_enable.attr,
> - &dev_attr_pwm1.attr,
> - &sensor_dev_attr_fan1_max_alarm.dev_attr.attr,
> - &sensor_dev_attr_fan1_min_alarm.dev_attr.attr,
> - &sensor_dev_attr_fan1_fault.dev_attr.attr,
> &sensor_dev_attr_gpio1_alarm.dev_attr.attr,
> &sensor_dev_attr_gpio2_alarm.dev_attr.attr,
> NULL
> @@ -595,21 +357,11 @@ static const struct attribute_group max6650_group = {
> .is_visible = max6650_attrs_visible,
> };
>
> -static struct attribute *max6651_attrs[] = {
> - &sensor_dev_attr_fan2_input.dev_attr.attr,
> - &sensor_dev_attr_fan3_input.dev_attr.attr,
> - &sensor_dev_attr_fan4_input.dev_attr.attr,
> +static const struct attribute_group *max6650_groups[] = {
> + &max6650_group,
> NULL
> };
>
> -static const struct attribute_group max6651_group = {
> - .attrs = max6651_attrs,
> -};
> -
> -/*
> - * Real code
> - */
> -
> static int max6650_init_client(struct max6650_data *data,
> struct i2c_client *client)
> {
> @@ -766,6 +518,241 @@ static void max6650_thermal_unregister(void *data)
> }
> #endif
>
> +static int max6650_read(struct device *dev, enum hwmon_sensor_types type,
> + u32 attr, int channel, long *val)
> +{
> + struct max6650_data *data = max6650_update_device(dev);
> + int mode;
> +
> + switch (type) {
> + case hwmon_pwm:
> + switch (attr) {
> + case hwmon_pwm_input:
> + *val = dac_to_pwm(data->dac,
> + data->config & MAX6650_CFG_V12);
> + break;
> + case hwmon_pwm_enable:
> + /*
> + * Possible values:
> + * 0 = Fan always on
> + * 1 = Open loop, Voltage is set according to speed,
> + * not regulated.
> + * 2 = Closed loop, RPM for all fans regulated by fan1
> + * tachometer
> + * 3 = Fan off
> + */
> + mode = (data->config & MAX6650_CFG_MODE_MASK) >> 4;
> + *val = (4 - mode) & 3; /* {0 1 2 3} -> {0 3 2 1} */
> + break;
> + default:
> + return -EOPNOTSUPP;
> + }
> + break;
> + case hwmon_fan:
> + switch (attr) {
> + case hwmon_fan_input:
> + /*
> + * Calculation details:
> + *
> + * Each tachometer counts over an interval given by the
> + * "count" register (0.25, 0.5, 1 or 2 seconds).
> + * The driver assumes that the fans produce two pulses
> + * per revolution (this seems to be the most common).
> + */
> + *val = DIV_ROUND_CLOSEST(data->tach[channel] * 120,
> + DIV_FROM_REG(data->count));
> + break;
> + case hwmon_fan_div:
> + *val = DIV_FROM_REG(data->count);
> + break;
> + case hwmon_fan_target:
> + /*
> + * Use the datasheet equation:
> + * FanSpeed = KSCALE x fCLK / [256 x (KTACH + 1)]
> + * then multiply by 60 to give rpm.
> + */
> + *val = 60 * DIV_FROM_REG(data->config) * clock /
> + (256 * (data->speed + 1));
> + break;
> + case hwmon_fan_min_alarm:
> + *val = !!(data->alarm & MAX6650_ALRM_MIN);
> + data->alarm &= ~MAX6650_ALRM_MIN;
> + data->valid = false;
> + break;
> + case hwmon_fan_max_alarm:
> + *val = !!(data->alarm & MAX6650_ALRM_MAX);
> + data->alarm &= ~MAX6650_ALRM_MAX;
> + data->valid = false;
> + break;
> + case hwmon_fan_fault:
> + *val = !!(data->alarm & MAX6650_ALRM_TACH);
> + data->alarm &= ~MAX6650_ALRM_TACH;
> + data->valid = false;
> + break;
> + default:
> + return -EOPNOTSUPP;
> + }
> + break;
> + default:
> + return -EOPNOTSUPP;
> + }
> + return 0;
> +}
> +
> +const u8 max6650_pwm_modes[] = {
static const max6650_pwm_modes
> + MAX6650_CFG_MODE_ON,
> + MAX6650_CFG_MODE_OPEN_LOOP,
> + MAX6650_CFG_MODE_CLOSED_LOOP,
> + MAX6650_CFG_MODE_OFF,
> +};
> +
> +static int max6650_write(struct device *dev, enum hwmon_sensor_types type,
> + u32 attr, int channel, long val)
> +{
> + struct max6650_data *data = dev_get_drvdata(dev);
> + int ret = 0;
> + u8 reg;
> +
> + mutex_lock(&data->update_lock);
> +
> + switch (type) {
> + case hwmon_pwm:
> + switch (attr) {
> + case hwmon_pwm_input:
> + reg = pwm_to_dac(clamp_val(val, 0, 255),
> + data->config & MAX6650_CFG_V12);
> + ret = i2c_smbus_write_byte_data(data->client,
> + MAX6650_REG_DAC, reg);
> + if (ret)
> + break;
> + data->dac = reg;
> + break;
> + case hwmon_pwm_enable:
> + if (val < 0 || val >= ARRAY_SIZE(max6650_pwm_modes))
> + return -EINVAL;
{
ret = -EINVAL;
break;
}
> + ret = max6650_set_operating_mode(data,
> + max6650_pwm_modes[val]);
> + break;
> + default:
> + ret = -EOPNOTSUPP;
> + break;
> + }
> + case hwmon_fan:
> + switch (attr) {
> + case hwmon_fan_div:
> + switch (val) {
> + case 1:
> + reg = 0;
> + break;
> + case 2:
> + reg = 1;
> + break;
> + case 4:
> + reg = 2;
> + break;
> + case 8:
> + reg = 3;
> + break;
> + default:
> + ret = -EINVAL;
> + goto error;
> + }
> + ret = i2c_smbus_write_byte_data(data->client,
> + MAX6650_REG_COUNT, reg);
> + if (ret)
> + break;
> + data->count = reg;
> + break;
> + case hwmon_fan_target:
> + if (val < 0) {
> + ret = -EINVAL;
> + break;
> + }
> + ret = max6650_set_target(data, val);
> + break;
> + default:
> + ret = -EOPNOTSUPP;
> + break;
> + }
> + break;
> + default:
> + ret = -EOPNOTSUPP;
> + break;
> + }
> +
> +error:
> + mutex_unlock(&data->update_lock);
> + return ret;
> +}
> +
> +static umode_t max6650_is_visible(const void *_data,
> + enum hwmon_sensor_types type, u32 attr,
> + int channel)
> +{
> + const struct max6650_data *data = _data;
> +
> + if (channel && (channel >= data->nr_fans || type != hwmon_fan))
> + return 0;
> +
> + switch (type) {
> + case hwmon_fan:
> + switch (attr) {
> + case hwmon_fan_input:
> + return 0444;
> + case hwmon_fan_target:
> + case hwmon_fan_div:
> + return 0644;
> + case hwmon_fan_min_alarm:
> + if (data->alarm_en & MAX6650_ALRM_MIN)
> + return 0444;
> + break;
> + case hwmon_fan_max_alarm:
> + if (data->alarm_en & MAX6650_ALRM_MAX)
> + return 0444;
> + break;
> + case hwmon_fan_fault:
> + if (data->alarm_en & MAX6650_ALRM_TACH)
> + return 0444;
> + break;
> + default:
> + break;
> + }
> + break;
> + case hwmon_pwm:
> + switch (attr) {
> + case hwmon_pwm_input:
> + case hwmon_pwm_enable:
> + return 0644;
> + default:
> + break;
> + }
> + break;
> + default:
> + break;
> + }
> + return 0;
> +}
> +
> +static const struct hwmon_channel_info *max6650_info[] = {
> + HWMON_CHANNEL_INFO(fan, HWMON_F_INPUT | HWMON_F_TARGET | HWMON_F_DIV |
> + HWMON_F_MIN_ALARM | HWMON_F_MAX_ALARM |
> + HWMON_F_FAULT,
> + HWMON_F_INPUT, HWMON_F_INPUT, HWMON_F_INPUT),
> + HWMON_CHANNEL_INFO(pwm, HWMON_PWM_INPUT | HWMON_PWM_ENABLE),
> + NULL
> +};
> +
> +static const struct hwmon_ops max6650_hwmon_ops = {
> + .read = max6650_read,
> + .write = max6650_write,
> + .is_visible = max6650_is_visible,
> +};
> +
> +static const struct hwmon_chip_info max6650_chip_info = {
> + .ops = &max6650_hwmon_ops,
> + .info = max6650_info,
> +};
> +
> static int max6650_probe(struct i2c_client *client,
> const struct i2c_device_id *id)
> {
> @@ -792,14 +779,10 @@ static int max6650_probe(struct i2c_client *client,
> if (err)
> return err;
>
> - data->groups[0] = &max6650_group;
> - /* 3 additional fan inputs for the MAX6651 */
> - if (data->nr_fans == 4)
> - data->groups[1] = &max6651_group;
> -
> - hwmon_dev = devm_hwmon_device_register_with_groups(dev,
> - client->name, data,
> - data->groups);
> + hwmon_dev = devm_hwmon_device_register_with_info(dev,
> + client->name, data,
> + &max6650_chip_info,
> + max6650_groups);
> err = PTR_ERR_OR_ZERO(hwmon_dev);
> if (err)
> return err;
