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

[Johan Hovold]

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@xxxxxxxxx>
> ---
> 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).

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

I do think you need to drop the "-gpio" suffix when requesting the gpio
though. Please double check to make sure.

Johan