Re: [PATCH v2] GPIO: add Microchip MCP23017 / MCP23008 GPIO driver

["Florian Vallée" <fvallee@eukrea.fr>]

On 15 December 2015 at 20:15, Trent Piepho <tpiepho@kymetacorp.com> wrote:
> On Tue, 2015-12-15 at 18:28 +0100, Florian Vallee wrote:
>> +
>> +/* A given spi_device can represent up to eight mcp23sxx chips
>> + * sharing the same chipselect but using different addresses
>> + * (e.g. chips #0 and #3 might be populated, but not #1 or $2).
>> + * Driver data holds all the per-chip data.
>> + */
>> +struct mcp23s08_driver_data {
>> +     unsigned                ngpio;
>> +     struct mcp23s08         *mcp[8];
>> +     struct mcp23s08         chip[];
>> +};
>
> It doesn't look like anything uses this struct.
>
>> +static int
>> +mcp23008_read_regs(struct mcp23s08 *mcp, unsigned reg, u16 *vals, unsigned n)
>> +{
>> +     while (n--) {
>> +             int ret = mcp23008_read(mcp, reg++);
>
> This could use mcp->ops->read(mcp, reg++) instead of mcp23008_read().
> Which would make it exactly the same as this function:
>
>> +static int
>> +mcp23017_read_regs(struct mcp23s08 *mcp, unsigned reg, u16 *vals, unsigned n)
>> +{
>> +     while (n--) {
>> +             int ret = mcp23017_read(mcp, reg++);
>> +             if (ret < 0)
>> +                     return ret;
>> +             *vals++ = ret;
>> +     }
>> +
>> +     return 0;
>> +}
>
> So just one read_regs fuction is needed.  Which also would allow
> read_regs to be removed from the ops struct since it's the same for both
> chips.
>

Should we go as far as removing the read_regs ops ? Doing so will
specialize "mcp23s08_probe_one" for the i2c version of the chip. If we
ever want to integrate back the SPI part of the linux driver, leaving
the op has a minimal cost while enabling us to keep mcp23s08_probe_one
in sync with the linux's version.

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox