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

[PATCH] 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.

Special thanks to Doug Anderson for suggestions related to the involved
math.

Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
---
 drivers/gpu/drm/bridge/ti-sn65dsi86.c | 202 ++++++++++++++++++++++++++
 1 file changed, 202 insertions(+)

diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
index f27306c51e4d..43c0acba57ab 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/bits.h>
 #include <linux/clk.h>
 #include <linux/debugfs.h>
@@ -14,6 +15,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>
 
@@ -89,6 +91,11 @@
 #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_AUX_CMD_STATUS_REG			0xF4
 #define  AUX_IRQ_STATUS_AUX_RPLY_TOUT		BIT(3)
 #define  AUX_IRQ_STATUS_AUX_SHORT		BIT(5)
@@ -111,6 +118,8 @@
 
 #define SN_LINK_TRAINING_TRIES		10
 
+#define SN_PWM_GPIO			3
+
 /**
  * struct ti_sn_bridge - Platform data for ti-sn65dsi86 driver.
  * @dev:          Pointer to our device.
@@ -162,6 +171,12 @@ struct ti_sn_bridge {
 	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;
+	atomic_t			pwm_pin_busy;
+#endif
 };
 
 static const struct regmap_range ti_sn_bridge_volatile_ranges[] = {
@@ -499,6 +514,14 @@ static void ti_sn_bridge_set_refclk_freq(struct ti_sn_bridge *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 = ti_sn_bridge_refclk_lut[i];
+#endif
 }
 
 static void ti_sn_bridge_set_dsi_rate(struct ti_sn_bridge *pdata)
@@ -981,6 +1004,161 @@ static int ti_sn_bridge_parse_dsi_host(struct ti_sn_bridge *pdata)
 	return 0;
 }
 
+#if defined(CONFIG_PWM)
+static int ti_sn_pwm_pin_request(struct ti_sn_bridge *pdata)
+{
+	return atomic_xchg(&pdata->pwm_pin_busy, 1) ? -EBUSY : 0;
+}
+
+static void ti_sn_pwm_pin_release(struct ti_sn_bridge *pdata)
+{
+	atomic_set(&pdata->pwm_pin_busy, 0);
+}
+
+static struct ti_sn_bridge *
+pwm_chip_to_ti_sn_bridge(struct pwm_chip *chip)
+{
+	return container_of(chip, struct ti_sn_bridge, pchip);
+}
+
+static int ti_sn_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm)
+{
+	struct ti_sn_bridge *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_sn_bridge *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_sn_bridge *pdata = pwm_chip_to_ti_sn_bridge(chip);
+	unsigned int pwm_en_inv;
+	unsigned int backlight;
+	unsigned int pwm_freq;
+	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),
+					 SN_GPIO_MUX_SPECIAL << (2 * SN_PWM_GPIO));
+		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)
+		 *
+		 * In order to find the PWM_FREQ that best suits the requested
+		 * state->period, the PWM_PRE_DIV is calculated with the
+		 * maximum possible number of steps (BACKLIGHT_SCALE_MAX). The
+		 * actual BACKLIGHT_SCALE is then adjusted down to match the
+		 * requested period.
+		 *
+		 * The BACKLIGHT value is then calculated against the
+		 * BACKLIGHT_SCALE, based on the requested duty_cycle and
+		 * period.
+		 */
+		pwm_freq = NSEC_PER_SEC / state->period;
+		pre_div = DIV_ROUND_UP(pdata->pwm_refclk / pwm_freq - 1, BACKLIGHT_SCALE_MAX);
+		scale = (pdata->pwm_refclk / pwm_freq - 1) / pre_div;
+
+		backlight = scale * state->duty_cycle / 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_sn_bridge_write_u16(pdata, SN_BACKLIGHT_SCALE_REG, scale);
+		ti_sn_bridge_write_u16(pdata, SN_BACKLIGHT_REG, backlight);
+	}
+
+	pwm_en_inv = FIELD_PREP(BIT(1), !!state->enabled) |
+		     FIELD_PREP(BIT(0), 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 const struct pwm_ops ti_sn_pwm_ops = {
+	.request = ti_sn_pwm_request,
+	.free = ti_sn_pwm_free,
+	.apply = ti_sn_pwm_apply,
+	.owner = THIS_MODULE,
+};
+
+static struct pwm_device *ti_sn_pwm_of_xlate(struct pwm_chip *pc,
+					     const struct of_phandle_args *args)
+{
+	struct pwm_device *pwm;
+
+	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];
+
+	return pwm;
+}
+
+static int ti_sn_setup_pwmchip(struct ti_sn_bridge *pdata)
+{
+	pdata->pchip.dev = pdata->dev;
+	pdata->pchip.ops = &ti_sn_pwm_ops;
+	pdata->pchip.base = -1;
+	pdata->pchip.npwm = 1;
+	pdata->pchip.of_xlate = ti_sn_pwm_of_xlate;
+	pdata->pchip.of_pwm_n_cells = 1;
+
+	return pwmchip_add(&pdata->pchip);
+}
+
+static void ti_sn_remove_pwmchip(struct ti_sn_bridge *pdata)
+{
+	pwmchip_remove(&pdata->pchip);
+
+	if (pdata->pwm_enabled)
+		pm_runtime_put_sync(pdata->dev);
+}
+#else
+static int ti_sn_pwm_pin_request(struct ti_sn_bridge *pdata) { return 0; }
+static void ti_sn_pwm_pin_release(struct ti_sn_bridge *pdata) {}
+static int ti_sn_setup_pwmchip(struct ti_sn_bridge *pdata) { return 0; }
+static void ti_sn_remove_pwmchip(struct ti_sn_bridge *pdata) {}
+#endif
+
 #if defined(CONFIG_OF_GPIO)
 
 static int tn_sn_bridge_of_xlate(struct gpio_chip *chip,
@@ -1113,10 +1291,25 @@ static int ti_sn_bridge_gpio_direction_output(struct gpio_chip *chip,
 	return ret;
 }
 
+static int ti_sn_bridge_gpio_request(struct gpio_chip *chip, unsigned int offset)
+{
+	struct ti_sn_bridge *pdata = gpiochip_get_data(chip);
+
+	if (offset == SN_PWM_GPIO)
+		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_sn_bridge *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)
+		ti_sn_pwm_pin_release(pdata);
 }
 
 static const char * const ti_sn_bridge_gpio_names[SN_NUM_GPIOS] = {
@@ -1136,6 +1329,7 @@ static int ti_sn_setup_gpio_controller(struct ti_sn_bridge *pdata)
 	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;
@@ -1282,6 +1476,12 @@ static int ti_sn_bridge_probe(struct i2c_client *client,
 		return ret;
 	}
 
+	ret = ti_sn_setup_pwmchip(pdata);
+	if (ret)  {
+		pm_runtime_disable(pdata->dev);
+		return ret;
+	}
+
 	i2c_set_clientdata(client, pdata);
 
 	pdata->aux.name = "ti-sn65dsi86-aux";
@@ -1320,6 +1520,8 @@ static int ti_sn_bridge_remove(struct i2c_client *client)
 
 	drm_bridge_remove(&pdata->bridge);
 
+	ti_sn_remove_pwmchip(pdata);
+
 	return 0;
 }
 
-- 
2.29.2

[PATCH] 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.

Special thanks to Doug Anderson for suggestions related to the involved
math.

Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
---
 drivers/gpu/drm/bridge/ti-sn65dsi86.c | 202 ++++++++++++++++++++++++++
 1 file changed, 202 insertions(+)

diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
index f27306c51e4d..43c0acba57ab 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/bits.h>
 #include <linux/clk.h>
 #include <linux/debugfs.h>
@@ -14,6 +15,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>
 
@@ -89,6 +91,11 @@
 #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_AUX_CMD_STATUS_REG			0xF4
 #define  AUX_IRQ_STATUS_AUX_RPLY_TOUT		BIT(3)
 #define  AUX_IRQ_STATUS_AUX_SHORT		BIT(5)
@@ -111,6 +118,8 @@
 
 #define SN_LINK_TRAINING_TRIES		10
 
+#define SN_PWM_GPIO			3
+
 /**
  * struct ti_sn_bridge - Platform data for ti-sn65dsi86 driver.
  * @dev:          Pointer to our device.
@@ -162,6 +171,12 @@ struct ti_sn_bridge {
 	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;
+	atomic_t			pwm_pin_busy;
+#endif
 };
 
 static const struct regmap_range ti_sn_bridge_volatile_ranges[] = {
@@ -499,6 +514,14 @@ static void ti_sn_bridge_set_refclk_freq(struct ti_sn_bridge *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 = ti_sn_bridge_refclk_lut[i];
+#endif
 }
 
 static void ti_sn_bridge_set_dsi_rate(struct ti_sn_bridge *pdata)
@@ -981,6 +1004,161 @@ static int ti_sn_bridge_parse_dsi_host(struct ti_sn_bridge *pdata)
 	return 0;
 }
 
+#if defined(CONFIG_PWM)
+static int ti_sn_pwm_pin_request(struct ti_sn_bridge *pdata)
+{
+	return atomic_xchg(&pdata->pwm_pin_busy, 1) ? -EBUSY : 0;
+}
+
+static void ti_sn_pwm_pin_release(struct ti_sn_bridge *pdata)
+{
+	atomic_set(&pdata->pwm_pin_busy, 0);
+}
+
+static struct ti_sn_bridge *
+pwm_chip_to_ti_sn_bridge(struct pwm_chip *chip)
+{
+	return container_of(chip, struct ti_sn_bridge, pchip);
+}
+
+static int ti_sn_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm)
+{
+	struct ti_sn_bridge *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_sn_bridge *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_sn_bridge *pdata = pwm_chip_to_ti_sn_bridge(chip);
+	unsigned int pwm_en_inv;
+	unsigned int backlight;
+	unsigned int pwm_freq;
+	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),
+					 SN_GPIO_MUX_SPECIAL << (2 * SN_PWM_GPIO));
+		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)
+		 *
+		 * In order to find the PWM_FREQ that best suits the requested
+		 * state->period, the PWM_PRE_DIV is calculated with the
+		 * maximum possible number of steps (BACKLIGHT_SCALE_MAX). The
+		 * actual BACKLIGHT_SCALE is then adjusted down to match the
+		 * requested period.
+		 *
+		 * The BACKLIGHT value is then calculated against the
+		 * BACKLIGHT_SCALE, based on the requested duty_cycle and
+		 * period.
+		 */
+		pwm_freq = NSEC_PER_SEC / state->period;
+		pre_div = DIV_ROUND_UP(pdata->pwm_refclk / pwm_freq - 1, BACKLIGHT_SCALE_MAX);
+		scale = (pdata->pwm_refclk / pwm_freq - 1) / pre_div;
+
+		backlight = scale * state->duty_cycle / 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_sn_bridge_write_u16(pdata, SN_BACKLIGHT_SCALE_REG, scale);
+		ti_sn_bridge_write_u16(pdata, SN_BACKLIGHT_REG, backlight);
+	}
+
+	pwm_en_inv = FIELD_PREP(BIT(1), !!state->enabled) |
+		     FIELD_PREP(BIT(0), 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 const struct pwm_ops ti_sn_pwm_ops = {
+	.request = ti_sn_pwm_request,
+	.free = ti_sn_pwm_free,
+	.apply = ti_sn_pwm_apply,
+	.owner = THIS_MODULE,
+};
+
+static struct pwm_device *ti_sn_pwm_of_xlate(struct pwm_chip *pc,
+					     const struct of_phandle_args *args)
+{
+	struct pwm_device *pwm;
+
+	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];
+
+	return pwm;
+}
+
+static int ti_sn_setup_pwmchip(struct ti_sn_bridge *pdata)
+{
+	pdata->pchip.dev = pdata->dev;
+	pdata->pchip.ops = &ti_sn_pwm_ops;
+	pdata->pchip.base = -1;
+	pdata->pchip.npwm = 1;
+	pdata->pchip.of_xlate = ti_sn_pwm_of_xlate;
+	pdata->pchip.of_pwm_n_cells = 1;
+
+	return pwmchip_add(&pdata->pchip);
+}
+
+static void ti_sn_remove_pwmchip(struct ti_sn_bridge *pdata)
+{
+	pwmchip_remove(&pdata->pchip);
+
+	if (pdata->pwm_enabled)
+		pm_runtime_put_sync(pdata->dev);
+}
+#else
+static int ti_sn_pwm_pin_request(struct ti_sn_bridge *pdata) { return 0; }
+static void ti_sn_pwm_pin_release(struct ti_sn_bridge *pdata) {}
+static int ti_sn_setup_pwmchip(struct ti_sn_bridge *pdata) { return 0; }
+static void ti_sn_remove_pwmchip(struct ti_sn_bridge *pdata) {}
+#endif
+
 #if defined(CONFIG_OF_GPIO)
 
 static int tn_sn_bridge_of_xlate(struct gpio_chip *chip,
@@ -1113,10 +1291,25 @@ static int ti_sn_bridge_gpio_direction_output(struct gpio_chip *chip,
 	return ret;
 }
 
+static int ti_sn_bridge_gpio_request(struct gpio_chip *chip, unsigned int offset)
+{
+	struct ti_sn_bridge *pdata = gpiochip_get_data(chip);
+
+	if (offset == SN_PWM_GPIO)
+		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_sn_bridge *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)
+		ti_sn_pwm_pin_release(pdata);
 }
 
 static const char * const ti_sn_bridge_gpio_names[SN_NUM_GPIOS] = {
@@ -1136,6 +1329,7 @@ static int ti_sn_setup_gpio_controller(struct ti_sn_bridge *pdata)
 	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;
@@ -1282,6 +1476,12 @@ static int ti_sn_bridge_probe(struct i2c_client *client,
 		return ret;
 	}
 
+	ret = ti_sn_setup_pwmchip(pdata);
+	if (ret)  {
+		pm_runtime_disable(pdata->dev);
+		return ret;
+	}
+
 	i2c_set_clientdata(client, pdata);
 
 	pdata->aux.name = "ti-sn65dsi86-aux";
@@ -1320,6 +1520,8 @@ static int ti_sn_bridge_remove(struct i2c_client *client)
 
 	drm_bridge_remove(&pdata->bridge);
 
+	ti_sn_remove_pwmchip(pdata);
+
 	return 0;
 }
 
