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
