On Tue, Apr 15, 2025 at 12:01:43PM +0200, Ivan Vecera wrote: > On 10. 04. 25 11:54 odp., Andrew Lunn wrote:
... > > So a small number of registers in the regmap need special locking. It > > was not clear to me what exactly those locking requirements are, > > because they don't appear to be described. > > > > But when i look at the code above, the scoped guard gives the > > impression that i have to read id, revision, fw_vr and cfg_ver all in > > one go without any other reads/writes happening. I strongly suspect > > that impression is wrong. The question then becomes, how can i tell > > apart reads/writes which do need to be made as one group, form others > > which can be arbitrarily ordered with other read/writes. > > > > What i suggest you do is try to work out how to push the locking down > > as low as possible. Make the lock cover only what it needs to cover. > > > > Probably for 95% of the registers, the regmap lock is sufficient. > > > > Just throwing out ideas, i've no idea if they are good or not. Create > > two regmaps onto your i2c device, covering different register > > ranges. The 'normal' one uses standard regmap locking, the second > > 'special' one has locking disabled. You additionally provide your own > > lock functions to the 'normal' one, so you have access to the > > lock. When you need to access the mailboxes, take the lock, so you > > know the 'normal' regmap cannot access anything, and then use the > > 'special' regmap to do what you need to do. A structure like this > > should help explain what the special steps are for those special > > registers, while not scattering wrong ideas about what the locking > > scheme actually is all over the code. > > Hi Andrew, > the idea looks interesting but there are some caveats and disadvantages. > I thought about it but the idea with two regmaps (one for simple registers > and one for mailboxes) where the simple one uses implicit locking and > mailbox one has locking disabled with explicit locking requirement. There > are two main problems: > > 1) Regmap cache has to be disabled as it cannot be shared between multiple > regmaps... so also page selector cannot be cached. > > 2) You cannot mix access to mailbox registers and to simple registers. This > means that mailbox accesses have to be wrapped e.g. inside scoped_guard() > > The first problem is really pain as I would like to extend later the driver > with proper caching (page selector for now). > The second one brings only confusions for a developer how to properly access > different types of registers. > > I think the best approach would be to use just single regmap for all > registers with implicit locking enabled and have extra mailbox mutex to > protect mailbox registers and ensure atomic operations with them. > This will allow to use regmap cache and also intermixing mailbox and simple > registers' accesses won't be an issue. > > @Andy Shevchenko, wdym about it? Sounds like a good plan to me, but I'm not in the exact area of this driver's interest, so others may have better suggestions. -- With Best Regards, Andy Shevchenko
