Re: [PATCH 09/17] irqchip/irq-mvebu-icu: support ICU subnodes
From: Thomas Petazzoni <thomas.petazzoni@bootlin.com>
Date: 2018-05-02 08:13:00
Also in:
linux-arm-kernel
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);
+ }quoted hunk ↗ jump to hunk
@@ -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 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel