Re: Fwd: Surface Go VCM type (was: Need to pass acpi_enforce_resources=lax on the Surface Go (version1))

[Hans de Goede <hdegoede@redhat.com>]

Hi,

On 11/10/21 01:01, Daniel Scally wrote:
> Hi Hans
> 
> On 09/11/2021 16:35, Daniel Scally wrote:
>>>>> That's not working correctly for me at the moment, but I think this is a
>>>>> surmountable problem rather than the wrong approach, so I'm just working
>>>>> through the differences to try and get the matching working.
>>>> OK, I eventually got this working - the dw9719 registers as
>>>> /dev/v4l-subdev7 for me now ... long story short is the attached patch
>>>> was needed to make the references work, as the internals of v4l2 aren't
>>>> checking for fwnode->secondary. Prior to your latest series as well, an
>>>> additional problem was that once the VCMs fwnode was linked to the
>>>> sensor's the .complete() callback for ipu3-cio2 would never call
>>>> (because it needs ALL the devices for the linked fwnodes to be bound to
>>>> do that)...which meant the VCMs never got instantiated, because that was
>>>> where that function was called. With your new set separating those
>>>> processes it works well, so yes I like that new approach very much :D
>>>>
>>>>
>>>> In the end we don't have to add a call creating the subdev's - it turns
>>>> out that v4l2 knows it's part of ipu3-cio2's v4l2-device so it registers
>>>> the nodes for the vcm when .complete() is called for that driver. I
>>>> still think we should add a bit creating the link to expose to userspace
>>>> in match_notify() though.
>>>>
>>>>
>>>> Trying to list controls for the dw9719 with v4l2-ctl -d /dev/v4l-subdev7
>>>> -L fails with an IOCTL error, so I have some remedial work on the driver
>>>> which I'll do tonight; I'd expect to be able to control focus with
>>>> v4l2-ctl -d /dev/v4l-subdev7 -c absolute_focus=n once this is sorted.
>>> That is great, thank you so much. I wanted to look into this myself
>>> today but I got distracted by other stuff.
>>
>> No problem; I'll link you the patches for the updated versions of
>> everything once I've sorted the IOCTL error tonight.
> 
> 
> OK, this is running now. With the attached patches on top of your v5
> series and the 4-patch series from earlier today, the dw9719 registers
> as a v4l2 subdev and I can control it with v4l2-ctl -d /dev/v4l-subdev7
> -c focus_absolute=1200 (or whatever value).

Great, thank you! I've given this a quick test and indeed everything
works :)

I did notice a typo in a comment in the dw9719.c file which I added
myself, can you squash in this fix pleas? :

diff --git a/drivers/media/i2c/dw9719.c b/drivers/media/i2c/dw9719.c
index 047f7636efde..c647b50c2ebf 100644
--- a/drivers/media/i2c/dw9719.c
+++ b/drivers/media/i2c/dw9719.c
@@ -283,7 +283,7 @@ static int dw9719_probe(struct i2c_client *client)
 	 * the TPS68470 PMIC have I2C passthrough capability, to disconnect the
 	 * sensor's I2C pins from the I2C bus when the sensors VSIO (Sensor-IO)
 	 * is off, because some sensors then short these pins to ground;
-	 * and the DW9719 might sit behind this passthrough, this it needs to
+	 * and the DW9719 might sit behind this passthrough, thus it needs to
 	 * enable VSIO as that will also enable the I2C passthrough.
 	 */
 	dw9719->regulators[1].supply = "vsio";

Also I think that the 

"device property: Check fwnode->secondary when finding properties"

That patch looks good to me, so please add my:

Reviewed-by: Hans de Goede <hdegoede@redhat.com>

Can you submit this upstream please?

I will prepare a new version of my:

"[PATCH v5 00/11] Add support for X86/ACPI camera sensor/PMIC setup with clk and regulator platform data"

series, addressing the few remaining comments and adding the regulator
data + instantiating support for the VCM.

> One problem I'm experiencing
> is that the focus position I set isn't maintained; it holds for a couple
> of seconds and then resets to the "normal" focus...this happens when the
> .close() callback for the driver is called, which happens right after
> the control value is applied. All the other VCM drivers in the kernel
> power down on .close() so I did the same>

Right, I believe that this is fine though, we expect people to use
libcamera with this and once libcamera gets autofocus support, then
I would expect libcamera to keep the fd open the entire time while
streaming.

>, but the behaviour is not
> particularly useful - since removing the power seems to reset it, it
> needs to be on whilst the linked sensor is streaming I suppose. Given
> that ascertaining the state of the sensor probably will require some
> link established between them anyway I guess I will look at that next,
> unless you'd rather do it?

I don't think this is necessary, see above.

What is necessary is some way for libcamera to:

1. See if there is a VCM which belongs to the sensor; and
2. If there is a VCM figure out which v4l2-subdev it is.

Also see this email thread, where Hans Verkuil came to the
conclusion that this info is currently missing from the MC
representation (link is to the conclusion):

https://lists.libcamera.org/pipermail/libcamera-devel/2021-October/026144.html

Regards,

Hans