From: Daniel Thompson <daniel.thompson@linaro.org>
To: DRI Development <dri-devel@lists.freedesktop.org>,
LKML <linux-kernel@vger.kernel.org>,
Lee Jones <lee.jones@linaro.org>,
Jingoo Han <jingoohan1@gmail.com>,
Daniel Vetter <daniel.vetter@intel.com>
Subject: Re: [PATCH 3/6] backlight/pandora: Stop using BL_CORE_DRIVER1
Date: Thu, 18 Jan 2018 09:42:24 +0000 [thread overview]
Message-ID: <7f1630fc-c764-e0d9-07b4-541918ec2574@linaro.org> (raw)
In-Reply-To: <20180117213819.GB2759@phenom.ffwll.local>
On 17/01/18 21:38, Daniel Vetter wrote:
> Thanks a lot for your comments.
>
> On Wed, Jan 17, 2018 at 04:47:41PM +0000, Daniel Thompson wrote:
>> On 17/01/18 14:01, Daniel Vetter wrote:
>>> Leaking driver internal tracking into the already massively confusing
>>> backlight power tracking is really confusing.
>>>
>>> Stop that by allocating a tiny driver private data structure instead.
>>>
>>> Cc: Lee Jones <lee.jones@linaro.org>
>>> Cc: Daniel Thompson <daniel.thompson@linaro.org>
>>> Cc: Jingoo Han <jingoohan1@gmail.com>
>>> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
>>> ---
>>> drivers/video/backlight/pandora_bl.c | 26 +++++++++++++++++++-------
>>> 1 file changed, 19 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/drivers/video/backlight/pandora_bl.c b/drivers/video/backlight/pandora_bl.c
>>> index a186bc677c7d..6bd159946a47 100644
>>> --- a/drivers/video/backlight/pandora_bl.c
>>> +++ b/drivers/video/backlight/pandora_bl.c
>>> @@ -35,11 +35,15 @@
>>> #define MAX_VALUE 63
>>> #define MAX_USER_VALUE (MAX_VALUE - MIN_VALUE)
>>> -#define PANDORABL_WAS_OFF BL_CORE_DRIVER1
>>> +struct pandora_private {
>>> + unsigned old_state;
>>> +#define PANDORABL_WAS_OFF 1
>>
>> Nit, but we using old_state like a bitfield so, BIT(0)?
>>
>>
>>> +};
>>> static int pandora_backlight_update_status(struct backlight_device *bl)
>>> {
>>> int brightness = bl->props.brightness;
>>> + struct pandora_private *priv = bl_get_data(bl);
>>> u8 r;
>>> if (bl->props.power != FB_BLANK_UNBLANK)
>>> @@ -53,7 +57,7 @@ static int pandora_backlight_update_status(struct backlight_device *bl)
>>> brightness = MAX_USER_VALUE;
>>> if (brightness == 0) {
>>> - if (bl->props.state & PANDORABL_WAS_OFF)
>>> + if (priv->old_state & PANDORABL_WAS_OFF)
>>> goto done;
>>> /* first disable PWM0 output, then clock */
>>> @@ -66,7 +70,7 @@ static int pandora_backlight_update_status(struct backlight_device *bl)
>>> goto done;
>>> }
>>> - if (bl->props.state & PANDORABL_WAS_OFF) {
>>> + if (priv->old_state & PANDORABL_WAS_OFF) {
>>> /*
>>> * set PWM duty cycle to max. TPS61161 seems to use this
>>> * to calibrate it's PWM sensitivity when it starts.
>>> @@ -93,9 +97,9 @@ static int pandora_backlight_update_status(struct backlight_device *bl)
>>> done:
>>> if (brightness != 0)
>>> - bl->props.state &= ~PANDORABL_WAS_OFF;
>>> + priv->old_state 0;
>>> else
>>> - bl->props.state |= PANDORABL_WAS_OFF;
>>> + priv->old_state = PANDORABL_WAS_OFF;
>>
>> Well, we were using it like a bitfield until this bit...
>
> I had a simple boolean first (because that's all we neeed), but that made
> the code less readable. Should I s/1/true/ in the #define? The entire C99
> bool tends to be a bit a bikeshed sometimes :-)
I'd prefer a simple boolean or just storing old_brightness directly in
priv... it is only ever consumed as a boolean anyway. I'd have to say
I'm not keen on #define true alongside the existing bitmask checks.
However... I'm happy to leave that with you. Providing the spurious
kfree() is removed then please feel free to repost with:
Acked-by: Daniel Thompson <daniel.thompson@linaro.org>
Daniel.
>
>>
>>
>>> return 0;
>>> }
>>> @@ -109,15 +113,23 @@ static int pandora_backlight_probe(struct platform_device *pdev)
>>> {
>>> struct backlight_properties props;
>>> struct backlight_device *bl;
>>> + struct pandora_private *priv;
>>> u8 r;
>>> + priv = devm_kmalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
>>> + if (!priv) {
>>> + dev_err(&pdev->dev, "failed to allocate driver private data\n");
>>> + return -ENOMEM;
>>> + }
>>> +
>>> memset(&props, 0, sizeof(props));
>>> props.max_brightness = MAX_USER_VALUE;
>>> props.type = BACKLIGHT_RAW;
>>> bl = devm_backlight_device_register(&pdev->dev, pdev->name, &pdev->dev,
>>> - NULL, &pandora_backlight_ops, &props);
>>> + priv, &pandora_backlight_ops, &props);
>>> if (IS_ERR(bl)) {
>>> dev_err(&pdev->dev, "failed to register backlight\n");
>>> + kfree(priv);
>>
>> Why can't we rely on devres for cleanup?
>
> Argh, I had kmalloc first and then changed to devm_kmalloc. The kfree here
> needs to go indeed.
>
> Cheers, Daniel
>
>>
>>
>>> return PTR_ERR(bl);
>>> }
>>> @@ -126,7 +138,7 @@ static int pandora_backlight_probe(struct platform_device *pdev)
>>> /* 64 cycle period, ON position 0 */
>>> twl_i2c_write_u8(TWL_MODULE_PWM, 0x80, TWL_PWM0_ON);
>>> - bl->props.state |= PANDORABL_WAS_OFF;
>>> + priv->old_state = PANDORABL_WAS_OFF;
>>> bl->props.brightness = MAX_USER_VALUE;
>>> backlight_update_status(bl);
>>>
>
next prev parent reply other threads:[~2018-01-18 9:42 UTC|newest]
Thread overview: 23+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-01-17 14:01 [PATCH 1/6] backlight: Nuke unused backlight.props.state states Daniel Vetter
2018-01-17 14:01 ` [PATCH 2/6] backlight/generic-bl: remove DRIVER1 state Daniel Vetter
2018-01-17 14:36 ` Emil Velikov
2018-01-17 16:37 ` Daniel Thompson
2018-01-18 13:08 ` Emil Velikov
2018-01-17 16:44 ` Daniel Thompson
2018-01-17 17:13 ` Daniel Vetter
2018-01-18 9:20 ` Daniel Thompson
2018-01-17 14:01 ` [PATCH 3/6] backlight/pandora: Stop using BL_CORE_DRIVER1 Daniel Vetter
2018-01-17 16:47 ` Daniel Thompson
2018-01-17 21:38 ` Daniel Vetter
2018-01-18 9:42 ` Daniel Thompson [this message]
2018-01-17 14:01 ` [PATCH 4/6] staging/fbtft: " Daniel Vetter
2018-01-17 16:50 ` Daniel Thompson
2018-01-17 14:01 ` [PATCH 5/6] backlight: Also nuke BL_CORE_DRIVER1 Daniel Vetter
2018-01-17 16:51 ` Daniel Thompson
2018-01-17 14:01 ` [PATCH 6/6] MAINTAINERS: add dri-devel for backlight subsystem patches Daniel Vetter
2018-01-17 16:52 ` Daniel Thompson
2018-01-17 17:06 ` Jingoo Han
2018-01-17 14:56 ` [PATCH 1/6] backlight: Nuke unused backlight.props.state states Daniel Thompson
2018-01-18 8:32 ` Lee Jones
2018-04-25 17:42 Daniel Vetter
2018-04-25 17:42 ` [PATCH 3/6] backlight/pandora: Stop using BL_CORE_DRIVER1 Daniel Vetter
2018-04-30 10:22 ` Jani Nikula
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=7f1630fc-c764-e0d9-07b4-541918ec2574@linaro.org \
--to=daniel.thompson@linaro.org \
--cc=daniel.vetter@intel.com \
--cc=dri-devel@lists.freedesktop.org \
--cc=jingoohan1@gmail.com \
--cc=lee.jones@linaro.org \
--cc=linux-kernel@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).