| Message ID | 1362414873-17676-1-git-send-email-larsi@wh2.tu-dresden.de (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
Hi Lars, sorry for taking eternities to review stuff :-( I recommend that you include SPI co-maintainer Mark Brown on subsequent postings. On Mon, Mar 4, 2013 at 5:34 PM, Lars Poeschel <larsi@wh2.tu-dresden.de> wrote: > This converts the mcp23s08 driver to be able to be used with device > tree. OK! > +++ b/Documentation/devicetree/bindings/gpio/gpio-mcp23s08.txt > @@ -0,0 +1,43 @@ > +Microchip MCP2308/MCP23S08/MCP23017/MCP23S17 driver for > +8-/16-bit I/O expander with serial interface (I2C/SPI) > + > +Required properties: > +- compatible : Should be > + - "mcp,mcp23s08" for 8 GPIO SPI version > + - "mcp,mcp23s17" for 16 GPIO SPI version > + - "mcp,mcp23008" for 8 GPIO I2C version or > + - "mcp,mcp23017" for 16 GPIO I2C version of the chip > +- #gpio-cells : Should be two. > + - first cell is the pin number > + - second cell is used to specify flags. Flags currently used: > + bit0 : activate a ~100k pullup Pullup is basically about pin config. This is sort of sneaking behind the subsystems, but I know I might be overzealous. Can the electronics do more things than pull-up? Like pull-down, open drain, drive strength... If it's a lot it's better to consider pinctrl from the start. I'm saying this because the DT bindings will be maintained perpetually and need to set a good example. I would currently feel a lot better if you did not include this flag. How would you control this the day drivers need to enable/disable pull-up at runtime? > +- gpio-controller : Marks the device node as a GPIO controller. > +- reg : For an address on its bus On the I2C/SPI bus? Please state here what kind of buses it can be. Explain if multiple buses are supported. > +Required device specific properties (only for SPI chips): > +- mcp,spi-present-mask : This is a present flag, that makes only sense for SPI > + chips - as the name suggests. AFAIK this is not how we disable/enable devices in the device tree. Istead we include a property on the node called "status" and set it to "disabled" if the device is not there. > + Multiple chips can share the same > + SPI chipselect. Set bit 0-7 in this mask to 1 if there is a chip > + connected with this spi address. If you have a chip with address 3 > + connected, you have to set bit3 to 1, which is 0x08. mcp23s08 only > + supports bits 0-3. It is not possible to mix mcp23s08 and mcp23s17 > + on the same chipselect. Set at least one bit to 1 for SPI chips. This looks awkward, why are you using a bitfield for this? Then you can only ever support 8 devices, since the text also implies that the value is 8bit (this should be stated). What about just using a number? > diff --git a/drivers/gpio/gpio-mcp23s08.c b/drivers/gpio/gpio-mcp23s08.c > index 3cea0ea..a8ca469 100644 > --- a/drivers/gpio/gpio-mcp23s08.c > +++ b/drivers/gpio/gpio-mcp23s08.c > @@ -12,6 +12,8 @@ > #include <linux/spi/mcp23s08.h> > #include <linux/slab.h> > #include <asm/byteorder.h> > +#include <linux/of.h> > +#include <linux/of_device.h> > > /** > * MCP types supported by driver > @@ -21,6 +23,11 @@ > #define MCP_TYPE_008 2 > #define MCP_TYPE_017 3 > > +/** > + * Flags used in device tree > + */ > +#define MCP_DT_FLAG_PULLUP 0x01 So I'm sceptical here. Is this already supported using platform data? > /* Registers are all 8 bits wide. > * > * The mcp23s17 has twice as many bits, and can be configured to work > @@ -75,6 +82,25 @@ struct mcp23s08_driver_data { > struct mcp23s08 chip[]; > }; > > +#ifdef CONFIG_OF > +static int mcp23s08_of_xlate(struct gpio_chip *gc, > + const struct of_phandle_args *gpiospec, u32 *flags); > + > +static int mcp23s08_set_pullup(struct mcp23s08 *mcp, unsigned offset) > +{ > + int status; > + u16 value; > + > + mutex_lock(&mcp->lock); > + value = mcp->cache[MCP_GPPU] | (1 << offset); > + status = mcp->ops->write(mcp, MCP_GPPU, value); > + if (!status) > + mcp->cache[MCP_GPPU] = value; > + mutex_unlock(&mcp->lock); > + > + return status; > +} The pull-up business actually looks like new functionality that has nothing to do with adding device tree support and should be a separate patch. Yours, Linus Walleij ------------------------------------------------------------------------------ Everyone hates slow websites. So do we. Make your web apps faster with AppDynamics Download AppDynamics Lite for free today: http://p.sf.net/sfu/appdyn_d2d_mar
>>>>> "Linus" == Linus Walleij <linus.walleij@linaro.org> writes:
Linus> Hi Lars,
Linus> sorry for taking eternities to review stuff :-(
Linus> I recommend that you include SPI co-maintainer Mark Brown on
Linus> subsequent postings.
And me, as I'm the only one who has done any significant work on that
driver since David..
On Friday 22 March 2013 at 09:33:10, Linus Walleij wrote: > > +++ b/Documentation/devicetree/bindings/gpio/gpio-mcp23s08.txt > > @@ -0,0 +1,43 @@ > > +Microchip MCP2308/MCP23S08/MCP23017/MCP23S17 driver for > > +8-/16-bit I/O expander with serial interface (I2C/SPI) > > + > > +Required properties: > > +- compatible : Should be > > + - "mcp,mcp23s08" for 8 GPIO SPI version > > + - "mcp,mcp23s17" for 16 GPIO SPI version > > + - "mcp,mcp23008" for 8 GPIO I2C version or > > + - "mcp,mcp23017" for 16 GPIO I2C version of the chip > > +- #gpio-cells : Should be two. > > + - first cell is the pin number > > + - second cell is used to specify flags. Flags currently used: > > + bit0 : activate a ~100k pullup > > Pullup is basically about pin config. This is sort of sneaking > behind the subsystems, but I know I might be overzealous. > > Can the electronics do more things than pull-up? > > Like pull-down, open drain, drive strength... No pull-down, no open drain, no drive strength, nothing - only 100k pull-up. > If it's a lot it's better to consider pinctrl from the start. > I'm saying this because the DT bindings will be maintained > perpetually and need to set a good example. > > I would currently feel a lot better if you did not include this > flag. How would you control this the day drivers need to > enable/disable pull-up at runtime? For me it would also be easier to remove the flag (for DT boot case). I don't need the pullups. I did only include it, because this is how the driver is designed to work and what it did already. What I wanted was to make a good transfer from what the features of the driver are and how it works to be able to use it with device tree. If it now turns out that's bad for some reason, I have no problem to remove this from the patch completely. You're right: Runtime config is not possible this way. What should I do now ? Remove it ? > > +- gpio-controller : Marks the device node as a GPIO controller. > > +- reg : For an address on its bus > > On the I2C/SPI bus? Yes, both. For I2C it's the I2C address, for SPI it's the chip select to use for this chip. > Please state here what kind of buses it can be. Explain if multiple > buses are supported. Ok, I will add a line about it. > > +Required device specific properties (only for SPI chips): > > +- mcp,spi-present-mask : This is a present flag, that makes only sense > > for SPI + chips - as the name suggests. > > AFAIK this is not how we disable/enable devices in the device tree. > > Istead we include a property on the node called "status" and set it > to "disabled" if the device is not there. This would require multiple instances with the same reg property as up to 8 chips can live on the same chip select. I wonder if this is possible ? > > + Multiple chips can share the same > > + SPI chipselect. Set bit 0-7 in this mask to 1 if there is a > > chip + connected with this spi address. If you have a chip with > > address 3 + connected, you have to set bit3 to 1, which is 0x08. > > mcp23s08 only + supports bits 0-3. It is not possible to mix > > mcp23s08 and mcp23s17 + on the same chipselect. Set at least one > > bit to 1 for SPI chips. > > This looks awkward, why are you using a bitfield for this? Then you > can only ever support 8 devices, since the text also implies that the > value is 8bit (this should be stated). Grant had the idea with the bitfield. You have the reg property specifying the chip select line. This bitfield is then used to indicate which of the 8 possible chips on this same chip select line is really present. Not beeing able to support more than 8 devices is not problem, because it is a hardware limitation, that not more than 8 devices can share the same SPI chip select. This is again how the driver worked so far. > What about just using a number? This would again require multiple instances with the same reg property for SPI. Is this really possible ? > > diff --git a/drivers/gpio/gpio-mcp23s08.c b/drivers/gpio/gpio-mcp23s08.c > > index 3cea0ea..a8ca469 100644 > > --- a/drivers/gpio/gpio-mcp23s08.c > > +++ b/drivers/gpio/gpio-mcp23s08.c > > @@ -12,6 +12,8 @@ > > > > #include <linux/spi/mcp23s08.h> > > #include <linux/slab.h> > > #include <asm/byteorder.h> > > > > +#include <linux/of.h> > > +#include <linux/of_device.h> > > > > /** > > > > * MCP types supported by driver > > > > @@ -21,6 +23,11 @@ > > > > #define MCP_TYPE_008 2 > > #define MCP_TYPE_017 3 > > > > +/** > > + * Flags used in device tree > > + */ > > +#define MCP_DT_FLAG_PULLUP 0x01 > > So I'm sceptical here. Is this already supported using platform data? Yes, setting the pullups using platform data on probe time is supported. > > /* Registers are all 8 bits wide. > > > > * > > * The mcp23s17 has twice as many bits, and can be configured to work > > > > @@ -75,6 +82,25 @@ struct mcp23s08_driver_data { > > > > struct mcp23s08 chip[]; > > > > }; > > > > +#ifdef CONFIG_OF > > +static int mcp23s08_of_xlate(struct gpio_chip *gc, > > + const struct of_phandle_args *gpiospec, u32 > > *flags); + > > +static int mcp23s08_set_pullup(struct mcp23s08 *mcp, unsigned offset) > > +{ > > + int status; > > + u16 value; > > + > > + mutex_lock(&mcp->lock); > > + value = mcp->cache[MCP_GPPU] | (1 << offset); > > + status = mcp->ops->write(mcp, MCP_GPPU, value); > > + if (!status) > > + mcp->cache[MCP_GPPU] = value; > > + mutex_unlock(&mcp->lock); > > + > > + return status; > > +} > > The pull-up business actually looks like new functionality that > has nothing to do with adding device tree support and should be > a separate patch. I can put this in a seperate patch if you want. There still is the general question: Remove the pullup thing from the device tree boot or keep it in ? Regards, Lars ------------------------------------------------------------------------------ Own the Future-Intel® Level Up Game Demo Contest 2013 Rise to greatness in Intel's independent game demo contest. Compete for recognition, cash, and the chance to get your game on Steam. $5K grand prize plus 10 genre and skill prizes. Submit your demo by 6/6/13. http://p.sf.net/sfu/intel_levelupd2d
On Wed, Mar 27, 2013 at 12:35 PM, Lars Poeschel <poeschel@lemonage.de> wrote: > On Friday 22 March 2013 at 09:33:10, Linus Walleij wrote: >> I would currently feel a lot better if you did not include this >> flag. How would you control this the day drivers need to >> enable/disable pull-up at runtime? > > For me it would also be easier to remove the flag (for DT boot case). I don't > need the pullups. I did only include it, because this is how the driver is > designed to work and what it did already. What I wanted was to make a good > transfer from what the features of the driver are and how it works to be able > to use it with device tree. If it now turns out that's bad for some reason, I > have no problem to remove this from the patch completely. > You're right: Runtime config is not possible this way. > What should I do now ? Remove it ? Remove. The less complicated binding, the better. >> > +- gpio-controller : Marks the device node as a GPIO controller. >> > +- reg : For an address on its bus >> >> On the I2C/SPI bus? > > Yes, both. For I2C it's the I2C address, for SPI it's the chip select to use > for this chip. OK please write these specifics in the binding doc. >> Please state here what kind of buses it can be. Explain if multiple >> buses are supported. > > Ok, I will add a line about it. Thx. >> > +Required device specific properties (only for SPI chips): >> > +- mcp,spi-present-mask : This is a present flag, that makes only sense >> > for SPI + chips - as the name suggests. >> >> AFAIK this is not how we disable/enable devices in the device tree. >> >> Istead we include a property on the node called "status" and set it >> to "disabled" if the device is not there. > > This would require multiple instances with the same reg property as up to 8 > chips can live on the same chip select. I wonder if this is possible ? If there is not one instance/device node per chip things are very wrong anyway. Maybe I don't understand fully... each device on the system should have a node, you can't have a node spanning several devices unless it's a bus node. > Grant had the idea with the bitfield. You have the reg property specifying > the chip select line. This bitfield is then used to indicate which of the 8 > possible chips on this same chip select line is really present. Not beeing > able to support more than 8 devices is not problem, because it is a hardware > limitation, that not more than 8 devices can share the same SPI chip select. > This is again how the driver worked so far. > >> What about just using a number? > > This would again require multiple instances with the same reg property for > SPI. Is this really possible ? If Grant is OK with this then so am I, he surely know this better than me. But you nee to trick him to come out and review it. Yours, Linus Walleij ------------------------------------------------------------------------------ Own the Future-Intel® Level Up Game Demo Contest 2013 Rise to greatness in Intel's independent game demo contest. Compete for recognition, cash, and the chance to get your game on Steam. $5K grand prize plus 10 genre and skill prizes. Submit your demo by 6/6/13. http://p.sf.net/sfu/intel_levelupd2d
diff --git a/Documentation/devicetree/bindings/gpio/gpio-mcp23s08.txt b/Documentation/devicetree/bindings/gpio/gpio-mcp23s08.txt new file mode 100644 index 0000000..99a2985 --- /dev/null +++ b/Documentation/devicetree/bindings/gpio/gpio-mcp23s08.txt @@ -0,0 +1,43 @@ +Microchip MCP2308/MCP23S08/MCP23017/MCP23S17 driver for +8-/16-bit I/O expander with serial interface (I2C/SPI) + +Required properties: +- compatible : Should be + - "mcp,mcp23s08" for 8 GPIO SPI version + - "mcp,mcp23s17" for 16 GPIO SPI version + - "mcp,mcp23008" for 8 GPIO I2C version or + - "mcp,mcp23017" for 16 GPIO I2C version of the chip +- #gpio-cells : Should be two. + - first cell is the pin number + - second cell is used to specify flags. Flags currently used: + bit0 : activate a ~100k pullup +- gpio-controller : Marks the device node as a GPIO controller. +- reg : For an address on its bus + +Required device specific properties (only for SPI chips): +- mcp,spi-present-mask : This is a present flag, that makes only sense for SPI + chips - as the name suggests. Multiple chips can share the same + SPI chipselect. Set bit 0-7 in this mask to 1 if there is a chip + connected with this spi address. If you have a chip with address 3 + connected, you have to set bit3 to 1, which is 0x08. mcp23s08 only + supports bits 0-3. It is not possible to mix mcp23s08 and mcp23s17 + on the same chipselect. Set at least one bit to 1 for SPI chips. +- spi-max-frequency = The maximum frequency this chip is able to handle + +Example I2C: +gpiom1: gpio@20 { + compatible = "mcp,mcp23017"; + gpio-controller; + #gpio-cells = <2>; + reg = <0x20>; +}; + +Example SPI: +gpiom1: gpio@0 { + compatible = "mcp,mcp23s17"; + gpio-controller; + #gpio-cells = <2>; + spi-present-mask = <0x01>; + reg = <0>; + spi-max-frequency = <1000000>; +}; diff --git a/drivers/gpio/gpio-mcp23s08.c b/drivers/gpio/gpio-mcp23s08.c index 3cea0ea..a8ca469 100644 --- a/drivers/gpio/gpio-mcp23s08.c +++ b/drivers/gpio/gpio-mcp23s08.c @@ -12,6 +12,8 @@ #include <linux/spi/mcp23s08.h> #include <linux/slab.h> #include <asm/byteorder.h> +#include <linux/of.h> +#include <linux/of_device.h> /** * MCP types supported by driver @@ -21,6 +23,11 @@ #define MCP_TYPE_008 2 #define MCP_TYPE_017 3 +/** + * Flags used in device tree + */ +#define MCP_DT_FLAG_PULLUP 0x01 + /* Registers are all 8 bits wide. * * The mcp23s17 has twice as many bits, and can be configured to work @@ -75,6 +82,25 @@ struct mcp23s08_driver_data { struct mcp23s08 chip[]; }; +#ifdef CONFIG_OF +static int mcp23s08_of_xlate(struct gpio_chip *gc, + const struct of_phandle_args *gpiospec, u32 *flags); + +static int mcp23s08_set_pullup(struct mcp23s08 *mcp, unsigned offset) +{ + int status; + u16 value; + + mutex_lock(&mcp->lock); + value = mcp->cache[MCP_GPPU] | (1 << offset); + status = mcp->ops->write(mcp, MCP_GPPU, value); + if (!status) + mcp->cache[MCP_GPPU] = value; + mutex_unlock(&mcp->lock); + + return status; +} +#endif /* CONFIG_OF */ /*----------------------------------------------------------------------*/ #if IS_ENABLED(CONFIG_I2C) @@ -383,6 +409,11 @@ static int mcp23s08_probe_one(struct mcp23s08 *mcp, struct device *dev, mcp->chip.direction_output = mcp23s08_direction_output; mcp->chip.set = mcp23s08_set; mcp->chip.dbg_show = mcp23s08_dbg_show; +#ifdef CONFIG_OF + mcp->chip.of_xlate = mcp23s08_of_xlate; + mcp->chip.of_gpio_n_cells = 2; + mcp->chip.of_node = dev->of_node; +#endif switch (type) { #ifdef CONFIG_SPI_MASTER @@ -473,6 +504,50 @@ fail: /*----------------------------------------------------------------------*/ +#ifdef CONFIG_OF +#ifdef CONFIG_SPI_MASTER +static struct of_device_id mcp23s08_spi_of_match[] = { + { + .compatible = "mcp,mcp23s08", .data = (void *) MCP_TYPE_S08, + }, + { + .compatible = "mcp,mcp23s17", .data = (void *) MCP_TYPE_S17, + }, + { }, +}; +MODULE_DEVICE_TABLE(of, mcp23s08_spi_of_match); +#endif + +#if IS_ENABLED(CONFIG_I2C) +static struct of_device_id mcp23s08_i2c_of_match[] = { + { + .compatible = "mcp,mcp23008", .data = (void *) MCP_TYPE_008, + }, + { + .compatible = "mcp,mcp23017", .data = (void *) MCP_TYPE_017, + }, + { }, +}; +MODULE_DEVICE_TABLE(of, mcp23s08_i2c_of_match); +#endif + +static int mcp23s08_of_xlate(struct gpio_chip *gc, + const struct of_phandle_args *gpiospec, u32 *flags) +{ + struct mcp23s08 *mcp = container_of(gc, struct mcp23s08, chip); + + if (flags) + *flags = gpiospec->args[1]; + + if (gpiospec->args[1] & MCP_DT_FLAG_PULLUP) + mcp23s08_set_pullup(mcp, gpiospec->args[0]); + + return gpiospec->args[0]; +} + +#endif /* CONFIG_OF */ + + #if IS_ENABLED(CONFIG_I2C) static int mcp230xx_probe(struct i2c_client *client, @@ -480,12 +555,23 @@ static int mcp230xx_probe(struct i2c_client *client, { struct mcp23s08_platform_data *pdata; struct mcp23s08 *mcp; - int status; - - pdata = client->dev.platform_data; - if (!pdata || !gpio_is_valid(pdata->base)) { - dev_dbg(&client->dev, "invalid or missing platform data\n"); - return -EINVAL; + int status, base, pullups; + const struct of_device_id *match; + + match = of_match_device(of_match_ptr(mcp23s08_i2c_of_match), + &client->dev); + if (match) { + base = -1; + pullups = 0; + } else { + pdata = client->dev.platform_data; + if (!pdata || !gpio_is_valid(pdata->base)) { + dev_dbg(&client->dev, + "invalid or missing platform data\n"); + return -EINVAL; + } + base = pdata->base; + pullups = pdata->chip[0].pullups; } mcp = kzalloc(sizeof *mcp, GFP_KERNEL); @@ -493,8 +579,7 @@ static int mcp230xx_probe(struct i2c_client *client, return -ENOMEM; status = mcp23s08_probe_one(mcp, &client->dev, client, client->addr, - id->driver_data, pdata->base, - pdata->chip[0].pullups); + id->driver_data, base, pullups); if (status) goto fail; @@ -531,6 +616,7 @@ static struct i2c_driver mcp230xx_driver = { .driver = { .name = "mcp230xx", .owner = THIS_MODULE, + .of_match_table = of_match_ptr(mcp23s08_i2c_of_match), }, .probe = mcp230xx_probe, .remove = mcp230xx_remove, @@ -565,28 +651,55 @@ static int mcp23s08_probe(struct spi_device *spi) unsigned chips = 0; struct mcp23s08_driver_data *data; int status, type; - unsigned base; - - type = spi_get_device_id(spi)->driver_data; - - pdata = spi->dev.platform_data; - if (!pdata || !gpio_is_valid(pdata->base)) { - dev_dbg(&spi->dev, "invalid or missing platform data\n"); - return -EINVAL; - } + unsigned base = -1, + ngpio = 0, + pullups[ARRAY_SIZE(pdata->chip)]; + const struct of_device_id *match; + u32 spi_present_mask = 0; + + match = of_match_device(of_match_ptr(mcp23s08_spi_of_match), &spi->dev); + if (match) { + type = (int)match->data; + status = of_property_read_u32(spi->dev.of_node, + "mcp,spi-present-mask", &spi_present_mask); + if (status) { + dev_err(&spi->dev, "DT has no spi-present-mask\n"); + return -ENODEV; + } + if ((spi_present_mask <= 0) || (spi_present_mask >= 256)) { + dev_err(&spi->dev, "invalid spi-present-mask\n"); + return -ENODEV; + } - for (addr = 0; addr < ARRAY_SIZE(pdata->chip); addr++) { - if (!pdata->chip[addr].is_present) - continue; - chips++; - if ((type == MCP_TYPE_S08) && (addr > 3)) { - dev_err(&spi->dev, - "mcp23s08 only supports address 0..3\n"); + for (addr = 0; addr < ARRAY_SIZE(pdata->chip); addr++) + pullups[addr] = 0; + } else { + type = spi_get_device_id(spi)->driver_data; + pdata = spi->dev.platform_data; + if (!pdata || !gpio_is_valid(pdata->base)) { + dev_dbg(&spi->dev, + "invalid or missing platform data\n"); return -EINVAL; } + + for (addr = 0; addr < ARRAY_SIZE(pdata->chip); addr++) { + if (!pdata->chip[addr].is_present) + continue; + chips++; + if ((type == MCP_TYPE_S08) && (addr > 3)) { + dev_err(&spi->dev, + "mcp23s08 only supports address 0..3\n"); + return -EINVAL; + } + spi_present_mask |= 1 << addr; + pullups[addr] = pdata->chip[addr].pullups; + } + + if (!chips) + return -ENODEV; + + base = pdata->base; } - if (!chips) - return -ENODEV; data = kzalloc(sizeof *data + chips * sizeof(struct mcp23s08), GFP_KERNEL); @@ -594,21 +707,22 @@ static int mcp23s08_probe(struct spi_device *spi) return -ENOMEM; spi_set_drvdata(spi, data); - base = pdata->base; for (addr = 0; addr < ARRAY_SIZE(pdata->chip); addr++) { - if (!pdata->chip[addr].is_present) + if (!(spi_present_mask & (1 << addr))) continue; chips--; data->mcp[addr] = &data->chip[chips]; status = mcp23s08_probe_one(data->mcp[addr], &spi->dev, spi, 0x40 | (addr << 1), type, base, - pdata->chip[addr].pullups); + pullups[addr]); if (status < 0) goto fail; - base += (type == MCP_TYPE_S17) ? 16 : 8; + if (base != -1) + base += (type == MCP_TYPE_S17) ? 16 : 8; + ngpio += (type == MCP_TYPE_S17) ? 16 : 8; } - data->ngpio = base - pdata->base; + data->ngpio = ngpio; /* NOTE: these chips have a relatively sane IRQ framework, with * per-signal masking and level/edge triggering. It's not yet @@ -668,6 +782,7 @@ static struct spi_driver mcp23s08_driver = { .driver = { .name = "mcp23s08", .owner = THIS_MODULE, + .of_match_table = of_match_ptr(mcp23s08_spi_of_match), }, };