* [PATCH v2 1/2] pwm: Introduce single-PWM of_xlate function @ 2021-06-15 23:18 Bjorn Andersson 2021-06-15 23:18 ` [PATCH v2 2/2] drm/bridge: ti-sn65dsi86: Implement the pwm_chip Bjorn Andersson 0 siblings, 1 reply; 9+ messages in thread From: Bjorn Andersson @ 2021-06-15 23:18 UTC (permalink / raw) To: Thierry Reding, Uwe Kleine-König, Lee Jones, Doug Anderson Cc: Andrzej Hajda, Neil Armstrong, Robert Foss, Laurent Pinchart, Jonas Karlman, Jernej Skrabec, David Airlie, Daniel Vetter, dri-devel, linux-kernel, linux-pwm 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 v1: - Pick up the optional flags cell as well 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 a42999f877d2..5e9c876fccc4 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 cfb683827d32..8cd82fb54483 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) pwm->chip.npwm = (id->driver_data & HAS_SECONDARY_PWM) ? 2 : 1; if (IS_ENABLED(CONFIG_OF)) { - pwm->chip.of_xlate = pxa_pwm_of_xlate; + pwm->chip.of_xlate = of_pwm_single_xlate; pwm->chip.of_pwm_n_cells = 1; } diff --git a/include/linux/pwm.h b/include/linux/pwm.h index 5a73251d28e3..6aff1fa4fe5d 100644 --- a/include/linux/pwm.h +++ b/include/linux/pwm.h @@ -411,6 +411,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, -- 2.31.0 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH v2 2/2] drm/bridge: ti-sn65dsi86: Implement the pwm_chip 2021-06-15 23:18 [PATCH v2 1/2] pwm: Introduce single-PWM of_xlate function Bjorn Andersson @ 2021-06-15 23:18 ` Bjorn Andersson 2021-06-16 7:56 ` Uwe Kleine-König 0 siblings, 1 reply; 9+ messages in thread From: Bjorn Andersson @ 2021-06-15 23:18 UTC (permalink / raw) To: Andrzej Hajda, Neil Armstrong, Robert Foss, Laurent Pinchart, Jonas Karlman, Jernej Skrabec, David Airlie, Daniel Vetter, Thierry Reding, Uwe Kleine-König, Lee Jones, Doug Anderson Cc: dri-devel, linux-kernel, linux-pwm 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 v1: - Rebased ontop of Doug's auxiliary_bus patches - Reworked the math, per Uwe's request - Added pwm_chip->get_state and made sure it's happy (only tested with a few limited periods, such as 1kHz) drivers/gpu/drm/bridge/ti-sn65dsi86.c | 298 +++++++++++++++++++++++++- 1 file changed, 297 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c index 5d712c8c3c3b..8f11c9b2da48 100644 --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c @@ -4,6 +4,7 @@ * datasheet: https://www.ti.com/lit/ds/symlink/sn65dsi86.pdf */ +#include <linux/atomic.h> #include <linux/auxiliary_bus.h> #include <linux/bits.h> #include <linux/clk.h> @@ -15,6 +16,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 +93,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 +122,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 +157,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_refclk_freq: Cache for the reference clock input to the PWM. + * @pwm_pin_busy: Track if GPIO4 is currently requested for GPIO or 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 +190,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; + unsigned int pwm_refclk_freq; + atomic_t pwm_pin_busy; +#endif }; static const struct regmap_range ti_sn65dsi86_volatile_ranges[] = { @@ -190,6 +214,25 @@ 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) { @@ -253,6 +296,14 @@ 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)); + +#if defined(CONFIG_PWM) + /* + * 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]; +#endif } static void ti_sn65dsi86_enable_comms(struct ti_sn65dsi86 *pdata) @@ -1044,6 +1095,221 @@ static int ti_sn_bridge_parse_dsi_host(struct ti_sn65dsi86 *pdata) return 0; } +#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); +} + +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; + int ret; + + if (!pdata->pwm_enabled) { + ret = pm_runtime_get_sync(pdata->dev); + if (ret < 0) + return ret; + + 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; + } + } + + if (state->enabled) { + /* + * Per the datasheet the PWM frequency is given by: + * + * PWM_FREQ = REFCLK_FREQ / (PWM_PRE_DIV * BACKLIGHT_SCALE + 1) + * + * which can be rewritten: + * + * T_pwm * REFCLK_FREQ - 1 = PWM_PRE_DIV * BACKLIGHT_SCALE + * + * In order to keep BACKLIGHT_SCALE within its 16 bits, PWM_PRE_DIV + * must be: + * + * PWM_PRE_DIV >= (T_pwm * REFCLK_FREQ - 1) / BACKLIGHT_SCALE_MAX; + * + * 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: + * + * BACKLIGHT_SCALE = (T_pwm * REFCLK_FREQ - 1) / PWM_PRE_DIV + * + * Here T_pwm is represented in seconds, so appropriate scaling to + * nanoseconds is necessary. + */ + pre_div = DIV_ROUND_UP((state->period * pdata->pwm_refclk_freq - 1), + (NSEC_PER_SEC * BACKLIGHT_SCALE_MAX)); + scale = (state->period * pdata->pwm_refclk_freq - 1) / (NSEC_PER_SEC * pre_div); + + /* + * The duty ratio is given as: + * + * duty = BACKLIGHT / (BACKLIGHT_SCALE + 1) + */ + backlight = state->duty_cycle * (scale + 1) / state->period; + + 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 = NSEC_PER_SEC * (pre_div * scale + 1) / pdata->pwm_refclk_freq; + state->duty_cycle = DIV_ROUND_UP(state->period * backlight, scale + 1); +} + +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.base = -1; + 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 + #if defined(CONFIG_OF_GPIO) static int tn_sn_bridge_of_xlate(struct gpio_chip *chip, @@ -1176,10 +1442,26 @@ 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] = { @@ -1201,6 +1483,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; @@ -1500,6 +1783,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 @@ -1543,10 +1832,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; @@ -1555,6 +1848,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: @@ -1568,6 +1863,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); } -- 2.31.0 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH v2 2/2] drm/bridge: ti-sn65dsi86: Implement the pwm_chip 2021-06-15 23:18 ` [PATCH v2 2/2] drm/bridge: ti-sn65dsi86: Implement the pwm_chip Bjorn Andersson @ 2021-06-16 7:56 ` Uwe Kleine-König 2021-06-17 3:22 ` Bjorn Andersson 0 siblings, 1 reply; 9+ messages in thread From: Uwe Kleine-König @ 2021-06-16 7:56 UTC (permalink / raw) To: Bjorn Andersson Cc: Andrzej Hajda, Neil Armstrong, Robert Foss, Laurent Pinchart, Jonas Karlman, Jernej Skrabec, David Airlie, Daniel Vetter, Thierry Reding, Lee Jones, Doug Anderson, dri-devel, linux-kernel, linux-pwm [-- Attachment #1: Type: text/plain, Size: 12822 bytes --] Hello Bjorn, On Tue, Jun 15, 2021 at 06:18:28PM -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 v1: > - Rebased ontop of Doug's auxiliary_bus patches > - Reworked the math, per Uwe's request > - Added pwm_chip->get_state and made sure it's happy (only tested with a few > limited periods, such as 1kHz) > > drivers/gpu/drm/bridge/ti-sn65dsi86.c | 298 +++++++++++++++++++++++++- > 1 file changed, 297 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c > index 5d712c8c3c3b..8f11c9b2da48 100644 > --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c > +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c > @@ -4,6 +4,7 @@ > * datasheet: https://www.ti.com/lit/ds/symlink/sn65dsi86.pdf > */ > > +#include <linux/atomic.h> > #include <linux/auxiliary_bus.h> > #include <linux/bits.h> > #include <linux/clk.h> > @@ -15,6 +16,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 +93,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 +122,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 +157,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_refclk_freq: Cache for the reference clock input to the PWM. > + * @pwm_pin_busy: Track if GPIO4 is currently requested for GPIO or 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 +190,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; > + unsigned int pwm_refclk_freq; > + atomic_t pwm_pin_busy; > +#endif > }; > > static const struct regmap_range ti_sn65dsi86_volatile_ranges[] = { > @@ -190,6 +214,25 @@ 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) > { > @@ -253,6 +296,14 @@ 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)); > + > +#if defined(CONFIG_PWM) > + /* > + * 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]; > +#endif > } > > static void ti_sn65dsi86_enable_comms(struct ti_sn65dsi86 *pdata) > @@ -1044,6 +1095,221 @@ static int ti_sn_bridge_parse_dsi_host(struct ti_sn65dsi86 *pdata) > return 0; > } > > +#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); > +} > + > +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; > + int ret; > + > + if (!pdata->pwm_enabled) { > + ret = pm_runtime_get_sync(pdata->dev); > + if (ret < 0) > + return ret; > + > + 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; > + } Do you need to do this even if state->enabled is false? Does this already modify the output pin? > + } > + > + if (state->enabled) { > + /* > + * Per the datasheet the PWM frequency is given by: > + * > + * PWM_FREQ = REFCLK_FREQ / (PWM_PRE_DIV * BACKLIGHT_SCALE + 1) > + * > + * which can be rewritten: > + * > + * T_pwm * REFCLK_FREQ - 1 = PWM_PRE_DIV * BACKLIGHT_SCALE > + * > + * In order to keep BACKLIGHT_SCALE within its 16 bits, PWM_PRE_DIV > + * must be: > + * > + * PWM_PRE_DIV >= (T_pwm * REFCLK_FREQ - 1) / BACKLIGHT_SCALE_MAX; > + * > + * 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: > + * > + * BACKLIGHT_SCALE = (T_pwm * REFCLK_FREQ - 1) / PWM_PRE_DIV > + * > + * Here T_pwm is represented in seconds, so appropriate scaling to > + * nanoseconds is necessary. > + */ Very nice. > + pre_div = DIV_ROUND_UP((state->period * pdata->pwm_refclk_freq - 1), > + (NSEC_PER_SEC * BACKLIGHT_SCALE_MAX)); if (pre_div > 0xffff) pre_div = 0xffff; is needed here. (Assuming 0xffff is the bigest valid value for PRE_DIV.) > + scale = (state->period * pdata->pwm_refclk_freq - 1) / (NSEC_PER_SEC * pre_div); There is something wrong here. Consider: pdata->pwm_refclk_freq = 3333334 state->period = 100000 state->duty_cycle = 600 then you calculate: pre_div = 1 scale = 333 which yields an actual period of 100199.98 ns. However you should get a period less or equal than the requested period. It took me some time to spot the problem: Only state->period * pdata->pwm_refclk_freq must be divided by NSEC_PER_SEC, but not the -1. So the right formula is: scale = (state->period * pdata->pwm_refclk_freq - NSEC_PER_SEC) / (NSEC_PER_SEC * pre_div); (but you have to pay attention, the dividend might be negative in this formula). > + /* > + * The duty ratio is given as: > + * > + * duty = BACKLIGHT / (BACKLIGHT_SCALE + 1) > + */ > + backlight = state->duty_cycle * (scale + 1) / state->period; Lets continue the above example with the fixed calculation. So we have: pdata->pwm_refclk_freq = 3333334 state->period = 100000 [ns] state->duty_cycle = 600 scale = 332 so the actually emitted period = 99899.98002000399 ns Now you calculate: backlight = 1 which yields an actual duty_cycle of 299.99994 ns, with backlight = 2 you would get an actual duty_cycle of 599.99988 ns, which is better. The culprit here is that you divide by state->period but instead should divide by the actual period. > + > + 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); How does the output behave between these register writes? Can it happen that it emits a wave for corresponding to (e.g.) the new pre_div value but the old scale and 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 = NSEC_PER_SEC * (pre_div * scale + 1) / pdata->pwm_refclk_freq; round up here please. Then applying the result of .get_state() is a noop (as it should be). > + state->duty_cycle = DIV_ROUND_UP(state->period * backlight, scale + 1); I find it surprising that the actual duty_cycle is: state->period * backlight ------------------------- scale + 1 pre_div * scale + 1 = ------------------- refclk * scale where scale occurs twice. Can you confirm this to be right? > +} > + > +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.base = -1; base shouldn't be set since f9a8ee8c8bcd (pwm: Always allocate PWM chip base ID dynamically) > + pdata->pchip.npwm = 1; > + pdata->pchip.of_xlate = of_pwm_single_xlate; > + pdata->pchip.of_pwm_n_cells = 1; > + > + return pwmchip_add(&pdata->pchip); > +} Best regards Uwe -- Pengutronix e.K. | Uwe Kleine-König | Industrial Linux Solutions | https://www.pengutronix.de/ | [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2 2/2] drm/bridge: ti-sn65dsi86: Implement the pwm_chip 2021-06-16 7:56 ` Uwe Kleine-König @ 2021-06-17 3:22 ` Bjorn Andersson 2021-06-17 6:24 ` Uwe Kleine-König 0 siblings, 1 reply; 9+ messages in thread From: Bjorn Andersson @ 2021-06-17 3:22 UTC (permalink / raw) To: Uwe Kleine-K?nig Cc: Andrzej Hajda, Neil Armstrong, Robert Foss, Laurent Pinchart, Jonas Karlman, Jernej Skrabec, David Airlie, Daniel Vetter, Thierry Reding, Lee Jones, Doug Anderson, dri-devel, linux-kernel, linux-pwm On Wed 16 Jun 02:56 CDT 2021, Uwe Kleine-K?nig wrote: > Hello Bjorn, > > On Tue, Jun 15, 2021 at 06:18:28PM -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 v1: > > - Rebased ontop of Doug's auxiliary_bus patches > > - Reworked the math, per Uwe's request > > - Added pwm_chip->get_state and made sure it's happy (only tested with a few > > limited periods, such as 1kHz) > > > > drivers/gpu/drm/bridge/ti-sn65dsi86.c | 298 +++++++++++++++++++++++++- > > 1 file changed, 297 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c > > index 5d712c8c3c3b..8f11c9b2da48 100644 > > --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c > > +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c > > @@ -4,6 +4,7 @@ > > * datasheet: https://www.ti.com/lit/ds/symlink/sn65dsi86.pdf > > */ > > > > +#include <linux/atomic.h> > > #include <linux/auxiliary_bus.h> > > #include <linux/bits.h> > > #include <linux/clk.h> > > @@ -15,6 +16,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 +93,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 +122,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 +157,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_refclk_freq: Cache for the reference clock input to the PWM. > > + * @pwm_pin_busy: Track if GPIO4 is currently requested for GPIO or 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 +190,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; > > + unsigned int pwm_refclk_freq; > > + atomic_t pwm_pin_busy; > > +#endif > > }; > > > > static const struct regmap_range ti_sn65dsi86_volatile_ranges[] = { > > @@ -190,6 +214,25 @@ 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) > > { > > @@ -253,6 +296,14 @@ 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)); > > + > > +#if defined(CONFIG_PWM) > > + /* > > + * 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]; > > +#endif > > } > > > > static void ti_sn65dsi86_enable_comms(struct ti_sn65dsi86 *pdata) > > @@ -1044,6 +1095,221 @@ static int ti_sn_bridge_parse_dsi_host(struct ti_sn65dsi86 *pdata) > > return 0; > > } > > > > +#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); > > +} > > + > > +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; > > + int ret; > > + > > + if (!pdata->pwm_enabled) { > > + ret = pm_runtime_get_sync(pdata->dev); > > + if (ret < 0) > > + return ret; > > + > > + 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; > > + } > > Do you need to do this even if state->enabled is false? I presume I should be able to explicitly mux in the GPIO function and configure that to output low. But I am not able to find anything in the data sheet that would indicate this to be preferred. > Does this already modify the output pin? > Yes, coming out of reset this pin is configured as input, so switching the mux here will effectively start driving the pin. > > + } > > + > > + if (state->enabled) { > > + /* > > + * Per the datasheet the PWM frequency is given by: > > + * > > + * PWM_FREQ = REFCLK_FREQ / (PWM_PRE_DIV * BACKLIGHT_SCALE + 1) > > + * > > + * which can be rewritten: > > + * > > + * T_pwm * REFCLK_FREQ - 1 = PWM_PRE_DIV * BACKLIGHT_SCALE > > + * > > + * In order to keep BACKLIGHT_SCALE within its 16 bits, PWM_PRE_DIV > > + * must be: > > + * > > + * PWM_PRE_DIV >= (T_pwm * REFCLK_FREQ - 1) / BACKLIGHT_SCALE_MAX; > > + * > > + * 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: > > + * > > + * BACKLIGHT_SCALE = (T_pwm * REFCLK_FREQ - 1) / PWM_PRE_DIV > > + * > > + * Here T_pwm is represented in seconds, so appropriate scaling to > > + * nanoseconds is necessary. > > + */ > > Very nice. > > > + pre_div = DIV_ROUND_UP((state->period * pdata->pwm_refclk_freq - 1), > > + (NSEC_PER_SEC * BACKLIGHT_SCALE_MAX)); > > if (pre_div > 0xffff) > pre_div = 0xffff; > > is needed here. (Assuming 0xffff is the bigest valid value for PRE_DIV.) > Yes, that makes sense. > > + scale = (state->period * pdata->pwm_refclk_freq - 1) / (NSEC_PER_SEC * pre_div); > > There is something wrong here. Consider: > > pdata->pwm_refclk_freq = 3333334 > state->period = 100000 > state->duty_cycle = 600 > > then you calculate: > > pre_div = 1 > scale = 333 > > which yields an actual period of 100199.98 ns. However you should get a > period less or equal than the requested period. > > It took me some time to spot the problem: Only state->period * > pdata->pwm_refclk_freq must be divided by NSEC_PER_SEC, but not the -1. > > So the right formula is: > > scale = (state->period * pdata->pwm_refclk_freq - NSEC_PER_SEC) / (NSEC_PER_SEC * pre_div); > Ahh, you're right! Thanks! > (but you have to pay attention, the dividend might be negative in this > formula). > Right and that defines the lower limit for the period, something I don't handle as this is currently written. > > + /* > > + * The duty ratio is given as: > > + * > > + * duty = BACKLIGHT / (BACKLIGHT_SCALE + 1) > > + */ > > + backlight = state->duty_cycle * (scale + 1) / state->period; > > Lets continue the above example with the fixed calculation. So we have: > > pdata->pwm_refclk_freq = 3333334 > state->period = 100000 [ns] > state->duty_cycle = 600 > scale = 332 > > so the actually emitted period = 99899.98002000399 ns > > Now you calculate: > > backlight = 1 > > which yields an actual duty_cycle of 299.99994 ns, with backlight = 2 > you would get an actual duty_cycle of 599.99988 ns, which is better. The > culprit here is that you divide by state->period but instead should > divide by the actual period. > What do I do about the case where the actual period is lower than the requested one and thereby the duty cycle becomes larger than the period? E.g. passing duty_cycle = period = 1,000,000 with a frequency of 3333334 results in a scale of 3332 and duty cycle (over the actual period) of 3333. > > + > > + 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); > > How does the output behave between these register writes? Can it happen > that it emits a wave for corresponding to (e.g.) the new pre_div value > but the old scale and backlight? > I don't see anything indicating in the documentation indicating that these writes would be buffered or similar. Unfortunately, as I said earlier I don't have any way to access the signal to see for myself. > > > + } > > + > > + 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 = NSEC_PER_SEC * (pre_div * scale + 1) / pdata->pwm_refclk_freq; > > round up here please. Then applying the result of .get_state() is > a noop (as it should be). > Together with the adjustment of the -1 above I can confirm that we get something that PWM_DEBUG is happy with (over a larger range of tests input than I previously tested...) > > + state->duty_cycle = DIV_ROUND_UP(state->period * backlight, scale + 1); > > I find it surprising that the actual duty_cycle is: > > state->period * backlight > ------------------------- > scale + 1 > > pre_div * scale + 1 > = ------------------- > refclk * scale > > where scale occurs twice. Can you confirm this to be right? > I came to the same conclusion - i.e. that this looks wrong. As states above, the period of the PWM is: pre_div * scale + 1 ------------------- refclk or: T_refclk * (pre_div * scale + 1) Which I interpret as us having two nested counters ticking based on refclk. Once we hit pre_div * scale the counter resets, which takes 1 refclk pulse. But then the duty cycle is described as: BACKLIGHT / (BACKLIGHT_SCALE + 1) which I would say looks like the signal is high pre_div * BACKLIGHT steps and then it resets at pre_div * (BACKLIGHT_SCALE + 1). So I don't know what's going on here. I will take another look tomorrow on why, but the including the + 1 in the denominator seems to be necessary to keep the duty cycle idempotent... > > +} > > + > > +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.base = -1; > > base shouldn't be set since > > f9a8ee8c8bcd (pwm: Always allocate PWM chip base ID dynamically) Thanks, that's nice! Many thanks for your feedback! Regards, Bjorn > > > + pdata->pchip.npwm = 1; > > + pdata->pchip.of_xlate = of_pwm_single_xlate; > > + pdata->pchip.of_pwm_n_cells = 1; > > + > > + return pwmchip_add(&pdata->pchip); > > +} > > Best regards > Uwe > > -- > Pengutronix e.K. | Uwe Kleine-König | > Industrial Linux Solutions | https://www.pengutronix.de/ | ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2 2/2] drm/bridge: ti-sn65dsi86: Implement the pwm_chip 2021-06-17 3:22 ` Bjorn Andersson @ 2021-06-17 6:24 ` Uwe Kleine-König 2021-06-17 16:38 ` Bjorn Andersson 0 siblings, 1 reply; 9+ messages in thread From: Uwe Kleine-König @ 2021-06-17 6:24 UTC (permalink / raw) To: Bjorn Andersson Cc: Andrzej Hajda, Neil Armstrong, Robert Foss, Laurent Pinchart, Jonas Karlman, Jernej Skrabec, David Airlie, Daniel Vetter, Thierry Reding, Lee Jones, Doug Anderson, dri-devel, linux-kernel, linux-pwm [-- Attachment #1: Type: text/plain, Size: 2853 bytes --] Hello Bjorn, On Wed, Jun 16, 2021 at 10:22:17PM -0500, Bjorn Andersson wrote: > > > +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; > > > + int ret; > > > + > > > + if (!pdata->pwm_enabled) { > > > + ret = pm_runtime_get_sync(pdata->dev); > > > + if (ret < 0) > > > + return ret; > > > + > > > + 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; > > > + } > > > > Do you need to do this even if state->enabled is false? > > I presume I should be able to explicitly mux in the GPIO function and > configure that to output low. But I am not able to find anything in the > data sheet that would indicate this to be preferred. My question targetted a different case. If the PWM is off (!pdata->pwm_enabled) and should remain off (state->enabled is false) you can shortcut here, can you not? > > Does this already modify the output pin? > > Yes, coming out of reset this pin is configured as input, so switching > the mux here will effectively start driving the pin. So please document this in the format the recently added drivers do, too. See e.g. drivers/pwm/pwm-sifive.c. (The idea is to start that with " * Limitations:" to make it easy to grep it.) > > Lets continue the above example with the fixed calculation. So we have: > > > > pdata->pwm_refclk_freq = 3333334 > > state->period = 100000 [ns] > > state->duty_cycle = 600 > > scale = 332 > > > > so the actually emitted period = 99899.98002000399 ns > > > > Now you calculate: > > > > backlight = 1 > > > > which yields an actual duty_cycle of 299.99994 ns, with backlight = 2 > > you would get an actual duty_cycle of 599.99988 ns, which is better. The > > culprit here is that you divide by state->period but instead should > > divide by the actual period. > > What do I do about the case where the actual period is lower than the > requested one and thereby the duty cycle becomes larger than the period? The general principle is: Pick the biggest possible duty_cycle available for the just picked period. So in your example you have to clamp it to period (assuming you can, otherwise pick the next lower possible value). Best regards Uwe -- Pengutronix e.K. | Uwe Kleine-König | Industrial Linux Solutions | https://www.pengutronix.de/ | [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2 2/2] drm/bridge: ti-sn65dsi86: Implement the pwm_chip 2021-06-17 6:24 ` Uwe Kleine-König @ 2021-06-17 16:38 ` Bjorn Andersson 2021-06-17 16:54 ` Uwe Kleine-König 0 siblings, 1 reply; 9+ messages in thread From: Bjorn Andersson @ 2021-06-17 16:38 UTC (permalink / raw) To: Uwe Kleine-K?nig Cc: Andrzej Hajda, Neil Armstrong, Robert Foss, Laurent Pinchart, Jonas Karlman, Jernej Skrabec, David Airlie, Daniel Vetter, Thierry Reding, Lee Jones, Doug Anderson, dri-devel, linux-kernel, linux-pwm On Thu 17 Jun 01:24 CDT 2021, Uwe Kleine-K?nig wrote: > Hello Bjorn, > > On Wed, Jun 16, 2021 at 10:22:17PM -0500, Bjorn Andersson wrote: > > > > +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; > > > > + int ret; > > > > + > > > > + if (!pdata->pwm_enabled) { > > > > + ret = pm_runtime_get_sync(pdata->dev); > > > > + if (ret < 0) > > > > + return ret; > > > > + > > > > + 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; > > > > + } > > > > > > Do you need to do this even if state->enabled is false? > > > > I presume I should be able to explicitly mux in the GPIO function and > > configure that to output low. But I am not able to find anything in the > > data sheet that would indicate this to be preferred. > > My question targetted a different case. If the PWM is off > (!pdata->pwm_enabled) and should remain off (state->enabled is false) > you can shortcut here, can you not? > Right, if we're going off->off then we don't need to touch the hardware. But am I expected to -EINVAL improper period and duty cycle even though enabled is false? And also, what is the supposed behavior of enabled = false? Is it supposedly equivalent of asking for a duty_cycle of 0? > > > Does this already modify the output pin? > > > > Yes, coming out of reset this pin is configured as input, so switching > > the mux here will effectively start driving the pin. > > So please document this in the format the recently added drivers do, > too. See e.g. drivers/pwm/pwm-sifive.c. (The idea is to start that with > " * Limitations:" to make it easy to grep it.) > Okay, will do. Although I believe that for this driver it makes sense to place such comment close to this function, rather than at the top of the driver. > > > Lets continue the above example with the fixed calculation. So we have: > > > > > > pdata->pwm_refclk_freq = 3333334 > > > state->period = 100000 [ns] > > > state->duty_cycle = 600 > > > scale = 332 > > > > > > so the actually emitted period = 99899.98002000399 ns > > > > > > Now you calculate: > > > > > > backlight = 1 > > > > > > which yields an actual duty_cycle of 299.99994 ns, with backlight = 2 > > > you would get an actual duty_cycle of 599.99988 ns, which is better. The > > > culprit here is that you divide by state->period but instead should > > > divide by the actual period. > > > > What do I do about the case where the actual period is lower than the > > requested one and thereby the duty cycle becomes larger than the period? > > The general principle is: Pick the biggest possible duty_cycle available > for the just picked period. So in your example you have to clamp it to > period (assuming you can, otherwise pick the next lower possible value). > Sounds good. Thank you, Bjorn > Best regards > Uwe > > -- > Pengutronix e.K. | Uwe Kleine-König | > Industrial Linux Solutions | https://www.pengutronix.de/ | ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2 2/2] drm/bridge: ti-sn65dsi86: Implement the pwm_chip 2021-06-17 16:38 ` Bjorn Andersson @ 2021-06-17 16:54 ` Uwe Kleine-König 2021-06-17 18:06 ` Bjorn Andersson 0 siblings, 1 reply; 9+ messages in thread From: Uwe Kleine-König @ 2021-06-17 16:54 UTC (permalink / raw) To: Bjorn Andersson Cc: Andrzej Hajda, Neil Armstrong, Robert Foss, Laurent Pinchart, Jonas Karlman, Jernej Skrabec, David Airlie, Daniel Vetter, Thierry Reding, Lee Jones, Doug Anderson, dri-devel, linux-kernel, linux-pwm [-- Attachment #1: Type: text/plain, Size: 3045 bytes --] Hello Bjorn, On Thu, Jun 17, 2021 at 11:38:26AM -0500, Bjorn Andersson wrote: > On Thu 17 Jun 01:24 CDT 2021, Uwe Kleine-K?nig wrote: > > On Wed, Jun 16, 2021 at 10:22:17PM -0500, Bjorn Andersson wrote: > > > > > +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; > > > > > + int ret; > > > > > + > > > > > + if (!pdata->pwm_enabled) { > > > > > + ret = pm_runtime_get_sync(pdata->dev); > > > > > + if (ret < 0) > > > > > + return ret; > > > > > + > > > > > + 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; > > > > > + } > > > > > > > > Do you need to do this even if state->enabled is false? > > > > > > I presume I should be able to explicitly mux in the GPIO function and > > > configure that to output low. But I am not able to find anything in the > > > data sheet that would indicate this to be preferred. > > > > My question targetted a different case. If the PWM is off > > (!pdata->pwm_enabled) and should remain off (state->enabled is false) > > you can shortcut here, can you not? > > Right, if we're going off->off then we don't need to touch the hardware. > > But am I expected to -EINVAL improper period and duty cycle even though > enabled is false? > > And also, what is the supposed behavior of enabled = false? Is it > supposedly equivalent of asking for a duty_cycle of 0? In my book enabled = false is just syntactic sugar to say: "duty_cycle=0, period=something small". So to answer your questions: if enabled = false, the consumer doesn't really care about period and duty_cycle. Some care that the output becomes inactive, some others don't, so from my POV just emit the inactive level on the output and ignore period and duty_cycle. > > > > Does this already modify the output pin? > > > > > > Yes, coming out of reset this pin is configured as input, so switching > > > the mux here will effectively start driving the pin. > > > > So please document this in the format the recently added drivers do, > > too. See e.g. drivers/pwm/pwm-sifive.c. (The idea is to start that with > > " * Limitations:" to make it easy to grep it.) > > > > Okay, will do. Although I believe that for this driver it makes sense to > place such comment close to this function, rather than at the top of the > driver. Yes, agreed. Best regards Uwe -- Pengutronix e.K. | Uwe Kleine-König | Industrial Linux Solutions | https://www.pengutronix.de/ | [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2 2/2] drm/bridge: ti-sn65dsi86: Implement the pwm_chip 2021-06-17 16:54 ` Uwe Kleine-König @ 2021-06-17 18:06 ` Bjorn Andersson 2021-06-18 7:46 ` Uwe Kleine-König 0 siblings, 1 reply; 9+ messages in thread From: Bjorn Andersson @ 2021-06-17 18:06 UTC (permalink / raw) To: Uwe Kleine-K?nig Cc: Andrzej Hajda, Neil Armstrong, Robert Foss, Laurent Pinchart, Jonas Karlman, Jernej Skrabec, David Airlie, Daniel Vetter, Thierry Reding, Lee Jones, Doug Anderson, dri-devel, linux-kernel, linux-pwm On Thu 17 Jun 11:54 CDT 2021, Uwe Kleine-K?nig wrote: > Hello Bjorn, > > On Thu, Jun 17, 2021 at 11:38:26AM -0500, Bjorn Andersson wrote: > > On Thu 17 Jun 01:24 CDT 2021, Uwe Kleine-K?nig wrote: > > > On Wed, Jun 16, 2021 at 10:22:17PM -0500, Bjorn Andersson wrote: > > > > > > +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; > > > > > > + int ret; > > > > > > + > > > > > > + if (!pdata->pwm_enabled) { > > > > > > + ret = pm_runtime_get_sync(pdata->dev); > > > > > > + if (ret < 0) > > > > > > + return ret; > > > > > > + > > > > > > + 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; > > > > > > + } > > > > > > > > > > Do you need to do this even if state->enabled is false? > > > > > > > > I presume I should be able to explicitly mux in the GPIO function and > > > > configure that to output low. But I am not able to find anything in the > > > > data sheet that would indicate this to be preferred. > > > > > > My question targetted a different case. If the PWM is off > > > (!pdata->pwm_enabled) and should remain off (state->enabled is false) > > > you can shortcut here, can you not? > > > > Right, if we're going off->off then we don't need to touch the hardware. > > > > But am I expected to -EINVAL improper period and duty cycle even though > > enabled is false? > > > > And also, what is the supposed behavior of enabled = false? Is it > > supposedly equivalent of asking for a duty_cycle of 0? > > In my book enabled = false is just syntactic sugar to say: > "duty_cycle=0, period=something small". So to answer your questions: if > enabled = false, the consumer doesn't really care about period and > duty_cycle. Some care that the output becomes inactive, some others > don't, so from my POV just emit the inactive level on the output and > ignore period and duty_cycle. > Giving this some further thought. In order to have a known state of the PWM signal we need the sn65dsi86 to be powered. The documentation describes a "suspend mode", but this is currently not implemented in the driver, so there's a large power cost coming from just keeping the pin low when disabled. As such in the current implementation I use state->enabled to also control if the device should be kept powered, which means that this follows the backlight power status of the pwm-backlight. Which is sufficient as the backlight won't be powered when !state->enabled. For the typical use case the pwm-backlight has some independent control over gpio and power to toggle the actual backlight. But in the even that this wouldn't be available I think we need to extend the driver to implement the suspend mode. In which case muxing in the PWM function should probably happen at the time the PWM channel is requested. This does come at an additional power cost (not as high as keeping the chip awake though), so (in addition to the effort) I think it's reasonable to document this as a limitation for now and implement this as the need arise. Thanks, Bjorn > > > > > Does this already modify the output pin? > > > > > > > > Yes, coming out of reset this pin is configured as input, so switching > > > > the mux here will effectively start driving the pin. > > > > > > So please document this in the format the recently added drivers do, > > > too. See e.g. drivers/pwm/pwm-sifive.c. (The idea is to start that with > > > " * Limitations:" to make it easy to grep it.) > > > > > > > Okay, will do. Although I believe that for this driver it makes sense to > > place such comment close to this function, rather than at the top of the > > driver. > > Yes, agreed. > > Best regards > Uwe > > -- > Pengutronix e.K. | Uwe Kleine-König | > Industrial Linux Solutions | https://www.pengutronix.de/ | ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2 2/2] drm/bridge: ti-sn65dsi86: Implement the pwm_chip 2021-06-17 18:06 ` Bjorn Andersson @ 2021-06-18 7:46 ` Uwe Kleine-König 0 siblings, 0 replies; 9+ messages in thread From: Uwe Kleine-König @ 2021-06-18 7:46 UTC (permalink / raw) To: Bjorn Andersson Cc: Andrzej Hajda, Neil Armstrong, Robert Foss, Laurent Pinchart, Jonas Karlman, Jernej Skrabec, David Airlie, Daniel Vetter, Thierry Reding, Lee Jones, Doug Anderson, dri-devel, linux-kernel, linux-pwm [-- Attachment #1: Type: text/plain, Size: 5483 bytes --] Hello Bjorn, On Thu, Jun 17, 2021 at 01:06:51PM -0500, Bjorn Andersson wrote: > On Thu 17 Jun 11:54 CDT 2021, Uwe Kleine-K?nig wrote: > > On Thu, Jun 17, 2021 at 11:38:26AM -0500, Bjorn Andersson wrote: > > > On Thu 17 Jun 01:24 CDT 2021, Uwe Kleine-K?nig wrote: > > > > On Wed, Jun 16, 2021 at 10:22:17PM -0500, Bjorn Andersson wrote: > > > > > > > +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; > > > > > > > + int ret; > > > > > > > + > > > > > > > + if (!pdata->pwm_enabled) { > > > > > > > + ret = pm_runtime_get_sync(pdata->dev); > > > > > > > + if (ret < 0) > > > > > > > + return ret; > > > > > > > + > > > > > > > + 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; > > > > > > > + } > > > > > > > > > > > > Do you need to do this even if state->enabled is false? > > > > > > > > > > I presume I should be able to explicitly mux in the GPIO function and > > > > > configure that to output low. But I am not able to find anything in the > > > > > data sheet that would indicate this to be preferred. > > > > > > > > My question targetted a different case. If the PWM is off > > > > (!pdata->pwm_enabled) and should remain off (state->enabled is false) > > > > you can shortcut here, can you not? > > > > > > Right, if we're going off->off then we don't need to touch the hardware. > > > > > > But am I expected to -EINVAL improper period and duty cycle even though > > > enabled is false? > > > > > > And also, what is the supposed behavior of enabled = false? Is it > > > supposedly equivalent of asking for a duty_cycle of 0? > > > > In my book enabled = false is just syntactic sugar to say: > > "duty_cycle=0, period=something small". So to answer your questions: if > > enabled = false, the consumer doesn't really care about period and > > duty_cycle. Some care that the output becomes inactive, some others > > don't, so from my POV just emit the inactive level on the output and > > ignore period and duty_cycle. > > Giving this some further thought. Very appreciated, still more as you come to the same conclusions as I do :-) > In order to have a known state of the PWM signal we need the sn65dsi86 > to be powered. The documentation describes a "suspend mode", but this is > currently not implemented in the driver, so there's a large power cost > coming from just keeping the pin low when disabled. In the past I already promoted the idea that a disabled PWM should give no guarantees about the output level. In fact there are several offenders, the ones I know off-hand are: - pwm-imx27 emits a low level independent of the programmed polarity - pwm-iqs620a makes the output tristated and so relies on an external pull to give the inactive level. - pwm-sl28cpld switches to a GPIO mode on disable which isn't controlled by the driver and I assume there are more because before no one actively asked for and tracked this kind of information. IMHO a consumer who wants the output to stay inactive should configure .enabled = true .period = DC (or something low to allow quick reprogramming) .duty_cycle = 0 , so there is no loss of functionality and enabled=false should mean the consumer doesn't care about the output which would allow some lowlevel drivers to save some more energy. So this makes the API more expressive because after dropping "disabled results in an inactive output" consumers can differentiate between "I care about the output staying inactive" and "I don't care". This in turn allows lowlevel drivers to better know when they can more aggressively save power and when they don't. Back then Thierry didn't like that approach though. (The thread started with a mail having Message-Id 20180820144357.7206-1-u.kleine-koenig@pengutronix.de, this is missing on lore.kernel.org however and I didn't find it on another mirror.) Thierry's arguments were: - "An API whose result is an undefined state is useless in my opinion." (from Message-Id: 20181009073407.GA5565@ulmo) Yes, this is a drawback for some consumers, but it matches reality that disabling the PWM counter on some PWM implementations doesn't result in an inactive level. And if they care about the output, they just use .duty_cycle = 0 + .enabled = true instead. In my book changing the semantic fixes a bug because the API promises more than some drivers are capable to do (with reasonable effort). - "[Emitting the inactive level] also matches what all other drivers, and users, assume." (also from Message-Id: 20181009073407.GA5565@ulmo) For the drivers this was an assumption, today we know it's wrong. Users can be fixed. Best regards Uwe -- Pengutronix e.K. | Uwe Kleine-König | Industrial Linux Solutions | https://www.pengutronix.de/ | [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2021-06-18 7:46 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-06-15 23:18 [PATCH v2 1/2] pwm: Introduce single-PWM of_xlate function Bjorn Andersson 2021-06-15 23:18 ` [PATCH v2 2/2] drm/bridge: ti-sn65dsi86: Implement the pwm_chip Bjorn Andersson 2021-06-16 7:56 ` Uwe Kleine-König 2021-06-17 3:22 ` Bjorn Andersson 2021-06-17 6:24 ` Uwe Kleine-König 2021-06-17 16:38 ` Bjorn Andersson 2021-06-17 16:54 ` Uwe Kleine-König 2021-06-17 18:06 ` Bjorn Andersson 2021-06-18 7:46 ` Uwe Kleine-König
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).