[PATCH v3 1/2] pwm: Introduce single-PWM of_xlate function

[PATCH v3 1/2] pwm: Introduce single-PWM of_xlate function

[Bjorn Andersson]

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 v2:
- 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 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

[PATCH v3 2/2] drm/bridge: ti-sn65dsi86: Implement the pwm_chip

[Bjorn Andersson]

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 v2:
- Corrected calculation of scale, to include a 1 instead of 1/NSEC_TO_SEC and
  rounded the period up in get_state, to make sure its idempotent
- Changed duty_cycle calculation to make sure it idempotent over my tested period
- Documented "Limitations"
- Documented muxing operation after pm_runtime_get_sync()

 drivers/gpu/drm/bridge/ti-sn65dsi86.c | 335 +++++++++++++++++++++++++-
 1 file changed, 334 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
index 5d712c8c3c3b..0eabbdad1830 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,258 @@ 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);
+}
+
+/*
+ * 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 keep
+ *   the determine if the chip needs to be kept powered.
+ * - Changing both period and duty_cycle is not done atomically, so the output
+ *   might briefly be a mix of the two settings.
+ */
+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 tick;
+	int ret;
+
+	if (!pdata->pwm_enabled) {
+		ret = pm_runtime_get_sync(pdata->dev);
+		if (ret < 0)
+			return ret;
+
+		/*
+		 * 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;
+		}
+	}
+
+	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.
+		 */
+
+		/* Minimum T_pwm is (1 * 1 + 1) / REFCLK_FREQ */
+		if (state->period * pdata->pwm_refclk_freq <= 2 * NSEC_PER_SEC) {
+			ret = -EINVAL;
+			goto out;
+		}
+
+		pre_div = DIV_ROUND_UP((state->period * pdata->pwm_refclk_freq - NSEC_PER_SEC),
+				       (NSEC_PER_SEC * BACKLIGHT_SCALE_MAX));
+		if (pre_div > 0xff)
+			pre_div = 0xff;
+
+		scale = (state->period * pdata->pwm_refclk_freq - NSEC_PER_SEC) / (NSEC_PER_SEC * pre_div);
+
+		/*
+		 * PWM duty cycle is given as:
+		 *
+		 *   duty = BACKLIGHT / (BACKLIGHT_SCALE + 1)
+		 *
+		 * The documentation is however inconsistent in its examples,
+		 * so the interpretation used here is that the duty cycle is
+		 * the period of BACKLIGHT * PRE_DIV / REFCLK_FREQ.
+		 *
+		 * The ratio PRE_DIV / REFCLK_FREQ is rounded up to whole
+		 * nanoseconds in order to ensure that the calculations are
+		 * idempotent and gives results that are smaller than the
+		 * requested value.
+		 */
+		tick = DIV_ROUND_UP(NSEC_PER_SEC * pre_div, pdata->pwm_refclk_freq);
+		backlight = state->duty_cycle / tick;
+		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(NSEC_PER_SEC * (pre_div * scale + 1), pdata->pwm_refclk_freq);
+	state->duty_cycle = backlight * DIV_ROUND_UP(NSEC_PER_SEC * pre_div, 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
+
 #if defined(CONFIG_OF_GPIO)
 
 static int tn_sn_bridge_of_xlate(struct gpio_chip *chip,
@@ -1176,10 +1479,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 +1520,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 +1820,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 +1869,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 +1885,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 +1900,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

Re: [PATCH v3 2/2] drm/bridge: ti-sn65dsi86: Implement the pwm_chip

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 12971 bytes --]

On Mon, Jun 21, 2021 at 10:09:48PM -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 v2:
> - Corrected calculation of scale, to include a 1 instead of 1/NSEC_TO_SEC and
>   rounded the period up in get_state, to make sure its idempotent
> - Changed duty_cycle calculation to make sure it idempotent over my tested period
> - Documented "Limitations"
> - Documented muxing operation after pm_runtime_get_sync()
> 
>  drivers/gpu/drm/bridge/ti-sn65dsi86.c | 335 +++++++++++++++++++++++++-
>  1 file changed, 334 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> index 5d712c8c3c3b..0eabbdad1830 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,258 @@ 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);
> +}
> +
> +/*
> + * 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 keep
> + *   the determine if the chip needs to be kept powered.
> + * - Changing both period and duty_cycle is not done atomically, so the output
> + *   might briefly be a mix of the two settings.
> + */
> +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 tick;
> +	int ret;
> +
> +	if (!pdata->pwm_enabled) {
> +		ret = pm_runtime_get_sync(pdata->dev);
> +		if (ret < 0)
> +			return ret;
> +
> +		/*
> +		 * 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;
> +		}

In reply to your v2 I requested to short-cut the case !pdata->pwm_enabled
&& !state->enabled without enabling stuff.

> +	}
> +
> +	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.
> +		 */
> +
> +		/* Minimum T_pwm is (1 * 1 + 1) / REFCLK_FREQ */
> +		if (state->period * pdata->pwm_refclk_freq <= 2 * NSEC_PER_SEC) {
> +			ret = -EINVAL;
> +			goto out;
> +		}
> +
> +		pre_div = DIV_ROUND_UP((state->period * pdata->pwm_refclk_freq - NSEC_PER_SEC),
> +				       (NSEC_PER_SEC * BACKLIGHT_SCALE_MAX));
> +		if (pre_div > 0xff)
> +			pre_div = 0xff;
> +
> +		scale = (state->period * pdata->pwm_refclk_freq - NSEC_PER_SEC) / (NSEC_PER_SEC * pre_div);

Please consider this multiplication to overflow. Something like:

	if (state->period > $someterm)
		period = $someterm;
	else
		period = state->period;

is usually appropriate. Also NSEC_PER_SEC * pre_div might overflow.
Moreover to divide a u64 you must not rely on / but need do_div() or
some variant of it.

> +
> +		/*
> +		 * PWM duty cycle is given as:
> +		 *
> +		 *   duty = BACKLIGHT / (BACKLIGHT_SCALE + 1)
> +		 *
> +		 * The documentation is however inconsistent in its examples,
> +		 * so the interpretation used here is that the duty cycle is
> +		 * the period of BACKLIGHT * PRE_DIV / REFCLK_FREQ.

I don't understand this.

> +		 *
> +		 * The ratio PRE_DIV / REFCLK_FREQ is rounded up to whole
> +		 * nanoseconds in order to ensure that the calculations are
> +		 * idempotent and gives results that are smaller than the
> +		 * requested value.
> +		 */
> +		tick = DIV_ROUND_UP(NSEC_PER_SEC * pre_div, pdata->pwm_refclk_freq);
> +		backlight = state->duty_cycle / tick;

You're loosing precision here by dividing by the result of a division.

> +		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(NSEC_PER_SEC * (pre_div * scale + 1), pdata->pwm_refclk_freq);
> +	state->duty_cycle = backlight * DIV_ROUND_UP(NSEC_PER_SEC * pre_div, pdata->pwm_refclk_freq);

If you use

	state->duty_cycle = DIV_ROUND_UP(backlight * NSEC_PER_SEC * pre_div, pdata->pwm_refclk_freq);

instead (with a cast to u64 to not yield an overflow) the result is more
exact.

I still find this surprising, I'd expect that SCALE also matters for the
duty_cycle. With the assumption implemented here modifying SCALE only
affects the period. This should be easy to verify?! I would expect that
changing SCALE doesn't affect the relative duty_cycle, so the brightness
of an LED is unaffected (unless the period gets too big of course).

I didn't spend much cycles to verify that the logic in .apply() matches
.get_state(). I'd keep that check for the next iteration.

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 --]

