[PATCH] media: ipu3-cio2: support more camera sensors in cio2-bridge

[PATCH] media: ipu3-cio2: support more camera sensors in cio2-bridge

[bingbu.cao]

From: Bingbu Cao <bingbu.cao@intel.com>

Add more camera sensors into the supported camera sensors list
to make cio2-bridge to support more camera sensors.

Signed-off-by: Bingbu Cao <bingbu.cao@intel.com>
---
 drivers/media/pci/intel/ipu3/cio2-bridge.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/drivers/media/pci/intel/ipu3/cio2-bridge.c b/drivers/media/pci/intel/ipu3/cio2-bridge.c
index dfefe0d8aa95..9e6046fa0bd0 100644
--- a/drivers/media/pci/intel/ipu3/cio2-bridge.c
+++ b/drivers/media/pci/intel/ipu3/cio2-bridge.c
@@ -29,6 +29,25 @@ static const struct cio2_sensor_config cio2_supported_sensors[] = {
 	CIO2_SENSOR_CONFIG("INT347E", 1, 319200000),
 	/* Omnivision OV2680 */
 	CIO2_SENSOR_CONFIG("OVTI2680", 0),
+	/* Omnivision ov02c10 */
+	CIO2_SENSOR_CONFIG("OVTI02C1", 0, 0),
+	/* Omnivision ov01a10 */
+	CIO2_SENSOR_CONFIG("OVTI01A0", 0, 0),
+	/* Omnivision ov01a1s */
+	CIO2_SENSOR_CONFIG("OVTI01AS", 0, 0),
+	/* Omnivision ov8856 */
+	CIO2_SENSOR_CONFIG("OVTI8856", 0, 0),
+	/* Omnivision ov2740 */
+	CIO2_SENSOR_CONFIG("INT3474", 0, 0),
+	/* Hynix hi556 */
+	CIO2_SENSOR_CONFIG("INT3537", 0, 0),
+	/* Himax hm2170 */
+	CIO2_SENSOR_CONFIG("HIMX2170", 0, 0),
+	/* Himax hm11b1 */
+	CIO2_SENSOR_CONFIG("HIMX11B1", 0, 0),
+	/* Omnivision ov13b10 */
+	CIO2_SENSOR_CONFIG("OVTIDB10", 0, 0),
+	CIO2_SENSOR_CONFIG("OVTI13B1", 0, 0),
 };
 
 static const struct cio2_property_names prop_names = {
-- 
2.39.1

Re: [PATCH] media: ipu3-cio2: support more camera sensors in cio2-bridge

[Dan Scally]

Hi Bingbu, thanks for the patch

On 03/03/2023 14:36, bingbu.cao@intel.com wrote:
> From: Bingbu Cao <bingbu.cao@intel.com>
>
> Add more camera sensors into the supported camera sensors list
> to make cio2-bridge to support more camera sensors.
>
> Signed-off-by: Bingbu Cao <bingbu.cao@intel.com>
> ---
>   drivers/media/pci/intel/ipu3/cio2-bridge.c | 19 +++++++++++++++++++
>   1 file changed, 19 insertions(+)
>
> diff --git a/drivers/media/pci/intel/ipu3/cio2-bridge.c b/drivers/media/pci/intel/ipu3/cio2-bridge.c
> index dfefe0d8aa95..9e6046fa0bd0 100644
> --- a/drivers/media/pci/intel/ipu3/cio2-bridge.c
> +++ b/drivers/media/pci/intel/ipu3/cio2-bridge.c
> @@ -29,6 +29,25 @@ static const struct cio2_sensor_config cio2_supported_sensors[] = {
>   	CIO2_SENSOR_CONFIG("INT347E", 1, 319200000),
>   	/* Omnivision OV2680 */
>   	CIO2_SENSOR_CONFIG("OVTI2680", 0),
> +	/* Omnivision ov02c10 */
> +	CIO2_SENSOR_CONFIG("OVTI02C1", 0, 0),


I don't see a driver that supports this HID. I think unless there's an actual driver that supports 
the sensor, adding an entry here is a bad idea. The problem with it is that the ipu3-cio2 driver 
will add async subdevs for each sensor that it finds listed here, and if a matching driver doesn't 
probe for that subdev then the .bound() callback for the notifier registered by ipu3-cio2 will never 
run, which means media links and v4l2 devnodes never get created, which means _none_ of the cameras 
in the laptop will work even if the other ones do have working drivers.


It's quite annoying because there's a good long list of sensors this _can_ support but until we fix 
the notifier's weirdness I think this list needs to be limited to entries that are supported by 
in-tree drivers.

> +	/* Omnivision ov01a10 */
> +	CIO2_SENSOR_CONFIG("OVTI01A0", 0, 0),
> +	/* Omnivision ov01a1s */
> +	CIO2_SENSOR_CONFIG("OVTI01AS", 0, 0),
> +	/* Omnivision ov8856 */
> +	CIO2_SENSOR_CONFIG("OVTI8856", 0, 0),


Similarly, although there is a driver that matches to OVTI8856 that driver checks what 
link-frequencies are reported by firmware and then fails out if it finds no entries or if none of 
the entries match the ones it knows how to configure [1]. That will again cause any sensors with 
fully functional drivers to also fail, because the ipu3-cio2's notifier will never see its 
.complete() callback run and so the links and devnodes don't get set up. So I think that you need to 
detail the link-frequencies here to make sure that the driver can find them.


[1] https://elixir.bootlin.com/linux/latest/source/drivers/media/i2c/ov8856.c#L2420

> +	/* Omnivision ov2740 */
> +	CIO2_SENSOR_CONFIG("INT3474", 0, 0),
> +	/* Hynix hi556 */
> +	CIO2_SENSOR_CONFIG("INT3537", 0, 0),
> +	/* Himax hm2170 */
> +	CIO2_SENSOR_CONFIG("HIMX2170", 0, 0),
> +	/* Himax hm11b1 */
> +	CIO2_SENSOR_CONFIG("HIMX11B1", 0, 0),
> +	/* Omnivision ov13b10 */
> +	CIO2_SENSOR_CONFIG("OVTIDB10", 0, 0),
> +	CIO2_SENSOR_CONFIG("OVTI13B1", 0, 0),
>   };
>   
>   static const struct cio2_property_names prop_names = {

RE: [PATCH] media: ipu3-cio2: support more camera sensors in cio2-bridge

[Cao, Bingbu]

Dan,

Thanks for your review.

------------------------------------------------------------------------
BRs,  
Intel VTG – Linux & Chrome IPU SW
Bingbu Cao 

> -----Original Message-----
> From: Dan Scally <dan.scally@ideasonboard.com>
> Sent: Friday, March 3, 2023 23:53
> To: Cao, Bingbu <bingbu.cao@intel.com>; linux-media@vger.kernel.org;
> sakari.ailus@linux.intel.com; andriy.shevchenko@linux.intel.com;
> djrscally@gmail.com
> Cc: bingbu.cao@linux.intel.com
> Subject: Re: [PATCH] media: ipu3-cio2: support more camera sensors in
> cio2-bridge
> 
> Hi Bingbu, thanks for the patch
> 
> On 03/03/2023 14:36, bingbu.cao@intel.com wrote:
> > From: Bingbu Cao <bingbu.cao@intel.com>
> >
> > Add more camera sensors into the supported camera sensors list to make
> > cio2-bridge to support more camera sensors.
> >
> > Signed-off-by: Bingbu Cao <bingbu.cao@intel.com>
> > ---
> >   drivers/media/pci/intel/ipu3/cio2-bridge.c | 19 +++++++++++++++++++
> >   1 file changed, 19 insertions(+)
> >
> > diff --git a/drivers/media/pci/intel/ipu3/cio2-bridge.c
> > b/drivers/media/pci/intel/ipu3/cio2-bridge.c
> > index dfefe0d8aa95..9e6046fa0bd0 100644
> > --- a/drivers/media/pci/intel/ipu3/cio2-bridge.c
> > +++ b/drivers/media/pci/intel/ipu3/cio2-bridge.c
> > @@ -29,6 +29,25 @@ static const struct cio2_sensor_config
> cio2_supported_sensors[] = {
> >   	CIO2_SENSOR_CONFIG("INT347E", 1, 319200000),
> >   	/* Omnivision OV2680 */
> >   	CIO2_SENSOR_CONFIG("OVTI2680", 0),
> > +	/* Omnivision ov02c10 */
> > +	CIO2_SENSOR_CONFIG("OVTI02C1", 0, 0),
> 
> 
> I don't see a driver that supports this HID. I think unless there's an
> actual driver that supports the sensor, adding an entry here is a bad idea.
> The problem with it is that the ipu3-cio2 driver will add async subdevs
> for each sensor that it finds listed here, and if a matching driver
> doesn't probe for that subdev then the .bound() callback for the notifier
> registered by ipu3-cio2 will never run, which means media links and v4l2
> devnodes never get created, which means _none_ of the cameras in the
> laptop will work even if the other ones do have working drivers.

Ack. I do forget that these device drivers which are not available
in current tree, will try to upstream the sensor driver firstly, also for
hm11b1 and hm2170.

> 
> 
> It's quite annoying because there's a good long list of sensors this _can_
> support but until we fix the notifier's weirdness I think this list needs
> to be limited to entries that are supported by in-tree drivers.
> 
> > +	/* Omnivision ov01a10 */
> > +	CIO2_SENSOR_CONFIG("OVTI01A0", 0, 0),
> > +	/* Omnivision ov01a1s */
> > +	CIO2_SENSOR_CONFIG("OVTI01AS", 0, 0),
> > +	/* Omnivision ov8856 */
> > +	CIO2_SENSOR_CONFIG("OVTI8856", 0, 0),
> 
> 
> Similarly, although there is a driver that matches to OVTI8856 that driver
> checks what link-frequencies are reported by firmware and then fails out
> if it finds no entries or if none of the entries match the ones it knows
> how to configure [1]. That will again cause any sensors with fully
> functional drivers to also fail, because the ipu3-cio2's notifier will
> never see its
> .complete() callback run and so the links and devnodes don't get set up.
> So I think that you need to detail the link-frequencies here to make sure
> that the driver can find them.
> 
> 
> [1]
> https://elixir.bootlin.com/linux/latest/source/drivers/media/i2c/ov8856.c#
> L2420

Thanks, let me address it.

> 
> > +	/* Omnivision ov2740 */
> > +	CIO2_SENSOR_CONFIG("INT3474", 0, 0),
> > +	/* Hynix hi556 */
> > +	CIO2_SENSOR_CONFIG("INT3537", 0, 0),
> > +	/* Himax hm2170 */
> > +	CIO2_SENSOR_CONFIG("HIMX2170", 0, 0),
> > +	/* Himax hm11b1 */
> > +	CIO2_SENSOR_CONFIG("HIMX11B1", 0, 0),
> > +	/* Omnivision ov13b10 */
> > +	CIO2_SENSOR_CONFIG("OVTIDB10", 0, 0),
> > +	CIO2_SENSOR_CONFIG("OVTI13B1", 0, 0),
> >   };
> >
> >   static const struct cio2_property_names prop_names = {