[PATCH 0/6] Add multiple-consumer support to int3472-tps68470 driver

[PATCH 0/6] Add multiple-consumer support to int3472-tps68470 driver

[Daniel Scally]

At the moment there are a few places in the int3472-tps68470 driver that are
limited to just working with a single consuming device dependent on the PMIC.
There are systems where multiple camera sensors share a single TPS68470, so
we need to extend the driver to support them. This requires a couple of tweaks
to the ACPI functions to fetch dependent devices, which also assumes that only
a single dependent will be found.

Hans - this (plus a series to media [1]) adds support for the Surface Go 2's
IR camera...the regulator settings for the Go1/2/3 world facing camera are the
same, so I'd expect them to match for the IR sensor too, which means it should
enable support for your Go too.

Thanks
Dan


[1] https://lore.kernel.org/linux-media/20220215230737.1870630-1-djrscally@gmail.com/

Daniel Scally (6):
  ACPI: scan: Add acpi_dev_get_next_consumer_dev()
  ACPI: bus: Add iterator for dependent devices
  platform/x86: int3472: Support multiple clock consumers
  platform/x86: int3472: Add terminator to gpiod_lookup_table
  platform/x86: int3472: Support multiple gpio lookups in board data
  platform/x86: int3472: Add board data for Surface Go2 IR camera

 drivers/acpi/scan.c                           | 47 +++++++++--
 drivers/clk/clk-tps68470.c                    | 13 +++-
 drivers/platform/x86/intel/int3472/tps68470.c | 77 +++++++++++++++----
 drivers/platform/x86/intel/int3472/tps68470.h |  3 +-
 .../x86/intel/int3472/tps68470_board_data.c   | 58 +++++++++++++-
 include/acpi/acpi_bus.h                       | 14 ++++
 include/linux/platform_data/tps68470.h        |  7 +-
 7 files changed, 188 insertions(+), 31 deletions(-)

-- 
2.25.1

[PATCH 1/6] ACPI: scan: Add acpi_dev_get_next_consumer_dev()

[Daniel Scally]

In commit b83e2b306736 ("ACPI: scan: Add function to fetch dependent of ACPI
device") we added a means of fetching the first device to declare itself
dependent on another ACPI device in the _DEP method. One assumption
in that patch was that there would only be a single consuming device,
but this has not held.

Extend the functionality by adding a new function that fetches the
next consumer of a supplier device. We can then simplify the original
function by simply calling the new one.

Signed-off-by: Daniel Scally <djrscally@gmail.com>
---
 drivers/acpi/scan.c     | 47 ++++++++++++++++++++++++++++++++++-------
 include/acpi/acpi_bus.h |  2 ++
 2 files changed, 41 insertions(+), 8 deletions(-)

diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
index 4463c2eda61e..b3ed664ee1cb 100644
--- a/drivers/acpi/scan.c
+++ b/drivers/acpi/scan.c
@@ -2256,9 +2256,21 @@ static void acpi_bus_attach(struct acpi_device *device, bool first_pass)
 		device->handler->hotplug.notify_online(device);
 }
 
-static int acpi_dev_get_first_consumer_dev_cb(struct acpi_dep_data *dep, void *data)
+static int acpi_dev_get_next_consumer_dev_cb(struct acpi_dep_data *dep, void *data)
 {
-	struct acpi_device *adev;
+	struct acpi_device *adev = *(struct acpi_device **)data;
+
+	/*
+	 * If we're passed a 'previous' consumer device then we need to skip
+	 * any consumers until we meet the previous one, and then NULL @data
+	 * so the next one can be returned.
+	 */
+	if (adev) {
+		if (dep->consumer == adev->handle)
+			*(struct acpi_device **)data = NULL;
+
+		return 0;
+	}
 
 	adev = acpi_bus_get_acpi_device(dep->consumer);
 	if (adev) {
@@ -2389,23 +2401,42 @@ bool acpi_dev_ready_for_enumeration(const struct acpi_device *device)
 EXPORT_SYMBOL_GPL(acpi_dev_ready_for_enumeration);
 
 /**
- * acpi_dev_get_first_consumer_dev - Return ACPI device dependent on @supplier
+ * acpi_dev_get_next_consumer_dev - Return the next adev dependent on @supplier
  * @supplier: Pointer to the dependee device
+ * @start: Pointer to the current dependent device
  *
- * Returns the first &struct acpi_device which declares itself dependent on
+ * Returns the next &struct acpi_device which declares itself dependent on
  * @supplier via the _DEP buffer, parsed from the acpi_dep_list.
  *
  * The caller is responsible for putting the reference to adev when it is no
  * longer needed.
  */
-struct acpi_device *acpi_dev_get_first_consumer_dev(struct acpi_device *supplier)
+struct acpi_device *acpi_dev_get_next_consumer_dev(struct acpi_device *supplier,
+						   struct acpi_device *start)
 {
-	struct acpi_device *adev = NULL;
+	struct acpi_device *adev = start;
 
 	acpi_walk_dep_device_list(supplier->handle,
-				  acpi_dev_get_first_consumer_dev_cb, &adev);
+				  acpi_dev_get_next_consumer_dev_cb, &adev);
 
-	return adev;
+	acpi_dev_put(start);
+	return adev == start ? NULL : adev;
+}
+EXPORT_SYMBOL_GPL(acpi_dev_get_next_consumer_dev);
+
+/**
+ * acpi_dev_get_first_consumer_dev - Return ACPI device dependent on @supplier
+ * @supplier: Pointer to the dependee device
+ *
+ * Returns the first &struct acpi_device which declares itself dependent on
+ * @supplier via the _DEP buffer, parsed from the acpi_dep_list.
+ *
+ * The caller is responsible for putting the reference to adev when it is no
+ * longer needed.
+ */
+struct acpi_device *acpi_dev_get_first_consumer_dev(struct acpi_device *supplier)
+{
+	return acpi_dev_get_next_consumer_dev(supplier, NULL);
 }
 EXPORT_SYMBOL_GPL(acpi_dev_get_first_consumer_dev);
 
diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h
index ca88c4706f2b..8b06fef04722 100644
--- a/include/acpi/acpi_bus.h
+++ b/include/acpi/acpi_bus.h
@@ -720,6 +720,8 @@ bool acpi_dev_hid_uid_match(struct acpi_device *adev, const char *hid2, const ch
 
 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_next_consumer_dev(struct acpi_device *supplier,
+						   struct acpi_device *start);
 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.25.1

Re: [PATCH 1/6] ACPI: scan: Add acpi_dev_get_next_consumer_dev()

[Hans de Goede]

Hi,

On 2/16/22 23:52, Daniel Scally wrote:
> In commit b83e2b306736 ("ACPI: scan: Add function to fetch dependent of ACPI
> device") we added a means of fetching the first device to declare itself
> dependent on another ACPI device in the _DEP method. One assumption
> in that patch was that there would only be a single consuming device,
> but this has not held.
> 
> Extend the functionality by adding a new function that fetches the
> next consumer of a supplier device. We can then simplify the original
> function by simply calling the new one.
> 
> Signed-off-by: Daniel Scally <djrscally@gmail.com>
> ---
>  drivers/acpi/scan.c     | 47 ++++++++++++++++++++++++++++++++++-------
>  include/acpi/acpi_bus.h |  2 ++
>  2 files changed, 41 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
> index 4463c2eda61e..b3ed664ee1cb 100644
> --- a/drivers/acpi/scan.c
> +++ b/drivers/acpi/scan.c
> @@ -2256,9 +2256,21 @@ static void acpi_bus_attach(struct acpi_device *device, bool first_pass)
>  		device->handler->hotplug.notify_online(device);
>  }
>  
> -static int acpi_dev_get_first_consumer_dev_cb(struct acpi_dep_data *dep, void *data)
> +static int acpi_dev_get_next_consumer_dev_cb(struct acpi_dep_data *dep, void *data)
>  {
> -	struct acpi_device *adev;
> +	struct acpi_device *adev = *(struct acpi_device **)data;
> +
> +	/*
> +	 * If we're passed a 'previous' consumer device then we need to skip
> +	 * any consumers until we meet the previous one, and then NULL @data
> +	 * so the next one can be returned.
> +	 */
> +	if (adev) {
> +		if (dep->consumer == adev->handle)
> +			*(struct acpi_device **)data = NULL;
> +
> +		return 0;
> +	}
>  
>  	adev = acpi_bus_get_acpi_device(dep->consumer);
>  	if (adev) {
> @@ -2389,23 +2401,42 @@ bool acpi_dev_ready_for_enumeration(const struct acpi_device *device)
>  EXPORT_SYMBOL_GPL(acpi_dev_ready_for_enumeration);
>  
>  /**
> - * acpi_dev_get_first_consumer_dev - Return ACPI device dependent on @supplier
> + * acpi_dev_get_next_consumer_dev - Return the next adev dependent on @supplier
>   * @supplier: Pointer to the dependee device
> + * @start: Pointer to the current dependent device
>   *
> - * Returns the first &struct acpi_device which declares itself dependent on
> + * Returns the next &struct acpi_device which declares itself dependent on
>   * @supplier via the _DEP buffer, parsed from the acpi_dep_list.
>   *
>   * The caller is responsible for putting the reference to adev when it is no
>   * longer needed.

This bit of the help text seems to not be entirely correct, since the reference to
start gets consumed by this, so the caller only needs to put the device when it
does NOT pass it as start to another acpi_dev_get_next_consumer_dev call.



>   */
> -struct acpi_device *acpi_dev_get_first_consumer_dev(struct acpi_device *supplier)
> +struct acpi_device *acpi_dev_get_next_consumer_dev(struct acpi_device *supplier,
> +						   struct acpi_device *start)
>  {
> -	struct acpi_device *adev = NULL;
> +	struct acpi_device *adev = start;
>  
>  	acpi_walk_dep_device_list(supplier->handle,
> -				  acpi_dev_get_first_consumer_dev_cb, &adev);
> +				  acpi_dev_get_next_consumer_dev_cb, &adev);
>  
> -	return adev;
> +	acpi_dev_put(start);
> +	return adev == start ? NULL : adev;
> +}
> +EXPORT_SYMBOL_GPL(acpi_dev_get_next_consumer_dev);
> +
> +/**
> + * acpi_dev_get_first_consumer_dev - Return ACPI device dependent on @supplier
> + * @supplier: Pointer to the dependee device
> + *
> + * Returns the first &struct acpi_device which declares itself dependent on
> + * @supplier via the _DEP buffer, parsed from the acpi_dep_list.
> + *
> + * The caller is responsible for putting the reference to adev when it is no
> + * longer needed.
> + */
> +struct acpi_device *acpi_dev_get_first_consumer_dev(struct acpi_device *supplier)
> +{
> +	return acpi_dev_get_next_consumer_dev(supplier, NULL);
>  }
>  EXPORT_SYMBOL_GPL(acpi_dev_get_first_consumer_dev);

The only caller of this is skl_int3472_get_sensor_adev_and_name() IMHO it would
be better to just move that over to acpi_dev_get_next_consumer_dev(..., NULL);
in this same patch and just drop acpi_dev_get_first_consumer_dev() all together.

I expect this entire series to get merged through the pdx86 tree, so from
that pov doing this should be fine..

Regards,

Hans



>  
> diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h
> index ca88c4706f2b..8b06fef04722 100644
> --- a/include/acpi/acpi_bus.h
> +++ b/include/acpi/acpi_bus.h
> @@ -720,6 +720,8 @@ bool acpi_dev_hid_uid_match(struct acpi_device *adev, const char *hid2, const ch
>  
>  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_next_consumer_dev(struct acpi_device *supplier,
> +						   struct acpi_device *start);
>  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 1/6] ACPI: scan: Add acpi_dev_get_next_consumer_dev()

[Hans de Goede]

Hi,

On 2/21/22 10:50, Hans de Goede wrote:
> Hi,
> 
> On 2/16/22 23:52, Daniel Scally wrote:
>> In commit b83e2b306736 ("ACPI: scan: Add function to fetch dependent of ACPI
>> device") we added a means of fetching the first device to declare itself
>> dependent on another ACPI device in the _DEP method. One assumption
>> in that patch was that there would only be a single consuming device,
>> but this has not held.
>>
>> Extend the functionality by adding a new function that fetches the
>> next consumer of a supplier device. We can then simplify the original
>> function by simply calling the new one.
>>
>> Signed-off-by: Daniel Scally <djrscally@gmail.com>
>> ---
>>  drivers/acpi/scan.c     | 47 ++++++++++++++++++++++++++++++++++-------
>>  include/acpi/acpi_bus.h |  2 ++
>>  2 files changed, 41 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
>> index 4463c2eda61e..b3ed664ee1cb 100644
>> --- a/drivers/acpi/scan.c
>> +++ b/drivers/acpi/scan.c
>> @@ -2256,9 +2256,21 @@ static void acpi_bus_attach(struct acpi_device *device, bool first_pass)
>>  		device->handler->hotplug.notify_online(device);
>>  }
>>  
>> -static int acpi_dev_get_first_consumer_dev_cb(struct acpi_dep_data *dep, void *data)
>> +static int acpi_dev_get_next_consumer_dev_cb(struct acpi_dep_data *dep, void *data)
>>  {
>> -	struct acpi_device *adev;
>> +	struct acpi_device *adev = *(struct acpi_device **)data;
>> +
>> +	/*
>> +	 * If we're passed a 'previous' consumer device then we need to skip
>> +	 * any consumers until we meet the previous one, and then NULL @data
>> +	 * so the next one can be returned.
>> +	 */
>> +	if (adev) {
>> +		if (dep->consumer == adev->handle)
>> +			*(struct acpi_device **)data = NULL;
>> +
>> +		return 0;
>> +	}
>>  
>>  	adev = acpi_bus_get_acpi_device(dep->consumer);
>>  	if (adev) {
>> @@ -2389,23 +2401,42 @@ bool acpi_dev_ready_for_enumeration(const struct acpi_device *device)
>>  EXPORT_SYMBOL_GPL(acpi_dev_ready_for_enumeration);
>>  
>>  /**
>> - * acpi_dev_get_first_consumer_dev - Return ACPI device dependent on @supplier
>> + * acpi_dev_get_next_consumer_dev - Return the next adev dependent on @supplier
>>   * @supplier: Pointer to the dependee device
>> + * @start: Pointer to the current dependent device
>>   *
>> - * Returns the first &struct acpi_device which declares itself dependent on
>> + * Returns the next &struct acpi_device which declares itself dependent on
>>   * @supplier via the _DEP buffer, parsed from the acpi_dep_list.
>>   *
>>   * The caller is responsible for putting the reference to adev when it is no
>>   * longer needed.
> 
> This bit of the help text seems to not be entirely correct, since the reference to
> start gets consumed by this, so the caller only needs to put the device when it
> does NOT pass it as start to another acpi_dev_get_next_consumer_dev call.
> 
> 
> 
>>   */
>> -struct acpi_device *acpi_dev_get_first_consumer_dev(struct acpi_device *supplier)
>> +struct acpi_device *acpi_dev_get_next_consumer_dev(struct acpi_device *supplier,
>> +						   struct acpi_device *start)
>>  {
>> -	struct acpi_device *adev = NULL;
>> +	struct acpi_device *adev = start;
>>  
>>  	acpi_walk_dep_device_list(supplier->handle,
>> -				  acpi_dev_get_first_consumer_dev_cb, &adev);
>> +				  acpi_dev_get_next_consumer_dev_cb, &adev);
>>  
>> -	return adev;
>> +	acpi_dev_put(start);
>> +	return adev == start ? NULL : adev;
>> +}
>> +EXPORT_SYMBOL_GPL(acpi_dev_get_next_consumer_dev);
>> +
>> +/**
>> + * acpi_dev_get_first_consumer_dev - Return ACPI device dependent on @supplier
>> + * @supplier: Pointer to the dependee device
>> + *
>> + * Returns the first &struct acpi_device which declares itself dependent on
>> + * @supplier via the _DEP buffer, parsed from the acpi_dep_list.
>> + *
>> + * The caller is responsible for putting the reference to adev when it is no
>> + * longer needed.
>> + */
>> +struct acpi_device *acpi_dev_get_first_consumer_dev(struct acpi_device *supplier)
>> +{
>> +	return acpi_dev_get_next_consumer_dev(supplier, NULL);
>>  }
>>  EXPORT_SYMBOL_GPL(acpi_dev_get_first_consumer_dev);
> 
> The only caller of this is skl_int3472_get_sensor_adev_and_name() IMHO it would
> be better to just move that over to acpi_dev_get_next_consumer_dev(..., NULL);
> in this same patch and just drop acpi_dev_get_first_consumer_dev() all together.
> 
> I expect this entire series to get merged through the pdx86 tree, so from
> that pov doing this should be fine..

I forgot to add: that otherwise this looks good to me, so with the above
addressed you may add my:

Reviewed-by: Hans de Goede <hdegoede@redhat.com>

Regards,

Hans


>> diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h
>> index ca88c4706f2b..8b06fef04722 100644
>> --- a/include/acpi/acpi_bus.h
>> +++ b/include/acpi/acpi_bus.h
>> @@ -720,6 +720,8 @@ bool acpi_dev_hid_uid_match(struct acpi_device *adev, const char *hid2, const ch
>>  
>>  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_next_consumer_dev(struct acpi_device *supplier,
>> +						   struct acpi_device *start);
>>  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);

[PATCH 2/6] ACPI: bus: Add iterator for dependent devices

[Daniel Scally]

Add a helper macro to iterate over ACPI devices that are flagged
as consumers of an initial supplier ACPI device.

Signed-off-by: Daniel Scally <djrscally@gmail.com>
---
 include/acpi/acpi_bus.h | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h
index 8b06fef04722..72103bcdd5b3 100644
--- a/include/acpi/acpi_bus.h
+++ b/include/acpi/acpi_bus.h
@@ -723,6 +723,18 @@ bool acpi_dev_ready_for_enumeration(const struct acpi_device *device);
 struct acpi_device *acpi_dev_get_next_consumer_dev(struct acpi_device *supplier,
 						   struct acpi_device *start);
 struct acpi_device *acpi_dev_get_first_consumer_dev(struct acpi_device *supplier);
+
+/**
+ * for_each_acpi_consumer_dev - iterate over the consumer ACPI devices for a
+ *				given supplier
+ * @supplier: Pointer to the supplier's ACPI device
+ * @consumer: Pointer to &struct acpi_device to hold the consumer, initially NULL
+ */
+#define for_each_acpi_consumer_dev(supplier, consumer)			\
+	for (consumer = acpi_dev_get_first_consumer_dev(supplier);	\
+	     consumer;							\
+	     consumer = acpi_dev_get_next_consumer_dev(supplier, consumer))
+
 struct acpi_device *
 acpi_dev_get_next_match_dev(struct acpi_device *adev, const char *hid, const char *uid, s64 hrv);
 struct acpi_device *
-- 
2.25.1

Re: [PATCH 2/6] ACPI: bus: Add iterator for dependent devices

[Hans de Goede]

Hi,

On 2/16/22 23:53, Daniel Scally wrote:
> Add a helper macro to iterate over ACPI devices that are flagged
> as consumers of an initial supplier ACPI device.
> 
> Signed-off-by: Daniel Scally <djrscally@gmail.com>
> ---
>  include/acpi/acpi_bus.h | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h
> index 8b06fef04722..72103bcdd5b3 100644
> --- a/include/acpi/acpi_bus.h
> +++ b/include/acpi/acpi_bus.h
> @@ -723,6 +723,18 @@ bool acpi_dev_ready_for_enumeration(const struct acpi_device *device);
>  struct acpi_device *acpi_dev_get_next_consumer_dev(struct acpi_device *supplier,
>  						   struct acpi_device *start);
>  struct acpi_device *acpi_dev_get_first_consumer_dev(struct acpi_device *supplier);
> +
> +/**
> + * for_each_acpi_consumer_dev - iterate over the consumer ACPI devices for a
> + *				given supplier
> + * @supplier: Pointer to the supplier's ACPI device
> + * @consumer: Pointer to &struct acpi_device to hold the consumer, initially NULL
> + */
> +#define for_each_acpi_consumer_dev(supplier, consumer)			\
> +	for (consumer = acpi_dev_get_first_consumer_dev(supplier);	\

If we drop acpi_dev_get_first_consumer_dev() this needs to be updated to:

	for (consumer = acpi_dev_get_next_consumer_dev(supplier, NULL);  \

Otherwise this looks good to me:

Reviewed-by: Hans de Goede <hdegoede@redhat.com>

Regards,

Hans

> +	     consumer;							\
> +	     consumer = acpi_dev_get_next_consumer_dev(supplier, consumer))
> +
>  struct acpi_device *
>  acpi_dev_get_next_match_dev(struct acpi_device *adev, const char *hid, const char *uid, s64 hrv);
>  struct acpi_device *

[PATCH 3/6] platform/x86: int3472: Support multiple clock consumers

[Daniel Scally]

At present, the int3472-tps68470 only supports a single clock consumer when
passing platform data to the clock driver. In some devices multiple
sensors depend on the clock provided by a single TPS68470 and so all
need to be able to acquire the clock. Support passing multiple
consumers as platform data.

Signed-off-by: Daniel Scally <djrscally@gmail.com>
---
 drivers/clk/clk-tps68470.c                    | 13 ++--
 drivers/platform/x86/intel/int3472/tps68470.c | 59 ++++++++++++++++---
 include/linux/platform_data/tps68470.h        |  7 ++-
 3 files changed, 67 insertions(+), 12 deletions(-)

diff --git a/drivers/clk/clk-tps68470.c b/drivers/clk/clk-tps68470.c
index e5fbefd6ac2d..38f44b5b9b1b 100644
--- a/drivers/clk/clk-tps68470.c
+++ b/drivers/clk/clk-tps68470.c
@@ -200,7 +200,9 @@ static int tps68470_clk_probe(struct platform_device *pdev)
 		.flags = CLK_SET_RATE_GATE,
 	};
 	struct tps68470_clkdata *tps68470_clkdata;
+	struct tps68470_clk_consumer *consumer;
 	int ret;
+	int i;
 
 	tps68470_clkdata = devm_kzalloc(&pdev->dev, sizeof(*tps68470_clkdata),
 					GFP_KERNEL);
@@ -223,10 +225,13 @@ static int tps68470_clk_probe(struct platform_device *pdev)
 		return ret;
 
 	if (pdata) {
-		ret = devm_clk_hw_register_clkdev(&pdev->dev,
-						  &tps68470_clkdata->clkout_hw,
-						  pdata->consumer_con_id,
-						  pdata->consumer_dev_name);
+		for (i = 0; i < pdata->n_consumers; i++) {
+			consumer = &pdata->consumers[i];
+			ret = devm_clk_hw_register_clkdev(&pdev->dev,
+							  &tps68470_clkdata->clkout_hw,
+							  consumer->consumer_con_id,
+							  consumer->consumer_dev_name);
+		}
 	}
 
 	return ret;
diff --git a/drivers/platform/x86/intel/int3472/tps68470.c b/drivers/platform/x86/intel/int3472/tps68470.c
index 22f61b47f9e5..b535564712bb 100644
--- a/drivers/platform/x86/intel/int3472/tps68470.c
+++ b/drivers/platform/x86/intel/int3472/tps68470.c
@@ -1,6 +1,7 @@
 // SPDX-License-Identifier: GPL-2.0
 /* Author: Dan Scally <djrscally@gmail.com> */
 
+#include <linux/acpi.h>
 #include <linux/i2c.h>
 #include <linux/kernel.h>
 #include <linux/mfd/core.h>
@@ -95,20 +96,64 @@ static int skl_int3472_tps68470_calc_type(struct acpi_device *adev)
 	return DESIGNED_FOR_WINDOWS;
 }
 
+/*
+ * Return the size of the flexible array member, because we'll need that later
+ * on to pass .pdata_size to cells.
+ */
+static int
+skl_int3472_fill_clk_pdata(struct device *dev, struct tps68470_clk_platform_data **clk_pdata)
+{
+	struct acpi_device *adev = ACPI_COMPANION(dev);
+	struct acpi_device *consumer;
+	unsigned int n_consumers = 0;
+	const char *sensor_name;
+	unsigned int i = 0;
+
+	for_each_acpi_consumer_dev(adev, consumer)
+		n_consumers++;
+
+	if (!n_consumers) {
+		dev_err(dev, "INT3472 seems to have no dependents\n");
+		return -ENODEV;
+	}
+
+	*clk_pdata = devm_kzalloc(dev, struct_size(*clk_pdata, consumers, n_consumers),
+				  GFP_KERNEL);
+	if (!*clk_pdata)
+		return -ENOMEM;
+
+	(*clk_pdata)->n_consumers = n_consumers;
+	i = 0;
+
+	for_each_acpi_consumer_dev(adev, consumer) {
+		sensor_name = devm_kasprintf(dev, GFP_KERNEL, I2C_DEV_NAME_FORMAT,
+					     acpi_dev_name(consumer));
+		if (!sensor_name)
+			return -ENOMEM;
+
+		(*clk_pdata)->consumers[i].consumer_dev_name = sensor_name;
+		i++;
+	}
+
+	acpi_dev_put(consumer);
+
+	return n_consumers;
+}
+
 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 tps68470_clk_platform_data *clk_pdata;
+	unsigned int n_consumers;
 	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;
+	n_consumers = skl_int3472_fill_clk_pdata(&client->dev, &clk_pdata);
+	if (n_consumers < 0)
+		return n_consumers;
 
 	regmap = devm_regmap_init_i2c(client, &tps68470_regmap_config);
 	if (IS_ERR(regmap)) {
@@ -142,8 +187,8 @@ static int skl_int3472_tps68470_probe(struct i2c_client *client)
 		 * the clk + regulators must be ready when this happens.
 		 */
 		cells[0].name = "tps68470-clk";
-		cells[0].platform_data = &clk_pdata;
-		cells[0].pdata_size = sizeof(clk_pdata);
+		cells[0].platform_data = clk_pdata;
+		cells[0].pdata_size = struct_size(clk_pdata, consumers, n_consumers);
 		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);
diff --git a/include/linux/platform_data/tps68470.h b/include/linux/platform_data/tps68470.h
index 126d082c3f2e..e605a2cab07f 100644
--- a/include/linux/platform_data/tps68470.h
+++ b/include/linux/platform_data/tps68470.h
@@ -27,9 +27,14 @@ struct tps68470_regulator_platform_data {
 	const struct regulator_init_data *reg_init_data[TPS68470_NUM_REGULATORS];
 };
 
-struct tps68470_clk_platform_data {
+struct tps68470_clk_consumer {
 	const char *consumer_dev_name;
 	const char *consumer_con_id;
 };
 
+struct tps68470_clk_platform_data {
+	unsigned int n_consumers;
+	struct tps68470_clk_consumer consumers[];
+};
+
 #endif
-- 
2.25.1

Re: [PATCH 3/6] platform/x86: int3472: Support multiple clock consumers

[kernel test robot]

Hi Daniel,

I love your patch! Perhaps something to improve:

[auto build test WARNING on rafael-pm/linux-next]
[also build test WARNING on clk/clk-next linus/master v5.17-rc4 next-20220216]
[cannot apply to platform-drivers-x86/for-next]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Daniel-Scally/Add-multiple-consumer-support-to-int3472-tps68470-driver/20220217-065452
base:   https://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git linux-next
config: x86_64-randconfig-m001 (https://download.01.org/0day-ci/archive/20220217/202202171110.7EOaTUJH-lkp@intel.com/config)
compiler: gcc-9 (Debian 9.3.0-22) 9.3.0

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

smatch warnings:
drivers/platform/x86/intel/int3472/tps68470.c:155 skl_int3472_tps68470_probe() warn: unsigned 'n_consumers' is never less than zero.

vim +/n_consumers +155 drivers/platform/x86/intel/int3472/tps68470.c

   142	
   143	static int skl_int3472_tps68470_probe(struct i2c_client *client)
   144	{
   145		struct acpi_device *adev = ACPI_COMPANION(&client->dev);
   146		const struct int3472_tps68470_board_data *board_data;
   147		struct tps68470_clk_platform_data *clk_pdata;
   148		unsigned int n_consumers;
   149		struct mfd_cell *cells;
   150		struct regmap *regmap;
   151		int device_type;
   152		int ret;
   153	
   154		n_consumers = skl_int3472_fill_clk_pdata(&client->dev, &clk_pdata);
 > 155		if (n_consumers < 0)
   156			return n_consumers;
   157	
   158		regmap = devm_regmap_init_i2c(client, &tps68470_regmap_config);
   159		if (IS_ERR(regmap)) {
   160			dev_err(&client->dev, "Failed to create regmap: %ld\n", PTR_ERR(regmap));
   161			return PTR_ERR(regmap);
   162		}
   163	
   164		i2c_set_clientdata(client, regmap);
   165	
   166		ret = tps68470_chip_init(&client->dev, regmap);
   167		if (ret < 0) {
   168			dev_err(&client->dev, "TPS68470 init error %d\n", ret);
   169			return ret;
   170		}
   171	
   172		device_type = skl_int3472_tps68470_calc_type(adev);
   173		switch (device_type) {
   174		case DESIGNED_FOR_WINDOWS:
   175			board_data = int3472_tps68470_get_board_data(dev_name(&client->dev));
   176			if (!board_data)
   177				return dev_err_probe(&client->dev, -ENODEV, "No board-data found for this model\n");
   178	
   179			cells = kcalloc(TPS68470_WIN_MFD_CELL_COUNT, sizeof(*cells), GFP_KERNEL);
   180			if (!cells)
   181				return -ENOMEM;
   182	
   183			/*
   184			 * The order of the cells matters here! The clk must be first
   185			 * because the regulator depends on it. The gpios must be last,
   186			 * acpi_gpiochip_add() calls acpi_dev_clear_dependencies() and
   187			 * the clk + regulators must be ready when this happens.
   188			 */
   189			cells[0].name = "tps68470-clk";
   190			cells[0].platform_data = clk_pdata;
   191			cells[0].pdata_size = struct_size(clk_pdata, consumers, n_consumers);
   192			cells[1].name = "tps68470-regulator";
   193			cells[1].platform_data = (void *)board_data->tps68470_regulator_pdata;
   194			cells[1].pdata_size = sizeof(struct tps68470_regulator_platform_data);
   195			cells[2].name = "tps68470-gpio";
   196	
   197			gpiod_add_lookup_table(board_data->tps68470_gpio_lookup_table);
   198	
   199			ret = devm_mfd_add_devices(&client->dev, PLATFORM_DEVID_NONE,
   200						   cells, TPS68470_WIN_MFD_CELL_COUNT,
   201						   NULL, 0, NULL);
   202			kfree(cells);
   203	
   204			if (ret)
   205				gpiod_remove_lookup_table(board_data->tps68470_gpio_lookup_table);
   206	
   207			break;
   208		case DESIGNED_FOR_CHROMEOS:
   209			ret = devm_mfd_add_devices(&client->dev, PLATFORM_DEVID_NONE,
   210						   tps68470_cros, ARRAY_SIZE(tps68470_cros),
   211						   NULL, 0, NULL);
   212			break;
   213		default:
   214			dev_err(&client->dev, "Failed to add MFD devices\n");
   215			return device_type;
   216		}
   217	
   218		/*
   219		 * No acpi_dev_clear_dependencies() here, since the acpi_gpiochip_add()
   220		 * for the GPIO cell already does this.
   221		 */
   222	
   223		return ret;
   224	}
   225	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

Re: [PATCH 3/6] platform/x86: int3472: Support multiple clock consumers

[Hans de Goede]

Hi,

On 2/17/22 04:22, kernel test robot wrote:
> Hi Daniel,
> 
> I love your patch! Perhaps something to improve:
> 
> [auto build test WARNING on rafael-pm/linux-next]
> [also build test WARNING on clk/clk-next linus/master v5.17-rc4 next-20220216]
> [cannot apply to platform-drivers-x86/for-next]
> [If your patch is applied to the wrong git tree, kindly drop us a note.
> And when submitting patch, we suggest to use '--base' as documented in
> https://git-scm.com/docs/git-format-patch]
> 
> url:    https://github.com/0day-ci/linux/commits/Daniel-Scally/Add-multiple-consumer-support-to-int3472-tps68470-driver/20220217-065452
> base:   https://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git linux-next
> config: x86_64-randconfig-m001 (https://download.01.org/0day-ci/archive/20220217/202202171110.7EOaTUJH-lkp@intel.com/config)
> compiler: gcc-9 (Debian 9.3.0-22) 9.3.0
> 
> If you fix the issue, kindly add following tag as appropriate
> Reported-by: kernel test robot <lkp@intel.com>
> 
> smatch warnings:
> drivers/platform/x86/intel/int3472/tps68470.c:155 skl_int3472_tps68470_probe() warn: unsigned 'n_consumers' is never less than zero.

Right this needs to be an int, not an unsigned int. Daniel, please fix this for v2.

Regards,

Hans



> 
> vim +/n_consumers +155 drivers/platform/x86/intel/int3472/tps68470.c
> 
>    142	
>    143	static int skl_int3472_tps68470_probe(struct i2c_client *client)
>    144	{
>    145		struct acpi_device *adev = ACPI_COMPANION(&client->dev);
>    146		const struct int3472_tps68470_board_data *board_data;
>    147		struct tps68470_clk_platform_data *clk_pdata;
>    148		unsigned int n_consumers;
>    149		struct mfd_cell *cells;
>    150		struct regmap *regmap;
>    151		int device_type;
>    152		int ret;
>    153	
>    154		n_consumers = skl_int3472_fill_clk_pdata(&client->dev, &clk_pdata);
>  > 155		if (n_consumers < 0)
>    156			return n_consumers;
>    157	
>    158		regmap = devm_regmap_init_i2c(client, &tps68470_regmap_config);
>    159		if (IS_ERR(regmap)) {
>    160			dev_err(&client->dev, "Failed to create regmap: %ld\n", PTR_ERR(regmap));
>    161			return PTR_ERR(regmap);
>    162		}
>    163	
>    164		i2c_set_clientdata(client, regmap);
>    165	
>    166		ret = tps68470_chip_init(&client->dev, regmap);
>    167		if (ret < 0) {
>    168			dev_err(&client->dev, "TPS68470 init error %d\n", ret);
>    169			return ret;
>    170		}
>    171	
>    172		device_type = skl_int3472_tps68470_calc_type(adev);
>    173		switch (device_type) {
>    174		case DESIGNED_FOR_WINDOWS:
>    175			board_data = int3472_tps68470_get_board_data(dev_name(&client->dev));
>    176			if (!board_data)
>    177				return dev_err_probe(&client->dev, -ENODEV, "No board-data found for this model\n");
>    178	
>    179			cells = kcalloc(TPS68470_WIN_MFD_CELL_COUNT, sizeof(*cells), GFP_KERNEL);
>    180			if (!cells)
>    181				return -ENOMEM;
>    182	
>    183			/*
>    184			 * The order of the cells matters here! The clk must be first
>    185			 * because the regulator depends on it. The gpios must be last,
>    186			 * acpi_gpiochip_add() calls acpi_dev_clear_dependencies() and
>    187			 * the clk + regulators must be ready when this happens.
>    188			 */
>    189			cells[0].name = "tps68470-clk";
>    190			cells[0].platform_data = clk_pdata;
>    191			cells[0].pdata_size = struct_size(clk_pdata, consumers, n_consumers);
>    192			cells[1].name = "tps68470-regulator";
>    193			cells[1].platform_data = (void *)board_data->tps68470_regulator_pdata;
>    194			cells[1].pdata_size = sizeof(struct tps68470_regulator_platform_data);
>    195			cells[2].name = "tps68470-gpio";
>    196	
>    197			gpiod_add_lookup_table(board_data->tps68470_gpio_lookup_table);
>    198	
>    199			ret = devm_mfd_add_devices(&client->dev, PLATFORM_DEVID_NONE,
>    200						   cells, TPS68470_WIN_MFD_CELL_COUNT,
>    201						   NULL, 0, NULL);
>    202			kfree(cells);
>    203	
>    204			if (ret)
>    205				gpiod_remove_lookup_table(board_data->tps68470_gpio_lookup_table);
>    206	
>    207			break;
>    208		case DESIGNED_FOR_CHROMEOS:
>    209			ret = devm_mfd_add_devices(&client->dev, PLATFORM_DEVID_NONE,
>    210						   tps68470_cros, ARRAY_SIZE(tps68470_cros),
>    211						   NULL, 0, NULL);
>    212			break;
>    213		default:
>    214			dev_err(&client->dev, "Failed to add MFD devices\n");
>    215			return device_type;
>    216		}
>    217	
>    218		/*
>    219		 * No acpi_dev_clear_dependencies() here, since the acpi_gpiochip_add()
>    220		 * for the GPIO cell already does this.
>    221		 */
>    222	
>    223		return ret;
>    224	}
>    225	
> 
> ---
> 0-DAY CI Kernel Test Service, Intel Corporation
> https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
> 

Re: [PATCH 3/6] platform/x86: int3472: Support multiple clock consumers

[Daniel Scally]

Hi Hans

On 22/02/2022 14:44, Hans de Goede wrote:
> Hi,
>
> On 2/17/22 04:22, kernel test robot wrote:
>> Hi Daniel,
>>
>> I love your patch! Perhaps something to improve:
>>
>> [auto build test WARNING on rafael-pm/linux-next]
>> [also build test WARNING on clk/clk-next linus/master v5.17-rc4 next-20220216]
>> [cannot apply to platform-drivers-x86/for-next]
>> [If your patch is applied to the wrong git tree, kindly drop us a note.
>> And when submitting patch, we suggest to use '--base' as documented in
>> https://git-scm.com/docs/git-format-patch]
>>
>> url:    https://github.com/0day-ci/linux/commits/Daniel-Scally/Add-multiple-consumer-support-to-int3472-tps68470-driver/20220217-065452
>> base:   https://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git linux-next
>> config: x86_64-randconfig-m001 (https://download.01.org/0day-ci/archive/20220217/202202171110.7EOaTUJH-lkp@intel.com/config)
>> compiler: gcc-9 (Debian 9.3.0-22) 9.3.0
>>
>> If you fix the issue, kindly add following tag as appropriate
>> Reported-by: kernel test robot <lkp@intel.com>
>>
>> smatch warnings:
>> drivers/platform/x86/intel/int3472/tps68470.c:155 skl_int3472_tps68470_probe() warn: unsigned 'n_consumers' is never less than zero.
> Right this needs to be an int, not an unsigned int. Daniel, please fix this for v2.


Will do! And thanks for your comments / tags on the rest of the series;
I'll post a new version some time this week.


Dan

>
> Regards,
>
> Hans
>
>
>
>> vim +/n_consumers +155 drivers/platform/x86/intel/int3472/tps68470.c
>>
>>    142	
>>    143	static int skl_int3472_tps68470_probe(struct i2c_client *client)
>>    144	{
>>    145		struct acpi_device *adev = ACPI_COMPANION(&client->dev);
>>    146		const struct int3472_tps68470_board_data *board_data;
>>    147		struct tps68470_clk_platform_data *clk_pdata;
>>    148		unsigned int n_consumers;
>>    149		struct mfd_cell *cells;
>>    150		struct regmap *regmap;
>>    151		int device_type;
>>    152		int ret;
>>    153	
>>    154		n_consumers = skl_int3472_fill_clk_pdata(&client->dev, &clk_pdata);
>>  > 155		if (n_consumers < 0)
>>    156			return n_consumers;
>>    157	
>>    158		regmap = devm_regmap_init_i2c(client, &tps68470_regmap_config);
>>    159		if (IS_ERR(regmap)) {
>>    160			dev_err(&client->dev, "Failed to create regmap: %ld\n", PTR_ERR(regmap));
>>    161			return PTR_ERR(regmap);
>>    162		}
>>    163	
>>    164		i2c_set_clientdata(client, regmap);
>>    165	
>>    166		ret = tps68470_chip_init(&client->dev, regmap);
>>    167		if (ret < 0) {
>>    168			dev_err(&client->dev, "TPS68470 init error %d\n", ret);
>>    169			return ret;
>>    170		}
>>    171	
>>    172		device_type = skl_int3472_tps68470_calc_type(adev);
>>    173		switch (device_type) {
>>    174		case DESIGNED_FOR_WINDOWS:
>>    175			board_data = int3472_tps68470_get_board_data(dev_name(&client->dev));
>>    176			if (!board_data)
>>    177				return dev_err_probe(&client->dev, -ENODEV, "No board-data found for this model\n");
>>    178	
>>    179			cells = kcalloc(TPS68470_WIN_MFD_CELL_COUNT, sizeof(*cells), GFP_KERNEL);
>>    180			if (!cells)
>>    181				return -ENOMEM;
>>    182	
>>    183			/*
>>    184			 * The order of the cells matters here! The clk must be first
>>    185			 * because the regulator depends on it. The gpios must be last,
>>    186			 * acpi_gpiochip_add() calls acpi_dev_clear_dependencies() and
>>    187			 * the clk + regulators must be ready when this happens.
>>    188			 */
>>    189			cells[0].name = "tps68470-clk";
>>    190			cells[0].platform_data = clk_pdata;
>>    191			cells[0].pdata_size = struct_size(clk_pdata, consumers, n_consumers);
>>    192			cells[1].name = "tps68470-regulator";
>>    193			cells[1].platform_data = (void *)board_data->tps68470_regulator_pdata;
>>    194			cells[1].pdata_size = sizeof(struct tps68470_regulator_platform_data);
>>    195			cells[2].name = "tps68470-gpio";
>>    196	
>>    197			gpiod_add_lookup_table(board_data->tps68470_gpio_lookup_table);
>>    198	
>>    199			ret = devm_mfd_add_devices(&client->dev, PLATFORM_DEVID_NONE,
>>    200						   cells, TPS68470_WIN_MFD_CELL_COUNT,
>>    201						   NULL, 0, NULL);
>>    202			kfree(cells);
>>    203	
>>    204			if (ret)
>>    205				gpiod_remove_lookup_table(board_data->tps68470_gpio_lookup_table);
>>    206	
>>    207			break;
>>    208		case DESIGNED_FOR_CHROMEOS:
>>    209			ret = devm_mfd_add_devices(&client->dev, PLATFORM_DEVID_NONE,
>>    210						   tps68470_cros, ARRAY_SIZE(tps68470_cros),
>>    211						   NULL, 0, NULL);
>>    212			break;
>>    213		default:
>>    214			dev_err(&client->dev, "Failed to add MFD devices\n");
>>    215			return device_type;
>>    216		}
>>    217	
>>    218		/*
>>    219		 * No acpi_dev_clear_dependencies() here, since the acpi_gpiochip_add()
>>    220		 * for the GPIO cell already does this.
>>    221		 */
>>    222	
>>    223		return ret;
>>    224	}
>>    225	
>>
>> ---
>> 0-DAY CI Kernel Test Service, Intel Corporation
>> https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
>>

Re: [PATCH 3/6] platform/x86: int3472: Support multiple clock consumers

[Hans de Goede]

Hi,

On 2/16/22 23:53, Daniel Scally wrote:
> At present, the int3472-tps68470 only supports a single clock consumer when
> passing platform data to the clock driver. In some devices multiple
> sensors depend on the clock provided by a single TPS68470 and so all
> need to be able to acquire the clock. Support passing multiple
> consumers as platform data.
> 
> Signed-off-by: Daniel Scally <djrscally@gmail.com>

Thanks, patch looks good to me:

Reviewed-by: Hans de Goede <hdegoede@redhat.com>

Michael, I plan to merge this entire series through the platform-drivers-x86 git
tree, may I have your ack for merging the clk bits from this ?

Regards,

Hans



> ---
>  drivers/clk/clk-tps68470.c                    | 13 ++--
>  drivers/platform/x86/intel/int3472/tps68470.c | 59 ++++++++++++++++---
>  include/linux/platform_data/tps68470.h        |  7 ++-
>  3 files changed, 67 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/clk/clk-tps68470.c b/drivers/clk/clk-tps68470.c
> index e5fbefd6ac2d..38f44b5b9b1b 100644
> --- a/drivers/clk/clk-tps68470.c
> +++ b/drivers/clk/clk-tps68470.c
> @@ -200,7 +200,9 @@ static int tps68470_clk_probe(struct platform_device *pdev)
>  		.flags = CLK_SET_RATE_GATE,
>  	};
>  	struct tps68470_clkdata *tps68470_clkdata;
> +	struct tps68470_clk_consumer *consumer;
>  	int ret;
> +	int i;
>  
>  	tps68470_clkdata = devm_kzalloc(&pdev->dev, sizeof(*tps68470_clkdata),
>  					GFP_KERNEL);
> @@ -223,10 +225,13 @@ static int tps68470_clk_probe(struct platform_device *pdev)
>  		return ret;
>  
>  	if (pdata) {
> -		ret = devm_clk_hw_register_clkdev(&pdev->dev,
> -						  &tps68470_clkdata->clkout_hw,
> -						  pdata->consumer_con_id,
> -						  pdata->consumer_dev_name);
> +		for (i = 0; i < pdata->n_consumers; i++) {
> +			consumer = &pdata->consumers[i];
> +			ret = devm_clk_hw_register_clkdev(&pdev->dev,
> +							  &tps68470_clkdata->clkout_hw,
> +							  consumer->consumer_con_id,
> +							  consumer->consumer_dev_name);
> +		}
>  	}
>  
>  	return ret;
> diff --git a/drivers/platform/x86/intel/int3472/tps68470.c b/drivers/platform/x86/intel/int3472/tps68470.c
> index 22f61b47f9e5..b535564712bb 100644
> --- a/drivers/platform/x86/intel/int3472/tps68470.c
> +++ b/drivers/platform/x86/intel/int3472/tps68470.c
> @@ -1,6 +1,7 @@
>  // SPDX-License-Identifier: GPL-2.0
>  /* Author: Dan Scally <djrscally@gmail.com> */
>  
> +#include <linux/acpi.h>
>  #include <linux/i2c.h>
>  #include <linux/kernel.h>
>  #include <linux/mfd/core.h>
> @@ -95,20 +96,64 @@ static int skl_int3472_tps68470_calc_type(struct acpi_device *adev)
>  	return DESIGNED_FOR_WINDOWS;
>  }
>  
> +/*
> + * Return the size of the flexible array member, because we'll need that later
> + * on to pass .pdata_size to cells.
> + */
> +static int
> +skl_int3472_fill_clk_pdata(struct device *dev, struct tps68470_clk_platform_data **clk_pdata)
> +{
> +	struct acpi_device *adev = ACPI_COMPANION(dev);
> +	struct acpi_device *consumer;
> +	unsigned int n_consumers = 0;
> +	const char *sensor_name;
> +	unsigned int i = 0;
> +
> +	for_each_acpi_consumer_dev(adev, consumer)
> +		n_consumers++;
> +
> +	if (!n_consumers) {
> +		dev_err(dev, "INT3472 seems to have no dependents\n");
> +		return -ENODEV;
> +	}
> +
> +	*clk_pdata = devm_kzalloc(dev, struct_size(*clk_pdata, consumers, n_consumers),
> +				  GFP_KERNEL);
> +	if (!*clk_pdata)
> +		return -ENOMEM;
> +
> +	(*clk_pdata)->n_consumers = n_consumers;
> +	i = 0;
> +
> +	for_each_acpi_consumer_dev(adev, consumer) {
> +		sensor_name = devm_kasprintf(dev, GFP_KERNEL, I2C_DEV_NAME_FORMAT,
> +					     acpi_dev_name(consumer));
> +		if (!sensor_name)
> +			return -ENOMEM;
> +
> +		(*clk_pdata)->consumers[i].consumer_dev_name = sensor_name;
> +		i++;
> +	}
> +
> +	acpi_dev_put(consumer);
> +
> +	return n_consumers;
> +}
> +
>  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 tps68470_clk_platform_data *clk_pdata;
> +	unsigned int n_consumers;
>  	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;
> +	n_consumers = skl_int3472_fill_clk_pdata(&client->dev, &clk_pdata);
> +	if (n_consumers < 0)
> +		return n_consumers;
>  
>  	regmap = devm_regmap_init_i2c(client, &tps68470_regmap_config);
>  	if (IS_ERR(regmap)) {
> @@ -142,8 +187,8 @@ static int skl_int3472_tps68470_probe(struct i2c_client *client)
>  		 * the clk + regulators must be ready when this happens.
>  		 */
>  		cells[0].name = "tps68470-clk";
> -		cells[0].platform_data = &clk_pdata;
> -		cells[0].pdata_size = sizeof(clk_pdata);
> +		cells[0].platform_data = clk_pdata;
> +		cells[0].pdata_size = struct_size(clk_pdata, consumers, n_consumers);
>  		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);
> diff --git a/include/linux/platform_data/tps68470.h b/include/linux/platform_data/tps68470.h
> index 126d082c3f2e..e605a2cab07f 100644
> --- a/include/linux/platform_data/tps68470.h
> +++ b/include/linux/platform_data/tps68470.h
> @@ -27,9 +27,14 @@ struct tps68470_regulator_platform_data {
>  	const struct regulator_init_data *reg_init_data[TPS68470_NUM_REGULATORS];
>  };
>  
> -struct tps68470_clk_platform_data {
> +struct tps68470_clk_consumer {
>  	const char *consumer_dev_name;
>  	const char *consumer_con_id;
>  };
>  
> +struct tps68470_clk_platform_data {
> +	unsigned int n_consumers;
> +	struct tps68470_clk_consumer consumers[];
> +};
> +
>  #endif

Re: [PATCH 3/6] platform/x86: int3472: Support multiple clock consumers

[Stephen Boyd]

Quoting Hans de Goede (2022-02-21 01:59:09)
> Hi,
> 
> On 2/16/22 23:53, Daniel Scally wrote:
> > At present, the int3472-tps68470 only supports a single clock consumer when
> > passing platform data to the clock driver. In some devices multiple
> > sensors depend on the clock provided by a single TPS68470 and so all
> > need to be able to acquire the clock. Support passing multiple
> > consumers as platform data.
> > 
> > Signed-off-by: Daniel Scally <djrscally@gmail.com>
> 
> Thanks, patch looks good to me:
> 
> Reviewed-by: Hans de Goede <hdegoede@redhat.com>
> 
> Michael, I plan to merge this entire series through the platform-drivers-x86 git
> tree, may I have your ack for merging the clk bits from this ?
> 

With the type fix

Reviewed-by: Stephen Boyd <sboyd@kernel.org>
Acked-by: Stephen Boyd <sboyd@kernel.org>

Re: [PATCH 3/6] platform/x86: int3472: Support multiple clock consumers

[Hans de Goede]

Hi Stephen,

On 2/25/22 01:49, Stephen Boyd wrote:
> Quoting Hans de Goede (2022-02-21 01:59:09)
>> Hi,
>>
>> On 2/16/22 23:53, Daniel Scally wrote:
>>> At present, the int3472-tps68470 only supports a single clock consumer when
>>> passing platform data to the clock driver. In some devices multiple
>>> sensors depend on the clock provided by a single TPS68470 and so all
>>> need to be able to acquire the clock. Support passing multiple
>>> consumers as platform data.
>>>
>>> Signed-off-by: Daniel Scally <djrscally@gmail.com>
>>
>> Thanks, patch looks good to me:
>>
>> Reviewed-by: Hans de Goede <hdegoede@redhat.com>
>>
>> Michael, I plan to merge this entire series through the platform-drivers-x86 git
>> tree, may I have your ack for merging the clk bits from this ?
>>
> 
> With the type fix
> 
> Reviewed-by: Stephen Boyd <sboyd@kernel.org>
> Acked-by: Stephen Boyd <sboyd@kernel.org>

Thanks, and sorry for not addressing you, I should have seen that
there are 2 clk subsys maintainers (and I know from experience
that you are the most active maintainer recently).

Regards,

Hans

[PATCH 4/6] platform/x86: int3472: Add terminator to gpiod_lookup_table

[Daniel Scally]

Without the terminator, if a con_id is passed to gpio_find() that
does not exist in the lookup table the function will not stop looping
correctly, and eventually cause an oops.

Fixes: 1596ef1251b5 ("platform/x86: int3472: Pass tps68470_regulator_platform_data to the tps68470-regulator MFD-cell")
Signed-off-by: Daniel Scally <djrscally@gmail.com>
---
 drivers/platform/x86/intel/int3472/tps68470_board_data.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/platform/x86/intel/int3472/tps68470_board_data.c b/drivers/platform/x86/intel/int3472/tps68470_board_data.c
index f93d437fd192..525f09a3b5ff 100644
--- a/drivers/platform/x86/intel/int3472/tps68470_board_data.c
+++ b/drivers/platform/x86/intel/int3472/tps68470_board_data.c
@@ -100,7 +100,8 @@ 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)
+		GPIO_LOOKUP("tps68470-gpio", 7, "powerdown", GPIO_ACTIVE_LOW),
+		{ }
 	}
 };
 
-- 
2.25.1

Re: [PATCH 4/6] platform/x86: int3472: Add terminator to gpiod_lookup_table

[Hans de Goede]

Hi,

On 2/16/22 23:53, Daniel Scally wrote:
> Without the terminator, if a con_id is passed to gpio_find() that
> does not exist in the lookup table the function will not stop looping
> correctly, and eventually cause an oops.
> 
> Fixes: 1596ef1251b5 ("platform/x86: int3472: Pass tps68470_regulator_platform_data to the tps68470-regulator MFD-cell")
> Signed-off-by: Daniel Scally <djrscally@gmail.com>
> ---
>  drivers/platform/x86/intel/int3472/tps68470_board_data.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/platform/x86/intel/int3472/tps68470_board_data.c b/drivers/platform/x86/intel/int3472/tps68470_board_data.c
> index f93d437fd192..525f09a3b5ff 100644
> --- a/drivers/platform/x86/intel/int3472/tps68470_board_data.c
> +++ b/drivers/platform/x86/intel/int3472/tps68470_board_data.c
> @@ -100,7 +100,8 @@ 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)
> +		GPIO_LOOKUP("tps68470-gpio", 7, "powerdown", GPIO_ACTIVE_LOW),
> +		{ }
>  	}
>  };
>  


Oops, I'll got and apply this to pdx86/for-next and pdx86/fixes right away. Please drop this
from the next version of the series.

Regards,

Hans

Re: [PATCH 4/6] platform/x86: int3472: Add terminator to gpiod_lookup_table

[Hans de Goede]

Hi,

On 2/16/22 23:53, Daniel Scally wrote:
> Without the terminator, if a con_id is passed to gpio_find() that
> does not exist in the lookup table the function will not stop looping
> correctly, and eventually cause an oops.
> 
> Fixes: 1596ef1251b5 ("platform/x86: int3472: Pass tps68470_regulator_platform_data to the tps68470-regulator MFD-cell")

This fixes tag is wrong, that sha does not exist?  See:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/log/drivers/platform/x86/intel/int3472/tps68470_board_data.c

I've updated the hash to 19d8d6e36b4b while merging this.

Regards,

Hans



> Signed-off-by: Daniel Scally <djrscally@gmail.com>
> ---
>  drivers/platform/x86/intel/int3472/tps68470_board_data.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/platform/x86/intel/int3472/tps68470_board_data.c b/drivers/platform/x86/intel/int3472/tps68470_board_data.c
> index f93d437fd192..525f09a3b5ff 100644
> --- a/drivers/platform/x86/intel/int3472/tps68470_board_data.c
> +++ b/drivers/platform/x86/intel/int3472/tps68470_board_data.c
> @@ -100,7 +100,8 @@ 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)
> +		GPIO_LOOKUP("tps68470-gpio", 7, "powerdown", GPIO_ACTIVE_LOW),
> +		{ }
>  	}
>  };
>  

