dvb-core: how should i2c subdev drivers be attached?

dvb-core: how should i2c subdev drivers be attached?

[Akihiro TSUKADA]

Hi,
excuse me for taking up a very old post again,
but I'd like to know the status of the patch:
  https://patchwork.linuxtv.org/patch/27922/
, which provides helper code for defining/loading i2c DVB subdev drivers.

Was it rejected and 
each i2c demod/tuner drivers should provide its own version of "attach" code?
Or is it acceptable (with some modifications) ?

Although not many drivers currently use i2c binding model (and use dvb_attach()),
but I expect that coming DVB subdev drivers will have a similar attach code,
including module request/ref-counting, device creation,
(re-)using i2c_board_info.platformdata to pass around both config parameters
and the resulting i2c_client* & dvb_frontend*.

Since I have a plan to split out demod/tuner drivers from pci/pt1 dvb-usb/friio
integrated drivers (because those share the tc90522 demod driver with pt3, and
friio also shares the bridge chip with gl861),
it would be nice if I can use the helper code,
instead of re-iterating similar "attach" code.

--
akihiro

Re: dvb-core: how should i2c subdev drivers be attached?

[Mauro Carvalho Chehab]

Hi Akihiro,

Em Thu, 09 Jun 2016 21:49:33 +0900
Akihiro TSUKADA <tskd08@gmail.com> escreveu:

> Hi,
> excuse me for taking up a very old post again,
> but I'd like to know the status of the patch:
>   https://patchwork.linuxtv.org/patch/27922/
> , which provides helper code for defining/loading i2c DVB subdev drivers.
> 
> Was it rejected and 

It was not rejected. It is just that I didn't have time yet to think
about that, and Antti has a different view. 

The thing is that, whatever we do, it should work fine on drivers that
also exposes the tuner via V4L2. One of the reasons is that devices
that also allow the usage for SDR use the V4L2 core for the SDR part.

> each i2c demod/tuner drivers should provide its own version of "attach" code?

Antti took this path, but I don't like it. Lots of duplicated and complex
stuff. Also, some static analyzers refuse to check it (like smatch),
due to its complexity.

> Or is it acceptable (with some modifications) ?

I guess we should discuss a way of doing it that will be acceptable
on existing drivers. Perhaps you should try to do such change for
an hybrid driver like em28xx or cx231xx. There are a few ISDB-T
devices using them. Not sure how easy would be to find one of those
in Japan, though.

> 
> Although not many drivers currently use i2c binding model (and use dvb_attach()),
> but I expect that coming DVB subdev drivers will have a similar attach code,
> including module request/ref-counting, device creation,
> (re-)using i2c_board_info.platformdata to pass around both config parameters
> and the resulting i2c_client* & dvb_frontend*.
> 
> Since I have a plan to split out demod/tuner drivers from pci/pt1 dvb-usb/friio
> integrated drivers (because those share the tc90522 demod driver with pt3, and
> friio also shares the bridge chip with gl861),
> it would be nice if I can use the helper code,
> instead of re-iterating similar "attach" code.

Yeah, sure.

---

An unrelated thing:

I'm now helping to to maintain Kaffeine upstream. I recently added
support for both ISDB-T and DVB-T2. It would be nice if you could
add support there for ISDB-S too.

You can find the kaffeine repository at kde.org. I'm also keeping
an updated copy at linuxtv.org:
	git://anongit.kde.org/kaffeine	(official repo)
	https://git.linuxtv.org/mchehab/kaffeine.git/
	

-- 
Thanks,
Mauro

Re: dvb-core: how should i2c subdev drivers be attached?

[Antti Palosaari]

