On 24/06/2020 10:11, Jacopo Mondi wrote:
> Hello Janusz,
> thanks for your quick reply
>
> On Sun, Jun 21, 2020 at 01:38:46PM +0200, Janusz Krzysztofik wrote:
>> Hi Jacopo,
>>
>> Thanks for bringing my attention to this patch.
>>
>> On Tuesday, June 16, 2020 4:12:38 P.M. CEST Jacopo Mondi wrote:
>>> Use the new get_mbus_config and set_mbus_config pad operations in place
>>> of the video operations currently in use.
>>>
>>> Compared to other drivers where the same conversion has been performed,
>>> ov6650 proved to be a bit more tricky, as the existing g_mbus_config
>>> implementation did not report the currently applied configuration but
>>> the set of all possible configuration options.
>>
>> Assuming that was in line with officially supported semantics of the old API,
>> not a misinterpretation, I would really like to see that limitation of the new
>> API actually compensated with V4L2_SUBDEV_FORMAT_TRY support added to it.
>>
>
> I'm not sure this is a limitation, it's more by design that the new
> get_mbus_config() only reports the current configuration.
Right. The old behavior was a left-over from the soc-camera driver that
required that. But soc-camera is from pre-device-tree times, and today this
information should come from the device tree.
>
> To be honest, compared to the other users of the old g_mbus_config()
> this driver was the only one implementing the operation in this way,
> maybe as it's the sole user of s_mbus_config() left out of staging ?
>
> I would however consider supporting FORMAT_TRY even if I'm not
> actually sure if fully makes sense. For the format operations
> (get/set_format()) FORMAT_TRY is used for concurrent applications to
> test a format without stepping on each other toes.
> get|set_mbus_config() are kAPI only, and I'm not sure we need to stay
> safe against concurrent configuration attempts... I'll think about
> this a bit more. Seems a development that could go on top, right ?
I wouldn't do this unless it turns out to be actually needed.
Regards,
Hans
>
>>>
>>> Adapt the driver to support the semantic of the two newly introducedV4L2_SUBDEV_FORMAT_TRY
>>> operations:
>>> - get_mbus_config reports the current media bus configuration
>>> - set_mbus_config applies only changes explicitly requested and updates
>>> the provided cfg parameter to report what has actually been applied to
>>> the hardware.
>>>
>>> Compile-tested only.
>>>
>>> Signed-off-by: Jacopo Mondi <jacopo+renesas@xxxxxxxxxx>
>>> ---
>>> drivers/media/i2c/ov6650.c | 56 ++++++++++++++++++++++++++------------
>>> 1 file changed, 39 insertions(+), 17 deletions(-)
>>>
>>> diff --git a/drivers/media/i2c/ov6650.c b/drivers/media/i2c/ov6650.c
>>> index 91906b94f978..d2e7a8556ed7 100644
>>> --- a/drivers/media/i2c/ov6650.c
>>> +++ b/drivers/media/i2c/ov6650.c
>>> @@ -921,46 +921,68 @@ static const struct v4l2_subdev_core_ops ov6650_core_ops = {
>>> };
>>>
>>> /* Request bus settings on camera side */
>>> -static int ov6650_g_mbus_config(struct v4l2_subdev *sd,
>>> - struct v4l2_mbus_config *cfg)
>>> +static int ov6650_get_mbus_config(struct v4l2_subdev *sd,
>>> + unsigned int pad,
>>> + struct v4l2_mbus_config *cfg)
>>> {
>>> + struct i2c_client *client = v4l2_get_subdevdata(sd);
>>> + u8 comj, comf;
>>> + int ret;
>>> +
>>> + ret = ov6650_reg_read(client, REG_COMJ, &comj);
>>> + if (ret)
>>> + return ret;
>>>
>>> - cfg->flags = V4L2_MBUS_MASTER |
>>> - V4L2_MBUS_PCLK_SAMPLE_RISING | V4L2_MBUS_PCLK_SAMPLE_FALLING |
>>> - V4L2_MBUS_HSYNC_ACTIVE_HIGH | V4L2_MBUS_HSYNC_ACTIVE_LOW |
>>> - V4L2_MBUS_VSYNC_ACTIVE_HIGH | V4L2_MBUS_VSYNC_ACTIVE_LOW |
>>> - V4L2_MBUS_DATA_ACTIVE_HIGH;
>>> + ret = ov6650_reg_read(client, REG_COMF, &comf);
>>> + if (ret)
>>> + return ret;
>>> +
>>> + cfg->flags = V4L2_MBUS_MASTER
>>> + | ((comj & COMJ_VSYNC_HIGH) ? V4L2_MBUS_VSYNC_ACTIVE_HIGH
>>> + : V4L2_MBUS_VSYNC_ACTIVE_LOW)
>>> + | ((comf & COMF_HREF_LOW) ? V4L2_MBUS_HSYNC_ACTIVE_LOW
>>> + : V4L2_MBUS_HSYNC_ACTIVE_HIGH)
>>> + | ((comj & COMJ_PCLK_RISING) ? V4L2_MBUS_PCLK_SAMPLE_RISING
>>> + : V4L2_MBUS_PCLK_SAMPLE_FALLING);
>>
>> You probably missed hardware default V4L2_MBUS_DATA_ACTIVE_HIGH.
>>
>
> Indeed I did :/
>
> Thanks for spotting
>
>>> cfg->type = V4L2_MBUS_PARALLEL;
>>>
>>> return 0;
>>> }
>>>
>>> /* Alter bus settings on camera side */
>>> -static int ov6650_s_mbus_config(struct v4l2_subdev *sd,
>>> - const struct v4l2_mbus_config *cfg)
>>> +static int ov6650_set_mbus_config(struct v4l2_subdev *sd,
>>> + unsigned int pad,
>>> + struct v4l2_mbus_config *cfg)
>>> {
>>> struct i2c_client *client = v4l2_get_subdevdata(sd);
>>> - int ret;
>>> + int ret = 0;
>>>
>>> if (cfg->flags & V4L2_MBUS_PCLK_SAMPLE_RISING)
>>> ret = ov6650_reg_rmw(client, REG_COMJ, COMJ_PCLK_RISING, 0);
>>> - else
>>> + else if (cfg->flags & V4L2_MBUS_PCLK_SAMPLE_FALLING)
>>
>> Have you thought of extending v4l2_subdev_call_pad_wrappers with a check for
>> only one of mutually exclusive flags specified by user?
>>
>
> Good question, but I wonder if this shouldn't be an accepted
> behaviour. The caller can provide all settings it want to allow the
> callee to chose which one to apply. The operation returns what has
> been actually applied by the callee, so that the caller can adjust
> itself to what the callee chose.
>
> Alternatively, it's up to the caller to specify its preference without
> mutually exclusive options, and the callee tries to adjust to what has
> been requested. Also in this case the operation returns what has
> actually been applied, so the caller can later adjust if it could.
>
> Seems like a small difference, but it might be good to exapnd the
> operations description to describe this to avoid each single
> implementer going in slightly different directions ?
>
>>> ret = ov6650_reg_rmw(client, REG_COMJ, 0, COMJ_PCLK_RISING);
>>> if (ret)
>>> - return ret;
>>> + goto error;
>>>
>>> if (cfg->flags & V4L2_MBUS_HSYNC_ACTIVE_LOW)
>>> ret = ov6650_reg_rmw(client, REG_COMF, COMF_HREF_LOW, 0);
>>> - else
>>> + else if (cfg->flags & V4L2_MBUS_HSYNC_ACTIVE_HIGH)
>>> ret = ov6650_reg_rmw(client, REG_COMF, 0, COMF_HREF_LOW);
>>> if (ret)
>>> - return ret;
>>> + goto error;
>>>
>>> if (cfg->flags & V4L2_MBUS_VSYNC_ACTIVE_HIGH)
>>> ret = ov6650_reg_rmw(client, REG_COMJ, COMJ_VSYNC_HIGH, 0);
>>> - else
>>> + else if (cfg->flags & V4L2_MBUS_VSYNC_ACTIVE_LOW)
>>> ret = ov6650_reg_rmw(client, REG_COMJ, 0, COMJ_VSYNC_HIGH);
>>>
>>> +error:
>>> + /*
>>> + * Update the configuration to report what is actually applied to
>>> + * the hardware.
>>> + */
>>> + ov6650_get_mbus_config(sd, pad, cfg);
>>
>> Populating cfg->flags by ov6650_get_mbus_config() without checking for a
>> potential error it may return can result in invalid data silently returned to
>> user. Maybe it would be better to fetch current hardware status first, fail on
>> error, then update the result with successfully performed hardware state
>> modifications.
>
> I'm not sure I got what you mean 8)
>
> Would if be enough to check for the return value of
> ov6650_get_mbus_config() (or actually returning it directly at the end
> of this function).
>
> Thanks
> j
>
>>
>> Thanks,
>> Janusz
>>
>>> +
>>> return ret;
>>> }
>>>
>>> @@ -968,8 +990,6 @@ static const struct v4l2_subdev_video_ops ov6650_video_ops = {
>>> .s_stream = ov6650_s_stream,
>>> .g_frame_interval = ov6650_g_frame_interval,
>>> .s_frame_interval = ov6650_s_frame_interval,
>>> - .g_mbus_config = ov6650_g_mbus_config,
>>> - .s_mbus_config = ov6650_s_mbus_config,
>>> };
>>>
>>> static const struct v4l2_subdev_pad_ops ov6650_pad_ops = {
>>> @@ -978,6 +998,8 @@ static const struct v4l2_subdev_pad_ops ov6650_pad_ops = {
>>> .set_selection = ov6650_set_selection,
>>> .get_fmt = ov6650_get_fmt,
>>> .set_fmt = ov6650_set_fmt,
>>> + .get_mbus_config = ov6650_get_mbus_config,
>>> + .set_mbus_config = ov6650_set_mbus_config,
>>> };
>>>
>>> static const struct v4l2_subdev_ops ov6650_subdev_ops = {
>>>
>>
>>
>>
>>
