[linux-sunxi] [PATCH 1/5] pinctrl: sunxi: refactor pinctrl choice selecting for ARM64

[Icenowy Zheng]

01.03.2017, 02:15, "Andre Przywara" <andre.przywara at arm.com>:
> Hi Icenowy,
>
> (first thing: could you create your series with --cover-letter and fill
> this in? There you could explain what this series is about and also
> state things like dependencies from other patches and the commit that
> you based that on.)
>
> On 28/02/17 17:24, Icenowy Zheng wrote:
>>  ARM64 Allwinner SoCs used to have every pinctrl driver selected in
>>  ARCH_SUNXI. Change this to make their default value to (ARM64 &&
>>  ARCH_SUNXI).
>>
>>  Signed-off-by: Icenowy Zheng <icenowy at aosc.xyz>
>
> Overall this is a nice solution. Thanks for posting this.
>
>>  ---
>>   drivers/pinctrl/sunxi/Kconfig | 9 +++++----
>>   1 file changed, 5 insertions(+), 4 deletions(-)
>>
>>  diff --git a/drivers/pinctrl/sunxi/Kconfig b/drivers/pinctrl/sunxi/Kconfig
>>  index 816015cf7053..92325736d953 100644
>>  --- a/drivers/pinctrl/sunxi/Kconfig
>>  +++ b/drivers/pinctrl/sunxi/Kconfig
>>  @@ -48,8 +48,9 @@ config PINCTRL_SUN8I_H3
>>           select PINCTRL_SUNXI
>>
>>   config PINCTRL_SUN8I_H3_R
>>  - def_bool MACH_SUN8I
>>  - select PINCTRL_SUNXI_COMMON
>>  + def_bool MACH_SUN8I || (ARM64 && ARCH_SUNXI)
>>  + depends on RESET_CONTROLLER
>
> So this looks a bit odd. I take it this is for the extra reset register
> in the PRCM block.
> 1) I don't think this belongs into this patch. If this has been
> forgotten in the past, please make an extra patch for this. It's cheap
> to do so ;-)
> 2) Is this really a "depends on"? Shouldn't this be a select? With
> sunxi_ng clocks we don't need the reset controller for the normal
> peripherals anymore, so this option might not be selected by default in
> the future. And having this "depends on" leads to the PINCTRL_ option
> being hidden if RESET_CONTROLLER isn't selected.
> I think this bites us already in arm64, where ARCH_HAS_RESET_CONTROLLER
> is not enabled for ARCH_SUNXI.

PINCTRL_ options are always hidden now, Maxime, do you
want to change that?

We should enable ARCH_HAS_RESET_CONTROLLER for
ARCH_SUNXI, as sunxi-ng ccu is a reset controller.

Without RESET_CONTROLLER enabled, errors will occur
when linking:
```
drivers/built-in.o: In function `sunxi_ccu_probe':
of_reserved_mem.c:(.text+0x16058): undefined reference to `reset_controller_register'
```

If you don't care, I will make it an additional patch.

And there's really a reset line for the R_PIO pinctrls.

But you're right, this line shouldn't occur in this patch.
I will make a more patch for it.

>
> But as the rest of the patch is fine, if you remove this line:
> Reviewed-by: Andre Przywara <andre.przywara at arm.com>
>
> Cheers,
> Andre.
>
>>  + select PINCTRL_SUNXI
>>
>>   config PINCTRL_SUN8I_V3S
>>           def_bool MACH_SUN8I
>>  @@ -65,11 +66,11 @@ config PINCTRL_SUN9I_A80_R
>>           select PINCTRL_SUNXI
>>
>>   config PINCTRL_SUN50I_A64
>>  - bool
>>  + def_bool ARM64 && ARCH_SUNXI
>>           select PINCTRL_SUNXI
>>
>>   config PINCTRL_SUN50I_H5
>>  - bool
>>  + def_bool ARM64 && ARCH_SUNXI
>>           select PINCTRL_SUNXI
>>
>>   endif