Re: [PATCH v4 1/9] reset: amlogic: convert driver to regmap

[Philipp Zabel <p.zabel@pengutronix.de>]

On Fr, 2024-09-06 at 16:46 +0200, Jerome Brunet wrote:
> On Fri 06 Sep 2024 at 16:19, Philipp Zabel <p.zabel@pengutronix.de> wrote:
> > On Fr, 2024-09-06 at 15:34 +0200, Jerome Brunet wrote:
[...]
> > > +static void meson_reset_offset_and_bit(struct meson_reset *data,
> > > +				       unsigned long id,
> > > +				       unsigned int *offset,
> > > +				       unsigned int *bit)
> > > +{
> > > +	unsigned int stride = regmap_get_reg_stride(data->map);
> > 
> > You know this is always 4. Having a #define for this (that is also used
> > to initialize regmap_config.reg_stride, and for now nr_resets, below)
> > instead of going through an exported function would allow the compiler
> > to optimize this all away:
> 
> Yes, for now. However, with the auxiliary you may get a regmap with
> different value. I've seen example with stride = 1. 
> 
> I'll admit is very unlikely but I does not really worth it considering
> how often we'll get through this and how difficult it will be to debug
> if stride ever get different.

Oh right, the aux regmap being passed in from the outside in a later
patch invalidates my point.

[...]
> > > @@ -48,25 +55,13 @@ static int meson_reset_level(struct reset_controller_dev *rcdev,
> > >  {
> > >  	struct meson_reset *data =
> > >  		container_of(rcdev, struct meson_reset, rcdev);
> > > -	unsigned int bank = id / BITS_PER_REG;
> > > -	unsigned int offset = id % BITS_PER_REG;
> > > -	void __iomem *reg_addr;
> > > -	unsigned long flags;
> > > -	u32 reg;
> > > +	unsigned int offset, bit;
> > >  
> > > -	reg_addr = data->reg_base + data->param->level_offset + (bank << 2);
> > > +	meson_reset_offset_and_bit(data, id, &offset, &bit);
> > > +	offset += data->param->level_offset;
> > >  
> > > -	spin_lock_irqsave(&data->lock, flags);
> > > -
> > > -	reg = readl(reg_addr);
> > > -	if (assert)
> > > -		writel(reg & ~BIT(offset), reg_addr);
> > > -	else
> > > -		writel(reg | BIT(offset), reg_addr);
> > > -
> > > -	spin_unlock_irqrestore(&data->lock, flags);
> > > -
> > > -	return 0;
> > > +	return regmap_update_bits(data->map, offset,
> > > +				  BIT(bit), assert ? 0 : BIT(bit));
> > 
> > Matter of taste, perhaps, but the BIT() could be moved into
> > meson_reset_offset_and_bit().
> 
> It's not really related to this particular patch which is about moving
> to regmap.
> 
> I can add it to another patch if you want

Either way is fine.


regards
Philipp


_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic