LKML Archive on lore.kernel.org
 help / color / Atom feed
From: Daniel Thompson <daniel.thompson@linaro.org>
To: Guru Das Srinagesh <gurus@codeaurora.org>
Cc: linux-pwm@vger.kernel.org,
	"Thierry Reding" <thierry.reding@gmail.com>,
	"Uwe Kleine-König" <u.kleine-koenig@pengutronix.de>,
	"Subbaraman Narayanamurthy" <subbaram@codeaurora.org>,
	"David Collins" <collinsd@codeaurora.org>,
	linux-kernel@vger.kernel.org, "Joe Perches" <joe@perches.com>,
	"Stephen Boyd" <sboyd@kernel.org>,
	"Lee Jones" <lee.jones@linaro.org>,
	"Arnd Bergmann" <arnd@arndb.de>,
	"Geert Uytterhoeven" <geert@linux-m68k.org>,
	"Guenter Roeck" <linux@roeck-us.net>,
	"Dan Carpenter" <dan.carpenter@oracle.com>,
	linux-arm-kernel@lists.infradead.org
Subject: Re: [RESEND PATCH v14 04/11] pwm: clps711x: Cast period to u32 before use as divisor
Date: Sat, 23 May 2020 18:08:06 +0100
Message-ID: <20200523170806.kzqcqzp2rtoqkqk4@holly.lan> (raw)
In-Reply-To: <20200522231904.GB2873@codeaurora.org>

