Re: [EXTERNAL] Re: [PATCH v3 3/6] soc: ti: pruss: Add pruss_cfg_read()/update() API

[Roger Quadros <rogerq@kernel.org>]

On 08/03/2023 13:36, Md Danish Anwar wrote:
> Hi Roger,
> 
> On 08/03/23 13:57, Roger Quadros wrote:
>> Hi,
>>
>> On 06/03/2023 13:09, MD Danish Anwar wrote:
>>> From: Suman Anna <s-anna@ti.com>
>>>
>>> Add two new generic API pruss_cfg_read() and pruss_cfg_update() to
>>> the PRUSS platform driver to allow other drivers to read and program
>>> respectively a register within the PRUSS CFG sub-module represented
>>> by a syscon driver. This interface provides a simple way for client
>>
>> Do you really need these 2 functions to be public?
>> I see that later patches (4-6) add APIs for doing specific things
>> and that should be sufficient than exposing entire CFG space via
>> pruss_cfg_read/update().
>>
>>
> 
> I think the intention here is to keep this APIs pruss_cfg_read() and
> pruss_cfg_update() public so that other drivers can read / modify PRUSS config
> when needed.

Where are these other drivers? If they don't exist then let's not make provision
for it now.
We can provide necessary API helpers when needed instead of letting client drivers
do what they want as they can be misused and hard to debug.

> 
> The later patches (4-6) add APIs to do specific thing, but those APIs also
> eventually call pruss_cfg_read/update().

They can still call them but they need to be private to pruss.c

> 
>>> drivers without having them to include and parse the CFG syscon node
>>> within their respective device nodes. Various useful registers and
>>> macros for certain register bit-fields and their values have also
>>> been added.
>>>
>>> It is the responsibility of the client drivers to reconfigure or
>>> reset a particular register upon any failures.
>>>
>>> Signed-off-by: Suman Anna <s-anna@ti.com>
>>> Co-developed-by: Grzegorz Jaszczyk <grzegorz.jaszczyk@linaro.org>
>>> Signed-off-by: Grzegorz Jaszczyk <grzegorz.jaszczyk@linaro.org>
>>> Signed-off-by: Puranjay Mohan <p-mohan@ti.com>
>>> ---
>>>  drivers/soc/ti/pruss.c           |  41 +++++++++++++
>>>  include/linux/remoteproc/pruss.h | 102 +++++++++++++++++++++++++++++++
>>>  2 files changed, 143 insertions(+)
>>>
>>> diff --git a/drivers/soc/ti/pruss.c b/drivers/soc/ti/pruss.c
>>> index c8053c0d735f..537a3910ffd8 100644
>>> --- a/drivers/soc/ti/pruss.c
>>> +++ b/drivers/soc/ti/pruss.c
>>> @@ -164,6 +164,47 @@ int pruss_release_mem_region(struct pruss *pruss,
>>>  }
>>>  EXPORT_SYMBOL_GPL(pruss_release_mem_region);
>>
>> cheers,
>> -roger
>