| Message ID | 20190613212553.10541-1-jeffrey.l.hugo@gmail.com |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | PM8005 and PMS405 regulator support | expand |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | success |
On Thu, Jun 13, 2019 at 02:25:53PM -0700, Jeffrey Hugo wrote: > +static int spmi_regulator_ftsmps426_set_voltage(struct regulator_dev *rdev, > + unsigned selector) > +{ > + struct spmi_regulator *vreg = rdev_get_drvdata(rdev); > + u8 buf[2]; > + int mV; > + > + mV = spmi_regulator_common_list_voltage(rdev, selector) / 1000; > + > + buf[0] = mV & 0xff; > + buf[1] = mV >> 8; > + return spmi_vreg_write(vreg, SPMI_FTSMPS426_REG_VOLTAGE_LSB, buf, 2); > +} This could just be a set_voltage_sel(), no need for it to be a set_voltage() operation.... > +static int spmi_regulator_ftsmps426_get_voltage(struct regulator_dev *rdev) > +{ > + struct spmi_regulator *vreg = rdev_get_drvdata(rdev); > + u8 buf[2]; > + > + spmi_vreg_read(vreg, SPMI_FTSMPS426_REG_VOLTAGE_LSB, buf, 2); > + > + return (((unsigned int)buf[1] << 8) | (unsigned int)buf[0]) * 1000; > +} ...or if the conversion is this trivial why do the list_voltage() lookup above? > +spmi_regulator_ftsmps426_set_mode(struct regulator_dev *rdev, unsigned int mode) > +{ > + struct spmi_regulator *vreg = rdev_get_drvdata(rdev); > + u8 mask = SPMI_FTSMPS426_MODE_MASK; > + u8 val; > + > + switch (mode) { > + case REGULATOR_MODE_NORMAL: > + val = SPMI_FTSMPS426_MODE_HPM_MASK; > + break; > + case REGULATOR_MODE_FAST: > + val = SPMI_FTSMPS426_MODE_AUTO_MASK; > + break; > + default: > + val = SPMI_FTSMPS426_MODE_LPM_MASK; > + break; > + } This should validate, it shouldn't just translate invalid values into valid ones.
On Mon, Jun 17, 2019 at 9:05 AM Mark Brown <broonie@kernel.org> wrote: > > On Thu, Jun 13, 2019 at 02:25:53PM -0700, Jeffrey Hugo wrote: > > > +static int spmi_regulator_ftsmps426_set_voltage(struct regulator_dev *rdev, > > + unsigned selector) > > +{ > > + struct spmi_regulator *vreg = rdev_get_drvdata(rdev); > > + u8 buf[2]; > > + int mV; > > + > > + mV = spmi_regulator_common_list_voltage(rdev, selector) / 1000; > > + > > + buf[0] = mV & 0xff; > > + buf[1] = mV >> 8; > > + return spmi_vreg_write(vreg, SPMI_FTSMPS426_REG_VOLTAGE_LSB, buf, 2); > > +} > > This could just be a set_voltage_sel(), no need for it to be a > set_voltage() operation.... This is a set_voltage_sel() in spmi_ftsmps426_ops. Is the issue because this function is "spmi_regulator_ftsmps426_set_voltage" and not "spmi_regulator_ftsmps426_set_voltage_sel"? > > > +static int spmi_regulator_ftsmps426_get_voltage(struct regulator_dev *rdev) > > +{ > > + struct spmi_regulator *vreg = rdev_get_drvdata(rdev); > > + u8 buf[2]; > > + > > + spmi_vreg_read(vreg, SPMI_FTSMPS426_REG_VOLTAGE_LSB, buf, 2); > > + > > + return (((unsigned int)buf[1] << 8) | (unsigned int)buf[0]) * 1000; > > +} > > ...or if the conversion is this trivial why do the list_voltage() lookup > above? We already have code in the driver to convert a selector to the voltage. Why duplicate that inline in spmi_regulator_ftsmps426_set_voltage? > > > +spmi_regulator_ftsmps426_set_mode(struct regulator_dev *rdev, unsigned int mode) > > +{ > > + struct spmi_regulator *vreg = rdev_get_drvdata(rdev); > > + u8 mask = SPMI_FTSMPS426_MODE_MASK; > > + u8 val; > > + > > + switch (mode) { > > + case REGULATOR_MODE_NORMAL: > > + val = SPMI_FTSMPS426_MODE_HPM_MASK; > > + break; > > + case REGULATOR_MODE_FAST: > > + val = SPMI_FTSMPS426_MODE_AUTO_MASK; > > + break; > > + default: > > + val = SPMI_FTSMPS426_MODE_LPM_MASK; > > + break; > > + } > > This should validate, it shouldn't just translate invalid values into > valid ones. Validate what? The other defines are REGULATOR_MODE_IDLE and REGULATOR_MODE_STANDBY which correspond to the LPM mode. Or are you suggesting that regulator framework is going to pass REGULATOR_MODE_INVALID to this operation?
On Mon, Jun 17, 2019 at 09:17:21AM -0600, Jeffrey Hugo wrote: > On Mon, Jun 17, 2019 at 9:05 AM Mark Brown <broonie@kernel.org> wrote: > > > +static int spmi_regulator_ftsmps426_set_voltage(struct regulator_dev *rdev, > > > + unsigned selector) > > > +{ > > > + mV = spmi_regulator_common_list_voltage(rdev, selector) / 1000; > > This could just be a set_voltage_sel(), no need for it to be a > > set_voltage() operation.... > This is a set_voltage_sel() in spmi_ftsmps426_ops. Is the issue because this > function is "spmi_regulator_ftsmps426_set_voltage" and not > "spmi_regulator_ftsmps426_set_voltage_sel"? Well, that's certainly confusing naming and there's some confusion in the code about what a selector is - it's supposed to be a raw register value so if you're having to convert it into a voltage something is going wrong. Just implement a set_voltage() operation? > We already have code in the driver to convert a selector to the > voltage. Why duplicate > that inline in spmi_regulator_ftsmps426_set_voltage? Either work with selectors or work with voltages, don't mix and match the two. > > > + switch (mode) { > > > + case REGULATOR_MODE_NORMAL: > > > + val = SPMI_FTSMPS426_MODE_HPM_MASK; > > > + break; > > > + case REGULATOR_MODE_FAST: > > > + val = SPMI_FTSMPS426_MODE_AUTO_MASK; > > > + break; > > > + default: > > > + val = SPMI_FTSMPS426_MODE_LPM_MASK; > > > + break; > > > + } > > This should validate, it shouldn't just translate invalid values into > > valid ones. > Validate what? The other defines are REGULATOR_MODE_IDLE > and REGULATOR_MODE_STANDBY which correspond to the LPM > mode. Or are you suggesting that regulator framework is going to pass > REGULATOR_MODE_INVALID to this operation? You should be validating that the argument passed in is one that the driver understands, your assumption will break if we add any new modes and in any case there should be a 1:1 mapping between hardware and API modes so you shouldn't be translating two different API modes into the same hardware mode.
On Mon, Jun 17, 2019 at 10:04 AM Mark Brown <broonie@kernel.org> wrote: > > On Mon, Jun 17, 2019 at 09:17:21AM -0600, Jeffrey Hugo wrote: > > On Mon, Jun 17, 2019 at 9:05 AM Mark Brown <broonie@kernel.org> wrote: > > > > > +static int spmi_regulator_ftsmps426_set_voltage(struct regulator_dev *rdev, > > > > + unsigned selector) > > > > +{ > > > > > + mV = spmi_regulator_common_list_voltage(rdev, selector) / 1000; > > > > This could just be a set_voltage_sel(), no need for it to be a > > > set_voltage() operation.... > > > This is a set_voltage_sel() in spmi_ftsmps426_ops. Is the issue because this > > function is "spmi_regulator_ftsmps426_set_voltage" and not > > "spmi_regulator_ftsmps426_set_voltage_sel"? > > Well, that's certainly confusing naming and there's some confusion in > the code about what a selector is - it's supposed to be a raw register > value so if you're having to convert it into a voltage something is > going wrong. Just implement a set_voltage() operation? No. Is what a selector is documented anywhere? I just looked again and I haven't found documentation explaining that a selector is the raw register value. Now I understand why this driver has the hardware to software selector translation. The selector being the raw register value seems to be a very limited assumption that I don't see working for more than very basic implementations. We've already figured out a virtualized selector mapping, I don't want to reimplement the complicated math to correctly map a requested voltage range to what the regulator can provide, and possibly get it wrong, or at the very least have two duplicate implementations. The naming is consistent with the rest of the driver, and the name seems long enough already. Lets just keep this. > > > We already have code in the driver to convert a selector to the > > voltage. Why duplicate > > that inline in spmi_regulator_ftsmps426_set_voltage? > > Either work with selectors or work with voltages, don't mix and match > the two. Fine. I'll fix up the get() to return the selector, and not the voltage since that works better with everything else that is implemented. Again, it would be nice if the documentation for regulator_ops indicated that a driver should only implement the voltage operations or the selector operations, not mix and match if that is your expectation. > > > > > + switch (mode) { > > > > + case REGULATOR_MODE_NORMAL: > > > > + val = SPMI_FTSMPS426_MODE_HPM_MASK; > > > > + break; > > > > + case REGULATOR_MODE_FAST: > > > > + val = SPMI_FTSMPS426_MODE_AUTO_MASK; > > > > + break; > > > > + default: > > > > + val = SPMI_FTSMPS426_MODE_LPM_MASK; > > > > + break; > > > > + } > > > > This should validate, it shouldn't just translate invalid values into > > > valid ones. > > > Validate what? The other defines are REGULATOR_MODE_IDLE > > and REGULATOR_MODE_STANDBY which correspond to the LPM > > mode. Or are you suggesting that regulator framework is going to pass > > REGULATOR_MODE_INVALID to this operation? > > You should be validating that the argument passed in is one that the > driver understands, your assumption will break if we add any new modes > and in any case there should be a 1:1 mapping between hardware and API > modes so you shouldn't be translating two different API modes into the > same hardware mode. Fine. I'll fix this per what you've stated. Again, would be nice if the documentation for the API modes clearly indicated they should match to one specific HW setting incases where the HW doesn't match the API 1:1.
On Mon, Jun 17, 2019 at 11:07:18AM -0600, Jeffrey Hugo wrote: > On Mon, Jun 17, 2019 at 10:04 AM Mark Brown <broonie@kernel.org> wrote: > > On Mon, Jun 17, 2019 at 09:17:21AM -0600, Jeffrey Hugo wrote: Something really weird is going on with the word wrapping in your mail, it looks like you're writing lines longer than 80 characters (120?) and they're getting a newline added in the middle of the line to wrap them without reflowing the paragraph. > > > This is a set_voltage_sel() in spmi_ftsmps426_ops. Is the issue because this > > > function is "spmi_regulator_ftsmps426_set_voltage" and not > > > "spmi_regulator_ftsmps426_set_voltage_sel"? > > Well, that's certainly confusing naming and there's some confusion in > > the code about what a selector is - it's supposed to be a raw register > > value so if you're having to convert it into a voltage something is > > going wrong. Just implement a set_voltage() operation? > No. > Is what a selector is documented anywhere? I just looked again and I > haven't found > documentation explaining that a selector is the raw register value. Well, it doesn't *have* to be the raw register value, more accurately it's some token which is useful for passing to and from the hardware; The documentation such as it is is in the documentation of the list_voltage() operation (which is what defines the selector values for a given driver). > Now I understand why this driver has the hardware to software selector > translation. > The selector being the raw register value seems to be a very limited > assumption that > I don't see working for more than very basic implementations. Your idea of very basic implementations is how the overwhelming majority of hardware is implemented. Regulators with register maps will tend to just have a bitfield where you set the voltage with each valid value in that bitfield mapped to a single voltage, the exceptions tend to use direct voltage values instead (and not support enumeration at all). Looking at the driver I think it's got what the helpers are terming pickable linear ranges (naming is hard) and could use those helpers. > We've already figured out a virtualized selector mapping, I don't want > to reimplement > the complicated math to correctly map a requested voltage range to > what the regulator > can provide, and possibly get it wrong, or at the very least have two duplicate > implementations. I don't know what a "virtualized selector mapping" is... We do have an extensive range of helper implementations which cover the majority of cases, are you sure that none of these cover what the hardware is doing? > The naming is consistent with the rest of the driver, and the name > seems long enough > already. Lets just keep this. I appreciate that the existing code is a bit of a mess but as you can see it's making it difficult to maintain so I'd rather push towards making it cleaner, it looks like we're getting more and more devices added to the driver so it's actually an issue. > Again, it would be nice if the documentation for regulator_ops > indicated that a driver > should only implement the voltage operations or the selector > operations, not mix and > match if that is your expectation. Please add whatever documentation you're looking for here. I genuinely don't know where people would look for that and TBH I'm having a hard time figuring out why you'd implement the operations asymmetrically. It's not an issue that ever comes up.
On Mon, Jun 17, 2019 at 1:24 PM Mark Brown <broonie@kernel.org> wrote: > > On Mon, Jun 17, 2019 at 01:06:42PM -0600, Jeffrey Hugo wrote: > > On Mon, Jun 17, 2019 at 12:37 PM Mark Brown <broonie@kernel.org> wrote: > > > > Something really weird is going on with the word wrapping in your mail, > > > it looks like you're writing lines longer than 80 characters (120?) and > > > they're getting a newline added in the middle of the line to wrap them > > > without reflowing the paragraph. > > > Doh. Hopefully this one is better. > > Yes, thanks! Though now it's off-list :/ Doh. Added everyone back. > > > > Well, it doesn't *have* to be the raw register value, more accurately > > > it's some token which is useful for passing to and from the hardware; > > > The documentation such as it > > > is is in the documentation of the list_voltage() operation (which is > > > what defines the selector values for a given driver). > > > Ok, so this one bit - > > Selectors range from zero to one less than regulator_desc.n_voltages. > > Voltages may be reported in any order. > > Yes. > > > So, I understand that. Its an indexing of the supported voltages. > > From my perspective, that has nothing to do with hardware. Its just a > > remapping of the values to a different set. Voltages X, Y, and Z map > > to 0, 1, and 2. Its a token so that the driver and the framework can > > use a common value. > > > I really think we are on the same page here, its just I was getting > > confused by how you were describing it in your replies. > > Yes, I was just coming from the perspective that for almost all hardware > the selectors are chosen to be the values that are in the bitfield that > the hardware uses to specify the voltage since that's the most common > thing and tends to make things simpler for people, it's also the primary > place where the concept came from. > > > > Your idea of very basic implementations is how the overwhelming majority > > > of hardware is implemented. Regulators with register maps will tend to > > > just have a bitfield where you set the voltage with each valid value in > > > that bitfield mapped to a single voltage, the exceptions tend to use > > > direct voltage values instead (and not support enumeration at all). > > > > Looking at the driver I think it's got what the helpers are terming > > > pickable linear ranges (naming is hard) and could use those helpers. > > > I'm pretty sure Stephen Boyd and Bjorn just investigated that a few > > weeks ago, and came to the conclusion that it can't because of things > > like the hardware really wants to stay in the same range if at all > > possible, which is not behavior the pickable linear ranges seems to > > support. > > Sounds like it just needs a custom map_voltage() function? Though > thinking about it it's possibly worth just making the standard map try > to keep things in the same range as that will if nothing else reduce the > number of I/O operations. Probably also reduce glitches if there's > overlapping ranges. I didn't think so when I was paying attention to their discussion. However maybe I missed something. I think I'll take another look. Might make for a nice cleanup, but if its just in the driver, or if it involves updating the framework, it seems to be outside scope for this series of changes.
diff --git a/Documentation/devicetree/bindings/regulator/qcom,spmi-regulator.txt b/Documentation/devicetree/bindings/regulator/qcom,spmi-regulator.txt index 406f2e570c50..ba94bc2d407a 100644 --- a/Documentation/devicetree/bindings/regulator/qcom,spmi-regulator.txt +++ b/Documentation/devicetree/bindings/regulator/qcom,spmi-regulator.txt @@ -4,6 +4,7 @@ Qualcomm SPMI Regulators Usage: required Value type: <string> Definition: must be one of: + "qcom,pm8005-regulators" "qcom,pm8841-regulators" "qcom,pm8916-regulators" "qcom,pm8941-regulators" @@ -120,6 +121,9 @@ The regulator node houses sub-nodes for each regulator within the device. Each sub-node is identified using the node's name, with valid values listed for each of the PMICs below. +pm8005: + s1, s2, s3, s4 + pm8841: s1, s2, s3, s4, s5, s6, s7, s8