Re: [PATCH v8 1/7] media: V4L2: add temporary clock helpers

Re: [PATCH v8 1/7] media: V4L2: add temporary clock helpers

[Barry Song]

Hi Guennadi,

> Typical video devices like camera sensors require an external clock source.
> Many such devices cannot even access their hardware registers without a
> running clock. These clock sources should be controlled by their consumers.
> This should be performed, using the generic clock framework. Unfortunately
> so far only very few systems have been ported to that framework. This patch
> adds a set of temporary helpers, mimicking the generic clock API, to V4L2.
> Platforms, adopting the clock API, should switch to using it. Eventually
> this temporary API should be removed.

> Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@xxxxxx>
> ---

for your patch 1/8 and 3/8, i think it makes a lot of senses to let
the object manages its own clock by itself.
is it possible for us to implement v4l2-clk.c directly as an instance
of standard clk driver for those systems which don't have generic
clock,  and remove the V4L2 clock APIs like v4l2_clk_get,
v4l2_clk_enable from the first day? i mean v4l2-clk.c becomes a temp
and fake clock controller driver. finally, after people have
generically clk, remove it.

> v8: Updated both (C) dates

>  drivers/media/v4l2-core/Makefile   |    2 +-
>  drivers/media/v4l2-core/v4l2-clk.c |  177 ++++++++++++++++++++++++++++++++++++
>  include/media/v4l2-clk.h           |   54 +++++++++++
>  3 files changed, 232 insertions(+), 1 deletions(-)
>  create mode 100644 drivers/media/v4l2-core/v4l2-clk.c
>  create mode 100644 include/media/v4l2-clk.h

> diff --git a/drivers/media/v4l2-core/Makefile b/drivers/media/v4l2-core/Makefile
> index aa50c46..628c630 100644
> --- a/drivers/media/v4l2-core/Makefile
> +++ b/drivers/media/v4l2-core/Makefile
> @@ -5,7 +5,7 @@
>  tuner-objs	:=	tuner-core.o

>  videodev-objs	:=	v4l2-dev.o v4l2-ioctl.o v4l2-device.o v4l2-fh.o \
> -			v4l2-event.o v4l2-ctrls.o v4l2-subdev.o
> +			v4l2-event.o v4l2-ctrls.o v4l2-subdev.o v4l2-clk.o
> ifeq ($(CONFIG_COMPAT),y)
>    videodev-objs += v4l2-compat-ioctl32.o
>  endif
> diff --git a/drivers/media/v4l2-core/v4l2-clk.c b/drivers/media/v4l2-core/v4l2-clk.c
> new file mode 100644
> index 0000000..d7cc13e
> --- /dev/null
> +++ b/drivers/media/v4l2-core/v4l2-clk.c
> @@ -0,0 +1,177 @@

-barry

Re: [PATCH v8 1/7] media: V4L2: add temporary clock helpers

[Guennadi Liakhovetski]

Hi Barry

On Thu, 11 Apr 2013, Barry Song wrote:

> Hi Guennadi,
> 
> > Typical video devices like camera sensors require an external clock source.
> > Many such devices cannot even access their hardware registers without a
> > running clock. These clock sources should be controlled by their consumers.
> > This should be performed, using the generic clock framework. Unfortunately
> > so far only very few systems have been ported to that framework. This patch
> > adds a set of temporary helpers, mimicking the generic clock API, to V4L2.
> > Platforms, adopting the clock API, should switch to using it. Eventually
> > this temporary API should be removed.
> 
> > Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@xxxxxx>
> > ---
> 
> for your patch 1/8 and 3/8, i think it makes a lot of senses to let
> the object manages its own clock by itself.
> is it possible for us to implement v4l2-clk.c directly as an instance
> of standard clk driver for those systems which don't have generic
> clock,  and remove the V4L2 clock APIs like v4l2_clk_get,
> v4l2_clk_enable from the first day? i mean v4l2-clk.c becomes a temp
> and fake clock controller driver. finally, after people have
> generically clk, remove it.

I don't think you can force-enable the CFF on systems, that don't support 
it, e.g. PXA.

Thanks
Guennadi

> > v8: Updated both (C) dates
> 
> >  drivers/media/v4l2-core/Makefile   |    2 +-
> >  drivers/media/v4l2-core/v4l2-clk.c |  177 ++++++++++++++++++++++++++++++++++++
> >  include/media/v4l2-clk.h           |   54 +++++++++++
> >  3 files changed, 232 insertions(+), 1 deletions(-)
> >  create mode 100644 drivers/media/v4l2-core/v4l2-clk.c
> >  create mode 100644 include/media/v4l2-clk.h
> 
> > diff --git a/drivers/media/v4l2-core/Makefile b/drivers/media/v4l2-core/Makefile
> > index aa50c46..628c630 100644
> > --- a/drivers/media/v4l2-core/Makefile
> > +++ b/drivers/media/v4l2-core/Makefile
> > @@ -5,7 +5,7 @@
> >  tuner-objs	:=	tuner-core.o
> 
> >  videodev-objs	:=	v4l2-dev.o v4l2-ioctl.o v4l2-device.o v4l2-fh.o \
> > -			v4l2-event.o v4l2-ctrls.o v4l2-subdev.o
> > +			v4l2-event.o v4l2-ctrls.o v4l2-subdev.o v4l2-clk.o
> > ifeq ($(CONFIG_COMPAT),y)
> >    videodev-objs += v4l2-compat-ioctl32.o
> >  endif
> > diff --git a/drivers/media/v4l2-core/v4l2-clk.c b/drivers/media/v4l2-core/v4l2-clk.c
> > new file mode 100644
> > index 0000000..d7cc13e
> > --- /dev/null
> > +++ b/drivers/media/v4l2-core/v4l2-clk.c
> > @@ -0,0 +1,177 @@
> 
> -barry
> 

