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

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

On Wed, Sep 04, 2024 at 01:06:16AM +0530, Shyam Sundar S K wrote:
> +Andy (this has some ACPI handling that adds AMD ASF support to the
> existing piix4 driver for SMBus)

Thanks.

> On 8/22/2024 19:51, 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.

...

> > +static acpi_status 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 AE_OK;
> > +}
> > +
> > +static int sb800_asf_add_adap(struct pci_dev *dev)
> > +{
> > +	struct i2c_piix4_adapdata *adapdata;
> > +	struct sb800_asf_data *data;
> > +	acpi_status status;
> > +	acpi_handle handle;
> > +	int ret;

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

> > +	data = devm_kzalloc(&dev->dev, sizeof(struct sb800_asf_data), GFP_KERNEL);
> > +	if (!data)
> > +		return -ENOMEM;

Why can't it be on stack?

> > +	status = acpi_walk_resources(handle, METHOD_NAME__CRS, sb800_asf_acpi_resource_cb, data);
> > +	if (ACPI_FAILURE(status))
> > +		return -EINVAL;
> > +
> > +	if (!data->addr)
> > +		return -EINVAL;

This is reinvention of acpi_dev_get_resources(). Many drivers are using it, you
may found a lot of examples.

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

-- 
With Best Regards,
Andy Shevchenko