Patchwork [v2,01/16] reset: add non CONFIG_RESET_CONTROLLER routines

login
register
mail settings
Submitter Chen-Yu Tsai
Date Jan. 10, 2014, 7 a.m.
Message ID <1389337217-29032-2-git-send-email-wens@csie.org>
Download mbox | patch
Permalink /patch/309163/
State Changes Requested
Delegated to: David Miller
Headers show

Comments

Chen-Yu Tsai - Jan. 10, 2014, 7 a.m.
Some drivers are shared between platforms that may or may not
have RESET_CONTROLLER selected for them. Add dummy functions
when RESET_CONTROLLER is not selected, thereby eliminating the
need for drivers to enclose reset_control_*() calls within
#ifdef CONFIG_RESET_CONTROLLER, #endif

Signed-off-by: Chen-Yu Tsai <wens@csie.org>
---
 include/linux/reset.h | 39 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 39 insertions(+)
Philipp Zabel - Jan. 10, 2014, 1:30 p.m.
Hi,

[Added Ivan, Stephen and Barry to Cc:]

Am Freitag, den 10.01.2014, 15:00 +0800 schrieb Chen-Yu Tsai:
> Some drivers are shared between platforms that may or may not
> have RESET_CONTROLLER selected for them.

I expected that drivers compiled for platforms without reset controllers
but use the reset API should select or depend on RESET_CONTROLLER.
Stubbing out device_reset and reset_control_get will turn a compile time
error into a runtime error for everyone forgetting to do this when
writing a new driver that needs the reset.

>  Add dummy functions
> when RESET_CONTROLLER is not selected, thereby eliminating the
> need for drivers to enclose reset_control_*() calls within
> #ifdef CONFIG_RESET_CONTROLLER, #endif
> 
> Signed-off-by: Chen-Yu Tsai <wens@csie.org>

This was already proposed by Ivan and Barry earlier, and so far we
didn't get to a proper conclusion:

https://lkml.org/lkml/2013/10/10/179
http://lists.infradead.org/pipermail/linux-arm-kernel/2013-June/174758.html

If included, the stubs should at least return an error to indicate a
reset couldn't be performed, but then I lose the compile time error for
drivers which should select RESET_CONTROLLER but don't.

Also this alone won't help you if you build multi-arch kernels where one
platform enables RESET_CONTROLLER and the other expects it to be
disabled.

regards
Philipp

> ---
>  include/linux/reset.h | 39 +++++++++++++++++++++++++++++++++++++++
>  1 file changed, 39 insertions(+)
> 
> diff --git a/include/linux/reset.h b/include/linux/reset.h
> index 6082247..38aa616 100644
> --- a/include/linux/reset.h
> +++ b/include/linux/reset.h
> @@ -4,6 +4,8 @@
>  struct device;
>  struct reset_control;
>  
> +#ifdef CONFIG_RESET_CONTROLLER
> +
>  int reset_control_reset(struct reset_control *rstc);
>  int reset_control_assert(struct reset_control *rstc);
>  int reset_control_deassert(struct reset_control *rstc);
> @@ -14,4 +16,41 @@ struct reset_control *devm_reset_control_get(struct device *dev, const char *id)
>  
>  int device_reset(struct device *dev);
>  
> +#else /* !CONFIG_RESET_CONTROLLER */
> +
> +static inline int reset_control_reset(struct reset_control *rstc)
> +{
> +	return 0;
> +}
> +
> +static inline int reset_control_assert(struct reset_control *rstc)
> +{
> +	return 0;
> +}
> +
> +static inline int reset_control_deassert(struct reset_control *rstc)
> +{
> +	return 0;
> +}

Those should probably have a WARN_ON(1) like the GPIO API stubs.

> +
> +static inline struct reset_control *reset_control_get(struct device *dev,
> +		const char *id)
> +{
> +	return NULL;
> +}
[...]
> +static inline struct reset_control *devm_reset_control_get(struct device *dev,
> +		const char *id)
> +{
> +	return NULL;
> +}

These should return ERR_PTR(-ENOSYS).

> +
> +static inline int device_reset(struct device *dev)
> +{
> +	return 0;
> +}

And this should return -ENOSYS.

