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

Hi,

On Fri, Jun 11, 2021 at 2:29 PM Linus Walleij linus.walleij@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?

•   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@chromium.org