[U-Boot] [U-Boot,2/5] power: pmic: rk816: support rk816 pmic

[Simon Glass]

Hi,

On 13 September 2017 at 14:22, Philipp Tomsich
<philipp.tomsich at theobroma-systems.com> wrote:
>
>
> On Wed, 13 Sep 2017, Kever Yang wrote:
>
>> From: Elaine Zhang <zhangqing at rock-chips.com>
>>
>> Add Rockchip pmic rk816 support.
>
>
> Could you summarise some of the key-features of the RK816 here? Just a few
> words, so we know how it's different from the others and what it's used for
> (e.g. "PMIC matching the RK3xxx").
>
>
>>
>> Signed-off-by: Elaine Zhang <zhangqing at rock-chips.com>
>> Signed-off-by: Kever Yang <kever.yang at rock-chips.com>
>> ---
>>
>> drivers/power/pmic/rk8xx.c      |   1 +
>> drivers/power/regulator/rk8xx.c | 212
>> ++++++++++++++++++++++++++++++++--------
>> include/power/rk8xx_pmic.h      |   9 +-
>> 3 files changed, 182 insertions(+), 40 deletions(-)
>>
>> diff --git a/drivers/power/pmic/rk8xx.c b/drivers/power/pmic/rk8xx.c
>> index eb3ec0f..0fdea95 100644
>> --- a/drivers/power/pmic/rk8xx.c
>> +++ b/drivers/power/pmic/rk8xx.c
>> @@ -100,6 +100,7 @@ static struct dm_pmic_ops rk8xx_ops = {
>>
>> static const struct udevice_id rk8xx_ids[] = {
>>         { .compatible = "rockchip,rk808" },
>> +       { .compatible = "rockchip,rk816" },
>>         { .compatible = "rockchip,rk818" },
>>         { }
>> };
>> diff --git a/drivers/power/regulator/rk8xx.c
>> b/drivers/power/regulator/rk8xx.c
>> index 76fc2ef..cf3566e 100644
>> --- a/drivers/power/regulator/rk8xx.c
>> +++ b/drivers/power/regulator/rk8xx.c
>> @@ -48,6 +48,21 @@ static const struct rk8xx_reg_info rk808_buck[] = {
>>         { 1800000, 100000, REG_BUCK4_ON_VSEL, RK808_BUCK4_VSEL_MASK, },
>> };
>>
>> +static const struct rk8xx_reg_info rk816_buck[] = {
>> +       /* buck 1 */
>> +       { 712500, 12500, REG_BUCK1_ON_VSEL, RK818_BUCK_VSEL_MASK, },
>> +       { 1800000, 200000, REG_BUCK1_ON_VSEL, RK818_BUCK_VSEL_MASK, },
>> +       { 2300000, 0, REG_BUCK1_ON_VSEL, RK818_BUCK_VSEL_MASK, },
>> +       /* buck 2 */
>> +       { 712500, 12500, REG_BUCK2_ON_VSEL, RK818_BUCK_VSEL_MASK, },
>> +       { 1800000, 200000, REG_BUCK2_ON_VSEL, RK818_BUCK_VSEL_MASK, },
>> +       { 2300000, 0, REG_BUCK2_ON_VSEL, RK818_BUCK_VSEL_MASK, },
>> +       /* buck 3 */
>> +       { 712500, 12500, -1, RK818_BUCK_VSEL_MASK, },
>> +       /* buck 4 */
>> +       { 800000, 100000, REG_BUCK4_ON_VSEL, RK818_BUCK4_VSEL_MASK, },
>> +};
>> +
>> static const struct rk8xx_reg_info rk818_buck[] = {
>>         { 712500, 12500, REG_BUCK1_ON_VSEL, RK818_BUCK_VSEL_MASK, },
>>         { 712500, 12500, REG_BUCK2_ON_VSEL, RK818_BUCK_VSEL_MASK, },
>> @@ -67,6 +82,15 @@ static const struct rk8xx_reg_info rk808_ldo[] = {
>>         { 1800000, 100000, REG_LDO8_ON_VSEL, RK808_LDO_VSEL_MASK, },
>> };
>>
>> +static const struct rk8xx_reg_info rk816_ldo[] = {
>> +       { 800000, 100000, REG_LDO1_ON_VSEL, RK818_LDO_VSEL_MASK, },
>> +       { 800000, 100000, REG_LDO2_ON_VSEL, RK818_LDO_VSEL_MASK, },
>> +       { 800000, 100000, REG_LDO3_ON_VSEL, RK818_LDO_VSEL_MASK, },
>> +       { 800000, 100000, REG_LDO4_ON_VSEL, RK818_LDO_VSEL_MASK, },
>> +       { 800000, 100000, REG_LDO5_ON_VSEL, RK818_LDO_VSEL_MASK, },
>> +       { 800000, 100000, REG_LDO6_ON_VSEL, RK818_LDO_VSEL_MASK, },
>> +};
>> +
>> static const struct rk8xx_reg_info rk818_ldo[] = {
>>         { 1800000, 100000, REG_LDO1_ON_VSEL, RK818_LDO_VSEL_MASK, },
>>         { 1800000, 100000, REG_LDO2_ON_VSEL, RK818_LDO_VSEL_MASK, },
>> @@ -88,10 +112,24 @@ static const uint rk818_chrg_shutdown_vsel_array[] =
>> {
>> };
>>
>> static const struct rk8xx_reg_info *get_buck_reg(struct udevice *pmic,
>> -                                            int num)
>> +                                                int num, int uvolt)
>> {
>>         struct rk8xx_priv *priv = dev_get_priv(pmic);
>> +
>>         switch (priv->variant) {
>> +       case RK816_ID:
>> +               switch (num) {
>> +               case 0:
>> +               case 1:
>> +                       if (uvolt <= 1450000)
>> +                               return &rk816_buck[num * 3 + 0];
>> +                       else if (uvolt <= 2200000)
>> +                               return &rk816_buck[num * 3 + 1];
>> +                       else
>> +                               return &rk816_buck[num * 3 + 2];
>> +               default:
>> +                       return &rk816_buck[num + 4];
>> +               }
>
>
> I am very concerned with this driver turning into a large switch statement:
> this really is not the way driver_data is supposed to be used.
>
> Can we have a single path through this function and use driver_data to
> parameterize the cut-offs? As an alternative, we'll need to start splitting
> this into separate drivers.

Yes it is worth considering that. I don't see a good reason to have
this new device in the same driver, given all the changes.

If there is shared code it could be split out into a new file.

Regards,
Simon