driverdev-devel.linuxdriverproject.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] media: ipu3: add a module to probe sensors via ACPI
@ 2020-05-16 10:43 Mauro Carvalho Chehab
  2020-05-17 10:36 ` Sakari Ailus
  0 siblings, 1 reply; 12+ messages in thread
From: Mauro Carvalho Chehab @ 2020-05-16 10:43 UTC (permalink / raw)
  To: Linux Media Mailing List
  Cc: devel, Mauro Carvalho Chehab, Greg Kroah-Hartman, Sakari Ailus,
	Bingbu Cao, Tian Shu Qiu

On devices without ACPI, or which ACPI is not prepared to
export sensor data via DT, we need a different probing
method.

This little driver adds initial support to probe the
sensors found on a Dell Latitude 7285.

For now, it just detects the hardware and use request_module()
to load a sensor driver.

In the specific case of this device, the ACPI DTST dable
describes 2 camera sensors for this module, but the
current upstream doesn't have yet drivers for such
sensors. So, this patch just detects the PMIC used on
this device and tries to load a sensor.

Once the sensor gets added, some additional code will
be needed to pass via platform_data other details, like
callbacks for PMIC's command to turn the sensor on/off
and other sensor-specific settings.

The idea of this patch was inspired on how the sensors
are probed by the staging atomisp driver.

The current result of this driver with the Dell
Latitude 7285 is:

	ipu3_acpi i2c-INT3477:00: ipu3_acpi_probe: ACPI detected it on bus ID=LNK1, HID=INT3477
	ipu3_acpi i2c-INT3477:00: Found DMI entry for 'Latitude 7285' with sensor INT3477
	ipu3_acpi i2c-INT3477:00: Loading sensor module ov8858
	ipu3_acpi i2c-OVTI9234:00: ipu3_acpi_probe: ACPI detected it on bus ID=LNK2, HID=OVTI9234
	ipu3_acpi i2c-OVTI9234:00: Found DMI entry for 'Latitude 7285' with sensor OVTI9234
	ipu3_acpi i2c-OVTI9234:00: Loading sensor module ov9234

Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
---
 drivers/staging/media/ipu3/Kconfig     |  16 ++
 drivers/staging/media/ipu3/Makefile    |   1 +
 drivers/staging/media/ipu3/ipu3-acpi.c | 241 +++++++++++++++++++++++++
 3 files changed, 258 insertions(+)
 create mode 100644 drivers/staging/media/ipu3/ipu3-acpi.c

diff --git a/drivers/staging/media/ipu3/Kconfig b/drivers/staging/media/ipu3/Kconfig
index 3e9640523e50..bede7910ea7b 100644
--- a/drivers/staging/media/ipu3/Kconfig
+++ b/drivers/staging/media/ipu3/Kconfig
@@ -14,3 +14,19 @@ config VIDEO_IPU3_IMGU
 
 	  Say Y or M here if you have a Skylake/Kaby Lake SoC with a MIPI
 	  camera. The module will be called ipu3-imgu.
+
+config VIDEO_IPU3_IMGU_ACPI
+	tristate "Probe sensors via ACPI"
+	depends on VIDEO_IPU3_IMGU
+	help
+	  The Intel ImgU device could be used on some Laptop-like
+	  hardware, like Dell Latitude 7285.
+
+	  On such devices, the sensors are defined via ACPI tables,
+	  and won't use Device Tree Open Firmware support.
+
+	  So, a different logic is needed in order to load the right
+	  camera sensors.
+
+	  Say Y or M here if you have a Laptop/Tablet device like
+	  Dell Latitude 7285.
diff --git a/drivers/staging/media/ipu3/Makefile b/drivers/staging/media/ipu3/Makefile
index 9def80ef28f3..3d0da2654376 100644
--- a/drivers/staging/media/ipu3/Makefile
+++ b/drivers/staging/media/ipu3/Makefile
@@ -10,3 +10,4 @@ ipu3-imgu-objs += \
 		ipu3-css.o ipu3-v4l2.o ipu3.o
 
 obj-$(CONFIG_VIDEO_IPU3_IMGU) += ipu3-imgu.o
+obj-$(CONFIG_VIDEO_IPU3_IMGU_ACPI) += ipu3-acpi.o
diff --git a/drivers/staging/media/ipu3/ipu3-acpi.c b/drivers/staging/media/ipu3/ipu3-acpi.c
new file mode 100644
index 000000000000..4653188ba4f3
--- /dev/null
+++ b/drivers/staging/media/ipu3/ipu3-acpi.c
@@ -0,0 +1,241 @@
+// SPDX-License-Identifier: GPL-2.0
+//
+// ipu3_acpi - Detects IPU3 camera sensors via ACPI
+//
+// Copyright (c) 2020 Mauro Carvalho Chehab <mchehab_huawei@kernel.org>
+
+#include <linux/acpi.h>
+#include <linux/device.h>
+#include <linux/dmi.h>
+#include <linux/i2c.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/pci.h>
+
+#define IMGU_PCI_ID                   0x1919
+
+#define PMIC_ACPI_TPS68470	"INT3472:06"
+
+#define CFG_VAR_NAME_MAX 64	/* Max name for a DMI (or EFI) var */
+
+
+/*
+ * Ancillary routines to work with PMIC
+ */
+
+static int ipu3_acpi_match_one(struct device *dev, const void *data)
+{
+	const char *name = data;
+	struct i2c_client *client;
+
+	if (dev->type != &i2c_client_type)
+		return 0;
+
+	client = to_i2c_client(dev);
+
+	return (!strcmp(name, client->name));
+}
+
+static struct i2c_client *ipu3_acpi_dev_exists(struct device *dev, char *name,
+					      struct i2c_client **client)
+{
+	struct device *d;
+
+	while ((d = bus_find_device(&i2c_bus_type, NULL, name,
+				    ipu3_acpi_match_one))) {
+		*client = to_i2c_client(d);
+		dev_dbg(dev, "found '%s' at address 0x%02x, adapter %d\n",
+			(*client)->name, (*client)->addr,
+			(*client)->adapter->nr);
+		return *client;
+	}
+
+	return NULL;
+}
+
+/*
+ * Get vars from ACPI DTST table
+ */
+
+static const struct dmi_system_id dmi_sensors_table[] = {
+	{
+		.ident = "Latitude 7285",
+		.matches = {
+			DMI_MATCH(DMI_PRODUCT_NAME, "Latitude 7285"),
+		},
+		// .driver_data = dell_latitude_7285_quirks,
+	},
+	{ 0 }
+};
+
+/*
+ * Convert from ACPI name into Linux module name
+ */
+
+struct acpi_linux_modules {
+	const char *acpi_name;
+	const char *linux_module;
+};
+
+/* FIXME:
+ *
+ * for now, list only the devices on the models we know, and
+ * the obvious ones. We need to map later the other sensors
+ */
+struct acpi_linux_modules modules[] = {
+	{"INT3471",	"imx135"},
+	{"INT33BE",	NULL},
+	{"INT3476",	NULL},
+	{"INT3477",	"ov8858"},
+	{"INT3474",	"ov2740"},
+	{"INT3473",	NULL},
+	{"INT3475",	NULL},
+	{"INT3478",	NULL},
+	{"INT3479",	NULL},
+	{"INT347A",	NULL},
+	{"INT347B",	NULL},
+	{"OVTI9234",	"ov9234"},
+	{"OVTI9734",	"ov9734"},
+	{"OVTI8856",	"ov8856"},
+	{"OVTIF860",	NULL},
+	{ 0 }
+};
+
+/*
+ * Scan for sensor platform data information and modprobe it
+ */
+static int ipu3_acpi_probe_sensor(struct acpi_device *adev,
+				  struct i2c_client *client)
+{
+	const char *sensor_name, *module_name = NULL;
+	struct device *dev = &client->dev;
+	struct i2c_client *power = NULL;
+	const struct dmi_system_id *id;
+	int ret, i;
+
+	/* Currently, the only PMIC supported is tps68470 */
+	if (!ipu3_acpi_dev_exists(dev, PMIC_ACPI_TPS68470, &power)) {
+		dev_err(dev, "Doesn't know how to turn the sensor on/off\n");
+		return -ENODEV;
+	}
+
+	id = dmi_first_match(dmi_sensors_table);
+	if (!id) {
+		dev_err(dev, "Didn't find device's product ID\n");
+		return -ENODEV;
+	}
+
+	sensor_name = acpi_device_hid(adev);
+
+	dev_info(dev, "Found DMI entry for '%s' with sensor %s\n",
+		 id->ident, sensor_name);
+
+	for (i =0; i < ARRAY_SIZE(modules); i++) {
+		if (!strcmp(sensor_name, modules[i].acpi_name)) {
+			module_name = modules[i].linux_module;
+			break;
+		}
+	}
+
+	if (!module_name) {
+		dev_err(dev, "Sensor currently not supported\n");
+		return -ENODEV;
+	}
+
+	/*
+	 * FIXME: we need to setup platform_data, using some hard-coded
+	 * logic and/or EFI and DTST table info, if available.
+	 * It should likely use power->addr somehow, as the sensor code
+	 * need to know how to power on/off the sensor.
+	 */
+	dev_info(dev, "Loading sensor module %s\n", module_name);
+	ret = request_module("%s", module_name);
+	if (ret < 0) {
+		dev_err(dev, "Couldn't load sensor module %s\n",
+			module_name);
+		return ret;
+	}
+
+	return 0;
+}
+
+/*
+ * Driver's probe/remove code
+ */
+
+static int ipu3_acpi_remove(struct i2c_client *client)
+{
+	return 0;
+}
+
+static int ipu3_acpi_probe(struct i2c_client *client)
+{
+	struct acpi_device *adev;
+	struct pci_dev *pdev;
+	acpi_handle handle;
+	int ret;
+
+	/*
+	 * As other drivers may try to bind the same ACPI sensor codes,
+	 * let's ignore them, if they don't have the IPU PCI device id.
+	 */
+	pdev = pci_get_device(PCI_VENDOR_ID_INTEL, IMGU_PCI_ID, NULL);
+	if (!pdev)
+		return -ENODEV;
+
+	handle = ACPI_HANDLE(&client->dev);
+	if (!handle || acpi_bus_get_device(handle, &adev)) {
+		dev_err(&client->dev, "Error could not get ACPI device\n");
+		return -ENODEV;
+	}
+	dev_info(&client->dev, "%s: ACPI detected it on bus ID=%s, HID=%s\n",
+		__func__, acpi_device_bid(adev), acpi_device_hid(adev));
+
+	ret = ipu3_acpi_probe_sensor(adev, client);
+
+// FIXME: do we need something to avoid memory leaks, like:
+//	acpi_dev_put(adev);
+	pci_dev_put(pdev);
+
+	return ret;
+}
+
+/*
+ * Should list known sensor devices found at DSDT table as "CAM0", "CAM1", ...
+ *
+ * The table below is probably incomplete. It came from the DSDT table found
+ * at a Dell Latitude 7285 (Method HCID).
+ */
+static const struct acpi_device_id ipu3_acpi_acpi_match[] = {
+	{"INT3471"},
+	{"INT33BE"},
+	{"INT3476"},
+	{"INT3477"},
+	{"INT3474"},
+	{"INT3473"},
+	{"INT3475"},
+	{"INT3478"},
+	{"INT3479"},
+	{"INT347A"},
+	{"INT347B"},
+	{"OVTI9234"},
+	{"OVTI9734"},
+	{"OVTI8856"},
+	{"OVTIF860"},
+	{},
+};
+MODULE_DEVICE_TABLE(acpi, ipu3_acpi_acpi_match);
+
+static struct i2c_driver ipu3_acpi_driver = {
+	.driver = {
+		.name = "ipu3_acpi",
+		.acpi_match_table = ipu3_acpi_acpi_match,
+	},
+	.probe_new = ipu3_acpi_probe,
+	.remove = ipu3_acpi_remove,
+};
+module_i2c_driver(ipu3_acpi_driver);
+
+MODULE_AUTHOR("Mauro Carvalho Chehab <mchehab_huawei@kernel.org>");
+MODULE_DESCRIPTION("Detects camera sensors used by IPU3 driver");
+MODULE_LICENSE("GPL");
-- 
2.26.2

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH] media: ipu3: add a module to probe sensors via ACPI
  2020-05-16 10:43 [PATCH] media: ipu3: add a module to probe sensors via ACPI Mauro Carvalho Chehab
@ 2020-05-17 10:36 ` Sakari Ailus
  2020-05-20  7:44   ` Mauro Carvalho Chehab
  0 siblings, 1 reply; 12+ messages in thread
From: Sakari Ailus @ 2020-05-17 10:36 UTC (permalink / raw)
  To: Mauro Carvalho Chehab
  Cc: devel, Bingbu Cao, Greg Kroah-Hartman, Tian Shu Qiu,
	Linux Media Mailing List

Hi Mauro,

Thanks for the patch.

On Sat, May 16, 2020 at 12:43:39PM +0200, Mauro Carvalho Chehab wrote:
> On devices without ACPI, or which ACPI is not prepared to
> export sensor data via DT, we need a different probing
> method.
> 
> This little driver adds initial support to probe the
> sensors found on a Dell Latitude 7285.
> 
> For now, it just detects the hardware and use request_module()
> to load a sensor driver.
> 
> In the specific case of this device, the ACPI DTST dable
> describes 2 camera sensors for this module, but the
> current upstream doesn't have yet drivers for such
> sensors. So, this patch just detects the PMIC used on
> this device and tries to load a sensor.
> 
> Once the sensor gets added, some additional code will
> be needed to pass via platform_data other details, like
> callbacks for PMIC's command to turn the sensor on/off
> and other sensor-specific settings.
> 
> The idea of this patch was inspired on how the sensors
> are probed by the staging atomisp driver.
> 
> The current result of this driver with the Dell
> Latitude 7285 is:
> 
> 	ipu3_acpi i2c-INT3477:00: ipu3_acpi_probe: ACPI detected it on bus ID=LNK1, HID=INT3477
> 	ipu3_acpi i2c-INT3477:00: Found DMI entry for 'Latitude 7285' with sensor INT3477
> 	ipu3_acpi i2c-INT3477:00: Loading sensor module ov8858
> 	ipu3_acpi i2c-OVTI9234:00: ipu3_acpi_probe: ACPI detected it on bus ID=LNK2, HID=OVTI9234
> 	ipu3_acpi i2c-OVTI9234:00: Found DMI entry for 'Latitude 7285' with sensor OVTI9234
> 	ipu3_acpi i2c-OVTI9234:00: Loading sensor module ov9234
> 
> Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>

...

> +/*
> + * Should list known sensor devices found at DSDT table as "CAM0", "CAM1", ...
> + *
> + * The table below is probably incomplete. It came from the DSDT table found
> + * at a Dell Latitude 7285 (Method HCID).
> + */
> +static const struct acpi_device_id ipu3_acpi_acpi_match[] = {
> +	{"INT3471"},
> +	{"INT33BE"},
> +	{"INT3476"},
> +	{"INT3477"},
> +	{"INT3474"},
> +	{"INT3473"},
> +	{"INT3475"},
> +	{"INT3478"},
> +	{"INT3479"},
> +	{"INT347A"},
> +	{"INT347B"},
> +	{"OVTI9234"},
> +	{"OVTI9734"},
> +	{"OVTI8856"},
> +	{"OVTIF860"},
> +	{},
> +};
> +MODULE_DEVICE_TABLE(acpi, ipu3_acpi_acpi_match);

Instead of creating a new way to probe drivers on ACPI systems, please add
the appropriate ACPI device IDs to the respective drivers. E.g.
drivers/media/i2c/imx319.c implements this.

-- 
Kind regards,

Sakari Ailus
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] media: ipu3: add a module to probe sensors via ACPI
  2020-05-17 10:36 ` Sakari Ailus
@ 2020-05-20  7:44   ` Mauro Carvalho Chehab
  2020-05-20  8:26     ` Sakari Ailus
  0 siblings, 1 reply; 12+ messages in thread
From: Mauro Carvalho Chehab @ 2020-05-20  7:44 UTC (permalink / raw)
  To: Sakari Ailus
  Cc: devel, Bingbu Cao, Greg Kroah-Hartman, Tian Shu Qiu,
	Linux Media Mailing List

Hi Sakari,

Em Sun, 17 May 2020 13:36:59 +0300
Sakari Ailus <sakari.ailus@linux.intel.com> escreveu:

> Hi Mauro,
> 
> Thanks for the patch.

Thanks for reviewing it.

> 
> On Sat, May 16, 2020 at 12:43:39PM +0200, Mauro Carvalho Chehab wrote:
> > On devices without ACPI, or which ACPI is not prepared to
> > export sensor data via DT, we need a different probing
> > method.
> > 
> > This little driver adds initial support to probe the
> > sensors found on a Dell Latitude 7285.
> > 
> > For now, it just detects the hardware and use request_module()
> > to load a sensor driver.
> > 
> > In the specific case of this device, the ACPI DTST dable
> > describes 2 camera sensors for this module, but the
> > current upstream doesn't have yet drivers for such
> > sensors. So, this patch just detects the PMIC used on
> > this device and tries to load a sensor.
> > 
> > Once the sensor gets added, some additional code will
> > be needed to pass via platform_data other details, like
> > callbacks for PMIC's command to turn the sensor on/off
> > and other sensor-specific settings.
> > 
> > The idea of this patch was inspired on how the sensors
> > are probed by the staging atomisp driver.
> > 
> > The current result of this driver with the Dell
> > Latitude 7285 is:
> > 
> > 	ipu3_acpi i2c-INT3477:00: ipu3_acpi_probe: ACPI detected it on bus ID=LNK1, HID=INT3477
> > 	ipu3_acpi i2c-INT3477:00: Found DMI entry for 'Latitude 7285' with sensor INT3477
> > 	ipu3_acpi i2c-INT3477:00: Loading sensor module ov8858
> > 	ipu3_acpi i2c-OVTI9234:00: ipu3_acpi_probe: ACPI detected it on bus ID=LNK2, HID=OVTI9234
> > 	ipu3_acpi i2c-OVTI9234:00: Found DMI entry for 'Latitude 7285' with sensor OVTI9234
> > 	ipu3_acpi i2c-OVTI9234:00: Loading sensor module ov9234
> > 
> > Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>  

With regards to the approach this patch took: it is currently
incomplete: the I2C core doesn't currently allow to have two 
drivers for the same I2C address at the same bus. So, if we end by having
some ancillary drivers to help the I2C core to work with media devs,
we may need some changes at I2C core (or to use an I2C virtual mux).

> ...
> 
> > +/*
> > + * Should list known sensor devices found at DSDT table as "CAM0", "CAM1", ...
> > + *
> > + * The table below is probably incomplete. It came from the DSDT table found
> > + * at a Dell Latitude 7285 (Method HCID).
> > + */
> > +static const struct acpi_device_id ipu3_acpi_acpi_match[] = {
> > +	{"INT3471"},
> > +	{"INT33BE"},
> > +	{"INT3476"},
> > +	{"INT3477"},
> > +	{"INT3474"},
> > +	{"INT3473"},
> > +	{"INT3475"},
> > +	{"INT3478"},
> > +	{"INT3479"},
> > +	{"INT347A"},
> > +	{"INT347B"},
> > +	{"OVTI9234"},
> > +	{"OVTI9734"},
> > +	{"OVTI8856"},
> > +	{"OVTIF860"},
> > +	{},
> > +};
> > +MODULE_DEVICE_TABLE(acpi, ipu3_acpi_acpi_match);  
> 
> Instead of creating a new way to probe drivers on ACPI systems, please add
> the appropriate ACPI device IDs to the respective drivers. E.g.
> drivers/media/i2c/imx319.c implements this.

The ACPI code at imx319 is incomplete. I mean, it will only tell the I2C
core that the driver should be probed via ACPI, but it tells nothing
how to power up the device. It just assumes that the driver will work
using pm_runtime support.

It also doesn't tell how to get device-specific platform data from
the BIOS (with is machine-specific).

Also, at least in the case of the atomisp approach, a single code
to parse BIOS for devices with ISP2300/ISP2400/ISP2401/ISP2500 should
work with all sensors supported by those models.

Copying those inside all sensor drivers is probably a bad idea.
I mean, we should likely need a core support for parsing it, as
the code is the same for a given set of PCI IDs.

-

As the atomisp driver is now minimally working, my plan is to merge
it upstream, under staging.

Before going ahead and fixing other troubles there at atomisp,
I may try to port the needed ACPI bits from the atomisp-ov2880 
staging driver into the mainline one. This should be an interesting 
exercise to check what's missing there.

Even if the atomisp never gets out of staging, doing that will help
to identify what it would take for a sensor to be able to work with
more than one different ISP. As as result, we may design something
that will properly support ACPI at the media subsystem.

Thanks,
Mauro
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] media: ipu3: add a module to probe sensors via ACPI
  2020-05-20  7:44   ` Mauro Carvalho Chehab
@ 2020-05-20  8:26     ` Sakari Ailus
  2020-05-20 11:18       ` Mauro Carvalho Chehab
  0 siblings, 1 reply; 12+ messages in thread
From: Sakari Ailus @ 2020-05-20  8:26 UTC (permalink / raw)
  To: Mauro Carvalho Chehab
  Cc: devel, Bingbu Cao, Greg Kroah-Hartman, Tian Shu Qiu,
	Linux Media Mailing List

Hi Mauro,

On Wed, May 20, 2020 at 09:44:00AM +0200, Mauro Carvalho Chehab wrote:
> Hi Sakari,
> 
> Em Sun, 17 May 2020 13:36:59 +0300
> Sakari Ailus <sakari.ailus@linux.intel.com> escreveu:
> 
> > Hi Mauro,
> > 
> > Thanks for the patch.
> 
> Thanks for reviewing it.
> 
> > 
> > On Sat, May 16, 2020 at 12:43:39PM +0200, Mauro Carvalho Chehab wrote:
> > > On devices without ACPI, or which ACPI is not prepared to
> > > export sensor data via DT, we need a different probing
> > > method.
> > > 
> > > This little driver adds initial support to probe the
> > > sensors found on a Dell Latitude 7285.
> > > 
> > > For now, it just detects the hardware and use request_module()
> > > to load a sensor driver.
> > > 
> > > In the specific case of this device, the ACPI DTST dable
> > > describes 2 camera sensors for this module, but the
> > > current upstream doesn't have yet drivers for such
> > > sensors. So, this patch just detects the PMIC used on
> > > this device and tries to load a sensor.
> > > 
> > > Once the sensor gets added, some additional code will
> > > be needed to pass via platform_data other details, like
> > > callbacks for PMIC's command to turn the sensor on/off
> > > and other sensor-specific settings.
> > > 
> > > The idea of this patch was inspired on how the sensors
> > > are probed by the staging atomisp driver.
> > > 
> > > The current result of this driver with the Dell
> > > Latitude 7285 is:
> > > 
> > > 	ipu3_acpi i2c-INT3477:00: ipu3_acpi_probe: ACPI detected it on bus ID=LNK1, HID=INT3477
> > > 	ipu3_acpi i2c-INT3477:00: Found DMI entry for 'Latitude 7285' with sensor INT3477
> > > 	ipu3_acpi i2c-INT3477:00: Loading sensor module ov8858
> > > 	ipu3_acpi i2c-OVTI9234:00: ipu3_acpi_probe: ACPI detected it on bus ID=LNK2, HID=OVTI9234
> > > 	ipu3_acpi i2c-OVTI9234:00: Found DMI entry for 'Latitude 7285' with sensor OVTI9234
> > > 	ipu3_acpi i2c-OVTI9234:00: Loading sensor module ov9234
> > > 
> > > Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>  
> 
> With regards to the approach this patch took: it is currently
> incomplete: the I2C core doesn't currently allow to have two 
> drivers for the same I2C address at the same bus. So, if we end by having
> some ancillary drivers to help the I2C core to work with media devs,
> we may need some changes at I2C core (or to use an I2C virtual mux).

Are the two really on the same bus with the same address? Both sensors
support address selection in hardware...

> 
> > ...
> > 
> > > +/*
> > > + * Should list known sensor devices found at DSDT table as "CAM0", "CAM1", ...
> > > + *
> > > + * The table below is probably incomplete. It came from the DSDT table found
> > > + * at a Dell Latitude 7285 (Method HCID).
> > > + */
> > > +static const struct acpi_device_id ipu3_acpi_acpi_match[] = {
> > > +	{"INT3471"},
> > > +	{"INT33BE"},
> > > +	{"INT3476"},
> > > +	{"INT3477"},
> > > +	{"INT3474"},
> > > +	{"INT3473"},
> > > +	{"INT3475"},
> > > +	{"INT3478"},
> > > +	{"INT3479"},
> > > +	{"INT347A"},
> > > +	{"INT347B"},
> > > +	{"OVTI9234"},
> > > +	{"OVTI9734"},
> > > +	{"OVTI8856"},
> > > +	{"OVTIF860"},
> > > +	{},
> > > +};
> > > +MODULE_DEVICE_TABLE(acpi, ipu3_acpi_acpi_match);  
> > 
> > Instead of creating a new way to probe drivers on ACPI systems, please add
> > the appropriate ACPI device IDs to the respective drivers. E.g.
> > drivers/media/i2c/imx319.c implements this.
> 
> The ACPI code at imx319 is incomplete. I mean, it will only tell the I2C
> core that the driver should be probed via ACPI, but it tells nothing
> how to power up the device. It just assumes that the driver will work
> using pm_runtime support.

The driver is complete; this is how it is supposed to work with ACPI.

Also note that there are systems where this works at the moment, using the
the above ACPI HIDs. Those must not be broken.

> 
> It also doesn't tell how to get device-specific platform data from
> the BIOS (with is machine-specific).

In some systems that is the case, yes. It means system specific drivers or
fixups at least to some extent.

> 
> Also, at least in the case of the atomisp approach, a single code
> to parse BIOS for devices with ISP2300/ISP2400/ISP2401/ISP2500 should
> work with all sensors supported by those models.
> 
> Copying those inside all sensor drivers is probably a bad idea.
> I mean, we should likely need a core support for parsing it, as
> the code is the same for a given set of PCI IDs.

Agreed. The more we can keep that away from the sensor drivers, the better.

> 
> -
> 
> As the atomisp driver is now minimally working, my plan is to merge
> it upstream, under staging.

How is it "minimally working" for you?

> 
> Before going ahead and fixing other troubles there at atomisp,
> I may try to port the needed ACPI bits from the atomisp-ov2880 
> staging driver into the mainline one. This should be an interesting 
> exercise to check what's missing there.
> 
> Even if the atomisp never gets out of staging, doing that will help
> to identify what it would take for a sensor to be able to work with
> more than one different ISP. As as result, we may design something
> that will properly support ACPI at the media subsystem.

Hmm. Generally ACPI based devices are supported, there are no issues as
such there. The ACPI tables in some systems, though, are a problem.

-- 
Regards,

Sakari Ailus
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] media: ipu3: add a module to probe sensors via ACPI
  2020-05-20  8:26     ` Sakari Ailus
