Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Tue, 18 Dec 2018 12:35:40 +0100
From:   Johan Hovold <johan@kernel.org>
To:     Nishad Kamdar <nishadkamdar@gmail.com>
Cc:     Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Johan Hovold <johan@kernel.org>, Alex Elder <elder@kernel.org>,
        Rui Miguel Silva <rmfrfs@gmail.com>,
        greybus-dev@lists.linaro.org, devel@driverdev.osuosl.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH v3 2/3] staging: greybus: arche-apb-ctrl.c: Switch to the
 gpio descriptor interface
Message-ID: <20181218113540.GN20658@localhost>
References: <cover.1542898267.git.nishadkamdar@gmail.com>
 <9ebe2c3909643701e5936da3778bf4fab3e53036.1542898267.git.nishadkamdar@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <9ebe2c3909643701e5936da3778bf4fab3e53036.1542898267.git.nishadkamdar@gmail.com>
User-Agent: Mutt/1.11.1 (2018-12-01)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Thu, Nov 22, 2018 at 10:38:18PM +0530, Nishad Kamdar wrote:
> Use the gpiod interface instead of the deprecated old non-descriptor
> interface.
> 
> Signed-off-by: Nishad Kamdar <nishadkamdar@gmail.com>
> ---
> Changes in v2:
>  - Resolved compilation errors.
> ---
>  drivers/staging/greybus/arche-apb-ctrl.c | 159 +++++++++--------------
>  1 file changed, 65 insertions(+), 94 deletions(-)
> 
> diff --git a/drivers/staging/greybus/arche-apb-ctrl.c b/drivers/staging/greybus/arche-apb-ctrl.c
> index be5ffed90bcf..e887f6aee20b 100644
> --- a/drivers/staging/greybus/arche-apb-ctrl.c
> +++ b/drivers/staging/greybus/arche-apb-ctrl.c
> @@ -8,9 +8,8 @@
>  
>  #include <linux/clk.h>
>  #include <linux/delay.h>
> -#include <linux/gpio.h>
> +#include <linux/gpio/consumer.h>
>  #include <linux/interrupt.h>
> -#include <linux/of_gpio.h>
>  #include <linux/of_irq.h>
>  #include <linux/module.h>
>  #include <linux/pinctrl/consumer.h>
> @@ -24,12 +23,12 @@ static void apb_bootret_deassert(struct device *dev);
>  
>  struct arche_apb_ctrl_drvdata {
>  	/* Control GPIO signals to and from AP <=> AP Bridges */
> -	int resetn_gpio;
> -	int boot_ret_gpio;
> -	int pwroff_gpio;
> -	int wake_in_gpio;
> -	int wake_out_gpio;
> -	int pwrdn_gpio;
> +	struct gpio_desc *resetn;
> +	struct gpio_desc *boot_ret;
> +	struct gpio_desc *pwroff;
> +	struct gpio_desc *wake_in;
> +	struct gpio_desc *wake_out;
> +	struct gpio_desc *pwrdn;
>  
>  	enum arche_platform_state state;
>  	bool init_disabled;
> @@ -37,28 +36,28 @@ struct arche_apb_ctrl_drvdata {
>  	struct regulator *vcore;
>  	struct regulator *vio;
>  
> -	int clk_en_gpio;
> +	struct gpio_desc *clk_en;
>  	struct clk *clk;
>  
>  	struct pinctrl *pinctrl;
>  	struct pinctrl_state *pin_default;
>  
>  	/* V2: SPI Bus control  */
> -	int spi_en_gpio;
> +	struct gpio_desc *spi_en;
>  	bool spi_en_polarity_high;
>  };
>  
>  /*
>   * Note that these low level api's are active high
>   */
> -static inline void deassert_reset(unsigned int gpio)
> +static inline void deassert_reset(struct gpio_desc *gpio)
>  {
> -	gpio_set_value(gpio, 1);
> +	gpiod_set_value(gpio, 1);
>  }
>  
> -static inline void assert_reset(unsigned int gpio)
> +static inline void assert_reset(struct gpio_desc *gpio)
>  {
> -	gpio_set_value(gpio, 0);
> +	gpiod_set_value(gpio, 0);
>  }

As the comment above deassert_reset() suggests, this change will
actually change the semantics of these calls by taking any gpio flags
into account (e.g. ACTIVE_LOW which will invert the signals).

Perhaps you should just use gpiod_set_raw_value() for now, and this can
be addressed later. Alternatively, drop the comment and invert the
polarity here and any users will need to update their device trees.

Either way, mention this in the commit message.

>  /*
> @@ -75,11 +74,11 @@ static int coldboot_seq(struct platform_device *pdev)
>  		return 0;
>  
>  	/* Hold APB in reset state */
> -	assert_reset(apb->resetn_gpio);
> +	assert_reset(apb->resetn);
>  
>  	if (apb->state == ARCHE_PLATFORM_STATE_FW_FLASHING &&
> -	    gpio_is_valid(apb->spi_en_gpio))
> -		devm_gpio_free(dev, apb->spi_en_gpio);
> +	    apb->spi_en)

