Re: [PATCH v2 4/5] tc90522: add driver for Toshiba TC90522 quad demodulator
Moi!
Yes, using the I2C binding way provides a better decoupling than using the
legacy way. The current dvb_attach() macros are hacks that were created
by the time where the I2C standard bind didn't work with DVB.
I understand. I converted my code to use i2c binding model,
but I'm uncertain about a few things.
1. How to load the modules of i2c driver?
Currently I use request_module()/module_put()
like an example (ddbrige-core.c) from Antti does,
but I'd prefer implicit module loading/ref-counting
like in dvb_attach() if it exists.
2. Is there a standard way to pass around dvb_frontend*, i2c_client*,
regmap* between bridge(dvb_adapter) and demod/tuner drivers?
Currently I use config structure for the purpose, which is set to
dev.platform_data (via i2c_board_info/i2c_new_device()) or
dev.driver_data (via i2c_{get,set}_clientdata()),
but using config as both IN/OUT looks a bit hacky.
3. Should I also use RegMap API for register access?
I tried using it but gave up,
because it does not fit well to one of my use-case,
where (only) reads must be done via 0xfb register, like
READ(reg, buf, len) - [addr/w, 0xfb, reg], [addr/r, buf[0]...],
WRITE(reg, buf, len) - [addr/w, reg, buf[0]...],
and regmap looked to me overkill for 8bit-reg, 8bit-val cases
and did not simplify the code.
so I'd like to go without RegMap if possible,
since I'm already puzzled enough by I2C binding, regmap, clock source,
as well as dvb-core, PCI ;)
regards,
Akihiro
--
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 v2 4/5] tc90522: add driver for Toshiba TC90522 quad demodulator
moikka!
On 09/06/2014 09:09 AM, Akihiro TSUKADA wrote:
Moi!
Yes, using the I2C binding way provides a better decoupling than using the
legacy way. The current dvb_attach() macros are hacks that were created
by the time where the I2C standard bind didn't work with DVB.
I understand. I converted my code to use i2c binding model,
but I'm uncertain about a few things.
1. How to load the modules of i2c driver?
Currently I use request_module()/module_put()
like an example (ddbrige-core.c) from Antti does,
but I'd prefer implicit module loading/ref-counting
like in dvb_attach() if it exists.
Maybe I haven't found the best way yet, but that was implementation I
made for AF9035 driver:
https://patchwork.linuxtv.org/patch/25764/
Basically it is 2 functions, af9035_add_i2c_dev() and af9035_del_i2c_dev()
2. Is there a standard way to pass around dvb_frontend*, i2c_client*,
regmap* between bridge(dvb_adapter) and demod/tuner drivers?
Currently I use config structure for the purpose, which is set to
dev.platform_data (via i2c_board_info/i2c_new_device()) or
dev.driver_data (via i2c_{get,set}_clientdata()),
but using config as both IN/OUT looks a bit hacky.
In my understanding, platform_data is place to pass environment specific
config to driver. get/set client_data() is aimed to carry pointer for
device specific instance state inside a driver. Is it possible to set
I2C client data before calling i2c_new_device() and pass pointer to driver?
There is also IOCTL style command for I2C, but it is legacy and should
not be used.
Documentation/i2c/busses/i2c-ocores
Yet, using config to OUT seems to be bit hacky for my eyes too. I though
replacing all OUT with ops when converted af9033 driver. Currently
caller fills struct af9033_ops and then af9033 I2C probe populates ops.
See that patch:
https://patchwork.linuxtv.org/patch/25746/
Does this kind of ops sounds any better?
EXPORT_SYMBOL() is easiest way to offer outputs, like
EXPORT_SYMBOL(get_frontend), EXPORT_SYMBOL(get_i2c_adapter). But we want
avoid those exported symbols.
3. Should I also use RegMap API for register access?
I tried using it but gave up,
because it does not fit well to one of my use-case,
where (only) reads must be done via 0xfb register, like
READ(reg, buf, len) - [addr/w, 0xfb, reg], [addr/r, buf[0]...],
WRITE(reg, buf, len) - [addr/w, reg, buf[0]...],
and regmap looked to me overkill for 8bit-reg, 8bit-val cases
and did not simplify the code.
so I'd like to go without RegMap if possible,
since I'm already puzzled enough by I2C binding, regmap, clock source,
as well as dvb-core, PCI ;)
I prefer RegMap API, but I am only one who has started using it yet. And
I haven't converted any demod driver having I2C bus/gate/repeater for
tuner to that API. It is because of I2C locking with I2C mux adapter,
you need use unlocked version of i2c_transfer() for I2C mux as I2C bus
lock is already taken. RegMap API does not support that, but I think it
should be possible if you implement own xfer callback for regmap. For RF
tuners RegMap API should be trivial and it will reduce ~100 LOC from driver.
But if you decide avoid RegMap API, I ask you add least implementing
those I2C read / write function parameters similarly than RegMap API
does. Defining all kind of weird register write / read functions makes
life harder. I converted recently IT913x driver to RegMap API and
biggest pain was there very different register read / write routines. So
I need to do a lot of work in order convert functions first some common
style and then it was trivial to change RegMap API.
https://patchwork.linuxtv.org/patch/25766/
https://patchwork.linuxtv.org/patch/25757/
I quickly overlooked that demod driver and one which looked crazy was
LNA stuff. You implement set_lna callback in demod, but it is then
passed back to PCI driver using frontend callback. Is there some reason
you hooked it via demod? You could implement set_lna in PCI driver too.
regards
Antti
--
http://palosaari.fi/
--
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 v2 4/5] tc90522: add driver for Toshiba TC90522 quad demodulator
On 09/06/2014 09:09 AM, Akihiro TSUKADA wrote: 3. Should I also use RegMap API for register access? I tried using it but gave up, because it does not fit well to one of my use-case, where (only) reads must be done via 0xfb register, like READ(reg, buf, len) - [addr/w, 0xfb, reg], [addr/r, buf[0]...], WRITE(reg, buf, len) - [addr/w, reg, buf[0]...], and regmap looked to me overkill for 8bit-reg, 8bit-val cases and did not simplify the code. so I'd like to go without RegMap if possible, since I'm already puzzled enough by I2C binding, regmap, clock source, as well as dvb-core, PCI ;) That is MaxLinear MxL301RF tuner I2C. Problem is there that it uses write + STOP + write, so you should not even do that as a one I2C i2c_transfer. All I2C messages send using i2c_transfer are send so called REPEATED START condition. I ran that same problem ears ago in a case of, surprise, MxL5007 tuner. https://patchwork.linuxtv.org/patch/17847/ I think you could just write wanted register to command register 0xfb. And after that all the reads are coming from that active register until you change it. RegMap API cannot handle I2C command format like that, it relies repeated start for reads. Si2157 / Si2168 are using I2C access with STOP condition - but it is otherwise bad example as there is firmware API, not register API. Look still as a example. regards Antti -- http://palosaari.fi/ -- 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 v2 4/5] tc90522: add driver for Toshiba TC90522 quad demodulator
moikka!, Basically it is 2 functions, af9035_add_i2c_dev() and af9035_del_i2c_dev() I used request_module()/try_module_get()/module_put() just like the above example (and bridge-core.c). It works, but when I unload bridge driver(earth_pt3), its demod and tuner modules stay loaded, with the refcount of 0. Is it ok that the auto loaded modules remain with 0 ref count? Yet, using config to OUT seems to be bit hacky for my eyes too. I though replacing all OUT with ops when converted af9033 driver. Currently caller fills struct af9033_ops and then af9033 I2C probe populates ops. See that patch: https://patchwork.linuxtv.org/patch/25746/ Does this kind of ops sounds any better? Do you mean using ops in struct config? if so, I don't find much difference with the current situation where demod/tuner probe() sets dvb_frontend* to config-fe. I quickly overlooked that demod driver and one which looked crazy was LNA stuff. You implement set_lna callback in demod, but it is then passed back to PCI driver using frontend callback. Is there some reason you hooked it via demod? You could implement set_lna in PCI driver too. Stupidly I forgot that FE's ops can be set from the PCI driver. I will remove those callbacks and set the corresponding ops instead. regards, akihiro -- 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 v2 4/5] tc90522: add driver for Toshiba TC90522 quad demodulator
On 09/06/2014 10:35 PM, Akihiro TSUKADA wrote: moikka!, Basically it is 2 functions, af9035_add_i2c_dev() and af9035_del_i2c_dev() I used request_module()/try_module_get()/module_put() just like the above example (and bridge-core.c). It works, but when I unload bridge driver(earth_pt3), its demod and tuner modules stay loaded, with the refcount of 0. Is it ok that the auto loaded modules remain with 0 ref count? So there is no other problem than those modules were left loaded? If you could unload those using rmmod it is OK then. Ref counting is here to prevent unloading demod and tuner driver while those are used by some other module. So when bridge is loaded, you should not be able to unload demod or tuner. But when bridge is unloaded, you should be able to unload demod and tuner. And your question, I think there is no way to unload modules automatically or at least no need. Yet, using config to OUT seems to be bit hacky for my eyes too. I though replacing all OUT with ops when converted af9033 driver. Currently caller fills struct af9033_ops and then af9033 I2C probe populates ops. See that patch: https://patchwork.linuxtv.org/patch/25746/ Does this kind of ops sounds any better? Do you mean using ops in struct config? if so, I don't find much difference with the current situation where demod/tuner probe() sets dvb_frontend* to config-fe. Alloc driver specific ops in bridge driver, then put pointer to that ops to config struct. Driver fills ops during probe. Maybe that patch clears the idea: af9033: Don't export functions for the hardware filter https://patchwork.linuxtv.org/patch/23087/ Functionality is not much different than passing pointer to frontend pointer from bridge to I2C demod driver via platform_data... Somehow you will need to transfer data during driver loading and there is not many alternatives: * platform data pointer * driver data pointer * export function * i2c_clients_command (legacy) * device ID string (not very suitable) * + the rest from i2c client, not related at all regards Antti -- http://palosaari.fi/ -- 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 v2 4/5] tc90522: add driver for Toshiba TC90522 quad demodulator
On 09/01/2014 12:54 PM, Akihiro TSUKADA wrote: Hi, Also, I would like to see all new drivers (demod and tuner) implemented as a standard kernel I2C drivers (or any other bus). I have converted already quite many drivers, si2168, si2157, m88ds3103, m88ts2022, it913x, tda18212, ... I wrote the code in the old style using dvb_attach() because (I felt) it is simpler than using i2c_new_device() by introducing new i2c-related data structures, registering to both dvb and i2c, without any new practical features that i2c client provides. Of course it is simpler to do old style as you could copy paste older drivers and so. However, for a long term we must get rid of all DVB specific hacks and use common kernel solutions. The gap between common kernel solutions and DVB proprietary is already too big, without any good reason - just a laziness of developers to find out proper solutions as adding hacks is easier. I mentioned quite many reasons earlier and If you look that driver you will see you use dev_foo() logging, that does not even work properly unless you convert driver to some kernel binding model (I2C on that case) (as I explained earlier). There is also review issues. For more people do own tricks and hacks the harder code is review and also maintain as you don't never know what breaks when you do small change, which due to some trick used causes some other error. Here is one example I fixed recently: https://patchwork.linuxtv.org/patch/25776/ Lets mention that I am not even now fully happy to solution, even it somehow now works. Proper solution is implement clock source and clock client. Then register client to that source. And when client needs a clock (or power) it makes call to enable clock. But if the use of dvb_attach() is (almost) deprecated and i2c client driver is the standard/prefered way, I'll convert my code. regards Antti -- http://palosaari.fi/ -- 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 v2 4/5] tc90522: add driver for Toshiba TC90522 quad demodulator
Em Sat, 06 Sep 2014 05:02:15 +0300 Antti Palosaari cr...@iki.fi escreveu: On 09/01/2014 12:54 PM, Akihiro TSUKADA wrote: Hi, Also, I would like to see all new drivers (demod and tuner) implemented as a standard kernel I2C drivers (or any other bus). I have converted already quite many drivers, si2168, si2157, m88ds3103, m88ts2022, it913x, tda18212, ... I wrote the code in the old style using dvb_attach() because (I felt) it is simpler than using i2c_new_device() by introducing new i2c-related data structures, registering to both dvb and i2c, without any new practical features that i2c client provides. Of course it is simpler to do old style as you could copy paste older drivers and so. However, for a long term we must get rid of all DVB specific hacks and use common kernel solutions. The gap between common kernel solutions and DVB proprietary is already too big, without any good reason - just a laziness of developers to find out proper solutions as adding hacks is easier. I mentioned quite many reasons earlier and If you look that driver you will see you use dev_foo() logging, that does not even work properly unless you convert driver to some kernel binding model (I2C on that case) (as I explained earlier). There is also review issues. For more people do own tricks and hacks the harder code is review and also maintain as you don't never know what breaks when you do small change, which due to some trick used causes some other error. Here is one example I fixed recently: https://patchwork.linuxtv.org/patch/25776/ Yes, using the I2C binding way provides a better decoupling than using the legacy way. The current dvb_attach() macros are hacks that were created by the time where the I2C standard bind didn't work with DVB. We need to move on. Lets mention that I am not even now fully happy to solution, even it somehow now works. Proper solution is implement clock source and clock client. Then register client to that source. And when client needs a clock (or power) it makes call to enable clock. Well, we need to discuss more about that, because you need to convince me first about that ;) We had already some discussions about that related to V4L2 I2C devices. The consensus we've reached is that it makes sense to use the clock framework only for the cases where the bridge driver doesn't know anything about the clock to be used by a given device, e. g. in the case where this data comes from the Device Tree (embedded systems). In the case where the bridge is the ownership of the information that will be used by a given device model (clock, serial/parallel mode, etc), then a series of data information should be passed by a call from the bridge driver to the device at setup time, and doing it in an atomic way is the best way to go. Anyway, such discussions don't belong to this thread. For the PT3 and tc90522 to be merged, the only pending stuff to be done is to use the I2C binding. Akihiro-san, Please change the driver to use the I2C model as pointed by Antti. Thank you! Mauro But if the use of dvb_attach() is (almost) deprecated and i2c client driver is the standard/prefered way, I'll convert my code. regards Antti -- 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
DVB clock source (Re: [PATCH v2 4/5] tc90522: add driver for Toshiba TC90522 quad demodulator)
On 09/06/2014 05:27 AM, Mauro Carvalho Chehab wrote: Em Sat, 06 Sep 2014 05:02:15 +0300 Antti Palosaari cr...@iki.fi escreveu: Lets mention that I am not even now fully happy to solution, even it somehow now works. Proper solution is implement clock source and clock client. Then register client to that source. And when client needs a clock (or power) it makes call to enable clock. Well, we need to discuss more about that, because you need to convince me first about that ;) We had already some discussions about that related to V4L2 I2C devices. The consensus we've reached is that it makes sense to use the clock framework only for the cases where the bridge driver doesn't know anything about the clock to be used by a given device, e. g. in the case where this data comes from the Device Tree (embedded systems). In the case where the bridge is the ownership of the information that will be used by a given device model (clock, serial/parallel mode, etc), then a series of data information should be passed by a call from the bridge driver to the device at setup time, and doing it in an atomic way is the best way to go. For AF9033/IT9133 demod + tuner I resolved it like that: https://patchwork.linuxtv.org/patch/25772/ https://patchwork.linuxtv.org/patch/25774/ It is demod which provides clock for tuner. It is very common situation nowadays, one or more clocks are shared. And clock sharing is routed via chips so that there is clock gates you have enable/disable for power management reasons. Currently we just enable clocks always. Clock output is put on when driver is attached and it is never disabled after that, leaving power management partly broken. Lets take a example, dual tuner case: tuner#0 gets clock from Xtal tuner#1 gets clock from #tuner0 All possible use cases are: 1) #tuner0 off #tuner1 off 2) #tuner0 on #tuner1 off 3) #tuner1 off #tuner1 on 4) #tuner1 on #tuner1 on you will need, as per aforementioned use case: 1) #tuner0 clock out off #tuner1 clock out off 2) #tuner0 clock out off #tuner1 clock out off 3) #tuner0 clock out on #tuner1 clock out off 4) #tuner0 clock out on #tuner1 clock out off Implementing that currently is simply impossible. But if you use clock framework (or what ever its name is) I think it is possible to implement that properly. When tuner#1 driver needs a clock, it calls get clock and that call is routed to #tuner0 which enables clock. And that was not even the most complicated case, as many times clock is routed to demod and USB bridge too. Quite same situation is for power on/off gpios (which should likely implemented as a regulator). Also there is many times reset gpio (for PM chip is powered off by switching power totally off *or* chip is put to reset using GPIO) regards Antti -- http://palosaari.fi/ -- 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: DVB clock source (Re: [PATCH v2 4/5] tc90522: add driver for Toshiba TC90522 quad demodulator)
Em Sat, 06 Sep 2014 06:00:28 +0300 Antti Palosaari cr...@iki.fi escreveu: On 09/06/2014 05:27 AM, Mauro Carvalho Chehab wrote: Em Sat, 06 Sep 2014 05:02:15 +0300 Antti Palosaari cr...@iki.fi escreveu: Lets mention that I am not even now fully happy to solution, even it somehow now works. Proper solution is implement clock source and clock client. Then register client to that source. And when client needs a clock (or power) it makes call to enable clock. Well, we need to discuss more about that, because you need to convince me first about that ;) We had already some discussions about that related to V4L2 I2C devices. The consensus we've reached is that it makes sense to use the clock framework only for the cases where the bridge driver doesn't know anything about the clock to be used by a given device, e. g. in the case where this data comes from the Device Tree (embedded systems). In the case where the bridge is the ownership of the information that will be used by a given device model (clock, serial/parallel mode, etc), then a series of data information should be passed by a call from the bridge driver to the device at setup time, and doing it in an atomic way is the best way to go. For AF9033/IT9133 demod + tuner I resolved it like that: https://patchwork.linuxtv.org/patch/25772/ https://patchwork.linuxtv.org/patch/25774/ It is demod which provides clock for tuner. It is very common situation nowadays, one or more clocks are shared. And clock sharing is routed via chips so that there is clock gates you have enable/disable for power management reasons. Currently we just enable clocks always. Clock output is put on when driver is attached and it is never disabled after that, leaving power management partly broken. Lets take a example, dual tuner case: tuner#0 gets clock from Xtal tuner#1 gets clock from #tuner0 All possible use cases are: 1) #tuner0 off #tuner1 off 2) #tuner0 on #tuner1 off 3) #tuner1 off #tuner1 on 4) #tuner1 on #tuner1 on you will need, as per aforementioned use case: 1) #tuner0 clock out off #tuner1 clock out off 2) #tuner0 clock out off #tuner1 clock out off 3) #tuner0 clock out on #tuner1 clock out off 4) #tuner0 clock out on #tuner1 clock out off Implementing that currently is simply impossible. But if you use clock framework (or what ever its name is) I think it is possible to implement that properly. When tuner#1 driver needs a clock, it calls get clock and that call is routed to #tuner0 which enables clock. And that was not even the most complicated case, as many times clock is routed to demod and USB bridge too. Quite same situation is for power on/off gpios (which should likely implemented as a regulator). Also there is many times reset gpio (for PM chip is powered off by switching power totally off *or* chip is put to reset using GPIO) Ok, in the above scenario, I agree that using the clock framework makes sense, but, on devices where the clock is independent (e. g. each chip has its on XTAL), I'm yet to see a scenario where using the clock framework will simplify the code or bring some extra benefit. 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: DVB clock source (Re: [PATCH v2 4/5] tc90522: add driver for Toshiba TC90522 quad demodulator)
On 09/06/2014 06:11 AM, Mauro Carvalho Chehab wrote: Em Sat, 06 Sep 2014 06:00:28 +0300 Antti Palosaari cr...@iki.fi escreveu: On 09/06/2014 05:27 AM, Mauro Carvalho Chehab wrote: Em Sat, 06 Sep 2014 05:02:15 +0300 Antti Palosaari cr...@iki.fi escreveu: Lets mention that I am not even now fully happy to solution, even it somehow now works. Proper solution is implement clock source and clock client. Then register client to that source. And when client needs a clock (or power) it makes call to enable clock. Well, we need to discuss more about that, because you need to convince me first about that ;) We had already some discussions about that related to V4L2 I2C devices. The consensus we've reached is that it makes sense to use the clock framework only for the cases where the bridge driver doesn't know anything about the clock to be used by a given device, e. g. in the case where this data comes from the Device Tree (embedded systems). In the case where the bridge is the ownership of the information that will be used by a given device model (clock, serial/parallel mode, etc), then a series of data information should be passed by a call from the bridge driver to the device at setup time, and doing it in an atomic way is the best way to go. For AF9033/IT9133 demod + tuner I resolved it like that: https://patchwork.linuxtv.org/patch/25772/ https://patchwork.linuxtv.org/patch/25774/ It is demod which provides clock for tuner. It is very common situation nowadays, one or more clocks are shared. And clock sharing is routed via chips so that there is clock gates you have enable/disable for power management reasons. Currently we just enable clocks always. Clock output is put on when driver is attached and it is never disabled after that, leaving power management partly broken. Lets take a example, dual tuner case: tuner#0 gets clock from Xtal tuner#1 gets clock from #tuner0 All possible use cases are: 1) #tuner0 off #tuner1 off 2) #tuner0 on #tuner1 off 3) #tuner1 off #tuner1 on 4) #tuner1 on #tuner1 on you will need, as per aforementioned use case: 1) #tuner0 clock out off #tuner1 clock out off 2) #tuner0 clock out off #tuner1 clock out off 3) #tuner0 clock out on #tuner1 clock out off 4) #tuner0 clock out on #tuner1 clock out off Implementing that currently is simply impossible. But if you use clock framework (or what ever its name is) I think it is possible to implement that properly. When tuner#1 driver needs a clock, it calls get clock and that call is routed to #tuner0 which enables clock. And that was not even the most complicated case, as many times clock is routed to demod and USB bridge too. Quite same situation is for power on/off gpios (which should likely implemented as a regulator). Also there is many times reset gpio (for PM chip is powered off by switching power totally off *or* chip is put to reset using GPIO) Ok, in the above scenario, I agree that using the clock framework makes sense, but, on devices where the clock is independent (e. g. each chip has its on XTAL), I'm yet to see a scenario where using the clock framework will simplify the code or bring some extra benefit. And I resolved it like that for IT9135: https://patchwork.linuxtv.org/patch/25763/ 1) defined tuner role config parameter (master feeds a clock, slave does not) 2) master is then never put 100% deep sleep Comment on code explains that: /* * Writing '0x00' to master tuner register '0x80ec08' causes slave tuner * communication lost. Due to that, we cannot put master full sleep. */ but it will be much more elegant solution to use clock framework which allows implementing correct power management. regards Antti -- http://palosaari.fi/ -- 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 v2 4/5] tc90522: add driver for Toshiba TC90522 quad demodulator
Hi, Also, I would like to see all new drivers (demod and tuner) implemented as a standard kernel I2C drivers (or any other bus). I have converted already quite many drivers, si2168, si2157, m88ds3103, m88ts2022, it913x, tda18212, ... I wrote the code in the old style using dvb_attach() because (I felt) it is simpler than using i2c_new_device() by introducing new i2c-related data structures, registering to both dvb and i2c, without any new practical features that i2c client provides. But if the use of dvb_attach() is (almost) deprecated and i2c client driver is the standard/prefered way, I'll convert my code. regards, akihiro -- 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 v2 4/5] tc90522: add driver for Toshiba TC90522 quad demodulator
On 27.08.2014 17:29, tsk...@gmail.com wrote:
From: Akihiro Tsukada tsk...@gmail.com
Hi Akihiro,
This patch adds driver for tc90522 demodulator chips.
The chip contains 4 demod modules that run in parallel and are independently
controllable via separate I2C addresses.
Two of the modules are for ISDB-T and the rest for ISDB-S.
It is used in earthsoft pt3 cards.
Note that this driver does not init the chip,
because the initilization sequence / register setting is not disclosed.
Thus, the driver assumes that the chips are initilized externally
by its parent board driver before fe-ops-init() are called.
Earthsoft PT3 PCIe card, for example, contains the init sequence
in its private memory and provides a command to trigger the sequence.
Signed-off-by: Akihiro Tsukada tsk...@gmail.com
+
+struct dvb_frontend *
+tc90522_attach(const struct tc90522_config *cfg, struct i2c_adapter *i2c)
+{
+ struct tc90522_state *state;
+ const struct dvb_frontend_ops *ops;
+ struct i2c_adapter *adap;
+ int ret;
+
+ state = kzalloc(sizeof(*state), GFP_KERNEL);
+ if (!state)
+ return NULL;
+
+ memcpy(state-cfg, cfg, sizeof(*cfg));
+ state-i2c = i2c;
+ ops = TC90522_IS_ISDBS(cfg-addr) ? tc90522_ops_sat : tc90522_ops_ter;
+ memcpy(state-dvb_fe.ops, ops, sizeof(*ops));
+ state-dvb_fe.demodulator_priv = state;
+
+ adap = state-tuner_i2c;
+ adap-owner = i2c-owner;
+ adap-algo = tc90522_tuner_i2c_algo;
+ adap-dev.parent = i2c-dev.parent;
+ strlcpy(adap-name, tc90522 tuner, sizeof(adap-name));
+ i2c_set_adapdata(adap, state);
+ ret = i2c_add_adapter(adap);
+ if (ret 0) {
+ kfree(state);
+ return NULL;
+ }
+ dev_info(i2c-dev, Toshiba TC90522 attached.\n);
+ return state-dvb_fe;
+}
+EXPORT_SYMBOL(tc90522_attach);
+
+
+struct i2c_adapter *tc90522_get_tuner_i2c(struct dvb_frontend *fe)
+{
+ struct tc90522_state *state;
+
+ state = fe-demodulator_priv;
+ return state-tuner_i2c;
+}
+EXPORT_SYMBOL(tc90522_get_tuner_i2c);
+
+
+MODULE_DESCRIPTION(Toshiba TC90522 frontend);
+MODULE_AUTHOR(Akihiro TSUKADA);
+MODULE_LICENSE(GPL);
diff --git a/drivers/media/dvb-frontends/tc90522.h
b/drivers/media/dvb-frontends/tc90522.h
new file mode 100644
index 000..d55a6be
--- /dev/null
+++ b/drivers/media/dvb-frontends/tc90522.h
@@ -0,0 +1,63 @@
+/*
+ * Toshiba TC90522 Demodulator
+ *
+ * Copyright (C) 2014 Akihiro Tsukada tsk...@gmail.com
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation version 2.
+ *
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ */
+
+/*
+ * The demod has 4 input (2xISDB-T and 2xISDB-S),
+ * and provides independent sub modules for each input.
+ * As the sub modules work in parallel and have the separate i2c addr's,
+ * this driver treats each sub module as one demod device.
+ */
+
+#ifndef TC90522_H
+#define TC90522_H
+
+#include linux/i2c.h
+#include linux/kconfig.h
+#include dvb_frontend.h
+
+#define TC90522_IS_ISDBS(addr) (addr 1)
+
+#define TC90522T_CMD_SET_LNA 1
+#define TC90522S_CMD_SET_LNB 1
+
+struct tc90522_config {
+ u8 addr;
+};
+
+#if IS_ENABLED(CONFIG_DVB_TC90522)
+
+extern struct dvb_frontend *tc90522_attach(const struct tc90522_config *cfg,
+ struct i2c_adapter *i2c);
+
+extern struct i2c_adapter *tc90522_get_tuner_i2c(struct dvb_frontend *fe);
+
it sounds wrong to export a second function besides tc90522_attach.
This way there is a hard dependency of the bridge driver to the demod
driver.
In this case it is the only possible demod, but in general it violates
the design of demod drivers and their connection to bridge drivers.
si2168_probe at least has a solution for this:
Write the pointer to the new i2c adapter into location stored in struct
i2c_adapter ** in the config structure.
Regards
Matthias
--
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 v2 4/5] tc90522: add driver for Toshiba TC90522 quad demodulator
Hi Matthias, thanks for the comment. it sounds wrong to export a second function besides tc90522_attach. This way there is a hard dependency of the bridge driver to the demod driver. In this case it is the only possible demod, but in general it violates the design of demod drivers and their connection to bridge drivers. I agree. I missed that point. si2168_probe at least has a solution for this: Write the pointer to the new i2c adapter into location stored in struct i2c_adapter ** in the config structure. I'll look into the si2168 code and update tc90522 in v3. regards, akihiro -- 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 v2 4/5] tc90522: add driver for Toshiba TC90522 quad demodulator
On 08/31/2014 04:32 PM, Akihiro TSUKADA wrote: Hi Matthias, thanks for the comment. it sounds wrong to export a second function besides tc90522_attach. This way there is a hard dependency of the bridge driver to the demod driver. In this case it is the only possible demod, but in general it violates the design of demod drivers and their connection to bridge drivers. I agree. I missed that point. si2168_probe at least has a solution for this: Write the pointer to the new i2c adapter into location stored in struct i2c_adapter ** in the config structure. I'll look into the si2168 code and update tc90522 in v3. Also, I would like to see all new drivers (demod and tuner) implemented as a standard kernel I2C drivers (or any other bus). I have converted already quite many drivers, si2168, si2157, m88ds3103, m88ts2022, it913x, tda18212, ... When drivers are using proper kernel driver models, it allows using kernel services. For example dev_ / pr_ logging (it does not work properly without), RegMap API, I2C client, I2C multiplex, and so... Here is few recent examples: https://patchwork.linuxtv.org/patch/25495/ https://patchwork.linuxtv.org/patch/25152/ https://patchwork.linuxtv.org/patch/25146/ regards Antti -- http://palosaari.fi/ -- 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 v2 4/5] tc90522: add driver for Toshiba TC90522 quad demodulator
From: Akihiro Tsukada tsk...@gmail.com
This patch adds driver for tc90522 demodulator chips.
The chip contains 4 demod modules that run in parallel and are independently
controllable via separate I2C addresses.
Two of the modules are for ISDB-T and the rest for ISDB-S.
It is used in earthsoft pt3 cards.
Note that this driver does not init the chip,
because the initilization sequence / register setting is not disclosed.
Thus, the driver assumes that the chips are initilized externally
by its parent board driver before fe-ops-init() are called.
Earthsoft PT3 PCIe card, for example, contains the init sequence
in its private memory and provides a command to trigger the sequence.
Signed-off-by: Akihiro Tsukada tsk...@gmail.com
---
Changes in v2:
- renamed badly named variables
- moved static const tables out of function scope
- calculate symbol rate per output TS
- improve in _init() to support suspend/resume
drivers/media/dvb-frontends/Kconfig | 8 +
drivers/media/dvb-frontends/Makefile | 1 +
drivers/media/dvb-frontends/tc90522.c | 857 ++
drivers/media/dvb-frontends/tc90522.h | 63 +++
4 files changed, 929 insertions(+)
diff --git a/drivers/media/dvb-frontends/Kconfig
b/drivers/media/dvb-frontends/Kconfig
index aa5ae22..123adb2 100644
--- a/drivers/media/dvb-frontends/Kconfig
+++ b/drivers/media/dvb-frontends/Kconfig
@@ -648,6 +648,14 @@ config DVB_MB86A20S
A driver for Fujitsu mb86a20s ISDB-T/ISDB-Tsb demodulator.
Say Y when you want to support this frontend.
+config DVB_TC90522
+ tristate Toshiba TC90522
+ depends on DVB_CORE I2C
+ default m if !MEDIA_SUBDRV_AUTOSELECT
+ help
+ A Toshiba TC90522 2xISDB-T + 2xISDB-S demodulator.
+ Say Y when you want to support this frontend.
+
comment Digital terrestrial only tuners/PLL
depends on DVB_CORE
diff --git a/drivers/media/dvb-frontends/Makefile
b/drivers/media/dvb-frontends/Makefile
index fc4e689..00cc299 100644
--- a/drivers/media/dvb-frontends/Makefile
+++ b/drivers/media/dvb-frontends/Makefile
@@ -114,3 +114,4 @@ obj-$(CONFIG_DVB_RTL2832_SDR) += rtl2832_sdr.o
obj-$(CONFIG_DVB_M88RS2000) += m88rs2000.o
obj-$(CONFIG_DVB_AF9033) += af9033.o
obj-$(CONFIG_DVB_AS102_FE) += as102_fe.o
+obj-$(CONFIG_DVB_TC90522) += tc90522.o
diff --git a/drivers/media/dvb-frontends/tc90522.c
b/drivers/media/dvb-frontends/tc90522.c
new file mode 100644
index 000..1f0f6f7
--- /dev/null
+++ b/drivers/media/dvb-frontends/tc90522.c
@@ -0,0 +1,857 @@
+/*
+ * Toshiba TC90522 Demodulator
+ *
+ * Copyright (C) 2014 Akihiro Tsukada tsk...@gmail.com
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation version 2.
+ *
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ */
+
+#include linux/kernel.h
+#include linux/dvb/frontend.h
+#include dvb_math.h
+#include tc90522.h
+
+#define TC90522_I2C_THRU_REG 0xfe
+
+#define TC90522_MODULE_IDX(addr) (((u8)(addr) 0x02U) 1)
+
+enum tc90522_tuning_status {
+ STATUS_IDLE,
+ STATUS_SET_FREQ,
+ STATUS_CHECK_TUNER,
+ STATUS_CHECK_DEMOD,
+ STATUS_TRACK,
+};
+
+struct tc90522_state {
+ struct dvb_frontend dvb_fe;
+
+ struct tc90522_config cfg;
+ struct i2c_adapter *i2c;
+ struct i2c_adapter tuner_i2c;
+ enum tc90522_tuning_status tuning_status;
+ int retry_count;
+
+ bool lna;
+};
+
+struct reg_val {
+ u8 reg;
+ u8 val;
+};
+
+
+static int
+reg_write(struct tc90522_state *state, const struct reg_val *regs, int num)
+{
+ int i, ret;
+ struct i2c_msg msg;
+
+ ret = 0;
+ msg.addr = state-cfg.addr;
+ msg.flags = 0;
+ msg.len = 2;
+ for (i = 0; i num; i++) {
+ msg.buf = (u8 *)regs[i];
+ ret = i2c_transfer(state-i2c, msg, 1);
+ if (ret 0)
+ break;
+ }
+ return ret;
+}
+
+static int reg_read(struct tc90522_state *state, u8 reg, u8 *val, u8 len)
+{
+ struct i2c_msg msgs[2] = {
+ {
+ .addr = state-cfg.addr,
+ .flags = 0,
+ .buf = reg,
+ .len = 1,
+ },
+ {
+ .addr = state-cfg.addr,
+ .flags = I2C_M_RD,
+ .buf = val,
+ .len = len,
+ },
+ };
+
+ return i2c_transfer(state-i2c, msgs, ARRAY_SIZE(msgs));
+}
+
+static int enable_lna(struct dvb_frontend *fe, bool on)
+{
+ struct tc90522_state *state;
+
+ state = fe-demodulator_priv;
+ /* delegate to the parent
