Message-ID: <20180116140241.GA1943@w540>
Date:   Tue, 16 Jan 2018 15:02:41 +0100
From:   jacopo mondi <jacopo@...ndi.org>
To:     Hans Verkuil <hverkuil@...all.nl>
Cc:     Jacopo Mondi <jacopo+renesas@...ndi.org>,
        laurent.pinchart@...asonboard.com, magnus.damm@...il.com,
        geert@...der.be, mchehab@...nel.org, festevam@...il.com,
        sakari.ailus@....fi, robh+dt@...nel.org, mark.rutland@....com,
        pombredanne@...b.com, linux-renesas-soc@...r.kernel.org,
        linux-media@...r.kernel.org, linux-sh@...r.kernel.org,
        devicetree@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v5 6/9] media: i2c: ov772x: Remove soc_camera dependencies

Hi Hans,

On Tue, Jan 16, 2018 at 11:08:17AM +0100, Hans Verkuil wrote:
> On 01/12/2018 03:04 PM, Jacopo Mondi wrote:
> > Remove soc_camera framework dependencies from ov772x sensor driver.
> > - Handle clock and gpios
> > - Register async subdevice
> > - Remove soc_camera specific g/s_mbus_config operations
> > - Change image format colorspace from JPEG to SRGB as the two use the
> >   same colorspace information but JPEG makes assumptions on color
> >   components quantization that do not apply to the sensor
> > - Remove sizes crop from get_selection as driver can't scale
> > - Add kernel doc to driver interface header file
> > - Adjust build system
> >
> > This commit does not remove the original soc_camera based driver as long
> > as other platforms depends on soc_camera-based CEU driver.
> >
> > Signed-off-by: Jacopo Mondi <jacopo+renesas@...ndi.org>
> > Reviewed-by: Laurent Pinchart <laurent.pinchart@...asonboard.com>

[snip]

