* [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.