Re: [PATCH v2 16/20] tty: serial: samsung: Add gs101 compatible and SoC data

Hi Greg,

Thanks for the review.

On Wed, 11 Oct 2023 at 08:47, Greg KH [off-list ref] wrote:

On Tue, Oct 10, 2023 at 11:49:24PM +0100, Peter Griffin wrote:

quoted

Add serial driver data for Google Tensor gs101 SoC.

Signed-off-by: Peter Griffin <peter.griffin@linaro.org>
---
 drivers/tty/serial/samsung_tty.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/tty/serial/samsung_tty.c b/drivers/tty/serial/samsung_tty.c
index 07fb8a9dac63..79a1a184d5c1 100644
--- a/drivers/tty/serial/samsung_tty.c
+++ b/drivers/tty/serial/samsung_tty.c

@@ -2597,14 +2597,21 @@ static const struct s3c24xx_serial_drv_data exynos850_serial_drv_data = {
      .fifosize = { 256, 64, 64, 64 },
 };

+static const struct s3c24xx_serial_drv_data gs101_serial_drv_data = {
+     EXYNOS_COMMON_SERIAL_DRV_DATA(),
+     .fifosize = { 256, 64, 64, 64 },
+};

Why are you duplicating a structure that is already in the same file?
What is the benifit here?

There is a mistake here, the struct shouldn't be the same as e850 it
should look like this

static const struct s3c24xx_serial_drv_data gs101_serial_drv_data = {
     EXYNOS_COMMON_SERIAL_DRV_DATA(),
     /* rely on samsung,uart-fifosize DT property for fifosize */
     .fifosize = { 0 },
};

This then allows the fifosize to be taken from the samsung,uart-fifosize
DT property for each of the 19 UARTs on this SoC.

quoted

 #define EXYNOS4210_SERIAL_DRV_DATA (&exynos4210_serial_drv_data)
 #define EXYNOS5433_SERIAL_DRV_DATA (&exynos5433_serial_drv_data)
 #define EXYNOS850_SERIAL_DRV_DATA (&exynos850_serial_drv_data)
+#define GS101_SERIAL_DRV_DATA (&gs101_serial_drv_data)

What is "GS101"?

gs101 is the name of the SoC in Pixel 6, 6 pro, 6a phones. I've put
some more info
about the various names of the SoC in the bindings documentation. See
https://lore.kernel.org/linux-arm-kernel/20231010224928.2296997-9-peter.griffin@linaro.org/T/#mb45492e58de0bef566df8cdf6191ab8f96f0cf99 (local)

quoted

 #else
 #define EXYNOS4210_SERIAL_DRV_DATA NULL
 #define EXYNOS5433_SERIAL_DRV_DATA NULL
 #define EXYNOS850_SERIAL_DRV_DATA NULL
+#define GS101_SERIAL_DRV_DATA NULL
 #endif

 #ifdef CONFIG_ARCH_APPLE

@@ -2688,6 +2695,9 @@ static const struct platform_device_id s3c24xx_serial_driver_ids[] = {
      }, {
              .name           = "artpec8-uart",
              .driver_data    = (kernel_ulong_t)ARTPEC8_SERIAL_DRV_DATA,
+     }, {
+             .name           = "gs101-uart",
+             .driver_data    = (kernel_ulong_t)GS101_SERIAL_DRV_DATA,
      },
      { },
 };

@@ -2709,6 +2719,8 @@ static const struct of_device_id s3c24xx_uart_dt_match[] = {
              .data = EXYNOS850_SERIAL_DRV_DATA },
      { .compatible = "axis,artpec8-uart",
              .data = ARTPEC8_SERIAL_DRV_DATA },
+     { .compatible = "google,gs101-uart",
+             .data =  GS101_SERIAL_DRV_DATA },

Why aren't you just listing this hardware as the same one above?  There
doesn't need to be a new entry if you just fix up the DT for the board
to declare it as the proper type of device.  No need to keep adding new
entries that do the exact same thing, we don't normally like that at all
for other bus types, why is DT different?

I believe Krzysztof already answered this from a dt maintainer point of view.

regards,

Peter.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel