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

[Dmitry Baryshkov]

On Mon, 30 Oct 2023 at 06:13, Sui Jingfeng <suijingfeng@xxxxxxxxxxx> wrote:
>
> 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) {
>      ...
> }
> ```

I was thinking about something easier:

static void lsdc_pipe_hdmi_encoder_reset(struct drm_encoder *encoder)
{
   ....
   lsdc_wreg32(ldev, LSDC_CRTCn_DVO_CONF_REG(ldev->pipe_id), val);
   ...
};

So, no ifs, just define per-pipe registers.


>
> 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.

Code duplication is both harmful and unmaintainable. It is _very_hard_
to spot the difference between pipe0 and pipe1. And it is _very_easy_
to patch just one instance of these functions leaving the issue in the
second one. So, I'd say, all the c&p functions are a no-go.

>
> 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,
> >> +};
>


-- 
With best wishes
Dmitry