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:
>
> 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?
I agree with your comments. I have only requested further
clarification on a few points where I need more understanding.
>
> ...
>
>>>> 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
>> 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.
>
>> 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 = ...
> }
>
Got it.
> ...
>
>>>> + 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. :-)
Thanks! I will change to controller/target instead of master/slave.
>
>> 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...
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.
Thanks,
Shyam
>
> ...
>
>>>> + 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(...);
>
