[PATCH 0/4] Consolidate DIV_ROUND_CLOSEST_ULL()

[PATCH 0/4] Consolidate DIV_ROUND_CLOSEST_ULL()

[Javi Merino]

The kernel has grown a number of different implementations of
DIV_ROUND_CLOSEST_ULL().  That is, a macro that does the same as
DIV_ROUND_CLOSEST() but with the first operand being an unsigned long
long.  That means that you have to do the division using do_div()
instead of using the C division operator '/'.

This series move the implementation in
drivers/gpu/drm/i915/intel_drv.h to linux/kernel.h and then removes
the other similar implementations of the same code in
drivers/clk/bcm/clk-kona.h, drivers/cpuidle/governors/menu.c and
drivers/media/dvb-frontends/cxd2820r_priv.h in favor of the one in
kernel.h

Javi Merino (4):
  kernel.h: Implement DIV_ROUND_CLOSEST_ULL
  clk: bcm/kona: use DIV_ROUND_CLOSEST_ULL()
  cpuidle: menu: use DIV_ROUND_CLOSEST_ULL()
  media: cxd2820r: use DIV_ROUND_CLOSEST_ULL()

 drivers/clk/bcm/clk-kona.c                  | 28 +++++++---------------------
 drivers/clk/bcm/clk-kona.h                  |  1 -
 drivers/cpuidle/governors/menu.c            |  8 +-------
 drivers/gpu/drm/i915/intel_drv.h            |  4 +---
 drivers/media/dvb-frontends/cxd2820r_c.c    |  2 +-
 drivers/media/dvb-frontends/cxd2820r_core.c |  6 ------
 drivers/media/dvb-frontends/cxd2820r_priv.h |  2 --
 drivers/media/dvb-frontends/cxd2820r_t.c    |  2 +-
 drivers/media/dvb-frontends/cxd2820r_t2.c   |  2 +-
 include/linux/kernel.h                      | 11 +++++++++++
 10 files changed, 23 insertions(+), 43 deletions(-)

-- 
1.9.1

[PATCH 1/4] kernel.h: Implement DIV_ROUND_CLOSEST_ULL

[Javi Merino]

We have grown a number of different implementations of
DIV_ROUND_CLOSEST_ULL throughout the kernel.  Move the i915 one to
kernel.h so that it can be reused.

Cc: Daniel Vetter <daniel.vetter@intel.com>
Cc: Jani Nikula <jani.nikula@linux.intel.com>
Cc: David Airlie <airlied@linux.ie>
Cc: Darrick J. Wong <djwong@us.ibm.com>
Cc: Guenter Roeck <linux@roeck-us.net>
Cc: Andrew Morton <akpm@linux-foundation.org>
Signed-off-by: Javi Merino <javi.merino@arm.com>
---
 drivers/gpu/drm/i915/intel_drv.h |  4 +---
 include/linux/kernel.h           | 11 +++++++++++
 2 files changed, 12 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index eef79ccd0b7c..346e28fdd7dd 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -28,6 +28,7 @@
 #include <linux/async.h>
 #include <linux/i2c.h>
 #include <linux/hdmi.h>
+#include <linux/kernel.h>
 #include <drm/i915_drm.h>
 #include "i915_drv.h"
 #include <drm/drm_crtc.h>
@@ -36,9 +37,6 @@
 #include <drm/drm_dp_mst_helper.h>
 #include <drm/drm_rect.h>
 
-#define DIV_ROUND_CLOSEST_ULL(ll, d)	\
-({ unsigned long long _tmp = (ll)+(d)/2; do_div(_tmp, d); _tmp; })
-
 /**
  * _wait_for - magic (register) wait macro
  *
diff --git a/include/linux/kernel.h b/include/linux/kernel.h
index d6d630d31ef3..f7d744e9d275 100644
--- a/include/linux/kernel.h
+++ b/include/linux/kernel.h
@@ -103,6 +103,17 @@
 		(((__x) - ((__d) / 2)) / (__d));	\
 }							\
 )
+/*
+ * Same as above but for u64 dividends.  divisor must be a 32-bit
+ * number.
+ */
+#define DIV_ROUND_CLOSEST_ULL(x, divisor)(		\
+{							\
+	unsigned long long _tmp = (x) + (divisor) / 2;	\
+	do_div(_tmp, divisor);				\
+	_tmp;						\
+}							\
+)
 
 /*
  * Multiplies an integer by a fraction, while avoiding unnecessary
-- 
1.9.1

Re: [PATCH 1/4] kernel.h: Implement DIV_ROUND_CLOSEST_ULL

[Alex Elder]

On 03/20/2015 06:14 AM, Javi Merino wrote:
> We have grown a number of different implementations of
> DIV_ROUND_CLOSEST_ULL throughout the kernel.  Move the i915 one to
> kernel.h so that it can be reused.
> 
> Cc: Daniel Vetter <daniel.vetter@intel.com>
> Cc: Jani Nikula <jani.nikula@linux.intel.com>
> Cc: David Airlie <airlied@linux.ie>
> Cc: Darrick J. Wong <djwong@us.ibm.com>
> Cc: Guenter Roeck <linux@roeck-us.net>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Signed-off-by: Javi Merino <javi.merino@arm.com>
> ---
>  drivers/gpu/drm/i915/intel_drv.h |  4 +---
>  include/linux/kernel.h           | 11 +++++++++++
>  2 files changed, 12 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index eef79ccd0b7c..346e28fdd7dd 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -28,6 +28,7 @@
>  #include <linux/async.h>
>  #include <linux/i2c.h>
>  #include <linux/hdmi.h>
> +#include <linux/kernel.h>
>  #include <drm/i915_drm.h>
>  #include "i915_drv.h"
>  #include <drm/drm_crtc.h>
> @@ -36,9 +37,6 @@
>  #include <drm/drm_dp_mst_helper.h>
>  #include <drm/drm_rect.h>
>  
> -#define DIV_ROUND_CLOSEST_ULL(ll, d)	\
> -({ unsigned long long _tmp = (ll)+(d)/2; do_div(_tmp, d); _tmp; })
> -
>  /**
>   * _wait_for - magic (register) wait macro
>   *
> diff --git a/include/linux/kernel.h b/include/linux/kernel.h
> index d6d630d31ef3..f7d744e9d275 100644
> --- a/include/linux/kernel.h
> +++ b/include/linux/kernel.h
> @@ -103,6 +103,17 @@
>  		(((__x) - ((__d) / 2)) / (__d));	\
>  }							\
>  )
> +/*
> + * Same as above but for u64 dividends.  divisor must be a 32-bit
> + * number.
> + */
> +#define DIV_ROUND_CLOSEST_ULL(x, divisor)(		\
> +{							\
> +	unsigned long long _tmp = (x) + (divisor) / 2;	\
> +	do_div(_tmp, divisor);				\
> +	_tmp;						\
> +}							\
> +)

Since you are stipulating the types of the arguments, this
should be defined as a static inline function instead.

DIV_ROUND_CLOSEST() could conceivably handle a 64-bit
dividend properly, although that macro is already a bit
hard to look at.

					-Alex

