hwmon: (ucd9000) Add gpio and debugfs
v3 cover hwmon: (ucd9000) Add gpio and debugfs
Add GPIO chip for user space access of the device's IO.
Add debugfs for register dump of mfr_status and individual gpi faults. Add sysfs attribute to clear logged faults.
Christopher Bostic (2):
hwmon: (ucd9000) Add gpio chip interface and clear logged faults
hwmon: (ucd9000) Add debugfs for mfr_status
drivers/hwmon/pmbus/ucd9000.c | 280 ++++++++-
Signature · 1 line
1.8.2.2
PATCH v3 1/2 hwmon: (ucd9000) Add gpio chip interface and clear logged faults
Add a struct gpio_chip and define some methods so that this device's I/O can be accessed via /sys/class/gpio.
Add capability to clear logged faults via sysfs.
---
v3 - Correct bug in gpio_chip get method. Wasn't retrieving
gpio config information correctly.
- Remove old debugfs flag from previous pmbus core changes.
- Remove all sysfs files for mfr_status command.
- Add comments on direct i2c_smbus calls to clarify that no page set is required.
v2 - Remove clear_faults file - redundant since all other sysfs
core accesses result in an automatic clear fault.
- Removed local status_word and status_vout register dumps and use the new pmbus core status facilities instead.
- Rename gpi<x>_fault to gpi<x>_alarm to better match core naming conventions.
- Add full register dump for mfr_status.
- Add gpio chip to device structure and use gpio interfaces.
--- drivers/hwmon/pmbus/ucd9000.c | 136 +++++++++++++++++++++++++++++++++++++++++- 1 file changed, 134 insertions(+), 2 deletions(-)
drivers/hwmon/pmbus/ucd9000.c +134 -2
@@ -26,6 +26,9 @@#include <linux/slab.h> #include <linux/i2c.h> #include <linux/i2c/pmbus.h> +#include <linux/sysfs.h> +#include <linux/hwmon-sysfs.h> +#include <linux/gpio.h> #include "pmbus.h" enum chips { ucd9000, ucd90120, ucd90124, ucd90160, ucd9090, ucd90910 };
@@ -34,8 +37,16 @@#define UCD9000_NUM_PAGES 0xd6#define UCD9000_FAN_CONFIG_INDEX 0xe7#define UCD9000_FAN_CONFIG 0xe8 +#define UCD9000_LOGGED_FAULTS 0xea +#define UCD9000_GPIO_SELECT 0xfa +#define UCD9000_GPIO_CONFIG 0xfb#define UCD9000_DEVICE_ID 0xfd +/* GPIO CONFIG bits */ +#define UCD9000_GPIO_CONFIG_ENABLE BIT(0) +#define UCD9000_GPIO_CONFIG_OUT_ENABLE BIT(1) +#define UCD9000_GPIO_CONFIG_STATUS BIT(3) +#define UCD9000_MON_TYPE (x) (((x) >> 5) & 0x07)#define UCD9000_MON_PAGE (x) ((x) & 0x0f)
@@ -46,9 +57,12 @@#define UCD9000_NUM_FAN 4 +#define UCD90160_NUM_GPIOS 26 +struct ucd9000_data {u8 fan_data[UCD9000_NUM_FAN ][I2C_SMBUS_BLOCK_MAX ];struct pmbus_driver_info info; +struct gpio_chip gpio; };#define to_ucd9000_data (_info) container_of(_info, struct ucd9000_data, info)
@@ -119,6 +133,92 @@ static int ucd9000_read_byte_data(struct i2c_client *client, int page, int reg) };MODULE_DEVICE_TABLE (i2c, ucd9000_id); +static int ucd9000_gpio_read_config (struct i2c_client *client, +unsigned int offset) +{ +int ret; + +/* No page set required */ + ret =i2c_smbus_write_byte_data (client,UCD9000_GPIO_SELECT , offset); +if (ret <0 ) { +dev_err (&client->dev, +"Failed to read pin ID %d\n" , offset); +return ret; + } + +return i2c_smbus_read_byte_data (client,UCD9000_GPIO_CONFIG ); +} + +static int ucd9000_gpio_get (struct gpio_chip *gc,unsigned int offset) +{ +struct i2c_client *client =gpiochip_get_data (gc); +int ret; + + ret =ucd9000_gpio_read_config (client, offset); +if (ret <0 ) { +dev_err (&client->dev, +"failed to read GPIO config\n" ); +return ret; + } + +return ret &UCD9000_GPIO_CONFIG_STATUS ?1 :0 ; +} + +static int ucd9000_gpio_get_direction (struct gpio_chip *gc, +unsigned int offset) +{ +struct i2c_client *client =gpiochip_get_data (gc); +int ret; + + ret =ucd9000_gpio_read_config (client, offset); +if (ret <0 ) { +dev_err (&client->dev, +"failed to read GPIO config\n" ); +return ret; + } + +return ret &UCD9000_GPIO_CONFIG_OUT_ENABLE ?0 :1 ; +} + +/* +* Defined so that libgpiod tool can access pin states. No plan to actually +* change pin direction within this driver. +*/ +static int ucd9000_gpio_direction_input (struct gpio_chip *gc, +unsigned int offset) +{ +return 0 ; +} + +static ssize_t ucd9000_clear_logged_faults (struct device *dev, +struct device_attribute *attr,const char *buf, +size_t count) +{ +struct i2c_client *client =to_i2c_client (dev); +int ret; + +/* No page set required */ + ret =i2c_smbus_write_byte_data (client,UCD9000_LOGGED_FAULTS ,0 ); +if (ret) { +dev_err (&client->dev,"Failed to clear logged faults\n" ); +return ret; + } + +return count; +} + +static DEVICE_ATTR(clear_logged_faults, 0200, NULL, + ucd9000_clear_logged_faults); + +static struct attribute *ucd9000_attributes[] = { + &dev_attr_clear_logged_faults.attr, +NULL +}; + +static const struct attribute_group ucd9000_attr_group = { + .attrs = ucd9000_attributes, +}; +static int ucd9000_probe (struct i2c_client *client,const struct i2c_device_id *id) {
@@ -227,7 +327,39 @@ static int ucd9000_probe(struct i2c_client *client, | PMBUS_HAVE_FAN34 | PMBUS_HAVE_STATUS_FAN34; } -return pmbus_do_probe (client, mid, info); + data->gpio.label ="gpio-ucd9000" ; + data->gpio.get_direction = ucd9000_gpio_get_direction; + data->gpio.direction_input = ucd9000_gpio_direction_input; + data->gpio.get = ucd9000_gpio_get; + data->gpio.can_sleep =1 ; + data->gpio.base =-1 ; +if (mid->driver_data == ucd90160) + data->gpio.ngpio =UCD90160_NUM_GPIOS ; + data->gpio.parent = &client->dev; + data->gpio.owner =THIS_MODULE ; + + ret =devm_gpiochip_add_data (&client->dev, &data->gpio, client); +if (ret) { + data->gpio.parent =NULL ; +dev_warn (&client->dev,"Count not add gpiochip\n" ); +return ret; + } + +dev_info (&client->dev,"gpios %i...%i\n" , data->gpio.base, + data->gpio.base + data->gpio.ngpio -1 ); + + ret =sysfs_create_group (&client->dev.kobj, + &ucd9000_attr_group); +if (ret <0 ) +return ret; + +return pmbus_do_probe (client, mid, info); +} + +static int ucd9000_remove (struct i2c_client *client) +{ +sysfs_remove_group (&client->dev.kobj, &ucd9000_attr_group); +return pmbus_do_remove (client); }/* This is the driver that will be inserted */
@@ -236,7 +368,7 @@ static int ucd9000_probe(struct i2c_client *client, .name = "ucd9000", }, .probe = ucd9000_probe, - .remove = pmbus_do_remove, + .remove = ucd9000_remove, .id_table = ucd9000_id, };
Signature · 1 line
1.8.2.2
Re: [PATCH linux dev-4.10 v3 1/2] hwmon: (ucd9000) Add gpio chip interface and clear logged faults
Hi Chris,
drivers/hwmon/pmbus/ucd9000.c static int ucd9000_read_byte_data(struct i2c_client *client, int page, int reg) · line 216
Quoted 7 lines · Christopher Bostic
drivers/hwmon/pmbus/ucd9000.c +7
-0+static DEVICE_ATTR(clear_logged_faults, 0200, NULL, + ucd9000_clear_logged_faults); + +static struct attribute *ucd9000_attributes[] = { + &dev_attr_clear_logged_faults.attr, +NULL +};
Given this introduces new userspace ABI that will need to be support, we will want to run this past upstream as soon as possible.
I suggest you post the next revision of these patches to the hwmon list.
Cheers,
Joel
Re: [PATCH linux dev-4.10 v3 1/2] hwmon: (ucd9000) Add gpio chip interface and clear logged faults
Quoted 15 lines · Joel Stanley
Hi Chris,
drivers/hwmon/pmbus/ucd9000.c static int ucd9000_read_byte_data(struct i2c_client *client, int page, int reg) · line 216
Quoted 7 lines · Christopher Bostic
drivers/hwmon/pmbus/ucd9000.c +7
-0+static DEVICE_ATTR(clear_logged_faults, 0200, NULL, + ucd9000_clear_logged_faults); + +static struct attribute *ucd9000_attributes[] = { + &dev_attr_clear_logged_faults.attr, +NULL +};Given this introduces new userspace ABI that will need to be support, we will want to run this past upstream as soon as possible.
Quoted 1 line
I suggest you post the next revision of these patches to the hwmon list.
Hi Joel,
Sure, will do that.
Thanks,
Chris
Quoted 5 lines
Cheers,
Joel
Re: [PATCH linux dev-4.10 v3 1/2] hwmon: (ucd9000) Add gpio chip interface and clear logged faults
In reply to Christopher BosticHi Chris,
Quoted 58 lines · Christopher Bostic
Add a struct
gpio_chipand define some methods so that this device's I/O can be accessed via/sys/class/gpio.
Add capability to clear logged faults via sysfs.Quoted 1 line
Signed-off-by Christopher Bostic <[email protected]>---
v3 - Correct bug ingpio_chipget method. Wasn't retrieving
gpio config information correctly.
- Remove old debugfs flag from previous pmbus core changes.
- Remove all sysfs files formfr_statuscommand.
- Add comments on directi2c_smbuscalls to clarify that no page set is required.
v2 - Removeclear_faultsfile - redundant since all other sysfs core accesses result in an automatic clear fault.
- Removed localstatus_wordandstatus_voutregister dumps and use the new pmbus core status facilities instead.
- Rename gpi<x>_fault to gpi<x>_alarm to better match core naming conventions.
- Add full register dump formfr_status.
- Add gpio chip to device structure and use gpio interfaces.
---
drivers/hwmon/pmbus/ucd9000.c | 136 +++++++++++++++++++++++++++++++++++++++++-1 file changed, 134 insertions(+), 2 deletions(-)drivers/hwmon/pmbus/ucd9000.c +3
-0@@ -26,6 +26,9 @@ #include <linux/slab.h> #include <linux/i2c.h> #include <linux/i2c/pmbus.h> +#include <linux/sysfs.h> +#include <linux/hwmon-sysfs.h> +#include <linux/gpio.h> #include "pmbus.h"
enum chips { ucd9000, ucd90120, ucd90124, ucd90160, ucd9090, ucd90910 };drivers/hwmon/pmbus/ucd9000.c +0
-0@@ -34,8 +37,16 @@Quoted 7 lines
#define
UCD9000_NUM_PAGES0xd6
#defineUCD9000_FAN_CONFIG_INDEX0xe7
#defineUCD9000_FAN_CONFIG0xe8drivers/hwmon/pmbus/ucd9000.c +3
-0+#define UCD9000_LOGGED_FAULTS 0xea +#define UCD9000_GPIO_SELECT 0xfa +#define UCD9000_GPIO_CONFIG 0xfb#define
UCD9000_DEVICE_ID0xfd
+/* GPIO CONFIG bits */drivers/hwmon/pmbus/ucd9000.c line 48
Quoted 3 lines
drivers/hwmon/pmbus/ucd9000.c +3
-0+#define UCD9000_GPIO_CONFIG_ENABLE BIT(0) +#define UCD9000_GPIO_CONFIG_OUT_ENABLE BIT(1) +#define UCD9000_GPIO_CONFIG_STATUS BIT(3)+
Quoted 2 lines
#define
UCD9000_MON_TYPE(x)(((x) >> 5) &0x07)
#defineUCD9000_MON_PAGE(x)((x) &0x0f)drivers/hwmon/pmbus/ucd9000.c +0
-0@@ -46,9 +57,12 @@
Quoted 3 lines
Quoted 1 line
#define
UCD9000_NUM_FAN4Quoted 1 line
+#define
UCD90160_NUM_GPIOS26
This is mostly true, though there are only 22 physical GPIOs, plus 4 GPIs. We're additionally constrained on the UCD90160 by "Table 3. GPIO Pin Configuration Options" in "16-Rail Power Supply Sequencer and Monitor with ACPI Support datasheet (Rev. C)" (ucd90160.pdf, page 22) to 8/26 as GPIs and 16/22 as GPOs. It would probably pay to think about how these constraints would impact the implementation.
Quoted 13 lines
+
structucd9000_data{drivers/hwmon/pmbus/ucd9000.c line 65
Quoted 3 lines
u8 fan_data[UCD9000_NUM_FAN][I2C_SMBUS_BLOCK_MAX]; struct
pmbus_driver_infoinfo;drivers/hwmon/pmbus/ucd9000.c +1
-0+struct gpio_chip gpio;};
#defineto_ucd9000_data(_info)container_of(_info, struct ucd9000_data, info)drivers/hwmon/pmbus/ucd9000.c +0
-0@@ -119,6 +133,92 @@ static int ucd9000_read_byte_data(struct i2c_client *client, int page, int reg) }; MODULE_DEVICE_TABLE(i2c, ucd9000_id);
+static int ucd9000_gpio_read_config(structi2c_client*client,
Quoted 8 lines
Review
Quoted 1 line
(quoted diff) +1
-0+ unsigned int offset)+{
Review
Quoted 1 line
(quoted diff) +1
-0+ int ret;+
drivers/hwmon/pmbus/ucd9000.c static int ucd9000_read_byte_data(struct i2c_client *client, int page, int reg) · line 144
Quoted 4 lines
drivers/hwmon/pmbus/ucd9000.c +4
-0+/* No page set required */ + ret =i2c_smbus_write_byte_data (client,UCD9000_GPIO_SELECT , offset); +if (ret <0 ) { + dev_err(&client->dev,
Why is there a linebreak here?
Quoted 1 line
drivers/hwmon/pmbus/ucd9000.c static int ucd9000_read_byte_data(struct i2c_client *client, int page, int reg) · line 145
Quoted 1 line
drivers/hwmon/pmbus/ucd9000.c +1
-0+ "Failed to read pin ID %d\n", offset);
The error message is confusing - it relates to the semantics of ucd9000_gpio_read_config() and not the operation on which the error occurred.
I feel strongly that we should also report the error code as well: It provides additional context on what went wrong.
As a consequence I would suggest something like:
dev_err(&client->dev, "Failed to select GPIO %d: %d\n", offset, ret);
Quoted 11 lines
Review
Quoted 2 lines
(quoted diff) +2
-0+ return ret; + }+
Quoted 1 line
drivers/hwmon/pmbus/ucd9000.c +1
-0+return i2c_smbus_read_byte_data (client,UCD9000_GPIO_CONFIG );drivers/hwmon/pmbus/ucd9000.c +4
-0+} + +static int ucd9000_gpio_get (struct gpio_chip *gc,unsigned int offset) +{Review
Quoted 2 lines
(quoted diff) +2
-0+ struct i2c_client *client = gpiochip_get_data(gc); + int ret;+
Quoted 4 lines
drivers/hwmon/pmbus/ucd9000.c static int ucd9000_read_byte_data(struct i2c_client *client, int page, int reg) · line 160
Quoted 4 lines
drivers/hwmon/pmbus/ucd9000.c +4
-0+ ret =ucd9000_gpio_read_config (client, offset); +if (ret <0 ) { +dev_err (&client->dev, +"failed to read GPIO config\n" );
Again, we don't need a linebreak above, but we probably should have one between the dev_err() and the return statement.
Quoted 4 lines
Review
Quoted 2 lines
(quoted diff) +2
-0+ return ret; + }+
drivers/hwmon/pmbus/ucd9000.c static int ucd9000_read_byte_data(struct i2c_client *client, int page, int reg) · line 164
Quoted 1 line
drivers/hwmon/pmbus/ucd9000.c +1
-0+return ret &UCD9000_GPIO_CONFIG_STATUS ?1 :0 ;
We can avoid the branch with:
return !!(ret & UCD9000_GPIO_CONFIG_STATUS);
Interestingly it's not clear from the datasheet whether the status bit contains a relevant value when the pin is in output mode, as there's a separate bit (bit
2) to set the value. Have you tested the behaviour?
Quoted 16 lines
drivers/hwmon/pmbus/ucd9000.c +3
-0+} + +static int ucd9000_gpio_get_direction(struct gpio_chip *gc,Review
Quoted 1 line
(quoted diff) +1
-0+ unsigned int offset)+{
Review
Quoted 2 lines
(quoted diff) +2
-0+ struct i2c_client *client = gpiochip_get_data(gc); + int ret;+
drivers/hwmon/pmbus/ucd9000.c static int ucd9000_read_byte_data(struct i2c_client *client, int page, int reg) · line 162
Quoted 6 lines
drivers/hwmon/pmbus/ucd9000.c +6
-0+ ret =ucd9000_gpio_read_config (client, offset); +if (ret <0 ) { +dev_err (&client->dev, +"failed to read GPIO config\n" ); +return ret; + }+
Quoted 1 line
drivers/hwmon/pmbus/ucd9000.c +1
-0+return ret &UCD9000_GPIO_CONFIG_OUT_ENABLE ?0 :1 ;
drivers/hwmon/pmbus/ucd9000.c static int ucd9000_read_byte_data(struct i2c_client *client, int page, int reg) · line 185
Quoted 5 lines
drivers/hwmon/pmbus/ucd9000.c +5
-0+} + +/* +* Defined so that libgpiod tool can access pin states. No plan to actually +* change pin direction within this driver.
That's not true - the driver is general purpose. Lets not limit it too much to *how you intend to use it* vs the actual hardware capability.
Quoted 5 lines
+ */
+static int ucd9000_gpio_direction_input(structgpio_chip*gc,Review
Quoted 1 line
(quoted diff) +1
-0+ unsigned int offset)+{
drivers/hwmon/pmbus/ucd9000.c static int ucd9000_read_byte_data(struct i2c_client *client, int page, int reg) · line 190
Quoted 1 line
drivers/hwmon/pmbus/ucd9000.c +1
-0+return 0 ;
I don't think we should do this for several reasons:
1) It's not that hard to implement the functionality
2) It's not true that all GPIOs are inputs with factory defaults
3) There will be uncertainty if custom defaults are programmed
Again from ucd90160.pdf, on page 34:
7.5 Programming
7.5.1 Device Configuration and Programming
From the factory, the device contains the sequencing and monitoring firmware. It is also configured so that all GPOs are high-impedance (except for FPWM/GPIO pins 17-24, which are driven low), with no sequencing or fault-response operation. See Configuration Programming of UCD Devices, available from the Documentation & Help Center that can be selected from the Fusion GUI Help menu, for full UCD90160 configuration details.
The FPWM pins are driven low, implying they are configured for output mode. It's not obvious to me from this whether the other GPIOs are input or output out-of-the-box as the chip supports active and open-drain drive types. It does say "GPOs" in the paragraph above, but I haven't found documentation on which pins are in what state from factory, and I don't know how we're configuring it for production.
Regardless, the driver shouldn't make assumptions about how the device is pre-programmed - without further information there's already ambiguity as to whether the default settings are in play or whether the system integrator has configured the chip. We need information from the platform about how the device is/should be managed.
Further, the note in section 10.43 (FBh) GPIO_CONFIG (MFR_SPECIFIC_43) says:
Configuring a pin that is also being used by another function (enable, fan control, and so forth) may likely result in unexpected and unwanted behavior.
This suggests my favourite topic: pinctrl.
Twenty-two of the pins on the UCD90160 are GPIO-capable. Of these 22 pins, 16 of them have alternate functions. See "Table 3. GPIO Pin Configuration Options" on page 22 of ucd90160.pdf. But a quick rundown:
1. 4 PWM pins (GPIOs 1-4, pins 31, 32, 42, 41)
2. 8 FPWM (Fast PWM) pins (GPIOs 5-12, pins 17-24)
3. 4 pins shared with JTAG (GPIOs 19-22, pins 36-39)
Now, in terms of devicetree we'd be breaking things in the future by specifying a GPIO controller that relies on a pinmux in hardware without also specifying the pin controller in the devicetree. This necessitates at least *some* pinctrl capability be baked into the driver as well, so we can point the gpiochip to the relevant pin controller (the same device, but the bindings are general) and map the pin spaces in the devicetree (the gpio and pin spaces are different).
Quoted 29 lines
drivers/hwmon/pmbus/ucd9000.c +3
-0+} + +static ssize_t ucd9000_clear_logged_faults(struct device *dev,drivers/hwmon/pmbus/ucd9000.c static int ucd9000_read_byte_data(struct i2c_client *client, int page, int reg) · line 195
Quoted 2 lines
drivers/hwmon/pmbus/ucd9000.c +2
-0+ struct device_attribute *attr, const char *buf, + size_t count)+{
drivers/hwmon/pmbus/ucd9000.c static int ucd9000_read_byte_data(struct i2c_client *client, int page, int reg) · line 198
Quoted 2 lines
drivers/hwmon/pmbus/ucd9000.c +2
-0+struct i2c_client *client =to_i2c_client (dev); +int ret;+
drivers/hwmon/pmbus/ucd9000.c static int ucd9000_read_byte_data(struct i2c_client *client, int page, int reg) · line 205
Quoted 6 lines
drivers/hwmon/pmbus/ucd9000.c +6
-0+/* No page set required */ + ret =i2c_smbus_write_byte_data (client,UCD9000_LOGGED_FAULTS ,0 ); +if (ret) { +dev_err (&client->dev,"Failed to clear logged faults\n" ); +return ret; + }+
Quoted 1 line
drivers/hwmon/pmbus/ucd9000.c +1
-0+return count;drivers/hwmon/pmbus/ucd9000.c +3
-0+} + +static DEVICE_ATTR(clear_logged_faults, 0200, NULL,Quoted 1 line
drivers/hwmon/pmbus/ucd9000.c +1
-0+ ucd9000_clear_logged_faults);drivers/hwmon/pmbus/ucd9000.c +2
-0+ +static struct attribute *ucd9000_attributes[] = {Quoted 2 lines
drivers/hwmon/pmbus/ucd9000.c +2
-0+ &dev_attr_clear_logged_faults.attr, +NULL drivers/hwmon/pmbus/ucd9000.c +3
-0+}; + +static const struct attribute_group ucd9000_attr_group = {Quoted 1 line
drivers/hwmon/pmbus/ucd9000.c +1
-0+ .attrs = ucd9000_attributes,
drivers/hwmon/pmbus/ucd9000.c
Quoted 2 lines
drivers/hwmon/pmbus/ucd9000.c +2
-0+}; +
Joel's made a fair comment here regarding upstream. Please address that.
Quoted 7 lines
static int ucd9000_probe(struct
i2c_client*client,Quoted 1 line
const struct
i2c_device_id*id){
drivers/hwmon/pmbus/ucd9000.c +0
-0@@ -227,7 +327,39 @@ static int ucd9000_probe(struct i2c_client *client, > | PMBUS_HAVE_FAN34 | PMBUS_HAVE_STATUS_FAN34; > }
Quoted 5 lines
drivers/hwmon/pmbus/ucd9000.c static int ucd9000_probe(struct i2c_client *client, · line 333
Quoted 5 lines
drivers/hwmon/pmbus/ucd9000.c +4
-1-return pmbus_do_probe (client, mid, info); + data->gpio.label ="gpio-ucd9000" ; + data->gpio.get_direction = ucd9000_gpio_get_direction; + data->gpio.direction_input = ucd9000_gpio_direction_input; + data->gpio.get = ucd9000_gpio_get;
I feel like it wouldn't be much effort to make this more complete and specify the direction_output() and set() callbacks as well.
Quoted 4 lines
drivers/hwmon/pmbus/ucd9000.c static int ucd9000_probe(struct i2c_client *client, · line 337
Quoted 4 lines
drivers/hwmon/pmbus/ucd9000.c +4
-0+ data->gpio.can_sleep =1 ; + data->gpio.base =-1 ; +if (mid->driver_data == ucd90160) + data->gpio.ngpio =UCD90160_NUM_GPIOS ;
This might need some rework in the future as the other ucd9000 variants want GPIO support.
In fact, what's the behaviour of the other drivers now given we're unconditionally registering a gpiochip?
Quoted 2 lines
drivers/hwmon/pmbus/ucd9000.c static int ucd9000_probe(struct i2c_client *client, · line 339
Quoted 2 lines
drivers/hwmon/pmbus/ucd9000.c +2
-0+ data->gpio.parent = &client->dev; + data->gpio.owner =THIS_MODULE ;
At some point we should think about pinconf as well (push-pull vs open-drain).
Quoted 10 lines
+
drivers/hwmon/pmbus/ucd9000.c static int ucd9000_probe(struct i2c_client *client, · line 346
Quoted 6 lines
drivers/hwmon/pmbus/ucd9000.c +6
-0+ ret =devm_gpiochip_add_data (&client->dev, &data->gpio, client); +if (ret) { + data->gpio.parent =NULL ; +dev_warn (&client->dev,"Count not add gpiochip\n" ); +return ret; + }+
drivers/hwmon/pmbus/ucd9000.c static int ucd9000_probe(struct i2c_client *client, · line 349
Quoted 2 lines
drivers/hwmon/pmbus/ucd9000.c +2
-0+dev_info (&client->dev,"gpios %i...%i\n" , data->gpio.base, + data->gpio.base + data->gpio.ngpio -1 );
Let's drop this.
Cheers,
Andrew
Quoted 25 lines
+
drivers/hwmon/pmbus/ucd9000.c static int ucd9000_probe(struct i2c_client *client, · line 354
Quoted 4 lines
drivers/hwmon/pmbus/ucd9000.c +4
-0+ ret =sysfs_create_group (&client->dev.kobj, + &ucd9000_attr_group); +if (ret <0 ) +return ret;+
Quoted 1 line
drivers/hwmon/pmbus/ucd9000.c +1
-0+ return pmbus_do_probe(client, mid, info);drivers/hwmon/pmbus/ucd9000.c +4
-0+} + +static int ucd9000_remove (struct i2c_client *client) +{drivers/hwmon/pmbus/ucd9000.c static int ucd9000_probe(struct i2c_client *client, · line 362
Quoted 2 lines
drivers/hwmon/pmbus/ucd9000.c +2
-0+sysfs_remove_group (&client->dev.kobj, &ucd9000_attr_group); +return pmbus_do_remove (client);}
/* This is the driver that will be inserted */drivers/hwmon/pmbus/ucd9000.c +0
-0@@ -236,7 +368,7 @@ static int ucd9000_probe(struct i2c_client *client, > .name = "ucd9000", > }, > .probe = ucd9000_probe, > - .remove = pmbus_do_remove, > + .remove = ucd9000_remove, > .id_table = ucd9000_id, };
Re: [PATCH linux dev-4.10 v3 1/2] hwmon: (ucd9000) Add gpio chip interface and clear logged faults
Hi Andrew,
At this point, we just need a working driver so we can deliver the function. This is all I need:
* read status_word
* read mfr_status (though it isn't that useful because the pmbus core code that Chris had to start using sends a clear faults every second and clears out the edge triggered bits I'm interested in)
* read status_vout from each page
* read the realtime GPI status using regs 0xFA and 0xFB for the 6 inputs that are wired on this system
Chris's current code does all that, as did several of his previous versions, so I think we need the path of least resistance and just go with one of them.
Quoted 322 lines · Andrew Jeffery
Hi Chris,
Quoted 60 lines · Christopher Bostic
Add a struct
gpio_chipand define some methods so that this device's I/O can be accessed via/sys/class/gpio.Add capability to clear logged faults via sysfs.
Quoted 1 line
Signed-off-by Christopher Bostic <[email protected]>---
v3 - Correct bug ingpio_chipget method. Wasn't retrieving
gpio config information correctly.
- Remove old debugfs flag from previous pmbus core changes.
- Remove all sysfs files formfr_statuscommand.
- Add comments on directi2c_smbuscalls to clarify that no page set is required.
v2 - Removeclear_faultsfile - redundant since all other sysfs
core accesses result in an automatic clear fault.
- Removed localstatus_wordandstatus_voutregister dumps and use the new pmbus core status facilities instead.
- Rename gpi<x>_fault to gpi<x>_alarm to better match core naming conventions.
- Add full register dump formfr_status.
- Add gpio chip to device structure and use gpio interfaces.--- drivers/hwmon/pmbus/ucd9000.c | 136 +++++++++++++++++++++++++++++++++++++++++- 1 file changed, 134 insertions(+), 2 deletions(-)drivers/hwmon/pmbus/ucd9000.c +3
-0@@ -26,6 +26,9 @@#include <linux/slab.h> #include <linux/i2c.h> #include <linux/i2c/pmbus.h> +#include <linux/sysfs.h> +#include <linux/hwmon-sysfs.h> +#include <linux/gpio.h> #include "pmbus.h" enum chips { ucd9000, ucd90120, ucd90124, ucd90160, ucd9090, ucd90910 };@@ -34,8 +37,16 @@drivers/hwmon/pmbus/ucd9000.c line 43
Quoted 7 lines
#define UCD9000_NUM_PAGES 0xd6 #define UCD9000_FAN_CONFIG_INDEX 0xe7 #define UCD9000_FAN_CONFIG 0xe8drivers/hwmon/pmbus/ucd9000.c +3
-0+#define UCD9000_LOGGED_FAULTS 0xea +#define UCD9000_GPIO_SELECT 0xfa +#define UCD9000_GPIO_CONFIG 0xfb#define UCD9000_DEVICE_ID 0xfd+/* GPIO CONFIG bits */
drivers/hwmon/pmbus/ucd9000.c line 48
Quoted 3 lines
drivers/hwmon/pmbus/ucd9000.c +3
-0+#define UCD9000_GPIO_CONFIG_ENABLE BIT(0) +#define UCD9000_GPIO_CONFIG_OUT_ENABLE BIT(1) +#define UCD9000_GPIO_CONFIG_STATUS BIT(3)+
Quoted 2 lines
#define UCD9000_MON_TYPE(x) (((x) >> 5) & 0x07) #define UCD9000_MON_PAGE(x) ((x) & 0x0f)drivers/hwmon/pmbus/ucd9000.c +0
-0@@ -46,9 +57,12 @@Quoted 1 line
#define UCD9000_NUM_FAN 4Quoted 1 line
Quoted 1 line
+#define
UCD90160_NUM_GPIOS26This is mostly true, though there are only 22 physical GPIOs, plus 4 GPIs. We're additionally constrained on the UCD90160 by "Table 3. GPIO Pin Configuration Options" in "16-Rail Power Supply Sequencer and Monitor with ACPI Support datasheet (Rev. C)" (ucd90160.pdf, page 22) to 8/26 as GPIs and 16/22 as GPOs. It would probably pay to think about how these constraints would impact the implementation.
Quoted 17 lines
+
structucd9000_data{drivers/hwmon/pmbus/ucd9000.c line 65
Quoted 3 lines
u8 fan_data[UCD9000_NUM_FAN][I2C_SMBUS_BLOCK_MAX]; struct pmbus_driver_info info;drivers/hwmon/pmbus/ucd9000.c +1
-0+struct gpio_chip gpio;};
#defineto_ucd9000_data(_info)container_of(_info, struct ucd9000_data, info)drivers/hwmon/pmbus/ucd9000.c +3
-0@@ -119,6 +133,92 @@ static int ucd9000_read_byte_data(struct i2c_client *client, int page, int reg) };MODULE_DEVICE_TABLE (i2c, ucd9000_id); +static int ucd9000_gpio_read_config (struct i2c_client *client, > + unsigned int offset) +{ > + int ret; +Quoted 4 lines
drivers/hwmon/pmbus/ucd9000.c static int ucd9000_read_byte_data(struct i2c_client *client, int page, int reg) · line 144
Quoted 4 lines
drivers/hwmon/pmbus/ucd9000.c +4
-0+/* No page set required */ + ret =i2c_smbus_write_byte_data (client,UCD9000_GPIO_SELECT , offset); +if (ret <0 ) { + dev_err(&client->dev,Why is there a linebreak here?
Quoted 1 line
drivers/hwmon/pmbus/ucd9000.c static int ucd9000_read_byte_data(struct i2c_client *client, int page, int reg) · line 145
Quoted 1 line
drivers/hwmon/pmbus/ucd9000.c +1
-0+ "Failed to read pin ID %d\n", offset);The error message is confusing - it relates to the semantics of
ucd9000_gpio_read_config()and not the operation on which the error occurred.I feel strongly that we should also report the error code as well: It provides additional context on what went wrong.
As a consequence I would suggest something like:
dev_err(&client->dev, "Failed to select GPIO %d: %d\n", offset, ret);Quoted 11 lines
Review
Quoted 2 lines
(quoted diff) +2
-0+ return ret; + }+
Quoted 1 line
drivers/hwmon/pmbus/ucd9000.c +1
-0+return i2c_smbus_read_byte_data (client,UCD9000_GPIO_CONFIG );drivers/hwmon/pmbus/ucd9000.c +4
-0+} + +static int ucd9000_gpio_get (struct gpio_chip *gc,unsigned int offset) +{Review
Quoted 2 lines
(quoted diff) +2
-0+ struct i2c_client *client = gpiochip_get_data(gc); + int ret;+
Quoted 4 lines
drivers/hwmon/pmbus/ucd9000.c static int ucd9000_read_byte_data(struct i2c_client *client, int page, int reg) · line 160
Quoted 4 lines
drivers/hwmon/pmbus/ucd9000.c +4
-0+ ret =ucd9000_gpio_read_config (client, offset); +if (ret <0 ) { +dev_err (&client->dev, +"failed to read GPIO config\n" );Again, we don't need a linebreak above, but we probably should have one between the
dev_err()and the return statement.Quoted 4 lines
Review
Quoted 2 lines
(quoted diff) +2
-0+ return ret; + }+
drivers/hwmon/pmbus/ucd9000.c static int ucd9000_read_byte_data(struct i2c_client *client, int page, int reg) · line 164
Quoted 1 line
drivers/hwmon/pmbus/ucd9000.c +1
-0+return ret &UCD9000_GPIO_CONFIG_STATUS ?1 :0 ;We can avoid the branch with:
return !!(ret & UCD9000_GPIO_CONFIG_STATUS);Interestingly it's not clear from the datasheet whether the status bit contains a relevant value when the pin is in output mode, as there's a separate bit (bit
2) to set the value. Have you tested the behaviour?Quoted 16 lines
drivers/hwmon/pmbus/ucd9000.c +3
-0+} + +static int ucd9000_gpio_get_direction(struct gpio_chip *gc,Review
Quoted 1 line
(quoted diff) +1
-0+ unsigned int offset)+{
Review
Quoted 2 lines
(quoted diff) +2
-0+ struct i2c_client *client = gpiochip_get_data(gc); + int ret;+
drivers/hwmon/pmbus/ucd9000.c static int ucd9000_read_byte_data(struct i2c_client *client, int page, int reg) · line 162
Quoted 6 lines
drivers/hwmon/pmbus/ucd9000.c +6
-0+ ret =ucd9000_gpio_read_config (client, offset); +if (ret <0 ) { +dev_err (&client->dev, +"failed to read GPIO config\n" ); +return ret; + }+
Quoted 1 line
drivers/hwmon/pmbus/ucd9000.c +1
-0+return ret &UCD9000_GPIO_CONFIG_OUT_ENABLE ?0 :1 ;drivers/hwmon/pmbus/ucd9000.c static int ucd9000_read_byte_data(struct i2c_client *client, int page, int reg) · line 185
Quoted 5 lines
drivers/hwmon/pmbus/ucd9000.c +5
-0+} + +/* +* Defined so that libgpiod tool can access pin states. No plan to actually +* change pin direction within this driver. That's not true - the driver is general purpose. Lets not limit it too much to *how you intend to use it* vs the actual hardware capability.
Quoted 5 lines
+ */
+static int ucd9000_gpio_direction_input(structgpio_chip*gc,Review
Quoted 1 line
(quoted diff) +1
-0+ unsigned int offset)+{
drivers/hwmon/pmbus/ucd9000.c static int ucd9000_read_byte_data(struct i2c_client *client, int page, int reg) · line 190
Quoted 1 line
drivers/hwmon/pmbus/ucd9000.c +1
-0+return 0 ;I don't think we should do this for several reasons:
1) It's not that hard to implement the functionality
2) It's not true that all GPIOs are inputs with factory defaults
3) There will be uncertainty if custom defaults are programmedAgain from ucd90160.pdf, on page 34:
7.5 Programming
7.5.1 Device Configuration and Programming
From the factory, the device contains the sequencing and monitoring firmware. It is also configured so that all GPOs are high-impedance (except for FPWM/GPIO pins 17-24, which are driven low), with no sequencing or fault-response operation. See Configuration Programming of UCD Devices, available from the Documentation & Help Center that can be selected from the Fusion GUI Help menu, for full UCD90160 configuration details.The FPWM pins are driven low, implying they are configured for output mode. It's not obvious to me from this whether the other GPIOs are input or output out-of-the-box as the chip supports active and open-drain drive types. It does say "GPOs" in the paragraph above, but I haven't found documentation on which pins are in what state from factory, and I don't know how we're configuring it for production.
Regardless, the driver shouldn't make assumptions about how the device is pre-programmed - without further information there's already ambiguity as to whether the default settings are in play or whether the system integrator has configured the chip. We need information from the platform about how the device is/should be managed.
Further, the note in section 10.43 (FBh)
GPIO_CONFIG(MFR_SPECIFIC_43) says:Configuring a pin that is also being used by another function (enable, fan control, and so forth) may likely result in unexpected and unwanted behavior.
This suggests my favourite topic: pinctrl.
Twenty-two of the pins on the UCD90160 are GPIO-capable. Of these 22 pins, 16 of them have alternate functions. See "Table 3. GPIO Pin Configuration Options" on page 22 of ucd90160.pdf. But a quick rundown:
1. 4 PWM pins (GPIOs 1-4, pins 31, 32, 42, 41)
2. 8 FPWM (Fast PWM) pins (GPIOs 5-12, pins 17-24)
3. 4 pins shared with JTAG (GPIOs 19-22, pins 36-39)Now, in terms of devicetree we'd be breaking things in the future by specifying a GPIO controller that relies on a pinmux in hardware without also specifying the pin controller in the devicetree. This necessitates at least *some* pinctrl capability be baked into the driver as well, so we can point the gpiochip to the relevant pin controller (the same device, but the bindings are general) and map the pin spaces in the devicetree (the gpio and pin spaces are different).
Quoted 29 lines
drivers/hwmon/pmbus/ucd9000.c +3
-0+} + +static ssize_t ucd9000_clear_logged_faults(struct device *dev,drivers/hwmon/pmbus/ucd9000.c static int ucd9000_read_byte_data(struct i2c_client *client, int page, int reg) · line 195
Quoted 2 lines
drivers/hwmon/pmbus/ucd9000.c +2
-0+ struct device_attribute *attr, const char *buf, + size_t count)+{
drivers/hwmon/pmbus/ucd9000.c static int ucd9000_read_byte_data(struct i2c_client *client, int page, int reg) · line 198
Quoted 2 lines
drivers/hwmon/pmbus/ucd9000.c +2
-0+struct i2c_client *client =to_i2c_client (dev); +int ret;+
drivers/hwmon/pmbus/ucd9000.c static int ucd9000_read_byte_data(struct i2c_client *client, int page, int reg) · line 205
Quoted 6 lines
drivers/hwmon/pmbus/ucd9000.c +6
-0+/* No page set required */ + ret =i2c_smbus_write_byte_data (client,UCD9000_LOGGED_FAULTS ,0 ); +if (ret) { +dev_err (&client->dev,"Failed to clear logged faults\n" ); +return ret; + }+
Quoted 1 line
drivers/hwmon/pmbus/ucd9000.c +1
-0+return count;drivers/hwmon/pmbus/ucd9000.c +3
-0+} + +static DEVICE_ATTR(clear_logged_faults, 0200, NULL,Quoted 1 line
drivers/hwmon/pmbus/ucd9000.c +1
-0+ ucd9000_clear_logged_faults);drivers/hwmon/pmbus/ucd9000.c +2
-0+ +static struct attribute *ucd9000_attributes[] = {Quoted 2 lines
drivers/hwmon/pmbus/ucd9000.c +2
-0+ &dev_attr_clear_logged_faults.attr, +NULL drivers/hwmon/pmbus/ucd9000.c +3
-0+}; + +static const struct attribute_group ucd9000_attr_group = {Quoted 1 line
drivers/hwmon/pmbus/ucd9000.c +1
-0+ .attrs = ucd9000_attributes,drivers/hwmon/pmbus/ucd9000.c
Quoted 2 lines
drivers/hwmon/pmbus/ucd9000.c +2
-0+}; +Joel's made a fair comment here regarding upstream. Please address that.
Quoted 11 lines
static int ucd9000_probe(struct
i2c_client*client,Quoted 1 line
const struct
i2c_device_id*id){
drivers/hwmon/pmbus/ucd9000.c +0
-0@@ -227,7 +327,39 @@ static int ucd9000_probe(struct i2c_client *client, > | PMBUS_HAVE_FAN34 | PMBUS_HAVE_STATUS_FAN34; > } > - return pmbus_do_probe(client, mid, info); > + data->gpio.label = "gpio-ucd9000"; > + data->gpio.get_direction = ucd9000_gpio_get_direction; > + data->gpio.direction_input = ucd9000_gpio_direction_input;Quoted 1 line
drivers/hwmon/pmbus/ucd9000.c static int ucd9000_probe(struct i2c_client *client, · line 333
Quoted 1 line
drivers/hwmon/pmbus/ucd9000.c +1
-0+ data->gpio.get = ucd9000_gpio_get;I feel like it wouldn't be much effort to make this more complete and specify the
direction_output()andset()callbacks as well.Quoted 4 lines
drivers/hwmon/pmbus/ucd9000.c static int ucd9000_probe(struct i2c_client *client, · line 337
Quoted 4 lines
drivers/hwmon/pmbus/ucd9000.c +4
-0+ data->gpio.can_sleep =1 ; + data->gpio.base =-1 ; +if (mid->driver_data == ucd90160) + data->gpio.ngpio =UCD90160_NUM_GPIOS ;This might need some rework in the future as the other ucd9000 variants want GPIO support.
In fact, what's the behaviour of the other drivers now given we're unconditionally registering a gpiochip?
Quoted 2 lines
drivers/hwmon/pmbus/ucd9000.c static int ucd9000_probe(struct i2c_client *client, · line 339
Quoted 2 lines
drivers/hwmon/pmbus/ucd9000.c +2
-0+ data->gpio.parent = &client->dev; + data->gpio.owner =THIS_MODULE ;At some point we should think about pinconf as well (push-pull vs open-drain).
Quoted 10 lines
+
drivers/hwmon/pmbus/ucd9000.c static int ucd9000_probe(struct i2c_client *client, · line 346
Quoted 6 lines
drivers/hwmon/pmbus/ucd9000.c +6
-0+ ret =devm_gpiochip_add_data (&client->dev, &data->gpio, client); +if (ret) { + data->gpio.parent =NULL ; +dev_warn (&client->dev,"Count not add gpiochip\n" ); +return ret; + }+
drivers/hwmon/pmbus/ucd9000.c static int ucd9000_probe(struct i2c_client *client, · line 349
Quoted 2 lines
drivers/hwmon/pmbus/ucd9000.c +2
-0+dev_info (&client->dev,"gpios %i...%i\n" , data->gpio.base, + data->gpio.base + data->gpio.ngpio -1 );Let's drop this.
Cheers,
Andrew
Quoted 25 lines
+
drivers/hwmon/pmbus/ucd9000.c static int ucd9000_probe(struct i2c_client *client, · line 354
Quoted 4 lines
drivers/hwmon/pmbus/ucd9000.c +4
-0+ ret =sysfs_create_group (&client->dev.kobj, + &ucd9000_attr_group); +if (ret <0 ) +return ret;+
Quoted 1 line
drivers/hwmon/pmbus/ucd9000.c +1
-0+return pmbus_do_probe (client, mid, info);drivers/hwmon/pmbus/ucd9000.c +4
-0+} + +static int ucd9000_remove (struct i2c_client *client) +{drivers/hwmon/pmbus/ucd9000.c static int ucd9000_probe(struct i2c_client *client, · line 362
Quoted 2 lines
drivers/hwmon/pmbus/ucd9000.c +2
-0+sysfs_remove_group (&client->dev.kobj, &ucd9000_attr_group); +return pmbus_do_remove (client);}
/* This is the driver that will be inserted */
drivers/hwmon/pmbus/ucd9000.c +0
-0@@ -236,7 +368,7 @@ static int ucd9000_probe(struct i2c_client *client, > .name = "ucd9000", > }, > .probe = ucd9000_probe, > - .remove = pmbus_do_remove, > + .remove = ucd9000_remove, > .id_table = ucd9000_id, };
Re: [PATCH linux dev-4.10 v3 1/2] hwmon: (ucd9000) Add gpio chip interface and clear logged faults
Hi Matt,
Thanks for clarifying your requirements. I have some comments inline below.
Quoted 10 lines · Matt Spinler
Hi Andrew,
At this point, we just need a working driver so we can deliver the function. This is all I need:
* readstatus_word
* readmfr_status(though it isn't that useful because the pmbus core code that Chris had to start using sends a clear faults every second and clears out the edge triggered bits I'm interested in)
That's a valid complaint. At the moment pmbus_clear_faults() is driven by calls to pmbus_update_device() if the sensor value needs to be updated (values are cached for a time-period by the core).
There's no timer that clears the faults, it's purely the accesses from userspace. So if the accesses are ordered such that MFR_STATUS is read before any sensors then you should not miss any edge-triggered faults.
However that's not to say that this behaviour is sane. Some work should be done to figure out if we can avoid sending a CLEAR_FAULTS on every update.
Quoted 9 lines
* readstatus_voutfrom each page
* read the realtime GPI status using regs0xFAand0xFBfor the 6 inputs that are wired on this system
Chris's current code does all that, as did several of his previous versions, so I think we need the path of least resistance and just go with one of them.
I disagree on several fronts:
1. We've seen how frustrating userspace-breaking changes are with FSI and it follows (in my mind) that we'd want to minimise such problems going forward. This driver has evolved from defining many new, non-standard ABIs in v1 and v2 down to one in v3, which we're looking to get a blessing on from upstream. I would be *very* surprised if v1 or v2 were accepted with no complaints by upstream, and thus v3 is a big improvement over what we had (through minimising new ABIs).
2. The implementation of v2 would have seriously disrupted the behaviour of the device or given incorrect values for queries of the GPIO state.
3. v3 makes many assumptions about how the device is programmed but documents none of them. In light of that I can only review for the general case of needing to put the device in the state required to read off the values you need. Prior to your email the exact requirements on the implementation were not clear to me either. The flexibility and complexity of the UCD90160 means this is non-trivial, so either we implement the required functionality to achieve generality OR we heavily document the assumptions about how the device is programmed so it's *obvious* to the reader why the implementation is hamstrung the way it is. The latter approach makes it unsuitable for sending upstream, but at least it documents the path that should be followed before doing so.
Cheers,
Andrew
Quoted 370 lines
Quoted 327 lines · Andrew Jeffery
Hi Chris,
Quoted 62 lines · Christopher Bostic
Add a struct
gpio_chipand define some methods so that this device's I/O can be accessed via/sys/class/gpio.
Add capability to clear logged faults via sysfs.Quoted 1 line
Signed-off-by Christopher Bostic <[email protected]>
---
v3 - Correct bug ingpio_chipget method. Wasn't retrieving
gpio config information correctly.
- Remove old debugfs flag from previous pmbus core changes.
- Remove all sysfs files formfr_statuscommand.
- Add comments on directi2c_smbuscalls to clarify that no page set is required.
v2 - Removeclear_faultsfile - redundant since all other sysfs core accesses result in an automatic clear fault.
- Removed localstatus_wordandstatus_voutregister dumps and use the new pmbus core status facilities instead.
- Rename gpi<x>_fault to gpi<x>_alarm to better match core naming conventions.
- Add full register dump formfr_status.
- Add gpio chip to device structure and use gpio interfaces.
---
drivers/hwmon/pmbus/ucd9000.c| 136 +++++++++++++++++++++++++++++++++++++++++-1 file changed, 134 insertions(+), 2 deletions(-)drivers/hwmon/pmbus/ucd9000.c +3
-0@@ -26,6 +26,9 @@ #include <linux/slab.h> #include <linux/i2c.h> #include <linux/i2c/pmbus.h> +#include <linux/sysfs.h> +#include <linux/hwmon-sysfs.h> +#include <linux/gpio.h> #include "pmbus.h"
enum chips { ucd9000, ucd90120, ucd90124, ucd90160, ucd9090, ucd90910 };drivers/hwmon/pmbus/ucd9000.c +0
-0@@ -34,8 +37,16 @@Quoted 7 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
drivers/hwmon/pmbus/ucd9000.c line 42
Quoted 6 lines
#define
UCD9000_NUM_PAGES0xd6
#defineUCD9000_FAN_CONFIG_INDEX0xe7
#defineUCD9000_FAN_CONFIG0xe8drivers/hwmon/pmbus/ucd9000.c +3
-0+#define UCD9000_LOGGED_FAULTS 0xea +#define UCD9000_GPIO_SELECT 0xfa +#define UCD9000_GPIO_CONFIG 0xfb#define
UCD9000_DEVICE_ID0xfd
+/* GPIO CONFIG bits */Quoted 3 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
drivers/hwmon/pmbus/ucd9000.c line 47
Quoted 2 lines
drivers/hwmon/pmbus/ucd9000.c +2
-0+#define UCD9000_GPIO_CONFIG_ENABLE BIT(0) +#define UCD9000_GPIO_CONFIG_OUT_ENABLE BIT(1)+#define
UCD9000_GPIO_CONFIG_STATUSBIT(3)
+Quoted 2 lines
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
#define
UCD9000_MON_TYPE(x)(((x) >> 5) &0x07)#define
UCD9000_MON_PAGE(x)((x) &0x0f)
drivers/hwmon/pmbus/ucd9000.c +0
-0@@ -46,9 +57,12 @@Quoted 4 lines
Quoted 1 line
#define
UCD9000_NUM_FAN4
Quoted 1 line
+#define
UCD90160_NUM_GPIOS26
This is mostly true, though there are only 22 physical GPIOs, plus 4 GPIs. We're additionally constrained on the UCD90160 by "Table 3. GPIO Pin Configuration Options" in "16-Rail Power Supply Sequencer and Monitor with ACPI Support datasheet (Rev. C)" (ucd90160.pdf, page 22) to 8/26 as GPIs and 16/22 as GPOs. It would probably pay to think about how these constraints would impact the implementation.Quoted 17 lines
+
structucd9000_data{drivers/hwmon/pmbus/ucd9000.c line 65
Quoted 3 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
u8 fan_data[UCD9000_NUM_FAN][I2C_SMBUS_BLOCK_MAX]; struct
pmbus_driver_infoinfo;drivers/hwmon/pmbus/ucd9000.c +1
-0+struct gpio_chip gpio;
};
#defineto_ucd9000_data(_info)container_of(_info, struct ucd9000_data, info)drivers/hwmon/pmbus/ucd9000.c +0
-0@@ -119,6 +133,92 @@ static int ucd9000_read_byte_data(struct i2c_client *client, int page, int reg) }; MODULE_DEVICE_TABLE(i2c, ucd9000_id);
+static int ucd9000_gpio_read_config(structi2c_client*client,Review
Quoted 1 line
(quoted diff) +1
-0+ unsigned int offset)
+{Quoted 7 lines
Review
Quoted 1 line
(quoted diff) +1
-0+ int ret;
+Review
Quoted 4 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
drivers/hwmon/pmbus/ucd9000.c +3
-0+/* No page set required */ + ret =i2c_smbus_write_byte_data (client,UCD9000_GPIO_SELECT , offset); +if (ret <0 ) {(quoted diff) +1
-0+ dev_err(&client->dev,
Why is there a linebreak here?Quoted 1 line
drivers/hwmon/pmbus/ucd9000.c static int ucd9000_read_byte_data(struct i2c_client *client, int page, int reg) · line 145
Quoted 1 line
drivers/hwmon/pmbus/ucd9000.c +1
-0+ "Failed to read pin ID %d\n", offset);
The error message is confusing - it relates to the semantics ofucd9000_gpio_read_config()and not the operation on which the error occurred.
I feel strongly that we should also report the error code as well: It provides additional context on what went wrong.
As a consequence I would suggest something like:Quoted 2 lines
Quoted 2 lines
dev_err(&client->dev, "Failed to select GPIO %d: %d\n", offset, ret);Quoted 14 lines
Review
Quoted 2 lines
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
(quoted diff) +1
-0+ return ret;(quoted diff) +1
-0+ }
+drivers/hwmon/pmbus/ucd9000.c static int ucd9000_read_byte_data(struct i2c_client *client, int page, int reg) · line 149
Quoted 1 line
drivers/hwmon/pmbus/ucd9000.c +1
-0+return i2c_smbus_read_byte_data (client,UCD9000_GPIO_CONFIG );drivers/hwmon/pmbus/ucd9000.c +4
-0+} + +static int ucd9000_gpio_get (struct gpio_chip *gc,unsigned int offset) +{Review
Quoted 2 lines
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
(quoted diff) +1
-0+ struct i2c_client *client = gpiochip_get_data(gc);(quoted diff) +1
-0+ int ret;
+Quoted 4 lines
Review
Quoted 4 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
drivers/hwmon/pmbus/ucd9000.c +3
-0+ ret =ucd9000_gpio_read_config (client, offset); +if (ret <0 ) { + dev_err(&client->dev,(quoted diff) +1
-0+ "failed to read GPIO config\n");
Again, we don't need a linebreak above, but we probably should have one between thedev_err()and the return statement.Quoted 5 lines
Review
Quoted 2 lines
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
(quoted diff) +1
-0+ return ret;(quoted diff) +1
-0+ }
+drivers/hwmon/pmbus/ucd9000.c static int ucd9000_read_byte_data(struct i2c_client *client, int page, int reg) · line 164
Quoted 1 line
drivers/hwmon/pmbus/ucd9000.c +1
-0+return ret &UCD9000_GPIO_CONFIG_STATUS ?1 :0 ;
We can avoid the branch with:
return !!(ret &UCD9000_GPIO_CONFIG_STATUS);
Interestingly it's not clear from the datasheet whether the status bit contains a relevant value when the pin is in output mode, as there's a separate bit (bit
2) to set the value. Have you tested the behaviour?Quoted 20 lines
drivers/hwmon/pmbus/ucd9000.c +3
-0+} + +static int ucd9000_gpio_get_direction(struct gpio_chip *gc,Review
Quoted 1 line
(quoted diff) +1
-0+ unsigned int offset)
+{Review
Quoted 2 lines
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
(quoted diff) +1
-0+ struct i2c_client *client = gpiochip_get_data(gc);(quoted diff) +1
-0+ int ret;
+Review
Quoted 6 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
drivers/hwmon/pmbus/ucd9000.c +5
-0+ ret =ucd9000_gpio_read_config (client, offset); +if (ret <0 ) { +dev_err (&client->dev, +"failed to read GPIO config\n" ); +return ret;(quoted diff) +1
-0+ }
+drivers/hwmon/pmbus/ucd9000.c static int ucd9000_read_byte_data(struct i2c_client *client, int page, int reg) · line 180
Quoted 1 line
drivers/hwmon/pmbus/ucd9000.c +1
-0+return ret &UCD9000_GPIO_CONFIG_OUT_ENABLE ?0 :1 ;drivers/hwmon/pmbus/ucd9000.c static int ucd9000_read_byte_data(struct i2c_client *client, int page, int reg) · line 185
Quoted 5 lines
drivers/hwmon/pmbus/ucd9000.c +5
-0+} + +/* +* Defined so that libgpiod tool can access pin states. No plan to actually +* change pin direction within this driver.
That's not true - the driver is general purpose. Lets not limit it too much to *how you intend to use it* vs the actual hardware capability.Quoted 6 lines
+ */
+static int ucd9000_gpio_direction_input(structgpio_chip*gc,Review
Quoted 1 line
(quoted diff) +1
-0+ unsigned int offset)
+{drivers/hwmon/pmbus/ucd9000.c static int ucd9000_read_byte_data(struct i2c_client *client, int page, int reg) · line 190
Quoted 1 line
drivers/hwmon/pmbus/ucd9000.c +1
-0+return 0 ;
I don't think we should do this for several reasons:
1) It's not that hard to implement the functionality
2) It's not true that all GPIOs are inputs with factory defaults
3) There will be uncertainty if custom defaults are programmed
Again from ucd90160.pdf, on page 34:
7.5 Programming
7.5.1 Device Configuration and Programming
From the factory, the device contains the sequencing and monitoring firmware. It is also configured so that all GPOs are high-impedance (except for FPWM/GPIO pins 17-24, which are driven low), with no sequencing or fault-response operation. See Configuration Programming of UCD Devices, available from the Documentation & Help Center that can be selected from the Fusion GUI Help menu, for full UCD90160 configuration details.
The FPWM pins are driven low, implying they are configured for output mode. It's not obvious to me from this whether the other GPIOs are input or output out-of-the-box as the chip supports active and open-drain drive types. It does say "GPOs" in the paragraph above, but I haven't found documentation on which pins are in what state from factory, and I don't know how we're configuring it for production.
Regardless, the driver shouldn't make assumptions about how the device is pre-programmed - without further information there's already ambiguity as to whether the default settings are in play or whether the system integrator has configured the chip. We need information from the platform about how the device is/should be managed.
Further, the note in section 10.43 (FBh)GPIO_CONFIG(MFR_SPECIFIC_43) says:
Configuring a pin that is also being used by another function (enable, fan control, and so forth) may likely result in unexpected and unwanted behavior.
This suggests my favourite topic: pinctrl.
Twenty-two of the pins on the UCD90160 are GPIO-capable. Of these 22 pins, 16 of them have alternate functions. See "Table 3. GPIO Pin Configuration Options" on page 22 of ucd90160.pdf. But a quick rundown:
1. 4 PWM pins (GPIOs 1-4, pins 31, 32, 42, 41)
2. 8 FPWM (Fast PWM) pins (GPIOs 5-12, pins 17-24)
3. 4 pins shared with JTAG (GPIOs 19-22, pins 36-39)
Now, in terms of devicetree we'd be breaking things in the future by specifying a GPIO controller that relies on a pinmux in hardware without also specifying the pin controller in the devicetree. This necessitates at least *some* pinctrl capability be baked into the driver as well, so we can point the gpiochip to the relevant pin controller (the same device, but the bindings are general) and map the pin spaces in the devicetree (the gpio and pin spaces are different).Quoted 36 lines
drivers/hwmon/pmbus/ucd9000.c +3
-0+} + +static ssize_t ucd9000_clear_logged_faults(struct device *dev,drivers/hwmon/pmbus/ucd9000.c static int ucd9000_read_byte_data(struct i2c_client *client, int page, int reg) · line 195
Quoted 2 lines
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
drivers/hwmon/pmbus/ucd9000.c +1
-0+ struct device_attribute *attr, const char *buf,drivers/hwmon/pmbus/ucd9000.c +1
-0+ size_t count)
+{Review
Quoted 2 lines
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
drivers/hwmon/pmbus/ucd9000.c +1
-0+struct i2c_client *client =to_i2c_client (dev);(quoted diff) +1
-0+ int ret;
+Review
Quoted 6 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
drivers/hwmon/pmbus/ucd9000.c +5
-0+/* No page set required */ + ret =i2c_smbus_write_byte_data (client,UCD9000_LOGGED_FAULTS ,0 ); +if (ret) { +dev_err (&client->dev,"Failed to clear logged faults\n" ); +return ret;(quoted diff) +1
-0+ }
+drivers/hwmon/pmbus/ucd9000.c static int ucd9000_read_byte_data(struct i2c_client *client, int page, int reg) · line 207
Quoted 1 line
drivers/hwmon/pmbus/ucd9000.c +1
-0+return count;drivers/hwmon/pmbus/ucd9000.c +3
-0+} + +static DEVICE_ATTR(clear_logged_faults, 0200, NULL,drivers/hwmon/pmbus/ucd9000.c static int ucd9000_read_byte_data(struct i2c_client *client, int page, int reg) · line 211
Quoted 1 line
drivers/hwmon/pmbus/ucd9000.c +1
-0+ ucd9000_clear_logged_faults);drivers/hwmon/pmbus/ucd9000.c +2
-0+ +static struct attribute *ucd9000_attributes[] = {drivers/hwmon/pmbus/ucd9000.c static int ucd9000_read_byte_data(struct i2c_client *client, int page, int reg) · line 215
Quoted 2 lines
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
drivers/hwmon/pmbus/ucd9000.c +1
-0+ &dev_attr_clear_logged_faults.attr,drivers/hwmon/pmbus/ucd9000.c +1
-0+NULL drivers/hwmon/pmbus/ucd9000.c +3
-0+}; + +static const struct attribute_group ucd9000_attr_group = {drivers/hwmon/pmbus/ucd9000.c static int ucd9000_read_byte_data(struct i2c_client *client, int page, int reg) · line 219
Quoted 1 line
drivers/hwmon/pmbus/ucd9000.c +1
-0+ .attrs = ucd9000_attributes,drivers/hwmon/pmbus/ucd9000.c
Quoted 2 lines
drivers/hwmon/pmbus/ucd9000.c +2
-0+}; +
Joel's made a fair comment here regarding upstream. Please address that.Quoted 9 lines
static int ucd9000_probe(struct
i2c_client*client,Quoted 1 line
const struct
i2c_device_id*id)
{drivers/hwmon/pmbus/ucd9000.c +0
-0@@ -227,7 +327,39 @@ static int ucd9000_probe(struct i2c_client *client, > > > > > | PMBUS_HAVE_FAN34 | PMBUS_HAVE_STATUS_FAN34; > }
Quoted 5 lines
drivers/hwmon/pmbus/ucd9000.c static int ucd9000_probe(struct i2c_client *client, · line 333
Quoted 5 lines
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
drivers/hwmon/pmbus/ucd9000.c +3
-1-return pmbus_do_probe (client, mid, info); + data->gpio.label ="gpio-ucd9000" ; + data->gpio.get_direction = ucd9000_gpio_get_direction; + data->gpio.direction_input = ucd9000_gpio_direction_input;drivers/hwmon/pmbus/ucd9000.c +1
-0+ data->gpio.get = ucd9000_gpio_get;
I feel like it wouldn't be much effort to make this more complete and specify thedirection_output()andset()callbacks as well.Quoted 4 lines
drivers/hwmon/pmbus/ucd9000.c static int ucd9000_probe(struct i2c_client *client, · line 337
Quoted 4 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
drivers/hwmon/pmbus/ucd9000.c +3
-0+ data->gpio.can_sleep =1 ; + data->gpio.base =-1 ; +if (mid->driver_data == ucd90160)drivers/hwmon/pmbus/ucd9000.c +1
-0+ data->gpio.ngpio =UCD90160_NUM_GPIOS ;
This might need some rework in the future as the other ucd9000 variants want GPIO support.
In fact, what's the behaviour of the other drivers now given we're unconditionally registering a gpiochip?Quoted 2 lines
drivers/hwmon/pmbus/ucd9000.c static int ucd9000_probe(struct i2c_client *client, · line 339
Quoted 2 lines
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
drivers/hwmon/pmbus/ucd9000.c +1
-0+ data->gpio.parent = &client->dev;drivers/hwmon/pmbus/ucd9000.c +1
-0+ data->gpio.owner =THIS_MODULE ;
At some point we should think about pinconf as well (push-pull vs open-drain).Quoted 9 lines
+
Review
Quoted 6 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
drivers/hwmon/pmbus/ucd9000.c +5
-0+ ret =devm_gpiochip_add_data (&client->dev, &data->gpio, client); +if (ret) { + data->gpio.parent =NULL ; +dev_warn (&client->dev,"Count not add gpiochip\n" ); +return ret;(quoted diff) +1
-0+ }
+Quoted 38 lines
Quoted 2 lines
drivers/hwmon/pmbus/ucd9000.c static int ucd9000_probe(struct i2c_client *client, · line 349
Quoted 2 lines
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
drivers/hwmon/pmbus/ucd9000.c +1
-0+ dev_info(&client->dev, "gpios %i...%i\n", data->gpio.base,drivers/hwmon/pmbus/ucd9000.c +1
-0+ data->gpio.base + data->gpio.ngpio - 1);
Let's drop this.
Cheers,
AndrewQuoted 29 lines
+
Review
Quoted 4 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
drivers/hwmon/pmbus/ucd9000.c +3
-0+ ret =sysfs_create_group (&client->dev.kobj, + &ucd9000_attr_group); +if (ret <0 )(quoted diff) +1
-0+ return ret;
+drivers/hwmon/pmbus/ucd9000.c static int ucd9000_probe(struct i2c_client *client, · line 356
Quoted 1 line
drivers/hwmon/pmbus/ucd9000.c +1
-0+ return pmbus_do_probe(client, mid, info);drivers/hwmon/pmbus/ucd9000.c +4
-0+} + +static int ucd9000_remove (struct i2c_client *client) +{drivers/hwmon/pmbus/ucd9000.c static int ucd9000_probe(struct i2c_client *client, · line 362
Quoted 2 lines
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
drivers/hwmon/pmbus/ucd9000.c +1
-0+sysfs_remove_group (&client->dev.kobj, &ucd9000_attr_group);drivers/hwmon/pmbus/ucd9000.c +1
-0+return pmbus_do_remove (client);
}
/* This is the driver that will be inserted */drivers/hwmon/pmbus/ucd9000.c +0
-0@@ -236,7 +368,7 @@ static int ucd9000_probe(struct i2c_client *client, > > > > > .name = "ucd9000", > > > > > }, > > > > > .probe = ucd9000_probe, > > > > > - .remove = pmbus_do_remove, > > > > > + .remove = ucd9000_remove, > .id_table = ucd9000_id,
};
Re: [PATCH linux dev-4.10 v3 1/2] hwmon: (ucd9000) Add gpio chip interface and clear logged faults
In reply to Andrew JefferyQuoted 63 lines · Andrew Jeffery
Hi Chris,
Quoted 60 lines · Christopher Bostic
Add a struct
gpio_chipand define some methods so that this device's I/O can be accessed via/sys/class/gpio.Add capability to clear logged faults via sysfs.
Quoted 1 line
Signed-off-by Christopher Bostic <[email protected]>---
v3 - Correct bug ingpio_chipget method. Wasn't retrieving
gpio config information correctly.
- Remove old debugfs flag from previous pmbus core changes.
- Remove all sysfs files formfr_statuscommand.
- Add comments on directi2c_smbuscalls to clarify that no page set is required.
v2 - Removeclear_faultsfile - redundant since all other sysfs
core accesses result in an automatic clear fault.
- Removed localstatus_wordandstatus_voutregister dumps and use the new pmbus core status facilities instead.
- Rename gpi<x>_fault to gpi<x>_alarm to better match core naming conventions.
- Add full register dump formfr_status.
- Add gpio chip to device structure and use gpio interfaces.--- drivers/hwmon/pmbus/ucd9000.c | 136 +++++++++++++++++++++++++++++++++++++++++- 1 file changed, 134 insertions(+), 2 deletions(-)drivers/hwmon/pmbus/ucd9000.c +3
-0@@ -26,6 +26,9 @@#include <linux/slab.h> #include <linux/i2c.h> #include <linux/i2c/pmbus.h> +#include <linux/sysfs.h> +#include <linux/hwmon-sysfs.h> +#include <linux/gpio.h> #include "pmbus.h" enum chips { ucd9000, ucd90120, ucd90124, ucd90160, ucd9090, ucd90910 };@@ -34,8 +37,16 @@drivers/hwmon/pmbus/ucd9000.c line 43
Quoted 7 lines
#define UCD9000_NUM_PAGES 0xd6 #define UCD9000_FAN_CONFIG_INDEX 0xe7 #define UCD9000_FAN_CONFIG 0xe8drivers/hwmon/pmbus/ucd9000.c +3
-0+#define UCD9000_LOGGED_FAULTS 0xea +#define UCD9000_GPIO_SELECT 0xfa +#define UCD9000_GPIO_CONFIG 0xfb#define UCD9000_DEVICE_ID 0xfd+/* GPIO CONFIG bits */
drivers/hwmon/pmbus/ucd9000.c line 48
Quoted 3 lines
drivers/hwmon/pmbus/ucd9000.c +3
-0+#define UCD9000_GPIO_CONFIG_ENABLE BIT(0) +#define UCD9000_GPIO_CONFIG_OUT_ENABLE BIT(1) +#define UCD9000_GPIO_CONFIG_STATUS BIT(3)+
Quoted 2 lines
#define UCD9000_MON_TYPE(x) (((x) >> 5) & 0x07) #define UCD9000_MON_PAGE(x) ((x) & 0x0f)drivers/hwmon/pmbus/ucd9000.c +0
-0@@ -46,9 +57,12 @@Quoted 1 line
#define UCD9000_NUM_FAN 4
Quoted 7 lines
Quoted 1 line
Quoted 1 line
+#define
UCD90160_NUM_GPIOS26This is mostly true, though there are only 22 physical GPIOs, plus 4 GPIs. We're additionally constrained on the UCD90160 by "Table 3. GPIO Pin Configuration Options" in "16-Rail Power Supply Sequencer and Monitor with ACPI Support datasheet (Rev. C)" (ucd90160.pdf, page 22) to 8/26 as GPIs and 16/22 as GPOs. It would probably pay to think about how these constraints would impact the implementation.
Hi Andrew,
Will go through and review the data for the various ucd9000 devs. I understand the need to cover all these device types but want to also be able to deliver internally what is needed which is just the gpi faults. Maybe we can discuss further on Sunday's call.
Quoted 15 lines
Quoted 15 lines
+
structucd9000_data{drivers/hwmon/pmbus/ucd9000.c line 65
Quoted 3 lines
u8 fan_data[UCD9000_NUM_FAN][I2C_SMBUS_BLOCK_MAX]; struct pmbus_driver_info info;drivers/hwmon/pmbus/ucd9000.c +1
-0+struct gpio_chip gpio;};
#defineto_ucd9000_data(_info)container_of(_info, struct ucd9000_data, info)drivers/hwmon/pmbus/ucd9000.c +2
-0@@ -119,6 +133,92 @@ static int ucd9000_read_byte_data(struct i2c_client *client, int page, int reg) };MODULE_DEVICE_TABLE (i2c, ucd9000_id); +static int ucd9000_gpio_read_config (struct i2c_client *client, > + unsigned int offset) +{
Quoted 7 lines
Quoted 6 lines
drivers/hwmon/pmbus/ucd9000.c +1
-0> + int ret; +drivers/hwmon/pmbus/ucd9000.c static int ucd9000_read_byte_data(struct i2c_client *client, int page, int reg) · line 144
Quoted 4 lines
drivers/hwmon/pmbus/ucd9000.c +4
-0+/* No page set required */ + ret =i2c_smbus_write_byte_data (client,UCD9000_GPIO_SELECT , offset); +if (ret <0 ) { + dev_err(&client->dev,Why is there a linebreak here?
No good reason, will correct.
Quoted 5 lines
Quoted 2 lines
drivers/hwmon/pmbus/ucd9000.c static int ucd9000_read_byte_data(struct i2c_client *client, int page, int reg) · line 145
Quoted 2 lines
drivers/hwmon/pmbus/ucd9000.c +1
-0+ "Failed to read pin ID %d\n", offset);The error message is confusing - it relates to the semantics of
ucd9000_gpio_read_config()and not the operation on which the error occurred.
Quoted 7 lines
I feel strongly that we should also report the error code as well: It provides additional context on what went wrong.
As a consequence I would suggest something like:
dev_err(&client->dev, "Failed to select GPIO %d: %d\n", offset, ret);
OK, will go with your suggestion.
Quoted 11 lines
Quoted 11 lines
Review
Quoted 2 lines
(quoted diff) +2
-0+ return ret; + }+
Quoted 1 line
drivers/hwmon/pmbus/ucd9000.c +1
-0+return i2c_smbus_read_byte_data (client,UCD9000_GPIO_CONFIG );drivers/hwmon/pmbus/ucd9000.c +4
-0+} + +static int ucd9000_gpio_get (struct gpio_chip *gc,unsigned int offset) +{Review
Quoted 2 lines
(quoted diff) +2
-0+ struct i2c_client *client = gpiochip_get_data(gc); + int ret;+
Quoted 6 lines
Quoted 4 lines
drivers/hwmon/pmbus/ucd9000.c static int ucd9000_read_byte_data(struct i2c_client *client, int page, int reg) · line 160
Quoted 4 lines
drivers/hwmon/pmbus/ucd9000.c +4
-0+ ret =ucd9000_gpio_read_config (client, offset); +if (ret <0 ) { +dev_err (&client->dev, +"failed to read GPIO config\n" );Again, we don't need a linebreak above, but we probably should have one between the
dev_err()and the return statement.
Will take care of that.
Quoted 7 lines
Quoted 4 lines
Review
Quoted 2 lines
(quoted diff) +2
-0+ return ret; + }+
drivers/hwmon/pmbus/ucd9000.c static int ucd9000_read_byte_data(struct i2c_client *client, int page, int reg) · line 164
Quoted 1 line
drivers/hwmon/pmbus/ucd9000.c +1
-0+return ret &UCD9000_GPIO_CONFIG_STATUS ?1 :0 ;We can avoid the branch with:
return !!(ret & UCD9000_GPIO_CONFIG_STATUS);
Will update.
Quoted 3 lines
Interestingly it's not clear from the datasheet whether the status bit contains a relevant value when the pin is in output mode, as there's a separate bit (bit
2) to set the value. Have you tested the behaviour?
I've not tested this in output mode. Up to this point there isn't any means to set direction so, at least for how we're using it currently these are all set to inputs. Will see how this behaves in output mode.
Quoted 15 lines
Quoted 15 lines
drivers/hwmon/pmbus/ucd9000.c +3
-0+} + +static int ucd9000_gpio_get_direction(struct gpio_chip *gc,Review
Quoted 1 line
(quoted diff) +1
-0+ unsigned int offset)+{
Review
Quoted 2 lines
(quoted diff) +2
-0+ struct i2c_client *client = gpiochip_get_data(gc); + int ret;+
drivers/hwmon/pmbus/ucd9000.c static int ucd9000_read_byte_data(struct i2c_client *client, int page, int reg) · line 162
Quoted 6 lines
drivers/hwmon/pmbus/ucd9000.c +6
-0+ ret =ucd9000_gpio_read_config (client, offset); +if (ret <0 ) { +dev_err (&client->dev, +"failed to read GPIO config\n" ); +return ret; + }+
Quoted 8 lines
drivers/hwmon/pmbus/ucd9000.c static int ucd9000_read_byte_data(struct i2c_client *client, int page, int reg) · line 185
Quoted 6 lines
Quoted 1 line
drivers/hwmon/pmbus/ucd9000.c +1
-0+return ret &UCD9000_GPIO_CONFIG_OUT_ENABLE ?0 :1 ;drivers/hwmon/pmbus/ucd9000.c +5
-0+} + +/* +* Defined so that libgpiod tool can access pin states. No plan to actually +* change pin direction within this driver. That's not true - the driver is general purpose. Lets not limit it too much to *how you intend to use it* vs the actual hardware capability.
Understand. Will implement direction switch for next revision in any case.
Quoted 36 lines
Quoted 5 lines
+ */
+static int ucd9000_gpio_direction_input(structgpio_chip*gc,Review
Quoted 1 line
(quoted diff) +1
-0+ unsigned int offset)+{
drivers/hwmon/pmbus/ucd9000.c static int ucd9000_read_byte_data(struct i2c_client *client, int page, int reg) · line 190
Quoted 1 line
drivers/hwmon/pmbus/ucd9000.c +1
-0+return 0 ;I don't think we should do this for several reasons:
1) It's not that hard to implement the functionality
2) It's not true that all GPIOs are inputs with factory defaults
3) There will be uncertainty if custom defaults are programmedAgain from ucd90160.pdf, on page 34:
7.5 Programming
7.5.1 Device Configuration and Programming
From the factory, the device contains the sequencing and monitoring firmware. It is also configured so that all GPOs are high-impedance (except for FPWM/GPIO pins 17-24, which are driven low), with no sequencing or fault-response operation. See Configuration Programming of UCD Devices, available from the Documentation & Help Center that can be selected from the Fusion GUI Help menu, for full UCD90160 configuration details.The FPWM pins are driven low, implying they are configured for output mode. It's not obvious to me from this whether the other GPIOs are input or output out-of-the-box as the chip supports active and open-drain drive types. It does say "GPOs" in the paragraph above, but I haven't found documentation on which pins are in what state from factory, and I don't know how we're configuring it for production.
Regardless, the driver shouldn't make assumptions about how the device is pre-programmed - without further information there's already ambiguity as to whether the default settings are in play or whether the system integrator has configured the chip. We need information from the platform about how the device is/should be managed.
Quoted 5 lines
Further, the note in section 10.43 (FBh)
GPIO_CONFIG(MFR_SPECIFIC_43) says:Configuring a pin that is also being used by another function (enable, fan control, and so forth) may likely result in unexpected and unwanted behavior.
All good points. Will implement direction switching for next submit.
Quoted 10 lines
This suggests my favourite topic: pinctrl.
Twenty-two of the pins on the UCD90160 are GPIO-capable. Of these 22 pins, 16 of them have alternate functions. See "Table 3. GPIO Pin Configuration Options" on page 22 of ucd90160.pdf. But a quick rundown:
1. 4 PWM pins (GPIOs 1-4, pins 31, 32, 42, 41)
2. 8 FPWM (Fast PWM) pins (GPIOs 5-12, pins 17-24)
3. 4 pins shared with JTAG (GPIOs 19-22, pins 36-39)
Quoted 6 lines
Now, in terms of devicetree we'd be breaking things in the future by specifying a GPIO controller that relies on a pinmux in hardware without also specifying the pin controller in the devicetree. This necessitates at least *some* pinctrl capability be baked into the driver as well, so we can point the gpiochip to the relevant pin controller (the same device, but the bindings are general) and map the pin spaces in the devicetree (the gpio and pin spaces are different).
I'd like to avoid implementing pinctrl for the time being given the limited function we need internally. I do see the advantages as you point out so maybe we can phase this in?
Quoted 28 lines
Quoted 28 lines
drivers/hwmon/pmbus/ucd9000.c +3
-0+} + +static ssize_t ucd9000_clear_logged_faults(struct device *dev,drivers/hwmon/pmbus/ucd9000.c static int ucd9000_read_byte_data(struct i2c_client *client, int page, int reg) · line 195
Quoted 2 lines
drivers/hwmon/pmbus/ucd9000.c +2
-0+ struct device_attribute *attr, const char *buf, + size_t count)+{
drivers/hwmon/pmbus/ucd9000.c static int ucd9000_read_byte_data(struct i2c_client *client, int page, int reg) · line 198
Quoted 2 lines
drivers/hwmon/pmbus/ucd9000.c +2
-0+struct i2c_client *client =to_i2c_client (dev); +int ret;+
drivers/hwmon/pmbus/ucd9000.c static int ucd9000_read_byte_data(struct i2c_client *client, int page, int reg) · line 205
Quoted 6 lines
drivers/hwmon/pmbus/ucd9000.c +6
-0+/* No page set required */ + ret =i2c_smbus_write_byte_data (client,UCD9000_LOGGED_FAULTS ,0 ); +if (ret) { +dev_err (&client->dev,"Failed to clear logged faults\n" ); +return ret; + }+
Quoted 1 line
drivers/hwmon/pmbus/ucd9000.c +1
-0+return count;drivers/hwmon/pmbus/ucd9000.c +3
-0+} + +static DEVICE_ATTR(clear_logged_faults, 0200, NULL,Quoted 1 line
drivers/hwmon/pmbus/ucd9000.c +1
-0+ ucd9000_clear_logged_faults);drivers/hwmon/pmbus/ucd9000.c +2
-0+ +static struct attribute *ucd9000_attributes[] = {Quoted 2 lines
drivers/hwmon/pmbus/ucd9000.c +2
-0+ &dev_attr_clear_logged_faults.attr, +NULL drivers/hwmon/pmbus/ucd9000.c +3
-0+}; + +static const struct attribute_group ucd9000_attr_group = {
Quoted 4 lines
drivers/hwmon/pmbus/ucd9000.c
Quoted 3 lines
Quoted 1 line
drivers/hwmon/pmbus/ucd9000.c +1
-0+ .attrs = ucd9000_attributes,drivers/hwmon/pmbus/ucd9000.c +2
-0+}; +Joel's made a fair comment here regarding upstream. Please address that.
Next patch set will be sent upstream.
Quoted 7 lines
Quoted 7 lines
static int ucd9000_probe(struct
i2c_client*client,Quoted 1 line
const struct
i2c_device_id*id){
drivers/hwmon/pmbus/ucd9000.c +0
-0@@ -227,7 +327,39 @@ static int ucd9000_probe(struct i2c_client *client, > | PMBUS_HAVE_FAN34 | PMBUS_HAVE_STATUS_FAN34; > }
Quoted 7 lines
Quoted 5 lines
drivers/hwmon/pmbus/ucd9000.c +0
-0> - return pmbus_do_probe(client, mid, info); > + data->gpio.label = "gpio-ucd9000"; > + data->gpio.get_direction = ucd9000_gpio_get_direction; > + data->gpio.direction_input = ucd9000_gpio_direction_input;drivers/hwmon/pmbus/ucd9000.c static int ucd9000_probe(struct i2c_client *client, · line 333
Quoted 1 line
drivers/hwmon/pmbus/ucd9000.c +1
-0+ data->gpio.get = ucd9000_gpio_get;I feel like it wouldn't be much effort to make this more complete and specify the
direction_output()andset()callbacks as well.
Right, will do that.
Quoted 10 lines
Quoted 5 lines
drivers/hwmon/pmbus/ucd9000.c static int ucd9000_probe(struct i2c_client *client, · line 337
Quoted 5 lines
drivers/hwmon/pmbus/ucd9000.c +4
-0+ data->gpio.can_sleep =1 ; + data->gpio.base =-1 ; +if (mid->driver_data == ucd90160) + data->gpio.ngpio =UCD90160_NUM_GPIOS ;This might need some rework in the future as the other ucd9000 variants want GPIO support.
In fact, what's the behaviour of the other drivers now given we're unconditionally registering a gpiochip?
The other drivers would have a gpio_chip that is available but if they don't use it I'm not thinking this would break them in any way. Would it be better to only register a gpiochip if its a ucd90160 in particular?
Quoted 3 lines
Quoted 2 lines
drivers/hwmon/pmbus/ucd9000.c static int ucd9000_probe(struct i2c_client *client, · line 339
Quoted 2 lines
drivers/hwmon/pmbus/ucd9000.c +2
-0+ data->gpio.parent = &client->dev; + data->gpio.owner =THIS_MODULE ;At some point we should think about pinconf as well (push-pull vs open-drain).
Ok, will make a note of that.
Quoted 11 lines
Quoted 10 lines
+
drivers/hwmon/pmbus/ucd9000.c static int ucd9000_probe(struct i2c_client *client, · line 346
Quoted 6 lines
drivers/hwmon/pmbus/ucd9000.c +6
-0+ ret =devm_gpiochip_add_data (&client->dev, &data->gpio, client); +if (ret) { + data->gpio.parent =NULL ; +dev_warn (&client->dev,"Count not add gpiochip\n" ); +return ret; + }+
drivers/hwmon/pmbus/ucd9000.c static int ucd9000_probe(struct i2c_client *client, · line 349
Quoted 2 lines
drivers/hwmon/pmbus/ucd9000.c +2
-0+dev_info (&client->dev,"gpios %i...%i\n" , data->gpio.base, + data->gpio.base + data->gpio.ngpio -1 );Let's drop this.
Will remove.
Thanks,
Chris
Quoted 30 lines
Cheers,
Andrew
Quoted 25 lines
+
drivers/hwmon/pmbus/ucd9000.c static int ucd9000_probe(struct i2c_client *client, · line 354
Quoted 4 lines
drivers/hwmon/pmbus/ucd9000.c +4
-0+ ret =sysfs_create_group (&client->dev.kobj, + &ucd9000_attr_group); +if (ret <0 ) +return ret;+
Quoted 1 line
drivers/hwmon/pmbus/ucd9000.c +1
-0+return pmbus_do_probe (client, mid, info);drivers/hwmon/pmbus/ucd9000.c +4
-0+} + +static int ucd9000_remove (struct i2c_client *client) +{drivers/hwmon/pmbus/ucd9000.c static int ucd9000_probe(struct i2c_client *client, · line 362
Quoted 2 lines
drivers/hwmon/pmbus/ucd9000.c +2
-0+sysfs_remove_group (&client->dev.kobj, &ucd9000_attr_group); +return pmbus_do_remove (client);}
/* This is the driver that will be inserted */
drivers/hwmon/pmbus/ucd9000.c +0
-0@@ -236,7 +368,7 @@ static int ucd9000_probe(struct i2c_client *client, > .name = "ucd9000", > }, > .probe = ucd9000_probe, > - .remove = pmbus_do_remove, > + .remove = ucd9000_remove, > .id_table = ucd9000_id, };
Re: [PATCH linux dev-4.10 v3 1/2] hwmon: (ucd9000) Add gpio chip interface and clear logged faults
Hi Chris,
Quoted 78 lines · Christopher Bostic
Quoted 68 lines · Andrew Jeffery
Hi Chris,
Quoted 65 lines · Christopher Bostic
Add a struct
gpio_chipand define some methods so that this device's I/O can be accessed via/sys/class/gpio.
Add capability to clear logged faults via sysfs.Quoted 1 line
Signed-off-by Christopher Bostic <[email protected]>
---
v3 - Correct bug ingpio_chipget method. Wasn't retrieving
gpio config information correctly.
- Remove old debugfs flag from previous pmbus core changes.
- Remove all sysfs files formfr_statuscommand.
- Add comments on directi2c_smbuscalls to clarify that no page set is required.
v2 - Removeclear_faultsfile - redundant since all other sysfs core accesses result in an automatic clear fault.
- Removed localstatus_wordandstatus_voutregister dumps and use the new pmbus core status facilities instead.
- Rename gpi<x>_fault to gpi<x>_alarm to better match core naming conventions.
- Add full register dump formfr_status.
- Add gpio chip to device structure and use gpio interfaces.
---
drivers/hwmon/pmbus/ucd9000.c| 136 +++++++++++++++++++++++++++++++++++++++++-1 file changed, 134 insertions(+), 2 deletions(-)drivers/hwmon/pmbus/ucd9000.c +3
-0@@ -26,6 +26,9 @@ #include <linux/slab.h> #include <linux/i2c.h> #include <linux/i2c/pmbus.h> +#include <linux/sysfs.h> +#include <linux/hwmon-sysfs.h> +#include <linux/gpio.h> #include "pmbus.h"
enum chips { ucd9000, ucd90120, ucd90124, ucd90160, ucd9090, ucd90910 };drivers/hwmon/pmbus/ucd9000.c +0
-0@@ -34,8 +37,16 @@Quoted 7 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
drivers/hwmon/pmbus/ucd9000.c line 42
Quoted 6 lines
#define
UCD9000_NUM_PAGES0xd6
#defineUCD9000_FAN_CONFIG_INDEX0xe7
#defineUCD9000_FAN_CONFIG0xe8drivers/hwmon/pmbus/ucd9000.c +3
-0+#define UCD9000_LOGGED_FAULTS 0xea +#define UCD9000_GPIO_SELECT 0xfa +#define UCD9000_GPIO_CONFIG 0xfb#define
UCD9000_DEVICE_ID0xfd
+/* GPIO CONFIG bits */Quoted 3 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
drivers/hwmon/pmbus/ucd9000.c line 47
Quoted 2 lines
drivers/hwmon/pmbus/ucd9000.c +2
-0+#define UCD9000_GPIO_CONFIG_ENABLE BIT(0) +#define UCD9000_GPIO_CONFIG_OUT_ENABLE BIT(1)+#define
UCD9000_GPIO_CONFIG_STATUSBIT(3)
+Quoted 2 lines
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
#define
UCD9000_MON_TYPE(x)(((x) >> 5) &0x07)#define
UCD9000_MON_PAGE(x)((x) &0x0f)
drivers/hwmon/pmbus/ucd9000.c +0
-0@@ -46,9 +57,12 @@Quoted 1 line
#define
UCD9000_NUM_FAN4
Quoted 8 lines
Quoted 1 line
Quoted 1 line
+#define
UCD90160_NUM_GPIOS26
This is mostly true, though there are only 22 physical GPIOs, plus 4 GPIs. We're additionally constrained on the UCD90160 by "Table 3. GPIO Pin Configuration Options" in "16-Rail Power Supply Sequencer and Monitor with ACPI Support datasheet (Rev. C)" (ucd90160.pdf, page 22) to 8/26 as GPIs and 16/22 as GPOs. It would probably pay to think about how these constraints would impact the implementation.
Quoted 7 lines
Hi Andrew,
Will go through and review the data for the various ucd9000 devs. I understand the need to cover all these device types but want to also be able to deliver internally what is needed which is just the gpi faults. Maybe we can discuss further on Sunday's call.
Yeah, understood, it was a thinking-out-loud comment. However I would hate to get too deep on an implementation only to find that it's not appropriate for the device's behaviour.
Quoted 83 lines
Quoted 20 lines
Quoted 20 lines
+
structucd9000_data{drivers/hwmon/pmbus/ucd9000.c line 65
Quoted 3 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
u8 fan_data[UCD9000_NUM_FAN][I2C_SMBUS_BLOCK_MAX]; struct
pmbus_driver_infoinfo;drivers/hwmon/pmbus/ucd9000.c +1
-0+struct gpio_chip gpio;
};
#defineto_ucd9000_data(_info)container_of(_info, struct ucd9000_data, info)drivers/hwmon/pmbus/ucd9000.c +0
-0@@ -119,6 +133,92 @@ static int ucd9000_read_byte_data(struct i2c_client *client, int page, int reg) }; MODULE_DEVICE_TABLE(i2c, ucd9000_id);
+static int ucd9000_gpio_read_config(structi2c_client*client,Review
Quoted 1 line
(quoted diff) +1
-0+ unsigned int offset)
+{Review
Quoted 1 line
(quoted diff) +1
-0+ int ret;
+Quoted 6 lines
Quoted 4 lines
Review
Quoted 4 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
drivers/hwmon/pmbus/ucd9000.c +3
-0+/* No page set required */ + ret =i2c_smbus_write_byte_data (client,UCD9000_GPIO_SELECT , offset); +if (ret <0 ) {(quoted diff) +1
-0+ dev_err(&client->dev,
Why is there a linebreak here?
No good reason, will correct.Quoted 11 lines
Quoted 1 line
drivers/hwmon/pmbus/ucd9000.c static int ucd9000_read_byte_data(struct i2c_client *client, int page, int reg) · line 145
Quoted 1 line
drivers/hwmon/pmbus/ucd9000.c +1
-0+ "Failed to read pin ID %d\n", offset);
The error message is confusing - it relates to the semantics ofucd9000_gpio_read_config()and not the operation on which the error occurred.
I feel strongly that we should also report the error code as well: It provides additional context on what went wrong.
As a consequence I would suggest something like:Quoted 2 lines
Quoted 1 line
Quoted 1 line
dev_err(&client->dev, "Failed to select GPIO %d: %d\n", offset,
ret);
OK, will go with your suggestion.Quoted 14 lines
Quoted 14 lines
Review
Quoted 2 lines
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
(quoted diff) +1
-0+ return ret;(quoted diff) +1
-0+ }
+drivers/hwmon/pmbus/ucd9000.c static int ucd9000_read_byte_data(struct i2c_client *client, int page, int reg) · line 149
Quoted 1 line
drivers/hwmon/pmbus/ucd9000.c +1
-0+return i2c_smbus_read_byte_data (client,UCD9000_GPIO_CONFIG );drivers/hwmon/pmbus/ucd9000.c +4
-0+} + +static int ucd9000_gpio_get (struct gpio_chip *gc,unsigned int offset) +{Review
Quoted 2 lines
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
(quoted diff) +1
-0+ struct i2c_client *client = gpiochip_get_data(gc);(quoted diff) +1
-0+ int ret;
+Quoted 7 lines
Quoted 4 lines
Review
Quoted 4 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
drivers/hwmon/pmbus/ucd9000.c +3
-0+ ret =ucd9000_gpio_read_config (client, offset); +if (ret <0 ) { + dev_err(&client->dev,(quoted diff) +1
-0+ "failed to read GPIO config\n");
Again, we don't need a linebreak above, but we probably should have one between thedev_err()and the return statement.
Will take care of that.Quoted 9 lines
Quoted 5 lines
Review
Quoted 2 lines
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
(quoted diff) +1
-0+ return ret;(quoted diff) +1
-0+ }
+drivers/hwmon/pmbus/ucd9000.c static int ucd9000_read_byte_data(struct i2c_client *client, int page, int reg) · line 164
Quoted 1 line
drivers/hwmon/pmbus/ucd9000.c +1
-0+return ret &UCD9000_GPIO_CONFIG_STATUS ?1 :0 ;
We can avoid the branch with:
return !!(ret &UCD9000_GPIO_CONFIG_STATUS);
Will update.
Quoted 8 lines
Quoted 3 lines
Interestingly it's not clear from the datasheet whether the status bit contains a relevant value when the pin is in output mode, as there's a separate bit (bit
2) to set the value. Have you tested the behaviour?
I've not tested this in output mode. Up to this point there isn't any means to set direction so, at least for how we're using it currently these are all set to inputs.
Will see how this behaves in output mode.
Do it as you have time - as you point out there's not support for output currently so it's not the end of the world. I just wanted to note the behaviour so we didn't forget about it.
Quoted 28 lines
Quoted 18 lines
Quoted 18 lines
drivers/hwmon/pmbus/ucd9000.c +3
-0+} + +static int ucd9000_gpio_get_direction(struct gpio_chip *gc,Review
Quoted 1 line
(quoted diff) +1
-0+ unsigned int offset)
+{Review
Quoted 2 lines
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
(quoted diff) +1
-0+ struct i2c_client *client = gpiochip_get_data(gc);(quoted diff) +1
-0+ int ret;
+Review
Quoted 6 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
drivers/hwmon/pmbus/ucd9000.c +5
-0+ ret =ucd9000_gpio_read_config (client, offset); +if (ret <0 ) { +dev_err (&client->dev, +"failed to read GPIO config\n" ); +return ret;(quoted diff) +1
-0+ }
+Quoted 10 lines
drivers/hwmon/pmbus/ucd9000.c static int ucd9000_read_byte_data(struct i2c_client *client, int page, int reg) · line 185
Quoted 7 lines
drivers/hwmon/pmbus/ucd9000.c static int ucd9000_read_byte_data(struct i2c_client *client, int page, int reg) · line 180
Quoted 1 line
drivers/hwmon/pmbus/ucd9000.c +1
-0+return ret &UCD9000_GPIO_CONFIG_OUT_ENABLE ?0 :1 ;drivers/hwmon/pmbus/ucd9000.c +5
-0+} + +/* +* Defined so that libgpiod tool can access pin states. No plan to actually +* change pin direction within this driver.
That's not true - the driver is general purpose. Lets not limit it too much to *how you intend to use it* vs the actual hardware capability.
Quoted 2 lines
Understand. Will implement direction switch for next revision in any case.
Great.
Quoted 44 lines
Quoted 35 lines
Quoted 6 lines
+ */
+static int ucd9000_gpio_direction_input(structgpio_chip*gc,Review
Quoted 1 line
(quoted diff) +1
-0+ unsigned int offset)
+{drivers/hwmon/pmbus/ucd9000.c static int ucd9000_read_byte_data(struct i2c_client *client, int page, int reg) · line 190
Quoted 1 line
drivers/hwmon/pmbus/ucd9000.c +1
-0+return 0 ;
I don't think we should do this for several reasons:
1) It's not that hard to implement the functionality
2) It's not true that all GPIOs are inputs with factory defaults
3) There will be uncertainty if custom defaults are programmed
Again from ucd90160.pdf, on page 34:
7.5 Programming
7.5.1 Device Configuration and Programming
From the factory, the device contains the sequencing and monitoring firmware. It is also configured so that all GPOs are high-impedance (except for FPWM/GPIO pins 17-24, which are driven low), with no sequencing or fault-response operation. See Configuration Programming of UCD Devices, available from the Documentation & Help Center that can be selected from the Fusion GUI Help menu, for full UCD90160 configuration details.
The FPWM pins are driven low, implying they are configured for output mode. It's not obvious to me from this whether the other GPIOs are input or output out-of-the-box as the chip supports active and open-drain drive types. It does say "GPOs" in the paragraph above, but I haven't found documentation on which pins are in what state from factory, and I don't know how we're configuring it for production.
Regardless, the driver shouldn't make assumptions about how the device is pre-programmed - without further information there's already ambiguity as to whether the default settings are in play or whether the system integrator hasQuoted 8 lines
configured the chip. We need information from the platform about how the device is/should be managed.
Further, the note in section 10.43 (FBh)GPIO_CONFIG(MFR_SPECIFIC_43) says:
Configuring a pin that is also being used by another function (enable, fan control, and so forth) may likely result in unexpected and unwanted behavior.
Quoted 2 lines
All good points. Will implement direction switching for next submit.
Thanks.
Quoted 17 lines
Quoted 8 lines
This suggests my favourite topic: pinctrl.
Twenty-two of the pins on the UCD90160 are GPIO-capable. Of these 22 pins, 16 of them have alternate functions. See "Table 3. GPIO Pin Configuration Options" on page 22 of ucd90160.pdf. But a quick rundown:
1. 4 PWM pins (GPIOs 1-4, pins 31, 32, 42, 41)
2. 8 FPWM (Fast PWM) pins (GPIOs 5-12, pins 17-24)Quoted 8 lines
3. 4 pins shared with JTAG (GPIOs 19-22, pins 36-39)
Now, in terms of devicetree we'd be breaking things in the future by specifying a GPIO controller that relies on a pinmux in hardware without also specifying the pin controller in the devicetree. This necessitates at least *some* pinctrl capability be baked into the driver as well, so we can point the gpiochip to the relevant pin controller (the same device, but the bindings are general) and map the pin spaces in the devicetree (the gpio and pin spaces are different).
Quoted 4 lines
I'd like to avoid implementing pinctrl for the time being given the limited function we need internally. I do see the advantages as you point out so maybe we can phase this in?
I think it should be possible to do a bare-bones pinctrl implementation that describes no pinmux functions but allows allocation of GPIOs. That way the devicetree won't have to change in a breaking way. See how the GPIO controller defers to the pinctrl node to allocate pins in the Aspeed DTSIs:
https://github.com/openbmc/linux/blob/dev-4.10/Documentation/devicetree/bindings/gpio/gpio.txt#L230 https://github.com/openbmc/linux/blob/dev-4.10/arch/arm/boot/dts/aspeed-g5.dtsi#L271 https://github.com/openbmc/linux/blob/dev-4.10/drivers/pinctrl/aspeed/pinctrl-aspeed-g5.c#L2439 https://github.com/openbmc/linux/blob/dev-4.10/drivers/pinctrl/aspeed/pinctrl-aspeed.c#L453
The implementation should be no-where near as complex as the Aspeed pinmux, and hopefully we only need the GPIO portions of it.
Cheers,
Andrew
Quoted 8 lines
Quoted 7 lines
Quoted 7 lines
drivers/hwmon/pmbus/ucd9000.c +3
-0+} + +static ssize_t ucd9000_clear_logged_faults(struct device *dev,drivers/hwmon/pmbus/ucd9000.c static int ucd9000_read_byte_data(struct i2c_client *client, int page, int reg) · line 195
Quoted 2 lines
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
drivers/hwmon/pmbus/ucd9000.c +1
-0+ struct device_attribute *attr, const char *buf,drivers/hwmon/pmbus/ucd9000.c +1
-0+ size_t count)
+{
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Review
Quoted 2 lines
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
drivers/hwmon/pmbus/ucd9000.c +1
-0+struct i2c_client *client =to_i2c_client (dev);(quoted diff) +1
-0+ int ret;
+Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
drivers/hwmon/pmbus/ucd9000.c static int ucd9000_read_byte_data(struct i2c_client *client, int page, int reg) · line 203
Quoted 4 lines
drivers/hwmon/pmbus/ucd9000.c +4
-0+/* No page set required */ + ret =i2c_smbus_write_byte_data (client,UCD9000_LOGGED_FAULTS ,0 ); +if (ret) { +dev_err (&client->dev,"Failed to clear logged faults\n" );
"> > > > > > > > + return ret;
Quoted 127 lines
Quoted 18 lines
Quoted 18 lines
Review
Quoted 1 line
(quoted diff) +1
-0+ }
+drivers/hwmon/pmbus/ucd9000.c static int ucd9000_read_byte_data(struct i2c_client *client, int page, int reg) · line 207
Quoted 1 line
drivers/hwmon/pmbus/ucd9000.c +1
-0+return count;drivers/hwmon/pmbus/ucd9000.c +3
-0+} + +static DEVICE_ATTR(clear_logged_faults, 0200, NULL,drivers/hwmon/pmbus/ucd9000.c static int ucd9000_read_byte_data(struct i2c_client *client, int page, int reg) · line 211
Quoted 1 line
drivers/hwmon/pmbus/ucd9000.c +1
-0+ ucd9000_clear_logged_faults);drivers/hwmon/pmbus/ucd9000.c +2
-0+ +static struct attribute *ucd9000_attributes[] = {drivers/hwmon/pmbus/ucd9000.c static int ucd9000_read_byte_data(struct i2c_client *client, int page, int reg) · line 215
Quoted 2 lines
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
drivers/hwmon/pmbus/ucd9000.c +1
-0+ &dev_attr_clear_logged_faults.attr,drivers/hwmon/pmbus/ucd9000.c +1
-0+NULL drivers/hwmon/pmbus/ucd9000.c +3
-0+}; + +static const struct attribute_group ucd9000_attr_group = {Quoted 6 lines
drivers/hwmon/pmbus/ucd9000.c
Quoted 4 lines
drivers/hwmon/pmbus/ucd9000.c static int ucd9000_read_byte_data(struct i2c_client *client, int page, int reg) · line 219
Quoted 1 line
drivers/hwmon/pmbus/ucd9000.c +1
-0+ .attrs = ucd9000_attributes,drivers/hwmon/pmbus/ucd9000.c +2
-0+}; +
Joel's made a fair comment here regarding upstream. Please address that.
Next patch set will be sent upstream.Quoted 9 lines
Quoted 9 lines
static int ucd9000_probe(struct
i2c_client*client,Quoted 1 line
const struct
i2c_device_id*id)
{drivers/hwmon/pmbus/ucd9000.c +0
-0@@ -227,7 +327,39 @@ static int ucd9000_probe(struct i2c_client *client, > > > > > | PMBUS_HAVE_FAN34 | PMBUS_HAVE_STATUS_FAN34; > }
Quoted 8 lines
Quoted 5 lines
drivers/hwmon/pmbus/ucd9000.c static int ucd9000_probe(struct i2c_client *client, · line 333
Quoted 5 lines
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
drivers/hwmon/pmbus/ucd9000.c +3
-1-return pmbus_do_probe (client, mid, info); + data->gpio.label ="gpio-ucd9000" ; + data->gpio.get_direction = ucd9000_gpio_get_direction; + data->gpio.direction_input = ucd9000_gpio_direction_input;drivers/hwmon/pmbus/ucd9000.c +1
-0+ data->gpio.get = ucd9000_gpio_get;
I feel like it wouldn't be much effort to make this more complete and specify thedirection_output()andset()callbacks as well.
Right, will do that.Quoted 11 lines
Quoted 4 lines
drivers/hwmon/pmbus/ucd9000.c static int ucd9000_probe(struct i2c_client *client, · line 337
Quoted 4 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
drivers/hwmon/pmbus/ucd9000.c +3
-0+ data->gpio.can_sleep =1 ; + data->gpio.base =-1 ; +if (mid->driver_data == ucd90160)drivers/hwmon/pmbus/ucd9000.c +1
-0+ data->gpio.ngpio =UCD90160_NUM_GPIOS ;
This might need some rework in the future as the other ucd9000 variants want GPIO support.
In fact, what's the behaviour of the other drivers now given we're unconditionally registering a gpiochip?
The other drivers would have agpio_chipthat is available but if they don't use it I'm not thinking this would break them in any way. Would it be better to only register a gpiochip if its a ucd90160 in particular?Quoted 4 lines
Quoted 2 lines
drivers/hwmon/pmbus/ucd9000.c static int ucd9000_probe(struct i2c_client *client, · line 339
Quoted 2 lines
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
drivers/hwmon/pmbus/ucd9000.c +1
-0+ data->gpio.parent = &client->dev;drivers/hwmon/pmbus/ucd9000.c +1
-0+ data->gpio.owner =THIS_MODULE ;
At some point we should think about pinconf as well (push-pull vs open-drain).
Ok, will make a note of that.Quoted 9 lines
Quoted 9 lines
+
Review
Quoted 6 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
drivers/hwmon/pmbus/ucd9000.c +5
-0+ ret =devm_gpiochip_add_data (&client->dev, &data->gpio, client); +if (ret) { + data->gpio.parent =NULL ; +dev_warn (&client->dev,"Count not add gpiochip\n" ); +return ret;(quoted diff) +1
-0+ }
+Quoted 4 lines
Quoted 2 lines
drivers/hwmon/pmbus/ucd9000.c static int ucd9000_probe(struct i2c_client *client, · line 349
Quoted 2 lines
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
drivers/hwmon/pmbus/ucd9000.c +1
-0+ dev_info(&client->dev, "gpios %i...%i\n", data->gpio.base,drivers/hwmon/pmbus/ucd9000.c +1
-0+ data->gpio.base + data->gpio.ngpio - 1);
Let's drop this.
Will remove.
Thanks,
ChrisQuoted 34 lines
Cheers,
AndrewQuoted 29 lines
+
Review
Quoted 4 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
drivers/hwmon/pmbus/ucd9000.c +3
-0+ ret =sysfs_create_group (&client->dev.kobj, + &ucd9000_attr_group); +if (ret <0 )(quoted diff) +1
-0+ return ret;
+drivers/hwmon/pmbus/ucd9000.c static int ucd9000_probe(struct i2c_client *client, · line 356
Quoted 1 line
drivers/hwmon/pmbus/ucd9000.c +1
-0+ return pmbus_do_probe(client, mid, info);drivers/hwmon/pmbus/ucd9000.c +4
-0+} + +static int ucd9000_remove (struct i2c_client *client) +{drivers/hwmon/pmbus/ucd9000.c static int ucd9000_probe(struct i2c_client *client, · line 362
Quoted 2 lines
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
drivers/hwmon/pmbus/ucd9000.c +1
-0+sysfs_remove_group (&client->dev.kobj, &ucd9000_attr_group);drivers/hwmon/pmbus/ucd9000.c +1
-0+return pmbus_do_remove (client);
}
/* This is the driver that will be inserted */drivers/hwmon/pmbus/ucd9000.c +0
-0@@ -236,7 +368,7 @@ static int ucd9000_probe(struct i2c_client *client, > > > > > .name = "ucd9000", > > > > > }, > > > > > .probe = ucd9000_probe, > > > > > - .remove = pmbus_do_remove, > > > > > + .remove = ucd9000_remove, > .id_table = ucd9000_id,
};
PATCH v3 2/2 hwmon: (ucd9000) Add debugfs for mfr_status
In reply to Christopher BosticAdd a debugfs for the mfr_status register. Add individual files for each gpiN_fault and a separate file for the entire contents of the register.
--- drivers/hwmon/pmbus/ucd9000.c | 143 +++++++++++++++++++++++++++++++++++++++++- 1 file changed, 142 insertions(+), 1 deletion(-)
drivers/hwmon/pmbus/ucd9000.c +142 -1
@@ -19,6 +19,7 @@* Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA. */ +#include <linux/debugfs.h> #include <linux/kernel.h> #include <linux/module.h> #include <linux/init.h>
@@ -38,6 +39,7 @@#define UCD9000_FAN_CONFIG_INDEX 0xe7#define UCD9000_FAN_CONFIG 0xe8#define UCD9000_LOGGED_FAULTS 0xea +#define UCD9000_MFR_STATUS 0xf3#define UCD9000_GPIO_SELECT 0xfa#define UCD9000_GPIO_CONFIG 0xfb#define UCD9000_DEVICE_ID 0xfd
@@ -56,16 +58,25 @@#define UCD9000_MON_VOLTAGE_HW 4#define UCD9000_NUM_FAN 4 +#define UCD9000_NAME_SIZE 24#define UCD90160_NUM_GPIOS 26 +#define UCD90160_GPI_COUNT 8 +#define UCD90160_GPI_FAULT_BASE 16struct ucd9000_data {u8 fan_data[UCD9000_NUM_FAN ][I2C_SMBUS_BLOCK_MAX ];struct pmbus_driver_info info;struct gpio_chip gpio; +struct dentry *debugfs;/* debugfs device directory */ };#define to_ucd9000_data (_info) container_of(_info, struct ucd9000_data, info) +struct ucd9000_debugfs_entry { +struct i2c_client *client; +u8 index; +}; +static int ucd9000_get_fan_config (struct i2c_client *client,int fan) {int fan_config =0 ;
@@ -190,6 +201,113 @@ static int ucd9000_gpio_direction_input(struct gpio_chip *gc,return 0 ; } +static struct dentry *ucd9000_debugfs_dir; + +#if IS_ENABLED (CONFIG_DEBUG_FS ) +static int ucd9000_get_mfr_status (struct i2c_client *client,u32 *buffer) +{ +int ret; + + ret =pmbus_set_page (client,0 ); +if (ret <0 ) { +dev_err (&client->dev,"pmbus_set_page failed. rc:%d\n" , ret); +return ret; + } +return i2c_smbus_read_block_data (client,UCD9000_MFR_STATUS , + (u8 *)buffer); +} + +static int ucd9000_debugfs_get_mfr_status_bit (void *data,u64 *val) +{ +struct ucd9000_debugfs_entry *entry = data; +struct i2c_client *client = entry->client; +int nr = entry->index; +u32 buffer; +int ret; + + ret =ucd9000_get_mfr_status (client, &buffer); +if (ret <0 ) { +dev_err (&client->dev, +"Failed to read mfr status. rc:%d\n" , ret); +return ret; + } + + *val = ret &BIT (nr) ?1 :0 ; + +return 0 ; +} +DEFINE_DEBUGFS_ATTRIBUTE (ucd9000_debugfs_ops_mfr_status_bit, + ucd9000_debugfs_get_mfr_status_bit,NULL ,"%1lld\n" ); + +static int ucd9000_debugfs_get_mfr_status_word (void *data,u64 *val) +{ +struct ucd9000_debugfs_entry *entry = data; +struct i2c_client *client = entry->client; +u32 buffer; +int ret; + + ret =ucd9000_get_mfr_status (client, &buffer); +if (ret <0 ) { +dev_err (&client->dev, +"Failed to read mfr status. rc:%d\n" , ret); +return ret; + } + + *val = ret; + +return 0 ; +} +DEFINE_DEBUGFS_ATTRIBUTE (ucd9000_debugfs_ops_mfr_status_word, + ucd9000_debugfs_get_mfr_status_word,NULL ,"%08llx\n" ); + +static int ucd9000_init_debugfs (struct i2c_client *client, +struct ucd9000_data *data) +{ +struct ucd9000_debugfs_entry *entries; +char name[UCD9000_NAME_SIZE ]; +int i; + +if (!ucd9000_debugfs_dir) +return -ENODEV ; + +/* +* Create the debugfs directory for this device. +*/ + data->debugfs =debugfs_create_dir ("mfr_status" , + ucd9000_debugfs_dir); +if (IS_ERR_OR_NULL (data->debugfs)) { + data->debugfs =NULL ; +return -ENODEV ; + } + + entries =devm_kzalloc (&client->dev,sizeof (*entries) * + (UCD90160_GPI_COUNT +1 ) *10 , +GFP_KERNEL ); +if (!entries) +return -ENOMEM ; + +for (i =0 ; i <UCD90160_GPI_COUNT ; i++) { + entries[i].client = client; + entries[i].index =UCD90160_GPI_FAULT_BASE + i; +scnprintf (name,UCD9000_NAME_SIZE ,"gpi%d_alarm" , i+1 ); +debugfs_create_file (name,0444 , data->debugfs, &entries[i], + &ucd9000_debugfs_ops_mfr_status_bit); + } + entries[i].client = client; +scnprintf (name,UCD9000_NAME_SIZE ,"mfr_status" ); +debugfs_create_file (name,0444 , data->debugfs, &entries[i], + &ucd9000_debugfs_ops_mfr_status_word); + +return 0 ; +} +#else +static int ucd9000_init_debugfs (struct i2c_client *client, +struct ucd9000_data *data) +{ +return 0 ; +} +#endif /* IS_ENABLED(CONFIG_DEBUG_FS) */ +static ssize_t ucd9000_clear_logged_faults (struct device *dev,struct device_attribute *attr,const char *buf,size_t count)
@@ -353,12 +471,19 @@ static int ucd9000_probe(struct i2c_client *client,if (ret <0 )return ret; +if (mid->driver_data == ucd90160) { + ret =ucd9000_init_debugfs (client, data); +if (ret <0 ) +dev_warn (&client->dev,"Failed to register debugfs\n" ); + } +return pmbus_do_probe (client, mid, info); }static int ucd9000_remove (struct i2c_client *client) {sysfs_remove_group (&client->dev.kobj, &ucd9000_attr_group); +debugfs_remove_recursive (ucd9000_debugfs_dir);return pmbus_do_remove (client); }
@@ -372,7 +497,23 @@ static int ucd9000_remove(struct i2c_client *client) .id_table = ucd9000_id, }; -module_i2c_driver (ucd9000_driver); +static int __init ucd9000_init (void ) +{ + ucd9000_debugfs_dir =debugfs_create_dir ("ucd9000" ,NULL ); +if (IS_ERR (ucd9000_debugfs_dir)) + ucd9000_debugfs_dir =NULL ; + +return i2c_add_driver (&ucd9000_driver); +} + +static void __exit ucd9000_exit(void) +{ +i2c_del_driver (&ucd9000_driver); +debugfs_remove_recursive (ucd9000_debugfs_dir); +} + +module_init (ucd9000_init); +module_exit (ucd9000_exit);MODULE_AUTHOR ("Guenter Roeck" );MODULE_DESCRIPTION ("PMBus driver for TI UCD90xxx" );
Signature · 1 line
1.8.2.2