From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-8.3 required=3.0 tests=DKIM_SIGNED,DKIM_VALID, DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH,MAILING_LIST_MULTI, SIGNED_OFF_BY,SPF_HELO_NONE,SPF_PASS,UNPARSEABLE_RELAY,USER_AGENT_SANE_2 autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id AEDF8C433E0 for ; Fri, 12 Jun 2020 10:49:13 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 6F7902081A for ; Fri, 12 Jun 2020 10:49:13 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (1024-bit key) header.d=mediatek.com header.i=@mediatek.com header.b="dXdcdZjV" Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1726259AbgFLKtM (ORCPT ); Fri, 12 Jun 2020 06:49:12 -0400 Received: from Mailgw01.mediatek.com ([1.203.163.78]:12083 "EHLO mailgw01.mediatek.com" rhost-flags-OK-FAIL-OK-FAIL) by vger.kernel.org with ESMTP id S1726272AbgFLKtM (ORCPT ); Fri, 12 Jun 2020 06:49:12 -0400 X-UUID: a1b6cd7118d14020800a9f90a80c2100-20200612 DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=mediatek.com; s=dk; h=Content-Transfer-Encoding:MIME-Version:Content-Type:References:In-Reply-To:Date:CC:To:From:Subject:Message-ID; bh=TpVWyGTnlu5KQ1bLwhmMEtCgBHatHCR86J3sG18+1ro=; b=dXdcdZjV4l04zAUJl990F6csVmdi6DOXav/hW5FUPP9zEBf7vgAbvA92tdGUm2xyDCtaf9ZstwmUA51y3KcPvdPJpJ9ClhaSPIxexVNDLDAhbM4kD8GNoATk0E7UvIca8Y7bR2qKjBiRQOt6XqAgEZIJCkbzexEMxYl+905iDYg=; X-UUID: a1b6cd7118d14020800a9f90a80c2100-20200612 Received: from mtkcas36.mediatek.inc [(172.27.4.253)] by mailgw01.mediatek.com (envelope-from ) (mailgw01.mediatek.com ESMTP with TLS) with ESMTP id 1452544618; Fri, 12 Jun 2020 18:48:58 +0800 Received: from MTKCAS32.mediatek.inc (172.27.4.184) by MTKMBS31DR.mediatek.inc (172.27.6.102) with Microsoft SMTP Server (TLS) id 15.0.1497.2; Fri, 12 Jun 2020 18:48:57 +0800 Received: from [10.17.3.153] (10.17.3.153) by MTKCAS32.mediatek.inc (172.27.4.170) with Microsoft SMTP Server id 15.0.1497.2 via Frontend Transport; Fri, 12 Jun 2020 18:48:55 +0800 Message-ID: <1591958798.8804.660.camel@mhfsdcap03> Subject: Re: [V9, 2/2] media: i2c: ov02a10: Add OV02A10 image sensor driver From: Dongchun Zhu To: Tomasz Figa CC: , , , , , , , , , , , , , , , , , , Date: Fri, 12 Jun 2020 18:46:38 +0800 In-Reply-To: <20200610194455.GK201868@chromium.org> References: <20200523084103.31276-1-dongchun.zhu@mediatek.com> <20200523084103.31276-3-dongchun.zhu@mediatek.com> <20200610194455.GK201868@chromium.org> Content-Type: text/plain; charset="UTF-8" X-Mailer: Evolution 3.10.4-0ubuntu2 MIME-Version: 1.0 X-TM-SNTS-SMTP: F9B070F852F4BB7A908158EEA1657918D95D6AE037CA2AD2A6675516E8D3B9A92000:8 X-MTK: N Content-Transfer-Encoding: base64 Sender: devicetree-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: devicetree@vger.kernel.org SGkgVG9tYXN6LA0KDQpPbiBXZWQsIDIwMjAtMDYtMTAgYXQgMTk6NDQgKzAwMDAsIFRvbWFzeiBG aWdhIHdyb3RlOg0KPiBIaSBEb25nY2h1biwNCj4gDQo+IE9uIFNhdCwgTWF5IDIzLCAyMDIwIGF0 IDA0OjQxOjAzUE0gKzA4MDAsIERvbmdjaHVuIFpodSB3cm90ZToNCj4gPiBBZGQgYSBWNEwyIHN1 Yi1kZXZpY2UgZHJpdmVyIGZvciBPVjAyQTEwIGltYWdlIHNlbnNvci4NCj4gPiANCj4gPiBTaWdu ZWQtb2ZmLWJ5OiBEb25nY2h1biBaaHUgPGRvbmdjaHVuLnpodUBtZWRpYXRlay5jb20+DQo+ID4g LS0tDQo+ID4gIE1BSU5UQUlORVJTICAgICAgICAgICAgICAgICB8ICAgIDEgKw0KPiA+ICBkcml2 ZXJzL21lZGlhL2kyYy9LY29uZmlnICAgfCAgIDEzICsNCj4gPiAgZHJpdmVycy9tZWRpYS9pMmMv TWFrZWZpbGUgIHwgICAgMSArDQo+ID4gIGRyaXZlcnMvbWVkaWEvaTJjL292MDJhMTAuYyB8IDEw MjUgKysrKysrKysrKysrKysrKysrKysrKysrKysrKysrKysrKysrKysrKysrKw0KPiA+ICA0IGZp bGVzIGNoYW5nZWQsIDEwNDAgaW5zZXJ0aW9ucygrKQ0KPiA+ICBjcmVhdGUgbW9kZSAxMDA2NDQg ZHJpdmVycy9tZWRpYS9pMmMvb3YwMmExMC5jDQo+ID4gDQo+IA0KPiBUaGFuayB5b3UgZm9yIHRo ZSBwYXRjaC4gUGxlYXNlIHNlZSBteSBjb21tZW50cyBpbmxpbmUuDQo+IA0KPiBbc25pcF0NCj4g PiBkaWZmIC0tZ2l0IGEvZHJpdmVycy9tZWRpYS9pMmMvb3YwMmExMC5jIGIvZHJpdmVycy9tZWRp YS9pMmMvb3YwMmExMC5jDQo+ID4gbmV3IGZpbGUgbW9kZSAxMDA2NDQNCj4gPiBpbmRleCAwMDAw MDAwLi4xNjBhMGI1DQo+ID4gLS0tIC9kZXYvbnVsbA0KPiA+ICsrKyBiL2RyaXZlcnMvbWVkaWEv aTJjL292MDJhMTAuYw0KPiBbc25pcF0NCj4gPiArc3RhdGljIGNvbnN0IGNoYXIgKiBjb25zdCBv djAyYTEwX3Rlc3RfcGF0dGVybl9tZW51W10gPSB7DQo+ID4gKwkiRGlzYWJsZWQiLA0KPiA+ICsJ IkNvbG9yIEJhciIsDQo+IA0KPiBuaXQ6IFdlIHNob3VsZCBub3JtYWxpemUgdGhpcyB0byBvbmUg b2YgdGhlIHN0YW5kYXJkIG5hbWVzLiBXaGF0IGlzIHRoZQ0KPiBwYXR0ZXJuIG9uIHRoaXMgc2Vu c29yPyBJcyBpdCBwZXJoYXBzICJFaWdodCBWZXJ0aWNhbCBDb2xvdXIgQmFycyI/DQo+IA0KDQpZ ZXMuIEl0IGlzIG9uZSBraW5kIG9mICdFaWdodCBWZXJ0aWNhbCBDb2xvdXIgQmFycycuDQpUaGlz IHBhdHRlcm4gaXMgY2FsbGVkIGFzICdNSVBJIGNvbG9yIGJhcicgcGVyIHRoZSBkYXRhc2hlZXQu DQpDYW4gd2UgaGVyZSB1c2UgJ1ZlcnRpY2FsIENvbG9yIEJhcicgb3IgJ01JUEkgQ29sb3IgQmFy Jz8NCg0KPiA+ICt9Ow0KPiBbc25pcF0NCj4gPiArc3RhdGljIGludCBvdjAyYTEwX3NldF9mbXQo c3RydWN0IHY0bDJfc3ViZGV2ICpzZCwNCj4gPiArCQkJICAgc3RydWN0IHY0bDJfc3ViZGV2X3Bh ZF9jb25maWcgKmNmZywNCj4gPiArCQkJICAgc3RydWN0IHY0bDJfc3ViZGV2X2Zvcm1hdCAqZm10 KQ0KPiA+ICt7DQo+ID4gKwlzdHJ1Y3Qgb3YwMmExMCAqb3YwMmExMCA9IHRvX292MDJhMTAoc2Qp Ow0KPiA+ICsJc3RydWN0IHY0bDJfbWJ1c19mcmFtZWZtdCAqbWJ1c19mbXQgPSAmZm10LT5mb3Jt YXQ7DQo+ID4gKw0KPiA+ICsJbXV0ZXhfbG9jaygmb3YwMmExMC0+bXV0ZXgpOw0KPiA+ICsNCj4g DQo+IA0KPiBEb24ndCB3ZSBuZWVkIHRvIGhhbmRsZSB0aGUgY2FzZSB3aGVuIGZtdC0+d2hpY2gg aXMgVjRMMl9TVUJERVZfRk9STUFUX1RSWSwNCj4gd2hpY2ggaXMgdXNlZCBmb3IgdHJ5aW5nIHRo ZSBmb3JtYXQsIGJ1dCBub3QgYXBwbHlpbmcgaXQgdG8gdGhlIGhhcmR3YXJlPw0KPiANCg0KR290 IGl0IDotKQ0KDQo+ID4gKwlpZiAob3YwMmExMC0+c3RyZWFtaW5nKSB7DQo+ID4gKwkJbXV0ZXhf dW5sb2NrKCZvdjAyYTEwLT5tdXRleCk7DQo+ID4gKwkJcmV0dXJuIC1FQlVTWTsNCj4gPiArCX0N Cj4gPiArDQo+ID4gKwkvKiBPbmx5IG9uZSBzZW5zb3IgbW9kZSBzdXBwb3J0ZWQgKi8NCj4gPiAr CW1idXNfZm10LT5jb2RlID0gb3YwMmExMC0+Zm10LmNvZGU7DQo+ID4gKwlvdjAyYTEwX2ZpbGxf Zm10KG92MDJhMTAtPmN1cl9tb2RlLCBtYnVzX2ZtdCk7DQo+ID4gKwlvdjAyYTEwLT5mbXQgPSBm bXQtPmZvcm1hdDsNCj4gPiArDQo+ID4gKwltdXRleF91bmxvY2soJm92MDJhMTAtPm11dGV4KTsN Cj4gPiArDQo+ID4gKwlyZXR1cm4gMDsNCj4gPiArfQ0KPiA+ICsNCj4gPiArc3RhdGljIGludCBv djAyYTEwX2dldF9mbXQoc3RydWN0IHY0bDJfc3ViZGV2ICpzZCwNCj4gPiArCQkJICAgc3RydWN0 IHY0bDJfc3ViZGV2X3BhZF9jb25maWcgKmNmZywNCj4gPiArCQkJICAgc3RydWN0IHY0bDJfc3Vi ZGV2X2Zvcm1hdCAqZm10KQ0KPiA+ICt7DQo+ID4gKwlzdHJ1Y3Qgb3YwMmExMCAqb3YwMmExMCA9 IHRvX292MDJhMTAoc2QpOw0KPiA+ICsJc3RydWN0IHY0bDJfbWJ1c19mcmFtZWZtdCAqbWJ1c19m bXQgPSAmZm10LT5mb3JtYXQ7DQo+ID4gKw0KPiA+ICsJbXV0ZXhfbG9jaygmb3YwMmExMC0+bXV0 ZXgpOw0KPiA+ICsNCj4gPiArCWZtdC0+Zm9ybWF0ID0gb3YwMmExMC0+Zm10Ow0KPiANCj4gRGl0 dG8uDQo+IA0KPiA+ICsJbWJ1c19mbXQtPmNvZGUgPSBvdjAyYTEwLT5mbXQuY29kZTsNCj4gPiAr CW92MDJhMTBfZmlsbF9mbXQob3YwMmExMC0+Y3VyX21vZGUsIG1idXNfZm10KTsNCj4gPiArDQo+ ID4gKwltdXRleF91bmxvY2soJm92MDJhMTAtPm11dGV4KTsNCj4gPiArDQo+ID4gKwlyZXR1cm4g MDsNCj4gPiArfQ0KPiA+ICsNCj4gPiArc3RhdGljIGludCBvdjAyYTEwX2VudW1fbWJ1c19jb2Rl KHN0cnVjdCB2NGwyX3N1YmRldiAqc2QsDQo+ID4gKwkJCQkgIHN0cnVjdCB2NGwyX3N1YmRldl9w YWRfY29uZmlnICpjZmcsDQo+ID4gKwkJCQkgIHN0cnVjdCB2NGwyX3N1YmRldl9tYnVzX2NvZGVf ZW51bSAqY29kZSkNCj4gPiArew0KPiA+ICsJc3RydWN0IG92MDJhMTAgKm92MDJhMTAgPSB0b19v djAyYTEwKHNkKTsNCj4gPiArDQo+ID4gKwlpZiAoY29kZS0+aW5kZXggPj0gQVJSQVlfU0laRShz dXBwb3J0ZWRfbW9kZXMpKQ0KPiA+ICsJCXJldHVybiAtRUlOVkFMOw0KPiANCj4gSG1tLCBzdXBw b3J0ZWRfbW9kZXNbXSBkb2Vzbid0IHNlZW0gdG8gaG9sZCB0aGUgaW5mb3JtYXRpb24gYWJvdXQg bWJ1cw0KPiBjb2Rlcy4gU2hvdWxkIHRoaXMganVzdCBwZXJoYXBzIGJlICIhPSAwIj8NCj4gDQoN ClVuZGVyc3Rvb2QuDQoNCj4gPiArDQo+ID4gKwljb2RlLT5jb2RlID0gb3YwMmExMC0+Zm10LmNv ZGU7DQo+ID4gKw0KPiA+ICsJcmV0dXJuIDA7DQo+ID4gK30NCj4gW3NuaXBdDQo+ID4gK3N0YXRp YyBpbnQgb3YwMmExMF9lbnRpdHlfaW5pdF9jZmcoc3RydWN0IHY0bDJfc3ViZGV2ICpzZCwNCj4g PiArCQkJCSAgIHN0cnVjdCB2NGwyX3N1YmRldl9wYWRfY29uZmlnICpjZmcpDQo+ID4gK3sNCj4g PiArCXN0cnVjdCB2NGwyX3N1YmRldl9mb3JtYXQgZm10ID0gew0KPiA+ICsJCS53aGljaCA9IGNm ZyA/IFY0TDJfU1VCREVWX0ZPUk1BVF9UUlkgOiBWNEwyX1NVQkRFVl9GT1JNQVRfQUNUSVZFLA0K PiA+ICsJCS5mb3JtYXQgPSB7DQo+ID4gKwkJCS53aWR0aCA9IDE2MDAsDQo+ID4gKwkJCS5oZWln aHQgPSAxMjAwLA0KPiA+ICsJCX0NCj4gPiArCX07DQo+ID4gKw0KPiA+ICsJb3YwMmExMF9zZXRf Zm10KHNkLCBjZmcsICZmbXQpOw0KPiA+ICsNCj4gPiArCXJldHVybiAwOw0KPiA+ICt9DQo+ID4g Kw0KPiANCj4gSSdtIG5vdCBmYW1pbGlhciB3aXRoIHRoaXMgaW5pdF9jZmcgb3BlcmF0aW9uIGFu ZCB0aGUgZG9jdW1lbnRhdGlvbiBpcyB2ZXJ5DQo+IHNwYXJzZSBhYm91dCBpdC4gU2FrYXJpLCBp cyB0aGlzIGEgY29ycmVjdCBpbXBsZW1lbnRhdGlvbj8NCj4gDQo+IFtzbmlwXQ0KPiA+ICtzdGF0 aWMgaW50IG92MDJhMTBfc2V0X3Rlc3RfcGF0dGVybihzdHJ1Y3Qgb3YwMmExMCAqb3YwMmExMCwg aW50IHBhdHRlcm4pDQo+ID4gK3sNCj4gPiArCXN0cnVjdCBpMmNfY2xpZW50ICpjbGllbnQgPSB2 NGwyX2dldF9zdWJkZXZkYXRhKCZvdjAyYTEwLT5zdWJkZXYpOw0KPiA+ICsJaW50IHJldDsNCj4g PiArDQo+ID4gKwlyZXQgPSBpMmNfc21idXNfd3JpdGVfYnl0ZV9kYXRhKGNsaWVudCwgUkVHX1BB R0VfU1dJVENILCBSRUdfRU5BQkxFKTsNCj4gPiArCWlmIChyZXQgPCAwKQ0KPiA+ICsJCXJldHVy biByZXQ7DQo+ID4gKw0KPiA+ICsJcmV0ID0gaTJjX3NtYnVzX3dyaXRlX2J5dGVfZGF0YShjbGll bnQsIE9WMDJBMTBfUkVHX1RFU1RfUEFUVEVSTiwNCj4gPiArCQkJCQlwYXR0ZXJuKTsNCj4gPiAr CWlmIChyZXQgPCAwKQ0KPiA+ICsJCXJldHVybiByZXQ7DQo+ID4gKw0KPiA+ICsJcmV0ID0gaTJj X3NtYnVzX3dyaXRlX2J5dGVfZGF0YShjbGllbnQsIFJFR19HTE9CQUxfRUZGRUNUSVZFLA0KPiA+ ICsJCQkJCVJFR19FTkFCTEUpOw0KPiA+ICsJaWYgKHJldCA8IDApDQo+ID4gKwkJcmV0dXJuIHJl dDsNCj4gPiArDQo+ID4gKwlyZXR1cm4gaTJjX3NtYnVzX3dyaXRlX2J5dGVfZGF0YShjbGllbnQs IFJFR19TQ19DVFJMX01PREUsDQo+ID4gKwkJCQkJIFNDX0NUUkxfTU9ERV9TVFJFQU1JTkcpOw0K PiANCj4gV2h5IGlzIHRoaXMgbmVlZGVkPyBEb2VzIHdyaXRpbmcgdGhlIHRlc3QgcGF0dGVybiBy ZWdpc3RlciBzdG9wIHN0cmVhbWluZz8NCj4gDQoNCkxvb2tpbmcgYmFjayB0byB0aGUgc2V0dGlu ZyBoaXN0b3J5LCBJIGZvdW5kIGl0IHdhcyBzdWdnZXN0ZWQgYnkgT1YuDQpJIHdvdWxkIGxlYXZl IHlvdXIgcXVlc3Rpb24gdG8gT1YsIGFuZCB1cGRhdGUgdGhlaXIgZmVlZGJhY2suDQoNCj4gW3Nu aXBdDQo+ID4gK3N0YXRpYyBpbnQgb3YwMmExMF9pbml0aWFsaXplX2NvbnRyb2xzKHN0cnVjdCBv djAyYTEwICpvdjAyYTEwKQ0KPiA+ICt7DQo+ID4gKwlzdHJ1Y3QgaTJjX2NsaWVudCAqY2xpZW50 ID0gdjRsMl9nZXRfc3ViZGV2ZGF0YSgmb3YwMmExMC0+c3ViZGV2KTsNCj4gPiArCWNvbnN0IHN0 cnVjdCBvdjAyYTEwX21vZGUgKm1vZGU7DQo+ID4gKwlzdHJ1Y3QgdjRsMl9jdHJsX2hhbmRsZXIg KmhhbmRsZXI7DQo+ID4gKwlzdHJ1Y3QgdjRsMl9jdHJsICpjdHJsOw0KPiA+ICsJdTY0IGV4cG9z dXJlX21heDsNCj4gPiArCXUzMiBwaXhlbF9yYXRlOw0KPiA+ICsJaW50IHJldDsNCj4gPiArDQo+ ID4gKwloYW5kbGVyID0gJm92MDJhMTAtPmN0cmxfaGFuZGxlcjsNCj4gPiArCW1vZGUgPSBvdjAy YTEwLT5jdXJfbW9kZTsNCj4gPiArCXJldCA9IHY0bDJfY3RybF9oYW5kbGVyX2luaXQoaGFuZGxl ciwgNyk7DQo+ID4gKwlpZiAocmV0KQ0KPiA+ICsJCXJldHVybiByZXQ7DQo+ID4gKw0KPiA+ICsJ aGFuZGxlci0+bG9jayA9ICZvdjAyYTEwLT5tdXRleDsNCj4gPiArDQo+ID4gKwljdHJsID0gdjRs Ml9jdHJsX25ld19pbnRfbWVudShoYW5kbGVyLCBOVUxMLCBWNEwyX0NJRF9MSU5LX0ZSRVEsIDAs IDAsDQo+ID4gKwkJCQkgICAgICBsaW5rX2ZyZXFfbWVudV9pdGVtcyk7DQo+ID4gKwlpZiAoY3Ry bCkNCj4gPiArCQljdHJsLT5mbGFncyB8PSBWNEwyX0NUUkxfRkxBR19SRUFEX09OTFk7DQo+ID4g Kw0KPiA+ICsJcGl4ZWxfcmF0ZSA9IHRvX3BpeGVsX3JhdGUoMCk7DQo+ID4gKwl2NGwyX2N0cmxf bmV3X3N0ZChoYW5kbGVyLCBOVUxMLCBWNEwyX0NJRF9QSVhFTF9SQVRFLCAwLCBwaXhlbF9yYXRl LCAxLA0KPiA+ICsJCQkgIHBpeGVsX3JhdGUpOw0KPiA+ICsNCj4gPiArCWV4cG9zdXJlX21heCA9 IG1vZGUtPnZ0c19kZWYgLSA0Ow0KPiA+ICsJb3YwMmExMC0+ZXhwb3N1cmUgPSB2NGwyX2N0cmxf bmV3X3N0ZChoYW5kbGVyLCAmb3YwMmExMF9jdHJsX29wcywNCj4gPiArCQkJCQkgICAgICBWNEwy X0NJRF9FWFBPU1VSRSwNCj4gPiArCQkJCQkgICAgICBPVjAyQTEwX0VYUE9TVVJFX01JTiwNCj4g PiArCQkJCQkgICAgICBleHBvc3VyZV9tYXgsDQo+ID4gKwkJCQkJICAgICAgT1YwMkExMF9FWFBP U1VSRV9TVEVQLA0KPiA+ICsJCQkJCSAgICAgIG1vZGUtPmV4cF9kZWYpOw0KPiA+ICsNCj4gPiAr CXY0bDJfY3RybF9uZXdfc3RkKGhhbmRsZXIsICZvdjAyYTEwX2N0cmxfb3BzLA0KPiA+ICsJCQkg IFY0TDJfQ0lEX0FOQUxPR1VFX0dBSU4sDQo+ID4gKwkJCSAgT1YwMkExMF9HQUlOX01JTiwNCj4g PiArCQkJICBPVjAyQTEwX0dBSU5fTUFYLA0KPiA+ICsJCQkgIE9WMDJBMTBfR0FJTl9TVEVQLA0K PiA+ICsJCQkgIE9WMDJBMTBfR0FJTl9ERUZBVUxUKTsNCj4gPiArDQo+ID4gKwl2NGwyX2N0cmxf bmV3X3N0ZF9tZW51X2l0ZW1zKGhhbmRsZXIsICZvdjAyYTEwX2N0cmxfb3BzLA0KPiA+ICsJCQkJ ICAgICBWNEwyX0NJRF9URVNUX1BBVFRFUk4sDQo+ID4gKwkJCQkgICAgIEFSUkFZX1NJWkUob3Yw MmExMF90ZXN0X3BhdHRlcm5fbWVudSkgLSAxLA0KPiA+ICsJCQkJICAgICAwLCAwLCBvdjAyYTEw X3Rlc3RfcGF0dGVybl9tZW51KTsNCj4gPiArDQo+IA0KPiBJIGNhbiBzZWUgdGhhdCB3ZSdyZSBt aXNzaW5nIHNvbWUgY29udHJvbHMgaGVyZSBub3csIFZCTEFOSyBhbmQgSEJMQU5LIGlmIEkNCj4g cmVtZW1iZXIgY29ycmVjdGx5LiBFdmVuIHRob3VnaCByZWFkLW9ubHksIHNvbWUgdXNlcnNwYWNl IG5lZWQgdGhvc2UgdG8NCj4gZ2V0IGluZm9ybWF0aW9uIGFib3V0IGhvdyB0aGUgc2Vuc29yIG9w ZXJhdGVzLg0KPiANCg0KWWVzLiBJIG1hZGUgYSBtaXN0YWtlLg0KDQo+ID4gKwlpZiAoaGFuZGxl ci0+ZXJyb3IpIHsNCj4gPiArCQlyZXQgPSBoYW5kbGVyLT5lcnJvcjsNCj4gPiArCQlkZXZfZXJy KCZjbGllbnQtPmRldiwgImZhaWxlZCB0byBpbml0IGNvbnRyb2xzKCVkKVxuIiwgcmV0KTsNCj4g PiArCQlnb3RvIGVycl9mcmVlX2hhbmRsZXI7DQo+ID4gKwl9DQo+ID4gKw0KPiA+ICsJb3YwMmEx MC0+c3ViZGV2LmN0cmxfaGFuZGxlciA9IGhhbmRsZXI7DQo+ID4gKw0KPiA+ICsJcmV0dXJuIDA7 DQo+ID4gKw0KPiA+ICtlcnJfZnJlZV9oYW5kbGVyOg0KPiA+ICsJdjRsMl9jdHJsX2hhbmRsZXJf ZnJlZShoYW5kbGVyKTsNCj4gPiArDQo+ID4gKwlyZXR1cm4gcmV0Ow0KPiA+ICt9DQo+IFtzbmlw XQ0KPiA+ICsJcG1fcnVudGltZV9lbmFibGUoZGV2KTsNCj4gPiArCWlmICghcG1fcnVudGltZV9l bmFibGVkKGRldikpIHsNCj4gPiArCQlyZXQgPSBvdjAyYTEwX3Bvd2VyX29uKGRldik7DQo+ID4g KwkJaWYgKHJldCA8IDApIHsNCj4gPiArCQkJZGV2X2VycihkZXYsICJmYWlsZWQgdG8gcG93ZXIg b246ICVkXG4iLCByZXQpOw0KPiA+ICsJCQlnb3RvIGVycl9mcmVlX2hhbmRsZXI7DQo+ID4gKwkJ fQ0KPiA+ICsJfQ0KPiA+ICsNCj4gPiArCXJldCA9IHY0bDJfYXN5bmNfcmVnaXN0ZXJfc3ViZGV2 KCZvdjAyYTEwLT5zdWJkZXYpOw0KPiA+ICsJaWYgKHJldCkgew0KPiA+ICsJCWRldl9lcnIoZGV2 LCAiZmFpbGVkIHRvIHJlZ2lzdGVyIFY0TDIgc3ViZGV2OiAlZCIsIHJldCk7DQo+ID4gKwkJaWYg KCFwbV9ydW50aW1lX2VuYWJsZWQoZGV2KSkNCj4gPiArCQkJb3YwMmExMF9wb3dlcl9vZmYoZGV2 KTsNCj4gDQo+IFBsZWFzZSBkb24ndCBtaXggaW5saW5lIGFuZCBlcnJvci1wYXRoIGVycm9yIGhh bmRsaW5nLCBhcyBpdCBtYWtlcyBpdA0KPiBkaWZmaWN1bHQgdG8gdGVsbCBpZiBpdCdzIGNvcnJl Y3QuIFBsZWFzZSBtb3ZlIHRoaXMgYmVsb3cgdGhlIGFwcHJvcHJpYXRlDQo+IGVyciBsYWJlbCBp bnN0ZWFkLg0KPiANCg0KRml4ZWQgaW4gbmV4dCByZWxlYXNlLg0KDQo+ID4gKwkJZ290byBlcnJf Y2xlYW5fZW50aXR5Ow0KPiA+ICsJfQ0KPiA+ICsNCj4gPiArCXJldHVybiAwOw0KPiA+ICsNCj4g PiArZXJyX2NsZWFuX2VudGl0eToNCj4gDQo+IElmIG9uZSBjYWxscyBwbV9ydW50aW1lX2VuYWJs ZSgpIGluIHRoZSBwcm9iZSBwYXRoLCBvbmUgbmVlZHMgdG8gY2FsbA0KPiBwbV9ydW50aW1lX2Rp c2FibGUoKSBvbiB0aGUgZXJyb3IgYW5kIHJlbW92ZSBwYXRocy4NCj4gDQoNClllcywgZml4ZWQg aW4gbmV4dCByZWxlYXNlLg0KDQo+ID4gKwltZWRpYV9lbnRpdHlfY2xlYW51cCgmb3YwMmExMC0+ c3ViZGV2LmVudGl0eSk7DQo+ID4gK2Vycl9mcmVlX2hhbmRsZXI6DQo+ID4gKwl2NGwyX2N0cmxf aGFuZGxlcl9mcmVlKG92MDJhMTAtPnN1YmRldi5jdHJsX2hhbmRsZXIpOw0KPiA+ICtlcnJfZGVz dHJveV9tdXRleDoNCj4gPiArCW11dGV4X2Rlc3Ryb3koJm92MDJhMTAtPm11dGV4KTsNCj4gPiAr DQo+ID4gKwlyZXR1cm4gcmV0Ow0KPiA+ICt9DQo+ID4gKw0KPiANCj4gQmVzdCByZWdhcmRzLA0K PiBUb21hc3oNCg0K From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-8.2 required=3.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED, DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH,MAILING_LIST_MULTI, SIGNED_OFF_BY,SPF_HELO_NONE,SPF_PASS,UNPARSEABLE_RELAY,USER_AGENT_SANE_2 autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 09F02C433E0 for ; Fri, 12 Jun 2020 10:49:21 +0000 (UTC) Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id B9EF02081A for ; Fri, 12 Jun 2020 10:49:20 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=lists.infradead.org header.i=@lists.infradead.org header.b="cmeHA0LD"; dkim=fail reason="signature verification failed" (1024-bit key) header.d=mediatek.com header.i=@mediatek.com header.b="dXdcdZjV" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org B9EF02081A Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=mediatek.com Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-mediatek-bounces+linux-mediatek=archiver.kernel.org@lists.infradead.org DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20170209; h=Sender: Content-Transfer-Encoding:Content-Type:Cc:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:MIME-Version:References:In-Reply-To: Date:To:From:Subject:Message-ID:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=0WW667oJ7VJAXX2MDWsGjHBLuVWLeUUyEiIP3DOb7SU=; b=cmeHA0LD1yb6Wm TNDuFS8CLxNp7FAqsCSKyCwe37qm5kMeyAL6Bht7pyQwom+Ok5AeX3xjOFvDr4Vmt3XHPSurs7RCV 8yRmVdgARcOVeUQjbJ5Lj8VHZ2oE6TO/tIThkPr3Sr7EK/iTalxVTqJjud3PFGDmL1NWRbU8fhhXs UKhEyxUs7x4rwe2/PH1ybpJPibud4SFK1d6PBU/xbI83i0X9HVPsrn8Pf2Nndznw7Mt6aBHyIYX1i m5q3xz6jpAtGRBNW6SF3K7xZeHyr5LuBKrXSeWiEOlAkIrIYyGPmYEg6RMuCOLjsuH+jPQCQHCTHP da9QecKjvrUiD1uFNNKQ==; Received: from localhost ([127.0.0.1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.92.3 #3 (Red Hat Linux)) id 1jjhFI-0005hQ-Dv; Fri, 12 Jun 2020 10:49:12 +0000 Received: from mailgw02.mediatek.com ([216.200.240.185]) by bombadil.infradead.org with esmtps (Exim 4.92.3 #3 (Red Hat Linux)) id 1jjhFG-0005gv-1D; Fri, 12 Jun 2020 10:49:11 +0000 X-UUID: 10fb0c88711841498051ee63350ed78f-20200612 DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=mediatek.com; s=dk; h=Content-Transfer-Encoding:MIME-Version:Content-Type:References:In-Reply-To:Date:CC:To:From:Subject:Message-ID; bh=TpVWyGTnlu5KQ1bLwhmMEtCgBHatHCR86J3sG18+1ro=; b=dXdcdZjV4l04zAUJl990F6csVmdi6DOXav/hW5FUPP9zEBf7vgAbvA92tdGUm2xyDCtaf9ZstwmUA51y3KcPvdPJpJ9ClhaSPIxexVNDLDAhbM4kD8GNoATk0E7UvIca8Y7bR2qKjBiRQOt6XqAgEZIJCkbzexEMxYl+905iDYg=; X-UUID: 10fb0c88711841498051ee63350ed78f-20200612 Received: from mtkcas66.mediatek.inc [(172.29.193.44)] by mailgw02.mediatek.com (envelope-from ) (musrelay.mediatek.com ESMTP with TLS) with ESMTP id 1074978896; Fri, 12 Jun 2020 02:48:55 -0800 Received: from MTKMBS31DR.mediatek.inc (172.27.6.102) by MTKMBS62N1.mediatek.inc (172.29.193.41) with Microsoft SMTP Server (TLS) id 15.0.1497.2; Fri, 12 Jun 2020 03:48:59 -0700 Received: from MTKCAS32.mediatek.inc (172.27.4.184) by MTKMBS31DR.mediatek.inc (172.27.6.102) with Microsoft SMTP Server (TLS) id 15.0.1497.2; Fri, 12 Jun 2020 18:48:57 +0800 Received: from [10.17.3.153] (10.17.3.153) by MTKCAS32.mediatek.inc (172.27.4.170) with Microsoft SMTP Server id 15.0.1497.2 via Frontend Transport; Fri, 12 Jun 2020 18:48:55 +0800 Message-ID: <1591958798.8804.660.camel@mhfsdcap03> Subject: Re: [V9, 2/2] media: i2c: ov02a10: Add OV02A10 image sensor driver From: Dongchun Zhu To: Tomasz Figa Date: Fri, 12 Jun 2020 18:46:38 +0800 In-Reply-To: <20200610194455.GK201868@chromium.org> References: <20200523084103.31276-1-dongchun.zhu@mediatek.com> <20200523084103.31276-3-dongchun.zhu@mediatek.com> <20200610194455.GK201868@chromium.org> X-Mailer: Evolution 3.10.4-0ubuntu2 MIME-Version: 1.0 X-TM-SNTS-SMTP: F9B070F852F4BB7A908158EEA1657918D95D6AE037CA2AD2A6675516E8D3B9A92000:8 X-MTK: N X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20200612_034910_083628_DE9A8D33 X-CRM114-Status: GOOD ( 30.64 ) X-BeenThere: linux-mediatek@lists.infradead.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: mark.rutland@arm.com, drinkcat@chromium.org, andriy.shevchenko@linux.intel.com, srv_heupstream@mediatek.com, devicetree@vger.kernel.org, linus.walleij@linaro.org, shengnan.wang@mediatek.com, louis.kuo@mediatek.com, bgolaszewski@baylibre.com, sj.huang@mediatek.com, robh+dt@kernel.org, linux-mediatek@lists.infradead.org, dongchun.zhu@mediatek.com, sakari.ailus@linux.intel.com, matthias.bgg@gmail.com, bingbu.cao@intel.com, mchehab@kernel.org, linux-arm-kernel@lists.infradead.org, linux-media@vger.kernel.org Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "Linux-mediatek" Errors-To: linux-mediatek-bounces+linux-mediatek=archiver.kernel.org@lists.infradead.org Hi Tomasz, On Wed, 2020-06-10 at 19:44 +0000, Tomasz Figa wrote: > Hi Dongchun, > > On Sat, May 23, 2020 at 04:41:03PM +0800, Dongchun Zhu wrote: > > Add a V4L2 sub-device driver for OV02A10 image sensor. > > > > Signed-off-by: Dongchun Zhu > > --- > > MAINTAINERS | 1 + > > drivers/media/i2c/Kconfig | 13 + > > drivers/media/i2c/Makefile | 1 + > > drivers/media/i2c/ov02a10.c | 1025 +++++++++++++++++++++++++++++++++++++++++++ > > 4 files changed, 1040 insertions(+) > > create mode 100644 drivers/media/i2c/ov02a10.c > > > > Thank you for the patch. Please see my comments inline. > > [snip] > > diff --git a/drivers/media/i2c/ov02a10.c b/drivers/media/i2c/ov02a10.c > > new file mode 100644 > > index 0000000..160a0b5 > > --- /dev/null > > +++ b/drivers/media/i2c/ov02a10.c > [snip] > > +static const char * const ov02a10_test_pattern_menu[] = { > > + "Disabled", > > + "Color Bar", > > nit: We should normalize this to one of the standard names. What is the > pattern on this sensor? Is it perhaps "Eight Vertical Colour Bars"? > Yes. It is one kind of 'Eight Vertical Colour Bars'. This pattern is called as 'MIPI color bar' per the datasheet. Can we here use 'Vertical Color Bar' or 'MIPI Color Bar'? > > +}; > [snip] > > +static int ov02a10_set_fmt(struct v4l2_subdev *sd, > > + struct v4l2_subdev_pad_config *cfg, > > + struct v4l2_subdev_format *fmt) > > +{ > > + struct ov02a10 *ov02a10 = to_ov02a10(sd); > > + struct v4l2_mbus_framefmt *mbus_fmt = &fmt->format; > > + > > + mutex_lock(&ov02a10->mutex); > > + > > > Don't we need to handle the case when fmt->which is V4L2_SUBDEV_FORMAT_TRY, > which is used for trying the format, but not applying it to the hardware? > Got it :-) > > + if (ov02a10->streaming) { > > + mutex_unlock(&ov02a10->mutex); > > + return -EBUSY; > > + } > > + > > + /* Only one sensor mode supported */ > > + mbus_fmt->code = ov02a10->fmt.code; > > + ov02a10_fill_fmt(ov02a10->cur_mode, mbus_fmt); > > + ov02a10->fmt = fmt->format; > > + > > + mutex_unlock(&ov02a10->mutex); > > + > > + return 0; > > +} > > + > > +static int ov02a10_get_fmt(struct v4l2_subdev *sd, > > + struct v4l2_subdev_pad_config *cfg, > > + struct v4l2_subdev_format *fmt) > > +{ > > + struct ov02a10 *ov02a10 = to_ov02a10(sd); > > + struct v4l2_mbus_framefmt *mbus_fmt = &fmt->format; > > + > > + mutex_lock(&ov02a10->mutex); > > + > > + fmt->format = ov02a10->fmt; > > Ditto. > > > + mbus_fmt->code = ov02a10->fmt.code; > > + ov02a10_fill_fmt(ov02a10->cur_mode, mbus_fmt); > > + > > + mutex_unlock(&ov02a10->mutex); > > + > > + return 0; > > +} > > + > > +static int ov02a10_enum_mbus_code(struct v4l2_subdev *sd, > > + struct v4l2_subdev_pad_config *cfg, > > + struct v4l2_subdev_mbus_code_enum *code) > > +{ > > + struct ov02a10 *ov02a10 = to_ov02a10(sd); > > + > > + if (code->index >= ARRAY_SIZE(supported_modes)) > > + return -EINVAL; > > Hmm, supported_modes[] doesn't seem to hold the information about mbus > codes. Should this just perhaps be "!= 0"? > Understood. > > + > > + code->code = ov02a10->fmt.code; > > + > > + return 0; > > +} > [snip] > > +static int ov02a10_entity_init_cfg(struct v4l2_subdev *sd, > > + struct v4l2_subdev_pad_config *cfg) > > +{ > > + struct v4l2_subdev_format fmt = { > > + .which = cfg ? V4L2_SUBDEV_FORMAT_TRY : V4L2_SUBDEV_FORMAT_ACTIVE, > > + .format = { > > + .width = 1600, > > + .height = 1200, > > + } > > + }; > > + > > + ov02a10_set_fmt(sd, cfg, &fmt); > > + > > + return 0; > > +} > > + > > I'm not familiar with this init_cfg operation and the documentation is very > sparse about it. Sakari, is this a correct implementation? > > [snip] > > +static int ov02a10_set_test_pattern(struct ov02a10 *ov02a10, int pattern) > > +{ > > + struct i2c_client *client = v4l2_get_subdevdata(&ov02a10->subdev); > > + int ret; > > + > > + ret = i2c_smbus_write_byte_data(client, REG_PAGE_SWITCH, REG_ENABLE); > > + if (ret < 0) > > + return ret; > > + > > + ret = i2c_smbus_write_byte_data(client, OV02A10_REG_TEST_PATTERN, > > + pattern); > > + if (ret < 0) > > + return ret; > > + > > + ret = i2c_smbus_write_byte_data(client, REG_GLOBAL_EFFECTIVE, > > + REG_ENABLE); > > + if (ret < 0) > > + return ret; > > + > > + return i2c_smbus_write_byte_data(client, REG_SC_CTRL_MODE, > > + SC_CTRL_MODE_STREAMING); > > Why is this needed? Does writing the test pattern register stop streaming? > Looking back to the setting history, I found it was suggested by OV. I would leave your question to OV, and update their feedback. > [snip] > > +static int ov02a10_initialize_controls(struct ov02a10 *ov02a10) > > +{ > > + struct i2c_client *client = v4l2_get_subdevdata(&ov02a10->subdev); > > + const struct ov02a10_mode *mode; > > + struct v4l2_ctrl_handler *handler; > > + struct v4l2_ctrl *ctrl; > > + u64 exposure_max; > > + u32 pixel_rate; > > + int ret; > > + > > + handler = &ov02a10->ctrl_handler; > > + mode = ov02a10->cur_mode; > > + ret = v4l2_ctrl_handler_init(handler, 7); > > + if (ret) > > + return ret; > > + > > + handler->lock = &ov02a10->mutex; > > + > > + ctrl = v4l2_ctrl_new_int_menu(handler, NULL, V4L2_CID_LINK_FREQ, 0, 0, > > + link_freq_menu_items); > > + if (ctrl) > > + ctrl->flags |= V4L2_CTRL_FLAG_READ_ONLY; > > + > > + pixel_rate = to_pixel_rate(0); > > + v4l2_ctrl_new_std(handler, NULL, V4L2_CID_PIXEL_RATE, 0, pixel_rate, 1, > > + pixel_rate); > > + > > + exposure_max = mode->vts_def - 4; > > + ov02a10->exposure = v4l2_ctrl_new_std(handler, &ov02a10_ctrl_ops, > > + V4L2_CID_EXPOSURE, > > + OV02A10_EXPOSURE_MIN, > > + exposure_max, > > + OV02A10_EXPOSURE_STEP, > > + mode->exp_def); > > + > > + v4l2_ctrl_new_std(handler, &ov02a10_ctrl_ops, > > + V4L2_CID_ANALOGUE_GAIN, > > + OV02A10_GAIN_MIN, > > + OV02A10_GAIN_MAX, > > + OV02A10_GAIN_STEP, > > + OV02A10_GAIN_DEFAULT); > > + > > + v4l2_ctrl_new_std_menu_items(handler, &ov02a10_ctrl_ops, > > + V4L2_CID_TEST_PATTERN, > > + ARRAY_SIZE(ov02a10_test_pattern_menu) - 1, > > + 0, 0, ov02a10_test_pattern_menu); > > + > > I can see that we're missing some controls here now, VBLANK and HBLANK if I > remember correctly. Even though read-only, some userspace need those to > get information about how the sensor operates. > Yes. I made a mistake. > > + if (handler->error) { > > + ret = handler->error; > > + dev_err(&client->dev, "failed to init controls(%d)\n", ret); > > + goto err_free_handler; > > + } > > + > > + ov02a10->subdev.ctrl_handler = handler; > > + > > + return 0; > > + > > +err_free_handler: > > + v4l2_ctrl_handler_free(handler); > > + > > + return ret; > > +} > [snip] > > + pm_runtime_enable(dev); > > + if (!pm_runtime_enabled(dev)) { > > + ret = ov02a10_power_on(dev); > > + if (ret < 0) { > > + dev_err(dev, "failed to power on: %d\n", ret); > > + goto err_free_handler; > > + } > > + } > > + > > + ret = v4l2_async_register_subdev(&ov02a10->subdev); > > + if (ret) { > > + dev_err(dev, "failed to register V4L2 subdev: %d", ret); > > + if (!pm_runtime_enabled(dev)) > > + ov02a10_power_off(dev); > > Please don't mix inline and error-path error handling, as it makes it > difficult to tell if it's correct. Please move this below the appropriate > err label instead. > Fixed in next release. > > + goto err_clean_entity; > > + } > > + > > + return 0; > > + > > +err_clean_entity: > > If one calls pm_runtime_enable() in the probe path, one needs to call > pm_runtime_disable() on the error and remove paths. > Yes, fixed in next release. > > + media_entity_cleanup(&ov02a10->subdev.entity); > > +err_free_handler: > > + v4l2_ctrl_handler_free(ov02a10->subdev.ctrl_handler); > > +err_destroy_mutex: > > + mutex_destroy(&ov02a10->mutex); > > + > > + return ret; > > +} > > + > > Best regards, > Tomasz _______________________________________________ Linux-mediatek mailing list Linux-mediatek@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-mediatek From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-8.2 required=3.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED, DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH,MAILING_LIST_MULTI, SIGNED_OFF_BY,SPF_HELO_NONE,SPF_PASS,UNPARSEABLE_RELAY,USER_AGENT_SANE_2 autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id F0D96C433DF for ; Fri, 12 Jun 2020 10:49:14 +0000 (UTC) Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id C27CA20792 for ; Fri, 12 Jun 2020 10:49:14 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=lists.infradead.org header.i=@lists.infradead.org header.b="jF+LiO4h"; dkim=fail reason="signature verification failed" (1024-bit key) header.d=mediatek.com header.i=@mediatek.com header.b="dXdcdZjV" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org C27CA20792 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=mediatek.com Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-arm-kernel-bounces+infradead-linux-arm-kernel=archiver.kernel.org@lists.infradead.org DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20170209; h=Sender: Content-Transfer-Encoding:Content-Type:Cc:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:MIME-Version:References:In-Reply-To: Date:To:From:Subject:Message-ID:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=i8rfdc73qGMHhaVCaYiglWeYBpqYIfTs/EiJnMf2h7g=; b=jF+LiO4hmByN/6 QXYVhljkVIWCqLzm5Uwib5QNOeRUvzOfuYcKKpzxP+2uIy3URMoO8f7SEE2vdIiwfTwaDHFHGnPmn nLDgnoypWbZLLaYzOiUpngF21Ijdua5XVqt0HhMXMYqOBrL7Iimf5r/D6d9WdmO4Meu8LrBF2EmP7 Xs15LaJPA3A7kepDCk8Ntj0grZPzE29ihx0tcW7xhl5/V1wRR6S62RvMZ8WQrxJkSLTIY3gPkoUlK ZiuA3Kz6Xw3OAaewO8vNIDmGsSDcN2iViAgSoyPj+xpCOaLwX6r0WubHSxgFqIwRUdFcHzCh2LaE4 O73Qo1c2baul67Re3qtQ==; Received: from localhost ([127.0.0.1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.92.3 #3 (Red Hat Linux)) id 1jjhFK-0005iT-Aw; Fri, 12 Jun 2020 10:49:14 +0000 Received: from mailgw02.mediatek.com ([216.200.240.185]) by bombadil.infradead.org with esmtps (Exim 4.92.3 #3 (Red Hat Linux)) id 1jjhFG-0005gv-1D; Fri, 12 Jun 2020 10:49:11 +0000 X-UUID: 10fb0c88711841498051ee63350ed78f-20200612 DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=mediatek.com; s=dk; h=Content-Transfer-Encoding:MIME-Version:Content-Type:References:In-Reply-To:Date:CC:To:From:Subject:Message-ID; bh=TpVWyGTnlu5KQ1bLwhmMEtCgBHatHCR86J3sG18+1ro=; b=dXdcdZjV4l04zAUJl990F6csVmdi6DOXav/hW5FUPP9zEBf7vgAbvA92tdGUm2xyDCtaf9ZstwmUA51y3KcPvdPJpJ9ClhaSPIxexVNDLDAhbM4kD8GNoATk0E7UvIca8Y7bR2qKjBiRQOt6XqAgEZIJCkbzexEMxYl+905iDYg=; X-UUID: 10fb0c88711841498051ee63350ed78f-20200612 Received: from mtkcas66.mediatek.inc [(172.29.193.44)] by mailgw02.mediatek.com (envelope-from ) (musrelay.mediatek.com ESMTP with TLS) with ESMTP id 1074978896; Fri, 12 Jun 2020 02:48:55 -0800 Received: from MTKMBS31DR.mediatek.inc (172.27.6.102) by MTKMBS62N1.mediatek.inc (172.29.193.41) with Microsoft SMTP Server (TLS) id 15.0.1497.2; Fri, 12 Jun 2020 03:48:59 -0700 Received: from MTKCAS32.mediatek.inc (172.27.4.184) by MTKMBS31DR.mediatek.inc (172.27.6.102) with Microsoft SMTP Server (TLS) id 15.0.1497.2; Fri, 12 Jun 2020 18:48:57 +0800 Received: from [10.17.3.153] (10.17.3.153) by MTKCAS32.mediatek.inc (172.27.4.170) with Microsoft SMTP Server id 15.0.1497.2 via Frontend Transport; Fri, 12 Jun 2020 18:48:55 +0800 Message-ID: <1591958798.8804.660.camel@mhfsdcap03> Subject: Re: [V9, 2/2] media: i2c: ov02a10: Add OV02A10 image sensor driver From: Dongchun Zhu To: Tomasz Figa Date: Fri, 12 Jun 2020 18:46:38 +0800 In-Reply-To: <20200610194455.GK201868@chromium.org> References: <20200523084103.31276-1-dongchun.zhu@mediatek.com> <20200523084103.31276-3-dongchun.zhu@mediatek.com> <20200610194455.GK201868@chromium.org> X-Mailer: Evolution 3.10.4-0ubuntu2 MIME-Version: 1.0 X-TM-SNTS-SMTP: F9B070F852F4BB7A908158EEA1657918D95D6AE037CA2AD2A6675516E8D3B9A92000:8 X-MTK: N X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20200612_034910_083628_DE9A8D33 X-CRM114-Status: GOOD ( 30.64 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: mark.rutland@arm.com, drinkcat@chromium.org, andriy.shevchenko@linux.intel.com, srv_heupstream@mediatek.com, devicetree@vger.kernel.org, linus.walleij@linaro.org, shengnan.wang@mediatek.com, louis.kuo@mediatek.com, bgolaszewski@baylibre.com, sj.huang@mediatek.com, robh+dt@kernel.org, linux-mediatek@lists.infradead.org, dongchun.zhu@mediatek.com, sakari.ailus@linux.intel.com, matthias.bgg@gmail.com, bingbu.cao@intel.com, mchehab@kernel.org, linux-arm-kernel@lists.infradead.org, linux-media@vger.kernel.org Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+infradead-linux-arm-kernel=archiver.kernel.org@lists.infradead.org Hi Tomasz, On Wed, 2020-06-10 at 19:44 +0000, Tomasz Figa wrote: > Hi Dongchun, > > On Sat, May 23, 2020 at 04:41:03PM +0800, Dongchun Zhu wrote: > > Add a V4L2 sub-device driver for OV02A10 image sensor. > > > > Signed-off-by: Dongchun Zhu > > --- > > MAINTAINERS | 1 + > > drivers/media/i2c/Kconfig | 13 + > > drivers/media/i2c/Makefile | 1 + > > drivers/media/i2c/ov02a10.c | 1025 +++++++++++++++++++++++++++++++++++++++++++ > > 4 files changed, 1040 insertions(+) > > create mode 100644 drivers/media/i2c/ov02a10.c > > > > Thank you for the patch. Please see my comments inline. > > [snip] > > diff --git a/drivers/media/i2c/ov02a10.c b/drivers/media/i2c/ov02a10.c > > new file mode 100644 > > index 0000000..160a0b5 > > --- /dev/null > > +++ b/drivers/media/i2c/ov02a10.c > [snip] > > +static const char * const ov02a10_test_pattern_menu[] = { > > + "Disabled", > > + "Color Bar", > > nit: We should normalize this to one of the standard names. What is the > pattern on this sensor? Is it perhaps "Eight Vertical Colour Bars"? > Yes. It is one kind of 'Eight Vertical Colour Bars'. This pattern is called as 'MIPI color bar' per the datasheet. Can we here use 'Vertical Color Bar' or 'MIPI Color Bar'? > > +}; > [snip] > > +static int ov02a10_set_fmt(struct v4l2_subdev *sd, > > + struct v4l2_subdev_pad_config *cfg, > > + struct v4l2_subdev_format *fmt) > > +{ > > + struct ov02a10 *ov02a10 = to_ov02a10(sd); > > + struct v4l2_mbus_framefmt *mbus_fmt = &fmt->format; > > + > > + mutex_lock(&ov02a10->mutex); > > + > > > Don't we need to handle the case when fmt->which is V4L2_SUBDEV_FORMAT_TRY, > which is used for trying the format, but not applying it to the hardware? > Got it :-) > > + if (ov02a10->streaming) { > > + mutex_unlock(&ov02a10->mutex); > > + return -EBUSY; > > + } > > + > > + /* Only one sensor mode supported */ > > + mbus_fmt->code = ov02a10->fmt.code; > > + ov02a10_fill_fmt(ov02a10->cur_mode, mbus_fmt); > > + ov02a10->fmt = fmt->format; > > + > > + mutex_unlock(&ov02a10->mutex); > > + > > + return 0; > > +} > > + > > +static int ov02a10_get_fmt(struct v4l2_subdev *sd, > > + struct v4l2_subdev_pad_config *cfg, > > + struct v4l2_subdev_format *fmt) > > +{ > > + struct ov02a10 *ov02a10 = to_ov02a10(sd); > > + struct v4l2_mbus_framefmt *mbus_fmt = &fmt->format; > > + > > + mutex_lock(&ov02a10->mutex); > > + > > + fmt->format = ov02a10->fmt; > > Ditto. > > > + mbus_fmt->code = ov02a10->fmt.code; > > + ov02a10_fill_fmt(ov02a10->cur_mode, mbus_fmt); > > + > > + mutex_unlock(&ov02a10->mutex); > > + > > + return 0; > > +} > > + > > +static int ov02a10_enum_mbus_code(struct v4l2_subdev *sd, > > + struct v4l2_subdev_pad_config *cfg, > > + struct v4l2_subdev_mbus_code_enum *code) > > +{ > > + struct ov02a10 *ov02a10 = to_ov02a10(sd); > > + > > + if (code->index >= ARRAY_SIZE(supported_modes)) > > + return -EINVAL; > > Hmm, supported_modes[] doesn't seem to hold the information about mbus > codes. Should this just perhaps be "!= 0"? > Understood. > > + > > + code->code = ov02a10->fmt.code; > > + > > + return 0; > > +} > [snip] > > +static int ov02a10_entity_init_cfg(struct v4l2_subdev *sd, > > + struct v4l2_subdev_pad_config *cfg) > > +{ > > + struct v4l2_subdev_format fmt = { > > + .which = cfg ? V4L2_SUBDEV_FORMAT_TRY : V4L2_SUBDEV_FORMAT_ACTIVE, > > + .format = { > > + .width = 1600, > > + .height = 1200, > > + } > > + }; > > + > > + ov02a10_set_fmt(sd, cfg, &fmt); > > + > > + return 0; > > +} > > + > > I'm not familiar with this init_cfg operation and the documentation is very > sparse about it. Sakari, is this a correct implementation? > > [snip] > > +static int ov02a10_set_test_pattern(struct ov02a10 *ov02a10, int pattern) > > +{ > > + struct i2c_client *client = v4l2_get_subdevdata(&ov02a10->subdev); > > + int ret; > > + > > + ret = i2c_smbus_write_byte_data(client, REG_PAGE_SWITCH, REG_ENABLE); > > + if (ret < 0) > > + return ret; > > + > > + ret = i2c_smbus_write_byte_data(client, OV02A10_REG_TEST_PATTERN, > > + pattern); > > + if (ret < 0) > > + return ret; > > + > > + ret = i2c_smbus_write_byte_data(client, REG_GLOBAL_EFFECTIVE, > > + REG_ENABLE); > > + if (ret < 0) > > + return ret; > > + > > + return i2c_smbus_write_byte_data(client, REG_SC_CTRL_MODE, > > + SC_CTRL_MODE_STREAMING); > > Why is this needed? Does writing the test pattern register stop streaming? > Looking back to the setting history, I found it was suggested by OV. I would leave your question to OV, and update their feedback. > [snip] > > +static int ov02a10_initialize_controls(struct ov02a10 *ov02a10) > > +{ > > + struct i2c_client *client = v4l2_get_subdevdata(&ov02a10->subdev); > > + const struct ov02a10_mode *mode; > > + struct v4l2_ctrl_handler *handler; > > + struct v4l2_ctrl *ctrl; > > + u64 exposure_max; > > + u32 pixel_rate; > > + int ret; > > + > > + handler = &ov02a10->ctrl_handler; > > + mode = ov02a10->cur_mode; > > + ret = v4l2_ctrl_handler_init(handler, 7); > > + if (ret) > > + return ret; > > + > > + handler->lock = &ov02a10->mutex; > > + > > + ctrl = v4l2_ctrl_new_int_menu(handler, NULL, V4L2_CID_LINK_FREQ, 0, 0, > > + link_freq_menu_items); > > + if (ctrl) > > + ctrl->flags |= V4L2_CTRL_FLAG_READ_ONLY; > > + > > + pixel_rate = to_pixel_rate(0); > > + v4l2_ctrl_new_std(handler, NULL, V4L2_CID_PIXEL_RATE, 0, pixel_rate, 1, > > + pixel_rate); > > + > > + exposure_max = mode->vts_def - 4; > > + ov02a10->exposure = v4l2_ctrl_new_std(handler, &ov02a10_ctrl_ops, > > + V4L2_CID_EXPOSURE, > > + OV02A10_EXPOSURE_MIN, > > + exposure_max, > > + OV02A10_EXPOSURE_STEP, > > + mode->exp_def); > > + > > + v4l2_ctrl_new_std(handler, &ov02a10_ctrl_ops, > > + V4L2_CID_ANALOGUE_GAIN, > > + OV02A10_GAIN_MIN, > > + OV02A10_GAIN_MAX, > > + OV02A10_GAIN_STEP, > > + OV02A10_GAIN_DEFAULT); > > + > > + v4l2_ctrl_new_std_menu_items(handler, &ov02a10_ctrl_ops, > > + V4L2_CID_TEST_PATTERN, > > + ARRAY_SIZE(ov02a10_test_pattern_menu) - 1, > > + 0, 0, ov02a10_test_pattern_menu); > > + > > I can see that we're missing some controls here now, VBLANK and HBLANK if I > remember correctly. Even though read-only, some userspace need those to > get information about how the sensor operates. > Yes. I made a mistake. > > + if (handler->error) { > > + ret = handler->error; > > + dev_err(&client->dev, "failed to init controls(%d)\n", ret); > > + goto err_free_handler; > > + } > > + > > + ov02a10->subdev.ctrl_handler = handler; > > + > > + return 0; > > + > > +err_free_handler: > > + v4l2_ctrl_handler_free(handler); > > + > > + return ret; > > +} > [snip] > > + pm_runtime_enable(dev); > > + if (!pm_runtime_enabled(dev)) { > > + ret = ov02a10_power_on(dev); > > + if (ret < 0) { > > + dev_err(dev, "failed to power on: %d\n", ret); > > + goto err_free_handler; > > + } > > + } > > + > > + ret = v4l2_async_register_subdev(&ov02a10->subdev); > > + if (ret) { > > + dev_err(dev, "failed to register V4L2 subdev: %d", ret); > > + if (!pm_runtime_enabled(dev)) > > + ov02a10_power_off(dev); > > Please don't mix inline and error-path error handling, as it makes it > difficult to tell if it's correct. Please move this below the appropriate > err label instead. > Fixed in next release. > > + goto err_clean_entity; > > + } > > + > > + return 0; > > + > > +err_clean_entity: > > If one calls pm_runtime_enable() in the probe path, one needs to call > pm_runtime_disable() on the error and remove paths. > Yes, fixed in next release. > > + media_entity_cleanup(&ov02a10->subdev.entity); > > +err_free_handler: > > + v4l2_ctrl_handler_free(ov02a10->subdev.ctrl_handler); > > +err_destroy_mutex: > > + mutex_destroy(&ov02a10->mutex); > > + > > + return ret; > > +} > > + > > Best regards, > Tomasz _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel