On Fri, Jun 3, 2011 at 18:17, Scott Jiang <[email protected]> wrote: > --- a/drivers/media/video/Kconfig > +++ b/drivers/media/video/Kconfig > > +config VIDEO_ADV7183 > + tristate "Analog Devices ADV7183 decoder" > > +config VIDEO_VS6624 > + tristate "ST VS6624 sensor support" > > --- a/drivers/media/video/Makefile > +++ b/drivers/media/video/Makefile > +obj-$(CONFIG_VIDEO_ADV7183) += adv7183.o > +obj-$(CONFIG_VIDEO_VS6624) += vs6624.o
guess including these changes in this file was an accident > --- /dev/null > +++ b/drivers/media/video/blackfin/Kconfig > @@ -0,0 +1,19 @@ > +config VIDEO_BLACKFIN_PPI > + tristate "Blackfin PPI Driver" > + depends on VIDEO_BLACKFIN_CAPTURE > + help > + Support for Blackfin Parallel Peripheral Interface. > + > + To compile this driver as a module, choose M here: the > + module will be called ppi. "ppi" is too generic. please rename it to like "bfin_capture_ppi". > --- /dev/null > +++ b/drivers/media/video/blackfin/bfin_capture.c > @@ -0,0 +1,977 @@ > +/* > + * Analog Devices video capture driver > + * > + * Copyright (c) 2011 Scott Jiang <[email protected]> pretty sure there should be an Analog Devices copyright too > +static struct bcap_format bcap_formats[] = { shouldnt this be in the ppi code ? different ppi devices will have diff capabilities wont they ? the eppi supports more stuff than the ppi ... also, this should be const > +static int bcap_open(struct file *file) > +{ > + > + fh = kmalloc(sizeof(struct bcap_fh), GFP_KERNEL); sizeof(*bcap_dev) also, there seems to be a lot of fields you don't initialize, so perhaps you should be using kzalloc() instead > +static int bcap_probe(struct platform_device *pdev) __devinit > + bcap_dev = kzalloc(sizeof(*bcap_dev), GFP_KERNEL); > + if (!bcap_dev) { > + v4l2_err(pdev->dev.driver, "Unable to alloc bcap_dev\n"); > + return -ENOMEM; > + } > + > + config = pdev->dev.platform_data; > + if (!config) { > + v4l2_err(pdev->dev.driver, "Unable to get board config\n"); > + ret = -ENODEV; > + goto err; > + } should probably check the platform data before attempting an alloc > --- /dev/null > +++ b/drivers/media/video/blackfin/ppi.c > @@ -0,0 +1,266 @@ > +#include <asm/io.h> linux/io.h > +#define regr(reg) readw((reg) + ppi_base) > +#define regw(value, reg) writew(value, ((reg) + ppi_base)) > + > +#define REG_PPI_CONTROL 0x00 /* PPI Control */ > +#define REG_PPI_STATUS 0x04 /* PPI Status */ > +#define REG_PPI_COUNT 0x08 /* Transfer Count */ > +#define REG_PPI_DELAY 0x0C /* Delay Count */ > +#define REG_PPI_FRAME 0x10 /* Lines Per Frame */ ugh, no. include asm/bfin_ppi.h, and then use things like bfin_read(®s->control) and bfin_write(®s->control, 0x100). > +static const unsigned short ppi_req[] = { > + P_PPI0_D0, P_PPI0_D1, P_PPI0_D2, P_PPI0_D3, > + P_PPI0_D4, P_PPI0_D5, P_PPI0_D6, P_PPI0_D7, > + P_PPI0_CLK, P_PPI0_FS1, > + 0, > +}; this must be moved to the platform data > +static struct ppi_if ppi_intf = { > + .ops = &ppi_ops, > + .pin_req = ppi_req, > +}; > + > +static void __iomem *ppi_base; > +static resource_size_t res_start, res_len; this must be allocated dynamically. that way we have one driver that can handle multiple ppi's without a problem. > +static irqreturn_t ppi_irq_err(int irq, void *dev_id) > +{ > + unsigned short status; > + > + status = regr(REG_PPI_STATUS); > + printk(KERN_INFO, "%s: status = 0x%x\n", __func__, status); pr_debug(), although once you make this state properly dynamic, you can store it in the pdev->private member and pass pdev around and then use dev_dbg() > +static int ppi_attach_irq(struct ppi_if *intf, irq_handler_t handler) > + printk(KERN_ERR "Unable to allocate DMA channel for PPI\n"); > + printk(KERN_ERR "Unable to allocate IRQ for PPI\n"); either pr_err() or dev_err() once you fix the dynamic issue > +static int ppi_probe(struct platform_device *pdev) __devinit > +static struct platform_driver ppi_driver = { > + .driver = { > + .name = "ppi", "ppi" is too generic > +static void ppi_exit(void) __exit > +device_initcall(ppi_init); does this really need to be device_initcall() ? or is this just what everyone else uses ? > --- /dev/null > +++ b/include/media/blackfin/bfin_capture.h > @@ -0,0 +1,31 @@ this needs a comment block with info/copyright/license > +#ifdef __KERNEL__ this isnt necessary as we arent exporting this file to userspace > +struct bfin_capture_config { > + /* card name */ > + char *card_name; probably should be const -mike _______________________________________________ Linux-kernel-commits mailing list [email protected] https://blackfin.uclinux.org/mailman/listinfo/linux-kernel-commits
