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

["Chanho Park" <chanho61.park@xxxxxxxxxxx>]

Hi Andy,
Thanks for your reviews :)

> -----Original Message-----
> From: Andi Shyti <andi@xxxxxxxxxxx>
> Sent: Wednesday, June 29, 2022 6:43 PM
> To: Chanho Park <chanho61.park@xxxxxxxxxxx>
> Cc: Krzysztof Kozlowski <krzysztof.kozlowski@xxxxxxxxxx>; Andi Shyti
> <andi@xxxxxxxxxxx>; Mark Brown <broonie@xxxxxxxxxx>; Rob Herring
> <robh+dt@xxxxxxxxxx>; Krzysztof Kozlowski
> <krzysztof.kozlowski+dt@xxxxxxxxxx>; Alim Akhtar
<alim.akhtar@xxxxxxxxxxx>;
> devicetree@xxxxxxxxxxxxxxx; linux-spi@xxxxxxxxxxxxxxx; linux-samsung-
> soc@xxxxxxxxxxxxxxx; linux-arm-kernel@xxxxxxxxxxxxxxxxxxx
> Subject: Re: [PATCH v2 2/4] spi: s3c64xx: support custom value of internal
> clock divider
> 
> 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/

I'll correct it.

> 
> > 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@xxxxxxxxxxx>
> > ---
> >  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."

It has not been removed since v1.

> 
> [...]
> 
> > @@ -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.

It should be int to be matched with any other types of
s3c64xx_spi_port_config.

Best Regards,
Chanho Park