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 10:43:45 AEST 2019
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
>
> -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