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 22:07, Shyam Sundar S K wrote:
> 
> 
> 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?

Ignore this. I understand what you mean now..

Thanks,
Shyam

> 
>>
>>> +	/* 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;
>>