| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20210924075433.apkdkwur5ar67ge2@pengutronix.de>
Date: Fri, 24 Sep 2021 09:54:33 +0200
From: Uwe Kleine-König <u.kleine-koenig@...gutronix.de>
To: Bjorn Andersson <bjorn.andersson@...aro.org>
Cc: Andrzej Hajda <a.hajda@...sung.com>,
Neil Armstrong <narmstrong@...libre.com>,
Robert Foss <robert.foss@...aro.org>,
Laurent Pinchart <Laurent.pinchart@...asonboard.com>,
Jonas Karlman <jonas@...boo.se>,
Jernej Skrabec <jernej.skrabec@...il.com>,
David Airlie <airlied@...ux.ie>,
Daniel Vetter <daniel@...ll.ch>,
Thierry Reding <thierry.reding@...il.com>,
Lee Jones <lee.jones@...aro.org>,
dri-devel@...ts.freedesktop.org, linux-kernel@...r.kernel.org,
linux-pwm@...r.kernel.org, Doug Anderson <dianders@...gle.com>
Subject: Re: [PATCH v5 2/2] drm/bridge: ti-sn65dsi86: Implement the pwm_chip
Hello,
On Thu, Sep 23, 2021 at 09:12:25PM -0500, Bjorn Andersson wrote:
> The SN65DSI86 provides the ability to supply a PWM signal on GPIO 4,
> with the primary purpose of controlling the backlight of the attached
> panel. Add an implementation that exposes this using the standard PWM
> framework, to allow e.g. pwm-backlight to expose this to the user.
>
> Signed-off-by: Bjorn Andersson <bjorn.andersson@...aro.org>
> ---
>
> Changes since v4:
> - Rebased the patch
> - Turned ti_sn65dsi86_write_u16 into using regmap bulk write
> - Moved pwm_refclk_freq out of the #if CONFIG_PWM, to allow removing the guard
> from ti_sn_bridge_set_refclk_freq()
> - Updates "Limitations" text
> - pm_runtime_put_sync() on pm_runtime_get_sync() failure
> - Added parenthesis around (scale + 1) in the PWM_FREQ formula and thereby
> redid all the math
> - Rewrote the comments related to the math
> - Rewrote the math for calculating the backlight (duty cycle) register value
> - Dropped some !! on already boolean state->enabled
> - Dropped a spurious newline
> - Rewrote comment in ti_sn65dsi86_probe() talking about future PWM work
>
> drivers/gpu/drm/bridge/ti-sn65dsi86.c | 369 +++++++++++++++++++++++++-
> 1 file changed, 361 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> index 41d48a393e7f..857a53dc6c75 100644
> --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> @@ -4,7 +4,9 @@
> * datasheet: https://www.ti.com/lit/ds/symlink/sn65dsi86.pdf
> */
>
> +#include <linux/atomic.h>
> #include <linux/auxiliary_bus.h>
> +#include <linux/bitfield.h>
> #include <linux/bits.h>
> #include <linux/clk.h>
> #include <linux/debugfs.h>
> @@ -15,6 +17,7 @@
> #include <linux/module.h>
> #include <linux/of_graph.h>
> #include <linux/pm_runtime.h>
> +#include <linux/pwm.h>
> #include <linux/regmap.h>
> #include <linux/regulator/consumer.h>
>
> @@ -91,6 +94,13 @@
> #define SN_ML_TX_MODE_REG 0x96
> #define ML_TX_MAIN_LINK_OFF 0
> #define ML_TX_NORMAL_MODE BIT(0)
> +#define SN_PWM_PRE_DIV_REG 0xA0
> +#define SN_BACKLIGHT_SCALE_REG 0xA1
> +#define BACKLIGHT_SCALE_MAX 0xFFFF
> +#define SN_BACKLIGHT_REG 0xA3
> +#define SN_PWM_EN_INV_REG 0xA5
> +#define SN_PWM_INV_MASK BIT(0)
> +#define SN_PWM_EN_MASK BIT(1)
> #define SN_AUX_CMD_STATUS_REG 0xF4
> #define AUX_IRQ_STATUS_AUX_RPLY_TOUT BIT(3)
> #define AUX_IRQ_STATUS_AUX_SHORT BIT(5)
> @@ -113,11 +123,14 @@
>
> #define SN_LINK_TRAINING_TRIES 10
>
> +#define SN_PWM_GPIO_IDX 3 /* 4th GPIO */
> +
> /**
> * struct ti_sn65dsi86 - Platform data for ti-sn65dsi86 driver.
> * @bridge_aux: AUX-bus sub device for MIPI-to-eDP bridge functionality.
> * @gpio_aux: AUX-bus sub device for GPIO controller functionality.
> * @aux_aux: AUX-bus sub device for eDP AUX channel functionality.
> + * @pwm_aux: AUX-bus sub device for PWM controller functionality.
> *
> * @dev: Pointer to the top level (i2c) device.
> * @regmap: Regmap for accessing i2c.
> @@ -145,11 +158,17 @@
> * bitmap so we can do atomic ops on it without an extra
> * lock so concurrent users of our 4 GPIOs don't stomp on
> * each other's read-modify-write.
> + *
> + * @pchip: pwm_chip if the PWM is exposed.
> + * @pwm_enabled: Used to track if the PWM signal is currently enabled.
> + * @pwm_pin_busy: Track if GPIO4 is currently requested for GPIO or PWM.
> + * @pwm_refclk_freq: Cache for the reference clock input to the PWM.
> */
> struct ti_sn65dsi86 {
> struct auxiliary_device bridge_aux;
> struct auxiliary_device gpio_aux;
> struct auxiliary_device aux_aux;
> + struct auxiliary_device pwm_aux;
>
> struct device *dev;
> struct regmap *regmap;
> @@ -172,6 +191,12 @@ struct ti_sn65dsi86 {
> struct gpio_chip gchip;
> DECLARE_BITMAP(gchip_output, SN_NUM_GPIOS);
> #endif
> +#if defined(CONFIG_PWM)
> + struct pwm_chip pchip;
> + bool pwm_enabled;
> + atomic_t pwm_pin_busy;
> +#endif
> + unsigned int pwm_refclk_freq;
> };
>
> static const struct regmap_range ti_sn65dsi86_volatile_ranges[] = {
> @@ -190,11 +215,31 @@ static const struct regmap_config ti_sn65dsi86_regmap_config = {
> .cache_type = REGCACHE_NONE,
> };
>
> +static int ti_sn65dsi86_read_u16(struct ti_sn65dsi86 *pdata,
> + unsigned int reg, u16 *val)
> +{
> + unsigned int tmp;
> + int ret;
> +
> + ret = regmap_read(pdata->regmap, reg, &tmp);
> + if (ret)
> + return ret;
> + *val = tmp;
> +
> + ret = regmap_read(pdata->regmap, reg + 1, &tmp);
> + if (ret)
> + return ret;
> + *val |= tmp << 8;
> +
> + return 0;
> +}
> +
> static void ti_sn65dsi86_write_u16(struct ti_sn65dsi86 *pdata,
> unsigned int reg, u16 val)
> {
> - regmap_write(pdata->regmap, reg, val & 0xFF);
> - regmap_write(pdata->regmap, reg + 1, val >> 8);
> + u8 buf[2] = { val & 0xff, val >> 8 };
> +
> + regmap_bulk_write(pdata->regmap, reg, &buf, ARRAY_SIZE(buf));
Given that ti_sn65dsi86_write_u16 uses regmap_bulk_write I wonder why
ti_sn65dsi86_read_u16 doesn't use regmap_bulk_read.
> }
>
> static u32 ti_sn_bridge_get_dsi_freq(struct ti_sn65dsi86 *pdata)
> @@ -253,6 +298,12 @@ static void ti_sn_bridge_set_refclk_freq(struct ti_sn65dsi86 *pdata)
>
> regmap_update_bits(pdata->regmap, SN_DPPLL_SRC_REG, REFCLK_FREQ_MASK,
> REFCLK_FREQ(i));
> +
> + /*
> + * The PWM refclk is based on the value written to SN_DPPLL_SRC_REG,
> + * regardless of its actual sourcing.
> + */
> + pdata->pwm_refclk_freq = ti_sn_bridge_refclk_lut[i];
> }
>
> static void ti_sn65dsi86_enable_comms(struct ti_sn65dsi86 *pdata)
> @@ -1259,9 +1310,283 @@ static struct auxiliary_driver ti_sn_bridge_driver = {
> };
>
> /* -----------------------------------------------------------------------------
> - * GPIO Controller
> + * PWM Controller
> + */
> +#if defined(CONFIG_PWM)
> +static int ti_sn_pwm_pin_request(struct ti_sn65dsi86 *pdata)
> +{
> + return atomic_xchg(&pdata->pwm_pin_busy, 1) ? -EBUSY : 0;
> +}
> +
> +static void ti_sn_pwm_pin_release(struct ti_sn65dsi86 *pdata)
> +{
> + atomic_set(&pdata->pwm_pin_busy, 0);
> +}
> +
> +static struct ti_sn65dsi86 *pwm_chip_to_ti_sn_bridge(struct pwm_chip *chip)
> +{
> + return container_of(chip, struct ti_sn65dsi86, pchip);
> +}
> +
> +static int ti_sn_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm)
> +{
> + struct ti_sn65dsi86 *pdata = pwm_chip_to_ti_sn_bridge(chip);
> +
> + return ti_sn_pwm_pin_request(pdata);
> +}
> +
> +static void ti_sn_pwm_free(struct pwm_chip *chip, struct pwm_device *pwm)
> +{
> + struct ti_sn65dsi86 *pdata = pwm_chip_to_ti_sn_bridge(chip);
> +
> + ti_sn_pwm_pin_release(pdata);
> +}
> +
> +/*
> + * Limitations:
> + * - The PWM signal is not driven when the chip is powered down, or in its
> + * reset state and the driver does not implement the "suspend state"
> + * described in the documentation. In order to save power, state->enabled is
> + * interpreted as denoting if the signal is expected to be valid, and is used
> + * to determine if the chip needs to be kept powered.
> + * - Changing both period and duty_cycle is not done atomically, neither is the
> + * multi-byte register updates, so the output might briefly be undefined
> + * during update.
> */
> +static int ti_sn_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> + const struct pwm_state *state)
> +{
> + struct ti_sn65dsi86 *pdata = pwm_chip_to_ti_sn_bridge(chip);
> + unsigned int pwm_en_inv;
> + unsigned int backlight;
> + unsigned int pre_div;
> + unsigned int scale;
> + u64 period_max;
> + u64 period;
> + int ret;
> +
> + if (!pdata->pwm_enabled) {
> + ret = pm_runtime_get_sync(pdata->dev);
> + if (ret < 0) {
> + pm_runtime_put_sync(pdata->dev);
> + return ret;
> + }
> + }
> +
> + if (state->enabled) {
> + if (!pdata->pwm_enabled) {
> + /*
> + * The chip might have been powered down while we
> + * didn't hold a PM runtime reference, so mux in the
> + * PWM function on the GPIO pin again.
> + */
> + ret = regmap_update_bits(pdata->regmap, SN_GPIO_CTRL_REG,
> + SN_GPIO_MUX_MASK << (2 * SN_PWM_GPIO_IDX),
> + SN_GPIO_MUX_SPECIAL << (2 * SN_PWM_GPIO_IDX));
> + if (ret) {
> + dev_err(pdata->dev, "failed to mux in PWM function\n");
> + goto out;
> + }
> + }
> +
> + /*
> + * Per the datasheet the PWM frequency is given by:
> + *
> + * REFCLK_FREQ
> + * PWM_FREQ = -----------------------------------
> + * PWM_PRE_DIV * BACKLIGHT_SCALE + 1
> + *
> + * Unfortunately the math becomes overly complex unless we
> + * assume that this formula is missing parenthesis around
> + * BACKLIGHT_SCALE + 1.
We shouldn't assume a different formula than the reference manual
describes because it's complex. The reasoning should be that the
reference manual is wrong and that someone has convinced themselves the
assumed formula is the right one instead.
With the assumed formula: What happens if PWM_PRE_DIV is 0?
> + * With that the formula can be written:
> + *
> + * T_pwm * REFCLK_FREQ = PWM_PRE_DIV * (BACKLIGHT_SCALE + 1)
> + *
> + * In order to keep BACKLIGHT_SCALE within its 16 bits,
> + * PWM_PRE_DIV must be:
> + *
> + * T_pwm * REFCLK_FREQ
> + * PWM_PRE_DIV >= -------------------------
> + * BACKLIGHT_SCALE_MAX + 1
> + *
> + * To simplify the search and optimize the resolution of the
> + * PWM, the lowest possible PWM_PRE_DIV is used. Finally the
> + * scale is calculated as:
> + *
> + * T_pwm * REFCLK_FREQ
> + * BACKLIGHT_SCALE = ---------------------- - 1
> + * PWM_PRE_DIV
> + *
> + * Here T_pwm is represented in seconds, so appropriate scaling
> + * to nanoseconds is necessary.
> + */
> +
> + /* Minimum T_pwm is (0 * 1) / REFCLK_FREQ */
So the minimal T_pwm (which corresponds to .period, right?) is 0? That
sounds wrong.
> + if (state->period <= NSEC_PER_SEC / pdata->pwm_refclk_freq) {
The implemented check assumes 1 / REFCLK_FREQ, which looks more reasonable.
> + ret = -EINVAL;
> + goto out;
> + }
> +
> + /*
> + * Maximum T_pwm is 255 * (65535 + 1) / REFCLK_FREQ
> + * Limit period to this to avoid overflows
> + */
> + period_max = div_u64((u64)NSEC_PER_SEC * 255 * (65535 + 1), pdata->pwm_refclk_freq);
> + if (period > period_max)
> + period = period_max;
> + else
> + period = state->period;
> +
> + pre_div = DIV64_U64_ROUND_UP(period * pdata->pwm_refclk_freq,
> + (u64)NSEC_PER_SEC * (BACKLIGHT_SCALE_MAX + 1));
> + scale = div64_u64(period * pdata->pwm_refclk_freq, (u64)NSEC_PER_SEC * pre_div) - 1;
You're loosing precision here as you divide by the result of a division.
> +
> + /*
> + * The documentation has the duty ratio given as:
> + *
> + * duty BACKLIGHT
> + * ------- = ---------------------
> + * period BACKLIGHT_SCALE + 1
> + *
> + * Solve for BACKLIGHT, substituting BACKLIGHT_SCALE according
> + * to definition above and adjusting for nanosecond
> + * representation of duty cycle gives us:
> + */
> + backlight = div64_u64(state->duty_cycle * pdata->pwm_refclk_freq, (u64)NSEC_PER_SEC * pre_div);
> + if (backlight > scale)
> + backlight = scale;
> +
> + ret = regmap_write(pdata->regmap, SN_PWM_PRE_DIV_REG, pre_div);
> + if (ret) {
> + dev_err(pdata->dev, "failed to update PWM_PRE_DIV\n");
> + goto out;
> + }
>
> + ti_sn65dsi86_write_u16(pdata, SN_BACKLIGHT_SCALE_REG, scale);
> + ti_sn65dsi86_write_u16(pdata, SN_BACKLIGHT_REG, backlight);
> + }
> +
> + pwm_en_inv = FIELD_PREP(SN_PWM_EN_MASK, state->enabled) |
> + FIELD_PREP(SN_PWM_INV_MASK, state->polarity == PWM_POLARITY_INVERSED);
> + ret = regmap_write(pdata->regmap, SN_PWM_EN_INV_REG, pwm_en_inv);
> + if (ret) {
> + dev_err(pdata->dev, "failed to update PWM_EN/PWM_INV\n");
> + goto out;
> + }
> +
> + pdata->pwm_enabled = state->enabled;
> +out:
> +
> + if (!pdata->pwm_enabled)
> + pm_runtime_put_sync(pdata->dev);
> +
> + return ret;
> +}
> +
> +static void ti_sn_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
> + struct pwm_state *state)
> +{
> + struct ti_sn65dsi86 *pdata = pwm_chip_to_ti_sn_bridge(chip);
> + unsigned int pwm_en_inv;
> + unsigned int pre_div;
> + u16 backlight;
> + u16 scale;
> + int ret;
> +
> + ret = regmap_read(pdata->regmap, SN_PWM_EN_INV_REG, &pwm_en_inv);
> + if (ret)
> + return;
> +
> + ret = ti_sn65dsi86_read_u16(pdata, SN_BACKLIGHT_SCALE_REG, &scale);
> + if (ret)
> + return;
> +
> + ret = ti_sn65dsi86_read_u16(pdata, SN_BACKLIGHT_REG, &backlight);
> + if (ret)
> + return;
> +
> + ret = regmap_read(pdata->regmap, SN_PWM_PRE_DIV_REG, &pre_div);
> + if (ret)
> + return;
> +
> + state->enabled = FIELD_GET(SN_PWM_EN_MASK, pwm_en_inv);
> + if (FIELD_GET(SN_PWM_INV_MASK, pwm_en_inv))
> + state->polarity = PWM_POLARITY_INVERSED;
> + else
> + state->polarity = PWM_POLARITY_NORMAL;
> +
> + state->period = DIV_ROUND_UP_ULL((u64)NSEC_PER_SEC * pre_div * (scale + 1),
> + pdata->pwm_refclk_freq);
> + state->duty_cycle = DIV_ROUND_UP_ULL((u64)NSEC_PER_SEC * pre_div * backlight,
> + pdata->pwm_refclk_freq);
What happens if scale + 1 < backlight?
> +}
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | https://www.pengutronix.de/ |
Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)
Powered by blists - more mailing lists