Drivers that also need to run on platforms with CONFIG_RESET_CONTROLLER
disabled (and that don't need the reset) should ignore -ENOSYS and
-ENOENT return values from device_reset/(devm_)reset_control_get.

I wonder if it might be a good idea to add a RESET_CONTROLLER_OPTIONAL
that drivers need to select to enable the API stubs? That way we could
keep the compile time error for new drivers that need resets but forget
to select RESET_CONTROLLER.
Or add a
#warning If this driver can work without reset, please select CONFIG_RESET_CONTROLLER_OPTIONAL

Another possibility would be to add device_reset_optional and
(devm_)reset_control_get_optional variants and only provide stubs for
those, but not for device_reset/(devm_)reset_control_get. Then drivers
that need to work on platforms without the reset controller API enabled
could explicitly use the _optional variants, and all other drivers would
still be checked at compile time.

regards
Philipp

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Chen-Yu Tsai - Jan. 17, 2014, 3:46 a.m.
Hi,

On Fri, Jan 10, 2014 at 9:30 PM, Philipp Zabel <p.zabel@pengutronix.de> wrote:
> Hi,
>
> [Added Ivan, Stephen and Barry to Cc:]
>
> Am Freitag, den 10.01.2014, 15:00 +0800 schrieb Chen-Yu Tsai:
>> Some drivers are shared between platforms that may or may not
>> have RESET_CONTROLLER selected for them.
>
> I expected that drivers compiled for platforms without reset controllers
> but use the reset API should select or depend on RESET_CONTROLLER.
> Stubbing out device_reset and reset_control_get will turn a compile time
> error into a runtime error for everyone forgetting to do this when
> writing a new driver that needs the reset.

Since this was the intended behavior, I'll drop this patch and select
RESET_CONTROLLER for the stmmac driver for now.


Thanks
ChenYu

>
>>  Add dummy functions
>> when RESET_CONTROLLER is not selected, thereby eliminating the
>> need for drivers to enclose reset_control_*() calls within
>> #ifdef CONFIG_RESET_CONTROLLER, #endif
>>
>> Signed-off-by: Chen-Yu Tsai <wens@csie.org>
>
> This was already proposed by Ivan and Barry earlier, and so far we
> didn't get to a proper conclusion:
>
> https://lkml.org/lkml/2013/10/10/179
> http://lists.infradead.org/pipermail/linux-arm-kernel/2013-June/174758.html
>
> If included, the stubs should at least return an error to indicate a
> reset couldn't be performed, but then I lose the compile time error for
> drivers which should select RESET_CONTROLLER but don't.
>
> Also this alone won't help you if you build multi-arch kernels where one
> platform enables RESET_CONTROLLER and the other expects it to be
> disabled.
>
> regards
> Philipp
>
>> ---
>>  include/linux/reset.h | 39 +++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 39 insertions(+)
>>
>> diff --git a/include/linux/reset.h b/include/linux/reset.h
>> index 6082247..38aa616 100644
>> --- a/include/linux/reset.h
>> +++ b/include/linux/reset.h
>> @@ -4,6 +4,8 @@
>>  struct device;
>>  struct reset_control;
>>
>> +#ifdef CONFIG_RESET_CONTROLLER
>> +
>>  int reset_control_reset(struct reset_control *rstc);
>>  int reset_control_assert(struct reset_control *rstc);
>>  int reset_control_deassert(struct reset_control *rstc);
>> @@ -14,4 +16,41 @@ struct reset_control *devm_reset_control_get(struct device *dev, const char *id)
>>
>>  int device_reset(struct device *dev);
>>
>> +#else /* !CONFIG_RESET_CONTROLLER */
>> +
>> +static inline int reset_control_reset(struct reset_control *rstc)
>> +{
>> +     return 0;
>> +}
>> +
>> +static inline int reset_control_assert(struct reset_control *rstc)
>> +{
>> +     return 0;
>> +}
>> +
>> +static inline int reset_control_deassert(struct reset_control *rstc)
>> +{
>> +     return 0;
>> +}
>
> Those should probably have a WARN_ON(1) like the GPIO API stubs.
>
>> +
>> +static inline struct reset_control *reset_control_get(struct device *dev,
>> +             const char *id)
>> +{
>> +     return NULL;
>> +}
> [...]
>> +static inline struct reset_control *devm_reset_control_get(struct device *dev,
>> +             const char *id)
>> +{
>> +     return NULL;
>> +}
>
> These should return ERR_PTR(-ENOSYS).
>
>> +
>> +static inline int device_reset(struct device *dev)
>> +{
>> +     return 0;
>> +}
>
> And this should return -ENOSYS.
>
> Drivers that also need to run on platforms with CONFIG_RESET_CONTROLLER
> disabled (and that don't need the reset) should ignore -ENOSYS and
> -ENOENT return values from device_reset/(devm_)reset_control_get.
>
> I wonder if it might be a good idea to add a RESET_CONTROLLER_OPTIONAL
> that drivers need to select to enable the API stubs? That way we could
> keep the compile time error for new drivers that need resets but forget
> to select RESET_CONTROLLER.
> Or add a
> #warning If this driver can work without reset, please select CONFIG_RESET_CONTROLLER_OPTIONAL
>
> Another possibility would be to add device_reset_optional and
> (devm_)reset_control_get_optional variants and only provide stubs for
> those, but not for device_reset/(devm_)reset_control_get. Then drivers
> that need to work on platforms without the reset controller API enabled
> could explicitly use the _optional variants, and all other drivers would
> still be checked at compile time.
>
> regards
> Philipp
>
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/include/linux/reset.h b/include/linux/reset.h
index 6082247..38aa616 100644
--- a/include/linux/reset.h
+++ b/include/linux/reset.h
@@ -4,6 +4,8 @@ 
 struct device;
 struct reset_control;
 
+#ifdef CONFIG_RESET_CONTROLLER
+
 int reset_control_reset(struct reset_control *rstc);
 int reset_control_assert(struct reset_control *rstc);
 int reset_control_deassert(struct reset_control *rstc);
@@ -14,4 +16,41 @@  struct reset_control *devm_reset_control_get(struct device *dev, const char *id)
 
 int device_reset(struct device *dev);
 
+#else /* !CONFIG_RESET_CONTROLLER */
+
+static inline int reset_control_reset(struct reset_control *rstc)
+{
+	return 0;
+}
+
+static inline int reset_control_assert(struct reset_control *rstc)
+{
+	return 0;
+}
+
+static inline int reset_control_deassert(struct reset_control *rstc)
+{
+	return 0;
+}
+
+static inline struct reset_control *reset_control_get(struct device *dev,
+		const char *id)
+{
+	return NULL;
+}
+static inline void reset_control_put(struct reset_control *rstc) {}
+
+static inline struct reset_control *devm_reset_control_get(struct device *dev,
+		const char *id)
+{
+	return NULL;
+}
+
+static inline int device_reset(struct device *dev)
+{
+	return 0;
+}
+
+#endif
+
 #endif