| Message ID | 20231113134458.1423754-6-sakari.ailus@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Use V4L2 CCI in CCS driver | expand |
Hi Sakari, Thank you for the patch. On Mon, Nov 13, 2023 at 03:44:57PM +0200, Sakari Ailus wrote: > Separate CCS static data read-only register access in ccs-reg-access.c by > naming them differently. "naming the access functions differently" > The code in this file generally deals with reading and writing registers > where as static data (when it comes to ccs_static_read_only()) contains > the read-only register values but no hardware registers are accessed in > that case. > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> > --- > .../driver-api/media/drivers/ccs/mk-ccs-regs | 2 +- > drivers/media/i2c/ccs/ccs-reg-access.c | 20 +++++++++---------- > 2 files changed, 11 insertions(+), 11 deletions(-) > > diff --git a/Documentation/driver-api/media/drivers/ccs/mk-ccs-regs b/Documentation/driver-api/media/drivers/ccs/mk-ccs-regs > index 01252ee6062b..3d3152b45821 100755 > --- a/Documentation/driver-api/media/drivers/ccs/mk-ccs-regs > +++ b/Documentation/driver-api/media/drivers/ccs/mk-ccs-regs > @@ -136,7 +136,7 @@ if (! defined $kernel) { > > print $H "#define CCS_FL_FLOAT_IREAL " . flag_str(\$flag, \$all_flags) . "\n"; > print $H "#define CCS_FL_IREAL " . flag_str(\$flag, \$all_flags) . "\n"; > -print $H "#define CCS_BUILD_BUG \ > +print $H "#define CCS_BUILD_BUG \\ This doesn't seem related. > BUILD_BUG_ON(~CCI_REG_PRIVATE_MASK & ($all_flags))\n" > if defined $kernel; > > diff --git a/drivers/media/i2c/ccs/ccs-reg-access.c b/drivers/media/i2c/ccs/ccs-reg-access.c > index 25993445f4fe..652d705a2ef5 100644 > --- a/drivers/media/i2c/ccs/ccs-reg-access.c > +++ b/drivers/media/i2c/ccs/ccs-reg-access.c > @@ -197,8 +197,8 @@ static int __ccs_read_addr(struct ccs_sensor *sensor, u32 reg, u32 *val, > return 0; > } > > -static int __ccs_read_data(struct ccs_reg *regs, size_t num_regs, > - u32 reg, u32 *val) > +static int __ccs_static_read_only(struct ccs_reg *regs, size_t num_regs, "static read only" sounds weird when interpreting "read" as a verb. And if "read" is not a verb, you're missing a verb :-) Maybe __ccs_read_state_data() would be a better name ? Same below. > + u32 reg, u32 *val) > { > unsigned int width = ccs_reg_width(reg); > size_t i; > @@ -235,16 +235,16 @@ static int __ccs_read_data(struct ccs_reg *regs, size_t num_regs, > return -ENOENT; > } > > -static int ccs_read_data(struct ccs_sensor *sensor, u32 reg, u32 *val) > +static int ccs_static_read_only(struct ccs_sensor *sensor, u32 reg, u32 *val) > { > - if (!__ccs_read_data(sensor->sdata.sensor_read_only_regs, > - sensor->sdata.num_sensor_read_only_regs, > - reg, val)) > + if (!__ccs_static_read_only(sensor->sdata.sensor_read_only_regs, > + sensor->sdata.num_sensor_read_only_regs, > + reg, val)) > return 0; > > - return __ccs_read_data(sensor->mdata.module_read_only_regs, > - sensor->mdata.num_module_read_only_regs, > - reg, val); > + return __ccs_static_read_only(sensor->mdata.module_read_only_regs, > + sensor->mdata.num_module_read_only_regs, > + reg, val); > } > > static int ccs_read_addr_raw(struct ccs_sensor *sensor, u32 reg, u32 *val, > @@ -253,7 +253,7 @@ static int ccs_read_addr_raw(struct ccs_sensor *sensor, u32 reg, u32 *val, > int rval; > > if (data) { > - rval = ccs_read_data(sensor, reg, val); > + rval = ccs_static_read_only(sensor, reg, val); > if (!rval) > return 0; > }
On Mon, Nov 13, 2023 at 03:57:10PM +0200, Laurent Pinchart wrote: > Hi Sakari, > > Thank you for the patch. > > On Mon, Nov 13, 2023 at 03:44:57PM +0200, Sakari Ailus wrote: > > Separate CCS static data read-only register access in ccs-reg-access.c by > > naming them differently. > > "naming the access functions differently" > > > The code in this file generally deals with reading and writing registers > > where as static data (when it comes to ccs_static_read_only()) contains > > the read-only register values but no hardware registers are accessed in > > that case. > > > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> > > --- > > .../driver-api/media/drivers/ccs/mk-ccs-regs | 2 +- > > drivers/media/i2c/ccs/ccs-reg-access.c | 20 +++++++++---------- > > 2 files changed, 11 insertions(+), 11 deletions(-) > > > > diff --git a/Documentation/driver-api/media/drivers/ccs/mk-ccs-regs b/Documentation/driver-api/media/drivers/ccs/mk-ccs-regs > > index 01252ee6062b..3d3152b45821 100755 > > --- a/Documentation/driver-api/media/drivers/ccs/mk-ccs-regs > > +++ b/Documentation/driver-api/media/drivers/ccs/mk-ccs-regs > > @@ -136,7 +136,7 @@ if (! defined $kernel) { > > > > print $H "#define CCS_FL_FLOAT_IREAL " . flag_str(\$flag, \$all_flags) . "\n"; > > print $H "#define CCS_FL_IREAL " . flag_str(\$flag, \$all_flags) . "\n"; > > -print $H "#define CCS_BUILD_BUG \ > > +print $H "#define CCS_BUILD_BUG \\ > > This doesn't seem related. Oops. This was meant to go to the previous patch. I'll address this in v3... > > > BUILD_BUG_ON(~CCI_REG_PRIVATE_MASK & ($all_flags))\n" > > if defined $kernel; > > > > diff --git a/drivers/media/i2c/ccs/ccs-reg-access.c b/drivers/media/i2c/ccs/ccs-reg-access.c > > index 25993445f4fe..652d705a2ef5 100644 > > --- a/drivers/media/i2c/ccs/ccs-reg-access.c > > +++ b/drivers/media/i2c/ccs/ccs-reg-access.c > > @@ -197,8 +197,8 @@ static int __ccs_read_addr(struct ccs_sensor *sensor, u32 reg, u32 *val, > > return 0; > > } > > > > -static int __ccs_read_data(struct ccs_reg *regs, size_t num_regs, > > - u32 reg, u32 *val) > > +static int __ccs_static_read_only(struct ccs_reg *regs, size_t num_regs, > > "static read only" sounds weird when interpreting "read" as a verb. And > if "read" is not a verb, you're missing a verb :-) Maybe > __ccs_read_state_data() would be a better name ? Same below. Why state_data? I prefer to use terms used in the spec, not come up with new ones. There are also other blocks in static data beseides read-only register values. How about ccs_static_data_read_only_reg(), would that be more informative in your opinion? > > > + u32 reg, u32 *val) > > { > > unsigned int width = ccs_reg_width(reg); > > size_t i; > > @@ -235,16 +235,16 @@ static int __ccs_read_data(struct ccs_reg *regs, size_t num_regs, > > return -ENOENT; > > } > > > > -static int ccs_read_data(struct ccs_sensor *sensor, u32 reg, u32 *val) > > +static int ccs_static_read_only(struct ccs_sensor *sensor, u32 reg, u32 *val) > > { > > - if (!__ccs_read_data(sensor->sdata.sensor_read_only_regs, > > - sensor->sdata.num_sensor_read_only_regs, > > - reg, val)) > > + if (!__ccs_static_read_only(sensor->sdata.sensor_read_only_regs, > > + sensor->sdata.num_sensor_read_only_regs, > > + reg, val)) > > return 0; > > > > - return __ccs_read_data(sensor->mdata.module_read_only_regs, > > - sensor->mdata.num_module_read_only_regs, > > - reg, val); > > + return __ccs_static_read_only(sensor->mdata.module_read_only_regs, > > + sensor->mdata.num_module_read_only_regs, > > + reg, val); > > } > > > > static int ccs_read_addr_raw(struct ccs_sensor *sensor, u32 reg, u32 *val, > > @@ -253,7 +253,7 @@ static int ccs_read_addr_raw(struct ccs_sensor *sensor, u32 reg, u32 *val, > > int rval; > > > > if (data) { > > - rval = ccs_read_data(sensor, reg, val); > > + rval = ccs_static_read_only(sensor, reg, val); > > if (!rval) > > return 0; > > } >
On Mon, Nov 13, 2023 at 02:08:49PM +0000, Sakari Ailus wrote: > On Mon, Nov 13, 2023 at 03:57:10PM +0200, Laurent Pinchart wrote: > > Hi Sakari, > > > > Thank you for the patch. > > > > On Mon, Nov 13, 2023 at 03:44:57PM +0200, Sakari Ailus wrote: > > > Separate CCS static data read-only register access in ccs-reg-access.c by > > > naming them differently. > > > > "naming the access functions differently" > > > > > The code in this file generally deals with reading and writing registers > > > where as static data (when it comes to ccs_static_read_only()) contains > > > the read-only register values but no hardware registers are accessed in > > > that case. > > > > > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> > > > --- > > > .../driver-api/media/drivers/ccs/mk-ccs-regs | 2 +- > > > drivers/media/i2c/ccs/ccs-reg-access.c | 20 +++++++++---------- > > > 2 files changed, 11 insertions(+), 11 deletions(-) > > > > > > diff --git a/Documentation/driver-api/media/drivers/ccs/mk-ccs-regs b/Documentation/driver-api/media/drivers/ccs/mk-ccs-regs > > > index 01252ee6062b..3d3152b45821 100755 > > > --- a/Documentation/driver-api/media/drivers/ccs/mk-ccs-regs > > > +++ b/Documentation/driver-api/media/drivers/ccs/mk-ccs-regs > > > @@ -136,7 +136,7 @@ if (! defined $kernel) { > > > > > > print $H "#define CCS_FL_FLOAT_IREAL " . flag_str(\$flag, \$all_flags) . "\n"; > > > print $H "#define CCS_FL_IREAL " . flag_str(\$flag, \$all_flags) . "\n"; > > > -print $H "#define CCS_BUILD_BUG \ > > > +print $H "#define CCS_BUILD_BUG \\ > > > > This doesn't seem related. > > Oops. This was meant to go to the previous patch. I'll address this in > v3... > > > > > > BUILD_BUG_ON(~CCI_REG_PRIVATE_MASK & ($all_flags))\n" > > > if defined $kernel; > > > > > > diff --git a/drivers/media/i2c/ccs/ccs-reg-access.c b/drivers/media/i2c/ccs/ccs-reg-access.c > > > index 25993445f4fe..652d705a2ef5 100644 > > > --- a/drivers/media/i2c/ccs/ccs-reg-access.c > > > +++ b/drivers/media/i2c/ccs/ccs-reg-access.c > > > @@ -197,8 +197,8 @@ static int __ccs_read_addr(struct ccs_sensor *sensor, u32 reg, u32 *val, > > > return 0; > > > } > > > > > > -static int __ccs_read_data(struct ccs_reg *regs, size_t num_regs, > > > - u32 reg, u32 *val) > > > +static int __ccs_static_read_only(struct ccs_reg *regs, size_t num_regs, > > > > "static read only" sounds weird when interpreting "read" as a verb. And > > if "read" is not a verb, you're missing a verb :-) Maybe > > __ccs_read_state_data() would be a better name ? Same below. > > Why state_data? I prefer to use terms used in the spec, not come up with > new ones. Sorry, I mean __ccs_read_static_data(). I'm working on a patch related to V4L2 subdev state at the moment :-) > There are also other blocks in static data beseides read-only register > values. > > How about ccs_static_data_read_only_reg(), would that be more informative > in your opinion? ccs_static_data_read_ro_reg() would work for me. I interpret ccs_static_data_read_only_reg() as "read just the register and nothing else". > > > + u32 reg, u32 *val) > > > { > > > unsigned int width = ccs_reg_width(reg); > > > size_t i; > > > @@ -235,16 +235,16 @@ static int __ccs_read_data(struct ccs_reg *regs, size_t num_regs, > > > return -ENOENT; > > > } > > > > > > -static int ccs_read_data(struct ccs_sensor *sensor, u32 reg, u32 *val) > > > +static int ccs_static_read_only(struct ccs_sensor *sensor, u32 reg, u32 *val) > > > { > > > - if (!__ccs_read_data(sensor->sdata.sensor_read_only_regs, > > > - sensor->sdata.num_sensor_read_only_regs, > > > - reg, val)) > > > + if (!__ccs_static_read_only(sensor->sdata.sensor_read_only_regs, > > > + sensor->sdata.num_sensor_read_only_regs, > > > + reg, val)) > > > return 0; > > > > > > - return __ccs_read_data(sensor->mdata.module_read_only_regs, > > > - sensor->mdata.num_module_read_only_regs, > > > - reg, val); > > > + return __ccs_static_read_only(sensor->mdata.module_read_only_regs, > > > + sensor->mdata.num_module_read_only_regs, > > > + reg, val); > > > } > > > > > > static int ccs_read_addr_raw(struct ccs_sensor *sensor, u32 reg, u32 *val, > > > @@ -253,7 +253,7 @@ static int ccs_read_addr_raw(struct ccs_sensor *sensor, u32 reg, u32 *val, > > > int rval; > > > > > > if (data) { > > > - rval = ccs_read_data(sensor, reg, val); > > > + rval = ccs_static_read_only(sensor, reg, val); > > > if (!rval) > > > return 0; > > > }
On Mon, Nov 13, 2023 at 04:16:18PM +0200, Laurent Pinchart wrote: > On Mon, Nov 13, 2023 at 02:08:49PM +0000, Sakari Ailus wrote: > > On Mon, Nov 13, 2023 at 03:57:10PM +0200, Laurent Pinchart wrote: > > > Hi Sakari, > > > > > > Thank you for the patch. > > > > > > On Mon, Nov 13, 2023 at 03:44:57PM +0200, Sakari Ailus wrote: > > > > Separate CCS static data read-only register access in ccs-reg-access.c by > > > > naming them differently. > > > > > > "naming the access functions differently" > > > > > > > The code in this file generally deals with reading and writing registers > > > > where as static data (when it comes to ccs_static_read_only()) contains > > > > the read-only register values but no hardware registers are accessed in > > > > that case. > > > > > > > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> > > > > --- > > > > .../driver-api/media/drivers/ccs/mk-ccs-regs | 2 +- > > > > drivers/media/i2c/ccs/ccs-reg-access.c | 20 +++++++++---------- > > > > 2 files changed, 11 insertions(+), 11 deletions(-) > > > > > > > > diff --git a/Documentation/driver-api/media/drivers/ccs/mk-ccs-regs b/Documentation/driver-api/media/drivers/ccs/mk-ccs-regs > > > > index 01252ee6062b..3d3152b45821 100755 > > > > --- a/Documentation/driver-api/media/drivers/ccs/mk-ccs-regs > > > > +++ b/Documentation/driver-api/media/drivers/ccs/mk-ccs-regs > > > > @@ -136,7 +136,7 @@ if (! defined $kernel) { > > > > > > > > print $H "#define CCS_FL_FLOAT_IREAL " . flag_str(\$flag, \$all_flags) . "\n"; > > > > print $H "#define CCS_FL_IREAL " . flag_str(\$flag, \$all_flags) . "\n"; > > > > -print $H "#define CCS_BUILD_BUG \ > > > > +print $H "#define CCS_BUILD_BUG \\ > > > > > > This doesn't seem related. > > > > Oops. This was meant to go to the previous patch. I'll address this in > > v3... > > > > > > > > > BUILD_BUG_ON(~CCI_REG_PRIVATE_MASK & ($all_flags))\n" > > > > if defined $kernel; > > > > > > > > diff --git a/drivers/media/i2c/ccs/ccs-reg-access.c b/drivers/media/i2c/ccs/ccs-reg-access.c > > > > index 25993445f4fe..652d705a2ef5 100644 > > > > --- a/drivers/media/i2c/ccs/ccs-reg-access.c > > > > +++ b/drivers/media/i2c/ccs/ccs-reg-access.c > > > > @@ -197,8 +197,8 @@ static int __ccs_read_addr(struct ccs_sensor *sensor, u32 reg, u32 *val, > > > > return 0; > > > > } > > > > > > > > -static int __ccs_read_data(struct ccs_reg *regs, size_t num_regs, > > > > - u32 reg, u32 *val) > > > > +static int __ccs_static_read_only(struct ccs_reg *regs, size_t num_regs, > > > > > > "static read only" sounds weird when interpreting "read" as a verb. And > > > if "read" is not a verb, you're missing a verb :-) Maybe > > > __ccs_read_state_data() would be a better name ? Same below. > > > > Why state_data? I prefer to use terms used in the spec, not come up with > > new ones. > > Sorry, I mean __ccs_read_static_data(). I'm working on a patch related > to V4L2 subdev state at the moment :-) > > > There are also other blocks in static data beseides read-only register > > values. > > > > How about ccs_static_data_read_only_reg(), would that be more informative > > in your opinion? > > ccs_static_data_read_ro_reg() would work for me. I interpret > ccs_static_data_read_only_reg() as "read just the register and nothing > else". ccs_static_data_read_ro_reg() seems reasonable, I'll use it in v3. There will be more similarly named functions in the future.
diff --git a/Documentation/driver-api/media/drivers/ccs/mk-ccs-regs b/Documentation/driver-api/media/drivers/ccs/mk-ccs-regs index 01252ee6062b..3d3152b45821 100755 --- a/Documentation/driver-api/media/drivers/ccs/mk-ccs-regs +++ b/Documentation/driver-api/media/drivers/ccs/mk-ccs-regs @@ -136,7 +136,7 @@ if (! defined $kernel) { print $H "#define CCS_FL_FLOAT_IREAL " . flag_str(\$flag, \$all_flags) . "\n"; print $H "#define CCS_FL_IREAL " . flag_str(\$flag, \$all_flags) . "\n"; -print $H "#define CCS_BUILD_BUG \ +print $H "#define CCS_BUILD_BUG \\ BUILD_BUG_ON(~CCI_REG_PRIVATE_MASK & ($all_flags))\n" if defined $kernel; diff --git a/drivers/media/i2c/ccs/ccs-reg-access.c b/drivers/media/i2c/ccs/ccs-reg-access.c index 25993445f4fe..652d705a2ef5 100644 --- a/drivers/media/i2c/ccs/ccs-reg-access.c +++ b/drivers/media/i2c/ccs/ccs-reg-access.c @@ -197,8 +197,8 @@ static int __ccs_read_addr(struct ccs_sensor *sensor, u32 reg, u32 *val, return 0; } -static int __ccs_read_data(struct ccs_reg *regs, size_t num_regs, - u32 reg, u32 *val) +static int __ccs_static_read_only(struct ccs_reg *regs, size_t num_regs, + u32 reg, u32 *val) { unsigned int width = ccs_reg_width(reg); size_t i; @@ -235,16 +235,16 @@ static int __ccs_read_data(struct ccs_reg *regs, size_t num_regs, return -ENOENT; } -static int ccs_read_data(struct ccs_sensor *sensor, u32 reg, u32 *val) +static int ccs_static_read_only(struct ccs_sensor *sensor, u32 reg, u32 *val) { - if (!__ccs_read_data(sensor->sdata.sensor_read_only_regs, - sensor->sdata.num_sensor_read_only_regs, - reg, val)) + if (!__ccs_static_read_only(sensor->sdata.sensor_read_only_regs, + sensor->sdata.num_sensor_read_only_regs, + reg, val)) return 0; - return __ccs_read_data(sensor->mdata.module_read_only_regs, - sensor->mdata.num_module_read_only_regs, - reg, val); + return __ccs_static_read_only(sensor->mdata.module_read_only_regs, + sensor->mdata.num_module_read_only_regs, + reg, val); } static int ccs_read_addr_raw(struct ccs_sensor *sensor, u32 reg, u32 *val, @@ -253,7 +253,7 @@ static int ccs_read_addr_raw(struct ccs_sensor *sensor, u32 reg, u32 *val, int rval; if (data) { - rval = ccs_read_data(sensor, reg, val); + rval = ccs_static_read_only(sensor, reg, val); if (!rval) return 0; }
Separate CCS static data read-only register access in ccs-reg-access.c by naming them differently. The code in this file generally deals with reading and writing registers where as static data (when it comes to ccs_static_read_only()) contains the read-only register values but no hardware registers are accessed in that case. Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> --- .../driver-api/media/drivers/ccs/mk-ccs-regs | 2 +- drivers/media/i2c/ccs/ccs-reg-access.c | 20 +++++++++---------- 2 files changed, 11 insertions(+), 11 deletions(-)