Re: [PATCH v2 1/3] v4l: common: v4l2_get_link_freq: add printing a warning

From: Andrey Konovalov <andrey.konovalov@linaro.org>
To: Jacopo Mondi <jacopo@jmondi.org>
Cc: junak.pub@gmail.com, robert.foss@linaro.org,
	sakari.ailus@linux.intel.com, todor.too@gmail.com,
	agross@kernel.org, bjorn.andersson@linaro.org,
	mchehab@kernel.org, laurent.pinchart@ideasonboard.com,
	linux-media@vger.kernel.org, linux-arm-msm@vger.kernel.org,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2 1/3] v4l: common: v4l2_get_link_freq: add printing a warning
Date: Thu, 18 Feb 2021 20:14:10 +0300	[thread overview]
Message-ID: <b9bc3f15-ef8e-af4d-32dc-83cb82c1197b@linaro.org> (raw)
In-Reply-To: <20210218075510.dcqbpmft46bymmnd@uno.localdomain>

Hi Jacopo,

On 18.02.2021 10:55, Jacopo Mondi wrote:
> Hi Andrey,
> 
> On Thu, Feb 18, 2021 at 01:11:32AM +0300, Andrey Konovalov wrote:
>> Print a warning if V4L2_CID_LINK_FREQ control is not implemented.
>>
>> Signed-off-by: Andrey Konovalov <andrey.konovalov@linaro.org>
>> ---
>>   drivers/media/v4l2-core/v4l2-common.c | 2 ++
>>   1 file changed, 2 insertions(+)
>>
>> diff --git a/drivers/media/v4l2-core/v4l2-common.c b/drivers/media/v4l2-core/v4l2-common.c
>> index 133d20e40f82..f1abdf2ab4ec 100644
>> --- a/drivers/media/v4l2-core/v4l2-common.c
>> +++ b/drivers/media/v4l2-core/v4l2-common.c
>> @@ -461,6 +461,8 @@ s64 v4l2_get_link_freq(struct v4l2_ctrl_handler *handler, unsigned int mul,
>>
>>   		freq = qm.value;
>>   	} else {
>> +		pr_warn("%s: V4L2_CID_LINK_FREQ not implemented\n", __func__);
>> +
> 
> It's a shame we can't access a struct device * somehow :(

Right. Otherwise the message could be even more useful.

> Also, nitpicking (please bear with me here) it is absolutely correct
> that V4L2_CID_LINK_FREQ is not implemented, but I think the real deal
> here is that the link rate is estimanted from PIXEL_RATE and that
> might be wrong.

Correct.

> What about (insipired from the error message in match_fwnode() which I
> find useful)
> 
>                  pr_warn("%s: Link frequency estimanted using pixel rate: result might be inaccurate\n",
>                          __func__);
>                  pr_warn("%s: Consider implementing support for V4L2_CID_LINK_FREQ in the transmitter driver\n",
>                          __func___);

This text is fine for me.
Just the both of these pr_warn's wouldn't fit into the 80 characters limit.
I tried to think out something more descriptive within this limit, but
gave up soon.

Let me post the v2.1 of this single patch with the longer warning message.
Then the maintainers could probably merge whatever they prefer.
Or I could post the v3 of the whole patchset if the [PATCH v2.1 1/3]
gets positive feedback.

Thanks,
Andrey

> Anyway, whatever works
> Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
> 
> Thanks
>     j
> 
>>   		if (!mul || !div)
>>   			return -ENOENT;
>>
>> --
>> 2.17.1
>>