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@xxxxxxxxxx> 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
