Re: [PATCH 06/23] media: atomisp: gc0310: Add selection API support

[Hans de Goede <hansg@kernel.org>]

Hi Kieran,

On 17-May-25 10:41 PM, Kieran Bingham wrote:
> Quoting Hans de Goede (2025-05-17 12:40:49)
>> Add support for the selection API as expected by libcamera.
>>
>> Note the driver only supports a single fixed resolution and
>> no cropping, so this is a simple read-only implementation.
>>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>>  .../media/atomisp/i2c/atomisp-gc0310.c        | 42 ++++++++++++++++++-
>>  1 file changed, 41 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/staging/media/atomisp/i2c/atomisp-gc0310.c b/drivers/staging/media/atomisp/i2c/atomisp-gc0310.c
>> index 756e56f639b7..7902e732a3ca 100644
>> --- a/drivers/staging/media/atomisp/i2c/atomisp-gc0310.c
>> +++ b/drivers/staging/media/atomisp/i2c/atomisp-gc0310.c
>> @@ -3,7 +3,7 @@
>>   * Support for GalaxyCore GC0310 VGA camera sensor.
>>   *
>>   * Copyright (c) 2013 Intel Corporation. All Rights Reserved.
>> - * Copyright (c) 2023 Hans de Goede <hdegoede@redhat.com>
>> + * Copyright (c) 2023-2025 Hans de Goede <hansg@kernel.org>
>>   */
>>  
>>  #include <linux/delay.h>
>> @@ -352,6 +352,43 @@ static int gc0310_get_fmt(struct v4l2_subdev *sd,
>>         return 0;
>>  }
>>  
>> +static int gc0310_get_selection(struct v4l2_subdev *sd,
>> +                               struct v4l2_subdev_state *state,
>> +                               struct v4l2_subdev_selection *sel)
>> +{
>> +       /* Only the single fixed 656x496 mode is supported, without croping */
>> +       switch (sel->target) {
>> +       case V4L2_SEL_TGT_CROP:
>> +       case V4L2_SEL_TGT_CROP_BOUNDS:
>> +       case V4L2_SEL_TGT_CROP_DEFAULT:
>> +       case V4L2_SEL_TGT_NATIVE_SIZE:
>> +               sel->r.top = 0;
>> +               sel->r.left = 0;
>> +               sel->r.width = GC0310_NATIVE_WIDTH;
>> +               sel->r.height = GC0310_NATIVE_HEIGHT;
>> +               break;
>> +       default:
>> +               return -EINVAL;
>> +       }
>> +
>> +       return 0;
>> +}
>> +
>> +static int gc0310_set_selection(struct v4l2_subdev *sd,
>> +                               struct v4l2_subdev_state *state,
>> +                               struct v4l2_subdev_selection *sel)
>> +{
>> +       if (sel->target != V4L2_SEL_TGT_CROP)
>> +               return -EINVAL;
>> +
>> +       /* Only the single fixed 656x496 mode is supported, without croping */
>> +       sel->r.top = 0;
>> +       sel->r.left = 0;
>> +       sel->r.width = GC0310_NATIVE_WIDTH;
>> +       sel->r.height = GC0310_NATIVE_HEIGHT;
>> +       return 0;
>> +}
>> +
>>  static int gc0310_detect(struct gc0310_device *sensor)
>>  {
>>         struct i2c_client *client = v4l2_get_subdevdata(&sensor->sd);
>> @@ -509,6 +546,8 @@ static const struct v4l2_subdev_pad_ops gc0310_pad_ops = {
>>         .enum_frame_size = gc0310_enum_frame_size,
>>         .get_fmt = gc0310_get_fmt,
>>         .set_fmt = gc0310_set_fmt,
>> +       .get_selection = gc0310_get_selection,
>> +       .set_selection = gc0310_set_selection,
> 
> On other sensors I've worked on, we haven't implemented .set_selection()
> unless it can be changed. I think this could be simplified here? - Just
> the implementation in .get_selection should be enough I think ?
> 
> Saves a few lines when it's not configurable.

Right. I'm working on merging this series now (minus the last
patch), preparing an atomisp pull-request for 6.17, addressing
review comments as I go.

I've gone with using get_selection for set_selection while
merging this.

Regards,

Hans





> 
> In imx283.c we have no implementation of .set_selection; though in
> imx335.c - we simply set .set_selection = imx335_get_selection;
> 
> imx415.c also only sets the .get_selection callback ... so maybe I could
> already simplify imx335 too!
> 
> 
> 
>>         .get_frame_interval = gc0310_get_frame_interval,
>>  };
>>  
>> @@ -671,5 +710,6 @@ static struct i2c_driver gc0310_driver = {
>>  module_i2c_driver(gc0310_driver);
>>  
>>  MODULE_AUTHOR("Lai, Angie <angie.lai@intel.com>");
>> +MODULE_AUTHOR("Hans de Goede <hansg@kernel.org>");
>>  MODULE_DESCRIPTION("A low-level driver for GalaxyCore GC0310 sensors");
>>  MODULE_LICENSE("GPL");
>> -- 
>> 2.49.0
>>
>