On Wed, Nov 26, 2014 at 5:32 PM, Kim, Milo <milo.kim@xxxxxx> wrote:
> (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.
>
Hi Milo,
Thanks for the quick response. The reason I'm changing the existing
code is that it causes problems in the probe deferral case.
A concrete example: In my follow-on patch, I call devm_regulator_get
in the parse_dt function. Assume that something later on in probe()
returns -EPROBEDEFER. If we didn't have this patch, dev->platform_data
would continue to be set the next time probe() was called. This would
causes us to skip the parse_dt() function (because pdata is non-null
from last time) and we would have an invalid pointer to the regulator
we got the last time.
Sean
> 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@xxxxxxxxxxxx>
>> Signed-off-by: Sean Paul <seanpaul@xxxxxxxxxxxx>
>> ---
>> 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);
>>
>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
