Skip to contentSkip to conversation

mfd: sm501: Add device property

PATCH RESEND Superseded Yoshinori Sato 3 participants 6 messages Jun 29, 2016 – Jul 3, 2016

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

v2 of 3 Superseded Latest is v3 →

lore
Share & tools
lore

Yoshinori Sato

author
#0 lore raw

PATCH RESEND v2 1/1 mfd: sm501: Add device property

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 · 6 lines
2.7.0

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
#1 lore raw
Quoted 80 lines · Yoshinori Sato
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 +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;
+	}

I've asked about this 3 times now.

What consumes this platform data?

It also looks ugly and fragile.

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
#2 lore raw
Quoted 95 lines · Lee Jones
Quoted 80 lines · Yoshinori Sato
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 +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;
+	}

I've asked about this 3 times now.

Quoted 3 lines

What consumes this platform data?

It also looks ugly and fragile.

It's appropriate to use dev.of_node, isn't it?
If it's misunderstood, I'm sorry.

Quoted 8 lines
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]>
#3 lore raw
Quoted 11 lines · Yoshinori Sato
Quoted 9 lines · Lee Jones
Quoted 6 lines · Yoshinori Sato
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

[...]

Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
drivers/mfd/sm501.c +0 -0
index 65cd0d2..e2e3f9b 100644

[...]

Quoted 22 lines
Quoted 22 lines
Quoted 10 lines
drivers/mfd/sm501.c +2 -0
@@ -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;
+	}

I've asked about this 3 times now.

Quoted 6 lines
Quoted 3 lines

What consumes this platform data?

It also looks ugly and fragile.

It's appropriate to use dev.of_node, isn't it?
If it's misunderstood, I'm sorry.

Yes, that's acceptable.

I'm talking about the whole process of allocating an entire platform structure (on the stack, which will most likely be wiped when we return from probe()) just to populate this one, undocumented property.

Hold up ... I've just taken a look at the driver myself. What a mess. It appears this driver pre-dates the MFD API and does everything I hate.

First step is to see if the Device Tree guys like your new property. Please document it in bindings/display/sm501fb.txt, as suggested by Rob, so they can review it. Don't forget to CC me too.

We can look at the C code changes later.

Signature · 4 lines
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
author
#4 lore raw
Quoted 64 lines · Lee Jones
Quoted 11 lines · Yoshinori Sato
Quoted 9 lines · Lee Jones
Quoted 6 lines · Yoshinori Sato
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

[...]

Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
drivers/mfd/sm501.c +0 -0
index 65cd0d2..e2e3f9b 100644

[...]

Quoted 22 lines
Quoted 22 lines
Quoted 10 lines
drivers/mfd/sm501.c +2 -0
@@ -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;
+	}

I've asked about this 3 times now.

Quoted 6 lines
Quoted 3 lines

What consumes this platform data?

It also looks ugly and fragile.

It's appropriate to use dev.of_node, isn't it?
If it's misunderstood, I'm sorry.

Yes, that's acceptable.

I'm talking about the whole process of allocating an entire platform structure (on the stack, which will most likely be wiped when we return from probe()) just to populate this one, undocumented property.

Hold up ... I've just taken a look at the driver myself. What a mess. It appears this driver pre-dates the MFD API and does everything I hate.

Quoted 5 lines

First step is to see if the Device Tree guys like your new property. Please document it in bindings/display/sm501fb.txt, as suggested by Rob, so they can review it. Don't forget to CC me too.

We can look at the C code changes later.

OK.
Thanks comment.

Quoted 5 lines
Signature · 4 lines
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
Signature · 6 lines
Yoshinori Sato
<[email protected]>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
maintainer
#5 lore raw
In reply to Yoshinori Sato
Quoted 13 lines · Yoshinori Sato
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 +0 -0
new file mode 100644
index 0000000..9290094

Documentation/devicetree/bindings/mfd/sm501.txt line 4

Quoted 4 lines
Documentation/devicetree/bindings/mfd/sm501.txt new file +4 -0
@@ -0,0 +1,45 @@
+* SM SM501
+
+The SM SM501 is a LCD controller, with proper hardware, it can also
+drive DVI monitors.

There is already a binding doc in bindings/display/

Quoted 82 lines
Documentation/devicetree/bindings/mfd/sm501.txt new file +41 -0
+
+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 · 6 lines
2.7.0

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html