Hi Daniel, Jie,
Am Mittwoch, den 09.03.2016, 21:52 +0800 schrieb Daniel Kurtz:
Hi Philipp & Jie,
Sorry I only now had a chance to dig deeper and review the HDMI driver.
I wish you had had a chance to do this earlier, But better now than after the merge. I'll split the HDMI patches from the others in the next round.
Lots of comments inline below...
On Tue, Mar 8, 2016 at 9:27 PM, Philipp Zabel p.zabel@pengutronix.de wrote:
[...]
diff --git a/drivers/gpu/drm/mediatek/mtk_cec.c b/drivers/gpu/drm/mediatek/mtk_cec.c new file mode 100644 index 0000000..cba3647 --- /dev/null +++ b/drivers/gpu/drm/mediatek/mtk_cec.c +void mtk_cec_set_hpd_event(struct device *dev,
void (*hpd_event)(bool hpd, struct device *dev),struct device *hdmi_dev)+{
struct mtk_cec *cec = dev_get_drvdata(dev);cec->hdmi_dev = hdmi_dev;cec->hpd_event = hpd_event;Lock this so to prevent race with the irq?
Yes.
[...]
+int mtk_cec_irq(struct device *dev)
AFAICT, this function is never used.
Correct, since the IRQ number is not exported to the sound drivers anymore, I can drop it now.
[...]
+static void mtk_cec_htplg_irq_enable(struct mtk_cec *cec) +{
mtk_cec_mask(cec, CEC_CKGEN, 0, PDN);mtk_cec_mask(cec, CEC_CKGEN, CEC_32K_PDN, CEC_32K_PDN);mtk_cec_mask(cec, RX_GEN_WD, HDMI_PORD_INT_32K_CLR,HDMI_PORD_INT_32K_CLR);mtk_cec_mask(cec, RX_GEN_WD, RX_INT_32K_CLR, RX_INT_32K_CLR);mtk_cec_mask(cec, RX_GEN_WD, HDMI_HTPLG_INT_32K_CLR,HDMI_HTPLG_INT_32K_CLR);mtk_cec_mask(cec, RX_GEN_WD, 0, HDMI_PORD_INT_32K_CLR);mtk_cec_mask(cec, RX_GEN_WD, 0, RX_INT_32K_CLR);mtk_cec_mask(cec, RX_GEN_WD, 0, HDMI_HTPLG_INT_32K_CLR);mtk_cec_mask(cec, RX_GEN_WD, 0, HDMI_PORD_INT_32K_EN);mtk_cec_mask(cec, RX_GEN_WD, 0, RX_INT_32K_EN);mtk_cec_mask(cec, RX_GEN_WD, 0, HDMI_HTPLG_INT_32K_EN);This is a bit wasteful. Can you just clear all of these bits in a single write? (this applies to this entire file).
I think so. If there are no problems, I'll combine them into a single update per register.
mtk_cec_mask(cec, RX_EVENT, HDMI_PORD_INT_EN, HDMI_PORD_INT_EN);mtk_cec_mask(cec, RX_EVENT, HDMI_HTPLG_INT_EN, HDMI_HTPLG_INT_EN);+}
+static void mtk_cec_htplg_irq_disable(struct mtk_cec *cec) +{
Why does irq_enable do so much more work than irq_disable?
It also clears the interrupt status and sets the clock. I'll move the initialization out of this function.
mtk_cec_mask(cec, RX_EVENT, 0, HDMI_PORD_INT_EN);mtk_cec_mask(cec, RX_EVENT, 0, HDMI_HTPLG_INT_EN);+}
+static void mtk_cec_clear_htplg_irq(struct mtk_cec *cec) +{
mtk_cec_mask(cec, TR_CONFIG, CLEAR_CEC_IRQ, CLEAR_CEC_IRQ);mtk_cec_mask(cec, NORMAL_INT_CTRL, HDMI_HTPLG_INT_CLR,HDMI_HTPLG_INT_CLR);mtk_cec_mask(cec, NORMAL_INT_CTRL, HDMI_PORD_INT_CLR,HDMI_PORD_INT_CLR);mtk_cec_mask(cec, NORMAL_INT_CTRL, HDMI_FULL_INT_CLR,HDMI_FULL_INT_CLR);mtk_cec_mask(cec, RX_GEN_WD, HDMI_PORD_INT_32K_CLR,HDMI_PORD_INT_32K_CLR);mtk_cec_mask(cec, RX_GEN_WD, RX_INT_32K_CLR, RX_INT_32K_CLR);mtk_cec_mask(cec, RX_GEN_WD, HDMI_HTPLG_INT_32K_CLR,HDMI_HTPLG_INT_32K_CLR);udelay(5);Do you really need this delay in the middle of the isr handler?
I can turn it into an usleep_range(5, 10). Whether the delay is needed at all or how long it really has to be, I don't know.
[...]
+static irqreturn_t mtk_cec_htplg_isr_thread(int irq, void *arg) +{
struct device *dev = arg;struct mtk_cec *cec = dev_get_drvdata(dev);bool hpd;mtk_cec_clear_htplg_irq(cec);hpd = mtk_cec_hpd_high(dev);if (cec->hpd != hpd) {dev_info(dev, "hotplug event!,cur hpd = %d, hpd = %d\n",cec->hpd, hpd);dev_dbg if anything
Ok.
[...]
diff --git a/drivers/gpu/drm/mediatek/mtk_drm_hdmi_drv.c b/drivers/gpu/drm/mediatek/mtk_drm_hdmi_drv.c new file mode 100644 index 0000000..ff661e0 --- /dev/null +++ b/drivers/gpu/drm/mediatek/mtk_drm_hdmi_drv.c
[...]
+static void mtk_hdmi_bridge_disable(struct drm_bridge *bridge) +{
struct mtk_hdmi *hdmi = hdmi_ctx_from_bridge(bridge);phy_power_off(hdmi->phy);clk_disable_unprepare(hdmi->clk[MTK_HDMI_CLK_HDMI_PIXEL]);clk_disable_unprepare(hdmi->clk[MTK_HDMI_CLK_HDMI_PLL]);As far as I can tell, __drm_helper_disable_unused_functions() doesn't check if crtc/encoder/bridge are disabled before calling the ->*en/disable*() callbacks.
So, these clk_disable_unprepare() may be called with the HDMI already disabled, trigerring their WARN_ON().
So, perhaps we also need to track enabled/disabled state separately here in the driver.
I'll add that.
[...]
mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);hdmi->regs = devm_ioremap_resource(dev, mem);if (IS_ERR(hdmi->regs)) {dev_err(dev, "Failed to ioremap hdmi_shell: %ld\n",PTR_ERR(hdmi->regs));What is hdmi_shell?
I don't know, I assumed it's just the name of the HDMI control register space.
In any case, I don't think you need to print anything here.
I'll drop the printk.
[...]
+static int mtk_drm_hdmi_remove(struct platform_device *pdev) +{
struct mtk_hdmi *hdmi = platform_get_drvdata(pdev);drm_bridge_remove(&hdmi->bridge);platform_device_unregister(hdmi->audio_pdev);audio_pdev is not set in this patch. Is there more audio stuff that should be removed from this patch?
I suppose I could also remove the hdmi_audio_param struct and simplify mtk_hdmi_aud_set_input() a bit.
platform_set_drvdata(pdev, NULL);I don't think this is necessary.
Right.
[...]
+static int mtk_hdmi_resume(struct device *dev) +{
struct mtk_hdmi *hdmi = dev_get_drvdata(dev);int ret = 0;ret = mtk_hdmi_clk_enable_audio(hdmi);if (ret) {dev_err(dev, "hdmi resume failed!\n");return ret;}mtk_hdmi_power_on(hdmi);mtk_hdmi_output_set_display_mode(hdmi, &hdmi->mode);phy_power_on(hdmi->phy);dev_dbg(dev, "hdmi resume success!\n");return 0;+} +#endif +static SIMPLE_DEV_PM_OPS(mtk_hdmi_pm_ops,
mtk_hdmi_suspend, mtk_hdmi_resume);I do not think these suspend/resume ops are needed. The MTK DRM driver turn off connectors at suspend, and re-enables them at resume.
I have to move the audio clock enabling into the bridge enable/disable to drop these completely, not sure yet if that will have any side effects.
[...]
diff --git a/drivers/gpu/drm/mediatek/mtk_hdmi.c b/drivers/gpu/drm/mediatek/mtk_hdmi.c new file mode 100644 index 0000000..30ec7b5 --- /dev/null +++ b/drivers/gpu/drm/mediatek/mtk_hdmi.c
Hmm... what is the value in splitting this out into its own separate mtk_hdmi.c?
If nobody minds, I'll happily merge mtk_drm_hdmi_drv.c, mtk_hdmi.c and mtk_hdmi_hw.c. The downside is that the file is now >1.8k lines, but I think the amount of newly static functions is worth it.
[...]
+static int mtk_hdmi_aud_set_input(struct mtk_hdmi *hdmi) +{
[...]
mtk_hdmi_hw_aud_set_high_bitrate(hdmi, false);mtk_hdmi_phy_aud_dst_normal_double_enable(hdmi, false);mtk_hdmi_hw_aud_dst_enable(hdmi, false);These three all change the same register. Combine them into a single helper function that just writes GRL_AUDIO_CFG once.
Ok.
[...]
+static int mtk_hdmi_aud_set_src(struct mtk_hdmi *hdmi,
struct drm_display_mode *display_mode)+{
mtk_hdmi_aud_on_off_hw_ncts(hdmi, false);if (hdmi->aud_param.aud_input_type == HDMI_AUD_INPUT_I2S) {switch (hdmi->aud_param.aud_hdmi_fs) {case HDMI_AUDIO_SAMPLE_FREQUENCY_32000:case HDMI_AUDIO_SAMPLE_FREQUENCY_44100:case HDMI_AUDIO_SAMPLE_FREQUENCY_48000:case HDMI_AUDIO_SAMPLE_FREQUENCY_88200:case HDMI_AUDIO_SAMPLE_FREQUENCY_96000:mtk_hdmi_hw_aud_src_off(hdmi);/* mtk_hdmi_hw_aud_src_enable(hdmi, false); */why is this commented out?
Not yet implemented at this point. I'll remove it.
mtk_hdmi_hw_aud_set_mclk(hdmi,hdmi->aud_param.aud_mclk);indentation is funny here
Will fix, and the following similar issues, too.
mtk_hdmi_hw_aud_src_off(hdmi);mtk_hdmi_hw_aud_set_mclk(hdmi,HDMI_AUD_MCLK_128FS);mtk_hdmi_hw_aud_aclk_inv_enable(hdmi, false);These clauses all seem to do the same thing; only the mclk parameter is different. Can you refactor them into a helper function?
Yes, I'll have to integrate a few changes from the "drm/mediatek: hdmi: Add audio interface to the hdmi-codec driver" patch here.
[...]
+int mtk_hdmi_hpd_high(struct mtk_hdmi *hdmi) +{
return hdmi->cec_dev ? mtk_cec_hpd_high(hdmi->cec_dev) : false;I don't think we would get this far if cec_dev was NULL.
You are right, the HDMI device defers probing until it can find the CEC device. I'll drop mtk_hdmi_hpd_high and call mtk_cec_hpd_high directly.
+int mtk_hdmi_output_init(struct mtk_hdmi *hdmi) +{
struct hdmi_audio_param *aud_param = &hdmi->aud_param;if (hdmi->init)return -EINVAL;This check is not needed; this function is only called once, during probe. In fact, I don't think the "->init" field is needed at all.
I agree. mtk_hdmi_output_init is called in probe before drm_bridge_add. mtk_hdmi_output_set_display_mode, which this is supposed to protect, is only called from the bridge_enable callback. Another indication that the code is split over too many files.
[...]
+int mtk_hdmi_output_set_display_mode(struct mtk_hdmi *hdmi,
struct drm_display_mode *mode)+{
int ret;if (!hdmi->init) {Is this possible?
No. I'll remove it.
dev_err(hdmi->dev, "doesn't init hdmi control context!\n");return -EINVAL;}mtk_hdmi_hw_vid_black(hdmi, true);mtk_hdmi_hw_aud_mute(hdmi, true);mtk_hdmi_setup_av_mute_packet(hdmi);phy_power_off(hdmi->phy);ret = mtk_hdmi_video_change_vpll(hdmi,mode->clock * 1000);if (ret) {dev_err(hdmi->dev, "Failed to set vpll: %d\n", ret);cleanup on error?
The only things that happen before are vid_black/aud_mute and PHY power off. I assume neither can reasonably be reverted if we can't even set the PLL correctly?
return ret;}mtk_hdmi_video_set_display_mode(hdmi, mode);phy_power_on(hdmi->phy);mtk_hdmi_aud_output_config(hdmi, mode);mtk_hdmi_setup_audio_infoframe(hdmi);mtk_hdmi_setup_avi_infoframe(hdmi, mode);mtk_hdmi_setup_spd_infoframe(hdmi, "mediatek", "chromebook");what? No. The "product" should refer to the MTK HDMI block.
I need a little help here. "mediatek", "On-chip HDMI"? The Intel driver sets "intel", "Integrated gfx", for example.
diff --git a/drivers/gpu/drm/mediatek/mtk_hdmi.h b/drivers/gpu/drm/mediatek/mtk_hdmi.h new file mode 100644 index 0000000..9403915 --- /dev/null +++ b/drivers/gpu/drm/mediatek/mtk_hdmi.h
[...]
+struct mtk_hdmi {
struct drm_bridge bridge;struct drm_connector conn;struct device *dev;struct phy *phy;struct device *cec_dev;struct i2c_adapter *ddc_adpt;struct clk *clk[MTK_HDMI_CLK_COUNT];+#if defined(CONFIG_DEBUG_FS)
struct dentry *debugfs;+#endif
Remove all of the debugfs stuff from this patch, since it isn't implemented.
Ok.
[...]
diff --git a/drivers/gpu/drm/mediatek/mtk_hdmi_ddc_drv.c b/drivers/gpu/drm/mediatek/mtk_hdmi_ddc_drv.c new file mode 100644 index 0000000..22e5487 --- /dev/null +++ b/drivers/gpu/drm/mediatek/mtk_hdmi_ddc_drv.c
[...]
+struct mtk_hdmi_i2c {
Throughout this driver, I think we should:
s/mtk_hdmi_i2c/mtk_hdmi_ddc
I'm fine with that.
[...]
+static void ddcm_trigger_mode(struct mtk_hdmi_i2c *i2c, int mode) +{
[...]
while (sif_bit_is_set(i2c, DDC_DDCMCTL1, DDCM_TRI)) {timeout -= 2;udelay(2);if (timeout <= 0)break;}Use iopoll?
I'll replace the loop with readl_poll_timeout().
[...]
+static int mtk_hdmi_i2c_probe(struct platform_device *pdev) +{
[...]
i2c->clk = devm_clk_get(dev, "ddc-i2c");if (IS_ERR(i2c->clk)) {dev_err(dev, "get ddc_clk failed : %p ,\n", i2c->clk);nit, no space before ':'
Ok.
[...]
mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);i2c->regs = devm_ioremap_resource(&pdev->dev, mem);if (IS_ERR(i2c->regs)) {dev_err(dev, "get memory source fail!\n");nit: don't really need to print anything here.
Ok.
[...]
strlcpy(i2c->adap.name, "mediatek-hdmi-i2c", sizeof(i2c->adap.name));i2c->adap.owner = THIS_MODULE;i2c->adap.class = I2C_CLASS_DDC;
Ok.
i2c->adap.algo = &mtk_hdmi_i2c_algorithm;i2c->adap.retries = 3;why set this?
Jie, is there a known issue that made it necessary to enable the automatic retries?
[...]
dev_dbg(dev, "i2c->adap: %p\n", &i2c->adap);dev_dbg(dev, "i2c->clk: %p\n", i2c->clk);dev_dbg(dev, "physical adr: 0x%llx, end: 0x%llx\n", mem->start,mem->end);remove these debugging lines.
Ok.
[...]
+static int mtk_hdmi_i2c_remove(struct platform_device *pdev) +{
struct mtk_hdmi_i2c *i2c = platform_get_drvdata(pdev);clk_disable_unprepare(i2c->clk);i2c_del_adapter(&i2c->adap);To match probe order, call i2c_del_adapter() first.
Ok.
[...]
diff --git a/drivers/gpu/drm/mediatek/mtk_hdmi_hw.c b/drivers/gpu/drm/mediatek/mtk_hdmi_hw.c new file mode 100644 index 0000000..99c7ffc --- /dev/null +++ b/drivers/gpu/drm/mediatek/mtk_hdmi_hw.c
What is the value in having this as a separate mtk_hdmi_hw.c file? If these functions were in mtk_hdmi.c, they could all be static, and the compiler could inline them away.
Yes, I'd prefer to merge all three into mtk_hdmi.c.
[...]
+#include "mtk_hdmi_hw.h" +#include "mtk_hdmi_regs.h" +#include "mtk_hdmi.h"
I think these usually go after system includes.
Ok.
[...]
+#define NCTS_BYTES 0x07
move above the functions
Ok.
[...]
switch (frame_type) {case HDMI_INFOFRAME_TYPE_AVI:ctrl_frame_en = CTRL_AVI_EN;ctrl_reg = GRL_CTRL;break;case HDMI_INFOFRAME_TYPE_SPD:ctrl_frame_en = CTRL_SPD_EN;ctrl_reg = GRL_CTRL;break;case HDMI_INFOFRAME_TYPE_AUDIO:ctrl_frame_en = CTRL_AUDIO_EN;ctrl_reg = GRL_CTRL;break;case HDMI_INFOFRAME_TYPE_VENDOR:ctrl_frame_en = VS_EN;ctrl_reg = GRL_ACP_ISRC_CTRL;break;default:Just fall through if none of the above?
These are the only four currently defined. I'll change frame_type to enum hdmi_infoframe_type and drop the default label.
[...]
+void mtk_hdmi_hw_config_sys(struct mtk_hdmi *hdmi) +{
regmap_update_bits(hdmi->sys_regmap, hdmi->sys_offset + HDMI_SYS_CFG20,HDMI_OUT_FIFO_EN | MHL_MODE_ON, 0);mdelay(2);Can this be msleep instead of mdelay? It is a bit rude to hog the CPU for 2 msec.
Yes this is ultimately called by .bridge_enable. Same for the others two mdelay()s.
[...]
+void mtk_hdmi_hw_aud_set_bit_num(struct mtk_hdmi *hdmi,
enum hdmi_audio_sample_size bit_num)+{
u32 val = 0;no 0 init
I'll use a switch statement below instead.
if (bit_num == HDMI_AUDIO_SAMPLE_SIZE_16)val = AOUT_16BIT;else if (bit_num == HDMI_AUDIO_SAMPLE_SIZE_20)val = AOUT_20BIT;else if (bit_num == HDMI_AUDIO_SAMPLE_SIZE_24)val = AOUT_24BIT;mtk_hdmi_mask(hdmi, GRL_AOUT_BNUM_SEL, val, 0x03);+}
+void mtk_hdmi_hw_aud_set_i2s_fmt(struct mtk_hdmi *hdmi,
enum hdmi_aud_i2s_fmt i2s_fmt)+{
u32 val = 0;no 0 init
Ok.
val = mtk_hdmi_read(hdmi, GRL_CFG0);val &= ~0x33;#define this mask
Ok.
[...]
+void mtk_hdmi_hw_aud_set_i2s_chan_num(struct mtk_hdmi *hdmi,
enum hdmi_aud_channel_type channel_type,u8 channel_count)+{
u8 val_1, val_2, val_3, val_4;better: u8 sw[3], uv;
Yes, I'll try to make this function a bit more readable.
if (channel_count == 2) {val_1 = 0x04;val_2 = 0x50;Some #defines with meaningful names would be nice here.
I'll add a few defines. Also I notice that the ch_switch matrix configuration is always the same.
[...]
+void mtk_hdmi_hw_aud_set_input_type(struct mtk_hdmi *hdmi,
enum hdmi_aud_input_type input_type)+{
u32 val = 0;no need to 0 init
Ok.
[...]
+void mtk_hdmi_hw_aud_set_channel_status(struct mtk_hdmi *hdmi,
u8 *l_chan_status, u8 *r_chan_status,enum hdmi_audio_sample_frequencyaud_hdmi_fs)+{
u8 l_status[5];u8 r_status[5];u8 val = 0;no need to 0 init
Ok.
[...]
val = l_chan_status[4];val |= ((~(l_status[3] & 0x0f)) << 4);l_status[4] = val;r_status[4] = val;val = l_status[0];nit: You don't need to bounce through val here. You can just write the *_status[n] value directly.
You are right, I have already fixed this in the later audio patches. I'll reorganize this a bit.
[...]
+void mtk_hdmi_hw_aud_src_reenable(struct mtk_hdmi *hdmi) +{
u32 val;val = mtk_hdmi_read(hdmi, GRL_MIX_CTRL);if (val & MIX_CTRL_SRC_EN) {val &= ~MIX_CTRL_SRC_EN;mtk_hdmi_write(hdmi, GRL_MIX_CTRL, val);usleep_range(255, 512);Those are some very precise values for a range...
Peculiar. If there are no objections, I'll change them to 250-500.
val |= MIX_CTRL_SRC_EN;mtk_hdmi_write(hdmi, GRL_MIX_CTRL, val);}+}
+void mtk_hdmi_hw_aud_src_off(struct mtk_hdmi *hdmi)
Perhaps *_disable() would be more consistent.
Ok. Although the use of disable/reenable here seems to be inverted compared to the common enable/disable pattern.
[...]
+void mtk_hdmi_hw_aud_aclk_inv_enable(struct mtk_hdmi *hdmi, bool enable)
nit: I prefer explicit _enable() and _disable() functions w/out the 'enable' parameter.
This function doesn't enable/disable a clock, it justs sets the ACLK_INV bit, which supposedly determines whether the inverse audio clock is used to latch data when downsampling 192k to 48k in I2S. I'll rename this to mtk_hdmi_hw_aud_set_aclk_inv(). Or I could drop this function when merging hdmi_hw.c into hdmi.c and use something like mtk_hdmi_clear_bits(hdmi, GRL_CFG2, CFG2_ACLK_INV) directly.
[...]
+static unsigned int hdmi_expected_cts(unsigned int audio_sample_rate,
unsigned int tmds_clock, unsigned int n)+{
return DIV_ROUND_CLOSEST_ULL((u64)hdmi_mode_clock_to_hz(tmds_clock) * n,128 * audio_sample_rate);+}
Doug Anderson may have some opinions about how N & CTS are computed.
I intend to use Arnaud's N/CTS helpers instead, when they are merged.
[...]
+static void do_hdmi_hw_aud_set_ncts(struct mtk_hdmi *hdmi, unsigned int n,
unsigned int cts)+{
unsigned char val[NCTS_BYTES];int i;mtk_hdmi_write(hdmi, GRL_NCTS, 0);mtk_hdmi_write(hdmi, GRL_NCTS, 0);mtk_hdmi_write(hdmi, GRL_NCTS, 0);memset(val, 0, sizeof(val));not necessary, since you fill in all 7 bytes anyway.
Ok.
val[0] = (cts >> 24) & 0xff;val[1] = (cts >> 16) & 0xff;val[2] = (cts >> 8) & 0xff;val[3] = cts & 0xff;val[4] = (n >> 16) & 0xff;val[5] = (n >> 8) & 0xff;val[6] = n & 0xff;all of these "& 0xff" are not needed, since val is an unsigned char array.
Even so, this makes it clear what happens. I expect the compiler to optimize them away if possible.
for (i = 0; i < NCTS_BYTES; i++)mtk_hdmi_write(hdmi, GRL_NCTS, val[i]);What an interesting design. We write all 10 bytes to the same register address?
That is what I am told.
In this case, why bother with val at all? Just directly call mtk_hdmi_write() for each of the bytes above.
Yes, that would be better.
diff --git a/drivers/gpu/drm/mediatek/mtk_mt8173_hdmi_phy.c b/drivers/gpu/drm/mediatek/mtk_mt8173_hdmi_phy.c new file mode 100644 index 0000000..5d9f07f --- /dev/null +++ b/drivers/gpu/drm/mediatek/mtk_mt8173_hdmi_phy.c
[...]
+static void mtk_hdmi_pll_unprepare(struct clk_hw *hw) +{
struct mtk_hdmi_phy *hdmi_phy = to_mtk_hdmi_phy(hw);dev_dbg(hdmi_phy->dev, "prepare\n");nit: "unprepare" (or just '"%s\n", __func__)', here and everywhere.
Ok.
+static int mtk_hdmi_pll_set_rate(struct clk_hw *hw, unsigned long rate,
unsigned long parent_rate)+{
struct mtk_hdmi_phy *hdmi_phy = to_mtk_hdmi_phy(hw);unsigned int pre_div;unsigned int div;dev_dbg(hdmi_phy->dev, "set rate : %lu, parent: %lu\n", rate,nit, no space before the first ':'.
Ok.
+static void mtk_hdmi_phy_enable_tmds(struct mtk_hdmi_phy *hdmi_phy) +{
mtk_hdmi_phy_mask(hdmi_phy, HDMI_CON3, RG_HDMITX_SER_EN,RG_HDMITX_SER_EN);nit: lots of these calls might be more readable (and easier to maintain & review) if we used two helper functions:
mtk_hdmi_phy_set_bits(struct mtk_hdmi_phy *hdmi_phy, u32 offset, u32 mask); mtk_hdmi_phy_clr_bits(struct mtk_hdmi_phy *hdmi_phy, u32 offset, u32 mask);
and
mtk_hdmi_set_bits() mtk_hdmi_clr_bits()
It seems I'm too used to regmap_update_bits. I don't find separate set/clear functions easier to read at all, and I stumble over the clr abbreviation. I'll change to _set_bits and _clear_bits functions and hope that I am in the minority.
[...]
+static struct phy_ops mtk_hdmi_phy_ops = {
static const
Ok. Thanks for the review!
regards Philipp