Hello, Regarding the Subject: The patch has nothing to do with an LED, has it? On Fri, Jan 26, 2024 at 03:40:44PM +0800, Nylon Chen wrote: > The `frac` variable represents the pulse inactive time, and the result > of this algorithm is the pulse active time. Therefore, we must reverse the result. Please break lines at 75 columns in the commit log. > The reference is SiFive FU740-C000 Manual[0] > > Link: https://sifive.cdn.prismic.io/sifive/1a82e600-1f93-4f41-b2d8-86ed8b16acba_fu740-c000-manual-v1p6.pdf [0] I looked at Figure 29 in this document (version v1p6, pdf page 148). Not sure I understand that correctly, but I expect that the output of the ">=?" node below pwmcmp0 to become 1 if pwms has reached pwmcmp0, is that right? In that case this output is zero when pwmcount is zero and then pwmcmp0ip is zero, too. So a period starts with the inactive part and so it's inversed polarity. What made you think that the current driver implementation is wrong? > Co-developed-by: Zong Li <zong.li@xxxxxxxxxx> > Signed-off-by: Zong Li <zong.li@xxxxxxxxxx> > Co-developed-by: Vincent Chen <vincent.chen@xxxxxxxxxx> > Signed-off-by: Vincent Chen <vincent.chen@xxxxxxxxxx> > Signed-off-by: Nylon Chen <nylon.chen@xxxxxxxxxx> > --- > drivers/pwm/pwm-sifive.c | 7 ++++--- > 1 file changed, 4 insertions(+), 3 deletions(-) > > diff --git a/drivers/pwm/pwm-sifive.c b/drivers/pwm/pwm-sifive.c > index eabddb7c7820..b07c8598bb21 100644 > --- a/drivers/pwm/pwm-sifive.c > +++ b/drivers/pwm/pwm-sifive.c > @@ -113,6 +113,7 @@ static int pwm_sifive_get_state(struct pwm_chip *chip, struct pwm_device *pwm, > u32 duty, val; > > duty = readl(ddata->regs + PWM_SIFIVE_PWMCMP(pwm->hwpwm)); > + duty = (1U << PWM_SIFIVE_CMPWIDTH) - 1 - duty; I find it irritating that both values are assigned to duty. I'd spend another variable and make this: inactive = readl(ddata->regs + PWM_SIFIVE_PWMCMP(pwm->hwpwm)); duty = (1U << PWM_SIFIVE_CMPWIDTH) - 1 - inactive; > > state->enabled = duty > 0; > > @@ -123,11 +124,10 @@ static int pwm_sifive_get_state(struct pwm_chip *chip, struct pwm_device *pwm, > state->period = ddata->real_period; > state->duty_cycle = > (u64)duty * ddata->real_period >> PWM_SIFIVE_CMPWIDTH; > - state->polarity = PWM_POLARITY_INVERSED; > + state->polarity = PWM_POLARITY_NORMAL; > > return 0; > } > - Please keep this empty line between functions. > static int pwm_sifive_apply(struct pwm_chip *chip, struct pwm_device *pwm, > const struct pwm_state *state) > { Best regards Uwe -- Pengutronix e.K. | Uwe Kleine-König | Industrial Linux Solutions | https://www.pengutronix.de/ |
Attachment:
signature.asc
Description: PGP signature
