linux-kernel - Re: [PATCH v1 1/2] regulator: rtmv20: Adds support for Richtek RTMV20 load switch regulator

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>] [day] [month] [year] [list]

Date:   Fri, 25 Sep 2020 22:05:05 +0800
From:   ChiYuan Huang <u0084500@...il.com>
To:     Mark Brown <broonie@...nel.org>, lgirdwood@...il.com,
        robh+dt@...nel.org
Cc:     linux-kernel@...r.kernel.org, cy_huang <cy_huang@...htek.com>,
        devicetree@...r.kernel.org
Subject: Re: [PATCH v1 1/2] regulator: rtmv20: Adds support for Richtek RTMV20
 load switch regulator

Mark Brown <broonie@...nel.org> 於 2020年9月25日 週五 下午7:33寫道：
>
> On Fri, Sep 25, 2020 at 12:07:50PM +0800, ChiYuan Huang wrote:
> > Mark Brown <broonie@...nel.org> 於 2020年9月25日 週五 上午12:12寫道：
>
> Please don't take things off-list unless there is a really strong reason
> to do so.  Sending things to the list ensures that everyone gets a
> chance to read and comment on things.
>
Sorry, I press the wrong button to only reply one not reply all.
Loop in the mail list again.
> > > I can't help but feel that this looks like a register cache would be a
> > > simpler way of achieving this.
>
> > This is because of enable gpio limitation. If gpio low to high, all
> > registers will be reset.
> > And enable gpio high to low will cause I2C cannot be accessed.
> > That's why we need to apply register setting after hardware enable pin
> > be pulled high.
> > The consumption current for EN L vs H is also different from 1uA vs
> > 450uA defined as typical values.
>
> That's exactly the sort of situation that regmap caches are usually used
> for, mark the device as cache only when powering off then resync after
> power on.
>
Thx, I think I catch the point.
> > > > +             switch (props[i].len) {
> > > > +             case RTMV20_2BYTE_ACCES:
> > > > +             case RTMV20_1BYTE_ACCES:
>
> > > It feels like this should all be abstracted in the regmap with custom
> > > reg_read() and reg_write() operations - or have two regmaps for the two
> > > different register sizes.  Having the register sizes and endianness
> > > handling outside of regmap code seems like an abstraction failure.
>
> > Actually, it's the consecutive register address with two byte data.
> > Lower address defined as val_h, and the higher defined as val_l.
> > So I just use cpu_to_be16 to do the transformation.
>
> Oh, so just a straight regmap would be fine.
>
Thx.
> > From the above hardware description, do you have any suggestion to
> > achieve the register cache?
>
> Look at how other devices use regcache_cache_only().
>
Thx.
> > > Don't do this, the driver should not adjust the system state unless
> > > explicitly told to do so - we don't know if any state changes are safe
> > > on a given board.
>
> > From my original coding,  it's put before regulator register.
> > After regulator registered, it will follow the regulator framework to
> > do the turn on/off and the other settings.
> > I think it won't affect the system state, just to keep IC as shutdown
> > mode (1uA) after chip vendor info check.
>
> It depends if the device was enabled before the driver started, and if
> anything started powering on when the device was powered on.
Sure, I'll re-check the flow.