On Wed, Jan 18, 2017 at 12:15:58PM +0100, Benjamin Gaignard wrote:
> 2017-01-18 11:08 GMT+01:00 Thierry Reding <thierry.reding@xxxxxxxxx>:
> > On Thu, Jan 05, 2017 at 10:25:40AM +0100, Benjamin Gaignard wrote:
[...]
> >> +static u32 active_channels(struct stm32_pwm *dev)
> >> +{
> >> + u32 ccer;
> >> +
> >> + regmap_read(dev->regmap, TIM_CCER, &ccer);
> >> +
> >> + return ccer & TIM_CCER_CCXE;
> >> +}
> >
> > This looks like something that you could track in software, but this is
> > probably fine, too. Again, technically regmap_read() could fail, so you
> > might want to consider adding some code to handle it. In practice it
> > probably won't, so maybe you don't.
>
> TIM_CCER_CCXE is a value that IIO timer can also read (not write) so
> I have keep the same logic for pwm driver.
Would that not be racy? What happens if after active_channels() here,
the IIO timer modifies the TIM_CCER register?
> >> + ret = stm32_pwm_config(chip, pwm, state->duty_cycle, state->period);
> >> + if (ret)
> >> + return ret;
> >> +
> >> + if (!enabled && state->enabled)
> >> + ret = stm32_pwm_enable(chip, pwm);
> >> +
> >> + return ret;
> >> +}
> >
> > Would it be possible to merge stm32_pwm_disable(), stm32_pwm_enable(),
> > stm32_pwm_set_polarity() and stm32_pwm_config() into stm32_pwm_apply()?
> > Part of the reason for the atomic API was to make it easier to write
> > these drivers, but your implementation effectively copies what the
> > transitional helpers do.
> >
> > It might not make a difference technically in your case, but I think
> > it'd make the implementation more compact and set a better example for
> > future reference.
>
> hmm... it will create a fat function with lot of where
> enabling/disabling/configuration
> will be mixed I'm really not convince that will more compact and readable.
I don't object to splitting this up into separate functions, I just
don't think the functions should correspond to the legacy ones. One
variant that I think could work out nicely would be to have one
function that precomputes the various values, call in from ->apply()
and then do only the register writes along with a couple of
conditionals depending on enable state, for example.
> >> +static const struct pwm_ops stm32pwm_ops = {
> >> + .owner = THIS_MODULE,
> >> + .apply = stm32_pwm_apply,
> >> +};
> >> +
> >> +static int stm32_pwm_set_breakinput(struct stm32_pwm *priv,
> >> + int level, int filter)
> >> +{
> >> + u32 bdtr = TIM_BDTR_BKE;
> >> +
> >> + if (level)
> >> + bdtr |= TIM_BDTR_BKP;
> >> +
> >> + bdtr |= (filter & TIM_BDTR_BKF_MASK) << TIM_BDTR_BKF_SHIFT;
> >> +
> >> + regmap_update_bits(priv->regmap,
> >> + TIM_BDTR, TIM_BDTR_BKE | TIM_BDTR_BKP | TIM_BDTR_BKF,
> >> + bdtr);
> >> +
> >> + regmap_read(priv->regmap, TIM_BDTR, &bdtr);
> >> +
> >> + return (bdtr & TIM_BDTR_BKE) ? 0 : -EINVAL;
> >> +}
> >> +
> >> +static int stm32_pwm_set_breakinput2(struct stm32_pwm *priv,
> >> + int level, int filter)
> >> +{
> >> + u32 bdtr = TIM_BDTR_BK2E;
> >> +
> >> + if (level)
> >> + bdtr |= TIM_BDTR_BK2P;
> >> +
> >> + bdtr |= (filter & TIM_BDTR_BKF_MASK) << TIM_BDTR_BK2F_SHIFT;
> >> +
> >> + regmap_update_bits(priv->regmap,
> >> + TIM_BDTR, TIM_BDTR_BK2E |
> >> + TIM_BDTR_BK2P |
> >> + TIM_BDTR_BK2F,
> >> + bdtr);
> >> +
> >> + regmap_read(priv->regmap, TIM_BDTR, &bdtr);
> >> +
> >> + return (bdtr & TIM_BDTR_BK2E) ? 0 : -EINVAL;
> >> +}
> >
> > As far as I can tell the only difference here is the various bit
> > positions. Can you collapse the above two functions and add a new
> > parameter to unify some code?
>
> Yes it is all about bit shifting, I had try unify those two functions
> with index has additional parameter
> but it just add if() before each lines so no real benefit for code size.
How about if you precompute the values and masks? Something like:
u32 bke = (index == 0) ? ... : ...;
u32 bkp = (index == 0) ? ... : ...;
u32 bkf = (index == 0) ? ... : ...;
u32 mask = (index == 0) ? ... : ...;
bdtr = bke | bkf;
if (level)
bdtr |= bkp;
regmap_update_bits(priv->regmap, TIM_BDTR, mask, bdtr);
regmap_read(priv->regmap, TIM_BDTR, &bdtr);
return (bdtr & bke) ? 0 : -EINVAL;
?
Attachment:
signature.asc
Description: PGP signature