Re: [PATCH v3 2/2] drm/bridge: ti-sn65dsi86: Implement the pwm_chip

[Bjorn Andersson]

On Tue 22 Jun 15:29 CDT 2021, Uwe Kleine-K?nig wrote:

> On Mon, Jun 21, 2021 at 10:09:48PM -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 v2:
> > - Corrected calculation of scale, to include a 1 instead of 1/NSEC_TO_SEC and
> >   rounded the period up in get_state, to make sure its idempotent
> > - Changed duty_cycle calculation to make sure it idempotent over my tested period
> > - Documented "Limitations"
> > - Documented muxing operation after pm_runtime_get_sync()
> > 
> >  drivers/gpu/drm/bridge/ti-sn65dsi86.c | 335 +++++++++++++++++++++++++-
> >  1 file changed, 334 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> > index 5d712c8c3c3b..0eabbdad1830 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,258 @@ 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);
> > +}
> > +
> > +/*
> > + * 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 keep
> > + *   the determine if the chip needs to be kept powered.
> > + * - Changing both period and duty_cycle is not done atomically, so the output
> > + *   might briefly be a mix of the two settings.
> > + */
> > +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 tick;
> > +	int ret;
> > +
> > +	if (!pdata->pwm_enabled) {
> > +		ret = pm_runtime_get_sync(pdata->dev);
> > +		if (ret < 0)
> > +			return ret;
> > +
> > +		/*
> > +		 * 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;
> > +		}
> 
> In reply to your v2 I requested to short-cut the case !pdata->pwm_enabled
> && !state->enabled without enabling stuff.
> 

You're right, got lost in my thoughts about the power state. But per my
own reasoning there's no need enable the mux when state->enabled = false

> > +	}
> > +
> > +	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.
> > +		 */
> > +
> > +		/* Minimum T_pwm is (1 * 1 + 1) / REFCLK_FREQ */
> > +		if (state->period * pdata->pwm_refclk_freq <= 2 * NSEC_PER_SEC) {
> > +			ret = -EINVAL;
> > +			goto out;
> > +		}
> > +
> > +		pre_div = DIV_ROUND_UP((state->period * pdata->pwm_refclk_freq - NSEC_PER_SEC),
> > +				       (NSEC_PER_SEC * BACKLIGHT_SCALE_MAX));
> > +		if (pre_div > 0xff)
> > +			pre_div = 0xff;
> > +
> > +		scale = (state->period * pdata->pwm_refclk_freq - NSEC_PER_SEC) / (NSEC_PER_SEC * pre_div);
> 
> Please consider this multiplication to overflow. Something like:
> 
> 	if (state->period > $someterm)
> 		period = $someterm;
> 	else
> 		period = state->period;
> 
> is usually appropriate. Also NSEC_PER_SEC * pre_div might overflow.
> Moreover to divide a u64 you must not rely on / but need do_div() or
> some variant of it.
> 

Didn't consider that someone would ask for a period of 430 second, but
that sounds reasonable and you're right the denominator might be only 4
bytes on some architectures.

Will fix this, and ensure that things are promoted to 64 bits where
needed and divided appropriately.

> > +
> > +		/*
> > +		 * PWM duty cycle is given as:
> > +		 *
> > +		 *   duty = BACKLIGHT / (BACKLIGHT_SCALE + 1)
> > +		 *
> > +		 * The documentation is however inconsistent in its examples,
> > +		 * so the interpretation used here is that the duty cycle is
> > +		 * the period of BACKLIGHT * PRE_DIV / REFCLK_FREQ.
> 
> I don't understand this.
> 
> > +		 *
> > +		 * The ratio PRE_DIV / REFCLK_FREQ is rounded up to whole
> > +		 * nanoseconds in order to ensure that the calculations are
> > +		 * idempotent and gives results that are smaller than the
> > +		 * requested value.
> > +		 */
> > +		tick = DIV_ROUND_UP(NSEC_PER_SEC * pre_div, pdata->pwm_refclk_freq);
> > +		backlight = state->duty_cycle / tick;
> 
> You're loosing precision here by dividing by the result of a division.
> 

The actual period is also a result of a division and after spending too
many hours scratching my head I reached to conclusion that this was the
reason why I wasn't able to get the duty cycle calculation idempotent
over the ranges I tested.

But in my effort to describe this to you here, I finally spotted the
error and will follow up with a new version that does:

                actual = NSEC_PER_SEC * (pre_div * scale + 1) / pdata->pwm_refclk_freq);
                backlight = state->duty_cycle * (scale + 1) / actual;

(With appropriate u64 casts and divisions)

> > +		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(NSEC_PER_SEC * (pre_div * scale + 1), pdata->pwm_refclk_freq);
> > +	state->duty_cycle = backlight * DIV_ROUND_UP(NSEC_PER_SEC * pre_div, pdata->pwm_refclk_freq);
> 
> If you use
> 
> 	state->duty_cycle = DIV_ROUND_UP(backlight * NSEC_PER_SEC * pre_div, pdata->pwm_refclk_freq);
> 
> instead (with a cast to u64 to not yield an overflow) the result is more
> exact.
> 

The problem with this is that it sometimes yields duty_cycles larger
than what was requested... But going back to describing this as a ratio
of the period this becomes:

        state->duty_cycle = DIV_ROUND_UP_ULL(state->period * backlight, scale + 1);

> I still find this surprising, I'd expect that SCALE also matters for the
> duty_cycle. With the assumption implemented here modifying SCALE only
> affects the period. This should be easy to verify?! I would expect that
> changing SCALE doesn't affect the relative duty_cycle, so the brightness
> of an LED is unaffected (unless the period gets too big of course).
> 

I think the hardware is two nested counters, one (A) ticking at REFCLK_FREQ
and as that hits PRE_DIV, it kicks the second counter (B) (and resets).

As counter A is reset the output signal goes high, when A hits BACKLIGHT the
signal goes low and when A hits BACKLIGHT_SCALE it resets.

Per this implementation the actual length of the duty cycle would indeed
be independent of the BACKLIGHT_SCALE, but the length of the low signal
(and hence the ratio, which results in the actual brightness) does
depend on BACKLIGHT_SCALE.

> I didn't spend much cycles to verify that the logic in .apply() matches
> .get_state(). I'd keep that check for the next iteration.
> 

Your feedback is much appreciated!

Regards,
Bjorn

