Re: [PATCH 3/6] backlight/pandora: Stop using BL_CORE_DRIVER1

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);
>>>
> 