[v3,0/2] Add support for Microchip's pwm fpga core

Message

Conor Dooley June 17, 2022, 11:44 a.m. UTC

Hey Uwe,
Got a ~v2~ v3 for you...
I added some comments explaining the calculations and a documentation link
so hopefully things are a bit easier to follow.

Code wise, I went through and sorted out a bunch of issues that cycling
through the different periods/duties threw up. Along the way I found
some other problems - especially with the longer periods which I have
fixed. I also added a write to the sync register in the apply function,
which will resolve to a NOP for channels without "shadow registers".

Other than that, I managed to ditch the mchp_core_pwm_registers struct
entirely but had to add a short delay before reading back the registers
in order to compute the duty.

Thanks,
Conor.

Changes from v2:
- fix percieved idempotency and rounding issues when shadow registers were
  enabled
- use do_div() for divide of u64 tmp in apply_period()

Changes from v1:
- account for edge "quirk" while inverted
- block changing enabled channels' period
- document the hardware/driver limitations
- rearrange get_state() more logically
- fix cast sizes in get_state()
- fix remove() and probe error paths
- delete mchp_core_pwm_registers
- simplify .apply() logic
- don't warn in calculate_base()
- fix period calculation
- fix duty cycle calculation
- add COREPWM prefix to defines
- add a documentation link

Conor Dooley (2):
  pwm: add microchip soft ip corePWM driver
  MAINTAINERS: add pwm to PolarFire SoC entry

 MAINTAINERS                      |   1 +
 drivers/pwm/Kconfig              |  10 +
 drivers/pwm/Makefile             |   1 +
 drivers/pwm/pwm-microchip-core.c | 325 +++++++++++++++++++++++++++++++
 4 files changed, 337 insertions(+)
 create mode 100644 drivers/pwm/pwm-microchip-core.c


base-commit: 61114e734ccb804bc12561ab4020745e02c468c2

Comments

Uwe Kleine-König June 17, 2022, 1:09 p.m. UTC | #1

Hello,

On Fri, Jun 17, 2022 at 11:50:13AM +0000, [e-mail] wrote:
> On 17/06/2022 12:44, Conor Dooley wrote:
> > Hey Uwe,
> > Got a ~v2~ v3 for you...
> > I added some comments explaining the calculations and a documentation link
> > so hopefully things are a bit easier to follow.
> > 
> > Code wise, I went through and sorted out a bunch of issues that cycling
> > through the different periods/duties threw up. Along the way I found
> > some other problems - especially with the longer periods which I have
> > fixed. I also added a write to the sync register in the apply function,
> > which will resolve to a NOP for channels without "shadow registers".
> > 
> > Other than that, I managed to ditch the mchp_core_pwm_registers struct
> > entirely but had to add a short delay before reading back the registers
> > in order to compute the duty.
> > 
> > Thanks,
> > Conor.
> 
> *sigh* yet again I forgot to mention the potential maintainers conflict
> with spi-next..

I'm not a maintainer of a very active subsystem where MAINTAINER
conflicts are normal, but my expectation up to now was that conflicts in
that file are quite usual and trivial to resolve such that mentioning
these isn't very important.

Best regards
Uwe

Conor Dooley June 17, 2022, 1:13 p.m. UTC | #2

On 17/06/2022 14:09, Uwe Kleine-König wrote:
> Hello,
> 
> On Fri, Jun 17, 2022 at 11:50:13AM +0000, [e-mail] wrote:
>> On 17/06/2022 12:44, Conor Dooley wrote:
>>> Hey Uwe,
>>> Got a ~v2~ v3 for you...
>>> I added some comments explaining the calculations and a documentation link
>>> so hopefully things are a bit easier to follow.
>>>
>>> Code wise, I went through and sorted out a bunch of issues that cycling
>>> through the different periods/duties threw up. Along the way I found
>>> some other problems - especially with the longer periods which I have
>>> fixed. I also added a write to the sync register in the apply function,
>>> which will resolve to a NOP for channels without "shadow registers".
>>>
>>> Other than that, I managed to ditch the mchp_core_pwm_registers struct
>>> entirely but had to add a short delay before reading back the registers
>>> in order to compute the duty.
>>>
>>> Thanks,
>>> Conor.
>>
>> *sigh* yet again I forgot to mention the potential maintainers conflict
>> with spi-next..
> 
> I'm not a maintainer of a very active subsystem where MAINTAINER
> conflicts are normal, but my expectation up to now was that conflicts in
> that file are quite usual and trivial to resolve such that mentioning
> these isn't very important.
> 

Yeah, I figured such conflicts were rather normal but felt like it was
better to say it just in case.
Thanks,
Conor.