linux-kernel - Re: [PATCH] mcp23s08: support mcp23s17 variant

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 for Android: free password hash cracker in your pocket

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

Message-ID: <AANLkTikri_A1VknD3oLR2LwC4ysOgE20RnqMRdYnHbcP@mail.gmail.com>
Date:	Mon, 14 Feb 2011 11:59:16 -0700
From:	Grant Likely <grant.likely@...retlab.ca>
To:	Peter Korsgaard <jacmet@...site.dk>
Cc:	dbrownell@...rs.sourceforge.net, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] mcp23s08: support mcp23s17 variant

On Mon, Feb 14, 2011 at 11:29 AM, Peter Korsgaard <jacmet@...site.dk> wrote:
>>>>>> "Grant" == Grant Likely <grant.likely@...retlab.ca> writes:
>
> Hi,
>
>  >> -       return (status < 0) ? status : rx[0];
>  >> +
>  >> +       if (mcp->type == MCP_TYPE_S17) {
>  >> +               tx[1] <<= 1;
>  >> +
>  >> +               status = spi_write_then_read(mcp->spi, tx, sizeof tx, rx, 2);
>  >> +               return (status < 0) ? status : (rx[0] | (rx[1] << 8));
>  >> +       } else {
>  >> +               status = spi_write_then_read(mcp->spi, tx, sizeof tx, rx, 1);
>  >> +               return (status < 0) ? status : rx[0];
>  >> +       }
>
>  Grant> Rather than checking ->type for every transaction, would a set of
>  Grant> callbacks for each type be better?  It would probably have lower
>  Grant> overhead and be simpler to maintain.
>
> We could. I didn't do it because the 8bit and 16bit cases are very
> similar, and checking a simple integer is presumably a lot faster than
> the spi access - But I can rework the patch if you insists.

I won't insist, but it really does look like something that should
have separate accessors from a code maintenance perspective.

>
>  >> +++ b/include/linux/spi/mcp23s08.h
>  >> @@ -2,21 +2,24 @@
>  >>  /* FIXME driver should be able to handle IRQs...  */
>  >>
>  >>  struct mcp23s08_chip_info {
>  >> -       bool    is_present;             /* true iff populated */
>  >> -       u8      pullups;                /* BIT(x) means enable pullup x */
>  >> +       bool            is_present;     /* true if populated */
>  >> +       unsigned        pullups;        /* BIT(x) means enable pullup x */
>  >>  };
>
>  Grant> Unrelated whitespace changes.
>
> I wouldn't call it unrelated. It's to ensure it still lines up after the
> s/u8/unsigned/.

Sorry, you're right.  I missed the s/u8/unsigned/ change.  I don't
have any problem with whitespace changes in a hunk that is also
getting a real change.

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