Re: [PATCH 0/5] [RFC] ALSA ASoC Support for TLVaic23b audio codec

Re: [PATCH 0/5] [RFC] ALSA ASoC Support for TLVaic23b audio codec

[Troy Kisky]

Arun KS wrote:
> Hi all,
> 
> The following patches adds ASOC driver for TLVaic23b audio codec.
> 
> Tested playback and capture on omap 5912osk board with some hacks in the
> omap platform and machine driver.
> 
> Thanks,
> Arun
> --

Great. I just finished the same thing for davinci but using spi.
I was waiting until the adc issue I mentioned yesterday was resolved before posting it.
I'm glad I won't have to now. But my board also has I2c, just not with this
codec. Can the I2c code be moved into the board directory? I have my spi code
in the board directory and just pass the write function pointer. i.e.

	codec->hw_write = setup_data->hw_write;


Can you email me some data captured by from your mic input?
I'm interested in seeing if you have the same trouble in dsp_a
mode.


Thanks,
Troy

Re: [PATCH 0/5] [RFC] ALSA ASoC Support for TLVaic23b audio codec

[Mark Brown]

On Mon, Sep 29, 2008 at 09:47:10AM -0700, Troy Kisky wrote:

> Great. I just finished the same thing for davinci but using spi.
> I was waiting until the adc issue I mentioned yesterday was resolved before posting it.
> I'm glad I won't have to now. But my board also has I2c, just not with this

It'd be great if you could work with Arun to ensure that all the
features which you are using are supported in the driver - it looks like
at least SPI support will need to be added.

> codec. Can the I2c code be moved into the board directory? I have my spi code
> in the board directory and just pass the write function pointer. i.e.

No, the I2C and SPI support should both be part of the driver.  The
driver should just be modified to allow both I2C and SPI support to be
compiled in and let the board driver register the appropriate one at run
time.

Re: [PATCH 0/5] [RFC] ALSA ASoC Support for TLVaic23b audio codec

[Troy Kisky]

Mark Brown wrote:
> On Mon, Sep 29, 2008 at 09:47:10AM -0700, Troy Kisky wrote:
> 
> 
> No, the I2C and SPI support should both be part of the driver.  The
> driver should just be modified to allow both I2C and SPI support to be
> compiled in and let the board driver register the appropriate one at run
> time.
> 

Seems to me a Kconfig option would be more efficient. Otherwise, one of the
two options is just going to waste space.

Troy

Re: [PATCH 0/5] [RFC] ALSA ASoC Support for TLVaic23b audio codec

[Mark Brown]

On Mon, Sep 29, 2008 at 10:05:02AM -0700, Troy Kisky wrote:
> Mark Brown wrote:

> > No, the I2C and SPI support should both be part of the driver.  The

> Seems to me a Kconfig option would be more efficient. Otherwise, one of the
> two options is just going to waste space.

We need to support building for both I2C and SPI simultaneously in order
to allow kernels supporting multiple boards to be built.  If the
selection is done at build time then the number of configurations that
can be built in a single image is restricted needlessly.

I'd ack patches which made it possible to configure one or the other out
but given the tiny amount of glue required I'd personally not bother
writing that.

-- 
"You grabbed my hand and we fell into it, like a daydream - or a fever."

Re: [PATCH 0/5] [RFC] ALSA ASoC Support for TLVaic23b audio codec

[Troy Kisky]

Mark Brown wrote:
> On Mon, Sep 29, 2008 at 09:47:10AM -0700, Troy Kisky wrote:
> 
>> Great. I just finished the same thing for davinci but using spi.
>> I was waiting until the adc issue I mentioned yesterday was resolved before posting it.
>> I'm glad I won't have to now. But my board also has I2c, just not with this
> 
> It'd be great if you could work with Arun to ensure that all the
> features which you are using are supported in the driver - it looks like
> at least SPI support will need to be added.
> 
>> codec. Can the I2c code be moved into the board directory? I have my spi code
>> in the board directory and just pass the write function pointer. i.e.
> 
> No, the I2C and SPI support should both be part of the driver.  The
> driver should just be modified to allow both I2C and SPI support to be
> compiled in and let the board driver register the appropriate one at run
> time.
> 
Can you please explain to me why they should both be part of the codec driver?


Thanks
Troy

Re: [PATCH 0/5] [RFC] ALSA ASoC Support for TLVaic23b audio codec

[Mark Brown]

On Mon, Sep 29, 2008 at 10:27:06AM -0700, Troy Kisky wrote:
> Mark Brown wrote:

> >> codec. Can the I2c code be moved into the board directory? I have my spi code
> >> in the board directory and just pass the write function pointer. i.e.

