Hi Elaine, On 9 April 2017 at 20:09, Elaine Zhang <zhangq...@rock-chips.com> wrote: > > > On 04/10/2017 03:28 AM, Simon Glass wrote: >> >> Hi, >> >> On 7 April 2017 at 05:02, Kever Yang <kever.y...@rock-chips.com> 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 <kever.y...@rock-chips.com> >>> --- >>> >>> 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.
I've created something simple here. You can base your next series on u-boot-dm/pwm-working. http://patchwork.ozlabs.org/patch/751254/ >> >> >>> >>> 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. OK, so that's why it should a separate call. Seems OK to me. > >> >>> + 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? Did you see this one? Regards, Simon _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
