lkml.org 
[lkml]   [2019]   [Dec]   [5]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH 2/2] soc: bcm: iproc: Add Broadcom iProc IDM driver
From
Date
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

\
 
 \ /
  Last update: 2019-12-06 01:22    [W:0.072 / U:10.092 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site