| Message ID | 4553005.HBquOfbqXe@wuerfel (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Feb 18, 2015 at 04:29:10PM +0100, Arnd Bergmann wrote: > The adds a dependency in Kconfig to prevent it from being selected > if the resets are not available. Why is this a better fix than making the reset controller API stub itself out? It's probably not such a big deal here but for devices that could be integrated into SoCs with and without reset controllers it seems like we might want that.
On Thursday 19 February 2015 18:41:55 Mark Brown wrote: > On Wed, Feb 18, 2015 at 04:29:10PM +0100, Arnd Bergmann wrote: > > > The adds a dependency in Kconfig to prevent it from being selected > > if the resets are not available. > > Why is this a better fix than making the reset controller API stub > itself out? It's probably not such a big deal here but for devices that > could be integrated into SoCs with and without reset controllers it > seems like we might want that. I would prefer that too, but it's not what the reset maintainer decided to have, citing b424080a9e0 ("reset: Add optional resets and stubs"): This patch adds device_reset_optional and (devm_)reset_control_get_optional variants that drivers can use to indicate they can function without control over the reset line. For those functions, stubs are added so the drivers can be compiled with CONFIG_RESET_CONTROLLER disabled. Also, device_reset is annotated with __must_check. Drivers ignoring the return value should use device_reset_optional instead. Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de> Reviewed-by: Maxime Ripard <maxime.ripard@free-electrons.com> Reviewed-by: Marek Vasut <marex@denx.de> Acked-by: Wolfram Sang <wsa@the-dreams.de> So the stubs are provided for all functions except device_reset() and reset_control_get(), and drivers using the API need to either call the stubbed-out _optional() versions of these functions or have the explicit dependency. Arnd
On Thu, Feb 19, 2015 at 04:01:29PM +0100, Arnd Bergmann wrote: > So the stubs are provided for all functions except device_reset() and > reset_control_get(), and drivers using the API need to either call the > stubbed-out _optional() versions of these functions or have the explicit > dependency. 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?
On Saturday 21 February 2015 18:44:58 Mark Brown wrote: > On Thu, Feb 19, 2015 at 04:01:29PM +0100, Arnd Bergmann wrote: > > > So the stubs are provided for all functions except device_reset() and > > reset_control_get(), and drivers using the API need to either call the > > stubbed-out _optional() versions of these functions or have the explicit > > dependency. > > 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'. 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. 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. Arnd
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. > 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). > 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.
diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig index ab8dfbef6f1b..b3ae90e4c03c 100644 --- a/drivers/spi/Kconfig +++ b/drivers/spi/Kconfig @@ -507,6 +507,7 @@ config SPI_SH_HSPI config SPI_SIRF tristate "CSR SiRFprimaII SPI controller" depends on SIRF_DMA + depends on RESET_CONTROLLER select SPI_BITBANG help SPI driver for CSR SiRFprimaII SoCs
The sirf spi driver only builds correctly if the reset controller framework is available, otherwise we get an error: drivers/spi/spi-sirf.c: In function 'spi_sirfsoc_probe': drivers/spi/spi-sirf.c:651:2: error: implicit declaration of function 'device_reset' [-Werror=implicit-function-declaration] The adds a dependency in Kconfig to prevent it from being selected if the resets are not available. Signed-off-by: Arnd Bergmann <arnd@arndb.de> Fixes: 8509c55fcb51 ("spi: sirf: reset SPI controller in init stage")