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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <87v7sq5awt.fsf@bootlin.com>
Date: Mon, 03 Mar 2025 17:36:18 +0100
From: Miquel Raynal <miquel.raynal@...tlin.com>
To: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
Cc: linux-wpan@...r.kernel.org,  netdev@...r.kernel.org,
  linux-kernel@...r.kernel.org,  linux-gpio@...r.kernel.org,  Alexander
 Aring <alex.aring@...il.com>,  Stefan Schmidt <stefan@...enfreihafen.org>,
  Andrew Lunn <andrew+netdev@...n.ch>,  "David S. Miller"
 <davem@...emloft.net>,  Eric Dumazet <edumazet@...gle.com>,  Jakub
 Kicinski <kuba@...nel.org>,  Paolo Abeni <pabeni@...hat.com>,  Linus
 Walleij <linus.walleij@...aro.org>,  Bartosz Golaszewski <brgl@...ev.pl>
Subject: 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 <andriy.shevchenko@...ux.intel.com> 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!