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 <josh.wu@xxxxxxxxx>
>> ---
>> 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_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
>> +};
> 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 <media/videobuf2-dma-contig.h>
>> +#include <media/soc_camera.h>
>> +#include <media/soc_mediabus.h>
>> +
>> +#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 this already?
> VIDEOBUF_NEEDS_INIT,
> VIDEOBUF_PREPARED,
> Just reuse those.
I checked the code, above constants are used in videobuf not videobuf2. So I think I cannot use it.
>> +
>> +/* 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 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
> I wouldn't bother with this ifdef. The memory cost is pretty
> insignificant and it makes the code easier to read without it. It also
> means you don't need to patch it up whenever a new at91 platform with
> isi dma support appears.
I will remove this #if. It is very disappointed to add some ARCH macro every time when any new platform support ISI added.
>> + /* 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
> Same here, get rid of the ifdefs.
I'll remove this.
>> [snip]
>> + if (!isi->still_capture) {
>> + if (pending & (ISI_BIT(V2_VSYNC))) {
>> + switch (isi->state) {
>> + case STATE_IDLE:
>> + isi->state = STATE_CAPTURE_READY;
>> + wake_up_interruptible(&isi->capture_wq);
>> + break;
>> + }
>> + } else if (likely(pending & (ISI_BIT(V2_CXFR_DONE)))) {
>> + ret = atmel_isi_handle_streaming(isi);
>> + }
>> + } else {
>> + while (pending) {
>> + if (pending & (ISI_BIT(V2_C_OVR) | ISI_BIT(V2_FR_OVR)))
>> + printk(KERN_ERR "Overrun (status = 0x%x)\n",
>> + status);
> dev_err(isi->v4l2_dev.dev, "Overrun...");
I will fix it.
>> [snip]
>> +
>> +static int atmel_isi_init(struct atmel_isi *isi)
>> +{
>> + /*
>> + * The reset will only succeed if we have a
>> + * pixel clock from the camera.
>> + */
>> + isi_writel(isi, V2_CTRL, ISI_BIT(V2_SRST));
>> + isi_writel(isi, V2_INTDIS, ~0UL);
> Don't you need to wait for the reset bit to clear? Other implementations
> I have used of the Atmel ISI driver do a wait_for_completition and have
> the interrupt handler set a reset_complete flag.
You are right. I just remove the reset_complete waiting part in this version.
Actually I am thinking whether I need add such code again.
>> [snip]
>> + if (bytes_per_line < 0)
>> + return bytes_per_line;
>> +
>> + size = bytes_per_line * icd->user_height;
>> +
>> + if (0 == *nbuffers)
> if (*nbuffers == 0)
> is the more commonly preferred style I believe.
I will fix it for consistency.
>> [snip]
>> +
>> +static int buffer_finish(struct vb2_buffer *vb)
>> +{
>> + return 0;
>> +}
> Does soc-camera support having NULL function callbacks for functions
> that are not required?
I will remove this function at all. soc-camera can use default callback.
>> [snip]
>> +static inline void stride_align(u32 *width)
>> +{
>> + if (((*width + 7) & ~7) < 4096)
>> + *width = (*width + 7) & ~7;
>> + else
>> + *width = *width & ~7;
> This looks like something that may already have a function to do it?
Yes. you are right. There is a function ALIGN(*width, 8).
And I think I can remove this function since ISI is not sensetive for this width alignment.
>> [snip]
>> +
>> +static int isi_camera_try_fmt(struct soc_camera_device *icd,
>> + struct v4l2_format *f)
>> +{
>> + struct v4l2_subdev *sd = soc_camera_to_subdev(icd);
>> + const struct soc_camera_format_xlate *xlate;
>> + struct v4l2_pix_format *pix = &f->fmt.pix;
>> + struct v4l2_mbus_framefmt mf;
>> + __u32 pixfmt = pix->pixelformat;
> u32 inside the kernel, __u32 is for code which is exposed to userspace
>(i.e. API headers).
I will fixed it. no __u32 anymore.
>> [snip]
>> +static int test_platform_param(struct atmel_isi *isi,
>> + unsigned char buswidth, unsigned long *flags)
>> +{
>> + /*
>> + * Platform specified synchronization and pixel clock polarities are
>> + * only a recommendation and are only used during probing. Atmel ISI
>> + * camera interface only works in master mode, i.e., uses HSYNC and
>> + * VSYNC signals from the sensor
>> + */
>> + *flags = SOCAM_MASTER |
>> + SOCAM_HSYNC_ACTIVE_HIGH |
>> + SOCAM_HSYNC_ACTIVE_LOW |
>> + SOCAM_VSYNC_ACTIVE_HIGH |
>> + SOCAM_VSYNC_ACTIVE_LOW |
>> + SOCAM_PCLK_SAMPLE_RISING |
>> + SOCAM_PCLK_SAMPLE_FALLING |
>> + SOCAM_DATA_ACTIVE_HIGH;
>> +
>> + /* If requested data width is supported by the platform, use it */
>> + switch (buswidth) {
>> + case 10:
>> + if (!(isi->platform_flags & ISI_DATAWIDTH_10))
>> + return -EINVAL;
>> + *flags |= SOCAM_DATAWIDTH_10;
>> + break;
> If you have specified the platform data width as 10 bits can you not do
> 8 bit transfers on it?
no, you cannot use 8 bit transfers.
But normally we can specify the platform data width to support both 10 and 8.
>> [snip]
>> +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 = ATMEL_ISI_VERSION;
>> +
>> + cap->capabilities = (V4L2_CAP_VIDEO_CAPTURE
>> + | V4L2_CAP_READWRITE
>> + | V4L2_CAP_STREAMING
>> + );
> In other places you have the |'s at the end of the line. Should be
> consistent within a file.
I will fix it. And I find the same mistake in other place. Thank you.
>> + return 0;
>> +}
>> +
>> +static int isi_camera_set_bus_param(struct soc_camera_device *icd, __u32 pixfmt)
> u32.
will be fix.
>> [snip]
>> + 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,
>> + };
> Move this struct definition outside of this function.
OK. I will change that.
>> + 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.
now the isi->platform_flags only use for data width, so if it is 0, I will set is as default data width(ISI_DATA_WIDTH_8).
I think if we use platform_flags for more option (not only data width), then we need test it with a WIDTH_MASK.
>> [snip]
Best Regards,
Josh Wu
--
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
