On Tue, Mar 13, 2012 at 03:56:20PM +0100, Ola LILJA2 wrote:
Answers inline.
PS. New patch-set uploaded.
Ugh, you *really* should've sent this at the time (or in general before sending the new patches) since now a bunch of review comments are going to get repetitive...
Nothing in this header looks like it should be exported, I've not actually looked at any of the code to see what this stuff is doing but it all looks like it should be private to the relevant drivers.
The machine-driver is split up in a main machine-driver and several sub-machine-drivers (one for each platform<->codec), so this inteface is just so that the main machine-file can call reach the functions implemented in the sub-machine-driver-file. It is not meant for the world outside our driver.
This sounds suspicious, what is this "generic" machine driver?
+ifdef CONFIG_SND_SOC_UX500_DEBUG +CFLAGS_ab8500_audio.o := -DDEBUG +endif
No other driver has this. Why does your driver have it?
Can't answer to why other people don't have it =) I found it convenient to be able to turn on the debug-prints in our driver by just using a flag in menuncofig rather than modifying Makefiles every time. It is easier to tell our customer to just activate this flag. Do you want me to remove it?
Either make this a generic feature or remove it, we shouldn't have stuff like this available randomly.
+static int st_fir_value_control_get(struct snd_kcontrol *kcontrol,
struct snd_ctl_elem_value*ucontrol) {
- return 0;
+}
This looks obviously buggy.
These are FIR-coeffecients and can (by HW-design) not be read. So there is nothing to do here.
This still looks obviously buggy. If readback is not supported there shouldn't be a readback function, and returning success is clearly not the right thinng to do.
- data = ab8500_codec_read_reg(codec, AB8500_MISC,
AB8500_GPIO_DIR4_REG);
- data |= GPIO27_DIR_OUTPUT | GPIO29_DIR_OUTPUT |
GPIO31_DIR_OUTPUT;
- ab8500_codec_write_reg(codec, AB8500_MISC,
AB8500_GPIO_DIR4_REG,
+data);
This loooks like platform data...
Well, this is actually GPIO on the codec-chip and not the base-chip.
That doesn't seem germane to it being provied as platform data.
+void ab8500_audio_power_control(bool power_on) {
- if (ab8500_codec == NULL) {
pr_err("%s: ERROR: AB8500 ASoC-driver not yetprobed!\n", __func__);
return;- }
Use the framework features for power management, we've got enough ways for your driver to get told when to power up and down none of which require a global pointer to a device.
This is called as an effect of a DAPM-chain so it is using the ASoC-framework. The reason for having this like it is, is due to the fact that we need to call this funtion not only from the DAPM-chain, but also from another kernel-driver (our vibra-driver). I would love to see this extra way into the audio-driver but as the design of our platform require this, we are stuck with this solution.
No, you're not. Use a MFD like the existing drivers doing this. Having a global data thing like this is really not suitable for mainline.