LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Maxime Ripard <maxime.ripard@bootlin.com>
To: Linus Walleij <linus.walleij@linaro.org>
Cc: Thierry Reding <thierry.reding@gmail.com>,
	Chen-Yu Tsai <wens@csie.org>,
	"open list:DRM PANEL DRIVERS" <dri-devel@lists.freedesktop.org>,
	Gustavo Padovan <gustavo@padovan.org>,
	Daniel Vetter <daniel.vetter@intel.com>,
	Maarten Lankhorst <maarten.lankhorst@linux.intel.com>,
	Sean Paul <seanpaul@chromium.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v4 2/3] drm/panel: Add Ilitek ILI9881c panel driver
Date: Tue, 29 May 2018 11:23:16 +0200	[thread overview]
Message-ID: <20180529092316.twbwhjqutvnzzfse@flea> (raw)
In-Reply-To: <CACRpkdY+W02YfJb1zswy40Q0TGKupCFEG7=w78_A3VmFJ97GaA@mail.gmail.com>

Hi Linus,

On Fri, May 11, 2018 at 05:54:27PM +0200, Linus Walleij wrote:
> Hi Maxime,
> 
> sorry that noone had much time to look at the driver.
> But I can help out, hopefully.

Thanks for your feedback :)

> On Fri, May 4, 2018 at 5:23 PM, Maxime Ripard <maxime.ripard@bootlin.com> wrote:
> 
> > The LHR050H41 panel is the panel shipped with the BananaPi M2-Magic, and is
> > based on the Ilitek ILI9881c Controller. Add a driver for it, modelled
> > after the other Ilitek controller drivers.
> >
> > Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com>
> 
> Nice, I have some experience with those.
> 
> > +config DRM_PANEL_ILITEK_ILI9881C
> > +       tristate "Ilitek ILI9881C-based panels"
> > +       depends on OF
> > +       depends on DRM_MIPI_DSI
> > +       depends on BACKLIGHT_CLASS_DEVICE
> 
> If it absolutely needs DRM_MIPI_DSI and
> BACKLIGHT_CLASS_DEVICE it maybe you should
> be more helpful to the user to just select it?
> 
> Depending on OF is fine, that is more of a platform
> property.

All the other panels in this file seems to be using a depends on for
these two, so I'd rather remain consistent on this.

> > +struct ili9881c {
> > +       struct drm_panel        panel;
> > +       struct mipi_dsi_device  *dsi;
> > +
> > +       struct backlight_device *backlight;
> > +       struct gpio_desc        *power;
> 
> Should this not be modeled using a fixed regulator instead?

Probably, yes. On my board it's a GPIO that goes through the
connector, but it seems like the controller itself takes a regulator,
so there's probably a GPIO-controlled regulator on the display PCB
somewhere. I'll change it.

> > +static const struct ili9881c_instr ili9881c_init[] = {
> > +       ILI9881C_SWITCH_PAGE_INSTR(3),
> > +       ILI9881C_COMMAND_INSTR(0x01, 0x00),
> > +       ILI9881C_COMMAND_INSTR(0x02, 0x00),
> > +       ILI9881C_COMMAND_INSTR(0x03, 0x73),
> > +       ILI9881C_COMMAND_INSTR(0x04, 0x03),
> > +       ILI9881C_COMMAND_INSTR(0x05, 0x00),
> > +       ILI9881C_COMMAND_INSTR(0x06, 0x06),
> > +       ILI9881C_COMMAND_INSTR(0x07, 0x06),
> (...)
> 
> This is a bit hard to understand to say the least.

I know :/

> If this comes from a datasheet some more elaborate construction of
> the commands would be nice, maybe using some #defines?
> 
> If this just comes from some code drop or reverse engineering,
> OK bad luck, I can live with it :)

This comes from a code drop (or rather, an obscure initialisation
sequence coming straight from the vendor without any documentation or
comment associated to it).

> It looks a bit like you're uploading a small firmware.
> 
> > +static int ili9881c_switch_page(struct ili9881c *ctx, u8 page)
> > +{
> > +       u8 buf[4] = { 0xff, 0x98, 0x81, page };
> 
> That is also a bit unclear wrt what is going on.

My understanding is that the controller maps some registers (that
might be accessible through other buses if the controller is setup to
use something other than DCS) to DCS commands. Since there's more
commands than DCS would allow, it's setup in several pages, with each
pages having its own set of commands, and the page 0 accepting the
usual standard DCS commands.

It really doesn't look to me like a firmware, but just a poorly
documented initialisation sequence.. I'll add a comment

> > +static const struct drm_display_mode default_mode = {
> > +       .clock          = 62000,
> > +       .vrefresh       = 60,
> > +
> > +       .hdisplay       = 720,
> > +       .hsync_start    = 720 + 10,
> > +       .hsync_end      = 720 + 10 + 20,
> > +       .htotal         = 720 + 10 + 20 + 30,
> > +
> > +       .vdisplay       = 1280,
> > +       .vsync_start    = 1280 + 10,
> > +       .vsync_end      = 1280 + 10 + 10,
> > +       .vtotal         = 1280 + 10 + 10 + 20,
> > +};
> 
> Is that the default mode for this Ilitek device or just for the
> BananaPi? If it is the latter it should be named
> bananapi_default_mode or something so as not to confuse
> the next user of this driver.

I'll do it, thanks!
Maxime

-- 
Maxime Ripard, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com

  reply	other threads:[~2018-05-29  9:23 UTC|newest]

Thread overview: 11+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-05-04 15:23 [PATCH v4 0/3] drm/panel: Add Ilitek ILI9881c controller driver Maxime Ripard
2018-05-04 15:23 ` [PATCH v4 1/3] dt-bindings: panel: Add the Ilitek ILI9881c panel documentation Maxime Ripard
2018-05-11 15:40   ` Linus Walleij
2018-05-11 15:55   ` Linus Walleij
2018-05-04 15:23 ` [PATCH v4 2/3] drm/panel: Add Ilitek ILI9881c panel driver Maxime Ripard
2018-05-05  3:48   ` kbuild test robot
2018-05-05  7:15   ` kbuild test robot
2018-05-11 15:54   ` Linus Walleij
2018-05-29  9:23     ` Maxime Ripard [this message]
2018-05-04 15:23 ` [PATCH v4 3/3] [DO NOT MERGE] arm: dts: sun8i: bpi-m2m: Add DSI display Maxime Ripard
2018-05-05  7:15   ` kbuild test robot

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20180529092316.twbwhjqutvnzzfse@flea \
    --to=maxime.ripard@bootlin.com \
    --cc=daniel.vetter@intel.com \
    --cc=dri-devel@lists.freedesktop.org \
    --cc=gustavo@padovan.org \
    --cc=linus.walleij@linaro.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=maarten.lankhorst@linux.intel.com \
    --cc=seanpaul@chromium.org \
    --cc=thierry.reding@gmail.com \
    --cc=wens@csie.org \
    --subject='Re: [PATCH v4 2/3] drm/panel: Add Ilitek ILI9881c panel driver' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).