[PATCH] [v8, 2/2] drm/panel: Add Boe Himax8279d MIPI-DSI LCD panel

[Emil Velikov]

Hi Jerry,

Can you please disable HTML emails for the Gmail web client and reply
inline. There are multiple articles how to do that.

On 2019/07/26, Jerry Han wrote:
> Hi Emil Velikov:
> 
> First of all, thank you for your comments.
> 
> > Hi Jerry,
> >
> > On Thu, 25 Apr 2019 at 08:40, Jerry Han <jerry.han.hq at gmail.com> wrote:
> > >
> > > Support Boe Himax8279d 8.0" 1200x1920 TFT LCD panel, it is a MIPI DSI
> > > panel.
> > >
> > > V8:
> > > - Modify communication address
> > >
> > > V7:
> > > - Add the information of the reviewer
> > > - Remove unnecessary delays, The udelay_range code gracefully returns
> > >     without hitting the scheduler on a delay of 0. (Derek)
> > > - Merge the same data structures, like display_mode and off_cmds (Derek)
> > > - Optimize the processing of results returned by
> > >     devm_gpiod_get_optional (Derek)
> > >
> > > V6:
> > > - Add the information of the reviewer (Sam)
> > > - Delete unnecessary header files #include <linux/fb.h> (Sam)
> > > - The config DRM_PANEL_BOE_HIMAX8279D appears twice. Drop one of them
> > (Sam)
> > > - ADD static, set_gpios function is not used outside this module (Sam)
> > >
> > > V5:
> > > - Added changelog
> > >
> > > V4:
> > > - Frefix all function maes with boe_ (Sam)
> > > - Fsed "enable_gpio" replace "reset_gpio", Make it look clearer (Sam)
> > > - Sort include lines alphabetically (Sam)
> > > - Fixed entries in the makefile must be sorted alphabetically (Sam)
> > > - Add send_mipi_cmds function to avoid duplicating the code (Sam)
> > > - Add the necessary delay(reset_delay_t5) between reset and sending
> > >     the initialization command (Rock wang)
> > >
> > > V3:
> > > - Remove unnecessary delays in sending initialization commands (Jitao
> > Shi)
> > >
> > > V2:
> > > - Use SPDX identifier (Sam)
> > > - Use necessary header files replace drmP.h (Sam)
> > > - Delete unnecessary header files #include <linux/err.h> (Sam)
> > > - Specifies a GPIOs array to control the reset timing,
> > >     instead of reading "dsi-reset-sequence" data from DTS (Sam)
> > > - Delete backlight_disable() function when already disabled (Sam)
> > > - Use devm_of_find_backlight() replace of_find_backlight_by_node() (Sam)
> > > - Move the necessary data in the DTS to the current file,
> > >     like porch, display_mode and Init code etc. (Sam)
> > > - Add compatible device "boe,himax8279d10p" (Sam)
> > >
> > > Signed-off-by: Jerry Han <jerry.han.hq at gmail.com>
> > > Reviewed-by: Sam Ravnborg <sam at ravnborg.org>
> > > Reviewed-by: Derek Basehore <dbasehore at chromium.org>
> > > Cc: Jitao Shi <jitao.shi at mediatek.com>
> > > Cc: Rock wang <rock_wang at himax.com.cn>
> > > ---
> > While the DT has landed, this patch is not merged it seems.
> > I think that Sam is waiting for a version which does not trigger so
> > many check-patch warnings.
> >
> > That said, a couple of comments if I may.
> >
> > > +       struct gpio_desc *enable_gpio;
> > > +       struct gpio_desc *pp33_gpio;
> > > +       struct gpio_desc *pp18_gpio;
> > DT claims that these gpios are required, yet one is using the optional
> > gpio API.
> > Is the code off, or does the DT need fixing?
> >
> >
> Thank you for your advice , I will  fix code.
> 
> > > +static int send_mipi_cmds(struct drm_panel *panel, const struct
> > panel_cmd *cmds)
> > > +{
> > > +       struct panel_info *pinfo = to_panel_info(panel);
> > > +       unsigned int i = 0;
> > > +       int err;
> > > +
> > > +       if (!cmds)
> > > +               return -EFAULT;
> > > +
> > > +       for (i = 0; cmds[i].len != 0; i++) {
> > > +               const struct panel_cmd *cmd = &cmds[i];
> > > +
> > > +               if (cmd->len == 2)
> > > +                       err = mipi_dsi_dcs_write(pinfo->link,
> > > +                                                   cmd->data[1], NULL,
> > 0);
> > > +               else
> > > +                       err = mipi_dsi_dcs_write(pinfo->link,
> > > +                                                   cmd->data[1],
> > cmd->data + 2,
> > > +                                                   cmd->len - 2);
> > > +
> > > +               if (err < 0)
> > > +                       return err;
> > > +
> > > +               usleep_range((cmd->data[0]) * 1000,
> > > +                           (1 + cmd->data[0]) * 1000);
> > > +       }
> > > +
> > This format seems semi-magical. Any particular reason why you're not
> > using the helpers?
> > In particular, enter/exit sleep mode and set display on/off.
> >
> > Speaking of which - the 8inch display uses then, yet they are missing
> > for the 10inch one. Seems like there's a bug in there.
> >
> > Plus we have some repeating patterns in the vendor/undocumented
> > commands - good reason to get those into separate hunks.
> > With the above in place, you can effectively drop the .off_cmd
> > callback and sleep field from the INIT_CMD arrays.
> >
> > Hence, less code to debug and maintain ;-)
> >

> Normally, we only need to download 0x28 0x10 commands to power-key down.
> I noticed that helper func (mipi_dsi_dcs_set_display_off,
> mipi_dsi_dcs_enter_sleep_mode) help me send this commands(0x28 0x10).
> 
> But based on previous debugging experience, Some vendors are special and
> may send other commands after sending cmd 0x28 0x10 or before send cmd 0x28
> 0x10.
> this problem may also occur in our vendor,
> so i think this above approach helps me manage code better.
> 
Using the helpers helper does _not_ prevent you from issuing other
commands before or after them.

Looking through the existing codebase you'll see plenty of other panel
drivers doing so.

If anything you'll spot that nearly every driver calls 
mipi_dsi_dcs_set_display_{on,off} in their enable/disable callbacks.

So what I'm suggesting:
 - use the helpers in the respective callbacks:
a) display on/off for enable/disable
b) exit/enter sleep mode for prepare/unprepare
If any of these cause issues, omit them and adding an inline comment
clearly explain why they are missing.

 - add explicit delays around the helpers, if needed

 - kill off default_off_cmds and related infra

 - factor out the command stream differences, how you did in for
reset_delay*. Quck compare shows 8 differences in the 300 commands.

 - kill off the delay stored in data[0]

 - don't repeat the following sequence _12_ times
_INIT_CMD(0xB0, 0x08),
...
_INIT_CMD(0xCE, 0xFF),


With the above in place, the driver will become a lot smaller and easier
to understand.

Thanks
-Emil