All valid comments. Will fix them all in the next rev.

Thanks Thomas.

-- Lina

On Fri, Feb 02 2018 at 15:37 +0000, Thomas Gleixner wrote:
On Fri, 2 Feb 2018, Lina Iyer wrote:
+static inline void pdc_enable_intr(struct irq_data *d, bool on)
+{
+       int pin_out = d->hwirq;
+       u32 index, mask;
+       u32 enable;
+       unsigned long flags;
+
+       index = pin_out / 32;
+       mask = pin_out % 32;
+
+       spin_lock_irqsave(&pdc_lock, flags);

Please make this a raw spinlock. Aside of that the _irqsave() is pointless
as the chip callbacks are already called with interrupts disabled.

+       enable = pdc_reg_read(IRQ_ENABLE_BANK, index);
+       enable = on ? ENABLE_INTR(enable, mask) : CLEAR_INTR(enable, mask);

You really should cache the enable register content to avoid the read back

+       pdc_reg_write(IRQ_ENABLE_BANK, index, enable);
+       spin_unlock_irqrestore(&pdc_lock, flags);
+}
+
+static void qcom_pdc_gic_mask(struct irq_data *d)
+{
+       pdc_enable_intr(d, false);
+       irq_chip_mask_parent(d);
+}
+
+static void qcom_pdc_gic_unmask(struct irq_data *d)
+{
+       pdc_enable_intr(d, true);
+       irq_chip_unmask_parent(d);
+}
+
+/*
+ * GIC does not handle falling edge or active low. To allow falling edge and
+ * active low interrupts to be handled at GIC, PDC has an inverter that inverts
+ * falling edge into a rising edge and active low into an active high.
+ * For the inverter to work, the polarity bit in the IRQ_CONFIG register has to
+ * set as per the table below.
+ * (polarity, falling edge, rising edge ) POLARITY
+ * 3'b0 00  Level sensitive active low    LOW
+ * 3'b0 01  Rising edge sensitive         NOT USED
+ * 3'b0 10  Falling edge sensitive        LOW
+ * 3'b0 11  Dual Edge sensitive           NOT USED
+ * 3'b1 00  Level senstive active High    HIGH
+ * 3'b1 01  Falling Edge sensitive        NOT USED
+ * 3'b1 10  Rising edge sensitive         HIGH
+ * 3'b1 11  Dual Edge sensitive           HIGH
+ */
+enum pdc_irq_config_bits {
+       PDC_POLARITY_LOW        = 0, // 0 00

What's wrong with

      PDC_POLARITY_LOW          = 000b,
      PDC_FALLING_EDGE          = 010b,

instead of decimal and these weird comments ?

+static irq_hw_number_t get_irq_for_pin(int pin, struct pdc_pin_data *pdc_data)
+{
+       int i;
+
+       for (i = 0; pdc_data[i].pin >= 0; i++)
+               if (pdc_data[i].pin == pin)
+                       return pdc_data[i].hwirq;

Please let the for() loop have braces. See:

      https://marc.info/?l=linux-kernel&m=148467980905537&w=2

+
+       return pin;
+}
+
+static int qcom_pdc_translate(struct irq_domain *d,
+       struct irq_fwspec *fwspec, unsigned long *hwirq, unsigned int *type)

Please align the arguments right of the opening brace:

static int qcom_pdc_translate(struct irq_domain *d,
                              struct irq_fwspec *fwspec, unsigned long *hwirq,
                              unsigned int *type)


+{
+       if (is_of_node(fwspec->fwnode)) {
+               if (fwspec->param_count < 3)
+                       return -EINVAL;
+
+               *hwirq = fwspec->param[1];
+               *type = fwspec->param[2] & IRQ_TYPE_SENSE_MASK;
+               return 0;
+       }
+
+       return -EINVAL;
+}
+
+static int qcom_pdc_alloc(struct irq_domain *domain,
+                       unsigned int virq, unsigned int nr_irqs, void *data)

Ditto

+static int pdc_setup_pin_mapping(struct device_node *np,
+                               struct pdc_pin_data **data)
+{
+       int ret;
+       int n, i, j, k, pins = 0;
+       int *val;

I have no idea what's the rationale behind these 3 lines of int declarations.

+       struct pdc_pin_data *map;
+
+       n = of_property_count_elems_of_size(np, "qcom,pdc-ranges", sizeof(u32));
+       if (!n || n % 3)
+               return -EINVAL;
+
+       val = kcalloc(n, sizeof(u32), GFP_KERNEL);
+       if (!val)
+               return -ENOMEM;
+
+       ret = of_property_read_u32_array(np, "qcom,pdc-ranges", val, n);
+       if (ret)
+               return ret;
+
+       for (i = 0; i < n; i += 3)
+               pins += val[i + 2];
+
+       if (pins > PDC_MAX_IRQS)
+               return -EFAULT;
+
+       map = kcalloc(pins + 1, sizeof(*map), GFP_KERNEL);
+       if (!map) {
+               ret = -ENOMEM;
+               goto fail;
+       }
+
+       for (i = 0, k = 0; i < n; i += 3) {
+               for (j = 0; j < val[i + 2]; j++, k++) {
+                       map[k].pin = val[i] + j;
+                       map[k].hwirq = val[i + 1] + j;
+               }
+       }

This all is really horrible to read. First of all the val[] array. That can
be represented in a structure, right? Without looking at the DT patch the
code lets me assume:

  struct pdcrange {
        u32     pin;
        u32     hwirq;
        u32     numpins;
        u32     unused;
  };

So the above becomes:

        nelm = of_property_count_elems_of_size(np, "qcom,pdc-ranges", 
sizeof(u32));
        if (!nelm || nelm % 3)
                return -EINVAL;

        nranges = nelm / 4;
        ranges = kcalloc(nranges, sizeof(*prng), GFP_KERNEL);
        if (!ranges)
                return -ENOMEM;

which makes the pin counting readable:

        for (i = 0; i < nranges; i++)
                pins += ranges[i].numpins;

And then allows to write the map initialization with:

        *data = map;
        for (i = 0; i < nranges; i++) {
                for (j = 0; j < ranges[i].numpins; j++, map++) {
                        map->pin = ranges[i].pin + j;
                        map->hwirq = ranges[i].hwirq + j;
                }
        }
        map->pin = -1;

Hmm?

+int qcom_pdc_init(struct device_node *node, struct device_node *parent)
+{
+       struct irq_domain *parent_domain, *pdc_domain;
+       struct pdc_pin_data *pdc_data = NULL;
+       int ret;
+
+       pdc_base = of_iomap(node, 0);
+       if (!pdc_base) {
+               pr_err("%s(): unable to map PDC registers\n", node->full_name);
+               return -ENXIO;
+       }
+
+       parent_domain = irq_find_host(parent);
+       if (!parent_domain) {
+               pr_err("unable to obtain PDC parent domain\n");
+               ret = -ENXIO;
+               goto failure;
+       }
+
+       ret = pdc_setup_pin_mapping(node, &pdc_data);

You can let pdc_setup_pin_mapping() return a pointer to pdc_data or NULL
and check the pointer for ERR or NULL instead of having that ret
indirection.

+       if (ret) {
+               pr_err("failed to setup PDC pin mapping\n");
+               goto failure;
+       }
+
+       pdc_domain = irq_domain_create_hierarchy(parent_domain, 0, PDC_MAX_IRQS,
+                                       of_fwnode_handle(node), &qcom_pdc_ops,
+                                       pdc_data);

See comment about argument alignement above. Hint: shortening the *_domain
variable names helps.

Thanks,

        tglx

Reply via email to