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 <[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 -- 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/
signature.asc
Description: This is a digitally signed message part
