From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <linux-kernel-owner@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
        id S1752082AbdBFMef (ORCPT <rfc822;w@1wt.eu>);
        Mon, 6 Feb 2017 07:34:35 -0500
Received: from mailout3.w1.samsung.com ([210.118.77.13]:18284 "EHLO
        mailout3.w1.samsung.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
        with ESMTP id S1751468AbdBFMec (ORCPT
        <rfc822;linux-kernel@vger.kernel.org>);
        Mon, 6 Feb 2017 07:34:32 -0500
MIME-version: 1.0
Content-type: text/plain; charset=utf-8
X-AuditID: cbfec7f1-f793f6d000007796-0f-58986d545c6b
Subject: Re: [PATCH v3 3/7] drm/tinydrm: Add MIPI DBI support
To: Thierry Reding <thierry.reding@gmail.com>,
        Jani Nikula <jani.nikula@linux.intel.com>
Cc: thomas.petazzoni@free-electrons.com, devicetree@vger.kernel.org,
        linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org
From: Andrzej Hajda <a.hajda@samsung.com>
Message-id: <6fe746a0-07dd-5d45-fa90-f4c092178ac1@samsung.com>
Date: Mon, 06 Feb 2017 13:34:26 +0100
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101
        Thunderbird/45.7.0
In-reply-to: <20170206115347.GJ27607@ulmo.ba.sec>
Content-transfer-encoding: 8bit
X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFupmleLIzCtJLcpLzFFi42LZduznOd2Q3BkRBl828lvMP3KO1eLK1/ds
        Fssvr2e0uLxrDpvFz13zWCyePmhicmDzeLLpIqPHzll32T3mnQz0uN99nMnj8ya5ANYoLpuU
        1JzMstQifbsEroyVcxcwFzzVrLj5j7eBcYNSFyMnh4SAicSTv6+ZIWwxiQv31rN1MXJxCAks
        ZZR4evkmM4TzmVGi5/0iFpiOnhNTWCASyxglzm0/B5bgFRCU+DH5HpjNLKAp8eLLJKiiZ4wS
        bfeXsoMkhAVsJa7dOckIYosIREk8OwVRxCzQwiixoPMhE0iCDaj77+abbBBT7STWXrwOFOfg
        YBFQlbi+2g8kLCoQIbHjRg9YCaeAocQKsDkgi+UlDl55DjZTQmAeu0T3ibNsIL0SArISmw5A
        /ekiMav5DxOELSzx6vgWdghbRqKz4yATRG83o8Sn/hPsEM4URol/H2ZAdVtLHD5+kRViG5/E
        pG3TmSEW8Ep0tAlBlHhIvL4xCWqBo8TPNe/BDhUSWMAksW+R4wRG+VlIATYLKcBmIflhASPz
        KkaR1NLi3PTUYiO94sTc4tK8dL3k/NxNjMAkcvrf8Y87GN+fsDrEKMDBqMTD+0B/eoQQa2JZ
        cWXuIUYJDmYlEV6LjBkRQrwpiZVVqUX58UWlOanFhxilOViUxHn3LLgSLiSQnliSmp2aWpBa
        BJNl4uCUamCUCL1wWIX3VairyF+NyP/X4z5eTVdyFmA9ciDn94/fIgJXjD88b+LibevjD2S2
        d91w8tELpcrvIY6TT8zNbXxy8dQzkcdq2o/WfLFzfq+/8kPkg60PFpTcSJk8QeCnxl/Gj1wG
        gcvFP/KfiTjw4THT6b/dnJYL+o5q/ekp3CPKbHL/1hbd2qlrlFiKMxINtZiLihMBpL7DBh4D        AAA=
X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFlrOIsWRmVeSWpSXmKPExsVy+t/xy7pzcmdEGOyaomYx/8g5VosrX9+z
        WSy/vJ7R4vKuOWwWP3fNY7F4+qCJyYHN48mmi4weO2fdZfeYdzLQ4373cSaPz5vkAlij3Gwy
        UhNTUosUUvOS81My89JtlUJD3HQtlBTyEnNTbZUidH1DgpQUyhJzSoE8IwM04OAc4B6spG+X
        4Jaxcu4C5oKnmhU3//E2MG5Q6mLk5JAQMJHoOTGFBcIWk7hwbz1bFyMXh5DAEkaJU1emMIMk
        eAUEJX5MvgdUxMHBLKAuMWVKLkTNM0aJnau3soPUCAvYSly7c5IRxBYRiJLoOXKbEaJoAZPE
        mVWHGCGaWxglbiqC1LAJaEr83XyTDWK+ncTai9eZQEpYBFQlrq/2AwmLCkRIzH+6ignE5hQw
        lFhxahLYncwC8hIHrzxnmcAoMAvJdbMQrpuFpGoBI/MqRpHU0uLc9NxiQ73ixNzi0rx0veT8
        3E2MwEjaduzn5h2MlzYGH2IU4GBU4uF9oD89Qog1say4MvcQowQHs5IIr0XGjAgh3pTEyqrU
        ovz4otKc1OJDjKZAp05klhJNzgdGeV5JvKGJobmloZGxhYW5kZGSOG/JhyvhQgLpiSWp2amp
        BalFMH1MHJxSDYw9fzlVJWwYTnTOEV/JrxfkXPpcVM5nXaFq0WXB0/Jin3kLChj0pY4cXqqo
        LOddmaz/tsf8yq94jb+N/M4/Exj5L5zSuXBIdu/cU9NazIMMtjh0TDpaFVtxvp+t1sfx5o0L
        ejqvV999m76kqt97u7iV2fING2bfdr0YMoNbuO7GHN0fuoaJikosxRmJhlrMRcWJAIN1A4u6        AgAA
X-MTR: 20000000000000000@CPGS
X-CMS-MailID: 20170206123427eucas1p1df48c05b50eb7b0a814ccff38ec91de3
X-Msg-Generator: CA
X-Sender-IP: 182.198.249.179
X-Local-Sender: =?UTF-8?B?QW5kcnplaiBIYWpkYRtTUlBPTC1LZXJuZWwgKFRQKRvsgrw=?=
        =?UTF-8?B?7ISx7KCE7J6QG1NlbmlvciBTb2Z0d2FyZSBFbmdpbmVlcg==?=
X-Global-Sender: =?UTF-8?B?QW5kcnplaiBIYWpkYRtTUlBPTC1LZXJuZWwgKFRQKRtTYW1z?=
        =?UTF-8?B?dW5nIEVsZWN0cm9uaWNzG1NlbmlvciBTb2Z0d2FyZSBFbmdpbmVlcg==?=
X-Sender-Code: =?UTF-8?B?QzEwG0VIURtDMTBDRDAyQ0QwMjczOTI=?=
CMS-TYPE: 201P
X-HopCount: 7
X-CMS-RootMailID: 20170206115443epcas4p10e867df4180c52e15a10b7675ae7bbd8
X-RootMTR: 20170206115443epcas4p10e867df4180c52e15a10b7675ae7bbd8
References: <20170131160319.9695-1-noralf@tronnes.org>
 <20170131160319.9695-4-noralf@tronnes.org>
 <20170206084848.GC27607@ulmo.ba.sec> <87efzbblq6.fsf@intel.com>
 <CGME20170206115443epcas4p10e867df4180c52e15a10b7675ae7bbd8@epcas4p1.samsung.com>
 <20170206115347.GJ27607@ulmo.ba.sec>
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On 06.02.2017 12:53, Thierry Reding wrote:
> On Mon, Feb 06, 2017 at 01:30:09PM +0200, Jani Nikula wrote:
>> On Mon, 06 Feb 2017, Thierry Reding <thierry.reding@gmail.com> wrote:
>>> On Tue, Jan 31, 2017 at 05:03:15PM +0100, Noralf Trønnes wrote:
>>>> Add support for MIPI DBI compatible controllers.
>>>> Interface type C option 1 and 3 are supported (SPI).
>>>>
>>>> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
>>>> ---
>>>>  Documentation/gpu/tinydrm.rst      |   12 +
>>>>  drivers/gpu/drm/tinydrm/Kconfig    |    3 +
>>>>  drivers/gpu/drm/tinydrm/Makefile   |    3 +
>>>>  drivers/gpu/drm/tinydrm/mipi-dbi.c | 1005 ++++++++++++++++++++++++++++++++++++
>>>>  include/drm/tinydrm/mipi-dbi.h     |  107 ++++
>>>>  5 files changed, 1130 insertions(+)
>>>>  create mode 100644 drivers/gpu/drm/tinydrm/mipi-dbi.c
>>>>  create mode 100644 include/drm/tinydrm/mipi-dbi.h
>>> Any reason why this is in the tinydrm subdirectory? Looks like this
>>> could be useful to drivers outside of it.
>>>
>>>> diff --git a/drivers/gpu/drm/tinydrm/mipi-dbi.c b/drivers/gpu/drm/tinydrm/mipi-dbi.c
>>>> new file mode 100644
>>>> index 0000000..5ded299
>>>> --- /dev/null
>>>> +++ b/drivers/gpu/drm/tinydrm/mipi-dbi.c
>>>> @@ -0,0 +1,1005 @@
>>>> +/*
>>>> + * MIPI Display Bus Interface (DBI) LCD controller support
>>>> + *
>>>> + * Copyright 2016 Noralf Trønnes
>>>> + *
>>>> + * 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.
>>>> + */
>>>> +
>>>> +#include <drm/tinydrm/mipi-dbi.h>
>>>> +#include <drm/tinydrm/tinydrm-helpers.h>
>>>> +#include <linux/debugfs.h>
>>>> +#include <linux/dma-buf.h>
>>>> +#include <linux/gpio/consumer.h>
>>>> +#include <linux/module.h>
>>>> +#include <linux/regulator/consumer.h>
>>>> +#include <linux/spi/spi.h>
>>>> +#include <video/mipi_display.h>
>>>> +
>>>> +#define MIPI_DBI_MAX_SPI_READ_SPEED 2000000 /* 2MHz */
>>>> +
>>>> +#define DCS_POWER_MODE_DISPLAY			BIT(2)
>>>> +#define DCS_POWER_MODE_DISPLAY_NORMAL_MODE	BIT(3)
>>>> +#define DCS_POWER_MODE_SLEEP_MODE		BIT(4)
>>>> +#define DCS_POWER_MODE_PARTIAL_MODE		BIT(5)
>>>> +#define DCS_POWER_MODE_IDLE_MODE		BIT(6)
>>>> +#define DCS_POWER_MODE_RESERVED_MASK		(BIT(0) | BIT(1) | BIT(7))
>>> Should these perhaps be defined in include/video/mipi_display.h since
>>> that already defines the MIPI_DCS_GET_POWER_MODE that this is used with?
>> Agreed.
>>
>>>> +/**
>>>> + * mipi_dbi_command - MIPI DCS command with optional parameter(s)
>>>> + * @mipi: MIPI structure
>>>> + * @cmd: Command
>>>> + * @seq...: Optional parameter(s)
>>>> + *
>>>> + * Send MIPI DCS command to the controller. Use mipi_dbi_command_read() for
>>>> + * get/read.
>>>> + *
>>>> + * Returns:
>>>> + * Zero on success, negative error code on failure.
>>>> + */
>>>> +#define mipi_dbi_command(mipi, cmd, seq...) \
>>>> +({ \
>>>> +	u8 d[] = { seq }; \
>>>> +	mipi_dbi_command_buf(mipi, cmd, d, ARRAY_SIZE(d)); \
>>>> +})
>>> I feel obligated to object to this because I objected to the same macro
>>> when Andrzej wanted to add the same macro for MIPI DSI. But since I'm
>>> apparently the only one that doesn't like this, maybe it's time for me
>>> to embrace this.
>> I think that's less interesting for MIPI DSI now that we have specific
>> functions for most standard DCS commands anyway. That also avoids the
>> need to have a list of read functions like here.
>>
>> I guess the question is if it feels like too much duplication to have
>> the DCS commands as functions also for DBI. And whether it's worth
>> trying to deduplicate DSI and DBI in this case, or if it gets just too
>> confusing/complicated.
> The problem with this is that we can't possible have all accesses
> covered by specific functions. Most of the calls using this function or
> macro are for panel-specific commands that often take an arbitrary
> number of arguments.
>
> I personally think that Maxime's implementation for ST7789V is somewhat
> nicer, though it would require some more work for DSI and DBI to have it
> all stashed into a single buffer for the transfer.
>
> Anyway, my main concern with these is that we need to allow for proper
> error handling, and the above correctly propagates error codes, so the
> matter that it's a macro with a variable argument list and might store
> potentially huge arrays on the stack could be addressed when they
> become really problematic, rather than just annoying me because they
> aren't pretty.

You can always add compile time check for sequence length to the macro
[1]. Other thing is that sequence is often constant, in such case, to
save few bits of space, it can be stored in 'static const' array, but
for this array passed to mipi_dbi_command_buf should be constant. And of
course it requires another macro.

[1]:
http://lxr.free-electrons.com/source/drivers/gpu/drm/panel/panel-samsung-s6e8aa0.c?v=4.4#L169

Regards
Andrzej

>
> Thierry
>
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel