[3/4,v4] video, sm501: add OF binding to support SM501

- add binding to OF, compatible name "smi,sm501"

Signed-off-by: Heiko Schocher <hs@denx.de>
cc: linux-fbdev@vger.kernel.org
cc: devicetree-discuss@ozlabs.org
cc: Ben Dooks <ben@simtec.co.uk>
cc: Vincent Sanders <vince@simtec.co.uk>
cc: Samuel Ortiz <sameo@linux.intel.com>
cc: linux-kernel@vger.kernel.org
cc: Randy Dunlap <rdunlap@xenotime.net>
cc: Paul Mundt <lethal@linux-sh.org>

---
- changes since v1:
  add Ben Dooks, Vincent Sanders and Samuel Ortiz to cc, as suggested from
  Paul Mundt.
- changes since v2:
  add comments from Randy Dunlap:
  - move parameter documentation to Documentation/fb/sm501.txt
- changes since v3:
  - rebased against v2.6.38-rc2
  - split in 3 patches
    - of support patch
      - get rid of "#if defined(CONFIG_PPC_MPC52xx)" usage
        hide this in DTS, as Paul suggested.
    - i/o routine patch
    - edid support patch
- changes since v4
  replace remaining CONFIG_PPC_MPC52xx with CONFIG_OF, as
  it is no longer MPC52xx only.

./scripts/checkpatch.pl 0003-video-sm501-add-OF-binding-to-support-SM501.patch
total: 0 errors, 0 warnings, 117 lines checked

0003-video-sm501-add-OF-binding-to-support-SM501.patch has no obvious style problems and is ready for submission.

 Documentation/powerpc/dts-bindings/sm501.txt |   34 ++++++++++++++++++++++++++
 drivers/mfd/sm501.c                          |   16 +++++++++++-
 drivers/video/sm501fb.c                      |   33 ++++++++++++++++++++++++-
 3 files changed, 81 insertions(+), 2 deletions(-)
 create mode 100644 Documentation/powerpc/dts-bindings/sm501.txt

On Tue, Jan 25, 2011 at 08:20:31AM +0100, Heiko Schocher wrote:
> @@ -1934,7 +1943,29 @@ static int __devinit sm501fb_probe(struct platform_device *pdev)
>  	}
>  
>  	if (info->pdata == NULL) {
> -		dev_info(dev, "using default configuration data\n");
> +		int found = 0;
> +#if defined(CONFIG_OF)
> +		struct device_node *np = pdev->dev.parent->of_node;
> +		const u8 *prop;
> +		const char *cp;
> +		int len;
> +
> +		info->pdata = &sm501fb_def_pdata;
> +		if (np) {
> +			/* Get EDID */
> +			cp = of_get_property(np, "mode", &len);
> +			if (cp)
> +				strcpy(fb_mode, cp);
> +			prop = of_get_property(np, "edid", &len);
> +			if (prop && len == EDID_LENGTH) {
> +				info->edid_data = kmemdup(prop, EDID_LENGTH,
> +							  GFP_KERNEL);
> +				found = 1;
> +			}
> +		}
> +#endif
> +		if (!found)
> +			dev_info(dev, "using default configuration data\n");
>  	}
>  
>  	/* probe for the presence of each panel */

Starting to get a bit pedantic.. but kmemdup() tries to do a kmalloc(),
and so can fail. Your other patches handle the info->edid_data == NULL
case, in addition to the kfree(), but you're probably going to want to
chomp that found assignment incase of the allocation failing and falling
back on the default mode.

You also don't really have any need to keep the EDID block around after
probe as far as I can tell, so you should be able to rework this in to
something more like:

	info->edid_data = kmemdup(..);
	...
	if (info->edid_data) {
		fb_edid_to_monspecs(..);
		kfree(info->edid_data);
		fb_videomode_to_modelist(..);
	}

	...

Hello Paul,

Paul Mundt wrote:
> On Tue, Jan 25, 2011 at 08:20:31AM +0100, Heiko Schocher wrote:
>> @@ -1934,7 +1943,29 @@ static int __devinit sm501fb_probe(struct platform_device *pdev)
>>  	}
>>  
>>  	if (info->pdata == NULL) {
>> -		dev_info(dev, "using default configuration data\n");
>> +		int found = 0;
>> +#if defined(CONFIG_OF)
>> +		struct device_node *np = pdev->dev.parent->of_node;
>> +		const u8 *prop;
>> +		const char *cp;
>> +		int len;
>> +
>> +		info->pdata = &sm501fb_def_pdata;
>> +		if (np) {
>> +			/* Get EDID */
>> +			cp = of_get_property(np, "mode", &len);
>> +			if (cp)
>> +				strcpy(fb_mode, cp);
>> +			prop = of_get_property(np, "edid", &len);
>> +			if (prop && len == EDID_LENGTH) {
>> +				info->edid_data = kmemdup(prop, EDID_LENGTH,
>> +							  GFP_KERNEL);
>> +				found = 1;
>> +			}
>> +		}
>> +#endif
>> +		if (!found)
>> +			dev_info(dev, "using default configuration data\n");
>>  	}
>>  
>>  	/* probe for the presence of each panel */
> 
> Starting to get a bit pedantic.. but kmemdup() tries to do a kmalloc(),

No problem!

> and so can fail. Your other patches handle the info->edid_data == NULL
> case, in addition to the kfree(), but you're probably going to want to
> chomp that found assignment incase of the allocation failing and falling
> back on the default mode.
> 
> You also don't really have any need to keep the EDID block around after
> probe as far as I can tell, so you should be able to rework this in to
> something more like:
> 
> 	info->edid_data = kmemdup(..);
> 	...
> 	if (info->edid_data) {
> 		fb_edid_to_monspecs(..);
> 		kfree(info->edid_data);
> 		fb_videomode_to_modelist(..);
> 	}
> 
> 	...

Ok, rework this part, thanks!

bye,
Heiko