Re: [PATCH v5 4/4] platform/x86: Add ACPI i2c-multi-instantiate pseudo driver

[Hans de Goede <hdegoede@redhat.com>]

Hi,

On 09-08-18 13:09, Heikki Krogerus wrote:
> On Thu, Aug 09, 2018 at 11:15:58AM +0200, Hans de Goede wrote:
>> On systems with ACPI instantiated i2c-clients, normally there is 1 fw_node
>> per i2c-device and that fw-node contains 1 I2cSerialBus resource for that 1
>> i2c-device.
>>
>> But in some rare cases the manufacturer has decided to describe multiple
>> i2c-devices in a single ACPI fwnode with multiple I2cSerialBus resources.
>>
>> An earlier attempt to fix this in the i2c-core resulted in a lot of extra
>> code to support this corner-case.
>>
>> This commit introduces a new i2c-multi-instantiate driver which fixes this
>> in a different way. This new driver can be built as a module which will
>> only loaded on affected systems.
>>
>> This driver will instantiate a new i2c-client per I2cSerialBus resource,
>> using the driver_data from the acpi_device_id it is binding to to tell it
>> which chip-type (and optional irq-resource) to use when instantiating.
>>
>> Note this driver depends on a platform device being instantiated for the
>> ACPI fwnode, see the i2c_multi_instantiate_ids list of ACPI device-ids in
>> drivers/acpi/scan.c: acpi_device_enumeration_by_parent().
>>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
> 
> <snip>
> 
>> +static int i2c_multi_inst_probe(struct platform_device *pdev)
>> +{
>> +	struct i2c_multi_inst_data *multi;
>> +	const struct acpi_device_id *match;
>> +	const struct i2c_inst_data *inst_data;
>> +	struct i2c_board_info board_info = {};
>> +	struct device *dev = &pdev->dev;
>> +	struct acpi_device *adev;
>> +	char name[32];
>> +	int i, ret;
>> +
>> +	match = acpi_match_device(dev->driver->acpi_match_table, dev);
>> +	if (!match) {
>> +		dev_err(dev, "Error ACPI match data is missing\n");
>> +		return -ENODEV;
>> +	}
>> +	inst_data = (const struct i2c_inst_data *)match->driver_data;
>> +
>> +	adev = ACPI_COMPANION(dev);
>> +
>> +	/* Count number of clients to instantiate */
>> +	for (i = 0; inst_data[i].type; i++) {}
>> +
>> +	multi = devm_kmalloc(dev,
>> +			offsetof(struct i2c_multi_inst_data, clients[i]),
>> +			GFP_KERNEL);
>> +	if (!multi)
>> +		return -ENOMEM;
>> +
>> +	multi->num_clients = i;
>> +
>> +	for (i = 0; i < multi->num_clients; i++) {
>> +		memset(&board_info, 0, sizeof(board_info));
>> +		strlcpy(board_info.type, inst_data[i].type, I2C_NAME_SIZE);
>> +		snprintf(name, sizeof(name), "%s-%s", match->id,
>> +			 inst_data[i].type);
>> +		board_info.dev_name = name;
>> +		board_info.irq = 0;
>> +		if (inst_data[i].irq_idx != -1) {
>> +			ret = acpi_dev_gpio_irq_get(adev, inst_data[i].irq_idx);
>> +			if (ret < 0) {
>> +				dev_err(dev, "Error requesting irq at index %d: %d\n",
>> +					inst_data[i].irq_idx, ret);
>> +				goto error;
> 
> This seems to assume that we always have GpioInt with assigned for
> these devices, but that's wrong. This needs to work with normal
> Interrupt type resources as well.
> 
> The TI USB PD controller instances (HID 3515) for exmaple have normal
> Interrupts assigned to them
> 
> Why not use the "irq_idx" with normal interrupts, and add a new member
> for the GpioInts, something like gpio_irq_idx?

I don't have any use-cases to test using normal interrupts, so I
will leave adding support for that to the first patch adding support
for a device which needs that, such as the HID3515 you mention.

I do agree that it is good to prepare for adding this later, so
I'm going to post a v6 doing s/irq_idx/gpio_irq_idx/

Regards,

Hans



> 
>> +			}
>> +			board_info.irq = ret;
>> +		}
>> +		multi->clients[i] = i2c_acpi_new_device(dev, i, &board_info);
>> +		if (!multi->clients[i]) {
>> +			dev_err(dev, "Error creating i2c-client, idx %d\n", i);
>> +			ret = -ENODEV;
>> +			goto error;
>> +		}
>> +	}
>> +
>> +	platform_set_drvdata(pdev, multi);
>> +	return 0;
>> +
>> +error:
>> +	while (--i >= 0)
>> +		i2c_unregister_device(multi->clients[i]);
>> +
>> +	return ret;
>> +}
> 
> Thanks,
>