Re: [PATCH v3] power: smb347_charger: Use device managed API's

[Sebastian Reichel <sre@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 2888 bytes --]

Hi,

On Sat, Dec 10, 2016 at 04:38:31PM +0530, Atul Raj wrote:
> Using device managed API to make the code simpler
> 
> using devm_gpio_request_one in place of gpio_request_one
> using devm_request_threaded_irq in place of request_threaded_irq
> using devm_power_supply_register in place of power_supply_register
> using gpiod_* API's in place of gpio_* API's
> 
> Signed-off-by: Atul Raj <atul.r@samsung.com>
> ---
> Changes in v2:
>   - using gpiod_* API's in place of gpio_* API's
> 
> Changes in v3
>   - chaning suject as per directory in file path in supply git
>     adding signoff
>     sorry for wrong v2 patch. Kindly ignore v2.
> 
>  drivers/power/smb347-charger.c |   59 ++++++++++++++++------------------------
>  1 file changed, 24 insertions(+), 35 deletions(-)
> 
> diff --git a/drivers/power/smb347-charger.c b/drivers/power/smb347-charger.c
> index 072c518..5b2b5cb 100644
> --- a/drivers/power/smb347-charger.c
> +++ b/drivers/power/smb347-charger.c

Please rebase your patch against a more modern kernel. The supply
subsystem moved to drivers/power/supply.

> @@ -12,7 +12,7 @@
>   */
>  
>  #include <linux/err.h>
> -#include <linux/gpio.h>
> +#include <linux/gpio/consumer.h>
>  #include <linux/kernel.h>
>  #include <linux/module.h>
>  #include <linux/init.h>
> @@ -145,6 +145,7 @@ struct smb347_charger {
>  	bool			mains_online;
>  	bool			usb_online;
>  	bool			charging_enabled;
> +	struct gpio_desc	*irq_gpio;
>  	const struct smb347_charger_platform_data *pdata;
>  };
>  
> @@ -835,21 +836,29 @@ static int smb347_irq_init(struct smb347_charger *smb,
>  			   struct i2c_client *client)
>  {
>  	const struct smb347_charger_platform_data *pdata = smb->pdata;
> -	int ret, irq = gpio_to_irq(pdata->irq_gpio);
> +	int ret, irq;
> +	struct gpio_desc *gpiod;
>  
> -	ret = gpio_request_one(pdata->irq_gpio, GPIOF_IN, client->name);
> -	if (ret < 0)
> +	smb->irq_gpio = gpio_to_desc(pdata->irq_gpio);
> +
> +	irq = gpiod_to_irq(smb->irq_gpio);
> +
> +	gpiod = devm_gpiod_get_optional(&client->dev, NULL, GPIOD_IN);
> +	if (IS_ERR(gpiod)) {
> +		ret = PTR_ERR(gpiod);
>  		goto fail;
> +	}

There seems to be a misunderstanding how to use the gpiod API.
With the gpiod API the gpio description can be removed from the
platform data. Instead gpios are described in the boardcode file
similar to e.g. regulators. There are a few examples in the mainline
tree, just check for board files with "struct gpiod_lookup_table".
Then you get the correct one using

devm_gpiod_get_optional(&client->dev, "gpio-name", GPIOD_IN);

Actually checking the driver: Please just remove all gpio handling.
The boardcode can do all of that and put the correct irq number into
i2c_board_info.irq. This also makes it easier to add ACPI/DT
bindings, which supports doing this transparently.

-- Sebastian

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]