Re: [PATCH v2 1/3] pinctrl: add driver for Amlogic Meson SoCs

[Linus Walleij]

On Tue, Oct 14, 2014 at 11:21 PM, Beniamino Galvani <b.galvani@xxxxxxxxx> wrote:

> This is a driver for the pinmux and GPIO controller available in
> Amlogic Meson SoCs. At the moment it only supports Meson8 devices,
> however other SoC families like Meson6 and Meson8b (the Cortex-A5
> variant) appear to be similar, with just different sets of banks and
> registers.
>
> GPIO interrupts are not supported at the moment due to lack of
> documentation.
>
> Signed-off-by: Beniamino Galvani <b.galvani@xxxxxxxxx>

(...)
>  arch/arm/mach-meson/Kconfig            |   3 +

Still not split off from the main patch.

> +static void meson_domain_set_bit(struct meson_domain *domain,
> +                                void __iomem *addr, unsigned int bit,
> +                                unsigned int value)

Still not migrated to regmap MMIO.

> +       /* Copy group and function definitions from domains to pinctrl */
> +       pc->groups = devm_kzalloc(pc->dev, pc->num_groups *
> +                                 sizeof(struct meson_pmx_group), GFP_KERNEL);
> +       pc->funcs = devm_kzalloc(pc->dev, pc->num_funcs *
> +                                 sizeof(struct meson_pmx_func), GFP_KERNEL);
> +       if (!pc->groups || !pc->funcs)
> +               return -ENOMEM;

Still lots of copying of data.

> +static int meson_gpiolib_register(struct meson_pinctrl *pc)
> +{
> +       struct meson_domain *domain;
> +       unsigned int base = 0;

No.

> +       int i, ret;
> +
> +       for (i = 0; i < pc->num_domains; i++) {
> +               domain = &pc->domains[i];
> +
> +               domain->chip.label = domain->data->name;
> +               domain->chip.dev = pc->dev;
> +               domain->chip.request = meson_gpio_request;
> +               domain->chip.free = meson_gpio_free;
> +               domain->chip.direction_input = meson_gpio_direction_input;
> +               domain->chip.direction_output = meson_gpio_direction_output;
> +               domain->chip.get = meson_gpio_get;
> +               domain->chip.set = meson_gpio_set;
> +               domain->chip.base = base;

No. Set it to -1 and let the gpiolib assign some random GPIO numbers.

> +               domain->chip.ngpio = domain->data->num_pins;
> +               domain->chip.names = domain->data->pin_names;
> +               domain->chip.can_sleep = false;
> +               domain->chip.of_node = domain->of_node;
> +               domain->chip.of_gpio_n_cells = 2;
> +               domain->chip.of_xlate = meson_gpio_of_xlate;

Go for the generic xlate function instead.

> +               domain->gpio_range.name = domain->data->name;
> +               domain->gpio_range.id = i;
> +               domain->gpio_range.base = base;
> +               domain->gpio_range.pin_base = base;
> +               domain->gpio_range.npins = domain->data->num_pins;
> +               domain->gpio_range.gc = &domain->chip;
> +
> +               pinctrl_add_gpio_range(pc->pcdev, &domain->gpio_range);

Don't add the gpio range from the pinctrl side, add it from the
gpiolib side using gpiochip_add_pin_range() instead.

> +static int __init meson_pinctrl_drv_register(void)
> +{
> +       return platform_driver_register(&meson_pinctrl_driver);
> +}
> +postcore_initcall(meson_pinctrl_drv_register);

Does it have to be this early?

> +/**
> + * struct meson_domain
> + *
> + * @reg_mux:   registers for mux settings
> + * @reg_pullen:        registers for pull-enable settings
> + * @reg_pull:  registers for pull settings
> + * @reg_gpio:  registers for gpio settings
> + * @mux_size:  size of mux register range (in words)
> + * @pullen_size:size of pull-enable register range
> + * @pull_size: size of pull register range
> + * @gpio_size: size of gpio register range
> + * @chip:      gpio chip associated with the domain
> + * @data;      platform data for the domain
> + * @node:      device tree node for the domain
> + * @gpio_range:        gpio range that maps domain gpios to the pin controller
> + * @lock:      spinlock for accessing domain registers
> + *
> + * A domain represents a set of banks controlled by the same set of
> + * registers. Typically there is a domain for the normal banks and
> + * another one for the Always-On bus.
> + */

Insert all the info from your previous mail.

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/