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

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

On Wed, Sep 04, 2024 at 04:27:29PM +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.
> 
> 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.

> Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

Use --cc parameter when sending, instead of this, or alternatively move this
line...

> Co-developed-by: Sanket Goswami <Sanket.Goswami@amd.com>
> Signed-off-by: Sanket Goswami <Sanket.Goswami@amd.com>
> Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
> ---

...to be somewhere here. It will the same effect for you and me
(in case you use `git send-email`).

>  drivers/i2c/busses/i2c-piix4.c | 166 ++++++++++++++++++++++++++++++++-

...

> +struct sb800_asf_data {
> +	unsigned short addr;
> +	int irq;
> +};


> struct i2c_piix4_adapdata {

>  	struct sb800_mmio_cfg mmio_cfg;
>  	u8 algo_select;
>  	struct i2c_client *slave;
> +	bool is_asf;
> +	struct delayed_work work_buf;
>  };

Side note, always run `pahole` when adding new data structures or modifying
existing ones. It might save memory on the running systems.

...

> +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);

Better split is

	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;
> +	u8 len, val = 0;
> +	int i;
> +
> +	/* 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)) {

Why not use positive conditional? (This will require to swap if/else branches)

> +		/* Read data bank */
> +		reg = inb_p(ASFDATABNKSEL);
> +		bank = (reg & BIT(3)) >> 3;
> +
> +		/* Set read data bank */
> +		if (bank) {
> +			reg = reg | BIT(4);

> +			reg = reg & (~BIT(3));
> +		} else {
> +			reg = reg & (~BIT(4));
> +			reg = reg & (~BIT(2));

In all these three cases the outer parentheses are redundant.

> +		}
> +
> +		/* 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);
> +		}
> +	} else {
> +		/* Set bank as full */
> +		reg = reg | (BIT(3) | BIT(2));

GENMASK() ?

> +		outb_p(reg, ASFDATABNKSEL);
> +	}
> +
> +	outb_p(0, ASFSETDATARDPTR);
> +	if (!(cmd & BIT(0))) {

	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 = (struct i2c_piix4_adapdata *)ptr;

Unneeded explicit casting.

> +	unsigned short piix4_smba = adapdata->smba;
> +	u8 slave_int = inb_p(ASFSTA);
> +
> +	if ((slave_int & BIT(6))) {

Too many parentheses

> +		/* Slave Interrupt */
> +		outb_p(slave_int | BIT(6), ASFSTA);
> +		schedule_delayed_work(&adapdata->work_buf, HZ);
> +	} else {
> +		/* Master Interrupt */
> +		sb800_asf_update_bits(piix4_smba, SB800_ASF_SLV_INTR, SMBHSTSTS, true);
> +	}
> +
> +	return IRQ_HANDLED;
> +}

...

> +static int sb800_asf_acpi_resource_cb(struct acpi_resource *resource, void *context)
> +{
> +	struct sb800_asf_data *data = context;
> +
> +	switch (resource->type) {
> +	case ACPI_RESOURCE_TYPE_EXTENDED_IRQ:
> +		data->irq = resource->data.extended_irq.interrupts[0];
> +		break;
> +	case ACPI_RESOURCE_TYPE_IO:
> +		data->addr = resource->data.io.minimum;
> +		break;
> +	}
> +
> +	return 0;
> +}

Why do you still need this callback? What for?

...

> +static int sb800_asf_add_adap(struct pci_dev *dev)
> +{
> +	struct i2c_piix4_adapdata *adapdata;
> +	struct sb800_asf_data data;
> +	struct acpi_device *adev;
> +	LIST_HEAD(res_list);
> +	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;
> +
> +	ret = acpi_dev_get_resources(adev, &res_list, sb800_asf_acpi_resource_cb, &data);

	..., NULL, NULL):

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

Here iterate over the resources and take the ones you need by their respective
types.

> +	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;
> +	}
> +
> +	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;
> +	}
> +
> +	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 += 1;

++ also suffice.

> +	return 1;
> +}

-- 
With Best Regards,
Andy Shevchenko