Re: [PATCH v2 2/2] drm/bridge: ti-sn65dsi86: Implement the pwm_chip
Hello Bjorn,
On Thu, Jun 17, 2021 at 01:06:51PM -0500, Bjorn Andersson wrote:
> On Thu 17 Jun 11:54 CDT 2021, Uwe Kleine-K?nig wrote:
> > On Thu, Jun 17, 2021 at 11:38:26AM -0500, Bjorn Andersson wrote:
> > > On Thu 17 Jun 01:24 CDT 2021, Uwe Kleine-K?nig wrote:
> > > > On Wed, Jun 16, 2021 at 10:22:17PM -0500, Bjorn Andersson wrote:
> > > > > > > +static int ti_sn_pwm_apply(struct pwm_chip *chip, struct
> > > > > > > pwm_device *pwm,
> > > > > > > +const struct pwm_state *state)
> > > > > > > +{
> > > > > > > + struct ti_sn65dsi86 *pdata = pwm_chip_to_ti_sn_bridge(chip);
> > > > > > > + unsigned int pwm_en_inv;
> > > > > > > + unsigned int backlight;
> > > > > > > + unsigned int pre_div;
> > > > > > > + unsigned int scale;
> > > > > > > + int ret;
> > > > > > > +
> > > > > > > + if (!pdata->pwm_enabled) {
> > > > > > > + ret = pm_runtime_get_sync(pdata->dev);
> > > > > > > + if (ret < 0)
> > > > > > > + return ret;
> > > > > > > +
> > > > > > > + ret = regmap_update_bits(pdata->regmap,
> > > > > > > SN_GPIO_CTRL_REG,
> > > > > > > + SN_GPIO_MUX_MASK << (2 *
> > > > > > > SN_PWM_GPIO_IDX),
> > > > > > > + SN_GPIO_MUX_SPECIAL << (2 *
> > > > > > > SN_PWM_GPIO_IDX));
> > > > > > > + if (ret) {
> > > > > > > + dev_err(pdata->dev, "failed to mux in PWM
> > > > > > > function\n");
> > > > > > > + goto out;
> > > > > > > + }
> > > > > >
> > > > > > Do you need to do this even if state->enabled is false?
> > > > >
> > > > > I presume I should be able to explicitly mux in the GPIO function and
> > > > > configure that to output low. But I am not able to find anything in
> > > > > the
> > > > > data sheet that would indicate this to be preferred.
> > > >
> > > > My question targetted a different case. If the PWM is off
> > > > (!pdata->pwm_enabled) and should remain off (state->enabled is false)
> > > > you can shortcut here, can you not?
> > >
> > > Right, if we're going off->off then we don't need to touch the hardware.
> > >
> > > But am I expected to -EINVAL improper period and duty cycle even though
> > > enabled is false?
> > >
> > > And also, what is the supposed behavior of enabled = false? Is it
> > > supposedly equivalent of asking for a duty_cycle of 0?
> >
> > In my book enabled = false is just syntactic sugar to say:
> > "duty_cycle=0, period=something small". So to answer your questions: if
> > enabled = false, the consumer doesn't really care about period and
> > duty_cycle. Some care that the output becomes inactive, some others
> > don't, so from my POV just emit the inactive level on the output and
> > ignore period and duty_cycle.
>
> Giving this some further thought.
Very appreciated, still more as you come to the same conclusions as I do
:-)
> In order to have a known state of the PWM signal we need the sn65dsi86
> to be powered. The documentation describes a "suspend mode", but this is
> currently not implemented in the driver, so there's a large power cost
> coming from just keeping the pin low when disabled.
In the past I already promoted the idea that a disabled PWM should give
no guarantees about the output level. In fact there are several
offenders, the ones I know off-hand are:
- pwm-imx27 emits a low level independent of the programmed polarity
- pwm-iqs620a makes the output tristated and so relies on an external
pull to give the inactive level.
- pwm-sl28cpld switches to a GPIO mode on disable which isn't
controlled by the driver
and I assume there are more because before no one actively asked for
and tracked this kind of information.
IMHO a consumer who wants the output to stay inactive should configure
.enabled = true
.period = DC (or something low to allow quick reprogramming)
.duty_cycle = 0
, so there is no loss of functionality and enabled=false should mean the
consumer doesn't care about the output which would allow some lowlevel
drivers to save some more energy. So this makes the API more expressive
because after dropping "disabled results in an inactive output"
consumers can differentiate between "I care about the output staying
inactive" and "I don't care". This in turn allows lowlevel drivers to
better know when they can more aggressively save power and when they
don't.
Back then Thierry didn't like that approach though. (The thread started
with a mail having Message-Id
20180820144357.7206-1-u.kleine-koe...@pengutronix.de, this is missing on
lore.kernel.org however and I didn't find it on another mirror.)
Thierry's arguments were:
- "An API whose result is an undefined state is useless in my opinion."
(from Message-Id: 20181009073407.GA5565@ulmo)
Yes, this is a drawback for some consumers, but it matches reality
that disabling the PWM counter on some PWM implementations doesn't
result in an inactive level. And if they care about the output, they
Re: [PATCH v2 2/2] drm/bridge: ti-sn65dsi86: Implement the pwm_chip
On Thu 17 Jun 11:54 CDT 2021, Uwe Kleine-K?nig wrote:
> Hello Bjorn,
>
> On Thu, Jun 17, 2021 at 11:38:26AM -0500, Bjorn Andersson wrote:
> > On Thu 17 Jun 01:24 CDT 2021, Uwe Kleine-K?nig wrote:
> > > On Wed, Jun 16, 2021 at 10:22:17PM -0500, Bjorn Andersson wrote:
> > > > > > +static int ti_sn_pwm_apply(struct pwm_chip *chip, struct
> > > > > > pwm_device *pwm,
> > > > > > + const struct pwm_state *state)
> > > > > > +{
> > > > > > + struct ti_sn65dsi86 *pdata = pwm_chip_to_ti_sn_bridge(chip);
> > > > > > + unsigned int pwm_en_inv;
> > > > > > + unsigned int backlight;
> > > > > > + unsigned int pre_div;
> > > > > > + unsigned int scale;
> > > > > > + int ret;
> > > > > > +
> > > > > > + if (!pdata->pwm_enabled) {
> > > > > > + ret = pm_runtime_get_sync(pdata->dev);
> > > > > > + if (ret < 0)
> > > > > > + return ret;
> > > > > > +
> > > > > > + ret = regmap_update_bits(pdata->regmap,
> > > > > > SN_GPIO_CTRL_REG,
> > > > > > + SN_GPIO_MUX_MASK << (2 *
> > > > > > SN_PWM_GPIO_IDX),
> > > > > > + SN_GPIO_MUX_SPECIAL << (2 *
> > > > > > SN_PWM_GPIO_IDX));
> > > > > > + if (ret) {
> > > > > > + dev_err(pdata->dev, "failed to mux in PWM
> > > > > > function\n");
> > > > > > + goto out;
> > > > > > + }
> > > > >
> > > > > Do you need to do this even if state->enabled is false?
> > > >
> > > > I presume I should be able to explicitly mux in the GPIO function and
> > > > configure that to output low. But I am not able to find anything in the
> > > > data sheet that would indicate this to be preferred.
> > >
> > > My question targetted a different case. If the PWM is off
> > > (!pdata->pwm_enabled) and should remain off (state->enabled is false)
> > > you can shortcut here, can you not?
> >
> > Right, if we're going off->off then we don't need to touch the hardware.
> >
> > But am I expected to -EINVAL improper period and duty cycle even though
> > enabled is false?
> >
> > And also, what is the supposed behavior of enabled = false? Is it
> > supposedly equivalent of asking for a duty_cycle of 0?
>
> In my book enabled = false is just syntactic sugar to say:
> "duty_cycle=0, period=something small". So to answer your questions: if
> enabled = false, the consumer doesn't really care about period and
> duty_cycle. Some care that the output becomes inactive, some others
> don't, so from my POV just emit the inactive level on the output and
> ignore period and duty_cycle.
>
Giving this some further thought.
In order to have a known state of the PWM signal we need the sn65dsi86
to be powered. The documentation describes a "suspend mode", but this is
currently not implemented in the driver, so there's a large power cost
coming from just keeping the pin low when disabled.
As such in the current implementation I use state->enabled to also
control if the device should be kept powered, which means that this
follows the backlight power status of the pwm-backlight. Which is
sufficient as the backlight won't be powered when !state->enabled.
For the typical use case the pwm-backlight has some independent control
over gpio and power to toggle the actual backlight. But in the even that
this wouldn't be available I think we need to extend the driver to
implement the suspend mode.
In which case muxing in the PWM function should probably happen at the
time the PWM channel is requested.
This does come at an additional power cost (not as high as keeping the
chip awake though), so (in addition to the effort) I think it's
reasonable to document this as a limitation for now and implement this
as the need arise.
Thanks,
Bjorn
> > > > > Does this already modify the output pin?
> > > >
> > > > Yes, coming out of reset this pin is configured as input, so switching
> > > > the mux here will effectively start driving the pin.
> > >
> > > So please document this in the format the recently added drivers do,
> > > too. See e.g. drivers/pwm/pwm-sifive.c. (The idea is to start that with
> > > " * Limitations:" to make it easy to grep it.)
> > >
> >
> > Okay, will do. Although I believe that for this driver it makes sense to
> > place such comment close to this function, rather than at the top of the
> > driver.
>
> Yes, agreed.
>
> Best regards
> Uwe
>
> --
> Pengutronix e.K. | Uwe Kleine-König|
> Industrial Linux Solutions | https://www.pengutronix.de/ |
Re: [PATCH v2 2/2] drm/bridge: ti-sn65dsi86: Implement the pwm_chip
Hello Bjorn,
On Thu, Jun 17, 2021 at 11:38:26AM -0500, Bjorn Andersson wrote:
> On Thu 17 Jun 01:24 CDT 2021, Uwe Kleine-K?nig wrote:
> > On Wed, Jun 16, 2021 at 10:22:17PM -0500, Bjorn Andersson wrote:
> > > > > +static int ti_sn_pwm_apply(struct pwm_chip *chip, struct pwm_device
> > > > > *pwm,
> > > > > +const struct pwm_state *state)
> > > > > +{
> > > > > + struct ti_sn65dsi86 *pdata = pwm_chip_to_ti_sn_bridge(chip);
> > > > > + unsigned int pwm_en_inv;
> > > > > + unsigned int backlight;
> > > > > + unsigned int pre_div;
> > > > > + unsigned int scale;
> > > > > + int ret;
> > > > > +
> > > > > + if (!pdata->pwm_enabled) {
> > > > > + ret = pm_runtime_get_sync(pdata->dev);
> > > > > + if (ret < 0)
> > > > > + return ret;
> > > > > +
> > > > > + ret = regmap_update_bits(pdata->regmap,
> > > > > SN_GPIO_CTRL_REG,
> > > > > + SN_GPIO_MUX_MASK << (2 *
> > > > > SN_PWM_GPIO_IDX),
> > > > > + SN_GPIO_MUX_SPECIAL << (2 *
> > > > > SN_PWM_GPIO_IDX));
> > > > > + if (ret) {
> > > > > + dev_err(pdata->dev, "failed to mux in PWM
> > > > > function\n");
> > > > > + goto out;
> > > > > + }
> > > >
> > > > Do you need to do this even if state->enabled is false?
> > >
> > > I presume I should be able to explicitly mux in the GPIO function and
> > > configure that to output low. But I am not able to find anything in the
> > > data sheet that would indicate this to be preferred.
> >
> > My question targetted a different case. If the PWM is off
> > (!pdata->pwm_enabled) and should remain off (state->enabled is false)
> > you can shortcut here, can you not?
>
> Right, if we're going off->off then we don't need to touch the hardware.
>
> But am I expected to -EINVAL improper period and duty cycle even though
> enabled is false?
>
> And also, what is the supposed behavior of enabled = false? Is it
> supposedly equivalent of asking for a duty_cycle of 0?
In my book enabled = false is just syntactic sugar to say:
"duty_cycle=0, period=something small". So to answer your questions: if
enabled = false, the consumer doesn't really care about period and
duty_cycle. Some care that the output becomes inactive, some others
don't, so from my POV just emit the inactive level on the output and
ignore period and duty_cycle.
> > > > Does this already modify the output pin?
> > >
> > > Yes, coming out of reset this pin is configured as input, so switching
> > > the mux here will effectively start driving the pin.
> >
> > So please document this in the format the recently added drivers do,
> > too. See e.g. drivers/pwm/pwm-sifive.c. (The idea is to start that with
> > " * Limitations:" to make it easy to grep it.)
> >
>
> Okay, will do. Although I believe that for this driver it makes sense to
> place such comment close to this function, rather than at the top of the
> driver.
Yes, agreed.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König|
Industrial Linux Solutions | https://www.pengutronix.de/ |
signature.asc
Description: PGP signature
Re: [PATCH v2 2/2] drm/bridge: ti-sn65dsi86: Implement the pwm_chip
On Thu 17 Jun 01:24 CDT 2021, Uwe Kleine-K?nig wrote:
> Hello Bjorn,
>
> On Wed, Jun 16, 2021 at 10:22:17PM -0500, Bjorn Andersson wrote:
> > > > +static int ti_sn_pwm_apply(struct pwm_chip *chip, struct pwm_device
> > > > *pwm,
> > > > + const struct pwm_state *state)
> > > > +{
> > > > + struct ti_sn65dsi86 *pdata = pwm_chip_to_ti_sn_bridge(chip);
> > > > + unsigned int pwm_en_inv;
> > > > + unsigned int backlight;
> > > > + unsigned int pre_div;
> > > > + unsigned int scale;
> > > > + int ret;
> > > > +
> > > > + if (!pdata->pwm_enabled) {
> > > > + ret = pm_runtime_get_sync(pdata->dev);
> > > > + if (ret < 0)
> > > > + return ret;
> > > > +
> > > > + ret = regmap_update_bits(pdata->regmap,
> > > > SN_GPIO_CTRL_REG,
> > > > + SN_GPIO_MUX_MASK << (2 *
> > > > SN_PWM_GPIO_IDX),
> > > > + SN_GPIO_MUX_SPECIAL << (2 *
> > > > SN_PWM_GPIO_IDX));
> > > > + if (ret) {
> > > > + dev_err(pdata->dev, "failed to mux in PWM
> > > > function\n");
> > > > + goto out;
> > > > + }
> > >
> > > Do you need to do this even if state->enabled is false?
> >
> > I presume I should be able to explicitly mux in the GPIO function and
> > configure that to output low. But I am not able to find anything in the
> > data sheet that would indicate this to be preferred.
>
> My question targetted a different case. If the PWM is off
> (!pdata->pwm_enabled) and should remain off (state->enabled is false)
> you can shortcut here, can you not?
>
Right, if we're going off->off then we don't need to touch the hardware.
But am I expected to -EINVAL improper period and duty cycle even though
enabled is false?
And also, what is the supposed behavior of enabled = false? Is it
supposedly equivalent of asking for a duty_cycle of 0?
> > > Does this already modify the output pin?
> >
> > Yes, coming out of reset this pin is configured as input, so switching
> > the mux here will effectively start driving the pin.
>
> So please document this in the format the recently added drivers do,
> too. See e.g. drivers/pwm/pwm-sifive.c. (The idea is to start that with
> " * Limitations:" to make it easy to grep it.)
>
Okay, will do. Although I believe that for this driver it makes sense to
place such comment close to this function, rather than at the top of the
driver.
> > > Lets continue the above example with the fixed calculation. So we have:
> > >
> > > pdata->pwm_refclk_freq = 334
> > > state->period = 10 [ns]
> > > state->duty_cycle = 600
> > > scale = 332
> > >
> > > so the actually emitted period = 99899.98002000399 ns
> > >
> > > Now you calculate:
> > >
> > > backlight = 1
> > >
> > > which yields an actual duty_cycle of 299.4 ns, with backlight = 2
> > > you would get an actual duty_cycle of 599.99988 ns, which is better. The
> > > culprit here is that you divide by state->period but instead should
> > > divide by the actual period.
> >
> > What do I do about the case where the actual period is lower than the
> > requested one and thereby the duty cycle becomes larger than the period?
>
> The general principle is: Pick the biggest possible duty_cycle available
> for the just picked period. So in your example you have to clamp it to
> period (assuming you can, otherwise pick the next lower possible value).
>
Sounds good.
Thank you,
Bjorn
> Best regards
> Uwe
>
> --
> Pengutronix e.K. | Uwe Kleine-König|
> Industrial Linux Solutions | https://www.pengutronix.de/ |
Re: [PATCH v2 2/2] drm/bridge: ti-sn65dsi86: Implement the pwm_chip
Hello Bjorn,
On Wed, Jun 16, 2021 at 10:22:17PM -0500, Bjorn Andersson wrote:
> > > +static int ti_sn_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> > > +const struct pwm_state *state)
> > > +{
> > > + struct ti_sn65dsi86 *pdata = pwm_chip_to_ti_sn_bridge(chip);
> > > + unsigned int pwm_en_inv;
> > > + unsigned int backlight;
> > > + unsigned int pre_div;
> > > + unsigned int scale;
> > > + int ret;
> > > +
> > > + if (!pdata->pwm_enabled) {
> > > + ret = pm_runtime_get_sync(pdata->dev);
> > > + if (ret < 0)
> > > + return ret;
> > > +
> > > + ret = regmap_update_bits(pdata->regmap, SN_GPIO_CTRL_REG,
> > > + SN_GPIO_MUX_MASK << (2 * SN_PWM_GPIO_IDX),
> > > + SN_GPIO_MUX_SPECIAL << (2 * SN_PWM_GPIO_IDX));
> > > + if (ret) {
> > > + dev_err(pdata->dev, "failed to mux in PWM function\n");
> > > + goto out;
> > > + }
> >
> > Do you need to do this even if state->enabled is false?
>
> I presume I should be able to explicitly mux in the GPIO function and
> configure that to output low. But I am not able to find anything in the
> data sheet that would indicate this to be preferred.
My question targetted a different case. If the PWM is off
(!pdata->pwm_enabled) and should remain off (state->enabled is false)
you can shortcut here, can you not?
> > Does this already modify the output pin?
>
> Yes, coming out of reset this pin is configured as input, so switching
> the mux here will effectively start driving the pin.
So please document this in the format the recently added drivers do,
too. See e.g. drivers/pwm/pwm-sifive.c. (The idea is to start that with
" * Limitations:" to make it easy to grep it.)
> > Lets continue the above example with the fixed calculation. So we have:
> >
> > pdata->pwm_refclk_freq = 334
> > state->period = 10 [ns]
> > state->duty_cycle = 600
> > scale = 332
> >
> > so the actually emitted period = 99899.98002000399 ns
> >
> > Now you calculate:
> >
> > backlight = 1
> >
> > which yields an actual duty_cycle of 299.4 ns, with backlight = 2
> > you would get an actual duty_cycle of 599.99988 ns, which is better. The
> > culprit here is that you divide by state->period but instead should
> > divide by the actual period.
>
> What do I do about the case where the actual period is lower than the
> requested one and thereby the duty cycle becomes larger than the period?
The general principle is: Pick the biggest possible duty_cycle available
for the just picked period. So in your example you have to clamp it to
period (assuming you can, otherwise pick the next lower possible value).
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König|
Industrial Linux Solutions | https://www.pengutronix.de/ |
signature.asc
Description: PGP signature
Re: [PATCH v2 2/2] drm/bridge: ti-sn65dsi86: Implement the pwm_chip
On Wed 16 Jun 02:56 CDT 2021, Uwe Kleine-K?nig wrote:
> Hello Bjorn,
>
> On Tue, Jun 15, 2021 at 06:18:28PM -0500, Bjorn Andersson wrote:
> > The SN65DSI86 provides the ability to supply a PWM signal on GPIO 4,
> > with the primary purpose of controlling the backlight of the attached
> > panel. Add an implementation that exposes this using the standard PWM
> > framework, to allow e.g. pwm-backlight to expose this to the user.
> >
> > Signed-off-by: Bjorn Andersson
> > ---
> >
> > Changes since v1:
> > - Rebased ontop of Doug's auxiliary_bus patches
> > - Reworked the math, per Uwe's request
> > - Added pwm_chip->get_state and made sure it's happy (only tested with a few
> > limited periods, such as 1kHz)
> >
> > drivers/gpu/drm/bridge/ti-sn65dsi86.c | 298 +-
> > 1 file changed, 297 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> > b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> > index 5d712c8c3c3b..8f11c9b2da48 100644
> > --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> > +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> > @@ -4,6 +4,7 @@
> > * datasheet: https://www.ti.com/lit/ds/symlink/sn65dsi86.pdf
> > */
> >
> > +#include
> > #include
> > #include
> > #include
> > @@ -15,6 +16,7 @@
> > #include
> > #include
> > #include
> > +#include
> > #include
> > #include
> >
> > @@ -91,6 +93,13 @@
> > #define SN_ML_TX_MODE_REG 0x96
> > #define ML_TX_MAIN_LINK_OFF 0
> > #define ML_TX_NORMAL_MODE BIT(0)
> > +#define SN_PWM_PRE_DIV_REG 0xA0
> > +#define SN_BACKLIGHT_SCALE_REG 0xA1
> > +#define BACKLIGHT_SCALE_MAX 0x
> > +#define SN_BACKLIGHT_REG 0xA3
> > +#define SN_PWM_EN_INV_REG 0xA5
> > +#define SN_PWM_INV_MASK BIT(0)
> > +#define SN_PWM_EN_MASKBIT(1)
> > #define SN_AUX_CMD_STATUS_REG 0xF4
> > #define AUX_IRQ_STATUS_AUX_RPLY_TOUT BIT(3)
> > #define AUX_IRQ_STATUS_AUX_SHORT BIT(5)
> > @@ -113,11 +122,14 @@
> >
> > #define SN_LINK_TRAINING_TRIES 10
> >
> > +#define SN_PWM_GPIO_IDX3 /* 4th GPIO */
> > +
> > /**
> > * struct ti_sn65dsi86 - Platform data for ti-sn65dsi86 driver.
> > * @bridge_aux: AUX-bus sub device for MIPI-to-eDP bridge functionality.
> > * @gpio_aux: AUX-bus sub device for GPIO controller functionality.
> > * @aux_aux: AUX-bus sub device for eDP AUX channel functionality.
> > + * @pwm_aux: AUX-bus sub device for PWM controller functionality.
> > *
> > * @dev: Pointer to the top level (i2c) device.
> > * @regmap: Regmap for accessing i2c.
> > @@ -145,11 +157,17 @@
> > *bitmap so we can do atomic ops on it without an extra
> > *lock so concurrent users of our 4 GPIOs don't stomp on
> > *each other's read-modify-write.
> > + *
> > + * @pchip:pwm_chip if the PWM is exposed.
> > + * @pwm_enabled: Used to track if the PWM signal is currently enabled.
> > + * @pwm_refclk_freq: Cache for the reference clock input to the PWM.
> > + * @pwm_pin_busy: Track if GPIO4 is currently requested for GPIO or PWM.
> > */
> > struct ti_sn65dsi86 {
> > struct auxiliary_device bridge_aux;
> > struct auxiliary_device gpio_aux;
> > struct auxiliary_device aux_aux;
> > + struct auxiliary_device pwm_aux;
> >
> > struct device *dev;
> > struct regmap *regmap;
> > @@ -172,6 +190,12 @@ struct ti_sn65dsi86 {
> > struct gpio_chipgchip;
> > DECLARE_BITMAP(gchip_output, SN_NUM_GPIOS);
> > #endif
> > +#if defined(CONFIG_PWM)
> > + struct pwm_chip pchip;
> > + boolpwm_enabled;
> > + unsigned intpwm_refclk_freq;
> > + atomic_tpwm_pin_busy;
> > +#endif
> > };
> >
> > static const struct regmap_range ti_sn65dsi86_volatile_ranges[] = {
> > @@ -190,6 +214,25 @@ static const struct regmap_config
> > ti_sn65dsi86_regmap_config = {
> > .cache_type = REGCACHE_NONE,
> > };
> >
> > +static int ti_sn65dsi86_read_u16(struct ti_sn65dsi86 *pdata,
> > +unsigned int reg, u16 *val)
> > +{
> > + unsigned int tmp;
> > + int ret;
> > +
> > + ret = regmap_read(pdata->regmap, reg, );
> > + if (ret)
> > + return ret;
> > + *val = tmp;
> > +
> > + ret = regmap_read(pdata->regmap, reg + 1, );
> > + if (ret)
> > + return ret;
> > + *val |= tmp << 8;
> > +
> > + return 0;
> > +}
> > +
> > static void ti_sn65dsi86_write_u16(struct ti_sn65dsi86 *pdata,
> >unsigned int reg, u16 val)
> > {
> > @@ -253,6 +296,14 @@ static void ti_sn_bridge_set_refclk_freq(struct
Re: [PATCH v2 2/2] drm/bridge: ti-sn65dsi86: Implement the pwm_chip
Hello Bjorn,
On Tue, Jun 15, 2021 at 06:18:28PM -0500, Bjorn Andersson wrote:
> The SN65DSI86 provides the ability to supply a PWM signal on GPIO 4,
> with the primary purpose of controlling the backlight of the attached
> panel. Add an implementation that exposes this using the standard PWM
> framework, to allow e.g. pwm-backlight to expose this to the user.
>
> Signed-off-by: Bjorn Andersson
> ---
>
> Changes since v1:
> - Rebased ontop of Doug's auxiliary_bus patches
> - Reworked the math, per Uwe's request
> - Added pwm_chip->get_state and made sure it's happy (only tested with a few
> limited periods, such as 1kHz)
>
> drivers/gpu/drm/bridge/ti-sn65dsi86.c | 298 +-
> 1 file changed, 297 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> index 5d712c8c3c3b..8f11c9b2da48 100644
> --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> @@ -4,6 +4,7 @@
> * datasheet: https://www.ti.com/lit/ds/symlink/sn65dsi86.pdf
> */
>
> +#include
> #include
> #include
> #include
> @@ -15,6 +16,7 @@
> #include
> #include
> #include
> +#include
> #include
> #include
>
> @@ -91,6 +93,13 @@
> #define SN_ML_TX_MODE_REG0x96
> #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 0x
> +#define SN_BACKLIGHT_REG 0xA3
> +#define SN_PWM_EN_INV_REG0xA5
> +#define SN_PWM_INV_MASK BIT(0)
> +#define SN_PWM_EN_MASK BIT(1)
> #define SN_AUX_CMD_STATUS_REG0xF4
> #define AUX_IRQ_STATUS_AUX_RPLY_TOUTBIT(3)
> #define AUX_IRQ_STATUS_AUX_SHORTBIT(5)
> @@ -113,11 +122,14 @@
>
> #define SN_LINK_TRAINING_TRIES 10
>
> +#define SN_PWM_GPIO_IDX 3 /* 4th GPIO */
> +
> /**
> * struct ti_sn65dsi86 - Platform data for ti-sn65dsi86 driver.
> * @bridge_aux: AUX-bus sub device for MIPI-to-eDP bridge functionality.
> * @gpio_aux: AUX-bus sub device for GPIO controller functionality.
> * @aux_aux: AUX-bus sub device for eDP AUX channel functionality.
> + * @pwm_aux: AUX-bus sub device for PWM controller functionality.
> *
> * @dev: Pointer to the top level (i2c) device.
> * @regmap: Regmap for accessing i2c.
> @@ -145,11 +157,17 @@
> *bitmap so we can do atomic ops on it without an extra
> *lock so concurrent users of our 4 GPIOs don't stomp on
> *each other's read-modify-write.
> + *
> + * @pchip:pwm_chip if the PWM is exposed.
> + * @pwm_enabled: Used to track if the PWM signal is currently enabled.
> + * @pwm_refclk_freq: Cache for the reference clock input to the PWM.
> + * @pwm_pin_busy: Track if GPIO4 is currently requested for GPIO or PWM.
> */
> struct ti_sn65dsi86 {
> struct auxiliary_device bridge_aux;
> struct auxiliary_device gpio_aux;
> struct auxiliary_device aux_aux;
> + struct auxiliary_device pwm_aux;
>
> struct device *dev;
> struct regmap *regmap;
> @@ -172,6 +190,12 @@ struct ti_sn65dsi86 {
> struct gpio_chipgchip;
> DECLARE_BITMAP(gchip_output, SN_NUM_GPIOS);
> #endif
> +#if defined(CONFIG_PWM)
> + struct pwm_chip pchip;
> + boolpwm_enabled;
> + unsigned intpwm_refclk_freq;
> + atomic_tpwm_pin_busy;
> +#endif
> };
>
> static const struct regmap_range ti_sn65dsi86_volatile_ranges[] = {
> @@ -190,6 +214,25 @@ static const struct regmap_config
> ti_sn65dsi86_regmap_config = {
> .cache_type = REGCACHE_NONE,
> };
>
> +static int ti_sn65dsi86_read_u16(struct ti_sn65dsi86 *pdata,
> + unsigned int reg, u16 *val)
> +{
> + unsigned int tmp;
> + int ret;
> +
> + ret = regmap_read(pdata->regmap, reg, );
> + if (ret)
> + return ret;
> + *val = tmp;
> +
> + ret = regmap_read(pdata->regmap, reg + 1, );
> + if (ret)
> + return ret;
> + *val |= tmp << 8;
> +
> + return 0;
> +}
> +
> static void ti_sn65dsi86_write_u16(struct ti_sn65dsi86 *pdata,
> unsigned int reg, u16 val)
> {
> @@ -253,6 +296,14 @@ static void ti_sn_bridge_set_refclk_freq(struct
> ti_sn65dsi86 *pdata)
>
> regmap_update_bits(pdata->regmap, SN_DPPLL_SRC_REG, REFCLK_FREQ_MASK,
> REFCLK_FREQ(i));
> +
> +#if defined(CONFIG_PWM)
> + /*
> + * The PWM refclk is based on the value
[PATCH v2 2/2] drm/bridge: ti-sn65dsi86: Implement the pwm_chip
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
---
Changes since v1:
- Rebased ontop of Doug's auxiliary_bus patches
- Reworked the math, per Uwe's request
- Added pwm_chip->get_state and made sure it's happy (only tested with a few
limited periods, such as 1kHz)
drivers/gpu/drm/bridge/ti-sn65dsi86.c | 298 +-
1 file changed, 297 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
index 5d712c8c3c3b..8f11c9b2da48 100644
--- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
+++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
@@ -4,6 +4,7 @@
* datasheet: https://www.ti.com/lit/ds/symlink/sn65dsi86.pdf
*/
+#include
#include
#include
#include
@@ -15,6 +16,7 @@
#include
#include
#include
+#include
#include
#include
@@ -91,6 +93,13 @@
#define SN_ML_TX_MODE_REG 0x96
#define ML_TX_MAIN_LINK_OFF 0
#define ML_TX_NORMAL_MODE BIT(0)
+#define SN_PWM_PRE_DIV_REG 0xA0
+#define SN_BACKLIGHT_SCALE_REG 0xA1
+#define BACKLIGHT_SCALE_MAX 0x
+#define SN_BACKLIGHT_REG 0xA3
+#define SN_PWM_EN_INV_REG 0xA5
+#define SN_PWM_INV_MASK BIT(0)
+#define SN_PWM_EN_MASKBIT(1)
#define SN_AUX_CMD_STATUS_REG 0xF4
#define AUX_IRQ_STATUS_AUX_RPLY_TOUT BIT(3)
#define AUX_IRQ_STATUS_AUX_SHORT BIT(5)
@@ -113,11 +122,14 @@
#define SN_LINK_TRAINING_TRIES 10
+#define SN_PWM_GPIO_IDX3 /* 4th GPIO */
+
/**
* struct ti_sn65dsi86 - Platform data for ti-sn65dsi86 driver.
* @bridge_aux: AUX-bus sub device for MIPI-to-eDP bridge functionality.
* @gpio_aux: AUX-bus sub device for GPIO controller functionality.
* @aux_aux: AUX-bus sub device for eDP AUX channel functionality.
+ * @pwm_aux: AUX-bus sub device for PWM controller functionality.
*
* @dev: Pointer to the top level (i2c) device.
* @regmap: Regmap for accessing i2c.
@@ -145,11 +157,17 @@
*bitmap so we can do atomic ops on it without an extra
*lock so concurrent users of our 4 GPIOs don't stomp on
*each other's read-modify-write.
+ *
+ * @pchip:pwm_chip if the PWM is exposed.
+ * @pwm_enabled: Used to track if the PWM signal is currently enabled.
+ * @pwm_refclk_freq: Cache for the reference clock input to the PWM.
+ * @pwm_pin_busy: Track if GPIO4 is currently requested for GPIO or PWM.
*/
struct ti_sn65dsi86 {
struct auxiliary_device bridge_aux;
struct auxiliary_device gpio_aux;
struct auxiliary_device aux_aux;
+ struct auxiliary_device pwm_aux;
struct device *dev;
struct regmap *regmap;
@@ -172,6 +190,12 @@ struct ti_sn65dsi86 {
struct gpio_chipgchip;
DECLARE_BITMAP(gchip_output, SN_NUM_GPIOS);
#endif
+#if defined(CONFIG_PWM)
+ struct pwm_chip pchip;
+ boolpwm_enabled;
+ unsigned intpwm_refclk_freq;
+ atomic_tpwm_pin_busy;
+#endif
};
static const struct regmap_range ti_sn65dsi86_volatile_ranges[] = {
@@ -190,6 +214,25 @@ static const struct regmap_config
ti_sn65dsi86_regmap_config = {
.cache_type = REGCACHE_NONE,
};
+static int ti_sn65dsi86_read_u16(struct ti_sn65dsi86 *pdata,
+unsigned int reg, u16 *val)
+{
+ unsigned int tmp;
+ int ret;
+
+ ret = regmap_read(pdata->regmap, reg, );
+ if (ret)
+ return ret;
+ *val = tmp;
+
+ ret = regmap_read(pdata->regmap, reg + 1, );
+ if (ret)
+ return ret;
+ *val |= tmp << 8;
+
+ return 0;
+}
+
static void ti_sn65dsi86_write_u16(struct ti_sn65dsi86 *pdata,
unsigned int reg, u16 val)
{
@@ -253,6 +296,14 @@ static void ti_sn_bridge_set_refclk_freq(struct
ti_sn65dsi86 *pdata)
regmap_update_bits(pdata->regmap, SN_DPPLL_SRC_REG, REFCLK_FREQ_MASK,
REFCLK_FREQ(i));
+
+#if defined(CONFIG_PWM)
+ /*
+* The PWM refclk is based on the value written to SN_DPPLL_SRC_REG,
+* regardless of its actual sourcing.
+*/
+ pdata->pwm_refclk_freq = ti_sn_bridge_refclk_lut[i];
+#endif
}
static void ti_sn65dsi86_enable_comms(struct ti_sn65dsi86 *pdata)
@@ -1044,6 +1095,221 @@ static int
