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

[PATCH v4 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 v3:
- None

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 v4 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 v3:
- Use proper division operations for u64 divisions
- Calculate duty cycle per backlight / (scale + 1)
- Only mux in PWM when state->enabled
- Include linux/bitfield.h (for FIELD_{GET,PUT} on arm)
- Cap max period to avoid overflows

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 | 344 +++++++++++++++++++++++++-
 1 file changed, 343 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
index 5d712c8c3c3b..a5a0fa38a0ac 100644
--- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
+++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
@@ -4,7 +4,9 @@
  * datasheet: https://www.ti.com/lit/ds/symlink/sn65dsi86.pdf
  */
 
+#include <linux/atomic.h>
 #include <linux/auxiliary_bus.h>
+#include <linux/bitfield.h>
 #include <linux/bits.h>
 #include <linux/clk.h>
 #include <linux/debugfs.h>
@@ -15,6 +17,7 @@
 #include <linux/module.h>
 #include <linux/of_graph.h>
 #include <linux/pm_runtime.h>
+#include <linux/pwm.h>
 #include <linux/regmap.h>
 #include <linux/regulator/consumer.h>
 
@@ -91,6 +94,13 @@
 #define SN_ML_TX_MODE_REG			0x96
 #define  ML_TX_MAIN_LINK_OFF			0
 #define  ML_TX_NORMAL_MODE			BIT(0)
+#define SN_PWM_PRE_DIV_REG			0xA0
+#define SN_BACKLIGHT_SCALE_REG			0xA1
+#define  BACKLIGHT_SCALE_MAX			0xFFFF
+#define SN_BACKLIGHT_REG			0xA3
+#define SN_PWM_EN_INV_REG			0xA5
+#define  SN_PWM_INV_MASK			BIT(0)
+#define  SN_PWM_EN_MASK				BIT(1)
 #define SN_AUX_CMD_STATUS_REG			0xF4
 #define  AUX_IRQ_STATUS_AUX_RPLY_TOUT		BIT(3)
 #define  AUX_IRQ_STATUS_AUX_SHORT		BIT(5)
@@ -113,11 +123,14 @@
 
 #define SN_LINK_TRAINING_TRIES		10
 
+#define SN_PWM_GPIO_IDX			3 /* 4th GPIO */
+
 /**
  * struct ti_sn65dsi86 - Platform data for ti-sn65dsi86 driver.
  * @bridge_aux:   AUX-bus sub device for MIPI-to-eDP bridge functionality.
  * @gpio_aux:     AUX-bus sub device for GPIO controller functionality.
  * @aux_aux:      AUX-bus sub device for eDP AUX channel functionality.
+ * @pwm_aux:      AUX-bus sub device for PWM controller functionality.
  *
  * @dev:          Pointer to the top level (i2c) device.
  * @regmap:       Regmap for accessing i2c.
@@ -145,11 +158,17 @@
  *                bitmap so we can do atomic ops on it without an extra
  *                lock so concurrent users of our 4 GPIOs don't stomp on
  *                each other's read-modify-write.
+ *
+ * @pchip:        pwm_chip if the PWM is exposed.
+ * @pwm_enabled:  Used to track if the PWM signal is currently enabled.
+ * @pwm_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 +191,12 @@ struct ti_sn65dsi86 {
 	struct gpio_chip		gchip;
 	DECLARE_BITMAP(gchip_output, SN_NUM_GPIOS);
 #endif
+#if defined(CONFIG_PWM)
+	struct pwm_chip			pchip;
+	bool				pwm_enabled;
+	unsigned int			pwm_refclk_freq;
+	atomic_t			pwm_pin_busy;
+#endif
 };
 
 static const struct regmap_range ti_sn65dsi86_volatile_ranges[] = {
@@ -190,6 +215,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 +297,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 +1096,266 @@ 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 period_max;
+	u64 actual;
+	u64 period;
+	int ret;
+
+	if (!pdata->pwm_enabled) {
+		ret = pm_runtime_get_sync(pdata->dev);
+		if (ret < 0)
+			return ret;
+	}
+
+	if (state->enabled) {
+		if (!pdata->pwm_enabled) {
+			/*
+			 * The chip might have been powered down while we
+			 * didn't hold a PM runtime reference, so mux in the
+			 * PWM function on the GPIO pin again.
+			 */
+			ret = regmap_update_bits(pdata->regmap, SN_GPIO_CTRL_REG,
+						 SN_GPIO_MUX_MASK << (2 * SN_PWM_GPIO_IDX),
+						 SN_GPIO_MUX_SPECIAL << (2 * SN_PWM_GPIO_IDX));
+			if (ret) {
+				dev_err(pdata->dev, "failed to mux in PWM function\n");
+				goto out;
+			}
+		}
+
+		/*
+		 * Per the datasheet the PWM frequency is given by:
+		 *
+		 *   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 (0 * 0 + 1) / REFCLK_FREQ */
+		if (state->period <= NSEC_PER_SEC / pdata->pwm_refclk_freq) {
+			ret = -EINVAL;
+			goto out;
+		}
+
+		/*
+		 * Maximum T_pwm is (255 * 65535 + 1) / * REFCLK_FREQ
+		 * Limit period to this to avoid overflows
+		 */
+		period_max = div_u64((u64)NSEC_PER_SEC * (255 * 65535 + 1), pdata->pwm_refclk_freq);
+		if (period > period_max)
+			period = period_max;
+		else
+			period = state->period;
+
+		pre_div = DIV64_U64_ROUND_UP((period * pdata->pwm_refclk_freq - NSEC_PER_SEC),
+					     ((u64)NSEC_PER_SEC * BACKLIGHT_SCALE_MAX));
+		scale = div64_u64(period * pdata->pwm_refclk_freq - NSEC_PER_SEC,
+				  (u64)NSEC_PER_SEC * pre_div);
+
+		/*
+		 * The documentation has the duty ratio given as:
+		 *
+		 *     duty          BACKLIGHT
+		 *   ------- = ---------------------
+		 *    period    BACKLIGHT_SCALE + 1
+		 *
+		 * Solve for BACKLIGHT gives us:
+		 */
+		actual = DIV_ROUND_UP_ULL((u64)NSEC_PER_SEC * (pre_div * scale + 1),
+					  pdata->pwm_refclk_freq);
+		backlight = div64_u64(state->duty_cycle * (scale + 1), actual);
+		if (backlight > scale)
+			backlight = scale;
+
+		ret = regmap_write(pdata->regmap, SN_PWM_PRE_DIV_REG, pre_div);
+		if (ret) {
+			dev_err(pdata->dev, "failed to update PWM_PRE_DIV\n");
+			goto out;
+		}
+
+		ti_sn65dsi86_write_u16(pdata, SN_BACKLIGHT_SCALE_REG, scale);
+		ti_sn65dsi86_write_u16(pdata, SN_BACKLIGHT_REG, backlight);
+	}
+
+	pwm_en_inv = FIELD_PREP(SN_PWM_EN_MASK, !!state->enabled) |
+		     FIELD_PREP(SN_PWM_INV_MASK, state->polarity == PWM_POLARITY_INVERSED);
+	ret = regmap_write(pdata->regmap, SN_PWM_EN_INV_REG, pwm_en_inv);
+	if (ret) {
+		dev_err(pdata->dev, "failed to update PWM_EN/PWM_INV\n");
+		goto out;
+	}
+
+	pdata->pwm_enabled = !!state->enabled;
+out:
+
+	if (!pdata->pwm_enabled)
+		pm_runtime_put_sync(pdata->dev);
+
+	return ret;
+}
+
+static void ti_sn_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
+				struct pwm_state *state)
+{
+	struct ti_sn65dsi86 *pdata = pwm_chip_to_ti_sn_bridge(chip);
+	unsigned int pwm_en_inv;
+	unsigned int pre_div;
+	u16 backlight;
+	u16 scale;
+	int ret;
+
+	ret = regmap_read(pdata->regmap, SN_PWM_EN_INV_REG, &pwm_en_inv);
+	if (ret)
+		return;
+
+	ret = ti_sn65dsi86_read_u16(pdata, SN_BACKLIGHT_SCALE_REG, &scale);
+	if (ret)
+		return;
+
+	ret = ti_sn65dsi86_read_u16(pdata, SN_BACKLIGHT_REG, &backlight);
+	if (ret)
+		return;
+
+	ret = regmap_read(pdata->regmap, SN_PWM_PRE_DIV_REG, &pre_div);
+	if (ret)
+		return;
+
+	state->enabled = FIELD_GET(SN_PWM_EN_MASK, pwm_en_inv);
+	if (FIELD_GET(SN_PWM_INV_MASK, pwm_en_inv))
+		state->polarity = PWM_POLARITY_INVERSED;
+	else
+		state->polarity = PWM_POLARITY_NORMAL;
+
+	state->period = DIV_ROUND_UP_ULL((u64)NSEC_PER_SEC * (pre_div * scale + 1), pdata->pwm_refclk_freq);
+	state->duty_cycle = DIV_ROUND_UP_ULL(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.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 +1488,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 +1529,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 +1829,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 +1878,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 +1894,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 +1909,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 v4 1/2] pwm: Introduce single-PWM of_xlate function

[Doug Anderson]

Hi,

On Tue, Jun 22, 2021 at 8:28 PM Bjorn Andersson
<bjorn.andersson@linaro.org> wrote:
>
> The existing pxa driver and the upcoming addition of PWM support in the
> TI sn565dsi86 DSI/eDP bridge driver both has a single PWM channel and
> thereby a need for a of_xlate function with the period as its single
> argument.
>
> Introduce a common helper function in the core that can be used as
> of_xlate by such drivers and migrate the pxa driver to use this.
>
> Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> ---
>
> Changes since v3:
> - None
>
> 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)

It's probably up to PWM folks, but to make it symmetric to
of_pwm_xlate_with_flags() I probably would have named it with the
"_with_flags" suffix.


> +{
> +       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);

I don't know all the rules for attempted forward compatibility, but
unless there's a strong reason I'd expect to match the rules for
of_pwm_xlate_with_flags(). That function doesn't consider it to be an
error if either "pc->of_pwm_n_cells" or "args->args_count" is bigger
than you need. Unless there's a reason to be inconsistent, it seems
like we should be consistent between the two functions. That would
make the test:

if (args->args_count < 1)
  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)

Similar to above, should this be ">= 2" rather than "== 2" ?

I also notice that in commit cf38c978cf1d ("pwm: Make
of_pwm_xlate_with_flags() work with #pwm-cells = <2>") Uwe added a
check for "pc->of_pwm_n_cells" in of_pwm_xlate_with_flags() right
around here. You're not checking it in your function.

I _think_ your code is fine because I can't see how "args->args_count"
could ever be greater than "pc->of_pwm_n_cells" but maybe I'm not
seeing something. Assuming your code is correct then maybe the right
thing to do is to remove the extra check from
of_pwm_xlate_with_flags() to make the two functions more similar.


-Doug

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

[Doug Anderson]

Hi,

On Tue, Jun 22, 2021 at 8:28 PM Bjorn Andersson
<bjorn.andersson@linaro.org> wrote:
>
> +static int ti_sn65dsi86_read_u16(struct ti_sn65dsi86 *pdata,
> +                                unsigned int reg, u16 *val)
> +{
> +       unsigned int tmp;
> +       int ret;
> +
> +       ret = regmap_read(pdata->regmap, reg, &tmp);
> +       if (ret)
> +               return ret;
> +       *val = tmp;
> +
> +       ret = regmap_read(pdata->regmap, reg + 1, &tmp);
> +       if (ret)
> +               return ret;
> +       *val |= tmp << 8;
> +
> +       return 0;
> +}
> +
>  static void ti_sn65dsi86_write_u16(struct ti_sn65dsi86 *pdata,
>                                    unsigned int reg, u16 val)

I suspect we might want to update this function to use
regmap_bulk_write(). I believe that will allow PWM updates to happen
in a single i2c transaction. I don't know whether the bridge chip
implements that, but conceivably it could use this information to
avoid discontinuities when updating the "high" and "low" parts of a
register. Even if the bridge chip doesn't do anything special, though,
it will reduce the amount of time that they are inconsistent because
it'll be a single transaction on the bus rather than two separate
ones.


>  {
> @@ -253,6 +297,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

I really dislike #ifdefs inline in functions. Personally I'd rather
you just always put the member in the structure regardless of
CONFIG_PWM and always set it.


> +/*
> + * 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.

"and is used to keep the determine" ? Something about that wording
doesn't make sense to me.

> + * - Changing both period and duty_cycle is not done atomically, so the output
> + *   might briefly be a mix of the two settings.

In fact there's nothing atomic about _any_ of the updates, right?
We're setting the high and low bytes in separate transactions so if
you were watching carefully you might see this if you bumped the PWM
up by 1:

0x03ff
0x04ff
0x0400

> + */
> +static int ti_sn_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> +                          const struct pwm_state *state)
> +{
> +       struct ti_sn65dsi86 *pdata = pwm_chip_to_ti_sn_bridge(chip);
> +       unsigned int pwm_en_inv;
> +       unsigned int backlight;
> +       unsigned int pre_div;
> +       unsigned int scale;
> +       u64 period_max;
> +       u64 actual;
> +       u64 period;
> +       int ret;
> +
> +       if (!pdata->pwm_enabled) {
> +               ret = pm_runtime_get_sync(pdata->dev);
> +               if (ret < 0)
> +                       return ret;

You hit the classic pm_runtime trap! :-) You must always call put even
if get fails. I think a "goto out" would do it?


> +       }
> +
> +       if (state->enabled) {
> +               if (!pdata->pwm_enabled) {
> +                       /*
> +                        * The chip might have been powered down while we
> +                        * didn't hold a PM runtime reference, so mux in the
> +                        * PWM function on the GPIO pin again.
> +                        */
> +                       ret = regmap_update_bits(pdata->regmap, SN_GPIO_CTRL_REG,
> +                                                SN_GPIO_MUX_MASK << (2 * SN_PWM_GPIO_IDX),
> +                                                SN_GPIO_MUX_SPECIAL << (2 * SN_PWM_GPIO_IDX));
> +                       if (ret) {
> +                               dev_err(pdata->dev, "failed to mux in PWM function\n");
> +                               goto out;
> +                       }
> +               }
> +
> +               /*
> +                * Per the datasheet the PWM frequency is given by:
> +                *
> +                *   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 (0 * 0 + 1) / REFCLK_FREQ */
> +               if (state->period <= NSEC_PER_SEC / pdata->pwm_refclk_freq) {
> +                       ret = -EINVAL;
> +                       goto out;
> +               }
> +
> +               /*
> +                * Maximum T_pwm is (255 * 65535 + 1) / * REFCLK_FREQ
> +                * Limit period to this to avoid overflows
> +                */
> +               period_max = div_u64((u64)NSEC_PER_SEC * (255 * 65535 + 1), pdata->pwm_refclk_freq);
> +               if (period > period_max)
> +                       period = period_max;
> +               else
> +                       period = state->period;
> +
> +               pre_div = DIV64_U64_ROUND_UP((period * pdata->pwm_refclk_freq - NSEC_PER_SEC),
> +                                            ((u64)NSEC_PER_SEC * BACKLIGHT_SCALE_MAX));
> +               scale = div64_u64(period * pdata->pwm_refclk_freq - NSEC_PER_SEC,
> +                                 (u64)NSEC_PER_SEC * pre_div);
> +
> +               /*
> +                * The documentation has the duty ratio given as:
> +                *
> +                *     duty          BACKLIGHT
> +                *   ------- = ---------------------
> +                *    period    BACKLIGHT_SCALE + 1
> +                *
> +                * Solve for BACKLIGHT gives us:
> +                */
> +               actual = DIV_ROUND_UP_ULL((u64)NSEC_PER_SEC * (pre_div * scale + 1),
> +                                         pdata->pwm_refclk_freq);
> +               backlight = div64_u64(state->duty_cycle * (scale + 1), actual);
> +               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) |

nit: no need for "!!". state->enabled is a boolean.


> +                    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;

nit: no need for "!!". state->enabled is a boolean.


> +out:
> +
> +       if (!pdata->pwm_enabled)
> +               pm_runtime_put_sync(pdata->dev);
> +
> +       return ret;
> +}

