Re: [PATCH] drm/panel: auo novatek 1080p video mode panel

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Fri, Aug 7, 2015 at 9:19 AM, Thierry Reding <thierry.reding@xxxxxxxxx> wrote:
> On Tue, Jul 21, 2015 at 03:36:02PM -0400, Rob Clark wrote:
>> This is one of several different panels that are used on the Sony Xperia
>> Z3 phone.  It can operate in either command or video mode, although so
>> far only video mode is implemented (since that is the mode that the
>> downstream kernel version I happened to be looking at was using, and
>> that is where I got the programming sequences for the panel).
>>
>> Signed-off-by: Rob Clark <robdclark@xxxxxxxxx>
>> ---
>> Couple Notes:
>>  1) programming sequences and basically everything I know about the
>>     panel came from downstream android kernel.  I've started a wiki
>>     page to document how to translate from downstream kernel-msm way
>>     of doing things to upstream panel framework, which may be useful
>>     for others wanting to port downstream panel drivers for snapdragon
>>     devices:
>>
>>     https://github.com/freedreno/freedreno/wiki/DSI-Panel-Driver-Porting
>>
>>  2) The Sony phones at least (not sure if this is common) use one of
>>     several different panels, with runtime probing.  Depending on the
>>     device they seem to either use a gpio (simple) or send some DSI
>>     commands to read back the panel-id.  My rough thinking here about
>>     how to handle this is to implement a "panel-meta" driver (or maybe
>>     one each for the different probing methods), which would take a
>>     list of phandles to the actual candidate panels, and fwd the panel
>>     fxn calls to the chosen panel after probing.
>>
>>  .../bindings/panel/auo,novatek-1080p.txt           |  25 ++
>>  drivers/gpu/drm/panel/Kconfig                      |   9 +
>>  drivers/gpu/drm/panel/Makefile                     |   1 +
>>  drivers/gpu/drm/panel/panel-auo-novatek-1080p.c    | 470 +++++++++++++++++++++
>>  4 files changed, 505 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/panel/auo,novatek-1080p.txt
>>  create mode 100644 drivers/gpu/drm/panel/panel-auo-novatek-1080p.c
>>
>> diff --git a/Documentation/devicetree/bindings/panel/auo,novatek-1080p.txt b/Documentation/devicetree/bindings/panel/auo,novatek-1080p.txt
>> new file mode 100644
>> index 0000000..8a53093
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/panel/auo,novatek-1080p.txt
>> @@ -0,0 +1,25 @@
>> +AU Optronics Corporation 1080x1920 DSI panel
>> +
>> +This panel supports both video and command mode (although currently only video
>> +mode is implemented in the driver.
>> +
>> +Required properties:
>> +- compatible: should be "auo,novatek-1080p-vid"
>
> This looks a little generic for a compatible string. Can't we get at the
> specific panel model number that's been used? What if AUO ever produced
> some other Novatek panel with a 1080p resolution?

Maybe Sony or someone else can chime in?  That somewhat generic name
was all I could get from downstream android kernel.  I'm sure there is
a better possible name, although I have no means to find that out
myself.

> Also, what's the -vid suffix for?

the same panel seems to also work in cmd mode.. so idea was to have
-vid and -cmd compat strings to choose which mode to operate in.

>> +Optional properties:
>> +- power-supply: phandle of the regulator that provides the supply voltage
>> +- reset-gpio: phandle of gpio for reset line
>> +- backlight: phandle of the backlight device attached to the panel
>> +
>> +Example:
>> +
>> +     dsi@54300000 {
>> +             panel: panel@0 {
>> +                     compatible = "auo,novatek-1080p-vid";
>> +                     reg = <0>;
>> +
>> +                     power-supply = <...>;
>> +                     reset-gpio = <...>;
>> +                     backlight = <...>;
>> +             };
>> +     };
>> diff --git a/drivers/gpu/drm/panel/Kconfig b/drivers/gpu/drm/panel/Kconfig
>> index 6d64c7b..89f0e8c 100644
>> --- a/drivers/gpu/drm/panel/Kconfig
>> +++ b/drivers/gpu/drm/panel/Kconfig
>> @@ -43,4 +43,13 @@ config DRM_PANEL_SHARP_LQ101R1SX01
>>         To compile this driver as a module, choose M here: the module
>>         will be called panel-sharp-lq101r1sx01.
>>
>> +config DRM_PANEL_AUO_NOVATEK_1080P
>> +     tristate "AUO Novatek 1080p video mode panel"
>> +     depends on OF
>> +     depends on DRM_MIPI_DSI
>> +     depends on BACKLIGHT_CLASS_DEVICE
>> +     help
>> +       Say Y here if you want to enable support for AUO 1080p DSI panel
>> +       as found in some Sony XPERIA Z3 devices
>> +
>
> Can we sort this alphabetically, please?
>
>>  endmenu
>> diff --git a/drivers/gpu/drm/panel/Makefile b/drivers/gpu/drm/panel/Makefile
>> index 4b2a043..cbcfedf 100644
>> --- a/drivers/gpu/drm/panel/Makefile
>> +++ b/drivers/gpu/drm/panel/Makefile
>> @@ -2,3 +2,4 @@ obj-$(CONFIG_DRM_PANEL_SIMPLE) += panel-simple.o
>>  obj-$(CONFIG_DRM_PANEL_LD9040) += panel-ld9040.o
>>  obj-$(CONFIG_DRM_PANEL_S6E8AA0) += panel-s6e8aa0.o
>>  obj-$(CONFIG_DRM_PANEL_SHARP_LQ101R1SX01) += panel-sharp-lq101r1sx01.o
>> +obj-$(CONFIG_DRM_PANEL_AUO_NOVATEK_1080P) += panel-auo-novatek-1080p.o
>
> This too.
>
> Actually, nevermind. I have local patches to add vendor prefixes more
> consistently so that we can actually sort properly. I can fix that up
> in your patch when I apply.
>
>> diff --git a/drivers/gpu/drm/panel/panel-auo-novatek-1080p.c b/drivers/gpu/drm/panel/panel-auo-novatek-1080p.c
>> +static int auo_panel_init(struct auo_panel *auo)
>> +{
>> +     struct mipi_dsi_device *dsi = auo->dsi;
>> +     int ret;
>> +
>> +     dsi->mode_flags |= MIPI_DSI_MODE_LPM;
>> +
>> +     ret = mipi_dsi_generic_write(dsi, (u8[]){ 0xb0, 0x04 }, 2);
>
> I find this notation hard to read. Have you considered moving this into
> some sort of table that you can loop through? Or perhaps add some
> helpers, say, mipi_dsi_generic_writeb() and mipi_dsi_dcs_writeb() to
> help make this more readable?
>

Yeah, helper macro thing might be a reasonable idea.  The table option
makes it hard to use the helpers for things that are not non-standard,
or when you need delays, etc..


>> +     if (ret < 0)
>> +             return ret;
>> +
>> +     ret = mipi_dsi_dcs_write(dsi, 0xff, (u8[]){ 0xe0 }, 1);
>> +     if (ret < 0)
>> +             return ret;
>> +
>> +     ret = mipi_dsi_dcs_write(dsi, 0xfb, (u8[]){ 0x01 }, 1);
>> +     if (ret < 0)
>> +             return ret;
>> +
>> +     ret = mipi_dsi_dcs_write(dsi, 0xb5, (u8[]){ 0x86 }, 1);
>> +     if (ret < 0)
>> +             return ret;
>> +
>> +     ret = mipi_dsi_dcs_write(dsi, 0xb6, (u8[]){ 0x77 }, 1);
>> +     if (ret < 0)
>> +             return ret;
>> +
>> +     ret = mipi_dsi_dcs_write(dsi, 0xb8, (u8[]){ 0xad }, 1);
>> +     if (ret < 0)
>> +             return ret;
>> +
>> +     ret = mipi_dsi_dcs_write(dsi, 0xff, (u8[]){ 0x20 }, 1);
>> +     if (ret < 0)
>> +             return ret;
>> +
>> +     ret = mipi_dsi_dcs_write(dsi, 0xfb, (u8[]){ 0x01 }, 1);
>> +     if (ret < 0)
>> +             return ret;
>> +
>> +     ret = mipi_dsi_dcs_enter_sleep_mode(dsi);
>> +     if (ret < 0)
>> +             return ret;
>> +
>> +     ret = mipi_dsi_dcs_write(dsi, 0xff, (u8[]){ 0x24 }, 1);
>> +     if (ret < 0)
>> +             return ret;
>> +
>> +     ret = mipi_dsi_dcs_write(dsi, 0xfb, (u8[]){ 0x01 }, 1);
>> +     if (ret < 0)
>> +             return ret;
>> +
>> +     ret = mipi_dsi_dcs_write(dsi, 0xc6, (u8[]){ 0x00 }, 1);
>> +     if (ret < 0)
>> +             return ret;
>> +
>> +     ret = mipi_dsi_dcs_write(dsi, 0xc5, (u8[]){ 0x32 }, 1);
>> +     if (ret < 0)
>> +             return ret;
>> +
>> +     ret = mipi_dsi_dcs_write(dsi, 0x92, (u8[]){ 0x92 }, 1);
>> +     if (ret < 0)
>> +             return ret;
>> +
>> +     ret = mipi_dsi_dcs_write(dsi, 0xff, (u8[]){ 0x10 }, 1);
>> +     if (ret < 0)
>> +             return ret;
>> +
>> +     ret = mipi_dsi_dcs_set_tear_on(dsi, MIPI_DSI_DCS_TEAR_MODE_VBLANK);
>> +     if (ret < 0)
>> +             return ret;
>> +
>> +     ret = mipi_dsi_dcs_write(dsi, MIPI_DCS_SET_TEAR_SCANLINE,
>> +                     (u8[]){ 0x03, 0x00 }, 2);
>> +     if (ret < 0)
>> +             return ret;
>> +
>> +     ret = mipi_dsi_dcs_write(dsi, 0x3b, (u8[]){ 0x03, 0x30, 0x06 }, 3);
>> +     if (ret < 0)
>> +             return ret;
>> +
>> +     ret = mipi_dsi_dcs_write(dsi, 0xbb, (u8[]){ 0x10 }, 1);
>> +     if (ret < 0)
>> +             return ret;
>> +     msleep(1);
>> +
>> +     ret = mipi_dsi_dcs_exit_sleep_mode(dsi);
>> +     if (ret < 0)
>> +             return ret;
>> +     msleep(30);
>> +
>> +     return 0;
>> +}
>> +
>> +static int auo_panel_on(struct auo_panel *auo)
>> +{
>> +     struct mipi_dsi_device *dsi = auo->dsi;
>> +     int ret;
>> +
>> +     dsi->mode_flags |= MIPI_DSI_MODE_LPM;
>
> This is weird.
>
>> +     ret = mipi_dsi_dcs_set_display_on(dsi);
>> +     if (ret < 0)
>> +             return ret;
>> +
>> +     msleep(40);
>> +
>> +     return 0;
>> +}
>> +
>> +static int auo_panel_off(struct auo_panel *auo)
>> +{
>> +     struct mipi_dsi_device *dsi = auo->dsi;
>> +     int ret;
>> +
>> +     dsi->mode_flags &= ~MIPI_DSI_MODE_LPM;
>
> And this even more. Doesn't the panel work when you simply send
> everything in low-power mode?

I wouldn't expect low power mode to have enough bandwidth for the
video signal.. but otoh it seems like I need to use lpm for
power-on/off sequence.  Maybe we should wrap that up in a helper to
enable/disable lpm?  But that seemed a bit overkill.


>> +static int auo_panel_prepare(struct drm_panel *panel)
>> +{
>> +     struct auo_panel *auo = to_auo_panel(panel);
>> +     int ret;
>> +
>> +     if (auo->prepared)
>> +             return 0;
>> +
>> +     DRM_DEBUG("prepare\n");
>> +
>> +     if (auo->reset_gpio) {
>> +             gpiod_set_value(auo->reset_gpio, 0);
>> +             msleep(5);
>> +     }
>> +
>> +     ret = regulator_enable(auo->supply);
>> +     if (ret < 0)
>> +             return ret;
>> +
>> +     msleep(20);
>> +
>> +     if (auo->reset_gpio) {
>> +             gpiod_set_value(auo->reset_gpio, 1);
>> +             msleep(10);
>> +     }
>> +
>> +     msleep(150);
>> +
>> +     ret = auo_panel_init(auo);
>> +     if (ret) {
>> +             dev_err(panel->dev, "failed to init panel: %d\n", ret);
>> +             goto poweroff;
>> +     }
>> +
>> +     ret = auo_panel_on(auo);
>> +     if (ret) {
>> +             dev_err(panel->dev, "failed to set panel on: %d\n", ret);
>> +             goto poweroff;
>> +     }
>> +
>> +     auo->prepared = true;
>> +
>> +     return 0;
>> +
>> +poweroff:
>> +     regulator_disable(auo->supply);
>> +     if (auo->reset_gpio)
>> +             gpiod_set_value(auo->reset_gpio, 0);
>
> You should be able to do without the check here, because
> gpiod_set_value() will simply nop if the GPIO is NULL.

ok

> I assume you may not want to do it above because of the additional
> delays that are only relevant if you have a reset GPIO.
>
>> +     return ret;
>> +}
>> +
>> +static int auo_panel_enable(struct drm_panel *panel)
>> +{
>> +     struct auo_panel *auo = to_auo_panel(panel);
>> +
>> +     if (auo->enabled)
>> +             return 0;
>> +
>> +     DRM_DEBUG("enable\n");
>> +
>> +     if (auo->backlight) {
>> +             auo->backlight->props.power = FB_BLANK_UNBLANK;
>> +             backlight_update_status(auo->backlight);
>> +     }
>> +
>> +     auo->enabled = true;
>> +
>> +     return 0;
>> +}
>> +
>> +static int auo_panel_add(struct auo_panel *auo)
>> +{
>> +     struct device *dev= &auo->dsi->dev;
>> +     struct device_node *np;
>> +     int ret;
>> +
>> +     auo->mode = &default_mode;
>
> This seems to be unused.

yeah, I think just cargo cult'd from sharp panel..

>> +
>> +     auo->supply = devm_regulator_get(dev, "power");
>> +     if (IS_ERR(auo->supply))
>> +             return PTR_ERR(auo->supply);
>> +
>> +     auo->reset_gpio = devm_gpiod_get(dev, "reset", GPIOD_OUT_LOW);
>> +     if (IS_ERR(auo->reset_gpio)) {
>> +             dev_err(dev, "cannot get reset-gpios %ld\n",
>> +                     PTR_ERR(auo->reset_gpio));
>> +             auo->reset_gpio = NULL;
>> +     } else {
>> +             gpiod_direction_output(auo->reset_gpio, 0);
>
> Isn't that what GPIOD_OUT_LOW already does?
>

hmm, maybe?

BR,
-R
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/dri-devel




[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux