Hello Miquèl, On Sat, 21 Apr 2018 15:55:29 +0200, Miquel Raynal wrote: > Introduce new bindings for the ICU. Perhaps this should explain *why* we need new bindings. > Each DT subnode of the ICU represents a type of interrupt that should > be handled separately. Add the possibility for the ICU to have subnodes > and probe each of them automatically with devm_platform_populate(). If > the node as no child, the probe function for NSRs will still be called > 'manually'. ... in order to preserve backward compatibility with Device Trees using the old binding. > +static struct mvebu_icu *mvebu_dev_get_drvdata(struct platform_device *pdev) The function should be prefixed by mvebu_icu_, not just mvebu_. > +{ > + struct mvebu_icu *icu; > + > + icu = dev_get_drvdata(&pdev->dev); > + if (icu) { > + /* Legacy bindings: get the device data */ I find this comment weird, because it doesn't document what the test just below is doing. > + if (!icu->legacy_bindings) > + return ERR_PTR(-EINVAL); > + } else { > + /* New bindings: get the parent device (ICU) data */ > + icu = dev_get_drvdata(pdev->dev.parent); > + if (!icu) > + return ERR_PTR(-ENODEV); > + if (icu->legacy_bindings) > + return ERR_PTR(-EINVAL); > + } > @@ -144,7 +170,10 @@ mvebu_icu_irq_domain_alloc(struct irq_domain *domain, unsigned int virq, > goto free_irqd; > } > > - icu_irqd->icu_group = fwspec->param[0]; > + if (icu->legacy_bindings) > + icu_irqd->icu_group = fwspec->param[0]; > + else > + icu_irqd->icu_group = ICU_GRP_NSR; In practice here fwspec->param[0] is always going to be equal to ICU_GRP_NSR, but OK, the test makes sense as in a future commit, the "else" case will be changed to support SEIs. > +static const struct of_device_id mvebu_icu_nsr_of_match[] = { > + { .compatible = "marvell,cp110-icu-nsr", }, > + {}, > +}; > + > +static struct platform_driver mvebu_icu_nsr_driver = { > + .probe = mvebu_icu_nsr_probe, > + .driver = { > + .name = "mvebu-icu-nsr", > + .of_match_table = mvebu_icu_nsr_of_match, > + }, > +}; > +builtin_platform_driver(mvebu_icu_nsr_driver); I'm not sure why you call this icu_nsr here, and change it later to icu_subset. Wouldn't it make sense to call it right away with the final name ? Note that this is not a very strong request to change this aspect, I'm fine with how it's done today, it's just that I would have done it differently. Other than that, looks good to me. Thomas -- Thomas Petazzoni, CTO, Bootlin (formerly Free Electrons) Embedded Linux and Kernel engineering https://bootlin.com -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html