Re: [PATCH v8 02/11] power: reset: Add reboot driver for brcmstb

[Brian Norris <computersforpeace@gmail.com>]

+ Sebastian

On Tue, Jul 22, 2014 at 11:02:07PM +0200, Arnd Bergmann wrote:
> On Tuesday 22 July 2014 13:02:13 Brian Norris wrote:
> > How about a third option, where we drop the 'select' statement and
> > set POWER_RESET_BRCMSTB to be 'default y'? Then we don't have to modify
> > the defconfig, and it gives the added bonus of choosing a sane default
> > even if you're not based on the multi_v7_defconfig. i.e.:
> > 
> > diff --git a/arch/arm/mach-bcm/Kconfig b/arch/arm/mach-bcm/Kconfig
> > index 58c01aed9752..634de7b7fd28 100644
> > --- a/arch/arm/mach-bcm/Kconfig
> > +++ b/arch/arm/mach-bcm/Kconfig
> > @@ -94,7 +94,6 @@ config ARCH_BRCMSTB
> >         select MIGHT_HAVE_PCI
> >         select HAVE_SMP
> >         select HAVE_ARM_ARCH_TIMER
> > -       select POWER_RESET_BRCMSTB
> >         select BRCMSTB_GISB_ARB
> >         select BRCMSTB_L2_IRQ
> >         help
> > diff --git a/drivers/power/reset/Kconfig b/drivers/power/reset/Kconfig
> > index fcb9825debe5..ab5d57e2766d 100644
> > --- a/drivers/power/reset/Kconfig
> > +++ b/drivers/power/reset/Kconfig
> > @@ -23,6 +23,7 @@ config POWER_RESET_AXXIA
> >  config POWER_RESET_BRCMSTB
> >         bool "Broadcom STB reset driver"
> >         depends on POWER_RESET && ARCH_BRCMSTB
> > +       default y
> >         help
> >           This driver provides restart support for ARM-based Broadcom STB
> >           boards.
> 
> I don't like this too much. Why do you want to allow disabling the
> driver if you make it 'default y' in the first place?

Not much of a reason, but I thought configurability is a positive.
There's really not a good reason to not have this driver though; I can't
imagine someone wanting to build our platform but not be able to reboot
it.

> We try to avoid 'default y' for user-selectable drivers in general.

I didn't know that. A default is technically different than "should a
user be able to disable this," but I suppose the policy makes sense.

> I noticed that in my example, I was missing the default.

I see. That's why I immediately ruled it out.

> It should have been 
> 
> config POWER_RESET_BRCMSTB
>         bool "Broadcom STB reset driver" if COMPILE_TEST
>         depends on POWER_RESET && ARM
> 	default ARCH_BRCMSTB

OK, that's OK with me if it's OK with Sebastian, et al. Respin coming
soon, separate from the mach-bcm and ARM stuff...

(BTW, we might consider rewriting the other config entries to be
similar, if we really want to make this things consistently
nitpick-proof.)

> This way, it always gets selected when ARCH_BRCMSTB is on and
> COMPILE_TEST is off. With COMPILE_TEST enabled, it defaults to
> ARCH_BRCMSTB but can be enabled or disabled for the purpose
> of compile testing.

Thanks,
Brian