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

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

[Hans de Goede]

Hi All,

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

v3 of this patch-set further reworks how to defer the binding of the
camera-sensor drivers till all clk/regulator/gpio consumer/lookup info has
been registered. See the new patch 1 + 2 (replacing v1 patch 1-3 /
v2 patch 1-2).

I'm quite happy with how this works now, so from my pov this is the final
version of the device-instantiation deferral code / approach.

###

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.

Assuming this series is acceptable to everyone, we need to talk about how
to merge this.

Assuming the i2c-core-acpi.c are ok with it patches 1 + 2 can both be
merged into linux-pm by Rafael, independent of the rest of the series
(there are some runtime deps on other changes for everything to work,
but the camera-sensors impacted by this are not fully supported yet in
the mainline kernel anyways).

For "[PATCH 03/13] platform_data: Add linux/platform_data/tps68470.h file",
which all further patches depend on I plan to provide an immutable branch
myself (once it has been reviewed), which the clk / regulator maintainers
can then merge before merging the clk / regulator driver which depends on
this.

And I will merge that IM-branch + patches 6-11 into the pdx86 tree myself.

Regards,

Hans


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

Hans de Goede (10):
  ACPI: delay enumeration of devices with a _DEP pointing to an INT3472
    device
  i2c: acpi: Use acpi_dev_ready_for_enumeration() helper
  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

 drivers/acpi/scan.c                           |  36 ++-
 drivers/clk/Kconfig                           |   6 +
 drivers/clk/Makefile                          |   1 +
 drivers/clk/clk-tps68470.c                    | 256 ++++++++++++++++++
 drivers/i2c/i2c-core-acpi.c                   |   5 +-
 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} |  97 ++++++-
 drivers/platform/x86/intel/int3472/tps68470.h |  25 ++
 .../x86/intel/int3472/tps68470_board_data.c   | 118 ++++++++
 drivers/regulator/Kconfig                     |   9 +
 drivers/regulator/Makefile                    |   1 +
 drivers/regulator/tps68470-regulator.c        | 193 +++++++++++++
 include/acpi/acpi_bus.h                       |   5 +-
 include/linux/mfd/tps68470.h                  |  11 +
 include/linux/platform_data/tps68470.h        |  35 +++
 20 files changed, 904 insertions(+), 150 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} (55%)
 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.31.1

[PATCH v3 01/11] 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.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/acpi/scan.c     | 36 ++++++++++++++++++++++++++++++++++--
 include/acpi/acpi_bus.h |  5 ++++-
 2 files changed, 38 insertions(+), 3 deletions(-)

diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
index 5b54c80b9d32..efee6ee91c8f 100644
--- a/drivers/acpi/scan.c
+++ b/drivers/acpi/scan.c
@@ -796,6 +796,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;
@@ -1757,8 +1763,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++;
+		}
 	}
 }
 
@@ -1962,7 +1972,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)) {
@@ -1971,6 +1981,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)
@@ -1984,6 +1995,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);
@@ -2071,6 +2083,9 @@ static acpi_status acpi_bus_check_add_2(acpi_handle handle, u32 lvl_not_used,
 
 static void acpi_default_enumeration(struct acpi_device *device)
 {
+	if (!acpi_dev_ready_for_enumeration(device))
+		return;
+
 	/*
 	 * Do not enumerate devices with enumeration_by_parent flag set as
 	 * they will be enumerated by their respective parents.
@@ -2313,6 +2328,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 13d93371790e..2da53b7b4965 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 */
@@ -284,6 +285,7 @@ struct acpi_dep_data {
 	struct list_head node;
 	acpi_handle supplier;
 	acpi_handle consumer;
+	bool honor_dep;
 };
 
 /* Performance Management */
@@ -693,6 +695,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.31.1

[PATCH v3 02/11] 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.

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 aaeeacc12121..688cc71d650d 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.31.1

[PATCH v3 03/11] 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.

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.31.1

[PATCH v3 04/11] 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.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
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 | 193 +++++++++++++++++++++++++
 3 files changed, 203 insertions(+)
 create mode 100644 drivers/regulator/tps68470-regulator.c

diff --git a/drivers/regulator/Kconfig b/drivers/regulator/Kconfig
index 4fd13b06231f..d107af5bff6c 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 TPS68370 PMIC Regulators Driver"
+	depends on INTEL_SKL_INT3472
+	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_TPS80031
 	tristate "TI TPS80031/TPS80032 power regulator driver"
 	depends on MFD_TPS80031
diff --git a/drivers/regulator/Makefile b/drivers/regulator/Makefile
index 9e382b50a5ef..03c318110986 100644
--- a/drivers/regulator/Makefile
+++ b/drivers/regulator/Makefile
@@ -158,6 +158,7 @@ obj-$(CONFIG_REGULATOR_TPS6524X) += tps6524x-regulator.o
 obj-$(CONFIG_REGULATOR_TPS6586X) += tps6586x-regulator.o
 obj-$(CONFIG_REGULATOR_TPS65910) += tps65910-regulator.o
 obj-$(CONFIG_REGULATOR_TPS65912) += tps65912-regulator.o
+obj-$(CONFIG_REGULATOR_TPS68470) += tps68470-regulator.o
 obj-$(CONFIG_REGULATOR_TPS80031) += tps80031-regulator.o
 obj-$(CONFIG_REGULATOR_TPS65132) += tps65132-regulator.o
 obj-$(CONFIG_REGULATOR_TWL4030) += twl-regulator.o twl6030-regulator.o
diff --git a/drivers/regulator/tps68470-regulator.c b/drivers/regulator/tps68470-regulator.c
new file mode 100644
index 000000000000..3129fa13a122
--- /dev/null
+++ b/drivers/regulator/tps68470-regulator.c
@@ -0,0 +1,193 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Regulator driver for TPS68470 PMIC
+ *
+ * Copyright (C) 2018 Intel Corporation
+ *
+ * Authors:
+ *	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/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>
+
+#define TPS68470_REGULATOR(_name, _id, _ops, _n, _vr,			\
+			   _vm, _er, _em, _t, _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,				\
+		.volt_table		= _t,				\
+		.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),
+};
+
+/* Operations permitted on DCDCx, LDO2, LDO3 and LDO4 */
+static const struct regulator_ops tps68470_regulator_ops = {
+	.is_enabled		= regulator_is_enabled_regmap,
+	.enable			= regulator_enable_regmap,
+	.disable		= regulator_disable_regmap,
+	.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,
+			   NULL, 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,
+			   NULL, 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,
+			   NULL, tps68470_ldo_ranges,
+			   ARRAY_SIZE(tps68470_ldo_ranges)),
+	TPS68470_REGULATOR(VIO, TPS68470_VIO,
+			   tps68470_regulator_ops, 126, TPS68470_REG_VIOVAL,
+			   TPS68470_VIOVAL_IOVOLT_MASK, TPS68470_REG_S_I2C_CTL,
+			   TPS68470_S_I2C_CTL_EN_MASK,
+			   NULL, tps68470_ldo_ranges,
+			   ARRAY_SIZE(tps68470_ldo_ranges)),
+
+/*
+ * (1) This register must have same setting as VIOVAL if S_IO LDO is used to
+ *     power daisy chained IOs in the receive side.
+ * (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,
+			   NULL, 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,
+			   NULL, 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,
+			   NULL, tps68470_ldo_ranges,
+			   ARRAY_SIZE(tps68470_ldo_ranges)),
+};
+
+#define TPS68470_REG_INIT_DATA(_name, _min_uV, _max_uV)			\
+	[TPS68470_ ## _name] = {					\
+		.constraints = {					\
+			.name = # _name,				\
+			.valid_ops_mask = REGULATOR_CHANGE_VOLTAGE |	\
+					  REGULATOR_CHANGE_STATUS,	\
+			.min_uV = _min_uV,				\
+			.max_uV = _max_uV,				\
+		},							\
+	}
+
+struct regulator_init_data tps68470_init[] = {
+	TPS68470_REG_INIT_DATA(CORE, 900000, 1950000),
+	TPS68470_REG_INIT_DATA(ANA, 875000, 3100000),
+	TPS68470_REG_INIT_DATA(VCM, 875000, 3100000),
+	TPS68470_REG_INIT_DATA(VIO, 875000, 3100000),
+	TPS68470_REG_INIT_DATA(VSIO, 875000, 3100000),
+	TPS68470_REG_INIT_DATA(AUX1, 875000, 3100000),
+	TPS68470_REG_INIT_DATA(AUX2, 875000, 3100000),
+};
+
+static int tps68470_regulator_probe(struct platform_device *pdev)
+{
+	struct tps68470_regulator_platform_data *pdata = pdev->dev.platform_data;
+	struct regulator_config config = { };
+	struct regmap *tps68470_regmap;
+	struct regulator_dev *rdev;
+	int i;
+
+	tps68470_regmap = dev_get_drvdata(pdev->dev.parent);
+
+	for (i = 0; i < TPS68470_NUM_REGULATORS; i++) {
+		config.dev = pdev->dev.parent;
+		config.regmap = tps68470_regmap;
+		if (pdata && pdata->reg_init_data[i])
+			config.init_data = pdata->reg_init_data[i];
+		else
+			config.init_data = &tps68470_init[i];
+
+		rdev = devm_regulator_register(&pdev->dev, &regulators[i], &config);
+		if (IS_ERR(rdev)) {
+			dev_err(&pdev->dev, "failed to register %s regulator\n",
+				regulators[i].name);
+			return PTR_ERR(rdev);
+		}
+	}
+
+	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.31.1

[PATCH v3 05/11] 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.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
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          |   6 +
 drivers/clk/Makefile         |   1 +
 drivers/clk/clk-tps68470.c   | 256 +++++++++++++++++++++++++++++++++++
 include/linux/mfd/tps68470.h |  11 ++
 4 files changed, 274 insertions(+)
 create mode 100644 drivers/clk/clk-tps68470.c

diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig
index c5b3dc97396a..7dffecac83d1 100644
--- a/drivers/clk/Kconfig
+++ b/drivers/clk/Kconfig
@@ -169,6 +169,12 @@ 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 && REGMAP_I2C && INTEL_SKL_INT3472
+	help
+	 This driver supports the clocks provided by TPS68470
+
 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..27e8cbd0f60e
--- /dev/null
+++ b/drivers/clk/clk-tps68470.c
@@ -0,0 +1,256 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Clock driver for TPS68470 PMIC
+ *
+ * Copyright (C) 2018 Intel Corporation
+ *
+ * Authors:
+ *	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)
+
+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;
+	struct clk *clk;
+	int clk_cfg_idx;
+};
+
+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);
+	int idx = clkdata->clk_cfg_idx;
+
+	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);
+
+	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);
+
+	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);
+
+	/* write hw defaults */
+	regmap_write(clkdata->regmap, TPS68470_REG_BOOSTDIV, 0);
+	regmap_write(clkdata->regmap, TPS68470_REG_BUCKDIV, 0);
+	regmap_write(clkdata->regmap, TPS68470_REG_PLLSWR, 0);
+	regmap_write(clkdata->regmap, TPS68470_REG_XTALDIV, 0);
+	regmap_write(clkdata->regmap, TPS68470_REG_PLLDIV, 0);
+	regmap_write(clkdata->regmap, TPS68470_REG_POSTDIV, 0);
+	regmap_write(clkdata->regmap, TPS68470_REG_CLKCFG2, 0);
+	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 clk_freqs[clkdata->clk_cfg_idx].freq;
+}
+
+static int tps68470_clk_cfg_lookup(unsigned long rate)
+{
+	long diff, best_diff = LONG_MAX;
+	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)
+{
+	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);
+	int idx = tps68470_clk_cfg_lookup(rate);
+
+	if (rate != clk_freqs[idx].freq)
+		return -EINVAL;
+
+	clkdata->clk_cfg_idx = idx;
+	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 struct clk_init_data tps68470_clk_initdata = {
+	.name = TPS68470_CLK_NAME,
+	.ops = &tps68470_clk_ops,
+};
+
+static int tps68470_clk_probe(struct platform_device *pdev)
+{
+	struct tps68470_clk_platform_data *pdata = pdev->dev.platform_data;
+	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;
+	tps68470_clkdata->clk = devm_clk_register(&pdev->dev, &tps68470_clkdata->clkout_hw);
+	if (IS_ERR(tps68470_clkdata->clk))
+		return PTR_ERR(tps68470_clkdata->clk);
+
+	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);
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+}
+
+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.31.1

