Re: [PATCH 01/12] saa7164: Use i2c_rc properly to store i2c register status
Em 26-06-2012 13:40, Ezequiel Garcia escreveu: > Mauro, > > On Mon, Jun 25, 2012 at 5:53 PM, Mauro Carvalho Chehab > wrote: >> >> Yeah, research is needed ;) As "bttv" is the mother of the I2C code found at >> other PCI drivers, as it is one of the oldest implementations, I bet you'll >> find this field propagated without usage on some drivers (and probably other >> unused fields as well ;) ) >> > > I did some research and it looks like this patch series should be broke in > two: > - cleanup i2c_rc and i2c unused stuff > - struct i2c_algo_bit_data cleanup (already sent) > > Unless there is some problem with the latter, could you pick this patches: > (i.e, the whole series except the i2c_rc part) > cx25821: Replace struct memcpy with struct assignment > cx25821: Remove useless struct i2c_algo_bit_data usage > cx231xx: Replace struct memcpy with struct assignment > cx231xx: Remove useless struct i2c_algo_bit_data usage > cx23885: Replace struct memcpy with struct assignment > cx23885: Remove useless struct i2c_algo_bit_data > saa7164: Replace struct memcpy with struct assignment > saa7164: Remove useless struct i2c_algo_bit_data > > I'll fix a new patch series for i2c_rc cleaning. > > Hope this is not too much trouble for you. I prefer if you could re-send them with a proper description why i2c_rc is needed. Only really really trivial emails may not have descriptions ;) > Thanks, > Ezequiel. > -- 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 01/12] saa7164: Use i2c_rc properly to store i2c register status
Mauro, On Mon, Jun 25, 2012 at 5:53 PM, Mauro Carvalho Chehab wrote: > > Yeah, research is needed ;) As "bttv" is the mother of the I2C code found at > other PCI drivers, as it is one of the oldest implementations, I bet you'll > find this field propagated without usage on some drivers (and probably other > unused fields as well ;) ) > I did some research and it looks like this patch series should be broke in two: - cleanup i2c_rc and i2c unused stuff - struct i2c_algo_bit_data cleanup (already sent) Unless there is some problem with the latter, could you pick this patches: (i.e, the whole series except the i2c_rc part) cx25821: Replace struct memcpy with struct assignment cx25821: Remove useless struct i2c_algo_bit_data usage cx231xx: Replace struct memcpy with struct assignment cx231xx: Remove useless struct i2c_algo_bit_data usage cx23885: Replace struct memcpy with struct assignment cx23885: Remove useless struct i2c_algo_bit_data saa7164: Replace struct memcpy with struct assignment saa7164: Remove useless struct i2c_algo_bit_data I'll fix a new patch series for i2c_rc cleaning. Hope this is not too much trouble for you. Thanks, Ezequiel. -- 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 01/12] saa7164: Use i2c_rc properly to store i2c register status
Em 25-06-2012 17:06, Ezequiel Garcia escreveu: > On Mon, Jun 25, 2012 at 4:49 PM, Mauro Carvalho Chehab > wrote: >> >> If i2c_rc was never initialized, then just remove it. If it is required, >> then there's a bug somewhere out there on those drivers. >> >> IMHO, if the I2C bus doesn't register, any driver that requires I2C bus >> should return -ENODEV. > > Agreed. > >> >> It should be noticed that there are a few devices that don't need I2C bus >> to work: simple video grabber cards that don't have anything on their I2C. >> There are several of them at bttv, and a few at cx88 and saa7134. Maybe >> that's >> the reason why those drivers have a var to indicate if i2c got registered. > > Mmm, that would explain mysterious i2c_rc. > > Anyway, I'm still a *q-bit* unsure about which drivers require i2c to > work and which don't. > I'm gonna investigate this carefully and send a v2 (probably just to > send a v3 later :) Yeah, research is needed ;) As "bttv" is the mother of the I2C code found at other PCI drivers, as it is one of the oldest implementations, I bet you'll find this field propagated without usage on some drivers (and probably other unused fields as well ;) ) > > Thanks for reviewing, > Ezequiel. > Regards, Mauro -- 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 01/12] saa7164: Use i2c_rc properly to store i2c register status
On Mon, Jun 25, 2012 at 4:49 PM, Mauro Carvalho Chehab wrote: > > If i2c_rc was never initialized, then just remove it. If it is required, > then there's a bug somewhere out there on those drivers. > > IMHO, if the I2C bus doesn't register, any driver that requires I2C bus > should return -ENODEV. Agreed. > > It should be noticed that there are a few devices that don't need I2C bus > to work: simple video grabber cards that don't have anything on their I2C. > There are several of them at bttv, and a few at cx88 and saa7134. Maybe that's > the reason why those drivers have a var to indicate if i2c got registered. Mmm, that would explain mysterious i2c_rc. Anyway, I'm still a *q-bit* unsure about which drivers require i2c to work and which don't. I'm gonna investigate this carefully and send a v2 (probably just to send a v3 later :) Thanks for reviewing, Ezequiel. -- 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 01/12] saa7164: Use i2c_rc properly to store i2c register status
Em 25-06-2012 16:42, Ezequiel Garcia escreveu: > Hi Mauro, > > On Mon, Jun 25, 2012 at 4:29 PM, Mauro Carvalho Chehab > wrote: >>> diff --git a/drivers/media/video/saa7164/saa7164-i2c.c >>> b/drivers/media/video/saa7164/saa7164-i2c.c >>> index 26148f7..536f7dc 100644 >>> --- a/drivers/media/video/saa7164/saa7164-i2c.c >>> +++ b/drivers/media/video/saa7164/saa7164-i2c.c >>> @@ -123,7 +123,7 @@ int saa7164_i2c_register(struct saa7164_i2c *bus) >>>bus->i2c_algo.data = bus; >>>bus->i2c_adap.algo_data = bus; >>>i2c_set_adapdata(&bus->i2c_adap, bus); >>> - i2c_add_adapter(&bus->i2c_adap); >>> + bus->i2c_rc = i2c_add_adapter(&bus->i2c_adap); >>> >>>bus->i2c_client.adapter = &bus->i2c_adap; >>> >>> >> >> -ENODESCRIPTION. > > Okey. Sorry for that. > >> >> What are you intending with this change? AFAICT, i2c_add_bus_adapter() >> returns 0 on success and a negative value otherwise. Why should it be >> stored at bus->i2c_rc? > > My intention was to give i2c_rc its proper use. > I looked at bttv-i2c.c and cx88-i2c.c and (perhaps wrongly) guessed > the intended use to i2c_rc was to save i2c registration result. > > Without this patch, where is this bus->i2c_rc variable used? > Unless I've missed something, to me there are two options: > - use i2c_rc > - remove it If i2c_rc was never initialized, then just remove it. If it is required, then there's a bug somewhere out there on those drivers. IMHO, if the I2C bus doesn't register, any driver that requires I2C bus should return -ENODEV. It should be noticed that there are a few devices that don't need I2C bus to work: simple video grabber cards that don't have anything on their I2C. There are several of them at bttv, and a few at cx88 and saa7134. Maybe that's the reason why those drivers have a var to indicate if i2c got registered. > > Again sorry for lack of description, I thought it was self-explaining patch. > > If you provide some feedback about proper solution, I can resend the > patch series. Thanks! Mauro > > Thanks, > Ezequiel. > -- 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 01/12] saa7164: Use i2c_rc properly to store i2c register status
Hi Mauro, On Mon, Jun 25, 2012 at 4:29 PM, Mauro Carvalho Chehab wrote: >> diff --git a/drivers/media/video/saa7164/saa7164-i2c.c >> b/drivers/media/video/saa7164/saa7164-i2c.c >> index 26148f7..536f7dc 100644 >> --- a/drivers/media/video/saa7164/saa7164-i2c.c >> +++ b/drivers/media/video/saa7164/saa7164-i2c.c >> @@ -123,7 +123,7 @@ int saa7164_i2c_register(struct saa7164_i2c *bus) >> bus->i2c_algo.data = bus; >> bus->i2c_adap.algo_data = bus; >> i2c_set_adapdata(&bus->i2c_adap, bus); >> - i2c_add_adapter(&bus->i2c_adap); >> + bus->i2c_rc = i2c_add_adapter(&bus->i2c_adap); >> >> bus->i2c_client.adapter = &bus->i2c_adap; >> >> > > -ENODESCRIPTION. Okey. Sorry for that. > > What are you intending with this change? AFAICT, i2c_add_bus_adapter() > returns 0 on success and a negative value otherwise. Why should it be > stored at bus->i2c_rc? My intention was to give i2c_rc its proper use. I looked at bttv-i2c.c and cx88-i2c.c and (perhaps wrongly) guessed the intended use to i2c_rc was to save i2c registration result. Without this patch, where is this bus->i2c_rc variable used? Unless I've missed something, to me there are two options: - use i2c_rc - remove it Again sorry for lack of description, I thought it was self-explaining patch. If you provide some feedback about proper solution, I can resend the patch series. Thanks, Ezequiel. -- 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 01/12] saa7164: Use i2c_rc properly to store i2c register status
Em 18-06-2012 16:23, Ezequiel Garcia escreveu: > Signed-off-by: Ezequiel Garcia > --- > drivers/media/video/saa7164/saa7164-i2c.c |2 +- > 1 files changed, 1 insertions(+), 1 deletions(-) > > diff --git a/drivers/media/video/saa7164/saa7164-i2c.c > b/drivers/media/video/saa7164/saa7164-i2c.c > index 26148f7..536f7dc 100644 > --- a/drivers/media/video/saa7164/saa7164-i2c.c > +++ b/drivers/media/video/saa7164/saa7164-i2c.c > @@ -123,7 +123,7 @@ int saa7164_i2c_register(struct saa7164_i2c *bus) > bus->i2c_algo.data = bus; > bus->i2c_adap.algo_data = bus; > i2c_set_adapdata(&bus->i2c_adap, bus); > - i2c_add_adapter(&bus->i2c_adap); > + bus->i2c_rc = i2c_add_adapter(&bus->i2c_adap); > > bus->i2c_client.adapter = &bus->i2c_adap; > > -ENODESCRIPTION. What are you intending with this change? AFAICT, i2c_add_bus_adapter() returns 0 on success and a negative value otherwise. Why should it be stored at bus->i2c_rc? The same applies to the entire patch series. Regards, Mauro -- 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
[PATCH 01/12] saa7164: Use i2c_rc properly to store i2c register status
Signed-off-by: Ezequiel Garcia --- drivers/media/video/saa7164/saa7164-i2c.c |2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/drivers/media/video/saa7164/saa7164-i2c.c b/drivers/media/video/saa7164/saa7164-i2c.c index 26148f7..536f7dc 100644 --- a/drivers/media/video/saa7164/saa7164-i2c.c +++ b/drivers/media/video/saa7164/saa7164-i2c.c @@ -123,7 +123,7 @@ int saa7164_i2c_register(struct saa7164_i2c *bus) bus->i2c_algo.data = bus; bus->i2c_adap.algo_data = bus; i2c_set_adapdata(&bus->i2c_adap, bus); - i2c_add_adapter(&bus->i2c_adap); + bus->i2c_rc = i2c_add_adapter(&bus->i2c_adap); bus->i2c_client.adapter = &bus->i2c_adap; -- 1.7.4.4 -- 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
