On Tue, Mar 13, 2018 at 11:13:04PM +0200, Andy Shevchenko wrote: > On Tue, Mar 13, 2018 at 10:59 PM, Eddie James > <eaja...@linux.vnet.ibm.com> wrote: > > From: Christopher Bostic <cbos...@linux.vnet.ibm.com> > > > > 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