bus: unifier-system-bus: Remove open coded "ranges" parsing

[PATCH] bus: unifier-system-bus: Remove open coded "ranges" parsing

Posted by Rob Herring 2 years, 7 months ago

"ranges" is a standard property and we have common helper functions for
parsing it, so let's use them.

Signed-off-by: Rob Herring <robh@kernel.org>
---
Compile tested only!
---
 drivers/bus/uniphier-system-bus.c | 54 +++++++------------------------
 1 file changed, 11 insertions(+), 43 deletions(-)

diff --git a/drivers/bus/uniphier-system-bus.c b/drivers/bus/uniphier-system-bus.c
index f70dedace20b..cb5c89ce7b86 100644
--- a/drivers/bus/uniphier-system-bus.c
+++ b/drivers/bus/uniphier-system-bus.c
@@ -176,10 +176,9 @@ static int uniphier_system_bus_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
 	struct uniphier_system_bus_priv *priv;
-	const __be32 *ranges;
-	u32 cells, addr, size;
-	u64 paddr;
-	int pna, bank, rlen, rone, ret;
+	struct of_range_parser parser;
+	struct of_range range;
+	int ret;
 
 	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
 	if (!priv)
@@ -191,48 +190,17 @@ static int uniphier_system_bus_probe(struct platform_device *pdev)
 
 	priv->dev = dev;
 
-	pna = of_n_addr_cells(dev->of_node);
-
-	ret = of_property_read_u32(dev->of_node, "#address-cells", &cells);
-	if (ret) {
-		dev_err(dev, "failed to get #address-cells\n");
-		return ret;
-	}
-	if (cells != 2) {
-		dev_err(dev, "#address-cells must be 2\n");
-		return -EINVAL;
-	}
-
-	ret = of_property_read_u32(dev->of_node, "#size-cells", &cells);
-	if (ret) {
-		dev_err(dev, "failed to get #size-cells\n");
+	ret = of_range_parser_init(&parser, dev->of_node);
+	if (ret)
 		return ret;
-	}
-	if (cells != 1) {
-		dev_err(dev, "#size-cells must be 1\n");
-		return -EINVAL;
-	}
 
