[-,v1,4/6] V4L - vpfe_capture bug fix and enhancements
Commit Message
From: Muralidharan Karicheri <m-karicheri2@ti.com>
Added a experimental IOCTL, to read the CCDC parameters.
Default handler was not getting the original pointer from the core.
So a wrapper function added to handle the default handler properly.
Also fixed a bug in the probe() in the linux-next tree
Signed-off-by: Muralidharan Karicheri <m-karicheri2@ti.com>
---
drivers/media/video/davinci/vpfe_capture.c | 119 +++++++++++++++++-----------
include/media/davinci/vpfe_capture.h | 12 ++-
2 files changed, 81 insertions(+), 50 deletions(-)
Comments
On Thursday 10 December 2009 18:00:29 m-karicheri2@ti.com wrote:
> From: Muralidharan Karicheri <m-karicheri2@ti.com>
>
> Added a experimental IOCTL, to read the CCDC parameters.
> Default handler was not getting the original pointer from the core.
> So a wrapper function added to handle the default handler properly.
> Also fixed a bug in the probe() in the linux-next tree
>
> Signed-off-by: Muralidharan Karicheri <m-karicheri2@ti.com>
> ---
> drivers/media/video/davinci/vpfe_capture.c | 119 +++++++++++++++++-----------
> include/media/davinci/vpfe_capture.h | 12 ++-
> 2 files changed, 81 insertions(+), 50 deletions(-)
>
> diff --git a/drivers/media/video/davinci/vpfe_capture.c b/drivers/media/video/davinci/vpfe_capture.c
> index 091750e..8c6d856 100644
> --- a/drivers/media/video/davinci/vpfe_capture.c
> +++ b/drivers/media/video/davinci/vpfe_capture.c
> @@ -759,12 +759,83 @@ static unsigned int vpfe_poll(struct file *file, poll_table *wait)
> return 0;
> }
>
> +static long vpfe_param_handler(struct file *file, void *priv,
> + int cmd, void *param)
> +{
> + struct vpfe_device *vpfe_dev = video_drvdata(file);
> + int ret = 0;
> +
> + v4l2_dbg(1, debug, &vpfe_dev->v4l2_dev, "vpfe_param_handler\n");
> +
> + if (NULL == param) {
> + v4l2_dbg(1, debug, &vpfe_dev->v4l2_dev,
> + "Invalid user ptr\n");
Shouldn't there be an error return here? It looks weird.
> + }
> +
> + if (vpfe_dev->started) {
> + /* only allowed if streaming is not started */
> + v4l2_err(&vpfe_dev->v4l2_dev, "device already started\n");
> + return -EBUSY;
> + }
> +
> +
> + switch (cmd) {
> + case VPFE_CMD_S_CCDC_RAW_PARAMS:
> + v4l2_warn(&vpfe_dev->v4l2_dev,
> + "VPFE_CMD_S_CCDC_RAW_PARAMS: experimental ioctl\n");
> + ret = mutex_lock_interruptible(&vpfe_dev->lock);
> + if (ret)
> + return ret;
> + ret = ccdc_dev->hw_ops.set_params(param);
> + if (ret) {
> + v4l2_dbg(1, debug, &vpfe_dev->v4l2_dev,
> + "Error in setting parameters in CCDC\n");
> + goto unlock_out;
> + }
> +
> + if (vpfe_get_ccdc_image_format(vpfe_dev, &vpfe_dev->fmt) < 0) {
> + v4l2_err(&vpfe_dev->v4l2_dev,
> + "Invalid image format at CCDC\n");
> + ret = -EINVAL;
> + }
> +unlock_out:
> + mutex_unlock(&vpfe_dev->lock);
> + break;
> + case VPFE_CMD_G_CCDC_RAW_PARAMS:
> + v4l2_warn(&vpfe_dev->v4l2_dev,
> + "VPFE_CMD_G_CCDC_RAW_PARAMS: experimental ioctl\n");
> + if (!ccdc_dev->hw_ops.get_params) {
> + ret = -EINVAL;
> + break;
> + }
> + ret = ccdc_dev->hw_ops.get_params(param);
> + if (ret) {
> + v4l2_dbg(1, debug, &vpfe_dev->v4l2_dev,
> + "Error in getting parameters from CCDC\n");
> + }
> + break;
> +
> + default:
> + ret = -EINVAL;
Please add a break statement here.
> + }
> + return ret;
> +}
> +
> +static long vpfe_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
> +{
> + if (cmd == VPFE_CMD_S_CCDC_RAW_PARAMS ||
> + cmd == VPFE_CMD_G_CCDC_RAW_PARAMS)
> + return vpfe_param_handler(file, file->private_data, cmd,
> + (void *)arg);
> + return video_ioctl2(file, cmd, arg);
> +}
> +
> /* vpfe capture driver file operations */
> static const struct v4l2_file_operations vpfe_fops = {
> .owner = THIS_MODULE,
> .open = vpfe_open,
> .release = vpfe_release,
> - .unlocked_ioctl = video_ioctl2,
> + .unlocked_ioctl = vpfe_ioctl,
> .mmap = vpfe_mmap,
> .poll = vpfe_poll
> };
> @@ -1682,50 +1753,6 @@ unlock_out:
> return ret;
> }
>
> -
> -static long vpfe_param_handler(struct file *file, void *priv,
> - int cmd, void *param)
> -{
> - struct vpfe_device *vpfe_dev = video_drvdata(file);
> - int ret = 0;
> -
> - v4l2_dbg(1, debug, &vpfe_dev->v4l2_dev, "vpfe_param_handler\n");
> -
> - if (vpfe_dev->started) {
> - /* only allowed if streaming is not started */
> - v4l2_err(&vpfe_dev->v4l2_dev, "device already started\n");
> - return -EBUSY;
> - }
> -
> - ret = mutex_lock_interruptible(&vpfe_dev->lock);
> - if (ret)
> - return ret;
> -
> - switch (cmd) {
> - case VPFE_CMD_S_CCDC_RAW_PARAMS:
> - v4l2_warn(&vpfe_dev->v4l2_dev,
> - "VPFE_CMD_S_CCDC_RAW_PARAMS: experimental ioctl\n");
> - ret = ccdc_dev->hw_ops.set_params(param);
> - if (ret) {
> - v4l2_err(&vpfe_dev->v4l2_dev,
> - "Error in setting parameters in CCDC\n");
> - goto unlock_out;
> - }
> - if (vpfe_get_ccdc_image_format(vpfe_dev, &vpfe_dev->fmt) < 0) {
> - v4l2_err(&vpfe_dev->v4l2_dev,
> - "Invalid image format at CCDC\n");
> - goto unlock_out;
> - }
> - break;
> - default:
> - ret = -EINVAL;
> - }
> -unlock_out:
> - mutex_unlock(&vpfe_dev->lock);
> - return ret;
> -}
> -
> -
> /* vpfe capture ioctl operations */
> static const struct v4l2_ioctl_ops vpfe_ioctl_ops = {
> .vidioc_querycap = vpfe_querycap,
> @@ -1751,7 +1778,6 @@ static const struct v4l2_ioctl_ops vpfe_ioctl_ops = {
> .vidioc_cropcap = vpfe_cropcap,
> .vidioc_g_crop = vpfe_g_crop,
> .vidioc_s_crop = vpfe_s_crop,
> - .vidioc_default = vpfe_param_handler,
> };
>
> static struct vpfe_device *vpfe_initialize(void)
> @@ -1923,6 +1949,7 @@ static __init int vpfe_probe(struct platform_device *pdev)
> platform_set_drvdata(pdev, vpfe_dev);
> /* set driver private data */
> video_set_drvdata(vpfe_dev->video_dev, vpfe_dev);
> + vpfe_cfg = pdev->dev.platform_data;
> i2c_adap = i2c_get_adapter(vpfe_cfg->i2c_adapter_id);
> num_subdevs = vpfe_cfg->num_subdevs;
> vpfe_dev->sd = kmalloc(sizeof(struct v4l2_subdev *) * num_subdevs,
> diff --git a/include/media/davinci/vpfe_capture.h b/include/media/davinci/vpfe_capture.h
> index d863e5e..23043bd 100644
> --- a/include/media/davinci/vpfe_capture.h
> +++ b/include/media/davinci/vpfe_capture.h
> @@ -31,8 +31,6 @@
> #include <media/videobuf-dma-contig.h>
> #include <media/davinci/vpfe_types.h>
>
> -#define VPFE_CAPTURE_NUM_DECODERS 5
> -
> /* Macros */
> #define VPFE_MAJOR_RELEASE 0
> #define VPFE_MINOR_RELEASE 0
> @@ -91,8 +89,9 @@ struct vpfe_config {
> char *card_name;
> /* ccdc name */
> char *ccdc;
> - /* vpfe clock */
> + /* vpfe clock. Obsolete! Will be removed in next patch */
> struct clk *vpssclk;
> + /* Obsolete! Will be removed in next patch */
> struct clk *slaveclk;
> };
>
> @@ -193,8 +192,13 @@ struct vpfe_config_params {
> * color space conversion, culling etc. This is an experimental ioctl that
> * will change in future kernels. So use this ioctl with care !
> * TODO: This is to be split into multiple ioctls and also explore the
> - * possibility of extending the v4l2 api to include this
> + * possibility of extending the v4l2 api to include this.
> + * VPFE_CMD_G_CCDC_RAW_PARAMS - EXPERIMENTAL IOCTL to get the current raw
> + * capture parameters
> **/
> #define VPFE_CMD_S_CCDC_RAW_PARAMS _IOW('V', BASE_VIDIOC_PRIVATE + 1, \
> void *)
> +#define VPFE_CMD_G_CCDC_RAW_PARAMS _IOR('V', BASE_VIDIOC_PRIVATE + 2, \
> + void *)
> +
> #endif /* _DAVINCI_VPFE_H */
>
Note that the other patches from this series are fine as far as I am concerned.
One general note: I always have difficulties with constructions like this:
> + val = (bc->horz.win_count_calc &
> + ISIF_HORZ_BC_WIN_COUNT_MASK) |
> + ((!!bc->horz.base_win_sel_calc) <<
> + ISIF_HORZ_BC_WIN_SEL_SHIFT) |
> + ((!!bc->horz.clamp_pix_limit) <<
> + ISIF_HORZ_BC_PIX_LIMIT_SHIFT) |
> + ((bc->horz.win_h_sz_calc &
> + ISIF_HORZ_BC_WIN_H_SIZE_MASK) <<
> + ISIF_HORZ_BC_WIN_H_SIZE_SHIFT) |
> + ((bc->horz.win_v_sz_calc &
> + ISIF_HORZ_BC_WIN_V_SIZE_MASK) <<
> + ISIF_HORZ_BC_WIN_V_SIZE_SHIFT);
It's just about impossible for me to parse. And some of the patches in this
series are full of such constructs.
Unfortunately, I do not have a magic bullet solution. In some cases I suspect
that a static inline function can help.
In other cases it might help to split it up in smaller parts. For example:
u32 tmp;
val = bc->horz.win_count_calc &
ISIF_HORZ_BC_WIN_COUNT_MASK;
val |= !!bc->horz.base_win_sel_calc <<
ISIF_HORZ_BC_WIN_SEL_SHIFT;
val |= !!bc->horz.clamp_pix_limit <<
ISIF_HORZ_BC_PIX_LIMIT_SHIFT;
tmp = bc->horz.win_h_sz_calc &
ISIF_HORZ_BC_WIN_H_SIZE_MASK;
val |= tmp << ISIF_HORZ_BC_WIN_H_SIZE_SHIFT;
tmp = bc->horz.win_v_sz_calc &
ISIF_HORZ_BC_WIN_V_SIZE_MASK;
val |= tmp << ISIF_HORZ_BC_WIN_V_SIZE_SHIFT;
Of course, in this particular piece of code from the function isif_config_bclamp()
I am also wondering why bc->horz.win_h_sz_calc and bc->horz.win_v_sz_calc need to
be ANDed anyway. I would expect that to happen when these values are set. But I
did not look at this in-depth, so I may well have missed some subtlety here.
It would be interesting to know if people know of good ways of making awkward
code like this more elegant (or at least less awkward).
Regards,
Hans
Hans,
I remember there was a comment against an earlier patch that asks
for combining such statements since it makes the function appear
as big. Not sure who had made that comment. That is the reason you
find code like this in this patch. It was initially done with multiple
OR statements to construct the value to be written to the register as you stated below as
>val = bc->horz.win_count_calc &
> ISIF_HORZ_BC_WIN_COUNT_MASK;
>val |= !!bc->horz.base_win_sel_calc <<
> ISIF_HORZ_BC_WIN_SEL_SHIFT;
I have checked few other drivers such as bt819.c ir-kbd-i2c.c,
mt9m111.c etc, where similar statements are found, but they have used hardcoded values masks which makes it appears less complex. But I
like to reduce magic numbers as much possible in the code.
I think what I can do is to combine maximum of 2 such expressions in a statement which might make it less complex to read. Such as
val = (bc->horz.win_count_calc &
ISIF_HORZ_BC_WIN_COUNT_MASK) |
((!!bc->horz.base_win_sel_calc) <<
ISIF_HORZ_BC_WIN_SEL_SHIFT);
val |= (!!bc->horz.clamp_pix_limit) <<
ISIF_HORZ_BC_PIX_LIMIT_SHIFT) |
((bc->horz.win_h_sz_calc &
ISIF_HORZ_BC_WIN_H_SIZE_MASK) <<
ISIF_HORZ_BC_WIN_H_SIZE_SHIFT);
val |= ((bc->horz.win_v_sz_calc &
ISIF_HORZ_BC_WIN_V_SIZE_MASK) <<
ISIF_HORZ_BC_WIN_V_SIZE_SHIFT);
Also to make the line fits in 80 characters, I will consider reducing
the number of characters in #define names such as
val = (bc->horz.win_count_calc & HZ_BC_WIN_CNT_MASK) |
((!!bc->horz.base_win_sel_calc) << HZ_BC_WIN_SEL_SHIFT) |
(!!bc->horz.clamp_pix_limit) << HZ_BC_PIX_LIMIT_SHIFT);
Let me know if you don't agree.
>
>Of course, in this particular piece of code from the function
>isif_config_bclamp()
>I am also wondering why bc->horz.win_h_sz_calc and bc->horz.win_v_sz_calc
>need to
>be ANDed anyway. I would expect that to happen when these values are set.
>But I
>did not look at this in-depth, so I may well have missed some subtlety here.
Yes, isif_config_bclamp() set values in the register.
>
>It would be interesting to know if people know of good ways of making
>awkward
>code like this more elegant (or at least less awkward).
>
>Regards,
>
> Hans
>
>--
>Hans Verkuil - video4linux developer - sponsored by TANDBERG
--
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
On Wednesday 16 December 2009 00:37:52 Karicheri, Muralidharan wrote:
> Hans,
>
> I remember there was a comment against an earlier patch that asks
> for combining such statements since it makes the function appear
> as big. Not sure who had made that comment. That is the reason you
> find code like this in this patch. It was initially done with multiple
> OR statements to construct the value to be written to the register as you stated below as
>
> >val = bc->horz.win_count_calc &
> > ISIF_HORZ_BC_WIN_COUNT_MASK;
> >val |= !!bc->horz.base_win_sel_calc <<
> > ISIF_HORZ_BC_WIN_SEL_SHIFT;
>
> I have checked few other drivers such as bt819.c ir-kbd-i2c.c,
> mt9m111.c etc, where similar statements are found, but they have used hardcoded values masks which makes it appears less complex. But I
> like to reduce magic numbers as much possible in the code.
Personally I have mixed feelings about the use for symbolic names for things
like this. The problem is that they do not help me understanding the code.
The names tend to be long, leading to these broken up lines, and if I want
to know how the bits are distributed in the value I continuously have to
do the look up in the header containing these defines.
Frankly, I have a similar problem with using symbolic names for registers.
Every time I need to look up a register in the datasheet I first need to
look up the register number the register name maps to. All datasheets seem
to be organized around the register addresses and there rarely is a datasheet
that has an index of symbolic names.
Using hard-coded numbers together with a well written comment tends to be much
more readable in my opinion. I don't really think there is anything magic about
these numbers: these are exactly the numbers that I need to know whenever I
have to deal with the datasheet. Having to go through a layer of obfuscation
is just plain irritating to me.
However, I seem to be a minority when it comes to this and I've given up
arguing for this...
Note that all this assumes that the datasheet is publicly available. If it
is a reversed engineered driver or based on NDA datasheets only, then the
symbolic names may be all there is to make you understand what is going on.
>
> I think what I can do is to combine maximum of 2 such expressions in a statement which might make it less complex to read. Such as
>
> val = (bc->horz.win_count_calc &
> ISIF_HORZ_BC_WIN_COUNT_MASK) |
> ((!!bc->horz.base_win_sel_calc) <<
> ISIF_HORZ_BC_WIN_SEL_SHIFT);
>
> val |= (!!bc->horz.clamp_pix_limit) <<
> ISIF_HORZ_BC_PIX_LIMIT_SHIFT) |
> ((bc->horz.win_h_sz_calc &
> ISIF_HORZ_BC_WIN_H_SIZE_MASK) <<
> ISIF_HORZ_BC_WIN_H_SIZE_SHIFT);
> val |= ((bc->horz.win_v_sz_calc &
> ISIF_HORZ_BC_WIN_V_SIZE_MASK) <<
> ISIF_HORZ_BC_WIN_V_SIZE_SHIFT);
>
> Also to make the line fits in 80 characters, I will consider reducing
> the number of characters in #define names such as
>
> val = (bc->horz.win_count_calc & HZ_BC_WIN_CNT_MASK) |
> ((!!bc->horz.base_win_sel_calc) << HZ_BC_WIN_SEL_SHIFT) |
> (!!bc->horz.clamp_pix_limit) << HZ_BC_PIX_LIMIT_SHIFT);
>
> Let me know if you don't agree.
That seems overkill. I actually think it can be improved a lot by visually
grouping the lines:
val = (bc->horz.win_count_calc &
ISIF_HORZ_BC_WIN_COUNT_MASK) |
((!!bc->horz.base_win_sel_calc) <<
ISIF_HORZ_BC_WIN_SEL_SHIFT) |
((!!bc->horz.clamp_pix_limit) <<
ISIF_HORZ_BC_PIX_LIMIT_SHIFT) |
((bc->horz.win_h_sz_calc &
ISIF_HORZ_BC_WIN_H_SIZE_MASK) <<
ISIF_HORZ_BC_WIN_H_SIZE_SHIFT) |
((bc->horz.win_v_sz_calc &
ISIF_HORZ_BC_WIN_V_SIZE_MASK) <<
ISIF_HORZ_BC_WIN_V_SIZE_SHIFT);
Now I can at least see the various values that are ORed.
>
> >
> >Of course, in this particular piece of code from the function
> >isif_config_bclamp()
> >I am also wondering why bc->horz.win_h_sz_calc and bc->horz.win_v_sz_calc
> >need to
> >be ANDed anyway. I would expect that to happen when these values are set.
> >But I
> >did not look at this in-depth, so I may well have missed some subtlety here.
>
> Yes, isif_config_bclamp() set values in the register.
Huh? That does not explain why apparently bc->horz.win_h_sz_calc can be larger
than ISIF_HORZ_BC_WIN_H_SIZE_MASK.
Regards,
Hans
>
> >
> >It would be interesting to know if people know of good ways of making
> >awkward
> >code like this more elegant (or at least less awkward).
> >
> >Regards,
> >
> > Hans
> >
> >--
> >Hans Verkuil - video4linux developer - sponsored by TANDBERG
>
>
hans,
>>
>> Yes, isif_config_bclamp() set values in the register.
>
>Huh? That does not explain why apparently bc->horz.win_h_sz_calc can be
>larger
>than ISIF_HORZ_BC_WIN_H_SIZE_MASK.
because the values come from the user and since we can't use the enum
for the types, I have to make sure the value is within range. Other way
to do is to check the value in the validate() function. I am inclined to
do the validation so that the & statements with masks can be removed while setting it in the register.
>
>Regards,
>
> Hans
>
>>
>> >
>> >It would be interesting to know if people know of good ways of making
>> >awkward
>> >code like this more elegant (or at least less awkward).
>> >
>> >Regards,
>> >
>> > Hans
>> >
>> >--
>> >Hans Verkuil - video4linux developer - sponsored by TANDBERG
>>
>>
>
>--
>Hans Verkuil - video4linux developer - sponsored by TANDBERG
--
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
On Wed, Dec 16, 2009 at 13:11:57, Hans Verkuil wrote:
> On Wednesday 16 December 2009 00:37:52 Karicheri, Muralidharan wrote:
> > Hans,
> >
> > I remember there was a comment against an earlier patch that asks
> > for combining such statements since it makes the function appear
> > as big. Not sure who had made that comment. That is the reason you
> > find code like this in this patch. It was initially done with multiple
> > OR statements to construct the value to be written to the register as you stated below as
> >
> > >val = bc->horz.win_count_calc &
> > > ISIF_HORZ_BC_WIN_COUNT_MASK;
> > >val |= !!bc->horz.base_win_sel_calc <<
> > > ISIF_HORZ_BC_WIN_SEL_SHIFT;
> >
> > I have checked few other drivers such as bt819.c ir-kbd-i2c.c,
> > mt9m111.c etc, where similar statements are found, but they have used hardcoded values masks which makes it appears less complex. But I
> > like to reduce magic numbers as much possible in the code.
>
> Personally I have mixed feelings about the use for symbolic names for things
> like this. The problem is that they do not help me understanding the code.
> The names tend to be long, leading to these broken up lines, and if I want
> to know how the bits are distributed in the value I continuously have to
> do the look up in the header containing these defines.
>
> Frankly, I have a similar problem with using symbolic names for registers.
> Every time I need to look up a register in the datasheet I first need to
> look up the register number the register name maps to. All datasheets seem
> to be organized around the register addresses and there rarely is a datasheet
> that has an index of symbolic names.
>
> Using hard-coded numbers together with a well written comment tends to be much
> more readable in my opinion. I don't really think there is anything magic about
> these numbers: these are exactly the numbers that I need to know whenever I
> have to deal with the datasheet. Having to go through a layer of obfuscation
> is just plain irritating to me.
>
> However, I seem to be a minority when it comes to this and I've given up
> arguing for this...
>
> Note that all this assumes that the datasheet is publicly available. If it
> is a reversed engineered driver or based on NDA datasheets only, then the
> symbolic names may be all there is to make you understand what is going on.
>
[...]
>
> That seems overkill. I actually think it can be improved a lot by visually
> grouping the lines:
>
> val = (bc->horz.win_count_calc &
> ISIF_HORZ_BC_WIN_COUNT_MASK) |
> ((!!bc->horz.base_win_sel_calc) <<
> ISIF_HORZ_BC_WIN_SEL_SHIFT) |
> ((!!bc->horz.clamp_pix_limit) <<
> ISIF_HORZ_BC_PIX_LIMIT_SHIFT) |
> ((bc->horz.win_h_sz_calc &
> ISIF_HORZ_BC_WIN_H_SIZE_MASK) <<
> ISIF_HORZ_BC_WIN_H_SIZE_SHIFT) |
> ((bc->horz.win_v_sz_calc &
> ISIF_HORZ_BC_WIN_V_SIZE_MASK) <<
> ISIF_HORZ_BC_WIN_V_SIZE_SHIFT);
>
> Now I can at least see the various values that are ORed.
>
I liked this piece of code from drivers/mtd/nand/s3c2410.c. Serves as
a good template to do this sort of thing.
#define S3C2410_NFCONF_TACLS(x) ((x)<<8)
#define S3C2410_NFCONF_TWRPH0(x) ((x)<<4)
#define S3C2410_NFCONF_TWRPH1(x) ((x)<<0)
[Okay, spaces around '<<', please :)]
[...]
if (plat != NULL) {
tacls = s3c_nand_calc_rate(plat->tacls, clkrate, tacls_max);
twrph0 = s3c_nand_calc_rate(plat->twrph0, clkrate, 8);
twrph1 = s3c_nand_calc_rate(plat->twrph1, clkrate, 8);
}
[...]
mask = (S3C2410_NFCONF_TACLS(3) |
S3C2410_NFCONF_TWRPH0(7) |
S3C2410_NFCONF_TWRPH1(7));
set = S3C2410_NFCONF_EN;
set |= S3C2410_NFCONF_TACLS(tacls - 1);
set |= S3C2410_NFCONF_TWRPH0(twrph0 - 1);
set |= S3C2410_NFCONF_TWRPH1(twrph1 - 1);
[...]
cfg = readl(info->regs + S3C2410_NFCONF);
cfg &= ~mask;
cfg |= set;
writel(cfg, info->regs + S3C2410_NFCONF);
And Murali said:
> >Huh? That does not explain why apparently bc->horz.win_h_sz_calc can be
> >larger
> >than ISIF_HORZ_BC_WIN_H_SIZE_MASK.
> because the values come from the user and since we can't use the enum
> for the types, I have to make sure the value is within range. Other way
> to do is to check the value in the validate() function. I am inclined to
> do the validation so that the & statements with masks can be removed while setting it in
> the register.
Agree fully here. Either a separate validate function or
an if check before using the values. Results with masking
or without masking are both unpredictable.
Thanks,
Sekhar
--
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
@@ -759,12 +759,83 @@ static unsigned int vpfe_poll(struct file *file, poll_table *wait)
return 0;
}
+static long vpfe_param_handler(struct file *file, void *priv,
+ int cmd, void *param)
+{
+ struct vpfe_device *vpfe_dev = video_drvdata(file);
+ int ret = 0;
+
+ v4l2_dbg(1, debug, &vpfe_dev->v4l2_dev, "vpfe_param_handler\n");
+
+ if (NULL == param) {
+ v4l2_dbg(1, debug, &vpfe_dev->v4l2_dev,
+ "Invalid user ptr\n");
+ }
+
+ if (vpfe_dev->started) {
+ /* only allowed if streaming is not started */
+ v4l2_err(&vpfe_dev->v4l2_dev, "device already started\n");
+ return -EBUSY;
+ }
+
+
+ switch (cmd) {
+ case VPFE_CMD_S_CCDC_RAW_PARAMS:
+ v4l2_warn(&vpfe_dev->v4l2_dev,
+ "VPFE_CMD_S_CCDC_RAW_PARAMS: experimental ioctl\n");
+ ret = mutex_lock_interruptible(&vpfe_dev->lock);
+ if (ret)
+ return ret;
+ ret = ccdc_dev->hw_ops.set_params(param);
+ if (ret) {
+ v4l2_dbg(1, debug, &vpfe_dev->v4l2_dev,
+ "Error in setting parameters in CCDC\n");
+ goto unlock_out;
+ }
+
+ if (vpfe_get_ccdc_image_format(vpfe_dev, &vpfe_dev->fmt) < 0) {
+ v4l2_err(&vpfe_dev->v4l2_dev,
+ "Invalid image format at CCDC\n");
+ ret = -EINVAL;
+ }
+unlock_out:
+ mutex_unlock(&vpfe_dev->lock);
+ break;
+ case VPFE_CMD_G_CCDC_RAW_PARAMS:
+ v4l2_warn(&vpfe_dev->v4l2_dev,
+ "VPFE_CMD_G_CCDC_RAW_PARAMS: experimental ioctl\n");
+ if (!ccdc_dev->hw_ops.get_params) {
+ ret = -EINVAL;
+ break;
+ }
+ ret = ccdc_dev->hw_ops.get_params(param);
+ if (ret) {
+ v4l2_dbg(1, debug, &vpfe_dev->v4l2_dev,
+ "Error in getting parameters from CCDC\n");
+ }
+ break;
+
+ default:
+ ret = -EINVAL;
+ }
+ return ret;
+}
+
+static long vpfe_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
+{
+ if (cmd == VPFE_CMD_S_CCDC_RAW_PARAMS ||
+ cmd == VPFE_CMD_G_CCDC_RAW_PARAMS)
+ return vpfe_param_handler(file, file->private_data, cmd,
+ (void *)arg);
+ return video_ioctl2(file, cmd, arg);
+}
+
/* vpfe capture driver file operations */
static const struct v4l2_file_operations vpfe_fops = {
.owner = THIS_MODULE,
.open = vpfe_open,
.release = vpfe_release,
- .unlocked_ioctl = video_ioctl2,
+ .unlocked_ioctl = vpfe_ioctl,
.mmap = vpfe_mmap,
.poll = vpfe_poll
};
@@ -1682,50 +1753,6 @@ unlock_out:
return ret;
}
-
-static long vpfe_param_handler(struct file *file, void *priv,
- int cmd, void *param)
-{
- struct vpfe_device *vpfe_dev = video_drvdata(file);
- int ret = 0;
-
- v4l2_dbg(1, debug, &vpfe_dev->v4l2_dev, "vpfe_param_handler\n");
-
- if (vpfe_dev->started) {
- /* only allowed if streaming is not started */
- v4l2_err(&vpfe_dev->v4l2_dev, "device already started\n");
- return -EBUSY;
- }
-
- ret = mutex_lock_interruptible(&vpfe_dev->lock);
- if (ret)
- return ret;
-
- switch (cmd) {
- case VPFE_CMD_S_CCDC_RAW_PARAMS:
- v4l2_warn(&vpfe_dev->v4l2_dev,
- "VPFE_CMD_S_CCDC_RAW_PARAMS: experimental ioctl\n");
- ret = ccdc_dev->hw_ops.set_params(param);
- if (ret) {
- v4l2_err(&vpfe_dev->v4l2_dev,
- "Error in setting parameters in CCDC\n");
- goto unlock_out;
- }
- if (vpfe_get_ccdc_image_format(vpfe_dev, &vpfe_dev->fmt) < 0) {
- v4l2_err(&vpfe_dev->v4l2_dev,
- "Invalid image format at CCDC\n");
- goto unlock_out;
- }
- break;
- default:
- ret = -EINVAL;
- }
-unlock_out:
- mutex_unlock(&vpfe_dev->lock);
- return ret;
-}
-
-
/* vpfe capture ioctl operations */
static const struct v4l2_ioctl_ops vpfe_ioctl_ops = {
.vidioc_querycap = vpfe_querycap,
@@ -1751,7 +1778,6 @@ static const struct v4l2_ioctl_ops vpfe_ioctl_ops = {
.vidioc_cropcap = vpfe_cropcap,
.vidioc_g_crop = vpfe_g_crop,
.vidioc_s_crop = vpfe_s_crop,
- .vidioc_default = vpfe_param_handler,
};
static struct vpfe_device *vpfe_initialize(void)
@@ -1923,6 +1949,7 @@ static __init int vpfe_probe(struct platform_device *pdev)
platform_set_drvdata(pdev, vpfe_dev);
/* set driver private data */
video_set_drvdata(vpfe_dev->video_dev, vpfe_dev);
+ vpfe_cfg = pdev->dev.platform_data;
i2c_adap = i2c_get_adapter(vpfe_cfg->i2c_adapter_id);
num_subdevs = vpfe_cfg->num_subdevs;
vpfe_dev->sd = kmalloc(sizeof(struct v4l2_subdev *) * num_subdevs,
@@ -31,8 +31,6 @@
#include <media/videobuf-dma-contig.h>
#include <media/davinci/vpfe_types.h>
-#define VPFE_CAPTURE_NUM_DECODERS 5
-
/* Macros */
#define VPFE_MAJOR_RELEASE 0
#define VPFE_MINOR_RELEASE 0
@@ -91,8 +89,9 @@ struct vpfe_config {
char *card_name;
/* ccdc name */
char *ccdc;
- /* vpfe clock */
+ /* vpfe clock. Obsolete! Will be removed in next patch */
struct clk *vpssclk;
+ /* Obsolete! Will be removed in next patch */
struct clk *slaveclk;
};
@@ -193,8 +192,13 @@ struct vpfe_config_params {
* color space conversion, culling etc. This is an experimental ioctl that
* will change in future kernels. So use this ioctl with care !
* TODO: This is to be split into multiple ioctls and also explore the
- * possibility of extending the v4l2 api to include this
+ * possibility of extending the v4l2 api to include this.
+ * VPFE_CMD_G_CCDC_RAW_PARAMS - EXPERIMENTAL IOCTL to get the current raw
+ * capture parameters
**/
#define VPFE_CMD_S_CCDC_RAW_PARAMS _IOW('V', BASE_VIDIOC_PRIVATE + 1, \
void *)
+#define VPFE_CMD_G_CCDC_RAW_PARAMS _IOR('V', BASE_VIDIOC_PRIVATE + 2, \
+ void *)
+
#endif /* _DAVINCI_VPFE_H */