| Message ID | 1250283745-5671-1-git-send-email-m-karicheri2@ti.com (mailing list archive) |
|---|---|
| State | RFC |
| Headers | show |
Quick review comments below - > -----Original Message----- > From: linux-media-owner@vger.kernel.org [mailto:linux-media- > owner@vger.kernel.org] On Behalf Of Karicheri, Muralidharan > Sent: Saturday, August 15, 2009 2:32 AM > To: linux-media@vger.kernel.org > Cc: davinci-linux-open-source@linux.davincidsp.com; > hverkuil@xs4all.nl; Karicheri, Muralidharan > Subject: [PATCH v1 - 4/5] V4L : vpif updates for DM6467 vpif capture > driver > > From: Muralidharan Karicheri <m-karicheri2@ti.com> > > Following changes done for vpif driver to support vpif capture:- > 1) Current version of display driver defined vpif register > space as part for vpif display platform driver resource > This is not correct since vpif is common across capture > and display drivers. So the resource iomap function is > moved to this module > 2) Since there are common registers, a spinlock is added for > mutual exclusion. > > This has incorporated comments against version v0 of the patch > series > > Reviewed-by: Hans Verkuil <hverkuil@xs4all.nl> > > Signed-off-by: Muralidharan Karicheri <m-karicheri2@ti.com> > --- > Applies to V4L-DVB linux-next repository > drivers/media/video/davinci/vpif.c | 76 > ++++++++++++++++++++++++++++++++--- > drivers/media/video/davinci/vpif.h | 48 ++++++++++++++--------- > 2 files changed, 98 insertions(+), 26 deletions(-) > > diff --git a/drivers/media/video/davinci/vpif.c > b/drivers/media/video/davinci/vpif.c > index aa77126..3b8eac3 100644 > --- a/drivers/media/video/davinci/vpif.c > +++ b/drivers/media/video/davinci/vpif.c > @@ -19,7 +19,11 @@ > > #include <linux/init.h> > #include <linux/module.h> > +#include <linux/platform_device.h> > +#include <linux/spinlock.h> > #include <linux/kernel.h> > +#include <linux/io.h> [Hiremath, Vaibhav] You may want to put one line gap here. > +#include <mach/hardware.h> > > #include "vpif.h" > > @@ -31,6 +35,12 @@ MODULE_LICENSE("GPL"); > #define VPIF_CH2_MAX_MODES (15) > #define VPIF_CH3_MAX_MODES (02) > > +static resource_size_t res_len; > +static struct resource *res; [Hiremath, Vaibhav] Do we really require this to be static variable? I think we can manage it to be local variable. > +spinlock_t vpif_lock; > + > +void __iomem *vpif_base; > + > static inline void vpif_wr_bit(u32 reg, u32 bit, u32 val) > { > if (val) > @@ -151,17 +161,17 @@ static void config_vpif_params(struct > vpif_params *vpifparams, > else if (config->capture_format) { > /* Set the polarity of various pins */ > vpif_wr_bit(reg, VPIF_CH_FID_POLARITY_BIT, > - vpifparams- > >params.raw_params.fid_pol); > + vpifparams->iface.fid_pol); > vpif_wr_bit(reg, VPIF_CH_V_VALID_POLARITY_BIT, > - vpifparams->params.raw_params.vd_pol); > + vpifparams->iface.vd_pol); > vpif_wr_bit(reg, VPIF_CH_H_VALID_POLARITY_BIT, > - vpifparams->params.raw_params.hd_pol); > + vpifparams->iface.hd_pol); > > value = regr(reg); > /* Set data width */ > value &= ((~(unsigned int)(0x3)) << > VPIF_CH_DATA_WIDTH_BIT); > - value |= ((vpifparams->params.raw_params.data_sz) > << > + value |= ((vpifparams->params.data_sz) << > VPIF_CH_DATA_WIDTH_BIT); > regw(value, reg); > } > @@ -227,8 +237,60 @@ int vpif_channel_getfid(u8 channel_id) > } > EXPORT_SYMBOL(vpif_channel_getfid); > > -void vpif_base_addr_init(void __iomem *base) > +static int __init vpif_probe(struct platform_device *pdev) > +{ > + int status = 0; > + > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > + if (!res) > + return -ENOENT; > + > + res_len = res->end - res->start + 1; > + > + res = request_mem_region(res->start, res_len, res->name); > + if (!res) > + return -EBUSY; > + > + vpif_base = ioremap(res->start, res_len); > + if (!vpif_base) { > + status = -EBUSY; > + goto fail; > + } > + > + spin_lock_init(&vpif_lock); > + dev_info(&pdev->dev, "vpif probe success\n"); > + return 0; > + > +fail: > + release_mem_region(res->start, res_len); > + return status; > +} > + > +static int vpif_remove(struct platform_device *pdev) > { > - vpif_base = base; > + iounmap(vpif_base); > + release_mem_region(res->start, res_len); > + return 0; > } > -EXPORT_SYMBOL(vpif_base_addr_init); > + > +static struct platform_driver vpif_driver = { > + .driver = { > + .name = "vpif", > + .owner = THIS_MODULE, > + }, > + .remove = __devexit_p(vpif_remove), > + .probe = vpif_probe, > +}; > + > +static void vpif_exit(void) > +{ > + platform_driver_unregister(&vpif_driver); > +} > + > +static int __init vpif_init(void) > +{ > + return platform_driver_register(&vpif_driver); > +} > +subsys_initcall(vpif_init); > +module_exit(vpif_exit); > + > diff --git a/drivers/media/video/davinci/vpif.h > b/drivers/media/video/davinci/vpif.h > index fca26dc..188841b 100644 > --- a/drivers/media/video/davinci/vpif.h > +++ b/drivers/media/video/davinci/vpif.h > @@ -19,6 +19,7 @@ > #include <linux/io.h> > #include <linux/videodev2.h> [Hiremath, Vaibhav] one line gap here. > #include <mach/hardware.h> > +#include <mach/dm646x.h> > > /* Maximum channel allowed */ > #define VPIF_NUM_CHANNELS (4) > @@ -26,7 +27,9 @@ > #define VPIF_DISPLAY_NUM_CHANNELS (2) > > /* Macros to read/write registers */ > -static void __iomem *vpif_base; > +extern void __iomem *vpif_base; > +extern spinlock_t vpif_lock; [Hiremath, Vaibhav] I think I would want to check compete driver. How these extern variables have been used. > + > #define regr(reg) readl((reg) + vpif_base) > #define regw(value, reg) writel(value, (reg + vpif_base)) > > @@ -280,6 +283,10 @@ static inline void enable_channel1(int enable) > /* inline function to enable interrupt for channel0 */ > static inline void channel0_intr_enable(int enable) > { > + unsigned long flags; > + > + spin_lock_irqsave(&vpif_lock, flags); > + > if (enable) { > regw((regr(VPIF_INTEN) | 0x10), VPIF_INTEN); > regw((regr(VPIF_INTEN_SET) | 0x10), VPIF_INTEN_SET); > @@ -292,11 +299,16 @@ static inline void channel0_intr_enable(int > enable) > regw((regr(VPIF_INTEN_SET) | VPIF_INTEN_FRAME_CH0), > VPIF_INTEN_SET); > } > + spin_unlock_irqrestore(&vpif_lock, flags); > } > > /* inline function to enable interrupt for channel1 */ > static inline void channel1_intr_enable(int enable) > { > + unsigned long flags; > + > + spin_lock_irqsave(&vpif_lock, flags); > + > if (enable) { > regw((regr(VPIF_INTEN) | 0x10), VPIF_INTEN); > regw((regr(VPIF_INTEN_SET) | 0x10), VPIF_INTEN_SET); > @@ -309,6 +321,7 @@ static inline void channel1_intr_enable(int > enable) > regw((regr(VPIF_INTEN_SET) | VPIF_INTEN_FRAME_CH1), > VPIF_INTEN_SET); > } > + spin_unlock_irqrestore(&vpif_lock, flags); > } > > /* inline function to set buffer addresses in case of Y/C non mux > mode */ > @@ -431,6 +444,10 @@ static inline void enable_channel3(int enable) > /* inline function to enable interrupt for channel2 */ > static inline void channel2_intr_enable(int enable) > { > + unsigned long flags; > + > + spin_lock_irqsave(&vpif_lock, flags); > + > if (enable) { > regw((regr(VPIF_INTEN) | 0x10), VPIF_INTEN); > regw((regr(VPIF_INTEN_SET) | 0x10), VPIF_INTEN_SET); > @@ -442,11 +459,16 @@ static inline void channel2_intr_enable(int > enable) > regw((regr(VPIF_INTEN_SET) | VPIF_INTEN_FRAME_CH2), > VPIF_INTEN_SET); > } > + spin_unlock_irqrestore(&vpif_lock, flags); > } > > /* inline function to enable interrupt for channel3 */ > static inline void channel3_intr_enable(int enable) > { > + unsigned long flags; > + > + spin_lock_irqsave(&vpif_lock, flags); > + > if (enable) { > regw((regr(VPIF_INTEN) | 0x10), VPIF_INTEN); > regw((regr(VPIF_INTEN_SET) | 0x10), VPIF_INTEN_SET); > @@ -459,6 +481,7 @@ static inline void channel3_intr_enable(int > enable) > regw((regr(VPIF_INTEN_SET) | VPIF_INTEN_FRAME_CH3), > VPIF_INTEN_SET); > } > + spin_unlock_irqrestore(&vpif_lock, flags); > } > > /* inline function to enable raw vbi data for channel2 */ > @@ -571,7 +594,7 @@ struct vpif_channel_config_params { > v4l2_std_id stdid; > }; > > -struct vpif_interface; > +struct vpif_video_params; > struct vpif_params; > struct vpif_vbi_params; > > @@ -579,13 +602,6 @@ int vpif_set_video_params(struct vpif_params > *vpifparams, u8 channel_id); > void vpif_set_vbi_display_params(struct vpif_vbi_params *vbiparams, > u8 channel_id); > int vpif_channel_getfid(u8 channel_id); > -void vpif_base_addr_init(void __iomem *base); > - > -/* Enumerated data types */ > -enum vpif_capture_pinpol { > - VPIF_CAPTURE_PINPOL_SAME = 0, > - VPIF_CAPTURE_PINPOL_INVERT = 1 > -}; > > enum data_size { > _8BITS = 0, > @@ -593,13 +609,6 @@ enum data_size { > _12BITS, > }; > > -struct vpif_capture_params_raw { > - enum data_size data_sz; > - enum vpif_capture_pinpol fid_pol; > - enum vpif_capture_pinpol vd_pol; > - enum vpif_capture_pinpol hd_pol; > -}; > - > /* Structure for vpif parameters for raw vbi data */ > struct vpif_vbi_params { > __u32 hstart0; /* Horizontal start of raw vbi data for first > field */ > @@ -613,18 +622,19 @@ struct vpif_vbi_params { > }; > > /* structure for vpif parameters */ > -struct vpif_interface { > +struct vpif_video_params { > __u8 storage_mode; /* Indicates field or frame mode */ > unsigned long hpitch; > v4l2_std_id stdid; > }; > > struct vpif_params { > - struct vpif_interface video_params; > + struct vpif_interface iface; > + struct vpif_video_params video_params; > struct vpif_channel_config_params std_info; > union param { > struct vpif_vbi_params vbi_params; > - struct vpif_capture_params_raw raw_params; > + enum data_size data_sz; > } params; > }; > > -- > 1.6.0.4 > > -- > To unsubscribe from this list: send the line "unsubscribe linux- > media" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Vaibhav, I don't see any serious issues raised here. I can send another patch to fix this if needed. Regards, Murali >> +#include <linux/spinlock.h> >> #include <linux/kernel.h> >> +#include <linux/io.h> >[Hiremath, Vaibhav] You may want to put one line gap here. Ok. Just editorial. >> +#include <mach/hardware.h> >> >> #include "vpif.h" >> >> @@ -31,6 +35,12 @@ MODULE_LICENSE("GPL"); >> #define VPIF_CH2_MAX_MODES (15) >> #define VPIF_CH3_MAX_MODES (02) >> >> +static resource_size_t res_len; >> +static struct resource *res; >[Hiremath, Vaibhav] Do we really require this to be static variable? >I think we can manage it to be local variable. Yes. We could. Probably change it with a new patch. Don't want to hold up merge because of this. > >> +spinlock_t vpif_lock; >> + >> +void __iomem *vpif_base; >> + >> static inline void vpif_wr_bit(u32 reg, u32 bit, u32 val) >> { >> if (val) >> @@ -151,17 +161,17 @@ static void config_vpif_params(struct >> vpif_params *vpifparams, >> else if (config->capture_format) { >> /* Set the polarity of various pins */ >> vpif_wr_bit(reg, VPIF_CH_FID_POLARITY_BIT, >> - vpifparams- >> >params.raw_params.fid_pol); >> + vpifparams->iface.fid_pol); >> vpif_wr_bit(reg, VPIF_CH_V_VALID_POLARITY_BIT, >> - vpifparams->params.raw_params.vd_pol); >> + vpifparams->iface.vd_pol); >> vpif_wr_bit(reg, VPIF_CH_H_VALID_POLARITY_BIT, >> - vpifparams->params.raw_params.hd_pol); >> + vpifparams->iface.hd_pol); >> >> value = regr(reg); >> /* Set data width */ >> value &= ((~(unsigned int)(0x3)) << >> VPIF_CH_DATA_WIDTH_BIT); >> - value |= ((vpifparams->params.raw_params.data_sz) >> << >> + value |= ((vpifparams->params.data_sz) << >> VPIF_CH_DATA_WIDTH_BIT); >> regw(value, reg); >> } >> @@ -227,8 +237,60 @@ int vpif_channel_getfid(u8 channel_id) >> } >> EXPORT_SYMBOL(vpif_channel_getfid); >> >> -void vpif_base_addr_init(void __iomem *base) >> +static int __init vpif_probe(struct platform_device *pdev) >> +{ >> + int status = 0; >> + >> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); >> + if (!res) >> + return -ENOENT; >> + >> + res_len = res->end - res->start + 1; >> + >> + res = request_mem_region(res->start, res_len, res->name); >> + if (!res) >> + return -EBUSY; >> + >> + vpif_base = ioremap(res->start, res_len); >> + if (!vpif_base) { >> + status = -EBUSY; >> + goto fail; >> + } >> + >> + spin_lock_init(&vpif_lock); >> + dev_info(&pdev->dev, "vpif probe success\n"); >> + return 0; >> + >> +fail: >> + release_mem_region(res->start, res_len); >> + return status; >> +} >> + >> +static int vpif_remove(struct platform_device *pdev) >> { >> - vpif_base = base; >> + iounmap(vpif_base); >> + release_mem_region(res->start, res_len); >> + return 0; >> } >> -EXPORT_SYMBOL(vpif_base_addr_init); >> + >> +static struct platform_driver vpif_driver = { >> + .driver = { >> + .name = "vpif", >> + .owner = THIS_MODULE, >> + }, >> + .remove = __devexit_p(vpif_remove), >> + .probe = vpif_probe, >> +}; >> + >> +static void vpif_exit(void) >> +{ >> + platform_driver_unregister(&vpif_driver); >> +} >> + >> +static int __init vpif_init(void) >> +{ >> + return platform_driver_register(&vpif_driver); >> +} >> +subsys_initcall(vpif_init); >> +module_exit(vpif_exit); >> + >> diff --git a/drivers/media/video/davinci/vpif.h >> b/drivers/media/video/davinci/vpif.h >> index fca26dc..188841b 100644 >> --- a/drivers/media/video/davinci/vpif.h >> +++ b/drivers/media/video/davinci/vpif.h >> @@ -19,6 +19,7 @@ >> #include <linux/io.h> >> #include <linux/videodev2.h> >[Hiremath, Vaibhav] one line gap here. Again editorial. >> #include <mach/hardware.h> >> +#include <mach/dm646x.h> >> >> /* Maximum channel allowed */ >> #define VPIF_NUM_CHANNELS (4) >> @@ -26,7 +27,9 @@ >> #define VPIF_DISPLAY_NUM_CHANNELS (2) >> >> /* Macros to read/write registers */ >> -static void __iomem *vpif_base; >> +extern void __iomem *vpif_base; >> +extern spinlock_t vpif_lock; >[Hiremath, Vaibhav] I think I would want to check compete driver. How these >extern variables have been used. > Let me know if you find some thing wrong in the driver. At this time, I just don't see any issues with this. >> + >> #define regr(reg) readl((reg) + vpif_base) >> #define regw(value, reg) writel(value, (reg + vpif_base)) >> >> @@ -280,6 +283,10 @@ static inline void enable_channel1(int enable) >> /* inline function to enable interrupt for channel0 */ >> static inline void channel0_intr_enable(int enable) >> { >> + unsigned long flags; >> + >> + spin_lock_irqsave(&vpif_lock, flags); >> + >> if (enable) { >> regw((regr(VPIF_INTEN) | 0x10), VPIF_INTEN); >> regw((regr(VPIF_INTEN_SET) | 0x10), VPIF_INTEN_SET); >> @@ -292,11 +299,16 @@ static inline void channel0_intr_enable(int >> enable) >> regw((regr(VPIF_INTEN_SET) | VPIF_INTEN_FRAME_CH0), >> VPIF_INTEN_SET); >> } >> + spin_unlock_irqrestore(&vpif_lock, flags); >> } >> >> /* inline function to enable interrupt for channel1 */ >> static inline void channel1_intr_enable(int enable) >> { >> + unsigned long flags; >> + >> + spin_lock_irqsave(&vpif_lock, flags); >> + >> if (enable) { >> regw((regr(VPIF_INTEN) | 0x10), VPIF_INTEN); >> regw((regr(VPIF_INTEN_SET) | 0x10), VPIF_INTEN_SET); >> @@ -309,6 +321,7 @@ static inline void channel1_intr_enable(int >> enable) >> regw((regr(VPIF_INTEN_SET) | VPIF_INTEN_FRAME_CH1), >> VPIF_INTEN_SET); >> } >> + spin_unlock_irqrestore(&vpif_lock, flags); >> } >> >> /* inline function to set buffer addresses in case of Y/C non mux >> mode */ >> @@ -431,6 +444,10 @@ static inline void enable_channel3(int enable) >> /* inline function to enable interrupt for channel2 */ >> static inline void channel2_intr_enable(int enable) >> { >> + unsigned long flags; >> + >> + spin_lock_irqsave(&vpif_lock, flags); >> + >> if (enable) { >> regw((regr(VPIF_INTEN) | 0x10), VPIF_INTEN); >> regw((regr(VPIF_INTEN_SET) | 0x10), VPIF_INTEN_SET); >> @@ -442,11 +459,16 @@ static inline void channel2_intr_enable(int >> enable) >> regw((regr(VPIF_INTEN_SET) | VPIF_INTEN_FRAME_CH2), >> VPIF_INTEN_SET); >> } >> + spin_unlock_irqrestore(&vpif_lock, flags); >> } >> >> /* inline function to enable interrupt for channel3 */ >> static inline void channel3_intr_enable(int enable) >> { >> + unsigned long flags; >> + >> + spin_lock_irqsave(&vpif_lock, flags); >> + >> if (enable) { >> regw((regr(VPIF_INTEN) | 0x10), VPIF_INTEN); >> regw((regr(VPIF_INTEN_SET) | 0x10), VPIF_INTEN_SET); >> @@ -459,6 +481,7 @@ static inline void channel3_intr_enable(int >> enable) >> regw((regr(VPIF_INTEN_SET) | VPIF_INTEN_FRAME_CH3), >> VPIF_INTEN_SET); >> } >> + spin_unlock_irqrestore(&vpif_lock, flags); >> } >> >> /* inline function to enable raw vbi data for channel2 */ >> @@ -571,7 +594,7 @@ struct vpif_channel_config_params { >> v4l2_std_id stdid; >> }; >> >> -struct vpif_interface; >> +struct vpif_video_params; >> struct vpif_params; >> struct vpif_vbi_params; >> >> @@ -579,13 +602,6 @@ int vpif_set_video_params(struct vpif_params >> *vpifparams, u8 channel_id); >> void vpif_set_vbi_display_params(struct vpif_vbi_params *vbiparams, >> u8 channel_id); >> int vpif_channel_getfid(u8 channel_id); >> -void vpif_base_addr_init(void __iomem *base); >> - >> -/* Enumerated data types */ >> -enum vpif_capture_pinpol { >> - VPIF_CAPTURE_PINPOL_SAME = 0, >> - VPIF_CAPTURE_PINPOL_INVERT = 1 >> -}; >> >> enum data_size { >> _8BITS = 0, >> @@ -593,13 +609,6 @@ enum data_size { >> _12BITS, >> }; >> >> -struct vpif_capture_params_raw { >> - enum data_size data_sz; >> - enum vpif_capture_pinpol fid_pol; >> - enum vpif_capture_pinpol vd_pol; >> - enum vpif_capture_pinpol hd_pol; >> -}; >> - >> /* Structure for vpif parameters for raw vbi data */ >> struct vpif_vbi_params { >> __u32 hstart0; /* Horizontal start of raw vbi data for first >> field */ >> @@ -613,18 +622,19 @@ struct vpif_vbi_params { >> }; >> >> /* structure for vpif parameters */ >> -struct vpif_interface { >> +struct vpif_video_params { >> __u8 storage_mode; /* Indicates field or frame mode */ >> unsigned long hpitch; >> v4l2_std_id stdid; >> }; >> >> struct vpif_params { >> - struct vpif_interface video_params; >> + struct vpif_interface iface; >> + struct vpif_video_params video_params; >> struct vpif_channel_config_params std_info; >> union param { >> struct vpif_vbi_params vbi_params; >> - struct vpif_capture_params_raw raw_params; >> + enum data_size data_sz; >> } params; >> }; >> >> -- >> 1.6.0.4 >> >> -- >> To unsubscribe from this list: send the line "unsubscribe linux- >> media" in >> the body of a message to majordomo@vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
H Murali, > -----Original Message----- > From: Karicheri, Muralidharan > Sent: Monday, August 17, 2009 9:38 PM > To: Hiremath, Vaibhav; linux-media@vger.kernel.org > Cc: davinci-linux-open-source@linux.davincidsp.com; > hverkuil@xs4all.nl > Subject: RE: [PATCH v1 - 4/5] V4L : vpif updates for DM6467 vpif > capture driver > > Vaibhav, > > I don't see any serious issues raised here. I can send another patch > to fix this if needed. [Hiremath, Vaibhav] yes most of them are editorial, as I mentioned I just reviewed it quickly. But as far as static variables are concerned I still think we can avoid them completely, again it's not critical though. As I mentioned I will have to look for extern variables, how they have been used and stuff like that. As of now, I am ok if it gets merged. > > Regards, > Murali > >> +#include <linux/spinlock.h> > >> #include <linux/kernel.h> > >> +#include <linux/io.h> > >[Hiremath, Vaibhav] You may want to put one line gap here. > Ok. Just editorial. > >> +#include <mach/hardware.h> > >> > >> #include "vpif.h" > >> > >> @@ -31,6 +35,12 @@ MODULE_LICENSE("GPL"); > >> #define VPIF_CH2_MAX_MODES (15) > >> #define VPIF_CH3_MAX_MODES (02) > >> > >> +static resource_size_t res_len; > >> +static struct resource *res; > >[Hiremath, Vaibhav] Do we really require this to be static > variable? > >I think we can manage it to be local variable. > Yes. We could. Probably change it with a new patch. Don't want to > hold up merge because of this. > > > >> +spinlock_t vpif_lock; > >> + > >> +void __iomem *vpif_base; > >> + > >> static inline void vpif_wr_bit(u32 reg, u32 bit, u32 val) > >> { > >> if (val) > >> @@ -151,17 +161,17 @@ static void config_vpif_params(struct > >> vpif_params *vpifparams, > >> else if (config->capture_format) { > >> /* Set the polarity of various pins */ > >> vpif_wr_bit(reg, VPIF_CH_FID_POLARITY_BIT, > >> - vpifparams- > >> >params.raw_params.fid_pol); > >> + vpifparams->iface.fid_pol); > >> vpif_wr_bit(reg, VPIF_CH_V_VALID_POLARITY_BIT, > >> - vpifparams->params.raw_params.vd_pol); > >> + vpifparams->iface.vd_pol); > >> vpif_wr_bit(reg, VPIF_CH_H_VALID_POLARITY_BIT, > >> - vpifparams->params.raw_params.hd_pol); > >> + vpifparams->iface.hd_pol); > >> > >> value = regr(reg); > >> /* Set data width */ > >> value &= ((~(unsigned int)(0x3)) << > >> VPIF_CH_DATA_WIDTH_BIT); > >> - value |= ((vpifparams->params.raw_params.data_sz) > >> << > >> + value |= ((vpifparams->params.data_sz) << > >> VPIF_CH_DATA_WIDTH_BIT); > >> regw(value, reg); > >> } > >> @@ -227,8 +237,60 @@ int vpif_channel_getfid(u8 channel_id) > >> } > >> EXPORT_SYMBOL(vpif_channel_getfid); > >> > >> -void vpif_base_addr_init(void __iomem *base) > >> +static int __init vpif_probe(struct platform_device *pdev) > >> +{ > >> + int status = 0; > >> + > >> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > >> + if (!res) > >> + return -ENOENT; > >> + > >> + res_len = res->end - res->start + 1; > >> + > >> + res = request_mem_region(res->start, res_len, res->name); > >> + if (!res) > >> + return -EBUSY; > >> + > >> + vpif_base = ioremap(res->start, res_len); > >> + if (!vpif_base) { > >> + status = -EBUSY; > >> + goto fail; > >> + } > >> + > >> + spin_lock_init(&vpif_lock); > >> + dev_info(&pdev->dev, "vpif probe success\n"); > >> + return 0; > >> + > >> +fail: > >> + release_mem_region(res->start, res_len); > >> + return status; > >> +} > >> + > >> +static int vpif_remove(struct platform_device *pdev) > >> { > >> - vpif_base = base; > >> + iounmap(vpif_base); > >> + release_mem_region(res->start, res_len); > >> + return 0; > >> } > >> -EXPORT_SYMBOL(vpif_base_addr_init); > >> + > >> +static struct platform_driver vpif_driver = { > >> + .driver = { > >> + .name = "vpif", > >> + .owner = THIS_MODULE, > >> + }, > >> + .remove = __devexit_p(vpif_remove), > >> + .probe = vpif_probe, > >> +}; > >> + > >> +static void vpif_exit(void) > >> +{ > >> + platform_driver_unregister(&vpif_driver); > >> +} > >> + > >> +static int __init vpif_init(void) > >> +{ > >> + return platform_driver_register(&vpif_driver); > >> +} > >> +subsys_initcall(vpif_init); > >> +module_exit(vpif_exit); > >> + > >> diff --git a/drivers/media/video/davinci/vpif.h > >> b/drivers/media/video/davinci/vpif.h > >> index fca26dc..188841b 100644 > >> --- a/drivers/media/video/davinci/vpif.h > >> +++ b/drivers/media/video/davinci/vpif.h > >> @@ -19,6 +19,7 @@ > >> #include <linux/io.h> > >> #include <linux/videodev2.h> > >[Hiremath, Vaibhav] one line gap here. > Again editorial. > >> #include <mach/hardware.h> > >> +#include <mach/dm646x.h> > >> > >> /* Maximum channel allowed */ > >> #define VPIF_NUM_CHANNELS (4) > >> @@ -26,7 +27,9 @@ > >> #define VPIF_DISPLAY_NUM_CHANNELS (2) > >> > >> /* Macros to read/write registers */ > >> -static void __iomem *vpif_base; > >> +extern void __iomem *vpif_base; > >> +extern spinlock_t vpif_lock; > >[Hiremath, Vaibhav] I think I would want to check compete driver. > How these > >extern variables have been used. > > > Let me know if you find some thing wrong in the driver. At this > time, I just don't see any issues with this. > >> + > >> #define regr(reg) readl((reg) + vpif_base) > >> #define regw(value, reg) writel(value, (reg + vpif_base)) > >> > >> @@ -280,6 +283,10 @@ static inline void enable_channel1(int > enable) > >> /* inline function to enable interrupt for channel0 */ > >> static inline void channel0_intr_enable(int enable) > >> { > >> + unsigned long flags; > >> + > >> + spin_lock_irqsave(&vpif_lock, flags); > >> + > >> if (enable) { > >> regw((regr(VPIF_INTEN) | 0x10), VPIF_INTEN); > >> regw((regr(VPIF_INTEN_SET) | 0x10), VPIF_INTEN_SET); > >> @@ -292,11 +299,16 @@ static inline void channel0_intr_enable(int > >> enable) > >> regw((regr(VPIF_INTEN_SET) | VPIF_INTEN_FRAME_CH0), > >> VPIF_INTEN_SET); > >> } > >> + spin_unlock_irqrestore(&vpif_lock, flags); > >> } > >> > >> /* inline function to enable interrupt for channel1 */ > >> static inline void channel1_intr_enable(int enable) > >> { > >> + unsigned long flags; > >> + > >> + spin_lock_irqsave(&vpif_lock, flags); > >> + > >> if (enable) { > >> regw((regr(VPIF_INTEN) | 0x10), VPIF_INTEN); > >> regw((regr(VPIF_INTEN_SET) | 0x10), VPIF_INTEN_SET); > >> @@ -309,6 +321,7 @@ static inline void channel1_intr_enable(int > >> enable) > >> regw((regr(VPIF_INTEN_SET) | VPIF_INTEN_FRAME_CH1), > >> VPIF_INTEN_SET); > >> } > >> + spin_unlock_irqrestore(&vpif_lock, flags); > >> } > >> > >> /* inline function to set buffer addresses in case of Y/C non > mux > >> mode */ > >> @@ -431,6 +444,10 @@ static inline void enable_channel3(int > enable) > >> /* inline function to enable interrupt for channel2 */ > >> static inline void channel2_intr_enable(int enable) > >> { > >> + unsigned long flags; > >> + > >> + spin_lock_irqsave(&vpif_lock, flags); > >> + > >> if (enable) { > >> regw((regr(VPIF_INTEN) | 0x10), VPIF_INTEN); > >> regw((regr(VPIF_INTEN_SET) | 0x10), VPIF_INTEN_SET); > >> @@ -442,11 +459,16 @@ static inline void channel2_intr_enable(int > >> enable) > >> regw((regr(VPIF_INTEN_SET) | VPIF_INTEN_FRAME_CH2), > >> VPIF_INTEN_SET); > >> } > >> + spin_unlock_irqrestore(&vpif_lock, flags); > >> } > >> > >> /* inline function to enable interrupt for channel3 */ > >> static inline void channel3_intr_enable(int enable) > >> { > >> + unsigned long flags; > >> + > >> + spin_lock_irqsave(&vpif_lock, flags); > >> + > >> if (enable) { > >> regw((regr(VPIF_INTEN) | 0x10), VPIF_INTEN); > >> regw((regr(VPIF_INTEN_SET) | 0x10), VPIF_INTEN_SET); > >> @@ -459,6 +481,7 @@ static inline void channel3_intr_enable(int > >> enable) > >> regw((regr(VPIF_INTEN_SET) | VPIF_INTEN_FRAME_CH3), > >> VPIF_INTEN_SET); > >> } > >> + spin_unlock_irqrestore(&vpif_lock, flags); > >> } > >> > >> /* inline function to enable raw vbi data for channel2 */ > >> @@ -571,7 +594,7 @@ struct vpif_channel_config_params { > >> v4l2_std_id stdid; > >> }; > >> > >> -struct vpif_interface; > >> +struct vpif_video_params; > >> struct vpif_params; > >> struct vpif_vbi_params; > >> > >> @@ -579,13 +602,6 @@ int vpif_set_video_params(struct vpif_params > >> *vpifparams, u8 channel_id); > >> void vpif_set_vbi_display_params(struct vpif_vbi_params > *vbiparams, > >> u8 channel_id); > >> int vpif_channel_getfid(u8 channel_id); > >> -void vpif_base_addr_init(void __iomem *base); > >> - > >> -/* Enumerated data types */ > >> -enum vpif_capture_pinpol { > >> - VPIF_CAPTURE_PINPOL_SAME = 0, > >> - VPIF_CAPTURE_PINPOL_INVERT = 1 > >> -}; > >> > >> enum data_size { > >> _8BITS = 0, > >> @@ -593,13 +609,6 @@ enum data_size { > >> _12BITS, > >> }; > >> > >> -struct vpif_capture_params_raw { > >> - enum data_size data_sz; > >> - enum vpif_capture_pinpol fid_pol; > >> - enum vpif_capture_pinpol vd_pol; > >> - enum vpif_capture_pinpol hd_pol; > >> -}; > >> - > >> /* Structure for vpif parameters for raw vbi data */ > >> struct vpif_vbi_params { > >> __u32 hstart0; /* Horizontal start of raw vbi data for first > >> field */ > >> @@ -613,18 +622,19 @@ struct vpif_vbi_params { > >> }; > >> > >> /* structure for vpif parameters */ > >> -struct vpif_interface { > >> +struct vpif_video_params { > >> __u8 storage_mode; /* Indicates field or frame mode */ > >> unsigned long hpitch; > >> v4l2_std_id stdid; > >> }; > >> > >> struct vpif_params { > >> - struct vpif_interface video_params; > >> + struct vpif_interface iface; > >> + struct vpif_video_params video_params; > >> struct vpif_channel_config_params std_info; > >> union param { > >> struct vpif_vbi_params vbi_params; > >> - struct vpif_capture_params_raw raw_params; > >> + enum data_size data_sz; > >> } params; > >> }; > >> > >> -- > >> 1.6.0.4 > >> > >> -- > >> To unsubscribe from this list: send the line "unsubscribe linux- > >> media" in > >> the body of a message to majordomo@vger.kernel.org > >> More majordomo info at http://vger.kernel.org/majordomo- > info.html -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Yes. I will send another patch later to fix the static variables. Murali Karicheri Software Design Engineer Texas Instruments Inc. Germantown, MD 20874 new phone: 301-407-9583 Old Phone : 301-515-3736 (will be deprecated) email: m-karicheri2@ti.com >-----Original Message----- >From: Hiremath, Vaibhav >Sent: Monday, August 17, 2009 12:35 PM >To: Karicheri, Muralidharan; linux-media@vger.kernel.org >Cc: davinci-linux-open-source@linux.davincidsp.com; hverkuil@xs4all.nl >Subject: RE: [PATCH v1 - 4/5] V4L : vpif updates for DM6467 vpif capture >driver > >H Murali, > >> -----Original Message----- >> From: Karicheri, Muralidharan >> Sent: Monday, August 17, 2009 9:38 PM >> To: Hiremath, Vaibhav; linux-media@vger.kernel.org >> Cc: davinci-linux-open-source@linux.davincidsp.com; >> hverkuil@xs4all.nl >> Subject: RE: [PATCH v1 - 4/5] V4L : vpif updates for DM6467 vpif >> capture driver >> >> Vaibhav, >> >> I don't see any serious issues raised here. I can send another patch >> to fix this if needed. >[Hiremath, Vaibhav] yes most of them are editorial, as I mentioned I just >reviewed it quickly. > >But as far as static variables are concerned I still think we can avoid >them completely, again it's not critical though. > >As I mentioned I will have to look for extern variables, how they have been >used and stuff like that. >As of now, I am ok if it gets merged. > >> >> Regards, >> Murali >> >> +#include <linux/spinlock.h> >> >> #include <linux/kernel.h> >> >> +#include <linux/io.h> >> >[Hiremath, Vaibhav] You may want to put one line gap here. >> Ok. Just editorial. >> >> +#include <mach/hardware.h> >> >> >> >> #include "vpif.h" >> >> >> >> @@ -31,6 +35,12 @@ MODULE_LICENSE("GPL"); >> >> #define VPIF_CH2_MAX_MODES (15) >> >> #define VPIF_CH3_MAX_MODES (02) >> >> >> >> +static resource_size_t res_len; >> >> +static struct resource *res; >> >[Hiremath, Vaibhav] Do we really require this to be static >> variable? >> >I think we can manage it to be local variable. >> Yes. We could. Probably change it with a new patch. Don't want to >> hold up merge because of this. >> > >> >> +spinlock_t vpif_lock; >> >> + >> >> +void __iomem *vpif_base; >> >> + >> >> static inline void vpif_wr_bit(u32 reg, u32 bit, u32 val) >> >> { >> >> if (val) >> >> @@ -151,17 +161,17 @@ static void config_vpif_params(struct >> >> vpif_params *vpifparams, >> >> else if (config->capture_format) { >> >> /* Set the polarity of various pins */ >> >> vpif_wr_bit(reg, VPIF_CH_FID_POLARITY_BIT, >> >> - vpifparams- >> >> >params.raw_params.fid_pol); >> >> + vpifparams->iface.fid_pol); >> >> vpif_wr_bit(reg, VPIF_CH_V_VALID_POLARITY_BIT, >> >> - vpifparams->params.raw_params.vd_pol); >> >> + vpifparams->iface.vd_pol); >> >> vpif_wr_bit(reg, VPIF_CH_H_VALID_POLARITY_BIT, >> >> - vpifparams->params.raw_params.hd_pol); >> >> + vpifparams->iface.hd_pol); >> >> >> >> value = regr(reg); >> >> /* Set data width */ >> >> value &= ((~(unsigned int)(0x3)) << >> >> VPIF_CH_DATA_WIDTH_BIT); >> >> - value |= ((vpifparams->params.raw_params.data_sz) >> >> << >> >> + value |= ((vpifparams->params.data_sz) << >> >> VPIF_CH_DATA_WIDTH_BIT); >> >> regw(value, reg); >> >> } >> >> @@ -227,8 +237,60 @@ int vpif_channel_getfid(u8 channel_id) >> >> } >> >> EXPORT_SYMBOL(vpif_channel_getfid); >> >> >> >> -void vpif_base_addr_init(void __iomem *base) >> >> +static int __init vpif_probe(struct platform_device *pdev) >> >> +{ >> >> + int status = 0; >> >> + >> >> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); >> >> + if (!res) >> >> + return -ENOENT; >> >> + >> >> + res_len = res->end - res->start + 1; >> >> + >> >> + res = request_mem_region(res->start, res_len, res->name); >> >> + if (!res) >> >> + return -EBUSY; >> >> + >> >> + vpif_base = ioremap(res->start, res_len); >> >> + if (!vpif_base) { >> >> + status = -EBUSY; >> >> + goto fail; >> >> + } >> >> + >> >> + spin_lock_init(&vpif_lock); >> >> + dev_info(&pdev->dev, "vpif probe success\n"); >> >> + return 0; >> >> + >> >> +fail: >> >> + release_mem_region(res->start, res_len); >> >> + return status; >> >> +} >> >> + >> >> +static int vpif_remove(struct platform_device *pdev) >> >> { >> >> - vpif_base = base; >> >> + iounmap(vpif_base); >> >> + release_mem_region(res->start, res_len); >> >> + return 0; >> >> } >> >> -EXPORT_SYMBOL(vpif_base_addr_init); >> >> + >> >> +static struct platform_driver vpif_driver = { >> >> + .driver = { >> >> + .name = "vpif", >> >> + .owner = THIS_MODULE, >> >> + }, >> >> + .remove = __devexit_p(vpif_remove), >> >> + .probe = vpif_probe, >> >> +}; >> >> + >> >> +static void vpif_exit(void) >> >> +{ >> >> + platform_driver_unregister(&vpif_driver); >> >> +} >> >> + >> >> +static int __init vpif_init(void) >> >> +{ >> >> + return platform_driver_register(&vpif_driver); >> >> +} >> >> +subsys_initcall(vpif_init); >> >> +module_exit(vpif_exit); >> >> + >> >> diff --git a/drivers/media/video/davinci/vpif.h >> >> b/drivers/media/video/davinci/vpif.h >> >> index fca26dc..188841b 100644 >> >> --- a/drivers/media/video/davinci/vpif.h >> >> +++ b/drivers/media/video/davinci/vpif.h >> >> @@ -19,6 +19,7 @@ >> >> #include <linux/io.h> >> >> #include <linux/videodev2.h> >> >[Hiremath, Vaibhav] one line gap here. >> Again editorial. >> >> #include <mach/hardware.h> >> >> +#include <mach/dm646x.h> >> >> >> >> /* Maximum channel allowed */ >> >> #define VPIF_NUM_CHANNELS (4) >> >> @@ -26,7 +27,9 @@ >> >> #define VPIF_DISPLAY_NUM_CHANNELS (2) >> >> >> >> /* Macros to read/write registers */ >> >> -static void __iomem *vpif_base; >> >> +extern void __iomem *vpif_base; >> >> +extern spinlock_t vpif_lock; >> >[Hiremath, Vaibhav] I think I would want to check compete driver. >> How these >> >extern variables have been used. >> > >> Let me know if you find some thing wrong in the driver. At this >> time, I just don't see any issues with this. >> >> + >> >> #define regr(reg) readl((reg) + vpif_base) >> >> #define regw(value, reg) writel(value, (reg + vpif_base)) >> >> >> >> @@ -280,6 +283,10 @@ static inline void enable_channel1(int >> enable) >> >> /* inline function to enable interrupt for channel0 */ >> >> static inline void channel0_intr_enable(int enable) >> >> { >> >> + unsigned long flags; >> >> + >> >> + spin_lock_irqsave(&vpif_lock, flags); >> >> + >> >> if (enable) { >> >> regw((regr(VPIF_INTEN) | 0x10), VPIF_INTEN); >> >> regw((regr(VPIF_INTEN_SET) | 0x10), VPIF_INTEN_SET); >> >> @@ -292,11 +299,16 @@ static inline void channel0_intr_enable(int >> >> enable) >> >> regw((regr(VPIF_INTEN_SET) | VPIF_INTEN_FRAME_CH0), >> >> VPIF_INTEN_SET); >> >> } >> >> + spin_unlock_irqrestore(&vpif_lock, flags); >> >> } >> >> >> >> /* inline function to enable interrupt for channel1 */ >> >> static inline void channel1_intr_enable(int enable) >> >> { >> >> + unsigned long flags; >> >> + >> >> + spin_lock_irqsave(&vpif_lock, flags); >> >> + >> >> if (enable) { >> >> regw((regr(VPIF_INTEN) | 0x10), VPIF_INTEN); >> >> regw((regr(VPIF_INTEN_SET) | 0x10), VPIF_INTEN_SET); >> >> @@ -309,6 +321,7 @@ static inline void channel1_intr_enable(int >> >> enable) >> >> regw((regr(VPIF_INTEN_SET) | VPIF_INTEN_FRAME_CH1), >> >> VPIF_INTEN_SET); >> >> } >> >> + spin_unlock_irqrestore(&vpif_lock, flags); >> >> } >> >> >> >> /* inline function to set buffer addresses in case of Y/C non >> mux >> >> mode */ >> >> @@ -431,6 +444,10 @@ static inline void enable_channel3(int >> enable) >> >> /* inline function to enable interrupt for channel2 */ >> >> static inline void channel2_intr_enable(int enable) >> >> { >> >> + unsigned long flags; >> >> + >> >> + spin_lock_irqsave(&vpif_lock, flags); >> >> + >> >> if (enable) { >> >> regw((regr(VPIF_INTEN) | 0x10), VPIF_INTEN); >> >> regw((regr(VPIF_INTEN_SET) | 0x10), VPIF_INTEN_SET); >> >> @@ -442,11 +459,16 @@ static inline void channel2_intr_enable(int >> >> enable) >> >> regw((regr(VPIF_INTEN_SET) | VPIF_INTEN_FRAME_CH2), >> >> VPIF_INTEN_SET); >> >> } >> >> + spin_unlock_irqrestore(&vpif_lock, flags); >> >> } >> >> >> >> /* inline function to enable interrupt for channel3 */ >> >> static inline void channel3_intr_enable(int enable) >> >> { >> >> + unsigned long flags; >> >> + >> >> + spin_lock_irqsave(&vpif_lock, flags); >> >> + >> >> if (enable) { >> >> regw((regr(VPIF_INTEN) | 0x10), VPIF_INTEN); >> >> regw((regr(VPIF_INTEN_SET) | 0x10), VPIF_INTEN_SET); >> >> @@ -459,6 +481,7 @@ static inline void channel3_intr_enable(int >> >> enable) >> >> regw((regr(VPIF_INTEN_SET) | VPIF_INTEN_FRAME_CH3), >> >> VPIF_INTEN_SET); >> >> } >> >> + spin_unlock_irqrestore(&vpif_lock, flags); >> >> } >> >> >> >> /* inline function to enable raw vbi data for channel2 */ >> >> @@ -571,7 +594,7 @@ struct vpif_channel_config_params { >> >> v4l2_std_id stdid; >> >> }; >> >> >> >> -struct vpif_interface; >> >> +struct vpif_video_params; >> >> struct vpif_params; >> >> struct vpif_vbi_params; >> >> >> >> @@ -579,13 +602,6 @@ int vpif_set_video_params(struct vpif_params >> >> *vpifparams, u8 channel_id); >> >> void vpif_set_vbi_display_params(struct vpif_vbi_params >> *vbiparams, >> >> u8 channel_id); >> >> int vpif_channel_getfid(u8 channel_id); >> >> -void vpif_base_addr_init(void __iomem *base); >> >> - >> >> -/* Enumerated data types */ >> >> -enum vpif_capture_pinpol { >> >> - VPIF_CAPTURE_PINPOL_SAME = 0, >> >> - VPIF_CAPTURE_PINPOL_INVERT = 1 >> >> -}; >> >> >> >> enum data_size { >> >> _8BITS = 0, >> >> @@ -593,13 +609,6 @@ enum data_size { >> >> _12BITS, >> >> }; >> >> >> >> -struct vpif_capture_params_raw { >> >> - enum data_size data_sz; >> >> - enum vpif_capture_pinpol fid_pol; >> >> - enum vpif_capture_pinpol vd_pol; >> >> - enum vpif_capture_pinpol hd_pol; >> >> -}; >> >> - >> >> /* Structure for vpif parameters for raw vbi data */ >> >> struct vpif_vbi_params { >> >> __u32 hstart0; /* Horizontal start of raw vbi data for first >> >> field */ >> >> @@ -613,18 +622,19 @@ struct vpif_vbi_params { >> >> }; >> >> >> >> /* structure for vpif parameters */ >> >> -struct vpif_interface { >> >> +struct vpif_video_params { >> >> __u8 storage_mode; /* Indicates field or frame mode */ >> >> unsigned long hpitch; >> >> v4l2_std_id stdid; >> >> }; >> >> >> >> struct vpif_params { >> >> - struct vpif_interface video_params; >> >> + struct vpif_interface iface; >> >> + struct vpif_video_params video_params; >> >> struct vpif_channel_config_params std_info; >> >> union param { >> >> struct vpif_vbi_params vbi_params; >> >> - struct vpif_capture_params_raw raw_params; >> >> + enum data_size data_sz; >> >> } params; >> >> }; >> >> >> >> -- >> >> 1.6.0.4 >> >> >> >> -- >> >> To unsubscribe from this list: send the line "unsubscribe linux- >> >> media" in >> >> the body of a message to majordomo@vger.kernel.org >> >> More majordomo info at http://vger.kernel.org/majordomo- >> info.html -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/media/video/davinci/vpif.c b/drivers/media/video/davinci/vpif.c index aa77126..3b8eac3 100644 --- a/drivers/media/video/davinci/vpif.c +++ b/drivers/media/video/davinci/vpif.c @@ -19,7 +19,11 @@ #include <linux/init.h> #include <linux/module.h> +#include <linux/platform_device.h> +#include <linux/spinlock.h> #include <linux/kernel.h> +#include <linux/io.h> +#include <mach/hardware.h> #include "vpif.h" @@ -31,6 +35,12 @@ MODULE_LICENSE("GPL"); #define VPIF_CH2_MAX_MODES (15) #define VPIF_CH3_MAX_MODES (02) +static resource_size_t res_len; +static struct resource *res; +spinlock_t vpif_lock; + +void __iomem *vpif_base; + static inline void vpif_wr_bit(u32 reg, u32 bit, u32 val) { if (val) @@ -151,17 +161,17 @@ static void config_vpif_params(struct vpif_params *vpifparams, else if (config->capture_format) { /* Set the polarity of various pins */ vpif_wr_bit(reg, VPIF_CH_FID_POLARITY_BIT, - vpifparams->params.raw_params.fid_pol); + vpifparams->iface.fid_pol); vpif_wr_bit(reg, VPIF_CH_V_VALID_POLARITY_BIT, - vpifparams->params.raw_params.vd_pol); + vpifparams->iface.vd_pol); vpif_wr_bit(reg, VPIF_CH_H_VALID_POLARITY_BIT, - vpifparams->params.raw_params.hd_pol); + vpifparams->iface.hd_pol); value = regr(reg); /* Set data width */ value &= ((~(unsigned int)(0x3)) << VPIF_CH_DATA_WIDTH_BIT); - value |= ((vpifparams->params.raw_params.data_sz) << + value |= ((vpifparams->params.data_sz) << VPIF_CH_DATA_WIDTH_BIT); regw(value, reg); } @@ -227,8 +237,60 @@ int vpif_channel_getfid(u8 channel_id) } EXPORT_SYMBOL(vpif_channel_getfid); -void vpif_base_addr_init(void __iomem *base) +static int __init vpif_probe(struct platform_device *pdev) +{ + int status = 0; + + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); + if (!res) + return -ENOENT; + + res_len = res->end - res->start + 1; + + res = request_mem_region(res->start, res_len, res->name); + if (!res) + return -EBUSY; + + vpif_base = ioremap(res->start, res_len); + if (!vpif_base) { + status = -EBUSY; + goto fail; + } + + spin_lock_init(&vpif_lock); + dev_info(&pdev->dev, "vpif probe success\n"); + return 0; + +fail: + release_mem_region(res->start, res_len); + return status; +} + +static int vpif_remove(struct platform_device *pdev) { - vpif_base = base; + iounmap(vpif_base); + release_mem_region(res->start, res_len); + return 0; } -EXPORT_SYMBOL(vpif_base_addr_init); + +static struct platform_driver vpif_driver = { + .driver = { + .name = "vpif", + .owner = THIS_MODULE, + }, + .remove = __devexit_p(vpif_remove), + .probe = vpif_probe, +}; + +static void vpif_exit(void) +{ + platform_driver_unregister(&vpif_driver); +} + +static int __init vpif_init(void) +{ + return platform_driver_register(&vpif_driver); +} +subsys_initcall(vpif_init); +module_exit(vpif_exit); + diff --git a/drivers/media/video/davinci/vpif.h b/drivers/media/video/davinci/vpif.h index fca26dc..188841b 100644 --- a/drivers/media/video/davinci/vpif.h +++ b/drivers/media/video/davinci/vpif.h @@ -19,6 +19,7 @@ #include <linux/io.h> #include <linux/videodev2.h> #include <mach/hardware.h> +#include <mach/dm646x.h> /* Maximum channel allowed */ #define VPIF_NUM_CHANNELS (4) @@ -26,7 +27,9 @@ #define VPIF_DISPLAY_NUM_CHANNELS (2) /* Macros to read/write registers */ -static void __iomem *vpif_base; +extern void __iomem *vpif_base; +extern spinlock_t vpif_lock; + #define regr(reg) readl((reg) + vpif_base) #define regw(value, reg) writel(value, (reg + vpif_base)) @@ -280,6 +283,10 @@ static inline void enable_channel1(int enable) /* inline function to enable interrupt for channel0 */ static inline void channel0_intr_enable(int enable) { + unsigned long flags; + + spin_lock_irqsave(&vpif_lock, flags); + if (enable) { regw((regr(VPIF_INTEN) | 0x10), VPIF_INTEN); regw((regr(VPIF_INTEN_SET) | 0x10), VPIF_INTEN_SET); @@ -292,11 +299,16 @@ static inline void channel0_intr_enable(int enable) regw((regr(VPIF_INTEN_SET) | VPIF_INTEN_FRAME_CH0), VPIF_INTEN_SET); } + spin_unlock_irqrestore(&vpif_lock, flags); } /* inline function to enable interrupt for channel1 */ static inline void channel1_intr_enable(int enable) { + unsigned long flags; + + spin_lock_irqsave(&vpif_lock, flags); + if (enable) { regw((regr(VPIF_INTEN) | 0x10), VPIF_INTEN); regw((regr(VPIF_INTEN_SET) | 0x10), VPIF_INTEN_SET); @@ -309,6 +321,7 @@ static inline void channel1_intr_enable(int enable) regw((regr(VPIF_INTEN_SET) | VPIF_INTEN_FRAME_CH1), VPIF_INTEN_SET); } + spin_unlock_irqrestore(&vpif_lock, flags); } /* inline function to set buffer addresses in case of Y/C non mux mode */ @@ -431,6 +444,10 @@ static inline void enable_channel3(int enable) /* inline function to enable interrupt for channel2 */ static inline void channel2_intr_enable(int enable) { + unsigned long flags; + + spin_lock_irqsave(&vpif_lock, flags); + if (enable) { regw((regr(VPIF_INTEN) | 0x10), VPIF_INTEN); regw((regr(VPIF_INTEN_SET) | 0x10), VPIF_INTEN_SET); @@ -442,11 +459,16 @@ static inline void channel2_intr_enable(int enable) regw((regr(VPIF_INTEN_SET) | VPIF_INTEN_FRAME_CH2), VPIF_INTEN_SET); } + spin_unlock_irqrestore(&vpif_lock, flags); } /* inline function to enable interrupt for channel3 */ static inline void channel3_intr_enable(int enable) { + unsigned long flags; + + spin_lock_irqsave(&vpif_lock, flags); + if (enable) { regw((regr(VPIF_INTEN) | 0x10), VPIF_INTEN); regw((regr(VPIF_INTEN_SET) | 0x10), VPIF_INTEN_SET); @@ -459,6 +481,7 @@ static inline void channel3_intr_enable(int enable) regw((regr(VPIF_INTEN_SET) | VPIF_INTEN_FRAME_CH3), VPIF_INTEN_SET); } + spin_unlock_irqrestore(&vpif_lock, flags); } /* inline function to enable raw vbi data for channel2 */ @@ -571,7 +594,7 @@ struct vpif_channel_config_params { v4l2_std_id stdid; }; -struct vpif_interface; +struct vpif_video_params; struct vpif_params; struct vpif_vbi_params; @@ -579,13 +602,6 @@ int vpif_set_video_params(struct vpif_params *vpifparams, u8 channel_id); void vpif_set_vbi_display_params(struct vpif_vbi_params *vbiparams, u8 channel_id); int vpif_channel_getfid(u8 channel_id); -void vpif_base_addr_init(void __iomem *base); - -/* Enumerated data types */ -enum vpif_capture_pinpol { - VPIF_CAPTURE_PINPOL_SAME = 0, - VPIF_CAPTURE_PINPOL_INVERT = 1 -}; enum data_size { _8BITS = 0, @@ -593,13 +609,6 @@ enum data_size { _12BITS, }; -struct vpif_capture_params_raw { - enum data_size data_sz; - enum vpif_capture_pinpol fid_pol; - enum vpif_capture_pinpol vd_pol; - enum vpif_capture_pinpol hd_pol; -}; - /* Structure for vpif parameters for raw vbi data */ struct vpif_vbi_params { __u32 hstart0; /* Horizontal start of raw vbi data for first field */ @@ -613,18 +622,19 @@ struct vpif_vbi_params { }; /* structure for vpif parameters */ -struct vpif_interface { +struct vpif_video_params { __u8 storage_mode; /* Indicates field or frame mode */ unsigned long hpitch; v4l2_std_id stdid; }; struct vpif_params { - struct vpif_interface video_params; + struct vpif_interface iface; + struct vpif_video_params video_params; struct vpif_channel_config_params std_info; union param { struct vpif_vbi_params vbi_params; - struct vpif_capture_params_raw raw_params; + enum data_size data_sz; } params; };