| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <44d061723df8c01f27f3d95a944356eb.squirrel@www.codeaurora.org>
Date: Tue, 3 May 2011 22:48:24 -0700 (PDT)
From: "Willie Ruan" <wruan@...eaurora.org>
To: "Samuel Ortiz" <sameo@...ux.intel.com>
Cc: "Willie Ruan" <wruan@...eaurora.org>,
"David Brown" <davidb@...eaurora.org>,
"Daniel Walker" <dwalker@...o99.com>,
"Bryan Huntsman" <bryanh@...eaurora.org>,
linux-kernel@...r.kernel.org, linux-arm-msm@...r.kernel.org
Subject: Re: [PATCH 1/2] mfd: pm8xxx-pwm: add pm8xxx PWM driver
Hi Samuel,
Thank you for reviewing my patches. I'll make the two functions smaller by
creating sub routines as you suggested.
pm8xxx_pwm_calc_period()
pm8xxx_pwm_configure()
Other comments below.
> Hi Willie,
>
> On Thu, Apr 21, 2011 at 11:15:51PM -0700, Willie Ruan wrote:
>> Qualcomm PM8xxx chips, such as PM8058 and PM8921, have 8 channels of
>> PWM, also called LPG (Light Pulse Generator) in HW specs. All PWM
>> channels can be used as simple PWM machine or as a more advanced PWM
>> pattern generator using programmed lookup table.
>>
>> This patch supports all APIs listed in <linux/pwm.h> with a small
>> difference. The two parameters (duty_ns and period_ns) in pwm_config()
>> are used as values in microseconds instead of nanoseconds. Otherwise a
>> 32-bit integer can't fit for a range of 7 us to 300+ seconds.
> I can only lament the fact that we're still lacking a proper PWM API.
> Besides
> that, I have a couple comments:
I agree. Hopefully someone would create a better PWM API soon for us to
use. Fortunately PWM driver and API are not hard for our customers.
>
>> +static void pm8xxx_pwm_calc_period(unsigned int period_us,
>> + struct pm8xxx_pwm_config *pwm_conf)
>> +{
>> + int n, m, clk, div;
>> + int best_m, best_div, best_clk;
>> + int last_err, cur_err, better_err, better_m;
>> + unsigned int tmp_p, last_p, min_err, period_n;
>> +
>> + /* PWM Period / N */
>> + if (period_us < (40 * USEC_PER_SEC)) { /* ~6-bit max */
>> + period_n = (period_us * NSEC_PER_USEC) >> 6;
>> + n = 6;
>> + } else if (period_us < (274 * USEC_PER_SEC)) { /* overflow threshold
>> */
>> + period_n = (period_us >> 6) * NSEC_PER_USEC;
>> + if (period_n >= MAX_MPT) {
>> + n = 9;
>> + period_n >>= 3;
>> + } else
>> + n = 6;
>> + } else {
>> + period_n = (period_us >> 9) * NSEC_PER_USEC;
>> + n = 9;
>> + }
>> +
>> + min_err = MAX_MPT;
>> + best_m = 0;
>> + best_clk = 0;
>> + best_div = 0;
>> + for (clk = 0; clk < NUM_CLOCKS; clk++) {
>> + for (div = 0; div < NUM_PRE_DIVIDE; div++) {
>> + tmp_p = period_n;
>> + last_p = tmp_p;
>> + for (m = 0; m <= PM8XXX_PWM_M_MAX; m++) {
>> + if (tmp_p <= pt_t[div][clk]) {
>> + /* Found local best */
>> + if (!m) {
>> + better_err = pt_t[div][clk] -
>> + tmp_p;
>> + better_m = m;
>> + } else {
>> + last_err = last_p -
>> + pt_t[div][clk];
>> + cur_err = pt_t[div][clk] -
>> + tmp_p;
>> +
>> + if (cur_err < last_err) {
>> + better_err = cur_err;
>> + better_m = m;
>> + } else {
>> + better_err = last_err;
>> + better_m = m - 1;
>> + }
>> + }
>> +
>> + if (better_err < min_err) {
>> + min_err = better_err;
>> + best_m = better_m;
>> + best_clk = clk;
>> + best_div = div;
>> + }
>> + break;
>> + } else {
>> + last_p = tmp_p;
>> + tmp_p >>= 1;
>> + }
>> + }
>> + }
>> + }
> This loop needs to be splitted up into 2-3 different routines. Your code
> will be much more readable.
Sure I can change this.
>
>> +static int pm8xxx_pwm_configure(struct pwm_device *pwm,
>> + struct pm8xxx_pwm_config *pwm_conf)
>> +{
>> + int i, rc, len;
>> + u8 reg, ramp_enabled = 0;
>> +
>> + reg = (pwm_conf->pwm_size > 6) ? PM8XXX_PWM_SIZE_9_BIT : 0;
>> + pwm->pwm_ctl[5] = reg;
>> +
>> + reg = ((pwm_conf->clk + 1) << PM8XXX_PWM_CLK_SEL_SHIFT)
>> + & PM8XXX_PWM_CLK_SEL_MASK;
>> + reg |= (pwm_conf->pre_div << PM8XXX_PWM_PREDIVIDE_SHIFT)
>> + & PM8XXX_PWM_PREDIVIDE_MASK;
>> + reg |= pwm_conf->pre_div_exp & PM8XXX_PWM_M_MASK;
>> + pwm->pwm_ctl[4] = reg;
>> +
>> + if (pwm_conf->bypass_lut) {
>> + pwm->pwm_ctl[0] &= PM8XXX_PWM_PWM_START; /* keep enabled */
>> + pwm->pwm_ctl[1] = PM8XXX_PWM_BYPASS_LUT;
>> + pwm->pwm_ctl[2] = 0;
>> +
>> + if (pwm_conf->pwm_size > 6) {
>> + pwm->pwm_ctl[3] = pwm_conf->pwm_value
>> + & PM8XXX_PWM_VALUE_BIT7_0;
>> + pwm->pwm_ctl[4] |= (pwm_conf->pwm_value >> 1)
>> + & PM8XXX_PWM_VALUE_BIT8;
>> + } else {
>> + pwm->pwm_ctl[3] = pwm_conf->pwm_value
>> + & PM8XXX_PWM_VALUE_BIT5_0;
>> + }
>> +
>> + len = 6;
>> + } else {
>> + int pause_cnt, j;
>> +
>> + /* Linear search for duty time */
>> + for (i = 0; i < PM8XXX_PWM_1KHZ_COUNT_MAX; i++) {
>> + if (duty_msec[i] >= pwm_conf->lut_duty_ms)
>> + break;
>> + }
>> +
>> + ramp_enabled = pwm->pwm_ctl[0] & PM8XXX_PWM_RAMP_GEN_START;
>> + pwm->pwm_ctl[0] &= PM8XXX_PWM_PWM_START; /* keep enabled */
>> + pwm->pwm_ctl[0] |= (i << PM8XXX_PWM_1KHZ_COUNT_SHIFT) &
>> + PM8XXX_PWM_1KHZ_COUNT_MASK;
>> + pwm->pwm_ctl[1] = pwm_conf->lut_hi_index &
>> + PM8XXX_PWM_HIGH_INDEX_MASK;
>> + pwm->pwm_ctl[2] = pwm_conf->lut_lo_index &
>> + PM8XXX_PWM_LOW_INDEX_MASK;
>> +
>> + if (pwm_conf->flags & PM_PWM_LUT_REVERSE)
>> + pwm->pwm_ctl[1] |= PM8XXX_PWM_REVERSE_EN;
>> + if (pwm_conf->flags & PM_PWM_LUT_RAMP_UP)
>> + pwm->pwm_ctl[2] |= PM8XXX_PWM_RAMP_UP;
>> + if (pwm_conf->flags & PM_PWM_LUT_LOOP)
>> + pwm->pwm_ctl[2] |= PM8XXX_PWM_LOOP_EN;
>> +
>> + /* Pause time */
>> + if (pwm_conf->flags & PM_PWM_LUT_PAUSE_HI_EN) {
>> + /* Linear search for pause time */
>> + pause_cnt = (pwm_conf->lut_pause_hi + duty_msec[i] / 2)
>> + / duty_msec[i];
>> + for (j = 0; j < PM8XXX_PWM_PAUSE_COUNT_MAX; j++) {
>> + if (pause_count[j] >= pause_cnt)
>> + break;
>> + }
>> + pwm->pwm_ctl[5] = (j <<
>> + PM8XXX_PWM_PAUSE_COUNT_HI_SHIFT) &
>> + PM8XXX_PWM_PAUSE_COUNT_HI_MASK;
>> + pwm->pwm_ctl[5] |= PM8XXX_PWM_PAUSE_ENABLE_HIGH;
>> + } else
>> + pwm->pwm_ctl[5] = 0;
>> +
>> + if (pwm_conf->flags & PM_PWM_LUT_PAUSE_LO_EN) {
>> + /* Linear search for pause time */
>> + pause_cnt = (pwm_conf->lut_pause_lo + duty_msec[i] / 2)
>> + / duty_msec[i];
>> + for (j = 0; j < PM8XXX_PWM_PAUSE_COUNT_MAX; j++) {
>> + if (pause_count[j] >= pause_cnt)
>> + break;
>> + }
>> + pwm->pwm_ctl[6] = (j <<
>> + PM8XXX_PWM_PAUSE_COUNT_LO_SHIFT) &
>> + PM8XXX_PWM_PAUSE_COUNT_LO_MASK;
>> + pwm->pwm_ctl[6] |= PM8XXX_PWM_PAUSE_ENABLE_LOW;
>> + } else
>> + pwm->pwm_ctl[6] = 0;
>> +
>> + len = 7;
>> + }
>> +
>> + pm8xxx_pwm_bank_sel(pwm);
>> +
>> + for (i = 0; i < len; i++) {
>> + rc = pm8xxx_writeb(pwm->chip->dev->parent,
>> + SSBI_REG_ADDR_LPG_CTL(i),
>> + pwm->pwm_ctl[i]);
>> + if (rc) {
>> + pr_err("pm8xxx_write(): rc=%d (PWM Ctl[%d])\n", rc, i);
>> + break;
>> + }
>> + }
>> +
>> + if (ramp_enabled) {
>> + pwm->pwm_ctl[0] |= ramp_enabled;
>> + pm8xxx_writeb(pwm->chip->dev->parent,
>> + SSBI_REG_ADDR_LPG_CTL(0),
>> + pwm->pwm_ctl[0]);
>> + }
>> +
>> + return rc;
>> +}
> I would also appreciate if this routine could be made smaller by having it
> calling sub routines.
I'll add subroutines for this function.
Thank you,
~Willie
>
> Cheers,
> Samuel.
>
> --
> Intel Open Source Technology Centre
> http://oss.intel.com/
> --
> To unsubscribe from this list: send the line "unsubscribe linux-arm-msm"
> in
> the body of a message to majordomo@...r.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
--
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists