lkml.org 
[lkml]   [2017]   [Apr]   [12]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH v5 1/4] gpio: mvebu: Add limited PWM support
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

\
 
 \ /
  Last update: 2017-04-12 16:31    [W:0.093 / U:9.340 seconds]
©2003-2018 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site