Hi Seuni,

thanks for you review!

On 10/11/2010 07:09 AM, 박세운 wrote:
> Sewoon Park wrote:
> 
>> Add MIPI CSI format definitions, prepare DMA address
>> definitions for interlaced input frame mode.
>>
>> Signed-off-by: Sylwester Nawrocki <s.nawro...@samsung.com>
>> Signed-off-by: Kyungmin Park <kyungmin.p...@samsung.com>
>> ---
>>  drivers/media/video/s5p-fimc/fimc-reg.c  |    6 +-
>>  drivers/media/video/s5p-fimc/regs-fimc.h |   61 ++++++++++++-------------
> -
>> ---
>>  2 files changed, 28 insertions(+), 39 deletions(-)
>>
>> diff --git a/drivers/media/video/s5p-fimc/fimc-reg.c
>> b/drivers/media/video/s5p-fimc/fimc-reg.c
>> index 5570f1c..70f29c5 100644
>> --- a/drivers/media/video/s5p-fimc/fimc-reg.c
>> +++ b/drivers/media/video/s5p-fimc/fimc-reg.c
>> @@ -507,9 +507,9 @@ void fimc_hw_set_input_addr(struct fimc_dev *dev,
>> struct fimc_addr *paddr)
>>      cfg |= S5P_CIREAL_ISIZE_ADDR_CH_DIS;
>>      writel(cfg, dev->regs + S5P_CIREAL_ISIZE);
>>
>> -    writel(paddr->y, dev->regs + S5P_CIIYSA0);
>> -    writel(paddr->cb, dev->regs + S5P_CIICBSA0);
>> -    writel(paddr->cr, dev->regs + S5P_CIICRSA0);
>> +    writel(paddr->y, dev->regs + S5P_CIIYSA(0));
>> +    writel(paddr->cb, dev->regs + S5P_CIICBSA(0));
>> +    writel(paddr->cr, dev->regs + S5P_CIICRSA(0));
>>
>>      cfg &= ~S5P_CIREAL_ISIZE_ADDR_CH_DIS;
>>      writel(cfg, dev->regs + S5P_CIREAL_ISIZE);
>> diff --git a/drivers/media/video/s5p-fimc/regs-fimc.h
>> b/drivers/media/video/s5p-fimc/regs-fimc.h
>> index a3cfe82..9e83315 100644
>> --- a/drivers/media/video/s5p-fimc/regs-fimc.h
>> +++ b/drivers/media/video/s5p-fimc/regs-fimc.h
>> @@ -11,10 +11,6 @@
>>  #ifndef REGS_FIMC_H_
>>  #define REGS_FIMC_H_
>>
>> -#define S5P_CIOYSA(__x)                     (0x18 + (__x) * 4)
>> -#define S5P_CIOCBSA(__x)            (0x28 + (__x) * 4)
>> -#define S5P_CIOCRSA(__x)            (0x38 + (__x) * 4)
>> -
>>  /* Input source format */
>>  #define S5P_CISRCFMT                        0x00
>>  #define S5P_CISRCFMT_ITU601_8BIT    (1 << 31)
>> @@ -28,22 +24,21 @@
>>
>>  /* Window offset */
>>  #define S5P_CIWDOFST                        0x04
>> -#define S5P_CIWDOFST_WINOFSEN               (1 << 31)
>> +#define S5P_CIWDOFST_OFF_EN         (1 << 31)
>>  #define S5P_CIWDOFST_CLROVFIY               (1 << 30)
>>  #define S5P_CIWDOFST_CLROVRLB               (1 << 29)
>> -#define S5P_CIWDOFST_WINHOROFST_MASK        (0x7ff << 16)
>> +#define S5P_CIWDOFST_HOROFF_MASK    (0x7ff << 16)
>>  #define S5P_CIWDOFST_CLROVFICB              (1 << 15)
>>  #define S5P_CIWDOFST_CLROVFICR              (1 << 14)
>> -#define S5P_CIWDOFST_WINHOROFST(x)  ((x) << 16)
>> -#define S5P_CIWDOFST_WINVEROFST(x)  ((x) << 0)
>> -#define S5P_CIWDOFST_WINVEROFST_MASK        (0xfff << 0)
>> +#define S5P_CIWDOFST_HOROFF(x)              ((x) << 16)
>> +#define S5P_CIWDOFST_VEROFF(x)              ((x) << 0)
>> +#define S5P_CIWDOFST_VEROFF_MASK    (0xfff << 0)
>>
>>  /* Global control */
>>  #define S5P_CIGCTRL                 0x08
>>  #define S5P_CIGCTRL_SWRST           (1 << 31)
>>  #define S5P_CIGCTRL_CAMRST_A                (1 << 30)
>>  #define S5P_CIGCTRL_SELCAM_ITU_A    (1 << 29)
>> -#define S5P_CIGCTRL_SELCAM_ITU_MASK (1 << 29)
>>  #define S5P_CIGCTRL_TESTPAT_NORMAL  (0 << 27)
>>  #define S5P_CIGCTRL_TESTPAT_COLOR_BAR       (1 << 27)
>>  #define S5P_CIGCTRL_TESTPAT_HOR_INC (2 << 27)
>> @@ -61,6 +56,8 @@
>>  #define S5P_CIGCTRL_SHDW_DISABLE    (1 << 12)
>>  #define S5P_CIGCTRL_SELCAM_MIPI_A   (1 << 7)
>>  #define S5P_CIGCTRL_CAMIF_SELWB             (1 << 6)
>> +/* 0 - ITU601; 1 - ITU709 */
>> +#define S5P_CIGCTRL_CSC_ITU601_709  (1 << 5)
>>  #define S5P_CIGCTRL_INVPOLHSYNC             (1 << 4)
>>  #define S5P_CIGCTRL_SELCAM_MIPI             (1 << 3)
>>  #define S5P_CIGCTRL_INTERLACE               (1 << 0)
>> @@ -72,23 +69,10 @@
>>  #define S5P_CIWDOFST2_HOROFF(x)             ((x) << 16)
>>  #define S5P_CIWDOFST2_VEROFF(x)             ((x) << 0)
>>
>> -/* Output DMA Y plane start address */
>> -#define S5P_CIOYSA1                 0x18
>> -#define S5P_CIOYSA2                 0x1c
>> -#define S5P_CIOYSA3                 0x20
>> -#define S5P_CIOYSA4                 0x24
>> -
>> -/* Output DMA Cb plane start address */
>> -#define S5P_CIOCBSA1                        0x28
>> -#define S5P_CIOCBSA2                        0x2c
>> -#define S5P_CIOCBSA3                        0x30
>> -#define S5P_CIOCBSA4                        0x34
>> -
>> -/* Output DMA Cr plane start address */
>> -#define S5P_CIOCRSA1                        0x38
>> -#define S5P_CIOCRSA2                        0x3c
>> -#define S5P_CIOCRSA3                        0x40
>> -#define S5P_CIOCRSA4                        0x44
>> +/* Output DMA Y/Cb/Cr plane start addresses */
>> +#define S5P_CIOYSA(n)                       (0x18 + (n) * 4)
>> +#define S5P_CIOCBSA(n)                      (0x28 + (n) * 4)
>> +#define S5P_CIOCRSA(n)                      (0x38 + (n) * 4)
>>
> As you know, S5P series have S5PC210 also.
> Then,
> Why don't you add S5PC210's 32 registers for Output DMA start address some
> other time?
> For the next, the better approach would be to do something like:
> 
> #define S5P_CIOYSA1   0x18    /* Y 1st frame start address for output DMA
> */
> #define S5P_CIOYSA5 0x200 /* Y 5th frame start address for output DMA */
> #define S5P_CIOYSA(n) \
>         (((n) < 4) ?     \
>          (S5P_CIOYSA1  + (n) * 4) : (S5P_CIOYSA5  + ((n) - 4) * 4))

Thanks for pointing this out. I was aware of that and was going to handle
S5PC210 (S5PV310) wariant in another separate patch.
Your proposal looks OK but I am still working on how to use the extended
capabilities of S5PC210 and for now I decided to just mask out the additional
registers at offset 0x200 and above. This is for backward compatibility
and also I do not see much advantage from using all 32 registers just
an overhead of setting them up. So for now let us leave the CIO*SA(n)
definition as is and after I work out a relevant output DMA usage scheme
I will change it, possibly in the way you suggested.

Regards,
Sylwester

> 
[snip]
> 
> 
> I will review another patches soon.

> Thanks.
> 
> Best regards,
> Seuni.
> --
> Sewoon Park <seuni.p...@samsung.com>, Engineer,
> SW Solution Development Team, Samsung Electronics Co., Ltd.
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-media" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

-- 
Sylwester Nawrocki
Linux Platform Group
Samsung Poland R&D Center
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Reply via email to