Hi,
On 4/10/24 11:07 AM, Sakari Ailus wrote:
> Moi,
>
> On Fri, Apr 05, 2024 at 12:14:19PM +0300, Tomi Valkeinen wrote:
>> Add helper functions to enable and disable the privacy led. This moves
>> the #if from the call site to the privacy led functions, and makes
>> adding privacy led support to v4l2_subdev_enable/disable_streams()
>> cleaner.
>>
>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@xxxxxxxxxxxxxxxx>
>> ---
>> drivers/media/v4l2-core/v4l2-subdev.c | 31 ++++++++++++++++++++++---------
>> 1 file changed, 22 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
>> index 4c6198c48dd6..942c7a644033 100644
>> --- a/drivers/media/v4l2-core/v4l2-subdev.c
>> +++ b/drivers/media/v4l2-core/v4l2-subdev.c
>> @@ -148,6 +148,23 @@ static int subdev_close(struct file *file)
>> }
>> #endif /* CONFIG_VIDEO_V4L2_SUBDEV_API */
>>
>> +static void v4l2_subdev_enable_privacy_led(struct v4l2_subdev *sd)
>> +{
>> +#if IS_REACHABLE(CONFIG_LEDS_CLASS)
>> + if (!IS_ERR_OR_NULL(sd->privacy_led))
>> + led_set_brightness(sd->privacy_led,
>> + sd->privacy_led->max_brightness);
>> +#endif
>> +}
>> +
>> +static void v4l2_subdev_disable_privacy_led(struct v4l2_subdev *sd)
>> +{
>> +#if IS_REACHABLE(CONFIG_LEDS_CLASS)
>> + if (!IS_ERR_OR_NULL(sd->privacy_led))
>> + led_set_brightness(sd->privacy_led, 0);
>> +#endif
>> +}
>> +
>> static inline int check_which(u32 which)
>> {
>> if (which != V4L2_SUBDEV_FORMAT_TRY &&
>> @@ -412,15 +429,11 @@ static int call_s_stream(struct v4l2_subdev *sd, int enable)
>> if (WARN_ON(!!sd->enabled_streams == !!enable))
>> return 0;
>>
>> -#if IS_REACHABLE(CONFIG_LEDS_CLASS)
>> - if (!IS_ERR_OR_NULL(sd->privacy_led)) {
>> - if (enable)
>> - led_set_brightness(sd->privacy_led,
>> - sd->privacy_led->max_brightness);
>> - else
>> - led_set_brightness(sd->privacy_led, 0);
>> - }
>> -#endif
>> + if (enable)
>> + v4l2_subdev_enable_privacy_led(sd);
>> + else
>> + v4l2_subdev_disable_privacy_led(sd);
>> +
>> ret = sd->ops->video->s_stream(sd, enable);
>
> I see that you're only making changes before the s_stream call which also
> reveals a bug here: if streaming on fails, the LED will remain lit. It
> should be fixed before this set.
>
> I cc'd Hans de Goede.
Right that seems like a bug which I introduced. I think it would be
best to move the setting of the LED on/off to after the s_stream()
call in the:
if (!ret)
sd->enabled_streams = enable ? BIT(0) : 0;
block, so that the LED is not touched when s_stream(sd, 1) fails.
This delay enabling the LED slightly on stream start but IMHO
that is not a bit issue.
Regards,
Hans
