| Message ID | 20231010224928.2296997-17-peter.griffin@linaro.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add minimal Tensor/GS101 SoC support and Oriole/Pixel6 board | expand |
On Tue, Oct 10, 2023 at 11:49:24PM +0100, Peter Griffin wrote: > 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? > #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"? > #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? thanks, greg k-h
Hi Greg, Thanks for the review. On Wed, 11 Oct 2023 at 08:47, Greg KH <gregkh@linuxfoundation.org> wrote: > > On Tue, Oct 10, 2023 at 11:49:24PM +0100, Peter Griffin wrote: > > 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. > > > #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 > > > #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.
On Wed, Oct 11, 2023 at 07:03:44PM +0100, Peter Griffin wrote: > Hi Greg, > > Thanks for the review. > > On Wed, 11 Oct 2023 at 08:47, Greg KH <gregkh@linuxfoundation.org> wrote: > > > > On Tue, Oct 10, 2023 at 11:49:24PM +0100, Peter Griffin wrote: > > > 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. Ah, ok, then that makes more sense, and makes sense why you would want a unique dt identifier for this device. Make that change and I'll be much happier :) thanks, greg k-h
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 }, +}; + #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) #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 }, {}, }; MODULE_DEVICE_TABLE(of, s3c24xx_uart_dt_match);
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(+)