Re: [PATCH net-next v4 5/8] mfd: zl3073x: Add functions to work with register mailboxes

[Ivan Vecera <ivecera@redhat.com>]

On 24. 04. 25 6:43 odp., Andrew Lunn wrote:
>> +static int
>> +zl3073x_write_reg(struct zl3073x_dev *zldev, unsigned int reg, const void *val)
>> +{
>> +	unsigned int len;
>> +	u8 buf[6];
>> +	int rc;
>> +
>> +	/* Offset of the last item in the indexed register or offset of
>> +	 * the non-indexed register itself.
>> +	 */
>> +	if (ZL_REG_OFFSET(reg) > ZL_REG_MAX_OFFSET(reg)) {
>> +		dev_err(zldev->dev, "Index of out range for reg 0x%04lx\n",
>> +			ZL_REG_ADDR(reg));
>> +		return -EINVAL;
>> +	}
>> +
>> +	len = ZL_REG_SIZE(reg);
> 
> I suggested you add helpers for zl3073x_write_reg_u8(),
> zl3073x_write_reg_u16(), zl3073x_write_reg_32(), and
> zl3073x_write_reg_48(). The compiler will then do type checking for
> val, ensure what you pass is actually big enough.
> 
> Here you have a void *val. You have no idea how big a value that
> pointer points to, and the compiler is not helping you.

During taking 613cbb91e9ce ("media: Add MIPI CCI register access helper 
functions") approach I found they are using for these functions u64
regardless of register size... Just to accommodate the biggest
possible value. I know about weakness of 'void *' usage but u64 is not
also ideal as the caller is forced to pass always 8 bytes for reading
and forced to reserve 8 bytes for each read value on stack.

> I suggest you add the individual helpers. If you decided to keep the
> register meta data, you can validate the correct helper has been
> called.

Yes, this should be easily implemented, will follow this.

Anyway, still don't know what to do with mailboxes (aka multiple atomic 
register operations). Lee seems to be against the placement of this
code in MFD parent driver.

Each sequence has to be protected by some lock and this lock needs to be
placed in MFD. Yes the routines for MB access can be for example in DPLL
driver but still the locks have to be inside MFD. So they have to be
exposed to sub-devices.

Thanks,
Ivan