Hi Sakari,
Thanks for your comments.
On Mon, Oct 30, 2023 at 10:43:47PM +0000, Sakari Ailus wrote:
> Hi Tommaso,
>
> On Mon, Oct 30, 2023 at 01:26:58PM +0100, Tommaso Merciai wrote:
>
> ...
>
> > > > +static int alvium_get_host_supp_csi_lanes(struct alvium_dev *alvium)
> > > > +{
> > > > + u64 val;
> > > > + int ret = 0;
> > > > +
> > > > + alvium_read(alvium, REG_BCRM_CSI2_LANE_COUNT_RW, &val, &ret);
> > >
> > > Missing error checking before the use of the value. The same pattern
> > > remains prevalent throughout the driver.
> > >
> > > I think it'd be easier if you didn't use a temporary variable for reading,
> > > but instead had a register width specific access function. You could even
> > > introduce a helper macro to read this information as I suggested in an
> > > earlier review.
> >
> > oks.
> > We are moving to use the following macros:
> >
> > #define alvium_read_check(alvium, reg, value) \
> > { \
> > int ret = alvium_read(alvium, reg, value, NULL); \
> > if (ret) \
> > return ret; \
> > }
> >
>
> You could do something like (entirely untested):
>
> #define ALVIUM_DECLARE_READ(sign, bits) \
> static int
> alvium_read_ ## sign ## bits(struct alvium_dev *alvium, u32 reg, \
> sign ## bits *val, int *err) \
> { \
> u64 val64; \
> int ret; \
> \
> if (err && *err < 0) \
> return *err; \
> \
> alvium_read(alvium, reg, &val64, &ret); \
> if (ret < 0) { \
> if (err) \
> *err = ret; \
> return ret; \
> } \
> \
> *val = val64; \
> \
> return 0; \
> }
>
> ALVIUM_DECLARE_READ(u, 32);
>
> And then, e.g. instead of (and failing to check ret):
>
> u64 val;
>
> alvium_read(alvium, REG_BCRM_CONTRAST_VALUE_RW, &val, &ret);
> alvium->dft_contrast = val;
>
> you'd have a single call:
>
> alvium_read_u32(alvium, REG_BCRM_CONTRAST_VALUE_RW,
> &alvium->dft_contrast, &ret);
>
> And so on.
>
> You can drop sign if you don't need signed reads but some of the struct
> fields you're writing something appear to be signed.
>
> It'd be good to check the register size matches with the size of *val, too.
> Maybe something like:
>
> WARN_ON((CCI_REG ## bits(0) && CCI_REG_WIDTH_MASK) >> CCI_REG_WIDTH_SHIFT
> != sizeof(sign ## bits));
Laurent suggest me also a good way.
I switched to the Laurent suggested implementation in v11.
I think now is clear. Thanks to both again.
Let me know what do you think about :)
>
> > > > +static int alvium_get_csi_clk_params(struct alvium_dev *alvium)
> > > > +{
> > > > + u64 val;
> > > > + int ret = 0;
> > > > +
> > > > + alvium_read(alvium, REG_BCRM_CSI2_CLOCK_MIN_R, &val, &ret);
> > > > + alvium->min_csi_clk = val;
> > > > +
> > > > + alvium_read(alvium, REG_BCRM_CSI2_CLOCK_MAX_R, &val, &ret);
> > > > + alvium->max_csi_clk = val;
> > > > +
> > > > + return ret;
> > > > +}
> > > > +
> > > > +static int alvium_set_csi_clk(struct alvium_dev *alvium)
> > > > +{
> > > > + struct device *dev = &alvium->i2c_client->dev;
> > > > + u64 csi_clk;
> > > > + int ret;
> > > > +
> > > > + csi_clk = (u32)alvium->ep.link_frequencies[0];
> > >
> > > Why casting to u32? Shouldn't csi_clk be u32 instead?
> >
> > Ok we fix this in v11.
> > Change to use u64 for calculation because type of ep.link_frequencies[0]
> > Plan is to clamp csi_clk between min/max instead of returning error.
>
> I think I would keep it as-is: this isn't V4L2 UAPI.
>
> >
> > >
> > > > +
> > > > + if (csi_clk < alvium->min_csi_clk || csi_clk > alvium->max_csi_clk)
> > > > + return -EINVAL;
> > > > +
> > > > + ret = alvium_write_hshake(alvium, REG_BCRM_CSI2_CLOCK_RW, csi_clk);
> > > > + if (ret) {
> > > > + dev_err(dev, "Fail to set csi lanes reg\n");
> > > > + return ret;
> > > > + }
> > > > +
> > > > + alvium->link_freq = alvium->ep.link_frequencies[0];
> > > > +
> > > > + return 0;
> > > > +}
>
> ...
>
> > > > + goto out;
> > > > +
> > > > + ret = alvium_set_mode(alvium, state);
> > > > + if (ret)
> > > > + goto out;
> > > > +
> > > > + fmt = v4l2_subdev_get_pad_format(sd, state, 0);
> > > > + ret = alvium_set_framefmt(alvium, fmt);
> > > > + if (ret)
> > > > + goto out;
> > > > +
> > > > + ret = alvium_set_stream_mipi(alvium, enable);
> > > > + if (ret)
> > > > + goto out;
> > > > +
> > > > + } else {
> > > > + alvium_set_stream_mipi(alvium, enable);
> > > > + pm_runtime_mark_last_busy(&client->dev);
> > > > + pm_runtime_put_autosuspend(&client->dev);
> > >
> > > pm_runtime_put() here, too.
> >
> > Here is not needed we already have pm_runtime_put_autosuspend.
> > I'm missing something?
>
> Ah, I missed that while reviewing. Please ignore that comment then.
No problem, update in v11.
>
> >
> > >
> > > > + }
> > > > +
> > > > + alvium->streaming = !!enable;
> > > > + v4l2_subdev_unlock_state(state);
> > > > +
> > > > + return 0;
> > > > +
> > > > +out:
> > > > + v4l2_subdev_unlock_state(state);
> > > > + return ret;
> > > > +}
> > > > +
> > > > +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;
> > >
> > > Init_cfg() is expected to be configuration independent (as much as
> > > possible). Therefore you should use defaults here, not current mode.
> >
> > Defaults alvium mode already used here.
>
> Ah, indeed. Please ignore.
No problem.
Thanks & Regards,
Tommaso
>
> --
> Kind regards,
>
> Sakari Ailus
