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

[Chen-Yu Tsai]

On Thu, Jun 18, 2020 at 6:07 PM Lee Jones <lee.jones@xxxxxxxxxx> wrote:
>
> On Thu, 18 Jun 2020, Arnd Bergmann wrote:
>
> > On Thu, Jun 18, 2020 at 10:03 AM Lee Jones <lee.jones@xxxxxxxxxx> wrote:
> > >
> > > The existing SYSCON implementation only supports MMIO (memory mapped)
> > > accesses, facilitated by Regmap.  This extends support for registers
> > > held behind I2C busses.
> > >
> > > Signed-off-by: Lee Jones <lee.jones@xxxxxxxxxx>
> >
> > The implementation looks fine to me, but can you explain how this is going to
> > be used, and what the advantage is over open-coding the devm_regmap_init_i2c()
> > in each driver that would use this?
>
> Does Regmap let you register/initialise an I2C address more than once?
>
> When I attempt it, I get:
>
> [    0.522988] i2c i2c-0: Failed to register i2c client tmp105 at 0x32 (-16)
> [    0.523341] i2c i2c-0: of_i2c: Failure registering /bus@4000000/motherboard/iofpga@7,00000000/i2c@16000/temp@32
> [    0.523691] i2c i2c-0: Failed to create I2C device for /bus@4000000/motherboard/iofpga@7,00000000/i2c@16000/temp@32
>
> > Is this about using proper locking through the regmap framework for
> > shared i2c clients, or to reduce memory consumption when lots of drivers
> > access the same regmap?
>
> All of those things are valid.
>
> My use-case is regarding MFDs sharing an I2C interfaced address space
> with their children.

Is that an issue with the standard mfd + regmap pattern?

For the AXP20x PMICs, we register the regmap in the parent mfd driver [1],
and store that in dev_data for child drivers to fetch [2]. You could
easily just fetch the regmap with dev_get_regmap() and a pointer to the
parent device.

> > My impression of the existing syscon code is that the main value-add over
> > other ways of doing the same is the syscon_regmap_lookup_by_phandle()
> > interface that gives other drivers a much simpler way of getting the
> > regmap just based on the DT node. Are you planning to add something
> > like that here as well? An ideal driver interface might allow
> > syscon_regmap_lookup_by_phandle() to work for both mmio and i2c
> > based syscons, or additional ones as well, but implementing this would
> > be rather tricky when the i2c core is a loadable module.

The current MMIO syscon is decoupled from the DM, and there is no way
for drivers to export or register a syscon, meaning I have to open code
syscon_regmap_lookup_by_phandle() [3] if I want to only expose certain
registers and not the full address range, or if I want to share the
regmap with the existing driver (for locking purposes), or both [4].
Maybe there's room for improvement here? The same applies to the new
I2C case, and likely any other future syscon variants.

IMHO people are getting it wrong if they have both a syscon and a driver
for the same device.

> I expect the API would be expanded to cover other use-cases.  This is
> a bare bones implementation which has been kept as atomic as possible
> for ease of review.

Regards
ChenYu

[1] https://elixir.bootlin.com/linux/latest/source/drivers/mfd/axp20x-i2c.c#L43
[2] https://elixir.bootlin.com/linux/latest/source/drivers/pinctrl/pinctrl-axp209.c#L433
[3] https://elixir.bootlin.com/linux/latest/source/drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c#L1093
[4] https://elixir.bootlin.com/linux/latest/source/drivers/clk/sunxi-ng/ccu-sun8i-r40.c#L1333