Re: [PATCH 00/13] Ingenic JZ4740 / JZ4780 pinctrl driver

Re: [PATCH 00/13] Ingenic JZ4740 / JZ4780 pinctrl driver

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 2322 bytes --]

On Wed, Jan 18, 2017 at 12:14:08AM +0100, Paul Cercueil wrote:
[...]
> One problem still unresolved: the pinctrl framework does not allow us to
> configure each pin on demand (someone please prove me wrong), when the
> various PWM channels are requested or released. For instance, the PWM
> channels can be configured from sysfs, which would require all PWM pins
> to be configured properly beforehand for the PWM function, eventually
> causing conflicts with other platform or board drivers.

Still catching up on a lot of email, so I haven't gone through the
entire series. But I don't think the above is true.

My understanding is that you can have separate pin groups for each
pin (provided the hardware supports that) and then control each of
these groups dynamically at runtime.

That is you could have the PWM driver's ->request() and ->free()
call into the pinctrl framework to select the correct pinmux
configuration as necessary.

> The proper solution here would be to modify the pwm-jz4740 driver to
> handle only one PWM channel, and create an instance of this driver
> for each one of the 8 PWM channels. Then, it could use the pinctrl
> framework to dynamically configure the PWM pin it controls.

That could probably work. From only looking at the JZ4740 PWM driver
there's no separate IP block to deal with the PWM outputs, but they are
merely GPIOs controller via a timer, so one instance per GPIO seems like
a fine solution to me.

> Until this can be done, the only jz4740 board supported upstream
> (Qi lb60) could configure all of its connected PWM pins in PWM function
> mode, if those are not used by other drivers nor by GPIOs on the
> board. The only jz4780 board upstream (CI20) does not yet support the
> PWM driver.

Typically all of the pinmux is pre-determined by the board design. That
is if you've got 8 pins that can be driven by a PWM signal, not all of
those might be exposed by the design. If, say, only 0-4 and 6 expose the
PWM signal while 5 and 7 expose a different function then you can simply
use a static pinmux configuration and ignore PWMs 5 and 7. Even if
someone were to configure them, the signal would simply go nowhere.

Of course you'd have to check that your hardware actually matches those
assumptions. They certainly apply to many SoCs that I've come across.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 12/13] pwm: jz4740: Let the pinctrl driver configure the pins

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 1857 bytes --]

On Wed, Jan 18, 2017 at 12:14:20AM +0100, Paul Cercueil wrote:
> Now that the JZ4740 and similar SoCs have a pinctrl driver, we rely on
> the pins being properly configured before the driver probes.
> 
> One inherent problem of this new approach is that the pinctrl framework
> does not allow us to configure each pin on demand, when the various PWM
> channels are requested or released. For instance, the PWM channels can
> be configured from sysfs, which would require all PWM pins to be configured
> properly beforehand for the PWM function, eventually causing conflicts
> with other platform or board drivers.
> 
> The proper solution here would be to modify the pwm-jz4740 driver to
> handle only one PWM channel, and create an instance of this driver
> for each one of the 8 PWM channels. Then, it could use the pinctrl
> framework to dynamically configure the PWM pin it controls.
> 
> Until this can be done, the only jz4740 board supported upstream
> (Qi lb60) could configure all of its connected PWM pins in PWM function
> mode, if those are not used by other drivers nor by GPIOs on the
> board.
> 
> Signed-off-by: Paul Cercueil <paul@crapouillou.net>
> ---
>  drivers/pwm/pwm-jz4740.c | 29 -----------------------------
>  1 file changed, 29 deletions(-)
> 
> diff --git a/drivers/pwm/pwm-jz4740.c b/drivers/pwm/pwm-jz4740.c
> index 76d13150283f..a75ff3622450 100644
> --- a/drivers/pwm/pwm-jz4740.c
> +++ b/drivers/pwm/pwm-jz4740.c
> @@ -21,22 +21,10 @@
>  #include <linux/platform_device.h>
>  #include <linux/pwm.h>
>  
> -#include <asm/mach-jz4740/gpio.h>

What about the linux/gpio.h header? It seems to me like that would be no
longer needed after this patch either.

Other than that this looks like the patch I'd expect if the pinmux was
configured statically, based on board design.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 13/13] MIPS: jz4740: Remove custom GPIO code

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 1641 bytes --]

On Wed, Jan 18, 2017 at 12:14:21AM +0100, Paul Cercueil wrote:
> All the drivers for the various hardware elements of the jz4740 SoC have
> been modified to use the pinctrl framework for their pin configuration
> needs.
> As such, this platform code is now unused and can be deleted.
> 
> Signed-off-by: Paul Cercueil <paul@crapouillou.net>
> ---
>  arch/mips/include/asm/mach-jz4740/gpio.h | 371 ----------------------
>  arch/mips/jz4740/Makefile                |   2 -
>  arch/mips/jz4740/gpio.c                  | 519 -------------------------------
>  3 files changed, 892 deletions(-)
>  delete mode 100644 arch/mips/jz4740/gpio.c

Have you considered how this could best be merged? It's probably easiest
to take all of this through the MIPS tree because we have an addition of
the pinctrl that would be a replacement for the GPIO code, while at the
same time a bunch of drivers rely on the JZ4740 GPIO code for successful
compilation.

That's slightly complicated by the number of drivers, so needs a lot of
coordination, but it's not the worst I've seen.

Maybe one other solution that would make the transition easier would be
to stub out the GPIO functions if the pinctrl driver is enabled, and do
the removal of the mach-jz4740/gpio.h after all drivers have been merged
through their corresponding subsystem trees. That way all drivers should
remain compilable and functional at runtime, while still having the
possibility to merge through the subsystem trees.

That said, the whole series is fairly simple, so merging everything
through the MIPS tree sounds like the easiest way to go.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 02/13] pinctrl-jz4740: add a pinctrl driver for the Ingenic jz4740 SoC

[Linus Walleij]

n Wed, Jan 18, 2017 at 12:14 AM, Paul Cercueil <paul@crapouillou.net> wrote:

> From: Paul Burton <paul.burton@imgtec.com>
>
> This driver handles pin configuration, pin muxing, and GPIOs of the
> jz4740 SoC from Ingenic.
>
> It is separated into two files:
> - pinctrl-ingenic.c, which contains the core functions that can be
>   shared across all Ingenic SoCs,
> - pinctrl-jz4740.c, which contains the jz4740-pinctrl driver.
>
> The reason behind separating some functions out of the jz4740-pinctrl
> driver, is that the pin/GPIO controllers of the Ingenic SoCs are
> extremely similar across SoC versions, except that some have the
> registers shuffled around. Making a distinct separation will permit the
> reuse of large parts of the driver to support the other SoCs from
> Ingenic.
>
> Signed-off-by: Paul Cercueil <paul@crapouillou.net>

> diff --git a/drivers/pinctrl/ingenic/Kconfig b/drivers/pinctrl/ingenic/Kconfig
> new file mode 100644
> index 000000000000..9923ce127183
> --- /dev/null
> +++ b/drivers/pinctrl/ingenic/Kconfig
> @@ -0,0 +1,14 @@
> +#
> +# Ingenic SoCs pin control drivers
> +#
> +config PINCTRL_INGENIC
> +       bool
> +       select PINMUX
> +       select GPIOLIB_IRQCHIP
> +       select GENERIC_PINCONF

I like it already when it looks like that :D

> +#include <linux/compiler.h>
> +#include <linux/gpio.h>

For drivers, just
#include <linux/gpio/driver.h>

> +struct ingenic_gpio_chip {
> +       char name[3];
> +       unsigned int idx;
> +       void __iomem *base;
> +       struct gpio_chip gc;
> +       struct irq_chip irq_chip;
> +       struct ingenic_pinctrl *pinctrl;
> +       const struct ingenic_pinctrl_ops *ops;
> +       uint32_t pull_ups;
> +       uint32_t pull_downs;
> +       unsigned int irq;
> +       struct pinctrl_gpio_range grange;

Usually we add GPIO ranges from the device tree for device tree
drivers, look at the syntax in:
Documentation/devicetree/bindings/gpio/gpio.txt

git grep gpio-ranges arch/arm/boot/dts/
gives you a few examples.

> +#define gc_to_jzgc(gpiochip) \
> +       container_of(gpiochip, struct ingenic_gpio_chip, gc)

Unless you must have this, please switch to using
struct ingenic_gpio_chip *jzgc = gpiochip_get_data(gc);
and use [devm_]gpiochip_add_data() in the probe so
you can get the data from the gpiochip directly.

> +static void ingenic_gpio_set(struct gpio_chip *gc,
> +               unsigned int offset, int value)
> +{
> +       struct ingenic_gpio_chip *jzgc = gc_to_jzgc(gc);
> +
> +       jzgc->ops->gpio_set_value(jzgc->base, offset, value);
> +}

struct ingenic_gpio_chip *jzgc = gpiochip_get_data(gc);
etc everywhere.

> +static void ingenic_gpio_irq_ack(struct irq_data *irqd)
> +{
> +       struct gpio_chip *gc = irq_data_get_irq_chip_data(irqd);
> +       struct ingenic_gpio_chip *jzgc = gc_to_jzgc(gc);
> +       unsigned int high;
> +       int irq = irqd->hwirq;
> +
> +       if (irqd_get_trigger_type(irqd) == IRQ_TYPE_EDGE_BOTH) {
> +               /*
> +                * Switch to an interrupt for the opposite edge to the one that
> +                * triggered the interrupt being ACKed.
> +                */
> +               high = jzgc->ops->gpio_get_value(jzgc->base, irq);
> +               if (high)
> +                       jzgc->ops->irq_set_type(jzgc->base, irq,
> +                                       IRQ_TYPE_EDGE_FALLING);
> +               else
> +                       jzgc->ops->irq_set_type(jzgc->base, irq,
> +                                       IRQ_TYPE_EDGE_RISING);

Neat hack. This is often how you have to do it indeed.

> +static int ingenic_gpio_irq_set_type(struct irq_data *irqd, unsigned int type)
> +{
> +       struct gpio_chip *gc = irq_data_get_irq_chip_data(irqd);
> +       struct ingenic_gpio_chip *jzgc = gc_to_jzgc(gc);
> +
> +       switch (type) {
> +       case IRQ_TYPE_EDGE_BOTH:
> +       case IRQ_TYPE_EDGE_RISING:
> +       case IRQ_TYPE_EDGE_FALLING:
> +       case IRQ_TYPE_LEVEL_HIGH:
> +       case IRQ_TYPE_LEVEL_LOW:
> +               break;
> +       default:
> +               pr_err("unsupported external interrupt type\n");

Should you set the irq handlet to handle_bad_irq() in this case?
That's what I usually do.

> +               return -EINVAL;
> +       }
> +
> +       if (type & IRQ_TYPE_EDGE_BOTH)
> +               irq_set_handler_locked(irqd, handle_edge_irq);
> +       else
> +               irq_set_handler_locked(irqd, handle_level_irq);

Nice.

> +       jzgc->ops->irq_set_type(jzgc->base, irqd->hwirq, type);

Getting a bit of feeling that it's a bit much indirection vtable
business going on here, but depends on the series as a whole.

> +static int ingenic_gpio_irq_set_wake(struct irq_data *irqd, unsigned int on)
> +{
> +       struct gpio_chip *gc = irq_data_get_irq_chip_data(irqd);
> +       struct ingenic_gpio_chip *jzgc = gc_to_jzgc(gc);
> +
> +       return irq_set_irq_wake(jzgc->irq, on);
> +}

I'm uncertain with these. Allright I guess, I'm just too bad at understanding
wakeup IRQs.

> +static void ingenic_gpio_irq_handler(struct irq_desc *desc)
> +{
> +       struct gpio_chip *gc = irq_desc_get_handler_data(desc);
> +       struct ingenic_gpio_chip *jzgc = gc_to_jzgc(gc);
> +       struct irq_chip *irq_chip = irq_data_get_irq_chip(&desc->irq_data);
> +       unsigned long flag, i;
> +
> +       chained_irq_enter(irq_chip, desc);
> +       flag = jzgc->ops->irq_read(jzgc->base);
> +
> +       for_each_set_bit(i, &flag, 32)
> +               generic_handle_irq(irq_linear_revmap(gc->irqdomain, i));
> +       chained_irq_exit(irq_chip, desc);
> +}

Clean & nice.

> +static int ingenic_pinctrl_dt_node_to_map(
> +               struct pinctrl_dev *pctldev, struct device_node *np,
> +               struct pinctrl_map **map, unsigned int *num_maps)
> +{
> +       struct ingenic_pinctrl *jzpc = pinctrl_dev_get_drvdata(pctldev);
> +       struct ingenic_pinctrl_func *func;
> +       struct ingenic_pinctrl_group *group;
> +       struct pinctrl_map *new_map;
> +       unsigned int map_num, i;
> +
> +       group = find_group_by_of_node(jzpc, np);
> +       if (!group)
> +               return -EINVAL;
> +
> +       func = find_func_by_of_node(jzpc, of_get_parent(np));
> +       if (!func)
> +               return -EINVAL;
> +
> +       map_num = 1 + group->num_pins;
> +       new_map = devm_kzalloc(jzpc->dev,
> +                               sizeof(*new_map) * map_num, GFP_KERNEL);
> +       if (!new_map)
> +               return -ENOMEM;
> +
> +       new_map[0].type = PIN_MAP_TYPE_MUX_GROUP;
> +       new_map[0].data.mux.function = func->name;
> +       new_map[0].data.mux.group = group->name;
> +
> +       for (i = 0; i < group->num_pins; i++) {
> +               new_map[i + 1].type = PIN_MAP_TYPE_CONFIGS_PIN;
> +               new_map[i + 1].data.configs.group_or_pin =
> +                       jzpc->pdesc[group->pins[i].idx].name;
> +               new_map[i + 1].data.configs.configs = group->pins[i].configs;
> +               new_map[i + 1].data.configs.num_configs =
> +                       group->pins[i].num_configs;
> +       }
> +
> +       *map = new_map;
> +       *num_maps = map_num;
> +       return 0;
> +}

This may change due to DT bindings reviews. I would prefer if you use
generic functions.

> +static int ingenic_pinctrl_parse_dt_gpio(struct ingenic_pinctrl *jzpc,
> +               struct ingenic_gpio_chip *jzgc, struct device_node *np)

Naming here: parse or probe or init. This function is certainly not just
parsing the DT. init() or probe() is better.

> +       jzgc->gc.base = jzpc->base + (jzgc->idx * PINS_PER_GPIO_PORT);

No. No hard-coded GPIO bases on new GPIO driver.
Set this to -1 so it uses dynamic allocation of GPIO numbers.

> +       if (of_property_read_u32_index(np, "ingenic,pull-ups", 0,
> +                               &jzgc->pull_ups))
> +               jzgc->pull_ups = 0;
> +       if (of_property_read_u32_index(np, "ingenic,pull-downs", 0,
> +                               &jzgc->pull_downs))
> +               jzgc->pull_downs = 0;
> +
> +       if (jzgc->pull_ups & jzgc->pull_downs) {
> +               dev_err(jzpc->dev, "GPIO port %c has overlapping pull ups & pull downs\n",
> +                       'A' + jzgc->idx);
> +               return -EINVAL;
> +       }

These bindings look suspicious. But I will review them in the binding
document.

> +static int ingenic_pinctrl_parse_dt_pincfg(struct ingenic_pinctrl *jzpc,
> +               struct ingenic_pinctrl_pin *pin, phandle cfg_handle)
> +{
> +       struct device_node *cfg_node;
> +       int err;
> +
> +       cfg_node = of_find_node_by_phandle(cfg_handle);
> +       if (!cfg_node)
> +               return -EINVAL;
> +
> +       err = pinconf_generic_parse_dt_config(cfg_node, NULL,
> +                       &pin->configs, &pin->num_configs);
> +       if (err)
> +               return err;
> +
> +       err = devm_add_action(jzpc->dev, (void (*)(void *))kfree, pin->configs);

That looks very clever.

But when we have pinctrl_utils_free_map() and other helpers already
this free:ing looks like some reinvented wheel.

Can we create something that free:s the maps from
pinctrl_utils_reserve_map() in a similar way and use that?
Just thinking aloud.

> +static int ingenic_pinctrl_parse_dt_func(struct ingenic_pinctrl *jzpc,
> +               struct device_node *np, unsigned int *ifunc,
> +               unsigned int *igroup)
> +{
> +       struct ingenic_pinctrl_func *func;
> +       struct ingenic_pinctrl_group *grp;
> +       struct device_node *group_node, *gpio_node;
> +       struct gpio_chip *gpio_chip;
> +       phandle gpio_handle, cfg_handle;
> +       struct property *pp;
> +       __be32 *plist;
> +       unsigned int i, j;
> +       int err;
> +       const unsigned int vals_per_pin = 4;
> +
> +       func = &jzpc->funcs[(*ifunc)++];
> +       func->of_node = np;
> +       func->name = np->name;
> +
> +       func->num_groups = of_get_child_count(np);
> +       func->groups = devm_kzalloc(jzpc->dev, sizeof(*func->groups) *
> +                       func->num_groups, GFP_KERNEL);
> +       func->group_names = devm_kzalloc(jzpc->dev,
> +                       sizeof(*func->group_names) * func->num_groups,
> +                       GFP_KERNEL);
> +       if (!func->groups || !func->group_names)
> +               return -ENOMEM;
> +
> +       i = 0;
> +       for_each_child_of_node(np, group_node) {
> +               pp = of_find_property(group_node, "ingenic,pins", NULL);
> +               if (!pp)
> +                       return -EINVAL;
> +               if ((pp->length / sizeof(__be32)) % vals_per_pin)
> +                       return -EINVAL;
> +
> +               grp = &jzpc->groups[(*igroup)++];
> +               grp->of_node = group_node;
> +               grp->name = group_node->name;
> +               grp->num_pins = (pp->length / sizeof(__be32)) / vals_per_pin;
> +               grp->pins = devm_kzalloc(jzpc->dev, sizeof(*grp->pins) *
> +                               grp->num_pins, GFP_KERNEL);
> +               grp->pin_indices = devm_kzalloc(jzpc->dev,
> +                               sizeof(*grp->pin_indices) * grp->num_pins,
> +                               GFP_KERNEL);
> +               if (!grp->pins)
> +                       return -EINVAL;
> +
> +               plist = pp->value;
> +               for (j = 0; j < grp->num_pins; j++) {
> +                       gpio_handle = be32_to_cpup(plist++);
> +                       grp->pins[j].idx = be32_to_cpup(plist++);
> +                       grp->pins[j].func = be32_to_cpup(plist++);
> +                       cfg_handle = be32_to_cpup(plist++);
> +
> +                       gpio_node = of_find_node_by_phandle(gpio_handle);
> +                       if (!gpio_node)
> +                               return -EINVAL;
> +
> +                       gpio_chip = gpiochip_find(gpio_node,
> +                                       find_gpio_chip_by_of_node);
> +                       if (!gpio_chip)
> +                               return -EINVAL;
> +
> +                       grp->pins[j].gpio_chip = gc_to_jzgc(gpio_chip);
> +
> +                       err = ingenic_pinctrl_parse_dt_pincfg(jzpc,
> +                                       &grp->pins[j], cfg_handle);
> +                       if (err)
> +                               return err;
> +
> +                       grp->pins[j].idx += grp->pins[j].gpio_chip->idx *
> +                               PINS_PER_GPIO_PORT;
> +                       grp->pin_indices[j] = grp->pins[j].idx;
> +               }
> +
> +               func->groups[i] = grp;
> +               func->group_names[i] = grp->name;
> +               i++;
> +       }
> +
> +       return 0;
> +}

Tony Lindgren has added generic function and group parsing for drivers
that keep functions and groups in the device tree. This code is committed
and available in the pinctrl git tree.

Look at commits:
commit c7059c5ac70aea194b07b2d811df433eb0ca81b5
pinctrl: core: Add generic pinctrl functions for managing groups
commit a76edc89b100e4fefb2a5c00cd8cd557437659e7
pinctrl: core: Add generic pinctrl functions for managing groups
commit caeb774ea3b1bc25dc2f24681c27543aba6ca7ae
pinctrl: single: Use generic pinctrl helpers for managing groups
commit 571aec4df5b72a80f80d1e524da8fbd7ff525c98
pinctrl: single: Use generic pinmux helpers for managing functions
commit 3fd6d6ad73af90522321451a2d10b0a8967d47d1
pinctrl: imx: use generic pinmux helpers for managing functions

So two drivers already switch to generic code handling this.

Please investigate and try out the above.

> +int ingenic_pinctrl_probe(struct platform_device *pdev,
> +               const struct ingenic_pinctrl_ops *ops)
> +{
> +       struct device *dev = &pdev->dev;
> +       struct ingenic_pinctrl *jzpc;
> +       struct ingenic_gpio_chip *jzgc;
> +       struct pinctrl_desc *pctl_desc;
> +       struct device_node *np, *chips_node, *functions_node;
> +       unsigned int i, j;
> +       int err;
> +
> +       if (!dev->of_node) {
> +               dev_err(dev, "device tree node not found\n");
> +               return -ENODEV;
> +       }

I think this check is not necessary since you only probe from device tree.
We usually skip it these days.

> +       jzpc->base = 0;
> +       of_property_read_u32(dev->of_node, "base", &jzpc->base);

If this is the Linux GPIO number base then NACK, we don't define
Linux-specific things in the device tree.

Please use dynamic allocation of GPIO base anyways, as stated
above.

> +       chips_node = of_find_node_by_name(dev->of_node, "gpio-chips");

This looks like a very dubious new DT bindings. Will review in that
document.

> +       jzpc->num_gpio_chips = of_get_available_child_count(chips_node);
> +       if (!jzpc->num_gpio_chips) {
> +               dev_err(dev, "No GPIO chips found\n");
> +               return -EINVAL;
> +       }

Usually it is better to create one device per gpiochip.

> +       /* register pinctrl GPIO ranges */
> +       for (i = 0; i < jzpc->num_gpio_chips; i++) {
> +               jzgc = &jzpc->gpio_chips[i];
> +
> +               jzgc->grange.name = jzgc->name;
> +               jzgc->grange.id = jzgc->idx;
> +               jzgc->grange.pin_base = jzgc->idx * PINS_PER_GPIO_PORT;
> +               jzgc->grange.base = jzgc->gc.base;
> +               jzgc->grange.npins = jzgc->gc.ngpio;
> +               jzgc->grange.gc = &jzgc->gc;
> +               pinctrl_add_gpio_range(jzpc->pctl, &jzgc->grange);
> +       }

I strongly recommend defining the GPIO ranges in the device tree
and if not possible, to add the GPIO range from the gpiochip
side and not the pinctrl side.

> +struct ingenic_pinctrl_ops {
> +       unsigned int nb_functions;
> +
> +       void (*set_function)(void __iomem *base,
> +                       unsigned int offset, unsigned int function);
> +       void (*set_gpio)(void __iomem *base, unsigned int offset, bool output);
> +       int  (*get_bias)(void __iomem *base, unsigned int offset);
> +       void (*set_bias)(void __iomem *base, unsigned int offset, bool enable);
> +       void (*gpio_set_value)(void __iomem *base,
> +                       unsigned int offset, int value);
> +       int  (*gpio_get_value)(void __iomem *base, unsigned int offset);
> +       u32  (*irq_read)(void __iomem *base);
> +       void (*irq_mask)(void __iomem *base, unsigned int irq, bool mask);
> +       void (*irq_ack)(void __iomem *base, unsigned int irq);
> +       void (*irq_set_type)(void __iomem *base,
> +                       unsigned int irq, unsigned int type);
> +};

This is a *lot* of vtable indirections. Are you sure this is a good
idea?

> +static void jz4740_set_gpio(void __iomem *base,
> +               unsigned int offset, bool output)
> +{
> +       writel(1 << offset, base + GPIO_FUNCC);
> +       writel(1 << offset, base + GPIO_SELECTC);
> +       writel(1 << offset, base + GPIO_TRIGC);
> +
> +       if (output)
> +               writel(1 << offset, base + GPIO_DIRS);
> +       else
> +               writel(1 << offset, base + GPIO_DIRC);
> +}

Replace all (1 << offset) things with:

#include <linux/bitops.h>

BIT(offset)

Simple and clean.

> +static int jz4740_get_bias(void __iomem *base, unsigned int offset)
> +{
> +       return !((readl(base + GPIO_PULL_DIS) >> offset) & 0x1);
> +}

Similarly:
return !((readl(base + GPIO_PULL_DIR) & BIT(offset));

Follow these patterns everywhere.

> +static int jz4740_gpio_get_value(void __iomem *base, unsigned int offset)
> +{
> +       return (readl(base + GPIO_DATA) >> offset) & 0x1;
> +}

return !!(readl(base + GPIO_DATA) & BIT(offset));

Yours,
Linus Walleij

Re: [PATCH 01/13] Documentation: dt/bindings: Document pinctrl-ingenic

[Linus Walleij]

On Wed, Jan 18, 2017 at 12:14 AM, Paul Cercueil <paul@crapouillou.net> wrote:

> From: Paul Burton <paul.burton@imgtec.com>
>
> This commit adds documentation for the devicetree bidings of the
> pinctrl-ingenic driver, which handles pin configuration, pin muxing
> and GPIOs of the Ingenic SoCs currently supported by the Linux kernel.
>
> Signed-off-by: Paul Cercueil <paul@crapouillou.net>

(...)

> +##### 'gpio-chips' sub-node #####
> +
> +The gpio-chips node will contain sub-nodes that correspond to GPIO controllers
> +(one sub-node per GPIO controller).
> +
> +Required properties:
> +- #address-cells: Should contain the integer 1.
> +- #size-cells: Should contain the integer 1.
> +- ranges: Should be empty.

I do not see why the GPIO needs a special subnode. Can the pin controller
and the GPIO not simply spawn from a single node?

> +##### GPIO controller node #####
> +
> +Each subnode of the 'gpio-chips' node is a GPIO controller node.
> +
> +Required properties:
> +- gpio-controller: Identifies this node as a GPIO controller.
> +- #gpio-cells: Should contain the integer 2.
> +- reg: Should contain the physical address and length of the GPIO controller's
> +  configuration registers.
> +
> +Optional properties:
> +- interrupt-controller: The GPIO controllers can optionally configure the
> +  GPIOs as interrupt sources. In this case, the 'interrupt-controller'
> +  standalone property should be supplied.
> +- #interrupt-cells: Required if 'interrupt-controller' is also specified.
> +  In that case, it should contain the integer 2.
> +- interrupts: Required if 'interrupt-controller' is also specified.
> +  In that case, it should contain the IRQ number of this GPIO controller.
> +- ingenic,pull-ups: A bit mask identifying the pins associated with this GPIO
> +  port which feature a pull-up resistor. The default mask is 0x0.
> +- ingenic,pull-downs: A bit mask identifying the pins associated with this GPIO
> +  port which feature a pull-down resistor. The default mask is 0x0.

So these bits tell us which lines have a pull up and pull down resistor?

Isn't that readily know from the compatible string? Then just hardcode
that into the driver for each variant, there is no need to define that in
the device tree.

> +##### Pin function node #####
> +
> +Each subnode of the 'functions' node is a pin function node.
> +
> +These subnodes represent a functionality of the SoC which may be exposed
> +through one or more groups of pins, represented as subnodes of the pin
> +function node. For example a function may be uart0, which may be exposed
> +through the group of pins PF0 to PF3.
> +
> +Required pin function node properties:
> +- None.
> +
> +
> +##### Pin group node #####
> +
> +Each subnode of a pin function node is a pin group node.
> +
> +Required pin group node properties:
> +- ingenic,pins: A set of values representing the pins within this pin group and
> +  their configuration.

Look into using the standard pins property from the pinctrl bindings
if yoy want to do this.

> Four values should be provided for each pin:
> +  - The phandle of the GPIO controller node for the GPIO port within which the
> +    pin is found.
> +  - The index of the pin within its GPIO port (an integer in the range 0 to 31
> +    inclusive).

This is already supported by gpio ranges, please do not reimplement
stuff we already have.

> +  - The index of the shared function port to be programmed in the GPIO port
> +    registers for this pin.

I don't see why this can not be stored in the driver.
But some people prefer to shovel everything into the device
tree, I don't know. Can you elaborate why this should be in the
device tree?

> +  - The phandle of a pin configuration node specifying the electrical
> +    configuration that should be applied to the pin.

Why? This is something the standard pin control states handles.
I'm confused.

> +For example the function 'msc0' may be exposed through 2 different pin groups,
> +one in GPIO port A and one in GPIO port E:
> +
> +  bias-configs {
> +    nobias: nobias {
> +      bias-disable;
> +    };
> +  };
> +
> +  functions {
> +    pinfunc_msc0: msc0 {
> +      pins_msc0_pa: msc0-pa {
> +        ingenic,pins = <&gpa  4 1 &nobias   /* d4 */
> +                        &gpa  5 1 &nobias   /* d5 */
> +                        &gpa  6 1 &nobias   /* d6 */
> +                        &gpa  7 1 &nobias   /* d7 */
> +                        &gpa 18 1 &nobias   /* clk */
> +                        &gpa 19 1 &nobias   /* cmd */
> +                        &gpa 20 1 &nobias   /* d0 */
> +                        &gpa 21 1 &nobias   /* d1 */
> +                        &gpa 22 1 &nobias   /* d2 */
> +                        &gpa 23 1 &nobias   /* d3 */
> +                        &gpa 24 1 &nobias>; /* rst */
> +      };

Please look at other bindings and drivers and read pinctrl.txt
closely. This makes no sense to me compared to other
examples.

This is something that seems to cross-mix gpio ranges
and pin config, that doesn't work for me, we can't have an
idiomatic binding like this. I understand that it may fit your
single usecase perfectly but it will be a maintenance nightmare
for me.

Yours,
Linus Walleij

Re: [PATCH 05/13] MIPS: jz4740: DTS: Add node for the jz4740-pinctrl driver

[Linus Walleij]

On Wed, Jan 18, 2017 at 12:14 AM, Paul Cercueil <paul@crapouillou.net> wrote:

> For a description of the devicetree node, please read
> Documentation/devicetree/bindings/pinctrl/ingenic,pinctrl.txt
>
> Signed-off-by: Paul Cercueil <paul@crapouillou.net>

(...)

> +       pinctrl: ingenic-pinctrl@10010000 {
> +               compatible = "ingenic,jz4740-pinctrl";
> +               #address-cells = <1>;
> +               #size-cells = <1>;
> +               ranges;
> +
> +               gpio-chips {
> +                       #address-cells = <1>;
> +                       #size-cells = <1>;
> +                       ranges;
> +
> +                       gpa: gpa {
> +                               reg = <0x10010000 0x100>;
> +
> +                               gpio-controller;
> +                               #gpio-cells = <2>;
> +
> +                               interrupt-controller;
> +                               #interrupt-cells = <2>;
> +
> +                               interrupt-parent = <&intc>;
> +                               interrupts = <28>;
> +
> +                               ingenic,pull-ups = <0xffffffff>;
> +                       };
> +
> +                       gpb: gpb {
> +                               reg = <0x10010100 0x100>;
> +
> +                               gpio-controller;
> +                               #gpio-cells = <2>;
> +
> +                               interrupt-controller;
> +                               #interrupt-cells = <2>;
> +
> +                               interrupt-parent = <&intc>;
> +                               interrupts = <27>;
> +
> +                               ingenic,pull-ups = <0xffffffff>;
> +                       };
> +
> +                       gpc: gpc {
> +                               reg = <0x10010200 0x100>;
> +
> +                               gpio-controller;
> +                               #gpio-cells = <2>;
> +
> +                               interrupt-controller;
> +                               #interrupt-cells = <2>;
> +
> +                               interrupt-parent = <&intc>;
> +                               interrupts = <26>;
> +
> +                               ingenic,pull-ups = <0xffffffff>;
> +                       };
> +
> +                       gpd: gpd {
> +                               reg = <0x10010300 0x100>;
> +
> +                               gpio-controller;
> +                               #gpio-cells = <2>;
> +
> +                               interrupt-controller;
> +                               #interrupt-cells = <2>;
> +
> +                               interrupt-parent = <&intc>;
> +                               interrupts = <25>;
> +
> +                               ingenic,pull-ups = <0xdfffffff>;
> +                       };
> +               };

Just pull all these down two levels and make them one device
each instead of having them inside the pin controller node
like this.

Then make a pin controller node separately, it can reference the
pin controller by phandles if necessary, and use the standard
gpio-ranges property to cross make GPIO and pin control.

It seems you driver is similar to for example the
drivers/pinctrl/nomadik/* pin controller.

Look in arch/arm/boot/dts/ste-dbx500.dtsi for examples,
NB: I'm not fully using standard bindings in it, because they
were not invented at the time.

Yours,
Linus Walleij

Re: [PATCH 00/13] Ingenic JZ4740 / JZ4780 pinctrl driver

[Linus Walleij]

On Wed, Jan 18, 2017 at 12:14 AM, Paul Cercueil <paul@crapouillou.net> wrote:

> One problem still unresolved: the pinctrl framework does not allow us to
> configure each pin on demand (someone please prove me wrong), when the
> various PWM channels are requested or released. For instance, the PWM
> channels can be configured from sysfs, which would require all PWM pins
> to be configured properly beforehand for the PWM function, eventually
> causing conflicts with other platform or board drivers.

Why do you think this?

- Pincontrol handles can be obtained at runtime.
- Pincontrol states can be changed at runtime.

The fact that a the handle is retrived by the device core and set to
the states named "init" or "default" during boot is just a convenience.

You can have as many and as fine-grained states as you want. They
can pertain to just one pin too.

Linus Walleij

Re: [PATCH 13/13] MIPS: jz4740: Remove custom GPIO code

[Linus Walleij]

On Wed, Jan 18, 2017 at 12:14 AM, Paul Cercueil <paul@crapouillou.net> wrote:

> All the drivers for the various hardware elements of the jz4740 SoC have
> been modified to use the pinctrl framework for their pin configuration
> needs.
> As such, this platform code is now unused and can be deleted.
>
> Signed-off-by: Paul Cercueil <paul@crapouillou.net>

I feel I might have come across as a bit harsh in the previous review of the
patches leading up to this one. In that case I'm sorry.

I can clearly see that the intent of the series is to remove this hopelessly
idiomatic code from the MIPS hurdle, and move these systems over
to standard frameworks.

In a way, if I look at the kernel as a whole, it would likely look better
after these patches were merged, than before. Even with the
shortcomings I painted out in the previous review comments.

A very complicated piece of messy code is cut from MIPS.

I think this is very valuable work and well worth persuing, so please
go ahead and work with the series, your effort is very much appreciated!

Yours,
Linus Walleij

Re: [PATCH 09/13] mmc: jz4740: Let the pinctrl driver configure the pins

[Ulf Hansson]

[...]

>
> -#ifdef CONFIG_PM_SLEEP
> -
> -static int jz4740_mmc_suspend(struct device *dev)
> -{
> -       struct jz4740_mmc_host *host = dev_get_drvdata(dev);
> -
> -       jz_gpio_bulk_suspend(jz4740_mmc_pins, jz4740_mmc_num_pins(host));
> -

Shouldn't this be replaced with a call to:
pinctrl_pm_select_sleep_state();

> -       return 0;
> -}
> -
> -static int jz4740_mmc_resume(struct device *dev)
> -{
> -       struct jz4740_mmc_host *host = dev_get_drvdata(dev);
> -
> -       jz_gpio_bulk_resume(jz4740_mmc_pins, jz4740_mmc_num_pins(host));

Shouldn't this be replaced with a call to:
pinctrl_pm_select_default_state();

[...]

Kind regards
Uffe

Re: [PATCH 00/13] Ingenic JZ4740 / JZ4780 pinctrl driver

[Linus Walleij]

On Thu, Jan 19, 2017 at 12:19 PM, Paul Cercueil <paul@crapouillou.net> wrote:

> The problem with pinctrl and PWM, is that the pinctrl API works by "states".
> A default state, sleep state, and basically any custom state that the
> devicetree
> provides. This works well until you need to control individually each pin;
> with
> 8 pins, you would need 2^8 states, each one corresponding to a given
> configuration.

I do not really understand, do you really use all 2^8 states in a given
system?

The pin control states are to be used for practical situations, not
for all theoretical situations.

You should define in your device tree the states that your
particular system will use. Not all possible states on all possible
systems.

Yours,
Linus Walleij

Re: [PATCH v2 10/14] mmc: jz4740: Let the pinctrl driver configure the pins

[Ulf Hansson]

On 22 January 2017 at 15:49, Paul Cercueil <paul@crapouillou.net> wrote:
> Now that the JZ4740 and similar SoCs have a pinctrl driver, we rely on
> the pins being properly configured before the driver probes.
>
> Signed-off-by: Paul Cercueil <paul@crapouillou.net>

Acked-by: Ulf Hansson <ulf.hansson@linaro.org>

Kind regards
Uffe

> ---
>  drivers/mmc/host/jz4740_mmc.c | 45 +++++--------------------------------------
>  1 file changed, 5 insertions(+), 40 deletions(-)
>
> v2: Set pin sleep/default state in suspend/resume callbacks
>
> diff --git a/drivers/mmc/host/jz4740_mmc.c b/drivers/mmc/host/jz4740_mmc.c
> index 819ad32964fc..b5fec5b7ee7b 100644
> --- a/drivers/mmc/host/jz4740_mmc.c
> +++ b/drivers/mmc/host/jz4740_mmc.c
> @@ -20,14 +20,13 @@
>  #include <linux/irq.h>
>  #include <linux/interrupt.h>
>  #include <linux/module.h>
> +#include <linux/pinctrl/consumer.h>
>  #include <linux/platform_device.h>
>  #include <linux/delay.h>
>  #include <linux/scatterlist.h>
>  #include <linux/clk.h>
>
>  #include <linux/bitops.h>
> -#include <linux/gpio.h>
> -#include <asm/mach-jz4740/gpio.h>
>  #include <asm/cacheflush.h>
>  #include <linux/dma-mapping.h>
>  #include <linux/dmaengine.h>
> @@ -906,15 +905,6 @@ static const struct mmc_host_ops jz4740_mmc_ops = {
>         .enable_sdio_irq = jz4740_mmc_enable_sdio_irq,
>  };
>
> -static const struct jz_gpio_bulk_request jz4740_mmc_pins[] = {
> -       JZ_GPIO_BULK_PIN(MSC_CMD),
> -       JZ_GPIO_BULK_PIN(MSC_CLK),
> -       JZ_GPIO_BULK_PIN(MSC_DATA0),
> -       JZ_GPIO_BULK_PIN(MSC_DATA1),
> -       JZ_GPIO_BULK_PIN(MSC_DATA2),
> -       JZ_GPIO_BULK_PIN(MSC_DATA3),
> -};
> -
>  static int jz4740_mmc_request_gpio(struct device *dev, int gpio,
>         const char *name, bool output, int value)
>  {
> @@ -978,15 +968,6 @@ static void jz4740_mmc_free_gpios(struct platform_device *pdev)
>                 gpio_free(pdata->gpio_power);
>  }
>
> -static inline size_t jz4740_mmc_num_pins(struct jz4740_mmc_host *host)
> -{
> -       size_t num_pins = ARRAY_SIZE(jz4740_mmc_pins);
> -       if (host->pdata && host->pdata->data_1bit)
> -               num_pins -= 3;
> -
> -       return num_pins;
> -}
> -
>  static int jz4740_mmc_probe(struct platform_device* pdev)
>  {
>         int ret;
> @@ -1027,15 +1008,9 @@ static int jz4740_mmc_probe(struct platform_device* pdev)
>                 goto err_free_host;
>         }
>
> -       ret = jz_gpio_bulk_request(jz4740_mmc_pins, jz4740_mmc_num_pins(host));
> -       if (ret) {
> -               dev_err(&pdev->dev, "Failed to request mmc pins: %d\n", ret);
> -               goto err_free_host;
> -       }
> -
>         ret = jz4740_mmc_request_gpios(mmc, pdev);
>         if (ret)
> -               goto err_gpio_bulk_free;
> +               goto err_release_dma;
>
>         mmc->ops = &jz4740_mmc_ops;
>         mmc->f_min = JZ_MMC_CLK_RATE / 128;
> @@ -1091,10 +1066,9 @@ static int jz4740_mmc_probe(struct platform_device* pdev)
>         free_irq(host->irq, host);
>  err_free_gpios:
>         jz4740_mmc_free_gpios(pdev);
> -err_gpio_bulk_free:
> +err_release_dma:
>         if (host->use_dma)
>                 jz4740_mmc_release_dma_channels(host);
> -       jz_gpio_bulk_free(jz4740_mmc_pins, jz4740_mmc_num_pins(host));
>  err_free_host:
>         mmc_free_host(mmc);
>
> @@ -1114,7 +1088,6 @@ static int jz4740_mmc_remove(struct platform_device *pdev)
>         free_irq(host->irq, host);
>
>         jz4740_mmc_free_gpios(pdev);
> -       jz_gpio_bulk_free(jz4740_mmc_pins, jz4740_mmc_num_pins(host));
>
>         if (host->use_dma)
>                 jz4740_mmc_release_dma_channels(host);
> @@ -1128,20 +1101,12 @@ static int jz4740_mmc_remove(struct platform_device *pdev)
>
>  static int jz4740_mmc_suspend(struct device *dev)
>  {
> -       struct jz4740_mmc_host *host = dev_get_drvdata(dev);
> -
> -       jz_gpio_bulk_suspend(jz4740_mmc_pins, jz4740_mmc_num_pins(host));
> -
> -       return 0;
> +       return pinctrl_pm_select_sleep_state(dev);
>  }
>
>  static int jz4740_mmc_resume(struct device *dev)
>  {
> -       struct jz4740_mmc_host *host = dev_get_drvdata(dev);
> -
> -       jz_gpio_bulk_resume(jz4740_mmc_pins, jz4740_mmc_num_pins(host));
> -
> -       return 0;
> +       return pinctrl_pm_select_default_state(dev);
>  }
>
>  static SIMPLE_DEV_PM_OPS(jz4740_mmc_pm_ops, jz4740_mmc_suspend,
> --
> 2.11.0
>

Re: [PATCH v2 01/14] Documentation: dt/bindings: Document pinctrl-ingenic

[Linus Walleij]

On Sun, Jan 22, 2017 at 3:49 PM, Paul Cercueil <paul@crapouillou.net> wrote:


> +Required properties:
> +- compatible: One of:
> +  - "ingenic,jz4740-pinctrl"
> +  - "ingenic,jz4780-pinctrl"
> +
> +Optional properties:
> +- ingenic,pull-ups: A list of 32-bit bit fields, where each bit set tells the
> +  driver that a pull-up resistor is available for this pin.
> +  By default, the driver considers that all pins feature a pull-up resistor.
> +- ingenic,pull-downs: A list of 32-bit bit fields, where each bit set tells
> +  the driver that a pull-down resistor is available for this pin.
> +  By default, the driver considers that all pins feature a pull-down
> +  resistor.

So I still don't understand these properties.

Does this mean that there is a pull-up *inside* the SoC or *outside*
on the board where it is mounted?

If it is *outside* the SoC, on the board, the pulls should be set on
the consumers, not on the controller.

In the former case, if this pertains to the *inside* of the SoC:
is it just different between jz4740 and jz4780?
In that case, just code this into the driver as .data to the .compatible
in the DT match. No special DT properties needed.

If it is different between e.g. different versions of jz4740, why not
make different compatible-properties? If there are different versions
of the circuit, with different hard-wired hardware configuration, they
are different devices and should have different compatible strings.

> +                               /* CLK, CMD, D0 */
> +                               ingenic,pins = <0x69 0 0x68 0 0x6a 0>;

Standard bindings use just "pins". Why the custom ingenic,
prefix?

Yours,
Linus Walleij

Re: [PATCH 10/13] mtd: nand: jz4740: Let the pinctrl driver configure the pins

[Boris Brezillon]

On Wed, 18 Jan 2017 00:14:18 +0100
Paul Cercueil <paul@crapouillou.net> wrote:

> Before, this NAND driver would set itself the configuration of the
> chip-select pins for the various NAND banks.
> 
> Now that the JZ4740 and similar SoCs have a pinctrl driver, we rely on
> the pins being properly configured before the driver probes.
> 
> Signed-off-by: Paul Cercueil <paul@crapouillou.net>

Acked-by: Boris Brezillon <boris.brezillon@free-electrons.com>

> ---
>  drivers/mtd/nand/jz4740_nand.c | 23 +----------------------
>  1 file changed, 1 insertion(+), 22 deletions(-)
> 
> diff --git a/drivers/mtd/nand/jz4740_nand.c b/drivers/mtd/nand/jz4740_nand.c
> index 5551c36adbdf..0d06a1f07d82 100644
> --- a/drivers/mtd/nand/jz4740_nand.c
> +++ b/drivers/mtd/nand/jz4740_nand.c
> @@ -25,7 +25,6 @@
>  
>  #include <linux/gpio.h>
>  
> -#include <asm/mach-jz4740/gpio.h>
>  #include <asm/mach-jz4740/jz4740_nand.h>
>  
>  #define JZ_REG_NAND_CTRL	0x50
> @@ -310,34 +309,20 @@ static int jz_nand_detect_bank(struct platform_device *pdev,
>  			       uint8_t *nand_dev_id)
>  {
>  	int ret;
> -	int gpio;
> -	char gpio_name[9];
>  	char res_name[6];
>  	uint32_t ctrl;
>  	struct nand_chip *chip = &nand->chip;
>  	struct mtd_info *mtd = nand_to_mtd(chip);
>  
> -	/* Request GPIO port. */
> -	gpio = JZ_GPIO_MEM_CS0 + bank - 1;
> -	sprintf(gpio_name, "NAND CS%d", bank);
> -	ret = gpio_request(gpio, gpio_name);
> -	if (ret) {
> -		dev_warn(&pdev->dev,
> -			"Failed to request %s gpio %d: %d\n",
> -			gpio_name, gpio, ret);
> -		goto notfound_gpio;
> -	}
> -
>  	/* Request I/O resource. */
>  	sprintf(res_name, "bank%d", bank);
>  	ret = jz_nand_ioremap_resource(pdev, res_name,
>  					&nand->bank_mem[bank - 1],
>  					&nand->bank_base[bank - 1]);
>  	if (ret)
> -		goto notfound_resource;
> +		return ret;
>  
>  	/* Enable chip in bank. */
> -	jz_gpio_set_function(gpio, JZ_GPIO_FUNC_MEM_CS0);
>  	ctrl = readl(nand->base + JZ_REG_NAND_CTRL);
>  	ctrl |= JZ_NAND_CTRL_ENABLE_CHIP(bank - 1);
>  	writel(ctrl, nand->base + JZ_REG_NAND_CTRL);
> @@ -377,12 +362,8 @@ static int jz_nand_detect_bank(struct platform_device *pdev,
>  	dev_info(&pdev->dev, "No chip found on bank %i\n", bank);
>  	ctrl &= ~(JZ_NAND_CTRL_ENABLE_CHIP(bank - 1));
>  	writel(ctrl, nand->base + JZ_REG_NAND_CTRL);
> -	jz_gpio_set_function(gpio, JZ_GPIO_FUNC_NONE);
>  	jz_nand_iounmap_resource(nand->bank_mem[bank - 1],
>  				 nand->bank_base[bank - 1]);
> -notfound_resource:
> -	gpio_free(gpio);
> -notfound_gpio:
>  	return ret;
>  }
>  
> @@ -503,7 +484,6 @@ static int jz_nand_probe(struct platform_device *pdev)
>  err_unclaim_banks:
>  	while (chipnr--) {
>  		unsigned char bank = nand->banks[chipnr];
> -		gpio_free(JZ_GPIO_MEM_CS0 + bank - 1);
>  		jz_nand_iounmap_resource(nand->bank_mem[bank - 1],
>  					 nand->bank_base[bank - 1]);
>  	}
> @@ -530,7 +510,6 @@ static int jz_nand_remove(struct platform_device *pdev)
>  		if (bank != 0) {
>  			jz_nand_iounmap_resource(nand->bank_mem[bank - 1],
>  						 nand->bank_base[bank - 1]);
> -			gpio_free(JZ_GPIO_MEM_CS0 + bank - 1);
>  		}
>  	}
>  

Re: [PATCH v3 12/14] fbdev: jz4740-fb: Let the pinctrl driver configure the pins

[Bartlomiej Zolnierkiewicz]

Hi,

On Wednesday, January 25, 2017 07:52:05 PM Paul Cercueil wrote:
> Now that the JZ4740 and similar SoCs have a pinctrl driver, we rely on
> the pins being properly configured before the driver probes.
> 
> Signed-off-by: Paul Cercueil <paul@crapouillou.net>

Acked-by: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>

Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics

> ---
>  drivers/video/fbdev/jz4740_fb.c | 104 ++--------------------------------------
>  1 file changed, 3 insertions(+), 101 deletions(-)
> 
> v2: No changes
> v3: No changes
> 
> diff --git a/drivers/video/fbdev/jz4740_fb.c b/drivers/video/fbdev/jz4740_fb.c
> index 87790e9644d0..b57df83fdbd3 100644
> --- a/drivers/video/fbdev/jz4740_fb.c
> +++ b/drivers/video/fbdev/jz4740_fb.c
> @@ -17,6 +17,7 @@
>  #include <linux/module.h>
>  #include <linux/mutex.h>
>  #include <linux/platform_device.h>
> +#include <linux/pinctrl/consumer.h>
>  
>  #include <linux/clk.h>
>  #include <linux/delay.h>
> @@ -27,7 +28,6 @@
>  #include <linux/dma-mapping.h>
>  
>  #include <asm/mach-jz4740/jz4740_fb.h>
> -#include <asm/mach-jz4740/gpio.h>
>  
>  #define JZ_REG_LCD_CFG		0x00
>  #define JZ_REG_LCD_VSYNC	0x04
> @@ -146,93 +146,6 @@ static const struct fb_fix_screeninfo jzfb_fix = {
>  	.accel		= FB_ACCEL_NONE,
>  };
>  
> -static const struct jz_gpio_bulk_request jz_lcd_ctrl_pins[] = {
> -	JZ_GPIO_BULK_PIN(LCD_PCLK),
> -	JZ_GPIO_BULK_PIN(LCD_HSYNC),
> -	JZ_GPIO_BULK_PIN(LCD_VSYNC),
> -	JZ_GPIO_BULK_PIN(LCD_DE),
> -	JZ_GPIO_BULK_PIN(LCD_PS),
> -	JZ_GPIO_BULK_PIN(LCD_REV),
> -	JZ_GPIO_BULK_PIN(LCD_CLS),
> -	JZ_GPIO_BULK_PIN(LCD_SPL),
> -};
> -
> -static const struct jz_gpio_bulk_request jz_lcd_data_pins[] = {
> -	JZ_GPIO_BULK_PIN(LCD_DATA0),
> -	JZ_GPIO_BULK_PIN(LCD_DATA1),
> -	JZ_GPIO_BULK_PIN(LCD_DATA2),
> -	JZ_GPIO_BULK_PIN(LCD_DATA3),
> -	JZ_GPIO_BULK_PIN(LCD_DATA4),
> -	JZ_GPIO_BULK_PIN(LCD_DATA5),
> -	JZ_GPIO_BULK_PIN(LCD_DATA6),
> -	JZ_GPIO_BULK_PIN(LCD_DATA7),
> -	JZ_GPIO_BULK_PIN(LCD_DATA8),
> -	JZ_GPIO_BULK_PIN(LCD_DATA9),
> -	JZ_GPIO_BULK_PIN(LCD_DATA10),
> -	JZ_GPIO_BULK_PIN(LCD_DATA11),
> -	JZ_GPIO_BULK_PIN(LCD_DATA12),
> -	JZ_GPIO_BULK_PIN(LCD_DATA13),
> -	JZ_GPIO_BULK_PIN(LCD_DATA14),
> -	JZ_GPIO_BULK_PIN(LCD_DATA15),
> -	JZ_GPIO_BULK_PIN(LCD_DATA16),
> -	JZ_GPIO_BULK_PIN(LCD_DATA17),
> -};
> -
> -static unsigned int jzfb_num_ctrl_pins(struct jzfb *jzfb)
> -{
> -	unsigned int num;
> -
> -	switch (jzfb->pdata->lcd_type) {
> -	case JZ_LCD_TYPE_GENERIC_16_BIT:
> -		num = 4;
> -		break;
> -	case JZ_LCD_TYPE_GENERIC_18_BIT:
> -		num = 4;
> -		break;
> -	case JZ_LCD_TYPE_8BIT_SERIAL:
> -		num = 3;
> -		break;
> -	case JZ_LCD_TYPE_SPECIAL_TFT_1:
> -	case JZ_LCD_TYPE_SPECIAL_TFT_2:
> -	case JZ_LCD_TYPE_SPECIAL_TFT_3:
> -		num = 8;
> -		break;
> -	default:
> -		num = 0;
> -		break;
> -	}
> -	return num;
> -}
> -
> -static unsigned int jzfb_num_data_pins(struct jzfb *jzfb)
> -{
> -	unsigned int num;
> -
> -	switch (jzfb->pdata->lcd_type) {
> -	case JZ_LCD_TYPE_GENERIC_16_BIT:
> -		num = 16;
> -		break;
> -	case JZ_LCD_TYPE_GENERIC_18_BIT:
> -		num = 18;
> -		break;
> -	case JZ_LCD_TYPE_8BIT_SERIAL:
> -		num = 8;
> -		break;
> -	case JZ_LCD_TYPE_SPECIAL_TFT_1:
> -	case JZ_LCD_TYPE_SPECIAL_TFT_2:
> -	case JZ_LCD_TYPE_SPECIAL_TFT_3:
> -		if (jzfb->pdata->bpp == 18)
> -			num = 18;
> -		else
> -			num = 16;
> -		break;
> -	default:
> -		num = 0;
> -		break;
> -	}
> -	return num;
> -}
> -
>  /* Based on CNVT_TOHW macro from skeletonfb.c */
>  static inline uint32_t jzfb_convert_color_to_hw(unsigned val,
>  	struct fb_bitfield *bf)
> @@ -487,8 +400,7 @@ static void jzfb_enable(struct jzfb *jzfb)
>  
>  	clk_prepare_enable(jzfb->ldclk);
>  
> -	jz_gpio_bulk_resume(jz_lcd_ctrl_pins, jzfb_num_ctrl_pins(jzfb));
> -	jz_gpio_bulk_resume(jz_lcd_data_pins, jzfb_num_data_pins(jzfb));
> +	pinctrl_pm_select_default_state(&jzfb->pdev->dev);
>  
>  	writel(0, jzfb->base + JZ_REG_LCD_STATE);
>  
> @@ -511,8 +423,7 @@ static void jzfb_disable(struct jzfb *jzfb)
>  		ctrl = readl(jzfb->base + JZ_REG_LCD_STATE);
>  	} while (!(ctrl & JZ_LCD_STATE_DISABLED));
>  
> -	jz_gpio_bulk_suspend(jz_lcd_ctrl_pins, jzfb_num_ctrl_pins(jzfb));
> -	jz_gpio_bulk_suspend(jz_lcd_data_pins, jzfb_num_data_pins(jzfb));
> +	pinctrl_pm_select_sleep_state(&jzfb->pdev->dev);
>  
>  	clk_disable_unprepare(jzfb->ldclk);
>  }
> @@ -701,9 +612,6 @@ static int jzfb_probe(struct platform_device *pdev)
>  	fb->mode = NULL;
>  	jzfb_set_par(fb);
>  
> -	jz_gpio_bulk_request(jz_lcd_ctrl_pins, jzfb_num_ctrl_pins(jzfb));
> -	jz_gpio_bulk_request(jz_lcd_data_pins, jzfb_num_data_pins(jzfb));
> -
>  	ret = register_framebuffer(fb);
>  	if (ret) {
>  		dev_err(&pdev->dev, "Failed to register framebuffer: %d\n", ret);
> @@ -715,9 +623,6 @@ static int jzfb_probe(struct platform_device *pdev)
>  	return 0;
>  
>  err_free_devmem:
> -	jz_gpio_bulk_free(jz_lcd_ctrl_pins, jzfb_num_ctrl_pins(jzfb));
> -	jz_gpio_bulk_free(jz_lcd_data_pins, jzfb_num_data_pins(jzfb));
> -
>  	fb_dealloc_cmap(&fb->cmap);
>  	jzfb_free_devmem(jzfb);
>  err_framebuffer_release:
> @@ -731,9 +636,6 @@ static int jzfb_remove(struct platform_device *pdev)
>  
>  	jzfb_blank(FB_BLANK_POWERDOWN, jzfb->fb);
>  
> -	jz_gpio_bulk_free(jz_lcd_ctrl_pins, jzfb_num_ctrl_pins(jzfb));
> -	jz_gpio_bulk_free(jz_lcd_data_pins, jzfb_num_data_pins(jzfb));
> -
>  	fb_dealloc_cmap(&jzfb->fb->cmap);
>  	jzfb_free_devmem(jzfb);

Re: [PATCH v3 02/14] Documentation: dt/bindings: Document pinctrl-gpio

[Rob Herring]

On Wed, Jan 25, 2017 at 07:51:55PM +0100, Paul Cercueil wrote:
> This commit adds documentation for the devicetree bidings of the

s/biding/bindings/

> pinctrl-gpio driver, which handles GPIOs of the Ingenic SoCs
> currently supported by the Linux kernel.

The subject makes no reference that this binding is for Ingenic GPIO. 
Also, drop the "Documentation: " part. It's redundant.

> 
> Signed-off-by: Paul Cercueil <paul@crapouillou.net>
> ---
>  .../devicetree/bindings/gpio/ingenic,gpio.txt      | 45 ++++++++++++++++++++++
>  1 file changed, 45 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/gpio/ingenic,gpio.txt
> 
> v2: New patch
> v3: No changes
> 
> diff --git a/Documentation/devicetree/bindings/gpio/ingenic,gpio.txt b/Documentation/devicetree/bindings/gpio/ingenic,gpio.txt
> new file mode 100644
> index 000000000000..b2eb20494365
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/gpio/ingenic,gpio.txt
> @@ -0,0 +1,45 @@
> +Ingenic jz47xx GPIO controller
> +
> +Required properties:
> +  - compatible:
> +    - "ingenic,jz4740-gpio" for the JZ4740 SoC
> +    - "ingenic,jz4780-gpio" for the JZ4780 SoC
> +
> +  - reg: Base address and length of each memory resource used by the GPIO
> +    controller hardware module.
> +
> +  - gpio-controller: Marks the device node as a GPIO controller.
> +  - #gpio-cells: Should be 2. The first cell is the GPIO number and the second
> +    cell specifies GPIO flags, as defined in <dt-bindings/gpio/gpio.h>. Only the
> +    GPIO_ACTIVE_HIGH and GPIO_ACTIVE_LOW flags are supported.
> +  - gpio-ranges: Range of pins managed by the GPIO controller.
> +
> +Optional properties:
> +  - base: The GPIO number to use as the base for this driver.

Drop this please. This is a Linuxism.

> +  - interrupt-controller: Marks the device node as an interrupt controller.
> +  - interrupts: Interrupt specifier for the controllers interrupt.
> +    Required if 'interrupt-controller' is specified.

Some h/w doesn't have interrupt capability? If not, this should not be 
optional.

> +
> +Please refer to gpio.txt in this directory for details of gpio-ranges property
> +and the common GPIO bindings used by client devices.
> +
> +The GPIO controller also acts as an interrupt controller. It uses the default
> +two cells specifier as described in Documentation/devicetree/bindings/
> +interrupt-controller/interrupts.txt.

Just document #interrupt-cells and its value and drop this paragraph.

> +
> +Example:
> +
> +gpa: gpio-controller@10010000 {
> +	compatible = "ingenic,jz4740-gpio";
> +	reg = <0x10010000 0x100>;
> +
> +	gpio-controller;
> +	gpio-ranges = <&pinctrl 0 0 32>;
> +	#gpio-cells = <2>;
> +
> +	interrupt-controller;
> +	#interrupt-cells = <2>;
> +
> +	interrupt-parent = <&intc>;
> +	interrupts = <28>;
> +};
> -- 
> 2.11.0
> 

Re: [PATCH v3 01/14] Documentation: dt/bindings: Document pinctrl-ingenic

[Rob Herring]

On Wed, Jan 25, 2017 at 07:51:54PM +0100, Paul Cercueil wrote:
> This commit adds documentation for the devicetree bidings of the
> pinctrl-ingenic driver, which handles pin configuration and pin
> muxing of the Ingenic SoCs currently supported by the Linux kernel.
> 
> Signed-off-by: Paul Cercueil <paul@crapouillou.net>
> ---
>  .../bindings/pinctrl/ingenic,pinctrl.txt           | 77 ++++++++++++++++++++++
>  1 file changed, 77 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/pinctrl/ingenic,pinctrl.txt
> 
> v2: Rewrote the documentation for the new pinctrl-ingenic driver
> v3: No changes
> 
> diff --git a/Documentation/devicetree/bindings/pinctrl/ingenic,pinctrl.txt b/Documentation/devicetree/bindings/pinctrl/ingenic,pinctrl.txt
> new file mode 100644
> index 000000000000..ead5b01ad471
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pinctrl/ingenic,pinctrl.txt
> @@ -0,0 +1,77 @@
> +Ingenic jz47xx pin controller
> +
> +Please refer to pinctrl-bindings.txt in this directory for details of the
> +common pinctrl bindings used by client devices, including the meaning of the
> +phrase "pin configuration node".
> +
> +For the jz47xx SoCs, pin control is tightly bound with GPIO ports. All pins may
> +be used as GPIOs, multiplexed device functions are configured within the
> +GPIO port configuration registers and it is typical to refer to pins using the
> +naming scheme "PxN" where x is a character identifying the GPIO port with
> +which the pin is associated and N is an integer from 0 to 31 identifying the
> +pin within that GPIO port. For example PA0 is the first pin in GPIO port A, and
> +PB31 is the last pin in GPIO port B. The jz4740 contains 4 GPIO ports, PA to
> +PD, for a total of 128 pins. The jz4780 contains 6 GPIO ports, PA to PF, for a
> +total of 192 pins.

>From the overlapping register addresses in the examples and this 
description, it looks like the pinctrlr and gpio controller are 1 block. 
If so, then there should only be 1 node.

Rob

Re: [PATCH v2 01/14] Documentation: dt/bindings: Document pinctrl-ingenic

[Linus Walleij]

On Fri, Jan 27, 2017 at 4:27 PM, Paul Cercueil <paul@crapouillou.net> wrote:
> [Me]:
>> In the former case, if this pertains to the *inside* of the SoC:
>> is it just different between jz4740 and jz4780?
>> In that case, just code this into the driver as .data to the .compatible
>> in the DT match. No special DT properties needed.
>
> Well, I've been taught that devicetree is for describing the hardware, and
> the driver code is for functionality. So that's why I put it in devicetree.

This is a gray area.

But as GPIO maintainer I'm not happy about encoding information
about the hardware, that can be deducted from the compatible-string,
into the devicetree.

I prefer to encode per-compatible hardware information tables into
the driver, as structs, and pick the right table based on the compatible
string as .data in the of match entry.

It's simple to retrieve into the driver using of_device_get_match_data()
these days.

> That's also the reason why I put the list of functions and groups in
> devicetree and not in the driver code.

I'm not super-happy about that either, and it's not the way I would
have done it but the argument has been made
that it is a lot of opaque data and people prefer to maintain it
in the device tree.

I accept it for functions and groups, but I don't like it.

I will not fold to any consistency arguments of the type "now
you allowed this one thing, so you must allow this other thing
so you are consistent", just no. I didn't like it the first time, so I'm
not liking it anymore the second time.

I guess if the DT people tell me it has to be done this way I would
accept it. After a lot of discussion. But noone has.

Please make it a table and put it in the driver instead.

Yours,
Linus Walleij

Re: [PATCH v3 01/14] Documentation: dt/bindings: Document pinctrl-ingenic

[Linus Walleij]

On Tue, Jan 31, 2017 at 11:31 AM, Paul Cercueil <paul@crapouillou.net> wrote:
> [Rob]:
>> From the overlapping register addresses in the examples and this
>> description, it looks like the pinctrlr and gpio controller are 1 block.
>> If so, then there should only be 1 node.
>
> Well, that's what I had until Linus W. just told me to do the opposite:
>
>> Just pull all these down two levels and make them one device
>> each instead of having them inside the pin controller node
>> like this.

I guess the argument is that they are in the same coherent memory
range so they should be one device node. That is how we handle
e.g. system controllers so it makes some sense.

So can the two GPIO controllers be modeled as two subnodes of
the pin controller then?

Subnodes are certainly OK, we have that for many other devices
such as interrupt controllers on PCI bridges and what not.

So when the probing of the pin controller is ready it can just
walk down and populate the GPIO subdevices with
of_platform_default_populate() or simply by registering the
device directly with platform_device_add_data() just like an
MFD device does?

This is nice because we want to use the standard gpio ranges
to map pins to GPIO lines.

I'm sorry about the unclarities here, but it's essentially an intrinsic
problem with GPIO that has been with us for years: do we model
each "bank" as a device or do we just register each bank as a
gpiochip, or do we even make one gpiochip to cover all the banks.
All solutions can be found in the kernel... also the different DT bindings:
one node for a whole slew of GPIO controllers, or seveal nodes
and I bet also several nodes for memory ranges in close proximity.

I don't know for sure what is the most elegant solution, we might
need to build some consensus here for the future so it doesn't
get to heterogeneous.

Yours,
Linus Walleij

Re: [PATCH v3 03/14] pinctrl-ingenic: add a pinctrl driver for the Ingenic jz47xx SoCs

[Linus Walleij]

On Wed, Jan 25, 2017 at 7:51 PM, Paul Cercueil <paul@crapouillou.net> wrote:

> This driver handles pin configuration and pin muxing for the
> JZ4740 and JZ4780 SoCs from Ingenic.
>
> Signed-off-by: Paul Cercueil <paul@crapouillou.net>

This is starting to look very nice.

> +#include <linux/compiler.h>
> +#include <linux/gpio.h>

Use <linux/gpio/driver.h> if it is a GPIO driver. I should
be enough ... I think.

> +static int ingenic_pinctrl_parse_dt_func(struct ingenic_pinctrl *jzpc,
> +               struct device_node *np)
> +{
> +       unsigned int num_groups;
> +       struct device_node *group_node;
> +       unsigned int i, j;
> +       int err, npins, *pins, *confs;
> +       const char **groups;
> +
> +       num_groups = of_get_child_count(np);
> +       groups = devm_kzalloc(jzpc->dev,
> +                       sizeof(*groups) * num_groups, GFP_KERNEL);
> +       if (!groups)
> +               return -ENOMEM;
> +
> +       i = 0;
> +       for_each_child_of_node(np, group_node) {
> +               groups[i++] = group_node->name;
> +
> +               npins = of_property_count_elems_of_size(group_node,
> +                               "ingenic,pins", 8);
> +               if (npins < 0)
> +                       return npins;
> +
> +               pins = devm_kzalloc(jzpc->dev,
> +                               sizeof(*pins) * npins, GFP_KERNEL);
> +               confs = devm_kzalloc(jzpc->dev,
> +                               sizeof(*confs) * npins, GFP_KERNEL);
> +               if (!pins || !confs)
> +                       return -ENOMEM;
> +
> +               for (j = 0; j < npins; j++) {
> +                       of_property_read_u32_index(group_node,
> +                                       "ingenic,pins", j * 2, &pins[j]);
> +
> +                       of_property_read_u32_index(group_node,
> +                                       "ingenic,pins", j * 2 + 1, &confs[j]);

If I didn't mention before this could pperhaps just use "pins"?

Or does these DT entries not match the generic bindings?

> +               }
> +
> +               err = pinctrl_generic_add_group(jzpc->pctl, group_node->name,
> +                               pins, npins, confs);
> +               if (err)
> +                       return err;
> +       }
> +
> +       return pinmux_generic_add_function(jzpc->pctl, np->name,
> +                       groups, num_groups, NULL);
> +}

If you just use "pins" can this even be parsed by a generic parser function
pinconf_generic_dt_subnode_to_map()?

Yours,
Linus Walleij

Re: [PATCH v3 04/14] GPIO: Add gpio-ingenic driver

[Linus Walleij]

On Wed, Jan 25, 2017 at 7:51 PM, Paul Cercueil <paul@crapouillou.net> wrote:

> This driver handles the GPIOs of all the Ingenic JZ47xx SoCs
> currently supported by the upsteam Linux kernel.
>
> Signed-off-by: Paul Cercueil <paul@crapouillou.net>

Looking nice.

> +#define JZ4740_GPIO_DATA       0x10
> +#define JZ4740_GPIO_SELECT     0x50
> +#define JZ4740_GPIO_DIR                0x60
> +#define JZ4740_GPIO_TRIG       0x70
> +#define JZ4740_GPIO_FLAG       0x80
> +
> +#define JZ4780_GPIO_INT                0x10
> +#define JZ4780_GPIO_PAT1       0x30
> +#define JZ4780_GPIO_PAT0       0x40
> +#define JZ4780_GPIO_FLAG       0x50
> +
> +#define REG_SET(x) ((x) + 0x4)
> +#define REG_CLEAR(x) ((x) + 0x8)
(...)
> +enum jz_version {
> +       ID_JZ4740,
> +       ID_JZ4780,
> +};
(...)
> +static inline bool gpio_get_value(struct ingenic_gpio_chip *jzgc, u8 offset)
> +{
> +       if (jzgc->version >= ID_JZ4780)
> +               return readl(jzgc->base + GPIO_PIN) & BIT(offset);
> +       else
> +               return readl(jzgc->base + JZ4740_GPIO_DATA) & BIT(offset);
> +}

This works for me, for sure.

What some people do, is to put the right virtual address in to the state
container.

So it would be just:

return !!readl(jzgc->datareg) & BIT(offset));

Notice also the double-bang that clamps the value to a bool. I know
the core does it too but I like to see it in drivers just to be sure.

> +static void gpio_set_value(struct ingenic_gpio_chip *jzgc, u8 offset, int value)
> +{
> +       u8 reg;
> +
> +       if (jzgc->version >= ID_JZ4780)
> +               reg = JZ4780_GPIO_PAT0;
> +       else
> +               reg = JZ4740_GPIO_DATA;
> +
> +       if (value)
> +               writel(BIT(offset), jzgc->base + REG_SET(reg));
> +       else
> +               writel(BIT(offset), jzgc->base + REG_CLEAR(reg));
> +}

Same comment.

What some drivers do when they just get/set a bit in a register
to get/set or set the direction of a GPIO, is to select GPIO_GENERIC
and just bgpio_init() with the right iomem pointers, then the core
will register handlers for get, set, set_direcition callback and
get_direction and your driver can just focus on the remainders.

> +static void ingenic_gpio_set(struct gpio_chip *gc,
> +               unsigned int offset, int value)
> +{
> +       struct ingenic_gpio_chip *jzgc = gpiochip_get_data(gc);
> +
> +       gpio_set_value(jzgc, offset, value);
> +}
> +
> +static int ingenic_gpio_get(struct gpio_chip *gc, unsigned int offset)
> +{
> +       struct ingenic_gpio_chip *jzgc = gpiochip_get_data(gc);
> +
> +       return (int) gpio_get_value(jzgc, offset);
> +}
> +
> +static int ingenic_gpio_direction_input(struct gpio_chip *gc,
> +               unsigned int offset)
> +{
> +       return pinctrl_gpio_direction_input(gc->base + offset);
> +}
> +
> +static int ingenic_gpio_direction_output(struct gpio_chip *gc,
> +               unsigned int offset, int value)
> +{
> +       ingenic_gpio_set(gc, offset, value);
> +       return pinctrl_gpio_direction_output(gc->base + offset);
> +}

If you're not just replacing these with GPIO_GENERIC, please also
include a .get_direction() callback.

It's especially nice as it reads out the state at probe and "lsgpio"
lists if pins are inputs or outputs.

Yours,
Linus Walleij

Re: [PATCH v3 06/14] MIPS: jz4740: DTS: Add nodes for ingenic pinctrl and gpio drivers

[Linus Walleij]

On Wed, Jan 25, 2017 at 7:51 PM, Paul Cercueil <paul@crapouillou.net> wrote:

> For a description of the pinctrl devicetree node, please read
> Documentation/devicetree/bindings/pinctrl/ingenic,pinctrl.txt
>
> For a description of the gpio devicetree nodes, please read
> Documentation/devicetree/bindings/gpio/ingenic,gpio.txt
>
> Signed-off-by: Paul Cercueil <paul@crapouillou.net>
> ---
>  arch/mips/boot/dts/ingenic/jz4740.dtsi | 194 +++++++++++++++++++++++++++++++++
>  1 file changed, 194 insertions(+)
>
> v2: Changed the devicetree bindings to match the new driver
> v3: No changes

This looks good to me, except the use of ingenic,pins instead
of just pins and the GPIO base property
which needs to be removed from the DT bindings and
the driver alike.

But now we're discussing where to put these GPIO controllers
in the device tree in another thread. Maybe as subnodes of the
pin controller...

Yours,
Linus Walleij

Re: [PATCH v3 04/14] GPIO: Add gpio-ingenic driver

[Linus Walleij]

Forgot to mention this:

On Wed, Jan 25, 2017 at 7:51 PM, Paul Cercueil <paul@crapouillou.net> wrote:
> This driver handles the GPIOs of all the Ingenic JZ47xx SoCs
> currently supported by the upsteam Linux kernel.
>
> Signed-off-by: Paul Cercueil <paul@crapouillou.net>
(...)
> +       jzgc->gc.base = -1;

Nice

> +       of_property_read_u32(dev->of_node, "base", &jzgc->gc.base);

Remove this. Dynamic allocation should be fine, if you're using the
new userspace ABI like tools/gpio/* or libgpiod and only that and in-kernel
consumers, dynamic numbers are just fine.

If you have old sysfs userspace that you need to support using the
global GPIO numberspace, please look into ways to phase that out.

Yours,
Linus Walleij

Re: [PATCH v3 04/14] GPIO: Add gpio-ingenic driver

[Linus Walleij]

On Tue, Jan 31, 2017 at 4:29 PM, Paul Cercueil <paul@crapouillou.net> wrote:
> Le 2017-01-31 15:20, Linus Walleij a écrit :
>
>>> + of_property_read_u32(dev->of_node, "base", &jzgc->gc.base);
>>
>>
>> Remove this. Dynamic allocation should be fine, if you're using the
>> new userspace ABI like tools/gpio/* or libgpiod and only that and
>> in-kernel
>> consumers, dynamic numbers are just fine.
>
>
> The problem is that the QI_LB60 board code still have a lot of references
> to global GPIO numbers. Just grep for JZ_GPIO_PORT in
> arch/mips/jz4740/board-qi_lb60.c to see what I mean...

OK I understand we might need a compromise here to get the code moving.

But we need to keep it out of the device tree.

I think it's better to put a base table relative to the memory base in
the driver in that case:

unsigned int gpio_global_base;

switch (memory_base_address) {
case 0x41000000:
    gpio_global_base = 0x00;
    break;
case 0x42000000:
    gpio_global_base = 0x20;

(...)

etc. (Those are not your base addresses but you get the idea).

Include a few comments like:

/*
 * DO NOT EXPAND THIS: FOR BACKWARD GPIO NUMBERSPACE
 * COMPATIBIBILITY ONLY: WORK TO TRANSITION CONSUMERS TO
 * USE THE GPIO DESCRIPTOR API IN <linux/gpio/consumer.h> INSTEAD.
 */

Then I'll be happy :)

Yours,
Linus Walleij

Re: [PATCH v3 04/14] GPIO: Add gpio-ingenic driver

[Linus Walleij]

On Thu, Feb 9, 2017 at 6:14 PM, Paul Cercueil <paul@crapouillou.net> wrote:

>> If you're not just replacing these with GPIO_GENERIC, please also
>> include a .get_direction() callback.
>
> My .direction_input() and .direction_output() callbacks just call into
> the pinctrl driver, using pinctrl_gpio_direction_[in,out]put().
> I didn't find a way to get the direction info from the pinctrl driver,
> is that something that the core should provide?

Hm OK you have a clear point there, there is no such callback.

OK I do not require you to fix that at this time.

I am hesitant about providing ever more callbacks from GPIO
to pin control, I might need some help for consolidation here.

With Mika's patches we have a .set_config() call to
pinctrl_gpio_set_config() so essentially
we *could* actually refactor all pin control drivers providing
a GPIO back-end to use:

pinctrl_gpio_set_config(gpio, PIN_CONF_PACKED(PIN_CONFIG_INPUT_ENABLE, 0));
pinctrl_gpio_set_config(gpio, PIN_CONF_PACKED(PIN_CONFIG_OUTPUT, val));

And replace the calls to pinctrl_gpio_direction_input()
and pinctrl_gpio_direction_output() with this throughout.

It makes things a bit simpler. If we need to figure things
out the reverse direction then pinctrl_gpio_get_config()
should be implemented and used as back-end for
figuring out direction.

Yours,
Linus Walleij

Re: [PATCH v3 01/14] Documentation: dt/bindings: Document pinctrl-ingenic

[Linus Walleij]

On Thu, Feb 9, 2017 at 6:28 PM, Paul Cercueil <paul@crapouillou.net> wrote:

> I was thinking that instead of having one pinctrl-ingenic instance covering
> 0x600 of register space, and 6 instances of gpio-ingenic having 0x100 each,
> I could just have 6 instances of pinctrl-ingenic, each one with an instance
> of gpio-ingenic declared as a sub-node, each handling just 0x100 of memory
> space.

My head is spinning,  but I think I get it. What is wrong with the solution
I proposed with one pin control instance covering the whole 0x600 and with 6
subnodes of GPIO?

The GPIO nodes do not even have to have an address range associated with
them you know, that can be distributed out with regmap code accessing
the parent regmap.

Yours,
Linus Walleij

Re: [PATCH v3 01/14] Documentation: dt/bindings: Document pinctrl-ingenic

[Linus Walleij]

On Tue, Feb 21, 2017 at 12:20 PM, Paul Cercueil <paul@crapouillou.net> wrote:
> Le 2017-02-20 14:56, Linus Walleij a écrit :
>>
>> On Thu, Feb 9, 2017 at 6:28 PM, Paul Cercueil <paul@crapouillou.net>
>> wrote:
>>
>>> I was thinking that instead of having one pinctrl-ingenic instance
>>> covering
>>> 0x600 of register space, and 6 instances of gpio-ingenic having 0x100
>>> each,
>>> I could just have 6 instances of pinctrl-ingenic, each one with an
>>> instance
>>> of gpio-ingenic declared as a sub-node, each handling just 0x100 of
>>> memory
>>> space.
>>
>>
>> My head is spinning,  but I think I get it. What is wrong with the
>> solution
>> I proposed with one pin control instance covering the whole 0x600 and with
>> 6
>> subnodes of GPIO?
>>
>> The GPIO nodes do not even have to have an address range associated with
>> them you know, that can be distributed out with regmap code accessing
>> the parent regmap.
>
>
> OK, but then each GPIO chip 'X' still need to know its offset in the
> register
> area, which is (pinctrl_base + X * 0x100).
> What's the best way to pass that info to the driver? (I assume it's not with
> a custom DT binding...).

I do not really understand what driver you are referring to.

If the pin controller node is overarching and spawning children for
the gpiochips, you use the design pattern from MFD to pass data
from parents to children, e.g.:

#include <linux/regmap.h>

pinctrl driver:
     struct regmap_config mapconf = {
                .reg_bits = 32,
                .val_bits = 32,
                .reg_stride = 4,
     };
    struct regmap *map;

    map = regmap_init_mmio(dev, base, &mapconf);
    if (IS_ERR(map))
          ....
    dev_set_drvdata(dev, map);
    of_populate_children(dev,)..
    (can also use platform_device_add_data() or "simple-bus" etc)

gpio subdrivers:
     struct regmap *map;

     map = dev_get_drvdata(dev->parent);

There are examples of drivers passing more complex things to
their children than a regmap, just put some struct in a <linux/*/*.h>
file and pass it with drvdata as per above.

PS i2c_set_drvdata(), platform_set_drvdata() are just aliases
for dev_set_drvdata().

Yours,
Linus Walleij

Re: [PATCH v4 06/14] MIPS: jz4740: DTS: Add nodes for ingenic pinctrl and gpio drivers

[Sergei Shtylyov]

Hello!

On 4/2/2017 11:42 PM, Paul Cercueil wrote:

> For a description of the pinctrl devicetree node, please read
> Documentation/devicetree/bindings/pinctrl/ingenic,pinctrl.txt
>
> For a description of the gpio devicetree nodes, please read
> Documentation/devicetree/bindings/gpio/ingenic,gpio.txt
>
> Signed-off-by: Paul Cercueil <paul@crapouillou.net>
[...]

> diff --git a/arch/mips/boot/dts/ingenic/jz4740.dtsi b/arch/mips/boot/dts/ingenic/jz4740.dtsi
> index 3e1587f1f77a..9c23c877fc34 100644
> --- a/arch/mips/boot/dts/ingenic/jz4740.dtsi
> +++ b/arch/mips/boot/dts/ingenic/jz4740.dtsi
> @@ -55,6 +55,67 @@
>  		clock-names = "rtc";
>  	};
>
> +	pinctrl: ingenic-pinctrl@10010000 {

    The node name should be generic, so please rename it to something like 
"pin-controller@..."

> +		compatible = "ingenic,jz4740-pinctrl";
> +		reg = <0x10010000 0x400>;
> +
> +		gpa: gpio-controller@0 {

    The name should be just "gpio@0", according to ePAPR and its successor 
spec. Although, using the <unit-address> without the "reg" prop isn't allowed 
either...

[...]

MBR, Sergei

Re: [PATCH v4 06/14] MIPS: jz4740: DTS: Add nodes for ingenic pinctrl and gpio drivers

[Sergei Shtylyov]

On 4/3/2017 1:20 PM, Paul Cercueil wrote:

>>> For a description of the pinctrl devicetree node, please read
>>> Documentation/devicetree/bindings/pinctrl/ingenic,pinctrl.txt
>>>
>>> For a description of the gpio devicetree nodes, please read
>>> Documentation/devicetree/bindings/gpio/ingenic,gpio.txt
>>>
>>> Signed-off-by: Paul Cercueil <paul@crapouillou.net>
>> [...]
>>
>>> diff --git a/arch/mips/boot/dts/ingenic/jz4740.dtsi
>>> b/arch/mips/boot/dts/ingenic/jz4740.dtsi
>>> index 3e1587f1f77a..9c23c877fc34 100644
>>> --- a/arch/mips/boot/dts/ingenic/jz4740.dtsi
>>> +++ b/arch/mips/boot/dts/ingenic/jz4740.dtsi
>>> @@ -55,6 +55,67 @@
>>>          clock-names = "rtc";
>>>      };
>>>
>>> +    pinctrl: ingenic-pinctrl@10010000 {
>>
>>    The node name should be generic, so please rename it to something
>> like "pin-controller@..."
>
> OK.
>
>>> +        compatible = "ingenic,jz4740-pinctrl";
>>> +        reg = <0x10010000 0x400>;
>>> +
>>> +        gpa: gpio-controller@0 {
>>
>>    The name should be just "gpio@0", according to ePAPR and its
>> successor spec. Although, using the <unit-address> without the "reg"
>> prop isn't allowed either...
>
> ePAPR says: "If the node has no reg property, the unit-address may be

    My copy of ePAPR 1.1 says "must be omitted". :-)

> omitted if the node name alone differentiates the node from other nodes at
> the same level in the tree."

> I could use 'gpio@bank-a', it is allowed by the spec. Or do you prefer 'gpio@0'?

    Hm... maybe you should just use the "reg" prop.

> I'll wait from some feedback on the other patches then send a v5.
>
> Thanks,
> -Paul

MBR, Sergei

Re: [PATCH v4 04/14] GPIO: Add gpio-ingenic driver

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 980 bytes --]

Hi Paul,

[auto build test WARNING on linus/master]
[also build test WARNING on v4.11-rc5 next-20170403]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Paul-Cercueil/Ingenic-JZ4740-JZ4780-pinctrl-driver/20170403-184309
config: xtensa-allmodconfig (attached as .config)
compiler: xtensa-linux-gcc (GCC) 4.9.0
reproduce:
        wget https://raw.githubusercontent.com/01org/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        make.cross ARCH=xtensa 

All warnings (new ones prefixed by >>):

warning: (GPIO_INGENIC) selects PINCTRL_INGENIC which has unmet direct dependencies (PINCTRL && (MACH_INGENIC || COMPILE_TEST))

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 49094 bytes --]

Re: [PATCH v4 01/14] dt/bindings: Document pinctrl-ingenic

[Rob Herring]

On Sun, Apr 02, 2017 at 10:42:31PM +0200, Paul Cercueil wrote:
> This commit adds documentation for the devicetree bindings of the
> pinctrl-ingenic driver, which handles pin configuration and pin
> muxing of the Ingenic SoCs currently supported by the Linux kernel.
> 
> Signed-off-by: Paul Cercueil <paul@crapouillou.net>
> ---
>  .../bindings/pinctrl/ingenic,pinctrl.txt           | 41 ++++++++++++++++++++++
>  1 file changed, 41 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/pinctrl/ingenic,pinctrl.txt

Acked-by: Rob Herring <robh@kernel.org>

Re: [PATCH v4 02/14] dt/bindings: Document gpio-ingenic

[Rob Herring]

On Sun, Apr 02, 2017 at 10:42:32PM +0200, Paul Cercueil wrote:
> This commit adds documentation for the devicetree bindings of the
> gpio-ingenic driver, which handles GPIOs of the Ingenic SoCs
> currently supported by the Linux kernel.
> 
> Signed-off-by: Paul Cercueil <paul@crapouillou.net>
> ---
>  .../devicetree/bindings/gpio/ingenic,gpio.txt      | 48 ++++++++++++++++++++++
>  1 file changed, 48 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/gpio/ingenic,gpio.txt
> 
>  v2: New patch
>  v3: No changes
>  v4: Update for the v4 version of the gpio-ingenic driver
> 
> diff --git a/Documentation/devicetree/bindings/gpio/ingenic,gpio.txt b/Documentation/devicetree/bindings/gpio/ingenic,gpio.txt
> new file mode 100644
> index 000000000000..f54af444f7c3
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/gpio/ingenic,gpio.txt
> @@ -0,0 +1,48 @@
> +Ingenic jz47xx GPIO controller
> +
> +That the Ingenic GPIO driver node must be a sub-node of the Ingenic pinctrl
> +driver node.
> +
> +Required properties:
> +--------------------
> +
> + - compatible: Must contain one of:
> +    - "ingenic,jz4740-gpio"
> +    - "ingenic,jz4770-gpio"
> +    - "ingenic,jz4780-gpio"
> +	And one of:
> +	- "ingenic,gpio-bank-a"
> +	- "ingenic,gpio-bank-b"
> +	- "ingenic,gpio-bank-c"
> +	- "ingenic,gpio-bank-d"
> +	- "ingenic,gpio-bank-e" (for SoC version > jz4740)
> +	- "ingenic,gpio-bank-f" (for SoC version > jz4740)

This is quite strange. Why do you need the bank? Doesn't gpio-ranges 
give you that info? Maybe use reg property here instead.

> + - interrupt-controller: Marks the device node as an interrupt controller.
> + - interrupts: Interrupt specifier for the controllers interrupt.
> + - #interrupt-cells: Should be 2. Refer to
> +   ../interrupt-controller/interrupts.txt for more details.
> + - gpio-controller: Marks the device node as a GPIO controller.
> + - #gpio-cells: Should be 2. The first cell is the GPIO number and the second
> +    cell specifies GPIO flags, as defined in <dt-bindings/gpio/gpio.h>. Only the
> +    GPIO_ACTIVE_HIGH and GPIO_ACTIVE_LOW flags are supported.
> + - gpio-ranges: Range of pins managed by the GPIO controller. Refer to
> +   'gpio.txt' in this directory for more details.
> +
> +Example:
> +--------
> +
> +&pinctrl {
> +	gpa: gpio-controller@0 {

gpio@...

> +		compatible = "ingenic,gpio-bank-a", "ingenic,jz4740-gpio";
> +
> +		gpio-controller;
> +		gpio-ranges = <&pinctrl 0 0 32>;
> +		#gpio-cells = <2>;
> +
> +		interrupt-controller;
> +		#interrupt-cells = <2>;
> +
> +		interrupt-parent = <&intc>;
> +		interrupts = <28>;
> +	};
> +};
> -- 
> 2.11.0
> 

Re: [PATCH v4 13/14] pwm: jz4740: Let the pinctrl driver configure the pins

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 1455 bytes --]

On Sun, Apr 02, 2017 at 10:42:43PM +0200, Paul Cercueil wrote:
> Now that the JZ4740 and similar SoCs have a pinctrl driver, we rely on
> the pins being properly configured before the driver probes.
> 
> One inherent problem of this new approach is that the pinctrl framework
> does not allow us to configure each pin on demand, when the various PWM
> channels are requested or released. For instance, the PWM channels can
> be configured from sysfs, which would require all PWM pins to be configured
> properly beforehand for the PWM function, eventually causing conflicts
> with other platform or board drivers.
> 
> The proper solution here would be to modify the pwm-jz4740 driver to
> handle only one PWM channel, and create an instance of this driver
> for each one of the 8 PWM channels. Then, it could use the pinctrl
> framework to dynamically configure the PWM pin it controls.
> 
> Until this can be done, the only jz4740 board supported upstream
> (Qi lb60) can configure all of its connected PWM pins in PWM function
> mode, since those are not used by other drivers nor by GPIOs on the
> board.
> 
> Signed-off-by: Paul Cercueil <paul@crapouillou.net>
> ---
>  drivers/pwm/pwm-jz4740.c | 29 -----------------------------
>  1 file changed, 29 deletions(-)

Assuming that you want to take this through the pinctrl tree along with
the remainder of the series:

Acked-by: Thierry Reding <thierry.reding@gmail.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v4 04/14] GPIO: Add gpio-ingenic driver

[Linus Walleij]

On Sun, Apr 2, 2017 at 10:42 PM, Paul Cercueil <paul@crapouillou.net> wrote:

> This driver handles the GPIOs of all the Ingenic JZ47xx SoCs
> currently supported by the upsteam Linux kernel.
>
> Signed-off-by: Paul Cercueil <paul@crapouillou.net>

I guess you saw the Kconfig complaint from the build robot, please fix that
so we get silent builds.

>  v2: Consider it's a new patch. Completely rewritten from v1.
>  v3: Add missing include <linux/pinctrl/consumer.h> and drop semicolon after }
>  v4: Completely rewritten from v3.

I really like v4 :)

> +static inline bool gpio_get_value(struct ingenic_gpio_chip *jzgc, u8 offset)

Actually the return value should be an int.

I know, it is a historical artifact, if we change it we need to change
it everywhere.

> +       /* DO NOT EXPAND THIS: FOR BACKWARD GPIO NUMBERSPACE COMPATIBIBILITY
> +        * ONLY: WORK TO TRANSITION CONSUMERS TO USE THE GPIO DESCRIPTOR API IN
> +        * <linux/gpio/consumer.h> INSTEAD.
> +        */
> +       jzgc->gc.base = cell->id * 32;

OK then :)

This is merge material.

Yours,
Linus Walleij

Re: [PATCH v4 03/14] pinctrl-ingenic: add a pinctrl driver for the Ingenic jz47xx SoCs

[Linus Walleij]

On Sun, Apr 2, 2017 at 10:42 PM, Paul Cercueil <paul@crapouillou.net> wrote:

> This driver handles pin configuration and pin muxing for the
> JZ4740 and JZ4780 SoCs from Ingenic.
>
> Signed-off-by: Paul Cercueil <paul@crapouillou.net>
(...)
> +       select MFD_CORE
(...)
> +#include <linux/mfd/core.h>

That's unorthodox. Still quite pretty!
I would nee Lee Jones to say something about this, as it is
essentially hijacking MFD into the pinctrl subsystem.

> +static struct mfd_cell ingenic_pinctrl_mfd_cells[] = {
> +       {
> +               .id = 0,
> +               .name = "GPIOA",
> +               .of_compatible = "ingenic,gpio-bank-a",
> +       },
> +       {
> +               .id = 1,
> +               .name = "GPIOB",
> +               .of_compatible = "ingenic,gpio-bank-b",
> +       },
> +       {
> +               .id = 2,
> +               .name = "GPIOC",
> +               .of_compatible = "ingenic,gpio-bank-c",
> +       },
> +       {
> +               .id = 3,
> +               .name = "GPIOD",
> +               .of_compatible = "ingenic,gpio-bank-d",
> +       },
> +       {
> +               .id = 4,
> +               .name = "GPIOE",
> +               .of_compatible = "ingenic,gpio-bank-e",
> +       },
> +       {
> +               .id = 5,
> +               .name = "GPIOF",
> +               .of_compatible = "ingenic,gpio-bank-f",
> +       },
> +};
(...)
> +       err = devm_mfd_add_devices(dev, 0, ingenic_pinctrl_mfd_cells,
> +                       ARRAY_SIZE(ingenic_pinctrl_mfd_cells), NULL, 0, NULL);
> +       if (err) {
> +               dev_err(dev, "Failed to add MFD devices\n");
> +               return err;
> +       }

I guess the alternative would be to reimplement the MFD structure.

Did you check the approach to use "simple-mfd" and just let the subnodes
spawn as devices that way? I guess you did and this adds something
necessary.

Yours,
Linus Walleij

Re: [PATCH v4 06/14] MIPS: jz4740: DTS: Add nodes for ingenic pinctrl and gpio drivers

[Linus Walleij]

On Sun, Apr 2, 2017 at 10:42 PM, Paul Cercueil <paul@crapouillou.net> wrote:

> For a description of the pinctrl devicetree node, please read
> Documentation/devicetree/bindings/pinctrl/ingenic,pinctrl.txt
>
> For a description of the gpio devicetree nodes, please read
> Documentation/devicetree/bindings/gpio/ingenic,gpio.txt
>
> Signed-off-by: Paul Cercueil <paul@crapouillou.net>
> ---
>  arch/mips/boot/dts/ingenic/jz4740.dtsi | 61 ++++++++++++++++++++++++++++++++++
>  1 file changed, 61 insertions(+)
>
>  v2: Changed the devicetree bindings to match the new driver
>  v3: No changes
>  v4: Update the bindings for the v4 version of the drivers
(...)

> +       pinctrl: ingenic-pinctrl@10010000 {
> +               compatible = "ingenic,jz4740-pinctrl";
> +               reg = <0x10010000 0x400>;
> +
> +               gpa: gpio-controller@0 {
> +                       compatible = "ingenic,gpio-bank-a", "ingenic,jz4740-gpio";

As Sergei and Rob notes, the bank compatible properties look
a bit strange. Especially if they are all the same essentially.

I like Sergei's idea to simply use the reg property if what you want
is really a unique ID number. What do you think about this?

Yours,
Linus Walleij

Re: [PATCH v4 03/14] pinctrl-ingenic: add a pinctrl driver for the Ingenic jz47xx SoCs

[Lee Jones]

On Fri, 07 Apr 2017, Linus Walleij wrote:

> On Sun, Apr 2, 2017 at 10:42 PM, Paul Cercueil <paul@crapouillou.net> wrote:
> 
> > This driver handles pin configuration and pin muxing for the
> > JZ4740 and JZ4780 SoCs from Ingenic.
> >
> > Signed-off-by: Paul Cercueil <paul@crapouillou.net>
> (...)
> > +       select MFD_CORE
> (...)
> > +#include <linux/mfd/core.h>
> 
> That's unorthodox. Still quite pretty!
> I would nee Lee Jones to say something about this, as it is
> essentially hijacking MFD into the pinctrl subsystem.
> 
> > +static struct mfd_cell ingenic_pinctrl_mfd_cells[] = {
> > +       {
> > +               .id = 0,
> > +               .name = "GPIOA",
> > +               .of_compatible = "ingenic,gpio-bank-a",
> > +       },
> > +       {
> > +               .id = 1,
> > +               .name = "GPIOB",
> > +               .of_compatible = "ingenic,gpio-bank-b",
> > +       },
> > +       {
> > +               .id = 2,
> > +               .name = "GPIOC",
> > +               .of_compatible = "ingenic,gpio-bank-c",
> > +       },
> > +       {
> > +               .id = 3,
> > +               .name = "GPIOD",
> > +               .of_compatible = "ingenic,gpio-bank-d",
> > +       },
> > +       {
> > +               .id = 4,
> > +               .name = "GPIOE",
> > +               .of_compatible = "ingenic,gpio-bank-e",
> > +       },
> > +       {
> > +               .id = 5,
> > +               .name = "GPIOF",
> > +               .of_compatible = "ingenic,gpio-bank-f",
> > +       },
> > +};
> (...)
> > +       err = devm_mfd_add_devices(dev, 0, ingenic_pinctrl_mfd_cells,
> > +                       ARRAY_SIZE(ingenic_pinctrl_mfd_cells), NULL, 0, NULL);
> > +       if (err) {
> > +               dev_err(dev, "Failed to add MFD devices\n");
> > +               return err;
> > +       }
> 
> I guess the alternative would be to reimplement the MFD structure.
> 
> Did you check the approach to use "simple-mfd" and just let the subnodes
> spawn as devices that way? I guess you did and this adds something
> necessary.

I'd like to hear what the OP has to say about why this is necessary.
However, as a first glimpse, I'm dead against exporting MFD
functionality to outside of the subsystem.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH v4 06/14] MIPS: jz4740: DTS: Add nodes for ingenic pinctrl and gpio drivers

[Linus Walleij]

On Fri, Apr 7, 2017 at 3:57 PM, Paul Cercueil <paul@crapouillou.net> wrote:

> I think the 'reg' property makes more sense, yes. I'll fix this in the v5,
> this
> week-end. Do you think it can go in 4.12?

Surely, Torvalds just cut an -rc8 giving me more time to queue more
material, and I really like this series.

Yours,
Linus Walleij

Re: [PATCH v5 02/14] dt/bindings: Document gpio-ingenic

[Rob Herring]

On Fri, Apr 28, 2017 at 10:08:12PM +0200, Paul Cercueil wrote:
> This commit adds documentation for the devicetree bindings of the
> gpio-ingenic driver, which handles GPIOs of the Ingenic SoCs
> currently supported by the Linux kernel.
> 
> Signed-off-by: Paul Cercueil <paul@crapouillou.net>
> ---
>  .../devicetree/bindings/gpio/ingenic,gpio.txt      | 46 ++++++++++++++++++++++
>  1 file changed, 46 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/gpio/ingenic,gpio.txt
> 
>  v2: New patch
>  v3: No changes
>  v4: Update for the v4 version of the gpio-ingenic driver
>  v5: Remove gpio-bank-... compatible strings, and add 'reg' property

Acked-by: Rob Herring <robh@kernel.org>

Re: [PATCH v5 03/14] pinctrl: add a pinctrl driver for the Ingenic jz47xx SoCs

[Linus Walleij]

On Wed, May 3, 2017 at 11:12 AM, Paul Cercueil <paul@crapouillou.net> wrote:

> The dependency on MFD is gone but now I notice I forgot to remove the
> 'select MFD_CORE'
> in the Kconfig. It'd be great if you can make a quick edit when merging
> this,
> otherwise I'll send a v6.

No problem I can fix it up.

Yours,
Linus Walleij

Re: [PATCH v5 04/14] GPIO: Add gpio-ingenic driver

[Linus Walleij]

On Mon, May 8, 2017 at 12:05 AM, Paul Cercueil <paul@crapouillou.net> wrote:

> It looks like the gpio_get_value() is broken on jz4740.
>
> I'll send a v6.

OK I don't apply this series then, waiting for v6.

The series looks good so as soon you think it is finished
I can apply it I think.

Yours,
Linus Walleij

Re: [PATCH v5 05/14] MIPS: ingenic: Enable pinctrl for all ingenic SoCs

[Linus Walleij]

On Fri, Apr 28, 2017 at 10:08 PM, Paul Cercueil <paul@crapouillou.net> wrote:

> There is a pinctrl driver for each of the Ingenic SoCs supported by the
> upstream Linux kernel. In order to switch away from the old GPIO
> platform code, we now enable the pinctrl drivers by default for the
> Ingenic SoCs.
>
> Signed-off-by: Paul Cercueil <paul@crapouillou.net>
> ---
>  arch/mips/Kconfig | 1 +

So please tell me your desired merge strategy for these bits. I can
provide an immutable branch for pinctrl if you want to pull the deps in
or we can just merge this orthogonally in the MIPS tree and let things
smoothen together in the merge window.

This goes for everything outside of pinctrl/gpio.

If I should merge patches for other subsystems I need ACKs from
the maintainers of MIPS etc.

Yours,
Linus Walleij

Re: [PATCH v5 05/14] MIPS: ingenic: Enable pinctrl for all ingenic SoCs

[Linus Walleij]

On Fri, Apr 28, 2017 at 10:08 PM, Paul Cercueil <paul@crapouillou.net> wrote:

> There is a pinctrl driver for each of the Ingenic SoCs supported by the
> upstream Linux kernel. In order to switch away from the old GPIO
> platform code, we now enable the pinctrl drivers by default for the
> Ingenic SoCs.
>
> Signed-off-by: Paul Cercueil <paul@crapouillou.net>

I applied all the patches to a branch in pinctrl and merged into my
devel branch for testing:

https://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-pinctrl.git/
Branch: ingenic

Ralf: are you OK with this? It would be nice to have your ACK on
all patches. If you want you can pull this branch into the MIPS
tree, or we can hope for it all to settle nicely because of low platform
activity in MIPS on this platform, so it only needs to come in
from my trees.

Yours,
Linus Walleij

Re: [PATCH v5 05/14] MIPS: ingenic: Enable pinctrl for all ingenic SoCs

[Paul Cercueil]

Hi Linus,

> I applied all the patches to a branch in pinctrl and merged into my
> devel branch for testing:
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-pinctrl.git/
> Branch: ingenic
> 
> Ralf: are you OK with this? It would be nice to have your ACK on
> all patches. If you want you can pull this branch into the MIPS
> tree, or we can hope for it all to settle nicely because of low 
> platform
> activity in MIPS on this platform, so it only needs to come in
> from my trees.

There has been no word from Ralf, is this going into 4.13?

Re: [PATCH v5 05/14] MIPS: ingenic: Enable pinctrl for all ingenic SoCs

[Linus Walleij]

On Sun, Jul 2, 2017 at 6:35 PM, Paul Cercueil <paul@crapouillou.net> wrote:
> Hi Linus,
>
>> I applied all the patches to a branch in pinctrl and merged into my
>> devel branch for testing:
>>
>> https://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-pinctrl.git/
>> Branch: ingenic
>>
>> Ralf: are you OK with this? It would be nice to have your ACK on
>> all patches. If you want you can pull this branch into the MIPS
>> tree, or we can hope for it all to settle nicely because of low platform
>> activity in MIPS on this platform, so it only needs to come in
>> from my trees.
>
>
> There has been no word from Ralf, is this going into 4.13?

Yes.

Yours,
Linus Walleij

Re: [PATCH v5 05/14] MIPS: ingenic: Enable pinctrl for all ingenic SoCs

[Ralf Baechle]

On Mon, Jul 03, 2017 at 11:07:09AM +0200, Linus Walleij wrote:

> > There has been no word from Ralf, is this going into 4.13?

Acked-by: Ralf Baechle <ralf@linux-mips.org>

for the whole series.

Thanks,

  Ralf

Re: [PATCH v5 05/14] MIPS: ingenic: Enable pinctrl for all ingenic SoCs

[Linus Walleij]

On Mon, Jul 3, 2017 at 3:55 PM, Ralf Baechle <ralf@linux-mips.org> wrote:
> On Mon, Jul 03, 2017 at 11:07:09AM +0200, Linus Walleij wrote:
>
>> > There has been no word from Ralf, is this going into 4.13?
>
> Acked-by: Ralf Baechle <ralf@linux-mips.org>
>
> for the whole series.

Thanks Ralf, I missed to add this in, but everything seems to have
landed smoothly in v4.13.

Yours,
Linus Walleij