Hello,
Sorry for the late feedback about this.
On Sun, 9 Apr 2017 20:09:27 +0200, Ralph Sennhauser wrote:
> + gpio1: gpio@18140 {
> + compatible = "marvell,armada-370-xp-gpio";
> + reg = <0x18140 0x40>, <0x181c8 0x08>;
One issue I see is that you have only two counters A and B. You
associate counter A with the first bank of GPIOs, and counter B with
the second bank of GPIOs.
Which means that if you need to PWM a GPIO from the third bank of
GPIOs, you can't, even if the HW allows it.
While I'm fine with not supporting all the HW features, but it's a bit
sad that this gets encoded into the DT.
But I guess the only way to make this possible would be to have a
single node for all GPIOs rather than one per bank? Or do we have a way
to have those counter A/B registers bound to a separate PWM driver, and
then the GPIO driver being smart enough to select the counter to be
used? Seems not easy to do though :-/
> +struct mvebu_pwm {
> + void __iomem *membase;
> + unsigned long clk_rate;
> + bool used;
> + struct pwm_chip chip;
> + spinlock_t lock;
> + struct mvebu_gpio_chip *mvchip;
> +
> + /* Used to preserve GPIO/PWM registers across suspend/resume */
> + u32 blink_select;
> + u32 blink_on_duration;
> + u32 blink_off_duration;
> +};
> +
> struct mvebu_gpio_chip {
> struct gpio_chip chip;
> spinlock_t lock;
> @@ -87,6 +113,10 @@ struct mvebu_gpio_chip {
> struct irq_domain *domain;
> int soc_variant;
>
> + /* Used for PWM support */
> + struct clk *clk;
> + struct mvebu_pwm *mvpwm;
Why does mvpwm needs to be allocated separately? Why not directly embed
it inside the mvebu_gpio_chip structure?
Do we need a separate spinlock?
> @@ -555,6 +842,10 @@ static const struct of_device_id mvebu_gpio_of_match[] =
> {
> .data = (void *) MVEBU_GPIO_SOC_VARIANT_ARMADAXP,
> },
> {
> + .compatible = "marvell,armada-370-xp-gpio",
> + .data = (void *) MVEBU_GPIO_SOC_VARIANT_ORION,
Whum, what are you doing here?
Thanks,
Thomas
--
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
