From mboxrd@z Thu Jan 1 00:00:00 1970 Return-path: Received: from mail-fx0-f46.google.com ([209.85.161.46]:59118 "EHLO mail-fx0-f46.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1757929Ab1IAUr4 (ORCPT ); Thu, 1 Sep 2011 16:47:56 -0400 Received: by mail-fx0-f46.google.com with SMTP id 19so1029458fxh.19 for ; Thu, 01 Sep 2011 13:47:55 -0700 (PDT) Message-ID: <4E5FEF75.8060704@gmail.com> Date: Thu, 01 Sep 2011 22:47:49 +0200 From: Sylwester Nawrocki MIME-Version: 1.0 To: Manjunath Hadli CC: LMML , dlos , Nagabhushana Netagunte Subject: Re: [PATCH v2 1/8] davinci: vpfe: add dm3xx IPIPEIF hardware support module References: <1314630439-1122-1-git-send-email-manjunath.hadli@ti.com> <1314630439-1122-2-git-send-email-manjunath.hadli@ti.com> In-Reply-To: <1314630439-1122-2-git-send-email-manjunath.hadli@ti.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit Sender: linux-media-owner@vger.kernel.org List-ID: Hi Manjunath, A few comments below... On 08/29/2011 05:07 PM, Manjunath Hadli wrote: > add support for dm3xx IPIPEIF hardware setup. This is the > lowest software layer for the dm3x vpfe driver which directly > accesses hardware. Add support for features like default > pixel correction, dark frame substraction and hardware setup. > > Signed-off-by: Manjunath Hadli > Signed-off-by: Nagabhushana Netagunte > --- > drivers/media/video/davinci/dm3xx_ipipeif.c | 317 +++++++++++++++++++++++++++ > drivers/media/video/davinci/dm3xx_ipipeif.h | 258 ++++++++++++++++++++++ > include/linux/dm3xx_ipipeif.h | 64 ++++++ > 3 files changed, 639 insertions(+), 0 deletions(-) > create mode 100644 drivers/media/video/davinci/dm3xx_ipipeif.c > create mode 100644 drivers/media/video/davinci/dm3xx_ipipeif.h > create mode 100644 include/linux/dm3xx_ipipeif.h > > diff --git a/drivers/media/video/davinci/dm3xx_ipipeif.c b/drivers/media/video/davinci/dm3xx_ipipeif.c > new file mode 100644 > index 0000000..ebc8895 > --- /dev/null > +++ b/drivers/media/video/davinci/dm3xx_ipipeif.c > @@ -0,0 +1,317 @@ ... > + > +static void ipipeif_config_dpc(struct ipipeif_dpc *dpc) > +{ > + u32 val; > + > + /* Intialize val*/ > + val = 0; s/Intialize/Initialize ? But this comment doesn't seem much helpful and could probably be removed. Also it might be better to just do: u32 val = 0; > + > + if (dpc->en) { > + val = (dpc->en& 1)<< IPIPEIF_DPC2_EN_SHIFT; > + val |= dpc->thr& IPIPEIF_DPC2_THR_MASK; > + } > + > + regw_if(val, IPIPEIF_DPC2); > +} > + ... > + > +static int __devinit dm3xx_ipipeif_probe(struct platform_device *pdev) > +{ > + static resource_size_t res_len; > + struct resource *res; > + int status; > + > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > + if (!res) > + return -ENOENT; > + > + res_len = res->end - res->start + 1; resource_size(res) macro could be used here > + > + res = request_mem_region(res->start, res_len, res->name); > + if (!res) > + return -EBUSY; > + > + ipipeif_base_addr = ioremap_nocache(res->start, res_len); > + if (!ipipeif_base_addr) { > + status = -EBUSY; > + goto fail; > + } > + return 0; > + > +fail: > + release_mem_region(res->start, res_len); > + > + return status; > +} > + > +static int dm3xx_ipipeif_remove(struct platform_device *pdev) > +{ > + struct resource *res; > + > + iounmap(ipipeif_base_addr); > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > + if (res) > + release_mem_region(res->start, res->end - res->start + 1); release_mem_region(res->start, resource_size(res)); > + return 0; > +} > + > +static struct platform_driver dm3xx_ipipeif_driver = { > + .driver = { > + .name = "dm3xx_ipipeif", > + .owner = THIS_MODULE, > + }, > + .remove = __devexit_p(dm3xx_ipipeif_remove), > + .probe = dm3xx_ipipeif_probe, > +}; > + > +static int dm3xx_ipipeif_init(void) > +{ > + return platform_driver_register(&dm3xx_ipipeif_driver); > +} > + > +static void dm3xx_ipipeif_exit(void) > +{ > + platform_driver_unregister(&dm3xx_ipipeif_driver); > +} > + > +module_init(dm3xx_ipipeif_init); > +module_exit(dm3xx_ipipeif_exit); > + > +MODULE_LICENSE("GPL2"); > diff --git a/drivers/media/video/davinci/dm3xx_ipipeif.h b/drivers/media/video/davinci/dm3xx_ipipeif.h > new file mode 100644 > index 0000000..f3289f0 > --- /dev/null > +++ b/drivers/media/video/davinci/dm3xx_ipipeif.h > @@ -0,0 +1,258 @@ > +/* ... > + > +/* IPIPEIF Register Offsets from the base address */ > +#define IPIPEIF_ENABLE (0x00) > +#define IPIPEIF_CFG1 (0x04) > +#define IPIPEIF_PPLN (0x08) > +#define IPIPEIF_LPFR (0x0c) > +#define IPIPEIF_HNUM (0x10) > +#define IPIPEIF_VNUM (0x14) > +#define IPIPEIF_ADDRU (0x18) > +#define IPIPEIF_ADDRL (0x1c) > +#define IPIPEIF_ADOFS (0x20) > +#define IPIPEIF_RSZ (0x24) > +#define IPIPEIF_GAIN (0x28) > + > +/* Below registers are available only on IPIPE 5.1 */ > +#define IPIPEIF_DPCM (0x2c) > +#define IPIPEIF_CFG2 (0x30) > +#define IPIPEIF_INIRSZ (0x34) > +#define IPIPEIF_OCLIP (0x38) > +#define IPIPEIF_DTUDF (0x3c) > +#define IPIPEIF_CLKDIV (0x40) > +#define IPIPEIF_DPC1 (0x44) > +#define IPIPEIF_DPC2 (0x48) > +#define IPIPEIF_DFSGVL (0x4c) > +#define IPIPEIF_DFSGTH (0x50) > +#define IPIPEIF_RSZ3A (0x54) > +#define IPIPEIF_INIRSZ3A (0x58) > +#define IPIPEIF_RSZ_MIN (16) > +#define IPIPEIF_RSZ_MAX (112) > +#define IPIPEIF_RSZ_CONST (16) > +#define SETBIT(reg, bit) (reg = ((reg) | ((0x00000001)<<(bit)))) > +#define RESETBIT(reg, bit) (reg = ((reg)& (~(0x00000001<<(bit))))) > + > +#define IPIPEIF_ADOFS_LSB_MASK (0x1ff) > +#define IPIPEIF_ADOFS_LSB_SHIFT (5) > +#define IPIPEIF_ADOFS_MSB_MASK (0x200) > +#define IPIPEIF_ADDRU_MASK (0x7ff) > +#define IPIPEIF_ADDRL_SHIFT (5) > +#define IPIPEIF_ADDRL_MASK (0xffff) > +#define IPIPEIF_ADDRU_SHIFT (21) > +#define IPIPEIF_ADDRMSB_SHIFT (31) > +#define IPIPEIF_ADDRMSB_LEFT_SHIFT (10) > + > +/* CFG1 Masks and shifts */ > +#define ONESHOT_SHIFT (0) > +#define DECIM_SHIFT (1) > +#define INPSRC_SHIFT (2) > +#define CLKDIV_SHIFT (4) > +#define AVGFILT_SHIFT (7) > +#define PACK8IN_SHIFT (8) > +#define IALAW_SHIFT (9) > +#define CLKSEL_SHIFT (10) > +#define DATASFT_SHIFT (11) > +#define INPSRC1_SHIFT (14) > + > +/* DPC2 */ > +#define IPIPEIF_DPC2_EN_SHIFT (12) > +#define IPIPEIF_DPC2_THR_MASK (0xfff) > +/* Applicable for IPIPE 5.1 */ > +#define IPIPEIF_DF_GAIN_EN_SHIFT (10) > +#define IPIPEIF_DF_GAIN_MASK (0x3ff) > +#define IPIPEIF_DF_GAIN_THR_MASK (0xfff) > +/* DPCM */ > +#define IPIPEIF_DPCM_BITS_SHIFT (2) > +#define IPIPEIF_DPCM_PRED_SHIFT (1) > +/* CFG2 */ > +#define IPIPEIF_CFG2_HDPOL_SHIFT (1) > +#define IPIPEIF_CFG2_VDPOL_SHIFT (2) > +#define IPIPEIF_CFG2_YUV8_SHIFT (6) > +#define IPIPEIF_CFG2_YUV16_SHIFT (3) > +#define IPIPEIF_CFG2_YUV8P_SHIFT (7) > + > +/* INIRSZ */ > +#define IPIPEIF_INIRSZ_ALNSYNC_SHIFT (13) > +#define IPIPEIF_INIRSZ_MASK (0x1fff) Is there any good reason to use parentheses around the numbers ? -- Regards, Sylwester