Re: [RFC PATCH V7 1/3] media: platform: mtk-isp: Add Mediatek sensor interface driver

[Tomasz Figa <tfiga@chromium.org>]

On Fri, Jul 17, 2020 at 4:21 PM Chun-Kuang Hu <chunkuang.hu@kernel.org> wrote:
>
> Hi, Louis:
>
> Louis Kuo (郭德寧) <louis.kuo@mediatek.com> 於 2020年7月17日 週五 上午10:56寫道：
> >
> > Hi Chun-Kuang,
> >
> > Since phy driver is not belong to V4L2 scope
> >
> > Should I need to upsteam 8183 mipi phy driver with new a patch other than this one ?
>
> Yes, I think so. Maybe different series would be better.

Since both would not be usable without each other, I believe they
should be handled by the same series, although care should be taken to
have the patches in the series clearly separated between the two
subsystems.

Best regards,
Tomasz

>
> Regards,
> Chun-Kuang.
>
> >
> > BRs
> > Louis
> >
> > -----Original Message-----
> > From: Chun-Kuang Hu [mailto:chunkuang.hu@kernel.org]
> > Sent: Thursday, July 9, 2020 9:13 PM
> > To: Louis Kuo (郭德寧)
> > Cc: hans.verkuil@cisco.com; laurent.pinchart+renesas@ideasonboard.com; Tomasz Figa; keiichiw@chromium.org; Matthias Brugger; Mauro Carvalho Chehab; devicetree@vger.kernel.org; Sean Cheng (鄭昇弘); srv_heupstream; Jerry-ch Chen (陳敬憲); Jungo Lin (林明俊); Sj Huang (黃信璋); yuzhao@chromium.org; moderated list:ARM/Mediatek SoC support; zwisler@chromium.org; Christie Yu (游雅惠); Frederic Chen (陳俊元); Linux ARM; linux-media@vger.kernel.org
> > Subject: Re: [RFC PATCH V7 1/3] media: platform: mtk-isp: Add Mediatek sensor interface driver
> >
> > Hi, Louis:
> >
> > Louis Kuo <louis.kuo@mediatek.com> 於 2020年7月8日 週三 下午6:41寫道：
> > >
> > > This patch adds Mediatek's sensor interface driver. Sensor interface
> > > driver is a MIPI-CSI2 host driver, namely, a HW camera interface controller.
> > > It support a widely adopted, simple, high-speed protocol primarily
> > > intended for point-to-point image and video transmission between
> > > cameras and host devices. The mtk-isp directory will contain drivers
> > > for multiple IP blocks found in Mediatek ISP system. It will include
> > > ISP Pass 1 driver, sensor interface driver, DIP driver and face detection driver.
> > >
> > > Signed-off-by: Louis Kuo <louis.kuo@mediatek.com>
> > > ---
> > >  drivers/media/platform/Makefile               |    1 +
> > >  drivers/media/platform/mtk-isp/Kconfig        |   18 +
> > >  drivers/media/platform/mtk-isp/Makefile       |    3 +
> > >  .../media/platform/mtk-isp/seninf/Makefile    |    7 +
> > >  .../platform/mtk-isp/seninf/mtk_seninf.c      |  974 +++++++++++
> > >  .../platform/mtk-isp/seninf/mtk_seninf_dphy.c |  353 ++++
> >
> > I think phy driver should be placed in drivers/phy/mediatek and separate phy driver to an independent patch.
> >
> > >  .../platform/mtk-isp/seninf/mtk_seninf_reg.h  | 1491 +++++++++++++++++
> > >  .../mtk-isp/seninf/mtk_seninf_rx_reg.h        |  515 ++++++
> > >  8 files changed, 3362 insertions(+)
> > >  create mode 100644 drivers/media/platform/mtk-isp/Kconfig
> > >  create mode 100644 drivers/media/platform/mtk-isp/Makefile
> > >  create mode 100644 drivers/media/platform/mtk-isp/seninf/Makefile
> > >  create mode 100644 drivers/media/platform/mtk-isp/seninf/mtk_seninf.c
> > >  create mode 100644
> > > drivers/media/platform/mtk-isp/seninf/mtk_seninf_dphy.c
> > >  create mode 100644
> > > drivers/media/platform/mtk-isp/seninf/mtk_seninf_reg.h
> > >  create mode 100644
> > > drivers/media/platform/mtk-isp/seninf/mtk_seninf_rx_reg.h
> > >
> >
> > [snip]
> >
> > > +
> > > +#include <linux/clk.h>
> > > +#include <linux/delay.h>
> > > +#include <linux/interrupt.h>
> > > +#include <linux/module.h>
> > > +#include <linux/of_graph.h>
> > > +#include <linux/of_irq.h>
> >
> > No irq handler, so remove this.
> >
> > > +#include <linux/platform_device.h>
> > > +#include <linux/pm_runtime.h>
> > > +#include <linux/slab.h>
> > > +#include <linux/videodev2.h>
> > > +#include <media/v4l2-async.h>
> > > +#include <media/v4l2-ctrls.h>
> > > +#include <media/v4l2-event.h>
> > > +#include <media/v4l2-fwnode.h>
> > > +#include <media/v4l2-subdev.h>
> > > +#include <linux/phy/phy.h>
> > > +#include "mtk_seninf_reg.h"
> > > +
> >
> > [snip]
> >
> > > +
> > > +static int seninf_set_ctrl(struct v4l2_ctrl *ctrl) {
> > > +       struct mtk_seninf *priv = container_of(ctrl->handler,
> > > +                                            struct mtk_seninf,
> > > +ctrl_handler);
> > > +
> > > +       switch (ctrl->id) {
> > > +       case V4L2_CID_TEST_PATTERN:
> > > +               if (ctrl->val == TEST_GEN_PATTERN)
> > > +                       return seninf_enable_test_pattern(priv);
> >
> > Without this, this driver still works, so move this to an independent patch.
> >
> > > +               else if (ctrl->val == TEST_DUMP_DEBUG_INFO)
> > > +                       return seninf_dump_debug_info(priv);
> >
> > Ditto.
> >
> > > +               else
> > > +                       return -EINVAL;
> > > +       }
> > > +
> > > +       return 0;
> > > +}
> > > +
> >
> > [snip]
> >
> > > +
> > > +#ifdef CONFIG_OF
> > > +static const struct of_device_id mtk_mipi_dphy_of_match[] = {
> > > +       {.compatible = "mediatek,mt8183-mipi_dphy"},
> >
> > Where is the definition of "mediatek,mt8183-mipi_dphy"?
> >
> > Regards,
> > Chun-Kuang.
> >
> > > +       {},
> > > +};
> > > +MODULE_DEVICE_TABLE(of, mtk_mipi_dphy_of_match); #endif
> > > +
> > > +static struct platform_driver mipi_dphy_pdrv = {
> > > +       .probe  = mipi_dphy_probe,
> > > +       .driver = {
> > > +               .name   = "mipi_dphy",
> > > +               .of_match_table = of_match_ptr(mtk_mipi_dphy_of_match),
> > > +       },
> > > +};
> > > +
> > > +module_platform_driver(mipi_dphy_pdrv);
> > > +