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
