Re: [PATCH v2 1/2] hwmon: (ucd9000) Add gpio chip interface

2018-03-16 Thread kbuild test robot 
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(>dev, "Failed to read device ID\n");
   328  return ret;
   329  }
   330  block_buffer[ret] = '\0';
   331  dev_info(>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(>dev, "Unsupported device\n");
   339  return -ENODEV;
   340  }
   341  
   342  if (client->dev.of_node)
   343  chip = (enum 
chips)of_device_get_match_data(>dev);
   344  else
   345  chip = id->driver_data;
   346  
   347  if (chip != ucd9000 && chip != mid->driver_data)
   348  dev_notice(>dev,
   349 "Device mismatch: Configured %s, detected 
%s\n",
   350 id->name, mid->name);
   351  
   352  data = devm_kzalloc(>dev, sizeof(struct ucd9000_data),
   353  GFP_KERNEL);
   354  if (!data)
   355  return -ENOMEM;
   356  info = >info;
   357  
   358  ret = i2c_smbus_read_byte_data(client, UCD9000_NUM_PAGES);
   359  if (ret < 0) {
   360  dev_err(>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(>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(>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| PMBUS_HAVE_STATUS_IOUT;
   399  break;
   400

Re: [PATCH v2 1/2] hwmon: (ucd9000) Add gpio chip interface

2018-03-15 Thread kbuild test robot 
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(>dev, >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

2018-03-14 Thread Eddie James 



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 *)>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 = >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(>dev, >gpio,
+client);
+   if (ret)
+   dev_warn(>dev, "Could not add gpiochip: %d\n",
+ret);
+   }
+
 return pmbus_do_probe(client, mid, info);
  }

--
1.8.3.1




--
With Best Regards,
Andy Shevchenko


--
To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 1/2] hwmon: (ucd9000) Add gpio chip interface

2018-03-14 Thread Guenter Roeck 
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 *)>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 = >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(>dev, >gpio,
> > +client);
> > +   if (ret)
> > +   dev_warn(>dev, "Could not add gpiochip: 
> > %d\n",
> > +ret);
> > +   }
> > +
> > return pmbus_do_probe(client, mid, info);
> >  }
> >
> > --
> > 1.8.3.1
> >
> 
> 
> 
> -- 
> With Best Regards,
> Andy Shevchenko
--
To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 1/2] hwmon: (ucd9000) Add gpio chip interface

2018-03-13 Thread Andy Shevchenko 
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 *)>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 = >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(>dev, >gpio,
> +client);
> +   if (ret)
> +   dev_warn(>dev, "Could not add gpiochip: %d\n",
> +ret);
> +   }
> +
> return pmbus_do_probe(client, mid, info);
>  }
>
> --
> 1.8.3.1
>



-- 
With Best Regards,
Andy Shevchenko
--
To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html