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

[Nishad Kamdar <nishadkamdar@gmail.com>]

On Wed, Jan 09, 2019 at 12:30:43PM +0100, Johan Hovold wrote:
> On Sat, Dec 22, 2018 at 08:22:09PM +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 v4:
> >  - Use gpiod_set_raw_value() for deassert_reset() and
> >    assert_reset() as gpiod_set_value() will change the
> >    sematics of these calls by taking any gpio flags
> >    into account.
> 
> Please also mention this in the commit message (i.e. that we continue to
> ignore gpio flags from device tree for now).
> 
Ok, I'll do that.
> >  - Remove some unnecesssary line breaks.
> >  - Restore 'spi_en' gpio check in fw_flashing_seq()
> >    as it is currently optional.
> > Changes in v3:
> >  - Add this patch in a patchset.
> > Changes in v2:
> >  - Resolved compilation errors.
> > ---
> 
> > @@ -75,11 +74,10 @@ 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);
> > +	if (apb->state == ARCHE_PLATFORM_STATE_FW_FLASHING && apb->spi_en)
> > +		devm_gpiod_put(dev, apb->spi_en);
> >  
> >  	/* Enable power to APB */
> >  	if (!IS_ERR(apb->vcore)) {
> > @@ -101,13 +99,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;
> >  
> > @@ -136,25 +134,25 @@ static int fw_flashing_seq(struct platform_device *pdev)
> >  		return ret;
> >  	}
> >  
> > -	if (gpio_is_valid(apb->spi_en_gpio)) {
> > +	if (apb->spi_en) {
> >  		unsigned long flags;
> >  
> >  		if (apb->spi_en_polarity_high)
> > -			flags = GPIOF_OUT_INIT_HIGH;
> > +			flags = GPIOD_OUT_HIGH;
> >  		else
> > -			flags = GPIOF_OUT_INIT_LOW;
> > +			flags = GPIOD_OUT_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);
> > +		apb->spi_en = devm_gpiod_get(dev, "gb,spi-en-gpio", flags);
> 
> I just noticed that you change the name of the device-tree property here
> (and later in apb_ctrl_get_devtree_data()). How is that expected to
> work without breaking current systems? This will be unavoidable at some
> point, but must not be snuck into a patch like this without any comment.
> Please keep the current names for now.
> 
Ok, I'll do that.
> I do think you need to drop the "-gpio" suffix when requesting the gpio
> though. Please double check to make sure.
> 

Ok, I'll check it.
> Johan

Thanks for the review.

Regards,
Nishad