[PATCH v3 06/11] 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.

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.31.1

[PATCH v3 07/11] 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.

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 9fe0a2527e1c..d62d81cde6ef 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.31.1

[PATCH v3 08/11] 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.

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 d62d81cde6ef..fefe12850777 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.31.1

[PATCH v3 09/11] 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.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
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 | 33 ++++++++++++++-----
 1 file changed, 25 insertions(+), 8 deletions(-)

diff --git a/drivers/platform/x86/intel/int3472/tps68470.c b/drivers/platform/x86/intel/int3472/tps68470.c
index e95b0f50b384..cb161aef22bd 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"
@@ -17,12 +18,6 @@ static const struct mfd_cell tps68470_cros[] = {
 	{ .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 +100,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 +128,24 @@ 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:
-		ret = devm_mfd_add_devices(&client->dev, PLATFORM_DEVID_NONE,
-					   tps68470_win, ARRAY_SIZE(tps68470_win),
+		cells = kcalloc(3, sizeof(*cells), GFP_KERNEL);
+		if (!cells)
+			return -ENOMEM;
+
+		cells[0].name = "tps68470-clk";
+		cells[0].platform_data = &clk_pdata;
+		cells[0].pdata_size = sizeof(clk_pdata);
+		cells[1].name = "tps68470-regulator";
+		/*
+		 * The GPIO cell must be last because acpi_gpiochip_add() calls
+		 * acpi_dev_clear_dependencies() and the clk + regulators must
+		 * be ready when this happens.
+		 */
+		cells[2].name = "tps68470-gpio";
+
+		ret = devm_mfd_add_devices(&client->dev, PLATFORM_DEVID_NONE, cells, 3,
 					   NULL, 0, NULL);
+		kfree(cells);
 		break;
 	case DESIGNED_FOR_CHROMEOS:
 		ret = devm_mfd_add_devices(&client->dev, PLATFORM_DEVID_NONE,
-- 
2.31.1

[PATCH v3 10/11] 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 GPIOs which may be connected to the tps68470 gpios.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 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   | 118 ++++++++++++++++++
 4 files changed, 172 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 cb161aef22bd..c53c7960ee09 100644
--- a/drivers/platform/x86/intel/int3472/tps68470.c
+++ b/drivers/platform/x86/intel/int3472/tps68470.c
@@ -9,6 +9,7 @@
 #include <linux/regmap.h>
 
 #include "common.h"
+#include "tps68470.h"
 
 #define DESIGNED_FOR_CHROMEOS		1
 #define DESIGNED_FOR_WINDOWS		2
@@ -100,6 +101,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;
@@ -128,6 +130,12 @@ 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) {
+			dev_err(&client->dev, "No board-data found for this laptop/tablet model\n");
+			return -ENODEV;
+		}
+
 		cells = kcalloc(3, sizeof(*cells), GFP_KERNEL);
 		if (!cells)
 			return -ENOMEM;
@@ -136,6 +144,8 @@ 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);
 		/*
 		 * The GPIO cell must be last because acpi_gpiochip_add() calls
 		 * acpi_dev_clear_dependencies() and the clk + regulators must
@@ -143,9 +153,15 @@ static int skl_int3472_tps68470_probe(struct i2c_client *client)
 		 */
 		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, 3,
 					   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,
@@ -160,6 +176,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 },
 	{ }
@@ -172,6 +199,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..96954a789bb8
--- /dev/null
+++ b/drivers/platform/x86/intel/int3472/tps68470_board_data.c
@@ -0,0 +1,118 @@
+// 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_vsio_consumer_supplies[] = {
+	REGULATOR_SUPPLY("dovdd", "i2c-INT347A:00"),
+};
+
+static const struct regulator_init_data surface_go_tps68470_core_reg_init_data = {
+	.constraints = {
+		.min_uV = 1200000,
+		.max_uV = 1200000,
+		.apply_uV = 1,
+		.valid_ops_mask = REGULATOR_CHANGE_STATUS,
+	},
+	.num_consumer_supplies = ARRAY_SIZE(int347a_core_consumer_supplies),
+	.consumer_supplies = int347a_core_consumer_supplies,
+};
+
+static const struct regulator_init_data surface_go_tps68470_ana_reg_init_data = {
+	.constraints = {
+		.min_uV = 2815200,
+		.max_uV = 2815200,
+		.apply_uV = 1,
+		.valid_ops_mask = REGULATOR_CHANGE_STATUS,
+	},
+	.num_consumer_supplies = ARRAY_SIZE(int347a_ana_consumer_supplies),
+	.consumer_supplies = int347a_ana_consumer_supplies,
+};
+
+static const struct regulator_init_data surface_go_tps68470_vsio_reg_init_data = {
+	.constraints = {
+		.min_uV = 1800600,
+		.max_uV = 1800600,
+		.apply_uV = 1,
+		.valid_ops_mask = REGULATOR_CHANGE_STATUS,
+	},
+	.num_consumer_supplies = ARRAY_SIZE(int347a_vsio_consumer_supplies),
+	.consumer_supplies = int347a_vsio_consumer_supplies,
+};
+
+static const struct tps68470_regulator_platform_data surface_go_tps68470_pdata = {
+	.reg_init_data = {
+		[TPS68470_CORE] = &surface_go_tps68470_core_reg_init_data,
+		[TPS68470_ANA]  = &surface_go_tps68470_ana_reg_init_data,
+		[TPS68470_VSIO] = &surface_go_tps68470_vsio_reg_init_data,
+	},
+};
+
+static struct gpiod_lookup_table surface_go_tps68470_gpios = {
+	.dev_id = "i2c-INT347A:00",
+	.table = {
+		GPIO_LOOKUP("tps68470-gpio", 9, "reset", GPIO_ACTIVE_LOW),
+		GPIO_LOOKUP("tps68470-gpio", 7, "powerdown", GPIO_ACTIVE_LOW)
+	}
+};
+
+static const struct int3472_tps68470_board_data surface_go_tps68470_board_data = {
+	.dev_name = "i2c-INT3472:05",
+	.tps68470_gpio_lookup_table = &surface_go_tps68470_gpios,
+	.tps68470_regulator_pdata = &surface_go_tps68470_pdata,
+};
+
+static const struct dmi_system_id int3472_tps68470_board_data_table[] = {
+	{
+		.matches = {
+			DMI_EXACT_MATCH(DMI_SYS_VENDOR, "Microsoft Corporation"),
+			DMI_EXACT_MATCH(DMI_PRODUCT_NAME, "Surface Go"),
+		},
+		.driver_data = (void *)&surface_go_tps68470_board_data,
+	},
+	{
+		.matches = {
+			DMI_EXACT_MATCH(DMI_SYS_VENDOR, "Microsoft Corporation"),
+			DMI_EXACT_MATCH(DMI_PRODUCT_NAME, "Surface Go 2"),
+		},
+		.driver_data = (void *)&surface_go_tps68470_board_data,
+	},
+	{ }
+};
+
+const struct int3472_tps68470_board_data *int3472_tps68470_get_board_data(const char *dev_name)
+{
+	const struct int3472_tps68470_board_data *board_data;
+	const struct dmi_system_id *match;
+
+	match = dmi_first_match(int3472_tps68470_board_data_table);
+	while (match) {
+		board_data = match->driver_data;
+		if (strcmp(board_data->dev_name, dev_name) == 0)
+			return board_data;
+
+		dmi_first_match(++match);
+	}
+
+	return NULL;
+}
-- 
2.31.1

[PATCH v3 11/11] 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.

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 fefe12850777..e23a45b985dc 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 c53c7960ee09..fcc7083e8916 100644
--- a/drivers/platform/x86/intel/int3472/tps68470.c
+++ b/drivers/platform/x86/intel/int3472/tps68470.c
@@ -173,6 +173,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;
 }
 
@@ -206,3 +211,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.31.1

Re: [PATCH v3 04/11] regulator: Introduce tps68470-regulator driver

[Randy Dunlap]

On 10/10/21 11:57 AM, 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.
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
> 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 | 193 +++++++++++++++++++++++++
>   3 files changed, 203 insertions(+)
>   create mode 100644 drivers/regulator/tps68470-regulator.c
> 
> diff --git a/drivers/regulator/Kconfig b/drivers/regulator/Kconfig
> index 4fd13b06231f..d107af5bff6c 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 TPS68370 PMIC Regulators Driver"
> +	depends on INTEL_SKL_INT3472
> +	help
> +	  This driver adds support for the TPS68470 PMIC to register
> +	  regulators against the usual framework.
> +
> +	  The module will be called "tps68470-regulator"

End the final sentence with a period (a.k.a. full stop).

-- 
~Randy

Re: [PATCH v3 05/11] clk: Introduce clk-tps68470 driver

[Randy Dunlap]

On 10/10/21 11:57 AM, 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 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.
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
> 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          |   6 +
>   drivers/clk/Makefile         |   1 +
>   drivers/clk/clk-tps68470.c   | 256 +++++++++++++++++++++++++++++++++++
>   include/linux/mfd/tps68470.h |  11 ++
>   4 files changed, 274 insertions(+)
>   create mode 100644 drivers/clk/clk-tps68470.c
> 
> diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig
> index c5b3dc97396a..7dffecac83d1 100644
> --- a/drivers/clk/Kconfig
> +++ b/drivers/clk/Kconfig
> @@ -169,6 +169,12 @@ 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 && REGMAP_I2C && INTEL_SKL_INT3472
> +	help
> +	 This driver supports the clocks provided by TPS68470

End that sentence with a period (full stop): '.'.

Also it should be indented with one tab + 2 spaces.

-- 
~Randy

Re: [PATCH v3 02/11] i2c: acpi: Use acpi_dev_ready_for_enumeration() helper

[Wolfram Sang]

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

On Sun, Oct 10, 2021 at 08:56:58PM +0200, 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.
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>

If the ACPI and I2C-ACPI maintainers are happy, I am fine with this,
too:

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


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

Re: [PATCH v3 01/11] ACPI: delay enumeration of devices with a _DEP pointing to an INT3472 device

[Mika Westerberg]

Hi,

On Sun, Oct 10, 2021 at 08:56:57PM +0200, Hans de Goede wrote:
> +/* 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 */

Is there some reason why we can't do this for all devices with _DEP?
That way we don't need to maintain lists like this.

Otherwise looks good.

Re: [PATCH v3 01/11] ACPI: delay enumeration of devices with a _DEP pointing to an INT3472 device

[Hans de Goede]

Hi,

On 10/11/21 8:19 AM, Mika Westerberg wrote:
> Hi,
> 
> On Sun, Oct 10, 2021 at 08:56:57PM +0200, Hans de Goede wrote:
>> +/* 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 */
> 
> Is there some reason why we can't do this for all devices with _DEP?
> That way we don't need to maintain lists like this.

Up until now the ACPI core deliberate mostly ignores _DEP-s because the
_DEP method may point to pretty much any random ACPI object and Linux does
not necessarily have a driver for all ACPI objects the driver points too,
which would lead to the devices never getting instantiated.

In hindsight this might not have been the best solution (1), but if we
now start honoring _DEP-s for all devices all of a sudden then this
will almost certainly lead to a whole bunch of regressions.

Note that in this case the HID which triggers this is for the device
being depended upon and for all camera sensors used with the IPU3 and
IPU4 Intel camera blocks this is the INT3472 device. By triggering on
this HID (rather then on the sensor HIDs) I expect that we will not
need to update this list all that often.

Regards,

Hans



1) I believe that Windows does pay more reference to the _DEP-s and we've
had some other related issues lately.

Re: [PATCH v3 01/11] ACPI: delay enumeration of devices with a _DEP pointing to an INT3472 device

[Mika Westerberg]

On Mon, Oct 11, 2021 at 09:11:05AM +0200, Hans de Goede wrote:
> Hi,
> 
> On 10/11/21 8:19 AM, Mika Westerberg wrote:
> > Hi,
> > 
> > On Sun, Oct 10, 2021 at 08:56:57PM +0200, Hans de Goede wrote:
> >> +/* 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 */
> > 
> > Is there some reason why we can't do this for all devices with _DEP?
> > That way we don't need to maintain lists like this.
> 
> Up until now the ACPI core deliberate mostly ignores _DEP-s because the
> _DEP method may point to pretty much any random ACPI object and Linux does
> not necessarily have a driver for all ACPI objects the driver points too,
> which would lead to the devices never getting instantiated.
> 
> In hindsight this might not have been the best solution (1), but if we
> now start honoring _DEP-s for all devices all of a sudden then this
> will almost certainly lead to a whole bunch of regressions.
> 
> Note that in this case the HID which triggers this is for the device
> being depended upon and for all camera sensors used with the IPU3 and
> IPU4 Intel camera blocks this is the INT3472 device. By triggering on
> this HID (rather then on the sensor HIDs) I expect that we will not
> need to update this list all that often.

I see and agree. Thanks for the explanation!

No objections from my side then :)

Re: [PATCH v3 01/11] ACPI: delay enumeration of devices with a _DEP pointing to an INT3472 device

[Rafael J. Wysocki]

On Sun, Oct 10, 2021 at 8:57 PM Hans de Goede <hdegoede@redhat.com> 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.
>
> 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.
>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  drivers/acpi/scan.c     | 36 ++++++++++++++++++++++++++++++++++--
>  include/acpi/acpi_bus.h |  5 ++++-
>  2 files changed, 38 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
> index 5b54c80b9d32..efee6ee91c8f 100644
> --- a/drivers/acpi/scan.c
> +++ b/drivers/acpi/scan.c
> @@ -796,6 +796,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;
> @@ -1757,8 +1763,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;

Any concerns about doing

adev->flags.honor_deps = dep->honor_dep;

here?

> +
>                         adev->dep_unmet++;
> +               }
>         }
>  }
>
> @@ -1962,7 +1972,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)) {
> @@ -1971,6 +1981,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)
> @@ -1984,6 +1995,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);
> @@ -2071,6 +2083,9 @@ static acpi_status acpi_bus_check_add_2(acpi_handle handle, u32 lvl_not_used,
>
>  static void acpi_default_enumeration(struct acpi_device *device)
>  {
> +       if (!acpi_dev_ready_for_enumeration(device))
> +               return;

I'm not sure about this.

First of all, this adds an acpi_device_is_present() check here which
potentially is a change in behavior and I'm not sure how it is related
to the other changes in this patch (it is not mentioned in the
changelog AFAICS).

I'm saying "potentially", because if we get here at all,
acpi_device_is_present() has been evaluated already by
acpi_bus_attach().

Now, IIUC, the new acpi_dev_ready_for_enumeration() is kind of an
extension of acpi_device_is_present(), so shouldn't it be called by
acpi_bus_attach() instead of the latter rather than from here?

> +
>         /*
>          * Do not enumerate devices with enumeration_by_parent flag set as
>          * they will be enumerated by their respective parents.
> @@ -2313,6 +2328,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 13d93371790e..2da53b7b4965 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 */
> @@ -284,6 +285,7 @@ struct acpi_dep_data {
>         struct list_head node;
>         acpi_handle supplier;
>         acpi_handle consumer;
> +       bool honor_dep;
>  };
>
>  /* Performance Management */
> @@ -693,6 +695,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);
> --

Re: [PATCH v3 02/11] i2c: acpi: Use acpi_dev_ready_for_enumeration() helper

[Rafael J. Wysocki]

On Sun, Oct 10, 2021 at 8:57 PM Hans de Goede <hdegoede@redhat.com> 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.
>
> 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 aaeeacc12121..688cc71d650d 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;

I kind of prefer combining checks that cause the same error code to be
returned, but this is fine with me too.

Re: [PATCH v3 01/11] ACPI: delay enumeration of devices with a _DEP pointing to an INT3472 device

[Hans de Goede]

Hi,

On 10/13/21 7:29 PM, Rafael J. Wysocki wrote:
> On Sun, Oct 10, 2021 at 8:57 PM Hans de Goede <hdegoede@redhat.com> 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.
>>
>> 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.
>>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>>  drivers/acpi/scan.c     | 36 ++++++++++++++++++++++++++++++++++--
>>  include/acpi/acpi_bus.h |  5 ++++-
>>  2 files changed, 38 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
>> index 5b54c80b9d32..efee6ee91c8f 100644
>> --- a/drivers/acpi/scan.c
>> +++ b/drivers/acpi/scan.c
>> @@ -796,6 +796,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;
>> @@ -1757,8 +1763,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;
> 
> Any concerns about doing
> 
> adev->flags.honor_deps = dep->honor_dep;
> 
> here?

The idea is to set adev->flags.honor_deps even if the device has
multiple deps and only one of them has the honor_dep flag set.

If we just do:

	adev->flags.honor_deps = dep->honor_dep;

Then adev->flags.honor_deps ends up having the honor_dep
flag of the last dependency checked.


> 
>> +
>>                         adev->dep_unmet++;
>> +               }
>>         }
>>  }
>>
>> @@ -1962,7 +1972,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)) {
>> @@ -1971,6 +1981,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)
>> @@ -1984,6 +1995,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);
>> @@ -2071,6 +2083,9 @@ static acpi_status acpi_bus_check_add_2(acpi_handle handle, u32 lvl_not_used,
>>
>>  static void acpi_default_enumeration(struct acpi_device *device)
>>  {
>> +       if (!acpi_dev_ready_for_enumeration(device))
>> +               return;
> 
> I'm not sure about this.
> 
> First of all, this adds an acpi_device_is_present() check here which
> potentially is a change in behavior and I'm not sure how it is related
> to the other changes in this patch (it is not mentioned in the
> changelog AFAICS).
> 
> I'm saying "potentially", because if we get here at all,
> acpi_device_is_present() has been evaluated already by
> acpi_bus_attach().

Right the idea was that for this code-path the extra
acpi_device_is_present() check is a no-op since the only
caller of acpi_default_enumeration() has already done
that check before calling acpi_default_enumeration(),
where as the is_present check is useful for users outside
of the ACPI core code, like e.g. the i2c ACPI enumeration
code.

Although I see this is also called from
acpi_generic_device_attach which comes into play when there
is devicetree info embedded inside the ACPI tables.


> Now, IIUC, the new acpi_dev_ready_for_enumeration() is kind of an
> extension of acpi_device_is_present(), so shouldn't it be called by
> acpi_bus_attach() instead of the latter rather than from here?

That is an interesting proposal. I assume you want this to replace
the current acpi_device_is_present() call in acpi_bus_attach()
then ?

For the use-case at hand here that should work fine and it would also
make the honor_deps flag work for devices which bind to the actual
acpi_device (because we delay the device_attach()) or
use an acpi_scan_handler.

This would mean though that we can now have acpi_device-s where
acpi_device_is_present() returns true, but which are not
initialized (do not have device->flags.initialized set)
that would be a new acpi_device state which we have not had
before. I do not immediately forsee this causing issues,
but still...

If you want me to replace the current acpi_device_is_present() call
in acpi_bus_attach() with the new acpi_dev_ready_for_enumeration()
helper, let me know and I'll prepare a new version with this change
(and run some tests with that new version).

Regards,

Hans







> 
>> +
>>         /*
>>          * Do not enumerate devices with enumeration_by_parent flag set as
>>          * they will be enumerated by their respective parents.
>> @@ -2313,6 +2328,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 13d93371790e..2da53b7b4965 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 */
>> @@ -284,6 +285,7 @@ struct acpi_dep_data {
>>         struct list_head node;
>>         acpi_handle supplier;
>>         acpi_handle consumer;
>> +       bool honor_dep;
>>  };
>>
>>  /* Performance Management */
>> @@ -693,6 +695,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);
>> --
> 

Re: [PATCH v3 01/11] ACPI: delay enumeration of devices with a _DEP pointing to an INT3472 device

[Rafael J. Wysocki]

On Wed, Oct 13, 2021 at 8:23 PM Hans de Goede <hdegoede@redhat.com> wrote:
>
> Hi,
>
> On 10/13/21 7:29 PM, Rafael J. Wysocki wrote:
> > On Sun, Oct 10, 2021 at 8:57 PM Hans de Goede <hdegoede@redhat.com> 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.
> >>
> >> 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.
> >>
> >> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> >> ---
> >>  drivers/acpi/scan.c     | 36 ++++++++++++++++++++++++++++++++++--
> >>  include/acpi/acpi_bus.h |  5 ++++-
> >>  2 files changed, 38 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
> >> index 5b54c80b9d32..efee6ee91c8f 100644
> >> --- a/drivers/acpi/scan.c
> >> +++ b/drivers/acpi/scan.c
> >> @@ -796,6 +796,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;
> >> @@ -1757,8 +1763,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;
> >
> > Any concerns about doing
> >
> > adev->flags.honor_deps = dep->honor_dep;
> >
> > here?
>
> The idea is to set adev->flags.honor_deps even if the device has
> multiple deps and only one of them has the honor_dep flag set.
>
> If we just do:
>
>         adev->flags.honor_deps = dep->honor_dep;
>
> Then adev->flags.honor_deps ends up having the honor_dep
> flag of the last dependency checked.

OK, but in that case dep_unmet may be blocking the enumeration of the
device even if the one in the acpi_honor_dep_ids[] list has probed
successfully.

Isn't that a concern?

> >
> >> +
> >>                         adev->dep_unmet++;
> >> +               }
> >>         }
> >>  }
> >>
> >> @@ -1962,7 +1972,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)) {
> >> @@ -1971,6 +1981,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)
> >> @@ -1984,6 +1995,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);
> >> @@ -2071,6 +2083,9 @@ static acpi_status acpi_bus_check_add_2(acpi_handle handle, u32 lvl_not_used,
> >>
> >>  static void acpi_default_enumeration(struct acpi_device *device)
> >>  {
> >> +       if (!acpi_dev_ready_for_enumeration(device))
> >> +               return;
> >
> > I'm not sure about this.
> >
> > First of all, this adds an acpi_device_is_present() check here which
> > potentially is a change in behavior and I'm not sure how it is related
> > to the other changes in this patch (it is not mentioned in the
> > changelog AFAICS).
> >
> > I'm saying "potentially", because if we get here at all,
> > acpi_device_is_present() has been evaluated already by
> > acpi_bus_attach().
>
> Right the idea was that for this code-path the extra
> acpi_device_is_present() check is a no-op since the only
> caller of acpi_default_enumeration() has already done
> that check before calling acpi_default_enumeration(),
> where as the is_present check is useful for users outside
> of the ACPI core code, like e.g. the i2c ACPI enumeration
> code.
>
> Although I see this is also called from
> acpi_generic_device_attach which comes into play when there
> is devicetree info embedded inside the ACPI tables.

That too, but generally speaking this change should at least be
mentioned in the changelog.

> > Now, IIUC, the new acpi_dev_ready_for_enumeration() is kind of an
> > extension of acpi_device_is_present(), so shouldn't it be called by
> > acpi_bus_attach() instead of the latter rather than from here?
>
> That is an interesting proposal. I assume you want this to replace
> the current acpi_device_is_present() call in acpi_bus_attach()
> then ?

That seems consistent to me.

> For the use-case at hand here that should work fine and it would also
> make the honor_deps flag work for devices which bind to the actual
> acpi_device (because we delay the device_attach()) or
> use an acpi_scan_handler.
>
> This would mean though that we can now have acpi_device-s where
> acpi_device_is_present() returns true, but which are not
> initialized (do not have device->flags.initialized set)
> that would be a new acpi_device state which we have not had
> before. I do not immediately forsee this causing issues,
> but still...
>
> If you want me to replace the current acpi_device_is_present() call
> in acpi_bus_attach() with the new acpi_dev_ready_for_enumeration()
> helper, let me know and I'll prepare a new version with this change
> (and run some tests with that new version).

I would prefer doing that to making acpi_default_enumeration() special
with respect to the handling of dependencies.

Re: [PATCH v3 01/11] ACPI: delay enumeration of devices with a _DEP pointing to an INT3472 device

[Hans de Goede]

Hi,

On 10/13/21 8:48 PM, Rafael J. Wysocki wrote:
> On Wed, Oct 13, 2021 at 8:23 PM Hans de Goede <hdegoede@redhat.com> wrote:
>>
>> Hi,
>>
>> On 10/13/21 7:29 PM, Rafael J. Wysocki wrote:
>>> On Sun, Oct 10, 2021 at 8:57 PM Hans de Goede <hdegoede@redhat.com> 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.
>>>>
>>>> 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.
>>>>
>>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>>>> ---
>>>>  drivers/acpi/scan.c     | 36 ++++++++++++++++++++++++++++++++++--
>>>>  include/acpi/acpi_bus.h |  5 ++++-
>>>>  2 files changed, 38 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
>>>> index 5b54c80b9d32..efee6ee91c8f 100644
>>>> --- a/drivers/acpi/scan.c
>>>> +++ b/drivers/acpi/scan.c
>>>> @@ -796,6 +796,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;
>>>> @@ -1757,8 +1763,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;
>>>
>>> Any concerns about doing
>>>
>>> adev->flags.honor_deps = dep->honor_dep;
>>>
>>> here?
>>
>> The idea is to set adev->flags.honor_deps even if the device has
>> multiple deps and only one of them has the honor_dep flag set.
>>
>> If we just do:
>>
>>         adev->flags.honor_deps = dep->honor_dep;
>>
>> Then adev->flags.honor_deps ends up having the honor_dep
>> flag of the last dependency checked.
> 
> OK, but in that case dep_unmet may be blocking the enumeration of the
> device even if the one in the acpi_honor_dep_ids[] list has probed
> successfully.
> 
> Isn't that a concern?

For the devices where we set the dep->honor_dep flag this is
not a concern (based on the DSDTs which I've seen).

I also don't expect it to be a concern for other cases where we may
set that flag in the future either. This is an opt-in thing, so
I expect that in cases where we opt in to this, we also ensure that
any other _DEPs are also met (by having a Linux driver which calls
acpi_dev_clear_dependencies() for them).

And now a days we also have the acpi_ignore_dep_ids[] list so if
in the future there are some _DEP-s which never get fulfilled/met
on a device where we set the adev->flags.honor_deps flag, then we
can always add the ACPI HIDs for those devices to that list.

>>>> +
>>>>                         adev->dep_unmet++;
>>>> +               }
>>>>         }
>>>>  }
>>>>
>>>> @@ -1962,7 +1972,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)) {
>>>> @@ -1971,6 +1981,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)
>>>> @@ -1984,6 +1995,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);
>>>> @@ -2071,6 +2083,9 @@ static acpi_status acpi_bus_check_add_2(acpi_handle handle, u32 lvl_not_used,
>>>>
>>>>  static void acpi_default_enumeration(struct acpi_device *device)
>>>>  {
>>>> +       if (!acpi_dev_ready_for_enumeration(device))
>>>> +               return;
>>>
>>> I'm not sure about this.
>>>
>>> First of all, this adds an acpi_device_is_present() check here which
>>> potentially is a change in behavior and I'm not sure how it is related
>>> to the other changes in this patch (it is not mentioned in the
>>> changelog AFAICS).
>>>
>>> I'm saying "potentially", because if we get here at all,
>>> acpi_device_is_present() has been evaluated already by
>>> acpi_bus_attach().
>>
>> Right the idea was that for this code-path the extra
>> acpi_device_is_present() check is a no-op since the only
>> caller of acpi_default_enumeration() has already done
>> that check before calling acpi_default_enumeration(),
>> where as the is_present check is useful for users outside
>> of the ACPI core code, like e.g. the i2c ACPI enumeration
>> code.
>>
>> Although I see this is also called from
>> acpi_generic_device_attach which comes into play when there
>> is devicetree info embedded inside the ACPI tables.
> 
> That too, but generally speaking this change should at least be
> mentioned in the changelog.
> 
>>> Now, IIUC, the new acpi_dev_ready_for_enumeration() is kind of an
>>> extension of acpi_device_is_present(), so shouldn't it be called by
>>> acpi_bus_attach() instead of the latter rather than from here?
>>
>> That is an interesting proposal. I assume you want this to replace
>> the current acpi_device_is_present() call in acpi_bus_attach()
>> then ?
> 
> That seems consistent to me.
> 
>> For the use-case at hand here that should work fine and it would also
>> make the honor_deps flag work for devices which bind to the actual
>> acpi_device (because we delay the device_attach()) or
>> use an acpi_scan_handler.
>>
>> This would mean though that we can now have acpi_device-s where
>> acpi_device_is_present() returns true, but which are not
>> initialized (do not have device->flags.initialized set)
>> that would be a new acpi_device state which we have not had
>> before. I do not immediately forsee this causing issues,
>> but still...
>>
>> If you want me to replace the current acpi_device_is_present() call
>> in acpi_bus_attach() with the new acpi_dev_ready_for_enumeration()
>> helper, let me know and I'll prepare a new version with this change
>> (and run some tests with that new version).
> 
> I would prefer doing that to making acpi_default_enumeration() special
> with respect to the handling of dependencies.

Ok I will make this change in the next version (ETA sometime next week).

Regards,

Hans

Re: [PATCH v3 05/11] clk: Introduce clk-tps68470 driver

[Hans de Goede]

Hi Stephen,

Thank you for the review.

On 10/13/21 21:12, Stephen Boyd wrote:
> Quoting Hans de Goede (2021-10-10 11:57:01)
>> diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig
>> index c5b3dc97396a..7dffecac83d1 100644
>> --- a/drivers/clk/Kconfig
>> +++ b/drivers/clk/Kconfig
>> @@ -169,6 +169,12 @@ 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 && REGMAP_I2C && INTEL_SKL_INT3472
> 
> Pretty sure REGMAP_I2C should be selected, not depended on.
> 
> Can it
> 
> 	depends on INTEL_SKL_INT3472 || COMPILE_TEST
> 
> so that we don't have to enable the intel specific config to compile
> test this driver?

Ack, all fixed for v4.

>> +       help
>> +        This driver supports the clocks provided by TPS68470
>> +
>>  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..27e8cbd0f60e
>> --- /dev/null
>> +++ b/drivers/clk/clk-tps68470.c
>> @@ -0,0 +1,256 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Clock driver for TPS68470 PMIC
>> + *
>> + * Copyright (C) 2018 Intel Corporation
>> + *
>> + * Authors:
>> + *     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)
>> +
>> +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;
>> +       struct clk *clk;
>> +       int clk_cfg_idx;
>> +};
>> +
>> +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);
>> +       int idx = clkdata->clk_cfg_idx;
>> +
>> +       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);
>> +
>> +       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);
>> +
>> +       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);
>> +
>> +       /* write hw defaults */
> 
> Is it necessary to reset the registers to 0? Can the comment indicate
> why it is necessary instead of stating what the code is doing?

As mentioned in the commit msg this driver started out of tree, this part
comes unmodified from the out of tree driver.

After inspecting the datasheet you are right and most of these register
clears are not necessary to disable the clock.

Only the clearing of TPS68470_REG_CLKCFG1 is necesary to tristate the
clk output pin. I will remove the rest and add a comment about the
clearing of TPS68470_REG_CLKCFG1.

> 
>> +       regmap_write(clkdata->regmap, TPS68470_REG_BOOSTDIV, 0);
>> +       regmap_write(clkdata->regmap, TPS68470_REG_BUCKDIV, 0);
>> +       regmap_write(clkdata->regmap, TPS68470_REG_PLLSWR, 0);
>> +       regmap_write(clkdata->regmap, TPS68470_REG_XTALDIV, 0);
>> +       regmap_write(clkdata->regmap, TPS68470_REG_PLLDIV, 0);
>> +       regmap_write(clkdata->regmap, TPS68470_REG_POSTDIV, 0);
>> +       regmap_write(clkdata->regmap, TPS68470_REG_CLKCFG2, 0);
>> +       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 clk_freqs[clkdata->clk_cfg_idx].freq;
>> +}
>> +
>> +static int tps68470_clk_cfg_lookup(unsigned long rate)
> 
> unsigned? Doesn't seem to return negative numbers.

Will fix for v4.
> 
>> +{
>> +       long diff, best_diff = LONG_MAX;
>> +       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)
>> +{
>> +       int idx = tps68470_clk_cfg_lookup(rate);
> 
> unsigned?

Will fix for v4.


> 
>> +
>> +       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);
>> +       int idx = tps68470_clk_cfg_lookup(rate);
>> +
>> +       if (rate != clk_freqs[idx].freq)
>> +               return -EINVAL;
>> +
>> +       clkdata->clk_cfg_idx = idx;
> 
> Newline here please.

Done.

> Also, why isn't this function actually writing
> hardware?

set_rate can only be called when the clock is disabled, all the
necessary values are programmed based on the clk_cfg_idx in
tps68470_clk_prepare().

Note there is no enable() since enable() may not sleep and
this device is interfaced over I2C, so the clock is already
enabled from the prepare() op.

> 
>> +       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 struct clk_init_data tps68470_clk_initdata = {
> 
> const?

Ack.

> 
>> +       .name = TPS68470_CLK_NAME,
>> +       .ops = &tps68470_clk_ops,
>> +};
>> +
>> +static int tps68470_clk_probe(struct platform_device *pdev)
>> +{
>> +       struct tps68470_clk_platform_data *pdata = pdev->dev.platform_data;
>> +       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;
>> +       tps68470_clkdata->clk = devm_clk_register(&pdev->dev, &tps68470_clkdata->clkout_hw);
> 
> Please use devm_clk_hw_register()

Good idea, done for v4.

> 
>> +       if (IS_ERR(tps68470_clkdata->clk))
>> +               return PTR_ERR(tps68470_clkdata->clk);
>> +
>> +       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);
>> +               if (ret)
>> +                       return ret;
> 
> Drop these two lines?
> 
>> +       }
>> +
>> +       return 0;
> 
> And then
> 
> return ret;

Done for v4.

Regards,

Hans

Re: [PATCH v3 05/11] clk: Introduce clk-tps68470 driver

[Andy Shevchenko]

On Thu, Oct 21, 2021 at 8:31 PM Hans de Goede <hdegoede@redhat.com> wrote:
> On 10/13/21 21:12, Stephen Boyd wrote:

...

> >> +       regmap_write(clkdata->regmap, TPS68470_REG_CLKCFG1,

> >> +                          (TPS68470_PLL_OUTPUT_ENABLE <<
> >> +                          TPS68470_OUTPUT_A_SHIFT) |

One line, please?

> >> +                          (TPS68470_PLL_OUTPUT_ENABLE <<
> >> +                          TPS68470_OUTPUT_B_SHIFT));

Ditto.

...

> > Also, why isn't this function actually writing
> > hardware?
>
> set_rate can only be called when the clock is disabled, all the
> necessary values are programmed based on the clk_cfg_idx in
> tps68470_clk_prepare().
>
> Note there is no enable() since enable() may not sleep and
> this device is interfaced over I2C, so the clock is already
> enabled from the prepare() op.

This reminds me other drivers that do commit the changes to the
hardware on bus lock, but I'm not sure if anything like that is
applicable here.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 05/11] clk: Introduce clk-tps68470 driver

[Hans de Goede]

Hi,

On 10/22/21 10:46, Andy Shevchenko wrote:
> On Thu, Oct 21, 2021 at 8:31 PM Hans de Goede <hdegoede@redhat.com> wrote:
>> On 10/13/21 21:12, Stephen Boyd wrote:
> 
> ...
> 
>>>> +       regmap_write(clkdata->regmap, TPS68470_REG_CLKCFG1,
> 
>>>> +                          (TPS68470_PLL_OUTPUT_ENABLE <<
>>>> +                          TPS68470_OUTPUT_A_SHIFT) |
> 
> One line, please?
> 
>>>> +                          (TPS68470_PLL_OUTPUT_ENABLE <<
>>>> +                          TPS68470_OUTPUT_B_SHIFT));
> 
> Ditto.

Ack, both fixed for v4.

Regards,

Hans