Dan,
Thanks for your review.
------------------------------------------------------------------------
BRs,
Intel VTG – Linux & Chrome IPU SW
Bingbu Cao
> -----Original Message-----
> From: Dan Scally <dan.scally@xxxxxxxxxxxxxxxx>
> Sent: Friday, March 3, 2023 23:53
> To: Cao, Bingbu <bingbu.cao@xxxxxxxxx>; linux-media@xxxxxxxxxxxxxxx;
> sakari.ailus@xxxxxxxxxxxxxxx; andriy.shevchenko@xxxxxxxxxxxxxxx;
> djrscally@xxxxxxxxx
> Cc: bingbu.cao@xxxxxxxxxxxxxxx
> 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@xxxxxxxxx wrote:
> > From: Bingbu Cao <bingbu.cao@xxxxxxxxx>
> >
> > 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@xxxxxxxxx>
> > ---
> > 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 = {
