[PATCH v4 1/1] mfd: Add I2C based System Configuaration (SYSCON) access

[PATCH v4 1/1] mfd: Add I2C based System Configuaration (SYSCON) access

[Lee Jones]

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 <lee.jones@linaro.org>
---
Changelog:

v3 => v4
  - Add ability to provide a non-default Regmap configuration

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       | 104 +++++++++++++++++++++++++++++++++
 include/linux/mfd/syscon-i2c.h |  36 ++++++++++++
 4 files changed, 148 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..f6b429dff2ab1
--- /dev/null
+++ b/drivers/mfd/syscon-i2c.c
@@ -0,0 +1,104 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * System Control Driver accessed over I2C
+ *
+ * Copyright (C) 2020 Linaro Ltd.
+ *
+ * Author: Lee Jones <lee.jones@linaro.org>
+ */
+
+#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 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 *regmap_config)
+{
+	struct device *dev = &client->dev;
+	struct syscon *syscon;
+	struct regmap *regmap;
+	int ret;
+
+	if (!regmap_config) {
+		dev_err(dev, "No Regmap configuration supplied\n");
+		return ERR_PTR(-EINVAL);
+	}
+
+	syscon = devm_kzalloc(dev, sizeof(*syscon), GFP_KERNEL);
+	if (!syscon)
+		return ERR_PTR(-ENOMEM);
+
+	if (!regmap_config->name)
+		regmap_config->name = dev_name(dev);
+
+	regmap = devm_regmap_init_i2c(client, regmap_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 regmap_config *regmap_config)
+{
+	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, regmap_config);
+
+	if (IS_ERR(syscon))
+		return ERR_CAST(syscon);
+
+	return syscon->regmap;
+}
+
+struct regmap *syscon_i2c_to_regmap_config(struct i2c_client *client,
+					   struct regmap_config *regmap_config)
+{
+	return syscon_i2c_get_regmap(client, regmap_config);
+}
+EXPORT_SYMBOL_GPL(syscon_i2c_to_regmap_config);
+
+struct regmap *syscon_i2c_to_regmap(struct i2c_client *client)
+{
+	return syscon_i2c_get_regmap(client, &syscon_i2c_regmap_config);
+}
+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..4507572bc4f86
--- /dev/null
+++ b/include/linux/mfd/syscon-i2c.h
@@ -0,0 +1,36 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+/*
+ * System Control Driver accessed via I2C
+ *
+ * Copyright (C) 2020 Linaro Ltd.
+ *
+ * Author: Lee Jones <lee.jones@linaro.org>
+ */
+
+#ifndef __LINUX_MFD_SYSCON_I2C_H__
+#define __LINUX_MFD_SYSCON_I2C_H__
+
+#include <linux/err.h>
+#include <linux/errno.h>
+#include <linux/i2c.h>
+#include <linux/regmap.h>
+
+#if IS_ENABLED(CONFIG_MFD_SYSCON_I2C)
+extern struct regmap *syscon_i2c_to_regmap(struct i2c_client *client);
+extern
+struct regmap *syscon_i2c_to_regmap_config(struct i2c_client *client,
+					   struct regmap_config *regmap_config);
+#else
+static inline struct regmap *syscon_i2c_to_regmap(struct i2c_client *client)
+{
+	return ERR_PTR(-ENOTSUPP);
+}
+static inline
+struct regmap *syscon_i2c_to_regmap_config(struct i2c_client *client,
+					   struct regmap_config *regmap_config);
+{
+	return ERR_PTR(-ENOTSUPP);
+}
+#endif
+
+#endif /* __LINUX_MFD_SYSCON_I2C_H__ */
-- 
2.25.1

Re: [PATCH v4 1/1] mfd: Add I2C based System Configuaration (SYSCON) access

[Michael Walle]

Hi Lee,

I'm just trying to use this for my sl28 driver. Some remarks, see below.

Am 2020-06-22 09:51, schrieb Lee Jones:
> 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 <lee.jones@linaro.org>
> ---
> Changelog:
> 
> v3 => v4
>   - Add ability to provide a non-default Regmap configuration
> 
> 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       | 104 +++++++++++++++++++++++++++++++++
>  include/linux/mfd/syscon-i2c.h |  36 ++++++++++++
>  4 files changed, 148 insertions(+)
>  create mode 100644 drivers/mfd/syscon-i2c.c
>  create mode 100644 include/linux/mfd/syscon-i2c.h
> 

[..]

