[RFC PATCH 0/2] Add software node support to regulator framework

[RFC PATCH 0/2] Add software node support to regulator framework

[Daniel Scally]

Hello all

See previous series for some background context [1]

Some x86 laptops with ACPI tables designed for Windows have a TPS68470
PMIC providing regulators and clocks to camera modules. The DSDT tables for
those cameras lack any power control methods, declaring only a
dependency on the ACPI device representing the TPS68470. This leaves the
regulator framework with no means of determining appropriate voltages for the
regulators provided by the PMIC, or of determining which regulators relate to
which of the sensor's requested supplies. 

This series is a prototype of an emulation of the device tree regulator
initialisation and lookup functions, using software nodes. Software nodes
relating to each regulator are registered as children of the TPS68470's ACPI
firmware node. Those regulators have properties describing their constraints
(for example "regulator-min-microvolt"). Similarly, software nodes are
registered and assigned as secondary to the Camera's firmware node - these
software nodes have reference properties named after the supply in the same
way as device tree's phandles, for example "avdd-supply", and linking to the
software node assigned to the appropriate regulator. We can then use those
constraints to specify the appropriate voltages and the references to allow the
camera drivers to look up the correct regulator device. 

Although not included in this series, I would plan to use a similar method for
linking the clocks provided by the TPS68470 to the sensor so that it can be
discovered too.

I'm posting this to see if people agree it's a good approach for tackling the 
problem; I may be overthinking this and there's a much easier way that I should
be looking at instead. It will have knock-ons in the cio2-bridge code [2], as
that is adding software nodes to the same sensors to connect them to the media
graph. Similarly, is the board file an acceptable solution, or should we just
define the configuration for these devices (there's three orf our laptop models
in scope) in int3472-tps68470 instead?

Thanks
Dan

[1] https://lore.kernel.org/lkml/20210603224007.120560-1-djrscally@gmail.com/
[2] https://elixir.bootlin.com/linux/latest/source/drivers/media/pci/intel/ipu3/cio2-bridge.c#L166


Daniel Scally (2):
  regulator: Add support for software node connections
  platform/surface: Add Surface Go 2 board file

 MAINTAINERS                                |   6 +
 drivers/platform/surface/Kconfig           |  10 ++
 drivers/platform/surface/Makefile          |   1 +
 drivers/platform/surface/surface_go_2.c    | 135 +++++++++++++++++++++
 drivers/regulator/Kconfig                  |   6 +
 drivers/regulator/Makefile                 |   1 +
 drivers/regulator/core.c                   |  23 ++++
 drivers/regulator/swnode_regulator.c       | 111 +++++++++++++++++
 include/linux/regulator/swnode_regulator.h |  33 +++++
 9 files changed, 326 insertions(+)
 create mode 100644 drivers/platform/surface/surface_go_2.c
 create mode 100644 drivers/regulator/swnode_regulator.c
 create mode 100644 include/linux/regulator/swnode_regulator.h

-- 
2.25.1

[RFC PATCH 1/2] regulator: Add support for software node connections

[Daniel Scally]

On some x86 platforms, firmware provided by the OEM does not adequately
describe the constraints of regulator resources nor their connections to
consuming devices via ACPI. To compensate, extend the regulator framework
to allow discovery of constraints and supply-consumer relationships via
software node references and properties. This will allow us to define
those platform specific connections so that the resources can be used in
the usual way.

Signed-off-by: Daniel Scally <djrscally@gmail.com>
---

swnode_get_regulator_constraints() here is not as extensive as the equivalent
function in of_regulator...I could include all that now, but it didn't seem
worth it so we don't know what any of those values should be.

 drivers/regulator/Kconfig                  |   6 ++
 drivers/regulator/Makefile                 |   1 +
 drivers/regulator/core.c                   |  23 +++++
 drivers/regulator/swnode_regulator.c       | 111 +++++++++++++++++++++
 include/linux/regulator/swnode_regulator.h |  33 ++++++
 5 files changed, 174 insertions(+)
 create mode 100644 drivers/regulator/swnode_regulator.c
 create mode 100644 include/linux/regulator/swnode_regulator.h

diff --git a/drivers/regulator/Kconfig b/drivers/regulator/Kconfig
index 9d84d9245490..7e0351f8fe38 100644
--- a/drivers/regulator/Kconfig
+++ b/drivers/regulator/Kconfig
@@ -1171,6 +1171,12 @@ config REGULATOR_SY8827N
 	help
 	  This driver supports SY8827N single output regulator.
 
+config REGULATOR_SWNODE
+	bool "Software Node Regulator Support"
+	help
+	  This option configures support for providing regulator constraints and
+	  mappings to consumers through the use of software nodes.
+
 config REGULATOR_TPS51632
 	tristate "TI TPS51632 Power Regulator"
 	depends on I2C
diff --git a/drivers/regulator/Makefile b/drivers/regulator/Makefile
index 580b015296ea..25cb7bbad652 100644
--- a/drivers/regulator/Makefile
+++ b/drivers/regulator/Makefile
@@ -9,6 +9,7 @@ obj-$(CONFIG_OF) += of_regulator.o
 obj-$(CONFIG_REGULATOR_FIXED_VOLTAGE) += fixed.o
 obj-$(CONFIG_REGULATOR_VIRTUAL_CONSUMER) += virtual.o
 obj-$(CONFIG_REGULATOR_USERSPACE_CONSUMER) += userspace-consumer.o
+obj-$(CONFIG_REGULATOR_SWNODE) += swnode_regulator.o
 
 obj-$(CONFIG_REGULATOR_88PG86X) += 88pg86x.o
 obj-$(CONFIG_REGULATOR_88PM800) += 88pm800-regulator.o
diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index f192bf19492e..6e10f511db80 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -20,11 +20,13 @@
 #include <linux/gpio/consumer.h>
 #include <linux/of.h>
 #include <linux/regmap.h>
+#include <linux/property.h>
 #include <linux/regulator/of_regulator.h>
 #include <linux/regulator/consumer.h>
 #include <linux/regulator/coupler.h>
 #include <linux/regulator/driver.h>
 #include <linux/regulator/machine.h>
+#include <linux/regulator/swnode_regulator.h>
 #include <linux/module.h>
 
 #define CREATE_TRACE_POINTS
@@ -1763,6 +1765,7 @@ static struct regulator_dev *regulator_dev_lookup(struct device *dev,
 						  const char *supply)
 {
 	struct regulator_dev *r = NULL;
+	struct fwnode_handle *swnode;
 	struct device_node *node;
 	struct regulator_map *map;
 	const char *devname = NULL;
@@ -1785,6 +1788,22 @@ static struct regulator_dev *regulator_dev_lookup(struct device *dev,
 		}
 	}
 
+	/* next, try software nodes */
+	if (dev && dev->fwnode && is_software_node(dev->fwnode->secondary)) {
+		swnode = swnode_get_regulator_node(dev, supply);
+		if (!IS_ERR(swnode)) {
+			r = swnode_find_regulator_by_node(swnode);
+			if (r)
+				return r;
+
+			/*
+			 * We have a node, but there is no device.
+			 * assume it has not registered yet.
+			 */
+			return ERR_PTR(-EPROBE_DEFER);
+		}
+	}
+
 	/* if not found, try doing it non-dt way */
 	if (dev)
 		devname = dev_name(dev);
@@ -5242,6 +5261,10 @@ 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_swnode_get_init_data(dev, regulator_desc,
+							   config, &rdev->dev.fwnode);
+
 	/*
 	 * 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
diff --git a/drivers/regulator/swnode_regulator.c b/drivers/regulator/swnode_regulator.c
new file mode 100644
index 000000000000..75718de74366
--- /dev/null
+++ b/drivers/regulator/swnode_regulator.c
@@ -0,0 +1,111 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Author: Dan Scally <djrscally@gmail.com> */
+
+#include <linux/property.h>
+#include <linux/regulator/driver.h>
+#include <linux/regulator/machine.h>
+
+#include "internal.h"
+
+static struct fwnode_handle *
+regulator_swnode_get_init_node(struct fwnode_handle *fwnode,
+			       const struct regulator_desc *desc)
+{
+	const struct software_node *parent, *child;
+
+	parent = to_software_node(fwnode->secondary);
+
+	if (desc->regulators_node)
+		child = software_node_find_by_name(parent,
+						   desc->regulators_node);
+	else
+		child = software_node_find_by_name(parent, desc->name);
+
+	return software_node_fwnode(child);
+}
+
+static int swnode_get_regulator_constraints(struct fwnode_handle *swnode,
+					    struct regulator_init_data *init_data)
+{
+	struct regulation_constraints *constraints = &init_data->constraints;
+	u32 pval;
+	int ret;
+
+	ret = fwnode_property_read_string(swnode, "regulator-name", &constraints->name);
+	if (ret)
+		return ret;
+
+	if (!fwnode_property_read_u32(swnode, "regulator-min-microvolt", &pval))
+		constraints->min_uV = pval;
+
+	if (!fwnode_property_read_u32(swnode, "regulator-max-microvolt", &pval))
+		constraints->max_uV = pval;
+
+	/* Voltage change possible? */
+	if (constraints->min_uV != constraints->max_uV)
+		constraints->valid_ops_mask |= REGULATOR_CHANGE_VOLTAGE;
+
+	/* Do we have a voltage range, if so try to apply it? */
+	if (constraints->min_uV && constraints->max_uV)
+		constraints->apply_uV = true;
+
+	constraints->boot_on = fwnode_property_read_bool(swnode, "regulator-boot-on");
+	constraints->always_on = fwnode_property_read_bool(swnode, "regulator-always-on");
+	if (!constraints->always_on) /* status change should be possible. */
+		constraints->valid_ops_mask |= REGULATOR_CHANGE_STATUS;
+
+	return 0;
+}
+
+struct regulator_init_data *
+regulator_swnode_get_init_data(struct device *dev,
+			       const struct regulator_desc *desc,
+			       struct regulator_config *config,
+			       struct fwnode_handle **regnode)
+{
+	struct fwnode_handle *fwnode = dev_fwnode(dev);
+	struct regulator_init_data *init_data;
+	struct fwnode_handle *regulator;
+	int ret;
+
+	if (!fwnode || !is_software_node(fwnode->secondary))
+		return NULL;
+
+	regulator = regulator_swnode_get_init_node(fwnode, desc);
+	if (!regulator)
+		return NULL;
+
+	init_data = devm_kzalloc(dev, sizeof(*init_data), GFP_KERNEL);
+	if (!init_data)
+		return ERR_PTR(-ENOMEM);
+
+	ret = swnode_get_regulator_constraints(regulator, init_data);
+	if (ret)
+		return ERR_PTR(ret);
+
+	*regnode = regulator;
+
+	return init_data;
+}
+
+struct regulator_dev *
+swnode_find_regulator_by_node(struct fwnode_handle *swnode)
+{
+	struct device *dev;
+
+	dev = class_find_device_by_fwnode(&regulator_class, swnode);
+
+	return dev ? dev_to_rdev(dev) : NULL;
+}
+
+struct fwnode_handle *swnode_get_regulator_node(struct device *dev, const char *supply)
+{
+	struct fwnode_handle *fwnode = dev_fwnode(dev);
+	char *prop_name;
+
+	prop_name = devm_kasprintf(dev, GFP_KERNEL, "%s-supply", supply);
+	if (!prop_name)
+		return ERR_PTR(-ENOMEM);
+
+	return fwnode_find_reference(fwnode->secondary, prop_name, 0);
+}
diff --git a/include/linux/regulator/swnode_regulator.h b/include/linux/regulator/swnode_regulator.h
new file mode 100644
index 000000000000..97527f7dbe80
--- /dev/null
+++ b/include/linux/regulator/swnode_regulator.h
@@ -0,0 +1,33 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/* Author: Dan Scally <djrscally@gmail.com> */
+#ifndef __LINUX_SWNODE_REG_H
+#define __LINUX_SWNODE_REG_H
+
+#if defined(CONFIG_REGULATOR_SWNODE)
+struct regulator_init_data *regulator_swnode_get_init_data(struct device *dev,
+							   const struct regulator_desc *desc,
+							   struct regulator_config *config,
+							   struct fwnode_handle **regnode);
+struct regulator_dev *swnode_find_regulator_by_node(struct fwnode_handle *swnode);
+struct fwnode_handle *swnode_get_regulator_node(struct device *dev, const char *supply);
+#else
+struct regulator_init_data *regulator_swnode_get_init_data(struct device *dev,
+							   const struct regulator_desc *desc,
+							   struct regulator_config *config)
+{
+	return NULL;
+}
+
+struct regulator_dev *swnode_find_regulator_by_node(struct fwnode_handle *swnode)
+{
+	return NULL;
+}
+
+struct fwnode_handle *swnode_get_regulator_node(struct device *dev, const char *supply)
+{
+	return NULL;
+}
+
+#endif
+
+#endif
-- 
2.25.1

