Re: [PATCH v2 04/14] reset: starfive: Factor out common JH71X0 reset code

[Hal Feng <hal.feng@starfivetech.com>]

On Sat, 19 Nov 2022 00:39:35 +0800, Emil Renner Berthing wrote:
> On Fri, 18 Nov 2022 at 02:06, Hal Feng <hal.feng@starfivetech.com> wrote:
> > diff --git a/drivers/reset/starfive/reset-starfive-jh7100.c b/drivers/reset/starfive/reset-starfive-jh71x0.c
> > similarity index 50%
> > copy from drivers/reset/starfive/reset-starfive-jh7100.c
> > copy to drivers/reset/starfive/reset-starfive-jh71x0.c
> > index fc44b2fb3e03..1e230f3f9841 100644
> > --- a/drivers/reset/starfive/reset-starfive-jh7100.c
> > +++ b/drivers/reset/starfive/reset-starfive-jh71x0.c

[...]

> > -static int __init jh7100_reset_probe(struct platform_device *pdev)
> > +int reset_starfive_jh7100_register(struct device *dev, struct device_node *of_node,
> > +                                  void __iomem *assert, void __iomem *status,
> > +                                  const u64 *asserted, unsigned int nr_resets,
> > +                                  bool is_module)
> >  {
> >         struct jh7100_reset *data;
> >
> > -       data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
> > +       data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
> >         if (!data)
> >                 return -ENOMEM;
> >
> > -       data->base = devm_platform_ioremap_resource(pdev, 0);
> > -       if (IS_ERR(data->base))
> > -               return PTR_ERR(data->base);
> > -
> >         data->rcdev.ops = &jh7100_reset_ops;
> > -       data->rcdev.owner = THIS_MODULE;
> > -       data->rcdev.nr_resets = JH7100_RSTN_END;
> > -       data->rcdev.dev = &pdev->dev;
> > -       data->rcdev.of_node = pdev->dev.of_node;
> > +       if (is_module)
> > +               data->rcdev.owner = THIS_MODULE;
> 
> nit: consider just passing the owner directly, so this would just be
> data->rcdev.owner = owner;
> 
> ..and callers that used false can just pass NULL.

Yeah, will fix it.

> 
> > +       data->rcdev.nr_resets = nr_resets;
> > +       data->rcdev.dev = dev;
> > +       data->rcdev.of_node = of_node;
> 
> Is it important to register this with the auxiliary device and not
> just use the parent device?

I'm not sure whether it still works if we use the same device, but
it's general to separate the devices of clock and reset. They have
different device names and different drivers.

Best regards,
Hal

> If not you can just always pass the device that has the right of_node
> and have this be
> 
> data->rcdev.of_node = dev->of_node;
> 
> > +
> >         spin_lock_init(&data->lock);
> > +       data->assert = assert;
> > +       data->status = status;
> > +       data->asserted = asserted;
> >
> > -       return devm_reset_controller_register(&pdev->dev, &data->rcdev);
> > +       return devm_reset_controller_register(dev, &data->rcdev);
> >  }