Re: [PATCH REVIEW 03/18] Montage M88DS3103 DVB-S/S2 demodulator driver

[Antti Palosaari <crope@iki.fi>]

On 18.12.2013 14:35, Mauro Carvalho Chehab wrote:
> Hi Antti,
>
> Em Mon,  9 Dec 2013 00:31:20 +0200
> Antti Palosaari <crope@iki.fi> escreveu:
>
>> DVB-S/S2 satellite television demodulator driver.
>> + *    You should have received a copy of the GNU General Public License along
>> + *    with this program; if not, write to the Free Software Foundation, Inc.,
>> + *    51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
>> + */
>
> New versions of checkpatch complain about the above:
>
> ERROR: Do not include the paragraph about writing to the Free Software Foundation's mailing address from the sample GPL notice. The FSF has changed addresses in the past, and may do so again. Linux already includes a copy of the GPL.
> #73: FILE: drivers/media/dvb-frontends/m88ds3103.c:16:
> + *    You should have received a copy of the GNU General Public License along$
>
> What they're likely want to prevent is to have future big mass patches just
> to fix the GNU address.
>
> This is not needed, anyway, as kernel has the /COPYING file with has the
> GPLv2 license.
>
> So, could you please remove this and resend the pull request?

Hey, that is written almost year ago and those new checkpatch 
requirements are very new, I think from latest release candidate version.

However, I could send new patch top of that set which meets latest 
3.13-rc4 checkpatch requirements.

>> +static int m88ds3103_wr_regs(struct m88ds3103_priv *priv,
>> +	u8 buf[1 + len];
>
> Please, don't use dynamic buffer.

Same here. Fixed already by later patched.

>> +static int m88ds3103_rd_regs(struct m88ds3103_priv *priv,
> Same here.

Same here. Fixed already by later patched.



>> diff --git a/drivers/media/dvb-frontends/m88ds3103_priv.h b/drivers/media/dvb-frontends/m88ds3103_priv.h

>> +static const struct m88ds3103_reg_val m88ds3103_dvbs_init_reg_vals[] = {
>
> Why to put this inside a header file? Is it shared with some other c file?

It is private header file, not the shared one which is named as 
m88ds3103.h. Having inittabs inside private header file is common 
practice among DVB frontend driver. I think almost all demod driver has 
done it similarly. I suspect the main reason is just to separate static 
stuff out from code to keep driver file itself shorter.

I think it is good practice. It is place for another discussion whether 
it is good practice or not.

regards
Antti

-- 
http://palosaari.fi/