On 06/09/2016 06:24 PM, Mauro Carvalho Chehab wrote:
> Hi Akihiro,
>
> Em Thu, 09 Jun 2016 21:49:33 +0900
> Akihiro TSUKADA <tskd08@gmail.com> escreveu:
>
>> Hi,
>> excuse me for taking up a very old post again,
>> but I'd like to know the status of the patch:
>>   https://patchwork.linuxtv.org/patch/27922/
>> , which provides helper code for defining/loading i2c DVB subdev drivers.
>>
>> Was it rejected and
>
> It was not rejected. It is just that I didn't have time yet to think
> about that, and Antti has a different view.
>
> The thing is that, whatever we do, it should work fine on drivers that
> also exposes the tuner via V4L2. One of the reasons is that devices
> that also allow the usage for SDR use the V4L2 core for the SDR part.
>
>> each i2c demod/tuner drivers should provide its own version of "attach" code?
>
> Antti took this path, but I don't like it. Lots of duplicated and complex
> stuff. Also, some static analyzers refuse to check it (like smatch),
> due to its complexity.
>
>> Or is it acceptable (with some modifications) ?
>
> I guess we should discuss a way of doing it that will be acceptable
> on existing drivers. Perhaps you should try to do such change for
> an hybrid driver like em28xx or cx231xx. There are a few ISDB-T
> devices using them. Not sure how easy would be to find one of those
> in Japan, though.
>
>>
>> Although not many drivers currently use i2c binding model (and use dvb_attach()),
>> but I expect that coming DVB subdev drivers will have a similar attach code,
>> including module request/ref-counting, device creation,
>> (re-)using i2c_board_info.platformdata to pass around both config parameters
>> and the resulting i2c_client* & dvb_frontend*.
>>
>> Since I have a plan to split out demod/tuner drivers from pci/pt1 dvb-usb/friio
>> integrated drivers (because those share the tc90522 demod driver with pt3, and
>> friio also shares the bridge chip with gl861),
>> it would be nice if I can use the helper code,
>> instead of re-iterating similar "attach" code.

IMHO only thing which makes it looking complex is that module reference 
counting - otherwise it is just standard I2C binding. Ideally I2C 
modules should be possible to unbind and unload at runtime and also load 
and bind. There is "suppress_bind_attrs = true" set to prevent runtime 
unbinding and try_module_get() is to prevent module unloading. For me 
eyes all that is still some workaround - and now you want put this 
workaround to some generic code. Please find correct solutions for those 
two problems and then there we can get rid of things totally - no need 
to make generic functions at all.

regards
Antti

-- 
http://palosaari.fi/

Re: dvb-core: how should i2c subdev drivers be attached?

[Mauro Carvalho Chehab]

Em Thu, 09 Jun 2016 18:41:10 +0300
Antti Palosaari <crope@iki.fi> escreveu:

> On 06/09/2016 06:24 PM, Mauro Carvalho Chehab wrote:
> > Hi Akihiro,
> >
> > Em Thu, 09 Jun 2016 21:49:33 +0900
> > Akihiro TSUKADA <tskd08@gmail.com> escreveu:
> >  
> >> Hi,
> >> excuse me for taking up a very old post again,
> >> but I'd like to know the status of the patch:
> >>   https://patchwork.linuxtv.org/patch/27922/
> >> , which provides helper code for defining/loading i2c DVB subdev drivers.
> >>
> >> Was it rejected and  
> >
> > It was not rejected. It is just that I didn't have time yet to think
> > about that, and Antti has a different view.
> >
> > The thing is that, whatever we do, it should work fine on drivers that
> > also exposes the tuner via V4L2. One of the reasons is that devices
> > that also allow the usage for SDR use the V4L2 core for the SDR part.
> >  
> >> each i2c demod/tuner drivers should provide its own version of "attach" code?  
> >
> > Antti took this path, but I don't like it. Lots of duplicated and complex
> > stuff. Also, some static analyzers refuse to check it (like smatch),
> > due to its complexity.
> >  
> >> Or is it acceptable (with some modifications) ?  
> >
> > I guess we should discuss a way of doing it that will be acceptable
> > on existing drivers. Perhaps you should try to do such change for
> > an hybrid driver like em28xx or cx231xx. There are a few ISDB-T
> > devices using them. Not sure how easy would be to find one of those
> > in Japan, though.
> >  
> >>
> >> Although not many drivers currently use i2c binding model (and use dvb_attach()),
> >> but I expect that coming DVB subdev drivers will have a similar attach code,
> >> including module request/ref-counting, device creation,
> >> (re-)using i2c_board_info.platformdata to pass around both config parameters
> >> and the resulting i2c_client* & dvb_frontend*.
> >>
> >> Since I have a plan to split out demod/tuner drivers from pci/pt1 dvb-usb/friio
> >> integrated drivers (because those share the tc90522 demod driver with pt3, and
> >> friio also shares the bridge chip with gl861),
> >> it would be nice if I can use the helper code,
> >> instead of re-iterating similar "attach" code.  
> 
> IMHO only thing which makes it looking complex is that module reference 
> counting - otherwise it is just standard I2C binding. Ideally I2C 
> modules should be possible to unbind and unload at runtime and also load 
> and bind. There is "suppress_bind_attrs = true" set to prevent runtime 
> unbinding and try_module_get() is to prevent module unloading. For me 
> eyes all that is still some workaround - and now you want put this 
> workaround to some generic code. Please find correct solutions for those 
> two problems and then there we can get rid of things totally - no need 
> to make generic functions at all.

