Re: [PATCH v1 2/2] ASoC: codecs: add TAS2557 mono smart amplifier driver

[Gianluca Boiano]

> +	TAS2557_GPIO6_PIN_REG,            0x01,	/* GPIO6 = ASI2 WCLK input */
> +	TAS2557_GPIO8_PIN_REG,            0x02,	/* GPIO8 = ASI2 DIN */
> +	TAS2557_GPIO5_PIN_REG,            0x01,	/* GPIO5 = ASI2 BCLK input */
>
> This looks suspicously board specific, it should probably come from
> device tree.

It's board specific, and it's not even a TI reference sequence -
checked their Android driver, it never touches ASI2/GPIO5-8 at all,
only ASI1/GPIO1-2. All four boards on this branch happen to share the
same wiring, which is how it ended up hardcoded. Will add DT
properties for the ASI2 pin assignment in a follow-up, defaulting to
today's values.

> +	TAS2557_ASI2_DAC_FORMAT_REG,      0x18,	/* ASI2: 32-bit I2S */
>
> This should be set_dai_fmt().

Fixed in v2: the format is cached in set_dai_fmt() and reapplied in
tas2557_enable() after the reset, so the startup_data write no longer
wins.

> +	TAS2557_ASI2_BDIV_CLK_SEL_REG,   0x01,
> +	TAS2557_ASI2_BDIV_CLK_RATIO_REG, 0x01,
> +	TAS2557_ASI2_BDIV_CLK_RATIO_REG, 0x81,	/* power up BDIV */
> +	TAS2557_ASI2_WDIV_CLK_RATIO_REG, 0x40,
> +	TAS2557_ASI2_WDIV_CLK_RATIO_REG, 0xc0,	/* power up WDIV */
>
> Can't these be worked out dynamically?

No, not from anything the driver has. TI's own driver never writes
these two registers on any board, and unlike every other multi-bit
register in their header, there's no bitfield documentation for them.
GPIO5/6 are wired as ASI2 BCLK/WCLK inputs here, so the chip is a
clock slave on this interface; BDIV/WDIV sit next to the CLKOUT mux
registers in the map, which points to them being master-mode
clock-generation taps, not something a slave needs. Leaving them as
fixed constants.

> +	TAS2557_POWER_CTRL2_REG, 0xA3,		/* Class-D, Boost, IV-sense power up */
>
> tas2557_isense_put() also writes to this register, there's a
> disconnect with blind writing.

Fixed in v2: enable() now reasserts ISENSE/VSENSE from the cached
switch state right after startup_data, so the mixer controls and the
hardcoded 0xA3 don't fight anymore.

> Is there any overlap with other TI firmware parsers?

Checked tas2781-fmwlib.c. Same magic (0x35353532) but the formats
diverge immediately after: tas2557's header has DDC-name/description
fields tas2781's doesn't, tas2557 needs a PLL block loaded before
power-up which none of tas2781's parser variants have, and tas2781's
parser is built around its multi-amplifier device-index model that
tas2557 doesn't use. Not sharing code between them.

> +	if (!tas2557->fw || !tas2557->powered) {
> +		mutex_unlock(&tas2557->lock);
> +		return IRQ_HANDLED;
> +	}
>
> We definitely didn't handle an IRQ in this case - this will break
> sharing and debug features in the interrupt core.

Fixed in v2, returns IRQ_NONE there.

> +	tas2557_dev_write(tas2557, TAS2557_GPIO4_PIN_REG, 0x00);
>
> Is GPIO4 the only line that can be an interrupt source?

No, any GPIO can be assigned INT via its own PIN_REG. Same situation
as the ASI2 pins above - hardcoded because all four current boards
use GPIO4, rolling it into the same DT-properties follow-up.

Gianluca