Re: [PATCH 3/8] Montage M88DS3103 DVB-S/S2 demodulator driver

2013-11-08 Thread Manu Abraham
On Sat, Nov 9, 2013 at 8:18 AM, Antti Palosaari  wrote:
> On 09.11.2013 04:35, Manu Abraham wrote:
>>
>> On Wed, Nov 6, 2013 at 11:27 PM, Antti Palosaari  wrote:
>
>
>
>>> +/*
>>> + * Driver implements own I2C-adapter for tuner I2C access. That's since
>>> chip
>>> + * has I2C-gate control which closes gate automatically after I2C
>>> transfer.
>>> + * Using own I2C adapter we can workaround that.
>>> + */
>>
>>
>>
>> Why should the demodulator implement it's own adapter for tuner access ?
>
>
> In order to implement it properly.
>
>
>
>> DS3103 is identical to DS3002, DS3000 which is similar to all other
>> dvb demodulators. Comparing datsheets of these demodulators
>> with others, I can't see any difference in the repeater setup, except
>> for an additional bit field to control the repeater block itself.
>>
>> Also, from what I see, the vendor; Montage has a driver, which appears
>> to be more code complete looking at this url. http://goo.gl/biaPYu
>>
>> Do you still think the DS3103 is much different in comparison ?
>

DS3000 demodulator datasheet states:

To avoid unwanted noise disturbing the tuner performance, the
M88DS3000 offers a 2-wire bus repeater dedicated for tuner
control. The tuner is connected to the M88DS3000 through the
SCLT and SDAT pins. See Figure 11. Every time the 2-wire bus
master wants to access the tuner registers, it must enable the
repeater first. When the repeater is enabled, the SDAT and SCLT
pins are active. The messages on the SDA and SCL pins is
repeated on the SDAT and SCLT pins. The repeater will be
automatically disabled once the access times to the tuner
reaches the configured value. When disabled, the SCLT and
SDAT pins are completely isolated from the 2-wire bus and
become inactive (HIGH).

DS3002 demodulator datasheet states:

To avoid unwanted noise disturbing the tuner performance, the
M88DS3002B offers a 2-wire bus repeater dedicated for tuner
control. The tuner is connected to the M88DS3002B through
the SCLT and SDAT pins. See Figure 12. Every time the 2-wire
bus master wants to access the tuner registers, it must enable
the repeater first by configuring bit 2_WIRE_REP_EN (03H).
When the repeater is enabled, the SDAT and SCLT pins are
active. The messages on the SDA and SCL pins is repeated
on the SDAT and SCLT pins. The repeater will be automatically
disabled once the access times to the tuner reaches the
configured value set in bits 2_WIRE_REP_TM[2:0] (03H).
When disabled, the SCLT and SDAT pins are completely
isolated from the 2-wire bus and become inactive (HIGH).

DS3013 demodulator datasheet states:

To avoid unwanted noise disturbing the tuner performance, the
M88DS3103 offers a 2-wire bus repeater dedicated for tuner
control. The tuner is connected to the M88DS3103 through the
SCLT and SDAT pins. See Figure 12. Every time the 2-wire bus
master wants to access the tuner registers, it must enable the
repeater first by configuring bit 2_WIRE_REP_EN (03H). When
the repeater is enabled, the SDAT and SCLT pins are active.
The messages on the SDA and SCL pins is repeated on the
SDAT and SCLT pins. The repeater will be automatically
disabled once the access times to the tuner reaches the
configured value set in bits 2_WIRE_REP_TM[2:0] (03H).
When disabled, the SCLT and SDAT pins are completely
isolated from the 2-wire bus and become inactive (HIGH).

When you compare this with *almost* any other demodulator
that exists; This behaviour is much consistent with that which
exists in the mainline kernel source.


If you look at most DVB-S/S2 demodulator drivers almost all
of them do have an I2C repeater, which in some cases are
configurable for a) auto-manual close, b) auto close,
c) manual close. The majority of them do auto close,
unless bugs on the hardware implementation do exist.

What I don't understand why you need an I2C adapter to handle
the I2C repeater. All demodulator drivers use i2c_gate_ctl
to enable/disable the repeater.

ie, how is your i2c_adapter implementation for the ds3103
demodulator going to make things better than:

static int ds3103_i2c_gate_ctrl(struct dvb_frontend *fe, int enable)
{
struct ds3103_state *state = fe->demodulator_priv;

if (enable)
ds3103_writereg(state, 0x03, 0x12);
else
ds3103_writereg(state, 0x03, 0x02);

return 0;
}

which is more common to all other DVB demodulator drivers.
Please don't make weird implementations for straight forward
stuff.

>
> There was even some patches, maybe 2 years, ago in order to mainline that
> but it never happened.

??

>
> More complete is here 53 vs. 86 register writes, so yes it is more ~40 more
> complete if you like to compare it like that.

What I would stress more, is that the driver at this URL

http://goo.gl/biaPYu

is from Montage themselves rather than a reverse engineered one;
rather than the number of lines of code, or number of registers.
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the 

Re: [PATCH 3/8] Montage M88DS3103 DVB-S/S2 demodulator driver

2013-11-08 Thread Antti Palosaari

On 09.11.2013 04:35, Manu Abraham wrote:

On Wed, Nov 6, 2013 at 11:27 PM, Antti Palosaari  wrote:




+/*
+ * Driver implements own I2C-adapter for tuner I2C access. That's since chip
+ * has I2C-gate control which closes gate automatically after I2C transfer.
+ * Using own I2C adapter we can workaround that.
+ */



Why should the demodulator implement it's own adapter for tuner access ?


In order to implement it properly.



DS3103 is identical to DS3002, DS3000 which is similar to all other
dvb demodulators. Comparing datsheets of these demodulators
with others, I can't see any difference in the repeater setup, except
for an additional bit field to control the repeater block itself.

Also, from what I see, the vendor; Montage has a driver, which appears
to be more code complete looking at this url. http://goo.gl/biaPYu

Do you still think the DS3103 is much different in comparison ?


There was even some patches, maybe 2 years, ago in order to mainline 
that but it never happened.


More complete is here 53 vs. 86 register writes, so yes it is more ~40 
more complete if you like to compare it like that.


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 3/8] Montage M88DS3103 DVB-S/S2 demodulator driver

2013-11-08 Thread Manu Abraham
On Wed, Nov 6, 2013 at 11:27 PM, Antti Palosaari  wrote:
> ---
>  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.
> + */
> +
> +#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];
> +   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];
> +   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,

[PATCH 3/8] Montage M88DS3103 DVB-S/S2 demodulator driver

2013-11-06 Thread 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.
+ */
+
+#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];
+   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];
+   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;
+}
+
+/* write single register */
+static int m88ds3103_wr_reg(struct m88ds3103_priv *priv, u8 reg, u8 val)
+{
+   re