| Message ID | 20200228164126.17517-1-prabhakar.mahadev-lad.rj@bp.renesas.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | media: i2c: ov5645: Add virtual_channel module parameter | expand |
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?
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
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
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
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
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?
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
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
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 --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);
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(+)