From: "Uwe Kleine-König" <u.kleine-koenig@pengutronix.de>
To: Baruch Siach <baruch@tkos.co.il>
Cc: Thierry Reding <thierry.reding@gmail.com>,
Lee Jones <lee.jones@linaro.org>, Andy Gross <agross@kernel.org>,
Bjorn Andersson <bjorn.andersson@linaro.org>,
Balaji Prakash J <bjagadee@codeaurora.org>,
Rob Herring <robh+dt@kernel.org>,
Robert Marko <robert.marko@sartura.hr>,
linux-pwm@vger.kernel.org, devicetree@vger.kernel.org,
linux-arm-msm@vger.kernel.org,
linux-arm-kernel@lists.infradead.org
Subject: Re: [PATCH v4 1/3] pwm: driver for qualcomm ipq6018 pwm block
Date: Wed, 7 Jul 2021 07:42:36 +0200 [thread overview]
Message-ID: <20210707054236.rn6fjivsozuxecgv@pengutronix.de> (raw)
In-Reply-To: <875yxmg1pi.fsf@tarshish>
[-- Attachment #1: Type: text/plain, Size: 4125 bytes --]
Hello Baruch,
On Wed, Jul 07, 2021 at 07:58:01AM +0300, Baruch Siach wrote:
> On Mon, Jul 05 2021, Uwe Kleine-König wrote:
> > On Sun, Jun 27, 2021 at 08:24:04AM +0300, Baruch Siach wrote:
> >> +/*
> >> + * Enable bit is set to enable output toggling in pwm device.
> >> + * Update bit is set to reflect the changed divider and high duration
> >> + * values in register.
> >> + */
> >> +#define PWM_ENABLE 0x80000000
> >> +#define PWM_UPDATE 0x40000000
> >> +
> >> +/* The frequency range supported is 1Hz to 100MHz */
> >> +#define MIN_PERIOD_NS 10
> >> +#define MAX_PERIOD_NS 1000000000
> >
> > Please use a driver prefix for these defines.
>
> I take this to refer also to the defines below, right?
right.
> >> +
> >> +/*
> >> + * The max value specified for each field is based on the number of bits
> >> + * in the pwm control register for that field
> >> + */
> >> +#define MAX_PWM_CFG 0xFFFF
> >> +
> >> +#define PWM_CTRL_HI_SHIFT 16
> >> +
> >> +#define PWM_CFG_REG0 0 /*PWM_DIV PWM_HI*/
> >> +#define PWM_CFG_REG1 1 /*ENABLE UPDATE PWM_PRE_DIV*/
>
> ...
>
> >> +static void config_div_and_duty(struct pwm_device *pwm, int pre_div,
> >> + unsigned long long pwm_div, unsigned long period_ns,
> >> + unsigned long long duty_ns)
> >
> > Please also use a consistent prefix for function names.
> >
> > I suggest to use u64 for some of the parameters. While this doesn't
> > change anything, it is cleaner as the caller passes variables of this
> > type.
>
> Actually for pre_div and pwm_div the caller passes int values. I agree
> this is inconsistent.
>
> ...
>
> >> +static int ipq_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> >> + const struct pwm_state *state)
> >> +{
> >> + struct ipq_pwm_chip *ipq_chip = to_ipq_pwm_chip(chip);
> >> + unsigned long freq;
> >> + int pre_div, close_pre_div, close_pwm_div;
> >> + int pwm_div;
> >> + long long diff;
> >> + unsigned long rate = clk_get_rate(ipq_chip->clk);
> >> + unsigned long min_diff = rate;
> >> + uint64_t fin_ps;
> >> + u64 period_ns, duty_ns;
> >> +
> >> + if (state->period < MIN_PERIOD_NS)
> >> + return -ERANGE;
> >
> > MIN_PERIOD_NS depends on clk_get_rate(ipq_chip->clk), doesn't it?
>
> probe sets this clock to the fixed 100MHz rate (CLK_SRC_FREQ). Would you
> prefer to derive MIN_PERIOD_NS from CLK_SRC_FREQ?
I'd like to either have all of ipq_pwm_apply use this information that
the clock rate is 100MHz or nothing, but having MIN_PERIOD_NS hardcoding
that information and use clk_get_rate(ipq_chip->clk) for calculating
fin_ps is strange.
> >> + freq = div64_u64(NSEC_PER_SEC, period_ns);
> >> + fin_ps = div64_u64(NSEC_PER_SEC * 1000ULL, rate);
> >> + close_pre_div = MAX_PWM_CFG;
> >> + close_pwm_div = MAX_PWM_CFG;
> >> +
> >> + for (pre_div = 0; pre_div <= MAX_PWM_CFG; pre_div++) {
> >> + pwm_div = DIV64_U64_ROUND_CLOSEST(period_ns * 1000,
> >> + fin_ps * (pre_div + 1));
> >> + pwm_div--;
> >> + if (pwm_div < 0 || pwm_div > MAX_PWM_CFG)
> >> + continue;
> >> +
> >> + diff = ((uint64_t)freq * (pre_div + 1) * (pwm_div + 1))
> >> + - (uint64_t)rate;
> >> +
> >> + if (diff < 0) /* period larger than requested */
> >> + continue;
> >> + if (diff == 0) { /* bingo */
> >> + close_pre_div = pre_div;
> >> + close_pwm_div = pwm_div;
> >> + break;
> >> + }
> >> + if (diff < min_diff) {
> >> + min_diff = diff;
> >> + close_pre_div = pre_div;
> >> + close_pwm_div = pwm_div;
> >> + }
> >
> > I didn't check deeply, but I assume this calculation can be done more
> > efficiently.
>
> The thing is that we have two dividers to play with. I can't think of a
> cleaner way to find the best match for a given target frequency.
OK, with two equal dividers it might indeed be necessary to do it this
way. After seeing a get_state implemententation I will think about this
some more.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | https://www.pengutronix.de/ |
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
prev parent reply other threads:[~2021-07-07 5:42 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-06-27 5:24 [PATCH v4 1/3] pwm: driver for qualcomm ipq6018 pwm block Baruch Siach
2021-06-27 5:24 ` [PATCH v4 2/3] dt-bindings: pwm: add IPQ6018 binding Baruch Siach
2021-06-27 5:24 ` [PATCH v4 3/3] arm64: dts: ipq6018: add pwm node Baruch Siach
2021-07-05 7:20 ` [PATCH v4 1/3] pwm: driver for qualcomm ipq6018 pwm block Uwe Kleine-König
2021-07-07 4:58 ` Baruch Siach
2021-07-07 5:42 ` Uwe Kleine-König [this message]
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=20210707054236.rn6fjivsozuxecgv@pengutronix.de \
--to=u.kleine-koenig@pengutronix.de \
--cc=agross@kernel.org \
--cc=baruch@tkos.co.il \
--cc=bjagadee@codeaurora.org \
--cc=bjorn.andersson@linaro.org \
--cc=devicetree@vger.kernel.org \
--cc=lee.jones@linaro.org \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-arm-msm@vger.kernel.org \
--cc=linux-pwm@vger.kernel.org \
--cc=robert.marko@sartura.hr \
--cc=robh+dt@kernel.org \
--cc=thierry.reding@gmail.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).