> Best regards
> Uwe
> 
> -- 
> Pengutronix e.K.                           | Uwe Kleine-König            |
> Industrial Linux Solutions                 | https://www.pengutronix.de/ |

Re: [PATCH v3 2/2] drm/bridge: ti-sn65dsi86: Implement the pwm_chip

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 5970 bytes --]

Hello Bjorn,

On Tue, Jun 22, 2021 at 08:12:48PM -0500, Bjorn Andersson wrote:
> On Tue 22 Jun 15:29 CDT 2021, Uwe Kleine-K?nig wrote:
> > On Mon, Jun 21, 2021 at 10:09:48PM -0500, Bjorn Andersson wrote:
> > > +		/*
> > > +		 * PWM duty cycle is given as:
> > > +		 *
> > > +		 *   duty = BACKLIGHT / (BACKLIGHT_SCALE + 1)
> > > +		 *
> > > +		 * The documentation is however inconsistent in its examples,
> > > +		 * so the interpretation used here is that the duty cycle is
> > > +		 * the period of BACKLIGHT * PRE_DIV / REFCLK_FREQ.
> > 
> > I don't understand this.
> > 
> > > +		 *
> > > +		 * The ratio PRE_DIV / REFCLK_FREQ is rounded up to whole
> > > +		 * nanoseconds in order to ensure that the calculations are
> > > +		 * idempotent and gives results that are smaller than the
> > > +		 * requested value.
> > > +		 */
> > > +		tick = DIV_ROUND_UP(NSEC_PER_SEC * pre_div, pdata->pwm_refclk_freq);
> > > +		backlight = state->duty_cycle / tick;
> > 
> > You're loosing precision here by dividing by the result of a division.
> 
> The actual period is also a result of a division and after spending too
> many hours scratching my head I reached to conclusion that this was the
> reason why I wasn't able to get the duty cycle calculation idempotent
> over the ranges I tested.

How did you test? Using the sysfs interface?
 
> But in my effort to describe this to you here, I finally spotted the
> error and will follow up with a new version that does:
> 
>                 actual = NSEC_PER_SEC * (pre_div * scale + 1) / pdata->pwm_refclk_freq);
>                 backlight = state->duty_cycle * (scale + 1) / actual;

So the first term ("actual") is the period that you get for a given
pre_div, scale and pwm_refclk_freq, right? And the 2nd ("backlight")
defines the register value to configure the duty_cycle, right?

I wonder: Is it possible to configure a 100% relative duty cycle? Then
backlight would be scale + 1 which (at least if scale is 0xffff) would
overflow the 16 bit register width?!

> > > +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(NSEC_PER_SEC * (pre_div * scale + 1), pdata->pwm_refclk_freq);
> > > +	state->duty_cycle = backlight * DIV_ROUND_UP(NSEC_PER_SEC * pre_div, pdata->pwm_refclk_freq);
> > 
> > If you use
> > 
> > 	state->duty_cycle = DIV_ROUND_UP(backlight * NSEC_PER_SEC * pre_div, pdata->pwm_refclk_freq);
> > 
> > instead (with a cast to u64 to not yield an overflow) the result is more
> > exact.
> > 
> 
> The problem with this is that it sometimes yields duty_cycles larger
> than what was requested... But going back to describing this as a ratio
> of the period this becomes:
> 
>         state->duty_cycle = DIV_ROUND_UP_ULL(state->period * backlight, scale + 1);

I saw your next iteration of this patch set, but didn't look into it
yet. Note that if it uses this formula it sill looses precision.
Consider:

	pwm_refclk_freq = 1333333
	pre_div = 4
	scale = 60000
	backlight = 59999

then you calculate:

	state->period = 180000796 (exact value: 180000795.00019875)
	state->duty_cycle = 179994797 (exact value: 179994795.0736975)

so duty_cycle should actually be reported as 179994796. That happens
because state->period is already the result of a division, you get the
right value when doing:

	state->duty_cycle = round_up(NSEC_PER_SEC * (pre_div * scale + 1) * backlight, (scale + 1) * pdata->pwm_refclk_freq)

> > I still find this surprising, I'd expect that SCALE also matters for the
> > duty_cycle. With the assumption implemented here modifying SCALE only
> > affects the period. This should be easy to verify?! I would expect that
> > changing SCALE doesn't affect the relative duty_cycle, so the brightness
> > of an LED is unaffected (unless the period gets too big of course).
> > 
> 
> I think the hardware is two nested counters, one (A) ticking at REFCLK_FREQ
> and as that hits PRE_DIV, it kicks the second counter (B) (and resets).
> 
> As counter A is reset the output signal goes high, when A hits BACKLIGHT the
> signal goes low and when A hits BACKLIGHT_SCALE it resets.

