Re: [PATCH v3] gpio: mcp23s08: convert driver to DT
From: Linus Walleij <hidden>
Date: 2013-03-22 08:33:17
Also in:
linux-spi, lkml
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 [off-list ref] wrote:
This converts the mcp23s08 driver to be able to be used with device tree.
OK!
quoted hunk ↗ jump to hunk
+++ 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?
quoted hunk ↗ jump to hunk
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?
quoted hunk ↗ jump to hunk
/* 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