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 = jh7110_vout_top_crg_init(priv, top); > > >> + if (ret) > > >> + goto err_clk; > > >> + > > >> + top->base = priv->base; > > >> + dev_set_drvdata(priv->dev, (void *)(&top->base)); > > > > > > See comment later about setting this to 'top' instead. Casting away > > > iomem markings is not good hygiene. > > > > JH7110 resets as the auxiliary device of clocks use the same iomem as the clocks > > and the iomem will be got by dev_get_drvdata() in the 7110 reset drivers when registering reset. > > So I follow the basic 7110 reset driver and also set the iomem not top_crg struct. > > 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@xxxxxxxxxxxxxxxx/ 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!
Attachment:
signature.asc
Description: PGP signature
