Re: [PATCH v4 13/19] reset: starfive: Add StarFive JH7110 reset driver

From: Conor Dooley <conor@kernel.org>
To: Emil Renner Berthing <emil.renner.berthing@canonical.com>
Cc: Hal Feng <hal.feng@starfivetech.com>,
	linux-clk@vger.kernel.org, devicetree@vger.kernel.org,
	linux-riscv@lists.infradead.org, Stephen Boyd <sboyd@kernel.org>,
	Michael Turquette <mturquette@baylibre.com>,
	Philipp Zabel <p.zabel@pengutronix.de>,
	Rob Herring <robh+dt@kernel.org>,
	Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
	Palmer Dabbelt <palmer@dabbelt.com>,
	Paul Walmsley <paul.walmsley@sifive.com>,
	Albert Ou <aou@eecs.berkeley.edu>,
	Ben Dooks <ben.dooks@sifive.com>,
	Daniel Lezcano <daniel.lezcano@linaro.org>,
	Thomas Gleixner <tglx@linutronix.de>,
	Marc Zyngier <maz@kernel.org>,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH v4 13/19] reset: starfive: Add StarFive JH7110 reset driver
Date: Tue, 21 Feb 2023 16:34:11 +0000	[thread overview]
Message-ID: <Y/TygwbxyuhbDooX@spud> (raw)
In-Reply-To: <CAJM55Z_O9K=sCRXga9pMOXk9YjXc1v_V0e5S-8xN9Mi8gvHzvg@mail.gmail.com>

[-- Attachment #1: Type: text/plain, Size: 2040 bytes --]

On Tue, Feb 21, 2023 at 04:33:09PM +0100, Emil Renner Berthing wrote:
> On Tue, 21 Feb 2023 at 03:47, Hal Feng <hal.feng@starfivetech.com> wrote:
> >
> > Add auxiliary driver to support StarFive JH7110 system
> > and always-on resets.
> >
> > Reported-by: kernel test robot <lkp@intel.com>

Drop the reported-by here too please Hal.

> > Signed-off-by: Hal Feng <hal.feng@starfivetech.com>

> > +static int jh7110_reset_probe(struct auxiliary_device *adev,
> > +                             const struct auxiliary_device_id *id)
> > +{
> > +       struct reset_info *info = (struct reset_info *)(id->driver_data);
> > +       void __iomem **base = (void __iomem **)dev_get_drvdata(adev->dev.parent);
> 
> Hi Hal,
> 
> I saw the kernel test robot complain about this, but I still wonder if
> the extra level of indirection is really needed. Isn't it enough to
> just add the explicit casts, so
> 
> dev_set_drvdata(priv->dev, (void *)priv->base);
> 
> in the clock drivers and here just
> 
> void __iomem *base = (void __iomem *)dev_get_drvdata(adev->dev.parent);

I *think* if you do that, sparse will complain that you cast away the
__iomem. The complaint is something like "cast removes address space
qualifier from expression".

The other option is, rather than set the base as the drvdata, just pass
the whole priv struct. That's what I did for mpfs at least & I thought I
had suggested it on v3, but must not have.
It looks prettier than the casting madness at least ;)

> > +
> > +       if (!info || !base)
> > +               return -ENODEV;
> > +
> > +       return reset_starfive_jh71x0_register(&adev->dev, adev->dev.parent->of_node,
> > +                                             *base + info->assert_offset,
> > +                                             *base + info->status_offset,
> > +                                             NULL,
> > +                                             info->nr_resets,
> > +                                             NULL);
> > +}

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]