Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [PATCH v2 3/5] media: mt9p031: Implement [gs]_register debug
 calls
To:     Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc:     Mauro Carvalho Chehab <mchehab@kernel.org>,
        Sakari Ailus <sakari.ailus@linux.intel.com>,
        linux-media@vger.kernel.org, linux-kernel@vger.kernel.org,
        Enrico Scholz <enrico.scholz@sigma-chemnitz.de>
References: <20200930105133.139981-1-s.riedmueller@phytec.de>
 <20200930105133.139981-3-s.riedmueller@phytec.de>
 <20200930113831.GG5689@pendragon.ideasonboard.com>
From:   =?UTF-8?Q?Stefan_Riedm=c3=bcller?= <s.riedmueller@phytec.de>
Message-ID: <289d5897-424f-326f-0bc1-c9a1b79284c3@phytec.de>
Date:   Thu, 1 Oct 2020 10:56:24 +0200
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101
 Thunderbird/68.10.0
MIME-Version: 1.0
In-Reply-To: <20200930113831.GG5689@pendragon.ideasonboard.com>
Content-Transfer-Encoding: 7bit
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Precedence: bulk

Hi Laurent,

On 30.09.20 13:38, Laurent Pinchart wrote:
> Hi Stefan,
> 
> Thank you for the patch.
> 
> On Wed, Sep 30, 2020 at 12:51:31PM +0200, Stefan Riedmueller wrote:
>> From: Enrico Scholz <enrico.scholz@sigma-chemnitz.de>
>>
>> Implement g_register and s_register v4l2_subdev_core_ops to access
>> camera register directly from userspace for debug purposes.
> 
> As the name of the operations imply, this is meant for debug purpose
> only. They are however prone to be abused to configure the sensor from
> userspace in production, which isn't a direction we want to take.
> What's your use case for this ?  I'd rather drop this patch and see the
> driver extended with support for more controls if needed

thanks for your feedback.

I get your point. I myself solely use these operations for debugging 
purposes but I'm aware that others like to abuse them.

I thought I send it anyway since for me the DEBUG config is enough to 
signalize that these operations are not to be used with a productive system. 
But I'm OK with dropping this patch if you think it might send the wrong signal.

Regards,
Stefan

> 
>> Signed-off-by: Enrico Scholz <enrico.scholz@sigma-chemnitz.de>
>> Signed-off-by: Stefan Riedmueller <s.riedmueller@phytec.de>
>> ---
>> No changes in v2
>> ---
>>   drivers/media/i2c/mt9p031.c | 28 ++++++++++++++++++++++++++++
>>   1 file changed, 28 insertions(+)
>>
>> diff --git a/drivers/media/i2c/mt9p031.c b/drivers/media/i2c/mt9p031.c
>> index b4c042f418c1..de36025260a8 100644
>> --- a/drivers/media/i2c/mt9p031.c
>> +++ b/drivers/media/i2c/mt9p031.c
>> @@ -703,6 +703,30 @@ static int mt9p031_restore_blc(struct mt9p031 *mt9p031)
>>   	return 0;
>>   }
>>   
>> +#ifdef CONFIG_VIDEO_ADV_DEBUG
>> +static int mt9p031_g_register(struct v4l2_subdev *sd,
>> +			      struct v4l2_dbg_register *reg)
>> +{
>> +	struct i2c_client *client = v4l2_get_subdevdata(sd);
>> +	int ret;
>> +
>> +	ret = mt9p031_read(client, reg->reg);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	reg->val = ret;
>> +	return 0;
>> +}
>> +
>> +static int mt9p031_s_register(struct v4l2_subdev *sd,
>> +			      struct v4l2_dbg_register const *reg)
>> +{
>> +	struct i2c_client *client = v4l2_get_subdevdata(sd);
>> +
>> +	return mt9p031_write(client, reg->reg, reg->val);
>> +}
>> +#endif
>> +
>>   static int mt9p031_s_ctrl(struct v4l2_ctrl *ctrl)
>>   {
>>   	struct mt9p031 *mt9p031 =
>> @@ -1000,6 +1024,10 @@ static int mt9p031_close(struct v4l2_subdev *subdev, struct v4l2_subdev_fh *fh)
>>   
>>   static const struct v4l2_subdev_core_ops mt9p031_subdev_core_ops = {
>>   	.s_power        = mt9p031_set_power,
>> +#ifdef CONFIG_VIDEO_ADV_DEBUG
>> +	.s_register	= mt9p031_s_register,
>> +	.g_register	= mt9p031_g_register,
>> +#endif
>>   };
>>   
>>   static const struct v4l2_subdev_video_ops mt9p031_subdev_video_ops = {
>