RE: [PATCH v2 0/3] media: pci: intel: ivsc: Add driver of Intel Visual Sensing Controller(IVSC)

[Wu, Wentong]

> -----Original Message-----
> From: Sakari Ailus <sakari.ailus@xxxxxxxxxxxxxxx>
> Sent: Friday, March 17, 2023 4:59 PM
> 
> Hi Wentong,
> 
> On Fri, Mar 17, 2023 at 07:30:19AM +0000, Wu, Wentong wrote:
> >
> >
> > > -----Original Message-----
> > > From: Hans de Goede <hdegoede@xxxxxxxxxx>
> > > Sent: Thursday, March 16, 2023 5:04 PM
> > >
> > > Hi,
> > >
> > > On 3/16/23 03:58, Wu, Wentong wrote:
> > > >
> > > >
> > > >> -----Original Message-----
> > > >> From: Hans de Goede <hdegoede@xxxxxxxxxx>
> > > >> Sent: Thursday, March 9, 2023 11:24 PM
> > > >>
> > > >> <re-added the previous Cc list, which I dropped because of the
> > > >> large
> > > >> attachments>
> > > >>
> > > >> Hi Wentong,
> > > >>
> > > >> On 3/9/23 15:29, Wu, Wentong wrote:
> > > >>> Hi Hans,
> > > >>>
> > > >>> Thanks
> > > >>>
> > > >>> And AFAICT, there is no IVSC device on your Dell Latitude 9420
> > > >>> where the
> > > >> platform is based on TGL instead of ADL, and I have never heard
> > > >> IVSC runs on TGL,  if no IVSC, INT3472 will control sensor's power.
> > > >>> And I will double confirm with people who know dell product well
> tomorrow.
> > > >>
> > > >> Ah, I was under the impression that there was an IVSC there because:
> > > >>
> > > >> 1. The sensor driver for the used sensor (tries to) poke the IVSC 2.
> > > >> Things did not work without building the IVSC drivers, but that might
> > > >>    be due to a dependency on the LCJA GPIO expander instead
> > > >
> > > > Below is your dmesg log, the required SPI controller for IVSC isn't here.
> > > >
> > > > [   35.538114] ljca 2-6:1.0: acked sem wait timed out ret:0 timeout:20
> ack:0
> > > > [   35.538129] ljca 2-6:1.0: MNG_ENUM_SPI failed ret:-110 len:7 num:0
> > > > [   35.538621] ljca 2-6:1.0: LJCA USB device init success
> > > > [   35.538776] usbcore: registered new interface driver ljca
> > > >
> > > > Also I checked your SSDT, there is no IVSC device and the sensor
> > > > device depends on
> > > > INT3472 instead of IVSC device as on my setup.
> > >
> > > Ack.
> > >
> > > >> But you might very well be right, that would also explain the
> > > >> "intel vsc not
> > > ready"
> > > >> messages in dmesg.
> > > >>
> > > >> If with the IVSC case the IVSC controls the power to the sensor
> > > >> too, then another option might be to model the I2C-switch + the
> > > >> power-control as a powerdown GPIO for the sensor, which most
> > > >> sensor
> > > drivers already try to use.
> > > >> The advantage of doing this would be that GPIO lookups can
> > > >> reference the GPIO provider + consumer by device-name so then we
> > > >> don't need to have both devices instantiated at the time of
> > > >> adding the GPIO lookup.   And in that case we could e.g. add the lookup
> > > >> before registering the I2C controller.
> > > >
> > > > Can we add IVSC device to acpi_honor_dep_ids, so that when
> > > > everything is done during mei_ace probe,
> > > > acpi_dev_clear_dependencies can make sensor
> > > start probe?
> > >
> > > Does the sensor ACPI device node have an ACPI _DEP on the IVSC device ?
> >
> > Yes,
> >
> > >
> > > If yes, then yes we can add the IVSC device to acpi_honor_dep_id and
> > > make mei_ace probe call acpi_dev_clear_dependencies().
> >
> > But I prefer the powerdown gpio model, because we have to follow the
> > commands sequences as below which is required by firmware, runtime pm is
> hard to achieve this.
> 
> How so?

But we have to find a way to download commands to firmware following below
sequence before writing camera sensor's registers to start camera sensor's steam.

BR,
Wentong

> 
> I don't insist on the runtime PM based solution but I'd rather not have changes
> to virtually all sensor drivers --- this is an external chip to them.
> 
> > +	/* switch camera sensor ownership to host */
> > +	ret = ace_set_camera_owner(ACE_CAMERA_HOST);
> > +	if (ret)
> > +		goto error;
> > +
> > +	/* switch CSI-2 link to host */
> > +	ret = csi_set_link_owner(CSI_LINK_HOST, callback, context);
> > +	if (ret)
> > +		goto release_camera;
> > +
> > +	/* configure CSI-2 link */
> > +	ret = csi_set_link_cfg(nr_of_lanes, link_freq);
> > +	if (ret)
> > +		goto release_csi;
> 
> --
> Kind regards,
> 
> Sakari Ailus