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; }
Hello,
On Mon, Dec 07, 2020 at 10:40:22PM -0600, Bjorn Andersson wrote:
Did you test this with CONFIG_PWM_DEBUG? (I think you didn't, because otherwise there would be a .get_state callback.)
Would it make sense to introduce a separate config symbol for this? Something like CONFIG_PWM_SN65DSI87?
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.
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?)
All your functions share the same function prefix (which is fine), but this one doesn't.
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'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.
This is an u64 division, you must use do_div for that. Also you're losing precision here.
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?
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?
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.
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.
Best regards Uwe
On Tue 08 Dec 02:04 CST 2020, Uwe Kleine-K?nig wrote:
I had not, but have now explored this further and will follow up with a get_state implementation :)
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.
Yes, sorry for missing this.
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...
I'll fix that.
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...
Yes, this matches my math.
Which would imply that the PWM period is (PRE_DIV * SCALE + 1) ticks of the refclk.
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.
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;
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
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.
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.
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
On Mon, Dec 07, 2020 at 10:40:22PM -0600, Bjorn Andersson wrote:
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/7cde887ffb3b27a36e77a08bee3666d149...
Shawn
On Wed 09 Dec 19:51 CST 2020, Shawn Guo wrote:
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/7cde887ffb3b27a36e77a08bee3666d149...
These looks rather generic, but I like the consistency. Feel free to post this and I'll review it for you.
Regards, Bjorn
Hi Shawn,
On Thu, Dec 10, 2020 at 09:51:37AM +0800, Shawn Guo wrote:
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
Hi Uwe,
On Thu, Dec 10, 2020 at 9:05 PM Uwe Kleine-König u.kleine-koenig@pengutronix.de wrote:
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.)
On Thu, Dec 10, 2020 at 10:40:36PM +0800, Shawn Guo wrote:
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
dri-devel@lists.freedesktop.org
-
Bjorn Andersson -
Shawn Guo -
Uwe Kleine-König