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<manjunath.hadli@xxxxxx>
> Signed-off-by: Nagabhushana Netagunte<nagabhushana.netagunte@xxxxxx>
> ---
> 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
--
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
