| Message ID | 1409668935-10667-1-git-send-email-pankaj.dubey@samsung.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 02.09.2014 16:42, Pankaj Dubey wrote: > Currently a syscon entity can only be registered directly through a > platform device that binds to a dedicated syscon driver. However in > certain cases it is required to bind a device with it's dedicated > driver rather than binding with syscon driver. > > For example, certain SoCs (e.g. Exynos) contain system controller > blocks which perform various functions such as power domain control, > CPU power management, low power mode control, but in addition contain > certain IP integration glue, such as various signal masks, > coprocessor power control, etc. In such case, there is a need to have > a dedicated driver for such system controller but also share registers > with other drivers. The latter is where the syscon interface is helpful. > > This patch decouples syscon object from syscon platform driver, and > allows to create syscon objects first time when it is required by > calling of syscon_regmap_lookup_by APIs and keeps a list of such syscon > objects along with syscon provider device_nodes and regmap handles. > > Signed-off-by: Pankaj Dubey <pankaj.dubey@samsung.com> > Signed-off-by: Tomasz Figa <t.figa@samsung.com> > --- > V1 of this patchset [1] and related discussion can be found here [1]. > > Changes since v1: > - Removed of_syscon_unregister function. > - Modified of_syscon_register function and it will be used by syscon.c > to create syscon objects whenever required. > - Removed platform device support from syscon. > - Removed syscon_regmap_lookup_by_pdevname API support. > - As there are significant changes w.r.t patchset v1, I am taking over > author for this patchset from Tomasz Figa. I guess you should also drop my Signed-off-by too. I think the best thing would replacing it with my Suggested-by and adding Arnd's Suggested-by too. Best regards, Tomasz
On Tuesday 02 September 2014 20:12:15 Pankaj Dubey wrote: > Currently a syscon entity can only be registered directly through a > platform device that binds to a dedicated syscon driver. However in > certain cases it is required to bind a device with it's dedicated > driver rather than binding with syscon driver. > > For example, certain SoCs (e.g. Exynos) contain system controller > blocks which perform various functions such as power domain control, > CPU power management, low power mode control, but in addition contain > certain IP integration glue, such as various signal masks, > coprocessor power control, etc. In such case, there is a need to have > a dedicated driver for such system controller but also share registers > with other drivers. The latter is where the syscon interface is helpful. > > This patch decouples syscon object from syscon platform driver, and > allows to create syscon objects first time when it is required by > calling of syscon_regmap_lookup_by APIs and keeps a list of such syscon > objects along with syscon provider device_nodes and regmap handles. > > Signed-off-by: Pankaj Dubey <pankaj.dubey@samsung.com> > Signed-off-by: Tomasz Figa <t.figa@samsung.com> > --- > V1 of this patchset [1] and related discussion can be found here [1]. > > Changes since v1: > - Removed of_syscon_unregister function. > - Modified of_syscon_register function and it will be used by syscon.c > to create syscon objects whenever required. > - Removed platform device support from syscon. > - Removed syscon_regmap_lookup_by_pdevname API support. > - As there are significant changes w.r.t patchset v1, I am taking over > author for this patchset from Tomasz Figa. Note that you got the Signed-off-by: list wrong, you should never have any people listed as Signed-off-by after your own name, and they should be listed before your name only when you are forwarding their patches. > Note: Current kernel has clps711x user of syscon_regmap_lookup_by_pdevname and > will be broken after this patch. As per discussion over here [1], patches > for making these drivers DT based are ready and once that is done they can use > syscon_regmap_lookup_by_phandle or syscon_regmap_lookup_by_compatible. > > [1]: https://lkml.org/lkml/2014/8/22/81 Adding Alexander Shiyan to Cc here, so we can make sure this is well coordinated. I'm also adding Michal Simek, since here was involved in earlier discussions about doing this. > drivers/mfd/syscon.c | 143 +++++++++++++------------------------------- > include/linux/mfd/syscon.h | 1 + > 2 files changed, 44 insertions(+), 100 deletions(-) I certainly like the diffstat ;-) > struct syscon { > + struct device_node *np; > struct regmap *regmap; > + struct list_head list; > }; Right > @@ -68,27 +72,6 @@ struct regmap *syscon_regmap_lookup_by_compatible(const char *s) > } > EXPORT_SYMBOL_GPL(syscon_regmap_lookup_by_compatible); > > -static int syscon_match_pdevname(struct device *dev, void *data) > -{ > - return !strcmp(dev_name(dev), (const char *)data); > -} > - > -struct regmap *syscon_regmap_lookup_by_pdevname(const char *s) > -{ > - struct device *dev; > - struct syscon *syscon; > - > - dev = driver_find_device(&syscon_driver.driver, NULL, (void *)s, > - syscon_match_pdevname); > - if (!dev) > - return ERR_PTR(-EPROBE_DEFER); > - > - syscon = dev_get_drvdata(dev); > - > - return syscon->regmap; > -} > -EXPORT_SYMBOL_GPL(syscon_regmap_lookup_by_pdevname); I think this can actually be left intact if that helps with clps71xx. It could be done in a hacky way using bus_find_device_by_name() to keep it simple, or in a somewhat nicer way by keeping the syscon platform_driver around for the non-DT case. > + regmap = regmap_init_mmio(NULL, base, &syscon_regmap_config); > + if (IS_ERR(regmap)) { > + pr_err("regmap init failed\n"); > + return ERR_CAST(regmap); > + } The last time I looked over this code, I think it was not safe to call regmap_init_mmio() with a NULL device pointer, and we decided that should be fixed. Have you checked whether it is ok now? Arnd
Tue, 02 Sep 2014 17:20:03 +0200 ?? Arnd Bergmann <arnd@arndb.de>: > On Tuesday 02 September 2014 20:12:15 Pankaj Dubey wrote: > > Currently a syscon entity can only be registered directly through a > > platform device that binds to a dedicated syscon driver. However in > > certain cases it is required to bind a device with it's dedicated > > driver rather than binding with syscon driver. > > > > For example, certain SoCs (e.g. Exynos) contain system controller > > blocks which perform various functions such as power domain control, > > CPU power management, low power mode control, but in addition contain > > certain IP integration glue, such as various signal masks, > > coprocessor power control, etc. In such case, there is a need to have > > a dedicated driver for such system controller but also share registers > > with other drivers. The latter is where the syscon interface is helpful. > > > > This patch decouples syscon object from syscon platform driver, and > > allows to create syscon objects first time when it is required by > > calling of syscon_regmap_lookup_by APIs and keeps a list of such syscon > > objects along with syscon provider device_nodes and regmap handles. > > > > Signed-off-by: Pankaj Dubey <pankaj.dubey@samsung.com> > > Signed-off-by: Tomasz Figa <t.figa@samsung.com> > > --- > > V1 of this patchset [1] and related discussion can be found here [1]. > > > > Changes since v1: > > - Removed of_syscon_unregister function. > > - Modified of_syscon_register function and it will be used by syscon.c > > to create syscon objects whenever required. > > - Removed platform device support from syscon. > > - Removed syscon_regmap_lookup_by_pdevname API support. > > - As there are significant changes w.r.t patchset v1, I am taking over > > author for this patchset from Tomasz Figa. > > Note that you got the Signed-off-by: list wrong, you should never have > any people listed as Signed-off-by after your own name, and they should > be listed before your name only when you are forwarding their patches. > > > Note: Current kernel has clps711x user of syscon_regmap_lookup_by_pdevname and > > will be broken after this patch. As per discussion over here [1], patches > > for making these drivers DT based are ready and once that is done they can use > > syscon_regmap_lookup_by_phandle or syscon_regmap_lookup_by_compatible. > > > > [1]: https://lkml.org/lkml/2014/8/22/81 ... > > -struct regmap *syscon_regmap_lookup_by_pdevname(const char *s) > > -{ > > - struct device *dev; > > - struct syscon *syscon; > > - > > - dev = driver_find_device(&syscon_driver.driver, NULL, (void *)s, > > - syscon_match_pdevname); > > - if (!dev) > > - return ERR_PTR(-EPROBE_DEFER); > > - > > - syscon = dev_get_drvdata(dev); > > - > > - return syscon->regmap; > > -} > > -EXPORT_SYMBOL_GPL(syscon_regmap_lookup_by_pdevname); > > I think this can actually be left intact if that helps with clps71xx. > It could be done in a hacky way using bus_find_device_by_name() > to keep it simple, or in a somewhat nicer way by keeping the > syscon platform_driver around for the non-DT case. It will not work anyway because the patch involves the use of of_device_is_compatible(), of_iomap() etc... ---
On Tuesday 02 September 2014 19:42:52 Alexander Shiyan wrote: > > > -struct regmap *syscon_regmap_lookup_by_pdevname(const char *s) > > > -{ > > > - struct device *dev; > > > - struct syscon *syscon; > > > - > > > - dev = driver_find_device(&syscon_driver.driver, NULL, (void *)s, > > > - syscon_match_pdevname); > > > - if (!dev) > > > - return ERR_PTR(-EPROBE_DEFER); > > > - > > > - syscon = dev_get_drvdata(dev); > > > - > > > - return syscon->regmap; > > > -} > > > -EXPORT_SYMBOL_GPL(syscon_regmap_lookup_by_pdevname); > > > > I think this can actually be left intact if that helps with clps71xx. > > It could be done in a hacky way using bus_find_device_by_name() > > to keep it simple, or in a somewhat nicer way by keeping the > > syscon platform_driver around for the non-DT case. > > It will not work anyway because the patch involves the use of > of_device_is_compatible(), of_iomap() etc... What I meant was to have the pdevname stuff keep working the way it does today. At that point, you essentially have two completely independent drivers, one that registers a platform driver and provides syscon_regmap_lookup_by_pdevname, and one that provides the other interfaces using DT lookup. Arnd
On Tue, Sep 2, 2014 at 8:12 PM, Pankaj Dubey <pankaj.dubey@samsung.com> wrote: > Currently a syscon entity can only be registered directly through a > platform device that binds to a dedicated syscon driver. However in > certain cases it is required to bind a device with it's dedicated > driver rather than binding with syscon driver. > > For example, certain SoCs (e.g. Exynos) contain system controller > blocks which perform various functions such as power domain control, > CPU power management, low power mode control, but in addition contain > certain IP integration glue, such as various signal masks, > coprocessor power control, etc. In such case, there is a need to have > a dedicated driver for such system controller but also share registers > with other drivers. The latter is where the syscon interface is helpful. > > This patch decouples syscon object from syscon platform driver, and > allows to create syscon objects first time when it is required by > calling of syscon_regmap_lookup_by APIs and keeps a list of such syscon > objects along with syscon provider device_nodes and regmap handles. > > Signed-off-by: Pankaj Dubey <pankaj.dubey@samsung.com> > Signed-off-by: Tomasz Figa <t.figa@samsung.com> > --- > V1 of this patchset [1] and related discussion can be found here [1]. > > Changes since v1: > - Removed of_syscon_unregister function. > - Modified of_syscon_register function and it will be used by syscon.c > to create syscon objects whenever required. > - Removed platform device support from syscon. > - Removed syscon_regmap_lookup_by_pdevname API support. > - As there are significant changes w.r.t patchset v1, I am taking over > author for this patchset from Tomasz Figa. > > Note: Current kernel has clps711x user of syscon_regmap_lookup_by_pdevname and > will be broken after this patch. As per discussion over here [1], patches > for making these drivers DT based are ready and once that is done they can use > syscon_regmap_lookup_by_phandle or syscon_regmap_lookup_by_compatible. > > [1]: https://lkml.org/lkml/2014/8/22/81 [snip] Tested on smdk5250 board with 3.17-rc3 for USB 2.0 and USB 3.0, since USB takes syscon phandle for handling some of the pmu registers. Things work fine with all the dt side changes in this patch. Tested-by: Vivek Gautam <gautam.vivek@samsung.com>
Hi, I'm joining the thread cause I'm really interested in having a syscon dev available during early init (I need it to share at91 PMC registers among several subsystems, one of this subsystem being the CCF which is called during early ARM boot process to register system clocks). On Tue, 02 Sep 2014 17:20:03 +0200 Arnd Bergmann <arnd@arndb.de> wrote: > On Tuesday 02 September 2014 20:12:15 Pankaj Dubey wrote: > > Currently a syscon entity can only be registered directly through a > > platform device that binds to a dedicated syscon driver. However in > > certain cases it is required to bind a device with it's dedicated > > driver rather than binding with syscon driver. > > > > For example, certain SoCs (e.g. Exynos) contain system controller > > blocks which perform various functions such as power domain control, > > CPU power management, low power mode control, but in addition contain > > certain IP integration glue, such as various signal masks, > > coprocessor power control, etc. In such case, there is a need to have > > a dedicated driver for such system controller but also share registers > > with other drivers. The latter is where the syscon interface is helpful. > > > > This patch decouples syscon object from syscon platform driver, and > > allows to create syscon objects first time when it is required by > > calling of syscon_regmap_lookup_by APIs and keeps a list of such syscon > > objects along with syscon provider device_nodes and regmap handles. > > > > Signed-off-by: Pankaj Dubey <pankaj.dubey@samsung.com> > > Signed-off-by: Tomasz Figa <t.figa@samsung.com> > > --- > > V1 of this patchset [1] and related discussion can be found here [1]. > > > > Changes since v1: > > - Removed of_syscon_unregister function. > > - Modified of_syscon_register function and it will be used by syscon.c > > to create syscon objects whenever required. > > - Removed platform device support from syscon. > > - Removed syscon_regmap_lookup_by_pdevname API support. > > - As there are significant changes w.r.t patchset v1, I am taking over > > author for this patchset from Tomasz Figa. > > Note that you got the Signed-off-by: list wrong, you should never have > any people listed as Signed-off-by after your own name, and they should > be listed before your name only when you are forwarding their patches. > > > Note: Current kernel has clps711x user of syscon_regmap_lookup_by_pdevname and > > will be broken after this patch. As per discussion over here [1], patches > > for making these drivers DT based are ready and once that is done they can use > > syscon_regmap_lookup_by_phandle or syscon_regmap_lookup_by_compatible. > > > > [1]: https://lkml.org/lkml/2014/8/22/81 > > Adding Alexander Shiyan to Cc here, so we can make sure this is well > coordinated. > > I'm also adding Michal Simek, since here was involved in earlier discussions > about doing this. > > > drivers/mfd/syscon.c | 143 +++++++++++++------------------------------- > > include/linux/mfd/syscon.h | 1 + > > 2 files changed, 44 insertions(+), 100 deletions(-) > > I certainly like the diffstat ;-) > > > struct syscon { > > + struct device_node *np; > > struct regmap *regmap; > > + struct list_head list; > > }; > > Right > > > @@ -68,27 +72,6 @@ struct regmap *syscon_regmap_lookup_by_compatible(const char *s) > > } > > EXPORT_SYMBOL_GPL(syscon_regmap_lookup_by_compatible); > > > > -static int syscon_match_pdevname(struct device *dev, void *data) > > -{ > > - return !strcmp(dev_name(dev), (const char *)data); > > -} > > - > > -struct regmap *syscon_regmap_lookup_by_pdevname(const char *s) > > -{ > > - struct device *dev; > > - struct syscon *syscon; > > - > > - dev = driver_find_device(&syscon_driver.driver, NULL, (void *)s, > > - syscon_match_pdevname); > > - if (!dev) > > - return ERR_PTR(-EPROBE_DEFER); > > - > > - syscon = dev_get_drvdata(dev); > > - > > - return syscon->regmap; > > -} > > -EXPORT_SYMBOL_GPL(syscon_regmap_lookup_by_pdevname); > > I think this can actually be left intact if that helps with clps71xx. > It could be done in a hacky way using bus_find_device_by_name() > to keep it simple, or in a somewhat nicer way by keeping the > syscon platform_driver around for the non-DT case. > > > > + regmap = regmap_init_mmio(NULL, base, &syscon_regmap_config); > > + if (IS_ERR(regmap)) { > > + pr_err("regmap init failed\n"); > > + return ERR_CAST(regmap); > > + } > > The last time I looked over this code, I think it was not safe to > call regmap_init_mmio() with a NULL device pointer, and we decided > that should be fixed. Have you checked whether it is ok now? I checked that part, and it appears most of the code is already there (see usage of regmap_attach_dev function here [1]). The only problem I see is that errors are still printed with dev_err, which, AFAIK, will trigger a kernel panic if dev is NULL. This is an issue in both the regmap_init function itself and the regcache_init function (which is called by regmap_init). BTW, maybe we should remove this line [2], as this is now part of the regmap_attach_dev function [3] which is called at the end of regmap_init if dev is not NULL [4]. Adding Mark in Cc. Best Regards, Boris [1]http://lxr.free-electrons.com/source/drivers/base/regmap/regmap.c#L826 [2]http://lxr.free-electrons.com/source/drivers/base/regmap/regmap.c#L511 [3]http://lxr.free-electrons.com/source/drivers/base/regmap/regmap.c#L434 [4]http://lxr.free-electrons.com/source/drivers/base/regmap/regmap.c#L826
On Wednesday 03 September 2014 15:16:11 Boris BREZILLON wrote: > I checked that part, and it appears most of the code is already there > (see usage of regmap_attach_dev function here [1]). > > The only problem I see is that errors are still printed with dev_err, > which, AFAIK, will trigger a kernel panic if dev is NULL. Actually not: static int __dev_printk(const char *level, const struct device *dev, struct va_format *vaf) { if (!dev) return printk("%s(NULL device *): %pV", level, vaf); return dev_printk_emit(level[1] - '0', dev, "%s %s: %pV", dev_driver_string(dev), dev_name(dev), vaf); } Arnd
On Wed, 03 Sep 2014 15:49:04 +0200 Arnd Bergmann <arnd@arndb.de> wrote: > On Wednesday 03 September 2014 15:16:11 Boris BREZILLON wrote: > > I checked that part, and it appears most of the code is already there > > (see usage of regmap_attach_dev function here [1]). > > > > The only problem I see is that errors are still printed with dev_err, > > which, AFAIK, will trigger a kernel panic if dev is NULL. > > Actually not: > > static int __dev_printk(const char *level, const struct device *dev, > struct va_format *vaf) > { > if (!dev) > return printk("%s(NULL device *): %pV", level, vaf); > > return dev_printk_emit(level[1] - '0', dev, > "%s %s: %pV", > dev_driver_string(dev), dev_name(dev), vaf); > } > My bad then (I don't know where I looked at to think NULL dev was not gracefully handled :-)). Thanks for pointing this out. Given that, I think it should work fine even with a NULL dev. I'll give it a try on at91 ;-). Best Regards, Boris
Hi Arnd, On Tuesday, September 02, 2014 Arnd Bergmann wrote, > To: Pankaj Dubey > Cc: linux-arm-kernel@lists.infradead.org; linux-samsung-soc@vger.kernel.org; linux- > kernel@vger.kernel.org; lee.jones@linaro.org; kgene.kim@samsung.com; > linux@arm.linux.org.uk; vikas.sajjan@samsung.com; joshi@samsung.com; > naushad@samsung.com; thomas.ab@samsung.com; chow.kim@samsung.com; > tomasz.figa@gmail.com; Tomasz Figa; Alexander Shiyan; Michal Simek > Subject: Re: [PATCH v2] mfd: syscon: Decouple syscon interface from platform > devices > > On Tuesday 02 September 2014 20:12:15 Pankaj Dubey wrote: > > Currently a syscon entity can only be registered directly through a > > platform device that binds to a dedicated syscon driver. However in > > certain cases it is required to bind a device with it's dedicated > > driver rather than binding with syscon driver. > > > > For example, certain SoCs (e.g. Exynos) contain system controller > > blocks which perform various functions such as power domain control, > > CPU power management, low power mode control, but in addition contain > > certain IP integration glue, such as various signal masks, coprocessor > > power control, etc. In such case, there is a need to have a dedicated > > driver for such system controller but also share registers with other > > drivers. The latter is where the syscon interface is helpful. > > > > This patch decouples syscon object from syscon platform driver, and > > allows to create syscon objects first time when it is required by > > calling of syscon_regmap_lookup_by APIs and keeps a list of such > > syscon objects along with syscon provider device_nodes and regmap handles. > > > > Signed-off-by: Pankaj Dubey <pankaj.dubey@samsung.com> > > Signed-off-by: Tomasz Figa <t.figa@samsung.com> > > --- > > V1 of this patchset [1] and related discussion can be found here [1]. > > > > Changes since v1: > > - Removed of_syscon_unregister function. > > - Modified of_syscon_register function and it will be used by syscon.c > > to create syscon objects whenever required. > > - Removed platform device support from syscon. > > - Removed syscon_regmap_lookup_by_pdevname API support. > > - As there are significant changes w.r.t patchset v1, I am taking over > > author for this patchset from Tomasz Figa. > > Note that you got the Signed-off-by: list wrong, you should never have any people > listed as Signed-off-by after your own name, and they should be listed before your > name only when you are forwarding their patches. > Sorry, I was not aware of this. I will take care in future. > > Note: Current kernel has clps711x user of > > syscon_regmap_lookup_by_pdevname and will be broken after this patch. > > As per discussion over here [1], patches for making these drivers DT > > based are ready and once that is done they can use > syscon_regmap_lookup_by_phandle or syscon_regmap_lookup_by_compatible. > > > > [1]: https://lkml.org/lkml/2014/8/22/81 > > Adding Alexander Shiyan to Cc here, so we can make sure this is well coordinated. > > I'm also adding Michal Simek, since here was involved in earlier discussions about > doing this. > Thanks. > > drivers/mfd/syscon.c | 143 +++++++++++++------------------------------- > > include/linux/mfd/syscon.h | 1 + > > 2 files changed, 44 insertions(+), 100 deletions(-) > > I certainly like the diffstat ;-) > > > struct syscon { > > + struct device_node *np; > > struct regmap *regmap; > > + struct list_head list; > > }; > > Right > > > @@ -68,27 +72,6 @@ struct regmap > > *syscon_regmap_lookup_by_compatible(const char *s) } > > EXPORT_SYMBOL_GPL(syscon_regmap_lookup_by_compatible); > > > > -static int syscon_match_pdevname(struct device *dev, void *data) -{ > > - return !strcmp(dev_name(dev), (const char *)data); > > -} > > - > > -struct regmap *syscon_regmap_lookup_by_pdevname(const char *s) -{ > > - struct device *dev; > > - struct syscon *syscon; > > - > > - dev = driver_find_device(&syscon_driver.driver, NULL, (void *)s, > > - syscon_match_pdevname); > > - if (!dev) > > - return ERR_PTR(-EPROBE_DEFER); > > - > > - syscon = dev_get_drvdata(dev); > > - > > - return syscon->regmap; > > -} > > -EXPORT_SYMBOL_GPL(syscon_regmap_lookup_by_pdevname); > > I think this can actually be left intact if that helps with clps71xx. > It could be done in a hacky way using bus_find_device_by_name() to keep it simple, > or in a somewhat nicer way by keeping the syscon platform_driver around for the > non-DT case. > Ok as per our last discussion you mentioned that clps71xx will be soon migrating to DT. So if that is not going to happen sooner, I would also prefer better keep syscon_regmap_lookup_by_pdevname and syscon platform_driver for non-DT case, so that this issue should not block this patch. So please let's make final call to keep syscon platform_driver for non-DT case which eventually can be dropped once clps71xx driver migrates to DT based. So that I can prepare next patchset keeping syscon platform_driver support and syscon_regmap_lookup_by_pdevname API support for non-DT case and send across for review. > > > + regmap = regmap_init_mmio(NULL, base, &syscon_regmap_config); > > + if (IS_ERR(regmap)) { > > + pr_err("regmap init failed\n"); > > + return ERR_CAST(regmap); > > + } > > The last time I looked over this code, I think it was not safe to > call regmap_init_mmio() with a NULL device pointer, and we decided > that should be fixed. Have you checked whether it is ok now? > At least we could not see any issues with this. We have tested it one Exynos5250 board where syscon object is getting created first time when other drivers (USB, SATA Phy, Watchdog) are calling helper function syscon_regmap_lookup_by_phandle and able to access regmap handle and able to use regmap_read/write APIs. > Arnd
Hi Boris, On Wednesday, September 03, 2014 Boris BREZILLON wrote, > To: Arnd Bergmann > Cc: Pankaj Dubey; kgene.kim@samsung.com; linux@arm.linux.org.uk; Alexander > Shiyan; naushad@samsung.com; Tomasz Figa; linux-kernel@vger.kernel.org; > joshi@samsung.com; linux-samsung-soc@vger.kernel.org; thomas.ab@samsung.com; > tomasz.figa@gmail.com; vikas.sajjan@samsung.com; chow.kim@samsung.com; > lee.jones@linaro.org; Michal Simek; linux-arm-kernel@lists.infradead.org; Mark > Brown > Subject: Re: [PATCH v2] mfd: syscon: Decouple syscon interface from platform > devices > > On Wed, 03 Sep 2014 15:49:04 +0200 > Arnd Bergmann <arnd@arndb.de> wrote: > > > On Wednesday 03 September 2014 15:16:11 Boris BREZILLON wrote: > > > I checked that part, and it appears most of the code is already > > > there (see usage of regmap_attach_dev function here [1]). > > > > > > The only problem I see is that errors are still printed with > > > dev_err, which, AFAIK, will trigger a kernel panic if dev is NULL. > > > > Actually not: > > > > static int __dev_printk(const char *level, const struct device *dev, > > struct va_format *vaf) { > > if (!dev) > > return printk("%s(NULL device *): %pV", level, vaf); > > > > return dev_printk_emit(level[1] - '0', dev, > > "%s %s: %pV", > > dev_driver_string(dev), dev_name(dev), > > vaf); } > > > > My bad then (I don't know where I looked at to think NULL dev was not gracefully > handled :-)). Thanks for pointing this out. > Given that, I think it should work fine even with a NULL dev. > I'll give it a try on at91 ;-). > We have tested this patch, on Exynos board and found working well. In our use case DT based drivers such as USB Phy, SATA Phy, Watchdog are calling syscon_regmap_lookup_by APIs to get regmap handle to Exynos PMU and it worked well for these drivers. It would be great if after testing you share result here or give a Tested-By. Thanks, Pankaj Dubey > Best Regards, > > Boris > > -- > Boris Brezillon, Free Electrons > Embedded Linux and Kernel engineering > http://free-electrons.com
Thu, 04 Sep 2014 10:09:19 +0530 ?? Pankaj Dubey <pankaj.dubey@samsung.com>: > Hi Arnd, > > On Tuesday, September 02, 2014 Arnd Bergmann wrote, > > To: Pankaj Dubey > > Cc: linux-arm-kernel@lists.infradead.org; > linux-samsung-soc@vger.kernel.org; linux- > > kernel@vger.kernel.org; lee.jones@linaro.org; kgene.kim@samsung.com; > > linux@arm.linux.org.uk; vikas.sajjan@samsung.com; joshi@samsung.com; > > naushad@samsung.com; thomas.ab@samsung.com; chow.kim@samsung.com; > > tomasz.figa@gmail.com; Tomasz Figa; Alexander Shiyan; Michal Simek > > Subject: Re: [PATCH v2] mfd: syscon: Decouple syscon interface from > platform > > devices ... > > > -struct regmap *syscon_regmap_lookup_by_pdevname(const char *s) -{ > > > - struct device *dev; > > > - struct syscon *syscon; > > > - > > > - dev = driver_find_device(&syscon_driver.driver, NULL, (void *)s, > > > - syscon_match_pdevname); > > > - if (!dev) > > > - return ERR_PTR(-EPROBE_DEFER); > > > - > > > - syscon = dev_get_drvdata(dev); > > > - > > > - return syscon->regmap; > > > -} > > > -EXPORT_SYMBOL_GPL(syscon_regmap_lookup_by_pdevname); > > > > I think this can actually be left intact if that helps with clps71xx. > > It could be done in a hacky way using bus_find_device_by_name() to keep it > simple, > > or in a somewhat nicer way by keeping the syscon platform_driver around > for the > > non-DT case. > > > > Ok as per our last discussion you mentioned that clps71xx will be soon > migrating to DT. > So if that is not going to happen sooner, I would also prefer better keep > syscon_regmap_lookup_by_pdevname and syscon platform_driver for non-DT case, > so that this issue should not block this patch. > > So please let's make final call to keep syscon platform_driver for non-DT > case which eventually > can be dropped once clps71xx driver migrates to DT based. So that I can > prepare next patchset > keeping syscon platform_driver support and syscon_regmap_lookup_by_pdevname > API support > for non-DT case and send across for review. Arnd, can you force the applying of the latest clps711x patches to accelerate the process? I mean latest 3 arm-soc patches from Aug, 19. After that I will need to make a patch for the SPI and TTY subsystems, then initial DT support will be introduced. ---
Hi Pankaj On Thu, 04 Sep 2014 10:15:27 +0530 Pankaj Dubey <pankaj.dubey@samsung.com> wrote: > Hi Boris, > > On Wednesday, September 03, 2014 Boris BREZILLON wrote, > > To: Arnd Bergmann > > Cc: Pankaj Dubey; kgene.kim@samsung.com; linux@arm.linux.org.uk; Alexander > > Shiyan; naushad@samsung.com; Tomasz Figa; linux-kernel@vger.kernel.org; > > joshi@samsung.com; linux-samsung-soc@vger.kernel.org; > thomas.ab@samsung.com; > > tomasz.figa@gmail.com; vikas.sajjan@samsung.com; chow.kim@samsung.com; > > lee.jones@linaro.org; Michal Simek; linux-arm-kernel@lists.infradead.org; > Mark > > Brown > > Subject: Re: [PATCH v2] mfd: syscon: Decouple syscon interface from > platform > > devices > > > > On Wed, 03 Sep 2014 15:49:04 +0200 > > Arnd Bergmann <arnd@arndb.de> wrote: > > > > > On Wednesday 03 September 2014 15:16:11 Boris BREZILLON wrote: > > > > I checked that part, and it appears most of the code is already > > > > there (see usage of regmap_attach_dev function here [1]). > > > > > > > > The only problem I see is that errors are still printed with > > > > dev_err, which, AFAIK, will trigger a kernel panic if dev is NULL. > > > > > > Actually not: > > > > > > static int __dev_printk(const char *level, const struct device *dev, > > > struct va_format *vaf) { > > > if (!dev) > > > return printk("%s(NULL device *): %pV", level, vaf); > > > > > > return dev_printk_emit(level[1] - '0', dev, > > > "%s %s: %pV", > > > dev_driver_string(dev), dev_name(dev), > > > vaf); } > > > > > > > My bad then (I don't know where I looked at to think NULL dev was not > gracefully > > handled :-)). Thanks for pointing this out. > > Given that, I think it should work fine even with a NULL dev. > > I'll give it a try on at91 ;-). > > > > We have tested this patch, on Exynos board and found working well. > In our use case DT based drivers such as USB Phy, SATA Phy, Watchdog are > calling > syscon_regmap_lookup_by APIs to get regmap handle to Exynos PMU and it > worked > well for these drivers. Great! > > It would be great if after testing you share result here or give a > Tested-By. Yep, that's the idea. Let me some time to rework the PMC drivers (drivers/clk/at91/pmc.c) and test it, and I'll add my Tested-by ;-). Best Regards, Boris
diff --git a/drivers/mfd/syscon.c b/drivers/mfd/syscon.c index ca15878..8247e93 100644 --- a/drivers/mfd/syscon.c +++ b/drivers/mfd/syscon.c @@ -14,41 +14,45 @@ #include <linux/err.h> #include <linux/io.h> -#include <linux/module.h> +#include <linux/list.h> #include <linux/of.h> #include <linux/of_address.h> -#include <linux/of_platform.h> -#include <linux/platform_data/syscon.h> -#include <linux/platform_device.h> #include <linux/regmap.h> #include <linux/mfd/syscon.h> +#include <linux/slab.h> -static struct platform_driver syscon_driver; +static DEFINE_SPINLOCK(syscon_list_slock); +static LIST_HEAD(syscon_list); struct syscon { + struct device_node *np; struct regmap *regmap; + struct list_head list; }; -static int syscon_match_node(struct device *dev, void *data) -{ - struct device_node *dn = data; - - return (dev->of_node == dn) ? 1 : 0; -} +static struct syscon *of_syscon_register(struct device_node *np); struct regmap *syscon_node_to_regmap(struct device_node *np) { - struct syscon *syscon; - struct device *dev; + struct syscon *entry, *syscon = NULL; + + spin_lock(&syscon_list_slock); - dev = driver_find_device(&syscon_driver.driver, NULL, np, - syscon_match_node); - if (!dev) - return ERR_PTR(-EPROBE_DEFER); + list_for_each_entry(entry, &syscon_list, list) + if (entry->np == np) { + syscon = entry; + break; + } - syscon = dev_get_drvdata(dev); + spin_unlock(&syscon_list_slock); - return syscon->regmap; + if (!syscon) + syscon = of_syscon_register(np); + + if (!IS_ERR(syscon)) + return syscon->regmap; + else + return ERR_CAST(syscon); } EXPORT_SYMBOL_GPL(syscon_node_to_regmap); @@ -68,27 +72,6 @@ struct regmap *syscon_regmap_lookup_by_compatible(const char *s) } EXPORT_SYMBOL_GPL(syscon_regmap_lookup_by_compatible); -static int syscon_match_pdevname(struct device *dev, void *data) -{ - return !strcmp(dev_name(dev), (const char *)data); -} - -struct regmap *syscon_regmap_lookup_by_pdevname(const char *s) -{ - struct device *dev; - struct syscon *syscon; - - dev = driver_find_device(&syscon_driver.driver, NULL, (void *)s, - syscon_match_pdevname); - if (!dev) - return ERR_PTR(-EPROBE_DEFER); - - syscon = dev_get_drvdata(dev); - - return syscon->regmap; -} -EXPORT_SYMBOL_GPL(syscon_regmap_lookup_by_pdevname); - struct regmap *syscon_regmap_lookup_by_phandle(struct device_node *np, const char *property) { @@ -110,81 +93,41 @@ struct regmap *syscon_regmap_lookup_by_phandle(struct device_node *np, } EXPORT_SYMBOL_GPL(syscon_regmap_lookup_by_phandle); -static const struct of_device_id of_syscon_match[] = { - { .compatible = "syscon", }, - { }, -}; - static struct regmap_config syscon_regmap_config = { .reg_bits = 32, .val_bits = 32, .reg_stride = 4, }; -static int syscon_probe(struct platform_device *pdev) +static struct syscon *of_syscon_register(struct device_node *np) { - struct device *dev = &pdev->dev; - struct syscon_platform_data *pdata = dev_get_platdata(dev); struct syscon *syscon; - struct resource *res; + struct regmap *regmap; void __iomem *base; - syscon = devm_kzalloc(dev, sizeof(*syscon), GFP_KERNEL); - if (!syscon) - return -ENOMEM; + if (!of_device_is_compatible(np, "syscon")) + return ERR_PTR(-EINVAL); - res = platform_get_resource(pdev, IORESOURCE_MEM, 0); - if (!res) - return -ENOENT; + syscon = kzalloc(sizeof(*syscon), GFP_KERNEL); + if (!syscon) + return ERR_PTR(-ENOMEM); - base = devm_ioremap(dev, res->start, resource_size(res)); + base = of_iomap(np, 0); if (!base) - return -ENOMEM; - - syscon_regmap_config.max_register = res->end - res->start - 3; - if (pdata) - syscon_regmap_config.name = pdata->label; - syscon->regmap = devm_regmap_init_mmio(dev, base, - &syscon_regmap_config); - if (IS_ERR(syscon->regmap)) { - dev_err(dev, "regmap init failed\n"); - return PTR_ERR(syscon->regmap); - } - - platform_set_drvdata(pdev, syscon); + return ERR_PTR(-ENOMEM); - dev_dbg(dev, "regmap %pR registered\n", res); - - return 0; -} - -static const struct platform_device_id syscon_ids[] = { - { "syscon", }, - { } -}; + regmap = regmap_init_mmio(NULL, base, &syscon_regmap_config); + if (IS_ERR(regmap)) { + pr_err("regmap init failed\n"); + return ERR_CAST(regmap); + } -static struct platform_driver syscon_driver = { - .driver = { - .name = "syscon", - .owner = THIS_MODULE, - .of_match_table = of_syscon_match, - }, - .probe = syscon_probe, - .id_table = syscon_ids, -}; + syscon->regmap = regmap; + syscon->np = np; -static int __init syscon_init(void) -{ - return platform_driver_register(&syscon_driver); -} -postcore_initcall(syscon_init); + spin_lock(&syscon_list_slock); + list_add_tail(&syscon->list, &syscon_list); + spin_unlock(&syscon_list_slock); -static void __exit syscon_exit(void) -{ - platform_driver_unregister(&syscon_driver); + return syscon; } -module_exit(syscon_exit); - -MODULE_AUTHOR("Dong Aisheng <dong.aisheng@linaro.org>"); -MODULE_DESCRIPTION("System Control driver"); -MODULE_LICENSE("GPL v2"); diff --git a/include/linux/mfd/syscon.h b/include/linux/mfd/syscon.h index 75e543b..a8e4c4b 100644 --- a/include/linux/mfd/syscon.h +++ b/include/linux/mfd/syscon.h @@ -18,6 +18,7 @@ #include <linux/err.h> struct device_node; +struct regmap; #ifdef CONFIG_MFD_SYSCON extern struct regmap *syscon_node_to_regmap(struct device_node *np);