Messages in this thread | | | Date | Thu, 8 Sep 2022 10:45:40 +0300 | From | Matti Vaittinen <> | Subject | Re: [PATCHv2 12/13] regulator: rk808: add rk806 support |
| |
On 9/8/22 03:31, Sebastian Reichel wrote: > Add rk806 support to the existing rk808 regulator > driver. > > This has been implemented using shengfei Xu's rk806 > specific driver from the vendor tree as reference. > > Co-Developed-by: shengfei Xu <xsf@rock-chips.com> > Signed-off-by: shengfei Xu <xsf@rock-chips.com> > Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com> > --- > drivers/regulator/rk808-regulator.c | 482 ++++++++++++++++++++++++++++ > 1 file changed, 482 insertions(+) > > diff --git a/drivers/regulator/rk808-regulator.c b/drivers/regulator/rk808-regulator.c > index fa9fc1aa1ae3..cd1a2cff4a37 100644 > --- a/drivers/regulator/rk808-regulator.c > +++ b/drivers/regulator/rk808-regulator.c
Thanks for upstreaming the downstream stuff! :) I wonder if we could drop some code by using the existing helpers? Or maybe I am missreading some code. Wouldn't be the first (and probably not the last) time...
//snip
> struct rk808_regulator_data { > struct gpio_desc *dvs_gpio[2]; > }; > @@ -216,6 +271,223 @@ static const unsigned int rk817_buck1_4_ramp_table[] = { > 3000, 6300, 12500, 25000 > }; > > +static int rk806_get_voltage_sel_regmap(struct regulator_dev *rdev) > +{ > + unsigned int val; > + int vsel_reg, ret; > + > + vsel_reg = rdev->desc->vsel_reg; > + > + ret = regmap_read(rdev->regmap, vsel_reg, &val); > + if (ret != 0) > + return ret; > + > + val &= rdev->desc->vsel_mask; > + val >>= ffs(rdev->desc->vsel_mask) - 1; > + > + return val; > +}
Could we just use the regulator_get_voltage_sel_regmap()?
> + > +static int rk806_set_voltage(struct regulator_dev *rdev, > + int req_min_uV, int req_max_uV, > + unsigned int *selector) > +{ > + int vsel_reg, ret, sel; > + > + ret = regulator_map_voltage_linear_range(rdev, req_min_uV, req_max_uV); > + if (ret >= 0) { > + *selector = ret; > + sel = ret; > + } else { > + return -EINVAL; > + } > + > + vsel_reg = rdev->desc->vsel_reg; > + > + sel <<= ffs(rdev->desc->vsel_mask) - 1; > + > + ret = regmap_update_bits(rdev->regmap, vsel_reg, > + rdev->desc->vsel_mask, sel); > + > + return ret; > +}
Hmm. Maybe this is not necessary? I wonder if we could get away just with the .map and .set_voltage_sel (regulator_get_voltage_sel_regmap() and regulator_map_voltage_linear_range()).
// snip > + > +static int rk806_set_ramp_delay_ldo(struct regulator_dev *rdev, int ramp_delay) > +{ > + unsigned int ramp_value = RK806_RAMP_RATE_2LSB_PER_1CLK; > + int regval; > + > + switch (ramp_delay) { > + case 1 ... 780: > + ramp_value = RK806_RAMP_RATE_1LSB_PER_32CLK; > + break; > + case 781 ... 1900: > + ramp_value = RK806_RAMP_RATE_1LSB_PER_13CLK; > + break; > + case 1901 ... 3120: > + ramp_value = RK806_RAMP_RATE_1LSB_PER_8CLK; > + break; > + case 3121 ... 6280: > + ramp_value = RK806_RAMP_RATE_1LSB_PER_4CLK; > + break; > + case 6281 ... 12500: > + ramp_value = RK806_RAMP_RATE_1LSB_PER_2CLK; > + break; > + case 12501 ... 25000: > + ramp_value = RK806_RAMP_RATE_1LSB_PER_1CLK; > + break; > + case 25001 ... 50000: > + ramp_value = RK806_RAMP_RATE_2LSB_PER_1CLK; > + break; > + case 50001 ... 100000: > + ramp_value = RK806_RAMP_RATE_4LSB_PER_1CLK; > + break; > + default: > + pr_warn("%s ramp_delay: %d not supported, setting 10000\n", > + rdev->desc->name, ramp_delay); > + } > + > + regval = ramp_value << (ffs(rdev->desc->ramp_mask) - 1); > + return regmap_update_bits(rdev->regmap, rdev->desc->ramp_reg, > + rdev->desc->ramp_mask, regval); > +}
Do you think we could get rid of this function by populating a ramp-delay table and using regulator_set_ramp_delay_regmap()?
Best Regards -- Matti
-- Matti Vaittinen Linux kernel developer at ROHM Semiconductors Oulu Finland
~~ When things go utterly wrong vim users can always type :help! ~~
| |