Re: [PATCH REVIEW 03/18] Montage M88DS3103 DVB-S/S2 demodulator driver
On 18.12.2013 14:35, Mauro Carvalho Chehab wrote:
Hi Antti,
Em Mon, 9 Dec 2013 00:31:20 +0200
Antti Palosaari 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/
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH REVIEW 03/18] Montage M88DS3103 DVB-S/S2 demodulator driver
Hi Antti,
Em Mon, 9 Dec 2013 00:31:20 +0200
Antti Palosaari escreveu:
> DVB-S/S2 satellite television demodulator driver.
>
> Signed-off-by: Antti Palosaari
> ---
> drivers/media/dvb-frontends/Kconfig |7 +
> drivers/media/dvb-frontends/Makefile |1 +
> drivers/media/dvb-frontends/m88ds3103.c | 1293
> ++
> drivers/media/dvb-frontends/m88ds3103.h | 108 +++
> drivers/media/dvb-frontends/m88ds3103_priv.h | 218 +
> 5 files changed, 1627 insertions(+)
> create mode 100644 drivers/media/dvb-frontends/m88ds3103.c
> create mode 100644 drivers/media/dvb-frontends/m88ds3103.h
> create mode 100644 drivers/media/dvb-frontends/m88ds3103_priv.h
>
> diff --git a/drivers/media/dvb-frontends/Kconfig
> b/drivers/media/dvb-frontends/Kconfig
> index bddbab4..6c46caf 100644
> --- a/drivers/media/dvb-frontends/Kconfig
> +++ b/drivers/media/dvb-frontends/Kconfig
> @@ -35,6 +35,13 @@ config DVB_STV6110x
> help
> A Silicon tuner that supports DVB-S and DVB-S2 modes
>
> +config DVB_M88DS3103
> + tristate "Montage M88DS3103"
> + depends on DVB_CORE && I2C
> + default m if !MEDIA_SUBDRV_AUTOSELECT
> + help
> + Say Y when you want to support this frontend.
> +
> comment "Multistandard (cable + terrestrial) frontends"
> depends on DVB_CORE
>
> diff --git a/drivers/media/dvb-frontends/Makefile
> b/drivers/media/dvb-frontends/Makefile
> index f9cb43d..0c75a6a 100644
> --- a/drivers/media/dvb-frontends/Makefile
> +++ b/drivers/media/dvb-frontends/Makefile
> @@ -85,6 +85,7 @@ obj-$(CONFIG_DVB_STV6110) += stv6110.o
> obj-$(CONFIG_DVB_STV0900) += stv0900.o
> obj-$(CONFIG_DVB_STV090x) += stv090x.o
> obj-$(CONFIG_DVB_STV6110x) += stv6110x.o
> +obj-$(CONFIG_DVB_M88DS3103) += m88ds3103.o
> obj-$(CONFIG_DVB_ISL6423) += isl6423.o
> obj-$(CONFIG_DVB_EC100) += ec100.o
> obj-$(CONFIG_DVB_HD29L2) += hd29l2.o
> diff --git a/drivers/media/dvb-frontends/m88ds3103.c
> b/drivers/media/dvb-frontends/m88ds3103.c
> new file mode 100644
> index 000..91b3729
> --- /dev/null
> +++ b/drivers/media/dvb-frontends/m88ds3103.c
> @@ -0,0 +1,1293 @@
> +/*
> + * Montage M88DS3103 demodulator driver
> + *
> + * Copyright (C) 2013 Antti Palosaari
> + *
> + *This program is free software; you can redistribute it and/or modify
> + *it under the terms of the GNU General Public License as published by
> + *the Free Software Foundation; either version 2 of the License, or
> + *(at your option) any later version.
> + *
> + *This program is distributed in the hope that it will be useful,
> + *but WITHOUT ANY WARRANTY; without even the implied warranty of
> + *MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + *GNU General Public License for more details.
> + *
> + *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?
> +
> +#include "m88ds3103_priv.h"
> +
> +static struct dvb_frontend_ops m88ds3103_ops;
> +
> +/* write multiple registers */
> +static int m88ds3103_wr_regs(struct m88ds3103_priv *priv,
> + u8 reg, const u8 *val, int len)
> +{
> + int ret;
> + u8 buf[1 + len];
Please, don't use dynamic buffer.
> + struct i2c_msg msg[1] = {
> + {
> + .addr = priv->cfg->i2c_addr,
> + .flags = 0,
> + .len = sizeof(buf),
> + .buf = buf,
> + }
> + };
> +
> + buf[0] = reg;
> + memcpy(&buf[1], val, len);
> +
> + mutex_lock(&priv->i2c_mutex);
> + ret = i2c_transfer(priv->i2c, msg, 1);
> + mutex_unlock(&priv->i2c_mutex);
> + if (ret == 1) {
> + ret = 0;
> + } else {
> + dev_warn(&priv->i2c->dev,
> + "%s: i2c wr failed=%d reg=%02x len=%d\n",
> + KBUILD_MODNAME, ret, reg, len);
> + ret = -EREMOTEIO;
> + }
> +
> + return ret;
> +}
> +
> +/* read multiple registers */
> +static int m88ds3103_rd_regs(struct m88ds3103_priv *priv,
> + u8 reg, u8 *val, int len)
> +{
> + int ret;
> + u8 buf[len
Re: [PATCH REVIEW 03/18] Montage M88DS3103 DVB-S/S2 demodulator driver
Hi Antti,
I have a small suggestion, see below.
On 08.12.2013 23:31, Antti Palosaari wrote:
> diff --git a/drivers/media/dvb-frontends/m88ds3103.c
> b/drivers/media/dvb-frontends/m88ds3103.c
> new file mode 100644
> index 000..91b3729
> --- /dev/null
> +++ b/drivers/media/dvb-frontends/m88ds3103.c
> @@ -0,0 +1,1293 @@
> +/*
> + * Montage M88DS3103 demodulator driver
> + *
> + * Copyright (C) 2013 Antti Palosaari
> + *
> + *This program is free software; you can redistribute it and/or modify
> + *it under the terms of the GNU General Public License as published by
> + *the Free Software Foundation; either version 2 of the License, or
> + *(at your option) any later version.
> + *
> + *This program is distributed in the hope that it will be useful,
> + *but WITHOUT ANY WARRANTY; without even the implied warranty of
> + *MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + *GNU General Public License for more details.
> + *
> + *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.
> + */
> +
> +#include "m88ds3103_priv.h"
> +
> +static struct dvb_frontend_ops m88ds3103_ops;
> +
> +/* write multiple registers */
> +static int m88ds3103_wr_regs(struct m88ds3103_priv *priv,
> + u8 reg, const u8 *val, int len)
> +{
> + int ret;
> + u8 buf[1 + len];
Looking at the recent patches for variable length arrays, I think this
should be converted to fixed size.
> + struct i2c_msg msg[1] = {
> + {
> + .addr = priv->cfg->i2c_addr,
> + .flags = 0,
> + .len = sizeof(buf),
> + .buf = buf,
> + }
> + };
> +
> + buf[0] = reg;
> + memcpy(&buf[1], val, len);
> +
> + mutex_lock(&priv->i2c_mutex);
> + ret = i2c_transfer(priv->i2c, msg, 1);
> + mutex_unlock(&priv->i2c_mutex);
> + if (ret == 1) {
> + ret = 0;
> + } else {
> + dev_warn(&priv->i2c->dev,
> + "%s: i2c wr failed=%d reg=%02x len=%d\n",
> + KBUILD_MODNAME, ret, reg, len);
> + ret = -EREMOTEIO;
> + }
> +
> + return ret;
> +}
> +
> +/* read multiple registers */
> +static int m88ds3103_rd_regs(struct m88ds3103_priv *priv,
> + u8 reg, u8 *val, int len)
> +{
> + int ret;
> + u8 buf[len];
Same as above.
> + struct i2c_msg msg[2] = {
> + {
> + .addr = priv->cfg->i2c_addr,
> + .flags = 0,
> + .len = 1,
> + .buf = ®,
> + }, {
> + .addr = priv->cfg->i2c_addr,
> + .flags = I2C_M_RD,
> + .len = sizeof(buf),
> + .buf = buf,
> + }
> + };
> +
> + mutex_lock(&priv->i2c_mutex);
> + ret = i2c_transfer(priv->i2c, msg, 2);
> + mutex_unlock(&priv->i2c_mutex);
> + if (ret == 2) {
> + memcpy(val, buf, len);
> + ret = 0;
> + } else {
> + dev_warn(&priv->i2c->dev,
> + "%s: i2c rd failed=%d reg=%02x len=%d\n",
> + KBUILD_MODNAME, ret, reg, len);
> + ret = -EREMOTEIO;
> + }
> +
> + return ret;
> +}
> +
Regards
Matthias
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
