Hi,
On 12/16/22 15:02, Andy Shevchenko wrote:
> On Fri, Dec 16, 2022 at 12:30:08PM +0100, Hans de Goede wrote:
>> Extend the call_s_stream() wrapper to enable/disable sensor privacy LEDs
>> for sensors with a privacy LED, rather then having to duplicate this code
>> in all the sensor drivers.
>
> ...
>
>> +static void call_s_stream_update_pled(struct v4l2_subdev *sd, int enable)
>> +{
>> + if (!sd->dev)
>> + return;
>> +
>> + /* Try to get privacy-led once, at first s_stream() */
>> + if (!sd->privacy_led)
>> + sd->privacy_led = led_get(sd->dev, "privacy-led");
>
>> +
>
> Redundant blank line?
I find this more readable with the blank line between the 2 ifs.
>
>> + if (IS_ERR(sd->privacy_led))
>> + return;
>
> I'm not sure I have got the logic right. Let's assume we call it with
> _led == NULL. Then in case of error, we feel it with the error pointer.
> If we call again, we check for NULL, and return error pointer.
>
> So, we won't try the second time. Is it by design? Or should it be
It is by design, there even is a comment which says so:
/* Try to get privacy-led once, at first s_stream() */
Regards,
Hans
>
> struct ... *led;
>
> if (!privacy_led) {
> led = ...
> if (IS_ERR())
> return;
> privacy_led = led;
> }
>
> ?
>
>> +}
>
