Hi Jai
On Tue, Oct 28, 2025 at 12:57:12PM +0530, Jai Luthra wrote:
> From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>
> Parse device properties and register controls for them using the V4L2
> fwnode properties helpers.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Signed-off-by: Jai Luthra <jai.luthra@ideasonboard.com>
> ---
> drivers/media/i2c/ov5647.c | 10 ++++++++--
> 1 file changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/media/i2c/ov5647.c b/drivers/media/i2c/ov5647.c
> index e193fef4fcedf4661564c032cd7dbd80a9fd30a6..985a8e81529d2f88cb38ccb8c94f8605026a28a9 100644
> --- a/drivers/media/i2c/ov5647.c
> +++ b/drivers/media/i2c/ov5647.c
> @@ -1284,10 +1284,11 @@ static const struct v4l2_ctrl_ops ov5647_ctrl_ops = {
> .s_ctrl = ov5647_s_ctrl,
> };
>
> -static int ov5647_init_controls(struct ov5647 *sensor)
> +static int ov5647_init_controls(struct ov5647 *sensor, struct device *dev)
> {
> struct i2c_client *client = v4l2_get_subdevdata(&sensor->sd);
> int hblank, exposure_max, exposure_def;
> + struct v4l2_fwnode_device_properties props;
Since I have other comments, let me annoying and suggest moving this
up to maintain line length ordering in variables declaration.
>
> v4l2_ctrl_handler_init(&sensor->ctrls, 9);
Should we make this 11 ? With this change we can end up registering 2
additional controls.
>
> @@ -1338,6 +1339,11 @@ static int ov5647_init_controls(struct ov5647 *sensor)
> ARRAY_SIZE(ov5647_test_pattern_menu) - 1,
> 0, 0, ov5647_test_pattern_menu);
>
> + v4l2_fwnode_device_parse(dev, &props);
> +
> + v4l2_ctrl_new_fwnode_properties(&sensor->ctrls, &ov5647_ctrl_ops,
> + &props);
> +
> if (sensor->ctrls.error)
> goto handler_free;
>
> @@ -1420,7 +1426,7 @@ static int ov5647_probe(struct i2c_client *client)
>
> sensor->mode = OV5647_DEFAULT_MODE;
>
> - ret = ov5647_init_controls(sensor);
> + ret = ov5647_init_controls(sensor, dev);
You know, I think we have a bug that went unnoticed..
ov5647_init_controls() retrieves the i2c client with
struct i2c_client *client = v4l2_get_subdevdata(&sensor->sd);
but v4l2_set_subdevdata() is called by v4l2_i2c_subdev_init() which is
called after ov5647_init_controls(). We don't hit a segfault because
client so far as only be used in the error path.
If you move init_controls() after v4l2_i2c_subdev_init() you can
access dev from the i2c_client instead of passing it to the function ?
Thanks
j
> if (ret)
> goto mutex_destroy;
>
>
> --
> 2.51.0
>