Hello Matthias, On 03/27/2017 01:39 PM, Matthias Kaehlcke wrote: > Thanks for the reviews and testing! >
You are welcome. [snip] >>>> + 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 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? > I'll send out a new version soon. > > Matthias > Best regards, -- Javier Martinez Canillas Open Source Group Samsung Research America