| Message ID | 20201201011513.1627028-1-j.neuschaefer@gmx.net |
|---|---|
| Headers | show |
| Series | Netronix embedded controller driver for Kobo and Tolino ebook readers | expand |
Hello Jonathan, very nice driver, just a few minor comments below. On Tue, Dec 01, 2020 at 02:15:10AM +0100, Jonathan Neuschäfer wrote: > +static struct ntxec_pwm *pwmchip_to_priv(struct pwm_chip *chip) a function prefix would be great here, I'd pick ntxec_pwm_from_chip as name. > +{ > + return container_of(chip, struct ntxec_pwm, chip); > +} > + > +[...] > +static int ntxec_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm_dev, > + const struct pwm_state *state) > +{ > + struct ntxec_pwm *priv = pwmchip_to_priv(pwm_dev->chip); > + unsigned int period, duty; > + struct reg_sequence regs[] = { > + { NTXEC_REG_PERIOD_HIGH }, > + { NTXEC_REG_PERIOD_LOW }, > + { NTXEC_REG_DUTY_HIGH }, > + { NTXEC_REG_DUTY_LOW } > + }; > + int res; > + > + if (state->polarity != PWM_POLARITY_NORMAL) > + return -EINVAL; > + > + period = min_t(u64, state->period, MAX_PERIOD_NS); > + duty = min_t(u64, state->duty_cycle, period); I'm not a big fan of aligning =. (As if you have to add a longer variable you have to realign all otherwise unrelated lines.) But that's subjective and it's up to you if you want to change this. > + period /= TIME_BASE_NS; > + duty /= TIME_BASE_NS; > + > + /* > + * Changes to the period and duty cycle take effect as soon as the > + * corresponding low byte is written, so the hardware may be configured > + * to an inconsistent state after the period is written and before the > + * duty cycle is fully written. If, in such a case, the old duty cycle > + * is longer than the new period, the EC may output 100% for a moment. > + */ > + > + regs[0].def = ntxec_reg8(period >> 8); > + regs[1].def = ntxec_reg8(period); > + regs[2].def = ntxec_reg8(duty >> 8); > + regs[3].def = ntxec_reg8(duty); You could even minimize the window by changing the order here to NTXEC_REG_PERIOD_HIGH NTXEC_REG_DUTY_HIGH NTXEC_REG_PERIOD_LOW NTXEC_REG_DUTY_LOW but it gets less readable. Maybe move that to a function to have the reg_sequence and the actual write nearer together? Or somehow name the indexes to make it more obvious? > + res = regmap_multi_reg_write(priv->ec->regmap, regs, ARRAY_SIZE(regs)); > + if (res) > + return res; > + > + /* > + * Writing a duty cycle of zero puts the device into a state where > + * writing a higher duty cycle doesn't result in the brightness that it > + * usually results in. This can be fixed by cycling the ENABLE register. > + * > + * As a workaround, write ENABLE=0 when the duty cycle is zero. If the device already has duty_cycle = 0 but ENABLE = 1, you might get a failure. But I guess this doesn't need addressing in the code. But maybe point it out in a comment? > + */ > + if (state->enabled && duty != 0) { > + res = regmap_write(priv->ec->regmap, NTXEC_REG_ENABLE, ntxec_reg8(1)); > + if (res) > + return res; > + > + /* Disable the auto-off timer */ > + res = regmap_write(priv->ec->regmap, NTXEC_REG_AUTO_OFF_HI, ntxec_reg8(0xff)); > + if (res) > + return res; > + > + return regmap_write(priv->ec->regmap, NTXEC_REG_AUTO_OFF_LO, ntxec_reg8(0xff)); Given that you cannot read back period and duty anyhow: Does it make sense to write these only if (state->enabled && duty != 0)? > + } else { > + return regmap_write(priv->ec->regmap, NTXEC_REG_ENABLE, ntxec_reg8(0)); > + } > +} Thanks Uwe
On Tue, Dec 01, 2020 at 08:20:26AM +0100, Uwe Kleine-König wrote: > Hello Jonathan, > > very nice driver, just a few minor comments below. > > On Tue, Dec 01, 2020 at 02:15:10AM +0100, Jonathan Neuschäfer wrote: > > +static struct ntxec_pwm *pwmchip_to_priv(struct pwm_chip *chip) > > a function prefix would be great here, I'd pick ntxec_pwm_from_chip as > name. Good point, will do. > > > +{ > > + return container_of(chip, struct ntxec_pwm, chip); > > +} > > + > > +[...] > > +static int ntxec_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm_dev, > > + const struct pwm_state *state) > > +{ > > + struct ntxec_pwm *priv = pwmchip_to_priv(pwm_dev->chip); > > + unsigned int period, duty; > > + struct reg_sequence regs[] = { > > + { NTXEC_REG_PERIOD_HIGH }, > > + { NTXEC_REG_PERIOD_LOW }, > > + { NTXEC_REG_DUTY_HIGH }, > > + { NTXEC_REG_DUTY_LOW } > > + }; > > + int res; > > + > > + if (state->polarity != PWM_POLARITY_NORMAL) > > + return -EINVAL; > > + > > + period = min_t(u64, state->period, MAX_PERIOD_NS); > > + duty = min_t(u64, state->duty_cycle, period); > > I'm not a big fan of aligning =. (As if you have to add a longer > variable you have to realign all otherwise unrelated lines.) But that's > subjective and it's up to you if you want to change this. In this case, I thought it helps the readability, because the lines are quite similar. > > + period /= TIME_BASE_NS; > > + duty /= TIME_BASE_NS; Here, I did it because I had already aligned the previous two lines. > > + > > + /* > > + * Changes to the period and duty cycle take effect as soon as the > > + * corresponding low byte is written, so the hardware may be configured > > + * to an inconsistent state after the period is written and before the > > + * duty cycle is fully written. If, in such a case, the old duty cycle > > + * is longer than the new period, the EC may output 100% for a moment. > > + */ > > + > > + regs[0].def = ntxec_reg8(period >> 8); > > + regs[1].def = ntxec_reg8(period); > > + regs[2].def = ntxec_reg8(duty >> 8); > > + regs[3].def = ntxec_reg8(duty); > > You could even minimize the window by changing the order here to > > NTXEC_REG_PERIOD_HIGH > NTXEC_REG_DUTY_HIGH > NTXEC_REG_PERIOD_LOW > NTXEC_REG_DUTY_LOW Good idea, but I'm not sure if the EC handles this kind of interleaving correctly. > but it gets less readable. Maybe move that to a function to have the > reg_sequence and the actual write nearer together? Indeed, a separate function would keep register names and values together (without resorting to declarations-after-statements). > Or somehow name the indexes to make it more obvious? Too much unnecessary complexity, IMHO. > > + res = regmap_multi_reg_write(priv->ec->regmap, regs, ARRAY_SIZE(regs)); > > + if (res) > > + return res; > > + > > + /* > > + * Writing a duty cycle of zero puts the device into a state where > > + * writing a higher duty cycle doesn't result in the brightness that it > > + * usually results in. This can be fixed by cycling the ENABLE register. > > + * > > + * As a workaround, write ENABLE=0 when the duty cycle is zero. > > If the device already has duty_cycle = 0 but ENABLE = 1, you might get > a failure. But I guess this doesn't need addressing in the code. But > maybe point it out in a comment? Good point. I'll add something to the comment. > > + */ > > + if (state->enabled && duty != 0) { > > + res = regmap_write(priv->ec->regmap, NTXEC_REG_ENABLE, ntxec_reg8(1)); > > + if (res) > > + return res; > > + > > + /* Disable the auto-off timer */ > > + res = regmap_write(priv->ec->regmap, NTXEC_REG_AUTO_OFF_HI, ntxec_reg8(0xff)); > > + if (res) > > + return res; > > + > > + return regmap_write(priv->ec->regmap, NTXEC_REG_AUTO_OFF_LO, ntxec_reg8(0xff)); > > Given that you cannot read back period and duty anyhow: Does it make > sense to write these only if (state->enabled && duty != 0)? I think it does. Thanks, Jonathan