Received-SPF: pass (google.com: domain of linux-kernel+bounces-27734-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Precedence: bulk
MIME-Version: 1.0
References: <20240110102102.61587-1-tudor.ambarus@linaro.org> <20240110102102.61587-17-tudor.ambarus@linaro.org>
In-Reply-To: <20240110102102.61587-17-tudor.ambarus@linaro.org>
From: Sam Protsenko <semen.protsenko@linaro.org>
Date: Tue, 16 Jan 2024 13:09:56 -0600
Message-ID: <CAPLW+4kndXo9Ab2PL3fts9pJNhhitX3Sr=V2b8mS4cArov4Tog@mail.gmail.com>
Subject: Re: [PATCH 16/18] tty: serial: samsung: shrink the clock selection to
 8 clocks
To: Tudor Ambarus <tudor.ambarus@linaro.org>
Cc: krzysztof.kozlowski@linaro.org, alim.akhtar@samsung.com, 
	gregkh@linuxfoundation.org, jirislaby@kernel.org, 
	linux-arm-kernel@lists.infradead.org, linux-samsung-soc@vger.kernel.org, 
	linux-kernel@vger.kernel.org, linux-serial@vger.kernel.org, 
	andre.draszik@linaro.org, peter.griffin@linaro.org, kernel-team@android.com, 
	willmcvicker@google.com
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Wed, Jan 10, 2024 at 4:25=E2=80=AFAM Tudor Ambarus <tudor.ambarus@linaro=
org> wrote:
>
> <linux/serial_s3c.h> provides a clock selection pool of maximum 4 clocks.

Then maybe it makes sense to turn those two field into 4-bit bit
fields? More importantly, what particular problem does this patch
solve, is this optimization really needed, and why? I'm not saying
it's not needed, just that commit message might've been more verbose
about this.

> Update the driver to consider a pool selection of maximum 8 clocks. The
> final scope is to reduce the memory footprint of
> ``struct s3c24xx_uart_info``.
>
> Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org>
> ---
>  drivers/tty/serial/samsung_tty.c | 22 +++++++++++-----------
>  1 file changed, 11 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/tty/serial/samsung_tty.c b/drivers/tty/serial/samsun=
g_tty.c
> index 436739cf9225..5df2bcebf9fb 100644
> --- a/drivers/tty/serial/samsung_tty.c
> +++ b/drivers/tty/serial/samsung_tty.c
> @@ -81,11 +81,11 @@ struct s3c24xx_uart_info {
>         unsigned long           tx_fifomask;
>         unsigned long           tx_fifoshift;
>         unsigned long           tx_fifofull;
> -       unsigned int            def_clk_sel;
> -       unsigned long           num_clks;
>         unsigned long           clksel_mask;
>         unsigned long           clksel_shift;
>         unsigned long           ucon_mask;
> +       u8                      def_clk_sel;
> +       u8                      num_clks;
>         u8                      iotype;
>
>         /* uart port features */
> @@ -1339,7 +1339,7 @@ static void s3c24xx_serial_pm(struct uart_port *por=
t, unsigned int level,
>
>  #define MAX_CLK_NAME_LENGTH 15
>
> -static inline int s3c24xx_serial_getsource(struct uart_port *port)
> +static inline u8 s3c24xx_serial_getsource(struct uart_port *port)
>  {
>         const struct s3c24xx_uart_info *info =3D s3c24xx_port_to_info(por=
t);
>         u32 ucon;
> @@ -1352,8 +1352,7 @@ static inline int s3c24xx_serial_getsource(struct u=
art_port *port)
>         return ucon >> info->clksel_shift;
>  }
>
> -static void s3c24xx_serial_setsource(struct uart_port *port,
> -                       unsigned int clk_sel)
> +static void s3c24xx_serial_setsource(struct uart_port *port, u8 clk_sel)
>  {
>         const struct s3c24xx_uart_info *info =3D s3c24xx_port_to_info(por=
t);
>         u32 ucon;
> @@ -1372,14 +1371,15 @@ static void s3c24xx_serial_setsource(struct uart_=
port *port,
>
>  static unsigned int s3c24xx_serial_getclk(struct s3c24xx_uart_port *ourp=
ort,
>                         unsigned int req_baud, struct clk **best_clk,
> -                       unsigned int *clk_num)
> +                       u8 *clk_num)
>  {
>         const struct s3c24xx_uart_info *info =3D ourport->info;
>         struct clk *clk;
>         unsigned long rate;
> -       unsigned int cnt, baud, quot, best_quot =3D 0;
> +       unsigned int baud, quot, best_quot =3D 0;
>         char clkname[MAX_CLK_NAME_LENGTH];
>         int calc_deviation, deviation =3D (1 << 30) - 1;
> +       u8 cnt;
>
>         for (cnt =3D 0; cnt < info->num_clks; cnt++) {
>                 /* Keep selected clock if provided */
> @@ -1472,9 +1472,10 @@ static void s3c24xx_serial_set_termios(struct uart=
_port *port,
>         struct s3c24xx_uart_port *ourport =3D to_ourport(port);
>         struct clk *clk =3D ERR_PTR(-EINVAL);
>         unsigned long flags;
> -       unsigned int baud, quot, clk_sel =3D 0;
> +       unsigned int baud, quot;
>         unsigned int udivslot =3D 0;
>         u32 ulcon, umcon;
> +       u8 clk_sel =3D 0;
>
>         /*
>          * We don't support modem control lines.
> @@ -1775,10 +1776,9 @@ static int s3c24xx_serial_enable_baudclk(struct s3=
c24xx_uart_port *ourport)
>         struct device *dev =3D ourport->port.dev;
>         const struct s3c24xx_uart_info *info =3D ourport->info;
>         char clk_name[MAX_CLK_NAME_LENGTH];
> -       unsigned int clk_sel;
>         struct clk *clk;
> -       int clk_num;
>         int ret;
> +       u8 clk_sel, clk_num;
>
>         clk_sel =3D ourport->cfg->clk_sel ? : info->def_clk_sel;
>         for (clk_num =3D 0; clk_num < info->num_clks; clk_num++) {
> @@ -2286,9 +2286,9 @@ s3c24xx_serial_get_options(struct uart_port *port, =
int *baud,
>  {
>         struct clk *clk;
>         unsigned long rate;
> -       unsigned int clk_sel;
>         u32 ulcon, ucon, ubrdiv;
>         char clk_name[MAX_CLK_NAME_LENGTH];
> +       u8 clk_sel;
>
>         ulcon  =3D rd_regl(port, S3C2410_ULCON);
>         ucon   =3D rd_regl(port, S3C2410_UCON);
> --
> 2.43.0.472.g3155946c3a-goog
>
>