[RFC PATCH 0/3] Add new reg_update_bits() support

[RFC PATCH 0/3] Add new reg_update_bits() support

[Baolin Wang]

The Spreadtrum platform uses a special set/clear method to update
registers' bits, thus this patch set introduces a new reg_update_bits()
callback, as well as adding a helper in syscon driver to support
this feature.

Any comments are welcome. Thanks.

Baolin Wang (3):
  mfd: syscon: Add reg_update_bits() callback support
  regmap: Add reg_update_bits() support
  soc: sprd: Add Spreadtrum special bits updating support

 drivers/base/regmap/regmap-mmio.c | 29 +++++++++++++++++++++-
 drivers/base/regmap/regmap.c      |  1 +
 drivers/mfd/syscon.c              | 10 ++++++++
 drivers/soc/Kconfig               |  1 +
 drivers/soc/Makefile              |  1 +
 drivers/soc/sprd/Kconfig          | 16 ++++++++++++
 drivers/soc/sprd/Makefile         |  2 ++
 drivers/soc/sprd/sprd_syscon.c    | 51 +++++++++++++++++++++++++++++++++++++++
 include/linux/mfd/syscon.h        |  8 ++++++
 include/linux/regmap.h            |  4 +++
 10 files changed, 122 insertions(+), 1 deletion(-)
 create mode 100644 drivers/soc/sprd/Kconfig
 create mode 100644 drivers/soc/sprd/Makefile
 create mode 100644 drivers/soc/sprd/sprd_syscon.c

-- 
1.9.1

[RFC PATCH 1/3] mfd: syscon: Add reg_update_bits() callback support

[Baolin Wang]

Some platforms such as Spreadtrum platform, define a special method to
update bits of the registers instead of reading and writing, thus add
a new reg_update_bits() callback for struct regmap_config and a helper
for syscon driver to support this new feature.

Signed-off-by: Baolin Wang <baolin.wang7@gmail.com>
---
 drivers/mfd/syscon.c       | 10 ++++++++++
 include/linux/mfd/syscon.h |  8 ++++++++
 include/linux/regmap.h     |  4 ++++
 3 files changed, 22 insertions(+)

diff --git a/drivers/mfd/syscon.c b/drivers/mfd/syscon.c
index 3a97816..cf6efe2 100644
--- a/drivers/mfd/syscon.c
+++ b/drivers/mfd/syscon.c
@@ -34,6 +34,8 @@ struct syscon {
 	struct list_head list;
 };
 
+static regmap_hw_reg_update_bits reg_update_bits;
+
 static const struct regmap_config syscon_regmap_config = {
 	.reg_bits = 32,
 	.val_bits = 32,
@@ -105,6 +107,7 @@ static struct syscon *of_syscon_register(struct device_node *np, bool check_clk)
 	syscon_config.reg_stride = reg_io_width;
 	syscon_config.val_bits = reg_io_width * 8;
 	syscon_config.max_register = resource_size(&res) - reg_io_width;
+	syscon_config.reg_update_bits = reg_update_bits;
 
 	regmap = regmap_init_mmio(NULL, base, &syscon_config);
 	if (IS_ERR(regmap)) {
@@ -172,6 +175,13 @@ static struct regmap *device_node_get_regmap(struct device_node *np,
 	return syscon->regmap;
 }
 
+void syscon_register_reg_update_bits(regmap_hw_reg_update_bits hw_reg_update_bits)
+{
+	if (hw_reg_update_bits)
+		reg_update_bits = hw_reg_update_bits;
+}
+EXPORT_SYMBOL_GPL(syscon_register_reg_update_bits);
+
 struct regmap *device_node_to_regmap(struct device_node *np)
 {
 	return device_node_get_regmap(np, false);
diff --git a/include/linux/mfd/syscon.h b/include/linux/mfd/syscon.h
index 7f20e9b..b64ef84 100644
--- a/include/linux/mfd/syscon.h
+++ b/include/linux/mfd/syscon.h
@@ -13,6 +13,7 @@
 
 #include <linux/err.h>
 #include <linux/errno.h>
+#include <linux/regmap.h>
 
 struct device_node;
 
@@ -28,6 +29,8 @@ extern struct regmap *syscon_regmap_lookup_by_phandle_args(
 					const char *property,
 					int arg_count,
 					unsigned int *out_args);
+extern void
+syscon_register_reg_update_bits(regmap_hw_reg_update_bits hw_reg_update_bits);
 #else
 static inline struct regmap *device_node_to_regmap(struct device_node *np)
 {
@@ -59,6 +62,11 @@ static inline struct regmap *syscon_regmap_lookup_by_phandle_args(
 {
 	return ERR_PTR(-ENOTSUPP);
 }
+
+static inline void
+syscon_register_reg_update_bits(regmap_hw_reg_update_bits hw_reg_update_bits)
+{
+}
 #endif
 
 #endif /* __LINUX_MFD_SYSCON_H__ */
diff --git a/include/linux/regmap.h b/include/linux/regmap.h
index 40b0716..78c1036 100644
--- a/include/linux/regmap.h
+++ b/include/linux/regmap.h
@@ -340,6 +340,8 @@ struct regmap_access_table {
  *		  read operation on a bus such as SPI, I2C, etc. Most of the
  *		  devices do not need this.
  * @reg_write:	  Same as above for writing.
+ * @reg_update_bits: Optional, should only be provided for devices whose update
+ *		     operation cannot be represented as read and write.
  * @fast_io:	  Register IO is fast. Use a spinlock instead of a mutex
  *	     	  to perform locking. This field is ignored if custom lock/unlock
  *	     	  functions are used (see fields lock/unlock of struct regmap_config).
@@ -416,6 +418,8 @@ struct regmap_config {
 
 	int (*reg_read)(void *context, unsigned int reg, unsigned int *val);
 	int (*reg_write)(void *context, unsigned int reg, unsigned int val);
+	int (*reg_update_bits)(void *context, unsigned int reg,
+			       unsigned int mask, unsigned int val);
 
 	bool fast_io;
 
-- 
1.9.1

[RFC PATCH 2/3] regmap: Add reg_update_bits() support

[Baolin Wang]

Add reg_update_bits() support in case some platforms use a special method
to update bits of registers.

Signed-off-by: Baolin Wang <baolin.wang7@gmail.com>
---
 drivers/base/regmap/regmap-mmio.c | 29 ++++++++++++++++++++++++++++-
 drivers/base/regmap/regmap.c      |  1 +
 2 files changed, 29 insertions(+), 1 deletion(-)

diff --git a/drivers/base/regmap/regmap-mmio.c b/drivers/base/regmap/regmap-mmio.c
index af967d8..dae0d28 100644
--- a/drivers/base/regmap/regmap-mmio.c
+++ b/drivers/base/regmap/regmap-mmio.c
@@ -24,6 +24,8 @@ struct regmap_mmio_context {
 			  unsigned int reg, unsigned int val);
 	unsigned int (*reg_read)(struct regmap_mmio_context *ctx,
 			         unsigned int reg);
+	int (*reg_update_bits)(void *ctx, unsigned int reg,
+			       unsigned int mask, unsigned int val);
 };
 
 static int regmap_mmio_regbits_check(size_t reg_bits)
@@ -188,6 +190,26 @@ static int regmap_mmio_read(void *context, unsigned int reg, unsigned int *val)
 	return 0;
 }
 
+static int regmap_mmio_update_bits(void *context, unsigned int reg,
+				   unsigned int mask, unsigned int val)
+{
+	struct regmap_mmio_context *ctx = context;
+	int ret;
+
+	if (!IS_ERR(ctx->clk)) {
+		ret = clk_enable(ctx->clk);
+		if (ret < 0)
+			return ret;
+	}
+
+	ctx->reg_update_bits(ctx->regs, reg, mask, val);
+
+	if (!IS_ERR(ctx->clk))
+		clk_disable(ctx->clk);
+
+	return 0;
+}
+
 static void regmap_mmio_free_context(void *context)
 {
 	struct regmap_mmio_context *ctx = context;
@@ -200,7 +222,7 @@ static void regmap_mmio_free_context(void *context)
 	kfree(context);
 }
 
-static const struct regmap_bus regmap_mmio = {
+static struct regmap_bus regmap_mmio = {
 	.fast_io = true,
 	.reg_write = regmap_mmio_write,
 	.reg_read = regmap_mmio_read,
@@ -239,6 +261,11 @@ static struct regmap_mmio_context *regmap_mmio_gen_context(struct device *dev,
 	ctx->val_bytes = config->val_bits / 8;
 	ctx->clk = ERR_PTR(-ENODEV);
 
+	if (config->reg_update_bits) {
+		ctx->reg_update_bits = config->reg_update_bits;
+		regmap_mmio.reg_update_bits = regmap_mmio_update_bits;
+	}
+
 	switch (regmap_get_val_endian(dev, &regmap_mmio, config)) {
 	case REGMAP_ENDIAN_DEFAULT:
 	case REGMAP_ENDIAN_LITTLE:
diff --git a/drivers/base/regmap/regmap.c b/drivers/base/regmap/regmap.c
index 59f911e..553d92a 100644
--- a/drivers/base/regmap/regmap.c
+++ b/drivers/base/regmap/regmap.c
@@ -827,6 +827,7 @@ struct regmap *__regmap_init(struct device *dev,
 	} else if (!bus->read || !bus->write) {
 		map->reg_read = _regmap_bus_reg_read;
 		map->reg_write = _regmap_bus_reg_write;
+		map->reg_update_bits = bus->reg_update_bits;
 
 		map->defer_caching = false;
 		goto skip_format_initialization;
-- 
1.9.1

[RFC PATCH 3/3] soc: sprd: Add Spreadtrum special bits updating support

[Baolin Wang]

The spreadtrum platform uses a special set/clear method to update
registers' bits, which can remove the race of updating the global
registers between the multiple subsystems.

Signed-off-by: Baolin Wang <baolin.wang7@gmail.com>
---
 drivers/soc/Kconfig            |  1 +
 drivers/soc/Makefile           |  1 +
 drivers/soc/sprd/Kconfig       | 16 +++++++++++++
 drivers/soc/sprd/Makefile      |  2 ++
 drivers/soc/sprd/sprd_syscon.c | 51 ++++++++++++++++++++++++++++++++++++++++++
 5 files changed, 71 insertions(+)
 create mode 100644 drivers/soc/sprd/Kconfig
 create mode 100644 drivers/soc/sprd/Makefile
 create mode 100644 drivers/soc/sprd/sprd_syscon.c

diff --git a/drivers/soc/Kconfig b/drivers/soc/Kconfig
index 1778f8c..a19cead 100644
--- a/drivers/soc/Kconfig
+++ b/drivers/soc/Kconfig
@@ -22,5 +22,6 @@ source "drivers/soc/ux500/Kconfig"
 source "drivers/soc/versatile/Kconfig"
 source "drivers/soc/xilinx/Kconfig"
 source "drivers/soc/zte/Kconfig"
+source "drivers/soc/sprd/Kconfig"
 
 endmenu
diff --git a/drivers/soc/Makefile b/drivers/soc/Makefile
index a39f17c..e3f6b1c 100644
--- a/drivers/soc/Makefile
+++ b/drivers/soc/Makefile
@@ -28,3 +28,4 @@ obj-$(CONFIG_ARCH_U8500)	+= ux500/
 obj-$(CONFIG_PLAT_VERSATILE)	+= versatile/
 obj-y				+= xilinx/
 obj-$(CONFIG_ARCH_ZX)		+= zte/
+obj-$(CONFIG_ARCH_SPRD)		+= sprd/
diff --git a/drivers/soc/sprd/Kconfig b/drivers/soc/sprd/Kconfig
new file mode 100644
index 0000000..38d1f59
--- /dev/null
+++ b/drivers/soc/sprd/Kconfig
@@ -0,0 +1,16 @@
+# SPDX-License-Identifier: GPL-2.0-only
+#
+# SPRD SoC drivers
+#
+
+menu "Spreadtrum SoC drivers"
+	depends on ARCH_SPRD || COMPILE_TEST
+
+config SPRD_SYSCON
+	tristate "Spreadtrum syscon support"
+	depends on ARCH_SPRD || COMPILE_TEST
+	help
+	  Say yes here to add support for the Spreadtrum syscon driver,
+	  which is used to implement the atomic method of bits updating.
+
+endmenu
diff --git a/drivers/soc/sprd/Makefile b/drivers/soc/sprd/Makefile
new file mode 100644
index 0000000..4d77155
--- /dev/null
+++ b/drivers/soc/sprd/Makefile
@@ -0,0 +1,2 @@
+# SPDX-License-Identifier: GPL-2.0-only
+obj-$(CONFIG_SPRD_SYSCON) += sprd_syscon.o
diff --git a/drivers/soc/sprd/sprd_syscon.c b/drivers/soc/sprd/sprd_syscon.c
new file mode 100644
index 0000000..00acf88
--- /dev/null
+++ b/drivers/soc/sprd/sprd_syscon.c
@@ -0,0 +1,51 @@
+//SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2020 Spreadtrum Communications Inc.
+ */
+#include <linux/io.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/mfd/syscon.h>
+
+#define SPRD_REG_SET_OFFSET	0x1000
+#define SPRD_REG_CLR_OFFSET	0x2000
+
+/*
+ * The Spreadtrum platform defines a special set/clear method to update
+ * registers' bits, which means it can write values to the register's SET
+ * address (offset is 0x1000) to set bits, and write values to the register's
+ * CLEAR address (offset is 0x2000) to clear bits.
+ *
+ * This set/clear method can help to remove the race of accessing the global
+ * registers between the multiple subsystems instead of using hardware
+ * spinlocks.
+ */
+static int sprd_syscon_update_bits(void *context, unsigned int reg,
+				   unsigned int mask, unsigned int val)
+{
+	void __iomem *base = context;
+	unsigned int set, clr;
+
+	set = val & mask;
+	clr = ~set & mask;
+
+	if (set)
+		writel(set, base + reg + SPRD_REG_SET_OFFSET);
+
+	if (clr)
+		writel(clr, base + reg + SPRD_REG_CLR_OFFSET);
+
+	return 0;
+}
+
+static int sprd_syscon_init(void)
+{
+	syscon_register_reg_update_bits(sprd_syscon_update_bits);
+
+	return 0;
+}
+core_initcall_sync(sprd_syscon_init);
+
+MODULE_DESCRIPTION("Spreadtrum syscon support");
+MODULE_AUTHOR("Baolin Wang <baolin.wang@unisoc.com>");
+MODULE_LICENSE("GPL v2");
-- 
1.9.1

Re: [RFC PATCH 0/3] Add new reg_update_bits() support

[Arnd Bergmann]

On Thu, Apr 9, 2020 at 10:58 AM Baolin Wang <baolin.wang7@gmail.com> wrote:
>
> The Spreadtrum platform uses a special set/clear method to update
> registers' bits, thus this patch set introduces a new reg_update_bits()
> callback, as well as adding a helper in syscon driver to support
> this feature.
>
> Any comments are welcome. Thanks.

This looks like a good idea to me, both the concept and the implementation.

The one thing I'd note is that we have a similar mechanism for devices made
by Sigmatel and later Freescale after they acquired them.
include/linux/stmp_device.h only holds a couple of definitions, so it's much
less abstract than your approach, but the idea is similar.

It would be nice in theory to move the sigmatel devices over to
reg_update_bits() in theory, but this seems unlikely to actually happen,
given that these are mostly obsolete drivers at this point.

      Arnd

Re: [RFC PATCH 0/3] Add new reg_update_bits() support

[Baolin Wang]

On Thu, Apr 9, 2020 at 5:15 PM Arnd Bergmann <arnd@arndb.de> wrote:
>
> On Thu, Apr 9, 2020 at 10:58 AM Baolin Wang <baolin.wang7@gmail.com> wrote:
> >
> > The Spreadtrum platform uses a special set/clear method to update
> > registers' bits, thus this patch set introduces a new reg_update_bits()
> > callback, as well as adding a helper in syscon driver to support
> > this feature.
> >
> > Any comments are welcome. Thanks.
>
> This looks like a good idea to me, both the concept and the implementation.

Thanks for your quick response :)

>
> The one thing I'd note is that we have a similar mechanism for devices made
> by Sigmatel and later Freescale after they acquired them.
> include/linux/stmp_device.h only holds a couple of definitions, so it's much
> less abstract than your approach, but the idea is similar.

Ah, yes, sigmatel devices' idea is similar with ours.

>
> It would be nice in theory to move the sigmatel devices over to
> reg_update_bits() in theory, but this seems unlikely to actually happen,
> given that these are mostly obsolete drivers at this point.

I checked all sigmatel devices (about 10 drivers), I think there will
be more work to change them to use syscon/regmap when changing to
reg_update_bits(). I am afraid I can not thelp to convert them without
a hardware, and as you said, some drivers are already dead. So I think
it would be nice if the driver owners can help to convert these
drivers, if we get a consensus about the reg_update_bits()
implementation at last.

-- 
Baolin Wang

Re: [RFC PATCH 0/3] Add new reg_update_bits() support

[Arnd Bergmann]

On Thu, Apr 9, 2020 at 11:40 AM Baolin Wang <baolin.wang7@gmail.com> wrote:

> > It would be nice in theory to move the sigmatel devices over to
> > reg_update_bits() in theory, but this seems unlikely to actually happen,
> > given that these are mostly obsolete drivers at this point.
>
> I checked all sigmatel devices (about 10 drivers), I think there will
> be more work to change them to use syscon/regmap when changing to
> reg_update_bits(). I am afraid I can not thelp to convert them without
> a hardware, and as you said, some drivers are already dead. So I think
> it would be nice if the driver owners can help to convert these
> drivers, if we get a consensus about the reg_update_bits()
> implementation at last.

Thanks for taking a closer look. I didn't want to imply that you should
fix them, just saying that it might have been nice if they had been
done like this in the first place. Of course, when the drivers were
written, we did not even have the regmap-mmio helpers at all.

      Arnd

Re: [RFC PATCH 0/3] Add new reg_update_bits() support

[Baolin Wang]

On Thu, Apr 9, 2020 at 5:52 PM Arnd Bergmann <arnd@arndb.de> wrote:
>
> On Thu, Apr 9, 2020 at 11:40 AM Baolin Wang <baolin.wang7@gmail.com> wrote:
>
> > > It would be nice in theory to move the sigmatel devices over to
> > > reg_update_bits() in theory, but this seems unlikely to actually happen,
> > > given that these are mostly obsolete drivers at this point.
> >
> > I checked all sigmatel devices (about 10 drivers), I think there will
> > be more work to change them to use syscon/regmap when changing to
> > reg_update_bits(). I am afraid I can not thelp to convert them without
> > a hardware, and as you said, some drivers are already dead. So I think
> > it would be nice if the driver owners can help to convert these
> > drivers, if we get a consensus about the reg_update_bits()
> > implementation at last.
>
> Thanks for taking a closer look. I didn't want to imply that you should
> fix them, just saying that it might have been nice if they had been
> done like this in the first place. Of course, when the drivers were
> written, we did not even have the regmap-mmio helpers at all.

Make sense. Thanks.

-- 
Baolin Wang

Re: [RFC PATCH 2/3] regmap: Add reg_update_bits() support

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 537 bytes --]

On Thu, Apr 09, 2020 at 04:57:58PM +0800, Baolin Wang wrote:
> Add reg_update_bits() support in case some platforms use a special method
> to update bits of registers.
> 
> Signed-off-by: Baolin Wang <baolin.wang7@gmail.com>
> ---
>  drivers/base/regmap/regmap-mmio.c | 29 ++++++++++++++++++++++++++++-
>  drivers/base/regmap/regmap.c      |  1 +

MMIO devices clearly don't physically have an update_bits() operation so
this should be implemented further up the stack where it applies to all
buses without physical support.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [RFC PATCH 1/3] mfd: syscon: Add reg_update_bits() callback support

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1136 bytes --]

On Thu, Apr 09, 2020 at 04:57:57PM +0800, Baolin Wang wrote:

> --- a/include/linux/regmap.h
> +++ b/include/linux/regmap.h
> @@ -340,6 +340,8 @@ struct regmap_access_table {
>   *		  read operation on a bus such as SPI, I2C, etc. Most of the
>   *		  devices do not need this.
>   * @reg_write:	  Same as above for writing.
> + * @reg_update_bits: Optional, should only be provided for devices whose update
> + *		     operation cannot be represented as read and write.
>   * @fast_io:	  Register IO is fast. Use a spinlock instead of a mutex
>   *	     	  to perform locking. This field is ignored if custom lock/unlock
>   *	     	  functions are used (see fields lock/unlock of struct regmap_config).
> @@ -416,6 +418,8 @@ struct regmap_config {
>  
>  	int (*reg_read)(void *context, unsigned int reg, unsigned int *val);
>  	int (*reg_write)(void *context, unsigned int reg, unsigned int val);
> +	int (*reg_update_bits)(void *context, unsigned int reg,
> +			       unsigned int mask, unsigned int val);

This is fine, we already have this operation for buses, but why is this
munged in with the MFD patch?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [RFC PATCH 2/3] regmap: Add reg_update_bits() support

[Baolin Wang]

Hi Mark,

On Thu, Apr 9, 2020 at 6:45 PM Mark Brown <broonie@kernel.org> wrote:
>
> On Thu, Apr 09, 2020 at 04:57:58PM +0800, Baolin Wang wrote:
> > Add reg_update_bits() support in case some platforms use a special method
> > to update bits of registers.
> >
> > Signed-off-by: Baolin Wang <baolin.wang7@gmail.com>
> > ---
> >  drivers/base/regmap/regmap-mmio.c | 29 ++++++++++++++++++++++++++++-
> >  drivers/base/regmap/regmap.c      |  1 +
>
> MMIO devices clearly don't physically have an update_bits() operation so
> this should be implemented further up the stack where it applies to all
> buses without physical support.

I understood your concern. But the syscon driver need use the MMIO
devices' resources (such as address mapping, clock management and so
on), if move this to further up stack, I am afraid the update_bits()
can not use the resources in 'struct regmap_mmio_context'. Do you have
any good suggestion? Thanks.

-- 
Baolin Wang

Re: [RFC PATCH 1/3] mfd: syscon: Add reg_update_bits() callback support

[Baolin Wang]

On Thu, Apr 9, 2020 at 6:48 PM Mark Brown <broonie@kernel.org> wrote:
>
> On Thu, Apr 09, 2020 at 04:57:57PM +0800, Baolin Wang wrote:
>
> > --- a/include/linux/regmap.h
> > +++ b/include/linux/regmap.h
> > @@ -340,6 +340,8 @@ struct regmap_access_table {
> >   *             read operation on a bus such as SPI, I2C, etc. Most of the
> >   *             devices do not need this.
> >   * @reg_write:         Same as above for writing.
> > + * @reg_update_bits: Optional, should only be provided for devices whose update
> > + *                operation cannot be represented as read and write.
> >   * @fast_io:   Register IO is fast. Use a spinlock instead of a mutex
> >   *             to perform locking. This field is ignored if custom lock/unlock
> >   *             functions are used (see fields lock/unlock of struct regmap_config).
> > @@ -416,6 +418,8 @@ struct regmap_config {
> >
> >       int (*reg_read)(void *context, unsigned int reg, unsigned int *val);
> >       int (*reg_write)(void *context, unsigned int reg, unsigned int val);
> > +     int (*reg_update_bits)(void *context, unsigned int reg,
> > +                            unsigned int mask, unsigned int val);
>
> This is fine, we already have this operation for buses, but why is this
> munged in with the MFD patch?

Originally I want to show a example usage of the new callback, but I
can spilt them into 2 patches as you suggested. Thanks.

-- 
Baolin Wang

Re: [RFC PATCH 2/3] regmap: Add reg_update_bits() support

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 872 bytes --]

On Thu, Apr 09, 2020 at 10:12:44PM +0800, Baolin Wang wrote:
> On Thu, Apr 9, 2020 at 6:45 PM Mark Brown <broonie@kernel.org> wrote:

> > MMIO devices clearly don't physically have an update_bits() operation so
> > this should be implemented further up the stack where it applies to all
> > buses without physical support.

> I understood your concern. But the syscon driver need use the MMIO
> devices' resources (such as address mapping, clock management and so
> on), if move this to further up stack, I am afraid the update_bits()
> can not use the resources in 'struct regmap_mmio_context'. Do you have
> any good suggestion? Thanks.

If the syscon driver needs to be peering into the regmap implementation
that seems like there's a serious abstraction problem - users of the
regmap shouldn't be looking at the implmentation at all.  Why do you
think this is needed?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [RFC PATCH 1/3] mfd: syscon: Add reg_update_bits() callback support

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 594 bytes --]

On Thu, Apr 09, 2020 at 10:13:58PM +0800, Baolin Wang wrote:
> On Thu, Apr 9, 2020 at 6:48 PM Mark Brown <broonie@kernel.org> wrote:

> > > +     int (*reg_update_bits)(void *context, unsigned int reg,
> > > +                            unsigned int mask, unsigned int val);

> > This is fine, we already have this operation for buses, but why is this
> > munged in with the MFD patch?

> Originally I want to show a example usage of the new callback, but I
> can spilt them into 2 patches as you suggested. Thanks.

Yes, it's good to have an example user - just not as part of the same
patch!

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [RFC PATCH 1/3] mfd: syscon: Add reg_update_bits() callback support

[Baolin Wang]

On Thu, Apr 9, 2020 at 10:27 PM Mark Brown <broonie@kernel.org> wrote:
>
> On Thu, Apr 09, 2020 at 10:13:58PM +0800, Baolin Wang wrote:
> > On Thu, Apr 9, 2020 at 6:48 PM Mark Brown <broonie@kernel.org> wrote:
>
> > > > +     int (*reg_update_bits)(void *context, unsigned int reg,
> > > > +                            unsigned int mask, unsigned int val);
>
> > > This is fine, we already have this operation for buses, but why is this
> > > munged in with the MFD patch?
>
> > Originally I want to show a example usage of the new callback, but I
> > can spilt them into 2 patches as you suggested. Thanks.
>
> Yes, it's good to have an example user - just not as part of the same
> patch!

Yes, make sense.

-- 
Baolin Wang

Re: [RFC PATCH 2/3] regmap: Add reg_update_bits() support

[Baolin Wang]

On Thu, Apr 9, 2020 at 10:26 PM Mark Brown <broonie@kernel.org> wrote:
>
> On Thu, Apr 09, 2020 at 10:12:44PM +0800, Baolin Wang wrote:
> > On Thu, Apr 9, 2020 at 6:45 PM Mark Brown <broonie@kernel.org> wrote:
>
> > > MMIO devices clearly don't physically have an update_bits() operation so
> > > this should be implemented further up the stack where it applies to all
> > > buses without physical support.
>
> > I understood your concern. But the syscon driver need use the MMIO
> > devices' resources (such as address mapping, clock management and so
> > on), if move this to further up stack, I am afraid the update_bits()
> > can not use the resources in 'struct regmap_mmio_context'. Do you have
> > any good suggestion? Thanks.
>
> If the syscon driver needs to be peering into the regmap implementation
> that seems like there's a serious abstraction problem - users of the
> regmap shouldn't be looking at the implmentation at all.  Why do you
> think this is needed?

Sorry for confusing, that's not what I mean. My point is the syscon
driver will call the regmap_init_mmio() to use the MMIO regmap_bus,
but as you said, MMIO bus should not a physical bus, so I suppose the
syscon driver should create a new phycical regmap bus for our special
case, but the problem is the phycical regmap bus implementation will
be similar with the MMIO bus except adding a reg_update_bits()
callback, which will introduce more duplicated code. What do you
think？

-- 
Baolin Wang