[PATCH V2 0/2] Add Vol+ support for sc7280-idp

David Collins (1):
  pinctrl: qcom: spmi-gpio: correct parent irqspec translation

satya priya (1):
  arm64: dts: sc7280: Add volume up support for sc7280-idp

 arch/arm64/boot/dts/qcom/sc7280-idp.dtsi | 31 ++++++++++++++++++++++++++
 drivers/pinctrl/qcom/pinctrl-spmi-gpio.c | 37 +++++++++++++++++++++++++++++---
 2 files changed, 65 insertions(+), 3 deletions(-)

-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member 
of Code Aurora Forum, hosted by The Linux Foundation

Add pm7325 PMIC gpio support for vol+ on sc7280-idp.

Signed-off-by: satya priya <redacted>
---
Changes in V2:
 - No changes.

 arch/arm64/boot/dts/qcom/sc7280-idp.dtsi | 31 +++++++++++++++++++++++++++++++
 1 file changed, 31 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi b/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi
index 371a2a9..52bcbbc 100644
--- a/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi
+++ b/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi

@@ -199,6 +199,37 @@
 	modem-init;
 };
 
+&soc {
+	gpio_keys {
+		compatible = "gpio-keys";
+		label = "gpio-keys";
+
+		pinctrl-names = "default";
+		pinctrl-0 = <&key_vol_up_default>;
+
+		vol_up {
+			label = "volume_up";
+			gpios = <&pm7325_gpios 6 GPIO_ACTIVE_LOW>;
+			linux,input-type = <1>;
+			linux,code = <KEY_VOLUMEUP>;
+			gpio-key,wakeup;
+			debounce-interval = <15>;
+			linux,can-disable;
+		};
+	};
+};
+
+&pm7325_gpios {
+	key_vol_up_default: key_vol_up_default {
+		pins = "gpio6";
+		function = "normal";
+		input-enable;
+		bias-pull-up;
+		power-source = <0>;
+		qcom,drive-strength = <3>;
+	};
+};
+
 &pmk8350_vadc {
 	pmk8350_die_temp {
 		reg = <PMK8350_ADC7_DIE_TEMP>;

-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member 
of Code Aurora Forum, hosted by The Linux Foundation

Quoting satya priya (2021-09-02 02:15:06)

's' comes after 'p' so this is in the wrong place.

+       gpio_keys {
+               compatible = "gpio-keys";
+               label = "gpio-keys";
+
+               pinctrl-names = "default";
+               pinctrl-0 = <&key_vol_up_default>;
+
+               vol_up {
+                       label = "volume_up";
+                       gpios = <&pm7325_gpios 6 GPIO_ACTIVE_LOW>;
+                       linux,input-type = <1>;
+                       linux,code = <KEY_VOLUMEUP>;
+                       gpio-key,wakeup;
+                       debounce-interval = <15>;
+                       linux,can-disable;
+               };
+       };
+};
+
+&pm7325_gpios {
+       key_vol_up_default: key_vol_up_default {

Please move this to the "PINCTRL - additions to nodes defined in
sc7280.dtsi" section and then sort alphabetically on node naem.

+               pins = "gpio6";
+               function = "normal";
+               input-enable;
+               bias-pull-up;
+               power-source = <0>;
+               qcom,drive-strength = <3>;
+       };
+};
+
 &pmk8350_vadc {
        pmk8350_die_temp {
                reg = <PMK8350_ADC7_DIE_TEMP>;

On 2021-09-03 23:42, Stephen Boyd wrote:

Quoting satya priya (2021-09-02 02:15:06)

quoted

diff --git a/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi 

b/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi
index 371a2a9..52bcbbc 100644

--- a/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi
+++ b/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi

@@ -199,6 +199,37 @@
        modem-init;
 };

+&soc {

's' comes after 'p' so this is in the wrong place.

Okay will move it accordingly.

quoted

+       gpio_keys {
+               compatible = "gpio-keys";
+               label = "gpio-keys";
+
+               pinctrl-names = "default";
+               pinctrl-0 = <&key_vol_up_default>;
+
+               vol_up {
+                       label = "volume_up";
+                       gpios = <&pm7325_gpios 6 GPIO_ACTIVE_LOW>;
+                       linux,input-type = <1>;
+                       linux,code = <KEY_VOLUMEUP>;
+                       gpio-key,wakeup;
+                       debounce-interval = <15>;
+                       linux,can-disable;
+               };
+       };
+};
+
+&pm7325_gpios {
+       key_vol_up_default: key_vol_up_default {

Please move this to the "PINCTRL - additions to nodes defined in
sc7280.dtsi" section and then sort alphabetically on node naem.

Okay.

quoted

+               pins = "gpio6";
+               function = "normal";
+               input-enable;
+               bias-pull-up;
+               power-source = <0>;
+               qcom,drive-strength = <3>;
+       };
+};
+
 &pmk8350_vadc {
        pmk8350_die_temp {
                reg = <PMK8350_ADC7_DIE_TEMP>;

From: David Collins <redacted>

pmic_gpio_child_to_parent_hwirq() and
gpiochip_populate_parent_fwspec_fourcell() translate a pinctrl-
spmi-gpio irqspec to an SPMI controller irqspec.  When they do
this, they use a fixed SPMI slave ID of 0 and a fixed GPIO
peripheral offset of 0xC0 (corresponding to SPMI address 0xC000).
This translation results in an incorrect irqspec for secondary
PMICs that don't have a slave ID of 0 as well as for PMIC chips
which have GPIO peripherals located at a base address other than
0xC000.

Correct this issue by passing the slave ID of the pinctrl-spmi-
gpio device's parent in the SPMI controller irqspec and by
calculating the peripheral ID base from the device tree 'reg'
property of the pinctrl-spmi-gpio device.

Signed-off-by: David Collins <redacted>
Signed-off-by: satya priya <redacted>
Fixes: ca69e2d165eb ("qcom: spmi-gpio: add support for hierarchical IRQ chip")
---
Changes in V2:
 - Added a fixes tag.

 drivers/pinctrl/qcom/pinctrl-spmi-gpio.c | 37 +++++++++++++++++++++++++++++---
 1 file changed, 34 insertions(+), 3 deletions(-)

diff --git a/drivers/pinctrl/qcom/pinctrl-spmi-gpio.c b/drivers/pinctrl/qcom/pinctrl-spmi-gpio.c
index 98bf0e2..dbae168 100644
--- a/drivers/pinctrl/qcom/pinctrl-spmi-gpio.c
+++ b/drivers/pinctrl/qcom/pinctrl-spmi-gpio.c

@@ -1,6 +1,6 @@
 // SPDX-License-Identifier: GPL-2.0-only
 /*
- * Copyright (c) 2012-2014, The Linux Foundation. All rights reserved.
+ * Copyright (c) 2012-2014, 2016-2021 The Linux Foundation. All rights reserved.
  */
 
 #include <linux/gpio/driver.h>

@@ -14,6 +14,7 @@
 #include <linux/platform_device.h>
 #include <linux/regmap.h>
 #include <linux/slab.h>
+#include <linux/spmi.h>
 #include <linux/types.h>
 
 #include <dt-bindings/pinctrl/qcom,pmic-gpio.h>

@@ -171,6 +172,8 @@ struct pmic_gpio_state {
 	struct pinctrl_dev *ctrl;
 	struct gpio_chip chip;
 	struct irq_chip irq;
+	u8 usid;
+	u8 pid_base;
 };
 
 static const struct pinconf_generic_params pmic_gpio_bindings[] = {

@@ -949,12 +952,36 @@ static int pmic_gpio_child_to_parent_hwirq(struct gpio_chip *chip,
 					   unsigned int *parent_hwirq,
 					   unsigned int *parent_type)
 {
-	*parent_hwirq = child_hwirq + 0xc0;
+	struct pmic_gpio_state *state = gpiochip_get_data(chip);
+
+	*parent_hwirq = child_hwirq + state->pid_base;
 	*parent_type = child_type;
 
 	return 0;
 }
 
+static void *pmic_gpio_populate_parent_fwspec(struct gpio_chip *chip,
+					     unsigned int parent_hwirq,
+					     unsigned int parent_type)
+{
+	struct pmic_gpio_state *state = gpiochip_get_data(chip);
+	struct irq_fwspec *fwspec;
+
+	fwspec = kzalloc(sizeof(*fwspec), GFP_KERNEL);
+	if (!fwspec)
+		return NULL;
+
+	fwspec->fwnode = chip->irq.parent_domain->fwnode;
+
+	fwspec->param_count = 4;
+	fwspec->param[0] = state->usid;
+	fwspec->param[1] = parent_hwirq;
+	fwspec->param[2] = 0;
+	fwspec->param[3] = parent_type;
+
+	return fwspec;
+}
+
 static int pmic_gpio_probe(struct platform_device *pdev)
 {
 	struct irq_domain *parent_domain;

@@ -965,6 +992,7 @@ static int pmic_gpio_probe(struct platform_device *pdev)
 	struct pmic_gpio_pad *pad, *pads;
 	struct pmic_gpio_state *state;
 	struct gpio_irq_chip *girq;
+	const struct spmi_device *parent_spmi_dev;
 	int ret, npins, i;
 	u32 reg;
 

@@ -984,6 +1012,9 @@ static int pmic_gpio_probe(struct platform_device *pdev)
 
 	state->dev = &pdev->dev;
 	state->map = dev_get_regmap(dev->parent, NULL);
+	parent_spmi_dev = to_spmi_device(dev->parent);
+	state->usid = parent_spmi_dev->usid;
+	state->pid_base = reg >> 8;
 
 	pindesc = devm_kcalloc(dev, npins, sizeof(*pindesc), GFP_KERNEL);
 	if (!pindesc)

@@ -1059,7 +1090,7 @@ static int pmic_gpio_probe(struct platform_device *pdev)
 	girq->fwnode = of_node_to_fwnode(state->dev->of_node);
 	girq->parent_domain = parent_domain;
 	girq->child_to_parent_hwirq = pmic_gpio_child_to_parent_hwirq;
-	girq->populate_parent_alloc_arg = gpiochip_populate_parent_fwspec_fourcell;
+	girq->populate_parent_alloc_arg = pmic_gpio_populate_parent_fwspec;
 	girq->child_offset_to_irq = pmic_gpio_child_offset_to_irq;
 	girq->child_irq_domain_ops.translate = pmic_gpio_domain_translate;
 

-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member 
of Code Aurora Forum, hosted by The Linux Foundation

Quoting satya priya (2021-09-02 02:15:05)

The implementation of gpiochip_populate_parent_fwspec_fourcell() uses
kmalloc() here. Should we also do that? Presumably the fwspec will be
set with the important parts so this will save a memset call.

+       if (!fwspec)
+               return NULL;
+
+       fwspec->fwnode = chip->irq.parent_domain->fwnode;
+
+       fwspec->param_count = 4;
+       fwspec->param[0] = state->usid;
+       fwspec->param[1] = parent_hwirq;
+       fwspec->param[2] = 0;

If the kzalloc stays, this can be dropped.

+       fwspec->param[3] = parent_type;
+
+       return fwspec;
+}
+
 static int pmic_gpio_probe(struct platform_device *pdev)
 {
        struct irq_domain *parent_domain;