LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Linus Walleij <linus.walleij@linaro.org>
To: Maxime Ripard <maxime.ripard@bootlin.com>
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: Fri, 11 May 2018 17:54:27 +0200	[thread overview]
Message-ID: <CACRpkdY+W02YfJb1zswy40Q0TGKupCFEG7=w78_A3VmFJ97GaA@mail.gmail.com> (raw)
In-Reply-To: <e9348b12b59e6236a7965ceba94011a1e5b72720.1525447375.git-series.maxime.ripard@bootlin.com>

Hi Maxime,

sorry that noone had much time to look at the driver.
But I can help out, hopefully.

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.

> +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?

> +       struct gpio_desc        *reset;
> +};

> +enum ili9881c_op {
> +       ILI9881C_SWITCH_PAGE,
> +       ILI9881C_COMMAND,
> +};
> +
> +struct ili9881c_instr {
> +       enum ili9881c_op        op;
> +
> +       union arg {
> +               struct cmd {
> +                       u8      cmd;
> +                       u8      data;
> +               } cmd;
> +               u8      page;
> +       } arg;
> +};
> +
> +#define ILI9881C_SWITCH_PAGE_INSTR(_page)      \
> +       {                                       \
> +               .op = ILI9881C_SWITCH_PAGE,     \
> +               .arg = {                        \
> +                       .page = (_page),        \
> +               },                              \
> +       }
> +
> +#define ILI9881C_COMMAND_INSTR(_cmd, _data)            \
> +       {                                               \
> +               .op = ILI9881C_COMMAND,         \
> +               .arg = {                                \
> +                       .cmd = {                        \
> +                               .cmd = (_cmd),          \
> +                               .data = (_data),        \
> +                       },                              \
> +               },                                      \
> +       }

That looks clever.

> +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. 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 :)

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.

> +static int ili9881c_prepare(struct drm_panel *panel)
> +{
> +       struct ili9881c *ctx = panel_to_ili9881c(panel);
> +       unsigned int i;
> +       int ret;
> +
> +       /* Power the panel */
> +       gpiod_set_value(ctx->power, 1);
> +       msleep(5);

So isn't this a fixed regulator using a GPIO?

> +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.

> +       ctx->power = devm_gpiod_get(&dsi->dev, "power", GPIOD_OUT_LOW);
> +       if (IS_ERR(ctx->power)) {
> +               dev_err(&dsi->dev, "Couldn't get our power GPIO\n");
> +               return PTR_ERR(ctx->power);
> +       }

So I'm a bit sceptical about this one.

Yours,
Linus Walleij

  parent reply	other threads:[~2018-05-11 15:54 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 [this message]
2018-05-29  9:23     ` Maxime Ripard
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='CACRpkdY+W02YfJb1zswy40Q0TGKupCFEG7=w78_A3VmFJ97GaA@mail.gmail.com' \
    --to=linus.walleij@linaro.org \
    --cc=daniel.vetter@intel.com \
    --cc=dri-devel@lists.freedesktop.org \
    --cc=gustavo@padovan.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=maarten.lankhorst@linux.intel.com \
    --cc=maxime.ripard@bootlin.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).