mbox series

[v3,0/6] Use V4L2 CCI in CCS driver

Message ID 20231113160601.1427972-1-sakari.ailus@linux.intel.com
Headers show
Series Use V4L2 CCI in CCS driver | expand

Message

Sakari Ailus Nov. 13, 2023, 4:05 p.m. UTC 
Hi folks,

This set adds a few features to the V4L2 CCI library and makes the CCS
driver use V4L2 CCI.

The additional features are about storing driver specific information in
the CCI registers (besides register address) and obtaining register width
using a convenient V4L2 CCI macro.

since v2:

- Rename CCS static data read-only register access function.

- Fix ccs-regs.h generation to include all changes in the 4th patch (some
  had slipped into the 5th).

since v1:

- Fix two SMIAPP register definitions using misspelled CCI macro names.

- Add macros using FIELD_GET() to obtain CCI register address and width,
  use the macros in V4L2 CCI.

- Use _SHIFT and _MASK for private register range.

- Check CCS driver's private flags only cover driver-private bits of the
  CCI register definition, using  BUILD_BUG_ON().

- Fix CCS CCI register macro generation (register address vs. flag vs.
  array indices).

- Use a nicer way to check for the guardian value in the limit array, i.e.
  don't pass the value unconditionally to CCI_REG_WIDTH_BYTES().

- Include linux/bits.h and media/v4l2-cci.h in smiapp-reg-defs.h.

- Improve commit message of the  CCS static data register access function
  rename patch.

Sakari Ailus (6):
  media: v4l: cci: Include linux/bits.h
  media: v4l: cci: Add driver-private bit definitions
  media: v4l: cci: Add macros to obtain register width and address
  media: ccs: Generate V4L2 CCI compliant register definitions
  media: ccs: Better separate CCS static data access
  media: ccs: Use V4L2 CCI for accessing sensor registers

 .../driver-api/media/drivers/ccs/mk-ccs-regs  | 104 +-
 drivers/media/i2c/ccs/ccs-core.c              |  84 +-
 drivers/media/i2c/ccs/ccs-reg-access.c        | 213 +---
 drivers/media/i2c/ccs/ccs-regs.h              | 906 ++++++++---------
 drivers/media/i2c/ccs/ccs.h                   |   2 +
 drivers/media/i2c/ccs/smiapp-reg-defs.h       | 951 +++++++++---------
 drivers/media/v4l2-core/v4l2-cci.c            |   8 +-
 include/media/v4l2-cci.h                      |  11 +
 8 files changed, 1100 insertions(+), 1179 deletions(-)


base-commit: 62bdf633090d684c3ac6d3b46e926c0ac8cef466

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))
Laurent Pinchart Nov. 13, 2023, 4:21 p.m. UTC | #2 
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))