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@xxxxxxxxxx>
> > > > > > ---
> > > > > > 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.