Re: [PATCH net-next v1 2/2] ieee802154: ca8210: Switch to using gpiod API

[Miquel Raynal]

Hi Andy,

> @@ -350,8 +348,8 @@ struct work_priv_container {
>   * @extclockenable: true if the external clock is to be enabled
>   * @extclockfreq:   frequency of the external clock
>   * @extclockgpio:   ca8210 output gpio of the external clock
> - * @gpio_reset:     gpio number of ca8210 reset line
> - * @gpio_irq:       gpio number of ca8210 interrupt line
> + * @reset_gpio:     GPIO of ca8210 reset line

What about "CA8210 Reset GPIO line"? Or Just "Reset GPIO line"? Or even
"Reset GPIO descriptor" (whatever).

> + * @irq_gpio:       GPIO of ca8210 interrupt line

Same

>   * @irq_id:         identifier for the ca8210 irq
>   *
>   */
> @@ -359,8 +357,8 @@ struct ca8210_platform_data {
>  	bool extclockenable;
>  	unsigned int extclockfreq;
>  	unsigned int extclockgpio;
> -	int gpio_reset;
> -	int gpio_irq;
> +	struct gpio_desc *reset_gpio;
> +	struct gpio_desc *irq_gpio;
>  	int irq_id;
>  };

[...]

>  	/* Wait until wakeup indication seen */
> @@ -2784,25 +2782,14 @@ static void ca8210_unregister_ext_clock(struct spi_device *spi)
>   */
>  static int ca8210_reset_init(struct spi_device *spi)
>  {
> -	int ret;
> -	struct ca8210_platform_data *pdata = spi->dev.platform_data;
> +	struct device *dev = &spi->dev;
> +	struct ca8210_platform_data *pdata = dev_get_platdata(dev);
>

Can you either mention the additional cleanup that you do in the commit
log or split it in a separate commit? (splitting is probably not
necessary here given that most of the cleanup anyway is related to the
actual changes.

> -	pdata->gpio_reset = of_get_named_gpio(
> -		spi->dev.of_node,
> -		"reset-gpio",
> -		0
> -	);
> +	pdata->reset_gpio = devm_gpiod_get(dev, "reset", GPIOD_OUT_HIGH);
> +	if (IS_ERR(pdata->reset_gpio))
> +		dev_crit(dev, "Reset GPIO did not set to output mode\n");
>  
> -	ret = gpio_direction_output(pdata->gpio_reset, 1);
> -	if (ret < 0) {
> -		dev_crit(
> -			&spi->dev,
> -			"Reset GPIO %d did not set to output mode\n",
> -			pdata->gpio_reset
> -		);
> -	}
> -
> -	return ret;
> +	return PTR_ERR_OR_ZERO(pdata->reset_gpio);

This is not a strong request, but in general I think it is preferred to return
immediately, so this looks easier to understand:

+	pdata->reset_gpio = devm_gpiod_get(dev, "reset", GPIOD_OUT_HIGH);
+	if (IS_ERR(pdata->reset_gpio)) {
+		dev_crit(dev, "Reset GPIO did not set to output mode\n");
+                return PTR_ERR(pdata->reset_pgio);
+       }
+
+       return 0;

Otherwise the rest lgtm.

Thanks,
Miquèl