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>]

[-- Attachment #1: Type: text/plain, Size: 4310 bytes --]

Hi Daniel,

On 11/2/21 00:43, Daniel Scally wrote:
> Hi Hans
> 
> On 01/11/2021 16:02, Hans de Goede wrote:

<snip>

>>> Having looked at this yesterday evening I'm more and more convinced it's
>>> necessary. I hacked it into the ov8865 driver in the interim (just by
>>> calling i2c_acpi_new_device() in probe) and then worked on that dw9719
>>> code you found [1] to turn it into an i2c driver (attached, though still
>>> needs a bit of work), which will successfully bind to the i2c client
>>> enumerated by that i2c_acpi_new_device() call. From there though it
>>> needs a way for the v4l2 subdev to be matched to the sensor's subdev.
>>> This can happen automatically by way of the lens-focus firmware property
>>> against the sensor - we currently build those in the cio2-bridge, so
>>> adding another software node for the VCM and creating a lens-focus
>>> property for the sensor's software_node with a pointer to the VCM's node
>>> seems like the best way to do that.
>> So besides prepping a v5 of my previous series, with update regulator
>> init-data for the VCM I've also been looking into this, attached are
>> the results.
>>
>> Some notes from initial testing:
>>
>> 1. The driver you attached will only successful probe if I insmod
>> it while streaming video from the sensor. So I think we need another
>> regulator or the clk for just the VCM too, I will investigate this
>> later this week.
> 
> Oh really, I'll test that too; thanks for the patches. There's a couple
> of tweaks to the driver anyway, so hopefully be able to get it ironed out.

Ok, I've figured this out now, with the attached patch (which also
explains what is going on) as well as an updated tps68470_board_data.c
with updated regulator_init_data for the VCM (also attached), the driver
can now successfully talk to the VCM in probe() while we are NOT
streaming from the ov8865.

Daniel, please feel free to squash this into your original dw9719 patch.

<snip>

>> 2. I need some help with all the fwnode link stuff (I'm not very familiar
>> with this). There seems to be a chicken and egg problem here though,
>> because the v4l2subdev for the VCM does not register because of async stuff
>> and if we add it to the "graph" then my idea to enumerate the VCMs
>> from the SSDB on the complete() callback won't work. But we can do this
>> on a per sensor basis instead from the cio2_notifier_bound() callback
>> instead I guess ?
> 
> 
> I think on top of your work in the cio2-bridge for patch 3 you can do this:
> 
> 
> 1. Create another software node against the cio2_sensor struct, with the
> name coming from the vcm_types array
> 
> 2. Assign that software node to board_info.swnode in
> cio2_bridge_instantiate_vcm_i2c_client()
> 
> 3. Add another entry to dev_properties for the sensor, that is named
> "lens-focus" and contains a reference to the software_node created in #2
> just like the references to the sensor/cio2 nodes.
> 
> 
> This way when the sensor driver calls
> v4l2_async_register_subdev_sensor() it should create a notifier that
> looks for that VCM client to bind. I think then rather than putting
> anything in the .bound() / .complete() callbacks, we should modify core
> to do _something_ when async matching some subdevs. The something would
> depend on the kind of devices that match, for example with the sensor
> driver and the ipu3-cio2 driver, there's an entity whos function is
> MEDIA_ENT_F_VID_IF_BRIDGE matching to an entity whos function is
> MEDIA_ENT_F_CAM_SENSOR, and it seems to me that every scenario like that
> is going to result in media pad links being created. Similarly for our
> sensor that's a device with entity function MEDIA_ENT_F_LENS matching to
> MEDIA_ENT_F_CAM_SENSOR, and I think that in those cases we can create
> either an interface link or a new kind of link (maybe
> "MEDIA_LNK_FL_ANCILLARY_LINK" or something...) between the two to show
> that they form a single logical unit, which we can then report to libcamera.
> 
> 
> Hope that makes sense...

Maybe? I have not looked into this closely yet. I'll continue working on
this coming Tuesday.

If you feel like tinkering I would not mind if you beat me to it this
weekend :)   OTOH please enjoy your weekend doing whatever, I can continue
working on this during office-hours next week.

Regards,

Hans

