Message-ID: <CAGb2v64hgW39c+gQXp-btpOu2_ddEnGBQk7udMGatCG8We+dBQ@mail.gmail.com>
Date:	Tue, 24 Feb 2015 18:56:44 +0800
From:	Chen-Yu Tsai <wens@...e.org>
To:	Arnd Bergmann <arnd@...db.de>
Cc:	linux-arm-kernel <linux-arm-kernel@...ts.infradead.org>,
	Marek Vasut <marex@...x.de>, Wolfram Sang <wsa@...-dreams.de>,
	linux-kernel <linux-kernel@...r.kernel.org>,
	linux-spi@...r.kernel.org, Mark Brown <broonie@...nel.org>,
	Philipp Zabel <p.zabel@...gutronix.de>,
	Barry Song <Baohua.Song@....com>,
	Maxime Ripard <maxime.ripard@...e-electrons.com>
Subject: Re: [PATCH] spi: sirf: add reset controller dependency

On Tue, Feb 24, 2015 at 6:27 PM, Arnd Bergmann <arnd@...db.de> wrote:
> On Tuesday 24 February 2015 16:50:18 Mark Brown wrote:
>> On Mon, Feb 23, 2015 at 11:01:18PM +0100, Arnd Bergmann wrote:
>> > On Saturday 21 February 2015 18:44:58 Mark Brown wrote:
>>
>> > > In that case a dependency seems wrong, I'd expect to see a select - it's
>> > > a bit obscure to have to grovel around to figure out what magic options
>> > > are needed to make things turn on and resets feel more like a utility
>> > > thing than a control bus (which tend to be the things we depend on).
>> > > Dunno, perhaps I'm wrong?
>>
>> > Mixing 'select' and 'depends on' causes recursive dependencies, and
>> > there are already lots of drivers that do 'depends on RESET_CONTROLLER'.
>>
>> Well, perhaps that's the cleanup we should be doing then...  one of the
>> big problems with some of the other randconfig work there's been is that
>> a lot of the patches just add dependencies without looking at if that
>> makes sense.
>
> I generally try to keep the big picture in mind, but sometimes I take
> an easier way out to avoid starting a long discussion.
>
>> > Most users of this symbol seem to follow the strategy of selecting
>> > RESET_CONTROLLER when a driver is there to provide the functionality,
>> > but depending on it when a driver uses it. We are however a bit
>> > inconsistent here and it would be nice to clean it up.
>>
>> Right, to me that feels the opposite way round to how we normally do
>> things - the drivers for the subsystem normally depend on the subsystem
>> (or are hidden by it).
>
> I think it's the more common of the two approaches, but we are definitely
> inconsistent here. To make everything consistent here, I'd just do this:
>
> diff --git a/drivers/gpu/drm/sti/Kconfig b/drivers/gpu/drm/sti/Kconfig
> index fbccc105819b..0670aa17409d 100644
> --- a/drivers/gpu/drm/sti/Kconfig
> +++ b/drivers/gpu/drm/sti/Kconfig
> @@ -1,7 +1,7 @@
>  config DRM_STI
>         tristate "DRM Support for STMicroelectronics SoC stiH41x Series"
>         depends on DRM && (SOC_STIH415 || SOC_STIH416 || ARCH_MULTIPLATFORM) && HAVE_DMA_ATTRS
> -       select RESET_CONTROLLER
> +       depends on RESET_CONTROLLER
>         select DRM_KMS_HELPER
>         select DRM_GEM_CMA_HELPER
>         select DRM_KMS_CMA_HELPER
> diff --git a/drivers/net/ethernet/stmicro/stmmac/Kconfig b/drivers/net/ethernet/stmicro/stmmac/Kconfig
> index 7d3af190be55..545b442253e4 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/Kconfig
> +++ b/drivers/net/ethernet/stmicro/stmmac/Kconfig
> @@ -1,11 +1,11 @@
>  config STMMAC_ETH
>         tristate "STMicroelectronics 10/100/1000 Ethernet driver"
>         depends on HAS_IOMEM && HAS_DMA
> +       depends on RESET_CONTROLLER
>         select MII
>         select PHYLIB
>         select CRC32
>         select PTP_1588_CLOCK
> -       select RESET_CONTROLLER

I was the one that introduced this. At the time a generic binding for the
stmmac core was requested, which supported an optional reset control. There
wasn't an *_optional stub available, so I went with this. See

    http://lists.infradead.org/pipermail/linux-arm-kernel/2014-January/224684.html

With commit b424080a9e08 ("reset: Add optional resets and stubs") in, I
think using devm_reset_control_get() in the driver is the proper solution.
The hardware is found in quite a few platforms, and not all of them have
reset controllers.


ChenYu

>         ---help---
>           This is the driver for the Ethernet IPs are built around a
>           Synopsys IP Core and only tested on the STMicroelectronics
>
>> > In this particular patch, I'm just following what others do.
>>
>> > We should probably 'select ARCH_HAS_RESET_CONTROLLER' unconditionally
>> > for ARM ARCH_MULTIPLATFORM, as it's a bit silly to select both
>> > ARCH_HAS_RESET_CONTROLLER and RESET_CONTROLLER from platform code.
>>
>> That does seem a bit odd.  TBH I'm never sure that ARCH_HAS_ is that
>> good an idea for the driver things, most of them can just as reasonably
>> be used by off-SoC things.
>
> I'm totally fine with a patch to kill that off. I suspect it was introduced
> in order to not show up on x86 and stay under Linus' radar. He does get
> upset sometimes about seeing too many options for stuff he does not care
> about, especially when it breaks on x86.
>
>         Arnd
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@...ts.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists