mbox series

[v5,00/16] acpi/pwm/i915: Convert pwm-crc and i915 driver's PWM code to use the atomic PWM API

Message ID 20200717133753.127282-1-hdegoede@redhat.com (mailing list archive)
Headers show
Series acpi/pwm/i915: Convert pwm-crc and i915 driver's PWM code to use the atomic PWM API | expand

Message

Hans de Goede July 17, 2020, 1:37 p.m. UTC
Hi All,

Here is v5 of my patch series converting the i915 driver's code for
controlling the panel's backlight with an external PWM controller to
use the atomic PWM API. See below for the changelog.

This series consists of 4 parts:

1. acpi_lpss fixes workarounds for Cherry Trail DSTD nastiness
2. various fixes to the pwm-lpss driver
3. convert the pwm-crc driver to support the atomic PWM API and
4. convert the i915 driver's PWM code to use the atomic PWM API

The involved acpi_lpss and pwm drivers do not see a whole lot of churn,
so the plan is to merge this all through drm-intel-next-queued (dinq)
once all the patches are reviewed / have acks.

Specifically patches 5-9, 11 still need an Acked- / Reviewed-by

Andy, can you please take a look at the unreviewed patches? Specifically
patches 5-6 should address your review remarks from v4 of this set
and I've addressed your review remarks on patches 7-9 in v3 already.
A review of patch 11 would also be welcome

Uwe, can you please take a look at the unreviewed patches?

Uwe, may I have your Acked-by for merging this series through the
drm-intel-next-queued branch once all PWM patches have an Acked- or
Reviewed-by ?

This series has been tested (and re-tested after adding various bug-fixes)
extensively. It has been tested on the following devices:

-Asus T100TA  BYT + CRC-PMIC PWM
-Toshiba WT8-A  BYT + CRC-PMIC PWM
-Thundersoft TS178 BYT + CRC-PMIC PWM, inverse PWM
-Asus T100HA  CHT + CRC-PMIC PWM
-Terra Pad 1061  BYT + LPSS PWM
-Trekstor Twin 10.1 BYT + LPSS PWM
-Asus T101HA  CHT + CRC-PMIC PWM
-GPD Pocket  CHT + CRC-PMIC PWM

Changelog:
Changes in v5:
- Dropped the "pwm: lpss: Correct get_state result for base_unit == 0"
  patch. The base_unit == 0 condition should never happen and sofar it is
  unclear what the proper behavior / correct values to store in the
  pwm_state should be when this does happen.  Since this patch was added as
  an extra pwm-lpss fix in v4 of this patch-set and otherwise is orthogonal
  to the of this patch-set just drop it (again).
- "[PATCH 04/16] pwm: lpss: Add range limit check for the base_unit register value"
  - Use clamp_val(... instead of clam_t(unsigned long long, ...
- "[PATCH 05/16] pwm: lpss: Add pwm_lpss_prepare_enable() helper"
  - This is a new patch in v5 of this patchset
- [PATCH 06/16] pwm: lpss: Use pwm_lpss_apply() when restoring state on resume
  - Use the new pwm_lpss_prepare_enable() helper

Changes in v4:
- "[PATCH v4 06/16] pwm: lpss: Correct get_state result for base_unit == 0"
  - This is a new patch in v4 of this patchset
- "[PATCH v4 12/16] pwm: crc: Implement get_state() method"
  - Use DIV_ROUND_UP when calculating the period and duty_cycle values
- "[PATCH v4 16/16] drm/i915: panel: Use atomic PWM API for devs with an external PWM controller"
  - Add a note to the commit message about the changes in pwm_disable_backlight()
  - Use the pwm_set/get_relative_duty_cycle() helpers

Changes in v3:
- "[PATCH v3 04/15] pwm: lpss: Add range limit check for the base_unit register value"
  - Use base_unit_range - 1 as maximum value for the clamp()
- "[PATCH v3 05/15] pwm: lpss: Use pwm_lpss_apply() when restoring state on resume"
  - This replaces the "pwm: lpss: Set SW_UPDATE bit when enabling the PWM"
    patch from previous versions of this patch-set, which really was a hack
    working around the resume issue which this patch fixes properly.
- PATCH v3 6 - 11 pwm-crc changes:
  - Various small changes resulting from the reviews by Andy and Uwe,
    including some refactoring of the patches to reduce the amount of churn
    in the patch-set

Changes in v2:
- Fix coverletter subject
- Drop accidentally included debugging patch
- "[PATCH v3 02/15] ACPI / LPSS: Save Cherry Trail PWM ctx registers only once (
  - Move #define LPSS_SAVE_CTX_ONCE define to group it with LPSS_SAVE_CTX

Regards,

Hans

Comments

Thierry Reding July 27, 2020, 7:41 a.m. UTC | #1
On Fri, Jul 17, 2020 at 03:37:37PM +0200, Hans de Goede wrote:
> Hi All,
> 
> Here is v5 of my patch series converting the i915 driver's code for
> controlling the panel's backlight with an external PWM controller to
> use the atomic PWM API. See below for the changelog.
> 
> This series consists of 4 parts:
> 
> 1. acpi_lpss fixes workarounds for Cherry Trail DSTD nastiness
> 2. various fixes to the pwm-lpss driver
> 3. convert the pwm-crc driver to support the atomic PWM API and
> 4. convert the i915 driver's PWM code to use the atomic PWM API
> 
> The involved acpi_lpss and pwm drivers do not see a whole lot of churn,
> so the plan is to merge this all through drm-intel-next-queued (dinq)
> once all the patches are reviewed / have acks.
> 
> Specifically patches 5-9, 11 still need an Acked- / Reviewed-by
> 
> Andy, can you please take a look at the unreviewed patches? Specifically
> patches 5-6 should address your review remarks from v4 of this set
> and I've addressed your review remarks on patches 7-9 in v3 already.
> A review of patch 11 would also be welcome
> 
> Uwe, can you please take a look at the unreviewed patches?
> 
> Uwe, may I have your Acked-by for merging this series through the
> drm-intel-next-queued branch once all PWM patches have an Acked- or
> Reviewed-by ?
> 
> This series has been tested (and re-tested after adding various bug-fixes)
> extensively. It has been tested on the following devices:
> 
> -Asus T100TA  BYT + CRC-PMIC PWM
> -Toshiba WT8-A  BYT + CRC-PMIC PWM
> -Thundersoft TS178 BYT + CRC-PMIC PWM, inverse PWM
> -Asus T100HA  CHT + CRC-PMIC PWM
> -Terra Pad 1061  BYT + LPSS PWM
> -Trekstor Twin 10.1 BYT + LPSS PWM
> -Asus T101HA  CHT + CRC-PMIC PWM
> -GPD Pocket  CHT + CRC-PMIC PWM
> 
> Changelog:
> Changes in v5:
> - Dropped the "pwm: lpss: Correct get_state result for base_unit == 0"
>   patch. The base_unit == 0 condition should never happen and sofar it is
>   unclear what the proper behavior / correct values to store in the
>   pwm_state should be when this does happen.  Since this patch was added as
>   an extra pwm-lpss fix in v4 of this patch-set and otherwise is orthogonal
>   to the of this patch-set just drop it (again).
> - "[PATCH 04/16] pwm: lpss: Add range limit check for the base_unit register value"
>   - Use clamp_val(... instead of clam_t(unsigned long long, ...
> - "[PATCH 05/16] pwm: lpss: Add pwm_lpss_prepare_enable() helper"
>   - This is a new patch in v5 of this patchset
> - [PATCH 06/16] pwm: lpss: Use pwm_lpss_apply() when restoring state on resume
>   - Use the new pwm_lpss_prepare_enable() helper
> 
> Changes in v4:
> - "[PATCH v4 06/16] pwm: lpss: Correct get_state result for base_unit == 0"
>   - This is a new patch in v4 of this patchset
> - "[PATCH v4 12/16] pwm: crc: Implement get_state() method"
>   - Use DIV_ROUND_UP when calculating the period and duty_cycle values
> - "[PATCH v4 16/16] drm/i915: panel: Use atomic PWM API for devs with an external PWM controller"
>   - Add a note to the commit message about the changes in pwm_disable_backlight()
>   - Use the pwm_set/get_relative_duty_cycle() helpers
> 
> Changes in v3:
> - "[PATCH v3 04/15] pwm: lpss: Add range limit check for the base_unit register value"
>   - Use base_unit_range - 1 as maximum value for the clamp()
> - "[PATCH v3 05/15] pwm: lpss: Use pwm_lpss_apply() when restoring state on resume"
>   - This replaces the "pwm: lpss: Set SW_UPDATE bit when enabling the PWM"
>     patch from previous versions of this patch-set, which really was a hack
>     working around the resume issue which this patch fixes properly.
> - PATCH v3 6 - 11 pwm-crc changes:
>   - Various small changes resulting from the reviews by Andy and Uwe,
>     including some refactoring of the patches to reduce the amount of churn
>     in the patch-set
> 
> Changes in v2:
> - Fix coverletter subject
> - Drop accidentally included debugging patch
> - "[PATCH v3 02/15] ACPI / LPSS: Save Cherry Trail PWM ctx registers only once (
>   - Move #define LPSS_SAVE_CTX_ONCE define to group it with LPSS_SAVE_CTX

Hi Hans,

I've applied patches 3 through 12 to the PWM tree. I thought it was a
bit odd that only a handful of these patches had been reviewed and there
were no Tested-bys, but I'm going to trust that you know what you're
doing. =) If this breaks things for anyone I'm sure they'll complain.

That said I see that Rafael has acked patches 1-2 and Jani did so for
patches 13-16. I'm not sure if you expect me to pick those patches up as
well. As far as I can tell the ACPI, PWM and DRM parts are all
independent, so these patches could be applied to the corresponding
subsystem trees.

Anyway, if you want me to pick those all up into the PWM tree, I suppose
that's something I can do as well.

Thierry
Andy Shevchenko July 29, 2020, 8:23 a.m. UTC | #2
On Mon, Jul 27, 2020 at 09:41:20AM +0200, Thierry Reding wrote:
> On Fri, Jul 17, 2020 at 03:37:37PM +0200, Hans de Goede wrote:

> I've applied patches 3 through 12 to the PWM tree. I thought it was a
> bit odd that only a handful of these patches had been reviewed and there
> were no Tested-bys, but I'm going to trust that you know what you're
> doing. =) If this breaks things for anyone I'm sure they'll complain.

Can we postpone a bit?

> That said I see that Rafael has acked patches 1-2 and Jani did so for
> patches 13-16. I'm not sure if you expect me to pick those patches up as
> well. As far as I can tell the ACPI, PWM and DRM parts are all
> independent, so these patches could be applied to the corresponding
> subsystem trees.
> 
> Anyway, if you want me to pick those all up into the PWM tree, I suppose
> that's something I can do as well.
Hans de Goede July 29, 2020, 9:32 a.m. UTC | #3
cHi,

On 7/29/20 10:23 AM, Andy Shevchenko wrote:
> On Mon, Jul 27, 2020 at 09:41:20AM +0200, Thierry Reding wrote:
>> On Fri, Jul 17, 2020 at 03:37:37PM +0200, Hans de Goede wrote:
> 
>> I've applied patches 3 through 12 to the PWM tree. I thought it was a
>> bit odd that only a handful of these patches had been reviewed and there
>> were no Tested-bys, but I'm going to trust that you know what you're
>> doing. =) If this breaks things for anyone I'm sure they'll complain.

Thank you for picking up these patches, but ...

> Can we postpone a bit?

I have to agree with Andy here, as mentioned my plan was to push the
entire series through drm-intel-next-queued once the last few PWM
patches are reviewed.

There are some fixes, to the pwm-crc driver which change behavior in
a possibly undesirable way, unless combined with the i915 changes.

E.g. there is a fix which makes the pwm-crc driver actually honor
the requested output frequency (it was not doing this due to a bug)
and before the i915 changes, the i915 driver was hardcoding an output
freq, rather then looking at the video-bios-tables as it should.

So having just the pwm-crc fix, will change the output frequency
which some LCD panels might not like.

Note things are probably fine with the hardcoded output freq, but I
would like to play it safe here.

Also Andy was still reviewing some of the PWM patches, and has requested
changes to 1 patch, nothing functional just some code-reshuffling for
cleaner code, so we could alternatively fix this up with a follow-up patch.

Either way please let us know how you want to proceed.

>> That said I see that Rafael has acked patches 1-2 and Jani did so for
>> patches 13-16. I'm not sure if you expect me to pick those patches up as
>> well. As far as I can tell the ACPI, PWM and DRM parts are all
>> independent, so these patches could be applied to the corresponding
>> subsystem trees.
>>
>> Anyway, if you want me to pick those all up into the PWM tree, I suppose
>> that's something I can do as well.

drm-intel-next-queued is usually seeing quite a bit of churn, so the i915
patches really should go upstream through that branch.

Regards,

Hans
Andy Shevchenko July 29, 2020, 10:54 a.m. UTC | #4
On Fri, Jul 17, 2020 at 03:37:37PM +0200, Hans de Goede wrote:
> Hi All,
> 
> Here is v5 of my patch series converting the i915 driver's code for
> controlling the panel's backlight with an external PWM controller to
> use the atomic PWM API. See below for the changelog.
> 
> This series consists of 4 parts:
> 
> 1. acpi_lpss fixes workarounds for Cherry Trail DSTD nastiness
> 2. various fixes to the pwm-lpss driver
> 3. convert the pwm-crc driver to support the atomic PWM API and
> 4. convert the i915 driver's PWM code to use the atomic PWM API
> 
> The involved acpi_lpss and pwm drivers do not see a whole lot of churn,
> so the plan is to merge this all through drm-intel-next-queued (dinq)
> once all the patches are reviewed / have acks.
> 
> Specifically patches 5-9, 11 still need an Acked- / Reviewed-by
> 
> Andy, can you please take a look at the unreviewed patches? Specifically
> patches 5-6 should address your review remarks from v4 of this set
> and I've addressed your review remarks on patches 7-9 in v3 already.
> A review of patch 11 would also be welcome

Done. Sorry for a delay.

One comment to consider, though. There are three channels in that PWM AFAIU.
One of them is backlight control, another one can be attached to haptics. The
concern is how this series may (or may not?) affect haptics behaviour.

> Uwe, can you please take a look at the unreviewed patches?
> 
> Uwe, may I have your Acked-by for merging this series through the
> drm-intel-next-queued branch once all PWM patches have an Acked- or
> Reviewed-by ?
> 
> This series has been tested (and re-tested after adding various bug-fixes)
> extensively. It has been tested on the following devices:
> 
> -Asus T100TA  BYT + CRC-PMIC PWM
> -Toshiba WT8-A  BYT + CRC-PMIC PWM
> -Thundersoft TS178 BYT + CRC-PMIC PWM, inverse PWM
> -Asus T100HA  CHT + CRC-PMIC PWM
> -Terra Pad 1061  BYT + LPSS PWM
> -Trekstor Twin 10.1 BYT + LPSS PWM
> -Asus T101HA  CHT + CRC-PMIC PWM
> -GPD Pocket  CHT + CRC-PMIC PWM
> 
> Changelog:
> Changes in v5:
> - Dropped the "pwm: lpss: Correct get_state result for base_unit == 0"
>   patch. The base_unit == 0 condition should never happen and sofar it is
>   unclear what the proper behavior / correct values to store in the
>   pwm_state should be when this does happen.  Since this patch was added as
>   an extra pwm-lpss fix in v4 of this patch-set and otherwise is orthogonal
>   to the of this patch-set just drop it (again).
> - "[PATCH 04/16] pwm: lpss: Add range limit check for the base_unit register value"
>   - Use clamp_val(... instead of clam_t(unsigned long long, ...
> - "[PATCH 05/16] pwm: lpss: Add pwm_lpss_prepare_enable() helper"
>   - This is a new patch in v5 of this patchset
> - [PATCH 06/16] pwm: lpss: Use pwm_lpss_apply() when restoring state on resume
>   - Use the new pwm_lpss_prepare_enable() helper
> 
> Changes in v4:
> - "[PATCH v4 06/16] pwm: lpss: Correct get_state result for base_unit == 0"
>   - This is a new patch in v4 of this patchset
> - "[PATCH v4 12/16] pwm: crc: Implement get_state() method"
>   - Use DIV_ROUND_UP when calculating the period and duty_cycle values
> - "[PATCH v4 16/16] drm/i915: panel: Use atomic PWM API for devs with an external PWM controller"
>   - Add a note to the commit message about the changes in pwm_disable_backlight()
>   - Use the pwm_set/get_relative_duty_cycle() helpers
> 
> Changes in v3:
> - "[PATCH v3 04/15] pwm: lpss: Add range limit check for the base_unit register value"
>   - Use base_unit_range - 1 as maximum value for the clamp()
> - "[PATCH v3 05/15] pwm: lpss: Use pwm_lpss_apply() when restoring state on resume"
>   - This replaces the "pwm: lpss: Set SW_UPDATE bit when enabling the PWM"
>     patch from previous versions of this patch-set, which really was a hack
>     working around the resume issue which this patch fixes properly.
> - PATCH v3 6 - 11 pwm-crc changes:
>   - Various small changes resulting from the reviews by Andy and Uwe,
>     including some refactoring of the patches to reduce the amount of churn
>     in the patch-set
> 
> Changes in v2:
> - Fix coverletter subject
> - Drop accidentally included debugging patch
> - "[PATCH v3 02/15] ACPI / LPSS: Save Cherry Trail PWM ctx registers only once (
>   - Move #define LPSS_SAVE_CTX_ONCE define to group it with LPSS_SAVE_CTX
> 
> Regards,
> 
> Hans
>
Thierry Reding July 30, 2020, 9:26 a.m. UTC | #5
On Wed, Jul 29, 2020 at 11:32:28AM +0200, Hans de Goede wrote:
> cHi,
> 
> On 7/29/20 10:23 AM, Andy Shevchenko wrote:
> > On Mon, Jul 27, 2020 at 09:41:20AM +0200, Thierry Reding wrote:
> > > On Fri, Jul 17, 2020 at 03:37:37PM +0200, Hans de Goede wrote:
> > 
> > > I've applied patches 3 through 12 to the PWM tree. I thought it was a
> > > bit odd that only a handful of these patches had been reviewed and there
> > > were no Tested-bys, but I'm going to trust that you know what you're
> > > doing. =) If this breaks things for anyone I'm sure they'll complain.
> 
> Thank you for picking up these patches, but ...
> 
> > Can we postpone a bit?
> 
> I have to agree with Andy here, as mentioned my plan was to push the
> entire series through drm-intel-next-queued once the last few PWM
> patches are reviewed.
> 
> There are some fixes, to the pwm-crc driver which change behavior in
> a possibly undesirable way, unless combined with the i915 changes.
> 
> E.g. there is a fix which makes the pwm-crc driver actually honor
> the requested output frequency (it was not doing this due to a bug)
> and before the i915 changes, the i915 driver was hardcoding an output
> freq, rather then looking at the video-bios-tables as it should.
> 
> So having just the pwm-crc fix, will change the output frequency
> which some LCD panels might not like.
> 
> Note things are probably fine with the hardcoded output freq, but I
> would like to play it safe here.
> 
> Also Andy was still reviewing some of the PWM patches, and has requested
> changes to 1 patch, nothing functional just some code-reshuffling for
> cleaner code, so we could alternatively fix this up with a follow-up patch.
> 
> Either way please let us know how you want to proceed.

Okay, that's fine, I'll drop them again.

> > > That said I see that Rafael has acked patches 1-2 and Jani did so for
> > > patches 13-16. I'm not sure if you expect me to pick those patches up as
> > > well. As far as I can tell the ACPI, PWM and DRM parts are all
> > > independent, so these patches could be applied to the corresponding
> > > subsystem trees.
> > > 
> > > Anyway, if you want me to pick those all up into the PWM tree, I suppose
> > > that's something I can do as well.
> 
> drm-intel-next-queued is usually seeing quite a bit of churn, so the i915
> patches really should go upstream through that branch.

During my build tests I ran into a small issue caused by this series
interacting with the conversion of period and duty-cycle to u64 that
I've queued for v5.9. This causes a build failure on x86.

I have this local diff to fix that:

--- >8 ---
diff --git a/drivers/pwm/pwm-crc.c b/drivers/pwm/pwm-crc.c
index 370ab826a20b..92e838797733 100644
--- a/drivers/pwm/pwm-crc.c
+++ b/drivers/pwm/pwm-crc.c
@@ -76,7 +76,9 @@ static int crc_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
 
 	if (pwm_get_duty_cycle(pwm) != state->duty_cycle ||
 	    pwm_get_period(pwm) != state->period) {
-		int level = state->duty_cycle * PWM_MAX_LEVEL / state->period;
+		u64 level = state->duty_cycle * PWM_MAX_LEVEL;
+
+		do_div(level, state->period);
 
 		err = regmap_write(crc_pwm->regmap, PWM0_DUTY_CYCLE, level);
 		if (err) {
@@ -141,10 +143,9 @@ static void crc_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
 
 	clk_div = (clk_div_reg & ~PWM_OUTPUT_ENABLE) + 1;
 
-	state->period =
-		DIV_ROUND_UP(clk_div * NSEC_PER_USEC * 256, PWM_BASE_CLK_MHZ);
-	state->duty_cycle =
-		DIV_ROUND_UP(duty_cycle_reg * state->period, PWM_MAX_LEVEL);
+	state->period = DIV_ROUND_UP(clk_div * NSEC_PER_USEC * 256, PWM_BASE_CLK_MHZ);
+	state->duty_cycle = duty_cycle_reg * state->period + PWM_MAX_LEVEL - 1;
+	do_div(state->duty_cycle, PWM_MAX_LEVEL);
 	state->polarity = PWM_POLARITY_NORMAL;
 	state->enabled = !!(clk_div_reg & PWM_OUTPUT_ENABLE);
 }
--- >8 ---

So perhaps you want to integrate that or something equivalent into your
series.

Also this could result in a tricky dependency between PWM and drm-misc,
although if you're targetting drm-misc it's too late for v5.9 anyway. In
that case you should be able to rebase your series on v5.9-rc1 when it's
out and then you'll get the prerequisite PWM changes for the u64
conversion as part of that. No need to track the dependency explicitly.

Thierry
Hans de Goede Aug. 1, 2020, 2:33 p.m. UTC | #6
Hi Thierry,

On 7/30/20 11:26 AM, Thierry Reding wrote:
> On Wed, Jul 29, 2020 at 11:32:28AM +0200, Hans de Goede wrote:
>> cHi,
>>
>> On 7/29/20 10:23 AM, Andy Shevchenko wrote:
>>> On Mon, Jul 27, 2020 at 09:41:20AM +0200, Thierry Reding wrote:
>>>> On Fri, Jul 17, 2020 at 03:37:37PM +0200, Hans de Goede wrote:
>>>
>>>> I've applied patches 3 through 12 to the PWM tree. I thought it was a
>>>> bit odd that only a handful of these patches had been reviewed and there
>>>> were no Tested-bys, but I'm going to trust that you know what you're
>>>> doing. =) If this breaks things for anyone I'm sure they'll complain.
>>
>> Thank you for picking up these patches, but ...
>>
>>> Can we postpone a bit?
>>
>> I have to agree with Andy here, as mentioned my plan was to push the
>> entire series through drm-intel-next-queued once the last few PWM
>> patches are reviewed.
>>
>> There are some fixes, to the pwm-crc driver which change behavior in
>> a possibly undesirable way, unless combined with the i915 changes.
>>
>> E.g. there is a fix which makes the pwm-crc driver actually honor
>> the requested output frequency (it was not doing this due to a bug)
>> and before the i915 changes, the i915 driver was hardcoding an output
>> freq, rather then looking at the video-bios-tables as it should.
>>
>> So having just the pwm-crc fix, will change the output frequency
>> which some LCD panels might not like.
>>
>> Note things are probably fine with the hardcoded output freq, but I
>> would like to play it safe here.
>>
>> Also Andy was still reviewing some of the PWM patches, and has requested
>> changes to 1 patch, nothing functional just some code-reshuffling for
>> cleaner code, so we could alternatively fix this up with a follow-up patch.
>>
>> Either way please let us know how you want to proceed.
> 
> Okay, that's fine, I'll drop them again.

Great, thank you.

>>>> That said I see that Rafael has acked patches 1-2 and Jani did so for
>>>> patches 13-16. I'm not sure if you expect me to pick those patches up as
>>>> well. As far as I can tell the ACPI, PWM and DRM parts are all
>>>> independent, so these patches could be applied to the corresponding
>>>> subsystem trees.
>>>>
>>>> Anyway, if you want me to pick those all up into the PWM tree, I suppose
>>>> that's something I can do as well.
>>
>> drm-intel-next-queued is usually seeing quite a bit of churn, so the i915
>> patches really should go upstream through that branch.
> 
> During my build tests I ran into a small issue caused by this series
> interacting with the conversion of period and duty-cycle to u64 that
> I've queued for v5.9. This causes a build failure on x86.
> 
> I have this local diff to fix that:
> 
> --- >8 ---
> diff --git a/drivers/pwm/pwm-crc.c b/drivers/pwm/pwm-crc.c
> index 370ab826a20b..92e838797733 100644
> --- a/drivers/pwm/pwm-crc.c
> +++ b/drivers/pwm/pwm-crc.c
> @@ -76,7 +76,9 @@ static int crc_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
>   
>   	if (pwm_get_duty_cycle(pwm) != state->duty_cycle ||
>   	    pwm_get_period(pwm) != state->period) {
> -		int level = state->duty_cycle * PWM_MAX_LEVEL / state->period;
> +		u64 level = state->duty_cycle * PWM_MAX_LEVEL;
> +
> +		do_div(level, state->period);
>   
>   		err = regmap_write(crc_pwm->regmap, PWM0_DUTY_CYCLE, level);
>   		if (err) {
> @@ -141,10 +143,9 @@ static void crc_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
>   
>   	clk_div = (clk_div_reg & ~PWM_OUTPUT_ENABLE) + 1;
>   
> -	state->period =
> -		DIV_ROUND_UP(clk_div * NSEC_PER_USEC * 256, PWM_BASE_CLK_MHZ);
> -	state->duty_cycle =
> -		DIV_ROUND_UP(duty_cycle_reg * state->period, PWM_MAX_LEVEL);
> +	state->period = DIV_ROUND_UP(clk_div * NSEC_PER_USEC * 256, PWM_BASE_CLK_MHZ);
> +	state->duty_cycle = duty_cycle_reg * state->period + PWM_MAX_LEVEL - 1;
> +	do_div(state->duty_cycle, PWM_MAX_LEVEL);
>   	state->polarity = PWM_POLARITY_NORMAL;
>   	state->enabled = !!(clk_div_reg & PWM_OUTPUT_ENABLE);
>   }
> --- >8 ---
> 
> So perhaps you want to integrate that or something equivalent into your
> series.

Ack, thank you for letting me know.

> Also this could result in a tricky dependency between PWM and drm-misc,
> although if you're targetting drm-misc it's too late for v5.9 anyway. In
> that case you should be able to rebase your series on v5.9-rc1 when it's
> out and then you'll get the prerequisite PWM changes for the u64
> conversion as part of that. No need to track the dependency explicitly.

Right, I agree that this is too late for this cycle, so I will rebase it
on top of v5.9-rc1 once that is out, making sure to take the u64 changes
into account.

As said my plan is to merge this all through the drm tree, the assumption
there was that the pwm-crc and pwm-lpss drivers normally do not see a whole
lot of chsnges. Rebasing on top of 5.9-rc1 should take care of the u64
changes. So unless there are more pwm-core changes planned for the 5.10
cycle, I would like to queue this in the drm-intel tree for 5.10.
By thta time Andy and I should have also wrapped up the review process
(it is as good as wrapped up now already).

Thierry, is merging these through the drm-intel tree for 5.10 ok with
you? And if this is ok with you may I add your Acked-by to the PWM
patches to indicate this ?

Regards,

Hans
Hans de Goede Aug. 1, 2020, 2:38 p.m. UTC | #7
Hi,

On 7/29/20 12:54 PM, Andy Shevchenko wrote:
> On Fri, Jul 17, 2020 at 03:37:37PM +0200, Hans de Goede wrote:
>> Hi All,
>>
>> Here is v5 of my patch series converting the i915 driver's code for
>> controlling the panel's backlight with an external PWM controller to
>> use the atomic PWM API. See below for the changelog.
>>
>> This series consists of 4 parts:
>>
>> 1. acpi_lpss fixes workarounds for Cherry Trail DSTD nastiness
>> 2. various fixes to the pwm-lpss driver
>> 3. convert the pwm-crc driver to support the atomic PWM API and
>> 4. convert the i915 driver's PWM code to use the atomic PWM API
>>
>> The involved acpi_lpss and pwm drivers do not see a whole lot of churn,
>> so the plan is to merge this all through drm-intel-next-queued (dinq)
>> once all the patches are reviewed / have acks.
>>
>> Specifically patches 5-9, 11 still need an Acked- / Reviewed-by
>>
>> Andy, can you please take a look at the unreviewed patches? Specifically
>> patches 5-6 should address your review remarks from v4 of this set
>> and I've addressed your review remarks on patches 7-9 in v3 already.
>> A review of patch 11 would also be welcome
> 
> Done. Sorry for a delay.
> 
> One comment to consider, though. There are three channels in that PWM AFAIU.
> One of them is backlight control, another one can be attached to haptics. The
> concern is how this series may (or may not?) affect haptics behaviour.

When you say "in that PWM" do you mean the LPSS one or the CRC one ?

The LPSS patches are all fixes, they don't change behavior, so those
should be fine for the haptics.

The CRC PWM driver patches do make it honor the requested output frequency,
where before, because of a bug, it would stick with the out frequency
setup by the firmware (or the power-on-reset value if no value is set
by the firmware). This fix causing this is very clearly correct, but
it could have unexpected side-effects.

However I guess that you are talking about the LPSS PWM controllers
and there I expect no impact on haptic or other non backlight users
from these changes.

Regards,

Hans
Andy Shevchenko Aug. 2, 2020, 11:25 a.m. UTC | #8
On Sat, Aug 01, 2020 at 04:38:16PM +0200, Hans de Goede wrote:
> On 7/29/20 12:54 PM, Andy Shevchenko wrote:
> > On Fri, Jul 17, 2020 at 03:37:37PM +0200, Hans de Goede wrote:

...

> > One comment to consider, though. There are three channels in that PWM AFAIU.
> > One of them is backlight control, another one can be attached to haptics. The
> > concern is how this series may (or may not?) affect haptics behaviour.
> 
> When you say "in that PWM" do you mean the LPSS one or the CRC one ?

CRC one. I have read it from PMIC spec, that's why the question.

> The CRC PWM driver patches do make it honor the requested output frequency,
> where before, because of a bug, it would stick with the out frequency
> setup by the firmware (or the power-on-reset value if no value is set
> by the firmware). This fix causing this is very clearly correct, but
> it could have unexpected side-effects.
Hans de Goede Aug. 2, 2020, 7:43 p.m. UTC | #9
Hi,

On 8/2/20 1:25 PM, Andy Shevchenko wrote:
> On Sat, Aug 01, 2020 at 04:38:16PM +0200, Hans de Goede wrote:
>> On 7/29/20 12:54 PM, Andy Shevchenko wrote:
>>> On Fri, Jul 17, 2020 at 03:37:37PM +0200, Hans de Goede wrote:
> 
> ...
> 
>>> One comment to consider, though. There are three channels in that PWM AFAIU.
>>> One of them is backlight control, another one can be attached to haptics. The
>>> concern is how this series may (or may not?) affect haptics behaviour.
>>
>> When you say "in that PWM" do you mean the LPSS one or the CRC one ?
> 
> CRC one. I have read it from PMIC spec, that's why the question.

Ah I see, well the kernel driver only implements support for 1 PWM output,
the one which is used for the backlight brighness control.

So this series should not affect haptics behavior, since it looks like
the haptic functionality is not supported in the mainline kernel at all.

And I'm also not aware of any tablets with a CRC PMIC which have
(non working) haptic support.

Regards,

Hans