2017-01-18 12:37 GMT+01:00 Thierry Reding thierry.reding@gmail.com:
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@gmail.com:
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?
IIO timer only read this register not write it so no racy condition here
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.
Ok I will change functions prototype so they will not be like legacy ones but I will keep the current split.
+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;?
ok done