---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

Re: [PATCH v8 1/7] media: V4L2: add temporary clock helpers

[Barry Song]

2013/4/11 Guennadi Liakhovetski <g.liakhovetski@gmx.de>:
> Hi Barry
>
> On Thu, 11 Apr 2013, Barry Song wrote:
>
>> Hi Guennadi,
>>
>> > Typical video devices like camera sensors require an external clock source.
>> > Many such devices cannot even access their hardware registers without a
>> > running clock. These clock sources should be controlled by their consumers.
>> > This should be performed, using the generic clock framework. Unfortunately
>> > so far only very few systems have been ported to that framework. This patch
>> > adds a set of temporary helpers, mimicking the generic clock API, to V4L2.
>> > Platforms, adopting the clock API, should switch to using it. Eventually
>> > this temporary API should be removed.
>>
>> > Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@xxxxxx>
>> > ---
>>
>> for your patch 1/8 and 3/8, i think it makes a lot of senses to let
>> the object manages its own clock by itself.
>> is it possible for us to implement v4l2-clk.c directly as an instance
>> of standard clk driver for those systems which don't have generic
>> clock,  and remove the V4L2 clock APIs like v4l2_clk_get,
>> v4l2_clk_enable from the first day? i mean v4l2-clk.c becomes a temp
>> and fake clock controller driver. finally, after people have
>> generically clk, remove it.
>
> I don't think you can force-enable the CFF on systems, that don't support
> it, e.g. PXA.

yes. we can. clock is only a framework, has it any limitation to
implement a driver instance on any platform?
people have tried to move to common clk and generic framework for a
long time, now you still try to provide a v4l2 specific clock APIs, it
just makes v4l2 unacceptable and much complex.

>
> Thanks
> Guennadi
>
>> > v8: Updated both (C) dates
>>
>> >  drivers/media/v4l2-core/Makefile   |    2 +-
>> >  drivers/media/v4l2-core/v4l2-clk.c |  177 ++++++++++++++++++++++++++++++++++++
>> >  include/media/v4l2-clk.h           |   54 +++++++++++
>> >  3 files changed, 232 insertions(+), 1 deletions(-)
>> >  create mode 100644 drivers/media/v4l2-core/v4l2-clk.c
>> >  create mode 100644 include/media/v4l2-clk.h
>>
>> > diff --git a/drivers/media/v4l2-core/Makefile b/drivers/media/v4l2-core/Makefile
>> > index aa50c46..628c630 100644
>> > --- a/drivers/media/v4l2-core/Makefile
>> > +++ b/drivers/media/v4l2-core/Makefile
>> > @@ -5,7 +5,7 @@
>> >  tuner-objs :=      tuner-core.o
>>
>> >  videodev-objs      :=      v4l2-dev.o v4l2-ioctl.o v4l2-device.o v4l2-fh.o \
>> > -                   v4l2-event.o v4l2-ctrls.o v4l2-subdev.o
>> > +                   v4l2-event.o v4l2-ctrls.o v4l2-subdev.o v4l2-clk.o
>> > ifeq ($(CONFIG_COMPAT),y)
>> >    videodev-objs += v4l2-compat-ioctl32.o
>> >  endif
>> > diff --git a/drivers/media/v4l2-core/v4l2-clk.c b/drivers/media/v4l2-core/v4l2-clk.c
>> > new file mode 100644
>> > index 0000000..d7cc13e
>> > --- /dev/null
>> > +++ b/drivers/media/v4l2-core/v4l2-clk.c
>> > @@ -0,0 +1,177 @@
>>
>> -barry

-barry

Re: [PATCH v8 1/7] media: V4L2: add temporary clock helpers

[Guennadi Liakhovetski]

On Thu, 11 Apr 2013, Barry Song wrote:

> 2013/4/11 Guennadi Liakhovetski <g.liakhovetski@gmx.de>:
> > Hi Barry
> >
> > On Thu, 11 Apr 2013, Barry Song wrote:
> >
> >> Hi Guennadi,
> >>
> >> > Typical video devices like camera sensors require an external clock source.
> >> > Many such devices cannot even access their hardware registers without a
> >> > running clock. These clock sources should be controlled by their consumers.
> >> > This should be performed, using the generic clock framework. Unfortunately
> >> > so far only very few systems have been ported to that framework. This patch
> >> > adds a set of temporary helpers, mimicking the generic clock API, to V4L2.
> >> > Platforms, adopting the clock API, should switch to using it. Eventually
> >> > this temporary API should be removed.
> >>
> >> > Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@xxxxxx>
> >> > ---
> >>
> >> for your patch 1/8 and 3/8, i think it makes a lot of senses to let
> >> the object manages its own clock by itself.
> >> is it possible for us to implement v4l2-clk.c directly as an instance
> >> of standard clk driver for those systems which don't have generic
> >> clock,  and remove the V4L2 clock APIs like v4l2_clk_get,
> >> v4l2_clk_enable from the first day? i mean v4l2-clk.c becomes a temp
> >> and fake clock controller driver. finally, after people have
> >> generically clk, remove it.
> >
> > I don't think you can force-enable the CFF on systems, that don't support
> > it, e.g. PXA.
> 
> yes. we can. clock is only a framework, has it any limitation to
> implement a driver instance on any platform?

So, you enable CFF, it provides its own clk_* implementation like 
clk_get_rate() etc. Now, PXA already has it defined in 
arch/arm/mach-pxa/clock.c. Don't think this is going to fly.

Thanks
Guennadi

> people have tried to move to common clk and generic framework for a
> long time, now you still try to provide a v4l2 specific clock APIs, it
> just makes v4l2 unacceptable and much complex.
> 
> >
> > Thanks
> > Guennadi
> >
> >> > v8: Updated both (C) dates
> >>
> >> >  drivers/media/v4l2-core/Makefile   |    2 +-
> >> >  drivers/media/v4l2-core/v4l2-clk.c |  177 ++++++++++++++++++++++++++++++++++++
> >> >  include/media/v4l2-clk.h           |   54 +++++++++++
> >> >  3 files changed, 232 insertions(+), 1 deletions(-)
> >> >  create mode 100644 drivers/media/v4l2-core/v4l2-clk.c
> >> >  create mode 100644 include/media/v4l2-clk.h
> >>
> >> > diff --git a/drivers/media/v4l2-core/Makefile b/drivers/media/v4l2-core/Makefile
> >> > index aa50c46..628c630 100644
> >> > --- a/drivers/media/v4l2-core/Makefile
> >> > +++ b/drivers/media/v4l2-core/Makefile
> >> > @@ -5,7 +5,7 @@
> >> >  tuner-objs :=      tuner-core.o
> >>
> >> >  videodev-objs      :=      v4l2-dev.o v4l2-ioctl.o v4l2-device.o v4l2-fh.o \
> >> > -                   v4l2-event.o v4l2-ctrls.o v4l2-subdev.o
> >> > +                   v4l2-event.o v4l2-ctrls.o v4l2-subdev.o v4l2-clk.o
> >> > ifeq ($(CONFIG_COMPAT),y)
> >> >    videodev-objs += v4l2-compat-ioctl32.o
> >> >  endif
> >> > diff --git a/drivers/media/v4l2-core/v4l2-clk.c b/drivers/media/v4l2-core/v4l2-clk.c
> >> > new file mode 100644
> >> > index 0000000..d7cc13e
> >> > --- /dev/null
> >> > +++ b/drivers/media/v4l2-core/v4l2-clk.c
> >> > @@ -0,0 +1,177 @@
> >>
> >> -barry
> 
> -barry
> 

---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

Re: [PATCH v8 1/7] media: V4L2: add temporary clock helpers

[Barry Song]

2013/4/11 Guennadi Liakhovetski <g.liakhovetski@gmx.de>:
> On Thu, 11 Apr 2013, Barry Song wrote:
>
>> 2013/4/11 Guennadi Liakhovetski <g.liakhovetski@gmx.de>:
>> > Hi Barry
>> >
>> > On Thu, 11 Apr 2013, Barry Song wrote:
>> >
>> >> Hi Guennadi,
>> >>
>> >> > Typical video devices like camera sensors require an external clock source.
>> >> > Many such devices cannot even access their hardware registers without a
>> >> > running clock. These clock sources should be controlled by their consumers.
>> >> > This should be performed, using the generic clock framework. Unfortunately
>> >> > so far only very few systems have been ported to that framework. This patch
>> >> > adds a set of temporary helpers, mimicking the generic clock API, to V4L2.
>> >> > Platforms, adopting the clock API, should switch to using it. Eventually
>> >> > this temporary API should be removed.
>> >>
>> >> > Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@xxxxxx>
>> >> > ---
>> >>
>> >> for your patch 1/8 and 3/8, i think it makes a lot of senses to let
>> >> the object manages its own clock by itself.
>> >> is it possible for us to implement v4l2-clk.c directly as an instance
>> >> of standard clk driver for those systems which don't have generic
>> >> clock,  and remove the V4L2 clock APIs like v4l2_clk_get,
>> >> v4l2_clk_enable from the first day? i mean v4l2-clk.c becomes a temp
>> >> and fake clock controller driver. finally, after people have
>> >> generically clk, remove it.
>> >
>> > I don't think you can force-enable the CFF on systems, that don't support
>> > it, e.g. PXA.
>>
>> yes. we can. clock is only a framework, has it any limitation to
>> implement a driver instance on any platform?
>
> So, you enable CFF, it provides its own clk_* implementation like
> clk_get_rate() etc. Now, PXA already has it defined in
> arch/arm/mach-pxa/clock.c. Don't think this is going to fly.

agree.

