Re: [PATCH V4 05/11] coresight: replicator: Move ACPI support from AMBA driver to platform driver

[Anshuman Khandual]

On 2/15/24 16:55, Suzuki K Poulose wrote:
> On 15/02/2024 11:23, Suzuki K Poulose wrote:
>> Hi Anshuman
>>
>> On 23/01/2024 05:46, Anshuman Khandual wrote:
>>> Add support for the dynamic replicator device in the platform driver, which
>>> can then be used on ACPI based platforms. This change would now allow
>>> runtime power management for repliacator devices on ACPI based systems.
>>>
>>> The driver would try to enable the APB clock if available. Also, rename the
>>> code to reflect the fact that it now handles both static and dynamic
>>> replicators.
>>>
>>> Cc: Lorenzo Pieralisi <lpieralisi@xxxxxxxxxx>
>>> Cc: Sudeep Holla <sudeep.holla@xxxxxxx>
>>> Cc: Suzuki K Poulose <suzuki.poulose@xxxxxxx>
>>> Cc: Mike Leach <mike.leach@xxxxxxxxxx>
>>> Cc: James Clark <james.clark@xxxxxxx>
>>> Cc: linux-acpi@xxxxxxxxxxxxxxx
>>> Cc: linux-arm-kernel@xxxxxxxxxxxxxxxxxxx
>>> Cc: linux-kernel@xxxxxxxxxxxxxxx
>>> Cc: coresight@xxxxxxxxxxxxxxxx
>>> Tested-by: Sudeep Holla <sudeep.holla@xxxxxxx> # Boot and driver probe only
>>> Acked-by: Sudeep Holla <sudeep.holla@xxxxxxx> # For ACPI related changes
>>> Signed-off-by: Anshuman Khandual <anshuman.khandual@xxxxxxx>
>>
>> I think the patch is doing three different things:
>>
>> 1) Use new helper to register/remove AMBA/Platform drivers
>> 2) Refactor replicator_probe() to make sure it can be reused for platform/amba driver, by moving the pm_runtime_put() to the callers.
>> 3) Actually moving the ACPI driver to Platform driver
>>
>> While (1) and (3) are obvious, (2) gave me hard time to review this
>> patch, without proper description. If you don't mind, are you able to
>> split the patch and add proper description of the 3 changes mentioned
>> above.
>>
> 
> You could even move (1) for all the existing drivers into a single patch
> or even fold it with the patch that introduces the helpers. That way it

There are only two existing coresight devices with both AMBA and platform
drivers available i.e replicator and funnel. Such devices could use these
new helpers right from the beginning. As you mentioned earlier such changes
might be folded back into the patch adding the helpers.

But coresight devices such as catu, tpiu, tmc, stm and debug don't have
platform drivers to begin with. Hence the helpers could only be used in
their respective patches adding platform drivers.

> is cleaner and easier to review. And (2) & (3) could be in the same patch for each driver, but please add something in the description for (2).

Please find the updated commit message here, does this look okay ?

    coresight: replicator: Move ACPI support from AMBA driver to platform driver

    Add support for the dynamic replicator device in the platform driver, which
    can then be used on ACPI based platforms. This change would now allow
    runtime power management for replicator devices on ACPI based systems.

    The driver would try to enable the APB clock if available. Also, rename the
    code to reflect the fact that it now handles both static and dynamic
    replicators. But first this refactors replicator_probe() making sure it can
    be used both for platform and AMBA drivers, by moving the pm_runtime_put()
    to the callers.