Re: [PATCH v6 6/6] drm/sprd: add Unisoc's drm mipi dsi&dphy driver

[Sam Ravnborg <sam@ravnborg.org>]

Hi Kevin,

> > > +     reg->_0b.bits.out_sel = pll->out_sel;
> > > +     reg->_0b.bits.kint_l = pll->kint & 0xf;
> > > +     reg->_0e.bits.pll_pu_byp = 0;
> > > +     reg->_0e.bits.pll_pu = 0;
> > > +     reg->_0e.bits.stopstate_sel = 1;
> > > +     reg->_0f.bits.det_delay = pll->det_delay;
> > > +
> > > +     val = (u8 *)®
> > > +
> > > +     for (i = 0; i < sizeof(reg_addr); ++i) {
> > > +             regmap_write(regmap, reg_addr[i], val[i]);
> > > +             DRM_DEBUG("%02x: %02x\n", reg_addr[i], val[i]);
> > > +     }
> > > +}
> >
> > It would be great to also convert this part to a pattern without
> > structures.
> I will try it, but our pll registers, all not have official name from
> aisc design owner.
> If need to convert it, our pll regiters macro define, it can only be
> named as reg_01, reg_02...
IT is better to use the stupid names provided by you asic design owner,
than just numbers. Add a comment explaining this is due to the asic
design owner the names are stupid and not your fault.

> > > +
> > > +     if (dsi->panel) {
> > > +             drm_panel_prepare(dsi->panel);
> > > +             drm_panel_enable(dsi->panel);
> > > +     }
> >
> > Please use the new devm_drm_of_get_bridge helper here, and use the
> > bridge API instead.
> We use drm_panel_init and drm_panel_add API to add panel, so here is a
> panel device, not a bridge.

The new way to do this is to always wrap the panel in a bridge. We will
start to slowly migrate away from direct use of the panel API, and let
display drivers always wrap the panles in a bridge.
So please do as Maxime suggests.

	Sam