>
> Thanks
> Guennadi
>
>> people have tried to move to common clk and generic framework for a
>> long time, now you still try to provide a v4l2 specific clock APIs, it
>> just makes v4l2 unacceptable and much complex.
>>
>> >
>> > Thanks
>> > Guennadi
>> >
>> >> > v8: Updated both (C) dates
>> >>
>> >> >  drivers/media/v4l2-core/Makefile   |    2 +-
>> >> >  drivers/media/v4l2-core/v4l2-clk.c |  177 ++++++++++++++++++++++++++++++++++++
>> >> >  include/media/v4l2-clk.h           |   54 +++++++++++
>> >> >  3 files changed, 232 insertions(+), 1 deletions(-)
>> >> >  create mode 100644 drivers/media/v4l2-core/v4l2-clk.c
>> >> >  create mode 100644 include/media/v4l2-clk.h
>> >>
>> >> > diff --git a/drivers/media/v4l2-core/Makefile b/drivers/media/v4l2-core/Makefile
>> >> > index aa50c46..628c630 100644
>> >> > --- a/drivers/media/v4l2-core/Makefile
>> >> > +++ b/drivers/media/v4l2-core/Makefile
>> >> > @@ -5,7 +5,7 @@
>> >> >  tuner-objs :=      tuner-core.o
>> >>
>> >> >  videodev-objs      :=      v4l2-dev.o v4l2-ioctl.o v4l2-device.o v4l2-fh.o \
>> >> > -                   v4l2-event.o v4l2-ctrls.o v4l2-subdev.o
>> >> > +                   v4l2-event.o v4l2-ctrls.o v4l2-subdev.o v4l2-clk.o
>> >> > ifeq ($(CONFIG_COMPAT),y)
>> >> >    videodev-objs += v4l2-compat-ioctl32.o
>> >> >  endif
>> >> > diff --git a/drivers/media/v4l2-core/v4l2-clk.c b/drivers/media/v4l2-core/v4l2-clk.c
>> >> > new file mode 100644
>> >> > index 0000000..d7cc13e
>> >> > --- /dev/null
>> >> > +++ b/drivers/media/v4l2-core/v4l2-clk.c
>> >> > @@ -0,0 +1,177 @@
>> >>
>> >> -barry

-barry

Re: [PATCH v8 1/7] media: V4L2: add temporary clock helpers

[Mike Turquette]

Quoting Barry Song (2013-04-11 01:59:28)
> 2013/4/11 Guennadi Liakhovetski <g.liakhovetski@gmx.de>:
> > On Thu, 11 Apr 2013, Barry Song wrote:
> >
> >> 2013/4/11 Guennadi Liakhovetski <g.liakhovetski@gmx.de>:
> >> > Hi Barry
> >> >
> >> > On Thu, 11 Apr 2013, Barry Song wrote:
> >> >
> >> >> Hi Guennadi,
> >> >>
> >> >> > Typical video devices like camera sensors require an external clock source.
> >> >> > Many such devices cannot even access their hardware registers without a
> >> >> > running clock. These clock sources should be controlled by their consumers.
> >> >> > This should be performed, using the generic clock framework. Unfortunately
> >> >> > so far only very few systems have been ported to that framework. This patch
> >> >> > adds a set of temporary helpers, mimicking the generic clock API, to V4L2.
> >> >> > Platforms, adopting the clock API, should switch to using it. Eventually
> >> >> > this temporary API should be removed.
> >> >>
> >> >> > Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@xxxxxx>
> >> >> > ---
> >> >>
> >> >> for your patch 1/8 and 3/8, i think it makes a lot of senses to let
> >> >> the object manages its own clock by itself.
> >> >> is it possible for us to implement v4l2-clk.c directly as an instance
> >> >> of standard clk driver for those systems which don't have generic
> >> >> clock,  and remove the V4L2 clock APIs like v4l2_clk_get,
> >> >> v4l2_clk_enable from the first day? i mean v4l2-clk.c becomes a temp
> >> >> and fake clock controller driver. finally, after people have
> >> >> generically clk, remove it.
> >> >
> >> > I don't think you can force-enable the CFF on systems, that don't support
> >> > it, e.g. PXA.
> >>
> >> yes. we can. clock is only a framework, has it any limitation to
> >> implement a driver instance on any platform?
> >
> > So, you enable CFF, it provides its own clk_* implementation like
> > clk_get_rate() etc. Now, PXA already has it defined in
> > arch/arm/mach-pxa/clock.c. Don't think this is going to fly.
> 
> agree.
> 

Hi,

I came into this thread late and don't have the actual patches in my
inbox for review.  That said, I don't understand why V4L2 cares about
the clk framework *implementation*?  The clk.h api is the same for
platforms using the common struct clk and those still using the legacy
method of defining their own struct clk.  If drivers are only consumers
of the clk.h api then the implementation underneath should not matter.

Regards,
Mike

> >
> > Thanks
> > Guennadi
> >
> >> people have tried to move to common clk and generic framework for a
> >> long time, now you still try to provide a v4l2 specific clock APIs, it
> >> just makes v4l2 unacceptable and much complex.
> >>
> >> >
> >> > Thanks
> >> > Guennadi
> >> >
> >> >> > v8: Updated both (C) dates
> >> >>
> >> >> >  drivers/media/v4l2-core/Makefile   |    2 +-
> >> >> >  drivers/media/v4l2-core/v4l2-clk.c |  177 ++++++++++++++++++++++++++++++++++++
> >> >> >  include/media/v4l2-clk.h           |   54 +++++++++++
> >> >> >  3 files changed, 232 insertions(+), 1 deletions(-)
> >> >> >  create mode 100644 drivers/media/v4l2-core/v4l2-clk.c
> >> >> >  create mode 100644 include/media/v4l2-clk.h
> >> >>
> >> >> > diff --git a/drivers/media/v4l2-core/Makefile b/drivers/media/v4l2-core/Makefile
> >> >> > index aa50c46..628c630 100644
> >> >> > --- a/drivers/media/v4l2-core/Makefile
> >> >> > +++ b/drivers/media/v4l2-core/Makefile
> >> >> > @@ -5,7 +5,7 @@
> >> >> >  tuner-objs :=      tuner-core.o
> >> >>
> >> >> >  videodev-objs      :=      v4l2-dev.o v4l2-ioctl.o v4l2-device.o v4l2-fh.o \
> >> >> > -                   v4l2-event.o v4l2-ctrls.o v4l2-subdev.o
> >> >> > +                   v4l2-event.o v4l2-ctrls.o v4l2-subdev.o v4l2-clk.o
> >> >> > ifeq ($(CONFIG_COMPAT),y)
> >> >> >    videodev-objs += v4l2-compat-ioctl32.o
> >> >> >  endif
> >> >> > diff --git a/drivers/media/v4l2-core/v4l2-clk.c b/drivers/media/v4l2-core/v4l2-clk.c
> >> >> > new file mode 100644
> >> >> > index 0000000..d7cc13e
> >> >> > --- /dev/null
> >> >> > +++ b/drivers/media/v4l2-core/v4l2-clk.c
> >> >> > @@ -0,0 +1,177 @@
> >> >>
> >> >> -barry
> 
> -barry
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v8 1/7] media: V4L2: add temporary clock helpers

[Sylwester Nawrocki]

Hi,

On 04/11/2013 08:52 PM, Mike Turquette wrote:
[...]
>>> So, you enable CFF, it provides its own clk_* implementation like
>>> clk_get_rate() etc. Now, PXA already has it defined in
>>> arch/arm/mach-pxa/clock.c. Don't think this is going to fly.
>>
>> agree.
>
> Hi,
>
> I came into this thread late and don't have the actual patches in my
> inbox for review.  That said, I don't understand why V4L2 cares about
> the clk framework *implementation*?  The clk.h api is the same for
> platforms using the common struct clk and those still using the legacy
> method of defining their own struct clk.  If drivers are only consumers
> of the clk.h api then the implementation underneath should not matter.

I came to similar conclusions previously, but in case when one of the two
drivers is the clock provider I think there is still an issue there.

The drivers are supposed to be platform agnostic, but the clock provider
would have to include mach specific declarations of struct clk, wouldn't
it ?

Regards,
Sylwester

Re: [PATCH v8 1/7] media: V4L2: add temporary clock helpers

[Laurent Pinchart]

Hi Mike,

On Thursday 11 April 2013 11:52:58 Mike Turquette wrote:
> Quoting Barry Song (2013-04-11 01:59:28)
> > 2013/4/11 Guennadi Liakhovetski <g.liakhovetski@gmx.de>:
> > > On Thu, 11 Apr 2013, Barry Song wrote:
> > >> 2013/4/11 Guennadi Liakhovetski <g.liakhovetski@gmx.de>:
> > >> > On Thu, 11 Apr 2013, Barry Song wrote:
> > >> >> Hi Guennadi,
> > >> >> 
> > >> >> > Typical video devices like camera sensors require an external
> > >> >> > clock source. Many such devices cannot even access their hardware
> > >> >> > registers without a running clock. These clock sources should be
> > >> >> > controlled by their consumers. This should be performed, using the
> > >> >> > generic clock framework. Unfortunately so far only very few
> > >> >> > systems have been ported to that framework. This patch adds a set
> > >> >> > of temporary helpers, mimicking the generic clock API, to V4L2.
> > >> >> > Platforms, adopting the clock API, should switch to using it.
> > >> >> > Eventually this temporary API should be removed.
> > >> >> > 
> > >> >> > Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@xxxxxx>
> > >> >> > ---
> > >> >> 
> > >> >> for your patch 1/8 and 3/8, i think it makes a lot of senses to let
> > >> >> the object manages its own clock by itself.
> > >> >> is it possible for us to implement v4l2-clk.c directly as an
> > >> >> instance of standard clk driver for those systems which don't have
> > >> >> generic clock,  and remove the V4L2 clock APIs like v4l2_clk_get,
> > >> >> v4l2_clk_enable from the first day? i mean v4l2-clk.c becomes a temp
> > >> >> and fake clock controller driver. finally, after people have
> > >> >> generically clk, remove it.
> > >> > 
> > >> > I don't think you can force-enable the CFF on systems, that don't
> > >> > support it, e.g. PXA.
> > >> 
> > >> yes. we can. clock is only a framework, has it any limitation to
> > >> implement a driver instance on any platform?
> > > 
> > > So, you enable CFF, it provides its own clk_* implementation like
> > > clk_get_rate() etc. Now, PXA already has it defined in
> > > arch/arm/mach-pxa/clock.c. Don't think this is going to fly.
> > 
> > agree.
> 
> I came into this thread late and don't have the actual patches in my inbox
> for review.  That said, I don't understand why V4L2 cares about the clk
> framework *implementation*?  The clk.h api is the same for platforms using
> the common struct clk and those still using the legacy method of defining
> their own struct clk.  If drivers are only consumers of the clk.h api then
> the implementation underneath should not matter.

The issue on non-CCF systems is that devices usually can't register clocks 
dynamically. (Most of) those systems provide system clocks only through their 
clock API, without a way for the camera IP core to hook up the clock(s) it can 
provide to the camera sensor. On the consumer side we don't care much about 
the clock framework implementation, but on the provider side we need a 
framework that allows registering non-system clocks at runtime.

-- 
Regards,

Laurent Pinchart

Re: [PATCH v8 1/7] media: V4L2: add temporary clock helpers

[Laurent Pinchart]

Hi Mike,

On Thursday 11 April 2013 16:19:23 Mike Turquette wrote:
> Quoting Laurent Pinchart (2013-04-11 15:35:38)
> > On Thursday 11 April 2013 11:52:58 Mike Turquette wrote:

[snip]

> > > I came into this thread late and don't have the actual patches in my
> > > inbox for review.  That said, I don't understand why V4L2 cares about
> > > the clk framework *implementation*?  The clk.h api is the same for
> > > platforms using the common struct clk and those still using the legacy
> > > method of defining their own struct clk.  If drivers are only consumers
> > > of the clk.h api then the implementation underneath should not matter.
> > 
> > The issue on non-CCF systems is that devices usually can't register clocks
> > dynamically. (Most of) those systems provide system clocks only through
> > their clock API, without a way for the camera IP core to hook up the
> > clock(s) it can provide to the camera sensor. On the consumer side we
> > don't care much about the clock framework implementation, but on the
> > provider side we need a framework that allows registering non-system
> > clocks at runtime.
> 
> Yes, you do care about the clock framework implementation if you are a clock
> provider.  I still haven't gone through the archives to find these patches
> but I hope that any dependency on CONFIG_COMMON_CLK is conditionalized to
> have the smallest impact possible. Making v4l2 as a whole depend on
> COMMON_CLK might be a bit overkill compared to just making individual camera
> drivers depend on it.

The basic idea is to push the dependency on CONFIG_COMMON_CLK to individual 
drivers, and provide a V4L2-specific clock framework (that looks like a 
stripped-down version of CCF) for platforms that don't implement CCF yet.

-- 
Regards,

Laurent Pinchart

[PATCH v8 0/7] V4L2 clock and async patches and soc-camera example

[Guennadi Liakhovetski]

Mostly just a re-spin of v7 with minor modifications.

Guennadi Liakhovetski (7):
  media: V4L2: add temporary clock helpers
  media: V4L2: support asynchronous subdevice registration
  media: soc-camera: switch I2C subdevice drivers to use v4l2-clk
  soc-camera: add V4L2-async support
  sh_mobile_ceu_camera: add asynchronous subdevice probing support
  imx074: support asynchronous probing
  ARM: shmobile: convert ap4evb to asynchronously register camera
    subdevices

 arch/arm/mach-shmobile/board-ap4evb.c              |  103 ++--
 arch/arm/mach-shmobile/clock-sh7372.c              |    1 +
 drivers/media/i2c/soc_camera/imx074.c              |   36 +-
 drivers/media/i2c/soc_camera/mt9m001.c             |   17 +-
 drivers/media/i2c/soc_camera/mt9m111.c             |   20 +-
 drivers/media/i2c/soc_camera/mt9t031.c             |   19 +-
 drivers/media/i2c/soc_camera/mt9t112.c             |   19 +-
 drivers/media/i2c/soc_camera/mt9v022.c             |   17 +-
 drivers/media/i2c/soc_camera/ov2640.c              |   19 +-
 drivers/media/i2c/soc_camera/ov5642.c              |   20 +-
 drivers/media/i2c/soc_camera/ov6650.c              |   17 +-
 drivers/media/i2c/soc_camera/ov772x.c              |   15 +-
 drivers/media/i2c/soc_camera/ov9640.c              |   17 +-
 drivers/media/i2c/soc_camera/ov9640.h              |    1 +
 drivers/media/i2c/soc_camera/ov9740.c              |   18 +-
 drivers/media/i2c/soc_camera/rj54n1cb0c.c          |   17 +-
 drivers/media/i2c/soc_camera/tw9910.c              |   18 +-
 .../platform/soc_camera/sh_mobile_ceu_camera.c     |  134 +++--
 drivers/media/platform/soc_camera/sh_mobile_csi2.c |  163 +++--
 drivers/media/platform/soc_camera/soc_camera.c     |  666 ++++++++++++++++----
 .../platform/soc_camera/soc_camera_platform.c      |    2 +-
 drivers/media/v4l2-core/Makefile                   |    3 +-
 drivers/media/v4l2-core/v4l2-async.c               |  262 ++++++++
 drivers/media/v4l2-core/v4l2-clk.c                 |  177 ++++++
 include/media/sh_mobile_ceu.h                      |    2 +
 include/media/sh_mobile_csi2.h                     |    2 +-
 include/media/soc_camera.h                         |   36 +-
 include/media/v4l2-async.h                         |  104 +++
 include/media/v4l2-clk.h                           |   54 ++
 29 files changed, 1675 insertions(+), 304 deletions(-)
 create mode 100644 drivers/media/v4l2-core/v4l2-async.c
 create mode 100644 drivers/media/v4l2-core/v4l2-clk.c
 create mode 100644 include/media/v4l2-async.h
 create mode 100644 include/media/v4l2-clk.h

-- 
1.7.2.5

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

[PATCH v8 1/7] media: V4L2: add temporary clock helpers

[Guennadi Liakhovetski]

Typical video devices like camera sensors require an external clock source.
Many such devices cannot even access their hardware registers without a
running clock. These clock sources should be controlled by their consumers.
This should be performed, using the generic clock framework. Unfortunately
so far only very few systems have been ported to that framework. This patch
adds a set of temporary helpers, mimicking the generic clock API, to V4L2.
Platforms, adopting the clock API, should switch to using it. Eventually
this temporary API should be removed.

Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
---

v8: Updated both (C) dates

 drivers/media/v4l2-core/Makefile   |    2 +-
 drivers/media/v4l2-core/v4l2-clk.c |  177 ++++++++++++++++++++++++++++++++++++
 include/media/v4l2-clk.h           |   54 +++++++++++
 3 files changed, 232 insertions(+), 1 deletions(-)
 create mode 100644 drivers/media/v4l2-core/v4l2-clk.c
 create mode 100644 include/media/v4l2-clk.h

diff --git a/drivers/media/v4l2-core/Makefile b/drivers/media/v4l2-core/Makefile
index aa50c46..628c630 100644
--- a/drivers/media/v4l2-core/Makefile
+++ b/drivers/media/v4l2-core/Makefile
@@ -5,7 +5,7 @@
 tuner-objs	:=	tuner-core.o
 
 videodev-objs	:=	v4l2-dev.o v4l2-ioctl.o v4l2-device.o v4l2-fh.o \
-			v4l2-event.o v4l2-ctrls.o v4l2-subdev.o
+			v4l2-event.o v4l2-ctrls.o v4l2-subdev.o v4l2-clk.o
 ifeq ($(CONFIG_COMPAT),y)
   videodev-objs += v4l2-compat-ioctl32.o
 endif
