Re: [PATCH v3] mfd: syscon: Support early initialization

[Tomasz Figa]

Hi Michal,

First of all, even though not touching DT bindings, this change is DT 
related and so devicetree mailing list should be on Cc, as well as some 
DT people, especially Grant. Adding them.

Otherwise, please see my comments inline.

On 08.04.2014 17:00, Michal Simek wrote:
> Some platforms need to get system controller
> ready as soon as possible.
> The patch provides early_syscon_initialization
> which create early mapping for all syscon compatible
> devices in early_syscon_probe.
> Regmap is get via syscon_early_regmap_lookup_by_phandle()
>
> Regular device probes attach device to regmap
> via regmap_attach_dev().
>
> For early syscon initialization is necessary to extend
> struct syscon and provide remove function
> which unmap all early init structures.
>
> Signed-off-by: Michal Simek <michal.simek@xxxxxxxxxx>
> ---
>
> Changes in v3:
> - Keep backward compatibility for platform drivers and test it
> - Use only one probe method which is early_syscon_probe
>    suggested by Lee Jones. Do not force anybody to call early_syscon_init
> - Add kernel-doc description
>
> Changes in v2:
> - Fix bad logic in early_syscon_probe
> - Fix compilation failure for x86_64 reported by zero day testing system
> - Regmap change available here
>    git://git.kernel.org/pub/scm/linux/kernel/git/broonie/regmap.git tags/nodev
>
>   drivers/mfd/syscon.c       | 159 ++++++++++++++++++++++++++++++++++++++++-----
>   include/linux/mfd/syscon.h |  11 ++++
>   2 files changed, 153 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/mfd/syscon.c b/drivers/mfd/syscon.c
> index 71841f9..8e2ff88 100644
> --- a/drivers/mfd/syscon.c
> +++ b/drivers/mfd/syscon.c
> @@ -20,12 +20,15 @@
>   #include <linux/of_platform.h>
>   #include <linux/platform_device.h>
>   #include <linux/regmap.h>
> +#include <linux/slab.h>
>   #include <linux/mfd/syscon.h>
>
>   static struct platform_driver syscon_driver;
>
>   struct syscon {
> +	void __iomem *base;
>   	struct regmap *regmap;
> +	struct resource res;
>   };
>
>   static int syscon_match_node(struct device *dev, void *data)
> @@ -95,6 +98,30 @@ struct regmap *syscon_regmap_lookup_by_pdevname(const char *s)
>   }
>   EXPORT_SYMBOL_GPL(syscon_regmap_lookup_by_pdevname);
>
> +/**
> + * syscon_early_regmap_lookup_by_phandle - Early phandle lookup function
> + * @np:		device_node pointer
> + * @property:	property name which handle system controller phandle
> + * Return:	regmap pointer, an error pointer otherwise
> + */
> +struct regmap *syscon_early_regmap_lookup_by_phandle(struct device_node *np,
> +						     const char *property)
> +{
> +	struct device_node *syscon_np;
> +	struct syscon *syscon;
> +
> +	syscon_np = of_parse_phandle(np, property, 0);
> +	if (!syscon_np)
> +		return ERR_PTR(-ENODEV);
> +
> +	syscon = syscon_np->data;
> +
> +	of_node_put(syscon_np);

As it was mentioned in other review comments, this is rather fishy.

Instead, couldn't you create a local list of system controllers 
available in the system and then use that to perform look-up? This could 
also let you eliminate the need to have separate functions for early and 
normal look-up as the list could be simply used for both cases. Of 
course you would have to add of_node and list_head fields to struct 
syscon, but I don't think that matters.

As a side effect, the look-up would not need to iterate over all devices 
in the system anymore, just over the list of registered system controllers.

Best regards,
Tomasz
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/