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. > 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 <[email protected]> > --- > 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
signature.asc
Description: PGP signature
