Messages in this thread | ![/](/images/icornerl.gif) | | Subject | Re: [PATCH 2/2] soc: ti: pruss: add support for AM18XX/OMAP-L138 PRUSS | From | David Lechner <> | Date | Sat, 16 Jan 2021 14:15:38 -0600 |
| |
On 1/15/21 6:52 PM, Suman Anna wrote: > Hi David, > > On 1/4/21 12:30 PM, David Lechner wrote: >> This adds support for the PRUSS found in AM18XX/OMAP-L138. This PRUSS >> doesn't have a CFG register, so that is made optional as selected by >> the device tree compatible string. >> >> ARCH_DAVINCI is added in the Kconfig so that the driver can be selected >> on that platform. >> >> Signed-off-by: David Lechner <david@lechnology.com> >> --- >> drivers/soc/ti/Kconfig | 2 +- >> drivers/soc/ti/pruss.c | 76 ++++++++++++++++++++++++------------------ >> 2 files changed, 45 insertions(+), 33 deletions(-) >> >> diff --git a/drivers/soc/ti/Kconfig b/drivers/soc/ti/Kconfig >> index 7e2fb1c16af1..7a692a21480a 100644 >> --- a/drivers/soc/ti/Kconfig >> +++ b/drivers/soc/ti/Kconfig >> @@ -85,7 +85,7 @@ config TI_K3_SOCINFO >> >> config TI_PRUSS >> tristate "TI PRU-ICSS Subsystem Platform drivers" >> - depends on SOC_AM33XX || SOC_AM43XX || SOC_DRA7XX || ARCH_KEYSTONE || ARCH_K3 >> + depends on ARCH_DAVINCI || SOC_AM33XX || SOC_AM43XX || SOC_DRA7XX || ARCH_KEYSTONE || ARCH_K3 >> select MFD_SYSCON >> help >> TI PRU-ICSS Subsystem platform specific support. >> diff --git a/drivers/soc/ti/pruss.c b/drivers/soc/ti/pruss.c >> index 5d6e7132a5c4..bfaf3ff74b01 100644 >> --- a/drivers/soc/ti/pruss.c >> +++ b/drivers/soc/ti/pruss.c >> @@ -24,10 +24,12 @@ >> * struct pruss_private_data - PRUSS driver private data >> * @has_no_sharedram: flag to indicate the absence of PRUSS Shared Data RAM >> * @has_core_mux_clock: flag to indicate the presence of PRUSS core clock >> + * @has_cfg: flag to indicate the presence of PRUSS CFG registers > > I recommend to change this to a negative flag as the Davinci platforms are the > only ones that don't have CFG (being the very first SoCs with a PRUSS IP) > sub-module.
Negative flags hurt my brain. :-)
I was actually thinking about submitting a patch to convert has_no_sharedram to a positive flag as well. But I understand the sentiment of only setting the flag true for the odd case rather than the usual case.
> >> */ >> struct pruss_private_data { >> bool has_no_sharedram; >> bool has_core_mux_clock; >> + bool has_cfg; >> }; >> >> static void pruss_of_free_clk_provider(void *data) >> @@ -239,42 +241,44 @@ static int pruss_probe(struct platform_device *pdev) >> goto rpm_disable; >> } >> > > And use it here to skip all the cfg code parsing. All the below delta is just > for the additional indentation for the flag. If you don't like goto's in > non-error paths, then we can refactor the CFG parse code into a separate function. >
Refactoring to a separate function sounds good to me.
| ![\](/images/icornerr.gif) |