Hi Wolfram,
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.
--
Cheers,
Stephen Rothwell
On Tue, Jun 11, 2019 at 10:25:28AM +1000, Stephen Rothwell wrote:
> Hi Wolfram,
>
> 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.
I suggest to fix the driver by adding a TUA6100_ prefix to the defines.
I can cook up a patch for that.
Em Wed, 12 Jun 2019 10:19:29 +0200
Wolfram Sang <[email protected]> escreveu:
> On Tue, Jun 11, 2019 at 10:25:28AM +1000, Stephen Rothwell wrote:
> > Hi Wolfram,
> >
> > 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.
>
> I suggest to fix the driver by adding a TUA6100_ prefix to the defines.
> I can cook up a patch for that.
>
That entire use of _P, _R and _ri looks weird into my eyes. The code there
do things like:
#define _P 32
...
if (_P == 64)
reg1[1] |= 0x40;
It sounds to me that _P and _R are actually some sort of setup
with depends on how the device is wired.
A quick read on its datasheet at page 19 - downloaded from:
http://www.datasheetcatalog.com/datasheets_pdf/T/U/A/6/TUA6100.shtml
Shows that:
P: divide ratio of the prescaler.
R: divide ratio of the R-counter.
ri: reference frequency input (XTAL osc).
IMO, the right fix would be to change struct tua6100_priv, in order to
add those parameters, and initialize them with the current values
at tua6100_attach.
That would allow changing those values during device initialization,
in the case we ever need to support a hardware with the same chipset,
with, for example, a different XTAL.
I'll work on a patch to address it.
Thanks,
Mauro
Hi Mauro,
> That entire use of _P, _R and _ri looks weird into my eyes. The code there
Yes.
> do things like:
>
> #define _P 32
>
> ...
>
> if (_P == 64)
> reg1[1] |= 0x40;
Yup, I saw this, too, but didn't feel like refactoring the driver.
Thanks for stepping up!
> I'll work on a patch to address it.
OK, so that means I should send my pull request after yours in the next
merge window? To avoid the build breakage?
Kind regards,
Wolfram
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 <[email protected]>
Signed-off-by: Mauro Carvalho Chehab <[email protected]>
---
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
Em Wed, 12 Jun 2019 13:09:04 +0200
Wolfram Sang <[email protected]> escreveu:
> Hi Mauro,
>
> > That entire use of _P, _R and _ri looks weird into my eyes. The code there
>
> Yes.
>
> > do things like:
> >
> > #define _P 32
> >
> > ...
> >
> > if (_P == 64)
> > reg1[1] |= 0x40;
>
> Yup, I saw this, too, but didn't feel like refactoring the driver.
> Thanks for stepping up!
>
> > I'll work on a patch to address it.
>
> OK, so that means I should send my pull request after yours in the next
> merge window? To avoid the build breakage?
Either that or you can apply my patch on your tree before the
patch that caused the breakage.
Just let me know what works best for you.
>
> Kind regards,
>
> Wolfram
>
Thanks,
Mauro
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 <[email protected]>
> Signed-off-by: Mauro Carvalho Chehab <[email protected]>
Acked-by: Wolfram Sang <[email protected]>
> > OK, so that means I should send my pull request after yours in the next
> > merge window? To avoid the build breakage?
>
> Either that or you can apply my patch on your tree before the
> patch that caused the breakage.
>
> Just let me know what works best for you.
Hmm, the offending patch is already in -next and I don't rebase my tree.
So, I guess it's the merge window dependency then.
Em Wed, 12 Jun 2019 14:04:39 +0200
Wolfram Sang <[email protected]> escreveu:
> > > OK, so that means I should send my pull request after yours in the next
> > > merge window? To avoid the build breakage?
> >
> > Either that or you can apply my patch on your tree before the
> > patch that caused the breakage.
> >
> > Just let me know what works best for you.
>
> Hmm, the offending patch is already in -next and I don't rebase my tree.
> So, I guess it's the merge window dependency then.
>
Ok, I'll merge it through my tree then.
Thanks,
Mauro
On Wed, Jun 12, 2019 at 11:15:35PM +1000, Stephen Rothwell wrote:
> Hi all,
>
> On Wed, 12 Jun 2019 09:32:29 -0300 Mauro Carvalho Chehab <[email protected]> wrote:
> >
> > Em Wed, 12 Jun 2019 14:04:39 +0200
> > Wolfram Sang <[email protected]> escreveu:
> >
> > > > > OK, so that means I should send my pull request after yours in the next
> > > > > merge window? To avoid the build breakage?
> > > >
> > > > Either that or you can apply my patch on your tree before the
> > > > patch that caused the breakage.
> > > >
> > > > Just let me know what works best for you.
> > >
> > > Hmm, the offending patch is already in -next and I don't rebase my tree.
> > > So, I guess it's the merge window dependency then.
> > >
> > Ok, I'll merge it through my tree then.
>
> It should go into the i2c tree (since that is where the *warning* was
> introduced). It is only a warning and there won't be many patches
> between the patch that introduced the warning and this one that fixes
> it. This patch could then have a Fixes tag that makes sense i.e. it
> will reference a previous commit.
I can apply the patch to my i2c/for-next branch, but not to my
i2c/for-5.3 branch (which I merge into i2c/for-next). This way, the
warning will go away, but the media patch still goes to Linus via the
media tree. Is that suitable for you?
Hi all,
On Wed, 12 Jun 2019 09:32:29 -0300 Mauro Carvalho Chehab <[email protected]> wrote:
>
> Em Wed, 12 Jun 2019 14:04:39 +0200
> Wolfram Sang <[email protected]> escreveu:
>
> > > > OK, so that means I should send my pull request after yours in the next
> > > > merge window? To avoid the build breakage?
> > >
> > > Either that or you can apply my patch on your tree before the
> > > patch that caused the breakage.
> > >
> > > Just let me know what works best for you.
> >
> > Hmm, the offending patch is already in -next and I don't rebase my tree.
> > So, I guess it's the merge window dependency then.
> >
> Ok, I'll merge it through my tree then.
It should go into the i2c tree (since that is where the *warning* was
introduced). It is only a warning and there won't be many patches
between the patch that introduced the warning and this one that fixes
it. This patch could then have a Fixes tag that makes sense i.e. it
will reference a previous commit.
--
Cheers,
Stephen Rothwell
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 <[email protected]>
> Signed-off-by: Mauro Carvalho Chehab <[email protected]>
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?
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 <[email protected]>
> Signed-off-by: Mauro Carvalho Chehab <[email protected]>
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
Hello Wolfram,
On Fri, Aug 02, 2019 at 01:21:23PM +1000, Stephen Rothwell wrote:
> After merging the i2c tree, today's linux-next build (x86_64 allmodconfig)
> produced this warning:
>
> drivers/i2c/busses/i2c-designware-master.c: In function 'i2c_dw_init_recovery_info':
> drivers/i2c/busses/i2c-designware-master.c:658:6: warning: unused variable 'r' [-Wunused-variable]
> int r;
> ^
>
> Introduced by commit
>
> 33eb09a02e8d ("i2c: designware: make use of devm_gpiod_get_optional")
The obvious fix is to just remove this variable. Should I send a new
patch, or do you fix up locally?
Best regards
Uwe