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

[Johan Hovold]

On Thu, Jan 10, 2019 at 11:21:04PM +0530, Nishad Kamdar wrote:
> Use the gpiod interface instead of the deprecated old non-descriptor
> interface while continuing to ignore gpio flags from device tree in
> functions "deassert_reset()" and "assert_reset()" for now.
> 
> Signed-off-by: Nishad Kamdar <nishadkamdar@xxxxxxxxx>
> ---
> Changes in v5:
>  - Change the commit message.
>  - Restore the names of the gpio device-tree properties without
>    the "-gpio" suffix.
> 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.
>  - 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.
> ---

Also looks good now. You can add my

Reviewed-by: Johan Hovold <johan@xxxxxxxxxx>

Found one really minor nit below, which doesn't really need to be fixed,
but since you may need to update the third patch, you might as well
consider this too.

>  	/* It's not mandatory to support power management interface */
> -	apb->pwroff_gpio = of_get_named_gpio(np, "pwr-off-gpios", 0);
> -	if (apb->pwroff_gpio < 0) {
> -		dev_err(dev, "failed to get power off gpio\n");
> -		return apb->pwroff_gpio;
> -	}
> -	ret = devm_gpio_request_one(dev, apb->pwroff_gpio,
> -				    GPIOF_IN, "pwroff_n");
> -	if (ret) {
> -		dev_err(dev, "Failed requesting pwroff_n gpio %d\n",
> -			apb->pwroff_gpio);
> +	apb->pwroff = devm_gpiod_get_optional(dev, "pwr-off",
> +					      GPIOD_IN);

Looks like you don't need to break the above statement any more either.

> +	if (IS_ERR(apb->pwroff)) {
> +		ret = PTR_ERR(apb->pwroff);
> +		dev_err(dev, "Failed requesting pwroff_n GPIO: %d\n", ret);
>  		return ret;
>  	}
>  
>  	/* Do not make clock mandatory as of now (for DB3) */
> -	apb->clk_en_gpio = of_get_named_gpio(np, "clock-en-gpio", 0);
> -	if (apb->clk_en_gpio < 0) {
> -		dev_warn(dev, "failed to get clock en gpio\n");
> -	} else if (gpio_is_valid(apb->clk_en_gpio)) {
> -		ret = devm_gpio_request_one(dev, apb->clk_en_gpio,
> -					    GPIOF_OUT_INIT_LOW, "apb_clk_en");
> -		if (ret) {
> -			dev_warn(dev, "Failed requesting APB clock en gpio %d\n",
> -				 apb->clk_en_gpio);
> -			return ret;
> -		}
> +	apb->clk_en = devm_gpiod_get_optional(dev, "clock-en",
> +					      GPIOD_OUT_LOW);

Same here?

> +	if (IS_ERR(apb->clk_en)) {
> +		ret = PTR_ERR(apb->clk_en);
> +		dev_err(dev, "Failed requesting APB clock en GPIO: %d\n", ret);
> +		return ret;
>  	}

Johan