diff mbox

[U-Boot,v3,1/3] mmc: fsl_esdhc: move 'status' property fixup into a weak function

Message ID 1481168538-29963-1-git-send-email-yangbo.lu@nxp.com
State Superseded
Delegated to: York Sun
Headers show

Commit Message

Yangbo Lu Dec. 8, 2016, 3:42 a.m. UTC 
Move fdt fixup of 'status' property into a weak function. This allows
board to define 'status' fdt fixup by themselves.

Signed-off-by: Yangbo Lu <yangbo.lu@nxp.com>
---
Changes for v2:
	- None
Changes for v3:
	- None
---
 drivers/mmc/fsl_esdhc.c | 21 ++++++++++++++-------
 1 file changed, 14 insertions(+), 7 deletions(-)

Comments

Yangbo Lu Jan. 12, 2017, 1:42 a.m. UTC | #1 
Hi York,

Any comments on this patchset?
Thanks a lot.


Best regards,
Yangbo Lu

> -----Original Message-----
> From: Yangbo Lu [mailto:yangbo.lu@nxp.com]
> Sent: Thursday, December 08, 2016 11:42 AM
> To: u-boot@lists.denx.de
> Cc: york sun; Y.B. Lu
> Subject: [v3, 1/3] mmc: fsl_esdhc: move 'status' property fixup into a
> weak function
> 
> Move fdt fixup of 'status' property into a weak function. This allows
> board to define 'status' fdt fixup by themselves.
> 
> Signed-off-by: Yangbo Lu <yangbo.lu@nxp.com>
> ---
> Changes for v2:
> 	- None
> Changes for v3:
> 	- None
> ---
>  drivers/mmc/fsl_esdhc.c | 21 ++++++++++++++-------
>  1 file changed, 14 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/mmc/fsl_esdhc.c b/drivers/mmc/fsl_esdhc.c index
> 9796d39..68de04e 100644
> --- a/drivers/mmc/fsl_esdhc.c
> +++ b/drivers/mmc/fsl_esdhc.c
> @@ -908,17 +908,26 @@ void mmc_adapter_card_type_ident(void)  #endif
> 
>  #ifdef CONFIG_OF_LIBFDT
> -void fdt_fixup_esdhc(void *blob, bd_t *bd)
> +__weak int esdhc_status_fixup(void *blob, const char *compat)
>  {
> -	const char *compat = "fsl,esdhc";
> -
>  #ifdef CONFIG_FSL_ESDHC_PIN_MUX
>  	if (!hwconfig("esdhc")) {
>  		do_fixup_by_compat(blob, compat, "status", "disabled",
> -				8 + 1, 1);
> -		return;
> +				sizeof("disabled"), 1);
> +		return 1;
>  	}
>  #endif
> +	do_fixup_by_compat(blob, compat, "status", "okay",
> +			   sizeof("okay"), 1);
> +	return 0;
> +}
> +
> +void fdt_fixup_esdhc(void *blob, bd_t *bd) {
> +	const char *compat = "fsl,esdhc";
> +
> +	if (esdhc_status_fixup(blob, compat))
> +		return;
> 
>  #ifdef CONFIG_FSL_ESDHC_USE_PERIPHERAL_CLK
>  	do_fixup_by_compat_u32(blob, compat, "peripheral-frequency", @@ -
> 931,8 +940,6 @@ void fdt_fixup_esdhc(void *blob, bd_t *bd)
>  	do_fixup_by_compat_u32(blob, compat, "adapter-type",
>  			       (u32)(gd->arch.sdhc_adapter), 1);  #endif
> -	do_fixup_by_compat(blob, compat, "status", "okay",
> -			   4 + 1, 1);
>  }
>  #endif
> 
> --
> 2.1.0.27.g96db324
York Sun Jan. 12, 2017, 5:08 p.m. UTC | #2 
On 01/11/2017 05:42 PM, Y.B. Lu wrote:
> Hi York,
>
> Any comments on this patchset?
> Thanks a lot.

You didn't CC me for this set. I didn't notice them in the list. See 
comment below.

>
>
> Best regards,
> Yangbo Lu
>
>> -----Original Message-----
>> From: Yangbo Lu [mailto:yangbo.lu@nxp.com]
>> Sent: Thursday, December 08, 2016 11:42 AM
>> To: u-boot@lists.denx.de
>> Cc: york sun; Y.B. Lu
>> Subject: [v3, 1/3] mmc: fsl_esdhc: move 'status' property fixup into a
>> weak function
>>
>> Move fdt fixup of 'status' property into a weak function. This allows
>> board to define 'status' fdt fixup by themselves.
>>
>> Signed-off-by: Yangbo Lu <yangbo.lu@nxp.com>
>> ---
>> Changes for v2:
>> 	- None
>> Changes for v3:
>> 	- None
>> ---
>>  drivers/mmc/fsl_esdhc.c | 21 ++++++++++++++-------
>>  1 file changed, 14 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/mmc/fsl_esdhc.c b/drivers/mmc/fsl_esdhc.c index
>> 9796d39..68de04e 100644
>> --- a/drivers/mmc/fsl_esdhc.c
>> +++ b/drivers/mmc/fsl_esdhc.c
>> @@ -908,17 +908,26 @@ void mmc_adapter_card_type_ident(void)  #endif
>>
>>  #ifdef CONFIG_OF_LIBFDT
>> -void fdt_fixup_esdhc(void *blob, bd_t *bd)
>> +__weak int esdhc_status_fixup(void *blob, const char *compat)
>>  {
>> -	const char *compat = "fsl,esdhc";
>> -
>>  #ifdef CONFIG_FSL_ESDHC_PIN_MUX
>>  	if (!hwconfig("esdhc")) {
>>  		do_fixup_by_compat(blob, compat, "status", "disabled",
>> -				8 + 1, 1);
>> -		return;
>> +				sizeof("disabled"), 1);
>> +		return 1;

What's your intention for the non-zero return? It is not considered an 
error, is it?

>>  	}
>>  #endif
>> +	do_fixup_by_compat(blob, compat, "status", "okay",
>> +			   sizeof("okay"), 1);
>> +	return 0;
>> +}
>> +
>> +void fdt_fixup_esdhc(void *blob, bd_t *bd) {
>> +	const char *compat = "fsl,esdhc";
>> +
>> +	if (esdhc_status_fixup(blob, compat))
>> +		return;

With non-zero return, following code will be skipped. This is the same 
as code flow before this change. What are you going to do with the new 
board-level function?

Review comment continues on other patches in this set.

York
Yangbo Lu Jan. 13, 2017, 3:06 a.m. UTC | #3 
> -----Original Message-----
> From: york sun
> Sent: Friday, January 13, 2017 1:09 AM
> To: Y.B. Lu; u-boot@lists.denx.de
> Subject: Re: [v3, 1/3] mmc: fsl_esdhc: move 'status' property fixup into
> a weak function
> 
> On 01/11/2017 05:42 PM, Y.B. Lu wrote:
> > Hi York,
> >
> > Any comments on this patchset?
> > Thanks a lot.
> 
> You didn't CC me for this set. I didn't notice them in the list. See
> comment below.
> 
> >
> >
> > Best regards,
> > Yangbo Lu
> >
> >> -----Original Message-----
> >> From: Yangbo Lu [mailto:yangbo.lu@nxp.com]
> >> Sent: Thursday, December 08, 2016 11:42 AM
> >> To: u-boot@lists.denx.de
> >> Cc: york sun; Y.B. Lu
> >> Subject: [v3, 1/3] mmc: fsl_esdhc: move 'status' property fixup into
> >> a weak function
> >>
> >> Move fdt fixup of 'status' property into a weak function. This allows
> >> board to define 'status' fdt fixup by themselves.
> >>
> >> Signed-off-by: Yangbo Lu <yangbo.lu@nxp.com>
> >> ---
> >> Changes for v2:
> >> 	- None
> >> Changes for v3:
> >> 	- None
> >> ---
> >>  drivers/mmc/fsl_esdhc.c | 21 ++++++++++++++-------
> >>  1 file changed, 14 insertions(+), 7 deletions(-)
> >>
> >> diff --git a/drivers/mmc/fsl_esdhc.c b/drivers/mmc/fsl_esdhc.c index
> >> 9796d39..68de04e 100644
> >> --- a/drivers/mmc/fsl_esdhc.c
> >> +++ b/drivers/mmc/fsl_esdhc.c
> >> @@ -908,17 +908,26 @@ void mmc_adapter_card_type_ident(void)  #endif
> >>
> >>  #ifdef CONFIG_OF_LIBFDT
> >> -void fdt_fixup_esdhc(void *blob, bd_t *bd)
> >> +__weak int esdhc_status_fixup(void *blob, const char *compat)
> >>  {
> >> -	const char *compat = "fsl,esdhc";
> >> -
> >>  #ifdef CONFIG_FSL_ESDHC_PIN_MUX
> >>  	if (!hwconfig("esdhc")) {
> >>  		do_fixup_by_compat(blob, compat, "status", "disabled",
> >> -				8 + 1, 1);
> >> -		return;
> >> +				sizeof("disabled"), 1);
> >> +		return 1;
> 
> What's your intention for the non-zero return? It is not considered an
> error, is it?
> 

[Lu Yangbo-B47093] I intend to get non-zero return when there is no esdhc with 'okay' status.
Then the following fixup could be dropped.

> >>  	}
> >>  #endif
> >> +	do_fixup_by_compat(blob, compat, "status", "okay",
> >> +			   sizeof("okay"), 1);
> >> +	return 0;
> >> +}
> >> +
> >> +void fdt_fixup_esdhc(void *blob, bd_t *bd) {
> >> +	const char *compat = "fsl,esdhc";
> >> +
> >> +	if (esdhc_status_fixup(blob, compat))
> >> +		return;
> 
> With non-zero return, following code will be skipped. This is the same as
> code flow before this change. What are you going to do with the new
> board-level function?

[Lu Yangbo-B47093] As I said, if we 'disabled' the status, the following fixup could be dropped.
The new board-level function is for some specific boards which have some hardware issue of esdhc.
We need to check board hardware in board-level function first and decide to enable it or not.

> 
> Review comment continues on other patches in this set.
> 
> York
diff mbox

Patch

diff --git a/drivers/mmc/fsl_esdhc.c b/drivers/mmc/fsl_esdhc.c
index 9796d39..68de04e 100644
--- a/drivers/mmc/fsl_esdhc.c
+++ b/drivers/mmc/fsl_esdhc.c
@@ -908,17 +908,26 @@  void mmc_adapter_card_type_ident(void)
 #endif
 
 #ifdef CONFIG_OF_LIBFDT
-void fdt_fixup_esdhc(void *blob, bd_t *bd)
+__weak int esdhc_status_fixup(void *blob, const char *compat)
 {
-	const char *compat = "fsl,esdhc";
-
 #ifdef CONFIG_FSL_ESDHC_PIN_MUX
 	if (!hwconfig("esdhc")) {
 		do_fixup_by_compat(blob, compat, "status", "disabled",
-				8 + 1, 1);
-		return;
+				sizeof("disabled"), 1);
+		return 1;
 	}
 #endif
+	do_fixup_by_compat(blob, compat, "status", "okay",
+			   sizeof("okay"), 1);
+	return 0;
+}
+
+void fdt_fixup_esdhc(void *blob, bd_t *bd)
+{
+	const char *compat = "fsl,esdhc";
+
+	if (esdhc_status_fixup(blob, compat))
+		return;
 
 #ifdef CONFIG_FSL_ESDHC_USE_PERIPHERAL_CLK
 	do_fixup_by_compat_u32(blob, compat, "peripheral-frequency",
@@ -931,8 +940,6 @@  void fdt_fixup_esdhc(void *blob, bd_t *bd)
 	do_fixup_by_compat_u32(blob, compat, "adapter-type",
 			       (u32)(gd->arch.sdhc_adapter), 1);
 #endif
-	do_fixup_by_compat(blob, compat, "status", "okay",
-			   4 + 1, 1);
 }
 #endif