It *is complex*. A single board binding on the way you're mapping is:

	case EM28174_BOARD_PCTV_460E: {
		struct i2c_client *client;
		struct i2c_board_info board_info;
		struct tda10071_platform_data tda10071_pdata = {};
		struct a8293_platform_data a8293_pdata = {};

		/* attach demod + tuner combo */
		tda10071_pdata.clk = 40444000, /* 40.444 MHz */
		tda10071_pdata.i2c_wr_max = 64,
		tda10071_pdata.ts_mode = TDA10071_TS_SERIAL,
		tda10071_pdata.pll_multiplier = 20,
		tda10071_pdata.tuner_i2c_addr = 0x14,
		memset(&board_info, 0, sizeof(board_info));
		strlcpy(board_info.type, "tda10071_cx24118", I2C_NAME_SIZE);
		board_info.addr = 0x55;
		board_info.platform_data = &tda10071_pdata;
		request_module("tda10071");
		client = i2c_new_device(&dev->i2c_adap[dev->def_i2c_bus], &board_info);
		if (client == NULL || client->dev.driver == NULL) {
			result = -ENODEV;
			goto out_free;
		}
		if (!try_module_get(client->dev.driver->owner)) {
			i2c_unregister_device(client);
			result = -ENODEV;
			goto out_free;
		}
		dvb->fe[0] = tda10071_pdata.get_dvb_frontend(client);
		dvb->i2c_client_demod = client;

		/* attach SEC */
		a8293_pdata.dvb_frontend = dvb->fe[0];
		memset(&board_info, 0, sizeof(board_info));
		strlcpy(board_info.type, "a8293", I2C_NAME_SIZE);
		board_info.addr = 0x08;
		board_info.platform_data = &a8293_pdata;
		request_module("a8293");
		client = i2c_new_device(&dev->i2c_adap[dev->def_i2c_bus], &board_info);
		if (client == NULL || client->dev.driver == NULL) {
			module_put(dvb->i2c_client_demod->dev.driver->owner);
			i2c_unregister_device(dvb->i2c_client_demod);
			result = -ENODEV;
			goto out_free;
		}
		if (!try_module_get(client->dev.driver->owner)) {
			i2c_unregister_device(client);
			module_put(dvb->i2c_client_demod->dev.driver->owner);
			i2c_unregister_device(dvb->i2c_client_demod);
			result = -ENODEV;
			goto out_free;
		}
		dvb->i2c_client_sec = client;
		break;
	}

So, 55 lines of code, plus an extra logic to dettach it on this random
example (the first occurrence of the i2c binding thing at the em28xx
driver).

At the V4L2 side, what we do, instead, is a single function call, for
each I2C driver that should be attached:

	v4l2_i2c_new_subdev(&v4l2->v4l2_dev,
			    &dev->i2c_adap[dev->def_i2c_bus],
			    "tuner", tuner_addr, NULL);

The V4L2 core handles everything that it is needed for it to work, and
no extra code is needed to do module_put() or i2c_unregister_device().

-- 
Thanks,
Mauro

Re: dvb-core: how should i2c subdev drivers be attached?

[Antti Palosaari]

On 06/09/2016 07:18 PM, Mauro Carvalho Chehab wrote:
> Em Thu, 09 Jun 2016 18:41:10 +0300
> Antti Palosaari <crope@iki.fi> escreveu:
>
>> On 06/09/2016 06:24 PM, Mauro Carvalho Chehab wrote:
>>> Hi Akihiro,
>>>
>>> Em Thu, 09 Jun 2016 21:49:33 +0900
>>> Akihiro TSUKADA <tskd08@gmail.com> escreveu:
>>>
>>>> Hi,
>>>> excuse me for taking up a very old post again,
>>>> but I'd like to know the status of the patch:
>>>>   https://patchwork.linuxtv.org/patch/27922/
>>>> , which provides helper code for defining/loading i2c DVB subdev drivers.
>>>>
>>>> Was it rejected and
>>>
>>> It was not rejected. It is just that I didn't have time yet to think
>>> about that, and Antti has a different view.
>>>
>>> The thing is that, whatever we do, it should work fine on drivers that
>>> also exposes the tuner via V4L2. One of the reasons is that devices
>>> that also allow the usage for SDR use the V4L2 core for the SDR part.
>>>
>>>> each i2c demod/tuner drivers should provide its own version of "attach" code?
>>>
>>> Antti took this path, but I don't like it. Lots of duplicated and complex
>>> stuff. Also, some static analyzers refuse to check it (like smatch),
>>> due to its complexity.
>>>
>>>> Or is it acceptable (with some modifications) ?
>>>
>>> I guess we should discuss a way of doing it that will be acceptable
>>> on existing drivers. Perhaps you should try to do such change for
>>> an hybrid driver like em28xx or cx231xx. There are a few ISDB-T
>>> devices using them. Not sure how easy would be to find one of those
>>> in Japan, though.
>>>
>>>>
>>>> Although not many drivers currently use i2c binding model (and use dvb_attach()),
>>>> but I expect that coming DVB subdev drivers will have a similar attach code,
>>>> including module request/ref-counting, device creation,
>>>> (re-)using i2c_board_info.platformdata to pass around both config parameters
>>>> and the resulting i2c_client* & dvb_frontend*.
>>>>
>>>> Since I have a plan to split out demod/tuner drivers from pci/pt1 dvb-usb/friio
>>>> integrated drivers (because those share the tc90522 demod driver with pt3, and
>>>> friio also shares the bridge chip with gl861),
>>>> it would be nice if I can use the helper code,
>>>> instead of re-iterating similar "attach" code.
>>
>> IMHO only thing which makes it looking complex is that module reference
>> counting - otherwise it is just standard I2C binding. Ideally I2C
>> modules should be possible to unbind and unload at runtime and also load
>> and bind. There is "suppress_bind_attrs = true" set to prevent runtime
>> unbinding and try_module_get() is to prevent module unloading. For me
>> eyes all that is still some workaround - and now you want put this
>> workaround to some generic code. Please find correct solutions for those
>> two problems and then there we can get rid of things totally - no need
>> to make generic functions at all.
>
> It *is complex*. A single board binding on the way you're mapping is:
>
> 	case EM28174_BOARD_PCTV_460E: {
> 		struct i2c_client *client;
> 		struct i2c_board_info board_info;
> 		struct tda10071_platform_data tda10071_pdata = {};
> 		struct a8293_platform_data a8293_pdata = {};
>
> 		/* attach demod + tuner combo */
> 		tda10071_pdata.clk = 40444000, /* 40.444 MHz */
> 		tda10071_pdata.i2c_wr_max = 64,
> 		tda10071_pdata.ts_mode = TDA10071_TS_SERIAL,
> 		tda10071_pdata.pll_multiplier = 20,
> 		tda10071_pdata.tuner_i2c_addr = 0x14,
> 		memset(&board_info, 0, sizeof(board_info));
> 		strlcpy(board_info.type, "tda10071_cx24118", I2C_NAME_SIZE);
> 		board_info.addr = 0x55;
> 		board_info.platform_data = &tda10071_pdata;
> 		request_module("tda10071");
> 		client = i2c_new_device(&dev->i2c_adap[dev->def_i2c_bus], &board_info);
> 		if (client == NULL || client->dev.driver == NULL) {
> 			result = -ENODEV;
> 			goto out_free;
> 		}
> 		if (!try_module_get(client->dev.driver->owner)) {
> 			i2c_unregister_device(client);
> 			result = -ENODEV;
> 			goto out_free;
> 		}
> 		dvb->fe[0] = tda10071_pdata.get_dvb_frontend(client);
> 		dvb->i2c_client_demod = client;
>
> 		/* attach SEC */
> 		a8293_pdata.dvb_frontend = dvb->fe[0];
> 		memset(&board_info, 0, sizeof(board_info));
> 		strlcpy(board_info.type, "a8293", I2C_NAME_SIZE);
> 		board_info.addr = 0x08;
> 		board_info.platform_data = &a8293_pdata;
> 		request_module("a8293");
> 		client = i2c_new_device(&dev->i2c_adap[dev->def_i2c_bus], &board_info);
> 		if (client == NULL || client->dev.driver == NULL) {
> 			module_put(dvb->i2c_client_demod->dev.driver->owner);
> 			i2c_unregister_device(dvb->i2c_client_demod);
> 			result = -ENODEV;
> 			goto out_free;
> 		}
> 		if (!try_module_get(client->dev.driver->owner)) {
> 			i2c_unregister_device(client);
> 			module_put(dvb->i2c_client_demod->dev.driver->owner);
> 			i2c_unregister_device(dvb->i2c_client_demod);
> 			result = -ENODEV;
> 			goto out_free;
> 		}
> 		dvb->i2c_client_sec = client;
> 		break;
> 	}
>
> So, 55 lines of code, plus an extra logic to dettach it on this random
> example (the first occurrence of the i2c binding thing at the em28xx
> driver).
>
> At the V4L2 side, what we do, instead, is a single function call, for
> each I2C driver that should be attached:
>
> 	v4l2_i2c_new_subdev(&v4l2->v4l2_dev,
> 			    &dev->i2c_adap[dev->def_i2c_bus],
> 			    "tuner", tuner_addr, NULL);
>
> The V4L2 core handles everything that it is needed for it to work, and
> no extra code is needed to do module_put() or i2c_unregister_device().

