Re: [PATCH v3 4/4] regulator: qcom_rpm-regulator: Add support for pm8018 rpm regulator

[Neil Armstrong]

Hi Stephen,
On 08/05/2016 12:04 AM, Stephen Boyd wrote:
> On 07/19/2016 06:51 AM, Neil Armstrong wrote:
>> In order to support eh Qualcomm MDM9615 SoC, add support for the
> 
> s/eh/the/
> 
>> PM8018 RPM regulator in the qcom_rpm-regulator driver.
>>
>> Acked-by: Mark Brown <broonie@xxxxxxxxxx>
>> Signed-off-by: Neil Armstrong <narmstrong@xxxxxxxxxxxx>
>>
>> diff --git a/drivers/regulator/qcom_rpm-regulator.c b/drivers/regulator/qcom_rpm-regulator.c
>> index e254272..0734a5f 100644
>> --- a/drivers/regulator/qcom_rpm-regulator.c
>> +++ b/drivers/regulator/qcom_rpm-regulator.c
>> @@ -70,6 +70,40 @@ struct qcom_rpm_reg {
>>  	bool supports_force_mode_bypass;
>>  };
>>  
>> +static struct rpm_reg_parts rpm8018_ldo_parts = {
>> +	.request_len	= 2,
>> +	.uV		= { 0, 0x007FFFFF,  0},
>> +	.pd		= { 0, 0x00800000, 23},
>> +	.pc		= { 0, 0x0F000000, 24},
>> +	.pf		= { 0, 0xF0000000, 28},
>> +	.ip		= { 1, 0x000003FF,  0},
>> +	.ia		= { 1, 0x000FFC00, 10},
>> +	.fm		= { 1, 0x00700000, 20},
>> +};
>> +
>> +static struct rpm_reg_parts rpm8018_smps_parts = {
>> +	.request_len	= 2,
>> +	.uV		= {  0, 0x007FFFFF,  0},
>> +	.pd		= {  0, 0x00800000, 23},
>> +	.pc		= {  0, 0x0F000000, 24},
>> +	.pf		= {  0, 0xF0000000, 28},
>> +	.ip		= {  1, 0x000003FF,  0},
>> +	.ia		= {  1, 0x000FFC00, 10},
>> +	.fm		= {  1, 0x00700000, 20},
>> +	.pm		= {  1, 0x00800000, 23},
>> +	.freq		= {  1, 0x1F000000, 24},
>> +	.freq_clk_src	= {  1, 0x60000000, 29},
>> +};
>> +
>> +static struct rpm_reg_parts rpm8018_switch_parts = {
>> +	.request_len	= 1,
>> +	.enable_state	= {  0, 0x00000001,  0},
>> +	.pd		= {  0, 0x00000002,  1},
>> +	.pc		= {  0, 0x0000003C,  2},
>> +	.pf		= {  0, 0x000003C0,  6},
>> +	.hpm		= {  0, 0x00000C00, 10},
>> +};
>> +
> 
> These are all the same as the rpm8960 ones, so why don't we reuse those
> structures?

Indeed, I will reuse them.

> 
>>  static const struct rpm_reg_parts rpm8660_ldo_parts = {
>>  	.request_len    = 2,
>>  	.mV             = { 0, 0x00000FFF,  0 },
>> @@ -448,6 +482,44 @@ static struct regulator_ops switch_ops = {
>>  };
>>  
>>  /*
>> + * PM8018 regulators
>> + */
>> +static const struct qcom_rpm_reg pm8018_pldo = {
>> +	.desc.linear_ranges = pldo_ranges,
>> +	.desc.n_linear_ranges = ARRAY_SIZE(pldo_ranges),
>> +	.desc.n_voltages = 161,
>> +	.desc.ops = &uV_ops,
>> +	.parts = &rpm8018_ldo_parts,
>> +	.supports_force_mode_auto = false,
>> +	.supports_force_mode_bypass = false,
>> +};
>> +
>> +static const struct qcom_rpm_reg pm8018_nldo = {
>> +	.desc.linear_ranges = nldo_ranges,
>> +	.desc.n_linear_ranges = ARRAY_SIZE(nldo_ranges),
>> +	.desc.n_voltages = 64,
>> +	.desc.ops = &uV_ops,
>> +	.parts = &rpm8018_ldo_parts,
>> +	.supports_force_mode_auto = false,
>> +	.supports_force_mode_bypass = false,
>> +};
>> +
>> +static const struct qcom_rpm_reg pm8018_smps = {
>> +	.desc.linear_ranges = smps_ranges,
>> +	.desc.n_linear_ranges = ARRAY_SIZE(smps_ranges),
>> +	.desc.n_voltages = 154,
>> +	.desc.ops = &uV_ops,
>> +	.parts = &rpm8018_smps_parts,
>> +	.supports_force_mode_auto = false,
>> +	.supports_force_mode_bypass = false,
>> +};
>> +
>> +static const struct qcom_rpm_reg pm8018_switch = {
>> +	.desc.ops = &switch_ops,
>> +	.parts = &rpm8018_switch_parts,
>> +};
>> +
> 
> These are all the same as the pm8921 ones too? So just use those instead?

They look but the supports_force_mode_bypass is set to "true" for pm8921.
These pm8018 structures are still needed.

> 
> We should probably rethink this design and have these structures based
> on PMIC derivatives instead of SoCs because things are quite similar
> across many PMICs.
> 
This would help to support the huge pmic family !

Neil