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

[Barry Song]

Hi Linus,
Thanks !

2011/8/29 Linus Walleij <linus.walleij@xxxxxxxxxx>
>
> 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.

i'll rebase it to your v5 and make some tests.

>
> > 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...

register seems to be magic by itself. this is register set mask of
group 3 for LCD. group in the structure is also an offset of register
layout.

>
> Maybe: #define SIRFSOC_PMX_ALLBITS 0x7FFFF

0x7fff is only for all LCD. maybe "#define SIRFSOC_LCD_GROUP3_MASK
0x7FFFF". that seems ugly too :-)

> (Just a suggestion)
>
> > + Â Â Â }, {
> > + Â Â Â Â Â Â Â .group = 2,
> > + Â Â Â Â Â Â Â .mask = 1 << 31,
>
> This too.
>
> To make it clearer:
>
> #include <linux/bitops.h>
>
> .mask = BIT(31),

agree

>
> > + Â Â Â },
> > +};
> > +
> > +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)

agree

> 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)
>
agree

> > +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).

ok.

>
> > +{
> > + Â Â Â struct sirfsoc_pmx *upmx;
>
> I think "upmx" = U300 padmux :-D
>
> Maybe rename it something like spmx or so, globally...

could be.


>
> Yours,
> Linus Walleij
>

-barry
--
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/