Message-ID: <20180212145908.fil3dtrdzlycy4ea@dell>
Date:   Mon, 12 Feb 2018 14:59:08 +0000
From:   Lee Jones <lee.jones@...aro.org>
To:     Linus Walleij <linus.walleij@...aro.org>
Cc:     Liam Girdwood <lgirdwood@...il.com>,
        Mark Brown <broonie@...nel.org>, linux-kernel@...r.kernel.org,
        linux-gpio@...r.kernel.org, Milo Kim <milo.kim@...com>
Subject: Re: [PATCH 10/21] regulator: lp8788-ldo: Pass descriptor instead of
 GPIO number

On Mon, 12 Feb 2018, Linus Walleij wrote:

> Instead of passing a global GPIO number, pass a descriptor looked
> up with the standard devm_gpiod_get_index_optional() call.
> 
> This driver has supported passing a LDO enable GPIO for years,
> yet this facility has never been put to use in the upstream kernel.
> If someone desires to put in place GPIO control for the LDOs,
> this can be done by adding a GPIO descriptor table in the MFD
> nexus in drivers/mfd/lp8788.c for the LDO device when spawning the
> MFD children, or using a board file.
> 
> Cc: Milo Kim <milo.kim@...com>
> Cc: Lee Jones <lee.jones@...aro.org>
> Signed-off-by: Linus Walleij <linus.walleij@...aro.org>
> ---
> Lee: would be nice if you could ACK this smallish patch to
> the MFD header.

Fine, but any sign of a conflict and I'll need a PR.

Acked-by: Lee Jones <lee.jones@...aro.org>

