Why do we need reset_control_get_optional() ?

Why do we need reset_control_get_optional() ?

[Masahiro Yamada]

Hi.


Now the reset subsystem provides
a bunch of reset_control_get variants.

I am still wondering why we need to have _optional ones.

As far as I see, the difference is WARN_ON(1)
when CONFIG_RESET_CONTROLLER is not defined.



[1] When the reset is mandatory,
the code of the reset consumer is probably like follows:

  rst = devm_reset_control_get(dev, NULL);
  if (IS_ERR(rst)) {
          dev_err(dev, "failed to get reset\n");
          return PTR_ERR(rst);
  }

  ret = reset_control_deassert(rst);
  if (ret) {
          dev_err(dev, "failed to deassert reset\n");
          return ret;
  }

   ...



[2] When the reset is optional,
  the code should be something like follows:

   rst = devm_reset_control_get(dev, NULL);
   if (ERR_PTR(rst) == -EPROBE_DEFER)
           return -EPROBE_DEFER;

   /* deassert reset if it is available */
   if (!IS_ERR(rst)) {
           ret = reset_control_deassert(rst);
           if (ret) {
                  dev_err(dev, "failed to deassert reset\n");
                  return ret;
           }
    }




What I mean is, we can write a driver in either way
without using the _optional one.

No need to call WARN_ON(1).


What does _optional buy us?



One more thing.
WARN_ON(1) is only useful on run-time,
but run-time test is more expensive than compile-time test.

If a driver really needs reset control,
it should not be complied without CONFIG_RESET_CONTROLLER.
So, the driver should have "depends on RESET_CONTROLLER" in Kconfig.



I dug the git-log to figure out historical reason.

The _optional functions were introduced by the following commit:

----------------->8-----------------
commit b424080a9e086e683ad5fdc624a7cf3c024e0c0f
Author: Philipp Zabel <p.zabel@pengutronix.de>
Date:   Fri Mar 7 15:18:47 2014 +0100

    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>
------------------8<-----------------------------

At that point of time, the reset_control_get  (without _optional)
did not have a stub, so drivers calling reset_control_get()
could not be built without CONFIG_RESET_CONTROLLER enabled.
So, it made sense to add _optional variants.




A while later, any drivers became able to be built
without CONFIG_RESET_CONTROLLER, by the following commit.

----------------->8------------------------------------
commit 5bcd0b7f3c56c616abffd89e11c841834dd1528c
Author: Axel Lin <axel.lin@ingics.com>
Date:   Tue Sep 1 07:56:38 2015 +0800

    reset: Add (devm_)reset_control_get stub functions

    So the drivers can be compiled with CONFIG_RESET_CONTROLLER disabled.

    Signed-off-by: Axel Lin <axel.lin@ingics.com>
    Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
-------------------8<------------------------------------


Since then, it became pointless to have _optional variants.




I want to deprecate _optional variants in the following steps:

[1] Add "depends on RESET_CONTROLLER" to drivers
    for which reset_control is mandatory.

    We can find those driver easily by grepping
    the reference to non-optional reset_control_get().


[2] Change all of _optional calls to non-optional ones.


[3] Remove _optional static inline functions from include/linux/reset.h



It will take some releases to complete this task,
but I am happy to volunteer to it if we agree on this idea.




-- 
Best Regards
Masahiro Yamada

Why do we need reset_control_get_optional() ?

[Philipp Zabel]

Am Samstag, den 23.07.2016, 20:22 +0900 schrieb Masahiro Yamada:
> Hi.
> 
> 
> Now the reset subsystem provides
> a bunch of reset_control_get variants.
> 
> I am still wondering why we need to have _optional ones.
> 
> As far as I see, the difference is WARN_ON(1)
> when CONFIG_RESET_CONTROLLER is not defined.
> 
> 
> 
> [1] When the reset is mandatory,
> the code of the reset consumer is probably like follows:
> 
>   rst = devm_reset_control_get(dev, NULL);
>   if (IS_ERR(rst)) {
>           dev_err(dev, "failed to get reset\n");
>           return PTR_ERR(rst);
>   }
> 
>   ret = reset_control_deassert(rst);
>   if (ret) {
>           dev_err(dev, "failed to deassert reset\n");
>           return ret;
>   }
> 
>    ...
> 
> 
> 
> [2] When the reset is optional,
>   the code should be something like follows:
> 
>    rst = devm_reset_control_get(dev, NULL);
>    if (ERR_PTR(rst) == -EPROBE_DEFER)
>            return -EPROBE_DEFER;
> 
>    /* deassert reset if it is available */
>    if (!IS_ERR(rst)) {
>            ret = reset_control_deassert(rst);
>            if (ret) {
>                   dev_err(dev, "failed to deassert reset\n");
>                   return ret;
>            }
>     }
> 
> 
> 
> 
> What I mean is, we can write a driver in either way
> without using the _optional one.
> 
> No need to call WARN_ON(1).
> 
> 
> What does _optional buy us?

It will complain loudly with a backtrace if a driver requests a
non-optional reset on a kernel/platform with the reset framework
disabled.

> One more thing.
> WARN_ON(1) is only useful on run-time,
> but run-time test is more expensive than compile-time test.
> 
> If a driver really needs reset control,
> it should not be complied without CONFIG_RESET_CONTROLLER.
> So, the driver should have "depends on RESET_CONTROLLER" in Kconfig.

If we do that, we can't compile test those drivers anymore in
configurations without RESET_CONTROLLER enabled.

[...]
> I want to deprecate _optional variants in the following steps:
> 
> [1] Add "depends on RESET_CONTROLLER" to drivers
>     for which reset_control is mandatory.
> 
>     We can find those driver easily by grepping
>     the reference to non-optional reset_control_get().

Since we have the stubs, the RESET_CONTROLLER dependency is only at
runtime, not at build time.

I think Arnd wanted to move this in the opposite direction and remove
the configurable RESET_CONTROLLER symbol. Maybe we should let all
drivers that currently request non-optional resets have:
	depends on (ARCH_HAS_)RESET_CONTROLLER || COMPILE_TEST
?

regards
Philipp

Why do we need reset_control_get_optional() ?

[Arnd Bergmann]

On Thursday, July 28, 2016 11:43:00 AM CEST Philipp Zabel wrote:
> > I want to deprecate _optional variants in the following steps:
> > 
> > [1] Add "depends on RESET_CONTROLLER" to drivers
> >     for which reset_control is mandatory.
> > 
> >     We can find those driver easily by grepping
> >     the reference to non-optional reset_control_get().
> 
> Since we have the stubs, the RESET_CONTROLLER dependency is only at
> runtime, not at build time.
> 
> I think Arnd wanted to move this in the opposite direction and remove
> the configurable RESET_CONTROLLER symbol. Maybe we should let all
> drivers that currently request non-optional resets have:
>         depends on (ARCH_HAS_)RESET_CONTROLLER || COMPILE_TEST
> ?

