* [PATCH] media: ov5640: fix virtual_channel parameter permissions
@ 2018-02-01 13:17 Hugues Fruchet
2018-02-01 16:56 ` jacopo mondi
0 siblings, 1 reply; 3+ messages in thread
From: Hugues Fruchet @ 2018-02-01 13:17 UTC (permalink / raw)
To: Steve Longerbeam, Sakari Ailus, Hans Verkuil,
Mauro Carvalho Chehab
Cc: linux-media, Hugues Fruchet, Benjamin Gaignard
Fix module_param(virtual_channel) permissions.
This problem was detected by checkpatch:
$ scripts/checkpatch.pl -f drivers/media/i2c/ov5640.c
ERROR: Use 4 digit octal (0777) not decimal permissions
#131: FILE: drivers/media/i2c/ov5640.c:131:
+module_param(virtual_channel, int, 0);
Also explicitly set initial value to 0 for default value
and add an error trace in case of virtual_channel not in
the valid range of values.
Signed-off-by: Hugues Fruchet <hugues.fruchet@st.com>
---
drivers/media/i2c/ov5640.c | 10 +++++++---
1 file changed, 7 insertions(+), 3 deletions(-)
diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
index 696a28b..906f202 100644
--- a/drivers/media/i2c/ov5640.c
+++ b/drivers/media/i2c/ov5640.c
@@ -127,8 +127,8 @@ struct ov5640_pixfmt {
* FIXME: remove this when a subdev API becomes available
* to set the MIPI CSI-2 virtual channel.
*/
-static unsigned int virtual_channel;
-module_param(virtual_channel, int, 0);
+static unsigned int virtual_channel = 0;
+module_param(virtual_channel, int, 0444);
MODULE_PARM_DESC(virtual_channel,
"MIPI CSI-2 virtual channel (0..3), default 0");
@@ -1358,11 +1358,15 @@ static int ov5640_binning_on(struct ov5640_dev *sensor)
static int ov5640_set_virtual_channel(struct ov5640_dev *sensor)
{
+ struct i2c_client *client = sensor->i2c_client;
u8 temp, channel = virtual_channel;
int ret;
- if (channel > 3)
+ if (channel > 3) {
+ dev_err(&client->dev, "%s: wrong virtual_channel parameter value, expected (0..3), got %d\n",
+ __func__, channel);
return -EINVAL;
+ }
ret = ov5640_read_reg(sensor, OV5640_REG_DEBUG_MODE, &temp);
if (ret)
--
1.9.1
^ permalink raw reply related [flat|nested] 3+ messages in thread* Re: [PATCH] media: ov5640: fix virtual_channel parameter permissions
2018-02-01 13:17 [PATCH] media: ov5640: fix virtual_channel parameter permissions Hugues Fruchet
@ 2018-02-01 16:56 ` jacopo mondi
2018-02-06 13:26 ` Hugues FRUCHET
0 siblings, 1 reply; 3+ messages in thread
From: jacopo mondi @ 2018-02-01 16:56 UTC (permalink / raw)
To: Hugues Fruchet
Cc: Steve Longerbeam, Sakari Ailus, Hans Verkuil,
Mauro Carvalho Chehab, linux-media, Benjamin Gaignard
Hi Hugues,
On Thu, Feb 01, 2018 at 02:17:34PM +0100, Hugues Fruchet wrote:
> Fix module_param(virtual_channel) permissions.
> This problem was detected by checkpatch:
> $ scripts/checkpatch.pl -f drivers/media/i2c/ov5640.c
> ERROR: Use 4 digit octal (0777) not decimal permissions
> #131: FILE: drivers/media/i2c/ov5640.c:131:
> +module_param(virtual_channel, int, 0);
>
> Also explicitly set initial value to 0 for default value
> and add an error trace in case of virtual_channel not in
> the valid range of values.
>
> Signed-off-by: Hugues Fruchet <hugues.fruchet@st.com>
> ---
> drivers/media/i2c/ov5640.c | 10 +++++++---
> 1 file changed, 7 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
> index 696a28b..906f202 100644
> --- a/drivers/media/i2c/ov5640.c
> +++ b/drivers/media/i2c/ov5640.c
> @@ -127,8 +127,8 @@ struct ov5640_pixfmt {
> * FIXME: remove this when a subdev API becomes available
> * to set the MIPI CSI-2 virtual channel.
> */
> -static unsigned int virtual_channel;
> -module_param(virtual_channel, int, 0);
> +static unsigned int virtual_channel = 0;
> +module_param(virtual_channel, int, 0444);
Parameter type is unsigned int, please use uint here.
As also checkpatch reports, it is not necessary to initialize static
variables to 0.
> MODULE_PARM_DESC(virtual_channel,
> "MIPI CSI-2 virtual channel (0..3), default 0");
>
> @@ -1358,11 +1358,15 @@ static int ov5640_binning_on(struct ov5640_dev *sensor)
>
> static int ov5640_set_virtual_channel(struct ov5640_dev *sensor)
> {
> + struct i2c_client *client = sensor->i2c_client;
> u8 temp, channel = virtual_channel;
> int ret;
>
> - if (channel > 3)
> + if (channel > 3) {
> + dev_err(&client->dev, "%s: wrong virtual_channel parameter value, expected (0..3), got %d\n",
I understand you don't want to break error messages to 80 columns to
ease grep for errors, but you can break the line after "client->dev"
to make this less painful for the eyes.
Thanks
j
> + __func__, channel);
> return -EINVAL;
> + }
>
> ret = ov5640_read_reg(sensor, OV5640_REG_DEBUG_MODE, &temp);
> if (ret)
> --
> 1.9.1
>
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [PATCH] media: ov5640: fix virtual_channel parameter permissions
2018-02-01 16:56 ` jacopo mondi
@ 2018-02-06 13:26 ` Hugues FRUCHET
0 siblings, 0 replies; 3+ messages in thread
From: Hugues FRUCHET @ 2018-02-06 13:26 UTC (permalink / raw)
To: jacopo mondi
Cc: Steve Longerbeam, Sakari Ailus, Hans Verkuil,
Mauro Carvalho Chehab, linux-media@vger.kernel.org,
Benjamin Gaignard
Hi Jacopo,
Thanks for review, v2 sent !
BR,
Hugues.
On 02/01/2018 05:56 PM, jacopo mondi wrote:
> Hi Hugues,
>
> On Thu, Feb 01, 2018 at 02:17:34PM +0100, Hugues Fruchet wrote:
>> Fix module_param(virtual_channel) permissions.
>> This problem was detected by checkpatch:
>> $ scripts/checkpatch.pl -f drivers/media/i2c/ov5640.c
>> ERROR: Use 4 digit octal (0777) not decimal permissions
>> #131: FILE: drivers/media/i2c/ov5640.c:131:
>> +module_param(virtual_channel, int, 0);
>>
>> Also explicitly set initial value to 0 for default value
>> and add an error trace in case of virtual_channel not in
>> the valid range of values.
>>
>> Signed-off-by: Hugues Fruchet <hugues.fruchet@st.com>
>> ---
>> drivers/media/i2c/ov5640.c | 10 +++++++---
>> 1 file changed, 7 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
>> index 696a28b..906f202 100644
>> --- a/drivers/media/i2c/ov5640.c
>> +++ b/drivers/media/i2c/ov5640.c
>> @@ -127,8 +127,8 @@ struct ov5640_pixfmt {
>> * FIXME: remove this when a subdev API becomes available
>> * to set the MIPI CSI-2 virtual channel.
>> */
>> -static unsigned int virtual_channel;
>> -module_param(virtual_channel, int, 0);
>> +static unsigned int virtual_channel = 0;
>> +module_param(virtual_channel, int, 0444);
>
> Parameter type is unsigned int, please use uint here.
> As also checkpatch reports, it is not necessary to initialize static
> variables to 0.
>
>> MODULE_PARM_DESC(virtual_channel,
>> "MIPI CSI-2 virtual channel (0..3), default 0");
>>
>> @@ -1358,11 +1358,15 @@ static int ov5640_binning_on(struct ov5640_dev *sensor)
>>
>> static int ov5640_set_virtual_channel(struct ov5640_dev *sensor)
>> {
>> + struct i2c_client *client = sensor->i2c_client;
>> u8 temp, channel = virtual_channel;
>> int ret;
>>
>> - if (channel > 3)
>> + if (channel > 3) {
>> + dev_err(&client->dev, "%s: wrong virtual_channel parameter value, expected (0..3), got %d\n",
>
> I understand you don't want to break error messages to 80 columns to
> ease grep for errors, but you can break the line after "client->dev"
> to make this less painful for the eyes.
>
> Thanks
> j
>
>> + __func__, channel);
>> return -EINVAL;
>> + }
>>
>> ret = ov5640_read_reg(sensor, OV5640_REG_DEBUG_MODE, &temp);
>> if (ret)
>> --
>> 1.9.1
>>
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2018-02-06 13:26 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2018-02-01 13:17 [PATCH] media: ov5640: fix virtual_channel parameter permissions Hugues Fruchet
2018-02-01 16:56 ` jacopo mondi
2018-02-06 13:26 ` Hugues FRUCHET
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).