[PATCH 5/6] platform/x86: int3472: Support multiple gpio lookups in board data

[Daniel Scally]

Currently, we only support passing a single gpiod_lookup_table as part
of the board data for the tps68470 driver. This carries the implicit
assumption that each TPS68470 device will only support a single
sensor, which does not hold true.

Extend the code to support the possibility of multiple sensors each
having a gpiod_lookup_table, and opportunistically add the lookup
table for the Surface Go line's IR camera.

Signed-off-by: Daniel Scally <djrscally@gmail.com>
---
 drivers/platform/x86/intel/int3472/tps68470.c | 18 ++++++++++-----
 drivers/platform/x86/intel/int3472/tps68470.h |  3 ++-
 .../x86/intel/int3472/tps68470_board_data.c   | 22 ++++++++++++++++---
 3 files changed, 33 insertions(+), 10 deletions(-)

diff --git a/drivers/platform/x86/intel/int3472/tps68470.c b/drivers/platform/x86/intel/int3472/tps68470.c
index b535564712bb..736480961ec3 100644
--- a/drivers/platform/x86/intel/int3472/tps68470.c
+++ b/drivers/platform/x86/intel/int3472/tps68470.c
@@ -150,6 +150,7 @@ static int skl_int3472_tps68470_probe(struct i2c_client *client)
 	struct regmap *regmap;
 	int device_type;
 	int ret;
+	int i;
 
 	n_consumers = skl_int3472_fill_clk_pdata(&client->dev, &clk_pdata);
 	if (n_consumers < 0)
@@ -194,15 +195,18 @@ static int skl_int3472_tps68470_probe(struct i2c_client *client)
 		cells[1].pdata_size = sizeof(struct tps68470_regulator_platform_data);
 		cells[2].name = "tps68470-gpio";
 
-		gpiod_add_lookup_table(board_data->tps68470_gpio_lookup_table);
+		for (i = 0; i < board_data->n_gpiod_lookups; i++)
+			gpiod_add_lookup_table(board_data->tps68470_gpio_lookup_tables[i]);
 
 		ret = devm_mfd_add_devices(&client->dev, PLATFORM_DEVID_NONE,
 					   cells, TPS68470_WIN_MFD_CELL_COUNT,
 					   NULL, 0, NULL);
 		kfree(cells);
 
-		if (ret)
-			gpiod_remove_lookup_table(board_data->tps68470_gpio_lookup_table);
+		if (ret) {
+			for (i = 0; i < board_data->n_gpiod_lookups; i++)
+				gpiod_remove_lookup_table(board_data->tps68470_gpio_lookup_tables[i]);
+		}
 
 		break;
 	case DESIGNED_FOR_CHROMEOS:
@@ -226,10 +230,12 @@ static int skl_int3472_tps68470_probe(struct i2c_client *client)
 static int skl_int3472_tps68470_remove(struct i2c_client *client)
 {
 	const struct int3472_tps68470_board_data *board_data;
-
+	int i;
 	board_data = int3472_tps68470_get_board_data(dev_name(&client->dev));
-	if (board_data)
-		gpiod_remove_lookup_table(board_data->tps68470_gpio_lookup_table);
+	if (board_data) {
+		for (i = 0; i < board_data->n_gpiod_lookups; i++)
+			gpiod_remove_lookup_table(board_data->tps68470_gpio_lookup_tables[i]);
+	}
 
 	return 0;
 }
diff --git a/drivers/platform/x86/intel/int3472/tps68470.h b/drivers/platform/x86/intel/int3472/tps68470.h
index cfd33eb62740..35915e701593 100644
--- a/drivers/platform/x86/intel/int3472/tps68470.h
+++ b/drivers/platform/x86/intel/int3472/tps68470.h
@@ -16,8 +16,9 @@ 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;
+	unsigned int n_gpiod_lookups;
+	struct gpiod_lookup_table *tps68470_gpio_lookup_tables[];
 };
 
 const struct int3472_tps68470_board_data *int3472_tps68470_get_board_data(const char *dev_name);
diff --git a/drivers/platform/x86/intel/int3472/tps68470_board_data.c b/drivers/platform/x86/intel/int3472/tps68470_board_data.c
index 525f09a3b5ff..442a8a2de224 100644
--- a/drivers/platform/x86/intel/int3472/tps68470_board_data.c
+++ b/drivers/platform/x86/intel/int3472/tps68470_board_data.c
@@ -96,7 +96,7 @@ static const struct tps68470_regulator_platform_data surface_go_tps68470_pdata =
 	},
 };
 
-static struct gpiod_lookup_table surface_go_tps68470_gpios = {
+static struct gpiod_lookup_table surface_go_int347a_gpios = {
 	.dev_id = "i2c-INT347A:00",
 	.table = {
 		GPIO_LOOKUP("tps68470-gpio", 9, "reset", GPIO_ACTIVE_LOW),
@@ -105,16 +105,32 @@ static struct gpiod_lookup_table surface_go_tps68470_gpios = {
 	}
 };
 
+static struct gpiod_lookup_table surface_go_int347e_gpios = {
+	.dev_id = "i2c-INT347E:00",
+	.table = {
+		GPIO_LOOKUP("tps68470-gpio", 5, "enable", GPIO_ACTIVE_HIGH),
+		{ }
+	}
+};
+
 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,
+	.n_gpiod_lookups = 2,
+	.tps68470_gpio_lookup_tables = {
+		&surface_go_int347a_gpios,
+		&surface_go_int347e_gpios,
+	}
 };
 
 static const struct int3472_tps68470_board_data surface_go3_tps68470_board_data = {
 	.dev_name = "i2c-INT3472:01",
-	.tps68470_gpio_lookup_table = &surface_go_tps68470_gpios,
 	.tps68470_regulator_pdata = &surface_go_tps68470_pdata,
+	.n_gpiod_lookups = 2,
+	.tps68470_gpio_lookup_tables = {
+		&surface_go_int347a_gpios,
+		&surface_go_int347e_gpios,
+	}
 };
 
 static const struct dmi_system_id int3472_tps68470_board_data_table[] = {
-- 
2.25.1

Re: [PATCH 5/6] platform/x86: int3472: Support multiple gpio lookups in board data

[Hans de Goede]

Hi,

On 2/16/22 23:53, Daniel Scally wrote:
> Currently, we only support passing a single gpiod_lookup_table as part
> of the board data for the tps68470 driver. This carries the implicit
> assumption that each TPS68470 device will only support a single
> sensor, which does not hold true.
> 
> Extend the code to support the possibility of multiple sensors each
> having a gpiod_lookup_table, and opportunistically add the lookup
> table for the Surface Go line's IR camera.
> 
> Signed-off-by: Daniel Scally <djrscally@gmail.com>
> ---
>  drivers/platform/x86/intel/int3472/tps68470.c | 18 ++++++++++-----
>  drivers/platform/x86/intel/int3472/tps68470.h |  3 ++-
>  .../x86/intel/int3472/tps68470_board_data.c   | 22 ++++++++++++++++---
>  3 files changed, 33 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/platform/x86/intel/int3472/tps68470.c b/drivers/platform/x86/intel/int3472/tps68470.c
> index b535564712bb..736480961ec3 100644
> --- a/drivers/platform/x86/intel/int3472/tps68470.c
> +++ b/drivers/platform/x86/intel/int3472/tps68470.c
> @@ -150,6 +150,7 @@ static int skl_int3472_tps68470_probe(struct i2c_client *client)
>  	struct regmap *regmap;
>  	int device_type;
>  	int ret;
> +	int i;
>  
>  	n_consumers = skl_int3472_fill_clk_pdata(&client->dev, &clk_pdata);
>  	if (n_consumers < 0)
> @@ -194,15 +195,18 @@ static int skl_int3472_tps68470_probe(struct i2c_client *client)
>  		cells[1].pdata_size = sizeof(struct tps68470_regulator_platform_data);
>  		cells[2].name = "tps68470-gpio";
>  
> -		gpiod_add_lookup_table(board_data->tps68470_gpio_lookup_table);
> +		for (i = 0; i < board_data->n_gpiod_lookups; i++)
> +			gpiod_add_lookup_table(board_data->tps68470_gpio_lookup_tables[i]);
>  
>  		ret = devm_mfd_add_devices(&client->dev, PLATFORM_DEVID_NONE,
>  					   cells, TPS68470_WIN_MFD_CELL_COUNT,
>  					   NULL, 0, NULL);
>  		kfree(cells);
>  
> -		if (ret)
> -			gpiod_remove_lookup_table(board_data->tps68470_gpio_lookup_table);
> +		if (ret) {
> +			for (i = 0; i < board_data->n_gpiod_lookups; i++)
> +				gpiod_remove_lookup_table(board_data->tps68470_gpio_lookup_tables[i]);
> +		}
>  
>  		break;
>  	case DESIGNED_FOR_CHROMEOS:
> @@ -226,10 +230,12 @@ static int skl_int3472_tps68470_probe(struct i2c_client *client)
>  static int skl_int3472_tps68470_remove(struct i2c_client *client)
>  {
>  	const struct int3472_tps68470_board_data *board_data;
> -
> +	int i;

Please keep an empty line between variable declarations and statements
(I believe checkpatch should have warned about this?)

>  	board_data = int3472_tps68470_get_board_data(dev_name(&client->dev));
> -	if (board_data)
> -		gpiod_remove_lookup_table(board_data->tps68470_gpio_lookup_table);
> +	if (board_data) {
> +		for (i = 0; i < board_data->n_gpiod_lookups; i++)
> +			gpiod_remove_lookup_table(board_data->tps68470_gpio_lookup_tables[i]);
> +	}
>  
>  	return 0;
>  }
> diff --git a/drivers/platform/x86/intel/int3472/tps68470.h b/drivers/platform/x86/intel/int3472/tps68470.h
> index cfd33eb62740..35915e701593 100644
> --- a/drivers/platform/x86/intel/int3472/tps68470.h
> +++ b/drivers/platform/x86/intel/int3472/tps68470.h
> @@ -16,8 +16,9 @@ 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;
> +	unsigned int n_gpiod_lookups;
> +	struct gpiod_lookup_table *tps68470_gpio_lookup_tables[];
>  };
>  
>  const struct int3472_tps68470_board_data *int3472_tps68470_get_board_data(const char *dev_name);
> diff --git a/drivers/platform/x86/intel/int3472/tps68470_board_data.c b/drivers/platform/x86/intel/int3472/tps68470_board_data.c
> index 525f09a3b5ff..442a8a2de224 100644
> --- a/drivers/platform/x86/intel/int3472/tps68470_board_data.c
> +++ b/drivers/platform/x86/intel/int3472/tps68470_board_data.c
> @@ -96,7 +96,7 @@ static const struct tps68470_regulator_platform_data surface_go_tps68470_pdata =
>  	},
>  };
>  
> -static struct gpiod_lookup_table surface_go_tps68470_gpios = {
> +static struct gpiod_lookup_table surface_go_int347a_gpios = {
>  	.dev_id = "i2c-INT347A:00",
>  	.table = {
>  		GPIO_LOOKUP("tps68470-gpio", 9, "reset", GPIO_ACTIVE_LOW),
> @@ -105,16 +105,32 @@ static struct gpiod_lookup_table surface_go_tps68470_gpios = {
>  	}
>  };
>  
> +static struct gpiod_lookup_table surface_go_int347e_gpios = {
> +	.dev_id = "i2c-INT347E:00",
> +	.table = {
> +		GPIO_LOOKUP("tps68470-gpio", 5, "enable", GPIO_ACTIVE_HIGH),
> +		{ }
> +	}
> +};
> +
>  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,
> +	.n_gpiod_lookups = 2,
> +	.tps68470_gpio_lookup_tables = {
> +		&surface_go_int347a_gpios,
> +		&surface_go_int347e_gpios,
> +	}
>  };
>  
>  static const struct int3472_tps68470_board_data surface_go3_tps68470_board_data = {
>  	.dev_name = "i2c-INT3472:01",
> -	.tps68470_gpio_lookup_table = &surface_go_tps68470_gpios,
>  	.tps68470_regulator_pdata = &surface_go_tps68470_pdata,
> +	.n_gpiod_lookups = 2,
> +	.tps68470_gpio_lookup_tables = {
> +		&surface_go_int347a_gpios,
> +		&surface_go_int347e_gpios,
> +	}
>  };
>  
>  static const struct dmi_system_id int3472_tps68470_board_data_table[] = {

Otherwise this looks good to me:

Reviewed-by: Hans de Goede <hdegoede@redhat.com>

Regards,

Hans

[PATCH 6/6] platform/x86: int3472: Add board data for Surface Go2 IR camera

[Daniel Scally]

Add the board data describing the regulators for the Microsoft
Surface Go line's IR camera.

Signed-off-by: Daniel Scally <djrscally@gmail.com>
---
 .../x86/intel/int3472/tps68470_board_data.c   | 33 +++++++++++++++++++
 1 file changed, 33 insertions(+)

diff --git a/drivers/platform/x86/intel/int3472/tps68470_board_data.c b/drivers/platform/x86/intel/int3472/tps68470_board_data.c
index 442a8a2de224..49a3591c6d85 100644
--- a/drivers/platform/x86/intel/int3472/tps68470_board_data.c
+++ b/drivers/platform/x86/intel/int3472/tps68470_board_data.c
@@ -30,6 +30,15 @@ static struct regulator_consumer_supply int347a_vcm_consumer_supplies[] = {
 static struct regulator_consumer_supply int347a_vsio_consumer_supplies[] = {
 	REGULATOR_SUPPLY("dovdd", "i2c-INT347A:00"),
 	REGULATOR_SUPPLY("vsio", "i2c-INT347A:00-VCM"),
+	REGULATOR_SUPPLY("vddd", "i2c-INT347E:00"),
+};
+
+static struct regulator_consumer_supply int347a_aux1_consumer_supplies[] = {
+	REGULATOR_SUPPLY("vdda", "i2c-INT347E:00"),
+};
+
+static struct regulator_consumer_supply int347a_aux2_consumer_supplies[] = {
+	REGULATOR_SUPPLY("vdddo", "i2c-INT347E:00"),
 };
 
 static const struct regulator_init_data surface_go_tps68470_core_reg_init_data = {
@@ -86,6 +95,28 @@ static const struct regulator_init_data surface_go_tps68470_vsio_reg_init_data =
 	.consumer_supplies = int347a_vsio_consumer_supplies,
 };
 
+static const struct regulator_init_data surface_go_tps68470_aux1_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_aux1_consumer_supplies),
+	.consumer_supplies = int347a_aux1_consumer_supplies,
+};
+
+static const struct regulator_init_data surface_go_tps68470_aux2_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_aux2_consumer_supplies),
+	.consumer_supplies = int347a_aux2_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,
@@ -93,6 +124,8 @@ static const struct tps68470_regulator_platform_data surface_go_tps68470_pdata =
 		[TPS68470_VCM]  = &surface_go_tps68470_vcm_reg_init_data,
 		[TPS68470_VIO] = &surface_go_tps68470_vio_reg_init_data,
 		[TPS68470_VSIO] = &surface_go_tps68470_vsio_reg_init_data,
+		[TPS68470_AUX1] = &surface_go_tps68470_aux1_reg_init_data,
+		[TPS68470_AUX2] = &surface_go_tps68470_aux2_reg_init_data,
 	},
 };
 
-- 
2.25.1

Re: [PATCH 6/6] platform/x86: int3472: Add board data for Surface Go2 IR camera

[Hans de Goede]

Hi,

On 2/16/22 23:53, Daniel Scally wrote:
> Add the board data describing the regulators for the Microsoft
> Surface Go line's IR camera.
> 
> Signed-off-by: Daniel Scally <djrscally@gmail.com>

Thanks, patch looks good to me:

Reviewed-by: Hans de Goede <hdegoede@redhat.com>

Regards,

Hans


> ---
>  .../x86/intel/int3472/tps68470_board_data.c   | 33 +++++++++++++++++++
>  1 file changed, 33 insertions(+)
> 
> diff --git a/drivers/platform/x86/intel/int3472/tps68470_board_data.c b/drivers/platform/x86/intel/int3472/tps68470_board_data.c
> index 442a8a2de224..49a3591c6d85 100644
> --- a/drivers/platform/x86/intel/int3472/tps68470_board_data.c
> +++ b/drivers/platform/x86/intel/int3472/tps68470_board_data.c
> @@ -30,6 +30,15 @@ static struct regulator_consumer_supply int347a_vcm_consumer_supplies[] = {
>  static struct regulator_consumer_supply int347a_vsio_consumer_supplies[] = {
>  	REGULATOR_SUPPLY("dovdd", "i2c-INT347A:00"),
>  	REGULATOR_SUPPLY("vsio", "i2c-INT347A:00-VCM"),
> +	REGULATOR_SUPPLY("vddd", "i2c-INT347E:00"),
> +};
> +
> +static struct regulator_consumer_supply int347a_aux1_consumer_supplies[] = {
> +	REGULATOR_SUPPLY("vdda", "i2c-INT347E:00"),
> +};
> +
> +static struct regulator_consumer_supply int347a_aux2_consumer_supplies[] = {
> +	REGULATOR_SUPPLY("vdddo", "i2c-INT347E:00"),
>  };
>  
>  static const struct regulator_init_data surface_go_tps68470_core_reg_init_data = {
> @@ -86,6 +95,28 @@ static const struct regulator_init_data surface_go_tps68470_vsio_reg_init_data =
>  	.consumer_supplies = int347a_vsio_consumer_supplies,
>  };
>  
> +static const struct regulator_init_data surface_go_tps68470_aux1_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_aux1_consumer_supplies),
> +	.consumer_supplies = int347a_aux1_consumer_supplies,
> +};
> +
> +static const struct regulator_init_data surface_go_tps68470_aux2_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_aux2_consumer_supplies),
> +	.consumer_supplies = int347a_aux2_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,
> @@ -93,6 +124,8 @@ static const struct tps68470_regulator_platform_data surface_go_tps68470_pdata =
>  		[TPS68470_VCM]  = &surface_go_tps68470_vcm_reg_init_data,
>  		[TPS68470_VIO] = &surface_go_tps68470_vio_reg_init_data,
>  		[TPS68470_VSIO] = &surface_go_tps68470_vsio_reg_init_data,
> +		[TPS68470_AUX1] = &surface_go_tps68470_aux1_reg_init_data,
> +		[TPS68470_AUX2] = &surface_go_tps68470_aux2_reg_init_data,
>  	},
>  };
>  

Re: [PATCH 0/6] Add multiple-consumer support to int3472-tps68470 driver

[Hans de Goede]

Hi Dan,

On 2/16/22 23:52, Daniel Scally wrote:
> At the moment there are a few places in the int3472-tps68470 driver that are
> limited to just working with a single consuming device dependent on the PMIC.
> There are systems where multiple camera sensors share a single TPS68470, so
> we need to extend the driver to support them. This requires a couple of tweaks
> to the ACPI functions to fetch dependent devices, which also assumes that only
> a single dependent will be found.

This sounds great, thank you for your work on this. I have a bit of a backlog wrt
pdx86 patches to review and I try to through those in FIFO order to keep things fair,
so it may be a while (approx. 1-2 weeks) before I get around to reviewing
these.

Regards,

Hans


> 
> Hans - this (plus a series to media [1]) adds support for the Surface Go 2's
> IR camera...the regulator settings for the Go1/2/3 world facing camera are the
> same, so I'd expect them to match for the IR sensor too, which means it should
> enable support for your Go too.
> 
> Thanks
> Dan
> 
> 
> [1] https://lore.kernel.org/linux-media/20220215230737.1870630-1-djrscally@gmail.com/
> 
> Daniel Scally (6):
>   ACPI: scan: Add acpi_dev_get_next_consumer_dev()
>   ACPI: bus: Add iterator for dependent devices
>   platform/x86: int3472: Support multiple clock consumers
>   platform/x86: int3472: Add terminator to gpiod_lookup_table
>   platform/x86: int3472: Support multiple gpio lookups in board data
>   platform/x86: int3472: Add board data for Surface Go2 IR camera
> 
>  drivers/acpi/scan.c                           | 47 +++++++++--
>  drivers/clk/clk-tps68470.c                    | 13 +++-
>  drivers/platform/x86/intel/int3472/tps68470.c | 77 +++++++++++++++----
>  drivers/platform/x86/intel/int3472/tps68470.h |  3 +-
>  .../x86/intel/int3472/tps68470_board_data.c   | 58 +++++++++++++-
>  include/acpi/acpi_bus.h                       | 14 ++++
>  include/linux/platform_data/tps68470.h        |  7 +-
>  7 files changed, 188 insertions(+), 31 deletions(-)
> 

Re: [PATCH 0/6] Add multiple-consumer support to int3472-tps68470 driver

[Daniel Scally]

Hi Hans

On 17/02/2022 09:55, Hans de Goede wrote:
> Hi Dan,
>
> On 2/16/22 23:52, Daniel Scally wrote:
>> At the moment there are a few places in the int3472-tps68470 driver that are
>> limited to just working with a single consuming device dependent on the PMIC.
>> There are systems where multiple camera sensors share a single TPS68470, so
>> we need to extend the driver to support them. This requires a couple of tweaks
>> to the ACPI functions to fetch dependent devices, which also assumes that only
>> a single dependent will be found.
> This sounds great, thank you for your work on this. I have a bit of a backlog wrt
> pdx86 patches to review and I try to through those in FIFO order to keep things fair,
> so it may be a while (approx. 1-2 weeks) before I get around to reviewing
> these.


No problem, thanks very much

>
> Regards,
>
> Hans
>
>
>> Hans - this (plus a series to media [1]) adds support for the Surface Go 2's
>> IR camera...the regulator settings for the Go1/2/3 world facing camera are the
>> same, so I'd expect them to match for the IR sensor too, which means it should
>> enable support for your Go too.
>>
>> Thanks
>> Dan
>>
>>
>> [1] https://lore.kernel.org/linux-media/20220215230737.1870630-1-djrscally@gmail.com/
>>
>> Daniel Scally (6):
>>   ACPI: scan: Add acpi_dev_get_next_consumer_dev()
>>   ACPI: bus: Add iterator for dependent devices
>>   platform/x86: int3472: Support multiple clock consumers
>>   platform/x86: int3472: Add terminator to gpiod_lookup_table
>>   platform/x86: int3472: Support multiple gpio lookups in board data
>>   platform/x86: int3472: Add board data for Surface Go2 IR camera
>>
>>  drivers/acpi/scan.c                           | 47 +++++++++--
>>  drivers/clk/clk-tps68470.c                    | 13 +++-
>>  drivers/platform/x86/intel/int3472/tps68470.c | 77 +++++++++++++++----
>>  drivers/platform/x86/intel/int3472/tps68470.h |  3 +-
>>  .../x86/intel/int3472/tps68470_board_data.c   | 58 +++++++++++++-
>>  include/acpi/acpi_bus.h                       | 14 ++++
>>  include/linux/platform_data/tps68470.h        |  7 +-
>>  7 files changed, 188 insertions(+), 31 deletions(-)
>>

Re: [PATCH 0/6] Add multiple-consumer support to int3472-tps68470 driver

[Hans de Goede]

Hi,

On 2/16/22 23:52, Daniel Scally wrote:
> At the moment there are a few places in the int3472-tps68470 driver that are
> limited to just working with a single consuming device dependent on the PMIC.
> There are systems where multiple camera sensors share a single TPS68470, so
> we need to extend the driver to support them. This requires a couple of tweaks
> to the ACPI functions to fetch dependent devices, which also assumes that only
> a single dependent will be found.
> 
> Hans - this (plus a series to media [1]) adds support for the Surface Go 2's
> IR camera...the regulator settings for the Go1/2/3 world facing camera are the
> same, so I'd expect them to match for the IR sensor too, which means it should
> enable support for your Go too.

Thank you, this mostly looks good to me. I've added some small remarks but
nothing big stands out.

Rafael, I think it would be easiest for me to merge this entire series through
pdx86/for-next. May I have your ack for patches 1+2 to merge them through
the pdx86 tree?

Regards,

Hans



> [1] https://lore.kernel.org/linux-media/20220215230737.1870630-1-djrscally@gmail.com/
> 
> Daniel Scally (6):
>   ACPI: scan: Add acpi_dev_get_next_consumer_dev()
>   ACPI: bus: Add iterator for dependent devices
>   platform/x86: int3472: Support multiple clock consumers
>   platform/x86: int3472: Add terminator to gpiod_lookup_table
>   platform/x86: int3472: Support multiple gpio lookups in board data
>   platform/x86: int3472: Add board data for Surface Go2 IR camera
> 
>  drivers/acpi/scan.c                           | 47 +++++++++--
>  drivers/clk/clk-tps68470.c                    | 13 +++-
>  drivers/platform/x86/intel/int3472/tps68470.c | 77 +++++++++++++++----
>  drivers/platform/x86/intel/int3472/tps68470.h |  3 +-
>  .../x86/intel/int3472/tps68470_board_data.c   | 58 +++++++++++++-
>  include/acpi/acpi_bus.h                       | 14 ++++
>  include/linux/platform_data/tps68470.h        |  7 +-
>  7 files changed, 188 insertions(+), 31 deletions(-)
>