[PATCH v3] mfd: syscon: Decouple syscon interface from platform devices

On Thu, Sep 18, 2014 at 03:06:59PM +0530, Pankaj Dubey wrote:

Hi,

On September 18, 2014 1:26, Dong Aisheng wrote

quoted

On Thu, Sep 18, 2014 at 11:33:26AM +0530, Pankaj Dubey wrote:

quoted

Hi,

Adding CC to Xiubo Li, Geert Uytterhoeven and Stephen Warren.

On Thursday, September 18, 2014, Dong Aisheng wrote,

quoted

On Wed, Sep 17, 2014 at 04:50:50PM +0530, Pankaj Dubey wrote:

quoted

Hi,

On Wednesday, September 17, 2014, Dong Aisheng Wrote,

quoted

quoted

+	regmap = regmap_init_mmio(NULL, base,

&syscon_regmap_config);

quoted

quoted

quoted

Does a NULL device pointer work?

Yes, it is safe, at least we are able to test on Exynos based SoC.
I have tested it with kgene/for-next kernel on Exynos3250.
Also it has been tested on Exynos5250 based Snow board with
3.17-rc5 based kernel by Vivek Gautam.

Patch V2 also has been tested by "Borris Brezillon" on AT91

platform.

quoted

quoted

quoted

quoted

The kernel i tested was next-20140915 of linux-next.

please see regmap_get_val_endian called in regmap_init function.
static enum regmap_endian regmap_get_val_endian(struct device *dev,
                                        const struct regmap_bus *bus,
                                        const struct regmap_config

*config) {

quoted

        struct device_node *np = dev->of_node;
        enum regmap_endian endian;
...
}
It will crash at the first line of dev->of_node if dev is NULL.

Can you check if you're using the same code as mine?

No, it's not same.
My bad that I was not using linux-next for testing this patch.
We tested on kgene/for-next where these changes still have not come.
Just now I checked linux-next and found that it will crash at first
line of "regmap_get_val_endian" as there is no check for NULL on dev.

I checked git history of regmap.c file and found recently this file
has been modified for adding DT endianness binding support. Following
are set of patches gone for this

cf673fb regmap: Split regmap_get_endian() in two functions 5844a8b
regmap: Fix handling of volatile registers for format_write() chips
45e1a27 regmap: of_regmap_get_endian() cleanup ba1b53f regmap: Fix DT
endianess parsing logic
d647c19 regmap: add DT endianness binding support.

I think there should have been a check for NULL on "dev" in
"regmap_get_val_endian", so that if dev pointer exist then only it
makes sense to get endianness property from DT.

I will suggest following fix in regmap.c for this. With following fix
I tested it and it works well on linux-next also. So if you can
confirm following fix is working for you then I can post this patch.

I tested the patch work.

Thanks for testing. In that case I will post this change, as I feel this
should be
fixed irrespective of my syscon patch.

quoted

But as Xiubo pointed in another mail, it may still cause other issues.
Looking at regmap.c, there're still some other places using the device

pointer, e.g.

quoted

dev_xxx debug information and some tracepoints also take device pointer as
parameter(not sure if it will break if dev is NULL).
Another thing is that if dev is NULL, we may not be able to use regmap

debugfs

quoted

feature which seems also not as our expected.

I would have preferred to check dev for NULL, as it's only at two places and
we could
still have debug prints for NULL dev, as normal pr_info instead of dev_info.

But Xiubo also pointed out that his patch [1] which updates syscon binding
information
will be useless if we pass NULL dev in regmap_init_mmio, which he posted
today,
and it requires dev pointer in "regmap_get_val_endian" function to read DT
property.

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

So instead of adding dummy device or creating device structure, I would
prefer to get actual
device pointer corresponding to "np" passed in of_syscon_register function
as shown below:

I wonder this may not work at early stage before the devices are populated
from device tree.
My initial understanding that one important thing for your patch is
to address this issue, isn't it?
Many people have asked for this feature before.

Regards
Dong Aisheng