[PATCH 3/5] media: ov2640: add primary dt support

[Laurent Pinchart]

Hi Josh,

On Tuesday 09 December 2014 11:10:01 Josh Wu wrote:
> On 12/9/2014 2:39 AM, Laurent Pinchart wrote:
> > Hi Josh,
> > 
> > Thank you for the patch.
> > 
> > On Monday 08 December 2014 19:29:05 Josh Wu wrote:
> >> Add device tree support for ov2640.
> >> 
> >> Cc: devicetree at vger.kernel.org
> >> Signed-off-by: Josh Wu <josh.wu at atmel.com>
> >> ---
> >> 
> >> v1 -> v2:
> >>    1. use gpiod APIs.
> >>    2. change the gpio pin's name according to datasheet.
> >>    3. reduce the delay for .reset() function.
> >>   
> >>   drivers/media/i2c/soc_camera/ov2640.c | 86 +++++++++++++++++++++++++---
> >>   1 file changed, 80 insertions(+), 6 deletions(-)
> >> 
> >> diff --git a/drivers/media/i2c/soc_camera/ov2640.c
> >> b/drivers/media/i2c/soc_camera/ov2640.c index 9ee910d..2a57979 100644
> >> --- a/drivers/media/i2c/soc_camera/ov2640.c
> >> +++ b/drivers/media/i2c/soc_camera/ov2640.c

[snip]

> >> +static int ov2640_probe_dt(struct i2c_client *client,
> >> +		struct ov2640_priv *priv)
> >> +{
> >> +	priv->resetb_gpio = devm_gpiod_get_optional(&client->dev, "resetb",
> >> +			GPIOD_OUT_HIGH);
> >> +	if (!priv->resetb_gpio)
> >> +		dev_warn(&client->dev, "resetb gpio not found!\n");
> > 
> > No need to warn here, it's perfectly fine if the reset signal isn't
> > connected to a GPIO.
> 
> I want to print some information if no GPIO is assigned. So I'd like use
> dev_dbg() here.
> What do you feel?

If you want to print a message in that case dev_dbg is the most appropriate 
log level. I would skip it completely, but that's up to you.

> >> +	else if (IS_ERR(priv->resetb_gpio))
> >> +		return -EINVAL;
> >> +
> >> +	priv->pwdn_gpio = devm_gpiod_get_optional(&client->dev, "pwdn",
> >> +			GPIOD_OUT_HIGH);
> >> +	if (!priv->pwdn_gpio)
> >> +		dev_warn(&client->dev, "pwdn gpio not found!\n");
> > 
> > Same here.
> 
> ditto.
> 
> >> +	else if (IS_ERR(priv->pwdn_gpio))
> >> +		return -EINVAL;
> >> +
> >> +	/* Initialize the soc_camera_subdev_desc */
> >> +	priv->ssdd_dt.power = ov2640_hw_power;
> >> +	priv->ssdd_dt.reset = ov2640_hw_reset;
> >> +	client->dev.platform_data = &priv->ssdd_dt;
> >> +
> >> +	return 0;
> >> +}
> >> +
> >>   /*
> >>    * i2c_driver functions
> >>    */

-- 
Regards,

Laurent Pinchart