Re: [PATCH v4 4/8] i2c: amd-asf: Add ACPI support for AMD ASF Controller

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

On 9/11/2024 20:46, Andy Shevchenko wrote:
> On Wed, Sep 11, 2024 at 05:24:03PM +0530, Shyam Sundar S K wrote:
>> The AMD ASF controller is presented to the operating system as an ACPI
>> device. The AMD ASF driver can use ACPI to obtain 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 should not be configured.
> 
> With brief look this is much better than the previous version(s).
> Thank you for rewriting it this way!
> 
> Some comments below.
> 
> ...
> 
>> +#include <linux/acpi.h>
> 
> No need (see below)
> 
> + device.h
> + errno.h
> + gfp_types.h
> 
>> +#include <linux/i2c-smbus.h>
> 
> This should be i2c.h
> 
> + mod_devicetable.h
> + module.h
> 
>> +#include <linux/platform_device.h>
> 
>> +#include <linux/slab.h>
> 
> Not in use.
> 
> + sprintf.h
> 
>> +#include "i2c-piix4.h"
>> +
>> +static const char *sb800_asf_port_name = " port 1";
>> +
>> +struct amd_asf_dev {
>> +	struct device *dev;
>> +	struct i2c_adapter adap;
> 
> Make it first member, it might help if we ever do a container_of() against
> this.
> 
>> +	struct sb800_mmio_cfg mmio_cfg;
>> +	unsigned short port_addr;
> 
> What you probably want is to have
> 
> 	void __iomem *addr;
> 

I will address the above remarks in the next patch.

I believe this should remain "unsigned short" because

- it is defined a unsigned short in i2c_piix4
- this is just a port address (like 0xb00, 0xb20) and not a real iomem
address.


> and use devm_ioport_map() somewhere (see
> drivers/pinctl/intel/pinctrl-lynxpoint.c, for example)
> 
>> +};
> 
>> +static int amd_asf_probe(struct platform_device *pdev)
>> +{
>> +	struct resource_entry *rentry;
>> +	struct amd_asf_dev *asf_dev;
>> +	struct acpi_device *adev;
>> +	LIST_HEAD(res_list);
>> +	int ret;
> 
>> +	adev = ACPI_COMPANION(&pdev->dev);
>> +	if (!adev)
>> +		return dev_err_probe(&pdev->dev, -ENODEV, "Failed to get ASF device\n");
> 
> No need. You will get here only if enumerated via ACPI (or if it's out-of-tree
> board file which we do not care about at all).

Not sure if I understand your comment correctly. But I used
ACPI_COMPANION to retrieve the acpi device that needs to be passed to
acpi_dev_get_resources(struct acpi_device *, ...) to address your
previous remarks.

> 
>> +	asf_dev = devm_kzalloc(&pdev->dev, sizeof(*asf_dev), GFP_KERNEL);
>> +	if (!asf_dev)
>> +		return dev_err_probe(&pdev->dev, -ENOMEM, "Failed to allocate memory\n");
>> +
>> +	asf_dev->dev = &pdev->dev;
> 
>> +	platform_set_drvdata(pdev, asf_dev);
> 
> Is it used?
> 
>> +	asf_dev->adap.owner = THIS_MODULE;
>> +	asf_dev->mmio_cfg.use_mmio = true;
>> +	asf_dev->adap.class = I2C_CLASS_HWMON;
> 
>> +	ret = acpi_dev_get_resources(adev, &res_list, NULL, NULL);
>> +	if (ret < 0)
>> +		return dev_err_probe(&pdev->dev, ret, "Error getting ASF ACPI resource: %d\n", ret);
>> +
>> +	list_for_each_entry(rentry, &res_list, node) {
>> +		switch (resource_type(rentry->res)) {
>> +		case IORESOURCE_IO:
>> +			asf_dev->port_addr = rentry->res->start;
>> +			break;
>> +		default:
>> +			dev_warn(&adev->dev, "Invalid ASF resource\n");
>> +			break;
>> +		}
>> +	}
>> +
>> +	acpi_dev_free_resource_list(&res_list);
> 
> Now this is a duplicate of what ACPI glue layer does. You have these already
> available as platform device resources.

looking at drivers/acpi/resource.c acpi_dev_get_resources() mentions
that the caller should call acpi_dev_free_resource_list(). Is that not
the case?

> 
>> +	/* Set up the sysfs linkage to our parent device */
>> +	asf_dev->adap.dev.parent = &pdev->dev;
>> +
>> +	snprintf(asf_dev->adap.name, sizeof(asf_dev->adap.name),
>> +		 "SMBus ASF adapter%s at %04x", sb800_asf_port_name, asf_dev->port_addr);
> 
>> +	i2c_set_adapdata(&asf_dev->adap, asf_dev);
> 
> Is it used?

Yes, in the subsequent patches.

> 
>> +	ret = i2c_add_adapter(&asf_dev->adap);
> 
> Use devm variant of this casll.
> 
>> +	if (ret) {
> 
>> +		release_region(asf_dev->port_addr, SMBIOSIZE);
> 
> Why?
> 
>> +		return ret;
>> +	}
>> +
>> +	return 0;
> 
> 	return devm_i2c_add_adapter(...);
> 
>> +}
>> +
>> +static void amd_asf_remove(struct platform_device *pdev)
>> +{
>> +	struct amd_asf_dev *dev = platform_get_drvdata(pdev);
> 
>> +	if (dev->port_addr) {
> 
> Redundant.
> 
>> +		i2c_del_adapter(&dev->adap);
> 
> With devm this should be removed.
> 
>> +		release_region(dev->port_addr, SMBIOSIZE);
> 
> Why?

My bad :-( Will remove it.

> 
>> +	}
>> +}
>> +
>> +static const struct acpi_device_id amd_asf_acpi_ids[] = {
>> +	{"AMDI001A", 0},
> 
> 	{ "AMDI001A" },
> 
>> +	{ }
>> +};
>> +MODULE_DEVICE_TABLE(acpi, amd_asf_acpi_ids);
>> +
>> +static struct platform_driver amd_asf_driver = {
>> +	.driver = {
>> +		.name = "i2c-amd-asf",
>> +		.acpi_match_table = amd_asf_acpi_ids,
>> +	},
>> +	.probe = amd_asf_probe,
>> +	.remove_new = amd_asf_remove,
>> +};
>> +module_platform_driver(amd_asf_driver);
> 
> ...
> 
>> +	status = acpi_get_handle(NULL, (acpi_string)SB800_ASF_ACPI_PATH, &handle);
> 
> Does it compile with CONFIG_ACPI=n?

I have used a explicit 'depends on' ACPI to this driver soon that LKP
does not complain with a randconfig.

> 
> Also don't you need to include acpi.h for this? Or is it already there?
> (I haven't checked).

acpi_get_handle() is defined in acpi.h.

please assume the rest of the unanswered remarks as "I agree" :-)

Thanks,
Shyam

> 
>> +	if (ACPI_SUCCESS(status))
>> +		is_asf = true;
>