-	ranges = of_get_property(dev->of_node, "ranges", &rlen);
-	if (!ranges) {
-		dev_err(dev, "failed to get ranges property\n");
-		return -ENOENT;
-	}
-
-	rlen /= sizeof(*ranges);
-	rone = pna + 2;
-
-	for (; rlen >= rone; rlen -= rone) {
-		bank = be32_to_cpup(ranges++);
-		addr = be32_to_cpup(ranges++);
-		paddr = of_translate_address(dev->of_node, ranges);
-		if (paddr == OF_BAD_ADDR)
+	for_each_of_range(&parser, &range) {
+		if (range.cpu_addr == OF_BAD_ADDR)
 			return -EINVAL;
-		ranges += pna;
-		size = be32_to_cpup(ranges++);
-
-		ret = uniphier_system_bus_add_bank(priv, bank, addr,
-						   paddr, size);
+		ret = uniphier_system_bus_add_bank(priv,
+						   upper_32_bits(range.bus_addr),
+						   lower_32_bits(range.bus_addr),
+						   range.cpu_addr, range.size);
 		if (ret)
 			return ret;
 	}
-- 
2.39.0

Re: [PATCH] bus: unifier-system-bus: Remove open coded "ranges" parsing

Posted by Kunihiko Hayashi 2 years, 7 months ago

Hi Rob,

On 2023/02/02 7:00, Rob Herring wrote:
> "ranges" is a standard property and we have common helper functions for
> parsing it, so let's use them.
> 
> Signed-off-by: Rob Herring <robh@kernel.org>
> ---
> Compile tested only!

Please fix the driver's name.

s/unifier-system-bus/uniphier-system-bus/

> ---
>   drivers/bus/uniphier-system-bus.c | 54 +++++++------------------------
>   1 file changed, 11 insertions(+), 43 deletions(-)
> 
> diff --git a/drivers/bus/uniphier-system-bus.c
> b/drivers/bus/uniphier-system-bus.c
> index f70dedace20b..cb5c89ce7b86 100644
> --- a/drivers/bus/uniphier-system-bus.c
> +++ b/drivers/bus/uniphier-system-bus.c
> @@ -176,10 +176,9 @@ static int uniphier_system_bus_probe(struct
> platform_device *pdev)
>   {
>   	struct device *dev = &pdev->dev;
>   	struct uniphier_system_bus_priv *priv;
> -	const __be32 *ranges;
> -	u32 cells, addr, size;
> -	u64 paddr;
> -	int pna, bank, rlen, rone, ret;
> +	struct of_range_parser parser;
> +	struct of_range range;
> +	int ret;
>   
>   	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
>   	if (!priv)
> @@ -191,48 +190,17 @@ static int uniphier_system_bus_probe(struct
> platform_device *pdev)
>   
>   	priv->dev = dev;
>   
> -	pna = of_n_addr_cells(dev->of_node);
> -
> -	ret = of_property_read_u32(dev->of_node, "#address-cells",
> &cells);
> -	if (ret) {
> -		dev_err(dev, "failed to get #address-cells\n");
> -		return ret;
> -	}
> -	if (cells != 2) {
> -		dev_err(dev, "#address-cells must be 2\n");
> -		return -EINVAL;
> -	}

Don't you need to check the value of "#address-cells"?

> -
> -	ret = of_property_read_u32(dev->of_node, "#size-cells", &cells);
> -	if (ret) {
> -		dev_err(dev, "failed to get #size-cells\n");
> +	ret = of_range_parser_init(&parser, dev->of_node);
> +	if (ret)
>   		return ret;
> -	}
> -	if (cells != 1) {
> -		dev_err(dev, "#size-cells must be 1\n");
> -		return -EINVAL;
> -	}

Same as "#size-cells"

> -	ranges = of_get_property(dev->of_node, "ranges", &rlen);
> -	if (!ranges) {
> -		dev_err(dev, "failed to get ranges property\n");
> -		return -ENOENT;
> -	}
> -
> -	rlen /= sizeof(*ranges);
> -	rone = pna + 2;
> -
> -	for (; rlen >= rone; rlen -= rone) {
> -		bank = be32_to_cpup(ranges++);
> -		addr = be32_to_cpup(ranges++);
> -		paddr = of_translate_address(dev->of_node, ranges);
> -		if (paddr == OF_BAD_ADDR)
> +	for_each_of_range(&parser, &range) {
> +		if (range.cpu_addr == OF_BAD_ADDR)
>   			return -EINVAL;
> -		ranges += pna;
> -		size = be32_to_cpup(ranges++);
> -
> -		ret = uniphier_system_bus_add_bank(priv, bank, addr,
> -						   paddr, size);
> +		ret = uniphier_system_bus_add_bank(priv,
> +						
> upper_32_bits(range.bus_addr),
> +						
> lower_32_bits(range.bus_addr),
> +						   range.cpu_addr,
> range.size);
>   		if (ret)
>   			return ret;
>   	}

I confirmed the value of all the arguments of uniphier_system_bus_add_bank()
match the previous ones.

Tested-by: Kunihiko Hayashi <hayashi.kunihiko@socionext.com>

Thank you,

---
Best Regards
Kunihiko Hayashi

Re: [PATCH] bus: unifier-system-bus: Remove open coded "ranges" parsing

Posted by Rob Herring 2 years, 7 months ago

On Wed, Feb 1, 2023 at 11:50 PM Kunihiko Hayashi
<hayashi.kunihiko@socionext.com> wrote:
>
> Hi Rob,
>
> On 2023/02/02 7:00, Rob Herring wrote:
> > "ranges" is a standard property and we have common helper functions for
> > parsing it, so let's use them.
> >
> > Signed-off-by: Rob Herring <robh@kernel.org>
> > ---
> > Compile tested only!
>
> Please fix the driver's name.
>
> s/unifier-system-bus/uniphier-system-bus/

doh!

>
> > ---
> >   drivers/bus/uniphier-system-bus.c | 54 +++++++------------------------
> >   1 file changed, 11 insertions(+), 43 deletions(-)
> >
> > diff --git a/drivers/bus/uniphier-system-bus.c
> > b/drivers/bus/uniphier-system-bus.c
> > index f70dedace20b..cb5c89ce7b86 100644
> > --- a/drivers/bus/uniphier-system-bus.c
> > +++ b/drivers/bus/uniphier-system-bus.c
> > @@ -176,10 +176,9 @@ static int uniphier_system_bus_probe(struct
> > platform_device *pdev)
> >   {
> >       struct device *dev = &pdev->dev;
> >       struct uniphier_system_bus_priv *priv;
> > -     const __be32 *ranges;
> > -     u32 cells, addr, size;
> > -     u64 paddr;
> > -     int pna, bank, rlen, rone, ret;
> > +     struct of_range_parser parser;
> > +     struct of_range range;
> > +     int ret;
> >
> >       priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> >       if (!priv)
> > @@ -191,48 +190,17 @@ static int uniphier_system_bus_probe(struct
> > platform_device *pdev)
> >
> >       priv->dev = dev;
> >
> > -     pna = of_n_addr_cells(dev->of_node);
> > -
> > -     ret = of_property_read_u32(dev->of_node, "#address-cells",
> > &cells);
> > -     if (ret) {
> > -             dev_err(dev, "failed to get #address-cells\n");
> > -             return ret;
> > -     }
> > -     if (cells != 2) {
> > -             dev_err(dev, "#address-cells must be 2\n");
> > -             return -EINVAL;
> > -     }
>
> Don't you need to check the value of "#address-cells"?

Doesn't your schema do that?

It's not the kernel's job to validate the DT. If it is, then it does a
terrible job.

> > -
> > -     ret = of_property_read_u32(dev->of_node, "#size-cells", &cells);
> > -     if (ret) {
> > -             dev_err(dev, "failed to get #size-cells\n");
> > +     ret = of_range_parser_init(&parser, dev->of_node);
> > +     if (ret)
> >               return ret;
> > -     }
> > -     if (cells != 1) {
> > -             dev_err(dev, "#size-cells must be 1\n");
> > -             return -EINVAL;
> > -     }
>
> Same as "#size-cells"

While the address clearly needs cells to hold the chip select, there's
no reason to restrict the size cells.

>
> I confirmed the value of all the arguments of uniphier_system_bus_add_bank()
> match the previous ones.
>
> Tested-by: Kunihiko Hayashi <hayashi.kunihiko@socionext.com>

Thanks.

Rob

Re: [PATCH] bus: unifier-system-bus: Remove open coded "ranges" parsing

Posted by Kunihiko Hayashi 2 years, 7 months ago

Hi Rob,

On 2023/02/04 0:06, Rob Herring wrote:
> On Wed, Feb 1, 2023 at 11:50 PM Kunihiko Hayashi
> <hayashi.kunihiko@socionext.com> wrote:
>>
>> Hi Rob,
>>
>> On 2023/02/02 7:00, Rob Herring wrote:
>>> "ranges" is a standard property and we have common helper functions for
>>> parsing it, so let's use them.
>>>
>>> Signed-off-by: Rob Herring <robh@kernel.org>
>>> ---
>>> Compile tested only!
>>
>> Please fix the driver's name.
>>
>> s/unifier-system-bus/uniphier-system-bus/
> 
> doh!
> 
>>
>>> ---
>>>    drivers/bus/uniphier-system-bus.c | 54 +++++++------------------------
>>>    1 file changed, 11 insertions(+), 43 deletions(-)
>>>
>>> diff --git a/drivers/bus/uniphier-system-bus.c
>>> b/drivers/bus/uniphier-system-bus.c
>>> index f70dedace20b..cb5c89ce7b86 100644
>>> --- a/drivers/bus/uniphier-system-bus.c
>>> +++ b/drivers/bus/uniphier-system-bus.c
>>> @@ -176,10 +176,9 @@ static int uniphier_system_bus_probe(struct
>>> platform_device *pdev)
>>>    {
>>>        struct device *dev = &pdev->dev;
>>>        struct uniphier_system_bus_priv *priv;
>>> -     const __be32 *ranges;
>>> -     u32 cells, addr, size;
>>> -     u64 paddr;
>>> -     int pna, bank, rlen, rone, ret;
>>> +     struct of_range_parser parser;
>>> +     struct of_range range;
>>> +     int ret;
>>>
>>>        priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
>>>        if (!priv)
>>> @@ -191,48 +190,17 @@ static int uniphier_system_bus_probe(struct
>>> platform_device *pdev)
>>>
>>>        priv->dev = dev;
>>>
>>> -     pna = of_n_addr_cells(dev->of_node);
>>> -
>>> -     ret = of_property_read_u32(dev->of_node, "#address-cells",
>>> &cells);
>>> -     if (ret) {
>>> -             dev_err(dev, "failed to get #address-cells\n");
>>> -             return ret;
>>> -     }
>>> -     if (cells != 2) {
>>> -             dev_err(dev, "#address-cells must be 2\n");
>>> -             return -EINVAL;
>>> -     }
>>
>> Don't you need to check the value of "#address-cells"?
> 
> Doesn't your schema do that?
> 
> It's not the kernel's job to validate the DT. If it is, then it does a
> terrible job.

Ah, this is the code before DT validation, and it's no longer necessary.

>>> -
>>> -     ret = of_property_read_u32(dev->of_node, "#size-cells", &cells);
>>> -     if (ret) {
>>> -             dev_err(dev, "failed to get #size-cells\n");
>>> +     ret = of_range_parser_init(&parser, dev->of_node);
>>> +     if (ret)
>>>                return ret;
>>> -     }
>>> -     if (cells != 1) {
>>> -             dev_err(dev, "#size-cells must be 1\n");
>>> -             return -EINVAL;
>>> -     }
>>
>> Same as "#size-cells"
> 
> While the address clearly needs cells to hold the chip select, there's
> no reason to restrict the size cells.

I see.
I understand that size is limited by value, not by cell width.
It's also no longer necessary.

>>
>> I confirmed the value of all the arguments of
>> uniphier_system_bus_add_bank()
>> match the previous ones.
>>
>> Tested-by: Kunihiko Hayashi <hayashi.kunihiko@socionext.com>
> 
> Thanks.
Thank you,

---
Best Regards
Kunihiko Hayashi