LinuxLists.cc - [PATCH 1/2] PCI: histb: switch to using gpiod API

[permalink] [raw]

Subject: Re: [PATCH 1/2] PCI: histb: switch to using gpiod API

On Tuesday 06 September 2022 13:43:00 Dmitry Torokhov wrote:
> + ret = gpiod_set_consumer_name(hipcie->reset_gpio,
> + "PCIe device power control");

Just unrelated thing, I know it was there before, but I saw it just now
and have to comment it: This is absolute nonsense name. "reset-gpios"
device tree property specifies PERST# signal pin (PciE ReSeT) as defined
in PCIe CEM (Card ElectroMagnetic) specification and it has absolute
nothing with PCIe power control.

My suggestion for maintainers would be to remove this critic name at
all as it would just mislead other people reading that code.

> + if (ret) {
> + dev_err(dev, "unable to set reset gpio name: %d\n", ret);
> + return ret;
> }

2022-09-06 21:30:43

[permalink] [raw]

Subject: [PATCH 2/2] PCI: mvebu: switch to using gpiod API

This patch switches the driver away from legacy gpio/of_gpio API to
gpiod API, and removes use of of_get_named_gpio_flags() which I want to
make private to gpiolib.

Signed-off-by: Dmitry Torokhov <[email protected]>
---
drivers/pci/controller/pci-mvebu.c | 48 +++++++++---------------------
1 file changed, 14 insertions(+), 34 deletions(-)

diff --git a/drivers/pci/controller/pci-mvebu.c b/drivers/pci/controller/pci-mvebu.c
index 1ced73726a26..a54beb8f611c 100644
--- a/drivers/pci/controller/pci-mvebu.c
+++ b/drivers/pci/controller/pci-mvebu.c
@@ -11,14 +11,13 @@
#include <linux/bitfield.h>
#include <linux/clk.h>
#include <linux/delay.h>
-#include <linux/gpio.h>
+#include <linux/gpio/consumer.h>
#include <linux/init.h>
#include <linux/mbus.h>
#include <linux/slab.h>
#include <linux/platform_device.h>
#include <linux/of_address.h>
#include <linux/of_irq.h>
-#include <linux/of_gpio.h>
#include <linux/of_pci.h>
#include <linux/of_platform.h>

@@ -1261,9 +1260,8 @@ static int mvebu_pcie_parse_port(struct mvebu_pcie *pcie,
struct mvebu_pcie_port *port, struct device_node *child)
{
struct device *dev = &pcie->pdev->dev;
- enum of_gpio_flags flags;
u32 slot_power_limit;
- int reset_gpio, ret;
+ int ret;
u32 num_lanes;

port->pcie = pcie;
@@ -1327,40 +1325,22 @@ static int mvebu_pcie_parse_port(struct mvebu_pcie *pcie,
port->name, child);
}

- reset_gpio = of_get_named_gpio_flags(child, "reset-gpios", 0, &flags);
- if (reset_gpio == -EPROBE_DEFER) {
- ret = reset_gpio;
+ port->reset_name = devm_kasprintf(dev, GFP_KERNEL, "%s-reset",
+ port->name);
+ if (!port->reset_name) {
+ ret = -ENOMEM;
goto err;
}

- if (gpio_is_valid(reset_gpio)) {
- unsigned long gpio_flags;
-
- port->reset_name = devm_kasprintf(dev, GFP_KERNEL, "%s-reset",
- port->name);
- if (!port->reset_name) {
- ret = -ENOMEM;
+ port->reset_gpio = devm_fwnode_gpiod_get(dev, of_fwnode_handle(child),
+ "reset", GPIOD_OUT_HIGH,
+ port->name);
+ ret = PTR_ERR_OR_ZERO(port->reset_gpio);
+ if (ret) {
+ if (ret != -ENOENT)
goto err;
- }
-
- if (flags & OF_GPIO_ACTIVE_LOW) {
- dev_info(dev, "%pOF: reset gpio is active low\n",
- child);
- gpio_flags = GPIOF_ACTIVE_LOW |
- GPIOF_OUT_INIT_LOW;
- } else {
- gpio_flags = GPIOF_OUT_INIT_HIGH;
- }
-
- ret = devm_gpio_request_one(dev, reset_gpio, gpio_flags,
- port->reset_name);
- if (ret) {
- if (ret == -EPROBE_DEFER)
- goto err;
- goto skip;
- }
-
- port->reset_gpio = gpio_to_desc(reset_gpio);
+ /* reset gpio is optional */
+ port->reset_gpio = NULL;
}

slot_power_limit = of_pci_get_slot_power_limit(child,
--
2.37.2.789.g6183377224-goog

2022-09-06 21:46:33

[permalink] [raw]

Subject: Re: [PATCH 2/2] PCI: mvebu: switch to using gpiod API

On Tuesday 06 September 2022 14:26:32 Dmitry Torokhov wrote:
> Hi Pali,
>
> On Tue, Sep 06, 2022 at 11:16:28PM +0200, Pali Rohár wrote:
> > Hello!
> >
> > On Tuesday 06 September 2022 13:43:01 Dmitry Torokhov wrote:
> > > This patch switches the driver away from legacy gpio/of_gpio API to
> > > gpiod API, and removes use of of_get_named_gpio_flags() which I want to
> > > make private to gpiolib.
> >
> > There are many pending pci-mvebu.c patches waiting for review and merge,
> > so I would suggest to wait until all other mvebu patches are processed
> > and then process this one... longer waiting period :-(
>
> OK, it is not super urgent. OTOH it is a very simple patch :)

In the worst case, I will take it into my pending list of pci-mvebu.c
patches, so it would not be lost. Just yesterday I collected patches and
created pending list:
https://git.kernel.org/pub/scm/linux/kernel/git/pali/linux.git/log/?h=pci-mvebu-pending

> >
> > > Signed-off-by: Dmitry Torokhov <[email protected]>
> > > ---
> > > drivers/pci/controller/pci-mvebu.c | 48 +++++++++---------------------
> > > 1 file changed, 14 insertions(+), 34 deletions(-)
> > >
> > > diff --git a/drivers/pci/controller/pci-mvebu.c b/drivers/pci/controller/pci-mvebu.c
> > > index 1ced73726a26..a54beb8f611c 100644
> > > --- a/drivers/pci/controller/pci-mvebu.c
> > > +++ b/drivers/pci/controller/pci-mvebu.c
> > > @@ -11,14 +11,13 @@
> > > #include <linux/bitfield.h>
> > > #include <linux/clk.h>
> > > #include <linux/delay.h>
> > > -#include <linux/gpio.h>
> > > +#include <linux/gpio/consumer.h>
> > > #include <linux/init.h>
> > > #include <linux/mbus.h>
> > > #include <linux/slab.h>
> > > #include <linux/platform_device.h>
> > > #include <linux/of_address.h>
> > > #include <linux/of_irq.h>
> > > -#include <linux/of_gpio.h>
> > > #include <linux/of_pci.h>
> > > #include <linux/of_platform.h>
> > >
> > > @@ -1261,9 +1260,8 @@ static int mvebu_pcie_parse_port(struct mvebu_pcie *pcie,
> > > struct mvebu_pcie_port *port, struct device_node *child)
> > > {
> > > struct device *dev = &pcie->pdev->dev;
> > > - enum of_gpio_flags flags;
> > > u32 slot_power_limit;
> > > - int reset_gpio, ret;
> > > + int ret;
> > > u32 num_lanes;
> > >
> > > port->pcie = pcie;
> > > @@ -1327,40 +1325,22 @@ static int mvebu_pcie_parse_port(struct mvebu_pcie *pcie,
> > > port->name, child);
> > > }
> > >
> > > - reset_gpio = of_get_named_gpio_flags(child, "reset-gpios", 0, &flags);
> > > - if (reset_gpio == -EPROBE_DEFER) {
> > > - ret = reset_gpio;
> > > + port->reset_name = devm_kasprintf(dev, GFP_KERNEL, "%s-reset",
> > > + port->name);
> > > + if (!port->reset_name) {
> > > + ret = -ENOMEM;
> > > goto err;
> > > }
> > >
> > > - if (gpio_is_valid(reset_gpio)) {
> > > - unsigned long gpio_flags;
> > > -
> > > - port->reset_name = devm_kasprintf(dev, GFP_KERNEL, "%s-reset",
> > > - port->name);
> > > - if (!port->reset_name) {
> > > - ret = -ENOMEM;
> > > + port->reset_gpio = devm_fwnode_gpiod_get(dev, of_fwnode_handle(child),
> > > + "reset", GPIOD_OUT_HIGH,
> >
> > What does it mean that there is a new GPIOD_OUT_HIGH flag passed to the
> > devm_fwnode_gpiod_get() function?
>
> This means that we drive the line as "active" as soon as we successfully
> grab GPIO. This is the same as we had with devm_gpio_request_one(), but

Ah :-( Another thing to fix. Driver should not change the signal line at
this stage, but only when it is explicitly asked - at later stage. Some
PCIe card do not like flipping reset line too quick. I see this fix
would not be such easy as during startup we need to reset endpoint card.
Normally just putting it from reset, but if card was not reset state
prior probing driver then it is needed to first put it into reset...

I would fix it this issue after your patch is merged to prevent any
other merge conflicts.

How to tell devm_fwnode_gpiod_get() function that caller is not
interested in changing signal line? Just by changing GPIOD_OUT_HIGH to 0?

> we do not need to figure out actual polarity.
>
> >
> > > + port->name);
> > > + ret = PTR_ERR_OR_ZERO(port->reset_gpio);
> > > + if (ret) {
> > > + if (ret != -ENOENT)

Just one check, I think that between "ret" and "!=" is TAB instead of
space. But I'm not sure if it was mangled by email client or of there is
really TAB.

> > > goto err;
> > > - }
> > > -
> > > - if (flags & OF_GPIO_ACTIVE_LOW) {
> > > - dev_info(dev, "%pOF: reset gpio is active low\n",
> > > - child);
> > > - gpio_flags = GPIOF_ACTIVE_LOW |
> > > - GPIOF_OUT_INIT_LOW;
> > > - } else {
> > > - gpio_flags = GPIOF_OUT_INIT_HIGH;
> > > - }
> > > -
> > > - ret = devm_gpio_request_one(dev, reset_gpio, gpio_flags,
> > > - port->reset_name);
> > > - if (ret) {
> > > - if (ret == -EPROBE_DEFER)
> > > - goto err;
> > > - goto skip;
> > > - }
> > > -
> > > - port->reset_gpio = gpio_to_desc(reset_gpio);
> > > + /* reset gpio is optional */
> > > + port->reset_gpio = NULL;
> >
> > Maybe you can also release port->reset_name as it is not used at this
> > stage?
>
> OK, I figured it was just a few bytes, but sure, I'll add devm_kfree().
>
> Thanks for the review.
>
> --
> Dmitry

2022-09-06 21:47:09

[permalink] [raw]

Subject: Re: [PATCH 2/2] PCI: mvebu: switch to using gpiod API

On Tuesday 06 September 2022 14:40:15 Dmitry Torokhov wrote:
> On Tue, Sep 06, 2022 at 02:26:32PM -0700, Dmitry Torokhov wrote:
> > Hi Pali,
> >
> > On Tue, Sep 06, 2022 at 11:16:28PM +0200, Pali Rohár wrote:
> > > Hello!
> > >
> > > On Tuesday 06 September 2022 13:43:01 Dmitry Torokhov wrote:
> > > > This patch switches the driver away from legacy gpio/of_gpio API to
> > > > gpiod API, and removes use of of_get_named_gpio_flags() which I want to
> > > > make private to gpiolib.
> > >
> > > There are many pending pci-mvebu.c patches waiting for review and merge,
> > > so I would suggest to wait until all other mvebu patches are processed
> > > and then process this one... longer waiting period :-(
> >
> > OK, it is not super urgent. OTOH it is a very simple patch :)
>
> By the way, do the pending patches address soft-leaking of reset GPIO
> when the driver fails to acquire clock for a port (I called it
> soft-leaking since devm will free it on unbind)?

I think no, because there is not any patch related to GPIO reset.

2022-09-06 21:53:58

[permalink] [raw]

Subject: Re: [PATCH 2/2] PCI: mvebu: switch to using gpiod API

Hello!

On Tuesday 06 September 2022 13:43:01 Dmitry Torokhov wrote:
> This patch switches the driver away from legacy gpio/of_gpio API to
> gpiod API, and removes use of of_get_named_gpio_flags() which I want to
> make private to gpiolib.

There are many pending pci-mvebu.c patches waiting for review and merge,
so I would suggest to wait until all other mvebu patches are processed
and then process this one... longer waiting period :-(

> Signed-off-by: Dmitry Torokhov <[email protected]>
> ---
> drivers/pci/controller/pci-mvebu.c | 48 +++++++++---------------------
> 1 file changed, 14 insertions(+), 34 deletions(-)
>
> diff --git a/drivers/pci/controller/pci-mvebu.c b/drivers/pci/controller/pci-mvebu.c
> index 1ced73726a26..a54beb8f611c 100644
> --- a/drivers/pci/controller/pci-mvebu.c
> +++ b/drivers/pci/controller/pci-mvebu.c
> @@ -11,14 +11,13 @@
> #include <linux/bitfield.h>
> #include <linux/clk.h>
> #include <linux/delay.h>
> -#include <linux/gpio.h>
> +#include <linux/gpio/consumer.h>
> #include <linux/init.h>
> #include <linux/mbus.h>
> #include <linux/slab.h>
> #include <linux/platform_device.h>
> #include <linux/of_address.h>
> #include <linux/of_irq.h>
> -#include <linux/of_gpio.h>
> #include <linux/of_pci.h>
> #include <linux/of_platform.h>
>
> @@ -1261,9 +1260,8 @@ static int mvebu_pcie_parse_port(struct mvebu_pcie *pcie,
> struct mvebu_pcie_port *port, struct device_node *child)
> {
> struct device *dev = &pcie->pdev->dev;
> - enum of_gpio_flags flags;
> u32 slot_power_limit;
> - int reset_gpio, ret;
> + int ret;
> u32 num_lanes;
>
> port->pcie = pcie;
> @@ -1327,40 +1325,22 @@ static int mvebu_pcie_parse_port(struct mvebu_pcie *pcie,
> port->name, child);
> }
>
> - reset_gpio = of_get_named_gpio_flags(child, "reset-gpios", 0, &flags);
> - if (reset_gpio == -EPROBE_DEFER) {
> - ret = reset_gpio;
> + port->reset_name = devm_kasprintf(dev, GFP_KERNEL, "%s-reset",
> + port->name);
> + if (!port->reset_name) {
> + ret = -ENOMEM;
> goto err;
> }
>
> - if (gpio_is_valid(reset_gpio)) {
> - unsigned long gpio_flags;
> -
> - port->reset_name = devm_kasprintf(dev, GFP_KERNEL, "%s-reset",
> - port->name);
> - if (!port->reset_name) {
> - ret = -ENOMEM;
> + port->reset_gpio = devm_fwnode_gpiod_get(dev, of_fwnode_handle(child),
> + "reset", GPIOD_OUT_HIGH,

What does it mean that there is a new GPIOD_OUT_HIGH flag passed to the
devm_fwnode_gpiod_get() function?

> + port->name);
> + ret = PTR_ERR_OR_ZERO(port->reset_gpio);
> + if (ret) {
> + if (ret != -ENOENT)
> goto err;
> - }
> -
> - if (flags & OF_GPIO_ACTIVE_LOW) {
> - dev_info(dev, "%pOF: reset gpio is active low\n",
> - child);
> - gpio_flags = GPIOF_ACTIVE_LOW |
> - GPIOF_OUT_INIT_LOW;
> - } else {
> - gpio_flags = GPIOF_OUT_INIT_HIGH;
> - }
> -
> - ret = devm_gpio_request_one(dev, reset_gpio, gpio_flags,
> - port->reset_name);
> - if (ret) {
> - if (ret == -EPROBE_DEFER)
> - goto err;
> - goto skip;
> - }
> -
> - port->reset_gpio = gpio_to_desc(reset_gpio);
> + /* reset gpio is optional */
> + port->reset_gpio = NULL;

Maybe you can also release port->reset_name as it is not used at this
stage?

> }
>
> slot_power_limit = of_pci_get_slot_power_limit(child,
> --
> 2.37.2.789.g6183377224-goog
>

2022-09-06 21:59:42

[permalink] [raw]

Subject: Re: [PATCH 2/2] PCI: mvebu: switch to using gpiod API

Hi Pali,

On Tue, Sep 06, 2022 at 11:16:28PM +0200, Pali Roh?r wrote:
> Hello!
>
> On Tuesday 06 September 2022 13:43:01 Dmitry Torokhov wrote:
> > This patch switches the driver away from legacy gpio/of_gpio API to
> > gpiod API, and removes use of of_get_named_gpio_flags() which I want to
> > make private to gpiolib.
>
> There are many pending pci-mvebu.c patches waiting for review and merge,
> so I would suggest to wait until all other mvebu patches are processed
> and then process this one... longer waiting period :-(

OK, it is not super urgent. OTOH it is a very simple patch :)

>
> > Signed-off-by: Dmitry Torokhov <[email protected]>
> > ---
> > drivers/pci/controller/pci-mvebu.c | 48 +++++++++---------------------
> > 1 file changed, 14 insertions(+), 34 deletions(-)
> >
> > diff --git a/drivers/pci/controller/pci-mvebu.c b/drivers/pci/controller/pci-mvebu.c
> > index 1ced73726a26..a54beb8f611c 100644
> > --- a/drivers/pci/controller/pci-mvebu.c
> > +++ b/drivers/pci/controller/pci-mvebu.c
> > @@ -11,14 +11,13 @@
> > #include <linux/bitfield.h>
> > #include <linux/clk.h>
> > #include <linux/delay.h>
> > -#include <linux/gpio.h>
> > +#include <linux/gpio/consumer.h>
> > #include <linux/init.h>
> > #include <linux/mbus.h>
> > #include <linux/slab.h>
> > #include <linux/platform_device.h>
> > #include <linux/of_address.h>
> > #include <linux/of_irq.h>
> > -#include <linux/of_gpio.h>
> > #include <linux/of_pci.h>
> > #include <linux/of_platform.h>
> >
> > @@ -1261,9 +1260,8 @@ static int mvebu_pcie_parse_port(struct mvebu_pcie *pcie,
> > struct mvebu_pcie_port *port, struct device_node *child)
> > {
> > struct device *dev = &pcie->pdev->dev;
> > - enum of_gpio_flags flags;
> > u32 slot_power_limit;
> > - int reset_gpio, ret;
> > + int ret;
> > u32 num_lanes;
> >
> > port->pcie = pcie;
> > @@ -1327,40 +1325,22 @@ static int mvebu_pcie_parse_port(struct mvebu_pcie *pcie,
> > port->name, child);
> > }
> >
> > - reset_gpio = of_get_named_gpio_flags(child, "reset-gpios", 0, &flags);
> > - if (reset_gpio == -EPROBE_DEFER) {
> > - ret = reset_gpio;
> > + port->reset_name = devm_kasprintf(dev, GFP_KERNEL, "%s-reset",
> > + port->name);
> > + if (!port->reset_name) {
> > + ret = -ENOMEM;
> > goto err;
> > }
> >
> > - if (gpio_is_valid(reset_gpio)) {
> > - unsigned long gpio_flags;
> > -
> > - port->reset_name = devm_kasprintf(dev, GFP_KERNEL, "%s-reset",
> > - port->name);
> > - if (!port->reset_name) {
> > - ret = -ENOMEM;
> > + port->reset_gpio = devm_fwnode_gpiod_get(dev, of_fwnode_handle(child),
> > + "reset", GPIOD_OUT_HIGH,
>
> What does it mean that there is a new GPIOD_OUT_HIGH flag passed to the
> devm_fwnode_gpiod_get() function?

This means that we drive the line as "active" as soon as we successfully
grab GPIO. This is the same as we had with devm_gpio_request_one(), but
we do not need to figure out actual polarity.

>
> > + port->name);
> > + ret = PTR_ERR_OR_ZERO(port->reset_gpio);
> > + if (ret) {
> > + if (ret != -ENOENT)
> > goto err;
> > - }
> > -
> > - if (flags & OF_GPIO_ACTIVE_LOW) {
> > - dev_info(dev, "%pOF: reset gpio is active low\n",
> > - child);
> > - gpio_flags = GPIOF_ACTIVE_LOW |
> > - GPIOF_OUT_INIT_LOW;
> > - } else {
> > - gpio_flags = GPIOF_OUT_INIT_HIGH;
> > - }
> > -
> > - ret = devm_gpio_request_one(dev, reset_gpio, gpio_flags,
> > - port->reset_name);
> > - if (ret) {
> > - if (ret == -EPROBE_DEFER)
> > - goto err;
> > - goto skip;
> > - }
> > -
> > - port->reset_gpio = gpio_to_desc(reset_gpio);
> > + /* reset gpio is optional */
> > + port->reset_gpio = NULL;
>
> Maybe you can also release port->reset_name as it is not used at this
> stage?

OK, I figured it was just a few bytes, but sure, I'll add devm_kfree().

Thanks for the review.

--
Dmitry

2022-09-06 21:59:53

[permalink] [raw]

Subject: Re: [PATCH 1/2] PCI: histb: switch to using gpiod API

On Tuesday 06 September 2022 14:41:20 Dmitry Torokhov wrote:
> On Tue, Sep 06, 2022 at 11:08:11PM +0200, Pali Rohár wrote:
> > On Tuesday 06 September 2022 13:43:00 Dmitry Torokhov wrote:
> > > + ret = gpiod_set_consumer_name(hipcie->reset_gpio,
> > > + "PCIe device power control");
> >
> > Just unrelated thing, I know it was there before, but I saw it just now
> > and have to comment it: This is absolute nonsense name. "reset-gpios"
> > device tree property specifies PERST# signal pin (PciE ReSeT) as defined
> > in PCIe CEM (Card ElectroMagnetic) specification and it has absolute
> > nothing with PCIe power control.
> >
> > My suggestion for maintainers would be to remove this critic name at
> > all as it would just mislead other people reading that code.
>
> I can respin the patch is you suggest a more sensible label...

Lets do renaming in different/separate patch. It is better to split API
change patch (which should have any visible functional changes) and
fixups (which will have some visible changes) in separate patches.

Lorenzo, Bjorn, Krzysztof: This is something for you... Do you have any
ideas or suggestions in unifying or fixing these names? I guess more
drivers have misleading names and it is better to do any such changes
globally and not just in one driver.

> >
> > > + if (ret) {
> > > + dev_err(dev, "unable to set reset gpio name: %d\n", ret);
> > > + return ret;
> > > }
>
> Thanks.
>
> --
> Dmitry

2022-09-06 22:01:48

[permalink] [raw]

Subject: Re: [PATCH 2/2] PCI: mvebu: switch to using gpiod API

On Tue, Sep 06, 2022 at 11:41:14PM +0200, Pali Roh?r wrote:
> On Tuesday 06 September 2022 14:26:32 Dmitry Torokhov wrote:
> > Hi Pali,
> >
> > On Tue, Sep 06, 2022 at 11:16:28PM +0200, Pali Roh?r wrote:
> > > Hello!
> > >
> > > On Tuesday 06 September 2022 13:43:01 Dmitry Torokhov wrote:
> > > > This patch switches the driver away from legacy gpio/of_gpio API to
> > > > gpiod API, and removes use of of_get_named_gpio_flags() which I want to
> > > > make private to gpiolib.
> > >
> > > There are many pending pci-mvebu.c patches waiting for review and merge,
> > > so I would suggest to wait until all other mvebu patches are processed
> > > and then process this one... longer waiting period :-(
> >
> > OK, it is not super urgent. OTOH it is a very simple patch :)
>
> In the worst case, I will take it into my pending list of pci-mvebu.c
> patches, so it would not be lost. Just yesterday I collected patches and
> created pending list:
> https://git.kernel.org/pub/scm/linux/kernel/git/pali/linux.git/log/?h=pci-mvebu-pending
>
> > >
> > > > Signed-off-by: Dmitry Torokhov <[email protected]>
> > > > ---
> > > > drivers/pci/controller/pci-mvebu.c | 48 +++++++++---------------------
> > > > 1 file changed, 14 insertions(+), 34 deletions(-)
> > > >
> > > > diff --git a/drivers/pci/controller/pci-mvebu.c b/drivers/pci/controller/pci-mvebu.c
> > > > index 1ced73726a26..a54beb8f611c 100644
> > > > --- a/drivers/pci/controller/pci-mvebu.c
> > > > +++ b/drivers/pci/controller/pci-mvebu.c
> > > > @@ -11,14 +11,13 @@
> > > > #include <linux/bitfield.h>
> > > > #include <linux/clk.h>
> > > > #include <linux/delay.h>
> > > > -#include <linux/gpio.h>
> > > > +#include <linux/gpio/consumer.h>
> > > > #include <linux/init.h>
> > > > #include <linux/mbus.h>
> > > > #include <linux/slab.h>
> > > > #include <linux/platform_device.h>
> > > > #include <linux/of_address.h>
> > > > #include <linux/of_irq.h>
> > > > -#include <linux/of_gpio.h>
> > > > #include <linux/of_pci.h>
> > > > #include <linux/of_platform.h>
> > > >
> > > > @@ -1261,9 +1260,8 @@ static int mvebu_pcie_parse_port(struct mvebu_pcie *pcie,
> > > > struct mvebu_pcie_port *port, struct device_node *child)
> > > > {
> > > > struct device *dev = &pcie->pdev->dev;
> > > > - enum of_gpio_flags flags;
> > > > u32 slot_power_limit;
> > > > - int reset_gpio, ret;
> > > > + int ret;
> > > > u32 num_lanes;
> > > >
> > > > port->pcie = pcie;
> > > > @@ -1327,40 +1325,22 @@ static int mvebu_pcie_parse_port(struct mvebu_pcie *pcie,
> > > > port->name, child);
> > > > }
> > > >
> > > > - reset_gpio = of_get_named_gpio_flags(child, "reset-gpios", 0, &flags);
> > > > - if (reset_gpio == -EPROBE_DEFER) {
> > > > - ret = reset_gpio;
> > > > + port->reset_name = devm_kasprintf(dev, GFP_KERNEL, "%s-reset",
> > > > + port->name);
> > > > + if (!port->reset_name) {
> > > > + ret = -ENOMEM;
> > > > goto err;
> > > > }
> > > >
> > > > - if (gpio_is_valid(reset_gpio)) {
> > > > - unsigned long gpio_flags;
> > > > -
> > > > - port->reset_name = devm_kasprintf(dev, GFP_KERNEL, "%s-reset",
> > > > - port->name);
> > > > - if (!port->reset_name) {
> > > > - ret = -ENOMEM;
> > > > + port->reset_gpio = devm_fwnode_gpiod_get(dev, of_fwnode_handle(child),
> > > > + "reset", GPIOD_OUT_HIGH,
> > >
> > > What does it mean that there is a new GPIOD_OUT_HIGH flag passed to the
> > > devm_fwnode_gpiod_get() function?
> >
> > This means that we drive the line as "active" as soon as we successfully
> > grab GPIO. This is the same as we had with devm_gpio_request_one(), but
>
> Ah :-( Another thing to fix. Driver should not change the signal line at
> this stage, but only when it is explicitly asked - at later stage. Some
> PCIe card do not like flipping reset line too quick. I see this fix

As far as I can see the driver has a delay of 100 usec before releasing
reset line, plus additional delay for post-reset. Is this really not
sufficient?

> would not be such easy as during startup we need to reset endpoint card.
> Normally just putting it from reset, but if card was not reset state
> prior probing driver then it is needed to first put it into reset...
>
> I would fix it this issue after your patch is merged to prevent any
> other merge conflicts.
>
> How to tell devm_fwnode_gpiod_get() function that caller is not
> interested in changing signal line? Just by changing GPIOD_OUT_HIGH to 0?

I think there are 2 options:

1. Start with GPIOD_OUT_LOW (i.e. reset is explicitly deasserted), and
then in powerup/powerdown you do explicit on/off transitions with proper
timings.

2. Start with GPIOD_ASIS (i.e. do not configure line at all), and then
when powering up you need

gpiod_direction_output(port->reset_gpio, GPIOD_OUT_HIGH);

on the first invocation (and you can skip call to
gpiod_set_value_cansleep(port->reset_gpio, 1) in that case).

>
> > we do not need to figure out actual polarity.
> >
> > >
> > > > + port->name);
> > > > + ret = PTR_ERR_OR_ZERO(port->reset_gpio);
> > > > + if (ret) {
> > > > + if (ret != -ENOENT)
>
> Just one check, I think that between "ret" and "!=" is TAB instead of
> space. But I'm not sure if it was mangled by email client or of there is
> really TAB.

Ah, indeed, sorry about that.

>
> > > > goto err;
> > > > - }
> > > > -
> > > > - if (flags & OF_GPIO_ACTIVE_LOW) {
> > > > - dev_info(dev, "%pOF: reset gpio is active low\n",
> > > > - child);
> > > > - gpio_flags = GPIOF_ACTIVE_LOW |
> > > > - GPIOF_OUT_INIT_LOW;
> > > > - } else {
> > > > - gpio_flags = GPIOF_OUT_INIT_HIGH;
> > > > - }
> > > > -
> > > > - ret = devm_gpio_request_one(dev, reset_gpio, gpio_flags,
> > > > - port->reset_name);
> > > > - if (ret) {
> > > > - if (ret == -EPROBE_DEFER)
> > > > - goto err;
> > > > - goto skip;
> > > > - }
> > > > -
> > > > - port->reset_gpio = gpio_to_desc(reset_gpio);
> > > > + /* reset gpio is optional */
> > > > + port->reset_gpio = NULL;
> > >
> > > Maybe you can also release port->reset_name as it is not used at this
> > > stage?
> >
> > OK, I figured it was just a few bytes, but sure, I'll add devm_kfree().
> >
> > Thanks for the review.
> >
> > --
> > Dmitry

--
Dmitry

2022-09-06 22:12:13

[permalink] [raw]

Subject: Re: [PATCH 2/2] PCI: mvebu: switch to using gpiod API

On Tue, Sep 06, 2022 at 02:26:32PM -0700, Dmitry Torokhov wrote:
> Hi Pali,
>
> On Tue, Sep 06, 2022 at 11:16:28PM +0200, Pali Roh?r wrote:
> > Hello!
> >
> > On Tuesday 06 September 2022 13:43:01 Dmitry Torokhov wrote:
> > > This patch switches the driver away from legacy gpio/of_gpio API to
> > > gpiod API, and removes use of of_get_named_gpio_flags() which I want to
> > > make private to gpiolib.
> >
> > There are many pending pci-mvebu.c patches waiting for review and merge,
> > so I would suggest to wait until all other mvebu patches are processed
> > and then process this one... longer waiting period :-(
>
> OK, it is not super urgent. OTOH it is a very simple patch :)

By the way, do the pending patches address soft-leaking of reset GPIO
when the driver fails to acquire clock for a port (I called it
soft-leaking since devm will free it on unbind)?

Thanks.

--
Dmitry

2022-09-06 22:14:38

[permalink] [raw]

Subject: Re: [PATCH 1/2] PCI: histb: switch to using gpiod API

On Tue, Sep 06, 2022 at 11:08:11PM +0200, Pali Roh?r wrote:
> On Tuesday 06 September 2022 13:43:00 Dmitry Torokhov wrote:
> > + ret = gpiod_set_consumer_name(hipcie->reset_gpio,
> > + "PCIe device power control");
>
> Just unrelated thing, I know it was there before, but I saw it just now
> and have to comment it: This is absolute nonsense name. "reset-gpios"
> device tree property specifies PERST# signal pin (PciE ReSeT) as defined
> in PCIe CEM (Card ElectroMagnetic) specification and it has absolute
> nothing with PCIe power control.
>
> My suggestion for maintainers would be to remove this critic name at
> all as it would just mislead other people reading that code.

I can respin the patch is you suggest a more sensible label...

>
> > + if (ret) {
> > + dev_err(dev, "unable to set reset gpio name: %d\n", ret);
> > + return ret;
> > }

Thanks.

--
Dmitry

2022-09-06 23:20:38

[permalink] [raw]

Subject: Re: [PATCH 2/2] PCI: mvebu: switch to using gpiod API

On Tuesday 06 September 2022 14:52:42 Dmitry Torokhov wrote:
> On Tue, Sep 06, 2022 at 11:41:14PM +0200, Pali Rohár wrote:
> > On Tuesday 06 September 2022 14:26:32 Dmitry Torokhov wrote:
> > > Hi Pali,
> > >
> > > On Tue, Sep 06, 2022 at 11:16:28PM +0200, Pali Rohár wrote:
> > > > Hello!
> > > >
> > > > On Tuesday 06 September 2022 13:43:01 Dmitry Torokhov wrote:
> > > > > This patch switches the driver away from legacy gpio/of_gpio API to
> > > > > gpiod API, and removes use of of_get_named_gpio_flags() which I want to
> > > > > make private to gpiolib.
> > > >
> > > > There are many pending pci-mvebu.c patches waiting for review and merge,
> > > > so I would suggest to wait until all other mvebu patches are processed
> > > > and then process this one... longer waiting period :-(
> > >
> > > OK, it is not super urgent. OTOH it is a very simple patch :)
> >
> > In the worst case, I will take it into my pending list of pci-mvebu.c
> > patches, so it would not be lost. Just yesterday I collected patches and
> > created pending list:
> > https://git.kernel.org/pub/scm/linux/kernel/git/pali/linux.git/log/?h=pci-mvebu-pending
> >
> > > >
> > > > > Signed-off-by: Dmitry Torokhov <[email protected]>
> > > > > ---
> > > > > drivers/pci/controller/pci-mvebu.c | 48 +++++++++---------------------
> > > > > 1 file changed, 14 insertions(+), 34 deletions(-)
> > > > >
> > > > > diff --git a/drivers/pci/controller/pci-mvebu.c b/drivers/pci/controller/pci-mvebu.c
> > > > > index 1ced73726a26..a54beb8f611c 100644
> > > > > --- a/drivers/pci/controller/pci-mvebu.c
> > > > > +++ b/drivers/pci/controller/pci-mvebu.c
> > > > > @@ -11,14 +11,13 @@
> > > > > #include <linux/bitfield.h>
> > > > > #include <linux/clk.h>
> > > > > #include <linux/delay.h>
> > > > > -#include <linux/gpio.h>
> > > > > +#include <linux/gpio/consumer.h>
> > > > > #include <linux/init.h>
> > > > > #include <linux/mbus.h>
> > > > > #include <linux/slab.h>
> > > > > #include <linux/platform_device.h>
> > > > > #include <linux/of_address.h>
> > > > > #include <linux/of_irq.h>
> > > > > -#include <linux/of_gpio.h>
> > > > > #include <linux/of_pci.h>
> > > > > #include <linux/of_platform.h>
> > > > >
> > > > > @@ -1261,9 +1260,8 @@ static int mvebu_pcie_parse_port(struct mvebu_pcie *pcie,
> > > > > struct mvebu_pcie_port *port, struct device_node *child)
> > > > > {
> > > > > struct device *dev = &pcie->pdev->dev;
> > > > > - enum of_gpio_flags flags;
> > > > > u32 slot_power_limit;
> > > > > - int reset_gpio, ret;
> > > > > + int ret;
> > > > > u32 num_lanes;
> > > > >
> > > > > port->pcie = pcie;
> > > > > @@ -1327,40 +1325,22 @@ static int mvebu_pcie_parse_port(struct mvebu_pcie *pcie,
> > > > > port->name, child);
> > > > > }
> > > > >
> > > > > - reset_gpio = of_get_named_gpio_flags(child, "reset-gpios", 0, &flags);
> > > > > - if (reset_gpio == -EPROBE_DEFER) {
> > > > > - ret = reset_gpio;
> > > > > + port->reset_name = devm_kasprintf(dev, GFP_KERNEL, "%s-reset",
> > > > > + port->name);
> > > > > + if (!port->reset_name) {
> > > > > + ret = -ENOMEM;
> > > > > goto err;
> > > > > }
> > > > >
> > > > > - if (gpio_is_valid(reset_gpio)) {
> > > > > - unsigned long gpio_flags;
> > > > > -
> > > > > - port->reset_name = devm_kasprintf(dev, GFP_KERNEL, "%s-reset",
> > > > > - port->name);
> > > > > - if (!port->reset_name) {
> > > > > - ret = -ENOMEM;
> > > > > + port->reset_gpio = devm_fwnode_gpiod_get(dev, of_fwnode_handle(child),
> > > > > + "reset", GPIOD_OUT_HIGH,
> > > >
> > > > What does it mean that there is a new GPIOD_OUT_HIGH flag passed to the
> > > > devm_fwnode_gpiod_get() function?
> > >
> > > This means that we drive the line as "active" as soon as we successfully
> > > grab GPIO. This is the same as we had with devm_gpio_request_one(), but
> >
> > Ah :-( Another thing to fix. Driver should not change the signal line at
> > this stage, but only when it is explicitly asked - at later stage. Some
> > PCIe card do not like flipping reset line too quick. I see this fix
>
> As far as I can see the driver has a delay of 100 usec before releasing
> reset line, plus additional delay for post-reset. Is this really not
> sufficient?

I do not know right now. Something which I planning to measure and check
after this... One point is that we do not want to sleep in kernel probe
functions for a longer time than needed as it is serialized and slow
down kernel boot. And another point is that there is still open question
how long must kernel hold reset line of endpoint card to ensure that any
connected PCIe card is properly reset... I remember from past testing
that some Qualcomm wifi ath10k cards requires at least 10ms.

> > would not be such easy as during startup we need to reset endpoint card.
> > Normally just putting it from reset, but if card was not reset state
> > prior probing driver then it is needed to first put it into reset...
> >
> > I would fix it this issue after your patch is merged to prevent any
> > other merge conflicts.
> >
> > How to tell devm_fwnode_gpiod_get() function that caller is not
> > interested in changing signal line? Just by changing GPIOD_OUT_HIGH to 0?
>
> I think there are 2 options:
>
> 1. Start with GPIOD_OUT_LOW (i.e. reset is explicitly deasserted), and
> then in powerup/powerdown you do explicit on/off transitions with proper
> timings.

PERST# is active-low. So deasserting means to put it into high state.
But device tree can specify if line is active-high as on some board
designs is GPIO output connected to inverter (or to level shifter with
additional logic of signal inversion). So what [GPIOD_]OUT_LOW means in
this context? Just it is needed that from driver point of view always
value 1 means reset active and 0 means reset inactive, independently of
double (triple?) inversions.

> 2. Start with GPIOD_ASIS (i.e. do not configure line at all), and then
> when powering up you need
>
> gpiod_direction_output(port->reset_gpio, GPIOD_OUT_HIGH);
>
> on the first invocation (and you can skip call to
> gpiod_set_value_cansleep(port->reset_gpio, 1) in that case).

This option is probably better as reset line logic is only at
appropriate place. And also this was my idea.

Anyway, in the past I wrote proposal how to cleanup and fixup it:
https://lore.kernel.org/linux-pci/20211022183808.jdeo7vntnagqkg7g@pali/

> >
> > > we do not need to figure out actual polarity.
> > >
> > > >
> > > > > + port->name);
> > > > > + ret = PTR_ERR_OR_ZERO(port->reset_gpio);
> > > > > + if (ret) {
> > > > > + if (ret != -ENOENT)
> >
> > Just one check, I think that between "ret" and "!=" is TAB instead of
> > space. But I'm not sure if it was mangled by email client or of there is
> > really TAB.
>
> Ah, indeed, sorry about that.
>
> >
> > > > > goto err;
> > > > > - }
> > > > > -
> > > > > - if (flags & OF_GPIO_ACTIVE_LOW) {
> > > > > - dev_info(dev, "%pOF: reset gpio is active low\n",
> > > > > - child);
> > > > > - gpio_flags = GPIOF_ACTIVE_LOW |
> > > > > - GPIOF_OUT_INIT_LOW;
> > > > > - } else {
> > > > > - gpio_flags = GPIOF_OUT_INIT_HIGH;
> > > > > - }
> > > > > -
> > > > > - ret = devm_gpio_request_one(dev, reset_gpio, gpio_flags,
> > > > > - port->reset_name);
> > > > > - if (ret) {
> > > > > - if (ret == -EPROBE_DEFER)
> > > > > - goto err;
> > > > > - goto skip;
> > > > > - }
> > > > > -
> > > > > - port->reset_gpio = gpio_to_desc(reset_gpio);
> > > > > + /* reset gpio is optional */
> > > > > + port->reset_gpio = NULL;
> > > >
> > > > Maybe you can also release port->reset_name as it is not used at this
> > > > stage?
> > >
> > > OK, I figured it was just a few bytes, but sure, I'll add devm_kfree().
> > >
> > > Thanks for the review.
> > >
> > > --
> > > Dmitry
>
> --
> Dmitry

2022-09-06 23:57:56

[permalink] [raw]

Subject: Re: [PATCH 2/2] PCI: mvebu: switch to using gpiod API

On Wed, Sep 07, 2022 at 12:09:01AM +0200, Pali Roh?r wrote:
> On Tuesday 06 September 2022 14:52:42 Dmitry Torokhov wrote:
> > On Tue, Sep 06, 2022 at 11:41:14PM +0200, Pali Roh?r wrote:
> > > On Tuesday 06 September 2022 14:26:32 Dmitry Torokhov wrote:

> > > would not be such easy as during startup we need to reset endpoint card.
> > > Normally just putting it from reset, but if card was not reset state
> > > prior probing driver then it is needed to first put it into reset...
> > >
> > > I would fix it this issue after your patch is merged to prevent any
> > > other merge conflicts.
> > >
> > > How to tell devm_fwnode_gpiod_get() function that caller is not
> > > interested in changing signal line? Just by changing GPIOD_OUT_HIGH to 0?
> >
> > I think there are 2 options:
> >
> > 1. Start with GPIOD_OUT_LOW (i.e. reset is explicitly deasserted), and
> > then in powerup/powerdown you do explicit on/off transitions with proper
> > timings.
>
> PERST# is active-low. So deasserting means to put it into high state.
> But device tree can specify if line is active-high as on some board
> designs is GPIO output connected to inverter (or to level shifter with
> additional logic of signal inversion). So what [GPIOD_]OUT_LOW means in
> this context? Just it is needed that from driver point of view always
> value 1 means reset active and 0 means reset inactive, independently of
> double (triple?) inversions.

Think of GPIOD_OUT_LOW and GPIOD_OUT_HIGH as logical off and on, or
logical deactivate/activate. Gpiolib will take into account declared
polarity of the line when it drives the output, so for lines marked as
GPIO_ACTIVE_HIGH GPIO_OUT_HIGH will result in line driven HIGH, whereas
for lines marked as GPIO_ACTIVE_LOW GPIO_OUT_HIGH will result in the
line being driven LOW.

Linus, do you think we should introduce GPIOD_OUT_INACTIVE /
GPIOD_OUT_ACTIVE or GPIOD_OUT_DEASSERTED / GPIOD_OUT_ASSERTED and
deprecate existing GPIOD_OUT_LOW and GPIO_OUT_HIGH?

Thanks.

--
Dmitry

2022-09-07 04:23:15

by kernel test robot

[permalink] [raw]

Subject: Re: [PATCH 2/2] PCI: mvebu: switch to using gpiod API

Hi Dmitry,

I love your patch! Yet something to improve:

[auto build test ERROR on helgaas-pci/next]
[also build test ERROR on linus/master v6.0-rc4 next-20220906]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url: https://github.com/intel-lab-lkp/linux/commits/Dmitry-Torokhov/PCI-histb-switch-to-using-gpiod-API/20220907-044417
base: https://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git next
config: arm-defconfig (https://download.01.org/0day-ci/archive/20220907/[email protected]/config)
compiler: arm-linux-gnueabi-gcc (GCC) 12.1.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/90d5c7ec598d196395d3a5934099b56d1c8e071a
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Dmitry-Torokhov/PCI-histb-switch-to-using-gpiod-API/20220907-044417
git checkout 90d5c7ec598d196395d3a5934099b56d1c8e071a
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=arm SHELL=/bin/bash

If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <[email protected]>

All errors (new ones prefixed by >>):

drivers/pci/controller/pci-mvebu.c: In function 'mvebu_pcie_irq_handler':
>> drivers/pci/controller/pci-mvebu.c:1100:9: error: implicit declaration of function 'chained_irq_enter'; did you mean 'ct_irq_enter'? [-Werror=implicit-function-declaration]
1100 | chained_irq_enter(chip, desc);
| ^~~~~~~~~~~~~~~~~
| ct_irq_enter
>> drivers/pci/controller/pci-mvebu.c:1115:9: error: implicit declaration of function 'chained_irq_exit'; did you mean 'ct_irq_exit'? [-Werror=implicit-function-declaration]
1115 | chained_irq_exit(chip, desc);
| ^~~~~~~~~~~~~~~~
| ct_irq_exit
cc1: some warnings being treated as errors

vim +1100 drivers/pci/controller/pci-mvebu.c

ec075262648f39 Pali Roh?r 2022-02-22 1091
ec075262648f39 Pali Roh?r 2022-02-22 1092 static void mvebu_pcie_irq_handler(struct irq_desc *desc)
ec075262648f39 Pali Roh?r 2022-02-22 1093 {
ec075262648f39 Pali Roh?r 2022-02-22 1094 struct mvebu_pcie_port *port = irq_desc_get_handler_data(desc);
ec075262648f39 Pali Roh?r 2022-02-22 1095 struct irq_chip *chip = irq_desc_get_chip(desc);
ec075262648f39 Pali Roh?r 2022-02-22 1096 struct device *dev = &port->pcie->pdev->dev;
ec075262648f39 Pali Roh?r 2022-02-22 1097 u32 cause, unmask, status;
ec075262648f39 Pali Roh?r 2022-02-22 1098 int i;
ec075262648f39 Pali Roh?r 2022-02-22 1099
ec075262648f39 Pali Roh?r 2022-02-22 @1100 chained_irq_enter(chip, desc);
ec075262648f39 Pali Roh?r 2022-02-22 1101
ec075262648f39 Pali Roh?r 2022-02-22 1102 cause = mvebu_readl(port, PCIE_INT_CAUSE_OFF);
ec075262648f39 Pali Roh?r 2022-02-22 1103 unmask = mvebu_readl(port, PCIE_INT_UNMASK_OFF);
ec075262648f39 Pali Roh?r 2022-02-22 1104 status = cause & unmask;
ec075262648f39 Pali Roh?r 2022-02-22 1105
ec075262648f39 Pali Roh?r 2022-02-22 1106 /* Process legacy INTx interrupts */
ec075262648f39 Pali Roh?r 2022-02-22 1107 for (i = 0; i < PCI_NUM_INTX; i++) {
ec075262648f39 Pali Roh?r 2022-02-22 1108 if (!(status & PCIE_INT_INTX(i)))
ec075262648f39 Pali Roh?r 2022-02-22 1109 continue;
ec075262648f39 Pali Roh?r 2022-02-22 1110
ec075262648f39 Pali Roh?r 2022-02-22 1111 if (generic_handle_domain_irq(port->intx_irq_domain, i) == -EINVAL)
ec075262648f39 Pali Roh?r 2022-02-22 1112 dev_err_ratelimited(dev, "unexpected INT%c IRQ\n", (char)i+'A');
ec075262648f39 Pali Roh?r 2022-02-22 1113 }
ec075262648f39 Pali Roh?r 2022-02-22 1114
ec075262648f39 Pali Roh?r 2022-02-22 @1115 chained_irq_exit(chip, desc);
ec075262648f39 Pali Roh?r 2022-02-22 1116 }
ec075262648f39 Pali Roh?r 2022-02-22 1117

--
0-DAY CI Kernel Test Service
https://01.org/lkp

2022-09-08 08:43:14

[permalink] [raw]

Subject: Re: [PATCH 1/2] PCI: histb: switch to using gpiod API

On Tue, Sep 6, 2022 at 10:43 PM Dmitry Torokhov
<[email protected]> wrote:

> This patch switches the driver away from legacy gpio/of_gpio API to
> gpiod API, and removes use of of_get_named_gpio_flags() which I want to
> make private to gpiolib.
>
> Signed-off-by: Dmitry Torokhov <[email protected]>

Reviewed-by: Linus Walleij <[email protected]>

Yours,
Linus Walleij

2022-09-08 09:21:26

[permalink] [raw]

Subject: Re: [PATCH 2/2] PCI: mvebu: switch to using gpiod API

On Tue, Sep 6, 2022 at 11:16 PM Pali Rohár <[email protected]> wrote:
> On Tuesday 06 September 2022 13:43:01 Dmitry Torokhov wrote:

> > This patch switches the driver away from legacy gpio/of_gpio API to
> > gpiod API, and removes use of of_get_named_gpio_flags() which I want to
> > make private to gpiolib.
>
> There are many pending pci-mvebu.c patches waiting for review and merge,
> so I would suggest to wait until all other mvebu patches are processed
> and then process this one... longer waiting period :-(

What about the MVEBU maintainers create a git branch and pile up
all patches and send a pull request to Bjorn then?
This usually works.

Yours,
Linus Walleij

2022-09-09 20:29:06

by kernel test robot

[permalink] [raw]

Subject: Re: [PATCH 2/2] PCI: mvebu: switch to using gpiod API

Hi Dmitry,

I love your patch! Yet something to improve:

[auto build test ERROR on helgaas-pci/next]
[also build test ERROR on linus/master v6.0-rc4 next-20220909]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url: https://github.com/intel-lab-lkp/linux/commits/Dmitry-Torokhov/PCI-histb-switch-to-using-gpiod-API/20220907-044417
base: https://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git next
config: arm-multi_v5_defconfig
compiler: clang version 16.0.0 (https://github.com/llvm/llvm-project 1546df49f5a6d09df78f569e4137ddb365a3e827)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# install arm cross compiling tool for clang build
# apt-get install binutils-arm-linux-gnueabi
# https://github.com/intel-lab-lkp/linux/commit/90d5c7ec598d196395d3a5934099b56d1c8e071a
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Dmitry-Torokhov/PCI-histb-switch-to-using-gpiod-API/20220907-044417
git checkout 90d5c7ec598d196395d3a5934099b56d1c8e071a
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=arm SHELL=/bin/bash

If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <[email protected]>

All errors (new ones prefixed by >>):

>> drivers/pci/controller/pci-mvebu.c:1100:2: error: call to undeclared function 'chained_irq_enter'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
chained_irq_enter(chip, desc);
^
>> drivers/pci/controller/pci-mvebu.c:1115:2: error: call to undeclared function 'chained_irq_exit'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
chained_irq_exit(chip, desc);
^
2 errors generated.

vim +/chained_irq_enter +1100 drivers/pci/controller/pci-mvebu.c

ec075262648f39 Pali Roh?r 2022-02-22 1091
ec075262648f39 Pali Roh?r 2022-02-22 1092 static void mvebu_pcie_irq_handler(struct irq_desc *desc)
ec075262648f39 Pali Roh?r 2022-02-22 1093 {
ec075262648f39 Pali Roh?r 2022-02-22 1094 struct mvebu_pcie_port *port = irq_desc_get_handler_data(desc);
ec075262648f39 Pali Roh?r 2022-02-22 1095 struct irq_chip *chip = irq_desc_get_chip(desc);
ec075262648f39 Pali Roh?r 2022-02-22 1096 struct device *dev = &port->pcie->pdev->dev;
ec075262648f39 Pali Roh?r 2022-02-22 1097 u32 cause, unmask, status;
ec075262648f39 Pali Roh?r 2022-02-22 1098 int i;
ec075262648f39 Pali Roh?r 2022-02-22 1099
ec075262648f39 Pali Roh?r 2022-02-22 @1100 chained_irq_enter(chip, desc);
ec075262648f39 Pali Roh?r 2022-02-22 1101
ec075262648f39 Pali Roh?r 2022-02-22 1102 cause = mvebu_readl(port, PCIE_INT_CAUSE_OFF);
ec075262648f39 Pali Roh?r 2022-02-22 1103 unmask = mvebu_readl(port, PCIE_INT_UNMASK_OFF);
ec075262648f39 Pali Roh?r 2022-02-22 1104 status = cause & unmask;
ec075262648f39 Pali Roh?r 2022-02-22 1105
ec075262648f39 Pali Roh?r 2022-02-22 1106 /* Process legacy INTx interrupts */
ec075262648f39 Pali Roh?r 2022-02-22 1107 for (i = 0; i < PCI_NUM_INTX; i++) {
ec075262648f39 Pali Roh?r 2022-02-22 1108 if (!(status & PCIE_INT_INTX(i)))
ec075262648f39 Pali Roh?r 2022-02-22 1109 continue;
ec075262648f39 Pali Roh?r 2022-02-22 1110
ec075262648f39 Pali Roh?r 2022-02-22 1111 if (generic_handle_domain_irq(port->intx_irq_domain, i) == -EINVAL)
ec075262648f39 Pali Roh?r 2022-02-22 1112 dev_err_ratelimited(dev, "unexpected INT%c IRQ\n", (char)i+'A');
ec075262648f39 Pali Roh?r 2022-02-22 1113 }
ec075262648f39 Pali Roh?r 2022-02-22 1114
ec075262648f39 Pali Roh?r 2022-02-22 @1115 chained_irq_exit(chip, desc);
ec075262648f39 Pali Roh?r 2022-02-22 1116 }
ec075262648f39 Pali Roh?r 2022-02-22 1117

--
0-DAY CI Kernel Test Service
https://01.org/lkp

Attachments:

(No filename) (4.22 kB)
config (174.76 kB)
Download all attachments

2022-09-11 13:29:19

[permalink] [raw]

Subject: Re: [PATCH 2/2] PCI: mvebu: switch to using gpiod API

On Tuesday 06 September 2022 15:41:23 Dmitry Torokhov wrote:
> Linus, do you think we should introduce GPIOD_OUT_INACTIVE /
> GPIOD_OUT_ACTIVE or GPIOD_OUT_DEASSERTED / GPIOD_OUT_ASSERTED and
> deprecate existing GPIOD_OUT_LOW and GPIO_OUT_HIGH?

+1 for GPIOD_OUT_INACTIVE / GPIOD_OUT_ACTIVE. It is less misleading than GPIOD_OUT_LOW.

2022-09-14 10:52:57

[permalink] [raw]

Subject: Re: [PATCH 2/2] PCI: mvebu: switch to using gpiod API

On Wed, Sep 7, 2022 at 12:41 AM Dmitry Torokhov
<[email protected]> wrote:

> Linus, do you think we should introduce GPIOD_OUT_INACTIVE /
> GPIOD_OUT_ACTIVE or GPIOD_OUT_DEASSERTED / GPIOD_OUT_ASSERTED and
> deprecate existing GPIOD_OUT_LOW and GPIO_OUT_HIGH?

They should rather be replaced everywhere in one go.

I think it is just a half-measure unless we also add
#define GPIOD_ASSERTED 1
#define GPIOD_DEASSERTED 0
to be used instead of 1/0 in gpiod_set_value().

It would also imply changing the signature of the function
gpiod_set_value() to gpiod_set_state() as we are not
really setting a value but a state.

I have thought about changing this, but the problem is that I felt
it should be accompanied with a change fixing as many users
as possible.

I think this is one of those occasions where we should merge
the new defines, and then send Linus Torvalds a sed script
that he can run at the end of the merge window to change all
gpiod_set_value(...., 1) -> gpiod_set_state(...., GPIOD_ASSERTED);
everywhere.

After all users are changed, the GPIOD_ASSERTED/DEASSERTED
defined can be turned into an enum.

That would be the silver bullet against a lot of confusion IMO.

We would need Bartosz' input on this.

Yours,
Linus Walleij

2022-09-14 12:19:44

by Bartosz Golaszewski

[permalink] [raw]

Subject: Re: [PATCH 2/2] PCI: mvebu: switch to using gpiod API

On Wed, Sep 14, 2022 at 12:35 PM Linus Walleij <[email protected]> wrote:
>
> On Wed, Sep 7, 2022 at 12:41 AM Dmitry Torokhov
> <[email protected]> wrote:
>
> > Linus, do you think we should introduce GPIOD_OUT_INACTIVE /
> > GPIOD_OUT_ACTIVE or GPIOD_OUT_DEASSERTED / GPIOD_OUT_ASSERTED and
> > deprecate existing GPIOD_OUT_LOW and GPIO_OUT_HIGH?
>
> They should rather be replaced everywhere in one go.
>
> I think it is just a half-measure unless we also add
> #define GPIOD_ASSERTED 1
> #define GPIOD_DEASSERTED 0
> to be used instead of 1/0 in gpiod_set_value().
>

We've had that discussion for libgpiod and went with
GPIOD_VALUE_ACTIVE and GPIOD_VALUE_INACTIVE but...

> It would also imply changing the signature of the function
> gpiod_set_value() to gpiod_set_state() as we are not
> really setting a value but a state.
>

... now that you've mentioned it it does seem like
GPIOD_STATE_ACTIVE/INACTIVE makes much more sense as well as naming
the relevant functions gpiod_line_request_set_line_state() etc.

> I have thought about changing this, but the problem is that I felt
> it should be accompanied with a change fixing as many users
> as possible.
>
> I think this is one of those occasions where we should merge
> the new defines, and then send Linus Torvalds a sed script
> that he can run at the end of the merge window to change all
> gpiod_set_value(...., 1) -> gpiod_set_state(...., GPIOD_ASSERTED);
> everywhere.
>
> After all users are changed, the GPIOD_ASSERTED/DEASSERTED
> defined can be turned into an enum.
>
> That would be the silver bullet against a lot of confusion IMO.
>
> We would need Bartosz' input on this.
>

We can also let Linus know that we'll do it ourselves late in the
merge window and send a separate PR on Saturday before rc1?

Bart

> Yours,
> Linus Walleij

2022-09-14 12:57:15

[permalink] [raw]

Subject: Re: [PATCH 2/2] PCI: mvebu: switch to using gpiod API

On Wed, Sep 14, 2022 at 2:10 PM Bartosz Golaszewski <[email protected]> wrote:
> On Wed, Sep 14, 2022 at 12:35 PM Linus Walleij <[email protected]> wrote:

> > I think this is one of those occasions where we should merge
> > the new defines, and then send Linus Torvalds a sed script
> > that he can run at the end of the merge window to change all
> > gpiod_set_value(...., 1) -> gpiod_set_state(...., GPIOD_ASSERTED);
> > everywhere.
>
> We can also let Linus know that we'll do it ourselves late in the
> merge window and send a separate PR on Saturday before rc1?

I suppose, it's a matter of taste. It's probably easier for him to
check the sanity of a sed script than of all the changes it generates
though.

Yours,
Linus Walleij

2022-09-14 13:47:34

[permalink] [raw]

Subject: Re: [PATCH 2/2] PCI: mvebu: switch to using gpiod API

On Wed, Sep 14, 2022 at 02:10:21PM +0200, Bartosz Golaszewski wrote:
> On Wed, Sep 14, 2022 at 12:35 PM Linus Walleij <[email protected]> wrote:
> >
> > On Wed, Sep 7, 2022 at 12:41 AM Dmitry Torokhov
> > <[email protected]> wrote:
> >
> > > Linus, do you think we should introduce GPIOD_OUT_INACTIVE /
> > > GPIOD_OUT_ACTIVE or GPIOD_OUT_DEASSERTED / GPIOD_OUT_ASSERTED and
> > > deprecate existing GPIOD_OUT_LOW and GPIO_OUT_HIGH?
> >
> > They should rather be replaced everywhere in one go.
> >
> > I think it is just a half-measure unless we also add
> > #define GPIOD_ASSERTED 1
> > #define GPIOD_DEASSERTED 0
> > to be used instead of 1/0 in gpiod_set_value().
> >
>
> We've had that discussion for libgpiod and went with
> GPIOD_VALUE_ACTIVE and GPIOD_VALUE_INACTIVE but...
>
> > It would also imply changing the signature of the function
> > gpiod_set_value() to gpiod_set_state() as we are not
> > really setting a value but a state.
> >
>

I might be in the minority here, but in this context value and state mean
the same thing to me, so changing from one to the other provides no
additional clarity and is at best pointless.

The key distinction is whether you are describing a physical or logical
value/state.
High/low should be reserved for physical.
Active/inactive describe logical.

(I personally dislike "deassert" as it is a manufactured word that feels
very awkward.)

So for gpiod_set_value(), which expects a logical value, you would use
ACTIVE/INACTIVE. For gpiod_set_raw_value(), which expects a physical
value, you would use HIGH/LOW.

Given there are gpiod functions that expect both, there is a case for
both to pairings exist, and for the caller to use the appropriate one
for the call.

Changing gpiod_set_value() to gpiod_set_state(), while leaving
gpiod_set_raw_value() as is, does not reduce confusion - at least not for
me.

Cheers,
Kent.

> ... now that you've mentioned it it does seem like
> GPIOD_STATE_ACTIVE/INACTIVE makes much more sense as well as naming
> the relevant functions gpiod_line_request_set_line_state() etc.
>
> > I have thought about changing this, but the problem is that I felt
> > it should be accompanied with a change fixing as many users
> > as possible.
> >
> > I think this is one of those occasions where we should merge
> > the new defines, and then send Linus Torvalds a sed script
> > that he can run at the end of the merge window to change all
> > gpiod_set_value(...., 1) -> gpiod_set_state(...., GPIOD_ASSERTED);
> > everywhere.
> >
> > After all users are changed, the GPIOD_ASSERTED/DEASSERTED
> > defined can be turned into an enum.
> >
> > That would be the silver bullet against a lot of confusion IMO.
> >
> > We would need Bartosz' input on this.
> >
>
> We can also let Linus know that we'll do it ourselves late in the
> merge window and send a separate PR on Saturday before rc1?
>
> Bart
>
> > Yours,
> > Linus Walleij

2022-09-14 13:52:22

[permalink] [raw]

Subject: Re: [PATCH 2/2] PCI: mvebu: switch to using gpiod API

On Wed, Sep 14, 2022 at 3:00 PM Kent Gibson <[email protected]> wrote:

> The key distinction is whether you are describing a physical or logical
> value/state.
> High/low should be reserved for physical.
> Active/inactive describe logical.
>
> (I personally dislike "deassert" as it is a manufactured word that feels
> very awkward.)

I would certainly trust anyone native Anglo-Saxon to say what is the
best word here, so I will happily fold on this.

> Changing gpiod_set_value() to gpiod_set_state(), while leaving
> gpiod_set_raw_value() as is, does not reduce confusion - at least not for
> me.

Sloppy of me, certainly these would need to be renamed
too. The _raw being high/low and the logic accessors
active/inactive.

Yours,
Linus Walleij

2022-09-15 02:54:47

[permalink] [raw]

Subject: Re: [PATCH 2/2] PCI: mvebu: switch to using gpiod API

On Wed, Sep 14, 2022 at 02:10:21PM +0200, Bartosz Golaszewski wrote:
> On Wed, Sep 14, 2022 at 12:35 PM Linus Walleij <[email protected]> wrote:
> >
> > On Wed, Sep 7, 2022 at 12:41 AM Dmitry Torokhov
> > <[email protected]> wrote:
> >
> > > Linus, do you think we should introduce GPIOD_OUT_INACTIVE /
> > > GPIOD_OUT_ACTIVE or GPIOD_OUT_DEASSERTED / GPIOD_OUT_ASSERTED and
> > > deprecate existing GPIOD_OUT_LOW and GPIO_OUT_HIGH?
> >
> > They should rather be replaced everywhere in one go.
> >
> > I think it is just a half-measure unless we also add
> > #define GPIOD_ASSERTED 1
> > #define GPIOD_DEASSERTED 0
> > to be used instead of 1/0 in gpiod_set_value().
> >
>
> We've had that discussion for libgpiod and went with
> GPIOD_VALUE_ACTIVE and GPIOD_VALUE_INACTIVE but...
>
> > It would also imply changing the signature of the function
> > gpiod_set_value() to gpiod_set_state() as we are not
> > really setting a value but a state.
> >
>
> ... now that you've mentioned it it does seem like
> GPIOD_STATE_ACTIVE/INACTIVE makes much more sense as well as naming
> the relevant functions gpiod_line_request_set_line_state() etc.
>

After sleeping on this, I'm even more in disagreement with renaming
value to state.

AIUI, the confusion here is distinguishing between the physical and
logical cases. libgpiod doesn't have this problem, as it only deals
with logical.

By convention, gpiolib uses _raw in the function name when referring to
physical, and otherwise deals with logical. e.g. gpiod_set_value() and
gpiod_set_raw_value(). Changing "value" to "state" is orthogonal to the
actual problem.

Further, "state" is a more broad term than "value", i.e. state may
include a number of attributes, whereas value indicates an individial
attribute.

For lines, state and value are typically synonymous, as the line level
(just to throw in another term for it) is what usually springs to
mind when considering a line. But a line's state could also be
interpreted to include direction, bias, drive,...
You may argue that those form the configuration state, and I would agree
with you - the "configuration" indicating a subset of the overall line
state. i.e. "state" is a broad term.

If you are trying to reduce confusion, switching from a more specific
to a more broad term is going in the wrong direction.

OTOH, I totally agree with the addition of GPIOD_ACTIVE/INACTIVE to be
used for the logical cases, and even a script to apply it globally.
Ideally that script would change both the calls to the logical functions
to use ACTIVE/INACTIVE, and the physical to HIGH/LOW.

Introducing enums for those, and changing the function signatures to
use those rather than int makes sense to me too. Though I'm not sure
why that has to wait until after all users are changed to the new macros.
Would that generate lint warnings?

Cheers,
Kent.

> > I have thought about changing this, but the problem is that I felt
> > it should be accompanied with a change fixing as many users
> > as possible.
> >
> > I think this is one of those occasions where we should merge
> > the new defines, and then send Linus Torvalds a sed script
> > that he can run at the end of the merge window to change all
> > gpiod_set_value(...., 1) -> gpiod_set_state(...., GPIOD_ASSERTED);
> > everywhere.
> >
> > After all users are changed, the GPIOD_ASSERTED/DEASSERTED
> > defined can be turned into an enum.
> >
> > That would be the silver bullet against a lot of confusion IMO.
> >
> > We would need Bartosz' input on this.
> >
>
> We can also let Linus know that we'll do it ourselves late in the
> merge window and send a separate PR on Saturday before rc1?
>
> Bart
>
> > Yours,
> > Linus Walleij

2022-09-15 09:03:20

[permalink] [raw]

Subject: Re: [PATCH 2/2] PCI: mvebu: switch to using gpiod API

On Thu, Sep 15, 2022 at 4:23 AM Kent Gibson <[email protected]> wrote:

> After sleeping on this, I'm even more in disagreement with renaming
> value to state.

OK let's not do that then.

> OTOH, I totally agree with the addition of GPIOD_ACTIVE/INACTIVE to be
> used for the logical cases, and even a script to apply it globally.
> Ideally that script would change both the calls to the logical functions
> to use ACTIVE/INACTIVE, and the physical to HIGH/LOW.

OK we have consensus on this.

> Introducing enums for those, and changing the function signatures to
> use those rather than int makes sense to me too.

Either they can be enum or defined to bool true/false. Not really
sure what is best. But intuitively enum "feels better" for me.

> Though I'm not sure
> why that has to wait until after all users are changed to the new macros.
> Would that generate lint warnings?

I rather want it all to happen at once. One preparatory commit
adding the new types and one sed script to refactor the whole
lot. Not gradual switchover.

The reason is purely administrative: we have too many refactorings
lagging behind, mainly the GPIO descriptors that have been
lagging behind for what is it? 5 years? 10? GPIO irqchips also dragged
out for way too long. We can't keep doing things gradually like
this, it takes too much time and effort.

I don't want any more "in-transition-indefinitely" stuff in the GPIO
subsystem if I can avoid it.

Therefore I would advice to switch it all over at the end of a merge
window and be done with it.

Yours,
Linus Walleij

2022-09-15 09:43:17

[permalink] [raw]

Subject: Re: [PATCH 2/2] PCI: mvebu: switch to using gpiod API

On Thu, Sep 15, 2022 at 10:51:02AM +0200, Linus Walleij wrote:
> On Thu, Sep 15, 2022 at 4:23 AM Kent Gibson <[email protected]> wrote:
>
> > After sleeping on this, I'm even more in disagreement with renaming
> > value to state.
>
> OK let's not do that then.
>
> > OTOH, I totally agree with the addition of GPIOD_ACTIVE/INACTIVE to be
> > used for the logical cases, and even a script to apply it globally.
> > Ideally that script would change both the calls to the logical functions
> > to use ACTIVE/INACTIVE, and the physical to HIGH/LOW.
>
> OK we have consensus on this.
>
> > Introducing enums for those, and changing the function signatures to
> > use those rather than int makes sense to me too.
>
> Either they can be enum or defined to bool true/false. Not really
> sure what is best. But intuitively enum "feels better" for me.
>

Enums work for me - especially if the goal is to differentiate
logical from physical - there should be a distinct enum for each.

> > Though I'm not sure
> > why that has to wait until after all users are changed to the new macros.
> > Would that generate lint warnings?
>
> I rather want it all to happen at once. One preparatory commit
> adding the new types and one sed script to refactor the whole
> lot. Not gradual switchover.
>
> The reason is purely administrative: we have too many refactorings
> lagging behind, mainly the GPIO descriptors that have been
> lagging behind for what is it? 5 years? 10? GPIO irqchips also dragged
> out for way too long. We can't keep doing things gradually like
> this, it takes too much time and effort.
>
> I don't want any more "in-transition-indefinitely" stuff in the GPIO
> subsystem if I can avoid it.
>
> Therefore I would advice to switch it all over at the end of a merge
> window and be done with it.
>

Agreed - do it all at once. My question was specific to the change of the
function signatures to using enums - what is to prevent us doing that
before running the sed script, and have the script switch usage over to
the enums?

Cheers,
Kent.

2022-09-16 07:58:26

by Bartosz Golaszewski

[permalink] [raw]

Subject: Re: [PATCH 2/2] PCI: mvebu: switch to using gpiod API

On Thu, Sep 15, 2022 at 11:30 AM Kent Gibson <[email protected]> wrote:
>
> On Thu, Sep 15, 2022 at 10:51:02AM +0200, Linus Walleij wrote:
> > On Thu, Sep 15, 2022 at 4:23 AM Kent Gibson <[email protected]> wrote:
> >
> > > After sleeping on this, I'm even more in disagreement with renaming
> > > value to state.
> >
> > OK let's not do that then.
> >
> > > OTOH, I totally agree with the addition of GPIOD_ACTIVE/INACTIVE to be
> > > used for the logical cases, and even a script to apply it globally.
> > > Ideally that script would change both the calls to the logical functions
> > > to use ACTIVE/INACTIVE, and the physical to HIGH/LOW.
> >
> > OK we have consensus on this.
> >
> > > Introducing enums for those, and changing the function signatures to
> > > use those rather than int makes sense to me too.
> >
> > Either they can be enum or defined to bool true/false. Not really
> > sure what is best. But intuitively enum "feels better" for me.
> >
>
> Enums work for me - especially if the goal is to differentiate
> logical from physical - there should be a distinct enum for each.
>

We won't even have to change the function signatures if we go with
enums - they already take an int and I'm in general against putting
enum types into function signatures in C as they give you a false
sense of a strong type.

Bart

> > > Though I'm not sure
> > > why that has to wait until after all users are changed to the new macros.
> > > Would that generate lint warnings?
> >
> > I rather want it all to happen at once. One preparatory commit
> > adding the new types and one sed script to refactor the whole
> > lot. Not gradual switchover.
> >
> > The reason is purely administrative: we have too many refactorings
> > lagging behind, mainly the GPIO descriptors that have been
> > lagging behind for what is it? 5 years? 10? GPIO irqchips also dragged
> > out for way too long. We can't keep doing things gradually like
> > this, it takes too much time and effort.
> >
> > I don't want any more "in-transition-indefinitely" stuff in the GPIO
> > subsystem if I can avoid it.
> >
> > Therefore I would advice to switch it all over at the end of a merge
> > window and be done with it.
> >
>
> Agreed - do it all at once. My question was specific to the change of the
> function signatures to using enums - what is to prevent us doing that
> before running the sed script, and have the script switch usage over to
> the enums?
>
> Cheers,
> Kent.

2022-09-18 15:00:44

[permalink] [raw]

Subject: Re: [PATCH 2/2] PCI: mvebu: switch to using gpiod API

On Fri, Sep 16, 2022 at 9:23 AM Bartosz Golaszewski <[email protected]> wrote:

> > Enums work for me - especially if the goal is to differentiate
> > logical from physical - there should be a distinct enum for each.
> >
>
> We won't even have to change the function signatures if we go with
> enums - they already take an int and I'm in general against putting
> enum types into function signatures in C as they give you a false
> sense of a strong type.

We are adding Rust to the kernel soon ;) then it's good for
documentation of what it is actually expected to be.

But I get your point.

Yours,
Linus Walleij

2022-09-19 00:27:30

[permalink] [raw]

Subject: Re: [PATCH 2/2] PCI: mvebu: switch to using gpiod API

On Fri, Sep 16, 2022 at 09:22:59AM +0200, Bartosz Golaszewski wrote:
> On Thu, Sep 15, 2022 at 11:30 AM Kent Gibson <[email protected]> wrote:
> >
> > Enums work for me - especially if the goal is to differentiate
> > logical from physical - there should be a distinct enum for each.
> >
>
> We won't even have to change the function signatures if we go with
> enums - they already take an int and I'm in general against putting
> enum types into function signatures in C as they give you a false
> sense of a strong type.
>

IMO it is far easier to remember that C doesn't range check enums than it
is to remember what specific values are appropriate for a function
accepting an enum as int. A specified type is a strong hint, and unlike
documentation is one that an IDE can parse and provide valid options for.

Passing enums as int is the norm in the kernel, so fair enough to keep
it that way, but that does contribute to the confusion that we are trying
to address here.

Cheers,
Kent.

2022-11-11 15:19:01

by Lorenzo Pieralisi

[permalink] [raw]

Subject: Re: [PATCH 1/2] PCI: histb: switch to using gpiod API

On Tue, Sep 06, 2022 at 01:43:00PM -0700, Dmitry Torokhov wrote:
> This patch switches the driver away from legacy gpio/of_gpio API to
> gpiod API, and removes use of of_get_named_gpio_flags() which I want to
> make private to gpiolib.
>
> Signed-off-by: Dmitry Torokhov <[email protected]>

Should I pick this up ? On patch (2/2) I am not sure we reached
a consensus - please let me know.

Thanks,
Lorenzo

> ---
> drivers/pci/controller/dwc/pcie-histb.c | 39 ++++++++++++-------------
> 1 file changed, 19 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/pci/controller/dwc/pcie-histb.c b/drivers/pci/controller/dwc/pcie-histb.c
> index e2b80f10030d..43c27812dd6d 100644
> --- a/drivers/pci/controller/dwc/pcie-histb.c
> +++ b/drivers/pci/controller/dwc/pcie-histb.c
> @@ -10,11 +10,11 @@
>
> #include <linux/clk.h>
> #include <linux/delay.h>
> +#include <linux/gpio/consumer.h>
> #include <linux/interrupt.h>
> #include <linux/kernel.h>
> #include <linux/module.h>
> #include <linux/of.h>
> -#include <linux/of_gpio.h>
> #include <linux/pci.h>
> #include <linux/phy/phy.h>
> #include <linux/platform_device.h>
> @@ -60,7 +60,7 @@ struct histb_pcie {
> struct reset_control *sys_reset;
> struct reset_control *bus_reset;
> void __iomem *ctrl;
> - int reset_gpio;
> + struct gpio_desc *reset_gpio;
> struct regulator *vpcie;
> };
>
> @@ -212,8 +212,8 @@ static void histb_pcie_host_disable(struct histb_pcie *hipcie)
> clk_disable_unprepare(hipcie->sys_clk);
> clk_disable_unprepare(hipcie->bus_clk);
>
> - if (gpio_is_valid(hipcie->reset_gpio))
> - gpio_set_value_cansleep(hipcie->reset_gpio, 0);
> + if (hipcie->reset_gpio)
> + gpiod_set_value_cansleep(hipcie->reset_gpio, 1);
>
> if (hipcie->vpcie)
> regulator_disable(hipcie->vpcie);
> @@ -235,8 +235,8 @@ static int histb_pcie_host_enable(struct dw_pcie_rp *pp)
> }
> }
>
> - if (gpio_is_valid(hipcie->reset_gpio))
> - gpio_set_value_cansleep(hipcie->reset_gpio, 1);
> + if (hipcie->reset_gpio)
> + gpiod_set_value_cansleep(hipcie->reset_gpio, 0);
>
> ret = clk_prepare_enable(hipcie->bus_clk);
> if (ret) {
> @@ -298,10 +298,7 @@ static int histb_pcie_probe(struct platform_device *pdev)
> struct histb_pcie *hipcie;
> struct dw_pcie *pci;
> struct dw_pcie_rp *pp;
> - struct device_node *np = pdev->dev.of_node;
> struct device *dev = &pdev->dev;
> - enum of_gpio_flags of_flags;
> - unsigned long flag = GPIOF_DIR_OUT;
> int ret;
>
> hipcie = devm_kzalloc(dev, sizeof(*hipcie), GFP_KERNEL);
> @@ -336,17 +333,19 @@ static int histb_pcie_probe(struct platform_device *pdev)
> hipcie->vpcie = NULL;
> }
>
> - hipcie->reset_gpio = of_get_named_gpio_flags(np,
> - "reset-gpios", 0, &of_flags);
> - if (of_flags & OF_GPIO_ACTIVE_LOW)
> - flag |= GPIOF_ACTIVE_LOW;
> - if (gpio_is_valid(hipcie->reset_gpio)) {
> - ret = devm_gpio_request_one(dev, hipcie->reset_gpio,
> - flag, "PCIe device power control");
> - if (ret) {
> - dev_err(dev, "unable to request gpio\n");
> - return ret;
> - }
> + hipcie->reset_gpio = devm_gpiod_get_optional(dev, "reset",
> + GPIOD_OUT_HIGH);
> + ret = PTR_ERR_OR_ZERO(hipcie->reset_gpio);
> + if (ret) {
> + dev_err(dev, "unable to request reset gpio: %d\n", ret);
> + return ret;
> + }
> +
> + ret = gpiod_set_consumer_name(hipcie->reset_gpio,
> + "PCIe device power control");
> + if (ret) {
> + dev_err(dev, "unable to set reset gpio name: %d\n", ret);
> + return ret;
> }
>
> hipcie->aux_clk = devm_clk_get(dev, "aux");
> --
> 2.37.2.789.g6183377224-goog
>

2022-11-11 16:08:47