From mboxrd@z Thu Jan 1 00:00:00 1970 From: Elaine Zhang Date: Mon, 10 Apr 2017 10:09:10 +0800 Subject: [U-Boot] [PATCH] power: regulator: pwm: support pwm polarity setting In-Reply-To: References: <1491562976-21917-1-git-send-email-kever.yang@rock-chips.com> Message-ID: <58EAE946.7050901@rock-chips.com> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de On 04/10/2017 03:28 AM, Simon Glass wrote: > Hi, > > On 7 April 2017 at 05:02, Kever Yang wrote: >> The latest kernel PWM drivers enable the polarity settings. When system >> run from U-Boot to kerenl, if there are differences in polarity set or >> duty cycle, the PMW will re-init: >> close -> set polarity and duty cycle -> enable the PWM. >> The power supply controled by pwm regulator may have voltage shaking, >> which lead to the system not stable. >> >> Signed-off-by: Kever Yang >> --- >> >> drivers/power/regulator/pwm_regulator.c | 12 ++++++++++-- >> drivers/pwm/pwm-uclass.c | 10 ++++++++++ >> drivers/pwm/rk_pwm.c | 17 ++++++++++++++++- >> include/pwm.h | 9 +++++++++ >> 4 files changed, 45 insertions(+), 3 deletions(-) > > If the generic uclass change and the rk change can be separated, it is > good to do it. > > Also we should have a test for pwm (test/dm/pwm.c). Are you able to do > that? If not I could give it a try. we have no plan to do it. > >> >> diff --git a/drivers/power/regulator/pwm_regulator.c b/drivers/power/regulator/pwm_regulator.c >> index 4875238..f8a6712 100644 >> --- a/drivers/power/regulator/pwm_regulator.c >> +++ b/drivers/power/regulator/pwm_regulator.c >> @@ -24,6 +24,8 @@ struct pwm_regulator_info { >> int pwm_id; >> /* the period of one PWM cycle */ >> int period_ns; >> + /* the polarity of one PWM */ >> + int polarity; > > Can you update the comment to indicate what the values mean? E.g. is 0 > the normal polarity and 1 inverted? 0 : normal polarity 1 : inverted polarity I will update the comment next version. > >> struct udevice *pwm; >> /* initialize voltage of regulator */ >> unsigned int init_voltage; >> @@ -49,7 +51,7 @@ static int pwm_voltage_to_duty_cycle_percentage(struct udevice *dev, int req_uV) >> int max_uV = priv->max_voltage; >> int diff = max_uV - min_uV; >> >> - return 100 - (((req_uV * 100) - (min_uV * 100)) / diff); >> + return ((req_uV * 100) - (min_uV * 100)) / diff; >> } >> >> static int pwm_regulator_get_voltage(struct udevice *dev) >> @@ -67,6 +69,12 @@ static int pwm_regulator_set_voltage(struct udevice *dev, int uvolt) >> >> duty_cycle = pwm_voltage_to_duty_cycle_percentage(dev, uvolt); >> >> + ret = pwm_set_init(priv->pwm, priv->pwm_id, priv->polarity); > > I wonder if it would be better to combine the polarity into the > pwm_set_config() call? Then we can do everything in one call. If not > then I think pwm_set_invert() would be a better name. The polarity set only once, so we set it in pwm_set_init() call. Using pwm_set_invert, of course, is also possible. > >> + if (ret) { >> + dev_err(dev, "Failed to init PWM\n"); >> + return ret; >> + } >> + >> ret = pwm_set_config(priv->pwm, priv->pwm_id, >> (priv->period_ns / 100) * duty_cycle, priv->period_ns); >> if (ret) { >> @@ -97,9 +105,9 @@ static int pwm_regulator_ofdata_to_platdata(struct udevice *dev) >> debug("%s: Cannot get PWM phandle: ret=%d\n", __func__, ret); >> return ret; >> } >> - /* TODO: pwm_id here from device tree if needed */ >> >> priv->period_ns = args.args[1]; >> + priv->polarity = args.args[2]; > > Does this mean that the binding has this argument and we have been ignoring it? > > Can you bring in the DT binding file from Linux to U-Boot also? > >> >> priv->init_voltage = fdtdec_get_int(blob, node, >> "regulator-init-microvolt", -1); >> diff --git a/drivers/pwm/pwm-uclass.c b/drivers/pwm/pwm-uclass.c >> index c2200af..287a7f3 100644 >> --- a/drivers/pwm/pwm-uclass.c >> +++ b/drivers/pwm/pwm-uclass.c >> @@ -9,6 +9,16 @@ >> #include >> #include >> >> +int pwm_set_init(struct udevice *dev, uint channel, uint polarity) >> +{ >> + struct pwm_ops *ops = pwm_get_ops(dev); >> + >> + if (!ops->set_init) >> + return -ENOSYS; >> + >> + return ops->set_init(dev, channel, polarity); >> +} >> + >> int pwm_set_config(struct udevice *dev, uint channel, uint period_ns, >> uint duty_ns) >> { >> diff --git a/drivers/pwm/rk_pwm.c b/drivers/pwm/rk_pwm.c >> index 9254f5b..5ff1e00 100644 >> --- a/drivers/pwm/rk_pwm.c >> +++ b/drivers/pwm/rk_pwm.c >> @@ -21,8 +21,22 @@ DECLARE_GLOBAL_DATA_PTR; >> struct rk_pwm_priv { >> struct rk3288_pwm *regs; >> ulong freq; >> + uint enable_conf; >> }; >> >> +static int rk_pwm_set_init(struct udevice *dev, uint channel, uint polarity) >> +{ >> + struct rk_pwm_priv *priv = dev_get_priv(dev); >> + >> + debug("%s: polarity=%u\n", __func__, polarity); >> + if (polarity) >> + priv->enable_conf |= PWM_DUTY_NEGATIVE | PWM_INACTIVE_POSTIVE; >> + else >> + priv->enable_conf |= PWM_DUTY_POSTIVE | PWM_INACTIVE_NEGATIVE; >> + >> + return 0; >> +} >> + >> static int rk_pwm_set_config(struct udevice *dev, uint channel, uint period_ns, >> uint duty_ns) >> { >> @@ -32,7 +46,7 @@ static int rk_pwm_set_config(struct udevice *dev, uint channel, uint period_ns, >> >> debug("%s: period_ns=%u, duty_ns=%u\n", __func__, period_ns, duty_ns); >> writel(PWM_SEL_SRC_CLK | PWM_OUTPUT_LEFT | PWM_LP_DISABLE | >> - PWM_CONTINUOUS | PWM_DUTY_POSTIVE | PWM_INACTIVE_POSTIVE | >> + PWM_CONTINUOUS | priv->enable_conf | >> RK_PWM_DISABLE, >> ®s->ctrl); >> >> @@ -83,6 +97,7 @@ static int rk_pwm_probe(struct udevice *dev) >> } >> >> static const struct pwm_ops rk_pwm_ops = { >> + .set_init = rk_pwm_set_init, >> .set_config = rk_pwm_set_config, >> .set_enable = rk_pwm_set_enable, >> }; >> diff --git a/include/pwm.h b/include/pwm.h >> index 851915e..a66ee1f 100644 >> --- a/include/pwm.h >> +++ b/include/pwm.h >> @@ -14,6 +14,15 @@ >> /* struct pwm_ops: Operations for the PWM uclass */ >> struct pwm_ops { >> /** >> + * set_init() - Set the PWM invert >> + * >> + * @dev: PWM device to update >> + * @channel: PWM channel to update >> + * @polarity: PWM invert polarity >> + * @return 0 if OK, -ve on error >> + */ >> + int (*set_init)(struct udevice *dev, uint channel, uint polarity); >> + /** >> * set_config() - Set the PWM configuration >> * >> * @dev: PWM device to update >> -- >> 1.9.1 >> > > Regards, > Simon > > >