[GitHub] [incubator-nuttx] gustavonihei commented on a diff in pull request #6511: Missing Data Command pin support

[GitBox <gi...@apache.org>]

gustavonihei commented on code in PR #6511:
URL: https://github.com/apache/incubator-nuttx/pull/6511#discussion_r906054724


##########
boards/xtensa/esp32/common/src/esp32_board_spi.c:
##########
@@ -84,6 +84,19 @@ static inline int spi_cmddata(struct spi_dev_s *dev, uint32_t devid,
     }
 #endif
 
+#ifdef CONFIG_LCD_SSD1680
+  if (devid == SPIDEV_DISPLAY(0))
+    {
+      /*  This is the Data/Command control pad which determines whether the
+       *  data bits are data or a command.
+       */
+
+      esp32_gpiowrite(CONFIG_SSD1680_GPIO_PIN_DTA_CMD, !cmd);
+
+      return OK;
+    }
+#endif

Review Comment:
   This block is basically identical to the one above for `LCD_ILI9341`, we should avoid this code duplication.
   Besides, `CONFIG_SSD1680_GPIO_PIN_DTA_CMD` is misplaced [here](CONFIG_SSD1680_GPIO_PIN_DTA_CMD). It is specific to just one ESP32-based board and, more important, it is an immutable configuration, so it should not be a Kconfig option.
   Could you please move them to the board header, like it is being done for the ESP-WROVER-KIT LCD [here](https://github.com/apache/incubator-nuttx/blob/master/boards/xtensa/esp32/esp32-wrover-kit/include/board.h#L75-L82)?
   This way we can avoid this code duplication and just extend the ifdef to consider both `CONFIG_LCD_ILI9341` and `CONFIG_LCD_SSD1680`.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org