Re: [PATCH v3 3/5] i2c: piix4: Add ACPI support for ASF SMBus device

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

On Fri, Sep 06, 2024 at 12:41:59PM +0530, Shyam Sundar S K wrote:
> The AMD ASF controller is presented to the operating system as an ACPI
> device. The piix4 driver can obtain the ASF handle through ACPI to
> retrieve information about the ASF controller's attributes, such as the
> ASF address space and interrupt number, and to handle ASF interrupts.

Can you share an excerpt of DSDT to see how it looks like?

> Currently, the piix4 driver assumes that a specific port address is
> designated for AUX operations. However, with the introduction of ASF, the
> same port address may also be used by the ASF controller. Therefore, a
> check needs to be added to ensure that if ASF is advertised and enabled in
> ACPI, the AUX port is not set up.

> Additionally, include a 'depends on X86' Kconfig entry for
> CONFIG_I2C_PIIX4, as the current patch utilizes acpi_dev_get_resources(),
> which is compiled only when CONFIG_ACPI is enabled, and CONFIG_ACPI
> depends on CONFIG_X86.

Yeah, please don't do that. If it requires ACPI, make it clear, there is
no x86 compile-time dependency.

Second issue with this is that now you require entire ACPI machinery for
the previous cases where it wasn't needed. Imagine an embedded system with
limited amount of memory for which you require +1Mbyte just for nothing.

Look how the other do (hint: ifdeffery in the code with stubs).

> +#define SB800_ASF_ACPI_PATH			"\\_SB.ASFC"

...

> +static void sb800_asf_process_slave(struct work_struct *work)
> +{
> +	struct i2c_piix4_adapdata *adapdata =
> +		container_of(work, struct i2c_piix4_adapdata, work_buf.work);
> +	unsigned short piix4_smba = adapdata->smba;
> +	u8 data[SB800_ASF_BLOCK_MAX_BYTES];

> +	u8 bank, reg, cmd = 0;

Move cmd assignment into the respective branch of the conditional below, in
that case it will be closer and more symmetrical.

> +	u8 len, val = 0;

> +	int i;

Why signed?

> +	/* Read slave status register */
> +	reg = inb_p(ASFSLVSTA);
> +
> +	/* Check if no error bits are set in slave status register */
> +	if (reg & SB800_ASF_ERROR_STATUS) {
> +		/* Set bank as full */
> +		reg = reg | GENMASK(3, 2);
> +		outb_p(reg, ASFDATABNKSEL);
> +	} else {
> +		/* Read data bank */
> +		reg = inb_p(ASFDATABNKSEL);

> +		bank = (reg & BIT(3)) >> 3;

Try
		bank = (reg & BIT(3)) ? 1 : 0;

Probably it doesn't affect the code generation, but at least seems cleaner
to read.

> +		/* Set read data bank */
> +		if (bank) {
> +			reg = reg | BIT(4);
> +			reg = reg & ~BIT(3);
> +		} else {
> +			reg = reg & ~BIT(4);
> +			reg = reg & ~BIT(2);
> +		}
> +
> +		/* Read command register */
> +		outb_p(reg, ASFDATABNKSEL);
> +		cmd = inb_p(ASFINDEX);
> +		len = inb_p(ASFDATARWPTR);
> +		for (i = 0; i < len; i++)
> +			data[i] = inb_p(ASFINDEX);
> +
> +		/* Clear data bank status */
> +		if (bank) {
> +			reg = reg | BIT(3);
> +			outb_p(reg, ASFDATABNKSEL);
> +		} else {
> +			reg = reg | BIT(2);
> +			outb_p(reg, ASFDATABNKSEL);
> +		}
> +	}
> +
> +	outb_p(0, ASFSETDATARDPTR);
> +	if (cmd & BIT(0))
> +		return;
> +
> +	i2c_slave_event(adapdata->slave, I2C_SLAVE_WRITE_REQUESTED, &val);
> +	for (i = 0; i < len; i++) {
> +		val = data[i];
> +		i2c_slave_event(adapdata->slave, I2C_SLAVE_WRITE_RECEIVED, &val);
> +	}
> +	i2c_slave_event(adapdata->slave, I2C_SLAVE_STOP, &val);
> +}

...

> +static irqreturn_t sb800_asf_irq_handler(int irq, void *ptr)
> +{
> +	struct i2c_piix4_adapdata *adapdata = ptr;
> +	unsigned short piix4_smba = adapdata->smba;
> +	u8 slave_int = inb_p(ASFSTA);
> +
> +	if (slave_int & BIT(6)) {
> +		/* Slave Interrupt */
> +		outb_p(slave_int | BIT(6), ASFSTA);
> +		schedule_delayed_work(&adapdata->work_buf, HZ);
> +	} else {
> +		/* Master Interrupt */

Please, start using inclusive non-offensive terms instead of old 'master/slave'
terminology. Nowadays it's a part of the standard AFAIU.

Note, I'm talking only about comments and messages, the APIs is another story
that should be addressed separately.

> +		sb800_asf_update_bits(piix4_smba, SB800_ASF_SLV_INTR, SMBHSTSTS, true);
> +	}
> +
> +	return IRQ_HANDLED;
> +}

...

> +static int sb800_asf_add_adap(struct pci_dev *dev)
> +{
> +	struct i2c_piix4_adapdata *adapdata;
> +	struct resource_entry *rentry;
> +	struct sb800_asf_data data;

> +	struct list_head res_list;

Why not LIST_HEAD(); as it was in the previous version?

> +	struct acpi_device *adev;
> +	acpi_status status;
> +	acpi_handle handle;
> +	int ret;

> +	status = acpi_get_handle(NULL, SB800_ASF_ACPI_PATH, &handle);
> +	if (ACPI_FAILURE(status))
> +		return -ENODEV;
> +
> +	adev = acpi_fetch_acpi_dev(handle);
> +	if (!adev)
> +		return -ENODEV;

This approach I don't like. I would like to see DSDT for that
as I mentioned above.

> +	INIT_LIST_HEAD(&res_list);

See above.

> +	ret = acpi_dev_get_resources(adev, &res_list, NULL, NULL);
> +	if (ret < 0) {

> +		dev_err(&dev->dev, "Error getting ASF ACPI resource: %d\n", ret);
> +		return ret;

		return dev_err_probe(...);

> +	}
> +
> +	list_for_each_entry(rentry, &res_list, node) {
> +		switch (resource_type(rentry->res)) {
> +		case IORESOURCE_IO:
> +			data.addr = rentry->res->start;
> +			break;
> +		case IORESOURCE_IRQ:
> +			data.irq = rentry->res->start;
> +			break;
> +		default:
> +			dev_warn(&adev->dev, "Invalid ASF resource\n");
> +			break;
> +		}
> +	}
> +
> +	acpi_dev_free_resource_list(&res_list);
> +	ret = piix4_add_adapter(dev, data.addr, SMBUS_ASF, piix4_adapter_count, false, 0,
> +				piix4_main_port_names_sb800[piix4_adapter_count],
> +				&piix4_main_adapters[piix4_adapter_count]);
> +	if (ret) {
> +		dev_err(&dev->dev, "Failed to add ASF adapter: %d\n", ret);
> +		return -ENODEV;

		return dev_err_probe(...);

> +	}
> +
> +	adapdata = i2c_get_adapdata(piix4_main_adapters[piix4_adapter_count]);
> +	ret = devm_request_irq(&dev->dev, data.irq, sb800_asf_irq_handler, IRQF_SHARED,
> +			       "sb800_smbus_asf", adapdata);
> +	if (ret) {
> +		dev_err(&dev->dev, "Unable to request irq: %d for use\n", data.irq);
> +		return ret;

		return dev_err_probe(...);

> +	}
> +
> +	INIT_DELAYED_WORK(&adapdata->work_buf, sb800_asf_process_slave);
> +	adapdata->is_asf = true;
> +	/* Increment the adapter count by 1 as ASF is added to the list */
> +	piix4_adapter_count++;
> +	return 1;
> +}

-- 
With Best Regards,
Andy Shevchenko