On Fri, Mar 08, 2013 at 12:07:13PM +0100, Daniel Mack wrote:
This patch adds a driver for TI's TA5086 6-channel PWM processor.
This chip has a very unusual register layout, specifically because the registers are of unequal size, and multi-byte registers require bulk writes to take effect. Regmap does not support these kind of mappings.
Well, now we have the no-bus mechanism so... doesn't matter anyway though given that in this version the funky registers aren't used.
Currently, the driver does not touch any of the registers >= 0x20, so it doesn't matter, because the register map is mapped to an 8-bit array. In case more features will be added in the future that require access to higher registers, the entire regmap H/W I/O routines have to be open-coded.
We should be able to interoperate with regmap for this somehow, though it'll need us to add lock/unlock access to ensure that the core won't interfere.
+static bool tas5086_volatile_reg(struct device *dev, unsigned int reg) +{
- return reg == TAS5086_DEV_ID ||
reg == TAS5086_ERROR_STATUS;+}
switch please.
+static int tas5086_set_dai_sysclk(struct snd_soc_dai *codec_dai,
int clk_id, unsigned int freq, int dir)+{
- struct snd_soc_codec *codec = codec_dai->codec;
- struct tas5086_private *priv = snd_soc_codec_get_drvdata(codec);
- switch (clk_id) {
- case 0: /* MCLK */
priv->mclk = freq;break;- case 1: /* SCLK */
priv->sclk = freq;break;
Defines in a header please.
+static int tas5086_digital_mute(struct snd_soc_dai *dai, int mute) +{
- struct snd_soc_codec *codec = dai->codec;
- struct tas5086_private *priv = snd_soc_codec_get_drvdata(codec);
- return regmap_write(priv->regmap, TAS5086_SOFT_MUTE,
mute ? 0x3f : 0x00);
Please avoid the ternery operator. It'd be nice to switch over to mute_stream() too.
+#ifdef CONFIG_PM +static int tas5086_soc_suspend(struct snd_soc_codec *codec) +{
- return 0;
+}
Empty functions can just be omitted, though it might make sense to hold the device in reset over suspend.
/* Codec needs ~15ms to wake up */msleep(15);- }
+msleep(300);
- priv->gpio_nreset = gpio_nreset;
Indentation.
+static int tas5086_i2c_probe(struct i2c_client *i2c,
const struct i2c_device_id *id)+{
- struct tas5086_private *priv;
- int ret;
- priv = devm_kzalloc(&i2c->dev, sizeof(*priv), GFP_KERNEL);
- if (!priv)
return -ENOMEM;- priv->regmap = devm_regmap_init_i2c(i2c, &tas5086_regmap);
- if (IS_ERR(priv->regmap)) {
ret = PTR_ERR(priv->regmap);dev_err(&i2c->dev, "Failed to create regmap: %d\n", ret);return ret;- }
- i2c_set_clientdata(i2c, priv);
- return snd_soc_register_codec(&i2c->dev, &soc_codec_dev_tas5086,
&tas5086_dai, 1);
I'd expect things like checking the device ID (and probably most if not all of the basic setup and GPIO free stuff) to be pulled out to the I2C level.