Messages in this thread | | | Date | Fri, 2 Feb 2018 16:41:02 +0000 | From | Lina Iyer <> | Subject | Re: [PATCH RFC v2 1/3] drivers: irqchip: pdc: Add PDC interrupt controller for QCOM SoCs |
| |
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
| |