Linux-Renesas-SoC Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH] of: Fix of_empty_ranges_quirk()
@ 2019-08-09 17:33 marek.vasut
  2019-08-09 20:41 ` Simon Horman
  2019-08-09 22:34 ` Rob Herring
  0 siblings, 2 replies; 7+ messages in thread
From: marek.vasut @ 2019-08-09 17:33 UTC (permalink / raw)
  To: devicetree; +Cc: Marek Vasut, Rob Herring, Frank Rowand, linux-renesas-soc

From: Marek Vasut <marek.vasut+renesas@gmail.com>

The of_empty_ranges_quirk() returns a mix of boolean and signed integer
types, which cannot work well. Replace that with boolean only and fix
usage logic in of_translate_one() -- the check should trigger when the
ranges are NULL and the quirk is applicable on the hardware.

Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com>
Cc: Rob Herring <robh+dt@kernel.org>
Cc: Frank Rowand <frowand.list@gmail.com>
Cc: linux-renesas-soc@vger.kernel.org
To: devicetree@vger.kernel.org
---
 drivers/of/address.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/of/address.c b/drivers/of/address.c
index b492176c0572..ae2819e148b8 100644
--- a/drivers/of/address.c
+++ b/drivers/of/address.c
@@ -616,7 +616,7 @@ static struct of_bus *of_match_bus(struct device_node *np)
 	return NULL;
 }
 
-static int of_empty_ranges_quirk(struct device_node *np)
+static bool of_empty_ranges_quirk(struct device_node *np)
 {
 	if (IS_ENABLED(CONFIG_PPC)) {
 		/* To save cycles, we cache the result for global "Mac" setting */
@@ -631,7 +631,8 @@ static int of_empty_ranges_quirk(struct device_node *np)
 			quirk_state =
 				of_machine_is_compatible("Power Macintosh") ||
 				of_machine_is_compatible("MacRISC");
-		return quirk_state;
+		if (quirk_state > 0)
+			return true;
 	}
 	return false;
 }
