Re: [PATCH v6 07/15] qcom: spmi-gpio: add support for hierarchical IRQ chip

2019-01-20 Thread Marc Zyngier
On Sat, 19 Jan 2019 20:42:44 +,
Brian Masney  wrote:
> 
> spmi-gpio did not have any irqchip support so consumers of this in
> device tree would need to call gpio[d]_to_irq() in order to get the
> proper IRQ on the underlying PMIC. IRQ chips in device tree should
> be usable from the start without the consumer having to make an
> additional call to get the proper IRQ on the parent. This patch adds
> hierarchical IRQ chip support to the spmi-gpio code to correct this
> issue.
> 
> Driver was tested using the volume buttons (via gpio-keys) on the LG
> Nexus 5 (hammerhead) phone with the following two configurations.
> 
> volume-up {
> interrupts-extended = <_gpios 2 IRQ_TYPE_EDGE_BOTH>;
> ...
> };
> 
> volume-up {
> gpios = <_gpios 2 GPIO_ACTIVE_LOW>;
> ...
> };
> 
> Both configurations now show that spmi-gpio is the IRQ domain and that
> the IRQ is setup in a hierarchy.
> 
> $ grep volume_up /proc/interrupts
>  72:  6  0  spmi-gpio   1 Edge  volume_up
> 
> $ cat /sys/kernel/debug/irq/irqs/72
> handler:  handle_edge_irq
> device:   (null)
> status:   0x0403
> _IRQ_NOPROBE
> istate:   0x
> ddepth:   0
> wdepth:   0
> dstate:   0x02400203
> IRQ_TYPE_EDGE_RISING
> IRQ_TYPE_EDGE_FALLING
> IRQD_ACTIVATED
> IRQD_IRQ_STARTED
> node: 0
> affinity: 0-3
> effectiv:
> domain:  :soc:spmi@fc4cf000:pm8941@0:gpios@c000
>  hwirq:   0x1
>  chip:spmi-gpio
>   flags:   0x4
>  IRQCHIP_MASK_ON_SUSPEND
>  parent:
> domain:  :soc:spmi@fc4cf000
>  hwirq:   0xc100057
>  chip:pmic_arb
>   flags:   0x4
>  IRQCHIP_MASK_ON_SUSPEND
> 
> Signed-off-by: Brian Masney 
> Reviewed-by: Stephen Boyd 
> ---
> Changes since v5:
> - Change IRQ_TYPE_NONE to IRQ_TYPE_EDGE_RISING
> 
> Changes since v4:
> - None
> 
> Changes since v3:
> - None
> 
> Changes since v2:
> - Use PMIC_GPIO_PHYSICAL_OFFSET instead of the 1 constant
> - Use gpiochip_irq_domain_{activate,deactivate}
> - Changed 'fwspec->param[0] + 0xc0 - 1' to 'hwirq + c0' in call to
>   irq_domain_alloc_irqs_parent
> 
> Changes since v1:
> - Use two cells for interrupts instead of four.
> - Pin numbers in interrupts-extended are now one based instead of zero
>   based so that they match the GPIO pin number.
> - Drop unnecessary parenthesis in pmic_gpio_domain_translate
> - Add missing of_node_put()
> - Remove irq field from pmic_gpio_pad struct that is no longer
>   necessary.
> 
>  drivers/pinctrl/qcom/pinctrl-spmi-gpio.c | 115 +--
>  1 file changed, 105 insertions(+), 10 deletions(-)

Reviewed-by: Marc Zyngier 

Thanks,

M.

-- 
Jazz is not dead, it just smell funny.


[PATCH v6 07/15] qcom: spmi-gpio: add support for hierarchical IRQ chip

2019-01-19 Thread Brian Masney
spmi-gpio did not have any irqchip support so consumers of this in
device tree would need to call gpio[d]_to_irq() in order to get the
proper IRQ on the underlying PMIC. IRQ chips in device tree should
be usable from the start without the consumer having to make an
additional call to get the proper IRQ on the parent. This patch adds
hierarchical IRQ chip support to the spmi-gpio code to correct this
issue.

Driver was tested using the volume buttons (via gpio-keys) on the LG
Nexus 5 (hammerhead) phone with the following two configurations.

volume-up {
interrupts-extended = <_gpios 2 IRQ_TYPE_EDGE_BOTH>;
...
};

volume-up {
gpios = <_gpios 2 GPIO_ACTIVE_LOW>;
...
};

Both configurations now show that spmi-gpio is the IRQ domain and that
the IRQ is setup in a hierarchy.

$ grep volume_up /proc/interrupts
 72:  6  0  spmi-gpio   1 Edge  volume_up

$ cat /sys/kernel/debug/irq/irqs/72
handler:  handle_edge_irq
device:   (null)
status:   0x0403
_IRQ_NOPROBE
istate:   0x
ddepth:   0
wdepth:   0
dstate:   0x02400203
IRQ_TYPE_EDGE_RISING
IRQ_TYPE_EDGE_FALLING
IRQD_ACTIVATED
IRQD_IRQ_STARTED
node: 0
affinity: 0-3
effectiv:
domain:  :soc:spmi@fc4cf000:pm8941@0:gpios@c000
 hwirq:   0x1
 chip:spmi-gpio
  flags:   0x4
 IRQCHIP_MASK_ON_SUSPEND
 parent:
domain:  :soc:spmi@fc4cf000
 hwirq:   0xc100057
 chip:pmic_arb
  flags:   0x4
 IRQCHIP_MASK_ON_SUSPEND

Signed-off-by: Brian Masney 
Reviewed-by: Stephen Boyd 
---
Changes since v5:
- Change IRQ_TYPE_NONE to IRQ_TYPE_EDGE_RISING

Changes since v4:
- None

Changes since v3:
- None

Changes since v2:
- Use PMIC_GPIO_PHYSICAL_OFFSET instead of the 1 constant
- Use gpiochip_irq_domain_{activate,deactivate}
- Changed 'fwspec->param[0] + 0xc0 - 1' to 'hwirq + c0' in call to
  irq_domain_alloc_irqs_parent

Changes since v1:
- Use two cells for interrupts instead of four.
- Pin numbers in interrupts-extended are now one based instead of zero
  based so that they match the GPIO pin number.
- Drop unnecessary parenthesis in pmic_gpio_domain_translate
- Add missing of_node_put()
- Remove irq field from pmic_gpio_pad struct that is no longer
  necessary.

 drivers/pinctrl/qcom/pinctrl-spmi-gpio.c | 115 +--
 1 file changed, 105 insertions(+), 10 deletions(-)

diff --git a/drivers/pinctrl/qcom/pinctrl-spmi-gpio.c 
b/drivers/pinctrl/qcom/pinctrl-spmi-gpio.c
index b6fa2c7dbb26..e01a556ac586 100644
--- a/drivers/pinctrl/qcom/pinctrl-spmi-gpio.c
+++ b/drivers/pinctrl/qcom/pinctrl-spmi-gpio.c
@@ -12,6 +12,7 @@
  */
 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -136,7 +137,6 @@ enum pmic_gpio_func_index {
 /**
  * struct pmic_gpio_pad - keep current GPIO settings
  * @base: Address base in SPMI device.
- * @irq: IRQ number which this GPIO generate.
  * @is_enabled: Set to false when GPIO should be put in high Z state.
  * @out_value: Cached pin output value
  * @have_buffer: Set to true if GPIO output could be configured in push-pull,
@@ -156,7 +156,6 @@ enum pmic_gpio_func_index {
  */
 struct pmic_gpio_pad {
u16 base;
-   int irq;
boolis_enabled;
boolout_value;
boolhave_buffer;
@@ -179,6 +178,8 @@ struct pmic_gpio_state {
struct regmap   *map;
struct pinctrl_dev *ctrl;
struct gpio_chip chip;
+   struct fwnode_handle *fwnode;
+   struct irq_domain *domain;
 };
 
 static const struct pinconf_generic_params pmic_gpio_bindings[] = {
@@ -761,11 +762,18 @@ static int pmic_gpio_of_xlate(struct gpio_chip *chip,
 static int pmic_gpio_to_irq(struct gpio_chip *chip, unsigned pin)
 {
struct pmic_gpio_state *state = gpiochip_get_data(chip);
-   struct pmic_gpio_pad *pad;
+   struct irq_fwspec fwspec;
 
-   pad = state->ctrl->desc->pins[pin].drv_data;
+   fwspec.fwnode = state->fwnode;
+   fwspec.param_count = 2;
+   fwspec.param[0] = pin + PMIC_GPIO_PHYSICAL_OFFSET;
+   /*
+* Set the type to a safe value temporarily. This will be overwritten
+* later with the proper value by irq_set_type.
+*/
+   fwspec.param[1] = IRQ_TYPE_EDGE_RISING;
 
-   return pad->irq;
+   return irq_create_fwspec_mapping();
 }
 
 static void pmic_gpio_dbg_show(struct seq_file *s, struct gpio_chip *chip)
@@ -935,8 +943,78 @@ static int pmic_gpio_populate(struct pmic_gpio_state 
*state,
return 0;
 }
 
+static struct irq_chip pmic_gpio_irq_chip = {
+   .name = "spmi-gpio",
+   .irq_ack = irq_chip_ack_parent,
+   .irq_mask = irq_chip_mask_parent,
+   .irq_unmask = irq_chip_unmask_parent,
+   .irq_set_type = irq_chip_set_type_parent,
+   .irq_set_wake = irq_chip_set_wake_parent,
+   .flags = IRQCHIP_MASK_ON_SUSPEND,
+};
+
+static int pmic_gpio_domain_translate(struct irq_domain *domain,
+