| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20201130232150.GC25713@pendragon.ideasonboard.com>
Date: Tue, 1 Dec 2020 01:21:50 +0200
From: Laurent Pinchart <laurent.pinchart@...asonboard.com>
To: Dan Scally <djrscally@...il.com>
Cc: Sakari Ailus <sakari.ailus@....fi>, linux-kernel@...r.kernel.org,
linux-acpi@...r.kernel.org, linux-gpio@...r.kernel.org,
linux-i2c@...r.kernel.org, linux-media@...r.kernel.org,
devel@...ica.org, rjw@...ysocki.net, lenb@...nel.org,
gregkh@...uxfoundation.org, mika.westerberg@...ux.intel.com,
andriy.shevchenko@...ux.intel.com, linus.walleij@...aro.org,
bgolaszewski@...libre.com, wsa@...nel.org, yong.zhi@...el.com,
sakari.ailus@...ux.intel.com, bingbu.cao@...el.com,
tian.shu.qiu@...el.com, mchehab@...nel.org, robert.moore@...el.com,
erik.kaneda@...el.com, pmladek@...e.com, rostedt@...dmis.org,
sergey.senozhatsky@...il.com, linux@...musvillemoes.dk,
kieran.bingham+renesas@...asonboard.com, jacopo+renesas@...ndi.org,
laurent.pinchart+renesas@...asonboard.com,
jorhand@...ux.microsoft.com, kitakar@...il.com,
heikki.krogerus@...ux.intel.com
Subject: Re: [PATCH 18/18] ipu3: Add driver for dummy INT3472 ACPI device
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@...il.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@...il.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 },
> >> +};
--
Regards,
Laurent Pinchart
Powered by blists - more mailing lists