Re: [PATCH 7/8] reset: amlogic: add auxiliary reset driver support

[Stephen Boyd]

Quoting Jerome Brunet (2024-07-11 02:01:16)
> On Wed 10 Jul 2024 at 15:49, Stephen Boyd <sboyd@xxxxxxxxxx> wrote:
> 
> > Quoting Jerome Brunet (2024-07-10 09:25:16)
> >> diff --git a/drivers/reset/reset-meson.c b/drivers/reset/reset-meson.c
> >> index e34a10b15593..5cc767d50e8f 100644
> >> --- a/drivers/reset/reset-meson.c
> >> +++ b/drivers/reset/reset-meson.c
> > [...]
> >> +
> >> +int devm_meson_rst_aux_register(struct device *dev,
> >> +                               struct regmap *map,
> >> +                               const char *adev_name)
> >> +{
> >> +       struct meson_reset_adev *raux;
> >> +       struct auxiliary_device *adev;
> >> +       int ret;
> >> +
> >> +       raux = kzalloc(sizeof(*raux), GFP_KERNEL);
> >> +       if (!raux)
> >> +               return -ENOMEM;
> >> +
> >> +       ret = ida_alloc(&meson_rst_aux_ida, GFP_KERNEL);
> >
> > Do we expect more than one device with the same name? I wonder if the
> > IDA can be skipped.
> 
> I've wondered about that too.
> 
> I don't think it is the case right now but I'm not 100% sure.
> Since I spent time thinking about it, I thought it would just be safer (and
> relatively cheap) to put in and enough annoying debugging the
> expectation does not hold true.
> 
> I don't have a strong opinion on this. What do you prefer ?

I don't have a strong opinion either so it's fine to leave it there.

> >> diff --git a/include/soc/amlogic/meson-auxiliary-reset.h b/include/soc/amlogic/meson-auxiliary-reset.h
> >> new file mode 100644
> >> index 000000000000..8fdb02b18d8c
> >> --- /dev/null
> >> +++ b/include/soc/amlogic/meson-auxiliary-reset.h
> >> @@ -0,0 +1,23 @@
> >> +/* SPDX-License-Identifier: GPL-2.0 */
> >> +#ifndef __SOC_AMLOGIC_MESON_AUX_RESET_H
> >> +#define __SOC_AMLOGIC_MESON_AUX_RESET_H
> >> +
> >> +#include <linux/err.h>
> >> +
> >> +struct device;
> >> +struct regmap;
> >> +
> >> +#ifdef CONFIG_RESET_MESON
> >> +int devm_meson_rst_aux_register(struct device *dev,
> >> +                               struct regmap *map,
> >> +                               const char *adev_name);
> >> +#else
> >> +static inline int devm_meson_rst_aux_register(struct device *dev,
> >> +                                             struct regmap *map,
> >> +                                             const char *adev_name)
> >> +{
> >> +       return -EOPNOTSUPP;
> >
> > Shouldn't this be 'return 0' so that the clk driver doesn't have to care
> > about the config?
> 
> I don't think the system (in general) would be able function without the reset
> driver, so the question is rather phylosophical.
> 
> Let's say it could, if this returns 0, consumers of the reset controller
> will defer indefinitely ... which is always a bit more difficult to sort
> out.
> 
> If it returns an error, the problem is pretty obvious, helping people
> solve it quickly.
> 
> Also the actual device we trying to register provides clocks and reset.
> It is not like the reset is an optional part we don't care about.
> 
> On this instance it starts from clock, but it could have been the other
> way around. Both are equally important.
> 
> I'd prefer if it returns an error when the registration can't even start.
> 

Ok. Fair enough.