Hi, On Sat, Jun 10, 2023 at 10:45:25PM +0200, Sam Ravnborg wrote: > Hi Miquel, > > On Fri, Jun 09, 2023 at 04:59:51PM +0200, Miquel Raynal wrote: > > A very basic debugging rule when a device is connected for the first > > time is to access a read-only register which contains known data in > > order to ensure the communication protocol is properly working. This > > driver lacked any read helper which is often a critical peace for > > fastening bring-ups. > > > > Add a read helper and use it to verify the communication with the panel > > is working as soon as possible in order to fail early if this is not the > > case. > > The read helper seems like a log of general boiler plate code. > I briefly looked into the use of regmap for the spi communication, > but it did not look like it supported the bit9 stuff. > > I did not stare enough to add a reviewd by, but the approach is fine > and it is good to detech issues early.
The st7789v datasheet describes a setup where SPI is connected
unidirectional (i.e. without MISO). In that case the ID check
will fail.
-- Sebastian
>
> Acked-by: Sam Ravnborg <s...@ravnborg.org>
> >
> > Signed-off-by: Miquel Raynal <miquel.ray...@bootlin.com>
> > ---
> > .../gpu/drm/panel/panel-sitronix-st7789v.c | 78 +++++++++++++++++++
> > 1 file changed, 78 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/panel/panel-sitronix-st7789v.c
> > b/drivers/gpu/drm/panel/panel-sitronix-st7789v.c
> > index 7de192a3a8aa..fb21d52a7a63 100644
> > --- a/drivers/gpu/drm/panel/panel-sitronix-st7789v.c
> > +++ b/drivers/gpu/drm/panel/panel-sitronix-st7789v.c
> > @@ -113,6 +113,9 @@
> > return val; \
> > } while (0)
> >
> > +#define ST7789V_IDS { 0x85, 0x85, 0x52 }
> > +#define ST7789V_IDS_SIZE 3
> > +
> > struct st7789v_panel_info {
> > const struct drm_display_mode *display_mode;
> > u16 width_mm;
> > @@ -165,6 +168,77 @@ static int st7789v_write_data(struct st7789v *ctx, u8
> > cmd)
> > return st7789v_spi_write(ctx, ST7789V_DATA, cmd);
> > }
> >
> > +static int st7789v_read_data(struct st7789v *ctx, u8 cmd, u8 *buf,
> > + unsigned int len)
> > +{
> > + struct spi_transfer xfer[2] = { };
> > + struct spi_message msg;
> > + u16 txbuf = ((ST7789V_COMMAND & 1) << 8) | cmd;
> > + u16 rxbuf[4] = {};
> > + u8 bit9 = 0;
> > + int ret, i;
> > +
> > + switch (len) {
> > + case 1:
> > + case 3:
> > + case 4:
> > + break;
> > + default:
> > + return -EOPNOTSUPP;
> > + }
> > +
> > + spi_message_init(&msg);
> > +
> > + xfer[0].tx_buf = &txbuf;
> > + xfer[0].len = sizeof(txbuf);
> > + spi_message_add_tail(&xfer[0], &msg);
> > +
> > + xfer[1].rx_buf = rxbuf;
> > + xfer[1].len = len * 2;
> > + spi_message_add_tail(&xfer[1], &msg);
> > +
> > + ret = spi_sync(ctx->spi, &msg);
> > + if (ret)
> > + return ret;
> > +
> > + for (i = 0; i < len; i++) {
> > + buf[i] = rxbuf[i] >> i | (bit9 << (9 - i));
> > + if (i)
> > + bit9 = rxbuf[i] & GENMASK(i - 1, 0);
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static int st7789v_check_id(struct drm_panel *panel)
> > +{
> > + const u8 st7789v_ids[ST7789V_IDS_SIZE] = ST7789V_IDS;
> > + struct st7789v *ctx = panel_to_st7789v(panel);
> > + bool invalid_ids = false;
> > + int ret, i;
> > + u8 ids[3];
> > +
> > + ret = st7789v_read_data(ctx, MIPI_DCS_GET_DISPLAY_ID, ids,
> > ST7789V_IDS_SIZE);
> > + if (ret) {
> > + dev_err(panel->dev, "Failed to read IDs\n");
> > + return ret;
> > + }
> > +
> > + for (i = 0; i < ST7789V_IDS_SIZE; i++) {
> > + if (ids[i] != st7789v_ids[i]) {
> > + invalid_ids = true;
> > + break;
> > + }
> > + }
> > +
> > + if (invalid_ids) {
> > + dev_err(panel->dev, "Unrecognized panel IDs");
> > + return -EIO;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > static const struct drm_display_mode default_mode = {
> > .clock = 7000,
> > .hdisplay = 240,
> > @@ -237,6 +311,10 @@ static int st7789v_prepare(struct drm_panel *panel)
> > gpiod_set_value(ctx->reset, 0);
> > msleep(120);
> >
> > + ret = st7789v_check_id(panel);
> > + if (ret)
> > + return ret;
> > +
> > ST7789V_TEST(ret, st7789v_write_command(ctx, MIPI_DCS_EXIT_SLEEP_MODE));
> >
> > /* We need to wait 120ms after a sleep out command */
> > --
> > 2.34.1
signature.asc
Description: PGP signature