> ---
>  drivers/regulator/lp8788-ldo.c | 32 ++++++++++++++++----------------
>  include/linux/mfd/lp8788.h     | 16 ----------------
>  2 files changed, 16 insertions(+), 32 deletions(-)
> 
> diff --git a/drivers/regulator/lp8788-ldo.c b/drivers/regulator/lp8788-ldo.c
> index cbfd35873575..f2347474a106 100644
> --- a/drivers/regulator/lp8788-ldo.c
> +++ b/drivers/regulator/lp8788-ldo.c
> @@ -16,7 +16,7 @@
>  #include <linux/err.h>
>  #include <linux/platform_device.h>
>  #include <linux/regulator/driver.h>
> -#include <linux/gpio.h>
> +#include <linux/gpio/consumer.h>
>  #include <linux/mfd/lp8788.h>
>  
>  /* register address */
> @@ -85,8 +85,6 @@
>  #define LP8788_STARTUP_TIME_S		3
>  
>  #define ENABLE_TIME_USEC		32
> -#define ENABLE				GPIOF_OUT_INIT_HIGH
> -#define DISABLE				GPIOF_OUT_INIT_LOW
>  
>  enum lp8788_ldo_id {
>  	DLDO1,
> @@ -117,7 +115,7 @@ struct lp8788_ldo {
>  	struct lp8788 *lp;
>  	struct regulator_desc *desc;
>  	struct regulator_dev *regulator;
> -	struct lp8788_ldo_enable_pin *en_pin;
> +	struct gpio_desc *ena_gpiod;
>  };
>  
>  /* DLDO 1, 2, 3, 9 voltage table */
> @@ -469,7 +467,6 @@ static int lp8788_config_ldo_enable_mode(struct platform_device *pdev,
>  					enum lp8788_ldo_id id)
>  {
>  	struct lp8788 *lp = ldo->lp;
> -	struct lp8788_platform_data *pdata = lp->pdata;
>  	enum lp8788_ext_ldo_en_id enable_id;
>  	u8 en_mask[] = {
>  		[EN_ALDO1]   = LP8788_EN_SEL_ALDO1_M,
> @@ -504,11 +501,18 @@ static int lp8788_config_ldo_enable_mode(struct platform_device *pdev,
>  		return 0;
>  	}
>  
> -	/* if no platform data for ldo pin, then set default enable mode */
> -	if (!pdata || !pdata->ldo_pin || !pdata->ldo_pin[enable_id])
> +	/* FIXME: check default mode for GPIO here: high or low? */
> +	ldo->ena_gpiod = devm_gpiod_get_index_optional(&pdev->dev,
> +						       "enable",
> +						       enable_id,
> +						       GPIOD_OUT_HIGH);
> +	if (IS_ERR(ldo->ena_gpiod))
> +		return PTR_ERR(ldo->ena_gpiod);
> +
> +	/* if no GPIO for ldo pin, then set default enable mode */
> +	if (!ldo->ena_gpiod)
>  		goto set_default_ldo_enable_mode;
>  
> -	ldo->en_pin = pdata->ldo_pin[enable_id];
>  	return 0;
>  
>  set_default_ldo_enable_mode:
> @@ -533,10 +537,8 @@ static int lp8788_dldo_probe(struct platform_device *pdev)
>  	if (ret)
>  		return ret;
>  
> -	if (ldo->en_pin) {
> -		cfg.ena_gpio = ldo->en_pin->gpio;
> -		cfg.ena_gpio_flags = ldo->en_pin->init_state;
> -	}
> +	if (ldo->ena_gpiod)
> +		cfg.ena_gpiod = ldo->ena_gpiod;
>  
>  	cfg.dev = pdev->dev.parent;
>  	cfg.init_data = lp->pdata ? lp->pdata->dldo_data[id] : NULL;
> @@ -582,10 +584,8 @@ static int lp8788_aldo_probe(struct platform_device *pdev)
>  	if (ret)
>  		return ret;
>  
> -	if (ldo->en_pin) {
> -		cfg.ena_gpio = ldo->en_pin->gpio;
> -		cfg.ena_gpio_flags = ldo->en_pin->init_state;
> -	}
> +	if (ldo->ena_gpiod)
> +		cfg.ena_gpiod = ldo->ena_gpiod;
>  
>  	cfg.dev = pdev->dev.parent;
>  	cfg.init_data = lp->pdata ? lp->pdata->aldo_data[id] : NULL;
> diff --git a/include/linux/mfd/lp8788.h b/include/linux/mfd/lp8788.h
> index 786bf6679a28..2010e0de3e34 100644
> --- a/include/linux/mfd/lp8788.h
> +++ b/include/linux/mfd/lp8788.h
> @@ -181,20 +181,6 @@ struct lp8788_buck2_dvs {
>  	enum lp8788_dvs_sel vsel;
>  };
>  
> -/*
> - * struct lp8788_ldo_enable_pin
> - *
> - *   Basically, all LDOs are enabled through the I2C commands.
> - *   But ALDO 1 ~ 5, 7, DLDO 7, 9, 11 can be enabled by external gpio pins.
> - *
> - * @gpio         : gpio number which is used for enabling ldos
> - * @init_state   : initial gpio state (ex. GPIOF_OUT_INIT_LOW)
> - */
> -struct lp8788_ldo_enable_pin {
> -	int gpio;
> -	int init_state;
> -};
> -
>  /*
>   * struct lp8788_chg_param
>   * @addr         : charging control register address (range : 0x11 ~ 0x1C)
> @@ -288,7 +274,6 @@ struct lp8788_vib_platform_data {
>   * @aldo_data    : regulator initial data for analog ldo
>   * @buck1_dvs    : gpio configurations for buck1 dvs
>   * @buck2_dvs    : gpio configurations for buck2 dvs
> - * @ldo_pin      : gpio configurations for enabling LDOs
>   * @chg_pdata    : platform data for charger driver
>   * @alarm_sel    : rtc alarm selection (1 or 2)
>   * @bl_pdata     : configurable data for backlight driver
> @@ -306,7 +291,6 @@ struct lp8788_platform_data {
>  	struct regulator_init_data *aldo_data[LP8788_NUM_ALDOS];
>  	struct lp8788_buck1_dvs *buck1_dvs;
>  	struct lp8788_buck2_dvs *buck2_dvs;
> -	struct lp8788_ldo_enable_pin *ldo_pin[EN_LDOS_MAX];
>  
>  	/* charger */
>  	struct lp8788_charger_platform_data *chg_pdata;

-- 
Lee Jones
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Powered by blists - more mailing lists