Hi Sakari,
Thank you for the patch.
On Mon, Nov 13, 2023 at 06:05:58PM +0200, Sakari Ailus wrote:
> Add CCI_REG_WIDTH() macro to obtain register width in bits and similarly,
> CCI_REG_WIDTH_BYTES() to obtain it in bytes.
>
> Also add CCI_REG_ADDR() macro to obtain the address of a register.
>
> Use both macros in v4l2-cci.c, too.
>
> Signed-off-by: Sakari Ailus <sakari.ailus@xxxxxxxxxxxxxxx>
> Reviewed-by: Hans de Goede <hdegoede@xxxxxxxxxx>
Please write per-patch changelogs. You would then have likely caught the
issue below.
> ---
> drivers/media/v4l2-core/v4l2-cci.c | 8 ++++----
> include/media/v4l2-cci.h | 9 +++++++--
> 2 files changed, 11 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/media/v4l2-core/v4l2-cci.c b/drivers/media/v4l2-core/v4l2-cci.c
> index bc2dbec019b0..3179160abde3 100644
> --- a/drivers/media/v4l2-core/v4l2-cci.c
> +++ b/drivers/media/v4l2-core/v4l2-cci.c
> @@ -25,8 +25,8 @@ int cci_read(struct regmap *map, u32 reg, u64 *val, int *err)
> if (err && *err)
> return *err;
>
> - len = FIELD_GET(CCI_REG_WIDTH_MASK, reg);
> - reg = FIELD_GET(CCI_REG_ADDR_MASK, reg);
> + len = CCI_REG_WIDTH_BYTES(reg);
> + reg = CCI_REG_ADDR(reg);
>
> ret = regmap_bulk_read(map, reg, buf, len);
> if (ret) {
> @@ -75,8 +75,8 @@ int cci_write(struct regmap *map, u32 reg, u64 val, int *err)
> if (err && *err)
> return *err;
>
> - len = FIELD_GET(CCI_REG_WIDTH_MASK, reg);
> - reg = FIELD_GET(CCI_REG_ADDR_MASK, reg);
> + len = CCI_REG_WIDTH_BYTES(reg);
> + reg = CCI_REG_ADDR(reg);
>
> switch (len) {
> case 1:
> diff --git a/include/media/v4l2-cci.h b/include/media/v4l2-cci.h
> index ee469f03e440..50df3aa4af1d 100644
> --- a/include/media/v4l2-cci.h
> +++ b/include/media/v4l2-cci.h
> @@ -7,6 +7,7 @@
> #ifndef _V4L2_CCI_H
> #define _V4L2_CCI_H
>
> +#include <linux/bitfield.h>
> #include <linux/bits.h>
> #include <linux/types.h>
>
> @@ -36,8 +37,12 @@ struct cci_reg_sequence {
> /*
> * Private CCI register flags, for the use of drivers.
> */
> -#define CCI_REG_PRIVATE_SHIFT 28U
> -#define CCI_REG_PRIVATE_MASK GENMASK(31U, CCI_REG_PRIVATE_SHIFT)
> +#define CCI_REG_PRIVATE_SHIFT 28
> +#define CCI_REG_PRIVATE_MASK GENMASK(31, CCI_REG_PRIVATE_SHIFT)
Was this meant to be in the previous patch ?
> +
> +#define CCI_REG_WIDTH_BYTES(x) FIELD_GET(CCI_REG_WIDTH_MASK, x)
> +#define CCI_REG_WIDTH(x) (CCI_REG_WIDTH_BYTES(x) << 3)
> +#define CCI_REG_ADDR(x) FIELD_GET(CCI_REG_ADDR_MASK, x)
>
> #define CCI_REG8(x) ((1 << CCI_REG_WIDTH_SHIFT) | (x))
> #define CCI_REG16(x) ((2 << CCI_REG_WIDTH_SHIFT) | (x))
--
Regards,
Laurent Pinchart
