From mboxrd@z Thu Jan  1 00:00:00 1970
From: Eugeniy Paltsev <Eugeniy.Paltsev@synopsys.com>
Date: Wed, 11 Oct 2017 12:48:53 +0000
Subject: [U-Boot] [PATCH RESEND] DW SPI: Get clock value from Device Tree
In-Reply-To: <CAD6G_RQjMr5jFof1KD5VZQq9K4v-9Ez33aouwuqs9W3h=TNXEA@mail.gmail.com>
References: <20170926131009.13759-1-Eugeniy.Paltsev@synopsys.com>
 <CAD6G_RQjMr5jFof1KD5VZQq9K4v-9Ez33aouwuqs9W3h=TNXEA@mail.gmail.com>
Message-ID: <1507726133.2676.2.camel@synopsys.com>
List-Id: <u-boot.lists.denx.de>
MIME-Version: 1.0
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: 8bit
To: u-boot@lists.denx.de

Hi Jagan,
Thanks for respond, my comments are given inline.

On Wed, 2017-10-11 at 16:36 +0530, Jagan Teki wrote:
> On Tue, Sep 26, 2017 at 6:40 PM, Eugeniy Paltsev
> <Eugeniy.Paltsev@synopsys.com> wrote:
> > Add option to set spi controller clock frequency via device tree
> > using standard clock bindings.
> > Old way of setting spi controller clock frequency (via implementation
> > of 'cm_get_spi_controller_clk_hz' function in platform specific code)
> > remains supported.
> > 
> > Signed-off-by: Eugeniy Paltsev <Eugeniy.Paltsev@synopsys.com>
> > ---
> > diff --git a/drivers/spi/designware_spi.c b/drivers/spi/designware_spi.c
> > index 5aa507b..c70697e 100644
> > --- a/drivers/spi/designware_spi.c
> > +++ b/drivers/spi/designware_spi.c
> > @@ -11,6 +11,7 @@
> > +/*
> > + * cm_get_spi_controller_clk_hz function is old way to set spi controller
> > + * frequency. If it isn't implemented and spi controller frequency isn't set via
> > + * device tree we will get into next default function.
> > + */
> > +__weak unsigned int cm_get_spi_controller_clk_hz(void)
> > +{
> > +       error("SPI clock is defined neither via device tree nor via cm_get_spi_controller_clk_hz!");
> > +
> > +       return 0;
> > +}
> > +
> > +static int dw_spi_get_clk(struct udevice *bus)
> > +{
> > +       struct dw_spi_priv *priv = dev_get_priv(bus);
> > +
> > +       /* Firstly try to get clock frequency from device tree */
> > +       if (!dw_spi_of_get_clk(bus))
> > +               return 0;
> > +
> > +       /* In case of failure rollback to cm_get_spi_controller_clk_hz */
> > +       priv->bus_clk_rate = cm_get_spi_controller_clk_hz();
> 
> Why weak function, we can just return with error message since weak
> does the same job

I used weak function for backward compatibility with the previous driver version:
The previous driver version used cm_get_spi_controller_clk_hz() function which
is set outside of designware_spi.c
You can look at
   arch/arm/mach-socfpga/clock_manager_arria10.c
   arch/arm/mach-socfpga/clock_manager_gen5.c
   ...
as examples.

So I define cm_get_spi_controller_clk_hz as weak in this driver to handle both
use cases:
1) We set frequency via device tree and cm_get_spi_controller_clk_hz is NOT
   defined outside of designware_spi.c (new way)
2) We set frequency using cm_get_spi_controller_clk_hz, which is defined
   outside of designware_spi.c (old way)

I guess it is OK?
-- 
 Eugeniy Paltsev