Hi,
On Tue, May 7, 2024 at 6:53 AM Cong Yang
<[email protected]> wrote:
>
> +static int hx83102_enable_extended_cmds(struct hx83102 *ctx, bool enable)
> +{
> + struct mipi_dsi_multi_context dsi_ctx = { .dsi = ctx->dsi };
> +
> + if (enable)
> + mipi_dsi_dcs_write_seq_multi(&dsi_ctx, HX83102_SETEXTC, 0x83, 0x10, 0x21, 0x55, 0x00);
> + else
> + mipi_dsi_dcs_write_seq_multi(&dsi_ctx, HX83102_SETEXTC, 0x00, 0x00, 0x00);
> +
> + return 0;
You're throwing away the error codes returned by the
mipi_dsi_dcs_write_seq_multi(), which you shouldn't do. You have two
options:
Option #1: return dsi_ctx.accum_err here and then check the return
value in callers.
Option #2: instead of having this function take "struct hx83102 *ctx",
just have it take "struct mipi_dsi_multi_context *dsi_ctx". Then it
can return void and everything will be fine.
I'd prefer option #2 but either is OK w/ me.
> +static int starry_himax83102_j02_init(struct hx83102 *ctx)
> +{
> + struct mipi_dsi_multi_context dsi_ctx = { .dsi = ctx->dsi };
> +
> + hx83102_enable_extended_cmds(ctx, true);
> + mipi_dsi_dcs_write_seq_multi(&dsi_ctx, HX83102_SETPOWER, 0x2c, 0xb5, 0xb5, 0x31, 0xf1,
> + 0x31, 0xd7, 0x2f, 0x36, 0x36, 0x36, 0x36, 0x1a, 0x8b, 0x11,
> + 0x65, 0x00, 0x88, 0xfa, 0xff, 0xff, 0x8f, 0xff, 0x08, 0x74,
> + 0x33);
The indentation is still off here. You have 5 tabs followed by a
space. To make things line up with the opening brace I think it should
be 4 tabs followed by 5 spaces.
> +static int hx83102_enable(struct drm_panel *panel)
> +{
> + struct hx83102 *ctx = panel_to_hx83102(panel);
> + struct mipi_dsi_device *dsi = ctx->dsi;
> + struct device *dev = &dsi->dev;
> + int ret;
> +
> + ret = ctx->desc->init(ctx);
> + if (ret)
> + return ret;
You're still changing behavior here. In the old boe-tv101wum-nl6
driver the init() function was invoked at the end of prepare(). Now
you've got it at the beginning of enable(). If this change is
important it should be in a separate commit and explained.
> + ret = mipi_dsi_dcs_exit_sleep_mode(dsi);
> + if (ret) {
> + dev_err(dev, "Failed to exit sleep mode: %d\n", ret);
> + return ret;
> + }
> +
> + msleep(120);
> +
> + ret = mipi_dsi_dcs_set_display_on(dsi);
> + if (ret) {
> + dev_err(dev, "Failed to turn on the display: %d\n", ret);
> + }
The old boe-tv101wum-nl6 driver didn't call
mipi_dsi_dcs_exit_sleep_mode() nor mipi_dsi_dcs_set_display_on() in
its enable routine, did it? If this change is important please put it
in a separate change and justify it.
-Doug
Hi,
Doug Anderson <[email protected]> 于2024年5月8日周三 07:35写道：
>
> Hi,
>
> On Tue, May 7, 2024 at 6:53 AM Cong Yang
> <[email protected]> wrote:
> >
> > +static int hx83102_enable_extended_cmds(struct hx83102 *ctx, bool enable)
> > +{
> > + struct mipi_dsi_multi_context dsi_ctx = { .dsi = ctx->dsi };
> > +
> > + if (enable)
> > + mipi_dsi_dcs_write_seq_multi(&dsi_ctx, HX83102_SETEXTC, 0x83, 0x10, 0x21, 0x55, 0x00);
> > + else
> > + mipi_dsi_dcs_write_seq_multi(&dsi_ctx, HX83102_SETEXTC, 0x00, 0x00, 0x00);
> > +
> > + return 0;
>
> You're throwing away the error codes returned by the
> mipi_dsi_dcs_write_seq_multi(), which you shouldn't do. You have two
> options:
>
> Option #1: return dsi_ctx.accum_err here and then check the return
> value in callers.
>
> Option #2: instead of having this function take "struct hx83102 *ctx",
> just have it take "struct mipi_dsi_multi_context *dsi_ctx". Then it
> can return void and everything will be fine.
>
> I'd prefer option #2 but either is OK w/ me.
Ok,I will fix in V4, thanks.
>
>
> > +static int starry_himax83102_j02_init(struct hx83102 *ctx)
> > +{
> > + struct mipi_dsi_multi_context dsi_ctx = { .dsi = ctx->dsi };
> > +
> > + hx83102_enable_extended_cmds(ctx, true);
> > + mipi_dsi_dcs_write_seq_multi(&dsi_ctx, HX83102_SETPOWER, 0x2c, 0xb5, 0xb5, 0x31, 0xf1,
> > + 0x31, 0xd7, 0x2f, 0x36, 0x36, 0x36, 0x36, 0x1a, 0x8b, 0x11,
> > + 0x65, 0x00, 0x88, 0xfa, 0xff, 0xff, 0x8f, 0xff, 0x08, 0x74,
> > + 0x33);
>
> The indentation is still off here. You have 5 tabs followed by a
> space. To make things line up with the opening brace I think it should
> be 4 tabs followed by 5 spaces.
Sorry, my editor 'Visual Studio Code' It seems that the correct indentation
is not recognized. I have checked it through the 'vim' editor in the V4 version.
Thanks.
>
>
> > +static int hx83102_enable(struct drm_panel *panel)
> > +{
> > + struct hx83102 *ctx = panel_to_hx83102(panel);
> > + struct mipi_dsi_device *dsi = ctx->dsi;
> > + struct device *dev = &dsi->dev;
> > + int ret;
> > +
> > + ret = ctx->desc->init(ctx);
> > + if (ret)
> > + return ret;
>
> You're still changing behavior here. In the old boe-tv101wum-nl6
> driver the init() function was invoked at the end of prepare(). Now
> you've got it at the beginning of enable(). If this change is
> important it should be in a separate commit and explained.
>
>
> > + ret = mipi_dsi_dcs_exit_sleep_mode(dsi);
> > + if (ret) {
> > + dev_err(dev, "Failed to exit sleep mode: %d\n", ret);
> > + return ret;
> > + }
> > +
> > + msleep(120);
> > +
> > + ret = mipi_dsi_dcs_set_display_on(dsi);
> > + if (ret) {
> > + dev_err(dev, "Failed to turn on the display: %d\n", ret);
> > + }
>
> The old boe-tv101wum-nl6 driver didn't call
> mipi_dsi_dcs_exit_sleep_mode() nor mipi_dsi_dcs_set_display_on() in
> its enable routine, did it? If this change is important please put it
> in a separate change and justify it.
In the old boe-tv101wum-nl6 driver inital cmds was invoked at the end of
prepare() function , and call 0x11 and 0x29 at end of inital. For
himax-hx83102 driver, we move inital cmds invoked at enable() function.
For panel timing, I think there is no much difference. They are
all initial cmds executed after meeting the power-on sequence.
I will update these in the v4 commit message.
>
>
> -Doug
Hi,
On Wed, May 8, 2024 at 4:52 AM cong yang
<[email protected]> wrote:
>
> > > +static int starry_himax83102_j02_init(struct hx83102 *ctx)
> > > +{
> > > + struct mipi_dsi_multi_context dsi_ctx = { .dsi = ctx->dsi };
> > > +
> > > + hx83102_enable_extended_cmds(ctx, true);
> > > + mipi_dsi_dcs_write_seq_multi(&dsi_ctx, HX83102_SETPOWER, 0x2c, 0xb5, 0xb5, 0x31, 0xf1,
> > > + 0x31, 0xd7, 0x2f, 0x36, 0x36, 0x36, 0x36, 0x1a, 0x8b, 0x11,
> > > + 0x65, 0x00, 0x88, 0xfa, 0xff, 0xff, 0x8f, 0xff, 0x08, 0x74,
> > > + 0x33);
> >
> > The indentation is still off here. You have 5 tabs followed by a
> > space. To make things line up with the opening brace I think it should
> > be 4 tabs followed by 5 spaces.
>
> Sorry, my editor 'Visual Studio Code' It seems that the correct indentation
> is not recognized. I have checked it through the 'vim' editor in the V4 version.
> Thanks.
FWIW, I use VS Code and it looks fine to me. Maybe check your VS Code
settings? Tab size should be 8.
> > > +static int hx83102_enable(struct drm_panel *panel)
> > > +{
> > > + struct hx83102 *ctx = panel_to_hx83102(panel);
> > > + struct mipi_dsi_device *dsi = ctx->dsi;
> > > + struct device *dev = &dsi->dev;
> > > + int ret;
> > > +
> > > + ret = ctx->desc->init(ctx);
> > > + if (ret)
> > > + return ret;
> >
> > You're still changing behavior here. In the old boe-tv101wum-nl6
> > driver the init() function was invoked at the end of prepare(). Now
> > you've got it at the beginning of enable(). If this change is
> > important it should be in a separate commit and explained.
> >
> >
> > > + ret = mipi_dsi_dcs_exit_sleep_mode(dsi);
> > > + if (ret) {
> > > + dev_err(dev, "Failed to exit sleep mode: %d\n", ret);
> > > + return ret;
> > > + }
> > > +
> > > + msleep(120);
> > > +
> > > + ret = mipi_dsi_dcs_set_display_on(dsi);
> > > + if (ret) {
> > > + dev_err(dev, "Failed to turn on the display: %d\n", ret);
> > > + }
> >
> > The old boe-tv101wum-nl6 driver didn't call
> > mipi_dsi_dcs_exit_sleep_mode() nor mipi_dsi_dcs_set_display_on() in
> > its enable routine, did it? If this change is important please put it
> > in a separate change and justify it.
>
> In the old boe-tv101wum-nl6 driver inital cmds was invoked at the end of
> prepare() function , and call 0x11 and 0x29 at end of inital. For
> himax-hx83102 driver, we move inital cmds invoked at enable() function.
> For panel timing, I think there is no much difference. They are
> all initial cmds executed after meeting the power-on sequence.
> I will update these in the v4 commit message.
Ah, I see! So the mipi_dsi_dcs_exit_sleep_mode() was the 0x11 in the
old code and the mipi_dsi_dcs_set_display_on() was the 0x29 in the old
code. OK, I agree that it's better like you've done it where those
functions are moved out of the "->init()" function and into the
caller, so please keep that as you have it.
The only thing I would request is to keep the ->init() call to be made
at the end of prepare() instead of the beginning of enable(). It may
not matter too much, but in that case I'd rather keep it how it was or
make it an explicit change and not an implicit part of the refactor.
-Doug