[4/8] WmT: m-5mols_core style pad handling for adv7604

  ---
 drivers/media/i2c/adv7604.c |   12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)
  

First of all, this subject puzzles me... What means WmT ??

2015-01-29 17:19 GMT+01:00 William Towle <william.towle@codethink.co.uk>:
> ---
>  drivers/media/i2c/adv7604.c |   12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 deletions(-)

Again, it it passing checkpatch without signed-off-by ? And a little
description does not hurt :).

> diff --git a/drivers/media/i2c/adv7604.c b/drivers/media/i2c/adv7604.c
> index 30bbd9d..6ed9303 100644
> --- a/drivers/media/i2c/adv7604.c
> +++ b/drivers/media/i2c/adv7604.c
> @@ -1976,7 +1976,11 @@ static int adv7604_get_format(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh,
>         if (format->which == V4L2_SUBDEV_FORMAT_TRY) {
>                 struct v4l2_mbus_framefmt *fmt;
>
> -               fmt = v4l2_subdev_get_try_format(fh, format->pad);
> +               fmt = (fh == NULL) ? NULL
> +                       : v4l2_subdev_get_try_format(fh, format->pad);
> +               if (fmt == NULL)
> +                       return EINVAL;
> +

Mmmh, Hans probably has an explanation on this, I just don't get a use
case where fh can be NULL... So can't see the point of this patch ?

>                 format->format.code = fmt->code;
>         } else {
>                 format->format.code = state->format->code;
> @@ -2008,7 +2012,11 @@ static int adv7604_set_format(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh,
>         if (format->which == V4L2_SUBDEV_FORMAT_TRY) {
>                 struct v4l2_mbus_framefmt *fmt;
>
> -               fmt = v4l2_subdev_get_try_format(fh, format->pad);
> +               fmt = (fh == NULL) ? NULL
> +                       : v4l2_subdev_get_try_format(fh, format->pad);
> +               if (fmt == NULL)
> +                       return -EINVAL;
> +
>                 fmt->code = format->format.code;
>         } else {
>                 state->format = info;

Same comment as above.
Thanks,
JM
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
  

Hi Jean-Michel and others,

On Thu, 29 Jan 2015, Jean-Michel Hautbois wrote:
> First of all, this subject puzzles me... What means WmT ??

   That's just my initialism, to differentiate my work from that of
colleagues'. I'll submit without those in due course (and SOBs).


>> -               fmt = v4l2_subdev_get_try_format(fh, format->pad);
>> +               fmt = (fh == NULL) ? NULL
>> +                       : v4l2_subdev_get_try_format(fh, format->pad);
>> +               if (fmt == NULL)
>> +                       return EINVAL;
>> +
>
> Mmmh, Hans probably has an explanation on this, I just don't get a use
> case where fh can be NULL... So can't see the point of this patch ?

   There isn't currently a case where fh can be NULL, but we do
introduce them into rcar_vin_try_fmt() in the places where we add
v4l2_subdev_has_op() tests, which I am hoping to clarify.

   I have seen Guennadi's suggestion regarding wrapper functions, and am
also looking at the patchset Hans mentioned. In particular, the
description of "v4l2-subdev: replace v4l2_subdev_fh by
v4l2_subdev_pad_config" stands out as relevant.

   ...Thanks all, I will let you know how I get on.

Cheers,
   Wills.
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
  

On 02/04/15 15:14, William Towle wrote:
> 
> Hi Jean-Michel and others,
> 
> On Thu, 29 Jan 2015, Jean-Michel Hautbois wrote:
>> First of all, this subject puzzles me... What means WmT ??
> 
>   That's just my initialism, to differentiate my work from that of
> colleagues'. I'll submit without those in due course (and SOBs).
> 
> 
>>> -               fmt = v4l2_subdev_get_try_format(fh, format->pad);
>>> +               fmt = (fh == NULL) ? NULL
>>> +                       : v4l2_subdev_get_try_format(fh, format->pad);
>>> +               if (fmt == NULL)
>>> +                       return EINVAL;
>>> +
>>
>> Mmmh, Hans probably has an explanation on this, I just don't get a use
>> case where fh can be NULL... So can't see the point of this patch ?
> 
>   There isn't currently a case where fh can be NULL, but we do
> introduce them into rcar_vin_try_fmt() in the places where we add
> v4l2_subdev_has_op() tests, which I am hoping to clarify.
> 
>   I have seen Guennadi's suggestion regarding wrapper functions, and am
> also looking at the patchset Hans mentioned. In particular, the
> description of "v4l2-subdev: replace v4l2_subdev_fh by
> v4l2_subdev_pad_config" stands out as relevant.

FYI: I've rebased my branch to the latest media_tree master. It's
available here:

http://git.linuxtv.org/cgit.cgi/hverkuil/media_tree.git/log/?h=subdev2

(the last patch fixing adv7180 conflicts will of course be properly
merged in the final version).

I might have time tomorrow to work on this a bit more.

I won't accept wrapper functions: this mess has gone on long enough
and I want to see a proper solution where the old non-pad functions
are removed so everyone uses the same APIs.

Regards,

	Hans

> 
>   ...Thanks all, I will let you know how I get on.
> 
> Cheers,
>   Wills.

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html