On Fri, Jul 29, 2011 at 11:46:31AM +0100, Jamie Iles wrote:
> Add device tree bindings so that the gpio-nand driver may be
> instantiated from the device tree. This also allows the partitions
> to be specified in the device tree.
>
> v2:
> - add CONFIG_OF guards for non-dt platforms
> - compatible becomes gpio-control-nand
> - clarify some binding details
>
> Cc: David Woodhouse <dwmw2 at infradead.org>
> Cc: Artem Bityutskiy <dedekind1 at gmail.com>
> Signed-off-by: Jamie Iles <jamie at jamieiles.com>
Hey Jamie,
Mostly looks good. Comments below. You can add my acked-by when you
address them.
[...]
> +static int gpio_nand_of_get_config(struct device *dev,
> + struct gpio_nand_platdata *plat)
> +{
> + int ret = gpio_nand_of_get_options(dev, plat);
> +
> + if (ret < 0)
> + return ret;
> +
> + gpio_nand_of_get_gpio(dev, plat);
> + gpio_nand_of_get_chip_delay(dev, plat);
Personally, I would just open code it and not bother with the extra
functions, but whatever you prefer.
> +
> + return 0;
> +}
> +#else /* CONFIG_OF */
> +#define gpio_nand_id_table NULL
> +static inline int gpio_nand_of_get_config(struct device *dev,
> + struct gpio_nand_platdata *plat)
> +{
> + return -ENODEV;
> +}
> +#endif /* CONFIG_OF */
> +
> static int __devinit gpio_nand_probe(struct platform_device *dev)
> {
> struct gpiomtd *gpiomtd;
> struct nand_chip *this;
> struct resource *res0, *res1;
> - int ret;
> + int ret = 0;
>
> - if (!dev->dev.platform_data)
> + if (!dev->dev.of_node && !dev->dev.platform_data)
> return -EINVAL;
>
> res0 = platform_get_resource(dev, IORESOURCE_MEM, 0);
> @@ -257,11 +325,17 @@ static int __devinit gpio_nand_probe(struct platform_device *dev)
> }
> }
>
> - memcpy(&gpiomtd->plat, dev->dev.platform_data, sizeof(gpiomtd->plat));
> + if (dev->dev.platform_data)
> + memcpy(&gpiomtd->plat, dev->dev.platform_data,
> + sizeof(gpiomtd->plat));
> + else
> + ret = gpio_nand_of_get_config(&dev->dev, &gpiomtd->plat);
> + if (ret)
> + goto err_config;
>
> ret = gpio_request(gpiomtd->plat.gpio_nce, "NAND NCE");
> if (ret)
> - goto err_nce;
> + goto err_config;
Nit: There's not much purpose it changing the label.
> gpio_direction_output(gpiomtd->plat.gpio_nce, 1);
> if (gpio_is_valid(gpiomtd->plat.gpio_nwp)) {
> ret = gpio_request(gpiomtd->plat.gpio_nwp, "NAND NWP");
> @@ -312,12 +386,21 @@ static int __devinit gpio_nand_probe(struct platform_device *dev)
> goto err_wp;
> }
>
> - if (gpiomtd->plat.adjust_parts)
> - gpiomtd->plat.adjust_parts(&gpiomtd->plat,
> - gpiomtd->mtd_info.size);
> + if (dev->dev.platform_data) {
> + if (gpiomtd->plat.adjust_parts)
> + gpiomtd->plat.adjust_parts(&gpiomtd->plat,
> + gpiomtd->mtd_info.size);
> + } else {
> + ret = of_mtd_parse_partitions(&dev->dev, dev->dev.of_node,
> + &gpiomtd->plat.parts);
> + if (ret < 0)
> + goto err_wp;
>
> + gpiomtd->plat.num_parts = (unsigned int)ret;
Is the casting necessary? I don't think it is.
> + }
> mtd_device_register(&gpiomtd->mtd_info, gpiomtd->plat.parts,
> gpiomtd->plat.num_parts);
> +
Nit: Unrelated whitespace change.
> platform_set_drvdata(dev, gpiomtd);
>
> return 0;
> @@ -335,7 +418,7 @@ err_ale:
> gpio_free(gpiomtd->plat.gpio_nwp);
> err_nwp:
> gpio_free(gpiomtd->plat.gpio_nce);
> -err_nce:
> +err_config:
> iounmap(gpiomtd->io_sync);
> if (res1)
> release_mem_region(res1->start, resource_size(res1));
> @@ -352,6 +435,7 @@ static struct platform_driver gpio_nand_driver = {
> .remove = gpio_nand_remove,
> .driver = {
> .name = "gpio-nand",
> + .of_match_table = gpio_nand_id_table,
> },
> };
>
> --
> 1.7.4.1
>
> _______________________________________________
> devicetree-discuss mailing list
> devicetree-discuss at lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/devicetree-discuss