| Message ID | 20161012041620.25077-2-vigneshr@ti.com |
|---|---|
| State | Superseded |
| Delegated to: | Jagannadha Sutradharudu Teki |
| Headers | show |
On Wed, Oct 12, 2016 at 9:46 AM, Vignesh R <vigneshr@ti.com> wrote: > Fix the divider calculation logic to choose a value so that the > resulting baudrate is either equal to or closest possible baudrate less > than the requested value. > > Signed-off-by: Vignesh R <vigneshr@ti.com> > --- > drivers/spi/ti_qspi.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/drivers/spi/ti_qspi.c b/drivers/spi/ti_qspi.c > index 52520dff6325..d97e2479d1b3 100644 > --- a/drivers/spi/ti_qspi.c > +++ b/drivers/spi/ti_qspi.c > @@ -16,6 +16,7 @@ > #include <asm/omap_gpio.h> > #include <asm/omap_common.h> > #include <asm/ti-common/ti-edma3.h> > +#include <linux/kernel.h> > > DECLARE_GLOBAL_DATA_PTR; > > @@ -118,7 +119,7 @@ static void ti_spi_set_speed(struct ti_qspi_priv *priv, uint hz) > if (!hz) > clk_div = 0; > else > - clk_div = (priv->fclk / hz) - 1; > + clk_div = DIV_ROUND_UP(priv->fclk, hz) - 1; Better to have a checks for min and max divider values or mask. > > debug("ti_spi_set_speed: hz: %d, clock divider %d\n", hz, clk_div); thanks!
Hi, On Thursday 13 October 2016 05:41 PM, Jagan Teki wrote: > On Wed, Oct 12, 2016 at 9:46 AM, Vignesh R <vigneshr@ti.com> wrote: >> Fix the divider calculation logic to choose a value so that the >> resulting baudrate is either equal to or closest possible baudrate less >> than the requested value. >> >> Signed-off-by: Vignesh R <vigneshr@ti.com> >> --- >> drivers/spi/ti_qspi.c | 3 ++- >> 1 file changed, 2 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/spi/ti_qspi.c b/drivers/spi/ti_qspi.c >> index 52520dff6325..d97e2479d1b3 100644 >> --- a/drivers/spi/ti_qspi.c >> +++ b/drivers/spi/ti_qspi.c >> @@ -16,6 +16,7 @@ >> #include <asm/omap_gpio.h> >> #include <asm/omap_common.h> >> #include <asm/ti-common/ti-edma3.h> >> +#include <linux/kernel.h> >> >> DECLARE_GLOBAL_DATA_PTR; >> >> @@ -118,7 +119,7 @@ static void ti_spi_set_speed(struct ti_qspi_priv *priv, uint hz) >> if (!hz) >> clk_div = 0; >> else >> - clk_div = (priv->fclk / hz) - 1; >> + clk_div = DIV_ROUND_UP(priv->fclk, hz) - 1; > > Better to have a checks for min and max divider values or mask. That code already exists in this function.
On Fri, Oct 14, 2016 at 10:54 AM, Vignesh R <vigneshr@ti.com> wrote: > Hi, > > On Thursday 13 October 2016 05:41 PM, Jagan Teki wrote: >> On Wed, Oct 12, 2016 at 9:46 AM, Vignesh R <vigneshr@ti.com> wrote: >>> Fix the divider calculation logic to choose a value so that the >>> resulting baudrate is either equal to or closest possible baudrate less >>> than the requested value. >>> >>> Signed-off-by: Vignesh R <vigneshr@ti.com> >>> --- >>> drivers/spi/ti_qspi.c | 3 ++- >>> 1 file changed, 2 insertions(+), 1 deletion(-) >>> >>> diff --git a/drivers/spi/ti_qspi.c b/drivers/spi/ti_qspi.c >>> index 52520dff6325..d97e2479d1b3 100644 >>> --- a/drivers/spi/ti_qspi.c >>> +++ b/drivers/spi/ti_qspi.c >>> @@ -16,6 +16,7 @@ >>> #include <asm/omap_gpio.h> >>> #include <asm/omap_common.h> >>> #include <asm/ti-common/ti-edma3.h> >>> +#include <linux/kernel.h> >>> >>> DECLARE_GLOBAL_DATA_PTR; >>> >>> @@ -118,7 +119,7 @@ static void ti_spi_set_speed(struct ti_qspi_priv *priv, uint hz) >>> if (!hz) >>> clk_div = 0; >>> else >>> - clk_div = (priv->fclk / hz) - 1; >>> + clk_div = DIV_ROUND_UP(priv->fclk, hz) - 1; >> >> Better to have a checks for min and max divider values or mask. > > That code already exists in this function. True but it's unnecessary to print the wrong baud prior to checking. Do the check, then print/debug and finally write reg. thanks!
On 10/14/2016 12:27 PM, Jagan Teki wrote: > On Fri, Oct 14, 2016 at 10:54 AM, Vignesh R <vigneshr@ti.com> wrote: ... >>>> DECLARE_GLOBAL_DATA_PTR; >>>> >>>> @@ -118,7 +119,7 @@ static void ti_spi_set_speed(struct ti_qspi_priv *priv, uint hz) >>>> if (!hz) >>>> clk_div = 0; >>>> else >>>> - clk_div = (priv->fclk / hz) - 1; >>>> + clk_div = DIV_ROUND_UP(priv->fclk, hz) - 1; >>> >>> Better to have a checks for min and max divider values or mask. >> >> That code already exists in this function. > > True but it's unnecessary to print the wrong baud prior to checking. > Do the check, then print/debug and finally write reg. > Posted a v2 in reply to the patch. Thanks for the review! Regards Vignesh
On Fri, Oct 14, 2016 at 4:18 PM, R, Vignesh <vigneshr@ti.com> wrote: > > > On 10/14/2016 12:27 PM, Jagan Teki wrote: >> On Fri, Oct 14, 2016 at 10:54 AM, Vignesh R <vigneshr@ti.com> wrote: > ... >>>>> DECLARE_GLOBAL_DATA_PTR; >>>>> >>>>> @@ -118,7 +119,7 @@ static void ti_spi_set_speed(struct ti_qspi_priv *priv, uint hz) >>>>> if (!hz) >>>>> clk_div = 0; >>>>> else >>>>> - clk_div = (priv->fclk / hz) - 1; >>>>> + clk_div = DIV_ROUND_UP(priv->fclk, hz) - 1; >>>> >>>> Better to have a checks for min and max divider values or mask. >>> >>> That code already exists in this function. >> >> True but it's unnecessary to print the wrong baud prior to checking. >> Do the check, then print/debug and finally write reg. >> > > Posted a v2 in reply to the patch. Thanks for the review! Please re-post the two patches again, so how I missed it. thanks!
diff --git a/drivers/spi/ti_qspi.c b/drivers/spi/ti_qspi.c index 52520dff6325..d97e2479d1b3 100644 --- a/drivers/spi/ti_qspi.c +++ b/drivers/spi/ti_qspi.c @@ -16,6 +16,7 @@ #include <asm/omap_gpio.h> #include <asm/omap_common.h> #include <asm/ti-common/ti-edma3.h> +#include <linux/kernel.h> DECLARE_GLOBAL_DATA_PTR; @@ -118,7 +119,7 @@ static void ti_spi_set_speed(struct ti_qspi_priv *priv, uint hz) if (!hz) clk_div = 0; else - clk_div = (priv->fclk / hz) - 1; + clk_div = DIV_ROUND_UP(priv->fclk, hz) - 1; debug("ti_spi_set_speed: hz: %d, clock divider %d\n", hz, clk_div);
Fix the divider calculation logic to choose a value so that the resulting baudrate is either equal to or closest possible baudrate less than the requested value. Signed-off-by: Vignesh R <vigneshr@ti.com> --- drivers/spi/ti_qspi.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)