Re: Patch "REMOVE OMAP LEGACY CODE: Reset mach-omap1/board-*.c files to mainline" breaks nokia770

[Tony Lindgren <tony@atomide.com>]

* Andrew de Quincey <adq_dvb@lidskialf.net> [090514 16:37]:
> Quoting Tony Lindgren <tony@atomide.com>:
>
>> * Andrew de Quincey <adq_dvb@lidskialf.net> [090514 16:19]:
>>> Quoting Tony Lindgren <tony@atomide.com>:
>>>
>>>> * Felipe Balbi <felipe.balbi@nokia.com> [090514 01:15]:
>>>>> On Thu, May 14, 2009 at 03:44:14AM +0200, ext Tony Lindgren wrote:
>>>>> > * Felipe Balbi <felipe.balbi@nokia.com> [090513 17:33]:
>>>>> > > On Thu, May 14, 2009 at 01:46:51AM +0200, ext Andrew de Quincey wrote:
>>>>> > > > Hi, I've just discovered that the patch at:
>>>>> > > >
>>>>> > > >
>>>>> http://git.kernel.org/?p=linux/kernel/git/tmlind/linux-omap-2.6.git;a=commitdiff;h=3eae3ea7c443fc4330574dffea65b6f2f53a2574
>>>>> > > >
>>>>> > > > Breaks the nokia770's framebuffer as it removes the  
>>>>> platform data for
>>>>> > > > the HWA742 LCD controller.
>>>>> > > >
>>>>> > > > As the patch says "Patches against the mainline tree are welcome to
>>>>> > > > add back the missing functionality if needed!", I'm happy  
>>>>> to do this.
>>>>> > > >
>>>>> > > > However, since I'm fairly new to the linux-omap project, is simply
>>>>> > > > extracting the removed nokia770 code and generating a patch against
>>>>> > > > the mainline kernel sufficient? or is there a newer style  
>>>>> of some sort
>>>>> > > > that should be adopted for this?
>>>>> > >
>>>>> > > You can start by generating the new patch against mainline and running
>>>>> > > scripts/checkpatch.pl, then you should probably find out if there are
>>>>> > > any API changes and stuff like that. Then send the patch to lkml ccing
>>>>> > > linux-omap and let's see what comments do you get from those guys :-)
>>>>> >
>>>>> > Please change the code to pass the struct clock to
>>>>> drivers/video/omap/hwa742.c
>>>>> > in hwa742_platform_data. The hw742.c can just do standard
>>>>> clk_enable/disable
>>>>> > there. Otherwise we won't be able to get this missing part to  
>>>>> the mainline
>>>>> > kernel.
>>>>>
>>>>> how about clkdev then ? are we gonna support that for omap1 still ?
>>>>
>>>> Yeah the clkdev is there, but in the case of the LCD the clock can be
>>>> any clock, so we cannot set just one "fck" for all the omap1 LCD panels.
>>>>
>>>> In this case the "bclk" is used for the LCD, but "bclk" could be used
>>>> for other devices as well, not just LCD.
>>>
>>> Hi, is the attached patch the sort of thing you had in mind? This is
>>> against linux-omap-2.6, latest git.
>>
>> Hey, that's pretty cool! Just one comment below.
>>
>>> Unfortunately, both mainline linux-2.6 and linux-omap-2.6 gits lock up
>>> on bootup before the LCD is initialised. However, the linux-omap 2.6.29
>>> that is compiled by openembedded still boots ok, and with the attached
>>> patch applied, the LCD works again!
>>
>> OK, good to hear. Need to debug that then.
>>
>>> I guess my next task is to figure out why the mainline kernels freeze;
>>> I've seen that before, and it seemed to be something to do with changes
>>> to the mcbsp support.
>>
>> OK
>>
>>> Reinstate HWA742 platform data for nokia 770 platform.
>>>
>>> Signed-off-by: Andrew de Quincey <adq_dvb@lidskialf.net>
>>>
>>> diff --git a/arch/arm/mach-omap1/board-nokia770.c  
>>> b/arch/arm/mach-omap1/board-nokia770.c
>>> index 8780ca6..df5f38b 100644
>>> --- a/arch/arm/mach-omap1/board-nokia770.c
>>> +++ b/arch/arm/mach-omap1/board-nokia770.c
>>> @@ -33,6 +33,7 @@
>>>  #include <mach/common.h>
>>>  #include <mach/dsp_common.h>
>>>  #include <mach/omapfb.h>
>>> +#include <mach/hwa742.h>
>>>  #include <mach/lcd_mipid.h>
>>>  #include <mach/mmc.h>
>>>  #include <mach/usb.h>
>>> @@ -163,6 +164,26 @@ static struct spi_board_info  
>>> nokia770_spi_board_info[] __initdata = {
>>>  	},
>>>  };
>>>
>>> +static struct hwa742_platform_data nokia770_hwa742_platform_data = {
>>> +	.sys_ck 		= NULL,
>>> +	.te_connected		= 1,
>>> +};
>>> +
>>> +static int hwa742_get_clocks(void)
>>> +{
>>> +	nokia770_hwa742_platform_data.sys_ck = clk_get(NULL, "bclk");
>>> +	if (IS_ERR(nokia770_hwa742_platform_data.sys_ck)) {
>>> +		printk(KERN_ERR "can't get HWA742 clock\n");
>>> +		return PTR_ERR(nokia770_hwa742_platform_data.sys_ck);
>>> +	}
>>> +	return 0;
>>> +}
>>> +
>>> +static void hwa742_dev_init(void)
>>> +{
>>> +	hwa742_get_clocks();
>>> +	omapfb_set_ctrl_platform_data(&nokia770_hwa742_platform_data);
>>> +}
>>
>> You can now get rid of the hwa742_get_clocks(), and move that code
>> to hwa742_dev_init() instead.
>
> [snip]
>
> Done! v2 attached.

