Re: [PATCH] regulator: core: Limit propagation of parent voltage count and list

[Matthias Kaehlcke <mka@chromium.org>]

El Mon, Mar 27, 2017 at 01:54:50PM -0400 Javier Martinez Canillas ha dit:

> On 03/27/2017 01:39 PM, Matthias Kaehlcke wrote:
>
> >>>> +		if (ops->get_voltage || ops->get_voltage_sel)
> >>
> >> It's valid to have a .get_voltage_sel callback without a .list_voltage?
> >>
> >> At least it seems that _regulator_get_voltage() assumes that having a
> >> .get_voltage_sel implies that a .list_voltage will also be available.
> >>
> >> static int _regulator_get_voltage(struct regulator_dev *rdev)
> >> {
> >> ...
> >> 	if (rdev->desc->ops->get_voltage_sel) {
> >> 		sel = rdev->desc->ops->get_voltage_sel(rdev);
> >> 		if (sel < 0)
> >> 			return sel;
> >> 		ret = rdev->desc->ops->list_voltage(rdev, sel);
> >> 	} else if (rdev->desc->ops->get_voltage) {
> >> ...
> >> }
> > 
> > The same function (from which I derived the conditions) suggests that
> > a regulator could have a .list_voltage op even if it doesn't have
> > .get_voltage_sel:
> > 
> >> ...
> >> if (rdev->desc->ops->get_voltage_sel) {
> >>   ...
> >> } else if (rdev->desc->ops->get_voltage) {
> >>   ...
> >> } else if (rdev->desc->ops->list_voltage) {
> > 
> > I don't know for sure if this condition is superfluous or if there are
> > cases where it makes sense to have a .list_voltage but not
> > .get_voltage_sel.
> >
> 
> I don't think is the same condition. Unless I'm misreading the code
> what it's checking is if there's a .list_voltage even when there is
> no .get_voltage_sel.
> 
> IOW, it's valid to have a .list_voltage even when there's no callback
> for .get_voltage_sel, but the opposite isn't true.

I see, thanks for the clarification.

> >> I wonder if instead of always checking if the regulator lacks operations,
> >> it wouldn't be better to do it just once and store that the regulator is
> >> a switch so that state can be used as explicit check for switch instead.
> >>
> >> Something like if (!rdev->supply || !rdev->switch) looks more clear
> >> to me.
> > 
> > I agree and we can even reduce it to if (!rdev_switch) since a switch
> > implicitly has a supply.
> >
> 
> I wonder if that's always true. What happens if you have a switch but
> its <name>-supply parent isn't defined in the Device Tree?

My idea was to only set rdev->switch after having resolved the
parent supply, though I concede this is not semantically. Maybe we
still want this logic but give the flag a different name?

Matthias