| Message ID | 20200620121758.14836-10-hdegoede@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | acpi/pwm/i915: Convert pwm-crc and i915 driver's PWM code to use the atomic PWM API | expand |
Hello, [adding Shobhit Kumar <shobhit.kumar@intel.com> to Cc who is the author of this driver according to the comment on the top of the driver] On Sat, Jun 20, 2020 at 02:17:52PM +0200, Hans de Goede wrote: > The pwm-crc code is using 2 different enable bits: > 1. bit 7 of the PWM0_CLK_DIV (PWM_OUTPUT_ENABLE) > 2. bit 0 of the BACKLIGHT_EN register > > So far we've kept the PWM_OUTPUT_ENABLE bit set when disabling the PWM, > this commit makes crc_pwm_disable() clear it on disable and makes > crc_pwm_enable() set it again on re-enable. > > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > --- > Changes in v3: > - Remove paragraph about tri-stating the output from the commit message, > we don't have a datasheet so this was just an unfounded guess I have the impression you spend quite some time with this driver trying to understand it. What I still think is a bit unfortunate is that there is quite some guesswork involved. I wonder if it would be possible to get the manual of that PWM. Do I understand correctly that this is IP from Intel? There are quite some Intel people on Cc; maybe someone can help/put in a good word/check and ack the changes? Best regards Uwe
Hi, On 6/22/20 9:55 AM, Uwe Kleine-König wrote: > Hello, > > [adding Shobhit Kumar <shobhit.kumar@intel.com> to Cc who is the author > of this driver according to the comment on the top of the driver] > > On Sat, Jun 20, 2020 at 02:17:52PM +0200, Hans de Goede wrote: >> The pwm-crc code is using 2 different enable bits: >> 1. bit 7 of the PWM0_CLK_DIV (PWM_OUTPUT_ENABLE) >> 2. bit 0 of the BACKLIGHT_EN register >> >> So far we've kept the PWM_OUTPUT_ENABLE bit set when disabling the PWM, >> this commit makes crc_pwm_disable() clear it on disable and makes >> crc_pwm_enable() set it again on re-enable. >> >> Signed-off-by: Hans de Goede <hdegoede@redhat.com> >> --- >> Changes in v3: >> - Remove paragraph about tri-stating the output from the commit message, >> we don't have a datasheet so this was just an unfounded guess > > I have the impression you spend quite some time with this driver trying > to understand it. Yes, my initial plan for this patch series was to just convert this driver to atomic PWM, but it turned out to need a bit of TLC first. > What I still think is a bit unfortunate is that there > is quite some guesswork involved. Actually for 99% of the changes I'm pretty sure they are correct. This patch is the 1% where I'm not sure, and in this case I'm playing it safe by keeping the code as is. As the commit message tries to explain I strongly suspect that bit 0 of the BACKLIGHT_EN register really drives a separate GPIO pin on the PMIC which is earmarked as backlight-enable pin (many LCD panels have both a pwm input for brightness-level and a separate enable/disable pin). If we can get information that my hunch here is correct then the right thing to do would be to change things so that the PWM driver stops poking bit 0 of the BACKLIGHT_EN register and this gets done by the CRC GPIO driver instead. But the poking of that bit is already happening now and since I'm not 100% sure that my hunch is correct, the safe thing to do is to keep this as is. Note that for the main consumer of the CRC PWM, the i915 driver it does not matter. If we change that bit into a GPIO then the i915 drv will need to be modified to drive the GPIO high / low when enabling / disabling the panel. Just like it already enables/ disables the PWM when enabling / disabling the panel. So the end result will still be bit 0 of the BACKLIGHT_EN register going high/low on LCD panel enable/disable. So even if my hunch is right functionality wise nothing will change. The code doing the poking will be technically more correct, but that is all that we would gain. > I wonder if it would be possible to > get the manual of that PWM. Do I understand correctly that this is IP > from Intel? There are quite some Intel people on Cc; maybe someone can > help/put in a good word/check and ack the changes? IIRC last time I asked no one from the Intel folks on the Cc has access to the Crystal Cove PMIC datasheet. Regards, Hans
Hello, On Sat, Jun 20, 2020 at 02:17:52PM +0200, Hans de Goede wrote: > The pwm-crc code is using 2 different enable bits: > 1. bit 7 of the PWM0_CLK_DIV (PWM_OUTPUT_ENABLE) > 2. bit 0 of the BACKLIGHT_EN register > > So far we've kept the PWM_OUTPUT_ENABLE bit set when disabling the PWM, > this commit makes crc_pwm_disable() clear it on disable and makes > crc_pwm_enable() set it again on re-enable. > > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > --- > Changes in v3: > - Remove paragraph about tri-stating the output from the commit message, > we don't have a datasheet so this was just an unfounded guess > --- > drivers/pwm/pwm-crc.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/drivers/pwm/pwm-crc.c b/drivers/pwm/pwm-crc.c > index 81232da0c767..b72008c9b072 100644 > --- a/drivers/pwm/pwm-crc.c > +++ b/drivers/pwm/pwm-crc.c > @@ -54,7 +54,9 @@ static int crc_pwm_calc_clk_div(int period_ns) > static int crc_pwm_enable(struct pwm_chip *c, struct pwm_device *pwm) > { > struct crystalcove_pwm *crc_pwm = to_crc_pwm(c); > + int div = crc_pwm_calc_clk_div(pwm_get_period(pwm)); > > + regmap_write(crc_pwm->regmap, PWM0_CLK_DIV, div | PWM_OUTPUT_ENABLE); > regmap_write(crc_pwm->regmap, BACKLIGHT_EN, 1); > > return 0; > @@ -63,8 +65,10 @@ static int crc_pwm_enable(struct pwm_chip *c, struct pwm_device *pwm) > static void crc_pwm_disable(struct pwm_chip *c, struct pwm_device *pwm) > { > struct crystalcove_pwm *crc_pwm = to_crc_pwm(c); > + int div = crc_pwm_calc_clk_div(pwm_get_period(pwm)); > > regmap_write(crc_pwm->regmap, BACKLIGHT_EN, 0); > + regmap_write(crc_pwm->regmap, PWM0_CLK_DIV, div); > } > > static int crc_pwm_config(struct pwm_chip *c, struct pwm_device *pwm, In the absence of documentation this looks reasonable. Acked-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> Best regards Uwe
diff --git a/drivers/pwm/pwm-crc.c b/drivers/pwm/pwm-crc.c index 81232da0c767..b72008c9b072 100644 --- a/drivers/pwm/pwm-crc.c +++ b/drivers/pwm/pwm-crc.c @@ -54,7 +54,9 @@ static int crc_pwm_calc_clk_div(int period_ns) static int crc_pwm_enable(struct pwm_chip *c, struct pwm_device *pwm) { struct crystalcove_pwm *crc_pwm = to_crc_pwm(c); + int div = crc_pwm_calc_clk_div(pwm_get_period(pwm)); + regmap_write(crc_pwm->regmap, PWM0_CLK_DIV, div | PWM_OUTPUT_ENABLE); regmap_write(crc_pwm->regmap, BACKLIGHT_EN, 1); return 0; @@ -63,8 +65,10 @@ static int crc_pwm_enable(struct pwm_chip *c, struct pwm_device *pwm) static void crc_pwm_disable(struct pwm_chip *c, struct pwm_device *pwm) { struct crystalcove_pwm *crc_pwm = to_crc_pwm(c); + int div = crc_pwm_calc_clk_div(pwm_get_period(pwm)); regmap_write(crc_pwm->regmap, BACKLIGHT_EN, 0); + regmap_write(crc_pwm->regmap, PWM0_CLK_DIV, div); } static int crc_pwm_config(struct pwm_chip *c, struct pwm_device *pwm,
The pwm-crc code is using 2 different enable bits: 1. bit 7 of the PWM0_CLK_DIV (PWM_OUTPUT_ENABLE) 2. bit 0 of the BACKLIGHT_EN register So far we've kept the PWM_OUTPUT_ENABLE bit set when disabling the PWM, this commit makes crc_pwm_disable() clear it on disable and makes crc_pwm_enable() set it again on re-enable. Signed-off-by: Hans de Goede <hdegoede@redhat.com> --- Changes in v3: - Remove paragraph about tri-stating the output from the commit message, we don't have a datasheet so this was just an unfounded guess --- drivers/pwm/pwm-crc.c | 4 ++++ 1 file changed, 4 insertions(+)