Hi Pankaj,
Please see my comments inline.
On 19.09.2014 15:06, Pankaj Dubey wrote:
> Currently a syscon entity can be only registered directly through a
> platform device that binds to a dedicated syscon driver. However in
> certain use cases it is desirable to make a device used with another
> driver a syscon interface provider.
[snip]
> -static int syscon_match_node(struct device *dev, void *data)
> +static struct syscon *of_syscon_register(struct device_node *np)
> {
> - struct device_node *dn = data;
> + struct platform_device *pdev = NULL;
> + struct syscon *syscon;
> + struct regmap *regmap;
> + void __iomem *base;
> +
> +
nit: Stray blank line.
> + if (!of_device_is_compatible(np, "syscon"))
> + return ERR_PTR(-EINVAL);
I don't think this check is needed at all. I'd say that drivers should
be free to register a syscon provider for any node.
> +
> + syscon = kzalloc(sizeof(*syscon), GFP_KERNEL);
> + if (!syscon)
> + return ERR_PTR(-ENOMEM);
> +
> + base = of_iomap(np, 0);
> + if (!base)
> + return ERR_PTR(-ENOMEM);
> +
> + if (!of_device_is_available(np) ||
Wouldn't it be enough to simply call of_find_device_by_node(np) and if
it fails then instead create a dummy device?
> + of_node_test_and_set_flag(np, OF_POPULATED)) {
> + /* if device is already populated and avaiable then use it */
> + pdev = of_find_device_by_node(np);
> + if (!(&pdev->dev))
This is just plain wrong, because this condition will always evaluate to
true (see the definition of struct platform_device). Shouldn't you
rather just check the pdev pointer?
> + return ERR_PTR(-ENODEV);
> +
> + } else {
> + /* for early users create dummy syscon device and use it */
> + pdev = kzalloc(sizeof(*pdev), GFP_KERNEL);
> + if (!pdev)
> + return ERR_PTR(-ENOMEM);
Any clean-up on error path?
> +
> + pdev->name = "dummy-syscon";
> + pdev->id = -1;
Wouldn't you get an ID collision if more than one syscon is registered
early? Maybe the naming scheme from of_device_alloc() could be adopted
partially?
> + device_initialize(&pdev->dev);
I wonder if you couldn't simply reuse platform_device_alloc() for all of
this, except the line below, which would still have to be handled
separately.
> + pdev->dev.of_node = np;
> + }
> +
> + regmap = regmap_init_mmio(&pdev->dev, base, &syscon_regmap_config);
> + if (IS_ERR(regmap)) {
> + pr_err("regmap init failed\n");
If you have a dev here then you should be able to use dev_err() already.
> + return ERR_CAST(regmap);
> + }
> +
> + syscon->regmap = regmap;
> + syscon->np = np;
>
> - return (dev->of_node == dn) ? 1 : 0;
> + spin_lock(&syscon_list_slock);
> + list_add_tail(&syscon->list, &syscon_list);
> + spin_unlock(&syscon_list_slock);
> +
> + return syscon;
> }
>
> struct regmap *syscon_node_to_regmap(struct device_node *np)
> {
> - struct syscon *syscon;
> - struct device *dev;
> + struct syscon *entry, *syscon = NULL;
>
> - dev = driver_find_device(&syscon_driver.driver, NULL, np,
> - syscon_match_node);
> - if (!dev)
> - return ERR_PTR(-EPROBE_DEFER);
> + spin_lock(&syscon_list_slock);
>
> - syscon = dev_get_drvdata(dev);
> + list_for_each_entry(entry, &syscon_list, list)
> + if (entry->np == np) {
> + syscon = entry;
> + break;
> + }
>
> - return syscon->regmap;
> + spin_unlock(&syscon_list_slock);
> +
> + if (!syscon)
> + syscon = of_syscon_register(np);
> +
> + if (!IS_ERR(syscon))
> + return syscon->regmap;
> +
> + return ERR_CAST(syscon);
nit: Usually error checking is done the opposite way, i.e.
if (IS_ERR(syscon))
return ERR_CAST(syscon);
return syscon->regmap;
Best regards,
Tomasz
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