That example attachs 2 I2C drivers, as your example only 1. Also it 
populates all the config to platform data on both I2C driver. Which 
annoys me is that try_module_get/module_put functionality.

You should be ideally able to unbind (and bind) modules like that:
echo 6-0008 > /sys/bus/i2c/drivers/a8293/unbind

and as it is not possible, that stuff is here to avoid problems. Some 
study is needed in order to find out how dynamic unbind/bind could be 
get working and after that I hope whole ref counting could be removed. 
Currently you cannot allow remove module as it leads to unbind, which 
does not work.

regards
Antti

-- 
http://palosaari.fi/

Re: dvb-core: how should i2c subdev drivers be attached?

[Akihiro TSUKADA]

thanks for the replys.

so, the new i2c "attach" helper code should
1. be usable for V4L2-DVB multi-functional(hybrid) tuner devices
   (or at least co-exist well with them),
2. properly ref-count driver module
   to prevent (accidental) 'unload before use' by users.
3. a. block un-binding of the device while in use,
   b. support run-time {un-,}binding of the devices
      via sysfs by users (to switch drivers?).
Is this right?

> I guess we should discuss a way of doing it that will be acceptable
> on existing drivers. Perhaps you should try to do such change for
> an hybrid driver like em28xx or cx231xx. There are a few ISDB-T
> devices using them. Not sure how easy would be to find one of those
> in Japan, though.

