| Message ID | 12fbe3f5c6a16c5f3447adbc09fe27ceb2b16823.1589625807.git.mchehab+huawei@kernel.org |
|---|---|
| State | New |
| Headers | show |
| Series | media: ipu3: add a module to probe sensors via ACPI | expand |
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
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);
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");
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