On Fri, 19 Jun 2020, Chen-Yu Tsai wrote: > On Fri, Jun 19, 2020 at 2:41 PM Lee Jones <lee.jones@xxxxxxxxxx> wrote: > > > > On Fri, 19 Jun 2020, Chen-Yu Tsai wrote: > > > > > 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? > > > > There is no relationship between MFD and Regmap. It is not more > > closely related to Regmap than it is any other public API provided > > within the kernel. *Some* parent drivers initialise one large, > > encompassing Regmap address space and pass it to their children, but > > this isn't suitable in all cases. > > > > > 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. > > > > Remember, not all use-cases are the same. Just because your H/W fits > > well within the current framework, doesn't mean all will. > > > > Initialising in the parent is no problem if the driver is meaningful > > in other ways, but what if that's all the parent driver does? In > > these cases Syscon can be used instead, rendering the driver > > superfluous. Meaning it can (and *should*) then be omitted. > > I'm guessing in your use case there isn't a need for a parent driver, > and you are looking for something like "simple-mfd", but for listing > sub-devices within an I2C slave device? In that case I understand. Bingo! Actually this will be used *with* "simple-mfd". "simple-mfd" will ensure the sub-devices are probed and "syscon" will allow them to share an address space. This is currently possible for MMIO, but not so for register maps located behind an I2C interface. > > > > > 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]. > > > > Not sure I understand the problem. > > > > Could you explain why the current implementation doesn't work for you? > > > > Open coding your own implementation of Syscon is non-optimal. > > For the DWMAC Ethernet controllers, the platform glue almost always has > a register for tuning the delays of the TX and RX clocks. In almost all > later Allwinner chips, this is in a separate area, which we use a syscon > for. However in one hybrid chip, this is located in the clock controller. > We deemed it risky to also have the whole clock controller address range > mapped as a syscon, and so we export a custom regmap. > > The Ethernet driver has to deal with both cases. > > Looking at it again, since syscon still has a platform driver, maybe I > should just use the dev_get_regmap() route for both cases. > > > > 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. > > > > Syscon is just a means to obtain a group of registers either a) > > without a dedicated driver OR b) to share amongst more than 1, > > potentially unrelated, user. So in the case of a) which appears to > > sit well with-in your use-case and expectations, you are correct. > > Whereas in the case of b) you are not. > > > > I hope that helps clarity the situation somewhat. > > The concern was mostly due to the commit message of > > bdb0066df96e mfd: syscon: Decouple syscon interface from platform devices > > which mentions > > there is a need to have a dedicated driver for such system controller > but also share registers with other drivers. The latter is where the > syscon interface is helpful. > > But does not provide any sort of coordination between the dedicated driver > and the syscon. I suppose the intention might have been that the driver > would get a syscon using its own device node. We avoided that but I wonder > if the extra code is worth it or not. Other platforms seem to do ok. What sort of co-ordination do you require beyond what is offered? > Thank you for helping me clear things up. Never a problem. -- Lee Jones [李琼斯] Senior Technical Lead - Developer Services Linaro.org │ Open source software for Arm SoCs Follow Linaro: Facebook | Twitter | Blog
