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

[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.

> 
>>
>>> +		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.

Best regards,
Krzysztof