Hi!
This seems like fairly straight forward cleanups/bugfixes. I don't
have the hardware and found the issues by reading code while doing
other things. So, it builds for me, but it's untested.
1/3 changes behavior on failure, but I think it's the right thing
to do. If it isn't for some reason, then the current code is crap
anyway, because as-is it compares with a value that is always zero
meaning that the entire "if (0 != bus->i2c_rc)"-clause with its
dev_warn can be removed from cx231xx_i2c_register.
Cheers,
Peter
Peter Rosin (3):
[media] cx231xx: fail probe if i2c_add_adapter fails
[media] cx231xx: drop return value of cx231xx_i2c_unregister
[media] cx231xx: only unregister successfully registered i2c adapters
drivers/media/usb/cx231xx/cx231xx-core.c | 3 +++
drivers/media/usb/cx231xx/cx231xx-i2c.c | 8 ++++----
drivers/media/usb/cx231xx/cx231xx.h | 4 ++--
3 files changed, 9 insertions(+), 6 deletions(-)
--
2.11.0
This prevents potentially scary debug messages from the i2c core.
Signed-off-by: Peter Rosin <[email protected]>
---
drivers/media/usb/cx231xx/cx231xx-core.c | 3 +++
drivers/media/usb/cx231xx/cx231xx-i2c.c | 3 ++-
2 files changed, 5 insertions(+), 1 deletion(-)
diff --git a/drivers/media/usb/cx231xx/cx231xx-core.c b/drivers/media/usb/cx231xx/cx231xx-core.c
index 46646ecd2dbc..f372ad3917a8 100644
--- a/drivers/media/usb/cx231xx/cx231xx-core.c
+++ b/drivers/media/usb/cx231xx/cx231xx-core.c
@@ -1311,6 +1311,7 @@ int cx231xx_dev_init(struct cx231xx *dev)
dev->i2c_bus[0].i2c_period = I2C_SPEED_100K; /* 100 KHz */
dev->i2c_bus[0].i2c_nostop = 0;
dev->i2c_bus[0].i2c_reserve = 0;
+ dev->i2c_bus[0].i2c_rc = -ENODEV;
/* External Master 2 Bus */
dev->i2c_bus[1].nr = 1;
@@ -1318,6 +1319,7 @@ int cx231xx_dev_init(struct cx231xx *dev)
dev->i2c_bus[1].i2c_period = I2C_SPEED_100K; /* 100 KHz */
dev->i2c_bus[1].i2c_nostop = 0;
dev->i2c_bus[1].i2c_reserve = 0;
+ dev->i2c_bus[1].i2c_rc = -ENODEV;
/* Internal Master 3 Bus */
dev->i2c_bus[2].nr = 2;
@@ -1325,6 +1327,7 @@ int cx231xx_dev_init(struct cx231xx *dev)
dev->i2c_bus[2].i2c_period = I2C_SPEED_100K; /* 100kHz */
dev->i2c_bus[2].i2c_nostop = 0;
dev->i2c_bus[2].i2c_reserve = 0;
+ dev->i2c_bus[2].i2c_rc = -ENODEV;
/* register I2C buses */
errCode = cx231xx_i2c_register(&dev->i2c_bus[0]);
diff --git a/drivers/media/usb/cx231xx/cx231xx-i2c.c b/drivers/media/usb/cx231xx/cx231xx-i2c.c
index 3e49517cb5e0..8ce6b815d16d 100644
--- a/drivers/media/usb/cx231xx/cx231xx-i2c.c
+++ b/drivers/media/usb/cx231xx/cx231xx-i2c.c
@@ -553,7 +553,8 @@ int cx231xx_i2c_register(struct cx231xx_i2c *bus)
*/
void cx231xx_i2c_unregister(struct cx231xx_i2c *bus)
{
- i2c_del_adapter(&bus->i2c_adap);
+ if (!bus->i2c_rc)
+ i2c_del_adapter(&bus->i2c_adap);
}
/*
--
2.11.0
Noone cares anyway.
Signed-off-by: Peter Rosin <[email protected]>
---
drivers/media/usb/cx231xx/cx231xx-i2c.c | 3 +--
drivers/media/usb/cx231xx/cx231xx.h | 2 +-
2 files changed, 2 insertions(+), 3 deletions(-)
diff --git a/drivers/media/usb/cx231xx/cx231xx-i2c.c b/drivers/media/usb/cx231xx/cx231xx-i2c.c
index 3a0c45ffd40f..3e49517cb5e0 100644
--- a/drivers/media/usb/cx231xx/cx231xx-i2c.c
+++ b/drivers/media/usb/cx231xx/cx231xx-i2c.c
@@ -551,10 +551,9 @@ int cx231xx_i2c_register(struct cx231xx_i2c *bus)
* cx231xx_i2c_unregister()
* unregister i2c_bus
*/
-int cx231xx_i2c_unregister(struct cx231xx_i2c *bus)
+void cx231xx_i2c_unregister(struct cx231xx_i2c *bus)
{
i2c_del_adapter(&bus->i2c_adap);
- return 0;
}
/*
diff --git a/drivers/media/usb/cx231xx/cx231xx.h b/drivers/media/usb/cx231xx/cx231xx.h
index 27ee035f9f84..72d5937a087e 100644
--- a/drivers/media/usb/cx231xx/cx231xx.h
+++ b/drivers/media/usb/cx231xx/cx231xx.h
@@ -762,7 +762,7 @@ int cx231xx_reset_analog_tuner(struct cx231xx *dev);
/* Provided by cx231xx-i2c.c */
void cx231xx_do_i2c_scan(struct cx231xx *dev, int i2c_port);
int cx231xx_i2c_register(struct cx231xx_i2c *bus);
-int cx231xx_i2c_unregister(struct cx231xx_i2c *bus);
+void cx231xx_i2c_unregister(struct cx231xx_i2c *bus);
int cx231xx_i2c_mux_create(struct cx231xx *dev);
int cx231xx_i2c_mux_register(struct cx231xx *dev, int mux_no);
void cx231xx_i2c_mux_unregister(struct cx231xx *dev);
--
2.11.0
While at it, change the type of the previously always-zero i2c_rc
member to int, matching the returned type from i2c_add_adapter.
Signed-off-by: Peter Rosin <[email protected]>
---
drivers/media/usb/cx231xx/cx231xx-i2c.c | 2 +-
drivers/media/usb/cx231xx/cx231xx.h | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/media/usb/cx231xx/cx231xx-i2c.c b/drivers/media/usb/cx231xx/cx231xx-i2c.c
index 8d95b1154e12..3a0c45ffd40f 100644
--- a/drivers/media/usb/cx231xx/cx231xx-i2c.c
+++ b/drivers/media/usb/cx231xx/cx231xx-i2c.c
@@ -538,7 +538,7 @@ int cx231xx_i2c_register(struct cx231xx_i2c *bus)
bus->i2c_adap.algo_data = bus;
i2c_set_adapdata(&bus->i2c_adap, &dev->v4l2_dev);
- i2c_add_adapter(&bus->i2c_adap);
+ bus->i2c_rc = i2c_add_adapter(&bus->i2c_adap);
if (0 != bus->i2c_rc)
dev_warn(dev->dev,
diff --git a/drivers/media/usb/cx231xx/cx231xx.h b/drivers/media/usb/cx231xx/cx231xx.h
index 986c64ba5b56..27ee035f9f84 100644
--- a/drivers/media/usb/cx231xx/cx231xx.h
+++ b/drivers/media/usb/cx231xx/cx231xx.h
@@ -476,7 +476,7 @@ struct cx231xx_i2c {
/* i2c i/o */
struct i2c_adapter i2c_adap;
- u32 i2c_rc;
+ int i2c_rc;
/* different settings for each bus */
u8 i2c_period;
--
2.11.0
Em Mon, 31 Jul 2017 15:38:52 +0200
Peter Rosin <[email protected]> escreveu:
> This prevents potentially scary debug messages from the i2c core.
>
> Signed-off-by: Peter Rosin <[email protected]>
> ---
> drivers/media/usb/cx231xx/cx231xx-core.c | 3 +++
> drivers/media/usb/cx231xx/cx231xx-i2c.c | 3 ++-
> 2 files changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/media/usb/cx231xx/cx231xx-core.c b/drivers/media/usb/cx231xx/cx231xx-core.c
> index 46646ecd2dbc..f372ad3917a8 100644
> --- a/drivers/media/usb/cx231xx/cx231xx-core.c
> +++ b/drivers/media/usb/cx231xx/cx231xx-core.c
> @@ -1311,6 +1311,7 @@ int cx231xx_dev_init(struct cx231xx *dev)
> dev->i2c_bus[0].i2c_period = I2C_SPEED_100K; /* 100 KHz */
> dev->i2c_bus[0].i2c_nostop = 0;
> dev->i2c_bus[0].i2c_reserve = 0;
> + dev->i2c_bus[0].i2c_rc = -ENODEV;
>
> /* External Master 2 Bus */
> dev->i2c_bus[1].nr = 1;
> @@ -1318,6 +1319,7 @@ int cx231xx_dev_init(struct cx231xx *dev)
> dev->i2c_bus[1].i2c_period = I2C_SPEED_100K; /* 100 KHz */
> dev->i2c_bus[1].i2c_nostop = 0;
> dev->i2c_bus[1].i2c_reserve = 0;
> + dev->i2c_bus[1].i2c_rc = -ENODEV;
>
> /* Internal Master 3 Bus */
> dev->i2c_bus[2].nr = 2;
> @@ -1325,6 +1327,7 @@ int cx231xx_dev_init(struct cx231xx *dev)
> dev->i2c_bus[2].i2c_period = I2C_SPEED_100K; /* 100kHz */
> dev->i2c_bus[2].i2c_nostop = 0;
> dev->i2c_bus[2].i2c_reserve = 0;
> + dev->i2c_bus[2].i2c_rc = -ENODEV;
>
> /* register I2C buses */
> errCode = cx231xx_i2c_register(&dev->i2c_bus[0]);
> diff --git a/drivers/media/usb/cx231xx/cx231xx-i2c.c b/drivers/media/usb/cx231xx/cx231xx-i2c.c
> index 3e49517cb5e0..8ce6b815d16d 100644
> --- a/drivers/media/usb/cx231xx/cx231xx-i2c.c
> +++ b/drivers/media/usb/cx231xx/cx231xx-i2c.c
> @@ -553,7 +553,8 @@ int cx231xx_i2c_register(struct cx231xx_i2c *bus)
> */
> void cx231xx_i2c_unregister(struct cx231xx_i2c *bus)
> {
> - i2c_del_adapter(&bus->i2c_adap);
> + if (!bus->i2c_rc)
> + i2c_del_adapter(&bus->i2c_adap);
That doesn't sound right. what happens if i2c_rc is 1 or 2?
IMHO, the right would would be, instead:
if (bus->i2c_rc >= 0)
i2c_del_adapter(&bus->i2c_adap);
Regards
Thanks,
Mauro
On 2017-08-09 16:27, Mauro Carvalho Chehab wrote:
> Em Mon, 31 Jul 2017 15:38:52 +0200
> Peter Rosin <[email protected]> escreveu:
>
>> This prevents potentially scary debug messages from the i2c core.
>>
>> Signed-off-by: Peter Rosin <[email protected]>
>> ---
>> drivers/media/usb/cx231xx/cx231xx-core.c | 3 +++
>> drivers/media/usb/cx231xx/cx231xx-i2c.c | 3 ++-
>> 2 files changed, 5 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/media/usb/cx231xx/cx231xx-core.c b/drivers/media/usb/cx231xx/cx231xx-core.c
>> index 46646ecd2dbc..f372ad3917a8 100644
>> --- a/drivers/media/usb/cx231xx/cx231xx-core.c
>> +++ b/drivers/media/usb/cx231xx/cx231xx-core.c
>> @@ -1311,6 +1311,7 @@ int cx231xx_dev_init(struct cx231xx *dev)
>> dev->i2c_bus[0].i2c_period = I2C_SPEED_100K; /* 100 KHz */
>> dev->i2c_bus[0].i2c_nostop = 0;
>> dev->i2c_bus[0].i2c_reserve = 0;
>> + dev->i2c_bus[0].i2c_rc = -ENODEV;
>>
>> /* External Master 2 Bus */
>> dev->i2c_bus[1].nr = 1;
>> @@ -1318,6 +1319,7 @@ int cx231xx_dev_init(struct cx231xx *dev)
>> dev->i2c_bus[1].i2c_period = I2C_SPEED_100K; /* 100 KHz */
>> dev->i2c_bus[1].i2c_nostop = 0;
>> dev->i2c_bus[1].i2c_reserve = 0;
>> + dev->i2c_bus[1].i2c_rc = -ENODEV;
>>
>> /* Internal Master 3 Bus */
>> dev->i2c_bus[2].nr = 2;
>> @@ -1325,6 +1327,7 @@ int cx231xx_dev_init(struct cx231xx *dev)
>> dev->i2c_bus[2].i2c_period = I2C_SPEED_100K; /* 100kHz */
>> dev->i2c_bus[2].i2c_nostop = 0;
>> dev->i2c_bus[2].i2c_reserve = 0;
>> + dev->i2c_bus[2].i2c_rc = -ENODEV;
>>
>> /* register I2C buses */
>> errCode = cx231xx_i2c_register(&dev->i2c_bus[0]);
>> diff --git a/drivers/media/usb/cx231xx/cx231xx-i2c.c b/drivers/media/usb/cx231xx/cx231xx-i2c.c
>> index 3e49517cb5e0..8ce6b815d16d 100644
>> --- a/drivers/media/usb/cx231xx/cx231xx-i2c.c
>> +++ b/drivers/media/usb/cx231xx/cx231xx-i2c.c
>> @@ -553,7 +553,8 @@ int cx231xx_i2c_register(struct cx231xx_i2c *bus)
>> */
>> void cx231xx_i2c_unregister(struct cx231xx_i2c *bus)
>> {
>> - i2c_del_adapter(&bus->i2c_adap);
>> + if (!bus->i2c_rc)
>> + i2c_del_adapter(&bus->i2c_adap);
>
> That doesn't sound right. what happens if i2c_rc is 1 or 2?
>
> IMHO, the right would would be, instead:
>
> if (bus->i2c_rc >= 0)
> i2c_del_adapter(&bus->i2c_adap);
In theory, yes. But in practice i2c_add_adapter never returns >0, and is
also documented so.
Let me know if you still want an update. In that case I'll also fix the
precedent present in the context of patch 1/3, i.e.
if (0 != bus->i2c_rc)
...
Cheers,
peda
Em Wed, 9 Aug 2017 16:43:03 +0200
Peter Rosin <[email protected]> escreveu:
> On 2017-08-09 16:27, Mauro Carvalho Chehab wrote:
> > Em Mon, 31 Jul 2017 15:38:52 +0200
> > Peter Rosin <[email protected]> escreveu:
> >
> >> This prevents potentially scary debug messages from the i2c core.
> >>
> >> Signed-off-by: Peter Rosin <[email protected]>
> >> ---
> >> drivers/media/usb/cx231xx/cx231xx-core.c | 3 +++
> >> drivers/media/usb/cx231xx/cx231xx-i2c.c | 3 ++-
> >> 2 files changed, 5 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/media/usb/cx231xx/cx231xx-core.c b/drivers/media/usb/cx231xx/cx231xx-core.c
> >> index 46646ecd2dbc..f372ad3917a8 100644
> >> --- a/drivers/media/usb/cx231xx/cx231xx-core.c
> >> +++ b/drivers/media/usb/cx231xx/cx231xx-core.c
> >> @@ -1311,6 +1311,7 @@ int cx231xx_dev_init(struct cx231xx *dev)
> >> dev->i2c_bus[0].i2c_period = I2C_SPEED_100K; /* 100 KHz */
> >> dev->i2c_bus[0].i2c_nostop = 0;
> >> dev->i2c_bus[0].i2c_reserve = 0;
> >> + dev->i2c_bus[0].i2c_rc = -ENODEV;
> >>
> >> /* External Master 2 Bus */
> >> dev->i2c_bus[1].nr = 1;
> >> @@ -1318,6 +1319,7 @@ int cx231xx_dev_init(struct cx231xx *dev)
> >> dev->i2c_bus[1].i2c_period = I2C_SPEED_100K; /* 100 KHz */
> >> dev->i2c_bus[1].i2c_nostop = 0;
> >> dev->i2c_bus[1].i2c_reserve = 0;
> >> + dev->i2c_bus[1].i2c_rc = -ENODEV;
> >>
> >> /* Internal Master 3 Bus */
> >> dev->i2c_bus[2].nr = 2;
> >> @@ -1325,6 +1327,7 @@ int cx231xx_dev_init(struct cx231xx *dev)
> >> dev->i2c_bus[2].i2c_period = I2C_SPEED_100K; /* 100kHz */
> >> dev->i2c_bus[2].i2c_nostop = 0;
> >> dev->i2c_bus[2].i2c_reserve = 0;
> >> + dev->i2c_bus[2].i2c_rc = -ENODEV;
> >>
> >> /* register I2C buses */
> >> errCode = cx231xx_i2c_register(&dev->i2c_bus[0]);
> >> diff --git a/drivers/media/usb/cx231xx/cx231xx-i2c.c b/drivers/media/usb/cx231xx/cx231xx-i2c.c
> >> index 3e49517cb5e0..8ce6b815d16d 100644
> >> --- a/drivers/media/usb/cx231xx/cx231xx-i2c.c
> >> +++ b/drivers/media/usb/cx231xx/cx231xx-i2c.c
> >> @@ -553,7 +553,8 @@ int cx231xx_i2c_register(struct cx231xx_i2c *bus)
> >> */
> >> void cx231xx_i2c_unregister(struct cx231xx_i2c *bus)
> >> {
> >> - i2c_del_adapter(&bus->i2c_adap);
> >> + if (!bus->i2c_rc)
> >> + i2c_del_adapter(&bus->i2c_adap);
> >
> > That doesn't sound right. what happens if i2c_rc is 1 or 2?
> >
> > IMHO, the right would would be, instead:
> >
> > if (bus->i2c_rc >= 0)
> > i2c_del_adapter(&bus->i2c_adap);
>
> In theory, yes. But in practice i2c_add_adapter never returns >0, and is
> also documented so.
Good point. Just applied the patch.
>
> Let me know if you still want an update. In that case I'll also fix the
> precedent present in the context of patch 1/3, i.e.
>
> if (0 != bus->i2c_rc)
> ...
>
> Cheers,
> peda
Thanks,
Mauro