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? -- With best wishes Dmitry