Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Sender: Carlo Caione <carlo.caione@gmail.com>
From:   Carlo Caione <carlo@caione.org>
Message-ID: <e36142ec-6b7f-e667-7d6b-48234318c8cd@baylibre.com>
Date:   Fri, 18 Nov 2022 11:36:27 +0100
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.4.2
Subject: Re: [PATCH 2/3] drm/tiny: ili9486: Do not assume 8-bit only SPI
 controllers
Content-Language: en-US
To:     Mark Brown <broonie@kernel.org>
Cc:     Kamlesh Gurudasani <kamlesh.gurudasani@gmail.com>,
        Neil Armstrong <neil.armstrong@linaro.org>,
        Jerome Brunet <jbrunet@baylibre.com>,
        David Airlie <airlied@gmail.com>,
        Martin Blumenstingl <martin.blumenstingl@googlemail.com>,
        Kevin Hilman <khilman@baylibre.com>,
        Daniel Vetter <daniel@ffwll.ch>,
        linux-arm-kernel@lists.infradead.org,
        linux-amlogic@lists.infradead.org, linux-kernel@vger.kernel.org,
        linux-spi@vger.kernel.org, dri-devel@lists.freedesktop.org
References: <20221116-s905x_spi_ili9486-v1-0-630401cb62d5@baylibre.com>
 <20221116-s905x_spi_ili9486-v1-2-630401cb62d5@baylibre.com>
 <Y3YWdeTLfmDh7UyB@sirena.org.uk>
 <cd2d4d1e-f42a-da5b-e498-fbb32f792094@baylibre.com>
 <Y3ZMT4F3+3bjNXKo@sirena.org.uk>
In-Reply-To: <Y3ZMT4F3+3bjNXKo@sirena.org.uk>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 17/11/2022 15:59, Mark Brown wrote:

> So this is an issue in the MIPI DBI code where the interpretation of 
> the buffer passed in depends on both the a caller parameter and the 
> capabilities of the underlying SPI controller, meaning that a driver 
> can suddenly become buggy when used with a new controller?

The MIPI DBI code is fine, in fact it is doing the correct thing in the
mipi_dbi_typec3_command() function. The problem is that the ILI9486
driver is hijacking that function installing its own hook that is wrong.

> I can't really tell what the bits per word being passed in along
> with the buffer is supposed to mean, I'd have expected it to
> correspond to the format of the buffer but it seems like perhaps the
> buffer is always formatted for 16 bits and the callers are needing to
> pass in the capabilities of the controller which is then also checked
> by the underlying code? This all seems extremely confusing, I'm not 
> surprised there's bugs.

Correct, the buffer (pixel data) is always formatted for 16 bits and the
bpw is to indicate how this data should be put on the bus (according to
the controller capability).

If the controller is only capable of 8-bit transfers, the 16-bit data
needs to be swapped to account for the big endian bus, while this is not
needed if the controller already supports 16-bit transfers.

The decision to swap the data or not is taken in the MIPI DBI code by
probing the controller capabilities, so if the controller only supports
8-bit the data is swapped, otherwise it is not.

The problem arrives when your controller does support 16-bits, so your
data is not swapped, but you still put the data on the bus with 8-bit
transfers.

> At the very least your changelog needs to express clearly what is 
> going on, the description doesn't appear to correspond to what
> you're changing.

Gotcha, I'll try to clarify that in the next revision.

Thanks,

--
Carlo Caione