Re: [PATCH 3/3] ASoC: Ux500: Add driver for Ux500/AB8500 platform/codec

[Mark Brown <broonie@opensource.wolfsonmicro.com>]

[-- Attachment #1.1: Type: text/plain, Size: 3342 bytes --]

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 yet
> > probed!\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.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]