| Message ID | 20220906204301.3736813-2-dmitry.torokhov@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Lorenzo Pieralisi |
| Headers | show |
| Series | [1/2] PCI: histb: switch to using gpiod API | expand |
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 <dmitry.torokhov@gmail.com> > --- > 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 >
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 <dmitry.torokhov@gmail.com> > > --- > > 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.
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.
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 <dmitry.torokhov@gmail.com> > > > --- > > > 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
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.
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 <dmitry.torokhov@gmail.com> > > > > --- > > > > 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
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 <dmitry.torokhov@gmail.com> > > > > > --- > > > > > 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
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.
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/202209071255.xlxq6qnE-lkp@intel.com/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 <lkp@intel.com>
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
On Tue, Sep 6, 2022 at 11:16 PM Pali Rohár <pali@kernel.org> 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
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.
On Wed, Sep 7, 2022 at 12:41 AM Dmitry Torokhov <dmitry.torokhov@gmail.com> 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
On Wed, Sep 14, 2022 at 12:35 PM Linus Walleij <linus.walleij@linaro.org> wrote: > > On Wed, Sep 7, 2022 at 12:41 AM Dmitry Torokhov > <dmitry.torokhov@gmail.com> 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
On Wed, Sep 14, 2022 at 2:10 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote: > On Wed, Sep 14, 2022 at 12:35 PM Linus Walleij <linus.walleij@linaro.org> 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
On Wed, Sep 14, 2022 at 02:10:21PM +0200, Bartosz Golaszewski wrote: > On Wed, Sep 14, 2022 at 12:35 PM Linus Walleij <linus.walleij@linaro.org> wrote: > > > > On Wed, Sep 7, 2022 at 12:41 AM Dmitry Torokhov > > <dmitry.torokhov@gmail.com> 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
On Wed, Sep 14, 2022 at 3:00 PM Kent Gibson <warthog618@gmail.com> 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
On Wed, Sep 14, 2022 at 02:10:21PM +0200, Bartosz Golaszewski wrote: > On Wed, Sep 14, 2022 at 12:35 PM Linus Walleij <linus.walleij@linaro.org> wrote: > > > > On Wed, Sep 7, 2022 at 12:41 AM Dmitry Torokhov > > <dmitry.torokhov@gmail.com> 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
On Thu, Sep 15, 2022 at 4:23 AM Kent Gibson <warthog618@gmail.com> 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
On Thu, Sep 15, 2022 at 10:51:02AM +0200, Linus Walleij wrote: > On Thu, Sep 15, 2022 at 4:23 AM Kent Gibson <warthog618@gmail.com> 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.
On Thu, Sep 15, 2022 at 11:30 AM Kent Gibson <warthog618@gmail.com> 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 <warthog618@gmail.com> 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.
On Fri, Sep 16, 2022 at 9:23 AM Bartosz Golaszewski <brgl@bgdev.pl> 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
On Fri, Sep 16, 2022 at 09:22:59AM +0200, Bartosz Golaszewski wrote: > On Thu, Sep 15, 2022 at 11:30 AM Kent Gibson <warthog618@gmail.com> 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.
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,
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 <dmitry.torokhov@gmail.com> --- drivers/pci/controller/pci-mvebu.c | 48 +++++++++--------------------- 1 file changed, 14 insertions(+), 34 deletions(-)