Seeking help for getting rid of i2c_clients_command()

Seeking help for getting rid of i2c_clients_command()

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 781 bytes --]

Hi Hans,

I know it is 13 years since you wrote that code:

ba8fc39954bf ("V4L/DVB (4270): Add tda9887-specific tuner configuration")

There, you added i2c_clients_command() which looks today like this:

	i2c_clients_command(priv->i2c_props.adap, TUNER_SET_CONFIG,
			    &tda9887_cfg);

Now, I would like to get rid of the i2c_clients_command() API, and this
is the one remaining user. While trying to convert it into a local
function, I started wondering why i2c_clients_command() is used at all.
Why do we need to try all devices on the bus? A few lines later we have:

	tuner_i2c_xfer_send(&priv->i2c_props, buffer, 4);

so i2c_props.addr seems to have a valid address? Or are there multiple
tuners on that bus? Can you kindly shed some light on this?

Thank you!

   Wolfram


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: Seeking help for getting rid of i2c_clients_command()

[Hans Verkuil]

On 11/10/19 4:05 PM, Wolfram Sang wrote:
> Hi Hans,
> 
> I know it is 13 years since you wrote that code:
> 
> ba8fc39954bf ("V4L/DVB (4270): Add tda9887-specific tuner configuration")
> 
> There, you added i2c_clients_command() which looks today like this:
> 
> 	i2c_clients_command(priv->i2c_props.adap, TUNER_SET_CONFIG,
> 			    &tda9887_cfg);
> 
> Now, I would like to get rid of the i2c_clients_command() API, and this
> is the one remaining user. While trying to convert it into a local
> function, I started wondering why i2c_clients_command() is used at all.
> Why do we need to try all devices on the bus? A few lines later we have:
> 
> 	tuner_i2c_xfer_send(&priv->i2c_props, buffer, 4);
> 
> so i2c_props.addr seems to have a valid address? Or are there multiple
> tuners on that bus? Can you kindly shed some light on this?

Oh boy... The tuner code is awful for many reasons.

One reason is that for a given video capture card there can be multiple
tuner devices: Analog TV, Digital TV, Analog demodulator and/or radio.

The TUNER_SET_CONFIG command is (AFAICT) only used to configure the
tda9887 (demodulator), and the problem is that when this code was written
the tuner instance that is calling TUNER_SET_CONFIG does not know on which
i2c address the demod is.

TUNER_SET_CONFIG is called whenever the TV or radio frequency is
changed, and when that happens the demod needs to update its configuration
as well. So the tuner_i2c_xfer_send(&priv->i2c_props, buffer, 4); is for the
tuner itself (whose i2c address is known), but i2c_clients_command is
basically broadcasting to anyone on the bus, and only the tda9987 will
actually act on the TUNER_SET_CONFIG command.

However, it looks like this code is only used by hybrid tuners (i.e.
supporting both digital and analog TV), and that the available tuners
are now added to a global list via hybrid_tuner_request_state().

This should make it possible for tuner-simple.c to actually lookup the
i2c address of the demod from that global list and use a direct call.

I've CC-ed a bunch of people who know more about DVB than I do, so
I hope they can double-check that I am on the right track.

Wolfram, do you just want to get rid of i2c_clients_command or the i2c
command() callback as well?

Regardless, it would be nice if we can get rid of the command() callback
in the media subsystem. If I read the code right, then you can get the
tuner struct of an i2c_client using 'struct tuner *t = to_tuner(i2c_get_clientdata(c));'
and from there you can get the analog_demod_ops that you need to
configure the tda9887.

So I think this would work:

1) walk hybrid_tuner_instance_list to find a tuner with name "tda9887" for
   the given i2c_adapter.
2) get the i2c_client from the adapter with address state->i2c_props.addr.
3) struct tuner *t = to_tuner(i2c_get_clientdata(c));
4) if (t->fe.ops.analog_ops.set_config)
	t->fe.ops.analog_ops.set_config(&t->fe, &config);

Regards,

	Hans

Re: Seeking help for getting rid of i2c_clients_command()

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 1083 bytes --]

Hi Hans!

> TUNER_SET_CONFIG is called whenever the TV or radio frequency is
> changed, and when that happens the demod needs to update its configuration
> as well. So the tuner_i2c_xfer_send(&priv->i2c_props, buffer, 4); is for the
> tuner itself (whose i2c address is known), but i2c_clients_command is
> basically broadcasting to anyone on the bus, and only the tda9987 will
> actually act on the TUNER_SET_CONFIG command.

I see. Thanks for the heads up.

> This should make it possible for tuner-simple.c to actually lookup the
> i2c address of the demod from that global list and use a direct call.

Sounds awesome!

> I've CC-ed a bunch of people who know more about DVB than I do, so
> I hope they can double-check that I am on the right track.

Their help would be much appreciated.

> Wolfram, do you just want to get rid of i2c_clients_command or the i2c
> command() callback as well?

I'd like to get rid of the callback as well. tuner is again the only
in-tree user. And this whole mechanism looks so 2.4ish... I think it can
go now.

Thanks for your help,

   Wolfram


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]