V1->V2:
- Incorporate changes from review feedback
- Change GPIOs to active low
- Create a tas571x_chip struct to capture the growing list of differences
between 5711 and 5717/5719
- Add register defaults for each chip
- Extend regcache_sync_region() to allow it to sync with hardware registers
that don't contain their power-on-reset values
Kevin Cernekee (4):
regmap: cache: Add "was_reset" argument to regcache_sync_region()
ASoC: tas571x: Add DT binding document
ASoC: tas571x: New driver for TI TAS571x power amplifiers
MAINTAINERS: Add entry for tas571x ASoC codec driver
.../devicetree/bindings/sound/tas571x.txt | 41 ++
MAINTAINERS | 6 +
drivers/base/regmap/internal.h | 5 +-
drivers/base/regmap/regcache-lzo.c | 2 +-
drivers/base/regmap/regcache-rbtree.c | 5 +-
drivers/base/regmap/regcache.c | 75 +--
drivers/media/radio/radio-si476x.c | 18 +-
drivers/mfd/wm8994-core.c | 5 +-
include/linux/regmap.h | 4 +-
include/sound/hda_regmap.h | 3 +-
sound/soc/codecs/Kconfig | 5 +
sound/soc/codecs/Makefile | 2 +
sound/soc/codecs/tas571x.c | 521 +++++++++++++++++++++
sound/soc/codecs/tas571x.h | 33 ++
sound/soc/codecs/wm8962.c | 3 +-
15 files changed, 680 insertions(+), 48 deletions(-)
create mode 100644 Documentation/devicetree/bindings/sound/tas571x.txt
create mode 100644 sound/soc/codecs/tas571x.c
create mode 100644 sound/soc/codecs/tas571x.h
--
2.2.0.rc0.207.ga3a616c
regcache_sync() and regcache_sync_region() currently assume that the
hardware has just emerged from a clean reset, and that all registers are
in their default states. But that isn't the only possibility; the device
may have been in a different state in which the registers were
inaccessible but have retained their contents, e.g. clock gating.
So we will extend the more versatile of the two functions,
regcache_sync_region(), to let the caller decide what assumptions should
be made.
One driver that can benefit from this is adau1977, which has hacks to
overwrite the registers that regcache_sync() might have missed. Also,
the powerdown pin on tas571x does not reset the register contents either,
so a similar feature will be required by that driver.
This commit just adds the new argument by changing the function
declarations and call sites, but doesn't wire it up yet.
Signed-off-by: Kevin Cernekee <[email protected]>
---
drivers/base/regmap/internal.h | 5 ++-
drivers/base/regmap/regcache-lzo.c | 2 +-
drivers/base/regmap/regcache-rbtree.c | 5 ++-
drivers/base/regmap/regcache.c | 75 ++++++++++++++++++++---------------
drivers/media/radio/radio-si476x.c | 18 ++++++---
drivers/mfd/wm8994-core.c | 5 ++-
include/linux/regmap.h | 4 +-
include/sound/hda_regmap.h | 3 +-
sound/soc/codecs/wm8962.c | 3 +-
9 files changed, 72 insertions(+), 48 deletions(-)
diff --git a/drivers/base/regmap/internal.h b/drivers/base/regmap/internal.h
index a13587b5c2be..89dfefeb168e 100644
--- a/drivers/base/regmap/internal.h
+++ b/drivers/base/regmap/internal.h
@@ -155,7 +155,8 @@ struct regcache_ops {
#endif
int (*read)(struct regmap *map, unsigned int reg, unsigned int *value);
int (*write)(struct regmap *map, unsigned int reg, unsigned int value);
- int (*sync)(struct regmap *map, unsigned int min, unsigned int max);
+ int (*sync)(struct regmap *map, unsigned int min, unsigned int max,
+ bool was_reset);
int (*drop)(struct regmap *map, unsigned int min, unsigned int max);
};
@@ -215,7 +216,7 @@ int regcache_sync(struct regmap *map);
int regcache_sync_block(struct regmap *map, void *block,
unsigned long *cache_present,
unsigned int block_base, unsigned int start,
- unsigned int end);
+ unsigned int end, bool was_reset);
static inline const void *regcache_get_val_addr(struct regmap *map,
const void *base,
diff --git a/drivers/base/regmap/regcache-lzo.c b/drivers/base/regmap/regcache-lzo.c
index 2d53f6f138e1..52ed0d03ce69 100644
--- a/drivers/base/regmap/regcache-lzo.c
+++ b/drivers/base/regmap/regcache-lzo.c
@@ -332,7 +332,7 @@ out:
}
static int regcache_lzo_sync(struct regmap *map, unsigned int min,
- unsigned int max)
+ unsigned int max, bool was_reset)
{
struct regcache_lzo_ctx **lzo_blocks;
unsigned int val;
diff --git a/drivers/base/regmap/regcache-rbtree.c b/drivers/base/regmap/regcache-rbtree.c
index 81751a49d8bf..8fc1727e635c 100644
--- a/drivers/base/regmap/regcache-rbtree.c
+++ b/drivers/base/regmap/regcache-rbtree.c
@@ -445,7 +445,7 @@ static int regcache_rbtree_write(struct regmap *map, unsigned int reg,
}
static int regcache_rbtree_sync(struct regmap *map, unsigned int min,
- unsigned int max)
+ unsigned int max, bool was_reset)
{
struct regcache_rbtree_ctx *rbtree_ctx;
struct rb_node *node;
@@ -477,7 +477,8 @@ static int regcache_rbtree_sync(struct regmap *map, unsigned int min,
ret = regcache_sync_block(map, rbnode->block,
rbnode->cache_present,
- rbnode->base_reg, start, end);
+ rbnode->base_reg, start, end,
+ was_reset);
if (ret != 0)
return ret;
}
diff --git a/drivers/base/regmap/regcache.c b/drivers/base/regmap/regcache.c
index 7eb7b3b98794..d27b45f50497 100644
--- a/drivers/base/regmap/regcache.c
+++ b/drivers/base/regmap/regcache.c
@@ -250,7 +250,7 @@ int regcache_write(struct regmap *map,
}
static int regcache_default_sync(struct regmap *map, unsigned int min,
- unsigned int max)
+ unsigned int max, bool was_reset)
{
unsigned int reg;
@@ -266,10 +266,12 @@ static int regcache_default_sync(struct regmap *map, unsigned int min,
if (ret)
return ret;
- /* Is this the hardware default? If so skip. */
- ret = regcache_lookup_reg(map, reg);
- if (ret >= 0 && val == map->reg_defaults[ret].def)
- continue;
+ if (was_reset) {
+ /* Is this the hardware default? If so skip. */
+ ret = regcache_lookup_reg(map, reg);
+ if (ret >= 0 && val == map->reg_defaults[ret].def)
+ continue;
+ }
map->cache_bypass = 1;
ret = _regmap_write(map, reg, val);
@@ -294,6 +296,10 @@ static int regcache_default_sync(struct regmap *map, unsigned int min,
* volatile. In general drivers can choose not to use the provided
* syncing functionality if they so require.
*
+ * This assumes that the hardware registers contain their default values.
+ * If the cached value matches the default value for a register, the write
+ * operation will be skipped.
+ *
* Return a negative value on failure, 0 on success.
*/
int regcache_sync(struct regmap *map)
@@ -331,9 +337,9 @@ int regcache_sync(struct regmap *map)
map->cache_bypass = 0;
if (map->cache_ops->sync)
- ret = map->cache_ops->sync(map, 0, map->max_register);
+ ret = map->cache_ops->sync(map, 0, map->max_register, true);
else
- ret = regcache_default_sync(map, 0, map->max_register);
+ ret = regcache_default_sync(map, 0, map->max_register, true);
if (ret == 0)
map->cache_dirty = false;
@@ -353,19 +359,18 @@ out:
EXPORT_SYMBOL_GPL(regcache_sync);
/**
- * regcache_sync_region: Sync part of the register cache with the hardware.
+ * regcache_sync_region: Sync part of the register cache with the hardware.
*
* @map: map to sync.
* @min: first register to sync
* @max: last register to sync
- *
- * Write all non-default register values in the specified region to
- * the hardware.
+ * @was_reset: true if the hardware is known to contain default register values
+ * in the given range; false otherwise
*
* Return a negative value on failure, 0 on success.
*/
int regcache_sync_region(struct regmap *map, unsigned int min,
- unsigned int max)
+ unsigned int max, bool was_reset)
{
int ret = 0;
const char *name;
@@ -389,9 +394,9 @@ int regcache_sync_region(struct regmap *map, unsigned int min,
map->async = true;
if (map->cache_ops->sync)
- ret = map->cache_ops->sync(map, min, max);
+ ret = map->cache_ops->sync(map, min, max, was_reset);
else
- ret = regcache_default_sync(map, min, max);
+ ret = regcache_default_sync(map, min, max, was_reset);
out:
/* Restore the bypass state */
@@ -600,7 +605,8 @@ static bool regcache_reg_present(unsigned long *cache_present, unsigned int idx)
static int regcache_sync_block_single(struct regmap *map, void *block,
unsigned long *cache_present,
unsigned int block_base,
- unsigned int start, unsigned int end)
+ unsigned int start, unsigned int end,
+ bool was_reset)
{
unsigned int i, regtmp, val;
int ret;
@@ -614,10 +620,12 @@ static int regcache_sync_block_single(struct regmap *map, void *block,
val = regcache_get_val(map, block, i);
- /* Is this the hardware default? If so skip. */
- ret = regcache_lookup_reg(map, regtmp);
- if (ret >= 0 && val == map->reg_defaults[ret].def)
- continue;
+ if (was_reset) {
+ /* Is this the hardware default? If so skip. */
+ ret = regcache_lookup_reg(map, regtmp);
+ if (ret >= 0 && val == map->reg_defaults[ret].def)
+ continue;
+ }
map->cache_bypass = 1;
@@ -667,7 +675,7 @@ static int regcache_sync_block_raw_flush(struct regmap *map, const void **data,
static int regcache_sync_block_raw(struct regmap *map, void *block,
unsigned long *cache_present,
unsigned int block_base, unsigned int start,
- unsigned int end)
+ unsigned int end, bool was_reset)
{
unsigned int i, val;
unsigned int regtmp = 0;
@@ -689,14 +697,17 @@ static int regcache_sync_block_raw(struct regmap *map, void *block,
val = regcache_get_val(map, block, i);
- /* Is this the hardware default? If so skip. */
- ret = regcache_lookup_reg(map, regtmp);
- if (ret >= 0 && val == map->reg_defaults[ret].def) {
- ret = regcache_sync_block_raw_flush(map, &data,
- base, regtmp);
- if (ret != 0)
- return ret;
- continue;
+ if (was_reset) {
+ /* Is this the hardware default? If so skip. */
+ ret = regcache_lookup_reg(map, regtmp);
+ if (ret >= 0 && val == map->reg_defaults[ret].def) {
+ ret = regcache_sync_block_raw_flush(map, &data,
+ base,
+ regtmp);
+ if (ret != 0)
+ return ret;
+ continue;
+ }
}
if (!data) {
@@ -712,12 +723,14 @@ static int regcache_sync_block_raw(struct regmap *map, void *block,
int regcache_sync_block(struct regmap *map, void *block,
unsigned long *cache_present,
unsigned int block_base, unsigned int start,
- unsigned int end)
+ unsigned int end, bool was_reset)
{
if (regmap_can_raw_write(map) && !map->use_single_rw)
return regcache_sync_block_raw(map, block, cache_present,
- block_base, start, end);
+ block_base, start, end,
+ was_reset);
else
return regcache_sync_block_single(map, block, cache_present,
- block_base, start, end);
+ block_base, start, end,
+ was_reset);
}
diff --git a/drivers/media/radio/radio-si476x.c b/drivers/media/radio/radio-si476x.c
index dccf58691650..ff4785f0416d 100644
--- a/drivers/media/radio/radio-si476x.c
+++ b/drivers/media/radio/radio-si476x.c
@@ -567,19 +567,22 @@ static int si476x_radio_do_post_powerup_init(struct si476x_radio *radio,
/* regcache_mark_dirty(radio->core->regmap); */
err = regcache_sync_region(radio->core->regmap,
SI476X_PROP_DIGITAL_IO_INPUT_SAMPLE_RATE,
- SI476X_PROP_DIGITAL_IO_OUTPUT_FORMAT);
+ SI476X_PROP_DIGITAL_IO_OUTPUT_FORMAT,
+ true);
if (err < 0)
return err;
err = regcache_sync_region(radio->core->regmap,
SI476X_PROP_AUDIO_DEEMPHASIS,
- SI476X_PROP_AUDIO_PWR_LINE_FILTER);
+ SI476X_PROP_AUDIO_PWR_LINE_FILTER,
+ true);
if (err < 0)
return err;
err = regcache_sync_region(radio->core->regmap,
SI476X_PROP_INT_CTL_ENABLE,
- SI476X_PROP_INT_CTL_ENABLE);
+ SI476X_PROP_INT_CTL_ENABLE,
+ true);
if (err < 0)
return err;
@@ -589,13 +592,15 @@ static int si476x_radio_do_post_powerup_init(struct si476x_radio *radio,
*/
err = regcache_sync_region(radio->core->regmap,
SI476X_PROP_VALID_MAX_TUNE_ERROR,
- SI476X_PROP_VALID_MAX_TUNE_ERROR);
+ SI476X_PROP_VALID_MAX_TUNE_ERROR,
+ true);
if (err < 0)
return err;
err = regcache_sync_region(radio->core->regmap,
SI476X_PROP_VALID_SNR_THRESHOLD,
- SI476X_PROP_VALID_RSSI_THRESHOLD);
+ SI476X_PROP_VALID_RSSI_THRESHOLD,
+ true);
if (err < 0)
return err;
@@ -609,7 +614,8 @@ static int si476x_radio_do_post_powerup_init(struct si476x_radio *radio,
err = regcache_sync_region(radio->core->regmap,
SI476X_PROP_FM_RDS_INTERRUPT_SOURCE,
- SI476X_PROP_FM_RDS_CONFIG);
+ SI476X_PROP_FM_RDS_CONFIG,
+ true);
if (err < 0)
return err;
}
diff --git a/drivers/mfd/wm8994-core.c b/drivers/mfd/wm8994-core.c
index 53ae5af5d6e4..d5632634a362 100644
--- a/drivers/mfd/wm8994-core.c
+++ b/drivers/mfd/wm8994-core.c
@@ -158,14 +158,15 @@ static int wm8994_suspend(struct device *dev)
* pin configurations.
*/
ret = regcache_sync_region(wm8994->regmap, WM8994_GPIO_1,
- WM8994_GPIO_11);
+ WM8994_GPIO_11, true);
if (ret != 0)
dev_err(dev, "Failed to restore GPIO registers: %d\n", ret);
/* In case one of the GPIOs is used as a wake input. */
ret = regcache_sync_region(wm8994->regmap,
WM8994_INTERRUPT_STATUS_1_MASK,
- WM8994_INTERRUPT_STATUS_1_MASK);
+ WM8994_INTERRUPT_STATUS_1_MASK,
+ true);
if (ret != 0)
dev_err(dev, "Failed to restore interrupt mask: %d\n", ret);
diff --git a/include/linux/regmap.h b/include/linux/regmap.h
index 116655d92269..ece122a6fdeb 100644
--- a/include/linux/regmap.h
+++ b/include/linux/regmap.h
@@ -438,7 +438,7 @@ bool regmap_can_raw_write(struct regmap *map);
int regcache_sync(struct regmap *map);
int regcache_sync_region(struct regmap *map, unsigned int min,
- unsigned int max);
+ unsigned int max, bool was_reset);
int regcache_drop_region(struct regmap *map, unsigned int min,
unsigned int max);
void regcache_cache_only(struct regmap *map, bool enable);
@@ -683,7 +683,7 @@ static inline int regcache_sync(struct regmap *map)
}
static inline int regcache_sync_region(struct regmap *map, unsigned int min,
- unsigned int max)
+ unsigned int max, bool was_reset)
{
WARN_ONCE(1, "regmap API is disabled");
return -EINVAL;
diff --git a/include/sound/hda_regmap.h b/include/sound/hda_regmap.h
index 53a18b3635e2..f6bb68137ca4 100644
--- a/include/sound/hda_regmap.h
+++ b/include/sound/hda_regmap.h
@@ -211,7 +211,8 @@ static inline void
snd_hdac_regmap_sync_node(struct hdac_device *codec, hda_nid_t nid)
{
regcache_mark_dirty(codec->regmap);
- regcache_sync_region(codec->regmap, nid << 20, ((nid + 1) << 20) - 1);
+ regcache_sync_region(codec->regmap, nid << 20,
+ ((nid + 1) << 20) - 1, true);
}
#endif /* __SOUND_HDA_REGMAP_H */
diff --git a/sound/soc/codecs/wm8962.c b/sound/soc/codecs/wm8962.c
index 118b0034ba23..3a25d2d93705 100644
--- a/sound/soc/codecs/wm8962.c
+++ b/sound/soc/codecs/wm8962.c
@@ -1483,7 +1483,8 @@ static int wm8962_dsp2_write_config(struct snd_soc_codec *codec)
struct wm8962_priv *wm8962 = snd_soc_codec_get_drvdata(codec);
return regcache_sync_region(wm8962->regmap,
- WM8962_HDBASS_AI_1, WM8962_MAX_REGISTER);
+ WM8962_HDBASS_AI_1, WM8962_MAX_REGISTER,
+ true);
}
static int wm8962_dsp2_set_enable(struct snd_soc_codec *codec, u16 val)
--
2.2.0.rc0.207.ga3a616c
Document the bindings for the soon-to-be-added tas571x driver.
Signed-off-by: Kevin Cernekee <[email protected]>
---
.../devicetree/bindings/sound/tas571x.txt | 41 ++++++++++++++++++++++
1 file changed, 41 insertions(+)
create mode 100644 Documentation/devicetree/bindings/sound/tas571x.txt
diff --git a/Documentation/devicetree/bindings/sound/tas571x.txt b/Documentation/devicetree/bindings/sound/tas571x.txt
new file mode 100644
index 000000000000..0ac31d8d5ac4
--- /dev/null
+++ b/Documentation/devicetree/bindings/sound/tas571x.txt
@@ -0,0 +1,41 @@
+Texas Instruments TAS5711/TAS5717/TAS5719 stereo power amplifiers
+
+The codec is controlled through an I2C interface. It also has two other
+signals that can be wired up to GPIOs: reset (strongly recommended), and
+powerdown (optional).
+
+Required properties:
+
+- compatible: "ti,tas5711", "ti,tas5717", or "ti,tas5719"
+- reg: The I2C address of the device
+- #sound-dai-cells: must be equal to 0
+
+Optional properties:
+
+- reset-gpios: GPIO specifier for the TAS571x's active low reset line
+- pdn-gpios: GPIO specifier for the TAS571x's active low powerdown line
+- clocks: clock phandle for the MCLK input
+- clock-names: should be "mclk"
+- AVDD-supply: regulator phandle for the AVDD supply (all chips)
+- DVDD-supply: regulator phandle for the DVDD supply (all chips)
+- HPVDD-supply: regulator phandle for the HPVDD supply (5717/5719)
+- PVDD_AB-supply: regulator phandle for the PVDD_AB supply (5717/5719)
+- PVDD_CD-supply: regulator phandle for the PVDD_CD supply (5717/5719)
+- PVDD_A-supply: regulator phandle for the PVDD_A supply (5711)
+- PVDD_B-supply: regulator phandle for the PVDD_B supply (5711)
+- PVDD_C-supply: regulator phandle for the PVDD_C supply (5711)
+- PVDD_D-supply: regulator phandle for the PVDD_D supply (5711)
+
+Example:
+
+ tas5717: audio-codec@2a {
+ compatible = "ti,tas5717";
+ reg = <0x2a>;
+ #sound-dai-cells = <0>;
+
+ reset-gpios = <&gpio5 1 GPIO_ACTIVE_LOW>;
+ pdn-gpios = <&gpio5 2 GPIO_ACTIVE_LOW>;
+
+ clocks = <&clk_core CLK_I2S>;
+ clock-names = "mclk";
+ };
--
2.2.0.rc0.207.ga3a616c
Introduce a new codec driver for the Texas Instruments
TAS5711/TAS5717/TAS5719 power amplifier chips. These chips are typically
used to take an I2S digital audio input and drive 10-20W into a pair of
speakers.
Signed-off-by: Kevin Cernekee <[email protected]>
---
sound/soc/codecs/Kconfig | 5 +
sound/soc/codecs/Makefile | 2 +
sound/soc/codecs/tas571x.c | 521 +++++++++++++++++++++++++++++++++++++++++++++
sound/soc/codecs/tas571x.h | 33 +++
4 files changed, 561 insertions(+)
create mode 100644 sound/soc/codecs/tas571x.c
create mode 100644 sound/soc/codecs/tas571x.h
diff --git a/sound/soc/codecs/Kconfig b/sound/soc/codecs/Kconfig
index 061c46587628..befff910d71a 100644
--- a/sound/soc/codecs/Kconfig
+++ b/sound/soc/codecs/Kconfig
@@ -104,6 +104,7 @@ config SND_SOC_ALL_CODECS
select SND_SOC_STAC9766 if SND_SOC_AC97_BUS
select SND_SOC_TAS2552 if I2C
select SND_SOC_TAS5086 if I2C
+ select SND_SOC_TAS571X if I2C
select SND_SOC_TFA9879 if I2C
select SND_SOC_TLV320AIC23_I2C if I2C
select SND_SOC_TLV320AIC23_SPI if SPI_MASTER
@@ -611,6 +612,10 @@ config SND_SOC_TAS5086
tristate "Texas Instruments TAS5086 speaker amplifier"
depends on I2C
+config SND_SOC_TAS571X
+ tristate "Texas Instruments TAS5711/TAS5717/TAS5719 power amplifiers"
+ depends on I2C
+
config SND_SOC_TFA9879
tristate "NXP Semiconductors TFA9879 amplifier"
depends on I2C
diff --git a/sound/soc/codecs/Makefile b/sound/soc/codecs/Makefile
index abe2d7edf65c..3dcf5ac85e89 100644
--- a/sound/soc/codecs/Makefile
+++ b/sound/soc/codecs/Makefile
@@ -106,6 +106,7 @@ snd-soc-sta350-objs := sta350.o
snd-soc-sta529-objs := sta529.o
snd-soc-stac9766-objs := stac9766.o
snd-soc-tas5086-objs := tas5086.o
+snd-soc-tas571x-objs := tas571x.o
snd-soc-tfa9879-objs := tfa9879.o
snd-soc-tlv320aic23-objs := tlv320aic23.o
snd-soc-tlv320aic23-i2c-objs := tlv320aic23-i2c.o
@@ -288,6 +289,7 @@ obj-$(CONFIG_SND_SOC_STA529) += snd-soc-sta529.o
obj-$(CONFIG_SND_SOC_STAC9766) += snd-soc-stac9766.o
obj-$(CONFIG_SND_SOC_TAS2552) += snd-soc-tas2552.o
obj-$(CONFIG_SND_SOC_TAS5086) += snd-soc-tas5086.o
+obj-$(CONFIG_SND_SOC_TAS571X) += snd-soc-tas571x.o
obj-$(CONFIG_SND_SOC_TFA9879) += snd-soc-tfa9879.o
obj-$(CONFIG_SND_SOC_TLV320AIC23) += snd-soc-tlv320aic23.o
obj-$(CONFIG_SND_SOC_TLV320AIC23_I2C) += snd-soc-tlv320aic23-i2c.o
diff --git a/sound/soc/codecs/tas571x.c b/sound/soc/codecs/tas571x.c
new file mode 100644
index 000000000000..08e358638f2e
--- /dev/null
+++ b/sound/soc/codecs/tas571x.c
@@ -0,0 +1,521 @@
+/*
+ * TAS571x amplifier audio driver
+ *
+ * Copyright (C) 2015 Google, Inc.
+ * Copyright (c) 2013 Daniel Mack <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ */
+
+#include <linux/clk.h>
+#include <linux/delay.h>
+#include <linux/device.h>
+#include <linux/gpio/consumer.h>
+#include <linux/i2c.h>
+#include <linux/init.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of_device.h>
+#include <linux/regmap.h>
+#include <linux/regulator/consumer.h>
+#include <linux/stddef.h>
+#include <sound/pcm_params.h>
+#include <sound/soc.h>
+#include <sound/tlv.h>
+
+#include "tas571x.h"
+
+#define TAS571X_MAX_SUPPLIES 6
+
+struct tas571x_chip {
+ const char *const *supply_names;
+ int num_supply_names;
+ const struct snd_kcontrol_new *controls;
+ int num_controls;
+ const struct regmap_config *regmap_config;
+ int vol_reg_size;
+};
+
+struct tas571x_private {
+ const struct tas571x_chip *chip;
+ struct regmap *regmap;
+ struct regulator_bulk_data supplies[TAS571X_MAX_SUPPLIES];
+ struct clk *mclk;
+ unsigned int format;
+ struct gpio_desc *reset_gpio;
+ struct gpio_desc *pdn_gpio;
+ struct snd_soc_codec_driver codec_driver;
+};
+
+static int tas571x_register_size(struct tas571x_private *priv, unsigned int reg)
+{
+ switch (reg) {
+ case TAS571X_MVOL_REG:
+ case TAS571X_CH1_VOL_REG:
+ case TAS571X_CH2_VOL_REG:
+ return priv->chip->vol_reg_size;
+ default:
+ return 1;
+ }
+}
+
+static int tas571x_reg_write(void *context, unsigned int reg,
+ unsigned int value)
+{
+ struct i2c_client *client = context;
+ struct tas571x_private *priv = i2c_get_clientdata(client);
+ unsigned int i, size;
+ uint8_t buf[5];
+ int ret;
+
+ size = tas571x_register_size(priv, reg);
+ buf[0] = reg;
+
+ for (i = size; i >= 1; --i) {
+ buf[i] = value;
+ value >>= 8;
+ }
+
+ ret = i2c_master_send(client, buf, size + 1);
+ if (ret == size + 1)
+ return 0;
+ else if (ret < 0)
+ return ret;
+ else
+ return -EIO;
+}
+
+static int tas571x_reg_read(void *context, unsigned int reg,
+ unsigned int *value)
+{
+ struct i2c_client *client = context;
+ struct tas571x_private *priv = i2c_get_clientdata(client);
+ uint8_t send_buf, recv_buf[4];
+ struct i2c_msg msgs[2];
+ unsigned int size;
+ unsigned int i;
+ int ret;
+
+ size = tas571x_register_size(priv, reg);
+ send_buf = reg;
+
+ msgs[0].addr = client->addr;
+ msgs[0].len = sizeof(send_buf);
+ msgs[0].buf = &send_buf;
+ msgs[0].flags = 0;
+
+ msgs[1].addr = client->addr;
+ msgs[1].len = size;
+ msgs[1].buf = recv_buf;
+ msgs[1].flags = I2C_M_RD;
+
+ ret = i2c_transfer(client->adapter, msgs, ARRAY_SIZE(msgs));
+ if (ret < 0)
+ return ret;
+ else if (ret != ARRAY_SIZE(msgs))
+ return -EIO;
+
+ *value = 0;
+
+ for (i = 0; i < size; i++) {
+ *value <<= 8;
+ *value |= recv_buf[i];
+ }
+
+ return 0;
+}
+
+static int tas571x_set_dai_fmt(struct snd_soc_dai *dai, unsigned int format)
+{
+ struct tas571x_private *priv = snd_soc_codec_get_drvdata(dai->codec);
+
+ priv->format = format;
+
+ return 0;
+}
+
+static int tas571x_hw_params(struct snd_pcm_substream *substream,
+ struct snd_pcm_hw_params *params,
+ struct snd_soc_dai *dai)
+{
+ struct tas571x_private *priv = snd_soc_codec_get_drvdata(dai->codec);
+ u32 val;
+
+ switch (priv->format & SND_SOC_DAIFMT_FORMAT_MASK) {
+ case SND_SOC_DAIFMT_RIGHT_J:
+ val = 0x00;
+ break;
+ case SND_SOC_DAIFMT_I2S:
+ val = 0x03;
+ break;
+ case SND_SOC_DAIFMT_LEFT_J:
+ val = 0x06;
+ break;
+ default:
+ return -EINVAL;
+ }
+
+ if (params_width(params) >= 24)
+ val += 2;
+ else if (params_width(params) >= 20)
+ val += 1;
+
+ return regmap_update_bits(priv->regmap, TAS571X_SDI_REG,
+ TAS571X_SDI_FMT_MASK, val);
+}
+
+static int tas571x_set_bias_level(struct snd_soc_codec *codec,
+ enum snd_soc_bias_level level)
+{
+ struct tas571x_private *priv = snd_soc_codec_get_drvdata(codec);
+ int ret;
+
+ switch (level) {
+ case SND_SOC_BIAS_ON:
+ break;
+ case SND_SOC_BIAS_PREPARE:
+ break;
+ case SND_SOC_BIAS_STANDBY:
+ if (codec->dapm.bias_level == SND_SOC_BIAS_OFF) {
+ if (!IS_ERR(priv->mclk)) {
+ ret = clk_prepare_enable(priv->mclk);
+ if (ret) {
+ dev_err(codec->dev,
+ "Failed to enable master clock: %d\n",
+ ret);
+ return ret;
+ }
+ }
+
+ gpiod_set_value(priv->pdn_gpio, 0);
+ usleep_range(5000, 6000);
+
+ regcache_cache_only(priv->regmap, false);
+ ret = regcache_sync_region(priv->regmap, 0,
+ priv->chip->regmap_config->max_register, false);
+ if (ret)
+ return ret;
+ }
+ break;
+ case SND_SOC_BIAS_OFF:
+ regcache_cache_only(priv->regmap, true);
+ gpiod_set_value(priv->pdn_gpio, 1);
+
+ if (!IS_ERR(priv->mclk))
+ clk_disable_unprepare(priv->mclk);
+ break;
+ }
+
+ codec->dapm.bias_level = level;
+ return 0;
+}
+
+static const struct snd_soc_dai_ops tas571x_dai_ops = {
+ .set_fmt = tas571x_set_dai_fmt,
+ .hw_params = tas571x_hw_params,
+};
+
+static const char *const tas5711_supply_names[] = {
+ "AVDD",
+ "DVDD",
+ "PVDD_A",
+ "PVDD_B",
+ "PVDD_C",
+ "PVDD_D",
+};
+
+static const DECLARE_TLV_DB_SCALE(tas5711_volume_tlv, -10350, 50, 1);
+
+static const struct snd_kcontrol_new tas5711_controls[] = {
+ SOC_SINGLE_TLV("Master Volume",
+ TAS571X_MVOL_REG,
+ 0, 0xff, 1, tas5711_volume_tlv),
+ SOC_DOUBLE_R_TLV("Speaker Volume",
+ TAS571X_CH1_VOL_REG,
+ TAS571X_CH2_VOL_REG,
+ 0, 0xff, 1, tas5711_volume_tlv),
+ SOC_DOUBLE("Speaker Switch",
+ TAS571X_SOFT_MUTE_REG,
+ TAS571X_SOFT_MUTE_CH1_SHIFT, TAS571X_SOFT_MUTE_CH2_SHIFT,
+ 1, 1),
+};
+
+static const struct reg_default tas5711_reg_defaults[] = {
+ { 0x04, 0x05 },
+ { 0x05, 0x40 },
+ { 0x06, 0x00 },
+ { 0x07, 0xff },
+ { 0x08, 0x30 },
+ { 0x09, 0x30 },
+ { 0x1b, 0x82 },
+};
+
+static const struct regmap_config tas5711_regmap_config = {
+ .reg_bits = 8,
+ .val_bits = 32,
+ .max_register = 0xff,
+ .reg_read = tas571x_reg_read,
+ .reg_write = tas571x_reg_write,
+ .reg_defaults = tas5711_reg_defaults,
+ .num_reg_defaults = ARRAY_SIZE(tas5711_reg_defaults),
+ .cache_type = REGCACHE_RBTREE,
+};
+
+static const struct tas571x_chip tas5711_chip = {
+ .supply_names = tas5711_supply_names,
+ .num_supply_names = ARRAY_SIZE(tas5711_supply_names),
+ .controls = tas5711_controls,
+ .num_controls = ARRAY_SIZE(tas5711_controls),
+ .regmap_config = &tas5711_regmap_config,
+ .vol_reg_size = 1,
+};
+
+static const char *const tas5717_supply_names[] = {
+ "AVDD",
+ "DVDD",
+ "HPVDD",
+ "PVDD_AB",
+ "PVDD_CD",
+};
+
+static const DECLARE_TLV_DB_SCALE(tas5717_volume_tlv, -10375, 25, 0);
+
+static const struct snd_kcontrol_new tas5717_controls[] = {
+ /* MVOL LSB is ignored - see comments in tas571x_i2c_probe() */
+ SOC_SINGLE_TLV("Master Volume",
+ TAS571X_MVOL_REG, 1, 0x1ff, 1,
+ tas5717_volume_tlv),
+ SOC_DOUBLE_R_TLV("Speaker Volume",
+ TAS571X_CH1_VOL_REG, TAS571X_CH2_VOL_REG,
+ 1, 0x1ff, 1, tas5717_volume_tlv),
+ SOC_DOUBLE("Speaker Switch",
+ TAS571X_SOFT_MUTE_REG,
+ TAS571X_SOFT_MUTE_CH1_SHIFT, TAS571X_SOFT_MUTE_CH2_SHIFT,
+ 1, 1),
+};
+
+static const struct reg_default tas5717_reg_defaults[] = {
+ { 0x04, 0x05 },
+ { 0x05, 0x40 },
+ { 0x06, 0x00 },
+ { 0x07, 0x03ff },
+ { 0x08, 0x00c0 },
+ { 0x09, 0x00c0 },
+ { 0x1b, 0x82 },
+};
+
+static const struct regmap_config tas5717_regmap_config = {
+ .reg_bits = 8,
+ .val_bits = 32,
+ .max_register = 0xff,
+ .reg_read = tas571x_reg_read,
+ .reg_write = tas571x_reg_write,
+ .reg_defaults = tas5717_reg_defaults,
+ .num_reg_defaults = ARRAY_SIZE(tas5717_reg_defaults),
+ .cache_type = REGCACHE_RBTREE,
+};
+
+/* This entry is reused for tas5719 as the software interface is identical. */
+static const struct tas571x_chip tas5717_chip = {
+ .supply_names = tas5717_supply_names,
+ .num_supply_names = ARRAY_SIZE(tas5717_supply_names),
+ .controls = tas5717_controls,
+ .num_controls = ARRAY_SIZE(tas5717_controls),
+ .regmap_config = &tas5717_regmap_config,
+ .vol_reg_size = 2,
+};
+
+static const struct snd_soc_dapm_widget tas571x_dapm_widgets[] = {
+ SND_SOC_DAPM_DAC("DACL", NULL, SND_SOC_NOPM, 0, 0),
+ SND_SOC_DAPM_DAC("DACR", NULL, SND_SOC_NOPM, 0, 0),
+
+ SND_SOC_DAPM_OUTPUT("OUT_A"),
+ SND_SOC_DAPM_OUTPUT("OUT_B"),
+ SND_SOC_DAPM_OUTPUT("OUT_C"),
+ SND_SOC_DAPM_OUTPUT("OUT_D"),
+};
+
+static const struct snd_soc_dapm_route tas571x_dapm_routes[] = {
+ { "DACL", NULL, "Playback" },
+ { "DACR", NULL, "Playback" },
+
+ { "OUT_A", NULL, "DACL" },
+ { "OUT_B", NULL, "DACL" },
+ { "OUT_C", NULL, "DACR" },
+ { "OUT_D", NULL, "DACR" },
+};
+
+static const struct snd_soc_codec_driver tas571x_codec = {
+ .set_bias_level = tas571x_set_bias_level,
+ .idle_bias_off = true,
+
+ .dapm_widgets = tas571x_dapm_widgets,
+ .num_dapm_widgets = ARRAY_SIZE(tas571x_dapm_widgets),
+ .dapm_routes = tas571x_dapm_routes,
+ .num_dapm_routes = ARRAY_SIZE(tas571x_dapm_routes),
+};
+
+static struct snd_soc_dai_driver tas571x_dai = {
+ .name = "tas571x-hifi",
+ .playback = {
+ .stream_name = "Playback",
+ .channels_min = 2,
+ .channels_max = 2,
+ .rates = SNDRV_PCM_RATE_8000_48000,
+ .formats = SNDRV_PCM_FMTBIT_S32_LE |
+ SNDRV_PCM_FMTBIT_S24_LE |
+ SNDRV_PCM_FMTBIT_S16_LE,
+ },
+ .ops = &tas571x_dai_ops,
+};
+
+static const struct of_device_id tas571x_of_match[];
+
+static int tas571x_i2c_probe(struct i2c_client *client,
+ const struct i2c_device_id *id)
+{
+ struct tas571x_private *priv;
+ struct device *dev = &client->dev;
+ int i, ret;
+
+ priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+ if (!priv)
+ return -ENOMEM;
+ i2c_set_clientdata(client, priv);
+
+ if (dev->of_node) {
+ const struct of_device_id *of_id;
+
+ of_id = of_match_device(tas571x_of_match, dev);
+ if (of_id)
+ priv->chip = of_id->data;
+ }
+
+ if (!priv->chip) {
+ dev_err(dev, "Unknown device type\n");
+ return -EINVAL;
+ }
+
+ priv->mclk = devm_clk_get(dev, "mclk");
+ if (IS_ERR(priv->mclk) && PTR_ERR(priv->mclk) != -ENOENT) {
+ dev_err(dev, "Failed to request mclk: %ld\n",
+ PTR_ERR(priv->mclk));
+ return PTR_ERR(priv->mclk);
+ }
+
+ BUG_ON(priv->chip->num_supply_names > TAS571X_MAX_SUPPLIES);
+ for (i = 0; i < priv->chip->num_supply_names; i++)
+ priv->supplies[i].supply = priv->chip->supply_names[i];
+
+ ret = devm_regulator_bulk_get(dev, priv->chip->num_supply_names,
+ priv->supplies);
+ if (ret) {
+ dev_err(dev, "Failed to get supplies: %d\n", ret);
+ return ret;
+ }
+ ret = regulator_bulk_enable(priv->chip->num_supply_names,
+ priv->supplies);
+ if (ret) {
+ dev_err(dev, "Failed to enable supplies: %d\n", ret);
+ return ret;
+ }
+
+ priv->regmap = devm_regmap_init(dev, NULL, client,
+ priv->chip->regmap_config);
+ if (IS_ERR(priv->regmap))
+ return PTR_ERR(priv->regmap);
+
+ priv->pdn_gpio = devm_gpiod_get_optional(dev, "pdn", GPIOD_OUT_LOW);
+ if (IS_ERR(priv->pdn_gpio)) {
+ dev_err(dev, "error requesting pdn_gpio: %ld\n",
+ PTR_ERR(priv->pdn_gpio));
+ return PTR_ERR(priv->pdn_gpio);
+ }
+
+ priv->reset_gpio = devm_gpiod_get_optional(dev, "reset",
+ GPIOD_OUT_HIGH);
+ if (IS_ERR(priv->reset_gpio)) {
+ dev_err(dev, "error requesting reset_gpio: %ld\n",
+ PTR_ERR(priv->reset_gpio));
+ return PTR_ERR(priv->reset_gpio);
+ } else if (priv->reset_gpio) {
+ /* pulse the active low reset line for ~100us */
+ usleep_range(100, 200);
+ gpiod_set_value(priv->reset_gpio, 0);
+ usleep_range(12000, 20000);
+ }
+
+ ret = regmap_write(priv->regmap, TAS571X_OSC_TRIM_REG, 0);
+ if (ret)
+ return ret;
+
+ ret = regmap_update_bits(priv->regmap, TAS571X_SYS_CTRL_2_REG,
+ TAS571X_SYS_CTRL_2_SDN_MASK, 0);
+ if (ret)
+ return ret;
+
+ memcpy(&priv->codec_driver, &tas571x_codec, sizeof(priv->codec_driver));
+ priv->codec_driver.controls = priv->chip->controls;
+ priv->codec_driver.num_controls = priv->chip->num_controls;
+
+ if (priv->chip->vol_reg_size == 2) {
+ /*
+ * The master volume defaults to 0x3ff (mute), but we ignore
+ * (zero) the LSB because the hardware step size is 0.125 dB
+ * and TLV_DB_SCALE_ITEM has a resolution of 0.01 dB.
+ */
+ ret = regmap_update_bits(priv->regmap, TAS571X_MVOL_REG, 1, 0);
+ if (ret)
+ return ret;
+ }
+
+ regcache_cache_only(priv->regmap, true);
+ gpiod_set_value(priv->pdn_gpio, 1);
+
+ return snd_soc_register_codec(&client->dev, &priv->codec_driver,
+ &tas571x_dai, 1);
+}
+
+static int tas571x_i2c_remove(struct i2c_client *client)
+{
+ struct tas571x_private *priv = i2c_get_clientdata(client);
+
+ snd_soc_unregister_codec(&client->dev);
+ regulator_bulk_disable(priv->chip->num_supply_names, priv->supplies);
+
+ return 0;
+}
+
+static const struct of_device_id tas571x_of_match[] = {
+ { .compatible = "ti,tas5711", .data = &tas5711_chip, },
+ { .compatible = "ti,tas5717", .data = &tas5717_chip, },
+ { .compatible = "ti,tas5719", .data = &tas5717_chip, },
+ { }
+};
+MODULE_DEVICE_TABLE(of, tas571x_of_match);
+
+static const struct i2c_device_id tas571x_i2c_id[] = {
+ { "tas5711", 0 },
+ { "tas5717", 0 },
+ { "tas5719", 0 },
+ { }
+};
+MODULE_DEVICE_TABLE(i2c, tas571x_i2c_id);
+
+static struct i2c_driver tas571x_i2c_driver = {
+ .driver = {
+ .name = "tas571x",
+ .of_match_table = of_match_ptr(tas571x_of_match),
+ },
+ .probe = tas571x_i2c_probe,
+ .remove = tas571x_i2c_remove,
+ .id_table = tas571x_i2c_id,
+};
+module_i2c_driver(tas571x_i2c_driver);
+
+MODULE_DESCRIPTION("ASoC TAS571x driver");
+MODULE_AUTHOR("Kevin Cernekee <[email protected]>");
+MODULE_LICENSE("GPL");
diff --git a/sound/soc/codecs/tas571x.h b/sound/soc/codecs/tas571x.h
new file mode 100644
index 000000000000..0aee471232cd
--- /dev/null
+++ b/sound/soc/codecs/tas571x.h
@@ -0,0 +1,33 @@
+/*
+ * TAS571x amplifier audio driver
+ *
+ * Copyright (C) 2015 Google, Inc.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ */
+
+#ifndef _TAS571X_H
+#define _TAS571X_H
+
+/* device registers */
+#define TAS571X_SDI_REG 0x04
+#define TAS571X_SDI_FMT_MASK 0x0f
+
+#define TAS571X_SYS_CTRL_2_REG 0x05
+#define TAS571X_SYS_CTRL_2_SDN_MASK 0x40
+
+#define TAS571X_SOFT_MUTE_REG 0x06
+#define TAS571X_SOFT_MUTE_CH1_SHIFT 0
+#define TAS571X_SOFT_MUTE_CH2_SHIFT 1
+#define TAS571X_SOFT_MUTE_CH3_SHIFT 2
+
+#define TAS571X_MVOL_REG 0x07
+#define TAS571X_CH1_VOL_REG 0x08
+#define TAS571X_CH2_VOL_REG 0x09
+
+#define TAS571X_OSC_TRIM_REG 0x1b
+
+#endif /* _TAS571X_H */
--
2.2.0.rc0.207.ga3a616c
Add self as maintainer for the new driver.
Signed-off-by: Kevin Cernekee <[email protected]>
---
MAINTAINERS | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/MAINTAINERS b/MAINTAINERS
index ea0001760035..15153fc37cc4 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -9878,6 +9878,12 @@ L: [email protected]
S: Maintained
F: drivers/net/ethernet/ti/netcp*
+TI TAS571X FAMILY ASoC CODEC DRIVER
+M: Kevin Cernekee <[email protected]>
+L: [email protected] (moderated for non-subscribers)
+S: Odd Fixes
+F: sound/soc/codecs/tas571x*
+
TI TWL4030 SERIES SOC CODEC DRIVER
M: Peter Ujfalusi <[email protected]>
L: [email protected] (moderated for non-subscribers)
--
2.2.0.rc0.207.ga3a616c
On Fri, Apr 24, 2015 at 3:36 PM, Kevin Cernekee <[email protected]> wrote:
> regcache_sync() and regcache_sync_region() currently assume that the
> hardware has just emerged from a clean reset, and that all registers are
> in their default states. But that isn't the only possibility; the device
> may have been in a different state in which the registers were
> inaccessible but have retained their contents, e.g. clock gating.
>
> So we will extend the more versatile of the two functions,
> regcache_sync_region(), to let the caller decide what assumptions should
> be made.
>
> One driver that can benefit from this is adau1977, which has hacks to
> overwrite the registers that regcache_sync() might have missed. Also,
> the powerdown pin on tas571x does not reset the register contents either,
> so a similar feature will be required by that driver.
>
> This commit just adds the new argument by changing the function
> declarations and call sites, but doesn't wire it up yet.
The last two lines of the changelog were inadvertently carried over
from an older version of the patch, and should be deleted. Will fix
in V3.
On 04/25/2015 12:36 AM, Kevin Cernekee wrote:
> regcache_sync() and regcache_sync_region() currently assume that the
> hardware has just emerged from a clean reset, and that all registers are
> in their default states. But that isn't the only possibility; the device
> may have been in a different state in which the registers were
> inaccessible but have retained their contents, e.g. clock gating.
>
> So we will extend the more versatile of the two functions,
> regcache_sync_region(), to let the caller decide what assumptions should
> be made.
>
> One driver that can benefit from this is adau1977, which has hacks to
> overwrite the registers that regcache_sync() might have missed.
The issue with the adau1977 is slightly different, there is only one single
register which is not affected, so we need special handling for that
register. For all other registers the default behavior of regcache_sync() is
correct.
> Also, the powerdown pin on tas571x does not reset the register contents
> either, so a similar feature will be required by that driver.
>
> This commit just adds the new argument by changing the function
> declarations and call sites, but doesn't wire it up yet.
I think a better approach is to introduce a new flag internally, similar to
the cache_dirty flag. Set both this new flag and cache_dirty when
regcache_mark_dirty() is called. But only cache_dirty when a write is
performed when the regmap is marked as cache only. Now in regcache_sync()
and friends check this new flag and only when it is set skip registers which
match the default. At the end of regcache_sync() clear both flags.
[...]
> @@ -600,7 +605,8 @@ static bool regcache_reg_present(unsigned long *cache_present, unsigned int idx)
> static int regcache_sync_block_single(struct regmap *map, void *block,
> unsigned long *cache_present,
> unsigned int block_base,
> - unsigned int start, unsigned int end)
> + unsigned int start, unsigned int end,
> + bool was_reset)
> {
> unsigned int i, regtmp, val;
> int ret;
> @@ -614,10 +620,12 @@ static int regcache_sync_block_single(struct regmap *map, void *block,
>
> val = regcache_get_val(map, block, i);
>
> - /* Is this the hardware default? If so skip. */
> - ret = regcache_lookup_reg(map, regtmp);
> - if (ret >= 0 && val == map->reg_defaults[ret].def)
> - continue;
> + if (was_reset) {
> + /* Is this the hardware default? If so skip. */
> + ret = regcache_lookup_reg(map, regtmp);
> + if (ret >= 0 && val == map->reg_defaults[ret].def)
> + continue;
> + }
Probably factor the check whether wee need to sync or not into a helper
function, that can be used here and below in sync_block_raw().
[...]
> @@ -689,14 +697,17 @@ static int regcache_sync_block_raw(struct regmap *map, void *block,
>
> val = regcache_get_val(map, block, i);
>
> - /* Is this the hardware default? If so skip. */
> - ret = regcache_lookup_reg(map, regtmp);
> - if (ret >= 0 && val == map->reg_defaults[ret].def) {
> - ret = regcache_sync_block_raw_flush(map, &data,
> - base, regtmp);
> - if (ret != 0)
> - return ret;
> - continue;
> + if (was_reset) {
> + /* Is this the hardware default? If so skip. */
> + ret = regcache_lookup_reg(map, regtmp);
> + if (ret >= 0 && val == map->reg_defaults[ret].def) {
> + ret = regcache_sync_block_raw_flush(map, &data,
> + base,
> + regtmp);
> + if (ret != 0)
> + return ret;
> + continue;
> + }
> }
>
[...]
On Fri, Apr 24, 2015 at 03:36:45PM -0700, Kevin Cernekee wrote:
> index 116655d92269..ece122a6fdeb 100644
> --- a/include/linux/regmap.h
> +++ b/include/linux/regmap.h
> @@ -438,7 +438,7 @@ bool regmap_can_raw_write(struct regmap *map);
>
> int regcache_sync(struct regmap *map);
> int regcache_sync_region(struct regmap *map, unsigned int min,
> - unsigned int max);
> + unsigned int max, bool was_reset);
This seems pretty ugly - both the fact that we're changing the signature
of the function and the naming of the argument feel inelegant. The
point isn't if the device has been reset, the point is if the device
currently has the default register values or not, and this means that
the user is responsible for tracking that state until the next time it
does the sync. That may be immediately like in your case but there's no
reason that has to be the case. The fact that we're passing in
something called "is_reset" which sounds like a state value for the
register map is a bit of a warning sign here.
What we should be doing here is providing a way for users to tell regmap
if they've reset the register map and actually we already have that
interface, it's just not got the best name - regcache_mark_dirty() is
effectively it since there's really not a lot of other reasons why a
driver would need to mark the cache as dirty. We're just not handling
it properly. What we should do instead is to keep the interface as it
is for now and make it behave in a more expected fashion so that if the
cache is explicitly marked dirty we assume that the hardware is in the
default state and otherwise we don't.
Ideally what we'd do is both improve the naming of mark_dirty() (though
that's API churn which is nasty) and arrange for rbtree to cache the
default values lazily, that way the only things in the cache will be
things that have been explicitly changed (we will still want default
checking but it makes life easier and means we don't end up having to do
a full writeout for cases where things have been put into cache mode
without a reset).
On Fri, Apr 24, 2015 at 03:36:47PM -0700, Kevin Cernekee wrote:
> + gpiod_set_value(priv->pdn_gpio, 0);
> + usleep_range(5000, 6000);
> +
> + regcache_cache_only(priv->regmap, false);
> + ret = regcache_sync_region(priv->regmap, 0,
> + priv->chip->regmap_config->max_register, false);
> + if (ret)
> + return ret;
This is also a problem with your first patch BTW - even if we are going
to add an argument we shouldn't be having drivers do _sync_region() to
sync the entire register map, that's just ugly.
On Sat, Apr 25, 2015 at 4:32 AM, Mark Brown <[email protected]> wrote:
> On Fri, Apr 24, 2015 at 03:36:45PM -0700, Kevin Cernekee wrote:
>
>> index 116655d92269..ece122a6fdeb 100644
>> --- a/include/linux/regmap.h
>> +++ b/include/linux/regmap.h
>> @@ -438,7 +438,7 @@ bool regmap_can_raw_write(struct regmap *map);
>>
>> int regcache_sync(struct regmap *map);
>> int regcache_sync_region(struct regmap *map, unsigned int min,
>> - unsigned int max);
>> + unsigned int max, bool was_reset);
>
> This seems pretty ugly - both the fact that we're changing the signature
> of the function and the naming of the argument feel inelegant. The
> point isn't if the device has been reset, the point is if the device
> currently has the default register values or not, and this means that
> the user is responsible for tracking that state until the next time it
> does the sync. That may be immediately like in your case but there's no
> reason that has to be the case. The fact that we're passing in
> something called "is_reset" which sounds like a state value for the
> register map is a bit of a warning sign here.
>
> What we should be doing here is providing a way for users to tell regmap
> if they've reset the register map and actually we already have that
> interface, it's just not got the best name - regcache_mark_dirty() is
> effectively it since there's really not a lot of other reasons why a
> driver would need to mark the cache as dirty. We're just not handling
> it properly. What we should do instead is to keep the interface as it
> is for now and make it behave in a more expected fashion so that if the
> cache is explicitly marked dirty we assume that the hardware is in the
> default state and otherwise we don't.
>
> Ideally what we'd do is both improve the naming of mark_dirty() (though
> that's API churn which is nasty) and arrange for rbtree to cache the
> default values lazily, that way the only things in the cache will be
> things that have been explicitly changed (we will still want default
> checking but it makes life easier and means we don't end up having to do
> a full writeout for cases where things have been put into cache mode
> without a reset).
Hi Mark,
I started prototyping this, but ran into a couple of issues:
1) How do we tell the difference between "regcache contains a
non-default value that correctly reflects the hardware register
contents" versus "regcache contains a non-default value that is
waiting to be written when we exit cache_only mode"?
2) Does that also mean that we should store default values in the
rbtree if they are part of a deferred cache_only write, but not store
them if the write went through to the hardware?
3) If we're caching the default values lazily, does that mean that
every regcache read would incur both an rbtree lookup and a bsearch of
the reg_defaults array?
4) If "the only things in the cache will be things that have been
explicitly changed," that could impact the semantics of
regcache_drop_region(). Which fortunately has no users.
Seems like it would be more straightforward just to add an
rbnode->dirty bitmask alongside rbnode->cache_present, rather than
trying to infer the hardware state from the presence/absence of the
cache entry. Knowing whether each individual register is out of sync
with the hardware lets us avoid unnecessary writes in both situations:
full reset, and temporary loss of register access.
What do you think?
On Tue, Apr 28, 2015 at 09:58:48PM -0700, Kevin Cernekee wrote:
> On Sat, Apr 25, 2015 at 4:32 AM, Mark Brown <[email protected]> wrote:
> > What we should be doing here is providing a way for users to tell regmap
> > if they've reset the register map and actually we already have that
> > interface, it's just not got the best name - regcache_mark_dirty() is
> > effectively it since there's really not a lot of other reasons why a
> > driver would need to mark the cache as dirty. We're just not handling
> 1) How do we tell the difference between "regcache contains a
> non-default value that correctly reflects the hardware register
> contents" versus "regcache contains a non-default value that is
> waiting to be written when we exit cache_only mode"?
Like I said above we can tell if the hardware was reset because
mark_dirty() is called.
> 2) Does that also mean that we should store default values in the
> rbtree if they are part of a deferred cache_only write, but not store
> them if the write went through to the hardware?
Well, remember that it's very expensive to remove a value from the cache
so actively trying to prune the cache would be bad.
> 3) If we're caching the default values lazily, does that mean that
> every regcache read would incur both an rbtree lookup and a bsearch of
> the reg_defaults array?
That'd happen on first read, yes.
> 4) If "the only things in the cache will be things that have been
> explicitly changed," that could impact the semantics of
> regcache_drop_region(). Which fortunately has no users.
Could you articulate what changes you believe would be seen?
> Seems like it would be more straightforward just to add an
> rbnode->dirty bitmask alongside rbnode->cache_present, rather than
> trying to infer the hardware state from the presence/absence of the
> cache entry. Knowing whether each individual register is out of sync
> with the hardware lets us avoid unnecessary writes in both situations:
> full reset, and temporary loss of register access.
I'm not suggesting that we do anything based on the presence of a cache
entry, I'm suggesting that we could avoid having to ever cache values
that never get referenced on a system (which can be a lot of them for
common use cases) saving us memory. Maintaining a dirty bitmask would
work too, but it does push the memory consumption up further which might
be a concern.
On Wed, Apr 29, 2015 at 3:40 AM, Mark Brown <[email protected]> wrote:
> On Tue, Apr 28, 2015 at 09:58:48PM -0700, Kevin Cernekee wrote:
>> On Sat, Apr 25, 2015 at 4:32 AM, Mark Brown <[email protected]> wrote:
>
>> > What we should be doing here is providing a way for users to tell regmap
>> > if they've reset the register map and actually we already have that
>> > interface, it's just not got the best name - regcache_mark_dirty() is
>> > effectively it since there's really not a lot of other reasons why a
>> > driver would need to mark the cache as dirty. We're just not handling
>
>> 1) How do we tell the difference between "regcache contains a
>> non-default value that correctly reflects the hardware register
>> contents" versus "regcache contains a non-default value that is
>> waiting to be written when we exit cache_only mode"?
>
> Like I said above we can tell if the hardware was reset because
> mark_dirty() is called.
That covers the public API, but I do not understand how you intended
for this data to be stored in the rbtree if the use of a dirty bitmask
is discouraged.
i.e. regcache_sync() finds a register value marked "present". How do
we know whether we need to write it back to the hardware? For the
special case of "cached non default register values immediately after
a HW reset" you can mostly figure this out, but if there was no HW
reset how do we know which entries changed while the HW was
inaccessible?
> I'm not suggesting that we do anything based on the presence of a cache
> entry, I'm suggesting that we could avoid having to ever cache values
> that never get referenced on a system (which can be a lot of them for
> common use cases) saving us memory.
This seems to be solving a different problem. It sounds like you are
more worried about regcache_sync() writing back lots of default values
for registers that were never touched, than performing unnecessary
writes to a few (actively used) registers that weren't changed while
we were in cache_only mode. Is that accurate?
FWIW, in the current iteration of the tas571x driver, there are few if
any registers that meet this criteria.
On Wed, Apr 29, 2015 at 07:13:27AM -0700, Kevin Cernekee wrote:
> On Wed, Apr 29, 2015 at 3:40 AM, Mark Brown <[email protected]> wrote:
> > Like I said above we can tell if the hardware was reset because
> > mark_dirty() is called.
> That covers the public API, but I do not understand how you intended
> for this data to be stored in the rbtree if the use of a dirty bitmask
> is discouraged.
We just need a single boolean?
> i.e. regcache_sync() finds a register value marked "present". How do
> we know whether we need to write it back to the hardware? For the
> special case of "cached non default register values immediately after
> a HW reset" you can mostly figure this out, but if there was no HW
> reset how do we know which entries changed while the HW was
> inaccessible?
In the first instance do we care?
> > I'm not suggesting that we do anything based on the presence of a cache
> > entry, I'm suggesting that we could avoid having to ever cache values
> > that never get referenced on a system (which can be a lot of them for
> > common use cases) saving us memory.
> This seems to be solving a different problem. It sounds like you are
> more worried about regcache_sync() writing back lots of default values
> for registers that were never touched, than performing unnecessary
> writes to a few (actively used) registers that weren't changed while
> we were in cache_only mode. Is that accurate?
No. This is nothing to do with sync, it's just something that might be
nice.
On Wed, Apr 29, 2015 at 9:46 AM, Mark Brown <[email protected]> wrote:
> On Wed, Apr 29, 2015 at 07:13:27AM -0700, Kevin Cernekee wrote:
>> On Wed, Apr 29, 2015 at 3:40 AM, Mark Brown <[email protected]> wrote:
>
>> > Like I said above we can tell if the hardware was reset because
>> > mark_dirty() is called.
>
>> That covers the public API, but I do not understand how you intended
>> for this data to be stored in the rbtree if the use of a dirty bitmask
>> is discouraged.
>
> We just need a single boolean?
Right, so if we add a per-regmap bool that tells us whether the device
has been reset, then in the case of "not reset" we will have to write
every regcache entry out to the device. Even the ones that weren't
touched while in cache_only mode. This makes the "not reset" case
much less efficient than the "reset" case.
Maybe that's good enough for most purposes. It's no worse than what
my original patch submission did, anyway.
BTW, any preferences on naming for the bool or for the renamed
mark_dirty function?
>> i.e. regcache_sync() finds a register value marked "present". How do
>> we know whether we need to write it back to the hardware? For the
>> special case of "cached non default register values immediately after
>> a HW reset" you can mostly figure this out, but if there was no HW
>> reset how do we know which entries changed while the HW was
>> inaccessible?
>
> In the first instance do we care?
I'm not sure I understand the question.
>> > I'm not suggesting that we do anything based on the presence of a cache
>> > entry, I'm suggesting that we could avoid having to ever cache values
>> > that never get referenced on a system (which can be a lot of them for
>> > common use cases) saving us memory.
>
>> This seems to be solving a different problem. It sounds like you are
>> more worried about regcache_sync() writing back lots of default values
>> for registers that were never touched, than performing unnecessary
>> writes to a few (actively used) registers that weren't changed while
>> we were in cache_only mode. Is that accurate?
>
> No. This is nothing to do with sync, it's just something that might be
> nice.
Thanks, that clarifies things. I was not aware that other drivers
even had an issue with excessively large regcache rbtrees, as my
reg_defaults list is short.
On Wed, Apr 29, 2015 at 10:02:03AM -0700, Kevin Cernekee wrote:
> On Wed, Apr 29, 2015 at 9:46 AM, Mark Brown <[email protected]> wrote:
> > We just need a single boolean?
> Right, so if we add a per-regmap bool that tells us whether the device
> has been reset, then in the case of "not reset" we will have to write
> every regcache entry out to the device. Even the ones that weren't
> touched while in cache_only mode. This makes the "not reset" case
> much less efficient than the "reset" case.
The immediate point is to provide a useful external interface, the
internal optimisation is much less urgent (and it's less clear to me how
often we're going to need to do that at all - generally cache only and
power loss go together.
> BTW, any preferences on naming for the bool or for the renamed
> mark_dirty function?
Not urgently. Note that I'm *not* suggesting an immediate rename,
that's definitely a separate change (which will be a lot more painful to
merge due to cross tree issues).
> >> i.e. regcache_sync() finds a register value marked "present". How do
> >> we know whether we need to write it back to the hardware? For the
> >> special case of "cached non default register values immediately after
> >> a HW reset" you can mostly figure this out, but if there was no HW
> >> reset how do we know which entries changed while the HW was
> >> inaccessible?
> > In the first instance do we care?
> I'm not sure I understand the question.
Do we actually care about getting a list of the changed registers that
much?