| Message ID | 20201130133129.1024662-1-djrscally@gmail.com |
|---|---|
| Headers | show |
| Series | Add functionality to ipu3-cio2 driver allowing software_node connections to sensors on platforms designed for Windows | expand |
Hi Daniel, Thank you for the patch. On Mon, Nov 30, 2020 at 01:31:12PM +0000, Daniel Scally wrote: > Some types of fwnode_handle do not implement the device_is_available() > check, such as those created by software_nodes. There isn't really a > meaningful way to check for the availability of a device that doesn't > actually exist, so if the check isn't implemented just assume that the > "device" is present. > > Suggested-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > Signed-off-by: Daniel Scally <djrscally@gmail.com> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > Changes since RFC v3: > > patch introduced > > drivers/base/property.c | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/drivers/base/property.c b/drivers/base/property.c > index 4c43d30145c6..a5ca2306796f 100644 > --- a/drivers/base/property.c > +++ b/drivers/base/property.c > @@ -785,9 +785,14 @@ EXPORT_SYMBOL_GPL(fwnode_handle_put); > /** > * fwnode_device_is_available - check if a device is available for use > * @fwnode: Pointer to the fwnode of the device. > + * > + * For fwnode node types that don't implement the .device_is_available() > + * operation, this function returns true. > */ > bool fwnode_device_is_available(const struct fwnode_handle *fwnode) > { > + if (!fwnode_has_op(fwnode, device_is_available)) > + return true; > return fwnode_call_bool_op(fwnode, device_is_available); > } > EXPORT_SYMBOL_GPL(fwnode_device_is_available);
Hi Daniel, Thank you for the patch. On Mon, Nov 30, 2020 at 01:31:14PM +0000, Daniel Scally wrote: > The software_node_get_next_child() function currently does not hold > references to the child software_node that it finds or put the ref that > is held against the old child - fix that. > > Signed-off-by: Daniel Scally <djrscally@gmail.com> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > Changes since RFC v3: > > Put reference to previous child. > > drivers/base/swnode.c | 8 ++++++-- > 1 file changed, 6 insertions(+), 2 deletions(-) > > diff --git a/drivers/base/swnode.c b/drivers/base/swnode.c > index 010828fc785b..615a0c93e116 100644 > --- a/drivers/base/swnode.c > +++ b/drivers/base/swnode.c > @@ -443,14 +443,18 @@ software_node_get_next_child(const struct fwnode_handle *fwnode, > struct swnode *c = to_swnode(child); > > if (!p || list_empty(&p->children) || > - (c && list_is_last(&c->entry, &p->children))) > + (c && list_is_last(&c->entry, &p->children))) { > + fwnode_handle_put(child); > return NULL; > + } > > if (c) > c = list_next_entry(c, entry); > else > c = list_first_entry(&p->children, struct swnode, entry); > - return &c->fwnode; > + > + fwnode_handle_put(child); > + return fwnode_handle_get(&c->fwnode); > } > > static struct fwnode_handle *
Hi Daniel, Thank you for the patch. On Mon, Nov 30, 2020 at 01:31:19PM +0000, Daniel Scally wrote: > Use the software_node_unregister_nodes() helper function to unwind this > array in a cleaner way. > > Acked-by: Petr Mladek <pmladek@suse.com> > Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > Suggested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > Signed-off-by: Daniel Scally <djrscally@gmail.com> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > Changes since RFC v3: > > Changed the called function name - didn't drop the tags since it's > such a trivial change, hope that's alright! > > lib/test_printf.c | 4 +--- > 1 file changed, 1 insertion(+), 3 deletions(-) > > diff --git a/lib/test_printf.c b/lib/test_printf.c > index 7ac87f18a10f..7d60f24240a4 100644 > --- a/lib/test_printf.c > +++ b/lib/test_printf.c > @@ -644,9 +644,7 @@ static void __init fwnode_pointer(void) > test(second_name, "%pfwP", software_node_fwnode(&softnodes[1])); > test(third_name, "%pfwP", software_node_fwnode(&softnodes[2])); > > - software_node_unregister(&softnodes[2]); > - software_node_unregister(&softnodes[1]); > - software_node_unregister(&softnodes[0]); > + software_node_unregister_nodes(softnodes); > } > > static void __init
Hi Daniel, On 30/11/2020 13:31, Daniel Scally wrote: > On platforms where ACPI is designed for use with Windows, resources > that are intended to be consumed by sensor devices are sometimes in > the _CRS of a dummy INT3472 device upon which the sensor depends. This > driver binds to the dummy acpi device (which does not represent a > physical PMIC) and maps them into GPIO lines and regulators for use by > the sensor device instead. > > Suggested-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > Signed-off-by: Daniel Scally <djrscally@gmail.com> > --- > Changes since RFC v3: > > - Patch introduced > > This patch contains the bits of this process that we're least sure about. > The sensors in scope for this work are called out as dependent (in their > DSDT entry's _DEP) on a device with _HID INT3472. These come in at least > 2 kinds; those with an I2cSerialBusV2 entry (which we presume therefore > are legitimate tps68470 PMICs that need handling by those drivers - work > on that in the future). And those without an I2C device. For those without > an I2C device they instead have an array of GPIO pins defined in _CRS. So > for example, my Lenovo Miix 510's OVTI2680 sensor is dependent on one of > the _latter_ kind of INT3472 devices, with this _CRS: > > Method (_CRS, 0, NotSerialized) // _CRS: Current Resource Settings > { > Name (SBUF, ResourceTemplate () > { > GpioIo (Exclusive, PullDefault, 0x0000, 0x0000, > IoRestrictionOutputOnly, "\\_SB.PCI0.GPI0", > 0x00, ResourceConsumer, , > ) > { // Pin list > 0x0079 > } > GpioIo (Exclusive, PullDefault, 0x0000, 0x0000, > IoRestrictionOutputOnly, "\\_SB.PCI0.GPI0", > 0x00, ResourceConsumer, , > ) > { // Pin list > 0x007A > } > GpioIo (Exclusive, PullDefault, 0x0000, 0x0000, > IoRestrictionOutputOnly, "\\_SB.PCI0.GPI0", > 0x00, ResourceConsumer, , > ) > { // Pin list > 0x008F > } > }) > Return (SBUF) /* \_SB_.PCI0.PMI1._CRS.SBUF */ > } > > and the same device has a _DSM Method, which returns 32-bit ints where > the second lowest byte we noticed to match the pin numbers of the GPIO > lines: > > Method (_DSM, 4, NotSerialized) // _DSM: Device-Specific Method > { > If ((Arg0 == ToUUID ("79234640-9e10-4fea-a5c1-b5aa8b19756f"))) > { > If ((Arg2 == One)) > { > Return (0x03) > } > > If ((Arg2 == 0x02)) > { > Return (0x01007900) > } > > If ((Arg2 == 0x03)) > { > Return (0x01007A0C) > } > > If ((Arg2 == 0x04)) > { > Return (0x01008F01) > } > } > > Return (Zero) > } > > We know that at least some of those pins have to be toggled active for the > sensor devices to be available in i2c, so the conclusion we came to was > that those GPIO entries assigned to the INT3472 device actually represent > GPIOs and regulators to be consumed by the sensors themselves. Tsuchiya > noticed that the lowest byte in the return values of the _DSM method > seemed to represent the type or function of the GPIO line, and we > confirmed that by testing on each surface device that GPIO lines where the > low byte in the _DSM entry for that pin was 0x0d controlled the privacy > LED of the cameras. > > We're guessing as to the exact meaning of the function byte, but I > conclude they're something like this: > > 0x00 - probably a reset GPIO > 0x01 - regulator for the sensor > 0x0c - regulator for the sensor > 0x0b - regulator again, but for a VCM or EEPROM > 0x0d - privacy led (only one we're totally confident of since we can see > it happen!) > > After much internal debate I decided to write this as a standalone > acpi_driver. Alternative options we considered: > > 1. Squash all this into the cio2-bridge code, which I did originally write > but decided I didn't like. > 2. Extend the existing tps68470 mfd driver...they share an ACPI ID so this > kinda makes sense, but ultimately given there is no actual physical > tps68470 in the scenario this patch handles I decided I didn't like this > either. > I would agree, keeping this in a unit file on it's own makes sense to me. I'm a bit worried about what happens if the tps68470 is also compiled in... Does the right device get mapped in the event that there are also actual devices already supported by the tps68470 mfd driver on the device as well? > MAINTAINERS | 7 + > drivers/media/pci/intel/ipu3/Kconfig | 14 + > drivers/media/pci/intel/ipu3/Makefile | 1 + > drivers/media/pci/intel/ipu3/int3472.c | 381 +++++++++++++++++++++++++ > drivers/media/pci/intel/ipu3/int3472.h | 96 +++++++ > 5 files changed, 499 insertions(+) > create mode 100644 drivers/media/pci/intel/ipu3/int3472.c > create mode 100644 drivers/media/pci/intel/ipu3/int3472.h > > diff --git a/MAINTAINERS b/MAINTAINERS > index 188559a0a610..d73471f9c2a3 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -8753,6 +8753,13 @@ L: linux-crypto@vger.kernel.org > S: Maintained > F: drivers/crypto/inside-secure/ > > +INT3472 ACPI DEVICE DRIVER > +M: Daniel Scally <djrscally@gmail.com> > +L: linux-media@vger.kernel.org > +S: Maintained > +F: drivers/media/pci/intel/ipu3/int3472.c > +F: drivers/media/pci/intel/ipu3/int3472.h > + > INTEGRITY MEASUREMENT ARCHITECTURE (IMA) > M: Mimi Zohar <zohar@linux.ibm.com> > M: Dmitry Kasatkin <dmitry.kasatkin@gmail.com> > diff --git a/drivers/media/pci/intel/ipu3/Kconfig b/drivers/media/pci/intel/ipu3/Kconfig > index 2b3350d042be..9dd3b280f821 100644 > --- a/drivers/media/pci/intel/ipu3/Kconfig > +++ b/drivers/media/pci/intel/ipu3/Kconfig > @@ -34,3 +34,17 @@ config CIO2_BRIDGE > - Dell 7285 > > If in doubt, say N here. > + > +config INT3472 > + tristate "INT3472 Dummy ACPI Device Driver" > + depends on VIDEO_IPU3_CIO2 > + depends on ACPI && REGULATOR && GPIOLIB > + help > + This module provides an ACPI driver for INT3472 devices that do not > + represent an actual physical tps68470 device. > + > + Say Y here if your device is a detachable / hybrid laptop that comes > + with Windows installed by the OEM. > + The module will be called int3472. > + > + If in doubt, say N here. > diff --git a/drivers/media/pci/intel/ipu3/Makefile b/drivers/media/pci/intel/ipu3/Makefile > index 933777e6ea8a..2285947b2bd2 100644 > --- a/drivers/media/pci/intel/ipu3/Makefile > +++ b/drivers/media/pci/intel/ipu3/Makefile > @@ -1,5 +1,6 @@ > # SPDX-License-Identifier: GPL-2.0-only > obj-$(CONFIG_VIDEO_IPU3_CIO2) += ipu3-cio2.o > +obj-$(CONFIG_INT3472) += int3472.o > > ipu3-cio2-y += ipu3-cio2-main.o > ipu3-cio2-$(CONFIG_CIO2_BRIDGE) += cio2-bridge.o > diff --git a/drivers/media/pci/intel/ipu3/int3472.c b/drivers/media/pci/intel/ipu3/int3472.c > new file mode 100644 > index 000000000000..6b0be75f7f35 > --- /dev/null > +++ b/drivers/media/pci/intel/ipu3/int3472.c > @@ -0,0 +1,381 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* Author: Dan Scally <djrscally@gmail.com> */ > +#include <linux/acpi.h> > +#include <linux/gpio/consumer.h> > +#include <linux/gpio/machine.h> > +#include <linux/i2c.h> > +#include <linux/kernel.h> > +#include <linux/list.h> > +#include <linux/module.h> > +#include <linux/regulator/driver.h> > + > +#include "int3472.h" > + > +/* > + * The regulators have to have .ops to be valid, but the only ops we actually > + * support are .enable and .disable which are handled via .ena_gpiod. Pass an > + * empty struct to clear the check without lying about capabilities. > + */ > +static const struct regulator_ops int3472_gpio_regulator_ops = { 0 }; > + > +static int int3472_map_gpio_to_sensor(struct int3472_device *int3472, > + struct acpi_resource *ares, char *func) > +{ > + char *path = ares->data.gpio.resource_source.string_ptr; > + struct gpiod_lookup table_entry; > + struct acpi_device *adev; > + acpi_handle handle; > + acpi_status status; > + int ret; > + > + /* Make sure we don't overflow, and leave room for a terminator */ > + if (int3472->n_sensor_gpios >= INT3472_MAX_SENSOR_GPIOS) { > + dev_warn(&int3472->sensor->dev, "Too many GPIOs mapped\n"); > + return -EINVAL; > + } > + > + /* Fetch ACPI handle for the GPIO chip */ > + status = acpi_get_handle(NULL, path, &handle); > + if (ACPI_FAILURE(status)) > + return -EINVAL; > + > + ret = acpi_bus_get_device(handle, &adev); > + if (ret) > + return -ENODEV; > + > + table_entry = (struct gpiod_lookup)GPIO_LOOKUP_IDX(acpi_dev_name(adev), > + ares->data.gpio.pin_table[0], > + func, 0, GPIO_ACTIVE_HIGH); > + > + memcpy(&int3472->gpios.table[int3472->n_sensor_gpios], &table_entry, > + sizeof(table_entry)); > + int3472->n_sensor_gpios++; > + > + return 0; > +} > + > +static struct int3472_sensor_regulator_map * > +int3472_get_sensor_supply_map(struct int3472_device *int3472) > +{ > + struct int3472_sensor_regulator_map *ret; > + union acpi_object *obj; > + unsigned int i; > + > + /* > + * Sensor modules seem to be identified by a unique string. We use that > + * to make sure we pass the right device and supply names to the new > + * regulator's consumer_supplies > + */ > + obj = acpi_evaluate_dsm_typed(int3472->sensor->handle, > + &cio2_sensor_module_guid, 0x00, > + 0x01, NULL, ACPI_TYPE_STRING); > + > + if (!obj) { > + dev_err(&int3472->sensor->dev, > + "Failed to get sensor module string from _DSM\n"); > + return ERR_PTR(-ENODEV); > + } > + > + if (obj->string.type != ACPI_TYPE_STRING) { > + dev_err(&int3472->sensor->dev, > + "Sensor _DSM returned a non-string value\n"); > + ret = ERR_PTR(-EINVAL); > + goto out_free_obj; > + } > + > + ret = ERR_PTR(-ENODEV); > + for (i = 0; i < ARRAY_SIZE(int3472_sensor_regulator_maps); i++) { > + if (!strcmp(int3472_sensor_regulator_maps[i].sensor_module_name, > + obj->string.pointer)) { > + ret = &int3472_sensor_regulator_maps[i]; > + goto out_free_obj; > + } > + } > + > +out_free_obj: > + ACPI_FREE(obj); > + return ret; > +} > + > +static int int3472_register_regulator(struct int3472_device *int3472, > + struct acpi_resource *ares) > +{ > + char *path = ares->data.gpio.resource_source.string_ptr; > + struct int3472_sensor_regulator_map *regulator_map; > + struct regulator_init_data init_data = { }; > + struct int3472_gpio_regulator *regulator; > + struct regulator_config cfg = { }; > + int ret; > + > + /* > + * We lookup supply names from machine specific tables, based on a > + * unique identifier in the sensor's _DSM > + */ > + regulator_map = int3472_get_sensor_supply_map(int3472); > + if (IS_ERR_OR_NULL(regulator_map)) { > + dev_err(&int3472->sensor->dev, > + "Found no supplies defined for this sensor\n"); > + return PTR_ERR(regulator_map); > + } > + > + if (int3472->n_regulators >= regulator_map->n_supplies) { > + dev_err(&int3472->sensor->dev, > + "All known supplies are already mapped\n"); > + return -EINVAL; > + } > + > + init_data.supply_regulator = NULL; > + init_data.constraints.valid_ops_mask = REGULATOR_CHANGE_STATUS; > + init_data.num_consumer_supplies = 1; > + init_data.consumer_supplies = ®ulator_map->supplies[int3472->n_regulators]; > + > + regulator = kmalloc(sizeof(*regulator), GFP_KERNEL); > + if (!regulator) > + return -ENOMEM; > + > + snprintf(regulator->regulator_name, GPIO_REGULATOR_NAME_LENGTH, > + "gpio-regulator-%d", int3472->n_regulators); > + snprintf(regulator->supply_name, GPIO_REGULATOR_SUPPLY_NAME_LENGTH, > + "supply-%d", int3472->n_regulators); > + > + regulator->rdesc = INT3472_REGULATOR(regulator->regulator_name, > + regulator->supply_name, > + int3472->n_regulators, > + &int3472_gpio_regulator_ops); > + > + regulator->gpio = acpi_get_gpiod(path, ares->data.gpio.pin_table[0]); > + if (IS_ERR(regulator->gpio)) { > + ret = PTR_ERR(regulator->gpio); > + goto err_free_regulator; > + } > + > + cfg.dev = &int3472->adev->dev; > + cfg.init_data = &init_data; > + cfg.ena_gpiod = regulator->gpio; > + > + regulator->rdev = regulator_register(®ulator->rdesc, &cfg); > + if (IS_ERR(regulator->rdev)) { > + ret = PTR_ERR(regulator->rdev); > + goto err_free_gpio; > + } > + > + list_add(®ulator->list, &int3472->regulators); > + int3472->n_regulators++; > + > + return 0; > + > +err_free_gpio: > + gpiod_put(regulator->gpio); > +err_free_regulator: > + kfree(regulator); > + > + return ret; > +} > + > +static int int3472_handle_gpio_resources(struct acpi_resource *ares, > + void *data) > +{ > + struct int3472_device *int3472 = data; > + union acpi_object *obj; > + int ret = 0; > + > + if (ares->type != ACPI_RESOURCE_TYPE_GPIO || > + ares->data.gpio.connection_type != ACPI_RESOURCE_GPIO_TYPE_IO) > + return EINVAL; /* Deliberately positive */ > + > + /* > + * n_gpios + 2 because the index of this _DSM function is 1-based and > + * the first function is just a count. > + */ > + obj = acpi_evaluate_dsm_typed(int3472->adev->handle, > + &int3472_gpio_guid, 0x00, > + int3472->n_gpios + 2, > + NULL, ACPI_TYPE_INTEGER); > + > + if (!obj) { > + dev_warn(&int3472->adev->dev, > + "No _DSM entry for this GPIO pin\n"); > + return ENODEV; > + } > + > + switch (obj->integer.value & 0xff) { /* low byte holds type data */ > + case 0x00: /* Purpose unclear, possibly a reset GPIO pin */ > + ret = int3472_map_gpio_to_sensor(int3472, ares, "reset"); > + if (ret) > + dev_warn(&int3472->adev->dev, > + "Failed to map reset pin to sensor\n"); > + > + break; > + case 0x01: /* Power regulators (we think) */ > + case 0x0c: A little annoying that 0x0c is before 0x0b ... but I see this is sharing with 0x01, so I think this can stay like this. It might be nice to wrap the values in some descriptive #defines though, but I'm not sure what we'd call them, and if we're not yet sure on their purposes - perhaps the raw values are better for now. We'll see I guess. > + ret = int3472_register_regulator(int3472, ares); > + if (ret) > + dev_warn(&int3472->adev->dev, > + "Failed to map regulator to sensor\n"); > + > + break; > + case 0x0b: /* Power regulators, but to a device separate to sensor */ > + ret = int3472_register_regulator(int3472, ares); > + if (ret) > + dev_warn(&int3472->adev->dev, > + "Failed to map regulator to sensor\n"); > + > + break; > + case 0x0d: /* Indicator LEDs */ > + ret = int3472_map_gpio_to_sensor(int3472, ares, "indicator-led"); > + if (ret) > + dev_warn(&int3472->adev->dev, > + "Failed to map indicator led to sensor\n"); > + > + break; > + default: > + /* if we've gotten here, we're not sure what they are yet */ > + dev_warn(&int3472->adev->dev, > + "GPIO type 0x%llx unknown; the sensor may not work\n", > + (obj->integer.value & 0xff)); > + ret = EINVAL; > + } > + > + int3472->n_gpios++; > + ACPI_FREE(obj); > + return abs(ret); > +} > + > +static void int3472_parse_crs(struct int3472_device *int3472) > +{ > + struct list_head resource_list; > + > + INIT_LIST_HEAD(&resource_list); > + > + acpi_dev_get_resources(int3472->adev, &resource_list, > + int3472_handle_gpio_resources, int3472); > + > + acpi_dev_free_resource_list(&resource_list); > + gpiod_add_lookup_table(&int3472->gpios); > +} > + > +static int int3472_add(struct acpi_device *adev) > +{ > + struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL }; > + struct int3472_device *int3472; > + struct int3472_cldb cldb; > + union acpi_object *obj; > + acpi_status status; > + int ret = 0; > + > + /* > + * This driver is only intended to support "dummy" INT3472 devices > + * which appear in ACPI designed for Windows. These are distinguishable > + * from INT3472 entries representing an actual tps68470 PMIC through > + * the presence of a CLDB buffer with a particular value set. > + */ > + status = acpi_evaluate_object(adev->handle, "CLDB", NULL, &buffer); > + if (ACPI_FAILURE(status)) > + return -ENODEV; > + > + obj = buffer.pointer; > + if (!obj) { > + dev_err(&adev->dev, "ACPI device has no CLDB object\n"); > + return -ENODEV; > + } > + > + if (obj->type != ACPI_TYPE_BUFFER) { > + dev_err(&adev->dev, "CLDB object is not an ACPI buffer\n"); > + ret = -EINVAL; > + goto out_free_buff; > + } > + > + if (obj->buffer.length > sizeof(cldb)) { > + dev_err(&adev->dev, "The CLDB buffer is too large\n"); > + ret = -EINVAL; > + goto out_free_buff; > + } > + > + memcpy(&cldb, obj->buffer.pointer, obj->buffer.length); > + > + /* > + * control_logic_type = 1 indicates this is a dummy INT3472 device of > + * the kind we're looking for. If any other value then we shouldn't try > + * to handle it > + */ > + if (cldb.control_logic_type != 1) { > + ret = -EINVAL; > + goto out_free_buff; > + } > + > + /* Space for 4 GPIOs - one more than we've seen so far plus a null */ > + int3472 = kzalloc(sizeof(*int3472) + > + ((INT3472_MAX_SENSOR_GPIOS + 1) * sizeof(struct gpiod_lookup)), > + GFP_KERNEL); > + if (!int3472) { > + ret = -ENOMEM; > + goto out_free_buff; > + } > + > + int3472->adev = adev; > + adev->driver_data = int3472; > + > + int3472->sensor = acpi_dev_get_next_dep_dev(adev, NULL); > + if (!int3472->sensor) { > + dev_err(&adev->dev, > + "This INT3472 entry seems to have no dependents.\n"); > + ret = -ENODEV; > + goto out_free_int3472; > + } > + > + int3472->gpios.dev_id = i2c_acpi_dev_name(int3472->sensor); > + > + INIT_LIST_HEAD(&int3472->regulators); > + > + int3472_parse_crs(int3472); > + > + goto out_free_buff; > + > +out_free_int3472: > + kfree(int3472); > +out_free_buff: > + kfree(buffer.pointer); > + return ret; > +} > + > +static int int3472_remove(struct acpi_device *adev) > +{ > + struct int3472_gpio_regulator *reg; > + struct int3472_device *int3472; > + > + int3472 = acpi_driver_data(adev); > + > + acpi_dev_put(int3472->sensor); > + gpiod_remove_lookup_table(&int3472->gpios); > + > + list_for_each_entry(reg, &int3472->regulators, list) { > + gpiod_put(reg->gpio); > + regulator_unregister(reg->rdev); > + } > + > + kfree(int3472); > + > + return 0; > +} > + > +static const struct acpi_device_id int3472_device_id[] = { > + { "INT3472", 0 }, > + { }, > +}; > +MODULE_DEVICE_TABLE(acpi, int3472_device_id); > + > +static struct acpi_driver int3472_driver = { > + .name = "int3472", > + .ids = int3472_device_id, > + .ops = { > + .add = int3472_add, > + .remove = int3472_remove, > + }, > + .owner = THIS_MODULE, > +}; > + > +module_acpi_driver(int3472_driver); > + > +MODULE_LICENSE("GPL v2"); > +MODULE_AUTHOR("Dan Scally <djrscally@gmail.com>"); > +MODULE_DESCRIPTION("ACPI Driver for Discrete type INT3472 ACPI Devices"); > diff --git a/drivers/media/pci/intel/ipu3/int3472.h b/drivers/media/pci/intel/ipu3/int3472.h > new file mode 100644 > index 000000000000..6964726e8e1f > --- /dev/null > +++ b/drivers/media/pci/intel/ipu3/int3472.h > @@ -0,0 +1,96 @@ > +/* SPDX-License-Identifier: GPL-2.0 */ > +/* Author: Dan Scally <djrscally@gmail.com> */ > +#include <linux/regulator/machine.h> > + > +#define INT3472_MAX_SENSOR_GPIOS 3 > +#define GPIO_REGULATOR_NAME_LENGTH 17 > +#define GPIO_REGULATOR_SUPPLY_NAME_LENGTH 9 > + > +#define INT3472_REGULATOR(_NAME, _SUPPLY, _ID, _OPS) \ > + ((const struct regulator_desc) { \ > + .name = _NAME, \ > + .supply_name = _SUPPLY, \ > + .id = _ID, \ > + .type = REGULATOR_VOLTAGE, \ > + .ops = _OPS, \ > + .owner = THIS_MODULE, \ > + }) > + > +const guid_t int3472_gpio_guid = GUID_INIT(0x79234640, 0x9e10, 0x4fea, > + 0xa5, 0xc1, 0xb5, 0xaa, 0x8b, > + 0x19, 0x75, 0x6f); > + > +const guid_t cio2_sensor_module_guid = GUID_INIT(0x822ace8f, 0x2814, 0x4174, > + 0xa5, 0x6b, 0x5f, 0x02, 0x9f, > + 0xe0, 0x79, 0xee); > + > +struct int3472_cldb { > + u8 version; > + /* > + * control logic type > + * 0: UNKNOWN > + * 1: DISCRETE(CRD-D) > + * 2: PMIC TPS68470 > + * 3: PMIC uP6641 > + */ > + u8 control_logic_type; > + u8 control_logic_id; > + u8 sensor_card_sku; > + u8 reserved[28]; > +}; > + > +struct int3472_device { > + struct acpi_device *adev; > + struct acpi_device *sensor; > + > + unsigned int n_gpios; /* how many GPIOs have we seen */ > + > + unsigned int n_regulators; > + struct list_head regulators; > + > + unsigned int n_sensor_gpios; /* how many have we mapped to sensor */ > + struct gpiod_lookup_table gpios; > +}; > + > +struct int3472_gpio_regulator { > + char regulator_name[GPIO_REGULATOR_NAME_LENGTH]; > + char supply_name[GPIO_REGULATOR_SUPPLY_NAME_LENGTH]; > + struct gpio_desc *gpio; > + struct regulator_dev *rdev; > + struct regulator_desc rdesc; > + struct list_head list; > +}; > + > +struct int3472_sensor_regulator_map { > + char *sensor_module_name; > + unsigned int n_supplies; > + struct regulator_consumer_supply *supplies; > +}; > + I think the data tables below can be moved to the .c file, and also marked as static const. > +/* > + * Here follows platform specific mapping information that we can pass to > + * regulator_init_data when we register our regulators. They're just mapped > + * via index, I.E. the first regulator pin that the code finds for the > + * i2c-OVTI2680:00 device is avdd, the second is dovdd and so on. > + */ > + > +static struct regulator_consumer_supply miix_510_ov2680[] = { > + { "i2c-OVTI2680:00", "avdd" }, > + { "i2c-OVTI2680:00", "dovdd" }, > +}; > + > +static struct regulator_consumer_supply surface_go2_ov5693[] = { > + { "i2c-INT33BE:00", "avdd" }, > + { "i2c-INT33BE:00", "dovdd" }, > +}; > + > +static struct regulator_consumer_supply surface_book_ov5693[] = { > + { "i2c-INT33BE:00", "avdd" }, > + { "i2c-INT33BE:00", "dovdd" }, > +}; Should we de-duplicate repeated identical entries? for instance, if we 'know' the surface range all uses the same configuration, or at least a reduced set of configuration we could point to a single definition for the each set, rather than a specific one for each device perhaps? -- Kieran > + > +static struct int3472_sensor_regulator_map int3472_sensor_regulator_maps[] = { > + { "GNDF140809R", 2, miix_510_ov2680 }, > + { "YHCU", 2, surface_go2_ov5693 }, > + { "MSHW0070", 2, surface_book_ov5693 }, > +}; >
On Mon, Nov 30, 2020 at 01:31:12PM +0000, Daniel Scally wrote: > Some types of fwnode_handle do not implement the device_is_available() > check, such as those created by software_nodes. There isn't really a > meaningful way to check for the availability of a device that doesn't > actually exist, so if the check isn't implemented just assume that the > "device" is present. Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > Suggested-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > Signed-off-by: Daniel Scally <djrscally@gmail.com> > --- > Changes since RFC v3: > > patch introduced > > drivers/base/property.c | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/drivers/base/property.c b/drivers/base/property.c > index 4c43d30145c6..a5ca2306796f 100644 > --- a/drivers/base/property.c > +++ b/drivers/base/property.c > @@ -785,9 +785,14 @@ EXPORT_SYMBOL_GPL(fwnode_handle_put); > /** > * fwnode_device_is_available - check if a device is available for use > * @fwnode: Pointer to the fwnode of the device. > + * > + * For fwnode node types that don't implement the .device_is_available() > + * operation, this function returns true. > */ > bool fwnode_device_is_available(const struct fwnode_handle *fwnode) > { > + if (!fwnode_has_op(fwnode, device_is_available)) > + return true; > return fwnode_call_bool_op(fwnode, device_is_available); > } > EXPORT_SYMBOL_GPL(fwnode_device_is_available); > -- > 2.25.1 >
On Mon, Nov 30, 2020 at 01:31:13PM +0000, Daniel Scally wrote: > This function is used to find fwnode endpoints against a device. In > some instances those endpoints are software nodes which are children of > fwnode->secondary. Add support to fwnode_graph_get_endpoint_by_id() to > find those endpoints by recursively calling itself passing the ptr to > fwnode->secondary in the event no endpoint is found for the primary. One nit below, after addressing: Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> ... > + if (!best_ep && fwnode && !IS_ERR_OR_NULL(fwnode->secondary)) > + return fwnode_graph_get_endpoint_by_id(fwnode->secondary, port, > + endpoint, flags); > return best_ep; Can we, please, do if (best_ep) return best_ep; if (fwnode && !IS_ERR_OR_NULL(fwnode->secondary)) return fwnode_graph_get_endpoint_by_id(fwnode->secondary, port, endpoint, flags); return NULL; ? This 'if (fwnode && !IS_ERR_OR_NULL(fwnode->secondary))' becomes kinda idiomatic to the cases when we need to proceed primary followed by the secondary in cases where it's not already done. > } > EXPORT_SYMBOL_GPL(fwnode_graph_get_endpoint_by_id);
On Mon, Nov 30, 2020 at 01:31:16PM +0000, Daniel Scally wrote: > Software nodes that are children of another software node should be > unregistered before their parent. To allow easy unregistering of an array > of software_nodes ordered parent to child, reverse the order in which > this function unregisters software_nodes. Should be folded in the previous patch. Otherwise we will have a history point where register() behaves differently to unregister(). ... > + * @nodes: Zero terminated array of software nodes to be unregistered. If > + * parent pointers are set up in any of the software nodes then the array > + * MUST be ordered such that parents come before their children. Please, leave field description short. Rather add another note to the Description below. > * > * Unregister multiple software nodes at once. > * > - * NOTE: Be careful using this call if the nodes had parent pointers set up in > - * them before registering. If so, it is wiser to remove the nodes > - * individually, in the correct order (child before parent) instead of relying > - * on the sequential order of the list of nodes in the array. > + * NOTE: If you are uncertain whether the array is ordered such that > + * parents will be unregistered before their children, it is wiser to > + * remove the nodes individually, in the correct order (child before > + * parent). > */
On Mon, Nov 30, 2020 at 07:28:57PM +0200, Laurent Pinchart wrote: > On Mon, Nov 30, 2020 at 07:29:00PM +0200, Andy Shevchenko wrote: > > On Mon, Nov 30, 2020 at 01:31:13PM +0000, Daniel Scally wrote: ... > > > + if (!best_ep && fwnode && !IS_ERR_OR_NULL(fwnode->secondary)) > > > + return fwnode_graph_get_endpoint_by_id(fwnode->secondary, port, > > > + endpoint, flags); > > > > > return best_ep; > > > > Can we, please, do > > > > if (best_ep) > > return best_ep; > > > > if (fwnode && !IS_ERR_OR_NULL(fwnode->secondary)) > > return fwnode_graph_get_endpoint_by_id(fwnode->secondary, port, > > endpoint, flags); > > > > return NULL; > > > > ? > > > > This 'if (fwnode && !IS_ERR_OR_NULL(fwnode->secondary))' becomes kinda > > idiomatic to the cases when we need to proceed primary followed by the > > secondary in cases where it's not already done. > > We could also move the !fwnode check to the beginning of the function. It's already there (1). What did I miss? 1) via fwnode_graph_get_next_endpoint() -> fwnode_call_ptr_op()
On Mon, Nov 30, 2020 at 01:31:28PM +0000, Daniel Scally wrote: > I need to be able to translate GPIO resources in an acpi_device's _CRS > into gpio_descs. Those are represented in _CRS as a pathname to a GPIO > device plus the pin's index number: this function is perfect for that > purpose. Destiny of this patch depends on the review of the next one.
On Mon, Nov 30, 2020 at 01:31:29PM +0000, Daniel Scally wrote: > On platforms where ACPI is designed for use with Windows, resources > that are intended to be consumed by sensor devices are sometimes in > the _CRS of a dummy INT3472 device upon which the sensor depends. This > driver binds to the dummy acpi device (which does not represent a acpi device -> acpi_device > physical PMIC) and maps them into GPIO lines and regulators for use by > the sensor device instead. ... > This patch contains the bits of this process that we're least sure about. > The sensors in scope for this work are called out as dependent (in their > DSDT entry's _DEP) on a device with _HID INT3472. These come in at least > 2 kinds; those with an I2cSerialBusV2 entry (which we presume therefore > are legitimate tps68470 PMICs that need handling by those drivers - work > on that in the future). And those without an I2C device. For those without > an I2C device they instead have an array of GPIO pins defined in _CRS. So > for example, my Lenovo Miix 510's OVTI2680 sensor is dependent on one of > the _latter_ kind of INT3472 devices, with this _CRS: > > Method (_CRS, 0, NotSerialized) // _CRS: Current Resource Settings > { > Name (SBUF, ResourceTemplate () > { > GpioIo (Exclusive, PullDefault, 0x0000, 0x0000, > IoRestrictionOutputOnly, "\\_SB.PCI0.GPI0", > 0x00, ResourceConsumer, , > ) > { // Pin list > 0x0079 > } > GpioIo (Exclusive, PullDefault, 0x0000, 0x0000, > IoRestrictionOutputOnly, "\\_SB.PCI0.GPI0", > 0x00, ResourceConsumer, , > ) > { // Pin list > 0x007A > } > GpioIo (Exclusive, PullDefault, 0x0000, 0x0000, > IoRestrictionOutputOnly, "\\_SB.PCI0.GPI0", > 0x00, ResourceConsumer, , > ) > { // Pin list > 0x008F > } > }) > Return (SBUF) /* \_SB_.PCI0.PMI1._CRS.SBUF */ > } > > and the same device has a _DSM Method, which returns 32-bit ints where > the second lowest byte we noticed to match the pin numbers of the GPIO > lines: > > Method (_DSM, 4, NotSerialized) // _DSM: Device-Specific Method > { > If ((Arg0 == ToUUID ("79234640-9e10-4fea-a5c1-b5aa8b19756f"))) > { > If ((Arg2 == One)) > { > Return (0x03) > } > > If ((Arg2 == 0x02)) > { > Return (0x01007900) > } > > If ((Arg2 == 0x03)) > { > Return (0x01007A0C) > } > > If ((Arg2 == 0x04)) > { > Return (0x01008F01) > } > } > > Return (Zero) > } > > We know that at least some of those pins have to be toggled active for the > sensor devices to be available in i2c, so the conclusion we came to was > that those GPIO entries assigned to the INT3472 device actually represent > GPIOs and regulators to be consumed by the sensors themselves. Tsuchiya > noticed that the lowest byte in the return values of the _DSM method > seemed to represent the type or function of the GPIO line, and we > confirmed that by testing on each surface device that GPIO lines where the > low byte in the _DSM entry for that pin was 0x0d controlled the privacy > LED of the cameras. > > We're guessing as to the exact meaning of the function byte, but I > conclude they're something like this: > > 0x00 - probably a reset GPIO > 0x01 - regulator for the sensor > 0x0c - regulator for the sensor > 0x0b - regulator again, but for a VCM or EEPROM > 0x0d - privacy led (only one we're totally confident of since we can see > it happen!) It's solely Windows driver design... Luckily I found some information and can clarify above table: 0x00 Reset 0x01 Power down 0x0b Power enable 0x0c Clock enable 0x0d LED (active high) The above text perhaps should go somewhere under Documentation. > After much internal debate I decided to write this as a standalone > acpi_driver. Alternative options we considered: > > 1. Squash all this into the cio2-bridge code, which I did originally write > but decided I didn't like. > 2. Extend the existing tps68470 mfd driver...they share an ACPI ID so this > kinda makes sense, but ultimately given there is no actual physical > tps68470 in the scenario this patch handles I decided I didn't like this > either. Looking to this I think the best is to create a module that can be consumed by tps68470 and separately. So, something near to it rather than under ipu3 hood. You may use same ID's in both drivers (in PMIC less case it can be simple platform and thus they won't conflict), but both of them should provide GPIO resources for consumption. So, something like tps68470.h with API to consume split tps68470 to -core, -i2c parts add int3472, which will serve for above and be standalone platform driver update cio2-bridge accordingly Would it be feasible? ... > + table_entry = (struct gpiod_lookup)GPIO_LOOKUP_IDX(acpi_dev_name(adev), > + ares->data.gpio.pin_table[0], > + func, 0, GPIO_ACTIVE_HIGH); You won't need this if you have regular INT3472 platform driver. Simple call there _DSM to map resources to the type and use devm_gpiod_get on consumer behalf. Thus, previous patch is not needed. ... > + case 0x01: /* Power regulators (we think) */ > + case 0x0c: > + case 0x0b: /* Power regulators, but to a device separate to sensor */ > + case 0x0d: /* Indicator LEDs */ Give names to those constants. #define INT3472_GPIO_TYPE_RESET 0x00 ... > +static struct acpi_driver int3472_driver = { No acpi_driver! Use platform_driver instead with plenty of examples all over the kernel. > + .name = "int3472", > + .ids = int3472_device_id, > + .ops = { > + .add = int3472_add, > + .remove = int3472_remove, > + }, > + .owner = THIS_MODULE, No need > +}; ... > +const guid_t int3472_gpio_guid = GUID_INIT(0x79234640, 0x9e10, 0x4fea, > + 0xa5, 0xc1, 0xb5, 0xaa, 0x8b, > + 0x19, 0x75, 0x6f); > + > +const guid_t cio2_sensor_module_guid = GUID_INIT(0x822ace8f, 0x2814, 0x4174, > + 0xa5, 0x6b, 0x5f, 0x02, 0x9f, > + 0xe0, 0x79, 0xee); Use more or less standard pattern for these, like /* 79234640-9e10-4fea-a5c1b5aa8b19756f */ const guid_t int3472_gpio_guid = GUID_INIT(0x79234640, 0x9e10, 0x4fea, 0xa5, 0xc1, 0xb5, 0xaa, 0x8b, 0x19, 0x75, 0x6f); ... > +static struct regulator_consumer_supply miix_510_ov2680[] = { > + { "i2c-OVTI2680:00", "avdd" }, > + { "i2c-OVTI2680:00", "dovdd" }, > +}; Can we use acpi_dev_first_match_dev() to get instance name out of their HIDs? > +static struct regulator_consumer_supply surface_go2_ov5693[] = { > + { "i2c-INT33BE:00", "avdd" }, > + { "i2c-INT33BE:00", "dovdd" }, > +}; > + > +static struct regulator_consumer_supply surface_book_ov5693[] = { > + { "i2c-INT33BE:00", "avdd" }, > + { "i2c-INT33BE:00", "dovdd" }, > +}; Ditto. ... > +static struct int3472_sensor_regulator_map int3472_sensor_regulator_maps[] = { > + { "GNDF140809R", 2, miix_510_ov2680 }, > + { "YHCU", 2, surface_go2_ov5693 }, > + { "MSHW0070", 2, surface_book_ov5693 }, > +}; Hmm... Usual way is to use DMI for that. I'm not sure above will not give us false positive matches.
Hi Sakari On 30/11/2020 20:52, Sakari Ailus wrote: >> +static const struct acpi_device_id int3472_device_id[] = { >> + { "INT3472", 0 }, > The INT3472 _HID is really allocated for the tps68470 PMIC chip. It may not > be used by other drivers; people will want to build kernels where both of > these ACPI table layouts are functional. > > Instead, I propose, that you add this as an option to the tps68470 driver > that figures out whether the ACPI device for the tps68470 device actually > describes something else, in a similar fashion you do with the cio2-bridge > driver. I think it may need a separate Kconfig option albeit this and > cio2-bridge cannot be used separately. It actually occurs to me that that may not work (I know I called that out as an option we considered, but that was a while ago actually). The reason I wasn't worried about the existing tps68470 driver binding to these devices is that it's an i2c driver, and these dummy devices don't have an I2cSerialBusV2, so no I2C device is created by them the kernel. Won't that mean the tps68470 driver won't ever be probed for these devices? > >> + { }, >> +}; >> +MODULE_DEVICE_TABLE(acpi, int3472_device_id); >> + >> +static struct acpi_driver int3472_driver = { >> + .name = "int3472", >> + .ids = int3472_device_id, >> + .ops = { >> + .add = int3472_add, >> + .remove = int3472_remove, >> + }, >> + .owner = THIS_MODULE, >> +}; >> + >> +module_acpi_driver(int3472_driver); >> + >> +MODULE_LICENSE("GPL v2"); >> +MODULE_AUTHOR("Dan Scally <djrscally@gmail.com>"); >> +MODULE_DESCRIPTION("ACPI Driver for Discrete type INT3472 ACPI Devices"); >> diff --git a/drivers/media/pci/intel/ipu3/int3472.h b/drivers/media/pci/intel/ipu3/int3472.h >> new file mode 100644 >> index 000000000000..6964726e8e1f >> --- /dev/null >> +++ b/drivers/media/pci/intel/ipu3/int3472.h >> @@ -0,0 +1,96 @@ >> +/* SPDX-License-Identifier: GPL-2.0 */ >> +/* Author: Dan Scally <djrscally@gmail.com> */ >> +#include <linux/regulator/machine.h> >> + >> +#define INT3472_MAX_SENSOR_GPIOS 3 >> +#define GPIO_REGULATOR_NAME_LENGTH 17 >> +#define GPIO_REGULATOR_SUPPLY_NAME_LENGTH 9 >> + >> +#define INT3472_REGULATOR(_NAME, _SUPPLY, _ID, _OPS) \ >> + ((const struct regulator_desc) { \ >> + .name = _NAME, \ >> + .supply_name = _SUPPLY, \ >> + .id = _ID, \ >> + .type = REGULATOR_VOLTAGE, \ >> + .ops = _OPS, \ >> + .owner = THIS_MODULE, \ >> + }) >> + >> +const guid_t int3472_gpio_guid = GUID_INIT(0x79234640, 0x9e10, 0x4fea, >> + 0xa5, 0xc1, 0xb5, 0xaa, 0x8b, >> + 0x19, 0x75, 0x6f); >> + >> +const guid_t cio2_sensor_module_guid = GUID_INIT(0x822ace8f, 0x2814, 0x4174, >> + 0xa5, 0x6b, 0x5f, 0x02, 0x9f, >> + 0xe0, 0x79, 0xee); >> + >> +struct int3472_cldb { >> + u8 version; >> + /* >> + * control logic type >> + * 0: UNKNOWN >> + * 1: DISCRETE(CRD-D) >> + * 2: PMIC TPS68470 >> + * 3: PMIC uP6641 >> + */ >> + u8 control_logic_type; >> + u8 control_logic_id; >> + u8 sensor_card_sku; >> + u8 reserved[28]; >> +}; >> + >> +struct int3472_device { >> + struct acpi_device *adev; >> + struct acpi_device *sensor; >> + >> + unsigned int n_gpios; /* how many GPIOs have we seen */ >> + >> + unsigned int n_regulators; >> + struct list_head regulators; >> + >> + unsigned int n_sensor_gpios; /* how many have we mapped to sensor */ >> + struct gpiod_lookup_table gpios; >> +}; >> + >> +struct int3472_gpio_regulator { >> + char regulator_name[GPIO_REGULATOR_NAME_LENGTH]; >> + char supply_name[GPIO_REGULATOR_SUPPLY_NAME_LENGTH]; >> + struct gpio_desc *gpio; >> + struct regulator_dev *rdev; >> + struct regulator_desc rdesc; >> + struct list_head list; >> +}; >> + >> +struct int3472_sensor_regulator_map { >> + char *sensor_module_name; >> + unsigned int n_supplies; >> + struct regulator_consumer_supply *supplies; >> +}; >> + >> +/* >> + * Here follows platform specific mapping information that we can pass to >> + * regulator_init_data when we register our regulators. They're just mapped >> + * via index, I.E. the first regulator pin that the code finds for the >> + * i2c-OVTI2680:00 device is avdd, the second is dovdd and so on. >> + */ >> + >> +static struct regulator_consumer_supply miix_510_ov2680[] = { >> + { "i2c-OVTI2680:00", "avdd" }, >> + { "i2c-OVTI2680:00", "dovdd" }, >> +}; >> + >> +static struct regulator_consumer_supply surface_go2_ov5693[] = { >> + { "i2c-INT33BE:00", "avdd" }, >> + { "i2c-INT33BE:00", "dovdd" }, >> +}; >> + >> +static struct regulator_consumer_supply surface_book_ov5693[] = { >> + { "i2c-INT33BE:00", "avdd" }, >> + { "i2c-INT33BE:00", "dovdd" }, >> +}; >> + >> +static struct int3472_sensor_regulator_map int3472_sensor_regulator_maps[] = { >> + { "GNDF140809R", 2, miix_510_ov2680 }, >> + { "YHCU", 2, surface_go2_ov5693 }, >> + { "MSHW0070", 2, surface_book_ov5693 }, >> +};
Hi Jean-Michel On 30/11/2020 16:17, Jean-Michel Hautbois wrote: >> We're guessing as to the exact meaning of the function byte, but I >> conclude they're something like this: >> >> 0x00 - probably a reset GPIO >> 0x01 - regulator for the sensor >> 0x0c - regulator for the sensor >> 0x0b - regulator again, but for a VCM or EEPROM > Is it possible that the ad5823 would be here, and controled by this bit ? That's one of the devices Laurent guessed might be there yes; when that GPIO line is toggled it causes an extra device to show on the i2c-bus, but the ACPI table doesn't define an I2CSerialBusV2 for it. Instead it's rolled under the sensor's entry, there's a second entry in _CRS for the sensor that matches the address of the new device: Method (_CRS, 0, NotSerialized) // _CRS: Current Resource Settings { Name (SBUF, ResourceTemplate () { I2cSerialBusV2 (0x0036, ControllerInitiated, 0x00061A80, AddressingMode7Bit, "\\_SB.PCI0.I2C2", 0x00, ResourceConsumer, , Exclusive, ) I2cSerialBusV2 (0x000C, ControllerInitiated, 0x00061A80, AddressingMode7Bit, "\\_SB.PCI0.I2C2", 0x00, ResourceConsumer, , Exclusive, ) }) Return (SBUF) /* \_SB_.PCI0.CAM0._CRS.SBUF */ } So that's another thing we need to work on. At the moment it doesn't exist as far as the kernel is concerned. >> 0x0d - privacy led (only one we're totally confident of since we can see >> it happen!) >> >> After much internal debate I decided to write this as a standalone >> acpi_driver. Alternative options we considered: >> >> 1. Squash all this into the cio2-bridge code, which I did originally write >> but decided I didn't like. >> 2. Extend the existing tps68470 mfd driver...they share an ACPI ID so this >> kinda makes sense, but ultimately given there is no actual physical >> tps68470 in the scenario this patch handles I decided I didn't like this >> either. >> >> MAINTAINERS | 7 + >> drivers/media/pci/intel/ipu3/Kconfig | 14 + >> drivers/media/pci/intel/ipu3/Makefile | 1 + >> drivers/media/pci/intel/ipu3/int3472.c | 381 +++++++++++++++++++++++++ >> drivers/media/pci/intel/ipu3/int3472.h | 96 +++++++ >> 5 files changed, 499 insertions(+) >> create mode 100644 drivers/media/pci/intel/ipu3/int3472.c >> create mode 100644 drivers/media/pci/intel/ipu3/int3472.h >> >> diff --git a/MAINTAINERS b/MAINTAINERS >> index 188559a0a610..d73471f9c2a3 100644 >> --- a/MAINTAINERS >> +++ b/MAINTAINERS >> @@ -8753,6 +8753,13 @@ L: linux-crypto@vger.kernel.org >> S: Maintained >> F: drivers/crypto/inside-secure/ >> >> +INT3472 ACPI DEVICE DRIVER >> +M: Daniel Scally <djrscally@gmail.com> >> +L: linux-media@vger.kernel.org >> +S: Maintained >> +F: drivers/media/pci/intel/ipu3/int3472.c >> +F: drivers/media/pci/intel/ipu3/int3472.h >> + >> INTEGRITY MEASUREMENT ARCHITECTURE (IMA) >> M: Mimi Zohar <zohar@linux.ibm.com> >> M: Dmitry Kasatkin <dmitry.kasatkin@gmail.com> >> diff --git a/drivers/media/pci/intel/ipu3/Kconfig b/drivers/media/pci/intel/ipu3/Kconfig >> index 2b3350d042be..9dd3b280f821 100644 >> --- a/drivers/media/pci/intel/ipu3/Kconfig >> +++ b/drivers/media/pci/intel/ipu3/Kconfig >> @@ -34,3 +34,17 @@ config CIO2_BRIDGE >> - Dell 7285 >> >> If in doubt, say N here. >> + >> +config INT3472 >> + tristate "INT3472 Dummy ACPI Device Driver" >> + depends on VIDEO_IPU3_CIO2 >> + depends on ACPI && REGULATOR && GPIOLIB >> + help >> + This module provides an ACPI driver for INT3472 devices that do not >> + represent an actual physical tps68470 device. >> + >> + Say Y here if your device is a detachable / hybrid laptop that comes >> + with Windows installed by the OEM. >> + The module will be called int3472. >> + >> + If in doubt, say N here. > Is there any issue if the tps68470 driver is also selected and probed ? > >> diff --git a/drivers/media/pci/intel/ipu3/Makefile b/drivers/media/pci/intel/ipu3/Makefile >> index 933777e6ea8a..2285947b2bd2 100644 >> --- a/drivers/media/pci/intel/ipu3/Makefile >> +++ b/drivers/media/pci/intel/ipu3/Makefile >> @@ -1,5 +1,6 @@ >> # SPDX-License-Identifier: GPL-2.0-only >> obj-$(CONFIG_VIDEO_IPU3_CIO2) += ipu3-cio2.o >> +obj-$(CONFIG_INT3472) += int3472.o >> >> ipu3-cio2-y += ipu3-cio2-main.o >> ipu3-cio2-$(CONFIG_CIO2_BRIDGE) += cio2-bridge.o >> diff --git a/drivers/media/pci/intel/ipu3/int3472.c b/drivers/media/pci/intel/ipu3/int3472.c >> new file mode 100644 >> index 000000000000..6b0be75f7f35 >> --- /dev/null >> +++ b/drivers/media/pci/intel/ipu3/int3472.c >> @@ -0,0 +1,381 @@ >> +// SPDX-License-Identifier: GPL-2.0 >> +/* Author: Dan Scally <djrscally@gmail.com> */ >> +#include <linux/acpi.h> >> +#include <linux/gpio/consumer.h> >> +#include <linux/gpio/machine.h> >> +#include <linux/i2c.h> >> +#include <linux/kernel.h> >> +#include <linux/list.h> >> +#include <linux/module.h> >> +#include <linux/regulator/driver.h> >> + >> +#include "int3472.h" >> + >> +/* >> + * The regulators have to have .ops to be valid, but the only ops we actually >> + * support are .enable and .disable which are handled via .ena_gpiod. Pass an >> + * empty struct to clear the check without lying about capabilities. >> + */ >> +static const struct regulator_ops int3472_gpio_regulator_ops = { 0 }; >> + >> +static int int3472_map_gpio_to_sensor(struct int3472_device *int3472, >> + struct acpi_resource *ares, char *func) >> +{ >> + char *path = ares->data.gpio.resource_source.string_ptr; >> + struct gpiod_lookup table_entry; >> + struct acpi_device *adev; >> + acpi_handle handle; >> + acpi_status status; >> + int ret; >> + >> + /* Make sure we don't overflow, and leave room for a terminator */ >> + if (int3472->n_sensor_gpios >= INT3472_MAX_SENSOR_GPIOS) { >> + dev_warn(&int3472->sensor->dev, "Too many GPIOs mapped\n"); >> + return -EINVAL; >> + } >> + >> + /* Fetch ACPI handle for the GPIO chip */ >> + status = acpi_get_handle(NULL, path, &handle); >> + if (ACPI_FAILURE(status)) >> + return -EINVAL; >> + >> + ret = acpi_bus_get_device(handle, &adev); >> + if (ret) >> + return -ENODEV; >> + >> + table_entry = (struct gpiod_lookup)GPIO_LOOKUP_IDX(acpi_dev_name(adev), >> + ares->data.gpio.pin_table[0], >> + func, 0, GPIO_ACTIVE_HIGH); >> + >> + memcpy(&int3472->gpios.table[int3472->n_sensor_gpios], &table_entry, >> + sizeof(table_entry)); >> + int3472->n_sensor_gpios++; >> + >> + return 0; >> +} >> + >> +static struct int3472_sensor_regulator_map * >> +int3472_get_sensor_supply_map(struct int3472_device *int3472) >> +{ >> + struct int3472_sensor_regulator_map *ret; >> + union acpi_object *obj; >> + unsigned int i; >> + >> + /* >> + * Sensor modules seem to be identified by a unique string. We use that >> + * to make sure we pass the right device and supply names to the new >> + * regulator's consumer_supplies >> + */ >> + obj = acpi_evaluate_dsm_typed(int3472->sensor->handle, >> + &cio2_sensor_module_guid, 0x00, >> + 0x01, NULL, ACPI_TYPE_STRING); >> + >> + if (!obj) { >> + dev_err(&int3472->sensor->dev, >> + "Failed to get sensor module string from _DSM\n"); >> + return ERR_PTR(-ENODEV); >> + } >> + >> + if (obj->string.type != ACPI_TYPE_STRING) { >> + dev_err(&int3472->sensor->dev, >> + "Sensor _DSM returned a non-string value\n"); >> + ret = ERR_PTR(-EINVAL); >> + goto out_free_obj; >> + } >> + >> + ret = ERR_PTR(-ENODEV); >> + for (i = 0; i < ARRAY_SIZE(int3472_sensor_regulator_maps); i++) { >> + if (!strcmp(int3472_sensor_regulator_maps[i].sensor_module_name, >> + obj->string.pointer)) { >> + ret = &int3472_sensor_regulator_maps[i]; >> + goto out_free_obj; >> + } >> + } >> + >> +out_free_obj: >> + ACPI_FREE(obj); >> + return ret; >> +} >> + >> +static int int3472_register_regulator(struct int3472_device *int3472, >> + struct acpi_resource *ares) >> +{ >> + char *path = ares->data.gpio.resource_source.string_ptr; >> + struct int3472_sensor_regulator_map *regulator_map; >> + struct regulator_init_data init_data = { }; >> + struct int3472_gpio_regulator *regulator; >> + struct regulator_config cfg = { }; >> + int ret; >> + >> + /* >> + * We lookup supply names from machine specific tables, based on a >> + * unique identifier in the sensor's _DSM >> + */ >> + regulator_map = int3472_get_sensor_supply_map(int3472); >> + if (IS_ERR_OR_NULL(regulator_map)) { >> + dev_err(&int3472->sensor->dev, >> + "Found no supplies defined for this sensor\n"); >> + return PTR_ERR(regulator_map); >> + } >> + >> + if (int3472->n_regulators >= regulator_map->n_supplies) { >> + dev_err(&int3472->sensor->dev, >> + "All known supplies are already mapped\n"); >> + return -EINVAL; >> + } >> + >> + init_data.supply_regulator = NULL; >> + init_data.constraints.valid_ops_mask = REGULATOR_CHANGE_STATUS; >> + init_data.num_consumer_supplies = 1; >> + init_data.consumer_supplies = ®ulator_map->supplies[int3472->n_regulators]; >> + >> + regulator = kmalloc(sizeof(*regulator), GFP_KERNEL); >> + if (!regulator) >> + return -ENOMEM; >> + >> + snprintf(regulator->regulator_name, GPIO_REGULATOR_NAME_LENGTH, >> + "gpio-regulator-%d", int3472->n_regulators); >> + snprintf(regulator->supply_name, GPIO_REGULATOR_SUPPLY_NAME_LENGTH, >> + "supply-%d", int3472->n_regulators); >> + >> + regulator->rdesc = INT3472_REGULATOR(regulator->regulator_name, >> + regulator->supply_name, >> + int3472->n_regulators, >> + &int3472_gpio_regulator_ops); >> + >> + regulator->gpio = acpi_get_gpiod(path, ares->data.gpio.pin_table[0]); >> + if (IS_ERR(regulator->gpio)) { >> + ret = PTR_ERR(regulator->gpio); >> + goto err_free_regulator; >> + } >> + >> + cfg.dev = &int3472->adev->dev; >> + cfg.init_data = &init_data; >> + cfg.ena_gpiod = regulator->gpio; >> + >> + regulator->rdev = regulator_register(®ulator->rdesc, &cfg); >> + if (IS_ERR(regulator->rdev)) { >> + ret = PTR_ERR(regulator->rdev); >> + goto err_free_gpio; >> + } >> + >> + list_add(®ulator->list, &int3472->regulators); >> + int3472->n_regulators++; >> + >> + return 0; >> + >> +err_free_gpio: >> + gpiod_put(regulator->gpio); >> +err_free_regulator: >> + kfree(regulator); >> + >> + return ret; >> +} >> + >> +static int int3472_handle_gpio_resources(struct acpi_resource *ares, >> + void *data) >> +{ >> + struct int3472_device *int3472 = data; >> + union acpi_object *obj; >> + int ret = 0; >> + >> + if (ares->type != ACPI_RESOURCE_TYPE_GPIO || >> + ares->data.gpio.connection_type != ACPI_RESOURCE_GPIO_TYPE_IO) >> + return EINVAL; /* Deliberately positive */ >> + >> + /* >> + * n_gpios + 2 because the index of this _DSM function is 1-based and >> + * the first function is just a count. >> + */ >> + obj = acpi_evaluate_dsm_typed(int3472->adev->handle, >> + &int3472_gpio_guid, 0x00, >> + int3472->n_gpios + 2, >> + NULL, ACPI_TYPE_INTEGER); >> + >> + if (!obj) { >> + dev_warn(&int3472->adev->dev, >> + "No _DSM entry for this GPIO pin\n"); >> + return ENODEV; >> + } >> + >> + switch (obj->integer.value & 0xff) { /* low byte holds type data */ >> + case 0x00: /* Purpose unclear, possibly a reset GPIO pin */ >> + ret = int3472_map_gpio_to_sensor(int3472, ares, "reset"); >> + if (ret) >> + dev_warn(&int3472->adev->dev, >> + "Failed to map reset pin to sensor\n"); >> + >> + break; >> + case 0x01: /* Power regulators (we think) */ >> + case 0x0c: >> + ret = int3472_register_regulator(int3472, ares); >> + if (ret) >> + dev_warn(&int3472->adev->dev, >> + "Failed to map regulator to sensor\n"); >> + >> + break; >> + case 0x0b: /* Power regulators, but to a device separate to sensor */ >> + ret = int3472_register_regulator(int3472, ares); >> + if (ret) >> + dev_warn(&int3472->adev->dev, >> + "Failed to map regulator to sensor\n"); >> + >> + break; >> + case 0x0d: /* Indicator LEDs */ >> + ret = int3472_map_gpio_to_sensor(int3472, ares, "indicator-led"); >> + if (ret) >> + dev_warn(&int3472->adev->dev, >> + "Failed to map indicator led to sensor\n"); >> + >> + break; >> + default: >> + /* if we've gotten here, we're not sure what they are yet */ >> + dev_warn(&int3472->adev->dev, >> + "GPIO type 0x%llx unknown; the sensor may not work\n", >> + (obj->integer.value & 0xff)); >> + ret = EINVAL; >> + } >> + >> + int3472->n_gpios++; >> + ACPI_FREE(obj); >> + return abs(ret); >> +} >> + >> +static void int3472_parse_crs(struct int3472_device *int3472) >> +{ >> + struct list_head resource_list; >> + >> + INIT_LIST_HEAD(&resource_list); >> + >> + acpi_dev_get_resources(int3472->adev, &resource_list, >> + int3472_handle_gpio_resources, int3472); >> + >> + acpi_dev_free_resource_list(&resource_list); >> + gpiod_add_lookup_table(&int3472->gpios); >> +} >> + >> +static int int3472_add(struct acpi_device *adev) >> +{ >> + struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL }; >> + struct int3472_device *int3472; >> + struct int3472_cldb cldb; >> + union acpi_object *obj; >> + acpi_status status; >> + int ret = 0; >> + >> + /* >> + * This driver is only intended to support "dummy" INT3472 devices >> + * which appear in ACPI designed for Windows. These are distinguishable >> + * from INT3472 entries representing an actual tps68470 PMIC through >> + * the presence of a CLDB buffer with a particular value set. >> + */ >> + status = acpi_evaluate_object(adev->handle, "CLDB", NULL, &buffer); >> + if (ACPI_FAILURE(status)) >> + return -ENODEV; >> + >> + obj = buffer.pointer; >> + if (!obj) { >> + dev_err(&adev->dev, "ACPI device has no CLDB object\n"); >> + return -ENODEV; >> + } >> + >> + if (obj->type != ACPI_TYPE_BUFFER) { >> + dev_err(&adev->dev, "CLDB object is not an ACPI buffer\n"); >> + ret = -EINVAL; >> + goto out_free_buff; >> + } >> + >> + if (obj->buffer.length > sizeof(cldb)) { >> + dev_err(&adev->dev, "The CLDB buffer is too large\n"); >> + ret = -EINVAL; >> + goto out_free_buff; >> + } >> + >> + memcpy(&cldb, obj->buffer.pointer, obj->buffer.length); >> + >> + /* >> + * control_logic_type = 1 indicates this is a dummy INT3472 device of >> + * the kind we're looking for. If any other value then we shouldn't try >> + * to handle it >> + */ >> + if (cldb.control_logic_type != 1) { >> + ret = -EINVAL; >> + goto out_free_buff; >> + } >> + >> + /* Space for 4 GPIOs - one more than we've seen so far plus a null */ >> + int3472 = kzalloc(sizeof(*int3472) + >> + ((INT3472_MAX_SENSOR_GPIOS + 1) * sizeof(struct gpiod_lookup)), >> + GFP_KERNEL); >> + if (!int3472) { >> + ret = -ENOMEM; >> + goto out_free_buff; >> + } >> + >> + int3472->adev = adev; >> + adev->driver_data = int3472; >> + >> + int3472->sensor = acpi_dev_get_next_dep_dev(adev, NULL); >> + if (!int3472->sensor) { >> + dev_err(&adev->dev, >> + "This INT3472 entry seems to have no dependents.\n"); >> + ret = -ENODEV; >> + goto out_free_int3472; >> + } >> + >> + int3472->gpios.dev_id = i2c_acpi_dev_name(int3472->sensor); >> + >> + INIT_LIST_HEAD(&int3472->regulators); >> + >> + int3472_parse_crs(int3472); >> + >> + goto out_free_buff; >> + >> +out_free_int3472: >> + kfree(int3472); >> +out_free_buff: >> + kfree(buffer.pointer); >> + return ret; >> +} >> + >> +static int int3472_remove(struct acpi_device *adev) >> +{ >> + struct int3472_gpio_regulator *reg; >> + struct int3472_device *int3472; >> + >> + int3472 = acpi_driver_data(adev); >> + >> + acpi_dev_put(int3472->sensor); >> + gpiod_remove_lookup_table(&int3472->gpios); >> + >> + list_for_each_entry(reg, &int3472->regulators, list) { >> + gpiod_put(reg->gpio); >> + regulator_unregister(reg->rdev); >> + } >> + >> + kfree(int3472); >> + >> + return 0; >> +} >> + >> +static const struct acpi_device_id int3472_device_id[] = { >> + { "INT3472", 0 }, >> + { }, >> +}; >> +MODULE_DEVICE_TABLE(acpi, int3472_device_id); >> + >> +static struct acpi_driver int3472_driver = { >> + .name = "int3472", >> + .ids = int3472_device_id, >> + .ops = { >> + .add = int3472_add, >> + .remove = int3472_remove, >> + }, >> + .owner = THIS_MODULE, >> +}; >> + >> +module_acpi_driver(int3472_driver); >> + >> +MODULE_LICENSE("GPL v2"); >> +MODULE_AUTHOR("Dan Scally <djrscally@gmail.com>"); >> +MODULE_DESCRIPTION("ACPI Driver for Discrete type INT3472 ACPI Devices"); >> diff --git a/drivers/media/pci/intel/ipu3/int3472.h b/drivers/media/pci/intel/ipu3/int3472.h >> new file mode 100644 >> index 000000000000..6964726e8e1f >> --- /dev/null >> +++ b/drivers/media/pci/intel/ipu3/int3472.h >> @@ -0,0 +1,96 @@ >> +/* SPDX-License-Identifier: GPL-2.0 */ >> +/* Author: Dan Scally <djrscally@gmail.com> */ >> +#include <linux/regulator/machine.h> >> + >> +#define INT3472_MAX_SENSOR_GPIOS 3 >> +#define GPIO_REGULATOR_NAME_LENGTH 17 >> +#define GPIO_REGULATOR_SUPPLY_NAME_LENGTH 9 >> + >> +#define INT3472_REGULATOR(_NAME, _SUPPLY, _ID, _OPS) \ >> + ((const struct regulator_desc) { \ >> + .name = _NAME, \ >> + .supply_name = _SUPPLY, \ >> + .id = _ID, \ >> + .type = REGULATOR_VOLTAGE, \ >> + .ops = _OPS, \ >> + .owner = THIS_MODULE, \ >> + }) >> + >> +const guid_t int3472_gpio_guid = GUID_INIT(0x79234640, 0x9e10, 0x4fea, >> + 0xa5, 0xc1, 0xb5, 0xaa, 0x8b, >> + 0x19, 0x75, 0x6f); >> + >> +const guid_t cio2_sensor_module_guid = GUID_INIT(0x822ace8f, 0x2814, 0x4174, >> + 0xa5, 0x6b, 0x5f, 0x02, 0x9f, >> + 0xe0, 0x79, 0xee); >> + >> +struct int3472_cldb { >> + u8 version; >> + /* >> + * control logic type >> + * 0: UNKNOWN >> + * 1: DISCRETE(CRD-D) >> + * 2: PMIC TPS68470 >> + * 3: PMIC uP6641 >> + */ >> + u8 control_logic_type; >> + u8 control_logic_id; >> + u8 sensor_card_sku; >> + u8 reserved[28]; >> +}; >> + >> +struct int3472_device { >> + struct acpi_device *adev; >> + struct acpi_device *sensor; >> + >> + unsigned int n_gpios; /* how many GPIOs have we seen */ >> + >> + unsigned int n_regulators; >> + struct list_head regulators; >> + >> + unsigned int n_sensor_gpios; /* how many have we mapped to sensor */ >> + struct gpiod_lookup_table gpios; >> +}; >> + >> +struct int3472_gpio_regulator { >> + char regulator_name[GPIO_REGULATOR_NAME_LENGTH]; >> + char supply_name[GPIO_REGULATOR_SUPPLY_NAME_LENGTH]; >> + struct gpio_desc *gpio; >> + struct regulator_dev *rdev; >> + struct regulator_desc rdesc; >> + struct list_head list; >> +}; >> + >> +struct int3472_sensor_regulator_map { >> + char *sensor_module_name; >> + unsigned int n_supplies; >> + struct regulator_consumer_supply *supplies; >> +}; >> + >> +/* >> + * Here follows platform specific mapping information that we can pass to >> + * regulator_init_data when we register our regulators. They're just mapped >> + * via index, I.E. the first regulator pin that the code finds for the >> + * i2c-OVTI2680:00 device is avdd, the second is dovdd and so on. >> + */ >> + >> +static struct regulator_consumer_supply miix_510_ov2680[] = { >> + { "i2c-OVTI2680:00", "avdd" }, >> + { "i2c-OVTI2680:00", "dovdd" }, >> +}; >> + >> +static struct regulator_consumer_supply surface_go2_ov5693[] = { >> + { "i2c-INT33BE:00", "avdd" }, >> + { "i2c-INT33BE:00", "dovdd" }, >> +}; >> + >> +static struct regulator_consumer_supply surface_book_ov5693[] = { >> + { "i2c-INT33BE:00", "avdd" }, >> + { "i2c-INT33BE:00", "dovdd" }, >> +}; >> + >> +static struct int3472_sensor_regulator_map int3472_sensor_regulator_maps[] = { >> + { "GNDF140809R", 2, miix_510_ov2680 }, >> + { "YHCU", 2, surface_go2_ov5693 }, >> + { "MSHW0070", 2, surface_book_ov5693 }, >> +}; >> > Thanks, > JM
Hi Andy, On Mon, Nov 30, 2020 at 10:07:19PM +0200, Andy Shevchenko wrote: > On Mon, Nov 30, 2020 at 01:31:29PM +0000, Daniel Scally wrote: > > On platforms where ACPI is designed for use with Windows, resources > > that are intended to be consumed by sensor devices are sometimes in > > the _CRS of a dummy INT3472 device upon which the sensor depends. This > > driver binds to the dummy acpi device (which does not represent a > > acpi device -> acpi_device > > > physical PMIC) and maps them into GPIO lines and regulators for use by > > the sensor device instead. > > ... > > > This patch contains the bits of this process that we're least sure about. > > The sensors in scope for this work are called out as dependent (in their > > DSDT entry's _DEP) on a device with _HID INT3472. These come in at least > > 2 kinds; those with an I2cSerialBusV2 entry (which we presume therefore > > are legitimate tps68470 PMICs that need handling by those drivers - work > > on that in the future). And those without an I2C device. For those without > > an I2C device they instead have an array of GPIO pins defined in _CRS. So > > for example, my Lenovo Miix 510's OVTI2680 sensor is dependent on one of > > the _latter_ kind of INT3472 devices, with this _CRS: > > > > Method (_CRS, 0, NotSerialized) // _CRS: Current Resource Settings > > { > > Name (SBUF, ResourceTemplate () > > { > > GpioIo (Exclusive, PullDefault, 0x0000, 0x0000, > > IoRestrictionOutputOnly, "\\_SB.PCI0.GPI0", > > 0x00, ResourceConsumer, , > > ) > > { // Pin list > > 0x0079 > > } > > GpioIo (Exclusive, PullDefault, 0x0000, 0x0000, > > IoRestrictionOutputOnly, "\\_SB.PCI0.GPI0", > > 0x00, ResourceConsumer, , > > ) > > { // Pin list > > 0x007A > > } > > GpioIo (Exclusive, PullDefault, 0x0000, 0x0000, > > IoRestrictionOutputOnly, "\\_SB.PCI0.GPI0", > > 0x00, ResourceConsumer, , > > ) > > { // Pin list > > 0x008F > > } > > }) > > Return (SBUF) /* \_SB_.PCI0.PMI1._CRS.SBUF */ > > } > > > > and the same device has a _DSM Method, which returns 32-bit ints where > > the second lowest byte we noticed to match the pin numbers of the GPIO > > lines: > > > > Method (_DSM, 4, NotSerialized) // _DSM: Device-Specific Method > > { > > If ((Arg0 == ToUUID ("79234640-9e10-4fea-a5c1-b5aa8b19756f"))) > > { > > If ((Arg2 == One)) > > { > > Return (0x03) > > } > > > > If ((Arg2 == 0x02)) > > { > > Return (0x01007900) > > } > > > > If ((Arg2 == 0x03)) > > { > > Return (0x01007A0C) > > } > > > > If ((Arg2 == 0x04)) > > { > > Return (0x01008F01) > > } > > } > > > > Return (Zero) > > } > > > > We know that at least some of those pins have to be toggled active for the > > sensor devices to be available in i2c, so the conclusion we came to was > > that those GPIO entries assigned to the INT3472 device actually represent > > GPIOs and regulators to be consumed by the sensors themselves. Tsuchiya > > noticed that the lowest byte in the return values of the _DSM method > > seemed to represent the type or function of the GPIO line, and we > > confirmed that by testing on each surface device that GPIO lines where the > > low byte in the _DSM entry for that pin was 0x0d controlled the privacy > > LED of the cameras. > > > > We're guessing as to the exact meaning of the function byte, but I > > conclude they're something like this: > > > > 0x00 - probably a reset GPIO > > 0x01 - regulator for the sensor > > 0x0c - regulator for the sensor > > 0x0b - regulator again, but for a VCM or EEPROM > > 0x0d - privacy led (only one we're totally confident of since we can see > > it happen!) > > It's solely Windows driver design... > Luckily I found some information and can clarify above table: > > 0x00 Reset > 0x01 Power down > 0x0b Power enable > 0x0c Clock enable > 0x0d LED (active high) That's very useful information ! Thank you. > The above text perhaps should go somewhere under Documentation. Or in the driver source code, but definitely somewhere else than in the commit message. > > After much internal debate I decided to write this as a standalone > > acpi_driver. Alternative options we considered: > > > > 1. Squash all this into the cio2-bridge code, which I did originally write > > but decided I didn't like. > > 2. Extend the existing tps68470 mfd driver...they share an ACPI ID so this > > kinda makes sense, but ultimately given there is no actual physical > > tps68470 in the scenario this patch handles I decided I didn't like this > > either. > > Looking to this I think the best is to create a module that can be consumed by tps68470 and separately. > So, something near to it rather than under ipu3 hood. > > You may use same ID's in both drivers (in PMIC less case it can be simple > platform and thus they won't conflict), but both of them should provide GPIO > resources for consumption. > > So, something like > > tps68470.h with API to consume > split tps68470 to -core, -i2c parts > add int3472, which will serve for above and be standalone platform driver > update cio2-bridge accordingly > > Would it be feasible? Given that INT3472 means Intel camera power management device (that's more or less the wording in Windows, I can double-check), would the following make sense ? A top-level module named intel-camera-pmic (or int3472, or ...) would register two drivers, a platform driver and an I2C driver, to accommodate for both cases ("discrete PMIC" that doesn't have an I2cSerialBusV2, and TPS64870 or uP6641Q that are I2C devices). The probe function would perform the following: - If there's no CLDB, then the device uses the Chrome OS "ACPI bindings", and refers to a TPS64870. The code that exists in the kernel today (registering GPIOs, and registering an OpRegion to communicate with the power management code in the DSDT) would be activated. - If there's a CLDB, then the device type would be retrieved from it: - If the device is a "discrete PMIC", the driver would register clocks and regulators controlled by GPIOs, and create clock, regulator and GPIO lookup entries for the sensor device that references the PMIC. - If the device is a TPS64870, the code that exists in the kernel today to register GPIOs would be activated, and new code would need to be written to register regulators and clocks. - If the device is a uP6641Q, a new driver will need to be written (I don't know on which devices this PMIC is used, so this can probably be deferred). We can split this in multiple files and/or modules. > ... > > > + table_entry = (struct gpiod_lookup)GPIO_LOOKUP_IDX(acpi_dev_name(adev), > > + ares->data.gpio.pin_table[0], > > + func, 0, GPIO_ACTIVE_HIGH); > > You won't need this if you have regular INT3472 platform driver. > Simple call there _DSM to map resources to the type and use devm_gpiod_get on > consumer behalf. Thus, previous patch is not needed. How does the consumer (the camera sensor) retrieve the GPIO though ? The _DSM is in the PMIC device object, while the real consumer is the camera sensor. > ... > > > + case 0x01: /* Power regulators (we think) */ > > + case 0x0c: > > + case 0x0b: /* Power regulators, but to a device separate to sensor */ > > + case 0x0d: /* Indicator LEDs */ > > > Give names to those constants. > > #define INT3472_GPIO_TYPE_RESET 0x00 > ... > > > > +static struct acpi_driver int3472_driver = { > > No acpi_driver! Use platform_driver instead with plenty of examples all over > the kernel. > > > + .name = "int3472", > > + .ids = int3472_device_id, > > + .ops = { > > + .add = int3472_add, > > + .remove = int3472_remove, > > + }, > > > + .owner = THIS_MODULE, > > No need > > > +}; > > ... > > > +const guid_t int3472_gpio_guid = GUID_INIT(0x79234640, 0x9e10, 0x4fea, > > + 0xa5, 0xc1, 0xb5, 0xaa, 0x8b, > > + 0x19, 0x75, 0x6f); > > + > > +const guid_t cio2_sensor_module_guid = GUID_INIT(0x822ace8f, 0x2814, 0x4174, > > + 0xa5, 0x6b, 0x5f, 0x02, 0x9f, > > + 0xe0, 0x79, 0xee); > > Use more or less standard pattern for these, like > > /* 79234640-9e10-4fea-a5c1b5aa8b19756f */ > const guid_t int3472_gpio_guid = > GUID_INIT(0x79234640, 0x9e10, 0x4fea, > 0xa5, 0xc1, 0xb5, 0xaa, 0x8b, 0x19, 0x75, 0x6f); > > ... > > > +static struct regulator_consumer_supply miix_510_ov2680[] = { > > + { "i2c-OVTI2680:00", "avdd" }, > > + { "i2c-OVTI2680:00", "dovdd" }, > > +}; > > Can we use acpi_dev_first_match_dev() to get instance name out of their HIDs? > > > +static struct regulator_consumer_supply surface_go2_ov5693[] = { > > + { "i2c-INT33BE:00", "avdd" }, > > + { "i2c-INT33BE:00", "dovdd" }, > > +}; > > + > > +static struct regulator_consumer_supply surface_book_ov5693[] = { > > + { "i2c-INT33BE:00", "avdd" }, > > + { "i2c-INT33BE:00", "dovdd" }, > > +}; > > Ditto. > > ... > > > +static struct int3472_sensor_regulator_map int3472_sensor_regulator_maps[] = { > > + { "GNDF140809R", 2, miix_510_ov2680 }, > > + { "YHCU", 2, surface_go2_ov5693 }, > > + { "MSHW0070", 2, surface_book_ov5693 }, > > +}; > > Hmm... Usual way is to use DMI for that. I'm not sure above will not give us > false positive matches.
Daniel, thanks for your patch. On 11/30/20 9:31 PM, Daniel Scally wrote: > Some types of fwnode_handle do not implement the device_is_available() > check, such as those created by software_nodes. There isn't really a > meaningful way to check for the availability of a device that doesn't > actually exist, so if the check isn't implemented just assume that the > "device" is present. > > Suggested-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > Signed-off-by: Daniel Scally <djrscally@gmail.com> > --- > Changes since RFC v3: > > patch introduced > > drivers/base/property.c | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/drivers/base/property.c b/drivers/base/property.c > index 4c43d30145c6..a5ca2306796f 100644 > --- a/drivers/base/property.c > +++ b/drivers/base/property.c > @@ -785,9 +785,14 @@ EXPORT_SYMBOL_GPL(fwnode_handle_put); > /** > * fwnode_device_is_available - check if a device is available for use > * @fwnode: Pointer to the fwnode of the device. > + * > + * For fwnode node types that don't implement the .device_is_available() > + * operation, this function returns true. > */ > bool fwnode_device_is_available(const struct fwnode_handle *fwnode) > { > + if (!fwnode_has_op(fwnode, device_is_available)) > + return true; blank line here? > return fwnode_call_bool_op(fwnode, device_is_available); > } > EXPORT_SYMBOL_GPL(fwnode_device_is_available); >
Hi Dan, On Mon, Nov 30, 2020 at 11:06:03PM +0000, Dan Scally wrote: > Hi Sakari > > On 30/11/2020 20:52, Sakari Ailus wrote: > >> +static const struct acpi_device_id int3472_device_id[] = { > >> + { "INT3472", 0 }, > > The INT3472 _HID is really allocated for the tps68470 PMIC chip. It may not > > be used by other drivers; people will want to build kernels where both of > > these ACPI table layouts are functional. > > > > Instead, I propose, that you add this as an option to the tps68470 driver > > that figures out whether the ACPI device for the tps68470 device actually > > describes something else, in a similar fashion you do with the cio2-bridge > > driver. I think it may need a separate Kconfig option albeit this and > > cio2-bridge cannot be used separately. > > It actually occurs to me that that may not work (I know I called that > out as an option we considered, but that was a while ago actually). The > reason I wasn't worried about the existing tps68470 driver binding to > these devices is that it's an i2c driver, and these dummy devices don't > have an I2cSerialBusV2, so no I2C device is created by them the kernel. > > > Won't that mean the tps68470 driver won't ever be probed for these devices? Oops. I missed this indeed was not an I²C driver. So please ignore the comment. So I guess this wouldn't be an actual problem. I'd still like to test this on a system with tps68470, as the rest of the set. -- Kind regards, Sakari Ailus
I see there will be multiple files, but there will be no conflict if keep as the main file name unchanged, right? If so, I prefer keep as it was. On 11/30/20 9:31 PM, Daniel Scally wrote: > ipu3-cio2 driver needs extending with multiple files; rename the main > source file and specify the renamed file in Makefile to accommodate that. > > Suggested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > Signed-off-by: Daniel Scally <djrscally@gmail.com> > --- > Changes since RFC v3: > > - None > > drivers/media/pci/intel/ipu3/Makefile | 2 ++ > drivers/media/pci/intel/ipu3/{ipu3-cio2.c => ipu3-cio2-main.c} | 0 > 2 files changed, 2 insertions(+) > rename drivers/media/pci/intel/ipu3/{ipu3-cio2.c => ipu3-cio2-main.c} (100%) > > diff --git a/drivers/media/pci/intel/ipu3/Makefile b/drivers/media/pci/intel/ipu3/Makefile > index 98ddd5beafe0..429d516452e4 100644 > --- a/drivers/media/pci/intel/ipu3/Makefile > +++ b/drivers/media/pci/intel/ipu3/Makefile > @@ -1,2 +1,4 @@ > # SPDX-License-Identifier: GPL-2.0-only > obj-$(CONFIG_VIDEO_IPU3_CIO2) += ipu3-cio2.o > + > +ipu3-cio2-y += ipu3-cio2-main.o > diff --git a/drivers/media/pci/intel/ipu3/ipu3-cio2.c b/drivers/media/pci/intel/ipu3/ipu3-cio2-main.c > similarity index 100% > rename from drivers/media/pci/intel/ipu3/ipu3-cio2.c > rename to drivers/media/pci/intel/ipu3/ipu3-cio2-main.c >
On 01/12/2020 06:44, Sakari Ailus wrote: > Hi Dan, > > On Mon, Nov 30, 2020 at 11:06:03PM +0000, Dan Scally wrote: >> Hi Sakari >> >> On 30/11/2020 20:52, Sakari Ailus wrote: >>>> +static const struct acpi_device_id int3472_device_id[] = { >>>> + { "INT3472", 0 }, >>> The INT3472 _HID is really allocated for the tps68470 PMIC chip. It may not >>> be used by other drivers; people will want to build kernels where both of >>> these ACPI table layouts are functional. >>> >>> Instead, I propose, that you add this as an option to the tps68470 driver >>> that figures out whether the ACPI device for the tps68470 device actually >>> describes something else, in a similar fashion you do with the cio2-bridge >>> driver. I think it may need a separate Kconfig option albeit this and >>> cio2-bridge cannot be used separately. >> It actually occurs to me that that may not work (I know I called that >> out as an option we considered, but that was a while ago actually). The >> reason I wasn't worried about the existing tps68470 driver binding to >> these devices is that it's an i2c driver, and these dummy devices don't >> have an I2cSerialBusV2, so no I2C device is created by them the kernel. >> >> >> Won't that mean the tps68470 driver won't ever be probed for these devices? > Oops. I missed this indeed was not an I²C driver. So please ignore the > comment. > > So I guess this wouldn't be an actual problem. I'd still like to test this > on a system with tps68470, as the rest of the set. On my Go2, it .probes() for the actual tps68740 (that machine has both types of INT3472 device) but fails with EINVAL when it can't find the CLDB buffer that these discrete type devices have. My understanding is that means it's free for the actual tps68470 driver to grab the device; although that's not happening because I had to blacklist that driver or it stops the machine from booting at the moment - haven't gotten round to investigating yet.
On 01/12/2020 08:08, Dan Scally wrote: > On 01/12/2020 06:44, Sakari Ailus wrote: >> Hi Dan, >> >> On Mon, Nov 30, 2020 at 11:06:03PM +0000, Dan Scally wrote: >>> Hi Sakari >>> >>> On 30/11/2020 20:52, Sakari Ailus wrote: >>>>> +static const struct acpi_device_id int3472_device_id[] = { >>>>> + { "INT3472", 0 }, >>>> The INT3472 _HID is really allocated for the tps68470 PMIC chip. It may not >>>> be used by other drivers; people will want to build kernels where both of >>>> these ACPI table layouts are functional. >>>> >>>> Instead, I propose, that you add this as an option to the tps68470 driver >>>> that figures out whether the ACPI device for the tps68470 device actually >>>> describes something else, in a similar fashion you do with the cio2-bridge >>>> driver. I think it may need a separate Kconfig option albeit this and >>>> cio2-bridge cannot be used separately. >>> It actually occurs to me that that may not work (I know I called that >>> out as an option we considered, but that was a while ago actually). The >>> reason I wasn't worried about the existing tps68470 driver binding to >>> these devices is that it's an i2c driver, and these dummy devices don't >>> have an I2cSerialBusV2, so no I2C device is created by them the kernel. >>> >>> >>> Won't that mean the tps68470 driver won't ever be probed for these devices? >> Oops. I missed this indeed was not an I²C driver. So please ignore the >> comment. >> >> So I guess this wouldn't be an actual problem. I'd still like to test this >> on a system with tps68470, as the rest of the set. > On my Go2, it .probes() for the actual tps68740 (that machine has both > types of INT3472 device) but fails with EINVAL when it can't find the > CLDB buffer that these discrete type devices have. My understanding is > that means it's free for the actual tps68470 driver to grab the device; > although that's not happening because I had to blacklist that driver or > it stops the machine from booting at the moment - haven't gotten round > to investigating yet. Though having said that, it looks like a separate driver like this is the least favoured option anyway, so probably it's going to change anyway.
Hi Andy, thanks for comments On 30/11/2020 20:07, Andy Shevchenko wrote: >> We know that at least some of those pins have to be toggled active for the >> sensor devices to be available in i2c, so the conclusion we came to was >> that those GPIO entries assigned to the INT3472 device actually represent >> GPIOs and regulators to be consumed by the sensors themselves. Tsuchiya >> noticed that the lowest byte in the return values of the _DSM method >> seemed to represent the type or function of the GPIO line, and we >> confirmed that by testing on each surface device that GPIO lines where the >> low byte in the _DSM entry for that pin was 0x0d controlled the privacy >> LED of the cameras. >> >> We're guessing as to the exact meaning of the function byte, but I >> conclude they're something like this: >> >> 0x00 - probably a reset GPIO >> 0x01 - regulator for the sensor >> 0x0c - regulator for the sensor >> 0x0b - regulator again, but for a VCM or EEPROM >> 0x0d - privacy led (only one we're totally confident of since we can see >> it happen!) > It's solely Windows driver design... > Luckily I found some information and can clarify above table: > > 0x00 Reset > 0x01 Power down > 0x0b Power enable > 0x0c Clock enable > 0x0d LED (active high) > > The above text perhaps should go somewhere under Documentation. Ah! That's really useful, thank you. We can handle the clock the same way as regulators are being handled now, so that's no problem. And likewise 0x01 for power down can just be mapped to the Sensor device along with the reset pin and led pins. "Power enable" sounds like a regulator indeed...it's not present on many (most) of our sensors actually but that's not a problem for them of course as they'll just be driven by the dummy regulators. Thanks for the info > > ... > >> + table_entry = (struct gpiod_lookup)GPIO_LOOKUP_IDX(acpi_dev_name(adev), >> + ares->data.gpio.pin_table[0], >> + func, 0, GPIO_ACTIVE_HIGH); > You won't need this if you have regular INT3472 platform driver. > Simple call there _DSM to map resources to the type and use devm_gpiod_get on > consumer behalf. Thus, previous patch is not needed. But the resources need to be available to the sensor devices; they're basically in the wrong place. They should be in _CRS of the sensor, rather than INT3472, so we need to map them across. > ... > >> +static struct regulator_consumer_supply miix_510_ov2680[] = { >> + { "i2c-OVTI2680:00", "avdd" }, >> + { "i2c-OVTI2680:00", "dovdd" }, >> +}; > Can we use acpi_dev_first_match_dev() to get instance name out of their HIDs? We need the full i2c device name, which afaik isn't available from the acpi_device >> +static struct int3472_sensor_regulator_map int3472_sensor_regulator_maps[] = { >> + { "GNDF140809R", 2, miix_510_ov2680 }, >> + { "YHCU", 2, surface_go2_ov5693 }, >> + { "MSHW0070", 2, surface_book_ov5693 }, >> +}; > Hmm... Usual way is to use DMI for that. I'm not sure above will not give us > false positive matches. I considered DMI too, no problem to switch to that if it's a better choice.
Hi Dan, On Tue, Dec 01, 2020 at 08:08:26AM +0000, Dan Scally wrote: > > On 01/12/2020 06:44, Sakari Ailus wrote: > > Hi Dan, > > > > On Mon, Nov 30, 2020 at 11:06:03PM +0000, Dan Scally wrote: > >> Hi Sakari > >> > >> On 30/11/2020 20:52, Sakari Ailus wrote: > >>>> +static const struct acpi_device_id int3472_device_id[] = { > >>>> + { "INT3472", 0 }, > >>> The INT3472 _HID is really allocated for the tps68470 PMIC chip. It may not > >>> be used by other drivers; people will want to build kernels where both of > >>> these ACPI table layouts are functional. > >>> > >>> Instead, I propose, that you add this as an option to the tps68470 driver > >>> that figures out whether the ACPI device for the tps68470 device actually > >>> describes something else, in a similar fashion you do with the cio2-bridge > >>> driver. I think it may need a separate Kconfig option albeit this and > >>> cio2-bridge cannot be used separately. > >> It actually occurs to me that that may not work (I know I called that > >> out as an option we considered, but that was a while ago actually). The > >> reason I wasn't worried about the existing tps68470 driver binding to > >> these devices is that it's an i2c driver, and these dummy devices don't > >> have an I2cSerialBusV2, so no I2C device is created by them the kernel. > >> > >> > >> Won't that mean the tps68470 driver won't ever be probed for these devices? > > Oops. I missed this indeed was not an I²C driver. So please ignore the > > comment. > > > > So I guess this wouldn't be an actual problem. I'd still like to test this > > on a system with tps68470, as the rest of the set. > On my Go2, it .probes() for the actual tps68740 (that machine has both > types of INT3472 device) but fails with EINVAL when it can't find the > CLDB buffer that these discrete type devices have. My understanding is > that means it's free for the actual tps68470 driver to grab the device; > although that's not happening because I had to blacklist that driver or > it stops the machine from booting at the moment - haven't gotten round > to investigating yet. Oh, then the problem is actually there. If it probes the tps68470 driver on the systems with Windows ACPI tables, then it should be that driver which works with the Windows ACPI tables, too. Checking for random objects such as CLDB in multiple drivers and returning an error based on them being there or not wouldn't be exactly neat. Although I'm not sure thare are options that are obviosly pretty here. I wouldn't two separate drivers checking for e.g. CLDB (tps68470 + this one). The tps68470 driver is an MFD driver that instantiates a number of platform devices. Alternatively, if you make this one a platform device, you can, in case the CLDB (or whatever object) is present, in the tps68470 driver instantiate a device for this driver instead of the rest. So I'd think what matters is that both drivers can be selected at the same time but the user does not need to manually select them. Both ways could work I guess? -- Kind regards, Sakari Ailus
Hi Sakari, On Tue, Dec 01, 2020 at 02:32:44PM +0200, Sakari Ailus wrote: > On Tue, Dec 01, 2020 at 08:08:26AM +0000, Dan Scally wrote: > > On 01/12/2020 06:44, Sakari Ailus wrote: > > > On Mon, Nov 30, 2020 at 11:06:03PM +0000, Dan Scally wrote: > > >> On 30/11/2020 20:52, Sakari Ailus wrote: > > >>>> +static const struct acpi_device_id int3472_device_id[] = { > > >>>> + { "INT3472", 0 }, > > >>> > > >>> The INT3472 _HID is really allocated for the tps68470 PMIC chip. It may not > > >>> be used by other drivers; people will want to build kernels where both of > > >>> these ACPI table layouts are functional. > > >>> > > >>> Instead, I propose, that you add this as an option to the tps68470 driver > > >>> that figures out whether the ACPI device for the tps68470 device actually > > >>> describes something else, in a similar fashion you do with the cio2-bridge > > >>> driver. I think it may need a separate Kconfig option albeit this and > > >>> cio2-bridge cannot be used separately. > > >> > > >> It actually occurs to me that that may not work (I know I called that > > >> out as an option we considered, but that was a while ago actually). The > > >> reason I wasn't worried about the existing tps68470 driver binding to > > >> these devices is that it's an i2c driver, and these dummy devices don't > > >> have an I2cSerialBusV2, so no I2C device is created by them the kernel. > > >> > > >> Won't that mean the tps68470 driver won't ever be probed for these devices? > > > > > > Oops. I missed this indeed was not an I²C driver. So please ignore the > > > comment. > > > > > > So I guess this wouldn't be an actual problem. I'd still like to test this > > > on a system with tps68470, as the rest of the set. > > > > On my Go2, it .probes() for the actual tps68740 (that machine has both > > types of INT3472 device) but fails with EINVAL when it can't find the > > CLDB buffer that these discrete type devices have. My understanding is > > that means it's free for the actual tps68470 driver to grab the device; > > although that's not happening because I had to blacklist that driver or > > it stops the machine from booting at the moment - haven't gotten round > > to investigating yet. > > Oh, then the problem is actually there. If it probes the tps68470 driver on > the systems with Windows ACPI tables, then it should be that driver which > works with the Windows ACPI tables, too. > > Checking for random objects such as CLDB in multiple drivers and returning > an error based on them being there or not wouldn't be exactly neat. > Although I'm not sure thare are options that are obviosly pretty here. I > wouldn't two separate drivers checking for e.g. CLDB (tps68470 + this one). > > The tps68470 driver is an MFD driver that instantiates a number of platform > devices. Alternatively, if you make this one a platform device, you can, in > case the CLDB (or whatever object) is present, in the tps68470 driver > instantiate a device for this driver instead of the rest. > > So I'd think what matters is that both drivers can be selected at the same > time but the user does not need to manually select them. Both ways could > work I guess? Let's make it simpler instead of creating lots of devices. Here's what I've proposed in a different e-mail in this thread. > Given that INT3472 means Intel camera power management device (that's > more or less the wording in Windows, I can double-check), would the > following make sense ? > > A top-level module named intel-camera-pmic (or int3472, or ...) would > register two drivers, a platform driver and an I2C driver, to > accommodate for both cases ("discrete PMIC" that doesn't have an > I2cSerialBusV2, and TPS64870 or uP6641Q that are I2C devices). The probe > function would perform the following: > > - If there's no CLDB, then the device uses the Chrome OS "ACPI > bindings", and refers to a TPS64870. The code that exists in the > kernel today (registering GPIOs, and registering an OpRegion to > communicate with the power management code in the DSDT) would be > activated. > > - If there's a CLDB, then the device type would be retrieved from it: > > - If the device is a "discrete PMIC", the driver would register clocks > and regulators controlled by GPIOs, and create clock, regulator and > GPIO lookup entries for the sensor device that references the PMIC. > > - If the device is a TPS64870, the code that exists in the kernel > today to register GPIOs would be activated, and new code would need > to be written to register regulators and clocks. > > - If the device is a uP6641Q, a new driver will need to be written (I > don't know on which devices this PMIC is used, so this can probably > be deferred). > > We can split this in multiple files and/or modules. Could you reply to 20201130233232.GD25713@pendragon.ideasonboard.com to avoid splitting the conversation ?
On 01/12/2020 12:32, Sakari Ailus wrote: > Hi Dan, > > On Tue, Dec 01, 2020 at 08:08:26AM +0000, Dan Scally wrote: >> On 01/12/2020 06:44, Sakari Ailus wrote: >>> Hi Dan, >>> >>> On Mon, Nov 30, 2020 at 11:06:03PM +0000, Dan Scally wrote: >>>> Hi Sakari >>>> >>>> On 30/11/2020 20:52, Sakari Ailus wrote: >>>>>> +static const struct acpi_device_id int3472_device_id[] = { >>>>>> + { "INT3472", 0 }, >>>>> The INT3472 _HID is really allocated for the tps68470 PMIC chip. It may not >>>>> be used by other drivers; people will want to build kernels where both of >>>>> these ACPI table layouts are functional. >>>>> >>>>> Instead, I propose, that you add this as an option to the tps68470 driver >>>>> that figures out whether the ACPI device for the tps68470 device actually >>>>> describes something else, in a similar fashion you do with the cio2-bridge >>>>> driver. I think it may need a separate Kconfig option albeit this and >>>>> cio2-bridge cannot be used separately. >>>> It actually occurs to me that that may not work (I know I called that >>>> out as an option we considered, but that was a while ago actually). The >>>> reason I wasn't worried about the existing tps68470 driver binding to >>>> these devices is that it's an i2c driver, and these dummy devices don't >>>> have an I2cSerialBusV2, so no I2C device is created by them the kernel. >>>> >>>> >>>> Won't that mean the tps68470 driver won't ever be probed for these devices? >>> Oops. I missed this indeed was not an I²C driver. So please ignore the >>> comment. >>> >>> So I guess this wouldn't be an actual problem. I'd still like to test this >>> on a system with tps68470, as the rest of the set. >> On my Go2, it .probes() for the actual tps68740 (that machine has both >> types of INT3472 device) but fails with EINVAL when it can't find the >> CLDB buffer that these discrete type devices have. My understanding is >> that means it's free for the actual tps68470 driver to grab the device; >> although that's not happening because I had to blacklist that driver or >> it stops the machine from booting at the moment - haven't gotten round >> to investigating yet. > Oh, then the problem is actually there. If it probes the tps68470 driver on > the systems with Windows ACPI tables, then it should be that driver which > works with the Windows ACPI tables, too. Sorry, clarification here: The INT3472 driver in patch #18 runs probe() for the device representing a physical tps68470, but then -EINVAL's. The existing tps68470 mfd driver doesn't probe() for the dummy INT3472 device.
Hi Laurent, On Tue, Dec 01, 2020 at 01:32:32AM +0200, Laurent Pinchart wrote: > Hi Andy, > > On Mon, Nov 30, 2020 at 10:07:19PM +0200, Andy Shevchenko wrote: > > On Mon, Nov 30, 2020 at 01:31:29PM +0000, Daniel Scally wrote: > > > On platforms where ACPI is designed for use with Windows, resources > > > that are intended to be consumed by sensor devices are sometimes in > > > the _CRS of a dummy INT3472 device upon which the sensor depends. This > > > driver binds to the dummy acpi device (which does not represent a > > > > acpi device -> acpi_device > > > > > physical PMIC) and maps them into GPIO lines and regulators for use by > > > the sensor device instead. > > > > ... > > > > > This patch contains the bits of this process that we're least sure about. > > > The sensors in scope for this work are called out as dependent (in their > > > DSDT entry's _DEP) on a device with _HID INT3472. These come in at least > > > 2 kinds; those with an I2cSerialBusV2 entry (which we presume therefore > > > are legitimate tps68470 PMICs that need handling by those drivers - work > > > on that in the future). And those without an I2C device. For those without > > > an I2C device they instead have an array of GPIO pins defined in _CRS. So > > > for example, my Lenovo Miix 510's OVTI2680 sensor is dependent on one of > > > the _latter_ kind of INT3472 devices, with this _CRS: > > > > > > Method (_CRS, 0, NotSerialized) // _CRS: Current Resource Settings > > > { > > > Name (SBUF, ResourceTemplate () > > > { > > > GpioIo (Exclusive, PullDefault, 0x0000, 0x0000, > > > IoRestrictionOutputOnly, "\\_SB.PCI0.GPI0", > > > 0x00, ResourceConsumer, , > > > ) > > > { // Pin list > > > 0x0079 > > > } > > > GpioIo (Exclusive, PullDefault, 0x0000, 0x0000, > > > IoRestrictionOutputOnly, "\\_SB.PCI0.GPI0", > > > 0x00, ResourceConsumer, , > > > ) > > > { // Pin list > > > 0x007A > > > } > > > GpioIo (Exclusive, PullDefault, 0x0000, 0x0000, > > > IoRestrictionOutputOnly, "\\_SB.PCI0.GPI0", > > > 0x00, ResourceConsumer, , > > > ) > > > { // Pin list > > > 0x008F > > > } > > > }) > > > Return (SBUF) /* \_SB_.PCI0.PMI1._CRS.SBUF */ > > > } > > > > > > and the same device has a _DSM Method, which returns 32-bit ints where > > > the second lowest byte we noticed to match the pin numbers of the GPIO > > > lines: > > > > > > Method (_DSM, 4, NotSerialized) // _DSM: Device-Specific Method > > > { > > > If ((Arg0 == ToUUID ("79234640-9e10-4fea-a5c1-b5aa8b19756f"))) > > > { > > > If ((Arg2 == One)) > > > { > > > Return (0x03) > > > } > > > > > > If ((Arg2 == 0x02)) > > > { > > > Return (0x01007900) > > > } > > > > > > If ((Arg2 == 0x03)) > > > { > > > Return (0x01007A0C) > > > } > > > > > > If ((Arg2 == 0x04)) > > > { > > > Return (0x01008F01) > > > } > > > } > > > > > > Return (Zero) > > > } > > > > > > We know that at least some of those pins have to be toggled active for the > > > sensor devices to be available in i2c, so the conclusion we came to was > > > that those GPIO entries assigned to the INT3472 device actually represent > > > GPIOs and regulators to be consumed by the sensors themselves. Tsuchiya > > > noticed that the lowest byte in the return values of the _DSM method > > > seemed to represent the type or function of the GPIO line, and we > > > confirmed that by testing on each surface device that GPIO lines where the > > > low byte in the _DSM entry for that pin was 0x0d controlled the privacy > > > LED of the cameras. > > > > > > We're guessing as to the exact meaning of the function byte, but I > > > conclude they're something like this: > > > > > > 0x00 - probably a reset GPIO > > > 0x01 - regulator for the sensor > > > 0x0c - regulator for the sensor > > > 0x0b - regulator again, but for a VCM or EEPROM > > > 0x0d - privacy led (only one we're totally confident of since we can see > > > it happen!) > > > > It's solely Windows driver design... > > Luckily I found some information and can clarify above table: > > > > 0x00 Reset > > 0x01 Power down > > 0x0b Power enable > > 0x0c Clock enable > > 0x0d LED (active high) > > That's very useful information ! Thank you. > > > The above text perhaps should go somewhere under Documentation. > > Or in the driver source code, but definitely somewhere else than in the > commit message. > > > > After much internal debate I decided to write this as a standalone > > > acpi_driver. Alternative options we considered: > > > > > > 1. Squash all this into the cio2-bridge code, which I did originally write > > > but decided I didn't like. > > > 2. Extend the existing tps68470 mfd driver...they share an ACPI ID so this > > > kinda makes sense, but ultimately given there is no actual physical > > > tps68470 in the scenario this patch handles I decided I didn't like this > > > either. > > > > Looking to this I think the best is to create a module that can be consumed by tps68470 and separately. > > So, something near to it rather than under ipu3 hood. > > > > You may use same ID's in both drivers (in PMIC less case it can be simple > > platform and thus they won't conflict), but both of them should provide GPIO > > resources for consumption. > > > > So, something like > > > > tps68470.h with API to consume > > split tps68470 to -core, -i2c parts > > add int3472, which will serve for above and be standalone platform driver > > update cio2-bridge accordingly > > > > Would it be feasible? > > Given that INT3472 means Intel camera power management device (that's > more or less the wording in Windows, I can double-check), would the > following make sense ? > > A top-level module named intel-camera-pmic (or int3472, or ...) would > register two drivers, a platform driver and an I2C driver, to > accommodate for both cases ("discrete PMIC" that doesn't have an > I2cSerialBusV2, and TPS64870 or uP6641Q that are I2C devices). The probe > function would perform the following: > > - If there's no CLDB, then the device uses the Chrome OS "ACPI > bindings", and refers to a TPS64870. The code that exists in the > kernel today (registering GPIOs, and registering an OpRegion to > communicate with the power management code in the DSDT) would be > activated. > > - If there's a CLDB, then the device type would be retrieved from it: > > - If the device is a "discrete PMIC", the driver would register clocks > and regulators controlled by GPIOs, and create clock, regulator and > GPIO lookup entries for the sensor device that references the PMIC. > > - If the device is a TPS64870, the code that exists in the kernel > today to register GPIOs would be activated, and new code would need > to be written to register regulators and clocks. > > - If the device is a uP6641Q, a new driver will need to be written (I > don't know on which devices this PMIC is used, so this can probably > be deferred). > > We can split this in multiple files and/or modules. That's what I thought of, too, as one option, but with some more detail. This would be indeed the cleanest option. I think it'd be nice if the CLDB stuff (apart from checking whether it's there) would be in a different module to avoid cluttering up the real tps68470 driver.
Hi Andy, On Tue, Dec 01, 2020 at 08:31:39PM +0200, Andy Shevchenko wrote: > On Mon, Nov 30, 2020 at 11:20:55PM +0000, Dan Scally wrote: > > On 30/11/2020 16:17, Jean-Michel Hautbois wrote: > > ... > > > but the ACPI table doesn't define an I2CSerialBusV2 for it. Instead it's > > rolled under the sensor's entry, there's a second entry in _CRS for the > > sensor that matches the address of the new device: > > > > > > Method (_CRS, 0, NotSerialized) // _CRS: Current Resource Settings > > { > > Name (SBUF, ResourceTemplate () > > { > > I2cSerialBusV2 (0x0036, ControllerInitiated, 0x00061A80, > > AddressingMode7Bit, "\\_SB.PCI0.I2C2", > > 0x00, ResourceConsumer, , Exclusive, > > ) > > I2cSerialBusV2 (0x000C, ControllerInitiated, 0x00061A80, > > AddressingMode7Bit, "\\_SB.PCI0.I2C2", > > 0x00, ResourceConsumer, , Exclusive, > > ) > > }) > > Return (SBUF) /* \_SB_.PCI0.CAM0._CRS.SBUF */ > > } > > > > So that's another thing we need to work on. At the moment it doesn't > > exist as far as the kernel is concerned. > > Maybe something along i2c-multi-instantiate can help here (maybe not). It's two different devices really. That's also one of the "annoyances" related to this platform. The INT* HID for the camera sensor actually refers to a camera module, with VCM, EEPROM, ... On Chrome OS devices, the same HID refers to the camera sensor only. *sigh* :-( > P.S. Dan, can you drop unrelated text when replying? I find a full quote actually useful, as it saves me from having to dig up original e-mails to read missing parts :-) It's a matter of preference I suppose.
On Tue, Dec 01, 2020 at 01:32:32AM +0200, Laurent Pinchart wrote: > On Mon, Nov 30, 2020 at 10:07:19PM +0200, Andy Shevchenko wrote: > > On Mon, Nov 30, 2020 at 01:31:29PM +0000, Daniel Scally wrote: ... > > So, something like > > > > tps68470.h with API to consume > > split tps68470 to -core, -i2c parts > > add int3472, which will serve for above and be standalone platform driver > > update cio2-bridge accordingly > > > > Would it be feasible? > > Given that INT3472 means Intel camera power management device (that's > more or less the wording in Windows, I can double-check), would the > following make sense ? > > A top-level module named intel-camera-pmic (or int3472, or ...) would > register two drivers, a platform driver and an I2C driver, to > accommodate for both cases ("discrete PMIC" that doesn't have an > I2cSerialBusV2, and TPS64870 or uP6641Q that are I2C devices). The probe > function would perform the following: > > - If there's no CLDB, then the device uses the Chrome OS "ACPI > bindings", and refers to a TPS64870. The code that exists in the > kernel today (registering GPIOs, and registering an OpRegion to > communicate with the power management code in the DSDT) would be > activated. > > - If there's a CLDB, then the device type would be retrieved from it: > > - If the device is a "discrete PMIC", the driver would register clocks > and regulators controlled by GPIOs, and create clock, regulator and > GPIO lookup entries for the sensor device that references the PMIC. > > - If the device is a TPS64870, the code that exists in the kernel > today to register GPIOs would be activated, and new code would need > to be written to register regulators and clocks. > > - If the device is a uP6641Q, a new driver will need to be written (I > don't know on which devices this PMIC is used, so this can probably > be deferred). > > We can split this in multiple files and/or modules. Seems we can do this, by locating intel_int3472.c under PDx86 hood and dropping ACPI ID table from TPS68470 MFD driver. The PMIC can be instantiated via i2c_acpi_new_device() (IIRC the API name). And actually it makes more sense since it's not and MFD and should not be there. (Dan, patch wise the one creates intel_int3472.c followed by another one that moves ACPI ID from PMIC and introduces its instantiation via I²C board info structure) ... > > > + table_entry = (struct gpiod_lookup)GPIO_LOOKUP_IDX(acpi_dev_name(adev), > > > + ares->data.gpio.pin_table[0], > > > + func, 0, GPIO_ACTIVE_HIGH); > > > > You won't need this if you have regular INT3472 platform driver. > > Simple call there _DSM to map resources to the type and use devm_gpiod_get on > > consumer behalf. Thus, previous patch is not needed. > > How does the consumer (the camera sensor) retrieve the GPIO though ? The > _DSM is in the PMIC device object, while the real consumer is the camera > sensor. 1. A GPIO proxy 2. A custom GPIO lookup tables 3. An fwnode passing to the sensor (via swnodes graph) First may issue deferred probe, while second needs some ordering tricks I guess. Third one should also provide an ACPI GPIO mapping table or so to make the consumer rely on names rather than custom numbers. Perhaps someone may propose other solutions.
On Tue, Dec 01, 2020 at 08:30:03AM +0000, Dan Scally wrote: > On 30/11/2020 20:07, Andy Shevchenko wrote: ... > >> +static struct int3472_sensor_regulator_map int3472_sensor_regulator_maps[] = { > >> + { "GNDF140809R", 2, miix_510_ov2680 }, > >> + { "YHCU", 2, surface_go2_ov5693 }, > >> + { "MSHW0070", 2, surface_book_ov5693 }, > >> +}; > > Hmm... Usual way is to use DMI for that. I'm not sure above will not give us > > false positive matches. > I considered DMI too, no problem to switch to that if it's a better choice. I prefer DMI as it's a standard way to describe platform quirks in x86 world.
On Mon, Nov 30, 2020 at 11:06:03PM +0000, Dan Scally wrote: > On 30/11/2020 20:52, Sakari Ailus wrote: > >> +static const struct acpi_device_id int3472_device_id[] = { > >> + { "INT3472", 0 }, > > The INT3472 _HID is really allocated for the tps68470 PMIC chip. It may not > > be used by other drivers; people will want to build kernels where both of > > these ACPI table layouts are functional. > > > > Instead, I propose, that you add this as an option to the tps68470 driver > > that figures out whether the ACPI device for the tps68470 device actually > > describes something else, in a similar fashion you do with the cio2-bridge > > driver. I think it may need a separate Kconfig option albeit this and > > cio2-bridge cannot be used separately. > > It actually occurs to me that that may not work (I know I called that > out as an option we considered, but that was a while ago actually). The > reason I wasn't worried about the existing tps68470 driver binding to > these devices is that it's an i2c driver, and these dummy devices don't > have an I2cSerialBusV2, so no I2C device is created by them the kernel. > > > Won't that mean the tps68470 driver won't ever be probed for these devices? It won't be probed by kernel as long as it stays pure I²C driver..
On Tue, Dec 01, 2020 at 08:55:48PM +0200, Laurent Pinchart wrote: > On Tue, Dec 01, 2020 at 08:54:17PM +0200, Andy Shevchenko wrote: > > On Tue, Dec 01, 2020 at 08:30:03AM +0000, Dan Scally wrote: > > > On 30/11/2020 20:07, Andy Shevchenko wrote: ... > > > >> +static struct int3472_sensor_regulator_map int3472_sensor_regulator_maps[] = { > > > >> + { "GNDF140809R", 2, miix_510_ov2680 }, > > > >> + { "YHCU", 2, surface_go2_ov5693 }, > > > >> + { "MSHW0070", 2, surface_book_ov5693 }, > > > >> +}; > > > > > > > > Hmm... Usual way is to use DMI for that. I'm not sure above will not give us > > > > false positive matches. > > > > > > I considered DMI too, no problem to switch to that if it's a better choice. > > > > I prefer DMI as it's a standard way to describe platform quirks in x86 world. > > Do you think the Windows driver would use DMI ? Linux is using DMI for quirks. > That seems quite > unlikely to me, given how they would have to release a new driver binary > for every machine. I'm pretty sure that a different mechanism is used to > identify camera integration, and I think it would make sense to follow > the same approach. That would allow us to avoid large tables of DMI > identifiers that would need to be constently updated, potentially making > user experience better. All Surface family can be matched in a way as Apple machines [1]. [1]: https://lkml.org/lkml/2020/4/15/1198
On Tue, Dec 01, 2020 at 09:05:23PM +0200, Andy Shevchenko wrote: > On Tue, Dec 01, 2020 at 08:55:48PM +0200, Laurent Pinchart wrote: > > On Tue, Dec 01, 2020 at 08:54:17PM +0200, Andy Shevchenko wrote: > > > On Tue, Dec 01, 2020 at 08:30:03AM +0000, Dan Scally wrote: > > > > On 30/11/2020 20:07, Andy Shevchenko wrote: > > ... > > > > > >> +static struct int3472_sensor_regulator_map int3472_sensor_regulator_maps[] = { > > > > >> + { "GNDF140809R", 2, miix_510_ov2680 }, > > > > >> + { "YHCU", 2, surface_go2_ov5693 }, > > > > >> + { "MSHW0070", 2, surface_book_ov5693 }, > > > > >> +}; > > > > > > > > > > Hmm... Usual way is to use DMI for that. I'm not sure above will not give us > > > > > false positive matches. > > > > > > > > I considered DMI too, no problem to switch to that if it's a better choice. > > > > > > I prefer DMI as it's a standard way to describe platform quirks in x86 world. > > > > Do you think the Windows driver would use DMI ? > > Linux is using DMI for quirks. > > > That seems quite > > unlikely to me, given how they would have to release a new driver binary > > for every machine. I'm pretty sure that a different mechanism is used to > > identify camera integration, and I think it would make sense to follow > > the same approach. That would allow us to avoid large tables of DMI > > identifiers that would need to be constently updated, potentially making > > user experience better. > > All Surface family can be matched in a way as Apple machines [1]. > > [1]: https://lkml.org/lkml/2020/4/15/1198 But not all Surface machines necessarily have the same camera architecture. My point is that there seems to be identifiers reported in ACPI for the exact purpose of identifying the camera architecture. If we used DMI instead, we would have to handle each machine individually. -- Regards, Laurent Pinchart
On Tue, Dec 01, 2020 at 09:06:38PM +0200, Laurent Pinchart wrote: > On Tue, Dec 01, 2020 at 09:05:23PM +0200, Andy Shevchenko wrote: > > On Tue, Dec 01, 2020 at 08:55:48PM +0200, Laurent Pinchart wrote: > > > On Tue, Dec 01, 2020 at 08:54:17PM +0200, Andy Shevchenko wrote: > > > > On Tue, Dec 01, 2020 at 08:30:03AM +0000, Dan Scally wrote: > > > > > On 30/11/2020 20:07, Andy Shevchenko wrote: > > > > ... > > > > > > > >> +static struct int3472_sensor_regulator_map int3472_sensor_regulator_maps[] = { > > > > > >> + { "GNDF140809R", 2, miix_510_ov2680 }, > > > > > >> + { "YHCU", 2, surface_go2_ov5693 }, > > > > > >> + { "MSHW0070", 2, surface_book_ov5693 }, > > > > > >> +}; > > > > > > > > > > > > Hmm... Usual way is to use DMI for that. I'm not sure above will not give us > > > > > > false positive matches. > > > > > > > > > > I considered DMI too, no problem to switch to that if it's a better choice. > > > > > > > > I prefer DMI as it's a standard way to describe platform quirks in x86 world. > > > > > > Do you think the Windows driver would use DMI ? > > > > Linux is using DMI for quirks. > > > > > That seems quite > > > unlikely to me, given how they would have to release a new driver binary > > > for every machine. I'm pretty sure that a different mechanism is used to > > > identify camera integration, and I think it would make sense to follow > > > the same approach. That would allow us to avoid large tables of DMI > > > identifiers that would need to be constently updated, potentially making > > > user experience better. > > > > All Surface family can be matched in a way as Apple machines [1]. > > > > [1]: https://lkml.org/lkml/2020/4/15/1198 > > But not all Surface machines necessarily have the same camera > architecture. My point is that there seems to be identifiers reported in > ACPI for the exact purpose of identifying the camera architecture. If we > used DMI instead, we would have to handle each machine individually. With help of DMI we may narrow down the search. But again, we are talking about uncertainity. It may be your way (a lot of platforms that have different settings), or mine (only a few with more or less standard sets of settings). DMI is simply standard in Linux (people usually easier can grep for quirks for a specific platform). I would rather ask Hans' opinion since he has quite an expertise with DMI for good and bad. -- With Best Regards, Andy Shevchenko
On Tue, Dec 1, 2020 at 10:39 PM Hans de Goede <hdegoede@redhat.com> wrote: > On 12/1/20 8:21 PM, Andy Shevchenko wrote: > > On Tue, Dec 01, 2020 at 09:06:38PM +0200, Laurent Pinchart wrote: > >> On Tue, Dec 01, 2020 at 09:05:23PM +0200, Andy Shevchenko wrote: > >>> On Tue, Dec 01, 2020 at 08:55:48PM +0200, Laurent Pinchart wrote: > >>>> On Tue, Dec 01, 2020 at 08:54:17PM +0200, Andy Shevchenko wrote: > >>>>> On Tue, Dec 01, 2020 at 08:30:03AM +0000, Dan Scally wrote: > >>>>>> On 30/11/2020 20:07, Andy Shevchenko wrote: ... > >>>>>>>> +static struct int3472_sensor_regulator_map int3472_sensor_regulator_maps[] = { > >>>>>>>> + { "GNDF140809R", 2, miix_510_ov2680 }, > >>>>>>>> + { "YHCU", 2, surface_go2_ov5693 }, > >>>>>>>> + { "MSHW0070", 2, surface_book_ov5693 }, > >>>>>>>> +}; > >>>>>>> > >>>>>>> Hmm... Usual way is to use DMI for that. I'm not sure above will not give us > >>>>>>> false positive matches. > >>>>>> > >>>>>> I considered DMI too, no problem to switch to that if it's a better choice. > >>>>> > >>>>> I prefer DMI as it's a standard way to describe platform quirks in x86 world. > >>>> > >>>> Do you think the Windows driver would use DMI ? > >>> > >>> Linux is using DMI for quirks. > >>> > >>>> That seems quite > >>>> unlikely to me, given how they would have to release a new driver binary > >>>> for every machine. I'm pretty sure that a different mechanism is used to > >>>> identify camera integration, and I think it would make sense to follow > >>>> the same approach. That would allow us to avoid large tables of DMI > >>>> identifiers that would need to be constently updated, potentially making > >>>> user experience better. > >>> > >>> All Surface family can be matched in a way as Apple machines [1]. > >>> > >>> [1]: https://lkml.org/lkml/2020/4/15/1198 > >> > >> But not all Surface machines necessarily have the same camera > >> architecture. My point is that there seems to be identifiers reported in > >> ACPI for the exact purpose of identifying the camera architecture. If we > >> used DMI instead, we would have to handle each machine individually. > > > > With help of DMI we may narrow down the search. > > > > But again, we are talking about uncertainity. It may be your way (a lot of > > platforms that have different settings), or mine (only a few with more or less > > standard sets of settings). > > > > DMI is simply standard in Linux (people usually easier can grep for quirks for > > a specific platform). > > > > I would rather ask Hans' opinion since he has quite an expertise with DMI for > > good and bad. > > So generally there are 2 ways how things like this can go: > > 1) There is sufficient information in the ACPI table and we use data from the > ACPI tables > > 2) There is unsufficient info in the ACPI tables (or we don't know how to > get / interpret the data) and we use DMI quirks > > Although we do often also use a combination, getting what we can from ACPI, > combined with a set of defaults for what we cannot get from ACPI > based on what reference designs use (IOW what most devices seem to have > copy and pasted). Combined with DMI quirks for when the defaults do not > work (which is quite often). > > Depending on if "not working because of wrong defaults" has bad side effects, > another option is also to only allow the driver to load on devices which > have the necessary info provided through a DMI match. > > I hope this helps. Thanks! Yes, it sounds to me as a useful input!
On 01/12/2020 19:21, Andy Shevchenko wrote: > On Tue, Dec 01, 2020 at 09:06:38PM +0200, Laurent Pinchart wrote: >> On Tue, Dec 01, 2020 at 09:05:23PM +0200, Andy Shevchenko wrote: >>> On Tue, Dec 01, 2020 at 08:55:48PM +0200, Laurent Pinchart wrote: >>> >>>> Do you think the Windows driver would use DMI ? >>> Linux is using DMI for quirks. >>> >>>> That seems quite >>>> unlikely to me, given how they would have to release a new driver binary >>>> for every machine. I'm pretty sure that a different mechanism is used to >>>> identify camera integration, and I think it would make sense to follow >>>> the same approach. That would allow us to avoid large tables of DMI >>>> identifiers that would need to be constently updated, potentially making >>>> user experience better. >>> All Surface family can be matched in a way as Apple machines [1]. >>> >>> [1]: https://lkml.org/lkml/2020/4/15/1198 >> But not all Surface machines necessarily have the same camera >> architecture. My point is that there seems to be identifiers reported in >> ACPI for the exact purpose of identifying the camera architecture. If we >> used DMI instead, we would have to handle each machine individually. > With help of DMI we may narrow down the search. > > But again, we are talking about uncertainity. It may be your way (a lot of > platforms that have different settings), or mine (only a few with more or less > standard sets of settings). > > DMI is simply standard in Linux (people usually easier can grep for quirks for > a specific platform). > > I would rather ask Hans' opinion since he has quite an expertise with DMI for > good and bad. > I have no real preference as to the current method or DMI, but thoughts that come to mind are: 1. given your info that low byte 0x0c means clock enable, we need to register a clock too. Do we need to extend this device specific section to map a clock name, or is it acceptable for them to be nameless (ISTR that the API will let you fetch a clock using devm_clock_get(dev, NULL);) 2. Given only 0x0b pin is actually a regulator and it's controlling multiple devices, my plan when we got round to adding the VCM / EEPROM support was simply to extend those mapping tables so that those supplementary devices were also able to get that regulator...and the two would share it. I think, from reading the regulator code and documentation, that that's all fine - and it won't actually be disabled until both drivers disable it. Does that sound about right?
On 30/11/2020 17:12, Laurent Pinchart wrote: > Hi Daniel, > > Thank you for the patch. > > On Mon, Nov 30, 2020 at 01:31:27PM +0000, Daniel Scally wrote: >> From: Dan Scally <djrscally@gmail.com> >> >> To make sure the new i2c_acpi_dev_name() always reflects the name of i2c >> devices sourced from ACPI, use it in i2c_set_dev_name(). >> >> Signed-off-by: Dan Scally <djrscally@gmail.com> > > I'd squash this with 15/18, which would make it clear there's a memory > leak :-) Ah - that was sloppy...switched from devm_ and forgot to go fix that. I'll add the kfree into i2c_unregister_device() and squash to 15/18 >> --- >> Changes since RFC v3: >> >> - Patch introduced >> >> drivers/i2c/i2c-core-base.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c >> index 573b5da145d1..a6d4ceb01077 100644 >> --- a/drivers/i2c/i2c-core-base.c >> +++ b/drivers/i2c/i2c-core-base.c >> @@ -814,7 +814,7 @@ static void i2c_dev_set_name(struct i2c_adapter *adap, >> } >> >> if (adev) { >> - dev_set_name(&client->dev, "i2c-%s", acpi_dev_name(adev)); >> + dev_set_name(&client->dev, i2c_acpi_dev_name(adev)); >> return; >> } >> >
On Tue, Dec 01, 2020 at 08:59:53PM +0000, Dan Scally wrote: > On 01/12/2020 18:49, Andy Shevchenko wrote: ... > > Seems we can do this, by locating intel_int3472.c under PDx86 hood and dropping > > ACPI ID table from TPS68470 MFD driver. The PMIC can be instantiated via > > i2c_acpi_new_device() (IIRC the API name). > > > > And actually it makes more sense since it's not and MFD and should not be there. > > > > (Dan, patch wise the one creates intel_int3472.c followed by another one that > > moves ACPI ID from PMIC and introduces its instantiation via I²C board info > > structure) > > I'm mostly following this, but why would we need an i2c_board_info or > i2c_acpi_new_device()? The INT3472 entries that refer to actual tps68470 > devices do have an I2cSerialBusV2 enumerated in _CRS so in their case > there's an i2c device registered with the kernel already. Because as we discussed already we can't have two drivers for the same ID without a big disruption in the driver(s). If you have a single point of enumeration, it will make things much easier (refer to the same intel_cht_int33fe driver you mentioned earlier). I just realize that the name of int3472 should follow the same pattern, i.e. intel_skl_int3472.c > I think we need those things when we get round to handling the > VCM/EEPROM that's hidden within the sensor's ACPI entry, but I've not > done any work on that yet at all. Let's consider this later — one step at a time.
On Tue, Dec 01, 2020 at 09:05:11PM +0000, Dan Scally wrote: > On 01/12/2020 19:21, Andy Shevchenko wrote: > > On Tue, Dec 01, 2020 at 09:06:38PM +0200, Laurent Pinchart wrote: ... > > I would rather ask Hans' opinion since he has quite an expertise with DMI for > > good and bad. > > > I have no real preference as to the current method or DMI, but thoughts > that come to mind are: > > > 1. given your info that low byte 0x0c means clock enable, we need to > register a clock too. Do we need to extend this device specific section > to map a clock name, or is it acceptable for them to be nameless (ISTR > that the API will let you fetch a clock using devm_clock_get(dev, NULL);) > > 2. Given only 0x0b pin is actually a regulator and it's controlling > multiple devices, my plan when we got round to adding the VCM / EEPROM > support was simply to extend those mapping tables so that those > supplementary devices were also able to get that regulator...and the two > would share it. I think, from reading the regulator code and > documentation, that that's all fine - and it won't actually be disabled > until both drivers disable it. Does that sound about right? Sounds right. Next step is to see the code. :-)
On 30/11/2020 17:29, Andy Shevchenko wrote: > On Mon, Nov 30, 2020 at 01:31:13PM +0000, Daniel Scally wrote: >> This function is used to find fwnode endpoints against a device. In >> some instances those endpoints are software nodes which are children of >> fwnode->secondary. Add support to fwnode_graph_get_endpoint_by_id() to >> find those endpoints by recursively calling itself passing the ptr to >> fwnode->secondary in the event no endpoint is found for the primary. > > One nit below, after addressing: > Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > > ... > >> + if (!best_ep && fwnode && !IS_ERR_OR_NULL(fwnode->secondary)) >> + return fwnode_graph_get_endpoint_by_id(fwnode->secondary, port, >> + endpoint, flags); > >> return best_ep; > > Can we, please, do > > if (best_ep) > return best_ep; > > if (fwnode && !IS_ERR_OR_NULL(fwnode->secondary)) > return fwnode_graph_get_endpoint_by_id(fwnode->secondary, port, > endpoint, flags); > > return NULL; > > ? > > This 'if (fwnode && !IS_ERR_OR_NULL(fwnode->secondary))' becomes kinda > idiomatic to the cases when we need to proceed primary followed by the > secondary in cases where it's not already done. Thanks - I made this change too
Hi Laurent, On Tue, Dec 01, 2020 at 08:37:58PM +0200, Laurent Pinchart wrote: > Hi Sakari, > > On Tue, Dec 01, 2020 at 05:55:13PM +0200, Sakari Ailus wrote: > > On Tue, Dec 01, 2020 at 01:32:32AM +0200, Laurent Pinchart wrote: > > > On Mon, Nov 30, 2020 at 10:07:19PM +0200, Andy Shevchenko wrote: > > > > On Mon, Nov 30, 2020 at 01:31:29PM +0000, Daniel Scally wrote: > > > > > On platforms where ACPI is designed for use with Windows, resources > > > > > that are intended to be consumed by sensor devices are sometimes in > > > > > the _CRS of a dummy INT3472 device upon which the sensor depends. This > > > > > driver binds to the dummy acpi device (which does not represent a > > > > > > > > acpi device -> acpi_device > > > > > > > > > physical PMIC) and maps them into GPIO lines and regulators for use by > > > > > the sensor device instead. > > > > > > > > ... > > > > > > > > > This patch contains the bits of this process that we're least sure about. > > > > > The sensors in scope for this work are called out as dependent (in their > > > > > DSDT entry's _DEP) on a device with _HID INT3472. These come in at least > > > > > 2 kinds; those with an I2cSerialBusV2 entry (which we presume therefore > > > > > are legitimate tps68470 PMICs that need handling by those drivers - work > > > > > on that in the future). And those without an I2C device. For those without > > > > > an I2C device they instead have an array of GPIO pins defined in _CRS. So > > > > > for example, my Lenovo Miix 510's OVTI2680 sensor is dependent on one of > > > > > the _latter_ kind of INT3472 devices, with this _CRS: > > > > > > > > > > Method (_CRS, 0, NotSerialized) // _CRS: Current Resource Settings > > > > > { > > > > > Name (SBUF, ResourceTemplate () > > > > > { > > > > > GpioIo (Exclusive, PullDefault, 0x0000, 0x0000, > > > > > IoRestrictionOutputOnly, "\\_SB.PCI0.GPI0", > > > > > 0x00, ResourceConsumer, , > > > > > ) > > > > > { // Pin list > > > > > 0x0079 > > > > > } > > > > > GpioIo (Exclusive, PullDefault, 0x0000, 0x0000, > > > > > IoRestrictionOutputOnly, "\\_SB.PCI0.GPI0", > > > > > 0x00, ResourceConsumer, , > > > > > ) > > > > > { // Pin list > > > > > 0x007A > > > > > } > > > > > GpioIo (Exclusive, PullDefault, 0x0000, 0x0000, > > > > > IoRestrictionOutputOnly, "\\_SB.PCI0.GPI0", > > > > > 0x00, ResourceConsumer, , > > > > > ) > > > > > { // Pin list > > > > > 0x008F > > > > > } > > > > > }) > > > > > Return (SBUF) /* \_SB_.PCI0.PMI1._CRS.SBUF */ > > > > > } > > > > > > > > > > and the same device has a _DSM Method, which returns 32-bit ints where > > > > > the second lowest byte we noticed to match the pin numbers of the GPIO > > > > > lines: > > > > > > > > > > Method (_DSM, 4, NotSerialized) // _DSM: Device-Specific Method > > > > > { > > > > > If ((Arg0 == ToUUID ("79234640-9e10-4fea-a5c1-b5aa8b19756f"))) > > > > > { > > > > > If ((Arg2 == One)) > > > > > { > > > > > Return (0x03) > > > > > } > > > > > > > > > > If ((Arg2 == 0x02)) > > > > > { > > > > > Return (0x01007900) > > > > > } > > > > > > > > > > If ((Arg2 == 0x03)) > > > > > { > > > > > Return (0x01007A0C) > > > > > } > > > > > > > > > > If ((Arg2 == 0x04)) > > > > > { > > > > > Return (0x01008F01) > > > > > } > > > > > } > > > > > > > > > > Return (Zero) > > > > > } > > > > > > > > > > We know that at least some of those pins have to be toggled active for the > > > > > sensor devices to be available in i2c, so the conclusion we came to was > > > > > that those GPIO entries assigned to the INT3472 device actually represent > > > > > GPIOs and regulators to be consumed by the sensors themselves. Tsuchiya > > > > > noticed that the lowest byte in the return values of the _DSM method > > > > > seemed to represent the type or function of the GPIO line, and we > > > > > confirmed that by testing on each surface device that GPIO lines where the > > > > > low byte in the _DSM entry for that pin was 0x0d controlled the privacy > > > > > LED of the cameras. > > > > > > > > > > We're guessing as to the exact meaning of the function byte, but I > > > > > conclude they're something like this: > > > > > > > > > > 0x00 - probably a reset GPIO > > > > > 0x01 - regulator for the sensor > > > > > 0x0c - regulator for the sensor > > > > > 0x0b - regulator again, but for a VCM or EEPROM > > > > > 0x0d - privacy led (only one we're totally confident of since we can see > > > > > it happen!) > > > > > > > > It's solely Windows driver design... > > > > Luckily I found some information and can clarify above table: > > > > > > > > 0x00 Reset > > > > 0x01 Power down > > > > 0x0b Power enable > > > > 0x0c Clock enable > > > > 0x0d LED (active high) > > > > > > That's very useful information ! Thank you. > > > > > > > The above text perhaps should go somewhere under Documentation. > > > > > > Or in the driver source code, but definitely somewhere else than in the > > > commit message. > > > > > > > > After much internal debate I decided to write this as a standalone > > > > > acpi_driver. Alternative options we considered: > > > > > > > > > > 1. Squash all this into the cio2-bridge code, which I did originally write > > > > > but decided I didn't like. > > > > > 2. Extend the existing tps68470 mfd driver...they share an ACPI ID so this > > > > > kinda makes sense, but ultimately given there is no actual physical > > > > > tps68470 in the scenario this patch handles I decided I didn't like this > > > > > either. > > > > > > > > Looking to this I think the best is to create a module that can be consumed by tps68470 and separately. > > > > So, something near to it rather than under ipu3 hood. > > > > > > > > You may use same ID's in both drivers (in PMIC less case it can be simple > > > > platform and thus they won't conflict), but both of them should provide GPIO > > > > resources for consumption. > > > > > > > > So, something like > > > > > > > > tps68470.h with API to consume > > > > split tps68470 to -core, -i2c parts > > > > add int3472, which will serve for above and be standalone platform driver > > > > update cio2-bridge accordingly > > > > > > > > Would it be feasible? > > > > > > Given that INT3472 means Intel camera power management device (that's > > > more or less the wording in Windows, I can double-check), would the > > > following make sense ? > > > > > > A top-level module named intel-camera-pmic (or int3472, or ...) would > > > register two drivers, a platform driver and an I2C driver, to > > > accommodate for both cases ("discrete PMIC" that doesn't have an > > > I2cSerialBusV2, and TPS64870 or uP6641Q that are I2C devices). The probe > > > function would perform the following: > > > > > > - If there's no CLDB, then the device uses the Chrome OS "ACPI > > > bindings", and refers to a TPS64870. The code that exists in the > > > kernel today (registering GPIOs, and registering an OpRegion to > > > communicate with the power management code in the DSDT) would be > > > activated. > > > > > > - If there's a CLDB, then the device type would be retrieved from it: > > > > > > - If the device is a "discrete PMIC", the driver would register clocks > > > and regulators controlled by GPIOs, and create clock, regulator and > > > GPIO lookup entries for the sensor device that references the PMIC. > > > > > > - If the device is a TPS64870, the code that exists in the kernel > > > today to register GPIOs would be activated, and new code would need > > > to be written to register regulators and clocks. > > > > > > - If the device is a uP6641Q, a new driver will need to be written (I > > > don't know on which devices this PMIC is used, so this can probably > > > be deferred). > > > > > > We can split this in multiple files and/or modules. > > > > That's what I thought of, too, as one option, but with some more detail. > > This would be indeed the cleanest option. > > > > I think it'd be nice if the CLDB stuff (apart from checking whether it's > > there) would be in a different module to avoid cluttering up the real > > tps68470 driver. > > Given the amount of code, and the fact that the driver should be > compiled as a module, I don't think it will make a huge difference in > the memory footprint. I'd still prefer to keep the ACPI hack support and the real driver well separated. That way it'd be also easy to put them to their respective modules. That's actually how the tps68470 MFD driver is currently arranged; the GPIO and OP region drivers are separate from each other. Could this be just one more platform device for each of the three cases (or one for the two latter; I'm not quite sure yet)? The GPIO regulator case is relatively safe, but the real PMICs require regulator voltage control as well as enabling and disabling the regulators. That probably requires either schematics or checking the register values at runtime on Windows (i.e. finding out which system you're dealing with, at runtime). -- Kind regards, Sakari Ailus
On Wed, Dec 02, 2020 at 11:39:52AM +0200, Andy Shevchenko wrote: > On Tue, Dec 01, 2020 at 08:59:53PM +0000, Dan Scally wrote: > > On 01/12/2020 18:49, Andy Shevchenko wrote: > > ... > > > > Seems we can do this, by locating intel_int3472.c under PDx86 hood and dropping > > > ACPI ID table from TPS68470 MFD driver. The PMIC can be instantiated via > > > i2c_acpi_new_device() (IIRC the API name). > > > > > > And actually it makes more sense since it's not and MFD and should not be there. > > > > > > (Dan, patch wise the one creates intel_int3472.c followed by another one that > > > moves ACPI ID from PMIC and introduces its instantiation via I²C board info > > > structure) > > > > I'm mostly following this, but why would we need an i2c_board_info or > > i2c_acpi_new_device()? The INT3472 entries that refer to actual tps68470 > > devices do have an I2cSerialBusV2 enumerated in _CRS so in their case > > there's an i2c device registered with the kernel already. > > Because as we discussed already we can't have two drivers for the same ID > without a big disruption in the driver(s). > > If you have a single point of enumeration, it will make things much easier > (refer to the same intel_cht_int33fe driver you mentioned earlier). > > I just realize that the name of int3472 should follow the same pattern, i.e. > intel_skl_int3472.c We're mostly focussing on Kaby Lake here though. From what I understand the ACPI infrastructure for camera support is mostly the same on Sky Lake, but not identical. I think a single driver should be able to cover both though. > > I think we need those things when we get round to handling the > > VCM/EEPROM that's hidden within the sensor's ACPI entry, but I've not > > done any work on that yet at all. > > Let's consider this later — one step at a time. -- Regards, Laurent Pinchart
On Wed, Dec 02, 2020 at 01:09:56PM +0200, Sakari Ailus wrote: > Hi Laurent, > > On Tue, Dec 01, 2020 at 08:37:58PM +0200, Laurent Pinchart wrote: > > Hi Sakari, > > > > On Tue, Dec 01, 2020 at 05:55:13PM +0200, Sakari Ailus wrote: > > > On Tue, Dec 01, 2020 at 01:32:32AM +0200, Laurent Pinchart wrote: > > > > On Mon, Nov 30, 2020 at 10:07:19PM +0200, Andy Shevchenko wrote: > > > > > On Mon, Nov 30, 2020 at 01:31:29PM +0000, Daniel Scally wrote: > > > > > > On platforms where ACPI is designed for use with Windows, resources > > > > > > that are intended to be consumed by sensor devices are sometimes in > > > > > > the _CRS of a dummy INT3472 device upon which the sensor depends. This > > > > > > driver binds to the dummy acpi device (which does not represent a > > > > > > > > > > acpi device -> acpi_device > > > > > > > > > > > physical PMIC) and maps them into GPIO lines and regulators for use by > > > > > > the sensor device instead. > > > > > > > > > > ... > > > > > > > > > > > This patch contains the bits of this process that we're least sure about. > > > > > > The sensors in scope for this work are called out as dependent (in their > > > > > > DSDT entry's _DEP) on a device with _HID INT3472. These come in at least > > > > > > 2 kinds; those with an I2cSerialBusV2 entry (which we presume therefore > > > > > > are legitimate tps68470 PMICs that need handling by those drivers - work > > > > > > on that in the future). And those without an I2C device. For those without > > > > > > an I2C device they instead have an array of GPIO pins defined in _CRS. So > > > > > > for example, my Lenovo Miix 510's OVTI2680 sensor is dependent on one of > > > > > > the _latter_ kind of INT3472 devices, with this _CRS: > > > > > > > > > > > > Method (_CRS, 0, NotSerialized) // _CRS: Current Resource Settings > > > > > > { > > > > > > Name (SBUF, ResourceTemplate () > > > > > > { > > > > > > GpioIo (Exclusive, PullDefault, 0x0000, 0x0000, > > > > > > IoRestrictionOutputOnly, "\\_SB.PCI0.GPI0", > > > > > > 0x00, ResourceConsumer, , > > > > > > ) > > > > > > { // Pin list > > > > > > 0x0079 > > > > > > } > > > > > > GpioIo (Exclusive, PullDefault, 0x0000, 0x0000, > > > > > > IoRestrictionOutputOnly, "\\_SB.PCI0.GPI0", > > > > > > 0x00, ResourceConsumer, , > > > > > > ) > > > > > > { // Pin list > > > > > > 0x007A > > > > > > } > > > > > > GpioIo (Exclusive, PullDefault, 0x0000, 0x0000, > > > > > > IoRestrictionOutputOnly, "\\_SB.PCI0.GPI0", > > > > > > 0x00, ResourceConsumer, , > > > > > > ) > > > > > > { // Pin list > > > > > > 0x008F > > > > > > } > > > > > > }) > > > > > > Return (SBUF) /* \_SB_.PCI0.PMI1._CRS.SBUF */ > > > > > > } > > > > > > > > > > > > and the same device has a _DSM Method, which returns 32-bit ints where > > > > > > the second lowest byte we noticed to match the pin numbers of the GPIO > > > > > > lines: > > > > > > > > > > > > Method (_DSM, 4, NotSerialized) // _DSM: Device-Specific Method > > > > > > { > > > > > > If ((Arg0 == ToUUID ("79234640-9e10-4fea-a5c1-b5aa8b19756f"))) > > > > > > { > > > > > > If ((Arg2 == One)) > > > > > > { > > > > > > Return (0x03) > > > > > > } > > > > > > > > > > > > If ((Arg2 == 0x02)) > > > > > > { > > > > > > Return (0x01007900) > > > > > > } > > > > > > > > > > > > If ((Arg2 == 0x03)) > > > > > > { > > > > > > Return (0x01007A0C) > > > > > > } > > > > > > > > > > > > If ((Arg2 == 0x04)) > > > > > > { > > > > > > Return (0x01008F01) > > > > > > } > > > > > > } > > > > > > > > > > > > Return (Zero) > > > > > > } > > > > > > > > > > > > We know that at least some of those pins have to be toggled active for the > > > > > > sensor devices to be available in i2c, so the conclusion we came to was > > > > > > that those GPIO entries assigned to the INT3472 device actually represent > > > > > > GPIOs and regulators to be consumed by the sensors themselves. Tsuchiya > > > > > > noticed that the lowest byte in the return values of the _DSM method > > > > > > seemed to represent the type or function of the GPIO line, and we > > > > > > confirmed that by testing on each surface device that GPIO lines where the > > > > > > low byte in the _DSM entry for that pin was 0x0d controlled the privacy > > > > > > LED of the cameras. > > > > > > > > > > > > We're guessing as to the exact meaning of the function byte, but I > > > > > > conclude they're something like this: > > > > > > > > > > > > 0x00 - probably a reset GPIO > > > > > > 0x01 - regulator for the sensor > > > > > > 0x0c - regulator for the sensor > > > > > > 0x0b - regulator again, but for a VCM or EEPROM > > > > > > 0x0d - privacy led (only one we're totally confident of since we can see > > > > > > it happen!) > > > > > > > > > > It's solely Windows driver design... > > > > > Luckily I found some information and can clarify above table: > > > > > > > > > > 0x00 Reset > > > > > 0x01 Power down > > > > > 0x0b Power enable > > > > > 0x0c Clock enable > > > > > 0x0d LED (active high) > > > > > > > > That's very useful information ! Thank you. > > > > > > > > > The above text perhaps should go somewhere under Documentation. > > > > > > > > Or in the driver source code, but definitely somewhere else than in the > > > > commit message. > > > > > > > > > > After much internal debate I decided to write this as a standalone > > > > > > acpi_driver. Alternative options we considered: > > > > > > > > > > > > 1. Squash all this into the cio2-bridge code, which I did originally write > > > > > > but decided I didn't like. > > > > > > 2. Extend the existing tps68470 mfd driver...they share an ACPI ID so this > > > > > > kinda makes sense, but ultimately given there is no actual physical > > > > > > tps68470 in the scenario this patch handles I decided I didn't like this > > > > > > either. > > > > > > > > > > Looking to this I think the best is to create a module that can be consumed by tps68470 and separately. > > > > > So, something near to it rather than under ipu3 hood. > > > > > > > > > > You may use same ID's in both drivers (in PMIC less case it can be simple > > > > > platform and thus they won't conflict), but both of them should provide GPIO > > > > > resources for consumption. > > > > > > > > > > So, something like > > > > > > > > > > tps68470.h with API to consume > > > > > split tps68470 to -core, -i2c parts > > > > > add int3472, which will serve for above and be standalone platform driver > > > > > update cio2-bridge accordingly > > > > > > > > > > Would it be feasible? > > > > > > > > Given that INT3472 means Intel camera power management device (that's > > > > more or less the wording in Windows, I can double-check), would the > > > > following make sense ? > > > > > > > > A top-level module named intel-camera-pmic (or int3472, or ...) would > > > > register two drivers, a platform driver and an I2C driver, to > > > > accommodate for both cases ("discrete PMIC" that doesn't have an > > > > I2cSerialBusV2, and TPS64870 or uP6641Q that are I2C devices). The probe > > > > function would perform the following: > > > > > > > > - If there's no CLDB, then the device uses the Chrome OS "ACPI > > > > bindings", and refers to a TPS64870. The code that exists in the > > > > kernel today (registering GPIOs, and registering an OpRegion to > > > > communicate with the power management code in the DSDT) would be > > > > activated. > > > > > > > > - If there's a CLDB, then the device type would be retrieved from it: > > > > > > > > - If the device is a "discrete PMIC", the driver would register clocks > > > > and regulators controlled by GPIOs, and create clock, regulator and > > > > GPIO lookup entries for the sensor device that references the PMIC. > > > > > > > > - If the device is a TPS64870, the code that exists in the kernel > > > > today to register GPIOs would be activated, and new code would need > > > > to be written to register regulators and clocks. > > > > > > > > - If the device is a uP6641Q, a new driver will need to be written (I > > > > don't know on which devices this PMIC is used, so this can probably > > > > be deferred). > > > > > > > > We can split this in multiple files and/or modules. > > > > > > That's what I thought of, too, as one option, but with some more detail. > > > This would be indeed the cleanest option. > > > > > > I think it'd be nice if the CLDB stuff (apart from checking whether it's > > > there) would be in a different module to avoid cluttering up the real > > > tps68470 driver. > > > > Given the amount of code, and the fact that the driver should be > > compiled as a module, I don't think it will make a huge difference in > > the memory footprint. > > I'd still prefer to keep the ACPI hack support and the real driver well > separated. That way it'd be also easy to put them to their respective > modules. That's actually how the tps68470 MFD driver is currently arranged; > the GPIO and OP region drivers are separate from each other. I think we should consider ACPI to be a hack in the first place :-) > Could this be just one more platform device for each of the three cases (or > one for the two latter; I'm not quite sure yet)? Using MFD for this seems a bit overkill to me. I won't care much as I won't maintain those drivers, but the current situation is complex enough, it was hard for me to understand how things worked. Adding yet another layer with another platform device won't make it any simpler. If we want to split this in two, I'd rather have a tps68470 driver on one side, without ACPI op region support, but registering regulators, GPIOs and clocks (without using separate drivers and devices for these three features), and an INT3472 driver on the other side, with all the ACPI glue and hacks. The tps68470 code could possibly even be structured in such a way that it would be used as a library by the INT3472 driver instead of requiring a separate platform device. > The GPIO regulator case is relatively safe, but the real PMICs require > regulator voltage control as well as enabling and disabling the regulators. > That probably requires either schematics or checking the register values at > runtime on Windows (i.e. finding out which system you're dealing with, at > runtime). -- Regards, Laurent Pinchart
Hi Hans, On Tue, Dec 01, 2020 at 09:34:58PM +0100, Hans de Goede wrote: > On 12/1/20 8:21 PM, Andy Shevchenko wrote: > > On Tue, Dec 01, 2020 at 09:06:38PM +0200, Laurent Pinchart wrote: > >> On Tue, Dec 01, 2020 at 09:05:23PM +0200, Andy Shevchenko wrote: > >>> On Tue, Dec 01, 2020 at 08:55:48PM +0200, Laurent Pinchart wrote: > >>>> On Tue, Dec 01, 2020 at 08:54:17PM +0200, Andy Shevchenko wrote: > >>>>> On Tue, Dec 01, 2020 at 08:30:03AM +0000, Dan Scally wrote: > >>>>>> On 30/11/2020 20:07, Andy Shevchenko wrote: > >>> > >>> ... > >>> > >>>>>>>> +static struct int3472_sensor_regulator_map int3472_sensor_regulator_maps[] = { > >>>>>>>> + { "GNDF140809R", 2, miix_510_ov2680 }, > >>>>>>>> + { "YHCU", 2, surface_go2_ov5693 }, > >>>>>>>> + { "MSHW0070", 2, surface_book_ov5693 }, > >>>>>>>> +}; > >>>>>>> > >>>>>>> Hmm... Usual way is to use DMI for that. I'm not sure above will not give us > >>>>>>> false positive matches. > >>>>>> > >>>>>> I considered DMI too, no problem to switch to that if it's a better choice. > >>>>> > >>>>> I prefer DMI as it's a standard way to describe platform quirks in x86 world. > >>>> > >>>> Do you think the Windows driver would use DMI ? > >>> > >>> Linux is using DMI for quirks. > >>> > >>>> That seems quite > >>>> unlikely to me, given how they would have to release a new driver binary > >>>> for every machine. I'm pretty sure that a different mechanism is used to > >>>> identify camera integration, and I think it would make sense to follow > >>>> the same approach. That would allow us to avoid large tables of DMI > >>>> identifiers that would need to be constently updated, potentially making > >>>> user experience better. > >>> > >>> All Surface family can be matched in a way as Apple machines [1]. > >>> > >>> [1]: https://lkml.org/lkml/2020/4/15/1198 > >> > >> But not all Surface machines necessarily have the same camera > >> architecture. My point is that there seems to be identifiers reported in > >> ACPI for the exact purpose of identifying the camera architecture. If we > >> used DMI instead, we would have to handle each machine individually. > > > > With help of DMI we may narrow down the search. > > > > But again, we are talking about uncertainity. It may be your way (a lot of > > platforms that have different settings), or mine (only a few with more or less > > standard sets of settings). > > > > DMI is simply standard in Linux (people usually easier can grep for quirks for > > a specific platform). > > > > I would rather ask Hans' opinion since he has quite an expertise with DMI for > > good and bad. > > So generally there are 2 ways how things like this can go: > > 1) There is sufficient information in the ACPI table and we use data from the > ACPI tables > > 2) There is unsufficient info in the ACPI tables (or we don't know how to > get / interpret the data) and we use DMI quirks And this specific case I believe there is sufficient data in the ACPI tables, as I don't believe the Windows driver uses DMI quirks, or comes in the form of machine-specific binaries. We however don't know how to interpret all the data, but that should hopefully get better over time (especially as we'll get more data points, with ACPI dumps from machines whose schematics have leaked). > Although we do often also use a combination, getting what we can from ACPI, > combined with a set of defaults for what we cannot get from ACPI > based on what reference designs use (IOW what most devices seem to have > copy and pasted). Combined with DMI quirks for when the defaults do not > work (which is quite often). > > Depending on if "not working because of wrong defaults" has bad side effects, > another option is also to only allow the driver to load on devices which > have the necessary info provided through a DMI match. Right now there shouldn't be bad side effects, but in the future we'll need to setup a PMIC whose output voltages can be controlled, and getting it wrong would be very bad. For that I'll definitely vote for DMI match to start with, but I don't think that precludes using data from ACPI. We could just prevent the driver from loading if the machine isn't whitelisted in DMI matches, and still use ACPI data. > I hope this helps.
On Wed, Dec 02, 2020 at 02:42:28PM +0200, Laurent Pinchart wrote: > On Wed, Dec 02, 2020 at 01:09:56PM +0200, Sakari Ailus wrote: > > On Tue, Dec 01, 2020 at 08:37:58PM +0200, Laurent Pinchart wrote: ... > I think we should consider ACPI to be a hack in the first place :-) I feel that about DT (and all chaos around it) but it's not a topic here. > > Could this be just one more platform device for each of the three cases (or > > one for the two latter; I'm not quite sure yet)? > > Using MFD for this seems a bit overkill to me. I won't care much as I > won't maintain those drivers, but the current situation is complex > enough, it was hard for me to understand how things worked. Adding yet > another layer with another platform device won't make it any simpler. > > If we want to split this in two, I'd rather have a tps68470 driver on > one side, without ACPI op region support, but registering regulators, > GPIOs and clocks (without using separate drivers and devices for these > three features), and an INT3472 driver on the other side, with all the > ACPI glue and hacks. The tps68470 code could possibly even be structured > in such a way that it would be used as a library by the INT3472 driver > instead of requiring a separate platform device. I'm afraid TPS68470 is MFD in hardware and its representation in the MFD is fine. What we need is to move IN3472 pieces out from it. And I agree with your proposal in general. > > The GPIO regulator case is relatively safe, but the real PMICs require > > regulator voltage control as well as enabling and disabling the regulators. > > That probably requires either schematics or checking the register values at > > runtime on Windows (i.e. finding out which system you're dealing with, at > > runtime).
On Wed, Dec 02, 2020 at 02:35:40PM +0200, Laurent Pinchart wrote: > On Wed, Dec 02, 2020 at 11:39:52AM +0200, Andy Shevchenko wrote: > > On Tue, Dec 01, 2020 at 08:59:53PM +0000, Dan Scally wrote: > > > On 01/12/2020 18:49, Andy Shevchenko wrote: ... > > I just realize that the name of int3472 should follow the same pattern, i.e. > > intel_skl_int3472.c > > We're mostly focussing on Kaby Lake here though. From what I understand > the ACPI infrastructure for camera support is mostly the same on Sky > Lake, but not identical. I think a single driver should be able to cover > both though. They (KBL and SKL) are sharing a lot, so I would name if after Skylake and use for Kaby Lake as well.
On Wed, Dec 02, 2020 at 02:48:47PM +0200, Laurent Pinchart wrote: > On Tue, Dec 01, 2020 at 09:34:58PM +0100, Hans de Goede wrote: > > On 12/1/20 8:21 PM, Andy Shevchenko wrote: > > > On Tue, Dec 01, 2020 at 09:06:38PM +0200, Laurent Pinchart wrote: ... > > > I would rather ask Hans' opinion since he has quite an expertise with DMI for > > > good and bad. > > > > So generally there are 2 ways how things like this can go: > > > > 1) There is sufficient information in the ACPI table and we use data from the > > ACPI tables > > > > 2) There is unsufficient info in the ACPI tables (or we don't know how to > > get / interpret the data) and we use DMI quirks > > And this specific case I believe there is sufficient data in the ACPI > tables, as I don't believe the Windows driver uses DMI quirks, or comes > in the form of machine-specific binaries. We however don't know how to > interpret all the data, but that should hopefully get better over time > (especially as we'll get more data points, with ACPI dumps from machines > whose schematics have leaked). I think you are too optimistic about this part of Windows drivers. I would rather think about hardware stuck with the same frequencies which simply are hard coded in the Windows driver. I have description of ASL for this camera, but I don't see anything like this you are describing. > > Although we do often also use a combination, getting what we can from ACPI, > > combined with a set of defaults for what we cannot get from ACPI > > based on what reference designs use (IOW what most devices seem to have > > copy and pasted). Combined with DMI quirks for when the defaults do not > > work (which is quite often). > > > > Depending on if "not working because of wrong defaults" has bad side effects, > > another option is also to only allow the driver to load on devices which > > have the necessary info provided through a DMI match. > > Right now there shouldn't be bad side effects, but in the future we'll > need to setup a PMIC whose output voltages can be controlled, and > getting it wrong would be very bad. For that I'll definitely vote for > DMI match to start with, but I don't think that precludes using data > from ACPI. We could just prevent the driver from loading if the machine > isn't whitelisted in DMI matches, and still use ACPI data. I also think about DMI as a narrowing scope of supported platforms. -- With Best Regards, Andy Shevchenko
On 02/12/2020 09:39, Andy Shevchenko wrote: > On Tue, Dec 01, 2020 at 08:59:53PM +0000, Dan Scally wrote: >> On 01/12/2020 18:49, Andy Shevchenko wrote: > > ... > >>> Seems we can do this, by locating intel_int3472.c under PDx86 hood and dropping >>> ACPI ID table from TPS68470 MFD driver. The PMIC can be instantiated via >>> i2c_acpi_new_device() (IIRC the API name). >>> >>> And actually it makes more sense since it's not and MFD and should not be there. >>> >>> (Dan, patch wise the one creates intel_int3472.c followed by another one that >>> moves ACPI ID from PMIC and introduces its instantiation via I²C board info >>> structure) >> >> I'm mostly following this, but why would we need an i2c_board_info or >> i2c_acpi_new_device()? The INT3472 entries that refer to actual tps68470 >> devices do have an I2cSerialBusV2 enumerated in _CRS so in their case >> there's an i2c device registered with the kernel already. > > Because as we discussed already we can't have two drivers for the same ID > without a big disruption in the driver(s). > > If you have a single point of enumeration, it will make things much easier > (refer to the same intel_cht_int33fe driver you mentioned earlier). > > I just realize that the name of int3472 should follow the same pattern, i.e. > intel_skl_int3472.c Ah! I didn't read intel_cht_int33fe_common.c before, just the typec.c. Having reviewed common I think I'm clear on the method now, thank you :) >> I think we need those things when we get round to handling the >> VCM/EEPROM that's hidden within the sensor's ACPI entry, but I've not >> done any work on that yet at all. > > Let's consider this later — one step at a time. Agree!
On 02/12/2020 15:08, Andy Shevchenko wrote: > On Wed, Dec 02, 2020 at 02:42:28PM +0200, Laurent Pinchart wrote: >> On Wed, Dec 02, 2020 at 01:09:56PM +0200, Sakari Ailus wrote: >>> On Tue, Dec 01, 2020 at 08:37:58PM +0200, Laurent Pinchart wrote: > > ... > >> I think we should consider ACPI to be a hack in the first place :-) > > I feel that about DT (and all chaos around it) but it's not a topic here. > >>> Could this be just one more platform device for each of the three cases (or >>> one for the two latter; I'm not quite sure yet)? >> >> Using MFD for this seems a bit overkill to me. I won't care much as I >> won't maintain those drivers, but the current situation is complex >> enough, it was hard for me to understand how things worked. Adding yet >> another layer with another platform device won't make it any simpler. >> >> If we want to split this in two, I'd rather have a tps68470 driver on >> one side, without ACPI op region support, but registering regulators, >> GPIOs and clocks (without using separate drivers and devices for these >> three features), and an INT3472 driver on the other side, with all the >> ACPI glue and hacks. The tps68470 code could possibly even be structured >> in such a way that it would be used as a library by the INT3472 driver >> instead of requiring a separate platform device. > > I'm afraid TPS68470 is MFD in hardware and its representation in the MFD is > fine. What we need is to move IN3472 pieces out from it. > > And I agree with your proposal in general. Way back when I first joined this project we thought we needed i2c drivers for driving the tps68470's clks and regulators. Tsuchiya found some in an old Intel tree; they needed some minor tweaks but nothing drastic. And I think they're designed to work with the mfd driver that's already in the kernel. So, can we do this by just checking (in a new platform/x86/intel_skl_int3472.c) for a CLDB buffer in the PMIC, and calling devm_mfd_add_devices() with either the GPIO and OpRegion drivers (if no CLDB buffer found) or with the GPIO, clk and regulator drivers (If there's a CLDB and it's not a discrete PMIC). Or else, using the code from this patch directly in the platform driver if the CLDB says it's a discrete PMIC? >>> The GPIO regulator case is relatively safe, but the real PMICs require >>> regulator voltage control as well as enabling and disabling the regulators. >>> That probably requires either schematics or checking the register values at >>> runtime on Windows (i.e. finding out which system you're dealing with, at >>> runtime). >
On Thu, Dec 03, 2020 at 12:37:12PM +0000, Dan Scally wrote: > On 02/12/2020 15:08, Andy Shevchenko wrote: > > On Wed, Dec 02, 2020 at 02:42:28PM +0200, Laurent Pinchart wrote: > >> On Wed, Dec 02, 2020 at 01:09:56PM +0200, Sakari Ailus wrote: > >>> On Tue, Dec 01, 2020 at 08:37:58PM +0200, Laurent Pinchart wrote: > > > > ... > > > >> I think we should consider ACPI to be a hack in the first place :-) > > > > I feel that about DT (and all chaos around it) but it's not a topic here. > > > >>> Could this be just one more platform device for each of the three cases (or > >>> one for the two latter; I'm not quite sure yet)? > >> > >> Using MFD for this seems a bit overkill to me. I won't care much as I > >> won't maintain those drivers, but the current situation is complex > >> enough, it was hard for me to understand how things worked. Adding yet > >> another layer with another platform device won't make it any simpler. > >> > >> If we want to split this in two, I'd rather have a tps68470 driver on > >> one side, without ACPI op region support, but registering regulators, > >> GPIOs and clocks (without using separate drivers and devices for these > >> three features), and an INT3472 driver on the other side, with all the > >> ACPI glue and hacks. The tps68470 code could possibly even be structured > >> in such a way that it would be used as a library by the INT3472 driver > >> instead of requiring a separate platform device. > > > > I'm afraid TPS68470 is MFD in hardware and its representation in the MFD is > > fine. What we need is to move IN3472 pieces out from it. > > > > And I agree with your proposal in general. > > Way back when I first joined this project we thought we needed i2c > drivers for driving the tps68470's clks and regulators. Tsuchiya found > some in an old Intel tree; they needed some minor tweaks but nothing > drastic. And I think they're designed to work with the mfd driver that's > already in the kernel. > > So, can we do this by just checking (in a new > platform/x86/intel_skl_int3472.c) for a CLDB buffer in the PMIC, and > calling devm_mfd_add_devices() with either the GPIO and OpRegion drivers > (if no CLDB buffer found) or with the GPIO, clk and regulator drivers > (If there's a CLDB and it's not a discrete PMIC). Or else, using the > code from this patch directly in the platform driver if the CLDB says > it's a discrete PMIC? Lee, who is a maintainer of MFD, is quite sensitive about this. I don't think he would approve this (however I see 8 drivers that are using MFD API out of drivers/mfd).
On 01/12/2020 18:49, Andy Shevchenko wrote: >>>> + table_entry = (struct gpiod_lookup)GPIO_LOOKUP_IDX(acpi_dev_name(adev), >>>> + ares->data.gpio.pin_table[0], >>>> + func, 0, GPIO_ACTIVE_HIGH); >>> >>> You won't need this if you have regular INT3472 platform driver. >>> Simple call there _DSM to map resources to the type and use devm_gpiod_get on >>> consumer behalf. Thus, previous patch is not needed. >> >> How does the consumer (the camera sensor) retrieve the GPIO though ? The >> _DSM is in the PMIC device object, while the real consumer is the camera >> sensor. > > 1. A GPIO proxy > 2. A custom GPIO lookup tables > 3. An fwnode passing to the sensor (via swnodes graph) > > First may issue deferred probe, while second needs some ordering tricks I guess. > Third one should also provide an ACPI GPIO mapping table or so to make the > consumer rely on names rather than custom numbers. > > Perhaps someone may propose other solutions. Hi Andy Sorry; some more clarification here if you have time please: 1. Do you mean here, register a new gpio_chip providing GPIOs to the sensors, and just have the .set() callback for that function set the corresponding line against the INT3472 device? 2. I thought custom GPIO lookup tables was what I was doing, are you referring to something else? 3. I guess you mean something like of_find_gpio() and acpi_find_gpio() here? As far as I can see there isn't currently a swnodes equivalent...we could just pass it via reference of course but it would mean the sensor drivers would all need to account for that.
On Sun, Dec 13, 2020 at 10:48:39PM +0000, Daniel Scally wrote: > On 01/12/2020 18:49, Andy Shevchenko wrote: > >>>> + table_entry = (struct gpiod_lookup)GPIO_LOOKUP_IDX(acpi_dev_name(adev), > >>>> + ares->data.gpio.pin_table[0], > >>>> + func, 0, GPIO_ACTIVE_HIGH); > >>> > >>> You won't need this if you have regular INT3472 platform driver. > >>> Simple call there _DSM to map resources to the type and use devm_gpiod_get on > >>> consumer behalf. Thus, previous patch is not needed. > >> > >> How does the consumer (the camera sensor) retrieve the GPIO though ? The > >> _DSM is in the PMIC device object, while the real consumer is the camera > >> sensor. > > > > 1. A GPIO proxy > > 2. A custom GPIO lookup tables > > 3. An fwnode passing to the sensor (via swnodes graph) > > > > First may issue deferred probe, while second needs some ordering tricks I guess. > > Third one should also provide an ACPI GPIO mapping table or so to make the > > consumer rely on names rather than custom numbers. > > > > Perhaps someone may propose other solutions. > > Hi Andy > > Sorry; some more clarification here if you have time please: No problem, thanks for discussing this. > 1. Do you mean here, register a new gpio_chip providing GPIOs to the > sensors, and just have the .set() callback for that function set the > corresponding line against the INT3472 device? Yes. On one hand it should be a consumer (*gpiod_get*() family of APIs), on the other it should be provider of known (artificial) GPIO chip. > 2. I thought custom GPIO lookup tables was what I was doing, are you > referring to something else? I think so, i.e. nothing else from high point of view. > 3. I guess you mean something like of_find_gpio() and acpi_find_gpio() > here? As far as I can see there isn't currently a swnodes > equivalent...we could just pass it via reference of course but it would > mean the sensor drivers would all need to account for that. Theoretically we may provide GPIOs as swnodes. In that case the consumer will get them as usual But I think it may be too complicated / over engineered.
Hi Andy, all On 30/11/2020 20:07, Andy Shevchenko wrote: > On Mon, Nov 30, 2020 at 01:31:29PM +0000, Daniel Scally wrote: >> On platforms where ACPI is designed for use with Windows, resources >> that are intended to be consumed by sensor devices are sometimes in >> the _CRS of a dummy INT3472 device upon which the sensor depends. This >> driver binds to the dummy acpi device (which does not represent a > acpi device -> acpi_device > >> physical PMIC) and maps them into GPIO lines and regulators for use by >> the sensor device instead. > ... > >> This patch contains the bits of this process that we're least sure about. >> The sensors in scope for this work are called out as dependent (in their >> DSDT entry's _DEP) on a device with _HID INT3472. These come in at least >> 2 kinds; those with an I2cSerialBusV2 entry (which we presume therefore >> are legitimate tps68470 PMICs that need handling by those drivers - work >> on that in the future). And those without an I2C device. For those without >> an I2C device they instead have an array of GPIO pins defined in _CRS. So >> for example, my Lenovo Miix 510's OVTI2680 sensor is dependent on one of >> the _latter_ kind of INT3472 devices, with this _CRS: >> >> Method (_CRS, 0, NotSerialized) // _CRS: Current Resource Settings >> { >> Name (SBUF, ResourceTemplate () >> { >> GpioIo (Exclusive, PullDefault, 0x0000, 0x0000, >> IoRestrictionOutputOnly, "\\_SB.PCI0.GPI0", >> 0x00, ResourceConsumer, , >> ) >> { // Pin list >> 0x0079 >> } >> GpioIo (Exclusive, PullDefault, 0x0000, 0x0000, >> IoRestrictionOutputOnly, "\\_SB.PCI0.GPI0", >> 0x00, ResourceConsumer, , >> ) >> { // Pin list >> 0x007A >> } >> GpioIo (Exclusive, PullDefault, 0x0000, 0x0000, >> IoRestrictionOutputOnly, "\\_SB.PCI0.GPI0", >> 0x00, ResourceConsumer, , >> ) >> { // Pin list >> 0x008F >> } >> }) >> Return (SBUF) /* \_SB_.PCI0.PMI1._CRS.SBUF */ >> } >> >> and the same device has a _DSM Method, which returns 32-bit ints where >> the second lowest byte we noticed to match the pin numbers of the GPIO >> lines: >> >> Method (_DSM, 4, NotSerialized) // _DSM: Device-Specific Method >> { >> If ((Arg0 == ToUUID ("79234640-9e10-4fea-a5c1-b5aa8b19756f"))) >> { >> If ((Arg2 == One)) >> { >> Return (0x03) >> } >> >> If ((Arg2 == 0x02)) >> { >> Return (0x01007900) >> } >> >> If ((Arg2 == 0x03)) >> { >> Return (0x01007A0C) >> } >> >> If ((Arg2 == 0x04)) >> { >> Return (0x01008F01) >> } >> } >> >> Return (Zero) >> } >> >> We know that at least some of those pins have to be toggled active for the >> sensor devices to be available in i2c, so the conclusion we came to was >> that those GPIO entries assigned to the INT3472 device actually represent >> GPIOs and regulators to be consumed by the sensors themselves. Tsuchiya >> noticed that the lowest byte in the return values of the _DSM method >> seemed to represent the type or function of the GPIO line, and we >> confirmed that by testing on each surface device that GPIO lines where the >> low byte in the _DSM entry for that pin was 0x0d controlled the privacy >> LED of the cameras. >> >> We're guessing as to the exact meaning of the function byte, but I >> conclude they're something like this: >> >> 0x00 - probably a reset GPIO >> 0x01 - regulator for the sensor >> 0x0c - regulator for the sensor >> 0x0b - regulator again, but for a VCM or EEPROM >> 0x0d - privacy led (only one we're totally confident of since we can see >> it happen!) > It's solely Windows driver design... > Luckily I found some information and can clarify above table: > > 0x00 Reset > 0x01 Power down > 0x0b Power enable > 0x0c Clock enable > 0x0d LED (active high) > > The above text perhaps should go somewhere under Documentation. Coming back to this; there's a bit of an anomaly with the 0x01 Power Down pin for at least one platform. As listed above, the OV2680 on one of my platforms has 3 GPIOs defined, and the table above gives them as type Reset, Power down and Clock enable. I'd assumed from this table that "power down" meant a powerdown GPIO (I.E. the one usually called PWDNB in Omnivision datasheets and "powerdown" in drivers), but the datasheet for the OV2680 doesn't list a separate reset and powerdown pin, but rather a single pin that performs both functions. Am I wrong to treat that as something that ought to be mapped as a powerdown GPIO to the sensors? Or do you know of any other way to reconcile that discrepancy? Failing that; the only way I can think to handle this is to register proxy GPIO pins assigned to the sensors as you suggested previously, and have them toggle the GPIO's assigned to the INT3472 based on platform specific mapping data (I.E. we register a pin called "reset", which on most platforms just toggles the 0x00 pin, but on this specific platform would drive both 0x00 and 0x01 together. We're already heading that way for the regulator consumer supplies so it's sort of nothing new, but I'd still rather not if it can be avoided.
On Fri, Jan 8, 2021 at 1:56 AM Daniel Scally <djrscally@gmail.com> wrote: > On 30/11/2020 20:07, Andy Shevchenko wrote: > > On Mon, Nov 30, 2020 at 01:31:29PM +0000, Daniel Scally wrote: ... > > It's solely Windows driver design... > > Luckily I found some information and can clarify above table: > > > > 0x00 Reset > > 0x01 Power down > > 0x0b Power enable > > 0x0c Clock enable > > 0x0d LED (active high) > > > > The above text perhaps should go somewhere under Documentation. > > Coming back to this; there's a bit of an anomaly with the 0x01 Power > Down pin for at least one platform. As listed above, the OV2680 on one > of my platforms has 3 GPIOs defined, and the table above gives them as > type Reset, Power down and Clock enable. I'd assumed from this table > that "power down" meant a powerdown GPIO (I.E. the one usually called > PWDNB in Omnivision datasheets and "powerdown" in drivers), but the > datasheet for the OV2680 doesn't list a separate reset and powerdown > pin, but rather a single pin that performs both functions. All of them are GPIOs, the question here is how they are actually connected on PCB level and I have no answer to that. You have to find schematics somewhere. > Am I wrong to treat that as something that ought to be mapped as a > powerdown GPIO to the sensors? Or do you know of any other way to > reconcile that discrepancy? The GPIOs can go directly to the sensors or be a control pin for separate discrete power gates. So, we can do one of the following: a) present PD GPIO as fixed regulator; b) present PD & Reset GPIOs as regulator; c) provide them as is to the sensor and sensor driver must do what it considers right. Since we don't have schematics (yet?) and we have plenty of variations of sensors, I would go to c) and update the driver of the affected sensor as needed. Because even if you have separate discrete PD for one sensor on one platform there is no guarantee that it will be the same on another. Providing a "virtual" PD in a sensor that doesn't support it is the best choice I think. Let's hear what Sakari and other experienced camera sensor developers say. My vision is purely based on electrical engineering background, experience with existing (not exactly camera) sensor drivers and generic cases. > Failing that; the only way I can think to handle this is to register > proxy GPIO pins assigned to the sensors as you suggested previously, and > have them toggle the GPIO's assigned to the INT3472 based on platform > specific mapping data (I.E. we register a pin called "reset", which on > most platforms just toggles the 0x00 pin, but on this specific platform > would drive both 0x00 and 0x01 together. We're already heading that way > for the regulator consumer supplies so it's sort of nothing new, but I'd > still rather not if it can be avoided.
Hi Andy On 08/01/2021 12:17, Andy Shevchenko wrote: > On Fri, Jan 8, 2021 at 1:56 AM Daniel Scally <djrscally@gmail.com> wrote: >> On 30/11/2020 20:07, Andy Shevchenko wrote: >>> On Mon, Nov 30, 2020 at 01:31:29PM +0000, Daniel Scally wrote: > ... > >>> It's solely Windows driver design... >>> Luckily I found some information and can clarify above table: >>> >>> 0x00 Reset >>> 0x01 Power down >>> 0x0b Power enable >>> 0x0c Clock enable >>> 0x0d LED (active high) >>> >>> The above text perhaps should go somewhere under Documentation. >> Coming back to this; there's a bit of an anomaly with the 0x01 Power >> Down pin for at least one platform. As listed above, the OV2680 on one >> of my platforms has 3 GPIOs defined, and the table above gives them as >> type Reset, Power down and Clock enable. I'd assumed from this table >> that "power down" meant a powerdown GPIO (I.E. the one usually called >> PWDNB in Omnivision datasheets and "powerdown" in drivers), but the >> datasheet for the OV2680 doesn't list a separate reset and powerdown >> pin, but rather a single pin that performs both functions. > All of them are GPIOs, the question here is how they are actually > connected on PCB level and I have no answer to that. You have to find > schematics somewhere. Yeah; I've been trying to get those but so far, no dice. > >> Am I wrong to treat that as something that ought to be mapped as a >> powerdown GPIO to the sensors? Or do you know of any other way to >> reconcile that discrepancy? > The GPIOs can go directly to the sensors or be a control pin for > separate discrete power gates. > So, we can do one of the following: > a) present PD GPIO as fixed regulator; > b) present PD & Reset GPIOs as regulator; > c) provide them as is to the sensor and sensor driver must do what it > considers right. > > Since we don't have schematics (yet?) and we have plenty of variations > of sensors, I would go to c) and update the driver of the affected > sensor as needed. Because even if you have separate discrete PD for > one sensor on one platform there is no guarantee that it will be the > same on another. Providing a "virtual" PD in a sensor that doesn't > support it is the best choice I think. Let's hear what Sakari and > other experienced camera sensor developers say. > > My vision is purely based on electrical engineering background, > experience with existing (not exactly camera) sensor drivers and > generic cases. Alright; thanks. I'm happy with C being the answer, so unless someone thinks differently I'll work on that basis. >> Failing that; the only way I can think to handle this is to register >> proxy GPIO pins assigned to the sensors as you suggested previously, and >> have them toggle the GPIO's assigned to the INT3472 based on platform >> specific mapping data (I.E. we register a pin called "reset", which on >> most platforms just toggles the 0x00 pin, but on this specific platform >> would drive both 0x00 and 0x01 together. We're already heading that way >> for the regulator consumer supplies so it's sort of nothing new, but I'd >> still rather not if it can be avoided. >
H Andy and Daniel, On Fri, Jan 08, 2021 at 02:17:49PM +0200, Andy Shevchenko wrote: > On Fri, Jan 8, 2021 at 1:56 AM Daniel Scally wrote: > > On 30/11/2020 20:07, Andy Shevchenko wrote: > > > On Mon, Nov 30, 2020 at 01:31:29PM +0000, Daniel Scally wrote: > > ... > > > > It's solely Windows driver design... > > > Luckily I found some information and can clarify above table: > > > > > > 0x00 Reset > > > 0x01 Power down > > > 0x0b Power enable > > > 0x0c Clock enable > > > 0x0d LED (active high) > > > > > > The above text perhaps should go somewhere under Documentation. > > > > Coming back to this; there's a bit of an anomaly with the 0x01 Power > > Down pin for at least one platform. As listed above, the OV2680 on one > > of my platforms has 3 GPIOs defined, and the table above gives them as > > type Reset, Power down and Clock enable. I'd assumed from this table > > that "power down" meant a powerdown GPIO (I.E. the one usually called > > PWDNB in Omnivision datasheets and "powerdown" in drivers), but the > > datasheet for the OV2680 doesn't list a separate reset and powerdown > > pin, but rather a single pin that performs both functions. First question, do we have a confirmation that the OV2680 sensor on that platform requires GPIO 0x01 to be toggled to work properly ? I'd like to rule out the option of the GPIO being simply not connected (that would be best for us, although my experience so far with this terrible ACPI design doesn't of course give me much hope). Where did you find the OV2680 datasheet by the way, can you share a link to a leaked version ? > All of them are GPIOs, the question here is how they are actually > connected on PCB level and I have no answer to that. You have to find > schematics somewhere. > > > Am I wrong to treat that as something that ought to be mapped as a > > powerdown GPIO to the sensors? Or do you know of any other way to > > reconcile that discrepancy? > > The GPIOs can go directly to the sensors or be a control pin for > separate discrete power gates. GPIO functions 0x00 and 0x01 are meant to control sensor signals, while GPIO function 0x0b is meant to control a power gate. Of course board designers may have thought clever to use function 0x01 to control a second power gate, this can't be ruled out without the schematics (or reverse engineering of the hardware). > So, we can do one of the following: > a) present PD GPIO as fixed regulator; > b) present PD & Reset GPIOs as regulator; > c) provide them as is to the sensor and sensor driver must do what it > considers right. > > Since we don't have schematics (yet?) and we have plenty of variations > of sensors, I would go to c) and update the driver of the affected > sensor as needed. Because even if you have separate discrete PD for > one sensor on one platform there is no guarantee that it will be the > same on another. Providing a "virtual" PD in a sensor that doesn't > support it is the best choice I think. Let's hear what Sakari and > other experienced camera sensor developers say. > > My vision is purely based on electrical engineering background, > experience with existing (not exactly camera) sensor drivers and > generic cases. If the OV2680 has indeed no power down pin, that won't work. Adding support for a non-existent powerdown pin to the corresponding driver won't be accepted. Workarounds and hacks to support IPU3-based devices need to be kept out of camera sensor drivers. If we need to map GPIO function 0x01 to a sensor GPIO on some platform, and to a regulator on other platforms, then we will need per-platform data in the INT3472 driver. For this particular platform, the reset (0x00) GPIO should be passed to the sensor, and the powerdown (0x01) GPIO should control a regulator (again assuming that our assumption that the GPIO is wired to a power gate is correct). > > Failing that; the only way I can think to handle this is to register > > proxy GPIO pins assigned to the sensors as you suggested previously, and > > have them toggle the GPIO's assigned to the INT3472 based on platform > > specific mapping data (I.E. we register a pin called "reset", which on > > most platforms just toggles the 0x00 pin, but on this specific platform > > would drive both 0x00 and 0x01 together. We're already heading that way > > for the regulator consumer supplies so it's sort of nothing new, but I'd > > still rather not if it can be avoided. -- Regards, Laurent Pinchart
Hi Laurent On 09/01/2021 00:18, Laurent Pinchart wrote: > H Andy and Daniel, > > On Fri, Jan 08, 2021 at 02:17:49PM +0200, Andy Shevchenko wrote: >> On Fri, Jan 8, 2021 at 1:56 AM Daniel Scally wrote: >>> On 30/11/2020 20:07, Andy Shevchenko wrote: >>>> On Mon, Nov 30, 2020 at 01:31:29PM +0000, Daniel Scally wrote: >> ... >> >>>> It's solely Windows driver design... >>>> Luckily I found some information and can clarify above table: >>>> >>>> 0x00 Reset >>>> 0x01 Power down >>>> 0x0b Power enable >>>> 0x0c Clock enable >>>> 0x0d LED (active high) >>>> >>>> The above text perhaps should go somewhere under Documentation. >>> Coming back to this; there's a bit of an anomaly with the 0x01 Power >>> Down pin for at least one platform. As listed above, the OV2680 on one >>> of my platforms has 3 GPIOs defined, and the table above gives them as >>> type Reset, Power down and Clock enable. I'd assumed from this table >>> that "power down" meant a powerdown GPIO (I.E. the one usually called >>> PWDNB in Omnivision datasheets and "powerdown" in drivers), but the >>> datasheet for the OV2680 doesn't list a separate reset and powerdown >>> pin, but rather a single pin that performs both functions. > First question, do we have a confirmation that the OV2680 sensor on that > platform requires GPIO 0x01 to be toggled to work properly ? Yes; without that toggled not even the i2c interface is available. > I'd like to > rule out the option of the GPIO being simply not connected (that would > be best for us, although my experience so far with this terrible ACPI > design doesn't of course give me much hope). Sorry to dash what little hope was left. > Where did you find the OV2680 datasheet by the way, can you share a link > to a leaked version ? Sure. I left the PC already, but I'll do that tomorrow. >> All of them are GPIOs, the question here is how they are actually >> connected on PCB level and I have no answer to that. You have to find >> schematics somewhere. >> >>> Am I wrong to treat that as something that ought to be mapped as a >>> powerdown GPIO to the sensors? Or do you know of any other way to >>> reconcile that discrepancy? >> The GPIOs can go directly to the sensors or be a control pin for >> separate discrete power gates. > GPIO functions 0x00 and 0x01 are meant to control sensor signals, while > GPIO function 0x0b is meant to control a power gate. Of course board > designers may have thought clever to use function 0x01 to control a > second power gate, this can't be ruled out without the schematics (or > reverse engineering of the hardware). > >> So, we can do one of the following: >> a) present PD GPIO as fixed regulator; >> b) present PD & Reset GPIOs as regulator; >> c) provide them as is to the sensor and sensor driver must do what it >> considers right. >> >> Since we don't have schematics (yet?) and we have plenty of variations >> of sensors, I would go to c) and update the driver of the affected >> sensor as needed. Because even if you have separate discrete PD for >> one sensor on one platform there is no guarantee that it will be the >> same on another. Providing a "virtual" PD in a sensor that doesn't >> support it is the best choice I think. Let's hear what Sakari and >> other experienced camera sensor developers say. >> >> My vision is purely based on electrical engineering background, >> experience with existing (not exactly camera) sensor drivers and >> generic cases. > If the OV2680 has indeed no power down pin, that won't work. Adding > support for a non-existent powerdown pin to the corresponding driver > won't be accepted. Workarounds and hacks to support IPU3-based devices > need to be kept out of camera sensor drivers. > > If we need to map GPIO function 0x01 to a sensor GPIO on some platform, > and to a regulator on other platforms, then we will need per-platform > data in the INT3472 driver. For this particular platform, the reset > (0x00) GPIO should be passed to the sensor, and the powerdown (0x01) > GPIO should control a regulator (again assuming that our assumption that > the GPIO is wired to a power gate is correct). Let me think of a neat way to do this then. > >>> Failing that; the only way I can think to handle this is to register >>> proxy GPIO pins assigned to the sensors as you suggested previously, and >>> have them toggle the GPIO's assigned to the INT3472 based on platform >>> specific mapping data (I.E. we register a pin called "reset", which on >>> most platforms just toggles the 0x00 pin, but on this specific platform >>> would drive both 0x00 and 0x01 together. We're already heading that way >>> for the regulator consumer supplies so it's sort of nothing new, but I'd >>> still rather not if it can be avoided.
On 09/01/2021 09:17, Andy Shevchenko wrote: > On Saturday, January 9, 2021, Daniel Scally <djrscally@gmail.com> wrote: > >> Hi Andy >> >> On 08/01/2021 12:17, Andy Shevchenko wrote: >>> On Fri, Jan 8, 2021 at 1:56 AM Daniel Scally <djrscally@gmail.com> >> wrote: >>>> On 30/11/2020 20:07, Andy Shevchenko wrote: >>>>> On Mon, Nov 30, 2020 at 01:31:29PM +0000, Daniel Scally wrote: >>> ... >>> >>>>> It's solely Windows driver design... >>>>> Luckily I found some information and can clarify above table: >>>>> >>>>> 0x00 Reset >>>>> 0x01 Power down >>>>> 0x0b Power enable >>>>> 0x0c Clock enable >>>>> 0x0d LED (active high) >>>>> >>>>> The above text perhaps should go somewhere under Documentation. >>>> Coming back to this; there's a bit of an anomaly with the 0x01 Power >>>> Down pin for at least one platform. As listed above, the OV2680 on one >>>> of my platforms has 3 GPIOs defined, and the table above gives them as >>>> type Reset, Power down and Clock enable. I'd assumed from this table >>>> that "power down" meant a powerdown GPIO (I.E. the one usually called >>>> PWDNB in Omnivision datasheets and "powerdown" in drivers), but the >>>> datasheet for the OV2680 doesn't list a separate reset and powerdown >>>> pin, but rather a single pin that performs both functions. >>> All of them are GPIOs, the question here is how they are actually >>> connected on PCB level and I have no answer to that. You have to find >>> schematics somewhere. >> Yeah; I've been trying to get those but so far, no dice. >> >> > Can you share the exact name / model of the hardware in question here? I > would try to search for the schematics. Lenovo Miix 510-12ISK 80U1 - I also tried asking Lenovo for them but that didn't really go anywhere; but of course I'm just contacting their usual support line and explaining what I'm after, so I didn't really expect it to. > > >>>> Am I wrong to treat that as something that ought to be mapped as a >>>> powerdown GPIO to the sensors? Or do you know of any other way to >>>> reconcile that discrepancy? >>> The GPIOs can go directly to the sensors or be a control pin for >>> separate discrete power gates. >>> So, we can do one of the following: >>> a) present PD GPIO as fixed regulator; >>> b) present PD & Reset GPIOs as regulator; >>> c) provide them as is to the sensor and sensor driver must do what it >>> considers right. >>> >>> Since we don't have schematics (yet?) and we have plenty of variations >>> of sensors, I would go to c) and update the driver of the affected >>> sensor as needed. Because even if you have separate discrete PD for >>> one sensor on one platform there is no guarantee that it will be the >>> same on another. Providing a "virtual" PD in a sensor that doesn't >>> support it is the best choice I think. Let's hear what Sakari and >>> other experienced camera sensor developers say. >>> >>> My vision is purely based on electrical engineering background, >>> experience with existing (not exactly camera) sensor drivers and >>> generic cases. >> Alright; thanks. I'm happy with C being the answer, so unless someone >> thinks differently I'll work on that basis. >> >> > Laurent answered that it is not the best choice from camera sensor driver > perspective. Yep, seen - no problem. I will look at doing it via the method he suggests.
Hello all Previous version: https://lore.kernel.org/linux-media/20201019225903.14276-1-djrscally@gmail.com/ This series aims to add support for webcams on laptops with ACPI tables designed for use with CIO2 on Windows. There are two main parts; the first is extending the ipu3-cio2 driver to allow for patching the firmware via software_nodes if endpoints aren't defined by ACPI. Patch #13 deals directly with that, all preceding patches are either supplemental changes or incidental fixes along the way. The second main part is a way to handle the unusual definition of resource destined for the sensors in these ACPI tables; regulators and GPIO lines that are supposed to be consumed by the sensor are lumped in the _CRS of a dummy ACPI device upon which the sensor is dependent. Patch 18 defines a new driver to handle those dummy devices and map the resources to the sensor instead. 14-17 are supporting changes for that driver. Changelogs mostly in the individual patches, but a broad summary: - Altered fwnode_device_is_available() to return true if the fwnode_handle doesn't implement that operation. - Altered fwnode_graph_get_endpoint_by_id() to parse secondary if no endpoint found on primary. - Enforce parent->child ordering of software_nodes on registration - Added a function to get the next ACPI device with matching _HID, plus an iterator macro - Altered cio2-bridge.c to store the bridge struct (and basically everything else) in heap, plus removed the requirement to delay ipu3-cio2 probe until after the i2c devices were instantiated. Also now ensured we handle multiple sensors with the same _HID. - Added a function to get devices _dependent_ on a given ACPI dev, according to their _DEP entries. - Added a function to explicitly construct the name of an I2C dev created from an ACPI dev. - Added a driver to handle the dummy ACPI devices discussed above. Comments very welcome! Dan Scally (1): i2c: i2c-core-base: Use the new i2c_acpi_dev_name() in i2c_set_dev_name() Daniel Scally (16): property: Return true in fwnode_device_is_available for node types that do not implement this operation property: Add support for calling fwnode_graph_get_endpoint_by_id() for fwnode->secondary software_node: Fix failure to put() and get() references to children in software_node_get_next_child() software_node: Enforce parent before child ordering of nodes array for software_node_register_nodes() software_node: Alter software_node_unregister_nodes() to unregister the array in reverse order software_node: amend software_node_unregister_node_group() to perform unregistration of array in reverse order to be consistent with software_node_unregister_nodes() lib/test_printf.c: Use helper function to unwind array of software_nodes ipu3-cio2: Add T: entry to MAINTAINERS ipu3-cio2: Rename ipu3-cio2.c to allow module to be built from multiple source files retaining ipu3-cio2 name media: v4l2-core: v4l2-async: Check possible match in match_fwnode based on sd->fwnode->secondary acpi: Add acpi_dev_get_next_match_dev() and macro to iterate through acpi_devices matching a given _HID ipu3-cio2: Add functionality allowing software_node connections to sensors on platforms designed for Windows acpi: utils: Add function to fetch dependent acpi_devices i2c: i2c-core-acpi: Add i2c_acpi_dev_name() gpio: gpiolib-acpi: Export acpi_get_gpiod() ipu3: Add driver for dummy INT3472 ACPI device Heikki Krogerus (1): software_node: Add support for fwnode_graph*() family of functions MAINTAINERS | 9 + drivers/acpi/utils.c | 98 ++++- drivers/base/property.c | 9 + drivers/base/swnode.c | 157 +++++++- drivers/gpio/gpiolib-acpi.c | 3 +- drivers/i2c/i2c-core-acpi.c | 14 + drivers/i2c/i2c-core-base.c | 2 +- drivers/media/pci/intel/ipu3/Kconfig | 32 ++ drivers/media/pci/intel/ipu3/Makefile | 4 + drivers/media/pci/intel/ipu3/cio2-bridge.c | 260 ++++++++++++ drivers/media/pci/intel/ipu3/cio2-bridge.h | 108 +++++ drivers/media/pci/intel/ipu3/int3472.c | 381 ++++++++++++++++++ drivers/media/pci/intel/ipu3/int3472.h | 96 +++++ .../ipu3/{ipu3-cio2.c => ipu3-cio2-main.c} | 27 ++ drivers/media/pci/intel/ipu3/ipu3-cio2.h | 6 + drivers/media/v4l2-core/v4l2-async.c | 8 + include/acpi/acpi_bus.h | 9 + include/linux/acpi.h | 5 + include/linux/i2c.h | 5 + lib/test_printf.c | 4 +- 20 files changed, 1213 insertions(+), 24 deletions(-) create mode 100644 drivers/media/pci/intel/ipu3/cio2-bridge.c create mode 100644 drivers/media/pci/intel/ipu3/cio2-bridge.h create mode 100644 drivers/media/pci/intel/ipu3/int3472.c create mode 100644 drivers/media/pci/intel/ipu3/int3472.h rename drivers/media/pci/intel/ipu3/{ipu3-cio2.c => ipu3-cio2-main.c} (98%)