No need to break the line any more.

> +		devm_gpiod_put(dev, apb->spi_en);
>  
>  	/* Enable power to APB */
>  	if (!IS_ERR(apb->vcore)) {
> @@ -101,13 +100,13 @@ static int coldboot_seq(struct platform_device *pdev)
>  	apb_bootret_deassert(dev);
>  
>  	/* On DB3 clock was not mandatory */
> -	if (gpio_is_valid(apb->clk_en_gpio))
> -		gpio_set_value(apb->clk_en_gpio, 1);
> +	if (apb->clk_en)
> +		gpiod_set_value(apb->clk_en, 1);
>  
>  	usleep_range(100, 200);
>  
>  	/* deassert reset to APB : Active-low signal */
> -	deassert_reset(apb->resetn_gpio);
> +	deassert_reset(apb->resetn);
>  
>  	apb->state = ARCHE_PLATFORM_STATE_ACTIVE;
>  
> @@ -119,6 +118,7 @@ static int fw_flashing_seq(struct platform_device *pdev)
>  	struct device *dev = &pdev->dev;
>  	struct arche_apb_ctrl_drvdata *apb = platform_get_drvdata(pdev);
>  	int ret;
> +	unsigned long flags;
>  
>  	if (apb->init_disabled ||
>  	    apb->state == ARCHE_PLATFORM_STATE_FW_FLASHING)
> @@ -136,25 +136,20 @@ static int fw_flashing_seq(struct platform_device *pdev)
>  		return ret;
>  	}
>  
> -	if (gpio_is_valid(apb->spi_en_gpio)) {

spi_en_gpio is currently optional, so cannot just drop the check.

> -		unsigned long flags;
> +	if (apb->spi_en_polarity_high)
> +		flags = GPIOD_OUT_HIGH;
> +	else
> +		flags = GPIOD_OUT_LOW;

This should probably also be converted to honouring the gpio flags, but
perhaps better to do in a later patch.

>  
> -		if (apb->spi_en_polarity_high)
> -			flags = GPIOF_OUT_INIT_HIGH;
> -		else
> -			flags = GPIOF_OUT_INIT_LOW;
> -
> -		ret = devm_gpio_request_one(dev, apb->spi_en_gpio,
> -					    flags, "apb_spi_en");
> -		if (ret) {
> -			dev_err(dev, "Failed requesting SPI bus en gpio %d\n",
> -				apb->spi_en_gpio);
> -			return ret;
> -		}
> +	apb->spi_en = devm_gpiod_get(dev, "gb,spi-en-gpio", flags);
> +	if (IS_ERR(apb->spi_en)) {
> +		ret = PTR_ERR(apb->spi_en);
> +		dev_err(dev, "Failed requesting SPI bus en GPIO: %d\n", ret);
> +		return ret;
>  	}
>  
>  	/* for flashing device should be in reset state */
> -	assert_reset(apb->resetn_gpio);
> +	assert_reset(apb->resetn);
>  	apb->state = ARCHE_PLATFORM_STATE_FW_FLASHING;
>  
>  	return 0;
> @@ -177,8 +172,8 @@ static int standby_boot_seq(struct platform_device *pdev)
>  		return 0;
>  
>  	if (apb->state == ARCHE_PLATFORM_STATE_FW_FLASHING &&
> -	    gpio_is_valid(apb->spi_en_gpio))
> -		devm_gpio_free(dev, apb->spi_en_gpio);
> +	    apb->spi_en)

No line break. Check this throughout.

> +		devm_gpiod_put(dev, apb->spi_en);
>  
>  	/*
>  	 * As per WDM spec, do nothing

> @@ -404,12 +379,8 @@ static int apb_ctrl_get_devtree_data(struct platform_device *pdev,
>  	}
>  
>  	/* Only applicable for platform >= V2 */
> -	apb->spi_en_gpio = of_get_named_gpio(np, "spi-en-gpio", 0);
> -	if (apb->spi_en_gpio >= 0) {
> -		if (of_property_read_bool(pdev->dev.of_node,
> -					  "spi-en-active-high"))
> -			apb->spi_en_polarity_high = true;
> -	}
> +	if (of_property_read_bool(pdev->dev.of_node, "gb,spi-en-active-high"))
> +		apb->spi_en_polarity_high = true;

Guess it's fine to drop the spi_en check here though.

Johan