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

[Miquel Raynal]

On 03/03/2025 at 18:28:41 +02, Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx> wrote:

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

Yes.

> I can actually split it to a separate
> change as I had done some of that in the past in other drivers.

Up to you, either way, as long as it is mentioned in the commit log, I'm
happy.

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

That's true, and I understand your choice in the first place. But given
that you're also doing a bit of cleanup, one more misc change feels okay.

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

Great!

> ...
>
>> Otherwise the rest lgtm.
>
> Thank you for the review!