| Message ID | 20190717115817.30110-6-noralf@tronnes.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/tinydrm: Remove tinydrm.ko | expand |
On Wed, Jul 17, 2019 at 01:58:12PM +0200, Noralf Trønnes wrote: > Prep work before moving the function to mipi-dbi. > > tinydrm_spi_transfer() was made to support one class of drivers in > drivers/staging/fbtft that has not been converted to DRM yet, so strip > away the unused functionality: > - Start byte (header) is not used. > - No driver relies on the automatic 16-bit byte swapping on little endian > machines with SPI controllers only supporting 8 bits per word. Keeping unused code around is never a good idea. On the other hand, should we not try to get the driver in questions ported so we have a user and we do not need to re-add this later? What driver/display needs this? Sam
Den 17.07.2019 15.09, skrev Sam Ravnborg: > On Wed, Jul 17, 2019 at 01:58:12PM +0200, Noralf Trønnes wrote: >> Prep work before moving the function to mipi-dbi. >> >> tinydrm_spi_transfer() was made to support one class of drivers in >> drivers/staging/fbtft that has not been converted to DRM yet, so strip >> away the unused functionality: >> - Start byte (header) is not used. >> - No driver relies on the automatic 16-bit byte swapping on little endian >> machines with SPI controllers only supporting 8 bits per word. > > Keeping unused code around is never a good idea. > On the other hand, should we not try to get the driver in questions > ported so we have a user and we do not need to re-add this later? > What driver/display needs this? At least drivers/staging/fbtft/fb_ili932{0,5}.c and maybe another one, I don't remember. I haven't worked on fbtft after I did tinydrm. It looks like they still sell the hy28b: https://www.hotmcu.com/28-touch-screen-tft-lcd-with-all-interface-p-63.html I'm not sure what the future of fbtft is. The idea was that the drivers should get cleaned up and move out of staging, but then fbdev was closed for new drivers and I did tinydrm. Only two drivers have been converted apart from mi0283qt that I did and that is hx8357 which Eric did and st7735 that David did. Those 3 covers a lot of the tiny SPI display marked, Adafruit sells them. It's a chicken and egg problem, as long as the fbtft drivers are there and working, there's no incentive to convert them. There's another challenge with these drivers since it is possible to override the init sequence in Device Tree, meaning they can work with all kinds of displays without writing a new driver. I was not allowed to keep that functionality outside of staging. When I'm done with the tinydrm cleanup, I'm going to work on an idea I have: turn the Raspberry Pi Zero into a $5 USB to HDMI/SDTV/DSI/DPI/SPI-display adapter. I'm planning to write a generic USB host display driver with a matching Linux OTG device driver. I plan to make it easy to do the display OTG side on a microcontroller as well. This way it will be possible for manufacturers to do USB connected displays of (nearly) all sizes without having to write a Linux driver. It's difficult to predict the future, but powerful microcontrollers are cheap nowadays so maybe these SPI displays will be fased out by cheap USB displays. The uC can replace the touch controller cutting some of the uC cost. Noralf.
Hi Noralf. On Wed, Jul 17, 2019 at 06:18:39PM +0200, Noralf Trønnes wrote: > > > Den 17.07.2019 15.09, skrev Sam Ravnborg: > > On Wed, Jul 17, 2019 at 01:58:12PM +0200, Noralf Trønnes wrote: > >> Prep work before moving the function to mipi-dbi. > >> > >> tinydrm_spi_transfer() was made to support one class of drivers in > >> drivers/staging/fbtft that has not been converted to DRM yet, so strip > >> away the unused functionality: > >> - Start byte (header) is not used. > >> - No driver relies on the automatic 16-bit byte swapping on little endian > >> machines with SPI controllers only supporting 8 bits per word. > > > > Keeping unused code around is never a good idea. > > On the other hand, should we not try to get the driver in questions > > ported so we have a user and we do not need to re-add this later? > > What driver/display needs this? > > At least drivers/staging/fbtft/fb_ili932{0,5}.c and maybe another one, I > don't remember. I haven't worked on fbtft after I did tinydrm. > It looks like they still sell the hy28b: > https://www.hotmcu.com/28-touch-screen-tft-lcd-with-all-interface-p-63.html I ordered one, then we will see if I also find time to port the driver and test it. > I'm not sure what the future of fbtft is. The idea was that the drivers > should get cleaned up and move out of staging, but then fbdev was closed > for new drivers and I did tinydrm. Only two drivers have been converted > apart from mi0283qt that I did and that is hx8357 which Eric did and > st7735 that David did. Those 3 covers a lot of the tiny SPI display > marked, Adafruit sells them. > It's a chicken and egg problem, as long as the fbtft drivers are there > and working, there's no incentive to convert them. I follow the average joe user here. If it works then why worry. But if I get HW and time I can at least port over a few of them. It looks like it takes more time to test than to do the porting. > There's another challenge with these drivers since it is possible to > override the init sequence in Device Tree, meaning they can work with > all kinds of displays without writing a new driver. > I was not allowed to keep that functionality outside of staging. > > When I'm done with the tinydrm cleanup, I'm going to work on an idea I > have: turn the Raspberry Pi Zero into a $5 USB to > HDMI/SDTV/DSI/DPI/SPI-display adapter. I'm planning to write a generic > USB host display driver with a matching Linux OTG device driver. > I plan to make it easy to do the display OTG side on a microcontroller > as well. This way it will be possible for manufacturers to do USB > connected displays of (nearly) all sizes without having to write a Linux > driver. It will be interesting to follow this, keep us posted. > It's difficult to predict the future, but powerful microcontrollers are > cheap nowadays so maybe these SPI displays will be fased out by cheap > USB displays. The uC can replace the touch controller cutting some of > the uC cost. Yep, it is impressive what one can get for USD 5 these days. Sam
On 7/17/19 6:58 AM, Noralf Trønnes wrote: > Prep work before moving the function to mipi-dbi. > > tinydrm_spi_transfer() was made to support one class of drivers in > drivers/staging/fbtft that has not been converted to DRM yet, so strip > away the unused functionality: > - Start byte (header) is not used. > - No driver relies on the automatic 16-bit byte swapping on little endian > machines with SPI controllers only supporting 8 bits per word. > > Other changes: > - No need to initialize ret > - No need for the WARN since mipi-dbi only uses 8 and 16 bpw. > - Use spi_message_init_with_transfers() > > Cc: David Lechner <david@lechnology.com> > Signed-off-by: Noralf Trønnes <noralf@tronnes.org> > --- Acked-by: : David Lechner <david@lechnology.com>
diff --git a/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c b/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c index af5bec8861de..d95eb50fa9d4 100644 --- a/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c +++ b/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c @@ -24,23 +24,18 @@ * tinydrm_spi_transfer - SPI transfer helper * @spi: SPI device * @speed_hz: Override speed (optional) - * @header: Optional header transfer * @bpw: Bits per word * @buf: Buffer to transfer * @len: Buffer length * * This SPI transfer helper breaks up the transfer of @buf into chunks which - * the SPI master driver can handle. If the machine is Little Endian and the - * SPI master driver doesn't support 16 bits per word, it swaps the bytes and - * does a 8-bit transfer. - * If @header is set, it is prepended to each SPI message. + * the SPI controller driver can handle. * * Returns: * Zero on success, negative error code on failure. */ int tinydrm_spi_transfer(struct spi_device *spi, u32 speed_hz, - struct spi_transfer *header, u8 bpw, const void *buf, - size_t len) + u8 bpw, const void *buf, size_t len) { size_t max_chunk = spi_max_transfer_size(spi); struct spi_transfer tr = { @@ -48,43 +43,16 @@ int tinydrm_spi_transfer(struct spi_device *spi, u32 speed_hz, .speed_hz = speed_hz, }; struct spi_message m; - u16 *swap_buf = NULL; size_t chunk; - int ret = 0; + int ret; - if (WARN_ON_ONCE(bpw != 8 && bpw != 16)) - return -EINVAL; - - if (bpw == 16 && !spi_is_bpw_supported(spi, 16)) { - tr.bits_per_word = 8; - if (tinydrm_machine_little_endian()) { - swap_buf = kmalloc(min(len, max_chunk), GFP_KERNEL); - if (!swap_buf) - return -ENOMEM; - } - } - - spi_message_init(&m); - if (header) - spi_message_add_tail(header, &m); - spi_message_add_tail(&tr, &m); + spi_message_init_with_transfers(&m, &tr, 1); while (len) { chunk = min(len, max_chunk); tr.tx_buf = buf; tr.len = chunk; - - if (swap_buf) { - const u16 *buf16 = buf; - unsigned int i; - - for (i = 0; i < chunk / 2; i++) - swap_buf[i] = swab16(buf16[i]); - - tr.tx_buf = swap_buf; - } - buf += chunk; len -= chunk; diff --git a/drivers/gpu/drm/tinydrm/ili9225.c b/drivers/gpu/drm/tinydrm/ili9225.c index 7a8e1b4a37ee..21677e3ed38b 100644 --- a/drivers/gpu/drm/tinydrm/ili9225.c +++ b/drivers/gpu/drm/tinydrm/ili9225.c @@ -323,7 +323,7 @@ static int ili9225_dbi_command(struct mipi_dbi *mipi, u8 *cmd, u8 *par, gpiod_set_value_cansleep(mipi->dc, 0); speed_hz = mipi_dbi_spi_cmd_max_speed(spi, 1); - ret = tinydrm_spi_transfer(spi, speed_hz, NULL, 8, cmd, 1); + ret = tinydrm_spi_transfer(spi, speed_hz, 8, cmd, 1); if (ret || !num) return ret; @@ -333,7 +333,7 @@ static int ili9225_dbi_command(struct mipi_dbi *mipi, u8 *cmd, u8 *par, gpiod_set_value_cansleep(mipi->dc, 1); speed_hz = mipi_dbi_spi_cmd_max_speed(spi, num); - return tinydrm_spi_transfer(spi, speed_hz, NULL, bpw, par, num); + return tinydrm_spi_transfer(spi, speed_hz, bpw, par, num); } static const struct drm_simple_display_pipe_funcs ili9225_pipe_funcs = { diff --git a/drivers/gpu/drm/tinydrm/mipi-dbi.c b/drivers/gpu/drm/tinydrm/mipi-dbi.c index ae31a5c9aa1b..8fb6ce4ca6fc 100644 --- a/drivers/gpu/drm/tinydrm/mipi-dbi.c +++ b/drivers/gpu/drm/tinydrm/mipi-dbi.c @@ -926,7 +926,7 @@ static int mipi_dbi_typec3_command(struct mipi_dbi *mipi, u8 *cmd, gpiod_set_value_cansleep(mipi->dc, 0); speed_hz = mipi_dbi_spi_cmd_max_speed(spi, 1); - ret = tinydrm_spi_transfer(spi, speed_hz, NULL, 8, cmd, 1); + ret = tinydrm_spi_transfer(spi, speed_hz, 8, cmd, 1); if (ret || !num) return ret; @@ -936,7 +936,7 @@ static int mipi_dbi_typec3_command(struct mipi_dbi *mipi, u8 *cmd, gpiod_set_value_cansleep(mipi->dc, 1); speed_hz = mipi_dbi_spi_cmd_max_speed(spi, num); - return tinydrm_spi_transfer(spi, speed_hz, NULL, bpw, par, num); + return tinydrm_spi_transfer(spi, speed_hz, bpw, par, num); } /** diff --git a/include/drm/tinydrm/tinydrm-helpers.h b/include/drm/tinydrm/tinydrm-helpers.h index 10b35375a009..708c5a7d51e0 100644 --- a/include/drm/tinydrm/tinydrm-helpers.h +++ b/include/drm/tinydrm/tinydrm-helpers.h @@ -42,7 +42,6 @@ int tinydrm_display_pipe_init(struct drm_device *drm, unsigned int rotation); int tinydrm_spi_transfer(struct spi_device *spi, u32 speed_hz, - struct spi_transfer *header, u8 bpw, const void *buf, - size_t len); + u8 bpw, const void *buf, size_t len); #endif /* __LINUX_TINYDRM_HELPERS_H */
Prep work before moving the function to mipi-dbi. tinydrm_spi_transfer() was made to support one class of drivers in drivers/staging/fbtft that has not been converted to DRM yet, so strip away the unused functionality: - Start byte (header) is not used. - No driver relies on the automatic 16-bit byte swapping on little endian machines with SPI controllers only supporting 8 bits per word. Other changes: - No need to initialize ret - No need for the WARN since mipi-dbi only uses 8 and 16 bpw. - Use spi_message_init_with_transfers() Cc: David Lechner <david@lechnology.com> Signed-off-by: Noralf Trønnes <noralf@tronnes.org> --- .../gpu/drm/tinydrm/core/tinydrm-helpers.c | 40 ++----------------- drivers/gpu/drm/tinydrm/ili9225.c | 4 +- drivers/gpu/drm/tinydrm/mipi-dbi.c | 4 +- include/drm/tinydrm/tinydrm-helpers.h | 3 +- 4 files changed, 9 insertions(+), 42 deletions(-)