Re: [PATCH v3] power: bq24735-charger: Request status GPIO with initial input setup

From: Paul Kocialkowski
Date: Fri Sep 02 2016 - 18:11:41 EST


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 <contact@xxxxxxxx>
> > ---
> > Â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/

Attachment: signature.asc
Description: This is a digitally signed message part