>  
>  /*
>   * Multiplies an integer by a fraction, while avoiding unnecessary
> 

Re: [PATCH 1/4] kernel.h: Implement DIV_ROUND_CLOSEST_ULL

On 20 March 2015 at 11:14, Javi Merino <javi.merino@arm.com> wrote:
> We have grown a number of different implementations of
> DIV_ROUND_CLOSEST_ULL throughout the kernel.  Move the i915 one to
> kernel.h so that it can be reused.
>
> Cc: Daniel Vetter <daniel.vetter@intel.com>
> Cc: Jani Nikula <jani.nikula@linux.intel.com>
> Cc: David Airlie <airlied@linux.ie>
> Cc: Darrick J. Wong <djwong@us.ibm.com>
> Cc: Guenter Roeck <linux@roeck-us.net>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Signed-off-by: Javi Merino <javi.merino@arm.com>
> ---
>  drivers/gpu/drm/i915/intel_drv.h |  4 +---
>  include/linux/kernel.h           | 11 +++++++++++
>  2 files changed, 12 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index eef79ccd0b7c..346e28fdd7dd 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -28,6 +28,7 @@
>  #include <linux/async.h>
>  #include <linux/i2c.h>
>  #include <linux/hdmi.h>
> +#include <linux/kernel.h>
Hi Javi,

Small suggestion - can we include the header only where needed ?
i915/intel_panel.c seems to be the only user of DIV_ROUND_CLOSEST
which will need an update.

Somewhat trivial pick but it will prevent ~40 unnecessary dives in kernel.h.

Cheers,
Emil

Re: [PATCH 1/4] kernel.h: Implement DIV_ROUND_CLOSEST_ULL

[Emil Velikov]

On 20 March 2015 at 11:14, Javi Merino <javi.merino@arm.com> wrote:
> We have grown a number of different implementations of
> DIV_ROUND_CLOSEST_ULL throughout the kernel.  Move the i915 one to
> kernel.h so that it can be reused.
>
> Cc: Daniel Vetter <daniel.vetter@intel.com>
> Cc: Jani Nikula <jani.nikula@linux.intel.com>
> Cc: David Airlie <airlied@linux.ie>
> Cc: Darrick J. Wong <djwong@us.ibm.com>
> Cc: Guenter Roeck <linux@roeck-us.net>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Signed-off-by: Javi Merino <javi.merino@arm.com>
> ---
>  drivers/gpu/drm/i915/intel_drv.h |  4 +---
>  include/linux/kernel.h           | 11 +++++++++++
>  2 files changed, 12 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index eef79ccd0b7c..346e28fdd7dd 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -28,6 +28,7 @@
>  #include <linux/async.h>
>  #include <linux/i2c.h>
>  #include <linux/hdmi.h>
> +#include <linux/kernel.h>
Hi Javi,

Small suggestion - can we include the header only where needed ?
i915/intel_panel.c seems to be the only user of DIV_ROUND_CLOSEST
which will need an update.

Somewhat trivial pick but it will prevent ~40 unnecessary dives in kernel.h.

Cheers,
Emil
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/4] kernel.h: Implement DIV_ROUND_CLOSEST_ULL

[Javi Merino]

On Fri, Mar 20, 2015 at 06:19:26PM +0000, Emil Velikov wrote:
> On 20 March 2015 at 11:14, Javi Merino <javi.merino@arm.com> wrote:
> > We have grown a number of different implementations of
> > DIV_ROUND_CLOSEST_ULL throughout the kernel.  Move the i915 one to
> > kernel.h so that it can be reused.
> >
> > Cc: Daniel Vetter <daniel.vetter@intel.com>
> > Cc: Jani Nikula <jani.nikula@linux.intel.com>
> > Cc: David Airlie <airlied@linux.ie>
> > Cc: Darrick J. Wong <djwong@us.ibm.com>
> > Cc: Guenter Roeck <linux@roeck-us.net>
> > Cc: Andrew Morton <akpm@linux-foundation.org>
> > Signed-off-by: Javi Merino <javi.merino@arm.com>
> > ---
> >  drivers/gpu/drm/i915/intel_drv.h |  4 +---
> >  include/linux/kernel.h           | 11 +++++++++++
> >  2 files changed, 12 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > index eef79ccd0b7c..346e28fdd7dd 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -28,6 +28,7 @@
> >  #include <linux/async.h>
> >  #include <linux/i2c.h>
> >  #include <linux/hdmi.h>
> > +#include <linux/kernel.h>
> Hi Javi,
> 
> Small suggestion - can we include the header only where needed ?
> i915/intel_panel.c seems to be the only user of DIV_ROUND_CLOSEST
> which will need an update.
> 
> Somewhat trivial pick but it will prevent ~40 unnecessary dives in kernel.h.

Sure, I'll fix it in the next version of the series.

Cheers,
Javi

Re: [PATCH 1/4] kernel.h: Implement DIV_ROUND_CLOSEST_ULL

[Daniel Vetter]

On Fri, Mar 20, 2015 at 11:14:40AM +0000, Javi Merino wrote:
> We have grown a number of different implementations of
> DIV_ROUND_CLOSEST_ULL throughout the kernel.  Move the i915 one to
> kernel.h so that it can be reused.
> 
> Cc: Daniel Vetter <daniel.vetter@intel.com>
> Cc: Jani Nikula <jani.nikula@linux.intel.com>
> Cc: David Airlie <airlied@linux.ie>
> Cc: Darrick J. Wong <djwong@us.ibm.com>
> Cc: Guenter Roeck <linux@roeck-us.net>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Signed-off-by: Javi Merino <javi.merino@arm.com>

Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch>

And preemptive ack for the next version with the includes at different
places.
-Daniel

> ---
>  drivers/gpu/drm/i915/intel_drv.h |  4 +---
>  include/linux/kernel.h           | 11 +++++++++++
>  2 files changed, 12 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index eef79ccd0b7c..346e28fdd7dd 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -28,6 +28,7 @@
>  #include <linux/async.h>
>  #include <linux/i2c.h>
>  #include <linux/hdmi.h>
> +#include <linux/kernel.h>
>  #include <drm/i915_drm.h>
>  #include "i915_drv.h"
>  #include <drm/drm_crtc.h>
> @@ -36,9 +37,6 @@
>  #include <drm/drm_dp_mst_helper.h>
>  #include <drm/drm_rect.h>
>  
> -#define DIV_ROUND_CLOSEST_ULL(ll, d)	\
> -({ unsigned long long _tmp = (ll)+(d)/2; do_div(_tmp, d); _tmp; })
> -
>  /**
>   * _wait_for - magic (register) wait macro
>   *
> diff --git a/include/linux/kernel.h b/include/linux/kernel.h
> index d6d630d31ef3..f7d744e9d275 100644
> --- a/include/linux/kernel.h
> +++ b/include/linux/kernel.h
> @@ -103,6 +103,17 @@
>  		(((__x) - ((__d) / 2)) / (__d));	\
>  }							\
>  )
> +/*
> + * Same as above but for u64 dividends.  divisor must be a 32-bit
> + * number.
> + */
> +#define DIV_ROUND_CLOSEST_ULL(x, divisor)(		\
> +{							\
> +	unsigned long long _tmp = (x) + (divisor) / 2;	\
> +	do_div(_tmp, divisor);				\
> +	_tmp;						\
> +}							\
> +)
>  
>  /*
>   * Multiplies an integer by a fraction, while avoiding unnecessary
> -- 
> 1.9.1
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH 1/4] kernel.h: Implement DIV_ROUND_CLOSEST_ULL

[Daniel Vetter]

On Fri, Mar 20, 2015 at 11:14:40AM +0000, Javi Merino wrote:
> We have grown a number of different implementations of
> DIV_ROUND_CLOSEST_ULL throughout the kernel.  Move the i915 one to
> kernel.h so that it can be reused.
> 
> Cc: Daniel Vetter <daniel.vetter@intel.com>
> Cc: Jani Nikula <jani.nikula@linux.intel.com>
> Cc: David Airlie <airlied@linux.ie>
> Cc: Darrick J. Wong <djwong@us.ibm.com>
> Cc: Guenter Roeck <linux@roeck-us.net>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Signed-off-by: Javi Merino <javi.merino@arm.com>

Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch>

And preemptive ack for the next version with the includes at different
places.
-Daniel

> ---
>  drivers/gpu/drm/i915/intel_drv.h |  4 +---
>  include/linux/kernel.h           | 11 +++++++++++
>  2 files changed, 12 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index eef79ccd0b7c..346e28fdd7dd 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -28,6 +28,7 @@
>  #include <linux/async.h>
>  #include <linux/i2c.h>
>  #include <linux/hdmi.h>
> +#include <linux/kernel.h>
>  #include <drm/i915_drm.h>
>  #include "i915_drv.h"
>  #include <drm/drm_crtc.h>
> @@ -36,9 +37,6 @@
>  #include <drm/drm_dp_mst_helper.h>
>  #include <drm/drm_rect.h>
>  
> -#define DIV_ROUND_CLOSEST_ULL(ll, d)	\
> -({ unsigned long long _tmp = (ll)+(d)/2; do_div(_tmp, d); _tmp; })
> -
>  /**
>   * _wait_for - magic (register) wait macro
>   *
> diff --git a/include/linux/kernel.h b/include/linux/kernel.h
> index d6d630d31ef3..f7d744e9d275 100644
> --- a/include/linux/kernel.h
> +++ b/include/linux/kernel.h
> @@ -103,6 +103,17 @@
>  		(((__x) - ((__d) / 2)) / (__d));	\
>  }							\
>  )
> +/*
> + * Same as above but for u64 dividends.  divisor must be a 32-bit
> + * number.
> + */
> +#define DIV_ROUND_CLOSEST_ULL(x, divisor)(		\
> +{							\
> +	unsigned long long _tmp = (x) + (divisor) / 2;	\
> +	do_div(_tmp, divisor);				\
> +	_tmp;						\
> +}							\
> +)
>  
>  /*
>   * Multiplies an integer by a fraction, while avoiding unnecessary
> -- 
> 1.9.1
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/4] kernel.h: Implement DIV_ROUND_CLOSEST_ULL

[Jeff Epler]

On Fri, Mar 20, 2015 at 11:14:40AM +0000, Javi Merino wrote:
> +/*
> + * Same as above but for u64 dividends.  divisor must be a 32-bit
> + * number.
> + */
> +#define DIV_ROUND_CLOSEST_ULL(x, divisor)(		\
> +{							\
> +	unsigned long long _tmp = (x) + (divisor) / 2;	\
> +	do_div(_tmp, divisor);				\
> +	_tmp;						\
> +}							\
> +)

