Re: [PATCH v2 1/2] hwmon: (ucd9000) Add gpio chip interface
Hi Christopher,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on linus/master]
[also build test ERROR on v4.16-rc5 next-20180315]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system]
url:
https://github.com/0day-ci/linux/commits/Eddie-James/hwmon-ucd9000-Add-gpio-and-debugfs-interfaces/20180316-125048
config: x86_64-randconfig-x017-201810 (attached as .config)
compiler: gcc-7 (Debian 7.3.0-1) 7.3.0
reproduce:
# save the attached .config to linux build tree
make ARCH=x86_64
All errors (new ones prefixed by >>):
drivers/hwmon/pmbus/ucd9000.c: In function 'ucd9000_probe':
>> drivers/hwmon/pmbus/ucd9000.c:457:12: error: 'struct gpio_chip' has no
>> member named 'of_node'
data->gpio.of_node = client->dev.of_node;
^
vim +457 drivers/hwmon/pmbus/ucd9000.c
308
309 static int ucd9000_probe(struct i2c_client *client,
310 const struct i2c_device_id *id)
311 {
312 u8 block_buffer[I2C_SMBUS_BLOCK_MAX + 1];
313 struct ucd9000_data *data;
314 struct pmbus_driver_info *info;
315 const struct i2c_device_id *mid;
316 enum chips chip;
317 int i, ret;
318
319 if (!i2c_check_functionality(client->adapter,
320 I2C_FUNC_SMBUS_BYTE_DATA |
321 I2C_FUNC_SMBUS_BLOCK_DATA))
322 return -ENODEV;
323
324 ret = i2c_smbus_read_block_data(client, UCD9000_DEVICE_ID,
325 block_buffer);
326 if (ret < 0) {
327 dev_err(&client->dev, "Failed to read device ID\n");
328 return ret;
329 }
330 block_buffer[ret] = '\0';
331 dev_info(&client->dev, "Device ID %s\n", block_buffer);
332
333 for (mid = ucd9000_id; mid->name[0]; mid++) {
334 if (!strncasecmp(mid->name, block_buffer,
strlen(mid->name)))
335 break;
336 }
337 if (!mid->name[0]) {
338 dev_err(&client->dev, "Unsupported device\n");
339 return -ENODEV;
340 }
341
342 if (client->dev.of_node)
343 chip = (enum
chips)of_device_get_match_data(&client->dev);
344 else
345 chip = id->driver_data;
346
347 if (chip != ucd9000 && chip != mid->driver_data)
348 dev_notice(&client->dev,
349 "Device mismatch: Configured %s, detected
%s\n",
350 id->name, mid->name);
351
352 data = devm_kzalloc(&client->dev, sizeof(struct ucd9000_data),
353 GFP_KERNEL);
354 if (!data)
355 return -ENOMEM;
356 info = &data->info;
357
358 ret = i2c_smbus_read_byte_data(client, UCD9000_NUM_PAGES);
359 if (ret < 0) {
360 dev_err(&client->dev,
361 "Failed to read number of active pages\n");
362 return ret;
363 }
364 info->pages = ret;
365 if (!info->pages) {
366 dev_err(&client->dev, "No pages configured\n");
367 return -ENODEV;
368 }
369
370 /* The internal temperature sensor is always active */
371 info->func[0] = PMBUS_HAVE_TEMP;
372
373 /* Everything else is configurable */
374 ret = i2c_smbus_read_block_data(client, UCD9000_MONITOR_CONFIG,
375 block_buffer);
376 if (ret <= 0) {
377 dev_err(&client->dev, "Failed to read configuration
data\n");
378 return -ENODEV;
379 }
380 for (i = 0; i < ret; i++) {
381 int page = UCD9000_MON_PAGE(block_buffer[i]);
382
383 if (page >= info->pages)
384 continue;
385
386 switch (UCD9000_MON_TYPE(block_buffer[i])) {
387 case UCD9000_MON_VOLTAGE:
388 case UCD9000_MON_VOLTAGE_HW:
389 info->func[page] |= PMBUS_HAVE_VOUT
390| PMBUS_HAVE_STATUS_VOUT;
391 break;
392 case UCD9000_MON_TEMPERATURE:
393 info->func[page] |= PMBUS_HAVE_TEMP2
394| PMBUS_HAVE_STATUS_TEMP;
395 break;
396 case UCD9000_MON_CURRENT:
397 info->func[page] |= PMBUS_HAVE_IOUT
398|
Re: [PATCH v2 1/2] hwmon: (ucd9000) Add gpio chip interface
Hi Christopher,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on linus/master]
[also build test ERROR on v4.16-rc5 next-20180315]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system]
url:
https://github.com/0day-ci/linux/commits/Eddie-James/hwmon-ucd9000-Add-gpio-and-debugfs-interfaces/20180316-125048
config: x86_64-randconfig-x019-201810 (attached as .config)
compiler: gcc-7 (Debian 7.3.0-1) 7.3.0
reproduce:
# save the attached .config to linux build tree
make ARCH=x86_64
All error/warnings (new ones prefixed by >>):
>> drivers/hwmon/pmbus/ucd9000.c:69:19: error: field 'gpio' has incomplete type
struct gpio_chip gpio;
^~~~
drivers/hwmon/pmbus/ucd9000.c: In function 'ucd9000_gpio_get':
>> drivers/hwmon/pmbus/ucd9000.c:184:31: error: implicit declaration of
>> function 'gpiochip_get_data'; did you mean 'gpio_get_value'?
>> [-Werror=implicit-function-declaration]
struct i2c_client *client = gpiochip_get_data(gc);
^
gpio_get_value
>> drivers/hwmon/pmbus/ucd9000.c:184:31: warning: initialization makes pointer
>> from integer without a cast [-Wint-conversion]
drivers/hwmon/pmbus/ucd9000.c: In function 'ucd9000_gpio_set':
drivers/hwmon/pmbus/ucd9000.c:197:30: warning: initialization makes pointer
from integer without a cast [-Wint-conversion]
struct i2c_client *client = gpiochip_get_data(gc);
^
drivers/hwmon/pmbus/ucd9000.c: In function 'ucd9000_gpio_get_direction':
drivers/hwmon/pmbus/ucd9000.c:240:30: warning: initialization makes pointer
from integer without a cast [-Wint-conversion]
struct i2c_client *client = gpiochip_get_data(gc);
^
drivers/hwmon/pmbus/ucd9000.c: In function 'ucd9000_gpio_set_direction':
drivers/hwmon/pmbus/ucd9000.c:254:30: warning: initialization makes pointer
from integer without a cast [-Wint-conversion]
struct i2c_client *client = gpiochip_get_data(gc);
^
drivers/hwmon/pmbus/ucd9000.c: In function 'ucd9000_probe':
>> drivers/hwmon/pmbus/ucd9000.c:460:9: error: implicit declaration of function
>> 'devm_gpiochip_add_data'; did you mean 'devm_gpio_request'?
>> [-Werror=implicit-function-declaration]
ret = devm_gpiochip_add_data(&client->dev, &data->gpio,
^~
devm_gpio_request
cc1: some warnings being treated as errors
vim +/gpio +69 drivers/hwmon/pmbus/ucd9000.c
65
66 struct ucd9000_data {
67 u8 fan_data[UCD9000_NUM_FAN][I2C_SMBUS_BLOCK_MAX];
68 struct pmbus_driver_info info;
> 69 struct gpio_chip gpio;
70 };
71 #define to_ucd9000_data(_info) container_of(_info, struct ucd9000_data,
info)
72
73 static int ucd9000_get_fan_config(struct i2c_client *client, int fan)
74 {
75 int fan_config = 0;
76 struct ucd9000_data *data
77= to_ucd9000_data(pmbus_get_driver_info(client));
78
79 if (data->fan_data[fan][3] & 1)
80 fan_config |= PB_FAN_2_INSTALLED; /* Use lower bit
position */
81
82 /* Pulses/revolution */
83 fan_config |= (data->fan_data[fan][3] & 0x06) >> 1;
84
85 return fan_config;
86 }
87
88 static int ucd9000_read_byte_data(struct i2c_client *client, int page,
int reg)
89 {
90 int ret = 0;
91 int fan_config;
92
93 switch (reg) {
94 case PMBUS_FAN_CONFIG_12:
95 if (page > 0)
96 return -ENXIO;
97
98 ret = ucd9000_get_fan_config(client, 0);
99 if (ret < 0)
100 return ret;
101 fan_config = ret << 4;
102 ret = ucd9000_get_fan_config(client, 1);
103 if (ret < 0)
104 return ret;
105 fan_config |= ret;
106 ret = fan_config;
107 break;
108 case PMBUS_FAN_CONFIG_34:
109 if (page > 0)
110 return -ENXIO;
111
112 ret = ucd9000_get_fan_config(client, 2);
113 if (ret < 0)
114 return ret;
115 fan_config = ret << 4;
116 ret = ucd9000_get_fan_config(client, 3);
117 if (ret < 0)
118 return ret;
119 fan_config |= ret;
120 ret = fan_config;
121 break;
122 default:
123 ret = -ENODATA;
124
Re: [PATCH v2 1/2] hwmon: (ucd9000) Add gpio chip interface
On 03/14/2018 01:55 PM, Guenter Roeck wrote:
On Tue, Mar 13, 2018 at 11:13:04PM +0200, Andy Shevchenko wrote:
On Tue, Mar 13, 2018 at 10:59 PM, Eddie James
wrote:
From: Christopher Bostic
Add a struct gpio_chip and define some methods so that this device's
I/O can be accessed via /sys/class/gpio.
+ /*
+* Note:
+*
+* Pinmux support has not been added to the new gpio_chip.
+* This support should be added when possible given the mux
+* behavior of these IO devices.
+*/
+ data->gpio.label = (const char *)&client->name;
Hmm... Why do you need this casting?
+ data->gpio.get_direction = ucd9000_gpio_get_direction;
+ data->gpio.direction_input = ucd9000_gpio_direction_input;
+ data->gpio.direction_output = ucd9000_gpio_direction_output;
+ data->gpio.get = ucd9000_gpio_get;
+ data->gpio.set = ucd9000_gpio_set;
+ data->gpio.can_sleep = 1;
Isn't it type of boolean?
You are right.
+ data->gpio.base = -1;
+ data->gpio.parent = &client->dev;
+ data->gpio.owner = THIS_MODULE;
+ data->gpio.of_node = client->dev.of_node;
I think GPIO core does this for you.
Same here. I checked and found that it also sets the owner, so maybe this can be
dropped as well.
Ah true. Especially since there is a TODO: remove chip->owner in the core.
+ if (data->gpio.ngpio) {
Hmm...
I would rather reorganize the above part to a separate helper, like
static int ..._probe_gpio()
{
...
switch () {
default:
return 0; /* GPIO part is optional */
}
return 0;
}
ret = _probe_gpio();
if (ret)
dev_warn();
I am neutral to positiva on that.
Might as well add the call to devm_gpiochip_add_data()
into that function as well.
Sure. I can do a v4.
Thanks,
Eddie
+ ret = devm_gpiochip_add_data(&client->dev, &data->gpio,
+client);
+ if (ret)
+ dev_warn(&client->dev, "Could not add gpiochip: %d\n",
+ret);
+ }
+
return pmbus_do_probe(client, mid, info);
}
--
1.8.3.1
--
With Best Regards,
Andy Shevchenko
Re: [PATCH v2 1/2] hwmon: (ucd9000) Add gpio chip interface
On Tue, Mar 13, 2018 at 11:13:04PM +0200, Andy Shevchenko wrote:
> On Tue, Mar 13, 2018 at 10:59 PM, Eddie James
> wrote:
> > From: Christopher Bostic
> >
> > Add a struct gpio_chip and define some methods so that this device's
> > I/O can be accessed via /sys/class/gpio.
>
> > + /*
> > +* Note:
> > +*
> > +* Pinmux support has not been added to the new gpio_chip.
> > +* This support should be added when possible given the mux
> > +* behavior of these IO devices.
> > +*/
>
> > + data->gpio.label = (const char *)&client->name;
>
> Hmm... Why do you need this casting?
>
> > + data->gpio.get_direction = ucd9000_gpio_get_direction;
> > + data->gpio.direction_input = ucd9000_gpio_direction_input;
> > + data->gpio.direction_output = ucd9000_gpio_direction_output;
> > + data->gpio.get = ucd9000_gpio_get;
> > + data->gpio.set = ucd9000_gpio_set;
>
> > + data->gpio.can_sleep = 1;
>
> Isn't it type of boolean?
>
You are right.
> > + data->gpio.base = -1;
>
> > + data->gpio.parent = &client->dev;
> > + data->gpio.owner = THIS_MODULE;
>
> > + data->gpio.of_node = client->dev.of_node;
>
> I think GPIO core does this for you.
>
Same here. I checked and found that it also sets the owner, so maybe this can be
dropped as well.
> > + if (data->gpio.ngpio) {
>
> Hmm...
>
> I would rather reorganize the above part to a separate helper, like
>
> static int ..._probe_gpio()
> {
> ...
> switch () {
>default:
> return 0; /* GPIO part is optional */
> }
> return 0;
> }
>
> ret = _probe_gpio();
> if (ret)
> dev_warn();
>
I am neutral to positiva on that.
Might as well add the call to devm_gpiochip_add_data()
into that function as well.
> > + ret = devm_gpiochip_add_data(&client->dev, &data->gpio,
> > +client);
> > + if (ret)
> > + dev_warn(&client->dev, "Could not add gpiochip:
> > %d\n",
> > +ret);
> > + }
> > +
> > return pmbus_do_probe(client, mid, info);
> > }
> >
> > --
> > 1.8.3.1
> >
>
>
>
> --
> With Best Regards,
> Andy Shevchenko
Re: [PATCH v2 1/2] hwmon: (ucd9000) Add gpio chip interface
On Tue, Mar 13, 2018 at 10:59 PM, Eddie James
wrote:
> From: Christopher Bostic
>
> Add a struct gpio_chip and define some methods so that this device's
> I/O can be accessed via /sys/class/gpio.
> + /*
> +* Note:
> +*
> +* Pinmux support has not been added to the new gpio_chip.
> +* This support should be added when possible given the mux
> +* behavior of these IO devices.
> +*/
> + data->gpio.label = (const char *)&client->name;
Hmm... Why do you need this casting?
> + data->gpio.get_direction = ucd9000_gpio_get_direction;
> + data->gpio.direction_input = ucd9000_gpio_direction_input;
> + data->gpio.direction_output = ucd9000_gpio_direction_output;
> + data->gpio.get = ucd9000_gpio_get;
> + data->gpio.set = ucd9000_gpio_set;
> + data->gpio.can_sleep = 1;
Isn't it type of boolean?
> + data->gpio.base = -1;
> + data->gpio.parent = &client->dev;
> + data->gpio.owner = THIS_MODULE;
> + data->gpio.of_node = client->dev.of_node;
I think GPIO core does this for you.
> + if (data->gpio.ngpio) {
Hmm...
I would rather reorganize the above part to a separate helper, like
static int ..._probe_gpio()
{
...
switch () {
default:
return 0; /* GPIO part is optional */
}
return 0;
}
ret = _probe_gpio();
if (ret)
dev_warn();
> + ret = devm_gpiochip_add_data(&client->dev, &data->gpio,
> +client);
> + if (ret)
> + dev_warn(&client->dev, "Could not add gpiochip: %d\n",
> +ret);
> + }
> +
> return pmbus_do_probe(client, mid, info);
> }
>
> --
> 1.8.3.1
>
--
With Best Regards,
Andy Shevchenko
[PATCH v2 1/2] hwmon: (ucd9000) Add gpio chip interface
From: Christopher Bostic
Add a struct gpio_chip and define some methods so that this device's
I/O can be accessed via /sys/class/gpio.
Signed-off-by: Christopher Bostic
Signed-off-by: Andrew Jeffery
Signed-off-by: Eddie James
---
drivers/hwmon/pmbus/ucd9000.c | 201 ++
1 file changed, 201 insertions(+)
diff --git a/drivers/hwmon/pmbus/ucd9000.c b/drivers/hwmon/pmbus/ucd9000.c
index b74dbec..023fb9e 100644
--- a/drivers/hwmon/pmbus/ucd9000.c
+++ b/drivers/hwmon/pmbus/ucd9000.c
@@ -27,6 +27,7 @@
#include
#include
#include
+#include
#include "pmbus.h"
enum chips { ucd9000, ucd90120, ucd90124, ucd90160, ucd9090, ucd90910 };
@@ -35,8 +36,18 @@
#define UCD9000_NUM_PAGES 0xd6
#define UCD9000_FAN_CONFIG_INDEX 0xe7
#define UCD9000_FAN_CONFIG 0xe8
+#define UCD9000_GPIO_SELECT0xfa
+#define UCD9000_GPIO_CONFIG0xfb
#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_OUT_VALUE BIT(2)
+#define UCD9000_GPIO_CONFIG_STATUS BIT(3)
+#define UCD9000_GPIO_INPUT 0
+#define UCD9000_GPIO_OUTPUT1
+
#define UCD9000_MON_TYPE(x)(((x) >> 5) & 0x07)
#define UCD9000_MON_PAGE(x)((x) & 0x0f)
@@ -47,9 +58,15 @@
#define UCD9000_NUM_FAN4
+#define UCD9000_GPIO_NAME_LEN 16
+#define UCD9090_NUM_GPIOS 23
+#define UCD901XX_NUM_GPIOS 26
+#define UCD90910_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)
@@ -149,6 +166,146 @@ static int ucd9000_read_byte_data(struct i2c_client
*client, int page, int reg)
};
MODULE_DEVICE_TABLE(of, ucd9000_of_match);
+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)
+ 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)
+ return ret;
+
+ return !!(ret & UCD9000_GPIO_CONFIG_STATUS);
+}
+
+static void ucd9000_gpio_set(struct gpio_chip *gc, unsigned int offset,
+int value)
+{
+ struct i2c_client *client = gpiochip_get_data(gc);
+ int ret;
+
+ ret = ucd9000_gpio_read_config(client, offset);
+ if (ret < 0) {
+ dev_dbg(&client->dev, "failed to read GPIO %d config: %d\n",
+ offset, ret);
+ return;
+ }
+
+ if (value) {
+ if (ret & UCD9000_GPIO_CONFIG_STATUS)
+ return;
+
+ ret |= UCD9000_GPIO_CONFIG_STATUS;
+ } else {
+ if (!(ret & UCD9000_GPIO_CONFIG_STATUS))
+ return;
+
+ ret &= ~UCD9000_GPIO_CONFIG_STATUS;
+ }
+
+ ret |= UCD9000_GPIO_CONFIG_ENABLE;
+
+ /* Page set not required */
+ ret = i2c_smbus_write_byte_data(client, UCD9000_GPIO_CONFIG, ret);
+ if (ret < 0) {
+ dev_dbg(&client->dev, "Failed to write GPIO %d config: %d\n",
+ offset, ret);
+ return;
+ }
+
+ ret &= ~UCD9000_GPIO_CONFIG_ENABLE;
+
+ ret = i2c_smbus_write_byte_data(client, UCD9000_GPIO_CONFIG, ret);
+ if (ret < 0)
+ dev_dbg(&client->dev, "Failed to write GPIO %d config: %d\n",
+ offset, ret);
+}
+
+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)
+ return ret;
+
+ return !(ret & UCD9000_GPIO_CONFIG_OUT_ENABLE);
+}
+
+static int ucd9000_gpio_set_direction(struct gpio_chip *gc,
+ unsigned int offset, bool direction_out,
+ int requested_out)
+{
+ struct i2c_client *client = gpiochip_get_data(gc);
+ int ret, config, out_val;
+
+ ret = ucd9000_gpio_read_config(client, offset);
+ if (ret < 0)
+ return ret;
+
+ if (direction_out) {
+ out_val = requested_out ? UCD9000_GPIO_CONFIG_OUT_VALUE : 0;
+
+ if (ret & UCD9000_GPIO_CONFIG_OUT_ENABLE) {
+ if ((ret & UC