> > No, the I2C and SPI support should both be part of the driver.  The
> > driver should just be modified to allow both I2C and SPI support to be
> > compiled in and let the board driver register the appropriate one at run
> > time.

> Can you please explain to me why they should both be part of the codec driver?

Neither is board specific - there's no sense in having each board that
needs SPI support replicate the code to register a SPI device and do the
marshalling of data for SPI writes.  What motivation do you see for not
doing that?

Re: [PATCH 0/5] [RFC] ALSA ASoC Support for TLVaic23b audio codec

[Troy Kisky]

Mark Brown wrote:
> On Mon, Sep 29, 2008 at 10:27:06AM -0700, Troy Kisky wrote:
>> Mark Brown wrote:
> 
>>>> codec. Can the I2c code be moved into the board directory? I have my spi code
>>>> in the board directory and just pass the write function pointer. i.e.
> 
>>> No, the I2C and SPI support should both be part of the driver.  The
>>> driver should just be modified to allow both I2C and SPI support to be
>>> compiled in and let the board driver register the appropriate one at run
>>> time.
> 
>> Can you please explain to me why they should both be part of the codec driver?
> 
> Neither is board specific - there's no sense in having each board that
> needs SPI support replicate the code to register a SPI device and do the
> marshalling of data for SPI writes.  What motivation do you see for not
> doing that?
> 

It just doesn't seem to be logically a part of the codec code. And I didn't register
an spi device. I just linked the simple spi routines with my board code (separate file).

Plus, it seems a lot of code duplication if each codec registers the spi device
and I2C device. Are there more boards, or more codecs???

Thanks
Troy

Re: [PATCH 0/5] [RFC] ALSA ASoC Support for TLVaic23b audio codec

[Mark Brown]

On Mon, Sep 29, 2008 at 10:58:43AM -0700, Troy Kisky wrote:
> Mark Brown wrote:

> > Neither is board specific - there's no sense in having each board that
> > needs SPI support replicate the code to register a SPI device and do the
> > marshalling of data for SPI writes.  What motivation do you see for not
> > doing that?

> It just doesn't seem to be logically a part of the codec code. And I didn't register

The data marshalling is a function of the codec hardware - whatever it
has been hooked up to the register address and data values written are
going to need to be in a given format when they hit the codec.  The SPI
and I2C APIs abstract away the details of the actual controller from the
codec driver.

> an spi device. I just linked the simple spi routines with my board code (separate file).

I'm not sure which simple SPI API you're referring to here?  In any
case, ASoC is shortly going to pretty much require a struct device for
the codec so it will be required to have a device of some kind
registered.

> Plus, it seems a lot of code duplication if each codec registers the spi device
> and I2C device. Are there more boards, or more codecs???

In the device model the board registers the *device*, the codec (or
whatever) driver registers the *driver* - the two are separated.  The
driver core then instantiates the drivers based on what the board
specifies.  There's not really any overlap between the two areas that I
can see.

Otherwise could you explain in more detail where you see the code
duplication coming from?

Re: [PATCH 0/5] [RFC] ALSA ASoC Support for TLVaic23b audio codec

[Troy Kisky]

Mark Brown wrote:
> On Mon, Sep 29, 2008 at 10:58:43AM -0700, Troy Kisky wrote:
> In the device model the board registers the *device*, the codec (or
> whatever) driver registers the *driver* - the two are separated.  The
> driver core then instantiates the drivers based on what the board
> specifies.  There's not really any overlap between the two areas that I
> can see.
> 
> Otherwise could you explain in more detail where you see the code
> duplication coming from?

My point, though admittedly a minor one, was that the I2C device
or spi device could be initialized before the codec is probed.

That would be moving code from codec files into board files.
Better would be moving the code into a common soc directory file.

