`false` was used as the keep_bitstream_buffers parameter permissions.
This looks more like a default value for the parameter, so change it to
0 to avoid confusion.
Reviewed-by: Daniel Almeida <[email protected]>
Signed-off-by: Detlev Casanova <[email protected]>
---
drivers/media/test-drivers/visl/visl-core.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/media/test-drivers/visl/visl-core.c b/drivers/media/test-drivers/visl/visl-core.c
index 9970dc739ca5..df6515530fbf 100644
--- a/drivers/media/test-drivers/visl/visl-core.c
+++ b/drivers/media/test-drivers/visl/visl-core.c
@@ -74,7 +74,7 @@ MODULE_PARM_DESC(visl_dprintk_nframes,
" the number of frames to trace with dprintk");
bool keep_bitstream_buffers;
-module_param(keep_bitstream_buffers, bool, false);
+module_param(keep_bitstream_buffers, bool, 0);
MODULE_PARM_DESC(keep_bitstream_buffers,
" keep bitstream buffers in debugfs after streaming is stopped");
--
2.41.0
On 24/10/2023 21:09, Detlev Casanova wrote:
> `false` was used as the keep_bitstream_buffers parameter permissions.
> This looks more like a default value for the parameter, so change it to
> 0 to avoid confusion.
>
> Reviewed-by: Daniel Almeida <[email protected]>
> Signed-off-by: Detlev Casanova <[email protected]>
> ---
> drivers/media/test-drivers/visl/visl-core.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/media/test-drivers/visl/visl-core.c b/drivers/media/test-drivers/visl/visl-core.c
> index 9970dc739ca5..df6515530fbf 100644
> --- a/drivers/media/test-drivers/visl/visl-core.c
> +++ b/drivers/media/test-drivers/visl/visl-core.c
> @@ -74,7 +74,7 @@ MODULE_PARM_DESC(visl_dprintk_nframes,
> " the number of frames to trace with dprintk");
>
> bool keep_bitstream_buffers;
> -module_param(keep_bitstream_buffers, bool, false);
> +module_param(keep_bitstream_buffers, bool, 0);
???
This last parameter is the permission, it makes no sense that this
is either 0 or false: then nobody can see it in /sys/modules/.
Typically this is 0444 if it is readable only, or 0644 if it can be
written by root.
Regards,
Hans
> MODULE_PARM_DESC(keep_bitstream_buffers,
> " keep bitstream buffers in debugfs after streaming is stopped");
>
On Wednesday, November 22, 2023 10:56:20 A.M. EST Hans Verkuil wrote:
> On 24/10/2023 21:09, Detlev Casanova wrote:
> > `false` was used as the keep_bitstream_buffers parameter permissions.
> > This looks more like a default value for the parameter, so change it to
> > 0 to avoid confusion.
> >
> > Reviewed-by: Daniel Almeida <[email protected]>
> > Signed-off-by: Detlev Casanova <[email protected]>
> > ---
> >
> > drivers/media/test-drivers/visl/visl-core.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/media/test-drivers/visl/visl-core.c
> > b/drivers/media/test-drivers/visl/visl-core.c index
> > 9970dc739ca5..df6515530fbf 100644
> > --- a/drivers/media/test-drivers/visl/visl-core.c
> > +++ b/drivers/media/test-drivers/visl/visl-core.c
> > @@ -74,7 +74,7 @@ MODULE_PARM_DESC(visl_dprintk_nframes,
> >
> > " the number of frames to trace with dprintk");
> >
> > bool keep_bitstream_buffers;
> >
> > -module_param(keep_bitstream_buffers, bool, false);
> > +module_param(keep_bitstream_buffers, bool, 0);
>
> ???
>
> This last parameter is the permission, it makes no sense that this
> is either 0 or false: then nobody can see it in /sys/modules/.
It makes sense if we want it set when the module is loaded only. This way, we
don't have to manage the parameters values changing while work is being done
and keep it simple.
It could be made readable if that looks better, but there is no real need for
it to be read either.
> Typically this is 0444 if it is readable only, or 0644 if it can be
> written by root.
>
> Regards,
>
> Hans
>
> > MODULE_PARM_DESC(keep_bitstream_buffers,
> >
> > " keep bitstream buffers in debugfs after streaming is
stopped");
On 22/11/2023 17:38, Detlev Casanova wrote:
> On Wednesday, November 22, 2023 10:56:20 A.M. EST Hans Verkuil wrote:
>> On 24/10/2023 21:09, Detlev Casanova wrote:
>>> `false` was used as the keep_bitstream_buffers parameter permissions.
>>> This looks more like a default value for the parameter, so change it to
>>> 0 to avoid confusion.
>>>
>>> Reviewed-by: Daniel Almeida <[email protected]>
>>> Signed-off-by: Detlev Casanova <[email protected]>
>>> ---
>>>
>>> drivers/media/test-drivers/visl/visl-core.c | 2 +-
>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/media/test-drivers/visl/visl-core.c
>>> b/drivers/media/test-drivers/visl/visl-core.c index
>>> 9970dc739ca5..df6515530fbf 100644
>>> --- a/drivers/media/test-drivers/visl/visl-core.c
>>> +++ b/drivers/media/test-drivers/visl/visl-core.c
>>> @@ -74,7 +74,7 @@ MODULE_PARM_DESC(visl_dprintk_nframes,
>>>
>>> " the number of frames to trace with dprintk");
>>>
>>> bool keep_bitstream_buffers;
>>>
>>> -module_param(keep_bitstream_buffers, bool, false);
>>> +module_param(keep_bitstream_buffers, bool, 0);
>>
>> ???
>>
>> This last parameter is the permission, it makes no sense that this
>> is either 0 or false: then nobody can see it in /sys/modules/.
>
> It makes sense if we want it set when the module is loaded only. This way, we
> don't have to manage the parameters values changing while work is being done
> and keep it simple.
> It could be made readable if that looks better, but there is no real need for
> it to be read either.
Why not? It makes it easy to read what this module option's value is.
I now see that both visl and vidtv uses 0 a lot, so I'm OK with this
patch for consistency.
But I think especially these test-drivers should use 0444 instead of 0
so you can see how the test driver is configured. That might actually
be relevant when writing tests using these drivers.
Perhaps separate patches for visl and vidtv that change 0 to 0444 for all
the module parameters?
Regards,
Hans
>
>> Typically this is 0444 if it is readable only, or 0644 if it can be
>> written by root.
>>
>> Regards,
>>
>> Hans
>>
>>> MODULE_PARM_DESC(keep_bitstream_buffers,
>>>
>>> " keep bitstream buffers in debugfs after streaming is
> stopped");
>