Messages in this thread | | | From | Grant Likely <> | Date | Mon, 14 Feb 2011 11:59:16 -0700 | Subject | Re: [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/
| |