Re: [PATCH v14 1/2] drm: add kms driver for loongson display controller

[WANG Xuerui]

On 2023/5/22 16:51, Sui Jingfeng wrote:
> Hi,
>
> On 2023/5/21 20:21, WANG Xuerui wrote:
> > > <snip>
> > > +
> > > +static void lsdc_crtc0_soft_reset(struct lsdc_crtc *lcrtc)
> > > +{
> > > +    struct lsdc_device *ldev = lcrtc->ldev;
> > > +    u32 val;
> > > +
> > > +    val = lsdc_rreg32(ldev, LSDC_CRTC0_CFG_REG);
> > > +
> > > +    val &= CFG_VALID_BITS_MASK;
> > > +
> > > +    /* soft reset bit, active low */
> > > +    val &= ~CFG_RESET_N;
> > > +
> > > +    val &= ~CFG_PIX_FMT_MASK;
> > > +
> > > +    lsdc_wreg32(ldev, LSDC_CRTC0_CFG_REG, val);
> > > +
> > > +    udelay(5);
> > > +
> > > +    val |= CFG_RESET_N | LSDC_PF_XRGB8888 | CFG_OUTPUT_ENABLE;
> > > +
> > > +    lsdc_wreg32(ldev, LSDC_CRTC0_CFG_REG, val);
> > > +
> > > +    mdelay(20);
> > > +}
> > > +
> > > +static void lsdc_crtc1_soft_reset(struct lsdc_crtc *lcrtc)
> > > +{
> > > +    struct lsdc_device *ldev = lcrtc->ldev;
> > > +    u32 val;
> > > +
> > > +    val = lsdc_rreg32(ldev, LSDC_CRTC1_CFG_REG);
> > > +
> > > +    val &= CFG_VALID_BITS_MASK;
> > > +
> > > +    /* soft reset bit, active low */
> > > +    val &= ~CFG_RESET_N;
> > > +
> > > +    val &= ~CFG_PIX_FMT_MASK;
> > > +
> > > +    lsdc_wreg32(ldev, LSDC_CRTC1_CFG_REG, val);
> > > +
> > > +    udelay(5);
> > > +
> > > +    val |= CFG_RESET_N | LSDC_PF_XRGB8888 | CFG_OUTPUT_ENABLE;
> > > +
> > > +    lsdc_wreg32(ldev, LSDC_CRTC1_CFG_REG, val);
> > > +
> > > +    msleep(20);
> >
> > So many magic sleeps without documentation?
> It is just that you should wait the device for a while before it can 
> reaction when doing the soft reset.
>
> I think this is engineering...

As an engineer myself, I fully concur with this, but I mainly wanted 
some explanation as to "why 5 there? why 20 here? why 9 there?" -- where 
did all the discrete values come from, implied by HDL or found out by 
experimentations? Can these be extracted to properly named constants? 
Can some of the values get coalesced into one without harming 
functionality? Can some of them get shorter? -- questions like this.

> <snip>
> > > +
> > > +/* All loongson display controller support scanout position hardware */
> >
> > Commit message implies only 7A2000+ LSDC IPs have the "scanout 
> > position recorders". Either that part or this code would need tweaking... 
>
> Both LS7A2000 and LS7A1000 have the scanout position recorders hardware.
>
> Preciously, datasheet of LS7A1000 didn't told us if it support this 
> feature.
>
> I will adjust the commit message at next version, the code doesn't need 
> change.

That's fine, the intent is always making the code more approachable and 
maintainable. Thanks.

--
WANG "xen0n" Xuerui

Linux/LoongArch mailing list: https://lore.kernel.org/loongarch/