-
Notifications
You must be signed in to change notification settings - Fork 860
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add AXI PWM generator driver to mainline #2363
Conversation
|
||
clocks: | ||
maxItems: 1 | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The docs also show an external sync signal. So for complete bindings, it seems like we should that listed here too as an optional property. Not sure if it makes sense to call it a gpio or something else since it could be connected to just about anything in the fabric.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some comments but it looks good to me in general. One thing that worries me though is the plan? is the plan to first have this simpler version in and then build on top of it?
I'm asking because one of the main goals of having this upstream is to also take care of the out of tree changes we have in the pwm core code. On top of it, I'm pretty sure we need things like phase and time_unit are needed in the projects using this core (and the core supports them). For the phase
I don't think it should be a big problem. The time_unit
stuff might be more questionable but I never gave it much thought. But both additions should be doable upstream and we do have a user for them.
This pull request is raised against spi/for-6.8 as that is the only upstream branch I could find in this repo.
This is fine for now but note that once accepted upstream, we want to backport it to our master and hence 6.1
unsigned int value) | ||
{ | ||
writel(value, pwm->base + reg); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: I would likely just use MMIO regmap... Just easier and then no need for any axi_pwmgen_write_mask()
. Not needed for now, but with regmap we can also build on top of it in terms of allowed registers to read/write (or ranges).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, I'll take a look at converting to MMIO regmap.
struct pwm_state *state) | ||
{ | ||
struct axi_pwmgen *pwmgen = to_axi_pwmgen(chip); | ||
unsigned long rate = clk_get_rate(pwmgen->clk); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you should sanity check this for !rate... Otherwise you end up with division by 0 exception. I would have to double check our projects but I wonder if it's ever the case that the clock changes. Thinking about reading it once during probe and be done with it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would have to double check our projects
Do you mean the HDL projects?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you mean the HDL projects?
I mean the devicetrees. Maybe the clock is just a fixed clock. Anyways, not that important. Like this is also fine. Just sanity check the rate
@dlech and I had been focused on the functionality needed to upstream drivers like ad400x. Thus this simpler version seemed to be a good strategy to upstream first. Do you know which parts need the time_unit and phase functionality?
Okay, good to know. |
I know at least this one is using it: https://github.com/analogdevicesinc/linux/blob/master/drivers/iio/adc/ad4630.c#L349 This one depends on the offload engine and these specific pwm bits. Anyways, if this is something you don't need to upstream, then it's ok. Eventually I'll have the time to take care of it :) |
Add bindings for Analog Devices AXI PWM generator. Link: https://wiki.analog.com/resources/fpga/docs/axi_pwm_gen Signed-off-by: Drew Fustini <[email protected]>
5555502
to
6508df9
Compare
Add support for the Analog Devices AXI PWM Generator. This device is an FPGA-implemented peripheral used as PWM signal generator and can be interfaced with AXI4. The register map of this peripheral makes it possible to configure the period and duty cycle of the output signal. Link: https://wiki.analog.com/resources/fpga/docs/axi_pwm_gen Co-developed-by: Sergiu Cuciurean <[email protected]> Signed-off-by: Sergiu Cuciurean <[email protected]> Co-developed-by: David Lechner <[email protected]> Signed-off-by: David Lechner <[email protected]> Signed-off-by: Drew Fustini <[email protected]>
6508df9
to
a53d3c9
Compare
Closing this since it has been superseded by #2395 |
The AXI PWM generator driver has been taken from adi/master and updated for mainline by @dlech and me. I've also created a DT binding.
This pull request is raised against spi/for-6.8 as that is the only upstream branch I could find in this repo.
The axi-pwmgen driver is related to the SPI offload upstreaming that @dlech has been doing in #2341 as both are required to upstream drivers like the ad400x.