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

[Hans de Goede]

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@xxxxxxxxxx>
> > ---
>
> <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,