>> +static int edp_bind(struct device *dev, struct device *master, void
>> *data)
>> +{
>> + struct drm_device *drm = dev_get_drvdata(master);
>> + struct msm_drm_private *priv = drm->dev_private;
>> + struct msm_edp *edp;
>> +
>> + DBG("");
>> + edp = edp_init(to_platform_device(dev));
>
> There's a lot of this casting to platform devices and then using
> pdev->dev to get at the struct device. I don't immediately see a use for
> the platform device, so why not just stick with struct device *
> consistently?
>
There are some places in edp_init() to use struct platform_device *.
Also, this is to make edp code consistent with hdmi.
>> + * It will call msm_edp_aux_ctrl() function to reset the aux channel,
>> + * if the waiting is timeout.
>> + * The caller who triggers the transaction should avoid the
>> + * msm_edp_aux_ctrl() running concurrently in other threads, i.e.
>> + * start transaction only when aux channel is fully enabled.
>> + */
>> +ssize_t edp_aux_transfer(struct drm_dp_aux *drm_aux, struct
>> drm_dp_aux_msg *msg)
>> +{
>> + struct edp_aux *aux = to_edp_aux(drm_aux);
>> + ssize_t ret;
>> + bool native = msg->request & (DP_AUX_NATIVE_WRITE &
>> DP_AUX_NATIVE_READ);
>> + bool read = msg->request & (DP_AUX_I2C_READ & DP_AUX_NATIVE_READ);
>
> These checks are confusing. It seems like they might actually work
> because of how these symbols are defined, but I'd expect something like:
>
> native = msg->request & (DP_AUX_NATIVE_WRITE | DP_AUX_NATIVE_READ);
>
I think the above solution will not work because it will take
DP_AUX_I2C_READ as "native".
> Or perhaps even clearer:
>
> switch (msg->request) {
> case DP_AUX_NATIVE_WRITE:
> case DP_AUX_NATIVE_READ:
> native = true;
> break;
>
> ...
> }
>
The switch/case code will work only if we remove other unrelated bits from
input msg->request.
>From my understanding, the idea to define these symbols is to take BIT(7)
as *native* mark and BIT(0) as *read* mark, and the
I2C_WRITE/I2C_READ/NATIVE_WRITE/NATIVE_READ are 4 combinations of these 2
bits. Hence i am still thinking the original way is reflecting the way
they are defined.
>> + /* Ignore address only message */
>> + if ((msg->size == 0) || (msg->buffer == NULL)) {
>> + msg->reply = native ?
>> + DP_AUX_NATIVE_REPLY_ACK : DP_AUX_I2C_REPLY_ACK;
>> + return msg->size;
>> + }
>
> How do you support I2C-over-AUX then? How else will the device know
> which I2C slave to address?
>
H/W takes care of the i2c details. S/W only needs to tell H/W if the
transaction is i2c or native and the address. Please see
edp_msg_fifo_tx().
>> +static int cont_splash; /* 1 to enable continuous splash screen */
>> +EXPORT_SYMBOL(cont_splash);
>> +
>> +module_param(cont_splash, int, 0);
>> +MODULE_PARM_DESC(cont_splash, "Enable continuous splash screen on
>> eDP");
>
> Huh? Is this supposed to allow hand-off from firmware to kernel? If so I
> don't think that's going to work without having proper support for it
> across the driver. I don't see support for this in the MDP subdriver, so
> I doubt that it's going to work at all.
>
> Either way, I don't think using a module parameter for this is the right
> solution.
>
I will remove the cont_splash support for now and will have a new change
to fully support hand-off, including all display subdrivers.
>> +struct edp_ctrl {
>> + struct platform_device *pdev;
>> +
>> + void __iomem *base;
>> +
>> + /* regulators */
>> + struct regulator *vdda_vreg;
>> + struct regulator *lvl_reg;
>> +
>> + /* clocks */
>> + struct clk *aux_clk;
>> + struct clk *pixel_clk;
>> + struct clk *ahb_clk;
>> + struct clk *link_clk;
>> + struct clk *mdp_core_clk;
>> +
>> + /* gpios */
>> + int gpio_panel_en;
>> + int gpio_panel_hpd;
>> + int gpio_lvl_en;
>> + int gpio_bkl_en;
>
> These should really be using the new gpiod_*() API. Also, at least
> panel_en and bkl_en seem wrongly placed. They should be handled in the
> panel and backlight drivers, not the eDP driver.
>
I will move bkl_en to pwm_backlight DT and use gpiod_* APIs. We don't have
a panel driver and hope the eDP driver can support all the panels.
>> +static const struct edp_pixel_clk_div clk_divs[2][EDP_PIXEL_CLK_NUM] =
>> {
>> + { /* Link clock = 162MHz, source clock = 810MHz */
>> + {119000, 31, 211}, /* WSXGA+ 1680x1050@60Hz CVT */
>> + {130250, 32, 199}, /* UXGA 1600x1200@60Hz CVT */
>> + {148500, 11, 60}, /* FHD 1920x1080@60Hz */
>> + {154000, 50, 263}, /* WUXGA 1920x1200@60Hz CVT */
>> + {209250, 31, 120}, /* QXGA 2048x1536@60Hz CVT */
>> + {268500, 119, 359}, /* WQXGA 2560x1600@60Hz CVT */
>> + {138530, 33, 193}, /* AUO B116HAN03.0 Panel */
>> + {141400, 48, 275}, /* AUO B133HTN01.2 Panel */
>> + },
>> + { /* Link clock = 270MHz, source clock = 675MHz */
>> + {119000, 52, 295}, /* WSXGA+ 1680x1050@60Hz CVT */
>> + {130250, 11, 57}, /* UXGA 1600x1200@60Hz CVT */
>> + {148500, 11, 50}, /* FHD 1920x1080@60Hz */
>> + {154000, 47, 206}, /* WUXGA 1920x1200@60Hz CVT */
>> + {209250, 31, 100}, /* QXGA 2048x1536@60Hz CVT */
>> + {268500, 107, 269}, /* WQXGA 2560x1600@60Hz CVT */
>> + {138530, 63, 307}, /* AUO B116HAN03.0 Panel */
>> + {141400, 53, 253}, /* AUO B133HTN01.2 Panel */
>> + },
>> +};
>
> Can't you compute these programmatically? If you rely on this table
> you'll need to extend it everytime you want to support a new panel or
> resolution.
>
The computation are from internal tools. We can add more entries when we
need to support new panels.
>> +static void edp_ctrl_on_worker(struct work_struct *work)
>> +{
> [...]
>> +}
>> +
>> +static void edp_ctrl_off_worker(struct work_struct *work)
>> +{
>> + struct edp_ctrl *ctrl = container_of(
>> + work, struct edp_ctrl, off_work);
>> +}
>
> Why are these two functions workers?
>
msm_edp_ctrl_power() is called by user thread in bridge->enable/disable
and during edp on/off, it will send command to panel and block and wait
for panel's response. We don't want to block user thread. Also, the
bridge->enable/disable have no return value and there is no way to report
error to user. During test, we had a issue when a signal is pending, it
will interrupt and wake up the user thread waiting process. In this case,
user has no way to know it.
>> +
>> +irqreturn_t msm_edp_ctrl_irq(struct edp_ctrl *ctrl)
>> +{
> [...]
>> + if (isr1 & EDP_INTERRUPT_REG_1_HPD)
>> + DBG("edp_hpd");
>
> Don't you want to handle this?
>
We will have another change to support HPD. Also, this is not a reliable
signal for HPD.
>> +
>> + if (!ctrl->power_on) {
>> + if (!ctrl->cont_splash)
>> + edp_ctrl_phy_aux_enable(ctrl, 1);
>> + edp_ctrl_irq_enable(ctrl, 1);
>> + }
>> +
>> + ret = drm_dp_link_probe(ctrl->drm_aux, &ctrl->dp_link);
>> + if (ret) {
>> + pr_err("%s: read dpcd cap failed, %d\n", __func__, ret);
>> + goto disable_ret;
>> + }
>> +
>> + /* Initialize link rate as panel max link rate */
>> + ctrl->link_rate = drm_dp_link_rate_to_bw_code(ctrl->dp_link.rate);
>
> There's a lot of code here that should probably be a separate function
> rather than be called as part of retrieving the EDID.
>
I will change the function name.
>> diff --git a/drivers/gpu/drm/msm/edp/edp_phy.c
>> b/drivers/gpu/drm/msm/edp/edp_phy.c
> [...]
>> +bool msm_edp_phy_ready(struct edp_phy *phy)
>> +{
>> + u32 status;
>> + int cnt;
>> +
>> + cnt = 100;
>> + while (--cnt) {
>> + status = edp_read(phy->base +
>> + REG_EDP_PHY_GLB_PHY_STATUS);
>> + if (status & 0x01)
>
> Can you add a define for 0x01?
>
>> + pr_err("%s: PHY NOT ready\n", __func__);
>> + return false;
>> + } else {
>> + return true;
>> + }
>> +}
>> +
>> +void msm_edp_phy_ctrl(struct edp_phy *phy, int enable)
>> +{
>> + DBG("enable=%d", enable);
>> + if (enable) {
>> + /* Reset */
>> + edp_write(phy->base + REG_EDP_PHY_CTRL, 0x005); /* bit 0, 2 */
>> + wmb();
>> + usleep_range(500, 1000);
>> + edp_write(phy->base + REG_EDP_PHY_CTRL, 0x000);
>> + edp_write(phy->base + REG_EDP_PHY_GLB_PD_CTL, 0x3f);
>> + edp_write(phy->base + REG_EDP_PHY_GLB_CFG, 0x1);
>> + } else {
>> + edp_write(phy->base + REG_EDP_PHY_GLB_PD_CTL, 0xc0);
>> + }
>
> Please, also add defines for the values here. It's impossible to tell
> from the code what this does or what might need fixing if there was a
> bug.
>
Some of the values are magic number for H/W. They are hard to define.
Thanks,
Hai
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/dri-devel
