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

[Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>]

On Mon, Mar 03, 2025 at 05:20:59PM +0100, Miquel Raynal wrote:

...

> > - * @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

Sure.

[...]

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

Do you mean the platform_data accessors? I can actually split it to a separate
change as I had done some of that in the past in other drivers.

...

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

I used the same logic as in the original flow.

> +	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;

Sure I can do this in v2.

...

> Otherwise the rest lgtm.

Thank you for the review!

-- 
With Best Regards,
Andy Shevchenko