Message ID | 20241224-veml3235_scale-v2-3-2e1286846c77@gmail.com (mailing list archive) |
---|---|
State | Accepted |
Headers | show |
Series | iio: light: fix scale in veml3235 and add helpers to iio-gts | expand |
On Tue, 24 Dec 2024 11:59:02 +0100 Javier Carrasco <javier.carrasco.cruz@gmail.com> wrote: > The configuration and ID registers are not volatile and are not affected > by read operations (i.e. not precious), making them suitable to be > cached in order to reduce the number of accesses to the device. > > Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com> This is also fine on it's own. Applied. > --- > drivers/iio/light/veml3235.c | 31 +++++++++++++++++++++++++++++++ > 1 file changed, 31 insertions(+) > > diff --git a/drivers/iio/light/veml3235.c b/drivers/iio/light/veml3235.c > index fa5c7e7dfbfaec1b96428612b1dcba91ea51603f..f754980ea156a6e128ff159b816e09099197c5c7 100644 > --- a/drivers/iio/light/veml3235.c > +++ b/drivers/iio/light/veml3235.c > @@ -101,12 +101,43 @@ static const struct iio_chan_spec veml3235_channels[] = { > }, > }; > > +static const struct regmap_range veml3235_readable_ranges[] = { > + regmap_reg_range(VEML3235_REG_CONF, VEML3235_REG_ID), > +}; > + > +static const struct regmap_access_table veml3235_readable_table = { > + .yes_ranges = veml3235_readable_ranges, > + .n_yes_ranges = ARRAY_SIZE(veml3235_readable_ranges), > +}; > + > +static const struct regmap_range veml3235_writable_ranges[] = { > + regmap_reg_range(VEML3235_REG_CONF, VEML3235_REG_CONF), > +}; > + > +static const struct regmap_access_table veml3235_writable_table = { > + .yes_ranges = veml3235_writable_ranges, > + .n_yes_ranges = ARRAY_SIZE(veml3235_writable_ranges), > +}; > + > +static const struct regmap_range veml3235_volatile_ranges[] = { > + regmap_reg_range(VEML3235_REG_WH_DATA, VEML3235_REG_ALS_DATA), > +}; > + > +static const struct regmap_access_table veml3235_volatile_table = { > + .yes_ranges = veml3235_volatile_ranges, > + .n_yes_ranges = ARRAY_SIZE(veml3235_volatile_ranges), > +}; > + > static const struct regmap_config veml3235_regmap_config = { > .name = "veml3235_regmap", > .reg_bits = 8, > .val_bits = 16, > .max_register = VEML3235_REG_ID, > .val_format_endian = REGMAP_ENDIAN_LITTLE, > + .rd_table = &veml3235_readable_table, > + .wr_table = &veml3235_writable_table, > + .volatile_table = &veml3235_volatile_table, > + .cache_type = REGCACHE_RBTREE, > }; > > static int veml3235_get_it(struct veml3235_data *data, int *val, int *val2) >
Tue, Dec 24, 2024 at 11:59:02AM +0100, Javier Carrasco kirjoitti: > The configuration and ID registers are not volatile and are not affected > by read operations (i.e. not precious), making them suitable to be > cached in order to reduce the number of accesses to the device. ... > static const struct regmap_config veml3235_regmap_config = { > .name = "veml3235_regmap", > .reg_bits = 8, > .val_bits = 16, > .max_register = VEML3235_REG_ID, > .val_format_endian = REGMAP_ENDIAN_LITTLE, > + .rd_table = &veml3235_readable_table, > + .wr_table = &veml3235_writable_table, > + .volatile_table = &veml3235_volatile_table, > + .cache_type = REGCACHE_RBTREE, Any specific reason why this is NOT a maple tree? > };
On Sun Jan 12, 2025 at 4:18 PM CET, Andy Shevchenko wrote: > Tue, Dec 24, 2024 at 11:59:02AM +0100, Javier Carrasco kirjoitti: > > The configuration and ID registers are not volatile and are not affected > > by read operations (i.e. not precious), making them suitable to be > > cached in order to reduce the number of accesses to the device. > > ... > > > static const struct regmap_config veml3235_regmap_config = { > > .name = "veml3235_regmap", > > .reg_bits = 8, > > .val_bits = 16, > > .max_register = VEML3235_REG_ID, > > .val_format_endian = REGMAP_ENDIAN_LITTLE, > > + .rd_table = &veml3235_readable_table, > > + .wr_table = &veml3235_writable_table, > > + .volatile_table = &veml3235_volatile_table, > > + .cache_type = REGCACHE_RBTREE, > > Any specific reason why this is NOT a maple tree? > Hello Andy, I followed the most common approach in IIO (52 RBTREE vs 2 MAPLE), assuming that the "low-end systems" comment for the different REGCACHE_* applies well to the typical systems that will make use of this driver, and many others under IIO. I considered that *possible* performance advantage for low-end systems above other considerations, like the general rule about using maple tree. Thanks and best regards, Javier Carrasco
On Sun, Jan 12, 2025 at 6:07 PM Javier Carrasco <javier.carrasco.cruz@gmail.com> wrote: > > On Sun Jan 12, 2025 at 4:18 PM CET, Andy Shevchenko wrote: > > Tue, Dec 24, 2024 at 11:59:02AM +0100, Javier Carrasco kirjoitti: > > > The configuration and ID registers are not volatile and are not affected > > > by read operations (i.e. not precious), making them suitable to be > > > cached in order to reduce the number of accesses to the device. > > > > ... > > > > > static const struct regmap_config veml3235_regmap_config = { > > > .name = "veml3235_regmap", > > > .reg_bits = 8, > > > .val_bits = 16, > > > .max_register = VEML3235_REG_ID, > > > .val_format_endian = REGMAP_ENDIAN_LITTLE, > > > + .rd_table = &veml3235_readable_table, > > > + .wr_table = &veml3235_writable_table, > > > + .volatile_table = &veml3235_volatile_table, > > > + .cache_type = REGCACHE_RBTREE, > > > > Any specific reason why this is NOT a maple tree? > > Hello Andy, > > I followed the most common approach in IIO (52 RBTREE vs 2 MAPLE), But it's historical and can't be taken as an argument. > assuming that the "low-end systems" comment for the different REGCACHE_* > applies well to the typical systems that will make use of this driver, > and many others under IIO. I considered that *possible* performance > advantage for low-end systems above other considerations, like the > general rule about using maple tree. https://elixir.bootlin.com/linux/v6.13-rc3/source/include/linux/regmap.h#L58 "Any new caches * should usually use the maple tree cache unless they specifically * require that there are never any allocations at runtime and can't * provide defaults in which case they should use the flat cache." Can you reconsider?
On Sun Jan 12, 2025 at 5:11 PM CET, Andy Shevchenko wrote: > On Sun, Jan 12, 2025 at 6:07 PM Javier Carrasco > <javier.carrasco.cruz@gmail.com> wrote: > > > > On Sun Jan 12, 2025 at 4:18 PM CET, Andy Shevchenko wrote: > > > Tue, Dec 24, 2024 at 11:59:02AM +0100, Javier Carrasco kirjoitti: > > > > The configuration and ID registers are not volatile and are not affected > > > > by read operations (i.e. not precious), making them suitable to be > > > > cached in order to reduce the number of accesses to the device. > > > > > > ... > > > > > > > static const struct regmap_config veml3235_regmap_config = { > > > > .name = "veml3235_regmap", > > > > .reg_bits = 8, > > > > .val_bits = 16, > > > > .max_register = VEML3235_REG_ID, > > > > .val_format_endian = REGMAP_ENDIAN_LITTLE, > > > > + .rd_table = &veml3235_readable_table, > > > > + .wr_table = &veml3235_writable_table, > > > > + .volatile_table = &veml3235_volatile_table, > > > > + .cache_type = REGCACHE_RBTREE, > > > > > > Any specific reason why this is NOT a maple tree? > > > > Hello Andy, > > > > I followed the most common approach in IIO (52 RBTREE vs 2 MAPLE), > > But it's historical and can't be taken as an argument. > > > assuming that the "low-end systems" comment for the different REGCACHE_* > > applies well to the typical systems that will make use of this driver, > > and many others under IIO. I considered that *possible* performance > > advantage for low-end systems above other considerations, like the > > general rule about using maple tree. > > https://elixir.bootlin.com/linux/v6.13-rc3/source/include/linux/regmap.h#L58 > > "Any new caches > * should usually use the maple tree cache unless they specifically > * require that there are never any allocations at runtime and can't > * provide defaults in which case they should use the flat cache." > > Can you reconsider? That was exactly the comment I referenced, actually the part about low-end systems that appears right after what you highlighted. I have nothing against switching to MAPLE, if that is preferred even if the main user of this driver will be a low-end system. I think that IIO is a typical subsystem that addresses needs for very low-end systems that are sometimes slightly more powerful than a microcontroller, but on the other hand I am by no means an expert, and if MAPLE is the way to go here as well, I will send a follow-up patch for it. Thank you for your feedback! Javier Carrasco
On Sun, Jan 12, 2025 at 6:21 PM Javier Carrasco <javier.carrasco.cruz@gmail.com> wrote: > On Sun Jan 12, 2025 at 5:11 PM CET, Andy Shevchenko wrote: > > On Sun, Jan 12, 2025 at 6:07 PM Javier Carrasco > > <javier.carrasco.cruz@gmail.com> wrote: > > > On Sun Jan 12, 2025 at 4:18 PM CET, Andy Shevchenko wrote: > > > > Tue, Dec 24, 2024 at 11:59:02AM +0100, Javier Carrasco kirjoitti: > > > > > The configuration and ID registers are not volatile and are not affected > > > > > by read operations (i.e. not precious), making them suitable to be > > > > > cached in order to reduce the number of accesses to the device. ... > > > > > + .cache_type = REGCACHE_RBTREE, > > > > > > > > Any specific reason why this is NOT a maple tree? > > > > > > I followed the most common approach in IIO (52 RBTREE vs 2 MAPLE), > > > > But it's historical and can't be taken as an argument. > > > > > assuming that the "low-end systems" comment for the different REGCACHE_* > > > applies well to the typical systems that will make use of this driver, > > > and many others under IIO. I considered that *possible* performance > > > advantage for low-end systems above other considerations, like the > > > general rule about using maple tree. > > > > https://elixir.bootlin.com/linux/v6.13-rc3/source/include/linux/regmap.h#L58 > > > > "Any new caches > > * should usually use the maple tree cache unless they specifically > > * require that there are never any allocations at runtime and can't > > * provide defaults in which case they should use the flat cache." > > > > Can you reconsider? > > That was exactly the comment I referenced, actually the part about > low-end systems that appears right after what you highlighted. > > I have nothing against switching to MAPLE, if that is preferred even if > the main user of this driver will be a low-end system. I think that IIO > is a typical subsystem that addresses needs for very low-end systems > that are sometimes slightly more powerful than a microcontroller, but on > the other hand I am by no means an expert, and if MAPLE is the way to go > here as well, I will send a follow-up patch for it. Ah, I see now. Okay, I leave it then to Jonathan as I am okay with any choice as long as it's understood and justified.
On Sun, 12 Jan 2025 20:50:41 +0200 Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > On Sun, Jan 12, 2025 at 6:21 PM Javier Carrasco > <javier.carrasco.cruz@gmail.com> wrote: > > On Sun Jan 12, 2025 at 5:11 PM CET, Andy Shevchenko wrote: > > > On Sun, Jan 12, 2025 at 6:07 PM Javier Carrasco > > > <javier.carrasco.cruz@gmail.com> wrote: > > > > On Sun Jan 12, 2025 at 4:18 PM CET, Andy Shevchenko wrote: > > > > > Tue, Dec 24, 2024 at 11:59:02AM +0100, Javier Carrasco kirjoitti: > > > > > > The configuration and ID registers are not volatile and are not affected > > > > > > by read operations (i.e. not precious), making them suitable to be > > > > > > cached in order to reduce the number of accesses to the device. > > ... > > > > > > > + .cache_type = REGCACHE_RBTREE, > > > > > > > > > > Any specific reason why this is NOT a maple tree? > > > > > > > > I followed the most common approach in IIO (52 RBTREE vs 2 MAPLE), > > > > > > But it's historical and can't be taken as an argument. > > > > > > > assuming that the "low-end systems" comment for the different REGCACHE_* > > > > applies well to the typical systems that will make use of this driver, > > > > and many others under IIO. I considered that *possible* performance > > > > advantage for low-end systems above other considerations, like the > > > > general rule about using maple tree. > > > > > > https://elixir.bootlin.com/linux/v6.13-rc3/source/include/linux/regmap.h#L58 > > > > > > "Any new caches > > > * should usually use the maple tree cache unless they specifically > > > * require that there are never any allocations at runtime and can't > > > * provide defaults in which case they should use the flat cache." > > > > > > Can you reconsider? > > > > That was exactly the comment I referenced, actually the part about > > low-end systems that appears right after what you highlighted. > > > > I have nothing against switching to MAPLE, if that is preferred even if > > the main user of this driver will be a low-end system. I think that IIO > > is a typical subsystem that addresses needs for very low-end systems > > that are sometimes slightly more powerful than a microcontroller, but on > > the other hand I am by no means an expert, and if MAPLE is the way to go > > here as well, I will send a follow-up patch for it. > > Ah, I see now. Okay, I leave it then to Jonathan as I am okay with any > choice as long as it's understood and justified. For a fairly small regmap, I doubt there is a strong reason to care about the choice. So stick to what you have. For future drivers we can reassess as makes sense. Jonathan > >
diff --git a/drivers/iio/light/veml3235.c b/drivers/iio/light/veml3235.c index fa5c7e7dfbfaec1b96428612b1dcba91ea51603f..f754980ea156a6e128ff159b816e09099197c5c7 100644 --- a/drivers/iio/light/veml3235.c +++ b/drivers/iio/light/veml3235.c @@ -101,12 +101,43 @@ static const struct iio_chan_spec veml3235_channels[] = { }, }; +static const struct regmap_range veml3235_readable_ranges[] = { + regmap_reg_range(VEML3235_REG_CONF, VEML3235_REG_ID), +}; + +static const struct regmap_access_table veml3235_readable_table = { + .yes_ranges = veml3235_readable_ranges, + .n_yes_ranges = ARRAY_SIZE(veml3235_readable_ranges), +}; + +static const struct regmap_range veml3235_writable_ranges[] = { + regmap_reg_range(VEML3235_REG_CONF, VEML3235_REG_CONF), +}; + +static const struct regmap_access_table veml3235_writable_table = { + .yes_ranges = veml3235_writable_ranges, + .n_yes_ranges = ARRAY_SIZE(veml3235_writable_ranges), +}; + +static const struct regmap_range veml3235_volatile_ranges[] = { + regmap_reg_range(VEML3235_REG_WH_DATA, VEML3235_REG_ALS_DATA), +}; + +static const struct regmap_access_table veml3235_volatile_table = { + .yes_ranges = veml3235_volatile_ranges, + .n_yes_ranges = ARRAY_SIZE(veml3235_volatile_ranges), +}; + static const struct regmap_config veml3235_regmap_config = { .name = "veml3235_regmap", .reg_bits = 8, .val_bits = 16, .max_register = VEML3235_REG_ID, .val_format_endian = REGMAP_ENDIAN_LITTLE, + .rd_table = &veml3235_readable_table, + .wr_table = &veml3235_writable_table, + .volatile_table = &veml3235_volatile_table, + .cache_type = REGCACHE_RBTREE, }; static int veml3235_get_it(struct veml3235_data *data, int *val, int *val2)
The configuration and ID registers are not volatile and are not affected by read operations (i.e. not precious), making them suitable to be cached in order to reduce the number of accesses to the device. Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com> --- drivers/iio/light/veml3235.c | 31 +++++++++++++++++++++++++++++++ 1 file changed, 31 insertions(+)