The existing SYSCON implementation only supports MMIO (memory mapped)
accesses, facilitated by Regmap. This extends support for registers
held behind I2C busses.
Signed-off-by: Lee Jones <[email protected]>
---
Changelog:
v2 => v3
- Change 'is CONFIG' present check to include loadable modules
- s/#ifdef CONFIG_MFD_SYSCON_I2C/#if IS_ENABLED(CONFIG_MFD_SYSCON_I2C)/
v1 => v2
- Remove legacy references to OF
- Allow building as a module (fixes h8300 0-day issue)
drivers/mfd/Kconfig | 7 +++
drivers/mfd/Makefile | 1 +
drivers/mfd/syscon-i2c.c | 90 ++++++++++++++++++++++++++++++++++
include/linux/mfd/syscon-i2c.h | 26 ++++++++++
4 files changed, 124 insertions(+)
create mode 100644 drivers/mfd/syscon-i2c.c
create mode 100644 include/linux/mfd/syscon-i2c.h
diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
index 0a59249198d34..f25f80f68edca 100644
--- a/drivers/mfd/Kconfig
+++ b/drivers/mfd/Kconfig
@@ -1300,6 +1300,13 @@ config MFD_SYSCON
Select this option to enable accessing system control registers
via regmap.
+config MFD_SYSCON_I2C
+ tristate "System Controller Register R/W Based on I2C Regmap"
+ select REGMAP_I2C
+ help
+ Select this option to enable accessing system control registers
+ via I2C using regmap.
+
config MFD_DAVINCI_VOICECODEC
tristate
select MFD_CORE
diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
index f935d10cbf0fc..0aec1f42ac979 100644
--- a/drivers/mfd/Makefile
+++ b/drivers/mfd/Makefile
@@ -219,6 +219,7 @@ obj-$(CONFIG_MFD_RK808) += rk808.o
obj-$(CONFIG_MFD_RN5T618) += rn5t618.o
obj-$(CONFIG_MFD_SEC_CORE) += sec-core.o sec-irq.o
obj-$(CONFIG_MFD_SYSCON) += syscon.o
+obj-$(CONFIG_MFD_SYSCON_I2C) += syscon-i2c.o
obj-$(CONFIG_MFD_LM3533) += lm3533-core.o lm3533-ctrlbank.o
obj-$(CONFIG_MFD_VEXPRESS_SYSREG) += vexpress-sysreg.o
obj-$(CONFIG_MFD_RETU) += retu-mfd.o
diff --git a/drivers/mfd/syscon-i2c.c b/drivers/mfd/syscon-i2c.c
new file mode 100644
index 0000000000000..be20ff45ece07
--- /dev/null
+++ b/drivers/mfd/syscon-i2c.c
@@ -0,0 +1,90 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * System Control Driver accessed over I2C
+ *
+ * Copyright (C) 2020 Linaro Ltd.
+ *
+ * Author: Lee Jones <[email protected]>
+ */
+
+#include <linux/device.h>
+#include <linux/err.h>
+#include <linux/i2c.h>
+#include <linux/list.h>
+#include <linux/mfd/syscon-i2c.h>
+#include <linux/regmap.h>
+
+static DEFINE_SPINLOCK(syscon_i2c_list_slock);
+static LIST_HEAD(syscon_i2c_list);
+
+struct syscon {
+ struct device *dev;
+ struct regmap *regmap;
+ struct list_head list;
+};
+
+static const struct regmap_config syscon_i2c_regmap_config = {
+ .reg_bits = 8,
+ .val_bits = 8,
+};
+
+static struct syscon *syscon_i2c_register(struct i2c_client *client)
+{
+ struct regmap_config syscon_config = syscon_i2c_regmap_config;
+ struct device *dev = &client->dev;
+ struct syscon *syscon;
+ struct regmap *regmap;
+ int ret;
+
+ syscon = devm_kzalloc(dev, sizeof(*syscon), GFP_KERNEL);
+ if (!syscon)
+ return ERR_PTR(-ENOMEM);
+
+ syscon_config.name = dev_name(dev);
+
+ regmap = devm_regmap_init_i2c(client, &syscon_config);
+ if (IS_ERR(regmap)) {
+ dev_err(dev, "Failed to initialise Regmap I2C\n");
+ ret = PTR_ERR(regmap);
+ return ERR_PTR(ret);
+ }
+
+ syscon->regmap = regmap;
+ syscon->dev = dev;
+
+ spin_lock(&syscon_i2c_list_slock);
+ list_add_tail(&syscon->list, &syscon_i2c_list);
+ spin_unlock(&syscon_i2c_list_slock);
+
+ return syscon;
+}
+
+static struct regmap *syscon_i2c_get_regmap(struct i2c_client *client)
+{
+ struct device *dev = &client->dev;
+ struct syscon *entry, *syscon = NULL;
+
+ spin_lock(&syscon_i2c_list_slock);
+
+ list_for_each_entry(entry, &syscon_i2c_list, list)
+ if (entry->dev == dev) {
+ syscon = entry;
+ break;
+ }
+
+ spin_unlock(&syscon_i2c_list_slock);
+
+ if (!syscon)
+ syscon = syscon_i2c_register(client);
+
+ if (IS_ERR(syscon))
+ return ERR_CAST(syscon);
+
+ return syscon->regmap;
+}
+
+struct regmap *syscon_i2c_to_regmap(struct i2c_client *client)
+{
+ return syscon_i2c_get_regmap(client);
+}
+EXPORT_SYMBOL_GPL(syscon_i2c_to_regmap);
diff --git a/include/linux/mfd/syscon-i2c.h b/include/linux/mfd/syscon-i2c.h
new file mode 100644
index 0000000000000..490bc37b603cb
--- /dev/null
+++ b/include/linux/mfd/syscon-i2c.h
@@ -0,0 +1,26 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+/*
+ * System Control Driver accessed via I2C
+ *
+ * Copyright (C) 2020 Linaro Ltd.
+ *
+ * Author: Lee Jones <[email protected]>
+ */
+
+#ifndef __LINUX_MFD_SYSCON_I2C_H__
+#define __LINUX_MFD_SYSCON_I2C_H__
+
+#include <linux/err.h>
+#include <linux/errno.h>
+#include <linux/i2c.h>
+
+#if IS_ENABLED(CONFIG_MFD_SYSCON_I2C)
+extern struct regmap *syscon_i2c_to_regmap(struct i2c_client *client);
+#else
+static inline struct regmap *syscon_i2c_to_regmap(struct i2c_client *client)
+{
+ return ERR_PTR(-ENOTSUPP);
+}
+#endif
+
+#endif /* __LINUX_MFD_SYSCON_I2C_H__ */
--
2.25.1
On Thu, Jun 18, 2020 at 10:03 AM Lee Jones <[email protected]> wrote:
>
> The existing SYSCON implementation only supports MMIO (memory mapped)
> accesses, facilitated by Regmap. This extends support for registers
> held behind I2C busses.
>
> Signed-off-by: Lee Jones <[email protected]>
The implementation looks fine to me, but can you explain how this is going to
be used, and what the advantage is over open-coding the devm_regmap_init_i2c()
in each driver that would use this?
Is this about using proper locking through the regmap framework for
shared i2c clients, or to reduce memory consumption when lots of drivers
access the same regmap?
My impression of the existing syscon code is that the main value-add over
other ways of doing the same is the syscon_regmap_lookup_by_phandle()
interface that gives other drivers a much simpler way of getting the
regmap just based on the DT node. Are you planning to add something
like that here as well? An ideal driver interface might allow
syscon_regmap_lookup_by_phandle() to work for both mmio and i2c
based syscons, or additional ones as well, but implementing this would
be rather tricky when the i2c core is a loadable module.
Arnd
On Thu, Jun 18, 2020 at 11:07:04AM +0100, Lee Jones wrote:
> Does Regmap let you register/initialise an I2C address more than once?
> When I attempt it, I get:
> [ 0.522988] i2c i2c-0: Failed to register i2c client tmp105 at 0x32 (-16)
That's not regmap, that's the I2C core.
On Thu, 18 Jun 2020, Arnd Bergmann wrote:
> On Thu, Jun 18, 2020 at 10:03 AM Lee Jones <[email protected]> wrote:
> >
> > The existing SYSCON implementation only supports MMIO (memory mapped)
> > accesses, facilitated by Regmap. This extends support for registers
> > held behind I2C busses.
> >
> > Signed-off-by: Lee Jones <[email protected]>
>
> The implementation looks fine to me, but can you explain how this is going to
> be used, and what the advantage is over open-coding the devm_regmap_init_i2c()
> in each driver that would use this?
Does Regmap let you register/initialise an I2C address more than once?
When I attempt it, I get:
[ 0.522988] i2c i2c-0: Failed to register i2c client tmp105 at 0x32 (-16)
[ 0.523341] i2c i2c-0: of_i2c: Failure registering /bus@4000000/motherboard/iofpga@7,00000000/i2c@16000/temp@32
[ 0.523691] i2c i2c-0: Failed to create I2C device for /bus@4000000/motherboard/iofpga@7,00000000/i2c@16000/temp@32
> Is this about using proper locking through the regmap framework for
> shared i2c clients, or to reduce memory consumption when lots of drivers
> access the same regmap?
All of those things are valid.
My use-case is regarding MFDs sharing an I2C interfaced address space
with their children.
> My impression of the existing syscon code is that the main value-add over
> other ways of doing the same is the syscon_regmap_lookup_by_phandle()
> interface that gives other drivers a much simpler way of getting the
> regmap just based on the DT node. Are you planning to add something
> like that here as well? An ideal driver interface might allow
> syscon_regmap_lookup_by_phandle() to work for both mmio and i2c
> based syscons, or additional ones as well, but implementing this would
> be rather tricky when the i2c core is a loadable module.
I expect the API would be expanded to cover other use-cases. This is
a bare bones implementation which has been kept as atomic as possible
for ease of review.
--
Lee Jones [李琼斯]
Senior Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog
On Thu, Jun 18, 2020 at 6:07 PM Lee Jones <[email protected]> wrote:
>
> On Thu, 18 Jun 2020, Arnd Bergmann wrote:
>
> > On Thu, Jun 18, 2020 at 10:03 AM Lee Jones <[email protected]> wrote:
> > >
> > > The existing SYSCON implementation only supports MMIO (memory mapped)
> > > accesses, facilitated by Regmap. This extends support for registers
> > > held behind I2C busses.
> > >
> > > Signed-off-by: Lee Jones <[email protected]>
> >
> > The implementation looks fine to me, but can you explain how this is going to
> > be used, and what the advantage is over open-coding the devm_regmap_init_i2c()
> > in each driver that would use this?
>
> Does Regmap let you register/initialise an I2C address more than once?
>
> When I attempt it, I get:
>
> [ 0.522988] i2c i2c-0: Failed to register i2c client tmp105 at 0x32 (-16)
> [ 0.523341] i2c i2c-0: of_i2c: Failure registering /bus@4000000/motherboard/iofpga@7,00000000/i2c@16000/temp@32
> [ 0.523691] i2c i2c-0: Failed to create I2C device for /bus@4000000/motherboard/iofpga@7,00000000/i2c@16000/temp@32
>
> > Is this about using proper locking through the regmap framework for
> > shared i2c clients, or to reduce memory consumption when lots of drivers
> > access the same regmap?
>
> All of those things are valid.
>
> My use-case is regarding MFDs sharing an I2C interfaced address space
> with their children.
Is that an issue with the standard mfd + regmap pattern?
For the AXP20x PMICs, we register the regmap in the parent mfd driver [1],
and store that in dev_data for child drivers to fetch [2]. You could
easily just fetch the regmap with dev_get_regmap() and a pointer to the
parent device.
> > My impression of the existing syscon code is that the main value-add over
> > other ways of doing the same is the syscon_regmap_lookup_by_phandle()
> > interface that gives other drivers a much simpler way of getting the
> > regmap just based on the DT node. Are you planning to add something
> > like that here as well? An ideal driver interface might allow
> > syscon_regmap_lookup_by_phandle() to work for both mmio and i2c
> > based syscons, or additional ones as well, but implementing this would
> > be rather tricky when the i2c core is a loadable module.
The current MMIO syscon is decoupled from the DM, and there is no way
for drivers to export or register a syscon, meaning I have to open code
syscon_regmap_lookup_by_phandle() [3] if I want to only expose certain
registers and not the full address range, or if I want to share the
regmap with the existing driver (for locking purposes), or both [4].
Maybe there's room for improvement here? The same applies to the new
I2C case, and likely any other future syscon variants.
IMHO people are getting it wrong if they have both a syscon and a driver
for the same device.
> I expect the API would be expanded to cover other use-cases. This is
> a bare bones implementation which has been kept as atomic as possible
> for ease of review.
Regards
ChenYu
[1] https://elixir.bootlin.com/linux/latest/source/drivers/mfd/axp20x-i2c.c#L43
[2] https://elixir.bootlin.com/linux/latest/source/drivers/pinctrl/pinctrl-axp209.c#L433
[3] https://elixir.bootlin.com/linux/latest/source/drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c#L1093
[4] https://elixir.bootlin.com/linux/latest/source/drivers/clk/sunxi-ng/ccu-sun8i-r40.c#L1333
On Fri, Jun 19, 2020 at 2:41 PM Lee Jones <[email protected]> wrote:
>
> On Fri, 19 Jun 2020, Chen-Yu Tsai wrote:
>
> > On Thu, Jun 18, 2020 at 6:07 PM Lee Jones <[email protected]> wrote:
> > >
> > > On Thu, 18 Jun 2020, Arnd Bergmann wrote:
> > >
> > > > On Thu, Jun 18, 2020 at 10:03 AM Lee Jones <[email protected]> wrote:
> > > > >
> > > > > The existing SYSCON implementation only supports MMIO (memory mapped)
> > > > > accesses, facilitated by Regmap. This extends support for registers
> > > > > held behind I2C busses.
> > > > >
> > > > > Signed-off-by: Lee Jones <[email protected]>
> > > >
> > > > The implementation looks fine to me, but can you explain how this is going to
> > > > be used, and what the advantage is over open-coding the devm_regmap_init_i2c()
> > > > in each driver that would use this?
> > >
> > > Does Regmap let you register/initialise an I2C address more than once?
> > >
> > > When I attempt it, I get:
> > >
> > > [ 0.522988] i2c i2c-0: Failed to register i2c client tmp105 at 0x32 (-16)
> > > [ 0.523341] i2c i2c-0: of_i2c: Failure registering /bus@4000000/motherboard/iofpga@7,00000000/i2c@16000/temp@32
> > > [ 0.523691] i2c i2c-0: Failed to create I2C device for /bus@4000000/motherboard/iofpga@7,00000000/i2c@16000/temp@32
> > >
> > > > Is this about using proper locking through the regmap framework for
> > > > shared i2c clients, or to reduce memory consumption when lots of drivers
> > > > access the same regmap?
> > >
> > > All of those things are valid.
> > >
> > > My use-case is regarding MFDs sharing an I2C interfaced address space
> > > with their children.
> >
> > Is that an issue with the standard mfd + regmap pattern?
>
> There is no relationship between MFD and Regmap. It is not more
> closely related to Regmap than it is any other public API provided
> within the kernel. *Some* parent drivers initialise one large,
> encompassing Regmap address space and pass it to their children, but
> this isn't suitable in all cases.
>
> > For the AXP20x PMICs, we register the regmap in the parent mfd driver [1],
> > and store that in dev_data for child drivers to fetch [2]. You could
> > easily just fetch the regmap with dev_get_regmap() and a pointer to the
> > parent device.
>
> Remember, not all use-cases are the same. Just because your H/W fits
> well within the current framework, doesn't mean all will.
>
> Initialising in the parent is no problem if the driver is meaningful
> in other ways, but what if that's all the parent driver does? In
> these cases Syscon can be used instead, rendering the driver
> superfluous. Meaning it can (and *should*) then be omitted.
I'm guessing in your use case there isn't a need for a parent driver,
and you are looking for something like "simple-mfd", but for listing
sub-devices within an I2C slave device? In that case I understand.
> > > > My impression of the existing syscon code is that the main value-add over
> > > > other ways of doing the same is the syscon_regmap_lookup_by_phandle()
> > > > interface that gives other drivers a much simpler way of getting the
> > > > regmap just based on the DT node. Are you planning to add something
> > > > like that here as well? An ideal driver interface might allow
> > > > syscon_regmap_lookup_by_phandle() to work for both mmio and i2c
> > > > based syscons, or additional ones as well, but implementing this would
> > > > be rather tricky when the i2c core is a loadable module.
> >
> > The current MMIO syscon is decoupled from the DM, and there is no way
> > for drivers to export or register a syscon, meaning I have to open code
> > syscon_regmap_lookup_by_phandle() [3] if I want to only expose certain
> > registers and not the full address range, or if I want to share the
> > regmap with the existing driver (for locking purposes), or both [4].
>
> Not sure I understand the problem.
>
> Could you explain why the current implementation doesn't work for you?
>
> Open coding your own implementation of Syscon is non-optimal.
For the DWMAC Ethernet controllers, the platform glue almost always has
a register for tuning the delays of the TX and RX clocks. In almost all
later Allwinner chips, this is in a separate area, which we use a syscon
for. However in one hybrid chip, this is located in the clock controller.
We deemed it risky to also have the whole clock controller address range
mapped as a syscon, and so we export a custom regmap.
The Ethernet driver has to deal with both cases.
Looking at it again, since syscon still has a platform driver, maybe I
should just use the dev_get_regmap() route for both cases.
> > Maybe there's room for improvement here? The same applies to the new
> > I2C case, and likely any other future syscon variants.
> >
> > IMHO people are getting it wrong if they have both a syscon and a driver
> > for the same device.
>
> Syscon is just a means to obtain a group of registers either a)
> without a dedicated driver OR b) to share amongst more than 1,
> potentially unrelated, user. So in the case of a) which appears to
> sit well with-in your use-case and expectations, you are correct.
> Whereas in the case of b) you are not.
>
> I hope that helps clarity the situation somewhat.
The concern was mostly due to the commit message of
bdb0066df96e mfd: syscon: Decouple syscon interface from platform devices
which mentions
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.
But does not provide any sort of coordination between the dedicated driver
and the syscon. I suppose the intention might have been that the driver
would get a syscon using its own device node. We avoided that but I wonder
if the extra code is worth it or not. Other platforms seem to do ok.
Thank you for helping me clear things up.
Regards
ChenYu
> > > I expect the API would be expanded to cover other use-cases. This is
> > > a bare bones implementation which has been kept as atomic as possible
> > > for ease of review.
> >
> > Regards
> > ChenYu
> >
> > [1] https://elixir.bootlin.com/linux/latest/source/drivers/mfd/axp20x-i2c.c#L43
> > [2] https://elixir.bootlin.com/linux/latest/source/drivers/pinctrl/pinctrl-axp209.c#L433
> > [3] https://elixir.bootlin.com/linux/latest/source/drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c#L1093
> > [4] https://elixir.bootlin.com/linux/latest/source/drivers/clk/sunxi-ng/ccu-sun8i-r40.c#L1333
>
> --
> Lee Jones [李琼斯]
> Senior Technical Lead - Developer Services
> Linaro.org │ Open source software for Arm SoCs
> Follow Linaro: Facebook | Twitter | Blog
On Fri, 19 Jun 2020, Chen-Yu Tsai wrote:
> On Thu, Jun 18, 2020 at 6:07 PM Lee Jones <[email protected]> wrote:
> >
> > On Thu, 18 Jun 2020, Arnd Bergmann wrote:
> >
> > > On Thu, Jun 18, 2020 at 10:03 AM Lee Jones <[email protected]> wrote:
> > > >
> > > > The existing SYSCON implementation only supports MMIO (memory mapped)
> > > > accesses, facilitated by Regmap. This extends support for registers
> > > > held behind I2C busses.
> > > >
> > > > Signed-off-by: Lee Jones <[email protected]>
> > >
> > > The implementation looks fine to me, but can you explain how this is going to
> > > be used, and what the advantage is over open-coding the devm_regmap_init_i2c()
> > > in each driver that would use this?
> >
> > Does Regmap let you register/initialise an I2C address more than once?
> >
> > When I attempt it, I get:
> >
> > [ 0.522988] i2c i2c-0: Failed to register i2c client tmp105 at 0x32 (-16)
> > [ 0.523341] i2c i2c-0: of_i2c: Failure registering /bus@4000000/motherboard/iofpga@7,00000000/i2c@16000/temp@32
> > [ 0.523691] i2c i2c-0: Failed to create I2C device for /bus@4000000/motherboard/iofpga@7,00000000/i2c@16000/temp@32
> >
> > > Is this about using proper locking through the regmap framework for
> > > shared i2c clients, or to reduce memory consumption when lots of drivers
> > > access the same regmap?
> >
> > All of those things are valid.
> >
> > My use-case is regarding MFDs sharing an I2C interfaced address space
> > with their children.
>
> Is that an issue with the standard mfd + regmap pattern?
There is no relationship between MFD and Regmap. It is not more
closely related to Regmap than it is any other public API provided
within the kernel. *Some* parent drivers initialise one large,
encompassing Regmap address space and pass it to their children, but
this isn't suitable in all cases.
> For the AXP20x PMICs, we register the regmap in the parent mfd driver [1],
> and store that in dev_data for child drivers to fetch [2]. You could
> easily just fetch the regmap with dev_get_regmap() and a pointer to the
> parent device.
Remember, not all use-cases are the same. Just because your H/W fits
well within the current framework, doesn't mean all will.
Initialising in the parent is no problem if the driver is meaningful
in other ways, but what if that's all the parent driver does? In
these cases Syscon can be used instead, rendering the driver
superfluous. Meaning it can (and *should*) then be omitted.
> > > My impression of the existing syscon code is that the main value-add over
> > > other ways of doing the same is the syscon_regmap_lookup_by_phandle()
> > > interface that gives other drivers a much simpler way of getting the
> > > regmap just based on the DT node. Are you planning to add something
> > > like that here as well? An ideal driver interface might allow
> > > syscon_regmap_lookup_by_phandle() to work for both mmio and i2c
> > > based syscons, or additional ones as well, but implementing this would
> > > be rather tricky when the i2c core is a loadable module.
>
> The current MMIO syscon is decoupled from the DM, and there is no way
> for drivers to export or register a syscon, meaning I have to open code
> syscon_regmap_lookup_by_phandle() [3] if I want to only expose certain
> registers and not the full address range, or if I want to share the
> regmap with the existing driver (for locking purposes), or both [4].
Not sure I understand the problem.
Could you explain why the current implementation doesn't work for you?
Open coding your own implementation of Syscon is non-optimal.
> Maybe there's room for improvement here? The same applies to the new
> I2C case, and likely any other future syscon variants.
>
> IMHO people are getting it wrong if they have both a syscon and a driver
> for the same device.
Syscon is just a means to obtain a group of registers either a)
without a dedicated driver OR b) to share amongst more than 1,
potentially unrelated, user. So in the case of a) which appears to
sit well with-in your use-case and expectations, you are correct.
Whereas in the case of b) you are not.
I hope that helps clarity the situation somewhat.
> > I expect the API would be expanded to cover other use-cases. This is
> > a bare bones implementation which has been kept as atomic as possible
> > for ease of review.
>
> Regards
> ChenYu
>
> [1] https://elixir.bootlin.com/linux/latest/source/drivers/mfd/axp20x-i2c.c#L43
> [2] https://elixir.bootlin.com/linux/latest/source/drivers/pinctrl/pinctrl-axp209.c#L433
> [3] https://elixir.bootlin.com/linux/latest/source/drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c#L1093
> [4] https://elixir.bootlin.com/linux/latest/source/drivers/clk/sunxi-ng/ccu-sun8i-r40.c#L1333
--
Lee Jones [李琼斯]
Senior Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog
On Fri, 19 Jun 2020, Chen-Yu Tsai wrote:
> On Fri, Jun 19, 2020 at 2:41 PM Lee Jones <[email protected]> wrote:
> >
> > On Fri, 19 Jun 2020, Chen-Yu Tsai wrote:
> >
> > > On Thu, Jun 18, 2020 at 6:07 PM Lee Jones <[email protected]> wrote:
> > > >
> > > > On Thu, 18 Jun 2020, Arnd Bergmann wrote:
> > > >
> > > > > On Thu, Jun 18, 2020 at 10:03 AM Lee Jones <[email protected]> wrote:
> > > > > >
> > > > > > The existing SYSCON implementation only supports MMIO (memory mapped)
> > > > > > accesses, facilitated by Regmap. This extends support for registers
> > > > > > held behind I2C busses.
> > > > > >
> > > > > > Signed-off-by: Lee Jones <[email protected]>
> > > > >
> > > > > The implementation looks fine to me, but can you explain how this is going to
> > > > > be used, and what the advantage is over open-coding the devm_regmap_init_i2c()
> > > > > in each driver that would use this?
> > > >
> > > > Does Regmap let you register/initialise an I2C address more than once?
> > > >
> > > > When I attempt it, I get:
> > > >
> > > > [ 0.522988] i2c i2c-0: Failed to register i2c client tmp105 at 0x32 (-16)
> > > > [ 0.523341] i2c i2c-0: of_i2c: Failure registering /bus@4000000/motherboard/iofpga@7,00000000/i2c@16000/temp@32
> > > > [ 0.523691] i2c i2c-0: Failed to create I2C device for /bus@4000000/motherboard/iofpga@7,00000000/i2c@16000/temp@32
> > > >
> > > > > Is this about using proper locking through the regmap framework for
> > > > > shared i2c clients, or to reduce memory consumption when lots of drivers
> > > > > access the same regmap?
> > > >
> > > > All of those things are valid.
> > > >
> > > > My use-case is regarding MFDs sharing an I2C interfaced address space
> > > > with their children.
> > >
> > > Is that an issue with the standard mfd + regmap pattern?
> >
> > There is no relationship between MFD and Regmap. It is not more
> > closely related to Regmap than it is any other public API provided
> > within the kernel. *Some* parent drivers initialise one large,
> > encompassing Regmap address space and pass it to their children, but
> > this isn't suitable in all cases.
> >
> > > For the AXP20x PMICs, we register the regmap in the parent mfd driver [1],
> > > and store that in dev_data for child drivers to fetch [2]. You could
> > > easily just fetch the regmap with dev_get_regmap() and a pointer to the
> > > parent device.
> >
> > Remember, not all use-cases are the same. Just because your H/W fits
> > well within the current framework, doesn't mean all will.
> >
> > Initialising in the parent is no problem if the driver is meaningful
> > in other ways, but what if that's all the parent driver does? In
> > these cases Syscon can be used instead, rendering the driver
> > superfluous. Meaning it can (and *should*) then be omitted.
>
> I'm guessing in your use case there isn't a need for a parent driver,
> and you are looking for something like "simple-mfd", but for listing
> sub-devices within an I2C slave device? In that case I understand.
Bingo!
Actually this will be used *with* "simple-mfd". "simple-mfd" will
ensure the sub-devices are probed and "syscon" will allow them to
share an address space. This is currently possible for MMIO, but not
so for register maps located behind an I2C interface.
> > > > > My impression of the existing syscon code is that the main value-add over
> > > > > other ways of doing the same is the syscon_regmap_lookup_by_phandle()
> > > > > interface that gives other drivers a much simpler way of getting the
> > > > > regmap just based on the DT node. Are you planning to add something
> > > > > like that here as well? An ideal driver interface might allow
> > > > > syscon_regmap_lookup_by_phandle() to work for both mmio and i2c
> > > > > based syscons, or additional ones as well, but implementing this would
> > > > > be rather tricky when the i2c core is a loadable module.
> > >
> > > The current MMIO syscon is decoupled from the DM, and there is no way
> > > for drivers to export or register a syscon, meaning I have to open code
> > > syscon_regmap_lookup_by_phandle() [3] if I want to only expose certain
> > > registers and not the full address range, or if I want to share the
> > > regmap with the existing driver (for locking purposes), or both [4].
> >
> > Not sure I understand the problem.
> >
> > Could you explain why the current implementation doesn't work for you?
> >
> > Open coding your own implementation of Syscon is non-optimal.
>
> For the DWMAC Ethernet controllers, the platform glue almost always has
> a register for tuning the delays of the TX and RX clocks. In almost all
> later Allwinner chips, this is in a separate area, which we use a syscon
> for. However in one hybrid chip, this is located in the clock controller.
> We deemed it risky to also have the whole clock controller address range
> mapped as a syscon, and so we export a custom regmap.
>
> The Ethernet driver has to deal with both cases.
>
> Looking at it again, since syscon still has a platform driver, maybe I
> should just use the dev_get_regmap() route for both cases.
>
> > > Maybe there's room for improvement here? The same applies to the new
> > > I2C case, and likely any other future syscon variants.
> > >
> > > IMHO people are getting it wrong if they have both a syscon and a driver
> > > for the same device.
> >
> > Syscon is just a means to obtain a group of registers either a)
> > without a dedicated driver OR b) to share amongst more than 1,
> > potentially unrelated, user. So in the case of a) which appears to
> > sit well with-in your use-case and expectations, you are correct.
> > Whereas in the case of b) you are not.
> >
> > I hope that helps clarity the situation somewhat.
>
> The concern was mostly due to the commit message of
>
> bdb0066df96e mfd: syscon: Decouple syscon interface from platform devices
>
> which mentions
>
> 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.
>
> But does not provide any sort of coordination between the dedicated driver
> and the syscon. I suppose the intention might have been that the driver
> would get a syscon using its own device node. We avoided that but I wonder
> if the extra code is worth it or not. Other platforms seem to do ok.
What sort of co-ordination do you require beyond what is offered?
> Thank you for helping me clear things up.
Never a problem.
--
Lee Jones [李琼斯]
Senior Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog