[RFC PATCH v2 0/3] Add regulator_lookup_list and API

[RFC PATCH v2 0/3] Add regulator_lookup_list and API

[Daniel Scally]

Hello all

This series adds a new list to the regulator core; regulator_lookup_list.
The intended purpose of this is to allow board files to pass instances of
struct regulator_init_data to regulators that are being registered by
_other_ pieces of code.

The problem this is intended to solve is where ACPI enumerates i2c devices
that provide one or more regulators, but which do not have any power
control methods defined in ACPI. This leaves the consumers of those
regulators unable to use them. With this series, the regulator drivers for
those i2c devices can bind and register regulators which will pick up the
init data as though it were parsed from devicetree. The new API emulates
gpiod_add_lookup_table() to allow early-running code to fill in that
init_data so that it's available by the time the i2c driver probes and
registers the regulators.

This is marked v2, because an earlier series [1] had the same aims albeit
using a totally different methodology. There's little commonality between
them, but I include the reference for context.

Thanks
Dan

[1] https://lore.kernel.org/platform-driver-x86/20210708224226.457224-1-djrscally@gmail.com/

Daniel Scally (3):
  regulator: core: Add regulator_lookup_list
  Documentation: power: Document regulator_lookup_list
  platform/surface: Add Surface Go 2 board file

 Documentation/power/regulator/machine.rst |  31 ++++++
 MAINTAINERS                               |   6 ++
 drivers/platform/surface/Kconfig          |  10 ++
 drivers/platform/surface/Makefile         |   1 +
 drivers/platform/surface/surface_go_2.c   |  97 ++++++++++++++++++
 drivers/regulator/core.c                  | 119 +++++++++++++++++++++-
 include/linux/regulator/machine.h         |  23 +++++
 7 files changed, 286 insertions(+), 1 deletion(-)
 create mode 100644 drivers/platform/surface/surface_go_2.c

-- 
2.25.1

[RFC PATCH v2 1/3] regulator: core: Add regulator_lookup_list

[Daniel Scally]

In some situations regulator devices can be enumerated via either
devicetree or ACPI and bound to regulator drivers but without any
init data being provided in firmware. This leaves their consumers
unable to acquire them via regulator_get().

To fix the issue, add the ability to register a lookup table to a
list within regulator core, which will allow board files to provide
init data via matching against the regulator name and device name in
the same fashion as the gpiod lookup table.

Signed-off-by: Daniel Scally <djrscally@gmail.com>
---
Changes in v2:
	- Patch introduced

 drivers/regulator/core.c          | 119 +++++++++++++++++++++++++++++-
 include/linux/regulator/machine.h |  23 ++++++
 2 files changed, 141 insertions(+), 1 deletion(-)

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index ca6caba8a191..6a6c1f34d9d6 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -36,6 +36,8 @@
 static DEFINE_WW_CLASS(regulator_ww_class);
 static DEFINE_MUTEX(regulator_nesting_mutex);
 static DEFINE_MUTEX(regulator_list_mutex);
+static DEFINE_MUTEX(regulator_lookup_mutex);
+static LIST_HEAD(regulator_lookup_list);
 static LIST_HEAD(regulator_map_list);
 static LIST_HEAD(regulator_ena_gpio_list);
 static LIST_HEAD(regulator_supply_alias_list);
