Hi, Le vendredi 02 septembre 2016 à 17:27 +0200, Sebastian Reichel a écrit : > Hi Paul, > > This looks mostly fine now. I have a few more comments, that I > missed last time:
Thanks for the review and for the useful suggestions. I have applied them to v4. > On Thu, Sep 01, 2016 at 11:27:00PM +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. > > > > While at it, this switches the driver over to gpio consumer. > > > > Signed-off-by: Paul Kocialkowski <cont...@paulk.fr> > > --- > > drivers/power/bq24735-charger.c | 42 +++++++++++++------------------- > > --- > > include/linux/power/bq24735-charger.h | 4 +--- > > 2 files changed, 17 insertions(+), 29 deletions(-) > > Please rebase next version to power-supply's for-next branch: > > https://git.kernel.org/cgit/linux/kernel/git/sre/linux-power-supply.git/log/?h > =for-next > > > > > diff --git a/drivers/power/bq24735-charger.c b/drivers/power/bq24735- > > charger.c > > index dc460bb..7f9b305 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> > > No need for <linux/gpio.h>, all gpiod bits used by you are in > <linux/gpio/consumer.h> > > > > > +#include <linux/gpio/consumer.h> > > #include <linux/power_supply.h> > > #include <linux/slab.h> > > > > @@ -178,11 +179,9 @@ static int bq24735_config_charger(struct bq24735 > > *charger) > > static bool bq24735_charger_is_present(struct bq24735 *charger) > > { > > struct bq24735_platform *pdata = charger->pdata; > > - int ret; > > > > - if (pdata->status_gpio_valid) { > > - ret = gpio_get_value_cansleep(pdata->status_gpio); > > - return ret ^= pdata->status_gpio_active_low == 0; > > + if (pdata->status_gpio) { > > + return !gpiod_get_value_cansleep(pdata->status_gpio); > > } else { > > int ac = 0; > > > > @@ -308,7 +307,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 +315,13 @@ 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_optional(&client->dev, > > + "ti,ac-detect", GPIOD_IN); > > + if (IS_ERR(pdata->status_gpio)) { > > + ret = PTR_ERR(pdata->status_gpio); > > + dev_err(&client->dev, "Getting gpio failed: %d\n", ret); > > + return (struct bq24735_platform *) pdata->status_gpio; > > + } > > > > ret = of_property_read_u32(np, "ti,charge-current", &val); > > if (!ret) > > @@ -357,8 +357,11 @@ static int bq24735_charger_probe(struct i2c_client > > *client, > > charger->charging = true; > > charger->pdata = client->dev.platform_data; > > > > - if (IS_ENABLED(CONFIG_OF) && !charger->pdata && client- > > >dev.of_node) > > + if (IS_ENABLED(CONFIG_OF) && !charger->pdata && client- > > >dev.of_node) { > > charger->pdata = bq24735_parse_dt_data(client); > > + if (IS_ERR(charger->pdata)) > > + return PTR_ERR(charger->pdata); > > + } > > > > if (!charger->pdata) { > > dev_err(&client->dev, "no platform data provided\n"); > > @@ -396,20 +399,7 @@ 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; > > - } > > Do the devm_gpiod_get_optional() call here and store the result in > "struct bq24735" instead of in pdata. It's not DT specific. The > gpiod interface can also get it's data from boardcode (or acpi, if > that becomes important at some point). > > (Check Documentation/gpio/board.txt for details) > > > > > - > > - if (!charger->pdata->status_gpio_valid > > + if (!charger->pdata->status_gpio > > || bq24735_charger_is_present(charger)) { > > ret = bq24735_read_word(client, BQ24735_MANUFACTURER_ID); > > if (ret < 0) { > > diff --git a/include/linux/power/bq24735-charger.h > > b/include/linux/power/bq24735-charger.h > > index 6b750c1a..afdb3704 100644 > > --- a/include/linux/power/bq24735-charger.h > > +++ b/include/linux/power/bq24735-charger.h > > @@ -28,9 +28,7 @@ struct bq24735_platform { > > > > const char *name; > > > > - int status_gpio; > > - int status_gpio_active_low; > > - bool status_gpio_valid; > > + struct gpio_desc *status_gpio; > > Move the gpio_desc into the private "struct bq24735" (but keep > dropping all gpio bits from the bq24735_platform). > > -- 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
