| Message ID | 20230609145951.853533-8-miquel.raynal@bootlin.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/panel: sitronix-st7789v: Support ET028013DMA panel | expand |
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. Acked-by: Sam Ravnborg <sam@ravnborg.org> > > Signed-off-by: Miquel Raynal <miquel.raynal@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
Hi Miquel, On 6/9/23 16:59, 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. I am afraid I don't get the last sentence. s/peace/piece? s/for fastening/to speed up ? Only guessing here. Best regards, Michael > 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. > > Signed-off-by: Miquel Raynal <miquel.raynal@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 */
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 <sam@ravnborg.org> > > > > Signed-off-by: Miquel Raynal <miquel.raynal@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
Hi Sebastian, sre@kernel.org wrote on Thu, 15 Jun 2023 01:27:24 +0200: > 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. Right. I'll add a (spi->mode & SPI_NO_RX) check, as the default is to have both lines, if there is no MISO line, I'd expect it to be described in the DT, otherwise the description is broken. Thanks, Miquèl
Hi, On Fri, Jun 16, 2023 at 12:13:45PM +0200, Miquel Raynal wrote: > sre@kernel.org wrote on Thu, 15 Jun 2023 01:27:24 +0200: > > On Sat, Jun 10, 2023 at 10:45:25PM +0200, Sam Ravnborg wrote: > > > 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. > > Right. I'll add a (spi->mode & SPI_NO_RX) check, as the default is to > have both lines, if there is no MISO line, I'd expect it to be > described in the DT, otherwise the description is broken. Checking for SPI_NO_RX sounds good to me. -- Sebastian
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 */
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. Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> --- .../gpu/drm/panel/panel-sitronix-st7789v.c | 78 +++++++++++++++++++ 1 file changed, 78 insertions(+)