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

[Sebastian Reichel]

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

Attachment: signature.asc
Description: PGP signature