Patchwork [v3] i.MX27: Fix emma-prp clocks in mx2_camera.c

login
register
mail settings
Submitter Javier Martin
Date July 6, 2012, 10:56 a.m.
Message ID <1341572162-29126-1-git-send-email-javier.martin@vista-silicon.com>
Download mbox | patch
Permalink /patch/169412/
State New
Headers show

Comments

Javier Martin - July 6, 2012, 10:56 a.m.
This driver wasn't converted to the new clock changes
(clk_prepare_enable/clk_disable_unprepare). Also naming
of emma-prp related clocks for the i.MX27 was not correct.
---
Enable clocks only for i.MX27.

---
 arch/arm/mach-imx/clk-imx27.c    |    8 +++--
 drivers/media/video/mx2_camera.c |   67 ++++++++++++++++++++++----------------
 2 files changed, 44 insertions(+), 31 deletions(-)
Sascha Hauer - July 9, 2012, 7:28 a.m.
On Fri, Jul 06, 2012 at 12:56:02PM +0200, Javier Martin wrote:
> This driver wasn't converted to the new clock changes
> (clk_prepare_enable/clk_disable_unprepare). Also naming
> of emma-prp related clocks for the i.MX27 was not correct.
> ---
> Enable clocks only for i.MX27.
> 

Indeed,