> >  static const struct ov772x_win_size *ov772x_select_win(u32 width, u32 height)
> > @@ -855,24 +910,21 @@ static int ov772x_get_selection(struct v4l2_subdev *sd,
> >  		struct v4l2_subdev_pad_config *cfg,
> >  		struct v4l2_subdev_selection *sel)
> >  {
> > +	struct ov772x_priv *priv = to_ov772x(sd);
> > +
> >  	if (sel->which != V4L2_SUBDEV_FORMAT_ACTIVE)
> >  		return -EINVAL;
> >
> > -	sel->r.left = 0;
> > -	sel->r.top = 0;
>
> Why are these two lines removed?
>
> >  	switch (sel->target) {
> >  	case V4L2_SEL_TGT_CROP_BOUNDS:
> >  	case V4L2_SEL_TGT_CROP_DEFAULT:
> > -		sel->r.width = OV772X_MAX_WIDTH;
> > -		sel->r.height = OV772X_MAX_HEIGHT;
> > -		return 0;
> >  	case V4L2_SEL_TGT_CROP:
> > -		sel->r.width = VGA_WIDTH;
> > -		sel->r.height = VGA_HEIGHT;
> > -		return 0;
> > -	default:
> > -		return -EINVAL;
>
> Why is this default case removed?
>

Ooops. I have badly addressed your comment on v1.

Will fix in v6.

> > +		sel->r.width = priv->win->rect.width;
> > +		sel->r.height = priv->win->rect.height;
> > +		break;
> >  	}
> > +
> > +	return 0;
> >  }
> >
> >  static int ov772x_get_fmt(struct v4l2_subdev *sd,
> > @@ -997,24 +1049,8 @@ static int ov772x_enum_mbus_code(struct v4l2_subdev *sd,
> >  	return 0;
> >  }
> >
> > -static int ov772x_g_mbus_config(struct v4l2_subdev *sd,
> > -				struct v4l2_mbus_config *cfg)
> > -{
> > -	struct i2c_client *client = v4l2_get_subdevdata(sd);
> > -	struct soc_camera_subdev_desc *ssdd = soc_camera_i2c_to_desc(client);
> > -
> > -	cfg->flags = V4L2_MBUS_PCLK_SAMPLE_RISING | V4L2_MBUS_MASTER |
> > -		V4L2_MBUS_VSYNC_ACTIVE_HIGH | V4L2_MBUS_HSYNC_ACTIVE_HIGH |
> > -		V4L2_MBUS_DATA_ACTIVE_HIGH;
> > -	cfg->type = V4L2_MBUS_PARALLEL;
> > -	cfg->flags = soc_camera_apply_board_flags(ssdd, cfg);
> > -
> > -	return 0;
> > -}
> > -
> >  static const struct v4l2_subdev_video_ops ov772x_subdev_video_ops = {
> >  	.s_stream	= ov772x_s_stream,
> > -	.g_mbus_config	= ov772x_g_mbus_config,
> >  };
> >
> >  static const struct v4l2_subdev_pad_ops ov772x_subdev_pad_ops = {
> > @@ -1038,12 +1074,11 @@ static int ov772x_probe(struct i2c_client *client,
> >  			const struct i2c_device_id *did)
> >  {
> >  	struct ov772x_priv	*priv;
> > -	struct soc_camera_subdev_desc *ssdd = soc_camera_i2c_to_desc(client);
> > -	struct i2c_adapter	*adapter = to_i2c_adapter(client->dev.parent);
> > +	struct i2c_adapter	*adapter = client->adapter;
> >  	int			ret;
> >
> > -	if (!ssdd || !ssdd->drv_priv) {
> > -		dev_err(&client->dev, "OV772X: missing platform data!\n");
> > +	if (!client->dev.platform_data) {
> > +		dev_err(&client->dev, "Missing OV7725 platform data\n");
>
> Nitpick: I'd prefer lowercase in this string: ov7725. It also should be
> ov772x.
>
> >  		return -EINVAL;
> >  	}
> >
> > @@ -1059,7 +1094,7 @@ static int ov772x_probe(struct i2c_client *client,
> >  	if (!priv)
> >  		return -ENOMEM;
> >
> > -	priv->info = ssdd->drv_priv;
> > +	priv->info = client->dev.platform_data;
> >
> >  	v4l2_i2c_subdev_init(&priv->subdev, client, &ov772x_subdev_ops);
> >  	v4l2_ctrl_handler_init(&priv->hdl, 3);
> > @@ -1073,22 +1108,42 @@ static int ov772x_probe(struct i2c_client *client,
> >  	if (priv->hdl.error)
> >  		return priv->hdl.error;
> >
> > -	priv->clk = v4l2_clk_get(&client->dev, "mclk");
> > +	priv->clk = clk_get(&client->dev, "xclk");
> >  	if (IS_ERR(priv->clk)) {
> > +		dev_err(&client->dev, "Unable to get xclk clock\n");
> >  		ret = PTR_ERR(priv->clk);
> > -		goto eclkget;
> > +		goto error_ctrl_free;
> >  	}
> >
> > -	ret = ov772x_video_probe(priv);
> > -	if (ret < 0) {
> > -		v4l2_clk_put(priv->clk);
> > -eclkget:
> > -		v4l2_ctrl_handler_free(&priv->hdl);
> > -	} else {
> > -		priv->cfmt = &ov772x_cfmts[0];
> > -		priv->win = &ov772x_win_sizes[0];
> > +	priv->pwdn_gpio = gpiod_get_optional(&client->dev, "pwdn",
> > +					     GPIOD_OUT_LOW);
> > +	if (IS_ERR(priv->pwdn_gpio)) {
> > +		dev_info(&client->dev, "Unable to get GPIO \"pwdn\"");
> > +		ret = PTR_ERR(priv->pwdn_gpio);
> > +		goto error_clk_put;
> >  	}
> >
> > +	ret = ov772x_video_probe(priv);
> > +	if (ret < 0)
> > +		goto error_gpio_put;
> > +
> > +	priv->cfmt = &ov772x_cfmts[0];
> > +	priv->win = &ov772x_win_sizes[0];
> > +
> > +	ret = v4l2_async_register_subdev(&priv->subdev);
> > +	if (ret)
> > +		goto error_gpio_put;
> > +
> > +	return 0;
> > +
> > +error_gpio_put:
> > +	if (priv->pwdn_gpio)
> > +		gpiod_put(priv->pwdn_gpio);
> > +error_clk_put:
> > +	clk_put(priv->clk);
> > +error_ctrl_free:
> > +	v4l2_ctrl_handler_free(&priv->hdl);
> > +
> >  	return ret;
> >  }
> >
> > @@ -1096,7 +1151,9 @@ static int ov772x_remove(struct i2c_client *client)
> >  {
> >  	struct ov772x_priv *priv = to_ov772x(i2c_get_clientdata(client));
> >
> > -	v4l2_clk_put(priv->clk);
> > +	clk_put(priv->clk);
> > +	if (priv->pwdn_gpio)
> > +		gpiod_put(priv->pwdn_gpio);
> >  	v4l2_device_unregister_subdev(&priv->subdev);
> >  	v4l2_ctrl_handler_free(&priv->hdl);
> >  	return 0;
> > @@ -1119,6 +1176,6 @@ static struct i2c_driver ov772x_i2c_driver = {
> >
> >  module_i2c_driver(ov772x_i2c_driver);
> >
> > -MODULE_DESCRIPTION("SoC Camera driver for ov772x");
> > +MODULE_DESCRIPTION("V4L2 driver for OV772x image sensor");
>
> Ditto: lower case ov772x.

Will change both. Thanks.

>
> >  MODULE_AUTHOR("Kuninori Morimoto");
> >  MODULE_LICENSE("GPL v2");
>
> Hmm, shouldn't there be a struct of_device_id as well? So this can be
> used in the device tree?
>
> I see this sensor was only tested with a non-dt platform. Is it possible
> to test this sensor with the GR-Peach platform (which I gather uses the
> device tree)?

I don't have that sensor, I'm sorry. I have it on Migo-R which I'm
accessing and developing on from remote.

>
> Making this driver DT compliant can be done as a follow-up patch.

Adding DT compatibility would imply adding bindings etc.
I can do it later and only compile-test, I assume.

Thanks
   j

>
> > diff --git a/include/media/i2c/ov772x.h b/include/media/i2c/ov772x.h
> > index 00dbb7c..27d087b 100644
> > --- a/include/media/i2c/ov772x.h
> > +++ b/include/media/i2c/ov772x.h
> > @@ -48,8 +48,10 @@ struct ov772x_edge_ctrl {
> >  	.threshold = (t & OV772X_EDGE_THRESHOLD_MASK),	\
> >  }
> >
> > -/*
> > - * ov772x camera info
> > +/**
> > + * ov772x_camera_info -	ov772x driver interface structure
> > + * @flags:		Sensor configuration flags
> > + * @edgectrl:		Sensor edge control
> >   */
> >  struct ov772x_camera_info {
> >  	unsigned long		flags;
> >
>
> Regards,
>
> 	Hans

Powered by blists - more mailing lists