Fix bugs in kona pwm driver and pwm core
Subsystems (2)
v6 cover Fix bugs in kona pwm driver and pwm core
This patchset fixes a number of bugs in the Broadcom Kona pwm driver. It also fixes a bug in the pwm core where the enabled state was incorrect on failed calls to enable. Also, a new function was added to the pwm core to add pwm chips with inversed polarity for chips that have a different default polarity than the core. The prevents incorrect polarity being reported.
These changes are required for the Kona pwm driver to work on Cygnus and the same pwm IP is being used.
Changes from v5:
- Address Dmitry's comment on code cleanup of pwm_enable() in pwm core.
- Including all patches from different contributors in this patchset. Some were left out in v4.
Changes from v4:
- Rebased to Tim Kryger's patch that adds support in pwm core to add driver with inversed polarity.
- Removed patch 2 that resolved difference between hardware default polarity and pwm framework default polarity. The default polarity is set properly now when the pwm driver is registered with the pwm framework.
Arun Ramamurthy (1):
drivers: pwm: bcm-kona: Dont set polarity in probe
Jonathan Richardson (2):
pwm: kona: Fix incorrect config, disable, and polarity procedures
pwm: core: Set enable state properly on failed call to enable
Tim Kryger (1):
drivers: pwm: core: Add pwmchip_add_inversed
drivers/pwm/core.c | 52 +++++++++++++++++++++------ drivers/pwm/pwm-bcm-kona.c | 84 +++++++++++++++++++++++++++++++++----------- include/linux/pwm.h | 6 ++++ 3 files changed, 110 insertions(+), 32 deletions(-)
Signature · 1 line
1.7.9.5
PATCH v6 1/4 drivers: pwm: core: Add pwmchip_add_inversed
From: Tim Kryger <[email protected]>
Add a new function to register a PWM chip with channels that have their initial polarity as inversed. This benefits drivers of controllers that by default operate with inversed polarity by removing the need to modify the polarity during initialization.
--- drivers/pwm/core.c | 36 ++++++++++++++++++++++++++++-------- include/linux/pwm.h | 6 ++++++ 2 files changed, 34 insertions(+), 8 deletions(-)
drivers/pwm/core.c +28 -8
@@ -222,14 +222,8 @@ void *pwm_get_chip_data(struct pwm_device *pwm) }EXPORT_SYMBOL_GPL (pwm_get_chip_data); -/** -* pwmchip_add() - register a new PWM chip -* @chip: the PWM chip to add -* -* Register a new PWM chip. If chip->base < 0 then a dynamically assigned base -* will be used. -*/ -int pwmchip_add (struct pwm_chip *chip) +static int pwmchip_add_with_polarity (struct pwm_chip *chip, +enum pwm_polarity polarity) {struct pwm_device *pwm;unsigned int i;
@@ -259,6 +253,7 @@ int pwmchip_add(struct pwm_chip *chip) pwm->chip = chip; pwm->pwm = chip->base + i; pwm->hwpwm = i; + pwm->polarity = polarity;radix_tree_insert (&pwm_tree, pwm->pwm, pwm); }
@@ -279,9 +274,34 @@ out:mutex_unlock (&pwm_lock);return ret; } + +/** +* pwmchip_add() - register a new PWM chip +* @chip: the PWM chip to add +* +* Register a new PWM chip. If chip->base < 0 then a dynamically assigned base +* will be used. The initial polarity for all channels is normal. +*/ +int pwmchip_add (struct pwm_chip *chip) +{ +return pwmchip_add_with_polarity (chip,PWM_POLARITY_NORMAL ); +}EXPORT_SYMBOL_GPL (pwmchip_add);/** +* pwmchip_add_inversed() - register a new PWM chip +* @chip: the PWM chip to add +* +* Register a new PWM chip. If chip->base < 0 then a dynamically assigned base +* will be used. The initial polarity for all channels is inversed. +*/ +int pwmchip_add_inversed (struct pwm_chip *chip) +{ +return pwmchip_add_with_polarity (chip,PWM_POLARITY_INVERSED ); +} +EXPORT_SYMBOL_GPL (pwmchip_add_inversed); + +/** * pwmchip_remove() - remove a PWM chip * @chip: the PWM chip to remove *
include/linux/pwm.h +6 -0
@@ -183,6 +183,7 @@ int pwm_set_chip_data(struct pwm_device *pwm, void *data);void *pwm_get_chip_data (struct pwm_device *pwm);int pwmchip_add (struct pwm_chip *chip); +int pwmchip_add_inversed (struct pwm_chip *chip);int pwmchip_remove (struct pwm_chip *chip); struct pwm_device *pwm_request_from_chip(struct pwm_chip *chip, unsigned int index,
@@ -217,6 +218,11 @@ static inline int pwmchip_add(struct pwm_chip *chip)return -EINVAL ; } +static inline int pwmchip_add_inversed (struct pwm_chip *chip) +{ +return -EINVAL ; +} +static inline int pwmchip_remove (struct pwm_chip *chip) {return -EINVAL ;
Signature · 1 line
1.7.9.5
PATCH v6 2/4 drivers: pwm: bcm-kona: Dont set polarity in probe
In reply to Jonathan RichardsonFrom: Arun Ramamurthy <[email protected]>
Omit setting the polarity to normal during probe and instead use the new pwmchip_add_inversed function to register a PWM chip with default polarity of inversed for all channels as this is the actual hardware default.
--- drivers/pwm/pwm-bcm-kona.c | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-)
drivers/pwm/pwm-bcm-kona.c +3 -6
@@ -266,18 +266,15 @@ static int kona_pwmc_probe(struct platform_device *pdev)return ret; } -/* Set smooth mode, push/pull, and normal polarity for all channels */ -for (chan =0 ; chan < kp->chip.npwm; chan++) { - value |= (1 <<PWM_CONTROL_SMOOTH_SHIFT (chan)); +/* Set push/pull for all channels */ +for (chan =0 ; chan < kp->chip.npwm; chan++) value |= (1 <<PWM_CONTROL_TYPE_SHIFT (chan)); - value |= (1 <<PWM_CONTROL_POLARITY_SHIFT (chan)); - }writel (value, kp->base +PWM_CONTROL_OFFSET );clk_disable_unprepare (kp->clk); - ret =pwmchip_add (&kp->chip); + ret =pwmchip_add_inversed (&kp->chip);if (ret <0 )dev_err (&pdev->dev,"failed to add PWM chip: %d\n" , ret);
Signature · 1 line
1.7.9.5
PATCH v6 3/4 pwm: kona: Fix incorrect config, disable, and polarity procedures
In reply to Jonathan RichardsonThe config procedure didn't follow the spec which periodically resulted in failing to enable the output signal. This happened one in ten or twenty attempts. Following the spec and adding a 400ns delay in the appropriate locations resolves this problem.
The disable procedure now also follows the spec. The old disable procedure would result in no change in signal when called.
The polarity procedure no longer applies the settings to change the output signal because it can't be called when the pwm is enabled anyway. The polarity is only updated in the control register. The correct polarity will be applied on enable. The old method of applying changes would result in no signal when the polarity was changed. The new apply_settings function would fix this problem but it isn't required anyway.
--- drivers/pwm/pwm-bcm-kona.c | 75 +++++++++++++++++++++++++++++++++++--------- 1 file changed, 60 insertions(+), 15 deletions(-)
drivers/pwm/pwm-bcm-kona.c +60 -15
@@ -80,15 +80,19 @@ static void kona_pwmc_apply_settings(struct kona_pwmc *kp, unsigned int chan) {unsigned int value =readl (kp->base +PWM_CONTROL_OFFSET ); -/* Clear trigger bit but set smooth bit to maintain old output */ - value |=1 <<PWM_CONTROL_SMOOTH_SHIFT (chan); - value &= ~(1 <<PWM_CONTROL_TRIGGER_SHIFT (chan)); -writel (value, kp->base +PWM_CONTROL_OFFSET ); +/* +* There must be a min 400ns delay between clearing enable and setting +* it. Failing to do this may result in no PWM signal. +*/ +ndelay (400 );/* Set trigger bit and clear smooth bit to apply new settings */ value &= ~(1 <<PWM_CONTROL_SMOOTH_SHIFT (chan)); value |=1 <<PWM_CONTROL_TRIGGER_SHIFT (chan);writel (value, kp->base +PWM_CONTROL_OFFSET ); + +/* PWMOUT_ENABLE must be held high for at least 400 ns. */ +ndelay (400 ); } static int kona_pwmc_config(struct pwm_chip *chip, struct pwm_device *pwm,
@@ -121,20 +125,56 @@ static int kona_pwmc_config(struct pwm_chip *chip, struct pwm_device *pwm, dc =div64_u64 (val, div);/* If duty_ns or period_ns are not achievable then return */ -if (pc <PERIOD_COUNT_MIN || dc <DUTY_CYCLE_HIGH_MIN ) +if (pc <PERIOD_COUNT_MIN ) { +dev_warn (chip->dev, +"%s: pwm[%d]: period=%d is not achievable, pc=%lu, prescale=%lu\n" , + __func__, chan, period_ns, pc, prescale); +return -EINVAL ; + } + +/* If duty_ns is not achievable then return */ +if (dc <DUTY_CYCLE_HIGH_MIN ) { +if (0 != duty_ns) { +dev_warn (chip->dev, +"%s: pwm[%d]: duty cycle=%d is not achievable, dc=%lu, prescale=%lu\n" , + __func__, chan, duty_ns, dc, prescale); + }return -EINVAL ; + }/* If pc and dc are in bounds, the calculation is done */ if (pc <=PERIOD_COUNT_MAX && dc <=DUTY_CYCLE_HIGH_MAX )break ;/* Otherwise, increase prescale and recalculate pc and dc */ -if (++prescale >PRESCALE_MAX ) +if (++prescale >PRESCALE_MAX ) { +dev_warn (chip->dev, +"%s: pwm[%d]: Prescale (=%lu) within max (=%d) for period=%d and duty cycle=%d is not achievable\n" , + __func__, chan, prescale,PRESCALE_MAX , + period_ns, duty_ns);return -EINVAL ; + } } -/* If the PWM channel is enabled, write the settings to the HW */ +dev_dbg (chip->dev,"pwm[%d]: period=%lu, duty_high=%lu, prescale=%lu\n" , + chan, pc, dc, prescale); + +/* +* Don't apply settings if disabled. The period and duty cycle are +* always calculated above to ensure the new values are +* validated immediately instead of on enable. +*/ if (test_bit (PWMF_ENABLED , &pwm->flags)) { + value =readl (kp->base +PWM_CONTROL_OFFSET ); + +/* +* Clear trigger bit but set smooth bit to maintain old +* output. +*/ + value |=1 <<PWM_CONTROL_SMOOTH_SHIFT (chan); + value &= ~(1 <<PWM_CONTROL_TRIGGER_SHIFT (chan)); +writel (value, kp->base +PWM_CONTROL_OFFSET ); + value =readl (kp->base +PRESCALE_OFFSET ); value &= ~PRESCALE_MASK (chan); value |= prescale <<PRESCALE_SHIFT (chan);
@@ -173,11 +213,6 @@ static int kona_pwmc_set_polarity(struct pwm_chip *chip, struct pwm_device *pwm,writel (value, kp->base +PWM_CONTROL_OFFSET ); -kona_pwmc_apply_settings (kp, chan); - -/* Wait for waveform to settle before gating off the clock */ -ndelay (400 ); -clk_disable_unprepare (kp->clk);return 0 ;
@@ -207,13 +242,23 @@ static void kona_pwmc_disable(struct pwm_chip *chip, struct pwm_device *pwm) {struct kona_pwmc *kp =to_kona_pwmc (chip);unsigned int chan = pwm->hwpwm; +unsigned int value =readl (kp->base +PWM_CONTROL_OFFSET ); + +/* Set smooth type to 1 and disable */ + value |=1 <<PWM_CONTROL_SMOOTH_SHIFT (chan); + value &= ~(1 <<PWM_CONTROL_TRIGGER_SHIFT (chan)); +writel (value, kp->base +PWM_CONTROL_OFFSET );/* Simulate a disable by configuring for zero duty */ writel (0 , kp->base +DUTY_CYCLE_HIGH_OFFSET (chan)); -kona_pwmc_apply_settings (kp, chan); +writel (0 , kp->base +PERIOD_COUNT_OFFSET (chan)); -/* Wait for waveform to settle before gating off the clock */ -ndelay (400 ); +/* Set prescale to 0 for this channel */ + value =readl (kp->base +PRESCALE_OFFSET ); + value &= ~PRESCALE_MASK (chan); +writel (value, kp->base +PRESCALE_OFFSET ); + +kona_pwmc_apply_settings (kp, chan);clk_disable_unprepare (kp->clk); }
Signature · 1 line
1.7.9.5
Re: [PATCH v6 3/4] pwm: kona: Fix incorrect config, disable, and polarity procedures
Quoted 7 lines · Jonathan Richardson
The polarity procedure no longer applies the settings to change the output signal because it can't be called when the pwm is enabled anyway. The polarity is only updated in the control register. The correct polarity will be applied on enable. The old method of applying changes would result in no signal when the polarity was changed. The new
apply_settingsfunction would fix this problem but it isn't required anyway.
Why does this bug fix need to alter when polarity changes take effect?
That is an an unrelated change and I don't really agree with it.
Quoted 23 lines
/* If duty_ns or period_ns are not achievable then return */(quoted diff) +15
-1- if (pc < PERIOD_COUNT_MIN || dc < DUTY_CYCLE_HIGH_MIN) + if (pc < PERIOD_COUNT_MIN) { + dev_warn(chip->dev, + "%s: pwm[%d]: period=%d is not achievable, pc=%lu, prescale=%lu\n", + __func__, chan, period_ns, pc, prescale); + return -EINVAL; + } + + /* If duty_ns is not achievable then return */ + if (dc < DUTY_CYCLE_HIGH_MIN) { + if (0 != duty_ns) { + dev_warn(chip->dev, + "%s: pwm[%d]: duty cycle=%d is not achievable, dc=%lu, prescale=%lu\n", + __func__, chan, duty_ns, dc, prescale); + } return -EINVAL; + } /* If pc and dc are in bounds, the calculation is done */ if (pc <= PERIOD_COUNT_MAX && dc <= DUTY_CYCLE_HIGH_MAX) break;
Review
Quoted 10 lines
(quoted diff) +6
-1/* Otherwise, increase prescale and recalculate pc and dc */ - if (++prescale > PRESCALE_MAX) + if (++prescale > PRESCALE_MAX) { + dev_warn(chip->dev, + "%s: pwm[%d]: Prescale (=%lu) within max (=%d) for period=%d and duty cycle=%d is not achievable\n", + __func__, chan, prescale, PRESCALE_MAX, + period_ns, duty_ns); return -EINVAL; + } }
This extra debug output seems helpful but could you put it in its own commit and keep this focused on fixing the instability you observed?
Review
Quoted 8 lines
(quoted diff) +8
-0+ /* + * Clear trigger bit but set smooth bit to maintain old + * output. + */ + value |= 1 << PWM_CONTROL_SMOOTH_SHIFT(chan); + value &= ~(1 << PWM_CONTROL_TRIGGER_SHIFT(chan)); + writel(value, kp->base + PWM_CONTROL_OFFSET); +
The above lines are duplicated below. Perhaps a function is in order?
Review
Quoted 4 lines
(quoted diff) +4
-0+ /* Set smooth type to 1 and disable */ + value |= 1 << PWM_CONTROL_SMOOTH_SHIFT(chan); + value &= ~(1 << PWM_CONTROL_TRIGGER_SHIFT(chan)); + writel(value, kp->base + PWM_CONTROL_OFFSET);
It seems like the important parts of the fix could be distilled down to something like this:
drivers/pwm/pwm-bcm-kona.c +0 -0
@@ -76,7 +76,8 @@ static inline struct kona_pwmc *to_kona_pwmc(struct
pwm_chip *_chip)
return container_of(_chip, struct kona_pwmc, chip);
}
-static void kona_pwmc_apply_settings(struct kona_pwmc *kp, unsigned int chan) +static void kona_pwmc_prepare_for_settings(struct kona_pwmc *kp,
+ unsigned int chan)
{
unsigned int value = readl(kp->base + PWM_CONTROL_OFFSET);@@ -85,10 +86,19 @@ static void kona_pwmc_apply_settings(struct kona_pwmc *kp, unsigned int chan)
value &= ~(1 << PWM_CONTROL_TRIGGER_SHIFT(chan));
writel(value, kp->base + PWM_CONTROL_OFFSET);+ ndelay(400);
+}
+
+static void kona_pwmc_apply_settings(struct kona_pwmc *kp, unsigned int chan) +{
+ unsigned int value = readl(kp->base + PWM_CONTROL_OFFSET);
+
/* Set trigger bit and clear smooth bit to apply new settings */
value &= ~(1 << PWM_CONTROL_SMOOTH_SHIFT(chan));
value |= 1 << PWM_CONTROL_TRIGGER_SHIFT(chan);
writel(value, kp->base + PWM_CONTROL_OFFSET);+
+ ndelay(400);
}
static int kona_pwmc_config(struct pwm_chip *chip, struct pwm_device *pwm,
@@ -135,6 +145,8 @@ static int kona_pwmc_config(struct pwm_chip *chip, struct pwm_device *pwm,
/* If the PWM channel is enabled, write the settings to the HW */
if (test_bit(PWMF_ENABLED, &pwm->flags)) {
+ kona_pwmc_prepare_for_settings(kp, chan);
+
value = readl(kp->base + PRESCALE_OFFSET);
value &= ~PRESCALE_MASK(chan);
value |= prescale << PRESCALE_SHIFT(chan);
@@ -164,6 +176,8 @@ static int kona_pwmc_set_polarity(struct pwm_chip
*chip, struct pwm_device *pwm,
return ret;
}+ kona_pwmc_prepare_for_settings(kp, chan);
+
value = readl(kp->base + PWM_CONTROL_OFFSET);
if (polarity == PWM_POLARITY_NORMAL)@@ -175,9 +189,6 @@ static int kona_pwmc_set_polarity(struct pwm_chip *chip, struct pwm_device *pwm,
kona_pwmc_apply_settings(kp, chan);
- /* Wait for waveform to settle before gating off the clock */
- ndelay(400);
-
clk_disable_unprepare(kp->clk);
return 0;
@@ -209,12 +220,10 @@ static void kona_pwmc_disable(struct pwm_chip
*chip, struct pwm_device *pwm)
unsigned int chan = pwm->hwpwm;
/* Simulate a disable by configuring for zero duty */
+ kona_pwmc_prepare_for_settings(kp, chan);
writel(0, kp->base + DUTY_CYCLE_HIGH_OFFSET(chan));
kona_pwmc_apply_settings(kp, chan);- /* Wait for waveform to settle before gating off the clock */
- ndelay(400);
-
clk_disable_unprepare(kp->clk);
}
Re: [PATCH v6 3/4] pwm: kona: Fix incorrect config, disable, and polarity procedures
Tim, just one comment below. All other suggestions will be made for the next patch set.
Quoted 11 lines · Tim Kryger
Quoted 7 lines · Jonathan Richardson
The polarity procedure no longer applies the settings to change the output signal because it can't be called when the pwm is enabled anyway. The polarity is only updated in the control register. The correct polarity will be applied on enable. The old method of applying changes would result in no signal when the polarity was changed. The new
apply_settingsfunction would fix this problem but it isn't required anyway.
Quoted 1 line
Why does this bug fix need to alter when polarity changes take effect?
The original kona_pwmc_set_polarity() function didn't work on Cygnus because of the updated kona_pwmc_apply_settings() that we fixed for to use the updated method received from the chip team. In the process of fixing it I just took out the apply settings completely because the polarity can only be changed when the pwm is disabled (from core.c) and it seemed simpler to just write the polarity to the config register and let the enable procedure handle the application of polarity when the signal was actually enabled. I didn't see the point of trying to apply the polarity when the output signal is disabled. It's the same as writing the duty cycle and period when the pwm is disabled - the settings are stored but don't take effect until enabled. I didn't see any change in signal behaviour by simplifying the polarity function.
Quoted 141 lines
That is an an unrelated change and I don't really agree with it.
Quoted 23 lines
/* If duty_ns or period_ns are not achievable then return */(quoted diff) +15
-1- if (pc < PERIOD_COUNT_MIN || dc < DUTY_CYCLE_HIGH_MIN) + if (pc < PERIOD_COUNT_MIN) { + dev_warn(chip->dev, + "%s: pwm[%d]: period=%d is not achievable, pc=%lu, prescale=%lu\n", + __func__, chan, period_ns, pc, prescale); + return -EINVAL; + } + + /* If duty_ns is not achievable then return */ + if (dc < DUTY_CYCLE_HIGH_MIN) { + if (0 != duty_ns) { + dev_warn(chip->dev, + "%s: pwm[%d]: duty cycle=%d is not achievable, dc=%lu, prescale=%lu\n", + __func__, chan, duty_ns, dc, prescale); + } return -EINVAL; + } /* If pc and dc are in bounds, the calculation is done */ if (pc <= PERIOD_COUNT_MAX && dc <= DUTY_CYCLE_HIGH_MAX) break;Review
Quoted 10 lines
(quoted diff) +6
-1/* Otherwise, increase prescale and recalculate pc and dc */ - if (++prescale > PRESCALE_MAX) + if (++prescale > PRESCALE_MAX) { + dev_warn(chip->dev, + "%s: pwm[%d]: Prescale (=%lu) within max (=%d) for period=%d and duty cycle=%d is not achievable\n", + __func__, chan, prescale, PRESCALE_MAX, + period_ns, duty_ns); return -EINVAL; + } }This extra debug output seems helpful but could you put it in its own commit and keep this focused on fixing the instability you observed?
Review
Quoted 8 lines
(quoted diff) +8
-0+ /* + * Clear trigger bit but set smooth bit to maintain old + * output. + */ + value |= 1 << PWM_CONTROL_SMOOTH_SHIFT(chan); + value &= ~(1 << PWM_CONTROL_TRIGGER_SHIFT(chan)); + writel(value, kp->base + PWM_CONTROL_OFFSET); +The above lines are duplicated below. Perhaps a function is in order?
Review
Quoted 4 lines
(quoted diff) +4
-0+ /* Set smooth type to 1 and disable */ + value |= 1 << PWM_CONTROL_SMOOTH_SHIFT(chan); + value &= ~(1 << PWM_CONTROL_TRIGGER_SHIFT(chan)); + writel(value, kp->base + PWM_CONTROL_OFFSET);It seems like the important parts of the fix could be distilled down to something like this:
drivers/pwm/pwm-bcm-kona.c +11
-1@@ -76,7 +76,8 @@ static inline struct kona_pwmc *to_kona_pwmc(struct pwm_chip *_chip) return container_of(_chip, struct kona_pwmc, chip); } -static void kona_pwmc_apply_settings (struct kona_pwmc *kp,unsigned int chan) +static void kona_pwmc_prepare_for_settings (struct kona_pwmc *kp, +unsigned int chan) {unsigned int value =readl (kp->base +PWM_CONTROL_OFFSET );@@ -85,10 +86,19 @@ static void kona_pwmc_apply_settings(struct kona_pwmc *kp, unsigned int chan) value &= ~(1 << PWM_CONTROL_TRIGGER_SHIFT(chan));writel (value, kp->base +PWM_CONTROL_OFFSET ); +ndelay (400 ); +} + +static void kona_pwmc_apply_settings (struct kona_pwmc *kp,unsigned int chan) +{ +unsigned int value =readl (kp->base +PWM_CONTROL_OFFSET ); +/* Set trigger bit and clear smooth bit to apply new settings */ value &= ~(1 <<PWM_CONTROL_SMOOTH_SHIFT (chan)); value |=1 <<PWM_CONTROL_TRIGGER_SHIFT (chan);writel (value, kp->base +PWM_CONTROL_OFFSET ); + +ndelay (400 ); }static int kona_pwmc_config(struct
pwm_chip*chip, structpwm_device*pwm,drivers/pwm/pwm-bcm-kona.c +2
-0@@ -135,6 +145,8 @@ static int kona_pwmc_config(struct pwm_chip *chip,struct pwm_device *pwm,/* If the PWM channel is enabled, write the settings to the HW */ if (test_bit(PWMF_ENABLED, &pwm->flags)) { +kona_pwmc_prepare_for_settings (kp ,chan ); + value =readl (kp->base +PRESCALE_OFFSET ); value &= ~PRESCALE_MASK (chan);value |= prescale << PRESCALE_SHIFT(chan);drivers/pwm/pwm-bcm-kona.c +2
-0@@ -164,6 +176,8 @@ static int kona_pwmc_set_polarity(struct pwm_chip *chip, struct pwm_device *pwm, return ret; } +kona_pwmc_prepare_for_settings (kp, chan); + value =readl (kp->base +PWM_CONTROL_OFFSET );if (polarity == PWM_POLARITY_NORMAL)drivers/pwm/pwm-bcm-kona.c +0
-3@@ -175,9 +189,6 @@ static int kona_pwmc_set_polarity(struct pwm_chip *chip, struct pwm_device *pwm,kona_pwmc_apply_settings (kp, chan); -/* Wait for waveform to settle before gating off the clock */ -ndelay (400 ); -clk_disable_unprepare (kp->clk);return 0;drivers/pwm/pwm-bcm-kona.c +1
-3@@ -209,12 +220,10 @@ static void kona_pwmc_disable(struct pwm_chip *chip, struct pwm_device *pwm)unsigned int chan = pwm->hwpwm;/* Simulate a disable by configuring for zero duty */ +kona_pwmc_prepare_for_settings (kp, chan);writel (0 , kp->base +DUTY_CYCLE_HIGH_OFFSET (chan));kona_pwmc_apply_settings (kp, chan); -/* Wait for waveform to settle before gating off the clock */ -ndelay (400 ); -clk_disable_unprepare (kp->clk); }
PATCH v6 4/4 pwm: core: Set enable state properly on failed call to enable
In reply to Jonathan RichardsonThe pwm_enable function didn't clear the enabled bit if a call to a clients enable function returned an error. The result was that the state of the pwm core was wrong. Clearing the bit when enable returns an error ensures the state is properly set.
--- drivers/pwm/core.c | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-)
drivers/pwm/core.c +13 -3
@@ -477,10 +477,20 @@ EXPORT_SYMBOL_GPL(pwm_set_polarity);*/ int pwm_enable (struct pwm_device *pwm) { -if (pwm && !test_and_set_bit (PWMF_ENABLED , &pwm->flags)) -return pwm->chip->ops->enable (pwm->chip, pwm); +int err; + +if (!pwm) +return -EINVAL ; + +if (!test_and_set_bit (PWMF_ENABLED , &pwm->flags)) { + err = pwm->chip->ops->enable (pwm->chip, pwm); +if (err) { +clear_bit (PWMF_ENABLED , &pwm->flags); +return err; + } + } -return pwm ?0 : -EINVAL ; +return 0 ; }EXPORT_SYMBOL_GPL (pwm_enable);
Signature · 1 line
1.7.9.5
Hi Jonathan,
Patcheset looks good.
Quoted 36 lines · Jonathan Richardson
This patchset fixes a number of bugs in the Broadcom Kona pwm driver. It also fixes a bug in the pwm core where the enabled state was incorrect on failed calls to enable. Also, a new function was added to the pwm core to add pwm chips with inversed polarity for chips that have a different default polarity than the core. The prevents incorrect polarity being reported.
These changes are required for the Kona pwm driver to work on Cygnus and the same pwm IP is being used.
Changes from v5:
- Address Dmitry's comment on code cleanup ofpwm_enable()in pwm core.
- Including all patches from different contributors in this patchset. Some were left out in v4.Changes from v4:
- Rebased to Tim Kryger's patch that adds support in pwm core to add driver with inversed polarity.
- Removed patch 2 that resolved difference between hardware default polarity and pwm framework default polarity. The default polarity is set properly now when the pwm driver is registered with the pwm framework.Arun Ramamurthy (1):
drivers: pwm: bcm-kona: Dont set polarity in probeJonathan Richardson (2):
pwm: kona: Fix incorrect config, disable, and polarity procedures
pwm: core: Set enable state properly on failed call to enableTim Kryger (1):
drivers: pwm: core: Addpwmchip_add_inverseddrivers/pwm/core.c | 52 +++++++++++++++++++++------ drivers/pwm/pwm-bcm-kona.c | 84 +++++++++++++++++++++++++++++++++----------- include/linux/pwm.h | 6 ++++ 3 files changed, 110 insertions(+), 32 deletions(-)
Pinging maintainer and others. Could you please let me know when you can provide feedback on this patchset.
Tim, were you ok with these changes now or do you still have concerns about changing the driver? I didn't hear back from after our last conversation. If there are still concerns we could possibly make a new cygnus pwm driver, though it would be mostly the same as the existing kona driver. Or add a DT version to the existing driver that has the new modified behaviour - though I think this would be messy.
Thanks.
Quoted 36 lines · Jonathan Richardson
This patchset fixes a number of bugs in the Broadcom Kona pwm driver. It also fixes a bug in the pwm core where the enabled state was incorrect on failed calls to enable. Also, a new function was added to the pwm core to add pwm chips with inversed polarity for chips that have a different default polarity than the core. The prevents incorrect polarity being reported.
These changes are required for the Kona pwm driver to work on Cygnus and the same pwm IP is being used.
Changes from v5:
- Address Dmitry's comment on code cleanup ofpwm_enable()in pwm core.
- Including all patches from different contributors in this patchset. Some were left out in v4.Changes from v4:
- Rebased to Tim Kryger's patch that adds support in pwm core to add driver with inversed polarity.
- Removed patch 2 that resolved difference between hardware default polarity and pwm framework default polarity. The default polarity is set properly now when the pwm driver is registered with the pwm framework.Arun Ramamurthy (1):
drivers: pwm: bcm-kona: Dont set polarity in probeJonathan Richardson (2):
pwm: kona: Fix incorrect config, disable, and polarity procedures
pwm: core: Set enable state properly on failed call to enableTim Kryger (1):
drivers: pwm: core: Addpwmchip_add_inverseddrivers/pwm/core.c | 52 +++++++++++++++++++++------ drivers/pwm/pwm-bcm-kona.c | 84 +++++++++++++++++++++++++++++++++----------- include/linux/pwm.h | 6 ++++ 3 files changed, 110 insertions(+), 32 deletions(-)