Re: [PATCH v13 06/14] drm/mediatek: Add HDMI support

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

 




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@xxxxxxxxxxxxxx> 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_frequency
> > +                                       aud_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

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]
  Powered by Linux