| Message ID | 20230201220011.247100-1-robh@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | bus: unifier-system-bus: Remove open coded "ranges" parsing | expand |
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
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
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
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; }
"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(-)