From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751305AbdBLN54 (ORCPT ); Sun, 12 Feb 2017 08:57:56 -0500 Received: from smtp.domeneshop.no ([194.63.252.55]:36270 "EHLO smtp.domeneshop.no" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751225AbdBLN5y (ORCPT ); Sun, 12 Feb 2017 08:57:54 -0500 Subject: Re: [PATCH v4 1/7] drm: Add DRM support for tiny LCD displays To: Andy Shevchenko References: <20170211184858.26421-1-noralf@tronnes.org> <20170211184858.26421-2-noralf@tronnes.org> Cc: dri-devel@lists.freedesktop.org, devicetree , robh@kernel.org, Thomas Petazzoni , "linux-kernel@vger.kernel.org" From: =?UTF-8?Q?Noralf_Tr=c3=b8nnes?= Message-ID: <70190e71-4d5a-7102-418f-09896b57c084@tronnes.org> Date: Sun, 12 Feb 2017 14:57:49 +0100 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:45.0) Gecko/20100101 Thunderbird/45.7.1 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Den 12.02.2017 12.05, skrev Andy Shevchenko: > On Sat, Feb 11, 2017 at 8:48 PM, Noralf Trønnes wrote: >> tinydrm provides helpers for very simple displays that can use >> CMA backed framebuffers and need flushing on changes. >> +/** >> + * tinydrm_gem_cma_free_object - Free resources associated with a CMA GEM >> + * object > Keep on one line? That means 81 columns and checkpatch complaint. This in one of the annoying things about Linux kernel coding. Always trying to avoid breaking up lines. Because for me it hinders redability. >> +const struct file_operations tinydrm_fops = { >> + .owner = THIS_MODULE, > Do we still need this? Looks like it, see drm_stub_open() >> +static int tinydrm_init(struct device *parent, struct tinydrm_device *tdev, >> + const struct drm_framebuffer_funcs *fb_funcs, >> + struct drm_driver *driver) >> +{ >> + struct drm_device *drm; >> + >> + mutex_init(&tdev->dirty_lock); >> + tdev->fb_funcs = fb_funcs; >> + >> + /* >> + * We don't embed drm_device, because that prevent us from using >> + * devm_kzalloc() to allocate tinydrm_device in the driver since >> + * drm_dev_unref() frees the structure. The devm_ functions provide > "devm_ functions" -> "devm_kzalloc()" ? > > Otherwise what else do you refer to and why here? yes, that last sentence can be dropped. >> + * for easy error handling. >> + */ >> +static int tinydrm_register(struct tinydrm_device *tdev) >> +{ >> + struct drm_device *drm = tdev->drm; >> + int bpp = drm->mode_config.preferred_depth; >> + struct drm_fbdev_cma *fbdev; >> + int ret; >> + >> + ret = drm_dev_register(tdev->drm, 0); >> + if (ret) >> + return ret; >> + >> + fbdev = drm_fbdev_cma_init_with_funcs(drm, bpp ? bpp : 32, >> + drm->mode_config.num_connector, >> + tdev->fb_funcs); >> + if (IS_ERR(fbdev)) >> + DRM_ERROR("Failed to initialize fbdev: %ld\n", PTR_ERR(fbdev)); >> + else >> + tdev->fbdev_cma = fbdev; > Apparently I missed previous round of reviews, can you just put in > short words why error is not propagated here to the caller? A comment might have helped here: /* fbdev is of minor importance, don't let it stop us */ >> + >> + return 0; >> +} >> +/** >> + * tinydrm_display_pipe_init - Initialize display pipe >> + * @tdev: tinydrm device >> + * @funcs: Display pipe functions >> + * @connector_type: Connector type >> + * @formats: Array of supported formats (DRM_FORMAT\_\*) > DRM_FORMAT_* ? sphinx wants it that way. >> + * @format_count: Number of elements in @formats >> + * @mode: Supported mode >> + * @rotation: Initial @mode rotation in degrees Counter Clock Wise >> + * >> + * This function sets up a &drm_simple_display_pipe with a &drm_connector that >> + * has one fixed &drm_display_mode which is rotated according to @rotation. >> + * >> + * Returns: >> + * Zero on success, negative error code on failure. >> + */ >> +{ >> + struct drm_device *drm = tdev->drm; >> + struct drm_display_mode *mode_copy; >> + struct drm_connector *connector; >> + int ret; >> + connector = tinydrm_connector_create(drm, mode_copy, connector_type); >> + if (IS_ERR(connector)) >> + return PTR_ERR(connector); >> + >> + ret = drm_simple_display_pipe_init(drm, &tdev->pipe, funcs, formats, >> + format_count, connector); >> + if (ret) >> + return ret; >> + >> + return 0; > return drm_... ? Yep. tinydrm will be merged the way it is now, unless someone points to something that is broken. But I collect your comments for a later cleanup patchset. It takes a lot of effort for me as an amateur to keeps this code up-to-date out-of-tree for months. It's not even sure that I've hit the mark with this, so there will most likely be changes when I start converting fbtft drivers, and I'll implement the remaining bits and pieces as I make changes. The core of tinydrm won't change because of unforseen fbtft quirks, but other parts might. Noralf. >> +} >> +EXPORT_SYMBOL(tinydrm_display_pipe_init); From mboxrd@z Thu Jan 1 00:00:00 1970 From: =?UTF-8?Q?Noralf_Tr=c3=b8nnes?= Subject: Re: [PATCH v4 1/7] drm: Add DRM support for tiny LCD displays Date: Sun, 12 Feb 2017 14:57:49 +0100 Message-ID: <70190e71-4d5a-7102-418f-09896b57c084@tronnes.org> References: <20170211184858.26421-1-noralf@tronnes.org> <20170211184858.26421-2-noralf@tronnes.org> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8"; Format="flowed" Content-Transfer-Encoding: base64 Return-path: In-Reply-To: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" To: Andy Shevchenko Cc: Thomas Petazzoni , devicetree , "linux-kernel@vger.kernel.org" , dri-devel@lists.freedesktop.org List-Id: devicetree@vger.kernel.org CkRlbiAxMi4wMi4yMDE3IDEyLjA1LCBza3JldiBBbmR5IFNoZXZjaGVua286Cj4gT24gU2F0LCBG ZWIgMTEsIDIwMTcgYXQgODo0OCBQTSwgTm9yYWxmIFRyw7hubmVzIDxub3JhbGZAdHJvbm5lcy5v cmc+IHdyb3RlOgo+PiB0aW55ZHJtIHByb3ZpZGVzIGhlbHBlcnMgZm9yIHZlcnkgc2ltcGxlIGRp c3BsYXlzIHRoYXQgY2FuIHVzZQo+PiBDTUEgYmFja2VkIGZyYW1lYnVmZmVycyBhbmQgbmVlZCBm bHVzaGluZyBvbiBjaGFuZ2VzLgo+PiArLyoqCj4+ICsgKiB0aW55ZHJtX2dlbV9jbWFfZnJlZV9v YmplY3QgLSBGcmVlIHJlc291cmNlcyBhc3NvY2lhdGVkIHdpdGggYSBDTUEgR0VNCj4+ICsgKiAg ICAgICAgICAgICAgICAgICAgICAgICAgICAgICBvYmplY3QKPiBLZWVwIG9uIG9uZSBsaW5lPwoK VGhhdCBtZWFucyA4MSBjb2x1bW5zIGFuZCBjaGVja3BhdGNoIGNvbXBsYWludC4KVGhpcyBpbiBv bmUgb2YgdGhlIGFubm95aW5nIHRoaW5ncyBhYm91dCBMaW51eAprZXJuZWwgY29kaW5nLiBBbHdh eXMgdHJ5aW5nCnRvIGF2b2lkCmJyZWFraW5nIHVwIGxpbmVzLgpCZWNhdXNlIGZvciBtZSBpdCBo aW5kZXJzIHJlZGFiaWxpdHkuCgo+PiArY29uc3Qgc3RydWN0IGZpbGVfb3BlcmF0aW9ucyB0aW55 ZHJtX2ZvcHMgPSB7Cj4+ICsgICAgICAgLm93bmVyICAgICAgICAgID0gVEhJU19NT0RVTEUsCj4g RG8gd2Ugc3RpbGwgbmVlZCB0aGlzPwoKTG9va3MgbGlrZSBpdCwgc2VlIGRybV9zdHViX29wZW4o KQoKPj4gK3N0YXRpYyBpbnQgdGlueWRybV9pbml0KHN0cnVjdCBkZXZpY2UgKnBhcmVudCwgc3Ry dWN0IHRpbnlkcm1fZGV2aWNlICp0ZGV2LAo+PiArICAgICAgICAgICAgICAgICAgICAgICBjb25z dCBzdHJ1Y3QgZHJtX2ZyYW1lYnVmZmVyX2Z1bmNzICpmYl9mdW5jcywKPj4gKyAgICAgICAgICAg ICAgICAgICAgICAgc3RydWN0IGRybV9kcml2ZXIgKmRyaXZlcikKPj4gK3sKPj4gKyAgICAgICBz dHJ1Y3QgZHJtX2RldmljZSAqZHJtOwo+PiArCj4+ICsgICAgICAgbXV0ZXhfaW5pdCgmdGRldi0+ ZGlydHlfbG9jayk7Cj4+ICsgICAgICAgdGRldi0+ZmJfZnVuY3MgPSBmYl9mdW5jczsKPj4gKwo+ PiArICAgICAgIC8qCj4+ICsgICAgICAgICogV2UgZG9uJ3QgZW1iZWQgZHJtX2RldmljZSwgYmVj YXVzZSB0aGF0IHByZXZlbnQgdXMgZnJvbSB1c2luZwo+PiArICAgICAgICAqIGRldm1fa3phbGxv YygpIHRvIGFsbG9jYXRlIHRpbnlkcm1fZGV2aWNlIGluIHRoZSBkcml2ZXIgc2luY2UKPj4gKyAg ICAgICAgKiBkcm1fZGV2X3VucmVmKCkgZnJlZXMgdGhlIHN0cnVjdHVyZS4gVGhlIGRldm1fIGZ1 bmN0aW9ucyBwcm92aWRlCj4gImRldm1fIGZ1bmN0aW9ucyIgLT4gImRldm1fa3phbGxvYygpIiA/ Cj4KPiBPdGhlcndpc2Ugd2hhdCBlbHNlIGRvIHlvdSByZWZlciB0byBhbmQgd2h5IGhlcmU/Cgp5 ZXMsIHRoYXQgbGFzdCBzZW50ZW5jZSBjYW4gYmUgZHJvcHBlZC4KCj4+ICsgICAgICAgICogZm9y IGVhc3kgZXJyb3IgaGFuZGxpbmcuCj4+ICsgICAgICAgICovCj4+ICtzdGF0aWMgaW50IHRpbnlk cm1fcmVnaXN0ZXIoc3RydWN0IHRpbnlkcm1fZGV2aWNlICp0ZGV2KQo+PiArewo+PiArICAgICAg IHN0cnVjdCBkcm1fZGV2aWNlICpkcm0gPSB0ZGV2LT5kcm07Cj4+ICsgICAgICAgaW50IGJwcCA9 IGRybS0+bW9kZV9jb25maWcucHJlZmVycmVkX2RlcHRoOwo+PiArICAgICAgIHN0cnVjdCBkcm1f ZmJkZXZfY21hICpmYmRldjsKPj4gKyAgICAgICBpbnQgcmV0Owo+PiArCj4+ICsgICAgICAgcmV0 ID0gZHJtX2Rldl9yZWdpc3Rlcih0ZGV2LT5kcm0sIDApOwo+PiArICAgICAgIGlmIChyZXQpCj4+ ICsgICAgICAgICAgICAgICByZXR1cm4gcmV0Owo+PiArCj4+ICsgICAgICAgZmJkZXYgPSBkcm1f ZmJkZXZfY21hX2luaXRfd2l0aF9mdW5jcyhkcm0sIGJwcCA/IGJwcCA6IDMyLAo+PiArICAgICAg ICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgZHJtLT5tb2RlX2NvbmZpZy5u dW1fY29ubmVjdG9yLAo+PiArICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAg ICAgICAgdGRldi0+ZmJfZnVuY3MpOwo+PiArICAgICAgIGlmIChJU19FUlIoZmJkZXYpKQo+PiAr ICAgICAgICAgICAgICAgRFJNX0VSUk9SKCJGYWlsZWQgdG8gaW5pdGlhbGl6ZSBmYmRldjogJWxk XG4iLCBQVFJfRVJSKGZiZGV2KSk7Cj4+ICsgICAgICAgZWxzZQo+PiArICAgICAgICAgICAgICAg dGRldi0+ZmJkZXZfY21hID0gZmJkZXY7Cj4gQXBwYXJlbnRseSBJIG1pc3NlZCBwcmV2aW91cyBy b3VuZCBvZiByZXZpZXdzLCBjYW4geW91IGp1c3QgcHV0IGluCj4gc2hvcnQgd29yZHMgd2h5IGVy cm9yIGlzIG5vdCBwcm9wYWdhdGVkIGhlcmUgdG8gdGhlIGNhbGxlcj8KCkEgY29tbWVudCBtaWdo dCBoYXZlIGhlbHBlZCBoZXJlOgovKiBmYmRldiBpcyBvZiBtaW5vciBpbXBvcnRhbmNlLCBkb24n dCBsZXQgaXQgc3RvcCB1cyAqLwoKPj4gKwo+PiArICAgICAgIHJldHVybiAwOwo+PiArfQo+PiAr LyoqCj4+ICsgKiB0aW55ZHJtX2Rpc3BsYXlfcGlwZV9pbml0IC0gSW5pdGlhbGl6ZSBkaXNwbGF5 IHBpcGUKPj4gKyAqIEB0ZGV2OiB0aW55ZHJtIGRldmljZQo+PiArICogQGZ1bmNzOiBEaXNwbGF5 IHBpcGUgZnVuY3Rpb25zCj4+ICsgKiBAY29ubmVjdG9yX3R5cGU6IENvbm5lY3RvciB0eXBlCj4+ ICsgKiBAZm9ybWF0czogQXJyYXkgb2Ygc3VwcG9ydGVkIGZvcm1hdHMgKERSTV9GT1JNQVRcX1wq KQo+IERSTV9GT1JNQVRfKiA/CgpzcGhpbnggd2FudHMgaXQgdGhhdCB3YXkuCgo+PiArICogQGZv cm1hdF9jb3VudDogTnVtYmVyIG9mIGVsZW1lbnRzIGluIEBmb3JtYXRzCj4+ICsgKiBAbW9kZTog U3VwcG9ydGVkIG1vZGUKPj4gKyAqIEByb3RhdGlvbjogSW5pdGlhbCBAbW9kZSByb3RhdGlvbiBp biBkZWdyZWVzIENvdW50ZXIgQ2xvY2sgV2lzZQo+PiArICoKPj4gKyAqIFRoaXMgZnVuY3Rpb24g c2V0cyB1cCBhICZkcm1fc2ltcGxlX2Rpc3BsYXlfcGlwZSB3aXRoIGEgJmRybV9jb25uZWN0b3Ig dGhhdAo+PiArICogaGFzIG9uZSBmaXhlZCAmZHJtX2Rpc3BsYXlfbW9kZSB3aGljaCBpcyByb3Rh dGVkIGFjY29yZGluZyB0byBAcm90YXRpb24uCj4+ICsgKgo+PiArICogUmV0dXJuczoKPj4gKyAq IFplcm8gb24gc3VjY2VzcywgbmVnYXRpdmUgZXJyb3IgY29kZSBvbiBmYWlsdXJlLgo+PiArICov Cj4+ICt7Cj4+ICsgICAgICAgc3RydWN0IGRybV9kZXZpY2UgKmRybSA9IHRkZXYtPmRybTsKPj4g KyAgICAgICBzdHJ1Y3QgZHJtX2Rpc3BsYXlfbW9kZSAqbW9kZV9jb3B5Owo+PiArICAgICAgIHN0 cnVjdCBkcm1fY29ubmVjdG9yICpjb25uZWN0b3I7Cj4+ICsgICAgICAgaW50IHJldDsKPj4gKyAg ICAgICBjb25uZWN0b3IgPSB0aW55ZHJtX2Nvbm5lY3Rvcl9jcmVhdGUoZHJtLCBtb2RlX2NvcHks IGNvbm5lY3Rvcl90eXBlKTsKPj4gKyAgICAgICBpZiAoSVNfRVJSKGNvbm5lY3RvcikpCj4+ICsg ICAgICAgICAgICAgICByZXR1cm4gUFRSX0VSUihjb25uZWN0b3IpOwo+PiArCj4+ICsgICAgICAg cmV0ID0gZHJtX3NpbXBsZV9kaXNwbGF5X3BpcGVfaW5pdChkcm0sICZ0ZGV2LT5waXBlLCBmdW5j cywgZm9ybWF0cywKPj4gKyAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAg IGZvcm1hdF9jb3VudCwgY29ubmVjdG9yKTsKPj4gKyAgICAgICBpZiAocmV0KQo+PiArICAgICAg ICAgICAgICAgcmV0dXJuIHJldDsKPj4gKwo+PiArICAgICAgIHJldHVybiAwOwo+IHJldHVybiBk cm1fLi4uID8KClllcC4KCgp0aW55ZHJtIHdpbGwgYmUgbWVyZ2VkIHRoZSB3YXkgaXQgaXMgbm93 LCB1bmxlc3Mgc29tZW9uZSBwb2ludHMgdG8Kc29tZXRoaW5nIHRoYXQgaXMgYnJva2VuLiBCdXQg SSBjb2xsZWN0IHlvdXIgY29tbWVudHMgZm9yIGEgbGF0ZXIKY2xlYW51cCBwYXRjaHNldC4gSXQg dGFrZXMgYSBsb3Qgb2YgZWZmb3J0IGZvciBtZSBhcyBhbiBhbWF0ZXVyIHRvCmtlZXBzIHRoaXMg Y29kZSB1cC10by1kYXRlIG91dC1vZi10cmVlIGZvciBtb250aHMuIEl0J3Mgbm90IGV2ZW4Kc3Vy ZSB0aGF0IEkndmUgaGl0IHRoZSBtYXJrIHdpdGggdGhpcywgc28gdGhlcmUgd2lsbCBtb3N0IGxp a2VseSBiZQpjaGFuZ2VzIHdoZW4gSSBzdGFydCBjb252ZXJ0aW5nIGZidGZ0IGRyaXZlcnMsIGFu ZCBJJ2xsIGltcGxlbWVudCB0aGUKcmVtYWluaW5nIGJpdHMgYW5kIHBpZWNlcyBhcyBJIG1ha2Ug Y2hhbmdlcy4gVGhlIGNvcmUgb2YgdGlueWRybSB3b24ndApjaGFuZ2UgYmVjYXVzZSBvZiB1bmZv cnNlZW4gZmJ0ZnQgcXVpcmtzLCBidXQgb3RoZXIgcGFydHMgbWlnaHQuCgoKTm9yYWxmLgoKPj4g K30KPj4gK0VYUE9SVF9TWU1CT0wodGlueWRybV9kaXNwbGF5X3BpcGVfaW5pdCk7CgpfX19fX19f X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fXwpkcmktZGV2ZWwgbWFpbGlu ZyBsaXN0CmRyaS1kZXZlbEBsaXN0cy5mcmVlZGVza3RvcC5vcmcKaHR0cHM6Ly9saXN0cy5mcmVl ZGVza3RvcC5vcmcvbWFpbG1hbi9saXN0aW5mby9kcmktZGV2ZWwK