[PATCH v6 00/15] Add support for X86/ACPI camera sensor/PMIC setup with clk and regulator platform data

[PATCH v6 00/15] Add support for X86/ACPI camera sensor/PMIC setup with clk and regulator platform data

[Hans de Goede]

Here is v6 of my patch-set adding support for camera sensor connected to a
TPS68470 PMIC on x86/ACPI devices.

Changes in v6:
- Add support for the VCM (Voice Coil Motor) controlling the focus of
  the back/main sensor camera lens:
  - Patch 3 and patches 13-15 are new patches for this
  - While working on this I learned that the VIO regulator is an always on
    regulator and must be configured at the same voltage as VSIO, the
    tps68470-regulator driver and the constraints have been updated for this
- Addressed clk-tps68470 driver review-remarks
- Some minor tweaks based on review-remarks from Andy
- Add Andy's Reviewed-by to all patches which were also in v5

I'm quite happy with how this works now, so from my pov this is ready
for merging now.

Patch 2 already has acks for merging through another tree. Patch 3
"i2c: acpi: Add i2c_acpi_new_device_by_fwnode() function" is new,
Mika, Wolfram may we have your Ack for merging this one through
Rafael's ACPI or through my platform/drivers/x86 tree too ?

Once we have acks to merge both i2c-core-acpi.c through another tree,
there are 2 options:

  a. 1. Patches 1-3 merged by Rafael, Rafael provides an IM branch
     2. I create an IM branch with patches 4 + 7-12
     3. clk + regulator maintainers merge my IM branch + clk / regulator patch
     4. media maintainers merge Rafael's IM branch + media patches
  b. 1. I create an IM branch with patches 1-4 + 7-12 (with Rafael's ack for 1)
     2. clk / regulator / media maintainers merge my IM branch + their resp. patches

Assuming Rafael does not foresee any conflicts caused by the few small ACPI
patches I believe that going with plan b would be best.

Rafael is plan b. ok with you ? You did already Ack patch 1 but IIRC that
was not specifically for merging it through another tree.

Regards,

Hans


p.s.

For the record here is part of the old cover-letter of v5:
    
Changes in v5:
- Update regulator_init_data in patch 10/11 to include the VCM regulator
- Address various small review remarks from Andy
- Make a couple of functions / vars static in the clk + regulator drivers
  Reported-by: kernel test robot <lkp@intel.com>

Changes in v4:
[PATCH 01/11] ACPI: delay enumeration of devices with a _DEP
              pointing to an INT3472 device:
- Move the acpi_dev_ready_for_enumeration() check to acpi_bus_attach()
  (replacing the acpi_device_is_present() check there)

[PATCH 04/11] regulator: Introduce tps68470-regulator driver:
- Make the top comment block use c++ style comments
- Drop the bogus builtin regulator_init_data
- Make the driver enable the PMIC clk when enabling the Core buck
  regulator, this switching regulator needs the PLL to be on
- Kconfig: add || COMPILE_TEST, fix help text

[PATCH 05/11] clk: Introduce clk-tps68470 driver
- Kconfig: select REGMAP_I2C, add || COMPILE_TEST, fix help text
- tps68470_clk_prepare(): Wait for the PLL to lock before returning
- tps68470_clk_unprepare(): Remove unnecessary clearing of divider regs
- tps68470_clk_probe(): Use devm_clk_hw_register()
- Misc. small cleanups

The clk and regulator frameworks expect clk/regulator consumer-devices
to have info about the consumed clks/regulators described in the device's
fw_node, but on ACPI this info is missing.

This series worksaround this by providing platform_data with the info to
the TPS68470 clk/regulator MFD cells.

Patches 1 - 2 deal with a probe-ordering problem this introduces,
since the lookups are only registered when the provider-driver binds,
trying to get these clks/regulators before then results in a -ENOENT
error for clks and a dummy regulator for regulators. See the patches
for more details.

Patch 3 adds a header file which adds tps68470_clk_platform_data and
tps68470_regulator_platform_data structs. The futher patches depend on
this new header file.

Patch 4 + 5 add the TPS68470 clk and regulator drivers

Patches 6 - 11 Modify the INT3472 driver which instantiates the MFD cells to
provide the necessary platform-data.


Daniel Scally (1):
  platform/x86: int3472: Enable I2c daisy chain

Hans de Goede (14):
  ACPI: delay enumeration of devices with a _DEP pointing to an INT3472
    device
  i2c: acpi: Use acpi_dev_ready_for_enumeration() helper
  i2c: acpi: Add i2c_acpi_new_device_by_fwnode() function
  platform_data: Add linux/platform_data/tps68470.h file
  regulator: Introduce tps68470-regulator driver
  clk: Introduce clk-tps68470 driver
  platform/x86: int3472: Split into 2 drivers
  platform/x86: int3472: Add get_sensor_adev_and_name() helper
  platform/x86: int3472: Pass tps68470_clk_platform_data to the
    tps68470-regulator MFD-cell
  platform/x86: int3472: Pass tps68470_regulator_platform_data to the
    tps68470-regulator MFD-cell
  platform/x86: int3472: Deal with probe ordering issues
  media: ipu3-cio2: Defer probing until the PMIC is fully setup
  media: ipu3-cio2: Call cio2_bridge_init() before anything else
  media: ipu3-cio2: Add support for instantiating i2c-clients for VCMs

 drivers/acpi/scan.c                           |  37 ++-
 drivers/clk/Kconfig                           |   8 +
 drivers/clk/Makefile                          |   1 +
 drivers/clk/clk-tps68470.c                    | 257 ++++++++++++++++++
 drivers/i2c/i2c-core-acpi.c                   |  23 +-
 drivers/media/pci/intel/ipu3/cio2-bridge.c    |  92 +++++++
 drivers/media/pci/intel/ipu3/cio2-bridge.h    |  16 +-
 drivers/media/pci/intel/ipu3/ipu3-cio2-main.c |  10 +-
 drivers/platform/x86/intel/int3472/Makefile   |   9 +-
 ...lk_and_regulator.c => clk_and_regulator.c} |   2 +-
 drivers/platform/x86/intel/int3472/common.c   |  82 ++++++
 .../{intel_skl_int3472_common.h => common.h}  |   6 +-
 ...ntel_skl_int3472_discrete.c => discrete.c} |  51 ++--
 .../intel/int3472/intel_skl_int3472_common.c  | 106 --------
 ...ntel_skl_int3472_tps68470.c => tps68470.c} |  99 ++++++-
 drivers/platform/x86/intel/int3472/tps68470.h |  25 ++
 .../x86/intel/int3472/tps68470_board_data.c   | 145 ++++++++++
 drivers/regulator/Kconfig                     |   9 +
 drivers/regulator/Makefile                    |   1 +
 drivers/regulator/tps68470-regulator.c        | 201 ++++++++++++++
 include/acpi/acpi_bus.h                       |   5 +-
 include/linux/i2c.h                           |  17 +-
 include/linux/mfd/tps68470.h                  |  11 +
 include/linux/platform_data/tps68470.h        |  35 +++
 24 files changed, 1080 insertions(+), 168 deletions(-)
 create mode 100644 drivers/clk/clk-tps68470.c
 rename drivers/platform/x86/intel/int3472/{intel_skl_int3472_clk_and_regulator.c => clk_and_regulator.c} (99%)
 create mode 100644 drivers/platform/x86/intel/int3472/common.c
 rename drivers/platform/x86/intel/int3472/{intel_skl_int3472_common.h => common.h} (94%)
 rename drivers/platform/x86/intel/int3472/{intel_skl_int3472_discrete.c => discrete.c} (91%)
 delete mode 100644 drivers/platform/x86/intel/int3472/intel_skl_int3472_common.c
 rename drivers/platform/x86/intel/int3472/{intel_skl_int3472_tps68470.c => tps68470.c} (54%)
 create mode 100644 drivers/platform/x86/intel/int3472/tps68470.h
 create mode 100644 drivers/platform/x86/intel/int3472/tps68470_board_data.c
 create mode 100644 drivers/regulator/tps68470-regulator.c
 create mode 100644 include/linux/platform_data/tps68470.h

-- 
2.33.1

[PATCH v6 01/15] ACPI: delay enumeration of devices with a _DEP pointing to an INT3472 device

[Hans de Goede]

The clk and regulator frameworks expect clk/regulator consumer-devices
to have info about the consumed clks/regulators described in the device's
fw_node.

To work around cases where this info is not present in the firmware tables,
which is often the case on x86/ACPI devices, both frameworks allow the
provider-driver to attach info about consumers to the clks/regulators
when registering these.

This causes problems with the probe ordering wrt drivers for consumers
of these clks/regulators. Since the lookups are only registered when the
provider-driver binds, trying to get these clks/regulators before then
results in a -ENOENT error for clks and a dummy regulator for regulators.

One case where we hit this issue is camera sensors such as e.g. the OV8865
sensor found on the Microsoft Surface Go. The sensor uses clks, regulators
and GPIOs provided by a TPS68470 PMIC which is described in an INT3472
ACPI device. There is special platform code handling this and setting
platform_data with the necessary consumer info on the MFD cells
instantiated for the PMIC under: drivers/platform/x86/intel/int3472.

For this to work properly the ov8865 driver must not bind to the I2C-client
for the OV8865 sensor until after the TPS68470 PMIC gpio, regulator and
clk MFD cells have all been fully setup.

The OV8865 on the Microsoft Surface Go is just one example, all X86
devices using the Intel IPU3 camera block found on recent Intel SoCs
have similar issues where there is an INT3472 HID ACPI-device, which
describes the clks and regulators, and the driver for this INT3472 device
must be fully initialized before the sensor driver (any sensor driver)
binds for things to work properly.

On these devices the ACPI nodes describing the sensors all have a _DEP
dependency on the matching INT3472 ACPI device (there is one per sensor).

This allows solving the probe-ordering problem by delaying the enumeration
(instantiation of the I2C-client in the ov8865 example) of ACPI-devices
which have a _DEP dependency on an INT3472 device.

The new acpi_dev_ready_for_enumeration() helper used for this is also
exported because for devices, which have the enumeration_by_parent flag
set, the parent-driver will do its own scan of child ACPI devices and
it will try to enumerate those during its probe(). Code doing this such
as e.g. the i2c-core-acpi.c code must call this new helper to ensure
that it too delays the enumeration until all the _DEP dependencies are
met on devices which have the new honor_deps flag set.

Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Changes in v4:
- Move the acpi_dev_ready_for_enumeration() check to acpi_bus_attach()
  (replacing the acpi_device_is_present() check there)
---
 drivers/acpi/scan.c     | 37 +++++++++++++++++++++++++++++++++----
 include/acpi/acpi_bus.h |  5 ++++-
 2 files changed, 37 insertions(+), 5 deletions(-)

diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
index c8d359bf8f73..526e823a33fb 100644
--- a/drivers/acpi/scan.c
+++ b/drivers/acpi/scan.c
@@ -797,6 +797,12 @@ static const char * const acpi_ignore_dep_ids[] = {
 	NULL
 };
 
+/* List of HIDs for which we honor deps of matching ACPI devs, when checking _DEP lists. */
+static const char * const acpi_honor_dep_ids[] = {
+	"INT3472", /* Camera sensor PMIC / clk and regulator info */
+	NULL
+};
+
 static struct acpi_device *acpi_bus_get_parent(acpi_handle handle)
 {
 	struct acpi_device *device = NULL;
@@ -1762,8 +1768,12 @@ static void acpi_scan_dep_init(struct acpi_device *adev)
 	struct acpi_dep_data *dep;
 
 	list_for_each_entry(dep, &acpi_dep_list, node) {
-		if (dep->consumer == adev->handle)
+		if (dep->consumer == adev->handle) {
+			if (dep->honor_dep)
+				adev->flags.honor_deps = 1;
+
 			adev->dep_unmet++;
+		}
 	}
 }
 
@@ -1967,7 +1977,7 @@ static u32 acpi_scan_check_dep(acpi_handle handle, bool check_dep)
 	for (count = 0, i = 0; i < dep_devices.count; i++) {
 		struct acpi_device_info *info;
 		struct acpi_dep_data *dep;
-		bool skip;
+		bool skip, honor_dep;
 
 		status = acpi_get_object_info(dep_devices.handles[i], &info);
 		if (ACPI_FAILURE(status)) {
@@ -1976,6 +1986,7 @@ static u32 acpi_scan_check_dep(acpi_handle handle, bool check_dep)
 		}
 
 		skip = acpi_info_matches_ids(info, acpi_ignore_dep_ids);
+		honor_dep = acpi_info_matches_ids(info, acpi_honor_dep_ids);
 		kfree(info);
 
 		if (skip)
@@ -1989,6 +2000,7 @@ static u32 acpi_scan_check_dep(acpi_handle handle, bool check_dep)
 
 		dep->supplier = dep_devices.handles[i];
 		dep->consumer = handle;
+		dep->honor_dep = honor_dep;
 
 		mutex_lock(&acpi_dep_list_lock);
 		list_add_tail(&dep->node , &acpi_dep_list);
@@ -2155,8 +2167,8 @@ static void acpi_bus_attach(struct acpi_device *device, bool first_pass)
 		register_dock_dependent_device(device, ejd);
 
 	acpi_bus_get_status(device);
-	/* Skip devices that are not present. */
-	if (!acpi_device_is_present(device)) {
+	/* Skip devices that are not ready for enumeration (e.g. not present) */
+	if (!acpi_dev_ready_for_enumeration(device)) {
 		device->flags.initialized = false;
 		acpi_device_clear_enumerated(device);
 		device->flags.power_manageable = 0;
@@ -2318,6 +2330,23 @@ void acpi_dev_clear_dependencies(struct acpi_device *supplier)
 }
 EXPORT_SYMBOL_GPL(acpi_dev_clear_dependencies);
 
+/**
+ * acpi_dev_ready_for_enumeration - Check if the ACPI device is ready for enumeration
+ * @device: Pointer to the &struct acpi_device to check
+ *
+ * Check if the device is present and has no unmet dependencies.
+ *
+ * Return true if the device is ready for enumeratino. Otherwise, return false.
+ */
+bool acpi_dev_ready_for_enumeration(const struct acpi_device *device)
+{
+	if (device->flags.honor_deps && device->dep_unmet)
+		return false;
+
+	return acpi_device_is_present(device);
+}
+EXPORT_SYMBOL_GPL(acpi_dev_ready_for_enumeration);
+
 /**
  * acpi_dev_get_first_consumer_dev - Return ACPI device dependent on @supplier
  * @supplier: Pointer to the dependee device
diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h
index d6fe27b695c3..5895f6c7f6db 100644
--- a/include/acpi/acpi_bus.h
+++ b/include/acpi/acpi_bus.h
@@ -202,7 +202,8 @@ struct acpi_device_flags {
 	u32 coherent_dma:1;
 	u32 cca_seen:1;
 	u32 enumeration_by_parent:1;
-	u32 reserved:19;
+	u32 honor_deps:1;
+	u32 reserved:18;
 };
 
 /* File System */
@@ -285,6 +286,7 @@ struct acpi_dep_data {
 	struct list_head node;
 	acpi_handle supplier;
 	acpi_handle consumer;
+	bool honor_dep;
 };
 
 /* Performance Management */
@@ -694,6 +696,7 @@ static inline bool acpi_device_can_poweroff(struct acpi_device *adev)
 bool acpi_dev_hid_uid_match(struct acpi_device *adev, const char *hid2, const char *uid2);
 
 void acpi_dev_clear_dependencies(struct acpi_device *supplier);
+bool acpi_dev_ready_for_enumeration(const struct acpi_device *device);
 struct acpi_device *acpi_dev_get_first_consumer_dev(struct acpi_device *supplier);
 struct acpi_device *
 acpi_dev_get_next_match_dev(struct acpi_device *adev, const char *hid, const char *uid, s64 hrv);
-- 
2.33.1

[PATCH v6 02/15] i2c: acpi: Use acpi_dev_ready_for_enumeration() helper

[Hans de Goede]

The clk and regulator frameworks expect clk/regulator consumer-devices
to have info about the consumed clks/regulators described in the device's
fw_node.

To work around cases where this info is not present in the firmware tables,
which is often the case on x86/ACPI devices, both frameworks allow the
provider-driver to attach info about consumers to the clks/regulators
when registering these.

This causes problems with the probe ordering wrt drivers for consumers
of these clks/regulators. Since the lookups are only registered when the
provider-driver binds, trying to get these clks/regulators before then
results in a -ENOENT error for clks and a dummy regulator for regulators.

To ensure the correct probe-ordering the ACPI core has code to defer the
enumeration of consumers affected by this until the providers are ready.

Call the new acpi_dev_ready_for_enumeration() helper to avoid
enumerating / instantiating i2c-clients too early.

Acked-by: Wolfram Sang <wsa@kernel.org>
Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/i2c/i2c-core-acpi.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/i2c/i2c-core-acpi.c b/drivers/i2c/i2c-core-acpi.c
index 92c1cc07ed46..04338cbd08a9 100644
--- a/drivers/i2c/i2c-core-acpi.c
+++ b/drivers/i2c/i2c-core-acpi.c
@@ -144,9 +144,12 @@ static int i2c_acpi_do_lookup(struct acpi_device *adev,
 	struct list_head resource_list;
 	int ret;
 
-	if (acpi_bus_get_status(adev) || !adev->status.present)
+	if (acpi_bus_get_status(adev))
 		return -EINVAL;
 
+	if (!acpi_dev_ready_for_enumeration(adev))
+		return -ENODEV;
+
 	if (acpi_match_device_ids(adev, i2c_acpi_ignored_device_ids) == 0)
 		return -ENODEV;
 
-- 
2.33.1

Re: [PATCH v6 02/15] i2c: acpi: Use acpi_dev_ready_for_enumeration() helper

[Mika Westerberg]

On Thu, Nov 25, 2021 at 05:53:59PM +0100, Hans de Goede wrote:
> The clk and regulator frameworks expect clk/regulator consumer-devices
> to have info about the consumed clks/regulators described in the device's
> fw_node.
> 
> To work around cases where this info is not present in the firmware tables,
> which is often the case on x86/ACPI devices, both frameworks allow the
> provider-driver to attach info about consumers to the clks/regulators
> when registering these.
> 
> This causes problems with the probe ordering wrt drivers for consumers
> of these clks/regulators. Since the lookups are only registered when the
> provider-driver binds, trying to get these clks/regulators before then
> results in a -ENOENT error for clks and a dummy regulator for regulators.
> 
> To ensure the correct probe-ordering the ACPI core has code to defer the
> enumeration of consumers affected by this until the providers are ready.
> 
> Call the new acpi_dev_ready_for_enumeration() helper to avoid
> enumerating / instantiating i2c-clients too early.
> 
> Acked-by: Wolfram Sang <wsa@kernel.org>
> Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>

Acked-by: Mika Westerberg <mika.westerberg@linux.intel.com>

[PATCH v6 03/15] i2c: acpi: Add i2c_acpi_new_device_by_fwnode() function

[Hans de Goede]

Change i2c_acpi_new_device() into i2c_acpi_new_device_by_fwnode() and
add a static inline wrapper providing the old i2c_acpi_new_device()
behavior.

This is necessary because in some cases we may only have access
to the fwnode / acpi_device and not to the matching physical-node
struct device *.

Suggested-by: Andy Shevchenko <andy.shevchenko@gmail.com>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Changes in v6:
- New patch in v6 of this patch series
---
 drivers/i2c/i2c-core-acpi.c | 18 ++++++++++++------
 include/linux/i2c.h         | 17 +++++++++++++----
 2 files changed, 25 insertions(+), 10 deletions(-)

diff --git a/drivers/i2c/i2c-core-acpi.c b/drivers/i2c/i2c-core-acpi.c
index 04338cbd08a9..1db3cc5fc47f 100644
--- a/drivers/i2c/i2c-core-acpi.c
+++ b/drivers/i2c/i2c-core-acpi.c
@@ -476,8 +476,9 @@ struct notifier_block i2c_acpi_notifier = {
 };
 
 /**
- * i2c_acpi_new_device - Create i2c-client for the Nth I2cSerialBus resource
- * @dev:     Device owning the ACPI resources to get the client from
+ * i2c_acpi_new_device_by_fwnode - Create i2c-client for the Nth I2cSerialBus
+ * resource
+ * @fwnode:  fwnode with the ACPI resources to get the client from
  * @index:   Index of ACPI resource to get
  * @info:    describes the I2C device; note this is modified (addr gets set)
  * Context: can sleep
@@ -493,15 +494,20 @@ struct notifier_block i2c_acpi_notifier = {
  * Returns a pointer to the new i2c-client, or error pointer in case of failure.
  * Specifically, -EPROBE_DEFER is returned if the adapter is not found.
  */
-struct i2c_client *i2c_acpi_new_device(struct device *dev, int index,
-				       struct i2c_board_info *info)
+struct i2c_client *i2c_acpi_new_device_by_fwnode(struct fwnode_handle *fwnode,
+						 int index,
+						 struct i2c_board_info *info)
 {
-	struct acpi_device *adev = ACPI_COMPANION(dev);
 	struct i2c_acpi_lookup lookup;
 	struct i2c_adapter *adapter;
+	struct acpi_device *adev;
 	LIST_HEAD(resource_list);
 	int ret;
 
+	adev = to_acpi_device_node(fwnode);
+	if (!adev)
+		return ERR_PTR(-ENODEV);
+
 	memset(&lookup, 0, sizeof(lookup));
 	lookup.info = info;
 	lookup.device_handle = acpi_device_handle(adev);
@@ -523,7 +529,7 @@ struct i2c_client *i2c_acpi_new_device(struct device *dev, int index,
 
 	return i2c_new_client_device(adapter, info);
 }
-EXPORT_SYMBOL_GPL(i2c_acpi_new_device);
+EXPORT_SYMBOL_GPL(i2c_acpi_new_device_by_fwnode);
 
 bool i2c_acpi_waive_d0_probe(struct device *dev)
 {
diff --git a/include/linux/i2c.h b/include/linux/i2c.h
index 16119ac1aa97..7d4f52ceb7b5 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -1025,8 +1025,9 @@ bool i2c_acpi_get_i2c_resource(struct acpi_resource *ares,
 			       struct acpi_resource_i2c_serialbus **i2c);
 int i2c_acpi_client_count(struct acpi_device *adev);
 u32 i2c_acpi_find_bus_speed(struct device *dev);
-struct i2c_client *i2c_acpi_new_device(struct device *dev, int index,
-				       struct i2c_board_info *info);
+struct i2c_client *i2c_acpi_new_device_by_fwnode(struct fwnode_handle *fwnode,
+						 int index,
+						 struct i2c_board_info *info);
 struct i2c_adapter *i2c_acpi_find_adapter_by_handle(acpi_handle handle);
 bool i2c_acpi_waive_d0_probe(struct device *dev);
 #else
@@ -1043,8 +1044,9 @@ static inline u32 i2c_acpi_find_bus_speed(struct device *dev)
 {
 	return 0;
 }
-static inline struct i2c_client *i2c_acpi_new_device(struct device *dev,
-					int index, struct i2c_board_info *info)
+static inline struct i2c_client *i2c_acpi_new_device_by_fwnode(
+					struct fwnode_handle *fwnode, int index,
+					struct i2c_board_info *info)
 {
 	return ERR_PTR(-ENODEV);
 }
@@ -1058,4 +1060,11 @@ static inline bool i2c_acpi_waive_d0_probe(struct device *dev)
 }
 #endif /* CONFIG_ACPI */
 
+static inline struct i2c_client *i2c_acpi_new_device(struct device *dev,
+						     int index,
+						     struct i2c_board_info *info)
+{
+	return i2c_acpi_new_device_by_fwnode(dev_fwnode(dev), index, info);
+}
+
 #endif /* _LINUX_I2C_H */
-- 
2.33.1

Re: [PATCH v6 03/15] i2c: acpi: Add i2c_acpi_new_device_by_fwnode() function

[Andy Shevchenko]

On Thu, Nov 25, 2021 at 6:54 PM Hans de Goede <hdegoede@redhat.com> wrote:
>
> Change i2c_acpi_new_device() into i2c_acpi_new_device_by_fwnode() and
> add a static inline wrapper providing the old i2c_acpi_new_device()
> behavior.
>
> This is necessary because in some cases we may only have access
> to the fwnode / acpi_device and not to the matching physical-node
> struct device *.

One nit-pick below.
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
Thanks!

> Suggested-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
> Changes in v6:
> - New patch in v6 of this patch series
> ---
>  drivers/i2c/i2c-core-acpi.c | 18 ++++++++++++------
>  include/linux/i2c.h         | 17 +++++++++++++----
>  2 files changed, 25 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/i2c/i2c-core-acpi.c b/drivers/i2c/i2c-core-acpi.c
> index 04338cbd08a9..1db3cc5fc47f 100644
> --- a/drivers/i2c/i2c-core-acpi.c
> +++ b/drivers/i2c/i2c-core-acpi.c
> @@ -476,8 +476,9 @@ struct notifier_block i2c_acpi_notifier = {
>  };
>
>  /**
> - * i2c_acpi_new_device - Create i2c-client for the Nth I2cSerialBus resource
> - * @dev:     Device owning the ACPI resources to get the client from

> + * i2c_acpi_new_device_by_fwnode - Create i2c-client for the Nth I2cSerialBus
> + * resource

Can be on one line.

> + * @fwnode:  fwnode with the ACPI resources to get the client from
>   * @index:   Index of ACPI resource to get
>   * @info:    describes the I2C device; note this is modified (addr gets set)
>   * Context: can sleep
> @@ -493,15 +494,20 @@ struct notifier_block i2c_acpi_notifier = {
>   * Returns a pointer to the new i2c-client, or error pointer in case of failure.
>   * Specifically, -EPROBE_DEFER is returned if the adapter is not found.
>   */
> -struct i2c_client *i2c_acpi_new_device(struct device *dev, int index,
> -                                      struct i2c_board_info *info)
> +struct i2c_client *i2c_acpi_new_device_by_fwnode(struct fwnode_handle *fwnode,
> +                                                int index,
> +                                                struct i2c_board_info *info)
>  {
> -       struct acpi_device *adev = ACPI_COMPANION(dev);
>         struct i2c_acpi_lookup lookup;
>         struct i2c_adapter *adapter;
> +       struct acpi_device *adev;
>         LIST_HEAD(resource_list);
>         int ret;
>
> +       adev = to_acpi_device_node(fwnode);
> +       if (!adev)
> +               return ERR_PTR(-ENODEV);
> +
>         memset(&lookup, 0, sizeof(lookup));
>         lookup.info = info;
>         lookup.device_handle = acpi_device_handle(adev);
> @@ -523,7 +529,7 @@ struct i2c_client *i2c_acpi_new_device(struct device *dev, int index,
>
>         return i2c_new_client_device(adapter, info);
>  }
> -EXPORT_SYMBOL_GPL(i2c_acpi_new_device);
> +EXPORT_SYMBOL_GPL(i2c_acpi_new_device_by_fwnode);
>
>  bool i2c_acpi_waive_d0_probe(struct device *dev)
>  {
> diff --git a/include/linux/i2c.h b/include/linux/i2c.h
> index 16119ac1aa97..7d4f52ceb7b5 100644
> --- a/include/linux/i2c.h
> +++ b/include/linux/i2c.h
> @@ -1025,8 +1025,9 @@ bool i2c_acpi_get_i2c_resource(struct acpi_resource *ares,
>                                struct acpi_resource_i2c_serialbus **i2c);
>  int i2c_acpi_client_count(struct acpi_device *adev);
>  u32 i2c_acpi_find_bus_speed(struct device *dev);
> -struct i2c_client *i2c_acpi_new_device(struct device *dev, int index,
> -                                      struct i2c_board_info *info);
> +struct i2c_client *i2c_acpi_new_device_by_fwnode(struct fwnode_handle *fwnode,
> +                                                int index,
> +                                                struct i2c_board_info *info);
>  struct i2c_adapter *i2c_acpi_find_adapter_by_handle(acpi_handle handle);
>  bool i2c_acpi_waive_d0_probe(struct device *dev);
>  #else
> @@ -1043,8 +1044,9 @@ static inline u32 i2c_acpi_find_bus_speed(struct device *dev)
>  {
>         return 0;
>  }
> -static inline struct i2c_client *i2c_acpi_new_device(struct device *dev,
> -                                       int index, struct i2c_board_info *info)
> +static inline struct i2c_client *i2c_acpi_new_device_by_fwnode(
> +                                       struct fwnode_handle *fwnode, int index,
> +                                       struct i2c_board_info *info)
>  {
>         return ERR_PTR(-ENODEV);
>  }
> @@ -1058,4 +1060,11 @@ static inline bool i2c_acpi_waive_d0_probe(struct device *dev)
>  }
>  #endif /* CONFIG_ACPI */
>
> +static inline struct i2c_client *i2c_acpi_new_device(struct device *dev,
> +                                                    int index,
> +                                                    struct i2c_board_info *info)
> +{
> +       return i2c_acpi_new_device_by_fwnode(dev_fwnode(dev), index, info);
> +}
> +
>  #endif /* _LINUX_I2C_H */
> --
> 2.33.1
>


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v6 03/15] i2c: acpi: Add i2c_acpi_new_device_by_fwnode() function

[Hans de Goede]

Hi,

On 11/25/21 18:22, Andy Shevchenko wrote:
> On Thu, Nov 25, 2021 at 6:54 PM Hans de Goede <hdegoede@redhat.com> wrote:
>>
>> Change i2c_acpi_new_device() into i2c_acpi_new_device_by_fwnode() and
>> add a static inline wrapper providing the old i2c_acpi_new_device()
>> behavior.
>>
>> This is necessary because in some cases we may only have access
>> to the fwnode / acpi_device and not to the matching physical-node
>> struct device *.
> 
> One nit-pick below.
> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>

Thank you, I've fixed the nit-pick in my local tree, either for v7 of
the series, of for in the immutable branch which I plan to create for
this.

Regards,

Hans



>> Suggested-by: Andy Shevchenko <andy.shevchenko@gmail.com>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>> Changes in v6:
>> - New patch in v6 of this patch series
>> ---
>>  drivers/i2c/i2c-core-acpi.c | 18 ++++++++++++------
>>  include/linux/i2c.h         | 17 +++++++++++++----
>>  2 files changed, 25 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/i2c/i2c-core-acpi.c b/drivers/i2c/i2c-core-acpi.c
>> index 04338cbd08a9..1db3cc5fc47f 100644
>> --- a/drivers/i2c/i2c-core-acpi.c
>> +++ b/drivers/i2c/i2c-core-acpi.c
>> @@ -476,8 +476,9 @@ struct notifier_block i2c_acpi_notifier = {
>>  };
>>
>>  /**
>> - * i2c_acpi_new_device - Create i2c-client for the Nth I2cSerialBus resource
>> - * @dev:     Device owning the ACPI resources to get the client from
> 
>> + * i2c_acpi_new_device_by_fwnode - Create i2c-client for the Nth I2cSerialBus
>> + * resource
> 
> Can be on one line.
> 
>> + * @fwnode:  fwnode with the ACPI resources to get the client from
>>   * @index:   Index of ACPI resource to get
>>   * @info:    describes the I2C device; note this is modified (addr gets set)
>>   * Context: can sleep
>> @@ -493,15 +494,20 @@ struct notifier_block i2c_acpi_notifier = {
>>   * Returns a pointer to the new i2c-client, or error pointer in case of failure.
>>   * Specifically, -EPROBE_DEFER is returned if the adapter is not found.
>>   */
>> -struct i2c_client *i2c_acpi_new_device(struct device *dev, int index,
>> -                                      struct i2c_board_info *info)
>> +struct i2c_client *i2c_acpi_new_device_by_fwnode(struct fwnode_handle *fwnode,
>> +                                                int index,
>> +                                                struct i2c_board_info *info)
>>  {
>> -       struct acpi_device *adev = ACPI_COMPANION(dev);
>>         struct i2c_acpi_lookup lookup;
>>         struct i2c_adapter *adapter;
>> +       struct acpi_device *adev;
>>         LIST_HEAD(resource_list);
>>         int ret;
>>
>> +       adev = to_acpi_device_node(fwnode);
>> +       if (!adev)
>> +               return ERR_PTR(-ENODEV);
>> +
>>         memset(&lookup, 0, sizeof(lookup));
>>         lookup.info = info;
>>         lookup.device_handle = acpi_device_handle(adev);
>> @@ -523,7 +529,7 @@ struct i2c_client *i2c_acpi_new_device(struct device *dev, int index,
>>
>>         return i2c_new_client_device(adapter, info);
>>  }
>> -EXPORT_SYMBOL_GPL(i2c_acpi_new_device);
>> +EXPORT_SYMBOL_GPL(i2c_acpi_new_device_by_fwnode);
>>
>>  bool i2c_acpi_waive_d0_probe(struct device *dev)
>>  {
>> diff --git a/include/linux/i2c.h b/include/linux/i2c.h
>> index 16119ac1aa97..7d4f52ceb7b5 100644
>> --- a/include/linux/i2c.h
>> +++ b/include/linux/i2c.h
>> @@ -1025,8 +1025,9 @@ bool i2c_acpi_get_i2c_resource(struct acpi_resource *ares,
>>                                struct acpi_resource_i2c_serialbus **i2c);
>>  int i2c_acpi_client_count(struct acpi_device *adev);
>>  u32 i2c_acpi_find_bus_speed(struct device *dev);
>> -struct i2c_client *i2c_acpi_new_device(struct device *dev, int index,
>> -                                      struct i2c_board_info *info);
>> +struct i2c_client *i2c_acpi_new_device_by_fwnode(struct fwnode_handle *fwnode,
>> +                                                int index,
>> +                                                struct i2c_board_info *info);
>>  struct i2c_adapter *i2c_acpi_find_adapter_by_handle(acpi_handle handle);
>>  bool i2c_acpi_waive_d0_probe(struct device *dev);
>>  #else
>> @@ -1043,8 +1044,9 @@ static inline u32 i2c_acpi_find_bus_speed(struct device *dev)
>>  {
>>         return 0;
>>  }
>> -static inline struct i2c_client *i2c_acpi_new_device(struct device *dev,
>> -                                       int index, struct i2c_board_info *info)
>> +static inline struct i2c_client *i2c_acpi_new_device_by_fwnode(
>> +                                       struct fwnode_handle *fwnode, int index,
>> +                                       struct i2c_board_info *info)
>>  {
>>         return ERR_PTR(-ENODEV);
>>  }
>> @@ -1058,4 +1060,11 @@ static inline bool i2c_acpi_waive_d0_probe(struct device *dev)
>>  }
>>  #endif /* CONFIG_ACPI */
>>
>> +static inline struct i2c_client *i2c_acpi_new_device(struct device *dev,
>> +                                                    int index,
>> +                                                    struct i2c_board_info *info)
>> +{
>> +       return i2c_acpi_new_device_by_fwnode(dev_fwnode(dev), index, info);
>> +}
>> +
>>  #endif /* _LINUX_I2C_H */
>> --
>> 2.33.1
>>
> 
> 

Re: [PATCH v6 03/15] i2c: acpi: Add i2c_acpi_new_device_by_fwnode() function

[Mika Westerberg]

On Thu, Nov 25, 2021 at 05:54:00PM +0100, Hans de Goede wrote:
> Change i2c_acpi_new_device() into i2c_acpi_new_device_by_fwnode() and
> add a static inline wrapper providing the old i2c_acpi_new_device()
> behavior.
> 
> This is necessary because in some cases we may only have access
> to the fwnode / acpi_device and not to the matching physical-node
> struct device *.
> 
> Suggested-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>

Acked-by: Mika Westerberg <mika.westerberg@linux.intel.com>

Re: [PATCH v6 03/15] i2c: acpi: Add i2c_acpi_new_device_by_fwnode() function

[Wolfram Sang]

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

On Thu, Nov 25, 2021 at 05:54:00PM +0100, Hans de Goede wrote:
> Change i2c_acpi_new_device() into i2c_acpi_new_device_by_fwnode() and
> add a static inline wrapper providing the old i2c_acpi_new_device()
> behavior.
> 
> This is necessary because in some cases we may only have access
> to the fwnode / acpi_device and not to the matching physical-node
> struct device *.
> 
> Suggested-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>

Acked-by: Wolfram Sang <wsa@kernel.org>


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH v6 04/15] platform_data: Add linux/platform_data/tps68470.h file

[Hans de Goede]

The clk and regulator frameworks expect clk/regulator consumer-devices
to have info about the consumed clks/regulators described in the device's
fw_node.

To work around cases where this info is not present in the firmware tables,
which is often the case on x86/ACPI devices, both frameworks allow the
provider-driver to attach info about consumers to the provider-device
during probe/registration of the provider device.

The TI TPS68470 PMIC is used x86/ACPI devices with the consumer-info
missing from the ACPI tables. Thus the tps68470-clk and tps68470-regulator
drivers must provide the consumer-info at probe time.

Define tps68470_clk_platform_data and tps68470_regulator_platform_data
structs to allow the x86 platform code to pass the necessary consumer info
to these drivers.

Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 include/linux/platform_data/tps68470.h | 35 ++++++++++++++++++++++++++
 1 file changed, 35 insertions(+)
 create mode 100644 include/linux/platform_data/tps68470.h

diff --git a/include/linux/platform_data/tps68470.h b/include/linux/platform_data/tps68470.h
new file mode 100644
index 000000000000..126d082c3f2e
--- /dev/null
+++ b/include/linux/platform_data/tps68470.h
@@ -0,0 +1,35 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+/*
+ * TI TPS68470 PMIC platform data definition.
+ *
+ * Copyright (c) 2021 Red Hat Inc.
+ *
+ * Red Hat authors:
+ * Hans de Goede <hdegoede@redhat.com>
+ */
+#ifndef __PDATA_TPS68470_H
+#define __PDATA_TPS68470_H
+
+enum tps68470_regulators {
+	TPS68470_CORE,
+	TPS68470_ANA,
+	TPS68470_VCM,
+	TPS68470_VIO,
+	TPS68470_VSIO,
+	TPS68470_AUX1,
+	TPS68470_AUX2,
+	TPS68470_NUM_REGULATORS
+};
+
+struct regulator_init_data;
+
+struct tps68470_regulator_platform_data {
+	const struct regulator_init_data *reg_init_data[TPS68470_NUM_REGULATORS];
+};
+
+struct tps68470_clk_platform_data {
+	const char *consumer_dev_name;
+	const char *consumer_con_id;
+};
+
+#endif
-- 
2.33.1

[PATCH v6 05/15] regulator: Introduce tps68470-regulator driver

[Hans de Goede]

The TPS68470 PMIC provides Clocks, GPIOs and Regulators. At present in
the kernel the Regulators and Clocks are controlled by an OpRegion
driver designed to work with power control methods defined in ACPI, but
some platforms lack those methods, meaning drivers need to be able to
consume the resources of these chips through the usual frameworks.

This commit adds a driver for the regulators provided by the tps68470,
and is designed to bind to the platform_device registered by the
intel_skl_int3472 module.

This is based on this out of tree driver written by Intel:
https://github.com/intel/linux-intel-lts/blob/4.14/base/drivers/regulator/tps68470-regulator.c
with various cleanups added.

Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Changes in v6:
- Drop the unused volt_table argument from the TPS68470_REGULATOR() macro
- While working on VCM (voice coil motor) support for the camera-module behind
  this PMIC I learned that the VIO voltage is always on. Instead of pointing its
  enable_reg and enable_mask at the same register-bits as the VSIO regulator
  (which is wrong), add a new tps68470_always_on_reg_ops struct without
  is_enabled, enable and disable ops and use that for the VIO regulator.

Changes in v5:
- Small comment / code cleanups based on review from Andy

Changes in v4:
- Make the top comment block use c++ style comments
- Drop the bogus builtin regulator_init_data
- Add || COMPILE_TEST to Kconfig snippet
- Make the driver enable the PMIC clk when enabling the Core buck
  regulator, this switching regulator needs the PLL to be on

Changes in v2:
- Update the comment on why a subsys_initcall is used to register the drv
- Make struct regulator_ops const
---
 drivers/regulator/Kconfig              |   9 ++
 drivers/regulator/Makefile             |   1 +
 drivers/regulator/tps68470-regulator.c | 201 +++++++++++++++++++++++++
 3 files changed, 211 insertions(+)
 create mode 100644 drivers/regulator/tps68470-regulator.c

diff --git a/drivers/regulator/Kconfig b/drivers/regulator/Kconfig
index 6be9b1c8a615..ebe46e09510e 100644
--- a/drivers/regulator/Kconfig
+++ b/drivers/regulator/Kconfig
@@ -1339,6 +1339,15 @@ config REGULATOR_TPS65912
 	help
 	    This driver supports TPS65912 voltage regulator chip.
 
+config REGULATOR_TPS68470
+	tristate "TI TPS68470 PMIC Regulators Driver"
+	depends on INTEL_SKL_INT3472 || COMPILE_TEST
+	help
+	  This driver adds support for the TPS68470 PMIC to register
+	  regulators against the usual framework.
+
+	  The module will be called "tps68470-regulator".
+
 config REGULATOR_TWL4030
 	tristate "TI TWL4030/TWL5030/TWL6030/TPS659x0 PMIC"
 	depends on TWL4030_CORE
diff --git a/drivers/regulator/Makefile b/drivers/regulator/Makefile
index b07d2a22df0b..257331d2caed 100644
--- a/drivers/regulator/Makefile
+++ b/drivers/regulator/Makefile
@@ -159,6 +159,7 @@ obj-$(CONFIG_REGULATOR_TPS6586X) += tps6586x-regulator.o
 obj-$(CONFIG_REGULATOR_TPS65910) += tps65910-regulator.o
 obj-$(CONFIG_REGULATOR_TPS65912) += tps65912-regulator.o
 obj-$(CONFIG_REGULATOR_TPS65132) += tps65132-regulator.o
+obj-$(CONFIG_REGULATOR_TPS68470) += tps68470-regulator.o
 obj-$(CONFIG_REGULATOR_TWL4030) += twl-regulator.o twl6030-regulator.o
 obj-$(CONFIG_REGULATOR_UNIPHIER) += uniphier-regulator.o
 obj-$(CONFIG_REGULATOR_VCTRL) += vctrl-regulator.o
diff --git a/drivers/regulator/tps68470-regulator.c b/drivers/regulator/tps68470-regulator.c
new file mode 100644
index 000000000000..9ad2d1eae8fe
--- /dev/null
+++ b/drivers/regulator/tps68470-regulator.c
@@ -0,0 +1,201 @@
+// SPDX-License-Identifier: GPL-2.0
+//
+// Regulator driver for TPS68470 PMIC
+//
+// Copyright (c) 2021 Red Hat Inc.
+// Copyright (C) 2018 Intel Corporation
+//
+// Authors:
+//	Hans de Goede <hdegoede@redhat.com>
+//	Zaikuo Wang <zaikuo.wang@intel.com>
+//	Tianshu Qiu <tian.shu.qiu@intel.com>
+//	Jian Xu Zheng <jian.xu.zheng@intel.com>
+//	Yuning Pu <yuning.pu@intel.com>
+//	Rajmohan Mani <rajmohan.mani@intel.com>
+
+#include <linux/clk.h>
+#include <linux/device.h>
+#include <linux/err.h>
+#include <linux/init.h>
+#include <linux/kernel.h>
+#include <linux/mfd/tps68470.h>
+#include <linux/module.h>
+#include <linux/platform_data/tps68470.h>
+#include <linux/platform_device.h>
+#include <linux/regulator/driver.h>
+#include <linux/regulator/machine.h>
+
+struct tps68470_regulator_data {
+	struct clk *clk;
+};
+
+#define TPS68470_REGULATOR(_name, _id, _ops, _n,			\
+			   _vr, _vm, _er, _em, _lr, _nlr)		\
+	[TPS68470_ ## _name] = {					\
+		.name			= # _name,			\
+		.id			= _id,				\
+		.ops			= &_ops,			\
+		.n_voltages		= _n,				\
+		.type			= REGULATOR_VOLTAGE,		\
+		.owner			= THIS_MODULE,			\
+		.vsel_reg		= _vr,				\
+		.vsel_mask		= _vm,				\
+		.enable_reg		= _er,				\
+		.enable_mask		= _em,				\
+		.linear_ranges		= _lr,				\
+		.n_linear_ranges	= _nlr,				\
+	}
+
+static const struct linear_range tps68470_ldo_ranges[] = {
+	REGULATOR_LINEAR_RANGE(875000, 0, 125, 17800),
+};
+
+static const struct linear_range tps68470_core_ranges[] = {
+	REGULATOR_LINEAR_RANGE(900000, 0, 42, 25000),
+};
+
+static int tps68470_regulator_enable(struct regulator_dev *rdev)
+{
+	struct tps68470_regulator_data *data = rdev->reg_data;
+	int ret;
+
+	/* The Core buck regulator needs the PMIC's PLL to be enabled */
+	if (rdev->desc->id == TPS68470_CORE) {
+		ret = clk_prepare_enable(data->clk);
+		if (ret) {
+			dev_err(&rdev->dev, "Error enabling TPS68470 clock\n");
+			return ret;
+		}
+	}
+
+	return regulator_enable_regmap(rdev);
+}
+
+static int tps68470_regulator_disable(struct regulator_dev *rdev)
+{
+	struct tps68470_regulator_data *data = rdev->reg_data;
+
+	if (rdev->desc->id == TPS68470_CORE)
+		clk_disable_unprepare(data->clk);
+
+	return regulator_disable_regmap(rdev);
+}
+
+/* Operations permitted on DCDCx, LDO2, LDO3 and LDO4 */
+static const struct regulator_ops tps68470_regulator_ops = {
+	.is_enabled		= regulator_is_enabled_regmap,
+	.enable			= tps68470_regulator_enable,
+	.disable		= tps68470_regulator_disable,
+	.get_voltage_sel	= regulator_get_voltage_sel_regmap,
+	.set_voltage_sel	= regulator_set_voltage_sel_regmap,
+	.list_voltage		= regulator_list_voltage_linear_range,
+	.map_voltage		= regulator_map_voltage_linear_range,
+};
+
+static const struct regulator_ops tps68470_always_on_reg_ops = {
+	.get_voltage_sel	= regulator_get_voltage_sel_regmap,
+	.set_voltage_sel	= regulator_set_voltage_sel_regmap,
+	.list_voltage		= regulator_list_voltage_linear_range,
+	.map_voltage		= regulator_map_voltage_linear_range,
+};
+
+static const struct regulator_desc regulators[] = {
+	TPS68470_REGULATOR(CORE, TPS68470_CORE, tps68470_regulator_ops, 43,
+			   TPS68470_REG_VDVAL, TPS68470_VDVAL_DVOLT_MASK,
+			   TPS68470_REG_VDCTL, TPS68470_VDCTL_EN_MASK,
+			   tps68470_core_ranges, ARRAY_SIZE(tps68470_core_ranges)),
+	TPS68470_REGULATOR(ANA, TPS68470_ANA, tps68470_regulator_ops, 126,
+			   TPS68470_REG_VAVAL, TPS68470_VAVAL_AVOLT_MASK,
+			   TPS68470_REG_VACTL, TPS68470_VACTL_EN_MASK,
+			   tps68470_ldo_ranges, ARRAY_SIZE(tps68470_ldo_ranges)),
+	TPS68470_REGULATOR(VCM, TPS68470_VCM, tps68470_regulator_ops, 126,
+			   TPS68470_REG_VCMVAL, TPS68470_VCMVAL_VCVOLT_MASK,
+			   TPS68470_REG_VCMCTL, TPS68470_VCMCTL_EN_MASK,
+			   tps68470_ldo_ranges, ARRAY_SIZE(tps68470_ldo_ranges)),
+	TPS68470_REGULATOR(VIO, TPS68470_VIO, tps68470_always_on_reg_ops, 126,
+			   TPS68470_REG_VIOVAL, TPS68470_VIOVAL_IOVOLT_MASK,
+			   0, 0,
+			   tps68470_ldo_ranges, ARRAY_SIZE(tps68470_ldo_ranges)),
+/*
+ * (1) This regulator must have the same voltage as VIO if S_IO LDO is used to
+ *     power a sensor/VCM which I2C is daisy chained behind the PMIC.
+ * (2) If there is no I2C daisy chain it can be set freely.
+ */
+	TPS68470_REGULATOR(VSIO, TPS68470_VSIO, tps68470_regulator_ops, 126,
+			   TPS68470_REG_VSIOVAL, TPS68470_VSIOVAL_IOVOLT_MASK,
+			   TPS68470_REG_S_I2C_CTL, TPS68470_S_I2C_CTL_EN_MASK,
+			   tps68470_ldo_ranges, ARRAY_SIZE(tps68470_ldo_ranges)),
+	TPS68470_REGULATOR(AUX1, TPS68470_AUX1, tps68470_regulator_ops, 126,
+			   TPS68470_REG_VAUX1VAL, TPS68470_VAUX1VAL_AUX1VOLT_MASK,
+			   TPS68470_REG_VAUX1CTL, TPS68470_VAUX1CTL_EN_MASK,
+			   tps68470_ldo_ranges, ARRAY_SIZE(tps68470_ldo_ranges)),
+	TPS68470_REGULATOR(AUX2, TPS68470_AUX2, tps68470_regulator_ops, 126,
+			   TPS68470_REG_VAUX2VAL, TPS68470_VAUX2VAL_AUX2VOLT_MASK,
+			   TPS68470_REG_VAUX2CTL, TPS68470_VAUX2CTL_EN_MASK,
+			   tps68470_ldo_ranges, ARRAY_SIZE(tps68470_ldo_ranges)),
+};
+
+static int tps68470_regulator_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct tps68470_regulator_platform_data *pdata = dev_get_platdata(dev);
+	struct tps68470_regulator_data *data;
+	struct regulator_config config = { };
+	struct regulator_dev *rdev;
+	int i;
+
+	data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
+	if (!data)
+		return -ENOMEM;
+
+	data->clk = devm_clk_get(dev, "tps68470-clk");
+	if (IS_ERR(data->clk))
+		return dev_err_probe(dev, PTR_ERR(data->clk), "getting tps68470-clk\n");
+
+	config.dev = dev->parent;
+	config.regmap = dev_get_drvdata(dev->parent);
+	config.driver_data = data;
+
+	for (i = 0; i < TPS68470_NUM_REGULATORS; i++) {
+		if (pdata)
+			config.init_data = pdata->reg_init_data[i];
+		else
+			config.init_data = NULL;
+
+		rdev = devm_regulator_register(dev, &regulators[i], &config);
+		if (IS_ERR(rdev))
+			return dev_err_probe(dev, PTR_ERR(data->clk),
+					     "registering %s regulator\n",
+					     regulators[i].name);
+	}
+
+	return 0;
+}
+
+static struct platform_driver tps68470_regulator_driver = {
+	.driver = {
+		.name = "tps68470-regulator",
+	},
+	.probe = tps68470_regulator_probe,
+};
+
+/*
+ * The ACPI tps68470 probe-ordering depends on the clk/gpio/regulator drivers
+ * registering before the drivers for the camera-sensors which use them bind.
+ * subsys_initcall() ensures this when the drivers are builtin.
+ */
+static int __init tps68470_regulator_init(void)
+{
+	return platform_driver_register(&tps68470_regulator_driver);
+}
+subsys_initcall(tps68470_regulator_init);
+
+static void __exit tps68470_regulator_exit(void)
+{
+	platform_driver_unregister(&tps68470_regulator_driver);
+}
+module_exit(tps68470_regulator_exit);
+
+MODULE_ALIAS("platform:tps68470-regulator");
+MODULE_DESCRIPTION("TPS68470 voltage regulator driver");
+MODULE_LICENSE("GPL v2");
-- 
2.33.1

Re: [PATCH v6 05/15] regulator: Introduce tps68470-regulator driver

[Laurent Pinchart]

Hi Hans,

Thank you for the patch.

I've had a quick look and the driver seems fine. Just a few comments
below.

On Thu, Nov 25, 2021 at 05:54:02PM +0100, Hans de Goede wrote:
> The TPS68470 PMIC provides Clocks, GPIOs and Regulators. At present in
> the kernel the Regulators and Clocks are controlled by an OpRegion
> driver designed to work with power control methods defined in ACPI, but
> some platforms lack those methods, meaning drivers need to be able to
> consume the resources of these chips through the usual frameworks.
> 
> This commit adds a driver for the regulators provided by the tps68470,
> and is designed to bind to the platform_device registered by the
> intel_skl_int3472 module.
> 
> This is based on this out of tree driver written by Intel:
> https://github.com/intel/linux-intel-lts/blob/4.14/base/drivers/regulator/tps68470-regulator.c
> with various cleanups added.
> 
> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
> Changes in v6:
> - Drop the unused volt_table argument from the TPS68470_REGULATOR() macro
> - While working on VCM (voice coil motor) support for the camera-module behind
>   this PMIC I learned that the VIO voltage is always on. Instead of pointing its
>   enable_reg and enable_mask at the same register-bits as the VSIO regulator
>   (which is wrong), add a new tps68470_always_on_reg_ops struct without
>   is_enabled, enable and disable ops and use that for the VIO regulator.
> 
> Changes in v5:
> - Small comment / code cleanups based on review from Andy
> 
> Changes in v4:
> - Make the top comment block use c++ style comments
> - Drop the bogus builtin regulator_init_data
> - Add || COMPILE_TEST to Kconfig snippet
> - Make the driver enable the PMIC clk when enabling the Core buck
>   regulator, this switching regulator needs the PLL to be on
> 
> Changes in v2:
> - Update the comment on why a subsys_initcall is used to register the drv
> - Make struct regulator_ops const
> ---
>  drivers/regulator/Kconfig              |   9 ++
>  drivers/regulator/Makefile             |   1 +
>  drivers/regulator/tps68470-regulator.c | 201 +++++++++++++++++++++++++
>  3 files changed, 211 insertions(+)
>  create mode 100644 drivers/regulator/tps68470-regulator.c
> 
> diff --git a/drivers/regulator/Kconfig b/drivers/regulator/Kconfig
> index 6be9b1c8a615..ebe46e09510e 100644
> --- a/drivers/regulator/Kconfig
> +++ b/drivers/regulator/Kconfig
> @@ -1339,6 +1339,15 @@ config REGULATOR_TPS65912
>  	help
>  	    This driver supports TPS65912 voltage regulator chip.
>  
> +config REGULATOR_TPS68470
> +	tristate "TI TPS68470 PMIC Regulators Driver"
> +	depends on INTEL_SKL_INT3472 || COMPILE_TEST
> +	help
> +	  This driver adds support for the TPS68470 PMIC to register
> +	  regulators against the usual framework.
> +
> +	  The module will be called "tps68470-regulator".
> +
>  config REGULATOR_TWL4030
>  	tristate "TI TWL4030/TWL5030/TWL6030/TPS659x0 PMIC"
>  	depends on TWL4030_CORE
> diff --git a/drivers/regulator/Makefile b/drivers/regulator/Makefile
> index b07d2a22df0b..257331d2caed 100644
> --- a/drivers/regulator/Makefile
> +++ b/drivers/regulator/Makefile
> @@ -159,6 +159,7 @@ obj-$(CONFIG_REGULATOR_TPS6586X) += tps6586x-regulator.o
>  obj-$(CONFIG_REGULATOR_TPS65910) += tps65910-regulator.o
>  obj-$(CONFIG_REGULATOR_TPS65912) += tps65912-regulator.o
>  obj-$(CONFIG_REGULATOR_TPS65132) += tps65132-regulator.o
> +obj-$(CONFIG_REGULATOR_TPS68470) += tps68470-regulator.o
>  obj-$(CONFIG_REGULATOR_TWL4030) += twl-regulator.o twl6030-regulator.o
>  obj-$(CONFIG_REGULATOR_UNIPHIER) += uniphier-regulator.o
>  obj-$(CONFIG_REGULATOR_VCTRL) += vctrl-regulator.o
> diff --git a/drivers/regulator/tps68470-regulator.c b/drivers/regulator/tps68470-regulator.c
> new file mode 100644
> index 000000000000..9ad2d1eae8fe
> --- /dev/null
> +++ b/drivers/regulator/tps68470-regulator.c
> @@ -0,0 +1,201 @@
> +// SPDX-License-Identifier: GPL-2.0
> +//
> +// Regulator driver for TPS68470 PMIC
> +//
> +// Copyright (c) 2021 Red Hat Inc.
> +// Copyright (C) 2018 Intel Corporation
> +//
> +// Authors:
> +//	Hans de Goede <hdegoede@redhat.com>
> +//	Zaikuo Wang <zaikuo.wang@intel.com>
> +//	Tianshu Qiu <tian.shu.qiu@intel.com>
> +//	Jian Xu Zheng <jian.xu.zheng@intel.com>
> +//	Yuning Pu <yuning.pu@intel.com>
> +//	Rajmohan Mani <rajmohan.mani@intel.com>
> +
> +#include <linux/clk.h>
> +#include <linux/device.h>
> +#include <linux/err.h>
> +#include <linux/init.h>
> +#include <linux/kernel.h>
> +#include <linux/mfd/tps68470.h>
> +#include <linux/module.h>
> +#include <linux/platform_data/tps68470.h>
> +#include <linux/platform_device.h>
> +#include <linux/regulator/driver.h>
> +#include <linux/regulator/machine.h>
> +
> +struct tps68470_regulator_data {
> +	struct clk *clk;
> +};
> +
> +#define TPS68470_REGULATOR(_name, _id, _ops, _n,			\
> +			   _vr, _vm, _er, _em, _lr, _nlr)		\
> +	[TPS68470_ ## _name] = {					\
> +		.name			= # _name,			\
> +		.id			= _id,				\
> +		.ops			= &_ops,			\
> +		.n_voltages		= _n,				\
> +		.type			= REGULATOR_VOLTAGE,		\
> +		.owner			= THIS_MODULE,			\
> +		.vsel_reg		= _vr,				\
> +		.vsel_mask		= _vm,				\
> +		.enable_reg		= _er,				\
> +		.enable_mask		= _em,				\
> +		.linear_ranges		= _lr,				\
> +		.n_linear_ranges	= _nlr,				\
> +	}
> +
> +static const struct linear_range tps68470_ldo_ranges[] = {
> +	REGULATOR_LINEAR_RANGE(875000, 0, 125, 17800),
> +};
> +
> +static const struct linear_range tps68470_core_ranges[] = {
> +	REGULATOR_LINEAR_RANGE(900000, 0, 42, 25000),
> +};
> +
> +static int tps68470_regulator_enable(struct regulator_dev *rdev)
> +{
> +	struct tps68470_regulator_data *data = rdev->reg_data;
> +	int ret;
> +
> +	/* The Core buck regulator needs the PMIC's PLL to be enabled */
> +	if (rdev->desc->id == TPS68470_CORE) {
> +		ret = clk_prepare_enable(data->clk);
> +		if (ret) {
> +			dev_err(&rdev->dev, "Error enabling TPS68470 clock\n");
> +			return ret;
> +		}
> +	}
> +
> +	return regulator_enable_regmap(rdev);
> +}
> +
> +static int tps68470_regulator_disable(struct regulator_dev *rdev)
> +{
> +	struct tps68470_regulator_data *data = rdev->reg_data;
> +
> +	if (rdev->desc->id == TPS68470_CORE)
> +		clk_disable_unprepare(data->clk);
> +
> +	return regulator_disable_regmap(rdev);
> +}
> +
> +/* Operations permitted on DCDCx, LDO2, LDO3 and LDO4 */
> +static const struct regulator_ops tps68470_regulator_ops = {
> +	.is_enabled		= regulator_is_enabled_regmap,
> +	.enable			= tps68470_regulator_enable,
> +	.disable		= tps68470_regulator_disable,
> +	.get_voltage_sel	= regulator_get_voltage_sel_regmap,
> +	.set_voltage_sel	= regulator_set_voltage_sel_regmap,
> +	.list_voltage		= regulator_list_voltage_linear_range,
> +	.map_voltage		= regulator_map_voltage_linear_range,
> +};
> +
> +static const struct regulator_ops tps68470_always_on_reg_ops = {
> +	.get_voltage_sel	= regulator_get_voltage_sel_regmap,
> +	.set_voltage_sel	= regulator_set_voltage_sel_regmap,
> +	.list_voltage		= regulator_list_voltage_linear_range,
> +	.map_voltage		= regulator_map_voltage_linear_range,
> +};
> +
> +static const struct regulator_desc regulators[] = {
> +	TPS68470_REGULATOR(CORE, TPS68470_CORE, tps68470_regulator_ops, 43,
> +			   TPS68470_REG_VDVAL, TPS68470_VDVAL_DVOLT_MASK,
> +			   TPS68470_REG_VDCTL, TPS68470_VDCTL_EN_MASK,
> +			   tps68470_core_ranges, ARRAY_SIZE(tps68470_core_ranges)),
> +	TPS68470_REGULATOR(ANA, TPS68470_ANA, tps68470_regulator_ops, 126,
> +			   TPS68470_REG_VAVAL, TPS68470_VAVAL_AVOLT_MASK,
> +			   TPS68470_REG_VACTL, TPS68470_VACTL_EN_MASK,
> +			   tps68470_ldo_ranges, ARRAY_SIZE(tps68470_ldo_ranges)),
> +	TPS68470_REGULATOR(VCM, TPS68470_VCM, tps68470_regulator_ops, 126,
> +			   TPS68470_REG_VCMVAL, TPS68470_VCMVAL_VCVOLT_MASK,
> +			   TPS68470_REG_VCMCTL, TPS68470_VCMCTL_EN_MASK,
> +			   tps68470_ldo_ranges, ARRAY_SIZE(tps68470_ldo_ranges)),
> +	TPS68470_REGULATOR(VIO, TPS68470_VIO, tps68470_always_on_reg_ops, 126,
> +			   TPS68470_REG_VIOVAL, TPS68470_VIOVAL_IOVOLT_MASK,
> +			   0, 0,
> +			   tps68470_ldo_ranges, ARRAY_SIZE(tps68470_ldo_ranges)),
> +/*
> + * (1) This regulator must have the same voltage as VIO if S_IO LDO is used to
> + *     power a sensor/VCM which I2C is daisy chained behind the PMIC.
> + * (2) If there is no I2C daisy chain it can be set freely.
> + */

Do we need safety checks for this ?

> +	TPS68470_REGULATOR(VSIO, TPS68470_VSIO, tps68470_regulator_ops, 126,
> +			   TPS68470_REG_VSIOVAL, TPS68470_VSIOVAL_IOVOLT_MASK,
> +			   TPS68470_REG_S_I2C_CTL, TPS68470_S_I2C_CTL_EN_MASK,
> +			   tps68470_ldo_ranges, ARRAY_SIZE(tps68470_ldo_ranges)),
> +	TPS68470_REGULATOR(AUX1, TPS68470_AUX1, tps68470_regulator_ops, 126,
> +			   TPS68470_REG_VAUX1VAL, TPS68470_VAUX1VAL_AUX1VOLT_MASK,
> +			   TPS68470_REG_VAUX1CTL, TPS68470_VAUX1CTL_EN_MASK,
> +			   tps68470_ldo_ranges, ARRAY_SIZE(tps68470_ldo_ranges)),
> +	TPS68470_REGULATOR(AUX2, TPS68470_AUX2, tps68470_regulator_ops, 126,
> +			   TPS68470_REG_VAUX2VAL, TPS68470_VAUX2VAL_AUX2VOLT_MASK,
> +			   TPS68470_REG_VAUX2CTL, TPS68470_VAUX2CTL_EN_MASK,
> +			   tps68470_ldo_ranges, ARRAY_SIZE(tps68470_ldo_ranges)),
> +};
> +
> +static int tps68470_regulator_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct tps68470_regulator_platform_data *pdata = dev_get_platdata(dev);
> +	struct tps68470_regulator_data *data;
> +	struct regulator_config config = { };
> +	struct regulator_dev *rdev;
> +	int i;
> +
> +	data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
> +	if (!data)
> +		return -ENOMEM;
> +
> +	data->clk = devm_clk_get(dev, "tps68470-clk");
> +	if (IS_ERR(data->clk))
> +		return dev_err_probe(dev, PTR_ERR(data->clk), "getting tps68470-clk\n");
> +
> +	config.dev = dev->parent;
> +	config.regmap = dev_get_drvdata(dev->parent);
> +	config.driver_data = data;
> +
> +	for (i = 0; i < TPS68470_NUM_REGULATORS; i++) {
> +		if (pdata)
> +			config.init_data = pdata->reg_init_data[i];
> +		else
> +			config.init_data = NULL;
> +
> +		rdev = devm_regulator_register(dev, &regulators[i], &config);
> +		if (IS_ERR(rdev))
> +			return dev_err_probe(dev, PTR_ERR(data->clk),

This should be PTR_ERR(rdev).

> +					     "registering %s regulator\n",
> +					     regulators[i].name);
> +	}
> +
> +	return 0;
> +}
> +
> +static struct platform_driver tps68470_regulator_driver = {
> +	.driver = {
> +		.name = "tps68470-regulator",
> +	},
> +	.probe = tps68470_regulator_probe,
> +};
> +
> +/*
> + * The ACPI tps68470 probe-ordering depends on the clk/gpio/regulator drivers
> + * registering before the drivers for the camera-sensors which use them bind.
> + * subsys_initcall() ensures this when the drivers are builtin.
> + */
> +static int __init tps68470_regulator_init(void)
> +{
> +	return platform_driver_register(&tps68470_regulator_driver);
> +}
> +subsys_initcall(tps68470_regulator_init);
> +
> +static void __exit tps68470_regulator_exit(void)
> +{
> +	platform_driver_unregister(&tps68470_regulator_driver);
> +}
> +module_exit(tps68470_regulator_exit);
> +
> +MODULE_ALIAS("platform:tps68470-regulator");
> +MODULE_DESCRIPTION("TPS68470 voltage regulator driver");
> +MODULE_LICENSE("GPL v2");

