Hello,
Hans, there's a question for you below.
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));
I think this could actually be automated, and implemented in v4l2-cci.
Something like the following:
diff --git a/drivers/media/v4l2-core/v4l2-cci.c b/drivers/media/v4l2-core/v4l2-cci.c
index bc2dbec019b0..27f1eaa7777d 100644
--- a/drivers/media/v4l2-core/v4l2-cci.c
+++ b/drivers/media/v4l2-core/v4l2-cci.c
@@ -16,7 +16,7 @@
#include <media/v4l2-cci.h>
-int cci_read(struct regmap *map, u32 reg, u64 *val, int *err)
+int __cci_read(struct regmap *map, u32 reg, void *val, int *err)
{
unsigned int len;
u8 buf[8];
@@ -37,19 +37,19 @@ int cci_read(struct regmap *map, u32 reg, u64 *val, int *err)
switch (len) {
case 1:
- *val = buf[0];
+ *(u8 *)val = buf[0];
break;
case 2:
- *val = get_unaligned_be16(buf);
+ *(u16 *)val = get_unaligned_be16(buf);
break;
case 3:
- *val = get_unaligned_be24(buf);
+ *(u32 *)val = get_unaligned_be24(buf);
break;
case 4:
- *val = get_unaligned_be32(buf);
+ *(u32 *)val = get_unaligned_be32(buf);
break;
case 8:
- *val = get_unaligned_be64(buf);
+ *(u64 *)val = get_unaligned_be64(buf);
break;
default:
dev_err(regmap_get_device(map), "Error invalid reg-width %u for reg 0x%04x\n",
@@ -64,7 +64,7 @@ int cci_read(struct regmap *map, u32 reg, u64 *val, int *err)
return ret;
}
-EXPORT_SYMBOL_GPL(cci_read);
+EXPORT_SYMBOL_GPL(__cci_read);
int cci_write(struct regmap *map, u32 reg, u64 val, int *err)
{
@@ -119,7 +119,7 @@ int cci_update_bits(struct regmap *map, u32 reg, u64 mask, u64 val, int *err)
u64 readval;
int ret;
- ret = cci_read(map, reg, &readval, err);
+ ret = __cci_read(map, reg, &readval, err);
if (ret)
return ret;
diff --git a/include/media/v4l2-cci.h b/include/media/v4l2-cci.h
index 0f6803e4b17e..31223ce8d741 100644
--- a/include/media/v4l2-cci.h
+++ b/include/media/v4l2-cci.h
@@ -7,6 +7,9 @@
#ifndef _V4L2_CCI_H
#define _V4L2_CCI_H
+#include <linux/bitfield.h>
+#include <linux/build_bug.h>
+#include <linux/log2.h>
#include <linux/types.h>
struct i2c_client;
@@ -39,6 +42,8 @@ struct cci_reg_sequence {
#define CCI_REG32(x) ((4 << CCI_REG_WIDTH_SHIFT) | (x))
#define CCI_REG64(x) ((8 << CCI_REG_WIDTH_SHIFT) | (x))
+int __cci_read(struct regmap *map, u32 reg, void *val, int *err);
+
/**
* cci_read() - Read a value from a single CCI register
*
@@ -48,9 +53,17 @@ struct cci_reg_sequence {
* @err: Optional pointer to store errors, if a previous error is set
* then the read will be skipped
*
+ * The type of the @val pointer must match the size of the register being read.
+ * Mismatches will result in compile-time errors.
+ *
* Return: %0 on success or a negative error code on failure.
*/
-int cci_read(struct regmap *map, u32 reg, u64 *val, int *err);
+#define cci_read(map, reg, val, err) ({ \
+ u32 __reg = (reg); \
+ u32 __size = FIELD_GET(CCI_REG_WIDTH_MASK, __reg); \
+ BUILD_BUG_ON(sizeof(*(val)) != roundup_pow_of_two(__size)); \
+ __cci_read(map, __reg, (void *)(val), err); \
+})
/**
* cci_write() - Write a value to a single CCI register
The change to cci_update_bits() is obviously wrong, I've hacked that to
compile-test the rest with the drivers using cci_read(), and I get nice
build-time errors due to usage of the wrong type :-)
Is this something that would be considered ? Bonus points to anyone who
would fix cci_update_bits() :-)
> > > > +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.
>
> > > > + }
> > > > +
> > > > + 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.
--
Regards,
Laurent Pinchart
