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
