Johannes Stezenbach <js@xxxxxxxxxxx> writes:
> The other difference is your use of dvb-pll.
Yes.
> I think it was a conscious decision to move PLL handling out of the
> frontend drivers (thus they are now just "demodulator drivers") and
> into the adapter drivers. I would have expected that you use
> dvb-pll from the cx88 driver, and not add PLL handling back to
> cx22702.
First time I hear that argument ...
Main point of moving the pll handling out of the frontend drivers was
to have some sane way to configure the drivers, i.e. not to have that
ugly guesswork based on strcmp() within the frontend drivers, but give
direct control of this to the dvb adapter drivers.
Handling the pll stuff within the frontend via dvb-pll lib IMHO is
conform to that design idea, the information which pll and which
address is explicitly passed to the frontend/demux driver by the
adapter using the config struct.
And we have the option to have a set_pll() callback for the cases
which can't be handled by the dvb-pll lib, it's just not needed at the
moment for the cx22702.c because such hardware doesn't exist.
I plan to do the a simliar transformation with mt352.c, i.e. do
something along these lines:
if (state->config->pll_desc) {
dvb_pll_configure(...)
} else if (state->config->set_pll) {
state->config->set_pll(...);
} else {
printk("Huh?\n");
}
I think stuffing the pll lib info into the demod config struct is
better than having a trivial set_pll callback like this one:
static int mt352_pll_set(struct dvb_frontend* fe,
struct dvb_frontend_parameters* params,
u8* pllbuf)
{
struct cx8802_dev *dev= fe->dvb->priv;
pllbuf[0] = dev->core->pll_addr << 1;
dvb_pll_configure(dev->core->pll_desc, pllbuf+1,
params->frequency,
params->u.ofdm.bandwidth);
return 0;
}
> So in essence the usage of the cx22702 driver in the adapter drivers
> is different from the usage of the other demodulator drivers.
Due to dvb-pll usage, yes. That switch was _far_ after the frontend
refactoring, I did'nt got until now that _this_ is the issue you have
with the code.
> Anyway, Andrew invested lots of time to rework the drivers according
> to what he thought the concensus was, and now you come and try to
> invalidate (part of) his work.
I don't IMHO, see above. But up to now the main argument was "the
patch is too big", and the other ones like the probing one turned out
to be false. As long as you don't tell me _why_ you dislike the stuff
it's hard do discuss that ...
> I don't get why you can't just send a small patch to get the cx22702
> driver to work, and then in a second step start a discussion to
> convert *all* frontend drivers to use i2c_client, outlining the
> improvements that would make for all. (The first step shouldn't be
> too difficult given that you claim that the use of i2c_client is
> just an internal implementation thing.)
And as it is a internal thing I don't see a strong need to instantly
convert all frontend drivers. Can be done one by one if they are
touched anyway for some reason, prefearable by someone with test
hardware to minimize the risk that they break.
Gerd
--
#define printk(args...) fprintf(stderr, ## args)
