Re: [PATCH] regmap: don't check for alignment when using reg_shift

[Maxime Chevallier <maxime.chevallier@bootlin.com>]

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