linux-arm-kernel.lists.infradead.org archive mirror
 help / color / mirror / Atom feed
* Re: [PATCH v4 1/1] mfd: Add I2C based System Configuaration (SYSCON) access
       [not found] <20200622075145.1464020-1-lee.jones@linaro.org>
@ 2020-06-30  9:16 ` Michael Walle
  2020-06-30 22:32   ` Michael Walle
  2020-07-02  7:14   ` Lee Jones
  2020-07-05 13:10 ` kernel test robot
  1 sibling, 2 replies; 11+ messages in thread
From: Michael Walle @ 2020-06-30  9:16 UTC (permalink / raw)
  To: Lee Jones
  Cc: devicetree, arnd, gregkh, linus.walleij, linux-kernel, robh+dt,
	bgolaszewski, broonie, andriy.shevchenko, linux-arm-kernel

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v4 1/1] mfd: Add I2C based System Configuaration (SYSCON) access
  2020-06-30  9:16 ` [PATCH v4 1/1] mfd: Add I2C based System Configuaration (SYSCON) access Michael Walle
@ 2020-06-30 22:32   ` Michael Walle
  2020-07-01  7:04     ` Lee Jones
  2020-07-02  7:14   ` Lee Jones
  1 sibling, 1 reply; 11+ messages in thread
From: Michael Walle @ 2020-06-30 22:32 UTC (permalink / raw)
  To: Lee Jones
  Cc: devicetree, arnd, gregkh, linus.walleij, linux-kernel, robh+dt,
	bgolaszewski, broonie, andriy.shevchenko, linux-arm-kernel

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v4 1/1] mfd: Add I2C based System Configuaration (SYSCON) access
  2020-06-30 22:32   ` Michael Walle
@ 2020-07-01  7:04     ` Lee Jones
  2020-07-01 21:01       ` Michael Walle
  0 siblings, 1 reply; 11+ messages in thread
From: Lee Jones @ 2020-07-01  7:04 UTC (permalink / raw)
  To: Michael Walle
  Cc: devicetree, arnd, gregkh, linus.walleij, linux-kernel, robh+dt,
	bgolaszewski, broonie, andriy.shevchenko, linux-arm-kernel

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v4 1/1] mfd: Add I2C based System Configuaration (SYSCON) access
  2020-07-01  7:04     ` Lee Jones
@ 2020-07-01 21:01       ` Michael Walle
  2020-07-02  6:54         ` Lee Jones
  0 siblings, 1 reply; 11+ messages in thread
From: Michael Walle @ 2020-07-01 21:01 UTC (permalink / raw)
  To: Lee Jones
  Cc: devicetree, arnd, gregkh, linus.walleij, linux-kernel, robh+dt,
	bgolaszewski, broonie, andriy.shevchenko, linux-arm-kernel

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v4 1/1] mfd: Add I2C based System Configuaration (SYSCON) access
  2020-07-01 21:01       ` Michael Walle
@ 2020-07-02  6:54         ` Lee Jones
  2020-07-02  7:02           ` Michael Walle
  0 siblings, 1 reply; 11+ messages in thread
From: Lee Jones @ 2020-07-02  6:54 UTC (permalink / raw)
  To: Michael Walle
  Cc: devicetree, arnd, gregkh, linus.walleij, linux-kernel, robh+dt,
	bgolaszewski, broonie, andriy.shevchenko, linux-arm-kernel

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v4 1/1] mfd: Add I2C based System Configuaration (SYSCON) access
  2020-07-02  6:54         ` Lee Jones
@ 2020-07-02  7:02           ` Michael Walle
  2020-07-02  8:18             ` Lee Jones
  0 siblings, 1 reply; 11+ messages in thread
From: Michael Walle @ 2020-07-02  7:02 UTC (permalink / raw)
  To: Lee Jones
  Cc: devicetree, arnd, gregkh, linus.walleij, linux-kernel, robh+dt,
	bgolaszewski, broonie, andriy.shevchenko, linux-arm-kernel

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.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v4 1/1] mfd: Add I2C based System Configuaration (SYSCON) access
  2020-06-30  9:16 ` [PATCH v4 1/1] mfd: Add I2C based System Configuaration (SYSCON) access Michael Walle
  2020-06-30 22:32   ` Michael Walle
@ 2020-07-02  7:14   ` Lee Jones
  2020-07-02  7:21     ` Michael Walle
  1 sibling, 1 reply; 11+ messages in thread
From: Lee Jones @ 2020-07-02  7:14 UTC (permalink / raw)
  To: Michael Walle
  Cc: devicetree, arnd, gregkh, linus.walleij, linux-kernel, robh+dt,
	bgolaszewski, broonie, andriy.shevchenko, linux-arm-kernel

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v4 1/1] mfd: Add I2C based System Configuaration (SYSCON) access
  2020-07-02  7:14   ` Lee Jones
@ 2020-07-02  7:21     ` Michael Walle
  0 siblings, 0 replies; 11+ messages in thread
From: Michael Walle @ 2020-07-02  7:21 UTC (permalink / raw)
  To: Lee Jones
  Cc: devicetree, arnd, gregkh, linus.walleij, linux-kernel, robh+dt,
	bgolaszewski, broonie, andriy.shevchenko, linux-arm-kernel

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?

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v4 1/1] mfd: Add I2C based System Configuaration (SYSCON) access
  2020-07-02  7:02           ` Michael Walle
@ 2020-07-02  8:18             ` Lee Jones
  2020-07-02  9:12               ` Michael Walle
  0 siblings, 1 reply; 11+ messages in thread
From: Lee Jones @ 2020-07-02  8:18 UTC (permalink / raw)
  To: Michael Walle, Wolfram Sang
  Cc: devicetree, arnd, gregkh, linus.walleij, linux-kernel, robh+dt,
	bgolaszewski, broonie, andriy.shevchenko, linux-arm-kernel

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v4 1/1] mfd: Add I2C based System Configuaration (SYSCON) access
  2020-07-02  8:18             ` Lee Jones
@ 2020-07-02  9:12               ` Michael Walle
  0 siblings, 0 replies; 11+ messages in thread
From: Michael Walle @ 2020-07-02  9:12 UTC (permalink / raw)
  To: Lee Jones
  Cc: devicetree, arnd, gregkh, linus.walleij, linux-kernel, robh+dt,
	Wolfram Sang, bgolaszewski, broonie, andriy.shevchenko,
	linux-arm-kernel

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v4 1/1] mfd: Add I2C based System Configuaration (SYSCON) access
       [not found] <20200622075145.1464020-1-lee.jones@linaro.org>
  2020-06-30  9:16 ` [PATCH v4 1/1] mfd: Add I2C based System Configuaration (SYSCON) access Michael Walle
@ 2020-07-05 13:10 ` kernel test robot
  1 sibling, 0 replies; 11+ messages in thread
From: kernel test robot @ 2020-07-05 13:10 UTC (permalink / raw)
  To: Lee Jones, michael, robh+dt, broonie, gregkh, andriy.shevchenko,
	devicetree, linus.walleij, bgolaszewski, arnd
  Cc: kbuild-all, linux-arm-kernel

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

Hi Lee,

I love your patch! Yet something to improve:

[auto build test ERROR on ljones-mfd/for-mfd-next]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use  as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Lee-Jones/mfd-Add-I2C-based-System-Configuaration-SYSCON-access/20200622-155310
base:   https://git.kernel.org/pub/scm/linux/kernel/git/lee/mfd.git for-mfd-next
config: riscv-randconfig-r035-20200705 (attached as .config)
compiler: riscv32-linux-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=riscv 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   riscv32-linux-ld: arch/riscv/kernel/sbi.o: in function `sbi_power_off':
   arch/riscv/kernel/sbi.c:544: undefined reference to `sbi_shutdown'
   riscv32-linux-ld: drivers/base/regmap/regmap-i2c.o: in function `regmap_smbus_byte_reg_read':
>> drivers/base/regmap/regmap-i2c.c:25: undefined reference to `i2c_smbus_read_byte_data'
   riscv32-linux-ld: drivers/base/regmap/regmap-i2c.o: in function `regmap_smbus_byte_reg_write':
>> drivers/base/regmap/regmap-i2c.c:43: undefined reference to `i2c_smbus_write_byte_data'
   riscv32-linux-ld: drivers/base/regmap/regmap-i2c.o: in function `regmap_smbus_word_reg_read':
>> drivers/base/regmap/regmap-i2c.c:61: undefined reference to `i2c_smbus_read_word_data'
   riscv32-linux-ld: drivers/base/regmap/regmap-i2c.o: in function `i2c_smbus_read_word_swapped':
>> include/linux/i2c.h:159: undefined reference to `i2c_smbus_read_word_data'
   riscv32-linux-ld: drivers/base/regmap/regmap-i2c.o: in function `i2c_smbus_write_word_swapped':
>> include/linux/i2c.h:168: undefined reference to `i2c_smbus_write_word_data'
   riscv32-linux-ld: drivers/base/regmap/regmap-i2c.o: in function `regmap_smbus_word_reg_write':
>> drivers/base/regmap/regmap-i2c.c:79: undefined reference to `i2c_smbus_write_word_data'
   riscv32-linux-ld: drivers/base/regmap/regmap-i2c.o: in function `regmap_i2c_smbus_i2c_read':
>> drivers/base/regmap/regmap-i2c.c:233: undefined reference to `i2c_smbus_read_i2c_block_data'
   riscv32-linux-ld: drivers/base/regmap/regmap-i2c.o: in function `regmap_i2c_smbus_i2c_write':
>> drivers/base/regmap/regmap-i2c.c:218: undefined reference to `i2c_smbus_write_i2c_block_data'
   riscv32-linux-ld: drivers/base/regmap/regmap-i2c.o: in function `regmap_i2c_read':
>> drivers/base/regmap/regmap-i2c.c:191: undefined reference to `i2c_transfer'
   riscv32-linux-ld: drivers/base/regmap/regmap-i2c.o: in function `i2c_master_send':
>> include/linux/i2c.h:105: undefined reference to `i2c_transfer_buffer_flags'
   riscv32-linux-ld: drivers/base/regmap/regmap-i2c.o: in function `regmap_i2c_gather_write':
   drivers/base/regmap/regmap-i2c.c:161: undefined reference to `i2c_transfer'

vim +159 include/linux/i2c.h

ba98645c7d5464 Wolfram Sang     2017-11-04   93  
8a91732b3b3345 Wolfram Sang     2017-11-04   94  /**
8a91732b3b3345 Wolfram Sang     2017-11-04   95   * i2c_master_send - issue a single I2C message in master transmit mode
8a91732b3b3345 Wolfram Sang     2017-11-04   96   * @client: Handle to slave device
8a91732b3b3345 Wolfram Sang     2017-11-04   97   * @buf: Data that will be written to the slave
8a91732b3b3345 Wolfram Sang     2017-11-04   98   * @count: How many bytes to write, must be less than 64k since msg.len is u16
8a91732b3b3345 Wolfram Sang     2017-11-04   99   *
8a91732b3b3345 Wolfram Sang     2017-11-04  100   * Returns negative errno, or else the number of bytes written.
8a91732b3b3345 Wolfram Sang     2017-11-04  101   */
8a91732b3b3345 Wolfram Sang     2017-11-04  102  static inline int i2c_master_send(const struct i2c_client *client,
8a91732b3b3345 Wolfram Sang     2017-11-04  103  				  const char *buf, int count)
8a91732b3b3345 Wolfram Sang     2017-11-04  104  {
8a91732b3b3345 Wolfram Sang     2017-11-04 @105  	return i2c_transfer_buffer_flags(client, (char *)buf, count, 0);
8a91732b3b3345 Wolfram Sang     2017-11-04  106  };
^1da177e4c3f41 Linus Torvalds   2005-04-16  107  
ba98645c7d5464 Wolfram Sang     2017-11-04  108  /**
ba98645c7d5464 Wolfram Sang     2017-11-04  109   * i2c_master_send_dmasafe - issue a single I2C message in master transmit mode
ba98645c7d5464 Wolfram Sang     2017-11-04  110   *			     using a DMA safe buffer
ba98645c7d5464 Wolfram Sang     2017-11-04  111   * @client: Handle to slave device
ba98645c7d5464 Wolfram Sang     2017-11-04  112   * @buf: Data that will be written to the slave, must be safe to use with DMA
ba98645c7d5464 Wolfram Sang     2017-11-04  113   * @count: How many bytes to write, must be less than 64k since msg.len is u16
ba98645c7d5464 Wolfram Sang     2017-11-04  114   *
ba98645c7d5464 Wolfram Sang     2017-11-04  115   * Returns negative errno, or else the number of bytes written.
ba98645c7d5464 Wolfram Sang     2017-11-04  116   */
ba98645c7d5464 Wolfram Sang     2017-11-04  117  static inline int i2c_master_send_dmasafe(const struct i2c_client *client,
ba98645c7d5464 Wolfram Sang     2017-11-04  118  					  const char *buf, int count)
ba98645c7d5464 Wolfram Sang     2017-11-04  119  {
ba98645c7d5464 Wolfram Sang     2017-11-04  120  	return i2c_transfer_buffer_flags(client, (char *)buf, count,
ba98645c7d5464 Wolfram Sang     2017-11-04  121  					 I2C_M_DMA_SAFE);
ba98645c7d5464 Wolfram Sang     2017-11-04  122  };
ba98645c7d5464 Wolfram Sang     2017-11-04  123  
^1da177e4c3f41 Linus Torvalds   2005-04-16  124  /* Transfer num messages.
^1da177e4c3f41 Linus Torvalds   2005-04-16  125   */
c807da539e8276 Luca Ceresoli    2019-12-04  126  int i2c_transfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num);
b37d2a3a75cb0e Jean Delvare     2012-06-29  127  /* Unlocked flavor */
c807da539e8276 Luca Ceresoli    2019-12-04  128  int __i2c_transfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num);
^1da177e4c3f41 Linus Torvalds   2005-04-16  129  
^1da177e4c3f41 Linus Torvalds   2005-04-16  130  /* This is the very generalized SMBus access routine. You probably do not
^1da177e4c3f41 Linus Torvalds   2005-04-16  131     want to use this, though; one of the functions below may be much easier,
^1da177e4c3f41 Linus Torvalds   2005-04-16  132     and probably just as fast.
^1da177e4c3f41 Linus Torvalds   2005-04-16  133     Note that we use i2c_adapter here, because you do not need a specific
^1da177e4c3f41 Linus Torvalds   2005-04-16  134     smbus adapter to call this function. */
63453b59e41173 Peter Rosin      2018-06-20  135  s32 i2c_smbus_xfer(struct i2c_adapter *adapter, u16 addr,
3ae70deef0a5cc Jean Delvare     2008-10-22  136  		   unsigned short flags, char read_write, u8 command,
63453b59e41173 Peter Rosin      2018-06-20  137  		   int protocol, union i2c_smbus_data *data);
63453b59e41173 Peter Rosin      2018-06-20  138  
63453b59e41173 Peter Rosin      2018-06-20  139  /* Unlocked flavor */
63453b59e41173 Peter Rosin      2018-06-20  140  s32 __i2c_smbus_xfer(struct i2c_adapter *adapter, u16 addr,
63453b59e41173 Peter Rosin      2018-06-20  141  		     unsigned short flags, char read_write, u8 command,
63453b59e41173 Peter Rosin      2018-06-20  142  		     int protocol, union i2c_smbus_data *data);
^1da177e4c3f41 Linus Torvalds   2005-04-16  143  
^1da177e4c3f41 Linus Torvalds   2005-04-16  144  /* Now follow the 'nice' access routines. These also document the calling
ae7193f7fa3e17 Jean Delvare     2008-07-14  145     conventions of i2c_smbus_xfer. */
^1da177e4c3f41 Linus Torvalds   2005-04-16  146  
c807da539e8276 Luca Ceresoli    2019-12-04  147  s32 i2c_smbus_read_byte(const struct i2c_client *client);
c807da539e8276 Luca Ceresoli    2019-12-04  148  s32 i2c_smbus_write_byte(const struct i2c_client *client, u8 value);
c807da539e8276 Luca Ceresoli    2019-12-04  149  s32 i2c_smbus_read_byte_data(const struct i2c_client *client, u8 command);
c807da539e8276 Luca Ceresoli    2019-12-04  150  s32 i2c_smbus_write_byte_data(const struct i2c_client *client,
^1da177e4c3f41 Linus Torvalds   2005-04-16  151  			      u8 command, u8 value);
c807da539e8276 Luca Ceresoli    2019-12-04  152  s32 i2c_smbus_read_word_data(const struct i2c_client *client, u8 command);
c807da539e8276 Luca Ceresoli    2019-12-04  153  s32 i2c_smbus_write_word_data(const struct i2c_client *client,
^1da177e4c3f41 Linus Torvalds   2005-04-16  154  			      u8 command, u16 value);
06a67848c6681a Jonathan Cameron 2011-10-30  155  
06a67848c6681a Jonathan Cameron 2011-10-30  156  static inline s32
06a67848c6681a Jonathan Cameron 2011-10-30  157  i2c_smbus_read_word_swapped(const struct i2c_client *client, u8 command)
06a67848c6681a Jonathan Cameron 2011-10-30  158  {
06a67848c6681a Jonathan Cameron 2011-10-30 @159  	s32 value = i2c_smbus_read_word_data(client, command);
06a67848c6681a Jonathan Cameron 2011-10-30  160  
06a67848c6681a Jonathan Cameron 2011-10-30  161  	return (value < 0) ? value : swab16(value);
06a67848c6681a Jonathan Cameron 2011-10-30  162  }
06a67848c6681a Jonathan Cameron 2011-10-30  163  
06a67848c6681a Jonathan Cameron 2011-10-30  164  static inline s32
06a67848c6681a Jonathan Cameron 2011-10-30  165  i2c_smbus_write_word_swapped(const struct i2c_client *client,
06a67848c6681a Jonathan Cameron 2011-10-30  166  			     u8 command, u16 value)
06a67848c6681a Jonathan Cameron 2011-10-30  167  {
06a67848c6681a Jonathan Cameron 2011-10-30 @168  	return i2c_smbus_write_word_data(client, command, swab16(value));
06a67848c6681a Jonathan Cameron 2011-10-30  169  }
06a67848c6681a Jonathan Cameron 2011-10-30  170  

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 28133 bytes --]

[-- Attachment #3: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2020-07-05 13:13 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
     [not found] <20200622075145.1464020-1-lee.jones@linaro.org>
2020-06-30  9:16 ` [PATCH v4 1/1] mfd: Add I2C based System Configuaration (SYSCON) access Michael Walle
2020-06-30 22:32   ` Michael Walle
2020-07-01  7:04     ` Lee Jones
2020-07-01 21:01       ` Michael Walle
2020-07-02  6:54         ` Lee Jones
2020-07-02  7:02           ` Michael Walle
2020-07-02  8:18             ` Lee Jones
2020-07-02  9:12               ` Michael Walle
2020-07-02  7:14   ` Lee Jones
2020-07-02  7:21     ` Michael Walle
2020-07-05 13:10 ` kernel test robot

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).