Re: [PATCH 1/2] backlight/lp855x: Refactor dt parsing code

["Kim, Milo" <milo.kim@ti.com>]

(Looping backlight class subsystem maintainers)

Hi Sean,

Thanks for checking this. I'd like to describe why the original code is 
preferred.

The original code is copying the values of the lp855x platform data from 
the DT in advance of allocating lp855x data.
It guarantees returning quickly in case of invalid platform data.
In other words, no memory allocation of lp855x proceeds if parsing the 
DT gets failed.
However, this patch allocates the lp855x first and checking the DT.
Of course, devm_kzalloc() will free allocated memory later but original 
code does NOT try to allocate it.
So I'd prefer to keep this way.

Best regards,
Milo

On 11/27/2014 4:11 AM, Sean Paul wrote:
> This patch refactors the dt parsing code to avoid setting platform_data,
> instead just setting lp->pdata directly. This facilitates easier
> probe deferral since the current scheme would require us to clear out
> dev->platform_data before deferring.
>
> Cc: Stéphane Marchesin <marcheu@chromium.org>
> Signed-off-by: Sean Paul <seanpaul@chromium.org>
> ---
>   drivers/video/backlight/lp855x_bl.c | 37 ++++++++++++++++++-------------------
>   1 file changed, 18 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/video/backlight/lp855x_bl.c b/drivers/video/backlight/lp855x_bl.c
> index 25fb8e3..257b3ba 100644
> --- a/drivers/video/backlight/lp855x_bl.c
> +++ b/drivers/video/backlight/lp855x_bl.c
> @@ -341,8 +341,10 @@ static const struct attribute_group lp855x_attr_group = {
>   };
>
>   #ifdef CONFIG_OF
> -static int lp855x_parse_dt(struct device *dev, struct device_node *node)
> +static int lp855x_parse_dt(struct lp855x *lp)
>   {
> +	struct device *dev = lp->dev;
> +	struct device_node *node = dev->of_node;
>   	struct lp855x_platform_data *pdata;
>   	int rom_length;
>
> @@ -381,12 +383,12 @@ static int lp855x_parse_dt(struct device *dev, struct device_node *node)
>   		pdata->rom_data = &rom[0];
>   	}
>
> -	dev->platform_data = pdata;
> +	lp->pdata = pdata;
>
>   	return 0;
>   }
>   #else
> -static int lp855x_parse_dt(struct device *dev, struct device_node *node)
> +static int lp855x_parse_dt(struct lp855x *lp)
>   {
>   	return -EINVAL;
>   }
> @@ -395,18 +397,8 @@ static int lp855x_parse_dt(struct device *dev, struct device_node *node)
>   static int lp855x_probe(struct i2c_client *cl, const struct i2c_device_id *id)
>   {
>   	struct lp855x *lp;
> -	struct lp855x_platform_data *pdata = dev_get_platdata(&cl->dev);
> -	struct device_node *node = cl->dev.of_node;
>   	int ret;
>
> -	if (!pdata) {
> -		ret = lp855x_parse_dt(&cl->dev, node);
> -		if (ret < 0)
> -			return ret;
> -
> -		pdata = dev_get_platdata(&cl->dev);
> -	}
> -
>   	if (!i2c_check_functionality(cl->adapter, I2C_FUNC_SMBUS_I2C_BLOCK))
>   		return -EIO;
>
> @@ -414,16 +406,23 @@ static int lp855x_probe(struct i2c_client *cl, const struct i2c_device_id *id)
>   	if (!lp)
>   		return -ENOMEM;
>
> -	if (pdata->period_ns > 0)
> -		lp->mode = PWM_BASED;
> -	else
> -		lp->mode = REGISTER_BASED;
> -
>   	lp->client = cl;
>   	lp->dev = &cl->dev;
> -	lp->pdata = pdata;
>   	lp->chipname = id->name;
>   	lp->chip_id = id->driver_data;
> +	lp->pdata = dev_get_platdata(&cl->dev);
> +
> +	if (!lp->pdata) {
> +		ret = lp855x_parse_dt(lp);
> +		if (ret < 0)
> +			return ret;
> +	}
> +
> +	if (lp->pdata->period_ns > 0)
> +		lp->mode = PWM_BASED;
> +	else
> +		lp->mode = REGISTER_BASED;
> +
>   	i2c_set_clientdata(cl, lp);
>
>   	ret = lp855x_configure(lp);
>