I'll look into those drivers' code, to begin with.
(I'm pretty sure those devices are hardly found here in Japan).

> I'm now helping to to maintain Kaffeine upstream. I recently added
> support for both ISDB-T and DVB-T2. It would be nice if you could
> add support there for ISDB-S too.

All the channels in ISDB-S are scrambled,
unlike ISDB-T(jp) where non-scrambled '1-seg' channels are delivered.
In Japan, it will be considered illegal to desramble them with
a non-authorized software that lacks private copy-guard encryption.
So, unfortunately, there will be no legitimate OSS player for ISDB-S.

regards,
akihiro

Re: dvb-core: how should i2c subdev drivers be attached?

[Mauro Carvalho Chehab]

Em Thu, 9 Jun 2016 19:38:04 +0300
Antti Palosaari <crope@iki.fi> escreveu:

> On 06/09/2016 07:18 PM, Mauro Carvalho Chehab wrote:
> > Em Thu, 09 Jun 2016 18:41:10 +0300
> > Antti Palosaari <crope@iki.fi> escreveu:
> >  
> >> On 06/09/2016 06:24 PM, Mauro Carvalho Chehab wrote:  
> >>> Hi Akihiro,
> >>>
> >>> Em Thu, 09 Jun 2016 21:49:33 +0900
> >>> Akihiro TSUKADA <tskd08@gmail.com> escreveu:
> >>>  
> >>>> Hi,
> >>>> excuse me for taking up a very old post again,
> >>>> but I'd like to know the status of the patch:
> >>>>   https://patchwork.linuxtv.org/patch/27922/
> >>>> , which provides helper code for defining/loading i2c DVB subdev drivers.
> >>>>
> >>>> Was it rejected and  
> >>>
> >>> It was not rejected. It is just that I didn't have time yet to think
> >>> about that, and Antti has a different view.
> >>>
> >>> The thing is that, whatever we do, it should work fine on drivers that
> >>> also exposes the tuner via V4L2. One of the reasons is that devices
> >>> that also allow the usage for SDR use the V4L2 core for the SDR part.
> >>>  
> >>>> each i2c demod/tuner drivers should provide its own version of "attach" code?  
> >>>
> >>> Antti took this path, but I don't like it. Lots of duplicated and complex
> >>> stuff. Also, some static analyzers refuse to check it (like smatch),
> >>> due to its complexity.
> >>>  
> >>>> Or is it acceptable (with some modifications) ?  
> >>>
> >>> I guess we should discuss a way of doing it that will be acceptable
> >>> on existing drivers. Perhaps you should try to do such change for
> >>> an hybrid driver like em28xx or cx231xx. There are a few ISDB-T
> >>> devices using them. Not sure how easy would be to find one of those
> >>> in Japan, though.
> >>>  
> >>>>
> >>>> Although not many drivers currently use i2c binding model (and use dvb_attach()),
> >>>> but I expect that coming DVB subdev drivers will have a similar attach code,
> >>>> including module request/ref-counting, device creation,
> >>>> (re-)using i2c_board_info.platformdata to pass around both config parameters
> >>>> and the resulting i2c_client* & dvb_frontend*.
> >>>>
> >>>> Since I have a plan to split out demod/tuner drivers from pci/pt1 dvb-usb/friio
> >>>> integrated drivers (because those share the tc90522 demod driver with pt3, and
> >>>> friio also shares the bridge chip with gl861),
> >>>> it would be nice if I can use the helper code,
> >>>> instead of re-iterating similar "attach" code.  
> >>
> >> IMHO only thing which makes it looking complex is that module reference
> >> counting - otherwise it is just standard I2C binding. Ideally I2C
> >> modules should be possible to unbind and unload at runtime and also load
> >> and bind. There is "suppress_bind_attrs = true" set to prevent runtime
> >> unbinding and try_module_get() is to prevent module unloading. For me
> >> eyes all that is still some workaround - and now you want put this
> >> workaround to some generic code. Please find correct solutions for those
> >> two problems and then there we can get rid of things totally - no need
> >> to make generic functions at all.  
> >
> > It *is complex*. A single board binding on the way you're mapping is:
> >
> > 	case EM28174_BOARD_PCTV_460E: {
> > 		struct i2c_client *client;
> > 		struct i2c_board_info board_info;
> > 		struct tda10071_platform_data tda10071_pdata = {};
> > 		struct a8293_platform_data a8293_pdata = {};
> >
> > 		/* attach demod + tuner combo */
> > 		tda10071_pdata.clk = 40444000, /* 40.444 MHz */
> > 		tda10071_pdata.i2c_wr_max = 64,
> > 		tda10071_pdata.ts_mode = TDA10071_TS_SERIAL,
> > 		tda10071_pdata.pll_multiplier = 20,
> > 		tda10071_pdata.tuner_i2c_addr = 0x14,
> > 		memset(&board_info, 0, sizeof(board_info));
> > 		strlcpy(board_info.type, "tda10071_cx24118", I2C_NAME_SIZE);
> > 		board_info.addr = 0x55;
> > 		board_info.platform_data = &tda10071_pdata;
> > 		request_module("tda10071");
> > 		client = i2c_new_device(&dev->i2c_adap[dev->def_i2c_bus], &board_info);
> > 		if (client == NULL || client->dev.driver == NULL) {
> > 			result = -ENODEV;
> > 			goto out_free;
> > 		}
> > 		if (!try_module_get(client->dev.driver->owner)) {
> > 			i2c_unregister_device(client);
> > 			result = -ENODEV;
> > 			goto out_free;
> > 		}
> > 		dvb->fe[0] = tda10071_pdata.get_dvb_frontend(client);
> > 		dvb->i2c_client_demod = client;
> >
> > 		/* attach SEC */
> > 		a8293_pdata.dvb_frontend = dvb->fe[0];
> > 		memset(&board_info, 0, sizeof(board_info));
> > 		strlcpy(board_info.type, "a8293", I2C_NAME_SIZE);
> > 		board_info.addr = 0x08;
> > 		board_info.platform_data = &a8293_pdata;
> > 		request_module("a8293");
> > 		client = i2c_new_device(&dev->i2c_adap[dev->def_i2c_bus], &board_info);
> > 		if (client == NULL || client->dev.driver == NULL) {
> > 			module_put(dvb->i2c_client_demod->dev.driver->owner);
> > 			i2c_unregister_device(dvb->i2c_client_demod);
> > 			result = -ENODEV;
> > 			goto out_free;
> > 		}
> > 		if (!try_module_get(client->dev.driver->owner)) {
> > 			i2c_unregister_device(client);
> > 			module_put(dvb->i2c_client_demod->dev.driver->owner);
> > 			i2c_unregister_device(dvb->i2c_client_demod);
> > 			result = -ENODEV;
> > 			goto out_free;
> > 		}
> > 		dvb->i2c_client_sec = client;
> > 		break;
> > 	}
> >
> > So, 55 lines of code, plus an extra logic to dettach it on this random
> > example (the first occurrence of the i2c binding thing at the em28xx
> > driver).
> >
> > At the V4L2 side, what we do, instead, is a single function call, for
> > each I2C driver that should be attached:
> >
> > 	v4l2_i2c_new_subdev(&v4l2->v4l2_dev,
> > 			    &dev->i2c_adap[dev->def_i2c_bus],
> > 			    "tuner", tuner_addr, NULL);
> >
> > The V4L2 core handles everything that it is needed for it to work, and
> > no extra code is needed to do module_put() or i2c_unregister_device().  
> 
> That example attachs 2 I2C drivers, as your example only 1.

Well, on V4L2, 2 I2C drivers, two statements.

> Also it 
> populates all the config to platform data on both I2C driver. 

Yes, this is annoying, but lots of the converted entries are
doing the same crap, instead of using a const var outside
the code.

> Which 
> annoys me is that try_module_get/module_put functionality.

That is scary, as any failure there would prevent removing/unbinding
a module. The core or some helper function should be handle it,
to avoid the risk of get twice, put twice, never call put, etc.

> You should be ideally able to unbind (and bind) modules like that:
> echo 6-0008 > /sys/bus/i2c/drivers/a8293/unbind

I guess unbinding a V4L2 module in real time won't cause any
crash (obviously, the device will stop work properly, if you
remove a component that it is being used).

I actually tested remove/reinsert the I2C remote controller
drivers a long time ago, while looking at some bugs. Those are
usually harder to get it right, as most of them have a poll logic
internally to get IR events on every 10ms. I guess I tested 
removing/reinserting the tuner too, but that was at the 
"stone ages"... to old for me to remember what I did.

Yet, I don't see any troubles preventing the I2C "slave" drivers to
be unbound before the master, by increasing their module refcounts
during their usage.

> and as it is not possible, that stuff is here to avoid problems. Some 
> study is needed in order to find out how dynamic unbind/bind could be 
> get working and after that I hope whole ref counting could be removed. 
> Currently you cannot allow remove module as it leads to unbind, which 
> does not work.
> 
> regards
> Antti
> 


-- 
Thanks,
Mauro

Re: dvb-core: how should i2c subdev drivers be attached?

[Antti Palosaari]

On 06/09/2016 09:30 PM, Mauro Carvalho Chehab wrote:
> Em Thu, 9 Jun 2016 19:38:04 +0300
> Antti Palosaari <crope@iki.fi> escreveu:

>>> The V4L2 core handles everything that it is needed for it to work, and
>>> no extra code is needed to do module_put() or i2c_unregister_device().
>>
>> That example attachs 2 I2C drivers, as your example only 1.
>
> Well, on V4L2, 2 I2C drivers, two statements.
>
>> Also it
>> populates all the config to platform data on both I2C driver.
>
> Yes, this is annoying, but lots of the converted entries are
> doing the same crap, instead of using a const var outside
> the code.
>
>> Which
>> annoys me is that try_module_get/module_put functionality.
>
> That is scary, as any failure there would prevent removing/unbinding
> a module. The core or some helper function should be handle it,
> to avoid the risk of get twice, put twice, never call put, etc.
>
>> You should be ideally able to unbind (and bind) modules like that:
>> echo 6-0008 > /sys/bus/i2c/drivers/a8293/unbind
>
> I guess unbinding a V4L2 module in real time won't cause any
> crash (obviously, the device will stop work properly, if you
> remove a component that it is being used).
>
> I actually tested remove/reinsert the I2C remote controller
> drivers a long time ago, while looking at some bugs. Those are
> usually harder to get it right, as most of them have a poll logic
> internally to get IR events on every 10ms. I guess I tested
> removing/reinserting the tuner too, but that was at the
> "stone ages"... to old for me to remember what I did.
>
> Yet, I don't see any troubles preventing the I2C "slave" drivers to
> be unbound before the master, by increasing their module refcounts
> during their usage.
>
>> and as it is not possible, that stuff is here to avoid problems. Some
>> study is needed in order to find out how dynamic unbind/bind could be
>> get working and after that I hope whole ref counting could be removed.
>> Currently you cannot allow remove module as it leads to unbind, which
>> does not work.

I did tons of work in order to get things work properly with I2C 
binding. And following things are now possible due to that:
* Kernel logging. You could now use standard dev_ logging.
* regmap. Could now use regmap in order to cover register access.
* I2C-mux. No need for i2c_gate_control.

And everytime there is someone asking why just don't do things like 
earlier :S

I really don't want add any new hacks but implement things as much as 
possible the way driver core makes possible. For long ran I feel it is 
better approach to follow driver core than make own hacks. Until someone 
study things and says it is not possible to implement things like core 
offers, then lets implement things. That's bind/unbind is one thing to 
study, another thing is power-management.

I suspect bind/unbind could be simple like just:
i2c_driver_remove()
{
     if (frontend_is_running)
         return -EBUSY;

     kfree(dev)
     return 0;
}

regards
Antti

-- 
http://palosaari.fi/

Re: dvb-core: how should i2c subdev drivers be attached?

[Mauro Carvalho Chehab]

Antti,

Em Thu, 09 Jun 2016 22:14:12 +0300
Antti Palosaari <crope@iki.fi> escreveu:

> On 06/09/2016 09:30 PM, Mauro Carvalho Chehab wrote:
> > Em Thu, 9 Jun 2016 19:38:04 +0300
> > Antti Palosaari <crope@iki.fi> escreveu:  
> 
> >>> The V4L2 core handles everything that it is needed for it to work, and
> >>> no extra code is needed to do module_put() or i2c_unregister_device().  
> >>
> >> That example attachs 2 I2C drivers, as your example only 1.  
> >
> > Well, on V4L2, 2 I2C drivers, two statements.
> >  
> >> Also it
> >> populates all the config to platform data on both I2C driver.  
> >
> > Yes, this is annoying, but lots of the converted entries are
> > doing the same crap, instead of using a const var outside
> > the code.
> >  
> >> Which
> >> annoys me is that try_module_get/module_put functionality.  
> >
> > That is scary, as any failure there would prevent removing/unbinding
> > a module. The core or some helper function should be handle it,
> > to avoid the risk of get twice, put twice, never call put, etc.
> >  
> >> You should be ideally able to unbind (and bind) modules like that:
> >> echo 6-0008 > /sys/bus/i2c/drivers/a8293/unbind  
> >
> > I guess unbinding a V4L2 module in real time won't cause any
> > crash (obviously, the device will stop work properly, if you
> > remove a component that it is being used).
> >
> > I actually tested remove/reinsert the I2C remote controller
> > drivers a long time ago, while looking at some bugs. Those are
> > usually harder to get it right, as most of them have a poll logic
> > internally to get IR events on every 10ms. I guess I tested
> > removing/reinserting the tuner too, but that was at the
> > "stone ages"... to old for me to remember what I did.
> >
> > Yet, I don't see any troubles preventing the I2C "slave" drivers to
> > be unbound before the master, by increasing their module refcounts
> > during their usage.
> >  
> >> and as it is not possible, that stuff is here to avoid problems. Some
> >> study is needed in order to find out how dynamic unbind/bind could be
> >> get working and after that I hope whole ref counting could be removed.
> >> Currently you cannot allow remove module as it leads to unbind, which
> >> does not work.  
> 
> I did tons of work in order to get things work properly with I2C 
> binding. And following things are now possible due to that:
> * Kernel logging. You could now use standard dev_ logging.
> * regmap. Could now use regmap in order to cover register access.
> * I2C-mux. No need for i2c_gate_control.
> 
> And everytime there is someone asking why just don't do things like 
> earlier :S
> 
> I really don't want add any new hacks but implement things as much as 
> possible the way driver core makes possible. For long ran I feel it is 
> better approach to follow driver core than make own hacks. Until someone 
> study things and says it is not possible to implement things like core 
> offers, then lets implement things. That's bind/unbind is one thing to 
> study, another thing is power-management.

Nobody is proposing to add hacks. I'm with you with that: hacks
should be removed (like that hybrid_instance ugly code used by most
hybrid tuners).

We should, however, put common code at the core or provide helper
functions, in order to:

1) Make life easier for developers to add support for new boards;

