Re: [PATCH v3 3/5] i2c: piix4: Add ACPI support for ASF SMBus device

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

On Fri, Sep 06, 2024 at 08:41:19PM +0530, Shyam Sundar S K wrote:
> On 9/6/2024 20:10, Andy Shevchenko wrote:
> > On Fri, Sep 06, 2024 at 06:50:48PM +0530, Shyam Sundar S K wrote:
> >> On 9/6/2024 17:54, Andy Shevchenko wrote:
> >>> On Fri, Sep 06, 2024 at 12:41:59PM +0530, Shyam Sundar S K wrote:

...

> >>>> The AMD ASF controller is presented to the operating system as an ACPI
> >>>> device. The piix4 driver can obtain the ASF handle through ACPI to
> >>>> retrieve information about the ASF controller's attributes, such as the
> >>>> ASF address space and interrupt number, and to handle ASF interrupts.
> >>>
> >>> Can you share an excerpt of DSDT to see how it looks like?
> >>
> >> Device (ASFC)
> >> {
> >> 	...
> > 
> > Can you put the necessary bits for the enumeration (you may replace some IDs if
> > they are not public yet to something like XX..XX or xx..xx)?
> 
> Name (_HID, "AMDIXXXX")  // _HID: Hardware ID
> Name (_UID, Zero)  // _UID: Unique ID

Thank you!

Now a question, why your case can't have a separate (platform) device driver?

> >>     Method (_CRS, 0, NotSerialized)  // _CRS: Current Resource Settings
> >>     {
> >>         Name (ASBB, ResourceTemplate ()
> >>         {
> >>             Interrupt (ResourceConsumer, Level, ActiveLow, Shared, ,, )
> >>             {
> >>                 0x00000014,
> >>             }
> >>             IO (Decode16,
> >>                 0x0B20,             // Range Minimum
> >>                 0x0B20,             // Range Maximum
> > 
> > Typo in value? Shouldn't this be 0x0b3f?
> 
> Its is 0xb20, that is meant for ASF.

Yes, I mixed up IO() vs. Memory*() resource. The IO() has two values for
the start address and you fixed that to the above mentioned value.

TL;DR: this looks okay.

> >>                 0x00,               // Alignment
> >>                 0x20,               // Length
> >>                 )
> >>             Memory32Fixed (ReadWrite,
> >>                 0xFEC00040,         // Address Base
> >>                 0x00000100,         // Address Length
> >>                 )
> >>         })
> >>         Return (ASBB) /* \_SB_.ASFC._CRS.ASBB */
> >>     }
> >> 	...
> >> }

...

> >>>> +	status = acpi_get_handle(NULL, SB800_ASF_ACPI_PATH, &handle);
> >>>> +	if (ACPI_FAILURE(status))
> >>>> +		return -ENODEV;
> >>>> +
> >>>> +	adev = acpi_fetch_acpi_dev(handle);
> >>>> +	if (!adev)
> >>>> +		return -ENODEV;
> >>>
> >>> This approach I don't like. I would like to see DSDT for that
> >>> as I mentioned above.
> >>
> >> I have posted the DSDT. Can you please elaborate your remarks?
> > 
> > Not that parts that affect this...
> 
> Alright, I have posted the _HID enumeration details above. Please let
> me know if using acpi_fetch_acpi_dev() is acceptable or if there's a
> better alternative.

> I am open to making changes based on these clarifications.

Since you have a proper Device object in ACPI, it seems to me that you should
do other way around, i.e. having a platform device driver for this ACPI device
(based on _HID) and use piix4 as a library for it.

-- 
With Best Regards,
Andy Shevchenko