Re: [PATCH 3/8] drm/loongson: Allow attach drm bridge driver by calling lsdc_output_init()

[Sui Jingfeng <suijingfeng@loongson.cn>]

Hi,


On 2023/10/30 07:10, Dmitry Baryshkov wrote:
>> +
>> +/* Built-in HDMI encoder funcs on display pipe 0 */
>> +
>> +static void lsdc_pipe0_hdmi_encoder_reset(struct drm_encoder *encoder)
>> +{
>> +       struct drm_device *ddev = encoder->dev;
>> +       struct lsdc_device *ldev = to_lsdc(ddev);
>> +       u32 val;
>> +
>> +       val = PHY_CLOCK_POL | PHY_CLOCK_EN | PHY_DATA_EN;
>> +       lsdc_wreg32(ldev, LSDC_CRTC0_DVO_CONF_REG, val);
>> +
>> +       /* Using built-in GPIO emulated I2C instead of the hardware I2C */
>> +       lsdc_ureg32_clr(ldev, LSDC_HDMI0_INTF_CTRL_REG, HW_I2C_EN);
>> +
>> +       /* Help the HDMI phy get out of reset state */
>> +       lsdc_wreg32(ldev, LSDC_HDMI0_PHY_CTRL_REG, HDMI_PHY_RESET_N);
>> +
>> +       drm_dbg(ddev, "%s reset\n", encoder->name);
>> +
>> +       mdelay(20);
>> +}
>> +
>> +const struct drm_encoder_funcs lsdc_pipe0_hdmi_encoder_funcs = {
>> +       .reset = lsdc_pipe0_hdmi_encoder_reset,
>> +       .destroy = drm_encoder_cleanup,
>> +};
>> +
>> +/* Built-in HDMI encoder funcs on display pipe 1 */
> All pipe 1 code looks like a pipe0, just the registers were changed.
> Could you please refactor that to use a single instance of all
> functions and pass pipe id through the data struct?
> Then you can use macros to determine whether to use pipe 0 or pipe 1 register.
>

Yes, you are right. But please allow me to explain something.

In the past, Thomas told me to untangle it, despite this idea lead to duplicated code(or pattern).
but at the long run, this pay off.

Because the method of passing pipe id will introduce the "if and else" side effects.
But my functions have no if and else.


```
if (pipe == 0) {
     ...
} else if (pipe == 1) {
     ...
}
```

Using the C program language's Macro(#define XXX) to generate code is not fun to me.
Because every time you want to change it, It needs my brains to thinking it twice. Maybe
more than twice.

1) It needs my brains to replace the macros manually each time I saw the code.

2) When I want to change(alter) the prototype, I need to worry about all of the instances.
    sometimes it is not symmetry. The DVO port and HDMI phy itself is symmetry, but the
    external display bridge connected with them are not symmetry. So, there are some registers
    located at the domain of this display controller side should change according to the
    different type of external display bridge.

3) Code duplication is actually less harmful than unmaintainable.
    macros are abstract, as noob level programmer, we completely drop the idea of abstract.
    Bad abstract means design failure, this is what we are most afraid of.
    Generally, we would like divide the whole into small cases, handle them one by one.
    It is actually to review and understand.

4) From the viewpoint of the hardware, the display output hardware suffer from changes.
    Because users always want *new* display interface. The need of the users are also varies.
    Personally, I think macros are best for the symmetry case, while the output part of a
    display pipe always suffer from change.

>> +
>> +static void lsdc_pipe1_hdmi_encoder_reset(struct drm_encoder *encoder)
>> +{
>> +       struct drm_device *ddev = encoder->dev;
>> +       struct lsdc_device *ldev = to_lsdc(ddev);
>> +       u32 val;
>> +
>> +       val = PHY_CLOCK_POL | PHY_CLOCK_EN | PHY_DATA_EN;
>> +       lsdc_wreg32(ldev, LSDC_CRTC1_DVO_CONF_REG, val);
>> +
>> +       /* Using built-in GPIO emulated I2C instead of the hardware I2C */
>> +       lsdc_ureg32_clr(ldev, LSDC_HDMI1_INTF_CTRL_REG, HW_I2C_EN);
>> +
>> +       /* Help the HDMI phy get out of reset state */
>> +       lsdc_wreg32(ldev, LSDC_HDMI1_PHY_CTRL_REG, HDMI_PHY_RESET_N);
>> +
>> +       drm_dbg(ddev, "%s reset\n", encoder->name);
>> +
>> +       mdelay(20);
>> +}
>> +
>> +const struct drm_encoder_funcs lsdc_pipe1_hdmi_encoder_funcs = {
>> +       .reset = lsdc_pipe1_hdmi_encoder_reset,
>> +       .destroy = drm_encoder_cleanup,
>> +};