Hi Laurent,
On Tue, Oct 31, 2023 at 01:38:09AM +0200, Laurent Pinchart wrote:
> 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() :-)
I like the idea of moving this to v4l2-cci.
I'd prefer _Generic() based solution as we'd have exact types there instead
of just size. E.g. with the above code, reading a value to a long variable
would work on some archs but fail on others.
--
Regards,
Sakari Ailus
