Re: [PATCH net-next v1 2/2] ieee802154: ca8210: Switch to using gpiod API
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
Re: [PATCH net-next v1 2/2] ieee802154: ca8210: Switch to using gpiod API
On 03/03/2025 at 18:28:41 +02, Andy Shevchenko
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!
Re: [PATCH net-next v1 2/2] ieee802154: ca8210: Switch to using gpiod API
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