@@ -1868,6 +1870,7 @@ static struct regulator_dev *regulator_lookup_by_name(const char *name)
 static struct regulator_dev *regulator_dev_lookup(struct device *dev,
 						  const char *supply)
 {
+	struct regulator_lookup *lookup;
 	struct regulator_dev *r = NULL;
 	struct device_node *node;
 	struct regulator_map *map;
@@ -1917,7 +1920,34 @@ static struct regulator_dev *regulator_dev_lookup(struct device *dev,
 	if (r)
 		return r;
 
-	return ERR_PTR(-ENODEV);
+	/*
+	 * The regulator might still exist and be called out in a lookup table,
+	 * but simply not yet have probed. Check to see if the consumer device
+	 * is referenced in any init data in the lookups.
+	 */
+
+	mutex_lock(&regulator_lookup_mutex);
+	list_for_each_entry(lookup, &regulator_lookup_list, list) {
+		struct regulator_init_data *init_data = &lookup->init_data;
+		unsigned int i;
+
+		if (!devname)
+			break;
+
+		for (i = 0; i < init_data->num_consumer_supplies; i++) {
+			if (strcmp(init_data->consumer_supplies[i].dev_name, devname))
+				continue;
+
+			r = ERR_PTR(-EPROBE_DEFER);
+			goto out;
+		}
+	}
+
+	r = ERR_PTR(-ENODEV);
+
+out:
+	mutex_unlock(&regulator_lookup_mutex);
+	return r;
 }
 
 static int regulator_resolve_supply(struct regulator_dev *rdev)
@@ -5302,6 +5332,54 @@ static struct regulator_coupler generic_regulator_coupler = {
 	.attach_regulator = generic_coupler_attach,
 };
 
+/**
+ * regulator_lookup_init_data - Check regulator_lookup_list for matching entries
+ * @desc:	Regulator desc containing name of the regulator
+ * @cfg:	Regulator config containing pointer to the registering device
+ *
+ * Calling this function scans the regulator_lookup_list and checks each entry
+ * to see if the .device_name and .regulator_name fields match the device name
+ * and regulator name contained in @cfg and @desc. If so, a pointer to the
+ * embedded &struct regulator_init_data is returned. No matches returns NULL.
+ */
+struct regulator_init_data *
+regulator_lookup_init_data(const struct regulator_desc *desc,
+			   const struct regulator_config *cfg)
+{
+	struct regulator_lookup *lookup;
+
+	if (!desc || !cfg || !cfg->dev)
+		return NULL;
+
+	mutex_lock(&regulator_lookup_mutex);
+
+	list_for_each_entry(lookup, &regulator_lookup_list, list) {
+		/*
+		 * We need the lookup to have at least a device_name or there's
+		 * no guarantee of a match, and regulator_register() checks to
+		 * make sure that desc->name is not null, so any entry with
+		 * either field null is invalid.
+		 */
+		if (!lookup->device_name || !lookup->regulator_name)
+			continue;
+
+		if (strcmp(lookup->device_name, dev_name(cfg->dev)))
+			continue;
+
+		if (strcmp(lookup->regulator_name, desc->name))
+			continue;
+
+		goto found;
+	}
+
+	lookup = NULL;
+
+found:
+	mutex_unlock(&regulator_lookup_mutex);
+
+	return lookup ? &lookup->init_data : NULL;
+}
+
 /**
  * regulator_register - register regulator
  * @regulator_desc: regulator to register
@@ -5386,6 +5464,9 @@ regulator_register(const struct regulator_desc *regulator_desc,
 	init_data = regulator_of_get_init_data(dev, regulator_desc, config,
 					       &rdev->dev.of_node);
 
+	if (!init_data)
+		init_data = regulator_lookup_init_data(regulator_desc, config);
+
 	/*
 	 * Sometimes not all resources are probed already so we need to take
 	 * that into account. This happens most the time if the ena_gpiod comes
@@ -5740,6 +5821,42 @@ void *regulator_get_init_drvdata(struct regulator_init_data *reg_init_data)
 }
 EXPORT_SYMBOL_GPL(regulator_get_init_drvdata);
 
+/**
+ * regulator_add_lookup - Add an entry to regulator_lookup_list
+ * @lookup:	Pointer to the &struct regulator_lookup to add to the list
+ *
+ * This function can be used in board files to add an entry to the
+ * regulator_lookup_list. During regulator_register(), entries will be checked
+ * for matching device name and regulator name and when matching, the associated
+ * &struct regulator_init_data will be used.
+ */
+void regulator_add_lookup(struct regulator_lookup *lookup)
+{
+	mutex_lock(&regulator_lookup_mutex);
+
+	list_add_tail(&lookup->list, &regulator_lookup_list);
+
+	mutex_unlock(&regulator_lookup_mutex);
+}
+EXPORT_SYMBOL_GPL(regulator_add_lookup);
+
+/**
+ * regulator_remove_lookup - Remove an entry from regulator_lookup_list
+ * @lookup:	Pointer to the &struct regulator_lookup to remove from the list
+ *
+ * Calling this function will remove the passed regulator_lookup from the list,
+ * intended for error handling paths in board files.
+ */
+void regulator_remove_lookup(struct regulator_lookup *lookup)
+{
+	mutex_lock(&regulator_lookup_mutex);
+
+	list_del(&lookup->list);
+
+	mutex_unlock(&regulator_lookup_mutex);
+}
+EXPORT_SYMBOL_GPL(regulator_remove_lookup);
+
 #ifdef CONFIG_DEBUG_FS
 static int supply_map_show(struct seq_file *sf, void *data)
 {
diff --git a/include/linux/regulator/machine.h b/include/linux/regulator/machine.h
index 68b4a514a410..d1b62a3962d8 100644
--- a/include/linux/regulator/machine.h
+++ b/include/linux/regulator/machine.h
@@ -12,6 +12,7 @@
 #ifndef __LINUX_REGULATOR_MACHINE_H_
 #define __LINUX_REGULATOR_MACHINE_H_
 
+#include <linux/list.h>
 #include <linux/regulator/consumer.h>
 #include <linux/suspend.h>
 
@@ -272,12 +273,34 @@ struct regulator_init_data {
 	void *driver_data;	/* core does not touch this */
 };
 
+/**
+ * struct regulator_lookup - lookup table entry for regulator_init_data
+ *
+ * @device_name:	The name of the device from regulator_config.dev
+ * @regulator_name:	The name of the regulator from the &struct regulator_desc
+ *			to match to during regulator_register()
+ * @init_data:		An instance of &struct regulator_init_data that you
+ *			wish to pass to the regulator during regulator_register()
+ */
+struct regulator_lookup {
+	const char *device_name;
+	const char *regulator_name;
+
+	struct regulator_init_data init_data;
+
+	struct list_head list;
+};
+
 #ifdef CONFIG_REGULATOR
 void regulator_has_full_constraints(void);
+void regulator_add_lookup(struct regulator_lookup *lookup);
+void regulator_remove_lookup(struct regulator_lookup *lookup);
 #else
 static inline void regulator_has_full_constraints(void)
 {
 }
+static inline void regulator_add_lookup(struct regulator_lookup *lookup) { }
+static inline void regulator_remove_lookup(struct regulator_lookup *lookup) { }
 #endif
 
 static inline int regulator_suspend_prepare(suspend_state_t state)
-- 
2.25.1

[RFC PATCH v2 2/3] Documentation: power: Document regulator_lookup_list

[Daniel Scally]

Add some explanatory documentation to machine.rst detailing when and
how to use the new regulator lookup functionality.

Signed-off-by: Daniel Scally <djrscally@gmail.com>
---
Changes in v2:
	- Patch introduced

 Documentation/power/regulator/machine.rst | 31 +++++++++++++++++++++++
 1 file changed, 31 insertions(+)

diff --git a/Documentation/power/regulator/machine.rst b/Documentation/power/regulator/machine.rst
index 22fffefaa3ad..7ad64e59b649 100644
--- a/Documentation/power/regulator/machine.rst
+++ b/Documentation/power/regulator/machine.rst
@@ -95,3 +95,34 @@ Finally the regulator devices must be registered in the usual manner::
 
   /* register regulator 2 device */
   platform_device_register(&regulator_devices[1]);
+
+Alternatively, if the struct regulator_init_data is being created in a process
+distinct from the registration of the regulator devices themselves (for example
+if the regulator devices themselves are enumerated through devicetree or ACPI
+but the system lacks regulator_init_data in firmware), then it must be added to
+a lookup table that will be checked when the regulator devices are registered.
+
+  static struct regulator_lookup regulator1_lookup = {
+	.device_name = "regulator.1",
+	.regulator_name = "Regulator-1",
+	.init_data = {
+		.constraints = {
+			.name = "Regulator-1",
+			.min_uV = 3300000,
+			.max_uV = 3300000,
+			.valid_ops_mask = REGULATOR_MODE_NORMAL,
+		},
+		.num_consumer_supplies = ARRAY_SIZE(regulator1_consumers),
+		.consumer_supplies = regulator1_consumers,
+	},
+  };
+
+  regulator_add_lookup(&regulator1_lookup);
+
+The .device_name field should match that of the struct device that the
+regulator driver will bind to. The .regulator_name field should match the name
+field in the struct regulator_desc that is used to register the regulator that
+this regulator_init_data is intended to be mapped to. Neither field can be null
+since regulator_register() will not allow the name field of the regulator_desc
+to be null, and without at least the device name we cannot guarantee we're
+passing the init_data to the right regulator.
-- 
2.25.1

[RFC PATCH v2 3/3] platform/surface: Add Surface Go 2 board file

[Daniel Scally]

The DSDT tables on the Surface Go 2 contain no power control methods for
the world facing camera, so the regulator, gpio and clk frameworks have no
way to discover the appropriate connections and settings.

To compensate for the shortcomings, define the connections and other
parameters in a board file for this device.

Signed-off-by: Daniel Scally <djrscally@gmail.com>
---
Changes in v2:
	- Re-written to use regulator_add_lookup() instead of the software
	node api from v1.
	- Added a dmi check to prevent the lookups from being added where the
	platform running the code is not a Microsoft Surface Go 2 (Laurent)

 MAINTAINERS                             |  6 ++
 drivers/platform/surface/Kconfig        | 10 +++
 drivers/platform/surface/Makefile       |  1 +
 drivers/platform/surface/surface_go_2.c | 97 +++++++++++++++++++++++++
 4 files changed, 114 insertions(+)
 create mode 100644 drivers/platform/surface/surface_go_2.c

diff --git a/MAINTAINERS b/MAINTAINERS
index dc24e97d5386..891189cecd51 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -12329,6 +12329,12 @@ F:	Documentation/driver-api/surface_aggregator/clients/dtx.rst
 F:	drivers/platform/surface/surface_dtx.c
 F:	include/uapi/linux/surface_aggregator/dtx.h
 
+MICROSOFT SURFACE GO 2 SUPPORT
+M:	Daniel Scally <djrscally@gmail.com>
+L:	platform-driver-x86@vger.kernel.org
+S:	Maintained
+F:	drivers/platform/surface/surface_go_2.c
+
 MICROSOFT SURFACE GPE LID SUPPORT DRIVER
 M:	Maximilian Luz <luzmaximilian@gmail.com>
 L:	platform-driver-x86@vger.kernel.org
diff --git a/drivers/platform/surface/Kconfig b/drivers/platform/surface/Kconfig
index 3105f651614f..0c55bd531592 100644
--- a/drivers/platform/surface/Kconfig
+++ b/drivers/platform/surface/Kconfig
@@ -124,6 +124,16 @@ config SURFACE_DTX
 	  Aggregator Bus (i.e. CONFIG_SURFACE_AGGREGATOR_BUS=n). In that case,
 	  some devices, specifically the Surface Book 3, will not be supported.
 
+config SURFACE_GO_2
+	bool "Surface Go 2 Specific Driver"
+	help
+	  This board file performs some platform specific configuration to help
+	  the regulator, gpio and clk frameworks link those resources to the
+	  consuming devices - specifically the world facing camera.
+
+	  Select Y here if your device is a Microsoft Surface Go 2, otherwise
+	  select N.
+
 config SURFACE_GPE
 	tristate "Surface GPE/Lid Support Driver"
 	depends on DMI
diff --git a/drivers/platform/surface/Makefile b/drivers/platform/surface/Makefile
index 32889482de55..9f858d2a3d77 100644
--- a/drivers/platform/surface/Makefile
+++ b/drivers/platform/surface/Makefile
@@ -12,6 +12,7 @@ obj-$(CONFIG_SURFACE_AGGREGATOR)	+= aggregator/
 obj-$(CONFIG_SURFACE_AGGREGATOR_CDEV)	+= surface_aggregator_cdev.o
 obj-$(CONFIG_SURFACE_AGGREGATOR_REGISTRY) += surface_aggregator_registry.o
 obj-$(CONFIG_SURFACE_DTX)		+= surface_dtx.o
+obj-$(CONFIG_SURFACE_GO_2)		+= surface_go_2.o
 obj-$(CONFIG_SURFACE_GPE)		+= surface_gpe.o
 obj-$(CONFIG_SURFACE_HOTPLUG)		+= surface_hotplug.o
 obj-$(CONFIG_SURFACE_PLATFORM_PROFILE)	+= surface_platform_profile.o
diff --git a/drivers/platform/surface/surface_go_2.c b/drivers/platform/surface/surface_go_2.c
new file mode 100644
index 000000000000..31fd6a3ca4aa
--- /dev/null
+++ b/drivers/platform/surface/surface_go_2.c
@@ -0,0 +1,97 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Author: Dan Scally <djrscally@gmail.com> */
+
+#include <linux/dmi.h>
+#include <linux/gpio/machine.h>
+#include <linux/init.h>
+#include <linux/kernel.h>
+#include <linux/regulator/machine.h>
+
+static const struct dmi_system_id surface_go_2_dmi_table[] = {
+	{
+		.matches = {
+			DMI_MATCH(DMI_SYS_VENDOR, "Microsoft Corporation"),
+			DMI_MATCH(DMI_PRODUCT_NAME, "Surface Go 2"),
+		},
+	},
+	{ }
+};
+
+static struct regulator_consumer_supply ana_consumer_supplies[] = {
+	REGULATOR_SUPPLY("avdd", "i2c-INT347A:00"),
+};
+
+static struct regulator_lookup ana_lookup = {
+	.device_name = "i2c-INT3472:05",
+	.regulator_name = "ANA",
+	.init_data = {
+		.constraints = {
+			.min_uV = 2815200,
+			.max_uV = 2815200,
+			.apply_uV = 1,
+			.valid_ops_mask = REGULATOR_CHANGE_STATUS,
+		},
+		.num_consumer_supplies = ARRAY_SIZE(ana_consumer_supplies),
+		.consumer_supplies = ana_consumer_supplies,
+	},
+};
+
+static struct regulator_consumer_supply vsio_consumer_supplies[] = {
+	REGULATOR_SUPPLY("dovdd", "i2c-INT347A:00"),
+};
+
+static struct regulator_lookup vsio_lookup = {
+	.device_name = "i2c-INT3472:05",
+	.regulator_name = "VSIO",
+	.init_data = {
+		.constraints = {
+			.min_uV = 1800600,
+			.max_uV = 1800600,
+			.apply_uV = 1,
+			.valid_ops_mask = REGULATOR_CHANGE_STATUS,
+		},
+		.num_consumer_supplies = ARRAY_SIZE(vsio_consumer_supplies),
+		.consumer_supplies = vsio_consumer_supplies,
+	},
+};
+
+static struct regulator_consumer_supply core_consumer_supplies[] = {
+	REGULATOR_SUPPLY("dvdd", "i2c-INT347A:00"),
+};
+
+static struct regulator_lookup core_lookup = {
+	.device_name = "i2c-INT3472:05",
+	.regulator_name = "CORE",
+	.init_data = {
+		.constraints = {
+			.min_uV = 1200000,
+			.max_uV = 1200000,
+			.apply_uV = 1,
+			.valid_ops_mask = REGULATOR_CHANGE_STATUS,
+		},
+		.num_consumer_supplies = ARRAY_SIZE(core_consumer_supplies),
+		.consumer_supplies = core_consumer_supplies,
+	},
+};
+
+static struct gpiod_lookup_table surface_go_2_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 int __init surface_go_2_init(void)
+{
+	if (!dmi_check_system(surface_go_2_dmi_table))
+		return -EINVAL;
+
+	regulator_add_lookup(&ana_lookup);
+	regulator_add_lookup(&vsio_lookup);
+	regulator_add_lookup(&core_lookup);
+	gpiod_add_lookup_table(&surface_go_2_gpios);
+
+	return 0;
+}
+device_initcall(surface_go_2_init);
-- 
2.25.1

Re: [RFC PATCH v2 1/3] regulator: core: Add regulator_lookup_list

[Mark Brown]

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

On Wed, Aug 25, 2021 at 12:06:18AM +0100, Daniel Scally wrote:
> In some situations regulator devices can be enumerated via either
> devicetree or ACPI and bound to regulator drivers but without any
> init data being provided in firmware. This leaves their consumers
> unable to acquire them via regulator_get().
> 
> To fix the issue, add the ability to register a lookup table to a
> list within regulator core, which will allow board files to provide
> init data via matching against the regulator name and device name in
> the same fashion as the gpiod lookup table.

This is the wrong level to do this I think, this is a generic problem
that affects all kinds of platform data so if we're not going to scatter
DMI quirks throughout the drivers like we currently do then we should
have a way for boards to just store generic platform data for a device
and then have that platform data joined up with the device later.  This
could for example also be used by all the laptop audio subsystems which
need DMI quirk tables in drivers for their components to figure out how
they're wired up and avoids the need to go through subsystems adding new
APIs.

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

Re: [RFC PATCH v2 1/3] regulator: core: Add regulator_lookup_list

[Andy Shevchenko]

On Wed, Aug 25, 2021 at 1:34 PM Mark Brown <broonie@kernel.org> wrote:
>
> On Wed, Aug 25, 2021 at 12:06:18AM +0100, Daniel Scally wrote:
> > In some situations regulator devices can be enumerated via either
> > devicetree or ACPI and bound to regulator drivers but without any
> > init data being provided in firmware. This leaves their consumers
> > unable to acquire them via regulator_get().
> >
> > To fix the issue, add the ability to register a lookup table to a
> > list within regulator core, which will allow board files to provide
> > init data via matching against the regulator name and device name in
> > the same fashion as the gpiod lookup table.
>
> This is the wrong level to do this I think, this is a generic problem
> that affects all kinds of platform data so if we're not going to scatter
> DMI quirks throughout the drivers like we currently do then we should
> have a way for boards to just store generic platform data for a device
> and then have that platform data joined up with the device later.  This
> could for example also be used by all the laptop audio subsystems which
> need DMI quirk tables in drivers for their components to figure out how
> they're wired up and avoids the need to go through subsystems adding new
> APIs.

What you are describing sounds similar to what we have, i.e. fwnode graph.
That's how v4l2 describes complex connections between devices in the
camera world.

But IIRC you don't like this idea to be present with regulators (as
per v1 of this series).

So, what do you propose then?


--
With Best Regards,
Andy Shevchenko

Re: [RFC PATCH v2 1/3] regulator: core: Add regulator_lookup_list

[Mark Brown]

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

On Wed, Aug 25, 2021 at 02:10:05PM +0300, Andy Shevchenko wrote:
> On Wed, Aug 25, 2021 at 1:34 PM Mark Brown <broonie@kernel.org> wrote:

> > DMI quirks throughout the drivers like we currently do then we should
> > have a way for boards to just store generic platform data for a device
> > and then have that platform data joined up with the device later.  This

> What you are describing sounds similar to what we have, i.e. fwnode graph.
> That's how v4l2 describes complex connections between devices in the
> camera world.

> But IIRC you don't like this idea to be present with regulators (as
> per v1 of this series).

No, what was proposed for regulator was to duplicate all the the DT
binding code in the regulator framework so it parses fwnodes then have
an API for encoding fwnodes from C data structures at runtime.  The bit
where the data gets joined up with the devices isn't the problem, it's
the duplication and fragility introduced by encoding everything into
an intermediate representation that has no purpose and passing that
around which is the problem.

> So, what do you propose then?

I propose what I suggested above, providing a way to attach platform
data to firmware provided devices.  Don't bother with an intermediate
encoding, just use platform data.  Or just put quirks in the drivers
like all the other systems using ACPI for platforms where it's not a
good fit.

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

Re: [RFC PATCH v2 1/3] regulator: core: Add regulator_lookup_list

[Andy Shevchenko]

On Wed, Aug 25, 2021 at 2:30 PM Mark Brown <broonie@kernel.org> wrote:
> On Wed, Aug 25, 2021 at 02:10:05PM +0300, Andy Shevchenko wrote:
> > On Wed, Aug 25, 2021 at 1:34 PM Mark Brown <broonie@kernel.org> wrote:
>
> > > DMI quirks throughout the drivers like we currently do then we should
> > > have a way for boards to just store generic platform data for a device
> > > and then have that platform data joined up with the device later.  This
>
> > What you are describing sounds similar to what we have, i.e. fwnode graph.
> > That's how v4l2 describes complex connections between devices in the
> > camera world.
>
> > But IIRC you don't like this idea to be present with regulators (as
> > per v1 of this series).
>
> No, what was proposed for regulator was to duplicate all the the DT
> binding code in the regulator framework so it parses fwnodes then have
> an API for encoding fwnodes from C data structures at runtime.  The bit
> where the data gets joined up with the devices isn't the problem, it's
> the duplication and fragility introduced by encoding everything into
> an intermediate representation that has no purpose and passing that
> around which is the problem.

The whole exercise with swnode is to minimize the driver intrusion and
evolving a unified way for (some) of the device properties. V4L2 won't
like what you are suggesting exactly because they don't like the idea
of spreading the board code over the drivers. In some cases it might
even be not so straightforward and easy.

Laurent, do I understand correctly the v4l2 expectations?

> > So, what do you propose then?
>
> I propose what I suggested above, providing a way to attach platform
> data to firmware provided devices.  Don't bother with an intermediate
> encoding, just use platform data.  Or just put quirks in the drivers
> like all the other systems using ACPI for platforms where it's not a
> good fit.

-- 
With Best Regards,
Andy Shevchenko

Re: [RFC PATCH v2 1/3] regulator: core: Add regulator_lookup_list

[Mark Brown]

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

On Wed, Aug 25, 2021 at 03:26:37PM +0300, Andy Shevchenko wrote:
> On Wed, Aug 25, 2021 at 2:30 PM Mark Brown <broonie@kernel.org> wrote:

> > No, what was proposed for regulator was to duplicate all the the DT
> > binding code in the regulator framework so it parses fwnodes then have
> > an API for encoding fwnodes from C data structures at runtime.  The bit
> > where the data gets joined up with the devices isn't the problem, it's
> > the duplication and fragility introduced by encoding everything into
> > an intermediate representation that has no purpose and passing that
> > around which is the problem.

> The whole exercise with swnode is to minimize the driver intrusion and
> evolving a unified way for (some) of the device properties. V4L2 won't

The practical implementation for regulators was to duplicate a
substantial amount of code in the core in order to give us a less type
safe and more indirect way of passing data from onen C file in the
kernel to another.  This proposal is a lot better in that it uses the
existing init_data and avoids the huge amounts of duplication, it's just
not clear from the changelog why it's doing this in a regulator specific
manner.

*Please* stop trying to force swnodes in everywhere, take on board the
feedback about why the swnode implementation is completely inappropriate
for regulators.  I don't understand why you continue to push this so
hard.  swnodes and fwnodes are a solution to a specific problem, they're
not the answer to every problem out there and having to rehash this
continually is getting in the way of actually discussing practical
workarounds for these poorly implemented ACPI platforms.

> like what you are suggesting exactly because they don't like the idea
> of spreading the board code over the drivers. In some cases it might
> even be not so straightforward and easy.

> Laurent, do I understand correctly the v4l2 expectations?

There will be some cases where swnodes make sense, for example where the
data is going to be read through the fwnode API since the binding is
firmware neutral which I think is the v4l case.  On the other hand
having a direct C representation is a very common way of implementing
DMI quirk tables, and we have things like the regulator API where
there's off the shelf platform data support and we actively don't want
to support fwnode.

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

Re: [RFC PATCH v2 1/3] regulator: core: Add regulator_lookup_list

[Laurent Pinchart]

Hello,

CC'ing Sakari.

On Wed, Aug 25, 2021 at 02:11:39PM +0100, Mark Brown wrote:
> On Wed, Aug 25, 2021 at 03:26:37PM +0300, Andy Shevchenko wrote:
> > On Wed, Aug 25, 2021 at 2:30 PM Mark Brown <broonie@kernel.org> wrote:
> 
> > > No, what was proposed for regulator was to duplicate all the the DT
> > > binding code in the regulator framework so it parses fwnodes then have
> > > an API for encoding fwnodes from C data structures at runtime.  The bit
> > > where the data gets joined up with the devices isn't the problem, it's
> > > the duplication and fragility introduced by encoding everything into
> > > an intermediate representation that has no purpose and passing that
> > > around which is the problem.
> 
> > The whole exercise with swnode is to minimize the driver intrusion and
> > evolving a unified way for (some) of the device properties. V4L2 won't
> 
> The practical implementation for regulators was to duplicate a
> substantial amount of code in the core in order to give us a less type
> safe and more indirect way of passing data from onen C file in the
> kernel to another.  This proposal is a lot better in that it uses the
> existing init_data and avoids the huge amounts of duplication, it's just
> not clear from the changelog why it's doing this in a regulator specific
> manner.
> 
> *Please* stop trying to force swnodes in everywhere, take on board the
> feedback about why the swnode implementation is completely inappropriate
> for regulators.  I don't understand why you continue to push this so
> hard.  swnodes and fwnodes are a solution to a specific problem, they're
> not the answer to every problem out there and having to rehash this
> continually is getting in the way of actually discussing practical
> workarounds for these poorly implemented ACPI platforms.
> 
> > like what you are suggesting exactly because they don't like the idea
> > of spreading the board code over the drivers. In some cases it might
> > even be not so straightforward and easy.
> 
> > Laurent, do I understand correctly the v4l2 expectations?
> 
> There will be some cases where swnodes make sense, for example where the
> data is going to be read through the fwnode API since the binding is
> firmware neutral which I think is the v4l case.  On the other hand
> having a direct C representation is a very common way of implementing
> DMI quirk tables, and we have things like the regulator API where
> there's off the shelf platform data support and we actively don't want
> to support fwnode.

From a camera sensor point of view, we want to avoid code duplication.
Having to look for regulators using OF lookups *and* platform data in
every single sensor driver is not a good solution. This means that, from
a camera sensor driver point of view, we want to call regulator_get()
(or the devm_ version) with a name, without caring about who establishes
the mapping and how the lookup is performed. I don't care much
personally if this would be implemented through swnode or a different
mechanism, as long as the implementation can be centralized.

-- 
Regards,

Laurent Pinchart

Re: [RFC PATCH v2 1/3] regulator: core: Add regulator_lookup_list

[Laurent Pinchart]

On Wed, Aug 25, 2021 at 04:59:36PM +0300, Laurent Pinchart wrote:
> Hello,
> 
> CC'ing Sakari.

With Sakari's correct address.

> On Wed, Aug 25, 2021 at 02:11:39PM +0100, Mark Brown wrote:
> > On Wed, Aug 25, 2021 at 03:26:37PM +0300, Andy Shevchenko wrote:
> > > On Wed, Aug 25, 2021 at 2:30 PM Mark Brown <broonie@kernel.org> wrote:
> > 
> > > > No, what was proposed for regulator was to duplicate all the the DT
> > > > binding code in the regulator framework so it parses fwnodes then have
> > > > an API for encoding fwnodes from C data structures at runtime.  The bit
> > > > where the data gets joined up with the devices isn't the problem, it's
> > > > the duplication and fragility introduced by encoding everything into
> > > > an intermediate representation that has no purpose and passing that
> > > > around which is the problem.
> > 
> > > The whole exercise with swnode is to minimize the driver intrusion and
> > > evolving a unified way for (some) of the device properties. V4L2 won't
> > 
> > The practical implementation for regulators was to duplicate a
> > substantial amount of code in the core in order to give us a less type
> > safe and more indirect way of passing data from onen C file in the
> > kernel to another.  This proposal is a lot better in that it uses the
> > existing init_data and avoids the huge amounts of duplication, it's just
> > not clear from the changelog why it's doing this in a regulator specific
> > manner.
> > 
> > *Please* stop trying to force swnodes in everywhere, take on board the
> > feedback about why the swnode implementation is completely inappropriate
> > for regulators.  I don't understand why you continue to push this so
> > hard.  swnodes and fwnodes are a solution to a specific problem, they're
> > not the answer to every problem out there and having to rehash this
> > continually is getting in the way of actually discussing practical
> > workarounds for these poorly implemented ACPI platforms.
> > 
> > > like what you are suggesting exactly because they don't like the idea
> > > of spreading the board code over the drivers. In some cases it might
> > > even be not so straightforward and easy.
> > 
> > > Laurent, do I understand correctly the v4l2 expectations?
> > 
> > There will be some cases where swnodes make sense, for example where the
> > data is going to be read through the fwnode API since the binding is
> > firmware neutral which I think is the v4l case.  On the other hand
> > having a direct C representation is a very common way of implementing
> > DMI quirk tables, and we have things like the regulator API where
> > there's off the shelf platform data support and we actively don't want
> > to support fwnode.
> 
> From a camera sensor point of view, we want to avoid code duplication.
> Having to look for regulators using OF lookups *and* platform data in
> every single sensor driver is not a good solution. This means that, from
> a camera sensor driver point of view, we want to call regulator_get()
> (or the devm_ version) with a name, without caring about who establishes
> the mapping and how the lookup is performed. I don't care much
> personally if this would be implemented through swnode or a different
> mechanism, as long as the implementation can be centralized.

-- 
Regards,

Laurent Pinchart

Re: [RFC PATCH v2 1/3] regulator: core: Add regulator_lookup_list

[Daniel Scally]

Hi Laurent

On 25/08/2021 14:59, Laurent Pinchart wrote:
> Hello,
>
> CC'ing Sakari.
>
> On Wed, Aug 25, 2021 at 02:11:39PM +0100, Mark Brown wrote:
>> On Wed, Aug 25, 2021 at 03:26:37PM +0300, Andy Shevchenko wrote:
>>> On Wed, Aug 25, 2021 at 2:30 PM Mark Brown <broonie@kernel.org> wrote:
>>>> No, what was proposed for regulator was to duplicate all the the DT
>>>> binding code in the regulator framework so it parses fwnodes then have
>>>> an API for encoding fwnodes from C data structures at runtime.  The bit
>>>> where the data gets joined up with the devices isn't the problem, it's
>>>> the duplication and fragility introduced by encoding everything into
>>>> an intermediate representation that has no purpose and passing that
>>>> around which is the problem.
>>> The whole exercise with swnode is to minimize the driver intrusion and
>>> evolving a unified way for (some) of the device properties. V4L2 won't
>> The practical implementation for regulators was to duplicate a
>> substantial amount of code in the core in order to give us a less type
>> safe and more indirect way of passing data from onen C file in the
>> kernel to another.  This proposal is a lot better in that it uses the
>> existing init_data and avoids the huge amounts of duplication, it's just
>> not clear from the changelog why it's doing this in a regulator specific
>> manner.
>>
>> *Please* stop trying to force swnodes in everywhere, take on board the
>> feedback about why the swnode implementation is completely inappropriate
>> for regulators.  I don't understand why you continue to push this so
>> hard.  swnodes and fwnodes are a solution to a specific problem, they're
>> not the answer to every problem out there and having to rehash this
>> continually is getting in the way of actually discussing practical
>> workarounds for these poorly implemented ACPI platforms.
>>
>>> like what you are suggesting exactly because they don't like the idea
>>> of spreading the board code over the drivers. In some cases it might
>>> even be not so straightforward and easy.
>>> Laurent, do I understand correctly the v4l2 expectations?
>> There will be some cases where swnodes make sense, for example where the
>> data is going to be read through the fwnode API since the binding is
>> firmware neutral which I think is the v4l case.  On the other hand
>> having a direct C representation is a very common way of implementing
>> DMI quirk tables, and we have things like the regulator API where
>> there's off the shelf platform data support and we actively don't want
>> to support fwnode.
> From a camera sensor point of view, we want to avoid code duplication.
> Having to look for regulators using OF lookups *and* platform data in
> every single sensor driver is not a good solution. This means that, from
> a camera sensor driver point of view, we want to call regulator_get()
> (or the devm_ version) with a name, without caring about who establishes
> the mapping and how the lookup is performed. I don't care much
> personally if this would be implemented through swnode or a different
> mechanism, as long as the implementation can be centralized.

I think rather than sensor drivers, the idea would be just to have the
tps68470-regulator driver check platform data for the init_data instead,
like the tps65023-regulator driver [1] does. I'm sure that'll work, but
it's not particularly centralized from the regulator driver's point of
view.


[1]
https://elixir.bootlin.com/linux/latest/source/drivers/regulator/tps65023-regulator.c#L268

Re: [RFC PATCH v2 1/3] regulator: core: Add regulator_lookup_list

[Laurent Pinchart]

Hi Dan,

On Wed, Aug 25, 2021 at 03:12:25PM +0100, Daniel Scally wrote:
> On 25/08/2021 14:59, Laurent Pinchart wrote:
> > Hello,
> >
> > CC'ing Sakari.
> >
> > On Wed, Aug 25, 2021 at 02:11:39PM +0100, Mark Brown wrote:
> >> On Wed, Aug 25, 2021 at 03:26:37PM +0300, Andy Shevchenko wrote:
> >>> On Wed, Aug 25, 2021 at 2:30 PM Mark Brown <broonie@kernel.org> wrote:
> >>>> No, what was proposed for regulator was to duplicate all the the DT
> >>>> binding code in the regulator framework so it parses fwnodes then have
> >>>> an API for encoding fwnodes from C data structures at runtime.  The bit
> >>>> where the data gets joined up with the devices isn't the problem, it's
> >>>> the duplication and fragility introduced by encoding everything into
> >>>> an intermediate representation that has no purpose and passing that
> >>>> around which is the problem.
> >>> The whole exercise with swnode is to minimize the driver intrusion and
> >>> evolving a unified way for (some) of the device properties. V4L2 won't
> >> The practical implementation for regulators was to duplicate a
> >> substantial amount of code in the core in order to give us a less type
> >> safe and more indirect way of passing data from onen C file in the
> >> kernel to another.  This proposal is a lot better in that it uses the
> >> existing init_data and avoids the huge amounts of duplication, it's just
> >> not clear from the changelog why it's doing this in a regulator specific
> >> manner.
> >>
> >> *Please* stop trying to force swnodes in everywhere, take on board the
> >> feedback about why the swnode implementation is completely inappropriate
> >> for regulators.  I don't understand why you continue to push this so
> >> hard.  swnodes and fwnodes are a solution to a specific problem, they're
> >> not the answer to every problem out there and having to rehash this
> >> continually is getting in the way of actually discussing practical
> >> workarounds for these poorly implemented ACPI platforms.
> >>
> >>> like what you are suggesting exactly because they don't like the idea
> >>> of spreading the board code over the drivers. In some cases it might
> >>> even be not so straightforward and easy.
> >>> Laurent, do I understand correctly the v4l2 expectations?
> >> There will be some cases where swnodes make sense, for example where the
> >> data is going to be read through the fwnode API since the binding is
> >> firmware neutral which I think is the v4l case.  On the other hand
> >> having a direct C representation is a very common way of implementing
> >> DMI quirk tables, and we have things like the regulator API where
> >> there's off the shelf platform data support and we actively don't want
> >> to support fwnode.
> > From a camera sensor point of view, we want to avoid code duplication.
> > Having to look for regulators using OF lookups *and* platform data in
> > every single sensor driver is not a good solution. This means that, from
> > a camera sensor driver point of view, we want to call regulator_get()
> > (or the devm_ version) with a name, without caring about who establishes
> > the mapping and how the lookup is performed. I don't care much
> > personally if this would be implemented through swnode or a different
> > mechanism, as long as the implementation can be centralized.
> 
> I think rather than sensor drivers, the idea would be just to have the
> tps68470-regulator driver check platform data for the init_data instead,
> like the tps65023-regulator driver [1] does. I'm sure that'll work, but
> it's not particularly centralized from the regulator driver's point of
> view.

