linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2 1/2] pwm: Introduce single-PWM of_xlate function
@ 2021-06-15 23:18 Bjorn Andersson
  2021-06-15 23:18 ` [PATCH v2 2/2] drm/bridge: ti-sn65dsi86: Implement the pwm_chip Bjorn Andersson
  0 siblings, 1 reply; 9+ messages in thread
From: Bjorn Andersson @ 2021-06-15 23:18 UTC (permalink / raw)
  To: Thierry Reding, Uwe Kleine-König, Lee Jones, Doug Anderson
  Cc: Andrzej Hajda, Neil Armstrong, Robert Foss, Laurent Pinchart,
	Jonas Karlman, Jernej Skrabec, David Airlie, Daniel Vetter,
	dri-devel, linux-kernel, linux-pwm

The existing pxa driver and the upcoming addition of PWM support in the
TI sn565dsi86 DSI/eDP bridge driver both has a single PWM channel and
thereby a need for a of_xlate function with the period as its single
argument.

Introduce a common helper function in the core that can be used as
of_xlate by such drivers and migrate the pxa driver to use this.

Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
---

Changes since v1:
- Pick up the optional flags cell as well

 drivers/pwm/core.c    | 26 ++++++++++++++++++++++++++
 drivers/pwm/pwm-pxa.c | 16 +---------------
 include/linux/pwm.h   |  2 ++
 3 files changed, 29 insertions(+), 15 deletions(-)

diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
index a42999f877d2..5e9c876fccc4 100644
--- a/drivers/pwm/core.c
+++ b/drivers/pwm/core.c
@@ -152,6 +152,32 @@ of_pwm_xlate_with_flags(struct pwm_chip *pc, const struct of_phandle_args *args)
 }
 EXPORT_SYMBOL_GPL(of_pwm_xlate_with_flags);
 
+struct pwm_device *
+of_pwm_single_xlate(struct pwm_chip *pc, const struct of_phandle_args *args)
+{
+	struct pwm_device *pwm;
+
+	if (pc->of_pwm_n_cells < 1)
+		return ERR_PTR(-EINVAL);
+
+	/* validate that one cell is specified, optionally with flags */
+	if (args->args_count != 1 && args->args_count != 2)
+		return ERR_PTR(-EINVAL);
+
+	pwm = pwm_request_from_chip(pc, 0, NULL);
+	if (IS_ERR(pwm))
+		return pwm;
+
+	pwm->args.period = args->args[0];
+	pwm->args.polarity = PWM_POLARITY_NORMAL;
+
+	if (args->args_count == 2 && args->args[2] & PWM_POLARITY_INVERTED)
+		pwm->args.polarity = PWM_POLARITY_INVERSED;
+
+	return pwm;
+}
+EXPORT_SYMBOL_GPL(of_pwm_single_xlate);
+
 static void of_pwmchip_add(struct pwm_chip *chip)
 {
 	if (!chip->dev || !chip->dev->of_node)
diff --git a/drivers/pwm/pwm-pxa.c b/drivers/pwm/pwm-pxa.c
index cfb683827d32..8cd82fb54483 100644
--- a/drivers/pwm/pwm-pxa.c
+++ b/drivers/pwm/pwm-pxa.c
@@ -148,20 +148,6 @@ static const struct platform_device_id *pxa_pwm_get_id_dt(struct device *dev)
 	return id ? id->data : NULL;
 }
 
-static struct pwm_device *
-pxa_pwm_of_xlate(struct pwm_chip *pc, const struct of_phandle_args *args)
-{
-	struct pwm_device *pwm;
-
-	pwm = pwm_request_from_chip(pc, 0, NULL);
-	if (IS_ERR(pwm))
-		return pwm;
-
-	pwm->args.period = args->args[0];
-
-	return pwm;
-}
-
 static int pwm_probe(struct platform_device *pdev)
 {
 	const struct platform_device_id *id = platform_get_device_id(pdev);
@@ -187,7 +173,7 @@ static int pwm_probe(struct platform_device *pdev)
 	pwm->chip.npwm = (id->driver_data & HAS_SECONDARY_PWM) ? 2 : 1;
 
 	if (IS_ENABLED(CONFIG_OF)) {
-		pwm->chip.of_xlate = pxa_pwm_of_xlate;
+		pwm->chip.of_xlate = of_pwm_single_xlate;
 		pwm->chip.of_pwm_n_cells = 1;
 	}
 
diff --git a/include/linux/pwm.h b/include/linux/pwm.h
index 5a73251d28e3..6aff1fa4fe5d 100644
--- a/include/linux/pwm.h
+++ b/include/linux/pwm.h
@@ -411,6 +411,8 @@ struct pwm_device *pwm_request_from_chip(struct pwm_chip *chip,
 
 struct pwm_device *of_pwm_xlate_with_flags(struct pwm_chip *pc,
 		const struct of_phandle_args *args);
+struct pwm_device *of_pwm_single_xlate(struct pwm_chip *pc,
+				       const struct of_phandle_args *args);
 
 struct pwm_device *pwm_get(struct device *dev, const char *con_id);
 struct pwm_device *of_pwm_get(struct device *dev, struct device_node *np,
-- 
2.31.0


^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH v2 2/2] drm/bridge: ti-sn65dsi86: Implement the pwm_chip
  2021-06-15 23:18 [PATCH v2 1/2] pwm: Introduce single-PWM of_xlate function Bjorn Andersson
@ 2021-06-15 23:18 ` Bjorn Andersson
  2021-06-16  7:56   ` Uwe Kleine-König
  0 siblings, 1 reply; 9+ messages in thread
From: Bjorn Andersson @ 2021-06-15 23:18 UTC (permalink / raw)
  To: Andrzej Hajda, Neil Armstrong, Robert Foss, Laurent Pinchart,
	Jonas Karlman, Jernej Skrabec, David Airlie, Daniel Vetter,
	Thierry Reding, Uwe Kleine-König, Lee Jones, Doug Anderson
  Cc: dri-devel, linux-kernel, linux-pwm

The SN65DSI86 provides the ability to supply a PWM signal on GPIO 4,
with the primary purpose of controlling the backlight of the attached
panel. Add an implementation that exposes this using the standard PWM
framework, to allow e.g. pwm-backlight to expose this to the user.

Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
---

Changes since v1:
- Rebased ontop of Doug's auxiliary_bus patches
- Reworked the math, per Uwe's request
- Added pwm_chip->get_state and made sure it's happy (only tested with a few
  limited periods, such as 1kHz)

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

diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
index 5d712c8c3c3b..8f11c9b2da48 100644
--- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
+++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
@@ -4,6 +4,7 @@
  * datasheet: https://www.ti.com/lit/ds/symlink/sn65dsi86.pdf
  */
 
+#include <linux/atomic.h>
 #include <linux/auxiliary_bus.h>
 #include <linux/bits.h>
 #include <linux/clk.h>
@@ -15,6 +16,7 @@
 #include <linux/module.h>
 #include <linux/of_graph.h>
 #include <linux/pm_runtime.h>
+#include <linux/pwm.h>
 #include <linux/regmap.h>
 #include <linux/regulator/consumer.h>
 
@@ -91,6 +93,13 @@
 #define SN_ML_TX_MODE_REG			0x96
 #define  ML_TX_MAIN_LINK_OFF			0
 #define  ML_TX_NORMAL_MODE			BIT(0)
+#define SN_PWM_PRE_DIV_REG			0xA0
+#define SN_BACKLIGHT_SCALE_REG			0xA1
+#define  BACKLIGHT_SCALE_MAX			0xFFFF
+#define SN_BACKLIGHT_REG			0xA3
+#define SN_PWM_EN_INV_REG			0xA5
+#define  SN_PWM_INV_MASK			BIT(0)
+#define  SN_PWM_EN_MASK				BIT(1)
 #define SN_AUX_CMD_STATUS_REG			0xF4
 #define  AUX_IRQ_STATUS_AUX_RPLY_TOUT		BIT(3)
 #define  AUX_IRQ_STATUS_AUX_SHORT		BIT(5)
@@ -113,11 +122,14 @@
 
 #define SN_LINK_TRAINING_TRIES		10
 
+#define SN_PWM_GPIO_IDX			3 /* 4th GPIO */
+
 /**
  * struct ti_sn65dsi86 - Platform data for ti-sn65dsi86 driver.
  * @bridge_aux:   AUX-bus sub device for MIPI-to-eDP bridge functionality.
  * @gpio_aux:     AUX-bus sub device for GPIO controller functionality.
  * @aux_aux:      AUX-bus sub device for eDP AUX channel functionality.
+ * @pwm_aux:      AUX-bus sub device for PWM controller functionality.
  *
  * @dev:          Pointer to the top level (i2c) device.
  * @regmap:       Regmap for accessing i2c.
@@ -145,11 +157,17 @@
  *                bitmap so we can do atomic ops on it without an extra
  *                lock so concurrent users of our 4 GPIOs don't stomp on
  *                each other's read-modify-write.
+ *
+ * @pchip:        pwm_chip if the PWM is exposed.
+ * @pwm_enabled:  Used to track if the PWM signal is currently enabled.
+ * @pwm_refclk_freq: Cache for the reference clock input to the PWM.
+ * @pwm_pin_busy: Track if GPIO4 is currently requested for GPIO or PWM.
  */
 struct ti_sn65dsi86 {
 	struct auxiliary_device		bridge_aux;
 	struct auxiliary_device		gpio_aux;
 	struct auxiliary_device		aux_aux;
+	struct auxiliary_device		pwm_aux;
 
 	struct device			*dev;
 	struct regmap			*regmap;
@@ -172,6 +190,12 @@ struct ti_sn65dsi86 {
 	struct gpio_chip		gchip;
 	DECLARE_BITMAP(gchip_output, SN_NUM_GPIOS);
 #endif
+#if defined(CONFIG_PWM)
+	struct pwm_chip			pchip;
+	bool				pwm_enabled;
+	unsigned int			pwm_refclk_freq;
+	atomic_t			pwm_pin_busy;
+#endif
 };
 
 static const struct regmap_range ti_sn65dsi86_volatile_ranges[] = {
@@ -190,6 +214,25 @@ static const struct regmap_config ti_sn65dsi86_regmap_config = {
 	.cache_type = REGCACHE_NONE,
 };
 
+static int ti_sn65dsi86_read_u16(struct ti_sn65dsi86 *pdata,
+				 unsigned int reg, u16 *val)
+{
+	unsigned int tmp;
+	int ret;
+
+	ret = regmap_read(pdata->regmap, reg, &tmp);
+	if (ret)
+		return ret;
+	*val = tmp;
+
+	ret = regmap_read(pdata->regmap, reg + 1, &tmp);
+	if (ret)
+		return ret;
+	*val |= tmp << 8;
+
+	return 0;
+}
+
 static void ti_sn65dsi86_write_u16(struct ti_sn65dsi86 *pdata,
 				   unsigned int reg, u16 val)
 {
@@ -253,6 +296,14 @@ static void ti_sn_bridge_set_refclk_freq(struct ti_sn65dsi86 *pdata)
 
 	regmap_update_bits(pdata->regmap, SN_DPPLL_SRC_REG, REFCLK_FREQ_MASK,
 			   REFCLK_FREQ(i));
+
+#if defined(CONFIG_PWM)
+	/*
+	 * The PWM refclk is based on the value written to SN_DPPLL_SRC_REG,
+	 * regardless of its actual sourcing.
+	 */
+	pdata->pwm_refclk_freq = ti_sn_bridge_refclk_lut[i];
+#endif
 }
 
 static void ti_sn65dsi86_enable_comms(struct ti_sn65dsi86 *pdata)
@@ -1044,6 +1095,221 @@ static int ti_sn_bridge_parse_dsi_host(struct ti_sn65dsi86 *pdata)
 	return 0;
 }
 
+#if defined(CONFIG_PWM)
+static int ti_sn_pwm_pin_request(struct ti_sn65dsi86 *pdata)
+{
+	return atomic_xchg(&pdata->pwm_pin_busy, 1) ? -EBUSY : 0;
+}
+
+static void ti_sn_pwm_pin_release(struct ti_sn65dsi86 *pdata)
+{
+	atomic_set(&pdata->pwm_pin_busy, 0);
+}
+
+static struct ti_sn65dsi86 *pwm_chip_to_ti_sn_bridge(struct pwm_chip *chip)
+{
+	return container_of(chip, struct ti_sn65dsi86, pchip);
+}
+
+static int ti_sn_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm)
+{
+	struct ti_sn65dsi86 *pdata = pwm_chip_to_ti_sn_bridge(chip);
+
+	return ti_sn_pwm_pin_request(pdata);
+}
+
+static void ti_sn_pwm_free(struct pwm_chip *chip, struct pwm_device *pwm)
+{
+	struct ti_sn65dsi86 *pdata = pwm_chip_to_ti_sn_bridge(chip);
+
+	ti_sn_pwm_pin_release(pdata);
+}
+
+static int ti_sn_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
+			   const struct pwm_state *state)
+{
+	struct ti_sn65dsi86 *pdata = pwm_chip_to_ti_sn_bridge(chip);
+	unsigned int pwm_en_inv;
+	unsigned int backlight;
+	unsigned int pre_div;
+	unsigned int scale;
+	int ret;
+
+	if (!pdata->pwm_enabled) {
+		ret = pm_runtime_get_sync(pdata->dev);
+		if (ret < 0)
+			return ret;
+
+		ret = regmap_update_bits(pdata->regmap, SN_GPIO_CTRL_REG,
+				SN_GPIO_MUX_MASK << (2 * SN_PWM_GPIO_IDX),
+				SN_GPIO_MUX_SPECIAL << (2 * SN_PWM_GPIO_IDX));
+		if (ret) {
+			dev_err(pdata->dev, "failed to mux in PWM function\n");
+			goto out;
+		}
+	}
+
+	if (state->enabled) {
+		/*
+		 * Per the datasheet the PWM frequency is given by:
+		 *
+		 *   PWM_FREQ = REFCLK_FREQ / (PWM_PRE_DIV * BACKLIGHT_SCALE + 1)
+		 *
+		 * which can be rewritten:
+		 *
+		 *   T_pwm * REFCLK_FREQ - 1 = PWM_PRE_DIV * BACKLIGHT_SCALE
+		 *
+		 * In order to keep BACKLIGHT_SCALE within its 16 bits, PWM_PRE_DIV
+		 * must be:
+		 *
+		 *   PWM_PRE_DIV >= (T_pwm * REFCLK_FREQ - 1) / BACKLIGHT_SCALE_MAX;
+		 *
+		 * To simplify the search and optimize the resolution of the PWM, the
+		 * lowest possible PWM_PRE_DIV is used. Finally the scale is calculated
+		 * as:
+		 *
+		 *   BACKLIGHT_SCALE = (T_pwm * REFCLK_FREQ - 1) / PWM_PRE_DIV
+		 *
+		 * Here T_pwm is represented in seconds, so appropriate scaling to
+		 * nanoseconds is necessary.
+		 */
+		pre_div = DIV_ROUND_UP((state->period * pdata->pwm_refclk_freq - 1),
+				       (NSEC_PER_SEC * BACKLIGHT_SCALE_MAX));
+		scale = (state->period * pdata->pwm_refclk_freq - 1) / (NSEC_PER_SEC * pre_div);
+
+		/*
+		 * The duty ratio is given as:
+		 *
+		 *   duty = BACKLIGHT / (BACKLIGHT_SCALE + 1)
+		 */
+		backlight = state->duty_cycle * (scale + 1) / state->period;
+
+		ret = regmap_write(pdata->regmap, SN_PWM_PRE_DIV_REG, pre_div);
+		if (ret) {
+			dev_err(pdata->dev, "failed to update PWM_PRE_DIV\n");
+			goto out;
+		}
+
+		ti_sn65dsi86_write_u16(pdata, SN_BACKLIGHT_SCALE_REG, scale);
+		ti_sn65dsi86_write_u16(pdata, SN_BACKLIGHT_REG, backlight);
+	}
+
+	pwm_en_inv = FIELD_PREP(SN_PWM_EN_MASK, !!state->enabled) |
+		     FIELD_PREP(SN_PWM_INV_MASK, state->polarity == PWM_POLARITY_INVERSED);
+	ret = regmap_write(pdata->regmap, SN_PWM_EN_INV_REG, pwm_en_inv);
+	if (ret) {
+		dev_err(pdata->dev, "failed to update PWM_EN/PWM_INV\n");
+		goto out;
+	}
+
+	pdata->pwm_enabled = !!state->enabled;
+out:
+
+	if (!pdata->pwm_enabled)
+		pm_runtime_put_sync(pdata->dev);
+
+	return ret;
+}
+
+static void ti_sn_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
+				struct pwm_state *state)
+{
+	struct ti_sn65dsi86 *pdata = pwm_chip_to_ti_sn_bridge(chip);
+	unsigned int pwm_en_inv;
+	unsigned int pre_div;
+	u16 backlight;
+	u16 scale;
+	int ret;
+
+	ret = regmap_read(pdata->regmap, SN_PWM_EN_INV_REG, &pwm_en_inv);
+	if (ret)
+		return;
+
+	ret = ti_sn65dsi86_read_u16(pdata, SN_BACKLIGHT_SCALE_REG, &scale);
+	if (ret)
+		return;
+
+	ret = ti_sn65dsi86_read_u16(pdata, SN_BACKLIGHT_REG, &backlight);
+	if (ret)
+		return;
+
+	ret = regmap_read(pdata->regmap, SN_PWM_PRE_DIV_REG, &pre_div);
+	if (ret)
+		return;
+
+	state->enabled = FIELD_GET(SN_PWM_EN_MASK, pwm_en_inv);
+	if (FIELD_GET(SN_PWM_INV_MASK, pwm_en_inv))
+		state->polarity = PWM_POLARITY_INVERSED;
+	else
+		state->polarity = PWM_POLARITY_NORMAL;
+
+	state->period = NSEC_PER_SEC * (pre_div * scale + 1) / pdata->pwm_refclk_freq;
+	state->duty_cycle = DIV_ROUND_UP(state->period * backlight, scale + 1);
+}
+
+static const struct pwm_ops ti_sn_pwm_ops = {
+	.request = ti_sn_pwm_request,
+	.free = ti_sn_pwm_free,
+	.apply = ti_sn_pwm_apply,
+	.get_state = ti_sn_pwm_get_state,
+	.owner = THIS_MODULE,
+};
+
+static int ti_sn_pwm_probe(struct auxiliary_device *adev,
+		const struct auxiliary_device_id *id)
+{
+	struct ti_sn65dsi86 *pdata = dev_get_drvdata(adev->dev.parent);
+
+	pdata->pchip.dev = pdata->dev;
+	pdata->pchip.ops = &ti_sn_pwm_ops;
+	pdata->pchip.base = -1;
+	pdata->pchip.npwm = 1;
+	pdata->pchip.of_xlate = of_pwm_single_xlate;
+	pdata->pchip.of_pwm_n_cells = 1;
+
+	return pwmchip_add(&pdata->pchip);
+}
+
+static void ti_sn_pwm_remove(struct auxiliary_device *adev)
+{
+	struct ti_sn65dsi86 *pdata = dev_get_drvdata(adev->dev.parent);
+
+	pwmchip_remove(&pdata->pchip);
+
+	if (pdata->pwm_enabled)
+		pm_runtime_put_sync(pdata->dev);
+}
+
+static const struct auxiliary_device_id ti_sn_pwm_id_table[] = {
+	{ .name = "ti_sn65dsi86.pwm", },
+	{},
+};
+
+static struct auxiliary_driver ti_sn_pwm_driver = {
+	.name = "pwm",
+	.probe = ti_sn_pwm_probe,
+	.remove = ti_sn_pwm_remove,
+	.id_table = ti_sn_pwm_id_table,
+};
+
+static int __init ti_sn_pwm_register(void)
+{
+	return auxiliary_driver_register(&ti_sn_pwm_driver);
+}
+
+static void ti_sn_pwm_unregister(void)
+{
+	auxiliary_driver_unregister(&ti_sn_pwm_driver);
+}
+
+#else
+static inline int ti_sn_pwm_pin_request(struct ti_sn65dsi86 *pdata) { return 0; }
+static inline void ti_sn_pwm_pin_release(struct ti_sn65dsi86 *pdata) {}
+
+static inline int ti_sn_pwm_register(void) { return 0; }
+static inline void ti_sn_pwm_unregister(void) {}
+#endif
+
 #if defined(CONFIG_OF_GPIO)
 
 static int tn_sn_bridge_of_xlate(struct gpio_chip *chip,
@@ -1176,10 +1442,26 @@ static int ti_sn_bridge_gpio_direction_output(struct gpio_chip *chip,
 	return ret;
 }
 
+static int ti_sn_bridge_gpio_request(struct gpio_chip *chip, unsigned int offset)
+{
+	struct ti_sn65dsi86 *pdata = gpiochip_get_data(chip);
+
+	if (offset == SN_PWM_GPIO_IDX)
+		return ti_sn_pwm_pin_request(pdata);
+
+	return 0;
+}
+
+
 static void ti_sn_bridge_gpio_free(struct gpio_chip *chip, unsigned int offset)
 {
+	struct ti_sn65dsi86 *pdata = gpiochip_get_data(chip);
+
 	/* We won't keep pm_runtime if we're input, so switch there on free */
 	ti_sn_bridge_gpio_direction_input(chip, offset);
+
+	if (offset == SN_PWM_GPIO_IDX)
+		ti_sn_pwm_pin_release(pdata);
 }
 
 static const char * const ti_sn_bridge_gpio_names[SN_NUM_GPIOS] = {
@@ -1201,6 +1483,7 @@ static int ti_sn_gpio_probe(struct auxiliary_device *adev,
 	pdata->gchip.owner = THIS_MODULE;
 	pdata->gchip.of_xlate = tn_sn_bridge_of_xlate;
 	pdata->gchip.of_gpio_n_cells = 2;
+	pdata->gchip.request = ti_sn_bridge_gpio_request;
 	pdata->gchip.free = ti_sn_bridge_gpio_free;
 	pdata->gchip.get_direction = ti_sn_bridge_gpio_get_direction;
 	pdata->gchip.direction_input = ti_sn_bridge_gpio_direction_input;
@@ -1500,6 +1783,12 @@ static int ti_sn65dsi86_probe(struct i2c_client *client,
 			return ret;
 	}
 
+	if (IS_ENABLED(CONFIG_PWM)) {
+		ret = ti_sn65dsi86_add_aux_device(pdata, &pdata->pwm_aux, "pwm");
+		if (ret)
+			return ret;
+	}
+
 	/*
 	 * NOTE: At the end of the AUX channel probe we'll add the aux device
 	 * for the bridge. This is because the bridge can't be used until the
@@ -1543,10 +1832,14 @@ static int __init ti_sn65dsi86_init(void)
 	if (ret)
 		goto err_main_was_registered;
 
-	ret = auxiliary_driver_register(&ti_sn_aux_driver);
+	ret = ti_sn_pwm_register();
 	if (ret)
 		goto err_gpio_was_registered;
 
+	ret = auxiliary_driver_register(&ti_sn_aux_driver);
+	if (ret)
+		goto err_pwm_was_registered;
+
 	ret = auxiliary_driver_register(&ti_sn_bridge_driver);
 	if (ret)
 		goto err_aux_was_registered;
@@ -1555,6 +1848,8 @@ static int __init ti_sn65dsi86_init(void)
 
 err_aux_was_registered:
 	auxiliary_driver_unregister(&ti_sn_aux_driver);
+err_pwm_was_registered:
+	ti_sn_pwm_unregister();
 err_gpio_was_registered:
 	ti_sn_gpio_unregister();
 err_main_was_registered:
@@ -1568,6 +1863,7 @@ static void __exit ti_sn65dsi86_exit(void)
 {
 	auxiliary_driver_unregister(&ti_sn_bridge_driver);
 	auxiliary_driver_unregister(&ti_sn_aux_driver);
+	ti_sn_pwm_unregister();
 	ti_sn_gpio_unregister();
 	i2c_del_driver(&ti_sn65dsi86_driver);
 }
-- 
2.31.0


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v2 2/2] drm/bridge: ti-sn65dsi86: Implement the pwm_chip
  2021-06-15 23:18 ` [PATCH v2 2/2] drm/bridge: ti-sn65dsi86: Implement the pwm_chip Bjorn Andersson
@ 2021-06-16  7:56   ` Uwe Kleine-König
  2021-06-17  3:22     ` Bjorn Andersson
  0 siblings, 1 reply; 9+ messages in thread
From: Uwe Kleine-König @ 2021-06-16  7:56 UTC (permalink / raw)
  To: Bjorn Andersson
  Cc: Andrzej Hajda, Neil Armstrong, Robert Foss, Laurent Pinchart,
	Jonas Karlman, Jernej Skrabec, David Airlie, Daniel Vetter,
	Thierry Reding, Lee Jones, Doug Anderson, dri-devel,
	linux-kernel, linux-pwm

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

Hello Bjorn,

On Tue, Jun 15, 2021 at 06:18:28PM -0500, Bjorn Andersson wrote:
> The SN65DSI86 provides the ability to supply a PWM signal on GPIO 4,
> with the primary purpose of controlling the backlight of the attached
> panel. Add an implementation that exposes this using the standard PWM
> framework, to allow e.g. pwm-backlight to expose this to the user.
> 
> Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> ---
> 
> Changes since v1:
> - Rebased ontop of Doug's auxiliary_bus patches
> - Reworked the math, per Uwe's request
> - Added pwm_chip->get_state and made sure it's happy (only tested with a few
>   limited periods, such as 1kHz)
> 
>  drivers/gpu/drm/bridge/ti-sn65dsi86.c | 298 +++++++++++++++++++++++++-
>  1 file changed, 297 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> index 5d712c8c3c3b..8f11c9b2da48 100644
> --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> @@ -4,6 +4,7 @@
>   * datasheet: https://www.ti.com/lit/ds/symlink/sn65dsi86.pdf
>   */
>  
> +#include <linux/atomic.h>
>  #include <linux/auxiliary_bus.h>
>  #include <linux/bits.h>
>  #include <linux/clk.h>
> @@ -15,6 +16,7 @@
>  #include <linux/module.h>
>  #include <linux/of_graph.h>
>  #include <linux/pm_runtime.h>
> +#include <linux/pwm.h>
>  #include <linux/regmap.h>
>  #include <linux/regulator/consumer.h>
>  
> @@ -91,6 +93,13 @@
>  #define SN_ML_TX_MODE_REG			0x96
>  #define  ML_TX_MAIN_LINK_OFF			0
>  #define  ML_TX_NORMAL_MODE			BIT(0)
> +#define SN_PWM_PRE_DIV_REG			0xA0
> +#define SN_BACKLIGHT_SCALE_REG			0xA1
> +#define  BACKLIGHT_SCALE_MAX			0xFFFF
> +#define SN_BACKLIGHT_REG			0xA3
> +#define SN_PWM_EN_INV_REG			0xA5
> +#define  SN_PWM_INV_MASK			BIT(0)
> +#define  SN_PWM_EN_MASK				BIT(1)
>  #define SN_AUX_CMD_STATUS_REG			0xF4
>  #define  AUX_IRQ_STATUS_AUX_RPLY_TOUT		BIT(3)
>  #define  AUX_IRQ_STATUS_AUX_SHORT		BIT(5)
> @@ -113,11 +122,14 @@
>  
>  #define SN_LINK_TRAINING_TRIES		10
>  
> +#define SN_PWM_GPIO_IDX			3 /* 4th GPIO */
> +
>  /**
>   * struct ti_sn65dsi86 - Platform data for ti-sn65dsi86 driver.
>   * @bridge_aux:   AUX-bus sub device for MIPI-to-eDP bridge functionality.
>   * @gpio_aux:     AUX-bus sub device for GPIO controller functionality.
>   * @aux_aux:      AUX-bus sub device for eDP AUX channel functionality.
> + * @pwm_aux:      AUX-bus sub device for PWM controller functionality.
>   *
>   * @dev:          Pointer to the top level (i2c) device.
>   * @regmap:       Regmap for accessing i2c.
> @@ -145,11 +157,17 @@
>   *                bitmap so we can do atomic ops on it without an extra
>   *                lock so concurrent users of our 4 GPIOs don't stomp on
>   *                each other's read-modify-write.
> + *
> + * @pchip:        pwm_chip if the PWM is exposed.
> + * @pwm_enabled:  Used to track if the PWM signal is currently enabled.
> + * @pwm_refclk_freq: Cache for the reference clock input to the PWM.
> + * @pwm_pin_busy: Track if GPIO4 is currently requested for GPIO or PWM.
>   */
>  struct ti_sn65dsi86 {
>  	struct auxiliary_device		bridge_aux;
>  	struct auxiliary_device		gpio_aux;
>  	struct auxiliary_device		aux_aux;
> +	struct auxiliary_device		pwm_aux;
>  
>  	struct device			*dev;
>  	struct regmap			*regmap;
> @@ -172,6 +190,12 @@ struct ti_sn65dsi86 {
>  	struct gpio_chip		gchip;
>  	DECLARE_BITMAP(gchip_output, SN_NUM_GPIOS);
>  #endif
> +#if defined(CONFIG_PWM)
> +	struct pwm_chip			pchip;
> +	bool				pwm_enabled;
> +	unsigned int			pwm_refclk_freq;
> +	atomic_t			pwm_pin_busy;
> +#endif
>  };
>  
>  static const struct regmap_range ti_sn65dsi86_volatile_ranges[] = {
> @@ -190,6 +214,25 @@ static const struct regmap_config ti_sn65dsi86_regmap_config = {
>  	.cache_type = REGCACHE_NONE,
>  };
>  
> +static int ti_sn65dsi86_read_u16(struct ti_sn65dsi86 *pdata,
> +				 unsigned int reg, u16 *val)
> +{
> +	unsigned int tmp;
> +	int ret;
> +
> +	ret = regmap_read(pdata->regmap, reg, &tmp);
> +	if (ret)
> +		return ret;
> +	*val = tmp;
> +
> +	ret = regmap_read(pdata->regmap, reg + 1, &tmp);
> +	if (ret)
> +		return ret;
> +	*val |= tmp << 8;
> +
> +	return 0;
> +}
> +
>  static void ti_sn65dsi86_write_u16(struct ti_sn65dsi86 *pdata,
>  				   unsigned int reg, u16 val)
>  {
> @@ -253,6 +296,14 @@ static void ti_sn_bridge_set_refclk_freq(struct ti_sn65dsi86 *pdata)
>  
>  	regmap_update_bits(pdata->regmap, SN_DPPLL_SRC_REG, REFCLK_FREQ_MASK,
>  			   REFCLK_FREQ(i));
> +
> +#if defined(CONFIG_PWM)
> +	/*
> +	 * The PWM refclk is based on the value written to SN_DPPLL_SRC_REG,
> +	 * regardless of its actual sourcing.
> +	 */
> +	pdata->pwm_refclk_freq = ti_sn_bridge_refclk_lut[i];
> +#endif
>  }
>  
>  static void ti_sn65dsi86_enable_comms(struct ti_sn65dsi86 *pdata)
> @@ -1044,6 +1095,221 @@ static int ti_sn_bridge_parse_dsi_host(struct ti_sn65dsi86 *pdata)
>  	return 0;
>  }
>  
> +#if defined(CONFIG_PWM)
> +static int ti_sn_pwm_pin_request(struct ti_sn65dsi86 *pdata)
> +{
> +	return atomic_xchg(&pdata->pwm_pin_busy, 1) ? -EBUSY : 0;
> +}
> +
> +static void ti_sn_pwm_pin_release(struct ti_sn65dsi86 *pdata)
> +{
> +	atomic_set(&pdata->pwm_pin_busy, 0);
> +}
> +
> +static struct ti_sn65dsi86 *pwm_chip_to_ti_sn_bridge(struct pwm_chip *chip)
> +{
> +	return container_of(chip, struct ti_sn65dsi86, pchip);
> +}
> +
> +static int ti_sn_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm)
> +{
> +	struct ti_sn65dsi86 *pdata = pwm_chip_to_ti_sn_bridge(chip);
> +
> +	return ti_sn_pwm_pin_request(pdata);
> +}
> +
> +static void ti_sn_pwm_free(struct pwm_chip *chip, struct pwm_device *pwm)
> +{
> +	struct ti_sn65dsi86 *pdata = pwm_chip_to_ti_sn_bridge(chip);
> +
> +	ti_sn_pwm_pin_release(pdata);
> +}
> +
> +static int ti_sn_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> +			   const struct pwm_state *state)
> +{
> +	struct ti_sn65dsi86 *pdata = pwm_chip_to_ti_sn_bridge(chip);
> +	unsigned int pwm_en_inv;
> +	unsigned int backlight;
> +	unsigned int pre_div;
> +	unsigned int scale;
> +	int ret;
> +
> +	if (!pdata->pwm_enabled) {
> +		ret = pm_runtime_get_sync(pdata->dev);
> +		if (ret < 0)
> +			return ret;
> +
> +		ret = regmap_update_bits(pdata->regmap, SN_GPIO_CTRL_REG,
> +				SN_GPIO_MUX_MASK << (2 * SN_PWM_GPIO_IDX),
> +				SN_GPIO_MUX_SPECIAL << (2 * SN_PWM_GPIO_IDX));
> +		if (ret) {
> +			dev_err(pdata->dev, "failed to mux in PWM function\n");
> +			goto out;
> +		}

Do you need to do this even if state->enabled is false? Does this
already modify the output pin?

> +	}
> +
> +	if (state->enabled) {
> +		/*
> +		 * Per the datasheet the PWM frequency is given by:
> +		 *
> +		 *   PWM_FREQ = REFCLK_FREQ / (PWM_PRE_DIV * BACKLIGHT_SCALE + 1)
> +		 *
> +		 * which can be rewritten:
> +		 *
> +		 *   T_pwm * REFCLK_FREQ - 1 = PWM_PRE_DIV * BACKLIGHT_SCALE
> +		 *
> +		 * In order to keep BACKLIGHT_SCALE within its 16 bits, PWM_PRE_DIV
> +		 * must be:
> +		 *
> +		 *   PWM_PRE_DIV >= (T_pwm * REFCLK_FREQ - 1) / BACKLIGHT_SCALE_MAX;
> +		 *
> +		 * To simplify the search and optimize the resolution of the PWM, the
> +		 * lowest possible PWM_PRE_DIV is used. Finally the scale is calculated
> +		 * as:
> +		 *
> +		 *   BACKLIGHT_SCALE = (T_pwm * REFCLK_FREQ - 1) / PWM_PRE_DIV
> +		 *
> +		 * Here T_pwm is represented in seconds, so appropriate scaling to
> +		 * nanoseconds is necessary.
> +		 */

Very nice.

> +		pre_div = DIV_ROUND_UP((state->period * pdata->pwm_refclk_freq - 1),
> +				       (NSEC_PER_SEC * BACKLIGHT_SCALE_MAX));

		if (pre_div > 0xffff)
			pre_div = 0xffff;

is needed here. (Assuming 0xffff is the bigest valid value for PRE_DIV.)

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

There is something wrong here. Consider:

	pdata->pwm_refclk_freq = 3333334
	state->period = 100000
	state->duty_cycle = 600

then you calculate:

	pre_div = 1
	scale = 333

which yields an actual period of 100199.98 ns. However you should get a
period less or equal than the requested period.

It took me some time to spot the problem: Only state->period *
pdata->pwm_refclk_freq must be divided by NSEC_PER_SEC, but not the -1.

So the right formula is:

	scale = (state->period * pdata->pwm_refclk_freq - NSEC_PER_SEC) / (NSEC_PER_SEC * pre_div);

(but you have to pay attention, the dividend might be negative in this
formula).

> +		/*
> +		 * The duty ratio is given as:
> +		 *
> +		 *   duty = BACKLIGHT / (BACKLIGHT_SCALE + 1)
> +		 */
> +		backlight = state->duty_cycle * (scale + 1) / state->period;

Lets continue the above example with the fixed calculation. So we have:

	pdata->pwm_refclk_freq = 3333334
	state->period = 100000 [ns]
	state->duty_cycle = 600
	scale = 332

so the actually emitted period = 99899.98002000399 ns

Now you calculate:

	backlight = 1

which yields an actual duty_cycle of 299.99994 ns, with backlight = 2
you would get an actual duty_cycle of 599.99988 ns, which is better. The
culprit here is that you divide by state->period but instead should
divide by the actual period.

> +
> +		ret = regmap_write(pdata->regmap, SN_PWM_PRE_DIV_REG, pre_div);
> +		if (ret) {
> +			dev_err(pdata->dev, "failed to update PWM_PRE_DIV\n");
> +			goto out;
> +		}
> +
> +		ti_sn65dsi86_write_u16(pdata, SN_BACKLIGHT_SCALE_REG, scale);
> +		ti_sn65dsi86_write_u16(pdata, SN_BACKLIGHT_REG, backlight);

How does the output behave between these register writes? Can it happen
that it emits a wave for corresponding to (e.g.) the new pre_div value
but the old scale and backlight?


> +	}
> +
> +	pwm_en_inv = FIELD_PREP(SN_PWM_EN_MASK, !!state->enabled) |
> +		     FIELD_PREP(SN_PWM_INV_MASK, state->polarity == PWM_POLARITY_INVERSED);
> +	ret = regmap_write(pdata->regmap, SN_PWM_EN_INV_REG, pwm_en_inv);
> +	if (ret) {
> +		dev_err(pdata->dev, "failed to update PWM_EN/PWM_INV\n");
> +		goto out;
> +	}
> +
> +	pdata->pwm_enabled = !!state->enabled;
> +out:
> +
> +	if (!pdata->pwm_enabled)
> +		pm_runtime_put_sync(pdata->dev);
> +
> +	return ret;
> +}
> +
> +static void ti_sn_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
> +				struct pwm_state *state)
> +{
> +	struct ti_sn65dsi86 *pdata = pwm_chip_to_ti_sn_bridge(chip);
> +	unsigned int pwm_en_inv;
> +	unsigned int pre_div;
> +	u16 backlight;
> +	u16 scale;
> +	int ret;
> +
> +	ret = regmap_read(pdata->regmap, SN_PWM_EN_INV_REG, &pwm_en_inv);
> +	if (ret)
> +		return;
> +
> +	ret = ti_sn65dsi86_read_u16(pdata, SN_BACKLIGHT_SCALE_REG, &scale);
> +	if (ret)
> +		return;
> +
> +	ret = ti_sn65dsi86_read_u16(pdata, SN_BACKLIGHT_REG, &backlight);
> +	if (ret)
> +		return;
> +
> +	ret = regmap_read(pdata->regmap, SN_PWM_PRE_DIV_REG, &pre_div);
> +	if (ret)
> +		return;
> +
> +	state->enabled = FIELD_GET(SN_PWM_EN_MASK, pwm_en_inv);
> +	if (FIELD_GET(SN_PWM_INV_MASK, pwm_en_inv))
> +		state->polarity = PWM_POLARITY_INVERSED;
> +	else
> +		state->polarity = PWM_POLARITY_NORMAL;
> +
> +	state->period = NSEC_PER_SEC * (pre_div * scale + 1) / pdata->pwm_refclk_freq;

round up here please. Then applying the result of .get_state() is
a noop (as it should be).

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

I find it surprising that the actual duty_cycle is:

	  state->period * backlight
	  -------------------------
	          scale + 1

          pre_div * scale + 1
	= -------------------
	    refclk * scale

where scale occurs twice. Can you confirm this to be right?

> +}
> +
> +static const struct pwm_ops ti_sn_pwm_ops = {
> +	.request = ti_sn_pwm_request,
> +	.free = ti_sn_pwm_free,
> +	.apply = ti_sn_pwm_apply,
> +	.get_state = ti_sn_pwm_get_state,
> +	.owner = THIS_MODULE,
> +};
> +
> +static int ti_sn_pwm_probe(struct auxiliary_device *adev,
> +		const struct auxiliary_device_id *id)
> +{
> +	struct ti_sn65dsi86 *pdata = dev_get_drvdata(adev->dev.parent);
> +
> +	pdata->pchip.dev = pdata->dev;
> +	pdata->pchip.ops = &ti_sn_pwm_ops;
> +	pdata->pchip.base = -1;

base shouldn't be set since

	f9a8ee8c8bcd (pwm: Always allocate PWM chip base ID dynamically)

> +	pdata->pchip.npwm = 1;
> +	pdata->pchip.of_xlate = of_pwm_single_xlate;
> +	pdata->pchip.of_pwm_n_cells = 1;
> +
> +	return pwmchip_add(&pdata->pchip);
> +}

