The first two patches make the sensor work in my setup, and adds a
missing error print that can happen when the dts is wrong.
The last two patches add extra API support for orientation/rotation and
get_selection that is wanted by libcamera to function correctly.
Signed-off-by: Luca Weiss <[email protected]>
---
Luca Weiss (4):
media: i2c: ov2685: Make reset gpio optional
media: i2c: ov2685: Add print for power on write failed
media: i2c: ov2685: Add controls from fwnode
media: i2c: ov2685: Add .get_selection() support
drivers/media/i2c/ov2685.c | 78 ++++++++++++++++++++++++++++++++++++++++++++--
1 file changed, 76 insertions(+), 2 deletions(-)
---
base-commit: b4c97ca9bb2381580a132a6c71b0e93ac6a4524a
change-id: 20230129-ov2685-improvements-b03bdcf1c290
Best regards,
--
Luca Weiss <[email protected]>
Add support for the .get_selection() pad operation to the ov2685 sensor
driver.
Report the native sensor size (pixel array), the crop bounds (readable
pixel array area) and the current and default analog crop rectangles.
Signed-off-by: Luca Weiss <[email protected]>
---
drivers/media/i2c/ov2685.c | 61 ++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 61 insertions(+)
diff --git a/drivers/media/i2c/ov2685.c b/drivers/media/i2c/ov2685.c
index bfced11b178b..7ebf36d1a8cc 100644
--- a/drivers/media/i2c/ov2685.c
+++ b/drivers/media/i2c/ov2685.c
@@ -56,6 +56,9 @@
#define OV2685_REG_VALUE_16BIT 2
#define OV2685_REG_VALUE_24BIT 3
+#define OV2685_NATIVE_WIDTH 1616
+#define OV2685_NATIVE_HEIGHT 1216
+
#define OV2685_LANES 1
#define OV2685_BITS_PER_SAMPLE 10
@@ -78,6 +81,7 @@ struct ov2685_mode {
u32 exp_def;
u32 hts_def;
u32 vts_def;
+ const struct v4l2_rect *analog_crop;
const struct regval *reg_list;
};
@@ -231,6 +235,13 @@ static const int ov2685_test_pattern_val[] = {
OV2685_TEST_PATTERN_COLOR_SQUARE,
};
+static const struct v4l2_rect ov2685_analog_crop = {
+ .left = 8,
+ .top = 8,
+ .width = 1600,
+ .height = 1200,
+};
+
static const struct ov2685_mode supported_modes[] = {
{
.width = 1600,
@@ -238,6 +249,7 @@ static const struct ov2685_mode supported_modes[] = {
.exp_def = 0x04ee,
.hts_def = 0x06a4,
.vts_def = 0x050e,
+ .analog_crop = &ov2685_analog_crop,
.reg_list = ov2685_1600x1200_regs,
},
};
@@ -384,6 +396,53 @@ static int ov2685_enum_frame_sizes(struct v4l2_subdev *sd,
return 0;
}
+static const struct v4l2_rect *
+__ov2685_get_pad_crop(struct ov2685 *ov2685,
+ struct v4l2_subdev_state *state, unsigned int pad,
+ enum v4l2_subdev_format_whence which)
+{
+ const struct ov2685_mode *mode = ov2685->cur_mode;
+
+ switch (which) {
+ case V4L2_SUBDEV_FORMAT_TRY:
+ return v4l2_subdev_get_try_crop(&ov2685->subdev, state, pad);
+ case V4L2_SUBDEV_FORMAT_ACTIVE:
+ return mode->analog_crop;
+ }
+
+ return NULL;
+}
+
+static int ov2685_get_selection(struct v4l2_subdev *sd,
+ struct v4l2_subdev_state *sd_state,
+ struct v4l2_subdev_selection *sel)
+{
+ struct ov2685 *ov2685 = to_ov2685(sd);
+
+ switch (sel->target) {
+ case V4L2_SEL_TGT_CROP:
+ mutex_lock(&ov2685->mutex);
+ sel->r = *__ov2685_get_pad_crop(ov2685, sd_state, sel->pad,
+ sel->which);
+ mutex_unlock(&ov2685->mutex);
+ break;
+ case V4L2_SEL_TGT_NATIVE_SIZE:
+ case V4L2_SEL_TGT_CROP_BOUNDS:
+ sel->r.top = 0;
+ sel->r.left = 0;
+ sel->r.width = OV2685_NATIVE_WIDTH;
+ sel->r.height = OV2685_NATIVE_HEIGHT;
+ break;
+ case V4L2_SEL_TGT_CROP_DEFAULT:
+ sel->r = ov2685_analog_crop;
+ break;
+ default:
+ return -EINVAL;
+ }
+
+ return 0;
+}
+
/* Calculate the delay in us by clock rate and clock cycles */
static inline u32 ov2685_cal_delay(u32 cycles)
{
@@ -592,6 +651,8 @@ static const struct v4l2_subdev_pad_ops ov2685_pad_ops = {
.enum_frame_size = ov2685_enum_frame_sizes,
.get_fmt = ov2685_get_fmt,
.set_fmt = ov2685_set_fmt,
+ .get_selection = ov2685_get_selection,
+ .set_selection = ov2685_get_selection,
};
static const struct v4l2_subdev_ops ov2685_subdev_ops = {
--
2.39.1
In some setups XSHUTDOWN is connected to DOVDD when it's unused,
therefore treat the reset gpio as optional.
Signed-off-by: Luca Weiss <[email protected]>
---
drivers/media/i2c/ov2685.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/media/i2c/ov2685.c b/drivers/media/i2c/ov2685.c
index a3b524f15d89..a422f4c8a2eb 100644
--- a/drivers/media/i2c/ov2685.c
+++ b/drivers/media/i2c/ov2685.c
@@ -734,7 +734,7 @@ static int ov2685_probe(struct i2c_client *client,
if (clk_get_rate(ov2685->xvclk) != OV2685_XVCLK_FREQ)
dev_warn(dev, "xvclk mismatched, modes are based on 24MHz\n");
- ov2685->reset_gpio = devm_gpiod_get(dev, "reset", GPIOD_OUT_LOW);
+ ov2685->reset_gpio = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_LOW);
if (IS_ERR(ov2685->reset_gpio)) {
dev_err(dev, "Failed to get reset-gpios\n");
return -EINVAL;
--
2.39.1
Hi Luca
On Sun, Jan 29, 2023 at 10:42:35AM +0100, Luca Weiss wrote:
> In some setups XSHUTDOWN is connected to DOVDD when it's unused,
> therefore treat the reset gpio as optional.
I don't have a datasheet for this sensor, but OV sensors usually have
to gpio lines to control powerdown and reset. Datasheets usually
suggest to hook one of the 2 to DOVDD and control the other from the
SoC. How is the sensor hooked up in your design ? No gpio lines is
controlled by the SoC ?
Another question is if we need to software-reset the sensor if no gpio
line is hooked up to XSHUTDOWN.
>
> Signed-off-by: Luca Weiss <[email protected]>
> ---
> drivers/media/i2c/ov2685.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/media/i2c/ov2685.c b/drivers/media/i2c/ov2685.c
> index a3b524f15d89..a422f4c8a2eb 100644
> --- a/drivers/media/i2c/ov2685.c
> +++ b/drivers/media/i2c/ov2685.c
> @@ -734,7 +734,7 @@ static int ov2685_probe(struct i2c_client *client,
> if (clk_get_rate(ov2685->xvclk) != OV2685_XVCLK_FREQ)
> dev_warn(dev, "xvclk mismatched, modes are based on 24MHz\n");
>
> - ov2685->reset_gpio = devm_gpiod_get(dev, "reset", GPIOD_OUT_LOW);
> + ov2685->reset_gpio = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_LOW);
> if (IS_ERR(ov2685->reset_gpio)) {
> dev_err(dev, "Failed to get reset-gpios\n");
> return -EINVAL;
>
> --
> 2.39.1
>
Hi Luca
On Sun, Jan 29, 2023 at 10:42:38AM +0100, Luca Weiss wrote:
> Add support for the .get_selection() pad operation to the ov2685 sensor
> driver.
>
> Report the native sensor size (pixel array), the crop bounds (readable
> pixel array area) and the current and default analog crop rectangles.
>
> Signed-off-by: Luca Weiss <[email protected]>
As the driver supports a single mode you could have hard-coded
the rectangle sizes in get_selection(), but this way is much better as
it prepares for adding more modes to the driver eventually.
Thanks
Reviewed-by: Jacopo Mondi <[email protected]>
> ---
> drivers/media/i2c/ov2685.c | 61 ++++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 61 insertions(+)
>
> diff --git a/drivers/media/i2c/ov2685.c b/drivers/media/i2c/ov2685.c
> index bfced11b178b..7ebf36d1a8cc 100644
> --- a/drivers/media/i2c/ov2685.c
> +++ b/drivers/media/i2c/ov2685.c
> @@ -56,6 +56,9 @@
> #define OV2685_REG_VALUE_16BIT 2
> #define OV2685_REG_VALUE_24BIT 3
>
> +#define OV2685_NATIVE_WIDTH 1616
> +#define OV2685_NATIVE_HEIGHT 1216
> +
> #define OV2685_LANES 1
> #define OV2685_BITS_PER_SAMPLE 10
>
> @@ -78,6 +81,7 @@ struct ov2685_mode {
> u32 exp_def;
> u32 hts_def;
> u32 vts_def;
> + const struct v4l2_rect *analog_crop;
> const struct regval *reg_list;
> };
>
> @@ -231,6 +235,13 @@ static const int ov2685_test_pattern_val[] = {
> OV2685_TEST_PATTERN_COLOR_SQUARE,
> };
>
> +static const struct v4l2_rect ov2685_analog_crop = {
> + .left = 8,
> + .top = 8,
> + .width = 1600,
> + .height = 1200,
> +};
> +
> static const struct ov2685_mode supported_modes[] = {
> {
> .width = 1600,
> @@ -238,6 +249,7 @@ static const struct ov2685_mode supported_modes[] = {
> .exp_def = 0x04ee,
> .hts_def = 0x06a4,
> .vts_def = 0x050e,
> + .analog_crop = &ov2685_analog_crop,
> .reg_list = ov2685_1600x1200_regs,
> },
> };
> @@ -384,6 +396,53 @@ static int ov2685_enum_frame_sizes(struct v4l2_subdev *sd,
> return 0;
> }
>
> +static const struct v4l2_rect *
> +__ov2685_get_pad_crop(struct ov2685 *ov2685,
> + struct v4l2_subdev_state *state, unsigned int pad,
> + enum v4l2_subdev_format_whence which)
> +{
> + const struct ov2685_mode *mode = ov2685->cur_mode;
> +
> + switch (which) {
> + case V4L2_SUBDEV_FORMAT_TRY:
> + return v4l2_subdev_get_try_crop(&ov2685->subdev, state, pad);
> + case V4L2_SUBDEV_FORMAT_ACTIVE:
> + return mode->analog_crop;
> + }
> +
> + return NULL;
> +}
> +
> +static int ov2685_get_selection(struct v4l2_subdev *sd,
> + struct v4l2_subdev_state *sd_state,
> + struct v4l2_subdev_selection *sel)
> +{
> + struct ov2685 *ov2685 = to_ov2685(sd);
> +
> + switch (sel->target) {
> + case V4L2_SEL_TGT_CROP:
> + mutex_lock(&ov2685->mutex);
> + sel->r = *__ov2685_get_pad_crop(ov2685, sd_state, sel->pad,
> + sel->which);
> + mutex_unlock(&ov2685->mutex);
> + break;
> + case V4L2_SEL_TGT_NATIVE_SIZE:
> + case V4L2_SEL_TGT_CROP_BOUNDS:
> + sel->r.top = 0;
> + sel->r.left = 0;
> + sel->r.width = OV2685_NATIVE_WIDTH;
> + sel->r.height = OV2685_NATIVE_HEIGHT;
> + break;
> + case V4L2_SEL_TGT_CROP_DEFAULT:
> + sel->r = ov2685_analog_crop;
> + break;
> + default:
> + return -EINVAL;
> + }
> +
> + return 0;
> +}
> +
> /* Calculate the delay in us by clock rate and clock cycles */
> static inline u32 ov2685_cal_delay(u32 cycles)
> {
> @@ -592,6 +651,8 @@ static const struct v4l2_subdev_pad_ops ov2685_pad_ops = {
> .enum_frame_size = ov2685_enum_frame_sizes,
> .get_fmt = ov2685_get_fmt,
> .set_fmt = ov2685_set_fmt,
> + .get_selection = ov2685_get_selection,
> + .set_selection = ov2685_get_selection,
> };
>
> static const struct v4l2_subdev_ops ov2685_subdev_ops = {
>
> --
> 2.39.1
>
On Sonntag, 29. J?nner 2023 12:22:49 CET Jacopo Mondi wrote:
> Hi Luca
>
> On Sun, Jan 29, 2023 at 10:42:35AM +0100, Luca Weiss wrote:
> > In some setups XSHUTDOWN is connected to DOVDD when it's unused,
> > therefore treat the reset gpio as optional.
>
> I don't have a datasheet for this sensor, but OV sensors usually have
> to gpio lines to control powerdown and reset. Datasheets usually
> suggest to hook one of the 2 to DOVDD and control the other from the
> SoC. How is the sensor hooked up in your design ? No gpio lines is
> controlled by the SoC ?
It looks like this sensor only has XSHUTDOWN pin and no extra reset pin.
In my setup there's the normal I2C & CSI & mclk hookups, but the supply lines
and shutdown line are all just connected to regulator-fixed, so gpio-
controlled on/off regulators.
>
> Another question is if we need to software-reset the sensor if no gpio
> line is hooked up to XSHUTDOWN.
The datasheet mentions it resets itself during power up (so when the supplies
are turned on), so I don't think we need to add anything.
Regards
Luca
>
> > Signed-off-by: Luca Weiss <[email protected]>
> > ---
> >
> > drivers/media/i2c/ov2685.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/media/i2c/ov2685.c b/drivers/media/i2c/ov2685.c
> > index a3b524f15d89..a422f4c8a2eb 100644
> > --- a/drivers/media/i2c/ov2685.c
> > +++ b/drivers/media/i2c/ov2685.c
> > @@ -734,7 +734,7 @@ static int ov2685_probe(struct i2c_client *client,
> >
> > if (clk_get_rate(ov2685->xvclk) != OV2685_XVCLK_FREQ)
> >
> > dev_warn(dev, "xvclk mismatched, modes are based on
24MHz\n");
> >
> > - ov2685->reset_gpio = devm_gpiod_get(dev, "reset", GPIOD_OUT_LOW);
> > + ov2685->reset_gpio = devm_gpiod_get_optional(dev, "reset",
> > GPIOD_OUT_LOW);>
> > if (IS_ERR(ov2685->reset_gpio)) {
> >
> > dev_err(dev, "Failed to get reset-gpios\n");
> > return -EINVAL;
> >
> > --
> > 2.39.1
Hi Luca
On Sun, Jan 29, 2023 at 12:49:03PM +0100, Luca Weiss wrote:
> On Sonntag, 29. Jänner 2023 12:22:49 CET Jacopo Mondi wrote:
> > Hi Luca
> >
> > On Sun, Jan 29, 2023 at 10:42:35AM +0100, Luca Weiss wrote:
> > > In some setups XSHUTDOWN is connected to DOVDD when it's unused,
> > > therefore treat the reset gpio as optional.
> >
> > I don't have a datasheet for this sensor, but OV sensors usually have
> > to gpio lines to control powerdown and reset. Datasheets usually
> > suggest to hook one of the 2 to DOVDD and control the other from the
> > SoC. How is the sensor hooked up in your design ? No gpio lines is
> > controlled by the SoC ?
>
> It looks like this sensor only has XSHUTDOWN pin and no extra reset pin.
>
Ack, I see the same for OV2680 (for which I have a datasheet)
> In my setup there's the normal I2C & CSI & mclk hookups, but the supply lines
> and shutdown line are all just connected to regulator-fixed, so gpio-
> controlled on/off regulators.
>
> >
> > Another question is if we need to software-reset the sensor if no gpio
> > line is hooked up to XSHUTDOWN.
>
> The datasheet mentions it resets itself during power up (so when the supplies
> are turned on), so I don't think we need to add anything.
>
Thanks for the clarification!
Reviewed-by: Jacopo Mondi <[email protected]>
Thanks
j
> Regards
> Luca
>
> >
> > > Signed-off-by: Luca Weiss <[email protected]>
> > > ---
> > >
> > > drivers/media/i2c/ov2685.c | 2 +-
> > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/media/i2c/ov2685.c b/drivers/media/i2c/ov2685.c
> > > index a3b524f15d89..a422f4c8a2eb 100644
> > > --- a/drivers/media/i2c/ov2685.c
> > > +++ b/drivers/media/i2c/ov2685.c
> > > @@ -734,7 +734,7 @@ static int ov2685_probe(struct i2c_client *client,
> > >
> > > if (clk_get_rate(ov2685->xvclk) != OV2685_XVCLK_FREQ)
> > >
> > > dev_warn(dev, "xvclk mismatched, modes are based on
> 24MHz\n");
> > >
> > > - ov2685->reset_gpio = devm_gpiod_get(dev, "reset", GPIOD_OUT_LOW);
> > > + ov2685->reset_gpio = devm_gpiod_get_optional(dev, "reset",
> > > GPIOD_OUT_LOW);>
> > > if (IS_ERR(ov2685->reset_gpio)) {
> > >
> > > dev_err(dev, "Failed to get reset-gpios\n");
> > > return -EINVAL;
> > >
> > > --
> > > 2.39.1
>
>
>
>