Re: [PATCH v3] gpio: mcp23s08: convert driver to DT

[Linus Walleij]

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@xxxxxxxxxxxxxxxxx> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/