+#if defined(CONFIG_I2C) || defined(CONFIG_I2C_MODULE)
+/*
+ * If the i2c layer weren't so broken, we could pass this kind of data
+ * around
+ */
+static int aic23_codec_probe(struct i2c_client *i2c,
+			     const struct i2c_device_id *i2c_id)
+{
+	struct snd_soc_device *socdev = aic23_socdev;
+	struct snd_soc_codec *codec = socdev->codec;
+	int ret;
+
+	if (!i2c_check_functionality(i2c->adapter, I2C_FUNC_SMBUS_BYTE_DATA))
+		return -EINVAL;
+
+
+	i2c_set_clientdata(i2c, codec);
+	codec->control_data = i2c;
+
....
+	ret = aic23_init(socdev);
+	if (ret < 0) {
+		printk(KERN_ERR "aic23: failed to initialise AIC23\n");
+		goto err;
+	}
... The above would move to codec probe

+	return ret;
+
+err:
+	kfree(codec);
+	kfree(i2c);
+	return ret;
+}
+static int __exit aic23_i2c_remove(struct i2c_client *i2c)
+{
+
+	put_device(&i2c->dev);
+	return 0;
+}
+
+static const struct i2c_device_id tlvaic23b_id[] = {
+	{"tlvaic23b", 0},
+	{}
+};
+
+MODULE_DEVICE_TABLE(i2c, tlvaic23b_id);
+
+static struct i2c_driver aic23b_i2c_driver = {
+	.driver = {
+		   .name = "tlvaic23b",
+		   },
+	.probe = aic23_codec_probe,
+	.remove = __exit_p(aic23_i2c_remove),
+	.id_table = tlvaic23b_id,
+};
+
+#endif

If this code could be generalized to be useable by most
codecs, I think the code would look better. This is the
"almost" duplication to which I'm referring.


Troy

Re: [PATCH 0/5] [RFC] ALSA ASoC Support for TLVaic23b audio codec

[Mark Brown]

On Mon, Sep 29, 2008 at 12:07:53PM -0700, Troy Kisky wrote:
> Mark Brown wrote:

> > Otherwise could you explain in more detail where you see the code
> > duplication coming from?

> My point, though admittedly a minor one, was that the I2C device
> or spi device could be initialized before the codec is probed.

> That would be moving code from codec files into board files.
> Better would be moving the code into a common soc directory file.

Ah, I think I see what you're talking about for at least this part of
your e-mails, though I'm not 100% sure.

Just to clarify, I *think* that what you're actually talking about here
is the fact that currently ASoC registers the entire ASoC subsystem off
the codec device registration with whatever control subsystem it uses
and therefore the final call to register the device is done within codec
driver code (though triggered by the machine driver)?

It would be much easier if you could be a *little* more verbose and/or
precise in your e-mails.  You started off by talking about wanting to
put the "I2C code" and "SPI code" into the board drivers which rather
suggests removing all bus access code.

> +	ret = aic23_init(socdev);
> +	if (ret < 0) {
> +		printk(KERN_ERR "aic23: failed to initialise AIC23\n");
> +		goto err;
> +	}
> ... The above would move to codec probe

The code you're quoting there is in the codec probe function?  init() is
split out so it can be shared between multiple control methods.

That said, there is an issue with this at the minute due to the fact
that the core does not yet use the device model properly and
(essentially) hangs all the drivers off the codec device which leads to
having both ai23_probe() which registers the I2C driver and
aic23_codec_probe().  This is half way to being rectified (the code
exists but is in the process of being merged) so that what happens is
that all the components of the system register with the ASoC core after
being probed normally from registrations done in the board init code or
similar.  This will remove all the socdev interaction from the codec
driver which is, I think, what you're getting at?

> +static struct i2c_driver aic23b_i2c_driver = {
> +       .driver = {
> +                  .name = "tlvaic23b",
> +                  },
> +       .probe = aic23_codec_probe,
> +       .remove = __exit_p(aic23_i2c_remove),
> +       .id_table = tlvaic23b_id,
> +};
> +
> +#endif

> If this code could be generalized to be useable by most
> codecs, I think the code would look better. This is the
> "almost" duplication to which I'm referring.

There's always a certain amount of bolierplate required to tell the
device core about drivers which isn't going away.  It's really very
small and the benefits in terms of things like refcounting modules and
autoloading them are worthwhile.

Re: [PATCH 0/5] [RFC] ALSA ASoC Support for TLVaic23b audio codec

[Troy Kisky]

> 
> That said, there is an issue with this at the minute due to the fact
> that the core does not yet use the device model properly and
> (essentially) hangs all the drivers off the codec device which leads to
> having both ai23_probe() which registers the I2C driver and
> aic23_codec_probe().  This is half way to being rectified (the code
> exists but is in the process of being merged) so that what happens is
> that all the components of the system register with the ASoC core after
> being probed normally from registrations done in the board init code or
> similar.  This will remove all the socdev interaction from the codec
> driver which is, I think, what you're getting at?

Thanks for the explanation and status update. I have no problem with doing
both in the codec until this code is merged.


Thanks
Troy

[PATCH 0/5] [RFC] ALSA ASoC Support for TLVaic23b audio codec

[Arun KS]

Hi all,

The following patches adds ASOC driver for TLVaic23b audio codec.

Tested playback and capture on omap 5912osk board with some hacks in the
omap platform and machine driver.

Thanks,
Arun