It all started in the pxa device-tree submission here :
https://lkml.org/lkml/2016/2/25/965
Last submission was a RFC in here:
http://www.gossamer-threads.com/lists/linux/kernel/2446863
It will be maintained in :
git fetch https://github.com/rjarzmik/linux.git work/ac97
The driving ideas are still the same, and I put them in [1] for memory. This is
the first post RFC submission. In order to make a full demonstration of the
framework, wm97xx was converted to an MFD, see [2] for the "why".
As this serie is a complete wm9713 change towards the new AC97 bus, here is the
patch organization :
- 1/9, 2/9, 3/9 and 5/9: the new AC97 bus, for Mark and Takashi to review
- 4/9: wm9713 conversion, this one has ugly #ifdefs, should be reviewed more
carefully
- 6/9: prerequisite for final 9/9, for Marek and Sebastian to approve
=> this one is independent and can flow through Sebastian's tree in this
next-cycle if accepted
- 7/9: prerequisite for final 9/9, for Mark/Dmitry
=> this one is also independent and can flow in this cycle if accepted
- 8/9: prerequisite for final 9/9, for Lee to review
=> this one has a dependency on 2/9, so it probably can't fit in this
next-cycle
- 9/9: this last one depends on all the patches before, so it can't fit in this
next-cycle
As a sum-up, I'd like to push for review for this -next cycle at least the
patches 1, 2, 3, 5, 6 and 9.
Happy review.
Robert Jarzmik (9):
ALSA: ac97: split out the generic ac97 registers
ALSA: ac97: add an ac97 bus
ASoC: add new ac97 bus support
ASoC: wm9713: add ac97 new bus support
ASoC: pxa: switch to new ac97 bus support
power_supply: wm97xx_battery: use power_supply_get_drvdata
Input: wm97xx: split out touchscreen registering
mfd: wm97xx-core: core support for wm97xx Codec
Input: wm97xx: add new AC97 bus support
drivers/input/touchscreen/Kconfig | 2 +-
drivers/input/touchscreen/wm97xx-core.c | 247 +++++++++++-----
drivers/mfd/Kconfig | 14 +
drivers/mfd/Makefile | 1 +
drivers/mfd/wm97xx-core.c | 282 ++++++++++++++++++
drivers/power/supply/wm97xx_battery.c | 25 +-
include/linux/mfd/wm97xx.h | 31 ++
include/sound/ac97/codec.h | 115 +++++++
include/sound/ac97/compat.h | 21 ++
include/sound/ac97/controller.h | 85 ++++++
include/sound/ac97/regs.h | 262 ++++++++++++++++
include/sound/ac97_codec.h | 239 +--------------
include/sound/pxa2xx-lib.h | 15 +-
sound/Kconfig | 2 +
sound/Makefile | 1 +
sound/ac97/Kconfig | 19 ++
sound/ac97/Makefile | 8 +
sound/ac97/ac97_core.h | 10 +
sound/ac97/bus.c | 510 ++++++++++++++++++++++++++++++++
sound/ac97/codec.c | 15 +
sound/ac97/snd_ac97_compat.c | 105 +++++++
sound/arm/Kconfig | 1 -
sound/arm/pxa2xx-ac97-lib.c | 39 ++-
sound/soc/Kconfig | 4 +
sound/soc/codecs/Kconfig | 3 +-
sound/soc/codecs/wm9713.c | 37 ++-
sound/soc/pxa/Kconfig | 5 +-
sound/soc/pxa/pxa2xx-ac97.c | 22 +-
28 files changed, 1738 insertions(+), 382 deletions(-)
create mode 100644 drivers/mfd/wm97xx-core.c
create mode 100644 include/linux/mfd/wm97xx.h
create mode 100644 include/sound/ac97/codec.h
create mode 100644 include/sound/ac97/compat.h
create mode 100644 include/sound/ac97/controller.h
create mode 100644 include/sound/ac97/regs.h
create mode 100644 sound/ac97/Kconfig
create mode 100644 sound/ac97/Makefile
create mode 100644 sound/ac97/ac97_core.h
create mode 100644 sound/ac97/bus.c
create mode 100644 sound/ac97/codec.c
create mode 100644 sound/ac97/snd_ac97_compat.c
--
2.1.4
[1] RFC PATCH reminder:
Amongst the driving ideas behind this RFC :
- use device/driver model for AC'97
device/driver binding is based on vendor_id{1,2}
- use auto-probe of the AC'97 bus to enumerate codecs and create devices
- be compatible with previous platform_data model for codecs
=> this enables a smooth transition, where a codec (here wm9713) can
be used either through platform_device or automatic discovery
- struct snd_ac97 is not used
This structure is really heavy, and doesn't represent an AC97 device, but
rather an AC'97 codec internals IIUC.
I think it's right place would be in include/sound/ac97/codec.h.
This is also a problem for regmap adherence, hence the
include/sound/ac97/compat.h.
- a new AC'97 bus Kconfig is created
This was done for a smooth transition ... let's see if it's a good idea.
- split the AC'97 into :
- the codec
- the digital controller
- the bus
Amongst the things that are not all touched yet :
- ac97_codec.c
The generic AC'97 codec in sound/pci/ac97/ac97_codec.c has really a lot
features, which are not at all PCI related. They could be transfered to
sound/ac97, without the pci specific field.
=> this deserves a deep ahead thinking, which I'll do if the current approach
is accepted by the communauty.
The most important patch is 2/7 "ALSA: ac97: add an ac97 bus". This is the main
evaluation point for the serie, the others are here so that I can test it all.
Well, this is a long term effort, which might need a complete rewrite according
to the comments it'll get. Let's expose it for comments and see how I can
progress with it.
[2] As a corollary, wm97xx is changed into a MFD device, which at least spawns:
- the touchscreen device
- the ac97 codec
- the problem to solve was stated in
http://www.gossamer-threads.com/lists/linux/kernel/2510113#2510113
As the power supply framework provides a way to store and retrieve
private supply data, use it.
In the process, change the platform data for wm97xx_battery from a
container of a single struct wm97xx_batt_pdata to the direct point to wm97xx_batt_pdata.
Signed-off-by: Robert Jarzmik <[email protected]>
---
drivers/input/touchscreen/wm97xx-core.c | 2 +-
drivers/power/supply/wm97xx_battery.c | 25 ++++++++++---------------
2 files changed, 11 insertions(+), 16 deletions(-)
diff --git a/drivers/input/touchscreen/wm97xx-core.c b/drivers/input/touchscreen/wm97xx-core.c
index 90d6be3c26cc..83cf11312fd9 100644
--- a/drivers/input/touchscreen/wm97xx-core.c
+++ b/drivers/input/touchscreen/wm97xx-core.c
@@ -682,7 +682,7 @@ static int wm97xx_probe(struct device *dev)
}
platform_set_drvdata(wm->battery_dev, wm);
wm->battery_dev->dev.parent = dev;
- wm->battery_dev->dev.platform_data = pdata;
+ wm->battery_dev->dev.platform_data = pdata->batt_pdata;
ret = platform_device_add(wm->battery_dev);
if (ret < 0)
goto batt_reg_err;
diff --git a/drivers/power/supply/wm97xx_battery.c b/drivers/power/supply/wm97xx_battery.c
index 6285626d142a..e3edb31ac880 100644
--- a/drivers/power/supply/wm97xx_battery.c
+++ b/drivers/power/supply/wm97xx_battery.c
@@ -30,8 +30,7 @@ static enum power_supply_property *prop;
static unsigned long wm97xx_read_bat(struct power_supply *bat_ps)
{
- struct wm97xx_pdata *wmdata = bat_ps->dev.parent->platform_data;
- struct wm97xx_batt_pdata *pdata = wmdata->batt_pdata;
+ struct wm97xx_batt_pdata *pdata = power_supply_get_drvdata(bat_ps);
return wm97xx_read_aux_adc(dev_get_drvdata(bat_ps->dev.parent),
pdata->batt_aux) * pdata->batt_mult /
@@ -40,8 +39,7 @@ static unsigned long wm97xx_read_bat(struct power_supply *bat_ps)
static unsigned long wm97xx_read_temp(struct power_supply *bat_ps)
{
- struct wm97xx_pdata *wmdata = bat_ps->dev.parent->platform_data;
- struct wm97xx_batt_pdata *pdata = wmdata->batt_pdata;
+ struct wm97xx_batt_pdata *pdata = power_supply_get_drvdata(bat_ps);
return wm97xx_read_aux_adc(dev_get_drvdata(bat_ps->dev.parent),
pdata->temp_aux) * pdata->temp_mult /
@@ -52,8 +50,7 @@ static int wm97xx_bat_get_property(struct power_supply *bat_ps,
enum power_supply_property psp,
union power_supply_propval *val)
{
- struct wm97xx_pdata *wmdata = bat_ps->dev.parent->platform_data;
- struct wm97xx_batt_pdata *pdata = wmdata->batt_pdata;
+ struct wm97xx_batt_pdata *pdata = power_supply_get_drvdata(bat_ps);
switch (psp) {
case POWER_SUPPLY_PROP_STATUS:
@@ -103,8 +100,7 @@ static void wm97xx_bat_external_power_changed(struct power_supply *bat_ps)
static void wm97xx_bat_update(struct power_supply *bat_ps)
{
int old_status = bat_status;
- struct wm97xx_pdata *wmdata = bat_ps->dev.parent->platform_data;
- struct wm97xx_batt_pdata *pdata = wmdata->batt_pdata;
+ struct wm97xx_batt_pdata *pdata = power_supply_get_drvdata(bat_ps);
mutex_lock(&work_lock);
@@ -166,15 +162,15 @@ static int wm97xx_bat_probe(struct platform_device *dev)
int ret = 0;
int props = 1; /* POWER_SUPPLY_PROP_PRESENT */
int i = 0;
- struct wm97xx_pdata *wmdata = dev->dev.platform_data;
- struct wm97xx_batt_pdata *pdata;
+ struct wm97xx_batt_pdata *pdata = dev->dev.platform_data;
+ struct power_supply_config cfg = {};
- if (!wmdata) {
+ if (!pdata) {
dev_err(&dev->dev, "No platform data supplied\n");
return -EINVAL;
}
- pdata = wmdata->batt_pdata;
+ cfg.drv_data = pdata;
if (dev->id != -1)
return -EINVAL;
@@ -243,7 +239,7 @@ static int wm97xx_bat_probe(struct platform_device *dev)
bat_psy_desc.properties = prop;
bat_psy_desc.num_properties = props;
- bat_psy = power_supply_register(&dev->dev, &bat_psy_desc, NULL);
+ bat_psy = power_supply_register(&dev->dev, &bat_psy_desc, &cfg);
if (!IS_ERR(bat_psy)) {
schedule_work(&bat_work);
} else {
@@ -266,8 +262,7 @@ static int wm97xx_bat_probe(struct platform_device *dev)
static int wm97xx_bat_remove(struct platform_device *dev)
{
- struct wm97xx_pdata *wmdata = dev->dev.platform_data;
- struct wm97xx_batt_pdata *pdata = wmdata->batt_pdata;
+ struct wm97xx_batt_pdata *pdata = dev->dev.platform_data;
if (pdata && gpio_is_valid(pdata->charge_gpio)) {
free_irq(gpio_to_irq(pdata->charge_gpio), dev);
--
2.1.4
Split out from the ac97_codec.h the ac97 generic registers, which can be
used by a codec, typically a generic ac97 codec, and by the ac97 bus, to
scan an ac97 AC-Link.
This split encompasses all the AC97 standard registers, but not the
codec specific ones.
In order to have a clean split between former ac97 bus implementation
and the new coming one in sound/ac97, it is safer to not include any
former ac97 includes, excepting in sound/ac97/compat.c.
Amongst the thing to isolate :
- don't have the struct snd_ac97 in sound/ac97/* (except compat.c) to not
be "fooled" by a definition which would come with ac97_codec.h by
"chance".
- don't have to have snd_a97_*() functions, as they rely on struct snd_ac97.
- don't want the struct snd_ac97_bus_ops, there is a new one
Signed-off-by: Robert Jarzmik <[email protected]>
---
Since v1: improve commit message to explain better the rationale
---
include/sound/ac97/regs.h | 262 +++++++++++++++++++++++++++++++++++++++++++++
include/sound/ac97_codec.h | 239 +----------------------------------------
2 files changed, 263 insertions(+), 238 deletions(-)
create mode 100644 include/sound/ac97/regs.h
diff --git a/include/sound/ac97/regs.h b/include/sound/ac97/regs.h
new file mode 100644
index 000000000000..4bb86d379bd5
--- /dev/null
+++ b/include/sound/ac97/regs.h
@@ -0,0 +1,262 @@
+/*
+ * Copyright (c) by Jaroslav Kysela <[email protected]>
+ * Universal interface for Audio Codec '97
+ *
+ * For more details look to AC '97 component specification revision 2.1
+ * by Intel Corporation (http://developer.intel.com).
+ *
+ *
+ * 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.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
+ *
+ */
+
+/*
+ * AC'97 codec registers
+ */
+
+#define AC97_RESET 0x00 /* Reset */
+#define AC97_MASTER 0x02 /* Master Volume */
+#define AC97_HEADPHONE 0x04 /* Headphone Volume (optional) */
+#define AC97_MASTER_MONO 0x06 /* Master Volume Mono (optional) */
+#define AC97_MASTER_TONE 0x08 /* Master Tone (Bass & Treble) (optional) */
+#define AC97_PC_BEEP 0x0a /* PC Beep Volume (optinal) */
+#define AC97_PHONE 0x0c /* Phone Volume (optional) */
+#define AC97_MIC 0x0e /* MIC Volume */
+#define AC97_LINE 0x10 /* Line In Volume */
+#define AC97_CD 0x12 /* CD Volume */
+#define AC97_VIDEO 0x14 /* Video Volume (optional) */
+#define AC97_AUX 0x16 /* AUX Volume (optional) */
+#define AC97_PCM 0x18 /* PCM Volume */
+#define AC97_REC_SEL 0x1a /* Record Select */
+#define AC97_REC_GAIN 0x1c /* Record Gain */
+#define AC97_REC_GAIN_MIC 0x1e /* Record Gain MIC (optional) */
+#define AC97_GENERAL_PURPOSE 0x20 /* General Purpose (optional) */
+#define AC97_3D_CONTROL 0x22 /* 3D Control (optional) */
+#define AC97_INT_PAGING 0x24 /* Audio Interrupt & Paging (AC'97 2.3) */
+#define AC97_POWERDOWN 0x26 /* Powerdown control / status */
+/* range 0x28-0x3a - AUDIO AC'97 2.0 extensions */
+#define AC97_EXTENDED_ID 0x28 /* Extended Audio ID */
+#define AC97_EXTENDED_STATUS 0x2a /* Extended Audio Status and Control */
+#define AC97_PCM_FRONT_DAC_RATE 0x2c /* PCM Front DAC Rate */
+#define AC97_PCM_SURR_DAC_RATE 0x2e /* PCM Surround DAC Rate */
+#define AC97_PCM_LFE_DAC_RATE 0x30 /* PCM LFE DAC Rate */
+#define AC97_PCM_LR_ADC_RATE 0x32 /* PCM LR ADC Rate */
+#define AC97_PCM_MIC_ADC_RATE 0x34 /* PCM MIC ADC Rate */
+#define AC97_CENTER_LFE_MASTER 0x36 /* Center + LFE Master Volume */
+#define AC97_SURROUND_MASTER 0x38 /* Surround (Rear) Master Volume */
+#define AC97_SPDIF 0x3a /* S/PDIF control */
+/* range 0x3c-0x58 - MODEM */
+#define AC97_EXTENDED_MID 0x3c /* Extended Modem ID */
+#define AC97_EXTENDED_MSTATUS 0x3e /* Extended Modem Status and Control */
+#define AC97_LINE1_RATE 0x40 /* Line1 DAC/ADC Rate */
+#define AC97_LINE2_RATE 0x42 /* Line2 DAC/ADC Rate */
+#define AC97_HANDSET_RATE 0x44 /* Handset DAC/ADC Rate */
+#define AC97_LINE1_LEVEL 0x46 /* Line1 DAC/ADC Level */
+#define AC97_LINE2_LEVEL 0x48 /* Line2 DAC/ADC Level */
+#define AC97_HANDSET_LEVEL 0x4a /* Handset DAC/ADC Level */
+#define AC97_GPIO_CFG 0x4c /* GPIO Configuration */
+#define AC97_GPIO_POLARITY 0x4e /* GPIO Pin Polarity/Type, 0=low, 1=high active */
+#define AC97_GPIO_STICKY 0x50 /* GPIO Pin Sticky, 0=not, 1=sticky */
+#define AC97_GPIO_WAKEUP 0x52 /* GPIO Pin Wakeup, 0=no int, 1=yes int */
+#define AC97_GPIO_STATUS 0x54 /* GPIO Pin Status, slot 12 */
+#define AC97_MISC_AFE 0x56 /* Miscellaneous Modem AFE Status and Control */
+/* range 0x5a-0x7b - Vendor Specific */
+#define AC97_VENDOR_ID1 0x7c /* Vendor ID1 */
+#define AC97_VENDOR_ID2 0x7e /* Vendor ID2 / revision */
+/* range 0x60-0x6f (page 1) - extended codec registers */
+#define AC97_CODEC_CLASS_REV 0x60 /* Codec Class/Revision */
+#define AC97_PCI_SVID 0x62 /* PCI Subsystem Vendor ID */
+#define AC97_PCI_SID 0x64 /* PCI Subsystem ID */
+#define AC97_FUNC_SELECT 0x66 /* Function Select */
+#define AC97_FUNC_INFO 0x68 /* Function Information */
+#define AC97_SENSE_INFO 0x6a /* Sense Details */
+
+/* volume controls */
+#define AC97_MUTE_MASK_MONO 0x8000
+#define AC97_MUTE_MASK_STEREO 0x8080
+
+/* slot allocation */
+#define AC97_SLOT_TAG 0
+#define AC97_SLOT_CMD_ADDR 1
+#define AC97_SLOT_CMD_DATA 2
+#define AC97_SLOT_PCM_LEFT 3
+#define AC97_SLOT_PCM_RIGHT 4
+#define AC97_SLOT_MODEM_LINE1 5
+#define AC97_SLOT_PCM_CENTER 6
+#define AC97_SLOT_MIC 6 /* input */
+#define AC97_SLOT_SPDIF_LEFT1 6
+#define AC97_SLOT_PCM_SLEFT 7 /* surround left */
+#define AC97_SLOT_PCM_LEFT_0 7 /* double rate operation */
+#define AC97_SLOT_SPDIF_LEFT 7
+#define AC97_SLOT_PCM_SRIGHT 8 /* surround right */
+#define AC97_SLOT_PCM_RIGHT_0 8 /* double rate operation */
+#define AC97_SLOT_SPDIF_RIGHT 8
+#define AC97_SLOT_LFE 9
+#define AC97_SLOT_SPDIF_RIGHT1 9
+#define AC97_SLOT_MODEM_LINE2 10
+#define AC97_SLOT_PCM_LEFT_1 10 /* double rate operation */
+#define AC97_SLOT_SPDIF_LEFT2 10
+#define AC97_SLOT_HANDSET 11 /* output */
+#define AC97_SLOT_PCM_RIGHT_1 11 /* double rate operation */
+#define AC97_SLOT_SPDIF_RIGHT2 11
+#define AC97_SLOT_MODEM_GPIO 12 /* modem GPIO */
+#define AC97_SLOT_PCM_CENTER_1 12 /* double rate operation */
+
+/* basic capabilities (reset register) */
+#define AC97_BC_DEDICATED_MIC 0x0001 /* Dedicated Mic PCM In Channel */
+#define AC97_BC_RESERVED1 0x0002 /* Reserved (was Modem Line Codec support) */
+#define AC97_BC_BASS_TREBLE 0x0004 /* Bass & Treble Control */
+#define AC97_BC_SIM_STEREO 0x0008 /* Simulated stereo */
+#define AC97_BC_HEADPHONE 0x0010 /* Headphone Out Support */
+#define AC97_BC_LOUDNESS 0x0020 /* Loudness (bass boost) Support */
+#define AC97_BC_16BIT_DAC 0x0000 /* 16-bit DAC resolution */
+#define AC97_BC_18BIT_DAC 0x0040 /* 18-bit DAC resolution */
+#define AC97_BC_20BIT_DAC 0x0080 /* 20-bit DAC resolution */
+#define AC97_BC_DAC_MASK 0x00c0
+#define AC97_BC_16BIT_ADC 0x0000 /* 16-bit ADC resolution */
+#define AC97_BC_18BIT_ADC 0x0100 /* 18-bit ADC resolution */
+#define AC97_BC_20BIT_ADC 0x0200 /* 20-bit ADC resolution */
+#define AC97_BC_ADC_MASK 0x0300
+#define AC97_BC_3D_TECH_ID_MASK 0x7c00 /* Per-vendor ID of 3D enhancement */
+
+/* general purpose */
+#define AC97_GP_DRSS_MASK 0x0c00 /* double rate slot select */
+#define AC97_GP_DRSS_1011 0x0000 /* LR(C) 10+11(+12) */
+#define AC97_GP_DRSS_78 0x0400 /* LR 7+8 */
+
+/* powerdown bits */
+#define AC97_PD_ADC_STATUS 0x0001 /* ADC status (RO) */
+#define AC97_PD_DAC_STATUS 0x0002 /* DAC status (RO) */
+#define AC97_PD_MIXER_STATUS 0x0004 /* Analog mixer status (RO) */
+#define AC97_PD_VREF_STATUS 0x0008 /* Vref status (RO) */
+#define AC97_PD_PR0 0x0100 /* Power down PCM ADCs and input MUX */
+#define AC97_PD_PR1 0x0200 /* Power down PCM front DAC */
+#define AC97_PD_PR2 0x0400 /* Power down Mixer (Vref still on) */
+#define AC97_PD_PR3 0x0800 /* Power down Mixer (Vref off) */
+#define AC97_PD_PR4 0x1000 /* Power down AC-Link */
+#define AC97_PD_PR5 0x2000 /* Disable internal clock usage */
+#define AC97_PD_PR6 0x4000 /* Headphone amplifier */
+#define AC97_PD_EAPD 0x8000 /* External Amplifer Power Down (EAPD) */
+
+/* extended audio ID bit defines */
+#define AC97_EI_VRA 0x0001 /* Variable bit rate supported */
+#define AC97_EI_DRA 0x0002 /* Double rate supported */
+#define AC97_EI_SPDIF 0x0004 /* S/PDIF out supported */
+#define AC97_EI_VRM 0x0008 /* Variable bit rate supported for MIC */
+#define AC97_EI_DACS_SLOT_MASK 0x0030 /* DACs slot assignment */
+#define AC97_EI_DACS_SLOT_SHIFT 4
+#define AC97_EI_CDAC 0x0040 /* PCM Center DAC available */
+#define AC97_EI_SDAC 0x0080 /* PCM Surround DACs available */
+#define AC97_EI_LDAC 0x0100 /* PCM LFE DAC available */
+#define AC97_EI_AMAP 0x0200 /* indicates optional slot/DAC mapping based on codec ID */
+#define AC97_EI_REV_MASK 0x0c00 /* AC'97 revision mask */
+#define AC97_EI_REV_22 0x0400 /* AC'97 revision 2.2 */
+#define AC97_EI_REV_23 0x0800 /* AC'97 revision 2.3 */
+#define AC97_EI_REV_SHIFT 10
+#define AC97_EI_ADDR_MASK 0xc000 /* physical codec ID (address) */
+#define AC97_EI_ADDR_SHIFT 14
+
+/* extended audio status and control bit defines */
+#define AC97_EA_VRA 0x0001 /* Variable bit rate enable bit */
+#define AC97_EA_DRA 0x0002 /* Double-rate audio enable bit */
+#define AC97_EA_SPDIF 0x0004 /* S/PDIF out enable bit */
+#define AC97_EA_VRM 0x0008 /* Variable bit rate for MIC enable bit */
+#define AC97_EA_SPSA_SLOT_MASK 0x0030 /* Mask for slot assignment bits */
+#define AC97_EA_SPSA_SLOT_SHIFT 4
+#define AC97_EA_SPSA_3_4 0x0000 /* Slot assigned to 3 & 4 */
+#define AC97_EA_SPSA_7_8 0x0010 /* Slot assigned to 7 & 8 */
+#define AC97_EA_SPSA_6_9 0x0020 /* Slot assigned to 6 & 9 */
+#define AC97_EA_SPSA_10_11 0x0030 /* Slot assigned to 10 & 11 */
+#define AC97_EA_CDAC 0x0040 /* PCM Center DAC is ready (Read only) */
+#define AC97_EA_SDAC 0x0080 /* PCM Surround DACs are ready (Read only) */
+#define AC97_EA_LDAC 0x0100 /* PCM LFE DAC is ready (Read only) */
+#define AC97_EA_MDAC 0x0200 /* MIC ADC is ready (Read only) */
+#define AC97_EA_SPCV 0x0400 /* S/PDIF configuration valid (Read only) */
+#define AC97_EA_PRI 0x0800 /* Turns the PCM Center DAC off */
+#define AC97_EA_PRJ 0x1000 /* Turns the PCM Surround DACs off */
+#define AC97_EA_PRK 0x2000 /* Turns the PCM LFE DAC off */
+#define AC97_EA_PRL 0x4000 /* Turns the MIC ADC off */
+
+/* S/PDIF control bit defines */
+#define AC97_SC_PRO 0x0001 /* Professional status */
+#define AC97_SC_NAUDIO 0x0002 /* Non audio stream */
+#define AC97_SC_COPY 0x0004 /* Copyright status */
+#define AC97_SC_PRE 0x0008 /* Preemphasis status */
+#define AC97_SC_CC_MASK 0x07f0 /* Category Code mask */
+#define AC97_SC_CC_SHIFT 4
+#define AC97_SC_L 0x0800 /* Generation Level status */
+#define AC97_SC_SPSR_MASK 0x3000 /* S/PDIF Sample Rate bits */
+#define AC97_SC_SPSR_SHIFT 12
+#define AC97_SC_SPSR_44K 0x0000 /* Use 44.1kHz Sample rate */
+#define AC97_SC_SPSR_48K 0x2000 /* Use 48kHz Sample rate */
+#define AC97_SC_SPSR_32K 0x3000 /* Use 32kHz Sample rate */
+#define AC97_SC_DRS 0x4000 /* Double Rate S/PDIF */
+#define AC97_SC_V 0x8000 /* Validity status */
+
+/* Interrupt and Paging bit defines (AC'97 2.3) */
+#define AC97_PAGE_MASK 0x000f /* Page Selector */
+#define AC97_PAGE_VENDOR 0 /* Vendor-specific registers */
+#define AC97_PAGE_1 1 /* Extended Codec Registers page 1 */
+#define AC97_INT_ENABLE 0x0800 /* Interrupt Enable */
+#define AC97_INT_SENSE 0x1000 /* Sense Cycle */
+#define AC97_INT_CAUSE_SENSE 0x2000 /* Sense Cycle Completed (RO) */
+#define AC97_INT_CAUSE_GPIO 0x4000 /* GPIO bits changed (RO) */
+#define AC97_INT_STATUS 0x8000 /* Interrupt Status */
+
+/* extended modem ID bit defines */
+#define AC97_MEI_LINE1 0x0001 /* Line1 present */
+#define AC97_MEI_LINE2 0x0002 /* Line2 present */
+#define AC97_MEI_HANDSET 0x0004 /* Handset present */
+#define AC97_MEI_CID1 0x0008 /* caller ID decode for Line1 is supported */
+#define AC97_MEI_CID2 0x0010 /* caller ID decode for Line2 is supported */
+#define AC97_MEI_ADDR_MASK 0xc000 /* physical codec ID (address) */
+#define AC97_MEI_ADDR_SHIFT 14
+
+/* extended modem status and control bit defines */
+#define AC97_MEA_GPIO 0x0001 /* GPIO is ready (ro) */
+#define AC97_MEA_MREF 0x0002 /* Vref is up to nominal level (ro) */
+#define AC97_MEA_ADC1 0x0004 /* ADC1 operational (ro) */
+#define AC97_MEA_DAC1 0x0008 /* DAC1 operational (ro) */
+#define AC97_MEA_ADC2 0x0010 /* ADC2 operational (ro) */
+#define AC97_MEA_DAC2 0x0020 /* DAC2 operational (ro) */
+#define AC97_MEA_HADC 0x0040 /* HADC operational (ro) */
+#define AC97_MEA_HDAC 0x0080 /* HDAC operational (ro) */
+#define AC97_MEA_PRA 0x0100 /* GPIO power down (high) */
+#define AC97_MEA_PRB 0x0200 /* reserved */
+#define AC97_MEA_PRC 0x0400 /* ADC1 power down (high) */
+#define AC97_MEA_PRD 0x0800 /* DAC1 power down (high) */
+#define AC97_MEA_PRE 0x1000 /* ADC2 power down (high) */
+#define AC97_MEA_PRF 0x2000 /* DAC2 power down (high) */
+#define AC97_MEA_PRG 0x4000 /* HADC power down (high) */
+#define AC97_MEA_PRH 0x8000 /* HDAC power down (high) */
+
+/* modem gpio status defines */
+#define AC97_GPIO_LINE1_OH 0x0001 /* Off Hook Line1 */
+#define AC97_GPIO_LINE1_RI 0x0002 /* Ring Detect Line1 */
+#define AC97_GPIO_LINE1_CID 0x0004 /* Caller ID path enable Line1 */
+#define AC97_GPIO_LINE1_LCS 0x0008 /* Loop Current Sense Line1 */
+#define AC97_GPIO_LINE1_PULSE 0x0010 /* Opt./ Pulse Dial Line1 (out) */
+#define AC97_GPIO_LINE1_HL1R 0x0020 /* Opt./ Handset to Line1 relay control (out) */
+#define AC97_GPIO_LINE1_HOHD 0x0040 /* Opt./ Handset off hook detect Line1 (in) */
+#define AC97_GPIO_LINE12_AC 0x0080 /* Opt./ Int.bit 1 / Line1/2 AC (out) */
+#define AC97_GPIO_LINE12_DC 0x0100 /* Opt./ Int.bit 2 / Line1/2 DC (out) */
+#define AC97_GPIO_LINE12_RS 0x0200 /* Opt./ Int.bit 3 / Line1/2 RS (out) */
+#define AC97_GPIO_LINE2_OH 0x0400 /* Off Hook Line2 */
+#define AC97_GPIO_LINE2_RI 0x0800 /* Ring Detect Line2 */
+#define AC97_GPIO_LINE2_CID 0x1000 /* Caller ID path enable Line2 */
+#define AC97_GPIO_LINE2_LCS 0x2000 /* Loop Current Sense Line2 */
+#define AC97_GPIO_LINE2_PULSE 0x4000 /* Opt./ Pulse Dial Line2 (out) */
+#define AC97_GPIO_LINE2_HL1R 0x8000 /* Opt./ Handset to Line2 relay control (out) */
+
diff --git a/include/sound/ac97_codec.h b/include/sound/ac97_codec.h
index 15aa5f07c955..89d311a503d3 100644
--- a/include/sound/ac97_codec.h
+++ b/include/sound/ac97_codec.h
@@ -28,6 +28,7 @@
#include <linux/bitops.h>
#include <linux/device.h>
#include <linux/workqueue.h>
+#include <sound/ac97/regs.h>
#include <sound/pcm.h>
#include <sound/control.h>
#include <sound/info.h>
@@ -35,244 +36,6 @@
/* maximum number of devices on the AC97 bus */
#define AC97_BUS_MAX_DEVICES 4
-/*
- * AC'97 codec registers
- */
-
-#define AC97_RESET 0x00 /* Reset */
-#define AC97_MASTER 0x02 /* Master Volume */
-#define AC97_HEADPHONE 0x04 /* Headphone Volume (optional) */
-#define AC97_MASTER_MONO 0x06 /* Master Volume Mono (optional) */
-#define AC97_MASTER_TONE 0x08 /* Master Tone (Bass & Treble) (optional) */
-#define AC97_PC_BEEP 0x0a /* PC Beep Volume (optinal) */
-#define AC97_PHONE 0x0c /* Phone Volume (optional) */
-#define AC97_MIC 0x0e /* MIC Volume */
-#define AC97_LINE 0x10 /* Line In Volume */
-#define AC97_CD 0x12 /* CD Volume */
-#define AC97_VIDEO 0x14 /* Video Volume (optional) */
-#define AC97_AUX 0x16 /* AUX Volume (optional) */
-#define AC97_PCM 0x18 /* PCM Volume */
-#define AC97_REC_SEL 0x1a /* Record Select */
-#define AC97_REC_GAIN 0x1c /* Record Gain */
-#define AC97_REC_GAIN_MIC 0x1e /* Record Gain MIC (optional) */
-#define AC97_GENERAL_PURPOSE 0x20 /* General Purpose (optional) */
-#define AC97_3D_CONTROL 0x22 /* 3D Control (optional) */
-#define AC97_INT_PAGING 0x24 /* Audio Interrupt & Paging (AC'97 2.3) */
-#define AC97_POWERDOWN 0x26 /* Powerdown control / status */
-/* range 0x28-0x3a - AUDIO AC'97 2.0 extensions */
-#define AC97_EXTENDED_ID 0x28 /* Extended Audio ID */
-#define AC97_EXTENDED_STATUS 0x2a /* Extended Audio Status and Control */
-#define AC97_PCM_FRONT_DAC_RATE 0x2c /* PCM Front DAC Rate */
-#define AC97_PCM_SURR_DAC_RATE 0x2e /* PCM Surround DAC Rate */
-#define AC97_PCM_LFE_DAC_RATE 0x30 /* PCM LFE DAC Rate */
-#define AC97_PCM_LR_ADC_RATE 0x32 /* PCM LR ADC Rate */
-#define AC97_PCM_MIC_ADC_RATE 0x34 /* PCM MIC ADC Rate */
-#define AC97_CENTER_LFE_MASTER 0x36 /* Center + LFE Master Volume */
-#define AC97_SURROUND_MASTER 0x38 /* Surround (Rear) Master Volume */
-#define AC97_SPDIF 0x3a /* S/PDIF control */
-/* range 0x3c-0x58 - MODEM */
-#define AC97_EXTENDED_MID 0x3c /* Extended Modem ID */
-#define AC97_EXTENDED_MSTATUS 0x3e /* Extended Modem Status and Control */
-#define AC97_LINE1_RATE 0x40 /* Line1 DAC/ADC Rate */
-#define AC97_LINE2_RATE 0x42 /* Line2 DAC/ADC Rate */
-#define AC97_HANDSET_RATE 0x44 /* Handset DAC/ADC Rate */
-#define AC97_LINE1_LEVEL 0x46 /* Line1 DAC/ADC Level */
-#define AC97_LINE2_LEVEL 0x48 /* Line2 DAC/ADC Level */
-#define AC97_HANDSET_LEVEL 0x4a /* Handset DAC/ADC Level */
-#define AC97_GPIO_CFG 0x4c /* GPIO Configuration */
-#define AC97_GPIO_POLARITY 0x4e /* GPIO Pin Polarity/Type, 0=low, 1=high active */
-#define AC97_GPIO_STICKY 0x50 /* GPIO Pin Sticky, 0=not, 1=sticky */
-#define AC97_GPIO_WAKEUP 0x52 /* GPIO Pin Wakeup, 0=no int, 1=yes int */
-#define AC97_GPIO_STATUS 0x54 /* GPIO Pin Status, slot 12 */
-#define AC97_MISC_AFE 0x56 /* Miscellaneous Modem AFE Status and Control */
-/* range 0x5a-0x7b - Vendor Specific */
-#define AC97_VENDOR_ID1 0x7c /* Vendor ID1 */
-#define AC97_VENDOR_ID2 0x7e /* Vendor ID2 / revision */
-/* range 0x60-0x6f (page 1) - extended codec registers */
-#define AC97_CODEC_CLASS_REV 0x60 /* Codec Class/Revision */
-#define AC97_PCI_SVID 0x62 /* PCI Subsystem Vendor ID */
-#define AC97_PCI_SID 0x64 /* PCI Subsystem ID */
-#define AC97_FUNC_SELECT 0x66 /* Function Select */
-#define AC97_FUNC_INFO 0x68 /* Function Information */
-#define AC97_SENSE_INFO 0x6a /* Sense Details */
-
-/* volume controls */
-#define AC97_MUTE_MASK_MONO 0x8000
-#define AC97_MUTE_MASK_STEREO 0x8080
-
-/* slot allocation */
-#define AC97_SLOT_TAG 0
-#define AC97_SLOT_CMD_ADDR 1
-#define AC97_SLOT_CMD_DATA 2
-#define AC97_SLOT_PCM_LEFT 3
-#define AC97_SLOT_PCM_RIGHT 4
-#define AC97_SLOT_MODEM_LINE1 5
-#define AC97_SLOT_PCM_CENTER 6
-#define AC97_SLOT_MIC 6 /* input */
-#define AC97_SLOT_SPDIF_LEFT1 6
-#define AC97_SLOT_PCM_SLEFT 7 /* surround left */
-#define AC97_SLOT_PCM_LEFT_0 7 /* double rate operation */
-#define AC97_SLOT_SPDIF_LEFT 7
-#define AC97_SLOT_PCM_SRIGHT 8 /* surround right */
-#define AC97_SLOT_PCM_RIGHT_0 8 /* double rate operation */
-#define AC97_SLOT_SPDIF_RIGHT 8
-#define AC97_SLOT_LFE 9
-#define AC97_SLOT_SPDIF_RIGHT1 9
-#define AC97_SLOT_MODEM_LINE2 10
-#define AC97_SLOT_PCM_LEFT_1 10 /* double rate operation */
-#define AC97_SLOT_SPDIF_LEFT2 10
-#define AC97_SLOT_HANDSET 11 /* output */
-#define AC97_SLOT_PCM_RIGHT_1 11 /* double rate operation */
-#define AC97_SLOT_SPDIF_RIGHT2 11
-#define AC97_SLOT_MODEM_GPIO 12 /* modem GPIO */
-#define AC97_SLOT_PCM_CENTER_1 12 /* double rate operation */
-
-/* basic capabilities (reset register) */
-#define AC97_BC_DEDICATED_MIC 0x0001 /* Dedicated Mic PCM In Channel */
-#define AC97_BC_RESERVED1 0x0002 /* Reserved (was Modem Line Codec support) */
-#define AC97_BC_BASS_TREBLE 0x0004 /* Bass & Treble Control */
-#define AC97_BC_SIM_STEREO 0x0008 /* Simulated stereo */
-#define AC97_BC_HEADPHONE 0x0010 /* Headphone Out Support */
-#define AC97_BC_LOUDNESS 0x0020 /* Loudness (bass boost) Support */
-#define AC97_BC_16BIT_DAC 0x0000 /* 16-bit DAC resolution */
-#define AC97_BC_18BIT_DAC 0x0040 /* 18-bit DAC resolution */
-#define AC97_BC_20BIT_DAC 0x0080 /* 20-bit DAC resolution */
-#define AC97_BC_DAC_MASK 0x00c0
-#define AC97_BC_16BIT_ADC 0x0000 /* 16-bit ADC resolution */
-#define AC97_BC_18BIT_ADC 0x0100 /* 18-bit ADC resolution */
-#define AC97_BC_20BIT_ADC 0x0200 /* 20-bit ADC resolution */
-#define AC97_BC_ADC_MASK 0x0300
-#define AC97_BC_3D_TECH_ID_MASK 0x7c00 /* Per-vendor ID of 3D enhancement */
-
-/* general purpose */
-#define AC97_GP_DRSS_MASK 0x0c00 /* double rate slot select */
-#define AC97_GP_DRSS_1011 0x0000 /* LR(C) 10+11(+12) */
-#define AC97_GP_DRSS_78 0x0400 /* LR 7+8 */
-
-/* powerdown bits */
-#define AC97_PD_ADC_STATUS 0x0001 /* ADC status (RO) */
-#define AC97_PD_DAC_STATUS 0x0002 /* DAC status (RO) */
-#define AC97_PD_MIXER_STATUS 0x0004 /* Analog mixer status (RO) */
-#define AC97_PD_VREF_STATUS 0x0008 /* Vref status (RO) */
-#define AC97_PD_PR0 0x0100 /* Power down PCM ADCs and input MUX */
-#define AC97_PD_PR1 0x0200 /* Power down PCM front DAC */
-#define AC97_PD_PR2 0x0400 /* Power down Mixer (Vref still on) */
-#define AC97_PD_PR3 0x0800 /* Power down Mixer (Vref off) */
-#define AC97_PD_PR4 0x1000 /* Power down AC-Link */
-#define AC97_PD_PR5 0x2000 /* Disable internal clock usage */
-#define AC97_PD_PR6 0x4000 /* Headphone amplifier */
-#define AC97_PD_EAPD 0x8000 /* External Amplifer Power Down (EAPD) */
-
-/* extended audio ID bit defines */
-#define AC97_EI_VRA 0x0001 /* Variable bit rate supported */
-#define AC97_EI_DRA 0x0002 /* Double rate supported */
-#define AC97_EI_SPDIF 0x0004 /* S/PDIF out supported */
-#define AC97_EI_VRM 0x0008 /* Variable bit rate supported for MIC */
-#define AC97_EI_DACS_SLOT_MASK 0x0030 /* DACs slot assignment */
-#define AC97_EI_DACS_SLOT_SHIFT 4
-#define AC97_EI_CDAC 0x0040 /* PCM Center DAC available */
-#define AC97_EI_SDAC 0x0080 /* PCM Surround DACs available */
-#define AC97_EI_LDAC 0x0100 /* PCM LFE DAC available */
-#define AC97_EI_AMAP 0x0200 /* indicates optional slot/DAC mapping based on codec ID */
-#define AC97_EI_REV_MASK 0x0c00 /* AC'97 revision mask */
-#define AC97_EI_REV_22 0x0400 /* AC'97 revision 2.2 */
-#define AC97_EI_REV_23 0x0800 /* AC'97 revision 2.3 */
-#define AC97_EI_REV_SHIFT 10
-#define AC97_EI_ADDR_MASK 0xc000 /* physical codec ID (address) */
-#define AC97_EI_ADDR_SHIFT 14
-
-/* extended audio status and control bit defines */
-#define AC97_EA_VRA 0x0001 /* Variable bit rate enable bit */
-#define AC97_EA_DRA 0x0002 /* Double-rate audio enable bit */
-#define AC97_EA_SPDIF 0x0004 /* S/PDIF out enable bit */
-#define AC97_EA_VRM 0x0008 /* Variable bit rate for MIC enable bit */
-#define AC97_EA_SPSA_SLOT_MASK 0x0030 /* Mask for slot assignment bits */
-#define AC97_EA_SPSA_SLOT_SHIFT 4
-#define AC97_EA_SPSA_3_4 0x0000 /* Slot assigned to 3 & 4 */
-#define AC97_EA_SPSA_7_8 0x0010 /* Slot assigned to 7 & 8 */
-#define AC97_EA_SPSA_6_9 0x0020 /* Slot assigned to 6 & 9 */
-#define AC97_EA_SPSA_10_11 0x0030 /* Slot assigned to 10 & 11 */
-#define AC97_EA_CDAC 0x0040 /* PCM Center DAC is ready (Read only) */
-#define AC97_EA_SDAC 0x0080 /* PCM Surround DACs are ready (Read only) */
-#define AC97_EA_LDAC 0x0100 /* PCM LFE DAC is ready (Read only) */
-#define AC97_EA_MDAC 0x0200 /* MIC ADC is ready (Read only) */
-#define AC97_EA_SPCV 0x0400 /* S/PDIF configuration valid (Read only) */
-#define AC97_EA_PRI 0x0800 /* Turns the PCM Center DAC off */
-#define AC97_EA_PRJ 0x1000 /* Turns the PCM Surround DACs off */
-#define AC97_EA_PRK 0x2000 /* Turns the PCM LFE DAC off */
-#define AC97_EA_PRL 0x4000 /* Turns the MIC ADC off */
-
-/* S/PDIF control bit defines */
-#define AC97_SC_PRO 0x0001 /* Professional status */
-#define AC97_SC_NAUDIO 0x0002 /* Non audio stream */
-#define AC97_SC_COPY 0x0004 /* Copyright status */
-#define AC97_SC_PRE 0x0008 /* Preemphasis status */
-#define AC97_SC_CC_MASK 0x07f0 /* Category Code mask */
-#define AC97_SC_CC_SHIFT 4
-#define AC97_SC_L 0x0800 /* Generation Level status */
-#define AC97_SC_SPSR_MASK 0x3000 /* S/PDIF Sample Rate bits */
-#define AC97_SC_SPSR_SHIFT 12
-#define AC97_SC_SPSR_44K 0x0000 /* Use 44.1kHz Sample rate */
-#define AC97_SC_SPSR_48K 0x2000 /* Use 48kHz Sample rate */
-#define AC97_SC_SPSR_32K 0x3000 /* Use 32kHz Sample rate */
-#define AC97_SC_DRS 0x4000 /* Double Rate S/PDIF */
-#define AC97_SC_V 0x8000 /* Validity status */
-
-/* Interrupt and Paging bit defines (AC'97 2.3) */
-#define AC97_PAGE_MASK 0x000f /* Page Selector */
-#define AC97_PAGE_VENDOR 0 /* Vendor-specific registers */
-#define AC97_PAGE_1 1 /* Extended Codec Registers page 1 */
-#define AC97_INT_ENABLE 0x0800 /* Interrupt Enable */
-#define AC97_INT_SENSE 0x1000 /* Sense Cycle */
-#define AC97_INT_CAUSE_SENSE 0x2000 /* Sense Cycle Completed (RO) */
-#define AC97_INT_CAUSE_GPIO 0x4000 /* GPIO bits changed (RO) */
-#define AC97_INT_STATUS 0x8000 /* Interrupt Status */
-
-/* extended modem ID bit defines */
-#define AC97_MEI_LINE1 0x0001 /* Line1 present */
-#define AC97_MEI_LINE2 0x0002 /* Line2 present */
-#define AC97_MEI_HANDSET 0x0004 /* Handset present */
-#define AC97_MEI_CID1 0x0008 /* caller ID decode for Line1 is supported */
-#define AC97_MEI_CID2 0x0010 /* caller ID decode for Line2 is supported */
-#define AC97_MEI_ADDR_MASK 0xc000 /* physical codec ID (address) */
-#define AC97_MEI_ADDR_SHIFT 14
-
-/* extended modem status and control bit defines */
-#define AC97_MEA_GPIO 0x0001 /* GPIO is ready (ro) */
-#define AC97_MEA_MREF 0x0002 /* Vref is up to nominal level (ro) */
-#define AC97_MEA_ADC1 0x0004 /* ADC1 operational (ro) */
-#define AC97_MEA_DAC1 0x0008 /* DAC1 operational (ro) */
-#define AC97_MEA_ADC2 0x0010 /* ADC2 operational (ro) */
-#define AC97_MEA_DAC2 0x0020 /* DAC2 operational (ro) */
-#define AC97_MEA_HADC 0x0040 /* HADC operational (ro) */
-#define AC97_MEA_HDAC 0x0080 /* HDAC operational (ro) */
-#define AC97_MEA_PRA 0x0100 /* GPIO power down (high) */
-#define AC97_MEA_PRB 0x0200 /* reserved */
-#define AC97_MEA_PRC 0x0400 /* ADC1 power down (high) */
-#define AC97_MEA_PRD 0x0800 /* DAC1 power down (high) */
-#define AC97_MEA_PRE 0x1000 /* ADC2 power down (high) */
-#define AC97_MEA_PRF 0x2000 /* DAC2 power down (high) */
-#define AC97_MEA_PRG 0x4000 /* HADC power down (high) */
-#define AC97_MEA_PRH 0x8000 /* HDAC power down (high) */
-
-/* modem gpio status defines */
-#define AC97_GPIO_LINE1_OH 0x0001 /* Off Hook Line1 */
-#define AC97_GPIO_LINE1_RI 0x0002 /* Ring Detect Line1 */
-#define AC97_GPIO_LINE1_CID 0x0004 /* Caller ID path enable Line1 */
-#define AC97_GPIO_LINE1_LCS 0x0008 /* Loop Current Sense Line1 */
-#define AC97_GPIO_LINE1_PULSE 0x0010 /* Opt./ Pulse Dial Line1 (out) */
-#define AC97_GPIO_LINE1_HL1R 0x0020 /* Opt./ Handset to Line1 relay control (out) */
-#define AC97_GPIO_LINE1_HOHD 0x0040 /* Opt./ Handset off hook detect Line1 (in) */
-#define AC97_GPIO_LINE12_AC 0x0080 /* Opt./ Int.bit 1 / Line1/2 AC (out) */
-#define AC97_GPIO_LINE12_DC 0x0100 /* Opt./ Int.bit 2 / Line1/2 DC (out) */
-#define AC97_GPIO_LINE12_RS 0x0200 /* Opt./ Int.bit 3 / Line1/2 RS (out) */
-#define AC97_GPIO_LINE2_OH 0x0400 /* Off Hook Line2 */
-#define AC97_GPIO_LINE2_RI 0x0800 /* Ring Detect Line2 */
-#define AC97_GPIO_LINE2_CID 0x1000 /* Caller ID path enable Line2 */
-#define AC97_GPIO_LINE2_LCS 0x2000 /* Loop Current Sense Line2 */
-#define AC97_GPIO_LINE2_PULSE 0x4000 /* Opt./ Pulse Dial Line2 (out) */
-#define AC97_GPIO_LINE2_HL1R 0x8000 /* Opt./ Handset to Line2 relay control (out) */
-
/* specific - SigmaTel */
#define AC97_SIGMATEL_OUTSEL 0x64 /* Output Select, STAC9758 */
#define AC97_SIGMATEL_INSEL 0x66 /* Input Select, STAC9758 */
--
2.1.4
AC97 is a bus for sound usage. It enables for a AC97 AC-Link to link one
controller to 0 to 4 AC97 codecs.
The goal of this new implementation is to implement a device/driver
model for AC97, with an automatic scan of the bus and automatic
discovery of AC97 codec devices.
Signed-off-by: Robert Jarzmik <[email protected]>
---
Since v1:
- Takashi's review
- changed the codec.h guard ... a better name could be found ...
- added the AC97_* macros missing parenthesis
- constantified the id_table in the codec driver structure
- changed the 4 codecs linked list into an array
- enabled the ac97 bus to be a module
- added a slots_available to snd_ac97_controller_register() to have a
way to prevent scanning and probing of unconnected codecs
- removed useless ac97 bus index
- all exported functions begin with snd_ac97_*()
- change bus operations to controller+slot parameters instead of
codec device
- Mark's review
- changed ac97_digital_controller into ac97_controller
- rename ac97_digital_controller_*() into ac97_controller_*()
- add the ac97 ac-link clock to the codec device (ie. the AC'97
BIT_CLK)
Since v2:
- more snd_ac97 namespace review
- change the compat allocation prototype to force the user to provide
and ac97_codec_device structure pointer
---
include/sound/ac97/codec.h | 115 +++++++++
include/sound/ac97/compat.h | 21 ++
include/sound/ac97/controller.h | 85 +++++++
sound/ac97/Kconfig | 19 ++
sound/ac97/Makefile | 8 +
sound/ac97/ac97_core.h | 10 +
sound/ac97/bus.c | 510 ++++++++++++++++++++++++++++++++++++++++
sound/ac97/codec.c | 15 ++
sound/ac97/snd_ac97_compat.c | 105 +++++++++
9 files changed, 888 insertions(+)
create mode 100644 include/sound/ac97/codec.h
create mode 100644 include/sound/ac97/compat.h
create mode 100644 include/sound/ac97/controller.h
create mode 100644 sound/ac97/Kconfig
create mode 100644 sound/ac97/Makefile
create mode 100644 sound/ac97/ac97_core.h
create mode 100644 sound/ac97/bus.c
create mode 100644 sound/ac97/codec.c
create mode 100644 sound/ac97/snd_ac97_compat.c
diff --git a/include/sound/ac97/codec.h b/include/sound/ac97/codec.h
new file mode 100644
index 000000000000..8901c1200522
--- /dev/null
+++ b/include/sound/ac97/codec.h
@@ -0,0 +1,115 @@
+/*
+ * Copyright (C) 2016 Robert Jarzmik <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+#ifndef __SOUND_AC97_CODEC2_H
+#define __SOUND_AC97_CODEC2_H
+
+#include <linux/device.h>
+
+#define AC97_ID(vendor_id1, vendor_id2) \
+ ((((vendor_id1) & 0xffff) << 16) | ((vendor_id2) & 0xffff))
+#define AC97_DRIVER_ID(vendor_id1, vendor_id2, mask_id1, mask_id2, _data) \
+ { .id = (((vendor_id1) & 0xffff) << 16) | ((vendor_id2) & 0xffff), \
+ .mask = (((mask_id1) & 0xffff) << 16) | ((mask_id2) & 0xffff), \
+ .data = (_data) }
+
+#define to_ac97_device(d) container_of(d, struct ac97_codec_device, dev)
+#define to_ac97_driver(d) container_of(d, struct ac97_codec_driver, driver)
+
+struct ac97_controller;
+struct clk;
+
+/**
+ * struct ac97_id - matches a codec device and driver on an ac97 bus
+ * @id: The significant bits if the codec vendor ID1 and ID2
+ * @mask: Bitmask specifying which bits of the id field are significant when
+ * matching. A driver binds to a device when :
+ * ((vendorID1 << 8 | vendorID2) & (mask_id1 << 8 | mask_id2)) == id.
+ * @data: Private data used by the driver.
+ */
+struct ac97_id {
+ unsigned int id;
+ unsigned int mask;
+ void *data;
+};
+
+/**
+ * ac97_codec_device - a ac97 codec
+ * @dev: the core device
+ * @vendor_id: the vendor_id of the codec, as sensed on the AC-link
+ * @num: the codec number, 0 is primary, 1 is first slave, etc ...
+ * @clk: the clock BIT_CLK provided by the codec
+ * @ac97_ctrl: ac97 digital controller on the same AC-link
+ *
+ * This is the device instanciated for each codec living on a AC-link. There are
+ * normally 0 to 4 codec devices per AC-link, and all of them are controlled by
+ * an AC97 digital controller.
+ */
+struct ac97_codec_device {
+ struct device dev;
+ unsigned int vendor_id;
+ unsigned int num;
+ struct clk *clk;
+ struct ac97_controller *ac97_ctrl;
+};
+
+/**
+ * ac97_codec_driver - a ac97 codec driver
+ * @driver: the device driver structure
+ * @probe: the function called when a ac97_codec_device is matched
+ * @remove: the function called when the device is unbound/removed
+ * @suspend: suspend function (might be NULL)
+ * @resume: resume function (might be NULL)
+ * @shutdown: shutdown function (might be NULL)
+ * @id_table: ac97 vendor_id match table, { } member terminated
+ */
+struct ac97_codec_driver {
+ struct device_driver driver;
+ int (*probe)(struct ac97_codec_device *);
+ int (*remove)(struct ac97_codec_device *);
+ int (*suspend)(struct ac97_codec_device *);
+ int (*resume)(struct ac97_codec_device *);
+ void (*shutdown)(struct ac97_codec_device *);
+ const struct ac97_id *id_table;
+};
+
+#if defined(CONFIG_AC97_BUS_NEW)
+int snd_ac97_codec_driver_register(struct ac97_codec_driver *drv);
+void snd_ac97_codec_driver_unregister(struct ac97_codec_driver *drv);
+#else
+static inline int
+snd_ac97_codec_driver_register(struct ac97_codec_driver *drv)
+{
+ return 0;
+}
+static inline void
+snd_ac97_codec_driver_unregister(struct ac97_codec_driver *drv)
+{
+}
+#endif
+
+
+static inline struct device *
+ac97_codec_dev2dev(struct ac97_codec_device *adev)
+{
+ return &adev->dev;
+}
+
+static inline void *ac97_get_drvdata(struct ac97_codec_device *adev)
+{
+ return dev_get_drvdata(ac97_codec_dev2dev(adev));
+}
+
+static inline void ac97_set_drvdata(struct ac97_codec_device *adev,
+ void *data)
+{
+ dev_set_drvdata(ac97_codec_dev2dev(adev), data);
+}
+
+void *snd_ac97_codec_get_platdata(const struct ac97_codec_device *adev);
+
+#endif
diff --git a/include/sound/ac97/compat.h b/include/sound/ac97/compat.h
new file mode 100644
index 000000000000..d876464bf7e4
--- /dev/null
+++ b/include/sound/ac97/compat.h
@@ -0,0 +1,21 @@
+/*
+ * Copyright (C) 2016 Robert Jarzmik <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * This file is for backward compatibility with snd_ac97 structure and its
+ * multiple usages, such as the snd_ac97_bus and snd_ac97_build_ops.
+ *
+ */
+#ifndef AC97_COMPAT_H
+#define AC97_COMPAT_H
+
+#include <sound/ac97_codec.h>
+#include <sound/soc.h>
+
+struct snd_ac97 *snd_ac97_compat_alloc(struct ac97_codec_device *adev);
+void snd_ac97_compat_release(struct snd_ac97 *ac97);
+
+#endif
diff --git a/include/sound/ac97/controller.h b/include/sound/ac97/controller.h
new file mode 100644
index 000000000000..5ff59bd7e324
--- /dev/null
+++ b/include/sound/ac97/controller.h
@@ -0,0 +1,85 @@
+/*
+ * Copyright (C) 2016 Robert Jarzmik <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+#ifndef AC97_CONTROLLER_H
+#define AC97_CONTROLLER_H
+
+#include <linux/list.h>
+
+#define AC97_BUS_MAX_CODECS 4
+#define AC97_SLOTS_AVAILABLE_ALL 0xf
+
+struct device;
+
+/**
+ * struct ac97_controller - The AC97 controller of the AC-Link
+ * @ops: the AC97 operations.
+ * @controllers: linked list of all existing controllers.
+ * @dev: the device providing the AC97 controller.
+ * @slots_available: the mask of accessible/scanable codecs.
+ * @codecs: the 4 possible AC97 codecs (NULL if none found).
+ * @codecs_pdata: platform_data for each codec (NULL if no pdata).
+ *
+ * This structure is internal to AC97 bus, and should not be used by the
+ * controllers themselves, excepting for using @dev.
+ */
+struct ac97_controller {
+ const struct ac97_controller_ops *ops;
+ struct list_head controllers;
+ struct device *dev;
+ unsigned short slots_available;
+ struct ac97_codec_device *codecs[AC97_BUS_MAX_CODECS];
+ void *codecs_pdata[AC97_BUS_MAX_CODECS];
+};
+
+/**
+ * struct ac97_controller_ops - The AC97 operations
+ * @reset: Cold reset of the AC97 AC-Link.
+ * @warm_reset: Warm reset of the AC97 AC-Link.
+ * @read: Read of a single AC97 register.
+ * Returns the register value or a negative error code.
+ * @write: Write of a single AC97 register.
+ * @wait: Wait for the current AC97 operation to finish (might be NULL).
+ * @init: Initialization of the AC97 AC-Link (might be NULL).
+ *
+ * These are the basic operation an AC97 controller must provide for an AC97
+ * access functions. Amongst these, all but the last 2 are mandatory.
+ * The slot number is also known as the AC97 codec number, between 0 and 3.
+ */
+struct ac97_controller_ops {
+ void (*reset)(struct ac97_controller *adrv);
+ void (*warm_reset)(struct ac97_controller *adrv);
+ int (*write)(struct ac97_controller *adrv, int slot,
+ unsigned short reg, unsigned short val);
+ int (*read)(struct ac97_controller *adrv, int slot, unsigned short reg);
+ void (*wait)(struct ac97_controller *adrv, int slot);
+ void (*init)(struct ac97_controller *adrv, int slot);
+};
+
+#if defined(CONFIG_AC97_BUS_NEW)
+int snd_ac97_controller_register(const struct ac97_controller_ops *ops,
+ struct device *dev,
+ unsigned short slots_available,
+ void **codecs_pdata);
+int snd_ac97_controller_unregister(struct device *dev);
+#else
+static inline int
+snd_ac97_controller_register(const struct ac97_controller_ops *ops,
+ struct device *dev,
+ unsigned short slots_available,
+ void **codecs_pdata)
+{
+ return 0;
+}
+
+static inline int snd_ac97_controller_unregister(struct device *dev)
+{
+ return 0;
+}
+#endif
+
+#endif
diff --git a/sound/ac97/Kconfig b/sound/ac97/Kconfig
new file mode 100644
index 000000000000..f8a64e15e5bf
--- /dev/null
+++ b/sound/ac97/Kconfig
@@ -0,0 +1,19 @@
+#
+# AC97 configuration
+#
+
+
+config AC97_BUS_NEW
+ tristate
+ select AC97
+ help
+ This is the new AC97 bus type, successor of AC97_BUS. The ported
+ drivers which benefit from the AC97 automatic probing should "select"
+ this instead of the AC97_BUS.
+ Say Y here if you want to have AC97 devices, which are sound oriented
+ devices around an AC-Link.
+
+config AC97_BUS_COMPAT
+ bool
+ depends on AC97_BUS_NEW
+ depends on !AC97_BUS
diff --git a/sound/ac97/Makefile b/sound/ac97/Makefile
new file mode 100644
index 000000000000..f9c2640bfb59
--- /dev/null
+++ b/sound/ac97/Makefile
@@ -0,0 +1,8 @@
+#
+# make for AC97 bus drivers
+#
+
+obj-$(CONFIG_AC97_BUS_NEW) += ac97.o
+
+ac97-y += bus.o codec.o
+ac97-$(CONFIG_AC97_BUS_COMPAT) += snd_ac97_compat.o
diff --git a/sound/ac97/ac97_core.h b/sound/ac97/ac97_core.h
new file mode 100644
index 000000000000..10cdfad8ad52
--- /dev/null
+++ b/sound/ac97/ac97_core.h
@@ -0,0 +1,10 @@
+/*
+ * Copyright (C) 2016 Robert Jarzmik <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+unsigned int snd_ac97_bus_scan_one(struct ac97_controller *ac97,
+ int codec_num);
diff --git a/sound/ac97/bus.c b/sound/ac97/bus.c
new file mode 100644
index 000000000000..6f26053d8be9
--- /dev/null
+++ b/sound/ac97/bus.c
@@ -0,0 +1,510 @@
+/*
+ * Copyright (C) 2016 Robert Jarzmik <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include <linux/module.h>
+#include <linux/bitops.h>
+#include <linux/clk.h>
+#include <linux/device.h>
+#include <linux/list.h>
+#include <linux/mutex.h>
+#include <linux/pm.h>
+#include <linux/pm_runtime.h>
+#include <linux/slab.h>
+#include <linux/sysfs.h>
+#include <sound/ac97/codec.h>
+#include <sound/ac97/controller.h>
+#include <sound/ac97/regs.h>
+
+#include "ac97_core.h"
+
+/*
+ * Protects ac97_controllers and each ac97_controller structure.
+ */
+static DEFINE_MUTEX(ac97_controllers_mutex);
+static LIST_HEAD(ac97_controllers);
+
+static struct bus_type ac97_bus_type;
+
+static struct ac97_codec_device *
+ac97_codec_find(struct ac97_controller *ac97_ctrl, int codec_num)
+{
+ if ((codec_num < 0) || (codec_num >= AC97_BUS_MAX_CODECS))
+ return ERR_PTR(-ERANGE);
+
+ return ac97_ctrl->codecs[codec_num];
+}
+
+static void ac97_codec_release(struct device *dev)
+{
+ struct ac97_codec_device *adev;
+ struct ac97_controller *ac97_ctrl;
+
+ adev = container_of(dev, struct ac97_codec_device, dev);
+ ac97_ctrl = adev->ac97_ctrl;
+ ac97_ctrl->codecs[adev->num] = NULL;
+ sysfs_remove_link(&dev->kobj, "ac97_controller");
+ kfree(adev);
+}
+
+static int ac97_codec_add(struct ac97_controller *ac97_ctrl, int idx,
+ unsigned int vendor_id)
+{
+ struct ac97_codec_device *codec;
+ char *codec_name;
+ int ret;
+
+ codec = kzalloc(sizeof(*codec), GFP_KERNEL);
+ if (!codec)
+ return -ENOMEM;
+ ac97_ctrl->codecs[idx] = codec;
+ codec->vendor_id = vendor_id;
+ codec->dev.release = ac97_codec_release;
+ codec->dev.bus = &ac97_bus_type;
+ codec->dev.parent = ac97_ctrl->dev;
+ codec->num = idx;
+ codec->ac97_ctrl = ac97_ctrl;
+
+ codec_name = kasprintf(GFP_KERNEL, "%s:%d", dev_name(ac97_ctrl->dev),
+ idx);
+ codec->dev.init_name = codec_name;
+
+ ret = device_register(&codec->dev);
+ kfree(codec_name);
+ if (ret)
+ goto err_free_codec;
+
+ ret = sysfs_create_link(&codec->dev.kobj, &ac97_ctrl->dev->kobj,
+ "ac97_controller");
+ if (ret)
+ goto err_unregister_device;
+
+ return 0;
+err_unregister_device:
+ put_device(&codec->dev);
+err_free_codec:
+ kfree(codec);
+ ac97_ctrl->codecs[idx] = NULL;
+
+ return ret;
+}
+
+unsigned int snd_ac97_bus_scan_one(struct ac97_controller *adrv,
+ int codec_num)
+{
+ unsigned short vid1, vid2;
+ int ret;
+
+ ret = adrv->ops->read(adrv, codec_num, AC97_VENDOR_ID1);
+ vid1 = (ret & 0xffff);
+ if (ret < 0)
+ return 0;
+
+ ret = adrv->ops->read(adrv, codec_num, AC97_VENDOR_ID2);
+ vid2 = (ret & 0xffff);
+ if (ret < 0)
+ return 0;
+
+ dev_dbg(adrv->dev, "%s(codec_num=%d): vendor_id=0x%08x\n",
+ __func__, codec_num, AC97_ID(vid1, vid2));
+ return AC97_ID(vid1, vid2);
+}
+
+static int ac97_bus_scan(struct ac97_controller *ac97_ctrl)
+{
+ int ret, i;
+ unsigned int vendor_id;
+
+ for (i = 0; i < AC97_BUS_MAX_CODECS; i++) {
+ if (ac97_codec_find(ac97_ctrl, i))
+ continue;
+ if (!(ac97_ctrl->slots_available & BIT(i)))
+ continue;
+ vendor_id = snd_ac97_bus_scan_one(ac97_ctrl, i);
+ if (!vendor_id)
+ continue;
+
+ ret = ac97_codec_add(ac97_ctrl, i, vendor_id);
+ if (ret < 0)
+ return ret;
+ }
+ return 0;
+}
+
+static void ac97_rescan_all_controllers(void)
+{
+ struct ac97_controller *ac97_ctrl;
+ int ret;
+
+ mutex_lock(&ac97_controllers_mutex);
+ list_for_each_entry(ac97_ctrl, &ac97_controllers, controllers) {
+ ret = ac97_bus_scan(ac97_ctrl);
+ if (ret)
+ dev_warn(ac97_ctrl->dev, "scan failed: %d\n", ret);
+ }
+ mutex_unlock(&ac97_controllers_mutex);
+}
+
+static int ac97_bus_reset(struct ac97_controller *ac97_ctrl)
+{
+ ac97_ctrl->ops->reset(ac97_ctrl);
+
+ return 0;
+}
+
+/**
+ * snd_ac97_codec_driver_register - register an AC97 codec driver
+ * @dev: AC97 driver codec to register
+ *
+ * Register an AC97 codec driver to the ac97 bus driver, aka. the AC97 digital
+ * controller.
+ *
+ * Returns 0 on success or error code
+ */
+int snd_ac97_codec_driver_register(struct ac97_codec_driver *drv)
+{
+ int ret;
+
+ drv->driver.bus = &ac97_bus_type;
+
+ ret = driver_register(&drv->driver);
+ if (!ret)
+ ac97_rescan_all_controllers();
+
+ return ret;
+}
+EXPORT_SYMBOL(snd_ac97_codec_driver_register);
+
+/**
+ * snd_ac97_codec_driver_unregister - unregister an AC97 codec driver
+ * @dev: AC97 codec driver to unregister
+ *
+ * Unregister a previously registered ac97 codec driver.
+ */
+void snd_ac97_codec_driver_unregister(struct ac97_codec_driver *drv)
+{
+ driver_unregister(&drv->driver);
+}
+EXPORT_SYMBOL(snd_ac97_codec_driver_unregister);
+
+/**
+ * snd_ac97_codec_get_platdata - get platform_data
+ * @adev: the ac97 codec device
+ *
+ * For legacy platforms, in order to have platform_data in codec drivers
+ * available, while ac97 device are auto-created upon probe, this retrieves the
+ * platdata which was setup on ac97 controller registration.
+ *
+ * Returns the platform data pointer
+ */
+void *snd_ac97_codec_get_platdata(const struct ac97_codec_device *adev)
+{
+ struct ac97_controller *ac97_ctrl = adev->ac97_ctrl;
+
+ return ac97_ctrl->codecs_pdata[adev->num];
+}
+EXPORT_SYMBOL(snd_ac97_codec_get_platdata);
+
+static struct ac97_controller *ac97_ctrl_find(struct device *dev)
+{
+ struct ac97_controller *ac97_ctrl, *tmp;
+
+ list_for_each_entry_safe(ac97_ctrl, tmp, &ac97_controllers,
+ controllers)
+ if (ac97_ctrl->dev == dev)
+ return ac97_ctrl;
+
+ return NULL;
+}
+static int ac97_ctrl_codecs_unregister(struct ac97_controller *ac97_ctrl)
+{
+ int i;
+
+ for (i = 0; i < AC97_BUS_MAX_CODECS; i++)
+ if (ac97_ctrl->codecs[i])
+ put_device(&ac97_ctrl->codecs[i]->dev);
+
+ return 0;
+}
+
+static ssize_t cold_reset_store(struct device *dev,
+ struct device_attribute *attr, const char *buf,
+ size_t len)
+{
+ struct ac97_controller *ac97_ctrl = ac97_ctrl_find(dev);
+
+ if (!dev)
+ return -ENODEV;
+
+ ac97_ctrl->ops->reset(ac97_ctrl);
+ return len;
+}
+static DEVICE_ATTR_WO(cold_reset);
+
+static ssize_t warm_reset_store(struct device *dev,
+ struct device_attribute *attr, const char *buf,
+ size_t len)
+{
+ struct ac97_controller *ac97_ctrl = ac97_ctrl_find(dev);
+
+ if (!dev)
+ return -ENODEV;
+
+ ac97_ctrl->ops->warm_reset(ac97_ctrl);
+ return len;
+}
+static DEVICE_ATTR_WO(warm_reset);
+
+static struct attribute *ac97_controller_device_attrs[] = {
+ &dev_attr_cold_reset.attr,
+ &dev_attr_warm_reset.attr,
+ NULL
+};
+
+static const struct attribute_group ac97_controller_attr_group = {
+ .name = "ac97_operations",
+ .attrs = ac97_controller_device_attrs,
+};
+
+/**
+ * snd_ac97_controller_register - register an ac97 controller
+ * @ops: the ac97 bus operations
+ * @dev: the device providing the ac97 DC function
+ * @slots_available: mask of the ac97 codecs that can be scanned and probed
+ * bit0 => codec 0, bit1 => codec 1 ... bit 3 => codec 3
+ *
+ * Register a digital controller which can control up to 4 ac97 codecs. This is
+ * the controller side of the AC97 AC-link, while the slave side are the codecs.
+ *
+ * Returns 0 upon success, negative value upon error
+ */
+int snd_ac97_controller_register(const struct ac97_controller_ops *ops,
+ struct device *dev,
+ unsigned short slots_available,
+ void **codecs_pdata)
+{
+ struct ac97_controller *ac97_ctrl;
+ int ret, i;
+
+ ac97_ctrl = kzalloc(sizeof(*ac97_ctrl), GFP_KERNEL);
+ if (!ac97_ctrl)
+ return -ENOMEM;
+
+ for (i = 0; i < AC97_BUS_MAX_CODECS && codecs_pdata; i++)
+ ac97_ctrl->codecs_pdata[i] = codecs_pdata[i];
+
+ ret = sysfs_create_group(&dev->kobj, &ac97_controller_attr_group);
+ if (ret)
+ return ret;
+
+ mutex_lock(&ac97_controllers_mutex);
+ ac97_ctrl->ops = ops;
+ ac97_ctrl->slots_available = slots_available;
+ ac97_ctrl->dev = dev;
+ list_add(&ac97_ctrl->controllers, &ac97_controllers);
+ mutex_unlock(&ac97_controllers_mutex);
+
+ ac97_bus_reset(ac97_ctrl);
+ ac97_bus_scan(ac97_ctrl);
+
+ return 0;
+}
+EXPORT_SYMBOL(snd_ac97_controller_register);
+
+/**
+ * snd_ac97_controller_unregister - unregister an ac97 controller
+ * @dev: the device previously provided to ac97_controller_register()
+ *
+ * Returns 0 on success, negative upon error
+ */
+int snd_ac97_controller_unregister(struct device *dev)
+{
+ struct ac97_controller *ac97_ctrl;
+ int ret = -ENODEV, i;
+
+ mutex_lock(&ac97_controllers_mutex);
+ ac97_ctrl = ac97_ctrl_find(dev);
+ if (ac97_ctrl) {
+ ret = 0;
+ for (i = 0; i < AC97_BUS_MAX_CODECS; i++)
+ if (ac97_ctrl->codecs[i] &&
+ device_is_registered(&ac97_ctrl->codecs[i]->dev))
+ ret = -EBUSY;
+ if (!ret)
+ ret = ac97_ctrl_codecs_unregister(ac97_ctrl);
+ if (!ret) {
+ list_del(&ac97_ctrl->controllers);
+ sysfs_remove_group(&dev->kobj,
+ &ac97_controller_attr_group);
+ }
+ }
+ mutex_unlock(&ac97_controllers_mutex);
+
+ return ret;
+}
+EXPORT_SYMBOL(snd_ac97_controller_unregister);
+
+#ifdef CONFIG_PM
+static int ac97_pm_runtime_suspend(struct device *dev)
+{
+ struct ac97_codec_device *codec = to_ac97_device(dev);
+ int ret = pm_generic_runtime_suspend(dev);
+
+ if (ret == 0 && dev->driver) {
+ if (pm_runtime_is_irq_safe(dev))
+ clk_disable(codec->clk);
+ else
+ clk_disable_unprepare(codec->clk);
+ }
+
+ return ret;
+}
+
+static int ac97_pm_runtime_resume(struct device *dev)
+{
+ struct ac97_codec_device *codec = to_ac97_device(dev);
+ int ret;
+
+ if (dev->driver) {
+ if (pm_runtime_is_irq_safe(dev))
+ ret = clk_enable(codec->clk);
+ else
+ ret = clk_prepare_enable(codec->clk);
+ if (ret)
+ return ret;
+ }
+
+ return pm_generic_runtime_resume(dev);
+}
+#endif /* CONFIG_PM */
+
+static const struct dev_pm_ops ac97_pm = {
+ .suspend = pm_generic_suspend,
+ .resume = pm_generic_resume,
+ .freeze = pm_generic_freeze,
+ .thaw = pm_generic_thaw,
+ .poweroff = pm_generic_poweroff,
+ .restore = pm_generic_restore,
+ SET_RUNTIME_PM_OPS(
+ ac97_pm_runtime_suspend,
+ ac97_pm_runtime_resume,
+ NULL)
+};
+
+static int ac97_get_enable_clk(struct ac97_codec_device *adev)
+{
+ int ret;
+
+ adev->clk = clk_get(&adev->dev, "ac97_clk");
+ if (IS_ERR(adev->clk))
+ return PTR_ERR(adev->clk);
+
+ ret = clk_prepare_enable(adev->clk);
+ if (ret)
+ clk_put(adev->clk);
+
+ return ret;
+}
+
+static void ac97_put_disable_clk(struct ac97_codec_device *adev)
+{
+ clk_disable_unprepare(adev->clk);
+ clk_put(adev->clk);
+}
+
+static ssize_t vendor_id_show(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ struct ac97_codec_device *codec = to_ac97_device(dev);
+
+ return sprintf(buf, "%08x", codec->vendor_id);
+}
+
+static struct device_attribute ac97_dev_attrs[] = {
+ __ATTR_RO(vendor_id),
+ __ATTR_NULL,
+};
+
+static int ac97_bus_match(struct device *dev, struct device_driver *drv)
+{
+ struct ac97_codec_device *adev = to_ac97_device(dev);
+ struct ac97_codec_driver *adrv = to_ac97_driver(drv);
+ const struct ac97_id *id = adrv->id_table;
+ int i = 0;
+
+ if (adev->vendor_id == 0x0 || adev->vendor_id == 0xffffffff)
+ return false;
+
+ do {
+ if ((id[i].id & id->mask) == (adev->vendor_id & id[i].mask))
+ return true;
+ } while (id[i++].id);
+
+ return false;
+}
+
+static int ac97_bus_probe(struct device *dev)
+{
+ struct ac97_codec_device *adev = to_ac97_device(dev);
+ struct ac97_codec_driver *adrv = to_ac97_driver(dev->driver);
+ int ret;
+
+ ret = ac97_get_enable_clk(adev);
+ if (ret)
+ return ret;
+
+ pm_runtime_get_noresume(dev);
+ pm_runtime_set_active(dev);
+ pm_runtime_enable(dev);
+
+ ret = adrv->probe(adev);
+ if (ret == 0)
+ return 0;
+
+ pm_runtime_disable(dev);
+ pm_runtime_set_suspended(dev);
+ pm_runtime_put_noidle(dev);
+ ac97_put_disable_clk(adev);
+
+ return ret;
+}
+
+static int ac97_bus_remove(struct device *dev)
+{
+ struct ac97_codec_device *adev = to_ac97_device(dev);
+ struct ac97_codec_driver *adrv = to_ac97_driver(dev->driver);
+ int ret;
+
+ ret = pm_runtime_get_sync(dev);
+ if (ret)
+ return ret;
+
+ ret = adrv->remove(adev);
+ pm_runtime_put_noidle(dev);
+ if (ret == 0)
+ ac97_put_disable_clk(adev);
+
+ return ret;
+}
+
+static struct bus_type ac97_bus_type = {
+ .name = "ac97",
+ .dev_attrs = ac97_dev_attrs,
+ .match = ac97_bus_match,
+ .pm = &ac97_pm,
+ .probe = ac97_bus_probe,
+ .remove = ac97_bus_remove,
+};
+
+static int __init ac97_bus_init(void)
+{
+ return bus_register(&ac97_bus_type);
+}
+subsys_initcall(ac97_bus_init);
+
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Robert Jarzmik <[email protected]>");
diff --git a/sound/ac97/codec.c b/sound/ac97/codec.c
new file mode 100644
index 000000000000..a835f03744bf
--- /dev/null
+++ b/sound/ac97/codec.c
@@ -0,0 +1,15 @@
+/*
+ * Copyright (C) 2016 Robert Jarzmik <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include <sound/ac97_codec.h>
+#include <sound/ac97/codec.h>
+#include <sound/ac97/controller.h>
+#include <linux/device.h>
+#include <linux/slab.h>
+#include <sound/soc.h> /* For compat_ac97_* */
+
diff --git a/sound/ac97/snd_ac97_compat.c b/sound/ac97/snd_ac97_compat.c
new file mode 100644
index 000000000000..ac8d835c1513
--- /dev/null
+++ b/sound/ac97/snd_ac97_compat.c
@@ -0,0 +1,105 @@
+/*
+ * Copyright (C) 2016 Robert Jarzmik <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include <linux/list.h>
+#include <linux/slab.h>
+#include <sound/ac97/codec.h>
+#include <sound/ac97/compat.h>
+#include <sound/ac97/controller.h>
+#include <sound/soc.h>
+
+#include "ac97_core.h"
+
+static void compat_ac97_reset(struct snd_ac97 *ac97)
+{
+ struct ac97_codec_device *adev = to_ac97_device(ac97->private_data);
+ struct ac97_controller *actrl = adev->ac97_ctrl;
+
+ if (actrl->ops->reset)
+ actrl->ops->reset(actrl);
+}
+
+static void compat_ac97_warm_reset(struct snd_ac97 *ac97)
+{
+ struct ac97_codec_device *adev = to_ac97_device(ac97->private_data);
+ struct ac97_controller *actrl = adev->ac97_ctrl;
+
+ if (actrl->ops->warm_reset)
+ actrl->ops->warm_reset(actrl);
+}
+
+static void compat_ac97_write(struct snd_ac97 *ac97, unsigned short reg,
+ unsigned short val)
+{
+ struct ac97_codec_device *adev = to_ac97_device(ac97->private_data);
+ struct ac97_controller *actrl = adev->ac97_ctrl;
+
+ actrl->ops->write(actrl, ac97->num, reg, val);
+}
+
+static unsigned short compat_ac97_read(struct snd_ac97 *ac97,
+ unsigned short reg)
+{
+ struct ac97_codec_device *adev = to_ac97_device(ac97->private_data);
+ struct ac97_controller *actrl = adev->ac97_ctrl;
+
+ return actrl->ops->read(actrl, ac97->num, reg);
+}
+
+static struct snd_ac97_bus_ops compat_snd_ac97_bus_ops = {
+ .reset = compat_ac97_reset,
+ .warm_reset = compat_ac97_warm_reset,
+ .write = compat_ac97_write,
+ .read = compat_ac97_read,
+};
+
+static struct snd_ac97_bus compat_soc_ac97_bus = {
+ .ops = &compat_snd_ac97_bus_ops,
+};
+
+struct snd_ac97 *snd_ac97_compat_alloc(struct ac97_codec_device *adev)
+{
+ struct snd_ac97 *ac97;
+
+ ac97 = kzalloc(sizeof(struct snd_ac97), GFP_KERNEL);
+ if (ac97 == NULL)
+ return ERR_PTR(-ENOMEM);
+
+ ac97->dev = adev->dev;
+ ac97->private_data = adev;
+ ac97->bus = &compat_soc_ac97_bus;
+ return ac97;
+}
+EXPORT_SYMBOL_GPL(snd_ac97_compat_alloc);
+
+void snd_ac97_compat_release(struct snd_ac97 *ac97)
+{
+ kfree(ac97);
+}
+EXPORT_SYMBOL_GPL(snd_ac97_compat_release);
+
+int snd_ac97_reset(struct snd_ac97 *ac97, bool try_warm, unsigned int id,
+ unsigned int id_mask)
+{
+ struct ac97_codec_device *adev = to_ac97_device(ac97->private_data);
+ struct ac97_controller *actrl = adev->ac97_ctrl;
+
+ if (try_warm) {
+ compat_ac97_warm_reset(ac97);
+ if (snd_ac97_bus_scan_one(actrl, adev->num) == adev->vendor_id)
+ return 1;
+ }
+
+ compat_ac97_reset(ac97);
+ compat_ac97_warm_reset(ac97);
+ if (snd_ac97_bus_scan_one(actrl, adev->num) == adev->vendor_id)
+ return 0;
+
+ return -ENODEV;
+}
+EXPORT_SYMBOL_GPL(snd_ac97_reset);
--
2.1.4
Add the new ac97 bus support, with ac97 bus automatic probing.
Signed-off-by: Robert Jarzmik <[email protected]>
---
sound/Kconfig | 2 ++
sound/Makefile | 1 +
sound/soc/Kconfig | 4 ++++
3 files changed, 7 insertions(+)
diff --git a/sound/Kconfig b/sound/Kconfig
index 5a240e050ae6..c7e1cbe84951 100644
--- a/sound/Kconfig
+++ b/sound/Kconfig
@@ -80,6 +80,8 @@ source "sound/hda/Kconfig"
source "sound/ppc/Kconfig"
+source "sound/ac97/Kconfig"
+
source "sound/aoa/Kconfig"
source "sound/arm/Kconfig"
diff --git a/sound/Makefile b/sound/Makefile
index c41bdf5fdf24..cae6a8807bec 100644
--- a/sound/Makefile
+++ b/sound/Makefile
@@ -10,6 +10,7 @@ obj-$(CONFIG_SND_AOA) += aoa/
# This one must be compilable even if sound is configured out
obj-$(CONFIG_AC97_BUS) += ac97_bus.o
+obj-$(CONFIG_AC97_BUS_NEW) += ac97/
ifeq ($(CONFIG_SND),y)
obj-y += last.o
diff --git a/sound/soc/Kconfig b/sound/soc/Kconfig
index 182d92efc7c8..5ad0a0422054 100644
--- a/sound/soc/Kconfig
+++ b/sound/soc/Kconfig
@@ -6,6 +6,7 @@ menuconfig SND_SOC
tristate "ALSA for SoC audio support"
select SND_PCM
select AC97_BUS if SND_SOC_AC97_BUS
+ select AC97_BUS_NEW if SND_SOC_AC97_BUS_NEW
select SND_JACK
select REGMAP_I2C if I2C
select REGMAP_SPI if SPI_MASTER
@@ -25,6 +26,9 @@ if SND_SOC
config SND_SOC_AC97_BUS
bool
+config SND_SOC_AC97_BUS_NEW
+ bool
+
config SND_SOC_GENERIC_DMAENGINE_PCM
bool
select SND_DMAENGINE_PCM
--
2.1.4
This adds support for the new AC97 bus code, which discovers the devices
rather than uses platform data.
As part of this discovery, it enables a multi-function device wm97xx,
which supports touchscreen, battery, ADC and an audio codec. This patch
adds the code to bind the touchscreen "cell" as the touchscreen driver.
This was tested on the pxa architecture with a pxa270 + wm9713 + the
mioa701 touchscreen.
Signed-off-by: Robert Jarzmik <[email protected]>
---
drivers/input/touchscreen/Kconfig | 2 +-
drivers/input/touchscreen/wm97xx-core.c | 56 ++++++++++++++++++++++++++++++++-
2 files changed, 56 insertions(+), 2 deletions(-)
diff --git a/drivers/input/touchscreen/Kconfig b/drivers/input/touchscreen/Kconfig
index efca0133e266..7af06015e704 100644
--- a/drivers/input/touchscreen/Kconfig
+++ b/drivers/input/touchscreen/Kconfig
@@ -737,7 +737,7 @@ config TOUCHSCREEN_WM831X
config TOUCHSCREEN_WM97XX
tristate "Support for WM97xx AC97 touchscreen controllers"
- depends on AC97_BUS
+ depends on AC97_BUS || AC97_BUS_NEW
help
Say Y here if you have a Wolfson Microelectronics WM97xx
touchscreen connected to your system. Note that this option
diff --git a/drivers/input/touchscreen/wm97xx-core.c b/drivers/input/touchscreen/wm97xx-core.c
index 50a110e2988b..4d5c96a5ab04 100644
--- a/drivers/input/touchscreen/wm97xx-core.c
+++ b/drivers/input/touchscreen/wm97xx-core.c
@@ -44,6 +44,7 @@
#include <linux/pm.h>
#include <linux/interrupt.h>
#include <linux/bitops.h>
+#include <linux/mfd/wm97xx.h>
#include <linux/workqueue.h>
#include <linux/wm97xx.h>
#include <linux/uaccess.h>
@@ -763,6 +764,39 @@ static int wm97xx_remove(struct device *dev)
return 0;
}
+static int wm97xx_mfd_probe(struct platform_device *pdev)
+{
+ struct wm97xx *wm;
+ struct wm97xx_platform_data *mfd_pdata = dev_get_platdata(&pdev->dev);
+ int ret;
+
+ wm = devm_kzalloc(&pdev->dev, sizeof(struct wm97xx), GFP_KERNEL);
+ if (!wm)
+ return -ENOMEM;
+
+ wm->dev = &pdev->dev;
+ wm->ac97 = mfd_pdata->ac97;
+
+ ret = _wm97xx_probe(wm);
+ if (ret)
+ return ret;
+
+ ret = wm97xx_add_battery(wm, mfd_pdata->batt_pdata);
+ if (ret < 0)
+ goto batt_err;
+
+ return ret;
+
+batt_err:
+ wm97xx_unregister_touch(wm);
+ return ret;
+}
+
+static int wm97xx_mfd_remove(struct platform_device *pdev)
+{
+ return wm97xx_remove(&pdev->dev);
+}
+
static int __maybe_unused wm97xx_suspend(struct device *dev)
{
struct wm97xx *wm = dev_get_drvdata(dev);
@@ -859,21 +893,41 @@ EXPORT_SYMBOL_GPL(wm97xx_unregister_mach_ops);
static struct device_driver wm97xx_driver = {
.name = "wm97xx-ts",
+#ifdef CONFIG_AC97_BUS
.bus = &ac97_bus_type,
+#endif
.owner = THIS_MODULE,
.probe = wm97xx_probe,
.remove = wm97xx_remove,
.pm = &wm97xx_pm_ops,
};
+static struct platform_driver wm97xx_mfd_driver = {
+ .driver = {
+ .name = "wm97xx-ts",
+ .pm = &wm97xx_pm_ops,
+ },
+ .probe = wm97xx_mfd_probe,
+ .remove = wm97xx_mfd_remove,
+};
+
static int __init wm97xx_init(void)
{
- return driver_register(&wm97xx_driver);
+ int ret;
+
+ ret = platform_driver_register(&wm97xx_mfd_driver);
+ if (ret)
+ return ret;
+
+ if (IS_BUILTIN(CONFIG_AC97_BUS))
+ ret = driver_register(&wm97xx_driver);
+ return ret;
}
static void __exit wm97xx_exit(void)
{
driver_unregister(&wm97xx_driver);
+ platform_driver_unregister(&wm97xx_mfd_driver);
}
module_init(wm97xx_init);
--
2.1.4
Switch to the new ac97 bus support in sound/ac97 instead of the legacy
snd_ac97 one.
Signed-off-by: Robert Jarzmik <[email protected]>
---
include/sound/pxa2xx-lib.h | 15 +++++++++------
sound/arm/Kconfig | 1 -
sound/arm/pxa2xx-ac97-lib.c | 39 +++++++++++++++++++++++----------------
sound/soc/pxa/Kconfig | 5 ++---
sound/soc/pxa/pxa2xx-ac97.c | 22 +++++++++++++---------
5 files changed, 47 insertions(+), 35 deletions(-)
diff --git a/include/sound/pxa2xx-lib.h b/include/sound/pxa2xx-lib.h
index 6ef629bde164..0e2b8ae3e00e 100644
--- a/include/sound/pxa2xx-lib.h
+++ b/include/sound/pxa2xx-lib.h
@@ -2,7 +2,8 @@
#define PXA2XX_LIB_H
#include <linux/platform_device.h>
-#include <sound/ac97_codec.h>
+#include <sound/ac97/controller.h>
+#include <sound/ac97/compat.h>
/* PCM */
@@ -21,12 +22,14 @@ extern void pxa2xx_pcm_free_dma_buffers(struct snd_pcm *pcm);
/* AC97 */
-extern unsigned short pxa2xx_ac97_read(struct snd_ac97 *ac97, unsigned short reg);
-extern void pxa2xx_ac97_write(struct snd_ac97 *ac97, unsigned short reg, unsigned short val);
+extern int pxa2xx_ac97_read(struct ac97_controller *adrv, int slot,
+ unsigned short reg);
+extern int pxa2xx_ac97_write(struct ac97_controller *adrv, int slot,
+ unsigned short reg, unsigned short val);
-extern bool pxa2xx_ac97_try_warm_reset(struct snd_ac97 *ac97);
-extern bool pxa2xx_ac97_try_cold_reset(struct snd_ac97 *ac97);
-extern void pxa2xx_ac97_finish_reset(struct snd_ac97 *ac97);
+extern bool pxa2xx_ac97_try_warm_reset(struct ac97_controller *adrv);
+extern bool pxa2xx_ac97_try_cold_reset(struct ac97_controller *adrv);
+extern void pxa2xx_ac97_finish_reset(struct ac97_controller *adrv);
extern int pxa2xx_ac97_hw_suspend(void);
extern int pxa2xx_ac97_hw_resume(void);
diff --git a/sound/arm/Kconfig b/sound/arm/Kconfig
index 65171f6657a2..f1f25704fe52 100644
--- a/sound/arm/Kconfig
+++ b/sound/arm/Kconfig
@@ -36,7 +36,6 @@ endif # SND_ARM
config SND_PXA2XX_LIB
tristate
- select SND_AC97_CODEC if SND_PXA2XX_LIB_AC97
select SND_DMAENGINE_PCM
config SND_PXA2XX_LIB_AC97
diff --git a/sound/arm/pxa2xx-ac97-lib.c b/sound/arm/pxa2xx-ac97-lib.c
index 39c3969ac1c7..62b31e909d31 100644
--- a/sound/arm/pxa2xx-ac97-lib.c
+++ b/sound/arm/pxa2xx-ac97-lib.c
@@ -20,7 +20,7 @@
#include <linux/io.h>
#include <linux/gpio.h>
-#include <sound/ac97_codec.h>
+#include <sound/ac97/controller.h>
#include <sound/pxa2xx-lib.h>
#include <mach/irqs.h>
@@ -46,38 +46,41 @@ extern void pxa27x_configure_ac97reset(int reset_gpio, bool to_gpio);
* 1 jiffy timeout if interrupt never comes).
*/
-unsigned short pxa2xx_ac97_read(struct snd_ac97 *ac97, unsigned short reg)
+int pxa2xx_ac97_read(struct ac97_controller *ac97, int slot, unsigned short reg)
{
- unsigned short val = -1;
+ int val = -ENODEV;
volatile u32 *reg_addr;
+ if (slot > 0)
+ return -ENODEV;
+
mutex_lock(&car_mutex);
/* set up primary or secondary codec space */
if (cpu_is_pxa25x() && reg == AC97_GPIO_STATUS)
- reg_addr = ac97->num ? &SMC_REG_BASE : &PMC_REG_BASE;
+ reg_addr = slot ? &SMC_REG_BASE : &PMC_REG_BASE;
else
- reg_addr = ac97->num ? &SAC_REG_BASE : &PAC_REG_BASE;
+ reg_addr = slot ? &SAC_REG_BASE : &PAC_REG_BASE;
reg_addr += (reg >> 1);
/* start read access across the ac97 link */
GSR = GSR_CDONE | GSR_SDONE;
gsr_bits = 0;
- val = *reg_addr;
+ val = (*reg_addr & 0xffff);
if (reg == AC97_GPIO_STATUS)
goto out;
if (wait_event_timeout(gsr_wq, (GSR | gsr_bits) & GSR_SDONE, 1) <= 0 &&
!((GSR | gsr_bits) & GSR_SDONE)) {
printk(KERN_ERR "%s: read error (ac97_reg=%d GSR=%#lx)\n",
__func__, reg, GSR | gsr_bits);
- val = -1;
+ val = -ETIMEDOUT;
goto out;
}
/* valid data now */
GSR = GSR_CDONE | GSR_SDONE;
gsr_bits = 0;
- val = *reg_addr;
+ val = (*reg_addr & 0xffff);
/* but we've just started another cycle... */
wait_event_timeout(gsr_wq, (GSR | gsr_bits) & GSR_SDONE, 1);
@@ -86,29 +89,33 @@ out: mutex_unlock(&car_mutex);
}
EXPORT_SYMBOL_GPL(pxa2xx_ac97_read);
-void pxa2xx_ac97_write(struct snd_ac97 *ac97, unsigned short reg,
- unsigned short val)
+int pxa2xx_ac97_write(struct ac97_controller *ac97, int slot,
+ unsigned short reg, unsigned short val)
{
volatile u32 *reg_addr;
+ int ret = 0;
mutex_lock(&car_mutex);
/* set up primary or secondary codec space */
if (cpu_is_pxa25x() && reg == AC97_GPIO_STATUS)
- reg_addr = ac97->num ? &SMC_REG_BASE : &PMC_REG_BASE;
+ reg_addr = slot ? &SMC_REG_BASE : &PMC_REG_BASE;
else
- reg_addr = ac97->num ? &SAC_REG_BASE : &PAC_REG_BASE;
+ reg_addr = slot ? &SAC_REG_BASE : &PAC_REG_BASE;
reg_addr += (reg >> 1);
GSR = GSR_CDONE | GSR_SDONE;
gsr_bits = 0;
*reg_addr = val;
if (wait_event_timeout(gsr_wq, (GSR | gsr_bits) & GSR_CDONE, 1) <= 0 &&
- !((GSR | gsr_bits) & GSR_CDONE))
+ !((GSR | gsr_bits) & GSR_CDONE)) {
printk(KERN_ERR "%s: write error (ac97_reg=%d GSR=%#lx)\n",
__func__, reg, GSR | gsr_bits);
+ ret = -EIO;
+ }
mutex_unlock(&car_mutex);
+ return ret;
}
EXPORT_SYMBOL_GPL(pxa2xx_ac97_write);
@@ -188,7 +195,7 @@ static inline void pxa_ac97_cold_pxa3xx(void)
}
#endif
-bool pxa2xx_ac97_try_warm_reset(struct snd_ac97 *ac97)
+bool pxa2xx_ac97_try_warm_reset(struct ac97_controller *adrv)
{
unsigned long gsr;
unsigned int timeout = 100;
@@ -225,7 +232,7 @@ bool pxa2xx_ac97_try_warm_reset(struct snd_ac97 *ac97)
}
EXPORT_SYMBOL_GPL(pxa2xx_ac97_try_warm_reset);
-bool pxa2xx_ac97_try_cold_reset(struct snd_ac97 *ac97)
+bool pxa2xx_ac97_try_cold_reset(struct ac97_controller *adrv)
{
unsigned long gsr;
unsigned int timeout = 1000;
@@ -263,7 +270,7 @@ bool pxa2xx_ac97_try_cold_reset(struct snd_ac97 *ac97)
EXPORT_SYMBOL_GPL(pxa2xx_ac97_try_cold_reset);
-void pxa2xx_ac97_finish_reset(struct snd_ac97 *ac97)
+void pxa2xx_ac97_finish_reset(struct ac97_controller *adrv)
{
GCR &= ~(GCR_PRIRDY_IEN|GCR_SECRDY_IEN);
GCR |= GCR_SDONE_IE|GCR_CDONE_IE;
diff --git a/sound/soc/pxa/Kconfig b/sound/soc/pxa/Kconfig
index f2bf8661dd21..d390a789f3ab 100644
--- a/sound/soc/pxa/Kconfig
+++ b/sound/soc/pxa/Kconfig
@@ -19,13 +19,12 @@ config SND_MMP_SOC
config SND_PXA2XX_AC97
tristate
- select SND_AC97_CODEC
config SND_PXA2XX_SOC_AC97
tristate
- select AC97_BUS
+ select AC97_BUS_NEW
select SND_PXA2XX_LIB_AC97
- select SND_SOC_AC97_BUS
+ select SND_SOC_AC97_BUS_NEW
config SND_PXA2XX_SOC_I2S
tristate
diff --git a/sound/soc/pxa/pxa2xx-ac97.c b/sound/soc/pxa/pxa2xx-ac97.c
index 9615e6de1306..aba1bd5eb817 100644
--- a/sound/soc/pxa/pxa2xx-ac97.c
+++ b/sound/soc/pxa/pxa2xx-ac97.c
@@ -17,6 +17,7 @@
#include <linux/dmaengine.h>
#include <linux/dma/pxa-dma.h>
+#include <sound/ac97/controller.h>
#include <sound/core.h>
#include <sound/ac97_codec.h>
#include <sound/soc.h>
@@ -29,21 +30,21 @@
#include "pxa2xx-ac97.h"
-static void pxa2xx_ac97_warm_reset(struct snd_ac97 *ac97)
+static void pxa2xx_ac97_warm_reset(struct ac97_controller *adrv)
{
- pxa2xx_ac97_try_warm_reset(ac97);
+ pxa2xx_ac97_try_warm_reset(adrv);
- pxa2xx_ac97_finish_reset(ac97);
+ pxa2xx_ac97_finish_reset(adrv);
}
-static void pxa2xx_ac97_cold_reset(struct snd_ac97 *ac97)
+static void pxa2xx_ac97_cold_reset(struct ac97_controller *adrv)
{
- pxa2xx_ac97_try_cold_reset(ac97);
+ pxa2xx_ac97_try_cold_reset(adrv);
- pxa2xx_ac97_finish_reset(ac97);
+ pxa2xx_ac97_finish_reset(adrv);
}
-static struct snd_ac97_bus_ops pxa2xx_ac97_ops = {
+static struct ac97_controller_ops pxa2xx_ac97_ops = {
.read = pxa2xx_ac97_read,
.write = pxa2xx_ac97_write,
.warm_reset = pxa2xx_ac97_warm_reset,
@@ -224,6 +225,7 @@ static const struct snd_soc_component_driver pxa_ac97_component = {
static int pxa2xx_ac97_dev_probe(struct platform_device *pdev)
{
int ret;
+ pxa2xx_audio_ops_t *pdata = pdev->dev.platform_data;
if (pdev->id != -1) {
dev_err(&pdev->dev, "PXA2xx has only one AC97 port.\n");
@@ -236,7 +238,9 @@ static int pxa2xx_ac97_dev_probe(struct platform_device *pdev)
return ret;
}
- ret = snd_soc_set_ac97_ops(&pxa2xx_ac97_ops);
+ ret = snd_ac97_controller_register(&pxa2xx_ac97_ops, &pdev->dev,
+ AC97_SLOTS_AVAILABLE_ALL,
+ pdata->codec_pdata);
if (ret != 0)
return ret;
@@ -251,7 +255,7 @@ static int pxa2xx_ac97_dev_probe(struct platform_device *pdev)
static int pxa2xx_ac97_dev_remove(struct platform_device *pdev)
{
snd_soc_unregister_component(&pdev->dev);
- snd_soc_set_ac97_ops(NULL);
+ snd_ac97_controller_unregister(&pdev->dev);
pxa2xx_ac97_hw_remove(pdev);
return 0;
}
--
2.1.4
Add support for the new ac97 bus model, where devices are automatically
discovered on AC-Links.
Signed-off-by: Robert Jarzmik <[email protected]>
---
sound/soc/codecs/Kconfig | 3 ++-
sound/soc/codecs/wm9713.c | 37 ++++++++++++++++++++++++++-----------
2 files changed, 28 insertions(+), 12 deletions(-)
diff --git a/sound/soc/codecs/Kconfig b/sound/soc/codecs/Kconfig
index c67667bb970f..c5871a299cfc 100644
--- a/sound/soc/codecs/Kconfig
+++ b/sound/soc/codecs/Kconfig
@@ -202,7 +202,7 @@ config SND_SOC_ALL_CODECS
select SND_SOC_WM9090 if I2C
select SND_SOC_WM9705 if SND_SOC_AC97_BUS
select SND_SOC_WM9712 if SND_SOC_AC97_BUS
- select SND_SOC_WM9713 if SND_SOC_AC97_BUS
+ select SND_SOC_WM9713 if (SND_SOC_AC97_BUS || SND_SOC_AC97_BUS_NEW)
help
Normally ASoC codec drivers are only built if a machine driver which
uses them is also built since they are only usable with a machine
@@ -1061,6 +1061,7 @@ config SND_SOC_WM9712
config SND_SOC_WM9713
tristate
select REGMAP_AC97
+ select AC97_BUS_COMPAT if AC97_BUS_NEW
# Amp
config SND_SOC_LM4857
diff --git a/sound/soc/codecs/wm9713.c b/sound/soc/codecs/wm9713.c
index e4301ddb1b84..2163232855cb 100644
--- a/sound/soc/codecs/wm9713.c
+++ b/sound/soc/codecs/wm9713.c
@@ -17,12 +17,15 @@
#include <linux/init.h>
#include <linux/slab.h>
+#include <linux/mfd/wm97xx.h>
#include <linux/module.h>
#include <linux/device.h>
#include <linux/regmap.h>
#include <sound/core.h>
#include <sound/pcm.h>
#include <sound/ac97_codec.h>
+#include <sound/ac97/codec.h>
+#include <sound/ac97/compat.h>
#include <sound/initval.h>
#include <sound/pcm_params.h>
#include <sound/tlv.h>
@@ -38,6 +41,7 @@ struct wm9713_priv {
u32 pll_in; /* PLL input frequency */
unsigned int hp_mixer[2];
struct mutex lock;
+ struct wm97xx_platform_data *mfd_pdata;
};
#define HPL_MIXER 0
@@ -1207,15 +1211,21 @@ static int wm9713_soc_probe(struct snd_soc_codec *codec)
struct wm9713_priv *wm9713 = snd_soc_codec_get_drvdata(codec);
struct regmap *regmap;
- wm9713->ac97 = snd_soc_new_ac97_codec(codec, WM9713_VENDOR_ID,
- WM9713_VENDOR_ID_MASK);
- if (IS_ERR(wm9713->ac97))
- return PTR_ERR(wm9713->ac97);
-
- regmap = regmap_init_ac97(wm9713->ac97, &wm9713_regmap_config);
- if (IS_ERR(regmap)) {
- snd_soc_free_ac97_codec(wm9713->ac97);
- return PTR_ERR(regmap);
+ if (wm9713->mfd_pdata) {
+ wm9713->ac97 = wm9713->mfd_pdata->ac97;
+ regmap = wm9713->mfd_pdata->regmap;
+ } else {
+#ifdef CONFIG_SND_SOC_AC97_BUS
+ wm9713->ac97 = snd_soc_new_ac97_codec(codec, WM9713_VENDOR_ID,
+ WM9713_VENDOR_ID_MASK);
+ if (IS_ERR(wm9713->ac97))
+ return PTR_ERR(wm9713->ac97);
+ regmap = regmap_init_ac97(wm9713->ac97, &wm9713_regmap_config);
+ if (IS_ERR(regmap)) {
+ snd_soc_free_ac97_codec(wm9713->ac97);
+ return PTR_ERR(regmap);
+ }
+#endif
}
snd_soc_codec_init_regmap(codec, regmap);
@@ -1230,8 +1240,12 @@ static int wm9713_soc_remove(struct snd_soc_codec *codec)
{
struct wm9713_priv *wm9713 = snd_soc_codec_get_drvdata(codec);
- snd_soc_codec_exit_regmap(codec);
- snd_soc_free_ac97_codec(wm9713->ac97);
+ if (!wm9713->mfd_pdata) {
+ snd_soc_codec_exit_regmap(codec);
+#ifdef CONFIG_SND_SOC_AC97_BUS
+ snd_soc_free_ac97_codec(wm9713->ac97);
+#endif
+ }
return 0;
}
@@ -1262,6 +1276,7 @@ static int wm9713_probe(struct platform_device *pdev)
mutex_init(&wm9713->lock);
+ wm9713->mfd_pdata = dev_get_platdata(&pdev->dev);
platform_set_drvdata(pdev, wm9713);
return snd_soc_register_codec(&pdev->dev,
--
2.1.4
wm97xx-core does several things in it initialization :
- touchscreen input device setup
- battery device creation
As the wm97xx is actually a multi-function device handling an audio
codec, a touchscreen, a gpio block and an ADC, reshape the probing to
isolate what is truly input/touchscreen specific from the remaining
part.
This is only code shuffling, there is no functional change.
Signed-off-by: Robert Jarzmik <[email protected]>
---
drivers/input/touchscreen/wm97xx-core.c | 193 ++++++++++++++++++--------------
1 file changed, 112 insertions(+), 81 deletions(-)
diff --git a/drivers/input/touchscreen/wm97xx-core.c b/drivers/input/touchscreen/wm97xx-core.c
index 83cf11312fd9..50a110e2988b 100644
--- a/drivers/input/touchscreen/wm97xx-core.c
+++ b/drivers/input/touchscreen/wm97xx-core.c
@@ -581,27 +581,82 @@ static void wm97xx_ts_input_close(struct input_dev *idev)
wm->codec->acc_enable(wm, 0);
}
-static int wm97xx_probe(struct device *dev)
+static int wm97xx_register_touch(struct wm97xx *wm)
{
- struct wm97xx *wm;
- struct wm97xx_pdata *pdata = dev_get_platdata(dev);
- int ret = 0, id = 0;
+ struct wm97xx_pdata *pdata = dev_get_platdata(wm->dev);
+ int ret;
- wm = kzalloc(sizeof(struct wm97xx), GFP_KERNEL);
- if (!wm)
+ wm->input_dev = devm_input_allocate_device(wm->dev);
+ if (wm->input_dev == NULL)
return -ENOMEM;
- mutex_init(&wm->codec_mutex);
- wm->dev = dev;
- dev_set_drvdata(dev, wm);
- wm->ac97 = to_ac97_t(dev);
+ /* set up touch configuration */
+ wm->input_dev->name = "wm97xx touchscreen";
+ wm->input_dev->phys = "wm97xx";
+ wm->input_dev->open = wm97xx_ts_input_open;
+ wm->input_dev->close = wm97xx_ts_input_close;
+
+ __set_bit(EV_ABS, wm->input_dev->evbit);
+ __set_bit(EV_KEY, wm->input_dev->evbit);
+ __set_bit(BTN_TOUCH, wm->input_dev->keybit);
+
+ input_set_abs_params(wm->input_dev, ABS_X, abs_x[0], abs_x[1],
+ abs_x[2], 0);
+ input_set_abs_params(wm->input_dev, ABS_Y, abs_y[0], abs_y[1],
+ abs_y[2], 0);
+ input_set_abs_params(wm->input_dev, ABS_PRESSURE, abs_p[0], abs_p[1],
+ abs_p[2], 0);
+
+ input_set_drvdata(wm->input_dev, wm);
+ wm->input_dev->dev.parent = wm->dev;
+
+ ret = input_register_device(wm->input_dev);
+ if (ret)
+ return ret;
+
+ /* register our extended touch device (for machine specific
+ * extensions) */
+ wm->touch_dev = platform_device_alloc("wm97xx-touch", -1);
+ if (!wm->touch_dev) {
+ ret = -ENOMEM;
+ goto touch_err;
+ }
+ platform_set_drvdata(wm->touch_dev, wm);
+ wm->touch_dev->dev.parent = wm->dev;
+ wm->touch_dev->dev.platform_data = pdata;
+ ret = platform_device_add(wm->touch_dev);
+ if (ret < 0)
+ goto touch_reg_err;
+
+ return 0;
+touch_reg_err:
+ platform_device_put(wm->touch_dev);
+touch_err:
+ input_unregister_device(wm->input_dev);
+ wm->input_dev = NULL;
+
+ return ret;
+}
+
+static void wm97xx_unregister_touch(struct wm97xx *wm)
+{
+ platform_device_unregister(wm->touch_dev);
+ input_unregister_device(wm->input_dev);
+ wm->input_dev = NULL;
+}
+
+static int _wm97xx_probe(struct wm97xx *wm)
+{
+ int id = 0;
+
+ mutex_init(&wm->codec_mutex);
+ dev_set_drvdata(wm->dev, wm);
/* check that we have a supported codec */
id = wm97xx_reg_read(wm, AC97_VENDOR_ID1);
if (id != WM97XX_ID1) {
- dev_err(dev, "Device with vendor %04x is not a wm97xx\n", id);
- ret = -ENODEV;
- goto alloc_err;
+ dev_err(wm->dev, "Device with vendor %04x is not a wm97xx\n", id);
+ return -ENODEV;
}
wm->id = wm97xx_reg_read(wm, AC97_VENDOR_ID2);
@@ -629,8 +684,7 @@ static int wm97xx_probe(struct device *dev)
default:
dev_err(wm->dev, "Support for wm97%02x not compiled in.\n",
wm->id & 0xff);
- ret = -ENODEV;
- goto alloc_err;
+ return -ENODEV;
}
/* set up physical characteristics */
@@ -644,79 +698,58 @@ static int wm97xx_probe(struct device *dev)
wm->gpio[4] = wm97xx_reg_read(wm, AC97_GPIO_STATUS);
wm->gpio[5] = wm97xx_reg_read(wm, AC97_MISC_AFE);
- wm->input_dev = input_allocate_device();
- if (wm->input_dev == NULL) {
- ret = -ENOMEM;
- goto alloc_err;
- }
-
- /* set up touch configuration */
- wm->input_dev->name = "wm97xx touchscreen";
- wm->input_dev->phys = "wm97xx";
- wm->input_dev->open = wm97xx_ts_input_open;
- wm->input_dev->close = wm97xx_ts_input_close;
-
- __set_bit(EV_ABS, wm->input_dev->evbit);
- __set_bit(EV_KEY, wm->input_dev->evbit);
- __set_bit(BTN_TOUCH, wm->input_dev->keybit);
-
- input_set_abs_params(wm->input_dev, ABS_X, abs_x[0], abs_x[1],
- abs_x[2], 0);
- input_set_abs_params(wm->input_dev, ABS_Y, abs_y[0], abs_y[1],
- abs_y[2], 0);
- input_set_abs_params(wm->input_dev, ABS_PRESSURE, abs_p[0], abs_p[1],
- abs_p[2], 0);
+ return wm97xx_register_touch(wm);
+}
- input_set_drvdata(wm->input_dev, wm);
- wm->input_dev->dev.parent = dev;
+static void wm97xx_remove_battery(struct wm97xx *wm)
+{
+ platform_device_put(wm->battery_dev);
+}
- ret = input_register_device(wm->input_dev);
- if (ret < 0)
- goto dev_alloc_err;
+static int wm97xx_add_battery(struct wm97xx *wm,
+ struct wm97xx_batt_pdata *pdata)
+{
+ int ret;
- /* register our battery device */
wm->battery_dev = platform_device_alloc("wm97xx-battery", -1);
- if (!wm->battery_dev) {
- ret = -ENOMEM;
- goto batt_err;
- }
+ if (!wm->battery_dev)
+ return -ENOMEM;
+
platform_set_drvdata(wm->battery_dev, wm);
- wm->battery_dev->dev.parent = dev;
- wm->battery_dev->dev.platform_data = pdata->batt_pdata;
+ wm->battery_dev->dev.parent = wm->dev;
+ wm->battery_dev->dev.platform_data = pdata;
ret = platform_device_add(wm->battery_dev);
- if (ret < 0)
- goto batt_reg_err;
+ if (ret)
+ wm97xx_remove_battery(wm);
- /* register our extended touch device (for machine specific
- * extensions) */
- wm->touch_dev = platform_device_alloc("wm97xx-touch", -1);
- if (!wm->touch_dev) {
- ret = -ENOMEM;
- goto touch_err;
- }
- platform_set_drvdata(wm->touch_dev, wm);
- wm->touch_dev->dev.parent = dev;
- wm->touch_dev->dev.platform_data = pdata;
- ret = platform_device_add(wm->touch_dev);
+ return ret;
+}
+
+static int wm97xx_probe(struct device *dev)
+{
+ struct wm97xx *wm;
+ int ret;
+ struct wm97xx_pdata *pdata = dev_get_platdata(dev);
+
+ wm = devm_kzalloc(dev, sizeof(struct wm97xx), GFP_KERNEL);
+ if (!wm)
+ return -ENOMEM;
+
+ wm->dev = dev;
+ wm->ac97 = to_ac97_t(dev);
+
+ ret = _wm97xx_probe(wm);
+ if (ret)
+ return ret;
+
+ ret = wm97xx_add_battery(wm, pdata->batt_pdata);
if (ret < 0)
- goto touch_reg_err;
+ goto batt_err;
return ret;
- touch_reg_err:
- platform_device_put(wm->touch_dev);
- touch_err:
- platform_device_del(wm->battery_dev);
- batt_reg_err:
- platform_device_put(wm->battery_dev);
- batt_err:
- input_unregister_device(wm->input_dev);
- wm->input_dev = NULL;
- dev_alloc_err:
- input_free_device(wm->input_dev);
- alloc_err:
- kfree(wm);
-
+batt_err:
+ wm97xx_unregister_touch(wm);
return ret;
}
@@ -724,10 +757,8 @@ static int wm97xx_remove(struct device *dev)
{
struct wm97xx *wm = dev_get_drvdata(dev);
- platform_device_unregister(wm->battery_dev);
- platform_device_unregister(wm->touch_dev);
- input_unregister_device(wm->input_dev);
- kfree(wm);
+ wm97xx_remove_battery(wm);
+ wm97xx_unregister_touch(wm);
return 0;
}
--
2.1.4
The WM9705, WM9712 and WM9713 are highly integrated codecs, with an
audio codec, DAC and ADC, GPIO unit and a touchscreen interface.
Historically the support was spread across drivers/input/touchscreen and
sound/soc/codecs. The sharing was done through ac97 bus sharing. This
model will not withstand the new AC97 bus model, where codecs are
discovered on runtime.
Signed-off-by: Robert Jarzmik <[email protected]>
---
drivers/mfd/Kconfig | 14 +++
drivers/mfd/Makefile | 1 +
drivers/mfd/wm97xx-core.c | 282 +++++++++++++++++++++++++++++++++++++++++++++
include/linux/mfd/wm97xx.h | 31 +++++
4 files changed, 328 insertions(+)
create mode 100644 drivers/mfd/wm97xx-core.c
create mode 100644 include/linux/mfd/wm97xx.h
diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
index c6df6442ba2b..2ac818127b0a 100644
--- a/drivers/mfd/Kconfig
+++ b/drivers/mfd/Kconfig
@@ -1597,6 +1597,20 @@ config MFD_WM8994
core support for the WM8994, in order to use the actual
functionaltiy of the device other drivers must be enabled.
+config MFD_WM97xx
+ tristate "Wolfson Microelectronics WM97xx"
+ select MFD_CORE
+ select REGMAP_AC97
+ select AC97_BUS_COMPAT if AC97_BUS_NEW
+ help
+
+ The WM9705, WM9712 and WM9713 is a highly integrated hi-fi CODEC
+ designed for smartphone applications. As well as audio functionality
+ it has on board GPIO and a touchscreen functionality which is
+ supported via the relevant subsystems. This driver provides core
+ support for the WM97xx, in order to use the actual functionaltiy of
+ the device other drivers must be enabled.
+
config MFD_STW481X
tristate "Support for ST Microelectronics STw481x"
depends on I2C && (ARCH_NOMADIK || COMPILE_TEST)
diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
index 9834e669d985..5c3534f4c7c3 100644
--- a/drivers/mfd/Makefile
+++ b/drivers/mfd/Makefile
@@ -69,6 +69,7 @@ obj-$(CONFIG_MFD_WM8350) += wm8350.o
obj-$(CONFIG_MFD_WM8350_I2C) += wm8350-i2c.o
wm8994-objs := wm8994-core.o wm8994-irq.o wm8994-regmap.o
obj-$(CONFIG_MFD_WM8994) += wm8994.o
+obj-$(CONFIG_MFD_WM97xx) += wm97xx-core.o
obj-$(CONFIG_TPS6105X) += tps6105x.o
obj-$(CONFIG_TPS65010) += tps65010.o
diff --git a/drivers/mfd/wm97xx-core.c b/drivers/mfd/wm97xx-core.c
new file mode 100644
index 000000000000..f2cd80354b4a
--- /dev/null
+++ b/drivers/mfd/wm97xx-core.c
@@ -0,0 +1,282 @@
+/*
+ * Wolfson WM97xx -- Core device
+ *
+ * Copyright (C) 2016 Robert Jarzmik
+ *
+ * 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.
+ *
+ * Features:
+ * - an AC97 audio codec
+ * - a touchscreen driver
+ * - a GPIO block
+ */
+
+#include <linux/module.h>
+
+#include <linux/device.h>
+#include <linux/mfd/core.h>
+#include <linux/mfd/wm97xx.h>
+#include <linux/wm97xx.h>
+#include <linux/regmap.h>
+#include <linux/slab.h>
+#include <sound/ac97/codec.h>
+#include <sound/ac97/compat.h>
+
+#define WM9705_VENDOR_ID 0x574d4c05
+#define WM9712_VENDOR_ID 0x574d4c12
+#define WM9713_VENDOR_ID 0x574d4c13
+#define WM97xx_VENDOR_ID_MASK 0xffffffff
+
+struct wm97xx_priv {
+ struct regmap *regmap;
+ struct snd_ac97 *ac97;
+ struct device *dev;
+};
+
+static bool wm97xx_readable_reg(struct device *dev, unsigned int reg)
+{
+ switch (reg) {
+ case AC97_RESET ... AC97_PCM_SURR_DAC_RATE:
+ case AC97_PCM_LR_ADC_RATE:
+ case AC97_CENTER_LFE_MASTER:
+ case AC97_SPDIF ... AC97_LINE1_LEVEL:
+ case AC97_GPIO_CFG ... 0x5c:
+ case AC97_CODEC_CLASS_REV ... AC97_PCI_SID:
+ case 0x74 ... AC97_VENDOR_ID2:
+ return true;
+ default:
+ return false;
+ }
+}
+
+static bool wm97xx_writeable_reg(struct device *dev, unsigned int reg)
+{
+ switch (reg) {
+ case AC97_VENDOR_ID1:
+ case AC97_VENDOR_ID2:
+ return false;
+ default:
+ return wm97xx_readable_reg(dev, reg);
+ }
+}
+
+static const struct reg_default wm97xx_reg_defaults[] = {
+};
+
+static const struct regmap_config wm9705_regmap_config = {
+ .reg_bits = 16,
+ .reg_stride = 2,
+ .val_bits = 16,
+ .max_register = 0x7e,
+ .cache_type = REGCACHE_RBTREE,
+
+ .reg_defaults = wm97xx_reg_defaults,
+ .num_reg_defaults = ARRAY_SIZE(wm97xx_reg_defaults),
+ .volatile_reg = regmap_ac97_default_volatile,
+ .readable_reg = wm97xx_readable_reg,
+ .writeable_reg = wm97xx_writeable_reg,
+};
+
+static const struct regmap_config wm9712_regmap_config = {
+ .reg_bits = 16,
+ .reg_stride = 2,
+ .val_bits = 16,
+ .max_register = 0x7e,
+ .cache_type = REGCACHE_RBTREE,
+
+ .reg_defaults = wm97xx_reg_defaults,
+ .num_reg_defaults = ARRAY_SIZE(wm97xx_reg_defaults),
+ .volatile_reg = regmap_ac97_default_volatile,
+ .readable_reg = wm97xx_readable_reg,
+ .writeable_reg = wm97xx_writeable_reg,
+};
+
+static int wm9705_register(struct wm97xx_priv *wm97xx)
+{
+ return 0;
+}
+
+static int wm9712_register(struct wm97xx_priv *wm97xx)
+{
+ return 0;
+}
+
+static const struct reg_default wm9713_reg_defaults[] = {
+ { 0x02, 0x8080 }, /* Speaker Output Volume */
+ { 0x04, 0x8080 }, /* Headphone Output Volume */
+ { 0x06, 0x8080 }, /* Out3/OUT4 Volume */
+ { 0x08, 0xc880 }, /* Mono Volume */
+ { 0x0a, 0xe808 }, /* LINEIN Volume */
+ { 0x0c, 0xe808 }, /* DAC PGA Volume */
+ { 0x0e, 0x0808 }, /* MIC PGA Volume */
+ { 0x10, 0x00da }, /* MIC Routing Control */
+ { 0x12, 0x8000 }, /* Record PGA Volume */
+ { 0x14, 0xd600 }, /* Record Routing */
+ { 0x16, 0xaaa0 }, /* PCBEEP Volume */
+ { 0x18, 0xaaa0 }, /* VxDAC Volume */
+ { 0x1a, 0xaaa0 }, /* AUXDAC Volume */
+ { 0x1c, 0x0000 }, /* Output PGA Mux */
+ { 0x1e, 0x0000 }, /* DAC 3D control */
+ { 0x20, 0x0f0f }, /* DAC Tone Control*/
+ { 0x22, 0x0040 }, /* MIC Input Select & Bias */
+ { 0x24, 0x0000 }, /* Output Volume Mapping & Jack */
+ { 0x26, 0x7f00 }, /* Powerdown Ctrl/Stat*/
+ { 0x28, 0x0405 }, /* Extended Audio ID */
+ { 0x2a, 0x0410 }, /* Extended Audio Start/Ctrl */
+ { 0x2c, 0xbb80 }, /* Audio DACs Sample Rate */
+ { 0x2e, 0xbb80 }, /* AUXDAC Sample Rate */
+ { 0x32, 0xbb80 }, /* Audio ADCs Sample Rate */
+ { 0x36, 0x4523 }, /* PCM codec control */
+ { 0x3a, 0x2000 }, /* SPDIF control */
+ { 0x3c, 0xfdff }, /* Powerdown 1 */
+ { 0x3e, 0xffff }, /* Powerdown 2 */
+ { 0x40, 0x0000 }, /* General Purpose */
+ { 0x42, 0x0000 }, /* Fast Power-Up Control */
+ { 0x44, 0x0080 }, /* MCLK/PLL Control */
+ { 0x46, 0x0000 }, /* MCLK/PLL Control */
+
+ { 0x4c, 0xfffe }, /* GPIO Pin Configuration */
+ { 0x4e, 0xffff }, /* GPIO Pin Polarity / Type */
+ { 0x50, 0x0000 }, /* GPIO Pin Sticky */
+ { 0x52, 0x0000 }, /* GPIO Pin Wake-Up */
+ /* GPIO Pin Status */
+ { 0x56, 0xfffe }, /* GPIO Pin Sharing */
+ { 0x58, 0x4000 }, /* GPIO PullUp/PullDown */
+ { 0x5a, 0x0000 }, /* Additional Functions 1 */
+ { 0x5c, 0x0000 }, /* Additional Functions 2 */
+ { 0x60, 0xb032 }, /* ALC Control */
+ { 0x62, 0x3e00 }, /* ALC / Noise Gate Control */
+ { 0x64, 0x0000 }, /* AUXDAC input control */
+ { 0x74, 0x0000 }, /* Digitiser Reg 1 */
+ { 0x76, 0x0006 }, /* Digitiser Reg 2 */
+ { 0x78, 0x0001 }, /* Digitiser Reg 3 */
+ { 0x7a, 0x0000 }, /* Digitiser Read Back */
+};
+
+static const struct regmap_config wm9713_regmap_config = {
+ .reg_bits = 16,
+ .reg_stride = 2,
+ .val_bits = 16,
+ .max_register = 0x7e,
+ .cache_type = REGCACHE_RBTREE,
+
+ .reg_defaults = wm9713_reg_defaults,
+ .num_reg_defaults = ARRAY_SIZE(wm9713_reg_defaults),
+ .volatile_reg = regmap_ac97_default_volatile,
+ .readable_reg = wm97xx_readable_reg,
+ .writeable_reg = wm97xx_writeable_reg,
+};
+
+static int wm9713_register(struct wm97xx_priv *wm97xx,
+ struct wm97xx_pdata *pdata)
+{
+ struct wm97xx_platform_data codec_pdata;
+ const struct mfd_cell cells[] = {
+ {
+ .name = "wm9713-codec",
+ .platform_data = &codec_pdata,
+ .pdata_size = sizeof(codec_pdata),
+ },
+ {
+ .name = "wm97xx-ts",
+ .platform_data = &codec_pdata,
+ .pdata_size = sizeof(codec_pdata),
+ },
+ };
+
+ codec_pdata.ac97 = wm97xx->ac97;
+ codec_pdata.regmap = devm_regmap_init_ac97(wm97xx->ac97,
+ &wm9713_regmap_config);
+ codec_pdata.batt_pdata = pdata->batt_pdata;
+ if (IS_ERR(codec_pdata.regmap))
+ return PTR_ERR(codec_pdata.regmap);
+
+ return devm_mfd_add_devices(wm97xx->dev, -1, cells,
+ ARRAY_SIZE(cells), NULL, 0, NULL);
+}
+
+static int wm97xx_ac97_probe(struct ac97_codec_device *adev)
+{
+ struct wm97xx_priv *wm97xx;
+ int ret;
+ void *pdata = snd_ac97_codec_get_platdata(adev);
+
+ wm97xx = devm_kzalloc(ac97_codec_dev2dev(adev),
+ sizeof(*wm97xx), GFP_KERNEL);
+ if (!wm97xx)
+ return -ENOMEM;
+
+ wm97xx->dev = ac97_codec_dev2dev(adev);
+ wm97xx->ac97 = snd_ac97_compat_alloc(adev);
+ if (IS_ERR(wm97xx->ac97))
+ return PTR_ERR(wm97xx->ac97);
+
+
+ ac97_set_drvdata(adev, wm97xx);
+ dev_info(wm97xx->dev, "wm97xx core found, id=0x%x\n",
+ adev->vendor_id);
+
+ switch (adev->vendor_id) {
+ case WM9705_VENDOR_ID:
+ ret = wm9705_register(wm97xx);
+ break;
+ case WM9712_VENDOR_ID:
+ ret = wm9712_register(wm97xx);
+ break;
+ case WM9713_VENDOR_ID:
+ ret = wm9713_register(wm97xx, pdata);
+ break;
+ default:
+ ret = -ENODEV;
+ }
+
+ if (ret)
+ snd_ac97_compat_release(wm97xx->ac97);
+
+ return ret;
+}
+
+static int wm97xx_ac97_remove(struct ac97_codec_device *adev)
+{
+ struct wm97xx_priv *wm97xx = ac97_get_drvdata(adev);
+
+ snd_ac97_compat_release(wm97xx->ac97);
+
+ return 0;
+}
+
+static const struct ac97_id wm97xx_ac97_ids[] = {
+ { .id = WM9705_VENDOR_ID, .mask = WM97xx_VENDOR_ID_MASK },
+ { .id = WM9712_VENDOR_ID, .mask = WM97xx_VENDOR_ID_MASK },
+ { .id = WM9713_VENDOR_ID, .mask = WM97xx_VENDOR_ID_MASK },
+ { }
+};
+
+static struct ac97_codec_driver wm97x3_ac97_driver = {
+ .driver = {
+ .name = "wm97xx-core",
+ },
+ .probe = wm97xx_ac97_probe,
+ .remove = wm97xx_ac97_remove,
+ .id_table = wm97xx_ac97_ids,
+};
+
+static int __init wm97xx_module_init(void)
+{
+ return snd_ac97_codec_driver_register(&wm97x3_ac97_driver);
+}
+module_init(wm97xx_module_init);
+
+static void __exit wm97xx_module_exit(void)
+{
+ snd_ac97_codec_driver_unregister(&wm97x3_ac97_driver);
+}
+module_exit(wm97xx_module_exit);
+
+MODULE_DESCRIPTION("WM9712, WM9713 core driver");
+MODULE_AUTHOR("Robert Jarzmik <[email protected]>");
+MODULE_LICENSE("GPL");
+
diff --git a/include/linux/mfd/wm97xx.h b/include/linux/mfd/wm97xx.h
new file mode 100644
index 000000000000..627322f14d48
--- /dev/null
+++ b/include/linux/mfd/wm97xx.h
@@ -0,0 +1,31 @@
+/*
+ * wm97xx client interface
+ *
+ * Copyright (C) 2016 Robert Jarzmik
+ *
+ * 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.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ */
+
+#ifndef __LINUX_MFD_WM97XX_H
+#define __LINUX_MFD_WM97XX_H
+
+struct regmap;
+struct wm97xx_batt_pdata;
+struct snd_ac97;
+
+struct wm97xx_platform_data {
+ struct snd_ac97 *ac97;
+ struct regmap *regmap;
+ struct wm97xx_batt_pdata *batt_pdata;
+};
+
+
+#endif
--
2.1.4
On Wed, Oct 26, 2016 at 09:41:46PM +0200, Robert Jarzmik wrote:
> The WM9705, WM9712 and WM9713 are highly integrated codecs, with an
> audio codec, DAC and ADC, GPIO unit and a touchscreen interface.
>
> Historically the support was spread across drivers/input/touchscreen and
> sound/soc/codecs. The sharing was done through ac97 bus sharing. This
> model will not withstand the new AC97 bus model, where codecs are
> discovered on runtime.
>
> Signed-off-by: Robert Jarzmik <[email protected]>
> ---
Acked-by: Charles Keepax <[email protected]>
Thanks,
Charles
On Wed, Oct 26, 2016 at 09:41:47PM +0200, Robert Jarzmik wrote:
> This adds support for the new AC97 bus code, which discovers the devices
> rather than uses platform data.
>
> As part of this discovery, it enables a multi-function device wm97xx,
> which supports touchscreen, battery, ADC and an audio codec. This patch
> adds the code to bind the touchscreen "cell" as the touchscreen driver.
>
> This was tested on the pxa architecture with a pxa270 + wm9713 + the
> mioa701 touchscreen.
>
> Signed-off-by: Robert Jarzmik <[email protected]>
> ---
Acked-by: Charles Keepax <[email protected]>
Thanks,
Charles
On Wed, Oct 26, 2016 at 09:41:42PM +0200, Robert Jarzmik wrote:
> Add support for the new ac97 bus model, where devices are automatically
> discovered on AC-Links.
>
> Signed-off-by: Robert Jarzmik <[email protected]>
> ---
Acked-by: Charles Keepax <[email protected]>
Thanks,
Charles
On Wed, Oct 26, 2016 at 09:41:45PM +0200, Robert Jarzmik wrote:
> wm97xx-core does several things in it initialization :
> - touchscreen input device setup
> - battery device creation
>
> As the wm97xx is actually a multi-function device handling an audio
> codec, a touchscreen, a gpio block and an ADC, reshape the probing to
> isolate what is truly input/touchscreen specific from the remaining
> part.
>
> This is only code shuffling, there is no functional change.
>
> Signed-off-by: Robert Jarzmik <[email protected]>
> ---
> drivers/input/touchscreen/wm97xx-core.c | 193 ++++++++++++++++++--------------
> 1 file changed, 112 insertions(+), 81 deletions(-)
>
> diff --git a/drivers/input/touchscreen/wm97xx-core.c b/drivers/input/touchscreen/wm97xx-core.c
> index 83cf11312fd9..50a110e2988b 100644
<snip>
> +static void wm97xx_remove_battery(struct wm97xx *wm)
> +{
> + platform_device_put(wm->battery_dev);
> +}
<snip>
> @@ -724,10 +757,8 @@ static int wm97xx_remove(struct device *dev)
> {
> struct wm97xx *wm = dev_get_drvdata(dev);
>
> - platform_device_unregister(wm->battery_dev);
> - platform_device_unregister(wm->touch_dev);
> - input_unregister_device(wm->input_dev);
> - kfree(wm);
> + wm97xx_remove_battery(wm);
The commit message says this is just shifting code around but the
platform_device_unregister for the battery_dev seems to have
turned into a platform_device_put here.
Thanks,
Charles
Charles Keepax <[email protected]> writes:
> On Wed, Oct 26, 2016 at 09:41:45PM +0200, Robert Jarzmik wrote:
>> diff --git a/drivers/input/touchscreen/wm97xx-core.c b/drivers/input/touchscreen/wm97xx-core.c
>> index 83cf11312fd9..50a110e2988b 100644
> <snip>
>> +static void wm97xx_remove_battery(struct wm97xx *wm)
>> +{
>> + platform_device_put(wm->battery_dev);
>> +}
> <snip>
>> @@ -724,10 +757,8 @@ static int wm97xx_remove(struct device *dev)
>> {
>> struct wm97xx *wm = dev_get_drvdata(dev);
>>
>> - platform_device_unregister(wm->battery_dev);
>> - platform_device_unregister(wm->touch_dev);
>> - input_unregister_device(wm->input_dev);
>> - kfree(wm);
>> + wm97xx_remove_battery(wm);
>
> The commit message says this is just shifting code around but the
> platform_device_unregister for the battery_dev seems to have
> turned into a platform_device_put here.
Thanks for spotting that, it's clearly a defect in the patch.
That implies a v2 at least for this one.
Cheers.
--
Robert
Robert Jarzmik <[email protected]> writes:
> Switch to the new ac97 bus support in sound/ac97 instead of the legacy
> snd_ac97 one.
>
> Signed-off-by: Robert Jarzmik <[email protected]>
I realized this one impacts sound/arm/pxa2xx-ac97.c.
This deserves a v2, with this patch being split into 2 pieces :
- one for pxa2xx-ac97-lib.*
- one which will be the true switch to the new ac97 bus.
Cheers.
--
Robert
Robert Jarzmik <[email protected]> writes:
> Robert Jarzmik <[email protected]> writes:
>
>> Switch to the new ac97 bus support in sound/ac97 instead of the legacy
>> snd_ac97 one.
>>
>> Signed-off-by: Robert Jarzmik <[email protected]>
> I realized this one impacts sound/arm/pxa2xx-ac97.c.
> This deserves a v2, with this patch being split into 2 pieces :
> - one for pxa2xx-ac97-lib.*
> - one which will be the true switch to the new ac97 bus.
And as another consequence, all pxa ac97 based codecs should be converted. So
far, machine drivers in sound/soc/pxa/*.c using ac97 all use either wm9705,
wm9712 or wm9713.
Therefore for this patch to be usable, I must also convert wm9705 and wm9712.
Cheers.
--
Robert
On 10/26/2016 09:41 PM, Robert Jarzmik wrote:
> AC97 is a bus for sound usage. It enables for a AC97 AC-Link to link one
> controller to 0 to 4 AC97 codecs.
>
> The goal of this new implementation is to implement a device/driver
> model for AC97, with an automatic scan of the bus and automatic
> discovery of AC97 codec devices.
>
Good work, a couple of comments inline.
[...]
> diff --git a/include/sound/ac97/codec.h b/include/sound/ac97/codec.h
> new file mode 100644
> index 000000000000..8901c1200522
> --- /dev/null
> +++ b/include/sound/ac97/codec.h
> @@ -0,0 +1,115 @@
> +/*
> + * Copyright (C) 2016 Robert Jarzmik <[email protected]>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +#ifndef __SOUND_AC97_CODEC2_H
> +#define __SOUND_AC97_CODEC2_H
> +
> +#include <linux/device.h>
> +
> +#define AC97_ID(vendor_id1, vendor_id2) \
> + ((((vendor_id1) & 0xffff) << 16) | ((vendor_id2) & 0xffff))
> +#define AC97_DRIVER_ID(vendor_id1, vendor_id2, mask_id1, mask_id2, _data) \
> + { .id = (((vendor_id1) & 0xffff) << 16) | ((vendor_id2) & 0xffff), \
> + .mask = (((mask_id1) & 0xffff) << 16) | ((mask_id2) & 0xffff), \
> + .data = (_data) }
> +
> +#define to_ac97_device(d) container_of(d, struct ac97_codec_device, dev)
> +#define to_ac97_driver(d) container_of(d, struct ac97_codec_driver, driver)
In my opinion these should be inline functions rather than macros as that
generates much more legible compiler errors e.g. in case there is a type
mismatch.
[...]
> +struct ac97_codec_driver {
> + struct device_driver driver;
> + int (*probe)(struct ac97_codec_device *);
> + int (*remove)(struct ac97_codec_device *);
> + int (*suspend)(struct ac97_codec_device *);
> + int (*resume)(struct ac97_codec_device *);
> + void (*shutdown)(struct ac97_codec_device *);
The suspend, resume and shutdown callbacks are never used. Which is good,
since all new frameworks should use dev_pm_ops. Just drop the from the struct.
> + const struct ac97_id *id_table;
> +};
[...]
> diff --git a/include/sound/ac97/controller.h b/include/sound/ac97/controller.h
> new file mode 100644
> index 000000000000..5ff59bd7e324
> --- /dev/null
> +++ b/include/sound/ac97/controller.h
> @@ -0,0 +1,85 @@
> +/*
> + * Copyright (C) 2016 Robert Jarzmik <[email protected]>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +#ifndef AC97_CONTROLLER_H
> +#define AC97_CONTROLLER_H
> +
> +#include <linux/list.h>
> +
> +#define AC97_BUS_MAX_CODECS 4
> +#define AC97_SLOTS_AVAILABLE_ALL 0xf
> +
> +struct device;
> +
> +/**
> + * struct ac97_controller - The AC97 controller of the AC-Link
> + * @ops: the AC97 operations.
> + * @controllers: linked list of all existing controllers.
> + * @dev: the device providing the AC97 controller.
> + * @slots_available: the mask of accessible/scanable codecs.
> + * @codecs: the 4 possible AC97 codecs (NULL if none found).
> + * @codecs_pdata: platform_data for each codec (NULL if no pdata).
> + *
> + * This structure is internal to AC97 bus, and should not be used by the
> + * controllers themselves, excepting for using @dev.
> + */
> +struct ac97_controller {
> + const struct ac97_controller_ops *ops;
> + struct list_head controllers;
> + struct device *dev;
I'd make the controller itself a struct dev, rather than just having the
pointer to the parent. This is more idiomatic and matches what other
subsystems do. It has several advantages, you get proper refcounting of your
controller structure, the controller gets its own sysfs directory where the
CODECs appear as children, you don't need the manual sysfs attribute
creation and removal in ac97_controler_{un,}register().
> + unsigned short slots_available;
> + struct ac97_codec_device *codecs[AC97_BUS_MAX_CODECS];
If you make the controller a struct dev you can also remove this, since the
device driver core tracks the children of a device.
> + void *codecs_pdata[AC97_BUS_MAX_CODECS];
> +};
> +
> +/**
> + * struct ac97_controller_ops - The AC97 operations
> + * @reset: Cold reset of the AC97 AC-Link.
> + * @warm_reset: Warm reset of the AC97 AC-Link.
> + * @read: Read of a single AC97 register.
> + * Returns the register value or a negative error code.
> + * @write: Write of a single AC97 register.
> + * @wait: Wait for the current AC97 operation to finish (might be NULL).
> + * @init: Initialization of the AC97 AC-Link (might be NULL).
> + *
> + * These are the basic operation an AC97 controller must provide for an AC97
> + * access functions. Amongst these, all but the last 2 are mandatory.
> + * The slot number is also known as the AC97 codec number, between 0 and 3.
> + */
> +struct ac97_controller_ops {
> + void (*reset)(struct ac97_controller *adrv);
> + void (*warm_reset)(struct ac97_controller *adrv);
> + int (*write)(struct ac97_controller *adrv, int slot,
> + unsigned short reg, unsigned short val);
> + int (*read)(struct ac97_controller *adrv, int slot, unsigned short reg);
> + void (*wait)(struct ac97_controller *adrv, int slot);
> + void (*init)(struct ac97_controller *adrv, int slot);
Neither wait nor init are ever used.
> +};
> +
[...]
> +/*
> + * Protects ac97_controllers and each ac97_controller structure.
> + */
> +static DEFINE_MUTEX(ac97_controllers_mutex);
> +static LIST_HEAD(ac97_controllers);
> +
> +static struct bus_type ac97_bus_type;
> +
> +static struct ac97_codec_device *
> +ac97_codec_find(struct ac97_controller *ac97_ctrl, int codec_num)
unsigned int codec_num
> +{
> + if ((codec_num < 0) || (codec_num >= AC97_BUS_MAX_CODECS))
> + return ERR_PTR(-ERANGE);
I'd make this EINVAL.
> +
> + return ac97_ctrl->codecs[codec_num];
> +}
> +
> +static void ac97_codec_release(struct device *dev)
> +{
> + struct ac97_codec_device *adev;
> + struct ac97_controller *ac97_ctrl;
> +
> + adev = container_of(dev, struct ac97_codec_device, dev);
to_ac97_device()
> + ac97_ctrl = adev->ac97_ctrl;
> + ac97_ctrl->codecs[adev->num] = NULL;
> + sysfs_remove_link(&dev->kobj, "ac97_controller");
> + kfree(adev);
> +}
> +
> +static int ac97_codec_add(struct ac97_controller *ac97_ctrl, int idx,
> + unsigned int vendor_id)
> +{
> + struct ac97_codec_device *codec;
> + char *codec_name;
> + int ret;
> +
> + codec = kzalloc(sizeof(*codec), GFP_KERNEL);
> + if (!codec)
> + return -ENOMEM;
> + ac97_ctrl->codecs[idx] = codec;
> + codec->vendor_id = vendor_id;
> + codec->dev.release = ac97_codec_release;
> + codec->dev.bus = &ac97_bus_type;
> + codec->dev.parent = ac97_ctrl->dev;
> + codec->num = idx;
> + codec->ac97_ctrl = ac97_ctrl;
> +
> + codec_name = kasprintf(GFP_KERNEL, "%s:%d", dev_name(ac97_ctrl->dev),
> + idx);
> + codec->dev.init_name = codec_name;
init_name is only for statically allocated devices. Use dev_set_name(dev,
...). No need for kasprintf() either as dev_set_name() takes a format string.
For this you need to split device_register into device_initialize() and
device_add(). But usually that is what you want anyway.
> +
> + ret = device_register(&codec->dev);
> + kfree(codec_name);
> + if (ret)
> + goto err_free_codec;
> +
> + ret = sysfs_create_link(&codec->dev.kobj, &ac97_ctrl->dev->kobj,
> + "ac97_controller");
Since the CODEC is a child of the controller this should not be necessary as
this just points one directory up. It's like `ln -s .. parent`
> + if (ret)
> + goto err_unregister_device;
> +
> + return 0;
> +err_unregister_device:
> + put_device(&codec->dev);
> +err_free_codec:
> + kfree(codec);
Since the struct is reference counted, the freeing is done in the release
callback and this leads to a double free.
> + ac97_ctrl->codecs[idx] = NULL;
> +
> + return ret;
> +}
[...]
> +/**
> + * snd_ac97_codec_driver_register - register an AC97 codec driver
> + * @dev: AC97 driver codec to register
> + *
> + * Register an AC97 codec driver to the ac97 bus driver, aka. the AC97 digital
> + * controller.
> + *
> + * Returns 0 on success or error code
> + */
> +int snd_ac97_codec_driver_register(struct ac97_codec_driver *drv)
> +{
> + int ret;
> +
> + drv->driver.bus = &ac97_bus_type;
> +
> + ret = driver_register(&drv->driver);
> + if (!ret)
> + ac97_rescan_all_controllers();
Rescanning the bus when a new codec driver is registered should not be
neccessary. The bus is scanned once when the controller is registered, this
creates the device. The device driver core will take care of binding the
device to the driver, if the driver is registered after thed evice.
> +
> + return ret;
> +}
> +EXPORT_SYMBOL(snd_ac97_codec_driver_register);
> +
[...]
> +static int ac97_ctrl_codecs_unregister(struct ac97_controller *ac97_ctrl)
> +{
> + int i;
> +
> + for (i = 0; i < AC97_BUS_MAX_CODECS; i++)
> + if (ac97_ctrl->codecs[i])
> + put_device(&ac97_ctrl->codecs[i]->dev);
This should be device_unregister() to match the device_register() in
ac97_codec_add().
> +
> + return 0;
> +}
> +
> +static ssize_t cold_reset_store(struct device *dev,
> + struct device_attribute *attr, const char *buf,
> + size_t len)
> +{
> + struct ac97_controller *ac97_ctrl = ac97_ctrl_find(dev);
> +
> + if (!dev)
> + return -ENODEV;
dev is never NULL here. And for the ac97_ctrl there is a race condition. It
could be unregistered and freed after ac97_ctrl_find() returned sucessfully,
but before ac97_ctrl->ops is used.
> +
> + ac97_ctrl->ops->reset(ac97_ctrl);
> + return len;
> +}
> +static DEVICE_ATTR_WO(cold_reset);
> +
> +static ssize_t warm_reset_store(struct device *dev,
> + struct device_attribute *attr, const char *buf,
> + size_t len)
> +{
> + struct ac97_controller *ac97_ctrl = ac97_ctrl_find(dev);
> +
> + if (!dev)
> + return -ENODEV;
Same here.
> +
> + ac97_ctrl->ops->warm_reset(ac97_ctrl);
> + return len;
> +}
> +static DEVICE_ATTR_WO(warm_reset);
> +
> +static struct attribute *ac97_controller_device_attrs[] = {
> + &dev_attr_cold_reset.attr,
> + &dev_attr_warm_reset.attr,
> + NULL
> +};
This adds new userspace ABI that is not documented at the moment.
> +
> +static const struct attribute_group ac97_controller_attr_group = {
> + .name = "ac97_operations",
> + .attrs = ac97_controller_device_attrs,
> +};
> +
> +/**
> + * snd_ac97_controller_register - register an ac97 controller
> + * @ops: the ac97 bus operations
> + * @dev: the device providing the ac97 DC function
> + * @slots_available: mask of the ac97 codecs that can be scanned and probed
> + * bit0 => codec 0, bit1 => codec 1 ... bit 3 => codec 3
> + *
> + * Register a digital controller which can control up to 4 ac97 codecs. This is
> + * the controller side of the AC97 AC-link, while the slave side are the codecs.
> + *
> + * Returns 0 upon success, negative value upon error
> + */
> +int snd_ac97_controller_register(const struct ac97_controller_ops *ops,
> + struct device *dev,
> + unsigned short slots_available,
> + void **codecs_pdata)
In my opinion this should return a handle to a ac97 controller which can
then be passed to snd_ac97_controller_unregister(). This is in my opinion
the better approach rather than looking up the controller by parent device.
> +{
> + struct ac97_controller *ac97_ctrl;
> + int ret, i;
> +
> + ac97_ctrl = kzalloc(sizeof(*ac97_ctrl), GFP_KERNEL);
> + if (!ac97_ctrl)
> + return -ENOMEM;
> +
> + for (i = 0; i < AC97_BUS_MAX_CODECS && codecs_pdata; i++)
> + ac97_ctrl->codecs_pdata[i] = codecs_pdata[i];
> +
> + ret = sysfs_create_group(&dev->kobj, &ac97_controller_attr_group);
> + if (ret)
> + return ret;
> +
> + mutex_lock(&ac97_controllers_mutex);
> + ac97_ctrl->ops = ops;
> + ac97_ctrl->slots_available = slots_available;
> + ac97_ctrl->dev = dev;
> + list_add(&ac97_ctrl->controllers, &ac97_controllers);
Stricly speeaking only the list_add needs to be protected.
> + mutex_unlock(&ac97_controllers_mutex);
> +
> + ac97_bus_reset(ac97_ctrl);
> + ac97_bus_scan(ac97_ctrl);
> +
> + return 0;
> +}
> +EXPORT_SYMBOL(snd_ac97_controller_register);
> +
> +/**
> + * snd_ac97_controller_unregister - unregister an ac97 controller
> + * @dev: the device previously provided to ac97_controller_register()
> + *
> + * Returns 0 on success, negative upon error
Unregister must not be able to fail. Hotunplug is one of the core concepts
of the device driver model and there is really nothing that can be done to
prevent a device from disappearing, so there is no sensible way of handling
the error (and your pxa driver modifications simply ignore it as well).
This also means the framework needs to cope with the case where the
controller is removed and the CODEC devices are still present. All future
operations should return -ENODEV in that case.
> + */
> +int snd_ac97_controller_unregister(struct device *dev)
> +{
> + struct ac97_controller *ac97_ctrl;
> + int ret = -ENODEV, i;
> +
> + mutex_lock(&ac97_controllers_mutex);
> + ac97_ctrl = ac97_ctrl_find(dev);
> + if (ac97_ctrl) {
> + ret = 0;
> + for (i = 0; i < AC97_BUS_MAX_CODECS; i++)
> + if (ac97_ctrl->codecs[i] &&
> + device_is_registered(&ac97_ctrl->codecs[i]->dev))
> + ret = -EBUSY;
> + if (!ret)
> + ret = ac97_ctrl_codecs_unregister(ac97_ctrl);
> + if (!ret) {
> + list_del(&ac97_ctrl->controllers);
> + sysfs_remove_group(&dev->kobj,
> + &ac97_controller_attr_group);
> + }
> + }
> + mutex_unlock(&ac97_controllers_mutex);
> +
> + return ret;
> +}
> +EXPORT_SYMBOL(snd_ac97_controller_unregister);
[...]
> +static struct bus_type ac97_bus_type = {
> + .name = "ac97",
> + .dev_attrs = ac97_dev_attrs,
dev_attrs is deprecated in favor of dev_groups (See commit 880ffb5c6).
> + .match = ac97_bus_match,
> + .pm = &ac97_pm,
> + .probe = ac97_bus_probe,
> + .remove = ac97_bus_remove,
> +};
> +
> +static int __init ac97_bus_init(void)
> +{
> + return bus_register(&ac97_bus_type);
> +}
> +subsys_initcall(ac97_bus_init);
> +
> +MODULE_LICENSE("GPL");
> +MODULE_AUTHOR("Robert Jarzmik <[email protected]>");
> diff --git a/sound/ac97/codec.c b/sound/ac97/codec.c
> new file mode 100644
> index 000000000000..a835f03744bf
> --- /dev/null
> +++ b/sound/ac97/codec.c
> @@ -0,0 +1,15 @@
> +/*
> + * Copyright (C) 2016 Robert Jarzmik <[email protected]>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +
> +#include <sound/ac97_codec.h>
> +#include <sound/ac97/codec.h>
> +#include <sound/ac97/controller.h>
> +#include <linux/device.h>
> +#include <linux/slab.h>
> +#include <sound/soc.h> /* For compat_ac97_* */
> +
I'm not sure I understand what this file does.
[...]
Lars-Peter Clausen <[email protected]> writes:
> On 10/26/2016 09:41 PM, Robert Jarzmik wrote:
>> +#define to_ac97_device(d) container_of(d, struct ac97_codec_device, dev)
>> +#define to_ac97_driver(d) container_of(d, struct ac97_codec_driver, driver)
>
> In my opinion these should be inline functions rather than macros as that
> generates much more legible compiler errors e.g. in case there is a type
> mismatch.
Sure, why not.
>> +struct ac97_codec_driver {
>> + struct device_driver driver;
>> + int (*probe)(struct ac97_codec_device *);
>> + int (*remove)(struct ac97_codec_device *);
>> + int (*suspend)(struct ac97_codec_device *);
>> + int (*resume)(struct ac97_codec_device *);
>> + void (*shutdown)(struct ac97_codec_device *);
>
> The suspend, resume and shutdown callbacks are never used. Which is good,
> since all new frameworks should use dev_pm_ops. Just drop the from the struct.
Ok.
>> +/**
>> + * struct ac97_controller - The AC97 controller of the AC-Link
>> + * @ops: the AC97 operations.
>> + * @controllers: linked list of all existing controllers.
>> + * @dev: the device providing the AC97 controller.
>> + * @slots_available: the mask of accessible/scanable codecs.
>> + * @codecs: the 4 possible AC97 codecs (NULL if none found).
>> + * @codecs_pdata: platform_data for each codec (NULL if no pdata).
>> + *
>> + * This structure is internal to AC97 bus, and should not be used by the
>> + * controllers themselves, excepting for using @dev.
>> + */
>> +struct ac97_controller {
>> + const struct ac97_controller_ops *ops;
>> + struct list_head controllers;
>> + struct device *dev;
>
> I'd make the controller itself a struct dev, rather than just having the
> pointer to the parent. This is more idiomatic and matches what other
> subsystems do. It has several advantages, you get proper refcounting of your
> controller structure, the controller gets its own sysfs directory where the
> CODECs appear as children, you don't need the manual sysfs attribute
> creation and removal in ac97_controler_{un,}register().
If you mean having "struct device dev" instead of "struct device *dev", it has
also a drawback : all the legacy platforms have already a probing method, be
that platform data, device-tree or something else.
I'm a bit converned about the conversion toll. Maybe I've not understood your
point fully, so please feel free to explain, with an actual example even better.
>> + void (*wait)(struct ac97_controller *adrv, int slot);
>> + void (*init)(struct ac97_controller *adrv, int slot);
>
> Neither wait nor init are ever used.
This is because I've not begun to porting sound/pci/ac97_codec.c into
sound/ac97.
>> + if ((codec_num < 0) || (codec_num >= AC97_BUS_MAX_CODECS))
>> + return ERR_PTR(-ERANGE);
>
> I'd make this EINVAL.
Ok.
>> + adev = container_of(dev, struct ac97_codec_device, dev);
>
> to_ac97_device()
Sure.
>> + codec_name = kasprintf(GFP_KERNEL, "%s:%d", dev_name(ac97_ctrl->dev),
>> + idx);
>> + codec->dev.init_name = codec_name;
>
> init_name is only for statically allocated devices. Use dev_set_name(dev,
> ...). No need for kasprintf() either as dev_set_name() takes a format string.
I'll try again, I seem to remember having tried that and it failed, but I can't
remember why.
> For this you need to split device_register into device_initialize() and
> device_add(). But usually that is what you want anyway.
Let me try again.
>> + ret = sysfs_create_link(&codec->dev.kobj, &ac97_ctrl->dev->kobj,
>> + "ac97_controller");
>
> Since the CODEC is a child of the controller this should not be necessary as
> this just points one directory up. It's like `ln -s .. parent`
This creates :
/sys/bus/ac97/devices/pxa2xx-ac97\:0/ac97_controller
Of course as you pointed out, it's a 'ln -s .. parent' like link, but it seems
to me very unfriendly to have :
- /sys/bus/ac97/devices/pxa2xx-ac97\:0/../ac97_operations
- while /sys/bus/ac97/devices/ac97_operations doesn't exist (obviously)
Mmm, I don't know for this one, my mind is not set, it's a bit hard to tell if
it's only an unecessary link bringing "comfort", or something usefull.
>
>> + if (ret)
>> + goto err_unregister_device;
>> +
>> + return 0;
>> +err_unregister_device:
>> + put_device(&codec->dev);
>> +err_free_codec:
>> + kfree(codec);
>
> Since the struct is reference counted, the freeing is done in the release
> callback and this leads to a double free.
A yes in the "goto err_unregister_device" case right, while it's necessary in
the "goto err_free_codec" case ... I need to rework that a bit.
>> +int snd_ac97_codec_driver_register(struct ac97_codec_driver *drv)
>> +{
>> + int ret;
>> +
>> + drv->driver.bus = &ac97_bus_type;
>> +
>> + ret = driver_register(&drv->driver);
>> + if (!ret)
>> + ac97_rescan_all_controllers();
>
> Rescanning the bus when a new codec driver is registered should not be
> neccessary. The bus is scanned once when the controller is registered, this
> creates the device. The device driver core will take care of binding the
> device to the driver, if the driver is registered after thed evice.
That's because you suppose the initial scanning found all the ac97 codecs.
But that's an incomplete vision as a codec might be powered off at that time and
not seen by the first scanning, while the new scanning will discover it.
>> +static int ac97_ctrl_codecs_unregister(struct ac97_controller *ac97_ctrl)
>> +{
>> + int i;
>> +
>> + for (i = 0; i < AC97_BUS_MAX_CODECS; i++)
>> + if (ac97_ctrl->codecs[i])
>> + put_device(&ac97_ctrl->codecs[i]->dev);
>
> This should be device_unregister() to match the device_register() in
> ac97_codec_add().
Right.
>> +
>> + return 0;
>> +}
>> +
>> +static ssize_t cold_reset_store(struct device *dev,
>> + struct device_attribute *attr, const char *buf,
>> + size_t len)
>> +{
>> + struct ac97_controller *ac97_ctrl = ac97_ctrl_find(dev);
>> +
>> + if (!dev)
>> + return -ENODEV;
>
> dev is never NULL here.
Ok.
> And for the ac97_ctrl there is a race condition. It could be unregistered and
> freed after ac97_ctrl_find() returned sucessfully, but before ac97_ctrl->ops
> is used.
A good catch, the ac97_controllers_mutex is missing.
> Same here.
Indeed.
>> +
>> + ac97_ctrl->ops->warm_reset(ac97_ctrl);
>> + return len;
>> +}
>> +static DEVICE_ATTR_WO(warm_reset);
>> +
>> +static struct attribute *ac97_controller_device_attrs[] = {
>> + &dev_attr_cold_reset.attr,
>> + &dev_attr_warm_reset.attr,
>> + NULL
>> +};
>
> This adds new userspace ABI that is not documented at the moment.
Very true. And as all userspace interface, it needs to be discussed. If nobody
complains, I'll add the documentation for next patch round.
>
>> +int snd_ac97_controller_register(const struct ac97_controller_ops *ops,
>> + struct device *dev,
>> + unsigned short slots_available,
>> + void **codecs_pdata)
>
> In my opinion this should return a handle to a ac97 controller which can
> then be passed to snd_ac97_controller_unregister(). This is in my opinion
> the better approach rather than looking up the controller by parent device.
This is another "legacy drivers" issue.
The legacy driver (the one probed by platform_data or devicetree) doesn't
necessarily have a private structure to store this ac97_controller pointer. This
enables an "easier" porting of existing drivers.
>> +/**
>> + * snd_ac97_controller_unregister - unregister an ac97 controller
>> + * @dev: the device previously provided to ac97_controller_register()
>> + *
>> + * Returns 0 on success, negative upon error
>
> Unregister must not be able to fail. Hotunplug is one of the core concepts
> of the device driver model and there is really nothing that can be done to
> prevent a device from disappearing, so there is no sensible way of handling
> the error (and your pxa driver modifications simply ignore it as well).
>
> This also means the framework needs to cope with the case where the
> controller is removed and the CODEC devices are still present. All future
> operations should return -ENODEV in that case.
Ah ... that will require a serious modification, and I see your point, so I'll
prepare this for next patchset.
>> +static struct bus_type ac97_bus_type = {
>> + .name = "ac97",
>> + .dev_attrs = ac97_dev_attrs,
>
> dev_attrs is deprecated in favor of dev_groups (See commit 880ffb5c6).
Ok.
>> index 000000000000..a835f03744bf
>> --- /dev/null
>> +++ b/sound/ac97/codec.c
>> @@ -0,0 +1,15 @@
>> +/*
>> + * Copyright (C) 2016 Robert Jarzmik <[email protected]>
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2 as
>> + * published by the Free Software Foundation.
>> + */
>> +
>> +#include <sound/ac97_codec.h>
>> +#include <sound/ac97/codec.h>
>> +#include <sound/ac97/controller.h>
>> +#include <linux/device.h>
>> +#include <linux/slab.h>
>> +#include <sound/soc.h> /* For compat_ac97_* */
>> +
>
> I'm not sure I understand what this file does.
Ah yes, I'll remove it.
It's the future conversion of sound/pci/ac97_codec.c, but it's ... empty so far.
Thanks very much for the very detailed review.
--
Robert
On 11/08/2016 10:18 PM, Robert Jarzmik wrote:
[...]
>>> +/**
>>> + * struct ac97_controller - The AC97 controller of the AC-Link
>>> + * @ops: the AC97 operations.
>>> + * @controllers: linked list of all existing controllers.
>>> + * @dev: the device providing the AC97 controller.
>>> + * @slots_available: the mask of accessible/scanable codecs.
>>> + * @codecs: the 4 possible AC97 codecs (NULL if none found).
>>> + * @codecs_pdata: platform_data for each codec (NULL if no pdata).
>>> + *
>>> + * This structure is internal to AC97 bus, and should not be used by the
>>> + * controllers themselves, excepting for using @dev.
>>> + */
>>> +struct ac97_controller {
>>> + const struct ac97_controller_ops *ops;
>>> + struct list_head controllers;
>>> + struct device *dev;
>>
>> I'd make the controller itself a struct dev, rather than just having the
>> pointer to the parent. This is more idiomatic and matches what other
>> subsystems do. It has several advantages, you get proper refcounting of your
>> controller structure, the controller gets its own sysfs directory where the
>> CODECs appear as children, you don't need the manual sysfs attribute
>> creation and removal in ac97_controler_{un,}register().
>
> If you mean having "struct device dev" instead of "struct device *dev", it has
> also a drawback : all the legacy platforms have already a probing method, be
> that platform data, device-tree or something else.
>
> I'm a bit converned about the conversion toll. Maybe I've not understood your
> point fully, so please feel free to explain, with an actual example even better.
This would be a struct device that is not bound to a driver, but just acts
as a shell for the controller and places it inside the device hierarchy. You
get reference counting and other management functions as well as a
consistent naming scheme. E.g. you can call the devices ac97c%d (or
something similar) and then call the CODEC ac97c%d.%d.
This is how most frameworks implementing some kind of control bus are
structured in the Linux kernel. E.g. take a look at I2C or SPI.
Your controller driver itself is unaffected by this, you still call
snd_ac97_controller_register() from the probe function and so on.
>
>>> + void (*wait)(struct ac97_controller *adrv, int slot);
>>> + void (*init)(struct ac97_controller *adrv, int slot);
>>
>> Neither wait nor init are ever used.
> This is because I've not begun to porting sound/pci/ac97_codec.c into
> sound/ac97.
Ok, makes sense. But maybe just leave them out for now and add them when
they are used.
[...]
>>> + ret = sysfs_create_link(&codec->dev.kobj, &ac97_ctrl->dev->kobj,
>>> + "ac97_controller");
>>
>> Since the CODEC is a child of the controller this should not be necessary as
>> this just points one directory up. It's like `ln -s .. parent`
> This creates :
> /sys/bus/ac97/devices/pxa2xx-ac97\:0/ac97_controller
>
> Of course as you pointed out, it's a 'ln -s .. parent' like link, but it seems
> to me very unfriendly to have :
> - /sys/bus/ac97/devices/pxa2xx-ac97\:0/../ac97_operations
> - while /sys/bus/ac97/devices/ac97_operations doesn't exist (obviously)
>
> Mmm, I don't know for this one, my mind is not set, it's a bit hard to tell if
> it's only an unecessary link bringing "comfort", or something usefull.
It is additional ABI and we do not have this for any other bus either (e.g.
you don't see a backlink for a I2C or SPI device to the parent). In my
opinion for the sake of keeping things consistent and simple we should not
add this.
>>
>>> + if (ret)
>>> + goto err_unregister_device;
>>> +
>>> + return 0;
>>> +err_unregister_device:
>>> + put_device(&codec->dev);
>>> +err_free_codec:
>>> + kfree(codec);
>>
>> Since the struct is reference counted, the freeing is done in the release
>> callback and this leads to a double free.
> A yes in the "goto err_unregister_device" case right, while it's necessary in
> the "goto err_free_codec" case ... I need to rework that a bit.
It should use put_device() in both cases, check the the device_register()
documentation. It says that put_device() must be used, even if
device_register() fails.
>
>>> +int snd_ac97_codec_driver_register(struct ac97_codec_driver *drv)
>>> +{
>>> + int ret;
>>> +
>>> + drv->driver.bus = &ac97_bus_type;
>>> +
>>> + ret = driver_register(&drv->driver);
>>> + if (!ret)
>>> + ac97_rescan_all_controllers();
>>
>> Rescanning the bus when a new codec driver is registered should not be
>> neccessary. The bus is scanned once when the controller is registered, this
>> creates the device. The device driver core will take care of binding the
>> device to the driver, if the driver is registered after thed evice.
> That's because you suppose the initial scanning found all the ac97 codecs.
> But that's an incomplete vision as a codec might be powered off at that time and
> not seen by the first scanning, while the new scanning will discover it.
But why would the device become suddenly visible when the driver is
registered. This seems to be an as arbitrary point in time as any other.
Also consider that when you build a driver as a module, the module will
typically only be auto-loaded after the device has been detected, based on
the device ID. On the other hand, if the driver is built-in driver
registration will happen either before or shortly after the controller
registration.
If there is the expectation that the AC97 CODEC might randomly appear on the
bus, the core should periodically scan the bus.
[...]
>>> + ac97_ctrl->ops->warm_reset(ac97_ctrl);
>>> + return len;
>>> +}
>>> +static DEVICE_ATTR_WO(warm_reset);
>>> +
>>> +static struct attribute *ac97_controller_device_attrs[] = {
>>> + &dev_attr_cold_reset.attr,
>>> + &dev_attr_warm_reset.attr,
>>> + NULL
>>> +};
>>
>> This adds new userspace ABI that is not documented at the moment.
> Very true. And as all userspace interface, it needs to be discussed. If nobody
> complains, I'll add the documentation for next patch round.
>>
>>> +int snd_ac97_controller_register(const struct ac97_controller_ops *ops,
>>> + struct device *dev,
>>> + unsigned short slots_available,
>>> + void **codecs_pdata)
>>
>> In my opinion this should return a handle to a ac97 controller which can
>> then be passed to snd_ac97_controller_unregister(). This is in my opinion
>> the better approach rather than looking up the controller by parent device.
> This is another "legacy drivers" issue.
>
> The legacy driver (the one probed by platform_data or devicetree) doesn't
> necessarily have a private structure to store this ac97_controller pointer.
I might be missing something, but I'm not convinced by this. Can you point
me to such a legacy driver where you think this would not work?
Lars-Peter Clausen <[email protected]> writes:
> On 11/08/2016 10:18 PM, Robert Jarzmik wrote:
>>> I'd make the controller itself a struct dev, rather than just having the
>>> pointer to the parent. This is more idiomatic and matches what other
>>> subsystems do. It has several advantages, you get proper refcounting of your
>>> controller structure, the controller gets its own sysfs directory where the
>>> CODECs appear as children, you don't need the manual sysfs attribute
>>> creation and removal in ac97_controler_{un,}register().
>>
>> If you mean having "struct device dev" instead of "struct device *dev", it has
>> also a drawback : all the legacy platforms have already a probing method, be
>> that platform data, device-tree or something else.
>>
>> I'm a bit converned about the conversion toll. Maybe I've not understood your
>> point fully, so please feel free to explain, with an actual example even better.
>
> This would be a struct device that is not bound to a driver, but just acts
> as a shell for the controller and places it inside the device hierarchy. You
> get reference counting and other management functions as well as a
> consistent naming scheme. E.g. you can call the devices ac97c%d (or
> something similar) and then call the CODEC ac97c%d.%d.
Let me think about it.
If I get you right, the device model would be, from a parenthood point of view:
pxa2xx_ac97 (platform_device)
^
|
ac97_controller.dev (device, son of pxa2xx_ac97)
/ \
/ \
/ \
/ \
/ \
wm97xx-core codec2 (sons ac97_controller.dev)
I have already the device hierarchy AFAIK, created in ac97_codec_add(). I'm
still failing to see the difference, as the refcounting is already used. The
only additional thing I see is the introduction of an intermediate
ac97_controller.dev which is refcounted.
In current model, pxa2xx_ac97 refcount is incremented for each codec device. In
the one you propose, pxa2xx_ac97 will be 1 refcounted (maybe 2 actually), and
ac97_controller.dev will be refcounted for each codec. This ac97_controller.dev
will be a spiritual twin of spi_master/i2c_adapter.
As I said, I must think about it and find which value is brought by this
additionnal layer.
As for the name change, I must check if this breaks the sound machine files, and
their dai_link structures (ex: mioa701_wm9713.c, structure mioa701_dai). In a
former state of this patchset I had changed the device names, ie. wm9713-codec
became pxa2xx-ac97.0, and the my machine file became broken.
> This is how most frameworks implementing some kind of control bus are
> structured in the Linux kernel. E.g. take a look at I2C or SPI.
I will.
>>>> + ret = sysfs_create_link(&codec->dev.kobj, &ac97_ctrl->dev->kobj,
>>>> + "ac97_controller");
>>>
>>> Since the CODEC is a child of the controller this should not be necessary as
>>> this just points one directory up. It's like `ln -s .. parent`
>> This creates :
>> /sys/bus/ac97/devices/pxa2xx-ac97\:0/ac97_controller
>>
>> Of course as you pointed out, it's a 'ln -s .. parent' like link, but it seems
>> to me very unfriendly to have :
>> - /sys/bus/ac97/devices/pxa2xx-ac97\:0/../ac97_operations
>> - while /sys/bus/ac97/devices/ac97_operations doesn't exist (obviously)
>>
>> Mmm, I don't know for this one, my mind is not set, it's a bit hard to tell if
>> it's only an unecessary link bringing "comfort", or something usefull.
>
> It is additional ABI and we do not have this for any other bus either (e.g.
> you don't see a backlink for a I2C or SPI device to the parent). In my
> opinion for the sake of keeping things consistent and simple we should not
> add this.
Fair enough.
>>>> +int snd_ac97_codec_driver_register(struct ac97_codec_driver *drv)
>>>> +{
>>>> + int ret;
>>>> +
>>>> + drv->driver.bus = &ac97_bus_type;
>>>> +
>>>> + ret = driver_register(&drv->driver);
>>>> + if (!ret)
>>>> + ac97_rescan_all_controllers();
>>>
>>> Rescanning the bus when a new codec driver is registered should not be
>>> neccessary. The bus is scanned once when the controller is registered, this
>>> creates the device. The device driver core will take care of binding the
>>> device to the driver, if the driver is registered after thed evice.
>> That's because you suppose the initial scanning found all the ac97 codecs.
>> But that's an incomplete vision as a codec might be powered off at that time and
>> not seen by the first scanning, while the new scanning will discover it.
>
> But why would the device become suddenly visible when the driver is
> registered. This seems to be an as arbitrary point in time as any other.
Because in the meantime, a regulator or something else provided power to the
AC97 IP, or a clock, and it can answer to requests after that.
And another point if that the clock of controller might be provided by the AC97
codec, and the scan will only succeed once the codec is actually providing this
clock, which it might do only once the module_init() is done.
> Also consider that when you build a driver as a module, the module will
> typically only be auto-loaded after the device has been detected, based on
> the device ID. On the other hand, if the driver is built-in driver
> registration will happen either before or shortly after the controller
> registration.
> If there is the expectation that the AC97 CODEC might randomly appear on the
> bus, the core should periodically scan the bus.
Power wise a periodical scan doesn't look good, at all.
More globally, I don't see if there is an actual issue we're trying to address
here, ie. that the rescan is a bug, or if it's more an "Occam's razor"
discussion ?
>>> In my opinion this should return a handle to a ac97 controller which can
>>> then be passed to snd_ac97_controller_unregister(). This is in my opinion
>>> the better approach rather than looking up the controller by parent device.
>> This is another "legacy drivers" issue.
>>
>> The legacy driver (the one probed by platform_data or devicetree) doesn't
>> necessarily have a private structure to store this ac97_controller pointer.
>
> I might be missing something, but I'm not convinced by this. Can you point
> me to such a legacy driver where you think this would not work?
The first one that popped out:
- hac_soc_platform_probe()
Cheers.
--
Robert
On 11/09/2016 10:27 PM, Robert Jarzmik wrote:
[...]
>>>>> +int snd_ac97_codec_driver_register(struct ac97_codec_driver *drv)
>>>>> +{
>>>>> + int ret;
>>>>> +
>>>>> + drv->driver.bus = &ac97_bus_type;
>>>>> +
>>>>> + ret = driver_register(&drv->driver);
>>>>> + if (!ret)
>>>>> + ac97_rescan_all_controllers();
>>>>
>>>> Rescanning the bus when a new codec driver is registered should not be
>>>> neccessary. The bus is scanned once when the controller is registered, this
>>>> creates the device. The device driver core will take care of binding the
>>>> device to the driver, if the driver is registered after thed evice.
>>> That's because you suppose the initial scanning found all the ac97 codecs.
>>> But that's an incomplete vision as a codec might be powered off at that time and
>>> not seen by the first scanning, while the new scanning will discover it.
>>
>> But why would the device become suddenly visible when the driver is
>> registered. This seems to be an as arbitrary point in time as any other.
> Because in the meantime, a regulator or something else provided power to the
> AC97 IP, or a clock, and it can answer to requests after that.
>
> And another point if that the clock of controller might be provided by the AC97
> codec, and the scan will only succeed once the codec is actually providing this
> clock, which it might do only once the module_init() is done.
>
>> Also consider that when you build a driver as a module, the module will
>> typically only be auto-loaded after the device has been detected, based on
>> the device ID. On the other hand, if the driver is built-in driver
>> registration will happen either before or shortly after the controller
>> registration.
>> If there is the expectation that the AC97 CODEC might randomly appear on the
>> bus, the core should periodically scan the bus.
> Power wise a periodical scan doesn't look good, at all.
>
> More globally, I don't see if there is an actual issue we're trying to address
> here, ie. that the rescan is a bug, or if it's more an "Occam's razor"
> discussion ?
It's a framework design discussion. In my opinion the driver being
registered and the device becoming visible on the physical bus are two
completely unrelated operations. Especially in the Linux device driver model.
You have a generic driver that calls ac97_codec_driver_register() in its
module_init() section. This generic driver does (at module_init() time) not
know anything about a device instance specific clocks, regulators or other
resources. Resources are handled on a per device instance basis, and you
won't have a device instance until the device has been detected, which
happens in ac97_bus_scan(). So you have a cyclic dependency loop here.
Maybe we can just leave the rescanning out for now and think about how to
best handle it when the need arises.
>
>>>> In my opinion this should return a handle to a ac97 controller which can
>>>> then be passed to snd_ac97_controller_unregister(). This is in my opinion
>>>> the better approach rather than looking up the controller by parent device.
>>> This is another "legacy drivers" issue.
>>>
>>> The legacy driver (the one probed by platform_data or devicetree) doesn't
>>> necessarily have a private structure to store this ac97_controller pointer.
>>
>> I might be missing something, but I'm not convinced by this. Can you point
>> me to such a legacy driver where you think this would not work?
> The first one that popped out:
> - hac_soc_platform_probe()
I think that driver should be able to use platform_set_drvdata() to store
the pointer.
On Wed, 26 Oct 2016, Robert Jarzmik wrote:
> The WM9705, WM9712 and WM9713 are highly integrated codecs, with an
> audio codec, DAC and ADC, GPIO unit and a touchscreen interface.
>
> Historically the support was spread across drivers/input/touchscreen and
> sound/soc/codecs. The sharing was done through ac97 bus sharing. This
> model will not withstand the new AC97 bus model, where codecs are
> discovered on runtime.
>
> Signed-off-by: Robert Jarzmik <[email protected]>
> ---
> drivers/mfd/Kconfig | 14 +++
> drivers/mfd/Makefile | 1 +
> drivers/mfd/wm97xx-core.c | 282 +++++++++++++++++++++++++++++++++++++++++++++
> include/linux/mfd/wm97xx.h | 31 +++++
> 4 files changed, 328 insertions(+)
> create mode 100644 drivers/mfd/wm97xx-core.c
> create mode 100644 include/linux/mfd/wm97xx.h
>
> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> index c6df6442ba2b..2ac818127b0a 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -1597,6 +1597,20 @@ config MFD_WM8994
> core support for the WM8994, in order to use the actual
> functionaltiy of the device other drivers must be enabled.
>
> +config MFD_WM97xx
> + tristate "Wolfson Microelectronics WM97xx"
> + select MFD_CORE
> + select REGMAP_AC97
> + select AC97_BUS_COMPAT if AC97_BUS_NEW
> + help
> +
Surplus '\n' here.
> + The WM9705, WM9712 and WM9713 is a highly integrated hi-fi CODEC
> + designed for smartphone applications. As well as audio functionality
> + it has on board GPIO and a touchscreen functionality which is
> + supported via the relevant subsystems. This driver provides core
> + support for the WM97xx, in order to use the actual functionaltiy of
Always spell check your work.
> + the device other drivers must be enabled.
> +
> config MFD_STW481X
> tristate "Support for ST Microelectronics STw481x"
> depends on I2C && (ARCH_NOMADIK || COMPILE_TEST)
> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> index 9834e669d985..5c3534f4c7c3 100644
> --- a/drivers/mfd/Makefile
> +++ b/drivers/mfd/Makefile
> @@ -69,6 +69,7 @@ obj-$(CONFIG_MFD_WM8350) += wm8350.o
> obj-$(CONFIG_MFD_WM8350_I2C) += wm8350-i2c.o
> wm8994-objs := wm8994-core.o wm8994-irq.o wm8994-regmap.o
> obj-$(CONFIG_MFD_WM8994) += wm8994.o
> +obj-$(CONFIG_MFD_WM97xx) += wm97xx-core.o
>
> obj-$(CONFIG_TPS6105X) += tps6105x.o
> obj-$(CONFIG_TPS65010) += tps65010.o
> diff --git a/drivers/mfd/wm97xx-core.c b/drivers/mfd/wm97xx-core.c
> new file mode 100644
> index 000000000000..f2cd80354b4a
> --- /dev/null
> +++ b/drivers/mfd/wm97xx-core.c
> @@ -0,0 +1,282 @@
> +/*
> + * Wolfson WM97xx -- Core device
> + *
> + * Copyright (C) 2016 Robert Jarzmik
> + *
> + * 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.
> + *
> + * Features:
> + * - an AC97 audio codec
> + * - a touchscreen driver
> + * - a GPIO block
Place this above the Copyright.
> + */
> +
> +#include <linux/module.h>
Why is this seperted?
> +#include <linux/device.h>
> +#include <linux/mfd/core.h>
> +#include <linux/mfd/wm97xx.h>
> +#include <linux/wm97xx.h>
> +#include <linux/regmap.h>
> +#include <linux/slab.h>
> +#include <sound/ac97/codec.h>
> +#include <sound/ac97/compat.h>
Alphabetical.
> +#define WM9705_VENDOR_ID 0x574d4c05
> +#define WM9712_VENDOR_ID 0x574d4c12
> +#define WM9713_VENDOR_ID 0x574d4c13
> +#define WM97xx_VENDOR_ID_MASK 0xffffffff
Use tabs, not spaces.
These are probably better represented as enums.
> +struct wm97xx_priv {
> + struct regmap *regmap;
> + struct snd_ac97 *ac97;
> + struct device *dev;
> +};
Replace _priv with _ddata.
Also document using kerneldoc.
> +static bool wm97xx_readable_reg(struct device *dev, unsigned int reg)
> +{
> + switch (reg) {
> + case AC97_RESET ... AC97_PCM_SURR_DAC_RATE:
> + case AC97_PCM_LR_ADC_RATE:
> + case AC97_CENTER_LFE_MASTER:
> + case AC97_SPDIF ... AC97_LINE1_LEVEL:
> + case AC97_GPIO_CFG ... 0x5c:
Please define.
> + case AC97_CODEC_CLASS_REV ... AC97_PCI_SID:
> + case 0x74 ... AC97_VENDOR_ID2:
As above.
> + default:
> + return false;
> + }
> +}
> +
> +static bool wm97xx_writeable_reg(struct device *dev, unsigned int reg)
> +{
> + switch (reg) {
> + case AC97_VENDOR_ID1:
> + case AC97_VENDOR_ID2:
> + return false;
> + default:
> + return wm97xx_readable_reg(dev, reg);
> + }
> +}
> +
> +static const struct reg_default wm97xx_reg_defaults[] = {
> +};
What's the point in this?
> +static const struct regmap_config wm9705_regmap_config = {
> + .reg_bits = 16,
> + .reg_stride = 2,
> + .val_bits = 16,
> + .max_register = 0x7e,
> + .cache_type = REGCACHE_RBTREE,
> +
> + .reg_defaults = wm97xx_reg_defaults,
> + .num_reg_defaults = ARRAY_SIZE(wm97xx_reg_defaults),
> + .volatile_reg = regmap_ac97_default_volatile,
> + .readable_reg = wm97xx_readable_reg,
> + .writeable_reg = wm97xx_writeable_reg,
> +};
> +
> +static const struct regmap_config wm9712_regmap_config = {
> + .reg_bits = 16,
> + .reg_stride = 2,
> + .val_bits = 16,
> + .max_register = 0x7e,
> + .cache_type = REGCACHE_RBTREE,
> +
> + .reg_defaults = wm97xx_reg_defaults,
> + .num_reg_defaults = ARRAY_SIZE(wm97xx_reg_defaults),
> + .volatile_reg = regmap_ac97_default_volatile,
> + .readable_reg = wm97xx_readable_reg,
> + .writeable_reg = wm97xx_writeable_reg,
> +};
> +
> +static int wm9705_register(struct wm97xx_priv *wm97xx)
> +{
> + return 0;
> +}
> +
> +static int wm9712_register(struct wm97xx_priv *wm97xx)
> +{
> + return 0;
> +}
I don't get it?
Either populate or don't provide.
> +static const struct reg_default wm9713_reg_defaults[] = {
> + { 0x02, 0x8080 }, /* Speaker Output Volume */
> + { 0x04, 0x8080 }, /* Headphone Output Volume */
> + { 0x06, 0x8080 }, /* Out3/OUT4 Volume */
> + { 0x08, 0xc880 }, /* Mono Volume */
> + { 0x0a, 0xe808 }, /* LINEIN Volume */
> + { 0x0c, 0xe808 }, /* DAC PGA Volume */
> + { 0x0e, 0x0808 }, /* MIC PGA Volume */
> + { 0x10, 0x00da }, /* MIC Routing Control */
> + { 0x12, 0x8000 }, /* Record PGA Volume */
> + { 0x14, 0xd600 }, /* Record Routing */
> + { 0x16, 0xaaa0 }, /* PCBEEP Volume */
> + { 0x18, 0xaaa0 }, /* VxDAC Volume */
> + { 0x1a, 0xaaa0 }, /* AUXDAC Volume */
> + { 0x1c, 0x0000 }, /* Output PGA Mux */
> + { 0x1e, 0x0000 }, /* DAC 3D control */
> + { 0x20, 0x0f0f }, /* DAC Tone Control*/
> + { 0x22, 0x0040 }, /* MIC Input Select & Bias */
> + { 0x24, 0x0000 }, /* Output Volume Mapping & Jack */
> + { 0x26, 0x7f00 }, /* Powerdown Ctrl/Stat*/
> + { 0x28, 0x0405 }, /* Extended Audio ID */
> + { 0x2a, 0x0410 }, /* Extended Audio Start/Ctrl */
> + { 0x2c, 0xbb80 }, /* Audio DACs Sample Rate */
> + { 0x2e, 0xbb80 }, /* AUXDAC Sample Rate */
> + { 0x32, 0xbb80 }, /* Audio ADCs Sample Rate */
> + { 0x36, 0x4523 }, /* PCM codec control */
> + { 0x3a, 0x2000 }, /* SPDIF control */
> + { 0x3c, 0xfdff }, /* Powerdown 1 */
> + { 0x3e, 0xffff }, /* Powerdown 2 */
> + { 0x40, 0x0000 }, /* General Purpose */
> + { 0x42, 0x0000 }, /* Fast Power-Up Control */
> + { 0x44, 0x0080 }, /* MCLK/PLL Control */
> + { 0x46, 0x0000 }, /* MCLK/PLL Control */
> +
> + { 0x4c, 0xfffe }, /* GPIO Pin Configuration */
> + { 0x4e, 0xffff }, /* GPIO Pin Polarity / Type */
> + { 0x50, 0x0000 }, /* GPIO Pin Sticky */
> + { 0x52, 0x0000 }, /* GPIO Pin Wake-Up */
> + /* GPIO Pin Status */
> + { 0x56, 0xfffe }, /* GPIO Pin Sharing */
> + { 0x58, 0x4000 }, /* GPIO PullUp/PullDown */
> + { 0x5a, 0x0000 }, /* Additional Functions 1 */
> + { 0x5c, 0x0000 }, /* Additional Functions 2 */
> + { 0x60, 0xb032 }, /* ALC Control */
> + { 0x62, 0x3e00 }, /* ALC / Noise Gate Control */
> + { 0x64, 0x0000 }, /* AUXDAC input control */
> + { 0x74, 0x0000 }, /* Digitiser Reg 1 */
> + { 0x76, 0x0006 }, /* Digitiser Reg 2 */
> + { 0x78, 0x0001 }, /* Digitiser Reg 3 */
> + { 0x7a, 0x0000 }, /* Digitiser Read Back */
> +};
> +
> +static const struct regmap_config wm9713_regmap_config = {
> + .reg_bits = 16,
> + .reg_stride = 2,
> + .val_bits = 16,
> + .max_register = 0x7e,
> + .cache_type = REGCACHE_RBTREE,
> +
> + .reg_defaults = wm9713_reg_defaults,
> + .num_reg_defaults = ARRAY_SIZE(wm9713_reg_defaults),
> + .volatile_reg = regmap_ac97_default_volatile,
> + .readable_reg = wm97xx_readable_reg,
> + .writeable_reg = wm97xx_writeable_reg,
> +};
> +
> +static int wm9713_register(struct wm97xx_priv *wm97xx,
> + struct wm97xx_pdata *pdata)
> +{
What are you lining this up with?
> + struct wm97xx_platform_data codec_pdata;
> + const struct mfd_cell cells[] = {
> + {
> + .name = "wm9713-codec",
> + .platform_data = &codec_pdata,
> + .pdata_size = sizeof(codec_pdata),
> + },
> + {
> + .name = "wm97xx-ts",
> + .platform_data = &codec_pdata,
> + .pdata_size = sizeof(codec_pdata),
> + },
> + };
> +
> + codec_pdata.ac97 = wm97xx->ac97;
> + codec_pdata.regmap = devm_regmap_init_ac97(wm97xx->ac97,
> + &wm9713_regmap_config);
> + codec_pdata.batt_pdata = pdata->batt_pdata;
> + if (IS_ERR(codec_pdata.regmap))
> + return PTR_ERR(codec_pdata.regmap);
This doesn't look like pdata. You can get rid of all of this hoop
jumping if you add it to ddata and set it as such.
> + return devm_mfd_add_devices(wm97xx->dev, -1, cells,
Use the defines.
> + ARRAY_SIZE(cells), NULL, 0, NULL);
> +}
> +
> +static int wm97xx_ac97_probe(struct ac97_codec_device *adev)
This looks sound specific.
Why isn't this a plane platform driver?
> +{
> + struct wm97xx_priv *wm97xx;
> + int ret;
> + void *pdata = snd_ac97_codec_get_platdata(adev);
> +
> + wm97xx = devm_kzalloc(ac97_codec_dev2dev(adev),
> + sizeof(*wm97xx), GFP_KERNEL);
> + if (!wm97xx)
> + return -ENOMEM;
> +
> + wm97xx->dev = ac97_codec_dev2dev(adev);
> + wm97xx->ac97 = snd_ac97_compat_alloc(adev);
> + if (IS_ERR(wm97xx->ac97))
> + return PTR_ERR(wm97xx->ac97);
> +
> +
> + ac97_set_drvdata(adev, wm97xx);
> + dev_info(wm97xx->dev, "wm97xx core found, id=0x%x\n",
> + adev->vendor_id);
All of this ac97/sound stuff should be done in the ac97/sound driver.
> + switch (adev->vendor_id) {
> + case WM9705_VENDOR_ID:
> + ret = wm9705_register(wm97xx);
> + break;
> + case WM9712_VENDOR_ID:
> + ret = wm9712_register(wm97xx);
> + break;
> + case WM9713_VENDOR_ID:
> + ret = wm9713_register(wm97xx, pdata);
> + break;
> + default:
> + ret = -ENODEV;
> + }
> +
> + if (ret)
> + snd_ac97_compat_release(wm97xx->ac97);
> +
> + return ret;
> +}
> +
> +static int wm97xx_ac97_remove(struct ac97_codec_device *adev)
> +{
> + struct wm97xx_priv *wm97xx = ac97_get_drvdata(adev);
> +
> + snd_ac97_compat_release(wm97xx->ac97);
> +
> + return 0;
> +}
> +
> +static const struct ac97_id wm97xx_ac97_ids[] = {
> + { .id = WM9705_VENDOR_ID, .mask = WM97xx_VENDOR_ID_MASK },
> + { .id = WM9712_VENDOR_ID, .mask = WM97xx_VENDOR_ID_MASK },
> + { .id = WM9713_VENDOR_ID, .mask = WM97xx_VENDOR_ID_MASK },
> + { }
> +};
> +
> +static struct ac97_codec_driver wm97x3_ac97_driver = {
> + .driver = {
> + .name = "wm97xx-core",
> + },
> + .probe = wm97xx_ac97_probe,
> + .remove = wm97xx_ac97_remove,
> + .id_table = wm97xx_ac97_ids,
> +};
> +
> +static int __init wm97xx_module_init(void)
> +{
> + return snd_ac97_codec_driver_register(&wm97x3_ac97_driver);
> +}
> +module_init(wm97xx_module_init);
> +
> +static void __exit wm97xx_module_exit(void)
> +{
> + snd_ac97_codec_driver_unregister(&wm97x3_ac97_driver);
> +}
> +module_exit(wm97xx_module_exit);
> +
> +MODULE_DESCRIPTION("WM9712, WM9713 core driver");
> +MODULE_AUTHOR("Robert Jarzmik <[email protected]>");
> +MODULE_LICENSE("GPL");
> +
> diff --git a/include/linux/mfd/wm97xx.h b/include/linux/mfd/wm97xx.h
> new file mode 100644
> index 000000000000..627322f14d48
> --- /dev/null
> +++ b/include/linux/mfd/wm97xx.h
> @@ -0,0 +1,31 @@
> +/*
> + * wm97xx client interface
> + *
> + * Copyright (C) 2016 Robert Jarzmik
> + *
> + * 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.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + */
> +
> +#ifndef __LINUX_MFD_WM97XX_H
> +#define __LINUX_MFD_WM97XX_H
> +
> +struct regmap;
> +struct wm97xx_batt_pdata;
> +struct snd_ac97;
Why can't you just add the include files?
> +struct wm97xx_platform_data {
> + struct snd_ac97 *ac97;
> + struct regmap *regmap;
> + struct wm97xx_batt_pdata *batt_pdata;
> +};
> +
> +
> +#endif
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
Lee Jones <[email protected]> writes:
> On Wed, 26 Oct 2016, Robert Jarzmik wrote:
>> +config MFD_WM97xx
>> + tristate "Wolfson Microelectronics WM97xx"
>> + select MFD_CORE
>> + select REGMAP_AC97
>> + select AC97_BUS_COMPAT if AC97_BUS_NEW
>> + help
>> +
>
> Surplus '\n' here.
Right, for v2.
>> + The WM9705, WM9712 and WM9713 is a highly integrated hi-fi CODEC
>> + designed for smartphone applications. As well as audio functionality
>> + it has on board GPIO and a touchscreen functionality which is
>> + supported via the relevant subsystems. This driver provides core
>> + support for the WM97xx, in order to use the actual functionaltiy of
>
> Always spell check your work.
For v2.
>> + * Copyright (C) 2016 Robert Jarzmik
>> + *
>> + * 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.
>> + *
>> + * Features:
>> + * - an AC97 audio codec
>> + * - a touchscreen driver
>> + * - a GPIO block
>
> Place this above the Copyright.
Right, for v2.
>> + */
>> +
>> +#include <linux/module.h>
>
> Why is this seperted?
No specific reason, I will remove the blank line, for v2.
>> +#include <linux/device.h>
>> +#include <linux/mfd/core.h>
>> +#include <linux/mfd/wm97xx.h>
>> +#include <linux/wm97xx.h>
>> +#include <linux/regmap.h>
>> +#include <linux/slab.h>
>> +#include <sound/ac97/codec.h>
>> +#include <sound/ac97/compat.h>
>
> Alphabetical.
Ah yeah, the linux/wm97xx.h ... for v2.
>> +#define WM9705_VENDOR_ID 0x574d4c05
>> +#define WM9712_VENDOR_ID 0x574d4c12
>> +#define WM9713_VENDOR_ID 0x574d4c13
>> +#define WM97xx_VENDOR_ID_MASK 0xffffffff
>
> Use tabs, not spaces.
Right, for v2.
> These are probably better represented as enums.
These are ids, just as devicetree ids, or PCI ids, I don't think an enum will
fit.
>> +struct wm97xx_priv {
>> + struct regmap *regmap;
>> + struct snd_ac97 *ac97;
>> + struct device *dev;
>> +};
>
> Replace _priv with _ddata.
Ok, will do, would you care to explain if this is something specific to your mfd
tree, or is it a kernel overall recommendation ?
> Also document using kerneldoc.
Right, for v2.
>
>> +static bool wm97xx_readable_reg(struct device *dev, unsigned int reg)
>> +{
>> + switch (reg) {
>> + case AC97_RESET ... AC97_PCM_SURR_DAC_RATE:
>> + case AC97_PCM_LR_ADC_RATE:
>> + case AC97_CENTER_LFE_MASTER:
>> + case AC97_SPDIF ... AC97_LINE1_LEVEL:
>> + case AC97_GPIO_CFG ... 0x5c:
>
> Please define.
>> + case AC97_CODEC_CLASS_REV ... AC97_PCI_SID:
>> + case 0x74 ... AC97_VENDOR_ID2:
>
> As above.
Right, for v2.
>> +static bool wm97xx_writeable_reg(struct device *dev, unsigned int reg)
>> +{
>> + switch (reg) {
>> + case AC97_VENDOR_ID1:
>> + case AC97_VENDOR_ID2:
>> + return false;
>> + default:
>> + return wm97xx_readable_reg(dev, reg);
>> + }
>> +}
>> +
>> +static const struct reg_default wm97xx_reg_defaults[] = {
>> +};
>
> What's the point in this?
Ah, that's a reminder I have still more work on this patch ... Either I remove
support for wm9705 and wm9712 in the first version, or I complete it and add the
tables :
- wm9705_reg_defaults
- wm9712_reg_defaults
>> +static int wm9705_register(struct wm97xx_priv *wm97xx)
>> +{
>> + return 0;
>> +}
>> +
>> +static int wm9712_register(struct wm97xx_priv *wm97xx)
>> +{
>> + return 0;
>> +}
>
> I don't get it?
>
> Either populate or don't provide.
Same story as above, either I complete wm9705 and wm9712 support or I remove it.
>> +static int wm9713_register(struct wm97xx_priv *wm97xx,
>> + struct wm97xx_pdata *pdata)
>> +{
>
> What are you lining this up with?
Emacs ... I don't see your point though, seeing it not aligned in the diff chunk
doesn't mean it's not properly aligned.
>> + struct wm97xx_platform_data codec_pdata;
>> + const struct mfd_cell cells[] = {
>> + {
>> + .name = "wm9713-codec",
>> + .platform_data = &codec_pdata,
>> + .pdata_size = sizeof(codec_pdata),
>> + },
>> + {
>> + .name = "wm97xx-ts",
>> + .platform_data = &codec_pdata,
>> + .pdata_size = sizeof(codec_pdata),
>> + },
>> + };
>> +
>> + codec_pdata.ac97 = wm97xx->ac97;
>> + codec_pdata.regmap = devm_regmap_init_ac97(wm97xx->ac97,
>> + &wm9713_regmap_config);
>> + codec_pdata.batt_pdata = pdata->batt_pdata;
>> + if (IS_ERR(codec_pdata.regmap))
>> + return PTR_ERR(codec_pdata.regmap);
>
> This doesn't look like pdata. You can get rid of all of this hoop
> jumping if you add it to ddata and set it as such.
You mean I should pass ddata to mfd sub-cells, right ?
>> + return devm_mfd_add_devices(wm97xx->dev, -1, cells,
>
> Use the defines.
Ok.
>
>> + ARRAY_SIZE(cells), NULL, 0, NULL);
>> +}
>> +
>> +static int wm97xx_ac97_probe(struct ac97_codec_device *adev)
>
> This looks sound specific.
>
> Why isn't this a plane platform driver?
That's the whole purpose of the patch serie.
This serie transforms the wm97xx drivers from a platform driver model to an ac97
model, where the ac97 devices are automatically discovered. The long explanation
is in patch 0/9, on how it started and what is the global purpose.
The short story is : switch to the new AC97 bus driver model.
As for the "sound specific part", it's because AC97 bus is mainly used in sound
oriented drivers, but still the codec IPs provide more than just sound, as the
Wolfson codecs for instance.
>> +{
>> + struct wm97xx_priv *wm97xx;
>> + int ret;
>> + void *pdata = snd_ac97_codec_get_platdata(adev);
>> +
>> + wm97xx = devm_kzalloc(ac97_codec_dev2dev(adev),
>> + sizeof(*wm97xx), GFP_KERNEL);
>> + if (!wm97xx)
>> + return -ENOMEM;
>> +
>> + wm97xx->dev = ac97_codec_dev2dev(adev);
>> + wm97xx->ac97 = snd_ac97_compat_alloc(adev);
>> + if (IS_ERR(wm97xx->ac97))
>> + return PTR_ERR(wm97xx->ac97);
>> +
>> +
>> + ac97_set_drvdata(adev, wm97xx);
>> + dev_info(wm97xx->dev, "wm97xx core found, id=0x%x\n",
>> + adev->vendor_id);
>
> All of this ac97/sound stuff should be done in the ac97/sound driver.
Nope, it's not sound adherence you're seeing here, it's ac97 bus and driver
model adherence you're seeing. Would the bus be in drivers/ac97 instead of
sound/ac97, the code would remain the same, would be bus be i2c you would see
the same kind of calls but with i2c_xxx and not ac97_xxx.
The wm97xx needs an ac97 bus to interact with the hardware, to provide sound,
gpio, adc, etc ... functions. That's what is expressed here, and the fact that
this ac97 access has to shared amongst the mfd sub-cells, and that these cells
require :
- wm97xx->ac97 : this one is for drivers/input/touchscreen/wm97xx-core.c
So the requirement in this case is not for ac97/sound but input/touchscreen.
>> diff --git a/include/linux/mfd/wm97xx.h b/include/linux/mfd/wm97xx.h
>> new file mode 100644
>> index 000000000000..627322f14d48
>> --- /dev/null
>> +++ b/include/linux/mfd/wm97xx.h
>> @@ -0,0 +1,31 @@
>> +/*
>> + * wm97xx client interface
>> + *
>> + * Copyright (C) 2016 Robert Jarzmik
>> + *
>> + * 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.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
>> + * GNU General Public License for more details.
>> + */
>> +
>> +#ifndef __LINUX_MFD_WM97XX_H
>> +#define __LINUX_MFD_WM97XX_H
>> +
>> +struct regmap;
>> +struct wm97xx_batt_pdata;
>> +struct snd_ac97;
>
> Why can't you just add the include files?
I could, but I don't need to, do I ?
Moreover, if a mfd sub-cell doesn't use regmap for example, I won't include a
useless define.
Thanks for the review, Lee. This will iterate, I'll split out mfd patch(es) to
follow up the review with you and Mark to lessen the burden on your mailbox.
Cheers.
--
Robert
Mark, please see below:
On Sat, 19 Nov 2016, Robert Jarzmik wrote:
> Lee Jones <[email protected]> writes:
>
> >> +#define WM9705_VENDOR_ID 0x574d4c05
> >> +#define WM9712_VENDOR_ID 0x574d4c12
> >> +#define WM9713_VENDOR_ID 0x574d4c13
> >> +#define WM97xx_VENDOR_ID_MASK 0xffffffff
> >
> > These are probably better represented as enums.
> These are ids, just as devicetree ids, or PCI ids, I don't think an enum will
> fit.
That's fine. We can use enums to simply group items of a similar
type. Representing these as enums would only serve to benefit code
cleanliness. What makes you think they won't fit?
> >> +struct wm97xx_priv {
> >> + struct regmap *regmap;
> >> + struct snd_ac97 *ac97;
> >> + struct device *dev;
> >> +};
> >
> > Replace _priv with _ddata.
> Ok, will do, would you care to explain if this is something specific to your mfd
> tree, or is it a kernel overall recommendation ?
It's personal preference. But these aren't necessarily private, so
priv doesn't really make a great deal of sense. These types of saved
pointers are better described as device data (ddata).
[...]
> >> +static const struct reg_default wm97xx_reg_defaults[] = {
> >> +};
> >
> > What's the point in this?
> Ah, that's a reminder I have still more work on this patch ... Either I remove
> support for wm9705 and wm9712 in the first version, or I complete it and add the
> tables :
> - wm9705_reg_defaults
> - wm9712_reg_defaults
Please don't add redundant code. I suggest you remove it for this
submission.
> >> +static int wm9705_register(struct wm97xx_priv *wm97xx)
> >> +{
> >> + return 0;
> >> +}
> >> +
> >> +static int wm9712_register(struct wm97xx_priv *wm97xx)
> >> +{
> >> + return 0;
> >> +}
> >
> > I don't get it?
> >
> > Either populate or don't provide.
> Same story as above, either I complete wm9705 and wm9712 support or I remove it.
Remove it please.
> >> +static int wm9713_register(struct wm97xx_priv *wm97xx,
> >> + struct wm97xx_pdata *pdata)
> >> +{
> >
> > What are you lining this up with?
> Emacs ... I don't see your point though, seeing it not aligned in the diff chunk
> doesn't mean it's not properly aligned.
That is true. So the two "scruct"s are line up in the source file,
right?
[...]
> >> + codec_pdata.ac97 = wm97xx->ac97;
> >> + codec_pdata.regmap = devm_regmap_init_ac97(wm97xx->ac97,
> >> + &wm9713_regmap_config);
> >> + codec_pdata.batt_pdata = pdata->batt_pdata;
> >> + if (IS_ERR(codec_pdata.regmap))
> >> + return PTR_ERR(codec_pdata.regmap);
> >
> > This doesn't look like pdata. You can get rid of all of this hoop
> > jumping if you add it to ddata and set it as such.
> You mean I should pass ddata to mfd sub-cells, right ?
Correct.
[...]
> >> +static int wm97xx_ac97_probe(struct ac97_codec_device *adev)
> >
> > This looks sound specific.
> >
> > Why isn't this a plane platform driver?
> That's the whole purpose of the patch serie.
>
> This serie transforms the wm97xx drivers from a platform driver model to an ac97
> model, where the ac97 devices are automatically discovered. The long explanation
> is in patch 0/9, on how it started and what is the global purpose.
>
> The short story is : switch to the new AC97 bus driver model.
>
> As for the "sound specific part", it's because AC97 bus is mainly used in sound
> oriented drivers, but still the codec IPs provide more than just sound, as the
> Wolfson codecs for instance.
I'd like to get Mark Brown's opinion on this.
> >> +{
> >> + struct wm97xx_priv *wm97xx;
> >> + int ret;
> >> + void *pdata = snd_ac97_codec_get_platdata(adev);
> >> +
> >> + wm97xx = devm_kzalloc(ac97_codec_dev2dev(adev),
> >> + sizeof(*wm97xx), GFP_KERNEL);
> >> + if (!wm97xx)
> >> + return -ENOMEM;
> >> +
> >> + wm97xx->dev = ac97_codec_dev2dev(adev);
> >> + wm97xx->ac97 = snd_ac97_compat_alloc(adev);
> >> + if (IS_ERR(wm97xx->ac97))
> >> + return PTR_ERR(wm97xx->ac97);
> >> +
> >> +
> >> + ac97_set_drvdata(adev, wm97xx);
> >> + dev_info(wm97xx->dev, "wm97xx core found, id=0x%x\n",
> >> + adev->vendor_id);
> >
> > All of this ac97/sound stuff should be done in the ac97/sound driver.
>
> Nope, it's not sound adherence you're seeing here, it's ac97 bus and driver
> model adherence you're seeing. Would the bus be in drivers/ac97 instead of
> sound/ac97, the code would remain the same, would be bus be i2c you would see
> the same kind of calls but with i2c_xxx and not ac97_xxx.
>
> The wm97xx needs an ac97 bus to interact with the hardware, to provide sound,
> gpio, adc, etc ... functions. That's what is expressed here, and the fact that
> this ac97 access has to shared amongst the mfd sub-cells, and that these cells
> require :
> - wm97xx->ac97 : this one is for drivers/input/touchscreen/wm97xx-core.c
>
> So the requirement in this case is not for ac97/sound but input/touchscreen.
>
> >> diff --git a/include/linux/mfd/wm97xx.h b/include/linux/mfd/wm97xx.h
> >> new file mode 100644
> >> index 000000000000..627322f14d48
> >> --- /dev/null
> >> +++ b/include/linux/mfd/wm97xx.h
> >> @@ -0,0 +1,31 @@
> >> +/*
> >> + * wm97xx client interface
> >> + *
> >> + * Copyright (C) 2016 Robert Jarzmik
> >> + *
> >> + * 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.
> >> + *
> >> + * This program is distributed in the hope that it will be useful,
> >> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> >> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> >> + * GNU General Public License for more details.
> >> + */
> >> +
> >> +#ifndef __LINUX_MFD_WM97XX_H
> >> +#define __LINUX_MFD_WM97XX_H
> >> +
> >> +struct regmap;
> >> +struct wm97xx_batt_pdata;
> >> +struct snd_ac97;
> >
> > Why can't you just add the include files?
> I could, but I don't need to, do I ?
> Moreover, if a mfd sub-cell doesn't use regmap for example, I won't include a
> useless define.
>
> Thanks for the review, Lee. This will iterate, I'll split out mfd patch(es) to
> follow up the review with you and Mark to lessen the burden on your mailbox.
>
> Cheers.
>
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
On Thu, Nov 10, 2016 at 12:38:40PM +0100, Lars-Peter Clausen wrote:
> On 11/09/2016 10:27 PM, Robert Jarzmik wrote:
> > here, ie. that the rescan is a bug, or if it's more an "Occam's razor"
> > discussion ?
> Maybe we can just leave the rescanning out for now and think about how to
> best handle it when the need arises.
Yes, I think that's best - I suspect it'd be something that'd need to be
triggered by a driver for an AC'97 CODEC doing power management but I
imagine that if we were going to have such a system we'd have run into
it already anyway.
On Wed, Oct 26, 2016 at 09:41:38PM +0200, Robert Jarzmik wrote:
> The driving ideas are still the same, and I put them in [1] for memory. This is
> the first post RFC submission. In order to make a full demonstration of the
> framework, wm97xx was converted to an MFD, see [2] for the "why".
This all looks nice to me, I did spot a few things but I don't think I
had anything that wasn't already raised by someone else so didn't repeat
them. Thanks for stepping up and doing this work!
Hi Robert,
On Wed, Oct 26, 2016 at 09:41:44PM +0200, Robert Jarzmik wrote:
> As the power supply framework provides a way to store and retrieve
> private supply data, use it.
>
> In the process, change the platform data for wm97xx_battery from a
> container of a single struct wm97xx_batt_pdata to the direct point to wm97xx_batt_pdata.
>
> Signed-off-by: Robert Jarzmik <[email protected]>
> ---
> drivers/input/touchscreen/wm97xx-core.c | 2 +-
> drivers/power/supply/wm97xx_battery.c | 25 ++++++++++---------------
> 2 files changed, 11 insertions(+), 16 deletions(-)
I queued this into power-supply's for-next branch.
-- Sebastian
Sebastian Reichel <[email protected]> writes:
> Hi Robert,
>
> On Wed, Oct 26, 2016 at 09:41:44PM +0200, Robert Jarzmik wrote:
>> As the power supply framework provides a way to store and retrieve
>> private supply data, use it.
>>
>> In the process, change the platform data for wm97xx_battery from a
>> container of a single struct wm97xx_batt_pdata to the direct point to wm97xx_batt_pdata.
>>
>> Signed-off-by: Robert Jarzmik <[email protected]>
>> ---
>> drivers/input/touchscreen/wm97xx-core.c | 2 +-
>> drivers/power/supply/wm97xx_battery.c | 25 ++++++++++---------------
>> 2 files changed, 11 insertions(+), 16 deletions(-)
>
> I queued this into power-supply's for-next branch.
Thanks Sebastian.
Cheers.
--
Robert
Lars-Peter Clausen <[email protected]> writes:
...
Ok Lars, let's do this : I integrate as many of your remarks as I can, and I
respin this serie, limited to patches 1, 2, 3, 4 and 5.
Then we'll iterate, that will enable me to settle the serie, as I was on
holidays and lost a bit my track. I'll send it hopefully next week once I've
gathered my thoughts.
Cheers.
--
Robert
Lee Jones <[email protected]> writes:
> Mark, please see below:
You'll get better chances to have an answer if you put Mark in the To: list.
Mark, Lee has question, especially in the part where he wrote "I'd like to get
Mark Brown's opinion on this.". I added the code extract in [1] to spare you
going through the all patch.
I copy-pasted his reply below in [2], which makes it top-posting ... let's say
for this time it's acceptable.
Cheers.
--
Robert
[1] Probe function and your opinion
+static int wm97xx_ac97_probe(struct ac97_codec_device *adev)
+{
+ struct wm97xx_priv *wm97xx;
+ int ret;
+ void *pdata = snd_ac97_codec_get_platdata(adev);
+
+ wm97xx = devm_kzalloc(ac97_codec_dev2dev(adev),
+ sizeof(*wm97xx), GFP_KERNEL);
+ if (!wm97xx)
+ return -ENOMEM;
+
+ wm97xx->dev = ac97_codec_dev2dev(adev);
+ wm97xx->ac97 = snd_ac97_compat_alloc(adev);
+ if (IS_ERR(wm97xx->ac97))
+ return PTR_ERR(wm97xx->ac97);
+
+
+ ac97_set_drvdata(adev, wm97xx);
+ dev_info(wm97xx->dev, "wm97xx core found, id=0x%x\n",
+ adev->vendor_id);
[2] Lee's previous mail
> On Sat, 19 Nov 2016, Robert Jarzmik wrote:
>> Lee Jones <[email protected]> writes:
>>
>> >> +#define WM9705_VENDOR_ID 0x574d4c05
>> >> +#define WM9712_VENDOR_ID 0x574d4c12
>> >> +#define WM9713_VENDOR_ID 0x574d4c13
>> >> +#define WM97xx_VENDOR_ID_MASK 0xffffffff
>> >
>> > These are probably better represented as enums.
>> These are ids, just as devicetree ids, or PCI ids, I don't think an enum will
>> fit.
>
> That's fine. We can use enums to simply group items of a similar
> type. Representing these as enums would only serve to benefit code
> cleanliness. What makes you think they won't fit?
>
>> >> +struct wm97xx_priv {
>> >> + struct regmap *regmap;
>> >> + struct snd_ac97 *ac97;
>> >> + struct device *dev;
>> >> +};
>> >
>> > Replace _priv with _ddata.
>> Ok, will do, would you care to explain if this is something specific to your mfd
>> tree, or is it a kernel overall recommendation ?
>
> It's personal preference. But these aren't necessarily private, so
> priv doesn't really make a great deal of sense. These types of saved
> pointers are better described as device data (ddata).
>
>
> [...]
>
>> >> +static const struct reg_default wm97xx_reg_defaults[] = {
>> >> +};
>> >
>> > What's the point in this?
>> Ah, that's a reminder I have still more work on this patch ... Either I remove
>> support for wm9705 and wm9712 in the first version, or I complete it and add the
>> tables :
>> - wm9705_reg_defaults
>> - wm9712_reg_defaults
>
> Please don't add redundant code. I suggest you remove it for this
> submission.
>
>> >> +static int wm9705_register(struct wm97xx_priv *wm97xx)
>> >> +{
>> >> + return 0;
>> >> +}
>> >> +
>> >> +static int wm9712_register(struct wm97xx_priv *wm97xx)
>> >> +{
>> >> + return 0;
>> >> +}
>> >
>> > I don't get it?
>> >
>> > Either populate or don't provide.
>> Same story as above, either I complete wm9705 and wm9712 support or I remove it.
>
> Remove it please.
>
>> >> +static int wm9713_register(struct wm97xx_priv *wm97xx,
>> >> + struct wm97xx_pdata *pdata)
>> >> +{
>> >
>> > What are you lining this up with?
>> Emacs ... I don't see your point though, seeing it not aligned in the diff chunk
>> doesn't mean it's not properly aligned.
>
> That is true. So the two "scruct"s are line up in the source file,
> right?
>
> [...]
>
>> >> + codec_pdata.ac97 = wm97xx->ac97;
>> >> + codec_pdata.regmap = devm_regmap_init_ac97(wm97xx->ac97,
>> >> + &wm9713_regmap_config);
>> >> + codec_pdata.batt_pdata = pdata->batt_pdata;
>> >> + if (IS_ERR(codec_pdata.regmap))
>> >> + return PTR_ERR(codec_pdata.regmap);
>> >
>> > This doesn't look like pdata. You can get rid of all of this hoop
>> > jumping if you add it to ddata and set it as such.
>> You mean I should pass ddata to mfd sub-cells, right ?
>
> Correct.
>
> [...]
>
>> >> +static int wm97xx_ac97_probe(struct ac97_codec_device *adev)
>> >
>> > This looks sound specific.
>> >
>> > Why isn't this a plane platform driver?
>> That's the whole purpose of the patch serie.
>>
>> This serie transforms the wm97xx drivers from a platform driver model to an ac97
>> model, where the ac97 devices are automatically discovered. The long explanation
>> is in patch 0/9, on how it started and what is the global purpose.
>>
>> The short story is : switch to the new AC97 bus driver model.
>>
>> As for the "sound specific part", it's because AC97 bus is mainly used in sound
>> oriented drivers, but still the codec IPs provide more than just sound, as the
>> Wolfson codecs for instance.
>
> I'd like to get Mark Brown's opinion on this.
>
>> >> +{
>> >> + struct wm97xx_priv *wm97xx;
>> >> + int ret;
>> >> + void *pdata = snd_ac97_codec_get_platdata(adev);
>> >> +
>> >> + wm97xx = devm_kzalloc(ac97_codec_dev2dev(adev),
>> >> + sizeof(*wm97xx), GFP_KERNEL);
>> >> + if (!wm97xx)
>> >> + return -ENOMEM;
>> >> +
>> >> + wm97xx->dev = ac97_codec_dev2dev(adev);
>> >> + wm97xx->ac97 = snd_ac97_compat_alloc(adev);
>> >> + if (IS_ERR(wm97xx->ac97))
>> >> + return PTR_ERR(wm97xx->ac97);
>> >> +
>> >> +
>> >> + ac97_set_drvdata(adev, wm97xx);
>> >> + dev_info(wm97xx->dev, "wm97xx core found, id=0x%x\n",
>> >> + adev->vendor_id);
>> >
>> > All of this ac97/sound stuff should be done in the ac97/sound driver.
>>
>> Nope, it's not sound adherence you're seeing here, it's ac97 bus and driver
>> model adherence you're seeing. Would the bus be in drivers/ac97 instead of
>> sound/ac97, the code would remain the same, would be bus be i2c you would see
>> the same kind of calls but with i2c_xxx and not ac97_xxx.
>>
>> The wm97xx needs an ac97 bus to interact with the hardware, to provide sound,
>> gpio, adc, etc ... functions. That's what is expressed here, and the fact that
>> this ac97 access has to shared amongst the mfd sub-cells, and that these cells
>> require :
>> - wm97xx->ac97 : this one is for drivers/input/touchscreen/wm97xx-core.c
>>
>> So the requirement in this case is not for ac97/sound but input/touchscreen.
>>
>> >> diff --git a/include/linux/mfd/wm97xx.h b/include/linux/mfd/wm97xx.h
>> >> new file mode 100644
>> >> index 000000000000..627322f14d48
>> >> --- /dev/null
>> >> +++ b/include/linux/mfd/wm97xx.h
>> >> @@ -0,0 +1,31 @@
>> >> +/*
>> >> + * wm97xx client interface
>> >> + *
>> >> + * Copyright (C) 2016 Robert Jarzmik
>> >> + *
>> >> + * 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.
>> >> + *
>> >> + * This program is distributed in the hope that it will be useful,
>> >> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> >> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
>> >> + * GNU General Public License for more details.
>> >> + */
>> >> +
>> >> +#ifndef __LINUX_MFD_WM97XX_H
>> >> +#define __LINUX_MFD_WM97XX_H
>> >> +
>> >> +struct regmap;
>> >> +struct wm97xx_batt_pdata;
>> >> +struct snd_ac97;
>> >
>> > Why can't you just add the include files?
>> I could, but I don't need to, do I ?
>> Moreover, if a mfd sub-cell doesn't use regmap for example, I won't include a
>> useless define.
>>
>> Thanks for the review, Lee. This will iterate, I'll split out mfd patch(es) to
>> follow up the review with you and Mark to lessen the burden on your mailbox.
>>
>> Cheers.
>>
On Sat, Nov 26, 2016 at 10:18:38AM +0100, Robert Jarzmik wrote:
> >> >> +#define WM9705_VENDOR_ID 0x574d4c05
> >> >> +#define WM9712_VENDOR_ID 0x574d4c12
> >> >> +#define WM9713_VENDOR_ID 0x574d4c13
> >> >> +#define WM97xx_VENDOR_ID_MASK 0xffffffff
> >> > These are probably better represented as enums.
> >> These are ids, just as devicetree ids, or PCI ids, I don't think an enum will
> >> fit.
> > That's fine. We can use enums to simply group items of a similar
> > type. Representing these as enums would only serve to benefit code
> > cleanliness. What makes you think they won't fit?
That would be like having all PCI or USB identifiers in an enum, it's
doable but it means having one big table that all drivers need to
modify.
> >> >> +static int wm97xx_ac97_probe(struct ac97_codec_device *adev)
> >> As for the "sound specific part", it's because AC97 bus is mainly used in sound
> >> oriented drivers, but still the codec IPs provide more than just sound, as the
> >> Wolfson codecs for instance.
> > I'd like to get Mark Brown's opinion on this.
I'm not sure what the question is, sorry.
The patch
Input: wm97xx: add new AC97 bus support
has been applied to the asoc tree at
git://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git
All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.
You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.
If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.
Please add any relevant lists and maintainers to the CCs when replying
to this mail.
Thanks,
Mark
>From ae9d1b5fbd7b7ee1c2d2f62a7cee3c0455e06f94 Mon Sep 17 00:00:00 2001
From: Robert Jarzmik <[email protected]>
Date: Wed, 13 Sep 2017 21:37:18 +0200
Subject: [PATCH] Input: wm97xx: add new AC97 bus support
This adds support for the new AC97 bus code, which discovers the devices
rather than uses platform data.
As part of this discovery, it enables a multi-function device wm97xx,
which supports touchscreen, battery, ADC and an audio codec. This patch
adds the code to bind the touchscreen "cell" as the touchscreen driver.
This was tested on the pxa architecture with a pxa270 + wm9713 + the
mioa701 touchscreen.
Signed-off-by: Robert Jarzmik <[email protected]>
Acked-by: Charles Keepax <[email protected]>
Acked-by: Dmitry Torokhov <[email protected]>
Signed-off-by: Mark Brown <[email protected]>
---
drivers/input/touchscreen/Kconfig | 2 +-
drivers/input/touchscreen/wm97xx-core.c | 56 ++++++++++++++++++++++++++++++++-
2 files changed, 56 insertions(+), 2 deletions(-)
diff --git a/drivers/input/touchscreen/Kconfig b/drivers/input/touchscreen/Kconfig
index 64b30fe273fd..176b1a74b2b7 100644
--- a/drivers/input/touchscreen/Kconfig
+++ b/drivers/input/touchscreen/Kconfig
@@ -727,7 +727,7 @@ config TOUCHSCREEN_WM831X
config TOUCHSCREEN_WM97XX
tristate "Support for WM97xx AC97 touchscreen controllers"
- depends on AC97_BUS
+ depends on AC97_BUS || AC97_BUS_NEW
help
Say Y here if you have a Wolfson Microelectronics WM97xx
touchscreen connected to your system. Note that this option
diff --git a/drivers/input/touchscreen/wm97xx-core.c b/drivers/input/touchscreen/wm97xx-core.c
index 39869ffdc4fa..fd714ee881f7 100644
--- a/drivers/input/touchscreen/wm97xx-core.c
+++ b/drivers/input/touchscreen/wm97xx-core.c
@@ -44,6 +44,7 @@
#include <linux/pm.h>
#include <linux/interrupt.h>
#include <linux/bitops.h>
+#include <linux/mfd/wm97xx.h>
#include <linux/workqueue.h>
#include <linux/wm97xx.h>
#include <linux/uaccess.h>
@@ -766,6 +767,39 @@ static int wm97xx_remove(struct device *dev)
return 0;
}
+static int wm97xx_mfd_probe(struct platform_device *pdev)
+{
+ struct wm97xx *wm;
+ struct wm97xx_platform_data *mfd_pdata = dev_get_platdata(&pdev->dev);
+ int ret;
+
+ wm = devm_kzalloc(&pdev->dev, sizeof(struct wm97xx), GFP_KERNEL);
+ if (!wm)
+ return -ENOMEM;
+
+ wm->dev = &pdev->dev;
+ wm->ac97 = mfd_pdata->ac97;
+
+ ret = _wm97xx_probe(wm);
+ if (ret)
+ return ret;
+
+ ret = wm97xx_add_battery(wm, mfd_pdata->batt_pdata);
+ if (ret < 0)
+ goto batt_err;
+
+ return ret;
+
+batt_err:
+ wm97xx_unregister_touch(wm);
+ return ret;
+}
+
+static int wm97xx_mfd_remove(struct platform_device *pdev)
+{
+ return wm97xx_remove(&pdev->dev);
+}
+
static int __maybe_unused wm97xx_suspend(struct device *dev)
{
struct wm97xx *wm = dev_get_drvdata(dev);
@@ -862,21 +896,41 @@ EXPORT_SYMBOL_GPL(wm97xx_unregister_mach_ops);
static struct device_driver wm97xx_driver = {
.name = "wm97xx-ts",
+#ifdef CONFIG_AC97_BUS
.bus = &ac97_bus_type,
+#endif
.owner = THIS_MODULE,
.probe = wm97xx_probe,
.remove = wm97xx_remove,
.pm = &wm97xx_pm_ops,
};
+static struct platform_driver wm97xx_mfd_driver = {
+ .driver = {
+ .name = "wm97xx-ts",
+ .pm = &wm97xx_pm_ops,
+ },
+ .probe = wm97xx_mfd_probe,
+ .remove = wm97xx_mfd_remove,
+};
+
static int __init wm97xx_init(void)
{
- return driver_register(&wm97xx_driver);
+ int ret;
+
+ ret = platform_driver_register(&wm97xx_mfd_driver);
+ if (ret)
+ return ret;
+
+ if (IS_BUILTIN(CONFIG_AC97_BUS))
+ ret = driver_register(&wm97xx_driver);
+ return ret;
}
static void __exit wm97xx_exit(void)
{
driver_unregister(&wm97xx_driver);
+ platform_driver_unregister(&wm97xx_mfd_driver);
}
module_init(wm97xx_init);
--
2.14.1