RE: [PATCH] [media] at91: add Atmel Image Sensor Interface (ISI)support
Hi, Guennadi Sorry to answer the question so later, From: Guennadi Liakhovetski Sent: Thursday, May 12, 2011 5:32 PM > On Thu, 12 May 2011, Wu, Josh wrote: >> Hi, Russell >> >> From: Russell King - ARM Linux [mailto:li...@arm.linux.org.uk] Sent: >> Thursday, May 12, 2011 3:47 PM >> > On Thu, May 12, 2011 at 03:42:18PM +0800, Josh Wu wrote: >> >> +err_alloc_isi: >> >> + clk_disable(pclk); >> > clk_put() ? >> Ok, will be fixed in V2 patch. Thanks. > You might wait with v2 until I find time to review your patch. Will take a > couple of days, I think. A general question, though: how compatible is > this driver with the AVR32? Now from the information I got from AVR team, The AVR32 is not compatible with ISI driver patch v2. Since AVR32 use the previous version of ISI IP core. (AT91SAM9M10 using ISI_V2 IP, which has different hardware registers from V1 IP core, such like dma control, etc) But I think in future we will add ISI V1 IP support since AT91SAM9263 also using ISI V1 IP core. Best Regards, Josh Wu -- 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
Re: [PATCH] [media] at91: add Atmel Image Sensor Interface (ISI) support
On 08:58 Wed 18 May , Ryan Mallon wrote:
> On 05/17/2011 08:59 PM, Wu, Josh wrote:
> >
> > On Friday, May 13, 2011 5:25 AM, Ryan Mallon wrote
> >
> >> On 05/12/2011 07:42 PM, Josh Wu wrote:
> >>> This patch is to enable Atmel Image Sensor Interface (ISI) driver
> >>> support.
> >>> - Using soc-camera framework with videobuf2 dma-contig allocator
> >>> - Supporting video streaming of YUV packed format
> >>> - Tested on AT91SAM9M10G45-EK with OV2640
> >
> >> Hi Josh,
> >
> >> Thansk for doing this. Overall the patch looks really good. A few
> >> comments below.
> > Hi, Ryan
> >
> > Thank you for the comments.
> >
> >>>
> >>> Signed-off-by: Josh Wu
> >>> ---
> >>> base on branch staging/for_v2.6.40
> >>>
> >>> arch/arm/mach-at91/include/mach/at91_isi.h | 454
> >>> drivers/media/video/Kconfig| 10 +
> >>> drivers/media/video/Makefile |1 +
> >>> drivers/media/video/atmel-isi.c| 1089
> >>>
> >>> 4 files changed, 1554 insertions(+), 0 deletions(-)
> >>> create mode 100644 arch/arm/mach-at91/include/mach/at91_isi.h
> >>> create mode 100644 drivers/media/video/atmel-isi.c
> >
> >>> [snip]
> >>> +
> >>> +/* Bit manipulation macros */
> >>> +#define ISI_BIT(name)\
> >>> + (1 << ISI_##name##_OFFSET)
> >>> +#define ISI_BF(name, value) \
> >>> + (((value) & ((1 << ISI_##name##_SIZE) - 1)) \
> >>> + << ISI_##name##_OFFSET)
> >>> +#define ISI_BFEXT(name, value) \
> >>> + (((value) >> ISI_##name##_OFFSET) \
> >>> + & ((1 << ISI_##name##_SIZE) - 1))
> >>> +#define ISI_BFINS(name, value, old) \
> >>> + (((old) & ~(((1 << ISI_##name##_SIZE) - 1) \
> >>> + << ISI_##name##_OFFSET))\
> >>> + | ISI_BF(name, value))
> >
> >> I really dislike this kind of register access magic. Not sure how others
> >> feel about it. These macros are really ugly.
> > I understand this. So I will try to find a better way (static inline
> > function) to solve this. :)
>
> >>> +/* Register access macros */
> >>> +#define isi_readl(port, reg) \
> >>> + __raw_readl((port)->regs + ISI_##reg)
> >>> +#define isi_writel(port, reg, value) \
> >>> + __raw_writel((value), (port)->regs + ISI_##reg)
>
> >> If the token pasting stuff gets dropped then these can be static inline
> >> functions which is preferred.
> > sure, I'll try.
>
> Something like this is pretty common (should be moved into the .c file):
>
> static inline unsigned atmel_isi_readl(struct atmel_isi *isi,
>unsigned reg)
> {
> return readl(isi->regs + reg);
> }
>
> static inline void atmel_isi_writel(struct atmel_isi *isi,
> unsigned reg, unsigned val)
> {
> writel(val, isi->regs + reg);
> }
really do not like it
and prefer the first implemetation
NACK for me
>
> Then for single bit values you can just do:
>
> #define ISI_REG_CR0x
> #define ISI_CR_GRAYSCALE (1 << 13)
>
> cr = isi_readl(isi, ISI_REG_CR);
> cr |= ISI_CR_GRAYSCALE;
> isi_writel(isi, ISI_REG_CR, cr);
>
> For bit-fields you could do something like:
>
> static void atmel_isi_set_bitfield(struct atmel_isi *isi, unsigned reg,
> unsigned offset, unsigned mask,
> unsigned val)
> {
> unsigned tmp;
>
> tmp = atmel_isi_readl(isi, reg);
> tmp &= ~(mask << offset);
> tmp |= (val & mask) << offset;
> atmel_isi_writel(isi, reg, tmp);
> }
>
stop to reinvent thinks
use the bitops of the kernel
> #define ISI_V2_VCC_SWAP_OFFSET28
> #define ISI_V2_VCC_SWAP_MASK 0x3
>
> atmel_isi_set_bitfield(isi, ISI_REG_CR, ISI_V2_VCC_SWAP_OFFSET,
> ISI_V2_SWAP_MASK, 2);
>
> There are only a handful of bit-field accesses in the driver so I don't
> think this will make the driver much more verbose and it will remove a
> number of _SIZE definitions for the single bit values.
>
>
>
> >>> diff --git a/drivers/media/video/Kconfig b/drivers/media/video/Kconfig
> >>> index d61414e..eae6005 100644
> >>> --- a/drivers/media/video/Kconfig
> >>> +++ b/drivers/media/video/Kconfig
> >>> @@ -80,6 +80,16 @@ menuconfig VIDEO_CAPTURE_DRIVERS
> >>> Some of those devices also supports FM radio.
> >>>
> >>> if VIDEO_CAPTURE_DRIVERS && VIDEO_V4L2
> >>> +config VIDEO_ATMEL_ISI
> >>> + tristate "ATMEL Image Sensor Interface (ISI) support"
> >>> + depends on VIDEO_DEV && SOC_CAMERA
> >
> >> Depends on AT91/AVR32?
> > I think I will use AT91
>
> Somebody else suggested leaving out the AT91 dependency to allow better
> build coverage. The reason for having the AT91 dependency is so that it
> doesn't show up in menuconfig for people on other platforms and
> architectures who cannot use the driver. I've a
Re: [PATCH] [media] at91: add Atmel Image Sensor Interface (ISI) support
On 05/17/2011 08:59 PM, Wu, Josh wrote:
>
> On Friday, May 13, 2011 5:25 AM, Ryan Mallon wrote
>
>> On 05/12/2011 07:42 PM, Josh Wu wrote:
>>> This patch is to enable Atmel Image Sensor Interface (ISI) driver support.
>>> - Using soc-camera framework with videobuf2 dma-contig allocator
>>> - Supporting video streaming of YUV packed format
>>> - Tested on AT91SAM9M10G45-EK with OV2640
>
>> Hi Josh,
>
>> Thansk for doing this. Overall the patch looks really good. A few
>> comments below.
> Hi, Ryan
>
> Thank you for the comments.
>
>>>
>>> Signed-off-by: Josh Wu
>>> ---
>>> base on branch staging/for_v2.6.40
>>>
>>> arch/arm/mach-at91/include/mach/at91_isi.h | 454
>>> drivers/media/video/Kconfig| 10 +
>>> drivers/media/video/Makefile |1 +
>>> drivers/media/video/atmel-isi.c| 1089
>>>
>>> 4 files changed, 1554 insertions(+), 0 deletions(-)
>>> create mode 100644 arch/arm/mach-at91/include/mach/at91_isi.h
>>> create mode 100644 drivers/media/video/atmel-isi.c
>
>>> [snip]
>>> +
>>> +/* Bit manipulation macros */
>>> +#define ISI_BIT(name) \
>>> + (1 << ISI_##name##_OFFSET)
>>> +#define ISI_BF(name, value)\
>>> + (((value) & ((1 << ISI_##name##_SIZE) - 1)) \
>>> +<< ISI_##name##_OFFSET)
>>> +#define ISI_BFEXT(name, value) \
>>> + (((value) >> ISI_##name##_OFFSET) \
>>> +& ((1 << ISI_##name##_SIZE) - 1))
>>> +#define ISI_BFINS(name, value, old)\
>>> + (((old) & ~(((1 << ISI_##name##_SIZE) - 1) \
>>> + << ISI_##name##_OFFSET))\
>>> +| ISI_BF(name, value))
>
>> I really dislike this kind of register access magic. Not sure how others
>> feel about it. These macros are really ugly.
> I understand this. So I will try to find a better way (static inline
> function) to solve this. :)
>>> +/* Register access macros */
>>> +#define isi_readl(port, reg) \
>>> + __raw_readl((port)->regs + ISI_##reg)
>>> +#define isi_writel(port, reg, value) \
>>> + __raw_writel((value), (port)->regs + ISI_##reg)
>> If the token pasting stuff gets dropped then these can be static inline
>> functions which is preferred.
> sure, I'll try.
Something like this is pretty common (should be moved into the .c file):
static inline unsigned atmel_isi_readl(struct atmel_isi *isi,
unsigned reg)
{
return readl(isi->regs + reg);
}
static inline void atmel_isi_writel(struct atmel_isi *isi,
unsigned reg, unsigned val)
{
writel(val, isi->regs + reg);
}
Then for single bit values you can just do:
#define ISI_REG_CR 0x
#define ISI_CR_GRAYSCALE(1 << 13)
cr = isi_readl(isi, ISI_REG_CR);
cr |= ISI_CR_GRAYSCALE;
isi_writel(isi, ISI_REG_CR, cr);
For bit-fields you could do something like:
static void atmel_isi_set_bitfield(struct atmel_isi *isi, unsigned reg,
unsigned offset, unsigned mask,
unsigned val)
{
unsigned tmp;
tmp = atmel_isi_readl(isi, reg);
tmp &= ~(mask << offset);
tmp |= (val & mask) << offset;
atmel_isi_writel(isi, reg, tmp);
}
#define ISI_V2_VCC_SWAP_OFFSET 28
#define ISI_V2_VCC_SWAP_MASK0x3
atmel_isi_set_bitfield(isi, ISI_REG_CR, ISI_V2_VCC_SWAP_OFFSET,
ISI_V2_SWAP_MASK, 2);
There are only a handful of bit-field accesses in the driver so I don't
think this will make the driver much more verbose and it will remove a
number of _SIZE definitions for the single bit values.
>>> diff --git a/drivers/media/video/Kconfig b/drivers/media/video/Kconfig
>>> index d61414e..eae6005 100644
>>> --- a/drivers/media/video/Kconfig
>>> +++ b/drivers/media/video/Kconfig
>>> @@ -80,6 +80,16 @@ menuconfig VIDEO_CAPTURE_DRIVERS
>>> Some of those devices also supports FM radio.
>>>
>>> if VIDEO_CAPTURE_DRIVERS && VIDEO_V4L2
>>> +config VIDEO_ATMEL_ISI
>>> + tristate "ATMEL Image Sensor Interface (ISI) support"
>>> + depends on VIDEO_DEV && SOC_CAMERA
>
>> Depends on AT91/AVR32?
> I think I will use AT91
Somebody else suggested leaving out the AT91 dependency to allow better
build coverage. The reason for having the AT91 dependency is so that it
doesn't show up in menuconfig for people on other platforms and
architectures who cannot use the driver. I've always made SoC drivers
depend on their architecture. Not sure what the correct answer is here?
>>> [snip]
>>> +
>>> +#include
>>> +#include
>>> +#include
>>> +
>>> +#define ATMEL_ISI_VERSION KERNEL_VERSION(1, 0, 0)
>
>> Do we really need this version?
> Since we need set a version for v4l2_capability.version in function
> isi_camera_querycap(). So we use this macr
RE: [PATCH] [media] at91: add Atmel Image Sensor Interface (ISI) support
On Friday, May 13, 2011 5:25 AM, Ryan Mallon wrote
> On 05/12/2011 07:42 PM, Josh Wu wrote:
>> This patch is to enable Atmel Image Sensor Interface (ISI) driver support.
>> - Using soc-camera framework with videobuf2 dma-contig allocator
>> - Supporting video streaming of YUV packed format
>> - Tested on AT91SAM9M10G45-EK with OV2640
> Hi Josh,
> Thansk for doing this. Overall the patch looks really good. A few
> comments below.
Hi, Ryan
Thank you for the comments.
>>
>> Signed-off-by: Josh Wu
>> ---
>> base on branch staging/for_v2.6.40
>>
>> arch/arm/mach-at91/include/mach/at91_isi.h | 454
>> drivers/media/video/Kconfig| 10 +
>> drivers/media/video/Makefile |1 +
>> drivers/media/video/atmel-isi.c| 1089
>>
>> 4 files changed, 1554 insertions(+), 0 deletions(-)
>> create mode 100644 arch/arm/mach-at91/include/mach/at91_isi.h
>> create mode 100644 drivers/media/video/atmel-isi.c
>> [snip]
>> +
>> +/* Bit manipulation macros */
>> +#define ISI_BIT(name) \
>> +(1 << ISI_##name##_OFFSET)
>> +#define ISI_BF(name, value) \
>> +(((value) & ((1 << ISI_##name##_SIZE) - 1)) \
>> + << ISI_##name##_OFFSET)
>> +#define ISI_BFEXT(name, value) \
>> +(((value) >> ISI_##name##_OFFSET) \
>> + & ((1 << ISI_##name##_SIZE) - 1))
>> +#define ISI_BFINS(name, value, old) \
>> +(((old) & ~(((1 << ISI_##name##_SIZE) - 1) \
>> +<< ISI_##name##_OFFSET))\
>> + | ISI_BF(name, value))
> I really dislike this kind of register access magic. Not sure how others
> feel about it. These macros are really ugly.
I understand this. So I will try to find a better way (static inline function)
to solve this. :)
>> +/* Register access macros */
>> +#define isi_readl(port, reg)\
>> +__raw_readl((port)->regs + ISI_##reg)
>> +#define isi_writel(port, reg, value)\
>> +__raw_writel((value), (port)->regs + ISI_##reg)
> If the token pasting stuff gets dropped then these can be static inline
> functions which is preferred.
sure, I'll try.
>> [snip]
>> +#define ISI_GS_2PIX_PER_WORD0x00
>> +#define ISI_GS_1PIX_PER_WORD0x01
>> +u8 pixfmt;
>> +u8 sfd;
>> +u8 sld;
>> +u8 thmask;
>> +#define ISI_BURST_4_8_160x00
>> +#define ISI_BURST_8_16 0x01
>> +#define ISI_BURST_160x02
>> +u8 frate;
>> +#define ISI_FRATE_DIV_2 0x01
>> +#define ISI_FRATE_DIV_3 0x02
>> +#define ISI_FRATE_DIV_4 0x03
>> +#define ISI_FRATE_DIV_5 0x04
>> +#define ISI_FRATE_DIV_6 0x05
>> +#define ISI_FRATE_DIV_7 0x06
>> +#define ISI_FRATE_DIV_8 0x07
>> +};
> Might need some comments in this structure so that board file writers
> know what each of the fields are for.
I think this part will be change a little bit. Also I will add the updated
comments.
>> +
>> +#endif /* __AT91_ISI_H__ */
>> diff --git a/drivers/media/video/Kconfig b/drivers/media/video/Kconfig
>> index d61414e..eae6005 100644
>> --- a/drivers/media/video/Kconfig
>> +++ b/drivers/media/video/Kconfig
>> @@ -80,6 +80,16 @@ menuconfig VIDEO_CAPTURE_DRIVERS
>>Some of those devices also supports FM radio.
>>
>> if VIDEO_CAPTURE_DRIVERS && VIDEO_V4L2
>> +config VIDEO_ATMEL_ISI
>> +tristate "ATMEL Image Sensor Interface (ISI) support"
>> +depends on VIDEO_DEV && SOC_CAMERA
> Depends on AT91/AVR32?
I think I will use AT91
>> [snip]
>> +
>> +#include
>> +#include
>> +#include
>> +
>> +#define ATMEL_ISI_VERSION KERNEL_VERSION(1, 0, 0)
> Do we really need this version?
Since we need set a version for v4l2_capability.version in function
isi_camera_querycap(). So we use this macro.
How about this?
static int isi_camera_querycap(struct soc_camera_host *ici,
struct v4l2_capability *cap)
{
strcpy(cap->driver, "atmel-isi");
strcpy(cap->card, "Atmel Image Sensor Interface");
cap->version = KERNEL_VERSION(1, 0, 0);
cap->capabilities = (V4L2_CAP_VIDEO_CAPTURE |
V4L2_CAP_STREAMING);
return 0;
}
>> +#define MAX_BUFFER_NUMS 32
>> +#define MAX_SUPPORT_WIDTH 2048
>> +#define MAX_SUPPORT_HEIGHT 2048
>> +
>> +static unsigned int vid_limit = 16;
> This never gets changed so it can become a const/define. The value is
> MB, which is not clear from the name, and it gets multiplied out to
> bytes in its only usage, so maybe:
> #define VID_LIMIT_BYTES (16 * 1024 * 1024)
Your code is good. I will change according to your suggestion.
>> +
>> +enum isi_buffer_state {
>> +ISI_BUF_NEEDS_INIT,
>> +ISI_BUF_PREPARED,
>> +};
> Aren't there v4l2 constants for
RE: [PATCH] [media] at91: add Atmel Image Sensor Interface (ISI) support
Hi, JC
>> +struct atmel_isi;
> do we really this here?
Not really. I'll remove this.
>> +
>> [snip]
>>
>> if VIDEO_CAPTURE_DRIVERS && VIDEO_V4L2
>> +config VIDEO_ATMEL_ISI
>> +tristate "ATMEL Image Sensor Interface (ISI) support"
>> +depends on VIDEO_DEV && SOC_CAMERA
> depends on AT91 if the drivers is at91 specific or avr32 otherwise
I'll add that. I think now it is only supported AT91. I am not sure for the
AVR32 part
>> +select VIDEOBUF2_DMA_CONTIG
>> +default n
> it's n by default please remove
I'll change that.
>> [snip]
>> +
>> +/* Frame buffer descriptor
>> + * Used by the ISI module as a linked list for the DMA controller.
>> + */
>> +struct fbd {
>> +/* Physical address of the frame buffer */
>> +u32 fb_address;
>> +#if defined(CONFIG_ARCH_AT91SAM9G45) ||\
>> +defined(CONFIG_ARCH_AT91SAM9X5)
>> +/* DMA Control Register(only in HISI2) */
>> +u32 dma_ctrl;
>> +#endif
> no ifdef in the struct
I'll remove this #if. I think for the non-HISI2 version, like AT91SAM9263, we
should define another FBD structure which not includes dma_ctrl.
>> +/* Physical address of the next fbd */
>> +u32 next_fbd_address;
>> +};
>> +
>> +#if defined(CONFIG_ARCH_AT91SAM9G45) ||\
>> +defined(CONFIG_ARCH_AT91SAM9X5)
>> +static void set_dma_ctrl(struct fbd *fb_desc, u32 ctrl) {
>> +fb_desc->dma_ctrl = ctrl;
>> +}
>> +#else
>> +static void set_dma_ctrl(struct fbd *fb_desc, u32 ctrl) { } #endif
> no ifdef here also as we want to have multi soc support
I'll remove this #if also.
>> [snip]
>> +/* State of the ISI module in capturing mode */
>> +int state;
>> +
>> +/* Capture/streaming wait queue for waiting for SOF */
>> +wait_queue_head_t capture_wq;
>> +
>> +struct v4l2_device v4l2_dev;
>> +
>> +struct vb2_alloc_ctx*alloc_ctx;
>> +
>> +struct clk *pclk;
>> +struct platform_device *pdev;
> do you really need to store the pdev?
I'll remove this. It is no use.
>> +unsigned intirq;
>> +
>> +struct isi_platform_data*pdata;
>> +unsigned long platform_flags;
>> +
>> +struct list_headvideo_buffer_list;
>> +struct frame_buffer *active;
>> +
>> +struct soc_camera_device*icd;
>> +struct soc_camera_host soc_host;
>> +};
>> +
>> +static int configure_geometry(struct atmel_isi *isi, u32 width,
>> +u32 height, enum v4l2_mbus_pixelcode code) {
>> +u32 cfg2, cr, ctrl;
>> +
>> +cr = 0;
> please move this in default
I'll remove this cr line. Seems it is not needed. cr will be initialized by the
following code.
>> [snip]
>> +
>> +size = bytes_per_line * icd->user_height;
>> +
>> +if (0 == *nbuffers)
> please invert the test
I'll fix it.
>> +*nbuffers = MAX_BUFFER_NUMS;
>> +if (*nbuffers > MAX_BUFFER_NUMS)
>> +*nbuffers = MAX_BUFFER_NUMS;
>> +
>> +while (size * *nbuffers > vid_limit * 1024 * 1024)
>> +(*nbuffers)--;
>> +
>> +*nplanes = 1;
>> +sizes[0] = size;
>> +alloc_ctxs[0] = dev->alloc_ctx;
>> +
>> +dev->sequence = 0;
>> +dev->active = NULL;
>> +
>> +dev_dbg(icd->dev.parent, "%s, count=%d, size=%ld\n", __func__,
>> +*nbuffers, size);
>> +
>> +return 0;
>> +}
>> +
>> +
>> +static void start_dma(struct atmel_isi *isi, struct frame_buffer
>> +*buffer) {
>> +u32 ctrl, cfg1;
> please add ine ligne here
OK. I'll change that.
>> +ctrl = isi_readl(isi, V2_CTRL);
>> +cfg1 = isi_readl(isi, V2_CFG1);
>> +/* Enable irq: cxfr for the codec path, pxfr for the preview path */
>> +isi_writel(isi, V2_INTEN,
>> +ISI_BIT(V2_CXFR_DONE) | ISI_BIT(V2_PXFR_DONE));
>> +
>> +/* Enable codec path */
>> +ctrl |= ISI_BIT(V2_CDC);
>> +/* Check if already in a frame */
>> +while (isi_readl(isi, V2_STATUS) & ISI_BIT(V2_CDC))
>> +cpu_relax();
> no timeout?
I'll add time out code and test it.
>> +
>> +/* Write the address of the first frame buffer in the C_ADDR reg
>> +* write the address of the first descriptor(link list of buffer)
>> +* in the C_DSCR reg, and enable dma channel.
>> +*/
>> +isi_writel(isi, V2_DMA_C_DSCR, (__pa(&(buffer->fb_desc;
>> +isi_writel(isi, V2_DMA_C_CTRL,
>> +ISI_BIT(V2_DMA_FETCH) | ISI_BIT(V2_DMA_DONE));
>> +isi_writel(isi, V2_DMA_CHER, ISI_BIT(V2_DMA_C_CH_EN));
>> +
>> +/* Enable linked list */
>> +cfg1 |= ISI_BF(V2_FRATE, isi->pdata->frate) | ISI_BIT(V2_DISCR);
>> +
>> +/* Enable ISI module*/
>> +ctrl |= ISI_BIT(V2_ENABLE);
>> +isi_writel(isi, V2_CTRL, ctrl);
>> +isi_writel(isi, V2_CFG1, cfg1);
>> +}
>> +
>> +
>> +/* abort streaming and wait for last buffer */ static int
>> +stop_streaming(struct vb2_queue *vq) {
>> +struct soc_camera_device *icd = soc_camera_from_vb2q(
Re: [PATCH] [media] at91: add Atmel Image Sensor Interface (ISI) support
On Thursday 12 May 2011, Josh Wu wrote:
> This patch is to enable Atmel Image Sensor Interface (ISI) driver support.
> - Using soc-camera framework with videobuf2 dma-contig allocator
> - Supporting video streaming of YUV packed format
> - Tested on AT91SAM9M10G45-EK with OV2640
>
> Signed-off-by: Josh Wu
This looks like a very well written driver for the most part. I have a few
comments, mainly regarding maintainability that you should probably look
into.
> arch/arm/mach-at91/include/mach/at91_isi.h | 454
> drivers/media/video/Kconfig| 10 +
> drivers/media/video/Makefile |1 +
> drivers/media/video/atmel-isi.c| 1089
>
So you have a mach specific header file that is used by a single driver?
That does not lean itself it code reuse. Better move the header file into
the same directory as the driver, or (better) merge its contents into the
same file.
This is especially important if the driver is possibly shared with avr32
socs.
> +/* Constants for RGB_CFG */
> +#define ISI_RGB_CFG_DEFAULT 0
> +#define ISI_RGB_CFG_MODE_1 1
> +#define ISI_RGB_CFG_MODE_2 2
> +#define ISI_RGB_CFG_MODE_3 3
> +
> +/* Constants for RGB_CFG(ISI_V2) */
> +#define ISI_V2_RGB_CFG_DEFAULT 0
> +#define ISI_V2_RGB_CFG_MODE_11
> +#define ISI_V2_RGB_CFG_MODE_22
> +#define ISI_V2_RGB_CFG_MODE_33
> +
> +/* Bit manipulation macros */
> +#define ISI_BIT(name)\
> + (1 << ISI_##name##_OFFSET)
> +#define ISI_BF(name, value) \
> + (((value) & ((1 << ISI_##name##_SIZE) - 1)) \
> + << ISI_##name##_OFFSET)
> +#define ISI_BFEXT(name, value) \
> + (((value) >> ISI_##name##_OFFSET) \
> + & ((1 << ISI_##name##_SIZE) - 1))
> +#define ISI_BFINS(name, value, old) \
> + (((old) & ~(((1 << ISI_##name##_SIZE) - 1) \
> + << ISI_##name##_OFFSET))\
> + | ISI_BF(name, value))
A much better way to express the above would be to define the constants as
mask values rather than using the macros with bit numbers. There are
conflicting views on how bits are counted, plus the use of macros
makes it harder to grep for the idenfiers.
For example, do
#define ISI_RGB_CFG_DEFAULT 0x0001
#define ISI_RGB_CFG_MODE_1 0x0002
#define ISI_RGB_CFG_MODE_2 0x0004
#define ISI_RGB_CFG_MODE_3 0x0008
For the two examples I quoted, the values are actually the same,
so you might also want to name them so that you don't have to
define multiple versions but can simply reuse the same macros.
Some people also prefer the use of enum over #define, but I
leave that to your judgement
> +/* Register access macros */
> +#define isi_readl(port, reg) \
> + __raw_readl((port)->regs + ISI_##reg)
> +#define isi_writel(port, reg, value) \
> + __raw_writel((value), (port)->regs + ISI_##reg)
Please explain why you use __raw_* instead of the proper
accessors in the comment. Normal drivers should always
use readl/writel.
Better don't concatenate identifiers, in order to make
grep and ctags work on the arguments.
> +
> +#define ATMEL_ISI_VERSIONKERNEL_VERSION(1, 0, 0)
Don't use KERNEL_VERSION() here, it means something else.
Better yet, don't define a version at all. It is not acceptable
to make any user space interface depend on specific versions,
this is a compatibility nightmare.
> +/* Frame buffer descriptor
> + * Used by the ISI module as a linked list for the DMA controller.
> + */
> +struct fbd {
> + /* Physical address of the frame buffer */
> + u32 fb_address;
> +#if defined(CONFIG_ARCH_AT91SAM9G45) ||\
> + defined(CONFIG_ARCH_AT91SAM9X5)
> + /* DMA Control Register(only in HISI2) */
> + u32 dma_ctrl;
> +#endif
> + /* Physical address of the next fbd */
> + u32 next_fbd_address;
> +};
> +
> +#if defined(CONFIG_ARCH_AT91SAM9G45) ||\
> + defined(CONFIG_ARCH_AT91SAM9X5)
> +static void set_dma_ctrl(struct fbd *fb_desc, u32 ctrl)
> +{
> + fb_desc->dma_ctrl = ctrl;
> +}
> +#else
> +static void set_dma_ctrl(struct fbd *fb_desc, u32 ctrl) { }
> +#endif
Make these runtime checks, not compile-time. Best define different
identifiers for the platform device, then key the differences off
the device, not the platform.
> +struct atmel_isi {
> + /* ISI module spin lock. Protects against concurrent access of variables
> + * that are shared with the ISR */
> + spinlock_t lock;
> + void __iomem*regs;
> +
> + /* If set ISI is in still capture mode */
> + int still_capture;
> + int sequence;
> + /* State of the ISI module in capturing mod
Re: [PATCH] [media] at91: add Atmel Image Sensor Interface (ISI) support
On Fri, 13 May 2011, Sylwester Nawrocki wrote: > On 05/13/2011 03:50 PM, Guennadi Liakhovetski wrote: > > On Thu, 12 May 2011, Josh Wu wrote: > > > >> This patch is to enable Atmel Image Sensor Interface (ISI) driver support. > >> - Using soc-camera framework with videobuf2 dma-contig allocator > >> - Supporting video streaming of YUV packed format > >> - Tested on AT91SAM9M10G45-EK with OV2640 > >> > >> Signed-off-by: Josh Wu > >> --- > ... > >> diff --git a/drivers/media/video/Makefile b/drivers/media/video/Makefile > >> index a10e4c3..f734a65 100644 > >> --- a/drivers/media/video/Makefile > >> +++ b/drivers/media/video/Makefile > >> @@ -166,6 +166,7 @@ obj-$(CONFIG_VIDEO_SH_MOBILE_CSI2) += > >> sh_mobile_csi2.o > >> obj-$(CONFIG_VIDEO_SH_MOBILE_CEU)+= sh_mobile_ceu_camera.o > >> obj-$(CONFIG_VIDEO_OMAP1)+= omap1_camera.o > >> obj-$(CONFIG_VIDEO_SAMSUNG_S5P_FIMC) += s5p-fimc/ > > > > [OT] hm, wow, who has decided to put a generic V4L driver (set) in the > > Makefile together with other soc-camera drivers? It has to be converted > > now;) > > In fact I've already converted that driver, but soc-camera wasn't > unfortunately > my choice, just the media controller ;-) I'll probably move the entry when > preparing upstream patches, it just doesn't feel safe here;) Yeah, you're right - it's a boilerplate here, nothing like safe;) Thanks Guennadi --- Guennadi Liakhovetski, Ph.D. Freelance Open-Source Software Developer http://www.open-technology.de/ -- 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
Re: [PATCH] [media] at91: add Atmel Image Sensor Interface (ISI) support
On 05/13/2011 03:50 PM, Guennadi Liakhovetski wrote: > On Thu, 12 May 2011, Josh Wu wrote: > >> This patch is to enable Atmel Image Sensor Interface (ISI) driver support. >> - Using soc-camera framework with videobuf2 dma-contig allocator >> - Supporting video streaming of YUV packed format >> - Tested on AT91SAM9M10G45-EK with OV2640 >> >> Signed-off-by: Josh Wu >> --- ... >> diff --git a/drivers/media/video/Makefile b/drivers/media/video/Makefile >> index a10e4c3..f734a65 100644 >> --- a/drivers/media/video/Makefile >> +++ b/drivers/media/video/Makefile >> @@ -166,6 +166,7 @@ obj-$(CONFIG_VIDEO_SH_MOBILE_CSI2) += >> sh_mobile_csi2.o >> obj-$(CONFIG_VIDEO_SH_MOBILE_CEU) += sh_mobile_ceu_camera.o >> obj-$(CONFIG_VIDEO_OMAP1) += omap1_camera.o >> obj-$(CONFIG_VIDEO_SAMSUNG_S5P_FIMC) += s5p-fimc/ > > [OT] hm, wow, who has decided to put a generic V4L driver (set) in the > Makefile together with other soc-camera drivers? It has to be converted now;) In fact I've already converted that driver, but soc-camera wasn't unfortunately my choice, just the media controller ;-) I'll probably move the entry when preparing upstream patches, it just doesn't feel safe here;) > >> +obj-$(CONFIG_VIDEO_ATMEL_ISI) += atmel-isi.o >> >> obj-$(CONFIG_ARCH_DAVINCI) += davinci/ -- Regards, Sylwester Nawrocki -- 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
Re: [PATCH] [media] at91: add Atmel Image Sensor Interface (ISI) support
On Thu, 12 May 2011, Josh Wu wrote: > This patch is to enable Atmel Image Sensor Interface (ISI) driver support. > - Using soc-camera framework with videobuf2 dma-contig allocator > - Supporting video streaming of YUV packed format > - Tested on AT91SAM9M10G45-EK with OV2640 > > Signed-off-by: Josh Wu > --- > base on branch staging/for_v2.6.40 > > arch/arm/mach-at91/include/mach/at91_isi.h | 454 > drivers/media/video/Kconfig| 10 + > drivers/media/video/Makefile |1 + > drivers/media/video/atmel-isi.c| 1089 > > 4 files changed, 1554 insertions(+), 0 deletions(-) > create mode 100644 arch/arm/mach-at91/include/mach/at91_isi.h > create mode 100644 drivers/media/video/atmel-isi.c > > diff --git a/arch/arm/mach-at91/include/mach/at91_isi.h > b/arch/arm/mach-at91/include/mach/at91_isi.h > new file mode 100644 > index 000..3219358 > --- /dev/null > +++ b/arch/arm/mach-at91/include/mach/at91_isi.h > @@ -0,0 +1,454 @@ > +/* > + * Register definitions for the Atmel Image Sensor Interface. Why do you put ISI register definitions in a header under arch/arm/...? It seems only your driver needs them. Then at most put it directly under drivers/media/video or even consider inlining all these definitions in the driver itself, although, I admit, in such quantities it probably looks cleaner in a separate header. But please put it under drivers/... Any common stuff, that you will need in your board code to configure the driver should go in a header under include/media/. > + * > + * Copyright (C) 2011 Atmel Corporation Consider adding an author with an email address? > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License version 2 as > + * published by the Free Software Foundation. > + */ > +#ifndef __AT91_ISI_H__ > +#define __AT91_ISI_H__ > + > +#include > +#include > +#include > +#include > +#include > +#include None of these headers seems to be needed in the header itself. OTOH, you do use __raw_readl()/writel(), which need something like linux/io.h. But you'll, probably, remove those __raw_* from the header. > + > +/* ISI register offsets */ > +#define ISI_CR1 0x > +#define ISI_CR2 0x0004 > +#define ISI_SR 0x0008 > +#define ISI_IER 0x000c > +#define ISI_IDR 0x0010 > +#define ISI_IMR 0x0014 > +#define ISI_PSIZE0x0020 > +#define ISI_PDECF0x0024 > +#define ISI_PPFBD0x0028 > +#define ISI_CDBA 0x002c > +#define ISI_Y2R_SET0 0x0030 > +#define ISI_Y2R_SET1 0x0034 > +#define ISI_R2Y_SET0 0x0038 > +#define ISI_R2Y_SET1 0x003c > +#define ISI_R2Y_SET2 0x0040 > + > +/* ISI_V2 register offsets */ > +#define ISI_V2_CFG1 0x > +#define ISI_V2_CFG2 0x0004 > +#define ISI_V2_PSIZE 0x0008 > +#define ISI_V2_PDECF 0x000c > +#define ISI_V2_Y2R_SET0 0x0010 > +#define ISI_V2_Y2R_SET1 0x0014 > +#define ISI_V2_R2Y_SET0 0x0018 > +#define ISI_V2_R2Y_SET1 0x001C > +#define ISI_V2_R2Y_SET2 0x0020 > +#define ISI_V2_CTRL 0x0024 > +#define ISI_V2_STATUS0x0028 > +#define ISI_V2_INTEN 0x002C > +#define ISI_V2_INTDIS0x0030 > +#define ISI_V2_INTMASK 0x0034 > +#define ISI_V2_DMA_CHER 0x0038 > +#define ISI_V2_DMA_CHDR 0x003C > +#define ISI_V2_DMA_CHSR 0x0040 > +#define ISI_V2_DMA_P_ADDR0x0044 > +#define ISI_V2_DMA_P_CTRL0x0048 > +#define ISI_V2_DMA_P_DSCR0x004C > +#define ISI_V2_DMA_C_ADDR0x0050 > +#define ISI_V2_DMA_C_CTRL0x0054 > +#define ISI_V2_DMA_C_DSCR0x0058 > + > +/* Bitfields in CR1 */ > +#define ISI_RST_OFFSET 0 > +#define ISI_RST_SIZE 1 > +#define ISI_DIS_OFFSET 1 > +#define ISI_DIS_SIZE 1 > +#define ISI_HSYNC_POL_OFFSET 2 > +#define ISI_HSYNC_POL_SIZE 1 > +#define ISI_VSYNC_POL_OFFSET 3 > +#define ISI_VSYNC_POL_SIZE 1 > +#define ISI_
RE: [PATCH] [media] at91: add Atmel Image Sensor Interface (ISI)support
On Thursday, May 12, 2011 5:35 PM, Russell King wrote:
> A few more points...
>> +static int __init atmel_isi_probe(struct platform_device *pdev)
> Should be __devinit otherwise you'll have section errors.
Ok, will be fixed in V2 patch.
>> +{
>> +unsigned int irq;
>> +struct atmel_isi *isi;
>> +struct clk *pclk;
>> +struct resource *regs;
>> +int ret;
>> +struct device *dev = &pdev->dev;
>> +struct isi_platform_data *pdata;
>> +struct soc_camera_host *soc_host;
>> +
>> +regs = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> +if (!regs)
>> +return -ENXIO;
>> +
>> +pclk = clk_get(&pdev->dev, "isi_clk");
>> +if (IS_ERR(pclk))
>> +return PTR_ERR(pclk);
>> +
>> +clk_enable(pclk);
> Return value of clk_enable() should be checked.
Yes. I will add code to check the return value in V2 patch.
>> +
>> +isi = kzalloc(sizeof(struct atmel_isi), GFP_KERNEL);
>> +if (!isi) {
>> +ret = -ENOMEM;
>> [...]
>> +isi_writel(isi, V2_CTRL, ISI_BIT(V2_DIS));
>> +/* Check if module disable */
>> +while (isi_readl(isi, V2_STATUS) & ISI_BIT(V2_DIS))
>> +cpu_relax();
>> +
>> +irq = platform_get_irq(pdev, 0);
> This should also be checked.
Ditto, thank you.
--
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
Re: [PATCH] [media] at91: add Atmel Image Sensor Interface (ISI) support
> > +
> > +/* Constants for RGB_CFG(ISI_V2) */
> > +#define ISI_V2_RGB_CFG_DEFAULT 0
> > +#define ISI_V2_RGB_CFG_MODE_1 1
> > +#define ISI_V2_RGB_CFG_MODE_2 2
> > +#define ISI_V2_RGB_CFG_MODE_3 3
> > +
> > +/* Bit manipulation macros */
> > +#define ISI_BIT(name) \
> > + (1 << ISI_##name##_OFFSET)
> > +#define ISI_BF(name, value)\
> > + (((value) & ((1 << ISI_##name##_SIZE) - 1)) \
> > +<< ISI_##name##_OFFSET)
> > +#define ISI_BFEXT(name, value) \
> > + (((value) >> ISI_##name##_OFFSET) \
> > +& ((1 << ISI_##name##_SIZE) - 1))
> > +#define ISI_BFINS(name, value, old)\
> > + (((old) & ~(((1 << ISI_##name##_SIZE) - 1) \
> > + << ISI_##name##_OFFSET))\
> > +| ISI_BF(name, value))
>
> I really dislike this kind of register access magic. Not sure how others
> feel about it. These macros are really ugly.
>
> > +/* Register access macros */
> > +#define isi_readl(port, reg) \
> > + __raw_readl((port)->regs + ISI_##reg)
> > +#define isi_writel(port, reg, value) \
> > + __raw_writel((value), (port)->regs + ISI_##reg)
>
> If the token pasting stuff gets dropped then these can be static inline
> functions which is preferred.
Sorry this make the C code much readable
and this can not be done as a inline
so please keep it as is
>
> > +
> > +#define ATMEL_V4L2_VID_FLAGS (V4L2_CAP_VIDEO_OUTPUT)
> > +
> > +struct atmel_isi;
> > +
> > +enum atmel_isi_pixfmt {
> > + ATMEL_ISI_PIXFMT_GREY, /* Greyscale */
> > + dev_info(&pdev->dev,
> > + "No config available using default values\n");
> > + pdata = &isi_default_data;
> > + }
> > +
> > + isi->pdata = pdata;
> > + isi->platform_flags = pdata->flags;
> > + if (isi->platform_flags == 0)
> > + isi->platform_flags = ISI_DATAWIDTH_8;
>
> We could be mean here an require that people get the flags correct, e.g.
>
> if (!((isi->platform_flags & ISI_DATA_WIDTH_8) ||
> (isi->platform_flags & ISI_DATA_WIDTH_8))) {
> dev_err(&pdev->dev, "No data width specified\n");
> err = -ENOMEM;
> goto fail;
> }
>
> Which discourages sloppy coding in the board files.
if this means default configuration so not need to have all board to set it
>
> > +
> > + isi_writel(isi, V2_CTRL, ISI_BIT(V2_DIS));
> > + /* Check if module disable */
> > + while (isi_readl(isi, V2_STATUS) & ISI_BIT(V2_DIS))
> > + cpu_relax();
> > +
> > + irq = platform_get_irq(pdev, 0);
Best Regards,
J.
--
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
Re: [PATCH] [media] at91: add Atmel Image Sensor Interface (ISI) support
On 05/13/2011 12:14 AM, Guennadi Liakhovetski wrote: > On Thu, 12 May 2011, Jean-Christophe PLAGNIOL-VILLARD wrote: > > [snip] > >>> + if (0 == *nbuffers) >> please invert the test > > Don't think this is required by CodingStyle or anything like that. If it > were, you'd have to revamp half of the kernel. It should at least be consistent within a file, which it is not true in this case. I think the preferred style is to have the variable on the left. ~Ryan -- Bluewater Systems Ltd - ARM Technology Solution Centre Ryan Mallon 5 Amuri Park, 404 Barbadoes St r...@bluewatersys.com PO Box 13 889, Christchurch 8013 http://www.bluewatersys.com New Zealand Phone: +64 3 3779127Freecall: Australia 1800 148 751 Fax: +64 3 3779135 USA 1800 261 2934 -- 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
Re: [PATCH] [media] at91: add Atmel Image Sensor Interface (ISI) support
On 05/12/2011 07:42 PM, Josh Wu wrote: > This patch is to enable Atmel Image Sensor Interface (ISI) driver support. > - Using soc-camera framework with videobuf2 dma-contig allocator > - Supporting video streaming of YUV packed format > - Tested on AT91SAM9M10G45-EK with OV2640 Hi Josh, Thansk for doing this. Overall the patch looks really good. A few comments below. ~Ryan > > Signed-off-by: Josh Wu > --- > base on branch staging/for_v2.6.40 > > arch/arm/mach-at91/include/mach/at91_isi.h | 454 > drivers/media/video/Kconfig| 10 + > drivers/media/video/Makefile |1 + > drivers/media/video/atmel-isi.c| 1089 > > 4 files changed, 1554 insertions(+), 0 deletions(-) > create mode 100644 arch/arm/mach-at91/include/mach/at91_isi.h > create mode 100644 drivers/media/video/atmel-isi.c > > diff --git a/arch/arm/mach-at91/include/mach/at91_isi.h > b/arch/arm/mach-at91/include/mach/at91_isi.h > new file mode 100644 > index 000..3219358 > --- /dev/null > +++ b/arch/arm/mach-at91/include/mach/at91_isi.h > @@ -0,0 +1,454 @@ > +/* > + * Register definitions for the Atmel Image Sensor Interface. > + * > + * Copyright (C) 2011 Atmel Corporation > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License version 2 as > + * published by the Free Software Foundation. > + */ > +#ifndef __AT91_ISI_H__ > +#define __AT91_ISI_H__ > + > +#include > +#include > +#include > +#include > +#include > +#include > + > +/* ISI register offsets */ > +#define ISI_CR1 0x > +#define ISI_CR2 0x0004 > +#define ISI_SR 0x0008 > +#define ISI_IER 0x000c > +#define ISI_IDR 0x0010 > +#define ISI_IMR 0x0014 > +#define ISI_PSIZE0x0020 > +#define ISI_PDECF0x0024 > +#define ISI_PPFBD0x0028 > +#define ISI_CDBA 0x002c > +#define ISI_Y2R_SET0 0x0030 > +#define ISI_Y2R_SET1 0x0034 > +#define ISI_R2Y_SET0 0x0038 > +#define ISI_R2Y_SET1 0x003c > +#define ISI_R2Y_SET2 0x0040 > + > +/* ISI_V2 register offsets */ > +#define ISI_V2_CFG1 0x > +#define ISI_V2_CFG2 0x0004 > +#define ISI_V2_PSIZE 0x0008 > +#define ISI_V2_PDECF 0x000c > +#define ISI_V2_Y2R_SET0 0x0010 > +#define ISI_V2_Y2R_SET1 0x0014 > +#define ISI_V2_R2Y_SET0 0x0018 > +#define ISI_V2_R2Y_SET1 0x001C > +#define ISI_V2_R2Y_SET2 0x0020 > +#define ISI_V2_CTRL 0x0024 > +#define ISI_V2_STATUS0x0028 > +#define ISI_V2_INTEN 0x002C > +#define ISI_V2_INTDIS0x0030 > +#define ISI_V2_INTMASK 0x0034 > +#define ISI_V2_DMA_CHER 0x0038 > +#define ISI_V2_DMA_CHDR 0x003C > +#define ISI_V2_DMA_CHSR 0x0040 > +#define ISI_V2_DMA_P_ADDR0x0044 > +#define ISI_V2_DMA_P_CTRL0x0048 > +#define ISI_V2_DMA_P_DSCR0x004C > +#define ISI_V2_DMA_C_ADDR0x0050 > +#define ISI_V2_DMA_C_CTRL0x0054 > +#define ISI_V2_DMA_C_DSCR0x0058 > + > +/* Bitfields in CR1 */ > +#define ISI_RST_OFFSET 0 > +#define ISI_RST_SIZE 1 > +#define ISI_DIS_OFFSET 1 > +#define ISI_DIS_SIZE 1 > +#define ISI_HSYNC_POL_OFFSET 2 > +#define ISI_HSYNC_POL_SIZE 1 > +#define ISI_VSYNC_POL_OFFSET 3 > +#define ISI_VSYNC_POL_SIZE 1 > +#define ISI_PIXCLK_POL_OFFSET4 > +#define ISI_PIXCLK_POL_SIZE 1 > +#define ISI_EMB_SYNC_OFFSET 6 > +#define ISI_EMB_SYNC_SIZE1 > +#define ISI_CRC_SYNC_OFFSET 7 > +#define ISI_CRC_SYNC_SIZE1 > +#define ISI_FRATE_OFFSET 8 > +#define ISI_FRATE_SIZE 3 > +#define ISI_FULL_OFFSET 12 > +#define ISI_FULL_SIZE1 > +#define ISI_THMASK_OFFSET13 > +#define ISI_THMASK_SIZE 2 > +#define ISI_C
Re: [PATCH] [media] at91: add Atmel Image Sensor Interface (ISI) support
On Thu, 12 May 2011, Jean-Christophe PLAGNIOL-VILLARD wrote: [snip] > > + if (0 == *nbuffers) > please invert the test Don't think this is required by CodingStyle or anything like that. If it were, you'd have to revamp half of the kernel. Thanks Guennadi --- Guennadi Liakhovetski, Ph.D. Freelance Open-Source Software Developer http://www.open-technology.de/ -- 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
Re: [PATCH] [media] at91: add Atmel Image Sensor Interface (ISI) support
> +struct atmel_isi;
do we really this here?
> +
> +enum atmel_isi_pixfmt {
> + ATMEL_ISI_PIXFMT_GREY, /* Greyscale */
> + ATMEL_ISI_PIXFMT_CbYCrY,
> + ATMEL_ISI_PIXFMT_CrYCbY,
> + ATMEL_ISI_PIXFMT_YCbYCr,
> + ATMEL_ISI_PIXFMT_YCrYCb,
> + ATMEL_ISI_PIXFMT_RGB24,
> + ATMEL_ISI_PIXFMT_BGR24,
> + ATMEL_ISI_PIXFMT_RGB16,
> + ATMEL_ISI_PIXFMT_BGR16,
> + ATMEL_ISI_PIXFMT_GRB16, /* G[2:0] R[4:0]/B[4:0] G[5:3] */
> + ATMEL_ISI_PIXFMT_GBR16, /* G[2:0] B[4:0]/R[4:0] G[5:3] */
> + ATMEL_ISI_PIXFMT_RGB24_REV,
> + ATMEL_ISI_PIXFMT_BGR24_REV,
> + ATMEL_ISI_PIXFMT_RGB16_REV,
> + ATMEL_ISI_PIXFMT_BGR16_REV,
> + ATMEL_ISI_PIXFMT_GRB16_REV, /* G[2:0] R[4:0]/B[4:0] G[5:3] */
> + ATMEL_ISI_PIXFMT_GBR16_REV, /* G[2:0] B[4:0]/R[4:0] G[5:3] */
> +};
> +
> +struct isi_platform_data {
> + u8 has_emb_sync;
> + u8 emb_crc_sync;
> + u8 hsync_act_low;
> + u8 vsync_act_low;
> + u8 pclk_act_falling;
> + u8 isi_full_mode;
> +#define ISI_HSYNC_ACT_LOW0x01
> +#define ISI_VSYNC_ACT_LOW0x02
> +#define ISI_PXCLK_ACT_FALLING0x04
> +#define ISI_EMB_SYNC 0x08
> +#define ISI_CRC_SYNC 0x10
> +#define ISI_FULL 0x20
> +#define ISI_DATAWIDTH_8 0x40
> +#define ISI_DATAWIDTH_10 0x80
> + u32 flags;
> + u8 gs_mode;
> +#define ISI_GS_2PIX_PER_WORD 0x00
> +#define ISI_GS_1PIX_PER_WORD 0x01
> + u8 pixfmt;
> + u8 sfd;
> + u8 sld;
> + u8 thmask;
> +#define ISI_BURST_4_8_16 0x00
> +#define ISI_BURST_8_16 0x01
> +#define ISI_BURST_16 0x02
> + u8 frate;
> +#define ISI_FRATE_DIV_2 0x01
> +#define ISI_FRATE_DIV_3 0x02
> +#define ISI_FRATE_DIV_4 0x03
> +#define ISI_FRATE_DIV_5 0x04
> +#define ISI_FRATE_DIV_6 0x05
> +#define ISI_FRATE_DIV_7 0x06
> +#define ISI_FRATE_DIV_8 0x07
> +};
> +
> +#endif /* __AT91_ISI_H__ */
> diff --git a/drivers/media/video/Kconfig b/drivers/media/video/Kconfig
> index d61414e..eae6005 100644
> --- a/drivers/media/video/Kconfig
> +++ b/drivers/media/video/Kconfig
> @@ -80,6 +80,16 @@ menuconfig VIDEO_CAPTURE_DRIVERS
> Some of those devices also supports FM radio.
>
> if VIDEO_CAPTURE_DRIVERS && VIDEO_V4L2
> +config VIDEO_ATMEL_ISI
> + tristate "ATMEL Image Sensor Interface (ISI) support"
> + depends on VIDEO_DEV && SOC_CAMERA
depends on AT91 if the drivers is at91 specific or avr32 otherwise
> + select VIDEOBUF2_DMA_CONTIG
> + default n
it's n by default please remove
> + ---help---
> + This module makes the ATMEL Image Sensor Interface available
> + as a v4l2 device.
> + Say Y here to enable selecting the Image Sensor Interface.
> + When in doubt, say N.
>
> config VIDEO_ADV_DEBUG
> bool "Enable advanced debug functionality"
> diff --git a/drivers/media/video/Makefile b/drivers/media/video/Makefile
> index a10e4c3..f734a65 100644
> --- a/drivers/media/video/Makefile
> +++ b/drivers/media/video/Makefile
> @@ -166,6 +166,7 @@ obj-$(CONFIG_VIDEO_SH_MOBILE_CSI2)+=
> sh_mobile_csi2.o
> obj-$(CONFIG_VIDEO_SH_MOBILE_CEU)+= sh_mobile_ceu_camera.o
> obj-$(CONFIG_VIDEO_OMAP1)+= omap1_camera.o
> obj-$(CONFIG_VIDEO_SAMSUNG_S5P_FIMC) += s5p-fimc/
> +obj-$(CONFIG_VIDEO_ATMEL_ISI)+= atmel-isi.o
>
> obj-$(CONFIG_ARCH_DAVINCI) += davinci/
>
> diff --git a/drivers/media/video/atmel-isi.c b/drivers/media/video/atmel-isi.c
> new file mode 100644
> index 000..33d0b83
> --- /dev/null
> +++ b/drivers/media/video/atmel-isi.c
> @@ -0,0 +1,1089 @@
> +/*
> + * Copyright (c) 2011 Atmel Corporation
> + *
> + * Based on previous work by Lars Haring and Sedji Gaouaou
> + *
> + * Based on the bttv driver for Bt848 with respective copyright holders
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#include
> +#include
> +#include
> +
> +#include
> +#include
> +#include
> +
> +#define ATMEL_ISI_VERSIONKERNEL_VERSION(1, 0, 0)
> +#define MAX_BUFFER_NUMS 32
> +#define MAX_SUPPORT_WIDTH2048
> +#define MAX_SUPPORT_HEIGHT 2048
> +
> +static unsigned int vid_limit = 16;
> +
> +enum isi_buffer_state {
> + ISI_BUF_NEEDS_INIT,
> + ISI_BUF_PREPARED,
> +};
> +
> +/* Single frame capturing states */
> +enum {
> + STATE_IDLE = 0,
> + STATE_CAPTURE_READY,
> + STATE_CAPTURE_WAIT_SOF,
> + STATE_CAPTURE_IN_PROGRESS,
> + STATE_CAPTURE_DONE,
> + STATE_CAPTURE_ERROR,
> +};
> +
> +/* Frame buffer descriptor
> + * Used by the ISI module as a linked list f
RE: [PATCH] [media] at91: add Atmel Image Sensor Interface (ISI)support
From: Guennadi Liakhovetski [mailto:g.liakhovet...@gmx.de] Sent: Thursday, May 12, 2011 5:32 PM > On Thu, 12 May 2011, Wu, Josh wrote: >> Hi, Russell >> >> From: Russell King - ARM Linux [mailto:li...@arm.linux.org.uk] Sent: >> Thursday, May 12, 2011 3:47 PM >> > On Thu, May 12, 2011 at 03:42:18PM +0800, Josh Wu wrote: >> >> +err_alloc_isi: >> >> + clk_disable(pclk); >> > clk_put() ? >> Ok, will be fixed in V2 patch. Thanks. > You might wait with v2 until I find time to review your patch. Will take a > couple of days, I think. A general question, though: how compatible is > this driver with the AVR32? That is ok. For AVR32, I think I need time to check with AVR team. I will update the status when I got more information. Best Regards, Josh Wu -- 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
Re: [PATCH] [media] at91: add Atmel Image Sensor Interface (ISI) support
On Thu, May 12, 2011 at 03:42:18PM +0800, Josh Wu wrote:
> This patch is to enable Atmel Image Sensor Interface (ISI) driver support.
> - Using soc-camera framework with videobuf2 dma-contig allocator
> - Supporting video streaming of YUV packed format
> - Tested on AT91SAM9M10G45-EK with OV2640
A few more points...
> +static int __init atmel_isi_probe(struct platform_device *pdev)
Should be __devinit otherwise you'll have section errors.
> +{
> + unsigned int irq;
> + struct atmel_isi *isi;
> + struct clk *pclk;
> + struct resource *regs;
> + int ret;
> + struct device *dev = &pdev->dev;
> + struct isi_platform_data *pdata;
> + struct soc_camera_host *soc_host;
> +
> + regs = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + if (!regs)
> + return -ENXIO;
> +
> + pclk = clk_get(&pdev->dev, "isi_clk");
> + if (IS_ERR(pclk))
> + return PTR_ERR(pclk);
> +
> + clk_enable(pclk);
Return value of clk_enable() should be checked.
> +
> + isi = kzalloc(sizeof(struct atmel_isi), GFP_KERNEL);
> + if (!isi) {
> + ret = -ENOMEM;
> + dev_err(&pdev->dev, "can't allocate interface!\n");
> + goto err_alloc_isi;
> + }
> +
> + isi->pclk = pclk;
> +
> + spin_lock_init(&isi->lock);
> + init_waitqueue_head(&isi->capture_wq);
> +
> + isi->alloc_ctx = vb2_dma_contig_init_ctx(&pdev->dev);
> + if (IS_ERR(isi->alloc_ctx)) {
> + ret = PTR_ERR(isi->alloc_ctx);
> + goto err_alloc_isi;
> + }
> +
> + isi->regs = ioremap(regs->start, resource_size(regs));
> + if (!isi->regs) {
> + ret = -ENOMEM;
> + goto err_ioremap;
> + }
> +
> + if (dev->platform_data)
> + pdata = (struct isi_platform_data *) dev->platform_data;
> + else {
> + static struct isi_platform_data isi_default_data = {
> + .frate = 0,
> + .has_emb_sync = 0,
> + .emb_crc_sync = 0,
> + .hsync_act_low = 0,
> + .vsync_act_low = 0,
> + .pclk_act_falling = 0,
> + .isi_full_mode = 1,
> + /* to use codec and preview path simultaneously */
> + .flags = ISI_DATAWIDTH_8 |
> + ISI_DATAWIDTH_10,
> + };
> + dev_info(&pdev->dev,
> + "No config available using default values\n");
> + pdata = &isi_default_data;
> + }
> +
> + isi->pdata = pdata;
> + isi->platform_flags = pdata->flags;
> + if (isi->platform_flags == 0)
> + isi->platform_flags = ISI_DATAWIDTH_8;
> +
> + isi_writel(isi, V2_CTRL, ISI_BIT(V2_DIS));
> + /* Check if module disable */
> + while (isi_readl(isi, V2_STATUS) & ISI_BIT(V2_DIS))
> + cpu_relax();
> +
> + irq = platform_get_irq(pdev, 0);
This should also be checked.
--
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
RE: [PATCH] [media] at91: add Atmel Image Sensor Interface (ISI)support
On Thu, 12 May 2011, Wu, Josh wrote: > Hi, Russell > > From: Russell King - ARM Linux [mailto:li...@arm.linux.org.uk] Sent: > Thursday, May 12, 2011 3:47 PM > > On Thu, May 12, 2011 at 03:42:18PM +0800, Josh Wu wrote: > >> +err_alloc_isi: > >> + clk_disable(pclk); > > clk_put() ? > Ok, will be fixed in V2 patch. Thanks. You might wait with v2 until I find time to review your patch. Will take a couple of days, I think. A general question, though: how compatible is this driver with the AVR32? Thanks Guennadi --- Guennadi Liakhovetski, Ph.D. Freelance Open-Source Software Developer http://www.open-technology.de/ -- 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
RE: [PATCH] [media] at91: add Atmel Image Sensor Interface (ISI)support
Hi, Russell From: Russell King - ARM Linux [mailto:li...@arm.linux.org.uk] Sent: Thursday, May 12, 2011 3:47 PM > On Thu, May 12, 2011 at 03:42:18PM +0800, Josh Wu wrote: >> +err_alloc_isi: >> +clk_disable(pclk); > clk_put() ? Ok, will be fixed in V2 patch. Thanks. Best Regards, Josh Wu -- 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
Re: [PATCH] [media] at91: add Atmel Image Sensor Interface (ISI) support
On Thu, May 12, 2011 at 03:42:18PM +0800, Josh Wu wrote: > +err_alloc_isi: > + clk_disable(pclk); clk_put() ? -- 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
