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 <[email protected]>
---
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
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/ |
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/ |
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 <[email protected]>
> ---
> 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
>
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 <[email protected]>
> > ---
> > 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
> >
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 <[email protected]>
> > ---
> > 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/ |
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
> <[email protected]> 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/ |
Hi Uwe,
On Thu, Dec 10, 2020 at 9:05 PM Uwe Kleine-König
<[email protected]> 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.)