Re: [PATCH dev-5.0 2/2] fixup! hwmon: (pmbus/isl68137): Add driver for Intersil ISL68137 PWM Controller
Andrew Jeffery
andrew at aj.id.au
Fri Apr 19 12:25:43 AEST 2019
On Fri, 19 Apr 2019, at 10:14, Andrew Jeffery wrote:
>
>
> On Fri, 19 Apr 2019, at 00:06, Patrick Venture wrote:
> > Apply differences from hwmon review to bring the driver code to the same
> > point. There were some key differences regarding attribute placement
> > from the review, beyond the normal cleanup.
> >
> > Signed-off-by: Patrick Venture <venture at google.com>
> > ---
> > Documentation/hwmon/isl68137 | 72 +++++++++++++
> > drivers/hwmon/pmbus/isl68137.c | 188 ++++++++++++++-------------------
> > 2 files changed, 149 insertions(+), 111 deletions(-)
> > create mode 100644 Documentation/hwmon/isl68137
> >
> > diff --git a/Documentation/hwmon/isl68137 b/Documentation/hwmon/isl68137
> > new file mode 100644
> > index 000000000000..92e5c5fc5b77
> > --- /dev/null
> > +++ b/Documentation/hwmon/isl68137
> > @@ -0,0 +1,72 @@
> > +Kernel driver isl68137
> > +======================
> > +
> > +Supported chips:
> > + * Intersil ISL68137
> > + Prefix: 'isl68137'
> > + Addresses scanned: -
> > + Datasheet: Publicly available at the Intersil website
> > +
> > https://www.intersil.com/content/dam/Intersil/documents/isl6/isl68137.pdf
> > +
> > +Authors:
> > + Maxim Sloyko <maxims at google.com>
> > + Robert Lippert <rlippert at google.com>
> > + Patrick Venture <venture at google.com>
> > +
> > +Description
> > +-----------
> > +
> > +Intersil ISL68137 is a digital output 7-phase configurable PWM
> > +controller with an AVSBus interface.
> > +
> > +Usage Notes
> > +-----------
> > +
> > +This driver does not probe for PMBus devices. You will have to
> > instantiate
> > +devices explicitly.
> > +
> > +The ISL68137 AVS operation mode must be enabled/disabled at runtime.
> > +
> > +Beyond the normal sysfs pmbus attributes, the driver exposes a control
> > attribute.
> > +
> > +Additional Sysfs attributes
> > +---------------------------
> > +
> > +avs(0|1)_enable Controls the AVS state of each rail.
> > +
> > +curr1_label "iin"
> > +curr1_input Measured input current
> > +curr1_crit Critical maximum current
> > +curr1_crit_alarm Current critical high alarm
> > +
> > +curr[2-3]_label "iout[1-2]"
> > +curr[2-3]_input Measured output current
> > +curr[2-3]_crit Critical maximum current
> > +curr[2-3]_crit_alarm Current critical high alarm
> > +
> > +in1_label "vin"
> > +in1_input Measured input voltage
> > +in1_lcrit Critical minimum input voltage
> > +in1_lcrit_alarm Input voltage critical low alarm
> > +in1_crit Critical maximum input voltage
> > +in1_crit_alarm Input voltage critical high alarm
> > +
> > +in[2-3]_label "vout[1-2]"
> > +in[2-3]_input Measured output voltage
> > +in[2-3]_lcrit Critical minimum output voltage
> > +in[2-3]_lcrit_alarm Output voltage critical low alarm
> > +in[2-3]_crit Critical maximum output voltage
> > +in[2-3]_crit_alarm Output voltage critical high alarm
> > +
> > +power1_label "pin"
> > +power1_input Measured input power
> > +power1_alarm Input power high alarm
> > +
> > +power[2-3]_label "pout[1-2]"
> > +power[2-3]_input Measured output power
> > +
> > +temp[1-3]_input Measured temperature
> > +temp[1-3]_crit Critical high temperature
> > +temp[1-3]_crit_alarm Chip temperature critical high alarm
> > +temp[1-3]_max Maximum temperature
> > +temp[1-3]_max_alarm Chip temperature high alarm
> > diff --git a/drivers/hwmon/pmbus/isl68137.c
> > b/drivers/hwmon/pmbus/isl68137.c
> > index ccac14bdb985..515596c92fe1 100644
> > --- a/drivers/hwmon/pmbus/isl68137.c
> > +++ b/drivers/hwmon/pmbus/isl68137.c
> > @@ -4,61 +4,19 @@
> > *
> > * Copyright (c) 2017 Google Inc
> > *
> > - * This program is free software; you can redistribute it and/or modify
> > - * it under the terms of the GNU General Public License as published by
> > - * the Free Software Foundation; either version 2 of the License, or
> > - * (at your option) any later version.
> > - *
> > - * This program is distributed in the hope that it will be useful,
> > - * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > - * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> > - * GNU General Public License for more details.
> > */
> >
> > -#include <linux/kernel.h>
> > -#include <linux/module.h>
> > -#include <linux/init.h>
> > #include <linux/err.h>
> > -#include <linux/i2c.h>
> > #include <linux/hwmon-sysfs.h>
> > +#include <linux/i2c.h>
> > +#include <linux/init.h>
> > +#include <linux/kernel.h>
> > +#include <linux/module.h>
> > +#include <linux/string.h>
> > +#include <linux/sysfs.h>
> > #include "pmbus.h"
> >
> > -#define ISL68137_VOUT_AVS 0x30
> > -
> > -static struct pmbus_driver_info isl68137_info = {
> > - .pages = 2,
> > - .format[PSC_VOLTAGE_IN] = direct,
> > - .format[PSC_VOLTAGE_OUT] = direct,
> > - .format[PSC_CURRENT_IN] = direct,
> > - .format[PSC_CURRENT_OUT] = direct,
> > - .format[PSC_POWER] = direct,
> > - .format[PSC_TEMPERATURE] = direct,
> > - .m[PSC_VOLTAGE_IN] = 1,
> > - .b[PSC_VOLTAGE_IN] = 0,
> > - .R[PSC_VOLTAGE_IN] = 3,
> > - .m[PSC_VOLTAGE_OUT] = 1,
> > - .b[PSC_VOLTAGE_OUT] = 0,
> > - .R[PSC_VOLTAGE_OUT] = 3,
> > - .m[PSC_CURRENT_IN] = 1,
> > - .b[PSC_CURRENT_IN] = 0,
> > - .R[PSC_CURRENT_IN] = 2,
> > - .m[PSC_CURRENT_OUT] = 1,
> > - .b[PSC_CURRENT_OUT] = 0,
> > - .R[PSC_CURRENT_OUT] = 1,
> > - .m[PSC_POWER] = 1,
> > - .b[PSC_POWER] = 0,
> > - .R[PSC_POWER] = 0,
> > - .m[PSC_TEMPERATURE] = 1,
> > - .b[PSC_TEMPERATURE] = 0,
> > - .R[PSC_TEMPERATURE] = 0,
> > - .func[0] = PMBUS_HAVE_VIN | PMBUS_HAVE_IIN | PMBUS_HAVE_PIN
> > - | PMBUS_HAVE_STATUS_INPUT | PMBUS_HAVE_TEMP | PMBUS_HAVE_TEMP2
> > - | PMBUS_HAVE_TEMP3 | PMBUS_HAVE_STATUS_TEMP
> > - | PMBUS_HAVE_VOUT | PMBUS_HAVE_STATUS_VOUT
> > - | PMBUS_HAVE_IOUT | PMBUS_HAVE_STATUS_IOUT | PMBUS_HAVE_POUT,
> > - .func[1] = PMBUS_HAVE_VOUT | PMBUS_HAVE_STATUS_VOUT
> > - | PMBUS_HAVE_IOUT | PMBUS_HAVE_STATUS_IOUT | PMBUS_HAVE_POUT,
> > -};
> > +#define ISL68137_VOUT_AVS 0x30
> >
> > static ssize_t isl68137_avs_enable_show_page(struct i2c_client *client,
> > int page,
> > @@ -75,23 +33,13 @@ static ssize_t
> > isl68137_avs_enable_store_page(struct i2c_client *client,
> > const char *buf, size_t count)
> > {
> > int rc, op_val;
> > + bool result;
> >
> > - if (count < 1) {
> > - rc = -EINVAL;
> > - goto out;
> > - }
> > + rc = kstrtobool(buf, &result);
> > + if (rc)
> > + return rc;
> >
> > - switch (buf[0]) {
> > - case '0':
> > - op_val = 0;
> > - break;
> > - case '1':
> > - op_val = ISL68137_VOUT_AVS;
> > - break;
> > - default:
> > - rc = -EINVAL;
> > - goto out;
> > - }
> > + op_val = result ? ISL68137_VOUT_AVS : 0;
> >
> > /*
> > * Writes to VOUT setpoint over AVSBus will persist after the VRM is
> > @@ -101,27 +49,28 @@ static ssize_t
> > isl68137_avs_enable_store_page(struct i2c_client *client,
> > * enabling AVS control is the workaround.
> > */
> > if (op_val == ISL68137_VOUT_AVS) {
> > - int vout_command = pmbus_read_word_data(client, page,
> > - PMBUS_VOUT_COMMAND);
> > + rc = pmbus_read_word_data(client, page, PMBUS_VOUT_COMMAND);
> > + if (rc < 0)
> > + return rc;
> > +
> > rc = pmbus_write_word_data(client, page, PMBUS_VOUT_COMMAND,
> > - vout_command);
> > - if (rc)
> > - goto out;
> > + rc);
> > + if (rc < 0)
> > + return rc;
> > }
> >
> > rc = pmbus_update_byte_data(client, page, PMBUS_OPERATION,
> > ISL68137_VOUT_AVS, op_val);
> >
> > -out:
> > - return rc < 0 ? rc : count;
> > + return (rc < 0) ? rc : count;
> > }
> >
> > static ssize_t isl68137_avs_enable_show(struct device *dev,
> > struct device_attribute *devattr,
> > char *buf)
> > {
> > - struct i2c_client *client = kobj_to_i2c_client(&dev->kobj);
> > - struct sensor_device_attribute_2 *attr = to_sensor_dev_attr_2(devattr);
> > + struct i2c_client *client = to_i2c_client(dev->parent);
> > + struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
> >
> > return isl68137_avs_enable_show_page(client, attr->index, buf);
> > }
> > @@ -130,53 +79,70 @@ static ssize_t isl68137_avs_enable_store(struct
> > device *dev,
> > struct device_attribute *devattr,
> > const char *buf, size_t count)
> > {
> > - struct i2c_client *client = kobj_to_i2c_client(&dev->kobj);
> > - struct sensor_device_attribute_2 *attr =
> > to_sensor_dev_attr_2(devattr);
> > + struct i2c_client *client = to_i2c_client(dev->parent);
> > + struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
> >
> > return isl68137_avs_enable_store_page(client, attr->index, buf, count);
> > }
> >
> > -static SENSOR_DEVICE_ATTR_2(avs0_enabled, 0644,
> > - isl68137_avs_enable_show, isl68137_avs_enable_store,
> > - 0, 0);
> > -static SENSOR_DEVICE_ATTR_2(avs1_enabled, 0644,
> > - isl68137_avs_enable_show, isl68137_avs_enable_store,
> > - 0, 1);
> > +static SENSOR_DEVICE_ATTR_RW(avs0_enable, isl68137_avs_enable, 0);
> > +static SENSOR_DEVICE_ATTR_RW(avs1_enable, isl68137_avs_enable, 1);
> >
> > -static int isl68137_remove(struct i2c_client *client);
> > -
> > -static int isl68137_probe(struct i2c_client *client,
> > - const struct i2c_device_id *id)
> > -{
> > - int rc;
> > -
> > - rc = pmbus_do_probe(client, id, &isl68137_info);
> > - if (rc)
> > - return rc;
> > +static struct attribute *enable_attrs[] = {
> > + &sensor_dev_attr_avs0_enable.dev_attr.attr,
> > + &sensor_dev_attr_avs1_enable.dev_attr.attr,
> > + NULL,
> > +};
> >
> > - rc = device_create_file(&client->dev,
> > - &sensor_dev_attr_avs0_enabled.dev_attr);
> > - if (rc)
> > - goto out_fail;
> > - rc = device_create_file(&client->dev,
> > - &sensor_dev_attr_avs1_enabled.dev_attr);
> > - if (rc)
> > - goto out_fail;
> > +static const struct attribute_group enable_group = {
> > + .attrs = enable_attrs,
> > +};
> >
> > - return rc;
> > +static const struct attribute_group *attribute_groups[] = {
> > + &enable_group,
> > + NULL,
> > +};
> >
> > -out_fail:
> > - isl68137_remove(client);
> > - return rc;
> > -}
> > +static struct pmbus_driver_info isl68137_info = {
> > + .pages = 2,
> > + .format[PSC_VOLTAGE_IN] = direct,
> > + .format[PSC_VOLTAGE_OUT] = direct,
> > + .format[PSC_CURRENT_IN] = direct,
> > + .format[PSC_CURRENT_OUT] = direct,
> > + .format[PSC_POWER] = direct,
> > + .format[PSC_TEMPERATURE] = direct,
> > + .m[PSC_VOLTAGE_IN] = 1,
> > + .b[PSC_VOLTAGE_IN] = 0,
> > + .R[PSC_VOLTAGE_IN] = 3,
> > + .m[PSC_VOLTAGE_OUT] = 1,
> > + .b[PSC_VOLTAGE_OUT] = 0,
> > + .R[PSC_VOLTAGE_OUT] = 3,
> > + .m[PSC_CURRENT_IN] = 1,
> > + .b[PSC_CURRENT_IN] = 0,
> > + .R[PSC_CURRENT_IN] = 2,
> > + .m[PSC_CURRENT_OUT] = 1,
> > + .b[PSC_CURRENT_OUT] = 0,
> > + .R[PSC_CURRENT_OUT] = 1,
> > + .m[PSC_POWER] = 1,
> > + .b[PSC_POWER] = 0,
> > + .R[PSC_POWER] = 0,
> > + .m[PSC_TEMPERATURE] = 1,
> > + .b[PSC_TEMPERATURE] = 0,
> > + .R[PSC_TEMPERATURE] = 0,
> > + .func[0] = PMBUS_HAVE_VIN | PMBUS_HAVE_IIN | PMBUS_HAVE_PIN
> > + | PMBUS_HAVE_STATUS_INPUT | PMBUS_HAVE_TEMP | PMBUS_HAVE_TEMP2
> > + | PMBUS_HAVE_TEMP3 | PMBUS_HAVE_STATUS_TEMP
> > + | PMBUS_HAVE_VOUT | PMBUS_HAVE_STATUS_VOUT
> > + | PMBUS_HAVE_IOUT | PMBUS_HAVE_STATUS_IOUT | PMBUS_HAVE_POUT,
> > + .func[1] = PMBUS_HAVE_VOUT | PMBUS_HAVE_STATUS_VOUT
> > + | PMBUS_HAVE_IOUT | PMBUS_HAVE_STATUS_IOUT | PMBUS_HAVE_POUT,
> > + .groups = attribute_groups,
> > +};
>
> This fails to build:
>
> drivers/hwmon/pmbus/isl68137.c:139:3: error: ‘struct pmbus_driver_info’
> has no member named ‘groups’
>
> .groups = attribute_groups,
> ^~~~~~
> drivers/hwmon/pmbus/isl68137.c:139:12: error: initialization of ‘int
> (*)(struct i2c_client *, int, int)’ from incompatible pointer type
> ‘const struct attribute_group **’ [-Werror=incompatible-pointer-types]
>
>
>
> .groups = attribute_groups,
>
>
> ^~~~~~~~~~~~~~~~
> drivers/hwmon/pmbus/isl68137.c:139:12: note: (near initialization for
> ‘isl68137_info.read_byte_data’)
>
> cc1: some warnings being treated as errors
Disregard that, found my mistake :)
>
> >
> > -static int isl68137_remove(struct i2c_client *client)
> > +static int isl68137_probe(struct i2c_client *client,
> > + const struct i2c_device_id *id)
> > {
> > - device_remove_file(&client->dev,
> > - &sensor_dev_attr_avs1_enabled.dev_attr);
> > - device_remove_file(&client->dev,
> > - &sensor_dev_attr_avs0_enabled.dev_attr);
> > - return pmbus_do_remove(client);
> > + return pmbus_do_probe(client, id, &isl68137_info);
> > }
> >
> > static const struct i2c_device_id isl68137_id[] = {
> > @@ -192,7 +158,7 @@ static struct i2c_driver isl68137_driver = {
> > .name = "isl68137",
> > },
> > .probe = isl68137_probe,
> > - .remove = isl68137_remove,
> > + .remove = pmbus_do_remove,
> > .id_table = isl68137_id,
> > };
> >
> > --
> > 2.21.0.593.g511ec345e18-goog
> >
> >
>
More information about the openbmc
mailing list