linux-kernel - Re: [RFC PATCH v2 1/5] clk: meson: axg: move reset controller's code to separate module

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date: Wed, 10 Apr 2024 10:50:39 +0100
From: Conor Dooley <conor@...nel.org>
To: Stephen Boyd <sboyd@...nel.org>
Cc: Jan Dakinevich <jan.dakinevich@...utedevices.com>,
	Jerome Brunet <jbrunet@...libre.com>,
	Philipp Zabel <p.zabel@...gutronix.de>,
	Neil Armstrong <neil.armstrong@...aro.org>,
	Michael Turquette <mturquette@...libre.com>,
	Rob Herring <robh@...nel.org>,
	Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
	Conor Dooley <conor+dt@...nel.org>,
	Kevin Hilman <khilman@...libre.com>,
	Martin Blumenstingl <martin.blumenstingl@...glemail.com>,
	linux-amlogic@...ts.infradead.org, linux-clk@...r.kernel.org,
	devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
	linux-arm-kernel@...ts.infradead.org
Subject: Re: [RFC PATCH v2 1/5] clk: meson: axg: move reset controller's code
 to separate module

On Tue, Apr 09, 2024 at 07:27:36PM -0700, Stephen Boyd wrote:
> Quoting Conor Dooley (2024-04-09 05:05:37)
> > On Mon, Apr 08, 2024 at 06:05:51PM +0100, Conor Dooley wrote:
> > 
> > > > > Seconded, the clk-mpfs/reset-mpfs and clk-starfive-jh7110-sys/reset-
> > > > > starfive-jh7110 drivers are examples of this.
> > > > > 
> > > > > > The auxiliary device creation function can also be in the
> > > > > > drivers/reset/ directory so that the clk driver calls some function
> > > > > > to create and register the device.
> > > > > 
> > > > > I'm undecided about this, do you think mpfs_reset_controller_register()
> > > > > and jh7110_reset_controller_register() should rather live with the
> > > > > reset aux drivers in drivers/reset/ ?
> > > > 
> > > > Yes, and also mpfs_reset_read() and friends. We should pass the base
> > > > iomem pointer and parent device to mpfs_reset_adev_alloc() instead and
> > > > then move all that code into drivers/reset with some header file
> > > > exported function to call. That way the clk driver hands over the data
> > > > without having to implement half the implementation.
> > > 
> > > I'll todo list that :)
> > 
> > Something like the below?
> > 
> > -- >8 --
> > From a12f281d2cb869bcd9a6ffc45d0c6a0d3aa2e9e2 Mon Sep 17 00:00:00 2001
> > From: Conor Dooley <conor.dooley@...rochip.com>
> > Date: Tue, 9 Apr 2024 11:54:34 +0100
> > Subject: [PATCH] clock, reset: microchip: move all mpfs reset code to the
> >  reset subsystem
> > 
> > <insert something here>
> > 
> > Signed-off-by: Conor Dooley <conor.dooley@...rochip.com>
> 
> Looks pretty good.
> 
> >  static const struct of_device_id mpfs_clk_of_match_table[] = {
> > diff --git a/drivers/reset/reset-mpfs.c b/drivers/reset/reset-mpfs.c
> > index 7f3fb2d472f4..27cd68b4ee81 100644
> > --- a/drivers/reset/reset-mpfs.c
> > +++ b/drivers/reset/reset-mpfs.c
> > @@ -137,9 +139,67 @@ static int mpfs_reset_probe(struct auxiliary_device *adev,
> >         return devm_reset_controller_register(dev, rcdev);
> >  }
> >  
> > +static void mpfs_reset_unregister_adev(void *_adev)
> > +{
> > +       struct auxiliary_device *adev = _adev;
> > +
> > +       auxiliary_device_delete(adev);
> > +       auxiliary_device_uninit(adev);
> > +}
> > +
> > +static void mpfs_reset_adev_release(struct device *dev)
> > +{
> > +       struct auxiliary_device *adev = to_auxiliary_dev(dev);
> > +
> > +       kfree(adev);
> > +}
> > +
> > +static struct auxiliary_device *mpfs_reset_adev_alloc(struct device *clk_dev)
> > +{
> > +       struct auxiliary_device *adev;
> > +       int ret;
> > +
> > +       adev = kzalloc(sizeof(*adev), GFP_KERNEL);
> > +       if (!adev)
> > +               return ERR_PTR(-ENOMEM);
> > +
> > +       adev->name = "reset-mpfs";
> > +       adev->dev.parent = clk_dev;
> > +       adev->dev.release = mpfs_reset_adev_release;
> > +       adev->id = 666u;
> > +
> > +       ret = auxiliary_device_init(adev);
> > +       if (ret) {
> > +               kfree(adev);
> > +               return ERR_PTR(ret);
> > +       }
> > +
> > +       return adev;
> > +}
> > +
> > +int mpfs_reset_controller_register(struct device *clk_dev, void __iomem *base)
> > +{
> > +       struct auxiliary_device *adev;
> > +       int ret;
> > +
> > +       mpfs_reset_addr = base;
> 
> Instead of a global this can be stashed in adev->dev.platform_data and
> grabbed in the driver probe?

I suppose, really I was just being "lazy" here and creating a global
rather than a `struct mpfs_reset` containing only the base address.

The test robot reported some issues with hexagon & COMPILE_TEST, so I'll
push it out again with this change and the fixes for the reported issues
and send the patch ones it gets the all clear.

Cheers,
Conor.

Download attachment "signature.asc" of type "application/pgp-signature" (229 bytes)