[PATCH 0/4] platform/x86: int3472: discrete: Regulator rework / Lenovo Miix 510 support

[PATCH 0/4] platform/x86: int3472: discrete: Regulator rework / Lenovo Miix 510 support

[Hans de Goede]

Hi Dan, Hao Yao and Bingbu Cao,

Patches 1/2 drop the sensor-config stuff since I thought we should be
able to make things work without any board specific fixups.

This is the result of my working on getting IPU6 to work on Jasper Lake
for $dayjob and then tonight I switched to trying to get the ov2680
on the Lenovo Miix 510 to work and it turns out that does require some
board specific workarounds after all :|

With this series together with my recent ov2680 sensor driver series:
https://lore.kernel.org/linux-media/20230607164712.63579-1-hdegoede@redhat.com/
I can get the ov2680 driver to load and successfully read the id register:

[   11.365319] ipu3-cio2 0000:00:14.3: Found supported sensor OVTI2680:00
[   11.431595] ov2680 i2c-OVTI2680:00: supply DOVDD not found, using dummy regulator
[   11.433125] ov2680 i2c-OVTI2680:00: supply DVDD not found, using dummy regulator
[   11.454698] ov2680 i2c-OVTI2680:00: sensor_revision id = 0x2680, rev= 0

Dan, currently the DMI match used only matches the 12IKB version of
the Miix 510 I think you have a 12ISK version. Can you verify this
works there too?  I guess we can just drop the KB part of the DMI
match if this works on the 12ISK version too.

Hao Yao and Bingbu Cao I think that the way the issue with how different
drivers may expect different regulator supply-ids is of interest to you
too. Note I see that the mainline version of ov13b10.c does not have
regulator support at all yet. So when adding this please just use
one of the existing set of supply-names + the bulk API like how
the ov5693.c driver is doing. In this case no int3472 driver changes
will be necessary at all.

Regards,

Hans


Hans de Goede (4):
  platform/x86: int3472: discrete: Drop GPIO remapping support
  platform/x86: int3472: discrete: Remove sensor_config-s
  platform/x86: int3472: discrete: Add support for 1 GPIO regulator
    shared between 2 sensors
  platform/x86: int3472: discrete: Add alternative "AVDD" regulator
    supply name

 .../x86/intel/int3472/clk_and_regulator.c     | 72 ++++++++++++++----
 drivers/platform/x86/intel/int3472/common.h   | 14 +---
 drivers/platform/x86/intel/int3472/discrete.c | 76 ++-----------------
 3 files changed, 66 insertions(+), 96 deletions(-)

-- 
2.40.1

[PATCH 1/4] platform/x86: int3472: discrete: Drop GPIO remapping support

[Hans de Goede]

The only sensor driver which needs GPIO remapping support is the ov2680
driver and ACPI enumeration support + other necessary changes to
the ov2680 driver were never upstreamed.

A new series updating the ov2680 driver is pending upstream now and
in this series the ov2680 driver is patched to look for "powerdown"
as con-id, instead of relying on GPIO remapping in the int3472 code,
so the GPIO remapping is no longer necessary.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/platform/x86/intel/int3472/common.h   |  6 ---
 drivers/platform/x86/intel/int3472/discrete.c | 37 ++-----------------
 2 files changed, 3 insertions(+), 40 deletions(-)

diff --git a/drivers/platform/x86/intel/int3472/common.h b/drivers/platform/x86/intel/int3472/common.h
index 0c9c899e017b..735567f374a6 100644
--- a/drivers/platform/x86/intel/int3472/common.h
+++ b/drivers/platform/x86/intel/int3472/common.h
@@ -69,15 +69,9 @@ struct int3472_cldb {
 	u8 reserved2[17];
 };
 
