diff mbox series

[v2] ufs: host: ufschd-pltfrm: Hold reference returned by of_parse_phandle()

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

Commit Message

Liang He July 15, 2022, 12:17 a.m. UTC
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(-)

Comments

Liang He July 19, 2022, 2:46 a.m. UTC | #1
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.
Bart Van Assche July 19, 2022, 6:12 p.m. UTC | #2
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.
Liang He July 20, 2022, 12:48 a.m. UTC | #3
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 mbox series

Patch

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;