Skip to contentSkip to conversation

sm501: Add device property

PATCH Superseded Yoshinori Sato 2 participants 7 messages Jun 28, 2016 – Jun 29, 2016

Subsystems (2)
Device tree bindings Multifunction devices (MFD)

v1 of 3 Superseded Latest is v3 →

lore
Share & tools
lore

Yoshinori Sato

author
#0 lore raw

PATCH 1/1 sm501: Add device property

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 <[email protected]>
---
 drivers/mfd/sm501.c | 9 +++++++++
 1 file changed, 9 insertions(+)
drivers/mfd/sm501.c +9 -0
index 65cd0d2..e2e3f9b 100644
@@ -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) {
Signature · 1 line
2.7.0
#1 lore raw
Quoted 4 lines · 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.

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?

Quoted 39 lines
Signed-off-by Yoshinori Sato <[email protected]>
---
 drivers/mfd/sm501.c | 9 +++++++++
 1 file changed, 9 insertions(+)
drivers/mfd/sm501.c +9 -0
index 65cd0d2..e2e3f9b 100644
@@ -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) {
Signature · 4 lines
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
author
#2 lore raw
Quoted 9 lines · Lee Jones
Quoted 4 lines · 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.

This is really ugly.

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

Quoted 1 line

1. Where are you documenting the binding?

It looks remove of commit 4295f9bf74a885da390abc49a3b42a011c1bb890. Revert.

Quoted 1 line

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.

Quoted 2 lines

3. Does this code even work?
Won't private_platdata get freed when you leave probe()?

Yes. This code works RTS7751R2D (SH4).

Quoted 1 line

4. Where is 'devices' consumed?

It used "sm501_init_dev".

I'll sent v2/
Thanks.

Quoted 45 lines
Quoted 39 lines
Signed-off-by Yoshinori Sato <[email protected]>
---
 drivers/mfd/sm501.c | 9 +++++++++
 1 file changed, 9 insertions(+)
drivers/mfd/sm501.c +9 -0
index 65cd0d2..e2e3f9b 100644
@@ -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) {
Signature · 4 lines
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
Signature · 2 lines
Yoshinori Sato
<[email protected]>
author
#3 lore raw

PATCH v2 1/1 sm501: Add device property

In reply to Lee Jones
Signed-off-by Yoshinori Sato <[email protected]>
---
 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
Documentation/devicetree/bindings/mfd/sm501.txt new file +45 -0
new file mode 100644
index 0000000..9290094
@@ -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];
+	};
drivers/mfd/sm501.c +9 -0
index 65cd0d2..e2e3f9b 100644
@@ -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) {
Signature · 1 line
2.7.0
#4 lore raw
Quoted 3 lines · Yoshinori Sato
Signed-off-by Yoshinori Sato <[email protected]>
---
 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?

Quoted 77 lines
 drivers/mfd/sm501.c                             |  9 +++++
 2 files changed, 54 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/mfd/sm501.txt
Documentation/devicetree/bindings/mfd/sm501.txt new file +45 -0
new file mode 100644
index 0000000..9290094
@@ -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];
+	};
drivers/mfd/sm501.c +3 -0
index 65cd0d2..e2e3f9b 100644
@@ -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) {

drivers/mfd/sm501.c static int sm501_plat_probe(struct platform_device *dev) · line 1399

Quoted 9 lines
drivers/mfd/sm501.c +6 -0
@@ -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?

Quoted 2 lines
drivers/mfd/sm501.c +0 -0
 	ret = platform_get_irq(dev, 0);
 	if (ret < 0) {
Signature · 4 lines
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
#5 lore raw

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

Quoted 104 lines · Lee Jones
Quoted 3 lines · Yoshinori Sato
Signed-off-by Yoshinori Sato <[email protected]>
---
 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?

Quoted 77 lines
 drivers/mfd/sm501.c                             |  9 +++++
 2 files changed, 54 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/mfd/sm501.txt
Documentation/devicetree/bindings/mfd/sm501.txt new file +45 -0
new file mode 100644
index 0000000..9290094
@@ -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];
+	};
drivers/mfd/sm501.c +3 -0
index 65cd0d2..e2e3f9b 100644
@@ -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) {

drivers/mfd/sm501.c static int sm501_plat_probe(struct platform_device *dev) · line 1399

Quoted 9 lines
drivers/mfd/sm501.c +6 -0
@@ -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?

Quoted 2 lines
drivers/mfd/sm501.c +0 -0
 	ret = platform_get_irq(dev, 0);
 	if (ret < 0) {
Signature · 4 lines
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
author
#6 lore raw
In reply to Lee Jones
Quoted 10 lines · Lee Jones
Quoted 3 lines · Yoshinori Sato
Signed-off-by Yoshinori Sato <[email protected]>
---
 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.

Quoted 99 lines
Quoted 77 lines
 drivers/mfd/sm501.c                             |  9 +++++
 2 files changed, 54 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/mfd/sm501.txt
Documentation/devicetree/bindings/mfd/sm501.txt new file +45 -0
new file mode 100644
index 0000000..9290094
@@ -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];
+	};
drivers/mfd/sm501.c +3 -0
index 65cd0d2..e2e3f9b 100644
@@ -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) {

drivers/mfd/sm501.c static int sm501_plat_probe(struct platform_device *dev) · line 1399

Quoted 9 lines
drivers/mfd/sm501.c +6 -0
@@ -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?

Quoted 2 lines
drivers/mfd/sm501.c +0 -0
 	ret = platform_get_irq(dev, 0);
 	if (ret < 0) {
Signature · 4 lines
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
Signature · 2 lines
Yoshinori Sato
<[email protected]>