| Message ID | 20201130133129.1024662-19-djrscally@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | Add functionality to ipu3-cio2 driver allowing software_node connections to sensors on platforms designed for Windows | expand |
Hi Daniel, Thanks for the patch ! On 30/11/2020 14: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 Is it possible that the ad5823 would be here, and controled by this bit ? > 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 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 }, > +}; >
Hello, On Mon, Nov 30, 2020 at 04:29:04PM +0000, Kieran Bingham wrote: > 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. Those are called "discrete regulators", and can also be identified by the type reported in the CLDB. They're not regulators, just ACPI device objects that group a set of GPIOs and a referenced from the consumers of those GPIOs. I'll refrain here from sharing my opinion on the ACPI design... > > 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 I think 0x01 is probably a power down GPIO. > > 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? That's my main concern here, two drivers for the same device won't play well together. We'll need to at least teach the tps68470 driver to ignore the "discrete regulators". I however believe this would be better handled in the cio2-bridge driver, given that the ACPI device object doesn't represent an actual device. > > 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? > > > + > > +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: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.
Hello, 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. I actually don't think it is, at least not if you consider how Intel uses it. It can mean TI TPS64870, uPI Semi uP6641Q, or "discrete regulator". It's called an "Intel camera power management device" in Windows if I remember correctly. If we go in the direction of creating a platform driver for this ACPI HID, it should be called accordingly (int3472, intel-camera-pmic, ...), check the device type from the CLDB, and register the right device. One Chrome OS platforms, INT3472 refers to the TPS64870 only, and the ACPI device object doesn't have a CLDB. That's easy to detect, and we can enable tps64870 support when there's no CLDB. Note that for the TPS64870 case, when a CLDB is present, the kernel driver will need to register regulators and clocks, while when no CLDB is present, it will need to register an opregion as done today. That's yet another mode of operation of this driver. > > 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? I think we can create a platform driver in that case. The same module can register multiple drivers (platform and I2C). > >> + { }, > >> +}; > >> +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 }, > >> +};
On 30/11/2020 23:21, Laurent Pinchart wrote: >>> 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? > I think we can create a platform driver in that case. The same module > can register multiple drivers (platform and I2C). Ah, I follow. OK, that's an option then.
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 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?
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 Tue, Dec 01, 2020 at 02:40:32PM +0200, Laurent Pinchart wrote: > 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 ? Yeah... I hadn't gotten that far yet. :-)
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.
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). P.S. Dan, can you drop unrelated text when replying?
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.
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.
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:36:16PM +0200, Laurent Pinchart wrote: > 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* :-( But it's not a problem for i2c-multi-instantiate. It's kinda MFD approach for I²C peripheral devices. -- With Best Regards, Andy Shevchenko
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.
Hi Andy, 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 ? 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.
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 12:48:28PM +0000, Dan Scally wrote: > On 01/12/2020 12:32, Sakari Ailus wrote: ... > 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. As I said in the other subthread, we need to take ACPI ID from MFD and move it to platform driver. I like the idea what Laurent proposed there.
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.
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.
Hi, 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. Regards, Hans
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!
Hi Andy On 01/12/2020 18:49, Andy Shevchenko wrote: > P.S. Dan, can you drop unrelated text when replying? Ah - sure, sorry. > 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. 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.
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 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. :-) -- With Best Regards, Andy Shevchenko
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).
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.
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: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.
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. -- With Best Regards, Andy Shevchenko
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. -- With Best Regards, Andy Shevchenko
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.
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.
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: + 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 }, +};
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. 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