-- 
2.29.2

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[kernel test robot]

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

Hi Bjorn,

I love your patch! Yet something to improve:

[auto build test ERROR on next-20201207]
[cannot apply to linus/master v5.10-rc7 v5.10-rc6 v5.10-rc5 v5.10-rc7]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Bjorn-Andersson/drm-bridge-ti-sn65dsi86-Implement-the-pwm_chip/20201208-124632
base:    15ac8fdb74403772780be1a8c4f7c1eff1040fc4
config: i386-randconfig-s002-20201208 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-15) 9.3.0
reproduce:
        # apt-get install sparse
        # sparse version: v0.6.3-179-ga00755aa-dirty
        # https://github.com/0day-ci/linux/commit/349c88e420ead9bf6dca594f68a81d58fd6ffff9
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Bjorn-Andersson/drm-bridge-ti-sn65dsi86-Implement-the-pwm_chip/20201208-124632
        git checkout 349c88e420ead9bf6dca594f68a81d58fd6ffff9
        # save the attached .config to linux build tree
        make W=1 C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' ARCH=i386 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   drivers/gpu/drm/bridge/ti-sn65dsi86.c: In function 'ti_sn_pwm_apply':
>> drivers/gpu/drm/bridge/ti-sn65dsi86.c:1095:15: error: implicit declaration of function 'FIELD_PREP' [-Werror=implicit-function-declaration]
    1095 |  pwm_en_inv = FIELD_PREP(BIT(1), !!state->enabled) |
         |               ^~~~~~~~~~
   cc1: some warnings being treated as errors

vim +/FIELD_PREP +1095 drivers/gpu/drm/bridge/ti-sn65dsi86.c

  1037	
  1038	static int ti_sn_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
  1039				   const struct pwm_state *state)
  1040	{
  1041		struct ti_sn_bridge *pdata = pwm_chip_to_ti_sn_bridge(chip);
  1042		unsigned int pwm_en_inv;
  1043		unsigned int backlight;
  1044		unsigned int pwm_freq;
  1045		unsigned int pre_div;
  1046		unsigned int scale;
  1047		int ret;
  1048	
  1049		if (!pdata->pwm_enabled) {
  1050			ret = pm_runtime_get_sync(pdata->dev);
  1051			if (ret < 0)
  1052				return ret;
  1053	
  1054			ret = regmap_update_bits(pdata->regmap, SN_GPIO_CTRL_REG,
  1055						 SN_GPIO_MUX_MASK << (2 * SN_PWM_GPIO),
  1056						 SN_GPIO_MUX_SPECIAL << (2 * SN_PWM_GPIO));
  1057			if (ret) {
  1058				dev_err(pdata->dev, "failed to mux in PWM function\n");
  1059				goto out;
  1060			}
  1061		}
  1062	
  1063		if (state->enabled) {
  1064			/*
  1065			 * Per the datasheet the PWM frequency is given by:
  1066			 *
  1067			 * PWM_FREQ = REFCLK_FREQ / (PWM_PRE_DIV * BACKLIGHT_SCALE + 1)
  1068			 *
  1069			 * In order to find the PWM_FREQ that best suits the requested
  1070			 * state->period, the PWM_PRE_DIV is calculated with the
  1071			 * maximum possible number of steps (BACKLIGHT_SCALE_MAX). The
  1072			 * actual BACKLIGHT_SCALE is then adjusted down to match the
  1073			 * requested period.
  1074			 *
  1075			 * The BACKLIGHT value is then calculated against the
  1076			 * BACKLIGHT_SCALE, based on the requested duty_cycle and
  1077			 * period.
  1078			 */
  1079			pwm_freq = NSEC_PER_SEC / state->period;
  1080			pre_div = DIV_ROUND_UP(pdata->pwm_refclk / pwm_freq - 1, BACKLIGHT_SCALE_MAX);
  1081			scale = (pdata->pwm_refclk / pwm_freq - 1) / pre_div;
  1082	
  1083			backlight = scale * state->duty_cycle / state->period;
  1084	
  1085			ret = regmap_write(pdata->regmap, SN_PWM_PRE_DIV_REG, pre_div);
  1086			if (ret) {
  1087				dev_err(pdata->dev, "failed to update PWM_PRE_DIV\n");
  1088				goto out;
  1089			}
  1090	
  1091			ti_sn_bridge_write_u16(pdata, SN_BACKLIGHT_SCALE_REG, scale);
  1092			ti_sn_bridge_write_u16(pdata, SN_BACKLIGHT_REG, backlight);
  1093		}
  1094	
> 1095		pwm_en_inv = FIELD_PREP(BIT(1), !!state->enabled) |
  1096			     FIELD_PREP(BIT(0), state->polarity == PWM_POLARITY_INVERSED);
  1097		ret = regmap_write(pdata->regmap, SN_PWM_EN_INV_REG, pwm_en_inv);
  1098		if (ret) {
  1099			dev_err(pdata->dev, "failed to update PWM_EN/PWM_INV\n");
  1100			goto out;
  1101		}
  1102	
  1103		pdata->pwm_enabled = !!state->enabled;
  1104	out:
  1105	
  1106		if (!pdata->pwm_enabled)
  1107			pm_runtime_put_sync(pdata->dev);
  1108	
  1109		return ret;
  1110	}
  1111	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 35220 bytes --]

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

[kernel test robot]

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

Hi Bjorn,

I love your patch! Yet something to improve:

[auto build test ERROR on next-20201207]
[cannot apply to linus/master v5.10-rc7 v5.10-rc6 v5.10-rc5 v5.10-rc7]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Bjorn-Andersson/drm-bridge-ti-sn65dsi86-Implement-the-pwm_chip/20201208-124632
base:    15ac8fdb74403772780be1a8c4f7c1eff1040fc4
config: x86_64-randconfig-a015-20201208 (attached as .config)
compiler: clang version 12.0.0 (https://github.com/llvm/llvm-project a2f922140f5380571fb74179f2bf622b3b925697)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # install x86_64 cross compiling tool for clang build
        # apt-get install binutils-x86-64-linux-gnu
        # https://github.com/0day-ci/linux/commit/349c88e420ead9bf6dca594f68a81d58fd6ffff9
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Bjorn-Andersson/drm-bridge-ti-sn65dsi86-Implement-the-pwm_chip/20201208-124632
        git checkout 349c88e420ead9bf6dca594f68a81d58fd6ffff9
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=x86_64 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

>> drivers/gpu/drm/bridge/ti-sn65dsi86.c:1095:15: error: implicit declaration of function 'FIELD_PREP' [-Werror,-Wimplicit-function-declaration]
           pwm_en_inv = FIELD_PREP(BIT(1), !!state->enabled) |
                        ^
   1 error generated.

vim +/FIELD_PREP +1095 drivers/gpu/drm/bridge/ti-sn65dsi86.c

  1037	
  1038	static int ti_sn_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
  1039				   const struct pwm_state *state)
  1040	{
  1041		struct ti_sn_bridge *pdata = pwm_chip_to_ti_sn_bridge(chip);
  1042		unsigned int pwm_en_inv;
  1043		unsigned int backlight;
  1044		unsigned int pwm_freq;
  1045		unsigned int pre_div;
  1046		unsigned int scale;
  1047		int ret;
  1048	
  1049		if (!pdata->pwm_enabled) {
  1050			ret = pm_runtime_get_sync(pdata->dev);
  1051			if (ret < 0)
  1052				return ret;
  1053	
  1054			ret = regmap_update_bits(pdata->regmap, SN_GPIO_CTRL_REG,
  1055						 SN_GPIO_MUX_MASK << (2 * SN_PWM_GPIO),
  1056						 SN_GPIO_MUX_SPECIAL << (2 * SN_PWM_GPIO));
  1057			if (ret) {
  1058				dev_err(pdata->dev, "failed to mux in PWM function\n");
  1059				goto out;
  1060			}
  1061		}
  1062	
  1063		if (state->enabled) {
  1064			/*
  1065			 * Per the datasheet the PWM frequency is given by:
  1066			 *
  1067			 * PWM_FREQ = REFCLK_FREQ / (PWM_PRE_DIV * BACKLIGHT_SCALE + 1)
  1068			 *
  1069			 * In order to find the PWM_FREQ that best suits the requested
  1070			 * state->period, the PWM_PRE_DIV is calculated with the
  1071			 * maximum possible number of steps (BACKLIGHT_SCALE_MAX). The
  1072			 * actual BACKLIGHT_SCALE is then adjusted down to match the
  1073			 * requested period.
  1074			 *
  1075			 * The BACKLIGHT value is then calculated against the
  1076			 * BACKLIGHT_SCALE, based on the requested duty_cycle and
  1077			 * period.
  1078			 */
  1079			pwm_freq = NSEC_PER_SEC / state->period;
  1080			pre_div = DIV_ROUND_UP(pdata->pwm_refclk / pwm_freq - 1, BACKLIGHT_SCALE_MAX);
  1081			scale = (pdata->pwm_refclk / pwm_freq - 1) / pre_div;
  1082	
  1083			backlight = scale * state->duty_cycle / state->period;
  1084	
  1085			ret = regmap_write(pdata->regmap, SN_PWM_PRE_DIV_REG, pre_div);
  1086			if (ret) {
  1087				dev_err(pdata->dev, "failed to update PWM_PRE_DIV\n");
  1088				goto out;
  1089			}
  1090	
  1091			ti_sn_bridge_write_u16(pdata, SN_BACKLIGHT_SCALE_REG, scale);
  1092			ti_sn_bridge_write_u16(pdata, SN_BACKLIGHT_REG, backlight);
  1093		}
  1094	
> 1095		pwm_en_inv = FIELD_PREP(BIT(1), !!state->enabled) |
  1096			     FIELD_PREP(BIT(0), state->polarity == PWM_POLARITY_INVERSED);
  1097		ret = regmap_write(pdata->regmap, SN_PWM_EN_INV_REG, pwm_en_inv);
  1098		if (ret) {
  1099			dev_err(pdata->dev, "failed to update PWM_EN/PWM_INV\n");
  1100			goto out;
  1101		}
  1102	
  1103		pdata->pwm_enabled = !!state->enabled;
  1104	out:
  1105	
  1106		if (!pdata->pwm_enabled)
  1107			pm_runtime_put_sync(pdata->dev);
  1108	
  1109		return ret;
  1110	}
  1111	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 35062 bytes --]

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

[Uwe Kleine-König]

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

Hello,

On Mon, Dec 07, 2020 at 10:40:22PM -0600, 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.
> 
> Special thanks to Doug Anderson for suggestions related to the involved
> math.

Did you test this with CONFIG_PWM_DEBUG? (I think you didn't, because
otherwise there would be a .get_state callback.)

> @@ -162,6 +171,12 @@ struct ti_sn_bridge {
>  	struct gpio_chip		gchip;
>  	DECLARE_BITMAP(gchip_output, SN_NUM_GPIOS);
>  #endif
> +#if defined(CONFIG_PWM)

Would it make sense to introduce a separate config symbol for this?
Something like CONFIG_PWM_SN65DSI87?

> +	struct pwm_chip			pchip;
> +	bool				pwm_enabled;
> +	unsigned int			pwm_refclk;
> +	atomic_t			pwm_pin_busy;

struct ti_sn_bridge has a kernel doc comment describing all members,
please add a description of the members you introduced here. Please also
point out that you use pwm_pin_busy to protect against concurrent use of
the pin as PWM and GPIO.

> +#endif
>  };
>  
>  static const struct regmap_range ti_sn_bridge_volatile_ranges[] = {
> @@ -499,6 +514,14 @@ static void ti_sn_bridge_set_refclk_freq(struct ti_sn_bridge *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 = ti_sn_bridge_refclk_lut[i];
> +#endif

I don't understand this code. 'i' seems to be something more special
than a counter variable, so I wonder if it should have a better name.
(This is however an issue separate from this patch, but it would be
great to first make the code a bit better understandable. Or is this
only me?)

>  }
>  
>  static void ti_sn_bridge_set_dsi_rate(struct ti_sn_bridge *pdata)
> @@ -981,6 +1004,161 @@ static int ti_sn_bridge_parse_dsi_host(struct ti_sn_bridge *pdata)
>  	return 0;
>  }
>  
> +#if defined(CONFIG_PWM)
> +static int ti_sn_pwm_pin_request(struct ti_sn_bridge *pdata)
> +{
> +	return atomic_xchg(&pdata->pwm_pin_busy, 1) ? -EBUSY : 0;
> +}
> +
> +static void ti_sn_pwm_pin_release(struct ti_sn_bridge *pdata)
> +{
> +	atomic_set(&pdata->pwm_pin_busy, 0);
> +}
> +
> +static struct ti_sn_bridge *
> +pwm_chip_to_ti_sn_bridge(struct pwm_chip *chip)

All your functions share the same function prefix (which is fine), but
this one doesn't.

> +{
> +	return container_of(chip, struct ti_sn_bridge, pchip);
> +}
> [...]
> +	if (state->enabled) {
> +		/*
> +		 * Per the datasheet the PWM frequency is given by:
> +		 *
> +		 * PWM_FREQ = REFCLK_FREQ / (PWM_PRE_DIV * BACKLIGHT_SCALE + 1)
> +		 *
> +		 * In order to find the PWM_FREQ that best suits the requested
> +		 * state->period, the PWM_PRE_DIV is calculated with the
> +		 * maximum possible number of steps (BACKLIGHT_SCALE_MAX). The
> +		 * actual BACKLIGHT_SCALE is then adjusted down to match the
> +		 * requested period.
> +		 *
> +		 * The BACKLIGHT value is then calculated against the
> +		 * BACKLIGHT_SCALE, based on the requested duty_cycle and
> +		 * period.
> +		 */
> +		pwm_freq = NSEC_PER_SEC / state->period;

Here you should better have some range checking. Consider for example
state->period being > NSEC_PER_SEC. (Hint: This makes pwm_freq = 0 and
in the next line you divide by pwm_freq.)

> +		pre_div = DIV_ROUND_UP(pdata->pwm_refclk / pwm_freq - 1, BACKLIGHT_SCALE_MAX);
> +		scale = (pdata->pwm_refclk / pwm_freq - 1) / pre_div;

I'm still trying to wrap my head around this calculation, but dividing
by the result of a division is always loosing precision. This is really
involved and I'm willing to bet this can be done easier and with more
precision.

... some time later ...

You wrote "PWM_FREQ = REFCLK_FREQ / (PWM_PRE_DIV * BACKLIGHT_SCALE + 1)",
so (I think) that means you have:

	period = (PWM_PRE_DIV * BACKLIGHT_SCALE + 1) / refclk

right? I deduce from your formula how the duty_cycle is defined and I
think it's:

	duty_cycle = (PWM_PRE_DIV * BACKLIGHT + 1) / refclk

is this right? And now your idea to "best suite the requested period" is
to select a small divider such that you can still use a big value in
SCALE to define the period and so have a fine separation for the
duty_cycle, right?

I will stop doing maths here now until you confirm my steps up to now
are right.

> +		backlight = scale * state->duty_cycle / state->period;

This is an u64 division, you must use do_div for that. Also you're
losing precision here.

> +		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_sn_bridge_write_u16(pdata, SN_BACKLIGHT_SCALE_REG, scale);
> +		ti_sn_bridge_write_u16(pdata, SN_BACKLIGHT_REG, backlight);

How does the PWM behave in between these writes? Are the register values
shadowed until the third write happens (which would be the optimum), or
does this result in (maybe) emitting an output wave that doesn't
correspond to the requested setting (assuming the PWM is already enabled
of course)?

What happens if the value written to SN_BACKLIGHT_SCALE_REG is less than
the previous value in SN_BACKLIGHT_REG? ti_sn_bridge_write_u16 wraps two
regmap writes, is there a race, too?

> +	}
> +
> +	pwm_en_inv = FIELD_PREP(BIT(1), !!state->enabled) |
> +		     FIELD_PREP(BIT(0), state->polarity == PWM_POLARITY_INVERSED);

Please introduce symbolic names for BIT(1) and BIT(0) here.

How does the hardware behave with the enable bit unset? Does it emit the
inactive level according to the polarity bit?

> +	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 struct pwm_device *ti_sn_pwm_of_xlate(struct pwm_chip *pc,
> +					     const struct of_phandle_args *args)
> +{
> +	struct pwm_device *pwm;
> +
> +	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];
> +
> +	return pwm;
> +}

This is done to optimise away the 0 needed in each phandle to implement
the "usual" pwm binding. IMHO this function should either move into the
pwm core, or you should stick to the usual binding.

Apropos binding: Is there already a binding document for the hardware?
You should expand it to describe your additions.

> @@ -1282,6 +1476,12 @@ static int ti_sn_bridge_probe(struct i2c_client *client,
>  		return ret;
>  	}
>  
> +	ret = ti_sn_setup_pwmchip(pdata);
> +	if (ret)  {
> +		pm_runtime_disable(pdata->dev);
> +		return ret;
> +	}

I'm not sure about the purpose of the containing hardware, but I wonder
if it would be saner to not break probing of the device if adding the
PWM functionality fails. Ideally the driver would provide an mfd driver
that allows its components to be probed independently.

>  	i2c_set_clientdata(client, pdata);
>  
>  	pdata->aux.name = "ti-sn65dsi86-aux";
> @@ -1320,6 +1520,8 @@ static int ti_sn_bridge_remove(struct i2c_client *client)
>  
>  	drm_bridge_remove(&pdata->bridge);
>  
> +	ti_sn_remove_pwmchip(pdata);
> +
>  	return 0;

Best regards
Uwe

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

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

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

[Uwe Kleine-König]

[-- Attachment #1.1: Type: text/plain, Size: 8023 bytes --]

Hello,

On Mon, Dec 07, 2020 at 10:40:22PM -0600, 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.
> 
> Special thanks to Doug Anderson for suggestions related to the involved
> math.

Did you test this with CONFIG_PWM_DEBUG? (I think you didn't, because
otherwise there would be a .get_state callback.)

> @@ -162,6 +171,12 @@ struct ti_sn_bridge {
>  	struct gpio_chip		gchip;
>  	DECLARE_BITMAP(gchip_output, SN_NUM_GPIOS);
>  #endif
> +#if defined(CONFIG_PWM)

Would it make sense to introduce a separate config symbol for this?
Something like CONFIG_PWM_SN65DSI87?

> +	struct pwm_chip			pchip;
> +	bool				pwm_enabled;
> +	unsigned int			pwm_refclk;
> +	atomic_t			pwm_pin_busy;

struct ti_sn_bridge has a kernel doc comment describing all members,
please add a description of the members you introduced here. Please also
point out that you use pwm_pin_busy to protect against concurrent use of
the pin as PWM and GPIO.

> +#endif
>  };
>  
>  static const struct regmap_range ti_sn_bridge_volatile_ranges[] = {
> @@ -499,6 +514,14 @@ static void ti_sn_bridge_set_refclk_freq(struct ti_sn_bridge *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 = ti_sn_bridge_refclk_lut[i];
> +#endif

I don't understand this code. 'i' seems to be something more special
than a counter variable, so I wonder if it should have a better name.
(This is however an issue separate from this patch, but it would be
great to first make the code a bit better understandable. Or is this
only me?)

>  }
>  
>  static void ti_sn_bridge_set_dsi_rate(struct ti_sn_bridge *pdata)
> @@ -981,6 +1004,161 @@ static int ti_sn_bridge_parse_dsi_host(struct ti_sn_bridge *pdata)
>  	return 0;
>  }
>  
> +#if defined(CONFIG_PWM)
> +static int ti_sn_pwm_pin_request(struct ti_sn_bridge *pdata)
> +{
> +	return atomic_xchg(&pdata->pwm_pin_busy, 1) ? -EBUSY : 0;
> +}
> +
> +static void ti_sn_pwm_pin_release(struct ti_sn_bridge *pdata)
> +{
> +	atomic_set(&pdata->pwm_pin_busy, 0);
> +}
> +
> +static struct ti_sn_bridge *
> +pwm_chip_to_ti_sn_bridge(struct pwm_chip *chip)

All your functions share the same function prefix (which is fine), but
this one doesn't.

> +{
> +	return container_of(chip, struct ti_sn_bridge, pchip);
> +}
> [...]
> +	if (state->enabled) {
> +		/*
> +		 * Per the datasheet the PWM frequency is given by:
> +		 *
> +		 * PWM_FREQ = REFCLK_FREQ / (PWM_PRE_DIV * BACKLIGHT_SCALE + 1)
> +		 *
> +		 * In order to find the PWM_FREQ that best suits the requested
> +		 * state->period, the PWM_PRE_DIV is calculated with the
> +		 * maximum possible number of steps (BACKLIGHT_SCALE_MAX). The
> +		 * actual BACKLIGHT_SCALE is then adjusted down to match the
> +		 * requested period.
> +		 *
> +		 * The BACKLIGHT value is then calculated against the
> +		 * BACKLIGHT_SCALE, based on the requested duty_cycle and
> +		 * period.
> +		 */
> +		pwm_freq = NSEC_PER_SEC / state->period;

Here you should better have some range checking. Consider for example
state->period being > NSEC_PER_SEC. (Hint: This makes pwm_freq = 0 and
in the next line you divide by pwm_freq.)

> +		pre_div = DIV_ROUND_UP(pdata->pwm_refclk / pwm_freq - 1, BACKLIGHT_SCALE_MAX);
> +		scale = (pdata->pwm_refclk / pwm_freq - 1) / pre_div;

I'm still trying to wrap my head around this calculation, but dividing
by the result of a division is always loosing precision. This is really
involved and I'm willing to bet this can be done easier and with more
precision.

... some time later ...

You wrote "PWM_FREQ = REFCLK_FREQ / (PWM_PRE_DIV * BACKLIGHT_SCALE + 1)",
so (I think) that means you have:

	period = (PWM_PRE_DIV * BACKLIGHT_SCALE + 1) / refclk

right? I deduce from your formula how the duty_cycle is defined and I
think it's:

	duty_cycle = (PWM_PRE_DIV * BACKLIGHT + 1) / refclk

is this right? And now your idea to "best suite the requested period" is
to select a small divider such that you can still use a big value in
SCALE to define the period and so have a fine separation for the
duty_cycle, right?

I will stop doing maths here now until you confirm my steps up to now
are right.

> +		backlight = scale * state->duty_cycle / state->period;

This is an u64 division, you must use do_div for that. Also you're
losing precision here.

> +		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_sn_bridge_write_u16(pdata, SN_BACKLIGHT_SCALE_REG, scale);
> +		ti_sn_bridge_write_u16(pdata, SN_BACKLIGHT_REG, backlight);

How does the PWM behave in between these writes? Are the register values
shadowed until the third write happens (which would be the optimum), or
does this result in (maybe) emitting an output wave that doesn't
correspond to the requested setting (assuming the PWM is already enabled
of course)?

What happens if the value written to SN_BACKLIGHT_SCALE_REG is less than
the previous value in SN_BACKLIGHT_REG? ti_sn_bridge_write_u16 wraps two
regmap writes, is there a race, too?

> +	}
> +
> +	pwm_en_inv = FIELD_PREP(BIT(1), !!state->enabled) |
> +		     FIELD_PREP(BIT(0), state->polarity == PWM_POLARITY_INVERSED);

Please introduce symbolic names for BIT(1) and BIT(0) here.

How does the hardware behave with the enable bit unset? Does it emit the
inactive level according to the polarity bit?

> +	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 struct pwm_device *ti_sn_pwm_of_xlate(struct pwm_chip *pc,
> +					     const struct of_phandle_args *args)
> +{
> +	struct pwm_device *pwm;
> +
> +	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];
> +
> +	return pwm;
> +}

This is done to optimise away the 0 needed in each phandle to implement
the "usual" pwm binding. IMHO this function should either move into the
pwm core, or you should stick to the usual binding.

Apropos binding: Is there already a binding document for the hardware?
You should expand it to describe your additions.

> @@ -1282,6 +1476,12 @@ static int ti_sn_bridge_probe(struct i2c_client *client,
>  		return ret;
>  	}
>  
> +	ret = ti_sn_setup_pwmchip(pdata);
> +	if (ret)  {
> +		pm_runtime_disable(pdata->dev);
> +		return ret;
> +	}

I'm not sure about the purpose of the containing hardware, but I wonder
if it would be saner to not break probing of the device if adding the
PWM functionality fails. Ideally the driver would provide an mfd driver
that allows its components to be probed independently.

>  	i2c_set_clientdata(client, pdata);
>  
>  	pdata->aux.name = "ti-sn65dsi86-aux";
> @@ -1320,6 +1520,8 @@ static int ti_sn_bridge_remove(struct i2c_client *client)
>  
>  	drm_bridge_remove(&pdata->bridge);
>  
> +	ti_sn_remove_pwmchip(pdata);
> +
>  	return 0;

Best regards
Uwe

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

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

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Bjorn Andersson]

On Tue 08 Dec 02:04 CST 2020, Uwe Kleine-K?nig wrote:

> Hello,
> 
> On Mon, Dec 07, 2020 at 10:40:22PM -0600, 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.
> > 
> > Special thanks to Doug Anderson for suggestions related to the involved
> > math.
> 
> Did you test this with CONFIG_PWM_DEBUG? (I think you didn't, because
> otherwise there would be a .get_state callback.)
> 

I had not, but have now explored this further and will follow up with a
get_state implementation :)

