[PATCH 0/6] platform/x86: int3472: Allow re-using sensor GPIO mapping in atomisp

[PATCH 0/6] platform/x86: int3472: Allow re-using sensor GPIO mapping in atomisp

[Hans de Goede]

Hi All,

This patch series does some small refactoring of the int3472 code to allow
re-using the sensor GPIO mapping code in the atomisp driver and then the
final patch in the series moves the atomisp driver over.

About merging this, maybe the int3472 patches can be merged in time for
the 6.16 merge window and then the atomisp patch can be merged after
6.16-rc1 is released, otherwise an immutable pdx86 branch with the first
5 patches will be necessary.

Regards,

Hans


Hans de Goede (6):
  platform/x86: int3472: Move common.h to public includes, symbols to
    INTEL_INT3472
  platform/x86: int3472: Stop using devm_gpiod_get()
  platform/x86: int3472: Export int3472_discrete_parse_crs()
  platform/x86: int3472: Remove unused sensor_config struct member
  platform/x86: int3472: For mt9m114 sensors map powerdown to
    powerenable
  media: atomisp: Switch to int3472 driver sensor GPIO mapping code

 MAINTAINERS                                   |   1 +
 .../x86/intel/int3472/clk_and_regulator.c     |   9 +-
 drivers/platform/x86/intel/int3472/common.c   |   9 +-
 drivers/platform/x86/intel/int3472/discrete.c |  28 +-
 .../x86/intel/int3472/discrete_quirks.c       |   3 +-
 drivers/platform/x86/intel/int3472/led.c      |   3 +-
 drivers/platform/x86/intel/int3472/tps68470.c |   3 +-
 .../staging/media/atomisp/pci/atomisp_csi2.h  |  17 --
 .../media/atomisp/pci/atomisp_csi2_bridge.c   | 239 ++----------------
 .../staging/media/atomisp/pci/atomisp_v4l2.c  |   1 +
 .../linux/platform_data/x86/int3472.h         |  16 +-
 11 files changed, 76 insertions(+), 253 deletions(-)
 rename drivers/platform/x86/intel/int3472/common.h => include/linux/platform_data/x86/int3472.h (92%)

-- 
2.49.0

[PATCH 1/6] platform/x86: int3472: Move common.h to public includes, symbols to INTEL_INT3472

[Hans de Goede]

Move the common.h header file to include/linux/platform_data/x86/int3472.h
and add a "INTEL_INT3472" kernel-symbol-namespace to the exported symbols.

This is a preparation patch for exporting some more symbols for re-use in
the atomisp driver.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 MAINTAINERS                                            |  1 +
 drivers/platform/x86/intel/int3472/clk_and_regulator.c |  3 +--
 drivers/platform/x86/intel/int3472/common.c            |  9 ++++-----
 drivers/platform/x86/intel/int3472/discrete.c          |  4 ++--
 drivers/platform/x86/intel/int3472/discrete_quirks.c   |  3 +--
 drivers/platform/x86/intel/int3472/led.c               |  2 +-
 drivers/platform/x86/intel/int3472/tps68470.c          |  3 ++-
 .../linux/platform_data/x86/int3472.h                  | 10 +++++++---
 8 files changed, 19 insertions(+), 16 deletions(-)
 rename drivers/platform/x86/intel/int3472/common.h => include/linux/platform_data/x86/int3472.h (96%)

diff --git a/MAINTAINERS b/MAINTAINERS
index b2d4558d12d6..43a3d9afbb76 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -12261,6 +12261,7 @@ INTEL SKYLAKE INT3472 ACPI DEVICE DRIVER
 M:	Daniel Scally <djrscally@gmail.com>
 S:	Maintained
 F:	drivers/platform/x86/intel/int3472/
+F:	include/linux/platform_data/x86/int3472.h
 
 INTEL SPEED SELECT TECHNOLOGY
 M:	Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
diff --git a/drivers/platform/x86/intel/int3472/clk_and_regulator.c b/drivers/platform/x86/intel/int3472/clk_and_regulator.c
index c85cbfbc16c1..4d00494a7670 100644
--- a/drivers/platform/x86/intel/int3472/clk_and_regulator.c
+++ b/drivers/platform/x86/intel/int3472/clk_and_regulator.c
@@ -6,11 +6,10 @@
 #include <linux/clk-provider.h>
 #include <linux/device.h>
 #include <linux/gpio/consumer.h>
+#include <linux/platform_data/x86/int3472.h>
 #include <linux/regulator/driver.h>
 #include <linux/slab.h>
 
-#include "common.h"
-
 /*
  * 82c0d13a-78c5-4244-9bb1-eb8b539a8d11
  * This _DSM GUID allows controlling the sensor clk when it is not controlled
diff --git a/drivers/platform/x86/intel/int3472/common.c b/drivers/platform/x86/intel/int3472/common.c
index 1638be8fa71e..6dc38d5cbd0b 100644
--- a/drivers/platform/x86/intel/int3472/common.c
+++ b/drivers/platform/x86/intel/int3472/common.c
@@ -2,10 +2,9 @@
 /* Author: Dan Scally <djrscally@gmail.com> */
 
 #include <linux/acpi.h>
+#include <linux/platform_data/x86/int3472.h>
 #include <linux/slab.h>
 
-#include "common.h"
-
 union acpi_object *skl_int3472_get_acpi_buffer(struct acpi_device *adev, char *id)
 {
 	struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL };
@@ -29,7 +28,7 @@ union acpi_object *skl_int3472_get_acpi_buffer(struct acpi_device *adev, char *i
 
 	return obj;
 }
-EXPORT_SYMBOL_GPL(skl_int3472_get_acpi_buffer);
+EXPORT_SYMBOL_NS_GPL(skl_int3472_get_acpi_buffer, "INTEL_INT3472");
 
 int skl_int3472_fill_cldb(struct acpi_device *adev, struct int3472_cldb *cldb)
 {
@@ -53,7 +52,7 @@ int skl_int3472_fill_cldb(struct acpi_device *adev, struct int3472_cldb *cldb)
 	kfree(obj);
 	return ret;
 }
-EXPORT_SYMBOL_GPL(skl_int3472_fill_cldb);
+EXPORT_SYMBOL_NS_GPL(skl_int3472_fill_cldb, "INTEL_INT3472");
 
 /* sensor_adev_ret may be NULL, name_ret must not be NULL */
 int skl_int3472_get_sensor_adev_and_name(struct device *dev,
@@ -84,7 +83,7 @@ int skl_int3472_get_sensor_adev_and_name(struct device *dev,
 
 	return ret;
 }
-EXPORT_SYMBOL_GPL(skl_int3472_get_sensor_adev_and_name);
+EXPORT_SYMBOL_NS_GPL(skl_int3472_get_sensor_adev_and_name, "INTEL_INT3472");
 
 MODULE_DESCRIPTION("Intel SkyLake INT3472 ACPI Device Driver library");
 MODULE_AUTHOR("Daniel Scally <djrscally@gmail.com>");
diff --git a/drivers/platform/x86/intel/int3472/discrete.c b/drivers/platform/x86/intel/int3472/discrete.c
index 394975f55d64..d0938da0a591 100644
--- a/drivers/platform/x86/intel/int3472/discrete.c
+++ b/drivers/platform/x86/intel/int3472/discrete.c
@@ -12,12 +12,11 @@
 #include <linux/kernel.h>
 #include <linux/module.h>
 #include <linux/overflow.h>
+#include <linux/platform_data/x86/int3472.h>
 #include <linux/platform_device.h>
 #include <linux/string_choices.h>
 #include <linux/uuid.h>
 
-#include "common.h"
-
 /*
  * 79234640-9e10-4fea-a5c1-b5aa8b19756f
  * This _DSM GUID returns information about the GPIO lines mapped to a
@@ -479,3 +478,4 @@ module_platform_driver(int3472_discrete);
 MODULE_DESCRIPTION("Intel SkyLake INT3472 ACPI Discrete Device Driver");
 MODULE_AUTHOR("Daniel Scally <djrscally@gmail.com>");
 MODULE_LICENSE("GPL v2");
+MODULE_IMPORT_NS("INTEL_INT3472");
diff --git a/drivers/platform/x86/intel/int3472/discrete_quirks.c b/drivers/platform/x86/intel/int3472/discrete_quirks.c
index bf88863803b2..552869ef91ab 100644
--- a/drivers/platform/x86/intel/int3472/discrete_quirks.c
+++ b/drivers/platform/x86/intel/int3472/discrete_quirks.c
@@ -2,8 +2,7 @@
 /* Author: Hans de Goede <hansg@kernel.org> */
 
 #include <linux/dmi.h>
-
-#include "common.h"
+#include <linux/platform_data/x86/int3472.h>
 
 static const struct int3472_discrete_quirks lenovo_miix_510_quirks = {
 	.avdd_second_sensor = "i2c-OVTI2680:00",
diff --git a/drivers/platform/x86/intel/int3472/led.c b/drivers/platform/x86/intel/int3472/led.c
index 9cbed694e2ca..c5588e143f7d 100644
--- a/drivers/platform/x86/intel/int3472/led.c
+++ b/drivers/platform/x86/intel/int3472/led.c
@@ -4,7 +4,7 @@
 #include <linux/acpi.h>
 #include <linux/gpio/consumer.h>
 #include <linux/leds.h>
-#include "common.h"
+#include <linux/platform_data/x86/int3472.h>
 
 static int int3472_pled_set(struct led_classdev *led_cdev,
 				     enum led_brightness brightness)
diff --git a/drivers/platform/x86/intel/int3472/tps68470.c b/drivers/platform/x86/intel/int3472/tps68470.c
index 81ac4c691963..0133405697dc 100644
--- a/drivers/platform/x86/intel/int3472/tps68470.c
+++ b/drivers/platform/x86/intel/int3472/tps68470.c
@@ -8,10 +8,10 @@
 #include <linux/mfd/tps68470.h>
 #include <linux/platform_device.h>
 #include <linux/platform_data/tps68470.h>
+#include <linux/platform_data/x86/int3472.h>
 #include <linux/regmap.h>
 #include <linux/string.h>
 
-#include "common.h"
 #include "tps68470.h"
 
 #define DESIGNED_FOR_CHROMEOS		1
@@ -261,4 +261,5 @@ module_i2c_driver(int3472_tps68470);
 MODULE_DESCRIPTION("Intel SkyLake INT3472 ACPI TPS68470 Device Driver");
 MODULE_AUTHOR("Daniel Scally <djrscally@gmail.com>");
 MODULE_LICENSE("GPL v2");
+MODULE_IMPORT_NS("INTEL_INT3472");
 MODULE_SOFTDEP("pre: clk-tps68470 tps68470-regulator");
diff --git a/drivers/platform/x86/intel/int3472/common.h b/include/linux/platform_data/x86/int3472.h
similarity index 96%
rename from drivers/platform/x86/intel/int3472/common.h
rename to include/linux/platform_data/x86/int3472.h
index 51b818e62a25..4cf02df6f753 100644
--- a/drivers/platform/x86/intel/int3472/common.h
+++ b/include/linux/platform_data/x86/int3472.h
@@ -1,8 +1,12 @@
 /* SPDX-License-Identifier: GPL-2.0 */
-/* Author: Dan Scally <djrscally@gmail.com> */
+/*
+ * Intel INT3472 ACPI camera sensor power-management support
+ *
+ * Author: Dan Scally <djrscally@gmail.com>
+ */
 
-#ifndef _INTEL_SKL_INT3472_H
-#define _INTEL_SKL_INT3472_H
+#ifndef __PLATFORM_DATA_X86_INT3472_H
+#define __PLATFORM_DATA_X86_INT3472_H
 
 #include <linux/clk-provider.h>
 #include <linux/gpio/machine.h>
-- 
2.49.0

[PATCH 2/6] platform/x86: int3472: Stop using devm_gpiod_get()

[Hans de Goede]

The intent is to re-use the INT3472 code for parsing Intel camera sensor
GPIOs and mapping them to the sensor for the atomisp driver, which
currently has duplicate code.

On atomisp devices there is no special INT3472 ACPI device, instead
the Intel _DSM to get the GPIO type is part of the ACPI device for
the sensor itself.

To deal with this the mapping is done from ipu_bridge_init() instead of
from a platform-device probe() function, there is no device to tie
the lifetime of the gpiod_get() calls done by the INT3472 code to.

Switch from devm_gpiod_get() to plain gpiod_get() + explicit gpiod_put()
calls, to prepare for the code being re-used in the atomisp driver.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/platform/x86/intel/int3472/clk_and_regulator.c | 6 +++++-
 drivers/platform/x86/intel/int3472/discrete.c          | 6 +++++-
 drivers/platform/x86/intel/int3472/led.c               | 1 +
 include/linux/platform_data/x86/int3472.h              | 1 +
 4 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/drivers/platform/x86/intel/int3472/clk_and_regulator.c b/drivers/platform/x86/intel/int3472/clk_and_regulator.c
index 4d00494a7670..476ec24d3702 100644
--- a/drivers/platform/x86/intel/int3472/clk_and_regulator.c
+++ b/drivers/platform/x86/intel/int3472/clk_and_regulator.c
@@ -182,6 +182,7 @@ void skl_int3472_unregister_clock(struct int3472_discrete_device *int3472)
 
 	clkdev_drop(int3472->clock.cl);
 	clk_unregister(int3472->clock.clk);
+	gpiod_put(int3472->clock.ena_gpio);
 }
 
 int skl_int3472_register_regulator(struct int3472_discrete_device *int3472,
@@ -244,12 +245,15 @@ int skl_int3472_register_regulator(struct int3472_discrete_device *int3472,
 	if (IS_ERR(regulator->rdev))
 		return PTR_ERR(regulator->rdev);
 
+	int3472->regulators[int3472->n_regulator_gpios].ena_gpio = gpio;
 	int3472->n_regulator_gpios++;
 	return 0;
 }
 
 void skl_int3472_unregister_regulator(struct int3472_discrete_device *int3472)
 {
-	for (int i = 0; i < int3472->n_regulator_gpios; i++)
+	for (int i = 0; i < int3472->n_regulator_gpios; i++) {
 		regulator_unregister(int3472->regulators[i].rdev);
+		gpiod_put(int3472->regulators[i].ena_gpio);
+	}
 }
diff --git a/drivers/platform/x86/intel/int3472/discrete.c b/drivers/platform/x86/intel/int3472/discrete.c
index d0938da0a591..808d75e8deda 100644
--- a/drivers/platform/x86/intel/int3472/discrete.c
+++ b/drivers/platform/x86/intel/int3472/discrete.c
@@ -117,7 +117,7 @@ skl_int3472_gpiod_get_from_temp_lookup(struct int3472_discrete_device *int3472,
 		return ERR_PTR(ret);
 
 	gpiod_add_lookup_table(lookup);
-	desc = devm_gpiod_get(int3472->dev, con_id, GPIOD_OUT_LOW);
+	desc = gpiod_get(int3472->dev, con_id, GPIOD_OUT_LOW);
 	gpiod_remove_lookup_table(lookup);
 
 	return desc;
@@ -340,6 +340,10 @@ static int skl_int3472_handle_gpio_resources(struct acpi_resource *ares,
 			ret = -EINVAL;
 			break;
 		}
+
+		if (ret)
+			gpiod_put(gpio);
+
 		break;
 	default:
 		dev_warn(int3472->dev,
diff --git a/drivers/platform/x86/intel/int3472/led.c b/drivers/platform/x86/intel/int3472/led.c
index c5588e143f7d..f1d6d7b0cb75 100644
--- a/drivers/platform/x86/intel/int3472/led.c
+++ b/drivers/platform/x86/intel/int3472/led.c
@@ -56,4 +56,5 @@ void skl_int3472_unregister_pled(struct int3472_discrete_device *int3472)
 
 	led_remove_lookup(&int3472->pled.lookup);
 	led_classdev_unregister(&int3472->pled.classdev);
+	gpiod_put(int3472->pled.gpio);
 }
diff --git a/include/linux/platform_data/x86/int3472.h b/include/linux/platform_data/x86/int3472.h
index 4cf02df6f753..0a835cc85c67 100644
--- a/include/linux/platform_data/x86/int3472.h
+++ b/include/linux/platform_data/x86/int3472.h
@@ -99,6 +99,7 @@ struct int3472_gpio_regulator {
 	struct regulator_consumer_supply supply_map[GPIO_REGULATOR_SUPPLY_MAP_COUNT * 2];
 	char supply_name_upper[GPIO_SUPPLY_NAME_LENGTH];
 	char regulator_name[GPIO_REGULATOR_NAME_LENGTH];
+	struct gpio_desc *ena_gpio;
 	struct regulator_dev *rdev;
 	struct regulator_desc rdesc;
 };
-- 
2.49.0

[PATCH 3/6] platform/x86: int3472: Export int3472_discrete_parse_crs()

[Hans de Goede]

At the moment the atomisp has duplicate code for parsing Intel camera
sensor GPIOS and calling the special 79234640-9e10-4fea-a5c1-b5aa8b19756f
_DSM to get the GPIO type and map it to the sensor.

Export int3472_discrete_parse_crs() so that the atomisp driver can reuse
the INT3472 code for this.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/platform/x86/intel/int3472/discrete.c | 15 ++++++++++-----
 include/linux/platform_data/x86/int3472.h     |  3 +++
 2 files changed, 13 insertions(+), 5 deletions(-)

diff --git a/drivers/platform/x86/intel/int3472/discrete.c b/drivers/platform/x86/intel/int3472/discrete.c
index 808d75e8deda..c706671e2f63 100644
--- a/drivers/platform/x86/intel/int3472/discrete.c
+++ b/drivers/platform/x86/intel/int3472/discrete.c
@@ -363,7 +363,7 @@ static int skl_int3472_handle_gpio_resources(struct acpi_resource *ares,
 	return 1;
 }
 
-static int skl_int3472_parse_crs(struct int3472_discrete_device *int3472)
+int int3472_discrete_parse_crs(struct int3472_discrete_device *int3472)
 {
 	LIST_HEAD(resource_list);
 	int ret;
@@ -388,17 +388,22 @@ static int skl_int3472_parse_crs(struct int3472_discrete_device *int3472)
 
 	return 0;
 }
+EXPORT_SYMBOL_NS_GPL(int3472_discrete_parse_crs, "INTEL_INT3472_DISCRETE");
 
-static void skl_int3472_discrete_remove(struct platform_device *pdev)
+void int3472_discrete_cleanup(struct int3472_discrete_device *int3472)
 {
-	struct int3472_discrete_device *int3472 = platform_get_drvdata(pdev);
-
 	gpiod_remove_lookup_table(&int3472->gpios);
 
 	skl_int3472_unregister_clock(int3472);
 	skl_int3472_unregister_pled(int3472);
 	skl_int3472_unregister_regulator(int3472);
 }
+EXPORT_SYMBOL_NS_GPL(int3472_discrete_cleanup, "INTEL_INT3472_DISCRETE");
+
+static void skl_int3472_discrete_remove(struct platform_device *pdev)
+{
+	int3472_discrete_cleanup(platform_get_drvdata(pdev));
+}
 
 static int skl_int3472_discrete_probe(struct platform_device *pdev)
 {
@@ -453,7 +458,7 @@ static int skl_int3472_discrete_probe(struct platform_device *pdev)
 	 */
 	INIT_LIST_HEAD(&int3472->gpios.list);
 
-	ret = skl_int3472_parse_crs(int3472);
+	ret = int3472_discrete_parse_crs(int3472);
 	if (ret) {
 		skl_int3472_discrete_remove(pdev);
 		return ret;
diff --git a/include/linux/platform_data/x86/int3472.h b/include/linux/platform_data/x86/int3472.h
index 0a835cc85c67..89410f0cb73a 100644
--- a/include/linux/platform_data/x86/int3472.h
+++ b/include/linux/platform_data/x86/int3472.h
@@ -147,6 +147,9 @@ int skl_int3472_get_sensor_adev_and_name(struct device *dev,
 					 struct acpi_device **sensor_adev_ret,
 					 const char **name_ret);
 
+int int3472_discrete_parse_crs(struct int3472_discrete_device *int3472);
+void int3472_discrete_cleanup(struct int3472_discrete_device *int3472);
+
 int skl_int3472_register_gpio_clock(struct int3472_discrete_device *int3472,
 				    struct gpio_desc *gpio);
 int skl_int3472_register_dsm_clock(struct int3472_discrete_device *int3472);
-- 
2.49.0

[PATCH 4/6] platform/x86: int3472: Remove unused sensor_config struct member

[Hans de Goede]

sensor_config is not used anywhere and its struct int3472_sensor_config
type also is not declared anywhere, drop it.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 include/linux/platform_data/x86/int3472.h | 2 --
 1 file changed, 2 deletions(-)

diff --git a/include/linux/platform_data/x86/int3472.h b/include/linux/platform_data/x86/int3472.h
index 89410f0cb73a..78276a11c48d 100644
--- a/include/linux/platform_data/x86/int3472.h
+++ b/include/linux/platform_data/x86/int3472.h
@@ -110,8 +110,6 @@ struct int3472_discrete_device {
 	struct acpi_device *sensor;
 	const char *sensor_name;
 
-	const struct int3472_sensor_config *sensor_config;
-
 	struct int3472_gpio_regulator regulators[INT3472_MAX_REGULATORS];
 
 	struct int3472_clock {
-- 
2.49.0

[PATCH 5/6] platform/x86: int3472: For mt9m114 sensors map powerdown to powerenable

[Hans de Goede]

mt9m114 atomisp designs declare both reset and powerdown pins in their
GPIO type DSM, but the mt9m114 only has a reser pin. The powerdown pin
seems to control the regulators suppyling power to the sensor and
the privacy LED.

Add a mapping of powerdown to powerenable for the mt9m114 for this.

While at is also add a comment to document the ov7251 mapping.

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

diff --git a/drivers/platform/x86/intel/int3472/discrete.c b/drivers/platform/x86/intel/int3472/discrete.c
index c706671e2f63..4c0aed6e626f 100644
--- a/drivers/platform/x86/intel/int3472/discrete.c
+++ b/drivers/platform/x86/intel/int3472/discrete.c
@@ -142,6 +142,9 @@ struct int3472_gpio_map {
 };
 
 static const struct int3472_gpio_map int3472_gpio_map[] = {
+	/* mt9m114 designs declare a powerdown pin which controls the regulators */
+	{ "INT33F0", INT3472_GPIO_TYPE_POWERDOWN, INT3472_GPIO_TYPE_POWER_ENABLE, false, "vdd" },
+	/* ov7251 driver / DT-bindings expect "enable" as con_id for reset */
 	{ "INT347E", INT3472_GPIO_TYPE_RESET, INT3472_GPIO_TYPE_RESET, false, "enable" },
 };
 
-- 
2.49.0

[PATCH 6/6] media: atomisp: Switch to int3472 driver sensor GPIO mapping code

[Hans de Goede]

Replace the duplicate code for calling the special Intel camera sensor GPIO
type _DSM (79234640-9e10-4fea-a5c1-b5aa8b19756f) and mapping GPIOs to
the sensor with a call to int3472_discrete_parse_crs() from the int3472
driver.

Besides avoiding code duplication the int3472 version of the code also
supports more features, like mapping the powerdown GPIO to a regulator on
the mt9m114 which is necessary to make the camera on the Asus T100TA work.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 .../staging/media/atomisp/pci/atomisp_csi2.h  |  17 --
 .../media/atomisp/pci/atomisp_csi2_bridge.c   | 239 ++----------------
 .../staging/media/atomisp/pci/atomisp_v4l2.c  |   1 +
 3 files changed, 29 insertions(+), 228 deletions(-)

diff --git a/drivers/staging/media/atomisp/pci/atomisp_csi2.h b/drivers/staging/media/atomisp/pci/atomisp_csi2.h
index bb998c82a438..ec762f8fb922 100644
--- a/drivers/staging/media/atomisp/pci/atomisp_csi2.h
+++ b/drivers/staging/media/atomisp/pci/atomisp_csi2.h
@@ -19,28 +19,11 @@
 #define CSI2_PAD_SOURCE		1
 #define CSI2_PADS_NUM		2
 
-#define CSI2_MAX_ACPI_GPIOS	2u
-
-struct acpi_device;
 struct v4l2_device;
 
 struct atomisp_device;
 struct atomisp_sub_device;
 
-struct atomisp_csi2_acpi_gpio_map {
-	struct acpi_gpio_params params[CSI2_MAX_ACPI_GPIOS];
-	struct acpi_gpio_mapping mapping[CSI2_MAX_ACPI_GPIOS + 1];
-};
-
-struct atomisp_csi2_acpi_gpio_parsing_data {
-	struct acpi_device *adev;
-	struct atomisp_csi2_acpi_gpio_map *map;
-	u32 settings[CSI2_MAX_ACPI_GPIOS];
-	unsigned int settings_count;
-	unsigned int res_count;
-	unsigned int map_count;
-};
-
 struct atomisp_mipi_csi2_device {
 	struct v4l2_subdev subdev;
 	struct media_pad pads[CSI2_PADS_NUM];
diff --git a/drivers/staging/media/atomisp/pci/atomisp_csi2_bridge.c b/drivers/staging/media/atomisp/pci/atomisp_csi2_bridge.c
index 92a9eef25a9f..7e783c28d39b 100644
--- a/drivers/staging/media/atomisp/pci/atomisp_csi2_bridge.c
+++ b/drivers/staging/media/atomisp/pci/atomisp_csi2_bridge.c
@@ -13,6 +13,7 @@
 #include <linux/clk.h>
 #include <linux/device.h>
 #include <linux/dmi.h>
+#include <linux/platform_data/x86/int3472.h>
 #include <linux/property.h>
 
 #include <media/ipu-bridge.h>
@@ -24,39 +25,6 @@
 
 #define PMC_CLK_RATE_19_2MHZ			19200000
 
-/*
- * 79234640-9e10-4fea-a5c1-b5aa8b19756f
- * This _DSM GUID returns information about the GPIO lines mapped to a sensor.
- * Function number 1 returns a count of the GPIO lines that are mapped.
- * Subsequent functions return 32 bit ints encoding information about the GPIO.
- */
-static const guid_t intel_sensor_gpio_info_guid =
-	GUID_INIT(0x79234640, 0x9e10, 0x4fea,
-		  0xa5, 0xc1, 0xb5, 0xaa, 0x8b, 0x19, 0x75, 0x6f);
-
-#define INTEL_GPIO_DSM_TYPE_SHIFT			0
-#define INTEL_GPIO_DSM_TYPE_MASK			GENMASK(7, 0)
-#define INTEL_GPIO_DSM_PIN_SHIFT			8
-#define INTEL_GPIO_DSM_PIN_MASK				GENMASK(15, 8)
-#define INTEL_GPIO_DSM_SENSOR_ON_VAL_SHIFT		24
-#define INTEL_GPIO_DSM_SENSOR_ON_VAL_MASK		GENMASK(31, 24)
-
-#define INTEL_GPIO_DSM_TYPE(x) \
-	(((x) & INTEL_GPIO_DSM_TYPE_MASK) >> INTEL_GPIO_DSM_TYPE_SHIFT)
-#define INTEL_GPIO_DSM_PIN(x) \
-	(((x) & INTEL_GPIO_DSM_PIN_MASK) >> INTEL_GPIO_DSM_PIN_SHIFT)
-#define INTEL_GPIO_DSM_SENSOR_ON_VAL(x) \
-	(((x) & INTEL_GPIO_DSM_SENSOR_ON_VAL_MASK) >> INTEL_GPIO_DSM_SENSOR_ON_VAL_SHIFT)
-
-/*
- * 822ace8f-2814-4174-a56b-5f029fe079ee
- * This _DSM GUID returns a string from the sensor device, which acts as a
- * module identifier.
- */
-static const guid_t intel_sensor_module_guid =
-	GUID_INIT(0x822ace8f, 0x2814, 0x4174,
-		  0xa5, 0x6b, 0x5f, 0x02, 0x9f, 0xe0, 0x79, 0xee);
-
 /*
  * dc2f6c4f-045b-4f1d-97b9-882a6860a4be
  * This _DSM GUID returns a package with n*2 strings, with each set of 2 strings
@@ -323,195 +291,44 @@ static int atomisp_csi2_get_port(struct acpi_device *adev, int clock_num)
 	return gmin_cfg_get_int(adev, "CsiPort", port);
 }
 
-/* Note this always returns 1 to continue looping so that res_count is accurate */
-static int atomisp_csi2_handle_acpi_gpio_res(struct acpi_resource *ares, void *_data)
-{
-	struct atomisp_csi2_acpi_gpio_parsing_data *data = _data;
-	struct acpi_resource_gpio *agpio;
-	const char *name;
-	bool active_low;
-	unsigned int i;
-	u32 settings = 0;
-	u16 pin;
-
-	if (!acpi_gpio_get_io_resource(ares, &agpio))
-		return 1; /* Not a GPIO, continue the loop */
-
-	data->res_count++;
-
-	pin = agpio->pin_table[0];
-	for (i = 0; i < data->settings_count; i++) {
-		if (INTEL_GPIO_DSM_PIN(data->settings[i]) == pin) {
-			settings = data->settings[i];
-			break;
-		}
-	}
-
-	if (i == data->settings_count) {
-		acpi_handle_warn(data->adev->handle,
-				 "%s: Could not find DSM GPIO settings for pin %u\n",
-				 dev_name(&data->adev->dev), pin);
-		return 1;
-	}
-
-	switch (INTEL_GPIO_DSM_TYPE(settings)) {
-	case 0:
-		name = "reset-gpios";
-		break;
-	case 1:
-		name = "powerdown-gpios";
-		break;
-	default:
-		acpi_handle_warn(data->adev->handle, "%s: Unknown GPIO type 0x%02lx for pin %u\n",
-				 dev_name(&data->adev->dev),
-				 INTEL_GPIO_DSM_TYPE(settings), pin);
-		return 1;
-	}
-
-	/*
-	 * Both reset and power-down need to be logical false when the sensor
-	 * is on (sensor should not be in reset and not be powered-down). So
-	 * when the sensor-on-value (which is the physical pin value) is high,
-	 * then the signal is active-low.
-	 */
-	active_low = INTEL_GPIO_DSM_SENSOR_ON_VAL(settings);
-
-	i = data->map_count;
-	if (i == CSI2_MAX_ACPI_GPIOS)
-		return 1;
-
-	/* res_count is already incremented */
-	data->map->params[i].crs_entry_index = data->res_count - 1;
-	data->map->params[i].active_low = active_low;
-	data->map->mapping[i].name = name;
-	data->map->mapping[i].data = &data->map->params[i];
-	data->map->mapping[i].size = 1;
-	data->map_count++;
-
-	acpi_handle_info(data->adev->handle, "%s: %s crs %d %s pin %u active-%s\n",
-			 dev_name(&data->adev->dev), name,
-			 data->res_count - 1, agpio->resource_source.string_ptr,
-			 pin, active_low ? "low" : "high");
-
-	return 1;
-}
-
 /*
- * Helper function to create an ACPI GPIO lookup table for sensor reset and
- * powerdown signals on Intel Bay Trail (BYT) and Cherry Trail (CHT) devices,
- * including setting the correct polarity for the GPIO.
+ * Alloc and fill an int3472_discrete_device struct so that we can re-use
+ * the INT3472 sensor GPIO mapping code.
  *
- * This uses the "79234640-9e10-4fea-a5c1-b5aa8b19756f" DSM method directly
- * on the sensor device's ACPI node. This is different from later Intel
- * hardware which has a separate INT3472 acpi_device with this info.
- *
- * This function must be called before creating the sw-noded describing
- * the fwnode graph endpoint. And sensor drivers used on these devices
- * must return -EPROBE_DEFER when there is no endpoint description yet.
- * Together this guarantees that the GPIO lookups are in place before
- * the sensor driver tries to get GPIOs with gpiod_get().
- *
- * Note this code uses the same DSM GUID as the int3472_gpio_guid in
- * the INT3472 discrete.c code and there is some overlap, but there are
- * enough differences that it is difficult to share the code.
+ * This gets called from ipu_bridge_init() which runs only once per boot,
+ * the created faux int3472 device is leaked on purpose to keep the created
+ * GPIO mappings around permanently.
  */
 static int atomisp_csi2_add_gpio_mappings(struct acpi_device *adev)
 {
-	struct atomisp_csi2_acpi_gpio_parsing_data data = { };
-	LIST_HEAD(resource_list);
-	union acpi_object *obj;
-	unsigned int i, j;
+	struct int3472_discrete_device *int3472;
 	int ret;
 
-	obj = acpi_evaluate_dsm_typed(adev->handle, &intel_sensor_module_guid,
-				      0x00, 1, NULL, ACPI_TYPE_STRING);
-	if (obj) {
-		acpi_handle_info(adev->handle, "%s: Sensor module id: '%s'\n",
-				 dev_name(&adev->dev), obj->string.pointer);
-		ACPI_FREE(obj);
-	}
-
-	/*
-	 * First get the GPIO-settings count and then get count GPIO-settings
-	 * values. Note the order of these may differ from the order in which
-	 * the GPIOs are listed on the ACPI resources! So we first store them all
-	 * and then enumerate the ACPI resources and match them up by pin number.
-	 */
-	obj = acpi_evaluate_dsm_typed(adev->handle,
-				      &intel_sensor_gpio_info_guid, 0x00, 1,
-				      NULL, ACPI_TYPE_INTEGER);
-	if (!obj) {
-		acpi_handle_err(adev->handle, "%s: No _DSM entry for GPIO pin count\n",
-				dev_name(&adev->dev));
-		return -EIO;
-	}
-
-	data.settings_count = obj->integer.value;
-	ACPI_FREE(obj);
-
-	if (data.settings_count > CSI2_MAX_ACPI_GPIOS) {
-		acpi_handle_err(adev->handle, "%s: Too many GPIOs %u > %u\n",
-				dev_name(&adev->dev), data.settings_count,
-				CSI2_MAX_ACPI_GPIOS);
-		return -EOVERFLOW;
-	}
-
-	for (i = 0; i < data.settings_count; i++) {
-		/*
-		 * i + 2 because the index of this _DSM function is 1-based
-		 * and the first function is just a count.
-		 */
-		obj = acpi_evaluate_dsm_typed(adev->handle,
-					      &intel_sensor_gpio_info_guid,
-					      0x00, i + 2,
-					      NULL, ACPI_TYPE_INTEGER);
-		if (!obj) {
-			acpi_handle_err(adev->handle, "%s: No _DSM entry for pin %u\n",
-					dev_name(&adev->dev), i);
-			return -EIO;
-		}
-
-		data.settings[i] = obj->integer.value;
-		ACPI_FREE(obj);
-	}
-
-	/* Since we match up by pin-number the pin-numbers must be unique */
-	for (i = 0; i < data.settings_count; i++) {
-		for (j = i + 1; j < data.settings_count; j++) {
-			if (INTEL_GPIO_DSM_PIN(data.settings[i]) !=
-			    INTEL_GPIO_DSM_PIN(data.settings[j]))
-				continue;
-
-			acpi_handle_err(adev->handle, "%s: Duplicate pin number %lu\n",
-					dev_name(&adev->dev),
-					INTEL_GPIO_DSM_PIN(data.settings[i]));
-			return -EIO;
-		}
-	}
-
-	data.map = kzalloc(sizeof(*data.map), GFP_KERNEL);
-	if (!data.map)
+	/* Max num GPIOs we've seen plus a terminator */
+	int3472 = kzalloc(struct_size(int3472, gpios.table, INT3472_MAX_SENSOR_GPIOS + 1),
+			  GFP_KERNEL);
+	if (!int3472)
 		return -ENOMEM;
 
-	/* Now parse the ACPI resources and build the lookup table */
-	data.adev = adev;
-	ret = acpi_dev_get_resources(adev, &resource_list,
-				     atomisp_csi2_handle_acpi_gpio_res, &data);
-	if (ret < 0)
-		return ret;
+	/*
+	 * On atomisp the _DSM to get the GPIO type must be made on the sensor
+	 * adev, rather then on a separate INT3472 adev.
+	 */
+	int3472->adev = adev;
+	int3472->dev = &adev->dev;
+	int3472->sensor = adev;
 
-	acpi_dev_free_resource_list(&resource_list);
+	int3472->sensor_name = kasprintf(GFP_KERNEL, I2C_DEV_NAME_FORMAT, acpi_dev_name(adev));
+	if (!int3472->sensor_name) {
+		kfree(int3472);
+		return -ENOMEM;
+	}
 
-	if (data.map_count != data.settings_count ||
-	    data.res_count != data.settings_count)
-		acpi_handle_warn(adev->handle, "%s: ACPI GPIO resources vs DSM GPIO-info count mismatch (dsm: %d res: %d map %d\n",
-				 dev_name(&adev->dev), data.settings_count,
-				 data.res_count, data.map_count);
-
-	ret = acpi_dev_add_driver_gpios(adev, data.map->mapping);
-	if (ret)
-		acpi_handle_err(adev->handle, "%s: Error adding driver GPIOs: %d\n",
-				dev_name(&adev->dev), ret);
+	ret = int3472_discrete_parse_crs(int3472);
+	if (ret) {
+		int3472_discrete_cleanup(int3472);
+		kfree(int3472);
+	}
 
 	return ret;
 }
diff --git a/drivers/staging/media/atomisp/pci/atomisp_v4l2.c b/drivers/staging/media/atomisp/pci/atomisp_v4l2.c
index 0bd0bfded4af..3fdaa4b7bbaf 100644
--- a/drivers/staging/media/atomisp/pci/atomisp_v4l2.c
+++ b/drivers/staging/media/atomisp/pci/atomisp_v4l2.c
@@ -1513,3 +1513,4 @@ MODULE_AUTHOR("Xiaolin Zhang <xiaolin.zhang@intel.com>");
 MODULE_LICENSE("GPL");
 MODULE_DESCRIPTION("Intel ATOM Platform ISP Driver");
 MODULE_IMPORT_NS("INTEL_IPU_BRIDGE");
+MODULE_IMPORT_NS("INTEL_INT3472_DISCRETE");
-- 
2.49.0

Re: [PATCH 6/6] media: atomisp: Switch to int3472 driver sensor GPIO mapping code

[Andy Shevchenko]

On Wed, May 7, 2025 at 9:48 PM Hans de Goede <hdegoede@redhat.com> wrote:
>
> Replace the duplicate code for calling the special Intel camera sensor GPIO
> type _DSM (79234640-9e10-4fea-a5c1-b5aa8b19756f) and mapping GPIOs to
> the sensor with a call to int3472_discrete_parse_crs() from the int3472
> driver.
>
> Besides avoiding code duplication the int3472 version of the code also
> supports more features, like mapping the powerdown GPIO to a regulator on
> the mt9m114 which is necessary to make the camera on the Asus T100TA work.

...

Don't you need the Kconfig(s) update to have proper dependencies all
over these cases?

Otherwise I am fully in favour of this change and the series as a whole, thanks!

...

> +       /*
> +        * On atomisp the _DSM to get the GPIO type must be made on the sensor
> +        * adev, rather then on a separate INT3472 adev.

rather than

(FWIW, it's your typical mistake, it's something like the 10th time I
noticed it :-)

> +        */

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 6/6] media: atomisp: Switch to int3472 driver sensor GPIO mapping code

[Hans de Goede]

Hi Andy,

On 8-May-25 10:34 AM, Andy Shevchenko wrote:
> On Wed, May 7, 2025 at 9:48 PM Hans de Goede <hdegoede@redhat.com> wrote:
>>
>> Replace the duplicate code for calling the special Intel camera sensor GPIO
>> type _DSM (79234640-9e10-4fea-a5c1-b5aa8b19756f) and mapping GPIOs to
>> the sensor with a call to int3472_discrete_parse_crs() from the int3472
>> driver.
>>
>> Besides avoiding code duplication the int3472 version of the code also
>> supports more features, like mapping the powerdown GPIO to a regulator on
>> the mt9m114 which is necessary to make the camera on the Asus T100TA work.
> 
> ...
> 
> Don't you need the Kconfig(s) update to have proper dependencies all
> over these cases?

Yes I do, I thought about doing this already but forgot to actually
do it, thank you for catching this.

When I've some time for it I'll prepare a v2 of just this patch
addressing this and your s/then/than/ remark.

Since you and Sakari are happy with them patches 1-5 can be picked up
and merged by Ilpo as is, so I do not plan to send a v2 of those.

> Otherwise I am fully in favour of this change and the series as a whole, thanks!
> 
> ...
> 
>> +       /*
>> +        * On atomisp the _DSM to get the GPIO type must be made on the sensor
>> +        * adev, rather then on a separate INT3472 adev.
> 
> rather than
> 
> (FWIW, it's your typical mistake, it's something like the 10th time I
> noticed it :-)

Yeah, I'll try to remember to double check for this spelling mistake
myself, but I'm afraid I'll probably never learn. We have something
somewhat similar to than vs then in Dutch and I even do it wrong there :)

Regards,

Hans

Re: [PATCH 6/6] media: atomisp: Switch to int3472 driver sensor GPIO mapping code

[Ilpo Järvinen]

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

On Thu, 8 May 2025, Hans de Goede wrote:
> On 8-May-25 10:34 AM, Andy Shevchenko wrote:
> > On Wed, May 7, 2025 at 9:48 PM Hans de Goede <hdegoede@redhat.com> wrote:
> >>
> >> Replace the duplicate code for calling the special Intel camera sensor GPIO
> >> type _DSM (79234640-9e10-4fea-a5c1-b5aa8b19756f) and mapping GPIOs to
> >> the sensor with a call to int3472_discrete_parse_crs() from the int3472
> >> driver.
> >>
> >> Besides avoiding code duplication the int3472 version of the code also
> >> supports more features, like mapping the powerdown GPIO to a regulator on
> >> the mt9m114 which is necessary to make the camera on the Asus T100TA work.
> > 
> > ...
> > 
> > Don't you need the Kconfig(s) update to have proper dependencies all
> > over these cases?
> 
> Yes I do, I thought about doing this already but forgot to actually
> do it, thank you for catching this.
> 
> When I've some time for it I'll prepare a v2 of just this patch
> addressing this and your s/then/than/ remark.
> 
> Since you and Sakari are happy with them patches 1-5 can be picked up
> and merged by Ilpo as is, so I do not plan to send a v2 of those.

Thanks for the quick reviews.

I took patch 1-5 into the review-ilpo-next branch with one typo in 
change log fixed (reser -> reset).

> > Otherwise I am fully in favour of this change and the series as a whole, thanks!
> > 
> > ...
> > 
> >> +       /*
> >> +        * On atomisp the _DSM to get the GPIO type must be made on the sensor
> >> +        * adev, rather then on a separate INT3472 adev.
> > 
> > rather than
> > 
> > (FWIW, it's your typical mistake, it's something like the 10th time I
> > noticed it :-)
> 
> Yeah, I'll try to remember to double check for this spelling mistake
> myself, but I'm afraid I'll probably never learn. We have something
> somewhat similar to than vs then in Dutch and I even do it wrong there :)

I know the feeling, muscle memory is extremely hard to override. :-)

-- 
 i.

Re: [PATCH 6/6] media: atomisp: Switch to int3472 driver sensor GPIO mapping code

[Hans de Goede]

Hi,

On 8-May-25 3:48 PM, Ilpo Järvinen wrote:
> On Thu, 8 May 2025, Hans de Goede wrote:
>> On 8-May-25 10:34 AM, Andy Shevchenko wrote:
>>> On Wed, May 7, 2025 at 9:48 PM Hans de Goede <hdegoede@redhat.com> wrote:
>>>>
>>>> Replace the duplicate code for calling the special Intel camera sensor GPIO
>>>> type _DSM (79234640-9e10-4fea-a5c1-b5aa8b19756f) and mapping GPIOs to
>>>> the sensor with a call to int3472_discrete_parse_crs() from the int3472
>>>> driver.
>>>>
>>>> Besides avoiding code duplication the int3472 version of the code also
>>>> supports more features, like mapping the powerdown GPIO to a regulator on
>>>> the mt9m114 which is necessary to make the camera on the Asus T100TA work.
>>>
>>> ...
>>>
>>> Don't you need the Kconfig(s) update to have proper dependencies all
>>> over these cases?
>>
>> Yes I do, I thought about doing this already but forgot to actually
>> do it, thank you for catching this.
>>
>> When I've some time for it I'll prepare a v2 of just this patch
>> addressing this and your s/then/than/ remark.
>>
>> Since you and Sakari are happy with them patches 1-5 can be picked up
>> and merged by Ilpo as is, so I do not plan to send a v2 of those.
> 
> Thanks for the quick reviews.
> 
> I took patch 1-5 into the review-ilpo-next branch with one typo in 
> change log fixed (reser -> reset).

Great, thank you!

Regards,

Hans

Re: [PATCH 6/6] media: atomisp: Switch to int3472 driver sensor GPIO mapping code

[Hans de Goede]

Hi,

On 8-May-25 3:56 PM, Hans de Goede wrote:
> Hi,
> 
> On 8-May-25 3:48 PM, Ilpo Järvinen wrote:
>> On Thu, 8 May 2025, Hans de Goede wrote:
>>> On 8-May-25 10:34 AM, Andy Shevchenko wrote:
>>>> On Wed, May 7, 2025 at 9:48 PM Hans de Goede <hdegoede@redhat.com> wrote:
>>>>>
>>>>> Replace the duplicate code for calling the special Intel camera sensor GPIO
>>>>> type _DSM (79234640-9e10-4fea-a5c1-b5aa8b19756f) and mapping GPIOs to
>>>>> the sensor with a call to int3472_discrete_parse_crs() from the int3472
>>>>> driver.
>>>>>
>>>>> Besides avoiding code duplication the int3472 version of the code also
>>>>> supports more features, like mapping the powerdown GPIO to a regulator on
>>>>> the mt9m114 which is necessary to make the camera on the Asus T100TA work.
>>>>
>>>> ...
>>>>
>>>> Don't you need the Kconfig(s) update to have proper dependencies all
>>>> over these cases?
>>>
>>> Yes I do, I thought about doing this already but forgot to actually
>>> do it, thank you for catching this.
>>>
>>> When I've some time for it I'll prepare a v2 of just this patch
>>> addressing this and your s/then/than/ remark.
>>>
>>> Since you and Sakari are happy with them patches 1-5 can be picked up
>>> and merged by Ilpo as is, so I do not plan to send a v2 of those.
>>
>> Thanks for the quick reviews.
>>
>> I took patch 1-5 into the review-ilpo-next branch with one typo in 
>> change log fixed (reser -> reset).
> 
> Great, thank you!

Ilpo, I just noticed that Sakari typod his Reviewed-by, it reads:

Reviwed-by: Sakari Ailus <sakari.ailus@linux.intel.com>

and this is also how the tag looks in review-ilpo-next now.
so missing an 'e' you probably will want to fix this.

Also Sakari gave an Acked-by for patch 6/6, but since he did so
in a reply to the cover letter all 5 (1-5) patches now have:

Reviwed-by: Sakari Ailus <sakari.ailus@linux.intel.com>
Acked-by: Sakari Ailus <sakari.ailus@linux.intel.com>

so besides adding the missing 'e' you probably will also want
to drop the entire Acked-by.

Regards,

Hans

Re: [PATCH 6/6] media: atomisp: Switch to int3472 driver sensor GPIO mapping code

[Ilpo Järvinen]

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

On Thu, 8 May 2025, Hans de Goede wrote:

> Hi,
> 
> On 8-May-25 3:56 PM, Hans de Goede wrote:
> > Hi,
> > 
> > On 8-May-25 3:48 PM, Ilpo Järvinen wrote:
> >> On Thu, 8 May 2025, Hans de Goede wrote:
> >>> On 8-May-25 10:34 AM, Andy Shevchenko wrote:
> >>>> On Wed, May 7, 2025 at 9:48 PM Hans de Goede <hdegoede@redhat.com> wrote:
> >>>>>
> >>>>> Replace the duplicate code for calling the special Intel camera sensor GPIO
> >>>>> type _DSM (79234640-9e10-4fea-a5c1-b5aa8b19756f) and mapping GPIOs to
> >>>>> the sensor with a call to int3472_discrete_parse_crs() from the int3472
> >>>>> driver.
> >>>>>
> >>>>> Besides avoiding code duplication the int3472 version of the code also
> >>>>> supports more features, like mapping the powerdown GPIO to a regulator on
> >>>>> the mt9m114 which is necessary to make the camera on the Asus T100TA work.
> >>>>
> >>>> ...
> >>>>
> >>>> Don't you need the Kconfig(s) update to have proper dependencies all
> >>>> over these cases?
> >>>
> >>> Yes I do, I thought about doing this already but forgot to actually
> >>> do it, thank you for catching this.
> >>>
> >>> When I've some time for it I'll prepare a v2 of just this patch
> >>> addressing this and your s/then/than/ remark.
> >>>
> >>> Since you and Sakari are happy with them patches 1-5 can be picked up
> >>> and merged by Ilpo as is, so I do not plan to send a v2 of those.
> >>
> >> Thanks for the quick reviews.
> >>
> >> I took patch 1-5 into the review-ilpo-next branch with one typo in 
> >> change log fixed (reser -> reset).
> > 
> > Great, thank you!
> 
> Ilpo, I just noticed that Sakari typod his Reviewed-by, it reads:
> 
> Reviwed-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> 
> and this is also how the tag looks in review-ilpo-next now.
> so missing an 'e' you probably will want to fix this.
> 
> Also Sakari gave an Acked-by for patch 6/6, but since he did so
> in a reply to the cover letter all 5 (1-5) patches now have:
> 
> Reviwed-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> Acked-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> 
> so besides adding the missing 'e' you probably will also want
> to drop the entire Acked-by.

Yeah, a perfect example of tunnel vision. I was looking specifically 
whether those reviewed-bys got added by b4 into all patches but failed to 
notice any of those errors despite staring at the very lines. Thanks for 
letting me know, I'll fix them.

-- 
 i.

Re: [PATCH 6/6] media: atomisp: Switch to int3472 driver sensor GPIO mapping code

[Hans de Goede]

Hi,

On 8-May-25 10:34 AM, Andy Shevchenko wrote:
> On Wed, May 7, 2025 at 9:48 PM Hans de Goede <hdegoede@redhat.com> wrote:
>>
>> Replace the duplicate code for calling the special Intel camera sensor GPIO
>> type _DSM (79234640-9e10-4fea-a5c1-b5aa8b19756f) and mapping GPIOs to
>> the sensor with a call to int3472_discrete_parse_crs() from the int3472
>> driver.
>>
>> Besides avoiding code duplication the int3472 version of the code also
>> supports more features, like mapping the powerdown GPIO to a regulator on
>> the mt9m114 which is necessary to make the camera on the Asus T100TA work.
> 
> ...
> 
> Don't you need the Kconfig(s) update to have proper dependencies all
> over these cases?

A good point, not sure what you mean with "all over these cases" since
this just affects the main atomisp module. But yes the Kconfig deps
of the main atomisp module needs to be updated, I've added the following
while merging this:

       depends on INTEL_SKL_INT3472

> Otherwise I am fully in favour of this change and the series as a whole, thanks!
> 
> ...
> 
>> +       /*
>> +        * On atomisp the _DSM to get the GPIO type must be made on the sensor
>> +        * adev, rather then on a separate INT3472 adev.
> 
> rather than

Thank you, I've also fixed this while merging this into
my media-atomisp branch:

https://git.kernel.org/pub/scm/linux/kernel/git/hansg/linux.git/log/?h=media-atomisp

And this patch will be included in my next
pull-request to Mauro (to media subsystem maintainer)

Regards,

Hans

Re: [PATCH 0/6] platform/x86: int3472: Allow re-using sensor GPIO mapping in atomisp

[Andy Shevchenko]

On Wed, May 7, 2025 at 9:52 PM Hans de Goede <hdegoede@redhat.com> wrote:

> This patch series does some small refactoring of the int3472 code to allow
> re-using the sensor GPIO mapping code in the atomisp driver and then the
> final patch in the series moves the atomisp driver over.
>
> About merging this, maybe the int3472 patches can be merged in time for
> the 6.16 merge window and then the atomisp patch can be merged after
> 6.16-rc1 is released, otherwise an immutable pdx86 branch with the first
> 5 patches will be necessary.

Overall I'm pretty much liking this series, but one comment against
the last patch (see there) and one question here. Can you isolate GPIO
mapping code in a separate file, please? This will help to generalise
this code outside of two mentioned drivers (I might need it in the
future for something else, not related to cameras at all).

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 0/6] platform/x86: int3472: Allow re-using sensor GPIO mapping in atomisp

[Andy Shevchenko]

On Thu, May 8, 2025 at 11:36 AM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
> On Wed, May 7, 2025 at 9:52 PM Hans de Goede <hdegoede@redhat.com> wrote:
>
> > This patch series does some small refactoring of the int3472 code to allow
> > re-using the sensor GPIO mapping code in the atomisp driver and then the
> > final patch in the series moves the atomisp driver over.
> >
> > About merging this, maybe the int3472 patches can be merged in time for
> > the 6.16 merge window and then the atomisp patch can be merged after
> > 6.16-rc1 is released, otherwise an immutable pdx86 branch with the first
> > 5 patches will be necessary.
>
> Overall I'm pretty much liking this series, but one comment against
> the last patch (see there) and one question here. Can you isolate GPIO
> mapping code in a separate file, please? This will help to generalise
> this code outside of two mentioned drivers (I might need it in the
> future for something else, not related to cameras at all).

Ah, and formal
Reviewed-by: Andy Shevchenko <andy@kernel.org>
for patches 1-5.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 0/6] platform/x86: int3472: Allow re-using sensor GPIO mapping in atomisp

[Hans de Goede]

Hi,

On 8-May-25 10:37 AM, Andy Shevchenko wrote:
> On Thu, May 8, 2025 at 11:36 AM Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
>> On Wed, May 7, 2025 at 9:52 PM Hans de Goede <hdegoede@redhat.com> wrote:
>>
>>> This patch series does some small refactoring of the int3472 code to allow
>>> re-using the sensor GPIO mapping code in the atomisp driver and then the
>>> final patch in the series moves the atomisp driver over.
>>>
>>> About merging this, maybe the int3472 patches can be merged in time for
>>> the 6.16 merge window and then the atomisp patch can be merged after
>>> 6.16-rc1 is released, otherwise an immutable pdx86 branch with the first
>>> 5 patches will be necessary.
>>
>> Overall I'm pretty much liking this series, but one comment against
>> the last patch (see there) and one question here. Can you isolate GPIO
>> mapping code in a separate file, please? This will help to generalise
>> this code outside of two mentioned drivers (I might need it in the
>> future for something else, not related to cameras at all).
> 
> Ah, and formal
> Reviewed-by: Andy Shevchenko <andy@kernel.org>
> for patches 1-5.

Andy, Sakari, Thank you both for your reviews of patches 1-5.

Ilpo, since patches 1-5 have 2 Reviewed-by-s now, can we maybe
still get those in before the 6.16 merge window ?

That would make merging patch 6/6 a bit easier, avoiding the need to
have an immutable branch for the int3472 bits patch 6/6 depends on.

Regards,

Hans

Re: [PATCH 0/6] platform/x86: int3472: Allow re-using sensor GPIO mapping in atomisp

[Hans de Goede]

Hi Andy,

On 8-May-25 10:36 AM, Andy Shevchenko wrote:
> On Wed, May 7, 2025 at 9:52 PM Hans de Goede <hdegoede@redhat.com> wrote:
> 
>> This patch series does some small refactoring of the int3472 code to allow
>> re-using the sensor GPIO mapping code in the atomisp driver and then the
>> final patch in the series moves the atomisp driver over.
>>
>> About merging this, maybe the int3472 patches can be merged in time for
>> the 6.16 merge window and then the atomisp patch can be merged after
>> 6.16-rc1 is released, otherwise an immutable pdx86 branch with the first
>> 5 patches will be necessary.
> 
> Overall I'm pretty much liking this series, but one comment against
> the last patch (see there) and one question here. Can you isolate GPIO
> mapping code in a separate file, please? This will help to generalise
> this code outside of two mentioned drivers (I might need it in the
> future for something else, not related to cameras at all).

If you want to re-use this elsewhere then splitting it out
further sounds like a good plan.

But which bits do you need? Do you actually need the full code calling
the special DSM and then adding either GPIO-lookups, or gpio controlled
regulators / clks / LEDs ?

Because atm the int3472/discrete.c + other c files linked into the .ko
does all of that and for the atomisp2 case we actually want all of
that (although for now GPIO -> clk and LED is unused there).

Anyway I think it would be best for you (Andy) to come up with
a proposal / RFC patch series to split out what you need. I'm certainly
open to that and happy to review such a series.

Regards,

Hans

Re: [PATCH 0/6] platform/x86: int3472: Allow re-using sensor GPIO mapping in atomisp

[Andy Shevchenko]

On Thu, May 8, 2025 at 5:00 PM Hans de Goede <hdegoede@redhat.com> wrote:
> On 8-May-25 10:36 AM, Andy Shevchenko wrote:
> > On Wed, May 7, 2025 at 9:52 PM Hans de Goede <hdegoede@redhat.com> wrote:

...

> > Can you isolate GPIO
> > mapping code in a separate file, please? This will help to generalise
> > this code outside of two mentioned drivers (I might need it in the
> > future for something else, not related to cameras at all).
>
> If you want to re-use this elsewhere then splitting it out
> further sounds like a good plan.
>
> But which bits do you need? Do you actually need the full code calling
> the special DSM and then adding either GPIO-lookups, or gpio controlled
> regulators / clks / LEDs ?
>
> Because atm the int3472/discrete.c + other c files linked into the .ko
> does all of that and for the atomisp2 case we actually want all of
> that (although for now GPIO -> clk and LED is unused there).

I basically want to have remap\ing quirk part to be isolated.

> Anyway I think it would be best for you (Andy) to come up with
> a proposal / RFC patch series to split out what you need. I'm certainly
> open to that and happy to review such a series.

Fine, but can you do this in the discrete.c internally, like an
embedded C file so it doesn't require headers to be used, but being an
interface-ready solution?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 0/6] platform/x86: int3472: Allow re-using sensor GPIO mapping in atomisp

[Sakari Ailus]

On Wed, May 07, 2025 at 08:47:31PM +0200, Hans de Goede wrote:
> Hi All,
> 
> This patch series does some small refactoring of the int3472 code to allow
> re-using the sensor GPIO mapping code in the atomisp driver and then the
> final patch in the series moves the atomisp driver over.
> 
> About merging this, maybe the int3472 patches can be merged in time for
> the 6.16 merge window and then the atomisp patch can be merged after
> 6.16-rc1 is released, otherwise an immutable pdx86 branch with the first
> 5 patches will be necessary.

Thanks, Hans!

For the first five:

Reviwed-by: Sakari Ailus <sakari.ailus@linux.intel.com>

Feel free to add to the last patch (i haven't looked at the details, but
the approach seems good):

Acked-by: Sakari Ailus <sakari.ailus@linux.intel.com>

-- 
Sakari Ailus

Re: [PATCH 0/6] platform/x86: int3472: Allow re-using sensor GPIO mapping in atomisp

[Andy Shevchenko]

On Wed, May 7, 2025 at 9:52 PM Hans de Goede <hdegoede@redhat.com> wrote:

> This patch series does some small refactoring of the int3472 code to allow
> re-using the sensor GPIO mapping code in the atomisp driver and then the
> final patch in the series moves the atomisp driver over.

I just realised that the AtomISP variant is very likely a predecessor
of INT3472, and hence a lot of code has to be shared between two. Or
even having INT3472 being enumerated as a pure platform driver for the
case of AtomISP.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 0/6] platform/x86: int3472: Allow re-using sensor GPIO mapping in atomisp

[Hans de Goede]

Hi,

On 8-May-25 4:55 PM, Andy Shevchenko wrote:
> On Wed, May 7, 2025 at 9:52 PM Hans de Goede <hdegoede@redhat.com> wrote:
> 
>> This patch series does some small refactoring of the int3472 code to allow
>> re-using the sensor GPIO mapping code in the atomisp driver and then the
>> final patch in the series moves the atomisp driver over.
> 
> I just realised that the AtomISP variant is very likely a predecessor
> of INT3472, and hence a lot of code has to be shared between two.

Yes at least the sensor-module identification _DSM and the GPIO-type/map
_DSM have the same GUID. So the INT3472 device is clearly derived from
the atomisp case,

The weird thing is that the atomisp got a bunch of things more correct
from an ACPI modelling pov. The _DSM and the GPIO resources are on
the sensor ACPI-dev rather then a separate dev and regulators on the PMIC
are simply handled through ACPI power-resources rather then having the OS
have to figure all the PMIC stuff out like on IPU3 and IPU6 models with
a separate camera PMIC.

Regards,

Hans