Re: [PATCH V2 1/2] pinctrl: qcom: spmi-gpio: correct parent irqspec translation

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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;



[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux