| Message ID | 1548675904-18324-3-git-send-email-jorge.ramirez-ortiz@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | qcom: add PMS405 SPMI regulator | expand |
On Mon, Jan 28, 2019 at 12:45:03PM +0100, Jorge Ramirez-Ortiz wrote: > @@ -653,6 +708,10 @@ spmi_regulator_find_range(struct spmi_regulator *vreg) > range = vreg->set_points->range; > end = range + vreg->set_points->count; > > + /* we know we only have one range for this type */ > + if (vreg->logical_type == SPMI_REGULATOR_LOGICAL_TYPE_HFS430) > + return range; > + > spmi_vreg_read(vreg, SPMI_COMMON_REG_VOLTAGE_RANGE, &range_sel, 1); > > for (; range < end; range++) Rather than have special casing for the logical type in here it seems better to just provide a specific op for this logical type, you could always make _find_range() call into that one if you really want code reuse here. You already have separate ops for this regulator type anyway. > +static unsigned int spmi_regulator_hfs430_get_mode(struct regulator_dev *rdev) > +{ > + struct spmi_regulator *vreg = rdev_get_drvdata(rdev); > + u8 reg; > + int ret; > + > + ret = spmi_vreg_read(vreg, SPMI_HFS430_REG_MODE, ®, 1); > + if (ret) { > + dev_err(&rdev->dev, "failed to get mode"); > + return ret; > + } > + > + if (reg == SPMI_HFS430_MODE_PWM) > + return REGULATOR_MODE_NORMAL; > + > + return REGULATOR_MODE_IDLE; > +} I'd have expected a switch statement here, ideally flagging a warning or error if we get a surprising value in there. > +static int spmi_regulator_hfs430_set_mode(struct regulator_dev *rdev, > + unsigned int mode) > +{ > + struct spmi_regulator *vreg = rdev_get_drvdata(rdev); > + u8 reg = mode == REGULATOR_MODE_NORMAL ? SPMI_HFS430_MODE_PWM : > + SPMI_HFS430_MODE_AUTO; Please write a normal if statement (or switch statement) to help legibility.
On 2/4/19 10:03, Mark Brown wrote: > On Mon, Jan 28, 2019 at 12:45:03PM +0100, Jorge Ramirez-Ortiz wrote: > >> @@ -653,6 +708,10 @@ spmi_regulator_find_range(struct spmi_regulator *vreg) >> range = vreg->set_points->range; >> end = range + vreg->set_points->count; >> >> + /* we know we only have one range for this type */ >> + if (vreg->logical_type == SPMI_REGULATOR_LOGICAL_TYPE_HFS430) >> + return range; >> + >> spmi_vreg_read(vreg, SPMI_COMMON_REG_VOLTAGE_RANGE, &range_sel, 1); >> >> for (; range < end; range++) > > Rather than have special casing for the logical type in here it seems > better to just provide a specific op for this logical type, you could > always make _find_range() call into that one if you really want code > reuse here. You already have separate ops for this regulator type > anyway. sorry I dont quite understand your point. static struct regulator_ops spmi_hfs430_ops = { /* always on regulators */ .set_voltage_sel = spmi_regulator_hfs430_set_voltage, * .set_voltage_time_sel = spmi_regulator_set_voltage_time_sel, * .get_voltage = spmi_regulator_hfs430_get_voltage, .map_voltage = spmi_regulator_common_map_voltage, * .list_voltage = spmi_regulator_common_list_voltage, .get_mode = spmi_regulator_hfs430_get_mode, .set_mode = spmi_regulator_hfs430_set_mode, }; find_range affects the functions above with * You are right and I can easily adjust the private spmi_regulator_hfs430_set_voltage. And since it is quite small I can also _duplicate_ the common function spmi_regulator_set_voltage_time_sel with a small change for hfs430. But spmi_regulator_common_map_voltage ends up being a large function and I dont see the point in replicating it to save the "if" statement above. why cant different logical_types extend spmi_regulator_find_range(..)? Or maybe are you saying that I should add a new interface to struct spmi_regulator that implements priv_find_range(..) for the logical types that dont want to use the common implementation? But also I am not sure I see the benefits with respect to the proposed change... > >> +static unsigned int spmi_regulator_hfs430_get_mode(struct regulator_dev *rdev) >> +{ >> + struct spmi_regulator *vreg = rdev_get_drvdata(rdev); >> + u8 reg; >> + int ret; >> + >> + ret = spmi_vreg_read(vreg, SPMI_HFS430_REG_MODE, ®, 1); >> + if (ret) { >> + dev_err(&rdev->dev, "failed to get mode"); >> + return ret; >> + } >> + >> + if (reg == SPMI_HFS430_MODE_PWM) >> + return REGULATOR_MODE_NORMAL; >> + >> + return REGULATOR_MODE_IDLE; >> +} > > I'd have expected a switch statement here, ideally flagging a warning or > error if we get a surprising value in there. this implementation follows what the common function spmi_regulator_common_get_mode implements (ie, checks for a case and defaults if that is not the one; and when defaulting, there is no reporting that it is actually defaulting: ie, defaulting is not being interpreted as an error..should it?) > >> +static int spmi_regulator_hfs430_set_mode(struct regulator_dev *rdev, >> + unsigned int mode) >> +{ >> + struct spmi_regulator *vreg = rdev_get_drvdata(rdev); >> + u8 reg = mode == REGULATOR_MODE_NORMAL ? SPMI_HFS430_MODE_PWM : >> + SPMI_HFS430_MODE_AUTO; > > Please write a normal if statement (or switch statement) to help > legibility. > ok.
On Fri, Apr 19, 2019 at 07:29:48PM +0200, Jorge Ramirez wrote: > On 2/4/19 10:03, Mark Brown wrote: > >> + /* we know we only have one range for this type */ > >> + if (vreg->logical_type == SPMI_REGULATOR_LOGICAL_TYPE_HFS430) > >> + return range; > > Rather than have special casing for the logical type in here it seems > > better to just provide a specific op for this logical type, you could > > always make _find_range() call into that one if you really want code > > reuse here. You already have separate ops for this regulator type > > anyway. > sorry I dont quite understand your point. If you need to skip the majority of the contents of the function for some regulators just define a separate function for those regulators and give them different ops structures rather than using the same ops structure and handling it in the functions. > But also I am not sure I see the benefits with respect to the proposed > change... The benefit is that the selection of which operations to use is done in only one place (the selection of the ops structure) rather than in multiple places (the selection of the ops structure and the contents of the operations).
On 4/25/19 20:37, Mark Brown wrote: > On Fri, Apr 19, 2019 at 07:29:48PM +0200, Jorge Ramirez wrote: >> On 2/4/19 10:03, Mark Brown wrote: > >>>> + /* we know we only have one range for this type */ >>>> + if (vreg->logical_type == SPMI_REGULATOR_LOGICAL_TYPE_HFS430) >>>> + return range; > >>> Rather than have special casing for the logical type in here it seems >>> better to just provide a specific op for this logical type, you could >>> always make _find_range() call into that one if you really want code >>> reuse here. You already have separate ops for this regulator type >>> anyway. > >> sorry I dont quite understand your point. > > If you need to skip the majority of the contents of the function for > some regulators just define a separate function for those regulators and > give them different ops structures rather than using the same ops > structure and handling it in the functions. sure this is 101 as a general rule: but this is not applicable to the situation that I described in my original note, so I dont think you read my points. > >> But also I am not sure I see the benefits with respect to the proposed >> change... > > The benefit is that the selection of which operations to use is done in > only one place (the selection of the ops structure) rather than in > multiple places (the selection of the ops structure and the contents of > the operations). all right, how do you propose that we handle spmi_regulator_select_voltage_same_range() then? the way I see it, if I follow your suggestion and since we are not allowed to extend spmi_regulator_find_range(), the only options are: 1) duplicate verbatim this whole function (spmi_regulator_select_voltage_same_range) with a minor change (this amount of code duplication in the kernel seems rather unnecessary to me) 2) modify the struct spmi_regulator definition with a new operation that calls a different implementation of find range (seems a massive overkill) because both seem wrong to me, can you confirm that you are ok with one of those two options? or if not give an alternative? But I still would like to understand why you think it is wrong extending spmi_regulator_find_range() to support HFS430. Are you saying that this function should not exist for this regulator? Sure the HFS430 doesnt use the register SPMI_COMMON_REG_VOLTAGE_RANGE and therefore doesnt need to use it to find its range....but that doesnt mean that the semantics of spmi_regulator_find_range are invalid. The way I understand your concern, you seem to be assuming that spmi_regulator_find_range means something like spmi_regulator_find_range_from_reg_voltage but that is not the case or if it is maybe it should be renamed.....
On Thu, Apr 25, 2019 at 09:44:00PM +0200, Jorge Ramirez wrote: > the way I see it, if I follow your suggestion and since we are not > allowed to extend spmi_regulator_find_range(), the only options are: > 1) duplicate verbatim this whole function > (spmi_regulator_select_voltage_same_range) with a minor change (this > amount of code duplication in the kernel seems rather unnecessary to me) > 2) modify the struct spmi_regulator definition with a new operation that > calls a different implementation of find range (seems a massive overkill) Since the point of this change is AFAICT that this regulator only has a single linear range it seems like it should just be able to use the existing generic functions shouldn't it? It just needs it's own set/get_voltage_sel() operations. As far as I can see the main thing the driver is doing with the custom stuff is handling the fact that there's multiple ranges but that's not an issue for this regulator. It's possible I'm missing something there but that was the main thing (and we do have some generic support for multiple linear ranges in the helper code already, can't remember why this driver isn't using that - the ranges overlap IIRC?). TBH looking at the uses of find_range() I'm not sure they're 100% sensible as they are - the existing _time_sel() is assuming we only need to work out the ramp time between voltages in the same range which is going to have trouble.
On 4/27/19 20:21, Mark Brown wrote: > On Thu, Apr 25, 2019 at 09:44:00PM +0200, Jorge Ramirez wrote: > >> the way I see it, if I follow your suggestion and since we are not >> allowed to extend spmi_regulator_find_range(), the only options are: > >> 1) duplicate verbatim this whole function >> (spmi_regulator_select_voltage_same_range) with a minor change (this >> amount of code duplication in the kernel seems rather unnecessary to me) > >> 2) modify the struct spmi_regulator definition with a new operation that >> calls a different implementation of find range (seems a massive overkill) > > Since the point of this change is AFAICT that this regulator only has a > single linear range it seems like it should just be able to use the > existing generic functions shouldn't it? yes that would have been ideal but it does not seem to be the case for this hardware. The register that stores the voltage range for all other SPMI regulators (SPMI_COMMON_REG_VOLTAGE_RANGE 0x40) is used by something else in the HFS430: SPMI_HFS430_REG_VOLTAGE_LB 0x40 stores the voltage level in two bytes 0x40 and 0x41; This overlap really what is creating the pain: HFS430 cant use 0x40 to store the range (even if it is only one) so yeah, most of the changes in the patch are working around this fact. enum spmi_common_regulator_registers { SPMI_COMMON_REG_DIG_MAJOR_REV = 0x01, SPMI_COMMON_REG_TYPE = 0x04, SPMI_COMMON_REG_SUBTYPE = 0x05, SPMI_COMMON_REG_VOLTAGE_RANGE = 0x40, ****** SPMI_COMMON_REG_VOLTAGE_SET = 0x41, SPMI_COMMON_REG_MODE = 0x45, SPMI_COMMON_REG_ENABLE = 0x46, SPMI_COMMON_REG_PULL_DOWN = 0x48, SPMI_COMMON_REG_SOFT_START = 0x4c, SPMI_COMMON_REG_STEP_CTRL = 0x61, }; enum spmi_hfs430_registers { SPMI_HFS430_REG_VOLTAGE_LB = 0x40, ******* SPMI_HFS430_REG_VOLTAGE_VALID_LB = 0x42, SPMI_HFS430_REG_MODE = 0x45, }; It just needs it's own > set/get_voltage_sel() operations. As far as I can see the main thing > the driver is doing with the custom stuff is handling the fact that > there's multiple ranges but that's not an issue for this regulator. > It's possible I'm missing something there but that was the main thing > (and we do have some generic support for multiple linear ranges in the > helper code already, can't remember why this driver isn't using that - > the ranges overlap IIRC?). > > TBH looking at the uses of find_range() I'm not sure they're 100% > sensible as they are - the existing _time_sel() is assuming we only need > to work out the ramp time between voltages in the same range which is > going to have trouble. >
On Mon, Apr 29, 2019 at 02:31:55PM +0200, Jorge Ramirez wrote: > On 4/27/19 20:21, Mark Brown wrote: > > Since the point of this change is AFAICT that this regulator only has a > > single linear range it seems like it should just be able to use the > > existing generic functions shouldn't it? > yes that would have been ideal but it does not seem to be the case for > this hardware. > The register that stores the voltage range for all other SPMI regulators > (SPMI_COMMON_REG_VOLTAGE_RANGE 0x40) is used by something else in the > HFS430: SPMI_HFS430_REG_VOLTAGE_LB 0x40 stores the voltage level in two > bytes 0x40 and 0x41; > This overlap really what is creating the pain: HFS430 cant use 0x40 to > store the range (even if it is only one) > so yeah, most of the changes in the patch are working around this fact. I'm not sure I follow here, sorry - I can see that the driver needs a custom get/set selector operation but shouldn't it be able to use the standard list and map operations for linear ranges? > > enum spmi_common_regulator_registers { > SPMI_COMMON_REG_DIG_MAJOR_REV = 0x01, > SPMI_COMMON_REG_TYPE = 0x04, > SPMI_COMMON_REG_SUBTYPE = 0x05, > SPMI_COMMON_REG_VOLTAGE_RANGE = 0x40, ****** > SPMI_COMMON_REG_VOLTAGE_SET = 0x41, > SPMI_COMMON_REG_MODE = 0x45, > SPMI_COMMON_REG_ENABLE = 0x46, > SPMI_COMMON_REG_PULL_DOWN = 0x48, > SPMI_COMMON_REG_SOFT_START = 0x4c, > SPMI_COMMON_REG_STEP_CTRL = 0x61, > }; > > enum spmi_hfs430_registers { > SPMI_HFS430_REG_VOLTAGE_LB = 0x40, ******* > SPMI_HFS430_REG_VOLTAGE_VALID_LB = 0x42, > SPMI_HFS430_REG_MODE = 0x45, > }; > > It just needs it's own > > set/get_voltage_sel() operations. As far as I can see the main thing > > the driver is doing with the custom stuff is handling the fact that > > there's multiple ranges but that's not an issue for this regulator. > > It's possible I'm missing something there but that was the main thing > > (and we do have some generic support for multiple linear ranges in the > > helper code already, can't remember why this driver isn't using that - > > the ranges overlap IIRC?). > > > > TBH looking at the uses of find_range() I'm not sure they're 100% > > sensible as they are - the existing _time_sel() is assuming we only need > > to work out the ramp time between voltages in the same range which is > > going to have trouble. > > >
On 5/2/19 04:33, Mark Brown wrote: > On Mon, Apr 29, 2019 at 02:31:55PM +0200, Jorge Ramirez wrote: >> On 4/27/19 20:21, Mark Brown wrote: > >>> Since the point of this change is AFAICT that this regulator only has a >>> single linear range it seems like it should just be able to use the >>> existing generic functions shouldn't it? > >> yes that would have been ideal but it does not seem to be the case for >> this hardware. > >> The register that stores the voltage range for all other SPMI regulators >> (SPMI_COMMON_REG_VOLTAGE_RANGE 0x40) is used by something else in the >> HFS430: SPMI_HFS430_REG_VOLTAGE_LB 0x40 stores the voltage level in two >> bytes 0x40 and 0x41; > >> This overlap really what is creating the pain: HFS430 cant use 0x40 to >> store the range (even if it is only one) > >> so yeah, most of the changes in the patch are working around this fact. > > I'm not sure I follow here, sorry - I can see that the driver needs a > custom get/set selector operation but shouldn't it be able to use the > standard list and map operations for linear ranges? I agree it should, but unfortunately that is not the case; when I first posted the patch I was concerned that for a regulator to be supported by this driver it should obey to the driver's internals (ie: comply with all of the spmi_common_regulator_registers definitions). However, since there was just a single range to support, the modifications I had to do to support this SPMI regulator were minimal - hence why I opted for the changes under discussion instead of writing a new driver (which IMO it is an overkill). what do you think? > >> >> enum spmi_common_regulator_registers { >> SPMI_COMMON_REG_DIG_MAJOR_REV = 0x01, >> SPMI_COMMON_REG_TYPE = 0x04, >> SPMI_COMMON_REG_SUBTYPE = 0x05, >> SPMI_COMMON_REG_VOLTAGE_RANGE = 0x40, ****** >> SPMI_COMMON_REG_VOLTAGE_SET = 0x41, >> SPMI_COMMON_REG_MODE = 0x45, >> SPMI_COMMON_REG_ENABLE = 0x46, >> SPMI_COMMON_REG_PULL_DOWN = 0x48, >> SPMI_COMMON_REG_SOFT_START = 0x4c, >> SPMI_COMMON_REG_STEP_CTRL = 0x61, >> }; >> >> enum spmi_hfs430_registers { >> SPMI_HFS430_REG_VOLTAGE_LB = 0x40, ******* >> SPMI_HFS430_REG_VOLTAGE_VALID_LB = 0x42, ah, this definition I can remove and use the common one above. I'll do that. >> SPMI_HFS430_REG_MODE = 0x45, >> }; >> >> It just needs it's own >>> set/get_voltage_sel() operations. As far as I can see the main thing >>> the driver is doing with the custom stuff is handling the fact that >>> there's multiple ranges but that's not an issue for this regulator. >>> It's possible I'm missing something there but that was the main thing >>> (and we do have some generic support for multiple linear ranges in the >>> helper code already, can't remember why this driver isn't using that - >>> the ranges overlap IIRC?). >>> >>> TBH looking at the uses of find_range() I'm not sure they're 100% >>> sensible as they are - the existing _time_sel() is assuming we only need >>> to work out the ramp time between voltages in the same range which is >>> going to have trouble. >>> >>
On Thu, May 02, 2019 at 01:30:48PM +0200, Jorge Ramirez wrote: > On 5/2/19 04:33, Mark Brown wrote: > > I'm not sure I follow here, sorry - I can see that the driver needs a > > custom get/set selector operation but shouldn't it be able to use the > > standard list and map operations for linear ranges? > I agree it should, but unfortunately that is not the case; when I first > posted the patch I was concerned that for a regulator to be supported by > this driver it should obey to the driver's internals (ie: comply with > all of the spmi_common_regulator_registers definitions). That's not a requirement that I'd particularly expect - it's not unusual for devices to have multiple different styles of regulators in a single chip (eg, DCDCs often have quite different register maps to LDOs). > However, since there was just a single range to support, the > modifications I had to do to support this SPMI regulator were minimal - > hence why I opted for the changes under discussion instead of writing a > new driver (which IMO it is an overkill). > what do you think? It seems a bit of a jump to add a new driver - it's just another descriptor and ops structure isn't it? Though as ever with the Qualcomm stuff this driver is pretty baroque which doesn't entirely help though I think it's just another regulator type which there's already some handling for.
On 5/3/19 08:26, Mark Brown wrote: > On Thu, May 02, 2019 at 01:30:48PM +0200, Jorge Ramirez wrote: >> On 5/2/19 04:33, Mark Brown wrote: > >>> I'm not sure I follow here, sorry - I can see that the driver needs a >>> custom get/set selector operation but shouldn't it be able to use the >>> standard list and map operations for linear ranges? > >> I agree it should, but unfortunately that is not the case; when I first >> posted the patch I was concerned that for a regulator to be supported by >> this driver it should obey to the driver's internals (ie: comply with >> all of the spmi_common_regulator_registers definitions). > > That's not a requirement that I'd particularly expect - it's not unusual > for devices to have multiple different styles of regulators in a single > chip (eg, DCDCs often have quite different register maps to LDOs). > >> However, since there was just a single range to support, the >> modifications I had to do to support this SPMI regulator were minimal - >> hence why I opted for the changes under discussion instead of writing a >> new driver (which IMO it is an overkill). > >> what do you think? > > It seems a bit of a jump to add a new driver - it's just another > descriptor and ops structure isn't it? Though as ever with the Qualcomm > stuff this driver is pretty baroque which doesn't entirely help though I > think it's just another regulator type which there's already some > handling for. > So how do we move this forward? To sum up his regulator needs to be able to bypass accesses to SPMI_COMMON_REG_VOLTAGE_RANGE and provide the range in some other way hence the change below I can't find a simpler solution than this since the function does now what is supposed to do for all the regulator types supported in the driver @@ -653,6 +708,10 @@ spmi_regulator_find_range(struct spmi_regulator *vreg) range = vreg->set_points->range; end = range + vreg->set_points->count; + /* we know we only have one range for this type */ + if (vreg->logical_type == SPMI_REGULATOR_LOGICAL_TYPE_HFS430) + return range; + spmi_vreg_read(vreg, SPMI_COMMON_REG_VOLTAGE_RANGE, &range_sel, 1); for (; range < end; range++)
On Fri, May 03, 2019 at 10:29:42AM +0200, Jorge Ramirez wrote: > On 5/3/19 08:26, Mark Brown wrote: > > On Thu, May 02, 2019 at 01:30:48PM +0200, Jorge Ramirez wrote: > > It seems a bit of a jump to add a new driver - it's just another > > descriptor and ops structure isn't it? Though as ever with the Qualcomm > > stuff this driver is pretty baroque which doesn't entirely help though I > > think it's just another regulator type which there's already some > > handling for. > So how do we move this forward? > To sum up his regulator needs to be able to bypass accesses to > SPMI_COMMON_REG_VOLTAGE_RANGE and provide the range in some other way > hence the change below > I can't find a simpler solution than this since the function does now > what is supposed to do for all the regulator types supported in the driver The assumption that you need to have this regulator use functions that use and provide ranges is the very thing I'm trying to get you to change. It looks like these regulators just need their own set_voltage_sel() and get_voltage_sel() then they can use the standard linear range mapping functions (and pobably the set_voltage_time_sel() needs fixing anyway for all the other regulators). There's already some conditional code in the probe function for handling different operations for the over current protection and SAW stuff, this looks like it should fit in reasonably well. Usually this would be even easier as probe functions are just data driven but for some reason more than usual of this driver's data initializaiton is done dynamically.
On 5/6/19 06:38, Mark Brown wrote: > On Fri, May 03, 2019 at 10:29:42AM +0200, Jorge Ramirez wrote: >> On 5/3/19 08:26, Mark Brown wrote: >>> On Thu, May 02, 2019 at 01:30:48PM +0200, Jorge Ramirez wrote: > >>> It seems a bit of a jump to add a new driver - it's just another >>> descriptor and ops structure isn't it? Though as ever with the Qualcomm >>> stuff this driver is pretty baroque which doesn't entirely help though I >>> think it's just another regulator type which there's already some >>> handling for. > >> So how do we move this forward? > >> To sum up his regulator needs to be able to bypass accesses to >> SPMI_COMMON_REG_VOLTAGE_RANGE and provide the range in some other way >> hence the change below > >> I can't find a simpler solution than this since the function does now >> what is supposed to do for all the regulator types supported in the driver > > The assumption that you need to have this regulator use functions that > use and provide ranges is the very thing I'm trying to get you to > change. It looks like these regulators just need their own > set_voltage_sel() and get_voltage_sel() then they can use the standard > linear range mapping functions (and pobably the set_voltage_time_sel() > needs fixing anyway for all the other regulators). Right, and I understand what you are asking, is just that I completely disagree with you. But moving on. Would you accept if I wrote a separate driver specific to pms405 or do you want me to integrate in qcom-spmi_regulator.c? I am asking because none of the ops will use the common functions (I wont be reusing much code from this qcom-spmi_regulator.c file) > > There's already some conditional code in the probe function for handling > different operations for the over current protection and SAW stuff, this > looks like it should fit in reasonably well. Usually this would be even > easier as probe functions are just data driven but for some reason more > than usual of this driver's data initializaiton is done dynamically. >
On Thu, May 23, 2019 at 10:35:46AM +0200, Jorge Ramirez wrote: > Would you accept if I wrote a separate driver specific to pms405 or do > you want me to integrate in qcom-spmi_regulator.c? > I am asking because none of the ops will use the common functions (I > wont be reusing much code from this qcom-spmi_regulator.c file) I don't really mind, if there's nothing really shared then making it a separate driver is probably best but it's not a strong opinion either way.
diff --git a/drivers/regulator/qcom_spmi-regulator.c b/drivers/regulator/qcom_spmi-regulator.c index 53a61fb..6b8dc9c 100644 --- a/drivers/regulator/qcom_spmi-regulator.c +++ b/drivers/regulator/qcom_spmi-regulator.c @@ -99,6 +99,7 @@ enum spmi_regulator_logical_type { SPMI_REGULATOR_LOGICAL_TYPE_VS, SPMI_REGULATOR_LOGICAL_TYPE_BOOST, SPMI_REGULATOR_LOGICAL_TYPE_FTSMPS, + SPMI_REGULATOR_LOGICAL_TYPE_HFS430, SPMI_REGULATOR_LOGICAL_TYPE_BOOST_BYP, SPMI_REGULATOR_LOGICAL_TYPE_LN_LDO, SPMI_REGULATOR_LOGICAL_TYPE_ULT_LO_SMPS, @@ -120,6 +121,7 @@ enum spmi_regulator_type { enum spmi_regulator_subtype { SPMI_REGULATOR_SUBTYPE_GP_CTL = 0x08, SPMI_REGULATOR_SUBTYPE_RF_CTL = 0x09, + SPMI_REGULATOR_SUBTYPE_HFS430 = 0x0a, SPMI_REGULATOR_SUBTYPE_N50 = 0x01, SPMI_REGULATOR_SUBTYPE_N150 = 0x02, SPMI_REGULATOR_SUBTYPE_N300 = 0x03, @@ -183,6 +185,12 @@ enum spmi_boost_byp_registers { SPMI_BOOST_BYP_REG_CURRENT_LIMIT = 0x4b, }; +enum spmi_hfs430_registers { + SPMI_HFS430_REG_VOLTAGE_LB = 0x40, + SPMI_HFS430_REG_VOLTAGE_VALID_LB = 0x42, + SPMI_HFS430_REG_MODE = 0x45, +}; + enum spmi_saw3_registers { SAW3_SECURE = 0x00, SAW3_ID = 0x04, @@ -260,20 +268,61 @@ enum spmi_common_control_register_index { #define SPMI_FTSMPS_STEP_CTRL_DELAY_MASK 0x07 #define SPMI_FTSMPS_STEP_CTRL_DELAY_SHIFT 0 -/* Clock rate in kHz of the FTSMPS regulator reference clock. */ +#define SPMI_HFS430_STEP_CTRL_STEP_MASK 0 +#define SPMI_HFS430_STEP_CTRL_STEP_SHIFT 0 +#define SPMI_HFS430_STEP_CTRL_DELAY_MASK 0x3 +#define SPMI_HFS430_STEP_CTRL_DELAY_SHIFT 0 + +/* Clock rate in kHz of the FTSMPS and HFS430 regulator reference clocks. */ #define SPMI_FTSMPS_CLOCK_RATE 19200 +#define SPMI_HFS430_CLOCK_RATE 1600 /* Minimum voltage stepper delay for each step. */ #define SPMI_FTSMPS_STEP_DELAY 8 +#define SPMI_HFS430_STEP_DELAY 2 #define SPMI_DEFAULT_STEP_DELAY 20 /* - * The ratio SPMI_FTSMPS_STEP_MARGIN_NUM/SPMI_FTSMPS_STEP_MARGIN_DEN is used to + * The ratio SPMI_xxxxx_STEP_MARGIN_NUM/SPMI_xxxxx_STEP_MARGIN_DEN is used to * adjust the step rate in order to account for oscillator variance. */ #define SPMI_FTSMPS_STEP_MARGIN_NUM 4 #define SPMI_FTSMPS_STEP_MARGIN_DEN 5 +#define SPMI_HFS430_STEP_MARGIN_NUM 10 +#define SPMI_HFS430_STEP_MARGIN_DEN 11 + +#define SPMI_STEP_MASK(__x) SPMI_##__x##_STEP_CTRL_STEP_MASK +#define SPMI_STEP_SHIFT(__x) SPMI_##__x##_STEP_CTRL_STEP_SHIFT +#define SPMI_DELAY_MASK(__x) SPMI_##__x##_STEP_CTRL_DELAY_MASK +#define SPMI_DELAY_SHIFT(__x) SPMI_##__x##_STEP_CTRL_DELAY_SHIFT +#define SPMI_CLOCK_RATE(__x) SPMI_##__x##_CLOCK_RATE +#define SPMI_STEP_DELAY(__x) SPMI_##__x##_STEP_DELAY +#define SPMI_MARGIN_NUM(__x) SPMI_##__x##_STEP_MARGIN_NUM +#define SPMI_MARGIN_DEN(__x) SPMI_##__x##_STEP_MARGIN_DEN + +struct slew_rate_config { + unsigned int delay_mask; + unsigned int delay_shift; + unsigned int step_mask; + unsigned int step_shift; + unsigned int margin_num; + unsigned int margin_den; + unsigned int step_delay; + unsigned int clock_rate; +}; + +#define SLEW_RATE_CONFIG(x) { \ + .delay_shift = SPMI_DELAY_SHIFT(x), \ + .delay_mask = SPMI_DELAY_MASK(x), \ + .step_shift = SPMI_STEP_SHIFT(x), \ + .step_mask = SPMI_STEP_MASK(x), \ + .margin_num = SPMI_MARGIN_NUM(x), \ + .margin_den = SPMI_MARGIN_DEN(x), \ + .step_delay = SPMI_STEP_DELAY(x), \ + .clock_rate = SPMI_CLOCK_RATE(x), \ +} + /* VSET value to decide the range of ULT SMPS */ #define ULT_SMPS_RANGE_SPLIT 0x60 @@ -472,6 +521,11 @@ static struct spmi_voltage_range ult_pldo_ranges[] = { SPMI_VOLTAGE_RANGE(0, 1750000, 1750000, 3337500, 3337500, 12500), }; +/* must be only one range */ +static struct spmi_voltage_range hfs430_ranges[] = { + SPMI_VOLTAGE_RANGE(0, 320000, 320000, 2040000, 2040000, 8000), +}; + static DEFINE_SPMI_SET_POINTS(pldo); static DEFINE_SPMI_SET_POINTS(nldo1); static DEFINE_SPMI_SET_POINTS(nldo2); @@ -486,6 +540,7 @@ static DEFINE_SPMI_SET_POINTS(ult_lo_smps); static DEFINE_SPMI_SET_POINTS(ult_ho_smps); static DEFINE_SPMI_SET_POINTS(ult_nldo); static DEFINE_SPMI_SET_POINTS(ult_pldo); +static DEFINE_SPMI_SET_POINTS(hfs430); static inline int spmi_vreg_read(struct spmi_regulator *vreg, u16 addr, u8 *buf, int len) @@ -653,6 +708,10 @@ spmi_regulator_find_range(struct spmi_regulator *vreg) range = vreg->set_points->range; end = range + vreg->set_points->count; + /* we know we only have one range for this type */ + if (vreg->logical_type == SPMI_REGULATOR_LOGICAL_TYPE_HFS430) + return range; + spmi_vreg_read(vreg, SPMI_COMMON_REG_VOLTAGE_RANGE, &range_sel, 1); for (; range < end; range++) @@ -1135,6 +1194,82 @@ spmi_regulator_saw_set_voltage(struct regulator_dev *rdev, unsigned selector) &voltage_sel, true); } +#define SPMI_HFS430_MODE_PWM 0x07 +#define SPMI_HFS430_MODE_AUTO 0x06 + +static unsigned int spmi_regulator_hfs430_get_mode(struct regulator_dev *rdev) +{ + struct spmi_regulator *vreg = rdev_get_drvdata(rdev); + u8 reg; + int ret; + + ret = spmi_vreg_read(vreg, SPMI_HFS430_REG_MODE, ®, 1); + if (ret) { + dev_err(&rdev->dev, "failed to get mode"); + return ret; + } + + if (reg == SPMI_HFS430_MODE_PWM) + return REGULATOR_MODE_NORMAL; + + return REGULATOR_MODE_IDLE; +} + +static int spmi_regulator_hfs430_set_mode(struct regulator_dev *rdev, + unsigned int mode) +{ + struct spmi_regulator *vreg = rdev_get_drvdata(rdev); + u8 reg = mode == REGULATOR_MODE_NORMAL ? SPMI_HFS430_MODE_PWM : + SPMI_HFS430_MODE_AUTO; + + return spmi_vreg_write(vreg, SPMI_HFS430_REG_MODE, ®, 1); +} + +int spmi_regulator_hfs430_get_voltage(struct regulator_dev *rdev) +{ + struct spmi_regulator *vreg = rdev_get_drvdata(rdev); + u8 val[2]; + int ret, uV; + + ret = spmi_vreg_read(vreg, SPMI_HFS430_REG_VOLTAGE_VALID_LB, val, 2); + if (ret) { + dev_err(&rdev->dev, "failed to get voltage"); + return ret; + } + + uV = 1000 * (((unsigned int) val[1] << 8) | val[0]); + dev_dbg(&rdev->dev, "read = %d", uV); + + return uV; +} + +static int spmi_regulator_hfs430_set_voltage(struct regulator_dev *rdev, + unsigned selector) +{ + struct spmi_regulator *vreg = rdev_get_drvdata(rdev); + const struct spmi_voltage_range *range; + int uV, vlevel; + u8 val[2]; + + range = spmi_regulator_find_range(vreg); + if (!range) + return -EINVAL; + + uV = spmi_regulator_common_list_voltage(rdev, selector); + if (uV <= 0) + return uV; + + vlevel = roundup(uV, range->step_uV) / 1000; + + dev_dbg(&rdev->dev, "write (%d, %d), mode (%d)", uV, vlevel, + spmi_regulator_hfs430_get_mode(rdev)); + + val[0] = vlevel & 0xFF; + val[1] = (vlevel >> 8) & 0xFF; + + return spmi_vreg_write(vreg, SPMI_HFS430_REG_VOLTAGE_LB, val, 2); +} + static struct regulator_ops spmi_saw_ops = {}; static struct regulator_ops spmi_smps_ops = { @@ -1264,12 +1399,24 @@ static struct regulator_ops spmi_ult_ldo_ops = { .set_soft_start = spmi_regulator_common_set_soft_start, }; +static struct regulator_ops spmi_hfs430_ops = { + /* always on regulators */ + .set_voltage_sel = spmi_regulator_hfs430_set_voltage, + .set_voltage_time_sel = spmi_regulator_set_voltage_time_sel, + .get_voltage = spmi_regulator_hfs430_get_voltage, + .map_voltage = spmi_regulator_common_map_voltage, + .list_voltage = spmi_regulator_common_list_voltage, + .get_mode = spmi_regulator_hfs430_get_mode, + .set_mode = spmi_regulator_hfs430_set_mode, +}; + /* Maximum possible digital major revision value */ #define INF 0xFF static const struct spmi_regulator_mapping supported_regulators[] = { - /* type subtype dig_min dig_max ltype ops setpoints hpm_min */ + /* type subtype dig_min dig_max ltype ops setpoints hpm_min */ SPMI_VREG(BUCK, GP_CTL, 0, INF, SMPS, smps, smps, 100000), + SPMI_VREG(BUCK, HFS430, 0, INF, HFS430, hfs430, hfs430, 10000), SPMI_VREG(LDO, N300, 0, INF, LDO, ldo, nldo1, 10000), SPMI_VREG(LDO, N600, 0, 0, LDO, ldo, nldo2, 10000), SPMI_VREG(LDO, N1200, 0, 0, LDO, ldo, nldo2, 10000), @@ -1396,8 +1543,12 @@ static int spmi_regulator_init_slew_rate(struct spmi_regulator *vreg) { int ret; u8 reg = 0; - int step, delay, slew_rate, step_delay; + int step, delay, slew_rate; const struct spmi_voltage_range *range; + struct slew_rate_config *config, configs[] = { + [0] = SLEW_RATE_CONFIG(HFS430), + [1] = SLEW_RATE_CONFIG(FTSMPS), + }; ret = spmi_vreg_read(vreg, SPMI_COMMON_REG_STEP_CTRL, ®, 1); if (ret) { @@ -1410,25 +1561,26 @@ static int spmi_regulator_init_slew_rate(struct spmi_regulator *vreg) return -EINVAL; switch (vreg->logical_type) { - case SPMI_REGULATOR_LOGICAL_TYPE_FTSMPS: - step_delay = SPMI_FTSMPS_STEP_DELAY; + case SPMI_REGULATOR_LOGICAL_TYPE_HFS430: + config = &configs[0]; break; default: - step_delay = SPMI_DEFAULT_STEP_DELAY; - break; - } + config = &configs[1]; + if (vreg->logical_type != SPMI_REGULATOR_LOGICAL_TYPE_FTSMPS) + config->step_delay = SPMI_STEP_DELAY(DEFAULT); + }; - step = reg & SPMI_FTSMPS_STEP_CTRL_STEP_MASK; - step >>= SPMI_FTSMPS_STEP_CTRL_STEP_SHIFT; + step = reg & config->step_mask; + step >>= config->step_shift; - delay = reg & SPMI_FTSMPS_STEP_CTRL_DELAY_MASK; - delay >>= SPMI_FTSMPS_STEP_CTRL_DELAY_SHIFT; + delay = reg & config->delay_mask; + delay >>= config->delay_shift; /* slew_rate has units of uV/us */ - slew_rate = SPMI_FTSMPS_CLOCK_RATE * range->step_uV * (1 << step); - slew_rate /= 1000 * (step_delay << delay); - slew_rate *= SPMI_FTSMPS_STEP_MARGIN_NUM; - slew_rate /= SPMI_FTSMPS_STEP_MARGIN_DEN; + slew_rate = config->clock_rate * range->step_uV * (1 << step); + slew_rate /= 1000 * (config->step_delay << delay); + slew_rate *= config->margin_num; + slew_rate /= config->margin_den; /* Ensure that the slew rate is greater than 0 */ vreg->slew_rate = max(slew_rate, 1); @@ -1445,6 +1597,9 @@ static int spmi_regulator_init_registers(struct spmi_regulator *vreg, type = vreg->logical_type; + if (type == SPMI_REGULATOR_LOGICAL_TYPE_HFS430) + return 0; + ret = spmi_vreg_read(vreg, SPMI_COMMON_REG_VOLTAGE_RANGE, ctrl_reg, 8); if (ret) return ret; @@ -1572,9 +1727,11 @@ static int spmi_regulator_of_parse(struct device_node *node, case SPMI_REGULATOR_LOGICAL_TYPE_ULT_LO_SMPS: case SPMI_REGULATOR_LOGICAL_TYPE_ULT_HO_SMPS: case SPMI_REGULATOR_LOGICAL_TYPE_SMPS: + case SPMI_REGULATOR_LOGICAL_TYPE_HFS430: ret = spmi_regulator_init_slew_rate(vreg); if (ret) return ret; + default: break; } @@ -1731,12 +1888,18 @@ static const struct spmi_regulator_data pmi8994_regulators[] = { { } }; +static const struct spmi_regulator_data pms405_regulators[] = { + { "s3", 0x1a00, }, /* supply name in the dts only */ + { } +}; + static const struct of_device_id qcom_spmi_regulator_match[] = { { .compatible = "qcom,pm8841-regulators", .data = &pm8841_regulators }, { .compatible = "qcom,pm8916-regulators", .data = &pm8916_regulators }, { .compatible = "qcom,pm8941-regulators", .data = &pm8941_regulators }, { .compatible = "qcom,pm8994-regulators", .data = &pm8994_regulators }, { .compatible = "qcom,pmi8994-regulators", .data = &pmi8994_regulators }, + { .compatible = "qcom,pms405-regulators", .data = &pms405_regulators }, { } }; MODULE_DEVICE_TABLE(of, qcom_spmi_regulator_match);
The PMS405 has 5 HFSMPS and 13 LDO regulators, This commit adds support for one of the 5 HFSMPS regulators (s3) to the spmi regulator driver. The PMIC HFSMPS 430 regulators have 8 mV step size and a voltage control scheme consisting of two 8-bit registers defining a 16-bit voltage set point in units of millivolts S3 controls the cpu voltages (s3 is a buck regulator of type HFS430); it is therefore required so we can enable voltage scaling for safely running cpufreq. Signed-off-by: Jorge Ramirez-Ortiz <jorge.ramirez-ortiz@linaro.org> --- drivers/regulator/qcom_spmi-regulator.c | 197 +++++++++++++++++++++++++++++--- 1 file changed, 180 insertions(+), 17 deletions(-)