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 <[email protected]>
---
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
From: David Collins <[email protected]>
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 <[email protected]>
Signed-off-by: satya priya <[email protected]>
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)
> 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);
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;
Quoting satya priya (2021-09-02 02:15:06)
> 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.
> + 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)
>> 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.
>> + 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.
>> + 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:39, Stephen Boyd wrote:
> Quoting satya priya (2021-09-02 02:15:05)
>> 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);
>
> 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.
>
Using kmalloc also should be fine but I'd like to keep the kzalloc, the
memset() operation takes some very small amount of time (once at boot
per consumer irqspec). kzalloc() is more deterministic and results in
code that doesn't care if elements are added to the definition of struct
irq_fwspec. It also ensures that all param[] elements are initialized to
0.
>> + 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.
>
I'll drop this and add a comment like this /* param[2] must be left as 0
*/, so that future reviewers don't think that there is a typo in the
indices going 0, 1, 3
>> + fwspec->param[3] = parent_type;
>> +
>> + return fwspec;
>> +}
>> +
>> static int pmic_gpio_probe(struct platform_device *pdev)
>> {
>> struct irq_domain *parent_domain;