On Wed, Dec 06, 2023 at 06:20:58PM +0800, Andy Yan wrote: > Hi Sascha: > > > > + unsigned int n = vop2->data->regs_dump_size; > > > > 'n' is used only once, it might be clearer just to use the value where > > needed and drop the extra variable. > > Okay, will do. > > > > > + unsigned int i; > > > + > > > + drm_modeset_lock_all(drm_dev); > > > + > > > + if (vop2->enable_count) { > > > + for (i = 0; i < n; i++) { > > > + dump = &vop2->data->regs_dump[i]; > > > + vop2_regs_print(vop2, s, dump, false); > > > + } > > > + } else { > > > + seq_printf(s, "VOP disabled:\n"); > > > > There's nothing following after the ':', right? Then this should be > > "VOP: disabled\n" or without the colon at all. > > the colon will be droped in next versin. > > > > > > + } > > > + drm_modeset_unlock_all(drm_dev); > > > > This code is repeated in vop2_active_regs_show() below. Maybe factor > > this out to a common function? > > > > > Do you mean all the code between drm_modeset_lock_all and drm_modeset_unlock_all ? Including drm_modeset_lock_all() and drm_modeset_unlock_all(), yes. Sascha -- Pengutronix e.K. | | Steuerwalder Str. 21 | http://www.pengutronix.de/ | 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |