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

[Shyam Sundar S K <Shyam-sundar.S-k@amd.com>]

On 9/4/2024 01:36, Andy Shevchenko wrote:
> 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.

Thank you for the quick feedback. I have submitted v2 addressing your
remarks. Kindly take a look.

Thanks,
Shyam