> > @@ -162,6 +171,12 @@ struct ti_sn_bridge {
> >  	struct gpio_chip		gchip;
> >  	DECLARE_BITMAP(gchip_output, SN_NUM_GPIOS);
> >  #endif
> > +#if defined(CONFIG_PWM)
> 
> Would it make sense to introduce a separate config symbol for this?
> Something like CONFIG_PWM_SN65DSI87?
> 

I don't think it adds much value at this point in time, if anyone needs
to squeeze those extra few bytes out of the kernel it's an easy change
to do later.

> > +	struct pwm_chip			pchip;
> > +	bool				pwm_enabled;
> > +	unsigned int			pwm_refclk;
> > +	atomic_t			pwm_pin_busy;
> 
> struct ti_sn_bridge has a kernel doc comment describing all members,
> please add a description of the members you introduced here. Please also
> point out that you use pwm_pin_busy to protect against concurrent use of
> the pin as PWM and GPIO.
> 

Yes, sorry for missing this.

> > +#endif
> >  };
> >  
> >  static const struct regmap_range ti_sn_bridge_volatile_ranges[] = {
> > @@ -499,6 +514,14 @@ static void ti_sn_bridge_set_refclk_freq(struct ti_sn_bridge *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 = ti_sn_bridge_refclk_lut[i];
> > +#endif
> 
> I don't understand this code. 'i' seems to be something more special
> than a counter variable, so I wonder if it should have a better name.
> (This is however an issue separate from this patch, but it would be
> great to first make the code a bit better understandable. Or is this
> only me?)
> 

The sn65dsi86 can either run off an external "reference clock" or
derive the clock from the video signal (afaict). In both cases 3 bits
are used to inform the chip about this clock rate - and the two
possible lists of frequencies are defined in ti_sn_bridge_refclk_lut[]
and ti_sn_bridge_dsiclk_lut[] above.

i is the index into these arrays, and the value being programmed into
this register. (And yes, that might not be the best named variable...)


The specification states that the PWM refclk is either straight off
the external refclk, in which case it's ti_sn_bridge_refclk_lut[i] or
somehow derived from the video signal down to
ti_sn_bridge_refclk_lut[i].

So this would be slightly cleaner if I just read SN_DPPLL_SRC_REG,
masked out the bits and did a lookup in ti_sn_pwm_apply(). But I wanted
to avoid the extra i2c read...

> >  }
> >  
> >  static void ti_sn_bridge_set_dsi_rate(struct ti_sn_bridge *pdata)
> > @@ -981,6 +1004,161 @@ static int ti_sn_bridge_parse_dsi_host(struct ti_sn_bridge *pdata)
> >  	return 0;
> >  }
> >  
> > +#if defined(CONFIG_PWM)
> > +static int ti_sn_pwm_pin_request(struct ti_sn_bridge *pdata)
> > +{
> > +	return atomic_xchg(&pdata->pwm_pin_busy, 1) ? -EBUSY : 0;
> > +}
> > +
> > +static void ti_sn_pwm_pin_release(struct ti_sn_bridge *pdata)
> > +{
> > +	atomic_set(&pdata->pwm_pin_busy, 0);
> > +}
> > +
> > +static struct ti_sn_bridge *
> > +pwm_chip_to_ti_sn_bridge(struct pwm_chip *chip)
> 
> All your functions share the same function prefix (which is fine), but
> this one doesn't.
> 

I'll fix that.

> > +{
> > +	return container_of(chip, struct ti_sn_bridge, pchip);
> > +}
> > [...]
> > +	if (state->enabled) {
> > +		/*
> > +		 * Per the datasheet the PWM frequency is given by:
> > +		 *
> > +		 * PWM_FREQ = REFCLK_FREQ / (PWM_PRE_DIV * BACKLIGHT_SCALE + 1)
> > +		 *
> > +		 * In order to find the PWM_FREQ that best suits the requested
> > +		 * state->period, the PWM_PRE_DIV is calculated with the
> > +		 * maximum possible number of steps (BACKLIGHT_SCALE_MAX). The
> > +		 * actual BACKLIGHT_SCALE is then adjusted down to match the
> > +		 * requested period.
> > +		 *
> > +		 * The BACKLIGHT value is then calculated against the
> > +		 * BACKLIGHT_SCALE, based on the requested duty_cycle and
> > +		 * period.
> > +		 */
> > +		pwm_freq = NSEC_PER_SEC / state->period;
> 
> Here you should better have some range checking. Consider for example
> state->period being > NSEC_PER_SEC. (Hint: This makes pwm_freq = 0 and
> in the next line you divide by pwm_freq.)
> 

I didn't consider the case where someone would drive their backlight <
1Hz. But given that this is a generic pwm_chip now I should do something
about it - or reject it...

> > +		pre_div = DIV_ROUND_UP(pdata->pwm_refclk / pwm_freq - 1, BACKLIGHT_SCALE_MAX);
> > +		scale = (pdata->pwm_refclk / pwm_freq - 1) / pre_div;
> 
> I'm still trying to wrap my head around this calculation, but dividing
> by the result of a division is always loosing precision. This is really
> involved and I'm willing to bet this can be done easier and with more
> precision.
> 
> ... some time later ...
> 
> You wrote "PWM_FREQ = REFCLK_FREQ / (PWM_PRE_DIV * BACKLIGHT_SCALE + 1)",
> so (I think) that means you have:
> 
> 	period = (PWM_PRE_DIV * BACKLIGHT_SCALE + 1) / refclk
> 

Yes, this matches my math.

Which would imply that the PWM period is (PRE_DIV * SCALE + 1) ticks of
the refclk.

> right? I deduce from your formula how the duty_cycle is defined and I
> think it's:
> 
> 	duty_cycle = (PWM_PRE_DIV * BACKLIGHT + 1) / refclk
> 
> is this right? And now your idea to "best suite the requested period" is
> to select a small divider such that you can still use a big value in
> SCALE to define the period and so have a fine separation for the
> duty_cycle, right?
> 

Above formula helps us calculate the period if we know the PRE_DIV and
SCALE, but what I'm given is the period by the framework and I need to
find suitable values for PRE_DIV and SCALE.

With the purpose of maximizing the resolution of duty_cycle (i.e.
[0, SCALE]) the SCALE is fixed to 65535 and the smallest PRE_DIV that
results in a period larger than the requested one is calculated.

With this PRE_DIV determined SCALE is then adjusted to match the
requested period. The duty_cycle is then just calculated as a fraction
of this.

> I will stop doing maths here now until you confirm my steps up to now
> are right.
> 
> > +		backlight = scale * state->duty_cycle / state->period;
> 
> This is an u64 division, you must use do_div for that. Also you're
> losing precision here.
> 

Are you suggesting that it would be better to just calculate the
backlight the same way as scale is calculated, rather then just scale
it? I.e:
		duty_freq = NSEC_PER_SEC / state->duty_cycle;
		backlight = (pdata->pwm_refclk / duty_freq - 1) / pre_div;

> > +		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_sn_bridge_write_u16(pdata, SN_BACKLIGHT_SCALE_REG, scale);
> > +		ti_sn_bridge_write_u16(pdata, SN_BACKLIGHT_REG, backlight);
> 
> How does the PWM behave in between these writes? Are the register values
> shadowed until the third write happens (which would be the optimum), or
> does this result in (maybe) emitting an output wave that doesn't
> correspond to the requested setting (assuming the PWM is already enabled
> of course)?
> 
> What happens if the value written to SN_BACKLIGHT_SCALE_REG is less than
> the previous value in SN_BACKLIGHT_REG? ti_sn_bridge_write_u16 wraps two
> regmap writes, is there a race, too?
> 

I have no idea, I don't find anything about this in the datasheet:
https://www.ti.com/lit/ds/symlink/sn65dsi86.pdf?ts=1607491553008

> > +	}
> > +
> > +	pwm_en_inv = FIELD_PREP(BIT(1), !!state->enabled) |
> > +		     FIELD_PREP(BIT(0), state->polarity == PWM_POLARITY_INVERSED);
> 
> Please introduce symbolic names for BIT(1) and BIT(0) here.
> 

Okay, will do.

> How does the hardware behave with the enable bit unset? Does it emit the
> inactive level according to the polarity bit?
> 

I'm doing this work on a consumer device, so unfortunately I have no way
to scope the signal. But as far as I can tell from my testing the
inverse bit does nothing when the PWM enabled bit is not set.

And clearing the PWM enable bit results in max brightness on the
display.

> > +	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 struct pwm_device *ti_sn_pwm_of_xlate(struct pwm_chip *pc,
> > +					     const struct of_phandle_args *args)
> > +{
> > +	struct pwm_device *pwm;
> > +
> > +	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];
> > +
> > +	return pwm;
> > +}
> 
> This is done to optimise away the 0 needed in each phandle to implement
> the "usual" pwm binding. IMHO this function should either move into the
> pwm core, or you should stick to the usual binding.
> 

You're right and I'd be happy to post a of_pwm_single_xlate() helper to
the PWM core.

> Apropos binding: Is there already a binding document for the hardware?
> You should expand it to describe your additions.
> 

The #pwm-cells = <1> was included in the DT binding from the beginning,
just no one ever implemented it in the driver.

> > @@ -1282,6 +1476,12 @@ static int ti_sn_bridge_probe(struct i2c_client *client,
> >  		return ret;
> >  	}
> >  
> > +	ret = ti_sn_setup_pwmchip(pdata);
> > +	if (ret)  {
> > +		pm_runtime_disable(pdata->dev);
> > +		return ret;
> > +	}
> 
> I'm not sure about the purpose of the containing hardware, but I wonder
> if it would be saner to not break probing of the device if adding the
> PWM functionality fails. Ideally the driver would provide an mfd driver
> that allows its components to be probed independently.
> 

Due to the possible circular dependency between this bridge driver and
the associated panel's dependency on the pwm_chip the suggestion of
splitting the driver in more pieces did came up. So I'm looking into
this now.

That said, the first issue I ran into while looking at this is how to
register a pwm_chip associated to the child's struct device and have
of_pwm_get() be allowed to find it (this made up node does not have an
of_node and there's no way to specify that the parent's of_node should
be used in the lookup).


Thanks for the review.

Regards,
Bjorn

> >  	i2c_set_clientdata(client, pdata);
> >  
> >  	pdata->aux.name = "ti-sn65dsi86-aux";
> > @@ -1320,6 +1520,8 @@ static int ti_sn_bridge_remove(struct i2c_client *client)
> >  
> >  	drm_bridge_remove(&pdata->bridge);
> >  
> > +	ti_sn_remove_pwmchip(pdata);
> > +
> >  	return 0;
> 
> Best regards
> Uwe
> 
> -- 
> Pengutronix e.K.                           | Uwe Kleine-König            |
> Industrial Linux Solutions                 | https://www.pengutronix.de/ |

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

[Bjorn Andersson]

On Tue 08 Dec 02:04 CST 2020, Uwe Kleine-K?nig wrote:

> Hello,
> 
> On Mon, Dec 07, 2020 at 10:40:22PM -0600, 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.
> > 
> > Special thanks to Doug Anderson for suggestions related to the involved
> > math.
> 
> Did you test this with CONFIG_PWM_DEBUG? (I think you didn't, because
> otherwise there would be a .get_state callback.)
> 

I had not, but have now explored this further and will follow up with a
get_state implementation :)

> > @@ -162,6 +171,12 @@ struct ti_sn_bridge {
> >  	struct gpio_chip		gchip;
> >  	DECLARE_BITMAP(gchip_output, SN_NUM_GPIOS);
> >  #endif
> > +#if defined(CONFIG_PWM)
> 
> Would it make sense to introduce a separate config symbol for this?
> Something like CONFIG_PWM_SN65DSI87?
> 

I don't think it adds much value at this point in time, if anyone needs
to squeeze those extra few bytes out of the kernel it's an easy change
to do later.

> > +	struct pwm_chip			pchip;
> > +	bool				pwm_enabled;
> > +	unsigned int			pwm_refclk;
> > +	atomic_t			pwm_pin_busy;
> 
> struct ti_sn_bridge has a kernel doc comment describing all members,
> please add a description of the members you introduced here. Please also
> point out that you use pwm_pin_busy to protect against concurrent use of
> the pin as PWM and GPIO.
> 

Yes, sorry for missing this.

> > +#endif
> >  };
> >  
> >  static const struct regmap_range ti_sn_bridge_volatile_ranges[] = {
> > @@ -499,6 +514,14 @@ static void ti_sn_bridge_set_refclk_freq(struct ti_sn_bridge *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 = ti_sn_bridge_refclk_lut[i];
> > +#endif
> 
> I don't understand this code. 'i' seems to be something more special
> than a counter variable, so I wonder if it should have a better name.
> (This is however an issue separate from this patch, but it would be
> great to first make the code a bit better understandable. Or is this
> only me?)
> 

The sn65dsi86 can either run off an external "reference clock" or
derive the clock from the video signal (afaict). In both cases 3 bits
are used to inform the chip about this clock rate - and the two
possible lists of frequencies are defined in ti_sn_bridge_refclk_lut[]
and ti_sn_bridge_dsiclk_lut[] above.

i is the index into these arrays, and the value being programmed into
this register. (And yes, that might not be the best named variable...)


The specification states that the PWM refclk is either straight off
the external refclk, in which case it's ti_sn_bridge_refclk_lut[i] or
somehow derived from the video signal down to
ti_sn_bridge_refclk_lut[i].

So this would be slightly cleaner if I just read SN_DPPLL_SRC_REG,
masked out the bits and did a lookup in ti_sn_pwm_apply(). But I wanted
to avoid the extra i2c read...

> >  }
> >  
> >  static void ti_sn_bridge_set_dsi_rate(struct ti_sn_bridge *pdata)
> > @@ -981,6 +1004,161 @@ static int ti_sn_bridge_parse_dsi_host(struct ti_sn_bridge *pdata)
> >  	return 0;
> >  }
> >  
> > +#if defined(CONFIG_PWM)
> > +static int ti_sn_pwm_pin_request(struct ti_sn_bridge *pdata)
> > +{
> > +	return atomic_xchg(&pdata->pwm_pin_busy, 1) ? -EBUSY : 0;
> > +}
> > +
> > +static void ti_sn_pwm_pin_release(struct ti_sn_bridge *pdata)
> > +{
> > +	atomic_set(&pdata->pwm_pin_busy, 0);
> > +}
> > +
> > +static struct ti_sn_bridge *
> > +pwm_chip_to_ti_sn_bridge(struct pwm_chip *chip)
> 
> All your functions share the same function prefix (which is fine), but
> this one doesn't.
> 

I'll fix that.

