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@xxxxxxxxxxxxx>
> > 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@xxxxxxxxxxxxx>
>
> 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.
Attachment:
signature.asc
Description: PGP signature
