On Sat January 19 2013 22:27:22 Sylwester Nawrocki wrote:
> This patch adds V4L2 sub-device driver for OV9650/OV9652 image sensors.
>
> The driver exposes following V4L2 controls:
> - auto/manual exposure,
> - auto/manual white balance,
> - auto/manual gain,
> - brightness, saturation, sharpness,
> - horizontal/vertical flip,
> - color bar test pattern,
> - banding filter (power line frequency).
>
> Frame rate can be configured with g/s_frame_interval pad level ops.
> Supported resolution are only: SXGA, VGA, QVGA.
>
> Signed-off-by: Sylwester Nawrocki <sylvester.nawrocki@xxxxxxxxx>
Some small comments:
<snip>
> +
> +static int ov965x_log_status(struct v4l2_subdev *sd)
> +{
> + v4l2_ctrl_handler_log_status(sd->ctrl_handler, sd->name);
> + return 0;
> +}
A short helper function (v4l2_ctrl_subdev_log_status) would simplify this
as that can be used as a core op directly.
> +
<snip>
> +
> +static int ov965x_subscribe_event(struct v4l2_subdev *sd, struct v4l2_fh *fh,
> + struct v4l2_event_subscription *sub)
> +{
> + return v4l2_ctrl_subscribe_event(fh, sub);
> +}
> +
> +static int ov965x_unsubscribe_event(struct v4l2_subdev *sd, struct v4l2_fh *fh,
> + struct v4l2_event_subscription *sub)
> +{
> + return v4l2_event_unsubscribe(fh, sub);
> +}
I suggest that two helper functions are added (v4l2_ctrl_subdev_subscribe_event
and v4l2_event_subdev_unsubscribe) that can be used as a core op directly.
<snip>
> diff --git a/include/media/ov9650.h b/include/media/ov9650.h
> new file mode 100644
> index 0000000..2fab780
> --- /dev/null
> +++ b/include/media/ov9650.h
> @@ -0,0 +1,27 @@
> +/*
> + * OV9650/OV9652 camera sensors driver
> + *
> + * Copyright (C) 2013 Sylwester Nawrocki <sylvester.nawrocki@xxxxxxxxx>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +#ifndef OV9650_H_
> +#define OV9650_H_
> +
> +/**
> + * struct ov9650_platform_data - ov9650 driver platform data
> + * @mclk_frequency: the sensor's master clock frequency
What's the unit? Hz?
> + * @gpio_pwdn: number of a GPIO connected to OV965X PWDN pin
> + * @gpio_reset: number of a GPIO connected to OV965X RESET pin
> + *
> + * If any of @gpio_pwdn or @gpio_reset are unused then should be
s/then should/then they should/
> + * set to negative value. @mclk_frequency must always be specified.
s/set to/set to a/
> + */
> +struct ov9650_platform_data {
> + unsigned long mclk_frequency;
> + int gpio_pwdn;
> + int gpio_reset;
> +};
> +#endif /* OV9650_H_ */
>
Regards,
Hans
--
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
