* [PATCH 0/2] hwmon: (ina3221) Apply _info API @ 2018-10-05 23:59 Nicolin Chen 2018-10-05 23:59 ` [PATCH 1/2] hwmon: (core) Add hwmon_in_enable attribute Nicolin Chen 2018-10-05 23:59 ` [PATCH 2/2] hwmon: (ina3221) Use _info API to register hwmon device Nicolin Chen 0 siblings, 2 replies; 6+ messages in thread From: Nicolin Chen @ 2018-10-05 23:59 UTC (permalink / raw) To: jdelvare, linux; +Cc: afd, linux-hwmon, linux-kernel This series of patches applies _info API of hwmon core to the ina3221 hwmon driver. Nicolin Chen (2): hwmon: (core) Add hwmon_in_enable attribute hwmon: (ina3221) Use _info API to register hwmon device drivers/hwmon/hwmon.c | 1 + drivers/hwmon/ina3221.c | 528 +++++++++++++++++++--------------------- include/linux/hwmon.h | 2 + 3 files changed, 248 insertions(+), 283 deletions(-) -- 2.17.1 ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 1/2] hwmon: (core) Add hwmon_in_enable attribute 2018-10-05 23:59 [PATCH 0/2] hwmon: (ina3221) Apply _info API Nicolin Chen @ 2018-10-05 23:59 ` Nicolin Chen 2018-10-06 19:24 ` Guenter Roeck 2018-10-05 23:59 ` [PATCH 2/2] hwmon: (ina3221) Use _info API to register hwmon device Nicolin Chen 1 sibling, 1 reply; 6+ messages in thread From: Nicolin Chen @ 2018-10-05 23:59 UTC (permalink / raw) To: jdelvare, linux; +Cc: afd, linux-hwmon, linux-kernel According to hwmon ABI, in%d_enable is a sysfs interface that allows user space to enable and disable the input sensor. So this patch just simply adds the attribute to the list. Signed-off-by: Nicolin Chen <nicoleotsuka@gmail.com> --- drivers/hwmon/hwmon.c | 1 + include/linux/hwmon.h | 2 ++ 2 files changed, 3 insertions(+) diff --git a/drivers/hwmon/hwmon.c b/drivers/hwmon/hwmon.c index 33d51281272b..ac1cdf88840f 100644 --- a/drivers/hwmon/hwmon.c +++ b/drivers/hwmon/hwmon.c @@ -356,6 +356,7 @@ static const char * const hwmon_in_attr_templates[] = { [hwmon_in_max_alarm] = "in%d_max_alarm", [hwmon_in_lcrit_alarm] = "in%d_lcrit_alarm", [hwmon_in_crit_alarm] = "in%d_crit_alarm", + [hwmon_in_enable] = "in%d_enable", }; static const char * const hwmon_curr_attr_templates[] = { diff --git a/include/linux/hwmon.h b/include/linux/hwmon.h index 9493d4a388db..99e0c1b0b5fb 100644 --- a/include/linux/hwmon.h +++ b/include/linux/hwmon.h @@ -118,6 +118,7 @@ enum hwmon_in_attributes { hwmon_in_max_alarm, hwmon_in_lcrit_alarm, hwmon_in_crit_alarm, + hwmon_in_enable, }; #define HWMON_I_INPUT BIT(hwmon_in_input) @@ -135,6 +136,7 @@ enum hwmon_in_attributes { #define HWMON_I_MAX_ALARM BIT(hwmon_in_max_alarm) #define HWMON_I_LCRIT_ALARM BIT(hwmon_in_lcrit_alarm) #define HWMON_I_CRIT_ALARM BIT(hwmon_in_crit_alarm) +#define HWMON_I_ENABLE BIT(hwmon_in_enable) enum hwmon_curr_attributes { hwmon_curr_input, -- 2.17.1 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] hwmon: (core) Add hwmon_in_enable attribute 2018-10-05 23:59 ` [PATCH 1/2] hwmon: (core) Add hwmon_in_enable attribute Nicolin Chen @ 2018-10-06 19:24 ` Guenter Roeck 0 siblings, 0 replies; 6+ messages in thread From: Guenter Roeck @ 2018-10-06 19:24 UTC (permalink / raw) To: Nicolin Chen; +Cc: jdelvare, afd, linux-hwmon, linux-kernel On Fri, Oct 05, 2018 at 04:59:04PM -0700, Nicolin Chen wrote: > According to hwmon ABI, in%d_enable is a sysfs interface that > allows user space to enable and disable the input sensor. So > this patch just simply adds the attribute to the list. > > Signed-off-by: Nicolin Chen <nicoleotsuka@gmail.com> Applied to hwmon-next. Thanks, Guenter > --- > drivers/hwmon/hwmon.c | 1 + > include/linux/hwmon.h | 2 ++ > 2 files changed, 3 insertions(+) > > diff --git a/drivers/hwmon/hwmon.c b/drivers/hwmon/hwmon.c > index 33d51281272b..ac1cdf88840f 100644 > --- a/drivers/hwmon/hwmon.c > +++ b/drivers/hwmon/hwmon.c > @@ -356,6 +356,7 @@ static const char * const hwmon_in_attr_templates[] = { > [hwmon_in_max_alarm] = "in%d_max_alarm", > [hwmon_in_lcrit_alarm] = "in%d_lcrit_alarm", > [hwmon_in_crit_alarm] = "in%d_crit_alarm", > + [hwmon_in_enable] = "in%d_enable", > }; > > static const char * const hwmon_curr_attr_templates[] = { > diff --git a/include/linux/hwmon.h b/include/linux/hwmon.h > index 9493d4a388db..99e0c1b0b5fb 100644 > --- a/include/linux/hwmon.h > +++ b/include/linux/hwmon.h > @@ -118,6 +118,7 @@ enum hwmon_in_attributes { > hwmon_in_max_alarm, > hwmon_in_lcrit_alarm, > hwmon_in_crit_alarm, > + hwmon_in_enable, > }; > > #define HWMON_I_INPUT BIT(hwmon_in_input) > @@ -135,6 +136,7 @@ enum hwmon_in_attributes { > #define HWMON_I_MAX_ALARM BIT(hwmon_in_max_alarm) > #define HWMON_I_LCRIT_ALARM BIT(hwmon_in_lcrit_alarm) > #define HWMON_I_CRIT_ALARM BIT(hwmon_in_crit_alarm) > +#define HWMON_I_ENABLE BIT(hwmon_in_enable) > > enum hwmon_curr_attributes { > hwmon_curr_input, ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 2/2] hwmon: (ina3221) Use _info API to register hwmon device 2018-10-05 23:59 [PATCH 0/2] hwmon: (ina3221) Apply _info API Nicolin Chen 2018-10-05 23:59 ` [PATCH 1/2] hwmon: (core) Add hwmon_in_enable attribute Nicolin Chen @ 2018-10-05 23:59 ` Nicolin Chen 2018-10-06 20:05 ` Guenter Roeck 1 sibling, 1 reply; 6+ messages in thread From: Nicolin Chen @ 2018-10-05 23:59 UTC (permalink / raw) To: jdelvare, linux; +Cc: afd, linux-hwmon, linux-kernel The hwmon core has a newer API which abstracts most of common things in the core so as to simplify the hwmon device drivers. This patch implements this _info API to ina3221 hwmon driver. Signed-off-by: Nicolin Chen <nicoleotsuka@gmail.com> --- drivers/hwmon/ina3221.c | 528 +++++++++++++++++++--------------------- 1 file changed, 245 insertions(+), 283 deletions(-) diff --git a/drivers/hwmon/ina3221.c b/drivers/hwmon/ina3221.c index c3a497aed345..08cc0b4bc0ba 100644 --- a/drivers/hwmon/ina3221.c +++ b/drivers/hwmon/ina3221.c @@ -77,21 +77,6 @@ enum ina3221_channels { INA3221_NUM_CHANNELS }; -static const unsigned int register_channel[] = { - [INA3221_BUS1] = INA3221_CHANNEL1, - [INA3221_BUS2] = INA3221_CHANNEL2, - [INA3221_BUS3] = INA3221_CHANNEL3, - [INA3221_SHUNT1] = INA3221_CHANNEL1, - [INA3221_SHUNT2] = INA3221_CHANNEL2, - [INA3221_SHUNT3] = INA3221_CHANNEL3, - [INA3221_CRIT1] = INA3221_CHANNEL1, - [INA3221_CRIT2] = INA3221_CHANNEL2, - [INA3221_CRIT3] = INA3221_CHANNEL3, - [INA3221_WARN1] = INA3221_CHANNEL1, - [INA3221_WARN2] = INA3221_CHANNEL2, - [INA3221_WARN3] = INA3221_CHANNEL3, -}; - /** * struct ina3221_input - channel input source specific information * @label: label of channel input source @@ -123,58 +108,6 @@ static inline bool ina3221_is_enabled(struct ina3221_data *ina, int channel) return ina->reg_config & INA3221_CONFIG_CHx_EN(channel); } -static ssize_t ina3221_show_label(struct device *dev, - struct device_attribute *attr, char *buf) -{ - struct sensor_device_attribute *sd_attr = to_sensor_dev_attr(attr); - struct ina3221_data *ina = dev_get_drvdata(dev); - unsigned int channel = sd_attr->index; - struct ina3221_input *input = &ina->inputs[channel]; - - return snprintf(buf, PAGE_SIZE, "%s\n", input->label); -} - -static ssize_t ina3221_show_enable(struct device *dev, - struct device_attribute *attr, char *buf) -{ - struct sensor_device_attribute *sd_attr = to_sensor_dev_attr(attr); - struct ina3221_data *ina = dev_get_drvdata(dev); - unsigned int channel = sd_attr->index; - - return snprintf(buf, PAGE_SIZE, "%d\n", - ina3221_is_enabled(ina, channel)); -} - -static ssize_t ina3221_set_enable(struct device *dev, - struct device_attribute *attr, - const char *buf, size_t count) -{ - struct sensor_device_attribute *sd_attr = to_sensor_dev_attr(attr); - struct ina3221_data *ina = dev_get_drvdata(dev); - unsigned int channel = sd_attr->index; - u16 config, mask = INA3221_CONFIG_CHx_EN(channel); - bool enable; - int ret; - - ret = kstrtobool(buf, &enable); - if (ret) - return ret; - - config = enable ? mask : 0; - - /* Enable or disable the channel */ - ret = regmap_update_bits(ina->regmap, INA3221_CONFIG, mask, config); - if (ret) - return ret; - - /* Cache the latest config register value */ - ret = regmap_read(ina->regmap, INA3221_CONFIG, &ina->reg_config); - if (ret) - return ret; - - return count; -} - static int ina3221_read_value(struct ina3221_data *ina, unsigned int reg, int *val) { @@ -190,94 +123,107 @@ static int ina3221_read_value(struct ina3221_data *ina, unsigned int reg, return 0; } -static ssize_t ina3221_show_bus_voltage(struct device *dev, - struct device_attribute *attr, - char *buf) +static const u8 ina3221_in_reg[] = { + INA3221_BUS1, + INA3221_BUS2, + INA3221_BUS3, + INA3221_SHUNT1, + INA3221_SHUNT2, + INA3221_SHUNT3, +}; + +static int ina3221_read_in(struct device *dev, u32 attr, int channel, long *val) { - struct sensor_device_attribute *sd_attr = to_sensor_dev_attr(attr); + const bool is_shunt = channel > INA3221_CHANNEL3; struct ina3221_data *ina = dev_get_drvdata(dev); - unsigned int reg = sd_attr->index; - unsigned int channel = register_channel[reg]; - int val, voltage_mv, ret; + u8 reg = ina3221_in_reg[channel]; + int regval, ret; - /* No data for read-only attribute if channel is disabled */ - if (!attr->store && !ina3221_is_enabled(ina, channel)) - return -ENODATA; + /* Translate shunt channel ID to sensor channel ID: 4->1, 5->2, 6->3 */ + channel %= INA3221_NUM_CHANNELS; - ret = ina3221_read_value(ina, reg, &val); - if (ret) - return ret; + switch (attr) { + case hwmon_in_input: + if (!ina3221_is_enabled(ina, channel)) + return -ENODATA; - voltage_mv = val * 8; + ret = ina3221_read_value(ina, reg, ®val); + if (ret) + return ret; - return snprintf(buf, PAGE_SIZE, "%d\n", voltage_mv); + /* + * Scale of shunt voltage (uV): LSB is 40uV + * Scale of bus voltage (mV): LSB is 8mV + */ + *val = regval * (is_shunt ? 40 : 8); + return 0; + case hwmon_in_enable: + *val = ina3221_is_enabled(ina, channel); + return 0; + default: + return -EOPNOTSUPP; + } } -static ssize_t ina3221_show_shunt_voltage(struct device *dev, - struct device_attribute *attr, - char *buf) +static const u8 ina3221_curr_reg[][INA3221_NUM_CHANNELS] = { + [hwmon_curr_input] = { INA3221_SHUNT1, INA3221_SHUNT2, INA3221_SHUNT3 }, + [hwmon_curr_max] = { INA3221_WARN1, INA3221_WARN2, INA3221_WARN3 }, + [hwmon_curr_crit] = { INA3221_CRIT1, INA3221_CRIT2, INA3221_CRIT3 }, + [hwmon_curr_max_alarm] = { F_WF1, F_WF2, F_WF3 }, + [hwmon_curr_crit_alarm] = { F_CF1, F_CF2, F_CF3 }, +}; + +static int ina3221_read_curr(struct device *dev, u32 attr, + int channel, long *val) { - struct sensor_device_attribute *sd_attr = to_sensor_dev_attr(attr); struct ina3221_data *ina = dev_get_drvdata(dev); - unsigned int reg = sd_attr->index; - unsigned int channel = register_channel[reg]; - int val, voltage_uv, ret; - - /* No data for read-only attribute if channel is disabled */ - if (!attr->store && !ina3221_is_enabled(ina, channel)) - return -ENODATA; - - ret = ina3221_read_value(ina, reg, &val); - if (ret) - return ret; - voltage_uv = val * 40; - - return snprintf(buf, PAGE_SIZE, "%d\n", voltage_uv); -} - -static ssize_t ina3221_show_current(struct device *dev, - struct device_attribute *attr, char *buf) -{ - struct sensor_device_attribute *sd_attr = to_sensor_dev_attr(attr); - struct ina3221_data *ina = dev_get_drvdata(dev); - unsigned int reg = sd_attr->index; - unsigned int channel = register_channel[reg]; struct ina3221_input *input = &ina->inputs[channel]; int resistance_uo = input->shunt_resistor; - int val, current_ma, voltage_nv, ret; + u8 reg = ina3221_curr_reg[attr][channel]; + int regval, voltage_nv, ret; - /* No data for read-only attribute if channel is disabled */ - if (!attr->store && !ina3221_is_enabled(ina, channel)) - return -ENODATA; + switch (attr) { + case hwmon_curr_input: + if (!ina3221_is_enabled(ina, channel)) + return -ENODATA; + /* fallthrough */ + case hwmon_curr_crit: + case hwmon_curr_max: + ret = ina3221_read_value(ina, reg, ®val); + if (ret) + return ret; - ret = ina3221_read_value(ina, reg, &val); - if (ret) - return ret; - voltage_nv = val * 40000; - - current_ma = DIV_ROUND_CLOSEST(voltage_nv, resistance_uo); - - return snprintf(buf, PAGE_SIZE, "%d\n", current_ma); + /* Scale of shunt voltage: LSB is 40uV (40000nV) */ + voltage_nv = regval * 40000; + /* Return current in mA */ + *val = DIV_ROUND_CLOSEST(voltage_nv, resistance_uo); + return 0; + case hwmon_curr_crit_alarm: + case hwmon_curr_max_alarm: + ret = regmap_field_read(ina->fields[reg], ®val); + if (ret) + return ret; + *val = regval; + return 0; + default: + return -EOPNOTSUPP; + } } -static ssize_t ina3221_set_current(struct device *dev, - struct device_attribute *attr, - const char *buf, size_t count) +static int ina3221_write_curr(struct device *dev, u32 attr, + int channel, long val) { - struct sensor_device_attribute *sd_attr = to_sensor_dev_attr(attr); struct ina3221_data *ina = dev_get_drvdata(dev); - unsigned int reg = sd_attr->index; - unsigned int channel = register_channel[reg]; struct ina3221_input *input = &ina->inputs[channel]; int resistance_uo = input->shunt_resistor; - int val, current_ma, voltage_uv, ret; + u8 reg = ina3221_curr_reg[attr][channel]; + int regval, current_ma, voltage_uv; - ret = kstrtoint(buf, 0, ¤t_ma); - if (ret) - return ret; + if (attr != hwmon_curr_crit && attr != hwmon_curr_max) + return -EOPNOTSUPP; /* clamp current */ - current_ma = clamp_val(current_ma, + current_ma = clamp_val(val, INT_MIN / resistance_uo, INT_MAX / resistance_uo); @@ -287,15 +233,175 @@ static ssize_t ina3221_set_current(struct device *dev, voltage_uv = clamp_val(voltage_uv, -163800, 163800); /* 1 / 40uV(scale) << 3(register shift) = 5 */ - val = DIV_ROUND_CLOSEST(voltage_uv, 5) & 0xfff8; + regval = DIV_ROUND_CLOSEST(voltage_uv, 5) & 0xfff8; - ret = regmap_write(ina->regmap, reg, val); + return regmap_write(ina->regmap, reg, regval); +} + +static int ina3221_write_enable(struct device *dev, int channel, bool enable) +{ + struct ina3221_data *ina = dev_get_drvdata(dev); + u16 config, mask = INA3221_CONFIG_CHx_EN(channel); + int ret; + + config = enable ? mask : 0; + + /* Enable or disable the channel */ + ret = regmap_update_bits(ina->regmap, INA3221_CONFIG, mask, config); if (ret) return ret; - return count; + /* Cache the latest config register value */ + ret = regmap_read(ina->regmap, INA3221_CONFIG, &ina->reg_config); + if (ret) + return ret; + + return 0; } +static int ina3221_read(struct device *dev, enum hwmon_sensor_types type, + u32 attr, int channel, long *val) +{ + switch (type) { + case hwmon_in: + return ina3221_read_in(dev, attr, channel - 1, val); + case hwmon_curr: + return ina3221_read_curr(dev, attr, channel, val); + default: + return -EOPNOTSUPP; + } +} + +static int ina3221_write(struct device *dev, enum hwmon_sensor_types type, + u32 attr, int channel, long val) +{ + switch (type) { + case hwmon_in: + if (attr != hwmon_in_enable) + return -EOPNOTSUPP; + return ina3221_write_enable(dev, channel - 1, val); + case hwmon_curr: + return ina3221_write_curr(dev, attr, channel, val); + default: + return -EOPNOTSUPP; + } +} + +static int ina3221_read_string(struct device *dev, enum hwmon_sensor_types type, + u32 attr, int channel, const char **str) +{ + struct ina3221_data *ina = dev_get_drvdata(dev); + int index = channel - 1; + + if (type != hwmon_in || attr != hwmon_in_label) + return -EOPNOTSUPP; + + /* Only 1-3 channels have hwmon_in_label */ + if (channel < 1 || channel > 3) + return -EOPNOTSUPP; + + *str = ina->inputs[index].label; + + return 0; +} + +static umode_t ina3221_is_visible(const void *drvdata, + enum hwmon_sensor_types type, + u32 attr, int channel) +{ + const struct ina3221_data *ina = drvdata; + const struct ina3221_input *input = NULL; + + switch (type) { + case hwmon_in: + /* Ignore in0_ */ + if (channel == 0) + return 0; + + switch (attr) { + case hwmon_in_label: + if (channel - 1 <= INA3221_CHANNEL3) + input = &ina->inputs[channel - 1]; + /* Hide label node if label is not provided */ + return (input && input->label) ? 0444 : 0; + case hwmon_in_input: + return 0444; + case hwmon_in_enable: + return 0644; + default: + return 0; + } + case hwmon_curr: + switch (attr) { + case hwmon_curr_input: + case hwmon_curr_crit_alarm: + case hwmon_curr_max_alarm: + return 0444; + case hwmon_curr_crit: + case hwmon_curr_max: + return 0644; + default: + return 0; + } + default: + return 0; + } +} + +static const u32 ina3221_in_config[] = { + /* 0: dummy, skipped in is_visible */ + HWMON_I_INPUT, + /* 1-3: input voltage Channels */ + HWMON_I_INPUT | HWMON_I_ENABLE | HWMON_I_LABEL, + HWMON_I_INPUT | HWMON_I_ENABLE | HWMON_I_LABEL, + HWMON_I_INPUT | HWMON_I_ENABLE | HWMON_I_LABEL, + /* 4-6: shunt voltage Channels */ + HWMON_I_INPUT, + HWMON_I_INPUT, + HWMON_I_INPUT, + 0 +}; + +static const struct hwmon_channel_info ina3221_in = { + .type = hwmon_in, + .config = ina3221_in_config, +}; + +#define INA3221_HWMON_CURR_CONFIG (HWMON_C_INPUT | \ + HWMON_C_CRIT | HWMON_C_CRIT_ALARM | \ + HWMON_C_MAX | HWMON_C_MAX_ALARM) + +static const u32 ina3221_curr_config[] = { + INA3221_HWMON_CURR_CONFIG, + INA3221_HWMON_CURR_CONFIG, + INA3221_HWMON_CURR_CONFIG, + 0 +}; + +static const struct hwmon_channel_info ina3221_curr = { + .type = hwmon_curr, + .config = ina3221_curr_config, +}; + +static const struct hwmon_channel_info *ina3221_info[] = { + &ina3221_in, + &ina3221_curr, + NULL +}; + +static const struct hwmon_ops ina3221_hwmon_ops = { + .is_visible = ina3221_is_visible, + .read_string = ina3221_read_string, + .read = ina3221_read, + .write = ina3221_write, +}; + +static const struct hwmon_chip_info ina3221_chip_info = { + .ops = &ina3221_hwmon_ops, + .info = ina3221_info, +}; + +/* Extra attribute groups */ static ssize_t ina3221_show_shunt(struct device *dev, struct device_attribute *attr, char *buf) { @@ -329,54 +435,6 @@ static ssize_t ina3221_set_shunt(struct device *dev, return count; } -static ssize_t ina3221_show_alert(struct device *dev, - struct device_attribute *attr, char *buf) -{ - struct sensor_device_attribute *sd_attr = to_sensor_dev_attr(attr); - struct ina3221_data *ina = dev_get_drvdata(dev); - unsigned int field = sd_attr->index; - unsigned int regval; - int ret; - - ret = regmap_field_read(ina->fields[field], ®val); - if (ret) - return ret; - - return snprintf(buf, PAGE_SIZE, "%d\n", regval); -} - -/* input channel label */ -static SENSOR_DEVICE_ATTR(in1_label, 0444, - ina3221_show_label, NULL, INA3221_CHANNEL1); -static SENSOR_DEVICE_ATTR(in2_label, 0444, - ina3221_show_label, NULL, INA3221_CHANNEL2); -static SENSOR_DEVICE_ATTR(in3_label, 0444, - ina3221_show_label, NULL, INA3221_CHANNEL3); - -/* voltage channel enable */ -static SENSOR_DEVICE_ATTR(in1_enable, 0644, - ina3221_show_enable, ina3221_set_enable, INA3221_CHANNEL1); -static SENSOR_DEVICE_ATTR(in2_enable, 0644, - ina3221_show_enable, ina3221_set_enable, INA3221_CHANNEL2); -static SENSOR_DEVICE_ATTR(in3_enable, 0644, - ina3221_show_enable, ina3221_set_enable, INA3221_CHANNEL3); - -/* bus voltage */ -static SENSOR_DEVICE_ATTR(in1_input, S_IRUGO, - ina3221_show_bus_voltage, NULL, INA3221_BUS1); -static SENSOR_DEVICE_ATTR(in2_input, S_IRUGO, - ina3221_show_bus_voltage, NULL, INA3221_BUS2); -static SENSOR_DEVICE_ATTR(in3_input, S_IRUGO, - ina3221_show_bus_voltage, NULL, INA3221_BUS3); - -/* calculated current */ -static SENSOR_DEVICE_ATTR(curr1_input, S_IRUGO, - ina3221_show_current, NULL, INA3221_SHUNT1); -static SENSOR_DEVICE_ATTR(curr2_input, S_IRUGO, - ina3221_show_current, NULL, INA3221_SHUNT2); -static SENSOR_DEVICE_ATTR(curr3_input, S_IRUGO, - ina3221_show_current, NULL, INA3221_SHUNT3); - /* shunt resistance */ static SENSOR_DEVICE_ATTR(shunt1_resistor, S_IRUGO | S_IWUSR, ina3221_show_shunt, ina3221_set_shunt, INA3221_CHANNEL1); @@ -385,109 +443,13 @@ static SENSOR_DEVICE_ATTR(shunt2_resistor, S_IRUGO | S_IWUSR, static SENSOR_DEVICE_ATTR(shunt3_resistor, S_IRUGO | S_IWUSR, ina3221_show_shunt, ina3221_set_shunt, INA3221_CHANNEL3); -/* critical current */ -static SENSOR_DEVICE_ATTR(curr1_crit, S_IRUGO | S_IWUSR, - ina3221_show_current, ina3221_set_current, INA3221_CRIT1); -static SENSOR_DEVICE_ATTR(curr2_crit, S_IRUGO | S_IWUSR, - ina3221_show_current, ina3221_set_current, INA3221_CRIT2); -static SENSOR_DEVICE_ATTR(curr3_crit, S_IRUGO | S_IWUSR, - ina3221_show_current, ina3221_set_current, INA3221_CRIT3); - -/* critical current alert */ -static SENSOR_DEVICE_ATTR(curr1_crit_alarm, S_IRUGO, - ina3221_show_alert, NULL, F_CF1); -static SENSOR_DEVICE_ATTR(curr2_crit_alarm, S_IRUGO, - ina3221_show_alert, NULL, F_CF2); -static SENSOR_DEVICE_ATTR(curr3_crit_alarm, S_IRUGO, - ina3221_show_alert, NULL, F_CF3); - -/* warning current */ -static SENSOR_DEVICE_ATTR(curr1_max, S_IRUGO | S_IWUSR, - ina3221_show_current, ina3221_set_current, INA3221_WARN1); -static SENSOR_DEVICE_ATTR(curr2_max, S_IRUGO | S_IWUSR, - ina3221_show_current, ina3221_set_current, INA3221_WARN2); -static SENSOR_DEVICE_ATTR(curr3_max, S_IRUGO | S_IWUSR, - ina3221_show_current, ina3221_set_current, INA3221_WARN3); - -/* warning current alert */ -static SENSOR_DEVICE_ATTR(curr1_max_alarm, S_IRUGO, - ina3221_show_alert, NULL, F_WF1); -static SENSOR_DEVICE_ATTR(curr2_max_alarm, S_IRUGO, - ina3221_show_alert, NULL, F_WF2); -static SENSOR_DEVICE_ATTR(curr3_max_alarm, S_IRUGO, - ina3221_show_alert, NULL, F_WF3); - -/* shunt voltage */ -static SENSOR_DEVICE_ATTR(in4_input, S_IRUGO, - ina3221_show_shunt_voltage, NULL, INA3221_SHUNT1); -static SENSOR_DEVICE_ATTR(in5_input, S_IRUGO, - ina3221_show_shunt_voltage, NULL, INA3221_SHUNT2); -static SENSOR_DEVICE_ATTR(in6_input, S_IRUGO, - ina3221_show_shunt_voltage, NULL, INA3221_SHUNT3); - static struct attribute *ina3221_attrs[] = { - /* channel 1 -- make sure label at first */ - &sensor_dev_attr_in1_label.dev_attr.attr, - &sensor_dev_attr_in1_enable.dev_attr.attr, - &sensor_dev_attr_in1_input.dev_attr.attr, - &sensor_dev_attr_curr1_input.dev_attr.attr, &sensor_dev_attr_shunt1_resistor.dev_attr.attr, - &sensor_dev_attr_curr1_crit.dev_attr.attr, - &sensor_dev_attr_curr1_crit_alarm.dev_attr.attr, - &sensor_dev_attr_curr1_max.dev_attr.attr, - &sensor_dev_attr_curr1_max_alarm.dev_attr.attr, - &sensor_dev_attr_in4_input.dev_attr.attr, - - /* channel 2 -- make sure label at first */ - &sensor_dev_attr_in2_label.dev_attr.attr, - &sensor_dev_attr_in2_enable.dev_attr.attr, - &sensor_dev_attr_in2_input.dev_attr.attr, - &sensor_dev_attr_curr2_input.dev_attr.attr, &sensor_dev_attr_shunt2_resistor.dev_attr.attr, - &sensor_dev_attr_curr2_crit.dev_attr.attr, - &sensor_dev_attr_curr2_crit_alarm.dev_attr.attr, - &sensor_dev_attr_curr2_max.dev_attr.attr, - &sensor_dev_attr_curr2_max_alarm.dev_attr.attr, - &sensor_dev_attr_in5_input.dev_attr.attr, - - /* channel 3 -- make sure label at first */ - &sensor_dev_attr_in3_label.dev_attr.attr, - &sensor_dev_attr_in3_enable.dev_attr.attr, - &sensor_dev_attr_in3_input.dev_attr.attr, - &sensor_dev_attr_curr3_input.dev_attr.attr, &sensor_dev_attr_shunt3_resistor.dev_attr.attr, - &sensor_dev_attr_curr3_crit.dev_attr.attr, - &sensor_dev_attr_curr3_crit_alarm.dev_attr.attr, - &sensor_dev_attr_curr3_max.dev_attr.attr, - &sensor_dev_attr_curr3_max_alarm.dev_attr.attr, - &sensor_dev_attr_in6_input.dev_attr.attr, - NULL, }; - -static umode_t ina3221_attr_is_visible(struct kobject *kobj, - struct attribute *attr, int n) -{ - const int max_attrs = ARRAY_SIZE(ina3221_attrs) - 1; - const int num_attrs = max_attrs / INA3221_NUM_CHANNELS; - struct device *dev = kobj_to_dev(kobj); - struct ina3221_data *ina = dev_get_drvdata(dev); - enum ina3221_channels channel = n / num_attrs; - struct ina3221_input *input = &ina->inputs[channel]; - int index = n % num_attrs; - - /* Hide label node if label is not provided */ - if (index == 0 && !input->label) - return 0; - - return attr->mode; -} - -static const struct attribute_group ina3221_group = { - .is_visible = ina3221_attr_is_visible, - .attrs = ina3221_attrs, -}; -__ATTRIBUTE_GROUPS(ina3221); +ATTRIBUTE_GROUPS(ina3221); static const struct regmap_range ina3221_yes_ranges[] = { regmap_reg_range(INA3221_CONFIG, INA3221_BUS3), @@ -620,9 +582,9 @@ static int ina3221_probe(struct i2c_client *client, dev_set_drvdata(dev, ina); - hwmon_dev = devm_hwmon_device_register_with_groups(dev, - client->name, - ina, ina3221_groups); + hwmon_dev = devm_hwmon_device_register_with_info(dev, client->name, ina, + &ina3221_chip_info, + ina3221_groups); if (IS_ERR(hwmon_dev)) { dev_err(dev, "Unable to register hwmon device\n"); return PTR_ERR(hwmon_dev); -- 2.17.1 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] hwmon: (ina3221) Use _info API to register hwmon device 2018-10-05 23:59 ` [PATCH 2/2] hwmon: (ina3221) Use _info API to register hwmon device Nicolin Chen @ 2018-10-06 20:05 ` Guenter Roeck 2018-10-07 7:07 ` Nicolin Chen 0 siblings, 1 reply; 6+ messages in thread From: Guenter Roeck @ 2018-10-06 20:05 UTC (permalink / raw) To: Nicolin Chen; +Cc: jdelvare, afd, linux-hwmon, linux-kernel Hi Nicolin, On Fri, Oct 05, 2018 at 04:59:05PM -0700, Nicolin Chen wrote: > The hwmon core has a newer API which abstracts most of common > things in the core so as to simplify the hwmon device drivers. > > This patch implements this _info API to ina3221 hwmon driver. > $ size drivers/hwmon/ina*.o text data bss dec hex filename 9251 3552 64 12867 3243 drivers/hwmon/ina3221.o 12959 9256 64 22279 5707 drivers/hwmon/ina3221-orig.o It looks like this version is about 45% smaller. Surprising, isn't it ? Something else I just noticed: When reading the shunt resistor value from devicetree, the value range is not checked. A shunt resistor value of 0 should result in a divide by zero error. Also, the value read from dt is an u32, but assigned to an int. A shunt resistor value of 0x80000000 or higher should yield quite interesting results. Not that this makes sense, but we should not accept bad values. Can you send a follow-up patch to fix that ? Other comments inline. Thanks, Guenter > Signed-off-by: Nicolin Chen <nicoleotsuka@gmail.com> > --- > drivers/hwmon/ina3221.c | 528 +++++++++++++++++++--------------------- > 1 file changed, 245 insertions(+), 283 deletions(-) > > diff --git a/drivers/hwmon/ina3221.c b/drivers/hwmon/ina3221.c > index c3a497aed345..08cc0b4bc0ba 100644 > --- a/drivers/hwmon/ina3221.c > +++ b/drivers/hwmon/ina3221.c > @@ -77,21 +77,6 @@ enum ina3221_channels { > INA3221_NUM_CHANNELS > }; > > -static const unsigned int register_channel[] = { > - [INA3221_BUS1] = INA3221_CHANNEL1, > - [INA3221_BUS2] = INA3221_CHANNEL2, > - [INA3221_BUS3] = INA3221_CHANNEL3, > - [INA3221_SHUNT1] = INA3221_CHANNEL1, > - [INA3221_SHUNT2] = INA3221_CHANNEL2, > - [INA3221_SHUNT3] = INA3221_CHANNEL3, > - [INA3221_CRIT1] = INA3221_CHANNEL1, > - [INA3221_CRIT2] = INA3221_CHANNEL2, > - [INA3221_CRIT3] = INA3221_CHANNEL3, > - [INA3221_WARN1] = INA3221_CHANNEL1, > - [INA3221_WARN2] = INA3221_CHANNEL2, > - [INA3221_WARN3] = INA3221_CHANNEL3, > -}; > - > /** > * struct ina3221_input - channel input source specific information > * @label: label of channel input source > @@ -123,58 +108,6 @@ static inline bool ina3221_is_enabled(struct ina3221_data *ina, int channel) > return ina->reg_config & INA3221_CONFIG_CHx_EN(channel); > } > > -static ssize_t ina3221_show_label(struct device *dev, > - struct device_attribute *attr, char *buf) > -{ > - struct sensor_device_attribute *sd_attr = to_sensor_dev_attr(attr); > - struct ina3221_data *ina = dev_get_drvdata(dev); > - unsigned int channel = sd_attr->index; > - struct ina3221_input *input = &ina->inputs[channel]; > - > - return snprintf(buf, PAGE_SIZE, "%s\n", input->label); > -} > - > -static ssize_t ina3221_show_enable(struct device *dev, > - struct device_attribute *attr, char *buf) > -{ > - struct sensor_device_attribute *sd_attr = to_sensor_dev_attr(attr); > - struct ina3221_data *ina = dev_get_drvdata(dev); > - unsigned int channel = sd_attr->index; > - > - return snprintf(buf, PAGE_SIZE, "%d\n", > - ina3221_is_enabled(ina, channel)); > -} > - > -static ssize_t ina3221_set_enable(struct device *dev, > - struct device_attribute *attr, > - const char *buf, size_t count) > -{ > - struct sensor_device_attribute *sd_attr = to_sensor_dev_attr(attr); > - struct ina3221_data *ina = dev_get_drvdata(dev); > - unsigned int channel = sd_attr->index; > - u16 config, mask = INA3221_CONFIG_CHx_EN(channel); > - bool enable; > - int ret; > - > - ret = kstrtobool(buf, &enable); > - if (ret) > - return ret; > - > - config = enable ? mask : 0; > - > - /* Enable or disable the channel */ > - ret = regmap_update_bits(ina->regmap, INA3221_CONFIG, mask, config); > - if (ret) > - return ret; > - > - /* Cache the latest config register value */ > - ret = regmap_read(ina->regmap, INA3221_CONFIG, &ina->reg_config); > - if (ret) > - return ret; > - > - return count; > -} > - > static int ina3221_read_value(struct ina3221_data *ina, unsigned int reg, > int *val) > { > @@ -190,94 +123,107 @@ static int ina3221_read_value(struct ina3221_data *ina, unsigned int reg, > return 0; > } > > -static ssize_t ina3221_show_bus_voltage(struct device *dev, > - struct device_attribute *attr, > - char *buf) > +static const u8 ina3221_in_reg[] = { > + INA3221_BUS1, > + INA3221_BUS2, > + INA3221_BUS3, > + INA3221_SHUNT1, > + INA3221_SHUNT2, > + INA3221_SHUNT3, > +}; > + > +static int ina3221_read_in(struct device *dev, u32 attr, int channel, long *val) > { > - struct sensor_device_attribute *sd_attr = to_sensor_dev_attr(attr); > + const bool is_shunt = channel > INA3221_CHANNEL3; > struct ina3221_data *ina = dev_get_drvdata(dev); > - unsigned int reg = sd_attr->index; > - unsigned int channel = register_channel[reg]; > - int val, voltage_mv, ret; > + u8 reg = ina3221_in_reg[channel]; > + int regval, ret; > > - /* No data for read-only attribute if channel is disabled */ > - if (!attr->store && !ina3221_is_enabled(ina, channel)) > - return -ENODATA; > + /* Translate shunt channel ID to sensor channel ID: 4->1, 5->2, 6->3 */ Misleading. The channel number as used here is 0-aligned, so it is 3 -> 0 etc. I know the chip talks about channels 1..3, but that is not how "channel" is used here. It took me a bit to find out that the calling code subtracts 1 from the channel number and that the code is correct. We don't want others to suffer the same fate. > + channel %= INA3221_NUM_CHANNELS; 3 % 3 = 0, not 1. > > - ret = ina3221_read_value(ina, reg, &val); > - if (ret) > - return ret; > + switch (attr) { > + case hwmon_in_input: > + if (!ina3221_is_enabled(ina, channel)) > + return -ENODATA; > > - voltage_mv = val * 8; > + ret = ina3221_read_value(ina, reg, ®val); > + if (ret) > + return ret; > > - return snprintf(buf, PAGE_SIZE, "%d\n", voltage_mv); > + /* > + * Scale of shunt voltage (uV): LSB is 40uV > + * Scale of bus voltage (mV): LSB is 8mV > + */ > + *val = regval * (is_shunt ? 40 : 8); > + return 0; > + case hwmon_in_enable: > + *val = ina3221_is_enabled(ina, channel); > + return 0; > + default: > + return -EOPNOTSUPP; > + } > } > > -static ssize_t ina3221_show_shunt_voltage(struct device *dev, > - struct device_attribute *attr, > - char *buf) > +static const u8 ina3221_curr_reg[][INA3221_NUM_CHANNELS] = { > + [hwmon_curr_input] = { INA3221_SHUNT1, INA3221_SHUNT2, INA3221_SHUNT3 }, > + [hwmon_curr_max] = { INA3221_WARN1, INA3221_WARN2, INA3221_WARN3 }, > + [hwmon_curr_crit] = { INA3221_CRIT1, INA3221_CRIT2, INA3221_CRIT3 }, > + [hwmon_curr_max_alarm] = { F_WF1, F_WF2, F_WF3 }, > + [hwmon_curr_crit_alarm] = { F_CF1, F_CF2, F_CF3 }, > +}; > + > +static int ina3221_read_curr(struct device *dev, u32 attr, > + int channel, long *val) > { > - struct sensor_device_attribute *sd_attr = to_sensor_dev_attr(attr); > struct ina3221_data *ina = dev_get_drvdata(dev); > - unsigned int reg = sd_attr->index; > - unsigned int channel = register_channel[reg]; > - int val, voltage_uv, ret; > - > - /* No data for read-only attribute if channel is disabled */ > - if (!attr->store && !ina3221_is_enabled(ina, channel)) > - return -ENODATA; > - > - ret = ina3221_read_value(ina, reg, &val); > - if (ret) > - return ret; > - voltage_uv = val * 40; > - > - return snprintf(buf, PAGE_SIZE, "%d\n", voltage_uv); > -} > - > -static ssize_t ina3221_show_current(struct device *dev, > - struct device_attribute *attr, char *buf) > -{ > - struct sensor_device_attribute *sd_attr = to_sensor_dev_attr(attr); > - struct ina3221_data *ina = dev_get_drvdata(dev); > - unsigned int reg = sd_attr->index; > - unsigned int channel = register_channel[reg]; > struct ina3221_input *input = &ina->inputs[channel]; > int resistance_uo = input->shunt_resistor; > - int val, current_ma, voltage_nv, ret; > + u8 reg = ina3221_curr_reg[attr][channel]; > + int regval, voltage_nv, ret; > > - /* No data for read-only attribute if channel is disabled */ > - if (!attr->store && !ina3221_is_enabled(ina, channel)) > - return -ENODATA; > + switch (attr) { > + case hwmon_curr_input: > + if (!ina3221_is_enabled(ina, channel)) > + return -ENODATA; > + /* fallthrough */ I think the official terminology is "fall through". But then it looks like "fallthrough" is also widely used, so maybe we are ok. > + case hwmon_curr_crit: > + case hwmon_curr_max: > + ret = ina3221_read_value(ina, reg, ®val); > + if (ret) > + return ret; > > - ret = ina3221_read_value(ina, reg, &val); > - if (ret) > - return ret; > - voltage_nv = val * 40000; > - > - current_ma = DIV_ROUND_CLOSEST(voltage_nv, resistance_uo); > - > - return snprintf(buf, PAGE_SIZE, "%d\n", current_ma); > + /* Scale of shunt voltage: LSB is 40uV (40000nV) */ > + voltage_nv = regval * 40000; > + /* Return current in mA */ > + *val = DIV_ROUND_CLOSEST(voltage_nv, resistance_uo); > + return 0; > + case hwmon_curr_crit_alarm: > + case hwmon_curr_max_alarm: > + ret = regmap_field_read(ina->fields[reg], ®val); > + if (ret) > + return ret; > + *val = regval; > + return 0; > + default: > + return -EOPNOTSUPP; > + } > } > > -static ssize_t ina3221_set_current(struct device *dev, > - struct device_attribute *attr, > - const char *buf, size_t count) > +static int ina3221_write_curr(struct device *dev, u32 attr, > + int channel, long val) > { > - struct sensor_device_attribute *sd_attr = to_sensor_dev_attr(attr); > struct ina3221_data *ina = dev_get_drvdata(dev); > - unsigned int reg = sd_attr->index; > - unsigned int channel = register_channel[reg]; > struct ina3221_input *input = &ina->inputs[channel]; > int resistance_uo = input->shunt_resistor; > - int val, current_ma, voltage_uv, ret; > + u8 reg = ina3221_curr_reg[attr][channel]; > + int regval, current_ma, voltage_uv; > > - ret = kstrtoint(buf, 0, ¤t_ma); > - if (ret) > - return ret; > + if (attr != hwmon_curr_crit && attr != hwmon_curr_max) > + return -EOPNOTSUPP; I don't think this is needed. > > /* clamp current */ > - current_ma = clamp_val(current_ma, > + current_ma = clamp_val(val, > INT_MIN / resistance_uo, > INT_MAX / resistance_uo); > > @@ -287,15 +233,175 @@ static ssize_t ina3221_set_current(struct device *dev, > voltage_uv = clamp_val(voltage_uv, -163800, 163800); > > /* 1 / 40uV(scale) << 3(register shift) = 5 */ > - val = DIV_ROUND_CLOSEST(voltage_uv, 5) & 0xfff8; > + regval = DIV_ROUND_CLOSEST(voltage_uv, 5) & 0xfff8; > > - ret = regmap_write(ina->regmap, reg, val); > + return regmap_write(ina->regmap, reg, regval); > +} > + > +static int ina3221_write_enable(struct device *dev, int channel, bool enable) > +{ > + struct ina3221_data *ina = dev_get_drvdata(dev); > + u16 config, mask = INA3221_CONFIG_CHx_EN(channel); > + int ret; > + > + config = enable ? mask : 0; > + > + /* Enable or disable the channel */ > + ret = regmap_update_bits(ina->regmap, INA3221_CONFIG, mask, config); > if (ret) > return ret; > > - return count; > + /* Cache the latest config register value */ > + ret = regmap_read(ina->regmap, INA3221_CONFIG, &ina->reg_config); > + if (ret) > + return ret; > + > + return 0; > } > > +static int ina3221_read(struct device *dev, enum hwmon_sensor_types type, > + u32 attr, int channel, long *val) > +{ > + switch (type) { > + case hwmon_in: > + return ina3221_read_in(dev, attr, channel - 1, val); > + case hwmon_curr: > + return ina3221_read_curr(dev, attr, channel, val); > + default: > + return -EOPNOTSUPP; > + } > +} > + > +static int ina3221_write(struct device *dev, enum hwmon_sensor_types type, > + u32 attr, int channel, long val) > +{ > + switch (type) { > + case hwmon_in: > + if (attr != hwmon_in_enable) > + return -EOPNOTSUPP; I think you can drop this. > + return ina3221_write_enable(dev, channel - 1, val); > + case hwmon_curr: > + return ina3221_write_curr(dev, attr, channel, val); > + default: > + return -EOPNOTSUPP; I tend to have drivers return -EOPNOTSUPP for default: cases since otherwise static checkers may complain about missing default cases, but otherwise we should refrain from unnecessary checks. > + } > +} > + > +static int ina3221_read_string(struct device *dev, enum hwmon_sensor_types type, > + u32 attr, int channel, const char **str) > +{ > + struct ina3221_data *ina = dev_get_drvdata(dev); > + int index = channel - 1; > + > + if (type != hwmon_in || attr != hwmon_in_label) > + return -EOPNOTSUPP; > + > + /* Only 1-3 channels have hwmon_in_label */ > + if (channel < 1 || channel > 3) > + return -EOPNOTSUPP; > + The above checks should be unnecessary. > + *str = ina->inputs[index].label; > + > + return 0; > +} > + > +static umode_t ina3221_is_visible(const void *drvdata, > + enum hwmon_sensor_types type, > + u32 attr, int channel) > +{ > + const struct ina3221_data *ina = drvdata; > + const struct ina3221_input *input = NULL; > + > + switch (type) { > + case hwmon_in: > + /* Ignore in0_ */ > + if (channel == 0) > + return 0; > + > + switch (attr) { > + case hwmon_in_label: > + if (channel - 1 <= INA3221_CHANNEL3) > + input = &ina->inputs[channel - 1]; > + /* Hide label node if label is not provided */ > + return (input && input->label) ? 0444 : 0; > + case hwmon_in_input: > + return 0444; > + case hwmon_in_enable: > + return 0644; > + default: > + return 0; > + } > + case hwmon_curr: > + switch (attr) { > + case hwmon_curr_input: > + case hwmon_curr_crit_alarm: > + case hwmon_curr_max_alarm: > + return 0444; > + case hwmon_curr_crit: > + case hwmon_curr_max: > + return 0644; > + default: > + return 0; > + } > + default: > + return 0; > + } > +} > + > +static const u32 ina3221_in_config[] = { > + /* 0: dummy, skipped in is_visible */ > + HWMON_I_INPUT, > + /* 1-3: input voltage Channels */ > + HWMON_I_INPUT | HWMON_I_ENABLE | HWMON_I_LABEL, > + HWMON_I_INPUT | HWMON_I_ENABLE | HWMON_I_LABEL, > + HWMON_I_INPUT | HWMON_I_ENABLE | HWMON_I_LABEL, > + /* 4-6: shunt voltage Channels */ > + HWMON_I_INPUT, > + HWMON_I_INPUT, > + HWMON_I_INPUT, > + 0 > +}; > + > +static const struct hwmon_channel_info ina3221_in = { > + .type = hwmon_in, > + .config = ina3221_in_config, > +}; > + > +#define INA3221_HWMON_CURR_CONFIG (HWMON_C_INPUT | \ > + HWMON_C_CRIT | HWMON_C_CRIT_ALARM | \ > + HWMON_C_MAX | HWMON_C_MAX_ALARM) > + > +static const u32 ina3221_curr_config[] = { > + INA3221_HWMON_CURR_CONFIG, > + INA3221_HWMON_CURR_CONFIG, > + INA3221_HWMON_CURR_CONFIG, > + 0 > +}; > + > +static const struct hwmon_channel_info ina3221_curr = { > + .type = hwmon_curr, > + .config = ina3221_curr_config, > +}; > + > +static const struct hwmon_channel_info *ina3221_info[] = { > + &ina3221_in, > + &ina3221_curr, > + NULL > +}; > + > +static const struct hwmon_ops ina3221_hwmon_ops = { > + .is_visible = ina3221_is_visible, > + .read_string = ina3221_read_string, > + .read = ina3221_read, > + .write = ina3221_write, > +}; > + > +static const struct hwmon_chip_info ina3221_chip_info = { > + .ops = &ina3221_hwmon_ops, > + .info = ina3221_info, > +}; > + > +/* Extra attribute groups */ > static ssize_t ina3221_show_shunt(struct device *dev, > struct device_attribute *attr, char *buf) > { > @@ -329,54 +435,6 @@ static ssize_t ina3221_set_shunt(struct device *dev, > return count; > } > > -static ssize_t ina3221_show_alert(struct device *dev, > - struct device_attribute *attr, char *buf) > -{ > - struct sensor_device_attribute *sd_attr = to_sensor_dev_attr(attr); > - struct ina3221_data *ina = dev_get_drvdata(dev); > - unsigned int field = sd_attr->index; > - unsigned int regval; > - int ret; > - > - ret = regmap_field_read(ina->fields[field], ®val); > - if (ret) > - return ret; > - > - return snprintf(buf, PAGE_SIZE, "%d\n", regval); > -} > - > -/* input channel label */ > -static SENSOR_DEVICE_ATTR(in1_label, 0444, > - ina3221_show_label, NULL, INA3221_CHANNEL1); > -static SENSOR_DEVICE_ATTR(in2_label, 0444, > - ina3221_show_label, NULL, INA3221_CHANNEL2); > -static SENSOR_DEVICE_ATTR(in3_label, 0444, > - ina3221_show_label, NULL, INA3221_CHANNEL3); > - > -/* voltage channel enable */ > -static SENSOR_DEVICE_ATTR(in1_enable, 0644, > - ina3221_show_enable, ina3221_set_enable, INA3221_CHANNEL1); > -static SENSOR_DEVICE_ATTR(in2_enable, 0644, > - ina3221_show_enable, ina3221_set_enable, INA3221_CHANNEL2); > -static SENSOR_DEVICE_ATTR(in3_enable, 0644, > - ina3221_show_enable, ina3221_set_enable, INA3221_CHANNEL3); > - > -/* bus voltage */ > -static SENSOR_DEVICE_ATTR(in1_input, S_IRUGO, > - ina3221_show_bus_voltage, NULL, INA3221_BUS1); > -static SENSOR_DEVICE_ATTR(in2_input, S_IRUGO, > - ina3221_show_bus_voltage, NULL, INA3221_BUS2); > -static SENSOR_DEVICE_ATTR(in3_input, S_IRUGO, > - ina3221_show_bus_voltage, NULL, INA3221_BUS3); > - > -/* calculated current */ > -static SENSOR_DEVICE_ATTR(curr1_input, S_IRUGO, > - ina3221_show_current, NULL, INA3221_SHUNT1); > -static SENSOR_DEVICE_ATTR(curr2_input, S_IRUGO, > - ina3221_show_current, NULL, INA3221_SHUNT2); > -static SENSOR_DEVICE_ATTR(curr3_input, S_IRUGO, > - ina3221_show_current, NULL, INA3221_SHUNT3); > - > /* shunt resistance */ > static SENSOR_DEVICE_ATTR(shunt1_resistor, S_IRUGO | S_IWUSR, > ina3221_show_shunt, ina3221_set_shunt, INA3221_CHANNEL1); > @@ -385,109 +443,13 @@ static SENSOR_DEVICE_ATTR(shunt2_resistor, S_IRUGO | S_IWUSR, > static SENSOR_DEVICE_ATTR(shunt3_resistor, S_IRUGO | S_IWUSR, > ina3221_show_shunt, ina3221_set_shunt, INA3221_CHANNEL3); > > -/* critical current */ > -static SENSOR_DEVICE_ATTR(curr1_crit, S_IRUGO | S_IWUSR, > - ina3221_show_current, ina3221_set_current, INA3221_CRIT1); > -static SENSOR_DEVICE_ATTR(curr2_crit, S_IRUGO | S_IWUSR, > - ina3221_show_current, ina3221_set_current, INA3221_CRIT2); > -static SENSOR_DEVICE_ATTR(curr3_crit, S_IRUGO | S_IWUSR, > - ina3221_show_current, ina3221_set_current, INA3221_CRIT3); > - > -/* critical current alert */ > -static SENSOR_DEVICE_ATTR(curr1_crit_alarm, S_IRUGO, > - ina3221_show_alert, NULL, F_CF1); > -static SENSOR_DEVICE_ATTR(curr2_crit_alarm, S_IRUGO, > - ina3221_show_alert, NULL, F_CF2); > -static SENSOR_DEVICE_ATTR(curr3_crit_alarm, S_IRUGO, > - ina3221_show_alert, NULL, F_CF3); > - > -/* warning current */ > -static SENSOR_DEVICE_ATTR(curr1_max, S_IRUGO | S_IWUSR, > - ina3221_show_current, ina3221_set_current, INA3221_WARN1); > -static SENSOR_DEVICE_ATTR(curr2_max, S_IRUGO | S_IWUSR, > - ina3221_show_current, ina3221_set_current, INA3221_WARN2); > -static SENSOR_DEVICE_ATTR(curr3_max, S_IRUGO | S_IWUSR, > - ina3221_show_current, ina3221_set_current, INA3221_WARN3); > - > -/* warning current alert */ > -static SENSOR_DEVICE_ATTR(curr1_max_alarm, S_IRUGO, > - ina3221_show_alert, NULL, F_WF1); > -static SENSOR_DEVICE_ATTR(curr2_max_alarm, S_IRUGO, > - ina3221_show_alert, NULL, F_WF2); > -static SENSOR_DEVICE_ATTR(curr3_max_alarm, S_IRUGO, > - ina3221_show_alert, NULL, F_WF3); > - > -/* shunt voltage */ > -static SENSOR_DEVICE_ATTR(in4_input, S_IRUGO, > - ina3221_show_shunt_voltage, NULL, INA3221_SHUNT1); > -static SENSOR_DEVICE_ATTR(in5_input, S_IRUGO, > - ina3221_show_shunt_voltage, NULL, INA3221_SHUNT2); > -static SENSOR_DEVICE_ATTR(in6_input, S_IRUGO, > - ina3221_show_shunt_voltage, NULL, INA3221_SHUNT3); > - > static struct attribute *ina3221_attrs[] = { > - /* channel 1 -- make sure label at first */ > - &sensor_dev_attr_in1_label.dev_attr.attr, > - &sensor_dev_attr_in1_enable.dev_attr.attr, > - &sensor_dev_attr_in1_input.dev_attr.attr, > - &sensor_dev_attr_curr1_input.dev_attr.attr, > &sensor_dev_attr_shunt1_resistor.dev_attr.attr, > - &sensor_dev_attr_curr1_crit.dev_attr.attr, > - &sensor_dev_attr_curr1_crit_alarm.dev_attr.attr, > - &sensor_dev_attr_curr1_max.dev_attr.attr, > - &sensor_dev_attr_curr1_max_alarm.dev_attr.attr, > - &sensor_dev_attr_in4_input.dev_attr.attr, > - > - /* channel 2 -- make sure label at first */ > - &sensor_dev_attr_in2_label.dev_attr.attr, > - &sensor_dev_attr_in2_enable.dev_attr.attr, > - &sensor_dev_attr_in2_input.dev_attr.attr, > - &sensor_dev_attr_curr2_input.dev_attr.attr, > &sensor_dev_attr_shunt2_resistor.dev_attr.attr, > - &sensor_dev_attr_curr2_crit.dev_attr.attr, > - &sensor_dev_attr_curr2_crit_alarm.dev_attr.attr, > - &sensor_dev_attr_curr2_max.dev_attr.attr, > - &sensor_dev_attr_curr2_max_alarm.dev_attr.attr, > - &sensor_dev_attr_in5_input.dev_attr.attr, > - > - /* channel 3 -- make sure label at first */ > - &sensor_dev_attr_in3_label.dev_attr.attr, > - &sensor_dev_attr_in3_enable.dev_attr.attr, > - &sensor_dev_attr_in3_input.dev_attr.attr, > - &sensor_dev_attr_curr3_input.dev_attr.attr, > &sensor_dev_attr_shunt3_resistor.dev_attr.attr, > - &sensor_dev_attr_curr3_crit.dev_attr.attr, > - &sensor_dev_attr_curr3_crit_alarm.dev_attr.attr, > - &sensor_dev_attr_curr3_max.dev_attr.attr, > - &sensor_dev_attr_curr3_max_alarm.dev_attr.attr, > - &sensor_dev_attr_in6_input.dev_attr.attr, > - > NULL, > }; > - > -static umode_t ina3221_attr_is_visible(struct kobject *kobj, > - struct attribute *attr, int n) > -{ > - const int max_attrs = ARRAY_SIZE(ina3221_attrs) - 1; > - const int num_attrs = max_attrs / INA3221_NUM_CHANNELS; > - struct device *dev = kobj_to_dev(kobj); > - struct ina3221_data *ina = dev_get_drvdata(dev); > - enum ina3221_channels channel = n / num_attrs; > - struct ina3221_input *input = &ina->inputs[channel]; > - int index = n % num_attrs; > - > - /* Hide label node if label is not provided */ > - if (index == 0 && !input->label) > - return 0; > - > - return attr->mode; > -} > - > -static const struct attribute_group ina3221_group = { > - .is_visible = ina3221_attr_is_visible, > - .attrs = ina3221_attrs, > -}; > -__ATTRIBUTE_GROUPS(ina3221); > +ATTRIBUTE_GROUPS(ina3221); > > static const struct regmap_range ina3221_yes_ranges[] = { > regmap_reg_range(INA3221_CONFIG, INA3221_BUS3), > @@ -620,9 +582,9 @@ static int ina3221_probe(struct i2c_client *client, > > dev_set_drvdata(dev, ina); > > - hwmon_dev = devm_hwmon_device_register_with_groups(dev, > - client->name, > - ina, ina3221_groups); > + hwmon_dev = devm_hwmon_device_register_with_info(dev, client->name, ina, > + &ina3221_chip_info, > + ina3221_groups); > if (IS_ERR(hwmon_dev)) { > dev_err(dev, "Unable to register hwmon device\n"); > return PTR_ERR(hwmon_dev); ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] hwmon: (ina3221) Use _info API to register hwmon device 2018-10-06 20:05 ` Guenter Roeck @ 2018-10-07 7:07 ` Nicolin Chen 0 siblings, 0 replies; 6+ messages in thread From: Nicolin Chen @ 2018-10-07 7:07 UTC (permalink / raw) To: Guenter Roeck; +Cc: jdelvare, afd, linux-hwmon, linux-kernel On Sat, Oct 06, 2018 at 01:05:27PM -0700, Guenter Roeck wrote: > Hi Nicolin, > > On Fri, Oct 05, 2018 at 04:59:05PM -0700, Nicolin Chen wrote: > > The hwmon core has a newer API which abstracts most of common > > things in the core so as to simplify the hwmon device drivers. > > > > This patch implements this _info API to ina3221 hwmon driver. > > > > $ size drivers/hwmon/ina*.o > text data bss dec hex filename > 9251 3552 64 12867 3243 drivers/hwmon/ina3221.o > 12959 9256 64 22279 5707 drivers/hwmon/ina3221-orig.o > > It looks like this version is about 45% smaller. Surprising, isn't it ? I was thinking about adding a memory saving report like this in the commit log until I noticed that the actual saving of binary size is 7~8KB on my side and there seems to be no difference at the size showed in the lsmod list (I built the driver with =m). Yet, I agree the saving is proportionally huge. > Something else I just noticed: When reading the shunt resistor > value from devicetree, the value range is not checked. A shunt > resistor value of 0 should result in a divide by zero error. > Also, the value read from dt is an u32, but assigned to an int. > A shunt resistor value of 0x80000000 or higher should yield quite > interesting results. Not that this makes sense, but we should not > accept bad values. Can you send a follow-up patch to fix that ? Will send a separate fix in the next version. > Other comments inline. Will fix them too. Thank you Nicolin -------- > Thanks, > Guenter > > > Signed-off-by: Nicolin Chen <nicoleotsuka@gmail.com> > > --- > > drivers/hwmon/ina3221.c | 528 +++++++++++++++++++--------------------- > > 1 file changed, 245 insertions(+), 283 deletions(-) > > > > diff --git a/drivers/hwmon/ina3221.c b/drivers/hwmon/ina3221.c > > index c3a497aed345..08cc0b4bc0ba 100644 > > --- a/drivers/hwmon/ina3221.c > > +++ b/drivers/hwmon/ina3221.c > > @@ -77,21 +77,6 @@ enum ina3221_channels { > > INA3221_NUM_CHANNELS > > }; > > > > -static const unsigned int register_channel[] = { > > - [INA3221_BUS1] = INA3221_CHANNEL1, > > - [INA3221_BUS2] = INA3221_CHANNEL2, > > - [INA3221_BUS3] = INA3221_CHANNEL3, > > - [INA3221_SHUNT1] = INA3221_CHANNEL1, > > - [INA3221_SHUNT2] = INA3221_CHANNEL2, > > - [INA3221_SHUNT3] = INA3221_CHANNEL3, > > - [INA3221_CRIT1] = INA3221_CHANNEL1, > > - [INA3221_CRIT2] = INA3221_CHANNEL2, > > - [INA3221_CRIT3] = INA3221_CHANNEL3, > > - [INA3221_WARN1] = INA3221_CHANNEL1, > > - [INA3221_WARN2] = INA3221_CHANNEL2, > > - [INA3221_WARN3] = INA3221_CHANNEL3, > > -}; > > - > > /** > > * struct ina3221_input - channel input source specific information > > * @label: label of channel input source > > @@ -123,58 +108,6 @@ static inline bool ina3221_is_enabled(struct ina3221_data *ina, int channel) > > return ina->reg_config & INA3221_CONFIG_CHx_EN(channel); > > } > > > > -static ssize_t ina3221_show_label(struct device *dev, > > - struct device_attribute *attr, char *buf) > > -{ > > - struct sensor_device_attribute *sd_attr = to_sensor_dev_attr(attr); > > - struct ina3221_data *ina = dev_get_drvdata(dev); > > - unsigned int channel = sd_attr->index; > > - struct ina3221_input *input = &ina->inputs[channel]; > > - > > - return snprintf(buf, PAGE_SIZE, "%s\n", input->label); > > -} > > - > > -static ssize_t ina3221_show_enable(struct device *dev, > > - struct device_attribute *attr, char *buf) > > -{ > > - struct sensor_device_attribute *sd_attr = to_sensor_dev_attr(attr); > > - struct ina3221_data *ina = dev_get_drvdata(dev); > > - unsigned int channel = sd_attr->index; > > - > > - return snprintf(buf, PAGE_SIZE, "%d\n", > > - ina3221_is_enabled(ina, channel)); > > -} > > - > > -static ssize_t ina3221_set_enable(struct device *dev, > > - struct device_attribute *attr, > > - const char *buf, size_t count) > > -{ > > - struct sensor_device_attribute *sd_attr = to_sensor_dev_attr(attr); > > - struct ina3221_data *ina = dev_get_drvdata(dev); > > - unsigned int channel = sd_attr->index; > > - u16 config, mask = INA3221_CONFIG_CHx_EN(channel); > > - bool enable; > > - int ret; > > - > > - ret = kstrtobool(buf, &enable); > > - if (ret) > > - return ret; > > - > > - config = enable ? mask : 0; > > - > > - /* Enable or disable the channel */ > > - ret = regmap_update_bits(ina->regmap, INA3221_CONFIG, mask, config); > > - if (ret) > > - return ret; > > - > > - /* Cache the latest config register value */ > > - ret = regmap_read(ina->regmap, INA3221_CONFIG, &ina->reg_config); > > - if (ret) > > - return ret; > > - > > - return count; > > -} > > - > > static int ina3221_read_value(struct ina3221_data *ina, unsigned int reg, > > int *val) > > { > > @@ -190,94 +123,107 @@ static int ina3221_read_value(struct ina3221_data *ina, unsigned int reg, > > return 0; > > } > > > > -static ssize_t ina3221_show_bus_voltage(struct device *dev, > > - struct device_attribute *attr, > > - char *buf) > > +static const u8 ina3221_in_reg[] = { > > + INA3221_BUS1, > > + INA3221_BUS2, > > + INA3221_BUS3, > > + INA3221_SHUNT1, > > + INA3221_SHUNT2, > > + INA3221_SHUNT3, > > +}; > > + > > +static int ina3221_read_in(struct device *dev, u32 attr, int channel, long *val) > > { > > - struct sensor_device_attribute *sd_attr = to_sensor_dev_attr(attr); > > + const bool is_shunt = channel > INA3221_CHANNEL3; > > struct ina3221_data *ina = dev_get_drvdata(dev); > > - unsigned int reg = sd_attr->index; > > - unsigned int channel = register_channel[reg]; > > - int val, voltage_mv, ret; > > + u8 reg = ina3221_in_reg[channel]; > > + int regval, ret; > > > > - /* No data for read-only attribute if channel is disabled */ > > - if (!attr->store && !ina3221_is_enabled(ina, channel)) > > - return -ENODATA; > > + /* Translate shunt channel ID to sensor channel ID: 4->1, 5->2, 6->3 */ > > Misleading. The channel number as used here is 0-aligned, so it is 3 -> 0 > etc. > > I know the chip talks about channels 1..3, but that is not how "channel" > is used here. It took me a bit to find out that the calling code > subtracts 1 from the channel number and that the code is correct. > We don't want others to suffer the same fate. > > > + channel %= INA3221_NUM_CHANNELS; > > 3 % 3 = 0, not 1. > > > > > - ret = ina3221_read_value(ina, reg, &val); > > - if (ret) > > - return ret; > > + switch (attr) { > > + case hwmon_in_input: > > + if (!ina3221_is_enabled(ina, channel)) > > + return -ENODATA; > > > > - voltage_mv = val * 8; > > + ret = ina3221_read_value(ina, reg, ®val); > > + if (ret) > > + return ret; > > > > - return snprintf(buf, PAGE_SIZE, "%d\n", voltage_mv); > > + /* > > + * Scale of shunt voltage (uV): LSB is 40uV > > + * Scale of bus voltage (mV): LSB is 8mV > > + */ > > + *val = regval * (is_shunt ? 40 : 8); > > + return 0; > > + case hwmon_in_enable: > > + *val = ina3221_is_enabled(ina, channel); > > + return 0; > > + default: > > + return -EOPNOTSUPP; > > + } > > } > > > > -static ssize_t ina3221_show_shunt_voltage(struct device *dev, > > - struct device_attribute *attr, > > - char *buf) > > +static const u8 ina3221_curr_reg[][INA3221_NUM_CHANNELS] = { > > + [hwmon_curr_input] = { INA3221_SHUNT1, INA3221_SHUNT2, INA3221_SHUNT3 }, > > + [hwmon_curr_max] = { INA3221_WARN1, INA3221_WARN2, INA3221_WARN3 }, > > + [hwmon_curr_crit] = { INA3221_CRIT1, INA3221_CRIT2, INA3221_CRIT3 }, > > + [hwmon_curr_max_alarm] = { F_WF1, F_WF2, F_WF3 }, > > + [hwmon_curr_crit_alarm] = { F_CF1, F_CF2, F_CF3 }, > > +}; > > + > > +static int ina3221_read_curr(struct device *dev, u32 attr, > > + int channel, long *val) > > { > > - struct sensor_device_attribute *sd_attr = to_sensor_dev_attr(attr); > > struct ina3221_data *ina = dev_get_drvdata(dev); > > - unsigned int reg = sd_attr->index; > > - unsigned int channel = register_channel[reg]; > > - int val, voltage_uv, ret; > > - > > - /* No data for read-only attribute if channel is disabled */ > > - if (!attr->store && !ina3221_is_enabled(ina, channel)) > > - return -ENODATA; > > - > > - ret = ina3221_read_value(ina, reg, &val); > > - if (ret) > > - return ret; > > - voltage_uv = val * 40; > > - > > - return snprintf(buf, PAGE_SIZE, "%d\n", voltage_uv); > > -} > > - > > -static ssize_t ina3221_show_current(struct device *dev, > > - struct device_attribute *attr, char *buf) > > -{ > > - struct sensor_device_attribute *sd_attr = to_sensor_dev_attr(attr); > > - struct ina3221_data *ina = dev_get_drvdata(dev); > > - unsigned int reg = sd_attr->index; > > - unsigned int channel = register_channel[reg]; > > struct ina3221_input *input = &ina->inputs[channel]; > > int resistance_uo = input->shunt_resistor; > > - int val, current_ma, voltage_nv, ret; > > + u8 reg = ina3221_curr_reg[attr][channel]; > > + int regval, voltage_nv, ret; > > > > - /* No data for read-only attribute if channel is disabled */ > > - if (!attr->store && !ina3221_is_enabled(ina, channel)) > > - return -ENODATA; > > + switch (attr) { > > + case hwmon_curr_input: > > + if (!ina3221_is_enabled(ina, channel)) > > + return -ENODATA; > > + /* fallthrough */ > > I think the official terminology is "fall through". > But then it looks like "fallthrough" is also widely used, > so maybe we are ok. > > > + case hwmon_curr_crit: > > + case hwmon_curr_max: > > + ret = ina3221_read_value(ina, reg, ®val); > > + if (ret) > > + return ret; > > > > - ret = ina3221_read_value(ina, reg, &val); > > - if (ret) > > - return ret; > > - voltage_nv = val * 40000; > > - > > - current_ma = DIV_ROUND_CLOSEST(voltage_nv, resistance_uo); > > - > > - return snprintf(buf, PAGE_SIZE, "%d\n", current_ma); > > + /* Scale of shunt voltage: LSB is 40uV (40000nV) */ > > + voltage_nv = regval * 40000; > > + /* Return current in mA */ > > + *val = DIV_ROUND_CLOSEST(voltage_nv, resistance_uo); > > + return 0; > > + case hwmon_curr_crit_alarm: > > + case hwmon_curr_max_alarm: > > + ret = regmap_field_read(ina->fields[reg], ®val); > > + if (ret) > > + return ret; > > + *val = regval; > > + return 0; > > + default: > > + return -EOPNOTSUPP; > > + } > > } > > > > -static ssize_t ina3221_set_current(struct device *dev, > > - struct device_attribute *attr, > > - const char *buf, size_t count) > > +static int ina3221_write_curr(struct device *dev, u32 attr, > > + int channel, long val) > > { > > - struct sensor_device_attribute *sd_attr = to_sensor_dev_attr(attr); > > struct ina3221_data *ina = dev_get_drvdata(dev); > > - unsigned int reg = sd_attr->index; > > - unsigned int channel = register_channel[reg]; > > struct ina3221_input *input = &ina->inputs[channel]; > > int resistance_uo = input->shunt_resistor; > > - int val, current_ma, voltage_uv, ret; > > + u8 reg = ina3221_curr_reg[attr][channel]; > > + int regval, current_ma, voltage_uv; > > > > - ret = kstrtoint(buf, 0, ¤t_ma); > > - if (ret) > > - return ret; > > + if (attr != hwmon_curr_crit && attr != hwmon_curr_max) > > + return -EOPNOTSUPP; > > I don't think this is needed. > > > > > /* clamp current */ > > - current_ma = clamp_val(current_ma, > > + current_ma = clamp_val(val, > > INT_MIN / resistance_uo, > > INT_MAX / resistance_uo); > > > > @@ -287,15 +233,175 @@ static ssize_t ina3221_set_current(struct device *dev, > > voltage_uv = clamp_val(voltage_uv, -163800, 163800); > > > > /* 1 / 40uV(scale) << 3(register shift) = 5 */ > > - val = DIV_ROUND_CLOSEST(voltage_uv, 5) & 0xfff8; > > + regval = DIV_ROUND_CLOSEST(voltage_uv, 5) & 0xfff8; > > > > - ret = regmap_write(ina->regmap, reg, val); > > + return regmap_write(ina->regmap, reg, regval); > > +} > > + > > +static int ina3221_write_enable(struct device *dev, int channel, bool enable) > > +{ > > + struct ina3221_data *ina = dev_get_drvdata(dev); > > + u16 config, mask = INA3221_CONFIG_CHx_EN(channel); > > + int ret; > > + > > + config = enable ? mask : 0; > > + > > + /* Enable or disable the channel */ > > + ret = regmap_update_bits(ina->regmap, INA3221_CONFIG, mask, config); > > if (ret) > > return ret; > > > > - return count; > > + /* Cache the latest config register value */ > > + ret = regmap_read(ina->regmap, INA3221_CONFIG, &ina->reg_config); > > + if (ret) > > + return ret; > > + > > + return 0; > > } > > > > +static int ina3221_read(struct device *dev, enum hwmon_sensor_types type, > > + u32 attr, int channel, long *val) > > +{ > > + switch (type) { > > + case hwmon_in: > > + return ina3221_read_in(dev, attr, channel - 1, val); > > + case hwmon_curr: > > + return ina3221_read_curr(dev, attr, channel, val); > > + default: > > + return -EOPNOTSUPP; > > + } > > +} > > + > > +static int ina3221_write(struct device *dev, enum hwmon_sensor_types type, > > + u32 attr, int channel, long val) > > +{ > > + switch (type) { > > + case hwmon_in: > > + if (attr != hwmon_in_enable) > > + return -EOPNOTSUPP; > > I think you can drop this. > > > + return ina3221_write_enable(dev, channel - 1, val); > > + case hwmon_curr: > > + return ina3221_write_curr(dev, attr, channel, val); > > + default: > > + return -EOPNOTSUPP; > > I tend to have drivers return -EOPNOTSUPP for default: cases > since otherwise static checkers may complain about missing default > cases, but otherwise we should refrain from unnecessary checks. > > > + } > > +} > > + > > +static int ina3221_read_string(struct device *dev, enum hwmon_sensor_types type, > > + u32 attr, int channel, const char **str) > > +{ > > + struct ina3221_data *ina = dev_get_drvdata(dev); > > + int index = channel - 1; > > + > > + if (type != hwmon_in || attr != hwmon_in_label) > > + return -EOPNOTSUPP; > > + > > + /* Only 1-3 channels have hwmon_in_label */ > > + if (channel < 1 || channel > 3) > > + return -EOPNOTSUPP; > > + > The above checks should be unnecessary. > > > + *str = ina->inputs[index].label; > > + > > + return 0; > > +} > > + > > +static umode_t ina3221_is_visible(const void *drvdata, > > + enum hwmon_sensor_types type, > > + u32 attr, int channel) > > +{ > > + const struct ina3221_data *ina = drvdata; > > + const struct ina3221_input *input = NULL; > > + > > + switch (type) { > > + case hwmon_in: > > + /* Ignore in0_ */ > > + if (channel == 0) > > + return 0; > > + > > + switch (attr) { > > + case hwmon_in_label: > > + if (channel - 1 <= INA3221_CHANNEL3) > > + input = &ina->inputs[channel - 1]; > > + /* Hide label node if label is not provided */ > > + return (input && input->label) ? 0444 : 0; > > + case hwmon_in_input: > > + return 0444; > > + case hwmon_in_enable: > > + return 0644; > > + default: > > + return 0; > > + } > > + case hwmon_curr: > > + switch (attr) { > > + case hwmon_curr_input: > > + case hwmon_curr_crit_alarm: > > + case hwmon_curr_max_alarm: > > + return 0444; > > + case hwmon_curr_crit: > > + case hwmon_curr_max: > > + return 0644; > > + default: > > + return 0; > > + } > > + default: > > + return 0; > > + } > > +} > > + > > +static const u32 ina3221_in_config[] = { > > + /* 0: dummy, skipped in is_visible */ > > + HWMON_I_INPUT, > > + /* 1-3: input voltage Channels */ > > + HWMON_I_INPUT | HWMON_I_ENABLE | HWMON_I_LABEL, > > + HWMON_I_INPUT | HWMON_I_ENABLE | HWMON_I_LABEL, > > + HWMON_I_INPUT | HWMON_I_ENABLE | HWMON_I_LABEL, > > + /* 4-6: shunt voltage Channels */ > > + HWMON_I_INPUT, > > + HWMON_I_INPUT, > > + HWMON_I_INPUT, > > + 0 > > +}; > > + > > +static const struct hwmon_channel_info ina3221_in = { > > + .type = hwmon_in, > > + .config = ina3221_in_config, > > +}; > > + > > +#define INA3221_HWMON_CURR_CONFIG (HWMON_C_INPUT | \ > > + HWMON_C_CRIT | HWMON_C_CRIT_ALARM | \ > > + HWMON_C_MAX | HWMON_C_MAX_ALARM) > > + > > +static const u32 ina3221_curr_config[] = { > > + INA3221_HWMON_CURR_CONFIG, > > + INA3221_HWMON_CURR_CONFIG, > > + INA3221_HWMON_CURR_CONFIG, > > + 0 > > +}; > > + > > +static const struct hwmon_channel_info ina3221_curr = { > > + .type = hwmon_curr, > > + .config = ina3221_curr_config, > > +}; > > + > > +static const struct hwmon_channel_info *ina3221_info[] = { > > + &ina3221_in, > > + &ina3221_curr, > > + NULL > > +}; > > + > > +static const struct hwmon_ops ina3221_hwmon_ops = { > > + .is_visible = ina3221_is_visible, > > + .read_string = ina3221_read_string, > > + .read = ina3221_read, > > + .write = ina3221_write, > > +}; > > + > > +static const struct hwmon_chip_info ina3221_chip_info = { > > + .ops = &ina3221_hwmon_ops, > > + .info = ina3221_info, > > +}; > > + > > +/* Extra attribute groups */ > > static ssize_t ina3221_show_shunt(struct device *dev, > > struct device_attribute *attr, char *buf) > > { > > @@ -329,54 +435,6 @@ static ssize_t ina3221_set_shunt(struct device *dev, > > return count; > > } > > > > -static ssize_t ina3221_show_alert(struct device *dev, > > - struct device_attribute *attr, char *buf) > > -{ > > - struct sensor_device_attribute *sd_attr = to_sensor_dev_attr(attr); > > - struct ina3221_data *ina = dev_get_drvdata(dev); > > - unsigned int field = sd_attr->index; > > - unsigned int regval; > > - int ret; > > - > > - ret = regmap_field_read(ina->fields[field], ®val); > > - if (ret) > > - return ret; > > - > > - return snprintf(buf, PAGE_SIZE, "%d\n", regval); > > -} > > - > > -/* input channel label */ > > -static SENSOR_DEVICE_ATTR(in1_label, 0444, > > - ina3221_show_label, NULL, INA3221_CHANNEL1); > > -static SENSOR_DEVICE_ATTR(in2_label, 0444, > > - ina3221_show_label, NULL, INA3221_CHANNEL2); > > -static SENSOR_DEVICE_ATTR(in3_label, 0444, > > - ina3221_show_label, NULL, INA3221_CHANNEL3); > > - > > -/* voltage channel enable */ > > -static SENSOR_DEVICE_ATTR(in1_enable, 0644, > > - ina3221_show_enable, ina3221_set_enable, INA3221_CHANNEL1); > > -static SENSOR_DEVICE_ATTR(in2_enable, 0644, > > - ina3221_show_enable, ina3221_set_enable, INA3221_CHANNEL2); > > -static SENSOR_DEVICE_ATTR(in3_enable, 0644, > > - ina3221_show_enable, ina3221_set_enable, INA3221_CHANNEL3); > > - > > -/* bus voltage */ > > -static SENSOR_DEVICE_ATTR(in1_input, S_IRUGO, > > - ina3221_show_bus_voltage, NULL, INA3221_BUS1); > > -static SENSOR_DEVICE_ATTR(in2_input, S_IRUGO, > > - ina3221_show_bus_voltage, NULL, INA3221_BUS2); > > -static SENSOR_DEVICE_ATTR(in3_input, S_IRUGO, > > - ina3221_show_bus_voltage, NULL, INA3221_BUS3); > > - > > -/* calculated current */ > > -static SENSOR_DEVICE_ATTR(curr1_input, S_IRUGO, > > - ina3221_show_current, NULL, INA3221_SHUNT1); > > -static SENSOR_DEVICE_ATTR(curr2_input, S_IRUGO, > > - ina3221_show_current, NULL, INA3221_SHUNT2); > > -static SENSOR_DEVICE_ATTR(curr3_input, S_IRUGO, > > - ina3221_show_current, NULL, INA3221_SHUNT3); > > - > > /* shunt resistance */ > > static SENSOR_DEVICE_ATTR(shunt1_resistor, S_IRUGO | S_IWUSR, > > ina3221_show_shunt, ina3221_set_shunt, INA3221_CHANNEL1); > > @@ -385,109 +443,13 @@ static SENSOR_DEVICE_ATTR(shunt2_resistor, S_IRUGO | S_IWUSR, > > static SENSOR_DEVICE_ATTR(shunt3_resistor, S_IRUGO | S_IWUSR, > > ina3221_show_shunt, ina3221_set_shunt, INA3221_CHANNEL3); > > > > -/* critical current */ > > -static SENSOR_DEVICE_ATTR(curr1_crit, S_IRUGO | S_IWUSR, > > - ina3221_show_current, ina3221_set_current, INA3221_CRIT1); > > -static SENSOR_DEVICE_ATTR(curr2_crit, S_IRUGO | S_IWUSR, > > - ina3221_show_current, ina3221_set_current, INA3221_CRIT2); > > -static SENSOR_DEVICE_ATTR(curr3_crit, S_IRUGO | S_IWUSR, > > - ina3221_show_current, ina3221_set_current, INA3221_CRIT3); > > - > > -/* critical current alert */ > > -static SENSOR_DEVICE_ATTR(curr1_crit_alarm, S_IRUGO, > > - ina3221_show_alert, NULL, F_CF1); > > -static SENSOR_DEVICE_ATTR(curr2_crit_alarm, S_IRUGO, > > - ina3221_show_alert, NULL, F_CF2); > > -static SENSOR_DEVICE_ATTR(curr3_crit_alarm, S_IRUGO, > > - ina3221_show_alert, NULL, F_CF3); > > - > > -/* warning current */ > > -static SENSOR_DEVICE_ATTR(curr1_max, S_IRUGO | S_IWUSR, > > - ina3221_show_current, ina3221_set_current, INA3221_WARN1); > > -static SENSOR_DEVICE_ATTR(curr2_max, S_IRUGO | S_IWUSR, > > - ina3221_show_current, ina3221_set_current, INA3221_WARN2); > > -static SENSOR_DEVICE_ATTR(curr3_max, S_IRUGO | S_IWUSR, > > - ina3221_show_current, ina3221_set_current, INA3221_WARN3); > > - > > -/* warning current alert */ > > -static SENSOR_DEVICE_ATTR(curr1_max_alarm, S_IRUGO, > > - ina3221_show_alert, NULL, F_WF1); > > -static SENSOR_DEVICE_ATTR(curr2_max_alarm, S_IRUGO, > > - ina3221_show_alert, NULL, F_WF2); > > -static SENSOR_DEVICE_ATTR(curr3_max_alarm, S_IRUGO, > > - ina3221_show_alert, NULL, F_WF3); > > - > > -/* shunt voltage */ > > -static SENSOR_DEVICE_ATTR(in4_input, S_IRUGO, > > - ina3221_show_shunt_voltage, NULL, INA3221_SHUNT1); > > -static SENSOR_DEVICE_ATTR(in5_input, S_IRUGO, > > - ina3221_show_shunt_voltage, NULL, INA3221_SHUNT2); > > -static SENSOR_DEVICE_ATTR(in6_input, S_IRUGO, > > - ina3221_show_shunt_voltage, NULL, INA3221_SHUNT3); > > - > > static struct attribute *ina3221_attrs[] = { > > - /* channel 1 -- make sure label at first */ > > - &sensor_dev_attr_in1_label.dev_attr.attr, > > - &sensor_dev_attr_in1_enable.dev_attr.attr, > > - &sensor_dev_attr_in1_input.dev_attr.attr, > > - &sensor_dev_attr_curr1_input.dev_attr.attr, > > &sensor_dev_attr_shunt1_resistor.dev_attr.attr, > > - &sensor_dev_attr_curr1_crit.dev_attr.attr, > > - &sensor_dev_attr_curr1_crit_alarm.dev_attr.attr, > > - &sensor_dev_attr_curr1_max.dev_attr.attr, > > - &sensor_dev_attr_curr1_max_alarm.dev_attr.attr, > > - &sensor_dev_attr_in4_input.dev_attr.attr, > > - > > - /* channel 2 -- make sure label at first */ > > - &sensor_dev_attr_in2_label.dev_attr.attr, > > - &sensor_dev_attr_in2_enable.dev_attr.attr, > > - &sensor_dev_attr_in2_input.dev_attr.attr, > > - &sensor_dev_attr_curr2_input.dev_attr.attr, > > &sensor_dev_attr_shunt2_resistor.dev_attr.attr, > > - &sensor_dev_attr_curr2_crit.dev_attr.attr, > > - &sensor_dev_attr_curr2_crit_alarm.dev_attr.attr, > > - &sensor_dev_attr_curr2_max.dev_attr.attr, > > - &sensor_dev_attr_curr2_max_alarm.dev_attr.attr, > > - &sensor_dev_attr_in5_input.dev_attr.attr, > > - > > - /* channel 3 -- make sure label at first */ > > - &sensor_dev_attr_in3_label.dev_attr.attr, > > - &sensor_dev_attr_in3_enable.dev_attr.attr, > > - &sensor_dev_attr_in3_input.dev_attr.attr, > > - &sensor_dev_attr_curr3_input.dev_attr.attr, > > &sensor_dev_attr_shunt3_resistor.dev_attr.attr, > > - &sensor_dev_attr_curr3_crit.dev_attr.attr, > > - &sensor_dev_attr_curr3_crit_alarm.dev_attr.attr, > > - &sensor_dev_attr_curr3_max.dev_attr.attr, > > - &sensor_dev_attr_curr3_max_alarm.dev_attr.attr, > > - &sensor_dev_attr_in6_input.dev_attr.attr, > > - > > NULL, > > }; > > - > > -static umode_t ina3221_attr_is_visible(struct kobject *kobj, > > - struct attribute *attr, int n) > > -{ > > - const int max_attrs = ARRAY_SIZE(ina3221_attrs) - 1; > > - const int num_attrs = max_attrs / INA3221_NUM_CHANNELS; > > - struct device *dev = kobj_to_dev(kobj); > > - struct ina3221_data *ina = dev_get_drvdata(dev); > > - enum ina3221_channels channel = n / num_attrs; > > - struct ina3221_input *input = &ina->inputs[channel]; > > - int index = n % num_attrs; > > - > > - /* Hide label node if label is not provided */ > > - if (index == 0 && !input->label) > > - return 0; > > - > > - return attr->mode; > > -} > > - > > -static const struct attribute_group ina3221_group = { > > - .is_visible = ina3221_attr_is_visible, > > - .attrs = ina3221_attrs, > > -}; > > -__ATTRIBUTE_GROUPS(ina3221); > > +ATTRIBUTE_GROUPS(ina3221); > > > > static const struct regmap_range ina3221_yes_ranges[] = { > > regmap_reg_range(INA3221_CONFIG, INA3221_BUS3), > > @@ -620,9 +582,9 @@ static int ina3221_probe(struct i2c_client *client, > > > > dev_set_drvdata(dev, ina); > > > > - hwmon_dev = devm_hwmon_device_register_with_groups(dev, > > - client->name, > > - ina, ina3221_groups); > > + hwmon_dev = devm_hwmon_device_register_with_info(dev, client->name, ina, > > + &ina3221_chip_info, > > + ina3221_groups); > > if (IS_ERR(hwmon_dev)) { > > dev_err(dev, "Unable to register hwmon device\n"); > > return PTR_ERR(hwmon_dev); ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2018-10-07 7:07 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-10-05 23:59 [PATCH 0/2] hwmon: (ina3221) Apply _info API Nicolin Chen 2018-10-05 23:59 ` [PATCH 1/2] hwmon: (core) Add hwmon_in_enable attribute Nicolin Chen 2018-10-06 19:24 ` Guenter Roeck 2018-10-05 23:59 ` [PATCH 2/2] hwmon: (ina3221) Use _info API to register hwmon device Nicolin Chen 2018-10-06 20:05 ` Guenter Roeck 2018-10-07 7:07 ` Nicolin Chen
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.