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

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

On Wed, Sep 11, 2024 at 10:07:42PM +0530, 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:

...

> >> +	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.

It all depends on how you use that. The devm_ioport_map() is just a trick (in
combination with ioreadXX()/iowriteXX() APIs) to have it in "mapped" address
space.

> > and use devm_ioport_map() somewhere (see
> > drivers/pinctl/intel/pinctrl-lynxpoint.c, for example)

...

> >> +	LIST_HEAD(res_list);

> >> +	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.

With platform device driver you don't need all this as you are repeating what
drivers/acpi/acpi_platform.c already does it for all ACPI devices.

> >> +	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?

I meant that entire block is just a dup of the existing code. See above.
Instead use simple platform_get_resource() and similar to retrieve this
information.

...

> >> +	i2c_set_adapdata(&asf_dev->adap, asf_dev);
> > 
> > Is it used?
> 
> Yes, in the subsequent patches.

I wasn't Cc'ed on the series. Please, make sure you Cc me in the entire series.

...

> >> +	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.

Yes, and I meant piix4 driver where you call this API.

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

Noted!

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

-- 
With Best Regards,
Andy Shevchenko