> > +{
> > +	return container_of(chip, struct ti_sn_bridge, pchip);
> > +}
> > [...]
> > +	if (state->enabled) {
> > +		/*
> > +		 * Per the datasheet the PWM frequency is given by:
> > +		 *
> > +		 * PWM_FREQ = REFCLK_FREQ / (PWM_PRE_DIV * BACKLIGHT_SCALE + 1)
> > +		 *
> > +		 * In order to find the PWM_FREQ that best suits the requested
> > +		 * state->period, the PWM_PRE_DIV is calculated with the
> > +		 * maximum possible number of steps (BACKLIGHT_SCALE_MAX). The
> > +		 * actual BACKLIGHT_SCALE is then adjusted down to match the
> > +		 * requested period.
> > +		 *
> > +		 * The BACKLIGHT value is then calculated against the
> > +		 * BACKLIGHT_SCALE, based on the requested duty_cycle and
> > +		 * period.
> > +		 */
> > +		pwm_freq = NSEC_PER_SEC / state->period;
> 
> Here you should better have some range checking. Consider for example
> state->period being > NSEC_PER_SEC. (Hint: This makes pwm_freq = 0 and
> in the next line you divide by pwm_freq.)
> 

I didn't consider the case where someone would drive their backlight <
1Hz. But given that this is a generic pwm_chip now I should do something
about it - or reject it...

> > +		pre_div = DIV_ROUND_UP(pdata->pwm_refclk / pwm_freq - 1, BACKLIGHT_SCALE_MAX);
> > +		scale = (pdata->pwm_refclk / pwm_freq - 1) / pre_div;
> 
> I'm still trying to wrap my head around this calculation, but dividing
> by the result of a division is always loosing precision. This is really
> involved and I'm willing to bet this can be done easier and with more
> precision.
> 
> ... some time later ...
> 
> You wrote "PWM_FREQ = REFCLK_FREQ / (PWM_PRE_DIV * BACKLIGHT_SCALE + 1)",
> so (I think) that means you have:
> 
> 	period = (PWM_PRE_DIV * BACKLIGHT_SCALE + 1) / refclk
> 

Yes, this matches my math.

Which would imply that the PWM period is (PRE_DIV * SCALE + 1) ticks of
the refclk.

> right? I deduce from your formula how the duty_cycle is defined and I
> think it's:
> 
> 	duty_cycle = (PWM_PRE_DIV * BACKLIGHT + 1) / refclk
> 
> is this right? And now your idea to "best suite the requested period" is
> to select a small divider such that you can still use a big value in
> SCALE to define the period and so have a fine separation for the
> duty_cycle, right?
> 

Above formula helps us calculate the period if we know the PRE_DIV and
SCALE, but what I'm given is the period by the framework and I need to
find suitable values for PRE_DIV and SCALE.

With the purpose of maximizing the resolution of duty_cycle (i.e.
[0, SCALE]) the SCALE is fixed to 65535 and the smallest PRE_DIV that
results in a period larger than the requested one is calculated.

With this PRE_DIV determined SCALE is then adjusted to match the
requested period. The duty_cycle is then just calculated as a fraction
of this.

> I will stop doing maths here now until you confirm my steps up to now
> are right.
> 
> > +		backlight = scale * state->duty_cycle / state->period;
> 
> This is an u64 division, you must use do_div for that. Also you're
> losing precision here.
> 

Are you suggesting that it would be better to just calculate the
backlight the same way as scale is calculated, rather then just scale
it? I.e:
		duty_freq = NSEC_PER_SEC / state->duty_cycle;
		backlight = (pdata->pwm_refclk / duty_freq - 1) / pre_div;

> > +		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_sn_bridge_write_u16(pdata, SN_BACKLIGHT_SCALE_REG, scale);
> > +		ti_sn_bridge_write_u16(pdata, SN_BACKLIGHT_REG, backlight);
> 
> How does the PWM behave in between these writes? Are the register values
> shadowed until the third write happens (which would be the optimum), or
> does this result in (maybe) emitting an output wave that doesn't
> correspond to the requested setting (assuming the PWM is already enabled
> of course)?
> 
> What happens if the value written to SN_BACKLIGHT_SCALE_REG is less than
> the previous value in SN_BACKLIGHT_REG? ti_sn_bridge_write_u16 wraps two
> regmap writes, is there a race, too?
> 

I have no idea, I don't find anything about this in the datasheet:
https://www.ti.com/lit/ds/symlink/sn65dsi86.pdf?ts=1607491553008

> > +	}
> > +
> > +	pwm_en_inv = FIELD_PREP(BIT(1), !!state->enabled) |
> > +		     FIELD_PREP(BIT(0), state->polarity == PWM_POLARITY_INVERSED);
> 
> Please introduce symbolic names for BIT(1) and BIT(0) here.
> 

Okay, will do.

> How does the hardware behave with the enable bit unset? Does it emit the
> inactive level according to the polarity bit?
> 

I'm doing this work on a consumer device, so unfortunately I have no way
to scope the signal. But as far as I can tell from my testing the
inverse bit does nothing when the PWM enabled bit is not set.

And clearing the PWM enable bit results in max brightness on the
display.

> > +	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 struct pwm_device *ti_sn_pwm_of_xlate(struct pwm_chip *pc,
> > +					     const struct of_phandle_args *args)
> > +{
> > +	struct pwm_device *pwm;
> > +
> > +	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];
> > +
> > +	return pwm;
> > +}
> 
> This is done to optimise away the 0 needed in each phandle to implement
> the "usual" pwm binding. IMHO this function should either move into the
> pwm core, or you should stick to the usual binding.
> 

You're right and I'd be happy to post a of_pwm_single_xlate() helper to
the PWM core.

> Apropos binding: Is there already a binding document for the hardware?
> You should expand it to describe your additions.
> 

The #pwm-cells = <1> was included in the DT binding from the beginning,
just no one ever implemented it in the driver.

> > @@ -1282,6 +1476,12 @@ static int ti_sn_bridge_probe(struct i2c_client *client,
> >  		return ret;
> >  	}
> >  
> > +	ret = ti_sn_setup_pwmchip(pdata);
> > +	if (ret)  {
> > +		pm_runtime_disable(pdata->dev);
> > +		return ret;
> > +	}
> 
> I'm not sure about the purpose of the containing hardware, but I wonder
> if it would be saner to not break probing of the device if adding the
> PWM functionality fails. Ideally the driver would provide an mfd driver
> that allows its components to be probed independently.
> 

Due to the possible circular dependency between this bridge driver and
the associated panel's dependency on the pwm_chip the suggestion of
splitting the driver in more pieces did came up. So I'm looking into
this now.

That said, the first issue I ran into while looking at this is how to
register a pwm_chip associated to the child's struct device and have
of_pwm_get() be allowed to find it (this made up node does not have an
of_node and there's no way to specify that the parent's of_node should
be used in the lookup).


Thanks for the review.

Regards,
Bjorn

> >  	i2c_set_clientdata(client, pdata);
> >  
> >  	pdata->aux.name = "ti-sn65dsi86-aux";
> > @@ -1320,6 +1520,8 @@ static int ti_sn_bridge_remove(struct i2c_client *client)
> >  
> >  	drm_bridge_remove(&pdata->bridge);
> >  
> > +	ti_sn_remove_pwmchip(pdata);
> > +
> >  	return 0;
> 
> Best regards
> Uwe
> 
> -- 
> Pengutronix e.K.                           | Uwe Kleine-König            |
> Industrial Linux Solutions                 | https://www.pengutronix.de/ |


_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Shawn Guo]

On Mon, Dec 07, 2020 at 10:40:22PM -0600, 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.
> 
> Special thanks to Doug Anderson for suggestions related to the involved
> math.
> 
> Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> ---
>  drivers/gpu/drm/bridge/ti-sn65dsi86.c | 202 ++++++++++++++++++++++++++
>  1 file changed, 202 insertions(+)
> 
> diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> index f27306c51e4d..43c0acba57ab 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/bits.h>
>  #include <linux/clk.h>
>  #include <linux/debugfs.h>
> @@ -14,6 +15,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>
>  
> @@ -89,6 +91,11 @@
>  #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_AUX_CMD_STATUS_REG			0xF4
>  #define  AUX_IRQ_STATUS_AUX_RPLY_TOUT		BIT(3)
>  #define  AUX_IRQ_STATUS_AUX_SHORT		BIT(5)
> @@ -111,6 +118,8 @@
>  
>  #define SN_LINK_TRAINING_TRIES		10
>  
> +#define SN_PWM_GPIO			3

So this maps to the GPIO4 described in sn65dsi86 datasheet.  I'm
wondering if it's more readable to define the following SHIFT constants
(your code), and use GPIO_MUX_GPIO4_SHIFT >> 2 where you need GPIO
offset?

#define  GPIO_MUX_GPIO1_SHIFT	0
#define  GPIO_MUX_GPIO2_SHIFT	2
#define  GPIO_MUX_GPIO3_SHIFT	4
#define  GPIO_MUX_GPIO4_SHIFT	6

If you agree, you may consider to integrate this patch beforehand:

https://github.com/shawnguo2/linux/commit/7cde887ffb3b27a36e77a08bee3666d14968b586


Shawn

> +
>  /**
>   * struct ti_sn_bridge - Platform data for ti-sn65dsi86 driver.
>   * @dev:          Pointer to our device.
> @@ -162,6 +171,12 @@ struct ti_sn_bridge {
>  	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;
> +	atomic_t			pwm_pin_busy;
> +#endif
>  };
>  
>  static const struct regmap_range ti_sn_bridge_volatile_ranges[] = {
> @@ -499,6 +514,14 @@ static void ti_sn_bridge_set_refclk_freq(struct ti_sn_bridge *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 = ti_sn_bridge_refclk_lut[i];
> +#endif
>  }
>  
>  static void ti_sn_bridge_set_dsi_rate(struct ti_sn_bridge *pdata)
> @@ -981,6 +1004,161 @@ static int ti_sn_bridge_parse_dsi_host(struct ti_sn_bridge *pdata)
>  	return 0;
>  }
>  
> +#if defined(CONFIG_PWM)
> +static int ti_sn_pwm_pin_request(struct ti_sn_bridge *pdata)
> +{
> +	return atomic_xchg(&pdata->pwm_pin_busy, 1) ? -EBUSY : 0;
> +}
> +
> +static void ti_sn_pwm_pin_release(struct ti_sn_bridge *pdata)
> +{
> +	atomic_set(&pdata->pwm_pin_busy, 0);
> +}
> +
> +static struct ti_sn_bridge *
> +pwm_chip_to_ti_sn_bridge(struct pwm_chip *chip)
> +{
> +	return container_of(chip, struct ti_sn_bridge, pchip);
> +}
> +
> +static int ti_sn_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm)
> +{
> +	struct ti_sn_bridge *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_sn_bridge *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_sn_bridge *pdata = pwm_chip_to_ti_sn_bridge(chip);
> +	unsigned int pwm_en_inv;
> +	unsigned int backlight;
> +	unsigned int pwm_freq;
> +	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),
> +					 SN_GPIO_MUX_SPECIAL << (2 * SN_PWM_GPIO));
> +		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)
> +		 *
> +		 * In order to find the PWM_FREQ that best suits the requested
> +		 * state->period, the PWM_PRE_DIV is calculated with the
> +		 * maximum possible number of steps (BACKLIGHT_SCALE_MAX). The
> +		 * actual BACKLIGHT_SCALE is then adjusted down to match the
> +		 * requested period.
> +		 *
> +		 * The BACKLIGHT value is then calculated against the
> +		 * BACKLIGHT_SCALE, based on the requested duty_cycle and
> +		 * period.
> +		 */
> +		pwm_freq = NSEC_PER_SEC / state->period;
> +		pre_div = DIV_ROUND_UP(pdata->pwm_refclk / pwm_freq - 1, BACKLIGHT_SCALE_MAX);
> +		scale = (pdata->pwm_refclk / pwm_freq - 1) / pre_div;
> +
> +		backlight = scale * state->duty_cycle / 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_sn_bridge_write_u16(pdata, SN_BACKLIGHT_SCALE_REG, scale);
> +		ti_sn_bridge_write_u16(pdata, SN_BACKLIGHT_REG, backlight);
> +	}
> +
> +	pwm_en_inv = FIELD_PREP(BIT(1), !!state->enabled) |
> +		     FIELD_PREP(BIT(0), 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 const struct pwm_ops ti_sn_pwm_ops = {
> +	.request = ti_sn_pwm_request,
> +	.free = ti_sn_pwm_free,
> +	.apply = ti_sn_pwm_apply,
> +	.owner = THIS_MODULE,
> +};
> +
> +static struct pwm_device *ti_sn_pwm_of_xlate(struct pwm_chip *pc,
> +					     const struct of_phandle_args *args)
> +{
> +	struct pwm_device *pwm;
> +
> +	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];
> +
> +	return pwm;
> +}
> +
> +static int ti_sn_setup_pwmchip(struct ti_sn_bridge *pdata)
> +{
> +	pdata->pchip.dev = pdata->dev;
> +	pdata->pchip.ops = &ti_sn_pwm_ops;
> +	pdata->pchip.base = -1;
> +	pdata->pchip.npwm = 1;
> +	pdata->pchip.of_xlate = ti_sn_pwm_of_xlate;
> +	pdata->pchip.of_pwm_n_cells = 1;
> +
> +	return pwmchip_add(&pdata->pchip);
> +}
> +
> +static void ti_sn_remove_pwmchip(struct ti_sn_bridge *pdata)
> +{
> +	pwmchip_remove(&pdata->pchip);
> +
> +	if (pdata->pwm_enabled)
> +		pm_runtime_put_sync(pdata->dev);
> +}
> +#else
> +static int ti_sn_pwm_pin_request(struct ti_sn_bridge *pdata) { return 0; }
> +static void ti_sn_pwm_pin_release(struct ti_sn_bridge *pdata) {}
> +static int ti_sn_setup_pwmchip(struct ti_sn_bridge *pdata) { return 0; }
> +static void ti_sn_remove_pwmchip(struct ti_sn_bridge *pdata) {}
> +#endif
> +
>  #if defined(CONFIG_OF_GPIO)
>  
>  static int tn_sn_bridge_of_xlate(struct gpio_chip *chip,
> @@ -1113,10 +1291,25 @@ static int ti_sn_bridge_gpio_direction_output(struct gpio_chip *chip,
>  	return ret;
>  }
>  
> +static int ti_sn_bridge_gpio_request(struct gpio_chip *chip, unsigned int offset)
> +{
> +	struct ti_sn_bridge *pdata = gpiochip_get_data(chip);
> +
> +	if (offset == SN_PWM_GPIO)
> +		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_sn_bridge *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)
> +		ti_sn_pwm_pin_release(pdata);
>  }
>  
>  static const char * const ti_sn_bridge_gpio_names[SN_NUM_GPIOS] = {
> @@ -1136,6 +1329,7 @@ static int ti_sn_setup_gpio_controller(struct ti_sn_bridge *pdata)
>  	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;
> @@ -1282,6 +1476,12 @@ static int ti_sn_bridge_probe(struct i2c_client *client,
>  		return ret;
>  	}
>  
> +	ret = ti_sn_setup_pwmchip(pdata);
> +	if (ret)  {
> +		pm_runtime_disable(pdata->dev);
> +		return ret;
> +	}
> +
>  	i2c_set_clientdata(client, pdata);
>  
>  	pdata->aux.name = "ti-sn65dsi86-aux";
> @@ -1320,6 +1520,8 @@ static int ti_sn_bridge_remove(struct i2c_client *client)
>  
>  	drm_bridge_remove(&pdata->bridge);
>  
> +	ti_sn_remove_pwmchip(pdata);
> +
>  	return 0;
>  }
>  
> -- 
> 2.29.2
> 

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

[Shawn Guo]

On Mon, Dec 07, 2020 at 10:40:22PM -0600, 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.
> 
> Special thanks to Doug Anderson for suggestions related to the involved
> math.
> 
> Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> ---
>  drivers/gpu/drm/bridge/ti-sn65dsi86.c | 202 ++++++++++++++++++++++++++
>  1 file changed, 202 insertions(+)
> 
> diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> index f27306c51e4d..43c0acba57ab 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/bits.h>
>  #include <linux/clk.h>
>  #include <linux/debugfs.h>
> @@ -14,6 +15,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>
>  
> @@ -89,6 +91,11 @@
>  #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_AUX_CMD_STATUS_REG			0xF4
>  #define  AUX_IRQ_STATUS_AUX_RPLY_TOUT		BIT(3)
>  #define  AUX_IRQ_STATUS_AUX_SHORT		BIT(5)
> @@ -111,6 +118,8 @@
>  
>  #define SN_LINK_TRAINING_TRIES		10
>  
> +#define SN_PWM_GPIO			3

So this maps to the GPIO4 described in sn65dsi86 datasheet.  I'm
wondering if it's more readable to define the following SHIFT constants
(your code), and use GPIO_MUX_GPIO4_SHIFT >> 2 where you need GPIO
offset?

#define  GPIO_MUX_GPIO1_SHIFT	0
#define  GPIO_MUX_GPIO2_SHIFT	2
#define  GPIO_MUX_GPIO3_SHIFT	4
#define  GPIO_MUX_GPIO4_SHIFT	6

If you agree, you may consider to integrate this patch beforehand:

https://github.com/shawnguo2/linux/commit/7cde887ffb3b27a36e77a08bee3666d14968b586


Shawn

> +
>  /**
>   * struct ti_sn_bridge - Platform data for ti-sn65dsi86 driver.
>   * @dev:          Pointer to our device.
> @@ -162,6 +171,12 @@ struct ti_sn_bridge {
>  	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;
> +	atomic_t			pwm_pin_busy;
> +#endif
>  };
>  
>  static const struct regmap_range ti_sn_bridge_volatile_ranges[] = {
> @@ -499,6 +514,14 @@ static void ti_sn_bridge_set_refclk_freq(struct ti_sn_bridge *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 = ti_sn_bridge_refclk_lut[i];
> +#endif
>  }
>  
>  static void ti_sn_bridge_set_dsi_rate(struct ti_sn_bridge *pdata)
> @@ -981,6 +1004,161 @@ static int ti_sn_bridge_parse_dsi_host(struct ti_sn_bridge *pdata)
>  	return 0;
>  }
>  
> +#if defined(CONFIG_PWM)
> +static int ti_sn_pwm_pin_request(struct ti_sn_bridge *pdata)
> +{
> +	return atomic_xchg(&pdata->pwm_pin_busy, 1) ? -EBUSY : 0;
> +}
> +
> +static void ti_sn_pwm_pin_release(struct ti_sn_bridge *pdata)
> +{
> +	atomic_set(&pdata->pwm_pin_busy, 0);
> +}
> +
> +static struct ti_sn_bridge *
> +pwm_chip_to_ti_sn_bridge(struct pwm_chip *chip)
> +{
> +	return container_of(chip, struct ti_sn_bridge, pchip);
> +}
> +
> +static int ti_sn_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm)
> +{
> +	struct ti_sn_bridge *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_sn_bridge *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_sn_bridge *pdata = pwm_chip_to_ti_sn_bridge(chip);
> +	unsigned int pwm_en_inv;
> +	unsigned int backlight;
> +	unsigned int pwm_freq;
> +	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),
> +					 SN_GPIO_MUX_SPECIAL << (2 * SN_PWM_GPIO));
> +		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)
> +		 *
> +		 * In order to find the PWM_FREQ that best suits the requested
> +		 * state->period, the PWM_PRE_DIV is calculated with the
> +		 * maximum possible number of steps (BACKLIGHT_SCALE_MAX). The
> +		 * actual BACKLIGHT_SCALE is then adjusted down to match the
> +		 * requested period.
> +		 *
> +		 * The BACKLIGHT value is then calculated against the
> +		 * BACKLIGHT_SCALE, based on the requested duty_cycle and
> +		 * period.
> +		 */
> +		pwm_freq = NSEC_PER_SEC / state->period;
> +		pre_div = DIV_ROUND_UP(pdata->pwm_refclk / pwm_freq - 1, BACKLIGHT_SCALE_MAX);
> +		scale = (pdata->pwm_refclk / pwm_freq - 1) / pre_div;
> +
> +		backlight = scale * state->duty_cycle / 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_sn_bridge_write_u16(pdata, SN_BACKLIGHT_SCALE_REG, scale);
> +		ti_sn_bridge_write_u16(pdata, SN_BACKLIGHT_REG, backlight);
> +	}
> +
> +	pwm_en_inv = FIELD_PREP(BIT(1), !!state->enabled) |
> +		     FIELD_PREP(BIT(0), 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 const struct pwm_ops ti_sn_pwm_ops = {
> +	.request = ti_sn_pwm_request,
> +	.free = ti_sn_pwm_free,
> +	.apply = ti_sn_pwm_apply,
> +	.owner = THIS_MODULE,
> +};
> +
> +static struct pwm_device *ti_sn_pwm_of_xlate(struct pwm_chip *pc,
> +					     const struct of_phandle_args *args)
> +{
> +	struct pwm_device *pwm;
> +
> +	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];
> +
> +	return pwm;
> +}
> +
> +static int ti_sn_setup_pwmchip(struct ti_sn_bridge *pdata)
> +{
> +	pdata->pchip.dev = pdata->dev;
> +	pdata->pchip.ops = &ti_sn_pwm_ops;
> +	pdata->pchip.base = -1;
> +	pdata->pchip.npwm = 1;
> +	pdata->pchip.of_xlate = ti_sn_pwm_of_xlate;
> +	pdata->pchip.of_pwm_n_cells = 1;
> +
> +	return pwmchip_add(&pdata->pchip);
> +}
> +
> +static void ti_sn_remove_pwmchip(struct ti_sn_bridge *pdata)
> +{
> +	pwmchip_remove(&pdata->pchip);
> +
> +	if (pdata->pwm_enabled)
> +		pm_runtime_put_sync(pdata->dev);
> +}
> +#else
> +static int ti_sn_pwm_pin_request(struct ti_sn_bridge *pdata) { return 0; }
> +static void ti_sn_pwm_pin_release(struct ti_sn_bridge *pdata) {}
> +static int ti_sn_setup_pwmchip(struct ti_sn_bridge *pdata) { return 0; }
> +static void ti_sn_remove_pwmchip(struct ti_sn_bridge *pdata) {}
> +#endif
> +
>  #if defined(CONFIG_OF_GPIO)
>  
>  static int tn_sn_bridge_of_xlate(struct gpio_chip *chip,
> @@ -1113,10 +1291,25 @@ static int ti_sn_bridge_gpio_direction_output(struct gpio_chip *chip,
>  	return ret;
>  }
>  
> +static int ti_sn_bridge_gpio_request(struct gpio_chip *chip, unsigned int offset)
> +{
> +	struct ti_sn_bridge *pdata = gpiochip_get_data(chip);
> +
> +	if (offset == SN_PWM_GPIO)
> +		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_sn_bridge *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)
> +		ti_sn_pwm_pin_release(pdata);
>  }
>  
>  static const char * const ti_sn_bridge_gpio_names[SN_NUM_GPIOS] = {
> @@ -1136,6 +1329,7 @@ static int ti_sn_setup_gpio_controller(struct ti_sn_bridge *pdata)
>  	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;
> @@ -1282,6 +1476,12 @@ static int ti_sn_bridge_probe(struct i2c_client *client,
>  		return ret;
>  	}
>  
> +	ret = ti_sn_setup_pwmchip(pdata);
> +	if (ret)  {
> +		pm_runtime_disable(pdata->dev);
> +		return ret;
> +	}
> +
>  	i2c_set_clientdata(client, pdata);
>  
>  	pdata->aux.name = "ti-sn65dsi86-aux";
> @@ -1320,6 +1520,8 @@ static int ti_sn_bridge_remove(struct i2c_client *client)
>  
>  	drm_bridge_remove(&pdata->bridge);
>  
> +	ti_sn_remove_pwmchip(pdata);
> +
>  	return 0;
>  }
>  
> -- 
> 2.29.2
> 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Bjorn Andersson]

On Wed 09 Dec 19:51 CST 2020, Shawn Guo wrote:

> On Mon, Dec 07, 2020 at 10:40:22PM -0600, 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.
> > 
> > Special thanks to Doug Anderson for suggestions related to the involved
> > math.
> > 
> > Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> > ---
> >  drivers/gpu/drm/bridge/ti-sn65dsi86.c | 202 ++++++++++++++++++++++++++
> >  1 file changed, 202 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> > index f27306c51e4d..43c0acba57ab 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/bits.h>
> >  #include <linux/clk.h>
> >  #include <linux/debugfs.h>
> > @@ -14,6 +15,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>
> >  
> > @@ -89,6 +91,11 @@
> >  #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_AUX_CMD_STATUS_REG			0xF4
> >  #define  AUX_IRQ_STATUS_AUX_RPLY_TOUT		BIT(3)
> >  #define  AUX_IRQ_STATUS_AUX_SHORT		BIT(5)
> > @@ -111,6 +118,8 @@
> >  
> >  #define SN_LINK_TRAINING_TRIES		10
> >  
> > +#define SN_PWM_GPIO			3
> 
> So this maps to the GPIO4 described in sn65dsi86 datasheet.  I'm
> wondering if it's more readable to define the following SHIFT constants
> (your code), and use GPIO_MUX_GPIO4_SHIFT >> 2 where you need GPIO
> offset?
> 
> #define  GPIO_MUX_GPIO1_SHIFT	0
> #define  GPIO_MUX_GPIO2_SHIFT	2
> #define  GPIO_MUX_GPIO3_SHIFT	4
> #define  GPIO_MUX_GPIO4_SHIFT	6
> 
> If you agree, you may consider to integrate this patch beforehand:
> 

Afaict this is the only place in the driver where the gpio number is a
compile time constant and as you say I need both the shifted value and
the value itself in the patch. But I think it's worth clarifying that
"3" means GPIO 4, so if nothing else I should add a comment about that.

> https://github.com/shawnguo2/linux/commit/7cde887ffb3b27a36e77a08bee3666d14968b586
> 

These looks rather generic, but I like the consistency. Feel free to
post this and I'll review it for you.

Regards,
Bjorn

> 
> Shawn
> 
> > +
> >  /**
> >   * struct ti_sn_bridge - Platform data for ti-sn65dsi86 driver.
> >   * @dev:          Pointer to our device.
> > @@ -162,6 +171,12 @@ struct ti_sn_bridge {
> >  	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;
> > +	atomic_t			pwm_pin_busy;
> > +#endif
> >  };
> >  
> >  static const struct regmap_range ti_sn_bridge_volatile_ranges[] = {
> > @@ -499,6 +514,14 @@ static void ti_sn_bridge_set_refclk_freq(struct ti_sn_bridge *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 = ti_sn_bridge_refclk_lut[i];
> > +#endif
> >  }
> >  
> >  static void ti_sn_bridge_set_dsi_rate(struct ti_sn_bridge *pdata)
> > @@ -981,6 +1004,161 @@ static int ti_sn_bridge_parse_dsi_host(struct ti_sn_bridge *pdata)
> >  	return 0;
> >  }
> >  
> > +#if defined(CONFIG_PWM)
> > +static int ti_sn_pwm_pin_request(struct ti_sn_bridge *pdata)
> > +{
> > +	return atomic_xchg(&pdata->pwm_pin_busy, 1) ? -EBUSY : 0;
> > +}
> > +
> > +static void ti_sn_pwm_pin_release(struct ti_sn_bridge *pdata)
> > +{
> > +	atomic_set(&pdata->pwm_pin_busy, 0);
> > +}
> > +
> > +static struct ti_sn_bridge *
> > +pwm_chip_to_ti_sn_bridge(struct pwm_chip *chip)
> > +{
> > +	return container_of(chip, struct ti_sn_bridge, pchip);
> > +}
> > +
> > +static int ti_sn_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm)
> > +{
> > +	struct ti_sn_bridge *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_sn_bridge *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_sn_bridge *pdata = pwm_chip_to_ti_sn_bridge(chip);
> > +	unsigned int pwm_en_inv;
> > +	unsigned int backlight;
> > +	unsigned int pwm_freq;
> > +	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),
> > +					 SN_GPIO_MUX_SPECIAL << (2 * SN_PWM_GPIO));
> > +		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)
> > +		 *
> > +		 * In order to find the PWM_FREQ that best suits the requested
> > +		 * state->period, the PWM_PRE_DIV is calculated with the
> > +		 * maximum possible number of steps (BACKLIGHT_SCALE_MAX). The
> > +		 * actual BACKLIGHT_SCALE is then adjusted down to match the
> > +		 * requested period.
> > +		 *
> > +		 * The BACKLIGHT value is then calculated against the
> > +		 * BACKLIGHT_SCALE, based on the requested duty_cycle and
> > +		 * period.
> > +		 */
> > +		pwm_freq = NSEC_PER_SEC / state->period;
> > +		pre_div = DIV_ROUND_UP(pdata->pwm_refclk / pwm_freq - 1, BACKLIGHT_SCALE_MAX);
> > +		scale = (pdata->pwm_refclk / pwm_freq - 1) / pre_div;
> > +
> > +		backlight = scale * state->duty_cycle / 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_sn_bridge_write_u16(pdata, SN_BACKLIGHT_SCALE_REG, scale);
> > +		ti_sn_bridge_write_u16(pdata, SN_BACKLIGHT_REG, backlight);
> > +	}
> > +
> > +	pwm_en_inv = FIELD_PREP(BIT(1), !!state->enabled) |
> > +		     FIELD_PREP(BIT(0), 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 const struct pwm_ops ti_sn_pwm_ops = {
> > +	.request = ti_sn_pwm_request,
> > +	.free = ti_sn_pwm_free,
> > +	.apply = ti_sn_pwm_apply,
> > +	.owner = THIS_MODULE,
> > +};
> > +
> > +static struct pwm_device *ti_sn_pwm_of_xlate(struct pwm_chip *pc,
> > +					     const struct of_phandle_args *args)
> > +{
> > +	struct pwm_device *pwm;
> > +
> > +	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];
> > +
> > +	return pwm;
> > +}
> > +
> > +static int ti_sn_setup_pwmchip(struct ti_sn_bridge *pdata)
> > +{
> > +	pdata->pchip.dev = pdata->dev;
> > +	pdata->pchip.ops = &ti_sn_pwm_ops;
> > +	pdata->pchip.base = -1;
> > +	pdata->pchip.npwm = 1;
> > +	pdata->pchip.of_xlate = ti_sn_pwm_of_xlate;
> > +	pdata->pchip.of_pwm_n_cells = 1;
> > +
> > +	return pwmchip_add(&pdata->pchip);
> > +}
> > +
> > +static void ti_sn_remove_pwmchip(struct ti_sn_bridge *pdata)
> > +{
> > +	pwmchip_remove(&pdata->pchip);
> > +
> > +	if (pdata->pwm_enabled)
> > +		pm_runtime_put_sync(pdata->dev);
> > +}
> > +#else
> > +static int ti_sn_pwm_pin_request(struct ti_sn_bridge *pdata) { return 0; }
> > +static void ti_sn_pwm_pin_release(struct ti_sn_bridge *pdata) {}
> > +static int ti_sn_setup_pwmchip(struct ti_sn_bridge *pdata) { return 0; }
> > +static void ti_sn_remove_pwmchip(struct ti_sn_bridge *pdata) {}
> > +#endif
> > +
> >  #if defined(CONFIG_OF_GPIO)
> >  
> >  static int tn_sn_bridge_of_xlate(struct gpio_chip *chip,
> > @@ -1113,10 +1291,25 @@ static int ti_sn_bridge_gpio_direction_output(struct gpio_chip *chip,
> >  	return ret;
> >  }
> >  
> > +static int ti_sn_bridge_gpio_request(struct gpio_chip *chip, unsigned int offset)
> > +{
> > +	struct ti_sn_bridge *pdata = gpiochip_get_data(chip);
> > +
> > +	if (offset == SN_PWM_GPIO)
> > +		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_sn_bridge *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)
> > +		ti_sn_pwm_pin_release(pdata);
> >  }
> >  
> >  static const char * const ti_sn_bridge_gpio_names[SN_NUM_GPIOS] = {
> > @@ -1136,6 +1329,7 @@ static int ti_sn_setup_gpio_controller(struct ti_sn_bridge *pdata)
> >  	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;
> > @@ -1282,6 +1476,12 @@ static int ti_sn_bridge_probe(struct i2c_client *client,
> >  		return ret;
> >  	}
> >  
> > +	ret = ti_sn_setup_pwmchip(pdata);
> > +	if (ret)  {
> > +		pm_runtime_disable(pdata->dev);
> > +		return ret;
> > +	}
> > +
> >  	i2c_set_clientdata(client, pdata);
> >  
> >  	pdata->aux.name = "ti-sn65dsi86-aux";
> > @@ -1320,6 +1520,8 @@ static int ti_sn_bridge_remove(struct i2c_client *client)
> >  
> >  	drm_bridge_remove(&pdata->bridge);
> >  
> > +	ti_sn_remove_pwmchip(pdata);
> > +
> >  	return 0;
> >  }
> >  
> > -- 
> > 2.29.2
> > 

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

[Bjorn Andersson]

On Wed 09 Dec 19:51 CST 2020, Shawn Guo wrote:

> On Mon, Dec 07, 2020 at 10:40:22PM -0600, 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.
> > 
> > Special thanks to Doug Anderson for suggestions related to the involved
> > math.
> > 
> > Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> > ---
> >  drivers/gpu/drm/bridge/ti-sn65dsi86.c | 202 ++++++++++++++++++++++++++
> >  1 file changed, 202 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> > index f27306c51e4d..43c0acba57ab 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/bits.h>
> >  #include <linux/clk.h>
> >  #include <linux/debugfs.h>
> > @@ -14,6 +15,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>
> >  
> > @@ -89,6 +91,11 @@
> >  #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_AUX_CMD_STATUS_REG			0xF4
> >  #define  AUX_IRQ_STATUS_AUX_RPLY_TOUT		BIT(3)
> >  #define  AUX_IRQ_STATUS_AUX_SHORT		BIT(5)
> > @@ -111,6 +118,8 @@
> >  
> >  #define SN_LINK_TRAINING_TRIES		10
> >  
> > +#define SN_PWM_GPIO			3
> 
> So this maps to the GPIO4 described in sn65dsi86 datasheet.  I'm
> wondering if it's more readable to define the following SHIFT constants
> (your code), and use GPIO_MUX_GPIO4_SHIFT >> 2 where you need GPIO
> offset?
> 
> #define  GPIO_MUX_GPIO1_SHIFT	0
> #define  GPIO_MUX_GPIO2_SHIFT	2
> #define  GPIO_MUX_GPIO3_SHIFT	4
> #define  GPIO_MUX_GPIO4_SHIFT	6
> 
> If you agree, you may consider to integrate this patch beforehand:
> 

Afaict this is the only place in the driver where the gpio number is a
compile time constant and as you say I need both the shifted value and
the value itself in the patch. But I think it's worth clarifying that
"3" means GPIO 4, so if nothing else I should add a comment about that.

> https://github.com/shawnguo2/linux/commit/7cde887ffb3b27a36e77a08bee3666d14968b586
> 

These looks rather generic, but I like the consistency. Feel free to
post this and I'll review it for you.

Regards,
Bjorn

> 
> Shawn
> 
> > +
> >  /**
> >   * struct ti_sn_bridge - Platform data for ti-sn65dsi86 driver.
> >   * @dev:          Pointer to our device.
> > @@ -162,6 +171,12 @@ struct ti_sn_bridge {
> >  	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;
> > +	atomic_t			pwm_pin_busy;
> > +#endif
> >  };
> >  
> >  static const struct regmap_range ti_sn_bridge_volatile_ranges[] = {
> > @@ -499,6 +514,14 @@ static void ti_sn_bridge_set_refclk_freq(struct ti_sn_bridge *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 = ti_sn_bridge_refclk_lut[i];
> > +#endif
> >  }
> >  
> >  static void ti_sn_bridge_set_dsi_rate(struct ti_sn_bridge *pdata)
> > @@ -981,6 +1004,161 @@ static int ti_sn_bridge_parse_dsi_host(struct ti_sn_bridge *pdata)
> >  	return 0;
> >  }
> >  
> > +#if defined(CONFIG_PWM)
> > +static int ti_sn_pwm_pin_request(struct ti_sn_bridge *pdata)
> > +{
> > +	return atomic_xchg(&pdata->pwm_pin_busy, 1) ? -EBUSY : 0;
> > +}
> > +
> > +static void ti_sn_pwm_pin_release(struct ti_sn_bridge *pdata)
> > +{
> > +	atomic_set(&pdata->pwm_pin_busy, 0);
> > +}
> > +
> > +static struct ti_sn_bridge *
> > +pwm_chip_to_ti_sn_bridge(struct pwm_chip *chip)
> > +{
> > +	return container_of(chip, struct ti_sn_bridge, pchip);
> > +}
> > +
> > +static int ti_sn_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm)
> > +{
> > +	struct ti_sn_bridge *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_sn_bridge *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_sn_bridge *pdata = pwm_chip_to_ti_sn_bridge(chip);
> > +	unsigned int pwm_en_inv;
> > +	unsigned int backlight;
> > +	unsigned int pwm_freq;
> > +	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),
> > +					 SN_GPIO_MUX_SPECIAL << (2 * SN_PWM_GPIO));
> > +		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)
> > +		 *
> > +		 * In order to find the PWM_FREQ that best suits the requested
> > +		 * state->period, the PWM_PRE_DIV is calculated with the
> > +		 * maximum possible number of steps (BACKLIGHT_SCALE_MAX). The
> > +		 * actual BACKLIGHT_SCALE is then adjusted down to match the
> > +		 * requested period.
> > +		 *
> > +		 * The BACKLIGHT value is then calculated against the
> > +		 * BACKLIGHT_SCALE, based on the requested duty_cycle and
> > +		 * period.
> > +		 */
> > +		pwm_freq = NSEC_PER_SEC / state->period;
> > +		pre_div = DIV_ROUND_UP(pdata->pwm_refclk / pwm_freq - 1, BACKLIGHT_SCALE_MAX);
> > +		scale = (pdata->pwm_refclk / pwm_freq - 1) / pre_div;
> > +
> > +		backlight = scale * state->duty_cycle / 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_sn_bridge_write_u16(pdata, SN_BACKLIGHT_SCALE_REG, scale);
> > +		ti_sn_bridge_write_u16(pdata, SN_BACKLIGHT_REG, backlight);
> > +	}
> > +
> > +	pwm_en_inv = FIELD_PREP(BIT(1), !!state->enabled) |
> > +		     FIELD_PREP(BIT(0), 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 const struct pwm_ops ti_sn_pwm_ops = {
> > +	.request = ti_sn_pwm_request,
> > +	.free = ti_sn_pwm_free,
> > +	.apply = ti_sn_pwm_apply,
> > +	.owner = THIS_MODULE,
> > +};
> > +
> > +static struct pwm_device *ti_sn_pwm_of_xlate(struct pwm_chip *pc,
> > +					     const struct of_phandle_args *args)
> > +{
> > +	struct pwm_device *pwm;
> > +
> > +	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];
> > +
> > +	return pwm;
> > +}
> > +
> > +static int ti_sn_setup_pwmchip(struct ti_sn_bridge *pdata)
> > +{
> > +	pdata->pchip.dev = pdata->dev;
> > +	pdata->pchip.ops = &ti_sn_pwm_ops;
> > +	pdata->pchip.base = -1;
> > +	pdata->pchip.npwm = 1;
> > +	pdata->pchip.of_xlate = ti_sn_pwm_of_xlate;
> > +	pdata->pchip.of_pwm_n_cells = 1;
> > +
> > +	return pwmchip_add(&pdata->pchip);
> > +}
> > +
> > +static void ti_sn_remove_pwmchip(struct ti_sn_bridge *pdata)
> > +{
> > +	pwmchip_remove(&pdata->pchip);
> > +
> > +	if (pdata->pwm_enabled)
> > +		pm_runtime_put_sync(pdata->dev);
> > +}
> > +#else
> > +static int ti_sn_pwm_pin_request(struct ti_sn_bridge *pdata) { return 0; }
> > +static void ti_sn_pwm_pin_release(struct ti_sn_bridge *pdata) {}
> > +static int ti_sn_setup_pwmchip(struct ti_sn_bridge *pdata) { return 0; }
> > +static void ti_sn_remove_pwmchip(struct ti_sn_bridge *pdata) {}
> > +#endif
> > +
> >  #if defined(CONFIG_OF_GPIO)
> >  
> >  static int tn_sn_bridge_of_xlate(struct gpio_chip *chip,
> > @@ -1113,10 +1291,25 @@ static int ti_sn_bridge_gpio_direction_output(struct gpio_chip *chip,
> >  	return ret;
> >  }
> >  
> > +static int ti_sn_bridge_gpio_request(struct gpio_chip *chip, unsigned int offset)
> > +{
> > +	struct ti_sn_bridge *pdata = gpiochip_get_data(chip);
> > +
> > +	if (offset == SN_PWM_GPIO)
> > +		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_sn_bridge *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)
> > +		ti_sn_pwm_pin_release(pdata);
> >  }
> >  
> >  static const char * const ti_sn_bridge_gpio_names[SN_NUM_GPIOS] = {
> > @@ -1136,6 +1329,7 @@ static int ti_sn_setup_gpio_controller(struct ti_sn_bridge *pdata)
> >  	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;
> > @@ -1282,6 +1476,12 @@ static int ti_sn_bridge_probe(struct i2c_client *client,
> >  		return ret;
> >  	}
> >  
> > +	ret = ti_sn_setup_pwmchip(pdata);
> > +	if (ret)  {
> > +		pm_runtime_disable(pdata->dev);
> > +		return ret;
> > +	}
> > +
> >  	i2c_set_clientdata(client, pdata);
> >  
> >  	pdata->aux.name = "ti-sn65dsi86-aux";
> > @@ -1320,6 +1520,8 @@ static int ti_sn_bridge_remove(struct i2c_client *client)
> >  
> >  	drm_bridge_remove(&pdata->bridge);
> >  
> > +	ti_sn_remove_pwmchip(pdata);
> > +
> >  	return 0;
> >  }
> >  
> > -- 
> > 2.29.2
> > 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Uwe Kleine-König]

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

