Re: [PATCH v5 3/3] staging: greybus: arche-platform: Switch to the gpio descriptor interface

[Johan Hovold <johan@kernel.org>]

On Thu, Jan 10, 2019 at 11:23:07PM +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
> "svc_reset_onoff()" for now.
> 
> Signed-off-by: Nishad Kamdar <nishadkamdar@gmail.com>
> ---
> Changes in v5:
>   - Change the commit message.
>   - Restore the names of the gpio device-tree properties without
>     the "-gpio" suffix.
> Changes in v4:
>  - Move 'gpio_desc *svc_sysboot' below the reset flag
>    as it is more logical to have reset flag below
>    reset gpio.
>  - Remove a few unnecessary line breaks.
> Changes in v3:
>  - Add this patch to a patchset.
> Changes in v2:
>  - Move comment to the same line as to what it applies to.
> ---

Also looks good. Some really minor comments to a couple of the error
messages. The issues are there in the current code, but since you modify
these messages anyway you might as well fix them up. Please fix that and
resend with my

Reviewed-by: Johan Hovold <johan@kernel.org>

Really good job with this!

> @@ -435,6 +428,7 @@ static int arche_platform_probe(struct platform_device *pdev)
>  	struct device *dev = &pdev->dev;
>  	struct device_node *np = dev->of_node;
>  	int ret;
> +	unsigned int flags;
>  
>  	arche_pdata = devm_kzalloc(&pdev->dev, sizeof(*arche_pdata),
>  				   GFP_KERNEL);
> @@ -444,61 +438,33 @@ static int arche_platform_probe(struct platform_device *pdev)
>  	/* setup svc reset gpio */
>  	arche_pdata->is_reset_act_hi = of_property_read_bool(np,
>  							     "svc,reset-active-high");
> -	arche_pdata->svc_reset_gpio = of_get_named_gpio(np,
> -							"svc,reset-gpio",
> -							0);
> -	if (!gpio_is_valid(arche_pdata->svc_reset_gpio)) {
> -		dev_err(dev, "failed to get reset-gpio\n");
> -		return arche_pdata->svc_reset_gpio;
> -	}
> -	ret = devm_gpio_request(dev, arche_pdata->svc_reset_gpio, "svc-reset");
> -	if (ret) {
> -		dev_err(dev, "failed to request svc-reset gpio:%d\n", ret);
> -		return ret;
> -	}
> -	ret = gpio_direction_output(arche_pdata->svc_reset_gpio,
> -				    arche_pdata->is_reset_act_hi);
> -	if (ret) {
> -		dev_err(dev, "failed to set svc-reset gpio dir:%d\n", ret);
> +	if (arche_pdata->is_reset_act_hi)
> +		flags = GPIOD_OUT_HIGH;
> +	else
> +		flags = GPIOD_OUT_LOW;
> +
> +	arche_pdata->svc_reset = devm_gpiod_get(dev, "svc,reset", flags);
> +	if (IS_ERR(arche_pdata->svc_reset)) {
> +		ret = PTR_ERR(arche_pdata->svc_reset);
> +		dev_err(dev, "failed to request svc-reset GPIO:%d\n", ret);

Add a space after ':' for consistency.

> @@ -515,19 +481,11 @@ static int arche_platform_probe(struct platform_device *pdev)
>  	arche_pdata->num_apbs = of_get_child_count(np);
>  	dev_dbg(dev, "Number of APB's available - %d\n", arche_pdata->num_apbs);
>  
> -	arche_pdata->wake_detect_gpio = of_get_named_gpio(np,
> -							  "svc,wake-detect-gpio",
> -							  0);
> -	if (arche_pdata->wake_detect_gpio < 0) {
> -		dev_err(dev, "failed to get wake detect gpio\n");
> -		return arche_pdata->wake_detect_gpio;
> -	}
> -
> -	ret = devm_gpio_request(dev, arche_pdata->wake_detect_gpio,
> -				"wake detect");
> -	if (ret) {
> -		dev_err(dev, "Failed requesting wake_detect gpio %d\n",
> -			arche_pdata->wake_detect_gpio);
> +	arche_pdata->wake_detect = devm_gpiod_get(dev, "svc,wake-detect",
> +						  GPIOD_IN);
> +	if (IS_ERR(arche_pdata->wake_detect)) {
> +		ret = PTR_ERR(arche_pdata->wake_detect);
> +		dev_err(dev, "Failed requesting wake_detect GPIO %d\n", ret);

Add colon after "GPIO" for consistency (and to avoid ambiguity).

>  		return ret;
>  	}

Thanks,
Johan