| Message ID | 20230803085734.340-1-nylon.chen@sifive.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Change PWM-controlled LED pin active mode and algorithm | expand |
On Thu, Aug 03, 2023 at 04:57:33PM +0800, Nylon Chen wrote: > According to the circuit diagram of User LEDs - RGB described in themanual hifive-unleashed-a00.pdf[0] and hifive-unmatched-schematics-v3.pdf[1]. > > The behavior of PWM is acitve-high. > > Removed patches: 1 > New patches: (none) > > Links: > - [0]: https://sifive.cdn.prismic.io/sifive/c52a8e32-05ce-4aaf-95c8-7bf8453f8698_hifive-unleashed-a00-schematics-1.pdf > - [1]: https://sifive.cdn.prismic.io/sifive/6a06d6c0-6e66-49b5-8e9e-e68ce76f4192_hifive-unmatched-schematics-v3.pdf > - [2]: https://sifive.cdn.prismic.io/sifive/1a82e600-1f93-4f41-b2d8-86ed8b16acba_fu740-c000-manual-v1p6.pdf > > Changed in v4: > - Remove previous updates to the PWM algorithm. Why? I don't recall the conclusion on the previous version being that that patch was not needed.
Hey Nylon, (I yoinked the reply to 1/1 to here, as it makes more sense in this context) > On Thu, Aug 03, 2023 at 10:15:10AM +0100, Conor Dooley wrote: > > On Thu, Aug 03, 2023 at 04:57:33PM +0800, Nylon Chen wrote: > > > According to the circuit diagram of User LEDs - RGB described in themanual hifive-unleashed-a00.pdf[0] and hifive-unmatched-schematics-v3.pdf[1]. > > > > > > The behavior of PWM is acitve-high. > > > > > > Removed patches: 1 > > > New patches: (none) > > > > > > Links: > > > - [0]: https://sifive.cdn.prismic.io/sifive/c52a8e32-05ce-4aaf-95c8-7bf8453f8698_hifive-unleashed-a00-schematics-1.pdf > > > - [1]: https://sifive.cdn.prismic.io/sifive/6a06d6c0-6e66-49b5-8e9e-e68ce76f4192_hifive-unmatched-schematics-v3.pdf > > > - [2]: https://sifive.cdn.prismic.io/sifive/1a82e600-1f93-4f41-b2d8-86ed8b16acba_fu740-c000-manual-v1p6.pdf > > > > > > Changed in v4: > > > - Remove previous updates to the PWM algorithm. > > > > Why? I don't recall the conclusion on the previous version being that > > that patch was not needed. > > I apologize for forgetting about this update earlier. Just now, > I tried to pull rebase master and noticed that other developers seem > to have made some fixes to the algorithm. Upon closer inspection, I > found that they addressed the part we previously discussed with Emil > and Uwe, such as "first pwm_apply_state." > > Therefore, my instinct tells me that they have already taken care of > the issues we discussed before. I didn't see anything in linux-next that would solve this problem of inversion. The last meaningful change is: commit 334c7b13d38321e47d1a51dba0bef9f4c403ec75 Author: Emil Renner Berthing <emil.renner.berthing@canonical.com> AuthorDate: Wed Nov 9 12:37:24 2022 +0100 Commit: Thierry Reding <thierry.reding@gmail.com> CommitDate: Mon Jan 30 16:42:45 2023 +0100 pwm: sifive: Always let the first pwm_apply_state succeed which predates your v3 by quite a bit. > I will review the conflicting parts in the pwm-sifive.c code in my v4 > version once again to ensure there are no omissions. If I find any, I > will submit v5 accordingly. And if this patch is okay in isolation, please reply here explaining which commit fixed the algorithm, so that I can pick it up. Thanks, Conor.
Hi Conor, Conor Dooley <conor.dooley@microchip.com> 於 2023年8月3日 週四 下午5:44寫道： > > Hey Nylon, > > (I yoinked the reply to 1/1 to here, as it makes more sense in this > context) > > > On Thu, Aug 03, 2023 at 10:15:10AM +0100, Conor Dooley wrote: > > > On Thu, Aug 03, 2023 at 04:57:33PM +0800, Nylon Chen wrote: > > > > According to the circuit diagram of User LEDs - RGB described in themanual hifive-unleashed-a00.pdf[0] and hifive-unmatched-schematics-v3.pdf[1]. > > > > > > > > The behavior of PWM is acitve-high. > > > > > > > > Removed patches: 1 > > > > New patches: (none) > > > > > > > > Links: > > > > - [0]: https://sifive.cdn.prismic.io/sifive/c52a8e32-05ce-4aaf-95c8-7bf8453f8698_hifive-unleashed-a00-schematics-1.pdf > > > > - [1]: https://sifive.cdn.prismic.io/sifive/6a06d6c0-6e66-49b5-8e9e-e68ce76f4192_hifive-unmatched-schematics-v3.pdf > > > > - [2]: https://sifive.cdn.prismic.io/sifive/1a82e600-1f93-4f41-b2d8-86ed8b16acba_fu740-c000-manual-v1p6.pdf > > > > > > > > Changed in v4: > > > > - Remove previous updates to the PWM algorithm. > > > > > > Why? I don't recall the conclusion on the previous version being that > > > that patch was not needed. > > > > I apologize for forgetting about this update earlier. Just now, > > I tried to pull rebase master and noticed that other developers seem > > to have made some fixes to the algorithm. Upon closer inspection, I > > found that they addressed the part we previously discussed with Emil > > and Uwe, such as "first pwm_apply_state." > > > > Therefore, my instinct tells me that they have already taken care of > > the issues we discussed before. > > I didn't see anything in linux-next that would solve this problem of > inversion. The last meaningful change is: > commit 334c7b13d38321e47d1a51dba0bef9f4c403ec75 > Author: Emil Renner Berthing <emil.renner.berthing@canonical.com> > AuthorDate: Wed Nov 9 12:37:24 2022 +0100 > Commit: Thierry Reding <thierry.reding@gmail.com> > CommitDate: Mon Jan 30 16:42:45 2023 +0100 > > pwm: sifive: Always let the first pwm_apply_state succeed > > which predates your v3 by quite a bit. > > > I will review the conflicting parts in the pwm-sifive.c code in my v4 > > version once again to ensure there are no omissions. If I find any, I > > will submit v5 accordingly. > > And if this patch is okay in isolation, please reply here explaining > which commit fixed the algorithm, so that I can pick it up. This patch needs to be accompanied by modifications to the pwm_sifive_apply() function to make sense. I will double-check and review the previous discussions to ensure that. Thank you for your response. > > Thanks, > Conor.
Hi Conor,
Thank you for patiently giving me advice. I appreciate your help.
Not long ago, I said, "This patch needs to be accompanied by
modifications to the pwm_sifive_apply() function to make sense."
I recently reviewed the v3 version, and after discussing it with Emil,
there are several areas that require modification. I will provide the
necessary changes for each of them:
1. polarity check. (Suggestion from Uwe)
- if (state->polarity != PWM_POLARITY_INVERSED)
+ if (state->polarity != PWM_POLARITY_NORMAL)
2. avoid using old periodperiod, not state->period
- period = max(state->period, ddata->approx_period);
- frac = DIV64_U64_ROUND_CLOSEST(num, state->period);
+ frac = DIV64_U64_ROUND_CLOSEST(num, period);
3. add a conditional check can be added in the code to set
ddata->approx_period to state->period when state->period is smaller
than ddata->approx_period
if (state->period != ddata->approx_period) {
...
+ if (state->period < ddata->approx_period) {
+ ddata->approx_period = state->period;
+ }
- ddata->approx_period = state->period;
+ period = ddata->approx_period;
I will use 'unmatched' on my end to verify again. If there are any
other errors, feel free to point them out. Thank you.
Nylon Chen <nylon.chen@sifive.com> 於 2023年8月4日 週五 上午9:42寫道：
>
> Hi Conor,
>
> Conor Dooley <conor.dooley@microchip.com> 於 2023年8月3日 週四 下午5:44寫道：
> >
> > Hey Nylon,
> >
> > (I yoinked the reply to 1/1 to here, as it makes more sense in this
> > context)
> >
> > > On Thu, Aug 03, 2023 at 10:15:10AM +0100, Conor Dooley wrote:
> > > > On Thu, Aug 03, 2023 at 04:57:33PM +0800, Nylon Chen wrote:
> > > > > According to the circuit diagram of User LEDs - RGB described in themanual hifive-unleashed-a00.pdf[0] and hifive-unmatched-schematics-v3.pdf[1].
> > > > >
> > > > > The behavior of PWM is acitve-high.
> > > > >
> > > > > Removed patches: 1
> > > > > New patches: (none)
> > > > >
> > > > > Links:
> > > > > - [0]: https://sifive.cdn.prismic.io/sifive/c52a8e32-05ce-4aaf-95c8-7bf8453f8698_hifive-unleashed-a00-schematics-1.pdf
> > > > > - [1]: https://sifive.cdn.prismic.io/sifive/6a06d6c0-6e66-49b5-8e9e-e68ce76f4192_hifive-unmatched-schematics-v3.pdf
> > > > > - [2]: https://sifive.cdn.prismic.io/sifive/1a82e600-1f93-4f41-b2d8-86ed8b16acba_fu740-c000-manual-v1p6.pdf
> > > > >
> > > > > Changed in v4:
> > > > > - Remove previous updates to the PWM algorithm.
> > > >
> > > > Why? I don't recall the conclusion on the previous version being that
> > > > that patch was not needed.
> > >
> > > I apologize for forgetting about this update earlier. Just now,
> > > I tried to pull rebase master and noticed that other developers seem
> > > to have made some fixes to the algorithm. Upon closer inspection, I
> > > found that they addressed the part we previously discussed with Emil
> > > and Uwe, such as "first pwm_apply_state."
> > >
> > > Therefore, my instinct tells me that they have already taken care of
> > > the issues we discussed before.
> >
> > I didn't see anything in linux-next that would solve this problem of
> > inversion. The last meaningful change is:
> > commit 334c7b13d38321e47d1a51dba0bef9f4c403ec75
> > Author: Emil Renner Berthing <emil.renner.berthing@canonical.com>
> > AuthorDate: Wed Nov 9 12:37:24 2022 +0100
> > Commit: Thierry Reding <thierry.reding@gmail.com>
> > CommitDate: Mon Jan 30 16:42:45 2023 +0100
> >
> > pwm: sifive: Always let the first pwm_apply_state succeed
> >
> > which predates your v3 by quite a bit.
> >
> > > I will review the conflicting parts in the pwm-sifive.c code in my v4
> > > version once again to ensure there are no omissions. If I find any, I
> > > will submit v5 accordingly.
> >
> > And if this patch is okay in isolation, please reply here explaining
> > which commit fixed the algorithm, so that I can pick it up.
> This patch needs to be accompanied by modifications to the
> pwm_sifive_apply() function to make sense.
>
> I will double-check and review the previous discussions to ensure
> that. Thank you for your response.
> >
> > Thanks,
> > Conor.
On Fri, Aug 04, 2023 at 02:54:33PM +0800, Nylon Chen wrote: > Hi Conor, > > Thank you for patiently giving me advice. I appreciate your help. > > Not long ago, I said, "This patch needs to be accompanied by > modifications to the pwm_sifive_apply() function to make sense." > > I recently reviewed the v3 version, and after discussing it with Emil, > there are several areas that require modification. I will provide the > necessary changes for each of them: > > 1. polarity check. (Suggestion from Uwe) > - if (state->polarity != PWM_POLARITY_INVERSED) > + if (state->polarity != PWM_POLARITY_NORMAL) > 2. avoid using old periodperiod, not state->period > - period = max(state->period, ddata->approx_period); > - frac = DIV64_U64_ROUND_CLOSEST(num, state->period); > + frac = DIV64_U64_ROUND_CLOSEST(num, period); > 3. add a conditional check can be added in the code to set > ddata->approx_period to state->period when state->period is smaller > than ddata->approx_period > if (state->period != ddata->approx_period) { > ... > + if (state->period < ddata->approx_period) { > + ddata->approx_period = state->period; > + } > - ddata->approx_period = state->period; > + period = ddata->approx_period; > > I will use 'unmatched' on my end to verify again. If there are any > other errors, feel free to point them out. Thank you. I'm not sure of the driver details without going and looking into the code itself, but this sounds like it makes a lot more sense than just flipping the polarity in the dts. Thanks for taking another look!
According to the circuit diagram of User LEDs - RGB described in themanual hifive-unleashed-a00.pdf[0] and hifive-unmatched-schematics-v3.pdf[1]. The behavior of PWM is acitve-high. Removed patches: 1 New patches: (none) Links: - [0]: https://sifive.cdn.prismic.io/sifive/c52a8e32-05ce-4aaf-95c8-7bf8453f8698_hifive-unleashed-a00-schematics-1.pdf - [1]: https://sifive.cdn.prismic.io/sifive/6a06d6c0-6e66-49b5-8e9e-e68ce76f4192_hifive-unmatched-schematics-v3.pdf - [2]: https://sifive.cdn.prismic.io/sifive/1a82e600-1f93-4f41-b2d8-86ed8b16acba_fu740-c000-manual-v1p6.pdf Changed in v4: - Remove previous updates to the PWM algorithm. Changed in v3: - Convert the reference link to standard link. - Move the inverted function before taking the minimum value. - Change polarity check condition(high and low). - Pick the biggest period length possible that is not bigger than the requested period. Changed in v2: - Convert the reference link to standard link. - Fix typo: s/sifive unmatched:/sifive: unmatched:/. - Remove active-low from hifive-unleashed-a00.dts. - Include this reference link in the dts and pwm commit messages. Nylon Chen (1): riscv: dts: sifive: unleashed/unmatched: Remove PWM controlled LED's active-low properties arch/riscv/boot/dts/sifive/hifive-unleashed-a00.dts | 4 ---- arch/riscv/boot/dts/sifive/hifive-unmatched-a00.dts | 4 ---- 2 files changed, 8 deletions(-)