On Fri, 30 Jul 2010, Michael Grzeschik wrote:
> removed unused rect and fmt structs from mt9m111 struct
Don't understand. Both rect and fmt do seem to be used to me. If they were
unused, you could have _just_ removed them. Instead you add a new struct
mt9m111_format. Why? So, I don't understand this patch. If you find some
unused data / code - it is ok to remove it, this is one patch. If you want
to change default data format:
> set default values for mf.colorspace and mf.code to the common raw
> format V4L2_MBUS_FMT_RGB555_2X8_PADHI_LE.
This is a separate patch too. From this patch description I don't
understand how these two changes are connected and why you need them and
why you put them in one patch.
Thanks
Guennadi
>
> rewrote following functions to make use the new format structure:
>
> * restore_state,
> * g_fmt,
> * s_fmt,
> * g_crop,
> * s_crop,
> * setup_rect
>
> Signed-off-by: Philipp Wiesner <p.wiesner@xxxxxxxxx>
> Signed-off-by: Michael Grzeschik <m.grzeschik@xxxxxxxxxxxxxx>
> ---
> drivers/media/video/mt9m111.c | 99 ++++++++++++++++++++++-------------------
> 1 files changed, 53 insertions(+), 46 deletions(-)
>
> diff --git a/drivers/media/video/mt9m111.c b/drivers/media/video/mt9m111.c
> index db5ac32..cc0f996 100644
> --- a/drivers/media/video/mt9m111.c
> +++ b/drivers/media/video/mt9m111.c
> @@ -173,13 +173,17 @@ enum mt9m111_context {
> LOWPOWER,
> };
>
> +struct mt9m111_format {
> + struct v4l2_rect rect;
> + struct v4l2_mbus_framefmt mf;
> +};
> +
> struct mt9m111 {
> struct v4l2_subdev subdev;
> int model; /* V4L2_IDENT_MT9M111 or V4L2_IDENT_MT9M112 code */
> /* from v4l2-chip-ident.h */
> enum mt9m111_context context;
> - struct v4l2_rect rect;
> - const struct mt9m111_datafmt *fmt;
> + struct mt9m111_format format;
> unsigned int gain;
> unsigned char autoexposure;
> unsigned char datawidth;
> @@ -278,15 +282,15 @@ static int mt9m111_set_context(struct i2c_client *client,
> }
>
> static int mt9m111_setup_rect(struct i2c_client *client,
> - struct v4l2_rect *rect)
> + struct mt9m111_format *format)
> {
> - struct mt9m111 *mt9m111 = to_mt9m111(client);
> + struct v4l2_rect *rect = &format->rect;
> int ret, is_raw_format;
> int width = rect->width;
> int height = rect->height;
>
> - if (mt9m111->fmt->code == V4L2_MBUS_FMT_SBGGR8_1X8 ||
> - mt9m111->fmt->code == V4L2_MBUS_FMT_SBGGR10_2X8_PADHI_LE)
> + if (format->mf.code == V4L2_MBUS_FMT_SBGGR8_1X8 ||
> + format->mf.code == V4L2_MBUS_FMT_SBGGR10_2X8_PADHI_LE)
> is_raw_format = 1;
> else
> is_raw_format = 0;
> @@ -425,10 +429,10 @@ static int mt9m111_set_bus_param(struct soc_camera_device *icd, unsigned long f)
> }
>
> static int mt9m111_make_rect(struct i2c_client *client,
> - struct v4l2_rect *rect)
> + struct mt9m111_format *format)
> {
> - struct mt9m111 *mt9m111 = to_mt9m111(client);
> - enum v4l2_mbus_pixelcode code = mt9m111->fmt->code;
> + struct v4l2_rect *rect = &format->rect;
> + enum v4l2_mbus_pixelcode code = format->mf.code;
>
> /* FIXME: the datasheet doesn't specify minimum sizes */
> soc_camera_limit_side(&rect->left, &rect->width,
> @@ -459,22 +463,30 @@ static int mt9m111_make_rect(struct i2c_client *client,
> "mf: pixelcode=%d\n", __func__, rect->left, rect->top,
> rect->width, rect->height, code);
>
> - return mt9m111_setup_rect(client, rect);
> + return mt9m111_setup_rect(client, format);
> }
>
> static int mt9m111_s_crop(struct v4l2_subdev *sd, struct v4l2_crop *a)
> {
> - struct v4l2_rect rect = a->c;
> struct i2c_client *client = sd->priv;
> struct mt9m111 *mt9m111 = to_mt9m111(client);
> + struct mt9m111_format format;
> + struct v4l2_mbus_framefmt *mf = &format.mf;
> int ret;
>
> - dev_dbg(&client->dev, "%s left=%d, top=%d, width=%d, height=%d\n",
> - __func__, rect.left, rect.top, rect.width, rect.height);
> + format.rect = a->c;
> + format.mf = mt9m111->format.mf;
> +
> + dev_dbg(&client->dev, "%s: rect: left=%d top=%d width=%d height=%d\n",
> + __func__, a->c.left, a->c.top, a->c.width, a->c.height);
> + dev_dbg(&client->dev, "%s: mf: width=%d height=%d pixelcode=%d "
> + "field=%x colorspace=%x\n", __func__, mf->width, mf->height,
> + mf->code, mf->field, mf->colorspace);
>
> - ret = mt9m111_make_rect(client, &rect);
> + ret = mt9m111_make_rect(client, &format);
> if (!ret)
> - mt9m111->rect = rect;
> + mt9m111->format = format;
> +
> return ret;
> }
>
> @@ -483,7 +495,7 @@ static int mt9m111_g_crop(struct v4l2_subdev *sd, struct v4l2_crop *a)
> struct i2c_client *client = sd->priv;
> struct mt9m111 *mt9m111 = to_mt9m111(client);
>
> - a->c = mt9m111->rect;
> + a->c = mt9m111->format.rect;
> a->type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
>
> return 0;
> @@ -514,10 +526,7 @@ static int mt9m111_g_fmt(struct v4l2_subdev *sd,
> struct i2c_client *client = sd->priv;
> struct mt9m111 *mt9m111 = to_mt9m111(client);
>
> - mf->width = mt9m111->rect.width;
> - mf->height = mt9m111->rect.height;
> - mf->code = mt9m111->fmt->code;
> - mf->field = V4L2_FIELD_NONE;
> + *mf = mt9m111->format.mf;
>
> return 0;
> }
> @@ -576,12 +585,8 @@ static int mt9m111_s_fmt(struct v4l2_subdev *sd,
> struct i2c_client *client = sd->priv;
> const struct mt9m111_datafmt *fmt;
> struct mt9m111 *mt9m111 = to_mt9m111(client);
> - struct v4l2_rect rect = {
> - .left = mt9m111->rect.left,
> - .top = mt9m111->rect.top,
> - .width = mf->width,
> - .height = mf->height,
> - };
> + struct v4l2_rect *rect;
> + struct mt9m111_format format;
> int ret;
>
> fmt = mt9m111_find_datafmt(mf->code, mt9m111_colour_fmts,
> @@ -589,18 +594,19 @@ static int mt9m111_s_fmt(struct v4l2_subdev *sd,
> if (!fmt)
> return -EINVAL;
>
> + format.rect = mt9m111->format.rect;
> + format.mf = *mf;
> + rect = &format.rect;
> +
> dev_dbg(&client->dev,
> "%s code=%x left=%d, top=%d, width=%d, height=%d\n", __func__,
> - mf->code, rect.left, rect.top, rect.width, rect.height);
> + mf->code, rect->left, rect->top, rect->width, rect->height);
>
> - ret = mt9m111_make_rect(client, &rect);
> + ret = mt9m111_make_rect(client, &format);
> if (!ret)
> - ret = mt9m111_set_pixfmt(client, mf->code);
> - if (!ret) {
> - mt9m111->rect = rect;
> - mt9m111->fmt = fmt;
> - mf->colorspace = fmt->colorspace;
> - }
> + ret = mt9m111_set_pixfmt(client, format.mf.code);
> + if (!ret)
> + mt9m111->format = format;
>
> return ret;
> }
> @@ -609,17 +615,14 @@ static int mt9m111_try_fmt(struct v4l2_subdev *sd,
> struct v4l2_mbus_framefmt *mf)
> {
> struct i2c_client *client = sd->priv;
> - struct mt9m111 *mt9m111 = to_mt9m111(client);
> const struct mt9m111_datafmt *fmt;
> bool bayer = mf->code == V4L2_MBUS_FMT_SBGGR8_1X8 ||
> mf->code == V4L2_MBUS_FMT_SBGGR10_2X8_PADHI_LE;
>
> fmt = mt9m111_find_datafmt(mf->code, mt9m111_colour_fmts,
> ARRAY_SIZE(mt9m111_colour_fmts));
> - if (!fmt) {
> - fmt = mt9m111->fmt;
> - mf->code = fmt->code;
> - }
> + if (!fmt)
> + return -EINVAL;
>
> /*
> * With Bayer format enforce even side lengths, but let the user play
> @@ -930,8 +933,8 @@ static int mt9m111_restore_state(struct i2c_client *client)
> struct mt9m111 *mt9m111 = to_mt9m111(client);
>
> mt9m111_set_context(client, mt9m111->context);
> - mt9m111_set_pixfmt(client, mt9m111->fmt->code);
> - mt9m111_setup_rect(client, &mt9m111->rect);
> + mt9m111_set_pixfmt(client, mt9m111->format.mf.code);
> + mt9m111_setup_rect(client, &mt9m111->format);
> mt9m111_set_flip(client, mt9m111->hflip, MT9M111_RMB_MIRROR_COLS);
> mt9m111_set_flip(client, mt9m111->vflip, MT9M111_RMB_MIRROR_ROWS);
> mt9m111_set_global_gain(client, mt9m111->gain);
> @@ -1096,11 +1099,15 @@ static int mt9m111_probe(struct i2c_client *client,
> /* Second stage probe - when a capture adapter is there */
> icd->ops = &mt9m111_ops;
>
> - mt9m111->rect.left = MT9M111_MIN_DARK_COLS;
> - mt9m111->rect.top = MT9M111_MIN_DARK_ROWS;
> - mt9m111->rect.width = MT9M111_MAX_WIDTH;
> - mt9m111->rect.height = MT9M111_MAX_HEIGHT;
> - mt9m111->fmt = &mt9m111_colour_fmts[0];
> + mt9m111->format.rect.left = MT9M111_DEF_DARK_COLS;
> + mt9m111->format.rect.top = MT9M111_DEF_DARK_ROWS;
> + mt9m111->format.rect.width = MT9M111_DEF_WIDTH;
> + mt9m111->format.rect.height = MT9M111_DEF_HEIGHT;
> + mt9m111->format.mf.width = MT9M111_DEF_WIDTH;
> + mt9m111->format.mf.height = MT9M111_DEF_HEIGHT;
> + mt9m111->format.mf.code = mt9m111_colour_fmts[2].code;
> + mt9m111->format.mf.field = V4L2_FIELD_NONE;
> + mt9m111->format.mf.colorspace = mt9m111_colour_fmts[2].colorspace;
>
> ret = mt9m111_video_probe(icd, client);
> if (ret) {
> --
> 1.7.1
>
>
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/
--
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
