[PATCH] [v1,2/2] drm/panel:Add Boe Himax8279d MIPI-DSI LCD panel
Sam Ravnborg
sam at ravnborg.org
Sat Feb 23 22:38:35 UTC 2019
Hi Jerry.
Thanks for this driver submission.
Please see the following for some review feedback.
Sam
On Fri, Feb 22, 2019 at 10:32:08AM +0800, Jerry Han wrote:
> Add panel driver for it.
>
> Signed-off-by: Jerry Han <hanxu5 at huaqin.corp-partner.google.com>
> ---
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2019, Huaqin Electronics Co., Ltd
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
With the SPDX identifier this boilerplat can be dropped.
> + */
> +
> +#include <linux/backlight.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_device.h>
> +#include <linux/regulator/consumer.h>
> +
> +#include <drm/drmP.h>
Please do not use drmP.h for new drivers, as this header file is deprecated.
> +#include <drm/drm_crtc.h>
> +#include <drm/drm_mipi_dsi.h>
> +#include <drm/drm_panel.h>
> +
> +#include <video/mipi_display.h>
> +#include <linux/err.h>
Please verify that you need err.h include, often it is not needed.
> +
> +struct dsi_cmd_desc {
> + char wait;
> + char cmd;
> + char len;
> + char *payload;
> +};
> +
> +struct dsi_panel_cmds {
> + char *buf;
> + int blen;
> + struct dsi_cmd_desc *cmds;
> + int cmd_cnt;
> +};
> +
> +struct panel_info {
> + struct drm_panel base;
> + struct mipi_dsi_device *link;
> +
> + struct backlight_device *backlight;
> + struct gpio_desc *reset_gpio;
> + struct gpio_desc *pp33000_gpio;
> + struct gpio_desc *pp18000_gpio;
> +
> + u32 rst_seq[10];
> + u32 rst_seq_len;
> +
> + struct dsi_panel_cmds *on_cmds;
> + struct dsi_panel_cmds *off_cmds;
> + struct drm_display_mode *panel_display_mode;
> +
> + unsigned int panel_width_mm;
> + unsigned int panel_height_mm;
> + unsigned int per_color_channel;
> +
> + bool prepared;
> + bool enabled;
> +};
> +
> +static int dsi_parse_reset_seq(struct device_node *np, u32 rst_seq[10],
> + u32 *rst_len, const char *name)
Indent following line so "u32" i aligned right after first opening '('
This comment is general, please fix for all functions.
> +{
> + int num = 0, i;
> + int rc;
> + struct property *data;
> + u32 tmp[10];
> + *rst_len = 0;
> + data = of_find_property(np, name, &num);
> +
> + num /= sizeof(u32);
> + if (!data || !num || num > 10 || num % 2) {
> + pr_err("%s:%d, error reading %s, length found = %d\n",
> + __func__, __LINE__, name, num);
> + return -EINVAL;
> + }
> +
> + rc = of_property_read_u32_array(np, name, tmp, num);
> + if (rc) {
> + pr_err("%s:%d, error reading %s, rc = %d\n",
> + __func__, __LINE__, name, rc);
> + return -EINVAL;
> + }
> +
> + for (i = 0; i < num; ++i) {
> + rst_seq[i] = tmp[i];
> + *rst_len = num;
> + }
> + return 0;
> +}
The above function is used only to parse "dsi-reset-sequence".
First it looks sub-optimal the way it is done.
Second, there is no documentation of "dsi-reset-sequence".
Please verify what other drivers do that has a similar need.
It looks like one can specify an array of GPIOs and there are other ways to do this.
> +
> +ssize_t mipi_dsi_dcs_write_delay(struct mipi_dsi_device *dsi, u8 cmd,
> + const void *data, size_t len, size_t time)
> +{
> + ssize_t ret;
> +
> + if (len == 0)
> + ret = mipi_dsi_dcs_write(dsi, cmd, NULL, 0);
> + else
> + ret = mipi_dsi_dcs_write(dsi, cmd, data, len);
> +
> + msleep(time);
> +
> + return ret;
> +}
> +
> +static int write_cmds(struct mipi_dsi_device *dsi_device,
> + struct dsi_panel_cmds *init_cmds)
> +{
> + struct dsi_cmd_desc *one_cmd = init_cmds->cmds;
> + int i = 0;
> + int ret;
> +
> + for (i = 0; i < init_cmds->cmd_cnt; i++) {
> + if (one_cmd == NULL)
> + return -EINVAL;
> +
> + ret = mipi_dsi_dcs_write_delay(dsi_device,
> + one_cmd->cmd, one_cmd->payload,
> + one_cmd->len, one_cmd->wait);
> + one_cmd++;
> + }
> +
> + return ret;
> +}
> +
> +static int parse_dcs_cmds(struct device_node *np,
> + struct dsi_panel_cmds *pcmds, char *cmd_key)
This function is used to parse on and off commands fro mthe DT.
This is commands that in other drivers are part of the driver, and not something
to be read in the DT.
Also, un-documented properties are sued.
Please consider to use the same approach as used in other drivers, and accept that
the driver will hardcode more. And then less in the DT.
> +{
> + const char *data;
> + int blen = 0, len, cnt = 0;
> + char *buf, *bp;
> + struct dsi_cmd_desc *temp_cmd;
> +
> + data = of_get_property(np, cmd_key, &blen);
> + if (!data) {
> + pr_err("%s: failed, key=%s\n", __func__, cmd_key);
> + return -ENOMEM;
> + }
> +
> + buf = kzalloc(sizeof(char) * blen, GFP_KERNEL);
> + if (!buf)
> + return -ENOMEM;
> +
> + memcpy(buf, data, blen);
> +
> + /* scan dcs commands */
> + bp = buf;
> + len = blen;
> +
> + /* get how much cmd cnt */
> + while (len > 0) {
> + cnt++;
> + len = len - *(bp);
> + bp = bp + *(bp);
> + }
> +
> + pcmds->cmd_cnt = cnt;
> +
> + pcmds->cmds = kcalloc(cnt, sizeof(struct dsi_cmd_desc), GFP_KERNEL);
> + if (!pcmds->cmds)
> + return -ENOMEM;
> + temp_cmd = pcmds->cmds;
> +
> + bp = buf;
> + len = blen;
> +
> + while (cnt != 0) {
> + temp_cmd->wait = *(bp+1);
> + temp_cmd->cmd = *(bp+2);
> + temp_cmd->len = *(bp) - 3;
> + temp_cmd->payload = bp + 3;
> +
> + cnt--;
> + temp_cmd++;
> + bp = bp + *(bp);
> + }
> +
> + pcmds->buf = buf;
> + pcmds->blen = blen;
> +
> + return 0;
> +}
> +
> +static int panel_parse_dt(struct panel_info *pinfo)
Likewise.
A lot of info is in this function read from DT, that is hardcoded in other drivers.
Follow what other drivers do and drop reading this from DT.
> +
> +static inline struct panel_info *to_panel_info(struct drm_panel *panel)
> +{
> + return container_of(panel, struct panel_info, base);
> +}
> +
> +static int panel_disable(struct drm_panel *panel)
> +{
> + struct panel_info *pinfo = to_panel_info(panel);
> +
> + if (!pinfo->enabled)
> + return 0;
> +
> + backlight_disable(pinfo->backlight);
There is really (to the best of my knowledge) no need to have a boolean
to protect from callign backlight_disable() when already disabled.
> +
> + pinfo->enabled = false;
> +
> + return 0;
> +}
> +
> +static int panel_unprepare(struct drm_panel *panel)
> +{
> + struct panel_info *pinfo = to_panel_info(panel);
> + int err;
> +
> + if (!pinfo->prepared)
> + return 0;
> +
> + err = write_cmds(pinfo->link, pinfo->off_cmds);
> + if (err < 0) {
> + dev_err(panel->dev,
> + "failed to send DCS Off Code: %d\n", err);
> + return err;
> + }
> +
> + usleep_range(1000, 1000);
> + gpiod_set_value_cansleep(pinfo->reset_gpio, 0);
> + gpiod_set_value_cansleep(pinfo->pp33000_gpio, 0);
> + gpiod_set_value_cansleep(pinfo->pp18000_gpio, 0);
> +
> + pinfo->prepared = false;
> +
> + return 0;
> +}
> +
> +static int panel_prepare(struct drm_panel *panel)
> +{
> + struct panel_info *pinfo = to_panel_info(panel);
> + int err, i;
> +
> + if (pinfo->prepared)
> + return 0;
> +
> + gpiod_set_value_cansleep(pinfo->pp18000_gpio, 1);
> + gpiod_set_value_cansleep(pinfo->pp33000_gpio, 1);
> +
> + /* reset sequence */
> + for (i = 0; i < pinfo->rst_seq_len; ++i) {
> + gpiod_set_value_cansleep(pinfo->reset_gpio, pinfo->rst_seq[i]);
> + if (pinfo->rst_seq[++i])
> + usleep_range(pinfo->rst_seq[i] * 1000,
> + pinfo->rst_seq[i] * 1000);
> + }
> +
> + /* send init code */
> + err = write_cmds(pinfo->link, pinfo->on_cmds);
> + if (err < 0) {
> + dev_err(panel->dev,
> + "failed to send DCS Init Code: %d\n", err);
> + goto poweroff;
> + }
> +
> + pinfo->prepared = true;
> +
> + return 0;
> +
> +poweroff:
> + gpiod_set_value_cansleep(pinfo->reset_gpio, 0);
> + gpiod_set_value_cansleep(pinfo->pp33000_gpio, 0);
> + gpiod_set_value_cansleep(pinfo->pp18000_gpio, 0);
The above sequence is used in two placed.
Put it in a helper function.
> + return err;
> +}
> +
> +static int panel_enable(struct drm_panel *panel)
> +{
> + struct panel_info *pinfo = to_panel_info(panel);
> + int ret;
> +
> + if (pinfo->enabled)
> + return 0;
> +
> + ret = backlight_enable(pinfo->backlight);
> + if (ret) {
> + DRM_DEV_ERROR(panel->drm->dev,
> + "Failed to enable backlight %d\n", ret);
> + return ret;
> + }
> +
> + pinfo->enabled = true;
> +
> + return 0;
> +}
> +
> +static int panel_get_modes(struct drm_panel *panel)
> +{
> + struct panel_info *pinfo = to_panel_info(panel);
> + struct drm_display_mode *mode;
> +
> + if (!pinfo->panel_display_mode) {
> + pr_err("Get panel_display_mode fail!\n");
> + return -1;
> + }
> +
> + mode = drm_mode_duplicate(panel->drm, pinfo->panel_display_mode);
> + if (!mode) {
> + pr_err("failed to add mode\n");
> + return -ENOMEM;
> + }
> +
> + drm_mode_set_name(mode);
> +
> + drm_mode_probed_add(panel->connector, mode);
> +
> + panel->connector->display_info.width_mm = pinfo->panel_width_mm;
> + panel->connector->display_info.height_mm = pinfo->panel_height_mm;
> + panel->connector->display_info.bpc = pinfo->per_color_channel;
> +
> + return 1;
> +}
> +
> +static const struct drm_panel_funcs panel_funcs = {
> + .disable = panel_disable,
> + .unprepare = panel_unprepare,
> + .prepare = panel_prepare,
> + .enable = panel_enable,
> + .get_modes = panel_get_modes,
> +};
> +
> +static const struct of_device_id panel_of_match[] = {
> + { .compatible = "boe,himax8279d", },
> + { }
To match other drivers use:
{ /* Sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, panel_of_match);
> +
> +static int panel_add(struct panel_info *pinfo)
> +{
> + struct device *dev = &pinfo->link->dev;
> + struct device_node *np;
> + int err;
> +
pinfo->pp33000_gpio = devm_gpiod_get_optional(dev,
"pp33000",
GPIOD_OUT_HIGH);
Indent is wrong here.
Please indent as shown above where the parameters starts right after
the opening '('
Same goes for several places in this file.
> + if (IS_ERR(pinfo->pp33000_gpio)) {
> + err = PTR_ERR(pinfo->pp33000_gpio);
> + dev_err(dev, "failed to get pp33000 gpio: %d\n", err);
> + pinfo->pp33000_gpio = NULL;
> + }
> +
> + pinfo->pp18000_gpio = devm_gpiod_get_optional(dev,
> + "pp18000",
> + GPIOD_OUT_HIGH);
> + if (IS_ERR(pinfo->pp18000_gpio)) {
> + err = PTR_ERR(pinfo->pp18000_gpio);
> + dev_err(dev, "failed to get pp18000 gpio: %d\n", err);
> + pinfo->pp18000_gpio = NULL;
> + }
> +
> + pinfo->reset_gpio = devm_gpiod_get_optional(dev, "enable",
> + GPIOD_OUT_HIGH);
> + if (IS_ERR(pinfo->reset_gpio)) {
> + err = PTR_ERR(pinfo->reset_gpio);
> + dev_err(dev, "failed to get enable gpio: %d\n", err);
> + pinfo->reset_gpio = NULL;
> + }
> +
> + np = of_parse_phandle(dev->of_node, "backlight", 0);
> + if (np) {
> + pinfo->backlight = of_find_backlight_by_node(np);
> + of_node_put(np);
> +
> + if (!pinfo->backlight)
> + return -EPROBE_DEFER;
> + }
Use devm_of_find_backlight(). See other drivers how this
simplifies the drivers.
All put_device(&pinfo->backlight->dev) in this file can be dropped
as it is done automatically.
> +
> +static void panel_del(struct panel_info *pinfo)
> +{
> + if (pinfo->base.dev)
> + drm_panel_remove(&pinfo->base);
> +
> + put_device(&pinfo->backlight->dev);
> +}
> +
> +static int panel_probe(struct mipi_dsi_device *dsi)
> +{
> + struct panel_info *pinfo;
> + int err;
> +
> + pinfo = devm_kzalloc(&dsi->dev, sizeof(*pinfo), GFP_KERNEL);
> + if (!pinfo)
> + return -ENOMEM;
> +
> + pinfo->link = dsi;
> + panel_parse_dt(pinfo);
> + mipi_dsi_set_drvdata(dsi, pinfo);
> +
> + err = panel_add(pinfo);
> + if (err < 0)
> + return err;
> +
> + return mipi_dsi_attach(dsi);
> +}
> +
> +static int panel_remove(struct mipi_dsi_device *dsi)
> +{
> + struct panel_info *pinfo = mipi_dsi_get_drvdata(dsi);
> + int err;
> +
> + err = panel_unprepare(&pinfo->base);
> + if (err < 0)
> + DRM_DEV_ERROR(&dsi->dev, "failed to unprepare panel: %d\n",
> + err);
No need to writhe this, the called function does it already.
> +
> + err = panel_disable(&pinfo->base);
> + if (err < 0)
> + DRM_DEV_ERROR(&dsi->dev, "failed to disable panel: %d\n", err);
> +
> + err = mipi_dsi_detach(dsi);
> + if (err < 0)
> + DRM_DEV_ERROR(&dsi->dev, "failed to detach from DSI host: %d\n",
> + err);
> +
> + drm_panel_detach(&pinfo->base);
> + panel_del(pinfo);
Inline this function.
> +
> + return 0;
> +}
> +
> +static void panel_shutdown(struct mipi_dsi_device *dsi)
> +{
> + struct panel_info *pinfo = mipi_dsi_get_drvdata(dsi);
> +
> + panel_unprepare(&pinfo->base);
> + panel_disable(&pinfo->base);
> +}
> +
> +static struct mipi_dsi_driver panel_driver = {
> + .driver = {
> + .name = "panel-innolux-ota7290b",
> + .of_match_table = panel_of_match,
> + },
> + .probe = panel_probe,
> + .remove = panel_remove,
> + .shutdown = panel_shutdown,
> +};
> +module_mipi_dsi_driver(panel_driver);
> +
> +MODULE_AUTHOR("Jerry Han <hanxu5 at huaqin.corp-partner.google.com>");
> +MODULE_DESCRIPTION("Boe Himax8279d driver");
> +MODULE_LICENSE("GPL v2");
> --
> 1.9.1
>
> _______________________________________________
> dri-devel mailing list
> dri-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
More information about the dri-devel
mailing list