There are various ways to improve the current situation.

I think it's important that a driver that has an optional
reset line behaves in exactly the same way whether the reset
subsystem is enabled or disabled when no reset line is
provided for a machine.

When a driver requires a reset line, we can either have a
build-time failure when the reset subsystem is disabled
(enforcing the Kconfig dependency), or cause a runtime
failure if either there is no reset line or the subsystem
is disabled.

In my experimental patch, I make the _optional functions
return NULL if no "resets" property is provided but return
an error if there are reset lines but the subsystem is
disabled, i.e. an optional reset must be used if it's in the
DT, but can be ignored otherwise.

	Arnd

Why do we need reset_control_get_optional() ?

[Masahiro Yamada]

Hi Philipp,


2016-07-28 18:43 GMT+09:00 Philipp Zabel <p.zabel@pengutronix.de>:
> Am Samstag, den 23.07.2016, 20:22 +0900 schrieb Masahiro Yamada:
>> Hi.
>>
>>
>> Now the reset subsystem provides
>> a bunch of reset_control_get variants.
>>
>> I am still wondering why we need to have _optional ones.
>>
>> As far as I see, the difference is WARN_ON(1)
>> when CONFIG_RESET_CONTROLLER is not defined.
>>
>>
>>
>> [1] When the reset is mandatory,
>> the code of the reset consumer is probably like follows:
>>
>>   rst = devm_reset_control_get(dev, NULL);
>>   if (IS_ERR(rst)) {
>>           dev_err(dev, "failed to get reset\n");
>>           return PTR_ERR(rst);
>>   }
>>
>>   ret = reset_control_deassert(rst);
>>   if (ret) {
>>           dev_err(dev, "failed to deassert reset\n");
>>           return ret;
>>   }
>>
>>    ...
>>
>>
>>
>> [2] When the reset is optional,
>>   the code should be something like follows:
>>
>>    rst = devm_reset_control_get(dev, NULL);
>>    if (ERR_PTR(rst) == -EPROBE_DEFER)
>>            return -EPROBE_DEFER;
>>
>>    /* deassert reset if it is available */
>>    if (!IS_ERR(rst)) {
>>            ret = reset_control_deassert(rst);
>>            if (ret) {
>>                   dev_err(dev, "failed to deassert reset\n");
>>                   return ret;
>>            }
>>     }
>>
>>
>>
>>
>> What I mean is, we can write a driver in either way
>> without using the _optional one.
>>
>> No need to call WARN_ON(1).
>>
>>
>> What does _optional buy us?
>
> It will complain loudly with a backtrace if a driver requests a
> non-optional reset on a kernel/platform with the reset framework
> disabled.

Right, but this situation will be solved with my suggestion.


>> One more thing.
>> WARN_ON(1) is only useful on run-time,
>> but run-time test is more expensive than compile-time test.
>>
>> If a driver really needs reset control,
>> it should not be complied without CONFIG_RESET_CONTROLLER.
>> So, the driver should have "depends on RESET_CONTROLLER" in Kconfig.
>
> If we do that, we can't compile test those drivers anymore in
> configurations without RESET_CONTROLLER enabled.
> [...]
>> I want to deprecate _optional variants in the following steps:
>>
>> [1] Add "depends on RESET_CONTROLLER" to drivers
>>     for which reset_control is mandatory.
>>
>>     We can find those driver easily by grepping
>>     the reference to non-optional reset_control_get().
>
> Since we have the stubs, the RESET_CONTROLLER dependency is only at
> runtime, not at build time.
>
> I think Arnd wanted to move this in the opposite direction and remove
> the configurable RESET_CONTROLLER symbol. Maybe we should let all
> drivers that currently request non-optional resets have:
>         depends on (ARCH_HAS_)RESET_CONTROLLER || COMPILE_TEST
> ?

No, I do not think we need to do that.

We should do
          depends on ARCH_<SOC_NAME> || COMPILE_TEST
because SOC_NAME is not a real dependency for the driver.


but,

      depends on RESET_CONTROLLER

is a genuine dependency, so it should not be OR'ed with COMPILE_TEST.




Currently, ARCH_HAS_RESET_CONTROLLER is only used to decide the default value
of RESET_CONTROLLER:


menuconfig RESET_CONTROLLER
        bool "Reset Controller Support"
        default y if ARCH_HAS_RESET_CONTROLLER
        help
          Generic Reset Controller support.



So, RESET_CONTROLLER can be enabled without any dependency,
i.e., COMPILE_TEST will be fine.



However, I think the following makes more sense:


menuconfig RESET_CONTROLLER
        bool "Reset Controller Support"
        depends on (ARCH_HAS_RESET_CONTROLLER || COMPILE_TEST)
        default y
        help
          Generic Reset Controller support.





-- 
Best Regards
Masahiro Yamada

Why do we need reset_control_get_optional() ?

[Masahiro Yamada]

Hi Arnd,


2016-07-28 19:09 GMT+09:00 Arnd Bergmann <arnd@arndb.de>:
> On Thursday, July 28, 2016 11:43:00 AM CEST Philipp Zabel wrote:
>> > I want to deprecate _optional variants in the following steps:
>> >
>> > [1] Add "depends on RESET_CONTROLLER" to drivers
>> >     for which reset_control is mandatory.
>> >
>> >     We can find those driver easily by grepping
>> >     the reference to non-optional reset_control_get().
>>
>> Since we have the stubs, the RESET_CONTROLLER dependency is only at
>> runtime, not at build time.
>>
>> I think Arnd wanted to move this in the opposite direction and remove
>> the configurable RESET_CONTROLLER symbol. Maybe we should let all
>> drivers that currently request non-optional resets have:
>>         depends on (ARCH_HAS_)RESET_CONTROLLER || COMPILE_TEST
>> ?
>
> There are various ways to improve the current situation.
>
> I think it's important that a driver that has an optional
> reset line behaves in exactly the same way whether the reset
> subsystem is enabled or disabled when no reset line is
> provided for a machine.
>
> When a driver requires a reset line, we can either have a
> build-time failure when the reset subsystem is disabled
> (enforcing the Kconfig dependency), or cause a runtime
> failure if either there is no reset line or the subsystem
> is disabled.

Yes.  I am suggesting the "enforcing the Kconfig dependency".

"I will let you build this driver, but it would never work"
is not the right thing to do, I think.



> In my experimental patch, I make the _optional functions
> return NULL if no "resets" property is provided but return
> an error if there are reset lines but the subsystem is
> disabled, i.e. an optional reset must be used if it's in the
> DT, but can be ignored otherwise.

