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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Mon, 29 Aug 2011 19:58:53 +0800
From:	Barry Song <21cnbao@...il.com>
To:	Linus Walleij <linus.walleij@...aro.org>
Cc:	Barry Song <Baohua.Song@....com>, linus.walleij@...ricsson.com,
	linux-kernel@...r.kernel.org, workgroup.linux@....com,
	grant.likely@...retlab.ca, Rongjun Ying <rongjun.ying@....com>,
	linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH] pinmux: add a driver for the CSR SiRFprimaII pinmux

Hi Linus,
Thanks !

2011/8/29 Linus Walleij <linus.walleij@...aro.org>
>
> On Fri, Aug 26, 2011 at 5:36 AM, Barry Song <Baohua.Song@....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@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/