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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <86eebdd7435ec1dc09765f4c4f8cb5ba8d20d785.camel@pengutronix.de>
Date: Tue, 10 Sep 2024 10:20:44 +0200
From: Philipp Zabel <p.zabel@...gutronix.de>
To: Jerome Brunet <jbrunet@...libre.com>
Cc: Stephen Boyd <sboyd@...nel.org>, Neil Armstrong
 <neil.armstrong@...aro.org>,  Kevin Hilman <khilman@...libre.com>, Martin
 Blumenstingl <martin.blumenstingl@...glemail.com>, Jiucheng Xu
 <jiucheng.xu@...ogic.com>, linux-arm-kernel@...ts.infradead.org, 
 linux-amlogic@...ts.infradead.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v4 1/9] reset: amlogic: convert driver to regmap

On Fr, 2024-09-06 at 16:46 +0200, Jerome Brunet wrote:
> On Fri 06 Sep 2024 at 16:19, Philipp Zabel <p.zabel@...gutronix.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