lkml.org 
[lkml]   [2016]   [Sep]   [1]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH v2] power: bq24735-charger: Request status GPIO with initial input setup
From
Date
Le mardi 30 août 2016 à 01:32 +0200, Sebastian Reichel a écrit :
> Hi,
>
> On Mon, Aug 29, 2016 at 08:15:03PM +0200, Paul Kocialkowski wrote:
> >
> > This requests the status GPIO with initial input setup. it is required
> > to read the GPIO status at probe time and thus correctly avoid sending
> > i2c messages when AC is not plugged.
> >
> > When requesting the GPIO without initial input setup, it always reads 0
> > which causes probe to fail as it assumes the charger is connected, sends
> > i2c messages and fails.
>
> That looks mostly fine. I have some more comments, though.

Thanks for the review, v3 sent. Feel free to add your Signed-off-by line since
your suggestions heavily influenced v3!

> > While at it, this switches the driver over to devm and gpio consumer.
>
> NIT: the driver was already using devm previously
>
> >
> > Signed-off-by: Paul Kocialkowski <contact@paulk.fr>
> > ---
> >  drivers/power/bq24735-charger.c       | 29 +++++++----------------------
> >  include/linux/power/bq24735-charger.h |  3 +--
> >  2 files changed, 8 insertions(+), 24 deletions(-)
> >
> > diff --git a/drivers/power/bq24735-charger.c b/drivers/power/bq24735-
> > charger.c
> > index dc460bb..5744a88 100644
> > --- a/drivers/power/bq24735-charger.c
> > +++ b/drivers/power/bq24735-charger.c
> > @@ -25,7 +25,8 @@
> >  #include <linux/kernel.h>
> >  #include <linux/module.h>
> >  #include <linux/of.h>
> > -#include <linux/of_gpio.h>
> > +#include <linux/gpio.h>
> > +#include <linux/gpio/consumer.h>
> >  #include <linux/power_supply.h>
> >  #include <linux/slab.h>
> >  
> > @@ -181,8 +182,8 @@ static bool bq24735_charger_is_present(struct bq24735
> > *charger)
> >   int ret;
> >  
> >   if (pdata->status_gpio_valid) {
> > - ret = gpio_get_value_cansleep(pdata->status_gpio);
> > - return ret ^= pdata->status_gpio_active_low == 0;
> > + ret = gpiod_get_value_cansleep(pdata->status_gpio);
> > + return ret ^= gpiod_is_active_low(pdata->status_gpio) == 0;
>
> This looks fishy. The gpiod API outputs converted values already (if
> one does not use the raw variant). I think you want to do:
>
> return !gpiod_get_value_cansleep(pdata->status_gpio);
>
> >
> >   } else {
> >   int ac = 0;
> >  
> > @@ -308,7 +309,6 @@ static struct bq24735_platform
> > *bq24735_parse_dt_data(struct i2c_client *client)
> >   struct device_node *np = client->dev.of_node;
> >   u32 val;
> >   int ret;
> > - enum of_gpio_flags flags;
> >  
> >   pdata = devm_kzalloc(&client->dev, sizeof(*pdata), GFP_KERNEL);
> >   if (!pdata) {
> > @@ -317,11 +317,9 @@ static struct bq24735_platform
> > *bq24735_parse_dt_data(struct i2c_client *client)
> >   return NULL;
> >   }
> >  
> > - pdata->status_gpio = of_get_named_gpio_flags(np, "ti,ac-detect-
> > gpios",
> > -      0, &flags);
> > -
> > - if (flags & OF_GPIO_ACTIVE_LOW)
> > - pdata->status_gpio_active_low = 1;
> > + pdata->status_gpio = devm_gpiod_get(&client->dev, "ti,ac-detect",
> > + GPIOD_IN);
> > + pdata->status_gpio_valid = !IS_ERR(pdata->status_gpio);
>
> Just use devm_gpiod_get_optional(). Then instead of
> checking if (pdata->status_gpio_valid) you do
> if (pdata->status_gpio).
>
> Also you should check for EPROBE_DEFER. With the
> _optional() change you can just do
>
> if (IS_ERR(pdata->status_gpio)) {
>     ret = PTR_ERR(pdata->status_gpio);
>     dev_err(&client->dev, "Could not get gpio: %d\n", ret);
>     return ret;
> }
>
> >
> >   ret = of_property_read_u32(np, "ti,charge-current", &val);
> >   if (!ret)
> > @@ -396,19 +394,6 @@ static int bq24735_charger_probe(struct i2c_client
> > *client,
> >  
> >   i2c_set_clientdata(client, charger);
> >  
> > - if (gpio_is_valid(charger->pdata->status_gpio)) {
> > - ret = devm_gpio_request(&client->dev,
> > - charger->pdata->status_gpio,
> > - name);
> > - if (ret) {
> > - dev_err(&client->dev,
> > - "Failed GPIO request for GPIO %d: %d\n",
> > - charger->pdata->status_gpio, ret);
> > - }
> > -
> > - charger->pdata->status_gpio_valid = !ret;
> > - }
> > -
> >   if (!charger->pdata->status_gpio_valid
> >       || bq24735_charger_is_present(charger)) {
> >   ret = bq24735_read_word(client, BQ24735_MANUFACTURER_ID);
> > diff --git a/include/linux/power/bq24735-charger.h
> > b/include/linux/power/bq24735-charger.h
> > index 6b750c1a..bbc284e 100644
> > --- a/include/linux/power/bq24735-charger.h
> > +++ b/include/linux/power/bq24735-charger.h
> > @@ -28,8 +28,7 @@ struct bq24735_platform {
> >  
> >   const char *name;
> >  
> > - int status_gpio;
> > - int status_gpio_active_low;
> > + struct gpio_desc *status_gpio;
> >   bool status_gpio_valid;
> >  
> >   bool ext_control;
>
> Otherwise looks fine.
>
> -- Sebastian
--
Paul Kocialkowski, developer of low-level free software for embedded devices

Website: https://www.paulk.fr/
Coding blog: https://code.paulk.fr/
Git repositories: https://git.paulk.fr/ https://git.code.paulk.fr/[unhandled content-type:application/pgp-signature]
\
 
 \ /
  Last update: 2016-09-17 09:58    [W:0.050 / U:0.192 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site