I do not like this idea.

reset_control_get() (or variants) should not return NULL, it is ambiguous.
It should return ERR_PTR(-ENOENT) if no "resets" property.

I only want two types for functions that return a pointer.

[1] return a valid pointer on success, or return NULL on failure
    (for example, kmalloc())
[2] return a valid pointer on success, or return error pointer on failure
   (many of _register() functions)

Mixing [1] and [2] will be a mess.



-- 
Best Regards
Masahiro Yamada

Why do we need reset_control_get_optional() ?

[Philipp Zabel]

Am Donnerstag, den 28.07.2016, 12:09 +0200 schrieb Arnd Bergmann:
> On Thursday, July 28, 2016 11:43:00 AM CEST Philipp Zabel wrote:
> > > I want to deprecate _optional variants in the following steps:
> > > 
> > > [1] Add "depends on RESET_CONTROLLER" to drivers
> > >     for which reset_control is mandatory.
> > > 
> > >     We can find those driver easily by grepping
> > >     the reference to non-optional reset_control_get().
> > 
> > Since we have the stubs, the RESET_CONTROLLER dependency is only at
> > runtime, not at build time.
> > 
> > I think Arnd wanted to move this in the opposite direction and remove
> > the configurable RESET_CONTROLLER symbol. Maybe we should let all
> > drivers that currently request non-optional resets have:
> >         depends on (ARCH_HAS_)RESET_CONTROLLER || COMPILE_TEST
> > ?
> 
> There are various ways to improve the current situation.
> 
> I think it's important that a driver that has an optional
> reset line behaves in exactly the same way whether the reset
> subsystem is enabled or disabled when no reset line is
> provided for a machine.

Agreed.

> When a driver requires a reset line, we can either have a
> build-time failure when the reset subsystem is disabled
> (enforcing the Kconfig dependency),

That would have been the case before commit 5bcd0b7f3c56 ("reset: Add
(devm_)reset_control_get stub functions"). Now that we allow compiling
all drivers with the stubs, I suppose option B is the way to go:

> or cause a runtime failure if either there is no reset line or
> the subsystem is disabled.

> In my experimental patch, I make the _optional functions
> return NULL if no "resets" property is provided but return
> an error if there are reset lines but the subsystem is
> disabled, i.e. an optional reset must be used if it's in the
> DT, but can be ignored otherwise.

I have to admit I don't understand the purpose of that.
reset_controller_get_optional returns -ENOENT if there's no reset in the
DT, and all drivers act accordingly.

regards
Philipp

Why do we need reset_control_get_optional() ?

[Philipp Zabel]

Am Donnerstag, den 28.07.2016, 19:52 +0900 schrieb Masahiro Yamada:
> Hi Arnd,
> 
> 
> 2016-07-28 19:09 GMT+09:00 Arnd Bergmann <arnd@arndb.de>:
> > On Thursday, July 28, 2016 11:43:00 AM CEST Philipp Zabel wrote:
> >> > I want to deprecate _optional variants in the following steps:
> >> >
> >> > [1] Add "depends on RESET_CONTROLLER" to drivers
> >> >     for which reset_control is mandatory.
> >> >
> >> >     We can find those driver easily by grepping
> >> >     the reference to non-optional reset_control_get().
> >>
> >> Since we have the stubs, the RESET_CONTROLLER dependency is only at
> >> runtime, not at build time.
> >>
> >> I think Arnd wanted to move this in the opposite direction and remove
> >> the configurable RESET_CONTROLLER symbol. Maybe we should let all
> >> drivers that currently request non-optional resets have:
> >>         depends on (ARCH_HAS_)RESET_CONTROLLER || COMPILE_TEST
> >> ?
> >
> > There are various ways to improve the current situation.
> >
> > I think it's important that a driver that has an optional
> > reset line behaves in exactly the same way whether the reset
> > subsystem is enabled or disabled when no reset line is
> > provided for a machine.
> >
> > When a driver requires a reset line, we can either have a
> > build-time failure when the reset subsystem is disabled
> > (enforcing the Kconfig dependency), or cause a runtime
> > failure if either there is no reset line or the subsystem
> > is disabled.
> 
> Yes.  I am suggesting the "enforcing the Kconfig dependency".
> 
> "I will let you build this driver, but it would never work"
> is not the right thing to do, I think.

We'd loose randconfig build coverage though. I think allowing to build
unusable drivers in COMPILE_TEST scenarios is ok.

> > In my experimental patch, I make the _optional functions
> > return NULL if no "resets" property is provided but return
> > an error if there are reset lines but the subsystem is
> > disabled, i.e. an optional reset must be used if it's in the
> > DT, but can be ignored otherwise.
> 
> I do not like this idea.
> 
> reset_control_get() (or variants) should not return NULL, it is ambiguous.
> It should return ERR_PTR(-ENOENT) if no "resets" property.
> 
> I only want two types for functions that return a pointer.
> 
> [1] return a valid pointer on success, or return NULL on failure
>     (for example, kmalloc())
> [2] return a valid pointer on success, or return error pointer on failure
>    (many of _register() functions)
> 
> Mixing [1] and [2] will be a mess.

I too would prefer to keep that as-is. The reset_control_get_optional
stub could return -ENOENT if there is no resets device tree property.

regards
Philipp

Why do we need reset_control_get_optional() ?

[Philipp Zabel]

Hi Masahiro,

Am Donnerstag, den 28.07.2016, 19:29 +0900 schrieb Masahiro Yamada:
[...]
> However, I think the following makes more sense:
> 
> 
> menuconfig RESET_CONTROLLER
>         bool "Reset Controller Support"
>         depends on (ARCH_HAS_RESET_CONTROLLER || COMPILE_TEST)
>         default y
>         help
>           Generic Reset Controller support.

That looks sensible to me. You'll only have to enable the reset
controller framework if either some enabled architecture has a reset
controller (in which case you want the driver for it to be activated by
default), or if you want to compile test some of the reset drivers.

regards
Philipp

Why do we need reset_control_get_optional() ?

[Arnd Bergmann]

On Friday, July 29, 2016 3:08:15 PM CEST Philipp Zabel wrote:
> Hi Masahiro,
> 
> Am Donnerstag, den 28.07.2016, 19:29 +0900 schrieb Masahiro Yamada:
> [...]
> > However, I think the following makes more sense:
> > 
> > 
> > menuconfig RESET_CONTROLLER
> >         bool "Reset Controller Support"
> >         depends on (ARCH_HAS_RESET_CONTROLLER || COMPILE_TEST)
> >         default y
> >         help
> >           Generic Reset Controller support.
> 
> That looks sensible to me. You'll only have to enable the reset
> controller framework if either some enabled architecture has a reset
> controller (in which case you want the driver for it to be activated by
> default), or if you want to compile test some of the reset drivers.

This still doesn't let a platform 'select RESET_FOO', unless they
also select RESET_CONTROLLER and ARCH_HAS_RESET_CONTROLLER.

Why do we need to guard all drivers inside of two symbols?

	Arnd

Why do we need reset_control_get_optional() ?

[Philipp Zabel]

Am Samstag, den 30.07.2016, 22:13 +0200 schrieb Arnd Bergmann:
> On Friday, July 29, 2016 3:08:15 PM CEST Philipp Zabel wrote:
> > Hi Masahiro,
> > 
> > Am Donnerstag, den 28.07.2016, 19:29 +0900 schrieb Masahiro Yamada:
> > [...]
> > > However, I think the following makes more sense:
> > > 
> > > 
> > > menuconfig RESET_CONTROLLER
> > >         bool "Reset Controller Support"
> > >         depends on (ARCH_HAS_RESET_CONTROLLER || COMPILE_TEST)
> > >         default y
> > >         help
> > >           Generic Reset Controller support.
> > 
> > That looks sensible to me. You'll only have to enable the reset
> > controller framework if either some enabled architecture has a reset
> > controller (in which case you want the driver for it to be activated by
> > default), or if you want to compile test some of the reset drivers.
> 
> This still doesn't let a platform 'select RESET_FOO', unless they
> also select RESET_CONTROLLER and ARCH_HAS_RESET_CONTROLLER.
> 
> Why do we need to guard all drivers inside of two symbols?

Does the platform have to select RESET_FOO at all? Wouldn't it be enough
for RESET_FOO to have "default ARCH_FOO" ?

Currently ARCH_HAS_RESET_CONTROLLER is used to default y the
RESET_CONTROLLER symbol. Maybe we should add another
ARCH_REQUIRE_RESET_CONTROLLER and have that select RESET_CONTROLLER,
similarly to how it is done for GPIOLIB?

config ARCH_HAS_RESET_CONTROLLER
	bool
	help
	  Selecting this option from the architecture Kconfig enables
	  the RESET_CONTROLLER framework by default but does not select
	  it. Use it for architectures that still work without reset
	  controller support and thus allow the user to disable it.

config ARCH_REQUIRE_RESET_CONTROLLER
	bool
	select RESET_CONTROLLER
	help
	  Selecting this option from the architecture Kconfig selects
	  the RESET_CONTROLLER framework. Use it for architectures that
	  should not be built without the reset controller framework
	  enabled.

menuconfig RESET_CONTROLLER
        bool "Reset Controller Support"
        default ARCH_HAS_RESET_CONTROLLER || COMPILE_TEST
        help
          Generic Reset Controller support.

          This framework is designed to abstract reset handling of devices
          via GPIOs or SoC-internal reset controller modules.

          If unsure, say no.

The platforms could then select one of the ARCH_*_RESET_CONTROLLER
symbols and nobody would have to select RESET_CONTROLLER directly, for
example:

menuconfig ARCH_TEGRA                                                                                                                         
        bool "NVIDIA Tegra"
        depends on ARCH_MULTI_V7
        select ARCH_REQUIRE_GPIOLIB
        select ARCH_REQUIRE_RESET_CONTROLLER
        select ARCH_SUPPORTS_TRUSTED_FOUNDATIONS
        select ARM_AMBA
        select ARM_GIC
        select CLKSRC_MMIO
        select HAVE_ARM_SCU if SMP
        select HAVE_ARM_TWD if SMP
        select PINCTRL
        select PM_OPP
        select SOC_BUS
        help
          This enables support for NVIDIA Tegra based systems.

regards
Philipp

Why do we need reset_control_get_optional() ?

[Arnd Bergmann]

On Friday, August 5, 2016 10:55:58 AM CEST Philipp Zabel wrote:
> Am Samstag, den 30.07.2016, 22:13 +0200 schrieb Arnd Bergmann:
> > On Friday, July 29, 2016 3:08:15 PM CEST Philipp Zabel wrote:
> > > Hi Masahiro,
> > > 
> > > Am Donnerstag, den 28.07.2016, 19:29 +0900 schrieb Masahiro Yamada:
> > > [...]
> > > > However, I think the following makes more sense:
> > > > 
> > > > 
> > > > menuconfig RESET_CONTROLLER
> > > >         bool "Reset Controller Support"
> > > >         depends on (ARCH_HAS_RESET_CONTROLLER || COMPILE_TEST)
> > > >         default y
> > > >         help
> > > >           Generic Reset Controller support.
> > > 
> > > That looks sensible to me. You'll only have to enable the reset
> > > controller framework if either some enabled architecture has a reset
> > > controller (in which case you want the driver for it to be activated by
> > > default), or if you want to compile test some of the reset drivers.
> > 
> > This still doesn't let a platform 'select RESET_FOO', unless they
> > also select RESET_CONTROLLER and ARCH_HAS_RESET_CONTROLLER.
> > 
> > Why do we need to guard all drivers inside of two symbols?
> 
> Does the platform have to select RESET_FOO at all? Wouldn't it be enough
> for RESET_FOO to have "default ARCH_FOO" ?

It depends on what you want to achieve. With a user-visible option
and "default ARCH_FOO", you can disable the driver manually, and
another driver that has "depends on ARCH_FOO" can not rely on this
one being present as it currently can.

If we do this as

config RESET_FOO
	bool "FOO reset controller" if COMPILE_TEST && !ARCH_FOO
	default ARCH_FOO

then I think we get both: you won't be able to turn it off
but also get the build testing.

> Currently ARCH_HAS_RESET_CONTROLLER is used to default y the
> RESET_CONTROLLER symbol. Maybe we should add another
> ARCH_REQUIRE_RESET_CONTROLLER and have that select RESET_CONTROLLER,
> similarly to how it is done for GPIOLIB?

GPIOLIB just stopped using it, there is now only CONFIG_GPIOLIB
that can get selected by platforms that need it.

> config ARCH_HAS_RESET_CONTROLLER
> 	bool
> 	help
> 	  Selecting this option from the architecture Kconfig enables
> 	  the RESET_CONTROLLER framework by default but does not select
> 	  it. Use it for architectures that still work without reset
> 	  controller support and thus allow the user to disable it.
> 
> config ARCH_REQUIRE_RESET_CONTROLLER
> 	bool
> 	select RESET_CONTROLLER
> 	help
> 	  Selecting this option from the architecture Kconfig selects
> 	  the RESET_CONTROLLER framework. Use it for architectures that
> 	  should not be built without the reset controller framework
> 	  enabled.
> 
> menuconfig RESET_CONTROLLER
>         bool "Reset Controller Support"
>         default ARCH_HAS_RESET_CONTROLLER || COMPILE_TEST
>         help
>           Generic Reset Controller support.
> 
>           This framework is designed to abstract reset handling of devices
>           via GPIOs or SoC-internal reset controller modules.
> 
>           If unsure, say no.
> 
> The platforms could then select one of the ARCH_*_RESET_CONTROLLER
> symbols and nobody would have to select RESET_CONTROLLER directly, for
> example:
> 
> menuconfig ARCH_TEGRA                                                                                                                         
>         bool "NVIDIA Tegra"
>         depends on ARCH_MULTI_V7
>         select ARCH_REQUIRE_GPIOLIB
>         select ARCH_REQUIRE_RESET_CONTROLLER
>         select ARCH_SUPPORTS_TRUSTED_FOUNDATIONS

I never really like the way it was done for gpiolib. I think the
easiest way would be to have a menu for the reset controllers that
does not have any dependencies whatsoever, and make the individual
reset drivers select CONFIG_RESET_CONTROLLER, which then becomes
a hidden symbol that enables the core code.

	Arnd

Why do we need reset_control_get_optional() ?

[Arnd Bergmann]

On Thursday, July 28, 2016 1:00:49 PM CEST Philipp Zabel wrote:
> Am Donnerstag, den 28.07.2016, 19:52 +0900 schrieb Masahiro Yamada:

> > > In my experimental patch, I make the _optional functions
> > > return NULL if no "resets" property is provided but return
> > > an error if there are reset lines but the subsystem is
> > > disabled, i.e. an optional reset must be used if it's in the
> > > DT, but can be ignored otherwise.
> > 
> > I do not like this idea.
> > 
> > reset_control_get() (or variants) should not return NULL, it is ambiguous.
> > It should return ERR_PTR(-ENOENT) if no "resets" property.
> > 
> > I only want two types for functions that return a pointer.
> > 
> > [1] return a valid pointer on success, or return NULL on failure
> >     (for example, kmalloc())
> > [2] return a valid pointer on success, or return error pointer on failure
> >    (many of _register() functions)
> > 
> > Mixing [1] and [2] will be a mess.

Ah, right. I was thinking only of the case where the reset subsystem
is completely disabled here, so returning NULL could be considered
a valid return code that can in turn be passed into the other
functions.

However, I agree that returning NULL as a valid result from
..._get_optional() would be bad style, so let's drop my idea
there.

> I too would prefer to keep that as-is. The reset_control_get_optional
> stub could return -ENOENT if there is no resets device tree property.

Now I'm also confused about what we really need
reset_control_get_optional() for, and which error codes the callers
are supposed to check.

This is the matrix I think you mean for _get_optional:

CONFIG_RESET_CONTROLLER=y, dt entry present: valid pointer (or other error)
CONFIG_RESET_CONTROLLER=n, dt entry present: -EOPNOTSUPP
CONFIG_RESET_CONTROLLER=y, dt entry missing: -ENOENT
CONFIG_RESET_CONTROLLER=n, dt entry missing: -ENOENT

Is this what you had in mind? If so, what is the value of the
added runtime warning for reset_control_get? Any caller of that
function would already check for errors, the only difference
I see is that callers of _optional can ignore -ENOENT.

	Arnd

Why do we need reset_control_get_optional() ?

[Philipp Zabel]

Am Freitag, den 05.08.2016, 17:50 +0200 schrieb Arnd Bergmann:
> On Thursday, July 28, 2016 1:00:49 PM CEST Philipp Zabel wrote:
> > Am Donnerstag, den 28.07.2016, 19:52 +0900 schrieb Masahiro Yamada:
> 
> > > > In my experimental patch, I make the _optional functions
> > > > return NULL if no "resets" property is provided but return
> > > > an error if there are reset lines but the subsystem is
> > > > disabled, i.e. an optional reset must be used if it's in the
> > > > DT, but can be ignored otherwise.
> > > 
> > > I do not like this idea.
> > > 
> > > reset_control_get() (or variants) should not return NULL, it is ambiguous.
> > > It should return ERR_PTR(-ENOENT) if no "resets" property.
> > > 
> > > I only want two types for functions that return a pointer.
> > > 
> > > [1] return a valid pointer on success, or return NULL on failure
> > >     (for example, kmalloc())
> > > [2] return a valid pointer on success, or return error pointer on failure
> > >    (many of _register() functions)
> > > 
> > > Mixing [1] and [2] will be a mess.
> 
> Ah, right. I was thinking only of the case where the reset subsystem
> is completely disabled here, so returning NULL could be considered
> a valid return code that can in turn be passed into the other
> functions.
> 
> However, I agree that returning NULL as a valid result from
> ..._get_optional() would be bad style, so let's drop my idea
> there.
> 
> > I too would prefer to keep that as-is. The reset_control_get_optional
> > stub could return -ENOENT if there is no resets device tree property.
> 
> Now I'm also confused about what we really need
> reset_control_get_optional() for, and which error codes the callers
> are supposed to check.
> 
> This is the matrix I think you mean for _get_optional:
> 
[...]
> CONFIG_RESET_CONTROLLER=n, dt entry present: -EOPNOTSUPP
> CONFIG_RESET_CONTROLLER=n, dt entry missing: -ENOENT

^^ I didn't consider this distiction.

> Is this what you had in mind? If so, what is the value of the
> added runtime warning for reset_control_get? Any caller of that
> function would already check for errors, the only difference
> I see is that callers of _optional can ignore -ENOENT.

My initial motivation was to make it as hard as possible to misconfigure
the kernel, which is why I initially didn't want stubs for the
non-optional variant. Of course that would cause build failures and/or
reduced compile test coverage, so we added the stubs and the warning to
make it obvious when a misconfigured kernel is running: on a kernel with
RESET_CONTROLLER=n drivers that use reset_control_get are expected to
build, but they are not expected to work. I suppose the same is the case
for _optional, if the dt entry is present, so maybe we should drop
reset_control_get_optional and add always a warning in case of
-EOPNOTSUPP.
I don't want all drivers to have to differentiate between -EOPNOTSUPP
and -ENOENT error codes, only current reset_control_get_optional users
have to do that.

regards
Philipp

Why do we need reset_control_get_optional() ?

[Philipp Zabel]

Am Freitag, den 05.08.2016, 17:35 +0200 schrieb Arnd Bergmann:
> On Friday, August 5, 2016 10:55:58 AM CEST Philipp Zabel wrote:
> > Am Samstag, den 30.07.2016, 22:13 +0200 schrieb Arnd Bergmann:
> > > On Friday, July 29, 2016 3:08:15 PM CEST Philipp Zabel wrote:
> > > > Hi Masahiro,
> > > > 
> > > > Am Donnerstag, den 28.07.2016, 19:29 +0900 schrieb Masahiro Yamada:
> > > > [...]
> > > > > However, I think the following makes more sense:
> > > > > 
> > > > > 
> > > > > menuconfig RESET_CONTROLLER
> > > > >         bool "Reset Controller Support"
> > > > >         depends on (ARCH_HAS_RESET_CONTROLLER || COMPILE_TEST)
> > > > >         default y
> > > > >         help
> > > > >           Generic Reset Controller support.
> > > > 
> > > > That looks sensible to me. You'll only have to enable the reset
> > > > controller framework if either some enabled architecture has a reset
> > > > controller (in which case you want the driver for it to be activated by
> > > > default), or if you want to compile test some of the reset drivers.
> > > 
> > > This still doesn't let a platform 'select RESET_FOO', unless they
> > > also select RESET_CONTROLLER and ARCH_HAS_RESET_CONTROLLER.
> > > 
> > > Why do we need to guard all drivers inside of two symbols?
> > 
> > Does the platform have to select RESET_FOO at all? Wouldn't it be enough
> > for RESET_FOO to have "default ARCH_FOO" ?
> 
> It depends on what you want to achieve. With a user-visible option
> and "default ARCH_FOO", you can disable the driver manually, and
> another driver that has "depends on ARCH_FOO" can not rely on this
> one being present as it currently can.
>
> If we do this as
> 
> config RESET_FOO
> 	bool "FOO reset controller" if COMPILE_TEST && !ARCH_FOO
> 	default ARCH_FOO
> 
> then I think we get both: you won't be able to turn it off
> but also get the build testing.

I like it. Automatically enable the mandatory reset controller driver
with its architecture, otherwise don't ask unless COMPILE_TEST is
enabled. For drivers that can be reasonably compiled as a module, it
could be

config RESET_BAR
	tristate "BAR reset controller" if COMPILE_TEST || ARCH_FOO
	default ARCH_FOO

> > Currently ARCH_HAS_RESET_CONTROLLER is used to default y the
> > RESET_CONTROLLER symbol. Maybe we should add another
> > ARCH_REQUIRE_RESET_CONTROLLER and have that select RESET_CONTROLLER,
> > similarly to how it is done for GPIOLIB?
> 
> GPIOLIB just stopped using it, there is now only CONFIG_GPIOLIB
> that can get selected by platforms that need it.

Ok.

[...]
> I never really like the way it was done for gpiolib. I think the
> easiest way would be to have a menu for the reset controllers that
> does not have any dependencies whatsoever, and make the individual
> reset drivers select CONFIG_RESET_CONTROLLER, which then becomes
> a hidden symbol that enables the core code.

I thought making all the driver config symbols visible by default is
problematic, but with "bool ... if COMPILE_TEST" that is addressed
already.

regards
Philipp

Why do we need reset_control_get_optional() ?

[Arnd Bergmann]

On Monday, August 8, 2016 6:39:36 PM CEST Philipp Zabel wrote:
> > Now I'm also confused about what we really need
> > reset_control_get_optional() for, and which error codes the callers
> > are supposed to check.
> > 
> > This is the matrix I think you mean for _get_optional:
> > 
> [...]
> > CONFIG_RESET_CONTROLLER=n, dt entry present: -EOPNOTSUPP
> > CONFIG_RESET_CONTROLLER=n, dt entry missing: -ENOENT
> 
> ^^ I didn't consider this distiction.
> 
> > Is this what you had in mind? If so, what is the value of the
> > added runtime warning for reset_control_get? Any caller of that
> > function would already check for errors, the only difference
> > I see is that callers of _optional can ignore -ENOENT.
> 
> My initial motivation was to make it as hard as possible to misconfigure
> the kernel, which is why I initially didn't want stubs for the
> non-optional variant. Of course that would cause build failures and/or
> reduced compile test coverage, so we added the stubs and the warning to
> make it obvious when a misconfigured kernel is running: on a kernel with
> RESET_CONTROLLER=n drivers that use reset_control_get are expected to
> build, but they are not expected to work. I suppose the same is the case
> for _optional, if the dt entry is present, so maybe we should drop
> reset_control_get_optional and add always a warning in case of
> -EOPNOTSUPP.
> I don't want all drivers to have to differentiate between -EOPNOTSUPP
> and -ENOENT error codes, only current reset_control_get_optional users
> have to do that.

In almost all cases, I think drivers that require the reset line
wouldn't even check the failure code but just pass it down to
the caller (usually platform_device_probe()), so the really don't
need to care.

Some drivers might want to handle -EPROBE_DEFER (by not warning
about it before returning from probe), and -ENOENT can be handled
in a similar way (by continuing instead of failing).

	Arnd

Why do we need reset_control_get_optional() ?

[Masahiro Yamada]

Hi Arnd,



2016-08-06 0:35 GMT+09:00 Arnd Bergmann <arnd@arndb.de>:

>
> config RESET_FOO
>         bool "FOO reset controller" if COMPILE_TEST && !ARCH_FOO
>         default ARCH_FOO
>
> then I think we get both: you won't be able to turn it off
> but also get the build testing.


This looks good to me, too.
I do not know if we need "&& !ARCH_FOO", though.

When we are compile-testing, we do not care if it works on run-time,
so perhaps it makes sense to allow to enable/disable RESET_FOO
regardless ARCH_FOO.


Philipp also agreed, so I guess the permission for this rework
is granted.
Would you do it?



-- 
Best Regards
Masahiro Yamada

Why do we need reset_control_get_optional() ?

[Masahiro Yamada]

Hi Philipp, Arnd.



2016-08-09 1:39 GMT+09:00 Philipp Zabel <p.zabel@pengutronix.de>:
> Am Freitag, den 05.08.2016, 17:50 +0200 schrieb Arnd Bergmann:
>> On Thursday, July 28, 2016 1:00:49 PM CEST Philipp Zabel wrote:
>> > Am Donnerstag, den 28.07.2016, 19:52 +0900 schrieb Masahiro Yamada:
>>
>> > > > In my experimental patch, I make the _optional functions
>> > > > return NULL if no "resets" property is provided but return
>> > > > an error if there are reset lines but the subsystem is
>> > > > disabled, i.e. an optional reset must be used if it's in the
>> > > > DT, but can be ignored otherwise.
>> > >
>> > > I do not like this idea.
>> > >
>> > > reset_control_get() (or variants) should not return NULL, it is ambiguous.
>> > > It should return ERR_PTR(-ENOENT) if no "resets" property.
>> > >
>> > > I only want two types for functions that return a pointer.
>> > >
>> > > [1] return a valid pointer on success, or return NULL on failure
>> > >     (for example, kmalloc())
>> > > [2] return a valid pointer on success, or return error pointer on failure
>> > >    (many of _register() functions)
>> > >
>> > > Mixing [1] and [2] will be a mess.
>>
>> Ah, right. I was thinking only of the case where the reset subsystem
>> is completely disabled here, so returning NULL could be considered
>> a valid return code that can in turn be passed into the other
>> functions.
>>
>> However, I agree that returning NULL as a valid result from
>> ..._get_optional() would be bad style, so let's drop my idea
>> there.
>>
>> > I too would prefer to keep that as-is. The reset_control_get_optional
>> > stub could return -ENOENT if there is no resets device tree property.
>>
>> Now I'm also confused about what we really need
>> reset_control_get_optional() for, and which error codes the callers
>> are supposed to check.
>>
>> This is the matrix I think you mean for _get_optional:
>>
> [...]
>> CONFIG_RESET_CONTROLLER=n, dt entry present: -EOPNOTSUPP
>> CONFIG_RESET_CONTROLLER=n, dt entry missing: -ENOENT
>
> ^^ I didn't consider this distiction.
>
>> Is this what you had in mind? If so, what is the value of the
>> added runtime warning for reset_control_get? Any caller of that
>> function would already check for errors, the only difference
>> I see is that callers of _optional can ignore -ENOENT.
>
> My initial motivation was to make it as hard as possible to misconfigure
> the kernel, which is why I initially didn't want stubs for the
> non-optional variant. Of course that would cause build failures and/or
> reduced compile test coverage, so we added the stubs and the warning to
> make it obvious when a misconfigured kernel is running: on a kernel with
> RESET_CONTROLLER=n drivers that use reset_control_get are expected to
> build, but they are not expected to work. I suppose the same is the case
> for _optional, if the dt entry is present, so maybe we should drop
> reset_control_get_optional and add always a warning in case of
> -EOPNOTSUPP.
> I don't want all drivers to have to differentiate between -EOPNOTSUPP
> and -ENOENT error codes, only current reset_control_get_optional users
> have to do that.


I've posted a patch to drop reset_control_get_optional;
https://patchwork.kernel.org/patch/9284063/

Could you check if it works?

If we go this way, my patch
289363fd99a17d6249ee1373541f1da43cbb22c5
in your reset/next branch is completely useless.

As the commits in the reset-subsystem do not appear
even in linux-next until they are pulled into the ASOC tree,
how about dropping 289363fd and turning around?


-- 
Best Regards
Masahiro Yamada

Why do we need reset_control_get_optional() ?

[Masahiro Yamada]

Hi Philipp,

2016-08-16 23:36 GMT+09:00 Masahiro Yamada <yamada.masahiro@socionext.com>:
> Hi Philipp, Arnd.
>
>
>
> 2016-08-09 1:39 GMT+09:00 Philipp Zabel <p.zabel@pengutronix.de>:
>> Am Freitag, den 05.08.2016, 17:50 +0200 schrieb Arnd Bergmann:
>>> On Thursday, July 28, 2016 1:00:49 PM CEST Philipp Zabel wrote:
>>> > Am Donnerstag, den 28.07.2016, 19:52 +0900 schrieb Masahiro Yamada:
>>>
>>> > > > In my experimental patch, I make the _optional functions
>>> > > > return NULL if no "resets" property is provided but return
>>> > > > an error if there are reset lines but the subsystem is
>>> > > > disabled, i.e. an optional reset must be used if it's in the
>>> > > > DT, but can be ignored otherwise.
>>> > >
>>> > > I do not like this idea.
>>> > >
>>> > > reset_control_get() (or variants) should not return NULL, it is ambiguous.
>>> > > It should return ERR_PTR(-ENOENT) if no "resets" property.
>>> > >
>>> > > I only want two types for functions that return a pointer.
>>> > >
>>> > > [1] return a valid pointer on success, or return NULL on failure
>>> > >     (for example, kmalloc())
>>> > > [2] return a valid pointer on success, or return error pointer on failure
>>> > >    (many of _register() functions)
>>> > >
>>> > > Mixing [1] and [2] will be a mess.
>>>
>>> Ah, right. I was thinking only of the case where the reset subsystem
>>> is completely disabled here, so returning NULL could be considered
>>> a valid return code that can in turn be passed into the other
>>> functions.
>>>
>>> However, I agree that returning NULL as a valid result from
>>> ..._get_optional() would be bad style, so let's drop my idea
>>> there.
>>>
>>> > I too would prefer to keep that as-is. The reset_control_get_optional
>>> > stub could return -ENOENT if there is no resets device tree property.
>>>
>>> Now I'm also confused about what we really need
>>> reset_control_get_optional() for, and which error codes the callers
>>> are supposed to check.
>>>
>>> This is the matrix I think you mean for _get_optional:
>>>
>> [...]
>>> CONFIG_RESET_CONTROLLER=n, dt entry present: -EOPNOTSUPP
>>> CONFIG_RESET_CONTROLLER=n, dt entry missing: -ENOENT
>>
>> ^^ I didn't consider this distiction.
>>
>>> Is this what you had in mind? If so, what is the value of the
>>> added runtime warning for reset_control_get? Any caller of that
>>> function would already check for errors, the only difference
>>> I see is that callers of _optional can ignore -ENOENT.
>>
>> My initial motivation was to make it as hard as possible to misconfigure
>> the kernel, which is why I initially didn't want stubs for the
>> non-optional variant. Of course that would cause build failures and/or
>> reduced compile test coverage, so we added the stubs and the warning to
>> make it obvious when a misconfigured kernel is running: on a kernel with
>> RESET_CONTROLLER=n drivers that use reset_control_get are expected to
>> build, but they are not expected to work. I suppose the same is the case
>> for _optional, if the dt entry is present, so maybe we should drop
>> reset_control_get_optional and add always a warning in case of
>> -EOPNOTSUPP.
>> I don't want all drivers to have to differentiate between -EOPNOTSUPP
>> and -ENOENT error codes, only current reset_control_get_optional users
>> have to do that.
>
>
> I've posted a patch to drop reset_control_get_optional;
> https://patchwork.kernel.org/patch/9284063/
>
> Could you check if it works?
>
> If we go this way, my patch
> 289363fd99a17d6249ee1373541f1da43cbb22c5
> in your reset/next branch is completely useless.
>
> As the commits in the reset-subsystem do not appear
> even in linux-next until they are pulled into the ASOC tree,
> how about dropping 289363fd and turning around?
>


If you want to take time for this topic,
how about dropping 289363fd99a17d6249ee1373541f1da43cbb22c5
("reset: add WARN_ON(1) to non-optional reset_control_get variants")
for now?


I noticed some reset consumers already started dropping _optional,
while their reset lines should be really optional.

See
commit d0e08b0077f49e209bc90305ddf1ca434ac6cc0e
commit 62d9694a003dba585026df36c181e3ca930aeafc

Even generic drivers such as ehci-platform.c / ohci-platform.c
opted out of _optional.


If commit 289363fd99a17d6249ee1373541f1da43cbb22c5 is merged,
users of the generic drivers but without reset-controller
will start to complain about the WARN_ON(1)  sooner or later.


-- 
Best Regards
Masahiro Yamada

Why do we need reset_control_get_optional() ?

[Philipp Zabel]

Am Mittwoch, den 24.08.2016, 15:58 +0900 schrieb Masahiro Yamada:
> Hi Philipp,
> 
> 2016-08-16 23:36 GMT+09:00 Masahiro Yamada <yamada.masahiro@socionext.com>:
> > Hi Philipp, Arnd.
> >
> >
> >
> > 2016-08-09 1:39 GMT+09:00 Philipp Zabel <p.zabel@pengutronix.de>:
> >> Am Freitag, den 05.08.2016, 17:50 +0200 schrieb Arnd Bergmann:
> >>> On Thursday, July 28, 2016 1:00:49 PM CEST Philipp Zabel wrote:
> >>> > Am Donnerstag, den 28.07.2016, 19:52 +0900 schrieb Masahiro Yamada:
> >>>
> >>> > > > In my experimental patch, I make the _optional functions
> >>> > > > return NULL if no "resets" property is provided but return
> >>> > > > an error if there are reset lines but the subsystem is
> >>> > > > disabled, i.e. an optional reset must be used if it's in the
> >>> > > > DT, but can be ignored otherwise.
> >>> > >
> >>> > > I do not like this idea.
> >>> > >
> >>> > > reset_control_get() (or variants) should not return NULL, it is ambiguous.
> >>> > > It should return ERR_PTR(-ENOENT) if no "resets" property.
> >>> > >
> >>> > > I only want two types for functions that return a pointer.
> >>> > >
> >>> > > [1] return a valid pointer on success, or return NULL on failure
> >>> > >     (for example, kmalloc())
> >>> > > [2] return a valid pointer on success, or return error pointer on failure
> >>> > >    (many of _register() functions)
> >>> > >
> >>> > > Mixing [1] and [2] will be a mess.
> >>>
> >>> Ah, right. I was thinking only of the case where the reset subsystem
> >>> is completely disabled here, so returning NULL could be considered
> >>> a valid return code that can in turn be passed into the other
> >>> functions.
> >>>
> >>> However, I agree that returning NULL as a valid result from
> >>> ..._get_optional() would be bad style, so let's drop my idea
> >>> there.
> >>>
> >>> > I too would prefer to keep that as-is. The reset_control_get_optional
> >>> > stub could return -ENOENT if there is no resets device tree property.
> >>>
> >>> Now I'm also confused about what we really need
> >>> reset_control_get_optional() for, and which error codes the callers
> >>> are supposed to check.
> >>>
> >>> This is the matrix I think you mean for _get_optional:
> >>>
> >> [...]
> >>> CONFIG_RESET_CONTROLLER=n, dt entry present: -EOPNOTSUPP
> >>> CONFIG_RESET_CONTROLLER=n, dt entry missing: -ENOENT
> >>
> >> ^^ I didn't consider this distiction.
> >>
> >>> Is this what you had in mind? If so, what is the value of the
> >>> added runtime warning for reset_control_get? Any caller of that
> >>> function would already check for errors, the only difference
> >>> I see is that callers of _optional can ignore -ENOENT.
> >>
> >> My initial motivation was to make it as hard as possible to misconfigure
> >> the kernel, which is why I initially didn't want stubs for the
> >> non-optional variant. Of course that would cause build failures and/or
> >> reduced compile test coverage, so we added the stubs and the warning to
> >> make it obvious when a misconfigured kernel is running: on a kernel with
> >> RESET_CONTROLLER=n drivers that use reset_control_get are expected to
> >> build, but they are not expected to work. I suppose the same is the case
> >> for _optional, if the dt entry is present, so maybe we should drop
> >> reset_control_get_optional and add always a warning in case of
> >> -EOPNOTSUPP.
> >> I don't want all drivers to have to differentiate between -EOPNOTSUPP
> >> and -ENOENT error codes, only current reset_control_get_optional users
> >> have to do that.
> >
> >
> > I've posted a patch to drop reset_control_get_optional;
> > https://patchwork.kernel.org/patch/9284063/
> >
> > Could you check if it works?
> >
> > If we go this way, my patch
> > 289363fd99a17d6249ee1373541f1da43cbb22c5
> > in your reset/next branch is completely useless.
> >
> > As the commits in the reset-subsystem do not appear
> > even in linux-next until they are pulled into the ASOC tree,
> > how about dropping 289363fd and turning around?
> >
> 
> 
> If you want to take time for this topic,
> how about dropping 289363fd99a17d6249ee1373541f1da43cbb22c5
> ("reset: add WARN_ON(1) to non-optional reset_control_get variants")
> for now?
> 
> 
> I noticed some reset consumers already started dropping _optional,
> while their reset lines should be really optional.
> 
> See
> commit d0e08b0077f49e209bc90305ddf1ca434ac6cc0e
> commit 62d9694a003dba585026df36c181e3ca930aeafc
> 
> Even generic drivers such as ehci-platform.c / ohci-platform.c
> opted out of _optional.
> 
> 
> If commit 289363fd99a17d6249ee1373541f1da43cbb22c5 is merged,
> users of the generic drivers but without reset-controller
> will start to complain about the WARN_ON(1)  sooner or later.

Hmm, I would really like to keep the warnings, but as Arnd suggested,
only if the device tree property is present. I'll drop your patch for
now.

regards
Philipp

Why do we need reset_control_get_optional() ?

[Philipp Zabel]

Am Dienstag, den 16.08.2016, 18:41 +0900 schrieb Masahiro Yamada:
> Hi Arnd,
>
> 2016-08-06 0:35 GMT+09:00 Arnd Bergmann <arnd@arndb.de>:
> 
> >
> > config RESET_FOO
> >         bool "FOO reset controller" if COMPILE_TEST && !ARCH_FOO
> >         default ARCH_FOO
> >
> > then I think we get both: you won't be able to turn it off
> > but also get the build testing.
>
> This looks good to me, too.
> I do not know if we need "&& !ARCH_FOO", though.
> 
> When we are compile-testing, we do not care if it works on run-time,
> so perhaps it makes sense to allow to enable/disable RESET_FOO
> regardless ARCH_FOO.

I'll send a few patches to add the Kconfig symbols. Turning around the
RESET_CONTROLLER selection can be done in a second step.

regards
Philipp