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