* [PATCH 0/2] drm/bridge: ti-sn65dsi86: Support backlight controls @ 2020-09-30 22:35 Bjorn Andersson 2020-09-30 22:35 ` [PATCH 1/2] dt-bindings: drm/bridge: ti-sn65dsi86: Replace #pwm-cells Bjorn Andersson 2020-09-30 22:35 ` [PATCH 2/2] drm/bridge: ti-sn65dsi86: Expose backlight controls Bjorn Andersson 0 siblings, 2 replies; 10+ messages in thread From: Bjorn Andersson @ 2020-09-30 22:35 UTC (permalink / raw) To: David Airlie, Daniel Vetter, Rob Herring, Andrzej Hajda, Neil Armstrong, Laurent Pinchart, Jonas Karlman, Jernej Skrabec, Douglas Anderson Cc: dri-devel, devicetree, linux-kernel, linux-arm-msm The TI SN65DSI86 support driving a backlight driver using a signal exposed on GPIO4, implement this as a backlight device. Bjorn Andersson (2): dt-bindings: drm/bridge: ti-sn65dsi86: Replace #pwm-cells drm/bridge: ti-sn65dsi86: Expose backlight controls .../bindings/display/bridge/ti,sn65dsi86.yaml | 9 +- drivers/gpu/drm/bridge/Kconfig | 1 + drivers/gpu/drm/bridge/ti-sn65dsi86.c | 143 +++++++++++++++++- 3 files changed, 146 insertions(+), 7 deletions(-) -- 2.28.0 ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 1/2] dt-bindings: drm/bridge: ti-sn65dsi86: Replace #pwm-cells 2020-09-30 22:35 [PATCH 0/2] drm/bridge: ti-sn65dsi86: Support backlight controls Bjorn Andersson @ 2020-09-30 22:35 ` Bjorn Andersson 2020-09-30 23:06 ` Steev Klimaszewski ` (2 more replies) 2020-09-30 22:35 ` [PATCH 2/2] drm/bridge: ti-sn65dsi86: Expose backlight controls Bjorn Andersson 1 sibling, 3 replies; 10+ messages in thread From: Bjorn Andersson @ 2020-09-30 22:35 UTC (permalink / raw) To: David Airlie, Daniel Vetter, Rob Herring, Andrzej Hajda, Neil Armstrong, Laurent Pinchart, Jonas Karlman, Jernej Skrabec, Douglas Anderson Cc: dri-devel, devicetree, linux-kernel, linux-arm-msm While the signal on GPIO4 to drive the backlight controller indeed is pulse width modulated its purpose is specifically to control the brightness of a backlight. Drop the #pwm-cells and instead expose a new property to configure the granularity of the backlight PWM signal. Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org> --- .../devicetree/bindings/display/bridge/ti,sn65dsi86.yaml | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi86.yaml b/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi86.yaml index f8622bd0f61e..e380218b4646 100644 --- a/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi86.yaml +++ b/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi86.yaml @@ -66,9 +66,12 @@ properties: 1-based to match the datasheet. See ../../gpio/gpio.txt for more information. - '#pwm-cells': - const: 1 - description: See ../../pwm/pwm.yaml for description of the cell formats. + ti,backlight-scale: + description: + The granularity of brightness for the PWM signal provided on GPIO4, if + this property is specified. + minimum: 0 + maximum: 65535 ports: type: object -- 2.28.0 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] dt-bindings: drm/bridge: ti-sn65dsi86: Replace #pwm-cells 2020-09-30 22:35 ` [PATCH 1/2] dt-bindings: drm/bridge: ti-sn65dsi86: Replace #pwm-cells Bjorn Andersson @ 2020-09-30 23:06 ` Steev Klimaszewski 2020-10-02 20:42 ` Doug Anderson 2020-10-05 13:50 ` Rob Herring 2 siblings, 0 replies; 10+ messages in thread From: Steev Klimaszewski @ 2020-09-30 23:06 UTC (permalink / raw) To: Bjorn Andersson, David Airlie, Daniel Vetter, Rob Herring, Andrzej Hajda, Neil Armstrong, Laurent Pinchart, Jonas Karlman, Jernej Skrabec, Douglas Anderson Cc: dri-devel, devicetree, linux-kernel, linux-arm-msm On 9/30/20 5:35 PM, Bjorn Andersson wrote: > While the signal on GPIO4 to drive the backlight controller indeed is > pulse width modulated its purpose is specifically to control the > brightness of a backlight. > > Drop the #pwm-cells and instead expose a new property to configure the > granularity of the backlight PWM signal. > > Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org> > --- > .../devicetree/bindings/display/bridge/ti,sn65dsi86.yaml | 9 ++++++--- > 1 file changed, 6 insertions(+), 3 deletions(-) > > diff --git a/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi86.yaml b/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi86.yaml > index f8622bd0f61e..e380218b4646 100644 > --- a/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi86.yaml > +++ b/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi86.yaml > @@ -66,9 +66,12 @@ properties: > 1-based to match the datasheet. See ../../gpio/gpio.txt for more > information. > > - '#pwm-cells': > - const: 1 > - description: See ../../pwm/pwm.yaml for description of the cell formats. > + ti,backlight-scale: > + description: > + The granularity of brightness for the PWM signal provided on GPIO4, if > + this property is specified. > + minimum: 0 > + maximum: 65535 > > ports: > type: object Tested-By: Steev Klimaszewski <steev@kali.org> ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] dt-bindings: drm/bridge: ti-sn65dsi86: Replace #pwm-cells 2020-09-30 22:35 ` [PATCH 1/2] dt-bindings: drm/bridge: ti-sn65dsi86: Replace #pwm-cells Bjorn Andersson 2020-09-30 23:06 ` Steev Klimaszewski @ 2020-10-02 20:42 ` Doug Anderson 2020-11-02 17:08 ` Bjorn Andersson 2020-10-05 13:50 ` Rob Herring 2 siblings, 1 reply; 10+ messages in thread From: Doug Anderson @ 2020-10-02 20:42 UTC (permalink / raw) To: Bjorn Andersson Cc: David Airlie, Daniel Vetter, Rob Herring, Andrzej Hajda, Neil Armstrong, Laurent Pinchart, Jonas Karlman, Jernej Skrabec, dri-devel, open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, LKML, linux-arm-msm Hi, On Wed, Sep 30, 2020 at 3:40 PM Bjorn Andersson <bjorn.andersson@linaro.org> wrote: > > While the signal on GPIO4 to drive the backlight controller indeed is > pulse width modulated its purpose is specifically to control the > brightness of a backlight. I'm a bit on the fence about this. I guess you're doing this because it avoids some -EPROBE_DEFER cycles in Linux? It does seem to have a few downsides, though. 1. It means a bit of re-inventing the wheel. It's not a very big wheel, though, I'll give you. ...but it's still something. 2. I'm not sure why you'd want to, but in theory one could use this PWM for some other purposes. It really is just a generic PWM. Your change prevents that. > Drop the #pwm-cells and instead expose a new property to configure the > granularity of the backlight PWM signal. > > Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org> > --- > .../devicetree/bindings/display/bridge/ti,sn65dsi86.yaml | 9 ++++++--- > 1 file changed, 6 insertions(+), 3 deletions(-) > > diff --git a/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi86.yaml b/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi86.yaml > index f8622bd0f61e..e380218b4646 100644 > --- a/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi86.yaml > +++ b/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi86.yaml > @@ -66,9 +66,12 @@ properties: > 1-based to match the datasheet. See ../../gpio/gpio.txt for more > information. > > - '#pwm-cells': > - const: 1 > - description: See ../../pwm/pwm.yaml for description of the cell formats. > + ti,backlight-scale: > + description: > + The granularity of brightness for the PWM signal provided on GPIO4, if > + this property is specified. > + minimum: 0 > + maximum: 65535 A few issues here: 1. Maybe call this "num-steps" instead of backlight-scale. That's essentially what it is, right? Saying how many discrete steps you're allowing in your backlight? 2. IMO you need the PWM frequency specified, since it can actually matter. NOTE: once you have the PWM frequency specified, you could imagine automatically figuring out what "num-steps" was. Really you'd want it to be the largest possible value you could achieve with your hardware at the specified frequency. There's no advantage (is there?) of providing fewer steps to the backlight client. 3. Some backlights are specified inverted. It looks like this maps nicely to the bridge chip, which has a bit for it. Probably nice to expose this? Of course, if we were just exposing the PWM directly to Linux we could just use the PWM backlight driver and it'd all magically work. ;-) -Doug ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] dt-bindings: drm/bridge: ti-sn65dsi86: Replace #pwm-cells 2020-10-02 20:42 ` Doug Anderson @ 2020-11-02 17:08 ` Bjorn Andersson 2020-11-11 0:48 ` Doug Anderson 0 siblings, 1 reply; 10+ messages in thread From: Bjorn Andersson @ 2020-11-02 17:08 UTC (permalink / raw) To: Doug Anderson Cc: David Airlie, Daniel Vetter, Rob Herring, Andrzej Hajda, Neil Armstrong, Laurent Pinchart, Jonas Karlman, Jernej Skrabec, dri-devel, open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, LKML, linux-arm-msm On Fri 02 Oct 15:42 CDT 2020, Doug Anderson wrote: > Hi, > > On Wed, Sep 30, 2020 at 3:40 PM Bjorn Andersson > <bjorn.andersson@linaro.org> wrote: > > > > While the signal on GPIO4 to drive the backlight controller indeed is > > pulse width modulated its purpose is specifically to control the > > brightness of a backlight. > > I'm a bit on the fence about this. I guess you're doing this because > it avoids some -EPROBE_DEFER cycles in Linux? It does seem to have a > few downsides, though. > No, the reason for exposing a backlight is that while the thing certainly is a PWM signal, the description of it and the registers available to control it surely seems "backlight" to me. In particular No, the reason for exposing a backlight is that while while the thing certainly is a PWM signal, the description of it and the registers available to control it surely seems "backlight" to me. > 1. It means a bit of re-inventing the wheel. It's not a very big > wheel, though, I'll give you. ...but it's still something. > The main problem I saw with exposing this as a PWM was the fact that we have both period and frequency to control... > 2. I'm not sure why you'd want to, but in theory one could use this > PWM for some other purposes. It really is just a generic PWM. Your > change prevents that. > ...and in the even that you use it as a "generic" PWM I'd expect that the specified period is related to the frequency of the signal. But the period is documented to be related to the number of brightness steps of the panel. > > > > Drop the #pwm-cells and instead expose a new property to configure the > > granularity of the backlight PWM signal. > > > > Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org> > > --- > > .../devicetree/bindings/display/bridge/ti,sn65dsi86.yaml | 9 ++++++--- > > 1 file changed, 6 insertions(+), 3 deletions(-) > > > > diff --git a/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi86.yaml b/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi86.yaml > > index f8622bd0f61e..e380218b4646 100644 > > --- a/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi86.yaml > > +++ b/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi86.yaml > > @@ -66,9 +66,12 @@ properties: > > 1-based to match the datasheet. See ../../gpio/gpio.txt for more > > information. > > > > - '#pwm-cells': > > - const: 1 > > - description: See ../../pwm/pwm.yaml for description of the cell formats. > > + ti,backlight-scale: > > + description: > > + The granularity of brightness for the PWM signal provided on GPIO4, if > > + this property is specified. > > + minimum: 0 > > + maximum: 65535 > > A few issues here: > > 1. Maybe call this "num-steps" instead of backlight-scale. That's > essentially what it is, right? Saying how many discrete steps you're > allowing in your backlight? > That would work, I had it as "max-brightness" for a while as well. But I reverted to backlight-scale, because that's the name used in the datasheet. I'm fine with whatever color of the shed though :) > 2. IMO you need the PWM frequency specified, since it can actually > matter. NOTE: once you have the PWM frequency specified, you could > imagine automatically figuring out what "num-steps" was. Really you'd > want it to be the largest possible value you could achieve with your > hardware at the specified frequency. There's no advantage (is there?) > of providing fewer steps to the backlight client. > I guess there's no problem in having a "num-steps" that is unrelated to the number of brightness steps of the panel - but I did distinguish them because the datasheet clearly does so. > 3. Some backlights are specified inverted. It looks like this maps > nicely to the bridge chip, which has a bit for it. Probably nice to > expose this? > Yes, that should be covered. > Of course, if we were just exposing the PWM directly to Linux we could > just use the PWM backlight driver and it'd all magically work. ;-) > Please help me figure out how to properly expose this in the PWM api and I'll be happy to respin it using this - as you say my wheel does look pretty similar... Regards, Bjorn ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] dt-bindings: drm/bridge: ti-sn65dsi86: Replace #pwm-cells 2020-11-02 17:08 ` Bjorn Andersson @ 2020-11-11 0:48 ` Doug Anderson 0 siblings, 0 replies; 10+ messages in thread From: Doug Anderson @ 2020-11-11 0:48 UTC (permalink / raw) To: Bjorn Andersson Cc: David Airlie, Daniel Vetter, Rob Herring, Andrzej Hajda, Neil Armstrong, Laurent Pinchart, Jonas Karlman, Jernej Skrabec, dri-devel, open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, LKML, linux-arm-msm Hi, On Mon, Nov 2, 2020 at 9:08 AM Bjorn Andersson <bjorn.andersson@linaro.org> wrote: > > On Fri 02 Oct 15:42 CDT 2020, Doug Anderson wrote: > > > Hi, > > > > On Wed, Sep 30, 2020 at 3:40 PM Bjorn Andersson > > <bjorn.andersson@linaro.org> wrote: > > > > > > While the signal on GPIO4 to drive the backlight controller indeed is > > > pulse width modulated its purpose is specifically to control the > > > brightness of a backlight. > > > > I'm a bit on the fence about this. I guess you're doing this because > > it avoids some -EPROBE_DEFER cycles in Linux? It does seem to have a > > few downsides, though. > > > > No, the reason for exposing a backlight is that while the thing > certainly is a PWM signal, the description of it and the registers > available to control it surely seems "backlight" to me. > > In particular No, the reason for exposing a backlight is that while > while the thing certainly is a PWM signal, the description of it and the > registers available to control it surely seems "backlight" to me. > > > 1. It means a bit of re-inventing the wheel. It's not a very big > > wheel, though, I'll give you. ...but it's still something. > > > > The main problem I saw with exposing this as a PWM was the fact that we > have both period and frequency to control... > > > 2. I'm not sure why you'd want to, but in theory one could use this > > PWM for some other purposes. It really is just a generic PWM. Your > > change prevents that. > > > > ...and in the even that you use it as a "generic" PWM I'd expect that > the specified period is related to the frequency of the signal. But the > period is documented to be related to the number of brightness steps of > the panel. I think the key here is that the "number of brightness steps of the panel" isn't really a thing that's worried about. At least in my experience, you can pretty much just use as many steps as you can represent based on your PWM hardware. If a panel happens to map some of those steps to the same brightness then it wouldn't be the end of the world, but in experience it's not really such a digital thing. If you choose 4096 steps then you likely get 4096 different brightness levels. If you choose 256 steps then you get 256 different brightness levels. Once you have "more than enough" steps then everything's pretty much fine. Looking at one random panel (just to get an idea of numbers), I see that it specifies: * min PWM Freq: 200 Hz * max PWM Freq: 10,000 Hz. ...and refclk is something between 12 MHz and 38.4 MHz, right? The bridge chip datasheet says: PWM_FREQ = REFCLK_FREQ / (PWM_PRE_DIV * BACKLIGHT_SCALE + 1) So let's see what we can do. I'm arguing that we want the client to be able to specify the PWM frequency and duty cycle and we'll do the job of picking the number of steps. We'll try for the most steps we can get (65535). I guess we need to solve for PWM_PRE_DIV : PWM_FREQ * (PWM_PRE_DIV * BACKLIGHT_SCALE + 1) = REFCLK_FREQ PWM_PRE_DIV * BACKLIGHT_SCALE + 1 = REFCLK_FREQ / PWM_FREQ PWM_PRE_DIV * BACKLIGHT_SCALE = REFCLK_FREQ / PWM_FREQ - 1 PWM_PRE_DIV = (REFCLK_FREQ / PWM_FREQ - 1) / BACKLIGHT_SCALE ...and solve for BACKLIGHT_SCALE: BACKLIGHT_SCALE = (REFCLK_FREQ / PWM_FREQ - 1) / PWM_PRE_DIV With 1000 Hz, 12 MHz refclk: PWM_PRE_DIV = DIV_ROUND_UP(12000000 / 1000 - 1, 65535) => 1 BACKLIGHT_SCALE = (12000000 / 1000 - 1) / 1 => 11999 With 1000 Hz, 38.4 MHz refclk: PWM_PRE_DIV = DIV_ROUND_UP(38400000 / 1000 - 1, 65535) => 1 BACKLIGHT_SCALE = (38400000 / 1000 - 1) / 1 => 38399 With 200 Hz, 38.4 MHz refclk: PWM_PRE_DIV = DIV_ROUND_UP(38400000 / 200 - 1, 65535) => 3 BACKLIGHT_SCALE = (38400000 / 200 - 1) / 3 => 63999 Now that you have BACKLIGHT_SCALE specified, then when someone tries to give you a duty cycle you just map it to the closest value you can make. Obviously you won't be able to perfectly make every exact duty cycle / period that a client requests, but that's true of all PWMs out there. The nice thing here is that (assuming my math is right) we should be getting nearly exactly the frequency that the client requested and that (in my mind) is what matters. You also get as many steps as possible which means that (with the PWM backlight API) you'll be able to get as close as possible to whatever a user requests. > > > Drop the #pwm-cells and instead expose a new property to configure the > > > granularity of the backlight PWM signal. > > > > > > Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org> > > > --- > > > .../devicetree/bindings/display/bridge/ti,sn65dsi86.yaml | 9 ++++++--- > > > 1 file changed, 6 insertions(+), 3 deletions(-) > > > > > > diff --git a/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi86.yaml b/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi86.yaml > > > index f8622bd0f61e..e380218b4646 100644 > > > --- a/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi86.yaml > > > +++ b/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi86.yaml > > > @@ -66,9 +66,12 @@ properties: > > > 1-based to match the datasheet. See ../../gpio/gpio.txt for more > > > information. > > > > > > - '#pwm-cells': > > > - const: 1 > > > - description: See ../../pwm/pwm.yaml for description of the cell formats. > > > + ti,backlight-scale: > > > + description: > > > + The granularity of brightness for the PWM signal provided on GPIO4, if > > > + this property is specified. > > > + minimum: 0 > > > + maximum: 65535 > > > > A few issues here: > > > > 1. Maybe call this "num-steps" instead of backlight-scale. That's > > essentially what it is, right? Saying how many discrete steps you're > > allowing in your backlight? > > > > That would work, I had it as "max-brightness" for a while as well. But I > reverted to backlight-scale, because that's the name used in the > datasheet. > > I'm fine with whatever color of the shed though :) > > > 2. IMO you need the PWM frequency specified, since it can actually > > matter. NOTE: once you have the PWM frequency specified, you could > > imagine automatically figuring out what "num-steps" was. Really you'd > > want it to be the largest possible value you could achieve with your > > hardware at the specified frequency. There's no advantage (is there?) > > of providing fewer steps to the backlight client. > > > > I guess there's no problem in having a "num-steps" that is unrelated to > the number of brightness steps of the panel - but I did distinguish them > because the datasheet clearly does so. I think the datasheet talks about the number of steps that you will be able to make, but that doesn't mean it has to be what's exposed to clients of this driver, right? > > 3. Some backlights are specified inverted. It looks like this maps > > nicely to the bridge chip, which has a bit for it. Probably nice to > > expose this? > > > > Yes, that should be covered. > > > Of course, if we were just exposing the PWM directly to Linux we could > > just use the PWM backlight driver and it'd all magically work. ;-) > > > > Please help me figure out how to properly expose this in the PWM api and > I'll be happy to respin it using this - as you say my wheel does look > pretty similar... Hopefully the above seems sane to you? -Doug ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] dt-bindings: drm/bridge: ti-sn65dsi86: Replace #pwm-cells 2020-09-30 22:35 ` [PATCH 1/2] dt-bindings: drm/bridge: ti-sn65dsi86: Replace #pwm-cells Bjorn Andersson 2020-09-30 23:06 ` Steev Klimaszewski 2020-10-02 20:42 ` Doug Anderson @ 2020-10-05 13:50 ` Rob Herring 2 siblings, 0 replies; 10+ messages in thread From: Rob Herring @ 2020-10-05 13:50 UTC (permalink / raw) To: Bjorn Andersson Cc: Jonas Karlman, Rob Herring, linux-arm-msm, linux-kernel, Neil Armstrong, David Airlie, Laurent Pinchart, Douglas Anderson, Andrzej Hajda, dri-devel, devicetree, Jernej Skrabec, Daniel Vetter On Wed, 30 Sep 2020 17:35:31 -0500, Bjorn Andersson wrote: > While the signal on GPIO4 to drive the backlight controller indeed is > pulse width modulated its purpose is specifically to control the > brightness of a backlight. > > Drop the #pwm-cells and instead expose a new property to configure the > granularity of the backlight PWM signal. > > Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org> > --- > .../devicetree/bindings/display/bridge/ti,sn65dsi86.yaml | 9 ++++++--- > 1 file changed, 6 insertions(+), 3 deletions(-) > My bot found errors running 'make dt_binding_check' on your patch: /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi86.yaml: properties:ti,backlight-scale: {'description': 'The granularity of brightness for the PWM signal provided on GPIO4, if this property is specified.', 'minimum': 0, 'maximum': 65535} is not valid under any of the given schemas (Possible causes of the failure): /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi86.yaml: properties:ti,backlight-scale: 'not' is a required property /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi86.yaml: ignoring, error in schema: properties: ti,backlight-scale warning: no schema found in file: ./Documentation/devicetree/bindings/display/bridge/ti,sn65dsi86.yaml See https://patchwork.ozlabs.org/patch/1374751 If you already ran 'make dt_binding_check' and didn't see the above error(s), then make sure dt-schema is up to date: pip3 install git+https://github.com/devicetree-org/dt-schema.git@master --upgrade Please check and re-submit. ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 2/2] drm/bridge: ti-sn65dsi86: Expose backlight controls 2020-09-30 22:35 [PATCH 0/2] drm/bridge: ti-sn65dsi86: Support backlight controls Bjorn Andersson 2020-09-30 22:35 ` [PATCH 1/2] dt-bindings: drm/bridge: ti-sn65dsi86: Replace #pwm-cells Bjorn Andersson @ 2020-09-30 22:35 ` Bjorn Andersson 2020-09-30 23:07 ` Steev Klimaszewski 2020-10-02 20:42 ` Doug Anderson 1 sibling, 2 replies; 10+ messages in thread From: Bjorn Andersson @ 2020-09-30 22:35 UTC (permalink / raw) To: David Airlie, Daniel Vetter, Rob Herring, Andrzej Hajda, Neil Armstrong, Laurent Pinchart, Jonas Karlman, Jernej Skrabec, Douglas Anderson Cc: dri-devel, devicetree, linux-kernel, linux-arm-msm The TI SN65DSI86 can be configured to generate a PWM pulse on GPIO4, to be used to drive a backlight driver. Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org> --- drivers/gpu/drm/bridge/Kconfig | 1 + drivers/gpu/drm/bridge/ti-sn65dsi86.c | 143 +++++++++++++++++++++++++- 2 files changed, 140 insertions(+), 4 deletions(-) diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig index 43271c21d3fc..eea310bd88e1 100644 --- a/drivers/gpu/drm/bridge/Kconfig +++ b/drivers/gpu/drm/bridge/Kconfig @@ -195,6 +195,7 @@ config DRM_TI_SN65DSI86 select REGMAP_I2C select DRM_PANEL select DRM_MIPI_DSI + select BACKLIGHT_CLASS_DEVICE help Texas Instruments SN65DSI86 DSI to eDP Bridge driver diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c index 5b6e19ecbc84..41e24d0dbd18 100644 --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c @@ -68,6 +68,7 @@ #define SN_GPIO_MUX_OUTPUT 1 #define SN_GPIO_MUX_SPECIAL 2 #define SN_GPIO_MUX_MASK 0x3 +#define SN_GPIO_MUX_SHIFT(gpio) ((gpio) * 2) #define SN_AUX_WDATA_REG(x) (0x64 + (x)) #define SN_AUX_ADDR_19_16_REG 0x74 #define SN_AUX_ADDR_15_8_REG 0x75 @@ -86,6 +87,12 @@ #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 SN_BACKLIGHT_REG 0xA3 +#define SN_PWM_CTL_REG 0xA5 +#define SN_PWM_ENABLE BIT(1) +#define SN_PWM_INVERT BIT(0) #define SN_AUX_CMD_STATUS_REG 0xF4 #define AUX_IRQ_STATUS_AUX_RPLY_TOUT BIT(3) #define AUX_IRQ_STATUS_AUX_SHORT BIT(5) @@ -155,6 +162,8 @@ struct ti_sn_bridge { struct gpio_chip gchip; DECLARE_BITMAP(gchip_output, SN_NUM_GPIOS); #endif + u32 brightness; + u32 max_brightness; }; static const struct regmap_range ti_sn_bridge_volatile_ranges[] = { @@ -173,6 +182,18 @@ static const struct regmap_config ti_sn_bridge_regmap_config = { .cache_type = REGCACHE_NONE, }; +static void ti_sn_bridge_read_u16(struct ti_sn_bridge *pdata, + unsigned int reg, u16 *val) +{ + unsigned int high; + unsigned int low; + + regmap_read(pdata->regmap, reg, &low); + regmap_read(pdata->regmap, reg + 1, &high); + + *val = high << 8 | low; +} + static void ti_sn_bridge_write_u16(struct ti_sn_bridge *pdata, unsigned int reg, u16 val) { @@ -180,6 +201,50 @@ static void ti_sn_bridge_write_u16(struct ti_sn_bridge *pdata, regmap_write(pdata->regmap, reg + 1, val >> 8); } +static int ti_sn_backlight_update(struct ti_sn_bridge *pdata) +{ + unsigned int pre_div; + + if (!pdata->max_brightness) + return 0; + + /* Enable PWM on GPIO4 */ + regmap_update_bits(pdata->regmap, SN_GPIO_CTRL_REG, + SN_GPIO_MUX_MASK << SN_GPIO_MUX_SHIFT(4 - 1), + SN_GPIO_MUX_SPECIAL << SN_GPIO_MUX_SHIFT(4 - 1)); + + if (pdata->brightness) { + /* Set max brightness */ + ti_sn_bridge_write_u16(pdata, SN_BACKLIGHT_SCALE_REG, pdata->max_brightness); + + /* Set brightness */ + ti_sn_bridge_write_u16(pdata, SN_BACKLIGHT_REG, pdata->brightness); + + /* + * The PWM frequency is derived from the refclk as: + * PWM_FREQ = REFCLK_FREQ / (PWM_PRE_DIV * BACKLIGHT_SCALE + 1) + * + * A hand wavy estimate based on 12MHz refclk and 500Hz desired + * PWM frequency gives us a pre_div resulting in a PWM + * frequency of between 500 and 1600Hz, depending on the actual + * refclk rate. + * + * One is added to avoid high BACKLIGHT_SCALE values to produce + * a pre_div of 0 - which cancels out the large BACKLIGHT_SCALE + * value. + */ + pre_div = 12000000 / (500 * pdata->max_brightness) + 1; + regmap_write(pdata->regmap, SN_PWM_PRE_DIV_REG, pre_div); + + /* Enable PWM */ + regmap_update_bits(pdata->regmap, SN_PWM_CTL_REG, SN_PWM_ENABLE, SN_PWM_ENABLE); + } else { + regmap_update_bits(pdata->regmap, SN_PWM_CTL_REG, SN_PWM_ENABLE, 0); + } + + return 0; +} + static int __maybe_unused ti_sn_bridge_resume(struct device *dev) { struct ti_sn_bridge *pdata = dev_get_drvdata(dev); @@ -193,7 +258,7 @@ static int __maybe_unused ti_sn_bridge_resume(struct device *dev) gpiod_set_value(pdata->enable_gpio, 1); - return ret; + return ti_sn_backlight_update(pdata); } static int __maybe_unused ti_sn_bridge_suspend(struct device *dev) @@ -1010,7 +1075,7 @@ static int ti_sn_bridge_gpio_direction_input(struct gpio_chip *chip, unsigned int offset) { struct ti_sn_bridge *pdata = gpiochip_get_data(chip); - int shift = offset * 2; + int shift = SN_GPIO_MUX_SHIFT(offset); int ret; if (!test_and_clear_bit(offset, pdata->gchip_output)) @@ -1038,7 +1103,7 @@ static int ti_sn_bridge_gpio_direction_output(struct gpio_chip *chip, unsigned int offset, int val) { struct ti_sn_bridge *pdata = gpiochip_get_data(chip); - int shift = offset * 2; + int shift = SN_GPIO_MUX_SHIFT(offset); int ret; if (test_and_set_bit(offset, pdata->gchip_output)) @@ -1073,12 +1138,17 @@ static const char * const ti_sn_bridge_gpio_names[SN_NUM_GPIOS] = { static int ti_sn_setup_gpio_controller(struct ti_sn_bridge *pdata) { + int ngpio = SN_NUM_GPIOS; int ret; /* Only init if someone is going to use us as a GPIO controller */ if (!of_property_read_bool(pdata->dev->of_node, "gpio-controller")) return 0; + /* If GPIO4 is used for backlight, reduce number of gpios */ + if (pdata->max_brightness) + ngpio--; + pdata->gchip.label = dev_name(pdata->dev); pdata->gchip.parent = pdata->dev; pdata->gchip.owner = THIS_MODULE; @@ -1092,7 +1162,7 @@ static int ti_sn_setup_gpio_controller(struct ti_sn_bridge *pdata) pdata->gchip.set = ti_sn_bridge_gpio_set; pdata->gchip.can_sleep = true; pdata->gchip.names = ti_sn_bridge_gpio_names; - pdata->gchip.ngpio = SN_NUM_GPIOS; + pdata->gchip.ngpio = ngpio; pdata->gchip.base = -1; ret = devm_gpiochip_add_data(pdata->dev, &pdata->gchip, pdata); if (ret) @@ -1159,6 +1229,65 @@ static void ti_sn_bridge_parse_lanes(struct ti_sn_bridge *pdata, pdata->ln_polrs = ln_polrs; } +static int ti_sn_backlight_update_status(struct backlight_device *bl) +{ + struct ti_sn_bridge *pdata = bl_get_data(bl); + int brightness = bl->props.brightness; + + if (bl->props.power != FB_BLANK_UNBLANK || + bl->props.fb_blank != FB_BLANK_UNBLANK || + bl->props.state & BL_CORE_FBBLANK) { + pdata->brightness = 0; + } + + pdata->brightness = brightness; + + return ti_sn_backlight_update(pdata); +} + +static int ti_sn_backlight_get_brightness(struct backlight_device *bl) +{ + struct ti_sn_bridge *pdata = bl_get_data(bl); + u16 val; + + ti_sn_bridge_read_u16(pdata, SN_BACKLIGHT_REG, &val); + + return val; +} + +const struct backlight_ops ti_sn_backlight_ops = { + .update_status = ti_sn_backlight_update_status, + .get_brightness = ti_sn_backlight_get_brightness, +}; + +static int ti_sn_backlight_init(struct ti_sn_bridge *pdata) +{ + struct backlight_properties props = {}; + struct backlight_device *bl; + struct device *dev = pdata->dev; + struct device_node *np = dev->of_node; + int ret; + + ret = of_property_read_u32(np, "ti,backlight-scale", &pdata->max_brightness); + if (ret == -EINVAL) { + return 0; + } else if (ret || pdata->max_brightness >= 0xffff) { + DRM_ERROR("invalid max-brightness\n"); + return -EINVAL; + } + + props.type = BACKLIGHT_RAW; + props.max_brightness = pdata->max_brightness; + bl = devm_backlight_device_register(dev, "sn65dsi86", dev, pdata, + &ti_sn_backlight_ops, &props); + if (IS_ERR(bl)) { + DRM_ERROR("failed to register backlight device\n"); + return PTR_ERR(bl); + } + + return 0; +} + static int ti_sn_bridge_probe(struct i2c_client *client, const struct i2c_device_id *id) { @@ -1224,6 +1353,12 @@ static int ti_sn_bridge_probe(struct i2c_client *client, pm_runtime_enable(pdata->dev); + ret = ti_sn_backlight_init(pdata); + if (ret) { + pm_runtime_disable(pdata->dev); + return ret; + } + ret = ti_sn_setup_gpio_controller(pdata); if (ret) { pm_runtime_disable(pdata->dev); -- 2.28.0 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] drm/bridge: ti-sn65dsi86: Expose backlight controls 2020-09-30 22:35 ` [PATCH 2/2] drm/bridge: ti-sn65dsi86: Expose backlight controls Bjorn Andersson @ 2020-09-30 23:07 ` Steev Klimaszewski 2020-10-02 20:42 ` Doug Anderson 1 sibling, 0 replies; 10+ messages in thread From: Steev Klimaszewski @ 2020-09-30 23:07 UTC (permalink / raw) To: Bjorn Andersson, David Airlie, Daniel Vetter, Rob Herring, Andrzej Hajda, Neil Armstrong, Laurent Pinchart, Jonas Karlman, Jernej Skrabec, Douglas Anderson Cc: dri-devel, devicetree, linux-kernel, linux-arm-msm On 9/30/20 5:35 PM, Bjorn Andersson wrote: > The TI SN65DSI86 can be configured to generate a PWM pulse on GPIO4, > to be used to drive a backlight driver. > > Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org> > --- > drivers/gpu/drm/bridge/Kconfig | 1 + > drivers/gpu/drm/bridge/ti-sn65dsi86.c | 143 +++++++++++++++++++++++++- > 2 files changed, 140 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig > index 43271c21d3fc..eea310bd88e1 100644 > --- a/drivers/gpu/drm/bridge/Kconfig > +++ b/drivers/gpu/drm/bridge/Kconfig > @@ -195,6 +195,7 @@ config DRM_TI_SN65DSI86 > select REGMAP_I2C > select DRM_PANEL > select DRM_MIPI_DSI > + select BACKLIGHT_CLASS_DEVICE > help > Texas Instruments SN65DSI86 DSI to eDP Bridge driver > > diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c > index 5b6e19ecbc84..41e24d0dbd18 100644 > --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c > +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c > @@ -68,6 +68,7 @@ > #define SN_GPIO_MUX_OUTPUT 1 > #define SN_GPIO_MUX_SPECIAL 2 > #define SN_GPIO_MUX_MASK 0x3 > +#define SN_GPIO_MUX_SHIFT(gpio) ((gpio) * 2) > #define SN_AUX_WDATA_REG(x) (0x64 + (x)) > #define SN_AUX_ADDR_19_16_REG 0x74 > #define SN_AUX_ADDR_15_8_REG 0x75 > @@ -86,6 +87,12 @@ > #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 SN_BACKLIGHT_REG 0xA3 > +#define SN_PWM_CTL_REG 0xA5 > +#define SN_PWM_ENABLE BIT(1) > +#define SN_PWM_INVERT BIT(0) > #define SN_AUX_CMD_STATUS_REG 0xF4 > #define AUX_IRQ_STATUS_AUX_RPLY_TOUT BIT(3) > #define AUX_IRQ_STATUS_AUX_SHORT BIT(5) > @@ -155,6 +162,8 @@ struct ti_sn_bridge { > struct gpio_chip gchip; > DECLARE_BITMAP(gchip_output, SN_NUM_GPIOS); > #endif > + u32 brightness; > + u32 max_brightness; > }; > > static const struct regmap_range ti_sn_bridge_volatile_ranges[] = { > @@ -173,6 +182,18 @@ static const struct regmap_config ti_sn_bridge_regmap_config = { > .cache_type = REGCACHE_NONE, > }; > > +static void ti_sn_bridge_read_u16(struct ti_sn_bridge *pdata, > + unsigned int reg, u16 *val) > +{ > + unsigned int high; > + unsigned int low; > + > + regmap_read(pdata->regmap, reg, &low); > + regmap_read(pdata->regmap, reg + 1, &high); > + > + *val = high << 8 | low; > +} > + > static void ti_sn_bridge_write_u16(struct ti_sn_bridge *pdata, > unsigned int reg, u16 val) > { > @@ -180,6 +201,50 @@ static void ti_sn_bridge_write_u16(struct ti_sn_bridge *pdata, > regmap_write(pdata->regmap, reg + 1, val >> 8); > } > > +static int ti_sn_backlight_update(struct ti_sn_bridge *pdata) > +{ > + unsigned int pre_div; > + > + if (!pdata->max_brightness) > + return 0; > + > + /* Enable PWM on GPIO4 */ > + regmap_update_bits(pdata->regmap, SN_GPIO_CTRL_REG, > + SN_GPIO_MUX_MASK << SN_GPIO_MUX_SHIFT(4 - 1), > + SN_GPIO_MUX_SPECIAL << SN_GPIO_MUX_SHIFT(4 - 1)); > + > + if (pdata->brightness) { > + /* Set max brightness */ > + ti_sn_bridge_write_u16(pdata, SN_BACKLIGHT_SCALE_REG, pdata->max_brightness); > + > + /* Set brightness */ > + ti_sn_bridge_write_u16(pdata, SN_BACKLIGHT_REG, pdata->brightness); > + > + /* > + * The PWM frequency is derived from the refclk as: > + * PWM_FREQ = REFCLK_FREQ / (PWM_PRE_DIV * BACKLIGHT_SCALE + 1) > + * > + * A hand wavy estimate based on 12MHz refclk and 500Hz desired > + * PWM frequency gives us a pre_div resulting in a PWM > + * frequency of between 500 and 1600Hz, depending on the actual > + * refclk rate. > + * > + * One is added to avoid high BACKLIGHT_SCALE values to produce > + * a pre_div of 0 - which cancels out the large BACKLIGHT_SCALE > + * value. > + */ > + pre_div = 12000000 / (500 * pdata->max_brightness) + 1; > + regmap_write(pdata->regmap, SN_PWM_PRE_DIV_REG, pre_div); > + > + /* Enable PWM */ > + regmap_update_bits(pdata->regmap, SN_PWM_CTL_REG, SN_PWM_ENABLE, SN_PWM_ENABLE); > + } else { > + regmap_update_bits(pdata->regmap, SN_PWM_CTL_REG, SN_PWM_ENABLE, 0); > + } > + > + return 0; > +} > + > static int __maybe_unused ti_sn_bridge_resume(struct device *dev) > { > struct ti_sn_bridge *pdata = dev_get_drvdata(dev); > @@ -193,7 +258,7 @@ static int __maybe_unused ti_sn_bridge_resume(struct device *dev) > > gpiod_set_value(pdata->enable_gpio, 1); > > - return ret; > + return ti_sn_backlight_update(pdata); > } > > static int __maybe_unused ti_sn_bridge_suspend(struct device *dev) > @@ -1010,7 +1075,7 @@ static int ti_sn_bridge_gpio_direction_input(struct gpio_chip *chip, > unsigned int offset) > { > struct ti_sn_bridge *pdata = gpiochip_get_data(chip); > - int shift = offset * 2; > + int shift = SN_GPIO_MUX_SHIFT(offset); > int ret; > > if (!test_and_clear_bit(offset, pdata->gchip_output)) > @@ -1038,7 +1103,7 @@ static int ti_sn_bridge_gpio_direction_output(struct gpio_chip *chip, > unsigned int offset, int val) > { > struct ti_sn_bridge *pdata = gpiochip_get_data(chip); > - int shift = offset * 2; > + int shift = SN_GPIO_MUX_SHIFT(offset); > int ret; > > if (test_and_set_bit(offset, pdata->gchip_output)) > @@ -1073,12 +1138,17 @@ static const char * const ti_sn_bridge_gpio_names[SN_NUM_GPIOS] = { > > static int ti_sn_setup_gpio_controller(struct ti_sn_bridge *pdata) > { > + int ngpio = SN_NUM_GPIOS; > int ret; > > /* Only init if someone is going to use us as a GPIO controller */ > if (!of_property_read_bool(pdata->dev->of_node, "gpio-controller")) > return 0; > > + /* If GPIO4 is used for backlight, reduce number of gpios */ > + if (pdata->max_brightness) > + ngpio--; > + > pdata->gchip.label = dev_name(pdata->dev); > pdata->gchip.parent = pdata->dev; > pdata->gchip.owner = THIS_MODULE; > @@ -1092,7 +1162,7 @@ static int ti_sn_setup_gpio_controller(struct ti_sn_bridge *pdata) > pdata->gchip.set = ti_sn_bridge_gpio_set; > pdata->gchip.can_sleep = true; > pdata->gchip.names = ti_sn_bridge_gpio_names; > - pdata->gchip.ngpio = SN_NUM_GPIOS; > + pdata->gchip.ngpio = ngpio; > pdata->gchip.base = -1; > ret = devm_gpiochip_add_data(pdata->dev, &pdata->gchip, pdata); > if (ret) > @@ -1159,6 +1229,65 @@ static void ti_sn_bridge_parse_lanes(struct ti_sn_bridge *pdata, > pdata->ln_polrs = ln_polrs; > } > > +static int ti_sn_backlight_update_status(struct backlight_device *bl) > +{ > + struct ti_sn_bridge *pdata = bl_get_data(bl); > + int brightness = bl->props.brightness; > + > + if (bl->props.power != FB_BLANK_UNBLANK || > + bl->props.fb_blank != FB_BLANK_UNBLANK || > + bl->props.state & BL_CORE_FBBLANK) { > + pdata->brightness = 0; > + } > + > + pdata->brightness = brightness; > + > + return ti_sn_backlight_update(pdata); > +} > + > +static int ti_sn_backlight_get_brightness(struct backlight_device *bl) > +{ > + struct ti_sn_bridge *pdata = bl_get_data(bl); > + u16 val; > + > + ti_sn_bridge_read_u16(pdata, SN_BACKLIGHT_REG, &val); > + > + return val; > +} > + > +const struct backlight_ops ti_sn_backlight_ops = { > + .update_status = ti_sn_backlight_update_status, > + .get_brightness = ti_sn_backlight_get_brightness, > +}; > + > +static int ti_sn_backlight_init(struct ti_sn_bridge *pdata) > +{ > + struct backlight_properties props = {}; > + struct backlight_device *bl; > + struct device *dev = pdata->dev; > + struct device_node *np = dev->of_node; > + int ret; > + > + ret = of_property_read_u32(np, "ti,backlight-scale", &pdata->max_brightness); > + if (ret == -EINVAL) { > + return 0; > + } else if (ret || pdata->max_brightness >= 0xffff) { > + DRM_ERROR("invalid max-brightness\n"); > + return -EINVAL; > + } > + > + props.type = BACKLIGHT_RAW; > + props.max_brightness = pdata->max_brightness; > + bl = devm_backlight_device_register(dev, "sn65dsi86", dev, pdata, > + &ti_sn_backlight_ops, &props); > + if (IS_ERR(bl)) { > + DRM_ERROR("failed to register backlight device\n"); > + return PTR_ERR(bl); > + } > + > + return 0; > +} > + > static int ti_sn_bridge_probe(struct i2c_client *client, > const struct i2c_device_id *id) > { > @@ -1224,6 +1353,12 @@ static int ti_sn_bridge_probe(struct i2c_client *client, > > pm_runtime_enable(pdata->dev); > > + ret = ti_sn_backlight_init(pdata); > + if (ret) { > + pm_runtime_disable(pdata->dev); > + return ret; > + } > + > ret = ti_sn_setup_gpio_controller(pdata); > if (ret) { > pm_runtime_disable(pdata->dev); Tested-By: Steev Klimaszewski <steev@kali.org> ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] drm/bridge: ti-sn65dsi86: Expose backlight controls 2020-09-30 22:35 ` [PATCH 2/2] drm/bridge: ti-sn65dsi86: Expose backlight controls Bjorn Andersson 2020-09-30 23:07 ` Steev Klimaszewski @ 2020-10-02 20:42 ` Doug Anderson 1 sibling, 0 replies; 10+ messages in thread From: Doug Anderson @ 2020-10-02 20:42 UTC (permalink / raw) To: Bjorn Andersson Cc: David Airlie, Daniel Vetter, Rob Herring, Andrzej Hajda, Neil Armstrong, Laurent Pinchart, Jonas Karlman, Jernej Skrabec, dri-devel, open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, LKML, linux-arm-msm Hi, On Wed, Sep 30, 2020 at 3:40 PM Bjorn Andersson <bjorn.andersson@linaro.org> wrote: > > The TI SN65DSI86 can be configured to generate a PWM pulse on GPIO4, > to be used to drive a backlight driver. > > Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org> > --- > drivers/gpu/drm/bridge/Kconfig | 1 + > drivers/gpu/drm/bridge/ti-sn65dsi86.c | 143 +++++++++++++++++++++++++- > 2 files changed, 140 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig > index 43271c21d3fc..eea310bd88e1 100644 > --- a/drivers/gpu/drm/bridge/Kconfig > +++ b/drivers/gpu/drm/bridge/Kconfig > @@ -195,6 +195,7 @@ config DRM_TI_SN65DSI86 > select REGMAP_I2C > select DRM_PANEL > select DRM_MIPI_DSI > + select BACKLIGHT_CLASS_DEVICE > help > Texas Instruments SN65DSI86 DSI to eDP Bridge driver > > diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c > index 5b6e19ecbc84..41e24d0dbd18 100644 > --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c > +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c > @@ -68,6 +68,7 @@ > #define SN_GPIO_MUX_OUTPUT 1 > #define SN_GPIO_MUX_SPECIAL 2 > #define SN_GPIO_MUX_MASK 0x3 > +#define SN_GPIO_MUX_SHIFT(gpio) ((gpio) * 2) > #define SN_AUX_WDATA_REG(x) (0x64 + (x)) > #define SN_AUX_ADDR_19_16_REG 0x74 > #define SN_AUX_ADDR_15_8_REG 0x75 > @@ -86,6 +87,12 @@ > #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 SN_BACKLIGHT_REG 0xA3 > +#define SN_PWM_CTL_REG 0xA5 > +#define SN_PWM_ENABLE BIT(1) > +#define SN_PWM_INVERT BIT(0) > #define SN_AUX_CMD_STATUS_REG 0xF4 > #define AUX_IRQ_STATUS_AUX_RPLY_TOUT BIT(3) > #define AUX_IRQ_STATUS_AUX_SHORT BIT(5) > @@ -155,6 +162,8 @@ struct ti_sn_bridge { > struct gpio_chip gchip; > DECLARE_BITMAP(gchip_output, SN_NUM_GPIOS); > #endif > + u32 brightness; > + u32 max_brightness; You missed adding to the docstring for brightness and max_brightness. Also: why do you need your own copy of these two values? Couldn't you just store the "struct backlight_device *" that came back from "devm_backlight_device_register()" and then reference bl->props.brightness / bl->props.max_brightness? > }; > > static const struct regmap_range ti_sn_bridge_volatile_ranges[] = { > @@ -173,6 +182,18 @@ static const struct regmap_config ti_sn_bridge_regmap_config = { > .cache_type = REGCACHE_NONE, > }; > > +static void ti_sn_bridge_read_u16(struct ti_sn_bridge *pdata, > + unsigned int reg, u16 *val) > +{ > + unsigned int high; > + unsigned int low; > + > + regmap_read(pdata->regmap, reg, &low); > + regmap_read(pdata->regmap, reg + 1, &high); > + > + *val = high << 8 | low; > +} Ideally you should be error checking your reads. I know this driver isn't very good about error checking the regmap reads in general, but probably that should be fixed. Certainly i2c-backed regmaps can have failures and you will then do your math on whatever uninitialized memory was on the stack. That seems bad. Presumably you'll then want to return the error code from this function? If for some reason you don't, your function should just return the val instead of passing by reference. > static void ti_sn_bridge_write_u16(struct ti_sn_bridge *pdata, > unsigned int reg, u16 val) > { > @@ -180,6 +201,50 @@ static void ti_sn_bridge_write_u16(struct ti_sn_bridge *pdata, > regmap_write(pdata->regmap, reg + 1, val >> 8); > } > > +static int ti_sn_backlight_update(struct ti_sn_bridge *pdata) > +{ > + unsigned int pre_div; > + > + if (!pdata->max_brightness) > + return 0; > + > + /* Enable PWM on GPIO4 */ > + regmap_update_bits(pdata->regmap, SN_GPIO_CTRL_REG, > + SN_GPIO_MUX_MASK << SN_GPIO_MUX_SHIFT(4 - 1), > + SN_GPIO_MUX_SPECIAL << SN_GPIO_MUX_SHIFT(4 - 1)); > + > + if (pdata->brightness) { > + /* Set max brightness */ > + ti_sn_bridge_write_u16(pdata, SN_BACKLIGHT_SCALE_REG, pdata->max_brightness); > + > + /* Set brightness */ > + ti_sn_bridge_write_u16(pdata, SN_BACKLIGHT_REG, pdata->brightness); > + > + /* > + * The PWM frequency is derived from the refclk as: > + * PWM_FREQ = REFCLK_FREQ / (PWM_PRE_DIV * BACKLIGHT_SCALE + 1) > + * > + * A hand wavy estimate based on 12MHz refclk and 500Hz desired > + * PWM frequency gives us a pre_div resulting in a PWM > + * frequency of between 500 and 1600Hz, depending on the actual > + * refclk rate. > + * > + * One is added to avoid high BACKLIGHT_SCALE values to produce > + * a pre_div of 0 - which cancels out the large BACKLIGHT_SCALE > + * value. > + */ > + pre_div = 12000000 / (500 * pdata->max_brightness) + 1; > + regmap_write(pdata->regmap, SN_PWM_PRE_DIV_REG, pre_div); Different panels have different requirements for PWM frequency. Some may also have different duty-cycle to brightness curves that differ based on the PWM frequency and it would be nice to make sure we know what frequency we're at rather than getting something random-ish. It feels like you need to be less hand-wavy. You should presumably specify the desired frequency in the device tree and then do the math. > + /* Enable PWM */ > + regmap_update_bits(pdata->regmap, SN_PWM_CTL_REG, SN_PWM_ENABLE, SN_PWM_ENABLE); > + } else { > + regmap_update_bits(pdata->regmap, SN_PWM_CTL_REG, SN_PWM_ENABLE, 0); > + } While technically it works OK to conflate brightness = 0 with backlight disabled (the PWM driver exposed by the Chrome OS EC does, at least), I believe the API in Linux does make a difference. Why not match the Linux API. If Linux says that the backlight should be at brightness 50 but should be off, set the brightness to 50 and turn the backlight off. If it says set the brightness to 0 and turn it on, honor it. I believe (but haven't tested) one side effect of the way you're doing is is that: set_brightness(50) blank() unblank() get_brightness() ...will return 0, not 50. I believe (but haven't tested) that if you don't implement get_brightness() it would fix things, > +static int ti_sn_backlight_update_status(struct backlight_device *bl) > +{ > + struct ti_sn_bridge *pdata = bl_get_data(bl); > + int brightness = bl->props.brightness; > + > + if (bl->props.power != FB_BLANK_UNBLANK || > + bl->props.fb_blank != FB_BLANK_UNBLANK || > + bl->props.state & BL_CORE_FBBLANK) { backlight_is_blank() instead of open-coding? ...or you somehow don't want the extra test for "BL_CORE_SUSPENDED" ? > + pdata->brightness = 0; As per comments in ti_sn_backlight_update(), IMO you want to keep enabled / disabled state separate from brightness. > + } > + > + pdata->brightness = brightness; > + > + return ti_sn_backlight_update(pdata); > +} Just to be neat and tidy, I'd expect something in the above would do a pm_runtime_get_sync() when the backlight first turns on and pm_runtime_put() when the backlight goes blank. Right now you're relying on the fact that the backlight is usually turned on later in the sequence, but it shouldn't hurt to add an extra pm_runtime reference and means you're no longer relying on the implicitness. ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2020-11-11 0:48 UTC | newest] Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-09-30 22:35 [PATCH 0/2] drm/bridge: ti-sn65dsi86: Support backlight controls Bjorn Andersson 2020-09-30 22:35 ` [PATCH 1/2] dt-bindings: drm/bridge: ti-sn65dsi86: Replace #pwm-cells Bjorn Andersson 2020-09-30 23:06 ` Steev Klimaszewski 2020-10-02 20:42 ` Doug Anderson 2020-11-02 17:08 ` Bjorn Andersson 2020-11-11 0:48 ` Doug Anderson 2020-10-05 13:50 ` Rob Herring 2020-09-30 22:35 ` [PATCH 2/2] drm/bridge: ti-sn65dsi86: Expose backlight controls Bjorn Andersson 2020-09-30 23:07 ` Steev Klimaszewski 2020-10-02 20:42 ` Doug Anderson
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).