Re: [PATCH v3 2/3] staging: greybus: arche-apb-ctrl.c: Switch to the gpio descriptor interface

[Nishad Kamdar <nishadkamdar@gmail.com>]

On Tue, Dec 18, 2018 at 12:35:40PM +0100, Johan Hovold wrote:
> 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.
> 
Ok, I'll use the gpiod_set_raw_value().

> >  /*
> > @@ -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.
> 
Ok.

> > +		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.
>
Ok, I'll restore it.

> > -		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.
> 
Ok.

> >  
> > -		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.
>
Yes.

> > +		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.
>
Ok.
Thanks for the review.
regards,
Nishad

> Johan