Hi Tommaso,
On Wed, Nov 22, 2023 at 09:36:21AM +0100, Tommaso Merciai wrote:
> > > > > > > +static int alvium_get_bcrm_vers(struct alvium_dev *alvium)
> > > > > > > +{
> > > > > > > + struct device *dev = &alvium->i2c_client->dev;
> > > > > > > + struct alvium_bcrm_vers *v;
> > > > > > > + u64 val;
> > > > > > > + int ret;
> > > > > > > +
> > > > > > > + ret = alvium_read(alvium, REG_BCRM_VERSION_R, &val, NULL);
> > > > > > > + if (ret)
> > > > > > > + return ret;
> > > > > > > +
> > > > > > > + v = (struct alvium_bcrm_vers *) &val;
> > > > > >
> > > > > > No space before "&" in type cast, please. The same elsewhere.
> > > > > >
> > > > > > As you cast a single value to a struct, I think the struct field values
> > > > > > will be swapped on BE systems. You'll need to convert each value
> > > > > > separately. Same for struct alvium_fw_vers below.
> > > > >
> > > > > What about:
> > > > >
> > > > > v->minor = le16_to_cpu(v->minor);
> > > > > v->major = le16_to_cpu(v->major);
> > > > >
> > > > > here. I posted this solution in some previous v :)
> > > >
> > > > You shouldn't assign it to a field marked little endian. Instead, use
> > > > le16_to_cpu() when you access the data below.
> > > >
> > > > If you want to access the struct in the driver without using the conversion
> > > > macros, you should read the data one field at a time (and use u16 instead
> > > > of __le16 type for the fields).
> > >
> > > It's fine with your suggestion thanks.
> > > I'm moving to use the following to prints those values:
> > >
> > > v = (struct alvium_bcrm_vers *)&val;
> > >
> > > dev_info(dev, "bcrm version: %u.%u\n",
> > > le16_to_cpu(v->minor), le16_to_cpu(v->major));
> > >
> > > thanks for the hint.
> >
> > Sorry, I forgot alvium_read(), via cci_read(), already does endianness
> > conversion here, from big endian to CPU endianness. Is there a need to do
> > further conversion here? Noting that le16_to_cpu() does nothing on little
> > endian systems, is it necessary here?
> >
> > The options here are either changing the struct fields to CPU endianness
> > and reading them individually or accessing the register as a single 32-bit
> > value.
> >
> > I'd do the former, it's easier to access them that way in the driver.
> >
> > The same applies to BCRM version below.
> >
> > struct alvium_bcrm_vers {
> > u16 minor;
> > u16 major;
> > };
>
> Understood, thanks.
>
> Then to resume just compared to v13 I just need to
> revert the alvium_bcrm_vers/alvium_fw_vers struct
> to:
>
> struct alvium_bcrm_vers {
> u16 minor;
> u16 major;
> };
>
> struct alvium_fw_vers {
> u8 special;
> u8 major;
> u16 minor;
> u16 patch;
> };
Yes, and accessing the registers one field at a time.
> > > > > > > +static int alvium_init_cfg(struct v4l2_subdev *sd,
> > > > > > > + struct v4l2_subdev_state *state)
> > > > > > > +{
> > > > > > > + struct alvium_dev *alvium = sd_to_alvium(sd);
> > > > > > > + struct alvium_mode *mode = &alvium->mode;
> > > > > > > + struct v4l2_subdev_format sd_fmt = {
> > > > > > > + .which = V4L2_SUBDEV_FORMAT_TRY,
> > > > > > > + .format = alvium_csi2_default_fmt,
> > > > > > > + };
> > > > > > > + struct v4l2_subdev_crop sd_crop = {
> > > > > > > + .which = V4L2_SUBDEV_FORMAT_TRY,
> > > > > > > + .rect = {
> > > > > > > + .left = mode->crop.left,
> > > > > > > + .top = mode->crop.top,
> > > > > > > + .width = mode->crop.width,
> > > > > > > + .height = mode->crop.height,
> > > > > > > + },
> > > > > > > + };
> > > > > > > +
> > > > > > > + *v4l2_subdev_get_pad_crop(sd, state, 0) = sd_crop.rect;
> > > > > > > + *v4l2_subdev_get_pad_format(sd, state, 0) = sd_fmt.format;
> > > > > >
> > > > > > Shouldn't the format have same width and height as crop? What about the
> > > > > > mbus code?
> > >
> > > Can you clarify to me this comment pls :)
> >
> > The purpose of the init_cfg operation is to initialise the sub-device
> > state, including format and crop rectangle (if applicable). The width and
> > height fields of the format are not initialised above, leaving them zeros.
>
> Mmmmm.
> Why zeros?
>
> .format = alvium_csi2_default_fmt
>
> where:
>
> static const struct v4l2_mbus_framefmt alvium_csi2_default_fmt = {
> .code = MEDIA_BUS_FMT_UYVY8_1X16,
> .width = 640,
> .height = 480,
> .colorspace = V4L2_COLORSPACE_SRGB,
> .ycbcr_enc = V4L2_MAP_YCBCR_ENC_DEFAULT(V4L2_COLORSPACE_SRGB),
> .quantization = V4L2_QUANTIZATION_FULL_RANGE,
> .xfer_func = V4L2_MAP_XFER_FUNC_DEFAULT(V4L2_COLORSPACE_SRGB),
> .field = V4L2_FIELD_NONE,
> };
Ah, I missed this one. I think this part should be fine then.
> > That doesn't seem to be a valid configuration, given that the crop
> > rectangle is initiliased with different values.
>
> Why different values?
> crop is initialized into subdev_init
>
> static int alvium_subdev_init(struct alvium_dev *alvium)
> {
> struct i2c_client *client = alvium->i2c_client;
> struct device *dev = &alvium->i2c_client->dev;
> struct v4l2_subdev *sd = &alvium->sd;
> int ret;
>
> /* Setup initial frame interval*/
> alvium->frame_interval.numerator = 1;
> alvium->frame_interval.denominator = ALVIUM_DEFAULT_FR_HZ;
> alvium->fr = ALVIUM_DEFAULT_FR_HZ;
>
> /* Setup the initial mode */
> alvium->mode.fmt = alvium_csi2_default_fmt;
> alvium->mode.width = alvium_csi2_default_fmt.width;
> alvium->mode.height = alvium_csi2_default_fmt.height;
> alvium->mode.crop.left = alvium->min_offx;
> alvium->mode.crop.top = alvium->min_offy;
> alvium->mode.crop.width = alvium_csi2_default_fmt.width;
> alvium->mode.crop.height = alvium_csi2_default_fmt.height;
> ...
>
> Then:
>
> static int alvium_init_cfg(struct v4l2_subdev *sd,
> struct v4l2_subdev_state *state)
> {
> struct alvium_dev *alvium = sd_to_alvium(sd);
> struct alvium_mode *mode = &alvium->mode;
> struct v4l2_subdev_format sd_fmt = {
> .which = V4L2_SUBDEV_FORMAT_TRY,
> .format = alvium_csi2_default_fmt,
> };
> struct v4l2_subdev_crop sd_crop = {
> .which = V4L2_SUBDEV_FORMAT_TRY,
> .rect = {
> .left = mode->crop.left,
> .top = mode->crop.top,
> .width = mode->crop.width,
> .height = mode->crop.height,
> },
> };
> ...
>
> Seems that crop and format are using the same init values
> or I'm wrong?
> Mmm.. maybe I'm missing something?
>
> Let me know
>
> >
> > >
> > > > > >
> > > > > > > +
> > > > > > > + return 0;
> > > > > > > +}
> > > > > > > +
> > > > > > > +static int alvium_set_fmt(struct v4l2_subdev *sd,
> > > > > > > + struct v4l2_subdev_state *sd_state,
> > > > > > > + struct v4l2_subdev_format *format)
> > > > > > > +{
> > > > > > > + struct alvium_dev *alvium = sd_to_alvium(sd);
> > > > > > > + const struct alvium_pixfmt *alvium_csi2_fmt;
> > > > > > > + struct v4l2_mbus_framefmt *fmt;
> > > > > > > + struct v4l2_rect *crop;
> > > > > > > +
> > > > > > > + fmt = v4l2_subdev_get_pad_format(sd, sd_state, 0);
> > > > > > > + crop = v4l2_subdev_get_pad_crop(sd, sd_state, 0);
> > > > > > > +
> > > > > > > + fmt->width = clamp(format->format.width, alvium->img_min_width,
> > > > > > > + alvium->img_max_width);
> > > > > > > + fmt->height = clamp(format->format.height, alvium->img_min_height,
> > > > > > > + alvium->img_max_height);
> > > > > > > +
> > > > > > > + /* Adjust left and top to prevent roll over sensor area */
> > > > > > > + crop->left = clamp((u32)crop->left, (u32)0,
> > > > > > > + (alvium->img_max_width - fmt->width));
> > > > > > > + crop->top = clamp((u32)crop->top, (u32)0,
> > > > > > > + (alvium->img_max_height - fmt->height));
> > > > > > > +
> > > > > > > + /* Set also the crop width and height when set a new fmt */
> > > > > > > + crop->width = fmt->width;
> > > > > > > + crop->height = fmt->height;
> > > > > > > +
> > > > > > > + alvium_csi2_fmt = alvium_code_to_pixfmt(alvium, format->format.code);
> > > > > > > + fmt->code = alvium_csi2_fmt->code;
> > > > > > > +
> > > > > > > + *fmt = format->format;
> > > > > > > +
> > > > > > > + return 0;
> > > > > > > +}
> > > > > > > +
> > > > > > > +static int alvium_set_selection(struct v4l2_subdev *sd,
> > > > > > > + struct v4l2_subdev_state *sd_state,
> > > > > > > + struct v4l2_subdev_selection *sel)
> > > > > > > +{
> > > > > > > + struct alvium_dev *alvium = sd_to_alvium(sd);
> > > > > > > + struct v4l2_mbus_framefmt *fmt;
> > > > > > > + struct v4l2_rect *crop;
> > > > > > > +
> > > > > > > + crop = v4l2_subdev_get_pad_crop(sd, sd_state, 0);
> > > > > > > + fmt = v4l2_subdev_get_pad_format(sd, sd_state, 0);
> > > > > > > +
> > > > > > > + /*
> > > > > > > + * Alvium can only shift the origin of the img
> > > > > > > + * then we accept only value with the same value of the actual fmt
> > > > > > > + */
> > > > > > > + if (sel->r.width != fmt->width)
> > > > > > > + sel->r.width = fmt->width;
> > > > > > > +
> > > > > > > + if (sel->r.height != fmt->height)
> > > > > > > + sel->r.height = fmt->height;
> > > > > > > +
> > > > > > > + if (sel->target != V4L2_SEL_TGT_CROP)
> > > > > > > + return -EINVAL;
> > > > > >
> > > > > > This should be the first thing to test.
> > > > >
> > > > > Oks
> > > > >
> > > > > >
> > > > > > > +
> > > > > > > + /* alvium don't accept negative crop left/top */
> > > > > > > + crop->left = clamp((u32)max(0, sel->r.left), alvium->min_offx,
> > > > > > > + alvium->img_max_width - sel->r.width);
> > > > > > > + crop->top = clamp((u32)max(0, sel->r.top), alvium->min_offy,
> > > > > > > + alvium->img_max_height - sel->r.height);
> > > > > > > +
> > > > > > > + sel->r = *crop;
> > > > > > > +
> > > > > > > + return 0;
> > > > > > > +}
> > > > > > > +
> > > > > > > +static int alvium_get_selection(struct v4l2_subdev *sd,
> > > > > > > + struct v4l2_subdev_state *sd_state,
> > > > > > > + struct v4l2_subdev_selection *sel)
> > > > > > > +{
> > > > > > > + struct alvium_dev *alvium = sd_to_alvium(sd);
> > > > > > > +
> > > > > > > + switch (sel->target) {
> > > > > > > + /* Current cropping area */
> > > > > > > + case V4L2_SEL_TGT_CROP:
> > > > > > > + sel->r = *v4l2_subdev_get_pad_crop(sd, sd_state, 0);
> > > > > > > + break;
> > > > > > > + /* Cropping bounds */
> > > > > > > + case V4L2_SEL_TGT_NATIVE_SIZE:
> > > > > > > + sel->r.top = 0;
> > > > > > > + sel->r.left = 0;
> > > > > > > + sel->r.width = alvium->img_max_width;
> > > > > > > + sel->r.height = alvium->img_max_height;
> > > > > > > + break;
> > > > > > > + /* Default cropping area */
> > > > > > > + case V4L2_SEL_TGT_CROP_BOUNDS:
> > > > > > > + case V4L2_SEL_TGT_CROP_DEFAULT:
> > > > > > > + sel->r.top = alvium->min_offy;
> > > > > > > + sel->r.left = alvium->min_offx;
> > > > > > > + sel->r.width = alvium->img_max_width;
> > > > > > > + sel->r.height = alvium->img_max_height;
> > > > > > > + break;
> > > > > > > + default:
> > > > > > > + return -EINVAL;
> > > > > > > + }
> > > > > > > +
> > > > > > > + return 0;
> > > > > > > +}
> > > > > > > +
> > > > > > > +static int alvium_g_volatile_ctrl(struct v4l2_ctrl *ctrl)
> > > > > > > +{
> > > > > > > + struct v4l2_subdev *sd = ctrl_to_sd(ctrl);
> > > > > > > + struct alvium_dev *alvium = sd_to_alvium(sd);
> > > > > > > + int val;
> > > > > > > +
> > > > > > > + switch (ctrl->id) {
> > > > > > > + case V4L2_CID_GAIN:
> > > > > > > + val = alvium_get_gain(alvium);
> > > > > > > + if (val < 0)
> > > > > > > + return val;
> > > > > > > + alvium->ctrls.gain->val = val;
> > > > > > > + break;
> > > > > > > + case V4L2_CID_EXPOSURE:
> > > > > > > + val = alvium_get_exposure(alvium);
> > > > > > > + if (val < 0)
> > > > > > > + return val;
> > > > > > > + alvium->ctrls.exposure->val = val;
> > > > > > > + break;
> > > > > > > + }
> > > > > > > +
> > > > > > > + return 0;
> > > > > > > +}
> > > > > > > +
> > > > > > > +static int alvium_s_ctrl(struct v4l2_ctrl *ctrl)
> > > > > > > +{
> > > > > > > + struct v4l2_subdev *sd = ctrl_to_sd(ctrl);
> > > > > > > + struct alvium_dev *alvium = sd_to_alvium(sd);
> > > > > > > + struct i2c_client *client = v4l2_get_subdevdata(&alvium->sd);
> > > > > > > + int ret;
> > > > > > > +
> > > > > > > + /*
> > > > > > > + * Applying V4L2 control value only happens
> > > > > > > + * when power is up for streaming
> > > > > > > + */
> > > > > > > + if (!pm_runtime_get_if_in_use(&client->dev))
> > > > > > > + return 0;
> > > > > > > +
> > > > > > > + switch (ctrl->id) {
> > > > > > > + case V4L2_CID_AUTOGAIN:
> > > > > > > + ret = alvium_set_ctrl_gain(alvium, ctrl->val);
> > > > > >
> > > > > > ret = alvium_set_autogain(alvium, ctrl->val);
> > > > > >
> > > > >
> > > > > Pls check [1]
> > > > >
> > > > > > Where do you set the manual gain value? What about the manual exposure
> > > > > > value? Both appear to be missing here.
> > > > > >
> > > > > > How have you tested this?
> > > > > >
> > > > > > > + break;
> > > > > > > + case V4L2_CID_EXPOSURE_AUTO:
> > > > > > > + ret = alvium_set_ctrl_exposure(alvium, ctrl->val);
> > > > > >
> > > > > > ret = alvium_set_autoexposure(alvium, ctrl->val);
> > > > > >
> > > > > > You're still missing grabbing the manual controls when the corresponding
> > > > > > automatic control is enabled. I've commented on the same matter previously.
> > > > >
> > > > > Same comment in [1]
> > > > >
> > > > > >
> > > > > > > + break;
> > > > > > > + case V4L2_CID_AUTO_WHITE_BALANCE:
> > > > > > > + ret = alvium_set_ctrl_white_balance(alvium, ctrl->val);
> > > > > > > + break;
> > > > > > > + case V4L2_CID_HUE:
> > > > > > > + ret = alvium_set_ctrl_hue(alvium, ctrl->val);
> > > > > > > + break;
> > > > > > > + case V4L2_CID_CONTRAST:
> > > > > > > + ret = alvium_set_ctrl_contrast(alvium, ctrl->val);
> > > > > > > + break;
> > > > > > > + case V4L2_CID_SATURATION:
> > > > > > > + ret = alvium_set_ctrl_saturation(alvium, ctrl->val);
> > > > > > > + break;
> > > > > > > + case V4L2_CID_GAMMA:
> > > > > > > + ret = alvium_set_ctrl_gamma(alvium, ctrl->val);
> > > > > > > + break;
> > > > > > > + case V4L2_CID_SHARPNESS:
> > > > > > > + ret = alvium_set_ctrl_sharpness(alvium, ctrl->val);
> > > > > > > + break;
> > > > > > > + case V4L2_CID_HFLIP:
> > > > > > > + ret = alvium_set_ctrl_hflip(alvium, ctrl->val);
> > > > > > > + break;
> > > > > > > + case V4L2_CID_VFLIP:
> > > > > > > + ret = alvium_set_ctrl_vflip(alvium, ctrl->val);
> > > > > > > + break;
> > > > > > > + default:
> > > > > > > + ret = -EINVAL;
> > > > > > > + break;
> > > > > > > + }
> > > > > > > +
> > > > > > > + pm_runtime_put(&client->dev);
> > > > > > > +
> > > > > > > + return ret;
> > > > > > > +}
>
> Here I plan to switch to:
>
> static int alvium_s_ctrl(struct v4l2_ctrl *ctrl)
> {
> struct v4l2_subdev *sd = ctrl_to_sd(ctrl);
> struct alvium_dev *alvium = sd_to_alvium(sd);
> struct i2c_client *client = v4l2_get_subdevdata(&alvium->sd);
> int ret;
>
> /*
> * Applying V4L2 control value only happens
> * when power is up for streaming
> */
> if (!pm_runtime_get_if_in_use(&client->dev))
> return 0;
>
> switch (ctrl->id) {
> case V4L2_CID_GAIN:
> ret = alvium_set_ctrl_gain(alvium, ctrl->val);
> break;
> case V4L2_CID_AUTOGAIN:
> ret = alvium_set_ctrl_auto_gain(alvium, ctrl->val);
> break;
> case V4L2_CID_EXPOSURE:
> ret = alvium_set_ctrl_exposure(alvium, ctrl->val);
> break;
> case V4L2_CID_EXPOSURE_AUTO:
> ret = alvium_set_ctrl_auto_exposure(alvium, ctrl->val);
> break;
> case V4L2_CID_RED_BALANCE:
> ret = alvium_set_ctrl_red_balance_ratio(alvium, ctrl->val);
> break;
> case V4L2_CID_BLUE_BALANCE:
> ret = alvium_set_ctrl_blue_balance_ratio(alvium, ctrl->val);
> break;
> case V4L2_CID_AUTO_WHITE_BALANCE:
> ret = alvium_set_ctrl_awb(alvium, ctrl->val);
> break;
> case V4L2_CID_HUE:
> ret = alvium_set_ctrl_hue(alvium, ctrl->val);
> break;
> case V4L2_CID_CONTRAST:
> ret = alvium_set_ctrl_contrast(alvium, ctrl->val);
> break;
> case V4L2_CID_SATURATION:
> ret = alvium_set_ctrl_saturation(alvium, ctrl->val);
> break;
> case V4L2_CID_GAMMA:
> ret = alvium_set_ctrl_gamma(alvium, ctrl->val);
> break;
> case V4L2_CID_SHARPNESS:
> ret = alvium_set_ctrl_sharpness(alvium, ctrl->val);
> break;
> case V4L2_CID_HFLIP:
> ret = alvium_set_ctrl_hflip(alvium, ctrl->val);
> break;
> case V4L2_CID_VFLIP:
> ret = alvium_set_ctrl_vflip(alvium, ctrl->val);
> break;
> default:
> ret = -EINVAL;
> break;
> }
>
> pm_runtime_put(&client->dev);
>
> return ret;
> }
>
> Like you suggested.
> I think is more clean/understandable.
> Thanks for your comment.
Ack.
--
Regards,
Sakari Ailus
