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

[Matthias Kaehlcke <mka@chromium.org>]

Thanks for the reviews and testing!

El Sat, Mar 25, 2017 at 02:05:47AM -0300 Javier Martinez Canillas ha dit:

 On 03/24/2017 05:38 PM, Brian Norris wrote:
> > On Fri, Mar 24, 2017 at 01:09:52PM -0700, Matthias Kaehlcke wrote:
> >> diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
> >> index 53d4fc70dbd0..121838e0125b 100644
> >> --- a/drivers/regulator/core.c
> >> +++ b/drivers/regulator/core.c
> >> @@ -2487,6 +2487,10 @@ static int _regulator_list_voltage(struct regulator *regulator,
> >>  		if (lock)
> >>  			mutex_unlock(&rdev->mutex);
> >>  	} else if (rdev->supply) {
> >> +		// Limit propagation of parent values to switch regulators
> > 
> > The kernel doesn't use C99 comments. Oddly enough, this isn't actually
> 
> +1

Will fix

> > in the coding style doc (Documentation/process/coding-style.rst), nor is
> > it caught by scripts/checkpatch.pl (even though it clearly has a 'C99
> > comment' rule).
> > 
> >> +		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 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'll send out a new version soon.

Matthias