Re: [PATCH] pinmux: add a driver for the CSR SiRFprimaII pinmux

[Linus Walleij]

On Fri, Aug 26, 2011 at 5:36 AM, Barry Song <Baohua.Song@xxxxxxx> wrote:

> Though there are still many discussions about data model and device/function
> mapping, pinmux core is basically usable to CSR SiRFprimaII for the moment.
>
> This patch is another example to use Linus W's pinmux framework.
>
> Tests by this example show basic pinmux APIs like pinmux_get, pinmux_enable,
> pinmux_disable and pinmux_put are able to work in Linus W's patch v4.

Thanks a lot Barry, if you rebase this to the v5 patch set I can probably
carry it as part of my set.

> diff --git a/drivers/pinctrl/pinmux-sirf.c b/drivers/pinctrl/pinmux-sirf.c
> +#include <linux/of.h>
> +#include <linux/of_address.h>
> +#include <linux/of_device.h>
> +#include <linux/of_platform.h>

It's especially nice to see these DT-bindings!

> +static const struct sirfsoc_muxmask lcd_16bits_sirfsoc_muxmask[] = {
> +       {
> +               .group = 3,
> +               .mask = 0x7FFFF,

These things look a bit magic...

Maybe: #define SIRFSOC_PMX_ALLBITS 0x7FFFF
(Just a suggestion)

> +       }, {
> +               .group = 2,
> +               .mask = 1 << 31,

This too.

To make it clearer:

#include <linux/bitops.h>

.mask = BIT(31),

> +       },
> +};
> +
> +static const struct sirfsoc_padmux lcd_16bits_padmux = {
> +       .muxmask_counts = ARRAY_SIZE(lcd_16bits_sirfsoc_muxmask),
> +       .muxmask = lcd_16bits_sirfsoc_muxmask,
> +       .funcmask = 1 << 4,
> +       .funcval = 0 << 4,

BIT(1)
BIT(0)

> +};
> +
> +static const unsigned lcd_16bits_pins[] = { 95, 96, 97, 98, 99, 100, 101, 102, 103, 104,
> +       105, 106, 107, 108, 109, 110, 111, 112, 113, 114 };
> +
> +static const struct sirfsoc_muxmask lcd_18bits_muxmask[] = {
> +       {
> +               .group = 3,
> +               .mask = 0x7FFFF,
> +       }, {
> +               .group = 2,
> +               .mask = 1 << 31,

BIT(31)

> +       }, {
> +               .group = 0,
> +               .mask = (1 << 16) | (1 << 17),

BIT(16)|BIT(17)

(etc)

> +static int sirfsoc_pinmux_enable(struct pinctrl_dev *pmxdev, unsigned selector)

These will have an optional "position" parameter now, which you can
ignore for this simple driver. (Compare U300 pinmux v5).

> +{
> +       struct sirfsoc_pmx *upmx;

I think "upmx" = U300 padmux :-D

Maybe rename it something like spmx or so, globally...

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/