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