Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <20190813212251.12316-1-ben.whitten@gmail.com> <20190814100115.GF4640@sirena.co.uk>
 <CAF3==iuZvCnmAg9hqs8ivHw0wHaUQEf8k9U8=KTekMMjdyyEKg@mail.gmail.com> <20190814161938.GI4640@sirena.co.uk>
In-Reply-To: <20190814161938.GI4640@sirena.co.uk>
From:   Ben Whitten <ben.whitten@gmail.com>
Date:   Tue, 3 Sep 2019 10:42:59 +0100
Message-ID: <CAF3==ivJycGpSpSy3Y9cL7kNf27=kZmETpb-v0=L7BNd1ZPJtg@mail.gmail.com>
Subject: Re: [PATCH] regmap: fix writes to non incrementing registers
To:     Mark Brown <broonie@kernel.org>
Cc:     LKML <linux-kernel@vger.kernel.org>,
        =?UTF-8?Q?Andreas_F=C3=A4rber?= <afaerber@suse.de>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        "Rafael J. Wysocki" <rafael@kernel.org>, nandor.han@vaisala.com
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Wed, 14 Aug 2019 at 17:19, Mark Brown <broonie@kernel.org> wrote:
>
> On Wed, Aug 14, 2019 at 02:09:11PM +0100, Ben Whitten wrote:
>
> > So it appeared that the last patch in this area for validating a register
> > block [1] broke the regmap_noinc_write use case.
>
> Please include human readable descriptions of things like commits and
> issues being discussed in e-mail in your mails, this makes them much
> easier for humans to read especially when they have no internet access.
> I do frequently catch up on my mail on flights or while otherwise
> travelling so this is even more pressing for me than just being about
> making things a bit easier to read.

Sure thing, the patch adds in a call which checks if range of registers
is writeable within _regmap_raw_write_impl. This check uses the length
of the data given to _regmap_raw_write_impl to determine the range
of registers to check from the given starting register.

> > Because regmap_noinc_write calls _regmap_raw_write and in
> > turn hits the _regmap_raw_write_impl, the val_len is the depth of the
> > one register to write to and not a block of registers which is assumed
> > by the previous check. By inserting a check that the first (and only)
> > register is a noinc one allows me to start writing to my FIFO again.
>
> > I'm all for an alternative solution though if there is a cleaner approach.
>
> Like I said if we're checking for nonincrementing registers it shouldn't
> just be on the first register, it should be for every address in the
> range.  Probably accept it if the nonincrementing register is the first
> and error otherwise, with some documentation explaining what's going on.

I see yes, we don't want to stumble into a noinc register by mistake when
writing a register range.

You mentioned that we likely have breakage elsewhere, I believe that
regmap_noinc_write probably shouldn't ever have been calling
_regmap_raw_write.

Whilst regmap_noinc_read calls _regmap_raw_read, this function doesn't
do any massage and just calls map->bus->read after selecting a page.
regmap_raw_write on the other hand is meant for writing blocks to
register ranges as these added checks show, and does split work based
on page length etc.
This splitting up is something that shouldn't apply to the FIFO as it's a
deep register.

I modified regmap_noinc_write to be much more like the raw_read
internals, just select page then attempt a map->bus->gather_write,
falling back to linearising if that's not supported.
This approach worked at getting writing working into the FIFO.

So I would propose that there are two 'Fixes:' patches here, one
enhancing the checking when writing a register range to ensure you
don't stumble into a noinc register.
And one to fix noinc_writes to send data directly to the bus once the
page is selected with no splitting up as you would a range.

Kind regards,
Ben