Hi Julien,
Thanks for the review.
On Thu, Mar 14, 2024 at 09:24:48AM +0100, Julien Massot wrote:
> Hi Sakari,
>
> On 3/13/24 08:25, Sakari Ailus wrote:
> > Add support for embedded data. This introduces two internal pads for pixel
> > and embedded data streams. As the driver supports a single mode only,
> > there's no need for backward compatibility in mode selection.
> >
> > The embedded data is configured to be placed before the image data whereas
> > after the image data is the default.
> >
> > Signed-off-by: Sakari Ailus <sakari.ailus@xxxxxxxxxxxxxxx>
> > ---
> > drivers/media/i2c/ov2740.c | 150 +++++++++++++++++++++++++++++++++----
> > 1 file changed, 137 insertions(+), 13 deletions(-)
> >
> > diff --git a/drivers/media/i2c/ov2740.c b/drivers/media/i2c/ov2740.c
> > index df57f0096e98..7488b2535071 100644
> > --- a/drivers/media/i2c/ov2740.c
> > +++ b/drivers/media/i2c/ov2740.c
> > @@ -11,6 +11,7 @@
> > #include <linux/pm_runtime.h>
> > #include <linux/nvmem-provider.h>
> > #include <linux/regmap.h>
> > +#include <media/mipi-csi2.h>
> > #include <media/v4l2-ctrls.h>
> > #include <media/v4l2-device.h>
> > #include <media/v4l2-fwnode.h>
> > @@ -71,11 +72,31 @@
> > #define OV2740_REG_ISP_CTRL00 0x5000
> > /* ISP CTRL01 */
> > #define OV2740_REG_ISP_CTRL01 0x5001
> > +
> > +/* Embedded data line location control */
> > +#define OV2740_REG_EMBEDDED_FLAG 0x5a08
> > +#define OV2740_EMBEDDED_FLAG_FOOTER BIT(2) /* otherwise it's in header */
> > +#define OV2740_EMBEDDED_FLAG_MYSTERY BIT(1)
> > /* Customer Addresses: 0x7010 - 0x710F */
> > #define CUSTOMER_USE_OTP_SIZE 0x100
> > /* OTP registers from sensor */
> > #define OV2740_REG_OTP_CUSTOMER 0x7010
> > +enum {
> > + OV2740_PAD_SOURCE,
> > + OV2740_PAD_PIXEL,
> > + OV2740_PAD_META,
> > + OV2740_NUM_PADS,
> > +};
> > +
> > +enum {
> > + OV2740_STREAM_PIXEL,
> > + OV2740_STREAM_META,
> > +};
> > +
> > +#define OV2740_META_WIDTH 100U /* 97 bytes of actual data */
> > +#define OV2740_META_HEIGHT 1U
> > +
> > struct nvm_data {
> > struct nvmem_device *nvmem;
> > struct regmap *regmap;
> > @@ -513,7 +534,7 @@ static const struct ov2740_mode supported_modes_180mhz[] = {
> > struct ov2740 {
> > struct v4l2_subdev sd;
> > - struct media_pad pad;
> > + struct media_pad pads[OV2740_NUM_PADS];
> > struct v4l2_ctrl_handler ctrl_handler;
> > /* V4L2 Controls */
> > @@ -976,6 +997,11 @@ static int ov2740_enable_streams(struct v4l2_subdev *sd,
> > if (ret)
> > goto out_pm_put;
> > + ret = ov2740_write_reg(ov2740, OV2740_REG_EMBEDDED_FLAG, 1,
> > + OV2740_EMBEDDED_FLAG_MYSTERY);
> > + if (ret)
> > + return ret;
> > +
> > ret = ov2740_write_reg(ov2740, OV2740_REG_MODE_SELECT, 1,
> > OV2740_MODE_STREAMING);
> > if (ret) {
> > @@ -1013,23 +1039,49 @@ static int ov2740_disable_streams(struct v4l2_subdev *sd,
> > return ret;
> > }
> > -static int ov2740_set_format(struct v4l2_subdev *sd,
> > - struct v4l2_subdev_state *sd_state,
> > - struct v4l2_subdev_format *fmt)
> > +static int __ov2740_set_format(struct v4l2_subdev *sd,
> > + struct v4l2_subdev_state *sd_state,
> > + struct v4l2_mbus_framefmt *format,
> > + enum v4l2_subdev_format_whence which,
> > + unsigned int pad, unsigned int stream)
> > {
> > + struct v4l2_mbus_framefmt *src_pix_fmt, *src_meta_fmt, *pix_fmt,
> > + *meta_fmt;
> > struct ov2740 *ov2740 = to_ov2740(sd);
> > const struct ov2740_mode *mode;
> > s32 vblank_def, h_blank;
> > + /*
> > + * Allow setting format on internal pixel pad as well as the source
> > + * pad's pixel stream (for compatibility).
> > + */
> > + if (pad == OV2740_PAD_SOURCE || pad == OV2740_PAD_META ||
> > + stream == OV2740_STREAM_META) {
> This is equivalent to
> if (pad != OV2740_PAD_PIXEL)
> Correct me if I'm wrong but this code doesn't allow to set the format on the
> source pad.
Good point. I put that to the comment but somehow failed to align the code
with the statement.
>
> Should it be:
> if ((pad == OV2740_PAD_SOURCE && stream == OV2740_STREAM_META) ||
> pad == OV2740_PAD_META) {
This seems better indeed!
>
>
> > + *format = *v4l2_subdev_state_get_format(sd_state, pad, stream);
> > + return 0;
> > + }
> > +
> > + pix_fmt = v4l2_subdev_state_get_format(sd_state, OV2740_PAD_PIXEL, 0);
> > + meta_fmt = v4l2_subdev_state_get_format(sd_state, OV2740_PAD_META, 0);
> > + src_pix_fmt = v4l2_subdev_state_get_format(sd_state, OV2740_PAD_SOURCE,
> > + OV2740_STREAM_PIXEL);
> > + src_meta_fmt = v4l2_subdev_state_get_format(sd_state, OV2740_PAD_SOURCE,
> > + OV2740_STREAM_META);
> > +
> > mode = v4l2_find_nearest_size(ov2740->supported_modes,
> > ov2740->supported_modes_count,
> > width, height,
> > - fmt->format.width, fmt->format.height);
> > + format->width, format->height);
> > + ov2740_update_pad_format(mode, pix_fmt);
> > + *format = *src_pix_fmt = *pix_fmt;
> > - ov2740_update_pad_format(mode, &fmt->format);
> > - *v4l2_subdev_state_get_format(sd_state, fmt->pad) = fmt->format;
> > + meta_fmt->code = MEDIA_BUS_FMT_OV2740_EMBEDDED;
> > + meta_fmt->width = OV2740_META_WIDTH;
> > + meta_fmt->height = OV2740_META_HEIGHT;
> > + *src_meta_fmt = *meta_fmt;
> > + src_meta_fmt->code = MEDIA_BUS_FMT_META_10;
> > - if (fmt->which == V4L2_SUBDEV_FORMAT_TRY)
> > + if (which == V4L2_SUBDEV_FORMAT_TRY)
> > return 0;
> > ov2740->cur_mode = mode;
--
Kind regards,
Sakari Ailus
