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

[David Collins]

On 04/17/2018 12:47 PM, Matthias Kaehlcke wrote:
> 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:
>>>> [...]
>>>> +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.

I'll add constants for the per-type regulator modes at the top of the
file.  We'll see if other reviewers like them.

Take care,
David

-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project