[PATCH v4 2/8] reset: Add reset controller API

[Philipp Zabel]

Hi Stephen,

Am Freitag, den 01.03.2013, 13:00 -0700 schrieb Stephen Warren:
> On 02/26/2013 04:39 AM, Philipp Zabel wrote:
> > This adds a simple API for devices to request being reset
> > by separate reset controller hardware and implements the
> > reset signal device tree binding.
> 
> > diff --git a/drivers/reset/core.c b/drivers/reset/core.c
> 
> > +int of_reset_simple_xlate(struct reset_controller_dev *rcdev,
> > +			  const struct of_phandle_args *reset_spec, u32 *flags)
> > +{
> > +	if (WARN_ON(reset_spec->args_count < rcdev->of_reset_n_cells))
> > +		return -EINVAL;
> 
> Would != make more sense than < ?

I copied this from of_gpio_simple_xlate, because the parsing will work
if args_count > of_reset_n_cells. In this case the device tree contains
a #reset-cells larger than what the reset controller driver expects.
Is this reason enough to assume the whole reset_spec is invalid?

> > +
> > +	if (reset_spec->args[0] >= rcdev->nr_resets)
> > +		return -EINVAL;
> > +
> > +	if (flags)
> > +		*flags = reset_spec->args[1];
> 
> if (flags)
>     if (reset_spec->args_count > 1)
>         *flags = reset_spec->args[1];
>     else
>         *flags = 0;
> 
> ?

In gpiolib, of_get_named_gpio_flags clears the flags, so that the xlate
implementations don't have to do it. The core doesn't use the flags, so
this is not a problem. I don't see the need for
of_get_named_reset_flags, since the reset consumer drivers shouldn't
care for those. Maybe it would be better to remove the flags parameter
from the xlate function altogether.

> > +struct reset_control *reset_control_get(struct device *dev, const char *id)
> ...
> > +	mutex_lock(&reset_controller_list_mutex);
> > +	rcdev = NULL;
> > +	list_for_each_entry(r, &reset_controller_list, list) {
> > +		if (args.np == r->of_node) {
> > +			rcdev = r;
> > +			break;
> > +		}
> > +	}
> > +	mutex_unlock(&reset_controller_list_mutex);
> 
> At this point, rcdev could be removed from that list, and perhaps even
> start to point at free'd memory.

I'll move the unlock down.

> > +	of_node_put(args.np);
> > +
> > +	if (!rcdev)
> > +		return ERR_PTR(-ENODEV);
> > +
> > +	rstc_id = rcdev->of_xlate(rcdev, &args, NULL);
> > +	if (rstc_id < 0)
> > +		return ERR_PTR(rstc_id);
> > +
> > +	try_module_get(rcdev->owner);
> 
> What about error-handling here?
>
> I think you want to drop reset_controller_list_mutex only after the call
> to try_module_get()?

I will.

> > +static int devm_reset_control_match(struct device *dev, void *res, void *data)
> > +{
> > +	struct reset_control **rstc = res;
> > +	if (!rstc || !*rstc) {
> > +		WARN_ON(!rstc || !*rstc);
> 
> I think you can if (WARN_ON(...)).

Yes.

> I'm not sure if the error-checks are quite right though;
> reset_control_get always returns an error-pointer for errors, never
> NULL, so the pointer can't ever be NULL. If it somehow was (e.g. client
> usage error), then that NULL pointer would never match anything, so the
> error-check still wouldn't be useful.

The void *res parameter of dr_match_t is given the data field of struct
devres (dr->data) when called by devres_release. A few subsystems do the
same check, so I figured there could be other devres managed resources
that could have dr->data == NULL. Looking at devres.c again, this
doesn't really seem to be the case. I'll drop it.

> I'm not sure why this is a ** here; below in devm_reset_control_put()
> you pass a just a *; are you expected to pass &rstc there instead?

dr->data is filled in and returned by devres_alloc in
devm_reset_control_get and contains a struct reset_control **ptr.
So that should be right, but ...

reset_control_get right now always allocates a new struct reset_control,
instead of keeping them around in a list and just returning a new
reference for already existing reset_controls, which kind of defeats the
purpose of the above. The idea was that drivers sharing a reset line
should also use the same struct reset_control.

thanks
Philipp