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 at freescale.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 at 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 >