Re: [PATCH 18/18] ipu3: Add driver for dummy INT3472 ACPI device

[Dan Scally <djrscally@gmail.com>]

Hi Sakari

On 30/11/2020 20:52, Sakari Ailus wrote:
>> +static const struct acpi_device_id int3472_device_id[] = {
>> +	{ "INT3472", 0 },
> The INT3472 _HID is really allocated for the tps68470 PMIC chip. It may not
> be used by other drivers; people will want to build kernels where both of
> these ACPI table layouts are functional.
>
> Instead, I propose, that you add this as an option to the tps68470 driver
> that figures out whether the ACPI device for the tps68470 device actually
> describes something else, in a similar fashion you do with the cio2-bridge
> driver. I think it may need a separate Kconfig option albeit this and
> cio2-bridge cannot be used separately.

It actually occurs to me that that may not work (I know I called that
out as an option we considered, but that was a while ago actually). The
reason I wasn't worried about the existing tps68470 driver binding to
these devices is that it's an i2c driver, and these dummy devices don't
have an I2cSerialBusV2, so no I2C device is created by them the kernel.


Won't that mean the tps68470 driver won't ever be probed for these devices?

>
>> +	{ },
>> +};
>> +MODULE_DEVICE_TABLE(acpi, int3472_device_id);
>> +
>> +static struct acpi_driver int3472_driver = {
>> +	.name = "int3472",
>> +	.ids = int3472_device_id,
>> +	.ops = {
>> +		.add = int3472_add,
>> +		.remove = int3472_remove,
>> +	},
>> +	.owner = THIS_MODULE,
>> +};
>> +
>> +module_acpi_driver(int3472_driver);
>> +
>> +MODULE_LICENSE("GPL v2");
>> +MODULE_AUTHOR("Dan Scally <djrscally@gmail.com>");
>> +MODULE_DESCRIPTION("ACPI Driver for Discrete type INT3472 ACPI Devices");
>> diff --git a/drivers/media/pci/intel/ipu3/int3472.h b/drivers/media/pci/intel/ipu3/int3472.h
>> new file mode 100644
>> index 000000000000..6964726e8e1f
>> --- /dev/null
>> +++ b/drivers/media/pci/intel/ipu3/int3472.h
>> @@ -0,0 +1,96 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +/* Author: Dan Scally <djrscally@gmail.com> */
>> +#include <linux/regulator/machine.h>
>> +
>> +#define INT3472_MAX_SENSOR_GPIOS			3
>> +#define GPIO_REGULATOR_NAME_LENGTH			17
>> +#define GPIO_REGULATOR_SUPPLY_NAME_LENGTH		9
>> +
>> +#define INT3472_REGULATOR(_NAME, _SUPPLY, _ID, _OPS)	\
>> +	((const struct regulator_desc) {		\
>> +		.name = _NAME,				\
>> +		.supply_name = _SUPPLY,			\
>> +		.id = _ID,				\
>> +		.type = REGULATOR_VOLTAGE,		\
>> +		.ops = _OPS,				\
>> +		.owner = THIS_MODULE,			\
>> +	})
>> +
>> +const guid_t int3472_gpio_guid = GUID_INIT(0x79234640, 0x9e10, 0x4fea,
>> +					     0xa5, 0xc1, 0xb5, 0xaa, 0x8b,
>> +					     0x19, 0x75, 0x6f);
>> +
>> +const guid_t cio2_sensor_module_guid = GUID_INIT(0x822ace8f, 0x2814, 0x4174,
>> +						 0xa5, 0x6b, 0x5f, 0x02, 0x9f,
>> +						 0xe0, 0x79, 0xee);
>> +
>> +struct int3472_cldb {
>> +	u8 version;
>> +	/*
>> +	 * control logic type
>> +	 * 0: UNKNOWN
>> +	 * 1: DISCRETE(CRD-D)
>> +	 * 2: PMIC TPS68470
>> +	 * 3: PMIC uP6641
>> +	 */
>> +	u8 control_logic_type;
>> +	u8 control_logic_id;
>> +	u8 sensor_card_sku;
>> +	u8 reserved[28];
>> +};
>> +
>> +struct int3472_device {
>> +	struct acpi_device *adev;
>> +	struct acpi_device *sensor;
>> +
>> +	unsigned int n_gpios; /* how many GPIOs have we seen */
>> +
>> +	unsigned int n_regulators;
>> +	struct list_head regulators;
>> +
>> +	unsigned int n_sensor_gpios; /* how many have we mapped to sensor */
>> +	struct gpiod_lookup_table gpios;
>> +};
>> +
>> +struct int3472_gpio_regulator {
>> +	char regulator_name[GPIO_REGULATOR_NAME_LENGTH];
>> +	char supply_name[GPIO_REGULATOR_SUPPLY_NAME_LENGTH];
>> +	struct gpio_desc *gpio;
>> +	struct regulator_dev *rdev;
>> +	struct regulator_desc rdesc;
>> +	struct list_head list;
>> +};
>> +
>> +struct int3472_sensor_regulator_map {
>> +	char *sensor_module_name;
>> +	unsigned int n_supplies;
>> +	struct regulator_consumer_supply *supplies;
>> +};
>> +
>> +/*
>> + * Here follows platform specific mapping information that we can pass to
>> + * regulator_init_data when we register our regulators. They're just mapped
>> + * via index, I.E. the first regulator pin that the code finds for the
>> + * i2c-OVTI2680:00 device is avdd, the second is dovdd and so on.
>> + */
>> +
>> +static struct regulator_consumer_supply miix_510_ov2680[] = {
>> +	{ "i2c-OVTI2680:00", "avdd" },
>> +	{ "i2c-OVTI2680:00", "dovdd" },
>> +};
>> +
>> +static struct regulator_consumer_supply surface_go2_ov5693[] = {
>> +	{ "i2c-INT33BE:00", "avdd" },
>> +	{ "i2c-INT33BE:00", "dovdd" },
>> +};
>> +
>> +static struct regulator_consumer_supply surface_book_ov5693[] = {
>> +	{ "i2c-INT33BE:00", "avdd" },
>> +	{ "i2c-INT33BE:00", "dovdd" },
>> +};
>> +
>> +static struct int3472_sensor_regulator_map int3472_sensor_regulator_maps[] = {
>> +	{ "GNDF140809R", 2, miix_510_ov2680 },
>> +	{ "YHCU", 2, surface_go2_ov5693 },
>> +	{ "MSHW0070", 2, surface_book_ov5693 },
>> +};