RE: [PATCH v5] mfd: syscon: Decouple syscon interface from platform devices

[Pankaj Dubey]

On Thursday, September 25, 2014 6:12 PM, Arnd Bergmann wrote,
> Subject: Re: [PATCH v5] mfd: syscon: Decouple syscon interface from
platform
> devices
> 
> On Tuesday 23 September 2014 15:59:43 Pankaj Dubey wrote:
> > > > -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;
> > > > +   int ret;
> > > > +
> > > > +   if (!of_device_is_compatible(np, "syscon"))
> > > > +           return ERR_PTR(-EINVAL);
> > > > +
> > > > +   syscon = kzalloc(sizeof(*syscon), GFP_KERNEL);
> > > > +   if (!syscon)
> > > > +           return ERR_PTR(-ENOMEM);
> > > > +
> > > > +   base = of_iomap(np, 0);
> > > > +   if (!base) {
> > > > +           ret = -ENOMEM;
> > > > +           goto err_map;
> > > > +   }
> > > > +
> > > > +   /* if device is already populated and available then use it */
> > > > +   pdev = of_find_device_by_node(np);
> > > > +   if (!pdev) {
> > > > +           /* for early users create dummy syscon device and use it
*/
> > > > +           pdev = platform_device_alloc("dummy-syscon", -1);
> > >
> > > Can we create specific devices for each syscon device?
> > > That looks more reasonable to me.
> > > Then we can dump registers in regmap debugfs more clearly, just like
> > > other
> > devices.
> > > And i think it's better to follow device tree way to generate
> > > devices from
> > node.
> > > If DT core find a device is already created, it can ignore that
> > > device to
> > avoid creating
> > > duplicated device during of_platform_populate.
> > >
> >
> > If you are referring to use "of_platform_device_create", then I am
> > afraid that it can't be used in very early stage. For example, current
> > patch I tested by calling syscon_lookup_by_phandle from "init_irq"
> > hook of machine_desc, and it worked well, but If I use
> > "of_platform_device_create"
> > instead of dummy device, it fails to create pdev and this I verified
> > on Exynos platform. The reason I selected "init_irq" is because this
> > comes just before smp init and where once we tried to get regmap
> > handle, but could not do so.
> 
> I don't remember noticing the of_find_device_by_node or
platform_device_alloc in
> earlier versions of this patch, but that could just be me failing to read
it right.
> 
Yes, till v2 of this patch set we do not used this and while calling
regmap_init_mmio, I used
NULL pointer while creating regmap handle.  But as you may be remember Dong
Aisheng reported crash
on latest Linus tree after applying this patch, and it caused because of
some recent changes in
regmap which has already landed in main tree, which makes passing of dev
pointer compulsory
while calling regmap_init_mmio. Please check following commits in current
tree

1) d647c19 regmap: add DT endianness binding support.
2) ba1b53f regmap: Fix DT endianess parsing logic.
3) 45e1a27 regmap: of_regmap_get_endian() cleanup

and following patch [1] for changing syscon binding from Xiubo Li

[1] [PATCH] mfd: syscon: binding: Add syscon endianness support
https://lkml.org/lkml/2014/9/18/67

So we discussed [2] and decided to use either actual device if it's
populated and available for use or
create either actual device or dummy device for using it during
regmap_init_mmio calls.

[2]: https://lkml.org/lkml/2014/9/18/35

So what I understood from these changes from Xiubo Li, they wanted to pass
endianness of regmap
handle to be passed from DT. 

So I am not sure if we can really make regmap code work without a device
pointer. 

> I think this should get removed: it would be much better to ensure that
the regmap
> code can work without a device pointer, which I thought it did. We should
probably
> also skip the creation of the debugfs directory in that case.
> 
> > Also if it was just a matter of creating platform_device at early
> > stage then early initialization of syscon could have been solved till
> > now.

As far as debugfs of regmap is concerned it can be solved as I replied to
Heiko in this thread [3]
if we are using device pointer while creating regmap handle. 
If we are not using device pointer while creating regmap handle it won't
cause any problem and 
there won't be any debugfs entry.

[3]: https://lkml.org/lkml/2014/9/26/20

> >
> > So I would suggest if we really want this patch to cover early
> > initialization of syscon (which is not it's main purpose) current
> > patch is sufficient.
> >
> > @Arnd, since I have addressed crash issue as reported by Dong Aisheng,
> > I would like to take your ack, but since there are some more code
> > changes other than what you suggested I request you to check this and
> > if you are ok, then I would like to your ack so that I can request to
> > maintainer for taking this patch.
> 
> I think we first need to resolve the question of the platform device.
> 

OK.

Thanks,
Pankaj Dubey

> 	Arnd

--
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/