On 21/09/18 17:03, Hans de Goede wrote:
> Hi,
>
> On 09/21/2018 03:01 PM, Adrian Hunter wrote:
>> On 19/09/18 22:15, Hans de Goede wrote:
>>> Commit e6ce0ce34f65 ("ACPI / LPSS: Add device link for CHT SD card
>>> dependency on I2C") tries to add device links from consumer to
>>> supplier *and* the other way around.
>>
>> That is not the intention. The intention is to add a link irrespective of
>> which device (consumer or supplier) is created last.
>
> Ah I see an upon rereading the code you are right.
>
>>> When the consumer gets registered the supplier is not yet registered, so
>>> acpi_lpss_find_device() cannot find it and we never end up adding the
>>> supplier link. Although not intended this is a good thing, because the
>>> device links are dependencies and if we add a link in both directions the
>>> power-management core will not know which one to suspend/resume first.
>>
>> Did that actually happen? It seems to me that could only happen if the _DEP
>> methods also pointed to each other.
>
> You are right again.
>
> So I guess that this patch can be dropped.
>
> Note that for the new GPU -> PMIC I2C controller consumer -> supplier link this
> patchset adds we rely on probe ordering. Relying on probe ordering works
> for the SDCARD -> PMIC I2C too, so we could keep this patch as it does
> simplify the code. If we do that the commit message needs to be corrected
> of course.
IIRC the ACPI platform devices end up being created in the order the device
nodes appear in the DSDT, so no guarantee of consumer before supplier.
>
> Regards,
>
> Hans
>
>
>>> I've verified through debug printk-s that indeed only the consumer
>>> to supplier dependency gets added even before this commit removes
>>> the code to add the link the other way.
>>>
>>> Signed-off-by: Hans de Goede <hdegoede@xxxxxxxxxx>
>>> ---
>>> drivers/acpi/acpi_lpss.c | 25 -------------------------
>>> 1 file changed, 25 deletions(-)
>>>
>>> diff --git a/drivers/acpi/acpi_lpss.c b/drivers/acpi/acpi_lpss.c
>>> index 83875305b1d4..40a8cab81cbd 100644
>>> --- a/drivers/acpi/acpi_lpss.c
>>> +++ b/drivers/acpi/acpi_lpss.c
>>> @@ -486,13 +486,6 @@ static bool acpi_lpss_is_supplier(struct acpi_device
>>> *adev,
>>> link->supplier_hid, link->supplier_uid);
>>> }
>>> -static bool acpi_lpss_is_consumer(struct acpi_device *adev,
>>> - const struct lpss_device_links *link)
>>> -{
>>> - return hid_uid_match(acpi_device_hid(adev), acpi_device_uid(adev),
>>> - link->consumer_hid, link->consumer_uid);
>>> -}
>>> -
>>> struct hid_uid {
>>> const char *hid;
>>> const char *uid;
>>> @@ -559,21 +552,6 @@ static void acpi_lpss_link_consumer(struct device
>>> *dev1,
>>> put_device(dev2);
>>> }
>>> -static void acpi_lpss_link_supplier(struct device *dev1,
>>> - const struct lpss_device_links *link)
>>> -{
>>> - struct device *dev2;
>>> -
>>> - dev2 = acpi_lpss_find_device(link->supplier_hid, link->supplier_uid);
>>> - if (!dev2)
>>> - return;
>>> -
>>> - if (acpi_lpss_dep(ACPI_COMPANION(dev1), ACPI_HANDLE(dev2)))
>>> - device_link_add(dev1, dev2, link->flags);
>>> -
>>> - put_device(dev2);
>>> -}
>>> -
>>> static void acpi_lpss_create_device_links(struct acpi_device *adev,
>>> struct platform_device *pdev)
>>> {
>>> @@ -584,9 +562,6 @@ static void acpi_lpss_create_device_links(struct
>>> acpi_device *adev,
>>> if (acpi_lpss_is_supplier(adev, link))
>>> acpi_lpss_link_consumer(&pdev->dev, link);
>>> -
>>> - if (acpi_lpss_is_consumer(adev, link))
>>> - acpi_lpss_link_supplier(&pdev->dev, link);
>>> }
>>> }
>>>
>>
>