> +static struct regmap *syscon_i2c_get_regmap(struct i2c_client *client,
> +					    struct regmap_config *regmap_config)
> +{
> +	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, regmap_config);
> +
> +	if (IS_ERR(syscon))
> +		return ERR_CAST(syscon);
> +
> +	return syscon->regmap;
> +}
> +
> +struct regmap *syscon_i2c_to_regmap_config(struct i2c_client *client,
> +					   struct regmap_config *regmap_config)
> +{
> +	return syscon_i2c_get_regmap(client, regmap_config);
> +}
> +EXPORT_SYMBOL_GPL(syscon_i2c_to_regmap_config);
> +
> +struct regmap *syscon_i2c_to_regmap(struct i2c_client *client)
> +{
> +	return syscon_i2c_get_regmap(client, &syscon_i2c_regmap_config);
> +}
> +EXPORT_SYMBOL_GPL(syscon_i2c_to_regmap);

What do you think about

struct regmap *syscon_i2c_to_regmap(struct device *dev)
{
	struct i2c_client *client = i2c_verify_client(dev);

	if (!client)
		return ERR_PTR(-EINVAL);

	return syscon_i2c_get_regmap(client, &syscon_i2c_regmap_config);
}

Or even move it to syscon_i2c_get_regmap().

This way, (a) a driver doesn't have to use "#include <linux/i2c.h>" just
to call to_i2c_client() (or i2c_verify_client()) and (b) you won't do it
all over again in all sub drivers.

So you could just do a
   regmap = syscon_i2c_to_regmap(pdev->dev.parent);

I've also noticed that the mmio syscon uses device_node as parameter. 
What
was the reason to divert from that? Just curious.

-michael

Re: [PATCH v4 1/1] mfd: Add I2C based System Configuaration (SYSCON) access

[Michael Walle]

Hi Lee,

Am 2020-06-30 11:16, schrieb Michael Walle:
> I'm just trying to use this for my sl28 driver. Some remarks, see 
> below.
> 
> Am 2020-06-22 09:51, schrieb Lee Jones:
>> 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 <lee.jones@linaro.org>
>> ---
>> Changelog:
>> 
>> v3 => v4
>>   - Add ability to provide a non-default Regmap configuration
>> 
>> 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       | 104 
>> +++++++++++++++++++++++++++++++++
>>  include/linux/mfd/syscon-i2c.h |  36 ++++++++++++
>>  4 files changed, 148 insertions(+)
>>  create mode 100644 drivers/mfd/syscon-i2c.c
>>  create mode 100644 include/linux/mfd/syscon-i2c.h
>> 
> 
> [..]
> 
>> +static struct regmap *syscon_i2c_get_regmap(struct i2c_client 
>> *client,
>> +					    struct regmap_config *regmap_config)
>> +{
>> +	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, regmap_config);
>> +
>> +	if (IS_ERR(syscon))
>> +		return ERR_CAST(syscon);
>> +
>> +	return syscon->regmap;
>> +}
>> +
>> +struct regmap *syscon_i2c_to_regmap_config(struct i2c_client *client,
>> +					   struct regmap_config *regmap_config)
>> +{
>> +	return syscon_i2c_get_regmap(client, regmap_config);
>> +}
>> +EXPORT_SYMBOL_GPL(syscon_i2c_to_regmap_config);
>> +
>> +struct regmap *syscon_i2c_to_regmap(struct i2c_client *client)
>> +{
>> +	return syscon_i2c_get_regmap(client, &syscon_i2c_regmap_config);
>> +}
>> +EXPORT_SYMBOL_GPL(syscon_i2c_to_regmap);
> 
> What do you think about
> 
> struct regmap *syscon_i2c_to_regmap(struct device *dev)
> {
> 	struct i2c_client *client = i2c_verify_client(dev);
> 
> 	if (!client)
> 		return ERR_PTR(-EINVAL);
> 
> 	return syscon_i2c_get_regmap(client, &syscon_i2c_regmap_config);
> }
> 
> Or even move it to syscon_i2c_get_regmap().
> 
> This way, (a) a driver doesn't have to use "#include <linux/i2c.h>" 
> just
> to call to_i2c_client() (or i2c_verify_client()) and (b) you won't do 
> it
> all over again in all sub drivers.
> 
> So you could just do a
>   regmap = syscon_i2c_to_regmap(pdev->dev.parent);
> 
> I've also noticed that the mmio syscon uses device_node as parameter. 
> What
> was the reason to divert from that? Just curious.

How is this supposed to be used?

I had something like the following in mind:

&i2c {
   cpld@4a {
     compatible = "simple-mfd";
     reg = <0x4a>;

     gpio@4 {
       compatible = "vendor,gpio";
       reg = <0x4>;
     };
   };
};

But I think the childen are not enumerated if its an I2C device. And
the actual i2c driver is also missing.

-michael

Re: [PATCH v4 1/1] mfd: Add I2C based System Configuaration (SYSCON) access

[Lee Jones]

On Wed, 01 Jul 2020, Michael Walle wrote:

