Hi Sekhar,
On Friday 17 August 2012 11:07 AM, Sekhar Nori wrote:
> On 8/17/2012 10:51 AM, Prabhakar Lad wrote:
>> Hi Laurent,
>>
>> Thanks for the review.
>>
>> On Thursday 16 August 2012 08:43 PM, Laurent Pinchart wrote:
>>> Hi Prabhakar,
>>>
>>> Thanks for the patch.
>>>
>>> On Thursday 16 August 2012 19:32:00 Prabhakar Lad wrote:
>>>> From: Lad, Prabhakar <prabhakar.lad@xxxxxx>
>>>>
>>>> Signed-off-by: Lad, Prabhakar <prabhakar.lad@xxxxxx>
>>>> Signed-off-by: Manjunath Hadli <manjunath.hadli@xxxxxx>
>>>> Cc: Hans Verkuil <hans.verkuil@xxxxxxxxx>
>>>> ---
>>>> drivers/media/video/davinci/vpif_capture.c | 12 +++++++-----
>>>> drivers/media/video/davinci/vpif_display.c | 14 ++++++++------
>>>> 2 files changed, 15 insertions(+), 11 deletions(-)
>>>>
>>>> diff --git a/drivers/media/video/davinci/vpif_capture.c
>>>> b/drivers/media/video/davinci/vpif_capture.c index 266025e..a87b7a5 100644
>>>> --- a/drivers/media/video/davinci/vpif_capture.c
>>>> +++ b/drivers/media/video/davinci/vpif_capture.c
>>>> @@ -311,12 +311,14 @@ static int vpif_start_streaming(struct vb2_queue *vq,
>>>> unsigned int count) }
>>>>
>>>> /* configure 1 or 2 channel mode */
>>>> - ret = vpif_config_data->setup_input_channel_mode
>>>> - (vpif->std_info.ycmux_mode);
>>>> + if (vpif_config_data->setup_input_channel_mode) {
>>>> + ret = vpif_config_data->setup_input_channel_mode
>>>> + (vpif->std_info.ycmux_mode);
>>>>
>>>> - if (ret < 0) {
>>>> - vpif_dbg(1, debug, "can't set vpif channel mode\n");
>>>> - return ret;
>>>> + if (ret < 0) {
>>>> + vpif_dbg(1, debug, "can't set vpif channel mode\n");
>>>> + return ret;
>>>> + }
>>>
>>> This change looks good to me. However, note that you will need to get rid of
>>> board code callbacks at some point to implement device tree support. It would
>>> be worth thinking about how to do so now.
>>>
>> Currently VPIF driver is only used by dm646x, and the handlers for this
>> in the the board code are not null. This patch is intended for da850
>> where this handlers will be null.
>>
>>>> }
>>>>
>>>> /* Call vpif_set_params function to set the parameters and addresses */
>>>> diff --git a/drivers/media/video/davinci/vpif_display.c
>>>> b/drivers/media/video/davinci/vpif_display.c index e129c98..1e35f92 100644
>>>> --- a/drivers/media/video/davinci/vpif_display.c
>>>> +++ b/drivers/media/video/davinci/vpif_display.c
>>>> @@ -280,12 +280,14 @@ static int vpif_start_streaming(struct vb2_queue *vq,
>>>> unsigned int count) }
>>>>
>>>> /* clock settings */
>>>> - ret =
>>>> - vpif_config_data->set_clock(ch->vpifparams.std_info.ycmux_mode,
>>>> - ch->vpifparams.std_info.hd_sd);
>>>> - if (ret < 0) {
>>>> - vpif_err("can't set clock\n");
>>>> - return ret;
>>>> + if (vpif_config_data->set_clock) {
>>>
>>> Does the DaVinci platform use the common clock framework ? If so, a better fix
>>> for this would be to pass a clock name through platform data instead of using
>>> a callback function.
>>>
>> Currently DaVinci is not using the common clock framework.
>>
>> Can you ACK this patch?
>
> Yes, DaVinci has not migrated to common clock framework (yet). However,
> even without that it should be possible to use clock API in driver code.
> Using a callback to enable clocks or even passing the clock name from
> platform data would be bypassing an existing framework. Clock name
> should be IP specific, so it should be possible to use that in driver.
>
The callback is not actually dealing with PSC clock's but with system
module registers.
Thx,
--Prabhakar
> Thanks,
> Sekhar
>
--
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
