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

[ChiYuan Huang]

Mark Brown <broonie@xxxxxxxxxx> 於 2020年9月25日 週五 下午7:33寫道：
>
> On Fri, Sep 25, 2020 at 12:07:50PM +0800, ChiYuan Huang wrote:
> > Mark Brown <broonie@xxxxxxxxxx> 於 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.