[PATCH] MTD: Onenand: Add device tree support for samsung onenand

[Mark Rutland]

On Mon, Sep 23, 2013 at 09:36:28AM +0100, Mateusz Krawczuk wrote:
> This patch add clk and device tree nodes for samsung onenand driver.
> 
> Signed-off-by: Mateusz Krawczuk <m.krawczuk at partner.samsung.com>
> Signed-off-by: Kyungmin Park <kyungmin.park at samsung.com>
> ---
>  .../devicetree/bindings/mtd/samsung-onenand.txt    | 40 ++++++++++++++++++++++
>  drivers/mtd/onenand/samsung.c                      | 37 +++++++++++++++++++-
>  2 files changed, 76 insertions(+), 1 deletion(-)
>  create mode 100644 Documentation/devicetree/bindings/mtd/samsung-onenand.txt
> 
> diff --git a/Documentation/devicetree/bindings/mtd/samsung-onenand.txt b/Documentation/devicetree/bindings/mtd/samsung-onenand.txt
> new file mode 100644
> index 0000000..cc46160
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mtd/samsung-onenand.txt
> @@ -0,0 +1,40 @@
> +Device tree bindings for Samsung Onenand
> +
> +Required properties:
> +  - compatible: value should be either of the following.
> +      (a) "samsung, s3c6400-onenand", for onenand compatible with s3c6400 onenand.
> +      (b) "samsung, s3c6410-onenand", for onenand compatible with s3c6410 onenand.
> +      (c) "samsung, s5pc100-onenand", for onenand compatible with s5pc100 onenand.
> +      (d) "samsung, s5pc110-onenand", for s5pc100-like onenand used
> +	   on s5pc110 which supports DMA.

These compatible strings shouldn't have spaces.

What are the differences between these, beyond "s5pc110-onenand"
supporting DMA and the others not supporting DMA?

Can we describe these more generically, or do they differ all over the
place in register offsets and so on?

> +
> +Required properties:
> +
> + - reg:					The CS line the peripheral
> +					is connected to

That's not a fantastic description.

As a start, how about something like:

 - reg: the offset and length of the control registers.

> + - interrupt-parent, interrupts		Width of the ONENAND device
> + - 					connected to the Samsung SoC
> +					in bytes. Must be 1 or 2.

Huh? This makes no sense.

Was this was hacked up from the gpmc-onenand binding?

> +
> +Optional properties:
> +
> + - dma-channel:		DMA Channel index

Is this channel internal to the onenand device? If so, why does it need
to be described in this way?

Is this an unnecessary carry-over from the gpmc-onenand binding?

> +
> +For inline partiton table parsing (optional):
> +
> + - #address-cells: should be set to 1
> + - #size-cells: should be set to 1
> +
> +Example for an s5pv210 board:
> +
> +		onenand at b0000000 {
> +			compatible = "samsung,s5pc110-onenand";
> +			reg = <0xb0000000 0x20000>, <0xb0600000 0x2000>;

You only described on reg region above.

Do you expect two, or any arbitrary number of regions?

What does each represent?

> +			interrupt-parent = <&vic1>;
> +			interrupts = <31>;
> +			#address-cells = <1>;
> +			#size-cells = <1>;
> +			clocks = <&clocks NANDXL>;
> +			clock-names = "gate";

The clocks weren't described in the binding. They should be.

> +			status = "okay";
> +		};
> diff --git a/drivers/mtd/onenand/samsung.c b/drivers/mtd/onenand/samsung.c
> index df7400d..5a2cc4b 100644
> --- a/drivers/mtd/onenand/samsung.c
> +++ b/drivers/mtd/onenand/samsung.c
> @@ -14,6 +14,7 @@
>   *	S5PC110: use DMA
>   */
>  
> +#include <linux/clk.h>
>  #include <linux/module.h>
>  #include <linux/platform_device.h>
>  #include <linux/sched.h>
> @@ -24,6 +25,7 @@
>  #include <linux/dma-mapping.h>
>  #include <linux/interrupt.h>
>  #include <linux/io.h>
> +#include <linux/of.h>
>  
>  #include <asm/mach/flash.h>
>  
> @@ -133,6 +135,7 @@ enum soc_type {
>  struct s3c_onenand {
>  	struct mtd_info	*mtd;
>  	struct platform_device	*pdev;
> +	struct clk *gate;
>  	enum soc_type	type;
>  	void __iomem	*base;
>  	struct resource *base_res;
> @@ -859,6 +862,19 @@ static void s3c_onenand_setup(struct mtd_info *mtd)
>  	this->write_bufferram = onenand_write_bufferram;
>  }
>  
> +static const struct of_device_id onenand_s3c_dt_match[] = {
> +	{ .compatible = "samsung,s3c6400-onenand",
> +		.data = (void *)TYPE_S3C6400 },
> +	{ .compatible = "samsung,s3c6410-onenand",
> +		.data = (void *)TYPE_S3C6410 },
> +	{ .compatible = "samsung,s5pc100-onenand",
> +		.data = (void *)TYPE_S5PC100 },
> +	{ .compatible = "samsung,s5pc110-onenand",
> +		.data = (void *)TYPE_S5PC110 },
> +	{},
> +};

What are the differences between these?

Cheers,
Mark.

> +MODULE_DEVICE_TABLE(of, onenand_s3c_dt_match);
> +
>  static int s3c_onenand_probe(struct platform_device *pdev)
>  {
>  	struct onenand_platform_data *pdata;
> @@ -883,12 +899,26 @@ static int s3c_onenand_probe(struct platform_device *pdev)
>  		goto onenand_fail;
>  	}
>  
> +	onenand->gate = clk_get(&pdev->dev, "gate");
> +	if (IS_ERR(onenand->gate))
> +		return PTR_ERR(onenand->gate);
> +	clk_prepare_enable(onenand->gate);
> +
>  	this = (struct onenand_chip *) &mtd[1];
>  	mtd->priv = this;
>  	mtd->dev.parent = &pdev->dev;
>  	mtd->owner = THIS_MODULE;
>  	onenand->pdev = pdev;
> -	onenand->type = platform_get_device_id(pdev)->driver_data;
> +
> +	if (pdev->dev.of_node) {
> +		const struct of_device_id *match;
> +
> +		match = of_match_node(onenand_s3c_dt_match, pdev->dev.of_node);
> +		onenand->type = (enum soc_type)match->data;
> +	} else {
> +		onenand->type = platform_get_device_id(pdev)->driver_data;
> +	}
> +
>  
>  	s3c_onenand_setup(mtd);
>  
> @@ -1077,6 +1107,10 @@ static int s3c_onenand_remove(struct platform_device *pdev)
>  	kfree(onenand->page_buf);
>  	kfree(onenand);
>  	kfree(mtd);
> +
> +	clk_disable_unprepare(onenand->gate);
> +	clk_put(onenand->gate);
> +
>  	return 0;
>  }
>  
> @@ -1125,6 +1159,7 @@ MODULE_DEVICE_TABLE(platform, s3c_onenand_driver_ids);
>  static struct platform_driver s3c_onenand_driver = {
>  	.driver         = {
>  		.name	= "samsung-onenand",
> +		.of_match_table = of_match_ptr(onenand_s3c_dt_match),
>  		.pm	= &s3c_pm_ops,
>  	},
>  	.id_table	= s3c_onenand_driver_ids,
> -- 
> 1.8.1.2
> 
>