The macro evaluates 'divisor' twice.

Jeff

Re: [PATCH 1/4] kernel.h: Implement DIV_ROUND_CLOSEST_ULL

[Javi Merino]

On Mon, Mar 23, 2015 at 12:34:04PM +0000, Jeff Epler wrote:
> On Fri, Mar 20, 2015 at 11:14:40AM +0000, Javi Merino wrote:
> > +/*
> > + * Same as above but for u64 dividends.  divisor must be a 32-bit
> > + * number.
> > + */
> > +#define DIV_ROUND_CLOSEST_ULL(x, divisor)(		\
> > +{							\
> > +	unsigned long long _tmp = (x) + (divisor) / 2;	\
> > +	do_div(_tmp, divisor);				\
> > +	_tmp;						\
> > +}							\
> > +)
> 
> The macro evaluates 'divisor' twice.

Good catch.  That needs to be fixed.  I could do the typeof trick that
DIV_ROUND_CLOSEST() does but it's probably better to just create a
static function as Alex Elder suggests.  I'll send a v2 tomorrow with
a static function instead.

Cheers,
Javi

Re: [PATCH 1/4] kernel.h: Implement DIV_ROUND_CLOSEST_ULL

[Javi Merino]

On Mon, Mar 23, 2015 at 12:34:04PM +0000, Jeff Epler wrote:
> On Fri, Mar 20, 2015 at 11:14:40AM +0000, Javi Merino wrote:
> > +/*
> > + * Same as above but for u64 dividends.  divisor must be a 32-bit
> > + * number.
> > + */
> > +#define DIV_ROUND_CLOSEST_ULL(x, divisor)(		\
> > +{							\
> > +	unsigned long long _tmp = (x) + (divisor) / 2;	\
> > +	do_div(_tmp, divisor);				\
> > +	_tmp;						\
> > +}							\
> > +)
> 
> The macro evaluates 'divisor' twice.

Good catch.  That needs to be fixed.  I could do the typeof trick that
DIV_ROUND_CLOSEST() does but it's probably better to just create a
static function as Alex Elder suggests.  I'll send a v2 tomorrow with
a static function instead.

Cheers,
Javi

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/4] kernel.h: Implement DIV_ROUND_CLOSEST_ULL

[Javi Merino]

On Mon, Mar 23, 2015 at 05:04:16PM +0000, Javi Merino wrote:
> On Mon, Mar 23, 2015 at 12:34:04PM +0000, Jeff Epler wrote:
> > On Fri, Mar 20, 2015 at 11:14:40AM +0000, Javi Merino wrote:
> > > +/*
> > > + * Same as above but for u64 dividends.  divisor must be a 32-bit
> > > + * number.
> > > + */
> > > +#define DIV_ROUND_CLOSEST_ULL(x, divisor)(		\
> > > +{							\
> > > +	unsigned long long _tmp = (x) + (divisor) / 2;	\
> > > +	do_div(_tmp, divisor);				\
> > > +	_tmp;						\
> > > +}							\
> > > +)
> > 
> > The macro evaluates 'divisor' twice.
> 
> Good catch.  That needs to be fixed.  I could do the typeof trick that
> DIV_ROUND_CLOSEST() does but it's probably better to just create a
> static function as Alex Elder suggests.  I'll send a v2 tomorrow with
> a static function instead.

Nah, thinking about it and seeing the other macros in this file
(esp. DIV_ROUND_UP() and DIV_ROUND_UP_ULL()) I don't think a static
function gives you anything.  I'll just modify the macro to look similar
to DIV_ROUND_CLOSEST()

Cheers,
Javi

Re: [PATCH 1/4] kernel.h: Implement DIV_ROUND_CLOSEST_ULL

[Javi Merino]

On Mon, Mar 23, 2015 at 05:04:16PM +0000, Javi Merino wrote:
> On Mon, Mar 23, 2015 at 12:34:04PM +0000, Jeff Epler wrote:
> > On Fri, Mar 20, 2015 at 11:14:40AM +0000, Javi Merino wrote:
> > > +/*
> > > + * Same as above but for u64 dividends.  divisor must be a 32-bit
> > > + * number.
> > > + */
> > > +#define DIV_ROUND_CLOSEST_ULL(x, divisor)(		\
> > > +{							\
> > > +	unsigned long long _tmp = (x) + (divisor) / 2;	\
> > > +	do_div(_tmp, divisor);				\
> > > +	_tmp;						\
> > > +}							\
> > > +)
> > 
> > The macro evaluates 'divisor' twice.
> 
> Good catch.  That needs to be fixed.  I could do the typeof trick that
> DIV_ROUND_CLOSEST() does but it's probably better to just create a
> static function as Alex Elder suggests.  I'll send a v2 tomorrow with
> a static function instead.

Nah, thinking about it and seeing the other macros in this file
(esp. DIV_ROUND_UP() and DIV_ROUND_UP_ULL()) I don't think a static
function gives you anything.  I'll just modify the macro to look similar
to DIV_ROUND_CLOSEST()

Cheers,
Javi

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 2/4] clk: bcm/kona: use DIV_ROUND_CLOSEST_ULL()

[Javi Merino]

Now that the kernel provides DIV_ROUND_CLOSEST_ULL(), drop the internal
implementation and use the kernel one.

Cc: Mike Turquette <mturquette@linaro.org>
Cc: Stephen Boyd <sboyd@codeaurora.org>
Cc: Alex Elder <elder@linaro.org>
Signed-off-by: Javi Merino <javi.merino@arm.com>
---
I've only compile-tested this, I don't have the hardware to test it.

 drivers/clk/bcm/clk-kona.c | 28 +++++++---------------------
 drivers/clk/bcm/clk-kona.h |  1 -
 2 files changed, 7 insertions(+), 22 deletions(-)

