[PATCH] media: tua6100: Remove some ugly defines

[PATCH] media: tua6100: Remove some ugly defines

[Mauro Carvalho Chehab]

As reported by Stephen:

> After merging the i2c tree, today's linux-next build (x86_64 allmodconfig)
> produced this warning:
>
> drivers/media/dvb-frontends/tua6100.c: In function 'tua6100_set_params':
> drivers/media/dvb-frontends/tua6100.c:71: warning: "_P" redefined
>  #define _P 32
>
> In file included from include/acpi/platform/aclinux.h:54,
>                  from include/acpi/platform/acenv.h:152,
>                  from include/acpi/acpi.h:22,
>                  from include/linux/acpi.h:21,
>                  from include/linux/i2c.h:17,
>                  from drivers/media/dvb-frontends/tua6100.h:22,
>                  from drivers/media/dvb-frontends/tua6100.c:24:
> include/linux/ctype.h:14: note: this is the location of the previous definition
>  #define _P 0x10 /* punct */
>
> Exposed by commit
>
>   5213d7efc8ec ("i2c: acpi: export i2c_acpi_find_adapter_by_handle")
>
> Since that included <linux/acpi.h> from <linux/i2c.h>
>
> Originally introduced by commit
>
>   00be2e7c6415 ("V4L/DVB (4606): Add driver for TUA6100")
>
> The _P in <linux/ctype.h> has existed since before git.

The addition of include <linux/ctype.h> at the I2C code caused a
breakage at the tua6100 driver. The reason is that the code there
used defines for 3 parameters used at the calculus for the
divide ratio.

In thesis, those are board-dependent, but, as there's just one
driver using it (ttpci/budget-av), there was no need to make
the code more generic. While it sounds unlikely that this old
DVB-S frontend would ever be used on new projects, one might
some day come with a variant using a different configuration. So,
let's do the right thing and store those values at its private
struct.

Reported-by: Stephen Rothwell <sfr@canb.auug.org.au>
Signed-off-by: Mauro Carvalho Chehab <mchehab+samsung@kernel.org>
---
 drivers/media/dvb-frontends/tua6100.c | 38 ++++++++++++++++-----------
 1 file changed, 23 insertions(+), 15 deletions(-)

diff --git a/drivers/media/dvb-frontends/tua6100.c b/drivers/media/dvb-frontends/tua6100.c
index b233b7be0b84..2990eb3fd475 100644
--- a/drivers/media/dvb-frontends/tua6100.c
+++ b/drivers/media/dvb-frontends/tua6100.c
@@ -31,11 +31,24 @@
 
 #include "tua6100.h"
 
+/**
+ * struct tua6100_priv - tuner's private data
+ *
+ * @i2c_address:	I2C address
+ * @i2c:		pointer to struct i2c_adapter
+ * @frequency:		tuned frequency
+ * @prescaler_div:	divide ratio of the prescaler (32 or 64)
+ * @ref_div:		reference frequency divider
+ * @ref_inp:		reference frequency input (XTAL osc)
+ */
 struct tua6100_priv {
 	/* i2c details */
 	int i2c_address;
 	struct i2c_adapter *i2c;
 	u32 frequency;
+	int prescaler_div;
+	int ref_div;
+	u32 ref_inp;
 };
 
 static void tua6100_release(struct dvb_frontend *fe)
@@ -75,10 +88,6 @@ static int tua6100_set_params(struct dvb_frontend *fe)
 	struct i2c_msg msg1 = { .addr = priv->i2c_address, .flags = 0, .buf = reg1, .len = 4 };
 	struct i2c_msg msg2 = { .addr = priv->i2c_address, .flags = 0, .buf = reg2, .len = 3 };
 
-#define _R 4
-#define _P 32
-#define _ri 4000000
-
 	// setup register 0
 	if (c->frequency < 2000000)
 		reg0[1] = 0x03;
@@ -91,14 +100,14 @@ static int tua6100_set_params(struct dvb_frontend *fe)
 	else
 		reg1[1] = 0x0c;
 
-	if (_P == 64)
+	if (priv->prescaler_div == 64)
 		reg1[1] |= 0x40;
 	if (c->frequency >= 1525000)
 		reg1[1] |= 0x80;
 
 	// register 2
-	reg2[1] = (_R >> 8) & 0x03;
-	reg2[2] = _R;
+	reg2[1] = (priv->ref_div >> 8) & 0x03;
+	reg2[2] = priv->ref_div;
 	if (c->frequency < 1455000)
 		reg2[1] |= 0x1c;
 	else if (c->frequency < 1630000)
@@ -110,19 +119,15 @@ static int tua6100_set_params(struct dvb_frontend *fe)
 	 * The N divisor ratio (note: c->frequency is in kHz, but we
 	 * need it in Hz)
 	 */
-	prediv = (c->frequency * _R) / (_ri / 1000);
-	div = prediv / _P;
+	prediv = (c->frequency * priv->ref_div) / (priv->ref_inp / 1000);
+	div = prediv / priv->prescaler_div;
 	reg1[1] |= (div >> 9) & 0x03;
 	reg1[2] = div >> 1;
 	reg1[3] = (div << 7);
-	priv->frequency = ((div * _P) * (_ri / 1000)) / _R;
+	priv->frequency = ((div * priv->prescaler_div) * (priv->ref_inp / 1000)) / priv->ref_div;
 
 	// Finally, calculate and store the value for A
-	reg1[3] |= (prediv - (div*_P)) & 0x7f;
-
-#undef _R
-#undef _P
-#undef _ri
+	reg1[3] |= (prediv - (div * priv->prescaler_div)) & 0x7f;
 
 	if (fe->ops.i2c_gate_ctrl)
 		fe->ops.i2c_gate_ctrl(fe, 1);
@@ -189,6 +194,9 @@ struct dvb_frontend *tua6100_attach(struct dvb_frontend *fe, int addr, struct i2
 
 	priv->i2c_address = addr;
 	priv->i2c = i2c;
+	priv->ref_div = 4;
+	priv->prescaler_div = 32;
+	priv->ref_inp = 4000000;
 
 	memcpy(&fe->ops.tuner_ops, &tua6100_tuner_ops, sizeof(struct dvb_tuner_ops));
 	fe->tuner_priv = priv;
-- 
2.21.0

Re: [PATCH] media: tua6100: Remove some ugly defines

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 2079 bytes --]

On Wed, Jun 12, 2019 at 08:25:03AM -0300, Mauro Carvalho Chehab wrote:
> As reported by Stephen:
> 
> > After merging the i2c tree, today's linux-next build (x86_64 allmodconfig)
> > produced this warning:
> >
> > drivers/media/dvb-frontends/tua6100.c: In function 'tua6100_set_params':
> > drivers/media/dvb-frontends/tua6100.c:71: warning: "_P" redefined
> >  #define _P 32
> >
> > In file included from include/acpi/platform/aclinux.h:54,
> >                  from include/acpi/platform/acenv.h:152,
> >                  from include/acpi/acpi.h:22,
> >                  from include/linux/acpi.h:21,
> >                  from include/linux/i2c.h:17,
> >                  from drivers/media/dvb-frontends/tua6100.h:22,
> >                  from drivers/media/dvb-frontends/tua6100.c:24:
> > include/linux/ctype.h:14: note: this is the location of the previous definition
> >  #define _P 0x10 /* punct */
> >
> > Exposed by commit
> >
> >   5213d7efc8ec ("i2c: acpi: export i2c_acpi_find_adapter_by_handle")
> >
> > Since that included <linux/acpi.h> from <linux/i2c.h>
> >
> > Originally introduced by commit
> >
> >   00be2e7c6415 ("V4L/DVB (4606): Add driver for TUA6100")
> >
> > The _P in <linux/ctype.h> has existed since before git.
> 
> The addition of include <linux/ctype.h> at the I2C code caused a
> breakage at the tua6100 driver. The reason is that the code there
> used defines for 3 parameters used at the calculus for the
> divide ratio.
> 
> In thesis, those are board-dependent, but, as there's just one
> driver using it (ttpci/budget-av), there was no need to make
> the code more generic. While it sounds unlikely that this old
> DVB-S frontend would ever be used on new projects, one might
> some day come with a variant using a different configuration. So,
> let's do the right thing and store those values at its private
> struct.
> 
> Reported-by: Stephen Rothwell <sfr@canb.auug.org.au>
> Signed-off-by: Mauro Carvalho Chehab <mchehab+samsung@kernel.org>

Acked-by: Wolfram Sang <wsa@the-dreams.de>


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] media: tua6100: Remove some ugly defines

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 2170 bytes --]

On Wed, Jun 12, 2019 at 08:25:03AM -0300, Mauro Carvalho Chehab wrote:
> As reported by Stephen:
> 
> > After merging the i2c tree, today's linux-next build (x86_64 allmodconfig)
> > produced this warning:
> >
> > drivers/media/dvb-frontends/tua6100.c: In function 'tua6100_set_params':
> > drivers/media/dvb-frontends/tua6100.c:71: warning: "_P" redefined
> >  #define _P 32
> >
> > In file included from include/acpi/platform/aclinux.h:54,
> >                  from include/acpi/platform/acenv.h:152,
> >                  from include/acpi/acpi.h:22,
> >                  from include/linux/acpi.h:21,
> >                  from include/linux/i2c.h:17,
> >                  from drivers/media/dvb-frontends/tua6100.h:22,
> >                  from drivers/media/dvb-frontends/tua6100.c:24:
> > include/linux/ctype.h:14: note: this is the location of the previous definition
> >  #define _P 0x10 /* punct */
> >
> > Exposed by commit
> >
> >   5213d7efc8ec ("i2c: acpi: export i2c_acpi_find_adapter_by_handle")
> >
> > Since that included <linux/acpi.h> from <linux/i2c.h>
> >
> > Originally introduced by commit
> >
> >   00be2e7c6415 ("V4L/DVB (4606): Add driver for TUA6100")
> >
> > The _P in <linux/ctype.h> has existed since before git.
> 
> The addition of include <linux/ctype.h> at the I2C code caused a
> breakage at the tua6100 driver. The reason is that the code there
> used defines for 3 parameters used at the calculus for the
> divide ratio.
> 
> In thesis, those are board-dependent, but, as there's just one
> driver using it (ttpci/budget-av), there was no need to make
> the code more generic. While it sounds unlikely that this old
> DVB-S frontend would ever be used on new projects, one might
> some day come with a variant using a different configuration. So,
> let's do the right thing and store those values at its private
> struct.
> 
> Reported-by: Stephen Rothwell <sfr@canb.auug.org.au>
> Signed-off-by: Mauro Carvalho Chehab <mchehab+samsung@kernel.org>

I see there is a build-fix from davem now in linux-next. Shall I still
apply this patch to my i2c tree once Mauro applied it to his?


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] media: tua6100: Remove some ugly defines

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 2625 bytes --]

On Wed, Jun 12, 2019 at 08:25:03AM -0300, Mauro Carvalho Chehab wrote:
> As reported by Stephen:
> 
> > After merging the i2c tree, today's linux-next build (x86_64 allmodconfig)
> > produced this warning:
> >
> > drivers/media/dvb-frontends/tua6100.c: In function 'tua6100_set_params':
> > drivers/media/dvb-frontends/tua6100.c:71: warning: "_P" redefined
> >  #define _P 32
> >
> > In file included from include/acpi/platform/aclinux.h:54,
> >                  from include/acpi/platform/acenv.h:152,
> >                  from include/acpi/acpi.h:22,
> >                  from include/linux/acpi.h:21,
> >                  from include/linux/i2c.h:17,
> >                  from drivers/media/dvb-frontends/tua6100.h:22,
> >                  from drivers/media/dvb-frontends/tua6100.c:24:
> > include/linux/ctype.h:14: note: this is the location of the previous definition
> >  #define _P 0x10 /* punct */
> >
> > Exposed by commit
> >
> >   5213d7efc8ec ("i2c: acpi: export i2c_acpi_find_adapter_by_handle")
> >
> > Since that included <linux/acpi.h> from <linux/i2c.h>
> >
> > Originally introduced by commit
> >
> >   00be2e7c6415 ("V4L/DVB (4606): Add driver for TUA6100")
> >
> > The _P in <linux/ctype.h> has existed since before git.
> 
> The addition of include <linux/ctype.h> at the I2C code caused a
> breakage at the tua6100 driver. The reason is that the code there
> used defines for 3 parameters used at the calculus for the
> divide ratio.
> 
> In thesis, those are board-dependent, but, as there's just one
> driver using it (ttpci/budget-av), there was no need to make
> the code more generic. While it sounds unlikely that this old
> DVB-S frontend would ever be used on new projects, one might
> some day come with a variant using a different configuration. So,
> let's do the right thing and store those values at its private
> struct.
> 
> Reported-by: Stephen Rothwell <sfr@canb.auug.org.au>
> Signed-off-by: Mauro Carvalho Chehab <mchehab+samsung@kernel.org>

Mauro, what do we do here? The merge window is open and I can't send my
I2C pull request unless the above issue is fixed. I see two options:

1) This patch gets fast-tracked into the media-tree (or I can pick it
into my I2C tree if you are fine with that)

2) I pick the intermediate fix from David Miller in linux-next into my
tree, but I'd need your ack for this patch (can be given here):

https://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git/commit/drivers/media/dvb-frontends/tua6100.c?id=621ccc6cc5f8d6730b740d31d4818227866c93c9

Kind regards,

   Wolfram


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]