Re: [PATCH 06/21] media: atomisp: ov2680: Implement selection support

[Hans de Goede <hdegoede@redhat.com>]

Hi,

On 5/30/23 13:28, Andy Shevchenko wrote:
> On Tue, May 30, 2023 at 1:36 PM Hans de Goede <hdegoede@redhat.com> wrote:
>> On 5/29/23 22:31, Andy Shevchenko wrote:
>>> On Mon, May 29, 2023 at 1:38 PM Hans de Goede <hdegoede@redhat.com> wrote:
> 
> ...
> 
>>>> +       switch (which) {
>>>> +       case V4L2_SUBDEV_FORMAT_TRY:
>>>> +               return v4l2_subdev_get_try_crop(&sensor->sd, state, pad);
>>>> +       case V4L2_SUBDEV_FORMAT_ACTIVE:
>>>> +               return &sensor->mode.crop;
>>>> +       }
>>>> +
>>>> +       return NULL;
>>>
>>> I would move this to default: case.
>>
>> That may cause the reader of the code to think that there are other cases,
>> which there are not. All possible values of enum v4l2_subdev_format_whence
>> are already handled, otherwise the compiler would also complain.
> 
> Why do we care about that?
> What is the common practice in the v4l2 subsystem?
>> The "return NULL" is there to shut up other compiler warnings.
> 
> Can you elaborate (I mean if the default will be present)?

As a human when I'm reading code if there is a default:
present in a switch-case then I expect that to be something which
may actually happen (which is not the case here).

This is important here because if the default can happen then
the function can return NULL and then callers need to check for
NULL (which they currently do not do).

Looking at other v4l2 drivers I think it would be best to
rewrite the function as:

static struct v4l2_rect *
__ov2680_get_pad_crop(struct ov2680_dev *sensor, struct v4l2_subdev_state *state,
                      unsigned int pad, enum v4l2_subdev_format_whence which)
{
        if (which == V4L2_SUBDEV_FORMAT_TRY)
                return v4l2_subdev_get_try_crop(&sensor->sd, state, pad);

        return &sensor->mode.crop;
}

Regards,

Hans