Re: [PATCH v2 2/2] drm: rcar-du: Add R-Car DSI driver

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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.ub@xxxxxxxxxxx>
>>>>
>>>> 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.ub@xxxxxxxxxxx>
>>>> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@xxxxxxxxxxxxxxxx>
>>>> Reviewed-by: Kieran Bingham <kieran.bingham+renesas@xxxxxxxxxxxxxxxx>
>>>> Tested-by: Kieran Bingham <kieran.bingham+renesas@xxxxxxxxxxxxxxxx>
>>>> ---
>>>>    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__ */



[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux