> -----Original Message-----
> From: Dmitry Baryshkov <dmitry.baryshkov@xxxxxxxxxx>
> Sent: 2024年6月24日 5:11
> To: Keith Zhao <keith.zhao@xxxxxxxxxxxxxxxx>
> Cc: andrzej.hajda@xxxxxxxxx; neil.armstrong@xxxxxxxxxx; rfoss@xxxxxxxxxx;
> Laurent.pinchart@xxxxxxxxxxxxxxxx; jonas@xxxxxxxxx;
> jernej.skrabec@xxxxxxxxx; maarten.lankhorst@xxxxxxxxxxxxxxx;
> mripard@xxxxxxxxxx; tzimmermann@xxxxxxx; airlied@xxxxxxxxx;
> daniel@xxxxxxxx; robh@xxxxxxxxxx; krzk+dt@xxxxxxxxxx; conor+dt@xxxxxxxxxx;
> hjc@xxxxxxxxxxxxxx; heiko@xxxxxxxxx; andy.yan@xxxxxxxxxxxxxx; Xingyu Wu
> <xingyu.wu@xxxxxxxxxxxxxxxx>; p.zabel@xxxxxxxxxxxxxx; Jack Zhu
> <jack.zhu@xxxxxxxxxxxxxxxx>; Shengyang Chen
> <shengyang.chen@xxxxxxxxxxxxxxxx>; dri-devel@xxxxxxxxxxxxxxxxxxxxx;
> devicetree@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx;
> linux-arm-kernel@xxxxxxxxxxxxxxxxxxx
> Subject: Re: [PATCH v4 10/10] drm/vs: add simple dsi encoder
>
> On Sun, Jun 23, 2024 at 07:17:09AM GMT, Keith Zhao wrote:
> > Hi Dmitry:
> >
> > > On Tue, May 21, 2024 at 06:58:17PM +0800, keith wrote:
>
> > > > + "starfive,syscon",
> > > > + 2, args);
> > > > +
> > > > + if (IS_ERR(simple->dss_regmap)) {
> > > > + return dev_err_probe(dev, PTR_ERR(simple->dss_regmap),
> > > > + "getting the regmap failed\n");
> > > > + }
> > > > +
> > > > + simple->offset = args[0];
> > > > + simple->mask = args[1];
> > >
> > > Is the value that you've read platform dependent or use case dependent?
> > > What is the actual value being written? Why are you using syscon for it?
> >
> > The syscon is used to select crtcs binded with encoder, If this
> > encoder binds to crtc0 , set the syscon reg bit0 = 1 If this encoder
> > binds to crtc1 , set the syscon reg bit1 = 1 (0x2) Maybe I can do this
> > by the possible_crtc instead of using args from dts
>
> If this is a constant between your platforms, it should not be a part of DT.
>
> >
> >
> > >
> > > > +
> > > > + return 0;
> > > > +}
> > > > +
> > > > +static void vs_encoder_atomic_enable(struct drm_encoder *encoder,
> > > > +struct drm_atomic_state *state) {
> > > > + struct vs_simple_encoder *simple = to_simple_encoder(encoder);
> > > > +
> > > > + regmap_update_bits(simple->dss_regmap, simple->offset,
> > > > +simple->mask,
> > > > +simple->mask);
> > >
> > >
> > > A purist in me would ask to have separate mask and value to write.
> > Understand , will avoid this action
> > >
> > > > +}
> > >
> > > Is it necessary to clear those bits when stopping the stream?
> > No need to do this , if clear those bits , the encoder will point to a
> > unknown crtc
>
> what are the consequences? Is it desirable or not?
There are two crtcs.
Each display terminal encoder can combine any crtc, depending on the value of possible crtc.
When the bit is 0, it means that the encoder matches crtc0.
When the bit is 1, it means that the encoder matches crtc1.
The possible crtc of this encoder is 2 , the reg bit is 1.
When the video stream is stopped, if the bit is cleared, the result is that the encoder hardware points to crtc0,
and the encoder points to crtc1 based on the drm framework(because the possible crtc no change).
>
> --
> With best wishes
> Dmitry
