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 Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 7C2D7C433F5 for ; Mon, 10 Oct 2022 09:19:15 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S230230AbiJJJTO (ORCPT ); Mon, 10 Oct 2022 05:19:14 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:33242 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S230201AbiJJJTN (ORCPT ); Mon, 10 Oct 2022 05:19:13 -0400 Received: from mx07-00178001.pphosted.com (mx08-00178001.pphosted.com [91.207.212.93]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 213594A801 for ; Mon, 10 Oct 2022 02:19:11 -0700 (PDT) Received: from pps.filterd (m0046661.ppops.net [127.0.0.1]) by mx07-00178001.pphosted.com (8.17.1.5/8.17.1.5) with ESMTP id 29A78g4w025478; Mon, 10 Oct 2022 11:19:05 +0200 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=foss.st.com; h=message-id : date : mime-version : subject : to : cc : references : from : in-reply-to : content-type : content-transfer-encoding; s=selector1; bh=Uethha4ISFWphstG0Le66mJfCfQpr/gJjzQTdicQID8=; b=Y3NyhtIG8ltUygWB1uz29QtEfzEa9FdWpN4zv+y99QIihaoDci/ULXX/wGBVckaG27iD GXRIuVua9+DzoSr+qbWAh/mHl9ip84FKYRj89Me8FJg2D/LhYX8a+8TQjdpaA9h40hoy wDuRxygNTKNAibINfykUD7oT+6LsLJM5cGkqTZpD/8qx6UJqIBCrWwaQuMI3/TyEDAlU cBw3X2PIDdHv91WfWqOTDYBYns9Ltm3q4nUf/YUft8xb04Ar5NQoFfgNxfqMXjeDkJ2v WQMAKKoq16wmdA48wgJQ6f2A1h8/z/idqleXLoOnsSEMaBjmQhVrRBI1tRpXWzygX+Qj lg== Received: from beta.dmz-eu.st.com (beta.dmz-eu.st.com [164.129.1.35]) by mx07-00178001.pphosted.com (PPS) with ESMTPS id 3k31gpjrb9-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Mon, 10 Oct 2022 11:19:05 +0200 Received: from euls16034.sgp.st.com (euls16034.sgp.st.com [10.75.44.20]) by beta.dmz-eu.st.com (STMicroelectronics) with ESMTP id AA7F410003B; Mon, 10 Oct 2022 11:19:01 +0200 (CEST) Received: from Webmail-eu.st.com (shfdag1node1.st.com [10.75.129.69]) by euls16034.sgp.st.com (STMicroelectronics) with ESMTP id 97E8B2194D5; Mon, 10 Oct 2022 11:19:01 +0200 (CEST) Received: from [10.129.166.189] (10.75.127.117) by SHFDAG1NODE1.st.com (10.75.129.69) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256) id 15.1.2375.31; Mon, 10 Oct 2022 11:19:01 +0200 Message-ID: <7c36fdaf-fa2a-4581-5f16-02fbfc3b20fb@foss.st.com> Date: Mon, 10 Oct 2022 11:19:01 +0200 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.11.0 Subject: Re: [PATCH v6 4/4] media: i2c: Add driver for ST VGXY61 camera sensor Content-Language: en-US To: Sakari Ailus CC: , , , , , , , , References: <20220927083702.14138-1-benjamin.mugnier@foss.st.com> <20220927083702.14138-5-benjamin.mugnier@foss.st.com> From: Benjamin MUGNIER In-Reply-To: Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: 7bit X-Originating-IP: [10.75.127.117] X-ClientProxiedBy: GPXDAG2NODE4.st.com (10.75.127.68) To SHFDAG1NODE1.st.com (10.75.129.69) X-Proofpoint-Virus-Version: vendor=baseguard engine=ICAP:2.0.205,Aquarius:18.0.895,Hydra:6.0.528,FMLib:17.11.122.1 definitions=2022-10-07_04,2022-10-07_01,2022-06-22_01 Precedence: bulk List-ID: X-Mailing-List: linux-media@vger.kernel.org Hi Sakari, On 10/10/22 10:29, Sakari Ailus wrote: > Hi Benjamin, > > On Fri, Oct 07, 2022 at 02:24:16PM +0200, Benjamin MUGNIER wrote: >> Hi Sakari, >> >> Thank you for your review. >> Please consider everything not commented as queued for v7. >> >> On 10/6/22 21:14, Sakari Ailus wrote: >>> Hi Benjamin, >>> >>> Thanks for the update. A few more comments below... >>> >>> On Tue, Sep 27, 2022 at 10:37:02AM +0200, Benjamin Mugnier wrote: >>>> +static const char * const vgxy61_hdr_mode_menu[] = { >>>> + "HDR linearize", >>>> + "HDR substraction", >>>> + "No HDR", >>>> +}; >>> >>> I think more documentation is needed on the HDR modes. What do these mean? >>> For instance, are they something that requires further processing or is the >>> result e.g. a ready HDR image? >>> >>> This should probably make it to driver specific documentation. >>> >> >> Sure, in fact I did something like this in v3: >> https://lore.kernel.org/linux-media/20220512074037.3829926-4-benjamin.mugnier@foss.st.com/ >> >> Since I standardized the control I was unsure what to do with this documentation, and dropped it. >> I will add back the Documentation/userspace-api/media/drivers/st-vgxy61.rst file from this commit to the patchset, while changing the control name to the new one. > > Yes, please. That seems reasonably good. > > I think Laurent's proposal on a HDR control for sensor-implemented HDR is a > good one. Further controls can be added later on. > >>>> + sensor->gpios_polarity = of_property_read_bool(dev->of_node, >>>> + "invert-gpios-polarity"); > > How about: > > device_property_read_bool(dev, ...); > Sounds great. >>> >>> I thought we did discuss dropping support for sensor synchronisation in >>> this version? >>> >> >> This properties affects strobing lights gpios polarities as you can see >> in vgxy61_update_gpios_strobe_polarity. If set to '1' all strobing gpios >> are inverted. This has nothing to do with the sensor synchronization. > > So this is for strobing a LED flash? It would be good to mention this in > DT bindings. > >> >> Now I realize this is poorly named, and I even forgot to document it in >> the device tree bindings file. I apologize. > > No problem. > >> >> I would like to rename it to 'st,strobe-polarity' since this is vendor >> specific and to better reflect that it affects strobing gpios. I'll make >> this change for v7 and document this in the bindings file too. Tell me if >> there is any issues with that. > > That name seems reasonable to me. Although, *if* this is actually usable as > a GPIO as the bindings suggest, then the GPIO flags would probably be a > better alternative. > If by GPIO flag you mean adding 'gpios' to the property, We could go with 'st,strobe-gpios-polarity', which in the end this leads to the same property name as it was in the dt bindings :) I'll add a bit of comments on the bindings. It seems to be the best choice. Thank you. -- Regards, Benjamin