gpiolib: fix GPIO <-> pin mapping registration
Subsystems (5)
v2 cover gpiolib: fix GPIO <-> pin mapping registration
This is the ordering fix for GPIO <-> pin mapping registration to allow delegation of IRQ chip to the GPIO library. Two drivers are actually converted to use the new callback followed by switching to use of irqchip structure inside gpiochip.
The series has been tested on Merrifield device (though no AEI) and on Thinkpad 10 where 4 events are registered, one of which has reasonable amount of interrupts coming from battery.
First two patches are definitely good for v5.4 cycle (and have Fixes tag) while the rest can be applied for v5.5 if I didn't miss anything. So, they can go like patches 1-3 -> v5.4 (gpio/fixes) and to gpio/for-next and to pinctrl/for-next patches 4-5 -> v5.5 (gpio/for-next)
patches 6-7 -> v5.5 (pinctrl/for-next)
Perhaps it would require to wait next v5.4 rc in order to avoid dependency issues.
Changelog v2 (resent):
- fixed cover letter
- resent with ML included
Changelog v2:
- added fixes tag to the first patch
- reworked commit message for the second patch
- added baytrail conversion
- tested on ThinkPad 10
Andy Shevchenko (6):
gpiolib: No need to call gpiochip_remove_pin_ranges() twice
gpiolib: Introduce ->add_pin_ranges() callback
gpio: merrifield: Add GPIO <-> pin mapping ranges via callback
gpio: merrifield: Pass irqchip when adding gpiochip
pinctrl: baytrail: Add GPIO <-> pin mapping ranges via callback
pinctrl: baytrail: Pass irqchip when adding gpiochip
Linus Walleij (1):
gpiolib: Switch order of valid mask and hw init
drivers/gpio/gpio-merrifield.c | 76 ++++++++++++++---------- drivers/gpio/gpiolib-of.c | 5 +- drivers/gpio/gpiolib.c | 19 +++++- drivers/pinctrl/intel/pinctrl-baytrail.c | 64 ++++++++++++-------- include/linux/gpio/driver.h | 5 ++ 5 files changed, 107 insertions(+), 62 deletions(-)
Signature · 1 line
2.24.0.rc1
PATCH v2 1/7 gpiolib: Switch order of valid mask and hw init
From: Linus Walleij <[email protected]>
The GPIO irqchip needs to initialize the valid mask before initializing the IRQ hardware, because sometimes the latter require the former to be executed first.
--- drivers/gpio/gpiolib.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/gpio/gpiolib.c +2 -2
@@ -1411,11 +1411,11 @@ int gpiochip_add_data_with_key(struct gpio_chip *chip, void *data,machine_gpiochip_add (chip); - ret =gpiochip_irqchip_init_hw (chip); + ret =gpiochip_irqchip_init_valid_mask (chip);if (ret) goto err_remove_acpi_chip; - ret =gpiochip_irqchip_init_valid_mask (chip); + ret =gpiochip_irqchip_init_hw (chip);if (ret) goto err_remove_acpi_chip;
Signature · 1 line
2.24.0.rc1
PATCH v2 2/7 gpiolib: No need to call gpiochip_remove_pin_ranges() twice
In reply to Andy Shevchenkoof_gpiochip_add(), when fails, calls gpiochip_remove_pin_ranges().
ADD:
gpiochip_add_data_with_key() ->
of_gpiochip_add() -> (ERROR path)
gpiochip_remove_pin_ranges()At the same time of_gpiochip_remove() calls exactly the above mentioned function unconditionally and so does gpiochip_remove().
REMOVE:
gpiochip_remove() ->
gpiochip_remove_pin_ranges()
of_gpiochip_remove() ->
gpiochip_remove_pin_ranges()Since gpiochip_remove() calls gpiochip_remove_pin_ranges() unconditionally, we have duplicate call to the same function when it's not necessary.
Move gpiochip_remove_pin_ranges() from of_gpiochip_add() to gpiochip_add() to avoid duplicate calls and be consistent with the explicit call in gpiochip_remove().
Depends-on: f7299d441a4d ("gpio: of: Fix of_gpiochip_add() error path")
--- drivers/gpio/gpiolib-of.c | 5 +---- drivers/gpio/gpiolib.c | 3 ++- 2 files changed, 3 insertions(+), 5 deletions(-)
drivers/gpio/gpiolib-of.c +1 -4
@@ -885,16 +885,13 @@ int of_gpiochip_add(struct gpio_chip *chip)of_node_get (chip->of_node); ret =of_gpiochip_scan_gpios (chip); -if (ret) { +if (ret)of_node_put (chip->of_node); -gpiochip_remove_pin_ranges (chip); - }return ret; }void of_gpiochip_remove (struct gpio_chip *chip) { -gpiochip_remove_pin_ranges (chip);of_node_put (chip->of_node); }
drivers/gpio/gpiolib.c +2 -1
@@ -1448,6 +1448,7 @@ int gpiochip_add_data_with_key(struct gpio_chip *chip, void *data,gpiochip_free_hogs (chip);of_gpiochip_remove (chip); err_free_gpiochip_mask: +gpiochip_remove_pin_ranges (chip);gpiochip_free_valid_mask (chip); err_remove_from_list:spin_lock_irqsave (&gpio_lock, flags);
@@ -1503,8 +1504,8 @@ void gpiochip_remove(struct gpio_chip *chip) gdev->chip =NULL ;gpiochip_irqchip_remove (chip);acpi_gpiochip_remove (chip); -gpiochip_remove_pin_ranges (chip);of_gpiochip_remove (chip); +gpiochip_remove_pin_ranges (chip);gpiochip_free_valid_mask (chip);/* * We accept no more calls into the driver from this point, so
Signature · 1 line
2.24.0.rc1
PATCH v2 3/7 gpiolib: Introduce ->add_pin_ranges() callback
In reply to Andy ShevchenkoWhen IRQ chip is being added by GPIO library, the ACPI based platform expects GPIO <-> pin mapping ranges to be initialized in order to correctly initialize ACPI event mechanism on affected platforms. Unfortunately this step is missed.
Introduce ->add_pin_ranges() callback to fill the above mentioned gap.
--- drivers/gpio/gpiolib.c | 12 ++++++++++++ include/linux/gpio/driver.h | 5 +++++ 2 files changed, 17 insertions(+)
drivers/gpio/gpiolib.c +12 -0
@@ -390,6 +390,14 @@ static void gpiochip_free_valid_mask(struct gpio_chip *gpiochip) gpiochip->valid_mask =NULL ; } +static int gpiochip_add_pin_ranges (struct gpio_chip *gc) +{ +if (gc->add_pin_ranges) +return gc->add_pin_ranges (gc); + +return 0 ; +} +bool gpiochip_line_is_valid (const struct gpio_chip *gpiochip,unsigned int offset) {
@@ -1407,6 +1415,10 @@ int gpiochip_add_data_with_key(struct gpio_chip *chip, void *data, } } + ret =gpiochip_add_pin_ranges (chip); +if (ret) + goto err_remove_of_chip; +acpi_gpiochip_add (chip);machine_gpiochip_add (chip);
include/linux/gpio/driver.h +5 -0
@@ -287,6 +287,9 @@ struct gpio_irq_chip {* state (such as pullup/pulldown configuration). * @init_valid_mask: optional routine to initialize @valid_mask, to be used if * not all GPIOs are valid. +* @add_pin_ranges: optional routine to initialize pin ranges, to be used when +* requires special mapping of the pins that provides GPIO functionality. +* It is called after adding GPIO chip and before adding IRQ chip. * @base: identifies the first GPIO number handled by this chip; * or, if negative during registration, requests dynamic ID allocation. * DEPRECATION: providing anything non-negative and nailing the base
@@ -377,6 +380,8 @@ struct gpio_chip {unsigned long *valid_mask, unsigned int ngpios); +int (*add_pin_ranges)(struct gpio_chip *chip); +int base;u16 ngpio;const char *const *names;
Signature · 1 line
2.24.0.rc1
Re: [PATCH v2 3/7] gpiolib: Introduce ->add_pin_ranges() callback
Quoted 7 lines · Andy Shevchenko
When IRQ chip is being added by GPIO library, the ACPI based platform expects GPIO <-> pin mapping ranges to be initialized in order to correctly initialize ACPI event mechanism on affected platforms. Unfortunately this step is missed.
Introduce ->add_pin_ranges() callback to fill the above mentioned gap.
Signed-off-by Andy Shevchenko <[email protected]>
Re: [PATCH v2 3/7] gpiolib: Introduce ->add_pin_ranges() callback
In reply to Andy ShevchenkoQuoted 7 lines · Andy Shevchenko
When IRQ chip is being added by GPIO library, the ACPI based platform expects GPIO <-> pin mapping ranges to be initialized in order to correctly initialize ACPI event mechanism on affected platforms. Unfortunately this step is missed.
Introduce ->add_pin_ranges() callback to fill the above mentioned gap.
Signed-off-by Andy Shevchenko <[email protected]>
I also think DT drivers can benefit from this.
I expect this in a big pull request :)
Yours,
Linus Walleij
Re: [PATCH v2 3/7] gpiolib: Introduce ->add_pin_ranges() callback
Quoted 7 lines · Linus Walleij
Quoted 4 lines · Andy Shevchenko
When IRQ chip is being added by GPIO library, the ACPI based platform expects GPIO <-> pin mapping ranges to be initialized in order to correctly initialize ACPI event mechanism on affected platforms. Unfortunately this step is missed.
Quoted 5 lines
Quoted 3 lines
Introduce ->add_pin_ranges() callback to fill the above mentioned gap.
Signed-off-by Andy Shevchenko <[email protected]>Reviewed-by Linus Walleij <[email protected]>
Thank you.
Quoted 2 lines
I also think DT drivers can benefit from this.
I expect this in a big pull request :)
You mean GPIO library one? Pin control is the other one and it seems that part of Intel pin control (despite Lynxpoint case) will be split between v5.5 and v5.6 due to this very patch as dependency.
Signature · 2 lines
With Best Regards, Andy Shevchenko
Re: [PATCH v2 3/7] gpiolib: Introduce ->add_pin_ranges() callback
Quoted 14 lines · Andy Shevchenko
Quoted 7 lines · Linus Walleij
Quoted 4 lines · Andy Shevchenko
When IRQ chip is being added by GPIO library, the ACPI based platform expects GPIO <-> pin mapping ranges to be initialized in order to correctly initialize ACPI event mechanism on affected platforms. Unfortunately this step is missed.
Quoted 5 lines
Quoted 3 lines
Introduce ->add_pin_ranges() callback to fill the above mentioned gap.
Signed-off-by Andy Shevchenko <[email protected]>Reviewed-by Linus Walleij <[email protected]>
Quoted 8 lines
Thank you.
Quoted 2 lines
I also think DT drivers can benefit from this.
I expect this in a big pull request :)You mean GPIO library one? Pin control is the other one and it seems that part of Intel pin control (despite Lynxpoint case) will be split between v5.5 and v5.6 due to this very patch as dependency.
No need for that I think, we can just create some shared baseline and pull that into both trees? (If it doesn't get too messy...)
Yours,
Linus Walleij
Re: [PATCH v2 3/7] gpiolib: Introduce ->add_pin_ranges() callback
Quoted 21 lines · Linus Walleij
Quoted 19 lines · Andy Shevchenko
Quoted 7 lines · Linus Walleij
Quoted 4 lines · Andy Shevchenko
When IRQ chip is being added by GPIO library, the ACPI based platform expects GPIO <-> pin mapping ranges to be initialized in order to correctly initialize ACPI event mechanism on affected platforms. Unfortunately this step is missed.
Quoted 5 lines
Quoted 3 lines
Introduce ->add_pin_ranges() callback to fill the above mentioned gap.
Signed-off-by Andy Shevchenko <[email protected]>Reviewed-by Linus Walleij <[email protected]>Thank you.
Quoted 2 lines
I also think DT drivers can benefit from this.
I expect this in a big pull request :)
Quoted 6 lines
Quoted 3 lines
You mean GPIO library one? Pin control is the other one and it seems that part of Intel pin control (despite Lynxpoint case) will be split between v5.5 and v5.6 due to this very patch as dependency.
No need for that I think, we can just create some shared baseline and pull that into both trees? (If it doesn't get too messy...)
Anyway, I sent two PRs, and looking forward for them being accepted. Then we can continue based on updated repositories.
Signature · 2 lines
With Best Regards, Andy Shevchenko
PATCH v2 4/7 gpio: merrifield: Add GPIO <-> pin mapping ranges via callback
In reply to Andy ShevchenkoWhen IRQ chip is instantiated via GPIO library flow, the few functions, in particular the ACPI event registration mechanism, on some of ACPI based platforms expect that the pin ranges are initialized to that point.
Add GPIO <-> pin mapping ranges via callback in the GPIO library flow.
--- drivers/gpio/gpio-merrifield.c | 43 ++++++++++++++++++++-------------- 1 file changed, 26 insertions(+), 17 deletions(-)
drivers/gpio/gpio-merrifield.c +26 -17
@@ -393,14 +393,36 @@ static const char *mrfld_gpio_get_pinctrl_dev_name(struct mrfld_gpio *priv)return name; } -static int mrfld_gpio_probe (struct pci_dev *pdev,const struct pci_device_id *id) +static int mrfld_gpio_add_pin_ranges (struct gpio_chip *chip) { +struct mrfld_gpio *priv =gpiochip_get_data (chip);const struct mrfld_gpio_pinrange *range;const char *pinctrl_dev_name; +unsigned int i; +int retval; + + pinctrl_dev_name =mrfld_gpio_get_pinctrl_dev_name (priv); +for (i =0 ; i <ARRAY_SIZE (mrfld_gpio_ranges); i++) { + range = &mrfld_gpio_ranges[i]; + retval =gpiochip_add_pin_range (&priv->chip, + pinctrl_dev_name, + range->gpio_base, + range->pin_base, + range->npins); +if (retval) { +dev_err (priv->dev,"failed to add GPIO pin range\n" ); +return retval; + } + } + +return 0 ; +} + +static int mrfld_gpio_probe (struct pci_dev *pdev,const struct pci_device_id *id) +{struct mrfld_gpio *priv;u32 gpio_base, irq_base;void __iomem *base; -unsigned int i;int retval; retval =pcim_enable_device (pdev);
@@ -441,30 +463,16 @@ static int mrfld_gpio_probe(struct pci_dev *pdev, const struct pci_device_id *id priv->chip.base = gpio_base; priv->chip.ngpio =MRFLD_NGPIO ; priv->chip.can_sleep = false; + priv->chip.add_pin_ranges = mrfld_gpio_add_pin_ranges;raw_spin_lock_init (&priv->lock); -pci_set_drvdata (pdev, priv); retval =devm_gpiochip_add_data (&pdev->dev, &priv->chip, priv);if (retval) {dev_err (&pdev->dev,"gpiochip_add error %d\n" , retval);return retval; } - pinctrl_dev_name =mrfld_gpio_get_pinctrl_dev_name (priv); -for (i =0 ; i <ARRAY_SIZE (mrfld_gpio_ranges); i++) { - range = &mrfld_gpio_ranges[i]; - retval =gpiochip_add_pin_range (&priv->chip, - pinctrl_dev_name, - range->gpio_base, - range->pin_base, - range->npins); -if (retval) { -dev_err (&pdev->dev,"failed to add GPIO pin range\n" ); -return retval; - } - } - retval =gpiochip_irqchip_add (&priv->chip, &mrfld_irqchip, irq_base, handle_bad_irq,IRQ_TYPE_NONE );if (retval) {
@@ -477,6 +485,7 @@ static int mrfld_gpio_probe(struct pci_dev *pdev, const struct pci_device_id *idgpiochip_set_chained_irqchip (&priv->chip, &mrfld_irqchip, pdev->irq, mrfld_irq_handler); +pci_set_drvdata (pdev, priv);return 0 ; }
Signature · 1 line
2.24.0.rc1
Re: [PATCH v2 4/7] gpio: merrifield: Add GPIO <-> pin mapping ranges via callback
Quoted 7 lines · Andy Shevchenko
When IRQ chip is instantiated via GPIO library flow, the few functions, in particular the ACPI event registration mechanism, on some of ACPI based platforms expect that the pin ranges are initialized to that point.
Add GPIO <-> pin mapping ranges via callback in the GPIO library flow.
Signed-off-by Andy Shevchenko <[email protected]>
One minor comment below.
Quoted 19 lines
--- drivers/gpio/gpio-merrifield.c | 43 ++++++++++++++++++++-------------- 1 file changed, 26 insertions(+), 17 deletions(-)drivers/gpio/gpio-merrifield.c +3
-1@@ -393,14 +393,36 @@ static const char *mrfld_gpio_get_pinctrl_dev_name(struct mrfld_gpio *priv)return name; } -static int mrfld_gpio_probe (struct pci_dev *pdev,const struct pci_device_id *id) +static int mrfld_gpio_add_pin_ranges (struct gpio_chip *chip) { +struct mrfld_gpio *priv =gpiochip_get_data (chip);const struct mrfld_gpio_pinrange *range;const char *pinctrl_dev_name; +unsigned int i;
drivers/gpio/gpio-merrifield.c static const char *mrfld_gpio_get_pinctrl_dev_name(struct mrfld_gpio *priv) · line 411
Quoted 10 lines
drivers/gpio/gpio-merrifield.c +10
-0+int retval; + + pinctrl_dev_name =mrfld_gpio_get_pinctrl_dev_name (priv); +for (i =0 ; i <ARRAY_SIZE (mrfld_gpio_ranges); i++) { + range = &mrfld_gpio_ranges[i]; + retval =gpiochip_add_pin_range (&priv->chip, + pinctrl_dev_name, + range->gpio_base, + range->pin_base, + range->npins);
IMHO the below looks slightly better:
ret = gpiochip_add_pin_range(&priv->chip, pinctrl_dev_name, range->gpio_base, range->pin_base, range->npins);
Quoted 8 lines
drivers/gpio/gpio-merrifield.c +8
-0+if (retval) { +dev_err (priv->dev,"failed to add GPIO pin range\n" ); +return retval; + } + } + +return 0 ; +}
Re: [PATCH v2 4/7] gpio: merrifield: Add GPIO <-> pin mapping ranges via callback
Quoted 10 lines · Mika Westerberg
Quoted 7 lines · Andy Shevchenko
When IRQ chip is instantiated via GPIO library flow, the few functions, in particular the ACPI event registration mechanism, on some of ACPI based platforms expect that the pin ranges are initialized to that point.
Add GPIO <-> pin mapping ranges via callback in the GPIO library flow.
Signed-off-by Andy Shevchenko <[email protected]>Reviewed-by Mika Westerberg <[email protected]>
Thanks.
Quoted 10 lines
drivers/gpio/gpio-merrifield.c static const char *mrfld_gpio_get_pinctrl_dev_name(struct mrfld_gpio *priv) · line 411
Quoted 5 lines
drivers/gpio/gpio-merrifield.c +5
-0+ retval =gpiochip_add_pin_range (&priv->chip, + pinctrl_dev_name, + range->gpio_base, + range->pin_base, + range->npins);IMHO the below looks slightly better:
ret = gpiochip_add_pin_range(&priv->chip, pinctrl_dev_name,
I did this one...
Quoted 1 line
range->gpio_base, range->pin_base,
...but this, since over going thru 80 limit.
Quoted 1 line
range->npins);
Signature · 2 lines
With Best Regards, Andy Shevchenko
PATCH v2 5/7 gpio: merrifield: Pass irqchip when adding gpiochip
In reply to Andy ShevchenkoWe need to convert all old gpio irqchips to pass the irqchip setup along when adding the gpio_chip. For more info see drivers/gpio/TODO.
For chained irqchips this is a pretty straight-forward conversion.
--- drivers/gpio/gpio-merrifield.c | 33 ++++++++++++++++++++------------- 1 file changed, 20 insertions(+), 13 deletions(-)
drivers/gpio/gpio-merrifield.c +20 -13
@@ -362,8 +362,9 @@ static void mrfld_irq_handler(struct irq_desc *desc)chained_irq_exit (irqchip, desc); } -static void mrfld_irq_init_hw (struct mrfld_gpio *priv) +static int mrfld_irq_init_hw (struct gpio_chip *chip) { +struct mrfld_gpio *priv =gpiochip_get_data (chip);void __iomem *reg;unsigned int base;
@@ -375,6 +376,8 @@ static void mrfld_irq_init_hw(struct mrfld_gpio *priv) reg =gpio_reg (&priv->chip, base,GFER );writel (0 , reg); } + +return 0 ; }static const char *mrfld_gpio_get_pinctrl_dev_name (struct mrfld_gpio *priv)
@@ -420,6 +423,7 @@ static int mrfld_gpio_add_pin_ranges(struct gpio_chip *chip)static int mrfld_gpio_probe (struct pci_dev *pdev,const struct pci_device_id *id) { +struct gpio_irq_chip *girq;struct mrfld_gpio *priv;u32 gpio_base, irq_base;void __iomem *base;
@@ -467,24 +471,27 @@ static int mrfld_gpio_probe(struct pci_dev *pdev, const struct pci_device_id *idraw_spin_lock_init (&priv->lock); + girq = &priv->chip.irq; + girq->chip = &mrfld_irqchip; + girq->init_hw = mrfld_irq_init_hw; + girq->parent_handler = mrfld_irq_handler; + girq->num_parents =1 ; + girq->parents =devm_kcalloc (&pdev->dev, girq->num_parents, +sizeof (*girq->parents), +GFP_KERNEL ); +if (!girq->parents) +return -ENOMEM ; + girq->parents[0 ] = pdev->irq; + girq->first = irq_base; + girq->default_type =IRQ_TYPE_NONE ; + girq->handler = handle_bad_irq; + retval =devm_gpiochip_add_data (&pdev->dev, &priv->chip, priv);if (retval) {dev_err (&pdev->dev,"gpiochip_add error %d\n" , retval);return retval; } - retval =gpiochip_irqchip_add (&priv->chip, &mrfld_irqchip, irq_base, - handle_bad_irq,IRQ_TYPE_NONE ); -if (retval) { -dev_err (&pdev->dev,"could not connect irqchip to gpiochip\n" ); -return retval; - } - -mrfld_irq_init_hw (priv); - -gpiochip_set_chained_irqchip (&priv->chip, &mrfld_irqchip, pdev->irq, - mrfld_irq_handler); -pci_set_drvdata (pdev, priv);return 0 ; }
Signature · 1 line
2.24.0.rc1
Re: [PATCH v2 5/7] gpio: merrifield: Pass irqchip when adding gpiochip
Quoted 8 lines · Andy Shevchenko
We need to convert all old gpio irqchips to pass the irqchip setup along when adding the
gpio_chip. For more info seedrivers/gpio/TODO.For chained irqchips this is a pretty straight-forward conversion.
Cc Andy Shevchenko <[email protected]>Cc Mika Westerberg <[email protected]>
Tiny nit below.
Quoted 42 lines
Cc David Cohen <[email protected]>Cc Thierry Reding <[email protected]>Signed-off-by Linus Walleij <[email protected]>Signed-off-by Andy Shevchenko <[email protected]>--- drivers/gpio/gpio-merrifield.c | 33 ++++++++++++++++++++------------- 1 file changed, 20 insertions(+), 13 deletions(-)drivers/gpio/gpio-merrifield.c +5
-1@@ -362,8 +362,9 @@ static void mrfld_irq_handler(struct irq_desc *desc)chained_irq_exit (irqchip, desc); } -static void mrfld_irq_init_hw (struct mrfld_gpio *priv) +static int mrfld_irq_init_hw (struct gpio_chip *chip) { +struct mrfld_gpio *priv =gpiochip_get_data (chip);void __iomem *reg;unsigned int base;@@ -375,6 +376,8 @@ static void mrfld_irq_init_hw(struct mrfld_gpio *priv) reg =gpio_reg (&priv->chip, base,GFER );writel (0 , reg); } + +return 0 ; }static const char *mrfld_gpio_get_pinctrl_dev_name (struct mrfld_gpio *priv)@@ -420,6 +423,7 @@ static int mrfld_gpio_add_pin_ranges(struct gpio_chip *chip)static int mrfld_gpio_probe (struct pci_dev *pdev,const struct pci_device_id *id) { +struct gpio_irq_chip *girq;struct mrfld_gpio *priv;u32 gpio_base, irq_base;void __iomem *base;@@ -467,24 +471,27 @@ static int mrfld_gpio_probe(struct pci_dev *pdev, const struct pci_device_id *id
drivers/gpio/gpio-merrifield.c static int mrfld_gpio_probe(struct pci_dev *pdev, const struct pci_device_id *id · line 481
Quoted 10 lines
drivers/gpio/gpio-merrifield.c +8
-0raw_spin_lock_init (&priv->lock); + girq = &priv->chip.irq; + girq->chip = &mrfld_irqchip; + girq->init_hw = mrfld_irq_init_hw; + girq->parent_handler = mrfld_irq_handler; + girq->num_parents =1 ; + girq->parents =devm_kcalloc (&pdev->dev, girq->num_parents, +sizeof (*girq->parents), +GFP_KERNEL );
Maybe write it using only two lines:
girq->parents = devm_kcalloc(&pdev->dev, girq->num_parents, sizeof(*girq->parents), GFP_KERNEL);
Quoted 30 lines
drivers/gpio/gpio-merrifield.c +7
-12+if (!girq->parents) +return -ENOMEM ; + girq->parents[0 ] = pdev->irq; + girq->first = irq_base; + girq->default_type =IRQ_TYPE_NONE ; + girq->handler = handle_bad_irq; + retval =devm_gpiochip_add_data (&pdev->dev, &priv->chip, priv);if (retval) {dev_err (&pdev->dev,"gpiochip_add error %d\n" , retval);return retval; } - retval =gpiochip_irqchip_add (&priv->chip, &mrfld_irqchip, irq_base, - handle_bad_irq,IRQ_TYPE_NONE ); -if (retval) { -dev_err (&pdev->dev,"could not connect irqchip to gpiochip\n" ); -return retval; - } - -mrfld_irq_init_hw (priv); - -gpiochip_set_chained_irqchip (&priv->chip, &mrfld_irqchip, pdev->irq, - mrfld_irq_handler); -pci_set_drvdata (pdev, priv);return 0 ; }Signature · 1 line
2.24.0.rc1
PATCH v2 6/7 pinctrl: baytrail: Add GPIO <-> pin mapping ranges via callback
In reply to Andy ShevchenkoWhen IRQ chip is instantiated via GPIO library flow, the few functions, in particular the ACPI event registration mechanism, on some of ACPI based platforms expect that the pin ranges are initialized to that point.
Add GPIO <-> pin mapping ranges via callback in the GPIO library flow.
--- drivers/pinctrl/intel/pinctrl-baytrail.c | 21 ++++++++++++++------- 1 file changed, 14 insertions(+), 7 deletions(-)
drivers/pinctrl/intel/pinctrl-baytrail.c +14 -7
@@ -1506,6 +1506,19 @@ static void byt_gpio_irq_init_hw(struct byt_gpio *vg) } } +static int byt_gpio_add_pin_ranges (struct gpio_chip *chip) +{ +struct byt_gpio *vg =gpiochip_get_data (chip); +int ret; + + ret =gpiochip_add_pin_range (&vg->chip,dev_name (&vg->pdev->dev), +0 ,0 , vg->soc_data->npins); +if (ret) +dev_err (&vg->pdev->dev,"failed to add GPIO pin range\n" ); + +return ret; +} +static int byt_gpio_probe (struct byt_gpio *vg) {struct gpio_chip *gc;
@@ -1518,6 +1531,7 @@ static int byt_gpio_probe(struct byt_gpio *vg) gc->label =dev_name (&vg->pdev->dev); gc->base =-1 ; gc->can_sleep = false; + gc->add_pin_ranges = byt_gpio_add_pin_ranges; gc->parent = &vg->pdev->dev; gc->ngpio = vg->soc_data->npins; gc->irq.init_valid_mask = byt_init_irq_valid_mask;
@@ -1534,13 +1548,6 @@ static int byt_gpio_probe(struct byt_gpio *vg)return ret; } - ret =gpiochip_add_pin_range (&vg->chip,dev_name (&vg->pdev->dev), -0 ,0 , vg->soc_data->npins); -if (ret) { -dev_err (&vg->pdev->dev,"failed to add GPIO pin range\n" ); -return ret; - } -/* set up interrupts */ irq_rc =platform_get_resource (vg->pdev,IORESOURCE_IRQ ,0 );if (irq_rc && irq_rc->start) {
Signature · 1 line
2.24.0.rc1
Re: [PATCH v2 6/7] pinctrl: baytrail: Add GPIO <-> pin mapping ranges via callback
Quoted 7 lines · Andy Shevchenko
When IRQ chip is instantiated via GPIO library flow, the few functions, in particular the ACPI event registration mechanism, on some of ACPI based platforms expect that the pin ranges are initialized to that point.
Add GPIO <-> pin mapping ranges via callback in the GPIO library flow.
Signed-off-by Andy Shevchenko <[email protected]>
PATCH v2 7/7 pinctrl: baytrail: Pass irqchip when adding gpiochip
In reply to Andy ShevchenkoWe need to convert all old gpio irqchips to pass the irqchip setup along when adding the gpio_chip. For more info see drivers/gpio/TODO.
For chained irqchips this is a pretty straight-forward conversion.
--- drivers/pinctrl/intel/pinctrl-baytrail.c | 43 ++++++++++++++---------- 1 file changed, 25 insertions(+), 18 deletions(-)
drivers/pinctrl/intel/pinctrl-baytrail.c +25 -18
@@ -1450,9 +1450,9 @@ static void byt_init_irq_valid_mask(struct gpio_chip *chip,*/ } -static void byt_gpio_irq_init_hw (struct byt_gpio *vg) +static int byt_gpio_irq_init_hw (struct gpio_chip *chip) { -struct gpio_chip *gc = &vg->chip; +struct byt_gpio *vg =gpiochip_get_data (chip);struct device *dev = &vg->pdev->dev;void __iomem *reg;u32 base, value;
@@ -1476,7 +1476,7 @@ static void byt_gpio_irq_init_hw(struct byt_gpio *vg) value =readl (reg);if (value &BYT_DIRECT_IRQ_EN ) { -clear_bit (i, gc->irq.valid_mask); +clear_bit (i, chip->irq.valid_mask);dev_dbg (dev,"excluding GPIO %d from IRQ domain\n" , i); }else if ((value &BYT_PIN_MUX ) ==byt_get_gpio_mux (vg, i)) {byt_gpio_clear_triggering (vg, i);
@@ -1504,6 +1504,8 @@ static void byt_gpio_irq_init_hw(struct byt_gpio *vg)"GPIO interrupt error, pins misconfigured. INT_STAT%u: 0x%08x\n" , base / 32, value); } + +return 0 ; }static int byt_gpio_add_pin_ranges (struct gpio_chip *chip)
@@ -1542,26 +1544,31 @@ static int byt_gpio_probe(struct byt_gpio *vg)if (!vg->saved_context)return -ENOMEM ;#endif - ret =devm_gpiochip_add_data (&vg->pdev->dev, gc, vg); -if (ret) { -dev_err (&vg->pdev->dev,"failed adding byt-gpio chip\n" ); -return ret; - }/* set up interrupts */ irq_rc =platform_get_resource (vg->pdev,IORESOURCE_IRQ ,0 );if (irq_rc && irq_rc->start) { -byt_gpio_irq_init_hw (vg); - ret =gpiochip_irqchip_add (gc, &byt_irqchip,0 , - handle_bad_irq,IRQ_TYPE_NONE ); -if (ret) { -dev_err (&vg->pdev->dev,"failed to add irqchip\n" ); -return ret; - } +struct gpio_irq_chip *girq; + + girq = &gc->irq; + girq->chip = &byt_irqchip; + girq->init_hw = byt_gpio_irq_init_hw; + girq->parent_handler = byt_gpio_irq_handler; + girq->num_parents =1 ; + girq->parents =devm_kcalloc (&vg->pdev->dev,1 , +sizeof (*girq->parents), +GFP_KERNEL ); +if (!girq->parents) +return -ENOMEM ; + girq->parents[0 ] = (unsigned int )irq_rc->start; + girq->default_type =IRQ_TYPE_NONE ; + girq->handler = handle_bad_irq; + } -gpiochip_set_chained_irqchip (gc, &byt_irqchip, - (unsigned )irq_rc->start, - byt_gpio_irq_handler); + ret =devm_gpiochip_add_data (&vg->pdev->dev, gc, vg); +if (ret) { +dev_err (&vg->pdev->dev,"failed adding byt-gpio chip\n" ); +return ret; }return ret;
Signature · 1 line
2.24.0.rc1
Re: [PATCH v2 7/7] pinctrl: baytrail: Pass irqchip when adding gpiochip
Quoted 70 lines · Andy Shevchenko
We need to convert all old gpio irqchips to pass the irqchip setup along when adding the
gpio_chip. For more info seedrivers/gpio/TODO.For chained irqchips this is a pretty straight-forward conversion.
Cc Andy Shevchenko <[email protected]>Cc Mika Westerberg <[email protected]>Cc Thierry Reding <[email protected]>Signed-off-by Linus Walleij <[email protected]>Acked-by Mika Westerberg <[email protected]>Signed-off-by Andy Shevchenko <[email protected]>--- drivers/pinctrl/intel/pinctrl-baytrail.c | 43 ++++++++++++++---------- 1 file changed, 25 insertions(+), 18 deletions(-)drivers/pinctrl/intel/pinctrl-baytrail.c +5
-15@@ -1450,9 +1450,9 @@ static void byt_init_irq_valid_mask(struct gpio_chip *chip,*/ } -static void byt_gpio_irq_init_hw (struct byt_gpio *vg) +static int byt_gpio_irq_init_hw (struct gpio_chip *chip) { -struct gpio_chip *gc = &vg->chip; +struct byt_gpio *vg =gpiochip_get_data (chip);struct device *dev = &vg->pdev->dev;void __iomem *reg;u32 base, value;@@ -1476,7 +1476,7 @@ static void byt_gpio_irq_init_hw(struct byt_gpio *vg) value =readl (reg);if (value &BYT_DIRECT_IRQ_EN ) { -clear_bit (i, gc->irq.valid_mask); +clear_bit (i, chip->irq.valid_mask);dev_dbg (dev,"excluding GPIO %d from IRQ domain\n" , i); }else if ((value &BYT_PIN_MUX ) ==byt_get_gpio_mux (vg, i)) {byt_gpio_clear_triggering (vg, i);@@ -1504,6 +1504,8 @@ static void byt_gpio_irq_init_hw(struct byt_gpio *vg)"GPIO interrupt error, pins misconfigured. INT_STAT%u: 0x%08x\n" , base / 32, value); } + +return 0 ; }static int byt_gpio_add_pin_ranges (struct gpio_chip *chip)@@ -1542,26 +1544,31 @@ static int byt_gpio_probe(struct byt_gpio *vg)if (!vg->saved_context)return -ENOMEM ;#endif - ret =devm_gpiochip_add_data (&vg->pdev->dev, gc, vg); -if (ret) { -dev_err (&vg->pdev->dev,"failed adding byt-gpio chip\n" ); -return ret; - }/* set up interrupts */ irq_rc =platform_get_resource (vg->pdev,IORESOURCE_IRQ ,0 );if (irq_rc && irq_rc->start) { -byt_gpio_irq_init_hw (vg); - ret =gpiochip_irqchip_add (gc, &byt_irqchip,0 , - handle_bad_irq,IRQ_TYPE_NONE ); -if (ret) { -dev_err (&vg->pdev->dev,"failed to add irqchip\n" ); -return ret; - }
drivers/pinctrl/intel/pinctrl-baytrail.c static int byt_gpio_probe(struct byt_gpio *vg) · line 1560
Quoted 10 lines
drivers/pinctrl/intel/pinctrl-baytrail.c +10
-0+struct gpio_irq_chip *girq; + + girq = &gc->irq; + girq->chip = &byt_irqchip; + girq->init_hw = byt_gpio_irq_init_hw; + girq->parent_handler = byt_gpio_irq_handler; + girq->num_parents =1 ; + girq->parents =devm_kcalloc (&vg->pdev->dev,1 , +sizeof (*girq->parents), +GFP_KERNEL );
Here also
girq->parents = devm_kcalloc(&vg->pdev->dev, 1, sizeof(*girq->parents), GFP_KERNEL);
Regardless of that,
Hi,
Quoted 9 lines · Andy Shevchenko
This is the ordering fix for GPIO <-> pin mapping registration to allow delegation of IRQ chip to the GPIO library. Two drivers are actually converted to use the new callback followed by switching to use of irqchip structure inside gpiochip.
The series has been tested on Merrifield device (though no AEI) and on Thinkpad 10 where 4 events are registered, one of which has reasonable amount of interrupts coming from battery.
Quoted 8 lines
First two patches are definitely good for v5.4 cycle (and have Fixes tag) while the rest can be applied for v5.5 if I didn't miss anything. So, they can go like patches 1-3 -> v5.4 (gpio/fixes) and to gpio/for-next and to pinctrl/for-next patches 4-5 -> v5.5 (gpio/for-next)
patches 6-7 -> v5.5 (pinctrl/for-next)Perhaps it would require to wait next v5.4 rc in order to avoid dependency issues.
Given were we ar ein the cycle I think it is probably best to merge everything for 5.5, but that is not my call.
I've given this a test run on a BYT device, and it works for me,
So patches 1-3 and 6-7 are:
I've also reviewed all the patch and the entire set (so 1-7) looks good to me:
One related remark, I notice that the following patches which were in -next for a bit and where then dropped because this needed more work are missing from this set:
"pinctrl: baytrail: Allocate IRQ chip dynamic"
"pinctrl: baytrail: Group GPIO IRQ chip initialization"
"pinctrl: baytrail: Move IRQ valid mask initialization to a dedicated callback"
IMHO it would be good to get these landed too (could be done in a follow up series I guess).
I've also prepared and tested a set of patches on top of this one to also "Pass irqchip when adding gpiochip" for CHT, I will post these right after this email.
Regards,
Hans
Quoted 29 lines
Changelog v2 (resent):
- fixed cover letter
- resent with ML includedChangelog v2:
- added fixes tag to the first patch
- reworked commit message for the second patch
- added baytrail conversion
- tested on ThinkPad 10Andy Shevchenko (6):
gpiolib: No need to callgpiochip_remove_pin_ranges()twice
gpiolib: Introduce ->add_pin_ranges() callback
gpio: merrifield: Add GPIO <-> pin mapping ranges via callback
gpio: merrifield: Pass irqchip when adding gpiochip
pinctrl: baytrail: Add GPIO <-> pin mapping ranges via callback
pinctrl: baytrail: Pass irqchip when adding gpiochipLinus Walleij (1):
gpiolib: Switch order of valid mask and hw initdrivers/gpio/gpio-merrifield.c | 76 ++++++++++++++---------- drivers/gpio/gpiolib-of.c | 5 +- drivers/gpio/gpiolib.c | 19 +++++- drivers/pinctrl/intel/pinctrl-baytrail.c | 64 ++++++++++++-------- include/linux/gpio/driver.h | 5 ++ 5 files changed, 107 insertions(+), 62 deletions(-)
Quoted 9 lines · Andy Shevchenko
This is the ordering fix for GPIO <-> pin mapping registration to allow delegation of IRQ chip to the GPIO library. Two drivers are actually converted to use the new callback followed by switching to use of irqchip structure inside gpiochip.
The series has been tested on Merrifield device (though no AEI) and on Thinkpad 10 where 4 events are registered, one of which has reasonable amount of interrupts coming from battery.
Quoted 8 lines
First two patches are definitely good for v5.4 cycle (and have Fixes tag) while the rest can be applied for v5.5 if I didn't miss anything. So, they can go like patches 1-3 -> v5.4 (gpio/fixes) and to gpio/for-next and to pinctrl/for-next patches 4-5 -> v5.5 (gpio/for-next)
patches 6-7 -> v5.5 (pinctrl/for-next)Perhaps it would require to wait next v5.4 rc in order to avoid dependency issues.
So, I consolidated everything I have under my review-andy branch in pinctrl/intel.git repository.
When I applied Hans' CHV series, I fixed minor conflict due to new small patch in for-next branch.
Linus, I think it would be good if we have first 3 (okay, I noticed you have pushed first one to your devel branch) patches to go to v5.4. It will reduce dependency burden in v5.5.
Hans, if you have time you can test altogether (actually that set I used for local testing, so, I'm not expecting anything unusual there).
Quoted 31 lines
Changelog v2 (resent):
- fixed cover letter
- resent with ML includedChangelog v2:
- added fixes tag to the first patch
- reworked commit message for the second patch
- added baytrail conversion
- tested on ThinkPad 10Andy Shevchenko (6):
gpiolib: No need to callgpiochip_remove_pin_ranges()twice
gpiolib: Introduce ->add_pin_ranges() callback
gpio: merrifield: Add GPIO <-> pin mapping ranges via callback
gpio: merrifield: Pass irqchip when adding gpiochip
pinctrl: baytrail: Add GPIO <-> pin mapping ranges via callback
pinctrl: baytrail: Pass irqchip when adding gpiochipLinus Walleij (1):
gpiolib: Switch order of valid mask and hw initdrivers/gpio/gpio-merrifield.c | 76 ++++++++++++++---------- drivers/gpio/gpiolib-of.c | 5 +- drivers/gpio/gpiolib.c | 19 +++++- drivers/pinctrl/intel/pinctrl-baytrail.c | 64 ++++++++++++-------- include/linux/gpio/driver.h | 5 ++ 5 files changed, 107 insertions(+), 62 deletions(-)Signature · 1 line
2.24.0.rc1
Signature · 2 lines
With Best Regards, Andy Shevchenko
Quoted 3 lines · Andy Shevchenko
Linus, I think it would be good if we have first 3 (okay, I noticed you have pushed first one to your devel branch) patches to go to v5.4. It will reduce dependency burden in v5.5.
At this point in the kernel release cycle I can really only apply patches for serious regressions. That's the policy I need to keep to.
It also causes a problem if I put dependencies on Torvald's tree this late in the kernel cycle, because I need to get the changes back into my tree in order to base new stuff on them. I can't really do that until he releases an -rc that I can merge back.
At this point I'd maybe have to merge back v5.4 and that doesn't feel good at all.
So far I applied patches 1 & 2 for v5.5.
Yours,
Linus Walleij
Quoted 17 lines · Linus Walleij
Quoted 3 lines · Andy Shevchenko
Linus, I think it would be good if we have first 3 (okay, I noticed you have pushed first one to your devel branch) patches to go to v5.4. It will reduce dependency burden in v5.5.
At this point in the kernel release cycle I can really only apply patches for serious regressions. That's the policy I need to keep to.
It also causes a problem if I put dependencies on Torvald's tree this late in the kernel cycle, because I need to get the changes back into my tree in order to base new stuff on them. I can't really do that until he releases an -rc that I can merge back.
Quoted 4 lines
At this point I'd maybe have to merge back v5.4 and that doesn't feel good at all.
So far I applied patches 1 & 2 for v5.5.
Thanks! Does it mean that the rest is material for v5.6?
Signature · 2 lines
With Best Regards, Andy Shevchenko
Quoted 18 lines · Andy Shevchenko
Quoted 17 lines · Linus Walleij
Quoted 3 lines · Andy Shevchenko
Linus, I think it would be good if we have first 3 (okay, I noticed you have pushed first one to your devel branch) patches to go to v5.4. It will reduce dependency burden in v5.5.
At this point in the kernel release cycle I can really only apply patches for serious regressions. That's the policy I need to keep to.
It also causes a problem if I put dependencies on Torvald's tree this late in the kernel cycle, because I need to get the changes back into my tree in order to base new stuff on them. I can't really do that until he releases an -rc that I can merge back.
Quoted 6 lines
Quoted 4 lines
At this point I'd maybe have to merge back v5.4 and that doesn't feel good at all.
So far I applied patches 1 & 2 for v5.5.
Thanks! Does it mean that the rest is material for v5.6?
No, if they work for you and Hans both I have 100% trust in you guys so just send me a big pull request for all of it for v5.5. Besides Torvalds is talking about an -rc8 this next weekend.
Please base your pull request on my "devel" branch though, since I already applied some of the patches (sorry for the mess!)
We can certainly hash out any remaining nits during the v5.5 kernel cycle.
Yours,
Linus Walleij
Quoted 25 lines · Linus Walleij
Quoted 23 lines · Andy Shevchenko
Quoted 21 lines · Linus Walleij
Quoted 3 lines · Andy Shevchenko
Linus, I think it would be good if we have first 3 (okay, I noticed you have pushed first one to your devel branch) patches to go to v5.4. It will reduce dependency burden in v5.5.
At this point in the kernel release cycle I can really only apply patches for serious regressions. That's the policy I need to keep to.
It also causes a problem if I put dependencies on Torvald's tree this late in the kernel cycle, because I need to get the changes back into my tree in order to base new stuff on them. I can't really do that until he releases an -rc that I can merge back.
At this point I'd maybe have to merge back v5.4 and that doesn't feel good at all.
So far I applied patches 1 & 2 for v5.5.
Quoted 7 lines
Quoted 1 line
Thanks! Does it mean that the rest is material for v5.6?
No, if they work for you and Hans both I have 100% trust in you guys so just send me a big pull request for all of it for v5.5. Besides Torvalds is talking about an -rc8 this next weekend.
Please base your pull request on my "devel" branch though,
I think you meant for-next, which contains reverts my series based on.
Quoted 4 lines
since I already applied some of the patches (sorry for the mess!)
We can certainly hash out any remaining nits during the v5.5 kernel cycle.
As I explained in previous mail we have the current state:
- gpiolib patch + merrifield fixes (may be submitted as PR for v5.5)
- Intel pin control (for-next is going as usual for v5.5)
- Intel pin control fixes based on gpiolib patch (for v5.6)
- Lynxpoint as separate PR (didn't check for which tree it suits better, I guess due to nature of patches, it rather pin control, but we can wait with it till v5.6)
Signature · 2 lines
With Best Regards, Andy Shevchenko
Linus,
okay, first three patches had been send as PR against gpio/for-next for v5.5.
Signature · 2 lines
With Best Regards, Andy Shevchenko