diff --git a/drivers/media/v4l2-core/v4l2-clk.c b/drivers/media/v4l2-core/v4l2-clk.c
new file mode 100644
index 0000000..d7cc13e
--- /dev/null
+++ b/drivers/media/v4l2-core/v4l2-clk.c
@@ -0,0 +1,177 @@
+/*
+ * V4L2 clock service
+ *
+ * Copyright (C) 2012-2013, Guennadi Liakhovetski <g.liakhovetski@gmx.de>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include <linux/atomic.h>
+#include <linux/device.h>
+#include <linux/errno.h>
+#include <linux/list.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/slab.h>
+#include <linux/string.h>
+
+#include <media/v4l2-clk.h>
+#include <media/v4l2-subdev.h>
+
+static DEFINE_MUTEX(clk_lock);
+static LIST_HEAD(clk_list);
+
+static struct v4l2_clk *v4l2_clk_find(const char *dev_id, const char *id)
+{
+	struct v4l2_clk *clk;
+
+	list_for_each_entry(clk, &clk_list, list) {
+		if (strcmp(dev_id, clk->dev_id))
+			continue;
+
+		if (!id || !clk->id || !strcmp(clk->id, id))
+			return clk;
+	}
+
+	return ERR_PTR(-ENODEV);
+}
+
+struct v4l2_clk *v4l2_clk_get(struct device *dev, const char *id)
+{
+	struct v4l2_clk *clk;
+
+	mutex_lock(&clk_lock);
+	clk = v4l2_clk_find(dev_name(dev), id);
+
+	if (!IS_ERR(clk) && !try_module_get(clk->ops->owner))
+		clk = ERR_PTR(-ENODEV);
+	mutex_unlock(&clk_lock);
+
+	if (!IS_ERR(clk))
+		atomic_inc(&clk->use_count);
+
+	return clk;
+}
+EXPORT_SYMBOL(v4l2_clk_get);
+
+void v4l2_clk_put(struct v4l2_clk *clk)
+{
+	if (!IS_ERR(clk)) {
+		atomic_dec(&clk->use_count);
+		module_put(clk->ops->owner);
+	}
+}
+EXPORT_SYMBOL(v4l2_clk_put);
+
+int v4l2_clk_enable(struct v4l2_clk *clk)
+{
+	int ret;
+	mutex_lock(&clk->lock);
+	if (++clk->enable == 1 && clk->ops->enable) {
+		ret = clk->ops->enable(clk);
+		if (ret < 0)
+			clk->enable--;
+	} else {
+		ret = 0;
+	}
+	mutex_unlock(&clk->lock);
+	return ret;
+}
+EXPORT_SYMBOL(v4l2_clk_enable);
+
+void v4l2_clk_disable(struct v4l2_clk *clk)
+{
+	int enable;
+
+	mutex_lock(&clk->lock);
+	enable = --clk->enable;
+	if (WARN(enable < 0, "Unbalanced %s() on %s:%s!\n", __func__,
+		 clk->dev_id, clk->id))
+		clk->enable++;
+	else if (!enable && clk->ops->disable)
+		clk->ops->disable(clk);
+	mutex_unlock(&clk->lock);
+}
+EXPORT_SYMBOL(v4l2_clk_disable);
+
+unsigned long v4l2_clk_get_rate(struct v4l2_clk *clk)
+{
+	if (!clk->ops->get_rate)
+		return -ENOSYS;
+
+	return clk->ops->get_rate(clk);
+}
+EXPORT_SYMBOL(v4l2_clk_get_rate);
+
+int v4l2_clk_set_rate(struct v4l2_clk *clk, unsigned long rate)
+{
+	if (!clk->ops->set_rate)
+		return -ENOSYS;
+
+	return clk->ops->set_rate(clk, rate);
+}
+EXPORT_SYMBOL(v4l2_clk_set_rate);
+
+struct v4l2_clk *v4l2_clk_register(const struct v4l2_clk_ops *ops,
+				   const char *dev_id,
+				   const char *id, void *priv)
+{
+	struct v4l2_clk *clk;
+	int ret;
+
+	if (!ops || !dev_id)
+		return ERR_PTR(-EINVAL);
+
+	clk = kzalloc(sizeof(struct v4l2_clk), GFP_KERNEL);
+	if (!clk)
+		return ERR_PTR(-ENOMEM);
+
+	clk->id = kstrdup(id, GFP_KERNEL);
+	clk->dev_id = kstrdup(dev_id, GFP_KERNEL);
+	if ((id && !clk->id) || !clk->dev_id) {
+		ret = -ENOMEM;
+		goto ealloc;
+	}
+	clk->ops = ops;
+	clk->priv = priv;
+	atomic_set(&clk->use_count, 0);
+	mutex_init(&clk->lock);
+
+	mutex_lock(&clk_lock);
+	if (!IS_ERR(v4l2_clk_find(dev_id, id))) {
+		mutex_unlock(&clk_lock);
+		ret = -EEXIST;
+		goto eexist;
+	}
+	list_add_tail(&clk->list, &clk_list);
+	mutex_unlock(&clk_lock);
+
+	return clk;
+
+eexist:
+ealloc:
+	kfree(clk->id);
+	kfree(clk->dev_id);
+	kfree(clk);
+	return ERR_PTR(ret);
+}
+EXPORT_SYMBOL(v4l2_clk_register);
+
+void v4l2_clk_unregister(struct v4l2_clk *clk)
+{
+	if (WARN(atomic_read(&clk->use_count),
+		 "%s(): Refusing to unregister ref-counted %s:%s clock!\n",
+		 __func__, clk->dev_id, clk->id))
+		return;
+
+	mutex_lock(&clk_lock);
+	list_del(&clk->list);
+	mutex_unlock(&clk_lock);
+
+	kfree(clk->id);
+	kfree(clk->dev_id);
+	kfree(clk);
+}
+EXPORT_SYMBOL(v4l2_clk_unregister);
diff --git a/include/media/v4l2-clk.h b/include/media/v4l2-clk.h
new file mode 100644
index 0000000..89efbd7
--- /dev/null
+++ b/include/media/v4l2-clk.h
@@ -0,0 +1,54 @@
+/*
+ * V4L2 clock service
+ *
+ * Copyright (C) 2012-2013, Guennadi Liakhovetski <g.liakhovetski@gmx.de>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * ATTENTION: This is a temporary API and it shall be replaced by the generic
+ * clock API, when the latter becomes widely available.
+ */
+
+#ifndef MEDIA_V4L2_CLK_H
+#define MEDIA_V4L2_CLK_H
+
+#include <linux/atomic.h>
+#include <linux/list.h>
+#include <linux/mutex.h>
+
+struct module;
+struct device;
+
+struct v4l2_clk {
+	struct list_head list;
+	const struct v4l2_clk_ops *ops;
+	const char *dev_id;
+	const char *id;
+	int enable;
+	struct mutex lock; /* Protect the enable count */
+	atomic_t use_count;
+	void *priv;
+};
+
+struct v4l2_clk_ops {
+	struct module	*owner;
+	int		(*enable)(struct v4l2_clk *clk);
+	void		(*disable)(struct v4l2_clk *clk);
+	unsigned long	(*get_rate)(struct v4l2_clk *clk);
+	int		(*set_rate)(struct v4l2_clk *clk, unsigned long);
+};
+
+struct v4l2_clk *v4l2_clk_register(const struct v4l2_clk_ops *ops,
+				   const char *dev_name,
+				   const char *name, void *priv);
+void v4l2_clk_unregister(struct v4l2_clk *clk);
+struct v4l2_clk *v4l2_clk_get(struct device *dev, const char *id);
+void v4l2_clk_put(struct v4l2_clk *clk);
+int v4l2_clk_enable(struct v4l2_clk *clk);
+void v4l2_clk_disable(struct v4l2_clk *clk);
+unsigned long v4l2_clk_get_rate(struct v4l2_clk *clk);
+int v4l2_clk_set_rate(struct v4l2_clk *clk, unsigned long rate);
+
+#endif

-- 
1.7.2.5