Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1433912 > unrolled thread

[PATCH RESEND v2] mfd: sm501: Add device property

Back to article view | Back to linux.kernel

Contents

  [PATCH RESEND v2] mfd: sm501: Add device property Yoshinori Sato <ysato@users.sourceforge.jp> - 2016-06-29 19:00 +0200
    Re: [PATCH RESEND v2] mfd: sm501: Add device property Lee Jones <lee.jones@linaro.org> - 2016-06-30 10:00 +0200
      Re: [PATCH RESEND v2] mfd: sm501: Add device property Yoshinori Sato <ysato@users.sourceforge.jp> - 2016-06-30 18:00 +0200
        Re: [PATCH RESEND v2] mfd: sm501: Add device property Lee Jones <lee.jones@linaro.org> - 2016-07-01 11:00 +0200
          Re: [PATCH RESEND v2] mfd: sm501: Add device property Yoshinori Sato <ysato@users.sourceforge.jp> - 2016-07-03 13:40 +0200
    Re: [PATCH RESEND v2] mfd: sm501: Add device property Rob Herring <robh@kernel.org> - 2016-07-01 04:20 +0200

#1433912 — [PATCH RESEND v2] mfd: sm501: Add device property

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

[toc] | [next] | [standalone]

#1434319

On Thu, 30 Jun 2016, Yoshinori Sato wrote:

> 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;
> +	}

I've asked about this 3 times now.

What consumes this platform data?

It also looks ugly and fragile.

>  	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

[toc] | [prev] | [next] | [standalone]

#1434684

On Thu, 30 Jun 2016 16:48:00 +0900,
Lee Jones wrote:
> 
> On Thu, 30 Jun 2016, Yoshinori Sato wrote:
> 
> > 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;
> > +	}
> 
> I've asked about this 3 times now.
> 
> 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.

> >  	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>

[toc] | [prev] | [next] | [standalone]

#1435169

On Fri, 01 Jul 2016, Yoshinori Sato wrote:

> On Thu, 30 Jun 2016 16:48:00 +0900,
> Lee Jones wrote:
> > 
> > On Thu, 30 Jun 2016, Yoshinori Sato wrote:
> > 
> > > 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/drivers/mfd/sm501.c b/drivers/mfd/sm501.c
> > > index 65cd0d2..e2e3f9b 100644
> > > --- a/drivers/mfd/sm501.c
> > > +++ b/drivers/mfd/sm501.c

[...]

> > > @@ -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;
> > > +	}
> > 
> > I've asked about this 3 times now.
> > 
> > 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.

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

[toc] | [prev] | [next] | [standalone]

#1435946

On Fri, 01 Jul 2016 17:59:11 +0900,
Lee Jones wrote:
> 
> On Fri, 01 Jul 2016, Yoshinori Sato wrote:
> 
> > On Thu, 30 Jun 2016 16:48:00 +0900,
> > Lee Jones wrote:
> > > 
> > > On Thu, 30 Jun 2016, Yoshinori Sato wrote:
> > > 
> > > > 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/drivers/mfd/sm501.c b/drivers/mfd/sm501.c
> > > > index 65cd0d2..e2e3f9b 100644
> > > > --- a/drivers/mfd/sm501.c
> > > > +++ b/drivers/mfd/sm501.c
> 
> [...]
> 
> > > > @@ -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;
> > > > +	}
> > > 
> > > I've asked about this 3 times now.
> > > 
> > > 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.

OK.
Thanks comment.

> -- 
> 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>

[toc] | [prev] | [next] | [standalone]

#1434965

On Thu, Jun 30, 2016 at 01:56:59AM +0900, Yoshinori Sato wrote:
> 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.

There is already a binding doc in bindings/display/

> +
> +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
> 
> --
> To unsubscribe from this list: send the line "unsubscribe devicetree" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web