Improve pwm-ir-tx precision
Subsystems (22)
Sean Young
maintainerv6 cover Improve pwm-ir-tx precision
The pwm-ir-tx driver has to turn the pwm signal on and off, and suffers from delays as this is done in process context. Make this work in atomic context.
changes:
v6:
- Rename pwm_apply_cansleep() to pwm_apply_might_sleep()
- rebase
v5:
- Use dev_err_probe() as Uwe suggested
- rebase
v4:
- Split out the pwm_apply_state() rename into seperate commit
- Atomic pwm function is called pwm_apply_atomic() and sleeping counterpart pwm_apply_cansleep()
- Fix commit messages
v3:
- Renamed pwm_apply_state() to pwm_apply_cansleep()
- Moved pwm atomic to struct pwm_chip
- Fixed pwm docs
- Other minor code review comments
v2:
- Simplified the pwm ir tx based on Ivaylo Dimitrov suggestions
- Renamed pwm_can_sleep() to pwm_is_atomic
- Fix sleeping issue in pwm-bcm2835 (clk_get_rate() can sleep)
- use non_block_start() in pwm to check atomic-ness
Sean Young (4):
pwm: rename pwm_apply_state() to pwm_apply_might_sleep()
pwm: make it possible to apply pwm changes in atomic context
pwm: bcm2835: allow pwm driver to be used in atomic context
media: pwm-ir-tx: trigger edges from hrtimer interrupt context
Documentation/driver-api/pwm.rst | 17 +++- .../gpu/drm/i915/display/intel_backlight.c | 6 +- drivers/gpu/drm/solomon/ssd130x.c | 2 +- drivers/hwmon/pwm-fan.c | 8 +- drivers/input/misc/da7280.c | 4 +- drivers/input/misc/pwm-beeper.c | 4 +- drivers/input/misc/pwm-vibra.c | 8 +- drivers/leds/leds-pwm.c | 2 +- drivers/leds/rgb/leds-pwm-multicolor.c | 4 +- drivers/media/rc/pwm-ir-tx.c | 83 +++++++++++++++++-- drivers/platform/x86/lenovo-yogabook.c | 2 +- drivers/pwm/core.c | 75 +++++++++++++---- drivers/pwm/pwm-bcm2835.c | 31 ++++--- drivers/pwm/pwm-renesas-tpu.c | 1 - drivers/pwm/pwm-twl-led.c | 2 +- drivers/pwm/pwm-vt8500.c | 2 +- drivers/pwm/sysfs.c | 10 +-- drivers/regulator/pwm-regulator.c | 4 +- drivers/video/backlight/lm3630a_bl.c | 2 +- drivers/video/backlight/lp855x_bl.c | 2 +- drivers/video/backlight/pwm_bl.c | 12 +-- drivers/video/fbdev/ssd1307fb.c | 2 +- include/linux/pwm.h | 57 +++++++++---- 23 files changed, 248 insertions(+), 92 deletions(-)
Signature · 1 line
2.43.0
Sean Young
maintainerPATCH v6 1/4 pwm: rename pwm_apply_state() to pwm_apply_might_sleep()
In order to introduce a pwm api which can be used from atomic context, we will need two functions for applying pwm changes:
int pwm_apply_might_sleep(struct pwm *, struct pwm_state *); int pwm_apply_atomic(struct pwm *, struct pwm_state *);
This commit just deals with renaming pwm_apply_state(), a following commit will introduce the pwm_apply_atomic() function.
--- Documentation/driver-api/pwm.rst | 8 +++--- .../gpu/drm/i915/display/intel_backlight.c | 6 ++-- drivers/gpu/drm/solomon/ssd130x.c | 2 +- drivers/hwmon/pwm-fan.c | 8 +++--- drivers/input/misc/da7280.c | 4 +-- drivers/input/misc/pwm-beeper.c | 4 +-- drivers/input/misc/pwm-vibra.c | 8 +++--- drivers/leds/leds-pwm.c | 2 +- drivers/leds/rgb/leds-pwm-multicolor.c | 4 +-- drivers/media/rc/pwm-ir-tx.c | 4 +-- drivers/platform/x86/lenovo-yogabook.c | 2 +- drivers/pwm/core.c | 18 ++++++------ drivers/pwm/pwm-twl-led.c | 2 +- drivers/pwm/pwm-vt8500.c | 2 +- drivers/pwm/sysfs.c | 10 +++---- drivers/regulator/pwm-regulator.c | 4 +-- drivers/video/backlight/lm3630a_bl.c | 2 +- drivers/video/backlight/lp855x_bl.c | 2 +- drivers/video/backlight/pwm_bl.c | 12 ++++---- drivers/video/fbdev/ssd1307fb.c | 2 +- include/linux/pwm.h | 28 +++++++++---------- 21 files changed, 67 insertions(+), 67 deletions(-)
Documentation/driver-api/pwm.rst +4 -4
@@ -41,7 +41,7 @@ the getter, devm_pwm_get() and devm_fwnode_pwm_get(), also exist. After being requested, a PWM has to be configured using:: - int pwm_apply_state(struct pwm_device *pwm, struct pwm_state *state); + int pwm_apply_might_sleep(struct pwm_device *pwm, struct pwm_state *state); This API controls both the PWM period/duty_cycle config and the enable/disable state.
@@ -57,13 +57,13 @@ If supported by the driver, the signal can be optimized, for example to improve EMI by phase shifting the individual channels of a chip. The pwm_config(), pwm_enable() and pwm_disable() functions are just wrappers -around pwm_apply_state() and should not be used if the user wants to change +around pwm_apply_might_sleep() and should not be used if the user wants to change several parameter at once. For example, if you see pwm_config() and pwm_{enable,disable}() calls in the same function, this probably means you -should switch to pwm_apply_state(). +should switch to pwm_apply_might_sleep(). The PWM user API also allows one to query the PWM state that was passed to the -last invocation of pwm_apply_state() using pwm_get_state(). Note this is +last invocation of pwm_apply_might_sleep() using pwm_get_state(). Note this is different to what the driver has actually implemented if the request cannot be satisfied exactly with the hardware in use. There is currently no way for consumers to get the actually implemented settings.
drivers/gpu/drm/i915/display/intel_backlight.c +3 -3
@@ -274,7 +274,7 @@ static void ext_pwm_set_backlight(const struct drm_connector_state *conn_state,struct intel_panel *panel = &to_intel_connector (conn_state->connector)->panel;pwm_set_relative_duty_cycle (&panel->backlight.pwm_state, level,100 ); -pwm_apply_state (panel->backlight.pwm, &panel->backlight.pwm_state); +pwm_apply_might_sleep (panel->backlight.pwm, &panel->backlight.pwm_state); } static void
@@ -427,7 +427,7 @@ static void ext_pwm_disable_backlight(const struct drm_connector_state *old_connintel_backlight_set_pwm_level (old_conn_state, level); panel->backlight.pwm_state.enabled = false; -pwm_apply_state (panel->backlight.pwm, &panel->backlight.pwm_state); +pwm_apply_might_sleep (panel->backlight.pwm, &panel->backlight.pwm_state); }void intel_backlight_disable (const struct drm_connector_state *old_conn_state)
@@ -749,7 +749,7 @@ static void ext_pwm_enable_backlight(const struct intel_crtc_state *crtc_state,pwm_set_relative_duty_cycle (&panel->backlight.pwm_state, level,100 ); panel->backlight.pwm_state.enabled = true; -pwm_apply_state (panel->backlight.pwm, &panel->backlight.pwm_state); +pwm_apply_might_sleep (panel->backlight.pwm, &panel->backlight.pwm_state); } static void __intel_backlight_enable(const struct intel_crtc_state *crtc_state,
drivers/gpu/drm/solomon/ssd130x.c +1 -1
@@ -319,7 +319,7 @@ static int ssd130x_pwm_enable(struct ssd130x_device *ssd130x)pwm_init_state (ssd130x->pwm, &pwmstate);pwm_set_relative_duty_cycle (&pwmstate,50 ,100 ); -pwm_apply_state (ssd130x->pwm, &pwmstate); +pwm_apply_might_sleep (ssd130x->pwm, &pwmstate);/* Enable the PWM */ pwm_enable (ssd130x->pwm);
drivers/hwmon/pwm-fan.c +4 -4
@@ -151,7 +151,7 @@ static int pwm_fan_power_on(struct pwm_fan_ctx *ctx) } state->enabled = true; - ret =pwm_apply_state (ctx->pwm, state); + ret =pwm_apply_might_sleep (ctx->pwm, state);if (ret) {dev_err (ctx->dev,"failed to enable PWM\n" ); goto disable_regulator;
@@ -181,7 +181,7 @@ static int pwm_fan_power_off(struct pwm_fan_ctx *ctx) state->enabled = false; state->duty_cycle =0 ; - ret =pwm_apply_state (ctx->pwm, state); + ret =pwm_apply_might_sleep (ctx->pwm, state);if (ret) {dev_err (ctx->dev,"failed to disable PWM\n" );return ret;
@@ -207,7 +207,7 @@ static int __set_pwm(struct pwm_fan_ctx *ctx, unsigned long pwm) period = state->period; state->duty_cycle =DIV_ROUND_UP (pwm * (period -1 ),MAX_PWM ); - ret =pwm_apply_state (ctx->pwm, state); + ret =pwm_apply_might_sleep (ctx->pwm, state);if (ret)return ret; ret =pwm_fan_power_on (ctx);
@@ -278,7 +278,7 @@ static int pwm_fan_update_enable(struct pwm_fan_ctx *ctx, long val) state, &enable_regulator); -pwm_apply_state (ctx->pwm, state); +pwm_apply_might_sleep (ctx->pwm, state);pwm_fan_switch_power (ctx, enable_regulator);pwm_fan_update_state (ctx,0 ); }
drivers/input/misc/da7280.c +2 -2
@@ -352,7 +352,7 @@ static int da7280_haptic_set_pwm(struct da7280_haptic *haptics, bool enabled) state.duty_cycle = period_mag_multi; } - error =pwm_apply_state (haptics->pwm_dev, &state); + error =pwm_apply_might_sleep (haptics->pwm_dev, &state);if (error)dev_err (haptics->dev,"Failed to apply pwm state: %d\n" , error);
@@ -1175,7 +1175,7 @@ static int da7280_probe(struct i2c_client *client)/* Sync up PWM state and ensure it is off. */ pwm_init_state (haptics->pwm_dev, &state); state.enabled = false; - error =pwm_apply_state (haptics->pwm_dev, &state); + error =pwm_apply_might_sleep (haptics->pwm_dev, &state);if (error) {dev_err (dev,"Failed to apply PWM state: %d\n" , error);return error;
drivers/input/misc/pwm-beeper.c +2 -2
@@ -39,7 +39,7 @@ static int pwm_beeper_on(struct pwm_beeper *beeper, unsigned long period) state.period = period;pwm_set_relative_duty_cycle (&state,50 ,100 ); - error =pwm_apply_state (beeper->pwm, &state); + error =pwm_apply_might_sleep (beeper->pwm, &state);if (error)return error;
@@ -138,7 +138,7 @@ static int pwm_beeper_probe(struct platform_device *pdev)/* Sync up PWM state and ensure it is off. */ pwm_init_state (beeper->pwm, &state); state.enabled = false; - error =pwm_apply_state (beeper->pwm, &state); + error =pwm_apply_might_sleep (beeper->pwm, &state);if (error) {dev_err (dev,"failed to apply initial PWM state: %d\n" , error);
drivers/input/misc/pwm-vibra.c +4 -4
@@ -56,7 +56,7 @@ static int pwm_vibrator_start(struct pwm_vibrator *vibrator)pwm_set_relative_duty_cycle (&state, vibrator->level,0xffff ); state.enabled = true; - err =pwm_apply_state (vibrator->pwm, &state); + err =pwm_apply_might_sleep (vibrator->pwm, &state);if (err) {dev_err (pdev,"failed to apply pwm state: %d\n" , err);return err;
@@ -67,7 +67,7 @@ static int pwm_vibrator_start(struct pwm_vibrator *vibrator) state.duty_cycle = vibrator->direction_duty_cycle; state.enabled = true; - err =pwm_apply_state (vibrator->pwm_dir, &state); + err =pwm_apply_might_sleep (vibrator->pwm_dir, &state);if (err) {dev_err (pdev,"failed to apply dir-pwm state: %d\n" , err);pwm_disable (vibrator->pwm);
@@ -160,7 +160,7 @@ static int pwm_vibrator_probe(struct platform_device *pdev)/* Sync up PWM state and ensure it is off. */ pwm_init_state (vibrator->pwm, &state); state.enabled = false; - err =pwm_apply_state (vibrator->pwm, &state); + err =pwm_apply_might_sleep (vibrator->pwm, &state);if (err) {dev_err (&pdev->dev,"failed to apply initial PWM state: %d\n" , err);
@@ -174,7 +174,7 @@ static int pwm_vibrator_probe(struct platform_device *pdev)/* Sync up PWM state and ensure it is off. */ pwm_init_state (vibrator->pwm_dir, &state); state.enabled = false; - err =pwm_apply_state (vibrator->pwm_dir, &state); + err =pwm_apply_might_sleep (vibrator->pwm_dir, &state);if (err) {dev_err (&pdev->dev,"failed to apply initial PWM state: %d\n" , err);
drivers/leds/leds-pwm.c +1 -1
@@ -54,7 +54,7 @@ static int led_pwm_set(struct led_classdev *led_cdev, led_dat->pwmstate.duty_cycle = duty; led_dat->pwmstate.enabled = true; -return pwm_apply_state (led_dat->pwm, &led_dat->pwmstate); +return pwm_apply_might_sleep (led_dat->pwm, &led_dat->pwmstate); } __attribute__((nonnull))
drivers/leds/rgb/leds-pwm-multicolor.c +2 -2
@@ -51,8 +51,8 @@ static int led_pwm_mc_set(struct led_classdev *cdev, priv->leds[i].state.duty_cycle = duty; priv->leds[i].state.enabled = duty >0 ; - ret =pwm_apply_state (priv->leds[i].pwm, - &priv->leds[i].state); + ret =pwm_apply_might_sleep (priv->leds[i].pwm, + &priv->leds[i].state);if (ret)break ; }
drivers/media/rc/pwm-ir-tx.c +2 -2
@@ -68,7 +68,7 @@ static int pwm_ir_tx(struct rc_dev *dev, unsigned int *txbuf,for (i =0 ; i < count; i++) { state.enabled = !(i %2 ); -pwm_apply_state (pwm, &state); +pwm_apply_might_sleep (pwm, &state); edge =ktime_add_us (edge, txbuf[i]); delta =ktime_us_delta (edge,ktime_get ());
@@ -77,7 +77,7 @@ static int pwm_ir_tx(struct rc_dev *dev, unsigned int *txbuf, } state.enabled = false; -pwm_apply_state (pwm, &state); +pwm_apply_might_sleep (pwm, &state);return count; }
drivers/platform/x86/lenovo-yogabook.c +1 -1
@@ -435,7 +435,7 @@ static int yogabook_pdev_set_kbd_backlight(struct yogabook_data *data, u8 level) .enabled = level, }; -pwm_apply_state (data->kbd_bl_pwm, &state); +pwm_apply_might_sleep (data->kbd_bl_pwm, &state);gpiod_set_value (data->kbd_bl_led_enable, level ?1 :0 );return 0 ; }
drivers/pwm/core.c +9 -9
@@ -356,8 +356,8 @@ struct pwm_device *pwm_request_from_chip(struct pwm_chip *chip, }EXPORT_SYMBOL_GPL (pwm_request_from_chip); -static void pwm_apply_state_debug (struct pwm_device *pwm, -const struct pwm_state *state) +static void pwm_apply_debug (struct pwm_device *pwm, +const struct pwm_state *state) {struct pwm_state *last = &pwm->last;struct pwm_chip *chip = pwm->chip;
@@ -463,11 +463,11 @@ static void pwm_apply_state_debug(struct pwm_device *pwm, }/** -* pwm_apply_state() - atomically apply a new state to a PWM device +* pwm_apply_might_sleep() - atomically apply a new state to a PWM device * @pwm: PWM device * @state: new state to apply */ -int pwm_apply_state (struct pwm_device *pwm,const struct pwm_state *state) +int pwm_apply_might_sleep (struct pwm_device *pwm,const struct pwm_state *state) {struct pwm_chip *chip;int err;
@@ -475,7 +475,7 @@ int pwm_apply_state(struct pwm_device *pwm, const struct pwm_state *state)/* * Some lowlevel driver's implementations of .apply() make use of * mutexes, also with some drivers only returning when the new -* configuration is active calling pwm_apply_state() from atomic context +* configuration is active calling pwm_apply_might_sleep() from atomic context * is a bad idea. So make it explicit that calling this function might * sleep. */
@@ -505,11 +505,11 @@ int pwm_apply_state(struct pwm_device *pwm, const struct pwm_state *state)* only do this after pwm->state was applied as some * implementations of .get_state depend on this */ -pwm_apply_state_debug (pwm, state); +pwm_apply_debug (pwm, state);return 0 ; } -EXPORT_SYMBOL_GPL (pwm_apply_state); +EXPORT_SYMBOL_GPL (pwm_apply_might_sleep);/** * pwm_capture() - capture and report a PWM signal
@@ -567,7 +567,7 @@ int pwm_adjust_config(struct pwm_device *pwm) state.period = pargs.period; state.polarity = pargs.polarity; -return pwm_apply_state (pwm, &state); +return pwm_apply_might_sleep (pwm, &state); }/*
@@ -590,7 +590,7 @@ int pwm_adjust_config(struct pwm_device *pwm) state.duty_cycle = state.period - state.duty_cycle; } -return pwm_apply_state (pwm, &state); +return pwm_apply_might_sleep (pwm, &state); }EXPORT_SYMBOL_GPL (pwm_adjust_config);
drivers/pwm/pwm-twl-led.c +1 -1
@@ -172,7 +172,7 @@ static int twl4030_pwmled_apply(struct pwm_chip *chip, struct pwm_device *pwm,* We cannot skip calling ->config even if state->period == * pwm->state.period && state->duty_cycle == pwm->state.duty_cycle * because we might have exited early in the last call to -* pwm_apply_state because of !state->enabled and so the two values in +* pwm_apply_might_sleep because of !state->enabled and so the two values in * pwm->state might not be configured in hardware. */ ret = twl4030_pwmled_config(pwm->chip, pwm,
drivers/pwm/pwm-vt8500.c +1 -1
@@ -206,7 +206,7 @@ static int vt8500_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,* We cannot skip calling ->config even if state->period == * pwm->state.period && state->duty_cycle == pwm->state.duty_cycle * because we might have exited early in the last call to -* pwm_apply_state because of !state->enabled and so the two values in +* pwm_apply_might_sleep because of !state->enabled and so the two values in * pwm->state might not be configured in hardware. */ err =vt8500_pwm_config (pwm->chip, pwm, state->duty_cycle, state->period);
drivers/pwm/sysfs.c +5 -5
@@ -62,7 +62,7 @@ static ssize_t period_store(struct device *child,mutex_lock (&export->lock);pwm_get_state (pwm, &state); state.period = val; - ret =pwm_apply_state (pwm, &state); + ret =pwm_apply_might_sleep (pwm, &state);mutex_unlock (&export->lock);return ret ? : size;
@@ -97,7 +97,7 @@ static ssize_t duty_cycle_store(struct device *child,mutex_lock (&export->lock);pwm_get_state (pwm, &state); state.duty_cycle = val; - ret =pwm_apply_state (pwm, &state); + ret =pwm_apply_might_sleep (pwm, &state);mutex_unlock (&export->lock);return ret ? : size;
@@ -144,7 +144,7 @@ static ssize_t enable_store(struct device *child, goto unlock; } - ret =pwm_apply_state (pwm, &state); + ret =pwm_apply_might_sleep (pwm, &state); unlock:mutex_unlock (&export->lock);
@@ -194,7 +194,7 @@ static ssize_t polarity_store(struct device *child,mutex_lock (&export->lock);pwm_get_state (pwm, &state); state.polarity = polarity; - ret =pwm_apply_state (pwm, &state); + ret =pwm_apply_might_sleep (pwm, &state);mutex_unlock (&export->lock);return ret ? : size;
@@ -401,7 +401,7 @@ static int pwm_class_apply_state(struct pwm_export *export,struct pwm_device *pwm, struct pwm_state *state) { - int ret = pwm_apply_state(pwm, state); + int ret = pwm_apply_might_sleep(pwm, state);/* release lock taken in pwm_class_get_state */ mutex_unlock (&export->lock);
drivers/regulator/pwm-regulator.c +2 -2
@@ -90,7 +90,7 @@ static int pwm_regulator_set_voltage_sel(struct regulator_dev *rdev,pwm_set_relative_duty_cycle (&pstate, drvdata->duty_cycle_table[selector].dutycycle,100 ); - ret =pwm_apply_state (drvdata->pwm, &pstate); + ret =pwm_apply_might_sleep (drvdata->pwm, &pstate);if (ret) {dev_err (&rdev->dev,"Failed to configure PWM: %d\n" , ret);return ret;
@@ -216,7 +216,7 @@ static int pwm_regulator_set_voltage(struct regulator_dev *rdev,pwm_set_relative_duty_cycle (&pstate, dutycycle, duty_unit); - ret =pwm_apply_state (drvdata->pwm, &pstate); + ret =pwm_apply_might_sleep (drvdata->pwm, &pstate);if (ret) {dev_err (&rdev->dev,"Failed to configure PWM: %d\n" , ret);return ret;
drivers/video/backlight/lm3630a_bl.c +1 -1
@@ -180,7 +180,7 @@ static int lm3630a_pwm_ctrl(struct lm3630a_chip *pchip, int br, int br_max) pchip->pwmd_state.enabled = pchip->pwmd_state.duty_cycle ? true : false; -return pwm_apply_state (pchip->pwmd, &pchip->pwmd_state); +return pwm_apply_might_sleep (pchip->pwmd, &pchip->pwmd_state); }/* update and get brightness */
drivers/video/backlight/lp855x_bl.c +1 -1
@@ -234,7 +234,7 @@ static int lp855x_pwm_ctrl(struct lp855x *lp, int br, int max_br) state.duty_cycle =div_u64 (br * state.period, max_br); state.enabled = state.duty_cycle; -return pwm_apply_state (lp->pwm, &state); +return pwm_apply_might_sleep (lp->pwm, &state); }static int lp855x_bl_update_status (struct backlight_device *bl)
drivers/video/backlight/pwm_bl.c +6 -6
@@ -103,7 +103,7 @@ static int pwm_backlight_update_status(struct backlight_device *bl)pwm_get_state (pb->pwm, &state); state.duty_cycle =compute_duty_cycle (pb, brightness, &state); state.enabled = true; -pwm_apply_state (pb->pwm, &state); +pwm_apply_might_sleep (pb->pwm, &state);pwm_backlight_power_on (pb); }else {
@@ -120,7 +120,7 @@ static int pwm_backlight_update_status(struct backlight_device *bl)* inactive output. */ state.enabled = !pb->power_supply && !pb->enable_gpio; -pwm_apply_state (pb->pwm, &state); +pwm_apply_might_sleep (pb->pwm, &state); }if (pb->notify_after)
@@ -528,7 +528,7 @@ static int pwm_backlight_probe(struct platform_device *pdev)if (!state.period && (data->pwm_period_ns >0 )) state.period = data->pwm_period_ns; - ret =pwm_apply_state (pb->pwm, &state); + ret =pwm_apply_might_sleep (pb->pwm, &state);if (ret) {dev_err (&pdev->dev,"failed to apply initial PWM state: %d\n" , ret);
@@ -633,7 +633,7 @@ static void pwm_backlight_remove(struct platform_device *pdev)pwm_get_state (pb->pwm, &state); state.duty_cycle =0 ; state.enabled = false; -pwm_apply_state (pb->pwm, &state); +pwm_apply_might_sleep (pb->pwm, &state);if (pb->exit) pb->exit (&pdev->dev);
@@ -649,7 +649,7 @@ static void pwm_backlight_shutdown(struct platform_device *pdev)pwm_get_state (pb->pwm, &state); state.duty_cycle =0 ; state.enabled = false; -pwm_apply_state (pb->pwm, &state); +pwm_apply_might_sleep (pb->pwm, &state); }#ifdef CONFIG_PM_SLEEP
@@ -673,7 +673,7 @@ static int pwm_backlight_suspend(struct device *dev)pwm_get_state (pb->pwm, &state); state.duty_cycle =0 ; state.enabled = false; -pwm_apply_state (pb->pwm, &state); +pwm_apply_might_sleep (pb->pwm, &state);if (pb->notify_after) pb->notify_after (pb->dev,0 );
drivers/video/fbdev/ssd1307fb.c +1 -1
@@ -347,7 +347,7 @@ static int ssd1307fb_init(struct ssd1307fb_par *par)pwm_init_state (par->pwm, &pwmstate);pwm_set_relative_duty_cycle (&pwmstate,50 ,100 ); -pwm_apply_state (par->pwm, &pwmstate); +pwm_apply_might_sleep (par->pwm, &pwmstate);/* Enable the PWM */ pwm_enable (par->pwm);
include/linux/pwm.h +14 -14
@@ -93,8 +93,8 @@ struct pwm_device {* @state: state to fill with the current PWM state * * The returned PWM state represents the state that was applied by a previous call to -* pwm_apply_state(). Drivers may have to slightly tweak that state before programming it to -* hardware. If pwm_apply_state() was never called, this returns either the current hardware +* pwm_apply_might_sleep(). Drivers may have to slightly tweak that state before programming it to +* hardware. If pwm_apply_might_sleep() was never called, this returns either the current hardware * state (if supported) or the default settings. */ static inline void pwm_get_state(const struct pwm_device *pwm,
@@ -158,20 +158,20 @@ static inline void pwm_get_args(const struct pwm_device *pwm, }/** -* pwm_init_state() - prepare a new state to be applied with pwm_apply_state() +* pwm_init_state() - prepare a new state to be applied with pwm_apply_might_sleep() * @pwm: PWM device * @state: state to fill with the prepared PWM state * * This functions prepares a state that can later be tweaked and applied -* to the PWM device with pwm_apply_state(). This is a convenient function +* to the PWM device with pwm_apply_might_sleep(). This is a convenient function * that first retrieves the current PWM state and the replaces the period * and polarity fields with the reference values defined in pwm->args. * Once the function returns, you can adjust the ->enabled and ->duty_cycle -* fields according to your needs before calling pwm_apply_state(). +* fields according to your needs before calling pwm_apply_might_sleep(). * * ->duty_cycle is initially set to zero to avoid cases where the current * ->duty_cycle value exceed the pwm_args->period one, which would trigger -* an error if the user calls pwm_apply_state() without adjusting ->duty_cycle +* an error if the user calls pwm_apply_might_sleep() without adjusting ->duty_cycle * first. */ static inline void pwm_init_state(const struct pwm_device *pwm,
@@ -227,7 +227,7 @@ pwm_get_relative_duty_cycle(const struct pwm_state *state, unsigned int scale)* * pwm_init_state(pwm, &state); * pwm_set_relative_duty_cycle(&state, 50, 100); -* pwm_apply_state(pwm, &state); +* pwm_apply_might_sleep(pwm, &state); * * This functions returns -EINVAL if @duty_cycle and/or @scale are * inconsistent (@scale == 0 or @duty_cycle > @scale).
@@ -307,7 +307,7 @@ struct pwm_chip {#if IS_ENABLED (CONFIG_PWM )/* PWM user APIs */ -int pwm_apply_state (struct pwm_device *pwm,const struct pwm_state *state); +int pwm_apply_might_sleep (struct pwm_device *pwm,const struct pwm_state *state);int pwm_adjust_config (struct pwm_device *pwm);/**
@@ -335,7 +335,7 @@ static inline int pwm_config(struct pwm_device *pwm, int duty_ns, state.duty_cycle = duty_ns; state.period = period_ns; -return pwm_apply_state (pwm, &state); +return pwm_apply_might_sleep (pwm, &state); }/**
@@ -356,7 +356,7 @@ static inline int pwm_enable(struct pwm_device *pwm)return 0 ; state.enabled = true; -return pwm_apply_state (pwm, &state); +return pwm_apply_might_sleep (pwm, &state); }/**
@@ -375,7 +375,7 @@ static inline void pwm_disable(struct pwm_device *pwm)return ; state.enabled = false; -pwm_apply_state (pwm, &state); +pwm_apply_might_sleep (pwm, &state); }/* PWM provider APIs */
@@ -406,8 +406,8 @@ struct pwm_device *devm_fwnode_pwm_get(struct device *dev,struct fwnode_handle *fwnode, const char *con_id);#else -static inline int pwm_apply_state (struct pwm_device *pwm, -const struct pwm_state *state) +static inline int pwm_apply_might_sleep (struct pwm_device *pwm, +const struct pwm_state *state) {might_sleep ();return -ENOTSUPP ;
@@ -524,7 +524,7 @@ static inline void pwm_apply_args(struct pwm_device *pwm) state.period = pwm->args.period; state.usage_power = false; -pwm_apply_state (pwm, &state); +pwm_apply_might_sleep (pwm, &state); }struct pwm_lookup {
Signature · 1 line
2.43.0
Re: [PATCH v6 1/4] pwm: rename pwm_apply_state() to pwm_apply_might_sleep()
Quoted 6 lines · Sean Young
In order to introduce a pwm api which can be used from atomic context, we will need two functions for applying pwm changes:
int pwm_apply_might_sleep(struct pwm *, struct pwm_state *); int pwm_apply_atomic(struct pwm *, struct pwm_state *);
Quoted 7 lines
This commit just deals with renaming
pwm_apply_state(), a following commit will introduce thepwm_apply_atomic()function.Acked-by Hans de Goede <[email protected]>Acked-by Jani Nikula <[email protected]>Acked-by Lee Jones <[email protected]>Signed-off-by Sean Young <[email protected]>
Not a in-detail-review, but I just noticed again, that we have
K: pwm_(config|apply_state|ops)
in MAINTAINERS. That one needs adaption, too.
Best regards
Uwe
Signature · 2 lines
Pengutronix e.K. | Uwe Kleine-König | Industrial Linux Solutions | https://www.pengutronix.de/ |
Sean Young
maintainerRe: [PATCH v6 1/4] pwm: rename pwm_apply_state() to pwm_apply_might_sleep()
Quoted 15 lines · Uwe Kleine-König
Quoted 6 lines · Sean Young
In order to introduce a pwm api which can be used from atomic context, we will need two functions for applying pwm changes:
int pwm_apply_might_sleep(struct pwm *, struct pwm_state *); int pwm_apply_atomic(struct pwm *, struct pwm_state *);Quoted 7 lines
This commit just deals with renaming
pwm_apply_state(), a following commit will introduce thepwm_apply_atomic()function.Acked-by Hans de Goede <[email protected]>Acked-by Jani Nikula <[email protected]>Acked-by Lee Jones <[email protected]>Signed-off-by Sean Young <[email protected]>
Quoted 5 lines
Not a in-detail-review, but I just noticed again, that we have
K:
pwm_(config|apply_state|ops)in
MAINTAINERS. That one needs adaption, too.
Fixed in v7.
Thanks,
Sean
Re: [PATCH v6 1/4] pwm: rename pwm_apply_state() to pwm_apply_might_sleep()
In reply to Sean YoungQuoted 3 lines · Sean Young
drivers/input/misc/da7280.c | 4 +-- drivers/input/misc/pwm-beeper.c | 4 +-- drivers/input/misc/pwm-vibra.c | 8 +++---
Thanks.
Signature · 1 line
Dmitry
Sean Young
maintainerPATCH v6 2/4 pwm: make it possible to apply pwm changes in atomic context
In reply to Sean YoungSome pwm devices require sleeping, for example if the pwm device is connected over i2c. However, many pwm devices could be used from atomic context, e.g. memmory mapped pwm. This is useful for, for example, the pwm-ir-tx driver which requires precise timing. Sleeping causes havoc with the generated IR signal.
Since not all pmw devices can support atomic context, we also add a pwm_is_atomic() function to check if it is supported.
--- Documentation/driver-api/pwm.rst | 9 +++++ drivers/pwm/core.c | 63 ++++++++++++++++++++++++++------ drivers/pwm/pwm-renesas-tpu.c | 1 - include/linux/pwm.h | 29 ++++++++++++++- 4 files changed, 87 insertions(+), 15 deletions(-)
Documentation/driver-api/pwm.rst +9 -0
@@ -43,6 +43,15 @@ After being requested, a PWM has to be configured using:: int pwm_apply_might_sleep(struct pwm_device *pwm, struct pwm_state *state); +Some PWM devices can be used from atomic context. You can check if this is +supported with:: + + bool pwm_is_atomic(struct pwm_device *pwm); + +If true, the PWM can be configured from atomic context with:: + + int pwm_apply_atomic(struct pwm_device *pwm, struct pwm_state *state); + This API controls both the PWM period/duty_cycle config and the enable/disable state.
drivers/pwm/core.c +51 -12
91 lines not on this page · raw message
drivers/pwm/pwm-renesas-tpu.c +0 -1
@@ -11,7 +11,6 @@#include <linux/init.h> #include <linux/ioport.h> #include <linux/module.h> -#include <linux/mutex.h> #include <linux/of.h> #include <linux/platform_device.h> #include <linux/pm_runtime.h>
include/linux/pwm.h +27 -2
@@ -286,6 +286,7 @@ struct pwm_ops {* @npwm: number of PWMs controlled by this chip * @of_xlate: request a PWM device given a device tree PWM specifier * @of_pwm_n_cells: number of cells expected in the device tree PWM specifier +* @atomic: can the driver call pwm_apply_atomic in atomic context * @list: list node for internal use * @pwms: array of PWM devices allocated by the framework */
@@ -299,6 +300,7 @@ struct pwm_chip {struct pwm_device * (*of_xlate)(struct pwm_chip *chip,const struct of_phandle_args *args);unsigned int of_pwm_n_cells; +bool atomic;/* only used internally by the PWM framework */ struct list_head list;
@@ -308,6 +310,7 @@ struct pwm_chip {#if IS_ENABLED (CONFIG_PWM )/* PWM user APIs */ int pwm_apply_might_sleep (struct pwm_device *pwm,const struct pwm_state *state); +int pwm_apply_atomic (struct pwm_device *pwm,const struct pwm_state *state);int pwm_adjust_config (struct pwm_device *pwm);/**
@@ -378,6 +381,17 @@ static inline void pwm_disable(struct pwm_device *pwm)pwm_apply_might_sleep (pwm, &state); } +/** +* pwm_is_atomic() - is pwm_apply_atomic() supported? +* @pwm: PWM device +* +* Returns: true pwm_apply_atomic() can be called from atomic context. +*/ +static inline bool pwm_is_atomic (struct pwm_device *pwm) +{ +return pwm->chip->atomic; +} +/* PWM provider APIs */ int pwm_capture (struct pwm_device *pwm,struct pwm_capture *result,unsigned long timeout);
@@ -406,16 +420,27 @@ struct pwm_device *devm_fwnode_pwm_get(struct device *dev,struct fwnode_handle *fwnode, const char *con_id);#else +static inline bool pwm_is_atomic (struct pwm_device *pwm) +{ +return false; +} +static inline int pwm_apply_might_sleep (struct pwm_device *pwm,const struct pwm_state *state) {might_sleep (); -return -ENOTSUPP ; +return -EOPNOTSUPP ; +} + +static inline int pwm_apply_atomic (struct pwm_device *pwm, +const struct pwm_state *state) +{ +return -EOPNOTSUPP ; }static inline int pwm_adjust_config (struct pwm_device *pwm) { -return -ENOTSUPP ; +return -EOPNOTSUPP ; } static inline int pwm_config(struct pwm_device *pwm, int duty_ns,
Signature · 1 line
2.43.0
Re: [PATCH v6 2/4] pwm: make it possible to apply pwm changes in atomic context
Quoted 8 lines · Sean Young
Some pwm devices require sleeping, for example if the pwm device is connected over i2c. However, many pwm devices could be used from atomic context, e.g. memmory mapped pwm. This is useful for, for example, the pwm-ir-tx driver which requires precise timing. Sleeping causes havoc with the generated IR signal.
Since not all pmw devices can support atomic context, we also add a
pwm_is_atomic()function to check if it is supported.
s/i2c/I2C/ and s/pwm/PWM/ in the above. Also, s/memmory/memory/
Quoted 14 lines
Signed-off-by Sean Young <[email protected]>--- Documentation/driver-api/pwm.rst | 9 +++++ drivers/pwm/core.c | 63 ++++++++++++++++++++++++++------ drivers/pwm/pwm-renesas-tpu.c | 1 - include/linux/pwm.h | 29 ++++++++++++++- 4 files changed, 87 insertions(+), 15 deletions(-)Documentation/driver-api/pwm.rst +0
-0
Documentation/driver-api/pwm.rst After being requested, a PWM has to be configured using:: · line 49
Quoted 7 lines
Documentation/driver-api/pwm.rst +4
-0@@ -43,6 +43,15 @@ After being requested, a PWM has to be configured using:: int pwm_apply_might_sleep(struct pwm_device *pwm, struct pwm_state *state); +Some PWM devices can be used from atomic context. You can check if this is +supported with:: + + bool pwm_is_atomic(struct pwm_device *pwm);
This is now going to look a bit odd. I think it'd be best to change this to pwm_might_sleep() for consistency with the pwm_apply_might_sleep() function. Fine to keep the actual member variable as atomic, though, so we don't have to change the default for all drivers.
Quoted 58 lines
Documentation/driver-api/pwm.rst +5
-0+ +If true, the PWM can be configured from atomic context with:: + + int pwm_apply_atomic(struct pwm_device *pwm, struct pwm_state *state); + This API controls both the PWM period/duty_cycle config and the enable/disable state.drivers/pwm/core.c +17
-11@@ -463,24 +463,15 @@ static void pwm_apply_debug(struct pwm_device *pwm, }/** -* pwm_apply_might_sleep() - atomically apply a new state to a PWM device +* pwm_apply_unchecked() - atomically apply a new state to a PWM device * @pwm: PWM device * @state: new state to apply */ -int pwm_apply_might_sleep (struct pwm_device *pwm,const struct pwm_state *state) +static int pwm_apply_unchecked (struct pwm_device *pwm,const struct pwm_state *state) {struct pwm_chip *chip;int err; -/* -* Some lowlevel driver's implementations of .apply() make use of -* mutexes, also with some drivers only returning when the new -* configuration is active calling pwm_apply_might_sleep() from atomic context -* is a bad idea. So make it explicit that calling this function might -* sleep. -*/ -might_sleep (); -if (!pwm || !state || !state->period || state->duty_cycle > state->period)return -EINVAL ;@@ -501,16 +492,64 @@ int pwm_apply_might_sleep(struct pwm_device *pwm, const struct pwm_state *state) pwm->state = *state; +return 0 ; +} + +/** +* pwm_apply_might_sleep() - atomically apply a new state to a PWM device +* Cannot be used in atomic context. +* @pwm: PWM device +* @state: new state to apply +*/ +int pwm_apply_might_sleep (struct pwm_device *pwm,const struct pwm_state *state) +{ +int err; + +/* +* Some lowlevel driver's implementations of .apply() make use of
drivers/pwm/core.c int pwm_apply_might_sleep(struct pwm_device *pwm, const struct pwm_state *state) · line 519
Quoted 10 lines
drivers/pwm/core.c +10
-0+* mutexes, also with some drivers only returning when the new +* configuration is active calling pwm_apply_might_sleep() from atomic context +* is a bad idea. So make it explicit that calling this function might +* sleep. +*/ +might_sleep (); + +if (IS_ENABLED (CONFIG_PWM_DEBUG ) && pwm->chip->atomic) { +/* +* Catch any sleeping drivers when atomic is set.
I think this is slightly confusing. What we're really trying to catch here is drivers that are marked as "atomic" but which still end up sleeping during ->apply(), right? So maybe something like this would be a bit more explicit:
/* * Catch any drivers that have been marked as atomic but * that will sleep anyway. */
drivers/pwm/core.c int pwm_apply_might_sleep(struct pwm_device *pwm, const struct pwm_state *state) · line 540
Quoted 3 lines
drivers/pwm/core.c +3
-0+/** +* pwm_apply_atomic() - apply a new state to a PWM device from atomic context +* Not all pwm devices support this function, check with pwm_is_atomic().
s/pwm/PWM/ here...
drivers/pwm/core.c int pwm_apply_might_sleep(struct pwm_device *pwm, const struct pwm_state *state) · line 547
Quoted 7 lines
drivers/pwm/core.c +7
-0+* @pwm: PWM device +* @state: new state to apply +*/ +int pwm_apply_atomic (struct pwm_device *pwm,const struct pwm_state *state) +{ +WARN_ONCE (!pwm->chip->atomic, +"sleeping pwm driver used in atomic context" );
... and in the warning message as well.
Quoted 25 lines
drivers/pwm/core.c +5
-0+ +return pwm_apply_unchecked (pwm, state); +} +EXPORT_SYMBOL_GPL (pwm_apply_atomic); +/** * pwm_capture() - capture and report a PWM signal * @pwm: PWM device drivers/pwm/pwm-renesas-tpu.c +0
-1@@ -11,7 +11,6 @@#include <linux/init.h> #include <linux/ioport.h> #include <linux/module.h> -#include <linux/mutex.h> #include <linux/of.h> #include <linux/platform_device.h> #include <linux/pm_runtime.h> include/linux/pwm.h +0
-0
include/linux/pwm.h struct pwm_ops { · line 289
Quoted 4 lines
include/linux/pwm.h +1
-0@@ -286,6 +286,7 @@ struct pwm_ops {* @npwm: number of PWMs controlled by this chip * @of_xlate: request a PWM device given a device tree PWM specifier * @of_pwm_n_cells: number of cells expected in the device tree PWM specifier +* @atomic: can the driver call pwm_apply_atomic in atomic context
Maybe: "can the driver's ->apply() be called in atomic context"?
Quoted 20 lines
include/linux/pwm.h +2
-0* @list: list node for internal use * @pwms: array of PWM devices allocated by the framework */ @@ -299,6 +300,7 @@ struct pwm_chip {struct pwm_device * (*of_xlate)(struct pwm_chip *chip,const struct of_phandle_args *args);unsigned int of_pwm_n_cells; +bool atomic;/* only used internally by the PWM framework */ struct list_head list;@@ -308,6 +310,7 @@ struct pwm_chip {#if IS_ENABLED (CONFIG_PWM )/* PWM user APIs */ int pwm_apply_might_sleep (struct pwm_device *pwm,const struct pwm_state *state); +int pwm_apply_atomic (struct pwm_device *pwm,const struct pwm_state *state);int pwm_adjust_config (struct pwm_device *pwm);/**
include/linux/pwm.h static inline void pwm_disable(struct pwm_device *pwm) · line 390
Quoted 10 lines
include/linux/pwm.h +7
-0@@ -378,6 +381,17 @@ static inline void pwm_disable(struct pwm_device *pwm)pwm_apply_might_sleep (pwm, &state); } +/** +* pwm_is_atomic() - is pwm_apply_atomic() supported? +* @pwm: PWM device +* +* Returns: true pwm_apply_atomic() can be called from atomic context. +*/ +static inline bool pwm_is_atomic (struct pwm_device *pwm)
As I mentioned above, I think it'd be more consistent to call this pwm_might_sleep().
Quoted 25 lines
include/linux/pwm.h +13
-1+{ +return pwm->chip->atomic; +} +/* PWM provider APIs */ int pwm_capture (struct pwm_device *pwm,struct pwm_capture *result,unsigned long timeout);@@ -406,16 +420,27 @@ struct pwm_device *devm_fwnode_pwm_get(struct device *dev,struct fwnode_handle *fwnode, const char *con_id);#else +static inline bool pwm_is_atomic (struct pwm_device *pwm) +{ +return false; +} +static inline int pwm_apply_might_sleep (struct pwm_device *pwm,const struct pwm_state *state) {might_sleep (); -return -ENOTSUPP ; +return -EOPNOTSUPP ; +} + +static inline int pwm_apply_atomic(struct pwm_device *pwm,
include/linux/pwm.h struct pwm_device *devm_fwnode_pwm_get(struct device *dev, · line 444
Quoted 10 lines
include/linux/pwm.h +4
-1+ const struct pwm_state *state) +{ +return -EOPNOTSUPP ; }static inline int pwm_adjust_config (struct pwm_device *pwm) { -return -ENOTSUPP ; +return -EOPNOTSUPP ; }
What's wrong with ENOTSUPP?
Thierry
Sean Young
maintainerRe: [PATCH v6 2/4] pwm: make it possible to apply pwm changes in atomic context
Hi Thierry,
Quoted 11 lines · Thierry Reding
Quoted 8 lines · Sean Young
Some pwm devices require sleeping, for example if the pwm device is connected over i2c. However, many pwm devices could be used from atomic context, e.g. memmory mapped pwm. This is useful for, for example, the pwm-ir-tx driver which requires precise timing. Sleeping causes havoc with the generated IR signal.
Since not all pmw devices can support atomic context, we also add a
pwm_is_atomic()function to check if it is supported.s/i2c/I2C/ and s/pwm/PWM/ in the above. Also, s/memmory/memory/
Thanks for your detailed review. I agree with all your points, they are all nice improvements. Just a question at the bottom:
Quoted 18 lines
Quoted 18 lines
Signed-off-by Sean Young <[email protected]>--- Documentation/driver-api/pwm.rst | 9 +++++ drivers/pwm/core.c | 63 ++++++++++++++++++++++++++------ drivers/pwm/pwm-renesas-tpu.c | 1 - include/linux/pwm.h | 29 ++++++++++++++- 4 files changed, 87 insertions(+), 15 deletions(-)Documentation/driver-api/pwm.rst +0
-0@@ -43,6 +43,15 @@ After being requested, a PWM has to be configured using:: int pwm_apply_might_sleep(struct pwm_device *pwm, struct pwm_state *state);
Quoted 9 lines
Documentation/driver-api/pwm.rst After being requested, a PWM has to be configured using:: · line 49
Quoted 4 lines
Documentation/driver-api/pwm.rst +4
-0+Some PWM devices can be used from atomic context. You can check if this is +supported with:: + + bool pwm_is_atomic(struct pwm_device *pwm);This is now going to look a bit odd. I think it'd be best to change this to
pwm_might_sleep()for consistency with thepwm_apply_might_sleep()function. Fine to keep the actual member variable as atomic, though, so we don't have to change the default for all drivers.
Agreed, I was struggling with finding good name and yours is much better, thanks.
Quoted 30 lines
+{
Quoted 29 lines
Documentation/driver-api/pwm.rst +15
-1+ return pwm->chip->atomic; +} + /* PWM provider APIs */ int pwm_capture(struct pwm_device *pwm, struct pwm_capture *result, unsigned long timeout);@@ -406,16 +420,27 @@ struct pwm_device *devm_fwnode_pwm_get(struct device *dev, struct fwnode_handle *fwnode, const char *con_id); #else +static inline bool pwm_is_atomic(struct pwm_device *pwm) +{ + return false; +} + static inline int pwm_apply_might_sleep(struct pwm_device *pwm, const struct pwm_state *state) { might_sleep(); - return -ENOTSUPP; + return -EOPNOTSUPP; +} + +static inline int pwm_apply_atomic(struct pwm_device *pwm, + const struct pwm_state *state) +{ + return -EOPNOTSUPP; }
Quoted 7 lines
Documentation/driver-api/pwm.rst struct pwm_device *devm_fwnode_pwm_get(struct device *dev, · line 444
Quoted 5 lines
Documentation/driver-api/pwm.rst +1
-1static inline int pwm_adjust_config(struct pwm_device *pwm) { - return -ENOTSUPP; + return -EOPNOTSUPP; }What's wrong with ENOTSUPP?
This was found by checkpatch, see
https://github.com/torvalds/linux/blob/master/scripts/checkpatch.pl#L4891-L4892
# ENOTSUPP is not a standard error code and should be avoided in new patches. # Folks usually mean EOPNOTSUPP (also called ENOTSUP), when they type ENOTSUPP.
ENOTSUPP is not really widely used in the tree.
So it was really done to keep checkpatch happy, please let me know what you would like me to do here.
Thanks,
Sean
Sean Young
maintainerPATCH v6 3/4 pwm: bcm2835: allow pwm driver to be used in atomic context
In reply to Sean Youngclk_get_rate() may do a mutex lock. Fetch the clock rate once, and prevent rate changes using clk_rate_exclusive_get().
--- drivers/pwm/pwm-bcm2835.c | 31 +++++++++++++++++++++---------- 1 file changed, 21 insertions(+), 10 deletions(-)
drivers/pwm/pwm-bcm2835.c +21 -10
@@ -28,6 +28,7 @@ struct bcm2835_pwm {struct device *dev;void __iomem *base;struct clk *clk; +unsigned long rate; };static inline struct bcm2835_pwm *to_bcm2835_pwm (struct pwm_chip *chip)
@@ -63,17 +64,11 @@ static int bcm2835_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm, {struct bcm2835_pwm *pc =to_bcm2835_pwm (chip); -unsigned long rate =clk_get_rate (pc->clk);unsigned long long period_cycles;u64 max_period;u32 val; -if (!rate) { -dev_err (pc->dev,"failed to get clock rate\n" ); -return -EINVAL ; - } -/* * period_cycles must be a 32 bit value, so period * rate / NSEC_PER_SEC * must be <= U32_MAX. As U32_MAX * NSEC_PER_SEC < U64_MAX the
@@ -88,13 +83,13 @@ static int bcm2835_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,* <=> period < ((U32_MAX * NSEC_PER_SEC + NSEC_PER_SEC/2) / rate * <=> period <= ceil((U32_MAX * NSEC_PER_SEC + NSEC_PER_SEC/2) / rate) - 1 */ - max_period =DIV_ROUND_UP_ULL ((u64 )U32_MAX *NSEC_PER_SEC +NSEC_PER_SEC /2 , rate) -1 ; + max_period =DIV_ROUND_UP_ULL ((u64 )U32_MAX *NSEC_PER_SEC +NSEC_PER_SEC /2 , pc->rate) -1 ;if (state->period > max_period)return -EINVAL ;/* set period */ - period_cycles =DIV_ROUND_CLOSEST_ULL (state->period * rate,NSEC_PER_SEC ); + period_cycles =DIV_ROUND_CLOSEST_ULL (state->period * pc->rate,NSEC_PER_SEC );/* don't accept a period that is too small */ if (period_cycles <PERIOD_MIN )
@@ -103,7 +98,7 @@ static int bcm2835_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,writel (period_cycles, pc->base +PERIOD (pwm->hwpwm));/* set duty cycle */ - val =DIV_ROUND_CLOSEST_ULL (state->duty_cycle * rate,NSEC_PER_SEC ); + val =DIV_ROUND_CLOSEST_ULL (state->duty_cycle * pc->rate,NSEC_PER_SEC );writel (val, pc->base +DUTY (pwm->hwpwm));/* set polarity */
@@ -151,14 +146,29 @@ static int bcm2835_pwm_probe(struct platform_device *pdev)return dev_err_probe (&pdev->dev,PTR_ERR (pc->clk),"clock not found\n" ); + ret =clk_rate_exclusive_get (pc->clk); +if (ret) +return dev_err_probe (&pdev->dev, ret, +"fail to get exclusive rate\n" ); + + pc->rate =clk_get_rate (pc->clk); +if (!pc->rate) { +clk_rate_exclusive_put (pc->clk); +return dev_err_probe (&pdev->dev, -EINVAL , +"failed to get clock rate\n" ); + } + pc->chip.dev = &pdev->dev; pc->chip.ops = &bcm2835_pwm_ops; + pc->chip.atomic = true; pc->chip.npwm =2 ; ret =devm_pwmchip_add (&pdev->dev, &pc->chip); -if (ret <0 ) +if (ret <0 ) { +clk_rate_exclusive_put (pc->clk);return dev_err_probe (&pdev->dev, ret,"failed to add pwmchip\n" ); + }return 0 ; }
@@ -167,6 +177,7 @@ static int bcm2835_pwm_suspend(struct device *dev) {struct bcm2835_pwm *pc =dev_get_drvdata (dev); +clk_rate_exclusive_put (pc->clk);clk_disable_unprepare (pc->clk);return 0 ;
Signature · 1 line
2.43.0
Re: [PATCH v6 3/4] pwm: bcm2835: allow pwm driver to be used in atomic context
Quoted 4 lines · Sean Young
clk_get_rate()may do a mutex lock. Fetch the clock rate once, and prevent rate changes usingclk_rate_exclusive_get().Signed-off-by Sean Young <[email protected]>
Signature · 1 line
Florian
Re: [PATCH v6 3/4] pwm: bcm2835: allow pwm driver to be used in atomic context
In reply to Sean YoungQuoted 7 lines · Sean Young
clk_get_rate()may do a mutex lock. Fetch the clock rate once, and prevent rate changes usingclk_rate_exclusive_get().Signed-off-by Sean Young <[email protected]>--- drivers/pwm/pwm-bcm2835.c | 31 +++++++++++++++++++++---------- 1 file changed, 21 insertions(+), 10 deletions(-)
s/pwm/PWM/ in the subject. Although, I guess you could just drop the "PWM" altogether because the subject prefix implies that this is for PWM. Also, please capitalize after the subject prefix.
Thierry
Sean Young
maintainerRe: [PATCH v6 3/4] pwm: bcm2835: allow pwm driver to be used in atomic context
Quoted 4 lines · Thierry Reding
Quoted 3 lines · Sean Young
clk_get_rate()may do a mutex lock. Fetch the clock rate once, and prevent rate changes usingclk_rate_exclusive_get().
Quoted 8 lines
Quoted 4 lines
Signed-off-by Sean Young <[email protected]>--- drivers/pwm/pwm-bcm2835.c | 31 +++++++++++++++++++++---------- 1 file changed, 21 insertions(+), 10 deletions(-)s/pwm/PWM/ in the subject. Although, I guess you could just drop the "PWM" altogether because the subject prefix implies that this is for PWM.
$ git log --no-merges --oneline drivers/pwm/ | sed -r 's/^\w* ([^:]+): .*/\1/' | sort | uniq -c
1197 pwm
1 PWM
...The vast majority of the commits use pwm: as a prefix, only one uses PWM:. In fact if you look across the tree almost everywhere lower case is used for the prefix.
I'm just trying to follow convention.
Having said that, I think the prefix is totally redundant, it is clear from the commit files what they are affecting. I am not sure what it really adds.
Quoted 1 line
Also, please capitalize after the subject prefix.
$ git log --no-merges --oneline drivers/pwm/ | grep -E '^\w* ([^:]+): [A-Z]' | wc -l 217 $ git log --no-merges --oneline drivers/pwm/ | grep -E '^\w* ([^:]+): [a-z]' | wc -l 1069
Although not as clear, convention seems to be lower case for commits. The first line of a commit is not really a sentence, there is no trailing period.
I am happy to oblige, just wanted to point this out. Sorry if this starts a bikeshed discussion.
Thanks,
Sean
Re: [PATCH v6 3/4] pwm: bcm2835: allow pwm driver to be used in atomic context
Quoted 14 lines · Sean Young
Quoted 4 lines · Thierry Reding
Quoted 3 lines · Sean Young
clk_get_rate()may do a mutex lock. Fetch the clock rate once, and prevent rate changes usingclk_rate_exclusive_get().Quoted 8 lines
Quoted 4 lines
Signed-off-by Sean Young <[email protected]>--- drivers/pwm/pwm-bcm2835.c | 31 +++++++++++++++++++++---------- 1 file changed, 21 insertions(+), 10 deletions(-)s/pwm/PWM/ in the subject. Although, I guess you could just drop the "PWM" altogether because the subject prefix implies that this is for PWM.
Quoted 8 lines
$ git log --no-merges --oneline drivers/pwm/ | sed -r 's/^\w* ([^:]+): .*/\1/' | sort | uniq -c 1197 pwm 1 PWM ...The vast majority of the commits use pwm: as a prefix, only one uses PWM:. In fact if you look across the tree almost everywhere lower case is used for the prefix.
Thierry doesn't want you to change the subject prefix, but only the second "pwm" to make it read:
pwm: bcm2835: allow PWM driver to be used in atomic context
While I understand Thierry here, I'm fine with a lowercase pwm here, too. In my book a PWM in all uppercase is the type of hardware and pwm in all lowercase is the framework's name. If you use "PWM driver" or "pwm driver" then doesn't matter much.
Quoted 6 lines
I'm just trying to follow convention.
Having said that, I think the prefix is totally redundant, it is clear from the commit files what they are affecting. I am not sure what it really adds.
Quoted 6 lines
Quoted 1 line
Also, please capitalize after the subject prefix.
$ git log --no-merges --oneline drivers/pwm/ | grep -E '^\w* ([^:]+): [A-Z]' | wc -l 217 $ git log --no-merges --oneline drivers/pwm/ | grep -E '^\w* ([^:]+): [a-z]' | wc -l 1069
Your matching things like:
pwm: pwm-tiehrpwm: Add support for configuring polarity of PWM
with the second command. These are perfectly fine as pwm-tiehrpwm is the driver name and so shouldn't be capitalized. With a bit more care here, we get:
$ git log --no-merges --oneline drivers/pwm/ | grep -E '^.+: [a-z][^:]*$' | wc -l 114 $ git log --no-merges --oneline drivers/pwm/ | grep -E '^.+: [A-Z][^:]*$' | wc -l 1167
And the newest of the 114 with a small letter is from 2013.
Best regards
Uwe
Signature · 2 lines
Pengutronix e.K. | Uwe Kleine-König | Industrial Linux Solutions | https://www.pengutronix.de/ |
Sean Young
maintainerRe: [PATCH v6 3/4] pwm: bcm2835: allow pwm driver to be used in atomic context
Quoted 20 lines · Uwe Kleine-König
Quoted 19 lines · Sean Young
Quoted 4 lines · Thierry Reding
Quoted 3 lines · Sean Young
clk_get_rate()may do a mutex lock. Fetch the clock rate once, and prevent rate changes usingclk_rate_exclusive_get().Quoted 8 lines
Quoted 4 lines
Signed-off-by Sean Young <[email protected]>--- drivers/pwm/pwm-bcm2835.c | 31 +++++++++++++++++++++---------- 1 file changed, 21 insertions(+), 10 deletions(-)s/pwm/PWM/ in the subject. Although, I guess you could just drop the "PWM" altogether because the subject prefix implies that this is for PWM.
$ git log --no-merges --oneline drivers/pwm/ | sed -r 's/^\w* ([^:]+): .*/\1/' | sort | uniq -c 1197 pwm 1 PWM ...
Quoted 8 lines
Quoted 3 lines
The vast majority of the commits use pwm: as a prefix, only one uses PWM:. In fact if you look across the tree almost everywhere lower case is used for the prefix.
Thierry doesn't want you to change the subject prefix, but only the second "pwm" to make it read:
pwm: bcm2835: allow PWM driver to be used in atomic context
Ah of course, my bad.
Quoted 27 lines
While I understand Thierry here, I'm fine with a lowercase pwm here, too. In my book a PWM in all uppercase is the type of hardware and pwm in all lowercase is the framework's name. If you use "PWM driver" or "pwm driver" then doesn't matter much.
Quoted 6 lines
I'm just trying to follow convention.
Having said that, I think the prefix is totally redundant, it is clear from the commit files what they are affecting. I am not sure what it really adds.
Quoted 6 lines
Quoted 1 line
Also, please capitalize after the subject prefix.
$ git log --no-merges --oneline drivers/pwm/ | grep -E '^\w* ([^:]+): [A-Z]' | wc -l 217 $ git log --no-merges --oneline drivers/pwm/ | grep -E '^\w* ([^:]+): [a-z]' | wc -l 1069Your matching things like:
pwm: pwm-tiehrpwm: Add support for configuring polarity of PWM
with the second command. These are perfectly fine as pwm-tiehrpwm is the driver name and so shouldn't be capitalized. With a bit more care here, we get:
Quoted 6 lines
$ git log --no-merges --oneline drivers/pwm/ | grep -E '^.+: [a-z][^:]*$' | wc -l 114 $ git log --no-merges --oneline drivers/pwm/ | grep -E '^.+: [A-Z][^:]*$' | wc -l 1167And the newest of the 114 with a small letter is from 2013.
Again, I stand corrected.
Thanks for pointing it out, I will fix in the next version.
Sean
Re: [PATCH v6 3/4] pwm: bcm2835: allow pwm driver to be used in atomic context
In reply to Uwe Kleine-KönigQuoted 27 lines · Uwe Kleine-König
Quoted 14 lines · Sean Young
Quoted 4 lines · Thierry Reding
Quoted 3 lines · Sean Young
clk_get_rate()may do a mutex lock. Fetch the clock rate once, and prevent rate changes usingclk_rate_exclusive_get().Quoted 8 lines
Quoted 4 lines
Signed-off-by Sean Young <[email protected]>--- drivers/pwm/pwm-bcm2835.c | 31 +++++++++++++++++++++---------- 1 file changed, 21 insertions(+), 10 deletions(-)s/pwm/PWM/ in the subject. Although, I guess you could just drop the "PWM" altogether because the subject prefix implies that this is for PWM.
Quoted 8 lines
$ git log --no-merges --oneline drivers/pwm/ | sed -r 's/^\w* ([^:]+): .*/\1/' | sort | uniq -c 1197 pwm 1 PWM ...The vast majority of the commits use pwm: as a prefix, only one uses PWM:. In fact if you look across the tree almost everywhere lower case is used for the prefix.
Thierry doesn't want you to change the subject prefix, but only the second "pwm" to make it read:
Quoted 6 lines
pwm: bcm2835: allow PWM driver to be used in atomic context
While I understand Thierry here, I'm fine with a lowercase pwm here, too. In my book a PWM in all uppercase is the type of hardware and pwm in all lowercase is the framework's name. If you use "PWM driver" or "pwm driver" then doesn't matter much.
I'm not fine with a lowercase "pwm" in what is clearly text. Text should be grammatically correct and PWM being an abbreviation it should be all caps. The framework name is also PWM, not pwm. We use the lowercase pwm as prefix because it represents the directory where the subsystem lives and we usually don't use capitalization in file and directory names.
Thierry
Sean Young
maintainerPATCH v6 4/4 media: pwm-ir-tx: trigger edges from hrtimer interrupt context
In reply to Sean YoungThis makes the generated IR much more precise. Before this change, the driver is unreliable and many users opted to use gpio-ir-tx instead.
--- drivers/media/rc/pwm-ir-tx.c | 79 ++++++++++++++++++++++++++++++++++-- 1 file changed, 76 insertions(+), 3 deletions(-)
drivers/media/rc/pwm-ir-tx.c +76 -3
@@ -10,6 +10,8 @@#include <linux/slab.h> #include <linux/of.h> #include <linux/platform_device.h> +#include <linux/hrtimer.h> +#include <linux/completion.h> #include <media/rc-core.h> #define DRIVER_NAME "pwm-ir-tx"
@@ -17,8 +19,14 @@struct pwm_ir {struct pwm_device *pwm; -unsigned int carrier; -unsigned int duty_cycle; +struct hrtimer timer; +struct completion tx_done; +struct pwm_state *state; +u32 carrier; +u32 duty_cycle; +uint *txbuf; +uint txbuf_len; +uint txbuf_index; };static const struct of_device_id pwm_ir_of_match[] = {
@@ -82,6 +90,62 @@ static int pwm_ir_tx(struct rc_dev *dev, unsigned int *txbuf,return count; } +static int pwm_ir_tx_atomic (struct rc_dev *dev,unsigned int *txbuf, +unsigned int count) +{ +struct pwm_ir *pwm_ir = dev->priv; +struct pwm_device *pwm = pwm_ir->pwm; +struct pwm_state state; + +pwm_init_state (pwm, &state); + + state.period =DIV_ROUND_CLOSEST (NSEC_PER_SEC , pwm_ir->carrier); +pwm_set_relative_duty_cycle (&state, pwm_ir->duty_cycle,100 ); + + pwm_ir->txbuf = txbuf; + pwm_ir->txbuf_len = count; + pwm_ir->txbuf_index =0 ; + pwm_ir->state = &state; + +hrtimer_start (&pwm_ir->timer,0 ,HRTIMER_MODE_REL ); + +wait_for_completion (&pwm_ir->tx_done); + +return count; +} + +static enum hrtimer_restart pwm_ir_timer (struct hrtimer *timer) +{ + struct pwm_ir *pwm_ir = container_of(timer, struct pwm_ir, timer); +ktime_t now; + +/* +* If we happen to hit an odd latency spike, loop through the +* pulses until we catch up. +*/ +do { +u64 ns; + + pwm_ir->state->enabled = !(pwm_ir->txbuf_index %2 ); +pwm_apply_atomic (pwm_ir->pwm, pwm_ir->state); + +if (pwm_ir->txbuf_index >= pwm_ir->txbuf_len) { +complete (&pwm_ir->tx_done); + +return HRTIMER_NORESTART ; + } + + ns =US_TO_NS (pwm_ir->txbuf[pwm_ir->txbuf_index]); +hrtimer_add_expires_ns (timer, ns); + + pwm_ir->txbuf_index++; + + now = timer->base->get_time (); + }while (hrtimer_get_expires_tv64 (timer) < now); + +return HRTIMER_RESTART ; +} +static int pwm_ir_probe (struct platform_device *pdev) {struct pwm_ir *pwm_ir;
@@ -103,10 +167,19 @@ static int pwm_ir_probe(struct platform_device *pdev)if (!rcdev)return -ENOMEM ; +if (pwm_is_atomic (pwm_ir->pwm)) { +init_completion (&pwm_ir->tx_done); +hrtimer_init (&pwm_ir->timer,CLOCK_MONOTONIC ,HRTIMER_MODE_REL ); + pwm_ir->timer.function = pwm_ir_timer; + rcdev->tx_ir = pwm_ir_tx_atomic; + }else { +dev_info (&pdev->dev,"tx will not be accurate as pwm device does not support atomic mode" ); + rcdev->tx_ir = pwm_ir_tx; + } + rcdev->priv = pwm_ir; rcdev->driver_name =DRIVER_NAME ; rcdev->device_name =DEVICE_NAME ; - rcdev->tx_ir = pwm_ir_tx; rcdev->s_tx_duty_cycle = pwm_ir_set_duty_cycle; rcdev->s_tx_carrier = pwm_ir_set_carrier;
Signature · 1 line
2.43.0
Re: [PATCH v6 4/4] media: pwm-ir-tx: trigger edges from hrtimer interrupt context
Quoted 23 lines · Sean Young
This makes the generated IR much more precise. Before this change, the driver is unreliable and many users opted to use gpio-ir-tx instead.
Signed-off-by Sean Young <[email protected]>--- drivers/media/rc/pwm-ir-tx.c | 79 ++++++++++++++++++++++++++++++++++-- 1 file changed, 76 insertions(+), 3 deletions(-)drivers/media/rc/pwm-ir-tx.c +2
-0@@ -10,6 +10,8 @@#include <linux/slab.h> #include <linux/of.h> #include <linux/platform_device.h> +#include <linux/hrtimer.h> +#include <linux/completion.h> #include <media/rc-core.h> #define DRIVER_NAME "pwm-ir-tx"@@ -17,8 +19,14 @@
drivers/media/rc/pwm-ir-tx.c line 27
Quoted 10 lines
drivers/media/rc/pwm-ir-tx.c +6
-2struct pwm_ir {struct pwm_device *pwm; -unsigned int carrier; -unsigned int duty_cycle; +struct hrtimer timer; +struct completion tx_done; +struct pwm_state *state; +u32 carrier; +u32 duty_cycle; +uint *txbuf;
Maybe mark this as const to signal that it's not going to get modified?
drivers/media/rc/pwm-ir-tx.c line 29
Quoted 2 lines
drivers/media/rc/pwm-ir-tx.c +2
-0+uint txbuf_len; +uint txbuf_index;
uint is rather rare. Or so I thought. There seem to be quite a few occurrences throughout the kernel. I'd still prefer unsigned int over this abbreviated form, but ultimately up to you and Mauro to decide.
Quoted 67 lines
drivers/media/rc/pwm-ir-tx.c +56
-0};static const struct of_device_id pwm_ir_of_match[] = {@@ -82,6 +90,62 @@ static int pwm_ir_tx(struct rc_dev *dev, unsigned int *txbuf,return count; } +static int pwm_ir_tx_atomic (struct rc_dev *dev,unsigned int *txbuf, +unsigned int count) +{ +struct pwm_ir *pwm_ir = dev->priv; +struct pwm_device *pwm = pwm_ir->pwm; +struct pwm_state state; + +pwm_init_state (pwm, &state); + + state.period =DIV_ROUND_CLOSEST (NSEC_PER_SEC , pwm_ir->carrier); +pwm_set_relative_duty_cycle (&state, pwm_ir->duty_cycle,100 ); + + pwm_ir->txbuf = txbuf; + pwm_ir->txbuf_len = count; + pwm_ir->txbuf_index =0 ; + pwm_ir->state = &state; + +hrtimer_start (&pwm_ir->timer,0 ,HRTIMER_MODE_REL ); + +wait_for_completion (&pwm_ir->tx_done); + +return count; +} + +static enum hrtimer_restart pwm_ir_timer (struct hrtimer *timer) +{ + struct pwm_ir *pwm_ir = container_of(timer, struct pwm_ir, timer); +ktime_t now; + +/* +* If we happen to hit an odd latency spike, loop through the +* pulses until we catch up. +*/ +do { +u64 ns; + + pwm_ir->state->enabled = !(pwm_ir->txbuf_index %2 ); +pwm_apply_atomic (pwm_ir->pwm, pwm_ir->state); + +if (pwm_ir->txbuf_index >= pwm_ir->txbuf_len) { +complete (&pwm_ir->tx_done); + +return HRTIMER_NORESTART ; + } + + ns =US_TO_NS (pwm_ir->txbuf[pwm_ir->txbuf_index]); +hrtimer_add_expires_ns (timer, ns); + + pwm_ir->txbuf_index++; + + now = timer->base->get_time (); + }while (hrtimer_get_expires_tv64 (timer) < now); + +return HRTIMER_RESTART ; +} +static int pwm_ir_probe (struct platform_device *pdev) {struct pwm_ir *pwm_ir;
drivers/media/rc/pwm-ir-tx.c static int pwm_ir_probe(struct platform_device *pdev) · line 176
Quoted 10 lines
drivers/media/rc/pwm-ir-tx.c +7
-0@@ -103,10 +167,19 @@ static int pwm_ir_probe(struct platform_device *pdev)if (!rcdev)return -ENOMEM ; +if (pwm_is_atomic (pwm_ir->pwm)) { +init_completion (&pwm_ir->tx_done); +hrtimer_init (&pwm_ir->timer,CLOCK_MONOTONIC ,HRTIMER_MODE_REL ); + pwm_ir->timer.function = pwm_ir_timer; + rcdev->tx_ir = pwm_ir_tx_atomic; + }else { +dev_info (&pdev->dev,"tx will not be accurate as pwm device does not support atomic mode" );
s/tx/TX and s/pwm/PWM/? Also, I'm a bit unhappy about "atomic mode" here because the term is overloaded in PWM. If you call pwm_appy_*() then by definition it's going to be "atomic" in the "atomic state" sense. So maybe switch to something like:
"TX will not be accurate as PWM device might sleep"
?
Thierry
Sean Young
maintainerRe: [PATCH v6 4/4] media: pwm-ir-tx: trigger edges from hrtimer interrupt context
Quoted 24 lines · Thierry Reding
Quoted 23 lines · Sean Young
This makes the generated IR much more precise. Before this change, the driver is unreliable and many users opted to use gpio-ir-tx instead.
Signed-off-by Sean Young <[email protected]>--- drivers/media/rc/pwm-ir-tx.c | 79 ++++++++++++++++++++++++++++++++++-- 1 file changed, 76 insertions(+), 3 deletions(-)drivers/media/rc/pwm-ir-tx.c +2
-0@@ -10,6 +10,8 @@#include <linux/slab.h> #include <linux/of.h> #include <linux/platform_device.h> +#include <linux/hrtimer.h> +#include <linux/completion.h> #include <media/rc-core.h> #define DRIVER_NAME "pwm-ir-tx"@@ -17,8 +19,14 @@
Quoted 12 lines
drivers/media/rc/pwm-ir-tx.c line 27
Quoted 10 lines
drivers/media/rc/pwm-ir-tx.c +6
-2struct pwm_ir {struct pwm_device *pwm; -unsigned int carrier; -unsigned int duty_cycle; +struct hrtimer timer; +struct completion tx_done; +struct pwm_state *state; +u32 carrier; +u32 duty_cycle; +uint *txbuf;Maybe mark this as const to signal that it's not going to get modified?
Ah nice, I usually forget const.
Quoted 6 lines
drivers/media/rc/pwm-ir-tx.c line 29
Quoted 2 lines
drivers/media/rc/pwm-ir-tx.c +2
-0+uint txbuf_len; +uint txbuf_index;uint is rather rare. Or so I thought. There seem to be quite a few occurrences throughout the kernel. I'd still prefer unsigned int over this abbreviated form, but ultimately up to you and Mauro to decide.
Yes, unsigned int is used a lot more. Changed.
Quoted 20 lines
Quoted 4 lines
drivers/media/rc/pwm-ir-tx.c +0
-0static int pwm_ir_probe (struct platform_device *pdev) {struct pwm_ir *pwm_ir;drivers/media/rc/pwm-ir-tx.c static int pwm_ir_probe(struct platform_device *pdev) · line 176
Quoted 10 lines
drivers/media/rc/pwm-ir-tx.c +7
-0@@ -103,10 +167,19 @@ static int pwm_ir_probe(struct platform_device *pdev)if (!rcdev)return -ENOMEM ; +if (pwm_is_atomic (pwm_ir->pwm)) { +init_completion (&pwm_ir->tx_done); +hrtimer_init (&pwm_ir->timer,CLOCK_MONOTONIC ,HRTIMER_MODE_REL ); + pwm_ir->timer.function = pwm_ir_timer; + rcdev->tx_ir = pwm_ir_tx_atomic; + }else { +dev_info (&pdev->dev,"tx will not be accurate as pwm device does not support atomic mode" );s/tx/TX and s/pwm/PWM/? Also, I'm a bit unhappy about "atomic mode" here because the term is overloaded in PWM. If you call pwm_appy_*() then by definition it's going to be "atomic" in the "atomic state" sense. So maybe switch to something like:
Quoted 3 lines
"TX will not be accurate as PWM device might sleep"
?
Very nice, changed.
Thanks
Sean