| Message ID | 1408694991-21615-1-git-send-email-pankaj.dubey@samsung.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Sorry I forgot to add maintainer into CC. +Lee Jones Any comments on this patch. As a lot of Exynos PMU patch sets are dependent on this patch. Thanks, Pankaj Dubey > -----Original Message----- > From: Pankaj Dubey [mailto:pankaj.dubey@samsung.com] > Sent: Friday, August 22, 2014 1:40 PM > To: linux-arm-kernel@lists.infradead.org; linux-samsung-soc@vger.kernel.org; linux- > kernel@vger.kernel.org > Cc: kgene.kim@samsung.com; linux@arm.linux.org.uk; arnd@arndb.de; > vikas.sajjan@samsung.com; joshi@samsung.com; naushad@samsung.com; > thomas.ab@samsung.com; chow.kim@samsung.com; Tomasz Figa; Pankaj Dubey > Subject: [PATCH] mfd: syscon: Decouple syscon interface from syscon devices > > From: Tomasz Figa <t.figa@samsung.com> > > Currently a syscon entity can be only registered directly through a platform device > that binds to a dedicated driver. However in certain use cases it is desirable to make a > device used with another driver a syscon interface provider. 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 driver, so that it can be registered > from any driver in addition to the original "syscon" platform driver. > > Signed-off-by: Tomasz Figa <t.figa@samsung.com> > Signed-off-by: Pankaj Dubey <pankaj.dubey@samsung.com> > --- > > RFC patch [1] was posted by Tomasz Figa. This patch addresses some of comments > given by Arnd to RFC patch, and further decouples syscon from device model. It also > gives flexibility of registering with syscon at early stage using device_node object. > > [1]: https://lkml.org/lkml/2014/6/17/331 > > drivers/mfd/syscon.c | 112 ++++++++++++++++++++++++++++---- > ------------ > include/linux/mfd/syscon.h | 14 ++++++ > 2 files changed, 86 insertions(+), 40 deletions(-) > > diff --git a/drivers/mfd/syscon.c b/drivers/mfd/syscon.c index ca15878..a91db30 > 100644 > --- a/drivers/mfd/syscon.c > +++ b/drivers/mfd/syscon.c > @@ -14,6 +14,7 @@ > > #include <linux/err.h> > #include <linux/io.h> > +#include <linux/list.h> > #include <linux/module.h> > #include <linux/of.h> > #include <linux/of_address.h> > @@ -22,33 +23,32 @@ > #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; > -} > - > 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); > + > + return syscon ? syscon->regmap : ERR_PTR(-EPROBE_DEFER); > } > EXPORT_SYMBOL_GPL(syscon_node_to_regmap); > > @@ -68,24 +68,22 @@ 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); > + struct syscon *entry, *syscon = NULL; > + struct platform_device *pdev = NULL; > + > + spin_lock(&syscon_list_slock); > + list_for_each_entry(entry, &syscon_list, list) { > + pdev = of_find_device_by_node(entry->np); > + if (pdev && !strcmp(dev_name(&pdev->dev), s)) { > + syscon = entry; > + break; > + } > + } > + spin_unlock(&syscon_list_slock); > > - return syscon->regmap; > + return syscon ? syscon->regmap : ERR_PTR(-EPROBE_DEFER); > } > EXPORT_SYMBOL_GPL(syscon_regmap_lookup_by_pdevname); > > @@ -121,17 +119,49 @@ static struct regmap_config syscon_regmap_config = { > .reg_stride = 4, > }; > > +void of_syscon_unregister(struct device_node *np) { > + struct syscon *entry; > + > + spin_lock(&syscon_list_slock); > + > + list_for_each_entry(entry, &syscon_list, list) > + if (entry->np == np) { > + list_del(&entry->list); > + break; > + } > + > + spin_unlock(&syscon_list_slock); > +} > +EXPORT_SYMBOL_GPL(of_syscon_unregister); > + > +int of_syscon_register(struct device_node *np, struct regmap *regmap) { > + struct syscon *syscon; > + > + syscon = kzalloc(sizeof(*syscon), GFP_KERNEL); > + if (!syscon) > + return -ENOMEM; > + > + syscon->regmap = regmap; > + syscon->np = np; > + > + spin_lock(&syscon_list_slock); > + list_add_tail(&syscon->list, &syscon_list); > + spin_unlock(&syscon_list_slock); > + > + return 0; > +} > +EXPORT_SYMBOL_GPL(of_syscon_register); > + > static int syscon_probe(struct platform_device *pdev) { > struct device *dev = &pdev->dev; > struct syscon_platform_data *pdata = dev_get_platdata(dev); > - struct syscon *syscon; > + struct regmap *regmap; > struct resource *res; > void __iomem *base; > - > - syscon = devm_kzalloc(dev, sizeof(*syscon), GFP_KERNEL); > - if (!syscon) > - return -ENOMEM; > + int ret; > > res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > if (!res) > @@ -144,14 +174,16 @@ static int syscon_probe(struct platform_device *pdev) > 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, > + regmap = devm_regmap_init_mmio(dev, base, > &syscon_regmap_config); > - if (IS_ERR(syscon->regmap)) { > + if (IS_ERR(regmap)) { > dev_err(dev, "regmap init failed\n"); > - return PTR_ERR(syscon->regmap); > + return PTR_ERR(regmap); > } > > - platform_set_drvdata(pdev, syscon); > + ret = of_syscon_register(dev->of_node, regmap); > + if (ret) > + return ret; > > dev_dbg(dev, "regmap %pR registered\n", res); > > diff --git a/include/linux/mfd/syscon.h b/include/linux/mfd/syscon.h index > 75e543b..dc2807b 100644 > --- a/include/linux/mfd/syscon.h > +++ b/include/linux/mfd/syscon.h > @@ -18,8 +18,12 @@ > #include <linux/err.h> > > struct device_node; > +struct regmap; > > #ifdef CONFIG_MFD_SYSCON > +extern int of_syscon_register(struct device_node *np, struct regmap > +*regmap); extern void of_syscon_unregister(struct device_node *np); > + > extern struct regmap *syscon_node_to_regmap(struct device_node *np); extern > struct regmap *syscon_regmap_lookup_by_compatible(const char *s); extern struct > regmap *syscon_regmap_lookup_by_pdevname(const char *s); @@ -27,6 +31,16 > @@ extern struct regmap *syscon_regmap_lookup_by_phandle( > struct device_node *np, > const char *property); > #else > +static inline int of_syscon_register(struct device_node *np, > + struct regmap *regmap) > +{ > + return -ENOSYS; > +} > + > +static inline void of_syscon_unregister(struct device_node *np) { } > + > static inline struct regmap *syscon_node_to_regmap(struct device_node *np) { > return ERR_PTR(-ENOSYS); > -- > 1.7.9.5
On Fri, 22 Aug 2014, Pankaj Dubey wrote: > From: Tomasz Figa <t.figa@samsung.com> > > Currently a syscon entity can be only registered directly through a > platform device that binds to a dedicated driver. However in certain use > cases it is desirable to make a device used with another driver a syscon > interface provider. 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 driver, so that it can be > registered from any driver in addition to the original "syscon" platform > driver. > > Signed-off-by: Tomasz Figa <t.figa@samsung.com> > Signed-off-by: Pankaj Dubey <pankaj.dubey@samsung.com> > --- > > RFC patch [1] was posted by Tomasz Figa. This patch addresses some of > comments given by Arnd to RFC patch, and further decouples syscon from > device model. It also gives flexibility of registering with syscon at > early stage using device_node object. It would be helpful if Arnd gave this revision his blessing (Ack). > [1]: https://lkml.org/lkml/2014/6/17/331 > > drivers/mfd/syscon.c | 112 ++++++++++++++++++++++++++++---------------- > include/linux/mfd/syscon.h | 14 ++++++ > 2 files changed, 86 insertions(+), 40 deletions(-) > > diff --git a/drivers/mfd/syscon.c b/drivers/mfd/syscon.c > index ca15878..a91db30 100644 > --- a/drivers/mfd/syscon.c > +++ b/drivers/mfd/syscon.c > @@ -14,6 +14,7 @@ [...] > 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); > + struct syscon *entry, *syscon = NULL; > + struct platform_device *pdev = NULL; > + > + spin_lock(&syscon_list_slock); > + list_for_each_entry(entry, &syscon_list, list) { > + pdev = of_find_device_by_node(entry->np); White space error. Did you run this through checkpatch.pl? > + if (pdev && !strcmp(dev_name(&pdev->dev), s)) { > + syscon = entry; > + break; > + } > + } > + spin_unlock(&syscon_list_slock); > > - return syscon->regmap; > + return syscon ? syscon->regmap : ERR_PTR(-EPROBE_DEFER); > } > EXPORT_SYMBOL_GPL(syscon_regmap_lookup_by_pdevname); [...]
On Monday 01 September 2014 08:49:18 Lee Jones wrote: > On Fri, 22 Aug 2014, Pankaj Dubey wrote: > > > From: Tomasz Figa <t.figa@samsung.com> > > > > Currently a syscon entity can be only registered directly through a > > platform device that binds to a dedicated driver. However in certain use > > cases it is desirable to make a device used with another driver a syscon > > interface provider. 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 driver, so that it can be > > registered from any driver in addition to the original "syscon" platform > > driver. > > > > Signed-off-by: Tomasz Figa <t.figa@samsung.com> > > Signed-off-by: Pankaj Dubey <pankaj.dubey@samsung.com> > > --- > > > > RFC patch [1] was posted by Tomasz Figa. This patch addresses some of > > comments given by Arnd to RFC patch, and further decouples syscon from > > device model. It also gives flexibility of registering with syscon at > > early stage using device_node object. > > It would be helpful if Arnd gave this revision his blessing (Ack). I never saw a reason why we don't take this all the way as discussed a few times: Completely remove the dependency of syscon on having a platform driver for it, and make it possible to just call syscon_regmap_lookup_by_phandle() without having to register it first. Arnd
On Mon, 01 Sep 2014, Arnd Bergmann wrote: > On Monday 01 September 2014 08:49:18 Lee Jones wrote: > > On Fri, 22 Aug 2014, Pankaj Dubey wrote: > > > > > From: Tomasz Figa <t.figa@samsung.com> > > > > > > Currently a syscon entity can be only registered directly through a > > > platform device that binds to a dedicated driver. However in certain use > > > cases it is desirable to make a device used with another driver a syscon > > > interface provider. 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 driver, so that it can be > > > registered from any driver in addition to the original "syscon" platform > > > driver. > > > > > > Signed-off-by: Tomasz Figa <t.figa@samsung.com> > > > Signed-off-by: Pankaj Dubey <pankaj.dubey@samsung.com> > > > --- > > > > > > RFC patch [1] was posted by Tomasz Figa. This patch addresses some of > > > comments given by Arnd to RFC patch, and further decouples syscon from > > > device model. It also gives flexibility of registering with syscon at > > > early stage using device_node object. > > > > It would be helpful if Arnd gave this revision his blessing (Ack). > > I never saw a reason why we don't take this all the way as discussed > a few times: Completely remove the dependency of syscon on having > a platform driver for it, and make it possible to just call > syscon_regmap_lookup_by_phandle() without having to register > it first. I think this sounds like a good end-state. Migrating over by supporting both methods in this way does sound like the correct thing to do though. Doing so is likely to dramatically reduce the effect on current users.
Hi Arnd, > -----Original Message----- > From: Arnd Bergmann [mailto:arnd@arndb.de] > Sent: Monday, September 01, 2014 4:07 PM > To: Lee Jones > Cc: Pankaj Dubey; linux-arm-kernel@lists.infradead.org; linux-samsung- > soc@vger.kernel.org; linux-kernel@vger.kernel.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; broonie@kernel.org > Subject: Re: [PATCH] mfd: syscon: Decouple syscon interface from syscon devices > > On Monday 01 September 2014 08:49:18 Lee Jones wrote: > > On Fri, 22 Aug 2014, Pankaj Dubey wrote: > > > > > From: Tomasz Figa <t.figa@samsung.com> > > > > > > Currently a syscon entity can be only registered directly through a > > > platform device that binds to a dedicated driver. However in certain > > > use cases it is desirable to make a device used with another driver > > > a syscon interface provider. 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 driver, so that it > > > can be registered from any driver in addition to the original > > > "syscon" platform driver. > > > > > > Signed-off-by: Tomasz Figa <t.figa@samsung.com> > > > Signed-off-by: Pankaj Dubey <pankaj.dubey@samsung.com> > > > --- > > > > > > RFC patch [1] was posted by Tomasz Figa. This patch addresses some > > > of comments given by Arnd to RFC patch, and further decouples syscon > > > from device model. It also gives flexibility of registering with > > > syscon at early stage using device_node object. > > > > It would be helpful if Arnd gave this revision his blessing (Ack). > > I never saw a reason why we don't take this all the way as discussed a few times: > Completely remove the dependency of syscon on having a platform driver for it, and > make it possible to just call > syscon_regmap_lookup_by_phandle() without having to register it first. > Please have a look at this thread [1], where we discussed the need of syscon as platform device. Looks like still there are users of syscon_regmap_lookup_by_pdevname (clps711x.c). So we can't make it completely independent of platform_device. [1] https://www.mail-archive.com/linux-samsung-soc@vger.kernel.org/msg35708.html > Arnd
On Monday 01 September 2014 17:05:33 Pankaj Dubey wrote: > > Please have a look at this thread [1], where we discussed the need of syscon > as platform device. > Looks like still there are users of syscon_regmap_lookup_by_pdevname > (clps711x.c). > So we can't make it completely independent of platform_device. > > [1] > https://www.mail-archive.com/linux-samsung-soc@vger.kernel.org/msg35708.html I think the clps711x platform will soon stop using it, we already have patches for migrating to DT. Arnd
On Monday 01 September 2014 12:25:49 Lee Jones wrote: > On Mon, 01 Sep 2014, Arnd Bergmann wrote: > > On Monday 01 September 2014 08:49:18 Lee Jones wrote: > > > On Fri, 22 Aug 2014, Pankaj Dubey wrote: > > > > > > > From: Tomasz Figa <t.figa@samsung.com> > > > > > > > > Currently a syscon entity can be only registered directly through a > > > > platform device that binds to a dedicated driver. However in certain use > > > > cases it is desirable to make a device used with another driver a syscon > > > > interface provider. 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 driver, so that it can be > > > > registered from any driver in addition to the original "syscon" platform > > > > driver. > > > > > > > > Signed-off-by: Tomasz Figa <t.figa@samsung.com> > > > > Signed-off-by: Pankaj Dubey <pankaj.dubey@samsung.com> > > > > --- > > > > > > > > RFC patch [1] was posted by Tomasz Figa. This patch addresses some of > > > > comments given by Arnd to RFC patch, and further decouples syscon from > > > > device model. It also gives flexibility of registering with syscon at > > > > early stage using device_node object. > > > > > > It would be helpful if Arnd gave this revision his blessing (Ack). > > > > I never saw a reason why we don't take this all the way as discussed > > a few times: Completely remove the dependency of syscon on having > > a platform driver for it, and make it possible to just call > > syscon_regmap_lookup_by_phandle() without having to register > > it first. > > I think this sounds like a good end-state. Migrating over by > supporting both methods in this way does sound like the correct thing > to do though. Doing so is likely to dramatically reduce the effect on > current users. Maybe I'm misreading the patch, but I don't see how it creates a migration path. What I want to end up with is infrastructure that lets anybody call syscon_regmap_lookup_by_pdevname or syscon_regmap_lookup_by_compatible (if they really need to) without needing the platform_driver for syscon. That should not require any form of compatibility layer because to the driver using it there is no API change. In contrast, this patch introduces a new of_syscon_{un,}register() interface that would get removed after the the above has been implemented, causing extra churn for any driver that also wants to provide a regmap-like interface. Arnd
On Mon, 01 Sep 2014, Arnd Bergmann wrote: > On Monday 01 September 2014 12:25:49 Lee Jones wrote: > > On Mon, 01 Sep 2014, Arnd Bergmann wrote: > > > On Monday 01 September 2014 08:49:18 Lee Jones wrote: > > > > On Fri, 22 Aug 2014, Pankaj Dubey wrote: > > > > > > > > > From: Tomasz Figa <t.figa@samsung.com> > > > > > > > > > > Currently a syscon entity can be only registered directly through a > > > > > platform device that binds to a dedicated driver. However in certain use > > > > > cases it is desirable to make a device used with another driver a syscon > > > > > interface provider. 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 driver, so that it can be > > > > > registered from any driver in addition to the original "syscon" platform > > > > > driver. > > > > > > > > > > Signed-off-by: Tomasz Figa <t.figa@samsung.com> > > > > > Signed-off-by: Pankaj Dubey <pankaj.dubey@samsung.com> > > > > > --- > > > > > > > > > > RFC patch [1] was posted by Tomasz Figa. This patch addresses some of > > > > > comments given by Arnd to RFC patch, and further decouples syscon from > > > > > device model. It also gives flexibility of registering with syscon at > > > > > early stage using device_node object. > > > > > > > > It would be helpful if Arnd gave this revision his blessing (Ack). > > > > > > I never saw a reason why we don't take this all the way as discussed > > > a few times: Completely remove the dependency of syscon on having > > > a platform driver for it, and make it possible to just call > > > syscon_regmap_lookup_by_phandle() without having to register > > > it first. > > > > I think this sounds like a good end-state. Migrating over by > > supporting both methods in this way does sound like the correct thing > > to do though. Doing so is likely to dramatically reduce the effect on > > current users. > > Maybe I'm misreading the patch, but I don't see how it creates a > migration path. What I want to end up with is infrastructure that > lets anybody call syscon_regmap_lookup_by_pdevname or > syscon_regmap_lookup_by_compatible (if they really need to) > without needing the platform_driver for syscon. That should not > require any form of compatibility layer because to the driver > using it there is no API change. Somehow I think the likelyhood is that I am misreading the patch. I thought that before this patch drivers we had to register a syscon device to bind to this driver, which was fine for the first use-cases of syscon as it wasn't required too early during boot. However, now there are use-cases where systems require access to syscon registers eariler in boot we require a means to obtain access prior to device probing. I thought this patch not only provides that possibilty, but also leaves in the ability to register direct from DT. > In contrast, this patch introduces a new of_syscon_{un,}register() > interface that would get removed after the the above has > been implemented, causing extra churn for any driver that also > wants to provide a regmap-like interface. When will we ever not have to register syscon?
On Monday 01 September 2014 17:04:26 Lee Jones wrote: > On Mon, 01 Sep 2014, Arnd Bergmann wrote: > > Maybe I'm misreading the patch, but I don't see how it creates a > > migration path. What I want to end up with is infrastructure that > > lets anybody call syscon_regmap_lookup_by_pdevname or > > syscon_regmap_lookup_by_compatible (if they really need to) > > without needing the platform_driver for syscon. That should not > > require any form of compatibility layer because to the driver > > using it there is no API change. > > Somehow I think the likelyhood is that I am misreading the patch. > > I thought that before this patch drivers we had to register a syscon > device to bind to this driver, which was fine for the first use-cases > of syscon as it wasn't required too early during boot. However, now > there are use-cases where systems require access to syscon registers > eariler in boot we require a means to obtain access prior to device > probing. I thought this patch not only provides that possibilty, but > also leaves in the ability to register direct from DT. Right, it does provide the ability to have syscon before devices are registered, I missed that part. > > In contrast, this patch introduces a new of_syscon_{un,}register() > > interface that would get removed after the the above has > > been implemented, causing extra churn for any driver that also > > wants to provide a regmap-like interface. > > When will we ever not have to register syscon? The idea is that we implicitly register the syscon block when someone calls syscon_regmap_lookup_by_compatible or syscon_regmap_lookup_by_phandle and then return a reference to that new syscon. When another driver looks up the same device node, we just pass a reference to the existing syscon. Arnd
On Mon, 01 Sep 2014, Arnd Bergmann wrote: > On Monday 01 September 2014 17:04:26 Lee Jones wrote: > > On Mon, 01 Sep 2014, Arnd Bergmann wrote: > > > Maybe I'm misreading the patch, but I don't see how it creates a > > > migration path. What I want to end up with is infrastructure that > > > lets anybody call syscon_regmap_lookup_by_pdevname or > > > syscon_regmap_lookup_by_compatible (if they really need to) > > > without needing the platform_driver for syscon. That should not > > > require any form of compatibility layer because to the driver > > > using it there is no API change. > > > > Somehow I think the likelyhood is that I am misreading the patch. > > > > I thought that before this patch drivers we had to register a syscon > > device to bind to this driver, which was fine for the first use-cases > > of syscon as it wasn't required too early during boot. However, now > > there are use-cases where systems require access to syscon registers > > eariler in boot we require a means to obtain access prior to device > > probing. I thought this patch not only provides that possibilty, but > > also leaves in the ability to register direct from DT. > > Right, it does provide the ability to have syscon before devices > are registered, I missed that part. > > > > In contrast, this patch introduces a new of_syscon_{un,}register() > > > interface that would get removed after the the above has > > > been implemented, causing extra churn for any driver that also > > > wants to provide a regmap-like interface. > > > > When will we ever not have to register syscon? > > The idea is that we implicitly register the syscon block when someone > calls syscon_regmap_lookup_by_compatible or syscon_regmap_lookup_by_phandle > and then return a reference to that new syscon. When another driver > looks up the same device node, we just pass a reference to the existing > syscon. Doesn't sound too unreasonable. So how about instead of exporting these new of_syscon_{un,}register() calls, we make them static and call them from syscon_regmap_lookup_by_{phandle,compatible}?
On Tuesday 02 September 2014 09:05:16 Lee Jones wrote: > On Mon, 01 Sep 2014, Arnd Bergmann wrote: > > > On Monday 01 September 2014 17:04:26 Lee Jones wrote: > > > On Mon, 01 Sep 2014, Arnd Bergmann wrote: > > > > Maybe I'm misreading the patch, but I don't see how it creates a > > > > migration path. What I want to end up with is infrastructure that > > > > lets anybody call syscon_regmap_lookup_by_pdevname or > > > > syscon_regmap_lookup_by_compatible (if they really need to) > > > > without needing the platform_driver for syscon. That should not > > > > require any form of compatibility layer because to the driver > > > > using it there is no API change. > > > > > > Somehow I think the likelyhood is that I am misreading the patch. > > > > > > I thought that before this patch drivers we had to register a syscon > > > device to bind to this driver, which was fine for the first use-cases > > > of syscon as it wasn't required too early during boot. However, now > > > there are use-cases where systems require access to syscon registers > > > eariler in boot we require a means to obtain access prior to device > > > probing. I thought this patch not only provides that possibilty, but > > > also leaves in the ability to register direct from DT. > > > > Right, it does provide the ability to have syscon before devices > > are registered, I missed that part. > > > > > > In contrast, this patch introduces a new of_syscon_{un,}register() > > > > interface that would get removed after the the above has > > > > been implemented, causing extra churn for any driver that also > > > > wants to provide a regmap-like interface. > > > > > > When will we ever not have to register syscon? > > > > The idea is that we implicitly register the syscon block when someone > > calls syscon_regmap_lookup_by_compatible or syscon_regmap_lookup_by_phandle > > and then return a reference to that new syscon. When another driver > > looks up the same device node, we just pass a reference to the existing > > syscon. > > Doesn't sound too unreasonable. So how about instead of exporting > these new of_syscon_{un,}register() calls, we make them static and > call them from syscon_regmap_lookup_by_{phandle,compatible}? Yes, that would be a good start. We should think about whether we want to remove the existing DT probing at the same time, since it becomes unused, and we might want to move the code to drivers/base/regmap_*.c at some point. Arnd
> -----Original Message----- > From: Arnd Bergmann [mailto:arnd@arndb.de] > Sent: Tuesday, September 02, 2014 1:45 PM > To: linux-arm-kernel@lists.infradead.org > Cc: Lee Jones; kgene.kim@samsung.com; linux@arm.linux.org.uk; > naushad@samsung.com; Pankaj Dubey; Tomasz Figa; linux-kernel@vger.kernel.org; > joshi@samsung.com; vikas.sajjan@samsung.com; linux-samsung- > soc@vger.kernel.org; broonie@kernel.org; thomas.ab@samsung.com; > chow.kim@samsung.com > Subject: Re: [PATCH] mfd: syscon: Decouple syscon interface from syscon devices > > On Tuesday 02 September 2014 09:05:16 Lee Jones wrote: > > On Mon, 01 Sep 2014, Arnd Bergmann wrote: > > > > > On Monday 01 September 2014 17:04:26 Lee Jones wrote: > > > > On Mon, 01 Sep 2014, Arnd Bergmann wrote: > > > > > Maybe I'm misreading the patch, but I don't see how it creates a > > > > > migration path. What I want to end up with is infrastructure > > > > > that lets anybody call syscon_regmap_lookup_by_pdevname or > > > > > syscon_regmap_lookup_by_compatible (if they really need to) > > > > > without needing the platform_driver for syscon. That should not > > > > > require any form of compatibility layer because to the driver > > > > > using it there is no API change. > > > > > > > > Somehow I think the likelyhood is that I am misreading the patch. > > > > > > > > I thought that before this patch drivers we had to register a > > > > syscon device to bind to this driver, which was fine for the first > > > > use-cases of syscon as it wasn't required too early during boot. > > > > However, now there are use-cases where systems require access to > > > > syscon registers eariler in boot we require a means to obtain > > > > access prior to device probing. I thought this patch not only > > > > provides that possibilty, but also leaves in the ability to register direct from > DT. > > > > > > Right, it does provide the ability to have syscon before devices are > > > registered, I missed that part. > > > > > > > > In contrast, this patch introduces a new > > > > > of_syscon_{un,}register() interface that would get removed after > > > > > the the above has been implemented, causing extra churn for any > > > > > driver that also wants to provide a regmap-like interface. > > > > > > > > When will we ever not have to register syscon? > > > > > > The idea is that we implicitly register the syscon block when > > > someone calls syscon_regmap_lookup_by_compatible or > > > syscon_regmap_lookup_by_phandle and then return a reference to that > > > new syscon. When another driver looks up the same device node, we > > > just pass a reference to the existing syscon. > > > > Doesn't sound too unreasonable. So how about instead of exporting > > these new of_syscon_{un,}register() calls, we make them static and > > call them from syscon_regmap_lookup_by_{phandle,compatible}? > > Yes, that would be a good start. We should think about whether we want to remove > the existing DT probing at the same time, since it becomes unused, and we might > want to move the code to drivers/base/regmap_*.c at some point. > OK, I am working on these two parts. Once code is in proper form and tested on our setup I will post here. Thanks, Pankaj > Arnd
On Tue, 02 Sep 2014, Arnd Bergmann wrote: > On Tuesday 02 September 2014 09:05:16 Lee Jones wrote: > > On Mon, 01 Sep 2014, Arnd Bergmann wrote: > > > > > On Monday 01 September 2014 17:04:26 Lee Jones wrote: > > > > On Mon, 01 Sep 2014, Arnd Bergmann wrote: > > > > > Maybe I'm misreading the patch, but I don't see how it creates a > > > > > migration path. What I want to end up with is infrastructure that > > > > > lets anybody call syscon_regmap_lookup_by_pdevname or > > > > > syscon_regmap_lookup_by_compatible (if they really need to) > > > > > without needing the platform_driver for syscon. That should not > > > > > require any form of compatibility layer because to the driver > > > > > using it there is no API change. > > > > > > > > Somehow I think the likelyhood is that I am misreading the patch. > > > > > > > > I thought that before this patch drivers we had to register a syscon > > > > device to bind to this driver, which was fine for the first use-cases > > > > of syscon as it wasn't required too early during boot. However, now > > > > there are use-cases where systems require access to syscon registers > > > > eariler in boot we require a means to obtain access prior to device > > > > probing. I thought this patch not only provides that possibilty, but > > > > also leaves in the ability to register direct from DT. > > > > > > Right, it does provide the ability to have syscon before devices > > > are registered, I missed that part. > > > > > > > > In contrast, this patch introduces a new of_syscon_{un,}register() > > > > > interface that would get removed after the the above has > > > > > been implemented, causing extra churn for any driver that also > > > > > wants to provide a regmap-like interface. > > > > > > > > When will we ever not have to register syscon? > > > > > > The idea is that we implicitly register the syscon block when someone > > > calls syscon_regmap_lookup_by_compatible or syscon_regmap_lookup_by_phandle > > > and then return a reference to that new syscon. When another driver > > > looks up the same device node, we just pass a reference to the existing > > > syscon. > > > > Doesn't sound too unreasonable. So how about instead of exporting > > these new of_syscon_{un,}register() calls, we make them static and > > call them from syscon_regmap_lookup_by_{phandle,compatible}? > > Yes, that would be a good start. We should think about whether we want > to remove the existing DT probing at the same time, since it becomes > unused, and we might want to move the code to drivers/base/regmap_*.c > at some point I'd support that move - and can even draft the patch if required.
diff --git a/drivers/mfd/syscon.c b/drivers/mfd/syscon.c index ca15878..a91db30 100644 --- a/drivers/mfd/syscon.c +++ b/drivers/mfd/syscon.c @@ -14,6 +14,7 @@ #include <linux/err.h> #include <linux/io.h> +#include <linux/list.h> #include <linux/module.h> #include <linux/of.h> #include <linux/of_address.h> @@ -22,33 +23,32 @@ #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; -} - 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); + + return syscon ? syscon->regmap : ERR_PTR(-EPROBE_DEFER); } EXPORT_SYMBOL_GPL(syscon_node_to_regmap); @@ -68,24 +68,22 @@ 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); + struct syscon *entry, *syscon = NULL; + struct platform_device *pdev = NULL; + + spin_lock(&syscon_list_slock); + list_for_each_entry(entry, &syscon_list, list) { + pdev = of_find_device_by_node(entry->np); + if (pdev && !strcmp(dev_name(&pdev->dev), s)) { + syscon = entry; + break; + } + } + spin_unlock(&syscon_list_slock); - return syscon->regmap; + return syscon ? syscon->regmap : ERR_PTR(-EPROBE_DEFER); } EXPORT_SYMBOL_GPL(syscon_regmap_lookup_by_pdevname); @@ -121,17 +119,49 @@ static struct regmap_config syscon_regmap_config = { .reg_stride = 4, }; +void of_syscon_unregister(struct device_node *np) +{ + struct syscon *entry; + + spin_lock(&syscon_list_slock); + + list_for_each_entry(entry, &syscon_list, list) + if (entry->np == np) { + list_del(&entry->list); + break; + } + + spin_unlock(&syscon_list_slock); +} +EXPORT_SYMBOL_GPL(of_syscon_unregister); + +int of_syscon_register(struct device_node *np, struct regmap *regmap) +{ + struct syscon *syscon; + + syscon = kzalloc(sizeof(*syscon), GFP_KERNEL); + if (!syscon) + return -ENOMEM; + + syscon->regmap = regmap; + syscon->np = np; + + spin_lock(&syscon_list_slock); + list_add_tail(&syscon->list, &syscon_list); + spin_unlock(&syscon_list_slock); + + return 0; +} +EXPORT_SYMBOL_GPL(of_syscon_register); + static int syscon_probe(struct platform_device *pdev) { struct device *dev = &pdev->dev; struct syscon_platform_data *pdata = dev_get_platdata(dev); - struct syscon *syscon; + struct regmap *regmap; struct resource *res; void __iomem *base; - - syscon = devm_kzalloc(dev, sizeof(*syscon), GFP_KERNEL); - if (!syscon) - return -ENOMEM; + int ret; res = platform_get_resource(pdev, IORESOURCE_MEM, 0); if (!res) @@ -144,14 +174,16 @@ static int syscon_probe(struct platform_device *pdev) 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, + regmap = devm_regmap_init_mmio(dev, base, &syscon_regmap_config); - if (IS_ERR(syscon->regmap)) { + if (IS_ERR(regmap)) { dev_err(dev, "regmap init failed\n"); - return PTR_ERR(syscon->regmap); + return PTR_ERR(regmap); } - platform_set_drvdata(pdev, syscon); + ret = of_syscon_register(dev->of_node, regmap); + if (ret) + return ret; dev_dbg(dev, "regmap %pR registered\n", res); diff --git a/include/linux/mfd/syscon.h b/include/linux/mfd/syscon.h index 75e543b..dc2807b 100644 --- a/include/linux/mfd/syscon.h +++ b/include/linux/mfd/syscon.h @@ -18,8 +18,12 @@ #include <linux/err.h> struct device_node; +struct regmap; #ifdef CONFIG_MFD_SYSCON +extern int of_syscon_register(struct device_node *np, struct regmap *regmap); +extern void of_syscon_unregister(struct device_node *np); + extern struct regmap *syscon_node_to_regmap(struct device_node *np); extern struct regmap *syscon_regmap_lookup_by_compatible(const char *s); extern struct regmap *syscon_regmap_lookup_by_pdevname(const char *s); @@ -27,6 +31,16 @@ extern struct regmap *syscon_regmap_lookup_by_phandle( struct device_node *np, const char *property); #else +static inline int of_syscon_register(struct device_node *np, + struct regmap *regmap) +{ + return -ENOSYS; +} + +static inline void of_syscon_unregister(struct device_node *np) +{ +} + static inline struct regmap *syscon_node_to_regmap(struct device_node *np) { return ERR_PTR(-ENOSYS);