[PATCH 1/5 v4] V4L/DVB: s5p-fimc: Register definition cleanup
Sylwester Nawrocki
s.nawrocki at samsung.com
Mon Oct 11 09:44:26 EDT 2010
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.nawrocki at samsung.com>
>> Signed-off-by: Kyungmin Park <kyungmin.park at 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.park at 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 majordomo at vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
--
Sylwester Nawrocki
Linux Platform Group
Samsung Poland R&D Center
More information about the linux-arm-kernel
mailing list