The existing pxa driver and the upcoming addition of PWM support in the TI sn565dsi86 DSI/eDP bridge driver both has a single PWM channel and thereby a need for a of_xlate function with the period as its single argument.
Introduce a common helper function in the core that can be used as of_xlate by such drivers and migrate the pxa driver to use this.
Signed-off-by: Bjorn Andersson bjorn.andersson@linaro.org ---
Changes since v4: - None
drivers/pwm/core.c | 26 ++++++++++++++++++++++++++ drivers/pwm/pwm-pxa.c | 16 +--------------- include/linux/pwm.h | 2 ++ 3 files changed, 29 insertions(+), 15 deletions(-)
diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c index 4527f09a5c50..2c6b155002a2 100644 --- a/drivers/pwm/core.c +++ b/drivers/pwm/core.c @@ -152,6 +152,32 @@ of_pwm_xlate_with_flags(struct pwm_chip *pc, const struct of_phandle_args *args) } EXPORT_SYMBOL_GPL(of_pwm_xlate_with_flags);
+struct pwm_device * +of_pwm_single_xlate(struct pwm_chip *pc, const struct of_phandle_args *args) +{ + struct pwm_device *pwm; + + if (pc->of_pwm_n_cells < 1) + return ERR_PTR(-EINVAL); + + /* validate that one cell is specified, optionally with flags */ + if (args->args_count != 1 && args->args_count != 2) + return ERR_PTR(-EINVAL); + + pwm = pwm_request_from_chip(pc, 0, NULL); + if (IS_ERR(pwm)) + return pwm; + + pwm->args.period = args->args[0]; + pwm->args.polarity = PWM_POLARITY_NORMAL; + + if (args->args_count == 2 && args->args[2] & PWM_POLARITY_INVERTED) + pwm->args.polarity = PWM_POLARITY_INVERSED; + + return pwm; +} +EXPORT_SYMBOL_GPL(of_pwm_single_xlate); + static void of_pwmchip_add(struct pwm_chip *chip) { if (!chip->dev || !chip->dev->of_node) diff --git a/drivers/pwm/pwm-pxa.c b/drivers/pwm/pwm-pxa.c index a9efdcf839ae..238ec88c130b 100644 --- a/drivers/pwm/pwm-pxa.c +++ b/drivers/pwm/pwm-pxa.c @@ -148,20 +148,6 @@ static const struct platform_device_id *pxa_pwm_get_id_dt(struct device *dev) return id ? id->data : NULL; }
-static struct pwm_device * -pxa_pwm_of_xlate(struct pwm_chip *pc, const struct of_phandle_args *args) -{ - struct pwm_device *pwm; - - pwm = pwm_request_from_chip(pc, 0, NULL); - if (IS_ERR(pwm)) - return pwm; - - pwm->args.period = args->args[0]; - - return pwm; -} - static int pwm_probe(struct platform_device *pdev) { const struct platform_device_id *id = platform_get_device_id(pdev); @@ -187,7 +173,7 @@ static int pwm_probe(struct platform_device *pdev) pc->chip.npwm = (id->driver_data & HAS_SECONDARY_PWM) ? 2 : 1;
if (IS_ENABLED(CONFIG_OF)) { - pc->chip.of_xlate = pxa_pwm_of_xlate; + pc->chip.of_xlate = of_pwm_single_xlate; pc->chip.of_pwm_n_cells = 1; }
diff --git a/include/linux/pwm.h b/include/linux/pwm.h index 725c9b784e60..dd51d4931fdc 100644 --- a/include/linux/pwm.h +++ b/include/linux/pwm.h @@ -414,6 +414,8 @@ struct pwm_device *pwm_request_from_chip(struct pwm_chip *chip,
struct pwm_device *of_pwm_xlate_with_flags(struct pwm_chip *pc, const struct of_phandle_args *args); +struct pwm_device *of_pwm_single_xlate(struct pwm_chip *pc, + const struct of_phandle_args *args);
struct pwm_device *pwm_get(struct device *dev, const char *con_id); struct pwm_device *of_pwm_get(struct device *dev, struct device_node *np,
The SN65DSI86 provides the ability to supply a PWM signal on GPIO 4, with the primary purpose of controlling the backlight of the attached panel. Add an implementation that exposes this using the standard PWM framework, to allow e.g. pwm-backlight to expose this to the user.
Signed-off-by: Bjorn Andersson bjorn.andersson@linaro.org ---
Changes since v4: - Rebased the patch - Turned ti_sn65dsi86_write_u16 into using regmap bulk write - Moved pwm_refclk_freq out of the #if CONFIG_PWM, to allow removing the guard from ti_sn_bridge_set_refclk_freq() - Updates "Limitations" text - pm_runtime_put_sync() on pm_runtime_get_sync() failure - Added parenthesis around (scale + 1) in the PWM_FREQ formula and thereby redid all the math - Rewrote the comments related to the math - Rewrote the math for calculating the backlight (duty cycle) register value - Dropped some !! on already boolean state->enabled - Dropped a spurious newline - Rewrote comment in ti_sn65dsi86_probe() talking about future PWM work
drivers/gpu/drm/bridge/ti-sn65dsi86.c | 369 +++++++++++++++++++++++++- 1 file changed, 361 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c index 41d48a393e7f..857a53dc6c75 100644 --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c @@ -4,7 +4,9 @@ * datasheet: https://www.ti.com/lit/ds/symlink/sn65dsi86.pdf */
+#include <linux/atomic.h> #include <linux/auxiliary_bus.h> +#include <linux/bitfield.h> #include <linux/bits.h> #include <linux/clk.h> #include <linux/debugfs.h> @@ -15,6 +17,7 @@ #include <linux/module.h> #include <linux/of_graph.h> #include <linux/pm_runtime.h> +#include <linux/pwm.h> #include <linux/regmap.h> #include <linux/regulator/consumer.h>
@@ -91,6 +94,13 @@ #define SN_ML_TX_MODE_REG 0x96 #define ML_TX_MAIN_LINK_OFF 0 #define ML_TX_NORMAL_MODE BIT(0) +#define SN_PWM_PRE_DIV_REG 0xA0 +#define SN_BACKLIGHT_SCALE_REG 0xA1 +#define BACKLIGHT_SCALE_MAX 0xFFFF +#define SN_BACKLIGHT_REG 0xA3 +#define SN_PWM_EN_INV_REG 0xA5 +#define SN_PWM_INV_MASK BIT(0) +#define SN_PWM_EN_MASK BIT(1) #define SN_AUX_CMD_STATUS_REG 0xF4 #define AUX_IRQ_STATUS_AUX_RPLY_TOUT BIT(3) #define AUX_IRQ_STATUS_AUX_SHORT BIT(5) @@ -113,11 +123,14 @@
#define SN_LINK_TRAINING_TRIES 10
+#define SN_PWM_GPIO_IDX 3 /* 4th GPIO */ + /** * struct ti_sn65dsi86 - Platform data for ti-sn65dsi86 driver. * @bridge_aux: AUX-bus sub device for MIPI-to-eDP bridge functionality. * @gpio_aux: AUX-bus sub device for GPIO controller functionality. * @aux_aux: AUX-bus sub device for eDP AUX channel functionality. + * @pwm_aux: AUX-bus sub device for PWM controller functionality. * * @dev: Pointer to the top level (i2c) device. * @regmap: Regmap for accessing i2c. @@ -145,11 +158,17 @@ * bitmap so we can do atomic ops on it without an extra * lock so concurrent users of our 4 GPIOs don't stomp on * each other's read-modify-write. + * + * @pchip: pwm_chip if the PWM is exposed. + * @pwm_enabled: Used to track if the PWM signal is currently enabled. + * @pwm_pin_busy: Track if GPIO4 is currently requested for GPIO or PWM. + * @pwm_refclk_freq: Cache for the reference clock input to the PWM. */ struct ti_sn65dsi86 { struct auxiliary_device bridge_aux; struct auxiliary_device gpio_aux; struct auxiliary_device aux_aux; + struct auxiliary_device pwm_aux;
struct device *dev; struct regmap *regmap; @@ -172,6 +191,12 @@ struct ti_sn65dsi86 { struct gpio_chip gchip; DECLARE_BITMAP(gchip_output, SN_NUM_GPIOS); #endif +#if defined(CONFIG_PWM) + struct pwm_chip pchip; + bool pwm_enabled; + atomic_t pwm_pin_busy; +#endif + unsigned int pwm_refclk_freq; };
static const struct regmap_range ti_sn65dsi86_volatile_ranges[] = { @@ -190,11 +215,31 @@ static const struct regmap_config ti_sn65dsi86_regmap_config = { .cache_type = REGCACHE_NONE, };
+static int ti_sn65dsi86_read_u16(struct ti_sn65dsi86 *pdata, + unsigned int reg, u16 *val) +{ + unsigned int tmp; + int ret; + + ret = regmap_read(pdata->regmap, reg, &tmp); + if (ret) + return ret; + *val = tmp; + + ret = regmap_read(pdata->regmap, reg + 1, &tmp); + if (ret) + return ret; + *val |= tmp << 8; + + return 0; +} + static void ti_sn65dsi86_write_u16(struct ti_sn65dsi86 *pdata, unsigned int reg, u16 val) { - regmap_write(pdata->regmap, reg, val & 0xFF); - regmap_write(pdata->regmap, reg + 1, val >> 8); + u8 buf[2] = { val & 0xff, val >> 8 }; + + regmap_bulk_write(pdata->regmap, reg, &buf, ARRAY_SIZE(buf)); }
static u32 ti_sn_bridge_get_dsi_freq(struct ti_sn65dsi86 *pdata) @@ -253,6 +298,12 @@ static void ti_sn_bridge_set_refclk_freq(struct ti_sn65dsi86 *pdata)
regmap_update_bits(pdata->regmap, SN_DPPLL_SRC_REG, REFCLK_FREQ_MASK, REFCLK_FREQ(i)); + + /* + * The PWM refclk is based on the value written to SN_DPPLL_SRC_REG, + * regardless of its actual sourcing. + */ + pdata->pwm_refclk_freq = ti_sn_bridge_refclk_lut[i]; }
static void ti_sn65dsi86_enable_comms(struct ti_sn65dsi86 *pdata) @@ -1259,9 +1310,283 @@ static struct auxiliary_driver ti_sn_bridge_driver = { };
/* ----------------------------------------------------------------------------- - * GPIO Controller + * PWM Controller + */ +#if defined(CONFIG_PWM) +static int ti_sn_pwm_pin_request(struct ti_sn65dsi86 *pdata) +{ + return atomic_xchg(&pdata->pwm_pin_busy, 1) ? -EBUSY : 0; +} + +static void ti_sn_pwm_pin_release(struct ti_sn65dsi86 *pdata) +{ + atomic_set(&pdata->pwm_pin_busy, 0); +} + +static struct ti_sn65dsi86 *pwm_chip_to_ti_sn_bridge(struct pwm_chip *chip) +{ + return container_of(chip, struct ti_sn65dsi86, pchip); +} + +static int ti_sn_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm) +{ + struct ti_sn65dsi86 *pdata = pwm_chip_to_ti_sn_bridge(chip); + + return ti_sn_pwm_pin_request(pdata); +} + +static void ti_sn_pwm_free(struct pwm_chip *chip, struct pwm_device *pwm) +{ + struct ti_sn65dsi86 *pdata = pwm_chip_to_ti_sn_bridge(chip); + + ti_sn_pwm_pin_release(pdata); +} + +/* + * Limitations: + * - The PWM signal is not driven when the chip is powered down, or in its + * reset state and the driver does not implement the "suspend state" + * described in the documentation. In order to save power, state->enabled is + * interpreted as denoting if the signal is expected to be valid, and is used + * to determine if the chip needs to be kept powered. + * - Changing both period and duty_cycle is not done atomically, neither is the + * multi-byte register updates, so the output might briefly be undefined + * during update. */ +static int ti_sn_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm, + const struct pwm_state *state) +{ + struct ti_sn65dsi86 *pdata = pwm_chip_to_ti_sn_bridge(chip); + unsigned int pwm_en_inv; + unsigned int backlight; + unsigned int pre_div; + unsigned int scale; + u64 period_max; + u64 period; + int ret; + + if (!pdata->pwm_enabled) { + ret = pm_runtime_get_sync(pdata->dev); + if (ret < 0) { + pm_runtime_put_sync(pdata->dev); + return ret; + } + } + + if (state->enabled) { + if (!pdata->pwm_enabled) { + /* + * The chip might have been powered down while we + * didn't hold a PM runtime reference, so mux in the + * PWM function on the GPIO pin again. + */ + ret = regmap_update_bits(pdata->regmap, SN_GPIO_CTRL_REG, + SN_GPIO_MUX_MASK << (2 * SN_PWM_GPIO_IDX), + SN_GPIO_MUX_SPECIAL << (2 * SN_PWM_GPIO_IDX)); + if (ret) { + dev_err(pdata->dev, "failed to mux in PWM function\n"); + goto out; + } + } + + /* + * Per the datasheet the PWM frequency is given by: + * + * REFCLK_FREQ + * PWM_FREQ = ----------------------------------- + * PWM_PRE_DIV * BACKLIGHT_SCALE + 1 + * + * Unfortunately the math becomes overly complex unless we + * assume that this formula is missing parenthesis around + * BACKLIGHT_SCALE + 1. + * With that the formula can be written: + * + * T_pwm * REFCLK_FREQ = PWM_PRE_DIV * (BACKLIGHT_SCALE + 1) + * + * In order to keep BACKLIGHT_SCALE within its 16 bits, + * PWM_PRE_DIV must be: + * + * T_pwm * REFCLK_FREQ + * PWM_PRE_DIV >= ------------------------- + * BACKLIGHT_SCALE_MAX + 1 + * + * To simplify the search and optimize the resolution of the + * PWM, the lowest possible PWM_PRE_DIV is used. Finally the + * scale is calculated as: + * + * T_pwm * REFCLK_FREQ + * BACKLIGHT_SCALE = ---------------------- - 1 + * PWM_PRE_DIV + * + * Here T_pwm is represented in seconds, so appropriate scaling + * to nanoseconds is necessary. + */ + + /* Minimum T_pwm is (0 * 1) / REFCLK_FREQ */ + if (state->period <= NSEC_PER_SEC / pdata->pwm_refclk_freq) { + ret = -EINVAL; + goto out; + } + + /* + * Maximum T_pwm is 255 * (65535 + 1) / REFCLK_FREQ + * Limit period to this to avoid overflows + */ + period_max = div_u64((u64)NSEC_PER_SEC * 255 * (65535 + 1), pdata->pwm_refclk_freq); + if (period > period_max) + period = period_max; + else + period = state->period; + + pre_div = DIV64_U64_ROUND_UP(period * pdata->pwm_refclk_freq, + (u64)NSEC_PER_SEC * (BACKLIGHT_SCALE_MAX + 1)); + scale = div64_u64(period * pdata->pwm_refclk_freq, (u64)NSEC_PER_SEC * pre_div) - 1; + + /* + * The documentation has the duty ratio given as: + * + * duty BACKLIGHT + * ------- = --------------------- + * period BACKLIGHT_SCALE + 1 + * + * Solve for BACKLIGHT, substituting BACKLIGHT_SCALE according + * to definition above and adjusting for nanosecond + * representation of duty cycle gives us: + */ + backlight = div64_u64(state->duty_cycle * pdata->pwm_refclk_freq, (u64)NSEC_PER_SEC * pre_div); + if (backlight > scale) + backlight = scale; + + ret = regmap_write(pdata->regmap, SN_PWM_PRE_DIV_REG, pre_div); + if (ret) { + dev_err(pdata->dev, "failed to update PWM_PRE_DIV\n"); + goto out; + }
+ ti_sn65dsi86_write_u16(pdata, SN_BACKLIGHT_SCALE_REG, scale); + ti_sn65dsi86_write_u16(pdata, SN_BACKLIGHT_REG, backlight); + } + + pwm_en_inv = FIELD_PREP(SN_PWM_EN_MASK, state->enabled) | + FIELD_PREP(SN_PWM_INV_MASK, state->polarity == PWM_POLARITY_INVERSED); + ret = regmap_write(pdata->regmap, SN_PWM_EN_INV_REG, pwm_en_inv); + if (ret) { + dev_err(pdata->dev, "failed to update PWM_EN/PWM_INV\n"); + goto out; + } + + pdata->pwm_enabled = state->enabled; +out: + + if (!pdata->pwm_enabled) + pm_runtime_put_sync(pdata->dev); + + return ret; +} + +static void ti_sn_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm, + struct pwm_state *state) +{ + struct ti_sn65dsi86 *pdata = pwm_chip_to_ti_sn_bridge(chip); + unsigned int pwm_en_inv; + unsigned int pre_div; + u16 backlight; + u16 scale; + int ret; + + ret = regmap_read(pdata->regmap, SN_PWM_EN_INV_REG, &pwm_en_inv); + if (ret) + return; + + ret = ti_sn65dsi86_read_u16(pdata, SN_BACKLIGHT_SCALE_REG, &scale); + if (ret) + return; + + ret = ti_sn65dsi86_read_u16(pdata, SN_BACKLIGHT_REG, &backlight); + if (ret) + return; + + ret = regmap_read(pdata->regmap, SN_PWM_PRE_DIV_REG, &pre_div); + if (ret) + return; + + state->enabled = FIELD_GET(SN_PWM_EN_MASK, pwm_en_inv); + if (FIELD_GET(SN_PWM_INV_MASK, pwm_en_inv)) + state->polarity = PWM_POLARITY_INVERSED; + else + state->polarity = PWM_POLARITY_NORMAL; + + state->period = DIV_ROUND_UP_ULL((u64)NSEC_PER_SEC * pre_div * (scale + 1), + pdata->pwm_refclk_freq); + state->duty_cycle = DIV_ROUND_UP_ULL((u64)NSEC_PER_SEC * pre_div * backlight, + pdata->pwm_refclk_freq); +} + +static const struct pwm_ops ti_sn_pwm_ops = { + .request = ti_sn_pwm_request, + .free = ti_sn_pwm_free, + .apply = ti_sn_pwm_apply, + .get_state = ti_sn_pwm_get_state, + .owner = THIS_MODULE, +}; + +static int ti_sn_pwm_probe(struct auxiliary_device *adev, + const struct auxiliary_device_id *id) +{ + struct ti_sn65dsi86 *pdata = dev_get_drvdata(adev->dev.parent); + + pdata->pchip.dev = pdata->dev; + pdata->pchip.ops = &ti_sn_pwm_ops; + pdata->pchip.npwm = 1; + pdata->pchip.of_xlate = of_pwm_single_xlate; + pdata->pchip.of_pwm_n_cells = 1; + + return pwmchip_add(&pdata->pchip); +} + +static void ti_sn_pwm_remove(struct auxiliary_device *adev) +{ + struct ti_sn65dsi86 *pdata = dev_get_drvdata(adev->dev.parent); + + pwmchip_remove(&pdata->pchip); + + if (pdata->pwm_enabled) + pm_runtime_put_sync(pdata->dev); +} + +static const struct auxiliary_device_id ti_sn_pwm_id_table[] = { + { .name = "ti_sn65dsi86.pwm", }, + {}, +}; + +static struct auxiliary_driver ti_sn_pwm_driver = { + .name = "pwm", + .probe = ti_sn_pwm_probe, + .remove = ti_sn_pwm_remove, + .id_table = ti_sn_pwm_id_table, +}; + +static int __init ti_sn_pwm_register(void) +{ + return auxiliary_driver_register(&ti_sn_pwm_driver); +} + +static void ti_sn_pwm_unregister(void) +{ + auxiliary_driver_unregister(&ti_sn_pwm_driver); +} + +#else +static inline int ti_sn_pwm_pin_request(struct ti_sn65dsi86 *pdata) { return 0; } +static inline void ti_sn_pwm_pin_release(struct ti_sn65dsi86 *pdata) {} + +static inline int ti_sn_pwm_register(void) { return 0; } +static inline void ti_sn_pwm_unregister(void) {} +#endif + +/* ----------------------------------------------------------------------------- + * GPIO Controller + */ #if defined(CONFIG_OF_GPIO)
static int tn_sn_bridge_of_xlate(struct gpio_chip *chip, @@ -1394,10 +1719,25 @@ static int ti_sn_bridge_gpio_direction_output(struct gpio_chip *chip, return ret; }
+static int ti_sn_bridge_gpio_request(struct gpio_chip *chip, unsigned int offset) +{ + struct ti_sn65dsi86 *pdata = gpiochip_get_data(chip); + + if (offset == SN_PWM_GPIO_IDX) + return ti_sn_pwm_pin_request(pdata); + + return 0; +} + static void ti_sn_bridge_gpio_free(struct gpio_chip *chip, unsigned int offset) { + struct ti_sn65dsi86 *pdata = gpiochip_get_data(chip); + /* We won't keep pm_runtime if we're input, so switch there on free */ ti_sn_bridge_gpio_direction_input(chip, offset); + + if (offset == SN_PWM_GPIO_IDX) + ti_sn_pwm_pin_release(pdata); }
static const char * const ti_sn_bridge_gpio_names[SN_NUM_GPIOS] = { @@ -1419,6 +1759,7 @@ static int ti_sn_gpio_probe(struct auxiliary_device *adev, pdata->gchip.owner = THIS_MODULE; pdata->gchip.of_xlate = tn_sn_bridge_of_xlate; pdata->gchip.of_gpio_n_cells = 2; + pdata->gchip.request = ti_sn_bridge_gpio_request; pdata->gchip.free = ti_sn_bridge_gpio_free; pdata->gchip.get_direction = ti_sn_bridge_gpio_get_direction; pdata->gchip.direction_input = ti_sn_bridge_gpio_direction_input; @@ -1545,10 +1886,9 @@ static int ti_sn65dsi86_probe(struct i2c_client *client, * ordering. The bridge wants the panel to be there when it probes. * The panel wants its HPD GPIO (provided by sn65dsi86 on some boards) * when it probes. The panel and maybe backlight might want the DDC - * bus. Soon the PWM provided by the bridge chip will have the same - * problem. Having sub-devices allows the some sub devices to finish - * probing even if others return -EPROBE_DEFER and gets us around the - * problems. + * bus or the pwm_chip. Having sub-devices allows the some sub devices + * to finish probing even if others return -EPROBE_DEFER and gets us + * around the problems. */
if (IS_ENABLED(CONFIG_OF_GPIO)) { @@ -1557,6 +1897,12 @@ static int ti_sn65dsi86_probe(struct i2c_client *client, return ret; }
+ if (IS_ENABLED(CONFIG_PWM)) { + ret = ti_sn65dsi86_add_aux_device(pdata, &pdata->pwm_aux, "pwm"); + if (ret) + return ret; + } + /* * NOTE: At the end of the AUX channel probe we'll add the aux device * for the bridge. This is because the bridge can't be used until the @@ -1600,10 +1946,14 @@ static int __init ti_sn65dsi86_init(void) if (ret) goto err_main_was_registered;
- ret = auxiliary_driver_register(&ti_sn_aux_driver); + ret = ti_sn_pwm_register(); if (ret) goto err_gpio_was_registered;
+ ret = auxiliary_driver_register(&ti_sn_aux_driver); + if (ret) + goto err_pwm_was_registered; + ret = auxiliary_driver_register(&ti_sn_bridge_driver); if (ret) goto err_aux_was_registered; @@ -1612,6 +1962,8 @@ static int __init ti_sn65dsi86_init(void)
err_aux_was_registered: auxiliary_driver_unregister(&ti_sn_aux_driver); +err_pwm_was_registered: + ti_sn_pwm_unregister(); err_gpio_was_registered: ti_sn_gpio_unregister(); err_main_was_registered: @@ -1625,6 +1977,7 @@ static void __exit ti_sn65dsi86_exit(void) { auxiliary_driver_unregister(&ti_sn_bridge_driver); auxiliary_driver_unregister(&ti_sn_aux_driver); + ti_sn_pwm_unregister(); ti_sn_gpio_unregister(); i2c_del_driver(&ti_sn65dsi86_driver); }
Hello,
On Thu, Sep 23, 2021 at 09:12:25PM -0500, Bjorn Andersson wrote:
The SN65DSI86 provides the ability to supply a PWM signal on GPIO 4, with the primary purpose of controlling the backlight of the attached panel. Add an implementation that exposes this using the standard PWM framework, to allow e.g. pwm-backlight to expose this to the user.
Signed-off-by: Bjorn Andersson bjorn.andersson@linaro.org
Changes since v4:
- Rebased the patch
- Turned ti_sn65dsi86_write_u16 into using regmap bulk write
- Moved pwm_refclk_freq out of the #if CONFIG_PWM, to allow removing the guard from ti_sn_bridge_set_refclk_freq()
- Updates "Limitations" text
- pm_runtime_put_sync() on pm_runtime_get_sync() failure
- Added parenthesis around (scale + 1) in the PWM_FREQ formula and thereby redid all the math
- Rewrote the comments related to the math
- Rewrote the math for calculating the backlight (duty cycle) register value
- Dropped some !! on already boolean state->enabled
- Dropped a spurious newline
- Rewrote comment in ti_sn65dsi86_probe() talking about future PWM work
drivers/gpu/drm/bridge/ti-sn65dsi86.c | 369 +++++++++++++++++++++++++- 1 file changed, 361 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c index 41d48a393e7f..857a53dc6c75 100644 --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c @@ -4,7 +4,9 @@
*/
+#include <linux/atomic.h> #include <linux/auxiliary_bus.h> +#include <linux/bitfield.h> #include <linux/bits.h> #include <linux/clk.h> #include <linux/debugfs.h> @@ -15,6 +17,7 @@ #include <linux/module.h> #include <linux/of_graph.h> #include <linux/pm_runtime.h> +#include <linux/pwm.h> #include <linux/regmap.h> #include <linux/regulator/consumer.h>
@@ -91,6 +94,13 @@ #define SN_ML_TX_MODE_REG 0x96 #define ML_TX_MAIN_LINK_OFF 0 #define ML_TX_NORMAL_MODE BIT(0) +#define SN_PWM_PRE_DIV_REG 0xA0 +#define SN_BACKLIGHT_SCALE_REG 0xA1 +#define BACKLIGHT_SCALE_MAX 0xFFFF +#define SN_BACKLIGHT_REG 0xA3 +#define SN_PWM_EN_INV_REG 0xA5 +#define SN_PWM_INV_MASK BIT(0) +#define SN_PWM_EN_MASK BIT(1) #define SN_AUX_CMD_STATUS_REG 0xF4 #define AUX_IRQ_STATUS_AUX_RPLY_TOUT BIT(3) #define AUX_IRQ_STATUS_AUX_SHORT BIT(5) @@ -113,11 +123,14 @@
#define SN_LINK_TRAINING_TRIES 10
+#define SN_PWM_GPIO_IDX 3 /* 4th GPIO */
/**
- struct ti_sn65dsi86 - Platform data for ti-sn65dsi86 driver.
- @bridge_aux: AUX-bus sub device for MIPI-to-eDP bridge functionality.
- @gpio_aux: AUX-bus sub device for GPIO controller functionality.
- @aux_aux: AUX-bus sub device for eDP AUX channel functionality.
- @pwm_aux: AUX-bus sub device for PWM controller functionality.
- @dev: Pointer to the top level (i2c) device.
- @regmap: Regmap for accessing i2c.
@@ -145,11 +158,17 @@
bitmap so we can do atomic ops on it without an extralock so concurrent users of our 4 GPIOs don't stomp oneach other's read-modify-write.
- @pchip: pwm_chip if the PWM is exposed.
- @pwm_enabled: Used to track if the PWM signal is currently enabled.
- @pwm_pin_busy: Track if GPIO4 is currently requested for GPIO or PWM.
*/
- @pwm_refclk_freq: Cache for the reference clock input to the PWM.
struct ti_sn65dsi86 { struct auxiliary_device bridge_aux; struct auxiliary_device gpio_aux; struct auxiliary_device aux_aux;
struct auxiliary_device pwm_aux;
struct device *dev; struct regmap *regmap;
@@ -172,6 +191,12 @@ struct ti_sn65dsi86 { struct gpio_chip gchip; DECLARE_BITMAP(gchip_output, SN_NUM_GPIOS); #endif +#if defined(CONFIG_PWM)
- struct pwm_chip pchip;
- bool pwm_enabled;
- atomic_t pwm_pin_busy;
+#endif
- unsigned int pwm_refclk_freq;
};
static const struct regmap_range ti_sn65dsi86_volatile_ranges[] = { @@ -190,11 +215,31 @@ static const struct regmap_config ti_sn65dsi86_regmap_config = { .cache_type = REGCACHE_NONE, };
+static int ti_sn65dsi86_read_u16(struct ti_sn65dsi86 *pdata,
unsigned int reg, u16 *val)+{
- unsigned int tmp;
- int ret;
- ret = regmap_read(pdata->regmap, reg, &tmp);
- if (ret)
return ret;- *val = tmp;
- ret = regmap_read(pdata->regmap, reg + 1, &tmp);
- if (ret)
return ret;- *val |= tmp << 8;
- return 0;
+}
static void ti_sn65dsi86_write_u16(struct ti_sn65dsi86 *pdata, unsigned int reg, u16 val) {
- regmap_write(pdata->regmap, reg, val & 0xFF);
- regmap_write(pdata->regmap, reg + 1, val >> 8);
- u8 buf[2] = { val & 0xff, val >> 8 };
- regmap_bulk_write(pdata->regmap, reg, &buf, ARRAY_SIZE(buf));
Given that ti_sn65dsi86_write_u16 uses regmap_bulk_write I wonder why ti_sn65dsi86_read_u16 doesn't use regmap_bulk_read.
}
static u32 ti_sn_bridge_get_dsi_freq(struct ti_sn65dsi86 *pdata) @@ -253,6 +298,12 @@ static void ti_sn_bridge_set_refclk_freq(struct ti_sn65dsi86 *pdata)
regmap_update_bits(pdata->regmap, SN_DPPLL_SRC_REG, REFCLK_FREQ_MASK, REFCLK_FREQ(i));
- /*
* The PWM refclk is based on the value written to SN_DPPLL_SRC_REG,* regardless of its actual sourcing.*/- pdata->pwm_refclk_freq = ti_sn_bridge_refclk_lut[i];
}
static void ti_sn65dsi86_enable_comms(struct ti_sn65dsi86 *pdata) @@ -1259,9 +1310,283 @@ static struct auxiliary_driver ti_sn_bridge_driver = { };
/* -----------------------------------------------------------------------------
- GPIO Controller
- PWM Controller
- */
+#if defined(CONFIG_PWM) +static int ti_sn_pwm_pin_request(struct ti_sn65dsi86 *pdata) +{
- return atomic_xchg(&pdata->pwm_pin_busy, 1) ? -EBUSY : 0;
+}
+static void ti_sn_pwm_pin_release(struct ti_sn65dsi86 *pdata) +{
- atomic_set(&pdata->pwm_pin_busy, 0);
+}
+static struct ti_sn65dsi86 *pwm_chip_to_ti_sn_bridge(struct pwm_chip *chip) +{
- return container_of(chip, struct ti_sn65dsi86, pchip);
+}
+static int ti_sn_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm) +{
- struct ti_sn65dsi86 *pdata = pwm_chip_to_ti_sn_bridge(chip);
- return ti_sn_pwm_pin_request(pdata);
+}
+static void ti_sn_pwm_free(struct pwm_chip *chip, struct pwm_device *pwm) +{
- struct ti_sn65dsi86 *pdata = pwm_chip_to_ti_sn_bridge(chip);
- ti_sn_pwm_pin_release(pdata);
+}
+/*
- Limitations:
- The PWM signal is not driven when the chip is powered down, or in its
- reset state and the driver does not implement the "suspend state"
- described in the documentation. In order to save power, state->enabled is
- interpreted as denoting if the signal is expected to be valid, and is used
- to determine if the chip needs to be kept powered.
- Changing both period and duty_cycle is not done atomically, neither is the
- multi-byte register updates, so the output might briefly be undefined
*/
- during update.
+static int ti_sn_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
const struct pwm_state *state)+{
- struct ti_sn65dsi86 *pdata = pwm_chip_to_ti_sn_bridge(chip);
- unsigned int pwm_en_inv;
- unsigned int backlight;
- unsigned int pre_div;
- unsigned int scale;
- u64 period_max;
- u64 period;
- int ret;
- if (!pdata->pwm_enabled) {
ret = pm_runtime_get_sync(pdata->dev);if (ret < 0) {pm_runtime_put_sync(pdata->dev);return ret;}- }
- if (state->enabled) {
if (!pdata->pwm_enabled) {/** The chip might have been powered down while we* didn't hold a PM runtime reference, so mux in the* PWM function on the GPIO pin again.*/ret = regmap_update_bits(pdata->regmap, SN_GPIO_CTRL_REG,SN_GPIO_MUX_MASK << (2 * SN_PWM_GPIO_IDX),SN_GPIO_MUX_SPECIAL << (2 * SN_PWM_GPIO_IDX));if (ret) {dev_err(pdata->dev, "failed to mux in PWM function\n");goto out;}}/** Per the datasheet the PWM frequency is given by:** REFCLK_FREQ* PWM_FREQ = -----------------------------------* PWM_PRE_DIV * BACKLIGHT_SCALE + 1** Unfortunately the math becomes overly complex unless we* assume that this formula is missing parenthesis around* BACKLIGHT_SCALE + 1.
We shouldn't assume a different formula than the reference manual describes because it's complex. The reasoning should be that the reference manual is wrong and that someone has convinced themselves the assumed formula is the right one instead.
With the assumed formula: What happens if PWM_PRE_DIV is 0?
* With that the formula can be written:** T_pwm * REFCLK_FREQ = PWM_PRE_DIV * (BACKLIGHT_SCALE + 1)** In order to keep BACKLIGHT_SCALE within its 16 bits,* PWM_PRE_DIV must be:** T_pwm * REFCLK_FREQ* PWM_PRE_DIV >= -------------------------* BACKLIGHT_SCALE_MAX + 1** To simplify the search and optimize the resolution of the* PWM, the lowest possible PWM_PRE_DIV is used. Finally the* scale is calculated as:** T_pwm * REFCLK_FREQ* BACKLIGHT_SCALE = ---------------------- - 1* PWM_PRE_DIV** Here T_pwm is represented in seconds, so appropriate scaling* to nanoseconds is necessary.*//* Minimum T_pwm is (0 * 1) / REFCLK_FREQ */
So the minimal T_pwm (which corresponds to .period, right?) is 0? That sounds wrong.
if (state->period <= NSEC_PER_SEC / pdata->pwm_refclk_freq) {
The implemented check assumes 1 / REFCLK_FREQ, which looks more reasonable.
ret = -EINVAL;goto out;}/** Maximum T_pwm is 255 * (65535 + 1) / REFCLK_FREQ* Limit period to this to avoid overflows*/period_max = div_u64((u64)NSEC_PER_SEC * 255 * (65535 + 1), pdata->pwm_refclk_freq);if (period > period_max)period = period_max;elseperiod = state->period;pre_div = DIV64_U64_ROUND_UP(period * pdata->pwm_refclk_freq,(u64)NSEC_PER_SEC * (BACKLIGHT_SCALE_MAX + 1));scale = div64_u64(period * pdata->pwm_refclk_freq, (u64)NSEC_PER_SEC * pre_div) - 1;
You're loosing precision here as you divide by the result of a division.
/** The documentation has the duty ratio given as:** duty BACKLIGHT* ------- = ---------------------* period BACKLIGHT_SCALE + 1** Solve for BACKLIGHT, substituting BACKLIGHT_SCALE according* to definition above and adjusting for nanosecond* representation of duty cycle gives us:*/backlight = div64_u64(state->duty_cycle * pdata->pwm_refclk_freq, (u64)NSEC_PER_SEC * pre_div);if (backlight > scale)backlight = scale;ret = regmap_write(pdata->regmap, SN_PWM_PRE_DIV_REG, pre_div);if (ret) {dev_err(pdata->dev, "failed to update PWM_PRE_DIV\n");goto out;}ti_sn65dsi86_write_u16(pdata, SN_BACKLIGHT_SCALE_REG, scale);ti_sn65dsi86_write_u16(pdata, SN_BACKLIGHT_REG, backlight);}
pwm_en_inv = FIELD_PREP(SN_PWM_EN_MASK, state->enabled) |
FIELD_PREP(SN_PWM_INV_MASK, state->polarity == PWM_POLARITY_INVERSED);ret = regmap_write(pdata->regmap, SN_PWM_EN_INV_REG, pwm_en_inv);
if (ret) {
dev_err(pdata->dev, "failed to update PWM_EN/PWM_INV\n");goto out;}
pdata->pwm_enabled = state->enabled;
+out:
- if (!pdata->pwm_enabled)
pm_runtime_put_sync(pdata->dev);- return ret;
+}
+static void ti_sn_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
struct pwm_state *state)+{
- struct ti_sn65dsi86 *pdata = pwm_chip_to_ti_sn_bridge(chip);
- unsigned int pwm_en_inv;
- unsigned int pre_div;
- u16 backlight;
- u16 scale;
- int ret;
- ret = regmap_read(pdata->regmap, SN_PWM_EN_INV_REG, &pwm_en_inv);
- if (ret)
return;- ret = ti_sn65dsi86_read_u16(pdata, SN_BACKLIGHT_SCALE_REG, &scale);
- if (ret)
return;- ret = ti_sn65dsi86_read_u16(pdata, SN_BACKLIGHT_REG, &backlight);
- if (ret)
return;- ret = regmap_read(pdata->regmap, SN_PWM_PRE_DIV_REG, &pre_div);
- if (ret)
return;- state->enabled = FIELD_GET(SN_PWM_EN_MASK, pwm_en_inv);
- if (FIELD_GET(SN_PWM_INV_MASK, pwm_en_inv))
state->polarity = PWM_POLARITY_INVERSED;- else
state->polarity = PWM_POLARITY_NORMAL;- state->period = DIV_ROUND_UP_ULL((u64)NSEC_PER_SEC * pre_div * (scale + 1),
pdata->pwm_refclk_freq);- state->duty_cycle = DIV_ROUND_UP_ULL((u64)NSEC_PER_SEC * pre_div * backlight,
pdata->pwm_refclk_freq);
What happens if scale + 1 < backlight?
+}
Best regards Uwe
On Fri 24 Sep 02:54 CDT 2021, Uwe Kleine-K?nig wrote:
Hello,
On Thu, Sep 23, 2021 at 09:12:25PM -0500, Bjorn Andersson wrote:
The SN65DSI86 provides the ability to supply a PWM signal on GPIO 4, with the primary purpose of controlling the backlight of the attached panel. Add an implementation that exposes this using the standard PWM framework, to allow e.g. pwm-backlight to expose this to the user.
Signed-off-by: Bjorn Andersson bjorn.andersson@linaro.org
Changes since v4:
- Rebased the patch
- Turned ti_sn65dsi86_write_u16 into using regmap bulk write
- Moved pwm_refclk_freq out of the #if CONFIG_PWM, to allow removing the guard from ti_sn_bridge_set_refclk_freq()
- Updates "Limitations" text
- pm_runtime_put_sync() on pm_runtime_get_sync() failure
- Added parenthesis around (scale + 1) in the PWM_FREQ formula and thereby redid all the math
- Rewrote the comments related to the math
- Rewrote the math for calculating the backlight (duty cycle) register value
- Dropped some !! on already boolean state->enabled
- Dropped a spurious newline
- Rewrote comment in ti_sn65dsi86_probe() talking about future PWM work
drivers/gpu/drm/bridge/ti-sn65dsi86.c | 369 +++++++++++++++++++++++++- 1 file changed, 361 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c index 41d48a393e7f..857a53dc6c75 100644 --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c @@ -4,7 +4,9 @@
*/
+#include <linux/atomic.h> #include <linux/auxiliary_bus.h> +#include <linux/bitfield.h> #include <linux/bits.h> #include <linux/clk.h> #include <linux/debugfs.h> @@ -15,6 +17,7 @@ #include <linux/module.h> #include <linux/of_graph.h> #include <linux/pm_runtime.h> +#include <linux/pwm.h> #include <linux/regmap.h> #include <linux/regulator/consumer.h>
@@ -91,6 +94,13 @@ #define SN_ML_TX_MODE_REG 0x96 #define ML_TX_MAIN_LINK_OFF 0 #define ML_TX_NORMAL_MODE BIT(0) +#define SN_PWM_PRE_DIV_REG 0xA0 +#define SN_BACKLIGHT_SCALE_REG 0xA1 +#define BACKLIGHT_SCALE_MAX 0xFFFF +#define SN_BACKLIGHT_REG 0xA3 +#define SN_PWM_EN_INV_REG 0xA5 +#define SN_PWM_INV_MASK BIT(0) +#define SN_PWM_EN_MASK BIT(1) #define SN_AUX_CMD_STATUS_REG 0xF4 #define AUX_IRQ_STATUS_AUX_RPLY_TOUT BIT(3) #define AUX_IRQ_STATUS_AUX_SHORT BIT(5) @@ -113,11 +123,14 @@
#define SN_LINK_TRAINING_TRIES 10
+#define SN_PWM_GPIO_IDX 3 /* 4th GPIO */
/**
- struct ti_sn65dsi86 - Platform data for ti-sn65dsi86 driver.
- @bridge_aux: AUX-bus sub device for MIPI-to-eDP bridge functionality.
- @gpio_aux: AUX-bus sub device for GPIO controller functionality.
- @aux_aux: AUX-bus sub device for eDP AUX channel functionality.
- @pwm_aux: AUX-bus sub device for PWM controller functionality.
- @dev: Pointer to the top level (i2c) device.
- @regmap: Regmap for accessing i2c.
@@ -145,11 +158,17 @@
bitmap so we can do atomic ops on it without an extralock so concurrent users of our 4 GPIOs don't stomp oneach other's read-modify-write.
- @pchip: pwm_chip if the PWM is exposed.
- @pwm_enabled: Used to track if the PWM signal is currently enabled.
- @pwm_pin_busy: Track if GPIO4 is currently requested for GPIO or PWM.
*/
- @pwm_refclk_freq: Cache for the reference clock input to the PWM.
struct ti_sn65dsi86 { struct auxiliary_device bridge_aux; struct auxiliary_device gpio_aux; struct auxiliary_device aux_aux;
struct auxiliary_device pwm_aux;
struct device *dev; struct regmap *regmap;
@@ -172,6 +191,12 @@ struct ti_sn65dsi86 { struct gpio_chip gchip; DECLARE_BITMAP(gchip_output, SN_NUM_GPIOS); #endif +#if defined(CONFIG_PWM)
- struct pwm_chip pchip;
- bool pwm_enabled;
- atomic_t pwm_pin_busy;
+#endif
- unsigned int pwm_refclk_freq;
};
static const struct regmap_range ti_sn65dsi86_volatile_ranges[] = { @@ -190,11 +215,31 @@ static const struct regmap_config ti_sn65dsi86_regmap_config = { .cache_type = REGCACHE_NONE, };
+static int ti_sn65dsi86_read_u16(struct ti_sn65dsi86 *pdata,
unsigned int reg, u16 *val)+{
- unsigned int tmp;
- int ret;
- ret = regmap_read(pdata->regmap, reg, &tmp);
- if (ret)
return ret;- *val = tmp;
- ret = regmap_read(pdata->regmap, reg + 1, &tmp);
- if (ret)
return ret;- *val |= tmp << 8;
- return 0;
+}
static void ti_sn65dsi86_write_u16(struct ti_sn65dsi86 *pdata, unsigned int reg, u16 val) {
- regmap_write(pdata->regmap, reg, val & 0xFF);
- regmap_write(pdata->regmap, reg + 1, val >> 8);
- u8 buf[2] = { val & 0xff, val >> 8 };
- regmap_bulk_write(pdata->regmap, reg, &buf, ARRAY_SIZE(buf));
Given that ti_sn65dsi86_write_u16 uses regmap_bulk_write I wonder why ti_sn65dsi86_read_u16 doesn't use regmap_bulk_read.
Seems reasonable to make that use the bulk interface as well and this would look better in its own patch.
}
static u32 ti_sn_bridge_get_dsi_freq(struct ti_sn65dsi86 *pdata) @@ -253,6 +298,12 @@ static void ti_sn_bridge_set_refclk_freq(struct ti_sn65dsi86 *pdata)
regmap_update_bits(pdata->regmap, SN_DPPLL_SRC_REG, REFCLK_FREQ_MASK, REFCLK_FREQ(i));
- /*
* The PWM refclk is based on the value written to SN_DPPLL_SRC_REG,* regardless of its actual sourcing.*/- pdata->pwm_refclk_freq = ti_sn_bridge_refclk_lut[i];
}
static void ti_sn65dsi86_enable_comms(struct ti_sn65dsi86 *pdata) @@ -1259,9 +1310,283 @@ static struct auxiliary_driver ti_sn_bridge_driver = { };
/* -----------------------------------------------------------------------------
- GPIO Controller
- PWM Controller
- */
+#if defined(CONFIG_PWM) +static int ti_sn_pwm_pin_request(struct ti_sn65dsi86 *pdata) +{
- return atomic_xchg(&pdata->pwm_pin_busy, 1) ? -EBUSY : 0;
+}
+static void ti_sn_pwm_pin_release(struct ti_sn65dsi86 *pdata) +{
- atomic_set(&pdata->pwm_pin_busy, 0);
+}
+static struct ti_sn65dsi86 *pwm_chip_to_ti_sn_bridge(struct pwm_chip *chip) +{
- return container_of(chip, struct ti_sn65dsi86, pchip);
+}
+static int ti_sn_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm) +{
- struct ti_sn65dsi86 *pdata = pwm_chip_to_ti_sn_bridge(chip);
- return ti_sn_pwm_pin_request(pdata);
+}
+static void ti_sn_pwm_free(struct pwm_chip *chip, struct pwm_device *pwm) +{
- struct ti_sn65dsi86 *pdata = pwm_chip_to_ti_sn_bridge(chip);
- ti_sn_pwm_pin_release(pdata);
+}
+/*
- Limitations:
- The PWM signal is not driven when the chip is powered down, or in its
- reset state and the driver does not implement the "suspend state"
- described in the documentation. In order to save power, state->enabled is
- interpreted as denoting if the signal is expected to be valid, and is used
- to determine if the chip needs to be kept powered.
- Changing both period and duty_cycle is not done atomically, neither is the
- multi-byte register updates, so the output might briefly be undefined
*/
- during update.
+static int ti_sn_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
const struct pwm_state *state)+{
- struct ti_sn65dsi86 *pdata = pwm_chip_to_ti_sn_bridge(chip);
- unsigned int pwm_en_inv;
- unsigned int backlight;
- unsigned int pre_div;
- unsigned int scale;
- u64 period_max;
- u64 period;
- int ret;
- if (!pdata->pwm_enabled) {
ret = pm_runtime_get_sync(pdata->dev);if (ret < 0) {pm_runtime_put_sync(pdata->dev);return ret;}- }
- if (state->enabled) {
if (!pdata->pwm_enabled) {/** The chip might have been powered down while we* didn't hold a PM runtime reference, so mux in the* PWM function on the GPIO pin again.*/ret = regmap_update_bits(pdata->regmap, SN_GPIO_CTRL_REG,SN_GPIO_MUX_MASK << (2 * SN_PWM_GPIO_IDX),SN_GPIO_MUX_SPECIAL << (2 * SN_PWM_GPIO_IDX));if (ret) {dev_err(pdata->dev, "failed to mux in PWM function\n");goto out;}}/** Per the datasheet the PWM frequency is given by:** REFCLK_FREQ* PWM_FREQ = -----------------------------------* PWM_PRE_DIV * BACKLIGHT_SCALE + 1** Unfortunately the math becomes overly complex unless we* assume that this formula is missing parenthesis around* BACKLIGHT_SCALE + 1.We shouldn't assume a different formula than the reference manual describes because it's complex. The reasoning should be that the reference manual is wrong and that someone has convinced themselves the assumed formula is the right one instead.
Given the effort put in to conclude that there must be some parenthesis there, I agree that "assume" might be to weak of a word...
With the assumed formula: What happens if PWM_PRE_DIV is 0?
Looking at the specification again and I don't see anything prohibiting from writing 0 in the PRE_DIV register, and writing a 0 to the register causes no visual on my backlight from writing 1.
Per our new understanding this should probably have resulted in a period of 0. That said, if the formula from the datasheet was correct then we'd have a period T_pwm (given T_ref as refclk pulse length) of:
T_pwm = T_ref * (div * scale + 1)
And with div = 0 we'd have:
T_pwm = T_ref * 1
Which wouldn't give any room for adjusting the duty cycle, and I can still do that. So that's not correct either.
Unfortunately I still don't want to dismantle my working laptop, so I don't know if I can do any better than this. Perhaps they do the typical trick of encoding prediv off-by-1 and the PWM duty is off by a factor of 2. Perhaps the comparator for the prediv counter trips at 1 even though the register is configured at 0...
* With that the formula can be written:** T_pwm * REFCLK_FREQ = PWM_PRE_DIV * (BACKLIGHT_SCALE + 1)** In order to keep BACKLIGHT_SCALE within its 16 bits,* PWM_PRE_DIV must be:** T_pwm * REFCLK_FREQ* PWM_PRE_DIV >= -------------------------* BACKLIGHT_SCALE_MAX + 1** To simplify the search and optimize the resolution of the* PWM, the lowest possible PWM_PRE_DIV is used. Finally the* scale is calculated as:** T_pwm * REFCLK_FREQ* BACKLIGHT_SCALE = ---------------------- - 1* PWM_PRE_DIV** Here T_pwm is represented in seconds, so appropriate scaling* to nanoseconds is necessary.*//* Minimum T_pwm is (0 * 1) / REFCLK_FREQ */So the minimal T_pwm (which corresponds to .period, right?) is 0? That sounds wrong.
Yes, that seems more likely.
if (state->period <= NSEC_PER_SEC / pdata->pwm_refclk_freq) {The implemented check assumes 1 / REFCLK_FREQ, which looks more reasonable.
I agree.
ret = -EINVAL;goto out;}/** Maximum T_pwm is 255 * (65535 + 1) / REFCLK_FREQ* Limit period to this to avoid overflows*/period_max = div_u64((u64)NSEC_PER_SEC * 255 * (65535 + 1), pdata->pwm_refclk_freq);if (period > period_max)period = period_max;elseperiod = state->period;pre_div = DIV64_U64_ROUND_UP(period * pdata->pwm_refclk_freq,(u64)NSEC_PER_SEC * (BACKLIGHT_SCALE_MAX + 1));scale = div64_u64(period * pdata->pwm_refclk_freq, (u64)NSEC_PER_SEC * pre_div) - 1;You're loosing precision here as you divide by the result of a division.
As described above, I'm trying to find suitable values for PRE_DIV and BACKLIGHT_SCALE in:
T_pwm * refclk_freq --------------------- = PRE_DIV * (BACKLIGHT_SCALE + 1) NSEC_PER_SEC
BACKLIGHT_SCALE must fit in 16 bits and in order to be useful for driving the backlight control be large enough that the resolution (BACKLIGTH = [0..BACKLIGHT_SCALE]) covers whatever resolution pwm-backlight might expose.
For the intended use case this seems pretty good, but I presume you could pick better values to get closer to the requested period.
Do you have a better suggestion for how to pick PRE_DIV and BACKLIGHT_SCALE?
/** The documentation has the duty ratio given as:** duty BACKLIGHT* ------- = ---------------------* period BACKLIGHT_SCALE + 1** Solve for BACKLIGHT, substituting BACKLIGHT_SCALE according* to definition above and adjusting for nanosecond* representation of duty cycle gives us:*/backlight = div64_u64(state->duty_cycle * pdata->pwm_refclk_freq, (u64)NSEC_PER_SEC * pre_div);if (backlight > scale)backlight = scale;ret = regmap_write(pdata->regmap, SN_PWM_PRE_DIV_REG, pre_div);if (ret) {dev_err(pdata->dev, "failed to update PWM_PRE_DIV\n");goto out;}ti_sn65dsi86_write_u16(pdata, SN_BACKLIGHT_SCALE_REG, scale);ti_sn65dsi86_write_u16(pdata, SN_BACKLIGHT_REG, backlight);}
pwm_en_inv = FIELD_PREP(SN_PWM_EN_MASK, state->enabled) |
FIELD_PREP(SN_PWM_INV_MASK, state->polarity == PWM_POLARITY_INVERSED);ret = regmap_write(pdata->regmap, SN_PWM_EN_INV_REG, pwm_en_inv);
if (ret) {
dev_err(pdata->dev, "failed to update PWM_EN/PWM_INV\n");goto out;}
pdata->pwm_enabled = state->enabled;
+out:
- if (!pdata->pwm_enabled)
pm_runtime_put_sync(pdata->dev);- return ret;
+}
+static void ti_sn_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
struct pwm_state *state)+{
- struct ti_sn65dsi86 *pdata = pwm_chip_to_ti_sn_bridge(chip);
- unsigned int pwm_en_inv;
- unsigned int pre_div;
- u16 backlight;
- u16 scale;
- int ret;
- ret = regmap_read(pdata->regmap, SN_PWM_EN_INV_REG, &pwm_en_inv);
- if (ret)
return;- ret = ti_sn65dsi86_read_u16(pdata, SN_BACKLIGHT_SCALE_REG, &scale);
- if (ret)
return;- ret = ti_sn65dsi86_read_u16(pdata, SN_BACKLIGHT_REG, &backlight);
- if (ret)
return;- ret = regmap_read(pdata->regmap, SN_PWM_PRE_DIV_REG, &pre_div);
- if (ret)
return;- state->enabled = FIELD_GET(SN_PWM_EN_MASK, pwm_en_inv);
- if (FIELD_GET(SN_PWM_INV_MASK, pwm_en_inv))
state->polarity = PWM_POLARITY_INVERSED;- else
state->polarity = PWM_POLARITY_NORMAL;- state->period = DIV_ROUND_UP_ULL((u64)NSEC_PER_SEC * pre_div * (scale + 1),
pdata->pwm_refclk_freq);- state->duty_cycle = DIV_ROUND_UP_ULL((u64)NSEC_PER_SEC * pre_div * backlight,
pdata->pwm_refclk_freq);What happens if scale + 1 < backlight?
When we write the backlight register above, we cap it at scale, so for the typical case that shouldn't happen.
So I guess the one case when this could happen is if we get_state() before pwm_apply(), when someone else has written broken values (reset values are scale = 0xff and backlight = 0)
Thanks, Bjorn
Hello Bjorn,
On Fri, Sep 24, 2021 at 05:04:41PM -0500, Bjorn Andersson wrote:
On Fri 24 Sep 02:54 CDT 2021, Uwe Kleine-K?nig wrote:
+static int ti_sn65dsi86_read_u16(struct ti_sn65dsi86 *pdata,
unsigned int reg, u16 *val)+{
- unsigned int tmp;
- int ret;
- ret = regmap_read(pdata->regmap, reg, &tmp);
- if (ret)
return ret;- *val = tmp;
- ret = regmap_read(pdata->regmap, reg + 1, &tmp);
- if (ret)
return ret;- *val |= tmp << 8;
- return 0;
+}
static void ti_sn65dsi86_write_u16(struct ti_sn65dsi86 *pdata, unsigned int reg, u16 val) {
- regmap_write(pdata->regmap, reg, val & 0xFF);
- regmap_write(pdata->regmap, reg + 1, val >> 8);
- u8 buf[2] = { val & 0xff, val >> 8 };
- regmap_bulk_write(pdata->regmap, reg, &buf, ARRAY_SIZE(buf));
Given that ti_sn65dsi86_write_u16 uses regmap_bulk_write I wonder why ti_sn65dsi86_read_u16 doesn't use regmap_bulk_read.
Seems reasonable to make that use the bulk interface as well and this would look better in its own patch.
Not sure I understand you here. My position here would be to introduce these two functions with a consistent behaviour. If both use bulk or both don't use it (and you introduce it later in a separate patch) doesn't matter to me.
static u32 ti_sn_bridge_get_dsi_freq(struct ti_sn65dsi86 *pdata) @@ -253,6 +298,12 @@ static void ti_sn_bridge_set_refclk_freq(struct ti_sn65dsi86 *pdata)
regmap_update_bits(pdata->regmap, SN_DPPLL_SRC_REG, REFCLK_FREQ_MASK, REFCLK_FREQ(i));
- /*
* The PWM refclk is based on the value written to SN_DPPLL_SRC_REG,* regardless of its actual sourcing.*/- pdata->pwm_refclk_freq = ti_sn_bridge_refclk_lut[i];
}
static void ti_sn65dsi86_enable_comms(struct ti_sn65dsi86 *pdata) @@ -1259,9 +1310,283 @@ static struct auxiliary_driver ti_sn_bridge_driver = { };
/* -----------------------------------------------------------------------------
- GPIO Controller
- PWM Controller
- */
+#if defined(CONFIG_PWM) +static int ti_sn_pwm_pin_request(struct ti_sn65dsi86 *pdata) +{
- return atomic_xchg(&pdata->pwm_pin_busy, 1) ? -EBUSY : 0;
+}
+static void ti_sn_pwm_pin_release(struct ti_sn65dsi86 *pdata) +{
- atomic_set(&pdata->pwm_pin_busy, 0);
+}
+static struct ti_sn65dsi86 *pwm_chip_to_ti_sn_bridge(struct pwm_chip *chip) +{
- return container_of(chip, struct ti_sn65dsi86, pchip);
+}
+static int ti_sn_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm) +{
- struct ti_sn65dsi86 *pdata = pwm_chip_to_ti_sn_bridge(chip);
- return ti_sn_pwm_pin_request(pdata);
+}
+static void ti_sn_pwm_free(struct pwm_chip *chip, struct pwm_device *pwm) +{
- struct ti_sn65dsi86 *pdata = pwm_chip_to_ti_sn_bridge(chip);
- ti_sn_pwm_pin_release(pdata);
+}
+/*
- Limitations:
- The PWM signal is not driven when the chip is powered down, or in its
- reset state and the driver does not implement the "suspend state"
- described in the documentation. In order to save power, state->enabled is
- interpreted as denoting if the signal is expected to be valid, and is used
- to determine if the chip needs to be kept powered.
- Changing both period and duty_cycle is not done atomically, neither is the
- multi-byte register updates, so the output might briefly be undefined
*/
- during update.
+static int ti_sn_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
const struct pwm_state *state)+{
- struct ti_sn65dsi86 *pdata = pwm_chip_to_ti_sn_bridge(chip);
- unsigned int pwm_en_inv;
- unsigned int backlight;
- unsigned int pre_div;
- unsigned int scale;
- u64 period_max;
- u64 period;
- int ret;
- if (!pdata->pwm_enabled) {
ret = pm_runtime_get_sync(pdata->dev);if (ret < 0) {pm_runtime_put_sync(pdata->dev);return ret;}- }
- if (state->enabled) {
if (!pdata->pwm_enabled) {/** The chip might have been powered down while we* didn't hold a PM runtime reference, so mux in the* PWM function on the GPIO pin again.*/ret = regmap_update_bits(pdata->regmap, SN_GPIO_CTRL_REG,SN_GPIO_MUX_MASK << (2 * SN_PWM_GPIO_IDX),SN_GPIO_MUX_SPECIAL << (2 * SN_PWM_GPIO_IDX));if (ret) {dev_err(pdata->dev, "failed to mux in PWM function\n");goto out;}}/** Per the datasheet the PWM frequency is given by:** REFCLK_FREQ* PWM_FREQ = -----------------------------------* PWM_PRE_DIV * BACKLIGHT_SCALE + 1** Unfortunately the math becomes overly complex unless we* assume that this formula is missing parenthesis around* BACKLIGHT_SCALE + 1.We shouldn't assume a different formula than the reference manual describes because it's complex. The reasoning should be that the reference manual is wrong and that someone has convinced themselves the assumed formula is the right one instead.
Given the effort put in to conclude that there must be some parenthesis there, I agree that "assume" might be to weak of a word...
With the assumed formula: What happens if PWM_PRE_DIV is 0?
Looking at the specification again and I don't see anything prohibiting from writing 0 in the PRE_DIV register, and writing a 0 to the register causes no visual on my backlight from writing 1.
So the backlight behaves identically for PRE_DIV = 0 and PRE_DIV = 1 as far as you can tell?
Per our new understanding this should probably have resulted in a period of 0.
... but a period of zero doesn't make sense.
That said, if the formula from the datasheet was correct then we'd have a period T_pwm (given T_ref as refclk pulse length) of:
T_pwm = T_ref * (div * scale + 1)
And with div = 0 we'd have:
T_pwm = T_ref * 1
Which wouldn't give any room for adjusting the duty cycle, and I can still do that. So that's not correct either.
I don't see a problem here (just the hardware would be unusual and complicated and so more expensive than the simple straigt forward way).
One way to find out if
PWM_PRE_DIV * BACKLIGHT_SCALE + 1 ----------------------------------- REFCLK_FREQ
or
PWM_PRE_DIV * (BACKLIGHT_SCALE + 1) ------------------------------------- REFCLK_FREQ
is the right formula is to program PWM_PRE_DIV=1 and BACKLIGHT_SCALE=1 and then check if the backlight is fully on with SN_BACKLIGHT_REG=2 (or only with SN_BACKLIGHT_REG=3).
Unfortunately I still don't want to dismantle my working laptop, so I don't know if I can do any better than this. Perhaps they do the typical trick of encoding prediv off-by-1 and the PWM duty is off by a factor of 2. Perhaps the comparator for the prediv counter trips at 1 even though the register is configured at 0...
I would guess the latter.
/** Maximum T_pwm is 255 * (65535 + 1) / REFCLK_FREQ* Limit period to this to avoid overflows*/period_max = div_u64((u64)NSEC_PER_SEC * 255 * (65535 + 1), pdata->pwm_refclk_freq);if (period > period_max)period = period_max;elseperiod = state->period;pre_div = DIV64_U64_ROUND_UP(period * pdata->pwm_refclk_freq,(u64)NSEC_PER_SEC * (BACKLIGHT_SCALE_MAX + 1));scale = div64_u64(period * pdata->pwm_refclk_freq, (u64)NSEC_PER_SEC * pre_div) - 1;You're loosing precision here as you divide by the result of a division.
As described above, I'm trying to find suitable values for PRE_DIV and BACKLIGHT_SCALE in:
T_pwm * refclk_freq --------------------- = PRE_DIV * (BACKLIGHT_SCALE + 1) NSEC_PER_SEC
BACKLIGHT_SCALE must fit in 16 bits and in order to be useful for driving the backlight control be large enough that the resolution (BACKLIGTH = [0..BACKLIGHT_SCALE]) covers whatever resolution pwm-backlight might expose.
For the intended use case this seems pretty good, but I presume you could pick better values to get closer to the requested period.
Do you have a better suggestion for how to pick PRE_DIV and BACKLIGHT_SCALE?
My motivation is limited to spend brain cycles here if we're not sure we're using the right formula. Maybe my concern is wrong given that pre_div isn't only an interim result but an actual register value. I will have to think about that when I'm not tired.
+static void ti_sn_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
struct pwm_state *state)+{
- struct ti_sn65dsi86 *pdata = pwm_chip_to_ti_sn_bridge(chip);
- unsigned int pwm_en_inv;
- unsigned int pre_div;
- u16 backlight;
- u16 scale;
- int ret;
- ret = regmap_read(pdata->regmap, SN_PWM_EN_INV_REG, &pwm_en_inv);
- if (ret)
return;- ret = ti_sn65dsi86_read_u16(pdata, SN_BACKLIGHT_SCALE_REG, &scale);
- if (ret)
return;- ret = ti_sn65dsi86_read_u16(pdata, SN_BACKLIGHT_REG, &backlight);
- if (ret)
return;- ret = regmap_read(pdata->regmap, SN_PWM_PRE_DIV_REG, &pre_div);
- if (ret)
return;- state->enabled = FIELD_GET(SN_PWM_EN_MASK, pwm_en_inv);
- if (FIELD_GET(SN_PWM_INV_MASK, pwm_en_inv))
state->polarity = PWM_POLARITY_INVERSED;- else
state->polarity = PWM_POLARITY_NORMAL;- state->period = DIV_ROUND_UP_ULL((u64)NSEC_PER_SEC * pre_div * (scale + 1),
pdata->pwm_refclk_freq);- state->duty_cycle = DIV_ROUND_UP_ULL((u64)NSEC_PER_SEC * pre_div * backlight,
pdata->pwm_refclk_freq);What happens if scale + 1 < backlight?
When we write the backlight register above, we cap it at scale, so for the typical case that shouldn't happen.
.get_state() is called before the first .apply(), to the values to interpret here originate from the bootloader which might write strange settings that the linux driver would never do. So being prudent here is IMHO a good idea.
It's a shame that .get_state() cannot return an error.
So I guess the one case when this could happen is if we get_state() before pwm_apply(), when someone else has written broken values
ack.
Best regards Uwe
On Sat 25 Sep 16:25 CDT 2021, Uwe Kleine-K?nig wrote:
Hello Bjorn,
On Fri, Sep 24, 2021 at 05:04:41PM -0500, Bjorn Andersson wrote:
On Fri 24 Sep 02:54 CDT 2021, Uwe Kleine-K?nig wrote:
+static int ti_sn65dsi86_read_u16(struct ti_sn65dsi86 *pdata,
unsigned int reg, u16 *val)+{
- unsigned int tmp;
- int ret;
- ret = regmap_read(pdata->regmap, reg, &tmp);
- if (ret)
return ret;- *val = tmp;
- ret = regmap_read(pdata->regmap, reg + 1, &tmp);
- if (ret)
return ret;- *val |= tmp << 8;
- return 0;
+}
static void ti_sn65dsi86_write_u16(struct ti_sn65dsi86 *pdata, unsigned int reg, u16 val) {
- regmap_write(pdata->regmap, reg, val & 0xFF);
- regmap_write(pdata->regmap, reg + 1, val >> 8);
- u8 buf[2] = { val & 0xff, val >> 8 };
- regmap_bulk_write(pdata->regmap, reg, &buf, ARRAY_SIZE(buf));
Given that ti_sn65dsi86_write_u16 uses regmap_bulk_write I wonder why ti_sn65dsi86_read_u16 doesn't use regmap_bulk_read.
Seems reasonable to make that use the bulk interface as well and this would look better in its own patch.
Not sure I understand you here. My position here would be to introduce these two functions with a consistent behaviour. If both use bulk or both don't use it (and you introduce it later in a separate patch) doesn't matter to me.
We're in agreement :)
static u32 ti_sn_bridge_get_dsi_freq(struct ti_sn65dsi86 *pdata) @@ -253,6 +298,12 @@ static void ti_sn_bridge_set_refclk_freq(struct ti_sn65dsi86 *pdata)
regmap_update_bits(pdata->regmap, SN_DPPLL_SRC_REG, REFCLK_FREQ_MASK, REFCLK_FREQ(i));
- /*
* The PWM refclk is based on the value written to SN_DPPLL_SRC_REG,* regardless of its actual sourcing.*/- pdata->pwm_refclk_freq = ti_sn_bridge_refclk_lut[i];
}
static void ti_sn65dsi86_enable_comms(struct ti_sn65dsi86 *pdata) @@ -1259,9 +1310,283 @@ static struct auxiliary_driver ti_sn_bridge_driver = { };
/* -----------------------------------------------------------------------------
- GPIO Controller
- PWM Controller
- */
+#if defined(CONFIG_PWM) +static int ti_sn_pwm_pin_request(struct ti_sn65dsi86 *pdata) +{
- return atomic_xchg(&pdata->pwm_pin_busy, 1) ? -EBUSY : 0;
+}
+static void ti_sn_pwm_pin_release(struct ti_sn65dsi86 *pdata) +{
- atomic_set(&pdata->pwm_pin_busy, 0);
+}
+static struct ti_sn65dsi86 *pwm_chip_to_ti_sn_bridge(struct pwm_chip *chip) +{
- return container_of(chip, struct ti_sn65dsi86, pchip);
+}
+static int ti_sn_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm) +{
- struct ti_sn65dsi86 *pdata = pwm_chip_to_ti_sn_bridge(chip);
- return ti_sn_pwm_pin_request(pdata);
+}
+static void ti_sn_pwm_free(struct pwm_chip *chip, struct pwm_device *pwm) +{
- struct ti_sn65dsi86 *pdata = pwm_chip_to_ti_sn_bridge(chip);
- ti_sn_pwm_pin_release(pdata);
+}
+/*
- Limitations:
- The PWM signal is not driven when the chip is powered down, or in its
- reset state and the driver does not implement the "suspend state"
- described in the documentation. In order to save power, state->enabled is
- interpreted as denoting if the signal is expected to be valid, and is used
- to determine if the chip needs to be kept powered.
- Changing both period and duty_cycle is not done atomically, neither is the
- multi-byte register updates, so the output might briefly be undefined
*/
- during update.
+static int ti_sn_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
const struct pwm_state *state)+{
- struct ti_sn65dsi86 *pdata = pwm_chip_to_ti_sn_bridge(chip);
- unsigned int pwm_en_inv;
- unsigned int backlight;
- unsigned int pre_div;
- unsigned int scale;
- u64 period_max;
- u64 period;
- int ret;
- if (!pdata->pwm_enabled) {
ret = pm_runtime_get_sync(pdata->dev);if (ret < 0) {pm_runtime_put_sync(pdata->dev);return ret;}- }
- if (state->enabled) {
if (!pdata->pwm_enabled) {/** The chip might have been powered down while we* didn't hold a PM runtime reference, so mux in the* PWM function on the GPIO pin again.*/ret = regmap_update_bits(pdata->regmap, SN_GPIO_CTRL_REG,SN_GPIO_MUX_MASK << (2 * SN_PWM_GPIO_IDX),SN_GPIO_MUX_SPECIAL << (2 * SN_PWM_GPIO_IDX));if (ret) {dev_err(pdata->dev, "failed to mux in PWM function\n");goto out;}}/** Per the datasheet the PWM frequency is given by:** REFCLK_FREQ* PWM_FREQ = -----------------------------------* PWM_PRE_DIV * BACKLIGHT_SCALE + 1** Unfortunately the math becomes overly complex unless we* assume that this formula is missing parenthesis around* BACKLIGHT_SCALE + 1.We shouldn't assume a different formula than the reference manual describes because it's complex. The reasoning should be that the reference manual is wrong and that someone has convinced themselves the assumed formula is the right one instead.
Given the effort put in to conclude that there must be some parenthesis there, I agree that "assume" might be to weak of a word...
With the assumed formula: What happens if PWM_PRE_DIV is 0?
Looking at the specification again and I don't see anything prohibiting from writing 0 in the PRE_DIV register, and writing a 0 to the register causes no visual on my backlight from writing 1.
So the backlight behaves identically for PRE_DIV = 0 and PRE_DIV = 1 as far as you can tell?
Flipping between them I see no difference on my screen.
Per our new understanding this should probably have resulted in a period of 0.
... but a period of zero doesn't make sense.
Right, that's what I meant. If the hardware really did operate with a zero period (or a period of 1 refclock cycle) that would be visually noticeable.
That said, if the formula from the datasheet was correct then we'd have a period T_pwm (given T_ref as refclk pulse length) of:
T_pwm = T_ref * (div * scale + 1)
And with div = 0 we'd have:
T_pwm = T_ref * 1
Which wouldn't give any room for adjusting the duty cycle, and I can still do that. So that's not correct either.
I don't see a problem here (just the hardware would be unusual and complicated and so more expensive than the simple straigt forward way).
One way to find out if
PWM_PRE_DIV * BACKLIGHT_SCALE + 1
REFCLK_FREQor
PWM_PRE_DIV * (BACKLIGHT_SCALE + 1)
REFCLK_FREQis the right formula is to program PWM_PRE_DIV=1 and BACKLIGHT_SCALE=1 and then check if the backlight is fully on with SN_BACKLIGHT_REG=2 (or only with SN_BACKLIGHT_REG=3).
Unfortunately I still don't want to dismantle my working laptop, so I don't know if I can do any better than this. Perhaps they do the typical trick of encoding prediv off-by-1 and the PWM duty is off by a factor of 2. Perhaps the comparator for the prediv counter trips at 1 even though the register is configured at 0...
I would guess the latter.
I will do some more experiments, but as we concluded previously, the first formula (without parenthesis) doesn't make sense.
/** Maximum T_pwm is 255 * (65535 + 1) / REFCLK_FREQ* Limit period to this to avoid overflows*/period_max = div_u64((u64)NSEC_PER_SEC * 255 * (65535 + 1), pdata->pwm_refclk_freq);if (period > period_max)period = period_max;elseperiod = state->period;pre_div = DIV64_U64_ROUND_UP(period * pdata->pwm_refclk_freq,(u64)NSEC_PER_SEC * (BACKLIGHT_SCALE_MAX + 1));scale = div64_u64(period * pdata->pwm_refclk_freq, (u64)NSEC_PER_SEC * pre_div) - 1;You're loosing precision here as you divide by the result of a division.
As described above, I'm trying to find suitable values for PRE_DIV and BACKLIGHT_SCALE in:
T_pwm * refclk_freq --------------------- = PRE_DIV * (BACKLIGHT_SCALE + 1) NSEC_PER_SEC
BACKLIGHT_SCALE must fit in 16 bits and in order to be useful for driving the backlight control be large enough that the resolution (BACKLIGTH = [0..BACKLIGHT_SCALE]) covers whatever resolution pwm-backlight might expose.
For the intended use case this seems pretty good, but I presume you could pick better values to get closer to the requested period.
Do you have a better suggestion for how to pick PRE_DIV and BACKLIGHT_SCALE?
My motivation is limited to spend brain cycles here if we're not sure we're using the right formula. Maybe my concern is wrong given that pre_div isn't only an interim result but an actual register value. I will have to think about that when I'm not tired.
The alternative to this approach, afaict, would be to search for PRE_DIV and BACKLIGHT_SCALE that gives us the closest period to the requested one.
But that would come at the expense of duty cycle resolution, which is what this currently optimizes for.
+static void ti_sn_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
struct pwm_state *state)+{
- struct ti_sn65dsi86 *pdata = pwm_chip_to_ti_sn_bridge(chip);
- unsigned int pwm_en_inv;
- unsigned int pre_div;
- u16 backlight;
- u16 scale;
- int ret;
- ret = regmap_read(pdata->regmap, SN_PWM_EN_INV_REG, &pwm_en_inv);
- if (ret)
return;- ret = ti_sn65dsi86_read_u16(pdata, SN_BACKLIGHT_SCALE_REG, &scale);
- if (ret)
return;- ret = ti_sn65dsi86_read_u16(pdata, SN_BACKLIGHT_REG, &backlight);
- if (ret)
return;- ret = regmap_read(pdata->regmap, SN_PWM_PRE_DIV_REG, &pre_div);
- if (ret)
return;- state->enabled = FIELD_GET(SN_PWM_EN_MASK, pwm_en_inv);
- if (FIELD_GET(SN_PWM_INV_MASK, pwm_en_inv))
state->polarity = PWM_POLARITY_INVERSED;- else
state->polarity = PWM_POLARITY_NORMAL;- state->period = DIV_ROUND_UP_ULL((u64)NSEC_PER_SEC * pre_div * (scale + 1),
pdata->pwm_refclk_freq);- state->duty_cycle = DIV_ROUND_UP_ULL((u64)NSEC_PER_SEC * pre_div * backlight,
pdata->pwm_refclk_freq);What happens if scale + 1 < backlight?
When we write the backlight register above, we cap it at scale, so for the typical case that shouldn't happen.
.get_state() is called before the first .apply(), to the values to interpret here originate from the bootloader which might write strange settings that the linux driver would never do. So being prudent here is IMHO a good idea.
Didn't notice that as I traced things, I will add some sanity nudging here.
It's a shame that .get_state() cannot return an error.
So I guess the one case when this could happen is if we get_state() before pwm_apply(), when someone else has written broken values
ack.
Thanks, Bjorn
On Thu, Sep 23, 2021 at 09:12:24PM -0500, Bjorn Andersson wrote:
The existing pxa driver and the upcoming addition of PWM support in the TI sn565dsi86 DSI/eDP bridge driver both has a single PWM channel and thereby a need for a of_xlate function with the period as its single argument.
Introduce a common helper function in the core that can be used as of_xlate by such drivers and migrate the pxa driver to use this.
Signed-off-by: Bjorn Andersson bjorn.andersson@linaro.org
Changes since v4:
- None
drivers/pwm/core.c | 26 ++++++++++++++++++++++++++ drivers/pwm/pwm-pxa.c | 16 +--------------- include/linux/pwm.h | 2 ++ 3 files changed, 29 insertions(+), 15 deletions(-)
diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c index 4527f09a5c50..2c6b155002a2 100644 --- a/drivers/pwm/core.c +++ b/drivers/pwm/core.c @@ -152,6 +152,32 @@ of_pwm_xlate_with_flags(struct pwm_chip *pc, const struct of_phandle_args *args) } EXPORT_SYMBOL_GPL(of_pwm_xlate_with_flags);
+struct pwm_device * +of_pwm_single_xlate(struct pwm_chip *pc, const struct of_phandle_args *args) +{
- struct pwm_device *pwm;
- if (pc->of_pwm_n_cells < 1)
return ERR_PTR(-EINVAL);- /* validate that one cell is specified, optionally with flags */
- if (args->args_count != 1 && args->args_count != 2)
return ERR_PTR(-EINVAL);- pwm = pwm_request_from_chip(pc, 0, NULL);
- if (IS_ERR(pwm))
return pwm;- pwm->args.period = args->args[0];
- pwm->args.polarity = PWM_POLARITY_NORMAL;
- if (args->args_count == 2 && args->args[2] & PWM_POLARITY_INVERTED)
pwm->args.polarity = PWM_POLARITY_INVERSED;
of_pwm_xlate_with_flags is a bit more complicated. Translating accordingly this would yield:
if (pc->of_pwm_n_cells >= 2) { if (args->args_count > 1 && args->args[1] & PWM_POLARITY_INVERTED) pwm->args.polarity = PWM_POLARITY_INVERSED; }
Given that pc->of_pwm_n_cells isn't used when a phandle is parsed (in of_pwm_get()) I think your variant is fine.
So I think technically the patch is good, for me the question is if we want to make new drivers of_pwm_xlate_with_flags for consistency even though this would mean that the first argument has to be 0 for all phandles. Thierry? Lee?
Best regards Uwe
On Fri 24 Sep 00:16 PDT 2021, Uwe Kleine-K?nig wrote:
On Thu, Sep 23, 2021 at 09:12:24PM -0500, Bjorn Andersson wrote:
The existing pxa driver and the upcoming addition of PWM support in the TI sn565dsi86 DSI/eDP bridge driver both has a single PWM channel and thereby a need for a of_xlate function with the period as its single argument.
Introduce a common helper function in the core that can be used as of_xlate by such drivers and migrate the pxa driver to use this.
Signed-off-by: Bjorn Andersson bjorn.andersson@linaro.org
Changes since v4:
- None
drivers/pwm/core.c | 26 ++++++++++++++++++++++++++ drivers/pwm/pwm-pxa.c | 16 +--------------- include/linux/pwm.h | 2 ++ 3 files changed, 29 insertions(+), 15 deletions(-)
diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c index 4527f09a5c50..2c6b155002a2 100644 --- a/drivers/pwm/core.c +++ b/drivers/pwm/core.c @@ -152,6 +152,32 @@ of_pwm_xlate_with_flags(struct pwm_chip *pc, const struct of_phandle_args *args) } EXPORT_SYMBOL_GPL(of_pwm_xlate_with_flags);
+struct pwm_device * +of_pwm_single_xlate(struct pwm_chip *pc, const struct of_phandle_args *args) +{
- struct pwm_device *pwm;
- if (pc->of_pwm_n_cells < 1)
return ERR_PTR(-EINVAL);- /* validate that one cell is specified, optionally with flags */
- if (args->args_count != 1 && args->args_count != 2)
return ERR_PTR(-EINVAL);- pwm = pwm_request_from_chip(pc, 0, NULL);
- if (IS_ERR(pwm))
return pwm;- pwm->args.period = args->args[0];
- pwm->args.polarity = PWM_POLARITY_NORMAL;
- if (args->args_count == 2 && args->args[2] & PWM_POLARITY_INVERTED)
pwm->args.polarity = PWM_POLARITY_INVERSED;of_pwm_xlate_with_flags is a bit more complicated. Translating accordingly this would yield:
if (pc->of_pwm_n_cells >= 2) { if (args->args_count > 1 && args->args[1] & PWM_POLARITY_INVERTED) pwm->args.polarity = PWM_POLARITY_INVERSED; }
Given that pc->of_pwm_n_cells isn't used when a phandle is parsed (in of_pwm_get()) I think your variant is fine.
Right, the difference from of_pwm_xlate_with_flags is that this version will pick up the flags even if the driver says it has n_cells = 1.
I didn't see a strong reason for doing the extra check and the drawback with it is that if I then write in my dts that my channel should be INVERTED the driver won't be able to bump the n_cells to 2, because that would cause a regression.
Would you like me to add this extra check? Or perhaps ensure that the commit message captures my reasoning here?
So I think technically the patch is good, for me the question is if we want to make new drivers of_pwm_xlate_with_flags for consistency even though this would mean that the first argument has to be 0 for all phandles. Thierry? Lee?
I find it typical for single entity providers to be defined with #foo-cells = <0> (or 1 if you have flags) and not pass a "dummy" 0.
We did talk about this with Rob in a previous version of this patch and came to the conclusion that this was the appropriate thing to do...
Thanks, Bjorn
dri-devel@lists.freedesktop.org
-
Bjorn Andersson -
Uwe Kleine-König