From: Billy Tsai <billy_tsai@aspeedtech.com>
To: "Uwe Kleine-König" <u.kleine-koenig@pengutronix.de>
Cc: "lee.jones@linaro.org" <lee.jones@linaro.org>,
"robh+dt@kernel.org" <robh+dt@kernel.org>,
"joel@jms.id.au" <joel@jms.id.au>,
"andrew@aj.id.au" <andrew@aj.id.au>,
"thierry.reding@gmail.com" <thierry.reding@gmail.com>,
"p.zabel@pengutronix.de" <p.zabel@pengutronix.de>,
"devicetree@vger.kernel.org" <devicetree@vger.kernel.org>,
"linux-arm-kernel@lists.infradead.org"
<linux-arm-kernel@lists.infradead.org>,
"linux-aspeed@lists.ozlabs.org" <linux-aspeed@lists.ozlabs.org>,
"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
"linux-pwm@vger.kernel.org" <linux-pwm@vger.kernel.org>,
BMC-SW <BMC-SW@aspeedtech.com>
Subject: Re: [v6 2/2] pwm: Add Aspeed ast2600 PWM support
Date: Mon, 24 May 2021 01:56:19 +0000 [thread overview]
Message-ID: <9EA46360-8F43-4D1B-9004-3965A6182FA1@aspeedtech.com> (raw)
In-Reply-To: <20210522160708.ryr7n7klapszu2da@pengutronix.de>
Hi,
On 2021/5/23, 12:07 AM,Uwe Kleine-Königwrote:
Hello,
On Tue, May 18, 2021 at 08:55:17AM +0800, Billy Tsai wrote:
> > +static u64 aspeed_pwm_get_period(struct pwm_chip *chip, struct pwm_device *pwm)
> > +{
> > + struct aspeed_pwm_data *priv = aspeed_pwm_chip_to_data(chip);
> > + unsigned long rate;
> > + u32 index = pwm->hwpwm;
> > + u32 val;
> > + u64 period, div_h, div_l, clk_period;
> > +
> > + rate = clk_get_rate(priv->clk);
> > + regmap_read(priv->regmap, PWM_ASPEED_CTRL_CH(index), &val);
> > + div_h = FIELD_GET(PWM_ASPEED_CTRL_CLK_DIV_H, val);
> > + div_l = FIELD_GET(PWM_ASPEED_CTRL_CLK_DIV_L, val);
> > + regmap_read(priv->regmap, PWM_ASPEED_DUTY_CYCLE_CH(index), &val);
> > + clk_period = FIELD_GET(PWM_ASPEED_DUTY_CYCLE_PERIOD, val);
> > + period = (NSEC_PER_SEC * BIT(div_h) * (div_l + 1) * (clk_period + 1));
> The outer pair of parenthesis on the RHS isn't necessary. The maximal
> value that period can have here is:
> 1000000000 * 2**15 * 256 * 256
> This fits into an u64, but as all but the last factor are 32 bit values
> you might get an overflow here.
I don’t know in which case the value will overflow, when my parameter types are all u64.
Can you tell me what is "the last factor"?
> > +static int aspeed_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> > + const struct pwm_state *state)
> > +{
> > + struct device *dev = chip->dev;
> > + struct aspeed_pwm_data *priv = aspeed_pwm_chip_to_data(chip);
> > + u32 index = pwm->hwpwm;
> > + int ret;
> > +
> > + dev_dbg(dev, "apply period: %lldns, duty_cycle: %lldns", state->period,
> > + state->duty_cycle);
> > +
> > + regmap_update_bits(priv->regmap, PWM_ASPEED_CTRL_CH(index),
> > + PWM_ASPEED_CTRL_PIN_ENABLE,
> > + state->enabled ? PWM_ASPEED_CTRL_PIN_ENABLE : 0);
> > + /*
> > + * Fixed the period to the max value and rising point to 0
> > + * for high resolution and simplify frequency calculation.
> > + */
> > + regmap_update_bits(priv->regmap, PWM_ASPEED_DUTY_CYCLE_CH(index),
> > + (PWM_ASPEED_DUTY_CYCLE_PERIOD |
> > + PWM_ASPEED_DUTY_CYCLE_RISING_POINT),
> > + FIELD_PREP(PWM_ASPEED_DUTY_CYCLE_PERIOD,
> > + PWM_ASPEED_FIXED_PERIOD));
> > +
> > + ret = aspeed_pwm_set_period(chip, pwm, state);
> > + if (ret)
> > + return ret;
> > + aspeed_pwm_set_duty(chip, pwm, state);
> aspeed_pwm_set_duty calls aspeed_pwm_get_period() which is a bit
> ineffective after just having set the period.
When I call aspeed_pwm_set_period it doesn't mean the period is equal to what I set (It may
lose some precision Ex: When I set the period 40000ns, the actual period I set is 39680ns) and
I didn't get this information when I call aspeed_pwm_set_period. Thus, I need to get the actual
period first before set duty.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
next prev parent reply other threads:[~2021-05-24 23:12 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-05-18 0:55 [v6 0/2] Support pwm driver for aspeed ast26xx Billy Tsai
2021-05-18 0:55 ` [v6 1/2] dt-bindings: Add bindings for aspeed pwm-tach Billy Tsai
2021-05-19 20:20 ` Rob Herring
2021-05-20 1:06 ` Billy Tsai
2021-05-18 0:55 ` [v6 2/2] pwm: Add Aspeed ast2600 PWM support Billy Tsai
2021-05-22 16:07 ` Uwe Kleine-König
2021-05-24 1:56 ` Billy Tsai [this message]
2021-05-24 11:02 ` Uwe Kleine-König
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=9EA46360-8F43-4D1B-9004-3965A6182FA1@aspeedtech.com \
--to=billy_tsai@aspeedtech.com \
--cc=BMC-SW@aspeedtech.com \
--cc=andrew@aj.id.au \
--cc=devicetree@vger.kernel.org \
--cc=joel@jms.id.au \
--cc=lee.jones@linaro.org \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-aspeed@lists.ozlabs.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-pwm@vger.kernel.org \
--cc=p.zabel@pengutronix.de \
--cc=robh+dt@kernel.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
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).