[-- Attachment #2: 0001-media-i2c-dw9719-Add-support-for-VSIO-regulator.patch --]
[-- Type: text/x-patch, Size: 2944 bytes --]

From 72760db407b98e02d5aea5ad6c1a1f4ebd668717 Mon Sep 17 00:00:00 2001
From: Hans de Goede <hdegoede@redhat.com>
Date: Thu, 4 Nov 2021 15:41:29 +0100
Subject: [PATCH] media: i2c: dw9719: Add support for VSIO regulator

The DW9719 has only the 1 VDD voltage input, but some PMICs such as
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
enable VSIO as that will also enable the I2C passthrough.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/media/i2c/dw9719.c | 27 +++++++++++++++++++--------
 1 file changed, 19 insertions(+), 8 deletions(-)

diff --git a/drivers/media/i2c/dw9719.c b/drivers/media/i2c/dw9719.c
index 9021caa915a3..78b712f5726f 100644
--- a/drivers/media/i2c/dw9719.c
+++ b/drivers/media/i2c/dw9719.c
@@ -27,12 +27,14 @@
 #define DW9719_DEFAULT_VCM_FREQ		0x60
 #define DW9719_ENABLE_RINGING		0x02
 
+#define NUM_REGULATORS			2
+
 #define to_dw9719_device(x) container_of(x, struct dw9719_device, sd)
 
 struct dw9719_device {
 	struct device *dev;
 	struct i2c_client *client;
-	struct regulator *vdd;
+	struct regulator_bulk_data regulators[NUM_REGULATORS];
 	struct v4l2_subdev sd;
 
 	struct dw9719_v4l2_ctrls {
@@ -115,14 +117,14 @@ static int dw9719_detect(struct dw9719_device *dw9719)
 
 static int dw9719_power_down(struct dw9719_device *dw9719)
 {
-	return regulator_disable(dw9719->vdd);
+	return regulator_bulk_disable(NUM_REGULATORS, dw9719->regulators);
 }
 
 static int dw9719_power_up(struct dw9719_device *dw9719)
 {
 	int ret;
 
-	ret = regulator_enable(dw9719->vdd);
+	ret = regulator_bulk_enable(NUM_REGULATORS, dw9719->regulators);
 	if (ret)
 		return ret;
 
@@ -276,11 +278,20 @@ static int dw9719_probe(struct i2c_client *client)
 	dw9719->client = client;
 	dw9719->dev = &client->dev;
 
-	dw9719->vdd = devm_regulator_get(&client->dev, "vdd");
-	if (IS_ERR(dw9719->vdd)) {
-		dev_err(&client->dev, "Error getting regulator\n");
-		return PTR_ERR(dw9719->vdd);
-	}
+	dw9719->regulators[0].supply = "vdd";
+	/*
+	 * The DW9719 has only the 1 VDD voltage input, but some PMICs such as
+	 * 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
+	 * enable VSIO as that will also enable the I2C passthrough.
+	 */
+	dw9719->regulators[1].supply = "vsio";
+
+	ret = devm_regulator_bulk_get(&client->dev, NUM_REGULATORS, dw9719->regulators);
+	if (ret)
+		return dev_err_probe(&client->dev, ret, "getting regulators\n");
 
 	v4l2_i2c_subdev_init(&dw9719->sd, client, &dw9719_ops);
 	dw9719->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
-- 
2.31.1


[-- Attachment #3: tps68470_board_data.c --]
[-- Type: text/x-csrc, Size: 4028 bytes --]

// SPDX-License-Identifier: GPL-2.0
/*
 * TI TPS68470 PMIC platform data definition.
 *
 * Copyright (c) 2021 Dan Scally <djrscally@gmail.com>
 * Copyright (c) 2021 Red Hat Inc.
 *
 * Red Hat authors:
 * Hans de Goede <hdegoede@redhat.com>
 */

#include <linux/dmi.h>
#include <linux/gpio/machine.h>
#include <linux/platform_data/tps68470.h>
#include <linux/regulator/machine.h>
#include "tps68470.h"

static struct regulator_consumer_supply int347a_core_consumer_supplies[] = {
	REGULATOR_SUPPLY("dvdd", "i2c-INT347A:00"),
};

static struct regulator_consumer_supply int347a_ana_consumer_supplies[] = {
	REGULATOR_SUPPLY("avdd", "i2c-INT347A:00"),
};

static struct regulator_consumer_supply int347a_vcm_consumer_supplies[] = {
	REGULATOR_SUPPLY("vdd", "i2c-INT347A:00-VCM"),
};

static struct regulator_consumer_supply int347a_vsio_consumer_supplies[] = {
	REGULATOR_SUPPLY("dovdd", "i2c-INT347A:00"),
	REGULATOR_SUPPLY("vsio", "i2c-INT347A:00-VCM"),
};

static const struct regulator_init_data surface_go_tps68470_core_reg_init_data = {
	.constraints = {
		.min_uV = 1200000,
		.max_uV = 1200000,
		.apply_uV = 1,
		.valid_ops_mask = REGULATOR_CHANGE_STATUS,
	},
	.num_consumer_supplies = ARRAY_SIZE(int347a_core_consumer_supplies),
	.consumer_supplies = int347a_core_consumer_supplies,
};

static const struct regulator_init_data surface_go_tps68470_ana_reg_init_data = {
	.constraints = {
		.min_uV = 2815200,
		.max_uV = 2815200,
		.apply_uV = 1,
		.valid_ops_mask = REGULATOR_CHANGE_STATUS,
	},
	.num_consumer_supplies = ARRAY_SIZE(int347a_ana_consumer_supplies),
	.consumer_supplies = int347a_ana_consumer_supplies,
};

static const struct regulator_init_data surface_go_tps68470_vcm_reg_init_data = {
	.constraints = {
		.min_uV = 2815200,
		.max_uV = 2815200,
		.apply_uV = 1,
		.valid_ops_mask = REGULATOR_CHANGE_STATUS,
	},
	.num_consumer_supplies = ARRAY_SIZE(int347a_vcm_consumer_supplies),
	.consumer_supplies = int347a_vcm_consumer_supplies,
};

static const struct regulator_init_data surface_go_tps68470_vsio_reg_init_data = {
	.constraints = {
		.min_uV = 1800600,
		.max_uV = 1800600,
		.apply_uV = 1,
		.valid_ops_mask = REGULATOR_CHANGE_STATUS,
	},
	.num_consumer_supplies = ARRAY_SIZE(int347a_vsio_consumer_supplies),
	.consumer_supplies = int347a_vsio_consumer_supplies,
};

static const struct tps68470_regulator_platform_data surface_go_tps68470_pdata = {
	.reg_init_data = {
		[TPS68470_CORE] = &surface_go_tps68470_core_reg_init_data,
		[TPS68470_ANA]  = &surface_go_tps68470_ana_reg_init_data,
		[TPS68470_VCM]  = &surface_go_tps68470_vcm_reg_init_data,
		[TPS68470_VSIO] = &surface_go_tps68470_vsio_reg_init_data,
	},
};

static struct gpiod_lookup_table surface_go_tps68470_gpios = {
	.dev_id = "i2c-INT347A:00",
	.table = {
		GPIO_LOOKUP("tps68470-gpio", 9, "reset", GPIO_ACTIVE_LOW),
		GPIO_LOOKUP("tps68470-gpio", 7, "powerdown", GPIO_ACTIVE_LOW)
	}
};

static const struct int3472_tps68470_board_data surface_go_tps68470_board_data = {
	.dev_name = "i2c-INT3472:05",
	.tps68470_gpio_lookup_table = &surface_go_tps68470_gpios,
	.tps68470_regulator_pdata = &surface_go_tps68470_pdata,
};

static const struct dmi_system_id int3472_tps68470_board_data_table[] = {
	{
		.matches = {
			DMI_EXACT_MATCH(DMI_SYS_VENDOR, "Microsoft Corporation"),
			DMI_EXACT_MATCH(DMI_PRODUCT_NAME, "Surface Go"),
		},
		.driver_data = (void *)&surface_go_tps68470_board_data,
	},
	{
		.matches = {
			DMI_EXACT_MATCH(DMI_SYS_VENDOR, "Microsoft Corporation"),
			DMI_EXACT_MATCH(DMI_PRODUCT_NAME, "Surface Go 2"),
		},
		.driver_data = (void *)&surface_go_tps68470_board_data,
	},
	{ }
};

const struct int3472_tps68470_board_data *int3472_tps68470_get_board_data(const char *dev_name)
{
	const struct int3472_tps68470_board_data *board_data;
	const struct dmi_system_id *match;

	match = dmi_first_match(int3472_tps68470_board_data_table);
	while (match) {
		board_data = match->driver_data;
		if (strcmp(board_data->dev_name, dev_name) == 0)
			return board_data;

		dmi_first_match(++match);
	}

	return NULL;
}