[PATCH] sm501: Add device property

[PATCH] sm501: Add device property

[Yoshinori Sato]

This driver have configuration parameter "device" in platform_data.
But don't have it in devicetree.

This patch add "device" configuration to devicetree.

Signed-off-by: Yoshinori Sato <ysato@users.sourceforge.jp>
---
 drivers/mfd/sm501.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/mfd/sm501.c b/drivers/mfd/sm501.c
index 65cd0d2..e2e3f9b 100644
--- a/drivers/mfd/sm501.c
+++ b/drivers/mfd/sm501.c
@@ -21,6 +21,7 @@
 #include <linux/pci.h>
 #include <linux/i2c-gpio.h>
 #include <linux/slab.h>
+#include <linux/of.h>
 
 #include <linux/sm501.h>
 #include <linux/sm501-regs.h>
@@ -1377,6 +1378,8 @@ static int sm501_plat_probe(struct platform_device *dev)
 {
 	struct sm501_devdata *sm;
 	int ret;
+	struct sm501_platdata private_platdata;
+	struct sm501_initdata private_initdata;
 
 	sm = kzalloc(sizeof(struct sm501_devdata), GFP_KERNEL);
 	if (sm == NULL) {
@@ -1388,6 +1391,12 @@ static int sm501_plat_probe(struct platform_device *dev)
 	sm->dev = &dev->dev;
 	sm->pdev_id = dev->id;
 	sm->platdata = dev_get_platdata(&dev->dev);
+	if (!sm->platdata) {
+		of_property_read_u32(dev->dev.of_node, "smi,devices",
+				     (u32 *)&private_initdata.devices);
+		private_platdata.init = &private_initdata;
+		sm->platdata = &private_platdata;
+	}
 
 	ret = platform_get_irq(dev, 0);
 	if (ret < 0) {
-- 
2.7.0

Re: [PATCH] sm501: Add device property

[Lee Jones]

On Tue, 28 Jun 2016, Yoshinori Sato wrote:

> This driver have configuration parameter "device" in platform_data.
> But don't have it in devicetree.
> 
> This patch add "device" configuration to devicetree.

This is really ugly.

1. Where are you documenting the binding?
2. Just because it's in platform data, it doesn't mean it lives in DT
3. Does this code even work?
   Won't private_platdata get freed when you leave probe()?
4. Where is 'devices' consumed?

> Signed-off-by: Yoshinori Sato <ysato@users.sourceforge.jp>
> ---
>  drivers/mfd/sm501.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/drivers/mfd/sm501.c b/drivers/mfd/sm501.c
> index 65cd0d2..e2e3f9b 100644
> --- a/drivers/mfd/sm501.c
> +++ b/drivers/mfd/sm501.c
> @@ -21,6 +21,7 @@
>  #include <linux/pci.h>
>  #include <linux/i2c-gpio.h>
>  #include <linux/slab.h>
> +#include <linux/of.h>
>  
>  #include <linux/sm501.h>
>  #include <linux/sm501-regs.h>
> @@ -1377,6 +1378,8 @@ static int sm501_plat_probe(struct platform_device *dev)
>  {
>  	struct sm501_devdata *sm;
>  	int ret;
> +	struct sm501_platdata private_platdata;
> +	struct sm501_initdata private_initdata;
>  
>  	sm = kzalloc(sizeof(struct sm501_devdata), GFP_KERNEL);
>  	if (sm == NULL) {
> @@ -1388,6 +1391,12 @@ static int sm501_plat_probe(struct platform_device *dev)
>  	sm->dev = &dev->dev;
>  	sm->pdev_id = dev->id;
>  	sm->platdata = dev_get_platdata(&dev->dev);
> +	if (!sm->platdata) {
> +		of_property_read_u32(dev->dev.of_node, "smi,devices",
> +				     (u32 *)&private_initdata.devices);
> +		private_platdata.init = &private_initdata;
> +		sm->platdata = &private_platdata;
> +	}
>  
>  	ret = platform_get_irq(dev, 0);
>  	if (ret < 0) {

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH] sm501: Add device property

[Yoshinori Sato]

On Tue, 28 Jun 2016 16:49:08 +0900,
Lee Jones wrote:
> 
> On Tue, 28 Jun 2016, Yoshinori Sato wrote:
> 
> > This driver have configuration parameter "device" in platform_data.
> > But don't have it in devicetree.
> > 
> > This patch add "device" configuration to devicetree.
> 
> This is really ugly.

I don't know details, either, so it may be wrong.

> 1. Where are you documenting the binding?
It looks remove of commit 4295f9bf74a885da390abc49a3b42a011c1bb890.
Revert.

> 2. Just because it's in platform data, it doesn't mean it lives in DT
SM501's devicetree support only framebuffer module.
This property not needed only framebuffer.

> 3. Does this code even work?
>    Won't private_platdata get freed when you leave probe()?
Yes. This code works RTS7751R2D (SH4).

> 4. Where is 'devices' consumed?
It used "sm501_init_dev".

I'll sent v2/
Thanks.

> > Signed-off-by: Yoshinori Sato <ysato@users.sourceforge.jp>
> > ---
> >  drivers/mfd/sm501.c | 9 +++++++++
> >  1 file changed, 9 insertions(+)
> > 
> > diff --git a/drivers/mfd/sm501.c b/drivers/mfd/sm501.c
> > index 65cd0d2..e2e3f9b 100644
> > --- a/drivers/mfd/sm501.c
> > +++ b/drivers/mfd/sm501.c
> > @@ -21,6 +21,7 @@
> >  #include <linux/pci.h>
> >  #include <linux/i2c-gpio.h>
> >  #include <linux/slab.h>
> > +#include <linux/of.h>
> >  
> >  #include <linux/sm501.h>
> >  #include <linux/sm501-regs.h>
> > @@ -1377,6 +1378,8 @@ static int sm501_plat_probe(struct platform_device *dev)
> >  {
> >  	struct sm501_devdata *sm;
> >  	int ret;
> > +	struct sm501_platdata private_platdata;
> > +	struct sm501_initdata private_initdata;
> >  
> >  	sm = kzalloc(sizeof(struct sm501_devdata), GFP_KERNEL);
> >  	if (sm == NULL) {
> > @@ -1388,6 +1391,12 @@ static int sm501_plat_probe(struct platform_device *dev)
> >  	sm->dev = &dev->dev;
> >  	sm->pdev_id = dev->id;
> >  	sm->platdata = dev_get_platdata(&dev->dev);
> > +	if (!sm->platdata) {
> > +		of_property_read_u32(dev->dev.of_node, "smi,devices",
> > +				     (u32 *)&private_initdata.devices);
> > +		private_platdata.init = &private_initdata;
> > +		sm->platdata = &private_platdata;
> > +	}
> >  
> >  	ret = platform_get_irq(dev, 0);
> >  	if (ret < 0) {
> 
> -- 
> Lee Jones
> Linaro STMicroelectronics Landing Team Lead
> Linaro.org │ Open source software for ARM SoCs
> Follow Linaro: Facebook | Twitter | Blog

-- 
Yoshinori Sato
<ysato@users.sourceforge.jp>

[PATCH v2] sm501: Add device property

[Yoshinori Sato]

Signed-off-by: Yoshinori Sato <ysato@users.sourceforge.jp>
---
 Documentation/devicetree/bindings/mfd/sm501.txt | 45 +++++++++++++++++++++++++
 drivers/mfd/sm501.c                             |  9 +++++
 2 files changed, 54 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/mfd/sm501.txt

diff --git a/Documentation/devicetree/bindings/mfd/sm501.txt b/Documentation/devicetree/bindings/mfd/sm501.txt
new file mode 100644
index 0000000..9290094
--- /dev/null
+++ b/Documentation/devicetree/bindings/mfd/sm501.txt
@@ -0,0 +1,45 @@
+* SM SM501
+
+The SM SM501 is a LCD controller, with proper hardware, it can also
+drive DVI monitors.
+
+Required properties:
+- compatible : should be "smi,sm501".
+- reg : contain two entries:
+    - First entry: System Configuration register
+    - Second entry: IO space (Display Controller register)
+- interrupts : SMI interrupt to the cpu should be described here.
+- interrupt-parent : the phandle for the interrupt controller that
+  services interrupts for this device.
+
+Optional properties:
+- mode : select a video mode:
+    <xres>x<yres>[-<bpp>][@<refresh>]
+- edid : verbatim EDID data block describing attached display.
+  Data from the detailed timing descriptor will be used to
+  program the display controller.
+- little-endian: available on big endian systems, to
+  set different foreign endian.
+- big-endian: available on little endian systems, to
+  set different foreign endian.
+- smi,devices: function block enable bitmap.
+	bit0: USB host
+	bit1: USB slave
+	bit2: SSP Ch0
+	bit3: SSP Ch1
+	bit4: UART Ch0
+	bit5: UART Ch1
+	bit6: Accelerator
+	bit7: AC97
+	bit8: I2S
+	bit9: GPIO
+
+Example for MPC5200:
+	display@1,0 {
+		compatible = "smi,sm501";
+		reg = <1 0x00000000 0x00800000
+		       1 0x03e00000 0x00200000>;
+		interrupts = <1 1 3>;
+		mode = "640x480-32@60";
+		edid = [edid-data];
+	};
diff --git a/drivers/mfd/sm501.c b/drivers/mfd/sm501.c
index 65cd0d2..e2e3f9b 100644
--- a/drivers/mfd/sm501.c
+++ b/drivers/mfd/sm501.c
@@ -21,6 +21,7 @@
 #include <linux/pci.h>
 #include <linux/i2c-gpio.h>
 #include <linux/slab.h>
+#include <linux/of.h>
 
 #include <linux/sm501.h>
 #include <linux/sm501-regs.h>
@@ -1377,6 +1378,8 @@ static int sm501_plat_probe(struct platform_device *dev)
 {
 	struct sm501_devdata *sm;
 	int ret;
+	struct sm501_platdata private_platdata;
+	struct sm501_initdata private_initdata;
 
 	sm = kzalloc(sizeof(struct sm501_devdata), GFP_KERNEL);
 	if (sm == NULL) {
@@ -1388,6 +1391,12 @@ static int sm501_plat_probe(struct platform_device *dev)
 	sm->dev = &dev->dev;
 	sm->pdev_id = dev->id;
 	sm->platdata = dev_get_platdata(&dev->dev);
+	if (!sm->platdata) {
+		of_property_read_u32(dev->dev.of_node, "smi,devices",
+				     (u32 *)&private_initdata.devices);
+		private_platdata.init = &private_initdata;
+		sm->platdata = &private_platdata;
+	}
 
 	ret = platform_get_irq(dev, 0);
 	if (ret < 0) {
-- 
2.7.0

Re: [PATCH v2] sm501: Add device property

[Lee Jones]

On Tue, 28 Jun 2016, Yoshinori Sato wrote:

> Signed-off-by: Yoshinori Sato <ysato@users.sourceforge.jp>
> ---
>  Documentation/devicetree/bindings/mfd/sm501.txt | 45 +++++++++++++++++++++++++

This needs a DT Ack, but you haven't sent it to the DT list.

Did you run ./scripts/get_maintainer.pl?

>  drivers/mfd/sm501.c                             |  9 +++++
>  2 files changed, 54 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/mfd/sm501.txt
> 
> diff --git a/Documentation/devicetree/bindings/mfd/sm501.txt b/Documentation/devicetree/bindings/mfd/sm501.txt
> new file mode 100644
> index 0000000..9290094
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mfd/sm501.txt
> @@ -0,0 +1,45 @@
> +* SM SM501
> +
> +The SM SM501 is a LCD controller, with proper hardware, it can also
> +drive DVI monitors.
> +
> +Required properties:
> +- compatible : should be "smi,sm501".
> +- reg : contain two entries:
> +    - First entry: System Configuration register
> +    - Second entry: IO space (Display Controller register)
> +- interrupts : SMI interrupt to the cpu should be described here.
> +- interrupt-parent : the phandle for the interrupt controller that
> +  services interrupts for this device.
> +
> +Optional properties:
> +- mode : select a video mode:
> +    <xres>x<yres>[-<bpp>][@<refresh>]
> +- edid : verbatim EDID data block describing attached display.
> +  Data from the detailed timing descriptor will be used to
> +  program the display controller.
> +- little-endian: available on big endian systems, to
> +  set different foreign endian.
> +- big-endian: available on little endian systems, to
> +  set different foreign endian.
> +- smi,devices: function block enable bitmap.
> +	bit0: USB host
> +	bit1: USB slave
> +	bit2: SSP Ch0
> +	bit3: SSP Ch1
> +	bit4: UART Ch0
> +	bit5: UART Ch1
> +	bit6: Accelerator
> +	bit7: AC97
> +	bit8: I2S
> +	bit9: GPIO
> +
> +Example for MPC5200:
> +	display@1,0 {
> +		compatible = "smi,sm501";
> +		reg = <1 0x00000000 0x00800000
> +		       1 0x03e00000 0x00200000>;
> +		interrupts = <1 1 3>;
> +		mode = "640x480-32@60";
> +		edid = [edid-data];
> +	};
> diff --git a/drivers/mfd/sm501.c b/drivers/mfd/sm501.c
> index 65cd0d2..e2e3f9b 100644
> --- a/drivers/mfd/sm501.c
> +++ b/drivers/mfd/sm501.c
> @@ -21,6 +21,7 @@
>  #include <linux/pci.h>
>  #include <linux/i2c-gpio.h>
>  #include <linux/slab.h>
> +#include <linux/of.h>
>  
>  #include <linux/sm501.h>
>  #include <linux/sm501-regs.h>
> @@ -1377,6 +1378,8 @@ static int sm501_plat_probe(struct platform_device *dev)
>  {
>  	struct sm501_devdata *sm;
>  	int ret;
> +	struct sm501_platdata private_platdata;
> +	struct sm501_initdata private_initdata;
>  
>  	sm = kzalloc(sizeof(struct sm501_devdata), GFP_KERNEL);
>  	if (sm == NULL) {
> @@ -1388,6 +1391,12 @@ static int sm501_plat_probe(struct platform_device *dev)
>  	sm->dev = &dev->dev;
>  	sm->pdev_id = dev->id;
>  	sm->platdata = dev_get_platdata(&dev->dev);
> +	if (!sm->platdata) {
> +		of_property_read_u32(dev->dev.of_node, "smi,devices",
> +				     (u32 *)&private_initdata.devices);
> +		private_platdata.init = &private_initdata;
> +		sm->platdata = &private_platdata;
> +	}

This is still really horrible.

Where is this platform data consumed?

>  	ret = platform_get_irq(dev, 0);
>  	if (ret < 0) {

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH v2] sm501: Add device property

[Lee Jones]

Oh, and the subject line needs to reflect the subsystem you are
submitting to.

On Wed, 29 Jun 2016, Lee Jones wrote:
> On Tue, 28 Jun 2016, Yoshinori Sato wrote:
> 
> > Signed-off-by: Yoshinori Sato <ysato@users.sourceforge.jp>
> > ---
> >  Documentation/devicetree/bindings/mfd/sm501.txt | 45 +++++++++++++++++++++++++
> 
> This needs a DT Ack, but you haven't sent it to the DT list.
> 
> Did you run ./scripts/get_maintainer.pl?
> 
> >  drivers/mfd/sm501.c                             |  9 +++++
> >  2 files changed, 54 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/mfd/sm501.txt
> > 
> > diff --git a/Documentation/devicetree/bindings/mfd/sm501.txt b/Documentation/devicetree/bindings/mfd/sm501.txt
> > new file mode 100644
> > index 0000000..9290094
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/mfd/sm501.txt
> > @@ -0,0 +1,45 @@
> > +* SM SM501
> > +
> > +The SM SM501 is a LCD controller, with proper hardware, it can also
> > +drive DVI monitors.
> > +
> > +Required properties:
> > +- compatible : should be "smi,sm501".
> > +- reg : contain two entries:
> > +    - First entry: System Configuration register
> > +    - Second entry: IO space (Display Controller register)
> > +- interrupts : SMI interrupt to the cpu should be described here.
> > +- interrupt-parent : the phandle for the interrupt controller that
> > +  services interrupts for this device.
> > +
> > +Optional properties:
> > +- mode : select a video mode:
> > +    <xres>x<yres>[-<bpp>][@<refresh>]
> > +- edid : verbatim EDID data block describing attached display.
> > +  Data from the detailed timing descriptor will be used to
> > +  program the display controller.
> > +- little-endian: available on big endian systems, to
> > +  set different foreign endian.
> > +- big-endian: available on little endian systems, to
> > +  set different foreign endian.
> > +- smi,devices: function block enable bitmap.
> > +	bit0: USB host
> > +	bit1: USB slave
> > +	bit2: SSP Ch0
> > +	bit3: SSP Ch1
> > +	bit4: UART Ch0
> > +	bit5: UART Ch1
> > +	bit6: Accelerator
> > +	bit7: AC97
> > +	bit8: I2S
> > +	bit9: GPIO
> > +
> > +Example for MPC5200:
> > +	display@1,0 {
> > +		compatible = "smi,sm501";
> > +		reg = <1 0x00000000 0x00800000
> > +		       1 0x03e00000 0x00200000>;
> > +		interrupts = <1 1 3>;
> > +		mode = "640x480-32@60";
> > +		edid = [edid-data];
> > +	};
> > diff --git a/drivers/mfd/sm501.c b/drivers/mfd/sm501.c
> > index 65cd0d2..e2e3f9b 100644
> > --- a/drivers/mfd/sm501.c
> > +++ b/drivers/mfd/sm501.c
> > @@ -21,6 +21,7 @@
> >  #include <linux/pci.h>
> >  #include <linux/i2c-gpio.h>
> >  #include <linux/slab.h>
> > +#include <linux/of.h>
> >  
> >  #include <linux/sm501.h>
> >  #include <linux/sm501-regs.h>
> > @@ -1377,6 +1378,8 @@ static int sm501_plat_probe(struct platform_device *dev)
> >  {
> >  	struct sm501_devdata *sm;
> >  	int ret;
> > +	struct sm501_platdata private_platdata;
> > +	struct sm501_initdata private_initdata;
> >  
> >  	sm = kzalloc(sizeof(struct sm501_devdata), GFP_KERNEL);
> >  	if (sm == NULL) {
> > @@ -1388,6 +1391,12 @@ static int sm501_plat_probe(struct platform_device *dev)
> >  	sm->dev = &dev->dev;
> >  	sm->pdev_id = dev->id;
> >  	sm->platdata = dev_get_platdata(&dev->dev);
> > +	if (!sm->platdata) {
> > +		of_property_read_u32(dev->dev.of_node, "smi,devices",
> > +				     (u32 *)&private_initdata.devices);
> > +		private_platdata.init = &private_initdata;
> > +		sm->platdata = &private_platdata;
> > +	}
> 
> This is still really horrible.
> 
> Where is this platform data consumed?
> 
> >  	ret = platform_get_irq(dev, 0);
> >  	if (ret < 0) {
> 

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH v2] sm501: Add device property

[Yoshinori Sato]

On Wed, 29 Jun 2016 17:16:33 +0900,
Lee Jones wrote:
> 
> On Tue, 28 Jun 2016, Yoshinori Sato wrote:
> 
> > Signed-off-by: Yoshinori Sato <ysato@users.sourceforge.jp>
> > ---
> >  Documentation/devicetree/bindings/mfd/sm501.txt | 45 +++++++++++++++++++++++++
> 
> This needs a DT Ack, but you haven't sent it to the DT list.
> 
> Did you run ./scripts/get_maintainer.pl?

OK. I'll resent it.
Thanks.

> >  drivers/mfd/sm501.c                             |  9 +++++
> >  2 files changed, 54 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/mfd/sm501.txt
> > 
> > diff --git a/Documentation/devicetree/bindings/mfd/sm501.txt b/Documentation/devicetree/bindings/mfd/sm501.txt
> > new file mode 100644
> > index 0000000..9290094
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/mfd/sm501.txt
> > @@ -0,0 +1,45 @@
> > +* SM SM501
> > +
> > +The SM SM501 is a LCD controller, with proper hardware, it can also
> > +drive DVI monitors.
> > +
> > +Required properties:
> > +- compatible : should be "smi,sm501".
> > +- reg : contain two entries:
> > +    - First entry: System Configuration register
> > +    - Second entry: IO space (Display Controller register)
> > +- interrupts : SMI interrupt to the cpu should be described here.
> > +- interrupt-parent : the phandle for the interrupt controller that
> > +  services interrupts for this device.
> > +
> > +Optional properties:
> > +- mode : select a video mode:
> > +    <xres>x<yres>[-<bpp>][@<refresh>]
> > +- edid : verbatim EDID data block describing attached display.
> > +  Data from the detailed timing descriptor will be used to
> > +  program the display controller.
> > +- little-endian: available on big endian systems, to
> > +  set different foreign endian.
> > +- big-endian: available on little endian systems, to
> > +  set different foreign endian.
> > +- smi,devices: function block enable bitmap.
> > +	bit0: USB host
> > +	bit1: USB slave
> > +	bit2: SSP Ch0
> > +	bit3: SSP Ch1
> > +	bit4: UART Ch0
> > +	bit5: UART Ch1
> > +	bit6: Accelerator
> > +	bit7: AC97
> > +	bit8: I2S
> > +	bit9: GPIO
> > +
> > +Example for MPC5200:
> > +	display@1,0 {
> > +		compatible = "smi,sm501";
> > +		reg = <1 0x00000000 0x00800000
> > +		       1 0x03e00000 0x00200000>;
> > +		interrupts = <1 1 3>;
> > +		mode = "640x480-32@60";
> > +		edid = [edid-data];
> > +	};
> > diff --git a/drivers/mfd/sm501.c b/drivers/mfd/sm501.c
> > index 65cd0d2..e2e3f9b 100644
> > --- a/drivers/mfd/sm501.c
> > +++ b/drivers/mfd/sm501.c
> > @@ -21,6 +21,7 @@
> >  #include <linux/pci.h>
> >  #include <linux/i2c-gpio.h>
> >  #include <linux/slab.h>
> > +#include <linux/of.h>
> >  
> >  #include <linux/sm501.h>
> >  #include <linux/sm501-regs.h>
> > @@ -1377,6 +1378,8 @@ static int sm501_plat_probe(struct platform_device *dev)
> >  {
> >  	struct sm501_devdata *sm;
> >  	int ret;
> > +	struct sm501_platdata private_platdata;
> > +	struct sm501_initdata private_initdata;
> >  
> >  	sm = kzalloc(sizeof(struct sm501_devdata), GFP_KERNEL);
> >  	if (sm == NULL) {
> > @@ -1388,6 +1391,12 @@ static int sm501_plat_probe(struct platform_device *dev)
> >  	sm->dev = &dev->dev;
> >  	sm->pdev_id = dev->id;
> >  	sm->platdata = dev_get_platdata(&dev->dev);
> > +	if (!sm->platdata) {
> > +		of_property_read_u32(dev->dev.of_node, "smi,devices",
> > +				     (u32 *)&private_initdata.devices);
> > +		private_platdata.init = &private_initdata;
> > +		sm->platdata = &private_platdata;
> > +	}
> 
> This is still really horrible.
> 
> Where is this platform data consumed?
> 
> >  	ret = platform_get_irq(dev, 0);
> >  	if (ret < 0) {
> 
> -- 
> Lee Jones
> Linaro STMicroelectronics Landing Team Lead
> Linaro.org │ Open source software for ARM SoCs
> Follow Linaro: Facebook | Twitter | Blog

-- 
Yoshinori Sato
<ysato@users.sourceforge.jp>