Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Thu, 13 Apr 2023 14:52:44 +0100
From:   Conor Dooley <conor.dooley@microchip.com>
To:     Stephen Boyd <sboyd@kernel.org>
CC:     Conor Dooley <conor@kernel.org>,
        Emil Renner Berthing <kernel@esmil.dk>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
        Michael Turquette <mturquette@baylibre.com>,
        Philipp Zabel <p.zabel@pengutronix.de>,
        Xingyu Wu <xingyu.wu@starfivetech.com>,
        <devicetree@vger.kernel.org>, <linux-riscv@lists.infradead.org>,
        Rob Herring <robh+dt@kernel.org>,
        Paul Walmsley <paul.walmsley@sifive.com>,
        Palmer Dabbelt <palmer@dabbelt.com>,
        Albert Ou <aou@eecs.berkeley.edu>,
        Hal Feng <hal.feng@starfivetech.com>,
        <linux-kernel@vger.kernel.org>, <linux-clk@vger.kernel.org>
Subject: Re: [PATCH v4 07/10] clk: starfive: Add StarFive JH7110 Video-Output
 clock driver
Message-ID: <20230413-hardcover-ground-efbd862c5ec3@wendy>
References: <20230411135558.44282-1-xingyu.wu@starfivetech.com>
 <20230411135558.44282-8-xingyu.wu@starfivetech.com>
 <683cbe934d1df9436e003466d2a419ef.sboyd@kernel.org>
 <463ee23c-f617-bed0-27a8-56c6fb40d092@starfivetech.com>
 <cd4a11ae65e186799145410969d40421.sboyd@kernel.org>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha256;
        protocol="application/pgp-signature"; boundary="v0ip4bKAlPiDmbMW"
Content-Disposition: inline
In-Reply-To: <cd4a11ae65e186799145410969d40421.sboyd@kernel.org>
Precedence: bulk

--v0ip4bKAlPiDmbMW
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

On Wed, Apr 12, 2023 at 09:04:08PM -0700, Stephen Boyd wrote:
> Quoting Xingyu Wu (2023-04-11 23:15:26)
> > On 2023/4/12 2:33, Stephen Boyd wrote:
> > > Quoting Xingyu Wu (2023-04-11 06:55:55)

> > >> +       if (ret < 0)
> > >> +               return dev_err_probe(priv->dev, ret, "failed to turn=
 on power\n");
> > >> +
> > >> +       ret =3D jh7110_vout_top_crg_init(priv, top);
> > >> +       if (ret)
> > >> +               goto err_clk;
> > >> +
> > >> +       top->base =3D priv->base;
> > >> +       dev_set_drvdata(priv->dev, (void *)(&top->base));
> > >=20
> > > See comment later about setting this to 'top' instead. Casting away
> > > iomem markings is not good hygiene.
> >=20
> > JH7110 resets as the auxiliary device of clocks use the same iomem as t=
he clocks
> > and the iomem will be got by dev_get_drvdata() in the 7110 reset driver=
s when registering reset.
> > So I follow the basic 7110 reset driver and also set the iomem not top_=
crg struct.
>=20
> Oh I totally missed that this is how it's been done for the other
> starfive driver. It's still not good hygiene to stash the iomem pointer
> that way because the iomem marking is lost and has to be recovered. Can
> you make a wrapper struct, either for the adev or to pass in struct
> device::platform_data?

FWIW, this did come up on an earlier version of the other starfive
driver:
https://lore.kernel.org/linux-clk/e0d8f9ba-5bf4-d7dd-5110-20d4196556f9@star=
fivetech.com/

I probably should've pushed Hal to use a struct, but evidently I didn't
reply to his final message there, so apologies for that!


--v0ip4bKAlPiDmbMW
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iHUEABYIAB0WIQRh246EGq/8RLhDjO14tDGHoIJi0gUCZDgJLAAKCRB4tDGHoIJi
0grQAQD5cpctgc5Bq3S7CyFGTLA6byxDhiBh5Iyvw4xFI+lLdAD/SOinBHpjM3Vf
npC+7trX9b+0Hc7eNQTp7L341SH1nAw=
=b8hm
-----END PGP SIGNATURE-----

--v0ip4bKAlPiDmbMW--