Re: [PATCH v2 2/4] spi: s3c64xx: support custom value of internal clock divider

[Andi Shyti <andi@etezian.org>]

Hi Chanho,

On Tue, Jun 28, 2022 at 01:42:20PM +0900, Chanho Park wrote:
> Modern exynos SoCs such as Exynos Auto v9 has different internal clock

/has/have/

> divider, for example "4". To support this internal value, this adds
> clk_div of the s3c64xx_spi_port_config and assign "2" as the default
> value to existing s3c64xx_spi_port_config.
> 
> Signed-off-by: Chanho Park <chanho61.park@samsung.com>
> ---
>  drivers/spi/spi-s3c64xx.c | 28 ++++++++++++++++++++--------
>  1 file changed, 20 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c
> index b3c50c7665fc..51a0e830441b 100644
> --- a/drivers/spi/spi-s3c64xx.c
> +++ b/drivers/spi/spi-s3c64xx.c
> @@ -131,6 +131,7 @@ struct s3c64xx_spi_dma_data {
>   * @fifo_lvl_mask: Bit-mask for {TX|RX}_FIFO_LVL bits in SPI_STATUS register.
>   * @rx_lvl_offset: Bit offset of RX_FIFO_LVL bits in SPI_STATUS regiter.
>   * @tx_st_done: Bit offset of TX_DONE bit in SPI_STATUS regiter.
> + * @clk_div: Internal clock divider, if not specified, use 2 as the default.

is it default? Is it not specified anywhere? I think you are
assigning '2' to everyone. I would just leave it "Internal
clock divider."

[...]

> @@ -871,6 +873,7 @@ static int s3c64xx_spi_setup(struct spi_device *spi)
>  	struct s3c64xx_spi_csinfo *cs = spi->controller_data;
>  	struct s3c64xx_spi_driver_data *sdd;
>  	int err;
> +	u32 div = 2;

As per Krzystof review.

>  	sdd = spi_master_get_devdata(spi->master);
>  	if (spi->dev.of_node) {
> @@ -889,22 +892,24 @@ static int s3c64xx_spi_setup(struct spi_device *spi)
>  
>  	pm_runtime_get_sync(&sdd->pdev->dev);
>  
> +	div = sdd->port_conf->clk_div;

Can you please be consistent with the data type? div is u32, but
clk_div is int.

[...]

Andi