[PATCH v2 0/9] platform/x86: int3472: Add handshake pin support

[PATCH v2 0/9] platform/x86: int3472: Add handshake pin support

[Hans de Goede]

New Intel Meteor Lake based laptops with IPU6 cameras have a new type 0x12
pin defined in the INT3472 sensor companion device which describes
the sensor's GPIOs.

This pin is primarily used on designs with a Lattice FPGA chip which is
capable of running the sensor independently of the main CPU for features
like presence detection. This pin needs to be driven high to make the FPGA
run the power-on sequence of the sensor. After driving the pin high
the FPGA "firmware" needs 25ms to comlpete the power-on sequence.

This series implements support for this by modelling the handshake GPIO
as a GPIO driven 'dvdd' regulator with an enable-time of 25 ms, also see:

https://lore.kernel.org/platform-driver-x86/59f672c3-6d87-4ec7-9b7f-f44fe2cce934@redhat.com/

Patch   1   Is an unrelated cleanup which I had lying around
Patches 2-7 Prepare + Implement the handshake GPIO
Patch   8   Is a small patch adding some extra debugging to GPIO remapping

Changes in v2:
- Add Andy's Reviewed-by to patches 1-3
- Address Andy's review remarks on patch 5
- Add 2 Tested-by tags to patch 8/9

This series applies on top of Torvald's latest master, for testing with
6.14 this patch needs to be cherry-picked first:
https://web.git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/commit/?id=81b251c66bdfe263fb5e7a16838512ddaeed77df

Regards,

Hans


Hans de Goede (9):
  platform/x86: int3472: Add skl_int3472_register_clock() helper
  platform/x86: int3472: Stop setting a supply-name for GPIO regulators
  platform/x86: int3472: Drop unused gpio field from struct
    int3472_gpio_regulator
  platform/x86: int3472: Rework AVDD second sensor quirk handling
  platform/x86: int3472: Make regulator supply name configurable
  platform/x86: int3472: Avoid GPIO regulator spikes
  platform/x86: int3472: Prepare for registering more then 1 GPIO
    regulator
  platform/x86: int3472: Add handshake pin support
  platform/x86: int3472: Debug log when remapping pins

 drivers/platform/x86/intel/int3472/Makefile   |   3 +-
 .../x86/intel/int3472/clk_and_regulator.c     | 164 ++++++------------
 drivers/platform/x86/intel/int3472/common.h   |  50 ++++--
 drivers/platform/x86/intel/int3472/discrete.c |  39 ++++-
 .../x86/intel/int3472/discrete_quirks.c       |  22 +++
 5 files changed, 147 insertions(+), 131 deletions(-)
 create mode 100644 drivers/platform/x86/intel/int3472/discrete_quirks.c

-- 
2.49.0

[PATCH v2 1/9] platform/x86: int3472: Add skl_int3472_register_clock() helper

[Hans de Goede]

skl_int3472_register_dsm_clock() and skl_int3472_register_gpio_clock() are
80% the same code. Factor out the common code into a new
skl_int3472_register_clock() helper.

Reviewed-by: Andy Shevchenko <andy@kernel.org>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 .../x86/intel/int3472/clk_and_regulator.c     | 57 +++++--------------
 1 file changed, 13 insertions(+), 44 deletions(-)

diff --git a/drivers/platform/x86/intel/int3472/clk_and_regulator.c b/drivers/platform/x86/intel/int3472/clk_and_regulator.c
index 16e36ac0a7b4..837990af24fe 100644
--- a/drivers/platform/x86/intel/int3472/clk_and_regulator.c
+++ b/drivers/platform/x86/intel/int3472/clk_and_regulator.c
@@ -118,7 +118,7 @@ static const struct clk_ops skl_int3472_clock_ops = {
 	.recalc_rate = skl_int3472_clk_recalc_rate,
 };
 
-int skl_int3472_register_dsm_clock(struct int3472_discrete_device *int3472)
+static int skl_int3472_register_clock(struct int3472_discrete_device *int3472)
 {
 	struct acpi_device *adev = int3472->adev;
 	struct clk_init_data init = {
@@ -127,12 +127,6 @@ int skl_int3472_register_dsm_clock(struct int3472_discrete_device *int3472)
 	};
 	int ret;
 
-	if (int3472->clock.cl)
-		return 0; /* A GPIO controlled clk has already been registered */
-
-	if (!acpi_check_dsm(adev->handle, &img_clk_guid, 0, BIT(1)))
-		return 0; /* DSM clock control is not available */
-
 	init.name = kasprintf(GFP_KERNEL, "%s-clk", acpi_dev_name(adev));
 	if (!init.name)
 		return -ENOMEM;
@@ -161,51 +155,26 @@ int skl_int3472_register_dsm_clock(struct int3472_discrete_device *int3472)
 	return ret;
 }
 
+int skl_int3472_register_dsm_clock(struct int3472_discrete_device *int3472)
+{
+	if (int3472->clock.cl)
+		return 0; /* A GPIO controlled clk has already been registered */
+
+	if (!acpi_check_dsm(int3472->adev->handle, &img_clk_guid, 0, BIT(1)))
+		return 0; /* DSM clock control is not available */
+
+	return skl_int3472_register_clock(int3472);
+}
+
 int skl_int3472_register_gpio_clock(struct int3472_discrete_device *int3472,
 				    struct gpio_desc *gpio)
 {
-	struct clk_init_data init = {
-		.ops = &skl_int3472_clock_ops,
-		.flags = CLK_GET_RATE_NOCACHE,
-	};
-	int ret;
-
 	if (int3472->clock.cl)
 		return -EBUSY;
 
 	int3472->clock.ena_gpio = gpio;
 
-	init.name = kasprintf(GFP_KERNEL, "%s-clk",
-			      acpi_dev_name(int3472->adev));
-	if (!init.name)
-		return -ENOMEM;
-
-	int3472->clock.frequency = skl_int3472_get_clk_frequency(int3472);
-
-	int3472->clock.clk_hw.init = &init;
-	int3472->clock.clk = clk_register(&int3472->adev->dev,
-					  &int3472->clock.clk_hw);
-	if (IS_ERR(int3472->clock.clk)) {
-		ret = PTR_ERR(int3472->clock.clk);
-		goto out_free_init_name;
-	}
-
-	int3472->clock.cl = clkdev_create(int3472->clock.clk, NULL,
-					  int3472->sensor_name);
-	if (!int3472->clock.cl) {
-		ret = -ENOMEM;
-		goto err_unregister_clk;
-	}
-
-	kfree(init.name);
-	return 0;
-
-err_unregister_clk:
-	clk_unregister(int3472->clock.clk);
-out_free_init_name:
-	kfree(init.name);
-
-	return ret;
+	return skl_int3472_register_clock(int3472);
 }
 
 void skl_int3472_unregister_clock(struct int3472_discrete_device *int3472)
-- 
2.49.0

[PATCH v2 2/9] platform/x86: int3472: Stop setting a supply-name for GPIO regulators

[Hans de Goede]

The supply_name field is not mandatory and is supposed to be set to
the name of another regulator when it is known that the regulator being
registered is supplied by that other regulator.

Having a regulator supplying the regulator which is being registered does
not apply to the INT3472 GPIO regulator, stop setting a supply_name.

Reviewed-by: Andy Shevchenko <andy@kernel.org>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/platform/x86/intel/int3472/clk_and_regulator.c | 3 ---
 drivers/platform/x86/intel/int3472/common.h            | 5 +----
 2 files changed, 1 insertion(+), 7 deletions(-)

diff --git a/drivers/platform/x86/intel/int3472/clk_and_regulator.c b/drivers/platform/x86/intel/int3472/clk_and_regulator.c
index 837990af24fe..40434591dd0b 100644
--- a/drivers/platform/x86/intel/int3472/clk_and_regulator.c
+++ b/drivers/platform/x86/intel/int3472/clk_and_regulator.c
@@ -256,12 +256,9 @@ int skl_int3472_register_regulator(struct int3472_discrete_device *int3472,
 	snprintf(int3472->regulator.regulator_name,
 		 sizeof(int3472->regulator.regulator_name), "%s-regulator",
 		 acpi_dev_name(int3472->adev));
-	snprintf(int3472->regulator.supply_name,
-		 GPIO_REGULATOR_SUPPLY_NAME_LENGTH, "supply-0");
 
 	int3472->regulator.rdesc = INT3472_REGULATOR(
 						int3472->regulator.regulator_name,
-						int3472->regulator.supply_name,
 						&int3472_gpio_regulator_ops);
 
 	int3472->regulator.gpio = gpio;
diff --git a/drivers/platform/x86/intel/int3472/common.h b/drivers/platform/x86/intel/int3472/common.h
index 145dec66df64..72ef222629b6 100644
--- a/drivers/platform/x86/intel/int3472/common.h
+++ b/drivers/platform/x86/intel/int3472/common.h
@@ -27,17 +27,15 @@
 #define INT3472_MAX_SENSOR_GPIOS				3
 
 #define GPIO_REGULATOR_NAME_LENGTH				21
-#define GPIO_REGULATOR_SUPPLY_NAME_LENGTH			9
 #define GPIO_REGULATOR_SUPPLY_MAP_COUNT				2
 
 #define INT3472_LED_MAX_NAME_LEN				32
 
 #define CIO2_SENSOR_SSDB_MCLKSPEED_OFFSET			86
 
-#define INT3472_REGULATOR(_name, _supply, _ops)			\
+#define INT3472_REGULATOR(_name, _ops)				\
 	(const struct regulator_desc) {				\
 		.name = _name,					\
-		.supply_name = _supply,				\
 		.type = REGULATOR_VOLTAGE,			\
 		.ops = _ops,					\
 		.owner = THIS_MODULE,				\
@@ -82,7 +80,6 @@ struct int3472_discrete_device {
 		/* 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;
 		struct regulator_dev *rdev;
 		struct regulator_desc rdesc;
-- 
2.49.0

[PATCH v2 3/9] platform/x86: int3472: Drop unused gpio field from struct int3472_gpio_regulator

[Hans de Goede]

The gpio field in struct int3472_gpio_regulator is only briefly used to
store the GPIO in skl_int3472_register_regulator(). Instead just store
the GPIO directly into cfg.ena_gpiod an drop the gpio field.

Reviewed-by: Andy Shevchenko <andy@kernel.org>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/platform/x86/intel/int3472/clk_and_regulator.c | 4 +---
 drivers/platform/x86/intel/int3472/common.h            | 1 -
 2 files changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/platform/x86/intel/int3472/clk_and_regulator.c b/drivers/platform/x86/intel/int3472/clk_and_regulator.c
index 40434591dd0b..5f6c66215f63 100644
--- a/drivers/platform/x86/intel/int3472/clk_and_regulator.c
+++ b/drivers/platform/x86/intel/int3472/clk_and_regulator.c
@@ -261,11 +261,9 @@ int skl_int3472_register_regulator(struct int3472_discrete_device *int3472,
 						int3472->regulator.regulator_name,
 						&int3472_gpio_regulator_ops);
 
-	int3472->regulator.gpio = gpio;
-
 	cfg.dev = &int3472->adev->dev;
 	cfg.init_data = &init_data;
-	cfg.ena_gpiod = int3472->regulator.gpio;
+	cfg.ena_gpiod = gpio;
 
 	int3472->regulator.rdev = regulator_register(int3472->dev,
 						     &int3472->regulator.rdesc,
diff --git a/drivers/platform/x86/intel/int3472/common.h b/drivers/platform/x86/intel/int3472/common.h
index 72ef222629b6..e0fa34be8a07 100644
--- a/drivers/platform/x86/intel/int3472/common.h
+++ b/drivers/platform/x86/intel/int3472/common.h
@@ -80,7 +80,6 @@ struct int3472_discrete_device {
 		/* 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];
-		struct gpio_desc *gpio;
 		struct regulator_dev *rdev;
 		struct regulator_desc rdesc;
 	} regulator;
-- 
2.49.0

[PATCH v2 4/9] platform/x86: int3472: Rework AVDD second sensor quirk handling

[Hans de Goede]

Rework the discrete quirk handling to use a quirks struct which is
pointed to by a dmi_system_id table, rather then having a dmi_system_id
table per type of quirk.

This is a preparation patch for adding support for multiple regulators,
where not all regulators might be shared between sensors.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/platform/x86/intel/int3472/Makefile   |  3 +-
 .../x86/intel/int3472/clk_and_regulator.c     | 28 ++-----------------
 drivers/platform/x86/intel/int3472/common.h   | 13 ++++++++-
 drivers/platform/x86/intel/int3472/discrete.c | 13 ++++++++-
 .../x86/intel/int3472/discrete_quirks.c       | 22 +++++++++++++++
 5 files changed, 50 insertions(+), 29 deletions(-)
 create mode 100644 drivers/platform/x86/intel/int3472/discrete_quirks.c

diff --git a/drivers/platform/x86/intel/int3472/Makefile b/drivers/platform/x86/intel/int3472/Makefile
index a8aba07bf1dc..103661e6685d 100644
--- a/drivers/platform/x86/intel/int3472/Makefile
+++ b/drivers/platform/x86/intel/int3472/Makefile
@@ -1,7 +1,8 @@
 obj-$(CONFIG_INTEL_SKL_INT3472)		+= intel_skl_int3472_discrete.o \
 					   intel_skl_int3472_tps68470.o \
 					   intel_skl_int3472_common.o
-intel_skl_int3472_discrete-y		:= discrete.o clk_and_regulator.o led.o
+intel_skl_int3472_discrete-y		:= discrete.o discrete_quirks.o \
+					   clk_and_regulator.o led.o
 intel_skl_int3472_tps68470-y		:= tps68470.o tps68470_board_data.o
 
 intel_skl_int3472_common-y		+= common.o
diff --git a/drivers/platform/x86/intel/int3472/clk_and_regulator.c b/drivers/platform/x86/intel/int3472/clk_and_regulator.c
index 5f6c66215f63..374a99dea7d1 100644
--- a/drivers/platform/x86/intel/int3472/clk_and_regulator.c
+++ b/drivers/platform/x86/intel/int3472/clk_and_regulator.c
@@ -5,7 +5,6 @@
 #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>
@@ -205,37 +204,14 @@ static const char * const skl_int3472_regulator_map_supplies[] = {
 static_assert(ARRAY_SIZE(skl_int3472_regulator_map_supplies) ==
 	      GPIO_REGULATOR_SUPPLY_MAP_COUNT);
 
-/*
- * 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.
- */
-static 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 gpio_desc *gpio)
+				   struct gpio_desc *gpio,
+				   const char *second_sensor)
 {
 	struct regulator_init_data init_data = { };
 	struct regulator_config cfg = { };
-	const char *second_sensor = NULL;
-	const struct dmi_system_id *id;
 	int i, j;
 
-	id = dmi_first_match(skl_int3472_regulator_second_sensor);
-	if (id)
-		second_sensor = id->driver_data;
-
 	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;
diff --git a/drivers/platform/x86/intel/int3472/common.h b/drivers/platform/x86/intel/int3472/common.h
index e0fa34be8a07..3728f3864a84 100644
--- a/drivers/platform/x86/intel/int3472/common.h
+++ b/drivers/platform/x86/intel/int3472/common.h
@@ -48,6 +48,7 @@
 	container_of(clk, struct int3472_discrete_device, clock)
 
 struct acpi_device;
+struct dmi_system_id;
 struct i2c_client;
 struct platform_device;
 
@@ -68,6 +69,11 @@ struct int3472_cldb {
 	u8 reserved2[17];
 };
 
+struct int3472_discrete_quirks {
+	/* For models where AVDD GPIO is shared between sensors */
+	const char *avdd_second_sensor;
+};
+
 struct int3472_discrete_device {
 	struct acpi_device *adev;
 	struct device *dev;
@@ -100,11 +106,15 @@ struct int3472_discrete_device {
 		struct gpio_desc *gpio;
 	} pled;
 
+	struct int3472_discrete_quirks quirks;
+
 	unsigned int ngpios; /* how many GPIOs have we seen */
 	unsigned int n_sensor_gpios; /* how many have we mapped to sensor */
 	struct gpiod_lookup_table gpios;
 };
 
+extern const struct dmi_system_id skl_int3472_discrete_quirks[];
+
 union acpi_object *skl_int3472_get_acpi_buffer(struct acpi_device *adev,
 					       char *id);
 int skl_int3472_fill_cldb(struct acpi_device *adev, struct int3472_cldb *cldb);
@@ -118,7 +128,8 @@ int skl_int3472_register_dsm_clock(struct int3472_discrete_device *int3472);
 void skl_int3472_unregister_clock(struct int3472_discrete_device *int3472);
 
 int skl_int3472_register_regulator(struct int3472_discrete_device *int3472,
-				   struct gpio_desc *gpio);
+				   struct gpio_desc *gpio,
+				   const char *second_sensor);
 void skl_int3472_unregister_regulator(struct int3472_discrete_device *int3472);
 
 int skl_int3472_register_pled(struct int3472_discrete_device *int3472, struct gpio_desc *gpio);
diff --git a/drivers/platform/x86/intel/int3472/discrete.c b/drivers/platform/x86/intel/int3472/discrete.c
index 30ff8f3ea1f5..cbf39459bdf0 100644
--- a/drivers/platform/x86/intel/int3472/discrete.c
+++ b/drivers/platform/x86/intel/int3472/discrete.c
@@ -5,6 +5,7 @@
 #include <linux/array_size.h>
 #include <linux/bitfield.h>
 #include <linux/device.h>
+#include <linux/dmi.h>
 #include <linux/gpio/consumer.h>
 #include <linux/gpio/machine.h>
 #include <linux/i2c.h>
@@ -310,7 +311,8 @@ static int skl_int3472_handle_gpio_resources(struct acpi_resource *ares,
 
 			break;
 		case INT3472_GPIO_TYPE_POWER_ENABLE:
-			ret = skl_int3472_register_regulator(int3472, gpio);
+			ret = skl_int3472_register_regulator(int3472, gpio,
+							     int3472->quirks.avdd_second_sensor);
 			if (ret)
 				err_msg = "Failed to map regulator to sensor\n";
 
@@ -378,13 +380,19 @@ static void skl_int3472_discrete_remove(struct platform_device *pdev)
 static int skl_int3472_discrete_probe(struct platform_device *pdev)
 {
 	struct acpi_device *adev = ACPI_COMPANION(&pdev->dev);
+	const struct int3472_discrete_quirks *quirks = NULL;
 	struct int3472_discrete_device *int3472;
+	const struct dmi_system_id *id;
 	struct int3472_cldb cldb;
 	int ret;
 
 	if (!adev)
 		return -ENODEV;
 
+	id = dmi_first_match(skl_int3472_discrete_quirks);
+	if (id)
+		quirks = id->driver_data;
+
 	ret = skl_int3472_fill_cldb(adev, &cldb);
 	if (ret) {
 		dev_err(&pdev->dev, "Couldn't fill CLDB structure\n");
@@ -408,6 +416,9 @@ static int skl_int3472_discrete_probe(struct platform_device *pdev)
 	platform_set_drvdata(pdev, int3472);
 	int3472->clock.imgclk_index = cldb.clock_source;
 
+	if (quirks)
+		int3472->quirks = *quirks;
+
 	ret = skl_int3472_get_sensor_adev_and_name(&pdev->dev, &int3472->sensor,
 						   &int3472->sensor_name);
 	if (ret)
diff --git a/drivers/platform/x86/intel/int3472/discrete_quirks.c b/drivers/platform/x86/intel/int3472/discrete_quirks.c
new file mode 100644
index 000000000000..bf88863803b2
--- /dev/null
+++ b/drivers/platform/x86/intel/int3472/discrete_quirks.c
@@ -0,0 +1,22 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Author: Hans de Goede <hansg@kernel.org> */
+
+#include <linux/dmi.h>
+
+#include "common.h"
+
+static const struct int3472_discrete_quirks lenovo_miix_510_quirks = {
+	.avdd_second_sensor = "i2c-OVTI2680:00",
+};
+
+const struct dmi_system_id skl_int3472_discrete_quirks[] = {
+	{
+		/* Lenovo Miix 510-12IKB */
+		.matches = {
+			DMI_MATCH(DMI_SYS_VENDOR, "LENOVO"),
+			DMI_MATCH(DMI_PRODUCT_VERSION, "MIIX 510-12IKB"),
+		},
+		.driver_data = (void *)&lenovo_miix_510_quirks,
+	},
+	{ }
+};
-- 
2.49.0

Re: [PATCH v2 4/9] platform/x86: int3472: Rework AVDD second sensor quirk handling

[Andy Shevchenko]

On Wed, Apr 2, 2025 at 11:25 PM Hans de Goede <hdegoede@redhat.com> wrote:
>
> Rework the discrete quirk handling to use a quirks struct which is
> pointed to by a dmi_system_id table, rather then having a dmi_system_id
> table per type of quirk.
>
> This is a preparation patch for adding support for multiple regulators,
> where not all regulators might be shared between sensors.

This might slow down the lookup, but it doesn't look like any critical
path. And I like having a separate file for DMI quirks.

Reviewed-by: Andy Shevchenko <andy@kernel.org>

-- 
With Best Regards,
Andy Shevchenko

[PATCH v2 5/9] platform/x86: int3472: Make regulator supply name configurable

[Hans de Goede]

This is a preparation patch for registering multiple regulators, which
requires a different supply-name for each regulator. Make supply-name
a parameter to skl_int3472_register_regulator() and use con-id to set it
so that the existing int3472_gpio_map remapping can be used with it.

Since supply-name now is a parameter, drop the fixed
skl_int3472_regulator_map_supplies[] array and instead add lower- +
upper-case mappings of the passed in supply-name to the regulator.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Changes in v2
- Use E2BIG instead of EOVERFLOW for too long supply-names
---
 .../x86/intel/int3472/clk_and_regulator.c     | 47 ++++++++-----------
 drivers/platform/x86/intel/int3472/common.h   |  6 ++-
 drivers/platform/x86/intel/int3472/discrete.c |  4 +-
 3 files changed, 26 insertions(+), 31 deletions(-)

diff --git a/drivers/platform/x86/intel/int3472/clk_and_regulator.c b/drivers/platform/x86/intel/int3472/clk_and_regulator.c
index 374a99dea7d1..a08b953b597a 100644
--- a/drivers/platform/x86/intel/int3472/clk_and_regulator.c
+++ b/drivers/platform/x86/intel/int3472/clk_and_regulator.c
@@ -185,42 +185,34 @@ void skl_int3472_unregister_clock(struct int3472_discrete_device *int3472)
 	clk_unregister(int3472->clock.clk);
 }
 
-/*
- * 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",
-	"AVDD",
-};
-
-static_assert(ARRAY_SIZE(skl_int3472_regulator_map_supplies) ==
-	      GPIO_REGULATOR_SUPPLY_MAP_COUNT);
-
 int skl_int3472_register_regulator(struct int3472_discrete_device *int3472,
 				   struct gpio_desc *gpio,
+				   const char *supply_name,
 				   const char *second_sensor)
 {
 	struct regulator_init_data init_data = { };
+	struct int3472_gpio_regulator *regulator;
 	struct regulator_config cfg = { };
 	int i, j;
 
-	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;
+	if (strlen(supply_name) >= GPIO_SUPPPLY_NAME_LENGTH) {
+		dev_err(int3472->dev, "supply-name '%s' length too long\n", supply_name);
+		return -E2BIG;
+	}
+
+	regulator = &int3472->regulator;
+	string_upper(regulator->supply_name_upper, supply_name);
+
+	for (i = 0, j = 0; i < GPIO_REGULATOR_SUPPLY_MAP_COUNT; i++) {
+		const char *supply = i ? regulator->supply_name_upper : supply_name;
+
+		regulator->supply_map[j].supply = supply;
+		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;
+			regulator->supply_map[j].supply = supply;
+			regulator->supply_map[j].dev_name = second_sensor;
 			j++;
 		}
 	}
@@ -229,9 +221,8 @@ int skl_int3472_register_regulator(struct int3472_discrete_device *int3472,
 	init_data.consumer_supplies = int3472->regulator.supply_map;
 	init_data.num_consumer_supplies = j;
 
-	snprintf(int3472->regulator.regulator_name,
-		 sizeof(int3472->regulator.regulator_name), "%s-regulator",
-		 acpi_dev_name(int3472->adev));
+	snprintf(regulator->regulator_name, sizeof(regulator->regulator_name), "%s-%s",
+		 acpi_dev_name(int3472->adev), supply_name);
 
 	int3472->regulator.rdesc = INT3472_REGULATOR(
 						int3472->regulator.regulator_name,
diff --git a/drivers/platform/x86/intel/int3472/common.h b/drivers/platform/x86/intel/int3472/common.h
index 3728f3864a84..b0638c29d847 100644
--- a/drivers/platform/x86/intel/int3472/common.h
+++ b/drivers/platform/x86/intel/int3472/common.h
@@ -26,7 +26,9 @@
 #define INT3472_PDEV_MAX_NAME_LEN				23
 #define INT3472_MAX_SENSOR_GPIOS				3
 
-#define GPIO_REGULATOR_NAME_LENGTH				21
+#define GPIO_SUPPPLY_NAME_LENGTH				5
+#define GPIO_REGULATOR_NAME_LENGTH				(12 + GPIO_SUPPPLY_NAME_LENGTH)
+/* lower- + upper-case mapping */
 #define GPIO_REGULATOR_SUPPLY_MAP_COUNT				2
 
 #define INT3472_LED_MAX_NAME_LEN				32
@@ -85,6 +87,7 @@ struct int3472_discrete_device {
 	struct int3472_gpio_regulator {
 		/* SUPPLY_MAP_COUNT * 2 to make room for second sensor mappings */
 		struct regulator_consumer_supply supply_map[GPIO_REGULATOR_SUPPLY_MAP_COUNT * 2];
+		char supply_name_upper[GPIO_SUPPPLY_NAME_LENGTH];
 		char regulator_name[GPIO_REGULATOR_NAME_LENGTH];
 		struct regulator_dev *rdev;
 		struct regulator_desc rdesc;
@@ -129,6 +132,7 @@ void skl_int3472_unregister_clock(struct int3472_discrete_device *int3472);
 
 int skl_int3472_register_regulator(struct int3472_discrete_device *int3472,
 				   struct gpio_desc *gpio,
+				   const char *supply_name,
 				   const char *second_sensor);
 void skl_int3472_unregister_regulator(struct int3472_discrete_device *int3472);
 
diff --git a/drivers/platform/x86/intel/int3472/discrete.c b/drivers/platform/x86/intel/int3472/discrete.c
index cbf39459bdf0..f6dae82739e5 100644
--- a/drivers/platform/x86/intel/int3472/discrete.c
+++ b/drivers/platform/x86/intel/int3472/discrete.c
@@ -188,7 +188,7 @@ static void int3472_get_con_id_and_polarity(struct acpi_device *adev, u8 *type,
 		*gpio_flags = GPIO_ACTIVE_HIGH;
 		break;
 	case INT3472_GPIO_TYPE_POWER_ENABLE:
-		*con_id = "power-enable";
+		*con_id = "avdd";
 		*gpio_flags = GPIO_ACTIVE_HIGH;
 		break;
 	default:
@@ -311,7 +311,7 @@ static int skl_int3472_handle_gpio_resources(struct acpi_resource *ares,
 
 			break;
 		case INT3472_GPIO_TYPE_POWER_ENABLE:
-			ret = skl_int3472_register_regulator(int3472, gpio,
+			ret = skl_int3472_register_regulator(int3472, gpio, con_id,
 							     int3472->quirks.avdd_second_sensor);
 			if (ret)
 				err_msg = "Failed to map regulator to sensor\n";
-- 
2.49.0

Re: [PATCH v2 5/9] platform/x86: int3472: Make regulator supply name configurable

[Andy Shevchenko]

On Wed, Apr 2, 2025 at 11:25 PM Hans de Goede <hdegoede@redhat.com> wrote:
>
> This is a preparation patch for registering multiple regulators, which
> requires a different supply-name for each regulator. Make supply-name
> a parameter to skl_int3472_register_regulator() and use con-id to set it
> so that the existing int3472_gpio_map remapping can be used with it.
>
> Since supply-name now is a parameter, drop the fixed
> skl_int3472_regulator_map_supplies[] array and instead add lower- +

+ --> and ?

> upper-case mappings of the passed in supply-name to the regulator.

passed-in ?

...

Would be nice to have a comment explaining magic 12
> +#define GPIO_REGULATOR_NAME_LENGTH                             (12 + GPIO_SUPPPLY_NAME_LENGTH)

...

> +/* lower- + upper-case mapping */

+ --> and ?

-- 
With Best Regards,
Andy Shevchenko

[PATCH v2 6/9] platform/x86: int3472: Avoid GPIO regulator spikes

[Hans de Goede]

Avoid the GPIO controlling the avdd regulator being driven low or high
for a very short time leading to spikes by adding an enable delay of 2 ms
and a minimum off to on delay of also 2 ms.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/platform/x86/intel/int3472/clk_and_regulator.c | 7 ++++---
 drivers/platform/x86/intel/int3472/common.h            | 8 +++++++-
 drivers/platform/x86/intel/int3472/discrete.c          | 4 +++-
 3 files changed, 14 insertions(+), 5 deletions(-)

diff --git a/drivers/platform/x86/intel/int3472/clk_and_regulator.c b/drivers/platform/x86/intel/int3472/clk_and_regulator.c
index a08b953b597a..b4af17e8dcaf 100644
--- a/drivers/platform/x86/intel/int3472/clk_and_regulator.c
+++ b/drivers/platform/x86/intel/int3472/clk_and_regulator.c
@@ -187,6 +187,7 @@ void skl_int3472_unregister_clock(struct int3472_discrete_device *int3472)
 
 int skl_int3472_register_regulator(struct int3472_discrete_device *int3472,
 				   struct gpio_desc *gpio,
+				   unsigned int enable_time,
 				   const char *supply_name,
 				   const char *second_sensor)
 {
@@ -224,9 +225,9 @@ int skl_int3472_register_regulator(struct int3472_discrete_device *int3472,
 	snprintf(regulator->regulator_name, sizeof(regulator->regulator_name), "%s-%s",
 		 acpi_dev_name(int3472->adev), supply_name);
 
-	int3472->regulator.rdesc = INT3472_REGULATOR(
-						int3472->regulator.regulator_name,
-						&int3472_gpio_regulator_ops);
+	regulator->rdesc = INT3472_REGULATOR(regulator->regulator_name,
+					     &int3472_gpio_regulator_ops,
+					     enable_time, GPIO_REGULATOR_OFF_ON_DELAY);
 
 	cfg.dev = &int3472->adev->dev;
 	cfg.init_data = &init_data;
diff --git a/drivers/platform/x86/intel/int3472/common.h b/drivers/platform/x86/intel/int3472/common.h
index b0638c29d847..271c23adb8ab 100644
--- a/drivers/platform/x86/intel/int3472/common.h
+++ b/drivers/platform/x86/intel/int3472/common.h
@@ -30,17 +30,22 @@
 #define GPIO_REGULATOR_NAME_LENGTH				(12 + GPIO_SUPPPLY_NAME_LENGTH)
 /* lower- + upper-case mapping */
 #define GPIO_REGULATOR_SUPPLY_MAP_COUNT				2
+/* Ensure the GPIO is driven low/high for at least 2 ms before changing */
+#define GPIO_REGULATOR_ENABLE_TIME				(2 * USEC_PER_MSEC)
+#define GPIO_REGULATOR_OFF_ON_DELAY				(2 * USEC_PER_MSEC)
 
 #define INT3472_LED_MAX_NAME_LEN				32
 
 #define CIO2_SENSOR_SSDB_MCLKSPEED_OFFSET			86
 
-#define INT3472_REGULATOR(_name, _ops)				\
+#define INT3472_REGULATOR(_name, _ops, _enable_time, _off_on_delay) \
 	(const struct regulator_desc) {				\
 		.name = _name,					\
 		.type = REGULATOR_VOLTAGE,			\
 		.ops = _ops,					\
 		.owner = THIS_MODULE,				\
+		.enable_time = _enable_time,			\
+		.off_on_delay = _off_on_delay,			\
 	}
 
 #define to_int3472_clk(hw)					\
@@ -132,6 +137,7 @@ void skl_int3472_unregister_clock(struct int3472_discrete_device *int3472);
 
 int skl_int3472_register_regulator(struct int3472_discrete_device *int3472,
 				   struct gpio_desc *gpio,
+				   unsigned int enable_time,
 				   const char *supply_name,
 				   const char *second_sensor);
 void skl_int3472_unregister_regulator(struct int3472_discrete_device *int3472);
diff --git a/drivers/platform/x86/intel/int3472/discrete.c b/drivers/platform/x86/intel/int3472/discrete.c
index f6dae82739e5..a2db4fae0e6d 100644
--- a/drivers/platform/x86/intel/int3472/discrete.c
+++ b/drivers/platform/x86/intel/int3472/discrete.c
@@ -311,7 +311,9 @@ static int skl_int3472_handle_gpio_resources(struct acpi_resource *ares,
 
 			break;
 		case INT3472_GPIO_TYPE_POWER_ENABLE:
-			ret = skl_int3472_register_regulator(int3472, gpio, con_id,
+			ret = skl_int3472_register_regulator(int3472, gpio,
+							     GPIO_REGULATOR_ENABLE_TIME,
+							     con_id,
 							     int3472->quirks.avdd_second_sensor);
 			if (ret)
 				err_msg = "Failed to map regulator to sensor\n";
-- 
2.49.0

Re: [PATCH v2 6/9] platform/x86: int3472: Avoid GPIO regulator spikes

[Andy Shevchenko]

On Wed, Apr 2, 2025 at 11:25 PM Hans de Goede <hdegoede@redhat.com> wrote:
>
> Avoid the GPIO controlling the avdd regulator being driven low or high
> for a very short time leading to spikes by adding an enable delay of 2 ms
> and a minimum off to on delay of also 2 ms.

How 2ms was derived? From experiments or is there any other hint
(Intel published sources somewhere, etc)?

The code looks good to me
Reviewed-by: Andy Shevchenko <andy@kernel.org>

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 6/9] platform/x86: int3472: Avoid GPIO regulator spikes

[Hans de Goede]

Hi,

On 2-Apr-25 11:04 PM, Andy Shevchenko wrote:
> On Wed, Apr 2, 2025 at 11:25 PM Hans de Goede <hdegoede@redhat.com> wrote:
>>
>> Avoid the GPIO controlling the avdd regulator being driven low or high
>> for a very short time leading to spikes by adding an enable delay of 2 ms
>> and a minimum off to on delay of also 2 ms.
> 
> How 2ms was derived? From experiments or is there any other hint
> (Intel published sources somewhere, etc)?

2 ms is the minimum time ovXXXX sensors need to have there
reset line driven logical high to properly register the reset.

So that seemed like a sane value to use here to me.

> The code looks good to me
> Reviewed-by: Andy Shevchenko <andy@kernel.org>

Regards,

Hans

Re: [PATCH v2 6/9] platform/x86: int3472: Avoid GPIO regulator spikes

[Andy Shevchenko]

On Thu, Apr 03, 2025 at 11:43:34AM +0200, Hans de Goede wrote:
> On 2-Apr-25 11:04 PM, Andy Shevchenko wrote:
> > On Wed, Apr 2, 2025 at 11:25 PM Hans de Goede <hdegoede@redhat.com> wrote:
> >>
> >> Avoid the GPIO controlling the avdd regulator being driven low or high
> >> for a very short time leading to spikes by adding an enable delay of 2 ms
> >> and a minimum off to on delay of also 2 ms.
> > 
> > How 2ms was derived? From experiments or is there any other hint
> > (Intel published sources somewhere, etc)?
> 
> 2 ms is the minimum time ovXXXX sensors need to have there
> reset line driven logical high to properly register the reset.
> 
> So that seemed like a sane value to use here to me.

Please, add it in the commit message / comment in next version.

-- 
With Best Regards,
Andy Shevchenko

[PATCH v2 7/9] platform/x86: int3472: Prepare for registering more then 1 GPIO regulator

[Hans de Goede]

Prepare the discrete code for registering more then 1 GPIO regulator.

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

diff --git a/drivers/platform/x86/intel/int3472/clk_and_regulator.c b/drivers/platform/x86/intel/int3472/clk_and_regulator.c
index b4af17e8dcaf..e4bb93892104 100644
--- a/drivers/platform/x86/intel/int3472/clk_and_regulator.c
+++ b/drivers/platform/x86/intel/int3472/clk_and_regulator.c
@@ -196,12 +196,17 @@ int skl_int3472_register_regulator(struct int3472_discrete_device *int3472,
 	struct regulator_config cfg = { };
 	int i, j;
 
+	if (int3472->n_regulators >= INT3472_MAX_REGULATORS) {
+		dev_err(int3472->dev, "Too many regulators mapped\n");
+		return -EINVAL;
+	}
+
 	if (strlen(supply_name) >= GPIO_SUPPPLY_NAME_LENGTH) {
 		dev_err(int3472->dev, "supply-name '%s' length too long\n", supply_name);
 		return -E2BIG;
 	}
 
-	regulator = &int3472->regulator;
+	regulator = &int3472->regulators[int3472->n_regulators];
 	string_upper(regulator->supply_name_upper, supply_name);
 
 	for (i = 0, j = 0; i < GPIO_REGULATOR_SUPPLY_MAP_COUNT; i++) {
@@ -219,7 +224,7 @@ int skl_int3472_register_regulator(struct int3472_discrete_device *int3472,
 	}
 
 	init_data.constraints.valid_ops_mask = REGULATOR_CHANGE_STATUS;
-	init_data.consumer_supplies = int3472->regulator.supply_map;
+	init_data.consumer_supplies = regulator->supply_map;
 	init_data.num_consumer_supplies = j;
 
 	snprintf(regulator->regulator_name, sizeof(regulator->regulator_name), "%s-%s",
@@ -233,14 +238,16 @@ int skl_int3472_register_regulator(struct int3472_discrete_device *int3472,
 	cfg.init_data = &init_data;
 	cfg.ena_gpiod = gpio;
 
-	int3472->regulator.rdev = regulator_register(int3472->dev,
-						     &int3472->regulator.rdesc,
-						     &cfg);
+	regulator->rdev = regulator_register(int3472->dev, &regulator->rdesc, &cfg);
+	if (IS_ERR(regulator->rdev))
+		return PTR_ERR(regulator->rdev);
 
-	return PTR_ERR_OR_ZERO(int3472->regulator.rdev);
+	int3472->n_regulators++;
+	return 0;
 }
 
 void skl_int3472_unregister_regulator(struct int3472_discrete_device *int3472)
 {
-	regulator_unregister(int3472->regulator.rdev);
+	for (int i = 0; i < int3472->n_regulators; i++)
+		regulator_unregister(int3472->regulators[i].rdev);
 }
diff --git a/drivers/platform/x86/intel/int3472/common.h b/drivers/platform/x86/intel/int3472/common.h
index 271c23adb8ab..cd94346270c5 100644
--- a/drivers/platform/x86/intel/int3472/common.h
+++ b/drivers/platform/x86/intel/int3472/common.h
@@ -25,6 +25,7 @@
 
 #define INT3472_PDEV_MAX_NAME_LEN				23
 #define INT3472_MAX_SENSOR_GPIOS				3
+#define INT3472_MAX_REGULATORS					3
 
 #define GPIO_SUPPPLY_NAME_LENGTH				5
 #define GPIO_REGULATOR_NAME_LENGTH				(12 + GPIO_SUPPPLY_NAME_LENGTH)
@@ -81,6 +82,15 @@ struct int3472_discrete_quirks {
 	const char *avdd_second_sensor;
 };
 
+struct int3472_gpio_regulator {
+	/* SUPPLY_MAP_COUNT * 2 to make room for second sensor mappings */
+	struct regulator_consumer_supply supply_map[GPIO_REGULATOR_SUPPLY_MAP_COUNT * 2];
+	char supply_name_upper[GPIO_SUPPPLY_NAME_LENGTH];
+	char regulator_name[GPIO_REGULATOR_NAME_LENGTH];
+	struct regulator_dev *rdev;
+	struct regulator_desc rdesc;
+};
+
 struct int3472_discrete_device {
 	struct acpi_device *adev;
 	struct device *dev;
@@ -89,14 +99,7 @@ struct int3472_discrete_device {
 
 	const struct int3472_sensor_config *sensor_config;
 
-	struct int3472_gpio_regulator {
-		/* SUPPLY_MAP_COUNT * 2 to make room for second sensor mappings */
-		struct regulator_consumer_supply supply_map[GPIO_REGULATOR_SUPPLY_MAP_COUNT * 2];
-		char supply_name_upper[GPIO_SUPPPLY_NAME_LENGTH];
-		char regulator_name[GPIO_REGULATOR_NAME_LENGTH];
-		struct regulator_dev *rdev;
-		struct regulator_desc rdesc;
-	} regulator;
+	struct int3472_gpio_regulator regulators[INT3472_MAX_REGULATORS];
 
 	struct int3472_clock {
 		struct clk *clk;
@@ -118,6 +121,7 @@ struct int3472_discrete_device {
 
 	unsigned int ngpios; /* how many GPIOs have we seen */
 	unsigned int n_sensor_gpios; /* how many have we mapped to sensor */
+	unsigned int n_regulators; /* how many have we mapped to a regulator */
 	struct gpiod_lookup_table gpios;
 };
 
-- 
2.49.0

Re: [PATCH v2 7/9] platform/x86: int3472: Prepare for registering more then 1 GPIO regulator

[Andy Shevchenko]

On Wed, Apr 2, 2025 at 11:25 PM Hans de Goede <hdegoede@redhat.com> wrote:
>
> Prepare the discrete code for registering more then 1 GPIO regulator.

than

...

>         unsigned int ngpios; /* how many GPIOs have we seen */
>         unsigned int n_sensor_gpios; /* how many have we mapped to sensor */
> +       unsigned int n_regulators; /* how many have we mapped to a regulator */

Looking at the above and the comment I would expect the name to be
n_regulator_gpios...

>         struct gpiod_lookup_table gpios;

-- 
With Best Regards,
Andy Shevchenko

[PATCH v2 8/9] platform/x86: int3472: Add handshake pin support

[Hans de Goede]

New Intel Meteor Lake based laptops with IPU6 cameras have a new type 0x12
pin defined in the INT3472 sensor companion device which describes
the sensor's GPIOs.

This pin is primarily used on designs with a Lattice FPGA chip which is
capable of running the sensor independently of the main CPU for features
like presence detection. This pin needs to be driven high to make the FPGA
run the power-on sequence of the sensor. After driving the pin high
the FPGA "firmware" needs 25ms to comlpete the power-on sequence.

Add support for this modelling the handshake pin as a GPIO driven "dvdd"
regulator with a 25 ms enable time. This model was chosen because:

1. Sensor chips don't have a handshake pin, so we need to abstract this
   in some way which does not require modification to the sensor drivers,
   sensor drivers using the bulk-regulator API to get avdd + vddio + dvdd
   is normal. So this will work to get the right value set to the handshake
   pin without requiring sensor driver modifications.

2. Sensors typically wait only a small time for the sensor to power-on
   after de-asserting reset. Not the 25ms the Lattice chip requires.
   Using the regulator framework's enable_time allows hiding the need for
   this delay from the sensor drivers.

Link: https://lore.kernel.org/platform-driver-x86/59f672c3-6d87-4ec7-9b7f-f44fe2cce934@redhat.com/
Link: https://bugzilla.redhat.com/show_bug.cgi?id=2341731
Tested-by: Alan Stern <stern@rowland.harvard.edu> # Dell Latitude 7450, ov02e10
Tested-by: Duane <duanek@chorus.net> # HP Spectre x360 16t-aa000, ov08x40
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 .../platform/x86/intel/int3472/clk_and_regulator.c |  1 +
 drivers/platform/x86/intel/int3472/common.h        |  1 +
 drivers/platform/x86/intel/int3472/discrete.c      | 14 ++++++++++++++
 3 files changed, 16 insertions(+)

diff --git a/drivers/platform/x86/intel/int3472/clk_and_regulator.c b/drivers/platform/x86/intel/int3472/clk_and_regulator.c
index e4bb93892104..04cc52d28f16 100644
--- a/drivers/platform/x86/intel/int3472/clk_and_regulator.c
+++ b/drivers/platform/x86/intel/int3472/clk_and_regulator.c
@@ -224,6 +224,7 @@ int skl_int3472_register_regulator(struct int3472_discrete_device *int3472,
 	}
 
 	init_data.constraints.valid_ops_mask = REGULATOR_CHANGE_STATUS;
+	init_data.constraints.enable_time = enable_time;
 	init_data.consumer_supplies = regulator->supply_map;
 	init_data.num_consumer_supplies = j;
 
diff --git a/drivers/platform/x86/intel/int3472/common.h b/drivers/platform/x86/intel/int3472/common.h
index cd94346270c5..c7b8f0224320 100644
--- a/drivers/platform/x86/intel/int3472/common.h
+++ b/drivers/platform/x86/intel/int3472/common.h
@@ -22,6 +22,7 @@
 #define INT3472_GPIO_TYPE_POWER_ENABLE				0x0b
 #define INT3472_GPIO_TYPE_CLK_ENABLE				0x0c
 #define INT3472_GPIO_TYPE_PRIVACY_LED				0x0d
+#define INT3472_GPIO_TYPE_HANDSHAKE				0x12
 
 #define INT3472_PDEV_MAX_NAME_LEN				23
 #define INT3472_MAX_SENSOR_GPIOS				3
diff --git a/drivers/platform/x86/intel/int3472/discrete.c b/drivers/platform/x86/intel/int3472/discrete.c
index a2db4fae0e6d..4dddba4e1fde 100644
--- a/drivers/platform/x86/intel/int3472/discrete.c
+++ b/drivers/platform/x86/intel/int3472/discrete.c
@@ -191,6 +191,10 @@ static void int3472_get_con_id_and_polarity(struct acpi_device *adev, u8 *type,
 		*con_id = "avdd";
 		*gpio_flags = GPIO_ACTIVE_HIGH;
 		break;
+	case INT3472_GPIO_TYPE_HANDSHAKE:
+		*con_id = "dvdd";
+		*gpio_flags = GPIO_ACTIVE_HIGH;
+		break;
 	default:
 		*con_id = "unknown";
 		*gpio_flags = GPIO_ACTIVE_HIGH;
@@ -290,6 +294,7 @@ static int skl_int3472_handle_gpio_resources(struct acpi_resource *ares,
 	case INT3472_GPIO_TYPE_CLK_ENABLE:
 	case INT3472_GPIO_TYPE_PRIVACY_LED:
 	case INT3472_GPIO_TYPE_POWER_ENABLE:
+	case INT3472_GPIO_TYPE_HANDSHAKE:
 		gpio = skl_int3472_gpiod_get_from_temp_lookup(int3472, agpio, con_id, gpio_flags);
 		if (IS_ERR(gpio)) {
 			ret = PTR_ERR(gpio);
@@ -318,6 +323,15 @@ static int skl_int3472_handle_gpio_resources(struct acpi_resource *ares,
 			if (ret)
 				err_msg = "Failed to map regulator to sensor\n";
 
+			break;
+		case INT3472_GPIO_TYPE_HANDSHAKE:
+			/* Setups using a handshake pin need 25 ms enable delay */
+			ret = skl_int3472_register_regulator(int3472, gpio,
+							     25 * USEC_PER_MSEC,
+							     con_id, NULL);
+			if (ret)
+				err_msg = "Failed to map regulator to sensor\n";
+
 			break;
 		default: /* Never reached */
 			ret = -EINVAL;
-- 
2.49.0

Re: [PATCH v2 8/9] platform/x86: int3472: Add handshake pin support

[Andy Shevchenko]

On Wed, Apr 2, 2025 at 11:25 PM Hans de Goede <hdegoede@redhat.com> wrote:
>
> New Intel Meteor Lake based laptops with IPU6 cameras have a new type 0x12
> pin defined in the INT3472 sensor companion device which describes
> the sensor's GPIOs.
>
> This pin is primarily used on designs with a Lattice FPGA chip which is
> capable of running the sensor independently of the main CPU for features
> like presence detection. This pin needs to be driven high to make the FPGA
> run the power-on sequence of the sensor. After driving the pin high

high,

(note comma) ?

> the FPGA "firmware" needs 25ms to comlpete the power-on sequence.

complete

> Add support for this modelling the handshake pin as a GPIO driven "dvdd"
> regulator with a 25 ms enable time. This model was chosen because:
>
> 1. Sensor chips don't have a handshake pin, so we need to abstract this
>    in some way which does not require modification to the sensor drivers,
>    sensor drivers using the bulk-regulator API to get avdd + vddio + dvdd
>    is normal. So this will work to get the right value set to the handshake
>    pin without requiring sensor driver modifications.
>
> 2. Sensors typically wait only a small time for the sensor to power-on
>    after de-asserting reset. Not the 25ms the Lattice chip requires.
>    Using the regulator framework's enable_time allows hiding the need for
>    this delay from the sensor drivers.

...

>                         if (ret)
>                                 err_msg = "Failed to map regulator to sensor\n";
>
> +                       break;
> +               case INT3472_GPIO_TYPE_HANDSHAKE:
> +                       /* Setups using a handshake pin need 25 ms enable delay */
> +                       ret = skl_int3472_register_regulator(int3472, gpio,
> +                                                            25 * USEC_PER_MSEC,
> +                                                            con_id, NULL);
> +                       if (ret)
> +                               err_msg = "Failed to map regulator to sensor\n";

A copy and paste mistake? Yes, I know that they are both represented
as regulators, but don't we want a bit of uniqueness in the messages?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 8/9] platform/x86: int3472: Add handshake pin support

[Hans de Goede]

Hi Andy,

Thank you for all the reviews.

On 2-Apr-25 10:56 PM, Andy Shevchenko wrote:
> On Wed, Apr 2, 2025 at 11:25 PM Hans de Goede <hdegoede@redhat.com> wrote:
>>
>> New Intel Meteor Lake based laptops with IPU6 cameras have a new type 0x12
>> pin defined in the INT3472 sensor companion device which describes
>> the sensor's GPIOs.
>>
>> This pin is primarily used on designs with a Lattice FPGA chip which is
>> capable of running the sensor independently of the main CPU for features
>> like presence detection. This pin needs to be driven high to make the FPGA
>> run the power-on sequence of the sensor. After driving the pin high
> 
> high,
> 
> (note comma) ?
> 
>> the FPGA "firmware" needs 25ms to comlpete the power-on sequence.
> 
> complete
> 
>> Add support for this modelling the handshake pin as a GPIO driven "dvdd"
>> regulator with a 25 ms enable time. This model was chosen because:
>>
>> 1. Sensor chips don't have a handshake pin, so we need to abstract this
>>    in some way which does not require modification to the sensor drivers,
>>    sensor drivers using the bulk-regulator API to get avdd + vddio + dvdd
>>    is normal. So this will work to get the right value set to the handshake
>>    pin without requiring sensor driver modifications.
>>
>> 2. Sensors typically wait only a small time for the sensor to power-on
>>    after de-asserting reset. Not the 25ms the Lattice chip requires.
>>    Using the regulator framework's enable_time allows hiding the need for
>>    this delay from the sensor drivers.
> 
> ...
> 
>>                         if (ret)
>>                                 err_msg = "Failed to map regulator to sensor\n";
>>
>> +                       break;
>> +               case INT3472_GPIO_TYPE_HANDSHAKE:
>> +                       /* Setups using a handshake pin need 25 ms enable delay */
>> +                       ret = skl_int3472_register_regulator(int3472, gpio,
>> +                                                            25 * USEC_PER_MSEC,
>> +                                                            con_id, NULL);
>> +                       if (ret)
>> +                               err_msg = "Failed to map regulator to sensor\n";
> 
> A copy and paste mistake? Yes, I know that they are both represented
> as regulators, but don't we want a bit of uniqueness in the messages?

I actually did this on purpose to allow the compiler to use a single string
for these saving some space. The difference of which case we hit should be clear
from the earlier printed (dbg) message printed above the switch-case.

As for all your other remarks I agree and I'll fix them for v3.

Regards,

Hans

Re: [PATCH v2 8/9] platform/x86: int3472: Add handshake pin support

[Andy Shevchenko]

On Wed, Apr 16, 2025 at 2:06 PM Hans de Goede <hdegoede@redhat.com> wrote:
> On 2-Apr-25 10:56 PM, Andy Shevchenko wrote:
> > On Wed, Apr 2, 2025 at 11:25 PM Hans de Goede <hdegoede@redhat.com> wrote:

...

> >>                         if (ret)
> >>                                 err_msg = "Failed to map regulator to sensor\n";
> >>
> >> +                       break;
> >> +               case INT3472_GPIO_TYPE_HANDSHAKE:
> >> +                       /* Setups using a handshake pin need 25 ms enable delay */
> >> +                       ret = skl_int3472_register_regulator(int3472, gpio,
> >> +                                                            25 * USEC_PER_MSEC,
> >> +                                                            con_id, NULL);
> >> +                       if (ret)
> >> +                               err_msg = "Failed to map regulator to sensor\n";
> >
> > A copy and paste mistake? Yes, I know that they are both represented
> > as regulators, but don't we want a bit of uniqueness in the messages?
>
> I actually did this on purpose to allow the compiler to use a single string
> for these saving some space. The difference of which case we hit should be clear
> from the earlier printed (dbg) message printed above the switch-case.

I understand the idea, but if debug messages are off, how do we know
that? Or err_msg here is also a debug leveled one?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 8/9] platform/x86: int3472: Add handshake pin support

[Andy Shevchenko]

On Wed, Apr 16, 2025 at 4:09 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
> On Wed, Apr 16, 2025 at 2:06 PM Hans de Goede <hdegoede@redhat.com> wrote:
> > On 2-Apr-25 10:56 PM, Andy Shevchenko wrote:
> > > On Wed, Apr 2, 2025 at 11:25 PM Hans de Goede <hdegoede@redhat.com> wrote:

...

> > >>                         if (ret)
> > >>                                 err_msg = "Failed to map regulator to sensor\n";
> > >>
> > >> +                       break;
> > >> +               case INT3472_GPIO_TYPE_HANDSHAKE:
> > >> +                       /* Setups using a handshake pin need 25 ms enable delay */
> > >> +                       ret = skl_int3472_register_regulator(int3472, gpio,
> > >> +                                                            25 * USEC_PER_MSEC,
> > >> +                                                            con_id, NULL);
> > >> +                       if (ret)
> > >> +                               err_msg = "Failed to map regulator to sensor\n";
> > >
> > > A copy and paste mistake? Yes, I know that they are both represented
> > > as regulators, but don't we want a bit of uniqueness in the messages?
> >
> > I actually did this on purpose to allow the compiler to use a single string
> > for these saving some space. The difference of which case we hit should be clear
> > from the earlier printed (dbg) message printed above the switch-case.
>
> I understand the idea, but if debug messages are off, how do we know
> that? Or err_msg here is also a debug leveled one?

FWIW, you may use "... %s ...", regulator->name (maybe, I haven't
checked) or similar, so the format string is the same.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 8/9] platform/x86: int3472: Add handshake pin support

[Hans de Goede]

Hi,

On 16-Apr-25 3:09 PM, Andy Shevchenko wrote:
> On Wed, Apr 16, 2025 at 4:09 PM Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
>> On Wed, Apr 16, 2025 at 2:06 PM Hans de Goede <hdegoede@redhat.com> wrote:
>>> On 2-Apr-25 10:56 PM, Andy Shevchenko wrote:
>>>> On Wed, Apr 2, 2025 at 11:25 PM Hans de Goede <hdegoede@redhat.com> wrote:
> 
> ...
> 
>>>>>                         if (ret)
>>>>>                                 err_msg = "Failed to map regulator to sensor\n";
>>>>>
>>>>> +                       break;
>>>>> +               case INT3472_GPIO_TYPE_HANDSHAKE:
>>>>> +                       /* Setups using a handshake pin need 25 ms enable delay */
>>>>> +                       ret = skl_int3472_register_regulator(int3472, gpio,
>>>>> +                                                            25 * USEC_PER_MSEC,
>>>>> +                                                            con_id, NULL);
>>>>> +                       if (ret)
>>>>> +                               err_msg = "Failed to map regulator to sensor\n";
>>>>
>>>> A copy and paste mistake? Yes, I know that they are both represented
>>>> as regulators, but don't we want a bit of uniqueness in the messages?
>>>
>>> I actually did this on purpose to allow the compiler to use a single string
>>> for these saving some space. The difference of which case we hit should be clear
>>> from the earlier printed (dbg) message printed above the switch-case.
>>
>> I understand the idea, but if debug messages are off, how do we know
>> that? Or err_msg here is also a debug leveled one?
> 
> FWIW, you may use "... %s ...", regulator->name (maybe, I haven't
> checked) or similar, so the format string is the same.

That is a good idea, since I've already send out v3, I'll do so for v4.

But first I'll give you some time to review the unreviewed / modified
patches in v3, so that v4 can hopefully be the last version.

Regards,

Hans

[PATCH v2 9/9] platform/x86: int3472: Debug log when remapping pins

[Hans de Goede]

Debug log when remapping a pin/function because of a int3472_gpio_map[]
match.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/platform/x86/intel/int3472/discrete.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/platform/x86/intel/int3472/discrete.c b/drivers/platform/x86/intel/int3472/discrete.c
index 4dddba4e1fde..9fd4fce7063c 100644
--- a/drivers/platform/x86/intel/int3472/discrete.c
+++ b/drivers/platform/x86/intel/int3472/discrete.c
@@ -146,9 +146,10 @@ static const struct int3472_gpio_map int3472_gpio_map[] = {
 	{ "INT347E", INT3472_GPIO_TYPE_RESET, INT3472_GPIO_TYPE_RESET, false, "enable" },
 };
 
-static void int3472_get_con_id_and_polarity(struct acpi_device *adev, u8 *type,
+static void int3472_get_con_id_and_polarity(struct int3472_discrete_device *int3472, u8 *type,
 					    const char **con_id, unsigned long *gpio_flags)
 {
+	struct acpi_device *adev = int3472->sensor;
 	unsigned int i;
 
 	for (i = 0; i < ARRAY_SIZE(int3472_gpio_map); i++) {
@@ -163,6 +164,9 @@ static void int3472_get_con_id_and_polarity(struct acpi_device *adev, u8 *type,
 		if (!acpi_dev_hid_uid_match(adev, int3472_gpio_map[i].hid, NULL))
 			continue;
 
+		dev_dbg(int3472->dev, "mapping type 0x%02x pin to 0x%02x %s\n",
+			*type, int3472_gpio_map[i].type_to, int3472_gpio_map[i].con_id);
+
 		*type = int3472_gpio_map[i].type_to;
 		*gpio_flags = int3472_gpio_map[i].polarity_low ?
 			      GPIO_ACTIVE_LOW : GPIO_ACTIVE_HIGH;
@@ -267,7 +271,7 @@ static int skl_int3472_handle_gpio_resources(struct acpi_resource *ares,
 
 	type = FIELD_GET(INT3472_GPIO_DSM_TYPE, obj->integer.value);
 
-	int3472_get_con_id_and_polarity(int3472->sensor, &type, &con_id, &gpio_flags);
+	int3472_get_con_id_and_polarity(int3472, &type, &con_id, &gpio_flags);
 
 	pin = FIELD_GET(INT3472_GPIO_DSM_PIN, obj->integer.value);
 	/* Pin field is not really used under Windows and wraps around at 8 bits */
-- 
2.49.0

Re: [PATCH v2 9/9] platform/x86: int3472: Debug log when remapping pins

[Andy Shevchenko]

On Wed, Apr 2, 2025 at 11:26 PM Hans de Goede <hdegoede@redhat.com> wrote:
>
> Debug log when remapping a pin/function because of a int3472_gpio_map[]
> match.

Reviewed-by: Andy Shevchenko <andy@kernel.org>

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 0/9] platform/x86: int3472: Add handshake pin support

[Andy Shevchenko]

On Wed, Apr 2, 2025 at 11:25 PM Hans de Goede <hdegoede@redhat.com> wrote:
>
> New Intel Meteor Lake based laptops with IPU6 cameras have a new type 0x12
> pin defined in the INT3472 sensor companion device which describes
> the sensor's GPIOs.
>
> This pin is primarily used on designs with a Lattice FPGA chip which is
> capable of running the sensor independently of the main CPU for features
> like presence detection. This pin needs to be driven high to make the FPGA
> run the power-on sequence of the sensor. After driving the pin high
> the FPGA "firmware" needs 25ms to comlpete the power-on sequence.
>
> This series implements support for this by modelling the handshake GPIO
> as a GPIO driven 'dvdd' regulator with an enable-time of 25 ms, also see:
>
> https://lore.kernel.org/platform-driver-x86/59f672c3-6d87-4ec7-9b7f-f44fe2cce934@redhat.com/
>
> Patch   1   Is an unrelated cleanup which I had lying around
> Patches 2-7 Prepare + Implement the handshake GPIO
> Patch   8   Is a small patch adding some extra debugging to GPIO remapping

You meant patch 9 here, right?

Offtopic: I sent you a message asking about AtomISP patch queue
status, but no answer. I understand that you are kinda very busy,
still it seems we missed one cycle for the patches you already have in
your queue. I haven't investigated where the bottle neck happened,
though. Hence just asking what's the plan with them and other patches
that are already in the mailing list (I have received at least two
that I was Cc'ed on).

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 0/9] platform/x86: int3472: Add handshake pin support

[Hans de Goede]

Hi Andy,

Thank you for all the reviews.

On 2-Apr-25 11:02 PM, Andy Shevchenko wrote:
> On Wed, Apr 2, 2025 at 11:25 PM Hans de Goede <hdegoede@redhat.com> wrote:
>>
>> New Intel Meteor Lake based laptops with IPU6 cameras have a new type 0x12
>> pin defined in the INT3472 sensor companion device which describes
>> the sensor's GPIOs.
>>
>> This pin is primarily used on designs with a Lattice FPGA chip which is
>> capable of running the sensor independently of the main CPU for features
>> like presence detection. This pin needs to be driven high to make the FPGA
>> run the power-on sequence of the sensor. After driving the pin high
>> the FPGA "firmware" needs 25ms to comlpete the power-on sequence.
>>
>> This series implements support for this by modelling the handshake GPIO
>> as a GPIO driven 'dvdd' regulator with an enable-time of 25 ms, also see:
>>
>> https://lore.kernel.org/platform-driver-x86/59f672c3-6d87-4ec7-9b7f-f44fe2cce934@redhat.com/
>>
>> Patch   1   Is an unrelated cleanup which I had lying around
>> Patches 2-7 Prepare + Implement the handshake GPIO
>> Patch   8   Is a small patch adding some extra debugging to GPIO remapping
> 
> You meant patch 9 here, right?
> 
> Offtopic: I sent you a message asking about AtomISP patch queue
> status, but no answer. I understand that you are kinda very busy,
> still it seems we missed one cycle for the patches you already have in
> your queue. I haven't investigated where the bottle neck happened,
> though. Hence just asking what's the plan with them and other patches
> that are already in the mailing list (I have received at least two
> that I was Cc'ed on).

Yeah, I've been burried in work a bit, so this fall through the cracks.

I've some more bandwidth now and I have a mailfolder with all
the pending atomisp patches in there. I plan to start merging these
soon and to send out an atomisp pull-request for 6.15 well in time.

Regards,

Hans

Re: [PATCH v2 0/9] platform/x86: int3472: Add handshake pin support

[Andy Shevchenko]

On Thu, Apr 03, 2025 at 12:30:29PM +0200, Hans de Goede wrote:
> On 2-Apr-25 11:02 PM, Andy Shevchenko wrote:
> > On Wed, Apr 2, 2025 at 11:25 PM Hans de Goede <hdegoede@redhat.com> wrote:

...

> > Offtopic: I sent you a message asking about AtomISP patch queue
> > status, but no answer. I understand that you are kinda very busy,
> > still it seems we missed one cycle for the patches you already have in
> > your queue. I haven't investigated where the bottle neck happened,
> > though. Hence just asking what's the plan with them and other patches
> > that are already in the mailing list (I have received at least two
> > that I was Cc'ed on).
> 
> Yeah, I've been burried in work a bit, so this fall through the cracks.
> 
> I've some more bandwidth now and I have a mailfolder with all
> the pending atomisp patches in there. I plan to start merging these
> soon and to send out an atomisp pull-request for 6.15 well in time.

Ah, cool! Don't hesitate to ask for a review the patches I haven't seen
for AtomISP driver.

-- 
With Best Regards,
Andy Shevchenko