@ 2020-05-20 11:18       ` Mauro Carvalho Chehab
  2020-05-21  8:00         ` Andy Shevchenko
  0 siblings, 1 reply; 12+ messages in thread
From: Mauro Carvalho Chehab @ 2020-05-20 11:18 UTC (permalink / raw)
  To: Sakari Ailus
  Cc: devel, Bingbu Cao, Greg Kroah-Hartman, Tian Shu Qiu,
	Linux Media Mailing List

Em Wed, 20 May 2020 11:26:08 +0300
Sakari Ailus <sakari.ailus@linux.intel.com> escreveu:

> Hi Mauro,
> 
> On Wed, May 20, 2020 at 09:44:00AM +0200, Mauro Carvalho Chehab wrote:
> > Hi Sakari,
> > 
> > Em Sun, 17 May 2020 13:36:59 +0300
> > Sakari Ailus <sakari.ailus@linux.intel.com> escreveu:
> >   
> > > Hi Mauro,
> > > 
> > > Thanks for the patch.  
> > 
> > Thanks for reviewing it.
> >   
> > > 
> > > On Sat, May 16, 2020 at 12:43:39PM +0200, Mauro Carvalho Chehab wrote:  
> > > > On devices without ACPI, or which ACPI is not prepared to
> > > > export sensor data via DT, we need a different probing
> > > > method.
> > > > 
> > > > This little driver adds initial support to probe the
> > > > sensors found on a Dell Latitude 7285.
> > > > 
> > > > For now, it just detects the hardware and use request_module()
> > > > to load a sensor driver.
> > > > 
> > > > In the specific case of this device, the ACPI DTST dable
> > > > describes 2 camera sensors for this module, but the
> > > > current upstream doesn't have yet drivers for such
> > > > sensors. So, this patch just detects the PMIC used on
> > > > this device and tries to load a sensor.
> > > > 
> > > > Once the sensor gets added, some additional code will
> > > > be needed to pass via platform_data other details, like
> > > > callbacks for PMIC's command to turn the sensor on/off
> > > > and other sensor-specific settings.
> > > > 
> > > > The idea of this patch was inspired on how the sensors
> > > > are probed by the staging atomisp driver.
> > > > 
> > > > The current result of this driver with the Dell
> > > > Latitude 7285 is:
> > > > 
> > > > 	ipu3_acpi i2c-INT3477:00: ipu3_acpi_probe: ACPI detected it on bus ID=LNK1, HID=INT3477
> > > > 	ipu3_acpi i2c-INT3477:00: Found DMI entry for 'Latitude 7285' with sensor INT3477
> > > > 	ipu3_acpi i2c-INT3477:00: Loading sensor module ov8858
> > > > 	ipu3_acpi i2c-OVTI9234:00: ipu3_acpi_probe: ACPI detected it on bus ID=LNK2, HID=OVTI9234
> > > > 	ipu3_acpi i2c-OVTI9234:00: Found DMI entry for 'Latitude 7285' with sensor OVTI9234
> > > > 	ipu3_acpi i2c-OVTI9234:00: Loading sensor module ov9234
> > > > 
> > > > Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>    
> > 
> > With regards to the approach this patch took: it is currently
> > incomplete: the I2C core doesn't currently allow to have two 
> > drivers for the same I2C address at the same bus. So, if we end by having
> > some ancillary drivers to help the I2C core to work with media devs,
> > we may need some changes at I2C core (or to use an I2C virtual mux).  
> 
> Are the two really on the same bus with the same address? Both sensors
> support address selection in hardware...

What I tried to do is to have an ACPI-probed driver loading a "normal" 
non-ACPI driver.

> > > > +/*
> > > > + * Should list known sensor devices found at DSDT table as "CAM0", "CAM1", ...
> > > > + *
> > > > + * The table below is probably incomplete. It came from the DSDT table found
> > > > + * at a Dell Latitude 7285 (Method HCID).
> > > > + */
> > > > +static const struct acpi_device_id ipu3_acpi_acpi_match[] = {
> > > > +	{"INT3471"},
> > > > +	{"INT33BE"},
> > > > +	{"INT3476"},
> > > > +	{"INT3477"},
> > > > +	{"INT3474"},
> > > > +	{"INT3473"},
> > > > +	{"INT3475"},
> > > > +	{"INT3478"},
> > > > +	{"INT3479"},
> > > > +	{"INT347A"},
> > > > +	{"INT347B"},
> > > > +	{"OVTI9234"},
> > > > +	{"OVTI9734"},
> > > > +	{"OVTI8856"},
> > > > +	{"OVTIF860"},
> > > > +	{},
> > > > +};
> > > > +MODULE_DEVICE_TABLE(acpi, ipu3_acpi_acpi_match);    
> > > 
> > > Instead of creating a new way to probe drivers on ACPI systems, please add
> > > the appropriate ACPI device IDs to the respective drivers. E.g.
> > > drivers/media/i2c/imx319.c implements this.  
> > 
> > The ACPI code at imx319 is incomplete. I mean, it will only tell the I2C
> > core that the driver should be probed via ACPI, but it tells nothing
> > how to power up the device. It just assumes that the driver will work
> > using pm_runtime support.  
> 
> The driver is complete; this is how it is supposed to work with ACPI.
> 
> Also note that there are systems where this works at the moment, using the
> the above ACPI HIDs. Those must not be broken.

If Atomisp driver would be using the sensor, what's there won't make
it work.

For example, this is the sensor driver for the device I'm using here to
test the atomisp driver:

	https://git.linuxtv.org/media_tree.git/tree/drivers/staging/media/atomisp/i2c/atomisp-ov2680.c

It does contain an ACPI table at the end:

	static const struct acpi_device_id ov2680_acpi_match[] = {
		{"XXOV2680"},
		{"OVTI2680"},
		{},
	};
	MODULE_DEVICE_TABLE(acpi, ov2680_acpi_match);

Which causes the driver to be properly probed via ACPI.

However, this sensor driver (and all other sensor drivers meant to work
with ISP2xxx chipsets) require this ancillary code:

	https://git.linuxtv.org/media_tree.git/tree/drivers/staging/media/atomisp/pci/atomisp_gmin_platform.c

Which parses the BIOS shipped on such devices.

The code there needs to identify if one of those I2C PMIC drivers
were also loaded:

	#define PMIC_ACPI_AXP		"INT33F4:00"	/* XPower AXP288 PMIC */
	#define PMIC_ACPI_TI		"INT33F5:00"	/* Dollar Cove TI PMIC */
	#define PMIC_ACPI_CRYSTALCOVE	"INT33FD:00"	/* Crystal Cove PMIC */

As those 3 different PMICs may control the sensor power up/down.
Some devices may also use a regulator driver. The ancillary routines
have some logic to detect the PMIC type (with can very from different
versions of the Atom CPU).

The driver also need to get sensor-specific platform data.

For the sensor I have, it need those:

	{"OVTI2680:00_CsiPort", "1"},
	{"OVTI2680:00_CsiLanes", "1"},
	{"OVTI2680:00_CsiFmt", "15"},
	{"OVTI2680:00_CsiBayer", "0"},
	{"OVTI2680:00_CamClk", "1"},

From some tests and from the comments at the atomisp driver, there are
3 ways used by BIOS developers to store those information:

1) via this EFI variable:

	#define GMIN_CFG_VAR_EFI_GUID EFI_GUID(0xecb54cd9, 0xe5ae, 0x4fdc, \
					        0xa9, 0x71, 0xe8, 0x77,     \
					        0x75, 0x60, 0x68, 0xf7)

2) by reading _DSM from the camera information:

        Device (CAM1)
        {
            Name (_ADR, Zero)  // _ADR: Address
            Name (_HID, "OVTI2680")  // _HID: Hardware ID
	...
            Name (C1CD, Buffer (0x0220){})
            Method (_DSM, 4, NotSerialized)  // _DSM: Device-Specific Method
            {
                If ((Arg0 == ToUUID ("dc2f6c4f-045b-4f1d-97b9-882a6860a4be")))
                {
                    Local0 = Package (0x12)
                        {
                            "CamId", 
                            "ov2680", 
                            "CamType", 
                            "1", 
                            "CsiPort", 
                            "0", 
                            "CsiLanes", 
                            "1", 
                            "CsiFmt", 
                            "15", 
                            "CsiBayer", 
                            "0", 
                            "CamClk", 
                            "1", 
                            "Regulator1p8v", 
                            "0", 
                            "Regulator2p8v", 
                            "0"
                        }
                    Return (Local0)
                }
	...

   (The current version of atomisp driver doesn't parse it yet)

3) Still, some devices don't have neither of them, so the driver needs
   to have their values hardcoded.

It sounds to me that (2) is actually an evolution of (1). So, older BIOS
would use GMIN_CFG_VAR_EFI_GUID way, while newer would use the _DSM
way, and others may just hardcode it inside the driver or at the
Windows .INF files.

So we need some code to parse device-specific ACPI stuff.

As I mentioned above, in the case of ISP2xxx, a single parser is used
for all atomisp-based devices.

So, except if all BIOS manufacturers started to provide an unified
reliable way to store camera data for IPU3 and newer versions
(with I seriously doubt), we'll need to add a parser for those
ACPI-specific things.

Such code could be added directly at the sensor drivers, but I
suspect that this will generate a lot of code duplication, making
it very painful to maintain.

So, IMHO, we should work on some code that would be parsing it
outside the sensor driver itself.

> 
> > 
> > It also doesn't tell how to get device-specific platform data from
> > the BIOS (with is machine-specific).  
> 
> In some systems that is the case, yes. It means system specific drivers or
> fixups at least to some extent.
> 
> > 
> > Also, at least in the case of the atomisp approach, a single code
> > to parse BIOS for devices with ISP2300/ISP2400/ISP2401/ISP2500 should
> > work with all sensors supported by those models.
> > 
> > Copying those inside all sensor drivers is probably a bad idea.
> > I mean, we should likely need a core support for parsing it, as
> > the code is the same for a given set of PCI IDs.  
> 
> Agreed. The more we can keep that away from the sensor drivers, the better.
> 
> > 
> > -
> > 
> > As the atomisp driver is now minimally working, my plan is to merge
> > it upstream, under staging.  
> 
> How is it "minimally working" for you?

- Atomisp driver probes fine and detects its associated hardware;
- Atomisp firmware code loads and runs properly (as far as we were
  able to test it);
- Sensor and ISP are properly powered up;
- v4l2-ctl and qv4l2 can read from device's controls;
- If set to the sensor resolution and format, streaming causes
  the driver code to receive IRQ as frame buffers arrive;

There are still lots of issues:

- the memory allocation code there is very suspicious, and
  it cause troubles, depending on what userspace does
  (for example, using the scaler);

- right now, VIDIOC_DQBUF doesn't return anything (using
  a very simple app with the absolute minimum set of ioctls
  needed to start streaming).
  Yet, the driver receives IRQs notifying that new frames
  arrived. So, from the chipset PoV, streaming is working.
  I suspect that the current driver is waiting for userspace
  to receive V4L2 events before releasing buffers via DQBUF.

> > Before going ahead and fixing other troubles there at atomisp,
> > I may try to port the needed ACPI bits from the atomisp-ov2880 
> > staging driver into the mainline one. This should be an interesting 
> > exercise to check what's missing there.
> > 
> > Even if the atomisp never gets out of staging, doing that will help
> > to identify what it would take for a sensor to be able to work with
> > more than one different ISP. As as result, we may design something
> > that will properly support ACPI at the media subsystem.  
> 
> Hmm. Generally ACPI based devices are supported, there are no issues as
> such there. The ACPI tables in some systems, though, are a problem.

As I said, the problem is not probing the sensor via ACPI, but, instead,
to be able receive platform-specific data.

Thanks,
Mauro
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] media: ipu3: add a module to probe sensors via ACPI
  2020-05-20 11:18       ` Mauro Carvalho Chehab
@ 2020-05-21  8:00         ` Andy Shevchenko
  2020-05-22  9:57           ` Mauro Carvalho Chehab
  2020-05-25  7:59           ` Heikki Krogerus
  0 siblings, 2 replies; 12+ messages in thread
From: Andy Shevchenko @ 2020-05-21  8:00 UTC (permalink / raw)
  To: Mauro Carvalho Chehab, Krogerus, Heikki
  Cc: open list:STAGING SUBSYSTEM, Greg Kroah-Hartman, Sakari Ailus,
	Bingbu Cao, Tian Shu Qiu, Linux Media Mailing List

+Cc: Heikki (swnode expert)

On Wed, May 20, 2020 at 2:19 PM Mauro Carvalho Chehab
<mchehab+huawei@kernel.org> wrote:
> Em Wed, 20 May 2020 11:26:08 +0300
> Sakari Ailus <sakari.ailus@linux.intel.com> escreveu:

...

> As I said, the problem is not probing the sensor via ACPI, but, instead,
> to be able receive platform-specific data.

There is no problem with swnodes, except missing parts (*).
I have Skylake laptop with IPU3 and with half-baked ACPI tables, but
since we have drivers in place with fwnode support, we only need to
recreate fwnode graph in some board file to compensate the gap in
ACPI.

*) Missing part is graph support for swnodes. With that done it will
be feasible to achieve the rest.
I forgot if we have anything for this already done. Heikki?

-- 
With Best Regards,
Andy Shevchenko
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] media: ipu3: add a module to probe sensors via ACPI
  2020-05-21  8:00         ` Andy Shevchenko
@ 2020-05-22  9:57           ` Mauro Carvalho Chehab
  2020-05-26 14:31             ` Heikki Krogerus
  2020-05-25  7:59           ` Heikki Krogerus
  1 sibling, 1 reply; 12+ messages in thread
From: Mauro Carvalho Chehab @ 2020-05-22  9:57 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: open list:STAGING SUBSYSTEM, Krogerus, Heikki,
	Greg Kroah-Hartman, Sakari Ailus, Bingbu Cao, Tian Shu Qiu,
	Linux Media Mailing List

Em Thu, 21 May 2020 11:00:19 +0300
Andy Shevchenko <andy.shevchenko@gmail.com> escreveu:

> +Cc: Heikki (swnode expert)
> 
> On Wed, May 20, 2020 at 2:19 PM Mauro Carvalho Chehab
> <mchehab+huawei@kernel.org> wrote:
> > Em Wed, 20 May 2020 11:26:08 +0300
> > Sakari Ailus <sakari.ailus@linux.intel.com> escreveu:  
> 
> ...
> 
> > As I said, the problem is not probing the sensor via ACPI, but, instead,
> > to be able receive platform-specific data.  
> 
> There is no problem with swnodes, except missing parts (*).
> I have Skylake laptop with IPU3 and with half-baked ACPI tables, but
> since we have drivers in place with fwnode support, we only need to
> recreate fwnode graph in some board file to compensate the gap in
> ACPI.
> 
> *) Missing part is graph support for swnodes. With that done it will
> be feasible to achieve the rest.
> I forgot if we have anything for this already done. Heikki?

Hmm... I guess I should try this approach. I never heard about swnodes
before. Do you have already some patch with the needed swnodes setup,
and the missing parts to recreate the fwnode graph?

Thanks,
Mauro
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] media: ipu3: add a module to probe sensors via ACPI
  2020-05-21  8:00         ` Andy Shevchenko
  2020-05-22  9:57           ` Mauro Carvalho Chehab
@ 2020-05-25  7:59           ` Heikki Krogerus
  1 sibling, 0 replies; 12+ messages in thread
From: Heikki Krogerus @ 2020-05-25  7:59 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: open list:STAGING SUBSYSTEM, Mauro Carvalho Chehab,
	Greg Kroah-Hartman, Sakari Ailus, Bingbu Cao, Tian Shu Qiu,
	Linux Media Mailing List

On Thu, May 21, 2020 at 11:00:19AM +0300, Andy Shevchenko wrote:
> +Cc: Heikki (swnode expert)
> 
> On Wed, May 20, 2020 at 2:19 PM Mauro Carvalho Chehab
> <mchehab+huawei@kernel.org> wrote:
> > Em Wed, 20 May 2020 11:26:08 +0300
> > Sakari Ailus <sakari.ailus@linux.intel.com> escreveu:
> 
> ...
> 
> > As I said, the problem is not probing the sensor via ACPI, but, instead,
> > to be able receive platform-specific data.
> 
> There is no problem with swnodes, except missing parts (*).
> I have Skylake laptop with IPU3 and with half-baked ACPI tables, but
> since we have drivers in place with fwnode support, we only need to
> recreate fwnode graph in some board file to compensate the gap in
> ACPI.
> 
> *) Missing part is graph support for swnodes. With that done it will
> be feasible to achieve the rest.
> I forgot if we have anything for this already done. Heikki?

I did implement the fwnode_graph* callbacks for swnodes, but I need to
rebase that patch on top of Dmitry's reference property changes.

thanks,

-- 
heikki
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] media: ipu3: add a module to probe sensors via ACPI
  2020-05-22  9:57           ` Mauro Carvalho Chehab
@ 2020-05-26 14:31             ` Heikki Krogerus
  2020-07-01  1:16               ` Jordan Hand
  2020-09-08 23:41               ` Dan Scally
  0 siblings, 2 replies; 12+ messages in thread
From: Heikki Krogerus @ 2020-05-26 14:31 UTC (permalink / raw)
  To: Mauro Carvalho Chehab
  Cc: open list:STAGING SUBSYSTEM, Greg Kroah-Hartman, Andy Shevchenko,
	Sakari Ailus, Bingbu Cao, Tian Shu Qiu, Linux Media Mailing List

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

On Fri, May 22, 2020 at 11:57:36AM +0200, Mauro Carvalho Chehab wrote:
> Em Thu, 21 May 2020 11:00:19 +0300
> Andy Shevchenko <andy.shevchenko@gmail.com> escreveu:
> 
> > +Cc: Heikki (swnode expert)
> > 
> > On Wed, May 20, 2020 at 2:19 PM Mauro Carvalho Chehab
> > <mchehab+huawei@kernel.org> wrote:
> > > Em Wed, 20 May 2020 11:26:08 +0300
> > > Sakari Ailus <sakari.ailus@linux.intel.com> escreveu:  
> > 
> > ...
> > 
> > > As I said, the problem is not probing the sensor via ACPI, but, instead,
> > > to be able receive platform-specific data.  
> > 
> > There is no problem with swnodes, except missing parts (*).
> > I have Skylake laptop with IPU3 and with half-baked ACPI tables, but
> > since we have drivers in place with fwnode support, we only need to
> > recreate fwnode graph in some board file to compensate the gap in
> > ACPI.
> > 
> > *) Missing part is graph support for swnodes. With that done it will
> > be feasible to achieve the rest.
> > I forgot if we have anything for this already done. Heikki?
> 
> Hmm... I guess I should try this approach. I never heard about swnodes
> before. Do you have already some patch with the needed swnodes setup,
> and the missing parts to recreate the fwnode graph?

Here you go. I tested it with this code:

        static const struct software_node nodes[];

        static const struct property_entry ep0_props[] = {
               PROPERTY_ENTRY_REF("remote-endpoint", &nodes[5]),
               { }
        };

        static const struct property_entry ep1_props[] = {
               PROPERTY_ENTRY_REF("remote-endpoint", &nodes[2]),
               { }
        };

        static const struct software_node nodes[] = {
               { "dev0" },
               { "port0", &nodes[0] },
               { "endpoint", &nodes[1], ep0_props },
               { "dev1" },
               { "port0", &nodes[3] },
               { "endpoint", &nodes[4], ep1_props },
               { }
        };

        void test(void)
        {
                const struct software_node *swnode;
                struct fwnode_handle *fwnode;

                software_node_register_nodes(nodes);

                fwnode = fwnode_graph_get_remote_port_parent(software_node_fwnode(&nodes[5]));
                swnode = to_software_node(fwnode);
                printk("first parent: %s\n", swnode->name);

                fwnode = fwnode_graph_get_remote_port_parent(software_node_fwnode(&nodes[2]));
                swnode = to_software_node(fwnode);
                printk("second parent: %s\n", swnode->name);

                software_node_unregister_nodes(nodes);
        }

thanks,

-- 
heikki

[-- Attachment #2: 0001-software-node-Add-support-for-fwnode_graph-family-of.patch --]
[-- Type: text/plain, Size: 4838 bytes --]

From c6f8f2253b09e68bfb74a9110165f04fc2f50c51 Mon Sep 17 00:00:00 2001
From: Heikki Krogerus <heikki.krogerus@linux.intel.com>
Date: Mon, 19 Aug 2019 11:09:41 +0300
Subject: [PATCH] software node: Add support for fwnode_graph* family of
 functions

This implements the remaining .graph_* callbacks in the
fwnode operations vector for the software nodes. That makes
the fwnode_graph*() functions available in the drivers also
when software nodes are used.

The implementation tries to mimic the "OF graph" as much as
possible, but there is no support for the "reg" device
property. The ports will need to have the index in their
name which starts with "port" (for example "port0", "port1",
...) and endpoints will use the index of the software node
that is given to them during creation. The port nodes can
also be grouped under a specially named "ports" subnode,
just like in DT, if necessary.

The remote-endpoints are reference properties under the
endpoint nodes that are named "remote-endpoint".

Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
---
 drivers/base/swnode.c | 109 +++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 108 insertions(+), 1 deletion(-)

diff --git a/drivers/base/swnode.c b/drivers/base/swnode.c
index de8d3543e8fe3..7359b3f4e5daa 100644
--- a/drivers/base/swnode.c
+++ b/drivers/base/swnode.c
@@ -536,6 +536,108 @@ software_node_get_reference_args(const struct fwnode_handle *fwnode,
 	return 0;
 }
 
+static struct fwnode_handle *
+swnode_graph_find_next_port(const struct fwnode_handle *parent,
+			    struct fwnode_handle *port)
+{
+	struct fwnode_handle *old = port;
+
+	while ((port = software_node_get_next_child(parent, old))) {
+		if (!strncmp(to_swnode(port)->node->name, "port", 4))
+			return port;
+		fwnode_handle_put(old);
+		old = port;
+	}
+
+	return NULL;
+}
+
+static struct fwnode_handle *
+software_node_graph_get_next_endpoint(const struct fwnode_handle *fwnode,
+				      struct fwnode_handle *endpoint)
+{
+	struct swnode *swnode = to_swnode(fwnode);
+	struct fwnode_handle *old = endpoint;
+	struct fwnode_handle *parent;
+	struct fwnode_handle *port;
+
+	if (!swnode)
+		return NULL;
+
+	if (endpoint) {
+		port = software_node_get_parent(endpoint);
+		parent = software_node_get_parent(port);
+	} else {
+		parent = software_node_get_named_child_node(fwnode, "ports");
+		if (!parent)
+			parent = software_node_get(&swnode->fwnode);
+
+		port = swnode_graph_find_next_port(parent, NULL);
+	}
+
+	for (; port; port = swnode_graph_find_next_port(parent, port)) {
+		endpoint = software_node_get_next_child(port, old);
+		fwnode_handle_put(old);
+		if (endpoint)
+			break;
+	}
+
+	fwnode_handle_put(port);
+	software_node_put(parent);
+
+	return endpoint;
+}
+
+static struct fwnode_handle *
+software_node_graph_get_remote_endpoint(const struct fwnode_handle *fwnode)
+{
+	struct swnode *swnode = to_swnode(fwnode);
+	const struct software_node_ref_args *ref;
+	const struct property_entry *prop;
+
+	if (!swnode)
+		return NULL;
+
+	prop = property_entry_get(swnode->node->properties, "remote-endpoint");
+	if (!prop || prop->type != DEV_PROP_REF || prop->is_inline)
+		return NULL;
+
+	ref = prop->pointer;
+
+	return software_node_get(software_node_fwnode(ref[0].node));
+}
+
+static struct fwnode_handle *
+software_node_graph_get_port_parent(struct fwnode_handle *fwnode)
+{
+	struct swnode *swnode = to_swnode(fwnode);
+	struct fwnode_handle *parent;
+
+	if (!strcmp(swnode->parent->node->name, "ports"))
+		parent = &swnode->parent->parent->fwnode;
+	else
+		parent = &swnode->parent->fwnode;
+
+	return software_node_get(parent);
+}
+
+static int
+software_node_graph_parse_endpoint(const struct fwnode_handle *fwnode,
+				   struct fwnode_endpoint *endpoint)
+{
+	struct swnode *swnode = to_swnode(fwnode);
+	int ret;
+
+	ret = kstrtou32(swnode->parent->node->name + 4, 10, &endpoint->port);
+	if (ret)
+		return ret;
+
+	endpoint->id = swnode->id;
+	endpoint->local_fwnode = fwnode;
+
+	return 0;
+}
+
 static const struct fwnode_operations software_node_ops = {
 	.get = software_node_get,
 	.put = software_node_put,
@@ -547,7 +649,12 @@ static const struct fwnode_operations software_node_ops = {
 	.get_parent = software_node_get_parent,
 	.get_next_child_node = software_node_get_next_child,
 	.get_named_child_node = software_node_get_named_child_node,
-	.get_reference_args = software_node_get_reference_args
+	.get_reference_args = software_node_get_reference_args,
+
+	.graph_get_next_endpoint = software_node_graph_get_next_endpoint,
+	.graph_get_remote_endpoint = software_node_graph_get_remote_endpoint,
+	.graph_get_port_parent = software_node_graph_get_port_parent,
+	.graph_parse_endpoint = software_node_graph_parse_endpoint,
 };
 
 /* -------------------------------------------------------------------------- */
-- 
2.26.2


[-- Attachment #3: Type: text/plain, Size: 169 bytes --]

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH] media: ipu3: add a module to probe sensors via ACPI
  2020-05-26 14:31             ` Heikki Krogerus