@@ -662,8 +663,8 @@ static int of_translate_one(struct device_node *parent, struct of_bus *bus,
 	 * This code is only enabled on powerpc. --gcl
 	 */
 	ranges = of_get_property(parent, rprop, &rlen);
-	if (ranges == NULL && !of_empty_ranges_quirk(parent)) {
-		pr_debug("no ranges; cannot translate\n");
+	if (ranges == NULL && of_empty_ranges_quirk(parent)) {
+		pr_err("no ranges; cannot translate\n");
 		return 1;
 	}
 	if (ranges == NULL || rlen == 0) {
-- 
2.20.1


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] of: Fix of_empty_ranges_quirk()
  2019-08-09 17:33 [PATCH] of: Fix of_empty_ranges_quirk() marek.vasut
@ 2019-08-09 20:41 ` Simon Horman
  2019-08-09 22:34 ` Rob Herring
  1 sibling, 0 replies; 7+ messages in thread
From: Simon Horman @ 2019-08-09 20:41 UTC (permalink / raw)
  To: marek.vasut
  Cc: devicetree, Marek Vasut, Rob Herring, Frank Rowand, linux-renesas-soc

On Fri, Aug 09, 2019 at 07:33:21PM +0200, marek.vasut@gmail.com wrote:
> From: Marek Vasut <marek.vasut+renesas@gmail.com>
> 
> The of_empty_ranges_quirk() returns a mix of boolean and signed integer
> types, which cannot work well. Replace that with boolean only and fix
> usage logic in of_translate_one() -- the check should trigger when the
> ranges are NULL and the quirk is applicable on the hardware.
> 
> Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com>
> Cc: Rob Herring <robh+dt@kernel.org>
> Cc: Frank Rowand <frowand.list@gmail.com>
> Cc: linux-renesas-soc@vger.kernel.org
> To: devicetree@vger.kernel.org

Reviewed-by: Simon Horman <horms+renesas@verge.net.au>

> ---
>  drivers/of/address.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/of/address.c b/drivers/of/address.c
> index b492176c0572..ae2819e148b8 100644
> --- a/drivers/of/address.c
> +++ b/drivers/of/address.c
> @@ -616,7 +616,7 @@ static struct of_bus *of_match_bus(struct device_node *np)
>  	return NULL;
>  }
>  
> -static int of_empty_ranges_quirk(struct device_node *np)
> +static bool of_empty_ranges_quirk(struct device_node *np)
>  {
>  	if (IS_ENABLED(CONFIG_PPC)) {
>  		/* To save cycles, we cache the result for global "Mac" setting */
> @@ -631,7 +631,8 @@ static int of_empty_ranges_quirk(struct device_node *np)
>  			quirk_state =
>  				of_machine_is_compatible("Power Macintosh") ||
>  				of_machine_is_compatible("MacRISC");
> -		return quirk_state;
> +		if (quirk_state > 0)
> +			return true;
>  	}
>  	return false;
>  }
> @@ -662,8 +663,8 @@ static int of_translate_one(struct device_node *parent, struct of_bus *bus,
>  	 * This code is only enabled on powerpc. --gcl
>  	 */
>  	ranges = of_get_property(parent, rprop, &rlen);
> -	if (ranges == NULL && !of_empty_ranges_quirk(parent)) {
> -		pr_debug("no ranges; cannot translate\n");
> +	if (ranges == NULL && of_empty_ranges_quirk(parent)) {
> +		pr_err("no ranges; cannot translate\n");
>  		return 1;
>  	}
>  	if (ranges == NULL || rlen == 0) {
> -- 
> 2.20.1
> 

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] of: Fix of_empty_ranges_quirk()
  2019-08-09 17:33 [PATCH] of: Fix of_empty_ranges_quirk() marek.vasut
  2019-08-09 20:41 ` Simon Horman
@ 2019-08-09 22:34 ` Rob Herring
  2019-08-10 13:39   ` Marek Vasut
  1 sibling, 1 reply; 7+ messages in thread
From: Rob Herring @ 2019-08-09 22:34 UTC (permalink / raw)
  To: Marek Vašut
  Cc: devicetree, Marek Vasut, Frank Rowand,
	open list:MEDIA DRIVERS FOR RENESAS - FCP

On Fri, Aug 9, 2019 at 11:33 AM <marek.vasut@gmail.com> wrote:
>
> From: Marek Vasut <marek.vasut+renesas@gmail.com>
>
> The of_empty_ranges_quirk() returns a mix of boolean and signed integer
> types, which cannot work well.

It never returns a negative. The negative is used as an uninitialized
flag. Note quirk_state is static.

> Replace that with boolean only and fix
> usage logic in of_translate_one() -- the check should trigger when the
> ranges are NULL and the quirk is applicable on the hardware.
>
> Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com>
> Cc: Rob Herring <robh+dt@kernel.org>
> Cc: Frank Rowand <frowand.list@gmail.com>
> Cc: linux-renesas-soc@vger.kernel.org
> To: devicetree@vger.kernel.org
> ---
>  drivers/of/address.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/of/address.c b/drivers/of/address.c
> index b492176c0572..ae2819e148b8 100644
> --- a/drivers/of/address.c
> +++ b/drivers/of/address.c
> @@ -616,7 +616,7 @@ static struct of_bus *of_match_bus(struct device_node *np)
>         return NULL;
>  }
>
> -static int of_empty_ranges_quirk(struct device_node *np)
> +static bool of_empty_ranges_quirk(struct device_node *np)
>  {
>         if (IS_ENABLED(CONFIG_PPC)) {
>                 /* To save cycles, we cache the result for global "Mac" setting */
> @@ -631,7 +631,8 @@ static int of_empty_ranges_quirk(struct device_node *np)
>                         quirk_state =
>                                 of_machine_is_compatible("Power Macintosh") ||
>                                 of_machine_is_compatible("MacRISC");
> -               return quirk_state;
> +               if (quirk_state > 0)
> +                       return true;
>         }
>         return false;
>  }
> @@ -662,8 +663,8 @@ static int of_translate_one(struct device_node *parent, struct of_bus *bus,
>          * This code is only enabled on powerpc. --gcl
>          */
>         ranges = of_get_property(parent, rprop, &rlen);
> -       if (ranges == NULL && !of_empty_ranges_quirk(parent)) {
> -               pr_debug("no ranges; cannot translate\n");
> +       if (ranges == NULL && of_empty_ranges_quirk(parent)) {
> +               pr_err("no ranges; cannot translate\n");

This is wrong. If you have NULL ranges and not the quirk, then no
ranges is an error. IOW, if you are getting an error here, you have an
error in your DT (because I assume you are not working on a PASemi or
Apple system).

Rob

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] of: Fix of_empty_ranges_quirk()
  2019-08-09 22:34 ` Rob Herring
@ 2019-08-10 13:39   ` Marek Vasut
  2019-08-10 19:47     ` Frank Rowand
  2019-08-10 23:06     ` Rob Herring
  0 siblings, 2 replies; 7+ messages in thread
From: Marek Vasut @ 2019-08-10 13:39 UTC (permalink / raw)
  To: Rob Herring
  Cc: devicetree, Marek Vasut, Frank Rowand,
	open list:MEDIA DRIVERS FOR RENESAS - FCP

On 8/10/19 12:34 AM, Rob Herring wrote:
> On Fri, Aug 9, 2019 at 11:33 AM <marek.vasut@gmail.com> wrote:
>>
>> From: Marek Vasut <marek.vasut+renesas@gmail.com>
>>
>> The of_empty_ranges_quirk() returns a mix of boolean and signed integer
>> types, which cannot work well.
> 
> It never returns a negative. The negative is used as an uninitialized
> flag. Note quirk_state is static.

It's still mixing boolean and signed int types though, which isn't right.

>> Replace that with boolean only and fix
>> usage logic in of_translate_one() -- the check should trigger when the
>> ranges are NULL and the quirk is applicable on the hardware.
>>
>> Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com>
>> Cc: Rob Herring <robh+dt@kernel.org>
>> Cc: Frank Rowand <frowand.list@gmail.com>
>> Cc: linux-renesas-soc@vger.kernel.org
>> To: devicetree@vger.kernel.org
>> ---
>>  drivers/of/address.c | 9 +++++----
>>  1 file changed, 5 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/of/address.c b/drivers/of/address.c
>> index b492176c0572..ae2819e148b8 100644
>> --- a/drivers/of/address.c
>> +++ b/drivers/of/address.c
>> @@ -616,7 +616,7 @@ static struct of_bus *of_match_bus(struct device_node *np)
>>         return NULL;
>>  }
>>
>> -static int of_empty_ranges_quirk(struct device_node *np)
>> +static bool of_empty_ranges_quirk(struct device_node *np)
>>  {
>>         if (IS_ENABLED(CONFIG_PPC)) {
>>                 /* To save cycles, we cache the result for global "Mac" setting */
>> @@ -631,7 +631,8 @@ static int of_empty_ranges_quirk(struct device_node *np)
>>                         quirk_state =
>>                                 of_machine_is_compatible("Power Macintosh") ||
>>                                 of_machine_is_compatible("MacRISC");
>> -               return quirk_state;
>> +               if (quirk_state > 0)
>> +                       return true;
>>         }
>>         return false;
>>  }
>> @@ -662,8 +663,8 @@ static int of_translate_one(struct device_node *parent, struct of_bus *bus,
>>          * This code is only enabled on powerpc. --gcl
>>          */
>>         ranges = of_get_property(parent, rprop, &rlen);
>> -       if (ranges == NULL && !of_empty_ranges_quirk(parent)) {
>> -               pr_debug("no ranges; cannot translate\n");
>> +       if (ranges == NULL && of_empty_ranges_quirk(parent)) {
>> +               pr_err("no ranges; cannot translate\n");
> 
> This is wrong. If you have NULL ranges and not the quirk, then no
> ranges is an error. IOW, if you are getting an error here, you have an
> error in your DT (because I assume you are not working on a PASemi or
> Apple system).

The way I understand the code is that
if (you have no ranges in the DT) AND (the quirk is applicable) then
print the message. Which is what this patch does.

Am I missing something ?

-- 
Best regards,
Marek Vasut

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] of: Fix of_empty_ranges_quirk()
  2019-08-10 13:39   ` Marek Vasut
@ 2019-08-10 19:47     ` Frank Rowand
  2019-09-07 15:15       ` Marek Vasut
  2019-08-10 23:06     ` Rob Herring
  1 sibling, 1 reply; 7+ messages in thread
From: Frank Rowand @ 2019-08-10 19:47 UTC (permalink / raw)
  To: Marek Vasut, Rob Herring
  Cc: devicetree, Marek Vasut, open list:MEDIA DRIVERS FOR RENESAS - FCP

On 8/10/19 6:39 AM, Marek Vasut wrote:
> On 8/10/19 12:34 AM, Rob Herring wrote:
>> On Fri, Aug 9, 2019 at 11:33 AM <marek.vasut@gmail.com> wrote:
>>>
>>> From: Marek Vasut <marek.vasut+renesas@gmail.com>
>>>
>>> The of_empty_ranges_quirk() returns a mix of boolean and signed integer
>>> types, which cannot work well.
>>
>> It never returns a negative. The negative is used as an uninitialized
>> flag. Note quirk_state is static.
> 
> It's still mixing boolean and signed int types though, which isn't right.

From a code readability aspect, Marek is correct.

The code author used "stupid (or clever) coding tricks" (tm) to save a
little bit of memory.  A more readable implementation would be:


static bool of_empty_ranges_quirk(struct device_node *np)
{
        /*
         * As far as we know, the missing "ranges" problem only exists on Apple
	 * machines, so only enable the exception on powerpc. --gcl
         */

        if (IS_ENABLED(CONFIG_PPC)) {
                /* Cache the result for global "Mac" setting */
                static int quirk_state_initialized = 0;
                static bool quirk_state;

                /* PA-SEMI sdc DT bug */
                if (of_device_is_compatible(np, "1682m-sdc"))
                        return true;

                if (!quirk_state_initialized)
			quirk_state_initialized = 1;
                        quirk_state =
                                of_machine_is_compatible("Power Macintosh") ||
                                of_machine_is_compatible("MacRISC");
                return quirk_state;
        }
        return false;
}


I would also rename of_empty_ranges_quirk() to something like
of_missing_ranges_is_ok() or of_missing_ranges_allowed().
"quirk" does not convey any useful information while my proposed rename
describes what the function is actually checking for.

The comment that I added is currently in the caller of of_empty_ranges_quirk(),
but instead belongs in of_empty_ranges_quirk().  When I read that comment in
of_translate_one(), my reaction was to look for the check for powerpc in
of_translate_one() and to be puzzled when I could not find it.  I also
modified the comment for the changed context.  Thus the "--gcl" portion
of the comment should also be removed from of_translate_one().

The more readable implementation (IMNSHO) uses slightly more memory and
slightly more code, but it is more direct about what it is doing and thus
more readable.

-Frank


> 
>>> Replace that with boolean only and fix
>>> usage logic in of_translate_one() -- the check should trigger when the
>>> ranges are NULL and the quirk is applicable on the hardware.
>>>
>>> Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com>
>>> Cc: Rob Herring <robh+dt@kernel.org>
>>> Cc: Frank Rowand <frowand.list@gmail.com>
>>> Cc: linux-renesas-soc@vger.kernel.org
>>> To: devicetree@vger.kernel.org
>>> ---
>>>  drivers/of/address.c | 9 +++++----
>>>  1 file changed, 5 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/of/address.c b/drivers/of/address.c
>>> index b492176c0572..ae2819e148b8 100644
>>> --- a/drivers/of/address.c
>>> +++ b/drivers/of/address.c
>>> @@ -616,7 +616,7 @@ static struct of_bus *of_match_bus(struct device_node *np)
>>>         return NULL;
>>>  }
>>>
>>> -static int of_empty_ranges_quirk(struct device_node *np)
>>> +static bool of_empty_ranges_quirk(struct device_node *np)
>>>  {
>>>         if (IS_ENABLED(CONFIG_PPC)) {
>>>                 /* To save cycles, we cache the result for global "Mac" setting */
>>> @@ -631,7 +631,8 @@ static int of_empty_ranges_quirk(struct device_node *np)
>>>                         quirk_state =
>>>                                 of_machine_is_compatible("Power Macintosh") ||
>>>                                 of_machine_is_compatible("MacRISC");
>>> -               return quirk_state;
>>> +               if (quirk_state > 0)
>>> +                       return true;
>>>         }
>>>         return false;
>>>  }
>>> @@ -662,8 +663,8 @@ static int of_translate_one(struct device_node *parent, struct of_bus *bus,
>>>          * This code is only enabled on powerpc. --gcl
>>>          */
>>>         ranges = of_get_property(parent, rprop, &rlen);
>>> -       if (ranges == NULL && !of_empty_ranges_quirk(parent)) {
>>> -               pr_debug("no ranges; cannot translate\n");
>>> +       if (ranges == NULL && of_empty_ranges_quirk(parent)) {
>>> +               pr_err("no ranges; cannot translate\n");
>>
>> This is wrong. If you have NULL ranges and not the quirk, then no
>> ranges is an error. IOW, if you are getting an error here, you have an
>> error in your DT (because I assume you are not working on a PASemi or
>> Apple system).
> 
> The way I understand the code is that
> if (you have no ranges in the DT) AND (the quirk is applicable) then
> print the message. Which is what this patch does.
> 
> Am I missing something ?
> 


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] of: Fix of_empty_ranges_quirk()
  2019-08-10 13:39   ` Marek Vasut
  2019-08-10 19:47     ` Frank Rowand
@ 2019-08-10 23:06     ` Rob Herring
  1 sibling, 0 replies; 7+ messages in thread
From: Rob Herring @ 2019-08-10 23:06 UTC (permalink / raw)
  To: Marek Vasut
  Cc: devicetree, Marek Vasut, Frank Rowand,
	open list:MEDIA DRIVERS FOR RENESAS - FCP

On Sat, Aug 10, 2019 at 7:39 AM Marek Vasut <marek.vasut@gmail.com> wrote:
>
> On 8/10/19 12:34 AM, Rob Herring wrote:
> > On Fri, Aug 9, 2019 at 11:33 AM <marek.vasut@gmail.com> wrote:
> >>
> >> From: Marek Vasut <marek.vasut+renesas@gmail.com>
> >>
> >> The of_empty_ranges_quirk() returns a mix of boolean and signed integer
> >> types, which cannot work well.
> >
> > It never returns a negative. The negative is used as an uninitialized
> > flag. Note quirk_state is static.
>
> It's still mixing boolean and signed int types though, which isn't right.

I'm really only interested in touching this code if it is too remove
it. But some reason people still run 1990s Macs.

> >> Replace that with boolean only and fix
> >> usage logic in of_translate_one() -- the check should trigger when the
> >> ranges are NULL and the quirk is applicable on the hardware.
> >>
> >> Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com>
> >> Cc: Rob Herring <robh+dt@kernel.org>
> >> Cc: Frank Rowand <frowand.list@gmail.com>
> >> Cc: linux-renesas-soc@vger.kernel.org
> >> To: devicetree@vger.kernel.org
> >> ---
> >>  drivers/of/address.c | 9 +++++----
> >>  1 file changed, 5 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/drivers/of/address.c b/drivers/of/address.c
> >> index b492176c0572..ae2819e148b8 100644
> >> --- a/drivers/of/address.c
> >> +++ b/drivers/of/address.c
> >> @@ -616,7 +616,7 @@ static struct of_bus *of_match_bus(struct device_node *np)
> >>         return NULL;
> >>  }
> >>
> >> -static int of_empty_ranges_quirk(struct device_node *np)
> >> +static bool of_empty_ranges_quirk(struct device_node *np)
> >>  {
> >>         if (IS_ENABLED(CONFIG_PPC)) {
> >>                 /* To save cycles, we cache the result for global "Mac" setting */
> >> @@ -631,7 +631,8 @@ static int of_empty_ranges_quirk(struct device_node *np)
> >>                         quirk_state =
> >>                                 of_machine_is_compatible("Power Macintosh") ||
> >>                                 of_machine_is_compatible("MacRISC");
> >> -               return quirk_state;
> >> +               if (quirk_state > 0)
> >> +                       return true;
> >>         }
> >>         return false;
> >>  }
> >> @@ -662,8 +663,8 @@ static int of_translate_one(struct device_node *parent, struct of_bus *bus,
> >>          * This code is only enabled on powerpc. --gcl
> >>          */
> >>         ranges = of_get_property(parent, rprop, &rlen);
> >> -       if (ranges == NULL && !of_empty_ranges_quirk(parent)) {
> >> -               pr_debug("no ranges; cannot translate\n");
> >> +       if (ranges == NULL && of_empty_ranges_quirk(parent)) {
> >> +               pr_err("no ranges; cannot translate\n");
> >
> > This is wrong. If you have NULL ranges and not the quirk, then no
> > ranges is an error. IOW, if you are getting an error here, you have an
> > error in your DT (because I assume you are not working on a PASemi or
> > Apple system).
>
> The way I understand the code is that
> if (you have no ranges in the DT) AND (the quirk is applicable) then
> print the message. Which is what this patch does.

Your understanding is wrong.

> Am I missing something ?

The normal case is you must have ranges to translate addresses. If you
don't have ranges (say for I2C addresses), then you shouldn't be in
this code. The quirk is for when there is not a ranges property but
should be. IOW, if the quirk is true, then pretend there is an empty
ranges (1:1 translation) property and continue to translate the
address.

Rob

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] of: Fix of_empty_ranges_quirk()
  2019-08-10 19:47     ` Frank Rowand
@ 2019-09-07 15:15       ` Marek Vasut
  0 siblings, 0 replies; 7+ messages in thread
From: Marek Vasut @ 2019-09-07 15:15 UTC (permalink / raw)
  To: Frank Rowand, Rob Herring
  Cc: devicetree, Marek Vasut, open list:MEDIA DRIVERS FOR RENESAS - FCP

On 8/10/19 9:47 PM, Frank Rowand wrote:
> On 8/10/19 6:39 AM, Marek Vasut wrote:
>> On 8/10/19 12:34 AM, Rob Herring wrote:
>>> On Fri, Aug 9, 2019 at 11:33 AM <marek.vasut@gmail.com> wrote:
>>>>
>>>> From: Marek Vasut <marek.vasut+renesas@gmail.com>
>>>>
>>>> The of_empty_ranges_quirk() returns a mix of boolean and signed integer
>>>> types, which cannot work well.
>>>
>>> It never returns a negative. The negative is used as an uninitialized
>>> flag. Note quirk_state is static.
>>
>> It's still mixing boolean and signed int types though, which isn't right.
> 
> From a code readability aspect, Marek is correct.
> 
> The code author used "stupid (or clever) coding tricks" (tm) to save a
> little bit of memory.  A more readable implementation would be:
> 
> 
> static bool of_empty_ranges_quirk(struct device_node *np)
> {
>         /*
>          * As far as we know, the missing "ranges" problem only exists on Apple
> 	 * machines, so only enable the exception on powerpc. --gcl
>          */
> 
>         if (IS_ENABLED(CONFIG_PPC)) {
>                 /* Cache the result for global "Mac" setting */
>                 static int quirk_state_initialized = 0;
>                 static bool quirk_state;
> 
>                 /* PA-SEMI sdc DT bug */
>                 if (of_device_is_compatible(np, "1682m-sdc"))
>                         return true;
> 
>                 if (!quirk_state_initialized)
> 			quirk_state_initialized = 1;
>                         quirk_state =
>                                 of_machine_is_compatible("Power Macintosh") ||
>                                 of_machine_is_compatible("MacRISC");
>                 return quirk_state;
>         }
>         return false;
> }
> 
> 
> I would also rename of_empty_ranges_quirk() to something like
> of_missing_ranges_is_ok() or of_missing_ranges_allowed().
> "quirk" does not convey any useful information while my proposed rename
> describes what the function is actually checking for.
> 
> The comment that I added is currently in the caller of of_empty_ranges_quirk(),
> but instead belongs in of_empty_ranges_quirk().  When I read that comment in
> of_translate_one(), my reaction was to look for the check for powerpc in
> of_translate_one() and to be puzzled when I could not find it.  I also
> modified the comment for the changed context.  Thus the "--gcl" portion
> of the comment should also be removed from of_translate_one().
> 
> The more readable implementation (IMNSHO) uses slightly more memory and
> slightly more code, but it is more direct about what it is doing and thus
> more readable.

Thanks for the input, sorry for the delay, let me send a V2.

-- 
Best regards,
Marek Vasut

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, back to index

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-08-09 17:33 [PATCH] of: Fix of_empty_ranges_quirk() marek.vasut
2019-08-09 20:41 ` Simon Horman
2019-08-09 22:34 ` Rob Herring
2019-08-10 13:39   ` Marek Vasut
2019-08-10 19:47     ` Frank Rowand
2019-09-07 15:15       ` Marek Vasut
2019-08-10 23:06     ` Rob Herring

Linux-Renesas-SoC Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-renesas-soc/0 linux-renesas-soc/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-renesas-soc linux-renesas-soc/ https://lore.kernel.org/linux-renesas-soc \
		linux-renesas-soc@vger.kernel.org linux-renesas-soc@archiver.kernel.org
	public-inbox-index linux-renesas-soc


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-renesas-soc


AGPL code for this site: git clone https://public-inbox.org/ public-inbox