Re: [RFC PATCH 1/2] regulator: Add support for software node connections

[Mark Brown]

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

On Thu, Jul 08, 2021 at 11:42:25PM +0100, Daniel Scally wrote:

> --- a/drivers/regulator/Makefile
> +++ b/drivers/regulator/Makefile
> @@ -9,6 +9,7 @@ obj-$(CONFIG_OF) += of_regulator.o
>  obj-$(CONFIG_REGULATOR_FIXED_VOLTAGE) += fixed.o
>  obj-$(CONFIG_REGULATOR_VIRTUAL_CONSUMER) += virtual.o
>  obj-$(CONFIG_REGULATOR_USERSPACE_CONSUMER) += userspace-consumer.o
> +obj-$(CONFIG_REGULATOR_SWNODE) += swnode_regulator.o

This appears to be sorted with regulator drivers but as far as I
understand it this is not a driver?  I'd put it next to the OF file.

> @@ -1785,6 +1788,22 @@ static struct regulator_dev *regulator_dev_lookup(struct device *dev,

The line numbers here look off...

> +++ b/drivers/regulator/swnode_regulator.c
> @@ -0,0 +1,111 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/* Author: Dan Scally <djrscally@gmail.com> */
> +

Please make the entire comment a C++ one so things look more
intentional.

> +static struct fwnode_handle *
> +regulator_swnode_get_init_node(struct fwnode_handle *fwnode,
> +			       const struct regulator_desc *desc)
> +{
> +	const struct software_node *parent, *child;
> +
> +	parent = to_software_node(fwnode->secondary);
> +
> +	if (desc->regulators_node)
> +		child = software_node_find_by_name(parent,
> +						   desc->regulators_node);
> +	else
> +		child = software_node_find_by_name(parent, desc->name);
> +
> +	return software_node_fwnode(child);
> +}

Nothing is documenting what the binding for these swnodes is supposed to
be so it's hard to tell if any of this is correct and makes sense, nor
how someone is supposed to take this stuff and integrate it into a
system.  I think this needs some more explicit documentation of what's
going on adding to the tree, this will help with review and help anyone
who needs to use this stuff figure out how to do so.

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

[RFC PATCH 2/2] 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>
---
 MAINTAINERS                             |   6 ++
 drivers/platform/surface/Kconfig        |  10 ++
 drivers/platform/surface/Makefile       |   1 +
 drivers/platform/surface/surface_go_2.c | 135 ++++++++++++++++++++++++
 4 files changed, 152 insertions(+)
 create mode 100644 drivers/platform/surface/surface_go_2.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 52e61092c63b..f11e68daf939 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -12143,6 +12143,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..4a335f0d5fd5
--- /dev/null
+++ b/drivers/platform/surface/surface_go_2.c
@@ -0,0 +1,135 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Author: Dan Scally <djrscally@gmail.com> */
+
+#include <linux/acpi.h>
+#include <linux/gpio/machine.h>
+#include <linux/init.h>
+#include <linux/kernel.h>
+#include <linux/property.h>
+
+static const struct software_node tps68470_node = {
+	"INT3472",
+};
+
+static const struct property_entry ana_properties[] = {
+	PROPERTY_ENTRY_STRING("regulator-name", "ANA"),
+	PROPERTY_ENTRY_U32("regulator-min-microvolt", 2815200),
+	PROPERTY_ENTRY_U32("regulator-max-microvolt", 2815200),
+	{ }
+};
+
+static const struct property_entry vsio_properties[] = {
+	PROPERTY_ENTRY_STRING("regulator-name", "VSIO"),
+	PROPERTY_ENTRY_U32("regulator-min-microvolt", 1800600),
+	PROPERTY_ENTRY_U32("regulator-max-microvolt", 1800600),
+	{ }
+};
+
+static const struct property_entry core_properties[] = {
+	PROPERTY_ENTRY_STRING("regulator-name", "CORE"),
+	PROPERTY_ENTRY_U32("regulator-min-microvolt", 1200000),
+	PROPERTY_ENTRY_U32("regulator-max-microvolt", 1200000),
+	{ }
+};
+
+static const struct software_node regulator_nodes[] = {
+	{"ANA", &tps68470_node, ana_properties},
+	{"VSIO", &tps68470_node, vsio_properties},
+	{"CORE", &tps68470_node, core_properties},
+};
+
+static const struct property_entry sensor_properties[] = {
+	PROPERTY_ENTRY_REF("avdd-supply", &regulator_nodes[0]),
+	PROPERTY_ENTRY_REF("dovdd-supply", &regulator_nodes[1]),
+	PROPERTY_ENTRY_REF("dvdd-supply", &regulator_nodes[2]),
+	{ }
+};
+
+static struct software_node sensor_node = {
+	.name		= "INT347A",
+	.properties	= sensor_properties,
+};
+
+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)
+{
+	struct fwnode_handle *fwnode, *sensor_fwnode;
+	struct acpi_device *adev, *sensor;
+	int ret;
+
+	adev = acpi_dev_get_first_match_dev("INT3472", "0", -1);
+	if (!adev) {
+		pr_err("%s(): Failed to find INT3472 ACPI device\n", __func__);
+		return -EINVAL;
+	}
+
+	ret = software_node_register(&tps68470_node);
+	if (ret) {
+		dev_err(&adev->dev, "Failed to add tps68470 software node\n");
+		goto err_put_adev;
+	}
+
+	fwnode = software_node_fwnode(&tps68470_node);
+	if (!fwnode) {
+		dev_err(&adev->dev, "Failed to find tps68470 fwnode\n");
+		ret = -ENODEV;
+		goto err_put_adev;
+	}
+
+	adev->fwnode.secondary = fwnode;
+
+	ret = software_node_register_nodes(regulator_nodes);
+	if (ret) {
+		dev_err(&adev->dev,
+			"failed to register software nodes for regulator\n");
+		goto err_unregister_node;
+	}
+
+	gpiod_add_lookup_table(&surface_go_2_gpios);
+
+	sensor = acpi_dev_get_first_match_dev("INT347A", "0", -1);
+	if (!sensor) {
+		pr_err("%s(): Failed to find sensor\n", __func__);
+		ret = -ENODEV;
+		goto err_remove_gpio_lookup;
+	}
+
+	ret = software_node_register(&sensor_node);
+	if (ret) {
+		dev_err(&sensor->dev, "Failed to add sensor node\n");
+		goto err_put_sensor;
+	}
+
+	sensor_fwnode = software_node_fwnode(&sensor_node);
+	if (!sensor_fwnode) {
+		dev_err(&sensor->dev, "Failed to find sensor fwnode\n");
+		ret = -ENODEV;
+		goto err_unregister_sensor_node;
+	}
+
+	sensor->fwnode.secondary = sensor_fwnode;
+
+	return ret;
+
+err_unregister_sensor_node:
+	software_node_unregister(&sensor_node);
+err_put_sensor:
+	acpi_dev_put(sensor);
+err_remove_gpio_lookup:
+	gpiod_remove_lookup_table(&surface_go_2_gpios);
+err_unregister_node:
+	adev->fwnode.secondary = -ENODEV;
+	software_node_unregister(&tps68470_node);
+err_put_adev:
+	acpi_dev_put(adev);
+
+	return ret;
+}
+device_initcall(surface_go_2_init);
-- 
2.25.1

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

[Mark Brown]

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

On Thu, Jul 08, 2021 at 11:42:26PM +0100, Daniel Scally wrote:

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

Does anything actually need these connections or settings?

> +static const struct property_entry core_properties[] = {
> +	PROPERTY_ENTRY_STRING("regulator-name", "CORE"),
> +	PROPERTY_ENTRY_U32("regulator-min-microvolt", 1200000),
> +	PROPERTY_ENTRY_U32("regulator-max-microvolt", 1200000),
> +	{ }
> +};

Does anything actually care about the voltages here - why specify them?
As far as I can tell from grepping all the supplies you're adding are
digital.

> +static const struct software_node regulator_nodes[] = {
> +	{"ANA", &tps68470_node, ana_properties},
> +	{"VSIO", &tps68470_node, vsio_properties},
> +	{"CORE", &tps68470_node, core_properties},
> +};

> +static const struct property_entry sensor_properties[] = {
> +	PROPERTY_ENTRY_REF("avdd-supply", &regulator_nodes[0]),
> +	PROPERTY_ENTRY_REF("dovdd-supply", &regulator_nodes[1]),
> +	PROPERTY_ENTRY_REF("dvdd-supply", &regulator_nodes[2]),
> +	{ }

I would strongly caution against this idiom of referencing elements in
other arrays using magic numbers, it is both hard to read and error
prone when someone goes in making changes.  If they need to be in arrays
then constants for the array indexes would help a lot.

> +static struct software_node sensor_node = {
> +	.name		= "INT347A",
> +	.properties	= sensor_properties,
> +};
> +
> +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)
> +	}
> +};

This appears to be using regular board file stuff for GPIOs, if this
swnode stuff is somehow needed for regulators why is it not also needed
for GPIOs?  I think this is going back to the thing I was saying earlier
about not really understanding the problem being solved here.

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

Re: [RFC PATCH 0/2] Add software node support to regulator framework

[Mark Brown]

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

On Thu, Jul 08, 2021 at 11:42:24PM +0100, Daniel Scally wrote:

> See previous series for some background context [1]

That's a link to a series "[PATCH v5 0/6] Introduce intel_skl_int3472
module" which doesn't have any explanatory text as to what it's doing in
the cover letter (just an inter version changelog) nor any obvious
relevance to this series, are you sure that's the right link?  In
general it's best if your patch series contains enough explanatory
information to allow someone to have a reasonable idea what the series
does without having to follow links like this.

> This series is a prototype of an emulation of the device tree regulator
> initialisation and lookup functions, using software nodes. Software nodes

What is a software node and why would we want to use one here?

> relating to each regulator are registered as children of the TPS68470's ACPI
> firmware node. Those regulators have properties describing their constraints
> (for example "regulator-min-microvolt"). Similarly, software nodes are
> registered and assigned as secondary to the Camera's firmware node - these
> software nodes have reference properties named after the supply in the same
> way as device tree's phandles, for example "avdd-supply", and linking to the
> software node assigned to the appropriate regulator. We can then use those
> constraints to specify the appropriate voltages and the references to allow the
> camera drivers to look up the correct regulator device. 

So these systems lack an enumerable description of the system provided
by hardware or firmware (or given that these are ACPI systems I guess
the firmware description is just broken) so we need to use board files.
Why are we not just using board files, what does this new abstraction
solve?

> I'm posting this to see if people agree it's a good approach for tackling the 
> problem; I may be overthinking this and there's a much easier way that I should

I don't think I understand what the problem you are trying to solve is
so it's hard to say if this is a good approach to solving it.

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

Re: [RFC PATCH 0/2] Add software node support to regulator framework

[Daniel Scally]

Hi Mark, thanks for the feedback - much appreciated.

On 09/07/2021 18:04, Mark Brown wrote:
> On Thu, Jul 08, 2021 at 11:42:24PM +0100, Daniel Scally wrote:
>
>> See previous series for some background context [1]
> That's a link to a series "[PATCH v5 0/6] Introduce intel_skl_int3472
> module" which doesn't have any explanatory text as to what it's doing in
> the cover letter (just an inter version changelog) nor any obvious
> relevance to this series, are you sure that's the right link?  In
> general it's best if your patch series contains enough explanatory
> information to allow someone to have a reasonable idea what the series
> does without having to follow links like this.


It is, but I certainly could have done a better job of explaining
context there rather than throwing it in (particularly given the
uselessness of the cover letter there, I forgot about that - sorry).
That series contains an i2c driver that binds to the TPS68470 when
enumerated via ACPI. That i2c driver is effectively an MFD driver, it
registers a regmap plus platform devices for the GPIO, clk, regulator
and OpRegion parts of the TPS68470, each of which has their own platform
driver to control them (though only the GPIO and OpRegion ones are
currently upstream - I need to post the clk and regulator ones), all
sharing the same regmap. This series is a follow on from that,
attempting to remedy problems with the ACPI that prevent those resources
from being discoverable by consumers on some devices.

>> This series is a prototype of an emulation of the device tree regulator
>> initialisation and lookup functions, using software nodes. Software nodes
> What is a software node and why would we want to use one here?


Like of_nodes, but defined entirely within kernel code rather than
parsed out of devicetree or from ACPI. I think we need to use them to
make up for the deficiencies in ACPI on these platforms.

>> relating to each regulator are registered as children of the TPS68470's ACPI
>> firmware node. Those regulators have properties describing their constraints
>> (for example "regulator-min-microvolt"). Similarly, software nodes are
>> registered and assigned as secondary to the Camera's firmware node - these
>> software nodes have reference properties named after the supply in the same
>> way as device tree's phandles, for example "avdd-supply", and linking to the
>> software node assigned to the appropriate regulator. We can then use those
>> constraints to specify the appropriate voltages and the references to allow the
>> camera drivers to look up the correct regulator device. 
> So these systems lack an enumerable description of the system provided
> by hardware or firmware (or given that these are ACPI systems I guess
> the firmware description is just broken) so we need to use board files.
> Why are we not just using board files, what does this new abstraction
> solve?


I went with this approach because the ACPI isn't entirely lacking, it
enumerates the TPS68470 as an i2c device for its driver to bind to
without a problem which results in the regulator driver registering the
regulator devices (7 of them for this chip), so I was thinking along the
lines of repairing the problems with ACPI to give those rdevs the right
init_data rather than sidestepping it altogether. I could register the
platform devices for the regulator driver to bind to in a board file
instead if that's the preferred option...usually this would involve
using i2c_board_info I think but as ACPI will enumerate the i2c device
for the chip independently we'd need to handle that somehow to stop them
racing each other I guess.


I'll take a look and see if I can make it work that way.


>> I'm posting this to see if people agree it's a good approach for tackling the 
>> problem; I may be overthinking this and there's a much easier way that I should
> I don't think I understand what the problem you are trying to solve is
> so it's hard to say if this is a good approach to solving it.


Hope this is a bit clearer now?

Re: [RFC PATCH 0/2] Add software node support to regulator framework

[Mark Brown]

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

On Sat, Jul 10, 2021 at 11:48:33PM +0100, Daniel Scally wrote:

> I went with this approach because the ACPI isn't entirely lacking, it
> enumerates the TPS68470 as an i2c device for its driver to bind to
> without a problem which results in the regulator driver registering the
> regulator devices (7 of them for this chip), so I was thinking along the
> lines of repairing the problems with ACPI to give those rdevs the right
> init_data rather than sidestepping it altogether. I could register the
> platform devices for the regulator driver to bind to in a board file
> instead if that's the preferred option...usually this would involve
> using i2c_board_info I think but as ACPI will enumerate the i2c device
> for the chip independently we'd need to handle that somehow to stop them
> racing each other I guess.

Like I said elsewhere it seems a lot easier to just have the I2C driver
set platform data based on parsing DMI information like we do elsewhere.
I really don't see any benefit to introducing an additional layer of
abstraction and binding here, it just seems to be making things more
fragile.

I'm not sure what you mean by "register the platform devices for the
regualtor to bind to" - if the PMIC is an I2C device it's going to need
to be an I2C device, and if the device is enumerated by firmware we'd
need to suppress that firmware enumeration to replace it.

> 
> 
> I'll take a look and see if I can make it work that way.
> 
> 
> >> I'm posting this to see if people agree it's a good approach for tackling the 
> >> problem; I may be overthinking this and there's a much easier way that I should
> > I don't think I understand what the problem you are trying to solve is
> > so it's hard to say if this is a good approach to solving it.
> 
> 
> Hope this is a bit clearer now?
> 

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

Re: [RFC PATCH 0/2] Add software node support to regulator framework

[Laurent Pinchart]

Hi Mark,

On Mon, Jul 12, 2021 at 03:15:28PM +0100, Mark Brown wrote:
> On Sat, Jul 10, 2021 at 11:48:33PM +0100, Daniel Scally wrote:
> 
> > I went with this approach because the ACPI isn't entirely lacking, it
> > enumerates the TPS68470 as an i2c device for its driver to bind to
> > without a problem which results in the regulator driver registering the
> > regulator devices (7 of them for this chip), so I was thinking along the
> > lines of repairing the problems with ACPI to give those rdevs the right
> > init_data rather than sidestepping it altogether. I could register the
> > platform devices for the regulator driver to bind to in a board file
> > instead if that's the preferred option...usually this would involve
> > using i2c_board_info I think but as ACPI will enumerate the i2c device
> > for the chip independently we'd need to handle that somehow to stop them
> > racing each other I guess.
> 
> Like I said elsewhere it seems a lot easier to just have the I2C driver
> set platform data based on parsing DMI information like we do elsewhere.
> I really don't see any benefit to introducing an additional layer of
> abstraction and binding here, it just seems to be making things more
> fragile.

The idea behind software nodes is that individual device drivers
shouldn't care about where platform-specific data come from. They can
use the same fwnode API, regardless of whether the device is described
through OF, ACPI, or software nodes created by a board file in the
kernel. It allows grouping all platform data that should be provided by
firmware in a single place, conditioned by a DMI match, instead of
distributing DMI matches to lots of drivers.

> I'm not sure what you mean by "register the platform devices for the
> regualtor to bind to" - if the PMIC is an I2C device it's going to need
> to be an I2C device, and if the device is enumerated by firmware we'd
> need to suppress that firmware enumeration to replace it.

We can't. ACPI describes the device, and that's how it's enumerated.
ACPI does provide information (as well as ACPI methods) needed by the
driver, but fails to provide all the required information. That's why a
mechanism to supplement the information provided by ACPI is needed.

> > I'll take a look and see if I can make it work that way.
> > 
> > >> I'm posting this to see if people agree it's a good approach for tackling the 
> > >> problem; I may be overthinking this and there's a much easier way that I should
> > > I don't think I understand what the problem you are trying to solve is
> > > so it's hard to say if this is a good approach to solving it.
> > 
> > Hope this is a bit clearer now?

-- 
Regards,

Laurent Pinchart

Re: [RFC PATCH 0/2] Add software node support to regulator framework

[Mark Brown]

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

On Mon, Jul 12, 2021 at 07:55:21PM +0300, Laurent Pinchart wrote:
> On Mon, Jul 12, 2021 at 03:15:28PM +0100, Mark Brown wrote:

> > Like I said elsewhere it seems a lot easier to just have the I2C driver
> > set platform data based on parsing DMI information like we do elsewhere.
> > I really don't see any benefit to introducing an additional layer of
> > abstraction and binding here, it just seems to be making things more
> > fragile.

> The idea behind software nodes is that individual device drivers
> shouldn't care about where platform-specific data come from. They can
> use the same fwnode API, regardless of whether the device is described
> through OF, ACPI, or software nodes created by a board file in the

That's much more fwnode than swnode.  fwnode is a cute hack for letting
ACPI based systems that don't fit the ACPI model reuse DT bindings which
can work well for some things which are just outside the scope of ACPI
completely but is a really bad idea for things where there's specific
firmware modelling for the thing being handled, in those cases it should
be avoided and firmware specific handling used instead.  Power handling
(including regulators) is one of those areas - ACPI has really strong
opinions on how it should be done and we don't want to be encouraging
systems to go against that model.  AFAICT swnode is mostly just a way of
patching up firmware that could be getting away with fwnode but didn't
for some reason, in this case we don't want to ever see ACPI systems
trying to use the DT regulator bindings in their firmware descriptions.

> kernel. It allows grouping all platform data that should be provided by
> firmware in a single place, conditioned by a DMI match, instead of
> distributing DMI matches to lots of drivers.

Like I said in reply to Andy's mail if we're essentially filling in a C
struct through a massive pile of indirection involving writing C code
containing a bunch of static initialisations of less specific structs
that doesn't seem great.  We can probably arrange to pass the init_data
from different files rather than putting the quirks in the driver,
that'd be fine if a bit more work, but swnodes only seem to be adding
problems here.

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

Re: [RFC PATCH 0/2] Add software node support to regulator framework

[Andy Shevchenko]

On Fri, Jul 9, 2021 at 8:05 PM Mark Brown <broonie@kernel.org> wrote:
> On Thu, Jul 08, 2021 at 11:42:24PM +0100, Daniel Scally wrote:

> > This series is a prototype of an emulation of the device tree regulator
> > initialisation and lookup functions, using software nodes. Software nodes
>
> What is a software node and why would we want to use one here?

Software node is a representation of (missed and / or quirked)
firmware nodes in the code.

> Why are we not just using board files, what does this new abstraction
> solve?

Software node _is_ a board file part. The idea behind that is that the
driver, which requires any additional / necessary property that has
been missed in the firmware nodes, wouldn't need special treatment if
that property comes from a board file rather than firmware.

-- 
With Best Regards,
Andy Shevchenko

On Fri, Jul 9, 2021 at 8:05 PM Mark Brown <broonie@kernel.org> wrote:
>
> On Thu, Jul 08, 2021 at 11:42:24PM +0100, Daniel Scally wrote:
>
> > See previous series for some background context [1]
>
> That's a link to a series "[PATCH v5 0/6] Introduce intel_skl_int3472
> module" which doesn't have any explanatory text as to what it's doing in
> the cover letter (just an inter version changelog) nor any obvious
> relevance to this series, are you sure that's the right link?  In
> general it's best if your patch series contains enough explanatory
> information to allow someone to have a reasonable idea what the series
> does without having to follow links like this.
>
> > This series is a prototype of an emulation of the device tree regulator
> > initialisation and lookup functions, using software nodes. Software nodes
>
> What is a software node and why would we want to use one here?
>
> > relating to each regulator are registered as children of the TPS68470's ACPI
> > firmware node. Those regulators have properties describing their constraints
> > (for example "regulator-min-microvolt"). Similarly, software nodes are
> > registered and assigned as secondary to the Camera's firmware node - these
> > software nodes have reference properties named after the supply in the same
> > way as device tree's phandles, for example "avdd-supply", and linking to the
> > software node assigned to the appropriate regulator. We can then use those
> > constraints to specify the appropriate voltages and the references to allow the
> > camera drivers to look up the correct regulator device.
>
> So these systems lack an enumerable description of the system provided
> by hardware or firmware (or given that these are ACPI systems I guess
> the firmware description is just broken) so we need to use board files.
> Why are we not just using board files, what does this new abstraction
> solve?
>
> > I'm posting this to see if people agree it's a good approach for tackling the
> > problem; I may be overthinking this and there's a much easier way that I should
>
> I don't think I understand what the problem you are trying to solve is
> so it's hard to say if this is a good approach to solving it.



-- 
With Best Regards,
Andy Shevchenko

Re: [RFC PATCH 0/2] Add software node support to regulator framework

[Mark Brown]

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

On Sun, Jul 11, 2021 at 12:37:03PM +0300, Andy Shevchenko wrote:
> On Fri, Jul 9, 2021 at 8:05 PM Mark Brown <broonie@kernel.org> wrote:
> > On Thu, Jul 08, 2021 at 11:42:24PM +0100, Daniel Scally wrote:

> > What is a software node and why would we want to use one here?

> Software node is a representation of (missed and / or quirked)
> firmware nodes in the code.

> > Why are we not just using board files, what does this new abstraction
> > solve?

> Software node _is_ a board file part. The idea behind that is that the
> driver, which requires any additional / necessary property that has
> been missed in the firmware nodes, wouldn't need special treatment if
> that property comes from a board file rather than firmware.

This doesn't seem to correspond with what these patches are doing,
they're creating something which bears no relation to any firmware
interface and the code is specifically looking for swnodes.

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

Re: [RFC PATCH 0/2] Add software node support to regulator framework

[Andy Shevchenko]

On Mon, Jul 12, 2021 at 3:42 PM Mark Brown <broonie@kernel.org> wrote:
> On Sun, Jul 11, 2021 at 12:37:03PM +0300, Andy Shevchenko wrote:
> > On Fri, Jul 9, 2021 at 8:05 PM Mark Brown <broonie@kernel.org> wrote:
> > > On Thu, Jul 08, 2021 at 11:42:24PM +0100, Daniel Scally wrote:
>
> > > What is a software node and why would we want to use one here?
>
> > Software node is a representation of (missed and / or quirked)
> > firmware nodes in the code.
>
> > > Why are we not just using board files, what does this new abstraction
> > > solve?
>
> > Software node _is_ a board file part. The idea behind that is that the
> > driver, which requires any additional / necessary property that has
> > been missed in the firmware nodes, wouldn't need special treatment if
> > that property comes from a board file rather than firmware.
>
> This doesn't seem to correspond with what these patches are doing,
> they're creating something which bears no relation to any firmware
> interface and the code is specifically looking for swnodes.

Okay, this seems like a different story.
The software nodes shouldn't appear on its own in the generic code.
When we use software nodes API in it, it means that we have tried
other providers _explicitly_ and haven't found what we are looking for
and hence we have to check if software nodes are providing the same.
For example, here it's done that way:
https://elixir.bootlin.com/linux/v5.14-rc1/source/kernel/irq/irqdomain.c#L178.

In all other cases it shouldn't be called explicitly.

-- 
With Best Regards,
Andy Shevchenko

Re: [RFC PATCH 0/2] Add software node support to regulator framework

[Mark Brown]

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

On Mon, Jul 12, 2021 at 04:01:05PM +0300, Andy Shevchenko wrote:

> The software nodes shouldn't appear on its own in the generic code.
> When we use software nodes API in it, it means that we have tried
> other providers _explicitly_ and haven't found what we are looking for
> and hence we have to check if software nodes are providing the same.
> For example, here it's done that way:
> https://elixir.bootlin.com/linux/v5.14-rc1/source/kernel/irq/irqdomain.c#L178.

> In all other cases it shouldn't be called explicitly.

But why?  I'm not seeing the advantage over providing platform data
based on DMI quirks here, it seems like a bunch of work for no reason.

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

Re: [RFC PATCH 0/2] Add software node support to regulator framework

[Andy Shevchenko]

On Mon, Jul 12, 2021 at 4:35 PM Mark Brown <broonie@kernel.org> wrote:
>
> On Mon, Jul 12, 2021 at 04:01:05PM +0300, Andy Shevchenko wrote:
>
> > The software nodes shouldn't appear on its own in the generic code.
> > When we use software nodes API in it, it means that we have tried
> > other providers _explicitly_ and haven't found what we are looking for
> > and hence we have to check if software nodes are providing the same.
> > For example, here it's done that way:
> > https://elixir.bootlin.com/linux/v5.14-rc1/source/kernel/irq/irqdomain.c#L178.
>
> > In all other cases it shouldn't be called explicitly.
>
> But why?  I'm not seeing the advantage over providing platform data
> based on DMI quirks here, it seems like a bunch of work for no reason.

What do you mean by additional work? It's exactly opposite since most
of the drivers in the kernel are using the fwnode interface rather
than platform data. Why should we _add_ the specific platform data
handling code in the certain drivers instead of not touching them at
all?

-- 
With Best Regards,
Andy Shevchenko

Re: [RFC PATCH 0/2] Add software node support to regulator framework

[Mark Brown]

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

On Mon, Jul 12, 2021 at 07:08:23PM +0300, Andy Shevchenko wrote:
> On Mon, Jul 12, 2021 at 4:35 PM Mark Brown <broonie@kernel.org> wrote:

> > But why?  I'm not seeing the advantage over providing platform data
> > based on DMI quirks here, it seems like a bunch of work for no reason.

> What do you mean by additional work? It's exactly opposite since most
> of the drivers in the kernel are using the fwnode interface rather
> than platform data. Why should we _add_ the specific platform data
> handling code in the certain drivers instead of not touching them at
> all?

It's adding an entirely new representation of standard data with less
validation support than either DT or platform data which seems like a
needlessly roundabout and error prone way of moving the data about with
less tooling support.  As far as I can tell the only advantage is that
it lets you write the quirk in a different source file but I'm finding
it hard to get excited about that.  If we were fixing up an existing
ACPI binding or something this approach would make sense to me but it's
making something entirely new out of whole cloth here.

We already have generic platform data support for the regulator API so
driver modifications would just be the DMI matching which is going to
have to happen somewhere anyway, I don't see a huge win from putting
them in one file rather than another.  It should basically just boil
down to being another data table, I imagine you could write a helper for
it, or probably even come up with some generic thing that let you
register a platform data/DMI combo independently of the driver to get it
out of the driver code (looking more like the existing GPIO code which
is already being used in another bit of this quirking).

It feels like this should be making more use of existing stuff than it
is.  If you look at what the core part of the code does it's taking data
which was provided by one part of the kernel in one set of C structs and
parsing those into a struct init_data which is what the core actually
wants to consume.  This seems like an entirely redundant process, we
should be able to just write the machine configuration into some struct
init_datas and get that associated with the regulators without creating
an otherwise entirely unused intermediate representation of the data.

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

Re: [RFC PATCH 0/2] Add software node support to regulator framework

[Daniel Scally]

Hi Mark - sorry for the late reply.

On 12/07/2021 18:01, Mark Brown wrote:
> On Mon, Jul 12, 2021 at 07:08:23PM +0300, Andy Shevchenko wrote:
>> On Mon, Jul 12, 2021 at 4:35 PM Mark Brown <broonie@kernel.org> wrote:
>>> But why?  I'm not seeing the advantage over providing platform data
>>> based on DMI quirks here, it seems like a bunch of work for no reason.
>> What do you mean by additional work? It's exactly opposite since most
>> of the drivers in the kernel are using the fwnode interface rather
>> than platform data. Why should we _add_ the specific platform data
>> handling code in the certain drivers instead of not touching them at
>> all?
> It's adding an entirely new representation of standard data with less
> validation support than either DT or platform data which seems like a
> needlessly roundabout and error prone way of moving the data about with
> less tooling support.  As far as I can tell the only advantage is that
> it lets you write the quirk in a different source file but I'm finding
> it hard to get excited about that.


I do think it can simplify driver code too; a lot of them aren't written
to parse platform data to get the init data, as they're just relying on
reading it from devicetree so in the event that we get more cases like
this, we need to modify those drivers to look for platform data too. On
the other hand, even the drivers that don't directly call
of_get_regulator_init_data() still do that lookup during the
regulator_of_get_init_data() call in regulator_register(), so the ones
that do parse platform data for init_data structs will check DT as part
of regulator_register() anyway. Imitating that seems simpler to me.


It also creates some problems to suppress the enumeration of the i2c
device via ACPI (which we'll have to do in a machine specific fashion,
because some laptops have this chip with properly configured ACPI and
thus work fine as-is) to re-enumerate it in a board file with the
platform data; the ACPI methods and buffers are used by the i2c device's
driver; there's a bunch of regrettably non-standards-compliant
information in there that we need to check to make sure we do things
properly.


I take the point about this being error-prone lacking tooling support -
happy to work on this as much as needed to alleviate your concerns if we
decide to proceed down this route.


> If we were fixing up an existing
> ACPI binding or something this approach would make sense to me but it's
> making something entirely new out of whole cloth here.
>
> We already have generic platform data support for the regulator API so
> driver modifications would just be the DMI matching which is going to
> have to happen somewhere anyway, I don't see a huge win from putting
> them in one file rather than another.  It should basically just boil
> down to being another data table, I imagine you could write a helper for
> it, or probably even come up with some generic thing that let you
> register a platform data/DMI combo independently of the driver to get it
> out of the driver code (looking more like the existing GPIO code which
> is already being used in another bit of this quirking).
>
> It feels like this should be making more use of existing stuff than it
> is.  If you look at what the core part of the code does it's taking data
> which was provided by one part of the kernel in one set of C structs and
> parsing those into a struct init_data which is what the core actually
> wants to consume.  This seems like an entirely redundant process, we
> should be able to just write the machine configuration into some struct
> init_datas and get that associated with the regulators without creating
> an otherwise entirely unused intermediate representation of the data.

The advantage of the GPIO lookups is there's no need to have the pointer
to the registered devices to register the lookup table; we could imitate
that, by adding entries to a list with the lookup values being device
and regulator name (with the init data as the thing that's "looked up")
and check for those during regulator_register() maybe?

Re: [RFC PATCH 0/2] Add software node support to regulator framework

[Mark Brown]

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

On Tue, Jul 13, 2021 at 12:32:26AM +0100, Daniel Scally wrote:

> I do think it can simplify driver code too; a lot of them aren't written
> to parse platform data to get the init data, as they're just relying on
> reading it from devicetree so in the event that we get more cases like
> this, we need to modify those drivers to look for platform data too. On
> the other hand, even the drivers that don't directly call
> of_get_regulator_init_data() still do that lookup during the
> regulator_of_get_init_data() call in regulator_register(), so the ones
> that do parse platform data for init_data structs will check DT as part
> of regulator_register() anyway. Imitating that seems simpler to me.

The driver code is trivial boilerplate, assuming someone doesn't go and
implement a helper to register stuff separately like I suggested.  The
proposed swnode stuff would involve duplicating the DT parsing code.
This seems like a whole lot of effort for something that provides a
worse result than either of the existing things.

> It also creates some problems to suppress the enumeration of the i2c
> device via ACPI (which we'll have to do in a machine specific fashion,
> because some laptops have this chip with properly configured ACPI and

To be clear I think that's a terrible idea.

> > down to being another data table, I imagine you could write a helper for
> > it, or probably even come up with some generic thing that let you
> > register a platform data/DMI combo independently of the driver to get it
> > out of the driver code (looking more like the existing GPIO code which
> > is already being used in another bit of this quirking).

> The advantage of the GPIO lookups is there's no need to have the pointer
> to the registered devices to register the lookup table; we could imitate
> that, by adding entries to a list with the lookup values being device
> and regulator name (with the init data as the thing that's "looked up")
> and check for those during regulator_register() maybe?

Like I keep saying I think that's a much better approach than trying to
use swnodes, they just seem like a terrible fit for the problem.

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

Re: [RFC PATCH 0/2] Add software node support to regulator framework

[Laurent Pinchart]

Hi Mark,

On Tue, Jul 13, 2021 at 04:24:54PM +0100, Mark Brown wrote:
> On Tue, Jul 13, 2021 at 12:32:26AM +0100, Daniel Scally wrote:
> 
> > I do think it can simplify driver code too; a lot of them aren't written
> > to parse platform data to get the init data, as they're just relying on
> > reading it from devicetree so in the event that we get more cases like
> > this, we need to modify those drivers to look for platform data too. On
> > the other hand, even the drivers that don't directly call
> > of_get_regulator_init_data() still do that lookup during the
> > regulator_of_get_init_data() call in regulator_register(), so the ones
> > that do parse platform data for init_data structs will check DT as part
> > of regulator_register() anyway. Imitating that seems simpler to me.
> 
> The driver code is trivial boilerplate, assuming someone doesn't go and
> implement a helper to register stuff separately like I suggested.  The
> proposed swnode stuff would involve duplicating the DT parsing code.
> This seems like a whole lot of effort for something that provides a
> worse result than either of the existing things.
> 
> > It also creates some problems to suppress the enumeration of the i2c
> > device via ACPI (which we'll have to do in a machine specific fashion,
> > because some laptops have this chip with properly configured ACPI and
> 
> To be clear I think that's a terrible idea.

If you're talking about the ACPI implementation on those machines,
nobody disagrees :-)

To make sure I understand you correctly, do you advocate for suppressing
registration of the I2C devices from ACPI and instantiate them from
board code instead, or to somehow supplement the I2C device with
board-specific data ?

> > > down to being another data table, I imagine you could write a helper for
> > > it, or probably even come up with some generic thing that let you
> > > register a platform data/DMI combo independently of the driver to get it
> > > out of the driver code (looking more like the existing GPIO code which
> > > is already being used in another bit of this quirking).
> 
> > The advantage of the GPIO lookups is there's no need to have the pointer
> > to the registered devices to register the lookup table; we could imitate
> > that, by adding entries to a list with the lookup values being device
> > and regulator name (with the init data as the thing that's "looked up")
> > and check for those during regulator_register() maybe?
> 
> Like I keep saying I think that's a much better approach than trying to
> use swnodes, they just seem like a terrible fit for the problem.

-- 
Regards,

Laurent Pinchart

Re: [RFC PATCH 0/2] Add software node support to regulator framework

[Mark Brown]

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

On Tue, Jul 13, 2021 at 06:42:33PM +0300, Laurent Pinchart wrote:
> On Tue, Jul 13, 2021 at 04:24:54PM +0100, Mark Brown wrote:
> > On Tue, Jul 13, 2021 at 12:32:26AM +0100, Daniel Scally wrote:

> > > It also creates some problems to suppress the enumeration of the i2c
> > > device via ACPI (which we'll have to do in a machine specific fashion,
> > > because some laptops have this chip with properly configured ACPI and

> > To be clear I think that's a terrible idea.

> If you're talking about the ACPI implementation on those machines,
> nobody disagrees :-)

> To make sure I understand you correctly, do you advocate for suppressing
> registration of the I2C devices from ACPI and instantiate them from
> board code instead, or to somehow supplement the I2C device with
> board-specific data ?

No, to repeat yet again that is what I think is a terrible idea.

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

Re: [RFC PATCH 0/2] Add software node support to regulator framework

[Laurent Pinchart]

On Tue, Jul 13, 2021 at 05:02:59PM +0100, Mark Brown wrote:
> On Tue, Jul 13, 2021 at 06:42:33PM +0300, Laurent Pinchart wrote:
> > On Tue, Jul 13, 2021 at 04:24:54PM +0100, Mark Brown wrote:
> > > On Tue, Jul 13, 2021 at 12:32:26AM +0100, Daniel Scally wrote:
> > > > It also creates some problems to suppress the enumeration of the i2c
> > > > device via ACPI (which we'll have to do in a machine specific fashion,
> > > > because some laptops have this chip with properly configured ACPI and
> > >
> > > To be clear I think that's a terrible idea.
> >
> > If you're talking about the ACPI implementation on those machines,
> > nobody disagrees :-)
> >
> > To make sure I understand you correctly, do you advocate for suppressing
> > registration of the I2C devices from ACPI and instantiate them from
> > board code instead, or to somehow supplement the I2C device with
> > board-specific data ?
> 
> No, to repeat yet again that is what I think is a terrible idea.

Which of those two ? :-)

-- 
Regards,

Laurent Pinchart

Re: [RFC PATCH 0/2] Add software node support to regulator framework

[Mark Brown]

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

On Tue, Jul 13, 2021 at 07:06:20PM +0300, Laurent Pinchart wrote:
> On Tue, Jul 13, 2021 at 05:02:59PM +0100, Mark Brown wrote:

> > > To make sure I understand you correctly, do you advocate for suppressing
> > > registration of the I2C devices from ACPI and instantiate them from
> > > board code instead, or to somehow supplement the I2C device with
> > > board-specific data ?

> > No, to repeat yet again that is what I think is a terrible idea.

> Which of those two ? :-)

Suppressing the registration data.  Frankly the way that ACPI systems
rely so extensively on OS provided fixups on non-server systems while
still being deployed routinely there is also a substantial failure, but
it's not quite so bad.

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

Re: [RFC PATCH 0/2] Add software node support to regulator framework

[Andy Shevchenko]

On Tue, Jul 13, 2021 at 6:25 PM Mark Brown <broonie@kernel.org> wrote:
> On Tue, Jul 13, 2021 at 12:32:26AM +0100, Daniel Scally wrote:

> > I do think it can simplify driver code too; a lot of them aren't written
> > to parse platform data to get the init data, as they're just relying on
> > reading it from devicetree so in the event that we get more cases like
> > this, we need to modify those drivers to look for platform data too. On
> > the other hand, even the drivers that don't directly call
> > of_get_regulator_init_data() still do that lookup during the
> > regulator_of_get_init_data() call in regulator_register(), so the ones
> > that do parse platform data for init_data structs will check DT as part
> > of regulator_register() anyway. Imitating that seems simpler to me.
>
> The driver code is trivial boilerplate, assuming someone doesn't go and
> implement a helper to register stuff separately like I suggested.  The
> proposed swnode stuff would involve duplicating the DT parsing code.
> This seems like a whole lot of effort for something that provides a
> worse result than either of the existing things.

I'm not sure I follow. Where did you see the duplication when I saw
the other way around?
Converting code from OF to fwnode APIs in most cases is smooth and
doesn't add any overhead to the codebase,

-- 
With Best Regards,
Andy Shevchenko

Re: [RFC PATCH 0/2] Add software node support to regulator framework

[Mark Brown]

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

On Tue, Jul 13, 2021 at 06:55:59PM +0300, Andy Shevchenko wrote:
> On Tue, Jul 13, 2021 at 6:25 PM Mark Brown <broonie@kernel.org> wrote:

> > The driver code is trivial boilerplate, assuming someone doesn't go and
> > implement a helper to register stuff separately like I suggested.  The
> > proposed swnode stuff would involve duplicating the DT parsing code.
> > This seems like a whole lot of effort for something that provides a
> > worse result than either of the existing things.

> I'm not sure I follow. Where did you see the duplication when I saw
> the other way around?

The current patch consists entirely of additions, it does not remove any
existing code at all, the diffstat is:

 5 files changed, 174 insertions(+)

> Converting code from OF to fwnode APIs in most cases is smooth and
> doesn't add any overhead to the codebase,

We explicitly do not want to attempt to parse regulator properties out
of ACPI platform descriptions because using the regulator binding on
ACPI platforms conflicts with the ACPI model for power management and
we really don't want to encourage platforms to attempt to mix and match
here, it's not going to lead to anything robust.  System integrators
that need this sort of OS visible low level power management really
should be working with the UEFI forum to get an ACPI specification for
it, or if they don't really need it fixing up their AML to DTRT.

If you were to say that we could bodge around that by somehow forcing
this binding to exist only for swnodes when running on ACPI systems then
we'd still have the problems with creating something with worse tooling
than what's there already.

Like I said in the other mail fwnode is a nice hack for systems that are
using ACPI but have hardware that's doing something totally outside the
ACPI model to allow them to reuse work that's been done for DT, it's not
a universal solution to the lack of appropriate support for describing
modern systems in ACPI.

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

Re: [RFC PATCH 0/2] Add software node support to regulator framework

[Andy Shevchenko]

On Tue, Jul 13, 2021 at 9:19 PM Mark Brown <broonie@kernel.org> wrote:
>
> On Tue, Jul 13, 2021 at 06:55:59PM +0300, Andy Shevchenko wrote:
> > On Tue, Jul 13, 2021 at 6:25 PM Mark Brown <broonie@kernel.org> wrote:
>
> > > The driver code is trivial boilerplate, assuming someone doesn't go and
> > > implement a helper to register stuff separately like I suggested.  The
> > > proposed swnode stuff would involve duplicating the DT parsing code.
> > > This seems like a whole lot of effort for something that provides a
> > > worse result than either of the existing things.
>
> > I'm not sure I follow. Where did you see the duplication when I saw
> > the other way around?
>
> The current patch consists entirely of additions, it does not remove any
> existing code at all, the diffstat is:
>
>  5 files changed, 174 insertions(+)

Ah, okay, you are talking with regard to the current patch. I talked
in generic terms.

> > Converting code from OF to fwnode APIs in most cases is smooth and
> > doesn't add any overhead to the codebase,
>
> We explicitly do not want to attempt to parse regulator properties out
> of ACPI platform descriptions because using the regulator binding on
> ACPI platforms conflicts with the ACPI model for power management and
> we really don't want to encourage platforms to attempt to mix and match
> here, it's not going to lead to anything robust.  System integrators
> that need this sort of OS visible low level power management really
> should be working with the UEFI forum to get an ACPI specification for
> it, or if they don't really need it fixing up their AML to DTRT.

No-one is objecting to this. I agree that integration of regulators
and ACPI should be done in a specific way if needed at all.

> If you were to say that we could bodge around that by somehow forcing
> this binding to exist only for swnodes when running on ACPI systems then
> we'd still have the problems with creating something with worse tooling
> than what's there already.

Of course, no objections to this.

> Like I said in the other mail fwnode is a nice hack for systems that are
> using ACPI but have hardware that's doing something totally outside the
> ACPI model to allow them to reuse work that's been done for DT, it's not
> a universal solution to the lack of appropriate support for describing
> modern systems in ACPI.

In some (I suppose rear) cases it may be used by DT-enabled platforms as well.
I can imagine the case when you have a system in ROM and only what you
can do to change DTB there is either use DT overlays (which seems to
be not working, plenty of gaps there according to a Wiki I saw once)
or do something in the board files.

So, if you replace "ACPI" with the "firmware resource provider" in the
above paragraph, I will agree 100% with you.

-- 
With Best Regards,
Andy Shevchenko

Re: [RFC PATCH 0/2] Add software node support to regulator framework

[Mark Brown]

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

On Tue, Jul 13, 2021 at 10:46:29PM +0300, Andy Shevchenko wrote:
> On Tue, Jul 13, 2021 at 9:19 PM Mark Brown <broonie@kernel.org> wrote:

> > Like I said in the other mail fwnode is a nice hack for systems that are
> > using ACPI but have hardware that's doing something totally outside the
> > ACPI model to allow them to reuse work that's been done for DT, it's not
> > a universal solution to the lack of appropriate support for describing
> > modern systems in ACPI.

> In some (I suppose rear) cases it may be used by DT-enabled platforms as well.
> I can imagine the case when you have a system in ROM and only what you
> can do to change DTB there is either use DT overlays (which seems to
> be not working, plenty of gaps there according to a Wiki I saw once)
> or do something in the board files.

DT overlays are pretty extensively deployed, it's just that there's very
few use cases where it's done in the kernel - it tends to be better to
do them in either the firmware or a thin wrapper around the kernel so
that the kernel never has to see an unfixed DT, and we don't have to
try to do fixups super early in the boot on a supposedly generic kernel.
The main kernel use cases are things like FPGAs loading a dynamic image
and matching DT at runtime which is a whole additional ball of fun.

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

Re: [RFC PATCH 0/2] Add software node support to regulator framework

[Laurent Pinchart]

Hi Mark,

On Tue, Jul 13, 2021 at 07:18:37PM +0100, Mark Brown wrote:
> On Tue, Jul 13, 2021 at 06:55:59PM +0300, Andy Shevchenko wrote:
> > On Tue, Jul 13, 2021 at 6:25 PM Mark Brown <broonie@kernel.org> wrote:
> 
> > > The driver code is trivial boilerplate, assuming someone doesn't go and
> > > implement a helper to register stuff separately like I suggested.  The
> > > proposed swnode stuff would involve duplicating the DT parsing code.
> > > This seems like a whole lot of effort for something that provides a
> > > worse result than either of the existing things.
> 
> > I'm not sure I follow. Where did you see the duplication when I saw
> > the other way around?
> 
> The current patch consists entirely of additions, it does not remove any
> existing code at all, the diffstat is:
> 
>  5 files changed, 174 insertions(+)
> 
> > Converting code from OF to fwnode APIs in most cases is smooth and
> > doesn't add any overhead to the codebase,
> 
> We explicitly do not want to attempt to parse regulator properties out
> of ACPI platform descriptions because using the regulator binding on
> ACPI platforms conflicts with the ACPI model for power management and
> we really don't want to encourage platforms to attempt to mix and match
> here, it's not going to lead to anything robust.  System integrators
> that need this sort of OS visible low level power management really
> should be working with the UEFI forum to get an ACPI specification for
> it, or if they don't really need it fixing up their AML to DTRT.
> 
> If you were to say that we could bodge around that by somehow forcing
> this binding to exist only for swnodes when running on ACPI systems then
> we'd still have the problems with creating something with worse tooling
> than what's there already.
> 
> Like I said in the other mail fwnode is a nice hack for systems that are
> using ACPI but have hardware that's doing something totally outside the
> ACPI model to allow them to reuse work that's been done for DT, it's not
> a universal solution to the lack of appropriate support for describing
> modern systems in ACPI.

fwnode, as an abstraction of ACPI and OF, is quite useful for camera
sensor drivers for instance. They need to read firmware properties (for
instance to identify whether a camera is located on the front or back of
the device, or to what port of the SoC it's connected), and being able
to do so without duplicating OF and ACPI code in drivers is useful.

swnode, on the other hand, is indeed more of a workaround for a
more-often-than-not broken ACPI implementation. It's ironic to think
that x86 ACPI-based systems, touted as being superior to ARM, are now in
a worst state than OF-based systems.

-- 
Regards,

Laurent Pinchart

Re: [RFC PATCH 0/2] Add software node support to regulator framework

[Mark Brown]

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

On Wed, Jul 14, 2021 at 10:25:02AM +0300, Laurent Pinchart wrote:
> On Tue, Jul 13, 2021 at 07:18:37PM +0100, Mark Brown wrote:

> > Like I said in the other mail fwnode is a nice hack for systems that are
> > using ACPI but have hardware that's doing something totally outside the
> > ACPI model to allow them to reuse work that's been done for DT, it's not
> > a universal solution to the lack of appropriate support for describing
> > modern systems in ACPI.

> fwnode, as an abstraction of ACPI and OF, is quite useful for camera
> sensor drivers for instance. They need to read firmware properties (for
> instance to identify whether a camera is located on the front or back of
> the device, or to what port of the SoC it's connected), and being able
> to do so without duplicating OF and ACPI code in drivers is useful.

I'd still say that's a bit of a hack, it's the sort of area where ACPI
just has absolutely no handling at all and so picking up the DT bindings
will work effectively although it results in something that's really not
at all idiomatic for ACPI since idiomatic DT and idiomatic ACPI don't
really look like each other and AIUI this stuff isn't getting adopted
for actual firmware (as opposed to swnodes) outside of the embedded x86
space.

> swnode, on the other hand, is indeed more of a workaround for a
> more-often-than-not broken ACPI implementation. It's ironic to think
> that x86 ACPI-based systems, touted as being superior to ARM, are now in
> a worst state than OF-based systems.

The unfortunate thing is that ACPI is super limited in what systems it
models, making assumptions that only really work for fairly simple
server class systems.  Outside of that the models it's offering just
can't cope with actual hardware yet people still insist on building
those systems with ACPI system descriptions so you end up with huge
piles of platform quirks.  Audio support for modern x86 laptops is just
an endless procession of quirks :(

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

Re: [RFC PATCH 0/2] Add software node support to regulator framework

[Laurent Pinchart]

On Wed, Jul 14, 2021 at 05:59:48PM +0100, Mark Brown wrote:
> On Wed, Jul 14, 2021 at 10:25:02AM +0300, Laurent Pinchart wrote:
> > On Tue, Jul 13, 2021 at 07:18:37PM +0100, Mark Brown wrote:
> 
> > > Like I said in the other mail fwnode is a nice hack for systems that are
> > > using ACPI but have hardware that's doing something totally outside the
> > > ACPI model to allow them to reuse work that's been done for DT, it's not
> > > a universal solution to the lack of appropriate support for describing
> > > modern systems in ACPI.
> 
> > fwnode, as an abstraction of ACPI and OF, is quite useful for camera
> > sensor drivers for instance. They need to read firmware properties (for
> > instance to identify whether a camera is located on the front or back of
> > the device, or to what port of the SoC it's connected), and being able
> > to do so without duplicating OF and ACPI code in drivers is useful.
> 
> I'd still say that's a bit of a hack, it's the sort of area where ACPI
> just has absolutely no handling at all and so picking up the DT bindings
> will work effectively although it results in something that's really not
> at all idiomatic for ACPI since idiomatic DT and idiomatic ACPI don't
> really look like each other and AIUI this stuff isn't getting adopted
> for actual firmware (as opposed to swnodes) outside of the embedded x86
> space.

It's only one data point, but we're seeing adoption of the ACPI
DT-in-DSD for camera. It's still not pretty of course.

> > swnode, on the other hand, is indeed more of a workaround for a
> > more-often-than-not broken ACPI implementation. It's ironic to think
> > that x86 ACPI-based systems, touted as being superior to ARM, are now in
> > a worst state than OF-based systems.
> 
> The unfortunate thing is that ACPI is super limited in what systems it
> models, making assumptions that only really work for fairly simple
> server class systems.  Outside of that the models it's offering just
> can't cope with actual hardware yet people still insist on building
> those systems with ACPI system descriptions so you end up with huge
> piles of platform quirks.  Audio support for modern x86 laptops is just
> an endless procession of quirks :(

I feel your pain. On the camera side, we have a case of an I2C
controller that has a PCI driver in the kernel used when dealing with
the camera sensor, and an AML "driver" used by an I2C GPIO expander,
completely modelled in the DSDT. Both poke the same PCI registers of the
I2C controller. I tried to get information from Intel on how this was
meant to be handled, but I think the people responsible for the design
have been exfiltrated to Guantanamo, or are expiating their sins in a
monastery lost in a mountain.

Once travel will be easier again, we'll plot a take over of the world in
a bar. Dealing with ACPI requires lots of whisky :-)

-- 
Regards,

Laurent Pinchart

Re: [RFC PATCH 0/2] Add software node support to regulator framework

[Mark Brown]

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

On Wed, Jul 14, 2021 at 08:18:13PM +0300, Laurent Pinchart wrote:
> On Wed, Jul 14, 2021 at 05:59:48PM +0100, Mark Brown wrote:

> > really look like each other and AIUI this stuff isn't getting adopted
> > for actual firmware (as opposed to swnodes) outside of the embedded x86
> > space.

> It's only one data point, but we're seeing adoption of the ACPI
> DT-in-DSD for camera. It's still not pretty of course.

By non-Linux system vendors?  My understanding has been that for audio
people are just unwilling to provide the level of firmware description
required to avoid quirks, there was some limited stuff with the NHLT
table but it still required system software to quirk things.  If there's
progress elsewhere perhaps the relevant people can be persuaded to have
another go...

> Once travel will be easier again, we'll plot a take over of the world in
> a bar. Dealing with ACPI requires lots of whisky :-)

Indeed!

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

Re: [RFC PATCH 0/2] Add software node support to regulator framework

[Laurent Pinchart]

On Wed, Jul 14, 2021 at 06:28:46PM +0100, Mark Brown wrote:
> On Wed, Jul 14, 2021 at 08:18:13PM +0300, Laurent Pinchart wrote:
> > On Wed, Jul 14, 2021 at 05:59:48PM +0100, Mark Brown wrote:
> 
> > > really look like each other and AIUI this stuff isn't getting adopted
> > > for actual firmware (as opposed to swnodes) outside of the embedded x86
> > > space.
> 
> > It's only one data point, but we're seeing adoption of the ACPI
> > DT-in-DSD for camera. It's still not pretty of course.
> 
> By non-Linux system vendors?

For Windows-based machines, yes. It's fairly new, and the information I
have is that those machines may ship DSDT containing both Windows-style
(read: crap) data and Linux-style data for the same nodes. My fear is
that only the former will be properly tested and the latter will thus be
incorrect. The future will tell (I'm as usual very hopeful).

> My understanding has been that for audio people are just unwilling to
> provide the level of firmware description required to avoid quirks,
> there was some limited stuff with the NHLT table but it still required
> system software to quirk things.  If there's progress elsewhere
> perhaps the relevant people can be persuaded to have another go...
> 
> > Once travel will be easier again, we'll plot a take over of the world in
> > a bar. Dealing with ACPI requires lots of whisky :-)
> 
> Indeed!

-- 
Regards,

Laurent Pinchart

Re: [RFC PATCH 0/2] Add software node support to regulator framework

[Mark Brown]

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

On Wed, Jul 14, 2021 at 08:41:27PM +0300, Laurent Pinchart wrote:
> On Wed, Jul 14, 2021 at 06:28:46PM +0100, Mark Brown wrote:
> > On Wed, Jul 14, 2021 at 08:18:13PM +0300, Laurent Pinchart wrote:

> > > It's only one data point, but we're seeing adoption of the ACPI
> > > DT-in-DSD for camera. It's still not pretty of course.

> > By non-Linux system vendors?

> For Windows-based machines, yes. It's fairly new, and the information I
> have is that those machines may ship DSDT containing both Windows-style
> (read: crap) data and Linux-style data for the same nodes. My fear is
> that only the former will be properly tested and the latter will thus be
> incorrect. The future will tell (I'm as usual very hopeful).

Adding the Intel audio people - it'd be good if we could get some
similar stuff started for the audio things.  Sadly in these sorts of
cases AIUI the Windows thing is broadly to match DMI data and supply
platform data so it's more a case of just not having essential
information in firmware, a bad format would be better TBH (assuming it's
accurate which also requires loads of quirks...).

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

Re: [RFC PATCH 0/2] Add software node support to regulator framework

[Laurent Pinchart]

On Wed, Jul 14, 2021 at 08:18:55PM +0100, Mark Brown wrote:
> On Wed, Jul 14, 2021 at 08:41:27PM +0300, Laurent Pinchart wrote:
> > On Wed, Jul 14, 2021 at 06:28:46PM +0100, Mark Brown wrote:
> > > On Wed, Jul 14, 2021 at 08:18:13PM +0300, Laurent Pinchart wrote:
> > > > It's only one data point, but we're seeing adoption of the ACPI
> > > > DT-in-DSD for camera. It's still not pretty of course.
> > >
> > > By non-Linux system vendors?
> >
> > For Windows-based machines, yes. It's fairly new, and the information I
> > have is that those machines may ship DSDT containing both Windows-style
> > (read: crap) data and Linux-style data for the same nodes. My fear is
> > that only the former will be properly tested and the latter will thus be
> > incorrect. The future will tell (I'm as usual very hopeful).
> 
> Adding the Intel audio people - it'd be good if we could get some
> similar stuff started for the audio things.  Sadly in these sorts of
> cases AIUI the Windows thing is broadly to match DMI data and supply
> platform data so it's more a case of just not having essential
> information in firmware, a bad format would be better TBH (assuming it's
> accurate which also requires loads of quirks...).

On the camera side, the Windows-based machines I've worked with (Skylake
and Kabylak) have data in the DSDT. There's data we can use directly,
and there's a lot that is hardcoded in the Windows driver (including
what voltage to program on the different outputs of an I2C-controlled
regulator - you get that wrong, you fry your camera). I believe Intel
provides a small set of reference designs with several options to the
OEMs, and I wouldn't be surprised if some of the data present in ACPI
that we don't know how to interpret would identify these options. I
don't think the Windows driver has DMI-based quirks, the driver isn't
machine-specific as far as I can tell.

For newer devices, ACPI should contain Windows data in a format that the
Windows team decides on its own, and data that is actually usable in the
_DSD for Linux. I've also heard that the power management would be
saner, with PM actually implemented in the DSDT. I haven't seen those
DSDT yet though.

-- 
Regards,

Laurent Pinchart

Re: [RFC PATCH 0/2] Add software node support to regulator framework

[Daniel Scally]

Hi Mark

On 13/07/2021 16:24, Mark Brown wrote:
> On Tue, Jul 13, 2021 at 12:32:26AM +0100, Daniel Scally wrote:
>
>> I do think it can simplify driver code too; a lot of them aren't written
>> to parse platform data to get the init data, as they're just relying on
>> reading it from devicetree so in the event that we get more cases like
>> this, we need to modify those drivers to look for platform data too. On
>> the other hand, even the drivers that don't directly call
>> of_get_regulator_init_data() still do that lookup during the
>> regulator_of_get_init_data() call in regulator_register(), so the ones
>> that do parse platform data for init_data structs will check DT as part
>> of regulator_register() anyway. Imitating that seems simpler to me.
> The driver code is trivial boilerplate, assuming someone doesn't go and
> implement a helper to register stuff separately like I suggested.  The
> proposed swnode stuff would involve duplicating the DT parsing code.
> This seems like a whole lot of effort for something that provides a
> worse result than either of the existing things.


Alright - let me look at adding a helper to register them instead then.

>> It also creates some problems to suppress the enumeration of the i2c
>> device via ACPI (which we'll have to do in a machine specific fashion,
>> because some laptops have this chip with properly configured ACPI and
> To be clear I think that's a terrible idea.


Me too. I thought you were suggesting that I do that - sorry to have
misunderstood there. The problem we're trying to resolve here is kinda
exacerbated by a lot non-standard stuff within the ACPI for these
devices, so for example, on top of not having any power management AML
(which is what I'm trying to fix up here), the ACPI device can actually
represent a bunch of different things that might be a TPS68470 PMIC, a
different PMIC entirely or even not a physical PMIC at all, but rather
just a convenient dummy device to collect a bunch of GPIOs under. Which
one it is is revealed by parsing a buffer out of the device's ACPI, so
we need the ACPI enumeration to be able to use that properly.


Part of the reasons I went with addressing this with software nodes is
that we've used them to correct other deficiencies in the ACPI on the
same devices, like the references between the cameras and the image
signal processor ought to be described in _DSD packages [1], but are
again just hidden inside a buffer that we need to parse to figure out
the right way to make the connection, and then we used software nodes to
represent that. The difference there is that that's implementing
something that should have been there in the first place, whereas the
regulators wouldn't ordinarily be described in this way.

>>> down to being another data table, I imagine you could write a helper for
>>> it, or probably even come up with some generic thing that let you
>>> register a platform data/DMI combo independently of the driver to get it
>>> out of the driver code (looking more like the existing GPIO code which
>>> is already being used in another bit of this quirking).
>> The advantage of the GPIO lookups is there's no need to have the pointer
>> to the registered devices to register the lookup table; we could imitate
>> that, by adding entries to a list with the lookup values being device
>> and regulator name (with the init data as the thing that's "looked up")
>> and check for those during regulator_register() maybe?
> Like I keep saying I think that's a much better approach than trying to
> use swnodes, they just seem like a terrible fit for the problem.


Okedokey; I'm happy to take another look at it from this angle then -
thanks for the feedback.


[1]
https://www.kernel.org/doc/html/latest/firmware-guide/acpi/dsd/graph.html

Re: [RFC PATCH 0/2] Add software node support to regulator framework

[Laurent Pinchart]

Hi Dan,

On Thu, Jul 08, 2021 at 11:42:24PM +0100, Daniel Scally wrote:
> Hello all
> 
> See previous series for some background context [1]
> 
> Some x86 laptops with ACPI tables designed for Windows have a TPS68470
> PMIC providing regulators and clocks to camera modules. The DSDT tables for
> those cameras lack any power control methods, declaring only a
> dependency on the ACPI device representing the TPS68470. This leaves the
> regulator framework with no means of determining appropriate voltages for the
> regulators provided by the PMIC, or of determining which regulators relate to
> which of the sensor's requested supplies. 
> 
> This series is a prototype of an emulation of the device tree regulator
> initialisation and lookup functions, using software nodes. Software nodes
> relating to each regulator are registered as children of the TPS68470's ACPI
> firmware node. Those regulators have properties describing their constraints
> (for example "regulator-min-microvolt"). Similarly, software nodes are
> registered and assigned as secondary to the Camera's firmware node - these
> software nodes have reference properties named after the supply in the same
> way as device tree's phandles, for example "avdd-supply", and linking to the
> software node assigned to the appropriate regulator. We can then use those
> constraints to specify the appropriate voltages and the references to allow the
> camera drivers to look up the correct regulator device. 
> 
> Although not included in this series, I would plan to use a similar method for
> linking the clocks provided by the TPS68470 to the sensor so that it can be
> discovered too.
> 
> I'm posting this to see if people agree it's a good approach for tackling the 
> problem; I may be overthinking this and there's a much easier way that I should
> be looking at instead. It will have knock-ons in the cio2-bridge code [2], as
> that is adding software nodes to the same sensors to connect them to the media
> graph. Similarly, is the board file an acceptable solution, or should we just
> define the configuration for these devices (there's three orf our laptop models
> in scope) in int3472-tps68470 instead?

I may have missed something, but if you load the SGo2 board file, won't
it create the regulator software nodes if it finds an INT3472,
regardless of whether the device is an SGo2 ? If you happen to do so on
a machine that requires different voltages, that sounds dangerous.

Given that INT3472 models the virtual "Intel Skylake and Kabylake camera
PMIC", I think moving device-specific information to the int3472 driver
may make sense. I'm unsure what option is best though, having all the
data (regulators, clocks, but also data currently stored in the
cio2-bridge driver) in a single file (or a single file per machine) is
tempting.

> [1] https://lore.kernel.org/lkml/20210603224007.120560-1-djrscally@gmail.com/
> [2] https://elixir.bootlin.com/linux/latest/source/drivers/media/pci/intel/ipu3/cio2-bridge.c#L166
> 
> 
> Daniel Scally (2):
>   regulator: Add support for software node connections
>   platform/surface: Add Surface Go 2 board file
> 
>  MAINTAINERS                                |   6 +
>  drivers/platform/surface/Kconfig           |  10 ++
>  drivers/platform/surface/Makefile          |   1 +
>  drivers/platform/surface/surface_go_2.c    | 135 +++++++++++++++++++++
>  drivers/regulator/Kconfig                  |   6 +
>  drivers/regulator/Makefile                 |   1 +
>  drivers/regulator/core.c                   |  23 ++++
>  drivers/regulator/swnode_regulator.c       | 111 +++++++++++++++++
>  include/linux/regulator/swnode_regulator.h |  33 +++++
>  9 files changed, 326 insertions(+)
>  create mode 100644 drivers/platform/surface/surface_go_2.c
>  create mode 100644 drivers/regulator/swnode_regulator.c
>  create mode 100644 include/linux/regulator/swnode_regulator.h

-- 
Regards,

Laurent Pinchart

Re: [RFC PATCH 0/2] Add software node support to regulator framework

[Daniel Scally]

Hi Laurent

On 10/07/2021 23:28, Laurent Pinchart wrote:
> Hi Dan,
>
> On Thu, Jul 08, 2021 at 11:42:24PM +0100, Daniel Scally wrote:
>> Hello all
>>
>> See previous series for some background context [1]
>>
>> Some x86 laptops with ACPI tables designed for Windows have a TPS68470
>> PMIC providing regulators and clocks to camera modules. The DSDT tables for
>> those cameras lack any power control methods, declaring only a
>> dependency on the ACPI device representing the TPS68470. This leaves the
>> regulator framework with no means of determining appropriate voltages for the
>> regulators provided by the PMIC, or of determining which regulators relate to
>> which of the sensor's requested supplies. 
>>
>> This series is a prototype of an emulation of the device tree regulator
>> initialisation and lookup functions, using software nodes. Software nodes
>> relating to each regulator are registered as children of the TPS68470's ACPI
>> firmware node. Those regulators have properties describing their constraints
>> (for example "regulator-min-microvolt"). Similarly, software nodes are
>> registered and assigned as secondary to the Camera's firmware node - these
>> software nodes have reference properties named after the supply in the same
>> way as device tree's phandles, for example "avdd-supply", and linking to the
>> software node assigned to the appropriate regulator. We can then use those
>> constraints to specify the appropriate voltages and the references to allow the
>> camera drivers to look up the correct regulator device. 
>>
>> Although not included in this series, I would plan to use a similar method for
>> linking the clocks provided by the TPS68470 to the sensor so that it can be
>> discovered too.
>>
>> I'm posting this to see if people agree it's a good approach for tackling the 
>> problem; I may be overthinking this and there's a much easier way that I should
>> be looking at instead. It will have knock-ons in the cio2-bridge code [2], as
>> that is adding software nodes to the same sensors to connect them to the media
>> graph. Similarly, is the board file an acceptable solution, or should we just
>> define the configuration for these devices (there's three orf our laptop models
>> in scope) in int3472-tps68470 instead?
> I may have missed something, but if you load the SGo2 board file, won't
> it create the regulator software nodes if it finds an INT3472,
> regardless of whether the device is an SGo2 ? If you happen to do so on
> a machine that requires different voltages, that sounds dangerous.


Ah, yes - hadn't thought of that. If a driver registered regulators with
those names, it would try to apply those voltages during registration.
Good point.

> Given that INT3472 models the virtual "Intel Skylake and Kabylake camera
> PMIC", I think moving device-specific information to the int3472 driver
> may make sense. I'm unsure what option is best though, having all the
> data (regulators, clocks, but also data currently stored in the
> cio2-bridge driver) in a single file (or a single file per machine) is
> tempting.


It is tempting, particularly because (assuming we do end up using this
approach) setting the references to the supplies in a board file like
this complicated the cio2-bridge code quite a bit, since it then needs
to extend the properties array against an already-existing software node
rather than registering a new one. But then, I don't particularly want
to handle that aspect of the problem in two separate places.

>> [1] https://lore.kernel.org/lkml/20210603224007.120560-1-djrscally@gmail.com/
>> [2] https://elixir.bootlin.com/linux/latest/source/drivers/media/pci/intel/ipu3/cio2-bridge.c#L166
>>
>>
>> Daniel Scally (2):
>>   regulator: Add support for software node connections
>>   platform/surface: Add Surface Go 2 board file
>>
>>  MAINTAINERS                                |   6 +
>>  drivers/platform/surface/Kconfig           |  10 ++
>>  drivers/platform/surface/Makefile          |   1 +
>>  drivers/platform/surface/surface_go_2.c    | 135 +++++++++++++++++++++
>>  drivers/regulator/Kconfig                  |   6 +
>>  drivers/regulator/Makefile                 |   1 +
>>  drivers/regulator/core.c                   |  23 ++++
>>  drivers/regulator/swnode_regulator.c       | 111 +++++++++++++++++
>>  include/linux/regulator/swnode_regulator.h |  33 +++++
>>  9 files changed, 326 insertions(+)
>>  create mode 100644 drivers/platform/surface/surface_go_2.c
>>  create mode 100644 drivers/regulator/swnode_regulator.c
>>  create mode 100644 include/linux/regulator/swnode_regulator.h

Re: [RFC PATCH 0/2] Add software node support to regulator framework

[Laurent Pinchart]

Hi Dan,

On Sat, Jul 10, 2021 at 11:54:26PM +0100, Daniel Scally wrote:
> On 10/07/2021 23:28, Laurent Pinchart wrote:
> > On Thu, Jul 08, 2021 at 11:42:24PM +0100, Daniel Scally wrote:
> >> Hello all
> >>
> >> See previous series for some background context [1]
> >>
> >> Some x86 laptops with ACPI tables designed for Windows have a TPS68470
> >> PMIC providing regulators and clocks to camera modules. The DSDT tables for
> >> those cameras lack any power control methods, declaring only a
> >> dependency on the ACPI device representing the TPS68470. This leaves the
> >> regulator framework with no means of determining appropriate voltages for the
> >> regulators provided by the PMIC, or of determining which regulators relate to
> >> which of the sensor's requested supplies. 
> >>
> >> This series is a prototype of an emulation of the device tree regulator
> >> initialisation and lookup functions, using software nodes. Software nodes
> >> relating to each regulator are registered as children of the TPS68470's ACPI
> >> firmware node. Those regulators have properties describing their constraints
> >> (for example "regulator-min-microvolt"). Similarly, software nodes are
> >> registered and assigned as secondary to the Camera's firmware node - these
> >> software nodes have reference properties named after the supply in the same
> >> way as device tree's phandles, for example "avdd-supply", and linking to the
> >> software node assigned to the appropriate regulator. We can then use those
> >> constraints to specify the appropriate voltages and the references to allow the
> >> camera drivers to look up the correct regulator device. 
> >>
> >> Although not included in this series, I would plan to use a similar method for
> >> linking the clocks provided by the TPS68470 to the sensor so that it can be
> >> discovered too.
> >>
> >> I'm posting this to see if people agree it's a good approach for tackling the 
> >> problem; I may be overthinking this and there's a much easier way that I should
> >> be looking at instead. It will have knock-ons in the cio2-bridge code [2], as
> >> that is adding software nodes to the same sensors to connect them to the media
> >> graph. Similarly, is the board file an acceptable solution, or should we just
> >> define the configuration for these devices (there's three orf our laptop models
> >> in scope) in int3472-tps68470 instead?
> >
> > I may have missed something, but if you load the SGo2 board file, won't
> > it create the regulator software nodes if it finds an INT3472,
> > regardless of whether the device is an SGo2 ? If you happen to do so on
> > a machine that requires different voltages, that sounds dangerous.
> 
> Ah, yes - hadn't thought of that. If a driver registered regulators with
> those names, it would try to apply those voltages during registration.
> Good point.
> 
> > Given that INT3472 models the virtual "Intel Skylake and Kabylake camera
> > PMIC", I think moving device-specific information to the int3472 driver
> > may make sense. I'm unsure what option is best though, having all the
> > data (regulators, clocks, but also data currently stored in the
> > cio2-bridge driver) in a single file (or a single file per machine) is
> > tempting.
> 
> It is tempting, particularly because (assuming we do end up using this
> approach) setting the references to the supplies in a board file like
> this complicated the cio2-bridge code quite a bit, since it then needs
> to extend the properties array against an already-existing software node
> rather than registering a new one. But then, I don't particularly want
> to handle that aspect of the problem in two separate places.

If technically feasible, gathering all the data in a single place would
be my preference. Whether that should take the form of software nodes in
all cases, or be modelled as custom data that the int3472 driver would
interpret to create the regulators and clocks is a different (but
related) question.

The very important part is to ensure that the correct board data will be
used, as otherwise we could damage the hardware.

> >> [1] https://lore.kernel.org/lkml/20210603224007.120560-1-djrscally@gmail.com/
> >> [2] https://elixir.bootlin.com/linux/latest/source/drivers/media/pci/intel/ipu3/cio2-bridge.c#L166
> >>
> >>
> >> Daniel Scally (2):
> >>   regulator: Add support for software node connections
> >>   platform/surface: Add Surface Go 2 board file
> >>
> >>  MAINTAINERS                                |   6 +
> >>  drivers/platform/surface/Kconfig           |  10 ++
> >>  drivers/platform/surface/Makefile          |   1 +
> >>  drivers/platform/surface/surface_go_2.c    | 135 +++++++++++++++++++++
> >>  drivers/regulator/Kconfig                  |   6 +
> >>  drivers/regulator/Makefile                 |   1 +
> >>  drivers/regulator/core.c                   |  23 ++++
> >>  drivers/regulator/swnode_regulator.c       | 111 +++++++++++++++++
> >>  include/linux/regulator/swnode_regulator.h |  33 +++++
> >>  9 files changed, 326 insertions(+)
> >>  create mode 100644 drivers/platform/surface/surface_go_2.c
> >>  create mode 100644 drivers/regulator/swnode_regulator.c
> >>  create mode 100644 include/linux/regulator/swnode_regulator.h

-- 
Regards,

Laurent Pinchart

Re: [RFC PATCH 0/2] Add software node support to regulator framework

[Daniel Scally]

Hi Laurent

On 11/07/2021 17:55, Laurent Pinchart wrote:
> Hi Dan,
>
> On Sat, Jul 10, 2021 at 11:54:26PM +0100, Daniel Scally wrote:
>> On 10/07/2021 23:28, Laurent Pinchart wrote:
>>> On Thu, Jul 08, 2021 at 11:42:24PM +0100, Daniel Scally wrote:
>>>> Hello all
>>>>
>>>> See previous series for some background context [1]
>>>>
>>>> Some x86 laptops with ACPI tables designed for Windows have a TPS68470
>>>> PMIC providing regulators and clocks to camera modules. The DSDT tables for
>>>> those cameras lack any power control methods, declaring only a
>>>> dependency on the ACPI device representing the TPS68470. This leaves the
>>>> regulator framework with no means of determining appropriate voltages for the
>>>> regulators provided by the PMIC, or of determining which regulators relate to
>>>> which of the sensor's requested supplies. 
>>>>
>>>> This series is a prototype of an emulation of the device tree regulator
>>>> initialisation and lookup functions, using software nodes. Software nodes
>>>> relating to each regulator are registered as children of the TPS68470's ACPI
>>>> firmware node. Those regulators have properties describing their constraints
>>>> (for example "regulator-min-microvolt"). Similarly, software nodes are
>>>> registered and assigned as secondary to the Camera's firmware node - these
>>>> software nodes have reference properties named after the supply in the same
>>>> way as device tree's phandles, for example "avdd-supply", and linking to the
>>>> software node assigned to the appropriate regulator. We can then use those
>>>> constraints to specify the appropriate voltages and the references to allow the
>>>> camera drivers to look up the correct regulator device. 
>>>>
>>>> Although not included in this series, I would plan to use a similar method for
>>>> linking the clocks provided by the TPS68470 to the sensor so that it can be
>>>> discovered too.
>>>>
>>>> I'm posting this to see if people agree it's a good approach for tackling the 
>>>> problem; I may be overthinking this and there's a much easier way that I should
>>>> be looking at instead. It will have knock-ons in the cio2-bridge code [2], as
>>>> that is adding software nodes to the same sensors to connect them to the media
>>>> graph. Similarly, is the board file an acceptable solution, or should we just
>>>> define the configuration for these devices (there's three orf our laptop models
>>>> in scope) in int3472-tps68470 instead?
>>> I may have missed something, but if you load the SGo2 board file, won't
>>> it create the regulator software nodes if it finds an INT3472,
>>> regardless of whether the device is an SGo2 ? If you happen to do so on
>>> a machine that requires different voltages, that sounds dangerous.
>> Ah, yes - hadn't thought of that. If a driver registered regulators with
>> those names, it would try to apply those voltages during registration.
>> Good point.
>>
>>> Given that INT3472 models the virtual "Intel Skylake and Kabylake camera
>>> PMIC", I think moving device-specific information to the int3472 driver
>>> may make sense. I'm unsure what option is best though, having all the
>>> data (regulators, clocks, but also data currently stored in the
>>> cio2-bridge driver) in a single file (or a single file per machine) is
>>> tempting.
>> It is tempting, particularly because (assuming we do end up using this
>> approach) setting the references to the supplies in a board file like
>> this complicated the cio2-bridge code quite a bit, since it then needs
>> to extend the properties array against an already-existing software node
>> rather than registering a new one. But then, I don't particularly want
>> to handle that aspect of the problem in two separate places.
> If technically feasible, gathering all the data in a single place would
> be my preference. Whether that should take the form of software nodes in
> all cases, or be modelled as custom data that the int3472 driver would
> interpret to create the regulators and clocks is a different (but
> related) question.


I'll have to think on that one then; the problem there is that the
cio2-bridge is just given ACPI HIDs for the sensors as "ok to parse
this", and of course the INT347A that is being dealt with here should
already be supported on most Surface platforms via the intel-skl-int3472
stuff, so once the ov8865 edits are (posted and) accepted and that
driver is supported my plan would be to add it into the bridge. So we'd
need a way to exclude Go2 from that if we wanted to define all the
software nodes parts in a single board file instead.

>
> The very important part is to ensure that the correct board data will be
> used, as otherwise we could damage the hardware.


Not sure how this is usually guarded against; we could do a DMI match at
the start of the init function to confirm it's running on a Go2 and exit
if not?

>
>>>> [1] https://lore.kernel.org/lkml/20210603224007.120560-1-djrscally@gmail.com/
>>>> [2] https://elixir.bootlin.com/linux/latest/source/drivers/media/pci/intel/ipu3/cio2-bridge.c#L166
>>>>
>>>>
>>>> Daniel Scally (2):
>>>>   regulator: Add support for software node connections
>>>>   platform/surface: Add Surface Go 2 board file
>>>>
>>>>  MAINTAINERS                                |   6 +
>>>>  drivers/platform/surface/Kconfig           |  10 ++
>>>>  drivers/platform/surface/Makefile          |   1 +
>>>>  drivers/platform/surface/surface_go_2.c    | 135 +++++++++++++++++++++
>>>>  drivers/regulator/Kconfig                  |   6 +
>>>>  drivers/regulator/Makefile                 |   1 +
>>>>  drivers/regulator/core.c                   |  23 ++++
>>>>  drivers/regulator/swnode_regulator.c       | 111 +++++++++++++++++
>>>>  include/linux/regulator/swnode_regulator.h |  33 +++++
>>>>  9 files changed, 326 insertions(+)
>>>>  create mode 100644 drivers/platform/surface/surface_go_2.c
>>>>  create mode 100644 drivers/regulator/swnode_regulator.c
>>>>  create mode 100644 include/linux/regulator/swnode_regulator.h

Re: [RFC PATCH 0/2] Add software node support to regulator framework

[Laurent Pinchart]

Hi Dan,

On Mon, Jul 12, 2021 at 09:13:00AM +0100, Daniel Scally wrote:
> On 11/07/2021 17:55, Laurent Pinchart wrote:
> > On Sat, Jul 10, 2021 at 11:54:26PM +0100, Daniel Scally wrote:
> >> On 10/07/2021 23:28, Laurent Pinchart wrote:
> >>> On Thu, Jul 08, 2021 at 11:42:24PM +0100, Daniel Scally wrote:
> >>>> Hello all
> >>>>
> >>>> See previous series for some background context [1]
> >>>>
> >>>> Some x86 laptops with ACPI tables designed for Windows have a TPS68470
> >>>> PMIC providing regulators and clocks to camera modules. The DSDT tables for
> >>>> those cameras lack any power control methods, declaring only a
> >>>> dependency on the ACPI device representing the TPS68470. This leaves the
> >>>> regulator framework with no means of determining appropriate voltages for the
> >>>> regulators provided by the PMIC, or of determining which regulators relate to
> >>>> which of the sensor's requested supplies. 
> >>>>
> >>>> This series is a prototype of an emulation of the device tree regulator
> >>>> initialisation and lookup functions, using software nodes. Software nodes
> >>>> relating to each regulator are registered as children of the TPS68470's ACPI
> >>>> firmware node. Those regulators have properties describing their constraints
> >>>> (for example "regulator-min-microvolt"). Similarly, software nodes are
> >>>> registered and assigned as secondary to the Camera's firmware node - these
> >>>> software nodes have reference properties named after the supply in the same
> >>>> way as device tree's phandles, for example "avdd-supply", and linking to the
> >>>> software node assigned to the appropriate regulator. We can then use those
> >>>> constraints to specify the appropriate voltages and the references to allow the
> >>>> camera drivers to look up the correct regulator device. 
> >>>>
> >>>> Although not included in this series, I would plan to use a similar method for
> >>>> linking the clocks provided by the TPS68470 to the sensor so that it can be
> >>>> discovered too.
> >>>>
> >>>> I'm posting this to see if people agree it's a good approach for tackling the 
> >>>> problem; I may be overthinking this and there's a much easier way that I should
> >>>> be looking at instead. It will have knock-ons in the cio2-bridge code [2], as
> >>>> that is adding software nodes to the same sensors to connect them to the media
> >>>> graph. Similarly, is the board file an acceptable solution, or should we just
> >>>> define the configuration for these devices (there's three orf our laptop models
> >>>> in scope) in int3472-tps68470 instead?
> >>>
> >>> I may have missed something, but if you load the SGo2 board file, won't
> >>> it create the regulator software nodes if it finds an INT3472,
> >>> regardless of whether the device is an SGo2 ? If you happen to do so on
> >>> a machine that requires different voltages, that sounds dangerous.
> >>
> >> Ah, yes - hadn't thought of that. If a driver registered regulators with
> >> those names, it would try to apply those voltages during registration.
> >> Good point.
> >>
> >>> Given that INT3472 models the virtual "Intel Skylake and Kabylake camera
> >>> PMIC", I think moving device-specific information to the int3472 driver
> >>> may make sense. I'm unsure what option is best though, having all the
> >>> data (regulators, clocks, but also data currently stored in the
> >>> cio2-bridge driver) in a single file (or a single file per machine) is
> >>> tempting.
> >>
> >> It is tempting, particularly because (assuming we do end up using this
> >> approach) setting the references to the supplies in a board file like
> >> this complicated the cio2-bridge code quite a bit, since it then needs
> >> to extend the properties array against an already-existing software node
> >> rather than registering a new one. But then, I don't particularly want
> >> to handle that aspect of the problem in two separate places.
> >
> > If technically feasible, gathering all the data in a single place would
> > be my preference. Whether that should take the form of software nodes in
> > all cases, or be modelled as custom data that the int3472 driver would
> > interpret to create the regulators and clocks is a different (but
> > related) question.
> 
> I'll have to think on that one then; the problem there is that the
> cio2-bridge is just given ACPI HIDs for the sensors as "ok to parse
> this", and of course the INT347A that is being dealt with here should
> already be supported on most Surface platforms via the intel-skl-int3472
> stuff, so once the ov8865 edits are (posted and) accepted and that
> driver is supported my plan would be to add it into the bridge. So we'd
> need a way to exclude Go2 from that if we wanted to define all the
> software nodes parts in a single board file instead.
> 
> > The very important part is to ensure that the correct board data will be
> > used, as otherwise we could damage the hardware.
> 
> Not sure how this is usually guarded against; we could do a DMI match at
> the start of the init function to confirm it's running on a Go2 and exit
> if not?

Unless the information is available in ACPI properties that the CIO2
bridge driver can access (and I wouldn't be surprised if it was the case
in some way, there's are different IDs that we're not sure how to use,
the Windows driver may very well map them to a set of reference
designs), then a DMI match is likely the best option.

> >>>> [1] https://lore.kernel.org/lkml/20210603224007.120560-1-djrscally@gmail.com/
> >>>> [2] https://elixir.bootlin.com/linux/latest/source/drivers/media/pci/intel/ipu3/cio2-bridge.c#L166
> >>>>
> >>>>
> >>>> Daniel Scally (2):
> >>>>   regulator: Add support for software node connections
> >>>>   platform/surface: Add Surface Go 2 board file
> >>>>
> >>>>  MAINTAINERS                                |   6 +
> >>>>  drivers/platform/surface/Kconfig           |  10 ++
> >>>>  drivers/platform/surface/Makefile          |   1 +
> >>>>  drivers/platform/surface/surface_go_2.c    | 135 +++++++++++++++++++++
> >>>>  drivers/regulator/Kconfig                  |   6 +
> >>>>  drivers/regulator/Makefile                 |   1 +
> >>>>  drivers/regulator/core.c                   |  23 ++++
> >>>>  drivers/regulator/swnode_regulator.c       | 111 +++++++++++++++++
> >>>>  include/linux/regulator/swnode_regulator.h |  33 +++++
> >>>>  9 files changed, 326 insertions(+)
> >>>>  create mode 100644 drivers/platform/surface/surface_go_2.c
> >>>>  create mode 100644 drivers/regulator/swnode_regulator.c
> >>>>  create mode 100644 include/linux/regulator/swnode_regulator.h

-- 
Regards,

Laurent Pinchart

Re: [RFC PATCH 0/2] Add software node support to regulator framework

[Mark Brown]

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

On Mon, Jul 12, 2021 at 09:13:00AM +0100, Daniel Scally wrote:
> On 11/07/2021 17:55, Laurent Pinchart wrote:

> > If technically feasible, gathering all the data in a single place would
> > be my preference. Whether that should take the form of software nodes in
> > all cases, or be modelled as custom data that the int3472 driver would
> > interpret to create the regulators and clocks is a different (but
> > related) question.

> I'll have to think on that one then; the problem there is that the
> cio2-bridge is just given ACPI HIDs for the sensors as "ok to parse
> this", and of course the INT347A that is being dealt with here should
> already be supported on most Surface platforms via the intel-skl-int3472
> stuff, so once the ov8865 edits are (posted and) accepted and that
> driver is supported my plan would be to add it into the bridge. So we'd
> need a way to exclude Go2 from that if we wanted to define all the
> software nodes parts in a single board file instead.

Why not just do what things like rt5670 do and have the driver probe
for the PMIC use DMI information to set up platform data?  That seems a
lot more straightforward.

So long as people keep building systems that don't fit the ACPI model
using ACPI, and indeed with no firmware description at all for important
bits of the system, it's just a question of which particular kind of
mess we end up with cleaning up after them.  These vendors really should
adopt a standards based approach rather than relying on these DMI hacks.

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