"ranges" is a standard property and we have common helper functions for
parsing it, so let's use them.
Signed-off-by: Rob Herring <[email protected]>
---
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
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 <[email protected]>
> ---
> 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 <[email protected]>
Thank you,
---
Best Regards
Kunihiko Hayashi
On Wed, Feb 1, 2023 at 11:50 PM Kunihiko Hayashi
<[email protected]> 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 <[email protected]>
> > ---
> > 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 <[email protected]>
Thanks.
Rob
Hi Rob,
On 2023/02/04 0:06, Rob Herring wrote:
> On Wed, Feb 1, 2023 at 11:50 PM Kunihiko Hayashi
> <[email protected]> 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 <[email protected]>
>>> ---
>>> 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 <[email protected]>
>
> Thanks.
Thank you,
---
Best Regards
Kunihiko Hayashi