Re: [PATCH v2 4/5] tc90522: add driver for Toshiba TC90522 quad demodulator

[Antti Palosaari <crope@iki.fi>]

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/