> Hi Lee,
> 
> Am 2020-06-30 11:16, schrieb Michael Walle:
> > I'm just trying to use this for my sl28 driver. Some remarks, see below.
> > 
> > Am 2020-06-22 09:51, schrieb Lee Jones:
> > > 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 <lee.jones@linaro.org>
> > > ---
> > > Changelog:
> > > 
> > > v3 => v4
> > >   - Add ability to provide a non-default Regmap configuration
> > > 
> > > 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       | 104
> > > +++++++++++++++++++++++++++++++++
> > >  include/linux/mfd/syscon-i2c.h |  36 ++++++++++++
> > >  4 files changed, 148 insertions(+)
> > >  create mode 100644 drivers/mfd/syscon-i2c.c
> > >  create mode 100644 include/linux/mfd/syscon-i2c.h
> > > 
> > 
> > [..]
> > 
> > > +static struct regmap *syscon_i2c_get_regmap(struct i2c_client
> > > *client,
> > > +					    struct regmap_config *regmap_config)
> > > +{
> > > +	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, regmap_config);
> > > +
> > > +	if (IS_ERR(syscon))
> > > +		return ERR_CAST(syscon);
> > > +
> > > +	return syscon->regmap;
> > > +}
> > > +
> > > +struct regmap *syscon_i2c_to_regmap_config(struct i2c_client *client,
> > > +					   struct regmap_config *regmap_config)
> > > +{
> > > +	return syscon_i2c_get_regmap(client, regmap_config);
> > > +}
> > > +EXPORT_SYMBOL_GPL(syscon_i2c_to_regmap_config);
> > > +
> > > +struct regmap *syscon_i2c_to_regmap(struct i2c_client *client)
> > > +{
> > > +	return syscon_i2c_get_regmap(client, &syscon_i2c_regmap_config);
> > > +}
> > > +EXPORT_SYMBOL_GPL(syscon_i2c_to_regmap);
> > 
> > What do you think about
> > 
> > struct regmap *syscon_i2c_to_regmap(struct device *dev)
> > {
> > 	struct i2c_client *client = i2c_verify_client(dev);
> > 
> > 	if (!client)
> > 		return ERR_PTR(-EINVAL);
> > 
> > 	return syscon_i2c_get_regmap(client, &syscon_i2c_regmap_config);
> > }
> > 
> > Or even move it to syscon_i2c_get_regmap().
> > 
> > This way, (a) a driver doesn't have to use "#include <linux/i2c.h>" just
> > to call to_i2c_client() (or i2c_verify_client()) and (b) you won't do it
> > all over again in all sub drivers.
> > 
> > So you could just do a
> >   regmap = syscon_i2c_to_regmap(pdev->dev.parent);
> > 
> > I've also noticed that the mmio syscon uses device_node as parameter.
> > What
> > was the reason to divert from that? Just curious.
> 
> How is this supposed to be used?
> 
> I had something like the following in mind:
> 
> &i2c {
>   cpld@4a {
>     compatible = "simple-mfd";
>     reg = <0x4a>;
> 
>     gpio@4 {
>       compatible = "vendor,gpio";
>       reg = <0x4>;
>     };
>   };
> };

Yes, that was the idea.

> But I think the childen are not enumerated if its an I2C device. And
> the actual i2c driver is also missing.

What do you mean?  Can you elaborate?

-- 
Lee Jones [李琼斯]
Senior Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH v4 1/1] mfd: Add I2C based System Configuaration (SYSCON) access

[Michael Walle]

Am 2020-07-01 09:04, schrieb Lee Jones:
> On Wed, 01 Jul 2020, Michael Walle wrote:
> 
>> Hi Lee,
>> 
>> Am 2020-06-30 11:16, schrieb Michael Walle:
>> > I'm just trying to use this for my sl28 driver. Some remarks, see below.
>> >
>> > Am 2020-06-22 09:51, schrieb Lee Jones:
>> > > 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 <lee.jones@linaro.org>
>> > > ---
>> > > Changelog:
>> > >
>> > > v3 => v4
>> > >   - Add ability to provide a non-default Regmap configuration
>> > >
>> > > 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       | 104
>> > > +++++++++++++++++++++++++++++++++
>> > >  include/linux/mfd/syscon-i2c.h |  36 ++++++++++++
>> > >  4 files changed, 148 insertions(+)
>> > >  create mode 100644 drivers/mfd/syscon-i2c.c
>> > >  create mode 100644 include/linux/mfd/syscon-i2c.h
>> > >
>> >
>> > [..]
>> >
>> > > +static struct regmap *syscon_i2c_get_regmap(struct i2c_client
>> > > *client,
>> > > +					    struct regmap_config *regmap_config)
>> > > +{
>> > > +	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, regmap_config);
>> > > +
>> > > +	if (IS_ERR(syscon))
>> > > +		return ERR_CAST(syscon);
>> > > +
>> > > +	return syscon->regmap;
>> > > +}
>> > > +
>> > > +struct regmap *syscon_i2c_to_regmap_config(struct i2c_client *client,
>> > > +					   struct regmap_config *regmap_config)
>> > > +{
>> > > +	return syscon_i2c_get_regmap(client, regmap_config);
>> > > +}
>> > > +EXPORT_SYMBOL_GPL(syscon_i2c_to_regmap_config);
>> > > +
>> > > +struct regmap *syscon_i2c_to_regmap(struct i2c_client *client)
>> > > +{
>> > > +	return syscon_i2c_get_regmap(client, &syscon_i2c_regmap_config);
>> > > +}
>> > > +EXPORT_SYMBOL_GPL(syscon_i2c_to_regmap);
>> >
>> > What do you think about
>> >
>> > struct regmap *syscon_i2c_to_regmap(struct device *dev)
>> > {
>> > 	struct i2c_client *client = i2c_verify_client(dev);
>> >
>> > 	if (!client)
>> > 		return ERR_PTR(-EINVAL);
>> >
>> > 	return syscon_i2c_get_regmap(client, &syscon_i2c_regmap_config);
>> > }
>> >
>> > Or even move it to syscon_i2c_get_regmap().
>> >
>> > This way, (a) a driver doesn't have to use "#include <linux/i2c.h>" just
>> > to call to_i2c_client() (or i2c_verify_client()) and (b) you won't do it
>> > all over again in all sub drivers.
>> >
>> > So you could just do a
>> >   regmap = syscon_i2c_to_regmap(pdev->dev.parent);
>> >
>> > I've also noticed that the mmio syscon uses device_node as parameter.
>> > What
>> > was the reason to divert from that? Just curious.
>> 
>> How is this supposed to be used?
>> 
>> I had something like the following in mind:
>> 
>> &i2c {
>>   cpld@4a {
>>     compatible = "simple-mfd";
>>     reg = <0x4a>;
>> 
>>     gpio@4 {
>>       compatible = "vendor,gpio";
>>       reg = <0x4>;
>>     };
>>   };
>> };
> 
> Yes, that was the idea.
> 
>> But I think the childen are not enumerated if its an I2C device. And
>> the actual i2c driver is also missing.
> 
> What do you mean?  Can you elaborate?

There is no i2c_driver instance who would create the regmap. If I'm
reading the I2C code correctly, it won't probe any i2c device of a
bus if there is no i2c_driver with an associated .probe() or
.probe_new(). And even if it is probed, its subnodes won't be
enumerated; the "simple-mfd" code only works for MMIO busses, right?
Or I'm getting something really wrong here..

-michael

Re: [PATCH v4 1/1] mfd: Add I2C based System Configuaration (SYSCON) access

[Lee Jones]

On Wed, 01 Jul 2020, Michael Walle wrote:

> Am 2020-07-01 09:04, schrieb Lee Jones:
> > On Wed, 01 Jul 2020, Michael Walle wrote:
> > 
> > > Hi Lee,
> > > 
> > > Am 2020-06-30 11:16, schrieb Michael Walle:
> > > > I'm just trying to use this for my sl28 driver. Some remarks, see below.
> > > >
> > > > Am 2020-06-22 09:51, schrieb Lee Jones:
> > > > > 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 <lee.jones@linaro.org>
> > > > > ---
> > > > > Changelog:
> > > > >
> > > > > v3 => v4
> > > > >   - Add ability to provide a non-default Regmap configuration
> > > > >
> > > > > 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       | 104
> > > > > +++++++++++++++++++++++++++++++++
> > > > >  include/linux/mfd/syscon-i2c.h |  36 ++++++++++++
> > > > >  4 files changed, 148 insertions(+)
> > > > >  create mode 100644 drivers/mfd/syscon-i2c.c
> > > > >  create mode 100644 include/linux/mfd/syscon-i2c.h

[...]

> > > > This way, (a) a driver doesn't have to use "#include <linux/i2c.h>" just
> > > > to call to_i2c_client() (or i2c_verify_client()) and (b) you won't do it
> > > > all over again in all sub drivers.
> > > >
> > > > So you could just do a
> > > >   regmap = syscon_i2c_to_regmap(pdev->dev.parent);
> > > >
> > > > I've also noticed that the mmio syscon uses device_node as parameter.
> > > > What
> > > > was the reason to divert from that? Just curious.
> > > 
> > > How is this supposed to be used?
> > > 
> > > I had something like the following in mind:
> > > 
> > > &i2c {
> > >   cpld@4a {
> > >     compatible = "simple-mfd";
> > >     reg = <0x4a>;
> > > 
> > >     gpio@4 {
> > >       compatible = "vendor,gpio";
> > >       reg = <0x4>;
> > >     };
> > >   };
> > > };
> > 
> > Yes, that was the idea.
> > 
> > > But I think the childen are not enumerated if its an I2C device. And
> > > the actual i2c driver is also missing.
> > 
> > What do you mean?  Can you elaborate?
> 
> There is no i2c_driver instance who would create the regmap.

The regmap is created by the first caller of:

 syscon_i2c_to_regmap{_config}()

