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@xxxxxxxxxxx> 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@xxxxxxxxxxx>
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
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
