Re: [PATCH 01/14] davinci: vpfe: add dm3xx IPIPEIF hardware support module
Hi Laurent,
Thanks for the review.
On Thu, Sep 20, 2012 at 3:31 AM, Laurent Pinchart
laurent.pinch...@ideasonboard.com wrote:
Hi Prabhakar,
Thanks for the patch.
On Friday 14 September 2012 18:16:31 Prabhakar Lad wrote:
From: Manjunath Hadli manjunath.ha...@ti.com
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.ha...@ti.com
Signed-off-by: Lad, Prabhakar prabhakar@ti.com
---
drivers/media/platform/davinci/dm3xx_ipipeif.c | 318 +
drivers/media/platform/davinci/dm3xx_ipipeif.h | 262 +++
include/linux/dm3xx_ipipeif.h | 62 +
3 files changed, 642 insertions(+), 0 deletions(-)
create mode 100644 drivers/media/platform/davinci/dm3xx_ipipeif.c
create mode 100644 drivers/media/platform/davinci/dm3xx_ipipeif.h
create mode 100644 include/linux/dm3xx_ipipeif.h
diff --git a/drivers/media/platform/davinci/dm3xx_ipipeif.c
b/drivers/media/platform/davinci/dm3xx_ipipeif.c new file mode 100644
index 000..7961a74
--- /dev/null
+++ b/drivers/media/platform/davinci/dm3xx_ipipeif.c
[snip]
+#include linux/io.h
+#include linux/module.h
+#include linux/kernel.h
+#include linux/uaccess.h
+#include linux/v4l2-mediabus.h
+#include linux/platform_device.h
Just a side note, I usually sort headers alphabetically, but feel free to use
whatever convention you like.
Ok I'll sort to.
+#include dm3xx_ipipeif.h
+
+static void *__iomem ipipeif_base_addr;
That's not good. You shouldn't have global constants like that. The value
should instead be stored in your device structure, that you will need to pass
around to all functions.
Ok.
+static inline u32 regr_if(u32 offset)
+{
+ return readl(ipipeif_base_addr + offset);
+}
+
+static inline void regw_if(u32 val, u32 offset)
+{
+ writel(val, ipipeif_base_addr + offset);
+}
Maybe ipipeif_read() and ipipeif_write() ?
+void ipipeif_set_enable()
+{
+ regw_if(1, IPIPEIF_ENABLE);
+}
Please define constants in a header file for register values, masks and shifts
instead of hardcoding the raw numbers.
Ok.
[snip]
+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 = resource_size(res);
+
+ 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, 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);
I'm not sure to like this. You're registering a module for a device that
essentially sits there without doing anything, except providing functions that
can be called by other modules.
I somehow feel that the way the code is split amongst the different layers
isn't optimal. Could you briefly explain the rationale behind the current
architecture ?
(BTW, please use the module_platform_driver() macro instead of
module_init/module_exit)
As discussed over the IRC, I am working on new design, hopefully
you will be happy this time :)
[snip]
diff --git a/include/linux/dm3xx_ipipeif.h b/include/linux/dm3xx_ipipeif.h
new file mode 100644
index 000..1c1a830
--- /dev/null
+++ b/include/linux/dm3xx_ipipeif.h
[snip]
+#include media/davinci/vpfe_types.h
+#include media/davinci/vpfe.h
This header file defines part of the userspace API, but includes media/
headers that are not exported to userspace.
Header files should be split between 3 directories:
- Definitions required by platform data used to go to media/ but the new
include/linux/platform_data/ directory might
Re: [PATCH 01/14] davinci: vpfe: add dm3xx IPIPEIF hardware support module
Hi Prabhakar,
Thanks for the patch.
On Friday 14 September 2012 18:16:31 Prabhakar Lad wrote:
From: Manjunath Hadli manjunath.ha...@ti.com
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.ha...@ti.com
Signed-off-by: Lad, Prabhakar prabhakar@ti.com
---
drivers/media/platform/davinci/dm3xx_ipipeif.c | 318 +
drivers/media/platform/davinci/dm3xx_ipipeif.h | 262 +++
include/linux/dm3xx_ipipeif.h | 62 +
3 files changed, 642 insertions(+), 0 deletions(-)
create mode 100644 drivers/media/platform/davinci/dm3xx_ipipeif.c
create mode 100644 drivers/media/platform/davinci/dm3xx_ipipeif.h
create mode 100644 include/linux/dm3xx_ipipeif.h
diff --git a/drivers/media/platform/davinci/dm3xx_ipipeif.c
b/drivers/media/platform/davinci/dm3xx_ipipeif.c new file mode 100644
index 000..7961a74
--- /dev/null
+++ b/drivers/media/platform/davinci/dm3xx_ipipeif.c
[snip]
+#include linux/io.h
+#include linux/module.h
+#include linux/kernel.h
+#include linux/uaccess.h
+#include linux/v4l2-mediabus.h
+#include linux/platform_device.h
Just a side note, I usually sort headers alphabetically, but feel free to use
whatever convention you like.
+#include dm3xx_ipipeif.h
+
+static void *__iomem ipipeif_base_addr;
That's not good. You shouldn't have global constants like that. The value
should instead be stored in your device structure, that you will need to pass
around to all functions.
+static inline u32 regr_if(u32 offset)
+{
+ return readl(ipipeif_base_addr + offset);
+}
+
+static inline void regw_if(u32 val, u32 offset)
+{
+ writel(val, ipipeif_base_addr + offset);
+}
Maybe ipipeif_read() and ipipeif_write() ?
+void ipipeif_set_enable()
+{
+ regw_if(1, IPIPEIF_ENABLE);
+}
Please define constants in a header file for register values, masks and shifts
instead of hardcoding the raw numbers.
[snip]
+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 = resource_size(res);
+
+ 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, 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);
I'm not sure to like this. You're registering a module for a device that
essentially sits there without doing anything, except providing functions that
can be called by other modules.
I somehow feel that the way the code is split amongst the different layers
isn't optimal. Could you briefly explain the rationale behind the current
architecture ?
(BTW, please use the module_platform_driver() macro instead of
module_init/module_exit)
[snip]
diff --git a/include/linux/dm3xx_ipipeif.h b/include/linux/dm3xx_ipipeif.h
new file mode 100644
index 000..1c1a830
--- /dev/null
+++ b/include/linux/dm3xx_ipipeif.h
[snip]
+#include media/davinci/vpfe_types.h
+#include media/davinci/vpfe.h
This header file defines part of the userspace API, but includes media/
headers that are not exported to userspace.
Header files should be split between 3 directories:
- Definitions required by platform data used to go to media/ but the new
include/linux/platform_data/ directory might be preferred nowadays. I have no
strong opinion on this, as other headers are already in media/ you can
probably keep using it for now.
- Definitions requires by userspace should go to include/linux/
- The rest should go to drivers/media/platform/davinci/.
--
[PATCH 01/14] davinci: vpfe: add dm3xx IPIPEIF hardware support module
From: Manjunath Hadli manjunath.ha...@ti.com
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.ha...@ti.com
Signed-off-by: Lad, Prabhakar prabhakar@ti.com
---
drivers/media/platform/davinci/dm3xx_ipipeif.c | 318
drivers/media/platform/davinci/dm3xx_ipipeif.h | 262 +++
include/linux/dm3xx_ipipeif.h | 62 +
3 files changed, 642 insertions(+), 0 deletions(-)
create mode 100644 drivers/media/platform/davinci/dm3xx_ipipeif.c
create mode 100644 drivers/media/platform/davinci/dm3xx_ipipeif.h
create mode 100644 include/linux/dm3xx_ipipeif.h
diff --git a/drivers/media/platform/davinci/dm3xx_ipipeif.c
b/drivers/media/platform/davinci/dm3xx_ipipeif.c
new file mode 100644
index 000..7961a74
--- /dev/null
+++ b/drivers/media/platform/davinci/dm3xx_ipipeif.c
@@ -0,0 +1,318 @@
+/*
+ * Copyright (C) 2012 Texas Instruments Inc
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation version 2.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
+ *
+ * Contributors:
+ * Manjunath Hadli manjunath.ha...@ti.com
+ * Prabhakar Lad prabhakar@ti.com
+ */
+
+#include linux/io.h
+#include linux/module.h
+#include linux/kernel.h
+#include linux/uaccess.h
+#include linux/v4l2-mediabus.h
+#include linux/platform_device.h
+
+#include dm3xx_ipipeif.h
+
+static void *__iomem ipipeif_base_addr;
+
+static inline u32 regr_if(u32 offset)
+{
+ return readl(ipipeif_base_addr + offset);
+}
+
+static inline void regw_if(u32 val, u32 offset)
+{
+ writel(val, ipipeif_base_addr + offset);
+}
+
+void ipipeif_set_enable()
+{
+ regw_if(1, IPIPEIF_ENABLE);
+}
+
+u32 ipipeif_get_enable(void)
+{
+ return regr_if(IPIPEIF_ENABLE);
+}
+
+int ipipeif_set_address(struct ipipeif *params, unsigned int address)
+{
+ u32 val;
+
+ if (params-source == 0)
+ return -EINVAL;
+
+ val = (params-adofs 5) IPIPEIF_ADOFS_LSB_MASK;
+ regw_if(val, IPIPEIF_ADOFS);
+
+ /* lower sixteen bit */
+ val = (address IPIPEIF_ADDRL_SHIFT) IPIPEIF_ADDRL_MASK;
+ regw_if(val, IPIPEIF_ADDRL);
+
+ /* upper next seven bit */
+ val = (address IPIPEIF_ADDRU_SHIFT) IPIPEIF_ADDRU_MASK;
+ regw_if(val, IPIPEIF_ADDRU);
+
+ return 0;
+}
+
+static void ipipeif_config_dpc(struct ipipeif_dpc *dpc)
+{
+ 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);
+}
+
+#define RD_DATA_15_2 0x7
+
+/*
+ * ipipeif_hw_setup() - This function sets up IPIPEIF
+ */
+int ipipeif_hw_setup(struct ipipeif *params, int device_type)
+{
+ enum v4l2_mbus_pixelcode isif_port_if;
+ unsigned int val;
+
+ if (params == NULL)
+ return -EINVAL;
+
+ /* Enable clock to IPIPEIF and IPIPE */
+ if (device_type == DM365)
+ vpss_enable_clock(VPSS_IPIPEIF_CLOCK, 1);
+
+ /* Combine all the fields to make CFG1 register of IPIPEIF */
+ val = params-mode ONESHOT_SHIFT;
+ val |= params-source INPSRC_SHIFT;
+ val |= params-clock_select CLKSEL_SHIFT;
+ val |= params-avg_filter AVGFILT_SHIFT;
+ val |= params-decimation DECIM_SHIFT;
+
+ if (device_type == DM355) {
+ val |= params-var.if_base.ialaw IALAW_SHIFT;
+ val |= params-var.if_base.pack_mode PACK8IN_SHIFT;
+ val |= params-var.if_base.clk_div CLKDIV_SHIFT;
+ val |= params-var.if_base.data_shift DATASFT_SHIFT;
+ } else {
+ /* DM365 IPIPE 5.1 */
+ val |= params-var.if_5_1.pack_mode PACK8IN_SHIFT;
+ val |= params-var.if_5_1.source1 INPSRC1_SHIFT;
+ if (params-source != IPIPEIF_SDRAM_YUV)
+ val |= params-var.if_5_1.data_shift DATASFT_SHIFT;
+ else
+ val = ~(RD_DATA_15_2 DATASFT_SHIFT);
+ }
+ regw_if(val, IPIPEIF_CFG1);
+
+ switch (params-source) {
+ case IPIPEIF_CCDC:
+ regw_if(params-gain, IPIPEIF_GAIN);
+ break;
+ case IPIPEIF_SDRAM_RAW:
+
