Re: [PATCH v2] nvmem: regmap: Fix nvmem size
Hello Robin,
On 21.12.23 15:02, Ahmad Fatoum wrote:
> On 20.12.23 13:38, Robin van der Gracht wrote:
>> Hi Ahmad,
>>
>> On 2023-12-20 10:00, Ahmad Fatoum wrote:
>>> Hello Robin,
>>>
>>> Thanks for the fix.
>>>
>>> On 20.12.23 09:29, Robin van der Gracht wrote:
- if (roffset + rbytes > stride * regmap_get_max_register(map))
+ if (roffset + rbytes > regmap_size_bytes(map) * stride)
>>>
>>> Shouldn't stride on the right hand side be dropped?
>>
>> roffset = register index * stride.
>>
>> I.e. 380 for register with index 95.
>>
>> For stm32mp1x bsec:
>> map->format.val_bytes = 4
>> map->reg_stride = 4
>>
>> regmap_size_bytes() = map->format.val_bytes * (95 + 1) / map->reg_stride = 96
>
> Ouch. What have I been thinking when I named regmap_size_bytes() this way...
> Change looks ok then, but we should really rename regmap_size_bytes()...
After talking it over with Sascha, I see that the name is correct with regard
to how the function was initially used. What's wrong it the implementation and
that goes back to 2016..
I just sent out a series fixing this. md -s /dev/stm32-bsec 380 now works.
Please verify if it's behaving as you expect.
Thanks,
Ahmad
>
>>
>> So the result with the stride on the right size is correct.
>>
>> I moved stride from left to right to be consistent with the size calculation
>> in nvmem_regmap_register_with_pp()
>>
>> Kind regards,
>> Robin
>>
>>
>>>
>>> Cheers,
>>> Ahmad
>>>
return -EINVAL;
for (i = roffset; i < roffset + rbytes; i += stride) {
@@ -78,7 +78,7 @@ nvmem_regmap_register_with_pp(struct regmap *map, const
char *name,
config.priv = map;
config.stride = 1;
config.word_size = 1;
- config.size = regmap_get_max_register(map) *
regmap_get_reg_stride(map);
+ config.size = regmap_size_bytes(map) * regmap_get_reg_stride(map);
config.cell_post_process = cell_post_process;
config.reg_write = nvmem_regmap_write;
config.reg_read = nvmem_regmap_read;
>>
>
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0|
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
Re: [PATCH v2] nvmem: regmap: Fix nvmem size
On Tue, Jan 02, 2024 at 11:31:43AM +0100, Sascha Hauer wrote: > On Wed, Dec 20, 2023 at 09:29:08AM +0100, Robin van der Gracht wrote: > > We should add 1 to the max_register index since counting is zero based. > > > > i.e. the stm32mp151 bsec has registers 0 - 95 with reg_stride 4. > > Size should be (95 + 1) * 4 = 384 bytes otherwise we can't access bsec > > register 95 (last one). > > > > regmap_size_bytes() does take the +1 into account so we can use that. > > > > Signed-off-by: Robin van der Gracht > > --- > > > > v2: Fix the size calculation in nvmem_regmap_read() as well. > > > > drivers/nvmem/regmap.c | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > Applied, thanks And un-applied again, see Ahmads patch Sascha -- Pengutronix e.K. | | Steuerwalder Str. 21 | http://www.pengutronix.de/ | 31137 Hildesheim, Germany | Phone: +49-5121-206917-0| Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
Re: [PATCH v2] nvmem: regmap: Fix nvmem size
On Wed, Dec 20, 2023 at 09:29:08AM +0100, Robin van der Gracht wrote:
> We should add 1 to the max_register index since counting is zero based.
>
> i.e. the stm32mp151 bsec has registers 0 - 95 with reg_stride 4.
> Size should be (95 + 1) * 4 = 384 bytes otherwise we can't access bsec
> register 95 (last one).
>
> regmap_size_bytes() does take the +1 into account so we can use that.
>
> Signed-off-by: Robin van der Gracht
> ---
>
> v2: Fix the size calculation in nvmem_regmap_read() as well.
>
> drivers/nvmem/regmap.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
Applied, thanks
Sascha
>
> diff --git a/drivers/nvmem/regmap.c b/drivers/nvmem/regmap.c
> index fa5405d7a8..b923c5787d 100644
> --- a/drivers/nvmem/regmap.c
> +++ b/drivers/nvmem/regmap.c
> @@ -38,7 +38,7 @@ static int nvmem_regmap_read(void *ctx, unsigned offset,
> void *buf, size_t bytes
> skip_bytes = offset & (stride - 1);
> rbytes = roundup(bytes + skip_bytes, stride);
>
> - if (roffset + rbytes > stride * regmap_get_max_register(map))
> + if (roffset + rbytes > regmap_size_bytes(map) * stride)
> return -EINVAL;
>
> for (i = roffset; i < roffset + rbytes; i += stride) {
> @@ -78,7 +78,7 @@ nvmem_regmap_register_with_pp(struct regmap *map, const
> char *name,
> config.priv = map;
> config.stride = 1;
> config.word_size = 1;
> - config.size = regmap_get_max_register(map) * regmap_get_reg_stride(map);
> + config.size = regmap_size_bytes(map) * regmap_get_reg_stride(map);
> config.cell_post_process = cell_post_process;
> config.reg_write = nvmem_regmap_write;
> config.reg_read = nvmem_regmap_read;
> --
> 2.40.1
>
>
>
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0|
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
Re: [PATCH v2] nvmem: regmap: Fix nvmem size
On 20.12.23 13:38, Robin van der Gracht wrote:
> Hi Ahmad,
>
> On 2023-12-20 10:00, Ahmad Fatoum wrote:
>> Hello Robin,
>>
>> Thanks for the fix.
>>
>> On 20.12.23 09:29, Robin van der Gracht wrote:
>>> - if (roffset + rbytes > stride * regmap_get_max_register(map))
>>> + if (roffset + rbytes > regmap_size_bytes(map) * stride)
>>
>> Shouldn't stride on the right hand side be dropped?
>
> roffset = register index * stride.
>
> I.e. 380 for register with index 95.
>
> For stm32mp1x bsec:
> map->format.val_bytes = 4
> map->reg_stride = 4
>
> regmap_size_bytes() = map->format.val_bytes * (95 + 1) / map->reg_stride = 96
Ouch. What have I been thinking when I named regmap_size_bytes() this way...
Change looks ok then, but we should really rename regmap_size_bytes()...
>
> So the result with the stride on the right size is correct.
>
> I moved stride from left to right to be consistent with the size calculation
> in nvmem_regmap_register_with_pp()
>
> Kind regards,
> Robin
>
>
>>
>> Cheers,
>> Ahmad
>>
>>> return -EINVAL;
>>>
>>> for (i = roffset; i < roffset + rbytes; i += stride) {
>>> @@ -78,7 +78,7 @@ nvmem_regmap_register_with_pp(struct regmap *map, const
>>> char *name,
>>> config.priv = map;
>>> config.stride = 1;
>>> config.word_size = 1;
>>> - config.size = regmap_get_max_register(map) *
>>> regmap_get_reg_stride(map);
>>> + config.size = regmap_size_bytes(map) * regmap_get_reg_stride(map);
>>> config.cell_post_process = cell_post_process;
>>> config.reg_write = nvmem_regmap_write;
>>> config.reg_read = nvmem_regmap_read;
>
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0|
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
Re: [PATCH v2] nvmem: regmap: Fix nvmem size
Hi Ahmad,
On 2023-12-20 10:00, Ahmad Fatoum wrote:
Hello Robin,
Thanks for the fix.
On 20.12.23 09:29, Robin van der Gracht wrote:
- if (roffset + rbytes > stride * regmap_get_max_register(map))
+ if (roffset + rbytes > regmap_size_bytes(map) * stride)
Shouldn't stride on the right hand side be dropped?
roffset = register index * stride.
I.e. 380 for register with index 95.
For stm32mp1x bsec:
map->format.val_bytes = 4
map->reg_stride = 4
regmap_size_bytes() = map->format.val_bytes * (95 + 1) / map->reg_stride
= 96
So the result with the stride on the right size is correct.
I moved stride from left to right to be consistent with the size
calculation
in nvmem_regmap_register_with_pp()
Kind regards,
Robin
Cheers,
Ahmad
return -EINVAL;
for (i = roffset; i < roffset + rbytes; i += stride) {
@@ -78,7 +78,7 @@ nvmem_regmap_register_with_pp(struct regmap *map,
const char *name,
config.priv = map;
config.stride = 1;
config.word_size = 1;
- config.size = regmap_get_max_register(map) *
regmap_get_reg_stride(map);
+ config.size = regmap_size_bytes(map) * regmap_get_reg_stride(map);
config.cell_post_process = cell_post_process;
config.reg_write = nvmem_regmap_write;
config.reg_read = nvmem_regmap_read;
Re: [PATCH v2] nvmem: regmap: Fix nvmem size
Hello Robin,
Thanks for the fix.
On 20.12.23 09:29, Robin van der Gracht wrote:
> - if (roffset + rbytes > stride * regmap_get_max_register(map))
> + if (roffset + rbytes > regmap_size_bytes(map) * stride)
Shouldn't stride on the right hand side be dropped?
Cheers,
Ahmad
> return -EINVAL;
>
> for (i = roffset; i < roffset + rbytes; i += stride) {
> @@ -78,7 +78,7 @@ nvmem_regmap_register_with_pp(struct regmap *map, const
> char *name,
> config.priv = map;
> config.stride = 1;
> config.word_size = 1;
> - config.size = regmap_get_max_register(map) * regmap_get_reg_stride(map);
> + config.size = regmap_size_bytes(map) * regmap_get_reg_stride(map);
> config.cell_post_process = cell_post_process;
> config.reg_write = nvmem_regmap_write;
> config.reg_read = nvmem_regmap_read;
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0|
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
[PATCH v2] nvmem: regmap: Fix nvmem size
We should add 1 to the max_register index since counting is zero based.
i.e. the stm32mp151 bsec has registers 0 - 95 with reg_stride 4.
Size should be (95 + 1) * 4 = 384 bytes otherwise we can't access bsec
register 95 (last one).
regmap_size_bytes() does take the +1 into account so we can use that.
Signed-off-by: Robin van der Gracht
---
v2: Fix the size calculation in nvmem_regmap_read() as well.
drivers/nvmem/regmap.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/nvmem/regmap.c b/drivers/nvmem/regmap.c
index fa5405d7a8..b923c5787d 100644
--- a/drivers/nvmem/regmap.c
+++ b/drivers/nvmem/regmap.c
@@ -38,7 +38,7 @@ static int nvmem_regmap_read(void *ctx, unsigned offset, void
*buf, size_t bytes
skip_bytes = offset & (stride - 1);
rbytes = roundup(bytes + skip_bytes, stride);
- if (roffset + rbytes > stride * regmap_get_max_register(map))
+ if (roffset + rbytes > regmap_size_bytes(map) * stride)
return -EINVAL;
for (i = roffset; i < roffset + rbytes; i += stride) {
@@ -78,7 +78,7 @@ nvmem_regmap_register_with_pp(struct regmap *map, const char
*name,
config.priv = map;
config.stride = 1;
config.word_size = 1;
- config.size = regmap_get_max_register(map) * regmap_get_reg_stride(map);
+ config.size = regmap_size_bytes(map) * regmap_get_reg_stride(map);
config.cell_post_process = cell_post_process;
config.reg_write = nvmem_regmap_write;
config.reg_read = nvmem_regmap_read;
--
2.40.1