Best regards
Uwe

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v2 2/2] drm/bridge: ti-sn65dsi86: Implement the pwm_chip
  2021-06-16  7:56   ` Uwe Kleine-König
@ 2021-06-17  3:22     ` Bjorn Andersson
  2021-06-17  6:24       ` Uwe Kleine-König
  0 siblings, 1 reply; 9+ messages in thread
From: Bjorn Andersson @ 2021-06-17  3:22 UTC (permalink / raw)
  To: Uwe Kleine-K?nig
  Cc: Andrzej Hajda, Neil Armstrong, Robert Foss, Laurent Pinchart,
	Jonas Karlman, Jernej Skrabec, David Airlie, Daniel Vetter,
	Thierry Reding, Lee Jones, Doug Anderson, dri-devel,
	linux-kernel, linux-pwm

On Wed 16 Jun 02:56 CDT 2021, Uwe Kleine-K?nig wrote:

> Hello Bjorn,
> 
> On Tue, Jun 15, 2021 at 06:18:28PM -0500, Bjorn Andersson wrote:
> > The SN65DSI86 provides the ability to supply a PWM signal on GPIO 4,
> > with the primary purpose of controlling the backlight of the attached
> > panel. Add an implementation that exposes this using the standard PWM
> > framework, to allow e.g. pwm-backlight to expose this to the user.
> > 
> > Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> > ---
> > 
> > Changes since v1:
> > - Rebased ontop of Doug's auxiliary_bus patches
> > - Reworked the math, per Uwe's request
> > - Added pwm_chip->get_state and made sure it's happy (only tested with a few
> >   limited periods, such as 1kHz)
> > 
> >  drivers/gpu/drm/bridge/ti-sn65dsi86.c | 298 +++++++++++++++++++++++++-
> >  1 file changed, 297 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> > index 5d712c8c3c3b..8f11c9b2da48 100644
> > --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> > +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> > @@ -4,6 +4,7 @@
> >   * datasheet: https://www.ti.com/lit/ds/symlink/sn65dsi86.pdf
> >   */
> >  
> > +#include <linux/atomic.h>
> >  #include <linux/auxiliary_bus.h>
> >  #include <linux/bits.h>
> >  #include <linux/clk.h>
> > @@ -15,6 +16,7 @@
> >  #include <linux/module.h>
> >  #include <linux/of_graph.h>
> >  #include <linux/pm_runtime.h>
> > +#include <linux/pwm.h>
> >  #include <linux/regmap.h>
> >  #include <linux/regulator/consumer.h>
> >  
> > @@ -91,6 +93,13 @@
> >  #define SN_ML_TX_MODE_REG			0x96
> >  #define  ML_TX_MAIN_LINK_OFF			0
> >  #define  ML_TX_NORMAL_MODE			BIT(0)
> > +#define SN_PWM_PRE_DIV_REG			0xA0
> > +#define SN_BACKLIGHT_SCALE_REG			0xA1
> > +#define  BACKLIGHT_SCALE_MAX			0xFFFF
> > +#define SN_BACKLIGHT_REG			0xA3
> > +#define SN_PWM_EN_INV_REG			0xA5
> > +#define  SN_PWM_INV_MASK			BIT(0)
> > +#define  SN_PWM_EN_MASK				BIT(1)
> >  #define SN_AUX_CMD_STATUS_REG			0xF4
> >  #define  AUX_IRQ_STATUS_AUX_RPLY_TOUT		BIT(3)
> >  #define  AUX_IRQ_STATUS_AUX_SHORT		BIT(5)
> > @@ -113,11 +122,14 @@
> >  
> >  #define SN_LINK_TRAINING_TRIES		10
> >  
> > +#define SN_PWM_GPIO_IDX			3 /* 4th GPIO */
> > +
> >  /**
> >   * struct ti_sn65dsi86 - Platform data for ti-sn65dsi86 driver.
> >   * @bridge_aux:   AUX-bus sub device for MIPI-to-eDP bridge functionality.
> >   * @gpio_aux:     AUX-bus sub device for GPIO controller functionality.
> >   * @aux_aux:      AUX-bus sub device for eDP AUX channel functionality.
> > + * @pwm_aux:      AUX-bus sub device for PWM controller functionality.
> >   *
> >   * @dev:          Pointer to the top level (i2c) device.
> >   * @regmap:       Regmap for accessing i2c.
> > @@ -145,11 +157,17 @@
> >   *                bitmap so we can do atomic ops on it without an extra
> >   *                lock so concurrent users of our 4 GPIOs don't stomp on
> >   *                each other's read-modify-write.
> > + *
> > + * @pchip:        pwm_chip if the PWM is exposed.
> > + * @pwm_enabled:  Used to track if the PWM signal is currently enabled.
> > + * @pwm_refclk_freq: Cache for the reference clock input to the PWM.
> > + * @pwm_pin_busy: Track if GPIO4 is currently requested for GPIO or PWM.
> >   */
> >  struct ti_sn65dsi86 {
> >  	struct auxiliary_device		bridge_aux;
> >  	struct auxiliary_device		gpio_aux;
> >  	struct auxiliary_device		aux_aux;
> > +	struct auxiliary_device		pwm_aux;
> >  
> >  	struct device			*dev;
> >  	struct regmap			*regmap;
> > @@ -172,6 +190,12 @@ struct ti_sn65dsi86 {
> >  	struct gpio_chip		gchip;
> >  	DECLARE_BITMAP(gchip_output, SN_NUM_GPIOS);
> >  #endif
> > +#if defined(CONFIG_PWM)
> > +	struct pwm_chip			pchip;
> > +	bool				pwm_enabled;
> > +	unsigned int			pwm_refclk_freq;
> > +	atomic_t			pwm_pin_busy;
> > +#endif
> >  };
> >  
> >  static const struct regmap_range ti_sn65dsi86_volatile_ranges[] = {
> > @@ -190,6 +214,25 @@ static const struct regmap_config ti_sn65dsi86_regmap_config = {
> >  	.cache_type = REGCACHE_NONE,
> >  };
> >  
> > +static int ti_sn65dsi86_read_u16(struct ti_sn65dsi86 *pdata,
> > +				 unsigned int reg, u16 *val)
> > +{
> > +	unsigned int tmp;
> > +	int ret;
> > +
> > +	ret = regmap_read(pdata->regmap, reg, &tmp);
> > +	if (ret)
> > +		return ret;
> > +	*val = tmp;
> > +
> > +	ret = regmap_read(pdata->regmap, reg + 1, &tmp);
> > +	if (ret)
> > +		return ret;
> > +	*val |= tmp << 8;
> > +
> > +	return 0;
> > +}
> > +
> >  static void ti_sn65dsi86_write_u16(struct ti_sn65dsi86 *pdata,
> >  				   unsigned int reg, u16 val)
> >  {
> > @@ -253,6 +296,14 @@ static void ti_sn_bridge_set_refclk_freq(struct ti_sn65dsi86 *pdata)
> >  
> >  	regmap_update_bits(pdata->regmap, SN_DPPLL_SRC_REG, REFCLK_FREQ_MASK,
> >  			   REFCLK_FREQ(i));
> > +
> > +#if defined(CONFIG_PWM)
> > +	/*
> > +	 * The PWM refclk is based on the value written to SN_DPPLL_SRC_REG,
> > +	 * regardless of its actual sourcing.
> > +	 */
> > +	pdata->pwm_refclk_freq = ti_sn_bridge_refclk_lut[i];
> > +#endif
> >  }
> >  
> >  static void ti_sn65dsi86_enable_comms(struct ti_sn65dsi86 *pdata)
> > @@ -1044,6 +1095,221 @@ static int ti_sn_bridge_parse_dsi_host(struct ti_sn65dsi86 *pdata)
> >  	return 0;
> >  }
> >  
> > +#if defined(CONFIG_PWM)
> > +static int ti_sn_pwm_pin_request(struct ti_sn65dsi86 *pdata)
> > +{
> > +	return atomic_xchg(&pdata->pwm_pin_busy, 1) ? -EBUSY : 0;
> > +}
> > +
> > +static void ti_sn_pwm_pin_release(struct ti_sn65dsi86 *pdata)
> > +{
> > +	atomic_set(&pdata->pwm_pin_busy, 0);
> > +}
> > +
> > +static struct ti_sn65dsi86 *pwm_chip_to_ti_sn_bridge(struct pwm_chip *chip)
> > +{
> > +	return container_of(chip, struct ti_sn65dsi86, pchip);
> > +}
> > +
> > +static int ti_sn_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm)
> > +{
> > +	struct ti_sn65dsi86 *pdata = pwm_chip_to_ti_sn_bridge(chip);
> > +
> > +	return ti_sn_pwm_pin_request(pdata);
> > +}
> > +
> > +static void ti_sn_pwm_free(struct pwm_chip *chip, struct pwm_device *pwm)
> > +{
> > +	struct ti_sn65dsi86 *pdata = pwm_chip_to_ti_sn_bridge(chip);
> > +
> > +	ti_sn_pwm_pin_release(pdata);
> > +}
> > +
> > +static int ti_sn_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> > +			   const struct pwm_state *state)
> > +{
> > +	struct ti_sn65dsi86 *pdata = pwm_chip_to_ti_sn_bridge(chip);
> > +	unsigned int pwm_en_inv;
> > +	unsigned int backlight;
> > +	unsigned int pre_div;
> > +	unsigned int scale;
> > +	int ret;
> > +
> > +	if (!pdata->pwm_enabled) {
> > +		ret = pm_runtime_get_sync(pdata->dev);
> > +		if (ret < 0)
> > +			return ret;
> > +
> > +		ret = regmap_update_bits(pdata->regmap, SN_GPIO_CTRL_REG,
> > +				SN_GPIO_MUX_MASK << (2 * SN_PWM_GPIO_IDX),
> > +				SN_GPIO_MUX_SPECIAL << (2 * SN_PWM_GPIO_IDX));
> > +		if (ret) {
> > +			dev_err(pdata->dev, "failed to mux in PWM function\n");
> > +			goto out;
> > +		}
> 
> Do you need to do this even if state->enabled is false?

I presume I should be able to explicitly mux in the GPIO function and
configure that to output low. But I am not able to find anything in the
data sheet that would indicate this to be preferred.

> Does this already modify the output pin?
> 

Yes, coming out of reset this pin is configured as input, so switching
the mux here will effectively start driving the pin.

> > +	}
> > +
> > +	if (state->enabled) {
> > +		/*
> > +		 * Per the datasheet the PWM frequency is given by:
> > +		 *
> > +		 *   PWM_FREQ = REFCLK_FREQ / (PWM_PRE_DIV * BACKLIGHT_SCALE + 1)
> > +		 *
> > +		 * which can be rewritten:
> > +		 *
> > +		 *   T_pwm * REFCLK_FREQ - 1 = PWM_PRE_DIV * BACKLIGHT_SCALE
> > +		 *
> > +		 * In order to keep BACKLIGHT_SCALE within its 16 bits, PWM_PRE_DIV
> > +		 * must be:
> > +		 *
> > +		 *   PWM_PRE_DIV >= (T_pwm * REFCLK_FREQ - 1) / BACKLIGHT_SCALE_MAX;
> > +		 *
> > +		 * To simplify the search and optimize the resolution of the PWM, the
> > +		 * lowest possible PWM_PRE_DIV is used. Finally the scale is calculated
> > +		 * as:
> > +		 *
> > +		 *   BACKLIGHT_SCALE = (T_pwm * REFCLK_FREQ - 1) / PWM_PRE_DIV
> > +		 *
> > +		 * Here T_pwm is represented in seconds, so appropriate scaling to
> > +		 * nanoseconds is necessary.
> > +		 */
> 
> Very nice.
> 
> > +		pre_div = DIV_ROUND_UP((state->period * pdata->pwm_refclk_freq - 1),
> > +				       (NSEC_PER_SEC * BACKLIGHT_SCALE_MAX));
> 
> 		if (pre_div > 0xffff)
> 			pre_div = 0xffff;
> 
> is needed here. (Assuming 0xffff is the bigest valid value for PRE_DIV.)
> 

Yes, that makes sense.

> > +		scale = (state->period * pdata->pwm_refclk_freq - 1) / (NSEC_PER_SEC * pre_div);
> 
> There is something wrong here. Consider:
> 
> 	pdata->pwm_refclk_freq = 3333334
> 	state->period = 100000
> 	state->duty_cycle = 600
> 
> then you calculate:
> 
> 	pre_div = 1
> 	scale = 333
> 
> which yields an actual period of 100199.98 ns. However you should get a
> period less or equal than the requested period.
> 
> It took me some time to spot the problem: Only state->period *
> pdata->pwm_refclk_freq must be divided by NSEC_PER_SEC, but not the -1.
> 
> So the right formula is:
> 
> 	scale = (state->period * pdata->pwm_refclk_freq - NSEC_PER_SEC) / (NSEC_PER_SEC * pre_div);
> 

Ahh, you're right! Thanks!

> (but you have to pay attention, the dividend might be negative in this
> formula).
> 

Right and that defines the lower limit for the period, something I don't
handle as this is currently written.

> > +		/*
> > +		 * The duty ratio is given as:
> > +		 *
> > +		 *   duty = BACKLIGHT / (BACKLIGHT_SCALE + 1)
> > +		 */
> > +		backlight = state->duty_cycle * (scale + 1) / state->period;
> 
> Lets continue the above example with the fixed calculation. So we have:
> 
> 	pdata->pwm_refclk_freq = 3333334
> 	state->period = 100000 [ns]
> 	state->duty_cycle = 600
> 	scale = 332
> 
> so the actually emitted period = 99899.98002000399 ns
> 
> Now you calculate:
> 
> 	backlight = 1
> 
> which yields an actual duty_cycle of 299.99994 ns, with backlight = 2
> you would get an actual duty_cycle of 599.99988 ns, which is better. The
> culprit here is that you divide by state->period but instead should
> divide by the actual period.
> 

What do I do about the case where the actual period is lower than the
requested one and thereby the duty cycle becomes larger than the period?

E.g. passing duty_cycle = period = 1,000,000 with a frequency of 3333334
results in a scale of 3332 and duty cycle (over the actual period) of
3333.

> > +
> > +		ret = regmap_write(pdata->regmap, SN_PWM_PRE_DIV_REG, pre_div);
> > +		if (ret) {
> > +			dev_err(pdata->dev, "failed to update PWM_PRE_DIV\n");
> > +			goto out;
> > +		}
> > +
> > +		ti_sn65dsi86_write_u16(pdata, SN_BACKLIGHT_SCALE_REG, scale);
> > +		ti_sn65dsi86_write_u16(pdata, SN_BACKLIGHT_REG, backlight);
> 
> How does the output behave between these register writes? Can it happen
> that it emits a wave for corresponding to (e.g.) the new pre_div value
> but the old scale and backlight?
> 

I don't see anything indicating in the documentation indicating that
these writes would be buffered or similar. Unfortunately, as I said
earlier I don't have any way to access the signal to see for myself.

> 
> > +	}
> > +
> > +	pwm_en_inv = FIELD_PREP(SN_PWM_EN_MASK, !!state->enabled) |
> > +		     FIELD_PREP(SN_PWM_INV_MASK, state->polarity == PWM_POLARITY_INVERSED);
> > +	ret = regmap_write(pdata->regmap, SN_PWM_EN_INV_REG, pwm_en_inv);
> > +	if (ret) {
> > +		dev_err(pdata->dev, "failed to update PWM_EN/PWM_INV\n");
> > +		goto out;
> > +	}
> > +
> > +	pdata->pwm_enabled = !!state->enabled;
> > +out:
> > +
> > +	if (!pdata->pwm_enabled)
> > +		pm_runtime_put_sync(pdata->dev);
> > +
> > +	return ret;
> > +}
> > +
> > +static void ti_sn_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
> > +				struct pwm_state *state)
> > +{
> > +	struct ti_sn65dsi86 *pdata = pwm_chip_to_ti_sn_bridge(chip);
> > +	unsigned int pwm_en_inv;
> > +	unsigned int pre_div;
> > +	u16 backlight;
> > +	u16 scale;
> > +	int ret;
> > +
> > +	ret = regmap_read(pdata->regmap, SN_PWM_EN_INV_REG, &pwm_en_inv);
> > +	if (ret)
> > +		return;
> > +
> > +	ret = ti_sn65dsi86_read_u16(pdata, SN_BACKLIGHT_SCALE_REG, &scale);
> > +	if (ret)
> > +		return;
> > +
> > +	ret = ti_sn65dsi86_read_u16(pdata, SN_BACKLIGHT_REG, &backlight);
> > +	if (ret)
> > +		return;
> > +
> > +	ret = regmap_read(pdata->regmap, SN_PWM_PRE_DIV_REG, &pre_div);
> > +	if (ret)
> > +		return;
> > +
> > +	state->enabled = FIELD_GET(SN_PWM_EN_MASK, pwm_en_inv);
> > +	if (FIELD_GET(SN_PWM_INV_MASK, pwm_en_inv))
> > +		state->polarity = PWM_POLARITY_INVERSED;
> > +	else
> > +		state->polarity = PWM_POLARITY_NORMAL;
> > +
> > +	state->period = NSEC_PER_SEC * (pre_div * scale + 1) / pdata->pwm_refclk_freq;
> 
> round up here please. Then applying the result of .get_state() is
> a noop (as it should be).
> 

Together with the adjustment of the -1 above I can confirm that we get
something that PWM_DEBUG is happy with (over a larger range of tests
input than I previously tested...)

> > +	state->duty_cycle = DIV_ROUND_UP(state->period * backlight, scale + 1);
> 
> I find it surprising that the actual duty_cycle is:
> 
> 	  state->period * backlight
> 	  -------------------------
> 	          scale + 1
> 
>           pre_div * scale + 1
> 	= -------------------
> 	    refclk * scale
> 
> where scale occurs twice. Can you confirm this to be right?
> 

I came to the same conclusion - i.e. that this looks wrong.

As states above, the period of the PWM is:

	pre_div * scale + 1
	-------------------
	       refclk

or:

	T_refclk * (pre_div * scale + 1)

Which I interpret as us having two nested counters ticking based on
refclk. Once we hit pre_div * scale the counter resets, which takes 1
refclk pulse.

But then the duty cycle is described as:

	BACKLIGHT / (BACKLIGHT_SCALE + 1)

which I would say looks like the signal is high pre_div * BACKLIGHT
steps and then it resets at pre_div * (BACKLIGHT_SCALE + 1).

So I don't know what's going on here.


I will take another look tomorrow on why, but the including the + 1 in
the denominator seems to be necessary to keep the duty cycle
idempotent...

> > +}
> > +
> > +static const struct pwm_ops ti_sn_pwm_ops = {
> > +	.request = ti_sn_pwm_request,
> > +	.free = ti_sn_pwm_free,
> > +	.apply = ti_sn_pwm_apply,
> > +	.get_state = ti_sn_pwm_get_state,
> > +	.owner = THIS_MODULE,
> > +};
> > +
> > +static int ti_sn_pwm_probe(struct auxiliary_device *adev,
> > +		const struct auxiliary_device_id *id)
> > +{
> > +	struct ti_sn65dsi86 *pdata = dev_get_drvdata(adev->dev.parent);
> > +
> > +	pdata->pchip.dev = pdata->dev;
> > +	pdata->pchip.ops = &ti_sn_pwm_ops;
> > +	pdata->pchip.base = -1;
> 
> base shouldn't be set since
> 
> 	f9a8ee8c8bcd (pwm: Always allocate PWM chip base ID dynamically)

Thanks, that's nice!


Many thanks for your feedback!

Regards,
Bjorn

> 
> > +	pdata->pchip.npwm = 1;
> > +	pdata->pchip.of_xlate = of_pwm_single_xlate;
> > +	pdata->pchip.of_pwm_n_cells = 1;
> > +
> > +	return pwmchip_add(&pdata->pchip);
> > +}
> 
> Best regards
> Uwe
> 
> -- 
> Pengutronix e.K.                           | Uwe Kleine-König            |
> Industrial Linux Solutions                 | https://www.pengutronix.de/ |



^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v2 2/2] drm/bridge: ti-sn65dsi86: Implement the pwm_chip
  2021-06-17  3:22     ` Bjorn Andersson