@ 2020-07-01  1:16               ` Jordan Hand
  2020-09-07 13:17                 ` Dan Scally
  2020-09-08 23:41               ` Dan Scally
  1 sibling, 1 reply; 12+ messages in thread
From: Jordan Hand @ 2020-07-01  1:16 UTC (permalink / raw)
  To: Heikki Krogerus, Mauro Carvalho Chehab
  Cc: open list:STAGING SUBSYSTEM, Greg Kroah-Hartman, Andy Shevchenko,
	Sakari Ailus, Bingbu Cao, Tian Shu Qiu, Linux Media Mailing List

On 5/26/20 7:31 AM, Heikki Krogerus wrote:
> On Fri, May 22, 2020 at 11:57:36AM +0200, Mauro Carvalho Chehab wrote:
>> Em Thu, 21 May 2020 11:00:19 +0300
>> Andy Shevchenko <andy.shevchenko@gmail.com> escreveu:
>>
>>> +Cc: Heikki (swnode expert)
>>>
>>> On Wed, May 20, 2020 at 2:19 PM Mauro Carvalho Chehab
>>> <mchehab+huawei@kernel.org> wrote:
>>>> Em Wed, 20 May 2020 11:26:08 +0300
>>>> Sakari Ailus <sakari.ailus@linux.intel.com> escreveu:
>>>
>>> ...
>>>
>>>> As I said, the problem is not probing the sensor via ACPI, but, instead,
>>>> to be able receive platform-specific data.
>>>
>>> There is no problem with swnodes, except missing parts (*).
>>> I have Skylake laptop with IPU3 and with half-baked ACPI tables, but
>>> since we have drivers in place with fwnode support, we only need to
>>> recreate fwnode graph in some board file to compensate the gap in
>>> ACPI.
>>>
>>> *) Missing part is graph support for swnodes. With that done it will
>>> be feasible to achieve the rest.
>>> I forgot if we have anything for this already done. Heikki?
>>
>> Hmm... I guess I should try this approach. I never heard about swnodes
>> before. Do you have already some patch with the needed swnodes setup,
>> and the missing parts to recreate the fwnode graph?
> 
> Here you go.
> 

For anyone interested, I have taken Heikki's patch and attempted to use 
swnodes to patch the incomplete dsdt on my laptop to use with ipu3; the 
code is currently in a github repo[1].

In particular, patches 1, 2, and 3 setup the software_node 
infrastructure. Patch 5 shows how we might use software nodes where ACPI 
fails.

My sensor driver (in patch 4) doesn't actually work right now which is 
why I haven't brought any of this to the mailing list yet, but that's 
another story :)

I would just submit a patchset, but since my sensor driver doesn't work, 
I can't gully test the rest of it. But if someone has a system where the 
drivers in question are upstream and work, something like this could be 
a good path forward.

- Jordan

[1] https://github.com/jhand2/surface-camera/tree/master/patches
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] media: ipu3: add a module to probe sensors via ACPI
  2020-07-01  1:16               ` Jordan Hand
@ 2020-09-07 13:17                 ` Dan Scally
  0 siblings, 0 replies; 12+ messages in thread
From: Dan Scally @ 2020-09-07 13:17 UTC (permalink / raw)
  To: Jordan Hand, Heikki Krogerus, Mauro Carvalho Chehab
  Cc: open list:STAGING SUBSYSTEM, Greg Kroah-Hartman, kieran.bingham,
	kitakar, Sakari Ailus, Bingbu Cao, Andy Shevchenko, Tian Shu Qiu,
	Linux Media Mailing List

On 01/07/2020 02:16, Jordan Hand wrote:
> On 5/26/20 7:31 AM, Heikki Krogerus wrote:
>> On Fri, May 22, 2020 at 11:57:36AM +0200, Mauro Carvalho Chehab wrote:
>>> Em Thu, 21 May 2020 11:00:19 +0300
>>> Andy Shevchenko <andy.shevchenko@gmail.com> escreveu:
>>>
>>>> +Cc: Heikki (swnode expert)
>>>>
>>>> On Wed, May 20, 2020 at 2:19 PM Mauro Carvalho Chehab
>>>> <mchehab+huawei@kernel.org> wrote:
>>>>> Em Wed, 20 May 2020 11:26:08 +0300
>>>>> Sakari Ailus <sakari.ailus@linux.intel.com> escreveu:
>>>>
>>>> ...
>>>>
>>>>> As I said, the problem is not probing the sensor via ACPI, but,
>>>>> instead,
>>>>> to be able receive platform-specific data.
>>>>
>>>> There is no problem with swnodes, except missing parts (*).
>>>> I have Skylake laptop with IPU3 and with half-baked ACPI tables, but
>>>> since we have drivers in place with fwnode support, we only need to
>>>> recreate fwnode graph in some board file to compensate the gap in
>>>> ACPI.
>>>>
>>>> *) Missing part is graph support for swnodes. With that done it will
>>>> be feasible to achieve the rest.
>>>> I forgot if we have anything for this already done. Heikki?
>>>
>>> Hmm... I guess I should try this approach. I never heard about swnodes
>>> before. Do you have already some patch with the needed swnodes setup,
>>> and the missing parts to recreate the fwnode graph?
>>
>> Here you go.
>>
>
> For anyone interested, I have taken Heikki's patch and attempted to
> use swnodes to patch the incomplete dsdt on my laptop to use with
> ipu3; the code is currently in a github repo[1].
>
> In particular, patches 1, 2, and 3 setup the software_node
> infrastructure. Patch 5 shows how we might use software nodes where
> ACPI fails.
>
> My sensor driver (in patch 4) doesn't actually work right now which is
> why I haven't brought any of this to the mailing list yet, but that's
> another story :)
>
> I would just submit a patchset, but since my sensor driver doesn't
> work, I can't gully test the rest of it. But if someone has a system
> where the drivers in question are upstream and work, something like
> this could be a good path forward.
>
> - Jordan
>
> [1] https://github.com/jhand2/surface-camera/tree/master/patches
>
>
Hello all

I joined in these efforts [2] to get cameras working on
Microsoft Surface and similar platforms, currently I'm working on
expanding Jordan's module connecting cameras to the cio2
infrastructure (which works - we can use it to take images), aiming to
discover connection information from SSDB in the DSDT, as well as
connect as many supported sensors as are found on the device. I'm just
struggling with a problem I've encountered using the swnode patch that
Heikki linked in this thread; the module's working ok when I only
attempt to connect a single one of my sensors (by preventing the
driver for the other sensor from loading, in which case this new
module ignores the sensor), but when I attempt to connect both cameras
at the same time I get a kernel oops as part of
software_node_get_next_child. The module is creating software_nodes
like this...

/sys/kernel/software_node/INT343E/port0/endpoint0
/sys/kernel/software_node/INT343E/port1/endpoint0
/sys/kernel/software_node/OVTI2680/port0/endpoint0
/sys/kernel/software_node/OVTI5648/port0/endpoint0

And that's the structure that I expect to see, but it seems like the
call to list_next_entry in that function is returning something that
isn't quite a valid swnode. Printing the address of c->fwnode after
that point returns "3", which isn't a valid address of course, and
that's causing the oops when it's either returned (in the version of
the function without the patches) or when the program flow tries to
call the "get" op against that fwnode (in the patched version). I've
been trying to track it down for a while now without success, so I
posted the problem on SO[3] and it was suggested that I mail these
addressees for advice - hope that that is ok.


My copy of Jordan's module is parked in my git repo [4] for now, and
requires the batch of patches from Jordan's repo [5] -
I've been applying those against 5.8.0-rc7. Any other criticism more
than welcome - I'm new to both c and kernel programming so I'm happy
to take all the advice people have the time to give.


On a more general note; Kieran from the libcamera project suggested we
ought to be talking to you guys anyway to get some guidance on design,
and some more expert eye on the things we don't really understand. In
particular; we haven't been able to figure out how sensors that are
intended to work with the cio2 ipu3 stuff have their clock/regulators
supplied - in fact I can strip all the "usual" clock/regulator
functionality out of my camera's driver and it still functions fine,
which seems a bit weird. So far all we're doing as "power management"
of the camera's is turning on the GPIO pins that DSDT tables assign to
the tps68470 PMICs the cameras are theoretically hooked up to...but
given the drivers continue to work without using the PMICs regulator
and clk drivers (which we found in the intel-lts tree on Github),
we're a bit confused exactly how these are connected. Given the
potential for actual hardware damage if we mess something up there
it'd be great if anyone can shed some light on those elements.


Regards

Dan


[2] https://github.com/linux-surface/linux-surface/issues/91

[3] https://stackoverflow.com/questions/63742967/what-is-causing-this-kernel-oops-when-parsing-firmware?

[4] https://github.com/djrscally/miix-510-cameras/blob/master/surface_camera/surface_camera.c

[5] https://github.com/jhand2/surface-camera/tree/master/patches



_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] media: ipu3: add a module to probe sensors via ACPI
  2020-05-26 14:31             ` Heikki Krogerus
  2020-07-01  1:16               ` Jordan Hand
@ 2020-09-08 23:41               ` Dan Scally
  1 sibling, 0 replies; 12+ messages in thread
From: Dan Scally @ 2020-09-08 23:41 UTC (permalink / raw)
  To: Heikki Krogerus, Mauro Carvalho Chehab
  Cc: open list:STAGING SUBSYSTEM, Jordan Hand, Greg Kroah-Hartman,
	Kieran Bingham, Tsuchiya Yuto, Sakari Ailus, Bingbu Cao,
	Andy Shevchenko, Tian Shu Qiu, Linux Media Mailing List

Hi Heikki

On 26/05/2020 15:31, Heikki Krogerus wrote:
> On Fri, May 22, 2020 at 11:57:36AM +0200, Mauro Carvalho Chehab wrote:
>> Em Thu, 21 May 2020 11:00:19 +0300
>> Andy Shevchenko <andy.shevchenko@gmail.com> escreveu:
>>
>>> +Cc: Heikki (swnode expert)
>>>
>>> On Wed, May 20, 2020 at 2:19 PM Mauro Carvalho Chehab
>>> <mchehab+huawei@kernel.org> wrote:
>>>> Em Wed, 20 May 2020 11:26:08 +0300
>>>> Sakari Ailus <sakari.ailus@linux.intel.com> escreveu:
>>>
>>> ...
>>>
>>>> As I said, the problem is not probing the sensor via ACPI, but, instead,
>>>> to be able receive platform-specific data.
>>>
>>> There is no problem with swnodes, except missing parts (*).
>>> I have Skylake laptop with IPU3 and with half-baked ACPI tables, but
>>> since we have drivers in place with fwnode support, we only need to
>>> recreate fwnode graph in some board file to compensate the gap in
>>> ACPI.
>>>
>>> *) Missing part is graph support for swnodes. With that done it will
>>> be feasible to achieve the rest.
>>> I forgot if we have anything for this already done. Heikki?
>>
>> Hmm... I guess I should try this approach. I never heard about swnodes
>> before. Do you have already some patch with the needed swnodes setup,
>> and the missing parts to recreate the fwnode graph?
> 
> Here you go. I tested it with this code:
> 
>          static const struct software_node nodes[];
> 
>          static const struct property_entry ep0_props[] = {
>                 PROPERTY_ENTRY_REF("remote-endpoint", &nodes[5]),
>                 { }
>          };
> 
>          static const struct property_entry ep1_props[] = {
>                 PROPERTY_ENTRY_REF("remote-endpoint", &nodes[2]),
>                 { }
>          };
> 
>          static const struct software_node nodes[] = {
>                 { "dev0" },
>                 { "port0", &nodes[0] },
>                 { "endpoint", &nodes[1], ep0_props },
>                 { "dev1" },
>                 { "port0", &nodes[3] },
>                 { "endpoint", &nodes[4], ep1_props },
>                 { }
>          };
> 
>          void test(void)
>          {
>                  const struct software_node *swnode;
>                  struct fwnode_handle *fwnode;
> 
>                  software_node_register_nodes(nodes);
> 
>                  fwnode = fwnode_graph_get_remote_port_parent(software_node_fwnode(&nodes[5]));
>                  swnode = to_software_node(fwnode);
>                  printk("first parent: %s\n", swnode->name);
> 
>                  fwnode = fwnode_graph_get_remote_port_parent(software_node_fwnode(&nodes[2]));
>                  swnode = to_software_node(fwnode);
>                  printk("second parent: %s\n", swnode->name);
> 
>                  software_node_unregister_nodes(nodes);
>          }
> 
> thanks,
> 

One of the problems we're having trying to build (using the changes you 
attached here) a module to connect sensors to the cio2 infrastructure is 
that we can't unload it cleanly. There seems to be a couple of reasons 
for that; but one of them is that cio2_parse_firmware() in ipu3-cio2.c 
ticks up the refcount for fwnode_handles of the ports for the CIO2 
device by calling software_node_graph_get_next_endpoint() once per 
_possible_ cio2 port; each time that happens it gets a reference to the 
port's fwnode_handle but doesn't release it.

This isn't really a patch as such, since I don't think the changes you 
attached are actually applied either upstream or in the media_tree git 
(what are the plans in that regard, by the way? Will that patch be sent 
upstream at some point?) so there's nowhere to apply it to, but I think 
something like the below fixes it.

What do you think?

Regards,
Dan

---
diff --git a/drivers/base/swnode.c b/drivers/base/swnode.c
index 3667467196f0..62a1e3de8cb3 100644
--- a/drivers/base/swnode.c
+++ b/drivers/base/swnode.c
@@ -584,7 +584,9 @@ software_node_graph_get_next_endpoint(const struct 
fwnode_handle *fwnode,
                 endpoint = software_node_get_next_child(port, old);
                 fwnode_handle_put(old);
                 if (endpoint)
-                       break;
+                       break;
+               else
+                       fwnode_handle_put(port);
         }

         fwnode_handle_put(port);
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

^ permalink raw reply related	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2020-09-08 23:41 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-05-16 10:43 [PATCH] media: ipu3: add a module to probe sensors via ACPI Mauro Carvalho Chehab
2020-05-17 10:36 ` Sakari Ailus
2020-05-20  7:44   ` Mauro Carvalho Chehab
2020-05-20  8:26     ` Sakari Ailus
2020-05-20 11:18       ` Mauro Carvalho Chehab
2020-05-21  8:00         ` Andy Shevchenko
2020-05-22  9:57           ` Mauro Carvalho Chehab
2020-05-26 14:31             ` Heikki Krogerus
2020-07-01  1:16               ` Jordan Hand
2020-09-07 13:17                 ` Dan Scally
2020-09-08 23:41               ` Dan Scally
2020-05-25  7:59           ` Heikki Krogerus

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).