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

[Alex Elder <elder@ieee.org>]

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);
>