@ 2021-06-17  6:24       ` Uwe Kleine-König
  2021-06-17 16:38         ` Bjorn Andersson
  0 siblings, 1 reply; 9+ messages in thread
From: Uwe Kleine-König @ 2021-06-17  6:24 UTC (permalink / raw)
  To: Bjorn Andersson
  Cc: Andrzej Hajda, Neil Armstrong, Robert Foss, Laurent Pinchart,
	Jonas Karlman, Jernej Skrabec, David Airlie, Daniel Vetter,
	Thierry Reding, Lee Jones, Doug Anderson, dri-devel,
	linux-kernel, linux-pwm

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

Hello Bjorn,

On Wed, Jun 16, 2021 at 10:22:17PM -0500, Bjorn Andersson wrote:
> > > +static int ti_sn_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> > > +			   const struct pwm_state *state)
> > > +{
> > > +	struct ti_sn65dsi86 *pdata = pwm_chip_to_ti_sn_bridge(chip);
> > > +	unsigned int pwm_en_inv;
> > > +	unsigned int backlight;
> > > +	unsigned int pre_div;
> > > +	unsigned int scale;
> > > +	int ret;
> > > +
> > > +	if (!pdata->pwm_enabled) {
> > > +		ret = pm_runtime_get_sync(pdata->dev);
> > > +		if (ret < 0)
> > > +			return ret;
> > > +
> > > +		ret = regmap_update_bits(pdata->regmap, SN_GPIO_CTRL_REG,
> > > +				SN_GPIO_MUX_MASK << (2 * SN_PWM_GPIO_IDX),
> > > +				SN_GPIO_MUX_SPECIAL << (2 * SN_PWM_GPIO_IDX));
> > > +		if (ret) {
> > > +			dev_err(pdata->dev, "failed to mux in PWM function\n");
> > > +			goto out;
> > > +		}
> > 
> > Do you need to do this even if state->enabled is false?
> 
> I presume I should be able to explicitly mux in the GPIO function and
> configure that to output low. But I am not able to find anything in the
> data sheet that would indicate this to be preferred.

My question targetted a different case. If the PWM is off
(!pdata->pwm_enabled) and should remain off (state->enabled is false)
you can shortcut here, can you not?

> > Does this already modify the output pin?
> 
> Yes, coming out of reset this pin is configured as input, so switching
> the mux here will effectively start driving the pin.

So please document this in the format the recently added drivers do,
too. See e.g. drivers/pwm/pwm-sifive.c. (The idea is to start that with
" * Limitations:" to make it easy to grep it.)

> > Lets continue the above example with the fixed calculation. So we have:
> > 
> > 	pdata->pwm_refclk_freq = 3333334
> > 	state->period = 100000 [ns]
> > 	state->duty_cycle = 600
> > 	scale = 332
> > 
> > so the actually emitted period = 99899.98002000399 ns
> > 
> > Now you calculate:
> > 
> > 	backlight = 1
> > 
> > which yields an actual duty_cycle of 299.99994 ns, with backlight = 2
> > you would get an actual duty_cycle of 599.99988 ns, which is better. The
> > culprit here is that you divide by state->period but instead should
> > divide by the actual period.
> 
> What do I do about the case where the actual period is lower than the
> requested one and thereby the duty cycle becomes larger than the period?

The general principle is: Pick the biggest possible duty_cycle available
for the just picked period. So in your example you have to clamp it to
period (assuming you can, otherwise pick the next lower possible value).

Best regards
Uwe

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v2 2/2] drm/bridge: ti-sn65dsi86: Implement the pwm_chip
  2021-06-17  6:24       ` Uwe Kleine-König
@ 2021-06-17 16:38         ` Bjorn Andersson
  2021-06-17 16:54           ` Uwe Kleine-König
  0 siblings, 1 reply; 9+ messages in thread
From: Bjorn Andersson @ 2021-06-17 16:38 UTC (permalink / raw)
  To: Uwe Kleine-K?nig
  Cc: Andrzej Hajda, Neil Armstrong, Robert Foss, Laurent Pinchart,
	Jonas Karlman, Jernej Skrabec, David Airlie, Daniel Vetter,
	Thierry Reding, Lee Jones, Doug Anderson, dri-devel,
	linux-kernel, linux-pwm

On Thu 17 Jun 01:24 CDT 2021, Uwe Kleine-K?nig wrote:

> Hello Bjorn,
> 
> On Wed, Jun 16, 2021 at 10:22:17PM -0500, Bjorn Andersson wrote:
> > > > +static int ti_sn_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> > > > +			   const struct pwm_state *state)
> > > > +{
> > > > +	struct ti_sn65dsi86 *pdata = pwm_chip_to_ti_sn_bridge(chip);
> > > > +	unsigned int pwm_en_inv;
> > > > +	unsigned int backlight;
> > > > +	unsigned int pre_div;
> > > > +	unsigned int scale;
> > > > +	int ret;
> > > > +
> > > > +	if (!pdata->pwm_enabled) {
> > > > +		ret = pm_runtime_get_sync(pdata->dev);
> > > > +		if (ret < 0)
> > > > +			return ret;
> > > > +
> > > > +		ret = regmap_update_bits(pdata->regmap, SN_GPIO_CTRL_REG,
> > > > +				SN_GPIO_MUX_MASK << (2 * SN_PWM_GPIO_IDX),
> > > > +				SN_GPIO_MUX_SPECIAL << (2 * SN_PWM_GPIO_IDX));
> > > > +		if (ret) {
> > > > +			dev_err(pdata->dev, "failed to mux in PWM function\n");
> > > > +			goto out;
> > > > +		}
> > > 
> > > Do you need to do this even if state->enabled is false?
> > 
> > I presume I should be able to explicitly mux in the GPIO function and
> > configure that to output low. But I am not able to find anything in the
> > data sheet that would indicate this to be preferred.
> 
> My question targetted a different case. If the PWM is off
> (!pdata->pwm_enabled) and should remain off (state->enabled is false)
> you can shortcut here, can you not?
> 

Right, if we're going off->off then we don't need to touch the hardware.

But am I expected to -EINVAL improper period and duty cycle even though
enabled is false?


And also, what is the supposed behavior of enabled = false? Is it
supposedly equivalent of asking for a duty_cycle of 0?

> > > Does this already modify the output pin?
> > 
> > Yes, coming out of reset this pin is configured as input, so switching
> > the mux here will effectively start driving the pin.
> 
> So please document this in the format the recently added drivers do,
> too. See e.g. drivers/pwm/pwm-sifive.c. (The idea is to start that with
> " * Limitations:" to make it easy to grep it.)
> 

Okay, will do. Although I believe that for this driver it makes sense to
place such comment close to this function, rather than at the top of the
driver.

> > > Lets continue the above example with the fixed calculation. So we have:
> > > 
> > > 	pdata->pwm_refclk_freq = 3333334
> > > 	state->period = 100000 [ns]
> > > 	state->duty_cycle = 600
> > > 	scale = 332
> > > 
> > > so the actually emitted period = 99899.98002000399 ns
> > > 
> > > Now you calculate:
> > > 
> > > 	backlight = 1
> > > 
> > > which yields an actual duty_cycle of 299.99994 ns, with backlight = 2
> > > you would get an actual duty_cycle of 599.99988 ns, which is better. The
> > > culprit here is that you divide by state->period but instead should
> > > divide by the actual period.
> > 
> > What do I do about the case where the actual period is lower than the
> > requested one and thereby the duty cycle becomes larger than the period?
> 
> The general principle is: Pick the biggest possible duty_cycle available
> for the just picked period. So in your example you have to clamp it to
> period (assuming you can, otherwise pick the next lower possible value).
> 

Sounds good.

Thank you,
Bjorn

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



^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v2 2/2] drm/bridge: ti-sn65dsi86: Implement the pwm_chip
  2021-06-17 16:38         ` Bjorn Andersson
@ 2021-06-17 16:54           ` Uwe Kleine-König
  2021-06-17 18:06             ` Bjorn Andersson
  0 siblings, 1 reply; 9+ messages in thread
From: Uwe Kleine-König @ 2021-06-17 16:54 UTC (permalink / raw)
  To: Bjorn Andersson
  Cc: Andrzej Hajda, Neil Armstrong, Robert Foss, Laurent Pinchart,
	Jonas Karlman, Jernej Skrabec, David Airlie, Daniel Vetter,
	Thierry Reding, Lee Jones, Doug Anderson, dri-devel,
	linux-kernel, linux-pwm

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

Hello Bjorn,

On Thu, Jun 17, 2021 at 11:38:26AM -0500, Bjorn Andersson wrote:
> On Thu 17 Jun 01:24 CDT 2021, Uwe Kleine-K?nig wrote:
> > On Wed, Jun 16, 2021 at 10:22:17PM -0500, Bjorn Andersson wrote:
> > > > > +static int ti_sn_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> > > > > +			   const struct pwm_state *state)
> > > > > +{
> > > > > +	struct ti_sn65dsi86 *pdata = pwm_chip_to_ti_sn_bridge(chip);
> > > > > +	unsigned int pwm_en_inv;
> > > > > +	unsigned int backlight;
> > > > > +	unsigned int pre_div;
> > > > > +	unsigned int scale;
> > > > > +	int ret;
> > > > > +
> > > > > +	if (!pdata->pwm_enabled) {
> > > > > +		ret = pm_runtime_get_sync(pdata->dev);
> > > > > +		if (ret < 0)
> > > > > +			return ret;
> > > > > +
> > > > > +		ret = regmap_update_bits(pdata->regmap, SN_GPIO_CTRL_REG,
> > > > > +				SN_GPIO_MUX_MASK << (2 * SN_PWM_GPIO_IDX),
> > > > > +				SN_GPIO_MUX_SPECIAL << (2 * SN_PWM_GPIO_IDX));
> > > > > +		if (ret) {
> > > > > +			dev_err(pdata->dev, "failed to mux in PWM function\n");
> > > > > +			goto out;
> > > > > +		}
> > > > 
> > > > Do you need to do this even if state->enabled is false?
> > > 
> > > I presume I should be able to explicitly mux in the GPIO function and
> > > configure that to output low. But I am not able to find anything in the
> > > data sheet that would indicate this to be preferred.
> > 
> > My question targetted a different case. If the PWM is off
> > (!pdata->pwm_enabled) and should remain off (state->enabled is false)
> > you can shortcut here, can you not?
> 
> Right, if we're going off->off then we don't need to touch the hardware.
> 
> But am I expected to -EINVAL improper period and duty cycle even though
> enabled is false?
> 
> And also, what is the supposed behavior of enabled = false? Is it
> supposedly equivalent of asking for a duty_cycle of 0?

In my book enabled = false is just syntactic sugar to say:
"duty_cycle=0, period=something small". So to answer your questions: if
enabled = false, the consumer doesn't really care about period and
duty_cycle. Some care that the output becomes inactive, some others
don't, so from my POV just emit the inactive level on the output and
ignore period and duty_cycle.

> > > > Does this already modify the output pin?
> > > 
> > > Yes, coming out of reset this pin is configured as input, so switching
> > > the mux here will effectively start driving the pin.
> > 
> > So please document this in the format the recently added drivers do,
> > too. See e.g. drivers/pwm/pwm-sifive.c. (The idea is to start that with
> > " * Limitations:" to make it easy to grep it.)
> > 
> 
> Okay, will do. Although I believe that for this driver it makes sense to
> place such comment close to this function, rather than at the top of the
> driver.

Yes, agreed.

Best regards
Uwe

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v2 2/2] drm/bridge: ti-sn65dsi86: Implement the pwm_chip
  2021-06-17 16:54           ` Uwe Kleine-König
@ 2021-06-17 18:06             ` Bjorn Andersson
  2021-06-18  7:46               ` Uwe Kleine-König
  0 siblings, 1 reply; 9+ messages in thread
From: Bjorn Andersson @ 2021-06-17 18:06 UTC (permalink / raw)
  To: Uwe Kleine-K?nig
  Cc: Andrzej Hajda, Neil Armstrong, Robert Foss, Laurent Pinchart,
	Jonas Karlman, Jernej Skrabec, David Airlie, Daniel Vetter,
	Thierry Reding, Lee Jones, Doug Anderson, dri-devel,
	linux-kernel, linux-pwm

On Thu 17 Jun 11:54 CDT 2021, Uwe Kleine-K?nig wrote:

> Hello Bjorn,
> 
> On Thu, Jun 17, 2021 at 11:38:26AM -0500, Bjorn Andersson wrote:
> > On Thu 17 Jun 01:24 CDT 2021, Uwe Kleine-K?nig wrote:
> > > On Wed, Jun 16, 2021 at 10:22:17PM -0500, Bjorn Andersson wrote:
> > > > > > +static int ti_sn_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> > > > > > +			   const struct pwm_state *state)
> > > > > > +{
> > > > > > +	struct ti_sn65dsi86 *pdata = pwm_chip_to_ti_sn_bridge(chip);
> > > > > > +	unsigned int pwm_en_inv;
> > > > > > +	unsigned int backlight;
> > > > > > +	unsigned int pre_div;
> > > > > > +	unsigned int scale;
> > > > > > +	int ret;
> > > > > > +
> > > > > > +	if (!pdata->pwm_enabled) {
> > > > > > +		ret = pm_runtime_get_sync(pdata->dev);
> > > > > > +		if (ret < 0)
> > > > > > +			return ret;
> > > > > > +
> > > > > > +		ret = regmap_update_bits(pdata->regmap, SN_GPIO_CTRL_REG,
> > > > > > +				SN_GPIO_MUX_MASK << (2 * SN_PWM_GPIO_IDX),
> > > > > > +				SN_GPIO_MUX_SPECIAL << (2 * SN_PWM_GPIO_IDX));
> > > > > > +		if (ret) {
> > > > > > +			dev_err(pdata->dev, "failed to mux in PWM function\n");
> > > > > > +			goto out;
> > > > > > +		}
> > > > > 
> > > > > Do you need to do this even if state->enabled is false?
> > > > 
> > > > I presume I should be able to explicitly mux in the GPIO function and
> > > > configure that to output low. But I am not able to find anything in the
> > > > data sheet that would indicate this to be preferred.
> > > 
> > > My question targetted a different case. If the PWM is off
> > > (!pdata->pwm_enabled) and should remain off (state->enabled is false)
> > > you can shortcut here, can you not?
> > 
> > Right, if we're going off->off then we don't need to touch the hardware.
> > 
> > But am I expected to -EINVAL improper period and duty cycle even though
> > enabled is false?
> > 
> > And also, what is the supposed behavior of enabled = false? Is it
> > supposedly equivalent of asking for a duty_cycle of 0?
> 
> In my book enabled = false is just syntactic sugar to say:
> "duty_cycle=0, period=something small". So to answer your questions: if
> enabled = false, the consumer doesn't really care about period and
> duty_cycle. Some care that the output becomes inactive, some others
> don't, so from my POV just emit the inactive level on the output and
> ignore period and duty_cycle.
> 

Giving this some further thought.

In order to have a known state of the PWM signal we need the sn65dsi86
to be powered. The documentation describes a "suspend mode", but this is
currently not implemented in the driver, so there's a large power cost
coming from just keeping the pin low when disabled.

As such in the current implementation I use state->enabled to also
control if the device should be kept powered, which means that this
follows the backlight power status of the pwm-backlight. Which is
sufficient as the backlight won't be powered when !state->enabled.


For the typical use case the pwm-backlight has some independent control
over gpio and power to toggle the actual backlight. But in the even that
this wouldn't be available I think we need to extend the driver to
implement the suspend mode.

In which case muxing in the PWM function should probably happen at the
time the PWM channel is requested.

This does come at an additional power cost (not as high as keeping the
chip awake though), so (in addition to the effort) I think it's
reasonable to document this as a limitation for now and implement this
as the need arise.

Thanks,
Bjorn

> > > > > Does this already modify the output pin?
> > > > 
> > > > Yes, coming out of reset this pin is configured as input, so switching
> > > > the mux here will effectively start driving the pin.
> > > 
> > > So please document this in the format the recently added drivers do,
> > > too. See e.g. drivers/pwm/pwm-sifive.c. (The idea is to start that with
> > > " * Limitations:" to make it easy to grep it.)
> > > 
> > 
> > Okay, will do. Although I believe that for this driver it makes sense to
> > place such comment close to this function, rather than at the top of the
> > driver.
> 
> Yes, agreed.
> 
> Best regards
> Uwe
> 
> -- 
> Pengutronix e.K.                           | Uwe Kleine-König            |
> Industrial Linux Solutions                 | https://www.pengutronix.de/ |



^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v2 2/2] drm/bridge: ti-sn65dsi86: Implement the pwm_chip
  2021-06-17 18:06             ` Bjorn Andersson
@ 2021-06-18  7:46               ` Uwe Kleine-König
  0 siblings, 0 replies; 9+ messages in thread
From: Uwe Kleine-König @ 2021-06-18  7:46 UTC (permalink / raw)
  To: Bjorn Andersson
  Cc: Andrzej Hajda, Neil Armstrong, Robert Foss, Laurent Pinchart,
	Jonas Karlman, Jernej Skrabec, David Airlie, Daniel Vetter,
	Thierry Reding, Lee Jones, Doug Anderson, dri-devel,
	linux-kernel, linux-pwm

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

Hello Bjorn,

On Thu, Jun 17, 2021 at 01:06:51PM -0500, Bjorn Andersson wrote:
> On Thu 17 Jun 11:54 CDT 2021, Uwe Kleine-K?nig wrote:
> > On Thu, Jun 17, 2021 at 11:38:26AM -0500, Bjorn Andersson wrote:
> > > On Thu 17 Jun 01:24 CDT 2021, Uwe Kleine-K?nig wrote:
> > > > On Wed, Jun 16, 2021 at 10:22:17PM -0500, Bjorn Andersson wrote:
> > > > > > > +static int ti_sn_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> > > > > > > +			   const struct pwm_state *state)
> > > > > > > +{
> > > > > > > +	struct ti_sn65dsi86 *pdata = pwm_chip_to_ti_sn_bridge(chip);
> > > > > > > +	unsigned int pwm_en_inv;
> > > > > > > +	unsigned int backlight;
> > > > > > > +	unsigned int pre_div;
> > > > > > > +	unsigned int scale;
> > > > > > > +	int ret;
> > > > > > > +
> > > > > > > +	if (!pdata->pwm_enabled) {
> > > > > > > +		ret = pm_runtime_get_sync(pdata->dev);
> > > > > > > +		if (ret < 0)
> > > > > > > +			return ret;
> > > > > > > +
> > > > > > > +		ret = regmap_update_bits(pdata->regmap, SN_GPIO_CTRL_REG,
> > > > > > > +				SN_GPIO_MUX_MASK << (2 * SN_PWM_GPIO_IDX),
> > > > > > > +				SN_GPIO_MUX_SPECIAL << (2 * SN_PWM_GPIO_IDX));
> > > > > > > +		if (ret) {
> > > > > > > +			dev_err(pdata->dev, "failed to mux in PWM function\n");
> > > > > > > +			goto out;
> > > > > > > +		}
> > > > > > 
> > > > > > Do you need to do this even if state->enabled is false?
> > > > > 
> > > > > I presume I should be able to explicitly mux in the GPIO function and
> > > > > configure that to output low. But I am not able to find anything in the
> > > > > data sheet that would indicate this to be preferred.
> > > > 
> > > > My question targetted a different case. If the PWM is off
> > > > (!pdata->pwm_enabled) and should remain off (state->enabled is false)
> > > > you can shortcut here, can you not?
> > > 
> > > Right, if we're going off->off then we don't need to touch the hardware.
> > > 
> > > But am I expected to -EINVAL improper period and duty cycle even though
> > > enabled is false?
> > > 
> > > And also, what is the supposed behavior of enabled = false? Is it
> > > supposedly equivalent of asking for a duty_cycle of 0?
> > 
> > In my book enabled = false is just syntactic sugar to say:
> > "duty_cycle=0, period=something small". So to answer your questions: if
> > enabled = false, the consumer doesn't really care about period and
> > duty_cycle. Some care that the output becomes inactive, some others
> > don't, so from my POV just emit the inactive level on the output and
> > ignore period and duty_cycle.
> 
> Giving this some further thought.

Very appreciated, still more as you come to the same conclusions as I do
:-)

> In order to have a known state of the PWM signal we need the sn65dsi86
> to be powered. The documentation describes a "suspend mode", but this is
> currently not implemented in the driver, so there's a large power cost
> coming from just keeping the pin low when disabled.

In the past I already promoted the idea that a disabled PWM should give
no guarantees about the output level. In fact there are several
offenders, the ones I know off-hand are:

 - pwm-imx27 emits a low level independent of the programmed polarity
 - pwm-iqs620a makes the output tristated and so relies on an external
   pull to give the inactive level.
 - pwm-sl28cpld switches to a GPIO mode on disable which isn't
   controlled by the driver

and I assume there are more because before no one actively asked for
and tracked this kind of information.

IMHO a consumer who wants the output to stay inactive should configure

	.enabled = true
	.period = DC (or something low to allow quick reprogramming)
	.duty_cycle = 0

, so there is no loss of functionality and enabled=false should mean the
consumer doesn't care about the output which would allow some lowlevel
drivers to save some more energy. So this makes the API more expressive
because after dropping "disabled results in an inactive output"
consumers can differentiate between "I care about the output staying
inactive" and "I don't care". This in turn allows lowlevel drivers to
better know when they can more aggressively save power and when they
don't.

Back then Thierry didn't like that approach though. (The thread started
with a mail having Message-Id
20180820144357.7206-1-u.kleine-koenig@pengutronix.de, this is missing on
lore.kernel.org however and I didn't find it on another mirror.)

Thierry's arguments were:

 - "An API whose result is an undefined state is useless in my opinion."
   (from Message-Id: 20181009073407.GA5565@ulmo)
   Yes, this is a drawback for some consumers, but it matches reality
   that disabling the PWM counter on some PWM implementations doesn't
   result in an inactive level. And if they care about the output, they
   just use .duty_cycle = 0 + .enabled = true instead.
   In my book changing the semantic fixes a bug because the API promises
   more than some drivers are capable to do (with reasonable effort).

 - "[Emitting the inactive level] also matches what all other drivers,
   and users, assume." (also from Message-Id: 20181009073407.GA5565@ulmo)
   For the drivers this was an assumption, today we know it's wrong.
   Users can be fixed.

Best regards
Uwe

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2021-06-18  7:46 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-06-15 23:18 [PATCH v2 1/2] pwm: Introduce single-PWM of_xlate function Bjorn Andersson
2021-06-15 23:18 ` [PATCH v2 2/2] drm/bridge: ti-sn65dsi86: Implement the pwm_chip Bjorn Andersson
2021-06-16  7:56   ` Uwe Kleine-König
2021-06-17  3:22     ` Bjorn Andersson
2021-06-17  6:24       ` Uwe Kleine-König
2021-06-17 16:38         ` Bjorn Andersson
2021-06-17 16:54           ` Uwe Kleine-König
2021-06-17 18:06             ` Bjorn Andersson
2021-06-18  7:46               ` Uwe Kleine-König

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).