[PATCH] [v8, 2/2] drm/panel: Add Boe Himax8279d MIPI-DSI LCD panel
Jerry Han
jerry.han.hq at gmail.com
Sat Aug 3 02:15:39 UTC 2019
Hi Emil:
Thank you for your advice ,
After careful thinking, I decided to modify the code.
Tanks
Emil Velikov <emil.l.velikov at gmail.com> 于2019年7月31日周三 上午12:26写道：
>
> 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0010-v10-drm-panel-Add-Boe-Himax8279d-MIPI-DSI-LCD-panel.patch
Type: text/x-patch
Size: 40300 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/dri-devel/attachments/20190803/f9aa0898/attachment-0001.bin>
More information about the dri-devel
mailing list