Skip to content
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

Closed
wants to merge 2 commits into from

Conversation

pdp7
Copy link
Collaborator

@pdp7 pdp7 commented Nov 28, 2023

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.

Documentation/devicetree/bindings/pwm/adi,axi-pwmgen.yaml Outdated Show resolved Hide resolved
drivers/pwm/Makefile Outdated Show resolved Hide resolved
Documentation/devicetree/bindings/pwm/adi,axi-pwmgen.yaml Outdated Show resolved Hide resolved

clocks:
maxItems: 1

Copy link
Collaborator

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.

Copy link
Collaborator

@nunojsa nunojsa left a 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);
}
Copy link
Collaborator

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).

Copy link
Collaborator Author

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.

drivers/pwm/pwm-axi-pwmgen.c Outdated Show resolved Hide resolved
drivers/pwm/pwm-axi-pwmgen.c Outdated Show resolved Hide resolved
struct pwm_state *state)
{
struct axi_pwmgen *pwmgen = to_axi_pwmgen(chip);
unsigned long rate = clk_get_rate(pwmgen->clk);
Copy link
Collaborator

@nunojsa nunojsa Nov 28, 2023

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.

Copy link
Collaborator Author

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?

Copy link
Collaborator

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

drivers/pwm/pwm-axi-pwmgen.c Outdated Show resolved Hide resolved
drivers/pwm/pwm-axi-pwmgen.c Show resolved Hide resolved
MAINTAINERS Show resolved Hide resolved
@pdp7
Copy link
Collaborator Author

pdp7 commented Nov 29, 2023

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.

@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?

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

Okay, good to know.

@nunojsa
Copy link
Collaborator

nunojsa commented Nov 30, 2023

Do you know which parts need the time_unit and phase functionality?

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]>
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]>
@pamolloy
Copy link
Collaborator

Closing this since it has been superseded by #2395

@pamolloy pamolloy closed this Jan 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants