Messages in this thread | | | Date | Fri, 28 Apr 2023 09:30:10 +0200 | From | Maxime Chevallier <> | Subject | Re: [PATCH] regmap: don't check for alignment when using reg_shift |
| |
Hello Mark, Colin,
On Tue, 25 Apr 2023 13:56:23 +0100 Mark Brown <broonie@kernel.org> wrote:
> On Fri, Apr 21, 2023 at 08:50:30AM -0700, Colin Foster wrote: > > On Thu, Apr 20, 2023 at 05:06:17PM +0200, Maxime Chevallier wrote: > > > > On regmap consumers that require address translation through > > > up/downshifting, the alignment check in the regmap core doesn't > > > take the translation into account. This doesn't matter when > > > downshifting the register address, as any address that fits a > > > given alignment requirement will still meet it when downshifted > > > (a 4-byte aligned address will always also be 2-bytes aligned for > > > example). > > > > However, when upshifting, this check causes spurious errors, as it > > > occurs before the upshifting. > > > I don't follow why upshifting should make a difference to alignment. > > Assuming it does though, would it make sense to test > > > map->format.reg_shift > 0 > > > instead of just !map->format.reg_shift? > > Yeah, I think the question is more when we should run the alignment > check than if we should have one. I think running the check after any > shifting makes sense, we'd be better off reorganising the checks if > needed than removing them.
In the initial RFC I suggested this [1] approach, which checked for alignment after shifting, that way we are sure that the alignment check is done according to the underlying regmap provider's constraints. Maybe this could be sufficient ?
Thanks,
Maxime
> > > > > - if (!IS_ALIGNED(reg, map->reg_stride)) > > > + if (!map->format.reg_shift && !IS_ALIGNED(reg, > > > map->reg_stride)) return -EINVAL; > > > > In the case of ocelot_spi, we'd want to flag an invalid access to a > > register like 0x71070003... Before this patch it would return > > -EINVAL, after this patch it would access 0x71070000. > > > > Colin Foster
| |