* [PATCH v1] reset: Make optional stuff optional for all users
@ 2017-04-03 12:26 Andy Shevchenko
2017-04-03 14:27 ` Philipp Zabel
0 siblings, 1 reply; 8+ messages in thread
From: Andy Shevchenko @ 2017-04-03 12:26 UTC (permalink / raw)
To: Philipp Zabel, linux-kernel, Mika Westerberg
Cc: Andy Shevchenko, Ramiro Oliveira
There is Device Tree oriented check for optional resource. Of course
it will fail on non-DT platforms.
Remove this check to make things optional for all users.
Fixes: bb475230b8e5 ("reset: make optional functions really optional")
Cc: Ramiro Oliveira <Ramiro.Oliveira@synopsys.com>
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
The reset framework is too Device Tree oriented, and who knows what
the logic was behind the commit which introduced devm_reset_*()
functions without thinking out of the DT box.
This commit fixes almost all Intel newest boards that have no legacy
UART since UART driver started using this DT-centric framework.
drivers/reset/core.c | 3 ---
1 file changed, 3 deletions(-)
diff --git a/drivers/reset/core.c b/drivers/reset/core.c
index f1e5e65388bb..62314e663f29 100644
--- a/drivers/reset/core.c
+++ b/drivers/reset/core.c
@@ -331,9 +331,6 @@ struct reset_control *__of_reset_control_get(struct device_node *node,
int rstc_id;
int ret;
- if (!node)
- return ERR_PTR(-EINVAL);
-
if (id) {
index = of_property_match_string(node,
"reset-names", id);
--
2.11.0
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH v1] reset: Make optional stuff optional for all users
2017-04-03 12:26 [PATCH v1] reset: Make optional stuff optional for all users Andy Shevchenko
@ 2017-04-03 14:27 ` Philipp Zabel
2017-04-03 14:31 ` Andy Shevchenko
0 siblings, 1 reply; 8+ messages in thread
From: Philipp Zabel @ 2017-04-03 14:27 UTC (permalink / raw)
To: Andy Shevchenko; +Cc: linux-kernel, Mika Westerberg, Ramiro Oliveira
Hi Andy,
thank you for the patch.
On Mon, 2017-04-03 at 15:26 +0300, Andy Shevchenko wrote:
> There is Device Tree oriented check for optional resource. Of course
> it will fail on non-DT platforms.
>
> Remove this check to make things optional for all users.
>
> Fixes: bb475230b8e5 ("reset: make optional functions really optional")
> Cc: Ramiro Oliveira <Ramiro.Oliveira@synopsys.com>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>
> The reset framework is too Device Tree oriented, and who knows what
> the logic was behind the commit which introduced devm_reset_*()
> functions without thinking out of the DT box.
At the time, there was nothing outside of the box to describe reset
lines, and as far as I am aware there still isn't, so the devm_reset_*
should behave as if the reset line is not specified in the non-DT case.
Returning -EINVAL was reasonable in that case, before the API was
changed to describe unavailable, optional reset controls as rstc = NULL.
> This commit fixes almost all Intel newest boards that have no legacy
> UART since UART driver started using this DT-centric framework.
Is this is about 8250_dw.c? Unfortunately it sometimes takes a little
while for me to get updated on the big picture, as I only get the actual
reset driver and framework patches in my inbox. Usually I only see the
reset consumer changes when I actively look for them.
But the fault in this case was is with me not considering all possible
code paths influenced by commit bb475230b8e5, in the configurations that
I can't test myself.
> drivers/reset/core.c | 3 ---
> 1 file changed, 3 deletions(-)
>
> diff --git a/drivers/reset/core.c b/drivers/reset/core.c
> index f1e5e65388bb..62314e663f29 100644
> --- a/drivers/reset/core.c
> +++ b/drivers/reset/core.c
> @@ -331,9 +331,6 @@ struct reset_control *__of_reset_control_get(struct device_node *node,
Ideally, __of_reset_control_get would not be called at all in the non-DT
case. I'll change that in the next round, but for now I'd prefer a small
fix in place.
> int rstc_id;
> int ret;
>
> - if (!node)
> - return ERR_PTR(-EINVAL);
> -
This should be
if (!node)
return optional ? NULL : ERR_PTR(-EINVAL);
instead. Can you confirm this works for Intel boards with DW UART? I can
fix it up when applying if you agree.
> if (id) {
> index = of_property_match_string(node,
> "reset-names", id);
regards
Philipp
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v1] reset: Make optional stuff optional for all users
2017-04-03 14:27 ` Philipp Zabel
@ 2017-04-03 14:31 ` Andy Shevchenko
2017-04-03 14:33 ` Shevchenko, Andriy
0 siblings, 1 reply; 8+ messages in thread
From: Andy Shevchenko @ 2017-04-03 14:31 UTC (permalink / raw)
To: Philipp Zabel; +Cc: linux-kernel, Mika Westerberg, Ramiro Oliveira
On Mon, 2017-04-03 at 16:27 +0200, Philipp Zabel wrote:
> Hi Andy,
>
> thank you for the patch.
>
> On Mon, 2017-04-03 at 15:26 +0300, Andy Shevchenko wrote:
> > There is Device Tree oriented check for optional resource. Of course
> > it will fail on non-DT platforms.
> >
> > Remove this check to make things optional for all users.
> >
> > Fixes: bb475230b8e5 ("reset: make optional functions really
> > optional")
> > Cc: Ramiro Oliveira <Ramiro.Oliveira@synopsys.com>
> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > ---
> >
> > The reset framework is too Device Tree oriented, and who knows what
> > the logic was behind the commit which introduced devm_reset_*()
> > functions without thinking out of the DT box.
>
> At the time, there was nothing outside of the box to describe reset
> lines, and as far as I am aware there still isn't, so the devm_reset_*
> should behave as if the reset line is not specified in the non-DT
> case.
> Returning -EINVAL was reasonable in that case, before the API was
> changed to describe unavailable, optional reset controls as rstc =
> NULL.
Fair enough.
>
> > This commit fixes almost all Intel newest boards that have no
> > legacy
> > UART since UART driver started using this DT-centric framework.
>
> Is this is about 8250_dw.c? Unfortunately it sometimes takes a little
> while for me to get updated on the big picture, as I only get the
> actual
> reset driver and framework patches in my inbox. Usually I only see the
> reset consumer changes when I actively look for them.
Yes, 8250_dw.c in this case.
>
> But the fault in this case was is with me not considering all possible
> code paths influenced by commit bb475230b8e5, in the configurations
> that
> I can't test myself.
Understood.
>
> > drivers/reset/core.c | 3 ---
> > 1 file changed, 3 deletions(-)
> >
> > diff --git a/drivers/reset/core.c b/drivers/reset/core.c
> > index f1e5e65388bb..62314e663f29 100644
> > --- a/drivers/reset/core.c
> > +++ b/drivers/reset/core.c
> > @@ -331,9 +331,6 @@ struct reset_control
> > *__of_reset_control_get(struct device_node *node,
>
> Ideally, __of_reset_control_get would not be called at all in the non-
> DT
> case. I'll change that in the next round, but for now I'd prefer a
> small
> fix in place.
>
> > int rstc_id;
> > int ret;
> >
> > - if (!node)
> > - return ERR_PTR(-EINVAL);
> > -
>
> This should be
>
> if (!node)
> return optional ? NULL : ERR_PTR(-EINVAL);
>
> instead. Can you confirm this works for Intel boards with DW UART? I
> can
> fix it up when applying if you agree.
I don't think it worth to change. I specifically checked all of_* calls
in that function and they cope pretty nice with node == NULL.
So, I rather to go with my initial change.
Thanks for review!
--
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v1] reset: Make optional stuff optional for all users
2017-04-03 14:31 ` Andy Shevchenko
@ 2017-04-03 14:33 ` Shevchenko, Andriy
2017-04-03 15:09 ` Philipp Zabel
0 siblings, 1 reply; 8+ messages in thread
From: Shevchenko, Andriy @ 2017-04-03 14:33 UTC (permalink / raw)
To: p.zabel; +Cc: mika.westerberg, linux-kernel, Ramiro.Oliveira
On Mon, 2017-04-03 at 17:31 +0300, Andy Shevchenko wrote:
> On Mon, 2017-04-03 at 16:27 +0200, Philipp Zabel wrote:
> >
> > > int rstc_id;
> > > int ret;
> > >
> > > - if (!node)
> > > - return ERR_PTR(-EINVAL);
> > > -
> >
> > This should be
> >
> > if (!node)
> > return optional ? NULL : ERR_PTR(-EINVAL);
> >
> > instead. Can you confirm this works for Intel boards with DW UART? I
> > can
> > fix it up when applying if you agree.
>
> I don't think it worth to change. I specifically checked all of_*
> calls
> in that function and they cope pretty nice with node == NULL.
>
> So, I rather to go with my initial change.
>
Hit Enter before closing another thought.
When you come with solution where this __of_reset_control_get() will be
called only for node != NULL case you will not need that check either.
So, I would go my solution because of two benefits:
- it fixes bug
- if will not bring ping-ponging code
> Thanks for review!
>
--
Andy Shevchenko <andriy.shevchenko@intel.com>
Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160 Espoo
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v1] reset: Make optional stuff optional for all users
2017-04-03 14:33 ` Shevchenko, Andriy
@ 2017-04-03 15:09 ` Philipp Zabel
2017-04-03 15:23 ` Andy Shevchenko
0 siblings, 1 reply; 8+ messages in thread
From: Philipp Zabel @ 2017-04-03 15:09 UTC (permalink / raw)
To: Shevchenko, Andriy; +Cc: mika.westerberg, linux-kernel, Ramiro.Oliveira
On Mon, 2017-04-03 at 14:33 +0000, Shevchenko, Andriy wrote:
> On Mon, 2017-04-03 at 17:31 +0300, Andy Shevchenko wrote:
> > On Mon, 2017-04-03 at 16:27 +0200, Philipp Zabel wrote:
> > >
>
> > > > int rstc_id;
> > > > int ret;
> > > >
> > > > - if (!node)
> > > > - return ERR_PTR(-EINVAL);
> > > > -
> > >
> > > This should be
> > >
> > > if (!node)
> > > return optional ? NULL : ERR_PTR(-EINVAL);
> > >
> > > instead. Can you confirm this works for Intel boards with DW UART? I
> > > can
> > > fix it up when applying if you agree.
> >
> > I don't think it worth to change. I specifically checked all of_*
> > calls
> > in that function and they cope pretty nice with node == NULL.
__of_reset_control_get called with id != NULL calls
of_property_match_string first, which then returns -EINVAL if
node == NULL, which makes __of_reset_control_get return NULL if optional
or -ENOENT otherwise, even though the correct return value would be
-EINVAL in the DT case.
__of_reset_control_get called with id == NULL calls
of_parse_phandle_with_args first, which calls
__of_parse_phandle_with_args, which returns an undefined value if
np == NULL, as far as I can tell:
of_for_each_phandle first calls of_phandle_iterator_init, which, when
called with np == NULL clears the iterator structure returns -ENOENT.
The return value is ignored in the of_for_each_phandle macro, and
of_phandle_iterator_next is then called and returns -ENOENT because
it->cur == NULL, ending the loop without ever assigning a value to rc.
__of_parse_phandle_with_args then returns the uninitialized value.
The point being, instead of having to regularly forage through a number
of of_ API functions to make sure my expectations are still met, I'd
prefer to keep the check in place.
> >
> > So, I rather to go with my initial change.
> >
>
> Hit Enter before closing another thought.
>
> When you come with solution where this __of_reset_control_get() will be
> called only for node != NULL case you will not need that check either.
__of_reset_control_get is public API (via of_reset_control_get), so I
can't guarantee node != NULL even in the DT case.
> So, I would go my solution because of two benefits:
> - it fixes bug
True.
> - if will not bring ping-ponging code
Unfortunately not.
regards
Philipp
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v1] reset: Make optional stuff optional for all users
2017-04-03 15:09 ` Philipp Zabel
@ 2017-04-03 15:23 ` Andy Shevchenko
2017-04-03 16:04 ` Philipp Zabel
0 siblings, 1 reply; 8+ messages in thread
From: Andy Shevchenko @ 2017-04-03 15:23 UTC (permalink / raw)
To: Philipp Zabel; +Cc: mika.westerberg, linux-kernel, Ramiro.Oliveira
On Mon, 2017-04-03 at 17:09 +0200, Philipp Zabel wrote:
> On Mon, 2017-04-03 at 14:33 +0000, Shevchenko, Andriy wrote:
> > On Mon, 2017-04-03 at 17:31 +0300, Andy Shevchenko wrote:
> > > On Mon, 2017-04-03 at 16:27 +0200, Philipp Zabel wrote:
> > > >
> > > > > int rstc_id;
> > > > > int ret;
> > > > >
> > > > > - if (!node)
> > > > > - return ERR_PTR(-EINVAL);
> > > > > -
> > > >
> > > > This should be
> > > >
> > > > if (!node)
> > > > return optional ? NULL : ERR_PTR(-EINVAL);
> > > >
> > > > instead. Can you confirm this works for Intel boards with DW
> > > > UART? I
> > > > can
> > > > fix it up when applying if you agree.
> > >
> > > I don't think it worth to change. I specifically checked all of_*
> > > calls
> > > in that function and they cope pretty nice with node == NULL.
>
> __of_reset_control_get called with id != NULL calls
> of_property_match_string first, which then returns -EINVAL if
> node == NULL, which makes __of_reset_control_get return NULL if
> optional
> or -ENOENT otherwise, even though the correct return value would be
> -EINVAL in the DT case.
Error handling mess as usual. :-)
>
> __of_reset_control_get called with id == NULL calls
> of_parse_phandle_with_args first, which calls
> __of_parse_phandle_with_args, which returns an undefined value if
> np == NULL, as far as I can tell:
> of_for_each_phandle first calls of_phandle_iterator_init, which, when
> called with np == NULL clears the iterator structure returns -ENOENT.
> The return value is ignored in the of_for_each_phandle macro, and
> of_phandle_iterator_next is then called and returns -ENOENT because
> it->cur == NULL, ending the loop without ever assigning a value to rc.
> __of_parse_phandle_with_args then returns the uninitialized value.
It returns -ENOENT. Error value is kept in function variable rc.
>
> The point being, instead of having to regularly forage through a
> number
> of of_ API functions to make sure my expectations are still met, I'd
> prefer to keep the check in place.
I would not insist, I already shared my view on this.
I really don't like ping-ponging of the code. Perhaps you would fix it
once for the best now?
>
> > >
> > > So, I rather to go with my initial change.
> > >
> >
> > Hit Enter before closing another thought.
> >
> > When you come with solution where this __of_reset_control_get() will
> > be
> > called only for node != NULL case you will not need that check
> > either.
>
> __of_reset_control_get is public API (via of_reset_control_get), so I
> can't guarantee node != NULL even in the DT case.
Yes, and that's why other callees will return some error codes there.
>
> > So, I would go my solution because of two benefits:
> > - it fixes bug
>
> True.
>
> > - if will not bring ping-ponging code
>
> Unfortunately not.
Fortunately yes, if someone will fix DT error code mess in the first
place.
--
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v1] reset: Make optional stuff optional for all users
2017-04-03 15:23 ` Andy Shevchenko
@ 2017-04-03 16:04 ` Philipp Zabel
2017-04-03 20:48 ` Andy Shevchenko
0 siblings, 1 reply; 8+ messages in thread
From: Philipp Zabel @ 2017-04-03 16:04 UTC (permalink / raw)
To: Andy Shevchenko; +Cc: mika.westerberg, linux-kernel, Ramiro.Oliveira
On Mon, 2017-04-03 at 18:23 +0300, Andy Shevchenko wrote:
> On Mon, 2017-04-03 at 17:09 +0200, Philipp Zabel wrote:
> > On Mon, 2017-04-03 at 14:33 +0000, Shevchenko, Andriy wrote:
> > > On Mon, 2017-04-03 at 17:31 +0300, Andy Shevchenko wrote:
> > > > On Mon, 2017-04-03 at 16:27 +0200, Philipp Zabel wrote:
> > > > >
> > > > > > int rstc_id;
> > > > > > int ret;
> > > > > >
> > > > > > - if (!node)
> > > > > > - return ERR_PTR(-EINVAL);
> > > > > > -
> > > > >
> > > > > This should be
> > > > >
> > > > > if (!node)
> > > > > return optional ? NULL : ERR_PTR(-EINVAL);
> > > > >
> > > > > instead. Can you confirm this works for Intel boards with DW
> > > > > UART? I
> > > > > can
> > > > > fix it up when applying if you agree.
> > > >
> > > > I don't think it worth to change. I specifically checked all of_*
> > > > calls
> > > > in that function and they cope pretty nice with node == NULL.
> >
> > __of_reset_control_get called with id != NULL calls
> > of_property_match_string first, which then returns -EINVAL if
> > node == NULL, which makes __of_reset_control_get return NULL if
> > optional
> > or -ENOENT otherwise, even though the correct return value would be
> > -EINVAL in the DT case.
>
> Error handling mess as usual. :-)
>
> >
> > __of_reset_control_get called with id == NULL calls
> > of_parse_phandle_with_args first, which calls
> > __of_parse_phandle_with_args, which returns an undefined value if
> > np == NULL, as far as I can tell:
> > of_for_each_phandle first calls of_phandle_iterator_init, which, when
> > called with np == NULL clears the iterator structure returns -ENOENT.
> > The return value is ignored in the of_for_each_phandle macro, and
> > of_phandle_iterator_next is then called and returns -ENOENT because
> > it->cur == NULL, ending the loop without ever assigning a value to rc.
> > __of_parse_phandle_with_args then returns the uninitialized value.
>
> It returns -ENOENT. Error value is kept in function variable rc.
Thanks for the correction. That's still the wrong error code IMHO, but
it certainly makes your patch unproblematic.
> > The point being, instead of having to regularly forage through a
> > number
> > of of_ API functions to make sure my expectations are still met, I'd
> > prefer to keep the check in place.
>
> I would not insist, I already shared my view on this.
Appreciated. With a correct understanding of the current of_* error
return paths it makes a lot more sense.
> I really don't like ping-ponging of the code. Perhaps you would fix it
> once for the best now?
How about:
----------8<----------
>From 75205aeaf52cef5c62710630cceb871468175c12 Mon Sep 17 00:00:00 2001
From: Philipp Zabel <p.zabel@pengutronix.de>
Date: Fri, 5 Feb 2016 13:41:39 +0100
Subject: [PATCH] reset: add exported __reset_control_get, return NULL if
optional
Rename the internal __reset_control_get/put functions to
__reset_control_get/put_internal and add an exported
__reset_control_get equivalent to __of_reset_control_get
that takes a struct device parameter.
This avoids the confusing call to __of_reset_control_get in
the non-DT case and fixes the devm_reset_control_get_optional
function to return NULL if RESET_CONTROLLER is enabled but
dev->of_node == NULL.
Fixes: bb475230b8e5 ("reset: make optional functions really optional")
Reported-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc: Ramiro Oliveira <Ramiro.Oliveira@synopsys.com>
Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
---
drivers/reset/core.c | 22 ++++++++++++++++------
include/linux/reset.h | 22 ++++++++++++++--------
2 files changed, 30 insertions(+), 14 deletions(-)
diff --git a/drivers/reset/core.c b/drivers/reset/core.c
index f1e5e65388bb5..d227168c8971a 100644
--- a/drivers/reset/core.c
+++ b/drivers/reset/core.c
@@ -275,7 +275,7 @@ int reset_control_status(struct reset_control *rstc)
}
EXPORT_SYMBOL_GPL(reset_control_status);
-static struct reset_control *__reset_control_get(
+static struct reset_control *__reset_control_get_internal(
struct reset_controller_dev *rcdev,
unsigned int index, bool shared)
{
@@ -308,7 +308,7 @@ static struct reset_control *__reset_control_get(
return rstc;
}
-static void __reset_control_put(struct reset_control *rstc)
+static void __reset_control_put_internal(struct reset_control *rstc)
{
lockdep_assert_held(&reset_list_mutex);
@@ -377,7 +377,7 @@ struct reset_control *__of_reset_control_get(struct device_node *node,
}
/* reset_list_mutex also protects the rcdev's reset_control list */
- rstc = __reset_control_get(rcdev, rstc_id, shared);
+ rstc = __reset_control_get_internal(rcdev, rstc_id, shared);
mutex_unlock(&reset_list_mutex);
@@ -385,6 +385,17 @@ struct reset_control *__of_reset_control_get(struct device_node *node,
}
EXPORT_SYMBOL_GPL(__of_reset_control_get);
+struct reset_control *__reset_control_get(struct device *dev, const char *id,
+ int index, bool shared, bool optional)
+{
+ if (dev->of_node)
+ return __of_reset_control_get(dev->of_node, id, index, shared,
+ optional);
+ else
+ return optional ? NULL : ERR_PTR(-EINVAL);
+}
+EXPORT_SYMBOL_GPL(__reset_control_get);
+
/**
* reset_control_put - free the reset controller
* @rstc: reset controller
@@ -396,7 +407,7 @@ void reset_control_put(struct reset_control *rstc)
return;
mutex_lock(&reset_list_mutex);
- __reset_control_put(rstc);
+ __reset_control_put_internal(rstc);
mutex_unlock(&reset_list_mutex);
}
EXPORT_SYMBOL_GPL(reset_control_put);
@@ -417,8 +428,7 @@ struct reset_control *__devm_reset_control_get(struct device *dev,
if (!ptr)
return ERR_PTR(-ENOMEM);
- rstc = __of_reset_control_get(dev ? dev->of_node : NULL,
- id, index, shared, optional);
+ rstc = __reset_control_get(dev, id, index, shared, optional);
if (!IS_ERR(rstc)) {
*ptr = rstc;
devres_add(dev, ptr);
diff --git a/include/linux/reset.h b/include/linux/reset.h
index 96fb139bdd08f..13d8681210d54 100644
--- a/include/linux/reset.h
+++ b/include/linux/reset.h
@@ -15,6 +15,9 @@ int reset_control_status(struct reset_control *rstc);
struct reset_control *__of_reset_control_get(struct device_node *node,
const char *id, int index, bool shared,
bool optional);
+struct reset_control *__reset_control_get(struct device *dev, const char *id,
+ int index, bool shared,
+ bool optional);
void reset_control_put(struct reset_control *rstc);
struct reset_control *__devm_reset_control_get(struct device *dev,
const char *id, int index, bool shared,
@@ -72,6 +75,13 @@ static inline struct reset_control *__of_reset_control_get(
return optional ? NULL : ERR_PTR(-ENOTSUPP);
}
+static inline struct reset_control *__reset_control_get(
+ struct device *dev, const char *id,
+ int index, bool shared, bool optional)
+{
+ return optional ? NULL : ERR_PTR(-ENOTSUPP);
+}
+
static inline struct reset_control *__devm_reset_control_get(
struct device *dev, const char *id,
int index, bool shared, bool optional)
@@ -102,8 +112,7 @@ __must_check reset_control_get_exclusive(struct device *dev, const char *id)
#ifndef CONFIG_RESET_CONTROLLER
WARN_ON(1);
#endif
- return __of_reset_control_get(dev ? dev->of_node : NULL, id, 0, false,
- false);
+ return __reset_control_get(dev, id, 0, false, false);
}
/**
@@ -131,22 +140,19 @@ __must_check reset_control_get_exclusive(struct device *dev, const char *id)
static inline struct reset_control *reset_control_get_shared(
struct device *dev, const char *id)
{
- return __of_reset_control_get(dev ? dev->of_node : NULL, id, 0, true,
- false);
+ return __reset_control_get(dev, id, 0, true, false);
}
static inline struct reset_control *reset_control_get_optional_exclusive(
struct device *dev, const char *id)
{
- return __of_reset_control_get(dev ? dev->of_node : NULL, id, 0, false,
- true);
+ return __reset_control_get(dev, id, 0, false, true);
}
static inline struct reset_control *reset_control_get_optional_shared(
struct device *dev, const char *id)
{
- return __of_reset_control_get(dev ? dev->of_node : NULL, id, 0, true,
- true);
+ return __reset_control_get(dev, id, 0, true, true);
}
/**
--
2.11.0
---------->8----------
That should make both the __reset_control_get stub and implementation
return NULL if optional && (dev->of_node == NULL), and stop calling
__of_reset_control_get in the non-DT case, while maintaining the -EINVAL
error return value if of_reset_control_get* is called with node == NULL.
regards
Philipp
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH v1] reset: Make optional stuff optional for all users
2017-04-03 16:04 ` Philipp Zabel
@ 2017-04-03 20:48 ` Andy Shevchenko
0 siblings, 0 replies; 8+ messages in thread
From: Andy Shevchenko @ 2017-04-03 20:48 UTC (permalink / raw)
To: Philipp Zabel
Cc: Andy Shevchenko, mika.westerberg, linux-kernel, Ramiro.Oliveira
On Mon, Apr 3, 2017 at 7:04 PM, Philipp Zabel <p.zabel@pengutronix.de> wrote:
> On Mon, 2017-04-03 at 18:23 +0300, Andy Shevchenko wrote:
>> On Mon, 2017-04-03 at 17:09 +0200, Philipp Zabel wrote:
>> > On Mon, 2017-04-03 at 14:33 +0000, Shevchenko, Andriy wrote:
>> > > On Mon, 2017-04-03 at 17:31 +0300, Andy Shevchenko wrote:
>> I really don't like ping-ponging of the code. Perhaps you would fix it
>> once for the best now?
>
> How about:
Below looks fine to me, one nit still and I would like to test it tomorrow.
Thanks for taking care.
> +struct reset_control *__reset_control_get(struct device *dev, const char *id,
> + int index, bool shared, bool optional)
> +{
> + if (dev->of_node)
> + return __of_reset_control_get(dev->of_node, id, index, shared,
> +
optional);
> + else
Redundant.
> + return optional ? NULL : ERR_PTR(-EINVAL);
> +}
> +EXPORT_SYMBOL_GPL(__reset_control_get);
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2017-04-03 20:48 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-04-03 12:26 [PATCH v1] reset: Make optional stuff optional for all users Andy Shevchenko
2017-04-03 14:27 ` Philipp Zabel
2017-04-03 14:31 ` Andy Shevchenko
2017-04-03 14:33 ` Shevchenko, Andriy
2017-04-03 15:09 ` Philipp Zabel
2017-04-03 15:23 ` Andy Shevchenko
2017-04-03 16:04 ` Philipp Zabel
2017-04-03 20:48 ` Andy Shevchenko
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.