| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <54915A78.9040904@freescale.com>
Date: Wed, 17 Dec 2014 18:27:04 +0800
From: Liu Ying <Ying.Liu@...escale.com>
To: Thierry Reding <thierry.reding@...il.com>
CC: <dri-devel@...ts.freedesktop.org>, <devicetree@...r.kernel.org>,
<linux-kernel@...r.kernel.org>,
<linux-arm-kernel@...ts.infradead.org>, <p.zabel@...gutronix.de>,
<shawn.guo@...aro.org>, <kernel@...gutronix.de>,
<linux@....linux.org.uk>, <mturquette@...aro.org>,
<airlied@...ux.ie>
Subject: Re: [PATCH RFC 10/15] drm: panel: Add support for Himax HX8369A MIPI
DSI panel
On 12/10/2014 10:03 PM, Thierry Reding wrote:
> On Wed, Dec 10, 2014 at 04:37:23PM +0800, Liu Ying wrote:
>> This patch adds support for Himax HX8369A MIPI DSI panel.
>>
>> Signed-off-by: Liu Ying <Ying.Liu@...escale.com>
>> ---
>> .../devicetree/bindings/panel/himax,hx8369a.txt | 86 +++
>> drivers/gpu/drm/panel/Kconfig | 6 +
>> drivers/gpu/drm/panel/Makefile | 1 +
>> drivers/gpu/drm/panel/panel-hx8369a.c | 627 +++++++++++++++++++++
>> 4 files changed, 720 insertions(+)
>> create mode 100644 Documentation/devicetree/bindings/panel/himax,hx8369a.txt
>> create mode 100644 drivers/gpu/drm/panel/panel-hx8369a.c
>>
>> diff --git a/Documentation/devicetree/bindings/panel/himax,hx8369a.txt b/Documentation/devicetree/bindings/panel/himax,hx8369a.txt
>> new file mode 100644
>> index 0000000..6fe251e
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/panel/himax,hx8369a.txt
>> @@ -0,0 +1,86 @@
>> +Himax HX8369A WVGA 16.7M color TFT single chip driver with internal GRAM
>> +
>> +Himax HX8369A is a WVGA resolution driving controller.
>> +It is designed to provide a single chip solution that combines a source
>> +driver and power supply circuits to drive a TFT dot matrix LCD with
>> +480RGBx864 dots at the maximum.
>> +
>> +The HX8369A supports several interface modes, including MPU MIPI DBI Type
>> +A/B mode, MIPI DPI/DBI Type C mode, MIPI DSI video mode, MIPI DSI command
>> +mode and MDDI mode. The interface mode is selected by the external hardware
>> +pins BS[3:0].
>> +
>> +Currently, only the MIPI DSI video mode is supported.
>> +
>> +Required properties:
>> + - compatible: "himax,hx8369a-dsi"
>> + - reg: the virtual channel number of a DSI peripheral
>> + - reset-gpios: a GPIO spec for the reset pin
>> + - data-lanes: the data lane number of a DSI peripheral
>
> This is implied by the compatible already.
Accepted.
>
>> + - display-timings: timings for the connected panel as described by [1]
>
> Also implied by the compatible value.
Accepted.
>
>> + - bs: the interface mode number described by the following table
>> + --------------------------
>> + | DBI_TYPE_A_8BIT | 0 |
>> + | DBI_TYPE_A_9BIT | 1 |
>> + | DBI_TYPE_A_16BIT | 2 |
>> + | DBI_TYPE_A_18BIT | 3 |
>> + | DBI_TYPE_B_8BIT | 4 |
>> + | DBI_TYPE_B_9BIT | 5 |
>> + | DBI_TYPE_B_16BIT | 6 |
>> + | DBI_TYPE_B_18BIT | 7 |
>> + | DSI_CMD_MODE | 8 |
>> + | DBI_TYPE_B_24BIT | 9 |
>> + | DSI_VIDEO_MODE | 10 |
>> + | MDDI | 11 |
>> + | DPI_DBI_TYPE_C_OPT1 | 12 |
>> + | DPI_DBI_TYPE_C_OPT2 | 13 |
>> + | DPI_DBI_TYPE_C_OPT3 | 14 |
>> + --------------------------
>
> Can this not be inferred by the driver? If it's a DSI driver can't it
> select between DSI_VIDEO_MODE or DSI_CMD_MODE based on its capabilities?
> That is, if the panel driver can setup command mode, shouldn't it be
> using command mode in that case? And use DSI_VIDEO_MODE otherwise?
I may remove this property.
But, I choose not to add any logic in the host and slave drivers to
handle the interface mode selection at present, since I only support
the DSI_VIDEO_MODE now. Is this acceptable?
>
>> +Optional properties:
>> + - power-on-delay: delay after turning regulators on [ms]
>> + - reset-delay: delay after reset sequence [ms]
>
> Surely these are constant for this panel?
Accepted.
>
>> + - panel-width-mm: physical panel width [mm]
>> + - panel-height-mm: physical panel height [mm]
>
> These are also implied by compatible.
>
Accepted.
>> +Example:
>> + panel@0 {
>> + compatible = "himax,hx8369a-dsi";
>> + reg = <0>;
>> + pinctrl-names = "default";
>> + pinctrl-0 = <&pinctrl_mipi_panel>;
>> + reset-gpios = <&gpio6 11 GPIO_ACTIVE_LOW>;
>> + reset-delay = <120>;
>> + bs2-gpios = <&gpio6 14 GPIO_ACTIVE_HIGH>;
>> + data-lanes = <2>;
>> + panel-width-mm = <45>;
>> + panel-height-mm = <76>;
>> + bs = <10>;
>> + status = "okay";
>
> status = "okay" is the default, so it probably shouldn't be in this
> example.
>
Accepted.
>> diff --git a/drivers/gpu/drm/panel/Kconfig b/drivers/gpu/drm/panel/Kconfig
>> index 024e98e..f1a5b58 100644
>> --- a/drivers/gpu/drm/panel/Kconfig
>> +++ b/drivers/gpu/drm/panel/Kconfig
>> @@ -40,4 +40,10 @@ 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_HX8369A
>> + tristate "HX8369A panel"
>> + depends on OF
>> + select DRM_MIPI_DSI
>> + select VIDEOMODE_HELPERS
>> +
>
> This should be sorted alphabetically. I think it would also be a good
> idea to use a HIMAX prefix here, just to reduce the potential for name
> clashes.
Accepted.
>
>> diff --git a/drivers/gpu/drm/panel/Makefile b/drivers/gpu/drm/panel/Makefile
>> index 4b2a043..d6768ca 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_HX8369A) += panel-hx8369a.o
>
> Needs to be sorted alphabetically, too.
Accepted.
>
>> diff --git a/drivers/gpu/drm/panel/panel-hx8369a.c b/drivers/gpu/drm/panel/panel-hx8369a.c
> [...]
>> +#include <drm/drmP.h>
>> +#include <drm/drm_mipi_dsi.h>
>> +#include <drm/drm_panel.h>
>> +
>> +#include <linux/gpio/consumer.h>
>> +#include <linux/regulator/consumer.h>
>> +
>> +#include <video/mipi_display.h>
>> +#include <video/of_videomode.h>
>> +#include <video/videomode.h>
>> +
>> +#define SETCLUMN_ADDR 0x2a
>> +#define SETPAGE_ADDR 0x2b
>> +#define SETPIXEL_FMT 0x3a
>
> There are standard DCS functions for these now.
Accepted.
>
>> +#define WRDISBV 0x51
>> +#define WRCTRLD 0x53
>> +#define WRCABC 0x55
>> +#define SETPOWER 0xb1
>> +#define SETDISP 0xb2
>> +#define SETCYC 0xb4
>> +#define SETVCOM 0xb6
>> +#define SETEXTC 0xb9
>> +#define SETMIPI 0xba
>> +#define SETPANEL 0xcc
>> +#define SETGIP 0xd5
>> +#define SETGAMMA 0xe0
>
> Watch your indentation here and above. Use tabs and spaces more
> consistently.
Accepted.
>
>> +static bool hx8369a_is_dsi_interface(struct hx8369a *ctx) {
>
> Coding style: opening { goes on a line by itself.
Accepted.
>
>> + if (ctx->mpu_interface == HX8369A_DSI_VIDEO_MODE ||
>> + ctx->mpu_interface == HX8369A_DSI_CMD_MODE)
>> + return true;
>> +
>> + return false;
>> +}
>> +
>> +static void hx8369a_dcs_write(struct hx8369a *ctx, const void *data, size_t len)
>> +{
>> + struct mipi_dsi_device *dsi = to_mipi_dsi_device(ctx->dev);
>> + ssize_t ret;
>> +
>> + ret = mipi_dsi_dcs_write_buffer(dsi, data, len);
>> + if (ret < 0)
>> + dev_err(ctx->dev, "error %zd writing dcs seq: %*ph\n", ret, len,
>> + data);
>> +}
>
> You really shouldn't have based this on the Samsung drivers. You really
> should do proper error handling here. These error messages are going to
> be completely cryptic and very hard for people to look up in the driver
> as opposed to a simple specific error message like:
>
> dev_err(ctx->dev, "failed to do XYZ: %d\n", err);
>
> which will immediately let you look up the right location without a need
> to decode the hexdump and look for the correct array.
Accepted.
>
>> +#define hx8369a_dcs_write_seq(ctx, seq...) \
>> +({\
>> + const u8 d[] = { seq };\
>> + BUILD_BUG_ON_MSG(ARRAY_SIZE(d) > 64, "DCS sequence too big for stack");\
>> + hx8369a_dcs_write(ctx, d, ARRAY_SIZE(d));\
>> +})
>> +
>> +#define hx8369a_dcs_write_seq_static(ctx, seq...) \
>> +({\
>> + static const u8 d[] = { seq };\
>> + hx8369a_dcs_write(ctx, d, ARRAY_SIZE(d));\
>> +})
>> +
>> +static void hx8369a_dsi_set_display_related_register(struct hx8369a *ctx)
>> +{
>> + u8 sec_p = (ctx->res_sel << 4) | 0x03;
>> +
>> + hx8369a_dcs_write_seq(ctx, SETDISP, 0x00, sec_p, 0x03,
>> + 0x03, 0x70, 0x00, 0xff, 0x00, 0x00, 0x00, 0x00,
>> + 0x03, 0x03, 0x00, 0x01);
>> +}
>
> This and all of the other initialization functions below completely
> ignore any errors. Other than spamming the kernel log the user won't get
> any indication of anything going wrong.
>
>> +static void hx8369a_dsi_set_interface_pixel_fomat(struct hx8369a *ctx)
>> +{
> [...]
>> +}
>> +
>> +static void hx8369a_dsi_set_column_address(struct hx8369a *ctx)
>> +{
> [...]
>> +}
>> +
>> +static void hx8369a_dsi_set_page_address(struct hx8369a *ctx)
>> +{
> [...]
>> +}
>
> We have standard functions for these, please use them.
>
>> +static void hx8369a_dsi_set_extension_command(struct hx8369a *ctx)
>> +{
>> + hx8369a_dcs_write_seq_static(ctx, SETEXTC, 0xff, 0x83, 0x69);
>> +}
>
> What does this do exactly? It seems to be more than setting an extension
> command.
The panel data sheet tells that several commands rely on the SETEXTC,
such as SETMIPI, SETPANEL and SETCYC ..., which are used in this driver.
>
>> +static void hx8369a_dsi_set_maximum_return_packet_size(struct hx8369a *ctx,
>> + u16 size)
>> +{
>> + struct mipi_dsi_device *dsi = to_mipi_dsi_device(ctx->dev);
>> + int ret;
>> +
>> + ret = mipi_dsi_set_maximum_return_packet_size(dsi, size);
>> + if (ret < 0)
>> + dev_err(ctx->dev,
>> + "error %d setting maximum return packet size to %d\n",
>> + ret, size);
>> +}
>> +
>> +static int hx8369a_dsi_set_sequence(struct hx8369a *ctx)
>> +{
>> + struct mipi_dsi_device *dsi = to_mipi_dsi_device(ctx->dev);
>> + int ret;
>> +
>> + hx8369a_dsi_set_extension_command(ctx);
>> + hx8369a_dsi_set_maximum_return_packet_size(ctx, 4);
>> + hx8369a_dsi_panel_init(ctx);
>> +
>> + ret = mipi_dsi_dcs_exit_sleep_mode(dsi);
>> + if (ret < 0) {
>> + dev_err(ctx->dev, "failed to exit sleep mode: %d\n", ret);
>> + return ret;
>> + }
>> +
>> + ret = mipi_dsi_dcs_set_display_on(dsi);
>> + if (ret < 0) {
>> + dev_err(ctx->dev, "failed to set display on: %d\n", ret);
>> + return ret;
>> + }
>> +
>> + return 0;
>> +}
>> +
>
>> +static int hx8369a_dsi_disable(struct drm_panel *panel)
>> +{
>> + return 0;
>> +}
> [...]
>> +static int hx8369a_dsi_enable(struct drm_panel *panel)
>> +{
>> + return 0;
>> +}
>
> Doesn't this usually come with a backlight attached that you want to
> control here?
Accepted.
>
>> +static int hx8369a_get_modes(struct drm_panel *panel)
>> +{
>> + struct drm_connector *connector = panel->connector;
>> + struct hx8369a *ctx = panel_to_hx8369a(panel);
>> + struct drm_display_mode *mode;
>> +
>> + mode = drm_mode_create(connector->dev);
>> + if (!mode) {
>> + DRM_ERROR("failed to create a new display mode\n");
>> + return 0;
>> + }
>> +
>> + drm_display_mode_from_videomode(&ctx->vm, mode);
>
> Like I've said before, the driver should hardcode the mode because it is
> implied by the compatible value.
Accepted.
>
>> + mode->width_mm = ctx->width_mm;
>> + mode->height_mm = ctx->height_mm;
>> + connector->display_info.width_mm = mode->width_mm;
>> + connector->display_info.height_mm = mode->height_mm;
>> +
>> + mode->type = DRM_MODE_TYPE_DRIVER | DRM_MODE_TYPE_PREFERRED;
>> + drm_mode_probed_add(connector, mode);
>> +
>> + return 1;
>> +}
>> +
>> +static const struct drm_panel_funcs hx8369a_dsi_drm_funcs = {
>> + .disable = hx8369a_dsi_disable,
>> + .unprepare = hx8369a_dsi_unprepare,
>> + .prepare = hx8369a_dsi_prepare,
>> + .enable = hx8369a_dsi_enable,
>> + .get_modes = hx8369a_get_modes,
>> +};
>
>> +static int hx8369a_dsi_probe(struct mipi_dsi_device *dsi)
>> +{
>> + struct device *dev = &dsi->dev;
>> + struct hx8369a *ctx;
>> + int ret, i;
>> + char bs[4];
>> +
>> + ctx = devm_kzalloc(dev, sizeof(struct hx8369a), GFP_KERNEL);
>
> I'd prefer sizeof(*ctx).
Accepted.
>
>> + ctx->reset_gpio = devm_gpiod_get(dev, "reset");
>> + if (IS_ERR(ctx->reset_gpio)) {
>> + dev_err(dev, "cannot get reset-gpios %ld\n",
>> + PTR_ERR(ctx->reset_gpio));
>> + return PTR_ERR(ctx->reset_gpio);
>> + }
>> + ret = gpiod_direction_output(ctx->reset_gpio, 0);
>> + if (ret < 0) {
>> + dev_err(dev, "cannot configure reset-gpios %d\n", ret);
>> + return ret;
>> + }
>
> I think you're supposed to combine this into something like:
>
> ret = devm_gpiod_get(dev, "reset", GPIOD_OUT_LOW);
>
> nowadays.
Accepted.
>
>> +
>> + for (i = 0; i < 4; i++) {
>> + snprintf(bs, sizeof(bs), "bs%d", i);
>> + ctx->bs_gpio[i] = devm_gpiod_get(dev, bs);
>> + if (IS_ERR(ctx->bs_gpio[i]))
>> + continue;
>> +
>> + ret = gpiod_direction_output(ctx->bs_gpio[i], 1);
>> + if (ret < 0) {
>> + dev_err(dev, "cannot configure bs%d-gpio %d\n", i, ret);
>> + return ret;
>> + }
>
> Similarly to the above:
>
> ret = devm_gpiod_get(dev, bs, GPIOD_OUT_HIGH);
>
>> +static int __init hx8369a_init(void)
>> +{
>> + int err;
>> +
>> + err = mipi_dsi_driver_register(&hx8369a_dsi_driver);
>> + if (err < 0)
>> + return err;
>> +
>> + return 0;
>> +}
>> +module_init(hx8369a_init);
>> +
>> +static void __exit hx8369a_exit(void)
>> +{
>> + mipi_dsi_driver_unregister(&hx8369a_dsi_driver);
>> +}
>> +module_exit(hx8369a_exit);
>
> Why can't this be module_mipi_dsi_driver(&hx8369a_dsi_driver)?
Like the panel-simple driver. I thought that we probably may add other
driver registers here. But, since the driver only supports DSI now,
I may change to use module_mipi_dsi_driver().
>
>> +
>> +MODULE_DESCRIPTION("Himax HX8369A panel driver");
>> +MODULE_LICENSE("GPL v2");
>
> Missing MODULE_AUTHOR()?
I'll add it.
Thanks,
Liu Ying
>
> Thierry
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists