diff mbox series

[resend,5/5] media: ov2680: Add camera orientation and sensor rotation controls

Message ID 20240415093704.208175-6-hdegoede@redhat.com (mailing list archive)
State New
Headers show
Series media: ov2680: Add all controls required by libcamera | expand

Commit Message

Hans de Goede April 15, 2024, 9:37 a.m. UTC 
Add camera orientation and sensor rotation controls using
the v4l2_fwnode_device_parse() and v4l2_ctrl_new_fwnode_properties()
helpers.

Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/media/i2c/ov2680.c | 9 +++++++++
 1 file changed, 9 insertions(+)

Comments

Sakari Ailus April 15, 2024, 11:36 a.m. UTC | #1 
Hi Hans,

On Mon, Apr 15, 2024 at 11:37:04AM +0200, Hans de Goede wrote:
> Add camera orientation and sensor rotation controls using
> the v4l2_fwnode_device_parse() and v4l2_ctrl_new_fwnode_properties()
> helpers.
> 
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  drivers/media/i2c/ov2680.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/drivers/media/i2c/ov2680.c b/drivers/media/i2c/ov2680.c
> index 14a5ac2bbf8b..840e6b63ad19 100644
> --- a/drivers/media/i2c/ov2680.c
> +++ b/drivers/media/i2c/ov2680.c
> @@ -946,6 +946,7 @@ static int ov2680_v4l2_register(struct ov2680_dev *sensor)
>  	const struct v4l2_ctrl_ops *ops = &ov2680_ctrl_ops;
>  	struct ov2680_ctrls *ctrls = &sensor->ctrls;
>  	struct v4l2_ctrl_handler *hdl = &ctrls->handler;
> +	struct v4l2_fwnode_device_properties props;
>  	int def, max, ret = 0;
>  
>  	v4l2_i2c_subdev_init(&sensor->sd, client, &ov2680_subdev_ops);
> @@ -1000,6 +1001,14 @@ static int ov2680_v4l2_register(struct ov2680_dev *sensor)
>  		goto cleanup_entity;
>  	}
>  
> +	ret = v4l2_fwnode_device_parse(sensor->dev, &props);
> +	if (ret)
> +		goto cleanup_entity;
> +
> +	ret = v4l2_ctrl_new_fwnode_properties(hdl, ops, &props);
> +	if (ret)
> +		goto cleanup_entity;

As discussed with Umang recently, you can postpone
v4l2_ctrl_new_fwnode_properties() error check until the control handler. Up
to you.

> +
>  	ctrls->vflip->flags |= V4L2_CTRL_FLAG_MODIFY_LAYOUT;
>  	ctrls->hflip->flags |= V4L2_CTRL_FLAG_MODIFY_LAYOUT;
>  	ctrls->link_freq->flags |= V4L2_CTRL_FLAG_READ_ONLY;
Hans de Goede April 15, 2024, 11:58 a.m. UTC | #2 
Hi Sakari,

On 4/15/24 1:36 PM, Sakari Ailus wrote:
> Hi Hans,
> 
> On Mon, Apr 15, 2024 at 11:37:04AM +0200, Hans de Goede wrote:
>> Add camera orientation and sensor rotation controls using
>> the v4l2_fwnode_device_parse() and v4l2_ctrl_new_fwnode_properties()
>> helpers.
>>
>> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>>  drivers/media/i2c/ov2680.c | 9 +++++++++
>>  1 file changed, 9 insertions(+)
>>
>> diff --git a/drivers/media/i2c/ov2680.c b/drivers/media/i2c/ov2680.c
>> index 14a5ac2bbf8b..840e6b63ad19 100644
>> --- a/drivers/media/i2c/ov2680.c
>> +++ b/drivers/media/i2c/ov2680.c
>> @@ -946,6 +946,7 @@ static int ov2680_v4l2_register(struct ov2680_dev *sensor)
>>  	const struct v4l2_ctrl_ops *ops = &ov2680_ctrl_ops;
>>  	struct ov2680_ctrls *ctrls = &sensor->ctrls;
>>  	struct v4l2_ctrl_handler *hdl = &ctrls->handler;
>> +	struct v4l2_fwnode_device_properties props;
>>  	int def, max, ret = 0;
>>  
>>  	v4l2_i2c_subdev_init(&sensor->sd, client, &ov2680_subdev_ops);
>> @@ -1000,6 +1001,14 @@ static int ov2680_v4l2_register(struct ov2680_dev *sensor)
>>  		goto cleanup_entity;
>>  	}
>>  
>> +	ret = v4l2_fwnode_device_parse(sensor->dev, &props);
>> +	if (ret)
>> +		goto cleanup_entity;
>> +
>> +	ret = v4l2_ctrl_new_fwnode_properties(hdl, ops, &props);
>> +	if (ret)
>> +		goto cleanup_entity;
> 
> As discussed with Umang recently, you can postpone
> v4l2_ctrl_new_fwnode_properties() error check until the control handler. Up
> to you.

So you mean move this up to above the 

        if (hdl->error) {
                ret = hdl->error;
                goto cleanup_entity;
        }

check and then drop error handling for the v4l2_ctrl_new_fwnode_properties()
call since any errors there will set hdl->error, right ?

That sounds like a nice cleanup. I'll do that for v2.

Regards,

Hans



> 
>> +
>>  	ctrls->vflip->flags |= V4L2_CTRL_FLAG_MODIFY_LAYOUT;
>>  	ctrls->hflip->flags |= V4L2_CTRL_FLAG_MODIFY_LAYOUT;
>>  	ctrls->link_freq->flags |= V4L2_CTRL_FLAG_READ_ONLY;
>
Sakari Ailus April 15, 2024, 12:17 p.m. UTC | #3 
Hi Hans,

On Mon, Apr 15, 2024 at 01:58:53PM +0200, Hans de Goede wrote:
> Hi Sakari,
> 
> On 4/15/24 1:36 PM, Sakari Ailus wrote:
> > Hi Hans,
> > 
> > On Mon, Apr 15, 2024 at 11:37:04AM +0200, Hans de Goede wrote:
> >> Add camera orientation and sensor rotation controls using
> >> the v4l2_fwnode_device_parse() and v4l2_ctrl_new_fwnode_properties()
> >> helpers.
> >>
> >> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> >> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> >> ---
> >>  drivers/media/i2c/ov2680.c | 9 +++++++++
> >>  1 file changed, 9 insertions(+)
> >>
> >> diff --git a/drivers/media/i2c/ov2680.c b/drivers/media/i2c/ov2680.c
> >> index 14a5ac2bbf8b..840e6b63ad19 100644
> >> --- a/drivers/media/i2c/ov2680.c
> >> +++ b/drivers/media/i2c/ov2680.c
> >> @@ -946,6 +946,7 @@ static int ov2680_v4l2_register(struct ov2680_dev *sensor)
> >>  	const struct v4l2_ctrl_ops *ops = &ov2680_ctrl_ops;
> >>  	struct ov2680_ctrls *ctrls = &sensor->ctrls;
> >>  	struct v4l2_ctrl_handler *hdl = &ctrls->handler;
> >> +	struct v4l2_fwnode_device_properties props;
> >>  	int def, max, ret = 0;
> >>  
> >>  	v4l2_i2c_subdev_init(&sensor->sd, client, &ov2680_subdev_ops);
> >> @@ -1000,6 +1001,14 @@ static int ov2680_v4l2_register(struct ov2680_dev *sensor)
> >>  		goto cleanup_entity;
> >>  	}
> >>  
> >> +	ret = v4l2_fwnode_device_parse(sensor->dev, &props);
> >> +	if (ret)
> >> +		goto cleanup_entity;
> >> +
> >> +	ret = v4l2_ctrl_new_fwnode_properties(hdl, ops, &props);
> >> +	if (ret)
> >> +		goto cleanup_entity;
> > 
> > As discussed with Umang recently, you can postpone
> > v4l2_ctrl_new_fwnode_properties() error check until the control handler. Up
> > to you.
> 
> So you mean move this up to above the 
> 
>         if (hdl->error) {
>                 ret = hdl->error;
>                 goto cleanup_entity;
>         }
> 
> check and then drop error handling for the v4l2_ctrl_new_fwnode_properties()
> call since any errors there will set hdl->error, right ?

Correct.

> 
> That sounds like a nice cleanup. I'll do that for v2.

Thanks!
diff mbox series

Patch

diff --git a/drivers/media/i2c/ov2680.c b/drivers/media/i2c/ov2680.c
index 14a5ac2bbf8b..840e6b63ad19 100644
--- a/drivers/media/i2c/ov2680.c
+++ b/drivers/media/i2c/ov2680.c
@@ -946,6 +946,7 @@  static int ov2680_v4l2_register(struct ov2680_dev *sensor)
 	const struct v4l2_ctrl_ops *ops = &ov2680_ctrl_ops;
 	struct ov2680_ctrls *ctrls = &sensor->ctrls;
 	struct v4l2_ctrl_handler *hdl = &ctrls->handler;
+	struct v4l2_fwnode_device_properties props;
 	int def, max, ret = 0;
 
 	v4l2_i2c_subdev_init(&sensor->sd, client, &ov2680_subdev_ops);
@@ -1000,6 +1001,14 @@  static int ov2680_v4l2_register(struct ov2680_dev *sensor)
 		goto cleanup_entity;
 	}
 
+	ret = v4l2_fwnode_device_parse(sensor->dev, &props);
+	if (ret)
+		goto cleanup_entity;
+
+	ret = v4l2_ctrl_new_fwnode_properties(hdl, ops, &props);
+	if (ret)
+		goto cleanup_entity;
+
 	ctrls->vflip->flags |= V4L2_CTRL_FLAG_MODIFY_LAYOUT;
 	ctrls->hflip->flags |= V4L2_CTRL_FLAG_MODIFY_LAYOUT;
 	ctrls->link_freq->flags |= V4L2_CTRL_FLAG_READ_ONLY;