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?
> + 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).
Luckily Noralf is also here to review that part! :-) I'm happy with:
Reviewed-by: Douglas Anderson <dianders@xxxxxxxxxxxx>
