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:50:37 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>

Applied to dev-5.0.

Cheers,

Andrew

> ---
>  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,
> +};
>  
> -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