Re: [PATCH v3 1/4] drm: rockchip: introduce rk3066 hdmi

Hi Johan.

Thanks for this nive driver.
A few review comments follows.

	Sam

On Sat, Mar 02, 2019 at 11:41:13AM +0100, Johan Jonker wrote:

From: Zheng Yang <redacted>

Introduce rk3066 hdmi.

A little more info would be good here.
Do not assume all reader knows as much as you do.

Please do not use drmP.h in new files. The usage of drmP.h is deprecated.
Also please sort the include files, but keep the nice grouping.

+
+#include "rockchip_drm_drv.h"
+#include "rockchip_drm_vop.h"
+
+#include "rk3066_hdmi.h"
+
+#define DEFAULT_PLLA_RATE 30000000
+
+struct hdmi_data_info {
+	int vic;

vic is used so much. It deserves an explanation.

+	bool sink_is_hdmi;
+	unsigned int enc_in_format;

enc_in_format is in this patch only assinged.
aybe drop it (if not used in later patches)

+	unsigned int enc_out_format;
+	unsigned int colorimetry;
+};
+
+struct rk3066_hdmi_i2c {
+	struct i2c_adapter adap;
+
+	u8 ddc_addr;
+	u8 segment_addr;

The two members above are only used in rk3066_hdmi_i2c_write()
Maybe they can be made local variables?

+	u8 stat;
+
+	struct mutex lock; /*for i2c operation*/

Name the lock after what is protects, to avoid mis-use.

+	struct completion cmp;

cmp is shorthand for "compare" - as we have a command named so.
Find a better name.

+};
+
+struct rk3066_hdmi {
+	struct device *dev;
+	struct drm_device *drm_dev;

The new way to do this is to embed the device.
See latest patches by Noralf in drm-misc-next, which include a nice example.

+	struct regmap *regmap;

+1 for using regmap.
But then there is still several readl_relaxed() writel_relaxed() calls?
They are in hdmi_readb() and hdmi_writeb(). Should a regmap had been used here too?

+	int irq;
+	struct clk *hclk;
+	void __iomem *regs;
+
+	struct drm_connector connector;
+	struct drm_encoder encoder;
+
+	struct rk3066_hdmi_i2c *i2c;
+	struct i2c_adapter *ddc;
+
+	unsigned int tmdsclk;
+
+	struct hdmi_data_info hdmi_data;
+	struct drm_display_mode previous_mode;
+};
+
+#define to_rk3066_hdmi(x) container_of(x, struct rk3066_hdmi, x)
+
+static inline u8 hdmi_readb(struct rk3066_hdmi *hdmi, u16 offset)
+{
+	return readl_relaxed(hdmi->regs + offset);
+}
+
+static inline void hdmi_writeb(struct rk3066_hdmi *hdmi, u16 offset, u32 val)
+{
+	writel_relaxed(val, hdmi->regs + offset);
+}
+
+static inline void hdmi_modb(struct rk3066_hdmi *hdmi, u16 offset,
+			     u32 msk, u32 val)
+{
+	u8 temp = hdmi_readb(hdmi, offset) & ~msk;
+
+	temp |= val & msk;
+	hdmi_writeb(hdmi, offset, temp);
+}
+
+static void rk3066_hdmi_i2c_init(struct rk3066_hdmi *hdmi)
+{
+	int ddc_bus_freq;
+
+	ddc_bus_freq = (hdmi->tmdsclk >> 2) / HDMI_SCL_RATE;
+
+	hdmi_writeb(hdmi, HDMI_DDC_BUS_FREQ_L, ddc_bus_freq & 0xFF);
+	hdmi_writeb(hdmi, HDMI_DDC_BUS_FREQ_H, (ddc_bus_freq >> 8) & 0xFF);
+
+	/* Clear the EDID interrupt flag and mute the interrupt */
+	hdmi_modb(hdmi, HDMI_INTR_MASK1, HDMI_INTR_EDID_MASK, 0);
+	hdmi_writeb(hdmi, HDMI_INTR_STATUS1, HDMI_INTR_EDID_MASK);
+}
+
+static inline u8 rk3066_hdmi_get_power_mode(struct rk3066_hdmi *hdmi)
+{
+	return hdmi_readb(hdmi, HDMI_SYS_CTRL) & HDMI_SYS_POWER_MODE_MASK;
+}
+
+static void rk3066_hdmi_set_power_mode(struct rk3066_hdmi *hdmi, int mode)
+{
+	u8 current_mode, next_mode;
+	u8 i = 0;
+
+	current_mode = rk3066_hdmi_get_power_mode(hdmi);
+
+	dev_dbg(hdmi->dev, "mode         :%d\n", mode);
+	dev_dbg(hdmi->dev, "current_mode :%d\n", current_mode);
+
+	if (current_mode == mode)
+		return;
+
+	do {
+		if (current_mode > mode)
+			next_mode = current_mode / 2;
+		else {
+			if (current_mode < HDMI_SYS_POWER_MODE_A)
+				next_mode = HDMI_SYS_POWER_MODE_A;
+			else
+				next_mode = current_mode * 2;
+		}
+
+		dev_dbg(hdmi->dev, "%d: next_mode :%d\n", i, next_mode);
+
+		if (next_mode != HDMI_SYS_POWER_MODE_D) {
+			hdmi_modb(hdmi, HDMI_SYS_CTRL,
+				  HDMI_SYS_POWER_MODE_MASK, next_mode);
+		} else {
+			hdmi_writeb(hdmi, HDMI_SYS_CTRL,
+				    HDMI_SYS_POWER_MODE_D |
+				    HDMI_SYS_PLL_RESET_MASK);
+			usleep_range(90, 100);
+			hdmi_writeb(hdmi, HDMI_SYS_CTRL,
+				    HDMI_SYS_POWER_MODE_D |
+				    HDMI_SYS_PLLB_RESET);
+			usleep_range(90, 100);
+			hdmi_writeb(hdmi, HDMI_SYS_CTRL,
+				    HDMI_SYS_POWER_MODE_D);
+		}
+		current_mode = next_mode;
+		i = i + 1;
+	} while ((next_mode != mode) && (i < 5));
+
+	/*
+	 * When IP controller haven't configured to an accurate video
+	 * timing, DDC_CLK is equal to PLLA freq which is 30MHz,so we
+	 * need to init the TMDS rate to PCLK rate, and reconfigure
+	 * the DDC clock.
+	 */
+	if (mode < HDMI_SYS_POWER_MODE_D)
+		hdmi->tmdsclk = DEFAULT_PLLA_RATE;
+}
+
+static int
+rk3066_hdmi_upload_frame(struct rk3066_hdmi *hdmi, int setup_rc,
+			 union hdmi_infoframe *frame, u32 frame_index,
+			 u32 mask, u32 disable, u32 enable)
+{
+	if (mask)
+		hdmi_modb(hdmi, HDMI_CP_AUTO_SEND_CTRL, mask, disable);
+
+	hdmi_writeb(hdmi, HDMI_CP_BUF_INDEX, frame_index);
+
+	if (setup_rc >= 0) {
+		u8 packed_frame[HDMI_MAXIMUM_INFO_FRAME_SIZE];
+		ssize_t rc, i;
+
+		rc = hdmi_infoframe_pack(frame, packed_frame,
+					 sizeof(packed_frame));
+		if (rc < 0)
+			return rc;
+
+		for (i = 0; i < rc; i++)
+			hdmi_writeb(hdmi, HDMI_CP_BUF_ACC_HB0 + i * 4,
+				    packed_frame[i]);
+
+		if (mask)
+			hdmi_modb(hdmi, HDMI_CP_AUTO_SEND_CTRL, mask, enable);
+	}
+
+	return setup_rc;
+}
+
+static int rk3066_hdmi_config_avi(struct rk3066_hdmi *hdmi,
+				  struct drm_display_mode *mode)
+{
+	union hdmi_infoframe frame;
+	int rc;
+
+	rc = drm_hdmi_avi_infoframe_from_display_mode(&frame.avi,
+						      &hdmi->connector, mode);
+
+	if (hdmi->hdmi_data.enc_out_format == HDMI_COLORSPACE_YUV444)
+		frame.avi.colorspace = HDMI_COLORSPACE_YUV444;
+	else if (hdmi->hdmi_data.enc_out_format == HDMI_COLORSPACE_YUV422)
+		frame.avi.colorspace = HDMI_COLORSPACE_YUV422;
+	else
+		frame.avi.colorspace = HDMI_COLORSPACE_RGB;
+
+	frame.avi.colorimetry = hdmi->hdmi_data.colorimetry;
+	frame.avi.scan_mode = HDMI_SCAN_MODE_NONE;
+
+	return rk3066_hdmi_upload_frame(hdmi, rc, &frame,
+					HDMI_INFOFRAME_AVI, 0, 0, 0);
+}
+
+static int rk3066_hdmi_config_video_timing(struct rk3066_hdmi *hdmi,
+					   struct drm_display_mode *mode)
+{
+	int value, vsync_offset;
+
+	/* Set detail external video timing polarity and interlace mode */
+	value = HDMI_EXT_VIDEO_SET_EN;
+	value |= mode->flags & DRM_MODE_FLAG_PHSYNC ?
+		 HDMI_VIDEO_HSYNC_ACTIVE_HIGH : HDMI_VIDEO_HSYNC_ACTIVE_LOW;
+	value |= mode->flags & DRM_MODE_FLAG_PVSYNC ?
+		 HDMI_VIDEO_VSYNC_ACTIVE_HIGH : HDMI_VIDEO_VSYNC_ACTIVE_LOW;
+	value |= mode->flags & DRM_MODE_FLAG_INTERLACE ?
+		 HDMI_VIDEO_MODE_INTERLACE : HDMI_VIDEO_MODE_PROGRESSIVE;
+	if (hdmi->hdmi_data.vic == 2 || hdmi->hdmi_data.vic == 3)
+		vsync_offset = 6;
+	else
+		vsync_offset = 0;
+	value |= vsync_offset << 4; 

Replace 4 with HDMI_VIDEO_VSYNC_OFFSET_SHIFT??

+	hdmi_writeb(hdmi, HDMI_EXT_VIDEO_PARA, value);
+

 +

+static int rk3066_hdmi_bind(struct device *dev, struct device *master,
+			    void *data)
+{
+	struct platform_device *pdev = to_platform_device(dev);
+	struct drm_device *drm = data;
+	struct rk3066_hdmi *hdmi;
+	struct resource *iores;
+	int irq;
+	int ret;
+
+	hdmi = devm_kzalloc(dev, sizeof(*hdmi), GFP_KERNEL);
+	if (!hdmi)
+		return -ENOMEM;
+
+	hdmi->dev = dev;
+	hdmi->drm_dev = drm;
+
+	iores = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	if (!iores)
+		return -ENXIO;
+
+	hdmi->regs = devm_ioremap_resource(dev, iores);
+	if (IS_ERR(hdmi->regs))
+		return PTR_ERR(hdmi->regs);
+
+	irq = platform_get_irq(pdev, 0);
+	if (irq < 0)
+		return irq;
+
+	hdmi->hclk = devm_clk_get(hdmi->dev, "hclk");
+	if (IS_ERR(hdmi->hclk)) {
+		dev_err(hdmi->dev, "unable to get HDMI hclk clock\n");
+		return PTR_ERR(hdmi->hclk);
+	}
+
+	ret = clk_prepare_enable(hdmi->hclk);
+	if (ret) {
+		dev_err(hdmi->dev, "cannot enable HDMI hclk clock: %d\n", ret);
+		return ret;
+	}
+
+	hdmi->regmap =
+		syscon_regmap_lookup_by_phandle(hdmi->dev->of_node,
+						"rockchip,grf");

dev->of_node would be fine here. No reason to go via hdmi->

+	if (IS_ERR(hdmi->regmap)) {
+		dev_err(hdmi->dev, "unable to get rockchip,grf\n");
+		ret = PTR_ERR(hdmi->regmap);
+		goto err_disable_hclk;
+	}
+
+	/* internal hclk = hdmi_hclk / 25 */
+	hdmi_writeb(hdmi, HDMI_INTERNAL_CLK_DIVIDER, 25);
+
+	hdmi->ddc = rk3066_hdmi_i2c_adapter(hdmi);
+	if (IS_ERR(hdmi->ddc)) {
+		ret = PTR_ERR(hdmi->ddc);
+		hdmi->ddc = NULL;
+		goto err_disable_hclk;
+	}
+
+	rk3066_hdmi_set_power_mode(hdmi, HDMI_SYS_POWER_MODE_B);
+	usleep_range(999, 1000);
+	hdmi_writeb(hdmi, HDMI_INTR_MASK1, HDMI_INTR_HOTPLUG);
+	hdmi_writeb(hdmi, HDMI_INTR_MASK2, 0);
+	hdmi_writeb(hdmi, HDMI_INTR_MASK3, 0);
+	hdmi_writeb(hdmi, HDMI_INTR_MASK4, 0);
+	rk3066_hdmi_set_power_mode(hdmi, HDMI_SYS_POWER_MODE_A);
+
+	ret = rk3066_hdmi_register(drm, hdmi);
+	if (ret)
+		goto err_disable_hclk;
+
+	dev_set_drvdata(dev, hdmi);
+
+	ret = devm_request_threaded_irq(dev, irq, rk3066_hdmi_hardirq,
+					rk3066_hdmi_irq, IRQF_SHARED,
+					dev_name(dev), hdmi);
+	if (ret) {
+		dev_err(hdmi->dev,
+			"failed to request hdmi irq: %d\n", ret);
+		goto err_disable_hclk;
+	}
+
+	return 0;
+
+err_disable_hclk:
+	clk_disable_unprepare(hdmi->hclk);
+
+	return ret;
+}
+
+static void rk3066_hdmi_unbind(struct device *dev, struct device *master,
+			       void *data)
+{
+	struct rk3066_hdmi *hdmi = dev_get_drvdata(dev);
+
+	hdmi->connector.funcs->destroy(&hdmi->connector);
+	hdmi->encoder.funcs->destroy(&hdmi->encoder);

I think the destroy() function should not be called directly.

+
+	clk_disable_unprepare(hdmi->hclk);
+	i2c_put_adapter(hdmi->ddc);
+}
+
+static const struct component_ops rk3066_hdmi_ops = {
+	.bind	= rk3066_hdmi_bind,
+	.unbind	= rk3066_hdmi_unbind,
+};
+
+static int rk3066_hdmi_probe(struct platform_device *pdev)
+{
+	return component_add(&pdev->dev, &rk3066_hdmi_ops);
+}
+
+static int rk3066_hdmi_remove(struct platform_device *pdev)
+{
+	component_del(&pdev->dev, &rk3066_hdmi_ops);
+
+	return 0;
+}
+
+static const struct of_device_id rk3066_hdmi_dt_ids[] = {
+	{ .compatible = "rockchip,rk3066-hdmi",
+	},

MAke this a one-liner.

+	{},

Us { /* sentinal */ }, like most other drivers.

+	.driver = {
+		.name = "rockchip-rk3066hdmi",

Different naming are used for the driver in this file.
Coniser using a macro to align the naming.

+		.of_match_table = rk3066_hdmi_dt_ids,
+	},
+};

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