Re: [PATCH v3] i2c: at91: support atomic write xfer

[Michał Mirosław]

On Tue, May 05, 2020 at 05:52:28PM +0200, Wolfram Sang wrote:
> Hi,
> 
> > I don't expect this to be used for much more than a simple write to PMIC
> > to kill the power. So this patch is tailor made for exactly this purpose.
> 
> Frankly, I don't like it much. The atomic callbacks are supposed to be
> drop-in replacements of the non-atomic contexts. There may be a need to
> read a PMIC register before writing something. I considered checking in
> the core if we can fall back to non-atomic calls if the the atomic ones
> return -EOPNOTSUPP, though,  but I still don't like the idea. I expect
> that people send me minimal versions then which are extended over time
> by very personal use cases. Having a proper implementation
> once-and-for-all (despite bugfixes) sounds much more maintainable to me.

Is it really possible to fall back to non-atomic calls? If that would be
possible, then I would actually want to use it in this case instead of
writing another implementation.

I must say, that I'm reluctant in investing a lot of time in doing
full-blown implementation for a feature that has only a very limited
use. I do understand your point about a proper implementation being
better than a special-cased-only one, but I really don't have a use
for the extension. The few PMICs I worked with just need a single
write to power off.

> > Though, if you would go for full support of atomic transfers, then
> > I would suggest to hack the non-atomic path to be usable in atomic mode
> > instead (some I2C drivers do just that, eg. i2c-tegra).
> 
> Yes, that is what I am aiming for.
> 
> > BTW, I found this comment in i2c-core.h:
> > 
> >  * We only allow atomic transfers for very late communication, e.g. to send
> >  * the powerdown command to a PMIC. Atomic transfers are a corner case and not
> >  * for generic use! 
> > 
> > I think this covers the idea.
> 
> Well, since I implemented the atomic_xfer mechanism, I think I am the
> primary authority of what "covers the idea", so I will fix the comment
> above :) Note, there is also this comment in the way more user-visible
> include/linux/i2c.h:
> 
>  509  * @master_xfer_atomic: same as @master_xfer. Yet, only using atomic context
>  510  *   so e.g. PMICs can be accessed very late before shutdown. Optional.

So, we don't have to wonder what the author had in mind. Lets expand
the idea then. :-) 

Shutdown is kind of special atomic context in that it is ok to do long
waits (as I2C requires) because nothing else is there to do. This is
very unlike normal atomic context. Do you plan to have it work in other
contexts? What are the idea and use cases for atomic-context transfers?

I guess we might want it for suspend/resume, but I think there is an
early stage (with all non-atomic stuff working) and NOIRQ stage (when
most everything is already shutdown). When a PMIC needs a read, I would
actually do it ("prepare" the PMIC) in the early stage if possible.

Best Regards,
Michał Mirosław