Hi Laurent,
On Mon, Feb 27, 2012 at 04:38:49PM +0100, Laurent Pinchart wrote:
> Hi Sakari,
>
> Thanks for the patch.
Thanks for the review!!
> On Monday 20 February 2012 03:57:11 Sakari Ailus wrote:
> > From: Sakari Ailus <sakari.ailus@xxxxxxxxxxxxxxxxxxxxxxxxxx>
> >
> > Add driver for SMIA++/SMIA image sensors. The driver exposes the sensor as
> > three subdevs, pixel array, binner and scaler --- in case the device has a
> > scaler.
> >
> > Currently it relies on the board code for external clock handling. There is
> > no fast way out of this dependency before the ISP drivers (omap3isp) among
> > others will be able to export that clock through the clock framework
> > instead.
> >
> > Signed-off-by: Sakari Ailus <sakari.ailus@xxxxxxxxxxxxxxxxxxxxxxxxxx>
>
> [snip]
>
> > diff --git a/drivers/media/video/smiapp/Kconfig
> > b/drivers/media/video/smiapp/Kconfig new file mode 100644
> > index 0000000..3f98e8e
> > --- /dev/null
> > +++ b/drivers/media/video/smiapp/Kconfig
> > @@ -0,0 +1,12 @@
> > +config VIDEO_SMIAPP
> > + tristate "SMIA++/SMIA sensor support"
> > + depends on I2C && VIDEO_V4L2 && VIDEO_V4L2_SUBDEV_API && MEDIA_CONTROLLER
>
> VIDEO_V4L2_SUBDEV_API depends on MEDIA_CONTROLLER, you don't have to set the
> dependency explicitly.
Ok. I'll remove the dependency.
> > + ---help---
> > + This is a generic driver for SMIA++/SMIA camera modules.
> > +
> > +config VIDEO_SMIAPP_DEBUG
> > + bool "Enable debugging for the generic SMIA++/SMIA driver"
> > + depends on VIDEO_SMIAPP
> > + ---help---
> > + Enable debugging output in the generic SMIA++/SMIA driver. If you
> > + are developing the driver you might want to enable this.
>
> [snip]
>
> > diff --git a/drivers/media/video/smiapp/smiapp-core.c
> > b/drivers/media/video/smiapp/smiapp-core.c new file mode 100644
> > index 0000000..9fd08a1
> > --- /dev/null
> > +++ b/drivers/media/video/smiapp/smiapp-core.c
>
> [snip]
>
> > +#define SMIAPP_ALIGN_DIM(dim, flags) \
> > + (flags & V4L2_SUBDEV_SEL_FLAG_SIZE_GE \
> > + ? ALIGN(dim, 2) \
> > + : dim & ~1)
>
> Please enclose dim in parenthesis in the macro definition.
Flags, too...
> > +
> > +/*
> > + * smiapp_module_idents - supported camera modules
> > + */
> > +static const struct smiapp_module_ident smiapp_module_idents[] = {
> > + SMIAPP_IDENT_LQ(0x10, 0x4141, -1, "jt8ev1", &smiapp_jt8ev1_quirk),
> > + SMIAPP_IDENT_LQ(0x10, 0x4241, -1, "imx125es", &smiapp_imx125es_quirk),
> > + SMIAPP_IDENT_L(0x01, 0x022b, -1, "vs6555"),
> > + SMIAPP_IDENT_L(0x0c, 0x208a, -1, "tcm8330md"),
> > + SMIAPP_IDENT_L(0x01, 0x022e, -1, "vw6558"),
> > + SMIAPP_IDENT_LQ(0x0c, 0x2134, -1, "tcm8500md", &smiapp_tcm8500md_quirk),
> > + SMIAPP_IDENT_L(0x07, 0x7698, -1, "ovm7698"),
> > + SMIAPP_IDENT_L(0x0b, 0x4242, -1, "smiapp-003"),
> > + SMIAPP_IDENT_LQ(0x0c, 0x560f, -1, "jt8ew9", &smiapp_jt8ew9_quirk),
> > + SMIAPP_IDENT_L(0x0c, 0x213e, -1, "et8en2"),
> > + SMIAPP_IDENT_L(0x0c, 0x2184, -1, "tcm8580md"),
> > +};
>
> What about sorting those either by ID or name ?
By ID it makes more sense, I think.
> > +
> > +/*
> > + *
> > + * Dynamic Capability Identification
> > + *
> > + */
> > +
> > +static int smiapp_read_frame_fmt(struct smiapp_sensor *sensor)
> > +{
> > + struct i2c_client *client = v4l2_get_subdevdata(&sensor->src->sd);
> > + u32 fmt_model_type, fmt_model_subtype, ncol_desc, nrow_desc;
> > + int i;
> > + int rval;
> > + int line_count = 0;
> > + int embedded_start = -1, embedded_end = -1;
> > + int image_start = 0;
> > +
> > + rval = smia_i2c_read_reg(client,
> > + SMIAPP_REG_U8_FRAME_FORMAT_MODEL_TYPE,
> > + &fmt_model_type);
> > + if (rval)
> > + return rval;
> > +
> > + rval = smia_i2c_read_reg(client,
> > + SMIAPP_REG_U8_FRAME_FORMAT_MODEL_SUBTYPE,
> > + &fmt_model_subtype);
> > + if (rval)
> > + return rval;
> > +
> > + ncol_desc = (fmt_model_subtype
> > + & SMIAPP_FRAME_FORMAT_MODEL_SUBTYPE_NCOLS_MASK)
> > + >> SMIAPP_FRAME_FORMAT_MODEL_SUBTYPE_NCOLS_SHIFT;
> > + nrow_desc = (fmt_model_subtype
> > + & SMIAPP_FRAME_FORMAT_MODEL_SUBTYPE_NROWS_MASK);
>
> No need for parenthesis.
Fixed.
> > +
> > + dev_dbg(&client->dev, "format_model_type %s\n",
> > + fmt_model_type == SMIAPP_FRAME_FORMAT_MODEL_TYPE_2BYTE
> > + ? "2 byte" :
> > + fmt_model_type == SMIAPP_FRAME_FORMAT_MODEL_TYPE_4BYTE
> > + ? "4 byte" : "is simply bad");
>
> Simply ? :-)
Yeah, well, it's not that complex. :-) Do you think I should change that to
something else?
> > +
> > + for (i = 0; i < ncol_desc + nrow_desc; i++) {
> > + u32 desc;
> > + u32 pixelcode;
> > + u32 pixels;
> > + char *which;
> > + char *what;
> > +
> > + if (fmt_model_type == SMIAPP_FRAME_FORMAT_MODEL_TYPE_2BYTE) {
> > + rval = smia_i2c_read_reg(
> > + client,
> > + SMIAPP_REG_U16_FRAME_FORMAT_DESCRIPTOR_2(i),
> > + &desc);
> > + if (rval)
> > + return rval;
> > +
> > + pixelcode =
> > + (desc
> > + & SMIAPP_FRAME_FORMAT_DESC_2_PIXELCODE_MASK)
> > + >> SMIAPP_FRAME_FORMAT_DESC_2_PIXELCODE_SHIFT;
> > + pixels = desc & SMIAPP_FRAME_FORMAT_DESC_2_PIXELS_MASK;
> > + } else if (fmt_model_type
> > + == SMIAPP_FRAME_FORMAT_MODEL_TYPE_4BYTE) {
> > + rval = smia_i2c_read_reg(
> > + client,
> > + SMIAPP_REG_U32_FRAME_FORMAT_DESCRIPTOR_4(i),
> > + &desc);
> > + if (rval)
> > + return rval;
> > +
> > + pixelcode =
> > + (desc
> > + & SMIAPP_FRAME_FORMAT_DESC_4_PIXELCODE_MASK)
> > + >> SMIAPP_FRAME_FORMAT_DESC_4_PIXELCODE_SHIFT;
> > + pixels = desc & SMIAPP_FRAME_FORMAT_DESC_4_PIXELS_MASK;
> > + } else {
> > + dev_dbg(&client->dev,
> > + "invalid frame format model type %d\n",
> > + fmt_model_type);
> > + return -EINVAL;
> > + }
> > +
> > + if (i < ncol_desc)
> > + which = "columns";
> > + else
> > + which = "rows";
> > +
> > + switch (pixelcode) {
> > + case SMIAPP_FRAME_FORMAT_DESC_PIXELCODE_EMBEDDED:
> > + what = "embedded";
> > + break;
> > + case SMIAPP_FRAME_FORMAT_DESC_PIXELCODE_DUMMY:
> > + what = "dummy";
> > + break;
> > + case SMIAPP_FRAME_FORMAT_DESC_PIXELCODE_BLACK:
> > + what = "black";
> > + break;
> > + case SMIAPP_FRAME_FORMAT_DESC_PIXELCODE_DARK:
> > + what = "dark";
> > + break;
> > + case SMIAPP_FRAME_FORMAT_DESC_PIXELCODE_VISIBLE:
> > + what = "visible";
> > + break;
> > + default:
> > + what = "invalid";
> > + dev_dbg(&client->dev, "pixelcode %d\n", pixelcode);
> > + break;
> > + }
> > +
> > + dev_dbg(&client->dev, "%s pixels: %d %s\n",
> > + what, pixels, which);
> > +
> > + if (i < ncol_desc)
> > + continue;
> > +
> > + /* Handle row descriptors */
> > + if (pixelcode
> > + == SMIAPP_FRAME_FORMAT_DESC_PIXELCODE_EMBEDDED) {
> > + embedded_start = line_count;
> > + } else {
> > + if (pixelcode
> > + == SMIAPP_FRAME_FORMAT_DESC_PIXELCODE_VISIBLE
> > + || pixels
> > + >= sensor->limits[
> > + SMIAPP_LIMIT_MIN_FRAME_LENGTH_LINES] / 2) {
>
> I'm not a huge fan of lines larger than 80 columns, but it would make sense
> here. This is hard to read. An option would be to shorten all the constants a
> bit.
Done. I also realised the braces were not needed.
I'd prefer to keep the names since they're quite descriptive this way, and
are local to the driver..
> > + image_start = line_count;
> > + }
> > + if (embedded_start != -1 && embedded_end == -1)
> > + embedded_end = line_count;
> > + }
> > + line_count += pixels;
> > + }
> > +
> > + if (embedded_start == -1 || embedded_end == -1)
> > + embedded_start = embedded_end = 0;
>
> One assignment per line please.
I'm not sure if you gain any clarity with that, but I guess it's a norm
still. Fixed.
> > +
> > + dev_dbg(&client->dev, "embedded data from lines %d to %d\n",
> > + embedded_start, embedded_end);
> > + dev_dbg(&client->dev, "image data starts at line %d\n", image_start);
> > +
> > + return 0;
> > +}
> > +
> > +/*
> > + *
> > + * V4L2 Controls handling
> > + *
> > + */
> > +
> > +static void __smiapp_update_exposure_limits(struct smiapp_sensor *sensor)
> > +{
> > + struct v4l2_ctrl *ctrl = sensor->exposure;
> > + int max;
> > +
> > + max = sensor->pixel_array->crop[SMIAPP_PAD_SOURCE].height
> > + + sensor->vblank->val -
> > + sensor->limits[SMIAPP_LIMIT_COARSE_INTEGRATION_TIME_MAX_MARGIN];
>
> Just for reference, I would have found
>
> max = sensor->pixel_array->crop[SMIAPP_PAD_SOURCE].height
> + sensor->vblank->val
> - sensor->limits[SMIAPP_LIMIT_COARSE_INTEGRATION_TIME_MAX_MARGIN];
>
> to be more readable, but it's obviously your call.
Moved "-" to the previous line.
> > +
> > + ctrl->maximum = max;
> > + if (ctrl->default_value > max)
> > + ctrl->default_value = max;
> > + if (ctrl->val > max)
> > + ctrl->val = max;
> > + if (ctrl->cur.val > max)
> > + ctrl->cur.val = max;
> > +}
> > +
> > +/*
> > + * Order matters.
> > + *
> > + * 1. Bits-per-pixel, descending.
> > + * 2. Bits-per-pixel compressed, descending.
> > + * 3. Pixel order, same as in pixel_order_str. Formats for all four pixel
> > + * orders must be defined.
> > + */
> > +static const struct smiapp_csi_data_format smiapp_csi_data_formats[] = {
> > + { V4L2_MBUS_FMT_SGRBG12_1X12, 12, 12, SMIAPP_PIXEL_ORDER_GRBG, },
> > + { V4L2_MBUS_FMT_SRGGB12_1X12, 12, 12, SMIAPP_PIXEL_ORDER_RGGB, },
> > + { V4L2_MBUS_FMT_SBGGR12_1X12, 12, 12, SMIAPP_PIXEL_ORDER_BGGR, },
> > + { V4L2_MBUS_FMT_SGBRG12_1X12, 12, 12, SMIAPP_PIXEL_ORDER_GBRG, },
> > + { V4L2_MBUS_FMT_SGRBG10_1X10, 10, 10, SMIAPP_PIXEL_ORDER_GRBG, },
> > + { V4L2_MBUS_FMT_SRGGB10_1X10, 10, 10, SMIAPP_PIXEL_ORDER_RGGB, },
> > + { V4L2_MBUS_FMT_SBGGR10_1X10, 10, 10, SMIAPP_PIXEL_ORDER_BGGR, },
> > + { V4L2_MBUS_FMT_SGBRG10_1X10, 10, 10, SMIAPP_PIXEL_ORDER_GBRG, },
> > + { V4L2_MBUS_FMT_SGRBG10_DPCM8_1X8, 10, 8, SMIAPP_PIXEL_ORDER_GRBG, },
> > + { V4L2_MBUS_FMT_SRGGB10_DPCM8_1X8, 10, 8, SMIAPP_PIXEL_ORDER_RGGB, },
> > + { V4L2_MBUS_FMT_SBGGR10_DPCM8_1X8, 10, 8, SMIAPP_PIXEL_ORDER_BGGR, },
> > + { V4L2_MBUS_FMT_SGBRG10_DPCM8_1X8, 10, 8, SMIAPP_PIXEL_ORDER_GBRG, },
> > +};
> > +
> > +const char *pixel_order_str[] = { "GRBG", "RGGB", "BGGR", "GBRG" };
> > +
> > +#define to_csi_format_idx(fmt) (((unsigned long)(fmt) \
> > + - (unsigned long)smiapp_csi_data_formats) \
> > + / sizeof(*smiapp_csi_data_formats))
> > +
> > +static void smiapp_update_mbus_formats(struct smiapp_sensor *sensor)
> > +{
> > + struct i2c_client *client = v4l2_get_subdevdata(&sensor->src->sd);
> > + int csi_format_idx = to_csi_format_idx(sensor->csi_format) & ~3;
> > + int internal_csi_format_idx =
> > + to_csi_format_idx(sensor->internal_csi_format) & ~3;
> > + int flip = 0;
> > + int pixel_order;
>
> Here again shortening names a bit might help.
>
> > +
> > + if (sensor->hflip) {
> > + if (sensor->hflip->val)
> > + flip |= SMIAPP_IMAGE_ORIENTATION_HFLIP;
> > +
> > + if (sensor->vflip->val)
> > + flip |= SMIAPP_IMAGE_ORIENTATION_VFLIP;
> > + }
> > +
> > + flip ^= sensor->hvflip_inv_mask;
> > +
> > + pixel_order = sensor->default_pixel_order ^ flip;
> > +
> > + sensor->mbus_frame_fmts =
> > + sensor->default_mbus_frame_fmts << pixel_order;
> > + sensor->csi_format =
> > + &smiapp_csi_data_formats[csi_format_idx + pixel_order];
> > + sensor->internal_csi_format =
> > + &smiapp_csi_data_formats[internal_csi_format_idx
> > + + pixel_order];
> > +
> > + BUG_ON(max(internal_csi_format_idx, csi_format_idx) + pixel_order
> > + >= ARRAY_SIZE(smiapp_csi_data_formats));
> > + BUG_ON(min(internal_csi_format_idx, csi_format_idx) < 0);
> > +
> > + dev_dbg(&client->dev, "flip %d; new pixel order %s\n",
> > + flip, pixel_order_str[pixel_order]);
> > +}
> > +
> > +static int smiapp_set_ctrl(struct v4l2_ctrl *ctrl)
> > +{
> > + struct smiapp_sensor *sensor =
> > + container_of(ctrl->handler, struct smiapp_subdev, ctrl_handler)
> > + ->sensor;
> > + struct i2c_client *client = v4l2_get_subdevdata(&sensor->src->sd);
> > + u32 orient;
> > + int exposure;
> > + int rval = 0;
> > +
> > + switch (ctrl->id) {
> > + case V4L2_CID_ANALOGUE_GAIN:
> > + return smia_i2c_write_reg(
> > + client,
> > + SMIAPP_REG_U16_ANALOGUE_GAIN_CODE_GLOBAL, ctrl->val);
> > +
> > + case V4L2_CID_EXPOSURE:
> > + return smia_i2c_write_reg(
> > + client,
> > + SMIAPP_REG_U16_COARSE_INTEGRATION_TIME, ctrl->val);
> > +
> > + case V4L2_CID_HFLIP:
> > + case V4L2_CID_VFLIP:
> > + orient = 0;
>
> You can move this line after the streaming check.
Moved the assignment to initialisation.
> > +
> > + if (sensor->streaming)
> > + return -EBUSY;
> > +
> > + if (sensor->hflip->val)
> > + orient |= SMIAPP_IMAGE_ORIENTATION_HFLIP;
> > +
> > + if (sensor->vflip->val)
> > + orient |= SMIAPP_IMAGE_ORIENTATION_VFLIP;
> > +
> > + orient ^= sensor->hvflip_inv_mask;
> > + rval = smia_i2c_write_reg(client,
> > + SMIAPP_REG_U8_IMAGE_ORIENTATION,
> > + orient);
> > + if (rval < 0)
> > + return rval;
> > +
> > + smiapp_update_mbus_formats(sensor);
> > +
> > + return 0;
> > +
> > + case V4L2_CID_VBLANK:
> > + exposure = sensor->exposure->val;
> > +
> > + __smiapp_update_exposure_limits(sensor);
> > +
> > + if (exposure > sensor->exposure->maximum) {
> > + sensor->exposure->val =
> > + sensor->exposure->maximum;
> > + rval = smiapp_set_ctrl(
> > + sensor->exposure);
>
> Shouldn't you call the V4L2 control API here instead ? Otherwise no control
> change event will be generated for the exposure time. Will this work as
> expected if the user sets the exposure time in the same VIDIOC_S_EXT_CTRLS
> call ?
Good question. I'm holding the ctrl handler lock here and so can't use the
regular functions to perform the change. Perhaps time to implement
__v4l2_subdev_s_ext_ctrls() and use that?
> > + }
> > +
> > + if (rval < 0)
> > + return rval;
>
> If you moved this check inside the if you wouldn't have to initialize rval to
> 0 above.
Fixed.
> > +
> > + return smia_i2c_write_reg(
> > + client, SMIAPP_REG_U16_FRAME_LENGTH_LINES,
> > + sensor->pixel_array->crop[SMIAPP_PAD_SOURCE].height
> > + + ctrl->val);
> > +
> > + case V4L2_CID_HBLANK:
> > + return smia_i2c_write_reg(
> > + client, SMIAPP_REG_U16_LINE_LENGTH_PCK,
> > + sensor->pixel_array->crop[SMIAPP_PAD_SOURCE].width
> > + + ctrl->val);
> > +
> > + case V4L2_CID_LINK_FREQ:
> > + if (sensor->streaming)
> > + return -EBUSY;
> > +
> > + return smiapp_pll_update(sensor);
> > +
> > + default:
> > + return -EINVAL;
> > + }
> > +}
> > +
> > +static const struct v4l2_ctrl_ops smiapp_ctrl_ops = {
> > + .s_ctrl = smiapp_set_ctrl,
> > +};
> > +
> > +static int smiapp_init_controls(struct smiapp_sensor *sensor)
> > +{
> > + struct i2c_client *client = v4l2_get_subdevdata(&sensor->src->sd);
> > + struct v4l2_ctrl_config cfg;
> > + int rval;
> > +
> > + rval = v4l2_ctrl_handler_init(&sensor->pixel_array->ctrl_handler, 7);
> > + if (rval)
> > + return rval;
> > + sensor->pixel_array->ctrl_handler.lock = &sensor->mutex;
> > +
> > + sensor->analog_gain = v4l2_ctrl_new_std(
> > + &sensor->pixel_array->ctrl_handler, &smiapp_ctrl_ops,
> > + V4L2_CID_ANALOGUE_GAIN,
> > + sensor->limits[SMIAPP_LIMIT_ANALOGUE_GAIN_CODE_MIN],
> > + sensor->limits[SMIAPP_LIMIT_ANALOGUE_GAIN_CODE_MAX],
> > + max_t(int,
> > + sensor->limits[SMIAPP_LIMIT_ANALOGUE_GAIN_CODE_STEP], 1),
>
> Won't max() work ? You might have to use 1U though.
Thanks. Fixed.
> > + sensor->limits[SMIAPP_LIMIT_ANALOGUE_GAIN_CODE_MIN]);
> > +
> > + sensor->exposure = v4l2_ctrl_new_std(
> > + &sensor->pixel_array->ctrl_handler, &smiapp_ctrl_ops,
> > + V4L2_CID_EXPOSURE,
> > + sensor->limits[SMIAPP_LIMIT_COARSE_INTEGRATION_TIME_MIN],
> > + sensor->limits[SMIAPP_LIMIT_COARSE_INTEGRATION_TIME_MIN], 1,
> > + sensor->limits[SMIAPP_LIMIT_COARSE_INTEGRATION_TIME_MIN]);
>
> Maybe a short comment explaining where this (and other controls below) will be
> updated would help future readers to figure out why maximum == minimum.
I initialise them to zero now and added a comment to explain it.
> > +
> > + sensor->hflip = v4l2_ctrl_new_std(
> > + &sensor->pixel_array->ctrl_handler, &smiapp_ctrl_ops,
> > + V4L2_CID_HFLIP, 0, 1, 1, 0);
> > + sensor->vflip = v4l2_ctrl_new_std(
> > + &sensor->pixel_array->ctrl_handler, &smiapp_ctrl_ops,
> > + V4L2_CID_VFLIP, 0, 1, 1, 0);
> > +
> > + sensor->vblank = v4l2_ctrl_new_std(
> > + &sensor->pixel_array->ctrl_handler, &smiapp_ctrl_ops,
> > + V4L2_CID_VBLANK, 0, 1, 1, 0);
> > +
> > + if (sensor->vblank)
> > + sensor->vblank->flags |= V4L2_CTRL_FLAG_UPDATE;
> > +
> > + sensor->hblank = v4l2_ctrl_new_std(
> > + &sensor->pixel_array->ctrl_handler, &smiapp_ctrl_ops,
> > + V4L2_CID_HBLANK, 0, 1, 1, 0);
> > +
> > + if (sensor->hblank)
> > + sensor->hblank->flags |= V4L2_CTRL_FLAG_UPDATE;
> > +
> > + sensor->pixel_rate_parray = v4l2_ctrl_new_std(
> > + &sensor->pixel_array->ctrl_handler, &smiapp_ctrl_ops,
> > + V4L2_CID_PIXEL_RATE, 0, 0, 1, 0);
> > +
> > + if (sensor->pixel_array->ctrl_handler.error) {
> > + dev_err(&client->dev,
> > + "pixel array controls initialization failed (%d)\n",
> > + sensor->pixel_array->ctrl_handler.error);
>
> Shouldn't you call v4l2_ctrl_handler_free() here ?
Yes. Fixed.
> > + return sensor->pixel_array->ctrl_handler.error;
> > + }
> > +
> > + sensor->pixel_array->sd.ctrl_handler =
> > + &sensor->pixel_array->ctrl_handler;
> > +
> > + v4l2_ctrl_cluster(2, &sensor->hflip);
>
> Shouldn't you move this before the control handler check ?
Why? It can't fail.
> > +
> > + rval = v4l2_ctrl_handler_init(&sensor->binner->ctrl_handler, 0);
> > + if (rval)
> > + return rval;
>
> The pixel array control handler won't be freed if this fails. Same for the
> other error cases below.
>
> > + sensor->binner->ctrl_handler.lock = &sensor->mutex;
>
> Just curious, what's the point in having an empty control handler ? Same
> question for the scaler. Is it because sensor->src will end up pointing to
> either the binner or scaler ? Maybe you could then just initialize
> sensor->src->ctrl_handler then.
Good idea. Fixed. I think I originally thought also binner would have
controls but currently there are none. There probably actually will be some,
but it's easy to change the code back when that's needed.
> > +
> > + if (sensor->scaler) {
> > + rval = v4l2_ctrl_handler_init(&sensor->scaler->ctrl_handler, 0);
> > + if (rval)
> > + return rval;
> > + sensor->scaler->ctrl_handler.lock = &sensor->mutex;
> > + }
> > +
> > + memset(&cfg, 0, sizeof(cfg));
> > +
> > + cfg.ops = &smiapp_ctrl_ops;
> > + cfg.id = V4L2_CID_LINK_FREQ;
> > + cfg.type = V4L2_CTRL_TYPE_INTEGER_MENU;
> > + while (sensor->platform_data->op_sys_clock[cfg.max])
> > + cfg.max++;
> > + cfg.max--;
>
> Maybe
>
> while (sensor->platform_data->op_sys_clock[cfg.max+1])
> cfg.max++;
>
> ? Not sure if the compiler will optimize that better though.
We don't care about compiler optimisations here; this is run once per sensor
registration. Nevertheless, it's probably better. There always must be at
least one possible setting for the link frequency.
> > + cfg.qmenu_int = sensor->platform_data->op_sys_clock;
> > +
> > + sensor->link_freq = v4l2_ctrl_new_custom(
> > + &sensor->src->ctrl_handler, &cfg, NULL);
> > +
> > + sensor->pixel_rate_csi = v4l2_ctrl_new_std(
> > + &sensor->src->ctrl_handler, &smiapp_ctrl_ops,
> > + V4L2_CID_PIXEL_RATE, 0, 0, 1, 0);
> > +
> > + if (sensor->src->ctrl_handler.error) {
> > + dev_err(&client->dev,
> > + "src controls initialization failed (%d)\n",
> > + sensor->src->ctrl_handler.error);
> > + return sensor->src->ctrl_handler.error;
> > + }
> > +
> > + sensor->src->sd.ctrl_handler =
> > + &sensor->src->ctrl_handler;
> > +
> > + return 0;
> > +}
> > +
> > +static void smiapp_free_controls(struct smiapp_sensor *sensor)
> > +{
> > + int i;
>
> unsigned ?
Fixed.
> > +
> > + for (i = 0; i < sensor->sds_used; i++)
> > + v4l2_ctrl_handler_free(&sensor->sds[i].ctrl_handler);
> > +}
> > +
> > +static int smiapp_get_limits(struct smiapp_sensor *sensor, int const
> > *limit, + int n)
>
> unsigned int n ?
Fixed.
> > +{
> > + struct i2c_client *client = v4l2_get_subdevdata(&sensor->src->sd);
> > + int i, val;
>
> unsigned int i;
> u32 val;
>
> ?
Fixed.
> > + int rval;
> > +
> > + for (i = 0; i < n; i++) {
> > + rval = smia_i2c_read_reg(
> > + client, smiapp_reg_limits[limit[i]].addr, &val);
> > + if (rval) {
> > + dev_dbg(&client->dev, "error reading register %4.4x\n",
> > + (u16)smiapp_reg_limits[limit[i]].addr);
>
> What about moving the error message to smia_i2c_read_reg ?
Interestingly there's already one. :-) Removed this one.
> > + return rval;
> > + }
> > + sensor->limits[limit[i]] = val;
> > + dev_dbg(&client->dev, "0x%8.8x \"%s\" = %d, 0x%x\n",
> > + smiapp_reg_limits[limit[i]].addr,
> > + smiapp_reg_limits[limit[i]].what, val, val);
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static int smiapp_get_all_limits(struct smiapp_sensor *sensor)
> > +{
> > + int rval, i;
>
> unsigned int i; ?
Fixed.
> > +
> > + for (i = 0; i < SMIAPP_LIMIT_LAST; i++) {
> > + rval = smiapp_get_limits(sensor, &i, 1);
> > + if (rval < 0)
> > + return rval;
> > + }
> > +
> > + if (sensor->limits[SMIAPP_LIMIT_SCALER_N_MIN] == 0)
> > + smiapp_replace_limit(sensor, SMIAPP_LIMIT_SCALER_N_MIN, 16);
> > +
> > + return 0;
> > +}
> > +
> > +static int smiapp_get_limits_binning(struct smiapp_sensor *sensor)
> > +{
> > + static u32 const limits[] = {
> > + SMIAPP_LIMIT_MIN_FRAME_LENGTH_LINES_BIN,
> > + SMIAPP_LIMIT_MAX_FRAME_LENGTH_LINES_BIN,
> > + SMIAPP_LIMIT_MIN_LINE_LENGTH_PCK_BIN,
> > + SMIAPP_LIMIT_MAX_LINE_LENGTH_PCK_BIN,
> > + SMIAPP_LIMIT_MIN_LINE_BLANKING_PCK_BIN,
> > + SMIAPP_LIMIT_FINE_INTEGRATION_TIME_MIN_BIN,
> > + SMIAPP_LIMIT_FINE_INTEGRATION_TIME_MAX_MARGIN_BIN,
> > + };
> > + static u32 const limits_replace[] = {
> > + SMIAPP_LIMIT_MIN_FRAME_LENGTH_LINES,
> > + SMIAPP_LIMIT_MAX_FRAME_LENGTH_LINES,
> > + SMIAPP_LIMIT_MIN_LINE_LENGTH_PCK,
> > + SMIAPP_LIMIT_MAX_LINE_LENGTH_PCK,
> > + SMIAPP_LIMIT_MIN_LINE_BLANKING_PCK,
> > + SMIAPP_LIMIT_FINE_INTEGRATION_TIME_MIN,
> > + SMIAPP_LIMIT_FINE_INTEGRATION_TIME_MAX_MARGIN,
> > + };
> > +
> > + if (sensor->limits[SMIAPP_LIMIT_BINNING_CAPABILITY] ==
> > + SMIAPP_BINNING_CAPABILITY_NO) {
> > + int i;
>
> unsigned int i; (and in the other functions below) ?
Ditto.
> > +
> > + for (i = 0; i < ARRAY_SIZE(limits); i++)
> > + sensor->limits[limits[i]] =
> > + sensor->limits[limits_replace[i]];
> > +
> > + return 0;
> > + }
> > +
> > + return smiapp_get_limits(sensor, limits, ARRAY_SIZE(limits));
> > +}
> > +
> > +static int smiapp_get_mbus_formats(struct smiapp_sensor *sensor)
> > +{
> > + struct i2c_client *client = v4l2_get_subdevdata(&sensor->src->sd);
> > + unsigned int type, n;
> > + int i, rval, pixel_order;
> > +
> > + rval = smia_i2c_read_reg(
> > + client, SMIAPP_REG_U8_DATA_FORMAT_MODEL_TYPE, &type);
> > + if (rval)
> > + return rval;
> > +
> > + dev_dbg(&client->dev, "data_format_model_type %d\n", type);
> > +
> > + rval = smia_i2c_read_reg(client, SMIAPP_REG_U8_PIXEL_ORDER,
> > + &pixel_order);
> > + if (rval)
> > + return rval;
> > +
> > + if (pixel_order >= ARRAY_SIZE(pixel_order_str)) {
> > + dev_dbg(&client->dev, "bad pixel order %d\n", pixel_order);
> > + return -EINVAL;
> > + }
> > +
> > + dev_dbg(&client->dev, "pixel order %d (%s)\n", pixel_order,
> > + pixel_order_str[pixel_order]);
> > +
> > + switch (type) {
> > + case SMIAPP_DATA_FORMAT_MODEL_TYPE_NORMAL:
> > + n = SMIAPP_DATA_FORMAT_MODEL_TYPE_NORMAL_N;
> > + break;
> > + case SMIAPP_DATA_FORMAT_MODEL_TYPE_EXTENDED:
> > + n = SMIAPP_DATA_FORMAT_MODEL_TYPE_EXTENDED_N;
> > + break;
> > + default:
> > + return -EINVAL;
> > + }
> > +
> > + sensor->default_pixel_order = pixel_order;
> > + sensor->mbus_frame_fmts = 0;
> > +
> > + for (i = 0; i < n; i++) {
> > + int fmt, j;
>
> j can be unsigned as well, this isn't restricted to i :-)
Hmm. I'll soon start using plain "unsigned". :-D
> > +
> > + rval = smia_i2c_read_reg(
> > + client,
> > + SMIAPP_REG_U16_DATA_FORMAT_DESCRIPTOR(i), &fmt);
> > + if (rval)
> > + return rval;
> > +
> > + dev_dbg(&client->dev, "bpp %d, compressed %d\n",
> > + fmt >> 8, (u8)fmt);
> > +
> > + for (j = 0; j < ARRAY_SIZE(smiapp_csi_data_formats); j++) {
> > + const struct smiapp_csi_data_format *f =
> > + &smiapp_csi_data_formats[j];
> > +
> > + if (f->pixel_order != SMIAPP_PIXEL_ORDER_GRBG)
> > + continue;
> > +
> > + if (f->width != fmt >> 8 || f->compressed != (u8)fmt)
> > + continue;
> > +
> > + dev_dbg(&client->dev, "jolly good! %d\n", j);
> > +
> > + sensor->default_mbus_frame_fmts |= 1 << j;
> > + if (!sensor->csi_format) {
> > + sensor->csi_format = f;
> > + sensor->internal_csi_format = f;
> > + }
> > + }
> > + }
> > +
> > + if (!sensor->csi_format) {
> > + dev_err(&client->dev, "no supported mbus code found\n");
> > + return -EINVAL;
> > + }
> > +
> > + smiapp_update_mbus_formats(sensor);
> > +
> > + return 0;
> > +}
> > +
> > +static void smiapp_update_blanking(struct smiapp_sensor *sensor)
> > +{
> > + struct v4l2_ctrl *vblank = sensor->vblank, *hblank = sensor->hblank;
>
> Two lines please.
Fixed.
> > +
> > + vblank->minimum =
> > + max_t(int,
> > + sensor->limits[SMIAPP_LIMIT_MIN_FRAME_BLANKING_LINES],
> > + sensor->limits[SMIAPP_LIMIT_MIN_FRAME_LENGTH_LINES_BIN] -
> > + sensor->pixel_array->crop[SMIAPP_PAD_SOURCE].height);
> > + vblank->maximum =
> > + sensor->limits[SMIAPP_LIMIT_MAX_FRAME_LENGTH_LINES_BIN] -
> > + sensor->pixel_array->crop[SMIAPP_PAD_SOURCE].height;
> > +
> > + vblank->val = clamp_t(int, vblank->val,
> > + vblank->minimum, vblank->maximum);
> > + vblank->default_value = vblank->minimum;
> > + vblank->val = vblank->val;
> > + vblank->cur.val = vblank->val;
> > +
> > + hblank->minimum =
> > + max_t(int,
> > + sensor->limits[SMIAPP_LIMIT_MIN_LINE_LENGTH_PCK_BIN] -
> > + sensor->pixel_array->crop[SMIAPP_PAD_SOURCE].width,
> > + sensor->limits[SMIAPP_LIMIT_MIN_LINE_BLANKING_PCK_BIN]);
> > + hblank->maximum =
> > + sensor->limits[SMIAPP_LIMIT_MAX_LINE_LENGTH_PCK_BIN] -
> > + sensor->pixel_array->crop[SMIAPP_PAD_SOURCE].width;
> > +
> > + hblank->val = clamp_t(int, hblank->val,
> > + hblank->minimum, hblank->maximum);
> > + hblank->default_value = hblank->minimum;
> > + hblank->val = hblank->val;
> > + hblank->cur.val = hblank->val;
> > +
> > + __smiapp_update_exposure_limits(sensor);
> > +}
> > +
> > +static int smiapp_update_mode(struct smiapp_sensor *sensor)
> > +{
>
> This function isn't protected by the sensor mutex when called from s_power,
> but it changes controls. The other call paths seem OK, but you might want to
> double-check them.
It's actually not an issue. When s_power is being called there are no other
users and the power_lock serialises it.
I implemented it this way since the control setup acquires the same lock
that I would have to hold while powering up. The power_lock fixes this
issue.
I cleaned this up so that I won't take sensor->mutex at all anymore.
> > + struct i2c_client *client = v4l2_get_subdevdata(&sensor->src->sd);
> > + int binning_mode;
> > + int rval;
> > +
> > + dev_dbg(&client->dev, "frame size: %dx%d\n",
> > + sensor->src->crop[SMIAPP_PAD_SOURCE].width,
> > + sensor->src->crop[SMIAPP_PAD_SOURCE].height);
> > + dev_dbg(&client->dev, "csi format width: %d\n",
> > + sensor->csi_format->width);
> > +
> > + /* Binning has to be set up here; it affects limits */
> > + if (sensor->binning_horizontal == 1 &&
> > + sensor->binning_vertical == 1) {
> > + binning_mode = 0;
> > + } else {
> > + u8 binning_type =
> > + (sensor->binning_horizontal << 4)
> > + | sensor->binning_vertical;
> > +
> > + rval = smia_i2c_write_reg(
> > + client, SMIAPP_REG_U8_BINNING_TYPE, binning_type);
> > + if (rval < 0)
> > + return rval;
> > +
> > + binning_mode = 1;
> > + }
> > + rval = smia_i2c_write_reg(
> > + client, SMIAPP_REG_U8_BINNING_MODE, binning_mode);
> > + if (rval < 0)
> > + return rval;
> > +
> > + /* Get updated limits due to binning */
> > + rval = smiapp_get_limits_binning(sensor);
> > + if (rval < 0)
> > + return rval;
> > +
> > + rval = smiapp_pll_update(sensor);
> > + if (rval < 0)
> > + return rval;
> > +
> > + /* Output from pixel array, including blanking */
> > + smiapp_update_blanking(sensor);
> > +
> > + dev_dbg(&client->dev, "vblank\t\t%d\n", sensor->vblank->val);
> > + dev_dbg(&client->dev, "hblank\t\t%d\n", sensor->hblank->val);
> > +
> > + dev_dbg(&client->dev, "real timeperframe\t100/%d\n",
> > + sensor->pll.vt_pix_clk_freq_hz /
> > + ((sensor->pixel_array->crop[SMIAPP_PAD_SOURCE].width
> > + + sensor->hblank->val) *
> > + (sensor->pixel_array->crop[SMIAPP_PAD_SOURCE].height
> > + + sensor->vblank->val) / 100));
> > +
> > + return 0;
> > +}
> > +
> > +/*
> > + *
> > + * SMIA++ NVM handling
> > + *
> > + */
> > +static int smiapp_read_nvm(struct smiapp_sensor *sensor,
> > + unsigned char *nvm)
> > +{
> > + struct i2c_client *client = v4l2_get_subdevdata(&sensor->src->sd);
> > + u32 i, s, p, np, v;
> > + int rval;
> > +
> > + np = sensor->nvm_size / SMIAPP_NVM_PAGE_SIZE;
>
> DIV_ROUND_UP() ? Or is sensor->nvm_size guaranteed to be a multiple of
> SMIAPP_NVM_PAGE_SIZE ?
I'd think so. I'm not sure if reading partial pages is supported either.
> > + for (p = 0; p < np; p++) {
> > + rval = smia_i2c_write_reg(
> > + client,
> > + SMIAPP_REG_U8_DATA_TRANSFER_IF_1_PAGE_SELECT, p);
> > + if (rval)
> > + goto out;
> > +
> > + rval = smia_i2c_write_reg(client,
> > + SMIAPP_REG_U8_DATA_TRANSFER_IF_1_CTRL,
> > + SMIAPP_DATA_TRANSFER_IF_1_CTRL_EN |
> > + SMIAPP_DATA_TRANSFER_IF_1_CTRL_RD_EN);
> > + if (rval)
> > + goto out;
> > +
> > + i = 1000;
> > + do {
> > + rval = smia_i2c_read_reg(client,
> > + SMIAPP_REG_U8_DATA_TRANSFER_IF_1_STATUS, &s);
> > +
> > + if (rval)
> > + goto out;
> > +
> > + if (s & SMIAPP_DATA_TRANSFER_IF_1_STATUS_RD_READY)
> > + break;
> > +
> > + if (--i == 0)
> > + goto out;
> > +
> > + } while (1);
>
> I'd use a for loop on i. BTW, isn't 1000 a bit high ?
Changed to for loop. Well, I don't know what's the typical time one has to
wait but at least 1000 should be enough. I don't expect this to happen but
if it does it's a bad thing; even more than 1000 might make sense.
> > +
> > + for (i = 0; i < SMIAPP_NVM_PAGE_SIZE; i++) {
> > + rval = smia_i2c_read_reg(client,
> > + SMIAPP_REG_U8_DATA_TRANSFER_IF_1_DATA_0 + i,
> > + &v);
> > + if (rval)
> > + goto out;
> > +
> > + *nvm++ = v;
> > + }
> > + }
> > +
> > +out:
> > + rval |= smia_i2c_write_reg(client,
> > + SMIAPP_REG_U8_DATA_TRANSFER_IF_1_CTRL, 0);
>
> Could this be optimized away by the compiler, as the return value of this
> function is only checked against 0 ?
Could be. But the logical or makes no sense at all. Removed it.
> > + return rval;
> > +}
> > +
> > +/*
> > + *
> > + * SMIA++ CCI address control
> > + *
> > + */
> > +static int smiapp_change_cci_addr(struct smiapp_sensor *sensor)
> > +{
> > + struct i2c_client *client = v4l2_get_subdevdata(&sensor->src->sd);
> > + int rval;
> > + u32 val;
> > +
> > + client->addr = sensor->platform_data->i2c_addr_dfl;
> > +
> > + rval = smia_i2c_write_reg(client,
> > + SMIAPP_REG_U8_CCI_ADDRESS_CONTROL,
> > + sensor->platform_data->i2c_addr_alt << 1);
> > + if (rval) {
> > + client->addr = sensor->platform_data->i2c_addr_alt;
>
> Why do you set the client address to the alternate one if the call failed ?
I think it's called a "bug". :-P
> > + return rval;
> > + }
> > +
> > + client->addr = sensor->platform_data->i2c_addr_alt;
> > +
> > + /* verify addr change went ok */
> > + rval = smia_i2c_read_reg(client,
> > + SMIAPP_REG_U8_CCI_ADDRESS_CONTROL, &val);
> > + if (rval)
> > + return rval;
> > +
> > + if (val != sensor->platform_data->i2c_addr_alt << 1)
> > + return -ENODEV;
> > +
> > + return 0;
> > +}
> > +
> > +/*
> > + *
> > + * SMIA++ Mode Control
> > + *
> > + */
> > +static int smiapp_setup_flash_strobe(struct smiapp_sensor *sensor)
> > +{
> > + struct i2c_client *client = v4l2_get_subdevdata(&sensor->src->sd);
> > + struct smiapp_flash_strobe_parms *strobe_setup;
> > + unsigned int ext_freq = sensor->platform_data->ext_clk;
> > + int rval;
> > + u32 tmp;
> > + u32 strobe_adjustment;
> > + u32 strobe_width_high_rs;
> > +
> > + strobe_setup = sensor->platform_data->strobe_setup;
> > +
> > + /*
> > + * How to calculate registers related to strobe length. Please
> > + * do not change, or if you do at least know what you're
> > + * doing. :-)
>
> You could reindent the text here up to 80 columns, that would shorten the
> comment a bit.
This is how my XEmacsitor indents it by default. And for good reason: it's
very readable that way --- filling the full 80 columns with text makes it
tougher to read.
> > + *
> > + * Sakari Ailus <sakari.ailus@xxxxxxxxxxxxxxxxxxxxxxxxxx> 2010-10-25
> > + *
> > + * flash_strobe_length [us] / 10^6 = (tFlash_strobe_width_ctrl
> > + * / EXTCLK freq [Hz]) * flash_strobe_adjustment
> > + *
> > + * tFlash_strobe_width_ctrl E N, [1 - 0xffff]
> > + * flash_strobe_adjustment E N, [1 - 0xff]
> > + *
> > + * The formula above is written as below to keep it on one
> > + * line:
> > + *
> > + * l / 10^6 = w / e * a
> > + *
> > + * Let's mark w * a by x:
> > + *
> > + * x = w * a
> > + *
> > + * Thus, we get:
> > + *
> > + * x = l * e / 10^6
> > + *
> > + * The strobe width must be at least as long as requested,
> > + * thus rounding upwards is needed.
> > + *
> > + * x = (l * e + 10^6 - 1) / 10^6
> > + * -----------------------------
> > + *
> > + * Maximum possible accuracy is wanted at all times. Thus keep
> > + * a as small as possible.
> > + *
> > + * Calculate a, assuming maximum w, with rounding upwards:
> > + *
> > + * a = (x + (2^16 - 1) - 1) / (2^16 - 1)
> > + * -------------------------------------
> > + *
> > + * Thus, we also get w, with that a, with rounding upwards:
> > + *
> > + * w = (x + a - 1) / a
> > + * -------------------
> > + *
> > + * To get limits:
> > + *
> > + * x E [1, (2^16 - 1) * (2^8 - 1)]
> > + *
> > + * Substituting maximum x to the original formula (with rounding),
> > + * the maximum l is thus
> > + *
> > + * (2^16 - 1) * (2^8 - 1) * 10^6 = l * e + 10^6 - 1
> > + *
> > + * l = (10^6 * (2^16 - 1) * (2^8 - 1) - 10^6 + 1) / e
> > + * --------------------------------------------------
> > + *
> > + * flash_strobe_length must be clamped between 1 and
> > + * (10^6 * (2^16 - 1) * (2^8 - 1) - 10^6 + 1) / EXTCLK freq.
> > + *
> > + * Then,
> > + *
> > + * flash_strobe_adjustment = ((flash_strobe_length *
> > + * EXTCLK freq + 10^6 - 1) / 10^6 + (2^16 - 1) - 1) / (2^16 - 1)
> > + *
> > + * tFlash_strobe_width_ctrl = ((flash_strobe_length *
> > + * EXTCLK freq + 10^6 - 1) / 10^6 +
> > + * flash_strobe_adjustment - 1) / flash_strobe_adjustment
> > + */
> > + tmp = div_u64(1000000ULL * ((1 << 16) - 1) * ((1 << 8) - 1) -
> > + 1000000 + 1, ext_freq);
> > + strobe_setup->strobe_width_high_us =
> > + clamp_t(u32, strobe_setup->strobe_width_high_us, 1, tmp);
> > +
> > + tmp = div_u64(((u64)strobe_setup->strobe_width_high_us * (u64)ext_freq +
> > + 1000000 - 1), 1000000ULL);
> > + strobe_adjustment = (tmp + (1 << 16) - 1 - 1) / ((1 << 16) - 1);
> > + strobe_width_high_rs = (tmp + strobe_adjustment - 1) /
> > + strobe_adjustment;
> > +
> > + rval = smia_i2c_write_reg(client,
> > + SMIAPP_REG_U8_FLASH_MODE_RS,
> > + strobe_setup->mode);
> > + if (rval < 0)
> > + goto out;
> > +
> > + rval = smia_i2c_write_reg(client,
> > + SMIAPP_REG_U8_FLASH_STROBE_ADJUSTMENT,
> > + strobe_adjustment);
> > + if (rval < 0)
> > + goto out;
> > +
> > + rval = smia_i2c_write_reg(
> > + client, SMIAPP_REG_U16_TFLASH_STROBE_WIDTH_HIGH_RS_CTRL,
> > + strobe_width_high_rs);
> > + if (rval < 0)
> > + goto out;
> > +
> > + rval = smia_i2c_write_reg(client,
> > + SMIAPP_REG_U16_TFLASH_STROBE_DELAY_RS_CTRL,
> > + strobe_setup->strobe_delay);
> > + if (rval < 0)
> > + goto out;
> > +
> > + rval = smia_i2c_write_reg(client,
> > + SMIAPP_REG_U16_FLASH_STROBE_START_POINT,
> > + strobe_setup->stobe_start_point);
> > + if (rval < 0)
> > + goto out;
> > +
> > + rval = smia_i2c_write_reg(client,
> > + SMIAPP_REG_U8_FLASH_TRIGGER_RS,
> > + strobe_setup->trigger);
> > +
> > +out:
> > + sensor->platform_data->strobe_setup->trigger = 0;
> > +
> > + return rval;
> > +}
> > +
> > +/*
> > ---------------------------------------------------------------------------
> > -- + * Power management
> > + */
> > +
> > +static int smiapp_power_on(struct smiapp_sensor *sensor)
> > +{
> > + struct i2c_client *client = v4l2_get_subdevdata(&sensor->src->sd);
> > + int sleep;
> > + int rval;
> > +
> > + rval = regulator_enable(sensor->vana);
> > + if (rval) {
> > + dev_err(&client->dev, "failed to enable vana regulator\n");
> > + return rval;
> > + }
> > + usleep_range(1000, 1000);
>
> That's a very tight range :-)
We're waiting for something that's got to do with user interaction. Thus no
extra delays should be added even if that generates more interrupts.
> > +
> > + rval = sensor->platform_data->set_xclk(&sensor->src->sd,
> > + sensor->platform_data->ext_clk);
> > + if (rval < 0) {
> > + dev_dbg(&client->dev, "failed to set xclk\n");
> > + goto out_xclk_fail;
> > + }
> > + usleep_range(1000, 1000);
> > +
> > + if (sensor->platform_data->xshutdown != SMIAPP_NO_XSHUTDOWN)
> > + gpio_set_value(sensor->platform_data->xshutdown, 1);
> > +
> > + sleep = SMIAPP_RESET_DELAY(sensor->platform_data->ext_clk);
> > + usleep_range(sleep, sleep);
> > +
> > + /*
> > + * Failures to respond to the address change command have been noticed.
> > + * Those failures seem to be caused by the sensor requiring a longer
> > + * boot time than advertised. An additional 10ms delay seems to work
> > + * around the issue, but the SMIA++ I2C write retry hack makes the delay
> > + * unnecessary. The failures need to be investigated to find a proper
> > + * fix, and a delay will likely need to be added here if the I2C write
> > + * retry hack is reverted before the root cause of the boot time issue
> > + * is found.
> > + */
> > +
> > + if (sensor->platform_data->i2c_addr_alt) {
> > + rval = smiapp_change_cci_addr(sensor);
> > + if (rval) {
> > + dev_err(&client->dev, "cci address change error\n");
> > + goto out_cci_addr_fail;
> > + }
> > + }
> > +
> > + rval = smia_i2c_write_reg(client, SMIAPP_REG_U8_SOFTWARE_RESET,
> > + SMIAPP_SOFTWARE_RESET);
> > + if (rval < 0) {
> > + dev_err(&client->dev, "software reset failed\n");
> > + goto out_cci_addr_fail;
> > + }
> > +
> > + if (sensor->platform_data->i2c_addr_alt) {
> > + rval = smiapp_change_cci_addr(sensor);
> > + if (rval) {
> > + dev_err(&client->dev, "cci address change error\n");
> > + goto out_cci_addr_fail;
> > + }
> > + }
> > +
> > + rval = smia_i2c_write_reg(client,
> > + SMIAPP_REG_U16_COMPRESSION_MODE,
> > + SMIAPP_COMPRESSION_MODE_SIMPLE_PREDICTOR);
> > + if (rval) {
> > + dev_err(&client->dev, "compression mode set failed\n");
> > + goto out_cci_addr_fail;
> > + }
> > +
> > + rval = smia_i2c_write_reg(
> > + client, SMIAPP_REG_U16_EXTCLK_FREQUENCY_MHZ,
> > + sensor->platform_data->ext_clk / (1000000 / (1 << 8)));
> > + if (rval) {
> > + dev_err(&client->dev, "extclk frequency set failed\n");
> > + goto out_cci_addr_fail;
> > + }
> > +
> > + rval = smia_i2c_write_reg(
> > + client, SMIAPP_REG_U8_CSI_LANE_MODE,
> > + sensor->platform_data->lanes - 1);
> > + if (rval) {
> > + dev_err(&client->dev, "csi lane mode set failed\n");
> > + goto out_cci_addr_fail;
> > + }
> > +
> > + rval = smia_i2c_write_reg(client, SMIAPP_REG_U8_FAST_STANDBY_CTRL,
> > + SMIAPP_FAST_STANDBY_CTRL_IMMEDIATE);
> > + if (rval) {
> > + dev_err(&client->dev, "fast standby set failed\n");
> > + goto out_cci_addr_fail;
> > + }
> > +
> > + /* DPHY control done by sensor based on requested link rate */
> > + rval = smia_i2c_write_reg(
> > + client, SMIAPP_REG_U8_DPHY_CTRL, SMIAPP_DPHY_CTRL_UI);
> > + if (rval < 0) {
> > + dev_err(&client->dev, "set dphy_ctrl_ui failed\n");
> > + goto out_cci_addr_fail;
> > + }
> > +
> > + rval = smia_i2c_write_reg(client, SMIAPP_REG_U8_CSI_SIGNALLING_MODE,
> > + sensor->platform_data->csi_signalling_mode);
> > + if (rval) {
> > + dev_err(&client->dev, "csi signalling mode set failed\n");
> > + goto out_cci_addr_fail;
> > + }
> > +
> > + /* DPHY control done by sensor based on requested link rate */
> > + rval = smia_i2c_write_reg(
> > + client, SMIAPP_REG_U8_DPHY_CTRL, SMIAPP_DPHY_CTRL_UI);
> > + if (rval < 0)
> > + return rval;
>
> Is there a need to the SMIAPP_REG_U8_DPHY_CTRL register twice to the same
> value ?
I think I intended to move the write, not copy the lines. Removed the first
one.
> > +
> > + rval = smiapp_call_quirk(sensor, post_poweron);
> > + if (rval) {
> > + dev_err(&client->dev, "post_poweron quirks failed\n");
> > + goto out_cci_addr_fail;
> > + }
> > +
> > + return 0;
> > +
> > +out_cci_addr_fail:
> > + if (sensor->platform_data->xshutdown != SMIAPP_NO_XSHUTDOWN)
> > + gpio_set_value(sensor->platform_data->xshutdown, 0);
> > + sensor->platform_data->set_xclk(&sensor->src->sd, 0);
> > +
> > +out_xclk_fail:
> > + regulator_disable(sensor->vana);
> > + return rval;
> > +}
> > +
> > +static void smiapp_power_off(struct smiapp_sensor *sensor)
> > +{
> > + struct i2c_client *client = v4l2_get_subdevdata(&sensor->src->sd);
> > +
> > + /*
> > + * Currently power/clock to lens are enable/disabled separately
> > + * but they are essentially the same signals. So if the sensor is
> > + * powered off while the lens is powered on the sensor does not
> > + * really see a power off and next time the cci address change
> > + * will fail. So do a soft reset explicitly here.
> > + */
> > + if (sensor->platform_data->i2c_addr_alt)
> > + smia_i2c_write_reg(client,
> > + SMIAPP_REG_U8_SOFTWARE_RESET,
> > + SMIAPP_SOFTWARE_RESET);
> > +
> > + if (sensor->platform_data->xshutdown != SMIAPP_NO_XSHUTDOWN)
> > + gpio_set_value(sensor->platform_data->xshutdown, 0);
> > + sensor->platform_data->set_xclk(&sensor->src->sd, 0);
> > + usleep_range(5000, 5000);
> > + regulator_disable(sensor->vana);
> > + sensor->streaming = 0;
> > +}
> > +
> > +static int smiapp_set_power(struct v4l2_subdev *subdev, int on)
> > +{
> > + struct smiapp_sensor *sensor = to_smiapp_sensor(subdev);
> > + int ret;
> > +
> > + /* If the power count is modified from 0 to != 0 or from != 0 to 0,
> > + * update the power state.
> > + */
> > + if (sensor->power_count == !on) {
> > + if (on) {
> > + ret = smiapp_power_on(sensor);
> > + if (ret < 0)
> > + return ret;
> > + ret = smiapp_update_mode(sensor);
> > + if (ret < 0)
> > + return ret;
> > + } else {
> > + smiapp_power_off(sensor);
> > + }
> > + }
> > +
> > + /* Update the power count. */
> > + sensor->power_count += on ? 1 : -1;
> > + WARN_ON(sensor->power_count < 0);
> > +
> > + return 0;
> > +}
> > +
> > +/*
> > ---------------------------------------------------------------------------
> > -- + * Video stream management
> > + */
> > +
> > +static int smiapp_start_streaming(struct smiapp_sensor *sensor)
> > +{
> > + struct i2c_client *client = v4l2_get_subdevdata(&sensor->src->sd);
> > + int rval;
> > +
> > + mutex_lock(&sensor->mutex);
> > +
> > + rval = smia_i2c_write_reg(
> > + client, SMIAPP_REG_U16_CSI_DATA_FORMAT,
> > + (sensor->csi_format->width << 8) |
> > + sensor->csi_format->compressed);
> > + if (rval)
> > + goto out;
> > +
> > + rval = smiapp_pll_configure(sensor);
> > + if (rval)
> > + goto out;
> > +
> > + /* Analog crop start coordinates */
> > + rval = smia_i2c_write_reg(
> > + client, SMIAPP_REG_U16_X_ADDR_START,
> > + sensor->pixel_array->crop[SMIAPP_PAD_SOURCE].left);
> > + if (rval < 0)
> > + goto out;
> > +
> > + rval = smia_i2c_write_reg(
> > + client, SMIAPP_REG_U16_Y_ADDR_START,
> > + sensor->pixel_array->crop[SMIAPP_PAD_SOURCE].top);
> > + if (rval < 0)
> > + goto out;
> > +
> > + /* Analog crop end coordinates */
> > + rval = smia_i2c_write_reg(
> > + client, SMIAPP_REG_U16_X_ADDR_END,
> > + sensor->pixel_array->crop[SMIAPP_PAD_SOURCE].left
> > + + sensor->pixel_array->crop[SMIAPP_PAD_SOURCE].width - 1);
> > + if (rval < 0)
> > + goto out;
> > +
> > + rval = smia_i2c_write_reg(
> > + client, SMIAPP_REG_U16_Y_ADDR_END,
> > + sensor->pixel_array->crop[SMIAPP_PAD_SOURCE].top
> > + + sensor->pixel_array->crop[SMIAPP_PAD_SOURCE].height - 1);
> > + if (rval < 0)
> > + goto out;
> > +
> > + /*
> > + * Output from pixel array, including blanking, is set using
> > + * controls below. No need to set here.
> > + */
> > +
> > + /* Digital crop */
> > + if (sensor->limits[SMIAPP_LIMIT_DIGITAL_CROP_CAPABILITY]
> > + == SMIAPP_DIGITAL_CROP_CAPABILITY_INPUT_CROP) {
> > + rval = smia_i2c_write_reg(
> > + client, SMIAPP_REG_U16_DIGITAL_CROP_X_OFFSET,
> > + sensor->scaler->crop[SMIAPP_PAD_SINK].left);
> > + if (rval < 0)
> > + goto out;
> > +
> > + rval = smia_i2c_write_reg(
> > + client, SMIAPP_REG_U16_DIGITAL_CROP_Y_OFFSET,
> > + sensor->scaler->crop[SMIAPP_PAD_SINK].top);
> > + if (rval < 0)
> > + goto out;
> > +
> > + rval = smia_i2c_write_reg(
> > + client, SMIAPP_REG_U16_DIGITAL_CROP_IMAGE_WIDTH,
> > + sensor->scaler->crop[SMIAPP_PAD_SINK].width);
> > + if (rval < 0)
> > + goto out;
> > +
> > + rval = smia_i2c_write_reg(
> > + client, SMIAPP_REG_U16_DIGITAL_CROP_IMAGE_HEIGHT,
> > + sensor->scaler->crop[SMIAPP_PAD_SINK].height);
> > + if (rval < 0)
> > + goto out;
> > + }
> > +
> > + /* Scaling */
> > + if (sensor->limits[SMIAPP_LIMIT_SCALING_CAPABILITY]
> > + != SMIAPP_SCALING_CAPABILITY_NONE) {
> > + rval = smia_i2c_write_reg(
> > + client, SMIAPP_REG_U16_SCALING_MODE,
> > + sensor->scaling_mode);
> > + if (rval < 0)
> > + goto out;
> > +
> > + if (sensor->scale_m
> > + != sensor->limits[SMIAPP_LIMIT_SCALER_N_MIN]) {
> > + rval = smia_i2c_write_reg(
> > + client, SMIAPP_REG_U16_SCALE_M,
> > + sensor->scale_m);
> > + if (rval < 0)
> > + goto out;
> > + }
>
> I could be wrong, but it seems to me like the scaling M factor won't be
> updated
> properly if you first enable/disable streaming with a non-default M factor,
> reconfigure the sensor to use the default value, and then start streaming
> again.
Excellent point. Fixed.
> > + }
> > +
> > + /* Output size from sensor */
> > + rval = smia_i2c_write_reg(
> > + client, SMIAPP_REG_U16_X_OUTPUT_SIZE,
> > + sensor->src->crop[SMIAPP_PAD_SOURCE].width);
> > + if (rval < 0)
> > + goto out;
> > + rval = smia_i2c_write_reg(
> > + client, SMIAPP_REG_U16_Y_OUTPUT_SIZE,
> > + sensor->src->crop[SMIAPP_PAD_SOURCE].height);
> > + if (rval < 0)
> > + goto out;
> > +
> > + if ((sensor->flash_capability &
> > + (SMIAPP_FLASH_MODE_CAPABILITY_SINGLE_STROBE |
> > + SMIAPP_FLASH_MODE_CAPABILITY_MULTIPLE_STROBE)) &&
> > + sensor->platform_data->strobe_setup != NULL &&
> > + sensor->platform_data->strobe_setup->trigger != 0) {
> > + rval = smiapp_setup_flash_strobe(sensor);
> > + if (rval)
> > + goto out;
> > + }
> > +
> > + rval = smiapp_call_quirk(sensor, pre_streamon);
> > + if (rval) {
> > + dev_err(&client->dev, "pre_streamon quirks failed\n");
> > + goto out;
> > + }
> > +
> > + rval = smia_i2c_write_reg(client, SMIAPP_REG_U8_MODE_SELECT,
> > + SMIAPP_MODE_SELECT_STREAMING);
> > +
> > +out:
> > + mutex_unlock(&sensor->mutex);
> > +
> > + return rval;
> > +}
> > +
> > +static int smiapp_stop_streaming(struct smiapp_sensor *sensor)
> > +{
> > + struct i2c_client *client = v4l2_get_subdevdata(&sensor->src->sd);
> > + int rval;
> > +
> > + mutex_lock(&sensor->mutex);
> > + rval = smia_i2c_write_reg(client, SMIAPP_REG_U8_MODE_SELECT,
> > + SMIAPP_MODE_SELECT_SOFTWARE_STANDBY);
> > + if (rval)
> > + goto out;
> > +
> > + rval = smiapp_call_quirk(sensor, post_streamoff);
> > + if (rval)
> > + dev_err(&client->dev, "post_streamoff quirks failed\n");
> > +
> > +out:
> > + mutex_unlock(&sensor->mutex);
> > + return rval;
> > +}
> > +
> > +/*
> > ---------------------------------------------------------------------------
> > -- + * V4L2 subdev video operations
> > + */
> > +
> > +static int smiapp_set_stream(struct v4l2_subdev *subdev, int enable)
> > +{
> > + struct smiapp_sensor *sensor = to_smiapp_sensor(subdev);
> > + int rval;
> > +
> > + if (sensor->streaming == enable)
> > + return 0;
> > +
> > + if (enable) {
> > + sensor->streaming = 1;
> > + rval = smiapp_start_streaming(sensor);
> > + if (rval < 0)
> > + sensor->streaming = 0;
>
> Is there a reason not to just set sensor->streaming after calling
> smiapp_start_streaming() ?
Yes. There are controls which return -EBUSY in those cases.
Ideally I guess I should grab them, but I'd like to make grabbing counted
instead of being boolean. I think I'll prepare additional patches for this
later on.
> > + } else {
> > + rval = smiapp_stop_streaming(sensor);
> > + sensor->streaming = 0;
> > + }
> > +
> > + return rval;
> > +}
> > +
> > +static int smiapp_enum_mbus_code(struct v4l2_subdev *subdev,
> > + struct v4l2_subdev_fh *fh,
> > + struct v4l2_subdev_mbus_code_enum *code)
> > +{
> > + struct i2c_client *client = v4l2_get_subdevdata(subdev);
> > + struct smiapp_sensor *sensor = to_smiapp_sensor(subdev);
> > + int i, idx = -1;
> > + int rval = -EINVAL;
> > +
> > + mutex_lock(&sensor->mutex);
> > +
> > + dev_err(&client->dev, "subdev %s, pad %d, index %d\n",
> > + subdev->name, code->pad, code->index);
> > +
> > + if (subdev != &sensor->src->sd
> > + || code->pad != SMIAPP_PAD_SOURCE) {
> > + if (code->index)
> > + goto out;
> > +
> > + code->code = sensor->internal_csi_format->code;
> > + rval = 0;
> > + goto out;
> > + }
> > +
> > + for (i = 0; i < ARRAY_SIZE(smiapp_csi_data_formats); i++) {
> > + if (sensor->mbus_frame_fmts & (1 << i))
> > + idx++;
> > +
> > + if (idx == code->index) {
> > + code->code = smiapp_csi_data_formats[i].code;
> > + dev_err(&client->dev, "found index %d, i %d, code %x\n",
> > + code->index, i, code->code);
> > + rval = 0;
> > + goto out;
>
> break; would do :-)
Fixed.
> > + }
> > + }
> > +
> > +out:
> > + mutex_unlock(&sensor->mutex);
> > +
> > + return rval;
> > +}
> > +
> > +static int __smiapp_get_format(struct v4l2_subdev *subdev,
> > + struct v4l2_subdev_fh *fh,
> > + struct v4l2_subdev_format *fmt)
> > +{
> > + struct smiapp_subdev *ssd = to_smiapp_subdev(subdev);
> > + struct smiapp_sensor *sensor = to_smiapp_sensor(subdev);
> > +
> > + if (fmt->which == V4L2_SUBDEV_FORMAT_TRY) {
> > + fmt->format = *v4l2_subdev_get_try_format(fh, fmt->pad);
> > + } else {
> > + struct v4l2_rect *r;
> > +
> > + if (fmt->pad == SMIAPP_PAD_SOURCE)
> > + r = &ssd->crop[SMIAPP_PAD_SOURCE];
> > + else
> > + r = &ssd->sink_fmt;
> > +
> > + fmt->format.width = r->width;
> > + fmt->format.height = r->height;
> > + if (subdev == &sensor->src->sd
> > + && fmt->pad == SMIAPP_PAD_SOURCE)
> > + fmt->format.code = sensor->csi_format->code;
> > + else
> > + fmt->format.code = sensor->internal_csi_format->code;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static int smiapp_get_format(struct v4l2_subdev *subdev,
> > + struct v4l2_subdev_fh *fh,
> > + struct v4l2_subdev_format *fmt)
> > +{
> > + struct smiapp_sensor *sensor = to_smiapp_sensor(subdev);
> > + int rval;
> > +
> > + mutex_lock(&sensor->mutex);
> > + rval = __smiapp_get_format(subdev, fh, fmt);
> > + mutex_unlock(&sensor->mutex);
> > +
> > + return rval;
> > +}
> > +
> > +static void smiapp_get_crop_compose(struct v4l2_subdev *subdev,
> > + struct v4l2_subdev_fh *fh,
> > + struct v4l2_rect **crops,
> > + struct v4l2_rect **comps, int which)
> > +{
> > + struct smiapp_subdev *ssd = to_smiapp_subdev(subdev);
> > + int i;
> > +
> > + if (which == V4L2_SUBDEV_FORMAT_ACTIVE) {
> > + for (i = 0; i < subdev->entity.num_pads; i++)
> > + if (crops)
> > + crops[i] = &ssd->crop[i];
>
> You could move the if outside the for.
Good point. The code wasn't originally written like that. :-)
> > + if (comps)
> > + *comps = &ssd->compose;
> > + } else {
> > + for (i = 0; i < subdev->entity.num_pads; i++)
> > + if (crops) {
> > + crops[i] = v4l2_subdev_get_try_crop(fh, i);
> > + BUG_ON(!crops[i]);
> > + }
>
> Same here.
Ditto.
> > + if (comps) {
> > + *comps = v4l2_subdev_get_try_compose(fh,
> > + SMIAPP_PAD_SINK);
> > + BUG_ON(!*comps);
> > + }
> > + }
> > +}
> > +
> > +/* Changes require propagation only on sink pad. */
> > +static void smiapp_propagate(struct v4l2_subdev *subdev,
> > + struct v4l2_subdev_fh *fh, int which,
> > + int target)
> > +{
> > + struct smiapp_sensor *sensor = to_smiapp_sensor(subdev);
> > + struct smiapp_subdev *ssd = to_smiapp_subdev(subdev);
> > + struct v4l2_rect *comp, *crops[SMIAPP_PADS];
> > +
> > + smiapp_get_crop_compose(subdev, fh, crops, &comp, which);
> > +
> > + switch (target) {
> > + case V4L2_SUBDEV_SEL_TGT_CROP_ACTIVE:
> > + comp->width = crops[SMIAPP_PAD_SINK]->width;
> > + comp->height = crops[SMIAPP_PAD_SINK]->height;
> > + if (which == V4L2_SUBDEV_FORMAT_ACTIVE) {
> > + if (ssd == sensor->scaler) {
> > + sensor->scale_m =
> > + sensor->limits[
> > + SMIAPP_LIMIT_SCALER_N_MIN];
> > + sensor->scaling_mode =
> > + SMIAPP_SCALING_MODE_NONE;
> > + } else if (ssd == sensor->binner) {
> > + sensor->binning_horizontal = 1;
> > + sensor->binning_vertical = 1;
> > + }
> > + }
> > + /* Fall through */
> > + case V4L2_SUBDEV_SEL_TGT_COMPOSE_ACTIVE:
> > + *crops[SMIAPP_PAD_SOURCE] = *comp;
> > + break;
> > + default:
> > + BUG();
> > + }
> > +}
> > +
> > +static int smiapp_set_format(struct v4l2_subdev *subdev,
> > + struct v4l2_subdev_fh *fh,
> > + struct v4l2_subdev_format *fmt)
> > +{
> > + struct smiapp_sensor *sensor = to_smiapp_sensor(subdev);
> > + struct smiapp_subdev *ssd = to_smiapp_subdev(subdev);
> > + struct v4l2_rect *crops[SMIAPP_PADS];
> > + int i = 0;
>
> There's no need to initialize i to 0.
Ack.
> > +
> > + mutex_lock(&sensor->mutex);
> > +
> > + smiapp_get_crop_compose(subdev, fh, crops, NULL, fmt->which);
> > +
> > + if (subdev == &sensor->src->sd && fmt->pad == SMIAPP_PAD_SOURCE) {
> > + for (i = 0; i < ARRAY_SIZE(smiapp_csi_data_formats); i++) {
> > + if (sensor->mbus_frame_fmts & (1 << i) &&
> > + smiapp_csi_data_formats[i].code
> > + == fmt->format.code) {
> > + if (fmt->which == V4L2_SUBDEV_FORMAT_ACTIVE)
> > + sensor->csi_format =
> > + &smiapp_csi_data_formats[i];
> > + break;
> > + }
> > + }
> > + }
> > +
> > + if (fmt->pad == SMIAPP_PAD_SOURCE) {
> > + int rval;
> > +
> > + rval = __smiapp_get_format(subdev, fh, fmt);
> > +
> > + mutex_unlock(&sensor->mutex);
> > + return rval;
> > + }
> > +
> > + fmt->format.code = sensor->csi_format->code;
>
> sensor->csi_format is the active format. You seem to always return the active
> format code. That will break setting the try format.
Fixed.
> > +
> > + fmt->format.width &= ~1;
> > + fmt->format.height &= ~1;
> > +
> > + if (fmt->format.width < sensor->limits[SMIAPP_LIMIT_MIN_X_OUTPUT_SIZE])
> > + fmt->format.width =
> > + sensor->limits[SMIAPP_LIMIT_MIN_X_OUTPUT_SIZE];
> > + if (fmt->format.height
> > + < sensor->limits[SMIAPP_LIMIT_MIN_Y_OUTPUT_SIZE])
> > + fmt->format.height =
> > + sensor->limits[SMIAPP_LIMIT_MIN_Y_OUTPUT_SIZE];
>
> Isn't there a maximum size as well ?
Hmm. I think there's something missing from here. :-)
> > +
> > + crops[SMIAPP_PAD_SINK]->left = crops[SMIAPP_PAD_SINK]->top = 0;
>
> One assignment per line please.
Ok...
> > + crops[SMIAPP_PAD_SINK]->width = fmt->format.width;
> > + crops[SMIAPP_PAD_SINK]->height = fmt->format.height;
> > + if (fmt->which == V4L2_SUBDEV_FORMAT_ACTIVE)
> > + ssd->sink_fmt = *crops[SMIAPP_PAD_SINK];
> > + smiapp_propagate(subdev, fh, fmt->which,
> > + V4L2_SUBDEV_SEL_TGT_CROP_ACTIVE);
> > +
> > + mutex_unlock(&sensor->mutex);
> > +
> > + return 0;
> > +}
> > +
> > +#define SCALING_GOODNESS 100000
> > +#define SCALING_GOODNESS_EXTREME 100000000
> > +static int scaling_goodness(struct v4l2_subdev *subdev, int w, int ask_w,
> > + int h, int ask_h, u32 flags)
> > +{
> > + struct smiapp_sensor *sensor = to_smiapp_sensor(subdev);
> > + struct i2c_client *client = v4l2_get_subdevdata(subdev);
> > + int val = 0;
> > +
> > + w &= ~1;
> > + ask_w &= ~1;
> > + h &= ~1;
> > + ask_h &= ~1;
> > +
> > + if (flags & V4L2_SUBDEV_SEL_FLAG_SIZE_GE) {
> > + if (w < ask_w)
> > + val -= SCALING_GOODNESS;
> > + if (h < ask_h)
> > + val -= SCALING_GOODNESS;
> > + }
> > +
> > + if (flags & V4L2_SUBDEV_SEL_FLAG_SIZE_LE) {
> > + if (w > ask_w)
> > + val -= SCALING_GOODNESS;
> > + if (h > ask_h)
> > + val -= SCALING_GOODNESS;
> > + }
> > +
> > + val -= abs(w - ask_w);
> > + val -= abs(h - ask_h);
> > +
> > + if (w < sensor->limits[SMIAPP_LIMIT_MIN_X_OUTPUT_SIZE])
> > + val -= SCALING_GOODNESS_EXTREME;
> > +
> > + dev_dbg(&client->dev, "w %d ask_w %d h %d ask_h %d goodness %d\n",
> > + w, ask_h, h, ask_h, val);
> > +
> > + return val;
> > +}
> > +
> > +/* We're only called on source pads. This function sets scaling. */
> > +static int smiapp_set_compose(struct v4l2_subdev *subdev,
> > + struct v4l2_subdev_fh *fh,
> > + struct v4l2_subdev_selection *sel)
> > +{
> > + struct i2c_client *client = v4l2_get_subdevdata(subdev);
> > + struct smiapp_sensor *sensor = to_smiapp_sensor(subdev);
> > + struct smiapp_subdev *ssd = to_smiapp_subdev(subdev);
> > + struct v4l2_rect *comp, *crops[SMIAPP_PADS];
> > +
> > + smiapp_get_crop_compose(subdev, fh, crops, &comp, sel->which);
> > +
> > + sel->r.top = 0;
> > + sel->r.left = 0;
> > +
> > + if (ssd == sensor->binner) {
>
> Wouldn't per-subdev operation handlers be more readable ?
Separated the two.
> > + int i;
> > + int binh = 1, binv = 1;
> > + int best = scaling_goodness(
> > + subdev,
> > + crops[SMIAPP_PAD_SINK]->width,
> > + sel->r.width,
> > + crops[SMIAPP_PAD_SINK]->height,
> > + sel->r.height, sel->flags);
> > +
> > + for (i = 0; i < sensor->nbinning_subtypes; i++) {
> > + int this = scaling_goodness(
> > + subdev,
> > + crops[SMIAPP_PAD_SINK]->width
> > + / sensor->binning_subtypes[i].horizontal,
> > + sel->r.width,
> > + crops[SMIAPP_PAD_SINK]->height
> > + / sensor->binning_subtypes[i].vertical,
> > + sel->r.height, sel->flags);
> > +
> > + if (this > best) {
> > + binh = sensor->binning_subtypes[i].horizontal;
> > + binv = sensor->binning_subtypes[i].vertical;
> > + best = this;
> > +
> > + if (sel->which == V4L2_SUBDEV_FORMAT_ACTIVE) {
> > + sensor->binning_vertical = binv;
> > + sensor->binning_horizontal = binh;
> > + }
>
> If the initial best value turns out the be the best one, binning_vertical and
> binning_horizontal will never be updated. Moving the if() outside the for loop
> would solve this.
Good catch. Fixed.
> > + }
> > + }
> > +
> > + sel->r.width =
> > + (crops[SMIAPP_PAD_SINK]->width / binh) & ~1;
> > + sel->r.height =
> > + (crops[SMIAPP_PAD_SINK]->height / binv) & ~1;
> > +
> > + } else {
> > + u32 min, max, a, b, max_m;
> > + u32 scale_m = sensor->limits[SMIAPP_LIMIT_SCALER_N_MIN];
> > + int mode = SMIAPP_SCALING_MODE_HORIZONTAL;
> > + u32 try[4];
> > + u32 ntry = 0;
> > + int i;
> > + int best = INT_MIN;
> > +
> > + sel->r.width = min_t(unsigned int, sel->r.width,
> > + crops[SMIAPP_PAD_SINK]->width);
> > + sel->r.height = min_t(unsigned int, sel->r.height,
> > + crops[SMIAPP_PAD_SINK]->height);
> > +
> > + a = crops[SMIAPP_PAD_SINK]->width
> > + * sensor->limits[SMIAPP_LIMIT_SCALER_N_MIN]
> > + / sel->r.width;
> > + b = crops[SMIAPP_PAD_SINK]->height
> > + * sensor->limits[SMIAPP_LIMIT_SCALER_N_MIN]
> > + / sel->r.height;
> > + max_m = crops[SMIAPP_PAD_SINK]->width
> > + * sensor->limits[SMIAPP_LIMIT_SCALER_N_MIN]
> > + / sensor->limits[SMIAPP_LIMIT_MIN_X_OUTPUT_SIZE];
> > +
> > + a = min(sensor->limits[SMIAPP_LIMIT_SCALER_M_MAX],
> > + max(a, sensor->limits[SMIAPP_LIMIT_SCALER_M_MIN]));
> > + b = min(sensor->limits[SMIAPP_LIMIT_SCALER_M_MAX],
> > + max(b, sensor->limits[SMIAPP_LIMIT_SCALER_M_MIN]));
> > + max_m = min(sensor->limits[SMIAPP_LIMIT_SCALER_M_MAX],
> > + max(max_m,
> > + sensor->limits[SMIAPP_LIMIT_SCALER_M_MIN]));
> > +
> > + dev_dbg(&client->dev, "scaling: a %d b %d max_m %d\n",
> > + a, b, max_m);
> > +
> > + min = min(max_m, min(a, b));
> > + max = min(max_m, max(a, b));
> > +
> > + try[ntry] = min;
> > + ntry++;
> > + if (min != max) {
> > + try[ntry] = max;
> > + ntry++;
> > + }
> > + if (max != max_m) {
> > + try[ntry] = min + 1;
> > + ntry++;
> > + if (min != max) {
> > + try[ntry] = max + 1;
> > + ntry++;
> > + }
> > + }
>
> Please add a comment to explain how you compute the scaling values, the code
> isn't self-explicit.
Added.
> > +
> > + for (i = 0; i < ntry; i++) {
> > + int this = scaling_goodness(
> > + subdev,
> > + crops[SMIAPP_PAD_SINK]->width
> > + / try[i]
> > + * sensor->limits[SMIAPP_LIMIT_SCALER_N_MIN],
> > + sel->r.width,
> > + crops[SMIAPP_PAD_SINK]->height,
> > + sel->r.height,
> > + sel->flags);
> > +
> > + dev_dbg(&client->dev, "trying factor %d (%d)\n",
> > + try[i], i);
> > +
> > + if (this > best) {
> > + scale_m = try[i];
> > + mode = SMIAPP_SCALING_MODE_HORIZONTAL;
> > + best = this;
> > + }
> > +
> > + if (sensor->limits[SMIAPP_LIMIT_SCALING_CAPABILITY]
> > + == SMIAPP_SCALING_CAPABILITY_HORIZONTAL)
> > + continue;
> > +
> > + this = scaling_goodness(
> > + subdev, crops[SMIAPP_PAD_SINK]->width
> > + / try[i]
> > + * sensor->limits[SMIAPP_LIMIT_SCALER_N_MIN],
> > + sel->r.width,
> > + crops[SMIAPP_PAD_SINK]->height
> > + / try[i]
> > + * sensor->limits[SMIAPP_LIMIT_SCALER_N_MIN],
> > + sel->r.height,
> > + sel->flags);
> > +
> > + if (this > best) {
> > + scale_m = try[i];
> > + mode = SMIAPP_SCALING_MODE_BOTH;
> > + best = this;
> > + }
> > + }
> > +
> > + sel->r.width =
> > + (crops[SMIAPP_PAD_SINK]->width
> > + / scale_m
> > + * sensor->limits[SMIAPP_LIMIT_SCALER_N_MIN]) & ~1;
> > + if (mode == SMIAPP_SCALING_MODE_BOTH)
> > + sel->r.height =
> > + (crops[SMIAPP_PAD_SINK]->height
> > + / scale_m
> > + * sensor->limits[SMIAPP_LIMIT_SCALER_N_MIN])
> > + & ~1;
> > + else
> > + sel->r.height = crops[SMIAPP_PAD_SINK]->height;
> > +
> > + if (sel->which == V4L2_SUBDEV_FORMAT_ACTIVE) {
> > + sensor->scale_m = scale_m;
> > + sensor->scaling_mode = mode;
> > + }
> > + }
> > +
> > + *comp = sel->r;
> > + smiapp_propagate(subdev, fh, sel->which,
> > + V4L2_SUBDEV_SEL_TGT_COMPOSE_ACTIVE);
> > +
> > + if (sel->which == V4L2_SUBDEV_FORMAT_ACTIVE)
> > + return smiapp_update_mode(sensor);
> > +
> > + return 0;
> > +}
> > +
> > +static int __smiapp_sel_supported(struct v4l2_subdev *subdev,
> > + struct v4l2_subdev_selection *sel)
> > +{
> > + struct smiapp_sensor *sensor = to_smiapp_sensor(subdev);
> > + struct smiapp_subdev *ssd = to_smiapp_subdev(subdev);
> > +
> > + /* We only implement crop in three places. */
> > + switch (sel->target) {
> > + case V4L2_SUBDEV_SEL_TGT_CROP_ACTIVE:
> > + case V4L2_SUBDEV_SEL_TGT_CROP_BOUNDS:
> > + if (ssd == sensor->pixel_array
> > + && sel->pad == SMIAPP_PAD_SOURCE)
> > + return 0;
> > + if (ssd == sensor->src
> > + && sel->pad == SMIAPP_PAD_SOURCE)
> > + return 0;
> > + if (ssd == sensor->scaler
> > + && sel->pad == SMIAPP_PAD_SINK
> > + && sensor->limits[SMIAPP_LIMIT_DIGITAL_CROP_CAPABILITY]
> > + == SMIAPP_DIGITAL_CROP_CAPABILITY_INPUT_CROP)
> > + return 0;
> > + return -EINVAL;
> > + case V4L2_SUBDEV_SEL_TGT_COMPOSE_ACTIVE:
> > + case V4L2_SUBDEV_SEL_TGT_COMPOSE_BOUNDS:
> > + if (sel->pad != SMIAPP_PAD_SINK)
> > + return -EINVAL;
> > + if (ssd == sensor->binner)
> > + return 0;
> > + if (ssd == sensor->scaler
> > + && sensor->limits[SMIAPP_LIMIT_SCALING_CAPABILITY]
> > + != SMIAPP_SCALING_CAPABILITY_NONE)
> > + return 0;
> > + /* Fall through */
> > + default:
> > + return -EINVAL;
> > + }
>
> What about returning 1 if the selection target is supported, and 0 if it isn't
> ?
Well, this way I can easily return a reasonable error code --- should it be
changed, it's just a single place in the driver. :-)
> > +}
> > +
> > +static int smiapp_set_crop(struct v4l2_subdev *subdev,
> > + struct v4l2_subdev_fh *fh,
> > + struct v4l2_subdev_selection *sel)
> > +{
> > + struct smiapp_sensor *sensor = to_smiapp_sensor(subdev);
> > + struct smiapp_subdev *ssd = to_smiapp_subdev(subdev);
> > + struct v4l2_rect *src_size, *crops[SMIAPP_PADS];
> > + struct v4l2_rect _r;
> > +
> > + smiapp_get_crop_compose(subdev, fh, crops, NULL, sel->which);
> > +
> > + if (sel->which == V4L2_SUBDEV_FORMAT_ACTIVE) {
> > + if (sel->pad == SMIAPP_PAD_SINK)
> > + src_size = &ssd->sink_fmt;
> > + else
> > + src_size = &ssd->compose;
> > + } else {
> > + if (sel->pad == SMIAPP_PAD_SINK) {
> > + _r.left = _r.top = 0;
>
> One assignment per line please.
Fixed.
> > + _r.width = v4l2_subdev_get_try_format(fh, sel->pad)
> > + ->width;
> > + _r.height = v4l2_subdev_get_try_format(fh, sel->pad)
> > + ->height;
> > + src_size = &_r;
> > + } else {
> > + src_size =
> > + v4l2_subdev_get_try_compose(fh,
> > + SMIAPP_PAD_SINK);
>
> Why do you get the try compose rectangle when setting the crop rectangle ?
This is source pad, so the source crop is directly related to sink compose.
> > + }
> > + }
>
> This looks a bit too complex to me (but it's just a feeling).
>
> > +
> > + if (ssd == sensor->src && sel->pad == SMIAPP_PAD_SOURCE)
> > + sel->r.left = 0, sel->r.top = 0;
>
> Please...
I think assigning a static value to multiple variables at the same time
should be allowed. I'm sure Kernighan and Ritchie didn't put it there just
to annoy people. :-)
I changed it nevertheless since you're asking so kindly. ;-)
> > +
> > + if (sel->r.width > src_size->width)
> > + sel->r.width = src_size->width;
> > + if (sel->r.height > src_size->height)
> > + sel->r.height = src_size->height;
> > +
> > + if (sel->r.left + sel->r.width > src_size->width)
> > + sel->r.left =
> > + src_size->width - sel->r.width;
> > + if (sel->r.top + sel->r.height > src_size->height)
> > + sel->r.top =
> > + src_size->height - sel->r.height;
> > +
> > + *crops[sel->pad] = sel->r;
> > +
> > + if (sel->pad == SMIAPP_PAD_SINK)
> > + smiapp_propagate(subdev, fh, sel->which,
> > + V4L2_SUBDEV_SEL_TGT_CROP_ACTIVE);
> > +
> > + return 0;
> > +}
> > +
> > +static int __smiapp_get_selection(struct v4l2_subdev *subdev,
> > + struct v4l2_subdev_fh *fh,
> > + struct v4l2_subdev_selection *sel)
> > +{
> > + struct smiapp_sensor *sensor = to_smiapp_sensor(subdev);
> > + struct smiapp_subdev *ssd = to_smiapp_subdev(subdev);
> > + struct v4l2_rect *comp, *crops[SMIAPP_PADS];
> > + struct v4l2_rect sink_fmt;
> > + int ret;
> > +
> > + ret = __smiapp_sel_supported(subdev, sel);
> > + if (ret)
> > + return ret;
> > +
> > + smiapp_get_crop_compose(subdev, fh, crops, &comp, sel->which);
> > +
> > + if (sel->which == V4L2_SUBDEV_FORMAT_ACTIVE) {
> > + sink_fmt = ssd->sink_fmt;
> > + } else if (ssd != sensor->pixel_array) {
> > + struct v4l2_mbus_framefmt *fmt =
> > + v4l2_subdev_get_try_format(fh, SMIAPP_PAD_SINK);
> > +
> > + sink_fmt.left = sink_fmt.top = 0;
> > + sink_fmt.width = fmt->width;
> > + sink_fmt.height = fmt->height;
> > + } else {
> > + BUG();
>
> So you support active selections on the pixel array subdev, but not try
> selections ?
Removed the pixel array check. The BUG()'s now in
v4l2_subdev_get_try_format(). The real check's in __smiapp_sel_supported()
above.
> > + }
> > +
> > + switch (sel->target) {
> > + case V4L2_SUBDEV_SEL_TGT_CROP_BOUNDS:
> > + if (ssd == sensor->pixel_array) {
> > + sel->r.width =
> > + sensor->limits[SMIAPP_LIMIT_X_ADDR_MAX] + 1;
> > + sel->r.height =
> > + sensor->limits[SMIAPP_LIMIT_Y_ADDR_MAX] + 1;
> > + } else if (sel->pad == SMIAPP_PAD_SINK) {
> > + sel->r = sink_fmt;
> > + } else {
> > + sel->r = *comp;
> > + }
> > + break;
> > + case V4L2_SUBDEV_SEL_TGT_CROP_ACTIVE:
> > + case V4L2_SUBDEV_SEL_TGT_COMPOSE_BOUNDS:
> > + sel->r = *crops[sel->pad];
> > + break;
> > + case V4L2_SUBDEV_SEL_TGT_COMPOSE_ACTIVE:
> > + sel->r = *comp;
> > + break;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static int smiapp_get_selection(struct v4l2_subdev *subdev,
> > + struct v4l2_subdev_fh *fh,
> > + struct v4l2_subdev_selection *sel)
> > +{
> > + struct smiapp_sensor *sensor = to_smiapp_sensor(subdev);
> > + int rval;
> > +
> > + mutex_lock(&sensor->mutex);
> > + rval = __smiapp_get_selection(subdev, fh, sel);
> > + mutex_unlock(&sensor->mutex);
> > +
> > + return rval;
> > +}
> > +static int smiapp_set_selection(struct v4l2_subdev *subdev,
> > + struct v4l2_subdev_fh *fh,
> > + struct v4l2_subdev_selection *sel)
> > +{
> > + struct smiapp_sensor *sensor = to_smiapp_sensor(subdev);
> > + int ret;
> > +
> > + ret = __smiapp_sel_supported(subdev, sel);
> > + if (ret)
> > + return ret;
> > +
> > + mutex_lock(&sensor->mutex);
> > +
> > + sel->r.left = max(0, sel->r.left & ~1);
> > + sel->r.top = max(0, sel->r.top & ~1);
> > + sel->r.width = max(0, SMIAPP_ALIGN_DIM(sel->r.width, sel->flags));
> > + sel->r.height = max(0, SMIAPP_ALIGN_DIM(sel->r.height, sel->flags));
> > +
> > + sel->r.width = max_t(unsigned int,
> > + sensor->limits[SMIAPP_LIMIT_MIN_X_OUTPUT_SIZE],
> > + sel->r.width);
> > + sel->r.height = max_t(unsigned int,
> > + sensor->limits[SMIAPP_LIMIT_MIN_Y_OUTPUT_SIZE],
> > + sel->r.height);
> > +
> > + switch (sel->target) {
> > + case V4L2_SUBDEV_SEL_TGT_CROP_ACTIVE:
> > + ret = smiapp_set_crop(subdev, fh, sel);
> > + break;
> > + case V4L2_SUBDEV_SEL_TGT_COMPOSE_ACTIVE:
> > + ret = smiapp_set_compose(subdev, fh, sel);
> > + break;
> > + default:
> > + BUG();
> > + }
> > +
> > + mutex_unlock(&sensor->mutex);
> > + return ret;
> > +}
> > +
> > +static int smiapp_get_skip_frames(struct v4l2_subdev *subdev, u32 *frames)
> > +{
> > + struct smiapp_sensor *sensor = to_smiapp_sensor(subdev);
> > +
> > + *frames = sensor->frame_skip;
> > + return 0;
> > +}
> > +
> > +/*
> > ---------------------------------------------------------------------------
> > -- + * sysfs attributes
> > + */
> > +
> > +static ssize_t
> > +smiapp_sysfs_nvm_read(struct device *dev, struct device_attribute *attr,
> > + char *buf)
> > +{
> > + struct v4l2_subdev *subdev = i2c_get_clientdata(to_i2c_client(dev));
> > + struct i2c_client *client = v4l2_get_subdevdata(subdev);
> > + struct smiapp_sensor *sensor = to_smiapp_sensor(subdev);
> > + int nbytes;
> > +
> > + if (!sensor->dev_init_done)
> > + return -EBUSY;
> > +
> > + if (!sensor->nvm_size) {
> > + /* NVM not read yet - read it now */
> > + sensor->nvm_size = sensor->platform_data->nvm_size;
> > + if (smiapp_set_power(subdev, 1) < 0)
> > + return -ENODEV;
> > + if (smiapp_read_nvm(sensor, sensor->nvm)) {
> > + dev_err(&client->dev, "nvm read failed\n");
> > + return -ENODEV;
> > + }
> > + smiapp_set_power(subdev, 0);
> > + }
> > + /*
> > + * NVM is still way below a PAGE_SIZE, so we can safely
> > + * assume this for now.
> > + */
> > + nbytes = min_t(unsigned int, sensor->nvm_size, PAGE_SIZE);
> > + memcpy(buf, sensor->nvm, nbytes);
> > +
> > + return nbytes;
> > +}
> > +static DEVICE_ATTR(nvm, S_IRUGO, smiapp_sysfs_nvm_read, NULL);
> > +
> > +/*
> > ---------------------------------------------------------------------------
> > -- + * V4L2 subdev core operations
> > + */
> > +
> > +static int smiapp_identify_module(struct v4l2_subdev *subdev)
> > +{
> > + struct smiapp_sensor *sensor = to_smiapp_sensor(subdev);
> > + struct i2c_client *client = v4l2_get_subdevdata(subdev);
> > + int i, rval = 0;
> > + struct smiapp_module_info *minfo = &sensor->minfo;
> > +
> > + minfo->name = SMIAPP_NAME;
> > +
> > + /* Module info */
> > + rval = smia_i2c_read_reg(client,
> > + SMIAPP_REG_U8_MANUFACTURER_ID,
> > + &minfo->manufacturer_id);
> > + if (!rval)
> > + rval = smia_i2c_read_reg(client,
> > + SMIAPP_REG_U16_MODEL_ID,
> > + &minfo->model_id);
> > + if (!rval)
> > + rval |= smia_i2c_read_reg(client,
> > + SMIAPP_REG_U8_REVISION_NUMBER_MAJOR,
> > + &minfo->revision_number_major);
>
> s/|=/=/
Fixed.
> > + if (!rval)
> > + rval |= smia_i2c_read_reg(client,
> > + SMIAPP_REG_U8_REVISION_NUMBER_MINOR,
> > + &minfo->revision_number_minor);
> > + if (!rval)
> > + rval |= smia_i2c_read_reg(client,
> > + SMIAPP_REG_U8_MODULE_DATE_YEAR,
> > + &minfo->module_year);
> > + if (!rval)
> > + rval |= smia_i2c_read_reg(client,
> > + SMIAPP_REG_U8_MODULE_DATE_MONTH,
> > + &minfo->module_month);
> > + if (!rval)
> > + rval |= smia_i2c_read_reg(client,
> > + SMIAPP_REG_U8_MODULE_DATE_DAY,
> > + &minfo->module_day);
> > +
> > + /* Sensor info */
> > + if (!rval)
> > + rval |= smia_i2c_read_reg(client,
> > + SMIAPP_REG_U8_SENSOR_MANUFACTURER_ID,
> > + &minfo->sensor_manufacturer_id);
> > + if (!rval)
> > + rval |= smia_i2c_read_reg(client,
> > + SMIAPP_REG_U16_SENSOR_MODEL_ID,
> > + &minfo->sensor_model_id);
> > + if (!rval)
> > + rval = smia_i2c_read_reg(client,
> > + SMIAPP_REG_U8_SENSOR_REVISION_NUMBER,
> > + &minfo->sensor_revision_number);
> > + if (!rval)
> > + rval = smia_i2c_read_reg(client,
> > + SMIAPP_REG_U8_SENSOR_FIRMWARE_VERSION,
> > + &minfo->sensor_firmware_version);
> > +
> > + /* SMIA */
> > + if (!rval)
> > + rval = smia_i2c_read_reg(client,
> > + SMIAPP_REG_U8_SMIA_VERSION,
> > + &minfo->smia_version);
> > + if (!rval)
> > + rval = smia_i2c_read_reg(client,
> > + SMIAPP_REG_U8_SMIAPP_VERSION,
> > + &minfo->smiapp_version);
> > +
> > + if (rval) {
> > + dev_err(&client->dev, "sensor detection failed\n");
> > + return -ENODEV;
> > + }
> > +
> > + dev_dbg(&client->dev, "module 0x%2.2x-0x%4.4x\n",
>
> "module 0x%02x-0x%04x\n" (and similarly below) ?
Hmm. %y.yx means exactly y characters of %x. What does %0yx mean?
> > + minfo->manufacturer_id, minfo->model_id);
> > +
> > + dev_dbg(&client->dev,
> > + "module revision 0x%2.2x-0x%2.2x date %2.2d-%2.2d-%2.2d\n",
> > + minfo->revision_number_major, minfo->revision_number_minor,
> > + minfo->module_year, minfo->module_month, minfo->module_day);
> > +
> > + dev_dbg(&client->dev, "sensor 0x%2.2x-0x%4.4x\n",
> > + minfo->sensor_manufacturer_id, minfo->sensor_model_id);
> > +
> > + dev_dbg(&client->dev,
> > + "sensor revision 0x%2.2x firmware version 0x%2.2x\n",
> > + minfo->sensor_revision_number, minfo->sensor_firmware_version);
> > +
> > + dev_dbg(&client->dev, "smia version %2.2d smiapp version %2.2d\n",
> > + minfo->smia_version, minfo->smiapp_version);
> > +
>
> Could you please add a short comment to explain why this is needed ?
The one below? Some devices just have bad data in these variables. Hopefully
the other variables have better stuff.
The lvalues are module parameters whereas the rvalues are sensor parameters.
> > + if (!minfo->manufacturer_id && !minfo->model_id) {
> > + minfo->manufacturer_id = minfo->sensor_manufacturer_id;
> > + minfo->model_id = minfo->sensor_model_id;
> > + minfo->revision_number_major = minfo->sensor_revision_number;
> > + }
> > +
> > + for (i = 0; i < ARRAY_SIZE(smiapp_module_idents); i++) {
> > + if (smiapp_module_idents[i].manufacturer_id
> > + != minfo->manufacturer_id)
> > + continue;
> > + if (smiapp_module_idents[i].model_id != minfo->model_id)
> > + continue;
> > + if (smiapp_module_idents[i].flags
> > + & SMIAPP_MODULE_IDENT_FLAG_REV_LE) {
> > + if (smiapp_module_idents[i].revision_number_major
> > + < minfo->revision_number_major)
> > + continue;
> > + } else {
> > + if (smiapp_module_idents[i].revision_number_major
> > + != minfo->revision_number_major)
> > + continue;
> > + }
> > +
> > + minfo->name = smiapp_module_idents[i].name;
> > + minfo->quirk = smiapp_module_idents[i].quirk;
> > + break;
> > + }
> > +
> > + if (i >= ARRAY_SIZE(smiapp_module_idents))
> > + dev_warn(&client->dev, "no quirks for this module\n");
>
> Maybe a message such as "unknown SMIA++ module - trying generic support" would
> be better ? Many of the known modules have no quirks.
I'd like to think it as a positive message of the conformance of the sensor
--- still it may inform that the quirks are actually missing. What do you
think?
> > +
> > + dev_dbg(&client->dev, "the sensor is called %s, ident %2.2x%4.4x%2.2x\n",
> > + minfo->name, minfo->manufacturer_id, minfo->model_id,
> > + minfo->revision_number_major);
> > +
> > + strlcpy(subdev->name, sensor->minfo.name, sizeof(subdev->name));
> > +
> > + return 0;
> > +}
> > +
> > +static const struct v4l2_subdev_ops smiapp_ops;
> > +static const struct v4l2_subdev_internal_ops smiapp_internal_ops;
> > +static const struct media_entity_operations smiapp_entity_ops;
> > +
> > +static int smiapp_registered(struct v4l2_subdev *subdev)
> > +{
> > + struct smiapp_sensor *sensor = to_smiapp_sensor(subdev);
> > + struct i2c_client *client = v4l2_get_subdevdata(subdev);
> > + struct smiapp_subdev *last = NULL;
> > + int rval;
> > + u32 tmp, i;
> > +
> > + sensor->vana = regulator_get(&client->dev, "VANA");
> > + if (IS_ERR(sensor->vana)) {
> > + dev_err(&client->dev, "could not get regulator for vana\n");
> > + return -ENODEV;
> > + }
> > +
> > + if (sensor->platform_data->xshutdown != SMIAPP_NO_XSHUTDOWN) {
> > + if (gpio_request_one(sensor->platform_data->xshutdown, 0,
> > + "SMIA++ xshutdown") != 0) {
> > + dev_err(&client->dev,
> > + "unable to acquire reset gpio %d\n",
> > + sensor->platform_data->xshutdown);
> > + rval = -ENODEV;
> > + goto out_gpio_request;
> > + }
> > + }
> > +
> > + rval = smiapp_power_on(sensor);
> > + if (rval) {
> > + rval = -ENODEV;
> > + goto out_smiapp_power_on;
> > + }
> > +
> > + rval = smiapp_identify_module(subdev);
> > + if (rval) {
> > + rval = -ENODEV;
> > + goto out_power_off;
> > + }
> > +
> > + rval = smiapp_get_all_limits(sensor);
> > + if (rval) {
> > + rval = -ENODEV;
> > + goto out_power_off;
> > + }
> > +
> > + /*
> > + * Handle Sensor Module orientation on the board.
> > + *
> > + * The application of H-FLIP and V-FLIP on the sensor is modified by
> > + * the sensor orientation on the board.
> > + *
> > + * For SMIAPP_BOARD_SENSOR_ORIENT_180 the default behaviour is to set
> > + * both H-FLIP and V-FLIP for normal operation which also implies
> > + * that a set/unset operation for user space HFLIP and VFLIP v4l2
> > + * controls will need to be internally inverted.
> > + *
> > + * Rotation also changes the bayer pattern.
> > + */
> > + if (sensor->platform_data->module_board_orient ==
> > + SMIAPP_MODULE_BOARD_ORIENT_180)
> > + sensor->hvflip_inv_mask = SMIAPP_IMAGE_ORIENTATION_HFLIP |
> > + SMIAPP_IMAGE_ORIENTATION_VFLIP;
> > +
> > + rval = smiapp_get_mbus_formats(sensor);
> > + if (rval) {
> > + rval = -ENODEV;
> > + goto out_power_off;
> > + }
> > +
> > + if (sensor->limits[SMIAPP_LIMIT_BINNING_CAPABILITY]) {
> > + int i;
> > + int val;
> > +
> > + rval = smia_i2c_read_reg(client,
> > + SMIAPP_REG_U8_BINNING_SUBTYPES, &val);
> > + if (rval < 0) {
> > + rval = -ENODEV;
> > + goto out_power_off;
> > + }
> > + sensor->nbinning_subtypes = min_t(u8, val,
> > + SMIAPP_BINNING_SUBTYPES);
> > +
> > + for (i = 0; i < sensor->nbinning_subtypes; i++) {
> > + rval = smia_i2c_read_reg(
> > + client, SMIAPP_REG_U8_BINNING_TYPE_n(i), &val);
> > + if (rval < 0) {
> > + rval = -ENODEV;
> > + goto out_power_off;
> > + }
> > + sensor->binning_subtypes[i] =
> > + *(struct smiapp_binning_subtype *)&val;
> > +
> > + dev_dbg(&client->dev, "binning %xx%x\n",
> > + sensor->binning_subtypes[i].horizontal,
> > + sensor->binning_subtypes[i].vertical);
> > + }
> > + }
> > + sensor->binning_horizontal = 1;
> > + sensor->binning_vertical = 1;
> > +
> > + /* SMIA++ NVM initialization - it will be read from the sensor
> > + * when it is first requested by userspace.
> > + */
> > + if (sensor->minfo.smiapp_version && sensor->platform_data->nvm_size) {
> > + sensor->nvm = kzalloc(sensor->platform_data->nvm_size,
> > + GFP_KERNEL);
> > + if (sensor->nvm == NULL) {
> > + dev_err(&client->dev, "nvm buf allocation failed\n");
> > + rval = -ENOMEM;
> > + goto out_power_off;
> > + }
> > +
> > + if (device_create_file(&client->dev, &dev_attr_nvm) != 0) {
> > + dev_err(&client->dev, "sysfs nvm entry failed\n");
> > + rval = -EBUSY;
> > + goto out_nvm_release1;
> > + }
> > + }
> > +
> > + rval = smiapp_call_quirk(sensor, limits);
> > + if (rval) {
> > + dev_err(&client->dev, "limits quirks failed\n");
> > + goto out_nvm_release2;
> > + }
> > +
> > + /* We consider this as profile 0 sensor if any of these are zero. */
> > + if (!sensor->limits[SMIAPP_LIMIT_MIN_OP_SYS_CLK_DIV] ||
> > + !sensor->limits[SMIAPP_LIMIT_MAX_OP_SYS_CLK_DIV] ||
> > + !sensor->limits[SMIAPP_LIMIT_MIN_OP_PIX_CLK_DIV] ||
> > + !sensor->limits[SMIAPP_LIMIT_MAX_OP_PIX_CLK_DIV]) {
> > + sensor->minfo.smiapp_profile = SMIAPP_PROFILE_0;
> > + } else if (sensor->limits[SMIAPP_LIMIT_SCALING_CAPABILITY]
> > + != SMIAPP_SCALING_CAPABILITY_NONE) {
> > + if (sensor->limits[SMIAPP_LIMIT_SCALING_CAPABILITY]
> > + == SMIAPP_SCALING_CAPABILITY_HORIZONTAL)
> > + sensor->minfo.smiapp_profile = SMIAPP_PROFILE_1;
> > + else
> > + sensor->minfo.smiapp_profile = SMIAPP_PROFILE_2;
> > + sensor->scaler = &sensor->sds[sensor->sds_used];
> > + sensor->sds_used++;
> > + } else if (sensor->limits[SMIAPP_LIMIT_DIGITAL_CROP_CAPABILITY]
> > + == SMIAPP_DIGITAL_CROP_CAPABILITY_INPUT_CROP) {
> > + sensor->scaler = &sensor->sds[sensor->sds_used];
> > + sensor->sds_used++;
> > + }
> > + sensor->binner = &sensor->sds[sensor->sds_used];
> > + sensor->sds_used++;
> > + sensor->pixel_array = &sensor->sds[sensor->sds_used];
> > + sensor->sds_used++;
> > +
> > + sensor->scale_m = sensor->limits[SMIAPP_LIMIT_SCALER_N_MIN];
> > +
> > + for (i = 0; i < SMIAPP_SUBDEVS; i++) {
> > + struct {
> > + struct smiapp_subdev *sds;
> > + char *name;
> > + } _t[] = {
> > + { sensor->scaler, "scaler", },
> > + { sensor->binner, "binner", },
> > + { sensor->pixel_array, "pixel array", },
> > + }, *this = &_t[i];
>
> What about moving _t outside of the loop (and renaming it to something more
> explicit) ? There's no need to initialize it at each iteration.
I will have to populate it in code rather than initialiser, or I'll add more
braces. I think the compiler will optimise that nevertheless. I could make
it const. How about that? :-)
> > + if (!this->sds)
> > + continue;
> > +
> > + if (this->sds != sensor->src)
> > + v4l2_subdev_init(&this->sds->sd, &smiapp_ops);
> > +
> > + this->sds->sensor = sensor;
> > +
> > + if (this->sds == sensor->pixel_array) {
> > + if (this->sds == sensor->src)
> > + sensor->sds->sd.entity.num_pads = 1;
>
> That's supposed to be initialized by media_entity_init(), why do you need to
> set it explictly here ?
For historical reasons... I once didn't expect the binner to always exist.
It now does. Removed the two lines.
> > + this->sds->npads = 1;
> > + } else {
> > + this->sds->npads = 2;
> > + }
> > +
> > + snprintf(this->sds->sd.name,
> > + sizeof(this->sds->sd.name), "%s %s",
> > + sensor->minfo.name, this->name);
> > +
> > + this->sds->sink_fmt.width =
> > + sensor->limits[SMIAPP_LIMIT_X_ADDR_MAX] + 1;
> > + this->sds->sink_fmt.height =
> > + sensor->limits[SMIAPP_LIMIT_Y_ADDR_MAX] + 1;
> > + this->sds->crop[SMIAPP_PAD_SINK].width =
> > + this->sds->sink_fmt.width;
> > + this->sds->crop[SMIAPP_PAD_SINK].height =
> > + this->sds->sink_fmt.height;
> > + this->sds->crop[SMIAPP_PAD_SOURCE] =
> > + this->sds->compose =
> > + this->sds->crop[SMIAPP_PAD_SINK];
> > +
> > + this->sds->pads[1].flags = MEDIA_PAD_FL_SINK;
> > + this->sds->pads[0].flags = MEDIA_PAD_FL_SOURCE;
>
> Pad 1 is the sink pad and pad 0 the source pad ? That's very unusual, couldn't
> you make it the other way around ?
Originally, when I was creating the src entity I didn't know whether it was
going to have one or two pads. There's one reason left tough: I now know all
my sink pads have id 1 and source pads have id 0. That makes things easier
in a number of places.
> > +
> > + this->sds->sd.entity.ops = &smiapp_entity_ops;
> > +
> > + if (last == NULL) {
> > + last = this->sds;
> > + continue;
> > + }
> > +
> > + this->sds->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
> > + this->sds->sd.internal_ops = &smiapp_internal_ops;
> > + this->sds->sd.owner = NULL;
> > + v4l2_set_subdevdata(&this->sds->sd, client);
> > +
> > + rval = v4l2_device_register_subdev(sensor->src->sd.v4l2_dev,
> > + &this->sds->sd);
> > + if (rval) {
> > + dev_err(&client->dev,
> > + "v4l2_device_register_subdev failed\n");
> > + goto out_nvm_release2;
> > + }
> > +
> > + rval = media_entity_init(&this->sds->sd.entity,
> > + this->sds->npads, this->sds->pads, 0);
> > + if (rval) {
> > + dev_err(&client->dev,
> > + "media_entity_init failed\n");
> > + goto out_nvm_release2;
> > + }
>
> You should initialize the entity (and possibly create the link) before
> registering the subdev.
Fixed.
> > +
> > + rval = media_entity_create_link(&this->sds->sd.entity, 0,
> > + &last->sd.entity, 1,
> > + MEDIA_LNK_FL_ENABLED |
> > + MEDIA_LNK_FL_IMMUTABLE);
> > + if (rval) {
> > + dev_err(&client->dev,
> > + "media_entity_create_link failed\n");
> > + goto out_nvm_release2;
> > + }
> > +
> > + last = this->sds;
> > + }
> > +
> > + dev_dbg(&client->dev, "profile %d\n", sensor->minfo.smiapp_profile);
> > +
> > + sensor->pixel_array->sd.entity.type = MEDIA_ENT_T_V4L2_SUBDEV_SENSOR;
> > +
> > + /* final steps */
> > + smiapp_read_frame_fmt(sensor);
> > + rval = smiapp_init_controls(sensor);
> > + if (rval < 0)
> > + goto out_nvm_release2;
> > +
> > + rval = smiapp_update_mode(sensor);
> > + if (rval) {
> > + dev_err(&client->dev, "update mode failed\n");
> > + goto out_nvm_release2;
> > + }
> > +
> > + sensor->streaming = false;
> > + sensor->dev_init_done = true;
> > +
> > + /* check flash capability */
> > + rval = smia_i2c_read_reg(client,
> > + SMIAPP_REG_U8_FLASH_MODE_CAPABILITY, &tmp);
> > + sensor->flash_capability = tmp;
> > + if (rval)
> > + goto out_nvm_release2;
> > +
> > + smiapp_power_off(sensor);
>
> Shouldn't you take the sensor mutex around the whole function ?
I don't think it's necessary --- the device nodes will be created by the
master driver later on so no-one can access them yet.
> > +
> > + return 0;
> > +
> > +out_nvm_release2:
> > + device_remove_file(&client->dev, &dev_attr_nvm);
> > +
> > +out_nvm_release1:
> > + kfree(sensor->nvm);
> > + sensor->nvm = NULL;
>
> You can move this under out_power_off, if sensor->nvm is already NULL kfree
> will be a no-op.
Ack.
> > +
> > +out_power_off:
> > + smiapp_power_off(sensor);
> > +
> > +out_smiapp_power_on:
> > + if (sensor->platform_data->xshutdown != SMIAPP_NO_XSHUTDOWN)
> > + gpio_free(sensor->platform_data->xshutdown);
> > +
> > +out_gpio_request:
> > + regulator_put(sensor->vana);
> > + sensor->vana = NULL;
> > + return rval;
> > +}
> > +
> > +static int smiapp_open(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh)
> > +{
> > + struct smiapp_subdev *ssd = to_smiapp_subdev(sd);
> > + struct v4l2_subdev_selection sel;
> > + struct v4l2_rect *try_sel;
> > + int i;
> > + int rval;
> > +
> > + mutex_lock(&ssd->sensor->power_mutex);
> > + mutex_lock(&ssd->sensor->mutex);
> > +
> > + for (i = 0; i < ssd->npads; i++) {
> > + struct v4l2_subdev_format fmt;
> > + struct v4l2_mbus_framefmt *try_fmt;
> > +
> > + fmt.which = V4L2_SUBDEV_FORMAT_ACTIVE;
> > + fmt.pad = i;
> > + __smiapp_get_format(sd, fh, &fmt);
> > + try_fmt = v4l2_subdev_get_try_format(fh, i);
> > + *try_fmt = fmt.format;
> > +
> > + sel.which = V4L2_SUBDEV_FORMAT_ACTIVE;
> > + sel.pad = i;
> > + sel.target = V4L2_SUBDEV_SEL_TGT_CROP_ACTIVE;
> > + __smiapp_get_selection(sd, fh, &sel);
> > + try_sel = v4l2_subdev_get_try_crop(fh, i);
> > + *try_sel = sel.r;
>
> Wouldn't it be better to use the default values instead of the active ones
> here ?
Good question.
> > + }
> > +
> > + if (ssd != ssd->sensor->pixel_array) {
> > + sel.which = V4L2_SUBDEV_FORMAT_ACTIVE;
> > + sel.pad = SMIAPP_PAD_SINK;
> > + sel.target = V4L2_SUBDEV_SEL_TGT_COMPOSE_ACTIVE;
> > + __smiapp_get_selection(sd, fh, &sel);
> > + try_sel = v4l2_subdev_get_try_compose(fh, SMIAPP_PAD_SINK);
> > + *try_sel = sel.r;
> > + }
> > +
> > + rval = smiapp_set_power(sd, 1);
> > +
> > + mutex_unlock(&ssd->sensor->mutex);
> > +
> > + if (rval < 0)
> > + goto out;
> > +
> > + /* Was the sensor already powered on? */
> > + if (ssd->sensor->power_count > 1)
>
> power_count is accessed in smiapp_set_power without taking the power_mutex
> lock. Are two locks really needed ?
Well, now that you mention it, control handler setup function that wouldn't
take the locks would resolve the issue, I think. Should I create one?
> > + goto out;
> > +
> > + for (i = 0; i < ssd->sensor->sds_used; i++) {
> > + rval = v4l2_ctrl_handler_setup(
> > + &ssd->sensor->sds[i].ctrl_handler);
> > + if (rval)
> > + goto out;
> > + }
>
> Doesn't this belong to the set power handler ?
Yes, it does. Fixed.
> > +
> > +out:
> > + mutex_unlock(&ssd->sensor->power_mutex);
> > +
> > + return rval;
> > +}
> > +
> > +static int smiapp_close(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh)
> > +{
> > + struct smiapp_sensor *sensor = to_smiapp_sensor(sd);
> > + int rval;
> > +
> > + mutex_lock(&sensor->power_mutex);
> > + mutex_lock(&sensor->mutex);
> > + rval = smiapp_set_power(sd, 0);
> > + mutex_unlock(&sensor->mutex);
> > + mutex_unlock(&sensor->power_mutex);
> > +
> > + return rval;
> > +}
> > +
> > +static const struct v4l2_subdev_video_ops smiapp_video_ops = {
> > + .s_stream = smiapp_set_stream,
> > +};
> > +
> > +static const struct v4l2_subdev_core_ops smiapp_core_ops = {
> > + .s_power = smiapp_set_power,
> > +};
> > +
> > +static const struct v4l2_subdev_pad_ops smiapp_pad_ops = {
> > + .enum_mbus_code = smiapp_enum_mbus_code,
> > + .get_fmt = smiapp_get_format,
> > + .set_fmt = smiapp_set_format,
> > + .get_selection = smiapp_get_selection,
> > + .set_selection = smiapp_set_selection,
> > + .link_validate = v4l2_subdev_link_validate_default,
>
> This can be left NULL.
Fixed.
> > +};
> > +
> > +static const struct v4l2_subdev_sensor_ops smiapp_sensor_ops = {
> > + .g_skip_frames = smiapp_get_skip_frames,
> > +};
> > +
> > +static const struct v4l2_subdev_ops smiapp_ops = {
> > + .core = &smiapp_core_ops,
> > + .video = &smiapp_video_ops,
> > + .pad = &smiapp_pad_ops,
> > + .sensor = &smiapp_sensor_ops,
> > +};
> > +
> > +static const struct media_entity_operations smiapp_entity_ops = {
> > + .link_validate = v4l2_subdev_link_validate,
> > +};
> > +
> > +static const struct v4l2_subdev_internal_ops smiapp_internal_src_ops = {
> > + .registered = smiapp_registered,
> > + .open = smiapp_open,
> > + .close = smiapp_close,
> > +};
> > +
> > +static const struct v4l2_subdev_internal_ops smiapp_internal_ops = {
> > + .open = smiapp_open,
> > + .close = smiapp_close,
> > +};
> > +
> > +/*
> > ---------------------------------------------------------------------------
> > -- + * I2C Driver
> > + */
> > +
> > +#ifdef CONFIG_PM
> > +
> > +static int smiapp_suspend(struct device *dev)
> > +{
> > + struct i2c_client *client = to_i2c_client(dev);
> > + struct v4l2_subdev *subdev = i2c_get_clientdata(client);
> > + struct smiapp_sensor *sensor = to_smiapp_sensor(subdev);
> > + int ss;
> > +
> > + BUG_ON(mutex_is_locked(&sensor->mutex));
> > +
> > + if (sensor->power_count == 0)
> > + return 0;
> > +
> > + if (sensor->streaming)
> > + smiapp_stop_streaming(sensor);
> > +
> > + ss = sensor->streaming;
> > +
> > + smiapp_power_off(sensor);
> > +
> > + /* save state for resume */
> > + sensor->streaming = ss;
> > +
> > + return 0;
> > +}
> > +
> > +static int smiapp_resume(struct device *dev)
> > +{
> > + struct i2c_client *client = to_i2c_client(dev);
> > + struct v4l2_subdev *subdev = i2c_get_clientdata(client);
> > + struct smiapp_sensor *sensor = to_smiapp_sensor(subdev);
> > + int rval;
> > +
> > + if (sensor->power_count == 0)
> > + return 0;
> > +
> > + rval = smiapp_power_on(sensor);
> > + if (rval)
> > + return rval;
> > +
> > + if (sensor->streaming)
> > + rval = smiapp_start_streaming(sensor);
> > +
> > + return rval;
> > +}
> > +
> > +#else
> > +
> > +#define smiapp_suspend NULL
> > +#define smiapp_resume NULL
> > +
> > +#endif /* CONFIG_PM */
> > +
> > +static int smiapp_probe(struct i2c_client *client,
> > + const struct i2c_device_id *devid)
> > +{
> > + struct smiapp_sensor *sensor;
> > + int rval;
> > +
> > + if (client->dev.platform_data == NULL)
> > + return -ENODEV;
> > +
> > + sensor = kzalloc(sizeof(*sensor), GFP_KERNEL);
> > + if (sensor == NULL)
> > + return -ENOMEM;
> > +
> > + sensor->platform_data = client->dev.platform_data;
> > + mutex_init(&sensor->mutex);
> > + mutex_init(&sensor->power_mutex);
> > + sensor->src = &sensor->sds[sensor->sds_used];
> > +
> > + v4l2_i2c_subdev_init(&sensor->src->sd, client, &smiapp_ops);
> > + sensor->src->sd.internal_ops = &smiapp_internal_src_ops;
> > + sensor->src->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
> > + sensor->src->sensor = sensor;
> > +
> > + sensor->src->pads[0].flags = MEDIA_PAD_FL_SOURCE;
> > + rval = media_entity_init(&sensor->src->sd.entity, 2,
> > + sensor->src->pads, 0);
> > + if (rval < 0)
> > + kfree(sensor);
> > +
> > + return rval;
> > +}
> > +
> > +static int __exit smiapp_remove(struct i2c_client *client)
> > +{
> > + struct v4l2_subdev *subdev = i2c_get_clientdata(client);
> > + struct smiapp_sensor *sensor = to_smiapp_sensor(subdev);
> > + int i;
> > +
> > + if (sensor->power_count) {
> > + if (sensor->platform_data->xshutdown != SMIAPP_NO_XSHUTDOWN)
> > + gpio_set_value(sensor->platform_data->xshutdown, 0);
> > + sensor->platform_data->set_xclk(&sensor->src->sd, 0);
> > + sensor->power_count = 0;
> > + }
> > +
> > + if (sensor->nvm) {
> > + device_remove_file(&client->dev, &dev_attr_nvm);
> > + kfree(sensor->nvm);
> > + }
> > +
> > + for (i = 0; i < sensor->sds_used; i++) {
> > + media_entity_cleanup(&sensor->sds[i].sd.entity);
> > + v4l2_device_unregister_subdev(&sensor->sds[i].sd);
> > + }
> > + smiapp_free_controls(sensor);
> > + if (sensor->platform_data->xshutdown != SMIAPP_NO_XSHUTDOWN)
> > + gpio_free(sensor->platform_data->xshutdown);
> > + if (sensor->vana)
> > + regulator_put(sensor->vana);
> > +
> > + kfree(sensor);
> > +
> > + return 0;
> > +}
> > +
> > +static const struct i2c_device_id smiapp_id_table[] = {
> > + { SMIAPP_NAME, 0 },
> > + { },
> > +};
> > +MODULE_DEVICE_TABLE(i2c, smiapp_id_table);
> > +
> > +static const struct dev_pm_ops smiapp_pm_ops = {
> > + .suspend = smiapp_suspend,
> > + .resume = smiapp_resume,
> > +};
> > +
> > +static struct i2c_driver smiapp_i2c_driver = {
> > + .driver = {
> > + .name = SMIAPP_NAME,
> > + .pm = &smiapp_pm_ops,
> > + },
> > + .probe = smiapp_probe,
> > + .remove = __exit_p(smiapp_remove),
> > + .id_table = smiapp_id_table,
> > +};
> > +
> > +static int __init smiapp_init(void)
> > +{
> > + int rval;
> > +
> > + rval = i2c_add_driver(&smiapp_i2c_driver);
> > + if (rval)
> > + pr_err("Failed registering driver" SMIAPP_NAME "\n");
> > +
> > + return rval;
> > +}
> > +
> > +static void __exit smiapp_exit(void)
> > +{
> > + i2c_del_driver(&smiapp_i2c_driver);
> > +}
> > +
> > +module_init(smiapp_init);
> > +module_exit(smiapp_exit);
> > +
> > +MODULE_AUTHOR("Sakari Ailus <sakari.ailus@xxxxxxxxxxxxxxxxxxxxxxxxxx>");
> > +MODULE_DESCRIPTION("Generic SMIA/SMIA++ camera module driver");
> > +MODULE_LICENSE("GPL");
>
> [snip]
>
> > diff --git a/drivers/media/video/smiapp/smiapp-pll.c
> > b/drivers/media/video/smiapp/smiapp-pll.c new file mode 100644
> > index 0000000..5014730
> > --- /dev/null
> > +++ b/drivers/media/video/smiapp/smiapp-pll.c
>
> [snip]
>
> I'll send a patch on top of this one to split smiapp-pll to a separate module,
> as the code is needed for at least one non-SMIA(++) Aptina sensor.
Ack.
> [snip]
>
> > --git a/drivers/media/video/smiapp/smiapp-reg.h
> > b/drivers/media/video/smiapp/smiapp-reg.h new file mode 100644
> > index 0000000..126ca5b
> > --- /dev/null
> > +++ b/drivers/media/video/smiapp/smiapp-reg.h
>
> [snip]
>
> > +#define SMIAPP_SCALING_CAPABILITY_NONE 0
> > +#define SMIAPP_SCALING_CAPABILITY_HORIZONTAL 1
> > +#define SMIAPP_SCALING_CAPABILITY_BOTH 2 /* horizontal/both */
>
> Do you mean horizontal/vertical ?
No. The BOTH capability means that either horizontal or (horizontal and
vertical) scaling is supported (parentheses for precedence).
> [snip]
>
> > diff --git a/drivers/media/video/smiapp/smiapp-regs.c
> > b/drivers/media/video/smiapp/smiapp-regs.c new file mode 100644
> > index 0000000..9a2326a
> > --- /dev/null
> > +++ b/drivers/media/video/smiapp/smiapp-regs.c
> > @@ -0,0 +1,230 @@
> > +/*
> > + * drivers/media/video/smiapp-regs.c
> > + *
> > + * Generic driver for SMIA/SMIA++ compliant camera modules
> > + *
> > + * Copyright (C) 2011--2012 Nokia Corporation
> > + * Contact: Sakari Ailus <sakari.ailus@xxxxxxxxxxxxxxxxxxxxxxxxxx>
> > + *
> > + * 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.
> > + *
> > + * This program is distributed in the hope that it will be useful, but
> > + * WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> > + * General Public License for more details.
> > + *
> > + * You should have received a copy of the GNU General Public License
> > + * along with this program; if not, write to the Free Software
> > + * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA
> > + * 02110-1301 USA
> > + *
> > + */
> > +
> > +#include "smiapp-debug.h"
> > +
> > +#include <linux/delay.h>
> > +#include <linux/i2c.h>
> > +
> > +#include "smiapp-regs.h"
> > +
> > +static uint32_t float_to_u32_mul_1000000(struct i2c_client *client,
> > + uint32_t phloat)
>
> Now that's creative :-)
I couldn't figure out a way to avoid that, unfortunately. There are a few
corresponding functions in math emulation libraries but it seems onethey
would require significant changes to be usable for this driver.
> > +{
> > + int32_t exp;
> > + uint64_t man;
> > +
> > + if (phloat >= 0x80000000) {
> > + dev_err(&client->dev, "this is a negative number\n");
> > + return 0;
> > + }
> > +
> > + if (phloat == 0x7f800000)
> > + return ~0; /* Inf. */
> > +
> > + if ((phloat & 0x7f800000) == 0x7f800000) {
> > + dev_err(&client->dev, "NaN or other special number\n");
> > + return 0;
> > + }
> > +
> > + /* Valid cases begin here */
> > + if (phloat == 0)
> > + return 0; /* Valid zero */
> > +
> > + if (phloat > 0x4f800000)
> > + return ~0; /* larger than 4294967295 */
> > +
> > + /*
> > + * Unbias exponent (note how phloat is now guaranteed to
> > + * have 0 in the high bit)
> > + */
> > + exp = ((int32_t)phloat >> 23) - 127;
> > +
> > + /* Extract mantissa, add missing '1' bit and it's in MHz */
> > + man = ((phloat & 0x7fffff) | 0x800000) * 1000000ULL;
> > +
> > + if (exp < 0)
> > + man >>= -exp;
> > + else
> > + man <<= exp;
> > +
> > + man >>= 23; /* Remove mantissa bias */
> > +
> > + return man & 0xffffffff;
> > +}
> > +
> > +
> > +/*
> > + * Read a 8/16/32-bit i2c register. The value is returned in 'val'.
> > + * Returns zero if successful, or non-zero otherwise.
> > + */
> > +int smia_i2c_read_reg(struct i2c_client *client, u32 reg, u32 *val)
> > +{
> > + struct i2c_msg msg[1];
>
> s/[1]// ?
Fixed.
> > + unsigned char data[4];
> > + unsigned int len = (u8)(reg >> 16);
> > + u16 offset = reg;
> > + int r;
> > +
> > + if (!client->adapter)
> > + return -ENODEV;
>
> Can this happen ?
I guess not. Fixed.
> > + if (len != SMIA_REG_8BIT && len != SMIA_REG_16BIT
> > + && len != SMIA_REG_32BIT)
> > + return -EINVAL;
> > +
> > + msg->addr = client->addr;
> > + msg->flags = 0;
> > + msg->len = 2;
> > + msg->buf = data;
> > +
> > + /* high byte goes out first */
> > + data[0] = (u8) (offset >> 8);
> > + data[1] = (u8) offset;
> > + r = i2c_transfer(client->adapter, msg, 1);
> > + if (r != 1) {
> > + if (r >= 0)
> > + r = -EBUSY;
> > + goto err;
> > + }
> > +
> > + msg->len = len;
> > + msg->flags = I2C_M_RD;
> > + r = i2c_transfer(client->adapter, msg, 1);
> > + if (r != 1) {
> > + if (r >= 0)
> > + r = -EBUSY;
> > + goto err;
> > + }
> > +
> > + *val = 0;
> > + /* high byte comes first */
> > + switch (len) {
> > + case SMIA_REG_32BIT:
> > + *val = (data[0] << 24) + (data[1] << 16) + (data[2] << 8) +
> > + data[3];
> > + break;
> > + case SMIA_REG_16BIT:
> > + *val = (data[0] << 8) + data[1];
> > + break;
> > + case SMIA_REG_8BIT:
> > + *val = data[0];
> > + break;
> > + default:
> > + BUG();
> > + }
> > +
> > + if (reg & SMIA_REG_FLAG_FLOAT)
> > + *val = float_to_u32_mul_1000000(client, *val);
> > +
> > + return 0;
> > +
> > +err:
> > + dev_err(&client->dev, "read from offset 0x%x error %d\n", offset, r);
> > +
> > + return r;
> > +}
> > +
> > +static void smia_i2c_create_msg(struct i2c_client *client, u16 len, u16
> > reg, + u32 val, struct i2c_msg *msg,
> > + unsigned char *buf)
> > +{
> > + msg->addr = client->addr;
> > + msg->flags = 0; /* Write */
> > + msg->len = 2 + len;
> > + msg->buf = buf;
> > +
> > + /* high byte goes out first */
> > + buf[0] = (u8) (reg >> 8);
> > + buf[1] = (u8) (reg & 0xff);
> > +
> > + switch (len) {
> > + case SMIA_REG_8BIT:
> > + buf[2] = val;
> > + break;
> > + case SMIA_REG_16BIT:
> > + buf[2] = val >> 8;
> > + buf[3] = val;
> > + break;
> > + case SMIA_REG_32BIT:
> > + buf[2] = val >> 24;
> > + buf[3] = val >> 16;
> > + buf[4] = val >> 8;
> > + buf[5] = val;
> > + break;
> > + default:
> > + BUG();
> > + }
>
> As this function is reused anywhere else, I would inline ti inside
> smia_i2c_write_reg().
Done.
> > +}
> > +
> > +/*
> > + * Write to a 8/16-bit register.
> > + * Returns zero if successful, or non-zero otherwise.
> > + */
> > +int smia_i2c_write_reg(struct i2c_client *client, u32 reg, u32 val)
> > +{
> > + struct i2c_msg msg[1];
>
> s/[1]// ?
Fixed.
> > + unsigned char data[6];
> > + unsigned int retries = 5;
> > + unsigned int flags = reg >> 24;
> > + unsigned int len = (u8)(reg >> 16);
> > + u16 offset = reg;
> > + int r;
> > +
> > + if (!client->adapter)
> > + return -ENODEV;
>
> Can this happen ?
Fixed.
> > +
> > + if ((len != SMIA_REG_8BIT && len != SMIA_REG_16BIT &&
> > + len != SMIA_REG_32BIT) || flags)
> > + return -EINVAL;
> > +
> > + smia_i2c_create_msg(client, len, offset, val, msg, data);
> > +
> > + do {
> > + /*
> > + * Due to unknown reason sensor stops responding. This
> > + * loop is a temporaty solution until the root cause
> > + * is found.
> > + */
> > + r = i2c_transfer(client->adapter, msg, 1);
> > + if (r == 1)
> > + break;
> > +
> > + usleep_range(2000, 2000);
> > + } while (retries--);
>
> What about a for loop ?
Fixed.
> > +
> > + if (r != 1) {
> > + dev_err(&client->dev,
> > + "wrote 0x%x to offset 0x%x error %d\n", val, offset, r);
> > + } else {
> > + if (r >= 0)
> > + r = -EBUSY;
> > + r = 0; /* on success i2c_transfer() return messages trasfered */
>
> Was this added at the end of the patch just to see if I would review
> everything ? :-)
No; it comes from the naming of the files I think. I didn't write much of
the code in this function --- it's mostly the same in the Harmattan version,
too.
> > + }
> > +
> > + if (retries < 5)
> > + dev_err(&client->dev, "sensor i2c stall encountered. "
> > + "retries: %d\n", 5 - retries);
>
> You can move this right after the loop and return an error.
I simplified error handling in this function.
> > +
> > + return r;
> > +}
> > diff --git a/drivers/media/video/smiapp/smiapp-regs.h
> > b/drivers/media/video/smiapp/smiapp-regs.h new file mode 100644
> > index 0000000..20c4c25
> > --- /dev/null
> > +++ b/drivers/media/video/smiapp/smiapp-regs.h
> > @@ -0,0 +1,46 @@
> > +/*
> > + * include/media/smiapp-regs.h
> > + *
> > + * Generic driver for SMIA/SMIA++ compliant camera modules
> > + *
> > + * Copyright (C) 2011--2012 Nokia Corporation
> > + * Contact: Sakari Ailus <sakari.ailus@xxxxxxxxxxxxxxxxxxxxxxxxxx>
> > + *
> > + * 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.
> > + *
> > + * This program is distributed in the hope that it will be useful, but
> > + * WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> > + * General Public License for more details.
> > + *
> > + * You should have received a copy of the GNU General Public License
> > + * along with this program; if not, write to the Free Software
> > + * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA
> > + * 02110-1301 USA
> > + *
> > + */
> > +
> > +#ifndef SMIAPP_REGS_H
> > +#define SMIAPP_REGS_H
> > +
> > +#include <linux/i2c.h>
> > +#include <linux/types.h>
> > +
> > +/* Use upper 8 bits of the type field for flags */
> > +#define SMIA_REG_FLAG_FLOAT (1 << 24)
> > +
> > +#define SMIA_REG_8BIT 1
> > +#define SMIA_REG_16BIT 2
> > +#define SMIA_REG_32BIT 4
> > +struct smia_reg {
> > + u16 type;
> > + u16 reg; /* 16-bit offset */
> > + u32 val; /* 8/16/32-bit value */
> > +};
> > +
> > +int smia_i2c_read_reg(struct i2c_client *client, u32 reg, u32 *val);
> > +int smia_i2c_write_reg(struct i2c_client *client, u32 reg, u32 val);
>
> What about renaming those to smia_read() and smia_write() (or possible
> smia_i2c_read() and smia_i2c_write()) ? It would help
> shortening long code lines.
smia_{read,write} sound good.
> > +
> > +#endif
> > diff --git a/drivers/media/video/smiapp/smiapp.h
> > b/drivers/media/video/smiapp/smiapp.h new file mode 100644
> > index 0000000..df514dd
> > --- /dev/null
> > +++ b/drivers/media/video/smiapp/smiapp.h
>
> [snip]
>
> > +struct smiapp_module_ident {
> > + u8 manufacturer_id;
> > + u16 model_id;
> > + u8 revision_number_major;
> > +
> > + u8 flags;
> > +
> > + char *name;
> > + const struct smiapp_quirk *quirk;
> > +} __packed;
>
> Is there really a need to pack this ? You could just move
> revision_number_major above model_id to save a couple of bytes and leave
> packing out.
The order is there for readability, packing to save memory. I can change the
order, too, if you think it's a good idea.
> > +#define SMIAPP_IDENT_FQ(manufacturer, model, rev, fl, _name, _quirk) \
> > + { .manufacturer_id = manufacturer, \
> > + .model_id = model, \
> > + .revision_number_major = rev, \
> > + .flags = fl, \
> > + .name = _name, \
> > + .quirk = _quirk, }
>
> Any reason for the strange indentation ?
This is standard indentation in my Emacsitor. Hmm. I think I might be fine
even if it indented less. It looks like it wouldn't be indended to work that
way.
> > +
> > +#define SMIAPP_IDENT_LQ(manufacturer, model, rev, _name, _quirk) \
> > + { .manufacturer_id = manufacturer, \
> > + .model_id = model, \
> > + .revision_number_major = rev, \
> > + .flags = SMIAPP_MODULE_IDENT_FLAG_REV_LE, \
> > + .name = _name, \
> > + .quirk = _quirk, }
> > +
> > +#define SMIAPP_IDENT_L(manufacturer, model, rev, _name) \
> > + { .manufacturer_id = manufacturer, \
> > + .model_id = model, \
> > + .revision_number_major = rev, \
> > + .flags = SMIAPP_MODULE_IDENT_FLAG_REV_LE, \
> > + .name = _name, }
> > +
> > +#define SMIAPP_IDENT_Q(manufacturer, model, rev, _name, _quirk) \
> > + { .manufacturer_id = manufacturer, \
> > + .model_id = model, \
> > + .revision_number_major = rev, \
> > + .flags = 0, \
> > + .name = _name, \
> > + .quirk = _quirk, }
> > +
> > +#define SMIAPP_IDENT(manufacturer, model, rev, _name) \
> > + { .manufacturer_id = manufacturer, \
> > + .model_id = model, \
> > + .revision_number_major = rev, \
> > + .flags = 0, \
> > + .name = _name, }
> > +
>
> [snip]
>
> > +/*
> > + * struct smiapp_sensor - Main device structure
> > + */
> > +struct smiapp_sensor {
> > + /*
> > + * "mutex" is used to serialise access to all fields here
> > + * except v4l2_ctrls at the end of the struct. Should both
> > + * "mutex" and the control handler locks be held
> > + * simultaneously, the control handler lock must be acquired
> > + * first. "mutex" is also used to serialise access to file
> > + * handle specific information. The exception to this rule is
> > + * the power_mutex below.
> > + */
>
> This comment is probably a bit outdated.
Fixed.
> > + struct mutex mutex;
> > + /*
> > + * power_mutex is used to serialise opening and closing of
> > + * file handles, including power management.
> > + */
> > + struct mutex power_mutex;
> > + struct smiapp_subdev sds[SMIAPP_SUBDEVS];
> > + u32 sds_used;
> > + struct smiapp_subdev *src;
> > + struct smiapp_subdev *binner;
> > + struct smiapp_subdev *scaler;
> > + struct smiapp_subdev *pixel_array;
> > + struct smiapp_platform_data *platform_data;
> > + struct regulator *vana;
> > + u32 limits[SMIAPP_LIMIT_LAST];
> > + u8 nbinning_subtypes;
> > + struct smiapp_binning_subtype binning_subtypes[SMIAPP_BINNING_SUBTYPES];
> > + u32 mbus_frame_fmts;
> > + const struct smiapp_csi_data_format *csi_format;
> > + const struct smiapp_csi_data_format *internal_csi_format;
> > + u32 default_mbus_frame_fmts;
> > + int default_pixel_order;
> > +
> > + u8 binning_horizontal;
> > + u8 binning_vertical;
> > +
> > + u8 scale_m;
> > + u8 scaling_mode;
> > +
> > + u8 hvflip_inv_mask; /* H/VFLIP inversion due to sensor orientation */
> > + u8 flash_capability;
> > + u8 frame_skip;
> > +
> > + int power_count;
> > +
> > + unsigned int streaming:1;
> > + unsigned int dev_init_done:1;
> > +
> > + u8 *nvm; /* nvm memory buffer */
> > + unsigned int nvm_size; /* bytes */
> > +
> > + struct smiapp_module_info minfo;
> > +
> > + struct smiapp_pll pll;
> > +
> > + /* Pixel array controls */
> > + struct v4l2_ctrl *analog_gain;
> > + struct v4l2_ctrl *exposure;
> > + struct v4l2_ctrl *hflip;
> > + struct v4l2_ctrl *vflip;
> > + struct v4l2_ctrl *vblank;
> > + struct v4l2_ctrl *hblank;
> > + struct v4l2_ctrl *pixel_rate_parray;
> > + /* src controls */
> > + struct v4l2_ctrl *link_freq;
> > + struct v4l2_ctrl *pixel_rate_csi;
> > +};
> > +
> > +#define to_smiapp_subdev(_sd) \
> > + container_of(_sd, struct smiapp_subdev, sd)
> > +
> > +#define to_smiapp_sensor(_sd) \
> > + (to_smiapp_subdev(_sd)->sensor)
> > +
> > +int smiapp_pll_update(struct smiapp_sensor *sensor);
> > +int smiapp_pll_configure(struct smiapp_sensor *sensor);
> > +
> > +#endif /* __SMIAPP_PRIV_H_ */
>
> [snip]
Many thanks for your comments in this review, Laurent!
Kind regards,
--
Sakari Ailus
e-mail: sakari.ailus@xxxxxx jabber/XMPP/Gmail: sailus@xxxxxxxxxxxxxx
--
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
