On Tue, Dec 29, 2020 at 2:02 AM Jerry-ch Chen
[off-list ref] wrote:

Hi Tomasz,

On Thu, 2020-11-12 at 20:05 +0800, Jerry-ch Chen wrote:

quoted

Hi Tomasz,

On Thu, 2020-11-12 at 13:26 +0900, Tomasz Figa wrote:

quoted

On Wed, Nov 11, 2020 at 8:51 PM Jerry-ch Chen
[off-list ref] wrote:

quoted

Hi Tomasz,

On Wed, 2020-07-01 at 01:19 +0800, Tomasz Figa wrote:

quoted

Hi Jerry,

On Tue, Jun 30, 2020 at 10:10:53PM +0800, Jerry-ch Chen wrote:

quoted

Hi Tomasz,

On Thu, 2020-05-21 at 18:38 +0000, Tomasz Figa wrote:

quoted

Hi Jerry,

On Wed, May 13, 2020 at 11:45:37PM +0200, Tomasz Figa wrote:

quoted

Hi Jerry,

On Fri, May 8, 2020 at 4:03 AM Jerry-ch Chen [off-list ref] wrote:

quoted

Hi Laurent, Tomasz, Matthias,

gentle ping for this patch set,
If no new comments, I would like to send a newer version.

Sorry, I still haven't had a chance to look at the series, so feel
free to send a new version and I will take a look at the new one.

Finally found some time to review the series. Again sorry for the delay
and thanks for your patience.

Some general comments:
1) The metadata format FourCC should be added in a separate patch,
together with documentation for it.
2) Control IDs, structs used by the userspace, etc. should be defined in
a header under include/uapi/linux.

Please also check my replies to particular patches for further comments.

Best regards,
Tomasz

Appreciate for your reply,

So far, I've locally created an uapi header:
include/uapi/linux/mtk_fd_40.h
which provides some values, control ids, and the definitions of
structures that would be needed by user of mtk_fd_40 driver.
In addition, I also provide a MACRO as example in comments that can
extract the struct member with bit length and offset
definitions(eliminate the bit-fields).

Also, I would like to rename struct fd_user_output with struct
mtk_fd_hw_result. I worry fd_user_output would be a confusing name.

The change sounds good to me.

quoted

I will add them in a separate patch in next version.

Okay.

quoted

I am still working on the documentation, which might be
Documentation/media/uapi/v4l/pixfmt-meta-mtk-fd-40.rst.
Refering the other pixfmt-*.rst files, I will try to provide the
flat-table of the metadata with the structure of the mtk_fd_hw_result.

Sounds good to me.

quoted

I am confusing that should I remain the name with -40 in the tail of rst
file?

The header and documentation file names should match the driver name.  I
just noticed there is some inconsistency in the naming, though. The
driver seems to be located under drivers/media/platform/mtk-isp/fd, but
the driver name in the platform driver struct and as reported by
VIDIOC_QUERYCAP seems to be "mtk-fd-4.0".

quoted

Since we have many mtk-* drivers in the tree currently, I think it might
make sense to consolidate them under drivers/media/platform/mediatek,
similarly to drivers/media/platform/qcom or /rockchip. But it could be
done later, as a follow-up.

My suggestion would be to place the driver under
drivers/media/platform/mtk-fd-40 and also rename the related Kconfig
symbol to include the _40 suffix.

What do you think?

I Appreciate your comments,
Sorry for the late reply.

Would it be possible for me to replace the driver as drivers/media/platform/mtk_fd/mtk-fd-40?(Just like mtk-isp/isp_50)

I'm not a big fan of duplicating "mtk fd" in the path. How about just
making it drivers/media/platform/mtk-fd-40?

Ok, I will make it drivers/media/platform/mtk-fd-40,
and also rename the related Kconfig symbol to include the _40 suffix.

Thanks and Best Regards,
Jerry.

quoted

Best regards,
Tomasz

I've finish the document of FD driver, describing the structure of the
mtk_fd_hw_result. Could I send the new version of the driver? would the
folder path replacement must be included in the new version?

Thanks Jerry and Happy New Year.

Since the driver should be in a good shape after addressing the last
comments, perhaps it would make sense to update the path as well, so
that it can be merged if no other issues are found?

Best regards,
Tomasz

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel