[PATCH 1/2] drm/dbi: Support DBI typec1 read operations

[Noralf Trønnes]

Den 14.06.2021 17.49, skrev Doug Anderson:
> Hi,
> 
> On Fri, Jun 11, 2021 at 2:29 PM Linus Walleij <linus.walleij at linaro.org> wrote:
>>
>> +static int mipi_dbi_typec1_command_read(struct mipi_dbi *dbi, u8 *cmd,
>> +                                       u8 *data, size_t len)
>> +{
>> +       struct spi_device *spi = dbi->spi;
>> +       u32 speed_hz = min_t(u32, MIPI_DBI_MAX_SPI_READ_SPEED,
>> +                            spi->max_speed_hz / 2);
> 
> I'm kinda curious why "max_speed_hz / 2", but you match the "typec3"
> version of this so I guess it's right?
> 

That's a good question, I can't remember the reason now, but I guess the
reasoning in something like: most MIPI DBI compatible controllers I've
seen is spec'ed at max 10MHz write speed and 2MHz read speed. I suppose
I have tried to match that read is slower than write. So I have probably
tried to play safe here. Not dividing by 2 is also fine by me.
The reason for allowing max_speed out of spec is that most controllers
can receive pixel data way above the nominal speed, often 4-6x, with the
occasional pixel glitch. We can't allow commands to glitch so must run
at nominal speed. mipi_dbi_spi_cmd_max_speed() is the one that caps the
write speed.

> 
>> +       struct spi_transfer tr[2] = {
>> +               {
>> +                       .speed_hz = speed_hz,
>> +                       .bits_per_word = 9,
>> +                       .tx_buf = dbi->tx_buf9,
>> +                       .len = 2,
>> +               }, {
>> +                       .speed_hz = speed_hz,
>> +                       .bits_per_word = 8,
>> +                       .len = len,
>> +                       .rx_buf = data,
>> +               },
>> +       };
>> +       struct spi_message m;
>> +       u16 *dst16;
>> +       int ret;
>> +
>> +       if (!len)
>> +               return -EINVAL;
>> +
>> +       if (!spi_is_bpw_supported(spi, 9)) {
>> +               /*
>> +                * FIXME: implement something like mipi_dbi_spi1e_transfer() but
>> +                * for reads using emulation.
>> +                */
>> +               dev_err(&spi->dev,
>> +                       "reading on host not supporting 9 bpw not yet implemented\n");
>> +               return -EOPNOTSUPP;
>> +       }
>> +
>> +       /*
>> +        * Turn the 8bit command into a 16bit version of the command in the
>> +        * buffer. Only 9 bits of this will be used when executing the actual
>> +        * transfer.
>> +        */
>> +       dst16 = dbi->tx_buf9;
>> +       dst16[0] = *cmd;
>> +
>> +       spi_message_init_with_transfers(&m, tr, ARRAY_SIZE(tr));
>> +       ret = spi_sync(spi, &m);
>> +
>> +       MIPI_DBI_DEBUG_COMMAND(*cmd, data, len);
> 
> nit: Only call MIPI_DBI_DEBUG_COMMAND() if !ret? Otherwise on an error
> you're just going to output whatever random data was already in the
> buffer that the user passed you. I suppose that same bug is present in
> mipi_dbi_typec3_command_read().
> 
> Other than that, this seems OK based on my feeble understanding of
> MIPI DBI (I couldn't even find the docs that I dug up before, sadly).

I have checked and the code lines up with Figure 26 Type C Interface
Read Sequence – Option 1 in the MIPI DBI v2.0 standard.

Noralf.

> Luckily Noralf is also here to review that part! :-) I'm happy with:
> 
> Reviewed-by: Douglas Anderson <dianders at chromium.org>
>