Hi Shawn,

On Thu, Dec 10, 2020 at 09:51:37AM +0800, Shawn Guo wrote:
> On Mon, Dec 07, 2020 at 10:40:22PM -0600, 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.
> > 
> > Special thanks to Doug Anderson for suggestions related to the involved
> > math.
> > 
> > Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> > ---
> >  drivers/gpu/drm/bridge/ti-sn65dsi86.c | 202 ++++++++++++++++++++++++++
> >  1 file changed, 202 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> > index f27306c51e4d..43c0acba57ab 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/bits.h>
> >  #include <linux/clk.h>
> >  #include <linux/debugfs.h>
> > @@ -14,6 +15,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>
> >  
> > @@ -89,6 +91,11 @@
> >  #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_AUX_CMD_STATUS_REG			0xF4
> >  #define  AUX_IRQ_STATUS_AUX_RPLY_TOUT		BIT(3)
> >  #define  AUX_IRQ_STATUS_AUX_SHORT		BIT(5)
> > @@ -111,6 +118,8 @@
> >  
> >  #define SN_LINK_TRAINING_TRIES		10
> >  
> > +#define SN_PWM_GPIO			3
> 
> So this maps to the GPIO4 described in sn65dsi86 datasheet.  I'm
> wondering if it's more readable to define the following SHIFT constants
> (your code), and use GPIO_MUX_GPIO4_SHIFT >> 2 where you need GPIO
> offset?
> 
> #define  GPIO_MUX_GPIO1_SHIFT	0
> #define  GPIO_MUX_GPIO2_SHIFT	2
> #define  GPIO_MUX_GPIO3_SHIFT	4
> #define  GPIO_MUX_GPIO4_SHIFT	6
> 
> If you agree, you may consider to integrate this patch beforehand:
> 
> https://github.com/shawnguo2/linux/commit/7cde887ffb3b27a36e77a08bee3666d14968b586

My preferred way here would be to add a prefix for the other constants.
It (IMHO) looks nicer and

	GPIO_INPUT_SHIFT

looks like a quite generic name for a hardware specific definition.
(Even if up to now there is no other code location using this name.)

Best regards
Uwe

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

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

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

[Uwe Kleine-König]

[-- Attachment #1.1: Type: text/plain, Size: 3022 bytes --]

Hi Shawn,

On Thu, Dec 10, 2020 at 09:51:37AM +0800, Shawn Guo wrote:
> On Mon, Dec 07, 2020 at 10:40:22PM -0600, 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.
> > 
> > Special thanks to Doug Anderson for suggestions related to the involved
> > math.
> > 
> > Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> > ---
> >  drivers/gpu/drm/bridge/ti-sn65dsi86.c | 202 ++++++++++++++++++++++++++
> >  1 file changed, 202 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> > index f27306c51e4d..43c0acba57ab 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/bits.h>
> >  #include <linux/clk.h>
> >  #include <linux/debugfs.h>
> > @@ -14,6 +15,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>
> >  
> > @@ -89,6 +91,11 @@
> >  #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_AUX_CMD_STATUS_REG			0xF4
> >  #define  AUX_IRQ_STATUS_AUX_RPLY_TOUT		BIT(3)
> >  #define  AUX_IRQ_STATUS_AUX_SHORT		BIT(5)
> > @@ -111,6 +118,8 @@
> >  
> >  #define SN_LINK_TRAINING_TRIES		10
> >  
> > +#define SN_PWM_GPIO			3
> 
> So this maps to the GPIO4 described in sn65dsi86 datasheet.  I'm
> wondering if it's more readable to define the following SHIFT constants
> (your code), and use GPIO_MUX_GPIO4_SHIFT >> 2 where you need GPIO
> offset?
> 
> #define  GPIO_MUX_GPIO1_SHIFT	0
> #define  GPIO_MUX_GPIO2_SHIFT	2
> #define  GPIO_MUX_GPIO3_SHIFT	4
> #define  GPIO_MUX_GPIO4_SHIFT	6
> 
> If you agree, you may consider to integrate this patch beforehand:
> 
> https://github.com/shawnguo2/linux/commit/7cde887ffb3b27a36e77a08bee3666d14968b586

My preferred way here would be to add a prefix for the other constants.
It (IMHO) looks nicer and

	GPIO_INPUT_SHIFT

looks like a quite generic name for a hardware specific definition.
(Even if up to now there is no other code location using this name.)

Best regards
Uwe

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

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

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Shawn Guo]

Hi Uwe,

On Thu, Dec 10, 2020 at 9:05 PM Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> wrote:
> > > @@ -111,6 +118,8 @@
> > >
> > >  #define SN_LINK_TRAINING_TRIES             10
> > >
> > > +#define SN_PWM_GPIO                        3
> >
> > So this maps to the GPIO4 described in sn65dsi86 datasheet.  I'm
> > wondering if it's more readable to define the following SHIFT constants
> > (your code), and use GPIO_MUX_GPIO4_SHIFT >> 2 where you need GPIO
> > offset?
> >
> > #define  GPIO_MUX_GPIO1_SHIFT 0
> > #define  GPIO_MUX_GPIO2_SHIFT 2
> > #define  GPIO_MUX_GPIO3_SHIFT 4
> > #define  GPIO_MUX_GPIO4_SHIFT 6
> >
> > If you agree, you may consider to integrate this patch beforehand:
> >
> > https://github.com/shawnguo2/linux/commit/7cde887ffb3b27a36e77a08bee3666d14968b586
>
> My preferred way here would be to add a prefix for the other constants.
> It (IMHO) looks nicer and
>
>         GPIO_INPUT_SHIFT
>
> looks like a quite generic name for a hardware specific definition.

While this looks like a reasonable argument, I also like the naming
choice for these constants in the beginning for that distinction
between registers and bits.  And changing the names the other way
around means there will be a much bigger diffstat, which I would like
to avoid.  I suggest let's just focus on what really matters here -
keep the naming consistent, so that people do not get confused when
they want to add more constants in there.

Shawn

> (Even if up to now there is no other code location using this name.)

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

[Shawn Guo]

Hi Uwe,

On Thu, Dec 10, 2020 at 9:05 PM Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> wrote:
> > > @@ -111,6 +118,8 @@
> > >
> > >  #define SN_LINK_TRAINING_TRIES             10
> > >
> > > +#define SN_PWM_GPIO                        3
> >
> > So this maps to the GPIO4 described in sn65dsi86 datasheet.  I'm
> > wondering if it's more readable to define the following SHIFT constants
> > (your code), and use GPIO_MUX_GPIO4_SHIFT >> 2 where you need GPIO
> > offset?
> >
> > #define  GPIO_MUX_GPIO1_SHIFT 0
> > #define  GPIO_MUX_GPIO2_SHIFT 2
> > #define  GPIO_MUX_GPIO3_SHIFT 4
> > #define  GPIO_MUX_GPIO4_SHIFT 6
> >
> > If you agree, you may consider to integrate this patch beforehand:
> >
> > https://github.com/shawnguo2/linux/commit/7cde887ffb3b27a36e77a08bee3666d14968b586
>
> My preferred way here would be to add a prefix for the other constants.
> It (IMHO) looks nicer and
>
>         GPIO_INPUT_SHIFT
>
> looks like a quite generic name for a hardware specific definition.

While this looks like a reasonable argument, I also like the naming
choice for these constants in the beginning for that distinction
between registers and bits.  And changing the names the other way
around means there will be a much bigger diffstat, which I would like
to avoid.  I suggest let's just focus on what really matters here -
keep the naming consistent, so that people do not get confused when
they want to add more constants in there.

Shawn

> (Even if up to now there is no other code location using this name.)
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Uwe Kleine-König]

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

On Thu, Dec 10, 2020 at 10:40:36PM +0800, Shawn Guo wrote:
> Hi Uwe,
> 
> On Thu, Dec 10, 2020 at 9:05 PM Uwe Kleine-König
> <u.kleine-koenig@pengutronix.de> wrote:
> > > > @@ -111,6 +118,8 @@
> > > >
> > > >  #define SN_LINK_TRAINING_TRIES             10
> > > >
> > > > +#define SN_PWM_GPIO                        3
> > >
> > > So this maps to the GPIO4 described in sn65dsi86 datasheet.  I'm
> > > wondering if it's more readable to define the following SHIFT constants
> > > (your code), and use GPIO_MUX_GPIO4_SHIFT >> 2 where you need GPIO
> > > offset?
> > >
> > > #define  GPIO_MUX_GPIO1_SHIFT 0
> > > #define  GPIO_MUX_GPIO2_SHIFT 2
> > > #define  GPIO_MUX_GPIO3_SHIFT 4
> > > #define  GPIO_MUX_GPIO4_SHIFT 6
> > >
> > > If you agree, you may consider to integrate this patch beforehand:
> > >
> > > https://github.com/shawnguo2/linux/commit/7cde887ffb3b27a36e77a08bee3666d14968b586
> >
> > My preferred way here would be to add a prefix for the other constants.
> > It (IMHO) looks nicer and
> >
> >         GPIO_INPUT_SHIFT
> >
> > looks like a quite generic name for a hardware specific definition.
> 
> While this looks like a reasonable argument, I also like the naming
> choice for these constants in the beginning for that distinction
> between registers and bits.  And changing the names the other way
> around means there will be a much bigger diffstat, which I would like
> to avoid.  I suggest let's just focus on what really matters here -
> keep the naming consistent, so that people do not get confused when
> they want to add more constants in there.

In my eyes the bigger diffstat is justified. As I wrote,
GPIO_INPUT_SHIFT isn't used in other files, but please look how many
definitions there are for RESET. The usefulness of ctags/cscope is quite
reduced if generic terms are used this way.

Best regards
Uwe

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

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

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

[Uwe Kleine-König]

[-- Attachment #1.1: Type: text/plain, Size: 2043 bytes --]

On Thu, Dec 10, 2020 at 10:40:36PM +0800, Shawn Guo wrote:
> Hi Uwe,
> 
> On Thu, Dec 10, 2020 at 9:05 PM Uwe Kleine-König
> <u.kleine-koenig@pengutronix.de> wrote:
> > > > @@ -111,6 +118,8 @@
> > > >
> > > >  #define SN_LINK_TRAINING_TRIES             10
> > > >
> > > > +#define SN_PWM_GPIO                        3
> > >
> > > So this maps to the GPIO4 described in sn65dsi86 datasheet.  I'm
> > > wondering if it's more readable to define the following SHIFT constants
> > > (your code), and use GPIO_MUX_GPIO4_SHIFT >> 2 where you need GPIO
> > > offset?
> > >
> > > #define  GPIO_MUX_GPIO1_SHIFT 0
> > > #define  GPIO_MUX_GPIO2_SHIFT 2
> > > #define  GPIO_MUX_GPIO3_SHIFT 4
> > > #define  GPIO_MUX_GPIO4_SHIFT 6
> > >
> > > If you agree, you may consider to integrate this patch beforehand:
> > >
> > > https://github.com/shawnguo2/linux/commit/7cde887ffb3b27a36e77a08bee3666d14968b586
> >
> > My preferred way here would be to add a prefix for the other constants.
> > It (IMHO) looks nicer and
> >
> >         GPIO_INPUT_SHIFT
> >
> > looks like a quite generic name for a hardware specific definition.
> 
> While this looks like a reasonable argument, I also like the naming
> choice for these constants in the beginning for that distinction
> between registers and bits.  And changing the names the other way
> around means there will be a much bigger diffstat, which I would like
> to avoid.  I suggest let's just focus on what really matters here -
> keep the naming consistent, so that people do not get confused when
> they want to add more constants in there.

In my eyes the bigger diffstat is justified. As I wrote,
GPIO_INPUT_SHIFT isn't used in other files, but please look how many
definitions there are for RESET. The usefulness of ctags/cscope is quite
reduced if generic terms are used this way.

Best regards
Uwe

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

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

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel