MIME-Version: 1.0
References: <20230201220011.247100-1-robh@kernel.org> <bb307806-fa02-d147-2c25-6b3b3bbd411b@socionext.com>
In-Reply-To: <bb307806-fa02-d147-2c25-6b3b3bbd411b@socionext.com>
From:   Rob Herring <robh@kernel.org>
Date:   Fri, 3 Feb 2023 09:06:17 -0600
Message-ID: <CAL_JsqLfq6U3OORsKE962f2RZ7kQXLNEb2ShzrWqGY+=oVsf=g@mail.gmail.com>
Subject: Re: [PATCH] bus: unifier-system-bus: Remove open coded "ranges" parsing
To:     Kunihiko Hayashi <hayashi.kunihiko@socionext.com>
Cc:     Masami Hiramatsu <mhiramat@kernel.org>,
        linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

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