Messages in this thread | | | From | Barry Song <> | Date | Mon, 29 Aug 2011 19:58:53 +0800 | Subject | Re: [PATCH] pinmux: add a driver for the CSR SiRFprimaII pinmux |
| |
Hi Linus, Thanks !
2011/8/29 Linus Walleij <linus.walleij@linaro.org> > > On Fri, Aug 26, 2011 at 5:36 AM, Barry Song <Baohua.Song@csr.com> 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |