From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756369AbcHDLbM (ORCPT ); Thu, 4 Aug 2016 07:31:12 -0400 Received: from mail-it0-f66.google.com ([209.85.214.66]:34525 "EHLO mail-it0-f66.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754055AbcHDLbJ (ORCPT ); Thu, 4 Aug 2016 07:31:09 -0400 MIME-Version: 1.0 In-Reply-To: References: <1464861445-11086-1-git-send-email-jitao.shi@mediatek.com> <1464861445-11086-2-git-send-email-jitao.shi@mediatek.com> From: Enric Balletbo Serra Date: Thu, 4 Aug 2016 12:35:59 +0200 Message-ID: Subject: Re: [PATCH 2/2 v16] drm/bridge: Add I2C based driver for ps8640 bridge To: Daniel Vetter Cc: Daniel Kurtz , Mark Rutland , stonea168@163.com, ML dri-devel , Ajay Kumar , Vincent Palatin , cawa cheng , Russell King , Thierry Reding , devicetree , Jitao Shi , Pawel Moll , Ian Campbell , Rob Herring , "moderated list:ARM/Mediatek SoC support" , Yingjoe Chen , Matthias Brugger , =?UTF-8?B?RWRkaWUgSHVhbmcgKOm7g+aZuuWCkSk=?= , LAKML , Rahul Sharma , srv_heupstream , Emil Velikov , "Linux-Kernel@Vger. Kernel. Org" , Sascha Hauer , Kumar Gala , Andy Yan Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 2016-07-12 12:13 GMT+02:00 Daniel Vetter : > On Wed, Jun 29, 2016 at 6:31 AM, Daniel Kurtz wrote: >> On Fri, Jun 17, 2016 at 3:14 AM, Emil Velikov wrote: >>>> +static ssize_t ps8640_update_fw_store(struct device *dev, >>>> + struct device_attribute *attr, >>>> + const char *buf, size_t count) >>>> +{ >>>> + struct i2c_client *client = to_i2c_client(dev); >>>> + struct ps8640 *ps_bridge = i2c_get_clientdata(client); >>>> + const struct firmware *fw; >>>> + int error; >>>> + >>>> + error = request_firmware(&fw, PS_FW_NAME, dev); >>> Can the device operate without a firmware ? If not, why is the >>> firmware loaded so later/after user interaction (via sysfs) ? I don't >>> recall any other driver in DRM to use such an approach. >> >> The PS8640 has internal flash, so it should always already have a >> working firmware. >> This sysfs interface is useful for user space initiated field firmware updates. > > Might be better to just do a request_firmware on driver load, and > simply proceed if it's not there. Adding a sysfs interface (which is > abi) seems way too much overkill for this imo. If you want to upgrade > the firmware you can then just drop it into the right directory, with > no further interaction needed. IMHO I'm not sure if for this use case request_firmware on driver load is a good idea. Flash the non-volatile internal chip can be a slow operation and if you forget to remove the firmware after drop it into the right directory apart from slow down the driver probe you can damage the chip depending on the write endurance of the chip. This sysfs interface is used on other subsystems when there is a non-volatile memory, as example you can see at [1], [2]. Unfortunately not all are using the same sysfs interface and maybe this should be standardized (maybe it's an opportunity to define it) Regards, Enric [1] http://lxr.free-electrons.com/source/drivers/input/touchscreen/wdt87xx_i2c.c#L922 [2] http://lxr.free-electrons.com/source/drivers/scsi/pm8001/pm8001_ctl.c#L732 > -Daniel > -- > Daniel Vetter > Software Engineer, Intel Corporation > +41 (0) 79 365 57 48 - http://blog.ffwll.ch > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel From mboxrd@z Thu Jan 1 00:00:00 1970 From: Enric Balletbo Serra Subject: Re: [PATCH 2/2 v16] drm/bridge: Add I2C based driver for ps8640 bridge Date: Thu, 4 Aug 2016 12:35:59 +0200 Message-ID: References: <1464861445-11086-1-git-send-email-jitao.shi@mediatek.com> <1464861445-11086-2-git-send-email-jitao.shi@mediatek.com> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8" 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: Daniel Vetter Cc: Mark Rutland , stonea168@163.com, ML dri-devel , Ajay Kumar , Vincent Palatin , cawa cheng , Yingjoe Chen , Thierry Reding , devicetree , Jitao Shi , Pawel Moll , Ian Campbell , Rob Herring , "moderated list:ARM/Mediatek SoC support" , Russell King , Matthias Brugger , =?UTF-8?B?RWRkaWUgSHVhbmcgKOm7g+aZuuWCkSk=?= , LAKML , Rahul Sharma , srv_heupstream , Emil Velikov List-Id: devicetree@vger.kernel.org MjAxNi0wNy0xMiAxMjoxMyBHTVQrMDI6MDAgRGFuaWVsIFZldHRlciA8ZGFuaWVsQGZmd2xsLmNo PjoKPiBPbiBXZWQsIEp1biAyOSwgMjAxNiBhdCA2OjMxIEFNLCBEYW5pZWwgS3VydHogPGRqa3Vy dHpAY2hyb21pdW0ub3JnPiB3cm90ZToKPj4gT24gRnJpLCBKdW4gMTcsIDIwMTYgYXQgMzoxNCBB TSwgRW1pbCBWZWxpa292IDxlbWlsLmwudmVsaWtvdkBnbWFpbC5jb20+IHdyb3RlOgo+Pj4+ICtz dGF0aWMgc3NpemVfdCBwczg2NDBfdXBkYXRlX2Z3X3N0b3JlKHN0cnVjdCBkZXZpY2UgKmRldiwK Pj4+PiArICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgIHN0cnVjdCBkZXZpY2Vf YXR0cmlidXRlICphdHRyLAo+Pj4+ICsgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAg ICAgY29uc3QgY2hhciAqYnVmLCBzaXplX3QgY291bnQpCj4+Pj4gK3sKPj4+PiArICAgICAgIHN0 cnVjdCBpMmNfY2xpZW50ICpjbGllbnQgPSB0b19pMmNfY2xpZW50KGRldik7Cj4+Pj4gKyAgICAg ICBzdHJ1Y3QgcHM4NjQwICpwc19icmlkZ2UgPSBpMmNfZ2V0X2NsaWVudGRhdGEoY2xpZW50KTsK Pj4+PiArICAgICAgIGNvbnN0IHN0cnVjdCBmaXJtd2FyZSAqZnc7Cj4+Pj4gKyAgICAgICBpbnQg ZXJyb3I7Cj4+Pj4gKwo+Pj4+ICsgICAgICAgZXJyb3IgPSByZXF1ZXN0X2Zpcm13YXJlKCZmdywg UFNfRldfTkFNRSwgZGV2KTsKPj4+IENhbiB0aGUgZGV2aWNlIG9wZXJhdGUgd2l0aG91dCBhIGZp cm13YXJlID8gSWYgbm90LCB3aHkgaXMgdGhlCj4+PiBmaXJtd2FyZSBsb2FkZWQgc28gbGF0ZXIv YWZ0ZXIgdXNlciBpbnRlcmFjdGlvbiAodmlhIHN5c2ZzKSA/IEkgZG9uJ3QKPj4+IHJlY2FsbCBh bnkgb3RoZXIgZHJpdmVyIGluIERSTSB0byB1c2Ugc3VjaCBhbiBhcHByb2FjaC4KPj4KPj4gVGhl IFBTODY0MCBoYXMgaW50ZXJuYWwgZmxhc2gsIHNvIGl0IHNob3VsZCBhbHdheXMgYWxyZWFkeSBo YXZlIGEKPj4gd29ya2luZyBmaXJtd2FyZS4KPj4gVGhpcyBzeXNmcyBpbnRlcmZhY2UgaXMgdXNl ZnVsIGZvciB1c2VyIHNwYWNlIGluaXRpYXRlZCBmaWVsZCBmaXJtd2FyZSB1cGRhdGVzLgo+Cj4g TWlnaHQgYmUgYmV0dGVyIHRvIGp1c3QgZG8gYSByZXF1ZXN0X2Zpcm13YXJlIG9uIGRyaXZlciBs b2FkLCBhbmQKPiBzaW1wbHkgcHJvY2VlZCBpZiBpdCdzIG5vdCB0aGVyZS4gQWRkaW5nIGEgc3lz ZnMgaW50ZXJmYWNlICh3aGljaCBpcwo+IGFiaSkgc2VlbXMgd2F5IHRvbyBtdWNoIG92ZXJraWxs IGZvciB0aGlzIGltby4gSWYgeW91IHdhbnQgdG8gdXBncmFkZQo+IHRoZSBmaXJtd2FyZSB5b3Ug Y2FuIHRoZW4ganVzdCBkcm9wIGl0IGludG8gdGhlIHJpZ2h0IGRpcmVjdG9yeSwgd2l0aAo+IG5v IGZ1cnRoZXIgaW50ZXJhY3Rpb24gbmVlZGVkLgoKSU1ITyBJJ20gbm90IHN1cmUgaWYgZm9yIHRo aXMgdXNlIGNhc2UgcmVxdWVzdF9maXJtd2FyZSBvbiBkcml2ZXIgbG9hZAppcyBhIGdvb2QgaWRl YS4gRmxhc2ggdGhlIG5vbi12b2xhdGlsZSBpbnRlcm5hbCBjaGlwIGNhbiBiZSBhIHNsb3cKb3Bl cmF0aW9uIGFuZCBpZiB5b3UgZm9yZ2V0IHRvIHJlbW92ZSB0aGUgZmlybXdhcmUgYWZ0ZXIgZHJv cCBpdCBpbnRvCnRoZSByaWdodCBkaXJlY3RvcnkgYXBhcnQgZnJvbSBzbG93IGRvd24gdGhlIGRy aXZlciBwcm9iZSB5b3UgY2FuCmRhbWFnZSB0aGUgY2hpcCBkZXBlbmRpbmcgb24gdGhlIHdyaXRl IGVuZHVyYW5jZSBvZiB0aGUgY2hpcC4KClRoaXMgc3lzZnMgaW50ZXJmYWNlIGlzIHVzZWQgb24g b3RoZXIgc3Vic3lzdGVtcyB3aGVuIHRoZXJlIGlzIGEKbm9uLXZvbGF0aWxlIG1lbW9yeSwgYXMg ZXhhbXBsZSB5b3UgY2FuIHNlZSBhdCBbMV0sIFsyXS4gVW5mb3J0dW5hdGVseQpub3QgYWxsIGFy ZSB1c2luZyB0aGUgc2FtZSBzeXNmcyBpbnRlcmZhY2UgYW5kIG1heWJlIHRoaXMgc2hvdWxkIGJl CnN0YW5kYXJkaXplZCAobWF5YmUgaXQncyBhbiBvcHBvcnR1bml0eSB0byBkZWZpbmUgaXQpCgpS ZWdhcmRzLAogRW5yaWMKClsxXSBodHRwOi8vbHhyLmZyZWUtZWxlY3Ryb25zLmNvbS9zb3VyY2Uv ZHJpdmVycy9pbnB1dC90b3VjaHNjcmVlbi93ZHQ4N3h4X2kyYy5jI0w5MjIKWzJdIGh0dHA6Ly9s eHIuZnJlZS1lbGVjdHJvbnMuY29tL3NvdXJjZS9kcml2ZXJzL3Njc2kvcG04MDAxL3BtODAwMV9j dGwuYyNMNzMyCgoKPiAtRGFuaWVsCj4gLS0KPiBEYW5pZWwgVmV0dGVyCj4gU29mdHdhcmUgRW5n aW5lZXIsIEludGVsIENvcnBvcmF0aW9uCj4gKzQxICgwKSA3OSAzNjUgNTcgNDggLSBodHRwOi8v YmxvZy5mZndsbC5jaAo+IF9fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19f X19fX19fCj4gZHJpLWRldmVsIG1haWxpbmcgbGlzdAo+IGRyaS1kZXZlbEBsaXN0cy5mcmVlZGVz a3RvcC5vcmcKPiBodHRwczovL2xpc3RzLmZyZWVkZXNrdG9wLm9yZy9tYWlsbWFuL2xpc3RpbmZv L2RyaS1kZXZlbApfX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19f XwpkcmktZGV2ZWwgbWFpbGluZyBsaXN0CmRyaS1kZXZlbEBsaXN0cy5mcmVlZGVza3RvcC5vcmcK aHR0cHM6Ly9saXN0cy5mcmVlZGVza3RvcC5vcmcvbWFpbG1hbi9saXN0aW5mby9kcmktZGV2ZWwK From mboxrd@z Thu Jan 1 00:00:00 1970 From: eballetbo@gmail.com (Enric Balletbo Serra) Date: Thu, 4 Aug 2016 12:35:59 +0200 Subject: [PATCH 2/2 v16] drm/bridge: Add I2C based driver for ps8640 bridge In-Reply-To: References: <1464861445-11086-1-git-send-email-jitao.shi@mediatek.com> <1464861445-11086-2-git-send-email-jitao.shi@mediatek.com> Message-ID: To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org 2016-07-12 12:13 GMT+02:00 Daniel Vetter : > On Wed, Jun 29, 2016 at 6:31 AM, Daniel Kurtz wrote: >> On Fri, Jun 17, 2016 at 3:14 AM, Emil Velikov wrote: >>>> +static ssize_t ps8640_update_fw_store(struct device *dev, >>>> + struct device_attribute *attr, >>>> + const char *buf, size_t count) >>>> +{ >>>> + struct i2c_client *client = to_i2c_client(dev); >>>> + struct ps8640 *ps_bridge = i2c_get_clientdata(client); >>>> + const struct firmware *fw; >>>> + int error; >>>> + >>>> + error = request_firmware(&fw, PS_FW_NAME, dev); >>> Can the device operate without a firmware ? If not, why is the >>> firmware loaded so later/after user interaction (via sysfs) ? I don't >>> recall any other driver in DRM to use such an approach. >> >> The PS8640 has internal flash, so it should always already have a >> working firmware. >> This sysfs interface is useful for user space initiated field firmware updates. > > Might be better to just do a request_firmware on driver load, and > simply proceed if it's not there. Adding a sysfs interface (which is > abi) seems way too much overkill for this imo. If you want to upgrade > the firmware you can then just drop it into the right directory, with > no further interaction needed. IMHO I'm not sure if for this use case request_firmware on driver load is a good idea. Flash the non-volatile internal chip can be a slow operation and if you forget to remove the firmware after drop it into the right directory apart from slow down the driver probe you can damage the chip depending on the write endurance of the chip. This sysfs interface is used on other subsystems when there is a non-volatile memory, as example you can see at [1], [2]. Unfortunately not all are using the same sysfs interface and maybe this should be standardized (maybe it's an opportunity to define it) Regards, Enric [1] http://lxr.free-electrons.com/source/drivers/input/touchscreen/wdt87xx_i2c.c#L922 [2] http://lxr.free-electrons.com/source/drivers/scsi/pm8001/pm8001_ctl.c#L732 > -Daniel > -- > Daniel Vetter > Software Engineer, Intel Corporation > +41 (0) 79 365 57 48 - http://blog.ffwll.ch > _______________________________________________ > dri-devel mailing list > dri-devel at lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel