Re: [PATCH 1/4] [SPI] spi_mpc83xx: convert to the OF platform driver

["Grant Likely" <grant.likely@secretlab.ca>]

On Wed, May 21, 2008 at 9:41 AM, Anton Vorontsov
<avorontsov@ru.mvista.com> wrote:
> This patch converts the driver to speak OF directly. FSL SPI controllers
> do not use internal chip-select machines, so boards must use GPIOs for
> these purposes.
>

Mostly this looks good to me, but I didn't look
> Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>
> ---
>  Documentation/powerpc/booting-without-of.txt |    1 +
>  drivers/spi/Kconfig                          |    3 +-
>  drivers/spi/spi_mpc83xx.c                    |  279 +++++++++++++++++---------
>  3 files changed, 184 insertions(+), 99 deletions(-)
>
> @@ -536,92 +539,152 @@ static void mpc83xx_spi_cleanup(struct spi_device *spi)
>        kfree(spi->controller_state);
>  }
>
> -static int __init mpc83xx_spi_probe(struct platform_device *dev)
> +static unsigned int of_num_children(struct device_node *parent)
> +{
> +       unsigned int count = 0;
> +       struct device_node *child = NULL;
> +
> +       for_each_child_of_node(parent, child)
> +               count++;
> +
> +       return count;
> +}

This smells like it should be in common of code; but I don't think
this routine is needed at all (see below)

[...]
> -       if (master == NULL) {
> +       bus_num = of_get_property(np, "reg", &size);
> +       if (!bus_num || size < sizeof(*bus_num)) {
> +               dev_err(dev, "unable to get reg property from OF\n");
> +               ret = -EINVAL;
> +               goto err_bus_num;
> +       }
> +       master->bus_num = *bus_num;

Does the driver really need to define its own bus num?  I know your
using it for binding with a board info structure, but I think there
are better ways to do it (I'll elaborate in my reply to your patch
that adds support for boardinfo structures).  It's better to let the
SPI infrastructure assign an SPI bus number.  and use other methods to
ensure that extra data is provided to the driver.  Besides, there is
no guarantee that 'reg' will be unique.

> +
> +       master->num_chipselect = of_num_children(np);

This assumes that there are no gaps in the assigned CS numbers of
child nodes, or that the child nodes are an exhaustive list of
attached devices, neither of which may be true.  num_chipselect should
be calculated from the number of GPIOs specified instead.

Cheers,
g.

-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.