[PATCH 1/3] watchdog: Select CONFIG_PARAMETER

[PATCH 1/3] watchdog: Select CONFIG_PARAMETER

[Christian Eggers]

Without CONFIG_PARAMETER, watchdog_register() will always fail.

Signed-off-by: Christian Eggers <ceggers@arri.de>
---
 drivers/watchdog/Kconfig | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index 45dd41a2a..34b7fea39 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig
@@ -4,6 +4,7 @@ config WATCHDOG_IMX_RESET_SOURCE
 
 menuconfig WATCHDOG
 	bool "Watchdog support"
+	select PARAMETER
 	help
 	  Many platforms support a watchdog to keep track of a working machine.
 	  This framework provides routines to handle these watchdogs.
-- 
Christian Eggers
Embedded software developer

Arnold & Richter Cine Technik GmbH & Co. Betriebs KG
Sitz: Muenchen - Registergericht: Amtsgericht Muenchen - Handelsregisternummer: HRA 57918
Persoenlich haftender Gesellschafter: Arnold & Richter Cine Technik GmbH
Sitz: Muenchen - Registergericht: Amtsgericht Muenchen - Handelsregisternummer: HRB 54477
Geschaeftsfuehrer: Dr. Michael Neuhaeuser; Stephan Schenk; Walter Trauninger; Markus Zeiler


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH 2/3] usb: otg: Select CONFIG_PARAMETER

[Christian Eggers]

Without CONFIG_PARAMETER, usb_register_otg_device() will always fail.

Signed-off-by: Christian Eggers <ceggers@arri.de>
---
 drivers/usb/otg/Kconfig | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/usb/otg/Kconfig b/drivers/usb/otg/Kconfig
index 2c094452b..20b4d7f8c 100644
--- a/drivers/usb/otg/Kconfig
+++ b/drivers/usb/otg/Kconfig
@@ -9,4 +9,5 @@ config USB_TWL4030
 
 config USB_OTGDEV
 	bool
+	select PARAMETER
 
-- 
Christian Eggers
Embedded software developer

Arnold & Richter Cine Technik GmbH & Co. Betriebs KG
Sitz: Muenchen - Registergericht: Amtsgericht Muenchen - Handelsregisternummer: HRA 57918
Persoenlich haftender Gesellschafter: Arnold & Richter Cine Technik GmbH
Sitz: Muenchen - Registergericht: Amtsgericht Muenchen - Handelsregisternummer: HRB 54477
Geschaeftsfuehrer: Dr. Michael Neuhaeuser; Stephan Schenk; Walter Trauninger; Markus Zeiler


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH 3/3] globalvar: Select CONFIG_PARAMETER

[Christian Eggers]

Without CONFIG_PARAMETER, __nvar_add() will always fail.

Signed-off-by: Christian Eggers <ceggers@arri.de>
---
 common/Kconfig | 1 +
 1 file changed, 1 insertion(+)

diff --git a/common/Kconfig b/common/Kconfig
index 60237d305..c2ec995f1 100644
--- a/common/Kconfig
+++ b/common/Kconfig
@@ -166,6 +166,7 @@ config GLOBALVAR
 	bool "global environment variables support"
 	default y if !SHELL_NONE
 	select FNMATCH
+	select PARAMETER
 	help
 	  Global environment variables begin with "global.". Unlike normal
 	  shell variables they have the same values in all contexts. Global
-- 
Christian Eggers
Embedded software developer

Arnold & Richter Cine Technik GmbH & Co. Betriebs KG
Sitz: Muenchen - Registergericht: Amtsgericht Muenchen - Handelsregisternummer: HRA 57918
Persoenlich haftender Gesellschafter: Arnold & Richter Cine Technik GmbH
Sitz: Muenchen - Registergericht: Amtsgericht Muenchen - Handelsregisternummer: HRB 54477
Geschaeftsfuehrer: Dr. Michael Neuhaeuser; Stephan Schenk; Walter Trauninger; Markus Zeiler


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 1/3] watchdog: Select CONFIG_PARAMETER

[Sascha Hauer]

Hi Christian,

On Tue, Jan 21, 2020 at 12:44:19PM +0100, Christian Eggers wrote:
> Without CONFIG_PARAMETER, watchdog_register() will always fail.
> 
> Signed-off-by: Christian Eggers <ceggers@arri.de>
> ---
>  drivers/watchdog/Kconfig | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> index 45dd41a2a..34b7fea39 100644
> --- a/drivers/watchdog/Kconfig
> +++ b/drivers/watchdog/Kconfig
> @@ -4,6 +4,7 @@ config WATCHDOG_IMX_RESET_SOURCE
>  
>  menuconfig WATCHDOG
>  	bool "Watchdog support"
> +	select PARAMETER

I think this goes into the wrong direction. With CONFIG_PARAMETER
enabled we get support for adjusting device parameters from the shell.
In environments without shell support parameter support is not needed.
For example the watchdog C API doesn't need parameter support and is
still usable.

The static inline wrappers for dev_add_param_* should return NULL
instead of returning ERR_PTR(-ENOSYS).

Sascha

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 1/3] watchdog: Select CONFIG_PARAMETER

[Christian Eggers]

Hi Sascha,

Am Mittwoch, 22. Januar 2020, 09:21:15 CET schrieb Sascha Hauer:
> Hi Christian,
> 
> > diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> > index 45dd41a2a..34b7fea39 100644
> > --- a/drivers/watchdog/Kconfig
> > +++ b/drivers/watchdog/Kconfig
> > @@ -4,6 +4,7 @@ config WATCHDOG_IMX_RESET_SOURCE
> > 
> >  menuconfig WATCHDOG
> >  
> >  	bool "Watchdog support"
> > 
> > +	select PARAMETER
> 
> I think this goes into the wrong direction. With CONFIG_PARAMETER
> enabled we get support for adjusting device parameters from the shell.
> In environments without shell support parameter support is not needed.
> For example the watchdog C API doesn't need parameter support and is
> still usable.
> 
> The static inline wrappers for dev_add_param_* should return NULL
> instead of returning ERR_PTR(-ENOSYS).

initially I came to the same result. But previous commits to param.h went in 
the opposite direction:

> 03b59bdb64 ("paramter: The dev_add_param_*() return ERR_PTR(), change
> no-ops") to return ERR_PTR(-ENOSYS) instead of NULL
> 
> Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>

and

> c5d95eb4c7 ("param: make parameter functions more consistent")
>
> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>

Most of the callers of dev_add_param*()  don't care about the returned param 
pointer at all. Some are checking against PTR_ERR() which would not be hit if 
returning NULL (this is what we want).

A few callers have to changed if a NULL pointer can be returned:
- __nvvar_add()
- state_string_create()  stores the result in state_string::param, but seems 
to be used nowhere
- mci_register() dito for mci::param_probe
- state_uint8_create() dito for state_uint32::param
- state_uint32_create() dito

For me it looks reasonable to return a NULL pointer if CONFIG_PARAMETER is not 
set (as you suggested). Only __nvvar_add() needs slight changes and I would 
remove needless storage of param in structs state_string, mci and 
state_uint32.

Shall I start?

regards
Christian



_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 1/3] watchdog: Select CONFIG_PARAMETER

[Sascha Hauer]

