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