lkml.org 
[lkml]   [2011]   [Feb]   [14]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
Date
SubjectRe: [PATCH] mcp23s08: support mcp23s17 variant
On Mon, Feb 14, 2011 at 11:29 AM, Peter Korsgaard <jacmet@sunsite.dk> wrote:
>>>>>> "Grant" == Grant Likely <grant.likely@secretlab.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@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

\
 
 \ /
  Last update: 2011-02-14 20:01    [W:0.042 / U:1.724 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site