That would obviously be less of an issue from a V4L2 point of view :-)
Given that the situation we're facing doesn't affect (to our knowledge)
a very large number of regulators, it may not be too bad in practice. If
I were to maintain the regulator subsystem I'd probably want a
centralized implementation there, but that's certainly a personal
preference, at least partly.

On a side note, this RFC looks quite similar to what the GPIO subsystem
does, which I personally consider nice as differences between regulator
and GPIO in these areas are confusing for users.

> [1]
> https://elixir.bootlin.com/linux/latest/source/drivers/regulator/tps65023-regulator.c#L268

-- 
Regards,

Laurent Pinchart

Re: [RFC PATCH v2 1/3] regulator: core: Add regulator_lookup_list

[Andy Shevchenko]

+Cc: Heikki

On Wed, Aug 25, 2021 at 5:03 PM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
> On Wed, Aug 25, 2021 at 04:59:36PM +0300, Laurent Pinchart wrote:
> > On Wed, Aug 25, 2021 at 02:11:39PM +0100, Mark Brown wrote:
> > > On Wed, Aug 25, 2021 at 03:26:37PM +0300, Andy Shevchenko wrote:
> > > > On Wed, Aug 25, 2021 at 2:30 PM Mark Brown <broonie@kernel.org> wrote:
> > >
> > > > > No, what was proposed for regulator was to duplicate all the the DT
> > > > > binding code in the regulator framework so it parses fwnodes then have
> > > > > an API for encoding fwnodes from C data structures at runtime.  The bit
> > > > > where the data gets joined up with the devices isn't the problem, it's
> > > > > the duplication and fragility introduced by encoding everything into
> > > > > an intermediate representation that has no purpose and passing that
> > > > > around which is the problem.
> > >
> > > > The whole exercise with swnode is to minimize the driver intrusion and
> > > > evolving a unified way for (some) of the device properties. V4L2 won't
> > >
> > > The practical implementation for regulators was to duplicate a
> > > substantial amount of code in the core in order to give us a less type
> > > safe and more indirect way of passing data from onen C file in the
> > > kernel to another.  This proposal is a lot better in that it uses the
> > > existing init_data and avoids the huge amounts of duplication, it's just
> > > not clear from the changelog why it's doing this in a regulator specific
> > > manner.
> > >
> > > *Please* stop trying to force swnodes in everywhere, take on board the
> > > feedback about why the swnode implementation is completely inappropriate
> > > for regulators.  I don't understand why you continue to push this so
> > > hard.  swnodes and fwnodes are a solution to a specific problem, they're
> > > not the answer to every problem out there and having to rehash this
> > > continually is getting in the way of actually discussing practical
> > > workarounds for these poorly implemented ACPI platforms.
> > >
> > > > like what you are suggesting exactly because they don't like the idea
> > > > of spreading the board code over the drivers. In some cases it might
> > > > even be not so straightforward and easy.
> > >
> > > > Laurent, do I understand correctly the v4l2 expectations?
> > >
> > > There will be some cases where swnodes make sense, for example where the
> > > data is going to be read through the fwnode API since the binding is
> > > firmware neutral which I think is the v4l case.  On the other hand
> > > having a direct C representation is a very common way of implementing
> > > DMI quirk tables, and we have things like the regulator API where
> > > there's off the shelf platform data support and we actively don't want
> > > to support fwnode.
> >
> > From a camera sensor point of view, we want to avoid code duplication.
> > Having to look for regulators using OF lookups *and* platform data in
> > every single sensor driver is not a good solution. This means that, from
> > a camera sensor driver point of view, we want to call regulator_get()
> > (or the devm_ version) with a name, without caring about who establishes
> > the mapping and how the lookup is performed. I don't care much
> > personally if this would be implemented through swnode or a different
> > mechanism, as long as the implementation can be centralized.

Heikki and I discussed another (possible) approach, He submitted at
some point a series [1] that adds PM domain to software nodes. In such
case the software node callbacks can handle regulator, clocks, etc
which are needed to have it work (of course in case of optional
resources, for mandatory ones we have to provide them anyway).

[1]: https://lore.kernel.org/linux-acpi/20201029105941.63410-1-heikki.krogerus@linux.intel.com/

-- 
With Best Regards,
Andy Shevchenko

Re: [RFC PATCH v2 1/3] regulator: core: Add regulator_lookup_list

[Mark Brown]

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

On Wed, Aug 25, 2021 at 04:59:35PM +0300, Laurent Pinchart wrote:

> From a camera sensor point of view, we want to avoid code duplication.
> Having to look for regulators using OF lookups *and* platform data in
> every single sensor driver is not a good solution. This means that, from
> a camera sensor driver point of view, we want to call regulator_get()
> (or the devm_ version) with a name, without caring about who establishes
> the mapping and how the lookup is performed. I don't care much
> personally if this would be implemented through swnode or a different
> mechanism, as long as the implementation can be centralized.

That's all orthogonal to this discussion, it's about how we configure
the regulators not how clients use the regulators - as you say anything
to do with how the regulator is configured should be totally transparent
there.

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

Re: [RFC PATCH v2 1/3] regulator: core: Add regulator_lookup_list

[Hans de Goede]

Hi all,

On 8/25/21 12:33 PM, Mark Brown wrote:
> On Wed, Aug 25, 2021 at 12:06:18AM +0100, Daniel Scally wrote:
>> In some situations regulator devices can be enumerated via either
>> devicetree or ACPI and bound to regulator drivers but without any
>> init data being provided in firmware. This leaves their consumers
>> unable to acquire them via regulator_get().
>>
>> To fix the issue, add the ability to register a lookup table to a
>> list within regulator core, which will allow board files to provide
>> init data via matching against the regulator name and device name in
>> the same fashion as the gpiod lookup table.
> 
> This is the wrong level to do this I think, this is a generic problem
> that affects all kinds of platform data so if we're not going to scatter
> DMI quirks throughout the drivers like we currently do then we should
> have a way for boards to just store generic platform data for a device
> and then have that platform data joined up with the device later.  This
> could for example also be used by all the laptop audio subsystems which
> need DMI quirk tables in drivers for their components to figure out how
> they're wired up and avoids the need to go through subsystems adding new
> APIs.

Daniel, I believe that what Mark wants here is something similar to what
we already do for the 5v boost converter regulator in the TI bq24190 charger
chip used on some Cherry Trail devices.

There the entire i2c-client is instantiated by platform code here:
drivers/i2c/busses/i2c-cht-wc.c

This attaches a struct bq24190_platform_data as platform data to
the i2c-client, this struct contains a single 

const struct regulator_init_data *regulator_init_data

member which then gets consumed (if there is platform data set) by
the regulator code in:

drivers/power/supply/bq24190_charger.c

For the tps68470 regulator code the platform_data then would need to
have 3 const struct regulator_init_data * members one for each of the
3 regulators.

This platform_data could then be directly set (based on a DMI match table)
from intel_skl_int3472_tps68470.c avoiding probe-ordering issues (1) with
the lookup solution and will allow the code containing the DMI and
regulator_init_data tables to be build as a module.

So all in all I think that this will be a better solution.

Regards,

Hans


1) You are forcing the DMI matching driver adding the lookups to be builtin
but what if the tps68740-MFD + regulatorcode is also builtin, then there
still is no guarantee the lookups will be added before the regulator drivers'
probe function runs


p.s.

I see that you mention something similar later in the thread referring to
the tps65023-regulator driver. I did not check, but assuming that uses what
I describe above; then yes the idea would be to do something similar for
the tps68740-code, setting the platform_data when (just before) the MFD-cells
are instantiated from intel_skl_int3472_tps68470.c

Re: [RFC PATCH v2 1/3] regulator: core: Add regulator_lookup_list

[Mark Brown]

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

On Wed, Aug 25, 2021 at 05:22:49PM +0300, Laurent Pinchart wrote:

> a very large number of regulators, it may not be too bad in practice. If
> I were to maintain the regulator subsystem I'd probably want a
> centralized implementation there, but that's certainly a personal
> preference, at least partly.

We already have some generic platform data for regulators so if it
doesn't want to define anything custom all a driver has to do is forward
that struct on to the core for handling, otherwise it just has to pick
the generic struct out of whatever custom thing it defines.

> On a side note, this RFC looks quite similar to what the GPIO subsystem
> does, which I personally consider nice as differences between regulator
> and GPIO in these areas are confusing for users.

My pushback here is that if all we're doing is providing a mechanism to
match platform data with firmware provided devices we shouldn't be
implementing it per API in the first place, we should have a generic
mechanism for system level quirking to pass platform data to devices
which works with anything - it could also absorb a lot of the DMI based
quirking we have in drivers already which boil down to using a DMI match
to pick some platform data.

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

Re: [RFC PATCH v2 1/3] regulator: core: Add regulator_lookup_list

[Mark Brown]

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

On Wed, Aug 25, 2021 at 04:48:15PM +0200, Hans de Goede wrote:

> Daniel, I believe that what Mark wants here is something similar to what
> we already do for the 5v boost converter regulator in the TI bq24190 charger
> chip used on some Cherry Trail devices.

Yeah, that or something like a generalized version of it which lets a
separate quirk file like they seem to have register the data to insert -
I'd be happy enough with the simple thing too given that it's not
visible to anything, or with DMI quirks in the regulator driver too for
that matter if it's just one or two platforms but there do seem to be
rather a lot of these platforms which need quirks.

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

Re: [RFC PATCH v2 1/3] regulator: core: Add regulator_lookup_list

[Laurent Pinchart]

On Wed, Aug 25, 2021 at 04:27:35PM +0100, Mark Brown wrote:
> On Wed, Aug 25, 2021 at 04:48:15PM +0200, Hans de Goede wrote:
> 
> > Daniel, I believe that what Mark wants here is something similar to what
> > we already do for the 5v boost converter regulator in the TI bq24190 charger
> > chip used on some Cherry Trail devices.
> 
> Yeah, that or something like a generalized version of it which lets a
> separate quirk file like they seem to have register the data to insert -
> I'd be happy enough with the simple thing too given that it's not
> visible to anything, or with DMI quirks in the regulator driver too for
> that matter if it's just one or two platforms but there do seem to be
> rather a lot of these platforms which need quirks.

Let's also remember that we have to handle not just regulators, but also
GPIOs and clocks. And I'm pretty sure there will be more. We could have
a mechanism specific to the tps68470 driver to pass platform data from
the board file to the driver, and replicate that mechanism in different
drivers (for other regulators, clocks and GPIOs), but I really would
like to avoid splitting the DMI-conditioned platform data in those
drivers directly. I'd like to store all the init data for a given
platform in a single "board" file.

-- 
Regards,

Laurent Pinchart

Re: [RFC PATCH v2 1/3] regulator: core: Add regulator_lookup_list

[Mark Brown]

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

On Wed, Aug 25, 2021 at 06:42:30PM +0300, Laurent Pinchart wrote:
> On Wed, Aug 25, 2021 at 04:27:35PM +0100, Mark Brown wrote:

> > Yeah, that or something like a generalized version of it which lets a
> > separate quirk file like they seem to have register the data to insert -

> Let's also remember that we have to handle not just regulators, but also
> GPIOs and clocks. And I'm pretty sure there will be more. We could have
> a mechanism specific to the tps68470 driver to pass platform data from
> the board file to the driver, and replicate that mechanism in different
> drivers (for other regulators, clocks and GPIOs), but I really would
> like to avoid splitting the DMI-conditioned platform data in those
> drivers directly. I'd like to store all the init data for a given
> platform in a single "board" file.

Right, that's the more general bit I'm suggesting.

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

Re: [RFC PATCH v2 1/3] regulator: core: Add regulator_lookup_list

[Hans de Goede]

Hi,

On 8/25/21 5:42 PM, Laurent Pinchart wrote:
> On Wed, Aug 25, 2021 at 04:27:35PM +0100, Mark Brown wrote:
>> On Wed, Aug 25, 2021 at 04:48:15PM +0200, Hans de Goede wrote:
>>
>>> Daniel, I believe that what Mark wants here is something similar to what
>>> we already do for the 5v boost converter regulator in the TI bq24190 charger
>>> chip used on some Cherry Trail devices.
>>
>> Yeah, that or something like a generalized version of it which lets a
>> separate quirk file like they seem to have register the data to insert -
>> I'd be happy enough with the simple thing too given that it's not
>> visible to anything, or with DMI quirks in the regulator driver too for
>> that matter if it's just one or two platforms but there do seem to be
>> rather a lot of these platforms which need quirks.
> 
> Let's also remember that we have to handle not just regulators, but also
> GPIOs and clocks. And I'm pretty sure there will be more. We could have
> a mechanism specific to the tps68470 driver to pass platform data from
> the board file to the driver, and replicate that mechanism in different
> drivers (for other regulators, clocks and GPIOs), but I really would
> like to avoid splitting the DMI-conditioned platform data in those
> drivers directly. I'd like to store all the init data for a given
> platform in a single "board" file.

I agree, but so far all the handling for clks/gpios for IPU3 (+ IPU4 (*))
laptops is done in the drivers/platform/x86/intel/int3472 code and the
passing of platform_data with regulator init-data would also happen in
the mfd-cell instantiation code living there. IOW if we just go with
that then we will already have everything in one place. At least
for the IPU3 case.

Regards,

Hans



*) IPU4 also used the INT3472 ACPI devices and what we have for discrete
IO devices seems to match.

Re: [RFC PATCH v2 1/3] regulator: core: Add regulator_lookup_list

[Laurent Pinchart]

Hi Hans,

On Wed, Aug 25, 2021 at 10:25:23PM +0200, Hans de Goede wrote:
> On 8/25/21 5:42 PM, Laurent Pinchart wrote:
> > On Wed, Aug 25, 2021 at 04:27:35PM +0100, Mark Brown wrote:
> >> On Wed, Aug 25, 2021 at 04:48:15PM +0200, Hans de Goede wrote:
> >>
> >>> Daniel, I believe that what Mark wants here is something similar to what
> >>> we already do for the 5v boost converter regulator in the TI bq24190 charger
> >>> chip used on some Cherry Trail devices.
> >>
> >> Yeah, that or something like a generalized version of it which lets a
> >> separate quirk file like they seem to have register the data to insert -
> >> I'd be happy enough with the simple thing too given that it's not
> >> visible to anything, or with DMI quirks in the regulator driver too for
> >> that matter if it's just one or two platforms but there do seem to be
> >> rather a lot of these platforms which need quirks.
> > 
> > Let's also remember that we have to handle not just regulators, but also
> > GPIOs and clocks. And I'm pretty sure there will be more. We could have
> > a mechanism specific to the tps68470 driver to pass platform data from
> > the board file to the driver, and replicate that mechanism in different
> > drivers (for other regulators, clocks and GPIOs), but I really would
> > like to avoid splitting the DMI-conditioned platform data in those
> > drivers directly. I'd like to store all the init data for a given
> > platform in a single "board" file.
> 
> I agree, but so far all the handling for clks/gpios for IPU3 (+ IPU4 (*))
> laptops is done in the drivers/platform/x86/intel/int3472 code and the
> passing of platform_data with regulator init-data would also happen in
> the mfd-cell instantiation code living there. IOW if we just go with
> that then we will already have everything in one place. At least
> for the IPU3 case.

If the GPIOs are also hooked up to the TPS68470 and not to GPIOs of the
SoC, then I suppose that would be fine in this case.

Do you have any plan to work on IPU4 support ? ;-)

> *) IPU4 also used the INT3472 ACPI devices and what we have for discrete
> IO devices seems to match.

-- 
Regards,

Laurent Pinchart

Re: [RFC PATCH v2 1/3] regulator: core: Add regulator_lookup_list

[Daniel Scally]

Hi Hans

On 25/08/2021 15:48, Hans de Goede wrote:
> Hi all,
> 
> On 8/25/21 12:33 PM, Mark Brown wrote:
>> On Wed, Aug 25, 2021 at 12:06:18AM +0100, Daniel Scally wrote:
>>> In some situations regulator devices can be enumerated via either
>>> devicetree or ACPI and bound to regulator drivers but without any
>>> init data being provided in firmware. This leaves their consumers
>>> unable to acquire them via regulator_get().
>>>
>>> To fix the issue, add the ability to register a lookup table to a
>>> list within regulator core, which will allow board files to provide
>>> init data via matching against the regulator name and device name in
>>> the same fashion as the gpiod lookup table.
>>
>> This is the wrong level to do this I think, this is a generic problem
>> that affects all kinds of platform data so if we're not going to scatter
>> DMI quirks throughout the drivers like we currently do then we should
>> have a way for boards to just store generic platform data for a device
>> and then have that platform data joined up with the device later.  This
>> could for example also be used by all the laptop audio subsystems which
>> need DMI quirk tables in drivers for their components to figure out how
>> they're wired up and avoids the need to go through subsystems adding new
>> APIs.
> 
> Daniel, I believe that what Mark wants here is something similar to what
> we already do for the 5v boost converter regulator in the TI bq24190 charger
> chip used on some Cherry Trail devices.
> 
> There the entire i2c-client is instantiated by platform code here:
> drivers/i2c/busses/i2c-cht-wc.c
> 
> This attaches a struct bq24190_platform_data as platform data to
> the i2c-client, this struct contains a single 
> 
> const struct regulator_init_data *regulator_init_data
> 
> member which then gets consumed (if there is platform data set) by
> the regulator code in:
> 
> drivers/power/supply/bq24190_charger.c
> 
> For the tps68470 regulator code the platform_data then would need to
> have 3 const struct regulator_init_data * members one for each of the
> 3 regulators.
> 
> This platform_data could then be directly set (based on a DMI match table)
> from intel_skl_int3472_tps68470.c avoiding probe-ordering issues (1) with
> the lookup solution and will allow the code containing the DMI and
> regulator_init_data tables to be build as a module.
> 
> So all in all I think that this will be a better solution.

So assign an array of the init_data to the i2c-INT3472:nn device as
platform data before registering the MFD cells in
intel_skl_int3472_tps68470.c and parse that out when the regulators
register. OK - that's fine by me.

> 
> Regards,
> 
> Hans
> 
> 
> 1) You are forcing the DMI matching driver adding the lookups to be builtin
> but what if the tps68740-MFD + regulatorcode is also builtin, then there
> still is no guarantee the lookups will be added before the regulator drivers'
> probe function runs

Err, excellent point - I hadn't thought of that if I'm honest.

> 
> p.s.
> 
> I see that you mention something similar later in the thread referring to
> the tps65023-regulator driver. I did not check, but assuming that uses what
> I describe above; then yes the idea would be to do something similar for
> the tps68740-code, setting the platform_data when (just before) the MFD-cells
> are instantiated from intel_skl_int3472_tps68470.c

The tps65023-regulator driver parses init data out of platform data yes.
I think that that's fine, but personally I'd prefer that done in core.c
rather than in the regulator driver itself if possible.

Re: [RFC PATCH v2 1/3] regulator: core: Add regulator_lookup_list

[Daniel Scally]

On 25/08/2021 21:40, Laurent Pinchart wrote:
> Hi Hans,
> 
> On Wed, Aug 25, 2021 at 10:25:23PM +0200, Hans de Goede wrote:
>> On 8/25/21 5:42 PM, Laurent Pinchart wrote:
>>> On Wed, Aug 25, 2021 at 04:27:35PM +0100, Mark Brown wrote:
>>>> On Wed, Aug 25, 2021 at 04:48:15PM +0200, Hans de Goede wrote:
>>>>
>>>>> Daniel, I believe that what Mark wants here is something similar to what
>>>>> we already do for the 5v boost converter regulator in the TI bq24190 charger
>>>>> chip used on some Cherry Trail devices.
>>>>
>>>> Yeah, that or something like a generalized version of it which lets a
>>>> separate quirk file like they seem to have register the data to insert -
>>>> I'd be happy enough with the simple thing too given that it's not
>>>> visible to anything, or with DMI quirks in the regulator driver too for
>>>> that matter if it's just one or two platforms but there do seem to be
>>>> rather a lot of these platforms which need quirks.
>>>
>>> Let's also remember that we have to handle not just regulators, but also
>>> GPIOs and clocks. And I'm pretty sure there will be more. We could have
>>> a mechanism specific to the tps68470 driver to pass platform data from
>>> the board file to the driver, and replicate that mechanism in different
>>> drivers (for other regulators, clocks and GPIOs), but I really would
>>> like to avoid splitting the DMI-conditioned platform data in those
>>> drivers directly. I'd like to store all the init data for a given
>>> platform in a single "board" file.
>>
>> I agree, but so far all the handling for clks/gpios for IPU3 (+ IPU4 (*))
>> laptops is done in the drivers/platform/x86/intel/int3472 code and the
>> passing of platform_data with regulator init-data would also happen in
>> the mfd-cell instantiation code living there. IOW if we just go with
>> that then we will already have everything in one place. At least
>> for the IPU3 case.
> 
> If the GPIOs are also hooked up to the TPS68470 and not to GPIOs of the
> SoC, then I suppose that would be fine in this case.

The GPIOs that we're translating into clks / mapping to the sensors in
the INT3472 code are SOC GPIOs actually...this is the first bit of code
that relates to a physical TPS68470.
> 
> Do you have any plan to work on IPU4 support ? ;-)
> 
>> *) IPU4 also used the INT3472 ACPI devices and what we have for discrete
>> IO devices seems to match.
> 

Re: [RFC PATCH v2 1/3] regulator: core: Add regulator_lookup_list

[Daniel Scally]

Hi Mark

On 25/08/2021 15:52, Mark Brown wrote:
> On Wed, Aug 25, 2021 at 05:22:49PM +0300, Laurent Pinchart wrote:
>
>> a very large number of regulators, it may not be too bad in practice. If
>> I were to maintain the regulator subsystem I'd probably want a
>> centralized implementation there, but that's certainly a personal
>> preference, at least partly.
> We already have some generic platform data for regulators so if it
> doesn't want to define anything custom all a driver has to do is forward
> that struct on to the core for handling, otherwise it just has to pick
> the generic struct out of whatever custom thing it defines.
>
>> On a side note, this RFC looks quite similar to what the GPIO subsystem
>> does, which I personally consider nice as differences between regulator
>> and GPIO in these areas are confusing for users.
> My pushback here is that if all we're doing is providing a mechanism to
> match platform data with firmware provided devices we shouldn't be
> implementing it per API in the first place, we should have a generic
> mechanism for system level quirking to pass platform data to devices
> which works with anything - it could also absorb a lot of the DMI based
> quirking we have in drivers already which boil down to using a DMI match
> to pick some platform data.


Alright; and what about parsing the platform data into a struct
regulator_init_data then...at the moment that's left up to the
individual regulator drivers. In my mind that's something better left to
core, so rather than finding the right instance of struct
regulator_init_data from the regulator_lookup_list the
regulator_lookup_init_data() function would be finding the right
instance of the struct from cfg->dev->platform_data instead...what are
your thoughts on that?

Re: [RFC PATCH v2 1/3] regulator: core: Add regulator_lookup_list

[Mark Brown]

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

On Wed, Aug 25, 2021 at 11:09:09PM +0100, Daniel Scally wrote:

> Alright; and what about parsing the platform data into a struct
> regulator_init_data then...at the moment that's left up to the
> individual regulator drivers. In my mind that's something better left to
> core, so rather than finding the right instance of struct
> regulator_init_data from the regulator_lookup_list the
> regulator_lookup_init_data() function would be finding the right
> instance of the struct from cfg->dev->platform_data instead...what are
> your thoughts on that?

That sounds like a useful helper to have - we need to let drivers have
custom platform data (especially for MFDs) but even there they could
use such a helper if they can point it at an array stored elsewhere.

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