When working with GPIO, its direction must be set either when the GPIO is
requested by gpiod_get*() or later on by one of the gpiod_direction_*()
functions. Neither of this is done here which result in undefined behavior
on some systems.
As the reset GPIO is used right after it is requested here, it makes sense
to configure it as GPIOD_OUT_HIGH right away.
Fixes: a653f2f538f9 ("net: dsa: qca8k: introduce reset via gpio feature")
Signed-off-by: Michal Vokáč <[email protected]>
---
drivers/net/dsa/qca/qca8k-8xxx.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/net/dsa/qca/qca8k-8xxx.c b/drivers/net/dsa/qca/qca8k-8xxx.c
index 4ce68e655a63..83b19c2d7b97 100644
--- a/drivers/net/dsa/qca/qca8k-8xxx.c
+++ b/drivers/net/dsa/qca/qca8k-8xxx.c
@@ -2037,8 +2037,7 @@ qca8k_sw_probe(struct mdio_device *mdiodev)
priv->dev = &mdiodev->dev;
priv->info = of_device_get_match_data(priv->dev);
- priv->reset_gpio = devm_gpiod_get_optional(priv->dev, "reset",
- GPIOD_ASIS);
+ priv->reset_gpio = devm_gpiod_get_optional(priv->dev, "reset", GPIOD_OUT_HIGH);
if (IS_ERR(priv->reset_gpio))
return PTR_ERR(priv->reset_gpio);
--
2.1.4
On Mon, Jan 22, 2024 at 01:04:09PM +0100, Michal Vokáč wrote:
> When working with GPIO, its direction must be set either when the GPIO is
> requested by gpiod_get*() or later on by one of the gpiod_direction_*()
> functions. Neither of this is done here which result in undefined behavior
> on some systems.
>
> As the reset GPIO is used right after it is requested here, it makes sense
> to configure it as GPIOD_OUT_HIGH right away.
> Fixes: a653f2f538f9 ("net: dsa: qca8k: introduce reset via gpio feature")
> Signed-off-by: Michal Vokáč <[email protected]>
> ---
> drivers/net/dsa/qca/qca8k-8xxx.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/drivers/net/dsa/qca/qca8k-8xxx.c b/drivers/net/dsa/qca/qca8k-8xxx.c
> index 4ce68e655a63..83b19c2d7b97 100644
> --- a/drivers/net/dsa/qca/qca8k-8xxx.c
> +++ b/drivers/net/dsa/qca/qca8k-8xxx.c
> @@ -2037,8 +2037,7 @@ qca8k_sw_probe(struct mdio_device *mdiodev)
> priv->dev = &mdiodev->dev;
> priv->info = of_device_get_match_data(priv->dev);
>
> - priv->reset_gpio = devm_gpiod_get_optional(priv->dev, "reset",
> - GPIOD_ASIS);
> + priv->reset_gpio = devm_gpiod_get_optional(priv->dev, "reset", GPIOD_OUT_HIGH);
> if (IS_ERR(priv->reset_gpio))
> return PTR_ERR(priv->reset_gpio);
Hi Michal
So the current code is:
priv->reset_gpio = devm_gpiod_get_optional(priv->dev, "reset",
GPIOD_ASIS);
if (IS_ERR(priv->reset_gpio))
return PTR_ERR(priv->reset_gpio);
if (priv->reset_gpio) {
gpiod_set_value_cansleep(priv->reset_gpio, 1);
/* The active low duration must be greater than 10 ms
* and checkpatch.pl wants 20 ms.
*/
msleep(20);
gpiod_set_value_cansleep(priv->reset_gpio, 0);
}
Doesn't your change make the gpiod_set_value_cansleep() pointless?
Please extend your patch to remove it, maybe extending the comment a
little.
Please also make sure what v2 Is Cc: to the qca8k Maintainers.
Andrew
---
pw-bot: cr
On 24. 01. 24 0:07, Andrew Lunn wrote:
> On Mon, Jan 22, 2024 at 01:04:09PM +0100, Michal Vokáč wrote:
>> When working with GPIO, its direction must be set either when the GPIO is
>> requested by gpiod_get*() or later on by one of the gpiod_direction_*()
>> functions. Neither of this is done here which result in undefined behavior
>> on some systems.
>>
>> As the reset GPIO is used right after it is requested here, it makes sense
>> to configure it as GPIOD_OUT_HIGH right away.
>> Fixes: a653f2f538f9 ("net: dsa: qca8k: introduce reset via gpio feature")
>> Signed-off-by: Michal Vokáč <[email protected]>
>> ---
>> drivers/net/dsa/qca/qca8k-8xxx.c | 3 +--
>> 1 file changed, 1 insertion(+), 2 deletions(-)
>>
>> diff --git a/drivers/net/dsa/qca/qca8k-8xxx.c b/drivers/net/dsa/qca/qca8k-8xxx.c
>> index 4ce68e655a63..83b19c2d7b97 100644
>> --- a/drivers/net/dsa/qca/qca8k-8xxx.c
>> +++ b/drivers/net/dsa/qca/qca8k-8xxx.c
>> @@ -2037,8 +2037,7 @@ qca8k_sw_probe(struct mdio_device *mdiodev)
>> priv->dev = &mdiodev->dev;
>> priv->info = of_device_get_match_data(priv->dev);
>>
>> - priv->reset_gpio = devm_gpiod_get_optional(priv->dev, "reset",
>> - GPIOD_ASIS);
>> + priv->reset_gpio = devm_gpiod_get_optional(priv->dev, "reset", GPIOD_OUT_HIGH);
>> if (IS_ERR(priv->reset_gpio))
>> return PTR_ERR(priv->reset_gpio);
>
> Hi Michal
>
> So the current code is:
>
> priv->reset_gpio = devm_gpiod_get_optional(priv->dev, "reset",
> GPIOD_ASIS);
> if (IS_ERR(priv->reset_gpio))
> return PTR_ERR(priv->reset_gpio);
>
> if (priv->reset_gpio) {
> gpiod_set_value_cansleep(priv->reset_gpio, 1);
> /* The active low duration must be greater than 10 ms
> * and checkpatch.pl wants 20 ms.
> */
> msleep(20);
> gpiod_set_value_cansleep(priv->reset_gpio, 0);
> }
>
> Doesn't your change make the gpiod_set_value_cansleep() pointless?
>
> Please extend your patch to remove it, maybe extending the comment a
> little.
Hi Andrew,
I agree, in this case the call to gpiod_set_value(1) is now pointless.
I will remove it and describe the change.
> Please also make sure what v2 Is Cc: to the qca8k Maintainers.
I wonder who do you mean by qca8k maintainers? There is no one explicitly
stated as a qca8k driver maintainer in MAINTAINERS file.
I admit that there is couple of people listed in get_maintainer output
as authors/commit signers that I have not Cc'd. I have added them to
the Cc list now and will do in the v2 as well.
Thanks for the comments!
Michal
> I wonder who do you mean by qca8k maintainers? There is no one explicitly
> stated as a qca8k driver maintainer in MAINTAINERS file.
>
> I admit that there is couple of people listed in get_maintainer output
> as authors/commit signers that I have not Cc'd.
get_maintainer gives you both the formal Maintainers, and the de-facto
maintainers due to actually working on the driver. I would expect
Christian Marangi to get Cc:ed, and maybe others.
> I have added them to the Cc list now and will do in the v2 as well.
Thanks
Andrew