Den 14.06.2021 17.49, skrev Doug Anderson:
> Hi,
>
> On Fri, Jun 11, 2021 at 2:29 PM Linus Walleij <linus.walleij@xxxxxxxxxx> 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@xxxxxxxxxxxx>
>
