diff mbox series

media: i2c: ov5645: Add virtual_channel module parameter

Message ID 20200228164126.17517-1-prabhakar.mahadev-lad.rj@bp.renesas.com (mailing list archive)
State Changes Requested, archived
Delegated to: Sakari Ailus
Headers
Series media: i2c: ov5645: Add virtual_channel module parameter |

Commit Message

Lad, Prabhakar Feb. 28, 2020, 4:41 p.m. UTC 
  OV5645 can operate in virtual channel 0-3 in CSI2 interfaces, this patch
adds support for module parameter virtual_channel to select the required
channel. By default OV5645 operates in virtual channel 0.

Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
---
 drivers/media/i2c/ov5645.c | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

Comments

Fabio Estevam Feb. 28, 2020, 5:31 p.m. UTC | #1 
Hi Lad,

On Fri, Feb 28, 2020 at 1:41 PM Lad Prabhakar
<prabhakar.csengg@gmail.com> wrote:
>
> OV5645 can operate in virtual channel 0-3 in CSI2 interfaces, this patch
> adds support for module parameter virtual_channel to select the required
> channel. By default OV5645 operates in virtual channel 0.
>
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> ---
>  drivers/media/i2c/ov5645.c | 28 ++++++++++++++++++++++++++++
>  1 file changed, 28 insertions(+)
>
> diff --git a/drivers/media/i2c/ov5645.c b/drivers/media/i2c/ov5645.c
> index a6c17d15d754..0a0671164623 100644
> --- a/drivers/media/i2c/ov5645.c
> +++ b/drivers/media/i2c/ov5645.c
> @@ -54,6 +54,7 @@
>  #define OV5645_TIMING_TC_REG21         0x3821
>  #define                OV5645_SENSOR_MIRROR            BIT(1)
>  #define OV5645_MIPI_CTRL00             0x4800
> +#define OV5645_REG_DEBUG_MODE          0x4814
>  #define OV5645_PRE_ISP_TEST_SETTING_1  0x503d
>  #define                OV5645_TEST_PATTERN_MASK        0x3
>  #define                OV5645_SET_TEST_PATTERN(x)      ((x) & OV5645_TEST_PATTERN_MASK)
> @@ -61,6 +62,11 @@
>  #define OV5645_SDE_SAT_U               0x5583
>  #define OV5645_SDE_SAT_V               0x5584
>
> +static u8 virtual_channel;
> +module_param(virtual_channel, byte, 0644);
> +MODULE_PARM_DESC(virtual_channel,
> +                "MIPI CSI-2 virtual channel (0..3), default 0");

Should this be a device tree property instead?
Lad, Prabhakar March 2, 2020, 7:18 a.m. UTC | #2 
Hi Fabio,

Thank you for the review.

On Fri, Feb 28, 2020 at 5:31 PM Fabio Estevam <festevam@gmail.com> wrote:
>
> Hi Lad,
>
> On Fri, Feb 28, 2020 at 1:41 PM Lad Prabhakar
> <prabhakar.csengg@gmail.com> wrote:
> >
> > OV5645 can operate in virtual channel 0-3 in CSI2 interfaces, this patch
> > adds support for module parameter virtual_channel to select the required
> > channel. By default OV5645 operates in virtual channel 0.
> >
> > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > ---
> >  drivers/media/i2c/ov5645.c | 28 ++++++++++++++++++++++++++++
> >  1 file changed, 28 insertions(+)
> >
> > diff --git a/drivers/media/i2c/ov5645.c b/drivers/media/i2c/ov5645.c
> > index a6c17d15d754..0a0671164623 100644
> > --- a/drivers/media/i2c/ov5645.c
> > +++ b/drivers/media/i2c/ov5645.c
> > @@ -54,6 +54,7 @@
> >  #define OV5645_TIMING_TC_REG21         0x3821
> >  #define                OV5645_SENSOR_MIRROR            BIT(1)
> >  #define OV5645_MIPI_CTRL00             0x4800
> > +#define OV5645_REG_DEBUG_MODE          0x4814
> >  #define OV5645_PRE_ISP_TEST_SETTING_1  0x503d
> >  #define                OV5645_TEST_PATTERN_MASK        0x3
> >  #define                OV5645_SET_TEST_PATTERN(x)      ((x) & OV5645_TEST_PATTERN_MASK)
> > @@ -61,6 +62,11 @@
> >  #define OV5645_SDE_SAT_U               0x5583
> >  #define OV5645_SDE_SAT_V               0x5584
> >
> > +static u8 virtual_channel;
> > +module_param(virtual_channel, byte, 0644);
> > +MODULE_PARM_DESC(virtual_channel,
> > +                "MIPI CSI-2 virtual channel (0..3), default 0");
>
> Should this be a device tree property instead?
I did give a thought about it, but making this as DT property would
make it more stiff.

Cheers,
--Prabhakar
Fabio Estevam March 2, 2020, 3:33 p.m. UTC | #3 
Hi Prabhakar,

On Mon, Mar 2, 2020 at 4:19 AM Lad, Prabhakar
<prabhakar.csengg@gmail.com> wrote:

> > Should this be a device tree property instead?
> I did give a thought about it, but making this as DT property would
> make it more stiff.

In case a system has two OV5645 and we want to operate each OV5645
with a different virtual channel, it will not be possible with the
module_param approach.

Using a device tree property would make it possible though, so I think
it makes more sense to use a device tree property for this.

Thanks
Ezequiel Garcia March 3, 2020, 3:01 a.m. UTC | #4 
Adding Niklas and Jacopo,

On Mon, Mar 2, 2020, 12:33 PM Fabio Estevam <festevam@gmail.com> wrote:
>
> Hi Prabhakar,
>
> On Mon, Mar 2, 2020 at 4:19 AM Lad, Prabhakar
> <prabhakar.csengg@gmail.com> wrote:
>
> > > Should this be a device tree property instead?
> > I did give a thought about it, but making this as DT property would
> > make it more stiff.
>
> In case a system has two OV5645 and we want to operate each OV5645
> with a different virtual channel, it will not be possible with the
> module_param approach.
>
> Using a device tree property would make it possible though, so I think
> it makes more sense to use a device tree property for this.
>

As often happens, driver parameter is probably the easiest and less
invasive way to customize a driver, so I can imagine myself carrying
something like this downstream if needed. Haven't we all?

It's definitely not suitable upstream, as Fabio points out, but
I don't think a devicetree approach is either.

It seems Niklas and Jacopo have been working on adding
proper support to route this, via some new ioctls.

https://patchwork.linuxtv.org/patch/55300/

Not sure what's the status of it.

Hope it helps,
Ezequiel
Lad, Prabhakar March 3, 2020, 7:01 a.m. UTC | #5 
Hi Ezequiel,

Thank you for the review.

On Tue, Mar 3, 2020 at 3:01 AM Ezequiel Garcia
<ezequiel@vanguardiasur.com.ar> wrote:
>
> Adding Niklas and Jacopo,
>
> On Mon, Mar 2, 2020, 12:33 PM Fabio Estevam <festevam@gmail.com> wrote:
> >
> > Hi Prabhakar,
> >
> > On Mon, Mar 2, 2020 at 4:19 AM Lad, Prabhakar
> > <prabhakar.csengg@gmail.com> wrote:
> >
> > > > Should this be a device tree property instead?
> > > I did give a thought about it, but making this as DT property would
> > > make it more stiff.
> >
> > In case a system has two OV5645 and we want to operate each OV5645
> > with a different virtual channel, it will not be possible with the
> > module_param approach.
> >
> > Using a device tree property would make it possible though, so I think
> > it makes more sense to use a device tree property for this.
> >
>
> As often happens, driver parameter is probably the easiest and less
> invasive way to customize a driver, so I can imagine myself carrying
> something like this downstream if needed. Haven't we all?
>
> It's definitely not suitable upstream, as Fabio points out, but
> I don't think a devicetree approach is either.
>
Agreed. I was suggesting maybe v4l2-ctl instead ?

> It seems Niklas and Jacopo have been working on adding
> proper support to route this, via some new ioctls.
>
> https://patchwork.linuxtv.org/patch/55300/
>
> Not sure what's the status of it.
>
something similar needs to be implemented for ov5645 driver.

Cheers,
--Prabhakar

> Hope it helps,
> Ezequiel
Sakari Ailus March 5, 2020, 11:43 a.m. UTC | #6 
Hi Prabhakar,

On Fri, Feb 28, 2020 at 04:41:26PM +0000, Lad Prabhakar wrote:
> OV5645 can operate in virtual channel 0-3 in CSI2 interfaces, this patch
> adds support for module parameter virtual_channel to select the required
> channel. By default OV5645 operates in virtual channel 0.

What's your use case for different virtual channels?
Lad, Prabhakar March 6, 2020, 10:18 a.m. UTC | #7 
Hi Sakari,

On Thu, Mar 5, 2020 at 11:43 AM Sakari Ailus
<sakari.ailus@linux.intel.com> wrote:
>
> Hi Prabhakar,
>
> On Fri, Feb 28, 2020 at 04:41:26PM +0000, Lad Prabhakar wrote:
> > OV5645 can operate in virtual channel 0-3 in CSI2 interfaces, this patch
> > adds support for module parameter virtual_channel to select the required
> > channel. By default OV5645 operates in virtual channel 0.
>
> What's your use case for different virtual channels?
>
Just ability to switch to different virtual channel, based on ov5640
driver. The rcar-csi2
has capability to capture  from multiple channels so that we can
capture simultaneously
from two sensors.

Cheers,
--Prabhakar

> --
> Regards,
>
> Sakari Ailus
Lad, Prabhakar March 10, 2020, 11:17 a.m. UTC | #8 
Hi Sakari,

