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
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
