On Wed, Mar 1, 2017 at 2:55 AM, Icenowy Zheng <icenowy@xxxxxxxx> wrote: > > > 01.03.2017, 02:15, "Andre Przywara" <andre.przywara@xxxxxxx>: >> 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@xxxxxxxx> >> >> 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. Agreed. > 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. Depends on is the right thing here, since this driver depends on the reset controller subsystem to be present for any devm_reset_control_get calls. Now if this pinctrl has a reset line, but is missing that call, it needs to be fixed. It seems only A23 and A31 R_PIO drivers have them. Regards ChenYu > > 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@xxxxxxx> >> >> 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 -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
