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

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

[Linus Walleij]

Implement SPI reads for typec1, for SPI controllers that
can support 9bpw in addition to 8bpw (such as GPIO bit-banged
SPI).

9bpw emulation is not supported but we have to start with
something.

This is used by s6e63m0 to read display MTP information
which is used by the driver for backlight control.

Cc: Douglas Anderson <dianders@chromium.org>
Cc: Noralf Trønnes <noralf@tronnes.org>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/gpu/drm/drm_mipi_dbi.c | 54 +++++++++++++++++++++++++++++++++-
 1 file changed, 53 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_mipi_dbi.c b/drivers/gpu/drm/drm_mipi_dbi.c
index 43a9b739bba7..3854fb9798e9 100644
--- a/drivers/gpu/drm/drm_mipi_dbi.c
+++ b/drivers/gpu/drm/drm_mipi_dbi.c
@@ -928,6 +928,58 @@ static int mipi_dbi_spi1_transfer(struct mipi_dbi *dbi, int dc,
 	return 0;
 }
 
+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);
+	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);
+
+	return ret;
+}
+
 static int mipi_dbi_typec1_command(struct mipi_dbi *dbi, u8 *cmd,
 				   u8 *parameters, size_t num)
 {
@@ -935,7 +987,7 @@ static int mipi_dbi_typec1_command(struct mipi_dbi *dbi, u8 *cmd,
 	int ret;
 
 	if (mipi_dbi_command_is_read(dbi, *cmd))
-		return -EOPNOTSUPP;
+		return mipi_dbi_typec1_command_read(dbi, cmd, parameters, num);
 
 	MIPI_DBI_DEBUG_COMMAND(*cmd, parameters, num);
 
-- 
2.31.1

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

[Noralf Trønnes]

Den 11.06.2021 23.27, skrev Linus Walleij:
> Implement SPI reads for typec1, for SPI controllers that
> can support 9bpw in addition to 8bpw (such as GPIO bit-banged
> SPI).
> 
> 9bpw emulation is not supported but we have to start with
> something.
> 
> This is used by s6e63m0 to read display MTP information
> which is used by the driver for backlight control.
> 
> Cc: Douglas Anderson <dianders@chromium.org>
> Cc: Noralf Trønnes <noralf@tronnes.org>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---

Reviewed-by: Noralf Trønnes <noralf@tronnes.org>

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

[Doug Anderson]

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>

Re: [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@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@chromium.org>
>