Re: [PATCH] staging: nvec: make i2c controller register writes robust

[Greg KH <gregkh@linuxfoundation.org>]

On Mon, May 20, 2024 at 10:11:20AM +0200, Marc Dietrich wrote:
> Hello Greg,
> 
> On Sun, 21 Apr 2024, Marc Dietrich wrote:
> 
> > The i2c controller needs to read back the data written to its registers.
> > This way we can avoid the long delay in the interrupt handler.
> > 
> > Signed-off-by: Marc Dietrich <marvin24@gmx.de>
> > ---
> > drivers/staging/nvec/nvec.c | 41 ++++++++++++++++++++++---------------
> > 1 file changed, 24 insertions(+), 17 deletions(-)
> 
> I think I answered all remaining comments. Are there any other
> objections?

Sorry, just went back and looked, and no, as-is I don't want to take
this:

> > diff --git a/drivers/staging/nvec/nvec.c b/drivers/staging/nvec/nvec.c
> > index 45df190c2f94..214839f51048 100644
> > --- a/drivers/staging/nvec/nvec.c
> > +++ b/drivers/staging/nvec/nvec.c
> > @@ -570,6 +570,22 @@ static void nvec_tx_set(struct nvec_chip *nvec)
> > 		(uint)nvec->tx->size, nvec->tx->data[1]);
> > }
> > 
> > +/**
> > + * i2c_writel - safely write to an I2C client controller register
> > + * @val: value to be written
> > + * @reg: register to write to
> > + *
> > + * A write to an I2C controller register needs to be read back to make sure
> > + * that the value has arrived.
> > + */
> > +static void i2c_writel(u32 val, void *reg)
> > +{
> > +	writel_relaxed(val, reg);
> > +
> > +	/* read back register to make sure that register writes completed */
> > +	readl_relaxed(reg);
> > +}

This is not an i2c write function, please don't call it that and give it
kernel doc formatting implying that it is an i2c write.

Name it after this odd device that requires the read to happen, that's
unique to it, nothing else.

And don't worry about "potential drivers in the future", we worry about
what we have here today.  You can rename/modify/whatever in the future
if anything else ever happens.  We can only go off of what we have in
front of us now.

thanks,

greg k-h