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

[Jerry Han]

Hi Emil Velikov:

First of all, thank you for your comments.

> +       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)
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.

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.

Thanks.


Emil Velikov <emil.l.velikov at gmail.com> 于2019年7月1日周一 下午9:41写道：

> 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?
>
>
> > +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 ;-)
>
> HTH
> Emil
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/dri-devel/attachments/20190726/c8849fdd/attachment.html>