>  
> -	pcdev->clk_csi = clk_get(&pdev->dev, NULL);
> -	if (IS_ERR(pcdev->clk_csi)) {
> -		dev_err(&pdev->dev, "Could not get csi clock\n");
> -		err = PTR_ERR(pcdev->clk_csi);
> -		goto exit_kfree;
> +	if (cpu_is_mx27()) {
> +		pcdev->clk_csi = devm_clk_get(&pdev->dev, "ahb");
> +		if (IS_ERR(pcdev->clk_csi)) {
> +			dev_err(&pdev->dev, "Could not get csi clock\n");
> +			err = PTR_ERR(pcdev->clk_csi);
> +			goto exit_kfree;
> +		}

but why? Now the i.MX25 won't get a clock anymore.

Sascha
Javier Martin - July 9, 2012, 7:37 a.m.
On 9 July 2012 09:28, Sascha Hauer <s.hauer@pengutronix.de> wrote:
> On Fri, Jul 06, 2012 at 12:56:02PM +0200, Javier Martin wrote:
>> This driver wasn't converted to the new clock changes
>> (clk_prepare_enable/clk_disable_unprepare). Also naming
>> of emma-prp related clocks for the i.MX27 was not correct.
>> ---
>> Enable clocks only for i.MX27.
>>
>
> Indeed,
>
>>
>> -     pcdev->clk_csi = clk_get(&pdev->dev, NULL);
>> -     if (IS_ERR(pcdev->clk_csi)) {
>> -             dev_err(&pdev->dev, "Could not get csi clock\n");
>> -             err = PTR_ERR(pcdev->clk_csi);
>> -             goto exit_kfree;
>> +     if (cpu_is_mx27()) {
>> +             pcdev->clk_csi = devm_clk_get(&pdev->dev, "ahb");
>> +             if (IS_ERR(pcdev->clk_csi)) {
>> +                     dev_err(&pdev->dev, "Could not get csi clock\n");
>> +                     err = PTR_ERR(pcdev->clk_csi);
>> +                     goto exit_kfree;
>> +             }
>
> but why? Now the i.MX25 won't get a clock anymore.

What are the clocks needed by i.MX25? csi only?

By the way, is anybody using this driver with i.MX25?
Sascha Hauer - July 9, 2012, 7:43 a.m.
On Mon, Jul 09, 2012 at 09:37:25AM +0200, javier Martin wrote:
> On 9 July 2012 09:28, Sascha Hauer <s.hauer@pengutronix.de> wrote:
> > On Fri, Jul 06, 2012 at 12:56:02PM +0200, Javier Martin wrote:
> >> This driver wasn't converted to the new clock changes
> >> (clk_prepare_enable/clk_disable_unprepare). Also naming
> >> of emma-prp related clocks for the i.MX27 was not correct.
> >> ---
> >> Enable clocks only for i.MX27.
> >>
> >
> > Indeed,
> >
> >>
> >> -     pcdev->clk_csi = clk_get(&pdev->dev, NULL);
> >> -     if (IS_ERR(pcdev->clk_csi)) {
> >> -             dev_err(&pdev->dev, "Could not get csi clock\n");
> >> -             err = PTR_ERR(pcdev->clk_csi);
> >> -             goto exit_kfree;
> >> +     if (cpu_is_mx27()) {
> >> +             pcdev->clk_csi = devm_clk_get(&pdev->dev, "ahb");
> >> +             if (IS_ERR(pcdev->clk_csi)) {
> >> +                     dev_err(&pdev->dev, "Could not get csi clock\n");
> >> +                     err = PTR_ERR(pcdev->clk_csi);
> >> +                     goto exit_kfree;
> >> +             }
> >
> > but why? Now the i.MX25 won't get a clock anymore.
> 
> What are the clocks needed by i.MX25? csi only?
> 
> By the way, is anybody using this driver with i.MX25?

It seems Baruch (added to Cc) has used it on an i.MX25.

Sascha
Javier Martin - July 9, 2012, 7:46 a.m.
On 9 July 2012 09:43, Sascha Hauer <s.hauer@pengutronix.de> wrote:
> On Mon, Jul 09, 2012 at 09:37:25AM +0200, javier Martin wrote:
>> On 9 July 2012 09:28, Sascha Hauer <s.hauer@pengutronix.de> wrote:
>> > On Fri, Jul 06, 2012 at 12:56:02PM +0200, Javier Martin wrote:
>> >> This driver wasn't converted to the new clock changes
>> >> (clk_prepare_enable/clk_disable_unprepare). Also naming
>> >> of emma-prp related clocks for the i.MX27 was not correct.
>> >> ---
>> >> Enable clocks only for i.MX27.
>> >>
>> >
>> > Indeed,
>> >
>> >>
>> >> -     pcdev->clk_csi = clk_get(&pdev->dev, NULL);
>> >> -     if (IS_ERR(pcdev->clk_csi)) {
>> >> -             dev_err(&pdev->dev, "Could not get csi clock\n");
>> >> -             err = PTR_ERR(pcdev->clk_csi);
>> >> -             goto exit_kfree;
>> >> +     if (cpu_is_mx27()) {
>> >> +             pcdev->clk_csi = devm_clk_get(&pdev->dev, "ahb");
>> >> +             if (IS_ERR(pcdev->clk_csi)) {
>> >> +                     dev_err(&pdev->dev, "Could not get csi clock\n");
>> >> +                     err = PTR_ERR(pcdev->clk_csi);
>> >> +                     goto exit_kfree;
>> >> +             }
>> >
>> > but why? Now the i.MX25 won't get a clock anymore.
>>
>> What are the clocks needed by i.MX25? csi only?
>>
>> By the way, is anybody using this driver with i.MX25?
>
> It seems Baruch (added to Cc) has used it on an i.MX25.

Baruch,
could you tell us what are the clocks needed by i.MX25?

Regards.
Baruch Siach - July 9, 2012, 7:52 a.m.
Hi Javier,

On Mon, Jul 09, 2012 at 09:46:03AM +0200, javier Martin wrote:
> On 9 July 2012 09:43, Sascha Hauer <s.hauer@pengutronix.de> wrote:
> > On Mon, Jul 09, 2012 at 09:37:25AM +0200, javier Martin wrote:
> >> On 9 July 2012 09:28, Sascha Hauer <s.hauer@pengutronix.de> wrote:
> >> > On Fri, Jul 06, 2012 at 12:56:02PM +0200, Javier Martin wrote:
> >> >> This driver wasn't converted to the new clock changes
> >> >> (clk_prepare_enable/clk_disable_unprepare). Also naming
> >> >> of emma-prp related clocks for the i.MX27 was not correct.
> >> >> ---
> >> >> Enable clocks only for i.MX27.
> >> >
> >> > Indeed,
> >> >
> >> >> -     pcdev->clk_csi = clk_get(&pdev->dev, NULL);
> >> >> -     if (IS_ERR(pcdev->clk_csi)) {
> >> >> -             dev_err(&pdev->dev, "Could not get csi clock\n");
> >> >> -             err = PTR_ERR(pcdev->clk_csi);
> >> >> -             goto exit_kfree;
> >> >> +     if (cpu_is_mx27()) {
> >> >> +             pcdev->clk_csi = devm_clk_get(&pdev->dev, "ahb");
> >> >> +             if (IS_ERR(pcdev->clk_csi)) {
> >> >> +                     dev_err(&pdev->dev, "Could not get csi clock\n");
> >> >> +                     err = PTR_ERR(pcdev->clk_csi);
> >> >> +                     goto exit_kfree;
> >> >> +             }
> >> >
> >> > but why? Now the i.MX25 won't get a clock anymore.
> >>
> >> What are the clocks needed by i.MX25? csi only?
> >>
> >> By the way, is anybody using this driver with i.MX25?
> >
> > It seems Baruch (added to Cc) has used it on an i.MX25.
> could you tell us what are the clocks needed by i.MX25?

According to the code the csi clock is sufficient for i.MX25. Unfortunately I 
don't have access to a running system anymore, so I can't test current code.

baruch
Sascha Hauer - July 9, 2012, 8:07 a.m.
On Mon, Jul 09, 2012 at 09:46:03AM +0200, javier Martin wrote:
> On 9 July 2012 09:43, Sascha Hauer <s.hauer@pengutronix.de> wrote:
> > On Mon, Jul 09, 2012 at 09:37:25AM +0200, javier Martin wrote:
> >> On 9 July 2012 09:28, Sascha Hauer <s.hauer@pengutronix.de> wrote:
> >> > On Fri, Jul 06, 2012 at 12:56:02PM +0200, Javier Martin wrote:
> >> >> This driver wasn't converted to the new clock changes
> >> >> (clk_prepare_enable/clk_disable_unprepare). Also naming
> >> >> of emma-prp related clocks for the i.MX27 was not correct.
> >> >> ---
> >> >> Enable clocks only for i.MX27.
> >> >>
> >> >
> >> > Indeed,
> >> >
> >> >>
> >> >> -     pcdev->clk_csi = clk_get(&pdev->dev, NULL);
> >> >> -     if (IS_ERR(pcdev->clk_csi)) {
> >> >> -             dev_err(&pdev->dev, "Could not get csi clock\n");
> >> >> -             err = PTR_ERR(pcdev->clk_csi);
> >> >> -             goto exit_kfree;
> >> >> +     if (cpu_is_mx27()) {
> >> >> +             pcdev->clk_csi = devm_clk_get(&pdev->dev, "ahb");
> >> >> +             if (IS_ERR(pcdev->clk_csi)) {
> >> >> +                     dev_err(&pdev->dev, "Could not get csi clock\n");
> >> >> +                     err = PTR_ERR(pcdev->clk_csi);
> >> >> +                     goto exit_kfree;
> >> >> +             }
> >> >
> >> > but why? Now the i.MX25 won't get a clock anymore.
> >>
> >> What are the clocks needed by i.MX25? csi only?
> >>
> >> By the way, is anybody using this driver with i.MX25?
> >
> > It seems Baruch (added to Cc) has used it on an i.MX25.
> 
> Baruch,
> could you tell us what are the clocks needed by i.MX25?

I just had a look and the i.MX25 it needs three clocks: ipg, ahb and
peripheral clock. So this is broken anyway and should probably be fixed
seperately, that is:

- provide dummy clocks for the csi clocks on i.MX27
- clk_get ipg, ahb and peripheral clocks on all SoCs
- clk_get emma clocks on i.MX27 only

As said, this is a separate topic, so your original patch should be fine
for now.

Sascha
Javier Martin - July 9, 2012, 8:22 a.m.
On 9 July 2012 10:07, Sascha Hauer <s.hauer@pengutronix.de> wrote:
> On Mon, Jul 09, 2012 at 09:46:03AM +0200, javier Martin wrote:
>> On 9 July 2012 09:43, Sascha Hauer <s.hauer@pengutronix.de> wrote:
>> > On Mon, Jul 09, 2012 at 09:37:25AM +0200, javier Martin wrote:
>> >> On 9 July 2012 09:28, Sascha Hauer <s.hauer@pengutronix.de> wrote:
>> >> > On Fri, Jul 06, 2012 at 12:56:02PM +0200, Javier Martin wrote:
>> >> >> This driver wasn't converted to the new clock changes
>> >> >> (clk_prepare_enable/clk_disable_unprepare). Also naming
>> >> >> of emma-prp related clocks for the i.MX27 was not correct.
>> >> >> ---
>> >> >> Enable clocks only for i.MX27.
>> >> >>
>> >> >
>> >> > Indeed,
>> >> >
>> >> >>
>> >> >> -     pcdev->clk_csi = clk_get(&pdev->dev, NULL);
>> >> >> -     if (IS_ERR(pcdev->clk_csi)) {
>> >> >> -             dev_err(&pdev->dev, "Could not get csi clock\n");
>> >> >> -             err = PTR_ERR(pcdev->clk_csi);
>> >> >> -             goto exit_kfree;
>> >> >> +     if (cpu_is_mx27()) {
>> >> >> +             pcdev->clk_csi = devm_clk_get(&pdev->dev, "ahb");
>> >> >> +             if (IS_ERR(pcdev->clk_csi)) {
>> >> >> +                     dev_err(&pdev->dev, "Could not get csi clock\n");
>> >> >> +                     err = PTR_ERR(pcdev->clk_csi);
>> >> >> +                     goto exit_kfree;
>> >> >> +             }
>> >> >
>> >> > but why? Now the i.MX25 won't get a clock anymore.
>> >>
>> >> What are the clocks needed by i.MX25? csi only?
>> >>
>> >> By the way, is anybody using this driver with i.MX25?
>> >
>> > It seems Baruch (added to Cc) has used it on an i.MX25.
>>
>> Baruch,
>> could you tell us what are the clocks needed by i.MX25?
>
> I just had a look and the i.MX25 it needs three clocks: ipg, ahb and
> peripheral clock. So this is broken anyway and should probably be fixed
> seperately, that is:
>
> - provide dummy clocks for the csi clocks on i.MX27
> - clk_get ipg, ahb and peripheral clocks on all SoCs
> - clk_get emma clocks on i.MX27 only
>
> As said, this is a separate topic, so your original patch should be fine
> for now.

OK, thanks for your interest.

Regards.
Guennadi Liakhovetski - July 20, 2012, 11:19 a.m.
Hi Javier

Thanks for the patch

On Mon, 9 Jul 2012, javier Martin wrote:

> On 9 July 2012 10:07, Sascha Hauer <s.hauer@pengutronix.de> wrote:
> > On Mon, Jul 09, 2012 at 09:46:03AM +0200, javier Martin wrote:
> >> On 9 July 2012 09:43, Sascha Hauer <s.hauer@pengutronix.de> wrote:
> >> > On Mon, Jul 09, 2012 at 09:37:25AM +0200, javier Martin wrote:
> >> >> On 9 July 2012 09:28, Sascha Hauer <s.hauer@pengutronix.de> wrote:
> >> >> > On Fri, Jul 06, 2012 at 12:56:02PM +0200, Javier Martin wrote:
> >> >> >> This driver wasn't converted to the new clock changes
> >> >> >> (clk_prepare_enable/clk_disable_unprepare). Also naming
> >> >> >> of emma-prp related clocks for the i.MX27 was not correct.
> >> >> >> ---
> >> >> >> Enable clocks only for i.MX27.
> >> >> >>
> >> >> >
> >> >> > Indeed,
> >> >> >
> >> >> >>
> >> >> >> -     pcdev->clk_csi = clk_get(&pdev->dev, NULL);
> >> >> >> -     if (IS_ERR(pcdev->clk_csi)) {
> >> >> >> -             dev_err(&pdev->dev, "Could not get csi clock\n");
> >> >> >> -             err = PTR_ERR(pcdev->clk_csi);
> >> >> >> -             goto exit_kfree;
> >> >> >> +     if (cpu_is_mx27()) {
> >> >> >> +             pcdev->clk_csi = devm_clk_get(&pdev->dev, "ahb");
> >> >> >> +             if (IS_ERR(pcdev->clk_csi)) {
> >> >> >> +                     dev_err(&pdev->dev, "Could not get csi clock\n");
> >> >> >> +                     err = PTR_ERR(pcdev->clk_csi);
> >> >> >> +                     goto exit_kfree;
> >> >> >> +             }
> >> >> >
> >> >> > but why? Now the i.MX25 won't get a clock anymore.
> >> >>
> >> >> What are the clocks needed by i.MX25? csi only?
> >> >>
> >> >> By the way, is anybody using this driver with i.MX25?
> >> >
> >> > It seems Baruch (added to Cc) has used it on an i.MX25.
> >>
> >> Baruch,
> >> could you tell us what are the clocks needed by i.MX25?
> >
> > I just had a look and the i.MX25 it needs three clocks: ipg, ahb and
> > peripheral clock. So this is broken anyway and should probably be fixed
> > seperately, that is:
> >
> > - provide dummy clocks for the csi clocks on i.MX27
> > - clk_get ipg, ahb and peripheral clocks on all SoCs
> > - clk_get emma clocks on i.MX27 only
> >
> > As said, this is a separate topic, so your original patch should be fine
> > for now.

Well, sorry, but I don't think I can share this.

1. it touches two areas - arch/ and drivers/ which isnÄt a good thing and 
   should be avoided wherever possible
2. it addresses several problems: (a) missing name for "ahb" camera clock, 
   (b) wrong device and connection names for emma clocks, (c) missing 
   _(un)prepare suffixes in clock API
3. it makes a possibly broken i.MX25 even more broken

IIUC, mx2-camera is broken on i.MX27 in current next because of wrong 
clock entries, right? So, we don't have to be bothered not to break 
bisection - it is already broken. Then we can clean up the problems 
separately under arch/ and drivers/.

So, would it be possible to split this into 3 parts:

(a) arch - fix clocks
(b) media - fix clocks on i.MX27 _without_ breaking it even further on 
    i.MX25. If we think i.MX25 support is already broken, let's schedule 
    its removal and remove properly, or add BROKEN to Kconfig, when built 
    on i.MX25. In your patch this would mean just adding an "else" to your 
    "if (cpu_is_mx27())" statement and moving the current clk_get() there
(c) add _(un)prepare.

Since these are fixes, I won't wait too long for these. If you don't 
resubmit them today, they'll go in after 3.6-rc1.

Thanks
Guennadi

> OK, thanks for your interest.
> 
> Regards.
> -- 
> Javier Martin
> Vista Silicon S.L.
> CDTUC - FASE C - Oficina S-345
> Avda de los Castros s/n
> 39005- Santander. Cantabria. Spain
> +34 942 25 32 60
> www.vista-silicon.com
> 

---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/
Javier Martin - July 20, 2012, 1:04 p.m.
On 20 July 2012 13:19, Guennadi Liakhovetski <g.liakhovetski@gmx.de> wrote:
> Hi Javier
>
> Thanks for the patch
>
> On Mon, 9 Jul 2012, javier Martin wrote:
>
>> On 9 July 2012 10:07, Sascha Hauer <s.hauer@pengutronix.de> wrote:
>> > On Mon, Jul 09, 2012 at 09:46:03AM +0200, javier Martin wrote:
>> >> On 9 July 2012 09:43, Sascha Hauer <s.hauer@pengutronix.de> wrote:
>> >> > On Mon, Jul 09, 2012 at 09:37:25AM +0200, javier Martin wrote:
>> >> >> On 9 July 2012 09:28, Sascha Hauer <s.hauer@pengutronix.de> wrote:
>> >> >> > On Fri, Jul 06, 2012 at 12:56:02PM +0200, Javier Martin wrote:
>> >> >> >> This driver wasn't converted to the new clock changes
>> >> >> >> (clk_prepare_enable/clk_disable_unprepare). Also naming
>> >> >> >> of emma-prp related clocks for the i.MX27 was not correct.
>> >> >> >> ---
>> >> >> >> Enable clocks only for i.MX27.
>> >> >> >>
>> >> >> >
>> >> >> > Indeed,
>> >> >> >
>> >> >> >>
>> >> >> >> -     pcdev->clk_csi = clk_get(&pdev->dev, NULL);
>> >> >> >> -     if (IS_ERR(pcdev->clk_csi)) {
>> >> >> >> -             dev_err(&pdev->dev, "Could not get csi clock\n");
>> >> >> >> -             err = PTR_ERR(pcdev->clk_csi);
>> >> >> >> -             goto exit_kfree;
>> >> >> >> +     if (cpu_is_mx27()) {
>> >> >> >> +             pcdev->clk_csi = devm_clk_get(&pdev->dev, "ahb");
>> >> >> >> +             if (IS_ERR(pcdev->clk_csi)) {
>> >> >> >> +                     dev_err(&pdev->dev, "Could not get csi clock\n");
>> >> >> >> +                     err = PTR_ERR(pcdev->clk_csi);
>> >> >> >> +                     goto exit_kfree;
>> >> >> >> +             }
>> >> >> >
>> >> >> > but why? Now the i.MX25 won't get a clock anymore.
>> >> >>
>> >> >> What are the clocks needed by i.MX25? csi only?
>> >> >>
>> >> >> By the way, is anybody using this driver with i.MX25?
>> >> >
>> >> > It seems Baruch (added to Cc) has used it on an i.MX25.
>> >>
>> >> Baruch,
>> >> could you tell us what are the clocks needed by i.MX25?
>> >
>> > I just had a look and the i.MX25 it needs three clocks: ipg, ahb and
>> > peripheral clock. So this is broken anyway and should probably be fixed
>> > seperately, that is:
>> >
>> > - provide dummy clocks for the csi clocks on i.MX27
>> > - clk_get ipg, ahb and peripheral clocks on all SoCs
>> > - clk_get emma clocks on i.MX27 only
>> >
>> > As said, this is a separate topic, so your original patch should be fine
>> > for now.
>
> Well, sorry, but I don't think I can share this.
>
> 1. it touches two areas - arch/ and drivers/ which isnÄt a good thing and
>    should be avoided wherever possible
> 2. it addresses several problems: (a) missing name for "ahb" camera clock,
>    (b) wrong device and connection names for emma clocks, (c) missing
>    _(un)prepare suffixes in clock API
> 3. it makes a possibly broken i.MX25 even more broken
>
> IIUC, mx2-camera is broken on i.MX27 in current next because of wrong
> clock entries, right? So, we don't have to be bothered not to break
> bisection - it is already broken. Then we can clean up the problems
> separately under arch/ and drivers/.
>
> So, would it be possible to split this into 3 parts:
>
> (a) arch - fix clocks
> (b) media - fix clocks on i.MX27 _without_ breaking it even further on
>     i.MX25. If we think i.MX25 support is already broken, let's schedule
>     its removal and remove properly, or add BROKEN to Kconfig, when built
>     on i.MX25. In your patch this would mean just adding an "else" to your
>     "if (cpu_is_mx27())" statement and moving the current clk_get() there
> (c) add _(un)prepare.
>
> Since these are fixes, I won't wait too long for these. If you don't
> resubmit them today, they'll go in after 3.6-rc1.
>
> Thanks
> Guennadi
>
>> OK, thanks for your interest.
>>
>> Regards.
>> --
>> Javier Martin
>> Vista Silicon S.L.
>> CDTUC - FASE C - Oficina S-345
>> Avda de los Castros s/n
>> 39005- Santander. Cantabria. Spain
>> +34 942 25 32 60
>> www.vista-silicon.com
>>
>

Hi Guennadi,
thanks for your review.

I could agree with some of your reasons for not accepting this patch.
Specially (1) and (2).
As regards of reason (3), i.MX25 has been broken for months and I
refuse that a broken chip that no known board in the mainline uses is
constantly slowing the development process.

Furthermore, I sent v3 of this patch 11 days ago. I know you are a
busy developer but, honestly, it doesn't seem fair that, having sent
the patch with plenty of time I am now given only this little time to
fix it. I'm out of the office right now and I won't come back until
monday. Couldn't you either make an exception and accept the path or
give me until monday in the morning to send the new version?

Alternatively, Sascha, Fabio or any other i.MX27 user could send it
too if they have the time.

Otherwise, as you stated it'll have to wait to 3.6-rc1. So 3.5 will be
broken for i.MX27.

Regards.
Guennadi Liakhovetski - July 20, 2012, 1:43 p.m.
On Fri, 20 Jul 2012, javier Martin wrote:

> On 20 July 2012 13:19, Guennadi Liakhovetski <g.liakhovetski@gmx.de> wrote:
> > Hi Javier
> >
> > Thanks for the patch
> >
> > On Mon, 9 Jul 2012, javier Martin wrote:
> >
> >> On 9 July 2012 10:07, Sascha Hauer <s.hauer@pengutronix.de> wrote:
> >> > On Mon, Jul 09, 2012 at 09:46:03AM +0200, javier Martin wrote:
> >> >> On 9 July 2012 09:43, Sascha Hauer <s.hauer@pengutronix.de> wrote:
> >> >> > On Mon, Jul 09, 2012 at 09:37:25AM +0200, javier Martin wrote:
> >> >> >> On 9 July 2012 09:28, Sascha Hauer <s.hauer@pengutronix.de> wrote:
> >> >> >> > On Fri, Jul 06, 2012 at 12:56:02PM +0200, Javier Martin wrote:
> >> >> >> >> This driver wasn't converted to the new clock changes
> >> >> >> >> (clk_prepare_enable/clk_disable_unprepare). Also naming
> >> >> >> >> of emma-prp related clocks for the i.MX27 was not correct.
> >> >> >> >> ---
> >> >> >> >> Enable clocks only for i.MX27.
> >> >> >> >>
> >> >> >> >
> >> >> >> > Indeed,
> >> >> >> >
> >> >> >> >>
> >> >> >> >> -     pcdev->clk_csi = clk_get(&pdev->dev, NULL);
> >> >> >> >> -     if (IS_ERR(pcdev->clk_csi)) {
> >> >> >> >> -             dev_err(&pdev->dev, "Could not get csi clock\n");
> >> >> >> >> -             err = PTR_ERR(pcdev->clk_csi);
> >> >> >> >> -             goto exit_kfree;
> >> >> >> >> +     if (cpu_is_mx27()) {
> >> >> >> >> +             pcdev->clk_csi = devm_clk_get(&pdev->dev, "ahb");
> >> >> >> >> +             if (IS_ERR(pcdev->clk_csi)) {
> >> >> >> >> +                     dev_err(&pdev->dev, "Could not get csi clock\n");
> >> >> >> >> +                     err = PTR_ERR(pcdev->clk_csi);
> >> >> >> >> +                     goto exit_kfree;
> >> >> >> >> +             }
> >> >> >> >
> >> >> >> > but why? Now the i.MX25 won't get a clock anymore.
> >> >> >>
> >> >> >> What are the clocks needed by i.MX25? csi only?
> >> >> >>
> >> >> >> By the way, is anybody using this driver with i.MX25?
> >> >> >
> >> >> > It seems Baruch (added to Cc) has used it on an i.MX25.
> >> >>
> >> >> Baruch,
> >> >> could you tell us what are the clocks needed by i.MX25?
> >> >
> >> > I just had a look and the i.MX25 it needs three clocks: ipg, ahb and
> >> > peripheral clock. So this is broken anyway and should probably be fixed
> >> > seperately, that is:
> >> >
> >> > - provide dummy clocks for the csi clocks on i.MX27
> >> > - clk_get ipg, ahb and peripheral clocks on all SoCs
> >> > - clk_get emma clocks on i.MX27 only
> >> >
> >> > As said, this is a separate topic, so your original patch should be fine
> >> > for now.
> >
> > Well, sorry, but I don't think I can share this.
> >
> > 1. it touches two areas - arch/ and drivers/ which isnÄt a good thing and
> >    should be avoided wherever possible
> > 2. it addresses several problems: (a) missing name for "ahb" camera clock,
> >    (b) wrong device and connection names for emma clocks, (c) missing
> >    _(un)prepare suffixes in clock API
> > 3. it makes a possibly broken i.MX25 even more broken
> >
> > IIUC, mx2-camera is broken on i.MX27 in current next because of wrong
> > clock entries, right? So, we don't have to be bothered not to break
> > bisection - it is already broken. Then we can clean up the problems
> > separately under arch/ and drivers/.
> >
> > So, would it be possible to split this into 3 parts:
> >
> > (a) arch - fix clocks
> > (b) media - fix clocks on i.MX27 _without_ breaking it even further on
> >     i.MX25. If we think i.MX25 support is already broken, let's schedule
> >     its removal and remove properly, or add BROKEN to Kconfig, when built
> >     on i.MX25. In your patch this would mean just adding an "else" to your
> >     "if (cpu_is_mx27())" statement and moving the current clk_get() there
> > (c) add _(un)prepare.
> >
> > Since these are fixes, I won't wait too long for these. If you don't
> > resubmit them today, they'll go in after 3.6-rc1.
> >
> > Thanks
> > Guennadi
> >
> >> OK, thanks for your interest.
> >>
> >> Regards.
> >> --
> >> Javier Martin
> >> Vista Silicon S.L.
> >> CDTUC - FASE C - Oficina S-345
> >> Avda de los Castros s/n
> >> 39005- Santander. Cantabria. Spain
> >> +34 942 25 32 60
> >> www.vista-silicon.com
> >>
> >
> 
> Hi Guennadi,
> thanks for your review.
> 
> I could agree with some of your reasons for not accepting this patch.
> Specially (1) and (2).
> As regards of reason (3), i.MX25 has been broken for months and I
> refuse that a broken chip that no known board in the mainline uses is
> constantly slowing the development process.
> 
> Furthermore, I sent v3 of this patch 11 days ago. I know you are a
> busy developer but, honestly, it doesn't seem fair that, having sent
> the patch with plenty of time I am now given only this little time to
> fix it. I'm out of the office right now and I won't come back until
> monday. Couldn't you either make an exception and accept the path or
> give me until monday in the morning to send the new version?
> 
> Alternatively, Sascha, Fabio or any other i.MX27 user could send it
> too if they have the time.
> 
> Otherwise, as you stated it'll have to wait to 3.6-rc1. So 3.5 will be
> broken for i.MX27.

Sorry, now I'm totally lost. Are you saying, that this fix is needed for 
3.5??? Huh, nice...

commit e038ed50a4a767add205094c035b6943e7b30140
Author: Sascha Hauer <s.hauer@pengutronix.de>
Date:   Fri Mar 9 09:11:46 2012 +0100

    ARM i.MX27: implement clocks using common clock framework

appeared in 3.5-rc4 and broke stuff... Cool. I don't think it was 
mentioned in this patch, that it is for 3.5, was it? So, I naturally 
assumed it was for 3.6. Nice.

So, if I am right, and it'd that patch that broke mx2-camera, my _firm_ 
conviction is, that now days before 3.5 is released, the only right way to 
fix it is to fix _that_ regression and that regression only. I.e., we need 
a patch _only_ to fix clk-imx27.c. For which I anyway cannot help you.

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/
Javier Martin - July 23, 2012, 8:15 a.m.
On 20 July 2012 15:43, Guennadi Liakhovetski <g.liakhovetski@gmx.de> wrote:
> On Fri, 20 Jul 2012, javier Martin wrote:
>
>> On 20 July 2012 13:19, Guennadi Liakhovetski <g.liakhovetski@gmx.de> wrote:
>> > Hi Javier
>> >
>> > Thanks for the patch
>> >
>> > On Mon, 9 Jul 2012, javier Martin wrote:
>> >
>> >> On 9 July 2012 10:07, Sascha Hauer <s.hauer@pengutronix.de> wrote:
>> >> > On Mon, Jul 09, 2012 at 09:46:03AM +0200, javier Martin wrote:
>> >> >> On 9 July 2012 09:43, Sascha Hauer <s.hauer@pengutronix.de> wrote:
>> >> >> > On Mon, Jul 09, 2012 at 09:37:25AM +0200, javier Martin wrote:
>> >> >> >> On 9 July 2012 09:28, Sascha Hauer <s.hauer@pengutronix.de> wrote:
>> >> >> >> > On Fri, Jul 06, 2012 at 12:56:02PM +0200, Javier Martin wrote:
>> >> >> >> >> This driver wasn't converted to the new clock changes
>> >> >> >> >> (clk_prepare_enable/clk_disable_unprepare). Also naming
>> >> >> >> >> of emma-prp related clocks for the i.MX27 was not correct.
>> >> >> >> >> ---
>> >> >> >> >> Enable clocks only for i.MX27.
>> >> >> >> >>
>> >> >> >> >
>> >> >> >> > Indeed,
>> >> >> >> >
>> >> >> >> >>
>> >> >> >> >> -     pcdev->clk_csi = clk_get(&pdev->dev, NULL);
>> >> >> >> >> -     if (IS_ERR(pcdev->clk_csi)) {
>> >> >> >> >> -             dev_err(&pdev->dev, "Could not get csi clock\n");
>> >> >> >> >> -             err = PTR_ERR(pcdev->clk_csi);
>> >> >> >> >> -             goto exit_kfree;
>> >> >> >> >> +     if (cpu_is_mx27()) {
>> >> >> >> >> +             pcdev->clk_csi = devm_clk_get(&pdev->dev, "ahb");
>> >> >> >> >> +             if (IS_ERR(pcdev->clk_csi)) {
>> >> >> >> >> +                     dev_err(&pdev->dev, "Could not get csi clock\n");
>> >> >> >> >> +                     err = PTR_ERR(pcdev->clk_csi);
>> >> >> >> >> +                     goto exit_kfree;
>> >> >> >> >> +             }
>> >> >> >> >
>> >> >> >> > but why? Now the i.MX25 won't get a clock anymore.
>> >> >> >>
>> >> >> >> What are the clocks needed by i.MX25? csi only?
>> >> >> >>
>> >> >> >> By the way, is anybody using this driver with i.MX25?
>> >> >> >
>> >> >> > It seems Baruch (added to Cc) has used it on an i.MX25.
>> >> >>
>> >> >> Baruch,
>> >> >> could you tell us what are the clocks needed by i.MX25?
>> >> >
>> >> > I just had a look and the i.MX25 it needs three clocks: ipg, ahb and
>> >> > peripheral clock. So this is broken anyway and should probably be fixed
>> >> > seperately, that is:
>> >> >
>> >> > - provide dummy clocks for the csi clocks on i.MX27
>> >> > - clk_get ipg, ahb and peripheral clocks on all SoCs
>> >> > - clk_get emma clocks on i.MX27 only
>> >> >
>> >> > As said, this is a separate topic, so your original patch should be fine
>> >> > for now.
>> >
>> > Well, sorry, but I don't think I can share this.
>> >
>> > 1. it touches two areas - arch/ and drivers/ which isnÄt a good thing and
>> >    should be avoided wherever possible
>> > 2. it addresses several problems: (a) missing name for "ahb" camera clock,
>> >    (b) wrong device and connection names for emma clocks, (c) missing
>> >    _(un)prepare suffixes in clock API
>> > 3. it makes a possibly broken i.MX25 even more broken
>> >
>> > IIUC, mx2-camera is broken on i.MX27 in current next because of wrong
>> > clock entries, right? So, we don't have to be bothered not to break
>> > bisection - it is already broken. Then we can clean up the problems
>> > separately under arch/ and drivers/.
>> >
>> > So, would it be possible to split this into 3 parts:
>> >
>> > (a) arch - fix clocks
>> > (b) media - fix clocks on i.MX27 _without_ breaking it even further on
>> >     i.MX25. If we think i.MX25 support is already broken, let's schedule
>> >     its removal and remove properly, or add BROKEN to Kconfig, when built
>> >     on i.MX25. In your patch this would mean just adding an "else" to your
>> >     "if (cpu_is_mx27())" statement and moving the current clk_get() there
>> > (c) add _(un)prepare.
>> >
>> > Since these are fixes, I won't wait too long for these. If you don't
>> > resubmit them today, they'll go in after 3.6-rc1.
>> >
>> > Thanks
>> > Guennadi
>> >
>> >> OK, thanks for your interest.
>> >>
>> >> Regards.
>> >> --
>> >> Javier Martin
>> >> Vista Silicon S.L.
>> >> CDTUC - FASE C - Oficina S-345
>> >> Avda de los Castros s/n
>> >> 39005- Santander. Cantabria. Spain
>> >> +34 942 25 32 60
>> >> www.vista-silicon.com
>> >>
>> >
>>
>> Hi Guennadi,
>> thanks for your review.
>>
>> I could agree with some of your reasons for not accepting this patch.
>> Specially (1) and (2).
>> As regards of reason (3), i.MX25 has been broken for months and I
>> refuse that a broken chip that no known board in the mainline uses is
>> constantly slowing the development process.
>>
>> Furthermore, I sent v3 of this patch 11 days ago. I know you are a
>> busy developer but, honestly, it doesn't seem fair that, having sent
>> the patch with plenty of time I am now given only this little time to
>> fix it. I'm out of the office right now and I won't come back until
>> monday. Couldn't you either make an exception and accept the path or
>> give me until monday in the morning to send the new version?
>>
>> Alternatively, Sascha, Fabio or any other i.MX27 user could send it
>> too if they have the time.
>>
>> Otherwise, as you stated it'll have to wait to 3.6-rc1. So 3.5 will be
>> broken for i.MX27.
>
> Sorry, now I'm totally lost. Are you saying, that this fix is needed for
> 3.5??? Huh, nice...
>
> commit e038ed50a4a767add205094c035b6943e7b30140
> Author: Sascha Hauer <s.hauer@pengutronix.de>
> Date:   Fri Mar 9 09:11:46 2012 +0100
>
>     ARM i.MX27: implement clocks using common clock framework
>
> appeared in 3.5-rc4 and broke stuff... Cool. I don't think it was
> mentioned in this patch, that it is for 3.5, was it? So, I naturally
> assumed it was for 3.6. Nice.

Sorry, I thought that being named as a fix it was clear that this
patch was meant for 3.5. This is my fault.

> So, if I am right, and it'd that patch that broke mx2-camera, my _firm_
> conviction is, that now days before 3.5 is released, the only right way to
> fix it is to fix _that_ regression and that regression only. I.e., we need
> a patch _only_ to fix clk-imx27.c. For which I anyway cannot help you.

Ok. Now the fact is that 3.5 is out and it's broken for i.MX27. Now,
what is the way to proceed?

Should Sascha revert his patch or should I split mine as Guennadi requested?

Regards.

Patch

diff --git a/arch/arm/mach-imx/clk-imx27.c b/arch/arm/mach-imx/clk-imx27.c
index 295cbd7..373c8fd 100644
--- a/arch/arm/mach-imx/clk-imx27.c
+++ b/arch/arm/mach-imx/clk-imx27.c
@@ -223,7 +223,7 @@  int __init mx27_clocks_init(unsigned long fref)
 	clk_register_clkdev(clk[per3_gate], "per", "imx-fb.0");
 	clk_register_clkdev(clk[lcdc_ipg_gate], "ipg", "imx-fb.0");
 	clk_register_clkdev(clk[lcdc_ahb_gate], "ahb", "imx-fb.0");
-	clk_register_clkdev(clk[csi_ahb_gate], NULL, "mx2-camera.0");
+	clk_register_clkdev(clk[csi_ahb_gate], "ahb", "mx2-camera.0");
 	clk_register_clkdev(clk[usb_div], "per", "fsl-usb2-udc");
 	clk_register_clkdev(clk[usb_ipg_gate], "ipg", "fsl-usb2-udc");
 	clk_register_clkdev(clk[usb_ahb_gate], "ahb", "fsl-usb2-udc");
@@ -250,8 +250,10 @@  int __init mx27_clocks_init(unsigned long fref)
 	clk_register_clkdev(clk[i2c2_ipg_gate], NULL, "imx-i2c.1");
 	clk_register_clkdev(clk[owire_ipg_gate], NULL, "mxc_w1.0");
 	clk_register_clkdev(clk[kpp_ipg_gate], NULL, "imx-keypad");
-	clk_register_clkdev(clk[emma_ahb_gate], "ahb", "imx-emma");
-	clk_register_clkdev(clk[emma_ipg_gate], "ipg", "imx-emma");
+	clk_register_clkdev(clk[emma_ahb_gate], "emma-ahb", "mx2-camera.0");
+	clk_register_clkdev(clk[emma_ipg_gate], "emma-ipg", "mx2-camera.0");
+	clk_register_clkdev(clk[emma_ahb_gate], "ahb", "m2m-emmaprp.0");
+	clk_register_clkdev(clk[emma_ipg_gate], "ipg", "m2m-emmaprp.0");
 	clk_register_clkdev(clk[iim_ipg_gate], "iim", NULL);
 	clk_register_clkdev(clk[gpio_ipg_gate], "gpio", NULL);
 	clk_register_clkdev(clk[brom_ahb_gate], "brom", NULL);
diff --git a/drivers/media/video/mx2_camera.c b/drivers/media/video/mx2_camera.c
index 41f9a25..11a9353 100644
--- a/drivers/media/video/mx2_camera.c
+++ b/drivers/media/video/mx2_camera.c
@@ -270,7 +270,7 @@  struct mx2_camera_dev {
 	struct device		*dev;
 	struct soc_camera_host	soc_host;
 	struct soc_camera_device *icd;
-	struct clk		*clk_csi, *clk_emma;
+	struct clk		*clk_csi, *clk_emma_ahb, *clk_emma_ipg;
 
 	unsigned int		irq_csi, irq_emma;
 	void __iomem		*base_csi, *base_emma;
@@ -389,7 +389,8 @@  static void mx2_camera_deactivate(struct mx2_camera_dev *pcdev)
 {
 	unsigned long flags;
 
-	clk_disable(pcdev->clk_csi);
+	if (cpu_is_mx27())
+		clk_disable_unprepare(pcdev->clk_csi);
 	writel(0, pcdev->base_csi + CSICR1);
 	if (cpu_is_mx27()) {
 		writel(0, pcdev->base_emma + PRP_CNTL);
@@ -417,9 +418,11 @@  static int mx2_camera_add_device(struct soc_camera_device *icd)
 	if (pcdev->icd)
 		return -EBUSY;
 
-	ret = clk_enable(pcdev->clk_csi);
-	if (ret < 0)
-		return ret;
+	if (cpu_is_mx27()) {
+		ret = clk_prepare_enable(pcdev->clk_csi);
+		if (ret < 0)
+			return ret;
+	}
 
 	csicr1 = CSICR1_MCLKEN;
 
@@ -1616,23 +1619,12 @@  static int __devinit mx27_camera_emma_init(struct mx2_camera_dev *pcdev)
 		goto exit_iounmap;
 	}
 
-	pcdev->clk_emma = clk_get(NULL, "emma");
-	if (IS_ERR(pcdev->clk_emma)) {
-		err = PTR_ERR(pcdev->clk_emma);
-		goto exit_free_irq;
-	}
-
-	clk_enable(pcdev->clk_emma);
-
 	err = mx27_camera_emma_prp_reset(pcdev);
 	if (err)
-		goto exit_clk_emma_put;
+		goto exit_free_irq;
 
 	return err;
 
-exit_clk_emma_put:
-	clk_disable(pcdev->clk_emma);
-	clk_put(pcdev->clk_emma);
 exit_free_irq:
 	free_irq(pcdev->irq_emma, pcdev);
 exit_iounmap:
@@ -1655,6 +1647,7 @@  static int __devinit mx2_camera_probe(struct platform_device *pdev)
 
 	res_csi = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	irq_csi = platform_get_irq(pdev, 0);
+
 	if (res_csi == NULL || irq_csi < 0) {
 		dev_err(&pdev->dev, "Missing platform resources data\n");
 		err = -ENODEV;
@@ -1668,11 +1661,26 @@  static int __devinit mx2_camera_probe(struct platform_device *pdev)
 		goto exit;
 	}
 
-	pcdev->clk_csi = clk_get(&pdev->dev, NULL);
-	if (IS_ERR(pcdev->clk_csi)) {
-		dev_err(&pdev->dev, "Could not get csi clock\n");
-		err = PTR_ERR(pcdev->clk_csi);
-		goto exit_kfree;
+	if (cpu_is_mx27()) {
+		pcdev->clk_csi = devm_clk_get(&pdev->dev, "ahb");
+		if (IS_ERR(pcdev->clk_csi)) {
+			dev_err(&pdev->dev, "Could not get csi clock\n");
+			err = PTR_ERR(pcdev->clk_csi);
+			goto exit_kfree;
+		}
+		pcdev->clk_emma_ipg = devm_clk_get(&pdev->dev, "emma-ipg");
+		if (IS_ERR(pcdev->clk_emma_ipg)) {
+			err = PTR_ERR(pcdev->clk_emma_ipg);
+			goto exit_kfree;
+		}
+		pcdev->clk_emma_ahb = devm_clk_get(&pdev->dev, "emma-ahb");
+		if (IS_ERR(pcdev->clk_emma_ahb)) {
+			err = PTR_ERR(pcdev->clk_emma_ahb);
+			goto exit_kfree;
+		}
+		clk_prepare_enable(pcdev->clk_csi);
+		clk_prepare_enable(pcdev->clk_emma_ipg);
+		clk_prepare_enable(pcdev->clk_emma_ahb);
 	}
 
 	pcdev->res_csi = res_csi;
@@ -1768,8 +1776,8 @@  exit_free_emma:
 eallocctx:
 	if (cpu_is_mx27()) {
 		free_irq(pcdev->irq_emma, pcdev);
-		clk_disable(pcdev->clk_emma);
-		clk_put(pcdev->clk_emma);
+		clk_disable_unprepare(pcdev->clk_emma_ipg);
+		clk_disable_unprepare(pcdev->clk_emma_ahb);
 		iounmap(pcdev->base_emma);
 		release_mem_region(pcdev->res_emma->start, resource_size(pcdev->res_emma));
 	}
@@ -1781,7 +1789,11 @@  exit_iounmap:
 exit_release:
 	release_mem_region(res_csi->start, resource_size(res_csi));
 exit_dma_free:
-	clk_put(pcdev->clk_csi);
+	if (cpu_is_mx27()) {
+		clk_disable_unprepare(pcdev->clk_emma_ipg);
+		clk_disable_unprepare(pcdev->clk_emma_ahb);
+		clk_disable_unprepare(pcdev->clk_csi);
+	}
 exit_kfree:
 	kfree(pcdev);
 exit:
@@ -1795,7 +1807,6 @@  static int __devexit mx2_camera_remove(struct platform_device *pdev)
 			struct mx2_camera_dev, soc_host);
 	struct resource *res;
 
-	clk_put(pcdev->clk_csi);
 	if (cpu_is_mx25())
 		free_irq(pcdev->irq_csi, pcdev);
 	if (cpu_is_mx27())
@@ -1808,8 +1819,8 @@  static int __devexit mx2_camera_remove(struct platform_device *pdev)
 	iounmap(pcdev->base_csi);
 
 	if (cpu_is_mx27()) {
-		clk_disable(pcdev->clk_emma);
-		clk_put(pcdev->clk_emma);
+		clk_disable_unprepare(pcdev->clk_emma_ipg);
+		clk_disable_unprepare(pcdev->clk_emma_ahb);
 		iounmap(pcdev->base_emma);
 		res = pcdev->res_emma;
 		release_mem_region(res->start, resource_size(res));