Re: [PATCH v2 07/14] mfd: zl3073x: Add components versions register defs

[Ivan Vecera <ivecera@redhat.com>]

On 15. 04. 25 2:57 odp., Andrew Lunn wrote:
>> 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.
> 
> As i said, it was just an idea, i had no idea if it was a good idea.
> 
> What is important is that the scope of the locking becomes clear,
> unlike what the first version had. So locking has to be pushed down to
> the lower levels so you lock a single register access, or you lock an
> mailbox access.
> 
> Also, you say this is an MFD partially because GPIOs could be added
> later. I assume that GPIO code would have the same locking issue,
> which suggests the locking should be in the MFD core, not the
> individual drivers stacked on top of it.

To work with GPIO block inside chip you use just simple registers not 
mailboxes. But it does not matter. The approach above exposes for 
individual sub-drivers an API (not direct usage of regmap (all registers 
are exposed by function helpers), helpers to enter/leave "mailbox mode" 
that locks/unlocks mailbox mutex)...

Something like this
{
	...
	rc = zl3073x_read_id(zldev, &id); // locked implicitly
	...
	scoped_guard(zl3073x_mailbox)(zldev) { // enter mailbox 'mode'
		rc = zl3073x_mb_ref_set(zldev, ref_idx);
		rc = zl3073x_mb_ref_op(zldev, REF_OP_READ);
		regmap_wait_poll_timeout(...);
		rc = zl3073x_mb_ref_freq(zldev, &freq);
	} // leave mailbox access 'mode'
	...
	rc = zl3073x_write_blahblah(zldev, value);
}

Thanks,
Ivan