Am 20.08.2013 18:34, schrieb Frank Schäfer:
> Am 20.08.2013 15:38, schrieb Laurent Pinchart:
>> Hi Mauro,
>>
>> On Sunday 18 August 2013 12:20:08 Mauro Carvalho Chehab wrote:
>>> Em Sun, 18 Aug 2013 13:40:25 +0200 Frank Schäfer escreveu:
>>>> Am 17.08.2013 12:51, schrieb Guennadi Liakhovetski:
>>>>> Hi Frank,
>>>>> As I mentioned on the list, I'm currently on a holiday, so, replying
>>>>> briefly.
>>>> Sorry, I missed that (can't read all mails on the list).
>>>>
>>>>> Since em28xx is a USB device, I conclude, that it's supplying clock to
>>>>> its components including the ov2640 sensor. So, yes, I think the driver
>>>>> should export a V4L2 clock.
>>>> Ok, so it's mandatory on purpose ?
>>>> I'll take a deeper into the v4l2-clk code and the
>>>> em28xx/ov2640/soc-camera interaction this week.
>>>> Have a nice holiday !
>>> commit 9aea470b399d797e88be08985c489855759c6c60
>>> Author: Guennadi Liakhovetski <g.liakhovetski@xxxxxx>
>>> Date: Fri Dec 21 13:01:55 2012 -0300
>>>
>>> [media] soc-camera: switch I2C subdevice drivers to use v4l2-clk
>>>
>>> Instead of centrally enabling and disabling subdevice master clocks in
>>> soc-camera core, let subdevice drivers do that themselves, using the
>>> V4L2 clock API and soc-camera convenience wrappers.
>>>
>>> Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@xxxxxx>
>>> Acked-by: Hans Verkuil <hans.verkuil@xxxxxxxxx>
>>> Acked-by: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
>>> Signed-off-by: Mauro Carvalho Chehab <mchehab@xxxxxxxxxx>
>>>
>>> (c/c the ones that acked with this broken changeset)
>>>
>>> We need to fix it ASAP or to revert the ov2640 changes, as some em28xx
>>> cameras are currently broken on 3.10.
>>>
>>> I'll also reject other ports to the async API if the drivers are
>>> used outside an embedded driver, as no PC driver currently defines
>>> any clock source. The same applies to regulators.
>>>
>>> Guennadi,
>>>
>>> Next time, please check if the i2c drivers are used outside soc_camera
>>> and apply the fixes where needed, as no regressions are allowed.
>> We definitely need to check all users of our sensor drivers when making such a
>> change. Mistakes happen, so let's fix them.
>>
>> Guennadi is on holidays until the end of this week. Would that be too late to
>> fix the issue (given that 3.10 is already broken) ? The fix shouldn't be too
>> complex, registering a dummy V4L2 clock in the em28xx driver should be enough.
> I would prefer either a) making the clock optional in the senor
> driver(s) or b) implementing a real V4L2 clock.
>
> Reading the soc-camera code, it looks like NULL-pointers for struct
> v4l2_clk are handled correctly. so a) should be pretty simple:
>
> priv->clk = v4l2_clk_get(&client->dev, "mclk");
> - if (IS_ERR(priv->clk)) {
> - ret = PTR_ERR(priv->clk);
> - goto eclkget;
> - }
> + if (IS_ERR(priv->clk))
> + priv->clk = NULL;
>
> Some additional NULL-pointer checks might be necessary, e.g. before
> calling v4l2_clk_put().
Tested and that works.
Patch follows.
> Concerning b): I'm not yet sure if it is really needed/makes sense...
> Who is supposed to configure/enable/disable the clock in a constellation
> like em28xx+ov2640 ?
> For UXGA for example, the clock needs to be switched to 12MHz, while
> 24MHz is used for smaller reolutions.
> But I'm not sure if it is a good idea to let the sensor driver do the
> switch (to define fixed bindings between resoultions and clock frequencies).
> Btw, what if a frequency is requested which isn't supported ? Set the
> clock to the next nearest supported frequency ?
>
> Regards,
> Frank
I tried to implement a v4l2_clk for the em28xx driver (not yet beeing
sure if it really makes sense) and I noticed the following problem:
The ov2640 driver (as well as all other sensor drivers) seems to have
specific expectations for the names of the clock.
The name must me "mclk" and dev_name must be the device name of the i2c
client device.
Is "mclk" supposed to be a clock type ? Wouldn't an enum be a better
choice in this case ?
Anyway, the sensor subdevices are registered using
v4l2_i2c_new_subdev_board(), which sets up an i2c client, loads the
module and returns v4l2_subdev.
The v4l2_clock needs to be registered before (otherwise no clock is
found during sensor probing), but at this point the device name of the
i2c_client isn't known yet...
Regards,
Frank
>
>>
>> v4l2-clk.c should provide a helper function to do so as that will be a pretty
>> common operation.
>>
--
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
