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