Message ID | 20220715001703.389981-1-windhl@126.com |
---|---|
State | Superseded |
Headers | show |
Series | [v2] ufs: host: ufschd-pltfrm: Hold reference returned by of_parse_phandle() | expand |
At 2022-07-17 22:58:45, "Bart Van Assche" <bvanassche@acm.org> wrote: >On 7/16/22 20:03, Liang He wrote: >> At 2022-07-16 21:50:08, "Bart Van Assche" <bvanassche@acm.org> wrote: >>> On 7/14/22 17:17, Liang He wrote: >>>> +static bool phandle_exists(const struct device_node *np, >>>> + const char *phandle_name, >>>> + int index) >>> >>> Indentation of the arguments now looks really odd :-( >> >> Yes, Bart, I also wonder this coding style, however I learned that >> from the definition of 'of_parse_phandle' in of.h. >> >> Is it OK if I put all of them in one line? > >No. From Documentation/process/coding-style.rst (please read that >document in its entirety): "The preferred limit on the length of a >single line is 80 columns. [...] A very commonly used style >is to align descendants to a function open parenthesis." > >Consider to use the following formatting: > >static bool phandle_exists(const struct device_node *np, > const char *phandle_name, int index) >{ > [ ... ] >} > Hi, Bart, Can you help me as I have a trouble about the indentation. When I align descendants to a function open parenthesis in VIM editor, but when I generate the patch, I find the second line always missing one space in patch format. So is there any problem if I send this patch? I make sure that the alignment in VIM is OK. Thanks, Liang >>>> +{ >>>> + struct device_node *parse_np = of_parse_phandle(np, phandle_name, index); >>>> + bool ret = false; >>>> + >>>> + if (parse_np) { >>>> + ret = true; >>>> + of_node_put(parse_np); >>>> + } >>>> + >>>> + return ret; >>>> +} >>> >>> The 'ret' variable is not necessary. If "return ret" is changed into >>> "return parse_np" then the variable 'ret' can be left out. >>> >> >> OK, I will use 'return parse_np' in new version when you confirm above coding style. > >You may want to use "return parse_np != NULL" if you want to be sure >that nobody else will complain about an implicit conversion of a pointer >to a boolean type. > >Thanks, > >Bart.
On 7/18/22 19:46, Liang He wrote: > Can you help me as I have a trouble about the indentation. > > When I align descendants to a function open parenthesis in VIM editor, > but when I generate the patch, I find the second line always missing one space in > patch format. So is there any problem if I send this patch? > > I make sure that the alignment in VIM is OK. Hi Liang, Please don't worry about this. When a code change is converted into a patch, a single character is inserted at the start of each line (plus, minus or space). That makes code that is indented with tabs look weird. This is normal. Bart.
At 2022-07-20 02:12:36, "Bart Van Assche" <bvanassche@acm.org> wrote: >On 7/18/22 19:46, Liang He wrote: >> Can you help me as I have a trouble about the indentation. >> >> When I align descendants to a function open parenthesis in VIM editor, >> but when I generate the patch, I find the second line always missing one space in >> patch format. So is there any problem if I send this patch? >> >> I make sure that the alignment in VIM is OK. > >Hi Liang, > >Please don't worry about this. When a code change is converted into a >patch, a single character is inserted at the start of each line (plus, >minus or space). That makes code that is indented with tabs look weird. >This is normal. > >Bart. Thanks very much for all your help and your effort to review my code. Liang
diff --git a/drivers/ufs/host/ufshcd-pltfrm.c b/drivers/ufs/host/ufshcd-pltfrm.c index e7332cc65b1f..46ded7813c42 100644 --- a/drivers/ufs/host/ufshcd-pltfrm.c +++ b/drivers/ufs/host/ufshcd-pltfrm.c @@ -108,6 +108,21 @@ static int ufshcd_parse_clock_info(struct ufs_hba *hba) return ret; } +static bool phandle_exists(const struct device_node *np, + const char *phandle_name, + int index) +{ + struct device_node *parse_np = of_parse_phandle(np, phandle_name, index); + bool ret = false; + + if (parse_np) { + ret = true; + of_node_put(parse_np); + } + + return ret; +} + #define MAX_PROP_SIZE 32 static int ufshcd_populate_vreg(struct device *dev, const char *name, struct ufs_vreg **out_vreg) @@ -122,7 +137,7 @@ static int ufshcd_populate_vreg(struct device *dev, const char *name, } snprintf(prop_name, MAX_PROP_SIZE, "%s-supply", name); - if (!of_parse_phandle(np, prop_name, 0)) { + if (!phandle_exists(np, prop_name, 0)) { dev_info(dev, "%s: Unable to find %s regulator, assuming enabled\n", __func__, prop_name); goto out;
In ufshcd_populate_vreg(), we should hold the reference returned by of_parse_phandle() and then use it to call of_node_put() for refcount balance. Fixes: aa4976130934 ("ufs: Add regulator enable support") Signed-off-by: Liang He <windhl@126.com> --- changelog: v2: use a proper helper name advised by Bart Van Assche. v1: add a helper to fix the bug v1 link: https://lore.kernel.org/all/20220714055413.373449-1-windhl@126.com/ drivers/ufs/host/ufshcd-pltfrm.c | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-)