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

[Kunihiko Hayashi]

Hi Rob,

On 2023/02/04 0:06, Rob Herring wrote:
> On Wed, Feb 1, 2023 at 11:50 PM Kunihiko Hayashi
> <hayashi.kunihiko@xxxxxxxxxxxxx> 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@xxxxxxxxxx>
> > > ---
> > > 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@xxxxxxxxxxxxx>
>
> Thanks.
Thank you,

---
Best Regards
Kunihiko Hayashi