Re: [PATCH v4 2/2] drm/panel: Add JDI LT070ME05000 WUXGA DSI Panel

[Vinay Simha]

On Tue, Jun 14, 2016 at 4:55 PM, Thierry Reding
<thierry.reding@xxxxxxxxx> wrote:
> On Tue, Jun 14, 2016 at 04:27:53PM +0530, Vinay Simha wrote:
> [...]
>> On Mon, Jun 13, 2016 at 6:05 PM, Thierry Reding
>> <thierry.reding@xxxxxxxxx> wrote:
>> > On Mon, Jun 13, 2016 at 03:55:28PM +0530, Vinay Simha BN wrote:
> [...]
>> >> +const char *regs[] = {
>> >> +     "vddp",
>> >> +     "dcdc_en",
>> >> +     "vcc"
>> >> +};
>> >
>> > This should be static. Also use a more sensible name, such as
>> > regulator_names, please.
>> it kept as regs, to keep constant names as used in
>> the dsi_cfg file
>> drivers/gpu/drm/msm/dsi/dsi_cfg.c
>
> That's a completely different driver, no need to be consistent.
>
>> >> +static int jdi_panel_init(struct jdi_panel *jdi)
>> >> +{
>> > [...]
>> >> +     struct mipi_dsi_device *dsi = jdi->dsi;
>> >> +     int ret;
>> >> +
>> >> +     dsi->mode_flags |= MIPI_DSI_MODE_LPM;
>> >> +
>> >> +     ret = mipi_dsi_dcs_soft_reset(dsi);
>> >> +     if (ret < 0)
>> >> +             return ret;
>> >> +
>> >> +     usleep_range(10000, 20000);
>> >> +
>> >> +     ret = mipi_dsi_dcs_set_pixel_format(dsi, 0x70);
>> >> +     if (ret < 0)
>> >> +             return ret;
>> >
>> > Please use the existing symbolic constants for this.
>> i am not clear on the symbolic constants for pixel_format ?
>
> See include/video/mipi_display.h
>
>> >> +
>> >> +     ret = mipi_dsi_dcs_write(dsi, MIPI_DCS_WRITE_CONTROL_DISPLAY,
>> >> +                              (u8[]){ 0x24 }, 1);
>> >> +     if (ret < 0)
>> >> +             return ret;
>> brightness control setting, to enable pwm/backlight
>> >> +
>> >> +     ret = mipi_dsi_dcs_write(dsi, MIPI_DCS_WRITE_POWER_SAVE,
>> >> +                              (u8[]){ 0x00 }, 1);
>> >> +     if (ret < 0)
>> >> +             return ret;
>> >
>> this is to set cabc off/on
>
> Can you point me to the specification of these? I'd like to investigate
> whether or not we can turn these into more sensible commands. As-is, it
> is completely obfuscated and I'd like to avoid that where possible.
https://cdn-shop.adafruit.com/datasheets/HX8357-D_DS_April2012.pdf

MIPI_DCS_WRITE_CONTROL_DISPLAY
This is one of the panel i got in google search,
refer 6.2.44 Write control display (53h) (page 171)

MIPI_DCS_WRITE_POWER_SAVE
6.2.46 Write content adaptive brightness control (55h) (page 173)

fyi,
nx7 2013 panel/schematic datasheet is not available in opensource,
>
>> >> +     ret = mipi_dsi_generic_write(dsi, (u8[]){0xB0, 0x00}, 2);
>> >> +     if (ret < 0)
>> >> +             return ret;
>> >> +     mdelay(10);
>> >
>> > Same here. This also needs at least a comment, though ideally you'd use
>> > symbolic names for those magic numbers.
>> i do not have the datasheet to give more description.
>> this is for interface setting, either command mode/video mode
>
> Okay, please add a comment on what this is supposed to do, then.
>
>> >> +
>> >> +     jdi->mode = &default_mode;
>> >> +
>> >> +     for (i = 0; i < num; i++)
>> >> +             s[i].supply = regs[i];
>> >> +
>> >> +     ret = devm_regulator_bulk_get(dev, num, s);
>> >> +     if (ret < 0) {
>> >> +             dev_err(dev, "%s: failed to init regulator, ret=%d\n",
>> >> +                     __func__, ret);
>> >> +             return ret;
>> >> +     }
>> >> +
>> >> +     jdi->reset_gpio = devm_gpiod_get(dev, "reset", GPIOD_OUT_LOW);
>> >> +     if (IS_ERR(jdi->reset_gpio)) {
>> >> +             dev_err(dev, "cannot get reset-gpios %ld\n",
>> >> +                     PTR_ERR(jdi->reset_gpio));
>> >
>> > This is a third variant of error reporting. Please stick to one.
>> for PTR_ERR(jdi->reset_gpio) returns unsigned long, so this error reporting
>> cannot be changed to ret,
>> others error reporting incorporated consistently.
>
> PTR_ERR() returns signed long, not unsigned.
>
> You can still use the same format for the message and substitute the
> %ld printk specifier to match the type.
>
> Thierry



-- 
Regards,

Vinay Simha.B.N.