Re: [PATCH] [media] staging/imx: remove confusing IS_ERR_OR_NULL usage

[Arnd Bergmann <arnd@arndb.de>]

On Thu, Jun 29, 2017 at 11:13 AM, Philipp Zabel <p.zabel@pengutronix.de> wrote:

>> @@ -134,23 +134,26 @@ static void csi_idmac_put_ipu_resources(struct csi_priv *priv)
>>  static int csi_idmac_get_ipu_resources(struct csi_priv *priv)
>>  {
>>       int ch_num, ret;
>> +     struct ipu_smfc *smfc, *idmac_ch;
>
> This should be
>
> +       struct ipuv3_channel *idmac_ch;
> +       struct ipu_smfc *smfc;
>
> instead.

Fixed in v2 now.

>
> ... this changes behaviour:
>
>     imx-media: imx_media_of_parse failed with -17
>     imx-media: probe of capture-subsystem failed with error -17
>
> We must continue to return NULL here if imxsd == -EEXIST:
>
> -               return imxsd;
> +               return PTR_ERR(imxsd) == -EEXIST ? NULL : imxsd;
>
> or change the code where of_parse_subdev is called (from
> imx_media_of_parse, and recursively from of_parse_subdev) to not handle
> the -EEXIST return value as an error.
>
> With those fixed,
>
> Reviewed-by: Philipp Zabel <p.zabel@pengutronix.de>
> Tested-by: Philipp Zabel <p.zabel@pengutronix.de>

I thought about it some more and tried to find a better solution for this
function, which is now a bit different, so I did not add your tags.

Can you have another look at v2? This time, of_parse_subdev separates
the return code from the pointer, which seems less confusing in a function
like that. There are in fact two cases where we return NULL and it's
not clear if the caller should treat that as success or failure. I've left
the current behavior the same but added comments there.

     Arnd