[U-Boot] [PATCH 2/4 v3] dm: core: Add dm_remove_devices_flags() and hook it into device_remove()

[Simon Glass]

Hi Stefan,

On 25 March 2017 at 19:17, Simon Glass <sjg at chromium.org> wrote:
> Hi Stefan,
>
> On 22 March 2017 at 00:28, Stefan Roese <sr at denx.de> wrote:
>> The new function dm_remove_devices_flags() is intented for driver specific
>> last-stage cleanup operations before the OS is started. This patch adds
>> this functionality and hooks it into the common device_remove()
>> function.
>>
>> Drivers wanting to use this feature for some last-stage removal calls,
>> need to add one of the DM_REMOVE_xx flags to their driver .flags.
>>
>> Signed-off-by: Stefan Roese <sr at denx.de>
>> Reviewed-by: Simon Glass <sjg at chromium.org>
>> ---
>> v3:
>> - Add conditional compilation to fix compilation breakage on platforms
>>   without DM and DM_DEVICE_REMOVE support. With this change, Travis
>>   compiles all targets without any error
>>
>> v2:
>> - Added Simons Reviewed-by
>>
>>  drivers/core/device-remove.c | 15 +++++++++++----
>>  drivers/core/root.c          |  9 +++++++++
>>  include/dm/root.h            | 16 ++++++++++++++++
>>  3 files changed, 36 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/core/device-remove.c b/drivers/core/device-remove.c
>> index b80bf52320..ca4680f7c2 100644
>> --- a/drivers/core/device-remove.c
>> +++ b/drivers/core/device-remove.c
>> @@ -174,7 +174,12 @@ int device_remove(struct udevice *dev, uint flags)
>>         if (ret)
>>                 goto err;
>>
>> -       if (drv->remove) {
>> +       /*
>> +        * Remove the device if called with the "normal" remove flag set,
>> +        * or if the remove flag matches the driver flags
>> +        */
>> +       if (drv->remove &&
>> +           ((flags & DM_REMOVE_NORMAL) || (flags & drv->flags))) {

This seems to be comparing different things. The DM_REMOVE_NORMAL flag
is not from the same enum as drv->flags, is it?

>
> You are changing the condition here but still call the post_remove()
> method immediately below. If you decide note to remove the device then
> I think you should not call that function.
>
>>                 ret = drv->remove(dev);
>>                 if (ret)
>>                         goto err_remove;
>> @@ -188,10 +193,12 @@ int device_remove(struct udevice *dev, uint flags)
>>                 }
>>         }
>>
>> -       device_free(dev);
>> +       if ((flags & DM_REMOVE_NORMAL) || (flags & drv->flags)) {
>> +               device_free(dev);
>>
>> -       dev->seq = -1;
>> -       dev->flags &= ~DM_FLAG_ACTIVATED;
>> +               dev->seq = -1;
>> +               dev->flags &= ~DM_FLAG_ACTIVATED;
>> +       }
>>
>>         return ret;

Regards,
Simon