then we would probably have:

	period = (scale + 1) * pre_div / refclk

but not

	period = (scale * pre_div + 1) / refclk

. The former would be nicer because then in the calculation for
duty_cycle the factor (scale + 1) would cancel.

> Per this implementation the actual length of the duty cycle would indeed
> be independent of the BACKLIGHT_SCALE,

In your formula for duty_cycle scale actually does matter. So I think
we're not there yet?

> but the length of the low signal (and hence the ratio, which results
> in the actual brightness) does depend on BACKLIGHT_SCALE.

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 --]

Re: [PATCH v3 2/2] drm/bridge: ti-sn65dsi86: Implement the pwm_chip

[Bjorn Andersson]

On Wed 23 Jun 03:29 CDT 2021, Uwe Kleine-K?nig wrote:

> Hello Bjorn,
> 
> On Tue, Jun 22, 2021 at 08:12:48PM -0500, Bjorn Andersson wrote:
> > On Tue 22 Jun 15:29 CDT 2021, Uwe Kleine-K?nig wrote:
> > > On Mon, Jun 21, 2021 at 10:09:48PM -0500, Bjorn Andersson wrote:
> > > > +		/*
> > > > +		 * PWM duty cycle is given as:
> > > > +		 *
> > > > +		 *   duty = BACKLIGHT / (BACKLIGHT_SCALE + 1)
> > > > +		 *
> > > > +		 * The documentation is however inconsistent in its examples,
> > > > +		 * so the interpretation used here is that the duty cycle is
> > > > +		 * the period of BACKLIGHT * PRE_DIV / REFCLK_FREQ.
> > > 
> > > I don't understand this.
> > > 
> > > > +		 *
> > > > +		 * The ratio PRE_DIV / REFCLK_FREQ is rounded up to whole
> > > > +		 * nanoseconds in order to ensure that the calculations are
> > > > +		 * idempotent and gives results that are smaller than the
> > > > +		 * requested value.
> > > > +		 */
> > > > +		tick = DIV_ROUND_UP(NSEC_PER_SEC * pre_div, pdata->pwm_refclk_freq);
> > > > +		backlight = state->duty_cycle / tick;
> > > 
> > > You're loosing precision here by dividing by the result of a division.
> > 
> > The actual period is also a result of a division and after spending too
> > many hours scratching my head I reached to conclusion that this was the
> > reason why I wasn't able to get the duty cycle calculation idempotent
> > over the ranges I tested.
> 
> How did you test? Using the sysfs interface?
>  

I primarily tested this by transplanting this into a user space thing
where I could sweep over various values for refclk, duty cycle and
period.

Then after that I tested it setting up pwm-backlight on top (as I don't
have access to the signal anyways) and try a few different periods and
for those test all possible brightness levels for those periods... (With
CONFIG_PWM_DEBUG enabled)

> > But in my effort to describe this to you here, I finally spotted the
> > error and will follow up with a new version that does:
> > 
> >                 actual = NSEC_PER_SEC * (pre_div * scale + 1) / pdata->pwm_refclk_freq);
> >                 backlight = state->duty_cycle * (scale + 1) / actual;
> 
> So the first term ("actual") is the period that you get for a given
> pre_div, scale and pwm_refclk_freq, right? And the 2nd ("backlight")
> defines the register value to configure the duty_cycle, right?
> 

Right, pre_div and pwm_refclk_freq defines the rate at which the PWM
ticks. "actual" is our estimate of the actual period that results in and
"backlight" is then the number of ticks (each prediv / refclk seconds
long) the signal should be high.

> I wonder: Is it possible to configure a 100% relative duty cycle? Then
> backlight would be scale + 1 which (at least if scale is 0xffff) would
> overflow the 16 bit register width?!
> 

The documentation gives two examples:
* backlight = 0x40, scale = 0xff results in 25% duty cycle, i.e. the
  duty is 0x40 / (0xff + 1).
* backlight = 0xff, scale = 0xff results in 100% duty cycle, i.e. the
  duty is 0xff / 0xff.

As you can see these are in  conflict and I think the latter is the one
that doesn't match the rest of what's described.

So I don't think it's possible to go beyond 99.6% - 99.998% duty cycle,
depending on BACKLIGHT_SCALE.

> > > > +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(NSEC_PER_SEC * (pre_div * scale + 1), pdata->pwm_refclk_freq);
> > > > +	state->duty_cycle = backlight * DIV_ROUND_UP(NSEC_PER_SEC * pre_div, pdata->pwm_refclk_freq);
> > > 
> > > If you use
> > > 
> > > 	state->duty_cycle = DIV_ROUND_UP(backlight * NSEC_PER_SEC * pre_div, pdata->pwm_refclk_freq);
> > > 
> > > instead (with a cast to u64 to not yield an overflow) the result is more
> > > exact.
> > > 
> > 
> > The problem with this is that it sometimes yields duty_cycles larger
> > than what was requested... But going back to describing this as a ratio
> > of the period this becomes:
> > 
> >         state->duty_cycle = DIV_ROUND_UP_ULL(state->period * backlight, scale + 1);
> 
> I saw your next iteration of this patch set, but didn't look into it
> yet. Note that if it uses this formula it sill looses precision.
> Consider:
> 
> 	pwm_refclk_freq = 1333333
> 	pre_div = 4
> 	scale = 60000
> 	backlight = 59999
> 
> then you calculate:
> 
> 	state->period = 180000796 (exact value: 180000795.00019875)
> 	state->duty_cycle = 179994797 (exact value: 179994795.0736975)
> 
> so duty_cycle should actually be reported as 179994796. That happens
> because state->period is already the result of a division, you get the
> right value when doing:
> 
> 	state->duty_cycle = round_up(NSEC_PER_SEC * (pre_div * scale + 2) * backlight, (scale + 1) * pdata->pwm_refclk_freq)
> 

The problem (in addition to that being hideous) with that added
precision is that if I plug in that duty_cycle and period with
pwm_refclk_freq = 19200000 (one of the valid ones) the function is no
longer idempotent.

With period given as 180000796 i get 179998542 back as actual period,
but the duty cycle becomes 3186264 and if I throw that in I get 3185473.

> > > I still find this surprising, I'd expect that SCALE also matters for the
> > > duty_cycle. With the assumption implemented here modifying SCALE only
> > > affects the period. This should be easy to verify?! I would expect that
> > > changing SCALE doesn't affect the relative duty_cycle, so the brightness
> > > of an LED is unaffected (unless the period gets too big of course).
> > > 
> > 
> > I think the hardware is two nested counters, one (A) ticking at REFCLK_FREQ
> > and as that hits PRE_DIV, it kicks the second counter (B) (and resets).
> > 
> > As counter A is reset the output signal goes high, when A hits BACKLIGHT the
> > signal goes low and when A hits BACKLIGHT_SCALE it resets.
> 
> then we would probably have:
> 
> 	period = (scale + 1) * pre_div / refclk
> 
> but not
> 
> 	period = (scale * pre_div + 1) / refclk
> 
> . The former would be nicer because then in the calculation for
> duty_cycle the factor (scale + 1) would cancel.
> 

Not only does scale + 1 cancel, there's something entity that actually
divides the (BACKLIGHT_SCALE + 1) in the denominator of the duty cycle
ratio.

> > Per this implementation the actual length of the duty cycle would indeed
> > be independent of the BACKLIGHT_SCALE,
> 
> In your formula for duty_cycle scale actually does matter. So I think
> we're not there yet?
> 

Right, the relationship between pre_div, backlight and duty_cycle should
be independent of period. I think is misinterpreted what you said
yesterday, and thought you where looking for there to be a relationship.


So, if we decide that we have a typo in the datasheet and make the
formula:

          NSEC_PER_SEC * PRE_DIV * (BACKLIGHT_SCALE + 1)
period = -----------------------------------------------
                        REFCLK_FREQ

then given the formula for the duty ratio:

  duty           BACKLIGHT
-------- = ---------------------
 period     BACKLIGHT_SCALE + 1

with NSEC_PER_SEC * PRE_DIV / REFCLK_FREQ cancelled out, this fits
better together and we can deduce that:

              NSEC_PER_SEC * PRE_DIV * BACKLIGHT
duty_cycle = ------------------------------------
                        REFCLK_FREQ

So after adjusting the calculations for pre_div and scale I can
calculate backlight, without first calculating the actual period using:

             duty_cycle * REFCLK_FREQ
BACKLIGHT = --------------------------
              NSEC_PER_SEC * PRE_DIV

Which I now assume is what you where trying to say but I misunderstood
the other day?


PS. With refclk 19200000 and period 180000796 this satisfies the
PWM_DEBUG requirements for all possible duty_cycles.

Regards,
Bjorn

> > but the length of the low signal (and hence the ratio, which results
> > in the actual brightness) does depend on BACKLIGHT_SCALE.
> 
> Best regards
> Uwe
> 
> -- 
> Pengutronix e.K.                           | Uwe Kleine-König            |
> Industrial Linux Solutions                 | https://www.pengutronix.de/ |

Re: [PATCH v3 2/2] drm/bridge: ti-sn65dsi86: Implement the pwm_chip

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 10547 bytes --]

Hello Bjorn,

On Wed, Jun 23, 2021 at 06:08:32PM -0500, Bjorn Andersson wrote:
> On Wed 23 Jun 03:29 CDT 2021, Uwe Kleine-K?nig wrote:
> > On Tue, Jun 22, 2021 at 08:12:48PM -0500, Bjorn Andersson wrote:
> > > On Tue 22 Jun 15:29 CDT 2021, Uwe Kleine-K?nig wrote:
> > > > On Mon, Jun 21, 2021 at 10:09:48PM -0500, Bjorn Andersson wrote:
> > > > > +		/*
> > > > > +		 * PWM duty cycle is given as:
> > > > > +		 *
> > > > > +		 *   duty = BACKLIGHT / (BACKLIGHT_SCALE + 1)
> > > > > +		 *
> > > > > +		 * The documentation is however inconsistent in its examples,
> > > > > +		 * so the interpretation used here is that the duty cycle is
> > > > > +		 * the period of BACKLIGHT * PRE_DIV / REFCLK_FREQ.
> > > > 
> > > > I don't understand this.
> > > > 
> > > > > +		 *
> > > > > +		 * The ratio PRE_DIV / REFCLK_FREQ is rounded up to whole
> > > > > +		 * nanoseconds in order to ensure that the calculations are
> > > > > +		 * idempotent and gives results that are smaller than the
> > > > > +		 * requested value.
> > > > > +		 */
> > > > > +		tick = DIV_ROUND_UP(NSEC_PER_SEC * pre_div, pdata->pwm_refclk_freq);
> > > > > +		backlight = state->duty_cycle / tick;
> > > > 
> > > > You're loosing precision here by dividing by the result of a division.
> > > 
> > > The actual period is also a result of a division and after spending too
> > > many hours scratching my head I reached to conclusion that this was the
> > > reason why I wasn't able to get the duty cycle calculation idempotent
> > > over the ranges I tested.
> > 
> > How did you test? Using the sysfs interface?
> >  
> 
> I primarily tested this by transplanting this into a user space thing
> where I could sweep over various values for refclk, duty cycle and
> period.

Is this something that is worth sharing as it has some use for others,
too? How do you change the refclk from userspace? How do you interface
the PWM? (Note that if you use /sys/class/pwm your updates are not
atomic.)

> Then after that I tested it setting up pwm-backlight on top (as I don't
> have access to the signal anyways) and try a few different periods and
> for those test all possible brightness levels for those periods... (With
> CONFIG_PWM_DEBUG enabled)
> 
> > > But in my effort to describe this to you here, I finally spotted the
> > > error and will follow up with a new version that does:
> > > 
> > >                 actual = NSEC_PER_SEC * (pre_div * scale + 1) / pdata->pwm_refclk_freq);
> > >                 backlight = state->duty_cycle * (scale + 1) / actual;
> > 
> > So the first term ("actual") is the period that you get for a given
> > pre_div, scale and pwm_refclk_freq, right? And the 2nd ("backlight")
> > defines the register value to configure the duty_cycle, right?
> > 
> 
> Right, pre_div and pwm_refclk_freq defines the rate at which the PWM
> ticks. "actual" is our estimate of the actual period that results in and
> "backlight" is then the number of ticks (each prediv / refclk seconds
> long) the signal should be high.
> 
> > I wonder: Is it possible to configure a 100% relative duty cycle? Then
> > backlight would be scale + 1 which (at least if scale is 0xffff) would
> > overflow the 16 bit register width?!
> > 
> 
> The documentation gives two examples:
> * backlight = 0x40, scale = 0xff results in 25% duty cycle, i.e. the
>   duty is 0x40 / (0xff + 1).
> * backlight = 0xff, scale = 0xff results in 100% duty cycle, i.e. the
>   duty is 0xff / 0xff.
> 
> As you can see these are in  conflict and I think the latter is the one
> that doesn't match the rest of what's described.

Please document that, preferably with the wording "The hardware cannot
generate a 100% duty cycle." to match what pwm-sifive already has.
 
> So I don't think it's possible to go beyond 99.6% - 99.998% duty cycle,
> depending on BACKLIGHT_SCALE.
> 
> > > > > +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(NSEC_PER_SEC * (pre_div * scale + 1), pdata->pwm_refclk_freq);
> > > > > +	state->duty_cycle = backlight * DIV_ROUND_UP(NSEC_PER_SEC * pre_div, pdata->pwm_refclk_freq);
> > > > 
> > > > If you use
> > > > 
> > > > 	state->duty_cycle = DIV_ROUND_UP(backlight * NSEC_PER_SEC * pre_div, pdata->pwm_refclk_freq);
> > > > 
> > > > instead (with a cast to u64 to not yield an overflow) the result is more
> > > > exact.
> > > > 
> > > 
> > > The problem with this is that it sometimes yields duty_cycles larger
> > > than what was requested... But going back to describing this as a ratio
> > > of the period this becomes:
> > > 
> > >         state->duty_cycle = DIV_ROUND_UP_ULL(state->period * backlight, scale + 1);
> > 
> > I saw your next iteration of this patch set, but didn't look into it
> > yet. Note that if it uses this formula it sill looses precision.
> > Consider:
> > 
> > 	pwm_refclk_freq = 1333333
> > 	pre_div = 4
> > 	scale = 60000
> > 	backlight = 59999
> > 
> > then you calculate:
> > 
> > 	state->period = 180000796 (exact value: 180000795.00019875)
> > 	state->duty_cycle = 179994797 (exact value: 179994795.0736975)
> > 
> > so duty_cycle should actually be reported as 179994796. That happens
> > because state->period is already the result of a division, you get the
> > right value when doing:
> > 
> > 	state->duty_cycle = round_up(NSEC_PER_SEC * (pre_div * scale + 2) * backlight, (scale + 1) * pdata->pwm_refclk_freq)
> 
> The problem (in addition to that being hideous) with that added
> precision is that if I plug in that duty_cycle and period with
> pwm_refclk_freq = 19200000 (one of the valid ones) the function is no
> longer idempotent.
> 
> With period given as 180000796 i get 179998542 back as actual period,
> but the duty cycle becomes 3186264 and if I throw that in I get 3185473.

I'm not sure if you're in need for some more mathematical assistance
here?! Independant of how complicate the calculation is, it should be
possible to make this idempotent. With the stuff you write below I think
everything is fine now, right?

> > > > I still find this surprising, I'd expect that SCALE also matters for the
> > > > duty_cycle. With the assumption implemented here modifying SCALE only
> > > > affects the period. This should be easy to verify?! I would expect that
> > > > changing SCALE doesn't affect the relative duty_cycle, so the brightness
> > > > of an LED is unaffected (unless the period gets too big of course).
> > > > 
> > > 
> > > I think the hardware is two nested counters, one (A) ticking at REFCLK_FREQ
> > > and as that hits PRE_DIV, it kicks the second counter (B) (and resets).
> > > 
> > > As counter A is reset the output signal goes high, when A hits BACKLIGHT the
> > > signal goes low and when A hits BACKLIGHT_SCALE it resets.
> > 
> > then we would probably have:
> > 
> > 	period = (scale + 1) * pre_div / refclk
> > 
> > but not
> > 
> > 	period = (scale * pre_div + 1) / refclk
> > 
> > . The former would be nicer because then in the calculation for
> > duty_cycle the factor (scale + 1) would cancel.
> > 
> 
> Not only does scale + 1 cancel, there's something entity that actually
> divides the (BACKLIGHT_SCALE + 1) in the denominator of the duty cycle
> ratio.
> 
> > > Per this implementation the actual length of the duty cycle would indeed
> > > be independent of the BACKLIGHT_SCALE,
> > 
> > In your formula for duty_cycle scale actually does matter. So I think
> > we're not there yet?
> > 
> 
> Right, the relationship between pre_div, backlight and duty_cycle should
> be independent of period. I think is misinterpreted what you said
> yesterday, and thought you where looking for there to be a relationship.
> 
> 
> So, if we decide that we have a typo in the datasheet and make the
> formula:
> 
>           NSEC_PER_SEC * PRE_DIV * (BACKLIGHT_SCALE + 1)
> period = -----------------------------------------------
>                         REFCLK_FREQ
> 
> then given the formula for the duty ratio:
> 
>   duty           BACKLIGHT
> -------- = ---------------------
>  period     BACKLIGHT_SCALE + 1
> 
> with NSEC_PER_SEC * PRE_DIV / REFCLK_FREQ cancelled out, this fits
> better together and we can deduce that:
> 
>               NSEC_PER_SEC * PRE_DIV * BACKLIGHT
> duty_cycle = ------------------------------------
>                         REFCLK_FREQ
> 
> So after adjusting the calculations for pre_div and scale I can
> calculate backlight, without first calculating the actual period using:
> 
>              duty_cycle * REFCLK_FREQ
> BACKLIGHT = --------------------------
>               NSEC_PER_SEC * PRE_DIV
> 
> Which I now assume is what you where trying to say but I misunderstood
> the other day?

I expected something similar because I didn't see why a hardware
engineer should make the hardware so complicated to implement the logic
needed to match your driver maths.

> PS. With refclk 19200000 and period 180000796 this satisfies the
> PWM_DEBUG requirements for all possible duty_cycles.

\o/

I'll mark your v4 in patchwork as "changes requested" and look forward
to your v5 now.

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 --]