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

[Andrew Lunn]

On Thu, Apr 10, 2025 at 08:44:43PM +0200, Ivan Vecera wrote:
> 
> 
> On 10. 04. 25 7:41 odp., Andrew Lunn wrote:
> > > > > +	/* Take device lock */
> > > > 
> > > > What is a device lock? Why do you need to comment standard guards/mutexes?
> > > 
> > > Just to inform code reader, this is a section that accesses device registers
> > > that are protected by this zl3073x device lock.
> > 
> > I didn't see a reply to my question about the big picture. Why is the
> > regmap lock not sufficient. Why do you need a device lock.
> > 
> > > 
> > > > > +	scoped_guard(zl3073x, zldev) {
> > > > > +		rc = zl3073x_read_id(zldev, &id);
> > > > > +		if (rc)
> > > > > +			return rc;
> > > > > +		rc = zl3073x_read_revision(zldev, &revision);
> > > > > +		if (rc)
> > > > > +			return rc;
> > > > > +		rc = zl3073x_read_fw_ver(zldev, &fw_ver);
> > > > > +		if (rc)
> > > > > +			return rc;
> > > > > +		rc = zl3073x_read_custom_config_ver(zldev, &cfg_ver);
> > > > > +		if (rc)
> > > > > +			return rc;
> > > > > +	}
> > > > 
> > > > Nothing improved here. Andrew comments are still valid and do not send
> > > > v3 before the discussion is resolved.
> > > 
> > > I'm accessing device registers here and they are protected by the device
> > > lock. I have to take the lock, register access functions expect this by
> > > lockdep_assert.
> > 
> > Because lockdep asserts is not a useful answer. Locks are there to
> > protect you from something. What are you protecting yourself from? If
> > you cannot answer that question, your locking scheme is built on sand
> > and very probably broken.
> > 
> >      Andrew
> 
> Hi Andrew,
> I have described the locking requirements under different message [v1
> 05/28]. Could you please take a look?

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.

	Andrew