Hi Mauro,
On Tuesday 03 March 2015 20:21:29 Mauro Carvalho Chehab wrote:
> Em Wed, 04 Mar 2015 00:56:18 +0200 Laurent Pinchart escreveu:
> > On Tuesday 03 March 2015 13:40:50 Mauro Carvalho Chehab wrote:
> >> Em Mon, 02 Mar 2015 20:52:41 +0000 Laurent Pinchart escreveu:
> >>> On Mon Mar 02 2015 18:55:23 GMT+0200 (EET), Mauro Carvalho Chehab wrote:
> >>>> Em Sun, 1 Feb 2015 12:12:33 +0100 (CET) Guennadi Liakhovetski escreveu:
> >>>>> V4L2 clocks, e.g. used by camera sensors for their master clock, do
> >>>>> not have to be supplied by a different V4L2 driver, they can also be
> >>>>> supplied by an independent source. In this case the standart kernel
> >>>>> clock API should be used to handle such clocks. This patch adds
> >>>>> support for such cases.
> >>>>>
> >>>>> Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@xxxxxx>
> >>>>> Acked-by: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
> >>>>> ---
> >>>>>
> >>>>> v4: sizeof(*clk) :)
> >>>>>
> >>>>> drivers/media/v4l2-core/v4l2-clk.c | 48 ++++++++++++++++++++++++---
> >>>>> include/media/v4l2-clk.h | 2 ++
> >>>>> 2 files changed, 47 insertions(+), 3 deletions(-)
> >>>>>
> >>>>> diff --git a/drivers/media/v4l2-core/v4l2-clk.c
> >>>>> b/drivers/media/v4l2-core/v4l2-clk.c index 3ff0b00..9f8cb20 100644
> >>>>> --- a/drivers/media/v4l2-core/v4l2-clk.c
> >>>>> +++ b/drivers/media/v4l2-core/v4l2-clk.c
> >
> > [snip]
> >
> >>>>> @@ -37,6 +38,21 @@ static struct v4l2_clk *v4l2_clk_find(const char
> >>>>> *dev_id)
> >>>>> struct v4l2_clk *v4l2_clk_get(struct device *dev, const char *id)
> >>>>> {
> >>>>> struct v4l2_clk *clk;
> >>>>> + struct clk *ccf_clk = clk_get(dev, id);
> >>>>> +
> >>>>> + if (PTR_ERR(ccf_clk) == -EPROBE_DEFER)
> >>>>> + return ERR_PTR(-EPROBE_DEFER);
> >>>>
> >>>> Why not do just:
> >>>> return ccf_clk;
> >>>
> >>> I find the explicit error slightly more readable, but that's a matter
> >>> of taste.
> >>
> >> Well, return(ccf_clk) will likely produce a smaller instruction code
> >> than return (long).
> >
> > Not if the compiler is smart :-)
> >
> >>>>> +
> >>>>> + if (!IS_ERR_OR_NULL(ccf_clk)) {
> >>>>> + clk = kzalloc(sizeof(*clk), GFP_KERNEL);
> >>>>> + if (!clk) {
> >>>>> + clk_put(ccf_clk);
> >>>>> + return ERR_PTR(-ENOMEM);
> >>>>> + }
> >>>>> + clk->clk = ccf_clk;
> >>>>> +
> >>>>> + return clk;
> >>>>> + }
> >>>>
> >>>> The error condition here looks a little weird to me. I mean, if the
> >>>> CCF clock returns an error, shouldn't it fail instead of silently
> >>>> run some logic to find another clock source? Isn't it risky on
> >>>> getting a wrong value?
> >>>
> >>> The idea is that, in the long term, everything should use CCF
> >>> directly. However, we have clock providers on platforms where CCF
> >>> isn't avalaible. V4L2 clock has been introduced as a single API
> >>> usable by V4L2 clock users allowing them to retrieve and use clocks
> >>> regardless of whether the provider uses CCF or not. Internally it
> >>> first tries CCF, and then falls back to the non-CCF implementation in
> >>> case of failure.
> >>
> >> Yeah, I got that the non-CCF is a fallback code, to be used on
> >> platforms that CCF isn't available.
> >>
> >> However, the above code doesn't seem to look if CCF is available
> >> or not. Instead, it assumes that *all* error codes, or even NULL,
> >> means that CCF isn't available.
> >>
> >> Shouldn't it be, instead, either waiting for NULL or for some
> >> specific error code, in order to:
> >>
> >> - return the error code, if CCF is available but getting
> >> the clock failed;
> >>
> >> - run the backward-compat code when CCF is not available.
> >
> > Isn't that pretty much what the code is doing ? If we get a -EPROBE_DEFER
> > error from CCF meaning that the clock is known but not registered yet we
> > return it. Otherwise, if the clock is unknown to CCF, or if CCF is
> > disabled, we fall back.
>
> I didn't check the CCF code, but couldn't it return error codes like
> ENOMEM? What are all the error codes it can return ATM? What, among
> them, can happen when CCF is available?
>
> Also, as the CCF code can be changed, if the intent behavior is to only
> allow EPROBE_DEFER or NULL, if the there is support for CCF, then you
> need to have an explicit comment there to avoid that any newer patches
> to add different error codes.
>
> IMHO, it seems a way better to define a single error code to be
> returned when the platform doesn't support CCF (like -ENOTSUPP),
But that's not how CCF works :-)
Regardless of that, note that the clk_get() call is part of the Linux clock
API, of which CCF is one implementation. clk_get() is implemented by custom
architecture code on platforms where CCF isn't used.
The idea of this patch is to
- return the platform clock if available (clk_get() returns a non-NULL, non-
error pointer)
- defer probing if the platform clock requests so
- fall back to the internal clocks list
I don't see a need for something else.
> and calling the fallback code only in this case. Something like:
>
> if (PTR_ERR(ccf_clk) != -ENOTSUPP)
> return ccf_clk;
>
> /* CCF is not supported. Fall back to the old way */
> k = kzalloc(sizeof(*clk), GFP_KERNEL);
> if (!clk) {
> clk_put(ccf_clk);
> return ERR_PTR(-ENOMEM);
> }
> clk->clk = ccf_clk;
> return clk;
--
Regards,
Laurent Pinchart
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