-- 
Regards,

Laurent Pinchart

Re: [PATCH v6 05/15] regulator: Introduce tps68470-regulator driver

[Hans de Goede]

Hi Laurent,

On 11/26/21 00:32, Laurent Pinchart wrote:
> Hi Hans,
> 
> Thank you for the patch.
> 
> I've had a quick look and the driver seems fine. Just a few comments
> below.
> 
> On Thu, Nov 25, 2021 at 05:54:02PM +0100, Hans de Goede wrote:
>> The TPS68470 PMIC provides Clocks, GPIOs and Regulators. At present in
>> the kernel the Regulators and Clocks are controlled by an OpRegion
>> driver designed to work with power control methods defined in ACPI, but
>> some platforms lack those methods, meaning drivers need to be able to
>> consume the resources of these chips through the usual frameworks.
>>
>> This commit adds a driver for the regulators provided by the tps68470,
>> and is designed to bind to the platform_device registered by the
>> intel_skl_int3472 module.
>>
>> This is based on this out of tree driver written by Intel:
>> https://github.com/intel/linux-intel-lts/blob/4.14/base/drivers/regulator/tps68470-regulator.c
>> with various cleanups added.
>>
>> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>> Changes in v6:
>> - Drop the unused volt_table argument from the TPS68470_REGULATOR() macro
>> - While working on VCM (voice coil motor) support for the camera-module behind
>>   this PMIC I learned that the VIO voltage is always on. Instead of pointing its
>>   enable_reg and enable_mask at the same register-bits as the VSIO regulator
>>   (which is wrong), add a new tps68470_always_on_reg_ops struct without
>>   is_enabled, enable and disable ops and use that for the VIO regulator.
>>
>> Changes in v5:
>> - Small comment / code cleanups based on review from Andy
>>
>> Changes in v4:
>> - Make the top comment block use c++ style comments
>> - Drop the bogus builtin regulator_init_data
>> - Add || COMPILE_TEST to Kconfig snippet
>> - Make the driver enable the PMIC clk when enabling the Core buck
>>   regulator, this switching regulator needs the PLL to be on
>>
>> Changes in v2:
>> - Update the comment on why a subsys_initcall is used to register the drv
>> - Make struct regulator_ops const
>> ---
>>  drivers/regulator/Kconfig              |   9 ++
>>  drivers/regulator/Makefile             |   1 +
>>  drivers/regulator/tps68470-regulator.c | 201 +++++++++++++++++++++++++
>>  3 files changed, 211 insertions(+)
>>  create mode 100644 drivers/regulator/tps68470-regulator.c
>>
>> diff --git a/drivers/regulator/Kconfig b/drivers/regulator/Kconfig
>> index 6be9b1c8a615..ebe46e09510e 100644
>> --- a/drivers/regulator/Kconfig
>> +++ b/drivers/regulator/Kconfig
>> @@ -1339,6 +1339,15 @@ config REGULATOR_TPS65912
>>  	help
>>  	    This driver supports TPS65912 voltage regulator chip.
>>  
>> +config REGULATOR_TPS68470
>> +	tristate "TI TPS68470 PMIC Regulators Driver"
>> +	depends on INTEL_SKL_INT3472 || COMPILE_TEST
>> +	help
>> +	  This driver adds support for the TPS68470 PMIC to register
>> +	  regulators against the usual framework.
>> +
>> +	  The module will be called "tps68470-regulator".
>> +
>>  config REGULATOR_TWL4030
>>  	tristate "TI TWL4030/TWL5030/TWL6030/TPS659x0 PMIC"
>>  	depends on TWL4030_CORE
>> diff --git a/drivers/regulator/Makefile b/drivers/regulator/Makefile
>> index b07d2a22df0b..257331d2caed 100644
>> --- a/drivers/regulator/Makefile
>> +++ b/drivers/regulator/Makefile
>> @@ -159,6 +159,7 @@ obj-$(CONFIG_REGULATOR_TPS6586X) += tps6586x-regulator.o
>>  obj-$(CONFIG_REGULATOR_TPS65910) += tps65910-regulator.o
>>  obj-$(CONFIG_REGULATOR_TPS65912) += tps65912-regulator.o
>>  obj-$(CONFIG_REGULATOR_TPS65132) += tps65132-regulator.o
>> +obj-$(CONFIG_REGULATOR_TPS68470) += tps68470-regulator.o
>>  obj-$(CONFIG_REGULATOR_TWL4030) += twl-regulator.o twl6030-regulator.o
>>  obj-$(CONFIG_REGULATOR_UNIPHIER) += uniphier-regulator.o
>>  obj-$(CONFIG_REGULATOR_VCTRL) += vctrl-regulator.o
>> diff --git a/drivers/regulator/tps68470-regulator.c b/drivers/regulator/tps68470-regulator.c
>> new file mode 100644
>> index 000000000000..9ad2d1eae8fe
>> --- /dev/null
>> +++ b/drivers/regulator/tps68470-regulator.c
>> @@ -0,0 +1,201 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +//
>> +// Regulator driver for TPS68470 PMIC
>> +//
>> +// Copyright (c) 2021 Red Hat Inc.
>> +// Copyright (C) 2018 Intel Corporation
>> +//
>> +// Authors:
>> +//	Hans de Goede <hdegoede@redhat.com>
>> +//	Zaikuo Wang <zaikuo.wang@intel.com>
>> +//	Tianshu Qiu <tian.shu.qiu@intel.com>
>> +//	Jian Xu Zheng <jian.xu.zheng@intel.com>
>> +//	Yuning Pu <yuning.pu@intel.com>
>> +//	Rajmohan Mani <rajmohan.mani@intel.com>
>> +
>> +#include <linux/clk.h>
>> +#include <linux/device.h>
>> +#include <linux/err.h>
>> +#include <linux/init.h>
>> +#include <linux/kernel.h>
>> +#include <linux/mfd/tps68470.h>
>> +#include <linux/module.h>
>> +#include <linux/platform_data/tps68470.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/regulator/driver.h>
>> +#include <linux/regulator/machine.h>
>> +
>> +struct tps68470_regulator_data {
>> +	struct clk *clk;
>> +};
>> +
>> +#define TPS68470_REGULATOR(_name, _id, _ops, _n,			\
>> +			   _vr, _vm, _er, _em, _lr, _nlr)		\
>> +	[TPS68470_ ## _name] = {					\
>> +		.name			= # _name,			\
>> +		.id			= _id,				\
>> +		.ops			= &_ops,			\
>> +		.n_voltages		= _n,				\
>> +		.type			= REGULATOR_VOLTAGE,		\
>> +		.owner			= THIS_MODULE,			\
>> +		.vsel_reg		= _vr,				\
>> +		.vsel_mask		= _vm,				\
>> +		.enable_reg		= _er,				\
>> +		.enable_mask		= _em,				\
>> +		.linear_ranges		= _lr,				\
>> +		.n_linear_ranges	= _nlr,				\
>> +	}
>> +
>> +static const struct linear_range tps68470_ldo_ranges[] = {
>> +	REGULATOR_LINEAR_RANGE(875000, 0, 125, 17800),
>> +};
>> +
>> +static const struct linear_range tps68470_core_ranges[] = {
>> +	REGULATOR_LINEAR_RANGE(900000, 0, 42, 25000),
>> +};
>> +
>> +static int tps68470_regulator_enable(struct regulator_dev *rdev)
>> +{
>> +	struct tps68470_regulator_data *data = rdev->reg_data;
>> +	int ret;
>> +
>> +	/* The Core buck regulator needs the PMIC's PLL to be enabled */
>> +	if (rdev->desc->id == TPS68470_CORE) {
>> +		ret = clk_prepare_enable(data->clk);
>> +		if (ret) {
>> +			dev_err(&rdev->dev, "Error enabling TPS68470 clock\n");
>> +			return ret;
>> +		}
>> +	}
>> +
>> +	return regulator_enable_regmap(rdev);
>> +}
>> +
>> +static int tps68470_regulator_disable(struct regulator_dev *rdev)
>> +{
>> +	struct tps68470_regulator_data *data = rdev->reg_data;
>> +
>> +	if (rdev->desc->id == TPS68470_CORE)
>> +		clk_disable_unprepare(data->clk);
>> +
>> +	return regulator_disable_regmap(rdev);
>> +}
>> +
>> +/* Operations permitted on DCDCx, LDO2, LDO3 and LDO4 */
>> +static const struct regulator_ops tps68470_regulator_ops = {
>> +	.is_enabled		= regulator_is_enabled_regmap,
>> +	.enable			= tps68470_regulator_enable,
>> +	.disable		= tps68470_regulator_disable,
>> +	.get_voltage_sel	= regulator_get_voltage_sel_regmap,
>> +	.set_voltage_sel	= regulator_set_voltage_sel_regmap,
>> +	.list_voltage		= regulator_list_voltage_linear_range,
>> +	.map_voltage		= regulator_map_voltage_linear_range,
>> +};
>> +
>> +static const struct regulator_ops tps68470_always_on_reg_ops = {
>> +	.get_voltage_sel	= regulator_get_voltage_sel_regmap,
>> +	.set_voltage_sel	= regulator_set_voltage_sel_regmap,
>> +	.list_voltage		= regulator_list_voltage_linear_range,
>> +	.map_voltage		= regulator_map_voltage_linear_range,
>> +};
>> +
>> +static const struct regulator_desc regulators[] = {
>> +	TPS68470_REGULATOR(CORE, TPS68470_CORE, tps68470_regulator_ops, 43,
>> +			   TPS68470_REG_VDVAL, TPS68470_VDVAL_DVOLT_MASK,
>> +			   TPS68470_REG_VDCTL, TPS68470_VDCTL_EN_MASK,
>> +			   tps68470_core_ranges, ARRAY_SIZE(tps68470_core_ranges)),
>> +	TPS68470_REGULATOR(ANA, TPS68470_ANA, tps68470_regulator_ops, 126,
>> +			   TPS68470_REG_VAVAL, TPS68470_VAVAL_AVOLT_MASK,
>> +			   TPS68470_REG_VACTL, TPS68470_VACTL_EN_MASK,
>> +			   tps68470_ldo_ranges, ARRAY_SIZE(tps68470_ldo_ranges)),
>> +	TPS68470_REGULATOR(VCM, TPS68470_VCM, tps68470_regulator_ops, 126,
>> +			   TPS68470_REG_VCMVAL, TPS68470_VCMVAL_VCVOLT_MASK,
>> +			   TPS68470_REG_VCMCTL, TPS68470_VCMCTL_EN_MASK,
>> +			   tps68470_ldo_ranges, ARRAY_SIZE(tps68470_ldo_ranges)),
>> +	TPS68470_REGULATOR(VIO, TPS68470_VIO, tps68470_always_on_reg_ops, 126,
>> +			   TPS68470_REG_VIOVAL, TPS68470_VIOVAL_IOVOLT_MASK,
>> +			   0, 0,
>> +			   tps68470_ldo_ranges, ARRAY_SIZE(tps68470_ldo_ranges)),
>> +/*
>> + * (1) This regulator must have the same voltage as VIO if S_IO LDO is used to
>> + *     power a sensor/VCM which I2C is daisy chained behind the PMIC.
>> + * (2) If there is no I2C daisy chain it can be set freely.
>> + */
> 
> Do we need safety checks for this ?

There really is no way to deal this condition needs to matches inside the driver,
this should be enforced by setting proper constraints on the 2 regulators where
the PMIC is used with a sensor I2C daisy chained behind it.

> 
>> +	TPS68470_REGULATOR(VSIO, TPS68470_VSIO, tps68470_regulator_ops, 126,
>> +			   TPS68470_REG_VSIOVAL, TPS68470_VSIOVAL_IOVOLT_MASK,
>> +			   TPS68470_REG_S_I2C_CTL, TPS68470_S_I2C_CTL_EN_MASK,
>> +			   tps68470_ldo_ranges, ARRAY_SIZE(tps68470_ldo_ranges)),
>> +	TPS68470_REGULATOR(AUX1, TPS68470_AUX1, tps68470_regulator_ops, 126,
>> +			   TPS68470_REG_VAUX1VAL, TPS68470_VAUX1VAL_AUX1VOLT_MASK,
>> +			   TPS68470_REG_VAUX1CTL, TPS68470_VAUX1CTL_EN_MASK,
>> +			   tps68470_ldo_ranges, ARRAY_SIZE(tps68470_ldo_ranges)),
>> +	TPS68470_REGULATOR(AUX2, TPS68470_AUX2, tps68470_regulator_ops, 126,
>> +			   TPS68470_REG_VAUX2VAL, TPS68470_VAUX2VAL_AUX2VOLT_MASK,
>> +			   TPS68470_REG_VAUX2CTL, TPS68470_VAUX2CTL_EN_MASK,
>> +			   tps68470_ldo_ranges, ARRAY_SIZE(tps68470_ldo_ranges)),
>> +};
>> +
>> +static int tps68470_regulator_probe(struct platform_device *pdev)
>> +{
>> +	struct device *dev = &pdev->dev;
>> +	struct tps68470_regulator_platform_data *pdata = dev_get_platdata(dev);
>> +	struct tps68470_regulator_data *data;
>> +	struct regulator_config config = { };
>> +	struct regulator_dev *rdev;
>> +	int i;
>> +
>> +	data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
>> +	if (!data)
>> +		return -ENOMEM;
>> +
>> +	data->clk = devm_clk_get(dev, "tps68470-clk");
>> +	if (IS_ERR(data->clk))
>> +		return dev_err_probe(dev, PTR_ERR(data->clk), "getting tps68470-clk\n");
>> +
>> +	config.dev = dev->parent;
>> +	config.regmap = dev_get_drvdata(dev->parent);
>> +	config.driver_data = data;
>> +
>> +	for (i = 0; i < TPS68470_NUM_REGULATORS; i++) {
>> +		if (pdata)
>> +			config.init_data = pdata->reg_init_data[i];
>> +		else
>> +			config.init_data = NULL;
>> +
>> +		rdev = devm_regulator_register(dev, &regulators[i], &config);
>> +		if (IS_ERR(rdev))
>> +			return dev_err_probe(dev, PTR_ERR(data->clk),
> 
> This should be PTR_ERR(rdev).

Good catch, thanks. Fixed for v7.

Regards,

Hans


> 
>> +					     "registering %s regulator\n",
>> +					     regulators[i].name);
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static struct platform_driver tps68470_regulator_driver = {
>> +	.driver = {
>> +		.name = "tps68470-regulator",
>> +	},
>> +	.probe = tps68470_regulator_probe,
>> +};
>> +
>> +/*
>> + * The ACPI tps68470 probe-ordering depends on the clk/gpio/regulator drivers
>> + * registering before the drivers for the camera-sensors which use them bind.
>> + * subsys_initcall() ensures this when the drivers are builtin.
>> + */
>> +static int __init tps68470_regulator_init(void)
>> +{
>> +	return platform_driver_register(&tps68470_regulator_driver);
>> +}
>> +subsys_initcall(tps68470_regulator_init);
>> +
>> +static void __exit tps68470_regulator_exit(void)
>> +{
>> +	platform_driver_unregister(&tps68470_regulator_driver);
>> +}
>> +module_exit(tps68470_regulator_exit);
>> +
>> +MODULE_ALIAS("platform:tps68470-regulator");
>> +MODULE_DESCRIPTION("TPS68470 voltage regulator driver");
>> +MODULE_LICENSE("GPL v2");
> 

Re: [PATCH v6 05/15] regulator: Introduce tps68470-regulator driver

[Laurent Pinchart]

Hi Hans,

On Fri, Nov 26, 2021 at 12:22:35PM +0100, Hans de Goede wrote:
> On 11/26/21 00:32, Laurent Pinchart wrote:
> > On Thu, Nov 25, 2021 at 05:54:02PM +0100, Hans de Goede wrote:
> >> The TPS68470 PMIC provides Clocks, GPIOs and Regulators. At present in
> >> the kernel the Regulators and Clocks are controlled by an OpRegion
> >> driver designed to work with power control methods defined in ACPI, but
> >> some platforms lack those methods, meaning drivers need to be able to
> >> consume the resources of these chips through the usual frameworks.
> >>
> >> This commit adds a driver for the regulators provided by the tps68470,
> >> and is designed to bind to the platform_device registered by the
> >> intel_skl_int3472 module.
> >>
> >> This is based on this out of tree driver written by Intel:
> >> https://github.com/intel/linux-intel-lts/blob/4.14/base/drivers/regulator/tps68470-regulator.c
> >> with various cleanups added.
> >>
> >> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> >> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> >> ---
> >> Changes in v6:
> >> - Drop the unused volt_table argument from the TPS68470_REGULATOR() macro
> >> - While working on VCM (voice coil motor) support for the camera-module behind
> >>   this PMIC I learned that the VIO voltage is always on. Instead of pointing its
> >>   enable_reg and enable_mask at the same register-bits as the VSIO regulator
> >>   (which is wrong), add a new tps68470_always_on_reg_ops struct without
> >>   is_enabled, enable and disable ops and use that for the VIO regulator.
> >>
> >> Changes in v5:
> >> - Small comment / code cleanups based on review from Andy
> >>
> >> Changes in v4:
> >> - Make the top comment block use c++ style comments
> >> - Drop the bogus builtin regulator_init_data
> >> - Add || COMPILE_TEST to Kconfig snippet
> >> - Make the driver enable the PMIC clk when enabling the Core buck
> >>   regulator, this switching regulator needs the PLL to be on
> >>
> >> Changes in v2:
> >> - Update the comment on why a subsys_initcall is used to register the drv
> >> - Make struct regulator_ops const
> >> ---
> >>  drivers/regulator/Kconfig              |   9 ++
> >>  drivers/regulator/Makefile             |   1 +
> >>  drivers/regulator/tps68470-regulator.c | 201 +++++++++++++++++++++++++
> >>  3 files changed, 211 insertions(+)
> >>  create mode 100644 drivers/regulator/tps68470-regulator.c
> >>
> >> diff --git a/drivers/regulator/Kconfig b/drivers/regulator/Kconfig
> >> index 6be9b1c8a615..ebe46e09510e 100644
> >> --- a/drivers/regulator/Kconfig
> >> +++ b/drivers/regulator/Kconfig
> >> @@ -1339,6 +1339,15 @@ config REGULATOR_TPS65912
> >>  	help
> >>  	    This driver supports TPS65912 voltage regulator chip.
> >>  
> >> +config REGULATOR_TPS68470
> >> +	tristate "TI TPS68470 PMIC Regulators Driver"
> >> +	depends on INTEL_SKL_INT3472 || COMPILE_TEST
> >> +	help
> >> +	  This driver adds support for the TPS68470 PMIC to register
> >> +	  regulators against the usual framework.
> >> +
> >> +	  The module will be called "tps68470-regulator".
> >> +
> >>  config REGULATOR_TWL4030
> >>  	tristate "TI TWL4030/TWL5030/TWL6030/TPS659x0 PMIC"
> >>  	depends on TWL4030_CORE
> >> diff --git a/drivers/regulator/Makefile b/drivers/regulator/Makefile
> >> index b07d2a22df0b..257331d2caed 100644
> >> --- a/drivers/regulator/Makefile
> >> +++ b/drivers/regulator/Makefile
> >> @@ -159,6 +159,7 @@ obj-$(CONFIG_REGULATOR_TPS6586X) += tps6586x-regulator.o
> >>  obj-$(CONFIG_REGULATOR_TPS65910) += tps65910-regulator.o
> >>  obj-$(CONFIG_REGULATOR_TPS65912) += tps65912-regulator.o
> >>  obj-$(CONFIG_REGULATOR_TPS65132) += tps65132-regulator.o
> >> +obj-$(CONFIG_REGULATOR_TPS68470) += tps68470-regulator.o
> >>  obj-$(CONFIG_REGULATOR_TWL4030) += twl-regulator.o twl6030-regulator.o
> >>  obj-$(CONFIG_REGULATOR_UNIPHIER) += uniphier-regulator.o
> >>  obj-$(CONFIG_REGULATOR_VCTRL) += vctrl-regulator.o
> >> diff --git a/drivers/regulator/tps68470-regulator.c b/drivers/regulator/tps68470-regulator.c
> >> new file mode 100644
> >> index 000000000000..9ad2d1eae8fe
> >> --- /dev/null
> >> +++ b/drivers/regulator/tps68470-regulator.c
> >> @@ -0,0 +1,201 @@
> >> +// SPDX-License-Identifier: GPL-2.0
> >> +//
> >> +// Regulator driver for TPS68470 PMIC
> >> +//
> >> +// Copyright (c) 2021 Red Hat Inc.
> >> +// Copyright (C) 2018 Intel Corporation
> >> +//
> >> +// Authors:
> >> +//	Hans de Goede <hdegoede@redhat.com>
> >> +//	Zaikuo Wang <zaikuo.wang@intel.com>
> >> +//	Tianshu Qiu <tian.shu.qiu@intel.com>
> >> +//	Jian Xu Zheng <jian.xu.zheng@intel.com>
> >> +//	Yuning Pu <yuning.pu@intel.com>
> >> +//	Rajmohan Mani <rajmohan.mani@intel.com>
> >> +
> >> +#include <linux/clk.h>
> >> +#include <linux/device.h>
> >> +#include <linux/err.h>
> >> +#include <linux/init.h>
> >> +#include <linux/kernel.h>
> >> +#include <linux/mfd/tps68470.h>
> >> +#include <linux/module.h>
> >> +#include <linux/platform_data/tps68470.h>
> >> +#include <linux/platform_device.h>
> >> +#include <linux/regulator/driver.h>
> >> +#include <linux/regulator/machine.h>
> >> +
> >> +struct tps68470_regulator_data {
> >> +	struct clk *clk;
> >> +};
> >> +
> >> +#define TPS68470_REGULATOR(_name, _id, _ops, _n,			\
> >> +			   _vr, _vm, _er, _em, _lr, _nlr)		\
> >> +	[TPS68470_ ## _name] = {					\
> >> +		.name			= # _name,			\
> >> +		.id			= _id,				\
> >> +		.ops			= &_ops,			\
> >> +		.n_voltages		= _n,				\
> >> +		.type			= REGULATOR_VOLTAGE,		\
> >> +		.owner			= THIS_MODULE,			\
> >> +		.vsel_reg		= _vr,				\
> >> +		.vsel_mask		= _vm,				\
> >> +		.enable_reg		= _er,				\
> >> +		.enable_mask		= _em,				\
> >> +		.linear_ranges		= _lr,				\
> >> +		.n_linear_ranges	= _nlr,				\
> >> +	}
> >> +
> >> +static const struct linear_range tps68470_ldo_ranges[] = {
> >> +	REGULATOR_LINEAR_RANGE(875000, 0, 125, 17800),
> >> +};
> >> +
> >> +static const struct linear_range tps68470_core_ranges[] = {
> >> +	REGULATOR_LINEAR_RANGE(900000, 0, 42, 25000),
> >> +};
> >> +
> >> +static int tps68470_regulator_enable(struct regulator_dev *rdev)
> >> +{
> >> +	struct tps68470_regulator_data *data = rdev->reg_data;
> >> +	int ret;
> >> +
> >> +	/* The Core buck regulator needs the PMIC's PLL to be enabled */
> >> +	if (rdev->desc->id == TPS68470_CORE) {
> >> +		ret = clk_prepare_enable(data->clk);
> >> +		if (ret) {
> >> +			dev_err(&rdev->dev, "Error enabling TPS68470 clock\n");
> >> +			return ret;
> >> +		}
> >> +	}
> >> +
> >> +	return regulator_enable_regmap(rdev);
> >> +}
> >> +
> >> +static int tps68470_regulator_disable(struct regulator_dev *rdev)
> >> +{
> >> +	struct tps68470_regulator_data *data = rdev->reg_data;
> >> +
> >> +	if (rdev->desc->id == TPS68470_CORE)
> >> +		clk_disable_unprepare(data->clk);
> >> +
> >> +	return regulator_disable_regmap(rdev);
> >> +}
> >> +
> >> +/* Operations permitted on DCDCx, LDO2, LDO3 and LDO4 */
> >> +static const struct regulator_ops tps68470_regulator_ops = {
> >> +	.is_enabled		= regulator_is_enabled_regmap,
> >> +	.enable			= tps68470_regulator_enable,
> >> +	.disable		= tps68470_regulator_disable,
> >> +	.get_voltage_sel	= regulator_get_voltage_sel_regmap,
> >> +	.set_voltage_sel	= regulator_set_voltage_sel_regmap,
> >> +	.list_voltage		= regulator_list_voltage_linear_range,
> >> +	.map_voltage		= regulator_map_voltage_linear_range,
> >> +};
> >> +
> >> +static const struct regulator_ops tps68470_always_on_reg_ops = {
> >> +	.get_voltage_sel	= regulator_get_voltage_sel_regmap,
> >> +	.set_voltage_sel	= regulator_set_voltage_sel_regmap,
> >> +	.list_voltage		= regulator_list_voltage_linear_range,
> >> +	.map_voltage		= regulator_map_voltage_linear_range,
> >> +};
> >> +
> >> +static const struct regulator_desc regulators[] = {
> >> +	TPS68470_REGULATOR(CORE, TPS68470_CORE, tps68470_regulator_ops, 43,
> >> +			   TPS68470_REG_VDVAL, TPS68470_VDVAL_DVOLT_MASK,
> >> +			   TPS68470_REG_VDCTL, TPS68470_VDCTL_EN_MASK,
> >> +			   tps68470_core_ranges, ARRAY_SIZE(tps68470_core_ranges)),
> >> +	TPS68470_REGULATOR(ANA, TPS68470_ANA, tps68470_regulator_ops, 126,
> >> +			   TPS68470_REG_VAVAL, TPS68470_VAVAL_AVOLT_MASK,
> >> +			   TPS68470_REG_VACTL, TPS68470_VACTL_EN_MASK,
> >> +			   tps68470_ldo_ranges, ARRAY_SIZE(tps68470_ldo_ranges)),
> >> +	TPS68470_REGULATOR(VCM, TPS68470_VCM, tps68470_regulator_ops, 126,
> >> +			   TPS68470_REG_VCMVAL, TPS68470_VCMVAL_VCVOLT_MASK,
> >> +			   TPS68470_REG_VCMCTL, TPS68470_VCMCTL_EN_MASK,
> >> +			   tps68470_ldo_ranges, ARRAY_SIZE(tps68470_ldo_ranges)),
> >> +	TPS68470_REGULATOR(VIO, TPS68470_VIO, tps68470_always_on_reg_ops, 126,
> >> +			   TPS68470_REG_VIOVAL, TPS68470_VIOVAL_IOVOLT_MASK,
> >> +			   0, 0,
> >> +			   tps68470_ldo_ranges, ARRAY_SIZE(tps68470_ldo_ranges)),
> >> +/*
> >> + * (1) This regulator must have the same voltage as VIO if S_IO LDO is used to
> >> + *     power a sensor/VCM which I2C is daisy chained behind the PMIC.
> >> + * (2) If there is no I2C daisy chain it can be set freely.
> >> + */
> > 
> > Do we need safety checks for this ?
> 
> There really is no way to deal this condition needs to matches inside the driver,
> this should be enforced by setting proper constraints on the 2 regulators where
> the PMIC is used with a sensor I2C daisy chained behind it.

Right. I tend to be cautious here, as incorrect settings can destroy the
hardware. We should err on the side of too many safety checks rather
than too few. I was thinking that the cio2-bridge driver could set a
daisy-chaining flag, which could trigger additional checks here, but it
wouldn't protect against someone experimenting to support a new device
and setting different voltages without the daisy-chaining flag.

My biggest worry is that someone with an unsupported machine may start
by copying and pasting an existing configuration to try it out, and fry
their hardware.

> >> +	TPS68470_REGULATOR(VSIO, TPS68470_VSIO, tps68470_regulator_ops, 126,
> >> +			   TPS68470_REG_VSIOVAL, TPS68470_VSIOVAL_IOVOLT_MASK,
> >> +			   TPS68470_REG_S_I2C_CTL, TPS68470_S_I2C_CTL_EN_MASK,
> >> +			   tps68470_ldo_ranges, ARRAY_SIZE(tps68470_ldo_ranges)),
> >> +	TPS68470_REGULATOR(AUX1, TPS68470_AUX1, tps68470_regulator_ops, 126,
> >> +			   TPS68470_REG_VAUX1VAL, TPS68470_VAUX1VAL_AUX1VOLT_MASK,
> >> +			   TPS68470_REG_VAUX1CTL, TPS68470_VAUX1CTL_EN_MASK,
> >> +			   tps68470_ldo_ranges, ARRAY_SIZE(tps68470_ldo_ranges)),
> >> +	TPS68470_REGULATOR(AUX2, TPS68470_AUX2, tps68470_regulator_ops, 126,
> >> +			   TPS68470_REG_VAUX2VAL, TPS68470_VAUX2VAL_AUX2VOLT_MASK,
> >> +			   TPS68470_REG_VAUX2CTL, TPS68470_VAUX2CTL_EN_MASK,
> >> +			   tps68470_ldo_ranges, ARRAY_SIZE(tps68470_ldo_ranges)),
> >> +};
> >> +
> >> +static int tps68470_regulator_probe(struct platform_device *pdev)
> >> +{
> >> +	struct device *dev = &pdev->dev;
> >> +	struct tps68470_regulator_platform_data *pdata = dev_get_platdata(dev);
> >> +	struct tps68470_regulator_data *data;
> >> +	struct regulator_config config = { };
> >> +	struct regulator_dev *rdev;
> >> +	int i;
> >> +
> >> +	data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
> >> +	if (!data)
> >> +		return -ENOMEM;
> >> +
> >> +	data->clk = devm_clk_get(dev, "tps68470-clk");
> >> +	if (IS_ERR(data->clk))
> >> +		return dev_err_probe(dev, PTR_ERR(data->clk), "getting tps68470-clk\n");
> >> +
> >> +	config.dev = dev->parent;
> >> +	config.regmap = dev_get_drvdata(dev->parent);
> >> +	config.driver_data = data;
> >> +
> >> +	for (i = 0; i < TPS68470_NUM_REGULATORS; i++) {
> >> +		if (pdata)
> >> +			config.init_data = pdata->reg_init_data[i];
> >> +		else
> >> +			config.init_data = NULL;
> >> +
> >> +		rdev = devm_regulator_register(dev, &regulators[i], &config);
> >> +		if (IS_ERR(rdev))
> >> +			return dev_err_probe(dev, PTR_ERR(data->clk),
> > 
> > This should be PTR_ERR(rdev).
> 
> Good catch, thanks. Fixed for v7.
> 
> >> +					     "registering %s regulator\n",
> >> +					     regulators[i].name);
> >> +	}
> >> +
> >> +	return 0;
> >> +}
> >> +
> >> +static struct platform_driver tps68470_regulator_driver = {
> >> +	.driver = {
> >> +		.name = "tps68470-regulator",
> >> +	},
> >> +	.probe = tps68470_regulator_probe,
> >> +};
> >> +
> >> +/*
> >> + * The ACPI tps68470 probe-ordering depends on the clk/gpio/regulator drivers
> >> + * registering before the drivers for the camera-sensors which use them bind.
> >> + * subsys_initcall() ensures this when the drivers are builtin.
> >> + */
> >> +static int __init tps68470_regulator_init(void)
> >> +{
> >> +	return platform_driver_register(&tps68470_regulator_driver);
> >> +}
> >> +subsys_initcall(tps68470_regulator_init);
> >> +
> >> +static void __exit tps68470_regulator_exit(void)
> >> +{
> >> +	platform_driver_unregister(&tps68470_regulator_driver);
> >> +}
> >> +module_exit(tps68470_regulator_exit);
> >> +
> >> +MODULE_ALIAS("platform:tps68470-regulator");
> >> +MODULE_DESCRIPTION("TPS68470 voltage regulator driver");
> >> +MODULE_LICENSE("GPL v2");

-- 
Regards,

Laurent Pinchart

Re: [PATCH v6 05/15] regulator: Introduce tps68470-regulator driver

[Mark Brown]

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

On Sun, Nov 28, 2021 at 01:38:34AM +0200, Laurent Pinchart wrote:
> On Fri, Nov 26, 2021 at 12:22:35PM +0100, Hans de Goede wrote:
> > On 11/26/21 00:32, Laurent Pinchart wrote:
> > > On Thu, Nov 25, 2021 at 05:54:02PM +0100, Hans de Goede wrote:
> > >> The TPS68470 PMIC provides Clocks, GPIOs and Regulators. At present in
> > >> the kernel the Regulators and Clocks are controlled by an OpRegion
> > >> driver designed to work with power control methods defined in ACPI, but

Please delete unneeded context from mails when replying.  Doing this
makes it much easier to find your reply in the message, helping ensure
it won't be missed by people scrolling through the irrelevant quoted
material.

> > >> + * (1) This regulator must have the same voltage as VIO if S_IO LDO is used to
> > >> + *     power a sensor/VCM which I2C is daisy chained behind the PMIC.
> > >> + * (2) If there is no I2C daisy chain it can be set freely.
> > >> + */

> > > Do we need safety checks for this ?

> > There really is no way to deal this condition needs to matches inside the driver,
> > this should be enforced by setting proper constraints on the 2 regulators where
> > the PMIC is used with a sensor I2C daisy chained behind it.

> Right. I tend to be cautious here, as incorrect settings can destroy the
> hardware. We should err on the side of too many safety checks rather
> than too few. I was thinking that the cio2-bridge driver could set a
> daisy-chaining flag, which could trigger additional checks here, but it
> wouldn't protect against someone experimenting to support a new device
> and setting different voltages without the daisy-chaining flag.

> My biggest worry is that someone with an unsupported machine may start
> by copying and pasting an existing configuration to try it out, and fry
> their hardware.

There's really nothing you can do that prevents this, especially in the
cut'n'paste scenario.  Overrides tend to get copied along with the rest
of the configuration, or checks hacked out if people think they're
getting in the way without realising what they're there for.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v6 05/15] regulator: Introduce tps68470-regulator driver

[Laurent Pinchart]

Hi Mark,

On Mon, Nov 29, 2021 at 12:08:06PM +0000, Mark Brown wrote:
> On Sun, Nov 28, 2021 at 01:38:34AM +0200, Laurent Pinchart wrote:
> > On Fri, Nov 26, 2021 at 12:22:35PM +0100, Hans de Goede wrote:
> > > On 11/26/21 00:32, Laurent Pinchart wrote:
> > > > On Thu, Nov 25, 2021 at 05:54:02PM +0100, Hans de Goede wrote:
> > > >> The TPS68470 PMIC provides Clocks, GPIOs and Regulators. At present in
> > > >> the kernel the Regulators and Clocks are controlled by an OpRegion
> > > >> driver designed to work with power control methods defined in ACPI, but
> 
> Please delete unneeded context from mails when replying.  Doing this
> makes it much easier to find your reply in the message, helping ensure
> it won't be missed by people scrolling through the irrelevant quoted
> material.

I have mixed feelings about that, someones the context is indeed not
needed, but I've found myself more often than not replying deep in a
mail thread and wishing the context hadn't been deleted, because it
ended up being relevant.

> > > >> + * (1) This regulator must have the same voltage as VIO if S_IO LDO is used to
> > > >> + *     power a sensor/VCM which I2C is daisy chained behind the PMIC.
> > > >> + * (2) If there is no I2C daisy chain it can be set freely.
> > > >> + */
> 
> > > > Do we need safety checks for this ?
> 
> > > There really is no way to deal this condition needs to matches inside the driver,
> > > this should be enforced by setting proper constraints on the 2 regulators where
> > > the PMIC is used with a sensor I2C daisy chained behind it.
> 
> > Right. I tend to be cautious here, as incorrect settings can destroy the
> > hardware. We should err on the side of too many safety checks rather
> > than too few. I was thinking that the cio2-bridge driver could set a
> > daisy-chaining flag, which could trigger additional checks here, but it
> > wouldn't protect against someone experimenting to support a new device
> > and setting different voltages without the daisy-chaining flag.
> 
> > My biggest worry is that someone with an unsupported machine may start
> > by copying and pasting an existing configuration to try it out, and fry
> > their hardware.
> 
> There's really nothing you can do that prevents this, especially in the
> cut'n'paste scenario.  Overrides tend to get copied along with the rest
> of the configuration, or checks hacked out if people think they're
> getting in the way without realising what they're there for.

Maybe a big fat warning comment in the code ? Apart from that, I agree,
I don't think we can do much.

-- 
Regards,

Laurent Pinchart

[PATCH v6 06/15] clk: Introduce clk-tps68470 driver

[Hans de Goede]

The TPS68470 PMIC provides Clocks, GPIOs and Regulators. At present in
the kernel the Regulators and Clocks are controlled by an OpRegion
driver designed to work with power control methods defined in ACPI, but
some platforms lack those methods, meaning drivers need to be able to
consume the resources of these chips through the usual frameworks.

This commit adds a driver for the clocks provided by the tps68470,
and is designed to bind to the platform_device registered by the
intel_skl_int3472 module.

This is based on this out of tree driver written by Intel:
https://github.com/intel/linux-intel-lts/blob/4.14/base/drivers/clk/clk-tps68470.c
with various cleanups added.

Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Chnages in v6:
- Use CLK_SET_RATE_GATE flag
- Move programming of dividers to tps68470_clk_set_rate()
- Put struct clk_init_data tps68470_clk_initdata on the stack

Changes in v5:
- Small comment cleanups based on review from Andy

Changes in v4:
- Kconfig: select REGMAP_I2C, add || COMPILE_TEST, fix help text
- tps68470_clk_prepare(): Wait for the PLL to lock before returning
- tps68470_clk_unprepare(): Remove unnecesary clearing of divider regs
- tps68470_clk_probe(): Use devm_clk_hw_register()
- Misc. small cleanups

Changes in v2:
- Update the comment on why a subsys_initcall is used to register the drv
- Fix trailing whitespice on line 100
---
 drivers/clk/Kconfig          |   8 ++
 drivers/clk/Makefile         |   1 +
 drivers/clk/clk-tps68470.c   | 261 +++++++++++++++++++++++++++++++++++
 include/linux/mfd/tps68470.h |  11 ++
 4 files changed, 281 insertions(+)
 create mode 100644 drivers/clk/clk-tps68470.c

diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig
index c5b3dc97396a..4e9098d79249 100644
--- a/drivers/clk/Kconfig
+++ b/drivers/clk/Kconfig
@@ -169,6 +169,14 @@ config COMMON_CLK_CDCE706
 	help
 	  This driver supports TI CDCE706 programmable 3-PLL clock synthesizer.
 
+config COMMON_CLK_TPS68470
+	tristate "Clock Driver for TI TPS68470 PMIC"
+	depends on I2C
+	depends on INTEL_SKL_INT3472 || COMPILE_TEST
+	select REGMAP_I2C
+	help
+	  This driver supports the clocks provided by the TPS68470 PMIC.
+
 config COMMON_CLK_CDCE925
 	tristate "Clock driver for TI CDCE913/925/937/949 devices"
 	depends on I2C
diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile
index e42312121e51..6b6a88ae1425 100644
--- a/drivers/clk/Makefile
+++ b/drivers/clk/Makefile
@@ -63,6 +63,7 @@ obj-$(CONFIG_COMMON_CLK_SI570)		+= clk-si570.o
 obj-$(CONFIG_COMMON_CLK_STM32F)		+= clk-stm32f4.o
 obj-$(CONFIG_COMMON_CLK_STM32H7)	+= clk-stm32h7.o
 obj-$(CONFIG_COMMON_CLK_STM32MP157)	+= clk-stm32mp1.o
+obj-$(CONFIG_COMMON_CLK_TPS68470)      += clk-tps68470.o
 obj-$(CONFIG_CLK_TWL6040)		+= clk-twl6040.o
 obj-$(CONFIG_ARCH_VT8500)		+= clk-vt8500.o
 obj-$(CONFIG_COMMON_CLK_VC5)		+= clk-versaclock5.o
diff --git a/drivers/clk/clk-tps68470.c b/drivers/clk/clk-tps68470.c
new file mode 100644
index 000000000000..e5fbefd6ac2d
--- /dev/null
+++ b/drivers/clk/clk-tps68470.c
@@ -0,0 +1,261 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Clock driver for TPS68470 PMIC
+ *
+ * Copyright (c) 2021 Red Hat Inc.
+ * Copyright (C) 2018 Intel Corporation
+ *
+ * Authors:
+ *	Hans de Goede <hdegoede@redhat.com>
+ *	Zaikuo Wang <zaikuo.wang@intel.com>
+ *	Tianshu Qiu <tian.shu.qiu@intel.com>
+ *	Jian Xu Zheng <jian.xu.zheng@intel.com>
+ *	Yuning Pu <yuning.pu@intel.com>
+ *	Antti Laakso <antti.laakso@intel.com>
+ */
+
+#include <linux/clk-provider.h>
+#include <linux/clkdev.h>
+#include <linux/kernel.h>
+#include <linux/mfd/tps68470.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/platform_data/tps68470.h>
+#include <linux/regmap.h>
+
+#define TPS68470_CLK_NAME "tps68470-clk"
+
+#define to_tps68470_clkdata(clkd) \
+	container_of(clkd, struct tps68470_clkdata, clkout_hw)
+
+static struct tps68470_clkout_freqs {
+	unsigned long freq;
+	unsigned int xtaldiv;
+	unsigned int plldiv;
+	unsigned int postdiv;
+	unsigned int buckdiv;
+	unsigned int boostdiv;
+} clk_freqs[] = {
+/*
+ *  The PLL is used to multiply the crystal oscillator
+ *  frequency range of 3 MHz to 27 MHz by a programmable
+ *  factor of F = (M/N)*(1/P) such that the output
+ *  available at the HCLK_A or HCLK_B pins are in the range
+ *  of 4 MHz to 64 MHz in increments of 0.1 MHz.
+ *
+ * hclk_# = osc_in * (((plldiv*2)+320) / (xtaldiv+30)) * (1 / 2^postdiv)
+ *
+ * PLL_REF_CLK should be as close as possible to 100kHz
+ * PLL_REF_CLK = input clk / XTALDIV[7:0] + 30)
+ *
+ * PLL_VCO_CLK = (PLL_REF_CLK * (plldiv*2 + 320))
+ *
+ * BOOST should be as close as possible to 2Mhz
+ * BOOST = PLL_VCO_CLK / (BOOSTDIV[4:0] + 16) *
+ *
+ * BUCK should be as close as possible to 5.2Mhz
+ * BUCK = PLL_VCO_CLK / (BUCKDIV[3:0] + 5)
+ *
+ * osc_in   xtaldiv  plldiv   postdiv   hclk_#
+ * 20Mhz    170      32       1         19.2Mhz
+ * 20Mhz    170      40       1         20Mhz
+ * 20Mhz    170      80       1         24Mhz
+ */
+	{ 19200000, 170, 32, 1, 2, 3 },
+	{ 20000000, 170, 40, 1, 3, 4 },
+	{ 24000000, 170, 80, 1, 4, 8 },
+};
+
+struct tps68470_clkdata {
+	struct clk_hw clkout_hw;
+	struct regmap *regmap;
+	unsigned long rate;
+};
+
+static int tps68470_clk_is_prepared(struct clk_hw *hw)
+{
+	struct tps68470_clkdata *clkdata = to_tps68470_clkdata(hw);
+	int val;
+
+	if (regmap_read(clkdata->regmap, TPS68470_REG_PLLCTL, &val))
+		return 0;
+
+	return val & TPS68470_PLL_EN_MASK;
+}
+
+static int tps68470_clk_prepare(struct clk_hw *hw)
+{
+	struct tps68470_clkdata *clkdata = to_tps68470_clkdata(hw);
+
+	regmap_write(clkdata->regmap, TPS68470_REG_CLKCFG1,
+			   (TPS68470_PLL_OUTPUT_ENABLE << TPS68470_OUTPUT_A_SHIFT) |
+			   (TPS68470_PLL_OUTPUT_ENABLE << TPS68470_OUTPUT_B_SHIFT));
+
+	regmap_update_bits(clkdata->regmap, TPS68470_REG_PLLCTL,
+			   TPS68470_PLL_EN_MASK, TPS68470_PLL_EN_MASK);
+
+	/*
+	 * The PLLCTL reg lock bit is set by the PMIC after approx. 4ms and
+	 * does not indicate a true lock, so just wait 4 ms.
+	 */
+	usleep_range(4000, 5000);
+
+	return 0;
+}
+
+static void tps68470_clk_unprepare(struct clk_hw *hw)
+{
+	struct tps68470_clkdata *clkdata = to_tps68470_clkdata(hw);
+
+	/* Disable clock first ... */
+	regmap_update_bits(clkdata->regmap, TPS68470_REG_PLLCTL, TPS68470_PLL_EN_MASK, 0);
+
+	/* ... and then tri-state the clock outputs. */
+	regmap_write(clkdata->regmap, TPS68470_REG_CLKCFG1, 0);
+}
+
+static unsigned long tps68470_clk_recalc_rate(struct clk_hw *hw, unsigned long parent_rate)
+{
+	struct tps68470_clkdata *clkdata = to_tps68470_clkdata(hw);
+
+	return clkdata->rate;
+}
+
+/*
+ * This returns the index of the clk_freqs[] cfg with the closest rate for
+ * use in tps68470_clk_round_rate(). tps68470_clk_set_rate() checks that
+ * the rate of the returned cfg is an exact match.
+ */
+static unsigned int tps68470_clk_cfg_lookup(unsigned long rate)
+{
+	long diff, best_diff = LONG_MAX;
+	unsigned int i, best_idx = 0;
+
+	for (i = 0; i < ARRAY_SIZE(clk_freqs); i++) {
+		diff = clk_freqs[i].freq - rate;
+		if (diff == 0)
+			return i;
+
+		diff = abs(diff);
+		if (diff < best_diff) {
+			best_diff = diff;
+			best_idx = i;
+		}
+	}
+
+	return best_idx;
+}
+
+static long tps68470_clk_round_rate(struct clk_hw *hw, unsigned long rate,
+				    unsigned long *parent_rate)
+{
+	unsigned int idx = tps68470_clk_cfg_lookup(rate);
+
+	return clk_freqs[idx].freq;
+}
+
+static int tps68470_clk_set_rate(struct clk_hw *hw, unsigned long rate,
+				 unsigned long parent_rate)
+{
+	struct tps68470_clkdata *clkdata = to_tps68470_clkdata(hw);
+	unsigned int idx = tps68470_clk_cfg_lookup(rate);
+
+	if (rate != clk_freqs[idx].freq)
+		return -EINVAL;
+
+	regmap_write(clkdata->regmap, TPS68470_REG_BOOSTDIV, clk_freqs[idx].boostdiv);
+	regmap_write(clkdata->regmap, TPS68470_REG_BUCKDIV, clk_freqs[idx].buckdiv);
+	regmap_write(clkdata->regmap, TPS68470_REG_PLLSWR, TPS68470_PLLSWR_DEFAULT);
+	regmap_write(clkdata->regmap, TPS68470_REG_XTALDIV, clk_freqs[idx].xtaldiv);
+	regmap_write(clkdata->regmap, TPS68470_REG_PLLDIV, clk_freqs[idx].plldiv);
+	regmap_write(clkdata->regmap, TPS68470_REG_POSTDIV, clk_freqs[idx].postdiv);
+	regmap_write(clkdata->regmap, TPS68470_REG_POSTDIV2, clk_freqs[idx].postdiv);
+	regmap_write(clkdata->regmap, TPS68470_REG_CLKCFG2, TPS68470_CLKCFG2_DRV_STR_2MA);
+
+	regmap_write(clkdata->regmap, TPS68470_REG_PLLCTL,
+		     TPS68470_OSC_EXT_CAP_DEFAULT << TPS68470_OSC_EXT_CAP_SHIFT |
+		     TPS68470_CLK_SRC_XTAL << TPS68470_CLK_SRC_SHIFT);
+
+	clkdata->rate = rate;
+
+	return 0;
+}
+
+static const struct clk_ops tps68470_clk_ops = {
+	.is_prepared = tps68470_clk_is_prepared,
+	.prepare = tps68470_clk_prepare,
+	.unprepare = tps68470_clk_unprepare,
+	.recalc_rate = tps68470_clk_recalc_rate,
+	.round_rate = tps68470_clk_round_rate,
+	.set_rate = tps68470_clk_set_rate,
+};
+
+static int tps68470_clk_probe(struct platform_device *pdev)
+{
+	struct tps68470_clk_platform_data *pdata = pdev->dev.platform_data;
+	struct clk_init_data tps68470_clk_initdata = {
+		.name = TPS68470_CLK_NAME,
+		.ops = &tps68470_clk_ops,
+		/* Changing the dividers when the PLL is on is not allowed */
+		.flags = CLK_SET_RATE_GATE,
+	};
+	struct tps68470_clkdata *tps68470_clkdata;
+	int ret;
+
+	tps68470_clkdata = devm_kzalloc(&pdev->dev, sizeof(*tps68470_clkdata),
+					GFP_KERNEL);
+	if (!tps68470_clkdata)
+		return -ENOMEM;
+
+	tps68470_clkdata->regmap = dev_get_drvdata(pdev->dev.parent);
+	tps68470_clkdata->clkout_hw.init = &tps68470_clk_initdata;
+
+	/* Set initial rate */
+	tps68470_clk_set_rate(&tps68470_clkdata->clkout_hw, clk_freqs[0].freq, 0);
+
+	ret = devm_clk_hw_register(&pdev->dev, &tps68470_clkdata->clkout_hw);
+	if (ret)
+		return ret;
+
+	ret = devm_clk_hw_register_clkdev(&pdev->dev, &tps68470_clkdata->clkout_hw,
+					  TPS68470_CLK_NAME, NULL);
+	if (ret)
+		return ret;
+
+	if (pdata) {
+		ret = devm_clk_hw_register_clkdev(&pdev->dev,
+						  &tps68470_clkdata->clkout_hw,
+						  pdata->consumer_con_id,
+						  pdata->consumer_dev_name);
+	}
+
+	return ret;
+}
+
+static struct platform_driver tps68470_clk_driver = {
+	.driver = {
+		.name = TPS68470_CLK_NAME,
+	},
+	.probe = tps68470_clk_probe,
+};
+
+/*
+ * The ACPI tps68470 probe-ordering depends on the clk/gpio/regulator drivers
+ * registering before the drivers for the camera-sensors which use them bind.
+ * subsys_initcall() ensures this when the drivers are builtin.
+ */
+static int __init tps68470_clk_init(void)
+{
+	return platform_driver_register(&tps68470_clk_driver);
+}
+subsys_initcall(tps68470_clk_init);
+
+static void __exit tps68470_clk_exit(void)
+{
+	platform_driver_unregister(&tps68470_clk_driver);
+}
+module_exit(tps68470_clk_exit);
+
+MODULE_ALIAS("platform:tps68470-clk");
+MODULE_DESCRIPTION("clock driver for TPS68470 pmic");
+MODULE_LICENSE("GPL");
diff --git a/include/linux/mfd/tps68470.h b/include/linux/mfd/tps68470.h
index ffe81127d91c..7807fa329db0 100644
--- a/include/linux/mfd/tps68470.h
+++ b/include/linux/mfd/tps68470.h
@@ -75,6 +75,17 @@
 #define TPS68470_CLKCFG1_MODE_A_MASK	GENMASK(1, 0)
 #define TPS68470_CLKCFG1_MODE_B_MASK	GENMASK(3, 2)
 
+#define TPS68470_CLKCFG2_DRV_STR_2MA	0x05
+#define TPS68470_PLL_OUTPUT_ENABLE	0x02
+#define TPS68470_CLK_SRC_XTAL		BIT(0)
+#define TPS68470_PLLSWR_DEFAULT		GENMASK(1, 0)
+#define TPS68470_OSC_EXT_CAP_DEFAULT	0x05
+
+#define TPS68470_OUTPUT_A_SHIFT		0x00
+#define TPS68470_OUTPUT_B_SHIFT		0x02
+#define TPS68470_CLK_SRC_SHIFT		GENMASK(2, 0)
+#define TPS68470_OSC_EXT_CAP_SHIFT	BIT(2)
+
 #define TPS68470_GPIO_CTL_REG_A(x)	(TPS68470_REG_GPCTL0A + (x) * 2)
 #define TPS68470_GPIO_CTL_REG_B(x)	(TPS68470_REG_GPCTL0B + (x) * 2)
 #define TPS68470_GPIO_MODE_MASK		GENMASK(1, 0)
-- 
2.33.1

[PATCH v6 07/15] platform/x86: int3472: Enable I2c daisy chain

[Hans de Goede]

From: Daniel Scally <djrscally@gmail.com>

The TPS68470 PMIC has an I2C passthrough mode through which I2C traffic
can be forwarded to a device connected to the PMIC as though it were
connected directly to the system bus. Enable this mode when the chip
is initialised.

Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
Signed-off-by: Daniel Scally <djrscally@gmail.com>
---
 .../x86/intel/int3472/intel_skl_int3472_tps68470.c         | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/platform/x86/intel/int3472/intel_skl_int3472_tps68470.c b/drivers/platform/x86/intel/int3472/intel_skl_int3472_tps68470.c
index c05b4cf502fe..42e688f4cad4 100644
--- a/drivers/platform/x86/intel/int3472/intel_skl_int3472_tps68470.c
+++ b/drivers/platform/x86/intel/int3472/intel_skl_int3472_tps68470.c
@@ -45,6 +45,13 @@ static int tps68470_chip_init(struct device *dev, struct regmap *regmap)
 		return ret;
 	}
 
+	/* Enable I2C daisy chain */
+	ret = regmap_write(regmap, TPS68470_REG_S_I2C_CTL, 0x03);
+	if (ret) {
+		dev_err(dev, "Failed to enable i2c daisy chain\n");
+		return ret;
+	}
+
 	dev_info(dev, "TPS68470 REVID: 0x%02x\n", version);
 
 	return 0;
-- 
2.33.1

Re: [PATCH v6 07/15] platform/x86: int3472: Enable I2c daisy chain

[Laurent Pinchart]

Hi Hans,

Thank you for the patch.

On Thu, Nov 25, 2021 at 05:54:04PM +0100, Hans de Goede wrote:
> From: Daniel Scally <djrscally@gmail.com>
> 
> The TPS68470 PMIC has an I2C passthrough mode through which I2C traffic
> can be forwarded to a device connected to the PMIC as though it were
> connected directly to the system bus. Enable this mode when the chip
> is initialised.

Is there any drawback doing this unconditionally, if nothing is
connected to the bus on the other side (including no pull-ups) ?

> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> Signed-off-by: Daniel Scally <djrscally@gmail.com>
> ---
>  .../x86/intel/int3472/intel_skl_int3472_tps68470.c         | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/drivers/platform/x86/intel/int3472/intel_skl_int3472_tps68470.c b/drivers/platform/x86/intel/int3472/intel_skl_int3472_tps68470.c
> index c05b4cf502fe..42e688f4cad4 100644
> --- a/drivers/platform/x86/intel/int3472/intel_skl_int3472_tps68470.c
> +++ b/drivers/platform/x86/intel/int3472/intel_skl_int3472_tps68470.c
> @@ -45,6 +45,13 @@ static int tps68470_chip_init(struct device *dev, struct regmap *regmap)
>  		return ret;
>  	}
>  
> +	/* Enable I2C daisy chain */
> +	ret = regmap_write(regmap, TPS68470_REG_S_I2C_CTL, 0x03);
> +	if (ret) {
> +		dev_err(dev, "Failed to enable i2c daisy chain\n");
> +		return ret;
> +	}
> +
>  	dev_info(dev, "TPS68470 REVID: 0x%02x\n", version);
>  
>  	return 0;

-- 
Regards,

Laurent Pinchart

Re: [PATCH v6 07/15] platform/x86: int3472: Enable I2c daisy chain

[Hans de Goede]

Hi,

On 11/26/21 00:39, Laurent Pinchart wrote:
> Hi Hans,
> 
> Thank you for the patch.
> 
> On Thu, Nov 25, 2021 at 05:54:04PM +0100, Hans de Goede wrote:
>> From: Daniel Scally <djrscally@gmail.com>
>>
>> The TPS68470 PMIC has an I2C passthrough mode through which I2C traffic
>> can be forwarded to a device connected to the PMIC as though it were
>> connected directly to the system bus. Enable this mode when the chip
>> is initialised.
> 
> Is there any drawback doing this unconditionally, if nothing is
> connected to the bus on the other side (including no pull-ups) ?

I actually never took a really close look at this patch, I just
sorta inherited it from Daniel.

Now that I have taken a close look, I see that it is setting the
exact same bits as which get set when enabling the VSIO regulator.

The idea here is that the I2C-passthrough only gets enabled when
the VSIO regulator is turned on, because some sensors end up
shorting the I2C pins to ground when the sensor is not powered.

Since we set these bits when powering up the VSIO regulator
and since we do that before trying to talk to the sensor
I don't think that we need this (hack) anymore.

I will give things a try without this change and if things
still work I will drop this patch from the set.

Daniel, what do you think?

Regards,

Hans






> 
>> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
>> Signed-off-by: Daniel Scally <djrscally@gmail.com>
>> ---
>>  .../x86/intel/int3472/intel_skl_int3472_tps68470.c         | 7 +++++++
>>  1 file changed, 7 insertions(+)
>>
>> diff --git a/drivers/platform/x86/intel/int3472/intel_skl_int3472_tps68470.c b/drivers/platform/x86/intel/int3472/intel_skl_int3472_tps68470.c
>> index c05b4cf502fe..42e688f4cad4 100644
>> --- a/drivers/platform/x86/intel/int3472/intel_skl_int3472_tps68470.c
>> +++ b/drivers/platform/x86/intel/int3472/intel_skl_int3472_tps68470.c
>> @@ -45,6 +45,13 @@ static int tps68470_chip_init(struct device *dev, struct regmap *regmap)
>>  		return ret;
>>  	}
>>  
>> +	/* Enable I2C daisy chain */
>> +	ret = regmap_write(regmap, TPS68470_REG_S_I2C_CTL, 0x03);
>> +	if (ret) {
>> +		dev_err(dev, "Failed to enable i2c daisy chain\n");
>> +		return ret;
>> +	}
>> +
>>  	dev_info(dev, "TPS68470 REVID: 0x%02x\n", version);
>>  
>>  	return 0;
> 

Re: [PATCH v6 07/15] platform/x86: int3472: Enable I2c daisy chain

[Daniel Scally]

Hello

On 26/11/2021 11:30, Hans de Goede wrote:
> Hi,
>
> On 11/26/21 00:39, Laurent Pinchart wrote:
>> Hi Hans,
>>
>> Thank you for the patch.
>>
>> On Thu, Nov 25, 2021 at 05:54:04PM +0100, Hans de Goede wrote:
>>> From: Daniel Scally <djrscally@gmail.com>
>>>
>>> The TPS68470 PMIC has an I2C passthrough mode through which I2C traffic
>>> can be forwarded to a device connected to the PMIC as though it were
>>> connected directly to the system bus. Enable this mode when the chip
>>> is initialised.
>> Is there any drawback doing this unconditionally, if nothing is
>> connected to the bus on the other side (including no pull-ups) ?
> I actually never took a really close look at this patch, I just
> sorta inherited it from Daniel.
>
> Now that I have taken a close look, I see that it is setting the
> exact same bits as which get set when enabling the VSIO regulator.
>
> The idea here is that the I2C-passthrough only gets enabled when
> the VSIO regulator is turned on, because some sensors end up
> shorting the I2C pins to ground when the sensor is not powered.
>
> Since we set these bits when powering up the VSIO regulator
> and since we do that before trying to talk to the sensor
> I don't think that we need this (hack) anymore.
>
> I will give things a try without this change and if things
> still work I will drop this patch from the set.
>
> Daniel, what do you think?


Humm, we're only using the VSIO regulator with the VCM though right?
Which might not be on when the ov8865 tries to probe. I haven't tried
without this patch to be honest; I set it because that was what Windows
does when powering on the PMIC.

> Regards,
>
> Hans
>
>
>
>
>
>
>>> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
>>> Signed-off-by: Daniel Scally <djrscally@gmail.com>
>>> ---
>>>  .../x86/intel/int3472/intel_skl_int3472_tps68470.c         | 7 +++++++
>>>  1 file changed, 7 insertions(+)
>>>
>>> diff --git a/drivers/platform/x86/intel/int3472/intel_skl_int3472_tps68470.c b/drivers/platform/x86/intel/int3472/intel_skl_int3472_tps68470.c
>>> index c05b4cf502fe..42e688f4cad4 100644
>>> --- a/drivers/platform/x86/intel/int3472/intel_skl_int3472_tps68470.c
>>> +++ b/drivers/platform/x86/intel/int3472/intel_skl_int3472_tps68470.c
>>> @@ -45,6 +45,13 @@ static int tps68470_chip_init(struct device *dev, struct regmap *regmap)
>>>  		return ret;
>>>  	}
>>>  
>>> +	/* Enable I2C daisy chain */
>>> +	ret = regmap_write(regmap, TPS68470_REG_S_I2C_CTL, 0x03);
>>> +	if (ret) {
>>> +		dev_err(dev, "Failed to enable i2c daisy chain\n");
>>> +		return ret;
>>> +	}
>>> +
>>>  	dev_info(dev, "TPS68470 REVID: 0x%02x\n", version);
>>>  
>>>  	return 0;

Re: [PATCH v6 07/15] platform/x86: int3472: Enable I2c daisy chain

[Hans de Goede]

Hi,

On 11/26/21 12:39, Daniel Scally wrote:
> Hello
> 
> On 26/11/2021 11:30, Hans de Goede wrote:
>> Hi,
>>
>> On 11/26/21 00:39, Laurent Pinchart wrote:
>>> Hi Hans,
>>>
>>> Thank you for the patch.
>>>
>>> On Thu, Nov 25, 2021 at 05:54:04PM +0100, Hans de Goede wrote:
>>>> From: Daniel Scally <djrscally@gmail.com>
>>>>
>>>> The TPS68470 PMIC has an I2C passthrough mode through which I2C traffic
>>>> can be forwarded to a device connected to the PMIC as though it were
>>>> connected directly to the system bus. Enable this mode when the chip
>>>> is initialised.
>>> Is there any drawback doing this unconditionally, if nothing is
>>> connected to the bus on the other side (including no pull-ups) ?
>> I actually never took a really close look at this patch, I just
>> sorta inherited it from Daniel.
>>
>> Now that I have taken a close look, I see that it is setting the
>> exact same bits as which get set when enabling the VSIO regulator.
>>
>> The idea here is that the I2C-passthrough only gets enabled when
>> the VSIO regulator is turned on, because some sensors end up
>> shorting the I2C pins to ground when the sensor is not powered.
>>
>> Since we set these bits when powering up the VSIO regulator
>> and since we do that before trying to talk to the sensor
>> I don't think that we need this (hack) anymore.
>>
>> I will give things a try without this change and if things
>> still work I will drop this patch from the set.
>>
>> Daniel, what do you think?
> 
> 
> Humm, we're only using the VSIO regulator with the VCM though right?

Nope, there is a mapping from VSIO to dovdd for the ov8865 in the
board_data; and I'm pretty sure I copied that from your earlier
attempts at getting regulator lookups registered :)

And even if the VSIO regulator was only used by the VCM, then it would
get turned off after probing the VCM, clearing the 2 bits which this
commit sets. Which would break things if we did not re-enable it when
the ov8865 needs it.

> Which might not be on when the ov8865 tries to probe. I haven't tried
> without this patch to be honest; I set it because that was what Windows
> does when powering on the PMIC.

See above, I'm pretty sure we can do without this patch which means
that the INT3472 code will no longer be poking at the PMIC directly
itself, which is good :)

Anyways I'll give this a try sometime next week and then drop the
patch.

Regards,

Hans




>>>> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
>>>> Signed-off-by: Daniel Scally <djrscally@gmail.com>
>>>> ---
>>>>  .../x86/intel/int3472/intel_skl_int3472_tps68470.c         | 7 +++++++
>>>>  1 file changed, 7 insertions(+)
>>>>
>>>> diff --git a/drivers/platform/x86/intel/int3472/intel_skl_int3472_tps68470.c b/drivers/platform/x86/intel/int3472/intel_skl_int3472_tps68470.c
>>>> index c05b4cf502fe..42e688f4cad4 100644
>>>> --- a/drivers/platform/x86/intel/int3472/intel_skl_int3472_tps68470.c
>>>> +++ b/drivers/platform/x86/intel/int3472/intel_skl_int3472_tps68470.c
>>>> @@ -45,6 +45,13 @@ static int tps68470_chip_init(struct device *dev, struct regmap *regmap)
>>>>  		return ret;
>>>>  	}
>>>>  
>>>> +	/* Enable I2C daisy chain */
>>>> +	ret = regmap_write(regmap, TPS68470_REG_S_I2C_CTL, 0x03);
>>>> +	if (ret) {
>>>> +		dev_err(dev, "Failed to enable i2c daisy chain\n");
>>>> +		return ret;
>>>> +	}
>>>> +
>>>>  	dev_info(dev, "TPS68470 REVID: 0x%02x\n", version);
>>>>  
>>>>  	return 0;
> 

Re: [PATCH v6 07/15] platform/x86: int3472: Enable I2c daisy chain

[Daniel Scally]

On 26/11/2021 11:45, Hans de Goede wrote:
> Hi,
>
> On 11/26/21 12:39, Daniel Scally wrote:
>> Hello
>>
>> On 26/11/2021 11:30, Hans de Goede wrote:
>>> Hi,
>>>
>>> On 11/26/21 00:39, Laurent Pinchart wrote:
>>>> Hi Hans,
>>>>
>>>> Thank you for the patch.
>>>>
>>>> On Thu, Nov 25, 2021 at 05:54:04PM +0100, Hans de Goede wrote:
>>>>> From: Daniel Scally <djrscally@gmail.com>
>>>>>
>>>>> The TPS68470 PMIC has an I2C passthrough mode through which I2C traffic
>>>>> can be forwarded to a device connected to the PMIC as though it were
>>>>> connected directly to the system bus. Enable this mode when the chip
>>>>> is initialised.
>>>> Is there any drawback doing this unconditionally, if nothing is
>>>> connected to the bus on the other side (including no pull-ups) ?
>>> I actually never took a really close look at this patch, I just
>>> sorta inherited it from Daniel.
>>>
>>> Now that I have taken a close look, I see that it is setting the
>>> exact same bits as which get set when enabling the VSIO regulator.
>>>
>>> The idea here is that the I2C-passthrough only gets enabled when
>>> the VSIO regulator is turned on, because some sensors end up
>>> shorting the I2C pins to ground when the sensor is not powered.
>>>
>>> Since we set these bits when powering up the VSIO regulator
>>> and since we do that before trying to talk to the sensor
>>> I don't think that we need this (hack) anymore.
>>>
>>> I will give things a try without this change and if things
>>> still work I will drop this patch from the set.
>>>
>>> Daniel, what do you think?
>>
>> Humm, we're only using the VSIO regulator with the VCM though right?
> Nope, there is a mapping from VSIO to dovdd for the ov8865 in the
> board_data; and I'm pretty sure I copied that from your earlier
> attempts at getting regulator lookups registered :)

Oh yeah derp; I was looking at the supply names rather than the
regulator names, my bad!
> And even if the VSIO regulator was only used by the VCM, then it would
> get turned off after probing the VCM, clearing the 2 bits which this
> commit sets. Which would break things if we did not re-enable it when
> the ov8865 needs it.
>
>> Which might not be on when the ov8865 tries to probe. I haven't tried
>> without this patch to be honest; I set it because that was what Windows
>> does when powering on the PMIC.
> See above, I'm pretty sure we can do without this patch which means
> that the INT3472 code will no longer be poking at the PMIC directly
> itself, which is good :)


Yeah, in that case I think you're right and this can be dropped.

> Anyways I'll give this a try sometime next week and then drop the
> patch.


Sounds good

>
> Regards,
>
> Hans
>
>
>
>
>>>>> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
>>>>> Signed-off-by: Daniel Scally <djrscally@gmail.com>
>>>>> ---
>>>>>  .../x86/intel/int3472/intel_skl_int3472_tps68470.c         | 7 +++++++
>>>>>  1 file changed, 7 insertions(+)
>>>>>
>>>>> diff --git a/drivers/platform/x86/intel/int3472/intel_skl_int3472_tps68470.c b/drivers/platform/x86/intel/int3472/intel_skl_int3472_tps68470.c
>>>>> index c05b4cf502fe..42e688f4cad4 100644
>>>>> --- a/drivers/platform/x86/intel/int3472/intel_skl_int3472_tps68470.c
>>>>> +++ b/drivers/platform/x86/intel/int3472/intel_skl_int3472_tps68470.c
>>>>> @@ -45,6 +45,13 @@ static int tps68470_chip_init(struct device *dev, struct regmap *regmap)
>>>>>  		return ret;
>>>>>  	}
>>>>>  
>>>>> +	/* Enable I2C daisy chain */
>>>>> +	ret = regmap_write(regmap, TPS68470_REG_S_I2C_CTL, 0x03);
>>>>> +	if (ret) {
>>>>> +		dev_err(dev, "Failed to enable i2c daisy chain\n");
>>>>> +		return ret;
>>>>> +	}
>>>>> +
>>>>>  	dev_info(dev, "TPS68470 REVID: 0x%02x\n", version);
>>>>>  
>>>>>  	return 0;

Re: [PATCH v6 07/15] platform/x86: int3472: Enable I2c daisy chain

[Hans de Goede]

Hi,

On 11/26/21 12:45, Hans de Goede wrote:
> Hi,
> 
> On 11/26/21 12:39, Daniel Scally wrote:
>> Hello
>>
>> On 26/11/2021 11:30, Hans de Goede wrote:
>>> Hi,
>>>
>>> On 11/26/21 00:39, Laurent Pinchart wrote:
>>>> Hi Hans,
>>>>
>>>> Thank you for the patch.
>>>>
>>>> On Thu, Nov 25, 2021 at 05:54:04PM +0100, Hans de Goede wrote:
>>>>> From: Daniel Scally <djrscally@gmail.com>
>>>>>
>>>>> The TPS68470 PMIC has an I2C passthrough mode through which I2C traffic
>>>>> can be forwarded to a device connected to the PMIC as though it were
>>>>> connected directly to the system bus. Enable this mode when the chip
>>>>> is initialised.
>>>> Is there any drawback doing this unconditionally, if nothing is
>>>> connected to the bus on the other side (including no pull-ups) ?
>>> I actually never took a really close look at this patch, I just
>>> sorta inherited it from Daniel.
>>>
>>> Now that I have taken a close look, I see that it is setting the
>>> exact same bits as which get set when enabling the VSIO regulator.
>>>
>>> The idea here is that the I2C-passthrough only gets enabled when
>>> the VSIO regulator is turned on, because some sensors end up
>>> shorting the I2C pins to ground when the sensor is not powered.
>>>
>>> Since we set these bits when powering up the VSIO regulator
>>> and since we do that before trying to talk to the sensor
>>> I don't think that we need this (hack) anymore.
>>>
>>> I will give things a try without this change and if things
>>> still work I will drop this patch from the set.
>>>
>>> Daniel, what do you think?
>>
>>
>> Humm, we're only using the VSIO regulator with the VCM though right?
> 
> Nope, there is a mapping from VSIO to dovdd for the ov8865 in the
> board_data; and I'm pretty sure I copied that from your earlier
> attempts at getting regulator lookups registered :)
> 
> And even if the VSIO regulator was only used by the VCM, then it would
> get turned off after probing the VCM, clearing the 2 bits which this
> commit sets. Which would break things if we did not re-enable it when
> the ov8865 needs it.
> 
>> Which might not be on when the ov8865 tries to probe. I haven't tried
>> without this patch to be honest; I set it because that was what Windows
>> does when powering on the PMIC.
> 
> See above, I'm pretty sure we can do without this patch which means
> that the INT3472 code will no longer be poking at the PMIC directly
> itself, which is good :)
> 
> Anyways I'll give this a try sometime next week and then drop the
> patch.

I can confirm that this patch indeed is no longer necessary with
the current regulator code already taking care of this.

I will post version 7 of this patch-set soon, with this patch dropped.

Regards,

Hans



>>>>> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
>>>>> Signed-off-by: Daniel Scally <djrscally@gmail.com>
>>>>> ---
>>>>>  .../x86/intel/int3472/intel_skl_int3472_tps68470.c         | 7 +++++++
>>>>>  1 file changed, 7 insertions(+)
>>>>>
>>>>> diff --git a/drivers/platform/x86/intel/int3472/intel_skl_int3472_tps68470.c b/drivers/platform/x86/intel/int3472/intel_skl_int3472_tps68470.c
>>>>> index c05b4cf502fe..42e688f4cad4 100644
>>>>> --- a/drivers/platform/x86/intel/int3472/intel_skl_int3472_tps68470.c
>>>>> +++ b/drivers/platform/x86/intel/int3472/intel_skl_int3472_tps68470.c
>>>>> @@ -45,6 +45,13 @@ static int tps68470_chip_init(struct device *dev, struct regmap *regmap)
>>>>>  		return ret;
>>>>>  	}
>>>>>  
>>>>> +	/* Enable I2C daisy chain */
>>>>> +	ret = regmap_write(regmap, TPS68470_REG_S_I2C_CTL, 0x03);
>>>>> +	if (ret) {
>>>>> +		dev_err(dev, "Failed to enable i2c daisy chain\n");
>>>>> +		return ret;
>>>>> +	}
>>>>> +
>>>>>  	dev_info(dev, "TPS68470 REVID: 0x%02x\n", version);
>>>>>  
>>>>>  	return 0;
>>

[PATCH v6 08/15] platform/x86: int3472: Split into 2 drivers

[Hans de Goede]

The intel_skl_int3472.ko module contains 2 separate drivers,
the int3472_discrete platform driver and the int3472_tps68470
I2C-driver.

These 2 drivers contain very little shared code, only
skl_int3472_get_acpi_buffer() and skl_int3472_fill_cldb() are
shared.

Split the module into 2 drivers, linking the little shared code
directly into both.

This will allow us to add soft-module dependencies for the
tps68470 clk, gpio and regulator drivers to the new
intel_skl_int3472_tps68470.ko to help with probe ordering issues
without causing these modules to get loaded on boards which only
use the int3472_discrete platform driver.

While at it also rename the .c and .h files to remove the
cumbersome intel_skl_int3472_ prefix.

Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Note git rename detection is failing for the new common.c but this is
just the old intel_skl_int3472_common.c with the driver registering
bits removed.
---
 drivers/platform/x86/intel/int3472/Makefile   |   9 +-
 ...lk_and_regulator.c => clk_and_regulator.c} |   2 +-
 drivers/platform/x86/intel/int3472/common.c   |  54 +++++++++
 .../{intel_skl_int3472_common.h => common.h}  |   3 -
 ...ntel_skl_int3472_discrete.c => discrete.c} |  28 ++++-
 .../intel/int3472/intel_skl_int3472_common.c  | 106 ------------------
 ...ntel_skl_int3472_tps68470.c => tps68470.c} |  23 +++-
 7 files changed, 105 insertions(+), 120 deletions(-)
 rename drivers/platform/x86/intel/int3472/{intel_skl_int3472_clk_and_regulator.c => clk_and_regulator.c} (99%)
 create mode 100644 drivers/platform/x86/intel/int3472/common.c
 rename drivers/platform/x86/intel/int3472/{intel_skl_int3472_common.h => common.h} (94%)
 rename drivers/platform/x86/intel/int3472/{intel_skl_int3472_discrete.c => discrete.c} (93%)
 delete mode 100644 drivers/platform/x86/intel/int3472/intel_skl_int3472_common.c
 rename drivers/platform/x86/intel/int3472/{intel_skl_int3472_tps68470.c => tps68470.c} (86%)

diff --git a/drivers/platform/x86/intel/int3472/Makefile b/drivers/platform/x86/intel/int3472/Makefile
index 2362e04db18d..771e720528a0 100644
--- a/drivers/platform/x86/intel/int3472/Makefile
+++ b/drivers/platform/x86/intel/int3472/Makefile
@@ -1,5 +1,4 @@
-obj-$(CONFIG_INTEL_SKL_INT3472)		+= intel_skl_int3472.o
-intel_skl_int3472-y			:= intel_skl_int3472_common.o \
-					   intel_skl_int3472_discrete.o \
-					   intel_skl_int3472_tps68470.o \
-					   intel_skl_int3472_clk_and_regulator.o
+obj-$(CONFIG_INTEL_SKL_INT3472)		+= intel_skl_int3472_discrete.o \
+					   intel_skl_int3472_tps68470.o
+intel_skl_int3472_discrete-y		:= discrete.o clk_and_regulator.o common.o
+intel_skl_int3472_tps68470-y		:= tps68470.o common.o
diff --git a/drivers/platform/x86/intel/int3472/intel_skl_int3472_clk_and_regulator.c b/drivers/platform/x86/intel/int3472/clk_and_regulator.c
similarity index 99%
rename from drivers/platform/x86/intel/int3472/intel_skl_int3472_clk_and_regulator.c
rename to drivers/platform/x86/intel/int3472/clk_and_regulator.c
index 1700e7557a82..1cf958983e86 100644
--- a/drivers/platform/x86/intel/int3472/intel_skl_int3472_clk_and_regulator.c
+++ b/drivers/platform/x86/intel/int3472/clk_and_regulator.c
@@ -9,7 +9,7 @@
 #include <linux/regulator/driver.h>
 #include <linux/slab.h>
 
-#include "intel_skl_int3472_common.h"
+#include "common.h"
 
 /*
  * The regulators have to have .ops to be valid, but the only ops we actually
diff --git a/drivers/platform/x86/intel/int3472/common.c b/drivers/platform/x86/intel/int3472/common.c
new file mode 100644
index 000000000000..350655a9515b
--- /dev/null
+++ b/drivers/platform/x86/intel/int3472/common.c
@@ -0,0 +1,54 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Author: Dan Scally <djrscally@gmail.com> */
+
+#include <linux/acpi.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 };
+	acpi_handle handle = adev->handle;
+	union acpi_object *obj;
+	acpi_status status;
+
+	status = acpi_evaluate_object(handle, id, NULL, &buffer);
+	if (ACPI_FAILURE(status))
+		return ERR_PTR(-ENODEV);
+
+	obj = buffer.pointer;
+	if (!obj)
+		return ERR_PTR(-ENODEV);
+
+	if (obj->type != ACPI_TYPE_BUFFER) {
+		acpi_handle_err(handle, "%s object is not an ACPI buffer\n", id);
+		kfree(obj);
+		return ERR_PTR(-EINVAL);
+	}
+
+	return obj;
+}
+
+int skl_int3472_fill_cldb(struct acpi_device *adev, struct int3472_cldb *cldb)
+{
+	union acpi_object *obj;
+	int ret;
+
+	obj = skl_int3472_get_acpi_buffer(adev, "CLDB");
+	if (IS_ERR(obj))
+		return PTR_ERR(obj);
+
+	if (obj->buffer.length > sizeof(*cldb)) {
+		acpi_handle_err(adev->handle, "The CLDB buffer is too large\n");
+		ret = -EINVAL;
+		goto out_free_obj;
+	}
+
+	memcpy(cldb, obj->buffer.pointer, obj->buffer.length);
+	ret = 0;
+
+out_free_obj:
+	kfree(obj);
+	return ret;
+}
diff --git a/drivers/platform/x86/intel/int3472/intel_skl_int3472_common.h b/drivers/platform/x86/intel/int3472/common.h
similarity index 94%
rename from drivers/platform/x86/intel/int3472/intel_skl_int3472_common.h
rename to drivers/platform/x86/intel/int3472/common.h
index 714fde73b524..d14944ee8586 100644
--- a/drivers/platform/x86/intel/int3472/intel_skl_int3472_common.h
+++ b/drivers/platform/x86/intel/int3472/common.h
@@ -105,9 +105,6 @@ struct int3472_discrete_device {
 	struct gpiod_lookup_table gpios;
 };
 
-int skl_int3472_discrete_probe(struct platform_device *pdev);
-int skl_int3472_discrete_remove(struct platform_device *pdev);
-int skl_int3472_tps68470_probe(struct i2c_client *client);
 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);
diff --git a/drivers/platform/x86/intel/int3472/intel_skl_int3472_discrete.c b/drivers/platform/x86/intel/int3472/discrete.c
similarity index 93%
rename from drivers/platform/x86/intel/int3472/intel_skl_int3472_discrete.c
rename to drivers/platform/x86/intel/int3472/discrete.c
index e59d79c7e82f..d2e8a87a077e 100644
--- a/drivers/platform/x86/intel/int3472/intel_skl_int3472_discrete.c
+++ b/drivers/platform/x86/intel/int3472/discrete.c
@@ -14,7 +14,7 @@
 #include <linux/platform_device.h>
 #include <linux/uuid.h>
 
-#include "intel_skl_int3472_common.h"
+#include "common.h"
 
 /*
  * 79234640-9e10-4fea-a5c1-b5aa8b19756f
@@ -332,7 +332,9 @@ static int skl_int3472_parse_crs(struct int3472_discrete_device *int3472)
 	return 0;
 }
 
-int skl_int3472_discrete_probe(struct platform_device *pdev)
+static int 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);
 	struct int3472_discrete_device *int3472;
@@ -395,7 +397,7 @@ int skl_int3472_discrete_probe(struct platform_device *pdev)
 	return ret;
 }
 
-int skl_int3472_discrete_remove(struct platform_device *pdev)
+static int skl_int3472_discrete_remove(struct platform_device *pdev)
 {
 	struct int3472_discrete_device *int3472 = platform_get_drvdata(pdev);
 
@@ -411,3 +413,23 @@ int skl_int3472_discrete_remove(struct platform_device *pdev)
 
 	return 0;
 }
+
+static const struct acpi_device_id int3472_device_id[] = {
+	{ "INT3472", 0 },
+	{ }
+};
+MODULE_DEVICE_TABLE(acpi, int3472_device_id);
+
+static struct platform_driver int3472_discrete = {
+	.driver = {
+		.name = "int3472-discrete",
+		.acpi_match_table = int3472_device_id,
+	},
+	.probe = skl_int3472_discrete_probe,
+	.remove = skl_int3472_discrete_remove,
+};
+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");
diff --git a/drivers/platform/x86/intel/int3472/intel_skl_int3472_common.c b/drivers/platform/x86/intel/int3472/intel_skl_int3472_common.c
deleted file mode 100644
index 497e74fba75f..000000000000
--- a/drivers/platform/x86/intel/int3472/intel_skl_int3472_common.c
+++ /dev/null
@@ -1,106 +0,0 @@
-// SPDX-License-Identifier: GPL-2.0
-/* Author: Dan Scally <djrscally@gmail.com> */
-
-#include <linux/acpi.h>
-#include <linux/i2c.h>
-#include <linux/platform_device.h>
-#include <linux/slab.h>
-
-#include "intel_skl_int3472_common.h"
-
-union acpi_object *skl_int3472_get_acpi_buffer(struct acpi_device *adev, char *id)
-{
-	struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL };
-	acpi_handle handle = adev->handle;
-	union acpi_object *obj;
-	acpi_status status;
-
-	status = acpi_evaluate_object(handle, id, NULL, &buffer);
-	if (ACPI_FAILURE(status))
-		return ERR_PTR(-ENODEV);
-
-	obj = buffer.pointer;
-	if (!obj)
-		return ERR_PTR(-ENODEV);
-
-	if (obj->type != ACPI_TYPE_BUFFER) {
-		acpi_handle_err(handle, "%s object is not an ACPI buffer\n", id);
-		kfree(obj);
-		return ERR_PTR(-EINVAL);
-	}
-
-	return obj;
-}
-
-int skl_int3472_fill_cldb(struct acpi_device *adev, struct int3472_cldb *cldb)
-{
-	union acpi_object *obj;
-	int ret;
-
-	obj = skl_int3472_get_acpi_buffer(adev, "CLDB");
-	if (IS_ERR(obj))
-		return PTR_ERR(obj);
-
-	if (obj->buffer.length > sizeof(*cldb)) {
-		acpi_handle_err(adev->handle, "The CLDB buffer is too large\n");
-		ret = -EINVAL;
-		goto out_free_obj;
-	}
-
-	memcpy(cldb, obj->buffer.pointer, obj->buffer.length);
-	ret = 0;
-
-out_free_obj:
-	kfree(obj);
-	return ret;
-}
-
-static const struct acpi_device_id int3472_device_id[] = {
-	{ "INT3472", 0 },
-	{ }
-};
-MODULE_DEVICE_TABLE(acpi, int3472_device_id);
-
-static struct platform_driver int3472_discrete = {
-	.driver = {
-		.name = "int3472-discrete",
-		.acpi_match_table = int3472_device_id,
-	},
-	.probe = skl_int3472_discrete_probe,
-	.remove = skl_int3472_discrete_remove,
-};
-
-static struct i2c_driver int3472_tps68470 = {
-	.driver = {
-		.name = "int3472-tps68470",
-		.acpi_match_table = int3472_device_id,
-	},
-	.probe_new = skl_int3472_tps68470_probe,
-};
-
-static int skl_int3472_init(void)
-{
-	int ret;
-
-	ret = platform_driver_register(&int3472_discrete);
-	if (ret)
-		return ret;
-
-	ret = i2c_register_driver(THIS_MODULE, &int3472_tps68470);
-	if (ret)
-		platform_driver_unregister(&int3472_discrete);
-
-	return ret;
-}
-module_init(skl_int3472_init);
-
-static void skl_int3472_exit(void)
-{
-	platform_driver_unregister(&int3472_discrete);
-	i2c_del_driver(&int3472_tps68470);
-}
-module_exit(skl_int3472_exit);
-
-MODULE_DESCRIPTION("Intel SkyLake INT3472 ACPI Device Driver");
-MODULE_AUTHOR("Daniel Scally <djrscally@gmail.com>");
-MODULE_LICENSE("GPL v2");
diff --git a/drivers/platform/x86/intel/int3472/intel_skl_int3472_tps68470.c b/drivers/platform/x86/intel/int3472/tps68470.c
similarity index 86%
rename from drivers/platform/x86/intel/int3472/intel_skl_int3472_tps68470.c
rename to drivers/platform/x86/intel/int3472/tps68470.c
index 42e688f4cad4..e95b0f50b384 100644
--- a/drivers/platform/x86/intel/int3472/intel_skl_int3472_tps68470.c
+++ b/drivers/platform/x86/intel/int3472/tps68470.c
@@ -7,7 +7,7 @@
 #include <linux/platform_device.h>
 #include <linux/regmap.h>
 
-#include "intel_skl_int3472_common.h"
+#include "common.h"
 
 #define DESIGNED_FOR_CHROMEOS		1
 #define DESIGNED_FOR_WINDOWS		2
@@ -102,7 +102,7 @@ static int skl_int3472_tps68470_calc_type(struct acpi_device *adev)
 	return DESIGNED_FOR_WINDOWS;
 }
 
-int skl_int3472_tps68470_probe(struct i2c_client *client)
+static int skl_int3472_tps68470_probe(struct i2c_client *client)
 {
 	struct acpi_device *adev = ACPI_COMPANION(&client->dev);
 	struct regmap *regmap;
@@ -142,3 +142,22 @@ int skl_int3472_tps68470_probe(struct i2c_client *client)
 
 	return ret;
 }
+
+static const struct acpi_device_id int3472_device_id[] = {
+	{ "INT3472", 0 },
+	{ }
+};
+MODULE_DEVICE_TABLE(acpi, int3472_device_id);
+
+static struct i2c_driver int3472_tps68470 = {
+	.driver = {
+		.name = "int3472-tps68470",
+		.acpi_match_table = int3472_device_id,
+	},
+	.probe_new = skl_int3472_tps68470_probe,
+};
+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");
-- 
2.33.1

[PATCH v6 09/15] platform/x86: int3472: Add get_sensor_adev_and_name() helper

[Hans de Goede]

The discrete.c code is not the only code which needs to lookup the
acpi_device and device-name for the sensor for which the INT3472
ACPI-device is a GPIO/clk/regulator provider.

The tps68470.c code also needs this functionality, so factor this
out into a new get_sensor_adev_and_name() helper.

Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/platform/x86/intel/int3472/common.c   | 28 +++++++++++++++++++
 drivers/platform/x86/intel/int3472/common.h   |  3 ++
 drivers/platform/x86/intel/int3472/discrete.c | 22 +++------------
 3 files changed, 35 insertions(+), 18 deletions(-)

diff --git a/drivers/platform/x86/intel/int3472/common.c b/drivers/platform/x86/intel/int3472/common.c
index 350655a9515b..77cf058e4168 100644
--- a/drivers/platform/x86/intel/int3472/common.c
+++ b/drivers/platform/x86/intel/int3472/common.c
@@ -52,3 +52,31 @@ int skl_int3472_fill_cldb(struct acpi_device *adev, struct int3472_cldb *cldb)
 	kfree(obj);
 	return ret;
 }
+
+/* sensor_adev_ret may be NULL, name_ret must not be NULL */
+int skl_int3472_get_sensor_adev_and_name(struct device *dev,
+					 struct acpi_device **sensor_adev_ret,
+					 const char **name_ret)
+{
+	struct acpi_device *adev = ACPI_COMPANION(dev);
+	struct acpi_device *sensor;
+	int ret = 0;
+
+	sensor = acpi_dev_get_first_consumer_dev(adev);
+	if (!sensor) {
+		dev_err(dev, "INT3472 seems to have no dependents.\n");
+		return -ENODEV;
+	}
+
+	*name_ret = devm_kasprintf(dev, GFP_KERNEL, I2C_DEV_NAME_FORMAT,
+				   acpi_dev_name(sensor));
+	if (!*name_ret)
+		ret = -ENOMEM;
+
+	if (ret == 0 && sensor_adev_ret)
+		*sensor_adev_ret = sensor;
+	else
+		acpi_dev_put(sensor);
+
+	return ret;
+}
diff --git a/drivers/platform/x86/intel/int3472/common.h b/drivers/platform/x86/intel/int3472/common.h
index d14944ee8586..53270d19c73a 100644
--- a/drivers/platform/x86/intel/int3472/common.h
+++ b/drivers/platform/x86/intel/int3472/common.h
@@ -108,6 +108,9 @@ struct int3472_discrete_device {
 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);
+int skl_int3472_get_sensor_adev_and_name(struct device *dev,
+					 struct acpi_device **sensor_adev_ret,
+					 const char **name_ret);
 
 int skl_int3472_register_clock(struct int3472_discrete_device *int3472);
 void skl_int3472_unregister_clock(struct int3472_discrete_device *int3472);
diff --git a/drivers/platform/x86/intel/int3472/discrete.c b/drivers/platform/x86/intel/int3472/discrete.c
index d2e8a87a077e..ff2bdbb8722c 100644
--- a/drivers/platform/x86/intel/int3472/discrete.c
+++ b/drivers/platform/x86/intel/int3472/discrete.c
@@ -363,19 +363,10 @@ static int skl_int3472_discrete_probe(struct platform_device *pdev)
 	int3472->dev = &pdev->dev;
 	platform_set_drvdata(pdev, int3472);
 
-	int3472->sensor = acpi_dev_get_first_consumer_dev(adev);
-	if (!int3472->sensor) {
-		dev_err(&pdev->dev, "INT3472 seems to have no dependents.\n");
-		return -ENODEV;
-	}
-
-	int3472->sensor_name = devm_kasprintf(int3472->dev, GFP_KERNEL,
-					      I2C_DEV_NAME_FORMAT,
-					      acpi_dev_name(int3472->sensor));
-	if (!int3472->sensor_name) {
-		ret = -ENOMEM;
-		goto err_put_sensor;
-	}
+	ret = skl_int3472_get_sensor_adev_and_name(&pdev->dev, &int3472->sensor,
+						   &int3472->sensor_name);
+	if (ret)
+		return ret;
 
 	/*
 	 * Initialising this list means we can call gpiod_remove_lookup_table()
@@ -390,11 +381,6 @@ static int skl_int3472_discrete_probe(struct platform_device *pdev)
 	}
 
 	return 0;
-
-err_put_sensor:
-	acpi_dev_put(int3472->sensor);
-
-	return ret;
 }
 
 static int skl_int3472_discrete_remove(struct platform_device *pdev)
-- 
2.33.1

[PATCH v6 10/15] platform/x86: int3472: Pass tps68470_clk_platform_data to the tps68470-regulator MFD-cell

[Hans de Goede]

Pass tps68470_clk_platform_data to the tps68470-clk MFD-cell,
so that sensors which use the TPS68470 can find their clock.

Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Changes in v5:
- Add TPS68470_WIN_MFD_CELL_COUNT define

Changes in v4:
- Update the comment about the cell ordering

Changes in v2:
- Put the GPIO cell last because acpi_gpiochip_add() calls
  acpi_dev_clear_dependencies() and the clk + regulators must
  be ready when this happens.
---
 drivers/platform/x86/intel/int3472/tps68470.c | 35 +++++++++++++++----
 1 file changed, 28 insertions(+), 7 deletions(-)

diff --git a/drivers/platform/x86/intel/int3472/tps68470.c b/drivers/platform/x86/intel/int3472/tps68470.c
index e95b0f50b384..05fcf35c1662 100644
--- a/drivers/platform/x86/intel/int3472/tps68470.c
+++ b/drivers/platform/x86/intel/int3472/tps68470.c
@@ -5,6 +5,7 @@
 #include <linux/mfd/core.h>
 #include <linux/mfd/tps68470.h>
 #include <linux/platform_device.h>
+#include <linux/platform_data/tps68470.h>
 #include <linux/regmap.h>
 
 #include "common.h"
@@ -12,17 +13,13 @@
 #define DESIGNED_FOR_CHROMEOS		1
 #define DESIGNED_FOR_WINDOWS		2
 
+#define TPS68470_WIN_MFD_CELL_COUNT	3
+
 static const struct mfd_cell tps68470_cros[] = {
 	{ .name = "tps68470-gpio" },
 	{ .name = "tps68470_pmic_opregion" },
 };
 
-static const struct mfd_cell tps68470_win[] = {
-	{ .name = "tps68470-gpio" },
-	{ .name = "tps68470-clk" },
-	{ .name = "tps68470-regulator" },
-};
-
 static const struct regmap_config tps68470_regmap_config = {
 	.reg_bits = 8,
 	.val_bits = 8,
@@ -105,10 +102,17 @@ static int skl_int3472_tps68470_calc_type(struct acpi_device *adev)
 static int skl_int3472_tps68470_probe(struct i2c_client *client)
 {
 	struct acpi_device *adev = ACPI_COMPANION(&client->dev);
+	struct tps68470_clk_platform_data clk_pdata = {};
+	struct mfd_cell *cells;
 	struct regmap *regmap;
 	int device_type;
 	int ret;
 
+	ret = skl_int3472_get_sensor_adev_and_name(&client->dev, NULL,
+						   &clk_pdata.consumer_dev_name);
+	if (ret)
+		return ret;
+
 	regmap = devm_regmap_init_i2c(client, &tps68470_regmap_config);
 	if (IS_ERR(regmap)) {
 		dev_err(&client->dev, "Failed to create regmap: %ld\n", PTR_ERR(regmap));
@@ -126,9 +130,26 @@ static int skl_int3472_tps68470_probe(struct i2c_client *client)
 	device_type = skl_int3472_tps68470_calc_type(adev);
 	switch (device_type) {
 	case DESIGNED_FOR_WINDOWS:
+		cells = kcalloc(TPS68470_WIN_MFD_CELL_COUNT, sizeof(*cells), GFP_KERNEL);
+		if (!cells)
+			return -ENOMEM;
+
+		/*
+		 * The order of the cells matters here! The clk must be first
+		 * because the regulator depends on it. The gpios must be last,
+		 * acpi_gpiochip_add() calls acpi_dev_clear_dependencies() and
+		 * the clk + regulators must be ready when this happens.
+		 */
+		cells[0].name = "tps68470-clk";
+		cells[0].platform_data = &clk_pdata;
+		cells[0].pdata_size = sizeof(clk_pdata);
+		cells[1].name = "tps68470-regulator";
+		cells[2].name = "tps68470-gpio";
+
 		ret = devm_mfd_add_devices(&client->dev, PLATFORM_DEVID_NONE,
-					   tps68470_win, ARRAY_SIZE(tps68470_win),
+					   cells, TPS68470_WIN_MFD_CELL_COUNT,
 					   NULL, 0, NULL);
+		kfree(cells);
 		break;
 	case DESIGNED_FOR_CHROMEOS:
 		ret = devm_mfd_add_devices(&client->dev, PLATFORM_DEVID_NONE,
-- 
2.33.1

[PATCH v6 11/15] platform/x86: int3472: Pass tps68470_regulator_platform_data to the tps68470-regulator MFD-cell

[Hans de Goede]

Pass tps68470_regulator_platform_data to the tps68470-regulator
MFD-cell, specifying the voltages of the various regulators and
tying the regulators to the sensor supplies so that sensors which use
the TPS68470 can find their regulators.

Since the voltages and supply connections are board-specific, this
introduces a DMI matches int3472_tps68470_board_data struct which
contains the necessary per-board info.

This per-board info also includes GPIO lookup information for the
sensor IO lines which may be connected to the tps68470 GPIOs.

Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Changes in v6:
- Also list the VCM module as VSIO regulator consumer
- Add constraints for VIO, it MUST have the same voltage configured as
  VSIO and mark it as always-on (it is always on there is no on/off
  control for it in the PMIC, as the PMIC uses it internally)
- Fix logic error in int3472_tps68470_get_board_data() (thank you Andy)

Changes in v5:
- Also pass regulator_init_data for the regulator used for the VCM
  (Voice Coil Motor) used for the focus of the back-camera lens
---
 drivers/platform/x86/intel/int3472/Makefile   |   2 +-
 drivers/platform/x86/intel/int3472/tps68470.c |  28 ++++
 drivers/platform/x86/intel/int3472/tps68470.h |  25 +++
 .../x86/intel/int3472/tps68470_board_data.c   | 145 ++++++++++++++++++
 4 files changed, 199 insertions(+), 1 deletion(-)
 create mode 100644 drivers/platform/x86/intel/int3472/tps68470.h
 create mode 100644 drivers/platform/x86/intel/int3472/tps68470_board_data.c

diff --git a/drivers/platform/x86/intel/int3472/Makefile b/drivers/platform/x86/intel/int3472/Makefile
index 771e720528a0..cfec7784c5c9 100644
--- a/drivers/platform/x86/intel/int3472/Makefile
+++ b/drivers/platform/x86/intel/int3472/Makefile
@@ -1,4 +1,4 @@
 obj-$(CONFIG_INTEL_SKL_INT3472)		+= intel_skl_int3472_discrete.o \
 					   intel_skl_int3472_tps68470.o
 intel_skl_int3472_discrete-y		:= discrete.o clk_and_regulator.o common.o
-intel_skl_int3472_tps68470-y		:= tps68470.o common.o
+intel_skl_int3472_tps68470-y		:= tps68470.o tps68470_board_data.o common.o
diff --git a/drivers/platform/x86/intel/int3472/tps68470.c b/drivers/platform/x86/intel/int3472/tps68470.c
index 05fcf35c1662..9c213b8e99a0 100644
--- a/drivers/platform/x86/intel/int3472/tps68470.c
+++ b/drivers/platform/x86/intel/int3472/tps68470.c
@@ -2,13 +2,16 @@
 /* Author: Dan Scally <djrscally@gmail.com> */
 
 #include <linux/i2c.h>
+#include <linux/kernel.h>
 #include <linux/mfd/core.h>
 #include <linux/mfd/tps68470.h>
 #include <linux/platform_device.h>
 #include <linux/platform_data/tps68470.h>
 #include <linux/regmap.h>
+#include <linux/string.h>
 
 #include "common.h"
+#include "tps68470.h"
 
 #define DESIGNED_FOR_CHROMEOS		1
 #define DESIGNED_FOR_WINDOWS		2
@@ -102,6 +105,7 @@ static int skl_int3472_tps68470_calc_type(struct acpi_device *adev)
 static int skl_int3472_tps68470_probe(struct i2c_client *client)
 {
 	struct acpi_device *adev = ACPI_COMPANION(&client->dev);
+	const struct int3472_tps68470_board_data *board_data;
 	struct tps68470_clk_platform_data clk_pdata = {};
 	struct mfd_cell *cells;
 	struct regmap *regmap;
@@ -130,6 +134,10 @@ static int skl_int3472_tps68470_probe(struct i2c_client *client)
 	device_type = skl_int3472_tps68470_calc_type(adev);
 	switch (device_type) {
 	case DESIGNED_FOR_WINDOWS:
+		board_data = int3472_tps68470_get_board_data(dev_name(&client->dev));
+		if (!board_data)
+			return dev_err_probe(&client->dev, -ENODEV, "No board-data found for this model\n");
+
 		cells = kcalloc(TPS68470_WIN_MFD_CELL_COUNT, sizeof(*cells), GFP_KERNEL);
 		if (!cells)
 			return -ENOMEM;
@@ -144,12 +152,20 @@ static int skl_int3472_tps68470_probe(struct i2c_client *client)
 		cells[0].platform_data = &clk_pdata;
 		cells[0].pdata_size = sizeof(clk_pdata);
 		cells[1].name = "tps68470-regulator";
+		cells[1].platform_data = (void *)board_data->tps68470_regulator_pdata;
+		cells[1].pdata_size = sizeof(struct tps68470_regulator_platform_data);
 		cells[2].name = "tps68470-gpio";
 
+		gpiod_add_lookup_table(board_data->tps68470_gpio_lookup_table);
+
 		ret = devm_mfd_add_devices(&client->dev, PLATFORM_DEVID_NONE,
 					   cells, TPS68470_WIN_MFD_CELL_COUNT,
 					   NULL, 0, NULL);
 		kfree(cells);
+
+		if (ret)
+			gpiod_remove_lookup_table(board_data->tps68470_gpio_lookup_table);
+
 		break;
 	case DESIGNED_FOR_CHROMEOS:
 		ret = devm_mfd_add_devices(&client->dev, PLATFORM_DEVID_NONE,
@@ -164,6 +180,17 @@ static int skl_int3472_tps68470_probe(struct i2c_client *client)
 	return ret;
 }
 
+static int skl_int3472_tps68470_remove(struct i2c_client *client)
+{
+	const struct int3472_tps68470_board_data *board_data;
+
+	board_data = int3472_tps68470_get_board_data(dev_name(&client->dev));
+	if (board_data)
+		gpiod_remove_lookup_table(board_data->tps68470_gpio_lookup_table);
+
+	return 0;
+}
+
 static const struct acpi_device_id int3472_device_id[] = {
 	{ "INT3472", 0 },
 	{ }
@@ -176,6 +203,7 @@ static struct i2c_driver int3472_tps68470 = {
 		.acpi_match_table = int3472_device_id,
 	},
 	.probe_new = skl_int3472_tps68470_probe,
+	.remove = skl_int3472_tps68470_remove,
 };
 module_i2c_driver(int3472_tps68470);
 
diff --git a/drivers/platform/x86/intel/int3472/tps68470.h b/drivers/platform/x86/intel/int3472/tps68470.h
new file mode 100644
index 000000000000..cfd33eb62740
--- /dev/null
+++ b/drivers/platform/x86/intel/int3472/tps68470.h
@@ -0,0 +1,25 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * TI TPS68470 PMIC platform data definition.
+ *
+ * Copyright (c) 2021 Red Hat Inc.
+ *
+ * Red Hat authors:
+ * Hans de Goede <hdegoede@redhat.com>
+ */
+
+#ifndef _INTEL_SKL_INT3472_TPS68470_H
+#define _INTEL_SKL_INT3472_TPS68470_H
+
+struct gpiod_lookup_table;
+struct tps68470_regulator_platform_data;
+
+struct int3472_tps68470_board_data {
+	const char *dev_name;
+	struct gpiod_lookup_table *tps68470_gpio_lookup_table;
+	const struct tps68470_regulator_platform_data *tps68470_regulator_pdata;
+};
+
+const struct int3472_tps68470_board_data *int3472_tps68470_get_board_data(const char *dev_name);
+
+#endif
diff --git a/drivers/platform/x86/intel/int3472/tps68470_board_data.c b/drivers/platform/x86/intel/int3472/tps68470_board_data.c
new file mode 100644
index 000000000000..faa5570f6e6b
--- /dev/null
+++ b/drivers/platform/x86/intel/int3472/tps68470_board_data.c
@@ -0,0 +1,145 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * TI TPS68470 PMIC platform data definition.
+ *
+ * Copyright (c) 2021 Dan Scally <djrscally@gmail.com>
+ * Copyright (c) 2021 Red Hat Inc.
+ *
+ * Red Hat authors:
+ * Hans de Goede <hdegoede@redhat.com>
+ */
+
+#include <linux/dmi.h>
+#include <linux/gpio/machine.h>
+#include <linux/platform_data/tps68470.h>
+#include <linux/regulator/machine.h>
+#include "tps68470.h"
+
+static struct regulator_consumer_supply int347a_core_consumer_supplies[] = {
+	REGULATOR_SUPPLY("dvdd", "i2c-INT347A:00"),
+};
+
+static struct regulator_consumer_supply int347a_ana_consumer_supplies[] = {
+	REGULATOR_SUPPLY("avdd", "i2c-INT347A:00"),
+};
+
+static struct regulator_consumer_supply int347a_vcm_consumer_supplies[] = {
+	REGULATOR_SUPPLY("vdd", "i2c-INT347A:00-VCM"),
+};
+
+static struct regulator_consumer_supply int347a_vsio_consumer_supplies[] = {
+	REGULATOR_SUPPLY("dovdd", "i2c-INT347A:00"),
+	REGULATOR_SUPPLY("vsio", "i2c-INT347A:00-VCM"),
+};
+
+static const struct regulator_init_data surface_go_tps68470_core_reg_init_data = {
+	.constraints = {
+		.min_uV = 1200000,
+		.max_uV = 1200000,
+		.apply_uV = true,
+		.valid_ops_mask = REGULATOR_CHANGE_STATUS,
+	},
+	.num_consumer_supplies = ARRAY_SIZE(int347a_core_consumer_supplies),
+	.consumer_supplies = int347a_core_consumer_supplies,
+};
+
+static const struct regulator_init_data surface_go_tps68470_ana_reg_init_data = {
+	.constraints = {
+		.min_uV = 2815200,
+		.max_uV = 2815200,
+		.apply_uV = true,
+		.valid_ops_mask = REGULATOR_CHANGE_STATUS,
+	},
+	.num_consumer_supplies = ARRAY_SIZE(int347a_ana_consumer_supplies),
+	.consumer_supplies = int347a_ana_consumer_supplies,
+};
+
+static const struct regulator_init_data surface_go_tps68470_vcm_reg_init_data = {
+	.constraints = {
+		.min_uV = 2815200,
+		.max_uV = 2815200,
+		.apply_uV = true,
+		.valid_ops_mask = REGULATOR_CHANGE_STATUS,
+	},
+	.num_consumer_supplies = ARRAY_SIZE(int347a_vcm_consumer_supplies),
+	.consumer_supplies = int347a_vcm_consumer_supplies,
+};
+
+/* Ensure the always-on VIO regulator has the same voltage as VSIO */
+static const struct regulator_init_data surface_go_tps68470_vio_reg_init_data = {
+	.constraints = {
+		.min_uV = 1800600,
+		.max_uV = 1800600,
+		.apply_uV = true,
+		.always_on = true,
+	},
+};
+
+static const struct regulator_init_data surface_go_tps68470_vsio_reg_init_data = {
+	.constraints = {
+		.min_uV = 1800600,
+		.max_uV = 1800600,
+		.apply_uV = true,
+		.valid_ops_mask = REGULATOR_CHANGE_STATUS,
+	},
+	.num_consumer_supplies = ARRAY_SIZE(int347a_vsio_consumer_supplies),
+	.consumer_supplies = int347a_vsio_consumer_supplies,
+};
+
+static const struct tps68470_regulator_platform_data surface_go_tps68470_pdata = {
+	.reg_init_data = {
+		[TPS68470_CORE] = &surface_go_tps68470_core_reg_init_data,
+		[TPS68470_ANA]  = &surface_go_tps68470_ana_reg_init_data,
+		[TPS68470_VCM]  = &surface_go_tps68470_vcm_reg_init_data,
+		[TPS68470_VIO] = &surface_go_tps68470_vio_reg_init_data,
+		[TPS68470_VSIO] = &surface_go_tps68470_vsio_reg_init_data,
+	},
+};
+
+static struct gpiod_lookup_table surface_go_tps68470_gpios = {
+	.dev_id = "i2c-INT347A:00",
+	.table = {
+		GPIO_LOOKUP("tps68470-gpio", 9, "reset", GPIO_ACTIVE_LOW),
+		GPIO_LOOKUP("tps68470-gpio", 7, "powerdown", GPIO_ACTIVE_LOW)
+	}
+};
+
+static const struct int3472_tps68470_board_data surface_go_tps68470_board_data = {
+	.dev_name = "i2c-INT3472:05",
+	.tps68470_gpio_lookup_table = &surface_go_tps68470_gpios,
+	.tps68470_regulator_pdata = &surface_go_tps68470_pdata,
+};
+
+static const struct dmi_system_id int3472_tps68470_board_data_table[] = {
+	{
+		.matches = {
+			DMI_EXACT_MATCH(DMI_SYS_VENDOR, "Microsoft Corporation"),
+			DMI_EXACT_MATCH(DMI_PRODUCT_NAME, "Surface Go"),
+		},
+		.driver_data = (void *)&surface_go_tps68470_board_data,
+	},
+	{
+		.matches = {
+			DMI_EXACT_MATCH(DMI_SYS_VENDOR, "Microsoft Corporation"),
+			DMI_EXACT_MATCH(DMI_PRODUCT_NAME, "Surface Go 2"),
+		},
+		.driver_data = (void *)&surface_go_tps68470_board_data,
+	},
+	{ }
+};
+
+const struct int3472_tps68470_board_data *int3472_tps68470_get_board_data(const char *dev_name)
+{
+	const struct int3472_tps68470_board_data *board_data;
+	const struct dmi_system_id *match;
+
+	for (match = dmi_first_match(int3472_tps68470_board_data_table);
+	     match;
+	     match = dmi_first_match(match + 1)) {
+		board_data = match->driver_data;
+		if (strcmp(board_data->dev_name, dev_name) == 0)
+			return board_data;
+	}
+
+	return NULL;
+}
-- 
2.33.1

[PATCH v6 12/15] platform/x86: int3472: Deal with probe ordering issues

[Hans de Goede]

The clk and regulator frameworks expect clk/regulator consumer-devices
to have info about the consumed clks/regulators described in the device's
fw_node.

To work around this info missing from the ACPI tables on devices where
the int3472 driver is used, the int3472 MFD-cell drivers attach info about
consumers to the clks/regulators when registering these.

This causes problems with the probe ordering wrt drivers for consumers
of these clks/regulators. Since the lookups are only registered when the
provider-driver binds, trying to get these clks/regulators before then
results in a -ENOENT error for clks and a dummy regulator for regulators.

All the sensor ACPI fw-nodes have a _DEP dependency on the INT3472 ACPI
fw-node, so to work around these probe ordering issues the ACPI core /
i2c-code does not instantiate the I2C-clients for any ACPI devices
which have a _DEP dependency on an INT3472 ACPI device until all
_DEP-s are met.

This relies on acpi_dev_clear_dependencies() getting called by the driver
for the _DEP-s when they are ready, add a acpi_dev_clear_dependencies()
call to the discrete.c probe code.

In the tps68470 case calling acpi_dev_clear_dependencies() is already done
by the acpi_gpiochip_add() call done by the driver for the GPIO MFD cell
(The GPIO cell is deliberately the last cell created to make sure the
clk + regulator cells are already instantiated when this happens).

However for proper probe ordering, the clk/regulator cells must not just
be instantiated the must be fully ready (the clks + regulators must be
registered with their subsystems).

Add MODULE_SOFTDEP dependencies for the clk and regulator drivers for
the instantiated MFD-cells so that these are loaded before us and so
that they bind immediately when the platform-devs are instantiated.

Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Changes in v2:
- Only call acpi_dev_clear_dependencies() in the discrete.c case, for the
  tps68470 case this is already done by the acpi_gpiochip_add() for the
  GPIO MFD cell.
---
 drivers/platform/x86/intel/int3472/discrete.c | 1 +
 drivers/platform/x86/intel/int3472/tps68470.c | 6 ++++++
 2 files changed, 7 insertions(+)

diff --git a/drivers/platform/x86/intel/int3472/discrete.c b/drivers/platform/x86/intel/int3472/discrete.c
index ff2bdbb8722c..5b514fa01a97 100644
--- a/drivers/platform/x86/intel/int3472/discrete.c
+++ b/drivers/platform/x86/intel/int3472/discrete.c
@@ -380,6 +380,7 @@ static int skl_int3472_discrete_probe(struct platform_device *pdev)
 		return ret;
 	}
 
+	acpi_dev_clear_dependencies(adev);
 	return 0;
 }
 
diff --git a/drivers/platform/x86/intel/int3472/tps68470.c b/drivers/platform/x86/intel/int3472/tps68470.c
index 9c213b8e99a0..e1de1ff40bba 100644
--- a/drivers/platform/x86/intel/int3472/tps68470.c
+++ b/drivers/platform/x86/intel/int3472/tps68470.c
@@ -177,6 +177,11 @@ static int skl_int3472_tps68470_probe(struct i2c_client *client)
 		return device_type;
 	}
 
+	/*
+	 * No acpi_dev_clear_dependencies() here, since the acpi_gpiochip_add()
+	 * for the GPIO cell already does this.
+	 */
+
 	return ret;
 }
 
@@ -210,3 +215,4 @@ 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_SOFTDEP("pre: clk-tps68470 tps68470-regulator");
-- 
2.33.1

[PATCH v6 13/15] media: ipu3-cio2: Defer probing until the PMIC is fully setup

[Hans de Goede]

On devices where things are not fully describe in devicetree (1)
and where the code thus falls back to calling cio2_bridge_init(),
the i2c-clients for any VCMs also need to be instantiated manually.

The VCM can be probed by its driver as soon as the code instantiates
the i2c-client and this probing must not happen before the PMIC is
fully setup.

Make cio2_bridge_init() return -EPROBE_DEFER when the PMIC is not
fully-setup, deferring the probe of the ipu3-cio2 driver.

This is a preparation patch for adding VCM enumeration support to
the ipu3-cio2-bridge code.

1) Through embedding of devicetree info in the ACPI tables

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/media/pci/intel/ipu3/cio2-bridge.c | 37 ++++++++++++++++++++++
 1 file changed, 37 insertions(+)

diff --git a/drivers/media/pci/intel/ipu3/cio2-bridge.c b/drivers/media/pci/intel/ipu3/cio2-bridge.c
index 1cbbcbf4e157..460ef7f78234 100644
--- a/drivers/media/pci/intel/ipu3/cio2-bridge.c
+++ b/drivers/media/pci/intel/ipu3/cio2-bridge.c
@@ -308,6 +308,40 @@ static int cio2_bridge_connect_sensors(struct cio2_bridge *bridge,
 	return ret;
 }
 
+/*
+ * The VCM cannot be probed until the PMIC is completely setup. We cannot rely
+ * on -EPROBE_DEFER for this, since the consumer<->supplier relations between
+ * the VCM and regulators/clks are not described in ACPI, instead they are
+ * passed as board-data to the PMIC drivers. Since -PROBE_DEFER does not work
+ * for the clks/regulators the VCM i2c-clients must not be instantiated until
+ * the PMIC is fully setup.
+ *
+ * The sensor/VCM ACPI device has an ACPI _DEP on the PMIC, check this using the
+ * acpi_dev_ready_for_enumeration() helper, like the i2c-core-acpi code does
+ * for the sensors.
+ */
+int cio2_bridge_sensors_are_ready(void)
+{
+	struct acpi_device *adev;
+	bool ready = true;
+	unsigned int i;
+
+	for (i = 0; i < ARRAY_SIZE(cio2_supported_sensors); i++) {
+		const struct cio2_sensor_config *cfg =
+			&cio2_supported_sensors[i];
+
+		for_each_acpi_dev_match(adev, cfg->hid, NULL, -1) {
+			if (!adev->status.enabled)
+				continue;
+
+			if (!acpi_dev_ready_for_enumeration(adev))
+				ready = false;
+		}
+	}
+
+	return ready;
+}
+
 int cio2_bridge_init(struct pci_dev *cio2)
 {
 	struct device *dev = &cio2->dev;
@@ -316,6 +350,9 @@ int cio2_bridge_init(struct pci_dev *cio2)
 	unsigned int i;
 	int ret;
 
+	if (!cio2_bridge_sensors_are_ready())
+		return -EPROBE_DEFER;
+
 	bridge = kzalloc(sizeof(*bridge), GFP_KERNEL);
 	if (!bridge)
 		return -ENOMEM;
-- 
2.33.1

[PATCH v6 14/15] media: ipu3-cio2: Call cio2_bridge_init() before anything else

[Hans de Goede]

Since cio2_bridge_init() may now return -EPROBE_DEFER it is best to
call it before anything else.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/media/pci/intel/ipu3/ipu3-cio2-main.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/media/pci/intel/ipu3/ipu3-cio2-main.c b/drivers/media/pci/intel/ipu3/ipu3-cio2-main.c
index e2874fee9530..0e9b0503b62a 100644
--- a/drivers/media/pci/intel/ipu3/ipu3-cio2-main.c
+++ b/drivers/media/pci/intel/ipu3/ipu3-cio2-main.c
@@ -1713,11 +1713,6 @@ static int cio2_pci_probe(struct pci_dev *pci_dev,
 	struct cio2_device *cio2;
 	int r;
 
-	cio2 = devm_kzalloc(dev, sizeof(*cio2), GFP_KERNEL);
-	if (!cio2)
-		return -ENOMEM;
-	cio2->pci_dev = pci_dev;
-
 	/*
 	 * On some platforms no connections to sensors are defined in firmware,
 	 * if the device has no endpoints then we can try to build those as
@@ -1735,6 +1730,11 @@ static int cio2_pci_probe(struct pci_dev *pci_dev,
 			return r;
 	}
 
+	cio2 = devm_kzalloc(dev, sizeof(*cio2), GFP_KERNEL);
+	if (!cio2)
+		return -ENOMEM;
+	cio2->pci_dev = pci_dev;
+
 	r = pcim_enable_device(pci_dev);
 	if (r) {
 		dev_err(dev, "failed to enable device (%d)\n", r);
-- 
2.33.1

[PATCH v6 15/15] media: ipu3-cio2: Add support for instantiating i2c-clients for VCMs

[Hans de Goede]

Some sensors come with a variable-focus lens where the lens focus is
controller by a VCM (Voice Coil Motor). If there is a VCM for the
lens-focus, and if so which one, is described on the vcm_type field
of the ACPI SSDB table.

These VCMs are a second I2C device listed as an extra I2cSerialBusV2
resource in the same ACPI device as the sensor. The i2c-core-acpi.c
code only instantiates an i2c-client for the first I2cSerialBusV2
resource.

Add support for instantiating an i2c-client for the VCM with
the type of the i2c-client set based on the SSDB vcm_type field.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/media/pci/intel/ipu3/cio2-bridge.c | 55 ++++++++++++++++++++++
 drivers/media/pci/intel/ipu3/cio2-bridge.h | 16 ++++++-
 2 files changed, 69 insertions(+), 2 deletions(-)

diff --git a/drivers/media/pci/intel/ipu3/cio2-bridge.c b/drivers/media/pci/intel/ipu3/cio2-bridge.c
index 460ef7f78234..9b02f5599082 100644
--- a/drivers/media/pci/intel/ipu3/cio2-bridge.c
+++ b/drivers/media/pci/intel/ipu3/cio2-bridge.c
@@ -3,6 +3,7 @@
 
 #include <linux/acpi.h>
 #include <linux/device.h>
+#include <linux/i2c.h>
 #include <linux/pci.h>
 #include <linux/property.h>
 #include <media/v4l2-fwnode.h>
@@ -38,6 +39,18 @@ static const struct cio2_property_names prop_names = {
 	.link_frequencies = "link-frequencies",
 };
 
+static const char * const cio2_vcm_types[] = {
+	"ad5823",
+	"dw9714",
+	"ad5816",
+	"dw9719",
+	"dw9718",
+	"dw9806b",
+	"wv517s",
+	"lc898122xa",
+	"lc898212axb",
+};
+
 static int cio2_bridge_read_acpi_buffer(struct acpi_device *adev, char *id,
 					void *data, u32 size)
 {
@@ -134,6 +147,12 @@ static void cio2_bridge_create_fwnode_properties(
 	sensor->dev_properties[2] = PROPERTY_ENTRY_U32(
 					sensor->prop_names.orientation,
 					orientation);
+	if (sensor->ssdb.vcmtype) {
+		sensor->vcm_ref[0] =
+			SOFTWARE_NODE_REFERENCE(&sensor->swnodes[SWNODE_VCM]);
+		sensor->dev_properties[3] =
+			PROPERTY_ENTRY_REF_ARRAY("lens-focus", sensor->vcm_ref);
+	}
 
 	sensor->ep_properties[0] = PROPERTY_ENTRY_U32(
 					sensor->prop_names.bus_type,
@@ -195,6 +214,33 @@ static void cio2_bridge_create_connection_swnodes(struct cio2_bridge *bridge,
 						sensor->node_names.endpoint,
 						&nodes[SWNODE_CIO2_PORT],
 						sensor->cio2_properties);
+	if (sensor->ssdb.vcmtype)
+		nodes[SWNODE_VCM] =
+			NODE_VCM(cio2_vcm_types[sensor->ssdb.vcmtype - 1]);
+}
+
+static void cio2_bridge_instantiate_vcm_i2c_client(struct cio2_sensor *sensor)
+{
+	struct i2c_board_info board_info = { };
+	char name[16];
+
+	if (!sensor->ssdb.vcmtype)
+		return;
+
+	snprintf(name, sizeof(name), "%s-VCM", acpi_dev_name(sensor->adev));
+	board_info.dev_name = name;
+	strscpy(board_info.type, cio2_vcm_types[sensor->ssdb.vcmtype - 1],
+		ARRAY_SIZE(board_info.type));
+	board_info.swnode = &sensor->swnodes[SWNODE_VCM];
+
+	sensor->vcm_i2c_client =
+		i2c_acpi_new_device_by_fwnode(acpi_fwnode_handle(sensor->adev),
+					      1, &board_info);
+	if (IS_ERR(sensor->vcm_i2c_client)) {
+		dev_warn(&sensor->adev->dev, "Error instantiation VCM i2c-client: %ld\n",
+			 PTR_ERR(sensor->vcm_i2c_client));
+		sensor->vcm_i2c_client = NULL;
+	}
 }
 
 static void cio2_bridge_unregister_sensors(struct cio2_bridge *bridge)
@@ -207,6 +253,7 @@ static void cio2_bridge_unregister_sensors(struct cio2_bridge *bridge)
 		software_node_unregister_nodes(sensor->swnodes);
 		ACPI_FREE(sensor->pld);
 		acpi_dev_put(sensor->adev);
+		i2c_unregister_device(sensor->vcm_i2c_client);
 	}
 }
 
@@ -239,6 +286,12 @@ static int cio2_bridge_connect_sensor(const struct cio2_sensor_config *cfg,
 		if (ret)
 			goto err_put_adev;
 
+		if (sensor->ssdb.vcmtype > ARRAY_SIZE(cio2_vcm_types)) {
+			dev_warn(&adev->dev, "Unknown VCM type %d\n",
+				 sensor->ssdb.vcmtype);
+			sensor->ssdb.vcmtype = 0;
+		}
+
 		status = acpi_get_physical_device_location(adev->handle, &sensor->pld);
 		if (ACPI_FAILURE(status)) {
 			ret = -ENODEV;
@@ -269,6 +322,8 @@ static int cio2_bridge_connect_sensor(const struct cio2_sensor_config *cfg,
 		sensor->adev = acpi_dev_get(adev);
 		adev->fwnode.secondary = fwnode;
 
+		cio2_bridge_instantiate_vcm_i2c_client(sensor);
+
 		dev_info(&cio2->dev, "Found supported sensor %s\n",
 			 acpi_dev_name(adev));
 
diff --git a/drivers/media/pci/intel/ipu3/cio2-bridge.h b/drivers/media/pci/intel/ipu3/cio2-bridge.h
index 202c7d494f7a..4418cbd08208 100644
--- a/drivers/media/pci/intel/ipu3/cio2-bridge.h
+++ b/drivers/media/pci/intel/ipu3/cio2-bridge.h
@@ -8,6 +8,8 @@
 
 #include "ipu3-cio2.h"
 
+struct i2c_client;
+
 #define CIO2_HID				"INT343E"
 #define CIO2_MAX_LANES				4
 #define MAX_NUM_LINK_FREQS			3
@@ -42,12 +44,19 @@
 		.properties = _PROPS,		\
 	}
 
+#define NODE_VCM(_TYPE)				\
+	(const struct software_node) {		\
+		.name = _TYPE,			\
+	}
+
 enum cio2_sensor_swnodes {
 	SWNODE_SENSOR_HID,
 	SWNODE_SENSOR_PORT,
 	SWNODE_SENSOR_ENDPOINT,
 	SWNODE_CIO2_PORT,
 	SWNODE_CIO2_ENDPOINT,
+	/* Must be last because it is optional / maybe empty */
+	SWNODE_VCM,
 	SWNODE_COUNT
 };
 
@@ -106,8 +115,10 @@ struct cio2_sensor_config {
 struct cio2_sensor {
 	char name[ACPI_ID_LEN];
 	struct acpi_device *adev;
+	struct i2c_client *vcm_i2c_client;
 
-	struct software_node swnodes[6];
+	/* SWNODE_COUNT + 1 for terminating empty node */
+	struct software_node swnodes[SWNODE_COUNT + 1];
 	struct cio2_node_names node_names;
 
 	struct cio2_sensor_ssdb ssdb;
@@ -115,10 +126,11 @@ struct cio2_sensor {
 
 	struct cio2_property_names prop_names;
 	struct property_entry ep_properties[5];
-	struct property_entry dev_properties[4];
+	struct property_entry dev_properties[5];
 	struct property_entry cio2_properties[3];
 	struct software_node_ref_args local_ref[1];
 	struct software_node_ref_args remote_ref[1];
+	struct software_node_ref_args vcm_ref[1];
 };
 
 struct cio2_bridge {
-- 
2.33.1