Re: [PATCH] MAINTAINERS fixups

[Sylwester Nawrocki]

On 11/08/2018 02:23 PM, Hans Verkuil wrote:
> Update file paths in MAINTAINERS.
> 
> Signed-off-by: Hans Verkuil 
> Reported-by: Joe Perches 

Reviewed-by: Sylwester Nawrocki 

Re: [RFC PATCH 00/11] Convert last remaining g/s_crop/cropcap drivers

[Sylwester Nawrocki]

Hi Hans,

On 11/05/2018 02:12 PM, Hans Verkuil wrote:
> Thank you for the review. One question: have you also tested this with at 
> least
> one of the affected drivers?
> 
> I'd like to have at least one Tested-by line.

I just tested it now - video playback on Exynos4210 Trats2 so it covers 
the s5p-mfc and exynos4-is (fimc-m2m) drivers. Well done, I couldn't see 
any breakage.

You can add "Tested-by: Sylwester Nawrocki " 
to patches: 1, 2, 3, 7, 8, 10.

-- 
Regards,
Sylwester

Re: [RFC PATCH 10/11] v4l2-ioctl: remove unused vidioc_g/s_crop

[Sylwester Nawrocki]

On Fri, 5 Oct 2018 at 09:49, Hans Verkuil  wrote:
>
> From: Hans Verkuil 
>
> Now that all drivers have dropped vidioc_g/s_crop we can remove
> support for them in the V4L2 core.
>
> Signed-off-by: Hans Verkuil 

Reviewed-by: Sylwester Nawrocki 

Re: [RFC PATCH 08/11] exynos4-is: convert g/s_crop to g/s_selection

[Sylwester Nawrocki]

On Fri, 5 Oct 2018 at 09:49, Hans Verkuil  wrote:
>
> From: Hans Verkuil 
>
> Replace g/s_crop by g/s_selection and set the V4L2_FL_QUIRK_INVERTED_CROP
> flag since this is one of the old drivers that predates the selection
> API. Those old drivers allowed g_crop when it really shouldn't have since
> g_crop returns a compose rectangle instead of a crop rectangle for the
> CAPTURE stream, and vice versa for the OUTPUT stream.
>
> Also drop the now unused vidioc_cropcap.
>
> Signed-off-by: Hans Verkuil 

Reviewed-by: Sylwester Nawrocki 

Re: [RFC PATCH 09/11] s5p-g2d: convert g/s_crop to g/s_selection

[Sylwester Nawrocki]

On Fri, 5 Oct 2018 at 09:49, Hans Verkuil  wrote:
>
> From: Hans Verkuil 
>
> Replace g/s_crop by g/s_selection and set the V4L2_FL_QUIRK_INVERTED_CROP
> flag since this is one of the old drivers that predates the selection
> API. Those old drivers allowed g_crop when it really shouldn't have since
> g_crop returns a compose rectangle instead of a crop rectangle for the
> CAPTURE stream, and vice versa for the OUTPUT stream.
>
> Also drop the now unused vidioc_cropcap.
>
> Signed-off-by: Hans Verkuil 

Reviewed-by: Sylwester Nawrocki 

Re: [RFC PATCH 07/11] s5p_mfc_dec.c: convert g_crop to g_selection

[Sylwester Nawrocki]

On Fri, 5 Oct 2018 at 09:49, Hans Verkuil  wrote:
>
> From: Hans Verkuil 
>
> The g_crop really implemented composition for the CAPTURE stream.
>
> Replace g_crop by g_selection and set the V4L2_FL_QUIRK_INVERTED_CROP
> flag since this is one of the old drivers that predates the selection
> API. Those old drivers allowed g_crop when it really shouldn't have
> since g_crop returns a compose rectangle instead of a crop rectangle.
>
> Signed-off-by: Hans Verkuil 

Reviewed-by: Sylwester Nawrocki 

Re: [RFC PATCH 06/11] exynos-gsc: replace v4l2_crop by v4l2_selection

[Sylwester Nawrocki]

On Fri, 5 Oct 2018 at 09:49, Hans Verkuil  wrote:
>
> From: Hans Verkuil 
>
> Replace the use of struct v4l2_crop by struct v4l2_selection.
> Also drop the unused gsc_g_crop function.
>
> Signed-off-by: Hans Verkuil 

Reviewed-by: Sylwester Nawrocki 

Re: [RFC PATCH 03/11] v4l2-ioctl: add QUIRK_INVERTED_CROP

[Sylwester Nawrocki]

On Fri, 5 Oct 2018 at 09:49, Hans Verkuil  wrote:
>
> From: Hans Verkuil 
>
> Some old Samsung drivers use the legacy crop API incorrectly:
> the crop and compose targets are swapped. Normally VIDIOC_G_CROP
> will return the CROP rectangle of a CAPTURE stream and the COMPOSE
> rectangle of an OUTPUT stream.
>
> The Samsung drivers do the opposite. Note that these drivers predate
> the selection API.
>
> If this 'QUIRK' flag is set, then the v4l2-ioctl core will swap
> the CROP and COMPOSE targets as well.
>
> That way backwards compatibility is ensured and we can convert the
> Samsung drivers to the selection API.
>
> Signed-off-by: Hans Verkuil 

Reviewed-by: Sylwester Nawrocki 

Re: [RFC PATCH 01/11] v4l2-ioctl: don't use CROP/COMPOSE_ACTIVE

[Sylwester Nawrocki]

On Fri, 5 Oct 2018 at 09:49, Hans Verkuil  wrote:
>
> From: Hans Verkuil 
>
> Drop the deprecated _ACTIVE part.
>
> Signed-off-by: Hans Verkuil 

Reviewed-by: Sylwester Nawrocki 

Re: [RFC PATCH 00/11] Convert last remaining g/s_crop/cropcap drivers

[Sylwester Nawrocki]

Hi Hans,

On Fri, 5 Oct 2018 at 09:49, Hans Verkuil  wrote:
>
> From: Hans Verkuil 
>
> This patch series converts the last remaining drivers that use g/s_crop and
> cropcap to g/s_selection.

Thank you for this clean up! I remember attempting conversion of those remaining
drivers to selection API long time ago but I didn't have a good idea
then how to address
that crop and compose target inversion mess so I abandoned that efforts then.

--
Regards,
Sylwester

Re: [RESEND PATCH 1/1] v4l: samsung, ov9650: Rely on V4L2-set sub-device names

[Sylwester Nawrocki]

Hi Sakari,

On 09/16/2018 12:52 AM, Sakari Ailus wrote:
> v4l2_i2c_subdev_init() sets the name of the sub-devices (as well as
> entities) to what is fairly certainly known to be unique in the system,
> even if there were more devices of the same kind.
> 
> These drivers (m5mols, noon010pc30, ov9650, s5c73m3, s5k4ecgx, s5k6aa) set
> the name to the name of the driver or the module while omitting the
> device's I²C address and bus, leaving the devices with a static name and
> effectively limiting the number of such devices in a media device to 1.
> 
> Address this by using the name set by the V4L2 framework.
> 
> Signed-off-by: Sakari Ailus
> Reviewed-by: Akinobu Mita  (ov9650)

I'm not against this patch but please don't expect an ack from me as this
patch breaks existing user space code, scripts using media-ctl, etc. will 
need to be updated after kernel upgrade. I'm mostly concerned about ov9650
as other drivers are likely only used by Samsung or are obsoleted.

--
Thanks,
Sylwester

Re: [PATCH v5] media: imx208: Add imx208 camera sensor driver

[Sylwester Nawrocki]

On 09/20/2018 06:49 PM, Grant Grundler wrote:
> On Thu, Sep 20, 2018 at 1:52 AM Tomasz Figa  wrote:
>> On Wed, Aug 8, 2018 at 4:08 PM Ping-chung Chen
>>  wrote:

>>> +/* Digital gain control */

>>> +#define IMX208_DGTL_GAIN_MIN   0
>>> +#define IMX208_DGTL_GAIN_MAX   4096
>>> +#define IMX208_DGTL_GAIN_DEFAULT   0x100
>>> +#define IMX208_DGTL_GAIN_STEP   1

>>> +/* Initialize control handlers */
>>> +static int imx208_init_controls(struct imx208 *imx208)
>>> +{
>> [snip]
>>> +   v4l2_ctrl_new_std(ctrl_hdlr, _ctrl_ops, 
>>> V4L2_CID_DIGITAL_GAIN,
>>> + IMX208_DGTL_GAIN_MIN, IMX208_DGTL_GAIN_MAX,
>>> + IMX208_DGTL_GAIN_STEP,
>>> + IMX208_DGTL_GAIN_DEFAULT);
>>
>> We have a problem here. The sensor supports only a discrete range of
>> values here - {1, 2, 4, 8, 16} (multiplied by 256, since the value is
>> fixed point). This makes it possible for the userspace to set values
>> that are not allowed by the sensor specification and also leaves no
>> way to enumerate the supported values.

The driver could always adjust the value in set_ctrl callback so invalid
settings are not allowed.

I'm not sure if it's best approach but I once did something similar for
the ov9650 sensor. The gain was fixed point 10-bits value with 4 bits
for fractional part. The driver reports values multiplied by 16. See 
ov965x_set_gain() function in drivers/media/i2c/ov9650.c and "Table 4-1.
Total Gain to Control Bit Correlation" in the OV9650 datasheet for details. 
The integer menu control just seemed not suitable for 2^10 values. 
Now the gain control has range 16...1984 out of which only 1024 values 
are valid. It might not be best approach for a GUI but at least the driver 
exposes mapping of all valid values, which could be enumerated with 
VIDIOC_TRY_EXT_CTRLS if required, without a need for a driver-specific 
user space code.  

>> I can see two solutions here:
>>
>> 1) Define the control range from 0 to 4 and treat it as an exponent of
>> 2, so that the value for the sensor becomes (1 << val) * 256.
>> (Suggested by Sakari offline.)
>>
>> This approach has the problem of losing the original unit (and scale)
>> of the value.
> 
> Exactly - will users be confused by this? If we have to explain it,
> probably not the best choice.
> 
>>
>> 2) Use an integer menu control, which reports only the supported
>> discrete values - {1, 2, 4, 8, 16}.
>>
>> With this approach, userspace can enumerate the real gain values, but
>> we would either need to introduce a new control (e.g.
>> V4L2_CID_DIGITAL_GAIN_DISCRETE) or abuse the specification and
>> register V4L2_CID_DIGITAL_GAIN as an integer menu.
>>
>> Any opinions or better ideas?
> 
> My $0.02: leave the user UI alone - let users specify/select anything
> in the range the normal API or UI allows. But have sensor specific
> code map all values in that range to values the sensor supports. Users
> will notice how it works when they play with it.  One can "adjust" the
> mapping so it "feels right".

--
Regards,
Sylwester

Re: [RFC 1/1] v4l: samsung, ov9650: Rely on V4L2-set sub-device names

[Sylwester Nawrocki]

Hi Sakari,

On Thu, 13 Sep 2018 at 11:21, Sakari Ailus  wrote:
[...]
> diff --git a/drivers/media/i2c/s5c73m3/s5c73m3-core.c 
> b/drivers/media/i2c/s5c73m3/s5c73m3-core.c
> index ce196b60f917..64212551524e 100644
> --- a/drivers/media/i2c/s5c73m3/s5c73m3-core.c
> +++ b/drivers/media/i2c/s5c73m3/s5c73m3-core.c
> @@ -1683,7 +1683,7 @@ static int s5c73m3_probe(struct i2c_client *client,
> v4l2_subdev_init(sd, _subdev_ops);
> sd->owner = client->dev.driver->owner;
> v4l2_set_subdevdata(sd, state);
> -   strlcpy(sd->name, "S5C73M3", sizeof(sd->name));
> +   v4l2_i2c_subdev_set_name(sd, client, NULL, NULL);
>
> sd->internal_ops = _internal_ops;
> sd->flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
> @@ -1698,7 +1698,7 @@ static int s5c73m3_probe(struct i2c_client *client,
> return ret;
>
> v4l2_i2c_subdev_init(oif_sd, client, _subdev_ops);
> -   strcpy(oif_sd->name, "S5C73M3-OIF");
> +   v4l2_i2c_subdev_set_name(sd, client, NULL, "-OIF");

I would suggest to change the "OIF-" prefix to lower case, to avoid
something like
"s5c73m3-OIF". IIRC client->name is derived from DT compatible string, which is
in lower case.
Otherwise looks OK to me.

--
Thanks,
Sylwester

Re: [PATCH v2 2/3] s5p-g2d: Remove unrequired wait in .job_abort

[Sylwester Nawrocki]

On 07/06/2018 03:43 PM, Ezequiel Garcia wrote:


Could you elaborate how the core ensures DMA operation is not in
progress
after VIDIOC_STREAMOFF, VIDIOC_DQBUF with this patch applied?



Well, .streamoff is handled by v4l2_m2m_streamoff, which
guarantees that no job is running by calling v4l2_m2m_cancel_job.

The call chain goes like this:

vidioc_streamoff
   v4l2_m2m_ioctl_streamoff
 v4l2_m2m_streamoff
   v4l2_m2m_cancel_job
 wait_event(m2m_ctx->finished, ...);

The wait_event() wakes up by v4l2_m2m_job_finish(),
which is called by g2d_isr after marking the buffers
as done.

The reason why I haven't elaborated this in the commit log
is because it's documented in .job_abort declaration.


Indeed, you are right, job_abort implementation can be safely removed
in this case. As it is it doesn't help to handle cases when the HW gets
stuck and refuses to generate an interrupt. The rcar_jpu seems to be
addressing such situation properly though.


diff --git a/drivers/media/platform/s5p-g2d/g2d.c
b/drivers/media/platform/s5p-g2d/g2d.c
index 66aa8cf1d048..e98708883413 100644
--- a/drivers/media/platform/s5p-g2d/g2d.c
+++ b/drivers/media/platform/s5p-g2d/g2d.c
@@ -483,15 +483,6 @@ static int vidioc_s_crop(struct file *file,
void *prv, const struct v4l2_crop *c
   
   static void job_abort(void *prv)

   {
-   struct g2d_ctx *ctx = prv;
-   struct g2d_dev *dev = ctx->dev;
-
-   if (dev->curr == NULL) /* No job currently running */
-   return;
-
-   wait_event_timeout(dev->irq_queue,
-  dev->curr == NULL,
-  msecs_to_jiffies(G2D_TIMEOUT));


I think after this patch there will be a potential race condition
possible,
we could have the hardware DMA and CPU writing to same buffer with
sequence like:
...
QBUF
STREAMON
STREAMOFF
DQBUF
CPU accessing the buffer  <--  at this point G2D DMA could still be
writing
to an already dequeued buffer. Image processing can take few
miliseconds, it should
be fairly easy to trigger such a condition.



I don't think this is the case, as I've explained above. This commit
merely removes a redundant wait, as job_abort simply waits the
interrupt handler to complete, and that is the purpose of
v4l2_m2m_job_finish.

It only makes sense to implement job_abort if you can actually stop
the current DMA. If you can only wait for it to complete, then it's not
needed.


Agreed.


The intention of this series is simply to make job_abort optional,
and remove those drivers that implement job_abort as a wait-for-
current-job.


Sure, thanks for your effort.

--
Regards,
Sylwester

Re: [PATCH v2 2/3] s5p-g2d: Remove unrequired wait in .job_abort

[Sylwester Nawrocki]

Hi,

On 07/04/2018 10:04 AM, Hans Verkuil wrote:
> On 18/06/18 06:38, Ezequiel Garcia wrote:
>> As per the documentation, job_abort is not required
>> to wait until the current job finishes. It is redundant
>> to do so, as the core will perform the wait operation.

Could you elaborate how the core ensures DMA operation is not in progress
after VIDIOC_STREAMOFF, VIDIOC_DQBUF with this patch applied?

>> Remove the wait infrastructure completely.
> 
> Sylwester, can you review this?

Thanks for forwarding Hans!

>> Cc: Kyungmin Park 
>> Cc: Kamil Debski 
>> Cc: Andrzej Hajda 
>> Signed-off-by: Ezequiel Garcia 
>> ---
>>   drivers/media/platform/s5p-g2d/g2d.c | 11 ---
>>   drivers/media/platform/s5p-g2d/g2d.h |  1 -
>>   2 files changed, 12 deletions(-)
>>
>> diff --git a/drivers/media/platform/s5p-g2d/g2d.c 
>> b/drivers/media/platform/s5p-g2d/g2d.c
>> index 66aa8cf1d048..e98708883413 100644
>> --- a/drivers/media/platform/s5p-g2d/g2d.c
>> +++ b/drivers/media/platform/s5p-g2d/g2d.c
>> @@ -483,15 +483,6 @@ static int vidioc_s_crop(struct file *file, void *prv, 
>> const struct v4l2_crop *c
>>   
>>   static void job_abort(void *prv)
>>   {
>> -struct g2d_ctx *ctx = prv;
>> -struct g2d_dev *dev = ctx->dev;
>> -
>> -if (dev->curr == NULL) /* No job currently running */
>> -return;
>> -
>> -wait_event_timeout(dev->irq_queue,
>> -   dev->curr == NULL,
>> -   msecs_to_jiffies(G2D_TIMEOUT));

I think after this patch there will be a potential race condition possible,
we could have the hardware DMA and CPU writing to same buffer with sequence 
like:
...
QBUF
STREAMON
STREAMOFF
DQBUF 
CPU accessing the buffer  <--  at this point G2D DMA could still be writing
to an already dequeued buffer. Image processing can take few miliseconds, it 
should
be fairly easy to trigger such a condition.

Not saying about DMA being still in progress after device file handle is closed,
but that's something that deserves a separate patch. It seems there is a bug in 
the driver as there is no call to v4l2_m2m_ctx_release()in the device fops 
release() 
callback.

I think we could remove the s5p-g2d driver altogether as the functionality is 
covered
by the exynos DRM IPP driver (drivers/gpu/drm/exynos).

>>   }
>>   
>>   static void device_run(void *prv)
>> @@ -563,7 +554,6 @@ static irqreturn_t g2d_isr(int irq, void *prv)
>>  v4l2_m2m_job_finish(dev->m2m_dev, ctx->fh.m2m_ctx);
>>   
>>  dev->curr = NULL;
>> -wake_up(>irq_queue);
>>  return IRQ_HANDLED;
>>   }

--
Regards,
Sylwester

Re: [PATCH] media: s3c-camif: ignore -ENOIOCTLCMD from v4l2_subdev_call for s_power

[Sylwester Nawrocki]

On 06/10/2018 05:42 PM, Akinobu Mita wrote:

When the subdevice doesn't provide s_power core ops callback, the
v4l2_subdev_call for s_power returns -ENOIOCTLCMD.  If the subdevice
doesn't have the special handling for its power saving mode, the s_power
isn't required.  So -ENOIOCTLCMD from the v4l2_subdev_call should be
ignored.



Signed-off-by: Akinobu Mita


Acked-by: Sylwester Nawrocki 

[PATCH] s5p-mfc: Fix buffer look up in s5p_mfc_handle_frame_{new, copy_time} functions

[Sylwester Nawrocki]

Look up of buffers in s5p_mfc_handle_frame_new, s5p_mfc_handle_frame_copy_time
functions is not working properly for DMA addresses above 2 GiB. As a result
flags and timestamp of returned buffers are not set correctly and it breaks
operation of GStreamer/OMX plugins which rely on the CAPTURE buffer queue
flags.

Due to improper return type of the get_dec_y_adr, get_dspl_y_adr callbacks
and sign bit extension these callbacks return incorrect address values,
e.g. 0xfefc instead of 0xfefc. Then the statement:

"if (vb2_dma_contig_plane_dma_addr(_buf->b->vb2_buf, 0) == dec_y_addr)"

is always false, which breaks looking up capture queue buffers.

To ensure proper matching by address u32 type is used for the DMA
addresses. This should work on all related SoCs, since the MFC DMA
address width is not larger than 32-bit.

Changes done in this patch are minimal as there is a larger patch series
pending refactoring the whole driver.

Signed-off-by: Sylwester Nawrocki 
---
 drivers/media/platform/s5p-mfc/s5p_mfc.c | 23 ---
 1 file changed, 12 insertions(+), 11 deletions(-)

diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc.c 
b/drivers/media/platform/s5p-mfc/s5p_mfc.c
index a80251ed3143..780548dd650e 100644
--- a/drivers/media/platform/s5p-mfc/s5p_mfc.c
+++ b/drivers/media/platform/s5p-mfc/s5p_mfc.c
@@ -254,24 +254,24 @@ static void s5p_mfc_handle_frame_all_extracted(struct 
s5p_mfc_ctx *ctx)
 static void s5p_mfc_handle_frame_copy_time(struct s5p_mfc_ctx *ctx)
 {
struct s5p_mfc_dev *dev = ctx->dev;
-   struct s5p_mfc_buf  *dst_buf, *src_buf;
-   size_t dec_y_addr;
+   struct s5p_mfc_buf *dst_buf, *src_buf;
+   u32 dec_y_addr;
unsigned int frame_type;
 
/* Make sure we actually have a new frame before continuing. */
frame_type = s5p_mfc_hw_call(dev->mfc_ops, get_dec_frame_type, dev);
if (frame_type == S5P_FIMV_DECODE_FRAME_SKIPPED)
return;
-   dec_y_addr = s5p_mfc_hw_call(dev->mfc_ops, get_dec_y_adr, dev);
+   dec_y_addr = (u32)s5p_mfc_hw_call(dev->mfc_ops, get_dec_y_adr, dev);
 
/* Copy timestamp / timecode from decoded src to dst and set
   appropriate flags. */
src_buf = list_entry(ctx->src_queue.next, struct s5p_mfc_buf, list);
list_for_each_entry(dst_buf, >dst_queue, list) {
-   if (vb2_dma_contig_plane_dma_addr(_buf->b->vb2_buf, 0)
-   == dec_y_addr) {
-   dst_buf->b->timecode =
-   src_buf->b->timecode;
+   u32 addr = 
(u32)vb2_dma_contig_plane_dma_addr(_buf->b->vb2_buf, 0);
+
+   if (addr == dec_y_addr) {
+   dst_buf->b->timecode = src_buf->b->timecode;
dst_buf->b->vb2_buf.timestamp =
src_buf->b->vb2_buf.timestamp;
dst_buf->b->flags &=
@@ -307,10 +307,10 @@ static void s5p_mfc_handle_frame_new(struct s5p_mfc_ctx 
*ctx, unsigned int err)
 {
struct s5p_mfc_dev *dev = ctx->dev;
struct s5p_mfc_buf  *dst_buf;
-   size_t dspl_y_addr;
+   u32 dspl_y_addr;
unsigned int frame_type;
 
-   dspl_y_addr = s5p_mfc_hw_call(dev->mfc_ops, get_dspl_y_adr, dev);
+   dspl_y_addr = (u32)s5p_mfc_hw_call(dev->mfc_ops, get_dspl_y_adr, dev);
if (IS_MFCV6_PLUS(dev))
frame_type = s5p_mfc_hw_call(dev->mfc_ops,
get_disp_frame_type, ctx);
@@ -329,9 +329,10 @@ static void s5p_mfc_handle_frame_new(struct s5p_mfc_ctx 
*ctx, unsigned int err)
/* The MFC returns address of the buffer, now we have to
 * check which videobuf does it correspond to */
list_for_each_entry(dst_buf, >dst_queue, list) {
+   u32 addr = 
(u32)vb2_dma_contig_plane_dma_addr(_buf->b->vb2_buf, 0);
+
/* Check if this is the buffer we're looking for */
-   if (vb2_dma_contig_plane_dma_addr(_buf->b->vb2_buf, 0)
-   == dspl_y_addr) {
+   if (addr == dspl_y_addr) {
list_del(_buf->list);
ctx->dst_queue_cnt--;
dst_buf->b->sequence = ctx->sequence;
-- 
2.14.2

Re: [PATCH 3/7] s5p-mfc: fix two sparse warnings

[Sylwester Nawrocki]

On 05/14/2018 03:13 PM, Hans Verkuil wrote:
> From: Hans Verkuil <hans.verk...@cisco.com>
> 
> media-git/drivers/media/platform/s5p-mfc/s5p_mfc_enc.c: In function 
> 'vidioc_querycap':
> media-git/drivers/media/platform/s5p-mfc/s5p_mfc_enc.c:1317:2: warning: 
> 'strncpy' output may be truncated copying 31 bytes from a string of length 31 
> [-Wstringop-truncation]
>   strncpy(cap->card, dev->vfd_enc->name, sizeof(cap->card) - 1);
>   ^
> media-git/drivers/media/platform/s5p-mfc/s5p_mfc_dec.c: In function 
> 'vidioc_querycap':
> media-git/drivers/media/platform/s5p-mfc/s5p_mfc_dec.c:275:2: warning: 
> 'strncpy' output may be truncated copying 31 bytes from a string of length 31 
> [-Wstringop-truncation]
>   strncpy(cap->card, dev->vfd_dec->name, sizeof(cap->card) - 1);
>   ^
> 
> Signed-off-by: Hans Verkuil <hans.verk...@cisco.com>

Acked-by: Sylwester Nawrocki <s.nawro...@samsung.com>

Re: [PATCH 26/61] media: platform: exynos4-is: simplify getting .drvdata

[Sylwester Nawrocki]

On 04/19/2018 04:05 PM, Wolfram Sang wrote:
> We should get drvdata from struct device directly. Going via
> platform_device is an unneeded step back and forth.
> 
> Signed-off-by: Wolfram Sang <wsa+rene...@sang-engineering.com>

Acked-by: Sylwester Nawrocki <s.nawro...@samsung.com>

Re: [PATCH 27/61] media: platform: s5p-mfc: simplify getting .drvdata

[Sylwester Nawrocki]

On 04/19/2018 04:05 PM, Wolfram Sang wrote:
> We should get drvdata from struct device directly. Going via
> platform_device is an unneeded step back and forth.
> 
> Signed-off-by: Wolfram Sang <wsa+rene...@sang-engineering.com>

Acked-by: Sylwester Nawrocki <s.nawro...@samsung.com>

[PATCH] exynos4-is: Prevent NULL pointer dereference in __isp_video_try_fmt()

[Sylwester Nawrocki]

This patch fixes potential NULL pointer dereference as indicated
by the following static checker warning:

drivers/media/platform/exynos4-is/fimc-isp-video.c:408 
isp_video_try_fmt_mplane()
error: NULL dereference inside function '__isp_video_try_fmt(isp, 
>fmt.pix_mp, (0))()'.

Fixes: 34947b8aebe3: ("[media] exynos4-is: Add the FIMC-IS ISP capture DMA 
driver")
Reported-by: Dan Carpenter <dan.carpen...@oracle.com>
Signed-off-by: Sylwester Nawrocki <s.nawro...@samsung.com>
---
 drivers/media/platform/exynos4-is/fimc-isp-video.c | 11 ---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/drivers/media/platform/exynos4-is/fimc-isp-video.c 
b/drivers/media/platform/exynos4-is/fimc-isp-video.c
index 55ba696b8cf4..a920164f53f1 100644
--- a/drivers/media/platform/exynos4-is/fimc-isp-video.c
+++ b/drivers/media/platform/exynos4-is/fimc-isp-video.c
@@ -384,12 +384,17 @@ static void __isp_video_try_fmt(struct fimc_isp *isp,
struct v4l2_pix_format_mplane *pixm,
const struct fimc_fmt **fmt)
 {
-   *fmt = fimc_isp_find_format(>pixelformat, NULL, 2);
+   const struct fimc_fmt *__fmt;
+
+   __fmt = fimc_isp_find_format(>pixelformat, NULL, 2);
+
+   if (fmt)
+   *fmt = __fmt;
 
pixm->colorspace = V4L2_COLORSPACE_SRGB;
pixm->field = V4L2_FIELD_NONE;
-   pixm->num_planes = (*fmt)->memplanes;
-   pixm->pixelformat = (*fmt)->fourcc;
+   pixm->num_planes = __fmt->memplanes;
+   pixm->pixelformat = __fmt->fourcc;
/*
 * TODO: double check with the docmentation these width/height
 * constraints are correct.
-- 
2.14.2

Re: [bug report] [media] exynos4-is: Add the FIMC-IS ISP capture DMA driver

[Sylwester Nawrocki]

On 05/03/2018 01:43 PM, Dan Carpenter wrote:
> [ This code is five years old now.  It's so weird to me that the warning
>   is showing up in my new warnings pile.  Perhaps this wasn't included
>   in my allmodconfig before?  - dan ]

Might be, the bug found is real. The module is normally disabled anyway 
for other reasons.

> Hello Sylwester Nawrocki,
> 
> The patch 34947b8aebe3: "[media] exynos4-is: Add the FIMC-IS ISP
> capture DMA driver" from Dec 20, 2013, leads to the following static
> checker warning:
> 
>   drivers/media/platform/exynos4-is/fimc-isp-video.c:408 
> isp_video_try_fmt_mplane()
>   error: NULL dereference inside function '__isp_video_try_fmt(isp, 
> >fmt.pix_mp, (0))()'.
> 
> drivers/media/platform/exynos4-is/fimc-isp-video.c
>383  static void __isp_video_try_fmt(struct fimc_isp *isp,
>384  struct v4l2_pix_format_mplane *pixm,
>385  const struct fimc_fmt **fmt)
>386  {
>387  *fmt = fimc_isp_find_format(>pixelformat, NULL, 2);
> 
> Unchecked dereference.  We're not allowed to pass a NULL "fmt".
> 
>388  
>389  pixm->colorspace = V4L2_COLORSPACE_SRGB;
>390  pixm->field = V4L2_FIELD_NONE;
>391  pixm->num_planes = (*fmt)->memplanes;
>392  pixm->pixelformat = (*fmt)->fourcc;
>393  /*
>394   * TODO: double check with the docmentation these width/height
>395   * constraints are correct.
>396   */
>397  v4l_bound_align_image(>width, FIMC_ISP_SOURCE_WIDTH_MIN,
>398FIMC_ISP_SOURCE_WIDTH_MAX, 3,
>399>height, 
> FIMC_ISP_SOURCE_HEIGHT_MIN,
>400FIMC_ISP_SOURCE_HEIGHT_MAX, 0, 0);
>401  }
>402  
>403  static int isp_video_try_fmt_mplane(struct file *file, void *fh,
>404  struct v4l2_format *f)
>405  {
>406  struct fimc_isp *isp = video_drvdata(file);
>407  
>408  __isp_video_try_fmt(isp, >fmt.pix_mp, NULL);
>  
> And yet here we are.
> 
>409  return 0;
>410  }

We may need something like:

8<
diff --git a/drivers/media/platform/exynos4-is/fimc-isp-video.c 
b/drivers/media/platform/exynos4-is/fimc-isp-video.c
index 55ba696b8cf4..a920164f53f1 100644
--- a/drivers/media/platform/exynos4-is/fimc-isp-video.c
+++ b/drivers/media/platform/exynos4-is/fimc-isp-video.c
@@ -384,12 +384,17 @@ static void __isp_video_try_fmt(struct fimc_isp *isp,
struct v4l2_pix_format_mplane *pixm,
const struct fimc_fmt **fmt)
 {
-   *fmt = fimc_isp_find_format(>pixelformat, NULL, 2);
+   const struct fimc_fmt *__fmt;
+
+   __fmt = fimc_isp_find_format(>pixelformat, NULL, 2);
+
+   if (fmt)
+   *fmt = __fmt;
 
pixm->colorspace = V4L2_COLORSPACE_SRGB;
pixm->field = V4L2_FIELD_NONE;
-   pixm->num_planes = (*fmt)->memplanes;
-   pixm->pixelformat = (*fmt)->fourcc;
+   pixm->num_planes = __fmt->memplanes;
+   pixm->pixelformat = __fmt->fourcc;
/*
 * TODO: double check with the docmentation these width/height
 * constraints are correct.
8<

I will post the patch to clear the warning.

-- 
Thanks,
Sylwester

Re: [git:media_tree/master] media: include/media: fix missing | operator when setting cfg

[Sylwester Nawrocki]

Hi Mauro,

On 05/05/2018 03:01 PM, Mauro Carvalho Chehab wrote:
> This is an automatic generated email to let you know that the following patch 
> were queued:
> 
> Subject: media: include/media: fix missing | operator when setting cfg
I intended to submit this patch in a pull request, with subject line
fixed as mentioned in comments to the submitted match: 
https://patchwork.linuxtv.org/patch/48784

It might not be a big deal but it's not the first time patch gets
applied with review comments ignored. Then it looks like I acked 
something that I actually didn't. 

-- 
Regards,
Sylwester

Re: [PATCH] [media] include/media: fix missing | operator when setting cfg

[Sylwester Nawrocki]

On 04/18/2018 05:24 PM, Colin Ian King wrote:
> Oops, shall I re-send?

There is no need to, thanks.

Re: [PATCH] [media] include/media: fix missing | operator when setting cfg

[Sylwester Nawrocki]

On 04/18/2018 05:20 PM, Sylwester Nawrocki wrote:
> On 04/18/2018 05:06 PM, Colin King wrote:
>> From: Colin Ian King <colin.k...@canonical.com>
>>
>> The value from a readl is being masked with ITE_REG_CIOCAN_MASK however
>> this is not being used and cfg is being re-assigned.  I believe the
>> assignment operator should actually be instead the |= operator.
>>
>> Detected by CoverityScan, CID#1467987 ("Unused value")
>>
>> Signed-off-by: Colin Ian King <colin.k...@canonical.com>
> Thanks for the patch.
> 
> Acked-by: Sylwester Nawrocki <s.nawro...@samsung.com>

I forgot to mention that the subject should rather looks something
like:

"exynos4-is: fimc-lite: : fix missing | operator when setting cfg"

-- 
Regards,
Sylwester

Re: [PATCH] [media] include/media: fix missing | operator when setting cfg

[Sylwester Nawrocki]

On 04/18/2018 05:06 PM, Colin King wrote:
> From: Colin Ian King <colin.k...@canonical.com>
> 
> The value from a readl is being masked with ITE_REG_CIOCAN_MASK however
> this is not being used and cfg is being re-assigned.  I believe the
> assignment operator should actually be instead the |= operator.
> 
> Detected by CoverityScan, CID#1467987 ("Unused value")
> 
> Signed-off-by: Colin Ian King <colin.k...@canonical.com>

Thanks for the patch.

Acked-by: Sylwester Nawrocki <s.nawro...@samsung.com>

Re: [PATCH 07/16] media: exymos4-is: allow compile test for EXYNOS FIMC-LITE

[Sylwester Nawrocki]

On 04/05/2018 07:54 PM, Mauro Carvalho Chehab wrote:
> There's nothing that prevents building this driver with
> COMPILE_TEST. So, enable it.
> 
> While here, make the Kconfig dependency cleaner by removing
> the unneeded if block.
> 
> Signed-off-by: Mauro Carvalho Chehab <mche...@s-opensource.com>

With s/exymos4-is/exynos4-is in the subject

Acked-by: Sylwester Nawrocki <s.nawro...@samsung.com>

-- 
Regards,
Sylwester

[GIT PULL] HEVC V4L2 controls and s5p-mfc update

[Sylwester Nawrocki]

Hi Mauro,

The following changes since commit 3f127ce11353fd1071cae9b65bc13add6aec6b90:

  media: em28xx-cards: fix em28xx_duplicate_dev() (2018-03-08 06:06:51 -0500)

are available in the git repository at:

  git://linuxtv.org/snawrocki/samsung.git tags/v4.17-media-samsung

for you to fetch changes up to 8e8ae9fe34ac0611494b0b540476c4b9b52eac5b:

  s5p-mfc: Amend initial min, max values of HEVC hierarchical coding QP 
controls (2018-03-19 17:44:24 +0100)


Support for MFC v10.10 in the s5p-mfc driver and addition
of related HEVC V4L2 controls.


Marek Szyprowski (1):
  media: s5p-mfc: Use real device for request_firmware() call

Smitha T Murthy (12):
  videodev2.h: Add v4l2 definition for HEVC
  v4l2-ioctl: add HEVC format description
  v4l2: Documentation of HEVC compressed format
  v4l2: Add v4l2 control IDs for HEVC encoder
  v4l2: Documentation for HEVC CIDs
  s5p-mfc: Rename IS_MFCV8 macro
  s5p-mfc: Adding initial support for MFC v10.10
  s5p-mfc: Use min scratch buffer size as provided by F/W
  s5p-mfc: Support MFCv10.10 buffer requirements
  s5p-mfc: Add support for HEVC decoder
  s5p-mfc: Add VP9 decoder support
  s5p-mfc: Add support for HEVC encoder

Sylwester Nawrocki (2):
  s5p-mfc: Ensure HEVC QP controls range is properly updated
  s5p-mfc: Amend initial min, max values of HEVC hierarchical coding QP 
controls

 Documentation/devicetree/bindings/media/s5p-mfc.txt |   1 +
 Documentation/media/uapi/v4l/extended-controls.rst  | 410 ++
 Documentation/media/uapi/v4l/pixfmt-compressed.rst  |   5 +
 drivers/media/platform/s5p-mfc/regs-mfc-v10.h   |  87 +++
 drivers/media/platform/s5p-mfc/regs-mfc-v8.h|   2 +
 drivers/media/platform/s5p-mfc/s5p_mfc.c|  28 +
 drivers/media/platform/s5p-mfc/s5p_mfc_cmd_v6.c |   9 +
 drivers/media/platform/s5p-mfc/s5p_mfc_common.h |  68 ++-
 drivers/media/platform/s5p-mfc/s5p_mfc_ctrl.c   |   8 +-
 drivers/media/platform/s5p-mfc/s5p_mfc_dec.c|  48 +-
 drivers/media/platform/s5p-mfc/s5p_mfc_enc.c| 597 -
 drivers/media/platform/s5p-mfc/s5p_mfc_opr.h|  14 +
 drivers/media/platform/s5p-mfc/s5p_mfc_opr_v6.c | 397 --
 drivers/media/platform/s5p-mfc/s5p_mfc_opr_v6.h |  15 +
 drivers/media/v4l2-core/v4l2-ctrls.c| 119 
 drivers/media/v4l2-core/v4l2-ioctl.c|   1 +
 include/uapi/linux/v4l2-controls.h  |  93 +++-
 include/uapi/linux/videodev2.h  |   1 +
 18 files changed, 1824 insertions(+), 79 deletions(-)
 create mode 100644 drivers/media/platform/s5p-mfc/regs-mfc-v10.h


-- 
Regards,
Sylwester

[PATCH] s5p-mfc: Amend initial min, max values of HEVC hierarchical coding QP controls

[Sylwester Nawrocki]

Valid range for those controls is specified in documentation as [0, 51],
so initialize the controls to such range rather than [INT_MIN, INT_MAX].

Signed-off-by: Sylwester Nawrocki <s.nawro...@samsung.com>
---
 drivers/media/platform/s5p-mfc/s5p_mfc_enc.c | 28 ++--
 1 file changed, 14 insertions(+), 14 deletions(-)

diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc_enc.c 
b/drivers/media/platform/s5p-mfc/s5p_mfc_enc.c
index 810dabe2f1b9..7382b41f4f6d 100644
--- a/drivers/media/platform/s5p-mfc/s5p_mfc_enc.c
+++ b/drivers/media/platform/s5p-mfc/s5p_mfc_enc.c
@@ -856,56 +856,56 @@ static struct mfc_control controls[] = {
{
.id = V4L2_CID_MPEG_VIDEO_HEVC_HIER_CODING_L0_QP,
.type = V4L2_CTRL_TYPE_INTEGER,
-   .minimum = INT_MIN,
-   .maximum = INT_MAX,
+   .minimum = 0,
+   .maximum = 51,
.step = 1,
.default_value = 0,
},
{
.id = V4L2_CID_MPEG_VIDEO_HEVC_HIER_CODING_L1_QP,
.type = V4L2_CTRL_TYPE_INTEGER,
-   .minimum = INT_MIN,
-   .maximum = INT_MAX,
+   .minimum = 0,
+   .maximum = 51,
.step = 1,
.default_value = 0,
},
{
.id = V4L2_CID_MPEG_VIDEO_HEVC_HIER_CODING_L2_QP,
.type = V4L2_CTRL_TYPE_INTEGER,
-   .minimum = INT_MIN,
-   .maximum = INT_MAX,
+   .minimum = 0,
+   .maximum = 51,
.step = 1,
.default_value = 0,
},
{
.id = V4L2_CID_MPEG_VIDEO_HEVC_HIER_CODING_L3_QP,
.type = V4L2_CTRL_TYPE_INTEGER,
-   .minimum = INT_MIN,
-   .maximum = INT_MAX,
+   .minimum = 0,
+   .maximum = 51,
.step = 1,
.default_value = 0,
},
{
.id = V4L2_CID_MPEG_VIDEO_HEVC_HIER_CODING_L4_QP,
.type = V4L2_CTRL_TYPE_INTEGER,
-   .minimum = INT_MIN,
-   .maximum = INT_MAX,
+   .minimum = 0,
+   .maximum = 51,
.step = 1,
.default_value = 0,
},
{
.id = V4L2_CID_MPEG_VIDEO_HEVC_HIER_CODING_L5_QP,
.type = V4L2_CTRL_TYPE_INTEGER,
-   .minimum = INT_MIN,
-   .maximum = INT_MAX,
+   .minimum = 0,
+   .maximum = 51,
.step = 1,
.default_value = 0,
},
{
.id = V4L2_CID_MPEG_VIDEO_HEVC_HIER_CODING_L6_QP,
.type = V4L2_CTRL_TYPE_INTEGER,
-   .minimum = INT_MIN,
-   .maximum = INT_MAX,
+   .minimum = 0,
+   .maximum = 51,
.step = 1,
.default_value = 0,
},
-- 
2.14.2

[PATCH] s5p-mfc: Ensure HEVC QP controls range is properly updated

[Sylwester Nawrocki]

When value of V4L2_CID_MPEG_VIDEO_HEVC_MIN_QP or V4L2_CID_MPEG_VIDEO_HEVC_MAX_QP
controls is changed we should update range of a set of HEVC quantization
parameter v4l2 controls as specified in the HEVC controls documentation.

Signed-off-by: Sylwester Nawrocki <s.nawro...@samsung.com>
---
 drivers/media/platform/s5p-mfc/s5p_mfc_enc.c | 40 
 1 file changed, 40 insertions(+)

diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc_enc.c 
b/drivers/media/platform/s5p-mfc/s5p_mfc_enc.c
index 6c80ebc5dbcc..810dabe2f1b9 100644
--- a/drivers/media/platform/s5p-mfc/s5p_mfc_enc.c
+++ b/drivers/media/platform/s5p-mfc/s5p_mfc_enc.c
@@ -1773,6 +1773,42 @@ static inline int vui_sar_idc(enum 
v4l2_mpeg_video_h264_vui_sar_idc sar)
return t[sar];
 }
 
+/*
+ * Update range of all HEVC quantization parameter controls that depend on the
+ * V4L2_CID_MPEG_VIDEO_HEVC_MIN_QP, V4L2_CID_MPEG_VIDEO_HEVC_MAX_QP controls.
+ */
+static void __enc_update_hevc_qp_ctrls_range(struct s5p_mfc_ctx *ctx,
+int min, int max)
+{
+   static const int __hevc_qp_ctrls[] = {
+   V4L2_CID_MPEG_VIDEO_HEVC_I_FRAME_QP,
+   V4L2_CID_MPEG_VIDEO_HEVC_P_FRAME_QP,
+   V4L2_CID_MPEG_VIDEO_HEVC_B_FRAME_QP,
+   V4L2_CID_MPEG_VIDEO_HEVC_HIER_CODING_L0_QP,
+   V4L2_CID_MPEG_VIDEO_HEVC_HIER_CODING_L1_QP,
+   V4L2_CID_MPEG_VIDEO_HEVC_HIER_CODING_L2_QP,
+   V4L2_CID_MPEG_VIDEO_HEVC_HIER_CODING_L3_QP,
+   V4L2_CID_MPEG_VIDEO_HEVC_HIER_CODING_L4_QP,
+   V4L2_CID_MPEG_VIDEO_HEVC_HIER_CODING_L5_QP,
+   V4L2_CID_MPEG_VIDEO_HEVC_HIER_CODING_L6_QP,
+   };
+   struct v4l2_ctrl *ctrl = NULL;
+   int i, j;
+
+   for (i = 0; i < ARRAY_SIZE(__hevc_qp_ctrls); i++) {
+   for (j = 0; j < ARRAY_SIZE(ctx->ctrls); j++) {
+   if (ctx->ctrls[j]->id == __hevc_qp_ctrls[i]) {
+   ctrl = ctx->ctrls[j];
+   break;
+   }
+   }
+   if (WARN_ON(!ctrl))
+   break;
+
+   __v4l2_ctrl_modify_range(ctrl, min, max, ctrl->step, min);
+   }
+}
+
 static int s5p_mfc_enc_s_ctrl(struct v4l2_ctrl *ctrl)
 {
struct s5p_mfc_ctx *ctx = ctrl_to_ctx(ctrl);
@@ -2038,9 +2074,13 @@ static int s5p_mfc_enc_s_ctrl(struct v4l2_ctrl *ctrl)
break;
case V4L2_CID_MPEG_VIDEO_HEVC_MIN_QP:
p->codec.hevc.rc_min_qp = ctrl->val;
+   __enc_update_hevc_qp_ctrls_range(ctx, ctrl->val,
+p->codec.hevc.rc_max_qp);
break;
case V4L2_CID_MPEG_VIDEO_HEVC_MAX_QP:
p->codec.hevc.rc_max_qp = ctrl->val;
+   __enc_update_hevc_qp_ctrls_range(ctx, p->codec.hevc.rc_min_qp,
+ctrl->val);
break;
case V4L2_CID_MPEG_VIDEO_HEVC_LEVEL:
p->codec.hevc.level_v4l2 = ctrl->val;
-- 
2.14.2

Re: [GIT PULL] HEVC V4L2 controls and s5p-mfc update

[Sylwester Nawrocki]

On 03/12/2018 05:49 PM, Hans Verkuil wrote:
> On 03/12/2018 09:35 AM, Sylwester Nawrocki wrote:
>> On 03/12/2018 05:19 PM, Hans Verkuil wrote:
>>> Does this include this __v4l2_ctrl_modify_range() request:
>>>
>>> https://patchwork.kernel.org/patch/10196605/
>>>
>>> I haven't seen anything for that. I'd like to see that implemented before 
>>> this
>>> is merged, otherwise this typically will never happen.
>> Not yet, I assumed it will come afterwards.
>> I will write the patch myself until end of this week, unless Smitha submits 
>> it earlier.
>>
> OK, in that case I am OK with this pull request. Nice to see HEVC support in!

Hm, I have found some issues in the documentation after those patches.
I will try to fix it and also post the control range update patch.
I have marked the pull request as obsoleted, please ignore it.

-- 
Regards,
Sylwester

Re: [GIT PULL] HEVC V4L2 controls and s5p-mfc update

[Sylwester Nawrocki]

Hi Hans,

On 03/12/2018 05:19 PM, Hans Verkuil wrote:
> Does this include this __v4l2_ctrl_modify_range() request:
> 
> https://patchwork.kernel.org/patch/10196605/
> 
> I haven't seen anything for that. I'd like to see that implemented before this
> is merged, otherwise this typically will never happen.

Not yet, I assumed it will come afterwards.
I will write the patch myself until end of this week, unless Smitha submits 
it earlier.

-- 
Regards,
Sylwester

[GIT PULL] HEVC V4L2 controls and s5p-mfc update

[Sylwester Nawrocki]

Hi Mauro,

The following changes since commit 3f127ce11353fd1071cae9b65bc13add6aec6b90:

  media: em28xx-cards: fix em28xx_duplicate_dev() (2018-03-08 06:06:51 -0500)

are available in the git repository at:

  git://linuxtv.org/snawrocki/samsung.git tags/for-v4.17/media/samsung

for you to fetch changes up to 4d26b437a1d98495454119082d50a68eadb2da4a:

  s5p-mfc: Add support for HEVC encoder (2018-03-12 16:15:28 +0100)


Support for MFC v10.10 in the s5p-mfc driver and addition
of related HEVC V4L2 controls.


Smitha T Murthy (12):
  videodev2.h: Add v4l2 definition for HEVC
  v4l2-ioctl: add HEVC format description
  v4l2: Documentation of HEVC compressed format
  v4l2: Add v4l2 control IDs for HEVC encoder
  v4l2: Documentation for HEVC CIDs
  s5p-mfc: Rename IS_MFCV8 macro
  s5p-mfc: Adding initial support for MFC v10.10
  s5p-mfc: Use min scratch buffer size as provided by F/W
  s5p-mfc: Support MFCv10.10 buffer requirements
  s5p-mfc: Add support for HEVC decoder
  s5p-mfc: Add VP9 decoder support
  s5p-mfc: Add support for HEVC encoder

 Documentation/devicetree/bindings/media/s5p-mfc.txt |   1 +
 Documentation/media/uapi/v4l/extended-controls.rst  | 410 +++
 Documentation/media/uapi/v4l/pixfmt-compressed.rst  |   5 +
 drivers/media/platform/s5p-mfc/regs-mfc-v10.h   |  87 
 drivers/media/platform/s5p-mfc/regs-mfc-v8.h|   2 +
 drivers/media/platform/s5p-mfc/s5p_mfc.c|  28 ++
 drivers/media/platform/s5p-mfc/s5p_mfc_cmd_v6.c |   9 +
 drivers/media/platform/s5p-mfc/s5p_mfc_common.h |  68 ++-
 drivers/media/platform/s5p-mfc/s5p_mfc_ctrl.c   |   6 +-
 drivers/media/platform/s5p-mfc/s5p_mfc_dec.c|  48 +-
 drivers/media/platform/s5p-mfc/s5p_mfc_enc.c| 557 -
 drivers/media/platform/s5p-mfc/s5p_mfc_opr.h|  14 +
 drivers/media/platform/s5p-mfc/s5p_mfc_opr_v6.c | 397 +--
 drivers/media/platform/s5p-mfc/s5p_mfc_opr_v6.h |  15 +
 drivers/media/v4l2-core/v4l2-ctrls.c| 119 +
 drivers/media/v4l2-core/v4l2-ioctl.c|   1 +
 include/uapi/linux/v4l2-controls.h  |  93 +++-
 include/uapi/linux/videodev2.h  |   1 +
 18 files changed, 1783 insertions(+), 78 deletions(-)
 create mode 100644 drivers/media/platform/s5p-mfc/regs-mfc-v10.h

--
Regards,
Sylwester

Re: [PATCH v3 2/3] media: MAINTAINERS: add entry for ov9650 driver

[Sylwester Nawrocki]

On 01/21/2018 04:14 PM, Akinobu Mita wrote:
> This adds an entry to the MAINTAINERS file for ov9650 driver.  The
> following persons are added in this entry.
> 
> * Sakari as a person who looks after media sensor driver patches
> * Sylwester as a module author
> * Myself as a person who has the hardware and can test the patches
> 
> Cc: Sylwester Nawrocki <s.nawro...@samsung.com>
> Cc: Jacopo Mondi <jac...@jmondi.org>
> Cc: H. Nikolaus Schaller <h...@goldelico.com>
> Cc: Hugues Fruchet <hugues.fruc...@st.com>
> Cc: Sakari Ailus <sakari.ai...@linux.intel.com>
> Cc: Mauro Carvalho Chehab <mche...@s-opensource.com>
> Cc: Rob Herring <r...@kernel.org>
> Signed-off-by: Akinobu Mita <akinobu.m...@gmail.com>

Feel free to add my:

Acked-by: Sylwester Nawrocki <s.nawro...@samsung.com>

> ---
>  MAINTAINERS | 9 +
>  1 file changed, 9 insertions(+)
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index e358141..8924e39 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -10052,6 +10052,15 @@ S:   Maintained
>  F:   drivers/media/i2c/ov7670.c
>  F:   Documentation/devicetree/bindings/media/i2c/ov7670.txt
>  
> +OMNIVISION OV9650 SENSOR DRIVER
> +M:   Sakari Ailus <sakari.ai...@linux.intel.com>
> +R:   Akinobu Mita <akinobu.m...@gmail.com>
> +R:   Sylwester Nawrocki <s.nawro...@samsung.com>
> +L:   linux-media@vger.kernel.org
> +T:   git git://linuxtv.org/media_tree.git
> +S:   Maintained
> +F:   drivers/media/i2c/ov9650.c

Regards,
Sylwester

Re: [PATCH] [media] s3c-camif: array underflow in __camif_subdev_try_format()

[Sylwester Nawrocki]

On 01/22/2018 11:37 AM, Dan Carpenter wrote:
> The while loop is a post op, "while (i-- >= 0)" so the last iteration
> will read camif_mbus_formats[-1] and then the loop will exit with "i"
> set to -2 and so we do: "mf->code = camif_mbus_formats[-2];".
> 
> I've changed it to a pre-op, I've added a check to ensure we found the
> right format and I've removed the "mf->code = camif_mbus_formats[i];"
> because that is a no-op anyway.
> 
> Fixes: babde1c243b2 ("[media] V4L: Add driver for S3C24XX/S3C64XX SoC series 
> camera interface")
> Signed-off-by: Dan Carpenter 
> 
> diff --git a/drivers/media/platform/s3c-camif/camif-capture.c 
> b/drivers/media/platform/s3c-camif/camif-capture.c
> index 437395a61065..012f4b389c55 100644
> --- a/drivers/media/platform/s3c-camif/camif-capture.c
> +++ b/drivers/media/platform/s3c-camif/camif-capture.c
> @@ -1261,11 +1261,11 @@ static void __camif_subdev_try_format(struct 
> camif_dev *camif,
>   /* FIXME: constraints against codec or preview path ? */
>   pix_lim = >vp_pix_limits[VP_CODEC];
>   
> - while (i-- >= 0)
> + while (--i >= 0)
>   if (camif_mbus_formats[i] == mf->code)
>   break;
> -
> - mf->code = camif_mbus_formats[i];
> + if (i < 0)
> + return;

Thanks for the patch Dan. mf->width needs to be aligned by this try_format
function so we shouldn't return here. Also it needs to be ensured mf->code 
is set to one of the supported values when this function returns. Sorry,
the current code really doesn't give a clue what was intended.

There is already queued a patch from Arnd [1] addressing the issues you 
have found.
 
>   if (pad == CAMIF_SD_PAD_SINK) {
>   v4l_bound_align_image(>width, 8, CAMIF_MAX_PIX_WIDTH,
> 

[1] https://patchwork.linuxtv.org/patch/46508

--
Regards,
Sylwester

Re: [PATCH v2 2/2] media: ov9650: add device tree binding

[Sylwester Nawrocki]

On 01/08/2018 10:35 AM, Sakari Ailus wrote:
 
> I was going to say you're missing the MAINTAINERS entry for this newly
> added file but then I noticed that the entire driver is missing an entry.
> Still this file should have a MAINTAINERS entry added for it independently
> of that, in the same patch.
> 
> Cc Sylwester.

I don't the hardware and I can't test the patches so Mita-san if you wish
so please add yourself as a maintainer of whole driver.

-- 
Regards,
Sylwester

Re: [PATCH v2 1/2] media: ov9650: support device tree probing

[Sylwester Nawrocki]

if (!pdata) {
> - dev_err(>dev, "platform data not specified\n");
> - return -EINVAL;
> - }
> -
> - if (pdata->mclk_frequency == 0) {
> - dev_err(>dev, "MCLK frequency not specified\n");
> - return -EINVAL;
> - }
> -
>   ov965x = devm_kzalloc(>dev, sizeof(*ov965x), GFP_KERNEL);
>   if (!ov965x)
>   return -ENOMEM;
>  
> - mutex_init(>lock);

I would leave mutex initialization as first thing after the private data
structure allocation, is there a need to move it further?

>   ov965x->client = client;
> - ov965x->mclk_frequency = pdata->mclk_frequency;
> +
> + if (pdata) {
> + if (pdata->mclk_frequency == 0) {
> + dev_err(>dev, "MCLK frequency not specified\n");
> + return -EINVAL;
> + }
> + ov965x->mclk_frequency = pdata->mclk_frequency;
> +
> + ret = ov965x_configure_gpios_pdata(ov965x, pdata);
> + if (ret < 0)
> + return ret;
> + } else if (dev_fwnode(>dev)) {
> + ov965x->clk = devm_clk_get(>client->dev, NULL);
> + if (IS_ERR(ov965x->clk))
> + return PTR_ERR(ov965x->clk);
> + ov965x->mclk_frequency = clk_get_rate(ov965x->clk);
> +
> + ret = ov965x_configure_gpios(ov965x);
> + if (ret < 0)
> + return ret;
> + } else {
> + dev_err(>dev,
> + "Neither platform data nor device property 
> specified\n");
> +
> + return -EINVAL;
> + }
> +
> + mutex_init(>lock);
>  
>   sd = >sd;
>   v4l2_i2c_subdev_init(sd, client, _subdev_ops);
> @@ -1502,10 +1550,6 @@ static int ov965x_probe(struct i2c_client *client,
>   sd->flags |= V4L2_SUBDEV_FL_HAS_DEVNODE |
>V4L2_SUBDEV_FL_HAS_EVENTS;
>  
> - ret = ov965x_configure_gpios(ov965x, pdata);
> - if (ret < 0)
> - goto err_mutex;
> -
>   ov965x->pad.flags = MEDIA_PAD_FL_SOURCE;
>   sd->entity.function = MEDIA_ENT_F_CAM_SENSOR;
>   ret = media_entity_pads_init(>entity, 1, >pad);
> @@ -1561,9 +1605,19 @@ static const struct i2c_device_id ov965x_id[] = {
>  };
>  MODULE_DEVICE_TABLE(i2c, ov965x_id);
>  
> +#if IS_ENABLED(CONFIG_OF)

Is there any advantage in using IS_ENABLED() rather than just
#ifdef CONFIG_OF ? of_match_ptr() is defined with just #idef CONFIG_OF/
#else/#endif. I would use simply #ifdef CONFIG_OF here.

Otherwise looks good.

Reviewed-by: Sylwester Nawrocki <s.nawro...@samsung.com>

-- 
Regards,
Sylwester

[GIT PULL] Samsung SoC related updates

[Sylwester Nawrocki]

Hi Mauro,

The following changes since commit 0ca4e3130402caea8731a7b54afde56a6edb17c9:

  media: pxa_camera: rename the soc_camera_ prefix to pxa_camera_ (2017-12-14 
12:40:01 -0500)

are available in the git repository at:

  git://linuxtv.org/snawrocki/samsung.git for-v4.16/media/next

for you to fetch changes up to 8d10c3a3fa56badd9d8691b59a88e7f00fdeaa7b:

  s5p-jpeg: Fix off-by-one problem (2017-12-15 17:33:50 +0100)


Arnd Bergmann (1):
  exynos4-is: properly initialize frame format

Flavio Ceolin (1):
  s5p-jpeg: Fix off-by-one problem

Marek Szyprowski (3):
  exynos-gsc: Drop obsolete capabilities
  exynos4-is: Drop obsolete capabilities
  exynos4-is: Remove dependency on obsolete SoC support

Shuah Khan (2):
  s5p-mfc: Remove firmware buf null check in s5p_mfc_load_firmware()
  s5p-mfc: Fix lock contention - request_firmware() once

Simon Shields (1):
  exynos4-is: Check pipe is valid before calling subdev

Sylwester Nawrocki (1):
  s5p-mfc: Fix encoder menu controls initialization

 drivers/media/platform/exynos-gsc/gsc-m2m.c |  4 +---
 drivers/media/platform/exynos4-is/Kconfig   |  2 +-
 drivers/media/platform/exynos4-is/fimc-core.c   |  2 +-
 drivers/media/platform/exynos4-is/fimc-isp.c| 14 +++---
 drivers/media/platform/exynos4-is/fimc-lite.c   |  2 +-
 drivers/media/platform/exynos4-is/fimc-m2m.c| 10 +-
 drivers/media/platform/s5p-jpeg/jpeg-core.c |  2 +-
 drivers/media/platform/s5p-mfc/s5p_mfc.c|  6 ++
 drivers/media/platform/s5p-mfc/s5p_mfc_common.h |  3 +++
 drivers/media/platform/s5p-mfc/s5p_mfc_ctrl.c   | 10 +-
 drivers/media/platform/s5p-mfc/s5p_mfc_enc.c|  2 +-
 include/media/drv-intf/exynos-fimc.h|  3 ++-
 12 files changed, 30 insertions(+), 30 deletions(-)

-- 
Regards,
Sylwester

[PATCH] s5p-mfc: Fix encoder menu controls initialization

[Sylwester Nawrocki]

This patch fixes the menu_skip_mask field initialization
and addresses a following issue found by the SVACE static
analysis:

* NO_EFFECT.SELF: assignment to self in expression 'cfg.menu_skip_mask = 
cfg.menu_skip_mask'
  No effect at drivers/media/platform/s5p-mfc/s5p_mfc_enc.c:2083

Signed-off-by: Sylwester Nawrocki <s.nawro...@samsung.com>
---
 drivers/media/platform/s5p-mfc/s5p_mfc_enc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc_enc.c 
b/drivers/media/platform/s5p-mfc/s5p_mfc_enc.c
index 2a5fd7c42cd5..0d5d465561be 100644
--- a/drivers/media/platform/s5p-mfc/s5p_mfc_enc.c
+++ b/drivers/media/platform/s5p-mfc/s5p_mfc_enc.c
@@ -2080,7 +2080,7 @@ int s5p_mfc_enc_ctrls_setup(struct s5p_mfc_ctx *ctx)
 
if (cfg.type == V4L2_CTRL_TYPE_MENU) {
cfg.step = 0;
-   cfg.menu_skip_mask = cfg.menu_skip_mask;
+   cfg.menu_skip_mask = controls[i].menu_skip_mask;
cfg.qmenu = mfc51_get_menu(cfg.id);
} else {
cfg.step = controls[i].step;
-- 
2.14.2

Re: [Patch v6 10/12] [media] v4l2: Add v4l2 control IDs for HEVC encoder

[Sylwester Nawrocki]

On 12/12/2017 03:34 AM, Smitha T Murthy wrote:
>> s/Lay/Layer here and below
>>
> Ok I will change it.

While it's fine to make such change for controls up to 
V4L2_CID_MPEG_VIDEO_HEVC_HIER_CODING_L6_QP...

>>> +   case V4L2_CID_MPEG_VIDEO_HEVC_HIER_CODING_L1_QP:return "HEVC 
>>> Hierarchical Lay 1 QP";
>>> +   case V4L2_CID_MPEG_VIDEO_HEVC_HIER_CODING_L2_QP:return "HEVC 
>>> Hierarchical Lay 2 QP";
>>> +   case V4L2_CID_MPEG_VIDEO_HEVC_HIER_CODING_L3_QP:return "HEVC 
>>> Hierarchical Lay 3 QP";
>>> +   case V4L2_CID_MPEG_VIDEO_HEVC_HIER_CODING_L4_QP:return "HEVC 
>>> Hierarchical Lay 4 QP";
>>> +   case V4L2_CID_MPEG_VIDEO_HEVC_HIER_CODING_L5_QP:return "HEVC 
>>> Hierarchical Lay 5 QP";
>>> +   case V4L2_CID_MPEG_VIDEO_HEVC_HIER_CODING_L6_QP:return "HEVC 
>>> Hierarchical Lay 6 QP";

...for the controls below we may need to replace "Lay" with "L." 
to make sure the length of the string don't exceed 31 characters 
(32 with terminating NULL). The names below seem to be 1 character 
too long and will be truncated when running VIDIOC_QUERY_CTRL ioctl.

>>> +   case V4L2_CID_MPEG_VIDEO_HEVC_HIER_CODING_L0_BR:return "HEVC 
>>> Hierarchical Lay 0 Bit Rate";
>>> +   case V4L2_CID_MPEG_VIDEO_HEVC_HIER_CODING_L1_BR:return "HEVC 
>>> Hierarchical Lay 1 Bit Rate";
>>> +   case V4L2_CID_MPEG_VIDEO_HEVC_HIER_CODING_L2_BR:return "HEVC 
>>> Hierarchical Lay 2 Bit Rate";
>>> +   case V4L2_CID_MPEG_VIDEO_HEVC_HIER_CODING_L3_BR:return "HEVC 
>>> Hierarchical Lay 3 Bit Rate";
>>> +   case V4L2_CID_MPEG_VIDEO_HEVC_HIER_CODING_L4_BR:return "HEVC 
>>> Hierarchical Lay 4 Bit Rate";
>>> +   case V4L2_CID_MPEG_VIDEO_HEVC_HIER_CODING_L5_BR:return "HEVC 
>>> Hierarchical Lay 5 Bit Rate";
>>> +   case V4L2_CID_MPEG_VIDEO_HEVC_HIER_CODING_L6_BR:return "HEVC 
>>> Hierarchical Lay 6 Bit Rate";

--
Regards,
Sylwester

Re: [PATCH 12/22] media: s3c-camif: add missing description at s3c_camif_find_format()

[Sylwester Nawrocki]

On 11/29/2017 08:08 PM, Mauro Carvalho Chehab wrote:
> Fix this warning:
>   drivers/media/platform/s3c-camif/camif-core.c:112: warning: No 
> description found for parameter 'vp'
> 
> Signed-off-by: Mauro Carvalho Chehab <mche...@s-opensource.com>

Acked-by: Sylwester Nawrocki <s.nawro...@samsung.com>

Re: [PATCH 4/7] media: exynos4-is: Remove dependency on obsolete SoC support

[Sylwester Nawrocki]

On 10/04/2017 08:38 AM, Marek Szyprowski wrote:
> Support for Exynos4212 SoCs has been removed by commit bca9085e0ae9 ("ARM:
> dts: exynos: remove Exynos4212 support (dead code)"), so there is no need
> to keep remaining dead code related to this SoC version.
> 
> Signed-off-by: Marek Szyprowski <m.szyprow...@samsung.com>

Acked-by: Sylwester Nawrocki <s.nawro...@samsung.com>

[GIT PULL] Exynos/S5P updates for 4.15

[Sylwester Nawrocki]

Hi Mauro,

The following changes since commit 8382e556b1a2f30c4bf866f021b33577a64f9ebf:

  Simplify major/minor non-dynamic logic (2017-10-11 15:32:11 -0400)

are available in the git repository at:

  git://linuxtv.org/snawrocki/samsung.git for-v4.15/media/next

for you to fetch changes up to e5fa99e5df93e815920e87e907e5cb61e765505b:

  s5p-mfc: Adjust a null pointer check in four functions (2017-10-13 13:17:49 
+0200)


Hoegeun Kwon (2):
  exynos-gsc: Add compatible for Exynos 5250 and 5420 SoC version
  exynos-gsc: Add hardware rotation limits

Markus Elfring (3):
  s5p-mfc: Delete an error message for a failed memory allocation
  s5p-mfc: Improve a size determination in s5p_mfc_alloc_memdev()
  s5p-mfc: Adjust a null pointer check in four functions

 .../devicetree/bindings/media/exynos5-gsc.txt |   9 +-
 drivers/media/platform/exynos-gsc/gsc-core.c  | 127 -
 drivers/media/platform/s5p-mfc/s5p_mfc.c  |  14 +-
 3 files changed, 134 insertions(+), 16 deletions(-)


-- 
Regards,
Sylwester

Re: [PATCH v15 01/32] v4l: async: Remove re-probing support

[Sylwester Nawrocki]

Hi Sakari,

On 10/09/2017 04:18 PM, Sakari Ailus wrote:
> Sure, how about this at the end of the current commit message:
> 
> If there is a need to support removing the clock provider in the future,
> this should be implemented in the clock framework instead, not in V4L2.

I find it a little bit misleading, there is already support for removing
the clock provider, only any clock references for consumers became then
stale.  Perhaps:

"If there is a need to support the clock provider unregister/register 
cycle while keeping the clock references in the consumers in the future, 
this should be implemented in the clock framework instead, not in V4L2."

? That said, I doubt this issue is going to be entirely solved solely 
in the clock framework, as it is a more general problem of resource 
dependencies.  It could be related to other resources, like regulator
or GPIO.  It has been discussed for a long time now and it will likely 
take time until a general solution is available.

--
Thanks, 
Sylwester

Re: [RESEND PATCH v2 13/17] media: v4l2-async: simplify v4l2_async_subdev structure

[Sylwester Nawrocki]

On 09/28/2017 02:53 PM, Mauro Carvalho Chehab wrote:
> (Resending for Mauro, while dropping the non-list recipients. The original
> likely had too many recipients.)
> 
> The V4L2_ASYNC_MATCH_FWNODE match criteria requires just one
> struct to be filled (struct fwnode_handle). The V4L2_ASYNC_MATCH_DEVNAME
> match criteria requires just a device name.
> 
> So, it doesn't make sense to enclose those into structs,
> as the criteria can go directly into the union.
> 
> That makes easier to document it, as we don't need to document
> weird senseless structs.
> 
> At drivers, this makes even clearer about the match criteria.
> 
> Signed-off-by: Mauro Carvalho Chehab <mche...@s-opensource.com>


Acked-by: Sylwester Nawrocki <s.nawro...@samsung.com>

Re: [PATCH v2 13/17] media: v4l2-async: simplify v4l2_async_subdev structure

[Sylwester Nawrocki]

On 09/27/2017 11:46 PM, Mauro Carvalho Chehab wrote:
> The V4L2_ASYNC_MATCH_FWNODE match criteria requires just one
> struct to be filled (struct fwnode_handle). The V4L2_ASYNC_MATCH_DEVNAME
> match criteria requires just a device name.
> 
> So, it doesn't make sense to enclose those into structs,
> as the criteria can go directly into the union.
> 
> That makes easier to document it, as we don't need to document
> weird senseless structs.
> 
> At drivers, this makes even clearer about the match criteria.
> 
> Signed-off-by: Mauro Carvalho Chehab <mche...@s-opensource.com>

Acked-by: Sylwester Nawrocki <s.nawro...@samsung.com>

Re: as3645a flash userland interface

[Sylwester Nawrocki]

On 09/14/2017 01:53 PM, Pavel Machek wrote:

On Thu 2017-09-14 13:01:19, Sylwester Nawrocki wrote:

On 09/14/2017 12:07 PM, Pavel Machek wrote:

Isn't the V4L2 subdev/Media Controller API supposed to provide means
for associating flash LEDs with camera sensors? You seem to be insisting
on using the sysfs leds interface for that, which is not a primary
interface for camera flash AFAICT.


a) subdev/media controller API currently does not provide such means.


Yes, but it should, that's what it was designed for AFAIK.


b) if we have /sys/class/leds interface to userland, it should be
useful.


At the same time we shouldn't overcomplicate it with the camera
functionality.


I'm advocating adding label = "main_camera" into the .dts. That's all.


c) having flashlight application going through media controller API is
a bad joke.


It doesn't have to, maybe I misunderstood what you exactly ask for.
Nevertheless what's missing is some user visible name/label for each
flash LED, right? Currently enumerating flash LEDs can be done by looking
at the function part of /sys/class/leds/::
 path.

Could additional information be appended to the  part, so
user can identify which LED is which? E.g. "flash(rear)", "flash(front)",
etc. This could be achieved by simply adding label property in DT.
Or is the list of supported  strings already standardized?


label = "flash_main_camera" would work for me, yes. And yes, I'd
prefer to do this before 4.14 release, so that userland-visible
interface does not change.


Looking at existing label entries in DT (git grep label.*led
-- arch/arm/boot/dts | sed 's\^.*label\label\' | sort | uniq)
I can't see why something like this couldn't be added.
Or label = "as3645a::flash_main_camera" so we abide the LED device
naming rules described in Documentation/leds/leds-class.txt.

--
Regards,
Sylwester

Re: as3645a flash userland interface

[Sylwester Nawrocki]

On 09/14/2017 12:07 PM, Pavel Machek wrote:

Isn't the V4L2 subdev/Media Controller API supposed to provide means
for associating flash LEDs with camera sensors? You seem to be insisting
on using the sysfs leds interface for that, which is not a primary
interface for camera flash AFAICT.

>

a) subdev/media controller API currently does not provide such means.


Yes, but it should, that's what it was designed for AFAIK.


b) if we have /sys/class/leds interface to userland, it should be
useful.


At the same time we shouldn't overcomplicate it with the camera
functionality.


c) having flashlight application going through media controller API is
a bad joke.


It doesn't have to, maybe I misunderstood what you exactly ask for.
Nevertheless what's missing is some user visible name/label for each
flash LED, right? Currently enumerating flash LEDs can be done by looking
at the function part of /sys/class/leds/::
 path.

Could additional information be appended to the  part, so
user can identify which LED is which? E.g. "flash(rear)", "flash(front)",
etc. This could be achieved by simply adding label property in DT.
Or is the list of supported  strings already standardized?

--
Regards,
Sylwester

Re: as3645a flash userland interface

[Sylwester Nawrocki]

Hi,

On 09/13/2017 07:53 PM, Jacek Anaszewski wrote:

On 09/12/2017 11:55 PM, Pavel Machek wrote:

On Tue 2017-09-12 20:53:33, Jacek Anaszewski wrote:

On 09/12/2017 12:36 PM, Pavel Machek wrote:



What directory are the flash controls in?

/sys/class/leds/led-controller:flash ?

Could we arrange for something less generic, like

/sys/class/leds/main-camera:flash ?


I'd rather avoid overcomplicating this. LED class device name pattern
is well defined to devicename:colour:function
(see Documentation/leds/leds-class.txt, "LED Device Naming" section).

In this case "flash" in place of the "function" segment makes the
things clear enough I suppose.


It does not.

Phones usually have two cameras, front and back, and these days both
cameras have their flash.

And poor userspace flashlight application can not know if as3645
drivers front LED or back LED. Thus, I'd set devicename to
front-camera or main-camera -- because that's what it is associated
with. Userspace does not care what hardware drives the LED, but needs
to know if it is front or back camera.


The name of a LED flash class device isn't fixed and is derived
from DT label property. Name in the example of some DT bindings
will not force people to apply similar pattern for the other
drivers and even for the related one. No worry about having
to keep anything forever basing on that.


Isn't the V4L2 subdev/Media Controller API supposed to provide means
for associating flash LEDs with camera sensors? You seem to be insisting
on using the sysfs leds interface for that, which is not a primary
interface for camera flash AFAICT.

--
Regards,
Sylwester

Re: [PATCH] [media] s3c-camif: fix out-of-bounds array access

[Sylwester Nawrocki]

On 09/13/2017 04:03 PM, Arnd Bergmann wrote:

On Wed, Sep 13, 2017 at 11:25 AM, Sylwester Nawrocki
<s.nawro...@samsung.com>  wrote:

On 09/12/2017 10:09 PM, Arnd Bergmann wrote:

   {
   const struct s3c_camif_variant *variant = camif->variant;
   const struct vp_pix_limits *pix_lim;
- int i = ARRAY_SIZE(camif_mbus_formats);

   /* FIXME: constraints against codec or preview path ? */
   pix_lim = >vp_pix_limits[VP_CODEC];

- while (i-- >= 0)
- if (camif_mbus_formats[i] == mf->code)
- break;
-
- mf->code = camif_mbus_formats[i];


Interesting finding... the function needs to ensure mf->code is set
to one of supported values by the driver, so instead of removing
how about changing the above line to:

 if (i < 0)
 mf->code = camif_mbus_formats[0];

?

That would still have one of the two out-of-bounds accesses;-)


Ah, indeed :/


maybe this

for (i = 0; i < ARRAY_SIZE(camif_mbus_formats); i++)
 if (camif_mbus_formats[i] == mf->code)
break;

if (i == ARRAY_SIZE(camif_mbus_formats))
mf->code = camif_mbus_formats[0];


Yes, it should work that way.

--
Thanks,
Sylwester

Re: [PATCH] s5p-cec: add NACK detection support

[Sylwester Nawrocki]

On 08/31/2017 06:56 PM, Hans Verkuil wrote:

The s5p-cec driver returned CEC_TX_STATUS_ERROR for the NACK condition.

Some digging into the datasheet uncovered the S5P_CEC_TX_STAT1 register where
bit 0 indicates if the transmit was nacked or not.

Use this to return the correct CEC_TX_STATUS_NACK status to userspace.

This was the only driver that couldn't tell a NACK from another error, and
that was very unusual. And a potential problem for applications as well.

Tested with my Odroid-U3.

Signed-off-by: Hans Verkuil <hans.verk...@cisco.com>


Acked-by: Sylwester Nawrocki <s.nawro...@samsung.com>

Re: [PATCH v4 0/4] Exynos-gsc: Support the hardware rotation limits

[Sylwester Nawrocki]

On 09/13/2017 01:41 PM, Hoegeun Kwon wrote:

Hoegeun Kwon (4):
   [media] exynos-gsc: Add compatible for Exynos 5250 and 5420 specific
 version
   ARM: dts: exynos: Add clean name of compatible.
   drm/exynos/gsc: Add hardware rotation limits
   [media] exynos-gsc: Add hardware rotation limits


Thanks Hoegeung, I have applied patches 1 nad 4 from this series.

--
Regards,
Sylwester

Re: [PATCH v4 4/4] [media] exynos-gsc: Add hardware rotation limits

[Sylwester Nawrocki]

On 09/13/2017 01:41 PM, Hoegeun Kwon wrote:

@@ -1004,11 +1088,33 @@ static irqreturn_t gsc_irq_handler(int irq, void *priv)
.num_clocks = 1,
  };
  
+static struct gsc_driverdata gsc_v_5250_drvdata = {

+   .variant = {
+   [0] = _v_5250_variant,
+   [1] = _v_5250_variant,
+   [2] = _v_5250_variant,
+   [3] = _v_5250_variant,
+   },
+   .num_entities = 4,
+   .clk_names = { "gscl" },
+   .num_clocks = 1,
+};
+
+static struct gsc_driverdata gsc_v_5420_drvdata = {
+   .variant = {
+   [0] = _v_5420_variant,
+   [1] = _v_5420_variant,
+   },
+   .num_entities = 4,


Shouldn't num_enities be 2 here? You don't need to resend, I can
amend that when applying.


+   .clk_names = { "gscl" },
+   .num_clocks = 1,
+};
+


--
Regards,
Sylwester

Re: [PATCH] [media] s3c-camif: fix out-of-bounds array access

[Sylwester Nawrocki]

On 09/12/2017 10:09 PM, Arnd Bergmann wrote:
> While experimenting with older compiler versions, I ran
> into a warning that no longer shows up on gcc-4.8 or newer:
> 
> drivers/media/platform/s3c-camif/camif-capture.c: In function 
> '__camif_subdev_try_format':
> drivers/media/platform/s3c-camif/camif-capture.c:1265:25: error: array 
> subscript is below array bounds
> 
> This is an off-by-one bug, leading to an access before the start of the
> array, while newer compilers silently assume this undefined behavior
> cannot happen and leave the loop at index 0 if no other entry matches.
> 
> Since the code is not only wrong, but also has no effect besides the
> out-of-bounds access, this patch just removes it.
> 
> I found an existing gcc bug for it and added a reduced version
> of the function there.
> 
> Link: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=69249#c3
> Fixes: babde1c243b2 ("[media] V4L: Add driver for S3C24XX/S3C64XX SoC series 
> camera interface")
> Signed-off-by: Arnd Bergmann 
> ---
>   drivers/media/platform/s3c-camif/camif-capture.c | 7 ---
>   1 file changed, 7 deletions(-)
> 
> diff --git a/drivers/media/platform/s3c-camif/camif-capture.c 
> b/drivers/media/platform/s3c-camif/camif-capture.c
> index 25c7a7d42292..c6921f6a5a6a 100644
> --- a/drivers/media/platform/s3c-camif/camif-capture.c
> +++ b/drivers/media/platform/s3c-camif/camif-capture.c
> @@ -1256,17 +1256,10 @@ static void __camif_subdev_try_format(struct 
> camif_dev *camif,
>   {
>   const struct s3c_camif_variant *variant = camif->variant;
>   const struct vp_pix_limits *pix_lim;
> - int i = ARRAY_SIZE(camif_mbus_formats);
>   
>   /* FIXME: constraints against codec or preview path ? */
>   pix_lim = >vp_pix_limits[VP_CODEC];
>   
> - while (i-- >= 0)
> - if (camif_mbus_formats[i] == mf->code)
> - break;
> -
> - mf->code = camif_mbus_formats[i];


Interesting finding... the function needs to ensure mf->code is set 
to one of supported values by the driver, so instead of removing 
how about changing the above line to:

if (i < 0)
mf->code = camif_mbus_formats[0];

?

-- 
Thanks,
Sylwester

Re: [PATCH v3 4/6] [media] exynos-gsc: Add hardware rotation limits

[Sylwester Nawrocki]

Hi Hoegeun,

On 09/13/2017 04:33 AM, Hoegeun Kwon wrote:
>>> @@ -1017,8 +1083,12 @@ static irqreturn_t gsc_irq_handler(int irq,
>>> void *priv)
>>>  static const struct of_device_id exynos_gsc_match[] = {
>>>{
>>> -.compatible = "samsung,exynos5-gsc",
>>> -.data = _v_100_drvdata,
>> Can you keep the "samsung,exynos5-gsc" entry with the gsc_v_5250_variant
>> data, so that it can work with "samsung,exynos5-gsc" compatible in DT
>> on both exynos5250 and exynos5420 SoCs?
>>
> Thank you for your question.
> 
> Exynos 5250 and 5420 have different hardware rotation limits.
> Exynos 5250 is '.real_rot_en_w/h = 2016' and 5420 is '.real_rot_en_w/h =
> 2048'.
> 
> So my opinion they must have different compatible.

I think there is some misunderstanding, mu suggestion was to keep the 
"samsung,exynos5-gsc" compatible entry in addition to the new introduced 
ones: "samsung,exynos5250-gsc" and "samsung,exynos5420-gsc". That's in
order to make your changes possibly backward compatible, in theory 
the updated driver should still work without changes in dts.

-- 
Regards,
Sylwester

Re: [media] s5p-mfc: Adjust a null pointer check in four functions

[Sylwester Nawrocki]

On 09/12/2017 05:00 PM, SF Markus Elfring wrote:

* Do you care to preserve an information like the author date?

In this case not, but actually the Date line is not an issue.

Thanks for your information.

It seems then that you quoted a line too much.
 

Anyway the patch is malformed, …

>

I have got doubts for this view because the file was automatically generated
by the command “git format-patch” also for the discussed three update steps.


Generating patch is only part of the story, it seems the patch is not sent
properly and tags which should be in SMTP header end up in the message
body. I think there would not be such issues if you have used git format-patch
+ git send-email.

I normally do amend things like this while applying, I will do that this time 
as well. It's already too much time wasted for such a dubious patch.


--
Thanks,
Sylwester

Re: [media] s5p-mfc: Adjust a null pointer check in four functions

[Sylwester Nawrocki]

On 09/11/2017 09:21 PM, SF Markus Elfring wrote:

Date: Fri, 8 Sep 2017 22:37:00 +0200
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit


Can you resend with that 4 lines removed?


* Do you care to preserve an information like the author date?


In this case not, but actually the Date line is not an issue.  Anyway
the patch is malformed, please try to save your posted patch and apply
with git am and see how finally the commit message looks like.


* Would you like to support special characters in the commit message?


I can't see any need for special characters in the patch itself.
Please submit the patch in a way that it can be applied properly with
patchwork client (or git am).


Are you using git send-email for sending patches?


Not so far.


I would suggest switching to git send-email, then issues like
above could be easily avoided.

--
Regards,
Sylwester

Re: [PATCH v3 4/6] [media] exynos-gsc: Add hardware rotation limits

[Sylwester Nawrocki]

On 09/08/2017 08:02 AM, Hoegeun Kwon wrote:

The hardware rotation limits of gsc depends on SOC (Exynos
5250/5420/5433). Distinguish them and add them to the driver data.

Signed-off-by: Hoegeun Kwon 
---
  drivers/media/platform/exynos-gsc/gsc-core.c | 96 
  1 file changed, 83 insertions(+), 13 deletions(-)

diff --git a/drivers/media/platform/exynos-gsc/gsc-core.c 
b/drivers/media/platform/exynos-gsc/gsc-core.c
index 4380150..8f8636e 100644
--- a/drivers/media/platform/exynos-gsc/gsc-core.c
+++ b/drivers/media/platform/exynos-gsc/gsc-core.c
@@ -943,7 +943,37 @@ static irqreturn_t gsc_irq_handler(int irq, void *priv)
return IRQ_HANDLED;
  }
  
-static struct gsc_pix_max gsc_v_100_max = {

+static struct gsc_pix_max gsc_v_5250_max = {
+   .org_scaler_bypass_w= 8192,
+   .org_scaler_bypass_h= 8192,
+   .org_scaler_input_w = 4800,
+   .org_scaler_input_h = 3344,
+   .real_rot_dis_w = 4800,
+   .real_rot_dis_h = 3344,
+   .real_rot_en_w  = 2016,
+   .real_rot_en_h  = 2016,
+   .target_rot_dis_w   = 4800,
+   .target_rot_dis_h   = 3344,
+   .target_rot_en_w= 2016,
+   .target_rot_en_h= 2016,
+};
+
+static struct gsc_pix_max gsc_v_5420_max = {
+   .org_scaler_bypass_w= 8192,
+   .org_scaler_bypass_h= 8192,
+   .org_scaler_input_w = 4800,
+   .org_scaler_input_h = 3344,
+   .real_rot_dis_w = 4800,
+   .real_rot_dis_h = 3344,
+   .real_rot_en_w  = 2048,
+   .real_rot_en_h  = 2048,
+   .target_rot_dis_w   = 4800,
+   .target_rot_dis_h   = 3344,
+   .target_rot_en_w= 2016,
+   .target_rot_en_h= 2016,
+};
+
+static struct gsc_pix_max gsc_v_5433_max = {
.org_scaler_bypass_w= 8192,
.org_scaler_bypass_h= 8192,
.org_scaler_input_w = 4800,
@@ -979,8 +1009,8 @@ static irqreturn_t gsc_irq_handler(int irq, void *priv)
.target_h   = 2,  /* yuv420 : 2, others : 1 */
  };
  
-static struct gsc_variant gsc_v_100_variant = {

-   .pix_max= _v_100_max,
+static struct gsc_variant gsc_v_5250_variant = {
+   .pix_max= _v_5250_max,
.pix_min= _v_100_min,
.pix_align  = _v_100_align,
.in_buf_cnt = 32,
@@ -992,12 +1022,48 @@ static irqreturn_t gsc_irq_handler(int irq, void *priv)
.local_sc_down  = 2,
  };
  
-static struct gsc_driverdata gsc_v_100_drvdata = {

+static struct gsc_variant gsc_v_5420_variant = {
+   .pix_max= _v_5420_max,
+   .pix_min= _v_100_min,
+   .pix_align  = _v_100_align,
+   .in_buf_cnt = 32,
+   .out_buf_cnt= 32,
+   .sc_up_max  = 8,
+   .sc_down_max= 16,
+   .poly_sc_down_max   = 4,
+   .pre_sc_down_max= 4,
+   .local_sc_down  = 2,
+};
+
+static struct gsc_variant gsc_v_5433_variant = {
+   .pix_max= _v_5433_max,
+   .pix_min= _v_100_min,
+   .pix_align  = _v_100_align,
+   .in_buf_cnt = 32,
+   .out_buf_cnt= 32,
+   .sc_up_max  = 8,
+   .sc_down_max= 16,
+   .poly_sc_down_max   = 4,
+   .pre_sc_down_max= 4,
+   .local_sc_down  = 2,
+};
+
+static struct gsc_driverdata gsc_v_5250_drvdata = {
.variant = {
-   [0] = _v_100_variant,
-   [1] = _v_100_variant,
-   [2] = _v_100_variant,
-   [3] = _v_100_variant,
+   [0] = _v_5250_variant,
+   [1] = _v_5250_variant,
+   [2] = _v_5250_variant,
+   [3] = _v_5250_variant,
+   },
+   .num_entities = 4,
+   .clk_names = { "gscl" },
+   .num_clocks = 1,
+};
+
+static struct gsc_driverdata gsc_v_5420_drvdata = {
+   .variant = {
+   [0] = _v_5420_variant,
+   [1] = _v_5420_variant,
},
.num_entities = 4,
.clk_names = { "gscl" },
@@ -1006,9 +1072,9 @@ static irqreturn_t gsc_irq_handler(int irq, void *priv)
  
  static struct gsc_driverdata gsc_5433_drvdata = {

.variant = {
-   [0] = _v_100_variant,
-   [1] = _v_100_variant,
-   [2] = _v_100_variant,
+   [0] = _v_5433_variant,
+   [1] = _v_5433_variant,
+   [2] = _v_5433_variant,
},
.num_entities = 3,
.clk_names = { "pclk", "aclk", "aclk_xiu", "aclk_gsclbend" },
@@ -1017,8 +1083,12 @@ static irqreturn_t gsc_irq_handler(int irq, void *priv)
  
  static const struct of_device_id exynos_gsc_match[] = {

{
-   .compatible = "samsung,exynos5-gsc",
-   .data = 

Re: [PATCH 3/3] [media] s5p-mfc: Adjust a null pointer check in four functions

[Sylwester Nawrocki]

On 09/08/2017 10:53 PM, SF Markus Elfring wrote:

From: Markus Elfring 



Date: Fri, 8 Sep 2017 22:37:00 +0200
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit


Can you resend with that 4 lines removed? Are you using git send-email
for sending patches?

--
Thanks,
Sylwester

Re: [PATCH 2/3] [media] s5p-mfc: Improve a size determination in s5p_mfc_alloc_memdev()

[Sylwester Nawrocki]

On 09/08/2017 10:52 PM, SF Markus Elfring wrote:

Replace the specification of a data structure by a pointer dereference
as the parameter for the operator "sizeof" to make the corresponding size
determination a bit safer according to the Linux coding style convention.

This issue was detected by using the Coccinelle software.

Signed-off-by: Markus Elfring<elfr...@users.sourceforge.net>


Acked-by: Sylwester Nawrocki <s.nawro...@samsung.com>

Re: [PATCH 1/3] [media] s5p-mfc: Delete an error message for a failed memory allocation in s5p_mfc_probe()

[Sylwester Nawrocki]

On 09/08/2017 10:51 PM, SF Markus Elfring wrote:

Omit an extra message for a memory allocation failure in this function.

This issue was detected by using the Coccinelle software.

Signed-off-by: Markus Elfring<elfr...@users.sourceforge.net>


Could you make the commit summary shorter, to keep it below 70
characters [1]? With that changed feel free to add

Acked-by: Sylwester Nawrocki <s.nawro...@samsung.com>

--
Regards,
Sylwester

[1] Documentation/process/submitting-patches.rst

Re: [PATCH 3/5] s5p-cec: use CEC_CAP_DEFAULTS

[Sylwester Nawrocki]

On 08/04/2017 12:41 PM, Hans Verkuil wrote:


Use the new CEC_CAP_DEFAULTS define in the s5p-cec driver.

Signed-off-by: Hans Verkuil<hans.verk...@cisco.com>


Acked-by: Sylwester Nawrocki <s.nawro...@samsung.com>

Re: [PATCH 1/5] media/cec.h: add CEC_CAP_DEFAULTS

[Sylwester Nawrocki]

On 08/04/2017 12:41 PM, Hans Verkuil wrote:


The CEC_CAP_LOG_ADDRS, CEC_CAP_TRANSMIT, CEC_CAP_PASSTHROUGH and
CEC_CAP_RC capabilities are normally always present.

Add a CEC_CAP_DEFAULTS define that ORs these four caps to simplify
drivers.

Signed-off-by: Hans Verkuil<hans.verk...@cisco.com>


Reviewed-by: Sylwester Nawrocki <s.nawro...@samsung.com>

[GIT PULL] s5p-jpeg fixes for v4.14-rc1

[Sylwester Nawrocki]

Hi Mauro,

The following changes since commit ec0c3ec497cabbf3bfa03a9eb5edcc252190a4e0:

  media: ddbridge: split code into multiple files (2017-08-09 12:17:01 -0400)

are available in the git repository at:

  git://linuxtv.org/snawrocki/samsung.git for-v4.14/media/next-2

for you to fetch changes up to a1c60f2c60228a6c0d31e95ae4e65e6afd4655df:

  s5p-jpeg: directly use parsed subsampling on exynos5433 (2017-08-11 19:13:06 
+0200)


Andrzej Pietrasiewicz (5):
  s5p-jpeg: don't overwrite result's "size" member
  s5p-jpeg: set w/h when encoding
  s5p-jpeg: disable encoder/decoder in exynos4-like hardware after use
  s5p-jpeg: fix number of components macro
  s5p-jpeg: directly use parsed subsampling on exynos5433

Tony K Nadackal (2):
  s5p-jpeg: Fix crash in jpeg isr due to multiple interrupts.
  s5p-jpeg: Clear JPEG_CODEC_ON bits in sw reset function

 drivers/media/platform/s5p-jpeg/jpeg-core.c   | 18 ++
 drivers/media/platform/s5p-jpeg/jpeg-core.h   |  1 +
 drivers/media/platform/s5p-jpeg/jpeg-hw-exynos4.c |  9 -
 drivers/media/platform/s5p-jpeg/jpeg-regs.h   |  2 +-
 4 files changed, 24 insertions(+), 6 deletions(-)

-- 
Regards,
Sylwester

Re: [PATCH 4/5] media: platform: s5p-jpeg: fix number of components macro

[Sylwester Nawrocki]

On 08/08/2017 01:27 PM, Andrzej Pietrasiewicz wrote:

The value to be processed must be first masked and then shifted,
not the other way round.

Signed-off-by: Andrzej Pietrasiewicz 
---
  drivers/media/platform/s5p-jpeg/jpeg-regs.h | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/platform/s5p-jpeg/jpeg-regs.h 
b/drivers/media/platform/s5p-jpeg/jpeg-regs.h
index 1870400..df790b1 100644
--- a/drivers/media/platform/s5p-jpeg/jpeg-regs.h
+++ b/drivers/media/platform/s5p-jpeg/jpeg-regs.h
@@ -371,7 +371,7 @@
  #define EXYNOS4_NF_SHIFT  16
  #define EXYNOS4_NF_MASK   0xff
  #define EXYNOS4_NF(x) \
-   (((x) << EXYNOS4_NF_SHIFT) & EXYNOS4_NF_MASK)
+   (((x) & EXYNOS4_NF_MASK) << EXYNOS4_NF_SHIFT)


I'm going to add below tag when applying this patch.

Fixes: 6c96dbbc2aa9f5b4a ("[media] s5p-jpeg: add support for 5433")

--
Regards,
Sylwester

Re: [PATCH 5/6] [media] exynos4-is: constify video_subdev structures

[Sylwester Nawrocki]

On 08/08/2017 12:58 PM, Julia Lawall wrote:

The v4l2_subdev_ops structures are only passed as the second
argument of v4l2_subdev_init, which is const, so the
v4l2_subdev_ops structures can be const as well.

Done with the help of Coccinelle.

Signed-off-by: Julia Lawall<julia.law...@lip6.fr>


Reviewed-by: Sylwester Nawrocki <s.nawro...@samsung.com>

Re: [PATCH 03/12] [media] s5p-g2d: constify v4l2_m2m_ops structures

[Sylwester Nawrocki]

On 08/06/2017 10:25 AM, Julia Lawall wrote:

The v4l2_m2m_ops structures are only passed as the only
argument to v4l2_m2m_init, which is declared as const.
Thus the v4l2_m2m_ops structures themselves can be const.

Done with the help of Coccinelle.



Signed-off-by: Julia Lawall <julia.law...@lip6.fr>


Reviewed-by: Sylwester Nawrocki <s.nawro...@samsung.com>

Re: [PATCH 11/12] [media] exynos4-is: constify v4l2_m2m_ops structures

[Sylwester Nawrocki]

On 08/06/2017 10:25 AM, Julia Lawall wrote:

The v4l2_m2m_ops structures are only passed as the only
argument to v4l2_m2m_init, which is declared as const.
Thus the v4l2_m2m_ops structures themselves can be const.

Done with the help of Coccinelle.



Signed-off-by: Julia Lawall <julia.law...@lip6.fr>


Reviewed-by: Sylwester Nawrocki <s.nawro...@samsung.com>

Re: [PATCH 06/12] [media] exynos-gsc: constify v4l2_m2m_ops structures

[Sylwester Nawrocki]

On 08/06/2017 10:25 AM, Julia Lawall wrote:

The v4l2_m2m_ops structures are only passed as the only
argument to v4l2_m2m_init, which is declared as const.
Thus the v4l2_m2m_ops structures themselves can be const.

Done with the help of Coccinelle.



Signed-off-by: Julia Lawall<julia.law...@lip6.fr>


Reviewed-by: Sylwester Nawrocki <s.nawro...@samsung.com>

Re: [PATCHv2 5/5] media-device: remove driver_version

[Sylwester Nawrocki]

On 07/21/2017 12:57 PM, Hans Verkuil wrote:

From: Hans Verkuil 

Since the driver_version field in struct media_device is no longer
used, just remove it.

Signed-off-by: Hans Verkuil 
---
  drivers/media/media-device.c | 3 ---
  include/media/media-device.h | 2 --
  2 files changed, 5 deletions(-)



diff --git a/include/media/media-device.h b/include/media/media-device.h
index 6896266031b9..7d268802cc2e 100644
--- a/include/media/media-device.h
+++ b/include/media/media-device.h
@@ -68,7 +68,6 @@ struct media_device_ops {
   * @serial:   Device serial number (optional)
   * @bus_info: Unique and stable device location identifier
   * @hw_revision: Hardware device revision
- * @driver_version: Device driver version
   * @topology_version: Monotonic counter for storing the version of the graph
   *topology. Should be incremented each time the topology changes.
   * @id:   Unique ID used on the last registered graph object
@@ -134,7 +133,6 @@ struct media_device {
char serial[40];
char bus_info[32];
u32 hw_revision;
-   u32 driver_version;


It seems we still have such paragraph in include/media/media-device.h:

 *  - _entity.driver_version is formatted with the KERNEL_VERSION()
 *macro. The version minor must be incremented when new features are added
 *to the userspace API without breaking binary compatibility. The version
 *major must be incremented when binary compatibility is broken.

Shouldn't this also be removed?

--
Regards,
Sylwester

Re: [PATCHv2 5/5] media-device: remove driver_version

[Sylwester Nawrocki]

On 07/21/2017 12:57 PM, Hans Verkuil wrote:

From: Hans Verkuil <hans.verk...@cisco.com>

Since the driver_version field in struct media_device is no longer
used, just remove it.

Signed-off-by: Hans Verkuil <hans.verk...@cisco.com>


Reviewed-by: Sylwester Nawrocki <s.nawro...@samsung.com>

Re: [PATCHv2 2/5] s3c-camif: don't set driver_version

[Sylwester Nawrocki]

On 07/21/2017 12:57 PM, Hans Verkuil wrote:

From: Hans Verkuil<hans.verk...@cisco.com>

This field will be removed as it is not needed anymore.

Signed-off-by: Hans Verkuil<hans.verk...@cisco.com>


Reviewed-by: Sylwester Nawrocki <s.nawro...@samsung.com>

Re: [PATCH v2] media: Convert to using %pOF instead of full_name

[Sylwester Nawrocki]

On 07/21/2017 09:28 PM, Rob Herring wrote:

Now that we have a custom printf format specifier, convert users of
full_name to use %pOF instead. This is preparation to remove storing
of the full path string for each node.

Signed-off-by: Rob Herring<r...@kernel.org>
Acked-by: Niklas Söderlund<niklas.soderlund+rene...@ragnatech.se>
Acked-by: Laurent Pinchart<laurent.pinch...@ideasonboard.com>
Reviewed-by: Matthias Brugger<matthias@gmail.com>
Acked-by: Nicolas Ferre<nicolas.fe...@microchip.com>
Acked-by: Lad, Prabhakar<prabhakar.cse...@gmail.com>



---
v2:
- Fix missing to_of_node() in xilinx-vipp.c
- Drop v4l2-async.c changes. Doing as revert instead.
- Add acks


Reviewed-by: Sylwester Nawrocki <s.nawro...@samsung.com>

[GIT PULL FOR 4.14] Samsung SoC related updates

[Sylwester Nawrocki]

Hi Mauro,

The following changes since commit 6538b02d210f52ef2a2e67d59fcb58be98451fbd:

  media: Make parameter of media_entity_remote_pad() const (2017-07-20 16:54:04 
-0400)

are available in the git repository at:

  git://linuxtv.org/snawrocki/samsung.git for-v4.14/media/next

for you to fetch changes up to c7782331ca78c9b84485051365c1aaceac6c634c:

  exynos4-is: fimc-is-i2c: constify dev_pm_ops structures. (2017-07-21 13:22:40 
+0200)


Arvind Yadav (1):
  exynos4-is: fimc-is-i2c: constify dev_pm_ops structures.

Gustavo A. R. Silva (1):
  s5k5baf: remove unnecessary static in s5k5baf_get_selection()

Thierry Escande (3):
  s5p-jpeg: Handle parsing error in s5p_jpeg_parse_hdr()
  s5p-jpeg: Don't use temporary structure in s5p_jpeg_buf_queue
  s5p-jpeg: Split s5p_jpeg_parse_hdr()

Tony K Nadackal (3):
  s5p-jpeg: Call jpeg_bound_align_image after qbuf
  s5p-jpeg: Correct WARN_ON statement for checking subsampling
  s5p-jpeg: Decode 4:1:1 chroma subsampling format

henryhsu (2):
  s5p-jpeg: Add support for resolution change event
  s5p-jpeg: Add stream error handling for Exynos5420

 drivers/media/i2c/s5k5baf.c  |   2 +-
 .../media/platform/exynos4-is/fimc-is-i2c.c  |   2 +-
 drivers/media/platform/s5p-jpeg/jpeg-core.c  | 186 
 drivers/media/platform/s5p-jpeg/jpeg-core.h  |   7 +
 4 files changed, 150 insertions(+), 47 deletions(-)

-- 
Thanks,
Sylwester

Re: [PATCH] media: Convert to using %pOF instead of full_name

[Sylwester Nawrocki]

On 07/19/2017 06:02 PM, Rob Herring wrote:

diff --git a/drivers/media/v4l2-core/v4l2-async.c 
b/drivers/media/v4l2-core/v4l2-async.c
index 851f128eba22..0a385d1ff28c 100644
--- a/drivers/media/v4l2-core/v4l2-async.c
+++ b/drivers/media/v4l2-core/v4l2-async.c
@@ -47,9 +47,7 @@ static bool match_fwnode(struct v4l2_subdev *sd, struct 
v4l2_async_subdev *asd)
   if (!is_of_node(sd->fwnode) || !is_of_node(asd->match.fwnode.fwnode))
   return sd->fwnode == asd->match.fwnode.fwnode;

- return !of_node_cmp(of_node_full_name(to_of_node(sd->fwnode)),
- of_node_full_name(
- to_of_node(asd->match.fwnode.fwnode)));
+ return to_of_node(sd->fwnode) == to_of_node(asd->match.fwnode.fwnode);

>>

I'm afraid this will not work, please see commit d2180e0cf77dc7a7049671d5d57d
"[media] v4l: async: make v4l2 coexist with devicetree nodes in a dt overlay"

>

Maybe I'm missing something, but how does that work exactly? Before
the overlay is applied, the remote endpoint node (and its parent)
can't be resolved. In the commit example, the endpoint in the
media_bridge would also have to be part of the overlay. If you apply
and un-apply the overlay, then the of_node (and fw_node) in the
overlay is once again invalid. IOW, you should be in the same state as
before the overlay was applied. The node is still around because of
paranoia that actually freeing nodes would break things. It seems this
paranoia is real, so i think we need to do something to prevent this
from spreading.

Furthermore, it does not appear that any media driver supports
overlays and we have no general way to apply them in mainline yet
(other than an in kernel API). So really this scenario is not one we
have to support yet.


Indeed, the motivation of the above mentioned commit was some out of
tree driver. I don't know was the exact use case, assuming that the
endpoint in the bridge node was also part of the overlay the bridge
driver must have not been rescanning device tree after overlay un-apply
and apply. Currently there is no other way to do this than to unbind
and bind. So the bridge driver must have been referencing an already
invalid node as you point out.

I haven't been following DT overlays very closely, as Frank explains
your change seems to be actually an improvement of current code.

--
Thanks,
Sylwester

Re: [PATCH] media: Convert to using %pOF instead of full_name

[Sylwester Nawrocki]

On 07/18/2017 11:43 PM, Rob Herring wrote:
> Now that we have a custom printf format specifier, convert users of
> full_name to use %pOF instead. This is preparation to remove storing
> of the full path string for each node.
> 
> Signed-off-by: Rob Herring 

> ---
>   drivers/media/i2c/s5c73m3/s5c73m3-core.c   |  3 +-
>   drivers/media/i2c/s5k5baf.c|  7 ++--
>   drivers/media/platform/am437x/am437x-vpfe.c|  4 +-
>   drivers/media/platform/atmel/atmel-isc.c   |  4 +-
>   drivers/media/platform/davinci/vpif_capture.c  | 16 
>   drivers/media/platform/exynos4-is/fimc-is.c|  8 ++--
>   drivers/media/platform/exynos4-is/fimc-lite.c  |  3 +-
>   drivers/media/platform/exynos4-is/media-dev.c  |  8 ++--
>   drivers/media/platform/exynos4-is/mipi-csis.c  |  4 +-
>   drivers/media/platform/mtk-mdp/mtk_mdp_comp.c  |  6 +--
>   drivers/media/platform/mtk-mdp/mtk_mdp_core.c  |  8 ++--
>   drivers/media/platform/omap3isp/isp.c  |  8 ++--
>   drivers/media/platform/pxa_camera.c|  2 +-
>   drivers/media/platform/rcar-vin/rcar-core.c|  4 +-
>   drivers/media/platform/soc_camera/soc_camera.c |  6 +--
>   drivers/media/platform/xilinx/xilinx-vipp.c| 52 
> +-
>   drivers/media/v4l2-core/v4l2-async.c   |  4 +-
>   drivers/media/v4l2-core/v4l2-clk.c |  3 +-
>   include/media/v4l2-clk.h   |  4 +-
>   19 files changed, 71 insertions(+), 83 deletions(-)

> diff --git a/drivers/media/platform/xilinx/xilinx-vipp.c 
> b/drivers/media/platform/xilinx/xilinx-vipp.c
> index ac4704388920..9233ad0b1b6b 100644
> --- a/drivers/media/platform/xilinx/xilinx-vipp.c
> +++ b/drivers/media/platform/xilinx/xilinx-vipp.c

> @@ -144,9 +144,8 @@ static int xvip_graph_build_one(struct 
> xvip_composite_device *xdev,
>   remote = ent->entity;
> 
>   if (link.remote_port >= remote->num_pads) {
> - dev_err(xdev->dev, "invalid port number %u on %s\n",
> - link.remote_port,
> - to_of_node(link.remote_node)->full_name);
> + dev_err(xdev->dev, "invalid port number %u on %pOF\n",
> + link.remote_port, link.remote_node);

Shouldn't there be to_of_node(link.remote_node) instead of link.remote_node ?

>   v4l2_fwnode_put_link();
>   ret = -EINVAL;
>   break;

> @@ -242,17 +241,17 @@ static int xvip_graph_build_dma(struct 
> xvip_composite_device *xdev)
>   ent = xvip_graph_find_entity(xdev,
>to_of_node(link.remote_node));
>   if (ent == NULL) {
> - dev_err(xdev->dev, "no entity found for %s\n",
> - to_of_node(link.remote_node)->full_name);
> + dev_err(xdev->dev, "no entity found for %pOF\n",
> + to_of_node(link.remote_node));
>   v4l2_fwnode_put_link();
>   ret = -ENODEV;
>   break;
>   }

> diff --git a/drivers/media/v4l2-core/v4l2-async.c 
> b/drivers/media/v4l2-core/v4l2-async.c
> index 851f128eba22..0a385d1ff28c 100644
> --- a/drivers/media/v4l2-core/v4l2-async.c
> +++ b/drivers/media/v4l2-core/v4l2-async.c
> @@ -47,9 +47,7 @@ static bool match_fwnode(struct v4l2_subdev *sd, struct 
> v4l2_async_subdev *asd)
>   if (!is_of_node(sd->fwnode) || !is_of_node(asd->match.fwnode.fwnode))
>   return sd->fwnode == asd->match.fwnode.fwnode;
> 
> - return !of_node_cmp(of_node_full_name(to_of_node(sd->fwnode)),
> - of_node_full_name(
> - to_of_node(asd->match.fwnode.fwnode)));
> + return to_of_node(sd->fwnode) == to_of_node(asd->match.fwnode.fwnode);

I'm afraid this will not work, please see commit d2180e0cf77dc7a7049671d5d57d
"[media] v4l: async: make v4l2 coexist with devicetree nodes in a dt overlay"

--
Regards,
Sylwester

Re: [PATCH v2 0/7] [PATCH v2 0/7] Add support of OV9655 camera

[Sylwester Nawrocki]

Hi Hugues,

On 07/03/2017 11:16 AM, Hugues Fruchet wrote:
> This patchset enables OV9655 camera support.
> 
> OV9655 support has been tested using STM32F4DIS-CAM extension board
> plugged on connector P1 of STM32F746G-DISCO board.
> Due to lack of OV9650/52 hardware support, the modified related code
> could not have been checked for non-regression.
> 
> First patches upgrade current support of OV9650/52 to prepare then
> introduction of OV9655 variant patch.
> Because of OV9655 register set slightly different from OV9650/9652,
> not all of the driver features are supported (controls). Supported
> resolutions are limited to VGA, QVGA, QQVGA.
> Supported format is limited to RGB565.
> Controls are limited to color bar test pattern for test purpose.

I appreciate your efforts towards making a common driver but IMO it would be 
better to create a separate driver for the OV9655 sensor.  The original driver 
is 1576 lines of code, your patch set adds half of that (816).  There are
significant differences in the feature set of both sensors, there are 
differences in the register layout.  I would go for a separate driver, we  
would then have code easier to follow and wouldn't need to worry about possible
regressions.  I'm afraid I have lost the camera module and won't be able 
to test the patch set against regressions.

IMHO from maintenance POV it's better to make a separate driver. In the end 
of the day we wouldn't be adding much more code than it is being done now.

>   .../devicetree/bindings/media/i2c/ov965x.txt   |  45 ++
>   drivers/media/i2c/Kconfig  |   6 +-
>   drivers/media/i2c/ov9650.c | 816 
> +
>   3 files changed, 736 insertions(+), 131 deletions(-)
>   create mode 100644 Documentation/devicetree/bindings/media/i2c/ov965x.txt

--
Thanks,
Sylwester

Re: [PATCH v2 3/7] [media] ov9650: add device tree support

[Sylwester Nawrocki]

On 07/03/2017 11:16 AM, Hugues Fruchet wrote:
> Allows use of device tree configuration data.
> If no device tree data is there, configuration is taken from platform data.
> In order to keep GPIOs configuration compatible between both way of doing,
> GPIOs are switched to descriptor-based interface.
> 
> Signed-off-by: H. Nikolaus Schaller 
> Signed-off-by: Hugues Fruchet 
> ---
>   drivers/media/i2c/Kconfig  |  2 +-
>   drivers/media/i2c/ov9650.c | 77 
> ++
>   2 files changed, 59 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
> index 121b3b5..168115c 100644
> --- a/drivers/media/i2c/Kconfig
> +++ b/drivers/media/i2c/Kconfig
> @@ -615,7 +615,7 @@ config VIDEO_OV7670
>   
>   config VIDEO_OV9650
>   tristate "OmniVision OV9650/OV9652 sensor support"
> - depends on I2C && VIDEO_V4L2 && VIDEO_V4L2_SUBDEV_API
> + depends on GPIOLIB && I2C && VIDEO_V4L2 && VIDEO_V4L2_SUBDEV_API
>   ---help---
> This is a V4L2 sensor-level driver for the Omnivision
> OV9650 and OV9652 camera sensors.
> diff --git a/drivers/media/i2c/ov9650.c b/drivers/media/i2c/ov9650.c
> index 1e4e99e..7e9a902 100644
> --- a/drivers/media/i2c/ov9650.c
> +++ b/drivers/media/i2c/ov9650.c
> @@ -11,12 +11,14 @@
>* it under the terms of the GNU General Public License version 2 as
>* published by the Free Software Foundation.
>*/
> +#include 
>   #include 
>   #include 
>   #include 
>   #include 
>   #include 
>   #include 
> +#include 
>   #include 
>   #include 
>   #include 
> @@ -249,9 +251,10 @@ struct ov965x {
>   struct v4l2_subdev sd;
>   struct media_pad pad;
>   enum v4l2_mbus_type bus_type;
> - int gpios[NUM_GPIOS];
> + struct gpio_desc *gpios[NUM_GPIOS];
>   /* External master clock frequency */
>   unsigned long mclk_frequency;
> + struct clk *clk;
>   
>   /* Protects the struct fields below */
>   struct mutex lock;
> @@ -511,10 +514,10 @@ static int ov965x_set_color_matrix(struct ov965x 
> *ov965x)
>   return 0;
>   }
>   
> -static void ov965x_gpio_set(int gpio, int val)
> +static void ov965x_gpio_set(struct gpio_desc *gpio, int val)
>   {
> - if (gpio_is_valid(gpio))
> - gpio_set_value(gpio, val);
> + if (gpio)
> + gpiod_set_value_cansleep(gpio, val);
>   }
>   
>   static void __ov965x_set_power(struct ov965x *ov965x, int on)
> @@ -1406,24 +1409,28 @@ static int ov965x_configure_gpios(struct ov965x 
> *ov965x,
> const struct ov9650_platform_data *pdata)
>   {
>   int ret, i;
> + int gpios[NUM_GPIOS];
>   
> - ov965x->gpios[GPIO_PWDN] = pdata->gpio_pwdn;
> - ov965x->gpios[GPIO_RST]  = pdata->gpio_reset;
> + gpios[GPIO_PWDN] = pdata->gpio_pwdn;
> + gpios[GPIO_RST]  = pdata->gpio_reset;
>   
> - for (i = 0; i < ARRAY_SIZE(ov965x->gpios); i++) {
> - int gpio = ov965x->gpios[i];
> + for (i = 0; i < ARRAY_SIZE(gpios); i++) {
> + int gpio = gpios[i];
>   
>   if (!gpio_is_valid(gpio))
>   continue;
>   ret = devm_gpio_request_one(>client->dev, gpio,
> - GPIOF_OUT_INIT_HIGH, "OV965X");
> - if (ret < 0)
> + GPIOF_OUT_INIT_HIGH, DRIVER_NAME);
> + if (ret < 0) {
> + dev_err(>client->dev,
> + "Failed to request gpio%d (%d)\n", gpio, ret);
>   return ret;
> + }
>   v4l2_dbg(1, debug, >sd, "set gpio %d to 1\n", gpio);
>   
>   gpio_set_value(gpio, 1);
>   gpio_export(gpio, 0);
> - ov965x->gpios[i] = gpio;
> + ov965x->gpios[i] = gpio_to_desc(gpio);
>   }
>   
>   return 0;
> @@ -1469,14 +1476,10 @@ static int ov965x_probe(struct i2c_client *client,
>   struct v4l2_subdev *sd;
>   struct ov965x *ov965x;
>   int ret;
> + struct device_node *np = client->dev.of_node;
>   
> - if (pdata == NULL) {
> - dev_err(>dev, "platform data not specified\n");
> - return -EINVAL;
> - }
> -
> - if (pdata->mclk_frequency == 0) {
> - dev_err(>dev, "MCLK frequency not specified\n");
> + if (!pdata && !np) {
> + dev_err(>dev, "Platform data or device tree data must 
> be provided\n");
>   return -EINVAL;
>   }
>   
> @@ -1486,7 +1489,35 @@ static int ov965x_probe(struct i2c_client *client,
>   
>   mutex_init(>lock);
>   ov965x->client = client;
> - ov965x->mclk_frequency = pdata->mclk_frequency;
> + mutex_init(>lock);

Are you initializing the mutex twice?

> + if (np) {
> + /* Device tree */
> + ov965x->gpios[GPIO_RST] =
> + devm_gpiod_get_optional(>dev, "resetb",
> +

Re: [PATCH v2 2/7] [media] ov9650: switch i2c device id to lower case

[Sylwester Nawrocki]

On 07/03/2017 11:16 AM, Hugues Fruchet wrote:
> Switch i2c device id to lower case as it is

s/i2c/I2C ?

> done for other omnivision cameras.

s/omnivision/Omnivision

This is required for properly matching driver with device on DT platforms,
right? It might be worth to mention that so it is clear why we break any
non-dt platform that could be already using this driver. There seem to be 
none in the mainline kernel tree though.

> Signed-off-by: Hugues Fruchet <hugues.fruc...@st.com>

Reviewed-by: Sylwester Nawrocki <snawro...@kernel.org>

> Signed-off-by: Hugues Fruchet <hugues.fruc...@st.com>
> ---
>   drivers/media/i2c/ov9650.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/i2c/ov9650.c b/drivers/media/i2c/ov9650.c
> index 2de2fbb..1e4e99e 100644
> --- a/drivers/media/i2c/ov9650.c
> +++ b/drivers/media/i2c/ov9650.c
> @@ -1545,8 +1545,8 @@ static int ov965x_remove(struct i2c_client *client)
>   }
>   
>   static const struct i2c_device_id ov965x_id[] = {
> - { "OV9650", 0 },
> - { "OV9652", 0 },
> + { "ov9650", 0 },
> + { "ov9652", 0 },
>   { /* sentinel */ }
>   };
>   MODULE_DEVICE_TABLE(i2c, ov965x_id);
 

Re: [PATCH v2 0/7] [PATCH v2 0/7] Add support of OV9655 camera

[Sylwester Nawrocki]

Hi Hugues,

On 07/06/2017 09:51 AM, Hugues FRUCHET wrote:

Hi Sylwester,

Do you have the possibility to check for non-regression of this patchset
on 9650/52 camera ?


I will try to test your patch set once I find the camera module for
my Micro2440SDK board. I've spent already a day on setting up everything 
and fixing multiple regressions in the kernel. I will likely try your 
patch series in coming week.


--
Thanks,
Sylwester

Re: [PATCH v6 4/4] dt-bindings: media: Document Synopsys Designware HDMI RX

[Sylwester Nawrocki]

On 07/06/2017 12:24 PM, Jose Abreu wrote:
>>> +- edid-phandle: phandle to the EDID handler block.
>>
>> Could you make this property optional and when it is missing assume that 
>> device
>> corresponding to the parent node of this node handles EDID? This way we could
>> avoid having property pointing to the parent node.
>
> Hmm, this is for the CEC notifier. Do you mean I should grab the
> parent device for the notifier? This property is already optional
> if cec is not enabled though.
 
Yes, device associated with the parent node. Something like:

 - edid-phandle - phandle to the EDID handler block; if this property is
  not specified it is assumed that EDID is handled by device described 
  by parent node of the HDMI RX node

Not sure if it is any better than always requiring edid-phandle property,
even when it is pointing to the parent node. We would need a DT maintainer's
opinion on that.

--
Thanks,
Sylwester

Re: [PATCH v6 4/4] dt-bindings: media: Document Synopsys Designware HDMI RX

[Sylwester Nawrocki]

On 07/04/2017 04:11 PM, Jose Abreu wrote:
> Document the bindings for the Synopsys Designware HDMI RX.
> 
> Signed-off-by: Jose Abreu 

> ---
>   .../devicetree/bindings/media/snps,dw-hdmi-rx.txt  | 70 
> ++
>   1 file changed, 70 insertions(+)
>   create mode 100644 
> Documentation/devicetree/bindings/media/snps,dw-hdmi-rx.txt

Could you make the DT binding documentation patch first patch in the series?
Now checkpatch will complain about undocumented compatible string when 
the driver patches are applied alone.

> diff --git a/Documentation/devicetree/bindings/media/snps,dw-hdmi-rx.txt 
>b/Documentation/devicetree/bindings/media/snps,dw-hdmi-rx.txt
> new file mode 100644
> index 000..449b8a2
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/snps,dw-hdmi-rx.txt
> @@ -0,0 +1,70 @@
> +Synopsys DesignWare HDMI RX Decoder
> +===
> +
> +This document defines device tree properties for the Synopsys DesignWare HDMI
> +RX Decoder (DWC HDMI RX). It doesn't constitute a device tree binding
> +specification by itself but is meant to be referenced by platform-specific
> +device tree bindings.
> +
> +When referenced from platform device tree bindings the properties defined in
> +this document are defined as follows.

It would be good to make it clear which properties are required and which are
optional. And also to mention the properties below belong to the HDMI RX node.

> +- compatible: Shall be "snps,dw-hdmi-rx".
> +
> +- reg: Memory mapped base address and length of the DWC HDMI RX registers.
> +
> +- interrupts: Reference to the DWC HDMI RX interrupt and 5v sense interrupt.

s/5v/HDMI 5V ?

> +
> +- clocks: Phandle to the config clock block.
> +
> +- clock-names: Shall be "cfg".
> +
> +- edid-phandle: phandle to the EDID handler block.

Could you make this property optional and when it is missing assume that device
corresponding to the parent node of this node handles EDID? This way we could
avoid having property pointing to the parent node.

> +- #address-cells: Shall be 1.
> +
> +- #size-cells: Shall be 0.
> +
> +You also have to create a subnode for phy driver. Phy properties are as 
> follows.

s/phy driver. Phy/the PHY device. PHY ?

Might be also worth to make it explicit these are all required properties.

> +- compatible: Shall be "snps,dw-hdmi-phy-e405".
> +
> +- reg: Shall be JTAG address of phy.

s/phy/the PHY ?

> +- clocks: Phandle for cfg clock.
> +
> +- clock-names:Shall be "cfg".
> +
> +A sample binding is now provided. The compatible string is for a SoC which 
> has
> +has a Synopsys DesignWare HDMI RX decoder inside.
> +
> +Example:
> +
> +dw_hdmi_soc: dw-hdmi-soc@0 {
> + compatible = "snps,dw-hdmi-soc";

Perhaps just make it

compatible = "...";
?

> + reg = <0x11c00 0x1000>; /* EDIDs */

This is not relevant and undocumented, will likely be part of documentation 
of other binding thus I'd suggest dropping this reg property.

> + #address-cells = <1>;
> + #size-cells = <1>;
> + ranges;
> +
> + hdmi-rx@0 {
> + compatible = "snps,dw-hdmi-rx";
> + reg = <0x0 0x1>;
> + interrupts = <1 2>;
> + edid-phandle = <_hdmi_soc>;
> +
> + clocks = <_hdmi_refclk>;
> + clock-names = "cfg";
> +
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + hdmi-phy@fc {
> + compatible = "snps,dw-hdmi-phy-e405";
> + reg = <0xfc>;
> +
> + clocks = <_hdmi_refclk>;
> + clock-names = "cfg";
> + };
> + };
> +};

Otherwise looks good. I'll likely not have comments to the other patches.

--
Regards,
Sylwester
 

Re: [PATCH v2 3/3] [media] intel-ipu3: cio2: Add new MIPI-CSI2 driver

[Sylwester Nawrocki]

Hi,

On 06/28/2017 03:31 PM, Sakari Ailus wrote:

IMO VB2/V4L2 could better support conversion between single and
multi-planar buffer types so that the applications could just use any and
drivers could manage with one.

I don't have a strong opinion either way, but IMO this could be well
addressed later on by improving the framework when (or if) the support for
formats such as NV12 is added.


We had already conversion between single and multi-planar buffer types
in the kernel.  But for some reasons it got removed. [1] The conversion
is supposed to be done in libv4l2, which is not mandatory so it cannot
be used to ensure backward compatibility while moving driver from one
API to the other.

[1]
commit 1d0c86cad38678fa42f6d048a7b9e4057c8c16fc
[media] media: v4l: remove single to multiplane conversion

Regards,
Sylwester

Re: [PATCH v1 1/6] DT bindings: add bindings for ov965x camera module

[Sylwester Nawrocki]

On 06/28/2017 11:12 AM, H. Nikolaus Schaller wrote:
>> Am 28.06.2017 um 00:57 schrieb Sylwester Nawrocki <snawro...@kernel.org>:
>> On 06/27/2017 07:48 AM, H. Nikolaus Schaller wrote:
>>>> Am 26.06.2017 um 22:04 schrieb Sylwester Nawrocki <snawro...@kernel.org>:
>>>> On 06/26/2017 12:35 PM, Hugues FRUCHET wrote:
>>>>>> What I am missing to support the GTA04 camera is the control of the 
>>>>>> optional "vana-supply".
>>>>>> So the driver does not power up the camera module when needed and 
>>>>>> therefore probing fails.
>>>>>>
>>>>>> - vana-supply: a regulator to power up the camera module.
>>>>>>
>>>>>> Driver code is not complex to add:
>>>>
>>>>> Yes, I saw it in your code, but as I don't have any programmable power
>>>>> supply on my setup, I have not pushed this commit.
>>>>
>>>> Since you are about to add voltage supplies to the DT binding I'd suggest
>>>> to include all three voltage supplies of the sensor chip. Looking at the 
>>>> OV9650
>>>> and the OV9655 datasheet there are following names used for the voltage 
>>>> supply
>>>> pins:
>>>>
>>>> AVDD - Analog power supply,
>>>> DVDD - Power supply for digital core logic,
>>>> DOVDD - Digital power supply for I/O.
>>>
>>> The latter two are usually not independently switchable from the SoC power
>>> the module is connected to.
>>>
>>> And sometimes DVDD and DOVDD are connected together.
>>>
>>> So the driver can't make much use of knowing or requesting them because the
>>> 1.8V supply is always active, even during suspend.
>>>
>>>>
>>>> I doubt the sensor can work without any of these voltage supplies, thus
>>>> regulator_get_optional() should not be used. I would just use the regulator
>>>> bulk API to handle all three power supplies.
>>>
>>> The digital part works with AVDD turned off. So the LDO supplying AVDD 
>>> should
>>> be switchable to save power ( on the GTA04 device).>
>>> But not all designs can switch it off. Hence the idea to define it as an
>>> /optional/ regulator. If it is not defined by DT, the driver simply assumes
>>> it is always powered on.
>>
>> I didn't say we can't define regulator supply properties as optional in the 
>> DT
>> binding.  If we define them as such and any of these *-supply properties is
>> missing in DT with regulator_get() the regulator core will use dummy 
>> regulator
>> for that particular voltage supply.  While with regulator_get_optional()
>> -ENODEV is returned when the regulator cannot be found.
> 
> Ah, ok. I see.
> 
> I had thought that it is the right thing to do like devm_gpiod_get_optional().
> 
> That one it is described as:
> 
> "* This is equivalent to gpiod_get(), except that when no GPIO was assigned to
>   * the requested function it will return NULL. This is convenient for drivers
>   * that need to handle optional GPIOs."
> 
> Seems to be inconsistent definition of what "optional" means.

Indeed, this commit explains it further:

commit de1dd9fd2156874b45803299b3b27e65d5defdd9
regulator: core: Provide hints to the core about optional supplies

> So we indeed should use devm_regulator_get() in this case. Thanks for > 
> pointing out!

>>> So in summary we only need AVDD switched for the GTA04 - but it does not
>>> matter if the others are optional properties. We would not use them.
>>>
>>> It does matter if they are mandatory because it adds DT complexity (size
>>> and processing) without added function.
>>
>> We should not be defining DT binding only with selected use cases/board
>> designs in mind.  IMO all three voltage supplies should be listed in the
>> binding, presumably all can be made optional, with an assumption that when
>> the property is missing selected pin is hooked up to a fixed regulator.
> 
> Ok, then it should just be defined in the bindings but not used by 
> the driver?

Yes, I think so. So we have a possibly complete binding right from the 
beginning. I someone needs handling other supplies than AVDD they could
update the driver in future.


Regards,
Sylwester

Re: [PATCH v4 2/4] [media] platform: Add Synopsys Designware HDMI RX Controller Driver

[Sylwester Nawrocki]

Hi Jose,

On 06/28/2017 11:25 AM, Jose Abreu wrote:

On 27-06-2017 21:34, Sylwester Nawrocki wrote:

On 06/27/2017 10:43 AM, Jose Abreu wrote:

On 25-06-2017 22:13, Sylwester Nawrocki wrote:

On 06/20/2017 07:26 PM, Jose Abreu wrote:

This is an initial submission for the Synopsys Designware HDMI RX
Controller Driver. This driver interacts with a phy driver so that
a communication between them is created and a video pipeline is
configured.
Signed-off-by: Jose Abreu <joab...@synopsys.com>



The modules are installed but I think I don't have udev :/ I'm
running this on an embedded platform called ARC AXS and I'm using
buildroot with minimal options.


OK, then let's keep this request_module.


+static int dw_hdmi_v4l2_notify_complete(struct v4l2_async_notifier *notifier)
+{
+   struct dw_hdmi_dev *dw_dev = notifier_to_dw_dev(notifier);
+   int ret;
+
+   ret = v4l2_device_register_subdev_nodes(_dev->v4l2_dev);

There shouldn't be multiple struct v4l2_device instances, instead we should
have only one created by the main driver. AFAIU, in your case it would be
driver associated with the dw-hdmi-soc DT node.  And normally such a top level
driver creates subdev device nodes when its all required sub-devices are
available.

I think this patch could be useful for you:
https://urldefense.proofpoint.com/v2/url?u=https-3A__patchwork.linuxtv.
org_patch_41834=DwICaQ=DPL6_X_6JkXFx7AXWqB0tg=WHDsc6kcWAl4i96Vm5hJ_19I
Jiuxx_p_Rzo2g-uHDKw=uNHRQkbP_-z8v5d30KFx9pcPEUhlr4ciWY3ZDAVExTA=dB9wpgeP
7AJg1eDRty0-RKhq3DY-7J5srIzyVoJey5I=

With that the dw-hdmi-soc driver would have it's v4l2-async notifier's
notify_complete() callback called only when both the hdmi-rx and the
hdmi-phy subdevs are registered.

Yeah, I saw the patches. I just implemented this way because they
are not merged yet, right?

>>

I think these patches will be merged in v4.14-rc1, so together with your driver.
You could apply them locally and indicate that your series depends on them in
the cover letter.


Ok, will apply them locally and re-test.


Thanks.


+   if (ret) {
+   dev_err(dw_dev->dev, "failed to register subdev nodes\n");
+   return ret;
+   }
+
+   return 0;
+}
+static int dw_hdmi_rx_probe(struct platform_device *pdev)
+{
+   /* V4L2 initialization */
+   sd = _dev->sd;
+   v4l2_subdev_init(sd, _hdmi_sd_ops);
+   strlcpy(sd->name, dev_name(dev), sizeof(sd->name));
+   sd->dev = dev;
+   sd->internal_ops = _hdmi_internal_ops;
+   sd->flags |= V4L2_SUBDEV_FL_HAS_EVENTS;

>>>>

Don't you also need V4L2_SUBDEV_FL_HAS_DEVNODE flag set?

Ouch. Yes I need otherwise the subdev will not be associated with
the v4l2_device.

>>

This flag indicates that the v4l2 subdev device node (/dev/v4l-subdev?)
should be created for this subdevice.


Ok, will add for controller driver only then: I think for phy
this should not be added because controller is responsible to
manage phy entirely so creating a /dev/ which can be seen by
userspace can lead to confusion, maybe?


Right, there should be no need for the PHY. It's up to you to
set it or not for the RX controller subdev. I just got confused
because in your dw_hdmi_sd_ops data structure there are ops which
would suggest that device node is used.


Regards,
Sylwester

Re: [PATCH 2/3] media: ti-vpe: cal: use of_graph_get_remote_endpoint()

[Sylwester Nawrocki]

On 06/28/2017 02:33 AM, Kuninori Morimoto wrote:

From: Kuninori Morimoto <kuninori.morimoto...@renesas.com>

Now, we can use of_graph_get_remote_endpoint(). Let's use it.

Signed-off-by: Kuninori Morimoto<kuninori.morimoto...@renesas.com>


Reviewed-by: Sylwester Nawrocki <s.nawro...@samsung.com>

Re: [PATCH v1 1/6] DT bindings: add bindings for ov965x camera module

[Sylwester Nawrocki]

On 06/27/2017 07:48 AM, H. Nikolaus Schaller wrote:
>> Am 26.06.2017 um 22:04 schrieb Sylwester Nawrocki <snawro...@kernel.org>:
>>
>> On 06/26/2017 12:35 PM, Hugues FRUCHET wrote:
>>>> What I am missing to support the GTA04 camera is the control of the 
>>>> optional "vana-supply".
>>>> So the driver does not power up the camera module when needed and 
>>>> therefore probing fails.
>>>>
>>>> - vana-supply: a regulator to power up the camera module.
>>>>
>>>> Driver code is not complex to add:
>>
>>> Yes, I saw it in your code, but as I don't have any programmable power
>>> supply on my setup, I have not pushed this commit.
>>
>> Since you are about to add voltage supplies to the DT binding I'd suggest
>> to include all three voltage supplies of the sensor chip. Looking at the 
>> OV9650
>> and the OV9655 datasheet there are following names used for the voltage 
>> supply
>> pins:
>>
>> AVDD - Analog power supply,
>> DVDD - Power supply for digital core logic,
>> DOVDD - Digital power supply for I/O.
> 
> The latter two are usually not independently switchable from the SoC power
> the module is connected to.
> 
> And sometimes DVDD and DOVDD are connected together.
> 
> So the driver can't make much use of knowing or requesting them because the
> 1.8V supply is always active, even during suspend.
> 
>>
>> I doubt the sensor can work without any of these voltage supplies, thus
>> regulator_get_optional() should not be used. I would just use the regulator
>> bulk API to handle all three power supplies.
> 
> The digital part works with AVDD turned off. So the LDO supplying AVDD should
> be switchable to save power ( on the GTA04 device).>
> But not all designs can switch it off. Hence the idea to define it as an
> /optional/ regulator. If it is not defined by DT, the driver simply assumes
> it is always powered on.

I didn't say we can't define regulator supply properties as optional in the DT 
binding.  If we define them as such and any of these *-supply properties is 
missing in DT with regulator_get() the regulator core will use dummy regulator 
for that particular voltage supply.  While with regulator_get_optional() 
-ENODEV is returned when the regulator cannot be found. 

> So in summary we only need AVDD switched for the GTA04 - but it does not
> matter if the others are optional properties. We would not use them.
> 
> It does matter if they are mandatory because it adds DT complexity (size
> and processing) without added function.
 
We should not be defining DT binding only with selected use cases/board
designs in mind.  IMO all three voltage supplies should be listed in the
binding, presumably all can be made optional, with an assumption that when
the property is missing selected pin is hooked up to a fixed regulator.

--
Thanks,
Sylwester

Re: [PATCH v4 2/4] [media] platform: Add Synopsys Designware HDMI RX Controller Driver

[Sylwester Nawrocki]

Hi Jose,

On 06/27/2017 10:43 AM, Jose Abreu wrote:
> On 25-06-2017 22:13, Sylwester Nawrocki wrote:
>> On 06/20/2017 07:26 PM, Jose Abreu wrote:
>>> This is an initial submission for the Synopsys Designware HDMI RX
>>> Controller Driver. This driver interacts with a phy driver so that
>>> a communication between them is created and a video pipeline is
>>> configured.
>>> Signed-off-by: Jose Abreu <joab...@synopsys.com>

>>> +static int dw_hdmi_phy_init(struct dw_hdmi_dev *dw_dev)
>>> +{
>>> +   struct dw_phy_pdata *phy = _dev->phy_config;
>>> +   const struct of_device_id *of_id;
>>> +   struct of_dev_auxdata lookup;
>>  struct of_dev_auxdata lookup = { };
>>
>> You could initialize to 0 here and...
>>
>>> +   struct device_node *child;
>>> +   const char *drvname;
>>> +   int ret;
>>> +
>>> +   child = dw_hdmi_get_phy_of_node(dw_dev, _id);
>>> +   if (!child || !of_id || !of_id->data) {
>>> +   dev_err(dw_dev->dev, "no supported phy found in DT\n");
>>> +   return -EINVAL;
>>> +   }
>>> +
>>> +   drvname = of_id->data;
>>> +   phy->funcs = _hdmi_phy_funcs;
>>> +   phy->funcs_arg = dw_dev;
>>> +
>>> +   lookup.compatible = (char *)of_id->compatible;
>>> +   lookup.phys_addr = 0x0;
>>> +   lookup.name = NULL;
>>
>> ...drop these two assignments.
> 
> Ok.
> 
>>> +   lookup.platform_data = phy;
>>> +
>>> +   request_module(drvname);
>>
>> I'd say this request_module() is not needed when you use the v4l2-async
>> subnotifiers and the modules are properly installed in the file system.
>> I might be missing something though.
> 
> Hmm, well I didn't actually test without request_module but I
> think its needed, otherwise I would have to do:
> 
> modprobe phy_module
> modprobe controller_module
> 
> With request_module I just have to do:
> 
> modprobe controller_module

If you are sure you need it I'm not against that.  But assuming you have udev 
in your system it should also work like this, without request_module():

1. modprobe controller_module -> phy device is created in the kernel, uevent 
sent
2. udev receives uevent, finds matching module and does modprobe phy_module

Remaining part is as before: phy_module registers the driver which gets matched 
with 
phy device; probe() is called which registers v4l2 subdev which then is 
registered
to v4l2_device through the v4l2-async mechanism.

All this assumes udev is running and modules are installed in 
/lib/modules/$(uname -r).
E.g. there should be your module alias as shown by modinfo phy_module in
/lib/modules/$(uname -r)/modules.alias.

>>> +   ret = of_platform_populate(dw_dev->of_node, NULL, , dw_dev->dev);
>>> +   if (ret) {
>>> +   dev_err(dw_dev->dev, "failed to populate phy driver\n");
>>> +   return ret;
>>> +   }
>>> +
>>> +   return 0;
>>> +}
>>> +
>>> +static void dw_hdmi_phy_exit(struct dw_hdmi_dev *dw_dev)
>>> +{
>>> +   of_platform_depopulate(dw_dev->dev);
>>> +}

>>> +static int dw_hdmi_v4l2_notify_complete(struct v4l2_async_notifier 
>>> *notifier)
>>> +{
>>> +   struct dw_hdmi_dev *dw_dev = notifier_to_dw_dev(notifier);
>>> +   int ret;
>>> +
>>> +   ret = v4l2_device_register_subdev_nodes(_dev->v4l2_dev);
>>
>> There shouldn't be multiple struct v4l2_device instances, instead we should
>> have only one created by the main driver. AFAIU, in your case it would be
>> driver associated with the dw-hdmi-soc DT node.  And normally such a top 
>> level
>> driver creates subdev device nodes when its all required sub-devices are
>> available.
>>
>> I think this patch could be useful for you:
>> https://patchwork.linuxtv.org/patch/41834
>>
>> With that the dw-hdmi-soc driver would have it's v4l2-async notifier's
>> notify_complete() callback called only when both the hdmi-rx and the
>> hdmi-phy subdevs are registered.
> 
> Yeah, I saw the patches. I just implemented this way because they
> are not merged yet, right?

I think these patches will be merged in v4.14-rc1, so together with your driver.
You could apply them locally and indicate that your series depends on them in 
the cover letter.

>>> +   if (ret) {
>>> +   dev_err(dw_dev->dev, "failed to register subdev nodes\n");
>>> +   return ret;
>>> +   }
>>> +
>>> +   return 0;
>>> +}

>>> +static int dw_hdmi_rx_probe(struct platform_device *pdev)
>>> +{
>>> +   /* V4L2 initialization */
>>> +   sd = _dev->sd;
>>> +   v4l2_subdev_init(sd, _hdmi_sd_ops);
>>> +   strlcpy(sd->name, dev_name(dev), sizeof(sd->name));
>>> +   sd->dev = dev;
>>> +   sd->internal_ops = _hdmi_internal_ops;
>>> +   sd->flags |= V4L2_SUBDEV_FL_HAS_EVENTS;
>>
>> Don't you also need V4L2_SUBDEV_FL_HAS_DEVNODE flag set?
> 
> Ouch. Yes I need otherwise the subdev will not be associated with
> the v4l2_device.

This flag indicates that the v4l2 subdev device node (/dev/v4l-subdev?)
should be created for this subdevice.

---
Regards,
Sylwester
 

Re: [PATCH v1 1/6] DT bindings: add bindings for ov965x camera module

[Sylwester Nawrocki]

On 06/26/2017 12:35 PM, Hugues FRUCHET wrote:
>> What I am missing to support the GTA04 camera is the control of the optional 
>> "vana-supply".
>> So the driver does not power up the camera module when needed and therefore 
>> probing fails.
>>
>> - vana-supply: a regulator to power up the camera module.
>>
>> Driver code is not complex to add:

> Yes, I saw it in your code, but as I don't have any programmable power
> supply on my setup, I have not pushed this commit.

Since you are about to add voltage supplies to the DT binding I'd suggest
to include all three voltage supplies of the sensor chip. Looking at the OV9650
and the OV9655 datasheet there are following names used for the voltage supply
pins:

AVDD - Analog power supply,
DVDD - Power supply for digital core logic,
DOVDD - Digital power supply for I/O.

I doubt the sensor can work without any of these voltage supplies, thus
regulator_get_optional() should not be used. I would just use the regulator
bulk API to handle all three power supplies.

--
Regards,
Sylwester

Re: [PATCH v3 2/2] v4l: async: add subnotifier registration for subdevices

[Sylwester Nawrocki]

On 06/13/2017 04:30 PM, Niklas Söderlund wrote:

When the registered() callback of v4l2_subdev_internal_ops is called the
subdevice has access to the master devices v4l2_dev and it's called with
the async frameworks list_lock held. In this context the subdevice can
register its own notifiers to allow for incremental discovery of
subdevices.

The master device registers the subdevices closest to itself in its
notifier while the subdevice(s) register notifiers for their closest
neighboring devices when they are registered. Using this incremental
approach two problems can be solved:

1. The master device no longer has to care how many devices exist in
the pipeline. It only needs to care about its closest subdevice and
arbitrary long pipelines can be created without having to adapt the
master device for each case.

2. Subdevices which are represented as a single DT node but register
more than one subdevice can use this to improve the pipeline
discovery, since the subdevice driver is the only one who knows which
of its subdevices is linked with which subdevice of a neighboring DT
node.

To enable subdevices to register/unregister notifiers from the
registered()/unregistered() callback v4l2_async_subnotifier_register()
and v4l2_async_subnotifier_unregister() are added. These new notifier
register functions are similar to the master device equivalent functions
but run without taking the v4l2-async list_lock which already is held
when the registered()/unregistered() callbacks are called.

Signed-off-by: Niklas Söderlund<niklas.soderlund+rene...@ragnatech.se>
Acked-by: Hans Verkuil<hans.verk...@cisco.com>
Acked-by: Sakari Ailus<sakari.ai...@linux.intel.com>


Acked-by: Sylwester Nawrocki <snawro...@kernel.org>

Re: [PATCH v3 1/2] v4l: async: check for v4l2_dev in v4l2_async_notifier_register()

[Sylwester Nawrocki]

On 06/13/2017 04:30 PM, Niklas Söderlund wrote:

Add a check for v4l2_dev to v4l2_async_notifier_register() as to fail as
early as possible since this will fail later in v4l2_async_test_notify().

Signed-off-by: Niklas Söderlund<niklas.soderlund+rene...@ragnatech.se>
Acked-by: Sakari Ailus<sakari.ai...@linux.intel.com>
Acked-by: Hans Verkuil<hans.verk...@cisco.com>


Acked-by: Sylwester Nawrocki <snawro...@kernel.org>

Re: [PATCH v4 2/4] [media] platform: Add Synopsys Designware HDMI RX Controller Driver

[Sylwester Nawrocki]

On 06/20/2017 07:26 PM, Jose Abreu wrote:
> This is an initial submission for the Synopsys Designware HDMI RX
> Controller Driver. This driver interacts with a phy driver so that
> a communication between them is created and a video pipeline is
> configured.

> Signed-off-by: Jose Abreu <joab...@synopsys.com>
> Cc: Carlos Palminha <palmi...@synopsys.com>
> Cc: Mauro Carvalho Chehab <mche...@kernel.org>
> Cc: Hans Verkuil <hans.verk...@cisco.com>
> Cc: Sylwester Nawrocki <snawro...@kernel.org>
> 
> Changes from v3:
>   - Use v4l2 async API (Sylwester)
>   - Do not block waiting for phy
>   - Do not use busy waiting delays (Sylwester)
>   - Simplify dw_hdmi_power_on (Sylwester)
>   - Use clock API (Sylwester)
>   - Use compatible string (Sylwester)
>   - Minor fixes (Sylwester)
> Changes from v2:
>   - Address review comments from Hans regarding CEC
>   - Use CEC notifier
>   - Enable SCDC
> Changes from v1:
>   - Add support for CEC
>   - Correct typo errors
>   - Correctly detect interlaced video modes
>   - Correct VIC parsing
> Changes from RFC:
>   - Add support for HDCP 1.4
>   - Fixup HDMI_VIC not being parsed (Hans)
>   - Send source change signal when powering off (Hans)
>   - Add a "wait stable delay"
>   - Detect interlaced video modes (Hans)
>   - Restrain g/s_register from reading/writing to HDCP regs (Hans)
> ---
>   drivers/media/platform/dwc/Kconfig  |   15 +
>   drivers/media/platform/dwc/Makefile |1 +
>   drivers/media/platform/dwc/dw-hdmi-rx.c | 1862 
> +++
>   drivers/media/platform/dwc/dw-hdmi-rx.h |  441 
>   include/media/dwc/dw-hdmi-rx-pdata.h|   97 ++
>   5 files changed, 2416 insertions(+)
>   create mode 100644 drivers/media/platform/dwc/dw-hdmi-rx.c
>   create mode 100644 drivers/media/platform/dwc/dw-hdmi-rx.h
>   create mode 100644 include/media/dwc/dw-hdmi-rx-pdata.h
> 

> diff --git a/drivers/media/platform/dwc/dw-hdmi-rx.c 
> b/drivers/media/platform/dwc/dw-hdmi-rx.c
> new file mode 100644
> index 000..22ee51d
> --- /dev/null
> +++ b/drivers/media/platform/dwc/dw-hdmi-rx.c

> +static const struct of_device_id dw_hdmi_supported_phys[] = {
> + { .compatible = "snps,dw-hdmi-phy-e405", .data = DW_PHY_E405_DRVNAME, },
> + { },
> +};
> +
> +static struct device_node *dw_hdmi_get_phy_of_node(struct dw_hdmi_dev 
> *dw_dev,
> + const struct of_device_id **found_id)
> +{
> + struct device_node *child = NULL;
> + const struct of_device_id *id;
> +
> + for_each_child_of_node(dw_dev->of_node, child) {
> + id = of_match_node(dw_hdmi_supported_phys, child);
> + if (id)
> + break;
> + }
> +
> + if (found_id)
> + *found_id = id;
> +
> + return child;
> +}
> +
> +static int dw_hdmi_phy_init(struct dw_hdmi_dev *dw_dev)
> +{
> + struct dw_phy_pdata *phy = _dev->phy_config;
> + const struct of_device_id *of_id;
> + struct of_dev_auxdata lookup;

struct of_dev_auxdata lookup = { };

You could initialize to 0 here and...

> + struct device_node *child;
> + const char *drvname;
> + int ret;
> +
> + child = dw_hdmi_get_phy_of_node(dw_dev, _id);
> + if (!child || !of_id || !of_id->data) {
> + dev_err(dw_dev->dev, "no supported phy found in DT\n");
> + return -EINVAL;
> + }
> +
> + drvname = of_id->data;
> + phy->funcs = _hdmi_phy_funcs;
> + phy->funcs_arg = dw_dev;
> +
> + lookup.compatible = (char *)of_id->compatible;
> + lookup.phys_addr = 0x0;
> + lookup.name = NULL;

...drop these two assignments.

> + lookup.platform_data = phy;
> +
> + request_module(drvname);

I'd say this request_module() is not needed when you use the v4l2-async 
subnotifiers and the modules are properly installed in the file system.
I might be missing something though.

> + ret = of_platform_populate(dw_dev->of_node, NULL, , dw_dev->dev);
> + if (ret) {
> + dev_err(dw_dev->dev, "failed to populate phy driver\n");
> + return ret;
> + }
> +
> + return 0;
> +}
> +
> +static void dw_hdmi_phy_exit(struct dw_hdmi_dev *dw_dev)
> +{
> + of_platform_depopulate(dw_dev->dev);
> +}

> +static int dw_hdmi_registered(struct v4l2_subdev *sd)
> +{
> + struct dw_hdmi_dev *dw_dev = to_dw_dev(sd);
> + int ret;
> +
> + ret = cec_register_adapter(dw_dev->cec_adap, dw_dev->dev);
> + if 

Re: [PATCH v4 1/4] [media] platform: Add Synopsys Designware HDMI RX PHY e405 Driver

[Sylwester Nawrocki]

Hi Jose,

Thank you for addressing my review comments. Couple more suggestions below.

On 06/20/2017 07:26 PM, Jose Abreu wrote:
> This adds support for the Synopsys Designware HDMI RX PHY e405. This
> phy receives and decodes HDMI video that is delivered to a controller.

> +static int dw_phy_config(struct dw_phy_dev *dw_dev, unsigned char 
> color_depth,
> + bool hdmi2, bool scrambling)
> +{

> + phy_reset(dw_dev, 1);
> + phy_pddq(dw_dev, 1);
> + phy_svsmode(dw_dev, 1);
> +
> + phy_zcal_reset(dw_dev);
> + do {
> + udelay(1000);

Could be mdelay(1) or better e.g. usleep_range(1000, 1100);

> + zcal_done = phy_zcal_done(dw_dev);
> + } while (!zcal_done && timeout--);
> +
> + if (!zcal_done) {
> + dev_err(dw_dev->dev, "Zcal calibration failed\n");
> + return -ETIMEDOUT;
> + }

> + return 0;
> +}

> +static int dw_phy_probe(struct platform_device *pdev)
> +{
> + struct dw_phy_pdata *pdata = pdev->dev.platform_data;
> + struct device *dev = >dev;
> + struct dw_phy_dev *dw_dev;
> + struct v4l2_subdev *sd;
> + int ret;
> +
> + dev_dbg(dev, "probe start\n");
> +
> + /* Resource allocation */

This comment is not needed.

> + dw_dev = devm_kzalloc(dev, sizeof(*dw_dev), GFP_KERNEL);
> + if (!dw_dev)
> + return -ENOMEM;
> +
> + /* Resource initialization */

Ditto.

> + if (!pdata) {
> + dev_err(dev, "no platform data suplied\n");
> + return -EINVAL;
> + }
> +
> + dw_dev->dev = dev;
> + dw_dev->config = pdata;
> + dw_dev->version = 405;

How about storing the version number in the dw_hdmi_phy_e405_id[] table
and retrieving it here with of_device_get_match_data() ?

> + mutex_init(_dev->lock);
> +
> + /* Get config clock value */

The comment is not needed, it's clear from the code we are getting the clock 
and its rate.

> + dw_dev->clk = devm_clk_get(dev, "cfg-clk");

As Rob suggested, it would be good to change name of the clock to just "cfg".

> + if (IS_ERR(dw_dev->clk)) {
> + dev_err(dev, "failed to get cfg-clk\n");
> + return PTR_ERR(dw_dev->clk);
> + }
> +
> + ret = clk_prepare_enable(dw_dev->clk);
> + if (ret) {
> + dev_err(dev, "failed to enable cfg-clk\n");
> + return ret;
> + }
> +
> + dw_dev->cfg_clk = clk_get_rate(dw_dev->clk) / 100;

100U ?

> + if (!dw_dev->cfg_clk) {
> + dev_err(dev, "invalid cfg-clk frequency\n");> + ret = 
> -EINVAL;
> + goto err_clk;
> + }
> +
> + /* V4L2 initialization */
> + sd = _dev->sd;
> + v4l2_subdev_init(sd, _phy_sd_ops);
> + strlcpy(sd->name, dev_name(dev), sizeof(sd->name));
> + sd->dev = dev;
> +
> + /* Force phy disabling */
> + dw_dev->phy_enabled = true;
> + dw_phy_disable(dw_dev);
> +
> + /* Register subdev */
> + ret = v4l2_async_register_subdev(sd);
> + if (ret) {
> + dev_err(dev, "failed to register subdev\n");
> + goto err_clk;
> + }
> +
> + /* All done */

Superfluous comment.

> + dev_set_drvdata(dev, sd);
> + dev_info(dev, "driver probed (cfg-clk=%d)\n", dw_dev->cfg_clk);

This should be at dev_dbg() level.

> + return 0;
> +
> +err_clk:
> + clk_disable_unprepare(dw_dev->clk);
> + return ret;
> +}
> +
> +static int dw_phy_remove(struct platform_device *pdev)
> +{
> + struct device *dev = >dev;

The assignment here could be dropped and >dev used directly below.

> + struct v4l2_subdev *sd = dev_get_drvdata(dev);

> + struct dw_phy_dev *dw_dev = to_dw_dev(sd);
> +
> + v4l2_async_unregister_subdev(sd);
> + clk_disable_unprepare(dw_dev->clk);

> + dev_info(dev, "driver removed\n");

This should be at dev_dbg() level or dropped entirely.

> + return 0;
> +}
> +
> +static const struct of_device_id dw_hdmi_phy_e405_id[] = {
> + { .compatible = "snps,dw-hdmi-phy-e405" },
> + { },
> +};
> +MODULE_DEVICE_TABLE(of, dw_hdmi_phy_e405_id);

--
Regards,
Sylwester

Re: [PATCH 2/4] media: s3c-camif: use LINUX_VERSION_CODE for driver's version

[Sylwester Nawrocki]

On 06/24/2017 10:40 PM, Mauro Carvalho Chehab wrote:

We seldomly increment version numbers on drivers, because... we
usually forget;-)

So, instead, just make it identical to the Kernel version, as what
we do on all other drivers.

Signed-off-by: Mauro Carvalho Chehab <mche...@s-opensource.com>


Acked-by: Sylwester Nawrocki <snawro...@kernel.org>

Re: [PATCH 2/2] media/uapi/v4l: clarify cropcap/crop/selection behavior

[Sylwester Nawrocki]

On 06/19/2017 03:49 PM, Hans Verkuil wrote:

From: Hans Verkuil<hans.verk...@cisco.com>

Unfortunately the use of 'type' was inconsistent for multiplanar
buffer types. Starting with 4.14 both the normal and _MPLANE variants
are allowed, thus making it possible to write sensible code.

Yes, we messed up:-(

Signed-off-by: Hans Verkuil<hans.verk...@cisco.com>


Reviewed-by: Sylwester Nawrocki <s.nawro...@samsung.com>

--
Thanks,
Sylwester

Re: [PATCH 1/2] v4l2-ioctl/exynos: fix G/S_SELECTION's type handling

[Sylwester Nawrocki]

On 06/19/2017 03:49 PM, Hans Verkuil wrote:

From: Hans Verkuil<hansv...@cisco.com>

The type field in struct v4l2_selection is supposed to never use the
_MPLANE variants. E.g. if the driver supports 
V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE,
then userspace should still pass V4L2_BUF_TYPE_VIDEO_CAPTURE.

The reasons for this are lost in the mists of time, but it is really
annoying. In addition, the exynos drivers didn't follow this rule and
instead expected the _MPLANE type.

To fix that code is added to the v4l2 core that maps the _MPLANE buffer
types to their regular equivalents before calling the driver.

Effectively this allows for userspace to use either _MPLANE or the regular
buffer type. This keeps backwards compatibility while making things easier
for userspace.

Since drivers now never see the _MPLANE buffer types the exynos drivers
had to be adapted as well.

Signed-off-by: Hans Verkuil <hansv...@cisco.com>
Acked-by: Sakari Ailus <sakari.ai...@linux.intel.com>


Acked-by: Sylwester Nawrocki <s.nawro...@samsung.com>

--
Thanks,
Sylwester

Re: [PATCH v3 2/4] [media] platform: Add Synopsys Designware HDMI RX Controller Driver

[Sylwester Nawrocki]

On 06/19/2017 11:33 AM, Jose Abreu wrote:
> On 18-06-2017 19:04, Sylwester Nawrocki wrote:
>> On 06/16/2017 06:38 PM, Jose Abreu wrote:
>>> This is an initial submission for the Synopsys Designware HDMI RX
>>> Controller Driver. This driver interacts with a phy driver so that
>>> a communication between them is created and a video pipeline is
>>> configured.
>>>
>>> The controller + phy pipeline can then be integrated into a fully
>>> featured system that can be able to receive video up to 4k@60Hz
>>> with deep color 48bit RGB, depending on the platform. Although,
>>> this initial version does not yet handle deep color modes.
>>> Signed-off-by: Jose Abreu <joab...@synopsys.com>
>>>
>>> +static int dw_hdmi_phy_init(struct dw_hdmi_dev *dw_dev)
>>> +{

>>> +   request_module(pdevinfo.name);
>>> +
>>> +   dw_dev->phy_pdev = platform_device_register_full();
>>> +   if (IS_ERR(dw_dev->phy_pdev)) {
>>> +   dev_err(dw_dev->dev, "failed to register phy device\n");
>>> +   return PTR_ERR(dw_dev->phy_pdev);
>>> +   }
>>> +
>>> +   if (!dw_dev->phy_pdev->dev.driver) {
>>> +   dev_err(dw_dev->dev, "failed to initialize phy driver\n");
>>> +   goto err;
>>> +   }
>> I think this is not safe because there is nothing preventing unbinding
>> or unloading the driver at this point.
>>
>>> +   if (!try_module_get(dw_dev->phy_pdev->dev.driver->owner)) {
>> So dw_dev->phy_pdev->dev.driver may be already NULL here.
> 
> How can I make sure it wont be NULL? Because I've seen other
> media drivers do this and I don't think they do any kind of
> locking, but they do this mainly for I2C subdevs.

You could do device_lock(dev)/device_unlock(dev) to avoid possible races. 
And setting 'suppress_bind_attrs' field in the sub-device drivers would 
disable sysfs unbind attributes, so sub-device driver wouldn't get unbound
unexpectedly trough sysfs.
 
>>> +   dev_err(dw_dev->dev, "failed to get phy module\n");
>>> +   goto err;
>>> +   }
>>> +
>>> +   dw_dev->phy_sd = dev_get_drvdata(_dev->phy_pdev->dev);
>>> +   if (!dw_dev->phy_sd) {
>>> +   dev_err(dw_dev->dev, "failed to get phy subdev\n");
>>> +   goto err_put;
>>> +   }
>>> +
>>> +   if (v4l2_device_register_subdev(_dev->v4l2_dev, dw_dev->phy_sd)) {
>>> +   dev_err(dw_dev->dev, "failed to register phy subdev\n");
>>> +   goto err_put;
>>> +   }
>>
>> I'd suggest usign v4l2-async API, so we use a common pattern for sub-device
>> registration.  And with recent change [1] you could handle this PHY subdev
>> in a standard way.  That might be more complicated than it is now but should
>> make any future platform integration easier.

> So I will instantiate phy driver and then wait for phy driver to
> register into v4l2 core?

Yes, for instance the RX controller driver registers a notifier, instantiates
the child PHY device and then waits until the PHY driver completes 
initialization.

>>> +   module_put(dw_dev->phy_pdev->dev.driver->owner);
>>> +   return 0;
>>> +
>>> +err_put:
>>> +   module_put(dw_dev->phy_pdev->dev.driver->owner);
>>> +err:
>>> +   platform_device_unregister(dw_dev->phy_pdev);
>>> +   return -EINVAL;
>>> +}

>>> +static int dw_hdmi_power_on(struct dw_hdmi_dev *dw_dev, u32 input)
>>> +{
>>> +   struct dw_hdmi_work_data *data;
>>> +   unsigned long flags;
>>> +
>>> +   data = devm_kzalloc(dw_dev->dev, sizeof(*data), GFP_KERNEL);
>>
>> Why use devm_{kzalloc, kfree} when dw_hdmi_power_on() is not only called
>> in the device's probe() callback, but in other places, including interrupt
>> handler?  devm_* API is normally used when life time of a resource is more
>> or less equal to life time of struct device or its matched driver.  Were
>> there any specific reasons to not just use kzalloc()/kfree() ?
> 
> No specific reason, I just thought it would be safer because if I
> cancel a work before it started then memory will remain
> allocated. But I will change to kzalloc().

OK, I overlooked such situation. Since you allow one job queued maybe
just embed struct work_struct in struct dw_hdmi_dev and retrieve it with
container_of() macro in the work handler and use additional field in
struct dw_hdmi_dev protected with dw_dev->lock for passing

Re: [PATCH v3 4/4] dt-bindings: media: Document Synopsys Designware HDMI RX

[Sylwester Nawrocki]

On 06/19/2017 11:01 AM, Jose Abreu wrote:
> Using fixed-clock was already in my todo list. Regarding phy I
> need to pass pdata so that the callbacks between controller and
> phy are established. I also need to make sure that phy driver
> will be loaded by the controller driver. Hmm, and also address of
> the phy on th JTAG bus is fed to the controller driver not to the
> phy driver. Maybe leave the property as is (the
> "snps,hdmi-phy-jtag-addr") or parse it from the phy node?

I think the RX controller can parse it's child phy node to retrieve JTAG 
address from the reg property.  That seems better than creating custom 
property for device address on the bus.

Does the PHY support any other type of control bus, e.g. I2C or SPI?

> I also need to pass pdata to the controller driver (the callbacks
> for 5v handling) which are agnostic of the controller. These

Is this about detecting +5V coming from the HDMI connector? Or some
other voltage supply?

> reasons prevented me from adding compatible strings to both
> drivers and just use a wrapper driver instead. This way i do

If you add struct of_device_id instance to your module and define
MODULE_DEVICE_ALIAS(of, ...) there, it will allow the module to be loaded 
when device with matching compatible string is created in the kernel. 
User space is notified with uevent about device creation.

> "modprobe wrapper_driver" and I get all the drivers loaded via
> request_module(). Still, I like your approach much better. I saw
> that I can pass pdata using of_platform_populate, could you
> please confirm if I can still maintain this architecture (i.e.
> prevent modules from loading until I get all the chain setup)?

You could try to pass platform data this way, that should work. But 
I doubt it's the right directions, I would rather see things done 
in the V4L2 layer. 

> Following your approach I could get something like this:
> 
> hdmi_system@ {
>  compatible = "snps,dw-hdmi-rx-wrapper";

This would need to refer to some hardware block, we should avoid virtual 
device nodes in DT.

>  reg = <0x 0x>;
>  interrupts = <3>;
>  #address-cells = <1>;
>  #size-cells = <1>;

You would need also an (empty) 'ranges' property here.

>  hdmi_controller@0 {
>  compatible = "snps,dw-hdmi-rx-controller";
>  reg = <0x0 0x1>;
>  interrupts = <1>;
>  edid-phandle = <_system>;
>  clocks = <>;
>  clock-names = "ref-clk";
>  #address-cells = <1>;
>  #size-cells = <0>;
> 
>  hdmi_phy@f3 {
>  compatible = "snps,dw-hdmi-phy-e405";
>  reg = <0xf3>;
>  clocks = <>;
>  clock-names = "cfg-clk";
>  }
>  }
> };
> 
> And then snps,dw-hdmi-rx-wrapper would call of_platform_populate
> for controller which would instead call of_platform_populate for
> phy. Is this possible, and maintainable? Isn't the controller
> driver get auto loaded because of the compatible string match?

As I mentioned above, auto loading should work if you have instance 
of MODULE_DEVICE_TABLE() defined in the module, but the module might
not be available immediately after creating devices with 
of_platform_populate().  You may want to have a look at the v4l2-async 
API (drivers/media/v4l2-core/v4l2-async.c). It allows one driver
to register a notifier for its sub-devices. And the parent driver
can complete initialization when it gets all its v4l2 subdevs
registered.

But I'm not sure about calls from the PHY back to the RX controller, 
possibly v4l2_set_subdev_hostdata()/v4l2_get_subdev_hostdata() could 
be used for passing the ops.

> And one more thing: The reg address of the hdmi_controller: Isn't
> this relative to the parent node? I mean isn't this going to be
> 0x + 0x0? Because I don't want that :/

Address space of child nodes doesn't need to be relative to 'reg' range 
of parent node, these can be entirely distinct address ranges. See for 
example I2C bus controllers, the I2C addresses of slave devices are not 
much related to the memory mapped IO registers region of the bus controller.

--
Regards,
Sylwester

Re: [RFC PATCH 2/2] media/uapi/v4l: clarify cropcap/crop/selection behavior

[Sylwester Nawrocki]

Hi Hans,

On 06/19/2017 09:35 AM, Hans Verkuil wrote:


diff --git a/Documentation/media/uapi/v4l/vidioc-cropcap.rst 
b/Documentation/media/uapi/v4l/vidioc-cropcap.rst
index f21a69b554e1..d354216846e6 100644
--- a/Documentation/media/uapi/v4l/vidioc-cropcap.rst
+++ b/Documentation/media/uapi/v4l/vidioc-cropcap.rst
@@ -39,16 +39,19 @@ structure. Drivers fill the rest of the structure. The 
results are
   constant except when switching the video standard. Remember this switch
   can occur implicit when switching the video input or output.
   
-Do not use the multiplanar buffer types. Use

-``V4L2_BUF_TYPE_VIDEO_CAPTURE`` instead of
-``V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE`` and use
-``V4L2_BUF_TYPE_VIDEO_OUTPUT`` instead of
-``V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE``.
-
   This ioctl must be implemented for video capture or output devices that
   support cropping and/or scaling and/or have non-square pixels, and for
   overlay devices.
   
+.. note::

+   Unfortunately in the case of multiplanar buffer types
+   (``V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE`` and 
``V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE``)
+   this API was messed up with regards to how the :c:type:`v4l2_cropcap` 
``type`` field
+   should be filled in. The Samsung Exynos drivers only accepted the


I propose I change this to "Some drivers only..." here and at the other places I
refer to Exynos.

Do you agree?


Yes, perhaps we could move the note paragraphs on the G_CROP,
G_SELECTION pages after v4l2_crop, v4l2_selection tables?

--
Regards,
Sylwester

Re: [RFC PATCH 2/2] media/uapi/v4l: clarify cropcap/crop/selection behavior

[Sylwester Nawrocki]

On 06/16/2017 03:08 PM, Sakari Ailus wrote:
> On Mon, May 08, 2017 at 04:35:06PM +0200, Hans Verkuil wrote:
>> From: Hans Verkuil 
>>
>> Unfortunately the use of 'type' was inconsistent for multiplanar
>> buffer types. Starting with 4.12 both the normal and _MPLANE variants
>> are allowed, thus making it possible to write sensible code.
>>
>> Yes, we messed up :-(
> 
> Things worse than this have happened. :-)
> 
> I don't think in general I would write about driver bugs that have already
> been fixed in developer documentation. That said, I'm not sure how otherwise
> developers would learn about this, but OTOH has it been reported to us as a
> bug?
> 
> Marek, Sylwester: any idea how widely the drivers in question are in use? If
> there's a real chance of hitting this, then it does make sense to mention it
> in the documentation.

I'm not sure how widely are used those drivers, I think we should just assume 
they are deployed and whatever we do should be backwards compatible. I don't 
think it is much helpful for Exynos to add notes like this in the 
documentation, 
so far I didn't receive any related bug report.

And even though now there is some confusion because drivers see "regular" 
buffer type while user space uses _MPLANE I like the $subject patch set
as it makes the API clearer from user perspective.

--
Regards,
Sylwester