diff --git a/drivers/clk/bcm/clk-kona.c b/drivers/clk/bcm/clk-kona.c
index 05abae89262e..a0ef4f75d457 100644
--- a/drivers/clk/bcm/clk-kona.c
+++ b/drivers/clk/bcm/clk-kona.c
@@ -15,6 +15,7 @@
 #include "clk-kona.h"
 
 #include <linux/delay.h>
+#include <linux/kernel.h>
 
 /*
  * "Policies" affect the frequencies of bus clocks provided by a
@@ -51,21 +52,6 @@ static inline u32 bitfield_replace(u32 reg_val, u32 shift, u32 width, u32 val)
 
 /* Divider and scaling helpers */
 
-/*
- * Implement DIV_ROUND_CLOSEST() for 64-bit dividend and both values
- * unsigned.  Note that unlike do_div(), the remainder is discarded
- * and the return value is the quotient (not the remainder).
- */
-u64 do_div_round_closest(u64 dividend, unsigned long divisor)
-{
-	u64 result;
-
-	result = dividend + ((u64)divisor >> 1);
-	(void)do_div(result, divisor);
-
-	return result;
-}
-
 /* Convert a divider into the scaled divisor value it represents. */
 static inline u64 scaled_div_value(struct bcm_clk_div *div, u32 reg_div)
 {
@@ -87,7 +73,7 @@ u64 scaled_div_build(struct bcm_clk_div *div, u32 div_value, u32 billionths)
 	combined = (u64)div_value * BILLION + billionths;
 	combined <<= div->u.s.frac_width;
 
-	return do_div_round_closest(combined, BILLION);
+	return DIV_ROUND_CLOSEST_ULL(combined, BILLION);
 }
 
 /* The scaled minimum divisor representable by a divider */
@@ -731,7 +717,7 @@ static unsigned long clk_recalc_rate(struct ccu_data *ccu,
 		scaled_rate = scale_rate(pre_div, parent_rate);
 		scaled_rate = scale_rate(div, scaled_rate);
 		scaled_div = divider_read_scaled(ccu, pre_div);
-		scaled_parent_rate = do_div_round_closest(scaled_rate,
+		scaled_parent_rate = DIV_ROUND_CLOSEST_ULL(scaled_rate,
 							scaled_div);
 	} else  {
 		scaled_parent_rate = scale_rate(div, parent_rate);
@@ -743,7 +729,7 @@ static unsigned long clk_recalc_rate(struct ccu_data *ccu,
 	 * rate.
 	 */
 	scaled_div = divider_read_scaled(ccu, div);
-	result = do_div_round_closest(scaled_parent_rate, scaled_div);
+	result = DIV_ROUND_CLOSEST_ULL(scaled_parent_rate, scaled_div);
 
 	return (unsigned long)result;
 }
@@ -790,7 +776,7 @@ static long round_rate(struct ccu_data *ccu, struct bcm_clk_div *div,
 		scaled_rate = scale_rate(pre_div, parent_rate);
 		scaled_rate = scale_rate(div, scaled_rate);
 		scaled_pre_div = divider_read_scaled(ccu, pre_div);
-		scaled_parent_rate = do_div_round_closest(scaled_rate,
+		scaled_parent_rate = DIV_ROUND_CLOSEST_ULL(scaled_rate,
 							scaled_pre_div);
 	} else {
 		scaled_parent_rate = scale_rate(div, parent_rate);
@@ -802,7 +788,7 @@ static long round_rate(struct ccu_data *ccu, struct bcm_clk_div *div,
 	 * the best we can do.
 	 */
 	if (!divider_is_fixed(div)) {
-		best_scaled_div = do_div_round_closest(scaled_parent_rate,
+		best_scaled_div = DIV_ROUND_CLOSEST_ULL(scaled_parent_rate,
 							rate);
 		min_scaled_div = scaled_div_min(div);
 		max_scaled_div = scaled_div_max(div);
@@ -815,7 +801,7 @@ static long round_rate(struct ccu_data *ccu, struct bcm_clk_div *div,
 	}
 
 	/* OK, figure out the resulting rate */
-	result = do_div_round_closest(scaled_parent_rate, best_scaled_div);
+	result = DIV_ROUND_CLOSEST_ULL(scaled_parent_rate, best_scaled_div);
 
 	if (scaled_div)
 		*scaled_div = best_scaled_div;
diff --git a/drivers/clk/bcm/clk-kona.h b/drivers/clk/bcm/clk-kona.h
index 2537b3072910..6849a64baf6d 100644
--- a/drivers/clk/bcm/clk-kona.h
+++ b/drivers/clk/bcm/clk-kona.h
@@ -503,7 +503,6 @@ extern struct clk_ops kona_peri_clk_ops;
 
 /* Externally visible functions */
 
-extern u64 do_div_round_closest(u64 dividend, unsigned long divisor);
 extern u64 scaled_div_max(struct bcm_clk_div *div);
 extern u64 scaled_div_build(struct bcm_clk_div *div, u32 div_value,
 				u32 billionths);
-- 
1.9.1

[PATCH 3/4] cpuidle: menu: use DIV_ROUND_CLOSEST_ULL()

[Javi Merino]

Now that the kernel provides DIV_ROUND_CLOSEST_ULL(), drop the internal
implementation and use the kernel one.

Cc: "Rafael J. Wysocki" <rafael.j.wysocki@intel.com>
Cc: Mel Gorman <mgorman@suse.de>
Cc: Stephen Hemminger <shemminger@linux-foundation.org>
Signed-off-by: Javi Merino <javi.merino@arm.com>
---
 drivers/cpuidle/governors/menu.c | 8 +-------
 1 file changed, 1 insertion(+), 7 deletions(-)

diff --git a/drivers/cpuidle/governors/menu.c b/drivers/cpuidle/governors/menu.c
index 40580794e23d..b8a5fa15ca24 100644
--- a/drivers/cpuidle/governors/menu.c
+++ b/drivers/cpuidle/governors/menu.c
@@ -190,12 +190,6 @@ static DEFINE_PER_CPU(struct menu_device, menu_devices);
 
 static void menu_update(struct cpuidle_driver *drv, struct cpuidle_device *dev);
 
-/* This implements DIV_ROUND_CLOSEST but avoids 64 bit division */
-static u64 div_round64(u64 dividend, u32 divisor)
-{
-	return div_u64(dividend + (divisor / 2), divisor);
-}
-
 /*
  * Try detecting repeating patterns by keeping track of the last 8
  * intervals, and checking if the standard deviation of that set
@@ -317,7 +311,7 @@ static int menu_select(struct cpuidle_driver *drv, struct cpuidle_device *dev)
 	 * operands are 32 bits.
 	 * Make sure to round up for half microseconds.
 	 */
-	data->predicted_us = div_round64((uint64_t)data->next_timer_us *
+	data->predicted_us = DIV_ROUND_CLOSEST_ULL((uint64_t)data->next_timer_us *
 					 data->correction_factor[data->bucket],
 					 RESOLUTION * DECAY);
 
-- 
1.9.1

[PATCH 4/4] media: cxd2820r: use DIV_ROUND_CLOSEST_ULL()

[Javi Merino]

Now that the kernel provides DIV_ROUND_CLOSEST_ULL(), drop the internal
implementation and use the kernel one.

Cc: Antti Palosaari <crope@iki.fi>
Cc: Mauro Carvalho Chehab <mchehab@osg.samsung.com>
Signed-off-by: Javi Merino <javi.merino@arm.com>
---
I've only compile-tested it, I don't have the hardware to run it.

 drivers/media/dvb-frontends/cxd2820r_c.c    | 2 +-
 drivers/media/dvb-frontends/cxd2820r_core.c | 6 ------
 drivers/media/dvb-frontends/cxd2820r_priv.h | 2 --
 drivers/media/dvb-frontends/cxd2820r_t.c    | 2 +-
 drivers/media/dvb-frontends/cxd2820r_t2.c   | 2 +-
 5 files changed, 3 insertions(+), 11 deletions(-)

diff --git a/drivers/media/dvb-frontends/cxd2820r_c.c b/drivers/media/dvb-frontends/cxd2820r_c.c
index 149fdca3fb44..72b0e2db3aab 100644
--- a/drivers/media/dvb-frontends/cxd2820r_c.c
+++ b/drivers/media/dvb-frontends/cxd2820r_c.c
@@ -79,7 +79,7 @@ int cxd2820r_set_frontend_c(struct dvb_frontend *fe)
 
 	num = if_freq / 1000; /* Hz => kHz */
 	num *= 0x4000;
-	if_ctl = 0x4000 - cxd2820r_div_u64_round_closest(num, 41000);
+	if_ctl = 0x4000 - DIV_ROUND_CLOSEST_ULL(num, 41000);
 	buf[0] = (if_ctl >> 8) & 0x3f;
 	buf[1] = (if_ctl >> 0) & 0xff;
 
diff --git a/drivers/media/dvb-frontends/cxd2820r_core.c b/drivers/media/dvb-frontends/cxd2820r_core.c
index 422e84bbb008..490e090048ef 100644
--- a/drivers/media/dvb-frontends/cxd2820r_core.c
+++ b/drivers/media/dvb-frontends/cxd2820r_core.c
@@ -244,12 +244,6 @@ error:
 	return ret;
 }
 
-/* 64 bit div with round closest, like DIV_ROUND_CLOSEST but 64 bit */
-u32 cxd2820r_div_u64_round_closest(u64 dividend, u32 divisor)
-{
-	return div_u64(dividend + (divisor / 2), divisor);
-}
-
 static int cxd2820r_set_frontend(struct dvb_frontend *fe)
 {
 	struct cxd2820r_priv *priv = fe->demodulator_priv;
diff --git a/drivers/media/dvb-frontends/cxd2820r_priv.h b/drivers/media/dvb-frontends/cxd2820r_priv.h
index 7ff5f60c83e1..4b428959b16e 100644
--- a/drivers/media/dvb-frontends/cxd2820r_priv.h
+++ b/drivers/media/dvb-frontends/cxd2820r_priv.h
@@ -64,8 +64,6 @@ int cxd2820r_wr_reg_mask(struct cxd2820r_priv *priv, u32 reg, u8 val,
 int cxd2820r_wr_regs(struct cxd2820r_priv *priv, u32 reginfo, u8 *val,
 	int len);
 
-u32 cxd2820r_div_u64_round_closest(u64 dividend, u32 divisor);
-
 int cxd2820r_wr_regs(struct cxd2820r_priv *priv, u32 reginfo, u8 *val,
 	int len);
 
diff --git a/drivers/media/dvb-frontends/cxd2820r_t.c b/drivers/media/dvb-frontends/cxd2820r_t.c
index 51401d036530..008cb2ac8480 100644
--- a/drivers/media/dvb-frontends/cxd2820r_t.c
+++ b/drivers/media/dvb-frontends/cxd2820r_t.c
@@ -103,7 +103,7 @@ int cxd2820r_set_frontend_t(struct dvb_frontend *fe)
 
 	num = if_freq / 1000; /* Hz => kHz */
 	num *= 0x1000000;
-	if_ctl = cxd2820r_div_u64_round_closest(num, 41000);
+	if_ctl = DIV_ROUND_CLOSEST_ULL(num, 41000);
 	buf[0] = ((if_ctl >> 16) & 0xff);
 	buf[1] = ((if_ctl >>  8) & 0xff);
 	buf[2] = ((if_ctl >>  0) & 0xff);
diff --git a/drivers/media/dvb-frontends/cxd2820r_t2.c b/drivers/media/dvb-frontends/cxd2820r_t2.c
index 9c0c4f42175c..35fe364c7182 100644
--- a/drivers/media/dvb-frontends/cxd2820r_t2.c
+++ b/drivers/media/dvb-frontends/cxd2820r_t2.c
@@ -120,7 +120,7 @@ int cxd2820r_set_frontend_t2(struct dvb_frontend *fe)
 
 	num = if_freq / 1000; /* Hz => kHz */
 	num *= 0x1000000;
-	if_ctl = cxd2820r_div_u64_round_closest(num, 41000);
+	if_ctl = DIV_ROUND_CLOSEST_ULL(num, 41000);
 	buf[0] = ((if_ctl >> 16) & 0xff);
 	buf[1] = ((if_ctl >>  8) & 0xff);
 	buf[2] = ((if_ctl >>  0) & 0xff);
-- 
1.9.1

Re: [PATCH 4/4] media: cxd2820r: use DIV_ROUND_CLOSEST_ULL()

[Antti Palosaari]

On 03/20/2015 01:14 PM, Javi Merino wrote:
> Now that the kernel provides DIV_ROUND_CLOSEST_ULL(), drop the internal
> implementation and use the kernel one.
>
> Cc: Antti Palosaari <crope@iki.fi>
> Cc: Mauro Carvalho Chehab <mchehab@osg.samsung.com>
> Signed-off-by: Javi Merino <javi.merino@arm.com>

Acked-by: Antti Palosaari <crope@iki.fi>
Reviewed-by: Antti Palosaari <crope@iki.fi>


Antti

> ---
> I've only compile-tested it, I don't have the hardware to run it.
>
>   drivers/media/dvb-frontends/cxd2820r_c.c    | 2 +-
>   drivers/media/dvb-frontends/cxd2820r_core.c | 6 ------
>   drivers/media/dvb-frontends/cxd2820r_priv.h | 2 --
>   drivers/media/dvb-frontends/cxd2820r_t.c    | 2 +-
>   drivers/media/dvb-frontends/cxd2820r_t2.c   | 2 +-
>   5 files changed, 3 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/media/dvb-frontends/cxd2820r_c.c b/drivers/media/dvb-frontends/cxd2820r_c.c
> index 149fdca3fb44..72b0e2db3aab 100644
> --- a/drivers/media/dvb-frontends/cxd2820r_c.c
> +++ b/drivers/media/dvb-frontends/cxd2820r_c.c
> @@ -79,7 +79,7 @@ int cxd2820r_set_frontend_c(struct dvb_frontend *fe)
>
>   	num = if_freq / 1000; /* Hz => kHz */
>   	num *= 0x4000;
> -	if_ctl = 0x4000 - cxd2820r_div_u64_round_closest(num, 41000);
> +	if_ctl = 0x4000 - DIV_ROUND_CLOSEST_ULL(num, 41000);
>   	buf[0] = (if_ctl >> 8) & 0x3f;
>   	buf[1] = (if_ctl >> 0) & 0xff;
>
> diff --git a/drivers/media/dvb-frontends/cxd2820r_core.c b/drivers/media/dvb-frontends/cxd2820r_core.c
> index 422e84bbb008..490e090048ef 100644
> --- a/drivers/media/dvb-frontends/cxd2820r_core.c
> +++ b/drivers/media/dvb-frontends/cxd2820r_core.c
> @@ -244,12 +244,6 @@ error:
>   	return ret;
>   }
>
> -/* 64 bit div with round closest, like DIV_ROUND_CLOSEST but 64 bit */
> -u32 cxd2820r_div_u64_round_closest(u64 dividend, u32 divisor)
> -{
> -	return div_u64(dividend + (divisor / 2), divisor);
> -}
> -
>   static int cxd2820r_set_frontend(struct dvb_frontend *fe)
>   {
>   	struct cxd2820r_priv *priv = fe->demodulator_priv;
> diff --git a/drivers/media/dvb-frontends/cxd2820r_priv.h b/drivers/media/dvb-frontends/cxd2820r_priv.h
> index 7ff5f60c83e1..4b428959b16e 100644
> --- a/drivers/media/dvb-frontends/cxd2820r_priv.h
> +++ b/drivers/media/dvb-frontends/cxd2820r_priv.h
> @@ -64,8 +64,6 @@ int cxd2820r_wr_reg_mask(struct cxd2820r_priv *priv, u32 reg, u8 val,
>   int cxd2820r_wr_regs(struct cxd2820r_priv *priv, u32 reginfo, u8 *val,
>   	int len);
>
> -u32 cxd2820r_div_u64_round_closest(u64 dividend, u32 divisor);
> -
>   int cxd2820r_wr_regs(struct cxd2820r_priv *priv, u32 reginfo, u8 *val,
>   	int len);
>
> diff --git a/drivers/media/dvb-frontends/cxd2820r_t.c b/drivers/media/dvb-frontends/cxd2820r_t.c
> index 51401d036530..008cb2ac8480 100644
> --- a/drivers/media/dvb-frontends/cxd2820r_t.c
> +++ b/drivers/media/dvb-frontends/cxd2820r_t.c
> @@ -103,7 +103,7 @@ int cxd2820r_set_frontend_t(struct dvb_frontend *fe)
>
>   	num = if_freq / 1000; /* Hz => kHz */
>   	num *= 0x1000000;
> -	if_ctl = cxd2820r_div_u64_round_closest(num, 41000);
> +	if_ctl = DIV_ROUND_CLOSEST_ULL(num, 41000);
>   	buf[0] = ((if_ctl >> 16) & 0xff);
>   	buf[1] = ((if_ctl >>  8) & 0xff);
>   	buf[2] = ((if_ctl >>  0) & 0xff);
> diff --git a/drivers/media/dvb-frontends/cxd2820r_t2.c b/drivers/media/dvb-frontends/cxd2820r_t2.c
> index 9c0c4f42175c..35fe364c7182 100644
> --- a/drivers/media/dvb-frontends/cxd2820r_t2.c
> +++ b/drivers/media/dvb-frontends/cxd2820r_t2.c
> @@ -120,7 +120,7 @@ int cxd2820r_set_frontend_t2(struct dvb_frontend *fe)
>
>   	num = if_freq / 1000; /* Hz => kHz */
>   	num *= 0x1000000;
> -	if_ctl = cxd2820r_div_u64_round_closest(num, 41000);
> +	if_ctl = DIV_ROUND_CLOSEST_ULL(num, 41000);
>   	buf[0] = ((if_ctl >> 16) & 0xff);
>   	buf[1] = ((if_ctl >>  8) & 0xff);
>   	buf[2] = ((if_ctl >>  0) & 0xff);
>

-- 
http://palosaari.fi/

Re: [PATCH 4/4] media: cxd2820r: use DIV_ROUND_CLOSEST_ULL()

[Alex Elder]

On 03/20/2015 06:14 AM, Javi Merino wrote:
> Now that the kernel provides DIV_ROUND_CLOSEST_ULL(), drop the internal
> implementation and use the kernel one.
> 
> Cc: Antti Palosaari <crope@iki.fi>
> Cc: Mauro Carvalho Chehab <mchehab@osg.samsung.com>
> Signed-off-by: Javi Merino <javi.merino@arm.com>
> ---
> I've only compile-tested it, I don't have the hardware to run it.
> 
>  drivers/media/dvb-frontends/cxd2820r_c.c    | 2 +-
>  drivers/media/dvb-frontends/cxd2820r_core.c | 6 ------
>  drivers/media/dvb-frontends/cxd2820r_priv.h | 2 --
>  drivers/media/dvb-frontends/cxd2820r_t.c    | 2 +-
>  drivers/media/dvb-frontends/cxd2820r_t2.c   | 2 +-
>  5 files changed, 3 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/media/dvb-frontends/cxd2820r_c.c b/drivers/media/dvb-frontends/cxd2820r_c.c
> index 149fdca3fb44..72b0e2db3aab 100644
> --- a/drivers/media/dvb-frontends/cxd2820r_c.c
> +++ b/drivers/media/dvb-frontends/cxd2820r_c.c
> @@ -79,7 +79,7 @@ int cxd2820r_set_frontend_c(struct dvb_frontend *fe)
>  
>  	num = if_freq / 1000; /* Hz => kHz */
>  	num *= 0x4000;
> -	if_ctl = 0x4000 - cxd2820r_div_u64_round_closest(num, 41000);
> +	if_ctl = 0x4000 - DIV_ROUND_CLOSEST_ULL(num, 41000);
>  	buf[0] = (if_ctl >> 8) & 0x3f;
>  	buf[1] = (if_ctl >> 0) & 0xff;
>  
> diff --git a/drivers/media/dvb-frontends/cxd2820r_core.c b/drivers/media/dvb-frontends/cxd2820r_core.c
> index 422e84bbb008..490e090048ef 100644
> --- a/drivers/media/dvb-frontends/cxd2820r_core.c
> +++ b/drivers/media/dvb-frontends/cxd2820r_core.c
> @@ -244,12 +244,6 @@ error:
>  	return ret;
>  }
>  
> -/* 64 bit div with round closest, like DIV_ROUND_CLOSEST but 64 bit */
> -u32 cxd2820r_div_u64_round_closest(u64 dividend, u32 divisor)
> -{
> -	return div_u64(dividend + (divisor / 2), divisor);
> -}

Technically, I'd say this has a bug, because the result
needs to be 64 bits wide or your results might be much
different from what might be desired.

Practically though, I'm pretty sure all callers provide
values that ensure the result is valid.

I only call attention because this patch changes the return
type of the function that gets called to do the calculation.

					-Alex

> -
>  static int cxd2820r_set_frontend(struct dvb_frontend *fe)
>  {
>  	struct cxd2820r_priv *priv = fe->demodulator_priv;
> diff --git a/drivers/media/dvb-frontends/cxd2820r_priv.h b/drivers/media/dvb-frontends/cxd2820r_priv.h
> index 7ff5f60c83e1..4b428959b16e 100644
> --- a/drivers/media/dvb-frontends/cxd2820r_priv.h
> +++ b/drivers/media/dvb-frontends/cxd2820r_priv.h
> @@ -64,8 +64,6 @@ int cxd2820r_wr_reg_mask(struct cxd2820r_priv *priv, u32 reg, u8 val,
>  int cxd2820r_wr_regs(struct cxd2820r_priv *priv, u32 reginfo, u8 *val,
>  	int len);
>  
> -u32 cxd2820r_div_u64_round_closest(u64 dividend, u32 divisor);
> -
>  int cxd2820r_wr_regs(struct cxd2820r_priv *priv, u32 reginfo, u8 *val,
>  	int len);
>  
> diff --git a/drivers/media/dvb-frontends/cxd2820r_t.c b/drivers/media/dvb-frontends/cxd2820r_t.c
> index 51401d036530..008cb2ac8480 100644
> --- a/drivers/media/dvb-frontends/cxd2820r_t.c
> +++ b/drivers/media/dvb-frontends/cxd2820r_t.c
> @@ -103,7 +103,7 @@ int cxd2820r_set_frontend_t(struct dvb_frontend *fe)
>  
>  	num = if_freq / 1000; /* Hz => kHz */
>  	num *= 0x1000000;
> -	if_ctl = cxd2820r_div_u64_round_closest(num, 41000);
> +	if_ctl = DIV_ROUND_CLOSEST_ULL(num, 41000);
>  	buf[0] = ((if_ctl >> 16) & 0xff);
>  	buf[1] = ((if_ctl >>  8) & 0xff);
>  	buf[2] = ((if_ctl >>  0) & 0xff);
> diff --git a/drivers/media/dvb-frontends/cxd2820r_t2.c b/drivers/media/dvb-frontends/cxd2820r_t2.c
> index 9c0c4f42175c..35fe364c7182 100644
> --- a/drivers/media/dvb-frontends/cxd2820r_t2.c
> +++ b/drivers/media/dvb-frontends/cxd2820r_t2.c
> @@ -120,7 +120,7 @@ int cxd2820r_set_frontend_t2(struct dvb_frontend *fe)
>  
>  	num = if_freq / 1000; /* Hz => kHz */
>  	num *= 0x1000000;
> -	if_ctl = cxd2820r_div_u64_round_closest(num, 41000);
> +	if_ctl = DIV_ROUND_CLOSEST_ULL(num, 41000);
>  	buf[0] = ((if_ctl >> 16) & 0xff);
>  	buf[1] = ((if_ctl >>  8) & 0xff);
>  	buf[2] = ((if_ctl >>  0) & 0xff);
> 

Re: [PATCH 4/4] media: cxd2820r: use DIV_ROUND_CLOSEST_ULL()

[Javi Merino]

On Fri, Mar 20, 2015 at 01:51:36PM +0000, Alex Elder wrote:
> On 03/20/2015 06:14 AM, Javi Merino wrote:
> > Now that the kernel provides DIV_ROUND_CLOSEST_ULL(), drop the internal
> > implementation and use the kernel one.
> > 
> > Cc: Antti Palosaari <crope@iki.fi>
> > Cc: Mauro Carvalho Chehab <mchehab@osg.samsung.com>
> > Signed-off-by: Javi Merino <javi.merino@arm.com>
> > ---
> > I've only compile-tested it, I don't have the hardware to run it.
> > 
> >  drivers/media/dvb-frontends/cxd2820r_c.c    | 2 +-
> >  drivers/media/dvb-frontends/cxd2820r_core.c | 6 ------
> >  drivers/media/dvb-frontends/cxd2820r_priv.h | 2 --
> >  drivers/media/dvb-frontends/cxd2820r_t.c    | 2 +-
> >  drivers/media/dvb-frontends/cxd2820r_t2.c   | 2 +-
> >  5 files changed, 3 insertions(+), 11 deletions(-)
> > 
> > diff --git a/drivers/media/dvb-frontends/cxd2820r_c.c b/drivers/media/dvb-frontends/cxd2820r_c.c
> > index 149fdca3fb44..72b0e2db3aab 100644
> > --- a/drivers/media/dvb-frontends/cxd2820r_c.c
> > +++ b/drivers/media/dvb-frontends/cxd2820r_c.c
> > @@ -79,7 +79,7 @@ int cxd2820r_set_frontend_c(struct dvb_frontend *fe)
> >  
> >  	num = if_freq / 1000; /* Hz => kHz */
> >  	num *= 0x4000;
> > -	if_ctl = 0x4000 - cxd2820r_div_u64_round_closest(num, 41000);
> > +	if_ctl = 0x4000 - DIV_ROUND_CLOSEST_ULL(num, 41000);
> >  	buf[0] = (if_ctl >> 8) & 0x3f;
> >  	buf[1] = (if_ctl >> 0) & 0xff;
> >  
> > diff --git a/drivers/media/dvb-frontends/cxd2820r_core.c b/drivers/media/dvb-frontends/cxd2820r_core.c
> > index 422e84bbb008..490e090048ef 100644
> > --- a/drivers/media/dvb-frontends/cxd2820r_core.c
> > +++ b/drivers/media/dvb-frontends/cxd2820r_core.c
> > @@ -244,12 +244,6 @@ error:
> >  	return ret;
> >  }
> >  
> > -/* 64 bit div with round closest, like DIV_ROUND_CLOSEST but 64 bit */
> > -u32 cxd2820r_div_u64_round_closest(u64 dividend, u32 divisor)
> > -{
> > -	return div_u64(dividend + (divisor / 2), divisor);
> > -}
> 
> Technically, I'd say this has a bug, because the result
> needs to be 64 bits wide or your results might be much
> different from what might be desired.
> 
> Practically though, I'm pretty sure all callers provide
> values that ensure the result is valid.

All the callers are substituted in this patch so we can make sure that
they are all correct.

> I only call attention because this patch changes the return
> type of the function that gets called to do the calculation.

I'm not sure I follow.  Do you mean that this:

	if_ctl = 0x4000 - DIV_ROUND_CLOSEST_ULL(num, 41000);

Should actually be:

	if_ctl = 0x4000 - (u32)DIV_ROUND_CLOSEST_ULL(num, 41000);

?

if_ctl is a u16 so I don't think you gain anything by doing that.

> > -
> >  static int cxd2820r_set_frontend(struct dvb_frontend *fe)
> >  {
> >  	struct cxd2820r_priv *priv = fe->demodulator_priv;
> > diff --git a/drivers/media/dvb-frontends/cxd2820r_priv.h b/drivers/media/dvb-frontends/cxd2820r_priv.h
> > index 7ff5f60c83e1..4b428959b16e 100644
> > --- a/drivers/media/dvb-frontends/cxd2820r_priv.h
> > +++ b/drivers/media/dvb-frontends/cxd2820r_priv.h
> > @@ -64,8 +64,6 @@ int cxd2820r_wr_reg_mask(struct cxd2820r_priv *priv, u32 reg, u8 val,
> >  int cxd2820r_wr_regs(struct cxd2820r_priv *priv, u32 reginfo, u8 *val,
> >  	int len);
> >  
> > -u32 cxd2820r_div_u64_round_closest(u64 dividend, u32 divisor);
> > -
> >  int cxd2820r_wr_regs(struct cxd2820r_priv *priv, u32 reginfo, u8 *val,
> >  	int len);
> >  
> > diff --git a/drivers/media/dvb-frontends/cxd2820r_t.c b/drivers/media/dvb-frontends/cxd2820r_t.c
> > index 51401d036530..008cb2ac8480 100644
> > --- a/drivers/media/dvb-frontends/cxd2820r_t.c
> > +++ b/drivers/media/dvb-frontends/cxd2820r_t.c
> > @@ -103,7 +103,7 @@ int cxd2820r_set_frontend_t(struct dvb_frontend *fe)
> >  
> >  	num = if_freq / 1000; /* Hz => kHz */
> >  	num *= 0x1000000;
> > -	if_ctl = cxd2820r_div_u64_round_closest(num, 41000);
> > +	if_ctl = DIV_ROUND_CLOSEST_ULL(num, 41000);

if_ctl is a u32, so you get the same behavior that you were getting
before: the downcasting of u64 to u32 happened in
cxd2820r_div_u64_round_closest(), now it happens here.

> >  	buf[0] = ((if_ctl >> 16) & 0xff);
> >  	buf[1] = ((if_ctl >>  8) & 0xff);
> >  	buf[2] = ((if_ctl >>  0) & 0xff);
> > diff --git a/drivers/media/dvb-frontends/cxd2820r_t2.c b/drivers/media/dvb-frontends/cxd2820r_t2.c
> > index 9c0c4f42175c..35fe364c7182 100644
> > --- a/drivers/media/dvb-frontends/cxd2820r_t2.c
> > +++ b/drivers/media/dvb-frontends/cxd2820r_t2.c
> > @@ -120,7 +120,7 @@ int cxd2820r_set_frontend_t2(struct dvb_frontend *fe)
> >  
> >  	num = if_freq / 1000; /* Hz => kHz */
> >  	num *= 0x1000000;
> > -	if_ctl = cxd2820r_div_u64_round_closest(num, 41000);
> > +	if_ctl = DIV_ROUND_CLOSEST_ULL(num, 41000);

Same here if_ctl is a u32.

Cheers,
Javi

Re: [PATCH 4/4] media: cxd2820r: use DIV_ROUND_CLOSEST_ULL()

[Alex Elder]

On 03/20/2015 12:27 PM, Javi Merino wrote:
> On Fri, Mar 20, 2015 at 01:51:36PM +0000, Alex Elder wrote:
>> On 03/20/2015 06:14 AM, Javi Merino wrote:
>>> Now that the kernel provides DIV_ROUND_CLOSEST_ULL(), drop the internal
>>> implementation and use the kernel one.
>>>
>>> Cc: Antti Palosaari <crope@iki.fi>
>>> Cc: Mauro Carvalho Chehab <mchehab@osg.samsung.com>
>>> Signed-off-by: Javi Merino <javi.merino@arm.com>
>>> ---
>>> I've only compile-tested it, I don't have the hardware to run it.
>>>
>>>  drivers/media/dvb-frontends/cxd2820r_c.c    | 2 +-
>>>  drivers/media/dvb-frontends/cxd2820r_core.c | 6 ------
>>>  drivers/media/dvb-frontends/cxd2820r_priv.h | 2 --
>>>  drivers/media/dvb-frontends/cxd2820r_t.c    | 2 +-
>>>  drivers/media/dvb-frontends/cxd2820r_t2.c   | 2 +-
>>>  5 files changed, 3 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/drivers/media/dvb-frontends/cxd2820r_c.c b/drivers/media/dvb-frontends/cxd2820r_c.c
>>> index 149fdca3fb44..72b0e2db3aab 100644
>>> --- a/drivers/media/dvb-frontends/cxd2820r_c.c
>>> +++ b/drivers/media/dvb-frontends/cxd2820r_c.c
>>> @@ -79,7 +79,7 @@ int cxd2820r_set_frontend_c(struct dvb_frontend *fe)
>>>  
>>>  	num = if_freq / 1000; /* Hz => kHz */
>>>  	num *= 0x4000;
>>> -	if_ctl = 0x4000 - cxd2820r_div_u64_round_closest(num, 41000);
>>> +	if_ctl = 0x4000 - DIV_ROUND_CLOSEST_ULL(num, 41000);
>>>  	buf[0] = (if_ctl >> 8) & 0x3f;
>>>  	buf[1] = (if_ctl >> 0) & 0xff;
>>>  
>>> diff --git a/drivers/media/dvb-frontends/cxd2820r_core.c b/drivers/media/dvb-frontends/cxd2820r_core.c
>>> index 422e84bbb008..490e090048ef 100644
>>> --- a/drivers/media/dvb-frontends/cxd2820r_core.c
>>> +++ b/drivers/media/dvb-frontends/cxd2820r_core.c
>>> @@ -244,12 +244,6 @@ error:
>>>  	return ret;
>>>  }
>>>  
>>> -/* 64 bit div with round closest, like DIV_ROUND_CLOSEST but 64 bit */
>>> -u32 cxd2820r_div_u64_round_closest(u64 dividend, u32 divisor)
>>> -{
>>> -	return div_u64(dividend + (divisor / 2), divisor);
>>> -}
>>
>> Technically, I'd say this has a bug, because the result
>> needs to be 64 bits wide or your results might be much
>> different from what might be desired.
>>
>> Practically though, I'm pretty sure all callers provide
>> values that ensure the result is valid.
> 
> All the callers are substituted in this patch so we can make sure that
> they are all correct.
> 
>> I only call attention because this patch changes the return
>> type of the function that gets called to do the calculation.
> 
> I'm not sure I follow.  Do you mean that this:
> 
> 	if_ctl = 0x4000 - DIV_ROUND_CLOSEST_ULL(num, 41000);
> 
> Should actually be:
> 
> 	if_ctl = 0x4000 - (u32)DIV_ROUND_CLOSEST_ULL(num, 41000);

I'm merely stating that your change includes changing the
type of the value returned (cxd2820r_div_u64_round_closest()
returns u32, DIV_ROUND_CLOSEST_ULL() returns U64).  So
in that respect, your patch is not completely trivial.

That said, as you point out (and I also did a quick check)
it does not seem to be an issue here.

The bug (if you call it that) existed before your patch, and
I am not suggesting you change anything.  Just hoping for
a verification that the different return type does not
lead to any problems.  I'm convinced.

					-Alex



> ?
> 
> if_ctl is a u16 so I don't think you gain anything by doing that.
> 
>>> -
>>>  static int cxd2820r_set_frontend(struct dvb_frontend *fe)
>>>  {
>>>  	struct cxd2820r_priv *priv = fe->demodulator_priv;
>>> diff --git a/drivers/media/dvb-frontends/cxd2820r_priv.h b/drivers/media/dvb-frontends/cxd2820r_priv.h
>>> index 7ff5f60c83e1..4b428959b16e 100644
>>> --- a/drivers/media/dvb-frontends/cxd2820r_priv.h
>>> +++ b/drivers/media/dvb-frontends/cxd2820r_priv.h
>>> @@ -64,8 +64,6 @@ int cxd2820r_wr_reg_mask(struct cxd2820r_priv *priv, u32 reg, u8 val,
>>>  int cxd2820r_wr_regs(struct cxd2820r_priv *priv, u32 reginfo, u8 *val,
>>>  	int len);
>>>  
>>> -u32 cxd2820r_div_u64_round_closest(u64 dividend, u32 divisor);
>>> -
>>>  int cxd2820r_wr_regs(struct cxd2820r_priv *priv, u32 reginfo, u8 *val,
>>>  	int len);
>>>  
>>> diff --git a/drivers/media/dvb-frontends/cxd2820r_t.c b/drivers/media/dvb-frontends/cxd2820r_t.c
>>> index 51401d036530..008cb2ac8480 100644
>>> --- a/drivers/media/dvb-frontends/cxd2820r_t.c
>>> +++ b/drivers/media/dvb-frontends/cxd2820r_t.c
>>> @@ -103,7 +103,7 @@ int cxd2820r_set_frontend_t(struct dvb_frontend *fe)
>>>  
>>>  	num = if_freq / 1000; /* Hz => kHz */
>>>  	num *= 0x1000000;
>>> -	if_ctl = cxd2820r_div_u64_round_closest(num, 41000);
>>> +	if_ctl = DIV_ROUND_CLOSEST_ULL(num, 41000);
> 
> if_ctl is a u32, so you get the same behavior that you were getting
> before: the downcasting of u64 to u32 happened in
> cxd2820r_div_u64_round_closest(), now it happens here.
> 
>>>  	buf[0] = ((if_ctl >> 16) & 0xff);
>>>  	buf[1] = ((if_ctl >>  8) & 0xff);
>>>  	buf[2] = ((if_ctl >>  0) & 0xff);
>>> diff --git a/drivers/media/dvb-frontends/cxd2820r_t2.c b/drivers/media/dvb-frontends/cxd2820r_t2.c
>>> index 9c0c4f42175c..35fe364c7182 100644
>>> --- a/drivers/media/dvb-frontends/cxd2820r_t2.c
>>> +++ b/drivers/media/dvb-frontends/cxd2820r_t2.c
>>> @@ -120,7 +120,7 @@ int cxd2820r_set_frontend_t2(struct dvb_frontend *fe)
>>>  
>>>  	num = if_freq / 1000; /* Hz => kHz */
>>>  	num *= 0x1000000;
>>> -	if_ctl = cxd2820r_div_u64_round_closest(num, 41000);
>>> +	if_ctl = DIV_ROUND_CLOSEST_ULL(num, 41000);
> 
> Same here if_ctl is a u32.
> 
> Cheers,
> Javi
>