On Wed, Mar 08, 2023 at 12:13:53PM +0100, Konrad Dybcio wrote:
>
>
> On 8.03.2023 05:37, Jianhua Lu wrote:
> > Add a driver for panels using the Novatek NT36523 display driver IC.
> >
> > Signed-off-by: Jianhua Lu <lujianhua000@xxxxxxxxx>
> > ---
> > Changes in v3:
> > - Refactor source code
> >
> > Changes in v2:
> > - Refactor and clean up source code
> This is a veeery vague changelog, akin to "change patch", please
> be more specific the next time around.
>
Acked, thanks
>
> [...]
>
> > +#define DSI_NUM_MIN 1
> > +
> > +/* Macro modified from mipi_dual_dsi_dcs_write_seq */
> > +#define mipi_dual_dsi_dcs_write_seq(dsi, cmd, seq...) \
> > + do { \
> > + static const u8 d[] = { cmd, seq }; \
> > + int i, ret; \
> > + for (i = 0; i < ARRAY_SIZE(dsi); i++) { \
> > + ret = mipi_dsi_dcs_write_buffer(dsi[i], d, ARRAY_SIZE(d)); \
> > + if (ret < 0) { \
> > + dev_err_ratelimited( \
> > + &dsi[i]->dev, "sending command %#02x failed: %d\n", \
> > + cmd, ret); \
> > + return ret; \
> > + } \
> > + } \
> > + } while (0)
> > +
> This should definitely be put in a common file..
Acked.
>
>
> > +static const struct drm_display_mode elish_boe_mode = {
> > + /* Current max freqency is 104HZ, TODO 120HZ */
> Is it a DPU issue, or does the panel not work correctly when you
> jack up the clocks? Did you use the correct init sequence and porches
> for the 120Hz mode, including the mode switch command set?
>
panel will turn into bluescreen when freqency overs 104HZ, downstream
seems use a dsi-cphy enhance feature to make it working. Init sequence
and porches are right
> Could you also implement a 60 (or whatever other value is also
> implemented downstream) Hz mode?
I will try to implement it.
>
> > + .clock = (1600 + 60 + 8 + 60) * (2560 + 26 + 4 + 168) * 104 / 1000,
> > + .hdisplay = 1600,
> > + .hsync_start = 1600 + 60,
> > + .hsync_end = 1600 + 60 + 8,
> > + .htotal = 1600 + 60 + 8 + 60,
> > + .vdisplay = 2560,
> > + .vsync_start = 2560 + 26,
> > + .vsync_end = 2560 + 26 + 4,
> > + .vtotal = 2560 + 26 + 4 + 168,
> > +};
> > +
> > +static const struct drm_display_mode elish_csot_mode = {
> > + /* Current max freqency is 104HZ, TODO 120HZ */
> > + .clock = (1600 + 200 + 40 + 52) * (2560 + 26 + 4 + 168) * 104 / 1000,
> > + .hdisplay = 1600,
> > + .hsync_start = 1600 + 200,
> > + .hsync_end = 1600 + 200 + 40,
> > + .htotal = 1600 + 200 + 40 + 52,
> > + .vdisplay = 2560,
> > + .vsync_start = 2560 + 26,
> > + .vsync_end = 2560 + 26 + 4,
> > + .vtotal = 2560 + 26 + 4 + 168,
> > +};
> > +
> > +static const struct panel_desc elish_boe_desc = {
> > + .modes = &elish_boe_mode,
> > + .dsi_info = {
> > + .type = "NT36523",
> That's a bit vague, the driver IC is not very telling about the
> panel itself. Since we're not able to determine much more information,
> this could contain the panel manufacturer and the device name.
Acked.
>
> > + .channel = 0,
> > + .node = NULL,
> > + },
> > + .width_mm = 127,
> > + .height_mm = 203,
> > + .bpc = 8,
> > + .lanes = 3,
> > + .format = MIPI_DSI_FMT_RGB888,
> > + .mode_flags = MIPI_DSI_MODE_VIDEO | MIPI_DSI_CLOCK_NON_CONTINUOUS | MIPI_DSI_MODE_LPM,
> > + .init_sequence = elish_boe_init_sequence,
> > + .is_dual_dsi = true,
> > +};
> > +
> [...]
>
> > +static int nt36523_probe(struct mipi_dsi_device *dsi)
> > +{
> [...]
>
> > + /* If the panel is dual dsi, register DSI1 */
> > + if (pinfo->desc->is_dual_dsi) {
> > + info = &pinfo->desc->dsi_info;
> > +
> > + dsi1 = of_graph_get_remote_node(dsi->dev.of_node, 1, -1);
> > + if (!dsi1) {
> > + dev_err(dev, "cannot get secondary DSI node.\n");
> > + return -ENODEV;
> > + }
> > +
> > + dsi1_host = of_find_mipi_dsi_host_by_node(dsi1);
> > + of_node_put(dsi1);
> Shouldn't you put the reference only if it's found?
thanks for spot it.
>
> > + if (!dsi1_host) {
> > + dev_err(dev, "cannot get secondary DSI host\n");
> > + return -EPROBE_DEFER;
> dev_err_probe, here and in neighbouring exit return paths?
Acked.
>
>
> Konrad
