From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752167AbcHBHfx (ORCPT ); Tue, 2 Aug 2016 03:35:53 -0400 Received: from lb3-smtp-cloud2.xs4all.net ([194.109.24.29]:40363 "EHLO lb3-smtp-cloud2.xs4all.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750963AbcHBHfo (ORCPT ); Tue, 2 Aug 2016 03:35:44 -0400 Subject: Re: [PATCH v7 1/2] [media] atmel-isc: add the Image Sensor Controller code To: "Wu, Songjun" , nicolas.ferre@atmel.com References: <1469778856-24253-1-git-send-email-songjun.wu@microchip.com> <1469778856-24253-2-git-send-email-songjun.wu@microchip.com> Cc: robh@kernel.org, laurent.pinchart@ideasonboard.com, linux-arm-kernel@lists.infradead.org, linux-media@vger.kernel.org, Arnd Bergmann , =?UTF-8?Q?Niklas_S=c3=83=c2=b6derlund?= , Benoit Parrot , linux-kernel@vger.kernel.org, Andrew-CT Chen , Sudip Mukherjee , Kamil Debski , Tiffany Lin , Peter Griffin , Geert Uytterhoeven , Mikhail Ulyanov , =?UTF-8?Q?Richard_R=c3=b6jfors?= , Hans Verkuil , Laurent Pinchart , Simon Horman From: Hans Verkuil Message-ID: Date: Tue, 2 Aug 2016 09:32:44 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Icedove/45.2.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 08/02/2016 08:20 AM, Wu, Songjun wrote: >>> +static unsigned int sensor_preferred = 1; >>> +module_param(sensor_preferred, uint, S_IRUGO|S_IWUSR); >>> +MODULE_PARM_DESC(sensor_preferred, >>> + "Sensor is preferred to output the specified format (1-on 0-off) default 1"); >> >> I have no idea what this means. Can you elaborate? Why would you want to set this to 0? >> > ISC can convert the raw format to the other format, e.g. YUYV. > If we want to output YUYV format, there are two choices, one is the > sensor output YUYV format, ISC bypass the data to the memory, the other > is the sensor output raw format, ISC convert raw format to YUYV. > > So I provide a module parameter to user to select. > I prefer to select the sensor to output the specified format, then I set > this parameter to '1', not '0'. Does this only apply to YUYV? The reason I am hesitant about this option is that I am not convinced you need it. The default (sensor preferred) makes sense and that's what other drivers do as well. Unless you know of a real use-case where you want to set this to 0, I would just drop this option. If there *is* a real use-case, then split off adding this module option into a separate patch so we can discuss it more without blocking getting this driver into mainline. I don't like the way this is done here. Regards, Hans From mboxrd@z Thu Jan 1 00:00:00 1970 From: hverkuil@xs4all.nl (Hans Verkuil) Date: Tue, 2 Aug 2016 09:32:44 +0200 Subject: [PATCH v7 1/2] [media] atmel-isc: add the Image Sensor Controller code In-Reply-To: References: <1469778856-24253-1-git-send-email-songjun.wu@microchip.com> <1469778856-24253-2-git-send-email-songjun.wu@microchip.com> Message-ID: To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org On 08/02/2016 08:20 AM, Wu, Songjun wrote: >>> +static unsigned int sensor_preferred = 1; >>> +module_param(sensor_preferred, uint, S_IRUGO|S_IWUSR); >>> +MODULE_PARM_DESC(sensor_preferred, >>> + "Sensor is preferred to output the specified format (1-on 0-off) default 1"); >> >> I have no idea what this means. Can you elaborate? Why would you want to set this to 0? >> > ISC can convert the raw format to the other format, e.g. YUYV. > If we want to output YUYV format, there are two choices, one is the > sensor output YUYV format, ISC bypass the data to the memory, the other > is the sensor output raw format, ISC convert raw format to YUYV. > > So I provide a module parameter to user to select. > I prefer to select the sensor to output the specified format, then I set > this parameter to '1', not '0'. Does this only apply to YUYV? The reason I am hesitant about this option is that I am not convinced you need it. The default (sensor preferred) makes sense and that's what other drivers do as well. Unless you know of a real use-case where you want to set this to 0, I would just drop this option. If there *is* a real use-case, then split off adding this module option into a separate patch so we can discuss it more without blocking getting this driver into mainline. I don't like the way this is done here. Regards, Hans