Hi,

Sorry, I missed this email.


W dniu 23.09.2021 o 00:32, Laurent Pinchart pisze:
> Hi Andrzej,
>
> On Wed, Sep 22, 2021 at 04:29:39AM +0300, Laurent Pinchart wrote:
>> On Tue, Sep 21, 2021 at 09:42:11PM +0200, Andrzej Hajda wrote:
>>> W dniu 23.06.2021 o 15:56, Laurent Pinchart pisze:
>>>> From: LUU HOAI <hoai.luu...@renesas.com>
>>>>
>>>> The driver supports the MIPI DSI/CSI-2 TX encoder found in the R-Car V3U
>>>> SoC. It currently supports DSI mode only.
>>>>
>>>> Signed-off-by: LUU HOAI <hoai.luu...@renesas.com>
>>>> Signed-off-by: Laurent Pinchart <laurent.pinchart+rene...@ideasonboard.com>
>>>> Reviewed-by: Kieran Bingham <kieran.bingham+rene...@ideasonboard.com>
>>>> Tested-by: Kieran Bingham <kieran.bingham+rene...@ideasonboard.com>
>>>> ---
>>>>    drivers/gpu/drm/rcar-du/Kconfig              |   6 +
>>>>    drivers/gpu/drm/rcar-du/Makefile             |   1 +
>>>>    drivers/gpu/drm/rcar-du/rcar_mipi_dsi.c      | 827 +++++++++++++++++++
>>>>    drivers/gpu/drm/rcar-du/rcar_mipi_dsi_regs.h | 172 ++++
>>>>    4 files changed, 1006 insertions(+)
>>>>    create mode 100644 drivers/gpu/drm/rcar-du/rcar_mipi_dsi.c
>>>>    create mode 100644 drivers/gpu/drm/rcar-du/rcar_mipi_dsi_regs.h
...
>>>> +
>>>> +  ret = drm_of_find_panel_or_bridge(dsi->dev->of_node, 1, 0, &panel,
>>>> +                                    &dsi->next_bridge);
>>> You are looking for sink but DSI host is not yet registered, thus DSI
>>> child devices not yet created/bound,  so in case of DSI-controlled sinks
>>> it will be always error.
>> Correct, it will not work for a sink that is controlled through DSI.
>> We've tested this with a sink controlled through I2C, as that's all we
>> have on the development board. That won't be very future-proof of
>> course.
>>
>>> Please look at pending documentation patch[1] for more in-depth explanation.
>>>
>>> [1]: 
>>> https://protect2.fireeye.com/v1/url?k=ccc70571-935c3c5e-ccc68e3e-0cc47a31cdf8-cd122187fddf557d&q=1&e=311a381f-74cc-4b35-a344-362bc742c941&u=https%3A%2F%2Flkml.org%2Flkml%2F2021%2F9%2F10%2F165
>> I'll review that series.
> To clarify your point, do you consider this a blocker for merging this
> series, or something that can be addressed on top ?


The best would be to fix it before merge - the rule of thumb is that bad 
patterns spread quite fast :)

If there is good reason to postpone the fix, please send it ASAP.


Regards

Andrzej

>
>>>> +  if (ret) {
>>>> +          dev_err_probe(dsi->dev, ret, "could not find next bridge\n");
>>>> +          return ret;
>>>> +  }
>>>> +
>>>> +  if (!dsi->next_bridge) {
>>>> +          dsi->next_bridge = devm_drm_panel_bridge_add(dsi->dev, panel);
>>>> +          if (IS_ERR(dsi->next_bridge)) {
>>>> +                  dev_err(dsi->dev, "failed to create panel bridge\n");
>>>> +                  return PTR_ERR(dsi->next_bridge);
>>>> +          }
>>>> +  }
>>>> +
>>>> +  /* Initialize the DSI host. */
>>>> +  dsi->host.dev = dsi->dev;
>>>> +  dsi->host.ops = &rcar_mipi_dsi_host_ops;
>>>> +  ret = mipi_dsi_host_register(&dsi->host);
>>>> +  if (ret < 0)
>>>> +          return ret;
>>>> +
>>>> +  /* Initialize the DRM bridge. */
>>>> +  dsi->bridge.funcs = &rcar_mipi_dsi_bridge_ops;
>>>> +  dsi->bridge.of_node = dsi->dev->of_node;
>>>> +  drm_bridge_add(&dsi->bridge);
>>>> +
>>>> +  return 0;
>>>> +}
>>>> +
>>>> +static int rcar_mipi_dsi_remove(struct platform_device *pdev)
>>>> +{
>>>> +  struct rcar_mipi_dsi *dsi = platform_get_drvdata(pdev);
>>>> +
>>>> +  drm_bridge_remove(&dsi->bridge);
>>>> +
>>>> +  mipi_dsi_host_unregister(&dsi->host);
>>>> +
>>>> +  return 0;
>>>> +}
>>>> +
>>>> +static const struct of_device_id rcar_mipi_dsi_of_table[] = {
>>>> +  { .compatible = "renesas,r8a779a0-dsi-csi2-tx" },
>>>> +  { }
>>>> +};
>>>> +
>>>> +MODULE_DEVICE_TABLE(of, rcar_mipi_dsi_of_table);
>>>> +
>>>> +static struct platform_driver rcar_mipi_dsi_platform_driver = {
>>>> +  .probe          = rcar_mipi_dsi_probe,
>>>> +  .remove         = rcar_mipi_dsi_remove,
>>>> +  .driver         = {
>>>> +          .name   = "rcar-mipi-dsi",
>>>> +          .of_match_table = rcar_mipi_dsi_of_table,
>>>> +  },
>>>> +};
>>>> +
>>>> +module_platform_driver(rcar_mipi_dsi_platform_driver);
>>>> +
>>>> +MODULE_DESCRIPTION("Renesas R-Car MIPI DSI Encoder Driver");
>>>> +MODULE_LICENSE("GPL");
>>>> diff --git a/drivers/gpu/drm/rcar-du/rcar_mipi_dsi_regs.h 
>>>> b/drivers/gpu/drm/rcar-du/rcar_mipi_dsi_regs.h
>>>> new file mode 100644
>>>> index 000000000000..0e7a9274749f
>>>> --- /dev/null
>>>> +++ b/drivers/gpu/drm/rcar-du/rcar_mipi_dsi_regs.h
>>>> @@ -0,0 +1,172 @@
>>>> +/* SPDX-License-Identifier: GPL-2.0 */
>>>> +/*
>>>> + * rcar_mipi_dsi_regs.h  --  R-Car MIPI DSI Interface Registers 
>>>> Definitions
>>>> + *
>>>> + * Copyright (C) 2020 Renesas Electronics Corporation
>>>> + */
>>>> +
>>>> +#ifndef __RCAR_MIPI_DSI_REGS_H__
>>>> +#define __RCAR_MIPI_DSI_REGS_H__
>>>> +
>>>> +#define LINKSR                            0x010
>>>> +#define LINKSR_LPBUSY                     (1 << 1)
>>>> +#define LINKSR_HSBUSY                     (1 << 0)
>>>> +
>>>> +/*
>>>> + * Video Mode Register
>>>> + */
>>>> +#define TXVMSETR                  0x180
>>>> +#define TXVMSETR_SYNSEQ_PULSES            (0 << 16)
>>>> +#define TXVMSETR_SYNSEQ_EVENTS            (1 << 16)
>>>> +#define TXVMSETR_VSTPM                    (1 << 15)
>>>> +#define TXVMSETR_PIXWDTH          (1 << 8)
>>>> +#define TXVMSETR_VSEN_EN          (1 << 4)
>>>> +#define TXVMSETR_VSEN_DIS         (0 << 4)
>>>> +#define TXVMSETR_HFPBPEN_EN               (1 << 2)
>>>> +#define TXVMSETR_HFPBPEN_DIS              (0 << 2)
>>>> +#define TXVMSETR_HBPBPEN_EN               (1 << 1)
>>>> +#define TXVMSETR_HBPBPEN_DIS              (0 << 1)
>>>> +#define TXVMSETR_HSABPEN_EN               (1 << 0)
>>>> +#define TXVMSETR_HSABPEN_DIS              (0 << 0)
>>>> +
>>>> +#define TXVMCR                            0x190
>>>> +#define TXVMCR_VFCLR                      (1 << 12)
>>>> +#define TXVMCR_EN_VIDEO                   (1 << 0)
>>>> +
>>>> +#define TXVMSR                            0x1a0
>>>> +#define TXVMSR_STR                        (1 << 16)
>>>> +#define TXVMSR_VFRDY                      (1 << 12)
>>>> +#define TXVMSR_ACT                        (1 << 8)
>>>> +#define TXVMSR_RDY                        (1 << 0)
>>>> +
>>>> +#define TXVMSCR                           0x1a4
>>>> +#define TXVMSCR_STR                       (1 << 16)
>>>> +
>>>> +#define TXVMPSPHSETR                      0x1c0
>>>> +#define TXVMPSPHSETR_DT_RGB16             (0x0e << 16)
>>>> +#define TXVMPSPHSETR_DT_RGB18             (0x1e << 16)
>>>> +#define TXVMPSPHSETR_DT_RGB18_LS  (0x2e << 16)
>>>> +#define TXVMPSPHSETR_DT_RGB24             (0x3e << 16)
>>>> +#define TXVMPSPHSETR_DT_YCBCR16           (0x2c << 16)
>>>> +
>>>> +#define TXVMVPRMSET0R                     0x1d0
>>>> +#define TXVMVPRMSET0R_HSPOL_HIG           (0 << 17)
>>>> +#define TXVMVPRMSET0R_HSPOL_LOW           (1 << 17)
>>>> +#define TXVMVPRMSET0R_VSPOL_HIG           (0 << 16)
>>>> +#define TXVMVPRMSET0R_VSPOL_LOW           (1 << 16)
>>>> +#define TXVMVPRMSET0R_CSPC_RGB            (0 << 4)
>>>> +#define TXVMVPRMSET0R_CSPC_YCbCr  (1 << 4)
>>>> +#define TXVMVPRMSET0R_BPP_16              (0 << 0)
>>>> +#define TXVMVPRMSET0R_BPP_18              (1 << 0)
>>>> +#define TXVMVPRMSET0R_BPP_24              (2 << 0)
>>>> +
>>>> +#define TXVMVPRMSET1R                     0x1d4
>>>> +#define TXVMVPRMSET1R_VACTIVE(x)  (((x) & 0x7fff) << 16)
>>>> +#define TXVMVPRMSET1R_VSA(x)              (((x) & 0xfff) << 0)
>>>> +
>>>> +#define TXVMVPRMSET2R                     0x1d8
>>>> +#define TXVMVPRMSET2R_VFP(x)              (((x) & 0x1fff) << 16)
>>>> +#define TXVMVPRMSET2R_VBP(x)              (((x) & 0x1fff) << 0)
>>>> +
>>>> +#define TXVMVPRMSET3R                     0x1dc
>>>> +#define TXVMVPRMSET3R_HACTIVE(x)  (((x) & 0x7fff) << 16)
>>>> +#define TXVMVPRMSET3R_HSA(x)              (((x) & 0xfff) << 0)
>>>> +
>>>> +#define TXVMVPRMSET4R                     0x1e0
>>>> +#define TXVMVPRMSET4R_HFP(x)              (((x) & 0x1fff) << 16)
>>>> +#define TXVMVPRMSET4R_HBP(x)              (((x) & 0x1fff) << 0)
>>>> +
>>>> +/*
>>>> + * PHY-Protocol Interface (PPI) Registers
>>>> + */
>>>> +#define PPISETR                           0x700
>>>> +#define PPISETR_DLEN_0                    (0x1 << 0)
>>>> +#define PPISETR_DLEN_1                    (0x3 << 0)
>>>> +#define PPISETR_DLEN_2                    (0x7 << 0)
>>>> +#define PPISETR_DLEN_3                    (0xf << 0)
>>>> +#define PPISETR_CLEN                      (1 << 8)
>>>> +
>>>> +#define PPICLCR                           0x710
>>>> +#define PPICLCR_TXREQHS                   (1 << 8)
>>>> +#define PPICLCR_TXULPSEXT         (1 << 1)
>>>> +#define PPICLCR_TXULPSCLK         (1 << 0)
>>>> +
>>>> +#define PPICLSR                           0x720
>>>> +#define PPICLSR_HSTOLP                    (1 << 27)
>>>> +#define PPICLSR_TOHS                      (1 << 26)
>>>> +#define PPICLSR_STPST                     (1 << 0)
>>>> +
>>>> +#define PPICLSCR                  0x724
>>>> +#define PPICLSCR_HSTOLP                   (1 << 27)
>>>> +#define PPICLSCR_TOHS                     (1 << 26)
>>>> +
>>>> +#define PPIDLSR                           0x760
>>>> +#define PPIDLSR_STPST                     (0xf << 0)
>>>> +
>>>> +/*
>>>> + * Clocks registers
>>>> + */
>>>> +#define LPCLKSET                  0x1000
>>>> +#define LPCLKSET_CKEN                     (1 << 8)
>>>> +#define LPCLKSET_LPCLKDIV(x)              (((x) & 0x3f) << 0)
>>>> +
>>>> +#define CFGCLKSET                 0x1004
>>>> +#define CFGCLKSET_CKEN                    (1 << 8)
>>>> +#define CFGCLKSET_CFGCLKDIV(x)            (((x) & 0x3f) << 0)
>>>> +
>>>> +#define DOTCLKDIV                 0x1008
>>>> +#define DOTCLKDIV_CKEN                    (1 << 8)
>>>> +#define DOTCLKDIV_DOTCLKDIV(x)            (((x) & 0x3f) << 0)
>>>> +
>>>> +#define VCLKSET                           0x100c
>>>> +#define VCLKSET_CKEN                      (1 << 16)
>>>> +#define VCLKSET_COLOR_RGB         (0 << 8)
>>>> +#define VCLKSET_COLOR_YCC         (1 << 8)
>>>> +#define VCLKSET_DIV(x)                    (((x) & 0x3) << 4)
>>>> +#define VCLKSET_BPP_16                    (0 << 2)
>>>> +#define VCLKSET_BPP_18                    (1 << 2)
>>>> +#define VCLKSET_BPP_18L                   (2 << 2)
>>>> +#define VCLKSET_BPP_24                    (3 << 2)
>>>> +#define VCLKSET_LANE(x)                   (((x) & 0x3) << 0)
>>>> +
>>>> +#define VCLKEN                            0x1010
>>>> +#define VCLKEN_CKEN                       (1 << 0)
>>>> +
>>>> +#define PHYSETUP                  0x1014
>>>> +#define PHYSETUP_HSFREQRANGE(x)           (((x) & 0x7f) << 16)
>>>> +#define PHYSETUP_HSFREQRANGE_MASK (0x7f << 16)
>>>> +#define PHYSETUP_CFGCLKFREQRANGE(x)       (((x) & 0x3f) << 8)
>>>> +#define PHYSETUP_SHUTDOWNZ                (1 << 1)
>>>> +#define PHYSETUP_RSTZ                     (1 << 0)
>>>> +
>>>> +#define CLOCKSET1                 0x101c
>>>> +#define CLOCKSET1_LOCK_PHY                (1 << 17)
>>>> +#define CLOCKSET1_LOCK                    (1 << 16)
>>>> +#define CLOCKSET1_CLKSEL          (1 << 8)
>>>> +#define CLOCKSET1_CLKINSEL_EXTAL  (0 << 2)
>>>> +#define CLOCKSET1_CLKINSEL_DIG            (1 << 2)
>>>> +#define CLOCKSET1_CLKINSEL_DU             (1 << 3)
>>>> +#define CLOCKSET1_SHADOW_CLEAR            (1 << 1)
>>>> +#define CLOCKSET1_UPDATEPLL               (1 << 0)
>>>> +
>>>> +#define CLOCKSET2                 0x1020
>>>> +#define CLOCKSET2_M(x)                    (((x) & 0xfff) << 16)
>>>> +#define CLOCKSET2_VCO_CNTRL(x)            (((x) & 0x3f) << 8)
>>>> +#define CLOCKSET2_N(x)                    (((x) & 0xf) << 0)
>>>> +
>>>> +#define CLOCKSET3                 0x1024
>>>> +#define CLOCKSET3_PROP_CNTRL(x)           (((x) & 0x3f) << 24)
>>>> +#define CLOCKSET3_INT_CNTRL(x)            (((x) & 0x3f) << 16)
>>>> +#define CLOCKSET3_CPBIAS_CNTRL(x) (((x) & 0x7f) << 8)
>>>> +#define CLOCKSET3_GMP_CNTRL(x)            (((x) & 0x3) << 0)
>>>> +
>>>> +#define PHTW                              0x1034
>>>> +#define PHTW_DWEN                 (1 << 24)
>>>> +#define PHTW_TESTDIN_DATA(x)              (((x) & 0xff) << 16)
>>>> +#define PHTW_CWEN                 (1 << 8)
>>>> +#define PHTW_TESTDIN_CODE(x)              (((x) & 0xff) << 0)
>>>> +
>>>> +#define PHTC                              0x103c
>>>> +#define PHTC_TESTCLR                      (1 << 0)
>>>> +
>>>> +#endif /* __RCAR_MIPI_DSI_REGS_H__ */

Reply via email to