On Thu, Sep 27, 2018 at 01:54:06PM -0700, Nicolin Chen wrote:
> An ina3221 chip has three input ports. Each port is used
> to measure the voltage and current of its input source.
>
> The DT binding now has defined bindings for their input
> sources, so the driver should read these information and
> handle accordingly.
>
> This patch adds a new structure of input source specific
> information including input source label, shunt resistor
> value and its connection status. It exposes these labels
> via in[123]_label sysfs nodes upon available, and also
> disables those channels where there are no input source
> being connected. Meanwhile, it also adds in[123]_enable
> sysfs nodes for users to get control of three channels,
> and returns -ENODATA code for any sensor read according
> to hwmon ABI.
>
> Signed-off-by: Nicolin Chen <nicoleotsuka@xxxxxxxxx>
> ---
> Changelog
> v6->v7:
> * N/A
> v5->v6:
> * Removed the code of hiding disconnected channels
> * Added in[123]_label sysfs nodes to control channels
> * Added -ENODATA return for sensor read at disabled channels
> v4->v5:
> * Replaced "shunt-resistor" with "shunt-resistor-micro-ohms"
> * Replaced "input-label" with "label"
> * Replaced "input-id" with "reg"
> * Simplified is_visible() by using index of the attr
> * Moved inX_label to index-0 and added comments for safety
> v3->v4:
> * Added is_visible callback function to hide sysfs nodes
> v2->v3:
> * N/A
> v1->v2:
> * Added a structure for input source corresponding to DT bindings
> * Moved shunt resistor value to the structure
> * Defined in[123]_label sysfs nodes instead of name[123]_input
> * Updated probe() function to get information from DT
> * Updated ina3221 hwmon documentation for the labels
> * Dropped dynamical group definition to keep all groups as they were
>
> Documentation/hwmon/ina3221 | 2 +
> drivers/hwmon/ina3221.c | 237 +++++++++++++++++++++++++++++++++---
> 2 files changed, 225 insertions(+), 14 deletions(-)
>
> diff --git a/Documentation/hwmon/ina3221 b/Documentation/hwmon/ina3221
> index 0ff74854cb2e..4b82cbfb551c 100644
> --- a/Documentation/hwmon/ina3221
> +++ b/Documentation/hwmon/ina3221
> @@ -21,6 +21,8 @@ and power are calculated host-side from these.
> Sysfs entries
> -------------
>
> +in[123]_label Voltage channel labels
> +in[123]_enable Voltage channel enable controls
> in[123]_input Bus voltage(mV) channels
> curr[123]_input Current(mA) measurement channels
> shunt[123]_resistor Shunt resistance(uOhm) channels
> diff --git a/drivers/hwmon/ina3221.c b/drivers/hwmon/ina3221.c
> index e6b49500c52a..5c16d78e7482 100644
> --- a/drivers/hwmon/ina3221.c
> +++ b/drivers/hwmon/ina3221.c
> @@ -41,6 +41,7 @@
> #define INA3221_CONFIG_MODE_SHUNT BIT(1)
> #define INA3221_CONFIG_MODE_BUS BIT(2)
> #define INA3221_CONFIG_MODE_CONTINUOUS BIT(3)
> +#define INA3221_CONFIG_CHx_EN(x) BIT(14 - (x))
>
> #define INA3221_RSHUNT_DEFAULT 10000
>
> @@ -75,6 +76,9 @@ enum ina3221_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,
> @@ -86,18 +90,93 @@ static const unsigned int register_channel[] = {
> [INA3221_WARN3] = INA3221_CHANNEL3,
> };
>
> +/**
> + * struct ina3221_input - channel input source specific information
> + * @label: label of channel input source
> + * @shunt_resistor: shunt resistor value of channel input source
> + * @disconnected: connection status of channel input source
> + */
> +struct ina3221_input {
> + const char *label;
> + int shunt_resistor;
> + bool disconnected;
> +};
> +
> /**
> * struct ina3221_data - device specific information
> * @regmap: Register map of the device
> * @fields: Register fields of the device
> - * @shunt_resistors: Array of resistor values per channel
> + * @inputs: Array of channel input source specific structures
> */
> struct ina3221_data {
> struct regmap *regmap;
> struct regmap_field *fields[F_MAX_FIELDS];
> - int shunt_resistors[INA3221_NUM_CHANNELS];
> + struct ina3221_input inputs[INA3221_NUM_CHANNELS];
> };
>
> +static inline bool ina3221_is_enable(struct ina3221_data *ina, int channel)
> +{
> + unsigned int config;
> + int ret;
> +
> + /* Return false to reject further read */
> + ret = regmap_read(ina->regmap, INA3221_CONFIG, &config);
> + if (ret)
> + return false;
> +
> + return (config & INA3221_CONFIG_CHx_EN(channel)) > 0;
I had asked you to either drop the "> 0" or use !!.
> +}
> +
> +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_enable(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;
> + unsigned int mask = INA3221_CONFIG_CHx_EN(channel);
> + unsigned int config;
> + int val, ret;
> +
> + ret = kstrtoint(buf, 0, &val);
> + if (ret)
> + return ret;
> +
> + /* inX_enable only accepts 1 for enabling or 0 for disabling */
> + if (val != 0 && val != 1)
> + return -EINVAL;
> +
I am quite sure I asked to use kstrtobool(). Did that get lost or do you have
some reason to not use it ?
I can understand if you don't want to change ina3221_is_enable() to
ina3221_is_enabled(), since that is POV, but I don't really understand
why you did not make the other changes. Am I missing something ?
Thanks,
Guenter
> + config = val ? mask : 0;
> +
> + ret = regmap_update_bits(ina->regmap, INA3221_CONFIG, mask, config);
> + if (ret)
> + return ret;
> +
> + return count;
> +}
> +
> static int ina3221_read_value(struct ina3221_data *ina, unsigned int reg,
> int *val)
> {
> @@ -120,8 +199,13 @@ static ssize_t ina3221_show_bus_voltage(struct device *dev,
> 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_mv, ret;
>
> + /* No data for read-only attribute if channel is disabled */
> + if (!attr->store && !ina3221_is_enable(ina, channel))
> + return -ENODATA;
> +
> ret = ina3221_read_value(ina, reg, &val);
> if (ret)
> return ret;
> @@ -138,8 +222,13 @@ static ssize_t ina3221_show_shunt_voltage(struct device *dev,
> 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_enable(ina, channel))
> + return -ENODATA;
> +
> ret = ina3221_read_value(ina, reg, &val);
> if (ret)
> return ret;
> @@ -155,9 +244,14 @@ static ssize_t ina3221_show_current(struct device *dev,
> struct ina3221_data *ina = dev_get_drvdata(dev);
> unsigned int reg = sd_attr->index;
> unsigned int channel = register_channel[reg];
> - int resistance_uo = ina->shunt_resistors[channel];
> + struct ina3221_input *input = &ina->inputs[channel];
> + int resistance_uo = input->shunt_resistor;
> int val, current_ma, voltage_nv, ret;
>
> + /* No data for read-only attribute if channel is disabled */
> + if (!attr->store && !ina3221_is_enable(ina, channel))
> + return -ENODATA;
> +
> ret = ina3221_read_value(ina, reg, &val);
> if (ret)
> return ret;
> @@ -176,7 +270,8 @@ static ssize_t ina3221_set_current(struct device *dev,
> struct ina3221_data *ina = dev_get_drvdata(dev);
> unsigned int reg = sd_attr->index;
> unsigned int channel = register_channel[reg];
> - int resistance_uo = ina->shunt_resistors[channel];
> + struct ina3221_input *input = &ina->inputs[channel];
> + int resistance_uo = input->shunt_resistor;
> int val, current_ma, voltage_uv, ret;
>
> ret = kstrtoint(buf, 0, ¤t_ma);
> @@ -209,11 +304,9 @@ static ssize_t ina3221_show_shunt(struct device *dev,
> 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;
> - unsigned int resistance_uo;
> -
> - resistance_uo = ina->shunt_resistors[channel];
> + struct ina3221_input *input = &ina->inputs[channel];
>
> - return snprintf(buf, PAGE_SIZE, "%d\n", resistance_uo);
> + return snprintf(buf, PAGE_SIZE, "%d\n", input->shunt_resistor);
> }
>
> static ssize_t ina3221_set_shunt(struct device *dev,
> @@ -223,6 +316,7 @@ static ssize_t ina3221_set_shunt(struct device *dev,
> 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];
> int val;
> int ret;
>
> @@ -232,7 +326,7 @@ static ssize_t ina3221_set_shunt(struct device *dev,
>
> val = clamp_val(val, 1, INT_MAX);
>
> - ina->shunt_resistors[channel] = val;
> + input->shunt_resistor = val;
>
> return count;
> }
> @@ -253,6 +347,22 @@ static ssize_t ina3221_show_alert(struct device *dev,
> 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);
> @@ -318,7 +428,9 @@ static SENSOR_DEVICE_ATTR(in6_input, S_IRUGO,
> ina3221_show_shunt_voltage, NULL, INA3221_SHUNT3);
>
> static struct attribute *ina3221_attrs[] = {
> - /* channel 1 */
> + /* 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,
> @@ -328,7 +440,9 @@ static struct attribute *ina3221_attrs[] = {
> &sensor_dev_attr_curr1_max_alarm.dev_attr.attr,
> &sensor_dev_attr_in4_input.dev_attr.attr,
>
> - /* channel 2 */
> + /* 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,
> @@ -338,7 +452,9 @@ static struct attribute *ina3221_attrs[] = {
> &sensor_dev_attr_curr2_max_alarm.dev_attr.attr,
> &sensor_dev_attr_in5_input.dev_attr.attr,
>
> - /* channel 3 */
> + /* 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,
> @@ -350,7 +466,30 @@ static struct attribute *ina3221_attrs[] = {
>
> NULL,
> };
> -ATTRIBUTE_GROUPS(ina3221);
> +
> +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);
>
> static const struct regmap_range ina3221_yes_ranges[] = {
> regmap_reg_range(INA3221_SHUNT1, INA3221_BUS3),
> @@ -370,6 +509,60 @@ static const struct regmap_config ina3221_regmap_config = {
> .volatile_table = &ina3221_volatile_table,
> };
>
> +static int ina3221_probe_child_from_dt(struct device *dev,
> + struct device_node *child,
> + struct ina3221_data *ina)
> +{
> + struct ina3221_input *input;
> + u32 val;
> + int ret;
> +
> + ret = of_property_read_u32(child, "reg", &val);
> + if (ret) {
> + dev_err(dev, "missing reg property of %s\n", child->name);
> + return ret;
> + } else if (val > INA3221_CHANNEL3) {
> + dev_err(dev, "invalid reg %d of %s\n", val, child->name);
> + return ret;
> + }
> +
> + input = &ina->inputs[val];
> +
> + /* Log the disconnected channel input */
> + if (!of_device_is_available(child)) {
> + input->disconnected = true;
> + return 0;
> + }
> +
> + /* Save the connected input label if available */
> + of_property_read_string(child, "label", &input->label);
> +
> + /* Overwrite default shunt resistor value optionally */
> + if (!of_property_read_u32(child, "shunt-resistor-micro-ohms", &val))
> + input->shunt_resistor = val;
> +
> + return 0;
> +}
> +
> +static int ina3221_probe_from_dt(struct device *dev, struct ina3221_data *ina)
> +{
> + const struct device_node *np = dev->of_node;
> + struct device_node *child;
> + int ret;
> +
> + /* Compatible with non-DT platforms */
> + if (!np)
> + return 0;
> +
> + for_each_child_of_node(np, child) {
> + ret = ina3221_probe_child_from_dt(dev, child, ina);
> + if (ret)
> + return ret;
> + }
> +
> + return 0;
> +}
> +
> static int ina3221_probe(struct i2c_client *client,
> const struct i2c_device_id *id)
> {
> @@ -377,6 +570,7 @@ static int ina3221_probe(struct i2c_client *client,
> struct ina3221_data *ina;
> struct device *hwmon_dev;
> int i, ret;
> + u16 mask;
>
> ina = devm_kzalloc(dev, sizeof(*ina), GFP_KERNEL);
> if (!ina)
> @@ -399,7 +593,13 @@ static int ina3221_probe(struct i2c_client *client,
> }
>
> for (i = 0; i < INA3221_NUM_CHANNELS; i++)
> - ina->shunt_resistors[i] = INA3221_RSHUNT_DEFAULT;
> + ina->inputs[i].shunt_resistor = INA3221_RSHUNT_DEFAULT;
> +
> + ret = ina3221_probe_from_dt(dev, ina);
> + if (ret) {
> + dev_err(dev, "Unable to probe from device treee\n");
> + return ret;
> + }
>
> ret = regmap_field_write(ina->fields[F_RST], true);
> if (ret) {
> @@ -407,6 +607,15 @@ static int ina3221_probe(struct i2c_client *client,
> return ret;
> }
>
> + /* Disable channels if their inputs are disconnected */
> + for (i = 0, mask = 0; i < INA3221_NUM_CHANNELS; i++) {
> + if (ina->inputs[i].disconnected)
> + mask |= INA3221_CONFIG_CHx_EN(i);
> + }
> + ret = regmap_update_bits(ina->regmap, INA3221_CONFIG, mask, 0);
> + if (ret)
> + return ret;
> +
> hwmon_dev = devm_hwmon_device_register_with_groups(dev,
> client->name,
> ina, ina3221_groups);
> --
> 2.17.1
>