On Wed, Jan 22, 2020 at 10:39:07AM +0100, Christian Eggers wrote:
> Hi Sascha,
> 
> Am Mittwoch, 22. Januar 2020, 09:21:15 CET schrieb Sascha Hauer:
> > Hi Christian,
> >
> > > diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> > > index 45dd41a2a..34b7fea39 100644
> > > --- a/drivers/watchdog/Kconfig
> > > +++ b/drivers/watchdog/Kconfig
> > > @@ -4,6 +4,7 @@ config WATCHDOG_IMX_RESET_SOURCE
> > >
> > >  menuconfig WATCHDOG
> > >
> > >     bool "Watchdog support"
> > >
> > > +   select PARAMETER
> >
> > I think this goes into the wrong direction. With CONFIG_PARAMETER
> > enabled we get support for adjusting device parameters from the shell.
> > In environments without shell support parameter support is not needed.
> > For example the watchdog C API doesn't need parameter support and is
> > still usable.
> >
> > The static inline wrappers for dev_add_param_* should return NULL
> > instead of returning ERR_PTR(-ENOSYS).
> 
> initially I came to the same result. But previous commits to param.h went in
> the opposite direction:
> 
> > 03b59bdb64 ("paramter: The dev_add_param_*() return ERR_PTR(), change
> > no-ops") to return ERR_PTR(-ENOSYS) instead of NULL

Shouldn't have merged this one as it lacks an explanation why this has
been done. Marc, do you have an idea what the motivation for this patch
was?

> >
> > Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
> > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> 
> and
> 
> > c5d95eb4c7 ("param: make parameter functions more consistent")
> >
> > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> 
> Most of the callers of dev_add_param*()  don't care about the returned param
> pointer at all. Some are checking against PTR_ERR() which would not be hit if
> returning NULL (this is what we want).
> 
> A few callers have to changed if a NULL pointer can be returned:
> - __nvvar_add()

nvvar doesn't really make sense without CONFIG_PARAMETER enabled. There
currently is no dependency between these options, but probably there
should be.

> - state_string_create()  stores the result in state_string::param, but seems
> to be used nowhere
> - mci_register() dito for mci::param_probe
> - state_uint8_create() dito for state_uint32::param
> - state_uint32_create() dito
> 
> For me it looks reasonable to return a NULL pointer if CONFIG_PARAMETER is not
> set (as you suggested). Only __nvvar_add() needs slight changes and I would
> remove needless storage of param in structs state_string, mci and
> state_uint32.
> 
> Shall I start?

Let's wait for Marc if he has an idea why we introduced 03b59bdb64, but
otherwise yes, this sounds like a good plan.


Sascha

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 1/3] watchdog: Select CONFIG_PARAMETER

[Marc Kleine-Budde]

[-- Attachment #1.1.1: Type: text/plain, Size: 1731 bytes --]

On 1/22/20 8:54 PM, Sascha Hauer wrote:
> On Wed, Jan 22, 2020 at 10:39:07AM +0100, Christian Eggers wrote:
>> Hi Sascha,
>>
>> Am Mittwoch, 22. Januar 2020, 09:21:15 CET schrieb Sascha Hauer:
>>> Hi Christian,
>>>
>>>> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
>>>> index 45dd41a2a..34b7fea39 100644
>>>> --- a/drivers/watchdog/Kconfig
>>>> +++ b/drivers/watchdog/Kconfig
>>>> @@ -4,6 +4,7 @@ config WATCHDOG_IMX_RESET_SOURCE
>>>>
>>>>  menuconfig WATCHDOG
>>>>
>>>>     bool "Watchdog support"
>>>>
>>>> +   select PARAMETER
>>>
>>> I think this goes into the wrong direction. With CONFIG_PARAMETER
>>> enabled we get support for adjusting device parameters from the shell.
>>> In environments without shell support parameter support is not needed.
>>> For example the watchdog C API doesn't need parameter support and is
>>> still usable.
>>>
>>> The static inline wrappers for dev_add_param_* should return NULL
>>> instead of returning ERR_PTR(-ENOSYS).
>>
>> initially I came to the same result. But previous commits to param.h went in
>> the opposite direction:
>>
>>> 03b59bdb64 ("paramter: The dev_add_param_*() return ERR_PTR(), change
>>> no-ops") to return ERR_PTR(-ENOSYS) instead of NULL
> 
> Shouldn't have merged this one as it lacks an explanation why this has
> been done. Marc, do you have an idea what the motivation for this patch
> was?

Sorry, I don't remember....

Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde           |
Embedded Linux                   | https://www.pengutronix.de  |
Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 149 bytes --]

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox