From: Florian Fainelli <f.fainelli@gmail.com> To: Ray Jui <ray.jui@broadcom.com>, Rob Herring <robh+dt@kernel.org>, Mark Rutland <mark.rutland@arm.com> Cc: devicetree@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, bcm-kernel-feedback-list@broadcom.com, Rayagonda Kokatanur <rayagonda.kokatanur@broadcom.com> Subject: Re: [PATCH 2/2] soc: bcm: iproc: Add Broadcom iProc IDM driver Date: Thu, 5 Dec 2019 16:22:35 -0800 [thread overview] Message-ID: <955f1d22-a1df-377a-1ed9-7fdaa4309b33@gmail.com> (raw) In-Reply-To: <20191202233127.31160-3-ray.jui@broadcom.com> On 12/2/19 3:31 PM, Ray Jui wrote: > Add Broadcom iProc IDM driver that controls that IDM devices available > on various iProc based SoCs for bus transaction timeout monitoring and > error logging. > > Signed-off-by: Ray Jui <ray.jui@broadcom.com> > Signed-off-by: Rayagonda Kokatanur <rayagonda.kokatanur@broadcom.com> > --- Looks good to me, just a few suggestions below [snip] > --- /dev/null > +++ b/drivers/soc/bcm/iproc/Kconfig > @@ -0,0 +1,6 @@ You would want an if SOC_BRCM_IPROC > +config IPROC_IDM > + bool "Broadcom iProc IDM driver" > + depends on (ARCH_BCM_IPROC || COMPILE_TEST) && OF > + default ARCH_BCM_IPROC > + help > + Enables support for iProc Interconnect and Device Management (IDM) control and monitoring and endif here to make this a nice menu. [snip] > + > +static int iproc_idm_dev_probe(struct platform_device *pdev) > +{ > + struct device *dev = &pdev->dev; > + struct device_node *np = dev->of_node; > + struct platform_device *elog_pdev; > + struct device_node *elog_np; > + struct iproc_idm *idm; > + const char *name; > + int ret; > + u32 val; > + > + idm = devm_kzalloc(dev, sizeof(*idm), GFP_KERNEL); > + if (!idm) > + return -ENOMEM; > + > + ret = of_property_read_string(np, "brcm,iproc-idm-bus", &name); > + if (ret) { > + dev_err(dev, "Unable to parse IDM bus name\n"); > + return ret; > + } > + idm->name = name; > + > + platform_set_drvdata(pdev, idm); > + idm->dev = dev; > + > + idm->base = of_iomap(np, 0); > + if (!idm->base) { > + dev_err(dev, "Unable to map I/O\n"); > + ret = -ENOMEM; > + goto err_exit; > + } > + > + ret = of_irq_get(np, 0); > + if (ret <= 0) { > + dev_err(dev, "Unable to find IRQ number. ret=%d\n", ret); > + goto err_iounmap; > + } Since this is a standard platform device, you can use the standard platform_get_resource() and platform_get_irq(). If you ever needed to support ACPI in the future, that would make it transparent and almost already ready. > + > + ret = devm_request_irq(dev, ret, iproc_idm_irq_handler, IRQF_SHARED, > + idm->name, idm); > + if (ret < 0) { > + dev_err(dev, "Unable to request irq. ret=%d\n", ret); > + goto err_iounmap; > + } > + > + /* > + * ELOG phandle is optional. If ELOG phandle is specified, it indicates > + * ELOG logging needs to be enabled > + */ > + elog_np = of_parse_phandle(dev->of_node, ELOG_IDM_COMPAT_STR, 0); > + if (elog_np) { > + elog_pdev = of_find_device_by_node(elog_np); > + if (!elog_pdev) { > + dev_err(dev, "Unable to find IDM ELOG device\n"); > + ret = -ENODEV; > + goto err_iounmap; > + } > + > + idm->elog = platform_get_drvdata(elog_pdev); > + if (!idm->elog) { > + dev_err(dev, "Unable to get IDM ELOG driver data\n"); > + ret = -EINVAL; > + goto err_iounmap; > + } > + } > + > + /* enable IDM timeout and its interrupt */ > + val = readl(idm->base + IDM_CTRL_OFFSET); > + val |= IDM_CTRL_TIMEOUT_EXP_MASK | IDM_CTRL_TIMEOUT_ENABLE | > + IDM_CTRL_TIMEOUT_IRQ; > + writel(val, idm->base + IDM_CTRL_OFFSET); > + > + ret = device_create_file(dev, &dev_attr_no_panic); > + if (ret < 0) > + goto err_iounmap; > + > + of_node_put(np); Did not you intend to drop the reference count on elog_np here? [snip] > +static struct platform_driver iproc_idm_driver = { > + .probe = iproc_idm_probe, Do not you need a remove function in order to unregister the sysfs file that you created in iproc_idm_dev_probe() to avoid bind/unbind (or rmmod/modprobe) to spit out an existing sysfs entry warning? -- Florian
WARNING: multiple messages have this Message-ID (diff)
From: Florian Fainelli <f.fainelli@gmail.com> To: Ray Jui <ray.jui@broadcom.com>, Rob Herring <robh+dt@kernel.org>, Mark Rutland <mark.rutland@arm.com> Cc: Rayagonda Kokatanur <rayagonda.kokatanur@broadcom.com>, devicetree@vger.kernel.org, bcm-kernel-feedback-list@broadcom.com, linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org Subject: Re: [PATCH 2/2] soc: bcm: iproc: Add Broadcom iProc IDM driver Date: Thu, 5 Dec 2019 16:22:35 -0800 [thread overview] Message-ID: <955f1d22-a1df-377a-1ed9-7fdaa4309b33@gmail.com> (raw) In-Reply-To: <20191202233127.31160-3-ray.jui@broadcom.com> On 12/2/19 3:31 PM, Ray Jui wrote: > Add Broadcom iProc IDM driver that controls that IDM devices available > on various iProc based SoCs for bus transaction timeout monitoring and > error logging. > > Signed-off-by: Ray Jui <ray.jui@broadcom.com> > Signed-off-by: Rayagonda Kokatanur <rayagonda.kokatanur@broadcom.com> > --- Looks good to me, just a few suggestions below [snip] > --- /dev/null > +++ b/drivers/soc/bcm/iproc/Kconfig > @@ -0,0 +1,6 @@ You would want an if SOC_BRCM_IPROC > +config IPROC_IDM > + bool "Broadcom iProc IDM driver" > + depends on (ARCH_BCM_IPROC || COMPILE_TEST) && OF > + default ARCH_BCM_IPROC > + help > + Enables support for iProc Interconnect and Device Management (IDM) control and monitoring and endif here to make this a nice menu. [snip] > + > +static int iproc_idm_dev_probe(struct platform_device *pdev) > +{ > + struct device *dev = &pdev->dev; > + struct device_node *np = dev->of_node; > + struct platform_device *elog_pdev; > + struct device_node *elog_np; > + struct iproc_idm *idm; > + const char *name; > + int ret; > + u32 val; > + > + idm = devm_kzalloc(dev, sizeof(*idm), GFP_KERNEL); > + if (!idm) > + return -ENOMEM; > + > + ret = of_property_read_string(np, "brcm,iproc-idm-bus", &name); > + if (ret) { > + dev_err(dev, "Unable to parse IDM bus name\n"); > + return ret; > + } > + idm->name = name; > + > + platform_set_drvdata(pdev, idm); > + idm->dev = dev; > + > + idm->base = of_iomap(np, 0); > + if (!idm->base) { > + dev_err(dev, "Unable to map I/O\n"); > + ret = -ENOMEM; > + goto err_exit; > + } > + > + ret = of_irq_get(np, 0); > + if (ret <= 0) { > + dev_err(dev, "Unable to find IRQ number. ret=%d\n", ret); > + goto err_iounmap; > + } Since this is a standard platform device, you can use the standard platform_get_resource() and platform_get_irq(). If you ever needed to support ACPI in the future, that would make it transparent and almost already ready. > + > + ret = devm_request_irq(dev, ret, iproc_idm_irq_handler, IRQF_SHARED, > + idm->name, idm); > + if (ret < 0) { > + dev_err(dev, "Unable to request irq. ret=%d\n", ret); > + goto err_iounmap; > + } > + > + /* > + * ELOG phandle is optional. If ELOG phandle is specified, it indicates > + * ELOG logging needs to be enabled > + */ > + elog_np = of_parse_phandle(dev->of_node, ELOG_IDM_COMPAT_STR, 0); > + if (elog_np) { > + elog_pdev = of_find_device_by_node(elog_np); > + if (!elog_pdev) { > + dev_err(dev, "Unable to find IDM ELOG device\n"); > + ret = -ENODEV; > + goto err_iounmap; > + } > + > + idm->elog = platform_get_drvdata(elog_pdev); > + if (!idm->elog) { > + dev_err(dev, "Unable to get IDM ELOG driver data\n"); > + ret = -EINVAL; > + goto err_iounmap; > + } > + } > + > + /* enable IDM timeout and its interrupt */ > + val = readl(idm->base + IDM_CTRL_OFFSET); > + val |= IDM_CTRL_TIMEOUT_EXP_MASK | IDM_CTRL_TIMEOUT_ENABLE | > + IDM_CTRL_TIMEOUT_IRQ; > + writel(val, idm->base + IDM_CTRL_OFFSET); > + > + ret = device_create_file(dev, &dev_attr_no_panic); > + if (ret < 0) > + goto err_iounmap; > + > + of_node_put(np); Did not you intend to drop the reference count on elog_np here? [snip] > +static struct platform_driver iproc_idm_driver = { > + .probe = iproc_idm_probe, Do not you need a remove function in order to unregister the sysfs file that you created in iproc_idm_dev_probe() to avoid bind/unbind (or rmmod/modprobe) to spit out an existing sysfs entry warning? -- Florian _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
next prev parent reply other threads:[~2019-12-06 0:22 UTC|newest] Thread overview: 32+ messages / expand[flat|nested] mbox.gz Atom feed top 2019-12-02 23:31 [PATCH 0/2] Add iProc IDM device support Ray Jui 2019-12-02 23:31 ` Ray Jui 2019-12-02 23:31 ` [PATCH 1/2] dt-bindings: soc: Add binding doc for iProc IDM device Ray Jui 2019-12-02 23:31 ` Ray Jui 2019-12-06 0:09 ` Florian Fainelli 2019-12-06 0:09 ` Florian Fainelli 2019-12-07 1:09 ` Ray Jui 2019-12-07 1:09 ` Ray Jui 2019-12-13 23:50 ` Rob Herring 2019-12-13 23:50 ` Rob Herring 2019-12-14 0:00 ` Florian Fainelli 2019-12-14 0:00 ` Florian Fainelli 2019-12-16 15:52 ` Rob Herring 2019-12-16 15:52 ` Rob Herring 2019-12-02 23:31 ` [PATCH 2/2] soc: bcm: iproc: Add Broadcom iProc IDM driver Ray Jui 2019-12-02 23:31 ` Ray Jui 2019-12-06 0:22 ` Florian Fainelli [this message] 2019-12-06 0:22 ` Florian Fainelli 2019-12-07 1:15 ` Ray Jui 2019-12-07 1:15 ` Ray Jui 2019-12-07 17:52 ` Florian Fainelli 2019-12-07 17:52 ` Florian Fainelli 2019-12-09 18:05 ` Ray Jui 2019-12-09 18:05 ` Ray Jui 2019-12-07 17:39 ` [PATCH 0/2] Add iProc IDM device support Marc Zyngier 2019-12-07 17:39 ` Marc Zyngier 2019-12-09 18:02 ` Ray Jui 2019-12-09 18:02 ` Ray Jui 2019-12-09 18:36 ` Marc Zyngier 2019-12-09 18:36 ` Marc Zyngier 2019-12-10 0:19 ` Ray Jui 2019-12-10 0:19 ` Ray Jui
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=955f1d22-a1df-377a-1ed9-7fdaa4309b33@gmail.com \ --to=f.fainelli@gmail.com \ --cc=bcm-kernel-feedback-list@broadcom.com \ --cc=devicetree@vger.kernel.org \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-kernel@vger.kernel.org \ --cc=mark.rutland@arm.com \ --cc=ray.jui@broadcom.com \ --cc=rayagonda.kokatanur@broadcom.com \ --cc=robh+dt@kernel.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.