> If I'm
> reading the I2C code correctly, it won't probe any i2c device of a
> bus if there is no i2c_driver with an associated .probe() or
> .probe_new().

Why wouldn't the children be registered using i2c_driver?

> And even if it is probed, its subnodes won't be
> enumerated; the "simple-mfd" code only works for MMIO busses, right?
> Or I'm getting something really wrong here..

Then how are these I2C based devices able to call of_platform_populate()?

 drivers/mfd/gateworks-gsc.c
 drivers/mfd/lochnagar-i2c.c
 drivers/mfd/palmas.c
 drivers/mfd/smsc-ece1099.c
 drivers/mfd/stpmic1.c
 drivers/mfd/twl-core.c

Might require some more research into where your use-case is breaking
down.

-- 
Lee Jones [李琼斯]
Senior Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH v4 1/1] mfd: Add I2C based System Configuaration (SYSCON) access

[Michael Walle]

Am 2020-07-02 08:54, schrieb Lee Jones:
> On Wed, 01 Jul 2020, Michael Walle wrote:
> 
>> Am 2020-07-01 09:04, schrieb Lee Jones:
>> > On Wed, 01 Jul 2020, Michael Walle wrote:
>> >
>> > > Hi Lee,
>> > >
>> > > Am 2020-06-30 11:16, schrieb Michael Walle:
>> > > > I'm just trying to use this for my sl28 driver. Some remarks, see below.
>> > > >
>> > > > Am 2020-06-22 09:51, schrieb Lee Jones:
>> > > > > 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 <lee.jones@linaro.org>
>> > > > > ---
>> > > > > Changelog:
>> > > > >
>> > > > > v3 => v4
>> > > > >   - Add ability to provide a non-default Regmap configuration
>> > > > >
>> > > > > 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       | 104
>> > > > > +++++++++++++++++++++++++++++++++
>> > > > >  include/linux/mfd/syscon-i2c.h |  36 ++++++++++++
>> > > > >  4 files changed, 148 insertions(+)
>> > > > >  create mode 100644 drivers/mfd/syscon-i2c.c
>> > > > >  create mode 100644 include/linux/mfd/syscon-i2c.h
> 
> [...]
> 
>> > > > This way, (a) a driver doesn't have to use "#include <linux/i2c.h>" just
>> > > > to call to_i2c_client() (or i2c_verify_client()) and (b) you won't do it
>> > > > all over again in all sub drivers.
>> > > >
>> > > > So you could just do a
>> > > >   regmap = syscon_i2c_to_regmap(pdev->dev.parent);
>> > > >
>> > > > I've also noticed that the mmio syscon uses device_node as parameter.
>> > > > What
>> > > > was the reason to divert from that? Just curious.
>> > >
>> > > How is this supposed to be used?
>> > >
>> > > I had something like the following in mind:
>> > >
>> > > &i2c {
>> > >   cpld@4a {
>> > >     compatible = "simple-mfd";
>> > >     reg = <0x4a>;
>> > >
>> > >     gpio@4 {
>> > >       compatible = "vendor,gpio";
>> > >       reg = <0x4>;
>> > >     };
>> > >   };
>> > > };
>> >
>> > Yes, that was the idea.
>> >
>> > > But I think the childen are not enumerated if its an I2C device. And
>> > > the actual i2c driver is also missing.
>> >
>> > What do you mean?  Can you elaborate?
>> 
>> There is no i2c_driver instance who would create the regmap.
> 
> The regmap is created by the first caller of:
> 
>  syscon_i2c_to_regmap{_config}()

But which one is an i2c_driver? All the sub devices are platform drivers
and there should be no need for them to know that they are behind an
i2c driver (or spi driver or just mmio). All they have to know is how
to access the registers.

>> If I'm
>> reading the I2C code correctly, it won't probe any i2c device of a
>> bus if there is no i2c_driver with an associated .probe() or
>> .probe_new().
> 
> Why wouldn't the children be registered using i2c_driver?

Where is the code which enumerates the children?

>> And even if it is probed, its subnodes won't be
>> enumerated; the "simple-mfd" code only works for MMIO busses, right?
>> Or I'm getting something really wrong here..
> 
> Then how are these I2C based devices able to call 
> of_platform_populate()?

I don't mean they can't do it, I mean, I'm calling 
of_platform_populate()
myself. But who is actually calling it when one uses the this patch
series?

-michael

>  drivers/mfd/gateworks-gsc.c
>  drivers/mfd/lochnagar-i2c.c
>  drivers/mfd/palmas.c
>  drivers/mfd/smsc-ece1099.c
>  drivers/mfd/stpmic1.c
>  drivers/mfd/twl-core.c
> 
> Might require some more research into where your use-case is breaking
> down.

Re: [PATCH v4 1/1] mfd: Add I2C based System Configuaration (SYSCON) access

[Lee Jones]

On Tue, 30 Jun 2020, Michael Walle wrote:

> Hi Lee,
> 
> I'm just trying to use this for my sl28 driver. Some remarks, see below.
> 
> Am 2020-06-22 09:51, schrieb Lee Jones:
> > 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 <lee.jones@linaro.org>
> > ---
> > Changelog:
> > 
> > v3 => v4
> >   - Add ability to provide a non-default Regmap configuration
> > 
> > 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       | 104 +++++++++++++++++++++++++++++++++
> >  include/linux/mfd/syscon-i2c.h |  36 ++++++++++++
> >  4 files changed, 148 insertions(+)
> >  create mode 100644 drivers/mfd/syscon-i2c.c
> >  create mode 100644 include/linux/mfd/syscon-i2c.h
> > 
> 
> [..]
> 
> > +static struct regmap *syscon_i2c_get_regmap(struct i2c_client *client,
> > +					    struct regmap_config *regmap_config)
> > +{
> > +	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, regmap_config);
> > +
> > +	if (IS_ERR(syscon))
> > +		return ERR_CAST(syscon);
> > +
> > +	return syscon->regmap;
> > +}
> > +
> > +struct regmap *syscon_i2c_to_regmap_config(struct i2c_client *client,
> > +					   struct regmap_config *regmap_config)
> > +{
> > +	return syscon_i2c_get_regmap(client, regmap_config);
> > +}
> > +EXPORT_SYMBOL_GPL(syscon_i2c_to_regmap_config);
> > +
> > +struct regmap *syscon_i2c_to_regmap(struct i2c_client *client)
> > +{
> > +	return syscon_i2c_get_regmap(client, &syscon_i2c_regmap_config);
> > +}
> > +EXPORT_SYMBOL_GPL(syscon_i2c_to_regmap);
> 
> What do you think about
> 
> struct regmap *syscon_i2c_to_regmap(struct device *dev)
> {
> 	struct i2c_client *client = i2c_verify_client(dev);
> 
> 	if (!client)
> 		return ERR_PTR(-EINVAL);
> 
> 	return syscon_i2c_get_regmap(client, &syscon_i2c_regmap_config);
> }
> 
> Or even move it to syscon_i2c_get_regmap().
> 
> This way, (a) a driver doesn't have to use "#include <linux/i2c.h>" just
> to call to_i2c_client() (or i2c_verify_client()) and (b) you won't do it
> all over again in all sub drivers.

What is your use-case?  This is set-up for based I2C drivers to call
into.  'client' is given to them as their .probe() arg.

> So you could just do a
>   regmap = syscon_i2c_to_regmap(pdev->dev.parent);
> 
> I've also noticed that the mmio syscon uses device_node as parameter. What
> was the reason to divert from that? Just curious.

This is a helper for I2C clients.  There aren't any OF helpers in here
(yet).  If you think they would be helpful we can add them.  How do
you see them being used?

-- 
Lee Jones [李琼斯]
Senior Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH v4 1/1] mfd: Add I2C based System Configuaration (SYSCON) access

[Michael Walle]

Am 2020-07-02 09:14, schrieb Lee Jones:
> On Tue, 30 Jun 2020, Michael Walle wrote:
> 
>> Hi Lee,
>> 
>> I'm just trying to use this for my sl28 driver. Some remarks, see 
>> below.
>> 
>> Am 2020-06-22 09:51, schrieb Lee Jones:
>> > 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 <lee.jones@linaro.org>
>> > ---
>> > Changelog:
>> >
>> > v3 => v4
>> >   - Add ability to provide a non-default Regmap configuration
>> >
>> > 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       | 104 +++++++++++++++++++++++++++++++++
>> >  include/linux/mfd/syscon-i2c.h |  36 ++++++++++++
>> >  4 files changed, 148 insertions(+)
>> >  create mode 100644 drivers/mfd/syscon-i2c.c
>> >  create mode 100644 include/linux/mfd/syscon-i2c.h
>> >
>> 
>> [..]
>> 
>> > +static struct regmap *syscon_i2c_get_regmap(struct i2c_client *client,
>> > +					    struct regmap_config *regmap_config)
>> > +{
>> > +	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, regmap_config);
>> > +
>> > +	if (IS_ERR(syscon))
>> > +		return ERR_CAST(syscon);
>> > +
>> > +	return syscon->regmap;
>> > +}
>> > +
>> > +struct regmap *syscon_i2c_to_regmap_config(struct i2c_client *client,
>> > +					   struct regmap_config *regmap_config)
>> > +{
>> > +	return syscon_i2c_get_regmap(client, regmap_config);
>> > +}
>> > +EXPORT_SYMBOL_GPL(syscon_i2c_to_regmap_config);
>> > +
>> > +struct regmap *syscon_i2c_to_regmap(struct i2c_client *client)
>> > +{
>> > +	return syscon_i2c_get_regmap(client, &syscon_i2c_regmap_config);
>> > +}
>> > +EXPORT_SYMBOL_GPL(syscon_i2c_to_regmap);
>> 
>> What do you think about
>> 
>> struct regmap *syscon_i2c_to_regmap(struct device *dev)
>> {
>> 	struct i2c_client *client = i2c_verify_client(dev);
>> 
>> 	if (!client)
>> 		return ERR_PTR(-EINVAL);
>> 
>> 	return syscon_i2c_get_regmap(client, &syscon_i2c_regmap_config);
>> }
>> 
>> Or even move it to syscon_i2c_get_regmap().
>> 
>> This way, (a) a driver doesn't have to use "#include <linux/i2c.h>" 
>> just
>> to call to_i2c_client() (or i2c_verify_client()) and (b) you won't do 
>> it
>> all over again in all sub drivers.
> 
> What is your use-case?

Still my sl28 mfd driver. There the sub devices just need a regmap.

>  This is set-up for based I2C drivers to call
> into.  'client' is given to them as their .probe() arg.

Ok, I see. Then this doesn't fit.

-michael

>> So you could just do a
>>   regmap = syscon_i2c_to_regmap(pdev->dev.parent);
>> 
>> I've also noticed that the mmio syscon uses device_node as parameter. 
>> What
>> was the reason to divert from that? Just curious.
> 
> This is a helper for I2C clients.  There aren't any OF helpers in here
> (yet).  If you think they would be helpful we can add them.  How do
> you see them being used?

Re: [PATCH v4 1/1] mfd: Add I2C based System Configuaration (SYSCON) access

[Lee Jones]

On Thu, 02 Jul 2020, Michael Walle wrote:

> Am 2020-07-02 08:54, schrieb Lee Jones:
> > On Wed, 01 Jul 2020, Michael Walle wrote:
> > 
> > > Am 2020-07-01 09:04, schrieb Lee Jones:
> > > > On Wed, 01 Jul 2020, Michael Walle wrote:
> > > >
> > > > > Hi Lee,
> > > > >
> > > > > Am 2020-06-30 11:16, schrieb Michael Walle:
> > > > > > I'm just trying to use this for my sl28 driver. Some remarks, see below.
> > > > > >
> > > > > > Am 2020-06-22 09:51, schrieb Lee Jones:
> > > > > > > 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 <lee.jones@linaro.org>
> > > > > > > ---
> > > > > > > Changelog:
> > > > > > >
> > > > > > > v3 => v4
> > > > > > >   - Add ability to provide a non-default Regmap configuration
> > > > > > >
> > > > > > > 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       | 104
> > > > > > > +++++++++++++++++++++++++++++++++
> > > > > > >  include/linux/mfd/syscon-i2c.h |  36 ++++++++++++
> > > > > > >  4 files changed, 148 insertions(+)
> > > > > > >  create mode 100644 drivers/mfd/syscon-i2c.c
> > > > > > >  create mode 100644 include/linux/mfd/syscon-i2c.h
> > 
> > [...]
> > 
> > > > > > This way, (a) a driver doesn't have to use "#include <linux/i2c.h>" just
> > > > > > to call to_i2c_client() (or i2c_verify_client()) and (b) you won't do it
> > > > > > all over again in all sub drivers.
> > > > > >
> > > > > > So you could just do a
> > > > > >   regmap = syscon_i2c_to_regmap(pdev->dev.parent);
> > > > > >
> > > > > > I've also noticed that the mmio syscon uses device_node as parameter.
> > > > > > What
> > > > > > was the reason to divert from that? Just curious.
> > > > >
> > > > > How is this supposed to be used?
> > > > >
> > > > > I had something like the following in mind:
> > > > >
> > > > > &i2c {
> > > > >   cpld@4a {
> > > > >     compatible = "simple-mfd";
> > > > >     reg = <0x4a>;
> > > > >
> > > > >     gpio@4 {
> > > > >       compatible = "vendor,gpio";
> > > > >       reg = <0x4>;
> > > > >     };
> > > > >   };
> > > > > };
> > > >
> > > > Yes, that was the idea.
> > > >
> > > > > But I think the childen are not enumerated if its an I2C device. And
> > > > > the actual i2c driver is also missing.
> > > >
> > > > What do you mean?  Can you elaborate?
> > > 
> > > There is no i2c_driver instance who would create the regmap.
> > 
> > The regmap is created by the first caller of:
> > 
> >  syscon_i2c_to_regmap{_config}()
> 
> But which one is an i2c_driver? All the sub devices are platform drivers
> and there should be no need for them to know that they are behind an
> i2c driver (or spi driver or just mmio). All they have to know is how
> to access the registers.
> 
> > > If I'm
> > > reading the I2C code correctly, it won't probe any i2c device of a
> > > bus if there is no i2c_driver with an associated .probe() or
> > > .probe_new().
> > 
> > Why wouldn't the children be registered using i2c_driver?
> 
> Where is the code which enumerates the children?

Yes, I see the problem now.  So I2C devices depend on a device
registering with the i2c_driver framework, which then probes the
device accordingly.  Thus a physical driver is required to convert I2C
devices to platform devices.  So this stops being an MFD problem and
starts being a 'simple-i2c' issue. :)

(not a genuine suggestion by the way)

Wolfram, do we have this correct?

-- 
Lee Jones [李琼斯]
Senior Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH v4 1/1] mfd: Add I2C based System Configuaration (SYSCON) access

[Michael Walle]

Am 2020-07-02 10:18, schrieb Lee Jones:
> On Thu, 02 Jul 2020, Michael Walle wrote:
> 
>> Am 2020-07-02 08:54, schrieb Lee Jones:
>> > On Wed, 01 Jul 2020, Michael Walle wrote:
>> >
>> > > Am 2020-07-01 09:04, schrieb Lee Jones:
>> > > > On Wed, 01 Jul 2020, Michael Walle wrote:
>> > > >
>> > > > > Hi Lee,
>> > > > >
>> > > > > Am 2020-06-30 11:16, schrieb Michael Walle:
>> > > > > > I'm just trying to use this for my sl28 driver. Some remarks, see below.
>> > > > > >
>> > > > > > Am 2020-06-22 09:51, schrieb Lee Jones:
>> > > > > > > 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 <lee.jones@linaro.org>
>> > > > > > > ---
>> > > > > > > Changelog:
>> > > > > > >
>> > > > > > > v3 => v4
>> > > > > > >   - Add ability to provide a non-default Regmap configuration
>> > > > > > >
>> > > > > > > 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       | 104
>> > > > > > > +++++++++++++++++++++++++++++++++
>> > > > > > >  include/linux/mfd/syscon-i2c.h |  36 ++++++++++++
>> > > > > > >  4 files changed, 148 insertions(+)
>> > > > > > >  create mode 100644 drivers/mfd/syscon-i2c.c
>> > > > > > >  create mode 100644 include/linux/mfd/syscon-i2c.h
>> >
>> > [...]
>> >
>> > > > > > This way, (a) a driver doesn't have to use "#include <linux/i2c.h>" just
>> > > > > > to call to_i2c_client() (or i2c_verify_client()) and (b) you won't do it
>> > > > > > all over again in all sub drivers.
>> > > > > >
>> > > > > > So you could just do a
>> > > > > >   regmap = syscon_i2c_to_regmap(pdev->dev.parent);
>> > > > > >
>> > > > > > I've also noticed that the mmio syscon uses device_node as parameter.
>> > > > > > What
>> > > > > > was the reason to divert from that? Just curious.
>> > > > >
>> > > > > How is this supposed to be used?
>> > > > >
>> > > > > I had something like the following in mind:
>> > > > >
>> > > > > &i2c {
>> > > > >   cpld@4a {
>> > > > >     compatible = "simple-mfd";
>> > > > >     reg = <0x4a>;
>> > > > >
>> > > > >     gpio@4 {
>> > > > >       compatible = "vendor,gpio";
>> > > > >       reg = <0x4>;
>> > > > >     };
>> > > > >   };
>> > > > > };
>> > > >
>> > > > Yes, that was the idea.
>> > > >
>> > > > > But I think the childen are not enumerated if its an I2C device. And
>> > > > > the actual i2c driver is also missing.
>> > > >
>> > > > What do you mean?  Can you elaborate?
>> > >
>> > > There is no i2c_driver instance who would create the regmap.
>> >
>> > The regmap is created by the first caller of:
>> >
>> >  syscon_i2c_to_regmap{_config}()
>> 
>> But which one is an i2c_driver? All the sub devices are platform 
>> drivers
>> and there should be no need for them to know that they are behind an
>> i2c driver (or spi driver or just mmio). All they have to know is how
>> to access the registers.
>> 
>> > > If I'm
>> > > reading the I2C code correctly, it won't probe any i2c device of a
>> > > bus if there is no i2c_driver with an associated .probe() or
>> > > .probe_new().
>> >
>> > Why wouldn't the children be registered using i2c_driver?
>> 
>> Where is the code which enumerates the children?
> 
> Yes, I see the problem now.  So I2C devices depend on a device
> registering with the i2c_driver framework, which then probes the
> device accordingly.  Thus a physical driver is required to convert I2C
> devices to platform devices.  So this stops being an MFD problem and
> starts being a 'simple-i2c' issue. :)

Yes, but this is still MFD specifc, because no other normal (in lack of
a better word, think of one-function-devices) I2C device has sub-nodes.
Thus my proposal for that simple-mfd-i2c driver. And keep in mind that
this likely isn't specific to I2C but also to SPI (without having
looked at it). So in theory that simple-mfd-i2c, could also register
a spi_driver which then registers an SPI regmap and enumerates the
children. So this is not only an I2C topic, but IMHO still MFD.

-michael