| Message ID | 20190912130007.4469-1-bparrot@ti.com |
|---|---|
| Headers | show |
| Series | media: i2c: ov2659: maintenance series | expand |
Hi Benoit, On Thu, Sep 12, 2019 at 1:58 PM Benoit Parrot <bparrot@ti.com> wrote: > > On some board it is possible that the sensor 'powerdown' > pin might be controlled with a gpio instead of being > tied to always powered. > > This patch add support to specify an optional gpio > which will be set at probe time and remained on. > > Signed-off-by: Benoit Parrot <bparrot@ti.com> > --- > drivers/media/i2c/Kconfig | 2 +- > drivers/media/i2c/ov2659.c | 13 +++++++++++++ > 2 files changed, 14 insertions(+), 1 deletion(-) > > diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig > index 7eee1812bba3..315c1d8bdb7b 100644 > --- a/drivers/media/i2c/Kconfig > +++ b/drivers/media/i2c/Kconfig > @@ -634,7 +634,7 @@ config VIDEO_OV2640 > config VIDEO_OV2659 > tristate "OmniVision OV2659 sensor support" > depends on VIDEO_V4L2 && I2C > - depends on MEDIA_CAMERA_SUPPORT > + depends on MEDIA_CAMERA_SUPPORT && GPIOLIB > select V4L2_FWNODE > help > This is a Video4Linux2 sensor driver for the OmniVision > diff --git a/drivers/media/i2c/ov2659.c b/drivers/media/i2c/ov2659.c > index efbe6dc720e2..c64f73bef336 100644 > --- a/drivers/media/i2c/ov2659.c > +++ b/drivers/media/i2c/ov2659.c > @@ -32,6 +32,8 @@ > #include <linux/module.h> > #include <linux/of.h> > #include <linux/of_graph.h> > +#include <linux/of_gpio.h> > +#include <linux/gpio/consumer.h> > #include <linux/slab.h> > #include <linux/uaccess.h> > #include <linux/videodev2.h> > @@ -232,6 +234,8 @@ struct ov2659 { > struct sensor_register *format_ctrl_regs; > struct ov2659_pll_ctrl pll; > int streaming; > + /* used to control the sensor powerdownN pin */ > + struct gpio_desc *pwrdn_gpio; > }; > > static const struct sensor_register ov2659_init_regs[] = { > @@ -1391,6 +1395,7 @@ static int ov2659_probe(struct i2c_client *client) > struct v4l2_subdev *sd; > struct ov2659 *ov2659; > struct clk *clk; > + struct gpio_desc *gpio; you don't need the local var here you can just assign it directly to pwrdn_gpio. > int ret; > > if (!pdata) { > @@ -1414,6 +1419,14 @@ static int ov2659_probe(struct i2c_client *client) > ov2659->xvclk_frequency > 27000000) > return -EINVAL; > > + /* Optional gpio don't fail if not present */ > + gpio = devm_gpiod_get_optional(&client->dev, "powerdown", > + GPIOD_OUT_HIGH); > + if (IS_ERR(gpio)) > + return PTR_ERR(gpio); > + > + ov2659->pwrdn_gpio = gpio; > + apart from assigning it you don't actually use it. you will also have to read the reset gpio pin and implement ov2659_set_power() and call it in appropriate places/ s_power ? Cheers, --Prabhakar Lad
Lad, Prabhakar <prabhakar.csengg@gmail.com> wrote on Sat [2019-Sep-14 11:33:42 +0100]: > Hi Benoit, > > On Thu, Sep 12, 2019 at 1:58 PM Benoit Parrot <bparrot@ti.com> wrote: > > > > On some board it is possible that the sensor 'powerdown' > > pin might be controlled with a gpio instead of being > > tied to always powered. > > > > This patch add support to specify an optional gpio > > which will be set at probe time and remained on. > > > > Signed-off-by: Benoit Parrot <bparrot@ti.com> > > --- > > drivers/media/i2c/Kconfig | 2 +- > > drivers/media/i2c/ov2659.c | 13 +++++++++++++ > > 2 files changed, 14 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig > > index 7eee1812bba3..315c1d8bdb7b 100644 > > --- a/drivers/media/i2c/Kconfig > > +++ b/drivers/media/i2c/Kconfig > > @@ -634,7 +634,7 @@ config VIDEO_OV2640 > > config VIDEO_OV2659 > > tristate "OmniVision OV2659 sensor support" > > depends on VIDEO_V4L2 && I2C > > - depends on MEDIA_CAMERA_SUPPORT > > + depends on MEDIA_CAMERA_SUPPORT && GPIOLIB > > select V4L2_FWNODE > > help > > This is a Video4Linux2 sensor driver for the OmniVision > > diff --git a/drivers/media/i2c/ov2659.c b/drivers/media/i2c/ov2659.c > > index efbe6dc720e2..c64f73bef336 100644 > > --- a/drivers/media/i2c/ov2659.c > > +++ b/drivers/media/i2c/ov2659.c > > @@ -32,6 +32,8 @@ > > #include <linux/module.h> > > #include <linux/of.h> > > #include <linux/of_graph.h> > > +#include <linux/of_gpio.h> > > +#include <linux/gpio/consumer.h> > > #include <linux/slab.h> > > #include <linux/uaccess.h> > > #include <linux/videodev2.h> > > @@ -232,6 +234,8 @@ struct ov2659 { > > struct sensor_register *format_ctrl_regs; > > struct ov2659_pll_ctrl pll; > > int streaming; > > + /* used to control the sensor powerdownN pin */ > > + struct gpio_desc *pwrdn_gpio; > > }; > > > > static const struct sensor_register ov2659_init_regs[] = { > > @@ -1391,6 +1395,7 @@ static int ov2659_probe(struct i2c_client *client) > > struct v4l2_subdev *sd; > > struct ov2659 *ov2659; > > struct clk *clk; > > + struct gpio_desc *gpio; > > you don't need the local var here you can just assign it directly to pwrdn_gpio. Ok. > > > int ret; > > > > if (!pdata) { > > @@ -1414,6 +1419,14 @@ static int ov2659_probe(struct i2c_client *client) > > ov2659->xvclk_frequency > 27000000) > > return -EINVAL; > > > > + /* Optional gpio don't fail if not present */ > > + gpio = devm_gpiod_get_optional(&client->dev, "powerdown", > > + GPIOD_OUT_HIGH); > > + if (IS_ERR(gpio)) > > + return PTR_ERR(gpio); > > + > > + ov2659->pwrdn_gpio = gpio; > > + > apart from assigning it you don't actually use it. > > you will also have to read the reset gpio pin and implement > ov2659_set_power() and > call it in appropriate places/ s_power ? Well I am not sure I want to go that far. On most board I have the sensor is always powered as soon as the board gets powered. Which is why we go through a S/W reset before starting a stream. I didn't want to change the logic here too much. I'll check this out a little more. Benoit > > Cheers, > --Prabhakar Lad
This patch series is a collection of patches we have been carrying for a while. It includes a few sensor register fixes which would cause visual artifacts at lower resolution and also at 720p. Also on some board the 'powerdown' is not tied to always on so we add support for an optional powerdown gpio. Since these camera are removable on some board we alos need the driver to actually fail when there is no hardware present so the driver is actually removed. Finally, we update the licensing statement to use SPDX licensing. Benoit Parrot (6): media: i2c: ov2659: Fix for image wrap-around in lower resolution media: i2c: ov2659: Fix sensor detection to actually fail when device is not present media: dt-bindings: ov2659: add powerdown-gpios optional property media: i2c: ov2659: Add optional powerdown gpio handling media: i2c: ov2659: Fix missing 720p register config media: i2c: ov2659: Switch to SPDX Licensing .../devicetree/bindings/media/i2c/ov2659.txt | 6 +++ drivers/media/i2c/Kconfig | 2 +- drivers/media/i2c/ov2659.c | 40 +++++++++++-------- 3 files changed, 30 insertions(+), 18 deletions(-)