Re: [v2,2/2] regulator: add QCOM RPMh regulator driver

[Matthias Kaehlcke]

On Tue, Apr 17, 2018 at 12:15:18PM -0700, David Collins wrote:
> On 04/17/2018 11:23 AM, Matthias Kaehlcke wrote:
> > On Fri, Apr 13, 2018 at 07:50:35PM -0700, David Collins wrote:
> >> Add the QCOM RPMh regulator driver to manage PMIC regulators
> >> which are controlled via RPMh on some Qualcomm Technologies, Inc.
> >> SoCs.  RPMh is a hardware block which contains several
> >> accelerators which are used to manage various hardware resources
> >> that are shared between the processors of the SoC.  The final
> >> hardware state of a regulator is determined within RPMh by
> >> performing max aggregation of the requests made by all of the
> >> processors.
> >> [...]
> >> +/**
> >> + * struct rpmh_vreg_hw_data - RPMh regulator hardware configurations
> >> + * @regulator_type:		RPMh accelerator type used to manage this
> >> + *				regulator
> >> + * @ops:			Pointer to regulator ops callback structure
> >> + * @voltage_range:		The single range of voltages supported by this
> >> + *				PMIC regulator type
> >> + * @n_voltages:			The number of unique voltage set points defined
> >> + *				by voltage_range
> >> + * @pmic_mode_map:		Array indexed by regulator framework mode
> >> + *				containing PMIC hardware modes.  Must be large
> >> + *				enough to index all framework modes supported
> >> + *				by this regulator hardware type.
> >> + * @of_map_mode:		Maps an RPMH_REGULATOR_MODE_* mode value defined
> >> + *				in device tree to a regulator framework mode
> > 
> > The name of the field is a bit misleading, this is a map of RPMh mode
> > to regulator framework mode, the device tree just happens to be the
> > place where this mapping is defined.
> 
> of_map_mode name is used here to match the struct regulator_desc field by
> the same name that it is assigned to [1].  Do you think that the name
> should be changed to something else?

Thanks, I missed that it's the name of the field in struct
regulator_desc field, in that case it certainly makes sense to use the
same name.

> >> +/**
> >> + * struct rpmh_vreg - individual rpmh regulator data structure encapsulating a
> >> + *		single regulator device
> >> + * @rpmh_client:		Handle used for rpmh communications
> > 
> > nit: RPMh
> 
> I'll change this.
> 
> 
> >> +struct rpmh_vreg {
> >> +	struct rpmh_client		*rpmh_client;
> >> +	u32				addr;
> >> +	struct regulator_desc		rdesc;
> >> +	const struct rpmh_vreg_hw_data	*hw_data;
> >> +	enum rpmh_regulator_type	regulator_type;
> > 
> > This value is already available via rpmh_vreg->hw_data->regulator_type,
> > why duplicate it? The field is assigned in rpmh_regulator_init_vreg()
> > and only read once in the same function, there seems to be no need for
> > it, not even to improve readability.
> 
> This is present to specifically allow for a future change to support
> overriding the regulator_type value from device tree in order to force
> RPMh resources to be handled via XOB instead of VRM in a board-specific
> manner.  I included support of the property qcom,rpmh-resource-type in the
> first version of this patch.  I removed this property from the second
> version of the patch based upon review feedback since SDM845 does not
> explicitly need it (though an upcoming chip will).
> 
> I'll remove regulator_type from struct rpmh_vreg.  It shouldn't be
> particularly painful to add it back in when needed for XOB override support.
> 
> 
> >> +static int rpmh_regulator_vrm_set_load(struct regulator_dev *rdev, int load_uA)
> >> +{
> >> +	struct rpmh_vreg *vreg = rdev_get_drvdata(rdev);
> >> +	int i;
> >> +
> >> +	for (i = 0; i < vreg->drms_mode_count - 1; i++)
> >> +		if (load_uA < vreg->drms_mode_max_uA[i])
> > 
> > Shouldn't this be '<='?
> > 
> > nit: IMO 'vreg->drms_mode_max_uA[i] >= load_uA' would be more readable.
> 
> I chose to use '<' here in order to maintain the non-inclusive limit
> semantics of the downstream RPMh regulator driver.  E.g. with an LPM
> threshold of 10000 uA, load_uA == 10000 would result in a request for HPM
> instead of LPM.
> 
> I suppose that I can change this to '<=' to be more logically consistent.
> 
> 
> >> +static const u32 pmic_mode_map_pmic4_ldo[REGULATOR_MODE_STANDBY + 1] = {
> >> +	[REGULATOR_MODE_STANDBY] = 4,
> >> +	[REGULATOR_MODE_IDLE]    = 5,
> >> +	[REGULATOR_MODE_NORMAL]  = -EINVAL,
> >> +	[REGULATOR_MODE_FAST]    = 7,
> >> +};
> > 
> > Define constants for the modes on the PMIC4 side?
> 
> Are you suggesting something like this?
> 
> #define PMIC4_LDO_MODE_RETENTION	4
> #define PMIC4_LDO_MODE_LPM		5
> #define PMIC4_LDO_MODE_HPM		7
> 
> static const u32 pmic_mode_map_pmic4_ldo[REGULATOR_MODE_STANDBY + 1] = {
> 	[REGULATOR_MODE_STANDBY] = PMIC4_LDO_MODE_RETENTION,
> 	[REGULATOR_MODE_IDLE]    = PMIC4_LDO_MODE_LPM,
> 	[REGULATOR_MODE_NORMAL]  = -EINVAL,
> 	[REGULATOR_MODE_FAST]    = PMIC4_LDO_MODE_HPM,
> };
> 
> #define PMIC4_SMPS_MODE_RETENTION	4
> #define PMIC4_SMPS_MODE_PFM		5
> #define PMIC4_SMPS_MODE_AUTO		6
> #define PMIC4_SMPS_MODE_PWM		7
> 
> static const u32 pmic_mode_map_pmic4_smps[REGULATOR_MODE_STANDBY + 1] = {
> 	[REGULATOR_MODE_STANDBY] = PMIC4_SMPS_MODE_RETENTION,
> 	[REGULATOR_MODE_IDLE]    = PMIC4_SMPS_MODE_PFM,
> 	[REGULATOR_MODE_NORMAL]  = PMIC4_SMPS_MODE_AUTO,
> 	[REGULATOR_MODE_FAST]    = PMIC4_SMPS_MODE_PWM,
> };
> 
> I considered using this approach, but it didn't seem like it increased
> readability and did increase the line count.  Each of the constants would
> only be used once.  Would you prefer this style (or something else)?

Personally I prefer this style, since the constants are more
expressive than the literals. I agree that the 'inline' constant
definition is a bit noisy, perhaps it would be better to move the
definitions to the top of the file or group them before the definition
of pmic_mode_map_pmic4_ldo. Alteratively you could create a
drivers/regulator/qcom_rpmh-regulator.h and also move the definitions
of struct struct rpmh_vreg_hw_data, rpmh_vreg, ... there.

Thanks

Matthias