2) Prevent, as much as possible, the risk of developer's mistakes
   to cause harm to the drivers;

3) Simplify the logic at the drivers, and, if possible, remove that
   long per-card switch() at the dvb part of the hybrid drivers;

4) Prevent, as much as possible, the risk of developer's mistakes
   to cause harm to the drivers;

5) Allow the code to be better reviewed by static code analyzers.

> 
> I suspect bind/unbind could be simple like just:
> i2c_driver_remove()
> {
>      if (frontend_is_running)
>          return -EBUSY;
> 
>      kfree(dev)
>      return 0;
> }

The above code is racy, as some other request to the frontend
may arrive between the if() statement and kfree(). A kref would
likely be safer.

Thanks,
Mauro

Re: dvb-core: how should i2c subdev drivers be attached?

[Antti Palosaari]

On 06/09/2016 10:35 PM, Mauro Carvalho Chehab wrote:
> The above code is racy, as some other request to the frontend
> may arrive between the if() statement and kfree(). A kref would
> likely be safer.

Here is working proof-of-concept hack. It is for si2157 tuner module. It 
prevents module unbind and unload when module is active. Of course it is 
not safe. Some more work needed in order to allow runtime bind (I tested 
it earlier with a8293 and unbind + bind was possible for deactive module 
at that time).

https://git.linuxtv.org/anttip/media_tree.git/log/?h=unbind_poc

regards
Antti

-- 
http://palosaari.fi/