Re: [PATCH v2 2/2] [media] s5p-mfc: update MFC v4l2 driver to support MFC6.x

[Arun Kumar K <arun.kk@samsung.com>]

Hi Kyungmin Park,

Thank you for the review.
Please find my comments inline.

On Fri, Jul 6, 2012 at 7:51 PM, Kyungmin Park <kmpark@infradead.org> wrote:
> Hi,
>
> On Fri, Jul 6, 2012 at 11:00 PM, Arun Kumar K <arun.kk@samsung.com> wrote:
>> From: Jeongtae Park <jtp.park@samsung.com>
>>
>> Multi Format Codec 6.x is a hardware video coding acceleration
>> module fount in new Exynos5 SoC series.
>> It is capable of handling a range of video codecs and this driver
>> provides a V4L2 interface for video decoding and encoding.
>>
>> Signed-off-by: Jeongtae Park <jtp.park@samsung.com>
>> Singed-off-by: Janghyuck Kim <janghyuck.kim@samsung.com>
>> Singed-off-by: Jaeryul Oh <jaeryul.oh@samsung.com>
>> Signed-off-by: Naveen Krishna Chatradhi <ch.naveen@samsung.com>
>> Signed-off-by: Arun Kumar K <arun.kk@samsung.com>
>> Cc: Marek Szyprowski <m.szyprowski@samsung.com>
>> Cc: Kamil Debski <k.debski@samsung.com>
>> ---
>>  drivers/media/video/Kconfig                  |   16 +-
>>  drivers/media/video/s5p-mfc/Makefile         |    7 +-
>>  drivers/media/video/s5p-mfc/regs-mfc-v6.h    |  676 ++++++++++
>>  drivers/media/video/s5p-mfc/regs-mfc.h       |   29 +
>>  drivers/media/video/s5p-mfc/s5p_mfc.c        |  163 ++-
>>  drivers/media/video/s5p-mfc/s5p_mfc_cmd.c    |    6 +-
>>  drivers/media/video/s5p-mfc/s5p_mfc_cmd.h    |    3 +
>>  drivers/media/video/s5p-mfc/s5p_mfc_cmd_v6.c |   96 ++
>>  drivers/media/video/s5p-mfc/s5p_mfc_common.h |  123 ++-
>>  drivers/media/video/s5p-mfc/s5p_mfc_ctrl.c   |  160 ++-
>>  drivers/media/video/s5p-mfc/s5p_mfc_ctrl.h   |    1 +
>>  drivers/media/video/s5p-mfc/s5p_mfc_dec.c    |  210 +++-
>>  drivers/media/video/s5p-mfc/s5p_mfc_dec.h    |    1 +
>>  drivers/media/video/s5p-mfc/s5p_mfc_enc.c    |  191 ++--
>>  drivers/media/video/s5p-mfc/s5p_mfc_enc.h    |    1 +
>>  drivers/media/video/s5p-mfc/s5p_mfc_intr.c   |    1 -
>>  drivers/media/video/s5p-mfc/s5p_mfc_opr.c    |  278 +++--
>>  drivers/media/video/s5p-mfc/s5p_mfc_opr.h    |   25 +-
>>  drivers/media/video/s5p-mfc/s5p_mfc_opr_v6.c | 1697 ++++++++++++++++++++++++++
>>  drivers/media/video/s5p-mfc/s5p_mfc_opr_v6.h |  140 +++
>>  drivers/media/video/s5p-mfc/s5p_mfc_pm.c     |    6 +-
>>  drivers/media/video/s5p-mfc/s5p_mfc_shm.c    |   28 +-
>>  drivers/media/video/s5p-mfc/s5p_mfc_shm.h    |   13 +-
>>  drivers/media/video/v4l2-ctrls.c             |    1 -
>>  24 files changed, 3476 insertions(+), 396 deletions(-)
>
> Doesn't it too big for one patch? Can you split it into several patches?
>

Ok. I will split it in the next patch.

>>  create mode 100644 drivers/media/video/s5p-mfc/regs-mfc-v6.h
>>  create mode 100644 drivers/media/video/s5p-mfc/s5p_mfc_cmd_v6.c
>>  create mode 100644 drivers/media/video/s5p-mfc/s5p_mfc_opr_v6.c
>>  create mode 100644 drivers/media/video/s5p-mfc/s5p_mfc_opr_v6.h
>>
>> diff --git a/drivers/media/video/Kconfig b/drivers/media/video/Kconfig
>> index 99937c9..0d7fe77 100644
>> --- a/drivers/media/video/Kconfig
>> +++ b/drivers/media/video/Kconfig
>> @@ -1198,13 +1198,27 @@ config VIDEO_SAMSUNG_S5P_JPEG
>>           This is a v4l2 driver for Samsung S5P and EXYNOS4 JPEG codec
>>
>>  config VIDEO_SAMSUNG_S5P_MFC
>> +       bool
>> +
>> +config VIDEO_SAMSUNG_S5P_MFC_V5
>>         tristate "Samsung S5P MFC 5.1 Video Codec"
>> -       depends on VIDEO_DEV && VIDEO_V4L2 && PLAT_S5P
>> +       depends on VIDEO_DEV && VIDEO_V4L2 && ARCH_EXYNOS4
>> +       select VIDEO_SAMSUNG_S5P_MFC
>>         select VIDEOBUF2_DMA_CONTIG
>>         default n
>>         help
>>             MFC 5.1 driver for V4L2.
>>
>> +config VIDEO_SAMSUNG_S5P_MFC_V6
>
> Yes, I know it's exynos5 series features. however, it's not good idea
> to add new config.
> It already handled platform device with proper name.
> e.g., s5p-mfc-v5, s5p-mfc-v6 and handle it with platform data.
>

Ok. Code changes are required for compiling both v5 and v6 together.
Will incorporate in next patch.

[snip]

>> -#define MFC_CLKNAME            "sclk_mfc"
>> +#if defined(CONFIG_VIDEO_SAMSUNG_S5P_MFC_V5)
>> +#define MFC_CLKNAME            "sclk_mfc"
>> +#elif defined(CONFIG_VIDEO_SAMSUNG_S5P_MFC_V6)
>> +#define MFC_CLKNAME            "aclk_333"
> I think it can handle clkname without new config.
>

Yes I will change it.

Regards
Arun