-struct int3472_gpio_function_remap {
-	const char *documented;
-	const char *actual;
-};
-
 struct int3472_sensor_config {
 	const char *sensor_module_name;
 	struct regulator_consumer_supply supply_map;
-	const struct int3472_gpio_function_remap *function_maps;
 };
 
 struct int3472_discrete_device {
diff --git a/drivers/platform/x86/intel/int3472/discrete.c b/drivers/platform/x86/intel/int3472/discrete.c
index 8111579a59d4..2ab3c7466986 100644
--- a/drivers/platform/x86/intel/int3472/discrete.c
+++ b/drivers/platform/x86/intel/int3472/discrete.c
@@ -39,27 +39,13 @@ static const guid_t cio2_sensor_module_guid =
  * the functions mapping resources to the sensors. Where the sensors have
  * a power enable pin defined in DSDT we need to provide a supply name so
  * the sensor drivers can find the regulator. The device name will be derived
- * from the sensor's ACPI device within the code. Optionally, we can provide a
- * NULL terminated array of function name mappings to deal with any platform
- * specific deviations from the documented behaviour of GPIOs.
- *
- * Map a GPIO function name to NULL to prevent the driver from mapping that
- * GPIO at all.
+ * from the sensor's ACPI device within the code.
  */
-
-static const struct int3472_gpio_function_remap ov2680_gpio_function_remaps[] = {
-	{ "reset", NULL },
-	{ "powerdown", "reset" },
-	{ }
-};
-
 static const struct int3472_sensor_config int3472_sensor_configs[] = {
-	/* Lenovo Miix 510-12ISK - OV2680, Front */
-	{ "GNDF140809R", { 0 }, ov2680_gpio_function_remaps },
 	/* Lenovo Miix 510-12ISK - OV5648, Rear */
-	{ "GEFF150023R", REGULATOR_SUPPLY("avdd", NULL), NULL },
+	{ "GEFF150023R", REGULATOR_SUPPLY("avdd", NULL) },
 	/* Surface Go 1&2 - OV5693, Front */
-	{ "YHCU", REGULATOR_SUPPLY("avdd", NULL), NULL },
+	{ "YHCU", REGULATOR_SUPPLY("avdd", NULL) },
 };
 
 static const struct int3472_sensor_config *
@@ -96,7 +82,6 @@ static int skl_int3472_map_gpio_to_sensor(struct int3472_discrete_device *int347
 					  struct acpi_resource_gpio *agpio,
 					  const char *func, u32 polarity)
 {
-	const struct int3472_sensor_config *sensor_config;
 	char *path = agpio->resource_source.string_ptr;
 	struct gpiod_lookup *table_entry;
 	struct acpi_device *adev;
@@ -108,22 +93,6 @@ static int skl_int3472_map_gpio_to_sensor(struct int3472_discrete_device *int347
 		return -EINVAL;
 	}
 
-	sensor_config = int3472->sensor_config;
-	if (!IS_ERR(sensor_config) && sensor_config->function_maps) {
-		const struct int3472_gpio_function_remap *remap;
-
-		for (remap = sensor_config->function_maps; remap->documented; remap++) {
-			if (!strcmp(func, remap->documented)) {
-				func = remap->actual;
-				break;
-			}
-		}
-	}
-
-	/* Functions mapped to NULL should not be mapped to the sensor */
-	if (!func)
-		return 0;
-
 	status = acpi_get_handle(NULL, path, &handle);
 	if (ACPI_FAILURE(status))
 		return -EINVAL;
-- 
2.40.1

[PATCH 2/4] platform/x86: int3472: discrete: Remove sensor_config-s

[Hans de Goede]

Currently the only 2 sensor_config-s both specify "avdd" as supply-id.

The INT3472 device is going to be the only supplier of a regulator for
the sensor device.

So there is no chance of collisions with other regulator suppliers
and it is undesirable to need to manually add new entries to
int3472_sensor_configs[] for each new sensor module which uses
a GPIO regulator.

Instead just always use "avdd" as supply-id when registering
the GPIO regulator.

If necessary for specific sensor drivers then other supply-ids can
be added as aliases in the future, adding aliases will be safe
since INT3472 will be the only regulator supplier for the sensor.

Cc: Hao Yao <hao.yao@intel.com>
Cc: Bingbu Cao <bingbu.cao@intel.com>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 .../x86/intel/int3472/clk_and_regulator.c     | 38 ++++++++++------
 drivers/platform/x86/intel/int3472/common.h   |  7 +--
 drivers/platform/x86/intel/int3472/discrete.c | 45 +++----------------
 3 files changed, 31 insertions(+), 59 deletions(-)

diff --git a/drivers/platform/x86/intel/int3472/clk_and_regulator.c b/drivers/platform/x86/intel/int3472/clk_and_regulator.c
index b3a55c618151..30686091300d 100644
--- a/drivers/platform/x86/intel/int3472/clk_and_regulator.c
+++ b/drivers/platform/x86/intel/int3472/clk_and_regulator.c
@@ -232,32 +232,42 @@ void skl_int3472_unregister_clock(struct int3472_discrete_device *int3472)
 	gpiod_put(int3472->clock.ena_gpio);
 }
 
+/*
+ * The INT3472 device is going to be the only supplier of a regulator for
+ * the sensor device. But unlike the clk framework the regulator framework
+ * does not allow matching by consumer-device-name only.
+ *
+ * Ideally all sensor drivers would use "avdd" as supply-id. But for drivers
+ * where this cannot be changed because another supply-id is already used in
+ * e.g. DeviceTree files an alias for the other supply-id can be added here.
+ *
+ * Do not forget to update GPIO_REGULATOR_SUPPLY_MAP_COUNT when changing this.
+ */
+static const char * const skl_int3472_regulator_map_supplies[] = {
+	"avdd",
+};
+
 int skl_int3472_register_regulator(struct int3472_discrete_device *int3472,
 				   struct acpi_resource_gpio *agpio)
 {
-	const struct int3472_sensor_config *sensor_config;
 	char *path = agpio->resource_source.string_ptr;
-	struct regulator_consumer_supply supply_map;
 	struct regulator_init_data init_data = { };
 	struct regulator_config cfg = { };
-	int ret;
+	int i, ret;
 
-	sensor_config = int3472->sensor_config;
-	if (IS_ERR(sensor_config)) {
-		dev_err(int3472->dev, "No sensor module config\n");
-		return PTR_ERR(sensor_config);
+	if (ARRAY_SIZE(skl_int3472_regulator_map_supplies) != GPIO_REGULATOR_SUPPLY_MAP_COUNT) {
+		dev_err(int3472->dev, "Internal error ARRAY_SIZE(skl_int3472_regulator_map_supplies) != GPIO_REGULATOR_SUPPLY_MAP_COUNT\n");
+		return -EINVAL;
 	}
 
-	if (!sensor_config->supply_map.supply) {
-		dev_err(int3472->dev, "No supply name defined\n");
-		return -ENODEV;
+	for (i = 0; i < ARRAY_SIZE(skl_int3472_regulator_map_supplies); i++) {
+		int3472->regulator.supply_map[i].supply = skl_int3472_regulator_map_supplies[i];
+		int3472->regulator.supply_map[i].dev_name = int3472->sensor_name;
 	}
 
 	init_data.constraints.valid_ops_mask = REGULATOR_CHANGE_STATUS;
-	init_data.num_consumer_supplies = 1;
-	supply_map = sensor_config->supply_map;
-	supply_map.dev_name = int3472->sensor_name;
-	init_data.consumer_supplies = &supply_map;
+	init_data.consumer_supplies = int3472->regulator.supply_map;
+	init_data.num_consumer_supplies = GPIO_REGULATOR_SUPPLY_MAP_COUNT;
 
 	snprintf(int3472->regulator.regulator_name,
 		 sizeof(int3472->regulator.regulator_name), "%s-regulator",
diff --git a/drivers/platform/x86/intel/int3472/common.h b/drivers/platform/x86/intel/int3472/common.h
index 735567f374a6..225b067c854d 100644
--- a/drivers/platform/x86/intel/int3472/common.h
+++ b/drivers/platform/x86/intel/int3472/common.h
@@ -28,6 +28,7 @@
 
 #define GPIO_REGULATOR_NAME_LENGTH				21
 #define GPIO_REGULATOR_SUPPLY_NAME_LENGTH			9
+#define GPIO_REGULATOR_SUPPLY_MAP_COUNT				1
 
 #define INT3472_LED_MAX_NAME_LEN				32
 
@@ -69,11 +70,6 @@ struct int3472_cldb {
 	u8 reserved2[17];
 };
 
-struct int3472_sensor_config {
-	const char *sensor_module_name;
-	struct regulator_consumer_supply supply_map;
-};
-
 struct int3472_discrete_device {
 	struct acpi_device *adev;
 	struct device *dev;
@@ -83,6 +79,7 @@ struct int3472_discrete_device {
 	const struct int3472_sensor_config *sensor_config;
 
 	struct int3472_gpio_regulator {
+		struct regulator_consumer_supply supply_map[GPIO_REGULATOR_SUPPLY_MAP_COUNT];
 		char regulator_name[GPIO_REGULATOR_NAME_LENGTH];
 		char supply_name[GPIO_REGULATOR_SUPPLY_NAME_LENGTH];
 		struct gpio_desc *gpio;
diff --git a/drivers/platform/x86/intel/int3472/discrete.c b/drivers/platform/x86/intel/int3472/discrete.c
index 2ab3c7466986..3b410428cec2 100644
--- a/drivers/platform/x86/intel/int3472/discrete.c
+++ b/drivers/platform/x86/intel/int3472/discrete.c
@@ -34,48 +34,17 @@ static const guid_t cio2_sensor_module_guid =
 	GUID_INIT(0x822ace8f, 0x2814, 0x4174,
 		  0xa5, 0x6b, 0x5f, 0x02, 0x9f, 0xe0, 0x79, 0xee);
 
-/*
- * Here follows platform specific mapping information that we can pass to
- * the functions mapping resources to the sensors. Where the sensors have
- * a power enable pin defined in DSDT we need to provide a supply name so
- * the sensor drivers can find the regulator. The device name will be derived
- * from the sensor's ACPI device within the code.
- */
-static const struct int3472_sensor_config int3472_sensor_configs[] = {
-	/* Lenovo Miix 510-12ISK - OV5648, Rear */
-	{ "GEFF150023R", REGULATOR_SUPPLY("avdd", NULL) },
-	/* Surface Go 1&2 - OV5693, Front */
-	{ "YHCU", REGULATOR_SUPPLY("avdd", NULL) },
-};
-
-static const struct int3472_sensor_config *
-skl_int3472_get_sensor_module_config(struct int3472_discrete_device *int3472)
+static void skl_int3472_log_sensor_module_name(struct int3472_discrete_device *int3472)
 {
 	union acpi_object *obj;
-	unsigned int i;
 
 	obj = acpi_evaluate_dsm_typed(int3472->sensor->handle,
 				      &cio2_sensor_module_guid, 0x00,
 				      0x01, NULL, ACPI_TYPE_STRING);
-
-	if (!obj) {
-		dev_err(int3472->dev,
-			"Failed to get sensor module string from _DSM\n");
-		return ERR_PTR(-ENODEV);
+	if (obj) {
+		dev_dbg(int3472->dev, "Sensor module id: '%s'\n", obj->string.pointer);
+		ACPI_FREE(obj);
 	}
-
-	for (i = 0; i < ARRAY_SIZE(int3472_sensor_configs); i++) {
-		if (!strcmp(int3472_sensor_configs[i].sensor_module_name,
-			    obj->string.pointer))
-			break;
-	}
-
-	ACPI_FREE(obj);
-
-	if (i >= ARRAY_SIZE(int3472_sensor_configs))
-		return ERR_PTR(-EINVAL);
-
-	return &int3472_sensor_configs[i];
 }
 
 static int skl_int3472_map_gpio_to_sensor(struct int3472_discrete_device *int3472,
@@ -266,11 +235,7 @@ static int skl_int3472_parse_crs(struct int3472_discrete_device *int3472)
 	LIST_HEAD(resource_list);
 	int ret;
 
-	/*
-	 * No error check, because not having a sensor config is not necessarily
-	 * a failure mode.
-	 */
-	int3472->sensor_config = skl_int3472_get_sensor_module_config(int3472);
+	skl_int3472_log_sensor_module_name(int3472);
 
 	ret = acpi_dev_get_resources(int3472->adev, &resource_list,
 				     skl_int3472_handle_gpio_resources,
-- 
2.40.1

[PATCH 3/4] platform/x86: int3472: discrete: Add support for 1 GPIO regulator shared between 2 sensors

[Hans de Goede]

On the Lenovo Miix 510-12IKB there is 1 GPIO regulator, with its GPIO
listed in the INT3472 device belonging to the OV5648 back sensor.
But this regulator also needs to be enabled for the OV2680 front sensor
to work.

Add support to skl_int3472_register_regulator() to add supply map entries
pointing to both sensors based on a DMI quirk table which gives the
dev_name part of the supply map for the second sensor (the sensor without
the GPIO listed in its matching INT3472 ACPI device).

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 .../x86/intel/int3472/clk_and_regulator.c     | 43 ++++++++++++++++---
 drivers/platform/x86/intel/int3472/common.h   |  3 +-
 2 files changed, 40 insertions(+), 6 deletions(-)

diff --git a/drivers/platform/x86/intel/int3472/clk_and_regulator.c b/drivers/platform/x86/intel/int3472/clk_and_regulator.c
index 30686091300d..9166f6afcdf9 100644
--- a/drivers/platform/x86/intel/int3472/clk_and_regulator.c
+++ b/drivers/platform/x86/intel/int3472/clk_and_regulator.c
@@ -5,6 +5,7 @@
 #include <linux/clkdev.h>
 #include <linux/clk-provider.h>
 #include <linux/device.h>
+#include <linux/dmi.h>
 #include <linux/gpio/consumer.h>
 #include <linux/regulator/driver.h>
 #include <linux/slab.h>
@@ -247,27 +248,59 @@ static const char * const skl_int3472_regulator_map_supplies[] = {
 	"avdd",
 };
 
+/*
+ * On some models there is a single GPIO regulator which is shared between
+ * sensors and only listed in the ACPI resources of one sensor.
+ * This DMI table contains the name of the second sensor. This is used to add
+ * entries for the second sensor to the supply_map.
+ */
+const struct dmi_system_id skl_int3472_regulator_second_sensor[] = {
+	{
+		/* Lenovo Miix 510-12IKB */
+		.matches = {
+			DMI_MATCH(DMI_SYS_VENDOR, "LENOVO"),
+			DMI_MATCH(DMI_PRODUCT_VERSION, "MIIX 510-12IKB"),
+		},
+		.driver_data = "i2c-OVTI2680:00",
+	},
+	{ }
+};
+
 int skl_int3472_register_regulator(struct int3472_discrete_device *int3472,
 				   struct acpi_resource_gpio *agpio)
 {
 	char *path = agpio->resource_source.string_ptr;
 	struct regulator_init_data init_data = { };
 	struct regulator_config cfg = { };
-	int i, ret;
+	const char *second_sensor = NULL;
+	const struct dmi_system_id *id;
+	int i, j, ret;
+
+	id = dmi_first_match(skl_int3472_regulator_second_sensor);
+	if (id)
+		second_sensor = id->driver_data;
 
 	if (ARRAY_SIZE(skl_int3472_regulator_map_supplies) != GPIO_REGULATOR_SUPPLY_MAP_COUNT) {
 		dev_err(int3472->dev, "Internal error ARRAY_SIZE(skl_int3472_regulator_map_supplies) != GPIO_REGULATOR_SUPPLY_MAP_COUNT\n");
 		return -EINVAL;
 	}
 
-	for (i = 0; i < ARRAY_SIZE(skl_int3472_regulator_map_supplies); i++) {
-		int3472->regulator.supply_map[i].supply = skl_int3472_regulator_map_supplies[i];
-		int3472->regulator.supply_map[i].dev_name = int3472->sensor_name;
+	for (i = 0, j = 0; i < ARRAY_SIZE(skl_int3472_regulator_map_supplies); i++) {
+		int3472->regulator.supply_map[j].supply = skl_int3472_regulator_map_supplies[i];
+		int3472->regulator.supply_map[j].dev_name = int3472->sensor_name;
+		j++;
+
+		if (second_sensor) {
+			int3472->regulator.supply_map[j].supply =
+				skl_int3472_regulator_map_supplies[i];
+			int3472->regulator.supply_map[j].dev_name = second_sensor;
+			j++;
+		}
 	}
 
 	init_data.constraints.valid_ops_mask = REGULATOR_CHANGE_STATUS;
 	init_data.consumer_supplies = int3472->regulator.supply_map;
-	init_data.num_consumer_supplies = GPIO_REGULATOR_SUPPLY_MAP_COUNT;
+	init_data.num_consumer_supplies = j;
 
 	snprintf(int3472->regulator.regulator_name,
 		 sizeof(int3472->regulator.regulator_name), "%s-regulator",
diff --git a/drivers/platform/x86/intel/int3472/common.h b/drivers/platform/x86/intel/int3472/common.h
index 225b067c854d..fd2a3d3884fa 100644
--- a/drivers/platform/x86/intel/int3472/common.h
+++ b/drivers/platform/x86/intel/int3472/common.h
@@ -79,7 +79,8 @@ struct int3472_discrete_device {
 	const struct int3472_sensor_config *sensor_config;
 
 	struct int3472_gpio_regulator {
-		struct regulator_consumer_supply supply_map[GPIO_REGULATOR_SUPPLY_MAP_COUNT];
+		/* SUPPLY_MAP_COUNT * 2 to make room for second sensor mappings */
+		struct regulator_consumer_supply supply_map[GPIO_REGULATOR_SUPPLY_MAP_COUNT * 2];
 		char regulator_name[GPIO_REGULATOR_NAME_LENGTH];
 		char supply_name[GPIO_REGULATOR_SUPPLY_NAME_LENGTH];
 		struct gpio_desc *gpio;
-- 
2.40.1

[PATCH 4/4] platform/x86: int3472: discrete: Add alternative "AVDD" regulator supply name

[Hans de Goede]

Add an "AVDD" regulator supply name alias to the supply-map which
gets registered for the INT3472 GPIO regulator.

This is necessary for the ov2680 driver which expects "AVDD" rather then
"avdd". Updating the ov2680 driver to use "avdd" is not possible because
that will break compatibility with existing DT / DTB files.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/platform/x86/intel/int3472/clk_and_regulator.c | 1 +
 drivers/platform/x86/intel/int3472/common.h            | 2 +-
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/platform/x86/intel/int3472/clk_and_regulator.c b/drivers/platform/x86/intel/int3472/clk_and_regulator.c
index 9166f6afcdf9..8fbcf1f41f65 100644
--- a/drivers/platform/x86/intel/int3472/clk_and_regulator.c
+++ b/drivers/platform/x86/intel/int3472/clk_and_regulator.c
@@ -246,6 +246,7 @@ void skl_int3472_unregister_clock(struct int3472_discrete_device *int3472)
  */
 static const char * const skl_int3472_regulator_map_supplies[] = {
 	"avdd",
+	"AVDD",
 };
 
 /*
diff --git a/drivers/platform/x86/intel/int3472/common.h b/drivers/platform/x86/intel/int3472/common.h
index fd2a3d3884fa..9f29baa13860 100644
--- a/drivers/platform/x86/intel/int3472/common.h
+++ b/drivers/platform/x86/intel/int3472/common.h
@@ -28,7 +28,7 @@
 
 #define GPIO_REGULATOR_NAME_LENGTH				21
 #define GPIO_REGULATOR_SUPPLY_NAME_LENGTH			9
-#define GPIO_REGULATOR_SUPPLY_MAP_COUNT				1
+#define GPIO_REGULATOR_SUPPLY_MAP_COUNT				2
 
 #define INT3472_LED_MAX_NAME_LEN				32
 
-- 
2.40.1

Re: [PATCH 0/4] platform/x86: int3472: discrete: Regulator rework / Lenovo Miix 510 support

[Hans de Goede]

Hi All,

On 6/9/23 22:42, Hans de Goede wrote:
> Hi Dan, Hao Yao and Bingbu Cao,
> 
> Patches 1/2 drop the sensor-config stuff since I thought we should be
> able to make things work without any board specific fixups.
> 
> This is the result of my working on getting IPU6 to work on Jasper Lake
> for $dayjob and then tonight I switched to trying to get the ov2680
> on the Lenovo Miix 510 to work and it turns out that does require some
> board specific workarounds after all :|
> 
> With this series together with my recent ov2680 sensor driver series:
> https://lore.kernel.org/linux-media/20230607164712.63579-1-hdegoede@redhat.com/
> I can get the ov2680 driver to load and successfully read the id register:
> 
> [   11.365319] ipu3-cio2 0000:00:14.3: Found supported sensor OVTI2680:00
> [   11.431595] ov2680 i2c-OVTI2680:00: supply DOVDD not found, using dummy regulator
> [   11.433125] ov2680 i2c-OVTI2680:00: supply DVDD not found, using dummy regulator
> [   11.454698] ov2680 i2c-OVTI2680:00: sensor_revision id = 0x2680, rev= 0
> 
> Dan, currently the DMI match used only matches the 12IKB version of
> the Miix 510 I think you have a 12ISK version. Can you verify this
> works there too?  I guess we can just drop the KB part of the DMI
> match if this works on the 12ISK version too.
> 
> Hao Yao and Bingbu Cao I think that the way the issue with how different
> drivers may expect different regulator supply-ids is of interest to you
> too. Note I see that the mainline version of ov13b10.c does not have
> regulator support at all yet. So when adding this please just use
> one of the existing set of supply-names + the bulk API like how
> the ov5693.c driver is doing. In this case no int3472 driver changes
> will be necessary at all.

I've added this to my review-hans (soon to be for-next) branch now.

Regards,

Hans




> Hans de Goede (4):
>   platform/x86: int3472: discrete: Drop GPIO remapping support
>   platform/x86: int3472: discrete: Remove sensor_config-s
>   platform/x86: int3472: discrete: Add support for 1 GPIO regulator
>     shared between 2 sensors
>   platform/x86: int3472: discrete: Add alternative "AVDD" regulator
>     supply name
> 
>  .../x86/intel/int3472/clk_and_regulator.c     | 72 ++++++++++++++----
>  drivers/platform/x86/intel/int3472/common.h   | 14 +---
>  drivers/platform/x86/intel/int3472/discrete.c | 76 ++-----------------
>  3 files changed, 66 insertions(+), 96 deletions(-)
> 

Re: [PATCH 0/4] platform/x86: int3472: discrete: Regulator rework / Lenovo Miix 510 support

[Hao Yao]

Hi Hans,

On 2023/6/15 20:38, Hans de Goede wrote:
> Hi All,
> 
> On 6/9/23 22:42, Hans de Goede wrote:
>> Hi Dan, Hao Yao and Bingbu Cao,
>>
>> Patches 1/2 drop the sensor-config stuff since I thought we should be
>> able to make things work without any board specific fixups.
>>
>> This is the result of my working on getting IPU6 to work on Jasper Lake
>> for $dayjob and then tonight I switched to trying to get the ov2680
>> on the Lenovo Miix 510 to work and it turns out that does require some
>> board specific workarounds after all :|
>>
>> With this series together with my recent ov2680 sensor driver series:
>> https://lore.kernel.org/linux-media/20230607164712.63579-1-hdegoede@redhat.com/
>> I can get the ov2680 driver to load and successfully read the id register:
>>
>> [   11.365319] ipu3-cio2 0000:00:14.3: Found supported sensor OVTI2680:00
>> [   11.431595] ov2680 i2c-OVTI2680:00: supply DOVDD not found, using dummy regulator
>> [   11.433125] ov2680 i2c-OVTI2680:00: supply DVDD not found, using dummy regulator
>> [   11.454698] ov2680 i2c-OVTI2680:00: sensor_revision id = 0x2680, rev= 0
>>
>> Dan, currently the DMI match used only matches the 12IKB version of
>> the Miix 510 I think you have a 12ISK version. Can you verify this
>> works there too?  I guess we can just drop the KB part of the DMI
>> match if this works on the 12ISK version too.
>>
>> Hao Yao and Bingbu Cao I think that the way the issue with how different
>> drivers may expect different regulator supply-ids is of interest to you
>> too. Note I see that the mainline version of ov13b10.c does not have
>> regulator support at all yet. So when adding this please just use
>> one of the existing set of supply-names + the bulk API like how
>> the ov5693.c driver is doing. In this case no int3472 driver changes
>> will be necessary at all.
> 
> I've added this to my review-hans (soon to be for-next) branch now.
> 
> Regards,
> 
> Hans
> 
> 
> 
> 
>> Hans de Goede (4):
>>    platform/x86: int3472: discrete: Drop GPIO remapping support
>>    platform/x86: int3472: discrete: Remove sensor_config-s
>>    platform/x86: int3472: discrete: Add support for 1 GPIO regulator
>>      shared between 2 sensors
>>    platform/x86: int3472: discrete: Add alternative "AVDD" regulator
>>      supply name
>>
>>   .../x86/intel/int3472/clk_and_regulator.c     | 72 ++++++++++++++----
>>   drivers/platform/x86/intel/int3472/common.h   | 14 +---
>>   drivers/platform/x86/intel/int3472/discrete.c | 76 ++-----------------
>>   3 files changed, 66 insertions(+), 96 deletions(-)
>>
> 

Thanks for your patches.

I tested them on my devices using int3472 regulator, including Lenovo 
Thinkpad X1 and testing platforms with ov13b10 camera sensor.

Tested-By: Hao Yao <hao.yao@intel.com>


Best Regards,
Hao Yao

Re: [PATCH 1/4] platform/x86: int3472: discrete: Drop GPIO remapping support

[Dan Scally]

Hi Hans - sorry I've been late replying to this series.

On 09/06/2023 21:42, Hans de Goede wrote:
> The only sensor driver which needs GPIO remapping support is the ov2680
> driver and ACPI enumeration support + other necessary changes to
> the ov2680 driver were never upstreamed.
>
> A new series updating the ov2680 driver is pending upstream now and
> in this series the ov2680 driver is patched to look for "powerdown"
> as con-id, instead of relying on GPIO remapping in the int3472 code,
> so the GPIO remapping is no longer necessary.
>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>


Reviewed-by: Daniel Scally <dan.scally@ideasonboard.com>

> ---
>   drivers/platform/x86/intel/int3472/common.h   |  6 ---
>   drivers/platform/x86/intel/int3472/discrete.c | 37 ++-----------------
>   2 files changed, 3 insertions(+), 40 deletions(-)
>
> diff --git a/drivers/platform/x86/intel/int3472/common.h b/drivers/platform/x86/intel/int3472/common.h
> index 0c9c899e017b..735567f374a6 100644
> --- a/drivers/platform/x86/intel/int3472/common.h
> +++ b/drivers/platform/x86/intel/int3472/common.h
> @@ -69,15 +69,9 @@ struct int3472_cldb {
>   	u8 reserved2[17];
>   };
>   
> -struct int3472_gpio_function_remap {
> -	const char *documented;
> -	const char *actual;
> -};
> -
>   struct int3472_sensor_config {
>   	const char *sensor_module_name;
>   	struct regulator_consumer_supply supply_map;
> -	const struct int3472_gpio_function_remap *function_maps;
>   };
>   
>   struct int3472_discrete_device {
> diff --git a/drivers/platform/x86/intel/int3472/discrete.c b/drivers/platform/x86/intel/int3472/discrete.c
> index 8111579a59d4..2ab3c7466986 100644
> --- a/drivers/platform/x86/intel/int3472/discrete.c
> +++ b/drivers/platform/x86/intel/int3472/discrete.c
> @@ -39,27 +39,13 @@ static const guid_t cio2_sensor_module_guid =
>    * the functions mapping resources to the sensors. Where the sensors have
>    * a power enable pin defined in DSDT we need to provide a supply name so
>    * the sensor drivers can find the regulator. The device name will be derived
> - * from the sensor's ACPI device within the code. Optionally, we can provide a
> - * NULL terminated array of function name mappings to deal with any platform
> - * specific deviations from the documented behaviour of GPIOs.
> - *
> - * Map a GPIO function name to NULL to prevent the driver from mapping that
> - * GPIO at all.
> + * from the sensor's ACPI device within the code.
>    */
> -
> -static const struct int3472_gpio_function_remap ov2680_gpio_function_remaps[] = {
> -	{ "reset", NULL },
> -	{ "powerdown", "reset" },
> -	{ }
> -};
> -
>   static const struct int3472_sensor_config int3472_sensor_configs[] = {
> -	/* Lenovo Miix 510-12ISK - OV2680, Front */
> -	{ "GNDF140809R", { 0 }, ov2680_gpio_function_remaps },
>   	/* Lenovo Miix 510-12ISK - OV5648, Rear */
> -	{ "GEFF150023R", REGULATOR_SUPPLY("avdd", NULL), NULL },
> +	{ "GEFF150023R", REGULATOR_SUPPLY("avdd", NULL) },
>   	/* Surface Go 1&2 - OV5693, Front */
> -	{ "YHCU", REGULATOR_SUPPLY("avdd", NULL), NULL },
> +	{ "YHCU", REGULATOR_SUPPLY("avdd", NULL) },
>   };
>   
>   static const struct int3472_sensor_config *
> @@ -96,7 +82,6 @@ static int skl_int3472_map_gpio_to_sensor(struct int3472_discrete_device *int347
>   					  struct acpi_resource_gpio *agpio,
>   					  const char *func, u32 polarity)
>   {
> -	const struct int3472_sensor_config *sensor_config;
>   	char *path = agpio->resource_source.string_ptr;
>   	struct gpiod_lookup *table_entry;
>   	struct acpi_device *adev;
> @@ -108,22 +93,6 @@ static int skl_int3472_map_gpio_to_sensor(struct int3472_discrete_device *int347
>   		return -EINVAL;
>   	}
>   
> -	sensor_config = int3472->sensor_config;
> -	if (!IS_ERR(sensor_config) && sensor_config->function_maps) {
> -		const struct int3472_gpio_function_remap *remap;
> -
> -		for (remap = sensor_config->function_maps; remap->documented; remap++) {
> -			if (!strcmp(func, remap->documented)) {
> -				func = remap->actual;
> -				break;
> -			}
> -		}
> -	}
> -
> -	/* Functions mapped to NULL should not be mapped to the sensor */
> -	if (!func)
> -		return 0;
> -
>   	status = acpi_get_handle(NULL, path, &handle);
>   	if (ACPI_FAILURE(status))
>   		return -EINVAL;

Re: [PATCH 2/4] platform/x86: int3472: discrete: Remove sensor_config-s

[Dan Scally]

Hi Hans

On 09/06/2023 21:42, Hans de Goede wrote:
> Currently the only 2 sensor_config-s both specify "avdd" as supply-id.
>
> The INT3472 device is going to be the only supplier of a regulator for
> the sensor device.
>
> So there is no chance of collisions with other regulator suppliers
> and it is undesirable to need to manually add new entries to
> int3472_sensor_configs[] for each new sensor module which uses
> a GPIO regulator.
>
> Instead just always use "avdd" as supply-id when registering
> the GPIO regulator.
>
> If necessary for specific sensor drivers then other supply-ids can
> be added as aliases in the future, adding aliases will be safe
> since INT3472 will be the only regulator supplier for the sensor.
>
> Cc: Hao Yao <hao.yao@intel.com>
> Cc: Bingbu Cao <bingbu.cao@intel.com>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>   .../x86/intel/int3472/clk_and_regulator.c     | 38 ++++++++++------
>   drivers/platform/x86/intel/int3472/common.h   |  7 +--
>   drivers/platform/x86/intel/int3472/discrete.c | 45 +++----------------
>   3 files changed, 31 insertions(+), 59 deletions(-)
>
> diff --git a/drivers/platform/x86/intel/int3472/clk_and_regulator.c b/drivers/platform/x86/intel/int3472/clk_and_regulator.c
> index b3a55c618151..30686091300d 100644
> --- a/drivers/platform/x86/intel/int3472/clk_and_regulator.c
> +++ b/drivers/platform/x86/intel/int3472/clk_and_regulator.c
> @@ -232,32 +232,42 @@ void skl_int3472_unregister_clock(struct int3472_discrete_device *int3472)
>   	gpiod_put(int3472->clock.ena_gpio);
>   }
>   
> +/*
> + * The INT3472 device is going to be the only supplier of a regulator for
> + * the sensor device. But unlike the clk framework the regulator framework
> + * does not allow matching by consumer-device-name only.
> + *
> + * Ideally all sensor drivers would use "avdd" as supply-id. But for drivers
> + * where this cannot be changed because another supply-id is already used in
> + * e.g. DeviceTree files an alias for the other supply-id can be added here.
> + *
> + * Do not forget to update GPIO_REGULATOR_SUPPLY_MAP_COUNT when changing this.
> + */
> +static const char * const skl_int3472_regulator_map_supplies[] = {
> +	"avdd",
> +};
> +
>   int skl_int3472_register_regulator(struct int3472_discrete_device *int3472,
>   				   struct acpi_resource_gpio *agpio)
>   {
> -	const struct int3472_sensor_config *sensor_config;
>   	char *path = agpio->resource_source.string_ptr;
> -	struct regulator_consumer_supply supply_map;
>   	struct regulator_init_data init_data = { };
>   	struct regulator_config cfg = { };
> -	int ret;
> +	int i, ret;
>   
> -	sensor_config = int3472->sensor_config;
> -	if (IS_ERR(sensor_config)) {
> -		dev_err(int3472->dev, "No sensor module config\n");
> -		return PTR_ERR(sensor_config);
> +	if (ARRAY_SIZE(skl_int3472_regulator_map_supplies) != GPIO_REGULATOR_SUPPLY_MAP_COUNT) {
> +		dev_err(int3472->dev, "Internal error ARRAY_SIZE(skl_int3472_regulator_map_supplies) != GPIO_REGULATOR_SUPPLY_MAP_COUNT\n");
> +		return -EINVAL;


It would be nice to be able to prevent this mismatch somehow so it's never a problem; can we use 
static_assert() perhaps? Or at least less of a problem, as I gather that gets compiled to a no-op 
sometimes.

>   	}
>   
> -	if (!sensor_config->supply_map.supply) {
> -		dev_err(int3472->dev, "No supply name defined\n");
> -		return -ENODEV;
> +	for (i = 0; i < ARRAY_SIZE(skl_int3472_regulator_map_supplies); i++) {
> +		int3472->regulator.supply_map[i].supply = skl_int3472_regulator_map_supplies[i];
> +		int3472->regulator.supply_map[i].dev_name = int3472->sensor_name;
>   	}
>   
>   	init_data.constraints.valid_ops_mask = REGULATOR_CHANGE_STATUS;
> -	init_data.num_consumer_supplies = 1;
> -	supply_map = sensor_config->supply_map;
> -	supply_map.dev_name = int3472->sensor_name;
> -	init_data.consumer_supplies = &supply_map;
> +	init_data.consumer_supplies = int3472->regulator.supply_map;
> +	init_data.num_consumer_supplies = GPIO_REGULATOR_SUPPLY_MAP_COUNT;
>   
>   	snprintf(int3472->regulator.regulator_name,
>   		 sizeof(int3472->regulator.regulator_name), "%s-regulator",
> diff --git a/drivers/platform/x86/intel/int3472/common.h b/drivers/platform/x86/intel/int3472/common.h
> index 735567f374a6..225b067c854d 100644
> --- a/drivers/platform/x86/intel/int3472/common.h
> +++ b/drivers/platform/x86/intel/int3472/common.h
> @@ -28,6 +28,7 @@
>   
>   #define GPIO_REGULATOR_NAME_LENGTH				21
>   #define GPIO_REGULATOR_SUPPLY_NAME_LENGTH			9
> +#define GPIO_REGULATOR_SUPPLY_MAP_COUNT				1
>   
>   #define INT3472_LED_MAX_NAME_LEN				32
>   
> @@ -69,11 +70,6 @@ struct int3472_cldb {
>   	u8 reserved2[17];
>   };
>   
> -struct int3472_sensor_config {
> -	const char *sensor_module_name;
> -	struct regulator_consumer_supply supply_map;
> -};
> -
>   struct int3472_discrete_device {
>   	struct acpi_device *adev;
>   	struct device *dev;
> @@ -83,6 +79,7 @@ struct int3472_discrete_device {
>   	const struct int3472_sensor_config *sensor_config;
>   
>   	struct int3472_gpio_regulator {
> +		struct regulator_consumer_supply supply_map[GPIO_REGULATOR_SUPPLY_MAP_COUNT];
>   		char regulator_name[GPIO_REGULATOR_NAME_LENGTH];
>   		char supply_name[GPIO_REGULATOR_SUPPLY_NAME_LENGTH];
>   		struct gpio_desc *gpio;
> diff --git a/drivers/platform/x86/intel/int3472/discrete.c b/drivers/platform/x86/intel/int3472/discrete.c
> index 2ab3c7466986..3b410428cec2 100644
> --- a/drivers/platform/x86/intel/int3472/discrete.c
> +++ b/drivers/platform/x86/intel/int3472/discrete.c
> @@ -34,48 +34,17 @@ static const guid_t cio2_sensor_module_guid =
>   	GUID_INIT(0x822ace8f, 0x2814, 0x4174,
>   		  0xa5, 0x6b, 0x5f, 0x02, 0x9f, 0xe0, 0x79, 0xee);
>   
> -/*
> - * Here follows platform specific mapping information that we can pass to
> - * the functions mapping resources to the sensors. Where the sensors have
> - * a power enable pin defined in DSDT we need to provide a supply name so
> - * the sensor drivers can find the regulator. The device name will be derived
> - * from the sensor's ACPI device within the code.
> - */
> -static const struct int3472_sensor_config int3472_sensor_configs[] = {
> -	/* Lenovo Miix 510-12ISK - OV5648, Rear */
> -	{ "GEFF150023R", REGULATOR_SUPPLY("avdd", NULL) },
> -	/* Surface Go 1&2 - OV5693, Front */
> -	{ "YHCU", REGULATOR_SUPPLY("avdd", NULL) },
> -};
> -
> -static const struct int3472_sensor_config *
> -skl_int3472_get_sensor_module_config(struct int3472_discrete_device *int3472)
> +static void skl_int3472_log_sensor_module_name(struct int3472_discrete_device *int3472)


I don't really think this is worth logging if we're removing the matching based on it - we can get 
it from the DSDT anyway if we need to.

>   {
>   	union acpi_object *obj;
> -	unsigned int i;
>   
>   	obj = acpi_evaluate_dsm_typed(int3472->sensor->handle,
>   				      &cio2_sensor_module_guid, 0x00,
>   				      0x01, NULL, ACPI_TYPE_STRING);
> -
> -	if (!obj) {
> -		dev_err(int3472->dev,
> -			"Failed to get sensor module string from _DSM\n");
> -		return ERR_PTR(-ENODEV);
> +	if (obj) {
> +		dev_dbg(int3472->dev, "Sensor module id: '%s'\n", obj->string.pointer);
> +		ACPI_FREE(obj);
>   	}
> -
> -	for (i = 0; i < ARRAY_SIZE(int3472_sensor_configs); i++) {
> -		if (!strcmp(int3472_sensor_configs[i].sensor_module_name,
> -			    obj->string.pointer))
> -			break;
> -	}
> -
> -	ACPI_FREE(obj);
> -
> -	if (i >= ARRAY_SIZE(int3472_sensor_configs))
> -		return ERR_PTR(-EINVAL);
> -
> -	return &int3472_sensor_configs[i];
>   }
>   
>   static int skl_int3472_map_gpio_to_sensor(struct int3472_discrete_device *int3472,
> @@ -266,11 +235,7 @@ static int skl_int3472_parse_crs(struct int3472_discrete_device *int3472)
>   	LIST_HEAD(resource_list);
>   	int ret;
>   
> -	/*
> -	 * No error check, because not having a sensor config is not necessarily
> -	 * a failure mode.
> -	 */
> -	int3472->sensor_config = skl_int3472_get_sensor_module_config(int3472);
> +	skl_int3472_log_sensor_module_name(int3472);
>   
>   	ret = acpi_dev_get_resources(int3472->adev, &resource_list,
>   				     skl_int3472_handle_gpio_resources,

Re: [PATCH 2/4] platform/x86: int3472: discrete: Remove sensor_config-s

[Hans de Goede]

Hi Dan,

On 6/16/23 14:34, Dan Scally wrote:
> Hi Hans
> 
> On 09/06/2023 21:42, Hans de Goede wrote:
>> Currently the only 2 sensor_config-s both specify "avdd" as supply-id.
>>
>> The INT3472 device is going to be the only supplier of a regulator for
>> the sensor device.
>>
>> So there is no chance of collisions with other regulator suppliers
>> and it is undesirable to need to manually add new entries to
>> int3472_sensor_configs[] for each new sensor module which uses
>> a GPIO regulator.
>>
>> Instead just always use "avdd" as supply-id when registering
>> the GPIO regulator.
>>
>> If necessary for specific sensor drivers then other supply-ids can
>> be added as aliases in the future, adding aliases will be safe
>> since INT3472 will be the only regulator supplier for the sensor.
>>
>> Cc: Hao Yao <hao.yao@intel.com>
>> Cc: Bingbu Cao <bingbu.cao@intel.com>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>>   .../x86/intel/int3472/clk_and_regulator.c     | 38 ++++++++++------
>>   drivers/platform/x86/intel/int3472/common.h   |  7 +--
>>   drivers/platform/x86/intel/int3472/discrete.c | 45 +++----------------
>>   3 files changed, 31 insertions(+), 59 deletions(-)
>>
>> diff --git a/drivers/platform/x86/intel/int3472/clk_and_regulator.c b/drivers/platform/x86/intel/int3472/clk_and_regulator.c
>> index b3a55c618151..30686091300d 100644
>> --- a/drivers/platform/x86/intel/int3472/clk_and_regulator.c
>> +++ b/drivers/platform/x86/intel/int3472/clk_and_regulator.c
>> @@ -232,32 +232,42 @@ void skl_int3472_unregister_clock(struct int3472_discrete_device *int3472)
>>       gpiod_put(int3472->clock.ena_gpio);
>>   }
>>   +/*
>> + * The INT3472 device is going to be the only supplier of a regulator for
>> + * the sensor device. But unlike the clk framework the regulator framework
>> + * does not allow matching by consumer-device-name only.
>> + *
>> + * Ideally all sensor drivers would use "avdd" as supply-id. But for drivers
>> + * where this cannot be changed because another supply-id is already used in
>> + * e.g. DeviceTree files an alias for the other supply-id can be added here.
>> + *
>> + * Do not forget to update GPIO_REGULATOR_SUPPLY_MAP_COUNT when changing this.
>> + */
>> +static const char * const skl_int3472_regulator_map_supplies[] = {
>> +    "avdd",
>> +};
>> +
>>   int skl_int3472_register_regulator(struct int3472_discrete_device *int3472,
>>                      struct acpi_resource_gpio *agpio)
>>   {
>> -    const struct int3472_sensor_config *sensor_config;
>>       char *path = agpio->resource_source.string_ptr;
>> -    struct regulator_consumer_supply supply_map;
>>       struct regulator_init_data init_data = { };
>>       struct regulator_config cfg = { };
>> -    int ret;
>> +    int i, ret;
>>   -    sensor_config = int3472->sensor_config;
>> -    if (IS_ERR(sensor_config)) {
>> -        dev_err(int3472->dev, "No sensor module config\n");
>> -        return PTR_ERR(sensor_config);
>> +    if (ARRAY_SIZE(skl_int3472_regulator_map_supplies) != GPIO_REGULATOR_SUPPLY_MAP_COUNT) {
>> +        dev_err(int3472->dev, "Internal error ARRAY_SIZE(skl_int3472_regulator_map_supplies) != GPIO_REGULATOR_SUPPLY_MAP_COUNT\n");
>> +        return -EINVAL;
> 
> 
> It would be nice to be able to prevent this mismatch somehow so it's never a problem; can we use static_assert() perhaps? Or at least less of a problem, as I gather that gets compiled to a no-op sometimes.

Using static_assert() in the header file indeed seems to be better. I'll prep a v2 with this (and drop the v1 from my review-hans branch).


>>       }
>>   -    if (!sensor_config->supply_map.supply) {
>> -        dev_err(int3472->dev, "No supply name defined\n");
>> -        return -ENODEV;
>> +    for (i = 0; i < ARRAY_SIZE(skl_int3472_regulator_map_supplies); i++) {
>> +        int3472->regulator.supply_map[i].supply = skl_int3472_regulator_map_supplies[i];
>> +        int3472->regulator.supply_map[i].dev_name = int3472->sensor_name;
>>       }
>>         init_data.constraints.valid_ops_mask = REGULATOR_CHANGE_STATUS;
>> -    init_data.num_consumer_supplies = 1;
>> -    supply_map = sensor_config->supply_map;
>> -    supply_map.dev_name = int3472->sensor_name;
>> -    init_data.consumer_supplies = &supply_map;
>> +    init_data.consumer_supplies = int3472->regulator.supply_map;
>> +    init_data.num_consumer_supplies = GPIO_REGULATOR_SUPPLY_MAP_COUNT;
>>         snprintf(int3472->regulator.regulator_name,
>>            sizeof(int3472->regulator.regulator_name), "%s-regulator",
>> diff --git a/drivers/platform/x86/intel/int3472/common.h b/drivers/platform/x86/intel/int3472/common.h
>> index 735567f374a6..225b067c854d 100644
>> --- a/drivers/platform/x86/intel/int3472/common.h
>> +++ b/drivers/platform/x86/intel/int3472/common.h
>> @@ -28,6 +28,7 @@
>>     #define GPIO_REGULATOR_NAME_LENGTH                21
>>   #define GPIO_REGULATOR_SUPPLY_NAME_LENGTH            9
>> +#define GPIO_REGULATOR_SUPPLY_MAP_COUNT                1
>>     #define INT3472_LED_MAX_NAME_LEN                32
>>   @@ -69,11 +70,6 @@ struct int3472_cldb {
>>       u8 reserved2[17];
>>   };
>>   -struct int3472_sensor_config {
>> -    const char *sensor_module_name;
>> -    struct regulator_consumer_supply supply_map;
>> -};
>> -
>>   struct int3472_discrete_device {
>>       struct acpi_device *adev;
>>       struct device *dev;
>> @@ -83,6 +79,7 @@ struct int3472_discrete_device {
>>       const struct int3472_sensor_config *sensor_config;
>>         struct int3472_gpio_regulator {
>> +        struct regulator_consumer_supply supply_map[GPIO_REGULATOR_SUPPLY_MAP_COUNT];
>>           char regulator_name[GPIO_REGULATOR_NAME_LENGTH];
>>           char supply_name[GPIO_REGULATOR_SUPPLY_NAME_LENGTH];
>>           struct gpio_desc *gpio;
>> diff --git a/drivers/platform/x86/intel/int3472/discrete.c b/drivers/platform/x86/intel/int3472/discrete.c
>> index 2ab3c7466986..3b410428cec2 100644
>> --- a/drivers/platform/x86/intel/int3472/discrete.c
>> +++ b/drivers/platform/x86/intel/int3472/discrete.c
>> @@ -34,48 +34,17 @@ static const guid_t cio2_sensor_module_guid =
>>       GUID_INIT(0x822ace8f, 0x2814, 0x4174,
>>             0xa5, 0x6b, 0x5f, 0x02, 0x9f, 0xe0, 0x79, 0xee);
>>   -/*
>> - * Here follows platform specific mapping information that we can pass to
>> - * the functions mapping resources to the sensors. Where the sensors have
>> - * a power enable pin defined in DSDT we need to provide a supply name so
>> - * the sensor drivers can find the regulator. The device name will be derived
>> - * from the sensor's ACPI device within the code.
>> - */
>> -static const struct int3472_sensor_config int3472_sensor_configs[] = {
>> -    /* Lenovo Miix 510-12ISK - OV5648, Rear */
>> -    { "GEFF150023R", REGULATOR_SUPPLY("avdd", NULL) },
>> -    /* Surface Go 1&2 - OV5693, Front */
>> -    { "YHCU", REGULATOR_SUPPLY("avdd", NULL) },
>> -};
>> -
>> -static const struct int3472_sensor_config *
>> -skl_int3472_get_sensor_module_config(struct int3472_discrete_device *int3472)
>> +static void skl_int3472_log_sensor_module_name(struct int3472_discrete_device *int3472)
> 
> 
> I don't really think this is worth logging if we're removing the matching based on it - we can get it from the DSDT anyway if we need to.

The DSDTs on these devices often read this from some memory location
rather then specifying it directly in the DSDT, so the only way to
get this often is to actually call the DSM.

Note this is logged with a dev_dbg, so normally users won't see this,
but I think this may be handy for debugging sometimes.

Regards,

Hans