On Fri, Mar 6, 2020 at 10:18 AM Lad, Prabhakar
<prabhakar.csengg@gmail.com> wrote:
>
> Hi Sakari,
>
> On Thu, Mar 5, 2020 at 11:43 AM Sakari Ailus
> <sakari.ailus@linux.intel.com> wrote:
> >
> > Hi Prabhakar,
> >
> > On Fri, Feb 28, 2020 at 04:41:26PM +0000, Lad Prabhakar wrote:
> > > OV5645 can operate in virtual channel 0-3 in CSI2 interfaces, this patch
> > > adds support for module parameter virtual_channel to select the required
> > > channel. By default OV5645 operates in virtual channel 0.
> >
> > What's your use case for different virtual channels?
> >
> Just ability to switch to different virtual channel, based on ov5640
> driver. The rcar-csi2
> has capability to capture  from multiple channels so that we can
> capture simultaneously
> from two sensors.
>
Any thoughts on how this could be handled ?

Cheers,
--Prabhakar
Sakari Ailus March 10, 2020, 12:43 p.m. UTC | #9 
On Tue, Mar 10, 2020 at 11:17:10AM +0000, Lad, Prabhakar wrote:
> Hi Sakari,
> 
> On Fri, Mar 6, 2020 at 10:18 AM Lad, Prabhakar
> <prabhakar.csengg@gmail.com> wrote:
> >
> > Hi Sakari,
> >
> > On Thu, Mar 5, 2020 at 11:43 AM Sakari Ailus
> > <sakari.ailus@linux.intel.com> wrote:
> > >
> > > Hi Prabhakar,
> > >
> > > On Fri, Feb 28, 2020 at 04:41:26PM +0000, Lad Prabhakar wrote:
> > > > OV5645 can operate in virtual channel 0-3 in CSI2 interfaces, this patch
> > > > adds support for module parameter virtual_channel to select the required
> > > > channel. By default OV5645 operates in virtual channel 0.
> > >
> > > What's your use case for different virtual channels?
> > >
> > Just ability to switch to different virtual channel, based on ov5640
> > driver. The rcar-csi2
> > has capability to capture  from multiple channels so that we can
> > capture simultaneously
> > from two sensors.
> >
> Any thoughts on how this could be handled ?

A module parameter to support sending the pixel data on virtual channels is
certainly a hack. You couldn't support the same kind of sensors with
different virtual channel configuration in a deterministic way nor the
receiver would have an ability to verify the hardware is properly
configured.

The multiplexed streams patchset (subject "[PATCH v3 00/31] v4l: add
support for multiplexed streams" on LMML) adds support for CSI-2 virtual
channels and data types. It's been a while since a version of that was
posted though.

Jacopo, what are your plans regarding that set?
diff mbox series

Patch

diff --git a/drivers/media/i2c/ov5645.c b/drivers/media/i2c/ov5645.c
index a6c17d15d754..0a0671164623 100644
--- a/drivers/media/i2c/ov5645.c
+++ b/drivers/media/i2c/ov5645.c
@@ -54,6 +54,7 @@ 
 #define OV5645_TIMING_TC_REG21		0x3821
 #define		OV5645_SENSOR_MIRROR		BIT(1)
 #define OV5645_MIPI_CTRL00		0x4800
+#define OV5645_REG_DEBUG_MODE		0x4814
 #define OV5645_PRE_ISP_TEST_SETTING_1	0x503d
 #define		OV5645_TEST_PATTERN_MASK	0x3
 #define		OV5645_SET_TEST_PATTERN(x)	((x) & OV5645_TEST_PATTERN_MASK)
@@ -61,6 +62,11 @@ 
 #define OV5645_SDE_SAT_U		0x5583
 #define OV5645_SDE_SAT_V		0x5584
 
+static u8 virtual_channel;
+module_param(virtual_channel, byte, 0644);
+MODULE_PARM_DESC(virtual_channel,
+		 "MIPI CSI-2 virtual channel (0..3), default 0");
+
 /* regulator supplies */
 static const char * const ov5645_supply_name[] = {
 	"vdddo", /* Digital I/O (1.8V) supply */
@@ -983,12 +989,34 @@  static int ov5645_get_selection(struct v4l2_subdev *sd,
 	return 0;
 }
 
+static int ov5645_set_virtual_channel(struct ov5645 *ov5645)
+{
+	u8 temp, channel = virtual_channel;
+	int ret;
+
+	if (channel > 3)
+		return -EINVAL;
+
+	ret = ov5645_read_reg(ov5645, OV5645_REG_DEBUG_MODE, &temp);
+	if (ret)
+		return ret;
+
+	temp &= ~(3 << 6);
+	temp |= (channel << 6);
+
+	return ov5645_write_reg(ov5645, OV5645_REG_DEBUG_MODE, temp);
+}
+
 static int ov5645_s_stream(struct v4l2_subdev *subdev, int enable)
 {
 	struct ov5645 *ov5645 = to_ov5645(subdev);
 	int ret;
 
 	if (enable) {
+		ret = ov5645_set_virtual_channel(ov5645);
+		if (ret < 0)
+			return ret;
+
 		ret = ov5645_set_register_array(ov5645,
 					ov5645->current_mode->data,
 					ov5645->current_mode->data_size);