Looks good to me.

Acked-by: Tony Lindgren <tony@atomide.com>

Imre, want to update your git branch for fbdev list with this one?

Regards,

Tony


> Reinstate HWA742 platform data for nokia 770 platform.
> 
> Signed-off-by: Andrew de Quincey <adq_dvb@lidskialf.net>
> 
> diff --git a/arch/arm/mach-omap1/board-nokia770.c b/arch/arm/mach-omap1/board-nokia770.c
> index 8780ca6..2c4785e 100644
> --- a/arch/arm/mach-omap1/board-nokia770.c
> +++ b/arch/arm/mach-omap1/board-nokia770.c
> @@ -33,6 +33,7 @@
>  #include <mach/common.h>
>  #include <mach/dsp_common.h>
>  #include <mach/omapfb.h>
> +#include <mach/hwa742.h>
>  #include <mach/lcd_mipid.h>
>  #include <mach/mmc.h>
>  #include <mach/usb.h>
> @@ -163,6 +164,20 @@ static struct spi_board_info nokia770_spi_board_info[] __initdata = {
>  	},
>  };
>  
> +static struct hwa742_platform_data nokia770_hwa742_platform_data = {
> +	.sys_ck 		= NULL,
> +	.te_connected		= 1,
> +};
> +
> +static void hwa742_dev_init(void)
> +{
> +	nokia770_hwa742_platform_data.sys_ck = clk_get(NULL, "bclk");
> +	if (IS_ERR(nokia770_hwa742_platform_data.sys_ck)) {
> +		printk(KERN_ERR "can't get HWA742 clock\n");
> +	} else {
> +		omapfb_set_ctrl_platform_data(&nokia770_hwa742_platform_data);
> +	}
> +}
>  
>  /* assume no Mini-AB port */
>  
> @@ -371,6 +386,7 @@ static void __init omap_nokia770_init(void)
>  	omap_serial_init();
>  	omap_register_i2c_bus(1, 100, NULL, 0);
>  	omap_dsp_init();
> +	hwa742_dev_init();
>  	ads7846_dev_init();
>  	mipid_dev_init();
>  	omap_usb_init(&nokia770_usb_config);
> diff --git a/arch/arm/plat-omap/include/mach/hwa742.h b/arch/arm/plat-omap/include/mach/hwa742.h
> index 577f492..c00e05d 100644
> --- a/arch/arm/plat-omap/include/mach/hwa742.h
> +++ b/arch/arm/plat-omap/include/mach/hwa742.h
> @@ -2,10 +2,7 @@
>  #define _HWA742_H
>  
>  struct hwa742_platform_data {
> -	void		(*power_up)(struct device *dev);
> -	void		(*power_down)(struct device *dev);
> -	unsigned long	(*get_clock_rate)(struct device *dev);
> -
> +	struct clk 	*sys_ck;
>  	unsigned	te_connected:1;
>  };
>  
> diff --git a/drivers/video/omap/hwa742.c b/drivers/video/omap/hwa742.c
> index 8aa6e47..1230476 100644
> --- a/drivers/video/omap/hwa742.c
> +++ b/drivers/video/omap/hwa742.c
> @@ -133,8 +133,7 @@ struct {
>  	struct lcd_ctrl_extif	*extif;
>  	struct lcd_ctrl		*int_ctrl;
>  
> -	void			(*power_up)(struct device *dev);
> -	void			(*power_down)(struct device *dev);
> +	struct clk		*sys_ck;
>  } hwa742;
>  
>  struct lcd_ctrl hwa742_ctrl;
> @@ -915,14 +914,13 @@ static void hwa742_suspend(void)
>  	hwa742_set_update_mode(OMAPFB_UPDATE_DISABLED);
>  	/* Enable sleep mode */
>  	hwa742_write_reg(HWA742_POWER_SAVE, 1 << 1);
> -	if (hwa742.power_down != NULL)
> -		hwa742.power_down(hwa742.fbdev->dev);
> +	clk_disable(hwa742.sys_ck);
>  }
>  
>  static void hwa742_resume(void)
>  {
> -	if (hwa742.power_up != NULL)
> -		hwa742.power_up(hwa742.fbdev->dev);
> +	clk_enable(hwa742.sys_ck);
> +
>  	/* Disable sleep mode */
>  	hwa742_write_reg(HWA742_POWER_SAVE, 0);
>  	while (1) {
> @@ -955,14 +953,13 @@ static int hwa742_init(struct omapfb_device *fbdev, int ext_mode,
>  	omapfb_conf = fbdev->dev->platform_data;
>  	ctrl_conf = omapfb_conf->ctrl_platform_data;
>  
> -	if (ctrl_conf == NULL || ctrl_conf->get_clock_rate == NULL) {
> +	if (ctrl_conf == NULL || ctrl_conf->sys_ck == NULL) {
>  		dev_err(fbdev->dev, "HWA742: missing platform data\n");
>  		r = -ENOENT;
>  		goto err1;
>  	}
>  
> -	hwa742.power_down = ctrl_conf->power_down;
> -	hwa742.power_up = ctrl_conf->power_up;
> +	hwa742.sys_ck = ctrl_conf->sys_ck;
>  
>  	spin_lock_init(&hwa742.req_lock);
>  
> @@ -972,12 +969,11 @@ static int hwa742_init(struct omapfb_device *fbdev, int ext_mode,
>  	if ((r = hwa742.extif->init(fbdev)) < 0)
>  		goto err2;
>  
> -	ext_clk = ctrl_conf->get_clock_rate(fbdev->dev);
> +	ext_clk = clk_get_rate(hwa742.sys_ck);
>  	if ((r = calc_extif_timings(ext_clk, &extif_mem_div)) < 0)
>  		goto err3;
>  	hwa742.extif->set_timings(&hwa742.reg_timings);
> -	if (hwa742.power_up != NULL)
> -		hwa742.power_up(fbdev->dev);
> +	clk_enable(hwa742.sys_ck);
>  
>  	calc_hwa742_clk_rates(ext_clk, &sys_clk, &pix_clk);
>  	if ((r = calc_extif_timings(sys_clk, &extif_mem_div)) < 0)
> @@ -1040,8 +1036,7 @@ static int hwa742_init(struct omapfb_device *fbdev, int ext_mode,
>  
>  	return 0;
>  err4:
> -	if (hwa742.power_down != NULL)
> -		hwa742.power_down(fbdev->dev);
> +	clk_disable(hwa742.sys_ck);
>  err3:
>  	hwa742.extif->cleanup();
>  err2:
> @@ -1055,8 +1050,7 @@ static void hwa742_cleanup(void)
>  	hwa742_set_update_mode(OMAPFB_UPDATE_DISABLED);
>  	hwa742.extif->cleanup();
>  	hwa742.int_ctrl->cleanup();
> -	if (hwa742.power_down != NULL)
> -		hwa742.power_down(hwa742.fbdev->dev);
> +	clk_disable(hwa742.sys_ck);
>  }
>  
>  struct lcd_ctrl hwa742_ctrl = {