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

First of all, you haven't replied to some of my comments, I assume that you
agree on them and are going to fix as suggested?

...

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

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

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

...

> >> Additionally, include a 'depends on X86' Kconfig entry for
> >> CONFIG_I2C_PIIX4, as the current patch utilizes acpi_dev_get_resources(),
> >> which is compiled only when CONFIG_ACPI is enabled, and CONFIG_ACPI
> >> depends on CONFIG_X86.
> > 
> > Yeah, please don't do that. If it requires ACPI, make it clear, there is
> > no x86 compile-time dependency.
> 
> You mean to say make the dependencies as:
> 
> depends on PCI && HAS_IOPORT && ACPI
> 
> instead of:
> 
> depends on PCI && HAS_IOPORT && X86

Yes, but see below as well about the stubs

~~~vvv
> > Second issue with this is that now you require entire ACPI machinery for
> > the previous cases where it wasn't needed. Imagine an embedded system with
> > limited amount of memory for which you require +1Mbyte just for nothing.
> > 
> > Look how the other do (hint: ifdeffery in the code with stubs).

___^^^

...

> >> +	u8 bank, reg, cmd = 0;
> > 
> > Move cmd assignment into the respective branch of the conditional below, in
> > that case it will be closer and more symmetrical.
> 
> meaning, make the cmd assignment only in the if() case.

Yes.

> Not sure if I understand your remark completely.

	if (...) {
		cmd = 0;
	} else {
		cmd = ...
	}

...

> >> +	if (slave_int & BIT(6)) {
> >> +		/* Slave Interrupt */
> >> +		outb_p(slave_int | BIT(6), ASFSTA);
> >> +		schedule_delayed_work(&adapdata->work_buf, HZ);
> >> +	} else {
> >> +		/* Master Interrupt */
> > 
> > Please, start using inclusive non-offensive terms instead of old 'master/slave'
> > terminology. Nowadays it's a part of the standard AFAIU.
> 
> OK. I get it ( tried to retain the names as mentioned in the AMD ASF
> databook).
> 
> Which one would you advise (instead of master/slave)?

As per official I2C specification. :-)

> Primary/secondary
> Controller/Worker
> Requester/Responder

See, e.g., a93c2e5fe766 ("i2c: reword i2c_algorithm according to newest specification").

> > Note, I'm talking only about comments and messages, the APIs is another story
> > that should be addressed separately.
> > 
> >> +		sb800_asf_update_bits(piix4_smba, SB800_ASF_SLV_INTR, SMBHSTSTS, true);
> >> +	}

...

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

...

> >> +	ret = acpi_dev_get_resources(adev, &res_list, NULL, NULL);
> >> +	if (ret < 0) {
> > 
> >> +		dev_err(&dev->dev, "Error getting ASF ACPI resource: %d\n", ret);
> >> +		return ret;
> > 
> > 		return dev_err_probe(...);
> 
> I thought dev_err_probe(...); is called only from the .probe
> functions. Is that not the case?

I assume you call this due to use of devm_*(). Either devm_*() should be
replaced to non-devm_*() analogues, or these moved to dev_err_probe().

> In the current proposed change, sb800_asf_add_adap() gets called from
> *_probe().
> 
> Or you mean to say, no need for a error print and just do a error return?

No. It's also possible, but this is up to you.

> if (ret < 0)
> 	return ret;
> 
> Likewise for below remarks on dev_err_probe(...);

-- 
With Best Regards,
Andy Shevchenko