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