On Fri, May 22, 2020 at 04:19:04PM -0700, Guru Das Srinagesh wrote:
> On Fri, May 22, 2020 at 10:37:38AM +0100, Daniel Thompson wrote:
> > On Thu, May 21, 2020 at 01:25:25PM -0700, Guru Das Srinagesh wrote:
> > > On Thu, May 21, 2020 at 11:19:34AM +0100, Daniel Thompson wrote:
> > > > On Wed, May 20, 2020 at 03:55:57PM -0700, Guru Das Srinagesh wrote:
> > > > > Since the PWM framework is switching struct pwm_args.period's datatype
> > > > > to u64, prepare for this transition by typecasting it to u32.
> > > > > 
> > > > > Also, since the dividend is still a 32-bit number, any divisor greater
> > > > > than the numerator will cause the quotient to be zero, so return 0 in
> > > > > that case to efficiently skip the division.
> > > > > 
> > > > > Signed-off-by: Guru Das Srinagesh <gurus@codeaurora.org>
> > > > > ---
> > > > >  drivers/pwm/pwm-clps711x.c | 5 ++++-
> > > > >  1 file changed, 4 insertions(+), 1 deletion(-)> > > 
> > > > > diff --git a/drivers/pwm/pwm-clps711x.c b/drivers/pwm/pwm-clps711x.c
> > > > > index 924d39a..da771b1 100644
> > > > > --- a/drivers/pwm/pwm-clps711x.c
> > > > > +++ b/drivers/pwm/pwm-clps711x.c
> > > > > @@ -43,7 +43,10 @@ static void clps711x_pwm_update_val(struct clps711x_chip *priv, u32 n, u32 v)
> > > > >  static unsigned int clps711x_get_duty(struct pwm_device *pwm, unsigned int v)
> > > > >  {
> > > > >  	/* Duty cycle 0..15 max */
> > > > > -	return DIV_ROUND_CLOSEST(v * 0xf, pwm->args.period);
> > > > > +	if (pwm->args.period > (v * 0xf))
> > > > > +		return 0;
> > > > 
> > > > This doesn't look right to me.
> > > > 
> > > > DIV_ROUND_CLOSEST() does rounded division and the short circuit doesn't
> > > > implement that.
> > > 
> > > My initial patch [1] was to simply use DIV64_U64_ROUND_CLOSEST(), but I
> > > got review feedback to add a short-circuit (same thread, [2]). I feel
> > > like I should skip the short-circuiting and type casting and simply just
> > > use DIV64_U64_ROUND_CLOSEST() - what do you think?
> > 
> > A trivial review of pwm-clps711x.c suggests that the period is always
> > 32-bit anyway so why not just throw away the short circuit entirely and
> > replace with a comment saying that CLPS711X has a hard coded period
> > that is always >1000000000 ?
> 
> Sorry, I don't follow the significance of 1000000000 - could you please
> explain?

One of the questions you are asked (by Arnd) was whether the period
could ever be larger than UINT_MAX. I think you gave the wrong answer
to that question when you said the divisor could be 64-bit. For this
driver I don't see how the period could ever be larger than 1000000000
(a number that is approximately 4x smaller than UINT_MAX).
 

> Just to clarify, what I was saying in my previous email was the
> following: I think it might be simpler to just throw away the short
> circuit and just do:
> 
> 	s/DIV_ROUND_CLOSEST/DIV64_U64_ROUND_CLOSEST
> 
> like in another patch in this series [1]. That should handle the
> rounding properly as per design. Is that okay?

The short circuit must go because it is broken and we wouldn't want
that code copied somewhere where the code would actually be reachable.
Personally I don't much care which macro you use although given the
divisor cannot be greater then UINT_MAX I guess DIV_ROUND_CLOSEST
is marginally better.


Daniel.

  reply index

Thread overview: 42+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-05-13  0:39 [RESEND PATCH v14 00/11] Convert PWM period and duty cycle to u64 Guru Das Srinagesh
2020-05-04 18:50 ` Guru Das Srinagesh
2020-05-04 18:50   ` [RESEND PATCH v14 01/11] drm/i915: Use 64-bit division macro Guru Das Srinagesh
2020-05-04 18:50   ` [RESEND PATCH v14 02/11] hwmon: pwm-fan: " Guru Das Srinagesh
2020-05-04 18:50   ` [RESEND PATCH v14 03/11] ir-rx51: " Guru Das Srinagesh
2020-05-04 18:50   ` [RESEND PATCH v14 04/11] pwm: clps711x: Cast period to u32 before use as divisor Guru Das Srinagesh
2020-05-04 18:50   ` [RESEND PATCH v14 05/11] pwm: pwm-imx-tpm: Use 64-bit division macro Guru Das Srinagesh
2020-05-04 18:50   ` [RESEND PATCH v14 06/11] pwm: imx27: Use 64-bit division macro and function Guru Das Srinagesh
2020-05-04 18:50   ` [RESEND PATCH v14 07/11] pwm: sifive: Use 64-bit division macro Guru Das Srinagesh
2020-05-04 18:50   ` [RESEND PATCH v14 08/11] pwm: sun4i: Use nsecs_to_jiffies to avoid a division Guru Das Srinagesh
2020-05-04 18:50   ` [RESEND PATCH v14 09/11] backlight: pwm_bl: Use 64-bit division function Guru Das Srinagesh
2020-05-04 18:50   ` [RESEND PATCH v14 10/11] clk: pwm: " Guru Das Srinagesh
2020-05-04 18:50   ` [RESEND PATCH v14 11/11] pwm: core: Convert period and duty cycle to u64 Guru Das Srinagesh
2020-05-13  0:39   ` [RESEND PATCH v14 00/11] Convert PWM " Guru Das Srinagesh
2020-05-13  0:39 ` [RESEND PATCH v14 01/11] drm/i915: Use 64-bit division macro Guru Das Srinagesh
2020-05-20 22:55   ` Guru Das Srinagesh
2020-05-13  0:39 ` [RESEND PATCH v14 02/11] hwmon: pwm-fan: " Guru Das Srinagesh
2020-05-20 22:55   ` Guru Das Srinagesh
2020-05-13  0:39 ` [RESEND PATCH v14 03/11] ir-rx51: " Guru Das Srinagesh
2020-05-20 22:55   ` Guru Das Srinagesh
2020-05-13  0:39 ` [RESEND PATCH v14 04/11] pwm: clps711x: Cast period to u32 before use as divisor Guru Das Srinagesh
2020-05-20 22:55   ` Guru Das Srinagesh
2020-05-21 10:19   ` Daniel Thompson
2020-05-21 20:25     ` Guru Das Srinagesh
2020-05-22  9:37       ` Daniel Thompson
2020-05-22 23:19         ` Guru Das Srinagesh
2020-05-23 17:08           ` Daniel Thompson [this message]
2020-05-13  0:39 ` [RESEND PATCH v14 05/11] pwm: pwm-imx-tpm: Use 64-bit division macro Guru Das Srinagesh
2020-05-20 22:55   ` Guru Das Srinagesh
2020-05-13  0:39 ` [RESEND PATCH v14 06/11] pwm: imx27: Use 64-bit division macro and function Guru Das Srinagesh
2020-05-20 22:55   ` Guru Das Srinagesh
2020-05-13  0:39 ` [RESEND PATCH v14 07/11] pwm: sifive: Use 64-bit division macro Guru Das Srinagesh
2020-05-20 22:56   ` Guru Das Srinagesh
2020-05-13  0:39 ` [RESEND PATCH v14 08/11] pwm: sun4i: Use nsecs_to_jiffies to avoid a division Guru Das Srinagesh
2020-05-20 22:56   ` Guru Das Srinagesh
2020-05-13  0:39 ` [RESEND PATCH v14 09/11] backlight: pwm_bl: Use 64-bit division function Guru Das Srinagesh
2020-05-20 22:56   ` Guru Das Srinagesh
2020-05-13  0:39 ` [RESEND PATCH v14 10/11] clk: pwm: " Guru Das Srinagesh
2020-05-20 22:56   ` Guru Das Srinagesh
2020-05-13  0:39 ` [RESEND PATCH v14 11/11] pwm: core: Convert period and duty cycle to u64 Guru Das Srinagesh
2020-05-20 22:56   ` Guru Das Srinagesh
2020-05-20 22:55 ` [RESEND PATCH v14 00/11] Convert PWM " Guru Das Srinagesh

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20200523170806.kzqcqzp2rtoqkqk4@holly.lan \
    --to=daniel.thompson@linaro.org \
    --cc=arnd@arndb.de \
    --cc=collinsd@codeaurora.org \
    --cc=dan.carpenter@oracle.com \
    --cc=geert@linux-m68k.org \
    --cc=gurus@codeaurora.org \
    --cc=joe@perches.com \
    --cc=lee.jones@linaro.org \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-pwm@vger.kernel.org \
    --cc=linux@roeck-us.net \
    --cc=sboyd@kernel.org \
    --cc=subbaram@codeaurora.org \
    --cc=thierry.reding@gmail.com \
    --cc=u.kleine-koenig@pengutronix.de \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

LKML Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/lkml/0 lkml/git/0.git
	git clone --mirror https://lore.kernel.org/lkml/1 lkml/git/1.git
	git clone --mirror https://lore.kernel.org/lkml/2 lkml/git/2.git
	git clone --mirror https://lore.kernel.org/lkml/3 lkml/git/3.git
	git clone --mirror https://lore.kernel.org/lkml/4 lkml/git/4.git
	git clone --mirror https://lore.kernel.org/lkml/5 lkml/git/5.git
	git clone --mirror https://lore.kernel.org/lkml/6 lkml/git/6.git
	git clone --mirror https://lore.kernel.org/lkml/7 lkml/git/7.git
	git clone --mirror https://lore.kernel.org/lkml/8 lkml/git/8.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 lkml lkml/ https://lore.kernel.org/lkml \
		linux-kernel@vger.kernel.org
	public-inbox-index lkml

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-kernel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git