Re: [RFC PATCH 1/8] davinci: vpfe: add dm3xx IPIPEIF hardware support module
On Mon, Aug 29, 2011 at 08:49:30PM +0530, Hadli, Manjunath wrote: Sakari, Hi Manju, I have sent a fresh patch-set with your comments fixed and and some cleanup and reorg of my own- mainly the headers. Please review. I'll try to review at the patches when I have time. Also, I had to keep one of your comments without code change as I felt it was Ok to keep it here as it is only a local variable which actually gets the info from the device specific data structures. I removed the other however. Looking forward for your comments on further patches as well. -Manju On Thu, Jul 14, 2011 at 00:20:50, Sakari Ailus wrote: Hi Manju, Thanks for the patchset! I have a few comments on this patch below. I haven't read the rest of the patches yet so I may have more comments on this one when I do that. On Thu, Jun 30, 2011 at 06:43:10PM +0530, 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.ha...@ti.com Signed-off-by: Nagabhushana Netagunte nagabhushana.netagu...@ti.com --- drivers/media/video/davinci/dm3xx_ipipeif.c | 368 +++ include/media/davinci/dm3xx_ipipeif.h | 292 + 2 files changed, 660 insertions(+), 0 deletions(-) create mode 100644 drivers/media/video/davinci/dm3xx_ipipeif.c create mode 100644 include/media/davinci/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 000..36cb61b --- /dev/null +++ b/drivers/media/video/davinci/dm3xx_ipipeif.c @@ -0,0 +1,368 @@ ---code +#include linux/kernel.h #include linux/platform_device.h #include +linux/uaccess.h #include linux/io.h #include +linux/v4l2-mediabus.h #include media/davinci/dm3xx_ipipeif.h + +#define DM3550 +#define DM3651 + +static void *__iomem ipipeif_base_addr; This looks device specific. What about using dev_set/get_drvdata and remove this one? This one is actually gotten from the platform data structure in the manner you suggested but stored here for local usage. You always will get this pointer from other sources, and it will be the pointer to the very device you will be accessing. Look at the OMAP 3 ISP driver, for example: there are no such static variables. Basically keeping this in a static variable which is specific to a driver rather than the device is just wrong. -- Sakari Ailus sakari.ai...@iki.fi -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [RFC PATCH 1/8] davinci: vpfe: add dm3xx IPIPEIF hardware support module
Sakari,
I have sent a fresh patch-set with your comments fixed and and some
cleanup and reorg of my own- mainly the headers. Please review.
Also, I had to keep one of your comments without code change as I felt it was
Ok to keep it here as it is only a local variable which actually gets the info
from the device specific data structures. I removed the other however.
Looking forward for your comments on further patches as well.
-Manju
On Thu, Jul 14, 2011 at 00:20:50, Sakari Ailus wrote:
Hi Manju,
Thanks for the patchset!
I have a few comments on this patch below. I haven't read the rest of the
patches yet so I may have more comments on this one when I do that.
On Thu, Jun 30, 2011 at 06:43:10PM +0530, 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.ha...@ti.com
Signed-off-by: Nagabhushana Netagunte nagabhushana.netagu...@ti.com
---
drivers/media/video/davinci/dm3xx_ipipeif.c | 368
+++
include/media/davinci/dm3xx_ipipeif.h | 292 +
2 files changed, 660 insertions(+), 0 deletions(-) create mode
100644 drivers/media/video/davinci/dm3xx_ipipeif.c
create mode 100644 include/media/davinci/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 000..36cb61b
--- /dev/null
+++ b/drivers/media/video/davinci/dm3xx_ipipeif.c
@@ -0,0 +1,368 @@
---code
+#include linux/kernel.h #include linux/platform_device.h #include
+linux/uaccess.h #include linux/io.h #include
+linux/v4l2-mediabus.h #include media/davinci/dm3xx_ipipeif.h
+
+#define DM355 0
+#define DM365 1
+
+static void *__iomem ipipeif_base_addr;
This looks device specific. What about using dev_set/get_drvdata and remove
this one?
This one is actually gotten from the platform data structure in the manner you
suggested but stored here for local usage.
+static int device_type;
Ditto. Both should be in a device specific struct.
This one I have removed.
+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); }
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [RFC PATCH 1/8] davinci: vpfe: add dm3xx IPIPEIF hardware support module
Sakari,
Thank you for your comments. I agree with them and fix. Please comment on the
rest of the patches as well.
-Manju
On Thu, Jul 14, 2011 at 00:20:50, Sakari Ailus wrote:
Hi Manju,
Thanks for the patchset!
I have a few comments on this patch below. I haven't read the rest of the
patches yet so I may have more comments on this one when I do that.
On Thu, Jun 30, 2011 at 06:43:10PM +0530, 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.ha...@ti.com
Signed-off-by: Nagabhushana Netagunte nagabhushana.netagu...@ti.com
---
drivers/media/video/davinci/dm3xx_ipipeif.c | 368
+++
include/media/davinci/dm3xx_ipipeif.h | 292 +
2 files changed, 660 insertions(+), 0 deletions(-) create mode
100644 drivers/media/video/davinci/dm3xx_ipipeif.c
create mode 100644 include/media/davinci/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 000..36cb61b
--- /dev/null
+++ b/drivers/media/video/davinci/dm3xx_ipipeif.c
@@ -0,0 +1,368 @@
+/*
+* Copyright (C) 2011 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
+*
+* ipipe module to hold common functionality across DM355 and DM365 */
+#include linux/kernel.h #include linux/platform_device.h #include
+linux/uaccess.h #include linux/io.h #include
+linux/v4l2-mediabus.h #include media/davinci/dm3xx_ipipeif.h
+
+#define DM355 0
+#define DM365 1
+
+static void *__iomem ipipeif_base_addr;
This looks device specific. What about using dev_set/get_drvdata and remove
this one?
+static int device_type;
Ditto. Both should be in a device specific struct.
I will move both of the above to platform file.
+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(char en, unsigned int mode) {
+ 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)
+{
If address is a value for a register you should use u32.
Okay.
+ unsigned int val1;
+ unsigned int val;
+
+ if (params-source != 0) {
+ val = ((params-adofs 5) IPIPEIF_ADOFS_LSB_MASK);
+ regw_if(val, IPIPEIF_ADOFS);
You may do without val as well.
+ /* lower sixteen bit */
+ val = ((address IPIPEIF_ADDRL_SHIFT) IPIPEIF_ADDRL_MASK);
+ regw_if(val, IPIPEIF_ADDRL);
+
+ /* upper next seven bit */
+ val1 =
+ ((address IPIPEIF_ADDRU_SHIFT) IPIPEIF_ADDRU_MASK);
+ regw_if(val1, IPIPEIF_ADDRU);
+ } else
+ return -1;
What's -1? If this is an error, Ex codes should be used.
The error check should become first and the rest of the function may be
unindented by one tab stop.
Okay.
+ return 0;
+}
+
+static void ipipeif_config_dpc(struct ipipeif_dpc *dpc) {
+ u32 val;
+
+ if (dpc-en) {
+ val = ((dpc-en 1) IPIPEIF_DPC2_EN_SHIFT);
+ val |= (dpc-thr IPIPEIF_DPC2_THR_MASK);
+ } else
+ val = 0;
+
+ regw_if(val, IPIPEIF_DPC2);
+}
+
+/* This function sets up IPIPEIF and is called from
+ * ipipe_hw_setup()
+ */
+int ipipeif_hw_setup(struct ipipeif *params) {
+ enum v4l2_mbus_pixelcode isif_port_if;
+ unsigned int val1 = 0x7;
7 looks like a magic number.
Will fix.
+ unsigned int val;
+
+ if (NULL == params)
+ return -1;
Same here, and I can also see elsewhere.
Will fix.
+ /* 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
Re: [RFC PATCH 1/8] davinci: vpfe: add dm3xx IPIPEIF hardware support module
Hadli, Manjunath wrote: Sakari, Thank you for your comments. I agree with them and fix. Please comment on the rest of the patches as well. -Manju Hi Manju, I'll attempt to find more time for this. [clip] +/* 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) IPIPEIF prefix. Are these related to a particular register or a set of registers? One register. Will need to add the IPIPEIF prefix. Assuming CFG1 is the name of the register, what about IPIPEIF_CFG1_...? +/* DPC2 */ +#define IPIPEIF_DPC2_EN_SHIFT (12) +#define IPIPEIF_DPC2_THR_MASK (0xFFF) +#define IPIPEIF_DF_GAIN_EN_SHIFT (10) +#define IPIPEIF_DF_GAIN_MASK (0x3FF) +#define IPIPEIF_DF_GAIN_THR_MASK (0xFFF) Also all of these should have DPC2 prefix before DPC2 if they're all for the same register. Regards, -- Sakari Ailus sakari.ai...@iki.fi -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH 1/8] davinci: vpfe: add dm3xx IPIPEIF hardware support module
Hi Manju,
Thanks for the patchset!
I have a few comments on this patch below. I haven't read the rest of the
patches yet so I may have more comments on this one when I do that.
On Thu, Jun 30, 2011 at 06:43:10PM +0530, 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.ha...@ti.com
Signed-off-by: Nagabhushana Netagunte nagabhushana.netagu...@ti.com
---
drivers/media/video/davinci/dm3xx_ipipeif.c | 368
+++
include/media/davinci/dm3xx_ipipeif.h | 292 +
2 files changed, 660 insertions(+), 0 deletions(-)
create mode 100644 drivers/media/video/davinci/dm3xx_ipipeif.c
create mode 100644 include/media/davinci/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 000..36cb61b
--- /dev/null
+++ b/drivers/media/video/davinci/dm3xx_ipipeif.c
@@ -0,0 +1,368 @@
+/*
+* Copyright (C) 2011 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
+*
+* ipipe module to hold common functionality across DM355 and DM365
+*/
+#include linux/kernel.h
+#include linux/platform_device.h
+#include linux/uaccess.h
+#include linux/io.h
+#include linux/v4l2-mediabus.h
+#include media/davinci/dm3xx_ipipeif.h
+
+#define DM3550
+#define DM3651
+
+static void *__iomem ipipeif_base_addr;
This looks device specific. What about using dev_set/get_drvdata and remove
this one?
+static int device_type;
Ditto. Both should be in a device specific struct.
+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(char en, unsigned int mode)
+{
+ 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)
+{
If address is a value for a register you should use u32.
+ unsigned int val1;
+ unsigned int val;
+
+ if (params-source != 0) {
+ val = ((params-adofs 5) IPIPEIF_ADOFS_LSB_MASK);
+ regw_if(val, IPIPEIF_ADOFS);
You may do without val as well.
+ /* lower sixteen bit */
+ val = ((address IPIPEIF_ADDRL_SHIFT) IPIPEIF_ADDRL_MASK);
+ regw_if(val, IPIPEIF_ADDRL);
+
+ /* upper next seven bit */
+ val1 =
+ ((address IPIPEIF_ADDRU_SHIFT) IPIPEIF_ADDRU_MASK);
+ regw_if(val1, IPIPEIF_ADDRU);
+ } else
+ return -1;
What's -1? If this is an error, Ex codes should be used.
The error check should become first and the rest of the function may be
unindented by one tab stop.
+ return 0;
+}
+
+static void ipipeif_config_dpc(struct ipipeif_dpc *dpc)
+{
+ u32 val;
+
+ if (dpc-en) {
+ val = ((dpc-en 1) IPIPEIF_DPC2_EN_SHIFT);
+ val |= (dpc-thr IPIPEIF_DPC2_THR_MASK);
+ } else
+ val = 0;
+
+ regw_if(val, IPIPEIF_DPC2);
+}
+
+/* This function sets up IPIPEIF and is called from
+ * ipipe_hw_setup()
+ */
+int ipipeif_hw_setup(struct ipipeif *params)
+{
+ enum v4l2_mbus_pixelcode isif_port_if;
+ unsigned int val1 = 0x7;
7 looks like a magic number.
+ unsigned int val;
+
+ if (NULL == params)
+ return -1;
Same here, and I can also see elsewhere.
+ /* 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 |=
[RFC PATCH 1/8] davinci: vpfe: add dm3xx IPIPEIF hardware support module
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: Nagabhushana Netagunte nagabhushana.netagu...@ti.com
---
drivers/media/video/davinci/dm3xx_ipipeif.c | 368 +++
include/media/davinci/dm3xx_ipipeif.h | 292 +
2 files changed, 660 insertions(+), 0 deletions(-)
create mode 100644 drivers/media/video/davinci/dm3xx_ipipeif.c
create mode 100644 include/media/davinci/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 000..36cb61b
--- /dev/null
+++ b/drivers/media/video/davinci/dm3xx_ipipeif.c
@@ -0,0 +1,368 @@
+/*
+* Copyright (C) 2011 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
+*
+* ipipe module to hold common functionality across DM355 and DM365
+*/
+#include linux/kernel.h
+#include linux/platform_device.h
+#include linux/uaccess.h
+#include linux/io.h
+#include linux/v4l2-mediabus.h
+#include media/davinci/dm3xx_ipipeif.h
+
+#define DM355 0
+#define DM365 1
+
+static void *__iomem ipipeif_base_addr;
+static int device_type;
+
+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(char en, unsigned int mode)
+{
+ 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)
+{
+ unsigned int val1;
+ unsigned int val;
+
+ if (params-source != 0) {
+ 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 */
+ val1 =
+ ((address IPIPEIF_ADDRU_SHIFT) IPIPEIF_ADDRU_MASK);
+ regw_if(val1, IPIPEIF_ADDRU);
+ } else
+ return -1;
+
+ return 0;
+}
+
+static void ipipeif_config_dpc(struct ipipeif_dpc *dpc)
+{
+ u32 val;
+
+ if (dpc-en) {
+ val = ((dpc-en 1) IPIPEIF_DPC2_EN_SHIFT);
+ val |= (dpc-thr IPIPEIF_DPC2_THR_MASK);
+ } else
+ val = 0;
+
+ regw_if(val, IPIPEIF_DPC2);
+}
+
+/* This function sets up IPIPEIF and is called from
+ * ipipe_hw_setup()
+ */
+int ipipeif_hw_setup(struct ipipeif *params)
+{
+ enum v4l2_mbus_pixelcode isif_port_if;
+ unsigned int val1 = 0x7;
+ unsigned int val;
+
+ if (NULL == params)
+ return -1;
+
+ /* 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 != SDRAM_YUV)
+ val |= params-var.if_5_1.data_shift DATASFT_SHIFT;
+ else
+ val = (~(val1 DATASFT_SHIFT));
+ }
+ regw_if(val, IPIPEIF_GFG1);
+
+ switch (params-source) {
+ case CCDC:
+ {
+ regw_if(params-gain, IPIPEIF_GAIN);
+ break;
+
