| Message ID | 1308757470-24421-3-git-send-email-s.nawrocki@samsung.com (mailing list archive) |
|---|---|
| State | RFC |
| Headers | show |
Hi Sylwester, Thanks for the patch. It's nice to see sensor drivers picking up the pad-level API :-) On Wednesday 22 June 2011 17:44:29 Sylwester Nawrocki wrote: > Replace g/s_mbus_fmt ops with the pad level get/set_fmt operations. > Add media entity initalization and set subdev flags so the host driver > creates a v4l-subdev device node for the driver. A mutex is added for > serializing operations on subdevice node. When setting format > is attempted during streaming EBUSY error code will be returned. [snip] > @@ -130,14 +130,19 @@ static const char * const noon010_supply_name[] = { > #define NOON010_NUM_SUPPLIES ARRAY_SIZE(noon010_supply_name) > > struct noon010_info { > + /* Mutex protecting this data structure and subdev operations */ > + struct mutex lock; Locks protect data, not operations. You should describe which data members are protected by the lock. > struct v4l2_subdev sd; > + struct media_pad pad; > struct v4l2_ctrl_handler hdl; > const struct noon010pc30_platform_data *pdata; > const struct noon010_format *curr_fmt; > const struct noon010_frmsize *curr_win; > + struct v4l2_mbus_framefmt format; > unsigned int hflip:1; > unsigned int vflip:1; > unsigned int power:1; > + unsigned int streaming:1; > u8 i2c_reg_page; > struct regulator_bulk_data supply[NOON010_NUM_SUPPLIES]; > u32 gpio_nreset; [snip] > @@ -374,6 +380,8 @@ static int noon010_try_frame_size(struct > v4l2_mbus_framefmt *mf) if (match) { > mf->width = match->width; > mf->height = match->height; > + if (size) > + *size = match; > return 0; > } > return -EINVAL; > @@ -464,36 +472,45 @@ static int noon010_s_ctrl(struct v4l2_ctrl *ctrl) [snip] > -static int noon010_g_fmt(struct v4l2_subdev *sd, struct v4l2_mbus_framefmt > *mf) > +static int noon010_get_fmt(struct v4l2_subdev *sd, struct v4l2_subdev_fh > *fh, > + struct v4l2_subdev_format *fmt) > { > struct noon010_info *info = to_noon010(sd); > - int ret; > + struct v4l2_mbus_framefmt *mf; > > - if (!mf) > + if (fmt->pad != 0) > return -EINVAL; subdev_do_ioctl() already validates fmt->pad. > - if (!info->curr_win || !info->curr_fmt) { > - ret = noon010_set_params(sd); > - if (ret) > - return ret; > + if (fmt->which == V4L2_SUBDEV_FORMAT_TRY) { > + if (fh) { > + mf = v4l2_subdev_get_try_format(fh, 0); > + fmt->format = *mf; > + } > + return 0; > } > + /* Active format */ > + mf = &fmt->format; > > + mutex_lock(&info->lock); > mf->width = info->curr_win->width; > mf->height = info->curr_win->height; > mf->code = info->curr_fmt->code; > mf->colorspace = info->curr_fmt->colorspace; > - mf->field = V4L2_FIELD_NONE; > + mutex_unlock(&info->lock); > > + mf->field = V4L2_FIELD_NONE; > + mf->colorspace = V4L2_COLORSPACE_JPEG; > return 0; > } > [snip] > @@ -583,6 +609,17 @@ static int noon010_s_power(struct v4l2_subdev *sd, int > on) return ret; > } > > +static int noon010_s_stream(struct v4l2_subdev *sd, int on) > +{ > + struct noon010_info *info = to_noon010(sd); > + > + mutex_lock(&info->lock); > + info->streaming = on; > + mutex_unlock(&info->lock); Does the sensor produce data continuously, without any way to stop it ? > + > + return 0; > +} > + > static int noon010_g_chip_ident(struct v4l2_subdev *sd, > struct v4l2_dbg_chip_ident *chip) You can get rid of g_chip_ident as well. > { [snip] > @@ -666,9 +707,11 @@ static int noon010_probe(struct i2c_client *client, > if (!info) > return -ENOMEM; > > + mutex_init(&info->lock); > sd = &info->sd; > strlcpy(sd->name, MODULE_NAME, sizeof(sd->name)); > v4l2_i2c_subdev_init(sd, client, &noon010_ops); > + sd->flags = V4L2_SUBDEV_FL_HAS_DEVNODE; You should |= V4L2_SUBDEV_FL_HAS_DEVNODE flag. v4l2_i2c_subdev_init() sets V4L2_SUBDEV_FL_IS_I2C. > v4l2_ctrl_handler_init(&info->hdl, 3);
Hi Laurent, thanks for your review. On 06/25/2011 02:08 AM, Laurent Pinchart wrote: > Hi Sylwester, > > Thanks for the patch. It's nice to see sensor drivers picking up the pad-level > API :-) This is somehow a consequence of converting our camera host driver to the pad-level API. Nevertheless for some of our devices the pad-level API just scales better than regular V4L2 interface. So my goal is to gradually introduce it in our BSP where relevant. > > On Wednesday 22 June 2011 17:44:29 Sylwester Nawrocki wrote: >> Replace g/s_mbus_fmt ops with the pad level get/set_fmt operations. >> Add media entity initialization and set subdev flags so the host driver >> creates a v4l-subdev device node for the driver. A mutex is added for >> serializing operations on subdevice node. When setting format >> is attempted during streaming EBUSY error code will be returned. > > [snip] > >> @@ -130,14 +130,19 @@ static const char * const noon010_supply_name[] = { >> #define NOON010_NUM_SUPPLIES ARRAY_SIZE(noon010_supply_name) >> >> struct noon010_info { >> + /* Mutex protecting this data structure and subdev operations */ >> + struct mutex lock; > > Locks protect data, not operations. You should describe which data members are > protected by the lock. OK, thanks for pointing this out. I'll try to be more precise in the next patch version.:) > >> struct v4l2_subdev sd; >> + struct media_pad pad; >> struct v4l2_ctrl_handler hdl; >> const struct noon010pc30_platform_data *pdata; >> const struct noon010_format *curr_fmt; >> const struct noon010_frmsize *curr_win; >> + struct v4l2_mbus_framefmt format; >> unsigned int hflip:1; >> unsigned int vflip:1; >> unsigned int power:1; >> + unsigned int streaming:1; >> u8 i2c_reg_page; >> struct regulator_bulk_data supply[NOON010_NUM_SUPPLIES]; >> u32 gpio_nreset; > > [snip] > >> @@ -374,6 +380,8 @@ static int noon010_try_frame_size(struct >> v4l2_mbus_framefmt *mf) if (match) { >> mf->width = match->width; >> mf->height = match->height; >> + if (size) >> + *size = match; >> return 0; >> } >> return -EINVAL; >> @@ -464,36 +472,45 @@ static int noon010_s_ctrl(struct v4l2_ctrl *ctrl) > > [snip] > >> -static int noon010_g_fmt(struct v4l2_subdev *sd, struct v4l2_mbus_framefmt >> *mf) >> +static int noon010_get_fmt(struct v4l2_subdev *sd, struct v4l2_subdev_fh >> *fh, >> + struct v4l2_subdev_format *fmt) >> { >> struct noon010_info *info = to_noon010(sd); >> - int ret; >> + struct v4l2_mbus_framefmt *mf; >> >> - if (!mf) >> + if (fmt->pad != 0) >> return -EINVAL; > > subdev_do_ioctl() already validates fmt->pad. OK, I'll get rid of the check. > >> - if (!info->curr_win || !info->curr_fmt) { >> - ret = noon010_set_params(sd); >> - if (ret) >> - return ret; >> + if (fmt->which == V4L2_SUBDEV_FORMAT_TRY) { >> + if (fh) { >> + mf = v4l2_subdev_get_try_format(fh, 0); >> + fmt->format = *mf; >> + } >> + return 0; >> } >> + /* Active format */ >> + mf =&fmt->format; >> >> + mutex_lock(&info->lock); >> mf->width = info->curr_win->width; >> mf->height = info->curr_win->height; >> mf->code = info->curr_fmt->code; >> mf->colorspace = info->curr_fmt->colorspace; >> - mf->field = V4L2_FIELD_NONE; >> + mutex_unlock(&info->lock); >> >> + mf->field = V4L2_FIELD_NONE; >> + mf->colorspace = V4L2_COLORSPACE_JPEG; >> return 0; >> } >> > > [snip] > >> @@ -583,6 +609,17 @@ static int noon010_s_power(struct v4l2_subdev *sd, int >> on) return ret; >> } >> >> +static int noon010_s_stream(struct v4l2_subdev *sd, int on) >> +{ >> + struct noon010_info *info = to_noon010(sd); >> + >> + mutex_lock(&info->lock); >> + info->streaming = on; >> + mutex_unlock(&info->lock); > > Does the sensor produce data continuously, without any way to stop it ? Hmm, looks like not enough attention to that from my side. The sensor has a software "power sleep" mode in which it is supposed to not generate an output signal and it tri-states its output pins. All registers' state is retained and the normal I2C register access should be possible. I'll look into details in the datasheet. I think the driver could be leaving the sensor chip in such 'suspended' state after s_power(1) and be switching it into normal operation within s_stream(1). > >> + >> + return 0; >> +} >> + >> static int noon010_g_chip_ident(struct v4l2_subdev *sd, >> struct v4l2_dbg_chip_ident *chip) > > You can get rid of g_chip_ident as well. All right, when I was originally writing this driver I thought it was mandatory to implement g_chip_indent. In fact it was never really used so far, so I'm going to do away with it in next iteration. > >> { > > [snip] > >> @@ -666,9 +707,11 @@ static int noon010_probe(struct i2c_client *client, >> if (!info) >> return -ENOMEM; >> >> + mutex_init(&info->lock); >> sd =&info->sd; >> strlcpy(sd->name, MODULE_NAME, sizeof(sd->name)); >> v4l2_i2c_subdev_init(sd, client,&noon010_ops); >> + sd->flags = V4L2_SUBDEV_FL_HAS_DEVNODE; > > You should |= V4L2_SUBDEV_FL_HAS_DEVNODE flag. v4l2_i2c_subdev_init() sets > V4L2_SUBDEV_FL_IS_I2C. Oops, my bad. Thanks, I'll fix this. > >> v4l2_ctrl_handler_init(&info->hdl, 3); > -- Regards, Sylwester -- 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/Kconfig b/drivers/media/video/Kconfig index 8de3476..b505120 100644 --- a/drivers/media/video/Kconfig +++ b/drivers/media/video/Kconfig @@ -746,7 +746,7 @@ config VIDEO_VIA_CAMERA config VIDEO_NOON010PC30 tristate "NOON010PC30 CIF camera sensor support" - depends on I2C && VIDEO_V4L2 + depends on I2C && VIDEO_V4L2 && EXPERIMENTAL && VIDEO_V4L2_SUBDEV_API ---help--- This driver supports NOON010PC30 CIF camera from Siliconfile diff --git a/drivers/media/video/noon010pc30.c b/drivers/media/video/noon010pc30.c index 50ca097..6920cc4 100644 --- a/drivers/media/video/noon010pc30.c +++ b/drivers/media/video/noon010pc30.c @@ -1,7 +1,7 @@ /* * Driver for SiliconFile NOON010PC30 CIF (1/11") Image Sensor with ISP * - * Copyright (C) 2010 Samsung Electronics + * Copyright (C) 2010 - 2011 Samsung Electronics * Contact: Sylwester Nawrocki, <s.nawrocki@samsung.com> * * Initial register configuration based on a driver authored by @@ -130,14 +130,19 @@ static const char * const noon010_supply_name[] = { #define NOON010_NUM_SUPPLIES ARRAY_SIZE(noon010_supply_name) struct noon010_info { + /* Mutex protecting this data structure and subdev operations */ + struct mutex lock; struct v4l2_subdev sd; + struct media_pad pad; struct v4l2_ctrl_handler hdl; const struct noon010pc30_platform_data *pdata; const struct noon010_format *curr_fmt; const struct noon010_frmsize *curr_win; + struct v4l2_mbus_framefmt format; unsigned int hflip:1; unsigned int vflip:1; unsigned int power:1; + unsigned int streaming:1; u8 i2c_reg_page; struct regulator_bulk_data supply[NOON010_NUM_SUPPLIES]; u32 gpio_nreset; @@ -342,7 +347,7 @@ static int noon010_set_params(struct v4l2_subdev *sd) struct noon010_info *info = to_noon010(sd); int ret; - if (!info->curr_win) + if (!info->curr_win || !info->power) return -EINVAL; ret = cam_i2c_write(sd, VDO_CTL_REG(0), info->curr_win->vid_ctl1); @@ -354,7 +359,8 @@ static int noon010_set_params(struct v4l2_subdev *sd) } /* Find nearest matching image pixel size. */ -static int noon010_try_frame_size(struct v4l2_mbus_framefmt *mf) +static int noon010_try_frame_size(struct v4l2_mbus_framefmt *mf, + const struct noon010_frmsize **size) { unsigned int min_err = ~0; int i = ARRAY_SIZE(noon010_sizes); @@ -374,6 +380,8 @@ static int noon010_try_frame_size(struct v4l2_mbus_framefmt *mf) if (match) { mf->width = match->width; mf->height = match->height; + if (size) + *size = match; return 0; } return -EINVAL; @@ -464,36 +472,45 @@ static int noon010_s_ctrl(struct v4l2_ctrl *ctrl) } } -static int noon010_enum_fmt(struct v4l2_subdev *sd, unsigned int index, - enum v4l2_mbus_pixelcode *code) +static int noon010_enum_mbus_code(struct v4l2_subdev *sd, + struct v4l2_subdev_fh *fh, + struct v4l2_subdev_mbus_code_enum *code) { - if (!code || index >= ARRAY_SIZE(noon010_formats)) + if (!code || code->index >= ARRAY_SIZE(noon010_formats)) return -EINVAL; - *code = noon010_formats[index].code; + code->code = noon010_formats[code->index].code; return 0; } -static int noon010_g_fmt(struct v4l2_subdev *sd, struct v4l2_mbus_framefmt *mf) +static int noon010_get_fmt(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh, + struct v4l2_subdev_format *fmt) { struct noon010_info *info = to_noon010(sd); - int ret; + struct v4l2_mbus_framefmt *mf; - if (!mf) + if (fmt->pad != 0) return -EINVAL; - if (!info->curr_win || !info->curr_fmt) { - ret = noon010_set_params(sd); - if (ret) - return ret; + if (fmt->which == V4L2_SUBDEV_FORMAT_TRY) { + if (fh) { + mf = v4l2_subdev_get_try_format(fh, 0); + fmt->format = *mf; + } + return 0; } + /* Active format */ + mf = &fmt->format; + mutex_lock(&info->lock); mf->width = info->curr_win->width; mf->height = info->curr_win->height; mf->code = info->curr_fmt->code; mf->colorspace = info->curr_fmt->colorspace; - mf->field = V4L2_FIELD_NONE; + mutex_unlock(&info->lock); + mf->field = V4L2_FIELD_NONE; + mf->colorspace = V4L2_COLORSPACE_JPEG; return 0; } @@ -503,38 +520,47 @@ static const struct noon010_format *try_fmt(struct v4l2_subdev *sd, { int i = ARRAY_SIZE(noon010_formats); - noon010_try_frame_size(mf); - - while (i--) + while (--i) if (mf->code == noon010_formats[i].code) break; - mf->code = noon010_formats[i].code; return &noon010_formats[i]; } -static int noon010_try_fmt(struct v4l2_subdev *sd, - struct v4l2_mbus_framefmt *mf) -{ - if (!sd || !mf) - return -EINVAL; - - try_fmt(sd, mf); - return 0; -} - -static int noon010_s_fmt(struct v4l2_subdev *sd, - struct v4l2_mbus_framefmt *mf) +static int noon010_set_fmt(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh, + struct v4l2_subdev_format *fmt) { struct noon010_info *info = to_noon010(sd); + const struct noon010_frmsize *size = NULL; + const struct noon010_format *nf; + struct v4l2_mbus_framefmt *mf; + int ret; - if (!sd || !mf) + if (fmt->pad != 0) return -EINVAL; - info->curr_fmt = try_fmt(sd, mf); + nf = try_fmt(sd, &fmt->format); + noon010_try_frame_size(&fmt->format, &size); + fmt->format.colorspace = V4L2_COLORSPACE_JPEG; - return noon010_set_params(sd); + if (fmt->which == V4L2_SUBDEV_FORMAT_TRY) { + if (fh) { + mf = v4l2_subdev_get_try_format(fh, 0); + *mf = fmt->format; + } + return 0; + } + mutex_lock(&info->lock); + if (info->streaming) { + ret = -EBUSY; + } else { + info->curr_fmt = nf; + info->curr_win = size; + ret = noon010_set_params(sd); + } + mutex_unlock(&info->lock); + return ret; } static int noon010_base_config(struct v4l2_subdev *sd) @@ -583,6 +609,17 @@ static int noon010_s_power(struct v4l2_subdev *sd, int on) return ret; } +static int noon010_s_stream(struct v4l2_subdev *sd, int on) +{ + struct noon010_info *info = to_noon010(sd); + + mutex_lock(&info->lock); + info->streaming = on; + mutex_unlock(&info->lock); + + return 0; +} + static int noon010_g_chip_ident(struct v4l2_subdev *sd, struct v4l2_dbg_chip_ident *chip) { @@ -617,15 +654,19 @@ static const struct v4l2_subdev_core_ops noon010_core_ops = { .log_status = noon010_log_status, }; -static const struct v4l2_subdev_video_ops noon010_video_ops = { - .g_mbus_fmt = noon010_g_fmt, - .s_mbus_fmt = noon010_s_fmt, - .try_mbus_fmt = noon010_try_fmt, - .enum_mbus_fmt = noon010_enum_fmt, +static struct v4l2_subdev_pad_ops noon010_pad_ops = { + .enum_mbus_code = noon010_enum_mbus_code, + .get_fmt = noon010_get_fmt, + .set_fmt = noon010_set_fmt, +}; + +static struct v4l2_subdev_video_ops noon010_video_ops = { + .s_stream = noon010_s_stream, }; static const struct v4l2_subdev_ops noon010_ops = { .core = &noon010_core_ops, + .pad = &noon010_pad_ops, .video = &noon010_video_ops, }; @@ -666,9 +707,11 @@ static int noon010_probe(struct i2c_client *client, if (!info) return -ENOMEM; + mutex_init(&info->lock); sd = &info->sd; strlcpy(sd->name, MODULE_NAME, sizeof(sd->name)); v4l2_i2c_subdev_init(sd, client, &noon010_ops); + sd->flags = V4L2_SUBDEV_FL_HAS_DEVNODE; v4l2_ctrl_handler_init(&info->hdl, 3); @@ -720,11 +763,16 @@ static int noon010_probe(struct i2c_client *client, if (ret) goto np_reg_err; + info->pad.flags = MEDIA_PAD_FL_SOURCE; + ret = media_entity_init(&sd->entity, 1, &info->pad, 0); + if (ret < 0) + goto np_me_err; + ret = noon010_detect(client, info); if (!ret) return 0; - /* the sensor detection failed */ +np_me_err: regulator_bulk_free(NOON010_NUM_SUPPLIES, info->supply); np_reg_err: if (gpio_is_valid(info->gpio_nstby)) @@ -751,10 +799,10 @@ static int noon010_remove(struct i2c_client *client) if (gpio_is_valid(info->gpio_nreset)) gpio_free(info->gpio_nreset); - if (gpio_is_valid(info->gpio_nstby)) gpio_free(info->gpio_nstby); + media_entity_cleanup(&sd->entity); kfree(info); return 0; }