lkml.org 
[lkml]   [2019]   [Mar]   [12]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH v9 2/2] pwm: sifive: Add a driver for SiFive SoC PWM
    Hello,

    there are just a few minor things left I commented below.

    On Tue, Mar 12, 2019 at 01:41:29PM +0530, Yash Shah wrote:
    > +#define div_u64_round(a, b) \
    > + ({typeof(b) __b = b; div_u64((a) + __b / 2, __b); })

    Parenthesis around b please. I guess I didn't had them in my suggestion
    either, sorry for that.

    > +static int pwm_sifive_enable(struct pwm_chip *chip, bool enable)
    > +{
    > + struct pwm_sifive_ddata *pwm = pwm_sifive_chip_to_ddata(chip);
    > + int ret;
    > +
    > + if (enable) {
    > + ret = clk_enable(pwm->clk);
    > + if (ret) {
    > + dev_err(pwm->chip.dev, "Enable clk failed:%d\n", ret);
    > + return ret;
    > + }
    > + }
    > +
    > + if (!enable)
    > + clk_disable(pwm->clk);
    > +
    > + return 0;
    > +}

    There is only a single caller for this function. I wonder if it is
    trivial enough to fold it into pwm_sifive_apply.

    > +static int pwm_sifive_apply(struct pwm_chip *chip, struct pwm_device *dev,
    > + struct pwm_state *state)
    > +{
    > + struct pwm_sifive_ddata *pwm = pwm_sifive_chip_to_ddata(chip);
    > + unsigned int duty_cycle;
    > + u32 frac;
    > + struct pwm_state cur_state;
    > + bool enabled;
    > + int ret = 0;
    > + unsigned long num;
    > +
    > + if (state->polarity != PWM_POLARITY_INVERSED)
    > + return -EINVAL;
    > +
    > + ret = clk_enable(pwm->clk);
    > + if (ret) {
    > + dev_err(pwm->chip.dev, "Enable clk failed:%d\n", ret);
    > + return ret;
    > + }
    > +
    > + mutex_lock(&pwm->lock);
    > + pwm_get_state(dev, &cur_state);
    > +
    > + enabled = cur_state.enabled;
    > +
    > + if (state->period != pwm->approx_period) {
    > + if (pwm->user_count != 1) {
    > + ret = -EBUSY;
    > + goto exit;
    > + }
    > + pwm->approx_period = state->period;
    > + pwm_sifive_update_clock(pwm, clk_get_rate(pwm->clk));
    > + }
    > +
    > + duty_cycle = state->duty_cycle;
    > + if (!state->enabled)
    > + duty_cycle = 0;
    > +
    > + num = (u64)duty_cycle * (1U << PWM_SIFIVE_CMPWIDTH);
    > + frac = div_u64_round(num, state->period);
    > + /* The hardware cannot generate a 100% duty cycle */
    > + frac = min(frac, (1U << PWM_SIFIVE_CMPWIDTH) - 1);
    > +
    > + writel(frac, pwm->regs + PWM_SIFIVE_PWMCMP0 +
    > + dev->hwpwm * PWM_SIFIVE_SIZE_PWMCMP);
    > +
    > + if (state->enabled != enabled) {
    > + ret = pwm_sifive_enable(chip, state->enabled);
    > + if (ret)
    > + goto exit;

    This goto is a noop.

    > + }
    > +
    > +exit:
    > + clk_disable(pwm->clk);
    > + mutex_unlock(&pwm->lock);
    > + return ret;
    > +}

    There are a few other things that could be improved, but I think they
    could be addressed later. For some of these I don't even know what to
    suggest, for some Thierry might not agree it is worth fixing:

    - rounding
    how to round? When should a request declined, when is rounding ok?
    There is still "if (state->period != pwm->approx_period) return -EBUSY"
    in this driver. This is better than before, but if state-period ==
    pwm->approx_period + 1 the result (if accepted) might be the same as
    without the +1 and so returning -EBUSY for one case and accepting the
    other is strange.
    - don't call PWM API functions designed for consumers (here: pwm_get_state)
    - Move div_u64_round to include/linux/math64.h

    Best regards
    Uwe

    --
    Pengutronix e.K. | Uwe Kleine-König |
    Industrial Linux Solutions | http://www.pengutronix.de/ |

    \
     
     \ /
      Last update: 2019-03-12 10:18    [W:6.546 / U:0.172 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site