Re: [PATCH] isl6421.c - added optional features: tone control and temporary diseqc overcurrent

[Mauro Carvalho Chehab <mchehab@redhat.com>]

HoP escreveu:
> Hi,
>
> this is my first kernel patch, so all comments are welcome.
>   
First of all, please check all your patches with checkpatch, to be sure
that they don't have any CodingStyle troubles. There are some on your
patch (the better is to read README.patches for more info useful
for developers).
> Attached patch adds two optional (so, disabled by default
> and therefore could not break any compatibility) features:
>
> 1, tone_control=1
> When enabled, ISL6421 overrides frontend's tone control
> function (fe->ops.set_tone) by its own one.
>   
On your comments, the better is to describe why someone would need
to use such option. You should also add a quick hint about that at the
option description.
> 2, overcurrent_enable=1
> When enabled, overcurrent protection is disabled during
> sending diseqc command. Such option is usable when ISL6421
> catch overcurrent threshold and starts limiting output.
> Note: protection is disabled only during sending
> of diseqc command, until next set_tone() usage.
> What typically means only max up to few hundreds of ms.
> WARNING: overcurrent_enable=1 is dangerous
> and can damage your device. Use with care
> and only if you really know what you do.
>   
I'm not sure if it is a good idea to have this... Why/when someone would 
need this?

If we go ahead and add this one, you should add a notice about it at the 
parameter.
I would also print a big WARNING message at the dmesg if the module were 
loaded
with this option turned on.
> /Honza
>
> Signed-off-by: Jan Petrous <jpetrous@gmail.com>
> ---
>