From: Thierry Reding <thierry.reding@gmail.com>
To: YH Huang <yh.huang@mediatek.com>
Cc: Matthias Brugger <matthias.bgg@gmail.com>,
Mark Rutland <mark.rutland@arm.com>,
Rob Herring <robh+dt@kernel.org>, Pawel Moll <pawel.moll@arm.com>,
linux-pwm@vger.kernel.org, devicetree@vger.kernel.org,
linux-kernel@vger.kernel.org,
linux-arm-kernel@lists.infradead.org,
srv_heupstream@mediatek.com, linux-mediatek@lists.infradead.org,
Sascha Hauer <kernel@pengutronix.de>,
yingjoe.chen@mediatek.com
Subject: Re: [PATCH v2 2/2] pwm: add MediaTek display PWM driver support
Date: Mon, 29 Jun 2015 11:21:47 +0200 [thread overview]
Message-ID: <20150629092145.GF5431@ulmo> (raw)
In-Reply-To: <1434622784.18278.39.camel@mtksdaap41>
[-- Attachment #1: Type: text/plain, Size: 1378 bytes --]
On Thu, Jun 18, 2015 at 06:19:44PM +0800, YH Huang wrote:
> On Fri, 2015-06-12 at 12:20 +0200, Thierry Reding wrote:
> > On Thu, May 21, 2015 at 09:29:24PM +0800, YH Huang wrote:
[...]
> > > +#define DISP_PWM_CON_1 0x14
> > > +#define PWM_PERIOD_MASK 0xfff
> > > +#define PWM_PERIOD_MAX 0x00000fff
> >
> > Same here. PWM_PERIOD_MAX isn't actually used anywhere, so perhaps just
> > drop it altogether. But see also below...
> >
> > > +/* Shift log2(PWM_PERIOD_MAX + 1) as divisor */
> > > +#define PWM_PERIOD_BIT_SHIFT 12
> >
> > I wasn't very clear about this in my earlier review, so let me try to
> > explain why I think this is confusing. You use this as a divisor, but
> > you encode it as a shift. It's also PWM_PERIOD_MAX + 1, so I think it
> > would make more sense to drop this, keep PWM_PERIOD_MAX as above and
> > then replace the
> >
> > >> PWM_PERIOD_BIT_SHIFT
> >
> > below by
> >
> > / (PWM_PERIOD_MAX + 1)
> >
>
> Maybe I can change in this way:
> Remove this: #define PWM_PERIOD_MAX 0x00000fff
> Using ">> PWM_PERIOD_BIT_SHIFT" is faster than "/ (PWM_PERIOD_MAX + 1)"
> Is this right?
The compiler should be able to optimize the division to a shift, so both
will likely result in the same code. But I don't mind much either way as
long as we don't define two symbols for essentially the same value.
Thierry
[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]
next prev parent reply other threads:[~2015-06-29 9:21 UTC|newest]
Thread overview: 16+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-05-21 13:29 [PATCH v2 0/2] Add MediaTek display PWM driver YH Huang
[not found] ` <1432214964-40644-1-git-send-email-yh.huang-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>
2015-05-21 13:29 ` [PATCH v2 1/2] dt-bindings: pwm: add MediaTek display PWM bindings YH Huang
2015-06-12 10:23 ` Thierry Reding
[not found] ` <20150612102344.GG19400-AwZRO8vwLAwmlAP/+Wk3EA@public.gmane.org>
2015-06-18 9:00 ` YH Huang
2015-05-21 13:29 ` [PATCH v2 2/2] pwm: add MediaTek display PWM driver support YH Huang
2015-06-12 10:20 ` Thierry Reding
2015-06-18 10:19 ` YH Huang
2015-06-18 13:58 ` Yingjoe Chen
2015-06-29 9:27 ` Thierry Reding
2015-06-29 9:21 ` Thierry Reding [this message]
2015-06-12 10:25 ` Thierry Reding
2015-06-18 9:02 ` YH Huang
2015-05-25 2:14 ` [PATCH v2 0/2] Add MediaTek display PWM driver Yingjoe Chen
2015-06-05 6:15 ` YH Huang
2015-06-05 8:39 ` Matthias Brugger
2015-06-05 10:45 ` YH Huang
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=20150629092145.GF5431@ulmo \
--to=thierry.reding@gmail.com \
--cc=devicetree@vger.kernel.org \
--cc=kernel@pengutronix.de \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mediatek@lists.infradead.org \
--cc=linux-pwm@vger.kernel.org \
--cc=mark.rutland@arm.com \
--cc=matthias.bgg@gmail.com \
--cc=pawel.moll@arm.com \
--cc=robh+dt@kernel.org \
--cc=srv_heupstream@mediatek.com \
--cc=yh.huang@mediatek.com \
--cc=yingjoe.chen@mediatek.com \
/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
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).