note: I didn't look at _any_ of your logic here. I figure that you and
Uwe already broke your brains on it. I'll try to take a quick peek
once you guys come to come agreement.

One note: in theory it ought to be not impossible to measure this even
if you're not an EE if you happen to have access to something like a
Salae Logic 16. The PWM ought to go out on the cable connecting to the
LCD on one of the pins and those pins tend to be easy enough to probe
that even a noob like myself can probe them. Of course it does mean
opening up your device...


> +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)

nit: did you need two blank lines before this function?


> @@ -1500,6 +1829,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;
> +       }
> +

nit: also update the comment block above that says "Soon the PWM
provided by the bridge chip..."


-Doug

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

[Bjorn Andersson]

On Wed 23 Jun 17:19 CDT 2021, Doug Anderson wrote:

> Hi,
> 
> On Tue, Jun 22, 2021 at 8:28 PM Bjorn Andersson
> <bjorn.andersson@linaro.org> wrote:
> >
> > The existing pxa driver and the upcoming addition of PWM support in the
> > TI sn565dsi86 DSI/eDP bridge driver both has a single PWM channel and
> > thereby a need for a of_xlate function with the period as its single
> > argument.
> >
> > Introduce a common helper function in the core that can be used as
> > of_xlate by such drivers and migrate the pxa driver to use this.
> >
> > Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> > ---
> >
> > Changes since v3:
> > - None
> >
> > 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)
> 
> It's probably up to PWM folks, but to make it symmetric to
> of_pwm_xlate_with_flags() I probably would have named it with the
> "_with_flags" suffix.
> 

I don't see a reason for having the no-flags variant of this, but you're
right in that it does look more uniform.

> 
> > +{
> > +       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);
> 
> I don't know all the rules for attempted forward compatibility, but
> unless there's a strong reason I'd expect to match the rules for
> of_pwm_xlate_with_flags(). That function doesn't consider it to be an
> error if either "pc->of_pwm_n_cells" or "args->args_count" is bigger
> than you need. Unless there's a reason to be inconsistent, it seems
> like we should be consistent between the two functions. That would
> make the test:
> 
> if (args->args_count < 1)
>   return ERR_PTR(-EINVAL);
> 

My crystal ball is foggy, but I guess I could follow suite even though I
don't see what that might be.

> 
> > +
> > +       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)
> 
> Similar to above, should this be ">= 2" rather than "== 2" ?
> 
> I also notice that in commit cf38c978cf1d ("pwm: Make
> of_pwm_xlate_with_flags() work with #pwm-cells = <2>") Uwe added a
> check for "pc->of_pwm_n_cells" in of_pwm_xlate_with_flags() right
> around here. You're not checking it in your function.
> 
> I _think_ your code is fine because I can't see how "args->args_count"
> could ever be greater than "pc->of_pwm_n_cells" but maybe I'm not
> seeing something. Assuming your code is correct then maybe the right
> thing to do is to remove the extra check from
> of_pwm_xlate_with_flags() to make the two functions more similar.
> 

I guess the way of_pwm_xlate_with_flags() is written the optional flags
will only be considered if the driver has stated that it supports the
3rd field.

The way I wrote this means that I don't care if the drivers supports
flags I will pick up that INVERTED bit. I suppose this means that if a
driver where to increment of_pwm_n_cells we suddenly start to care about
a cell that we previously never looked at...

But it would be consistent to follow this, and I don't really have an
opinion about these nuances.

Thanks for your feedback Doug.

Regards,
Bjorn

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

[Bjorn Andersson]

On Wed 23 Jun 18:37 CDT 2021, Doug Anderson wrote:

> Hi,
> 
> On Tue, Jun 22, 2021 at 8:28 PM Bjorn Andersson
> <bjorn.andersson@linaro.org> wrote:
> >
> > +static int ti_sn65dsi86_read_u16(struct ti_sn65dsi86 *pdata,
> > +                                unsigned int reg, u16 *val)
> > +{
> > +       unsigned int tmp;
> > +       int ret;
> > +
> > +       ret = regmap_read(pdata->regmap, reg, &tmp);
> > +       if (ret)
> > +               return ret;
> > +       *val = tmp;
> > +
> > +       ret = regmap_read(pdata->regmap, reg + 1, &tmp);
> > +       if (ret)
> > +               return ret;
> > +       *val |= tmp << 8;
> > +
> > +       return 0;
> > +}
> > +
> >  static void ti_sn65dsi86_write_u16(struct ti_sn65dsi86 *pdata,
> >                                    unsigned int reg, u16 val)
> 
> I suspect we might want to update this function to use
> regmap_bulk_write(). I believe that will allow PWM updates to happen
> in a single i2c transaction. I don't know whether the bridge chip
> implements that, but conceivably it could use this information to
> avoid discontinuities when updating the "high" and "low" parts of a
> register. Even if the bridge chip doesn't do anything special, though,
> it will reduce the amount of time that they are inconsistent because
> it'll be a single transaction on the bus rather than two separate
> ones.
> 

You bulk_write in ti_sn_aux_transfer() so I don't see why it wouldn't
work here, I'll update this - even though I don't know if it will
actually result in the two bytes being updated "atomically".

> 
> >  {
> > @@ -253,6 +297,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
> 
> I really dislike #ifdefs inline in functions. Personally I'd rather
> you just always put the member in the structure regardless of
> CONFIG_PWM and always set it.
> 

I'd be happy to do so.

> 
> > +/*
> > + * 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.
> 
> "and is used to keep the determine" ? Something about that wording
> doesn't make sense to me.
> 

Let's subtract "keep the" from that sentence...

> > + * - Changing both period and duty_cycle is not done atomically, so the output
> > + *   might briefly be a mix of the two settings.
> 
> In fact there's nothing atomic about _any_ of the updates, right?
> We're setting the high and low bytes in separate transactions so if
> you were watching carefully you might see this if you bumped the PWM
> up by 1:
> 
> 0x03ff
> 0x04ff
> 0x0400
> 

I've tested this several times with

    for i in `seq 4095`; do echo $i > /sys/class/backlight/backlight/brightness; done

And I am not able to see that "transient", but let's improve the u16
write anyways.

> > + */
> > +static int ti_sn_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> > +                          const struct pwm_state *state)
> > +{
> > +       struct ti_sn65dsi86 *pdata = pwm_chip_to_ti_sn_bridge(chip);
> > +       unsigned int pwm_en_inv;
> > +       unsigned int backlight;
> > +       unsigned int pre_div;
> > +       unsigned int scale;
> > +       u64 period_max;
> > +       u64 actual;
> > +       u64 period;
> > +       int ret;
> > +
> > +       if (!pdata->pwm_enabled) {
> > +               ret = pm_runtime_get_sync(pdata->dev);
> > +               if (ret < 0)
> > +                       return ret;
> 
> You hit the classic pm_runtime trap! :-) You must always call put even
> if get fails. I think a "goto out" would do it?
> 

Another one bits the dust...

Thanks for teaching me.

> 
> > +       }
> > +
> > +       if (state->enabled) {
> > +               if (!pdata->pwm_enabled) {
> > +                       /*
> > +                        * The chip might have been powered down while we
> > +                        * didn't hold a PM runtime reference, so mux in the
> > +                        * PWM function on the GPIO pin again.
> > +                        */
> > +                       ret = regmap_update_bits(pdata->regmap, SN_GPIO_CTRL_REG,
> > +                                                SN_GPIO_MUX_MASK << (2 * SN_PWM_GPIO_IDX),
> > +                                                SN_GPIO_MUX_SPECIAL << (2 * SN_PWM_GPIO_IDX));
> > +                       if (ret) {
> > +                               dev_err(pdata->dev, "failed to mux in PWM function\n");
> > +                               goto out;
> > +                       }
> > +               }
> > +
> > +               /*
> > +                * Per the datasheet the PWM frequency is given by:
> > +                *
> > +                *   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 (0 * 0 + 1) / REFCLK_FREQ */
> > +               if (state->period <= NSEC_PER_SEC / pdata->pwm_refclk_freq) {
> > +                       ret = -EINVAL;
> > +                       goto out;
> > +               }
> > +
> > +               /*
> > +                * Maximum T_pwm is (255 * 65535 + 1) / * REFCLK_FREQ
> > +                * Limit period to this to avoid overflows
> > +                */
> > +               period_max = div_u64((u64)NSEC_PER_SEC * (255 * 65535 + 1), pdata->pwm_refclk_freq);
> > +               if (period > period_max)
> > +                       period = period_max;
> > +               else
> > +                       period = state->period;
> > +
> > +               pre_div = DIV64_U64_ROUND_UP((period * pdata->pwm_refclk_freq - NSEC_PER_SEC),
> > +                                            ((u64)NSEC_PER_SEC * BACKLIGHT_SCALE_MAX));
> > +               scale = div64_u64(period * pdata->pwm_refclk_freq - NSEC_PER_SEC,
> > +                                 (u64)NSEC_PER_SEC * pre_div);
> > +
> > +               /*
> > +                * The documentation has the duty ratio given as:
> > +                *
> > +                *     duty          BACKLIGHT
> > +                *   ------- = ---------------------
> > +                *    period    BACKLIGHT_SCALE + 1
> > +                *
> > +                * Solve for BACKLIGHT gives us:
> > +                */
> > +               actual = DIV_ROUND_UP_ULL((u64)NSEC_PER_SEC * (pre_div * scale + 1),
> > +                                         pdata->pwm_refclk_freq);
> > +               backlight = div64_u64(state->duty_cycle * (scale + 1), actual);
> > +               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) |
> 
> nit: no need for "!!". state->enabled is a boolean.
> 

Ahh, you're right.

> 
> > +                    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;
> 
> nit: no need for "!!". state->enabled is a boolean.
> 
> 
> > +out:
> > +
> > +       if (!pdata->pwm_enabled)
> > +               pm_runtime_put_sync(pdata->dev);
> > +
> > +       return ret;
> > +}
> 
> note: I didn't look at _any_ of your logic here. I figure that you and
> Uwe already broke your brains on it. I'll try to take a quick peek
> once you guys come to come agreement.
> 

I think we're approaching a conclusion, at which time I certainly
appreciate the additional review :)

> One note: in theory it ought to be not impossible to measure this even
> if you're not an EE if you happen to have access to something like a
> Salae Logic 16. The PWM ought to go out on the cable connecting to the
> LCD on one of the pins and those pins tend to be easy enough to probe
> that even a noob like myself can probe them. Of course it does mean
> opening up your device...
> 

If I had access to the signal I would have already thrown my
oscilloscope at it and answered the question. But I only have this chip
inside my production-Lenovo Yoga C630...

> 
> > +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)
> 
> nit: did you need two blank lines before this function?
> 

No.

> 
> > @@ -1500,6 +1829,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;
> > +       }
> > +
> 
> nit: also update the comment block above that says "Soon the PWM
> provided by the bridge chip..."
> 

Missed that one, thanks.

Regards,
Bjorn