Re: [PATCH v2 2/2] leds: tps6131x: add support for Texas Instruments TPS6131X flash LED driver

[Matthias Fend <matthias.fend@emfend.at>]

Hi Krzysztof,

Am 19.03.2025 um 20:28 schrieb Krzysztof Kozlowski:
> On 19/03/2025 17:25, Matthias Fend wrote:
>>>> +
>>>> +	if (reg3 & TPS6131X_REG_3_HPFL)
>>>> +		*fault |= LED_FAULT_SHORT_CIRCUIT;
>>>> +
>>>> +	if (reg3 & TPS6131X_REG_3_SELSTIM_TO)
>>>> +		*fault |= LED_FAULT_TIMEOUT;
>>>> +
>>>> +	if (reg4 & TPS6131X_REG_4_HOTDIE_HI)
>>>> +		*fault |= LED_FAULT_OVER_TEMPERATURE;
>>>> +
>>>> +	if (reg6 & (TPS6131X_REG_6_LEDHOT | TPS6131X_REG_6_LEDWARN))
>>>> +		*fault |= LED_FAULT_LED_OVER_TEMPERATURE;
>>>> +
>>>> +	if (!(reg6 & TPS6131X_REG_6_LEDHDR))
>>>> +		*fault |= LED_FAULT_UNDER_VOLTAGE;
>>>> +
>>>> +	if (reg6 & TPS6131X_REG_6_LEDHOT) {
>>>> +		ret = regmap_update_bits_base(tps6131x->regmap, TPS6131X_REG_6,
>>>> +					      TPS6131X_REG_6_LEDHOT, 0, NULL, false, true);
>>>
>>> And this is not locked?
>>
>> The read modify write operation is protected by regmap. Since this
>> operation does not interact with any other functions, no lock is needed
>> here.
> 
> 
> Following that logic no lock is needed in the first place. Define what
> is the purpose of this lock, not just "hardware access". I assumed you
> want to keep consistent hardware state between multiple updates. If
> that's correct, how did you prevent returning value from reads happening
> in the middle of concurrent update? Or how this update_bits_base is
> prevented from happening while you are in the middle of earlier calls
> which are protected by your lock?
> 
> That's confusing lock, considering also too short comment explaining its
> purpose.

Registers 0, 1, 2, and 3 control parts of the controller that are not 
completely independent of each other.
For some operations, it is therefore necessary to write to the registers 
in a specific order to avoid unwanted side effects.
Therefore, I protected write access to these registers with a lock. The 
RMW sequence in regmap_update_bits_base uses the cached value of the 
register and does not read from the hardware.

Explicit reads to the status registers can be performed at any time. If 
a flag is set, this can be reported.
Since regmap_read_bypassed actually reads from the hardware but doesn't 
update the cache, this isn't a problem either.
Therefore, I don't see any need for a lock here.

My suggestion would be to expand the comment as follows:
/* Hardware access lock for register 0, 1, 2 and 3 */

and add this additional note before it:
/*
  * Registers 0, 1, 2, and 3 control parts of the controller that are 
not completely
  * independent of each other. Since some operations require the 
registers to be written in
  * a specific order to avoid unwanted side effects, they are 
synchronized with a lock.
  */

Do you think that's acceptable?

> 
>>
>>>
>>>> +		if (ret < 0)
>>>> +			return ret;
>>>> +	}
>>>> +
>>>
>>> ...
>>>
>>>> +
>>>> +static int tps6131x_flash_external_strobe_set(struct v4l2_flash *v4l2_flash, bool enable)
>>>> +{
>>>> +	struct led_classdev_flash *fled_cdev = v4l2_flash->fled_cdev;
>>>> +	struct tps6131x *tps6131x = fled_cdev_to_tps6131x(fled_cdev);
>>>> +
>>>> +	guard(mutex)(&tps6131x->lock);
>>>> +
>>>> +	return tps6131x_set_mode(tps6131x, enable ? TPS6131X_MODE_FLASH : TPS6131X_MODE_SHUTDOWN,
>>>> +				 false);
>>>> +}
>>>> +
>>>> +static const struct v4l2_flash_ops tps6131x_v4l2_flash_ops = {
>>>> +	.external_strobe_set = tps6131x_flash_external_strobe_set,
>>>> +};
>>>> +
>>>> +static int tps6131x_v4l2_setup(struct tps6131x *tps6131x)
>>>> +{
>>>> +	struct v4l2_flash_config v4l2_cfg = { 0 };
>>>> +	struct led_flash_setting *intensity = &v4l2_cfg.intensity;
>>>> +
>>>> +	if (!IS_BUILTIN(CONFIG_V4L2_FLASH_LED_CLASS))
>>>
>>> Why builtin? That's a tristate, so I don't get why driver and v4l flash
>>> cannot be modules. You wanted REACHABLE probably... but then it is
>>> anyway discouraged practice leading to runtime debugging. So actually
>>> you want CONFIG_V4L2_FLASH_LED_CLASS || !CONFIG_V4L2_FLASH_LED_CLASS
>>> dependency.
>>
>> Okay, I'll add 'depends on V4L2_FLASH_LED_CLASS ||
>> !V4L2_FLASH_LED_CLASS' to the Kconfig entry and do the check in the
>> driver like this:
> 
> Only this
> 
>>     if (!IS_ENABLED(CONFIG_V4L2_FLASH_LED_CLASS))
>>       return 0;
>>
>> Is this solution okay for you?
> 
> This should should not be needed, because there are v4l2 stubs.

True, it works that way too, you're right, of course.
I was initially tempted by the many 
'IS_ENABLED(CONFIG_V4L2_FLASH_LED_CLASS)' in the other drivers, and then 
by the requested switch to an early return.
I will now remove the remaining early return as well.

Thanks!
  ~Matthias

> 
> Best regards,
> Krzysztof