diff mbox series

[v3,3/6] media: v4l: cci: Add macros to obtain register width and address

Message ID 20231113160601.1427972-4-sakari.ailus@linux.intel.com (mailing list archive)
State New
Headers show
Series Use V4L2 CCI in CCS driver | expand

Commit Message

Sakari Ailus Nov. 13, 2023, 4:05 p.m. UTC 
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@linux.intel.com>
Reviewed-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/media/v4l2-core/v4l2-cci.c | 8 ++++----
 include/media/v4l2-cci.h           | 9 +++++++--
 2 files changed, 11 insertions(+), 6 deletions(-)

Comments

Laurent Pinchart Nov. 13, 2023, 4:11 p.m. UTC | #1 
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@linux.intel.com>
> Reviewed-by: Hans de Goede <hdegoede@redhat.com>

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))
Sakari Ailus Nov. 13, 2023, 4:18 p.m. UTC | #2 
Hi Laurent,

On Mon, Nov 13, 2023 at 06:11:38PM +0200, Laurent Pinchart wrote:
> 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@linux.intel.com>
> > Reviewed-by: Hans de Goede <hdegoede@redhat.com>
> 
> Please write per-patch changelogs. You would then have likely caught the
> issue below.

That forces the reviewer to read all the patches, I'm not convinced it
would have made any difference here.

> 
> > ---
> >  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 ?

Yes. But this was actually there in v2 already, and missed in review. I'll
fix in v4.

> 
> > +
> > +#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))
>
Laurent Pinchart Nov. 13, 2023, 4:21 p.m. UTC | #3 
On Mon, Nov 13, 2023 at 04:18:28PM +0000, Sakari Ailus wrote:
> On Mon, Nov 13, 2023 at 06:11:38PM +0200, Laurent Pinchart wrote:
> > 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@linux.intel.com>
> > > Reviewed-by: Hans de Goede <hdegoede@redhat.com>
> > 
> > Please write per-patch changelogs. You would then have likely caught the
> > issue below.
> 
> That forces the reviewer to read all the patches, I'm not convinced it
> would have made any difference here.

It makes review easier, as reviewers can then read each patch in
isolation and have the full context.

> > > ---
> > >  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 ?
> 
> Yes. But this was actually there in v2 already, and missed in review. I'll
> fix in v4.
> 
> > > +
> > > +#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))
diff mbox series

Patch

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)
+
+#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))