diff mbox series

[v2,2/5] mfd: lochnagar: Add support for the Cirrus Logic Lochnagar

Message ID 20181008132542.19775-2-ckeepax@opensource.cirrus.com (mailing list archive)
State Not Applicable, archived
Headers show
Series [v2,1/5] mfd: lochnagar: Add initial binding documentation | expand

Commit Message

Charles Keepax Oct. 8, 2018, 1:25 p.m. UTC
From: Charles Keepax <ckeepax@opensource.wolfsonmicro.com>

Lochnagar is an evaluation and development board for Cirrus
Logic Smart CODEC and Amp devices. It allows the connection of
most Cirrus Logic devices on mini-cards, as well as allowing
connection of various application processor systems to provide a
full evaluation platform. This driver supports the board
controller chip on the Lochnagar board. Audio system topology,
clocking and power can all be controlled through the Lochnagar
controller chip, allowing the device under test to be used in
a variety of possible use cases.

As the Lochnagar is a fairly complex device this MFD driver
allows the drivers for the various features to be bound
in. Initially clocking, regulator and pinctrl will be added as
these are necessary to configure the system. But in time at least
audio and voltage/current monitoring will also be added.

Signed-off-by: Charles Keepax <ckeepax@opensource.wolfsonmicro.com>
---

Changes since v1:
 - Update Kconfig to correctly specify dependencies
 - Update commit message a little

Thanks,
Charles

 MAINTAINERS                         |  13 +
 drivers/mfd/Kconfig                 |   8 +
 drivers/mfd/Makefile                |   2 +
 drivers/mfd/lochnagar-i2c.c         | 732 ++++++++++++++++++++++++++++++++++++
 include/linux/mfd/lochnagar.h       |  48 +++
 include/linux/mfd/lochnagar1_regs.h | 157 ++++++++
 include/linux/mfd/lochnagar2_regs.h | 253 +++++++++++++
 7 files changed, 1213 insertions(+)
 create mode 100644 drivers/mfd/lochnagar-i2c.c
 create mode 100644 include/linux/mfd/lochnagar.h
 create mode 100644 include/linux/mfd/lochnagar1_regs.h
 create mode 100644 include/linux/mfd/lochnagar2_regs.h

Comments

Lee Jones Oct. 25, 2018, 7:44 a.m. UTC | #1
On Mon, 08 Oct 2018, Charles Keepax wrote:

> From: Charles Keepax <ckeepax@opensource.wolfsonmicro.com>
> 
> Lochnagar is an evaluation and development board for Cirrus
> Logic Smart CODEC and Amp devices. It allows the connection of
> most Cirrus Logic devices on mini-cards, as well as allowing
> connection of various application processor systems to provide a
> full evaluation platform. This driver supports the board
> controller chip on the Lochnagar board. Audio system topology,
> clocking and power can all be controlled through the Lochnagar
> controller chip, allowing the device under test to be used in
> a variety of possible use cases.
> 
> As the Lochnagar is a fairly complex device this MFD driver
> allows the drivers for the various features to be bound
> in. Initially clocking, regulator and pinctrl will be added as
> these are necessary to configure the system. But in time at least
> audio and voltage/current monitoring will also be added.
> 
> Signed-off-by: Charles Keepax <ckeepax@opensource.wolfsonmicro.com>
> ---
> 
> Changes since v1:
>  - Update Kconfig to correctly specify dependencies
>  - Update commit message a little
> 
> Thanks,
> Charles
> 
>  MAINTAINERS                         |  13 +
>  drivers/mfd/Kconfig                 |   8 +
>  drivers/mfd/Makefile                |   2 +
>  drivers/mfd/lochnagar-i2c.c         | 732 ++++++++++++++++++++++++++++++++++++
>  include/linux/mfd/lochnagar.h       |  48 +++
>  include/linux/mfd/lochnagar1_regs.h | 157 ++++++++
>  include/linux/mfd/lochnagar2_regs.h | 253 +++++++++++++
>  7 files changed, 1213 insertions(+)
>  create mode 100644 drivers/mfd/lochnagar-i2c.c
>  create mode 100644 include/linux/mfd/lochnagar.h
>  create mode 100644 include/linux/mfd/lochnagar1_regs.h
>  create mode 100644 include/linux/mfd/lochnagar2_regs.h
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index d9e6d86488df4..f1f3494ac5cd8 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -3557,6 +3557,19 @@ L:	netdev@vger.kernel.org
>  S:	Maintained
>  F:	drivers/net/ethernet/cirrus/ep93xx_eth.c
>  
> +CIRRUS LOGIC LOCHNAGAR DRIVER
> +M:	Charles Keepax <ckeepax@opensource.cirrus.com>
> +M:	Richard Fitzgerald <rf@opensource.cirrus.com>
> +L:	patches@opensource.cirrus.com
> +S:	Supported
> +F:	drivers/clk/clk-lochnagar.c
> +F:	drivers/mfd/lochnagar-i2c.c
> +F:	drivers/pinctrl/cirrus/pinctrl-lochnagar.c
> +F:	drivers/regulator/lochnagar-regulator.c
> +F:	include/dt-bindings/clk/lochnagar.h
> +F:	include/dt-bindings/pinctrl/lochnagar.h
> +F:	include/linux/mfd/lochnagar*
> +
>  CISCO FCOE HBA DRIVER
>  M:	Satish Kharat <satishkh@cisco.com>
>  M:	Sesidhar Baddela <sebaddel@cisco.com>
> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> index f3a5f8d027906..dc64151373714 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -1676,6 +1676,14 @@ config MFD_VX855
>  	  VIA VX855/VX875 south bridge. You will need to enable the vx855_spi
>  	  and/or vx855_gpio drivers for this to do anything useful.
>  
> +config MFD_LOCHNAGAR
> +	bool "Cirrus Logic Lochnagar Audio Development Board"
> +	select MFD_CORE
> +	select REGMAP_I2C
> +	depends on I2C=y && OF
> +	help
> +	  Support for Cirrus Logic Lochnagar audio development board.
> +
>  config MFD_ARIZONA
>  	select REGMAP
>  	select REGMAP_IRQ
> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> index 5856a9489cbd8..f16773cb887ff 100644
> --- a/drivers/mfd/Makefile
> +++ b/drivers/mfd/Makefile
> @@ -37,6 +37,8 @@ obj-$(CONFIG_MFD_T7L66XB)	+= t7l66xb.o tmio_core.o
>  obj-$(CONFIG_MFD_TC6387XB)	+= tc6387xb.o tmio_core.o
>  obj-$(CONFIG_MFD_TC6393XB)	+= tc6393xb.o tmio_core.o
>  
> +obj-$(CONFIG_MFD_LOCHNAGAR)	+= lochnagar-i2c.o
> +
>  obj-$(CONFIG_MFD_ARIZONA)	+= arizona-core.o
>  obj-$(CONFIG_MFD_ARIZONA)	+= arizona-irq.o
>  obj-$(CONFIG_MFD_ARIZONA_I2C)	+= arizona-i2c.o
> diff --git a/drivers/mfd/lochnagar-i2c.c b/drivers/mfd/lochnagar-i2c.c
> new file mode 100644
> index 0000000000000..7ac7af9e3130b
> --- /dev/null
> +++ b/drivers/mfd/lochnagar-i2c.c
> @@ -0,0 +1,732 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Lochnagar I2C bus interface
> + *
> + * Copyright (c) 2012-2018 Cirrus Logic, Inc. and
> + *                         Cirrus Logic International Semiconductor Ltd.
> + *
> + * Author: Charles Keepax <ckeepax@opensource.cirrus.com>
> + */
> +
> +#include <linux/delay.h>
> +#include <linux/device.h>
> +#include <linux/err.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/i2c.h>
> +#include <linux/mfd/core.h>
> +#include <linux/mutex.h>
> +#include <linux/of.h>
> +#include <linux/of_platform.h>
> +#include <linux/regmap.h>
> +
> +#include <linux/mfd/lochnagar.h>
> +
> +#define LOCHNAGAR_BOOT_RETRIES		10
> +#define LOCHNAGAR_BOOT_DELAY_MS		350
> +
> +#define LOCHNAGAR_CONFIG_POLL_US	10000
> +
> +static bool lochnagar1_readable_register(struct device *dev, unsigned int reg)
> +{
> +	switch (reg) {
> +	case LOCHNAGAR_SOFTWARE_RESET:
> +	case LOCHNAGAR_FIRMWARE_ID1:
> +	case LOCHNAGAR_FIRMWARE_ID2:
> +	case LOCHNAGAR1_CDC_AIF1_SEL:
> +	case LOCHNAGAR1_CDC_AIF2_SEL:
> +	case LOCHNAGAR1_CDC_AIF3_SEL:
> +	case LOCHNAGAR1_CDC_MCLK1_SEL:
> +	case LOCHNAGAR1_CDC_MCLK2_SEL:
> +	case LOCHNAGAR1_CDC_AIF_CTRL1:
> +	case LOCHNAGAR1_CDC_AIF_CTRL2:
> +	case LOCHNAGAR1_EXT_AIF_CTRL:
> +	case LOCHNAGAR1_DSP_AIF1_SEL:
> +	case LOCHNAGAR1_DSP_AIF2_SEL:
> +	case LOCHNAGAR1_DSP_CLKIN_SEL:
> +	case LOCHNAGAR1_DSP_AIF:
> +	case LOCHNAGAR1_GF_AIF1:
> +	case LOCHNAGAR1_GF_AIF2:
> +	case LOCHNAGAR1_PSIA_AIF:
> +	case LOCHNAGAR1_PSIA1_SEL:
> +	case LOCHNAGAR1_PSIA2_SEL:
> +	case LOCHNAGAR1_SPDIF_AIF_SEL:
> +	case LOCHNAGAR1_GF_AIF3_SEL:
> +	case LOCHNAGAR1_GF_AIF4_SEL:
> +	case LOCHNAGAR1_GF_CLKOUT1_SEL:
> +	case LOCHNAGAR1_GF_AIF1_SEL:
> +	case LOCHNAGAR1_GF_AIF2_SEL:
> +	case LOCHNAGAR1_GF_GPIO2:
> +	case LOCHNAGAR1_GF_GPIO3:
> +	case LOCHNAGAR1_GF_GPIO7:
> +	case LOCHNAGAR1_RST:
> +	case LOCHNAGAR1_LED1:
> +	case LOCHNAGAR1_LED2:
> +	case LOCHNAGAR1_I2C_CTRL:
> +		return true;
> +	default:
> +		return false;
> +	}
> +}
> +
> +static const struct reg_default lochnagar1_reg_defaults[] = {
> +	{ LOCHNAGAR1_CDC_AIF1_SEL,    0x00 },
> +	{ LOCHNAGAR1_CDC_AIF2_SEL,    0x00 },
> +	{ LOCHNAGAR1_CDC_AIF3_SEL,    0x00 },
> +	{ LOCHNAGAR1_CDC_MCLK1_SEL,   0x00 },
> +	{ LOCHNAGAR1_CDC_MCLK2_SEL,   0x00 },
> +	{ LOCHNAGAR1_CDC_AIF_CTRL1,   0x00 },
> +	{ LOCHNAGAR1_CDC_AIF_CTRL2,   0x00 },
> +	{ LOCHNAGAR1_EXT_AIF_CTRL,    0x00 },
> +	{ LOCHNAGAR1_DSP_AIF1_SEL,    0x00 },
> +	{ LOCHNAGAR1_DSP_AIF2_SEL,    0x00 },
> +	{ LOCHNAGAR1_DSP_CLKIN_SEL,   0x01 },
> +	{ LOCHNAGAR1_DSP_AIF,         0x08 },
> +	{ LOCHNAGAR1_GF_AIF1,         0x00 },
> +	{ LOCHNAGAR1_GF_AIF2,         0x00 },
> +	{ LOCHNAGAR1_PSIA_AIF,        0x00 },
> +	{ LOCHNAGAR1_PSIA1_SEL,       0x00 },
> +	{ LOCHNAGAR1_PSIA2_SEL,       0x00 },
> +	{ LOCHNAGAR1_SPDIF_AIF_SEL,   0x00 },
> +	{ LOCHNAGAR1_GF_AIF3_SEL,     0x00 },
> +	{ LOCHNAGAR1_GF_AIF4_SEL,     0x00 },
> +	{ LOCHNAGAR1_GF_CLKOUT1_SEL,  0x00 },
> +	{ LOCHNAGAR1_GF_AIF1_SEL,     0x00 },
> +	{ LOCHNAGAR1_GF_AIF2_SEL,     0x00 },
> +	{ LOCHNAGAR1_GF_GPIO2,        0x00 },
> +	{ LOCHNAGAR1_GF_GPIO3,        0x00 },
> +	{ LOCHNAGAR1_GF_GPIO7,        0x00 },
> +	{ LOCHNAGAR1_RST,             0x00 },
> +	{ LOCHNAGAR1_LED1,            0x00 },
> +	{ LOCHNAGAR1_LED2,            0x00 },
> +	{ LOCHNAGAR1_I2C_CTRL,        0x01 },
> +};

Why do you need to specify each register value?

> +static const struct regmap_config lochnagar1_i2c_regmap = {
> +	.reg_bits = 8,
> +	.val_bits = 8,
> +	.reg_format_endian = REGMAP_ENDIAN_BIG,
> +	.val_format_endian = REGMAP_ENDIAN_BIG,
> +
> +	.max_register = 0x50,
> +	.readable_reg = lochnagar1_readable_register,
> +
> +	.use_single_rw = true,
> +
> +	.cache_type = REGCACHE_RBTREE,
> +
> +	.reg_defaults = lochnagar1_reg_defaults,
> +	.num_reg_defaults = ARRAY_SIZE(lochnagar1_reg_defaults),
> +};
> +
> +static const struct reg_sequence lochnagar1_patch[] = {
> +	{ 0x40, 0x0083 },
> +	{ 0x46, 0x0001 },
> +	{ 0x47, 0x0018 },
> +	{ 0x50, 0x0000 },
> +};

I'm really not a fan of these so call 'patches'.

Can't you set the registers up proper way?

> +static struct mfd_cell lochnagar1_devs[] = {
> +	{
> +		.name = "lochnagar-pinctrl"
> +	},
> +	{
> +		.name = "lochnagar-clk"
> +	},

This should each be on a single line.

> +};
> +
> +static bool lochnagar2_readable_register(struct device *dev, unsigned int reg)
> +{
> +	switch (reg) {
> +	case LOCHNAGAR_SOFTWARE_RESET:
> +	case LOCHNAGAR_FIRMWARE_ID1:
> +	case LOCHNAGAR_FIRMWARE_ID2:
> +	case LOCHNAGAR2_CDC_AIF1_CTRL:
> +	case LOCHNAGAR2_CDC_AIF2_CTRL:
> +	case LOCHNAGAR2_CDC_AIF3_CTRL:
> +	case LOCHNAGAR2_DSP_AIF1_CTRL:
> +	case LOCHNAGAR2_DSP_AIF2_CTRL:
> +	case LOCHNAGAR2_PSIA1_CTRL:
> +	case LOCHNAGAR2_PSIA2_CTRL:
> +	case LOCHNAGAR2_GF_AIF3_CTRL:
> +	case LOCHNAGAR2_GF_AIF4_CTRL:
> +	case LOCHNAGAR2_GF_AIF1_CTRL:
> +	case LOCHNAGAR2_GF_AIF2_CTRL:
> +	case LOCHNAGAR2_SPDIF_AIF_CTRL:
> +	case LOCHNAGAR2_USB_AIF1_CTRL:
> +	case LOCHNAGAR2_USB_AIF2_CTRL:
> +	case LOCHNAGAR2_ADAT_AIF_CTRL:
> +	case LOCHNAGAR2_CDC_MCLK1_CTRL:
> +	case LOCHNAGAR2_CDC_MCLK2_CTRL:
> +	case LOCHNAGAR2_DSP_CLKIN_CTRL:
> +	case LOCHNAGAR2_PSIA1_MCLK_CTRL:
> +	case LOCHNAGAR2_PSIA2_MCLK_CTRL:
> +	case LOCHNAGAR2_SPDIF_MCLK_CTRL:
> +	case LOCHNAGAR2_GF_CLKOUT1_CTRL:
> +	case LOCHNAGAR2_GF_CLKOUT2_CTRL:
> +	case LOCHNAGAR2_ADAT_MCLK_CTRL:
> +	case LOCHNAGAR2_SOUNDCARD_MCLK_CTRL:
> +	case LOCHNAGAR2_GPIO_FPGA_GPIO1:
> +	case LOCHNAGAR2_GPIO_FPGA_GPIO2:
> +	case LOCHNAGAR2_GPIO_FPGA_GPIO3:
> +	case LOCHNAGAR2_GPIO_FPGA_GPIO4:
> +	case LOCHNAGAR2_GPIO_FPGA_GPIO5:
> +	case LOCHNAGAR2_GPIO_FPGA_GPIO6:
> +	case LOCHNAGAR2_GPIO_CDC_GPIO1:
> +	case LOCHNAGAR2_GPIO_CDC_GPIO2:
> +	case LOCHNAGAR2_GPIO_CDC_GPIO3:
> +	case LOCHNAGAR2_GPIO_CDC_GPIO4:
> +	case LOCHNAGAR2_GPIO_CDC_GPIO5:
> +	case LOCHNAGAR2_GPIO_CDC_GPIO6:
> +	case LOCHNAGAR2_GPIO_CDC_GPIO7:
> +	case LOCHNAGAR2_GPIO_CDC_GPIO8:
> +	case LOCHNAGAR2_GPIO_DSP_GPIO1:
> +	case LOCHNAGAR2_GPIO_DSP_GPIO2:
> +	case LOCHNAGAR2_GPIO_DSP_GPIO3:
> +	case LOCHNAGAR2_GPIO_DSP_GPIO4:
> +	case LOCHNAGAR2_GPIO_DSP_GPIO5:
> +	case LOCHNAGAR2_GPIO_DSP_GPIO6:
> +	case LOCHNAGAR2_GPIO_GF_GPIO2:
> +	case LOCHNAGAR2_GPIO_GF_GPIO3:
> +	case LOCHNAGAR2_GPIO_GF_GPIO7:
> +	case LOCHNAGAR2_GPIO_CDC_AIF1_BCLK:
> +	case LOCHNAGAR2_GPIO_CDC_AIF1_RXDAT:
> +	case LOCHNAGAR2_GPIO_CDC_AIF1_LRCLK:
> +	case LOCHNAGAR2_GPIO_CDC_AIF1_TXDAT:
> +	case LOCHNAGAR2_GPIO_CDC_AIF2_BCLK:
> +	case LOCHNAGAR2_GPIO_CDC_AIF2_RXDAT:
> +	case LOCHNAGAR2_GPIO_CDC_AIF2_LRCLK:
> +	case LOCHNAGAR2_GPIO_CDC_AIF2_TXDAT:
> +	case LOCHNAGAR2_GPIO_CDC_AIF3_BCLK:
> +	case LOCHNAGAR2_GPIO_CDC_AIF3_RXDAT:
> +	case LOCHNAGAR2_GPIO_CDC_AIF3_LRCLK:
> +	case LOCHNAGAR2_GPIO_CDC_AIF3_TXDAT:
> +	case LOCHNAGAR2_GPIO_DSP_AIF1_BCLK:
> +	case LOCHNAGAR2_GPIO_DSP_AIF1_RXDAT:
> +	case LOCHNAGAR2_GPIO_DSP_AIF1_LRCLK:
> +	case LOCHNAGAR2_GPIO_DSP_AIF1_TXDAT:
> +	case LOCHNAGAR2_GPIO_DSP_AIF2_BCLK:
> +	case LOCHNAGAR2_GPIO_DSP_AIF2_RXDAT:
> +	case LOCHNAGAR2_GPIO_DSP_AIF2_LRCLK:
> +	case LOCHNAGAR2_GPIO_DSP_AIF2_TXDAT:
> +	case LOCHNAGAR2_GPIO_PSIA1_BCLK:
> +	case LOCHNAGAR2_GPIO_PSIA1_RXDAT:
> +	case LOCHNAGAR2_GPIO_PSIA1_LRCLK:
> +	case LOCHNAGAR2_GPIO_PSIA1_TXDAT:
> +	case LOCHNAGAR2_GPIO_PSIA2_BCLK:
> +	case LOCHNAGAR2_GPIO_PSIA2_RXDAT:
> +	case LOCHNAGAR2_GPIO_PSIA2_LRCLK:
> +	case LOCHNAGAR2_GPIO_PSIA2_TXDAT:
> +	case LOCHNAGAR2_GPIO_GF_AIF3_BCLK:
> +	case LOCHNAGAR2_GPIO_GF_AIF3_RXDAT:
> +	case LOCHNAGAR2_GPIO_GF_AIF3_LRCLK:
> +	case LOCHNAGAR2_GPIO_GF_AIF3_TXDAT:
> +	case LOCHNAGAR2_GPIO_GF_AIF4_BCLK:
> +	case LOCHNAGAR2_GPIO_GF_AIF4_RXDAT:
> +	case LOCHNAGAR2_GPIO_GF_AIF4_LRCLK:
> +	case LOCHNAGAR2_GPIO_GF_AIF4_TXDAT:
> +	case LOCHNAGAR2_GPIO_GF_AIF1_BCLK:
> +	case LOCHNAGAR2_GPIO_GF_AIF1_RXDAT:
> +	case LOCHNAGAR2_GPIO_GF_AIF1_LRCLK:
> +	case LOCHNAGAR2_GPIO_GF_AIF1_TXDAT:
> +	case LOCHNAGAR2_GPIO_GF_AIF2_BCLK:
> +	case LOCHNAGAR2_GPIO_GF_AIF2_RXDAT:
> +	case LOCHNAGAR2_GPIO_GF_AIF2_LRCLK:
> +	case LOCHNAGAR2_GPIO_GF_AIF2_TXDAT:
> +	case LOCHNAGAR2_GPIO_DSP_UART1_RX:
> +	case LOCHNAGAR2_GPIO_DSP_UART1_TX:
> +	case LOCHNAGAR2_GPIO_DSP_UART2_RX:
> +	case LOCHNAGAR2_GPIO_DSP_UART2_TX:
> +	case LOCHNAGAR2_GPIO_GF_UART2_RX:
> +	case LOCHNAGAR2_GPIO_GF_UART2_TX:
> +	case LOCHNAGAR2_GPIO_USB_UART_RX:
> +	case LOCHNAGAR2_GPIO_CDC_PDMCLK1:
> +	case LOCHNAGAR2_GPIO_CDC_PDMDAT1:
> +	case LOCHNAGAR2_GPIO_CDC_PDMCLK2:
> +	case LOCHNAGAR2_GPIO_CDC_PDMDAT2:
> +	case LOCHNAGAR2_GPIO_CDC_DMICCLK1:
> +	case LOCHNAGAR2_GPIO_CDC_DMICDAT1:
> +	case LOCHNAGAR2_GPIO_CDC_DMICCLK2:
> +	case LOCHNAGAR2_GPIO_CDC_DMICDAT2:
> +	case LOCHNAGAR2_GPIO_CDC_DMICCLK3:
> +	case LOCHNAGAR2_GPIO_CDC_DMICDAT3:
> +	case LOCHNAGAR2_GPIO_CDC_DMICCLK4:
> +	case LOCHNAGAR2_GPIO_CDC_DMICDAT4:
> +	case LOCHNAGAR2_GPIO_DSP_DMICCLK1:
> +	case LOCHNAGAR2_GPIO_DSP_DMICDAT1:
> +	case LOCHNAGAR2_GPIO_DSP_DMICCLK2:
> +	case LOCHNAGAR2_GPIO_DSP_DMICDAT2:
> +	case LOCHNAGAR2_GPIO_I2C2_SCL:
> +	case LOCHNAGAR2_GPIO_I2C2_SDA:
> +	case LOCHNAGAR2_GPIO_I2C3_SCL:
> +	case LOCHNAGAR2_GPIO_I2C3_SDA:
> +	case LOCHNAGAR2_GPIO_I2C4_SCL:
> +	case LOCHNAGAR2_GPIO_I2C4_SDA:
> +	case LOCHNAGAR2_GPIO_DSP_STANDBY:
> +	case LOCHNAGAR2_GPIO_CDC_MCLK1:
> +	case LOCHNAGAR2_GPIO_CDC_MCLK2:
> +	case LOCHNAGAR2_GPIO_DSP_CLKIN:
> +	case LOCHNAGAR2_GPIO_PSIA1_MCLK:
> +	case LOCHNAGAR2_GPIO_PSIA2_MCLK:
> +	case LOCHNAGAR2_GPIO_GF_GPIO1:
> +	case LOCHNAGAR2_GPIO_GF_GPIO5:
> +	case LOCHNAGAR2_GPIO_DSP_GPIO20:
> +	case LOCHNAGAR2_GPIO_CHANNEL1:
> +	case LOCHNAGAR2_GPIO_CHANNEL2:
> +	case LOCHNAGAR2_GPIO_CHANNEL3:
> +	case LOCHNAGAR2_GPIO_CHANNEL4:
> +	case LOCHNAGAR2_GPIO_CHANNEL5:
> +	case LOCHNAGAR2_GPIO_CHANNEL6:
> +	case LOCHNAGAR2_GPIO_CHANNEL7:
> +	case LOCHNAGAR2_GPIO_CHANNEL8:
> +	case LOCHNAGAR2_GPIO_CHANNEL9:
> +	case LOCHNAGAR2_GPIO_CHANNEL10:
> +	case LOCHNAGAR2_GPIO_CHANNEL11:
> +	case LOCHNAGAR2_GPIO_CHANNEL12:
> +	case LOCHNAGAR2_GPIO_CHANNEL13:
> +	case LOCHNAGAR2_GPIO_CHANNEL14:
> +	case LOCHNAGAR2_GPIO_CHANNEL15:
> +	case LOCHNAGAR2_GPIO_CHANNEL16:
> +	case LOCHNAGAR2_MINICARD_RESETS:
> +	case LOCHNAGAR2_ANALOGUE_PATH_CTRL1:
> +	case LOCHNAGAR2_ANALOGUE_PATH_CTRL2:
> +	case LOCHNAGAR2_COMMS_CTRL4:
> +	case LOCHNAGAR2_SPDIF_CTRL:
> +	case LOCHNAGAR2_POWER_CTRL:
> +	case LOCHNAGAR2_MICVDD_CTRL1:
> +	case LOCHNAGAR2_MICVDD_CTRL2:
> +	case LOCHNAGAR2_VDDCORE_CDC_CTRL1:
> +	case LOCHNAGAR2_VDDCORE_CDC_CTRL2:
> +	case LOCHNAGAR2_SOUNDCARD_AIF_CTRL:
> +		return true;
> +	default:
> +		return false;
> +	}
> +}
> +
> +static bool lochnagar2_volatile_register(struct device *dev, unsigned int reg)
> +{
> +	switch (reg) {
> +	case LOCHNAGAR2_GPIO_CHANNEL1:
> +	case LOCHNAGAR2_GPIO_CHANNEL2:
> +	case LOCHNAGAR2_GPIO_CHANNEL3:
> +	case LOCHNAGAR2_GPIO_CHANNEL4:
> +	case LOCHNAGAR2_GPIO_CHANNEL5:
> +	case LOCHNAGAR2_GPIO_CHANNEL6:
> +	case LOCHNAGAR2_GPIO_CHANNEL7:
> +	case LOCHNAGAR2_GPIO_CHANNEL8:
> +	case LOCHNAGAR2_GPIO_CHANNEL9:
> +	case LOCHNAGAR2_GPIO_CHANNEL10:
> +	case LOCHNAGAR2_GPIO_CHANNEL11:
> +	case LOCHNAGAR2_GPIO_CHANNEL12:
> +	case LOCHNAGAR2_GPIO_CHANNEL13:
> +	case LOCHNAGAR2_GPIO_CHANNEL14:
> +	case LOCHNAGAR2_GPIO_CHANNEL15:
> +	case LOCHNAGAR2_GPIO_CHANNEL16:
> +	case LOCHNAGAR2_ANALOGUE_PATH_CTRL1:
> +		return true;
> +	default:
> +		return false;
> +	}
> +}

This is getting silly now.  Can't you use ranges?

> +static const struct reg_default lochnagar2_reg_defaults[] = {
> +	{ LOCHNAGAR2_CDC_AIF1_CTRL,         0x0000 },
> +	{ LOCHNAGAR2_CDC_AIF2_CTRL,         0x0000 },
> +	{ LOCHNAGAR2_CDC_AIF3_CTRL,         0x0000 },
> +	{ LOCHNAGAR2_DSP_AIF1_CTRL,         0x0000 },
> +	{ LOCHNAGAR2_DSP_AIF2_CTRL,         0x0000 },
> +	{ LOCHNAGAR2_PSIA1_CTRL,            0x0000 },
> +	{ LOCHNAGAR2_PSIA2_CTRL,            0x0000 },
> +	{ LOCHNAGAR2_GF_AIF3_CTRL,          0x0000 },
> +	{ LOCHNAGAR2_GF_AIF4_CTRL,          0x0000 },
> +	{ LOCHNAGAR2_GF_AIF1_CTRL,          0x0000 },
> +	{ LOCHNAGAR2_GF_AIF2_CTRL,          0x0000 },
> +	{ LOCHNAGAR2_SPDIF_AIF_CTRL,        0x0000 },
> +	{ LOCHNAGAR2_USB_AIF1_CTRL,         0x0000 },
> +	{ LOCHNAGAR2_USB_AIF2_CTRL,         0x0000 },
> +	{ LOCHNAGAR2_ADAT_AIF_CTRL,         0x0000 },
> +	{ LOCHNAGAR2_CDC_MCLK1_CTRL,        0x0000 },
> +	{ LOCHNAGAR2_CDC_MCLK2_CTRL,        0x0000 },
> +	{ LOCHNAGAR2_DSP_CLKIN_CTRL,        0x0000 },
> +	{ LOCHNAGAR2_PSIA1_MCLK_CTRL,       0x0000 },
> +	{ LOCHNAGAR2_PSIA2_MCLK_CTRL,       0x0000 },
> +	{ LOCHNAGAR2_SPDIF_MCLK_CTRL,       0x0000 },
> +	{ LOCHNAGAR2_GF_CLKOUT1_CTRL,       0x0000 },
> +	{ LOCHNAGAR2_GF_CLKOUT2_CTRL,       0x0000 },
> +	{ LOCHNAGAR2_ADAT_MCLK_CTRL,        0x0000 },
> +	{ LOCHNAGAR2_SOUNDCARD_MCLK_CTRL,   0x0000 },
> +	{ LOCHNAGAR2_GPIO_FPGA_GPIO1,       0x0000 },
> +	{ LOCHNAGAR2_GPIO_FPGA_GPIO2,       0x0000 },
> +	{ LOCHNAGAR2_GPIO_FPGA_GPIO3,       0x0000 },
> +	{ LOCHNAGAR2_GPIO_FPGA_GPIO4,       0x0000 },
> +	{ LOCHNAGAR2_GPIO_FPGA_GPIO5,       0x0000 },
> +	{ LOCHNAGAR2_GPIO_FPGA_GPIO6,       0x0000 },
> +	{ LOCHNAGAR2_GPIO_CDC_GPIO1,        0x0000 },
> +	{ LOCHNAGAR2_GPIO_CDC_GPIO2,        0x0000 },
> +	{ LOCHNAGAR2_GPIO_CDC_GPIO3,        0x0000 },
> +	{ LOCHNAGAR2_GPIO_CDC_GPIO4,        0x0000 },
> +	{ LOCHNAGAR2_GPIO_CDC_GPIO5,        0x0000 },
> +	{ LOCHNAGAR2_GPIO_CDC_GPIO6,        0x0000 },
> +	{ LOCHNAGAR2_GPIO_CDC_GPIO7,        0x0000 },
> +	{ LOCHNAGAR2_GPIO_CDC_GPIO8,        0x0000 },
> +	{ LOCHNAGAR2_GPIO_DSP_GPIO1,        0x0000 },
> +	{ LOCHNAGAR2_GPIO_DSP_GPIO2,        0x0000 },
> +	{ LOCHNAGAR2_GPIO_DSP_GPIO3,        0x0000 },
> +	{ LOCHNAGAR2_GPIO_DSP_GPIO4,        0x0000 },
> +	{ LOCHNAGAR2_GPIO_DSP_GPIO5,        0x0000 },
> +	{ LOCHNAGAR2_GPIO_DSP_GPIO6,        0x0000 },
> +	{ LOCHNAGAR2_GPIO_GF_GPIO2,         0x0000 },
> +	{ LOCHNAGAR2_GPIO_GF_GPIO3,         0x0000 },
> +	{ LOCHNAGAR2_GPIO_GF_GPIO7,         0x0000 },
> +	{ LOCHNAGAR2_GPIO_CDC_AIF1_BCLK,    0x0000 },
> +	{ LOCHNAGAR2_GPIO_CDC_AIF1_RXDAT,   0x0000 },
> +	{ LOCHNAGAR2_GPIO_CDC_AIF1_LRCLK,   0x0000 },
> +	{ LOCHNAGAR2_GPIO_CDC_AIF1_TXDAT,   0x0000 },
> +	{ LOCHNAGAR2_GPIO_CDC_AIF2_BCLK,    0x0000 },
> +	{ LOCHNAGAR2_GPIO_CDC_AIF2_RXDAT,   0x0000 },
> +	{ LOCHNAGAR2_GPIO_CDC_AIF2_LRCLK,   0x0000 },
> +	{ LOCHNAGAR2_GPIO_CDC_AIF2_TXDAT,   0x0000 },
> +	{ LOCHNAGAR2_GPIO_CDC_AIF3_BCLK,    0x0000 },
> +	{ LOCHNAGAR2_GPIO_CDC_AIF3_RXDAT,   0x0000 },
> +	{ LOCHNAGAR2_GPIO_CDC_AIF3_LRCLK,   0x0000 },
> +	{ LOCHNAGAR2_GPIO_CDC_AIF3_TXDAT,   0x0000 },
> +	{ LOCHNAGAR2_GPIO_DSP_AIF1_BCLK,    0x0000 },
> +	{ LOCHNAGAR2_GPIO_DSP_AIF1_RXDAT,   0x0000 },
> +	{ LOCHNAGAR2_GPIO_DSP_AIF1_LRCLK,   0x0000 },
> +	{ LOCHNAGAR2_GPIO_DSP_AIF1_TXDAT,   0x0000 },
> +	{ LOCHNAGAR2_GPIO_DSP_AIF2_BCLK,    0x0000 },
> +	{ LOCHNAGAR2_GPIO_DSP_AIF2_RXDAT,   0x0000 },
> +	{ LOCHNAGAR2_GPIO_DSP_AIF2_LRCLK,   0x0000 },
> +	{ LOCHNAGAR2_GPIO_DSP_AIF2_TXDAT,   0x0000 },
> +	{ LOCHNAGAR2_GPIO_PSIA1_BCLK,       0x0000 },
> +	{ LOCHNAGAR2_GPIO_PSIA1_RXDAT,      0x0000 },
> +	{ LOCHNAGAR2_GPIO_PSIA1_LRCLK,      0x0000 },
> +	{ LOCHNAGAR2_GPIO_PSIA1_TXDAT,      0x0000 },
> +	{ LOCHNAGAR2_GPIO_PSIA2_BCLK,       0x0000 },
> +	{ LOCHNAGAR2_GPIO_PSIA2_RXDAT,      0x0000 },
> +	{ LOCHNAGAR2_GPIO_PSIA2_LRCLK,      0x0000 },
> +	{ LOCHNAGAR2_GPIO_PSIA2_TXDAT,      0x0000 },
> +	{ LOCHNAGAR2_GPIO_GF_AIF3_BCLK,     0x0000 },
> +	{ LOCHNAGAR2_GPIO_GF_AIF3_RXDAT,    0x0000 },
> +	{ LOCHNAGAR2_GPIO_GF_AIF3_LRCLK,    0x0000 },
> +	{ LOCHNAGAR2_GPIO_GF_AIF3_TXDAT,    0x0000 },
> +	{ LOCHNAGAR2_GPIO_GF_AIF4_BCLK,     0x0000 },
> +	{ LOCHNAGAR2_GPIO_GF_AIF4_RXDAT,    0x0000 },
> +	{ LOCHNAGAR2_GPIO_GF_AIF4_LRCLK,    0x0000 },
> +	{ LOCHNAGAR2_GPIO_GF_AIF4_TXDAT,    0x0000 },
> +	{ LOCHNAGAR2_GPIO_GF_AIF1_BCLK,     0x0000 },
> +	{ LOCHNAGAR2_GPIO_GF_AIF1_RXDAT,    0x0000 },
> +	{ LOCHNAGAR2_GPIO_GF_AIF1_LRCLK,    0x0000 },
> +	{ LOCHNAGAR2_GPIO_GF_AIF1_TXDAT,    0x0000 },
> +	{ LOCHNAGAR2_GPIO_GF_AIF2_BCLK,     0x0000 },
> +	{ LOCHNAGAR2_GPIO_GF_AIF2_RXDAT,    0x0000 },
> +	{ LOCHNAGAR2_GPIO_GF_AIF2_LRCLK,    0x0000 },
> +	{ LOCHNAGAR2_GPIO_GF_AIF2_TXDAT,    0x0000 },
> +	{ LOCHNAGAR2_GPIO_DSP_UART1_RX,     0x0000 },
> +	{ LOCHNAGAR2_GPIO_DSP_UART1_TX,     0x0000 },
> +	{ LOCHNAGAR2_GPIO_DSP_UART2_RX,     0x0000 },
> +	{ LOCHNAGAR2_GPIO_DSP_UART2_TX,     0x0000 },
> +	{ LOCHNAGAR2_GPIO_GF_UART2_RX,      0x0000 },
> +	{ LOCHNAGAR2_GPIO_GF_UART2_TX,      0x0000 },
> +	{ LOCHNAGAR2_GPIO_USB_UART_RX,      0x0000 },
> +	{ LOCHNAGAR2_GPIO_CDC_PDMCLK1,      0x0000 },
> +	{ LOCHNAGAR2_GPIO_CDC_PDMDAT1,      0x0000 },
> +	{ LOCHNAGAR2_GPIO_CDC_PDMCLK2,      0x0000 },
> +	{ LOCHNAGAR2_GPIO_CDC_PDMDAT2,      0x0000 },
> +	{ LOCHNAGAR2_GPIO_CDC_DMICCLK1,     0x0000 },
> +	{ LOCHNAGAR2_GPIO_CDC_DMICDAT1,     0x0000 },
> +	{ LOCHNAGAR2_GPIO_CDC_DMICCLK2,     0x0000 },
> +	{ LOCHNAGAR2_GPIO_CDC_DMICDAT2,     0x0000 },
> +	{ LOCHNAGAR2_GPIO_CDC_DMICCLK3,     0x0000 },
> +	{ LOCHNAGAR2_GPIO_CDC_DMICDAT3,     0x0000 },
> +	{ LOCHNAGAR2_GPIO_CDC_DMICCLK4,     0x0000 },
> +	{ LOCHNAGAR2_GPIO_CDC_DMICDAT4,     0x0000 },
> +	{ LOCHNAGAR2_GPIO_DSP_DMICCLK1,     0x0000 },
> +	{ LOCHNAGAR2_GPIO_DSP_DMICDAT1,     0x0000 },
> +	{ LOCHNAGAR2_GPIO_DSP_DMICCLK2,     0x0000 },
> +	{ LOCHNAGAR2_GPIO_DSP_DMICDAT2,     0x0000 },
> +	{ LOCHNAGAR2_GPIO_I2C2_SCL,         0x0000 },
> +	{ LOCHNAGAR2_GPIO_I2C2_SDA,         0x0000 },
> +	{ LOCHNAGAR2_GPIO_I2C3_SCL,         0x0000 },
> +	{ LOCHNAGAR2_GPIO_I2C3_SDA,         0x0000 },
> +	{ LOCHNAGAR2_GPIO_I2C4_SCL,         0x0000 },
> +	{ LOCHNAGAR2_GPIO_I2C4_SDA,         0x0000 },
> +	{ LOCHNAGAR2_GPIO_DSP_STANDBY,      0x0000 },
> +	{ LOCHNAGAR2_GPIO_CDC_MCLK1,        0x0000 },
> +	{ LOCHNAGAR2_GPIO_CDC_MCLK2,        0x0000 },
> +	{ LOCHNAGAR2_GPIO_DSP_CLKIN,        0x0000 },
> +	{ LOCHNAGAR2_GPIO_PSIA1_MCLK,       0x0000 },
> +	{ LOCHNAGAR2_GPIO_PSIA2_MCLK,       0x0000 },
> +	{ LOCHNAGAR2_GPIO_GF_GPIO1,         0x0000 },
> +	{ LOCHNAGAR2_GPIO_GF_GPIO5,         0x0000 },
> +	{ LOCHNAGAR2_GPIO_DSP_GPIO20,       0x0000 },
> +	{ LOCHNAGAR2_MINICARD_RESETS,       0x0000 },
> +	{ LOCHNAGAR2_ANALOGUE_PATH_CTRL2,   0x0000 },
> +	{ LOCHNAGAR2_COMMS_CTRL4,           0x0001 },
> +	{ LOCHNAGAR2_SPDIF_CTRL,            0x0008 },
> +	{ LOCHNAGAR2_POWER_CTRL,            0x0001 },
> +	{ LOCHNAGAR2_SOUNDCARD_AIF_CTRL,    0x0000 },
> +};

OMG!  Vile, vile vile!

> +static const struct regmap_config lochnagar2_i2c_regmap = {
> +	.reg_bits = 16,
> +	.val_bits = 16,
> +	.reg_format_endian = REGMAP_ENDIAN_BIG,
> +	.val_format_endian = REGMAP_ENDIAN_BIG,
> +
> +	.max_register = 0x1F1F,
> +	.readable_reg = lochnagar2_readable_register,
> +	.volatile_reg = lochnagar2_volatile_register,
> +
> +	.cache_type = REGCACHE_RBTREE,
> +
> +	.reg_defaults = lochnagar2_reg_defaults,
> +	.num_reg_defaults = ARRAY_SIZE(lochnagar2_reg_defaults),
> +};
> +
> +static const struct reg_sequence lochnagar2_patch[] = {
> +	{ 0x00EE, 0x0000 },
> +	{ 0x00F0, 0x0001 },
> +};
> +
> +static struct mfd_cell lochnagar2_devs[] = {
> +	{
> +		.name = "lochnagar-pinctrl"
> +	},
> +	{
> +		.name = "lochnagar-clk"
> +	},
> +	{
> +		.name = "lochnagar-regulator"
> +	},
> +	{
> +		.name = "lochnagar2-sound-card",
> +	},
> +};

All the same as above.

> +static struct lochnagar_config {
> +	int id;
> +	const char * const name;
> +	enum lochnagar_type type;
> +	const struct regmap_config *regmap;
> +	struct mfd_cell *devs;
> +	int ndevs;
> +	const struct reg_sequence *patch;
> +	int npatch;
> +} lochnagar_configs[] = {

Not a fan of this syntax.

Please separate the struct declaration and its use.

> +	{
> +		.id = 0x50,
> +		.name = "lochnagar1",
> +		.type = LOCHNAGAR1,
> +		.regmap = &lochnagar1_i2c_regmap,
> +		.devs = lochnagar1_devs,
> +		.ndevs = ARRAY_SIZE(lochnagar1_devs),
> +		.patch = lochnagar1_patch,
> +		.npatch = ARRAY_SIZE(lochnagar1_patch),
> +	},
> +	{
> +		.id = 0xCB58,
> +		.name = "lochnagar2",
> +		.type = LOCHNAGAR2,
> +		.regmap = &lochnagar2_i2c_regmap,
> +		.devs = lochnagar2_devs,
> +		.ndevs = ARRAY_SIZE(lochnagar2_devs),
> +		.patch = lochnagar2_patch,
> +		.npatch = ARRAY_SIZE(lochnagar2_patch),
> +	},
> +};
> +
> +static const struct of_device_id lochnagar_of_match[] = {
> +	{ .compatible = "cirrus,lochnagar1", .data = &lochnagar_configs[0] },
> +	{ .compatible = "cirrus,lochnagar2", .data = &lochnagar_configs[1] },
> +	{},
> +};
> +
> +static int lochnagar_wait_for_boot(struct regmap *regmap, unsigned int *id)
> +{
> +	int i, ret;
> +
> +	for (i = 0; i < LOCHNAGAR_BOOT_RETRIES; ++i) {
> +		msleep(LOCHNAGAR_BOOT_DELAY_MS);
> +
> +		ret = regmap_read(regmap, LOCHNAGAR_SOFTWARE_RESET, id);
> +		if (!ret)
> +			return ret;
> +	}
> +
> +	return -ETIMEDOUT;
> +}
> +
> +int lochnagar_update_config(struct lochnagar *lochnagar)
> +{
> +	struct regmap *regmap = lochnagar->regmap;
> +	unsigned int done = LOCHNAGAR2_ANALOGUE_PATH_UPDATE_STS_MASK;
> +	int timeout_ms = LOCHNAGAR_BOOT_DELAY_MS * LOCHNAGAR_BOOT_RETRIES;
> +	unsigned int val = 0;
> +	int ret;
> +
> +	switch (lochnagar->type) {
> +	case LOCHNAGAR1:
> +		return 0;
> +	case LOCHNAGAR2:
> +		break;
> +	default:
> +		return -EINVAL;
> +	}

This can be done with a simple if () statement:

if (lochnagar->type != LOCHNAGAR2)
   return 0;

> +	ret = regmap_write(regmap, LOCHNAGAR2_ANALOGUE_PATH_CTRL1, 0);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = regmap_write(regmap, LOCHNAGAR2_ANALOGUE_PATH_CTRL1,
> +			   LOCHNAGAR2_ANALOGUE_PATH_UPDATE_MASK);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = regmap_read_poll_timeout(regmap,
> +				       LOCHNAGAR2_ANALOGUE_PATH_CTRL1, val,
> +				       (val & done), LOCHNAGAR_CONFIG_POLL_US,
> +				       timeout_ms * 1000);
> +	if (ret < 0)
> +		return ret;

What does all this do then?  You need a comment.

> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(lochnagar_update_config);
> +
> +static int lochnagar_i2c_probe(struct i2c_client *i2c)
> +{
> +	struct device *dev = &i2c->dev;
> +	const struct lochnagar_config *config = NULL;
> +	const struct of_device_id *of_id;
> +	struct lochnagar *lochnagar;
> +	unsigned int val;
> +	struct gpio_desc *reset, *present;

Put this up higher in the list to keep the simple 'int's together.

> +	unsigned int firmwareid;
> +	int devid, rev;

Why do these need to be signed?

> +	int ret;
> +
> +	lochnagar = devm_kzalloc(dev, sizeof(*lochnagar), GFP_KERNEL);
> +	if (!lochnagar)
> +		return -ENOMEM;
> +
> +	of_id = of_match_device(lochnagar_of_match, dev);
> +	if (!of_id)
> +		return -EINVAL;
> +
> +	config = of_id->data;
> +
> +	lochnagar->dev = dev;
> +	mutex_init(&lochnagar->analogue_config_lock);
> +
> +	dev_set_drvdata(dev, lochnagar);
> +
> +	reset = devm_gpiod_get(dev, "reset", GPIOD_OUT_LOW);
> +	if (IS_ERR(reset)) {
> +		ret = PTR_ERR(reset);
> +		dev_err(dev, "Failed to get reset GPIO: %d\n", ret);
> +		return ret;
> +	}
> +
> +	present = devm_gpiod_get_optional(dev, "present", GPIOD_OUT_HIGH);
> +	if (IS_ERR(present)) {
> +		ret = PTR_ERR(present);
> +		dev_err(dev, "Failed to get present GPIO: %d\n", ret);
> +		return ret;
> +	}
> +
> +	msleep(20);

What's this for?

> +	/* Bring Lochnagar out of reset */
> +	gpiod_set_value_cansleep(reset, 1);
> +
> +	/* Identify Lochnagar */
> +	lochnagar->type = config->type;

'\n'

> +	lochnagar->regmap = devm_regmap_init_i2c(i2c, config->regmap);
> +	if (IS_ERR(lochnagar->regmap)) {
> +		ret = PTR_ERR(lochnagar->regmap);
> +		dev_err(dev, "Failed to allocate register map: %d\n", ret);
> +		return ret;
> +	}
> +
> +	/* Wait for Lochnagar to boot */
> +	ret = lochnagar_wait_for_boot(lochnagar->regmap, &val);
> +	if (ret < 0) {
> +		dev_err(dev, "Failed to read device ID: %d\n", ret);

Eh?

So you read the LOCHNAGAR_SOFTWARE_RESET register and out pops the
device/revision IDs?  That's just random!

> +		return ret;
> +	}
> +
> +	devid = val & LOCHNAGAR_DEVICE_ID_MASK;
> +	rev = val & LOCHNAGAR_REV_ID_MASK;
> +
> +	if (devid != config->id) {
> +		dev_err(dev,
> +			"ID does not match %s (expected 0x%x got 0x%x)\n",
> +			config->name, config->id, devid);
> +		return -ENODEV;
> +	}
> +
> +	/* Identify firmware */
> +	ret = regmap_read(lochnagar->regmap, LOCHNAGAR_FIRMWARE_ID1, &val);
> +	if (ret < 0) {
> +		dev_err(dev, "Failed to read firmware id 1: %d\n", ret);
> +		return ret;
> +	}
> +
> +	firmwareid = val;
> +
> +	ret = regmap_read(lochnagar->regmap, LOCHNAGAR_FIRMWARE_ID2, &val);
> +	if (ret < 0) {
> +		dev_err(dev, "Failed to read firmware id 2: %d\n", ret);
> +		return ret;
> +	}
> +
> +	firmwareid |= (val << config->regmap->val_bits);
> +
> +	dev_info(dev, "Found %s (0x%x) revision %d firmware 0x%.6x\n",
> +		 config->name, devid, rev + 1, firmwareid);
> +
> +	ret = regmap_register_patch(lochnagar->regmap, config->patch,
> +				    config->npatch);
> +	if (ret < 0) {
> +		dev_err(dev, "Failed to register patch: %d\n", ret);
> +		return ret;
> +	}
> +
> +	ret = devm_mfd_add_devices(dev, PLATFORM_DEVID_NONE, config->devs,
> +				   config->ndevs, NULL, 0, NULL);
> +	if (ret != 0) {

if (ret)

> +		dev_err(dev, "Failed to add subdevices: %d\n", ret);
> +		return ret;
> +	}
> +
> +	ret = devm_of_platform_populate(dev);
> +	if (ret < 0) {
> +		dev_err(dev, "Failed to populate child nodes: %d\n", ret);
> +		return ret;
> +	}

Please do not mix OF and MFD device registration strategies.

Pick one and register all devices through your chosen method.

> +	return ret;
> +}
> +
> +static struct i2c_driver lochnagar_i2c_driver = {
> +	.driver = {
> +		.name = "lochnagar",
> +		.of_match_table = of_match_ptr(lochnagar_of_match),
> +		.suppress_bind_attrs = true,
> +	},
> +

No need for '\n'.

> +	.probe_new = lochnagar_i2c_probe,

Hasn't this been replaced yet?

> +};
> +
> +static int __init lochnagar_i2c_init(void)
> +{
> +	int ret;
> +
> +	ret = i2c_add_driver(&lochnagar_i2c_driver);
> +	if (ret != 0)

if (ret)

> +		pr_err("Failed to register Lochnagar driver: %d\n", ret);
> +
> +	return ret;
> +}
> +subsys_initcall(lochnagar_i2c_init);
> diff --git a/include/linux/mfd/lochnagar.h b/include/linux/mfd/lochnagar.h
> new file mode 100644
> index 0000000000000..be485d6714f7c
> --- /dev/null
> +++ b/include/linux/mfd/lochnagar.h
> @@ -0,0 +1,48 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Lochnagar internals
> + *
> + * Copyright (c) 2013-2018 Cirrus Logic, Inc. and
> + *                         Cirrus Logic International Semiconductor Ltd.
> + *
> + * Author: Charles Keepax <ckeepax@opensource.cirrus.com>
> + */
> +
> +#ifndef CIRRUS_LOCHNAGAR_H
> +#define CIRRUS_LOCHNAGAR_H
> +
> +#include "lochnagar1_regs.h"
> +#include "lochnagar2_regs.h"

Why are you including these here?

> +struct device;
> +struct regmap;
> +struct mutex;

Just add the include files.

> +enum lochnagar_type {
> +	LOCHNAGAR1,
> +	LOCHNAGAR2,
> +};
> +
> +struct lochnagar {
> +	enum lochnagar_type type;
> +	struct device *dev;
> +	struct regmap *regmap;
> +
> +	/* Lock to protect updates to the analogue configuration */
> +	struct mutex analogue_config_lock;
> +};

Kerneldoc header please.

> +/* Register Addresses */
> +#define LOCHNAGAR_SOFTWARE_RESET                             0x00
> +#define LOCHNAGAR_FIRMWARE_ID1                               0x01
> +#define LOCHNAGAR_FIRMWARE_ID2                               0x02
> +
> +/* (0x0000)  Software Reset */
> +#define LOCHNAGAR_DEVICE_ID_MASK                           0xFFFC
> +#define LOCHNAGAR_DEVICE_ID_SHIFT                               2
> +#define LOCHNAGAR_REV_ID_MASK                              0x0003
> +#define LOCHNAGAR_REV_ID_SHIFT                                  0
> +
> +int lochnagar_update_config(struct lochnagar *lochnagar);
> +
> +#endif

> diff --git a/include/linux/mfd/lochnagar1_regs.h b/include/linux/mfd/lochnagar1_regs.h
> diff --git a/include/linux/mfd/lochnagar2_regs.h b/include/linux/mfd/lochnagar2_regs.h

So Lochnagar 1 and 2 are completely different devices?

What do they do?
Charles Keepax Oct. 25, 2018, 8:26 a.m. UTC | #2
On Thu, Oct 25, 2018 at 08:44:59AM +0100, Lee Jones wrote:
> On Mon, 08 Oct 2018, Charles Keepax wrote:
> > From: Charles Keepax <ckeepax@opensource.wolfsonmicro.com>
> > +static const struct reg_default lochnagar1_reg_defaults[] = {
> > +	{ LOCHNAGAR1_CDC_AIF1_SEL,    0x00 },
> > +	{ LOCHNAGAR1_CDC_AIF2_SEL,    0x00 },
...
> > +	{ LOCHNAGAR1_LED1,            0x00 },
> > +	{ LOCHNAGAR1_LED2,            0x00 },
> > +	{ LOCHNAGAR1_I2C_CTRL,        0x01 },
> > +};
> 
> Why do you need to specify each register value?
> 

The way regmap operates it needs to know the starting value of
each register. It will use this to initialise the cache and to
determine if writes need to actually update the hardware on
cache_syncs after devices have been powered back up.

> > +static const struct reg_sequence lochnagar1_patch[] = {
> > +	{ 0x40, 0x0083 },
> > +	{ 0x46, 0x0001 },
> > +	{ 0x47, 0x0018 },
> > +	{ 0x50, 0x0000 },
> > +};
> 
> I'm really not a fan of these so call 'patches'.
> 
> Can't you set the registers up proper way?
> 

I will see if we could move any out of here or define any of the
registers but as we have discussed before it is not always possible.

> > +static bool lochnagar2_readable_register(struct device *dev, unsigned int reg)
> > +{
> > +	switch (reg) {
> > +	case LOCHNAGAR_SOFTWARE_RESET:
> > +	case LOCHNAGAR_FIRMWARE_ID1:
> > +	case LOCHNAGAR_FIRMWARE_ID2:
...
> > +	case LOCHNAGAR2_MICVDD_CTRL2:
> > +	case LOCHNAGAR2_VDDCORE_CDC_CTRL1:
> > +	case LOCHNAGAR2_VDDCORE_CDC_CTRL2:
> > +	case LOCHNAGAR2_SOUNDCARD_AIF_CTRL:
> > +		return true;
> > +	default:
> > +		return false;
> > +	}
> > +}
> > +
> > +static bool lochnagar2_volatile_register(struct device *dev, unsigned int reg)
> > +{
> > +	switch (reg) {
> > +	case LOCHNAGAR2_GPIO_CHANNEL1:
> > +	case LOCHNAGAR2_GPIO_CHANNEL2:
> > +	case LOCHNAGAR2_GPIO_CHANNEL3:
...
> > +	case LOCHNAGAR2_GPIO_CHANNEL13:
> > +	case LOCHNAGAR2_GPIO_CHANNEL14:
> > +	case LOCHNAGAR2_GPIO_CHANNEL15:
> > +	case LOCHNAGAR2_GPIO_CHANNEL16:
> > +	case LOCHNAGAR2_ANALOGUE_PATH_CTRL1:
> > +		return true;
> > +	default:
> > +		return false;
> > +	}
> > +}
> 
> This is getting silly now.  Can't you use ranges?
> 

I can if you feel strongly about it? But it does make the drivers
much more error prone and significantly more annoying to work
with. I find it is really common to be checking that a register
is handled correctly through the regmap callbacks and it is nice
to just be able to grep for that. Obviously this won't work for
all devices/regmaps as well since many will not have consecutive
addresses on registers, for example having multi-byte registers
that are byte addressed.

How far would you like me to take this as well? Is it just the
numeric registers you want ranges for ie.

LOCHNAGAR2_GPIO_CHANNEL1...LOCHNAGAR_GPIO_CHANNEL16

Or is it all consecutive registers even if they are unrelated
(exmaple is probably not accurate as I haven't checked the
addresses):

LOCHNAGAR2_GPIO_CHANNEL1...LOCHNAGAR2_ANALOGURE_PATH_CTRL1

I don't mind the first at all but the second is getting really
horrible in my opinion.

> > +static const struct reg_default lochnagar2_reg_defaults[] = {
> > +	{ LOCHNAGAR2_CDC_AIF1_CTRL,         0x0000 },
> > +	{ LOCHNAGAR2_CDC_AIF2_CTRL,         0x0000 },
> > +	{ LOCHNAGAR2_CDC_AIF3_CTRL,         0x0000 },
> > +	{ LOCHNAGAR2_DSP_AIF1_CTRL,         0x0000 },
...
> > +	{ LOCHNAGAR2_MINICARD_RESETS,       0x0000 },
> > +	{ LOCHNAGAR2_ANALOGUE_PATH_CTRL2,   0x0000 },
> > +	{ LOCHNAGAR2_COMMS_CTRL4,           0x0001 },
> > +	{ LOCHNAGAR2_SPDIF_CTRL,            0x0008 },
> > +	{ LOCHNAGAR2_POWER_CTRL,            0x0001 },
> > +	{ LOCHNAGAR2_SOUNDCARD_AIF_CTRL,    0x0000 },
> > +};
> 
> OMG!  Vile, vile vile!
> 

I really feel this isn't the driver you are objecting to as such
but the way regmap operates and also we seem to always have the same
discussions around regmap every time we push a driver. Is there
any way me, you and Mark could hash this out and find out a way to
handle regmaps that is acceptable to you? I don't suppose you are
in Edinburgh at the moment for ELCE?

> > +	/* Wait for Lochnagar to boot */
> > +	ret = lochnagar_wait_for_boot(lochnagar->regmap, &val);
> > +	if (ret < 0) {
> > +		dev_err(dev, "Failed to read device ID: %d\n", ret);
> 
> Eh?
> 
> So you read the LOCHNAGAR_SOFTWARE_RESET register and out pops the
> device/revision IDs?  That's just random!

I shall let the hardware guys know you don't approve of their
life choices :-) and add some comments to the code.

> > +	ret = devm_of_platform_populate(dev);
> > +	if (ret < 0) {
> > +		dev_err(dev, "Failed to populate child nodes: %d\n", ret);
> > +		return ret;
> > +	}
> 
> Please do not mix OF and MFD device registration strategies.
> 
> Pick one and register all devices through your chosen method.
> 

Hmmm we use this to do things like register some fixed regulators
and clocks that don't need any control but do need to be associated
with the device. I could do that through the MFD although it is in
direct conflict with the feedback on the clock patches I received
to move the fixed clocks into devicetree rather than registering
them manually (see v2 of the patch chain).

I will have a look see if I can find any ideas that will make
everyone happy but we might need to discuss with Mark and the
clock guys.

> > +	.probe_new = lochnagar_i2c_probe,
> 
> Hasn't this been replaced yet?
> 

I will check, the patchset has been around internally for a while
so it is possible this is no longer needed.

> > +#ifndef CIRRUS_LOCHNAGAR_H
> > +#define CIRRUS_LOCHNAGAR_H
> > +
> > +#include "lochnagar1_regs.h"
> > +#include "lochnagar2_regs.h"
> 
> Why are you including these here?
> 

It is just a convenience so the drivers only need to include
lochnagar.h rather than including all three headers manually.

> > diff --git a/include/linux/mfd/lochnagar1_regs.h b/include/linux/mfd/lochnagar1_regs.h
> > diff --git a/include/linux/mfd/lochnagar2_regs.h b/include/linux/mfd/lochnagar2_regs.h
> 
> So Lochnagar 1 and 2 are completely different devices?
> 
> What do they do?
> 

Completely different devices is a bit strong, they are different
versions of the same system. They have quite different register
maps but provide very similar functionality.

All the other comments I will get fixed up for the next spin of
the patches.

Thanks,
Charles
Richard Fitzgerald Oct. 25, 2018, 9:28 a.m. UTC | #3
On 25/10/18 09:26, Charles Keepax wrote:
> On Thu, Oct 25, 2018 at 08:44:59AM +0100, Lee Jones wrote:
>> On Mon, 08 Oct 2018, Charles Keepax wrote:
>>> From: Charles Keepax <ckeepax@opensource.wolfsonmicro.com>
>>> +static const struct reg_default lochnagar1_reg_defaults[] = {
>>> +	{ LOCHNAGAR1_CDC_AIF1_SEL,    0x00 },
>>> +	{ LOCHNAGAR1_CDC_AIF2_SEL,    0x00 },
> ...
>>> +	{ LOCHNAGAR1_LED1,            0x00 },
>>> +	{ LOCHNAGAR1_LED2,            0x00 },
>>> +	{ LOCHNAGAR1_I2C_CTRL,        0x01 },
>>> +};
>>
>> Why do you need to specify each register value?
>>
> 
> The way regmap operates it needs to know the starting value of
> each register. It will use this to initialise the cache and to
> determine if writes need to actually update the hardware on
> cache_syncs after devices have been powered back up.
> 
>>> +static const struct reg_sequence lochnagar1_patch[] = {
>>> +	{ 0x40, 0x0083 },
>>> +	{ 0x46, 0x0001 },
>>> +	{ 0x47, 0x0018 },
>>> +	{ 0x50, 0x0000 },
>>> +};
>>
>> I'm really not a fan of these so call 'patches'.
>>
>> Can't you set the registers up proper way?
>>
> 
> I will see if we could move any out of here or define any of the
> registers but as we have discussed before it is not always possible.
> 

Also patches generally come out of hardware tuning/qualification/tools
as this list of address,value. So it's easy for people to dump an update
into the driver as a trivial copy-paste but more work if they have to
reverse-engineer the patch list from hardware/datasheet into what each
line "means" and then find the relevant lines of code to change. It's also
much easier to answer the question "Have these hardware patches been
applied to the driver?" if we have them in the original documented format.
It just makes people's lives more difficult if they have to search around
the code to try to find something that looks like the originally specified
patch list. We don't use them just as a lazy way to setup some registers.

>>> +static bool lochnagar2_readable_register(struct device *dev, unsigned int reg)
>>> +{
>>> +	switch (reg) {
>>> +	case LOCHNAGAR_SOFTWARE_RESET:
>>> +	case LOCHNAGAR_FIRMWARE_ID1:
>>> +	case LOCHNAGAR_FIRMWARE_ID2:
> ...
>>> +	case LOCHNAGAR2_MICVDD_CTRL2:
>>> +	case LOCHNAGAR2_VDDCORE_CDC_CTRL1:
>>> +	case LOCHNAGAR2_VDDCORE_CDC_CTRL2:
>>> +	case LOCHNAGAR2_SOUNDCARD_AIF_CTRL:
>>> +		return true;
>>> +	default:
>>> +		return false;
>>> +	}
>>> +}
>>> +
>>> +static bool lochnagar2_volatile_register(struct device *dev, unsigned int reg)
>>> +{
>>> +	switch (reg) {
>>> +	case LOCHNAGAR2_GPIO_CHANNEL1:
>>> +	case LOCHNAGAR2_GPIO_CHANNEL2:
>>> +	case LOCHNAGAR2_GPIO_CHANNEL3:
> ...
>>> +	case LOCHNAGAR2_GPIO_CHANNEL13:
>>> +	case LOCHNAGAR2_GPIO_CHANNEL14:
>>> +	case LOCHNAGAR2_GPIO_CHANNEL15:
>>> +	case LOCHNAGAR2_GPIO_CHANNEL16:
>>> +	case LOCHNAGAR2_ANALOGUE_PATH_CTRL1:
>>> +		return true;
>>> +	default:
>>> +		return false;
>>> +	}
>>> +}
>>
>> This is getting silly now.  Can't you use ranges?
>>
> 
> I can if you feel strongly about it? But it does make the drivers
> much more error prone and significantly more annoying to work
> with. I find it is really common to be checking that a register
> is handled correctly through the regmap callbacks and it is nice
> to just be able to grep for that. Obviously this won't work for
> all devices/regmaps as well since many will not have consecutive
> addresses on registers, for example having multi-byte registers
> that are byte addressed.
> 
> How far would you like me to take this as well? Is it just the
> numeric registers you want ranges for ie.
> 
> LOCHNAGAR2_GPIO_CHANNEL1...LOCHNAGAR_GPIO_CHANNEL16
> 
> Or is it all consecutive registers even if they are unrelated
> (exmaple is probably not accurate as I haven't checked the
> addresses):
> 
> LOCHNAGAR2_GPIO_CHANNEL1...LOCHNAGAR2_ANALOGURE_PATH_CTRL1
> 
> I don't mind the first at all but the second is getting really
> horrible in my opinion.
> 
>>> +static const struct reg_default lochnagar2_reg_defaults[] = {
>>> +	{ LOCHNAGAR2_CDC_AIF1_CTRL,         0x0000 },
>>> +	{ LOCHNAGAR2_CDC_AIF2_CTRL,         0x0000 },
>>> +	{ LOCHNAGAR2_CDC_AIF3_CTRL,         0x0000 },
>>> +	{ LOCHNAGAR2_DSP_AIF1_CTRL,         0x0000 },
> ...
>>> +	{ LOCHNAGAR2_MINICARD_RESETS,       0x0000 },
>>> +	{ LOCHNAGAR2_ANALOGUE_PATH_CTRL2,   0x0000 },
>>> +	{ LOCHNAGAR2_COMMS_CTRL4,           0x0001 },
>>> +	{ LOCHNAGAR2_SPDIF_CTRL,            0x0008 },
>>> +	{ LOCHNAGAR2_POWER_CTRL,            0x0001 },
>>> +	{ LOCHNAGAR2_SOUNDCARD_AIF_CTRL,    0x0000 },
>>> +};
>>
>> OMG!  Vile, vile vile!
>>
> 
> I really feel this isn't the driver you are objecting to as such
> but the way regmap operates and also we seem to always have the same
> discussions around regmap every time we push a driver. Is there
> any way me, you and Mark could hash this out and find out a way to
> handle regmaps that is acceptable to you? I don't suppose you are
> in Edinburgh at the moment for ELCE?
> 

I suppose if Mark was willing to promote the regmap drivers to be a
top-level subsystem that could contain the regmap definitions of devices
then we could dump our regmap definitions in there, where Mark can review
it as he's familiar with regmap and the chips and the reasons why things
are done the way they are, rather than Lee having to stress about it every
time we need to create an MFD device that uses regmap. Though that would
make the initialization of an MFD rather awkward with the code required
to init the MFD it not actually being in the MFD tree.

>>> +	/* Wait for Lochnagar to boot */
>>> +	ret = lochnagar_wait_for_boot(lochnagar->regmap, &val);
>>> +	if (ret < 0) {
>>> +		dev_err(dev, "Failed to read device ID: %d\n", ret);
>>
>> Eh?
>>
>> So you read the LOCHNAGAR_SOFTWARE_RESET register and out pops the
>> device/revision IDs?  That's just random!
>
> I shall let the hardware guys know you don't approve of their
> life choices :-) and add some comments to the code.
> 
>>> +	ret = devm_of_platform_populate(dev);
>>> +	if (ret < 0) {
>>> +		dev_err(dev, "Failed to populate child nodes: %d\n", ret);
>>> +		return ret;
>>> +	}
>>
>> Please do not mix OF and MFD device registration strategies.
>>
>> Pick one and register all devices through your chosen method.
>>
> 
> Hmmm we use this to do things like register some fixed regulators
> and clocks that don't need any control but do need to be associated
> with the device. I could do that through the MFD although it is in
> direct conflict with the feedback on the clock patches I received
> to move the fixed clocks into devicetree rather than registering
> them manually (see v2 of the patch chain).
> 
> I will have a look see if I can find any ideas that will make
> everyone happy but we might need to discuss with Mark and the
> clock guys.
> 
>>> +	.probe_new = lochnagar_i2c_probe,
>>
>> Hasn't this been replaced yet?
>>
> 
> I will check, the patchset has been around internally for a while
> so it is possible this is no longer needed.
> 
>>> +#ifndef CIRRUS_LOCHNAGAR_H
>>> +#define CIRRUS_LOCHNAGAR_H
>>> +
>>> +#include "lochnagar1_regs.h"
>>> +#include "lochnagar2_regs.h"
>>
>> Why are you including these here?
>>
> 
> It is just a convenience so the drivers only need to include
> lochnagar.h rather than including all three headers manually.
> 
>>> diff --git a/include/linux/mfd/lochnagar1_regs.h b/include/linux/mfd/lochnagar1_regs.h
>>> diff --git a/include/linux/mfd/lochnagar2_regs.h b/include/linux/mfd/lochnagar2_regs.h
>>
>> So Lochnagar 1 and 2 are completely different devices?
>>
>> What do they do?
>>
> 
> Completely different devices is a bit strong, they are different
> versions of the same system. They have quite different register
> maps but provide very similar functionality.
> 

The register maps are different partly because some silicon
used on the V1 is no longer manufactured and partly because some
silicon added in V2 didn't fit into the older register map.

> All the other comments I will get fixed up for the next spin of
> the patches.
> 
> Thanks,
> Charles
>
Mark Brown Oct. 25, 2018, 10:12 a.m. UTC | #4
On Thu, Oct 25, 2018 at 10:28:16AM +0100, Richard Fitzgerald wrote:
> On 25/10/18 09:26, Charles Keepax wrote:
> > On Thu, Oct 25, 2018 at 08:44:59AM +0100, Lee Jones wrote:

> > > I'm really not a fan of these so call 'patches'.

> > > Can't you set the registers up proper way?

> > I will see if we could move any out of here or define any of the
> > registers but as we have discussed before it is not always possible.

> Also patches generally come out of hardware tuning/qualification/tools
> as this list of address,value. So it's easy for people to dump an update
> into the driver as a trivial copy-paste but more work if they have to
> reverse-engineer the patch list from hardware/datasheet into what each
> line "means" and then find the relevant lines of code to change. It's also
> much easier to answer the question "Have these hardware patches been
> applied to the driver?" if we have them in the original documented format.
> It just makes people's lives more difficult if they have to search around
> the code to try to find something that looks like the originally specified
> patch list. We don't use them just as a lazy way to setup some registers.

Further, the main intended use for register patches is for registers
that the device manufacturer has no intention of documenting beyond
providing a list of register writes to be done on device startup.  The
common example is test registers with chicken bits that turn out to have
been needed, it's not realistic to expect that vendors are going to
start documenting their test registers.  It's normal to see these
applied only on early revisions of parts (eg, rev A needs a patch to
adjust things to match rev B which is the mass production version).

> > I really feel this isn't the driver you are objecting to as such
> > but the way regmap operates and also we seem to always have the same
> > discussions around regmap every time we push a driver. Is there
> > any way me, you and Mark could hash this out and find out a way to
> > handle regmaps that is acceptable to you? I don't suppose you are
> > in Edinburgh at the moment for ELCE?

> I suppose if Mark was willing to promote the regmap drivers to be a
> top-level subsystem that could contain the regmap definitions of devices
> then we could dump our regmap definitions in there, where Mark can review
> it as he's familiar with regmap and the chips and the reasons why things
> are done the way they are, rather than Lee having to stress about it every
> time we need to create an MFD device that uses regmap. Though that would
> make the initialization of an MFD rather awkward with the code required
> to init the MFD it not actually being in the MFD tree.

I'm not totally against dumping the data tables in some other directory
(we could do ../regmap/tables or whatever) but I fear it's going to
cause otherwise needless cross tree issues.
Charles Keepax Oct. 25, 2018, 10:56 a.m. UTC | #5
On Thu, Oct 25, 2018 at 11:12:48AM +0100, Mark Brown wrote:
> On Thu, Oct 25, 2018 at 10:28:16AM +0100, Richard Fitzgerald wrote:
> > On 25/10/18 09:26, Charles Keepax wrote:
> > > On Thu, Oct 25, 2018 at 08:44:59AM +0100, Lee Jones wrote:
> > > I really feel this isn't the driver you are objecting to as such
> > > but the way regmap operates and also we seem to always have the same
> > > discussions around regmap every time we push a driver. Is there
> > > any way me, you and Mark could hash this out and find out a way to
> > > handle regmaps that is acceptable to you? I don't suppose you are
> > > in Edinburgh at the moment for ELCE?
> 
> > I suppose if Mark was willing to promote the regmap drivers to be a
> > top-level subsystem that could contain the regmap definitions of devices
> > then we could dump our regmap definitions in there, where Mark can review
> > it as he's familiar with regmap and the chips and the reasons why things
> > are done the way they are, rather than Lee having to stress about it every
> > time we need to create an MFD device that uses regmap. Though that would
> > make the initialization of an MFD rather awkward with the code required
> > to init the MFD it not actually being in the MFD tree.
> 
> I'm not totally against dumping the data tables in some other directory
> (we could do ../regmap/tables or whatever) but I fear it's going to
> cause otherwise needless cross tree issues.

I guess there is a question here, if i split the regmap stuff
into a separate file within mfd would that be enough? Is it just
the mixing of these tables and the code you object to Lee or is
it the fact the tables exist at all?

Thanks,
Charles
Lee Jones Oct. 25, 2018, 11:42 a.m. UTC | #6
On Thu, 25 Oct 2018, Richard Fitzgerald wrote:
> On 25/10/18 09:26, Charles Keepax wrote:
> > On Thu, Oct 25, 2018 at 08:44:59AM +0100, Lee Jones wrote:
> > > On Mon, 08 Oct 2018, Charles Keepax wrote:
> > > > From: Charles Keepax <ckeepax@opensource.wolfsonmicro.com>
> > > > +static const struct reg_default lochnagar1_reg_defaults[] = {
> > > > +	{ LOCHNAGAR1_CDC_AIF1_SEL,    0x00 },
> > > > +	{ LOCHNAGAR1_CDC_AIF2_SEL,    0x00 },
> > ...
> > > > +	{ LOCHNAGAR1_LED1,            0x00 },
> > > > +	{ LOCHNAGAR1_LED2,            0x00 },
> > > > +	{ LOCHNAGAR1_I2C_CTRL,        0x01 },
> > > > +};
> > > 
> > > Why do you need to specify each register value?
> > 
> > The way regmap operates it needs to know the starting value of
> > each register. It will use this to initialise the cache and to
> > determine if writes need to actually update the hardware on
> > cache_syncs after devices have been powered back up.

That sounds crazy to me.  Some devices have thousands of registers.
At a line per register, that's thousands of lines of code/cruft.
Especially seeing as most (sane?) register layouts I've seen default
to zero.  Then default values can be changed at the leisure of the
s/w.

Even if it is absolutely critical that you have to supply these to
Regmap up-front, instead of on first use/read, why can't you just
supply the oddball non-zero ones?

> > > > +static const struct reg_sequence lochnagar1_patch[] = {
> > > > +	{ 0x40, 0x0083 },
> > > > +	{ 0x46, 0x0001 },
> > > > +	{ 0x47, 0x0018 },
> > > > +	{ 0x50, 0x0000 },
> > > > +};
> > > 
> > > I'm really not a fan of these so call 'patches'.
> > > 
> > > Can't you set the registers up proper way?
> > > 
> > 
> > I will see if we could move any out of here or define any of the
> > registers but as we have discussed before it is not always possible.
> > 
> 
> Also patches generally come out of hardware tuning/qualification/tools
> as this list of address,value. So it's easy for people to dump an update
> into the driver as a trivial copy-paste but more work if they have to
> reverse-engineer the patch list from hardware/datasheet into what each
> line "means" and then find the relevant lines of code to change. It's also
> much easier to answer the question "Have these hardware patches been
> applied to the driver?" if we have them in the original documented format.
> It just makes people's lives more difficult if they have to search around
> the code to try to find something that looks like the originally specified
> patch list. We don't use them just as a lazy way to setup some registers.

I understand why they normally exist (sometimes people are just lazy
too) (Mark: BTW chicken-bits sound delicious!).  They're just ugly
from an Open Source PoV.

> > > > +static bool lochnagar2_readable_register(struct device *dev, unsigned int reg)
> > > > +{
> > > > +	switch (reg) {
> > > > +	case LOCHNAGAR_SOFTWARE_RESET:
> > > > +	case LOCHNAGAR_FIRMWARE_ID1:
> > > > +	case LOCHNAGAR_FIRMWARE_ID2:
> > ...
> > > > +	case LOCHNAGAR2_MICVDD_CTRL2:
> > > > +	case LOCHNAGAR2_VDDCORE_CDC_CTRL1:
> > > > +	case LOCHNAGAR2_VDDCORE_CDC_CTRL2:
> > > > +	case LOCHNAGAR2_SOUNDCARD_AIF_CTRL:
> > > > +		return true;
> > > > +	default:
> > > > +		return false;
> > > > +	}
> > > > +}
> > > > +
> > > > +static bool lochnagar2_volatile_register(struct device *dev, unsigned int reg)
> > > > +{
> > > > +	switch (reg) {
> > > > +	case LOCHNAGAR2_GPIO_CHANNEL1:
> > > > +	case LOCHNAGAR2_GPIO_CHANNEL2:
> > > > +	case LOCHNAGAR2_GPIO_CHANNEL3:
> > ...
> > > > +	case LOCHNAGAR2_GPIO_CHANNEL13:
> > > > +	case LOCHNAGAR2_GPIO_CHANNEL14:
> > > > +	case LOCHNAGAR2_GPIO_CHANNEL15:
> > > > +	case LOCHNAGAR2_GPIO_CHANNEL16:
> > > > +	case LOCHNAGAR2_ANALOGUE_PATH_CTRL1:
> > > > +		return true;
> > > > +	default:
> > > > +		return false;
> > > > +	}
> > > > +}
> > > 
> > > This is getting silly now.  Can't you use ranges?
> > 
> > I can if you feel strongly about it? But it does make the drivers
> > much more error prone and significantly more annoying to work
> > with. I find it is really common to be checking that a register
> > is handled correctly through the regmap callbacks and it is nice
> > to just be able to grep for that. Obviously this won't work for
> > all devices/regmaps as well since many will not have consecutive
> > addresses on registers, for example having multi-byte registers
> > that are byte addressed.
> > 
> > How far would you like me to take this as well? Is it just the
> > numeric registers you want ranges for ie.
> > 
> > LOCHNAGAR2_GPIO_CHANNEL1...LOCHNAGAR_GPIO_CHANNEL16
> > 
> > Or is it all consecutive registers even if they are unrelated
> > (exmaple is probably not accurate as I haven't checked the
> > addresses):
> > 
> > LOCHNAGAR2_GPIO_CHANNEL1...LOCHNAGAR2_ANALOGURE_PATH_CTRL1
> > 
> > I don't mind the first at all but the second is getting really
> > horrible in my opinion.

My issue is that we have one end of the scale, where contributors are
submitting patches, trying to remove a line of code here, a line of
code there, then there are patches like this one which describe the
initial value, readable status, writable status and volatile status of
each and every register.

The API is obscene and needs a re-work IMHO.

I really hope we do not really have to list every register, but *if we
absolutely must*, let's do it once:

  REG_ADDRESS, WRV, INITIAL_VALUE

> > > > +static const struct reg_default lochnagar2_reg_defaults[] = {
> > > > +	{ LOCHNAGAR2_CDC_AIF1_CTRL,         0x0000 },
> > > > +	{ LOCHNAGAR2_CDC_AIF2_CTRL,         0x0000 },
> > > > +	{ LOCHNAGAR2_CDC_AIF3_CTRL,         0x0000 },
> > > > +	{ LOCHNAGAR2_DSP_AIF1_CTRL,         0x0000 },
> > ...
> > > > +	{ LOCHNAGAR2_MINICARD_RESETS,       0x0000 },
> > > > +	{ LOCHNAGAR2_ANALOGUE_PATH_CTRL2,   0x0000 },
> > > > +	{ LOCHNAGAR2_COMMS_CTRL4,           0x0001 },
> > > > +	{ LOCHNAGAR2_SPDIF_CTRL,            0x0008 },
> > > > +	{ LOCHNAGAR2_POWER_CTRL,            0x0001 },
> > > > +	{ LOCHNAGAR2_SOUNDCARD_AIF_CTRL,    0x0000 },
> > > > +};
> > > 
> > > OMG!  Vile, vile vile!
> > 
> > I really feel this isn't the driver you are objecting to as such
> > but the way regmap operates and also we seem to always have the same
> > discussions around regmap every time we push a driver.

Absolutely.  I didn't like it before.  I like it even less now.

> > Is there
> > any way me, you and Mark could hash this out and find out a way to
> > handle regmaps that is acceptable to you? I don't suppose you are
> > in Edinburgh at the moment for ELCE?

I'm not at ELCE I'm afraid.

> I suppose if Mark was willing to promote the regmap drivers to be a
> top-level subsystem that could contain the regmap definitions of devices
> then we could dump our regmap definitions in there, where Mark can review
> it as he's familiar with regmap and the chips and the reasons why things
> are done the way they are, rather than Lee having to stress about it every
> time we need to create an MFD device that uses regmap. Though that would
> make the initialization of an MFD rather awkward with the code required
> to init the MFD it not actually being in the MFD tree.

My issue isn't where all this bumph lives.

It's the fact that it's required (at least at this level) at all.

> > > > +	/* Wait for Lochnagar to boot */
> > > > +	ret = lochnagar_wait_for_boot(lochnagar->regmap, &val);
> > > > +	if (ret < 0) {
> > > > +		dev_err(dev, "Failed to read device ID: %d\n", ret);
> > > 
> > > Eh?
> > > 
> > > So you read the LOCHNAGAR_SOFTWARE_RESET register and out pops the
> > > device/revision IDs?  That's just random!
> > 
> > I shall let the hardware guys know you don't approve of their
> > life choices :-) and add some comments to the code.

Please do.  And tell them to stop drinking at lunch time. ;)

> > > > +	ret = devm_of_platform_populate(dev);
> > > > +	if (ret < 0) {
> > > > +		dev_err(dev, "Failed to populate child nodes: %d\n", ret);
> > > > +		return ret;
> > > > +	}
> > > 
> > > Please do not mix OF and MFD device registration strategies.
> > > 
> > > Pick one and register all devices through your chosen method.
> > 
> > Hmmm we use this to do things like register some fixed regulators
> > and clocks that don't need any control but do need to be associated
> > with the device. I could do that through the MFD although it is in
> > direct conflict with the feedback on the clock patches I received
> > to move the fixed clocks into devicetree rather than registering
> > them manually (see v2 of the patch chain).

The I suggest moving everything to DT.

> > I will have a look see if I can find any ideas that will make
> > everyone happy but we might need to discuss with Mark and the
> > clock guys.
> > 
> > > > +	.probe_new = lochnagar_i2c_probe,
> > > 
> > > Hasn't this been replaced yet?
> > 
> > I will check, the patchset has been around internally for a while
> > so it is possible this is no longer needed.
> > 
> > > > +#ifndef CIRRUS_LOCHNAGAR_H
> > > > +#define CIRRUS_LOCHNAGAR_H
> > > > +
> > > > +#include "lochnagar1_regs.h"
> > > > +#include "lochnagar2_regs.h"
> > > 
> > > Why are you including these here?
> > > 
> > 
> > It is just a convenience so the drivers only need to include
> > lochnagar.h rather than including all three headers manually.

That's against convention.

If a source file needs a head, it should include it explicitly.

> > > > diff --git a/include/linux/mfd/lochnagar1_regs.h b/include/linux/mfd/lochnagar1_regs.h
> > > > diff --git a/include/linux/mfd/lochnagar2_regs.h b/include/linux/mfd/lochnagar2_regs.h
> > > 
> > > So Lochnagar 1 and 2 are completely different devices?
> > > 
> > > What do they do?
> > 
> > Completely different devices is a bit strong, they are different
> > versions of the same system. They have quite different register
> > maps but provide very similar functionality.
> 
> The register maps are different partly because some silicon
> used on the V1 is no longer manufactured and partly because some
> silicon added in V2 didn't fit into the older register map.

I just looked at the maps, which appeared to be vastly different.

> > All the other comments I will get fixed up for the next spin of
> > the patches.
Charles Keepax Oct. 25, 2018, 12:49 p.m. UTC | #7
On Thu, Oct 25, 2018 at 12:42:05PM +0100, Lee Jones wrote:
> On Thu, 25 Oct 2018, Richard Fitzgerald wrote:
> > On 25/10/18 09:26, Charles Keepax wrote:
> > > On Thu, Oct 25, 2018 at 08:44:59AM +0100, Lee Jones wrote:
> > > > On Mon, 08 Oct 2018, Charles Keepax wrote:
> > > > > From: Charles Keepax <ckeepax@opensource.wolfsonmicro.com>
> > > > > +static const struct reg_default lochnagar1_reg_defaults[] = {
> > > > > +	{ LOCHNAGAR1_CDC_AIF1_SEL,    0x00 },
> > > > > +	{ LOCHNAGAR1_CDC_AIF2_SEL,    0x00 },
> > > ...
> > > > > +	{ LOCHNAGAR1_LED1,            0x00 },
> > > > > +	{ LOCHNAGAR1_LED2,            0x00 },
> > > > > +	{ LOCHNAGAR1_I2C_CTRL,        0x01 },
> > > > > +};
> > > > 
> > > > Why do you need to specify each register value?
> > > 
> > > The way regmap operates it needs to know the starting value of
> > > each register. It will use this to initialise the cache and to
> > > determine if writes need to actually update the hardware on
> > > cache_syncs after devices have been powered back up.
> 
> That sounds crazy to me.  Some devices have thousands of registers.
> At a line per register, that's thousands of lines of code/cruft.
> Especially seeing as most (sane?) register layouts I've seen default
> to zero.  Then default values can be changed at the leisure of the
> s/w.
> 
> Even if it is absolutely critical that you have to supply these to
> Regmap up-front, instead of on first use/read, why can't you just
> supply the oddball non-zero ones?
> 

I don't think you can sensibly get away with not supplying
default values. You say most sane register layouts have zero
values, alas again you may not be the biggest fan of our hardware
guys. The Lochnagar actually does have mostly zero defaults but
that is very much not generally the case with our hardware.

One of the main aims of regmap is to avoid bus accesses when
possible but I guess on the first write/read to any register you
could insert a read to pull the default, although I do worry
there is some corner case/flexibility that is going to cause
headaches later.

I am not sure that dynamically constructing the defaults would be
possible from a table of non-zero ones, or at least doing so would
probably require a fairly major change to the way registers are
specified since with the current callback based approach and a
sparse regmap you could need to iterate over billions of
potential registers to build the table.

> > > > > +static bool lochnagar2_readable_register(struct device *dev, unsigned int reg)
> > > > > +{
> > > > > +	switch (reg) {
> > > > > +	case LOCHNAGAR_SOFTWARE_RESET:
> > > > > +	case LOCHNAGAR_FIRMWARE_ID1:
> > > > > +	case LOCHNAGAR_FIRMWARE_ID2:
> > > ...
> > > > > +	case LOCHNAGAR2_MICVDD_CTRL2:
> > > > > +	case LOCHNAGAR2_VDDCORE_CDC_CTRL1:
> > > > > +	case LOCHNAGAR2_VDDCORE_CDC_CTRL2:
> > > > > +	case LOCHNAGAR2_SOUNDCARD_AIF_CTRL:
> > > > > +		return true;
> > > > > +	default:
> > > > > +		return false;
> > > > > +	}
> > > > > +}
> > > > > +
> > > > > +static bool lochnagar2_volatile_register(struct device *dev, unsigned int reg)
> > > > > +{
> > > > > +	switch (reg) {
> > > > > +	case LOCHNAGAR2_GPIO_CHANNEL1:
> > > > > +	case LOCHNAGAR2_GPIO_CHANNEL2:
> > > > > +	case LOCHNAGAR2_GPIO_CHANNEL3:
> > > ...
> > > > > +	case LOCHNAGAR2_GPIO_CHANNEL13:
> > > > > +	case LOCHNAGAR2_GPIO_CHANNEL14:
> > > > > +	case LOCHNAGAR2_GPIO_CHANNEL15:
> > > > > +	case LOCHNAGAR2_GPIO_CHANNEL16:
> > > > > +	case LOCHNAGAR2_ANALOGUE_PATH_CTRL1:
> > > > > +		return true;
> > > > > +	default:
> > > > > +		return false;
> > > > > +	}
> > > > > +}
> > > > 
> > > > This is getting silly now.  Can't you use ranges?
> > > 
> > > I can if you feel strongly about it? But it does make the drivers
> > > much more error prone and significantly more annoying to work
> > > with. I find it is really common to be checking that a register
> > > is handled correctly through the regmap callbacks and it is nice
> > > to just be able to grep for that. Obviously this won't work for
> > > all devices/regmaps as well since many will not have consecutive
> > > addresses on registers, for example having multi-byte registers
> > > that are byte addressed.
> > > 
> > > How far would you like me to take this as well? Is it just the
> > > numeric registers you want ranges for ie.
> > > 
> > > LOCHNAGAR2_GPIO_CHANNEL1...LOCHNAGAR_GPIO_CHANNEL16
> > > 
> > > Or is it all consecutive registers even if they are unrelated
> > > (exmaple is probably not accurate as I haven't checked the
> > > addresses):
> > > 
> > > LOCHNAGAR2_GPIO_CHANNEL1...LOCHNAGAR2_ANALOGURE_PATH_CTRL1
> > > 
> > > I don't mind the first at all but the second is getting really
> > > horrible in my opinion.
> 
> My issue is that we have one end of the scale, where contributors are
> submitting patches, trying to remove a line of code here, a line of
> code there, then there are patches like this one which describe the
> initial value, readable status, writable status and volatile status of
> each and every register.
> 

Removing code is one thing but I would argue that data tables are
somewhat less of a concern. I guess kernel size for very small
systems but then is someone going to be using Lochnagar on one of
those.

> The API is obscene and needs a re-work IMHO.
> 
> I really hope we do not really have to list every register, but *if we
> absolutely must*, let's do it once:
> 
>   REG_ADDRESS, WRV, INITIAL_VALUE
> 

It might be possible to combine all these into one thing,
although again its a fairly major core change.

> > > > > +static const struct reg_default lochnagar2_reg_defaults[] = {
> > > > > +	{ LOCHNAGAR2_CDC_AIF1_CTRL,         0x0000 },
> > > > > +	{ LOCHNAGAR2_CDC_AIF2_CTRL,         0x0000 },
> > > > > +	{ LOCHNAGAR2_CDC_AIF3_CTRL,         0x0000 },
> > > > > +	{ LOCHNAGAR2_DSP_AIF1_CTRL,         0x0000 },
> > > ...
> > > > > +	{ LOCHNAGAR2_MINICARD_RESETS,       0x0000 },
> > > > > +	{ LOCHNAGAR2_ANALOGUE_PATH_CTRL2,   0x0000 },
> > > > > +	{ LOCHNAGAR2_COMMS_CTRL4,           0x0001 },
> > > > > +	{ LOCHNAGAR2_SPDIF_CTRL,            0x0008 },
> > > > > +	{ LOCHNAGAR2_POWER_CTRL,            0x0001 },
> > > > > +	{ LOCHNAGAR2_SOUNDCARD_AIF_CTRL,    0x0000 },
> > > > > +};
> > > > 
> > > > OMG!  Vile, vile vile!
> > > 
> > > I really feel this isn't the driver you are objecting to as such
> > > but the way regmap operates and also we seem to always have the same
> > > discussions around regmap every time we push a driver.
> 
> Absolutely.  I didn't like it before.  I like it even less now.
> 

I guess the question from my side becomes do you want to block
this driver pending on major refactoring to regmap? I will have a
think about what I can do but its going to affect a LOT of drivers.

> > > > > +	ret = devm_of_platform_populate(dev);
> > > > > +	if (ret < 0) {
> > > > > +		dev_err(dev, "Failed to populate child nodes: %d\n", ret);
> > > > > +		return ret;
> > > > > +	}
> > > > 
> > > > Please do not mix OF and MFD device registration strategies.
> > > > 
> > > > Pick one and register all devices through your chosen method.
> > > 
> > > Hmmm we use this to do things like register some fixed regulators
> > > and clocks that don't need any control but do need to be associated
> > > with the device. I could do that through the MFD although it is in
> > > direct conflict with the feedback on the clock patches I received
> > > to move the fixed clocks into devicetree rather than registering
> > > them manually (see v2 of the patch chain).
> 
> The I suggest moving everything to DT.
> 

I will have a look and see what that would look like.

Thanks,
Charles
Charles Keepax Oct. 25, 2018, 1:20 p.m. UTC | #8
On Thu, Oct 25, 2018 at 01:49:05PM +0100, Charles Keepax wrote:
> On Thu, Oct 25, 2018 at 12:42:05PM +0100, Lee Jones wrote:
> > On Thu, 25 Oct 2018, Richard Fitzgerald wrote:
> > > On 25/10/18 09:26, Charles Keepax wrote:
> > > > On Thu, Oct 25, 2018 at 08:44:59AM +0100, Lee Jones wrote:
> > > > > On Mon, 08 Oct 2018, Charles Keepax wrote:
> > > > > > From: Charles Keepax <ckeepax@opensource.wolfsonmicro.com>
> > > > I really feel this isn't the driver you are objecting to as such
> > > > but the way regmap operates and also we seem to always have the same
> > > > discussions around regmap every time we push a driver.
> > 
> > Absolutely.  I didn't like it before.  I like it even less now.
> > 
> 
> I guess the question from my side becomes do you want to block
> this driver pending on major refactoring to regmap? I will have a
> think about what I can do but its going to affect a LOT of drivers.
> 

Actually one more thought, perhaps as a halfway for now i could
look into removing the readables and defaults. We lose some things
like error checking that we are reading real registers but as
this driver doesnt currently do cache syncs we might be able to
get away with this for now.

Unless anyone strenuously objects i will have a look at the
options there. As well as looking at wider refactoring but aiming
further out.

Thanks,
Charles
Richard Fitzgerald Oct. 25, 2018, 1:40 p.m. UTC | #9
On 25/10/18 12:42, Lee Jones wrote:
> On Thu, 25 Oct 2018, Richard Fitzgerald wrote:
>> On 25/10/18 09:26, Charles Keepax wrote:
>>> On Thu, Oct 25, 2018 at 08:44:59AM +0100, Lee Jones wrote:
>>>> On Mon, 08 Oct 2018, Charles Keepax wrote:
>>>>> From: Charles Keepax <ckeepax@opensource.wolfsonmicro.com>
>>>>> +static const struct reg_default lochnagar1_reg_defaults[] = {
>>>>> +	{ LOCHNAGAR1_CDC_AIF1_SEL,    0x00 },
>>>>> +	{ LOCHNAGAR1_CDC_AIF2_SEL,    0x00 },
>>> ...
>>>>> +	{ LOCHNAGAR1_LED1,            0x00 },
>>>>> +	{ LOCHNAGAR1_LED2,            0x00 },
>>>>> +	{ LOCHNAGAR1_I2C_CTRL,        0x01 },
>>>>> +};
>>>>
>>>> Why do you need to specify each register value?
>>>
>>> The way regmap operates it needs to know the starting value of
>>> each register. It will use this to initialise the cache and to
>>> determine if writes need to actually update the hardware on
>>> cache_syncs after devices have been powered back up.
> 
> That sounds crazy to me.  Some devices have thousands of registers.

Largely the point. How long do you think it would take to populate the
cache if you had to read thousands of registers over I2C? Boot time matters.
Deferring it until it's touched can create various unpredictable and
annoying behaviour later, for example if a lot of cache entries are
written while the chip is asleep and the initial values weren't known
then a suspend/resume cannot filter out writes that are setting the
register to its default (which regmap does to avoid unnecessary bus traffic).
So the resume could have a large amount of unnecessary overhead writing
registers to a value they already have or reading the initial values of
those registers.

> At a line per register, that's thousands of lines of code/cruft.
> Especially seeing as most (sane?) register layouts I've seen default
> to zero.

Not a valid generalization. And it's not a question of sanity, the purpose
of the register and the overhead to setup a use-case also matter.
There are many reasons why the default of a register might not be zero.
Take a look at drivers/mfd/wm5110-tables.c, a lot of the registers don't
have a default of zero (and that's only the registers accessed by the driver.)
It's particularly true of registers that affect things like voltage and
current sources, zero might be a very inappropriate default - even dangerous.
Also enable bits, if some things must power-up enabled and others don't, unless
you want a chip that has a confusing mix of inverted and non-inverted enable
bits. Another side to this is to reduce the number of writes to enable _typical_
behaviour - if an IP block has say 100 registers and you have to write all of
them to make it work that's a lot of overhead compared to them defaulting to
typical values used 99.9% of the time and you only need to write one or two
registers to use it.

   Then default values can be changed at the leisure of the
> s/w.

Potentially with a lot of overhead, especially on those chips with thousands
of registers to set to useful non-zero values before you can use it.

Lochnagar doesn't have that many registers but convention and consistency also
comes into play. Regmap is used in a particular way and it helps people a lot
if every driver using it follows the convention.

> 
> Even if it is absolutely critical that you have to supply these to
> Regmap up-front, instead of on first use/read, why can't you just
> supply the oddball non-zero ones?
> 

If you aren't happy with the regmap subsystem you could send some
patches to change it to what you would be happy with (and patch the ~1300
drivers that use it)

Like any kernel subsystem it has an API that we have to obey to be able to
use it.

>>>>> +static const struct reg_sequence lochnagar1_patch[] = {
>>>>> +	{ 0x40, 0x0083 },
>>>>> +	{ 0x46, 0x0001 },
>>>>> +	{ 0x47, 0x0018 },
>>>>> +	{ 0x50, 0x0000 },
>>>>> +};
>>>>
>>>> I'm really not a fan of these so call 'patches'.
>>>>
>>>> Can't you set the registers up proper way?
>>>>
>>>
>>> I will see if we could move any out of here or define any of the
>>> registers but as we have discussed before it is not always possible.
>>>
>>
>> Also patches generally come out of hardware tuning/qualification/tools
>> as this list of address,value. So it's easy for people to dump an update
>> into the driver as a trivial copy-paste but more work if they have to
>> reverse-engineer the patch list from hardware/datasheet into what each
>> line "means" and then find the relevant lines of code to change. It's also
>> much easier to answer the question "Have these hardware patches been
>> applied to the driver?" if we have them in the original documented format.
>> It just makes people's lives more difficult if they have to search around
>> the code to try to find something that looks like the originally specified
>> patch list. We don't use them just as a lazy way to setup some registers.
> 
> I understand why they normally exist (sometimes people are just lazy
> too) (Mark: BTW chicken-bits sound delicious!).  They're just ugly
> from an Open Source PoV.
> 

In my opinion a lot of the source code in Linux is much uglier than
these tables.

>>>>> +static bool lochnagar2_readable_register(struct device *dev, unsigned int reg)
>>>>> +{
>>>>> +	switch (reg) {
>>>>> +	case LOCHNAGAR_SOFTWARE_RESET:
>>>>> +	case LOCHNAGAR_FIRMWARE_ID1:
>>>>> +	case LOCHNAGAR_FIRMWARE_ID2:
>>> ...
>>>>> +	case LOCHNAGAR2_MICVDD_CTRL2:
>>>>> +	case LOCHNAGAR2_VDDCORE_CDC_CTRL1:
>>>>> +	case LOCHNAGAR2_VDDCORE_CDC_CTRL2:
>>>>> +	case LOCHNAGAR2_SOUNDCARD_AIF_CTRL:
>>>>> +		return true;
>>>>> +	default:
>>>>> +		return false;
>>>>> +	}
>>>>> +}
>>>>> +
>>>>> +static bool lochnagar2_volatile_register(struct device *dev, unsigned int reg)
>>>>> +{
>>>>> +	switch (reg) {
>>>>> +	case LOCHNAGAR2_GPIO_CHANNEL1:
>>>>> +	case LOCHNAGAR2_GPIO_CHANNEL2:
>>>>> +	case LOCHNAGAR2_GPIO_CHANNEL3:
>>> ...
>>>>> +	case LOCHNAGAR2_GPIO_CHANNEL13:
>>>>> +	case LOCHNAGAR2_GPIO_CHANNEL14:
>>>>> +	case LOCHNAGAR2_GPIO_CHANNEL15:
>>>>> +	case LOCHNAGAR2_GPIO_CHANNEL16:
>>>>> +	case LOCHNAGAR2_ANALOGUE_PATH_CTRL1:
>>>>> +		return true;
>>>>> +	default:
>>>>> +		return false;
>>>>> +	}
>>>>> +}
>>>>
>>>> This is getting silly now.  Can't you use ranges?
>>>
>>> I can if you feel strongly about it? But it does make the drivers
>>> much more error prone and significantly more annoying to work
>>> with. I find it is really common to be checking that a register
>>> is handled correctly through the regmap callbacks and it is nice
>>> to just be able to grep for that. Obviously this won't work for
>>> all devices/regmaps as well since many will not have consecutive
>>> addresses on registers, for example having multi-byte registers
>>> that are byte addressed.
>>>
>>> How far would you like me to take this as well? Is it just the
>>> numeric registers you want ranges for ie.
>>>
>>> LOCHNAGAR2_GPIO_CHANNEL1...LOCHNAGAR_GPIO_CHANNEL16
>>>
>>> Or is it all consecutive registers even if they are unrelated
>>> (exmaple is probably not accurate as I haven't checked the
>>> addresses):
>>>
>>> LOCHNAGAR2_GPIO_CHANNEL1...LOCHNAGAR2_ANALOGURE_PATH_CTRL1
>>>
>>> I don't mind the first at all but the second is getting really
>>> horrible in my opinion.
> 
> My issue is that we have one end of the scale, where contributors are
> submitting patches, trying to remove a line of code here, a line of
> code there, then there are patches like this one which describe the
> initial value, readable status, writable status and volatile status of
> each and every register.
> 

If we could add support for new devices by removing lines of code that
would be cool :). Eventually Linux would support every piece of hardware
and be zero lines of code.

> The API is obscene and needs a re-work IMHO.
> 
> I really hope we do not really have to list every register, but *if we
> absolutely must*, let's do it once:
> 
>    REG_ADDRESS, WRV, INITIAL_VALUE
> 

To re-iterate, regmap is a standard kernel subsystem. It's not owned by Cirrus,
so it's not our responsibility if you don't like it. Mark Brown is the maintainer.

Submit your patches to Mark and the owners of those ~1300 drivers to propose
changes to regmap that you would be happy with.

>>>>> +static const struct reg_default lochnagar2_reg_defaults[] = {
>>>>> +	{ LOCHNAGAR2_CDC_AIF1_CTRL,         0x0000 },
>>>>> +	{ LOCHNAGAR2_CDC_AIF2_CTRL,         0x0000 },
>>>>> +	{ LOCHNAGAR2_CDC_AIF3_CTRL,         0x0000 },
>>>>> +	{ LOCHNAGAR2_DSP_AIF1_CTRL,         0x0000 },
>>> ...
>>>>> +	{ LOCHNAGAR2_MINICARD_RESETS,       0x0000 },
>>>>> +	{ LOCHNAGAR2_ANALOGUE_PATH_CTRL2,   0x0000 },
>>>>> +	{ LOCHNAGAR2_COMMS_CTRL4,           0x0001 },
>>>>> +	{ LOCHNAGAR2_SPDIF_CTRL,            0x0008 },
>>>>> +	{ LOCHNAGAR2_POWER_CTRL,            0x0001 },
>>>>> +	{ LOCHNAGAR2_SOUNDCARD_AIF_CTRL,    0x0000 },
>>>>> +};
>>>>
>>>> OMG!  Vile, vile vile!
>>>
>>> I really feel this isn't the driver you are objecting to as such
>>> but the way regmap operates and also we seem to always have the same
>>> discussions around regmap every time we push a driver.
> 
> Absolutely.  I didn't like it before.  I like it even less now.
> 
>>> Is there
>>> any way me, you and Mark could hash this out and find out a way to
>>> handle regmaps that is acceptable to you? I don't suppose you are
>>> in Edinburgh at the moment for ELCE?
> 
> I'm not at ELCE I'm afraid.
> 
>> I suppose if Mark was willing to promote the regmap drivers to be a
>> top-level subsystem that could contain the regmap definitions of devices
>> then we could dump our regmap definitions in there, where Mark can review
>> it as he's familiar with regmap and the chips and the reasons why things
>> are done the way they are, rather than Lee having to stress about it every
>> time we need to create an MFD device that uses regmap. Though that would
>> make the initialization of an MFD rather awkward with the code required
>> to init the MFD it not actually being in the MFD tree.
> 
> My issue isn't where all this bumph lives.
> 
> It's the fact that it's required (at least at this level) at all.
> 

As above, if one subsystem owner doesn't like another subsystem then those
subsystem owners should talk to each other and sort something out. It shouldn't
block patches that are just trying to use the subsystem as it currently exists
in the kernel.

>>>>> +	/* Wait for Lochnagar to boot */
>>>>> +	ret = lochnagar_wait_for_boot(lochnagar->regmap, &val);
>>>>> +	if (ret < 0) {
>>>>> +		dev_err(dev, "Failed to read device ID: %d\n", ret);
>>>>
>>>> Eh?
>>>>
>>>> So you read the LOCHNAGAR_SOFTWARE_RESET register and out pops the
>>>> device/revision IDs?  That's just random!
>>>
>>> I shall let the hardware guys know you don't approve of their
>>> life choices :-) and add some comments to the code.
> 
> Please do.  And tell them to stop drinking at lunch time. ;)
> 
>>>>> +	ret = devm_of_platform_populate(dev);
>>>>> +	if (ret < 0) {
>>>>> +		dev_err(dev, "Failed to populate child nodes: %d\n", ret);
>>>>> +		return ret;
>>>>> +	}
>>>>
>>>> Please do not mix OF and MFD device registration strategies.
>>>>
>>>> Pick one and register all devices through your chosen method.
>>>
>>> Hmmm we use this to do things like register some fixed regulators
>>> and clocks that don't need any control but do need to be associated
>>> with the device. I could do that through the MFD although it is in
>>> direct conflict with the feedback on the clock patches I received
>>> to move the fixed clocks into devicetree rather than registering
>>> them manually (see v2 of the patch chain).
> 
> The I suggest moving everything to DT.
> 
>>> I will have a look see if I can find any ideas that will make
>>> everyone happy but we might need to discuss with Mark and the
>>> clock guys.
>>>
>>>>> +	.probe_new = lochnagar_i2c_probe,
>>>>
>>>> Hasn't this been replaced yet?
>>>
>>> I will check, the patchset has been around internally for a while
>>> so it is possible this is no longer needed.
>>>
>>>>> +#ifndef CIRRUS_LOCHNAGAR_H
>>>>> +#define CIRRUS_LOCHNAGAR_H
>>>>> +
>>>>> +#include "lochnagar1_regs.h"
>>>>> +#include "lochnagar2_regs.h"
>>>>
>>>> Why are you including these here?
>>>>
>>>
>>> It is just a convenience so the drivers only need to include
>>> lochnagar.h rather than including all three headers manually.
> 
> That's against convention.
> 
> If a source file needs a head, it should include it explicitly.
> 
>>>>> diff --git a/include/linux/mfd/lochnagar1_regs.h b/include/linux/mfd/lochnagar1_regs.h
>>>>> diff --git a/include/linux/mfd/lochnagar2_regs.h b/include/linux/mfd/lochnagar2_regs.h
>>>>
>>>> So Lochnagar 1 and 2 are completely different devices?
>>>>
>>>> What do they do?
>>>
>>> Completely different devices is a bit strong, they are different
>>> versions of the same system. They have quite different register
>>> maps but provide very similar functionality.
>>
>> The register maps are different partly because some silicon
>> used on the V1 is no longer manufactured and partly because some
>> silicon added in V2 didn't fit into the older register map.
> 
> I just looked at the maps, which appeared to be vastly different.
> 

We said the maps are different.

>>> All the other comments I will get fixed up for the next spin of
>>> the patches.
>
Richard Fitzgerald Oct. 25, 2018, 1:47 p.m. UTC | #10
On 25/10/18 14:20, Charles Keepax wrote:
> On Thu, Oct 25, 2018 at 01:49:05PM +0100, Charles Keepax wrote:
>> On Thu, Oct 25, 2018 at 12:42:05PM +0100, Lee Jones wrote:
>>> On Thu, 25 Oct 2018, Richard Fitzgerald wrote:
>>>> On 25/10/18 09:26, Charles Keepax wrote:
>>>>> On Thu, Oct 25, 2018 at 08:44:59AM +0100, Lee Jones wrote:
>>>>>> On Mon, 08 Oct 2018, Charles Keepax wrote:
>>>>>>> From: Charles Keepax <ckeepax@opensource.wolfsonmicro.com>
>>>>> I really feel this isn't the driver you are objecting to as such
>>>>> but the way regmap operates and also we seem to always have the same
>>>>> discussions around regmap every time we push a driver.
>>>
>>> Absolutely.  I didn't like it before.  I like it even less now.
>>>
>>
>> I guess the question from my side becomes do you want to block
>> this driver pending on major refactoring to regmap? I will have a
>> think about what I can do but its going to affect a LOT of drivers.
>>
> 
> Actually one more thought, perhaps as a halfway for now i could
> look into removing the readables

Be careful with that, there are some addresses that are illegal to
access. What does regmap debugfs do if you don't have a readables
list? Just reading a debugfs shouldn't be able to kill the hardware.
You might need to add a precious list which is more error prone
than listing the valid readables we are using.

  and defaults. We lose some things
> like error checking that we are reading real registers but as
> this driver doesnt currently do cache syncs we might be able to
> get away with this for now.
> 
> Unless anyone strenuously objects i will have a look at the
> options there. As well as looking at wider refactoring but aiming
> further out.
> 
> Thanks,
> Charles
>
Lee Jones Oct. 26, 2018, 7:33 a.m. UTC | #11
On Thu, 25 Oct 2018, Charles Keepax wrote:

> On Thu, Oct 25, 2018 at 12:42:05PM +0100, Lee Jones wrote:
> > On Thu, 25 Oct 2018, Richard Fitzgerald wrote:
> > > On 25/10/18 09:26, Charles Keepax wrote:
> > > > On Thu, Oct 25, 2018 at 08:44:59AM +0100, Lee Jones wrote:
> > > > > On Mon, 08 Oct 2018, Charles Keepax wrote:
> > > > > > From: Charles Keepax <ckeepax@opensource.wolfsonmicro.com>
> > > > > > +static const struct reg_default lochnagar1_reg_defaults[] = {
> > > > > > +	{ LOCHNAGAR1_CDC_AIF1_SEL,    0x00 },
> > > > > > +	{ LOCHNAGAR1_CDC_AIF2_SEL,    0x00 },
> > > > ...
> > > > > > +	{ LOCHNAGAR1_LED1,            0x00 },
> > > > > > +	{ LOCHNAGAR1_LED2,            0x00 },
> > > > > > +	{ LOCHNAGAR1_I2C_CTRL,        0x01 },
> > > > > > +};
> > > > > 
> > > > > Why do you need to specify each register value?
> > > > 
> > > > The way regmap operates it needs to know the starting value of
> > > > each register. It will use this to initialise the cache and to
> > > > determine if writes need to actually update the hardware on
> > > > cache_syncs after devices have been powered back up.
> > 
> > That sounds crazy to me.  Some devices have thousands of registers.
> > At a line per register, that's thousands of lines of code/cruft.
> > Especially seeing as most (sane?) register layouts I've seen default
> > to zero.  Then default values can be changed at the leisure of the
> > s/w.
> > 
> > Even if it is absolutely critical that you have to supply these to
> > Regmap up-front, instead of on first use/read, why can't you just
> > supply the oddball non-zero ones?
> 
> I don't think you can sensibly get away with not supplying
> default values. You say most sane register layouts have zero
> values, alas again you may not be the biggest fan of our hardware
> guys. The Lochnagar actually does have mostly zero defaults but
> that is very much not generally the case with our hardware.
> 
> One of the main aims of regmap is to avoid bus accesses when
> possible but I guess on the first write/read to any register you
> could insert a read to pull the default, although I do worry
> there is some corner case/flexibility that is going to cause
> headaches later.

This is basically what I am suggesting.

> I am not sure that dynamically constructing the defaults would be
> possible from a table of non-zero ones, or at least doing so would
> probably require a fairly major change to the way registers are
> specified since with the current callback based approach and a
> sparse regmap you could need to iterate over billions of
> potential registers to build the table.

What if a valid register range was provided with only the non-zero
values?

> > > > > > +static bool lochnagar2_readable_register(struct device *dev, unsigned int reg)
> > > > > > +{
> > > > > > +	switch (reg) {
> > > > > > +	case LOCHNAGAR_SOFTWARE_RESET:
> > > > > > +	case LOCHNAGAR_FIRMWARE_ID1:
> > > > > > +	case LOCHNAGAR_FIRMWARE_ID2:
> > > > ...
> > > > > > +	case LOCHNAGAR2_MICVDD_CTRL2:
> > > > > > +	case LOCHNAGAR2_VDDCORE_CDC_CTRL1:
> > > > > > +	case LOCHNAGAR2_VDDCORE_CDC_CTRL2:
> > > > > > +	case LOCHNAGAR2_SOUNDCARD_AIF_CTRL:
> > > > > > +		return true;
> > > > > > +	default:
> > > > > > +		return false;
> > > > > > +	}
> > > > > > +}
> > > > > > +
> > > > > > +static bool lochnagar2_volatile_register(struct device *dev, unsigned int reg)
> > > > > > +{
> > > > > > +	switch (reg) {
> > > > > > +	case LOCHNAGAR2_GPIO_CHANNEL1:
> > > > > > +	case LOCHNAGAR2_GPIO_CHANNEL2:
> > > > > > +	case LOCHNAGAR2_GPIO_CHANNEL3:
> > > > ...
> > > > > > +	case LOCHNAGAR2_GPIO_CHANNEL13:
> > > > > > +	case LOCHNAGAR2_GPIO_CHANNEL14:
> > > > > > +	case LOCHNAGAR2_GPIO_CHANNEL15:
> > > > > > +	case LOCHNAGAR2_GPIO_CHANNEL16:
> > > > > > +	case LOCHNAGAR2_ANALOGUE_PATH_CTRL1:
> > > > > > +		return true;
> > > > > > +	default:
> > > > > > +		return false;
> > > > > > +	}
> > > > > > +}
> > > > > 
> > > > > This is getting silly now.  Can't you use ranges?
> > > > 
> > > > I can if you feel strongly about it? But it does make the drivers
> > > > much more error prone and significantly more annoying to work
> > > > with. I find it is really common to be checking that a register
> > > > is handled correctly through the regmap callbacks and it is nice
> > > > to just be able to grep for that. Obviously this won't work for
> > > > all devices/regmaps as well since many will not have consecutive
> > > > addresses on registers, for example having multi-byte registers
> > > > that are byte addressed.
> > > > 
> > > > How far would you like me to take this as well? Is it just the
> > > > numeric registers you want ranges for ie.
> > > > 
> > > > LOCHNAGAR2_GPIO_CHANNEL1...LOCHNAGAR_GPIO_CHANNEL16
> > > > 
> > > > Or is it all consecutive registers even if they are unrelated
> > > > (exmaple is probably not accurate as I haven't checked the
> > > > addresses):
> > > > 
> > > > LOCHNAGAR2_GPIO_CHANNEL1...LOCHNAGAR2_ANALOGURE_PATH_CTRL1
> > > > 
> > > > I don't mind the first at all but the second is getting really
> > > > horrible in my opinion.
> > 
> > My issue is that we have one end of the scale, where contributors are
> > submitting patches, trying to remove a line of code here, a line of
> > code there, then there are patches like this one which describe the
> > initial value, readable status, writable status and volatile status of
> > each and every register.
> 
> Removing code is one thing but I would argue that data tables are
> somewhat less of a concern. I guess kernel size for very small
> systems but then is someone going to be using Lochnagar on one of
> those.
> 
> > The API is obscene and needs a re-work IMHO.
> > 
> > I really hope we do not really have to list every register, but *if we
> > absolutely must*, let's do it once:
> > 
> >   REG_ADDRESS, WRV, INITIAL_VALUE
> 
> It might be possible to combine all these into one thing,
> although again its a fairly major core change.

I'm not suggesting that this will be solved overnight.

> > > > > > +static const struct reg_default lochnagar2_reg_defaults[] = {
> > > > > > +	{ LOCHNAGAR2_CDC_AIF1_CTRL,         0x0000 },
> > > > > > +	{ LOCHNAGAR2_CDC_AIF2_CTRL,         0x0000 },
> > > > > > +	{ LOCHNAGAR2_CDC_AIF3_CTRL,         0x0000 },
> > > > > > +	{ LOCHNAGAR2_DSP_AIF1_CTRL,         0x0000 },
> > > > ...
> > > > > > +	{ LOCHNAGAR2_MINICARD_RESETS,       0x0000 },
> > > > > > +	{ LOCHNAGAR2_ANALOGUE_PATH_CTRL2,   0x0000 },
> > > > > > +	{ LOCHNAGAR2_COMMS_CTRL4,           0x0001 },
> > > > > > +	{ LOCHNAGAR2_SPDIF_CTRL,            0x0008 },
> > > > > > +	{ LOCHNAGAR2_POWER_CTRL,            0x0001 },
> > > > > > +	{ LOCHNAGAR2_SOUNDCARD_AIF_CTRL,    0x0000 },
> > > > > > +};
> > > > > 
> > > > > OMG!  Vile, vile vile!
> > > > 
> > > > I really feel this isn't the driver you are objecting to as such
> > > > but the way regmap operates and also we seem to always have the same
> > > > discussions around regmap every time we push a driver.
> > 
> > Absolutely.  I didn't like it before.  I like it even less now.
> 
> I guess the question from my side becomes do you want to block
> this driver pending on major refactoring to regmap? I will have a
> think about what I can do but its going to affect a LOT of drivers.

No one is NACKing this driver.

We're using it as a vehicle for discussion.

> > > > > > +	ret = devm_of_platform_populate(dev);
> > > > > > +	if (ret < 0) {
> > > > > > +		dev_err(dev, "Failed to populate child nodes: %d\n", ret);
> > > > > > +		return ret;
> > > > > > +	}
> > > > > 
> > > > > Please do not mix OF and MFD device registration strategies.
> > > > > 
> > > > > Pick one and register all devices through your chosen method.
> > > > 
> > > > Hmmm we use this to do things like register some fixed regulators
> > > > and clocks that don't need any control but do need to be associated
> > > > with the device. I could do that through the MFD although it is in
> > > > direct conflict with the feedback on the clock patches I received
> > > > to move the fixed clocks into devicetree rather than registering
> > > > them manually (see v2 of the patch chain).
> > 
> > The I suggest moving everything to DT.
> 
> I will have a look and see what that would look like.

Thanks.
Lee Jones Oct. 26, 2018, 8 a.m. UTC | #12
On Thu, 25 Oct 2018, Richard Fitzgerald wrote:

> On 25/10/18 12:42, Lee Jones wrote:
> > On Thu, 25 Oct 2018, Richard Fitzgerald wrote:
> > > On 25/10/18 09:26, Charles Keepax wrote:
> > > > On Thu, Oct 25, 2018 at 08:44:59AM +0100, Lee Jones wrote:
> > > > > On Mon, 08 Oct 2018, Charles Keepax wrote:
> > > > > > From: Charles Keepax <ckeepax@opensource.wolfsonmicro.com>
> > > > > > +static const struct reg_default lochnagar1_reg_defaults[] = {
> > > > > > +	{ LOCHNAGAR1_CDC_AIF1_SEL,    0x00 },
> > > > > > +	{ LOCHNAGAR1_CDC_AIF2_SEL,    0x00 },
> > > > ...
> > > > > > +	{ LOCHNAGAR1_LED1,            0x00 },
> > > > > > +	{ LOCHNAGAR1_LED2,            0x00 },
> > > > > > +	{ LOCHNAGAR1_I2C_CTRL,        0x01 },
> > > > > > +};
> > > > > 
> > > > > Why do you need to specify each register value?
> > > > 
> > > > The way regmap operates it needs to know the starting value of
> > > > each register. It will use this to initialise the cache and to
> > > > determine if writes need to actually update the hardware on
> > > > cache_syncs after devices have been powered back up.
> > 
> > That sounds crazy to me.  Some devices have thousands of registers.
> 
> Largely the point. How long do you think it would take to populate the
> cache if you had to read thousands of registers over I2C? Boot time matters.
> Deferring it until it's touched can create various unpredictable and
> annoying behaviour later, for example if a lot of cache entries are
> written while the chip is asleep and the initial values weren't known
> then a suspend/resume cannot filter out writes that are setting the
> register to its default (which regmap does to avoid unnecessary bus traffic).
> So the resume could have a large amount of unnecessary overhead writing
> registers to a value they already have or reading the initial values of
> those registers.

One more register read when initially writing to a register and one
more when resuming doesn't sound like a vast amount of over-head.

> > At a line per register, that's thousands of lines of code/cruft.
> > Especially seeing as most (sane?) register layouts I've seen default
> > to zero.
> 
> Not a valid generalization. And it's not a question of sanity, the purpose
> of the register and the overhead to setup a use-case also matter.
> There are many reasons why the default of a register might not be zero.
> Take a look at drivers/mfd/wm5110-tables.c, a lot of the registers don't
> have a default of zero (and that's only the registers accessed by the driver.)
> It's particularly true of registers that affect things like voltage and
> current sources, zero might be a very inappropriate default - even dangerous.
> Also enable bits, if some things must power-up enabled and others don't, unless
> you want a chip that has a confusing mix of inverted and non-inverted enable
> bits. Another side to this is to reduce the number of writes to enable _typical_
> behaviour - if an IP block has say 100 registers and you have to write all of
> them to make it work that's a lot of overhead compared to them defaulting to
> typical values used 99.9% of the time and you only need to write one or two
> registers to use it.

Not sure what you think I was suggesting above.  If the default values
are actually non-zero that's fine - we'll either leave them as they
are (if they are never changed, in which case Regmap doesn't even need
to know about them), document only those (non-zero) ones or wait until
they are read for the first time, then populate the cache.

Setting up the cache manually also sounds like a vector for potential
failure.  At least if you were to cache dynamically on first write
(either on start-up or after sleep) then the actual value will be
cached, rather than what a piece of C code says it should be.

>   Then default values can be changed at the leisure of the
> > s/w.
> 
> Potentially with a lot of overhead, especially on those chips with thousands
> of registers to set to useful non-zero values before you can use it.
> 
> Lochnagar doesn't have that many registers but convention and consistency also
> comes into play. Regmap is used in a particular way and it helps people a lot
> if every driver using it follows the convention.

Precisely my point.  Lochnagar is a small device yet it's required to
submit hundreds of lines of Regmap tables.  Imagine what that would
look like for a large device.

> > Even if it is absolutely critical that you have to supply these to
> > Regmap up-front, instead of on first use/read, why can't you just
> > supply the oddball non-zero ones?
> 
> If you aren't happy with the regmap subsystem you could send some
> patches to change it to what you would be happy with (and patch the ~1300
> drivers that use it)

That may well happen.  This is the pre-patch discussion.

Apologies that it had to happen on your submission, but this is that
alerted me to the issue(s).

> Like any kernel subsystem it has an API that we have to obey to be able to
> use it.

Again, this isn't about Lochnagar.

> > > > > > +static const struct reg_sequence lochnagar1_patch[] = {
> > > > > > +	{ 0x40, 0x0083 },
> > > > > > +	{ 0x46, 0x0001 },
> > > > > > +	{ 0x47, 0x0018 },
> > > > > > +	{ 0x50, 0x0000 },
> > > > > > +};
> > > > > 
> > > > > I'm really not a fan of these so call 'patches'.
> > > > > 
> > > > > Can't you set the registers up proper way?
> > > > > 
> > > > 
> > > > I will see if we could move any out of here or define any of the
> > > > registers but as we have discussed before it is not always possible.
> > > > 
> > > 
> > > Also patches generally come out of hardware tuning/qualification/tools
> > > as this list of address,value. So it's easy for people to dump an update
> > > into the driver as a trivial copy-paste but more work if they have to
> > > reverse-engineer the patch list from hardware/datasheet into what each
> > > line "means" and then find the relevant lines of code to change. It's also
> > > much easier to answer the question "Have these hardware patches been
> > > applied to the driver?" if we have them in the original documented format.
> > > It just makes people's lives more difficult if they have to search around
> > > the code to try to find something that looks like the originally specified
> > > patch list. We don't use them just as a lazy way to setup some registers.
> > 
> > I understand why they normally exist (sometimes people are just lazy
> > too) (Mark: BTW chicken-bits sound delicious!).  They're just ugly
> > from an Open Source PoV.
> 
> In my opinion a lot of the source code in Linux is much uglier than
> these tables.

Right.  I usually comment on those (when I see them) too.

Besides, just because some people committing murder, doesn't mean
other people shouldn't go to jail for stealing a car.

> > > > > > +static bool lochnagar2_readable_register(struct device *dev, unsigned int reg)
> > > > > > +{
> > > > > > +	switch (reg) {
> > > > > > +	case LOCHNAGAR_SOFTWARE_RESET:
> > > > > > +	case LOCHNAGAR_FIRMWARE_ID1:
> > > > > > +	case LOCHNAGAR_FIRMWARE_ID2:
> > > > ...
> > > > > > +	case LOCHNAGAR2_MICVDD_CTRL2:
> > > > > > +	case LOCHNAGAR2_VDDCORE_CDC_CTRL1:
> > > > > > +	case LOCHNAGAR2_VDDCORE_CDC_CTRL2:
> > > > > > +	case LOCHNAGAR2_SOUNDCARD_AIF_CTRL:
> > > > > > +		return true;
> > > > > > +	default:
> > > > > > +		return false;
> > > > > > +	}
> > > > > > +}
> > > > > > +
> > > > > > +static bool lochnagar2_volatile_register(struct device *dev, unsigned int reg)
> > > > > > +{
> > > > > > +	switch (reg) {
> > > > > > +	case LOCHNAGAR2_GPIO_CHANNEL1:
> > > > > > +	case LOCHNAGAR2_GPIO_CHANNEL2:
> > > > > > +	case LOCHNAGAR2_GPIO_CHANNEL3:
> > > > ...
> > > > > > +	case LOCHNAGAR2_GPIO_CHANNEL13:
> > > > > > +	case LOCHNAGAR2_GPIO_CHANNEL14:
> > > > > > +	case LOCHNAGAR2_GPIO_CHANNEL15:
> > > > > > +	case LOCHNAGAR2_GPIO_CHANNEL16:
> > > > > > +	case LOCHNAGAR2_ANALOGUE_PATH_CTRL1:
> > > > > > +		return true;
> > > > > > +	default:
> > > > > > +		return false;
> > > > > > +	}
> > > > > > +}
> > > > > 
> > > > > This is getting silly now.  Can't you use ranges?
> > > > 
> > > > I can if you feel strongly about it? But it does make the drivers
> > > > much more error prone and significantly more annoying to work
> > > > with. I find it is really common to be checking that a register
> > > > is handled correctly through the regmap callbacks and it is nice
> > > > to just be able to grep for that. Obviously this won't work for
> > > > all devices/regmaps as well since many will not have consecutive
> > > > addresses on registers, for example having multi-byte registers
> > > > that are byte addressed.
> > > > 
> > > > How far would you like me to take this as well? Is it just the
> > > > numeric registers you want ranges for ie.
> > > > 
> > > > LOCHNAGAR2_GPIO_CHANNEL1...LOCHNAGAR_GPIO_CHANNEL16
> > > > 
> > > > Or is it all consecutive registers even if they are unrelated
> > > > (exmaple is probably not accurate as I haven't checked the
> > > > addresses):
> > > > 
> > > > LOCHNAGAR2_GPIO_CHANNEL1...LOCHNAGAR2_ANALOGURE_PATH_CTRL1
> > > > 
> > > > I don't mind the first at all but the second is getting really
> > > > horrible in my opinion.
> > 
> > My issue is that we have one end of the scale, where contributors are
> > submitting patches, trying to remove a line of code here, a line of
> > code there, then there are patches like this one which describe the
> > initial value, readable status, writable status and volatile status of
> > each and every register.
> 
> If we could add support for new devices by removing lines of code that
> would be cool :). Eventually Linux would support every piece of hardware
> and be zero lines of code.

I'm starting to think that you've missed the point. ;)

> > The API is obscene and needs a re-work IMHO.
> > 
> > I really hope we do not really have to list every register, but *if we
> > absolutely must*, let's do it once:
> > 
> >    REG_ADDRESS, WRV, INITIAL_VALUE
> 
> To re-iterate, regmap is a standard kernel subsystem. It's not owned by Cirrus,
> so it's not our responsibility if you don't like it. Mark Brown is the maintainer.

Sounds very much like you are saying, "it's not Cirrus' fault,
therefore it is not my problem"?  Which is a terrible attitude.

Remember that the Linux kernel is an open community.  Anyone should be
free to discuss any relevant issue.  If we decide to take this
off-line, which is likely, then so be it.  In the mean time, as a
contributor to this community project, it's absolutely your
responsibly to help discuss and potentially solve wider issues than
just your lines of code.

> Submit your patches to Mark and the owners of those ~1300 drivers to propose
> changes to regmap that you would be happy with.

Quoting myself from above:

  "That may well happen.  This is the pre-patch discussion.

  Apologies that it had to happen on your submission, but this is that
  alerted me to the issue(s)."

> > > > > > +static const struct reg_default lochnagar2_reg_defaults[] = {
> > > > > > +	{ LOCHNAGAR2_CDC_AIF1_CTRL,         0x0000 },
> > > > > > +	{ LOCHNAGAR2_CDC_AIF2_CTRL,         0x0000 },
> > > > > > +	{ LOCHNAGAR2_CDC_AIF3_CTRL,         0x0000 },
> > > > > > +	{ LOCHNAGAR2_DSP_AIF1_CTRL,         0x0000 },
> > > > ...
> > > > > > +	{ LOCHNAGAR2_MINICARD_RESETS,       0x0000 },
> > > > > > +	{ LOCHNAGAR2_ANALOGUE_PATH_CTRL2,   0x0000 },
> > > > > > +	{ LOCHNAGAR2_COMMS_CTRL4,           0x0001 },
> > > > > > +	{ LOCHNAGAR2_SPDIF_CTRL,            0x0008 },
> > > > > > +	{ LOCHNAGAR2_POWER_CTRL,            0x0001 },
> > > > > > +	{ LOCHNAGAR2_SOUNDCARD_AIF_CTRL,    0x0000 },
> > > > > > +};
> > > > > 
> > > > > OMG!  Vile, vile vile!
> > > > 
> > > > I really feel this isn't the driver you are objecting to as such
> > > > but the way regmap operates and also we seem to always have the same
> > > > discussions around regmap every time we push a driver.
> > 
> > Absolutely.  I didn't like it before.  I like it even less now.
> > 
> > > > Is there
> > > > any way me, you and Mark could hash this out and find out a way to
> > > > handle regmaps that is acceptable to you? I don't suppose you are
> > > > in Edinburgh at the moment for ELCE?
> > 
> > I'm not at ELCE I'm afraid.
> > 
> > > I suppose if Mark was willing to promote the regmap drivers to be a
> > > top-level subsystem that could contain the regmap definitions of devices
> > > then we could dump our regmap definitions in there, where Mark can review
> > > it as he's familiar with regmap and the chips and the reasons why things
> > > are done the way they are, rather than Lee having to stress about it every
> > > time we need to create an MFD device that uses regmap. Though that would
> > > make the initialization of an MFD rather awkward with the code required
> > > to init the MFD it not actually being in the MFD tree.
> > 
> > My issue isn't where all this bumph lives.
> > 
> > It's the fact that it's required (at least at this level) at all.
> > 
> 
> As above, if one subsystem owner doesn't like another subsystem then those
> subsystem owners should talk to each other and sort something out. It shouldn't
> block patches that are just trying to use the subsystem as it currently exists
> in the kernel.

Again, no one is blocking this patch.

This driver was submitted for review/discussion.  We are discussing.
Mark Brown Oct. 26, 2018, 3:47 p.m. UTC | #13
On Thu, Oct 25, 2018 at 01:49:05PM +0100, Charles Keepax wrote:

> I don't think you can sensibly get away with not supplying
> default values. You say most sane register layouts have zero
> values, alas again you may not be the biggest fan of our hardware
> guys. The Lochnagar actually does have mostly zero defaults but
> that is very much not generally the case with our hardware.

There's large classes of hardware where it's just not generally the
default over all manufacturers - things like audio CODECs and regulators
that have to represent continuous ranges of numerical values have to
decide between a helpful representation of numbers with a non-zero
default or having a more complicated representation of numbers which
manage to make zero correspond to whatever the default value is.  It's
also quite common for booleans you want to default on.

> One of the main aims of regmap is to avoid bus accesses when
> possible but I guess on the first write/read to any register you
> could insert a read to pull the default, although I do worry
> there is some corner case/flexibility that is going to cause
> headaches later.

It will actually populate the cache from I/O if you don't provide
defaults but it then can't tell what the physical defaults are, we could
read back from hardware but then we have to sync that with the hardware
being reset and if you read the whole thing back that gets super
expensive with slow buses.
Mark Brown Oct. 26, 2018, 3:49 p.m. UTC | #14
On Thu, Oct 25, 2018 at 02:47:59PM +0100, Richard Fitzgerald wrote:

> access. What does regmap debugfs do if you don't have a readables
> list? Just reading a debugfs shouldn't be able to kill the hardware.
> You might need to add a precious list which is more error prone
> than listing the valid readables we are using.

It assumes everything is readable unless it's explicitly told otherwise
or the register map is overall write only (like a 7x9 one).  You could
mark the registers as precious to cause it to only do explicit I/O
operations on them but that's not what you mean and won't be as good a
guarantee that nothing can trigger a read.
Mark Brown Oct. 26, 2018, 8:32 p.m. UTC | #15
On Fri, Oct 26, 2018 at 09:00:51AM +0100, Lee Jones wrote:
> On Thu, 25 Oct 2018, Richard Fitzgerald wrote:

> > Largely the point. How long do you think it would take to populate the
> > cache if you had to read thousands of registers over I2C? Boot time matters.
> > Deferring it until it's touched can create various unpredictable and
> > annoying behaviour later, for example if a lot of cache entries are
> > written while the chip is asleep and the initial values weren't known
> > then a suspend/resume cannot filter out writes that are setting the
> > register to its default (which regmap does to avoid unnecessary bus traffic).
> > So the resume could have a large amount of unnecessary overhead writing
> > registers to a value they already have or reading the initial values of
> > those registers.

> One more register read when initially writing to a register and one
> more when resuming doesn't sound like a vast amount of over-head.

Especially on resume extra register I/O really adds up - people really
care how long their system takes to come back from suspend, and how
quickly individual devices come back.  For devices that are on slow
buses like I2C this means that every register operation counts.  Boot
can be similarly pressured of course, though it's a less frequent issue
for these devices.

> Not sure what you think I was suggesting above.  If the default values
> are actually non-zero that's fine - we'll either leave them as they
> are (if they are never changed, in which case Regmap doesn't even need
> to know about them), document only those (non-zero) ones or wait until
> they are read for the first time, then populate the cache.

You can't assume that the device is in power on reset state unless the
driver reset it itself which may or may not be a good idea or even
possible, sometimes it's what you want but at other times even if it's
possible it can cause user visible disruption during the boot process
which is undesirable.

> Setting up the cache manually also sounds like a vector for potential
> failure.  At least if you were to cache dynamically on first write
> (either on start-up or after sleep) then the actual value will be
> cached, rather than what a piece of C code says it should be.

Even where there's no problem getting the hardware into a clean state it
can rapidly get very, very expensive to do this with larger register
sets on slow buses, and at the framework level we can't assume that
readback support is even present on the device (the earliest versions of
cache support were written to support such devices).  Some of the
userspaces that regmap devices get used with end up wanting to apply a
bunch of configuration at startup, if we can cut down on the amount of
I/O that's involved in doing that it can help them quite a bit.  You
also get userspaces that want to enumerate device state at startup,
that's a bit easier to change in userspace but it's not an unreasonable
thing to want to do and can also get very I/O heavy.

There is some potential for errors to be introduced but equally these
tables can be both generated and verified mechanically, tasks that are
particularly straightforward for the device vendors to do.  There are
also potential risks in doing this at runtime if we didn't get the
device reset, if we don't accurately mark the volatile registers as
volatile or if there's otherwise bugs in the code.

> Precisely my point.  Lochnagar is a small device yet it's required to
> submit hundreds of lines of Regmap tables.  Imagine what that would
> look like for a large device.

There's no *requirement* to provide the data even if you're using the
cache (and the cache support is entirely optional), there's just costs
to not providing it in terms of what features you can get from the
regmap API and the performance of the system.  Not every device is going
to be bothered by those costs, many devices don't provide all of the
data they could.

I'm not clear to me that Lochnagar will particularly benefit from
providing the cache defaults but it sounds like you've raised concerns
about other devices which would, and it seems clear that the readability
information is very useful for this device if there's registers that
it's unsafe to try to read from.

> > > Even if it is absolutely critical that you have to supply these to
> > > Regmap up-front, instead of on first use/read, why can't you just
> > > supply the oddball non-zero ones?

That doesn't work, we need to know both if the register has a default
value and what that value is - there's no great value in only supplying
the defaults for registers with non-zero values.

> > If you aren't happy with the regmap subsystem you could send some
> > patches to change it to what you would be happy with (and patch the ~1300
> > drivers that use it)

> That may well happen.  This is the pre-patch discussion.

> Apologies that it had to happen on your submission, but this is that
> alerted me to the issue(s).

The regmap cache API has been this way since it was introduced in 2011
FWIW, we did add range based support later which is purely data driven.

> > Like any kernel subsystem it has an API that we have to obey to be able to
> > use it.

> Again, this isn't about Lochnagar.

I think from the perspective of Richard and Charles who are just trying
to get their driver merged this is something of an abstract distinction.
If the driver were merged and this discussion were happening separately
their perspective would most likely be different.

> > > The API is obscene and needs a re-work IMHO.

> > > I really hope we do not really have to list every register, but *if we
> > > absolutely must*, let's do it once:

> > >    REG_ADDRESS, WRV, INITIAL_VALUE

There is the option to specify range based access tables instead of a
function, for a lot of devices this is a nice, easy way to specify
things that makes more sense so we support it.  We don't combine the
tables because they're range based and if there is 100% overlap you can
always just point at the same table.

We allow the functions partly because it lets people handle weird cases
but also because it turned out when I looked at this that a lot of the
time the compiler output for switch statements was pretty efficient with
sparse register maps and it makes the code incredibly simple, much
simpler than trying to parse access tables into a more efficient data
structure and IIRC more compact too.  It's possible that those tradeoffs
have changed since but at the time there was a class of devices where it
wasn't clear that putting more effort in would result in substantially
better outcomes.

> > To re-iterate, regmap is a standard kernel subsystem. It's not owned by Cirrus,
> > so it's not our responsibility if you don't like it. Mark Brown is the maintainer.

> Sounds very much like you are saying, "it's not Cirrus' fault,
> therefore it is not my problem"?  Which is a terrible attitude.

I think from the perspective of Charles and Richard this is sounding an
awful lot like you want them (or someone else) to rewrite a very widely
used kernel API before they can get their driver merged.  To me that
would be a completely disproportionate amount of effort for their goal
but unfortunately people do get asked to do things like that so it's not
an unreasonable fear for them to have.

> Remember that the Linux kernel is an open community.  Anyone should be
> free to discuss any relevant issue.  If we decide to take this
> off-line, which is likely, then so be it.  In the mean time, as a
> contributor to this community project, it's absolutely your
> responsibly to help discuss and potentially solve wider issues than
> just your lines of code.

It's also a community of people with differing amounts of ability to
contribute, due to things like time, energy and so on.  Not everyone can
work on everything they want to, let alone other things people ask them
to look at.

> > As above, if one subsystem owner doesn't like another subsystem then those
> > subsystem owners should talk to each other and sort something out. It shouldn't
> > block patches that are just trying to use the subsystem as it currently exists
> > in the kernel.

> Again, no one is blocking this patch.

> This driver was submitted for review/discussion.  We are discussing.

I kind of said this above but just to be clear this driver seems to me
like an idiomatic user of the regmap API as it is today.  My guess is
that we could probably loose the defaults tables and not suffer too much
but equally well they don't hurt from a regmap point of view.

Reviwed-by: Mark Brown <broonie@kernel.org>
Lee Jones Oct. 29, 2018, 11:04 a.m. UTC | #16
On Fri, 26 Oct 2018, Mark Brown wrote:
> On Fri, Oct 26, 2018 at 09:00:51AM +0100, Lee Jones wrote:
> > On Thu, 25 Oct 2018, Richard Fitzgerald wrote:
> 
> > > Largely the point. How long do you think it would take to populate the
> > > cache if you had to read thousands of registers over I2C? Boot time matters.
> > > Deferring it until it's touched can create various unpredictable and
> > > annoying behaviour later, for example if a lot of cache entries are
> > > written while the chip is asleep and the initial values weren't known
> > > then a suspend/resume cannot filter out writes that are setting the
> > > register to its default (which regmap does to avoid unnecessary bus traffic).
> > > So the resume could have a large amount of unnecessary overhead writing
> > > registers to a value they already have or reading the initial values of
> > > those registers.
> 
> > One more register read when initially writing to a register and one
> > more when resuming doesn't sound like a vast amount of over-head.
> 
> Especially on resume extra register I/O really adds up - people really
> care how long their system takes to come back from suspend, and how
> quickly individual devices come back.  For devices that are on slow
> buses like I2C this means that every register operation counts.  Boot
> can be similarly pressured of course, though it's a less frequent issue
> for these devices.
> 
> > Not sure what you think I was suggesting above.  If the default values
> > are actually non-zero that's fine - we'll either leave them as they
> > are (if they are never changed, in which case Regmap doesn't even need
> > to know about them), document only those (non-zero) ones or wait until
> > they are read for the first time, then populate the cache.
> 
> You can't assume that the device is in power on reset state unless the
> driver reset it itself which may or may not be a good idea or even
> possible, sometimes it's what you want but at other times even if it's
> possible it can cause user visible disruption during the boot process
> which is undesirable.
> 
> > Setting up the cache manually also sounds like a vector for potential
> > failure.  At least if you were to cache dynamically on first write
> > (either on start-up or after sleep) then the actual value will be
> > cached, rather than what a piece of C code says it should be.
> 
> Even where there's no problem getting the hardware into a clean state it
> can rapidly get very, very expensive to do this with larger register
> sets on slow buses, and at the framework level we can't assume that
> readback support is even present on the device (the earliest versions of
> cache support were written to support such devices).  Some of the
> userspaces that regmap devices get used with end up wanting to apply a
> bunch of configuration at startup, if we can cut down on the amount of
> I/O that's involved in doing that it can help them quite a bit.  You
> also get userspaces that want to enumerate device state at startup,
> that's a bit easier to change in userspace but it's not an unreasonable
> thing to want to do and can also get very I/O heavy.
> 
> There is some potential for errors to be introduced but equally these
> tables can be both generated and verified mechanically, tasks that are
> particularly straightforward for the device vendors to do.  There are
> also potential risks in doing this at runtime if we didn't get the
> device reset, if we don't accurately mark the volatile registers as
> volatile or if there's otherwise bugs in the code.
> 
> > Precisely my point.  Lochnagar is a small device yet it's required to
> > submit hundreds of lines of Regmap tables.  Imagine what that would
> > look like for a large device.
> 
> There's no *requirement* to provide the data even if you're using the
> cache (and the cache support is entirely optional), there's just costs
> to not providing it in terms of what features you can get from the
> regmap API and the performance of the system.  Not every device is going
> to be bothered by those costs, many devices don't provide all of the
> data they could.

So what do we do in the case where, due to the size of the device, the
amount of lines required by these tables go from crazy to grotesque?

> I'm not clear to me that Lochnagar will particularly benefit from
> providing the cache defaults but it sounds like you've raised concerns
> about other devices which would, and it seems clear that the readability
> information is very useful for this device if there's registers that
> it's unsafe to try to read from.

Any reduction in lines would be a good thing.  Charles, could you
please define what specific benefits you gain from providing providing
the pre-cache data please?  With a particular emphasis on whether the
trade-off is justified.

> > > > Even if it is absolutely critical that you have to supply these to
> > > > Regmap up-front, instead of on first use/read, why can't you just
> > > > supply the oddball non-zero ones?
> 
> That doesn't work, we need to know both if the register has a default
> value and what that value is - there's no great value in only supplying
> the defaults for registers with non-zero values.

All registers have a default value.  Why can't we assume that if a
register is writable and a default value was omitted then the default
is zero?

Ah wait!  As I was typing the above, I just had a thought.  We don't
actually provide a list of writable registers do we?  Only a the
ability to verify if one is writable (presumably) before a write.

That's frustrating!

> > > If you aren't happy with the regmap subsystem you could send some
> > > patches to change it to what you would be happy with (and patch the ~1300
> > > drivers that use it)
> 
> > That may well happen.  This is the pre-patch discussion.
> 
> > Apologies that it had to happen on your submission, but this is that
> > alerted me to the issue(s).
> 
> The regmap cache API has been this way since it was introduced in 2011
> FWIW, we did add range based support later which is purely data driven.

Utilising range support here would certainly help IMHO.

> > > Like any kernel subsystem it has an API that we have to obey to be able to
> > > use it.
> 
> > Again, this isn't about Lochnagar.
> 
> I think from the perspective of Richard and Charles who are just trying
> to get their driver merged this is something of an abstract distinction.
> If the driver were merged and this discussion were happening separately
> their perspective would most likely be different.

Charles has already mentioned that he'd take a look at the current
*use* (not changing the API, but the way in which Lochnagar
*uses/consumes* it).  Actions which would be most welcomed.

> > > > The API is obscene and needs a re-work IMHO.
> 
> > > > I really hope we do not really have to list every register, but *if we
> > > > absolutely must*, let's do it once:
> 
> > > >    REG_ADDRESS, WRV, INITIAL_VALUE
> 
> There is the option to specify range based access tables instead of a
> function, for a lot of devices this is a nice, easy way to specify
> things that makes more sense so we support it.  We don't combine the
> tables because they're range based and if there is 100% overlap you can
> always just point at the same table.
> 
> We allow the functions partly because it lets people handle weird cases
> but also because it turned out when I looked at this that a lot of the
> time the compiler output for switch statements was pretty efficient with
> sparse register maps and it makes the code incredibly simple, much
> simpler than trying to parse access tables into a more efficient data
> structure and IIRC more compact too.  It's possible that those tradeoffs
> have changed since but at the time there was a class of devices where it
> wasn't clear that putting more effort in would result in substantially
> better outcomes.
> 
> > > To re-iterate, regmap is a standard kernel subsystem. It's not owned by Cirrus,
> > > so it's not our responsibility if you don't like it. Mark Brown is the maintainer.
> 
> > Sounds very much like you are saying, "it's not Cirrus' fault,
> > therefore it is not my problem"?  Which is a terrible attitude.
> 
> I think from the perspective of Charles and Richard this is sounding an
> awful lot like you want them (or someone else) to rewrite a very widely
> used kernel API before they can get their driver merged.  To me that
> would be a completely disproportionate amount of effort for their goal
> but unfortunately people do get asked to do things like that so it's not
> an unreasonable fear for them to have.

I would see that as an unreasonable request.

To be clear, that is *not* what I am asking.

> > Remember that the Linux kernel is an open community.  Anyone should be
> > free to discuss any relevant issue.  If we decide to take this
> > off-line, which is likely, then so be it.  In the mean time, as a
> > contributor to this community project, it's absolutely your
> > responsibly to help discuss and potentially solve wider issues than
> > just your lines of code.
> 
> It's also a community of people with differing amounts of ability to
> contribute, due to things like time, energy and so on.  Not everyone can
> work on everything they want to, let alone other things people ask them
> to look at.

I'm not asking for code submission.  Merely contributing to this
discussion, or simply allowing it to happen on the back of one of
their submission is enough.

Denouncing all responsibility and a lack of care is not okay.

> > > As above, if one subsystem owner doesn't like another subsystem then those
> > > subsystem owners should talk to each other and sort something out. It shouldn't
> > > block patches that are just trying to use the subsystem as it currently exists
> > > in the kernel.
> 
> > Again, no one is blocking this patch.
> 
> > This driver was submitted for review/discussion.  We are discussing.
> 
> I kind of said this above but just to be clear this driver seems to me
> like an idiomatic user of the regmap API as it is today.  My guess is
> that we could probably loose the defaults tables and not suffer too much
> but equally well they don't hurt from a regmap point of view.

Perhaps Charles could elaborate on whether this is possible or not?

> Reviwed-by: Mark Brown <broonie@kernel.org>

Thanks.
Richard Fitzgerald Oct. 29, 2018, 11:52 a.m. UTC | #17
On 29/10/18 11:04, Lee Jones wrote:
> On Fri, 26 Oct 2018, Mark Brown wrote:
>> On Fri, Oct 26, 2018 at 09:00:51AM +0100, Lee Jones wrote:
>>> On Thu, 25 Oct 2018, Richard Fitzgerald wrote:
>>
>>>> Largely the point. How long do you think it would take to populate the
>>>> cache if you had to read thousands of registers over I2C? Boot time matters.
>>>> Deferring it until it's touched can create various unpredictable and
>>>> annoying behaviour later, for example if a lot of cache entries are
>>>> written while the chip is asleep and the initial values weren't known
>>>> then a suspend/resume cannot filter out writes that are setting the
>>>> register to its default (which regmap does to avoid unnecessary bus traffic).
>>>> So the resume could have a large amount of unnecessary overhead writing
>>>> registers to a value they already have or reading the initial values of
>>>> those registers.
>>
>>> One more register read when initially writing to a register and one
>>> more when resuming doesn't sound like a vast amount of over-head.
>>
>> Especially on resume extra register I/O really adds up - people really
>> care how long their system takes to come back from suspend, and how
>> quickly individual devices come back.  For devices that are on slow
>> buses like I2C this means that every register operation counts.  Boot
>> can be similarly pressured of course, though it's a less frequent issue
>> for these devices.
>>
>>> Not sure what you think I was suggesting above.  If the default values
>>> are actually non-zero that's fine - we'll either leave them as they
>>> are (if they are never changed, in which case Regmap doesn't even need
>>> to know about them), document only those (non-zero) ones or wait until
>>> they are read for the first time, then populate the cache.
>>
>> You can't assume that the device is in power on reset state unless the
>> driver reset it itself which may or may not be a good idea or even
>> possible, sometimes it's what you want but at other times even if it's
>> possible it can cause user visible disruption during the boot process
>> which is undesirable.
>>
>>> Setting up the cache manually also sounds like a vector for potential
>>> failure.  At least if you were to cache dynamically on first write
>>> (either on start-up or after sleep) then the actual value will be
>>> cached, rather than what a piece of C code says it should be.
>>
>> Even where there's no problem getting the hardware into a clean state it
>> can rapidly get very, very expensive to do this with larger register
>> sets on slow buses, and at the framework level we can't assume that
>> readback support is even present on the device (the earliest versions of
>> cache support were written to support such devices).  Some of the
>> userspaces that regmap devices get used with end up wanting to apply a
>> bunch of configuration at startup, if we can cut down on the amount of
>> I/O that's involved in doing that it can help them quite a bit.  You
>> also get userspaces that want to enumerate device state at startup,
>> that's a bit easier to change in userspace but it's not an unreasonable
>> thing to want to do and can also get very I/O heavy.
>>
>> There is some potential for errors to be introduced but equally these
>> tables can be both generated and verified mechanically, tasks that are
>> particularly straightforward for the device vendors to do.  There are
>> also potential risks in doing this at runtime if we didn't get the
>> device reset, if we don't accurately mark the volatile registers as
>> volatile or if there's otherwise bugs in the code.
>>
>>> Precisely my point.  Lochnagar is a small device yet it's required to
>>> submit hundreds of lines of Regmap tables.  Imagine what that would
>>> look like for a large device.
>>
>> There's no *requirement* to provide the data even if you're using the
>> cache (and the cache support is entirely optional), there's just costs
>> to not providing it in terms of what features you can get from the
>> regmap API and the performance of the system.  Not every device is going
>> to be bothered by those costs, many devices don't provide all of the
>> data they could.
> 
> So what do we do in the case where, due to the size of the device, the
> amount of lines required by these tables go from crazy to grotesque?
> 
>> I'm not clear to me that Lochnagar will particularly benefit from
>> providing the cache defaults but it sounds like you've raised concerns
>> about other devices which would, and it seems clear that the readability
>> information is very useful for this device if there's registers that
>> it's unsafe to try to read from.
> 
> Any reduction in lines would be a good thing.  Charles, could you
> please define what specific benefits you gain from providing providing
> the pre-cache data please?  With a particular emphasis on whether the
> trade-off is justified.
> 

Why so much concern over the number of source lines of a data table?
If we were talking about removing lines of executable code to make it
more efficient - yes, that's a good thing.
Worrying about the number of lines of a data table, I don't see the
imperative for that.

Since this seems to be a significant part of your objection it would help
if you could tell us WHY you object so much to lines of tables.

>>>>> Even if it is absolutely critical that you have to supply these to
>>>>> Regmap up-front, instead of on first use/read, why can't you just
>>>>> supply the oddball non-zero ones?
>>
>> That doesn't work, we need to know both if the register has a default
>> value and what that value is - there's no great value in only supplying
>> the defaults for registers with non-zero values.
> 
> All registers have a default value.  Why can't we assume that if a
> register is writable and a default value was omitted then the default
> is zero?
> 
> Ah wait!  As I was typing the above, I just had a thought.  We don't
> actually provide a list of writable registers do we?  Only a the
> ability to verify if one is writable (presumably) before a write.
> 
> That's frustrating!
> 
>>>> If you aren't happy with the regmap subsystem you could send some
>>>> patches to change it to what you would be happy with (and patch the ~1300
>>>> drivers that use it)
>>
>>> That may well happen.  This is the pre-patch discussion.
>>
>>> Apologies that it had to happen on your submission, but this is that
>>> alerted me to the issue(s).
>>
>> The regmap cache API has been this way since it was introduced in 2011
>> FWIW, we did add range based support later which is purely data driven.
> 
> Utilising range support here would certainly help IMHO.
> 
>>>> Like any kernel subsystem it has an API that we have to obey to be able to
>>>> use it.
>>
>>> Again, this isn't about Lochnagar.
>>
>> I think from the perspective of Richard and Charles who are just trying
>> to get their driver merged this is something of an abstract distinction.
>> If the driver were merged and this discussion were happening separately
>> their perspective would most likely be different.
> 
> Charles has already mentioned that he'd take a look at the current
> *use* (not changing the API, but the way in which Lochnagar
> *uses/consumes* it).  Actions which would be most welcomed.
>

Maybe Charles will find an alternative that doesn't affect performance/functionality
or is an acceptable tradeoff. But still, it seems an odd maintainer requirement
"please use this other subsystem less effectively to make _my_ subsystem have fewer
lines of source".

>>>>> The API is obscene and needs a re-work IMHO.
>>
>>>>> I really hope we do not really have to list every register, but *if we
>>>>> absolutely must*, let's do it once:
>>
>>>>>     REG_ADDRESS, WRV, INITIAL_VALUE
>>
>> There is the option to specify range based access tables instead of a
>> function, for a lot of devices this is a nice, easy way to specify
>> things that makes more sense so we support it.  We don't combine the
>> tables because they're range based and if there is 100% overlap you can
>> always just point at the same table.
>>
>> We allow the functions partly because it lets people handle weird cases
>> but also because it turned out when I looked at this that a lot of the
>> time the compiler output for switch statements was pretty efficient with
>> sparse register maps and it makes the code incredibly simple, much
>> simpler than trying to parse access tables into a more efficient data
>> structure and IIRC more compact too.  It's possible that those tradeoffs
>> have changed since but at the time there was a class of devices where it
>> wasn't clear that putting more effort in would result in substantially
>> better outcomes.
>>
>>>> To re-iterate, regmap is a standard kernel subsystem. It's not owned by Cirrus,
>>>> so it's not our responsibility if you don't like it. Mark Brown is the maintainer.
>>
>>> Sounds very much like you are saying, "it's not Cirrus' fault,
>>> therefore it is not my problem"?  Which is a terrible attitude.
>>
>> I think from the perspective of Charles and Richard this is sounding an
>> awful lot like you want them (or someone else) to rewrite a very widely
>> used kernel API before they can get their driver merged.  To me that
>> would be a completely disproportionate amount of effort for their goal
>> but unfortunately people do get asked to do things like that so it's not
>> an unreasonable fear for them to have.
> 
> I would see that as an unreasonable request.
> 
> To be clear, that is *not* what I am asking.
>

It sounded that way, so I apologize if that wasn't what you meant. It just
read as if you thought we were the owners (or only users) of regmap so we
can just go and change it as per your suggestion and then resend this patch.

>>> Remember that the Linux kernel is an open community.  Anyone should be
>>> free to discuss any relevant issue.  If we decide to take this
>>> off-line, which is likely, then so be it.  In the mean time, as a
>>> contributor to this community project, it's absolutely your
>>> responsibly to help discuss and potentially solve wider issues than
>>> just your lines of code.
>>
>> It's also a community of people with differing amounts of ability to
>> contribute, due to things like time, energy and so on.  Not everyone can
>> work on everything they want to, let alone other things people ask them
>> to look at.
> 
> I'm not asking for code submission.  Merely contributing to this
> discussion, or simply allowing it to happen on the back of one of
> their submission is enough.
> 
> Denouncing all responsibility and a lack of care is not okay.
>
>>>> As above, if one subsystem owner doesn't like another subsystem then those
>>>> subsystem owners should talk to each other and sort something out. It shouldn't
>>>> block patches that are just trying to use the subsystem as it currently exists
>>>> in the kernel.
>>
>>> Again, no one is blocking this patch.
>>
>>> This driver was submitted for review/discussion.  We are discussing.
>>
>> I kind of said this above but just to be clear this driver seems to me
>> like an idiomatic user of the regmap API as it is today.  My guess is
>> that we could probably loose the defaults tables and not suffer too much
>> but equally well they don't hurt from a regmap point of view.
> 
> Perhaps Charles could elaborate on whether this is possible or not?
> 
>> Reviwed-by: Mark Brown <broonie@kernel.org>
> 
> Thanks.
>
Richard Fitzgerald Oct. 29, 2018, 12:36 p.m. UTC | #18
On 29/10/18 11:04, Lee Jones wrote:
> On Fri, 26 Oct 2018, Mark Brown wrote:
>> On Fri, Oct 26, 2018 at 09:00:51AM +0100, Lee Jones wrote:
>>> On Thu, 25 Oct 2018, Richard Fitzgerald wrote:
>>
>>>> Largely the point. How long do you think it would take to populate the
>>>> cache if you had to read thousands of registers over I2C? Boot time matters.
>>>> Deferring it until it's touched can create various unpredictable and
>>>> annoying behaviour later, for example if a lot of cache entries are
>>>> written while the chip is asleep and the initial values weren't known
>>>> then a suspend/resume cannot filter out writes that are setting the
>>>> register to its default (which regmap does to avoid unnecessary bus traffic).
>>>> So the resume could have a large amount of unnecessary overhead writing
>>>> registers to a value they already have or reading the initial values of
>>>> those registers.
>>
>>> One more register read when initially writing to a register and one
>>> more when resuming doesn't sound like a vast amount of over-head.
>>
>> Especially on resume extra register I/O really adds up - people really
>> care how long their system takes to come back from suspend, and how
>> quickly individual devices come back.  For devices that are on slow
>> buses like I2C this means that every register operation counts.  Boot
>> can be similarly pressured of course, though it's a less frequent issue
>> for these devices.
>>
>>> Not sure what you think I was suggesting above.  If the default values
>>> are actually non-zero that's fine - we'll either leave them as they
>>> are (if they are never changed, in which case Regmap doesn't even need
>>> to know about them), document only those (non-zero) ones or wait until
>>> they are read for the first time, then populate the cache.
>>
>> You can't assume that the device is in power on reset state unless the
>> driver reset it itself which may or may not be a good idea or even
>> possible, sometimes it's what you want but at other times even if it's
>> possible it can cause user visible disruption during the boot process
>> which is undesirable.
>>
>>> Setting up the cache manually also sounds like a vector for potential
>>> failure.  At least if you were to cache dynamically on first write
>>> (either on start-up or after sleep) then the actual value will be
>>> cached, rather than what a piece of C code says it should be.
>>
>> Even where there's no problem getting the hardware into a clean state it
>> can rapidly get very, very expensive to do this with larger register
>> sets on slow buses, and at the framework level we can't assume that
>> readback support is even present on the device (the earliest versions of
>> cache support were written to support such devices).  Some of the
>> userspaces that regmap devices get used with end up wanting to apply a
>> bunch of configuration at startup, if we can cut down on the amount of
>> I/O that's involved in doing that it can help them quite a bit.  You
>> also get userspaces that want to enumerate device state at startup,
>> that's a bit easier to change in userspace but it's not an unreasonable
>> thing to want to do and can also get very I/O heavy.
>>
>> There is some potential for errors to be introduced but equally these
>> tables can be both generated and verified mechanically, tasks that are
>> particularly straightforward for the device vendors to do.  There are
>> also potential risks in doing this at runtime if we didn't get the
>> device reset, if we don't accurately mark the volatile registers as
>> volatile or if there's otherwise bugs in the code.
>>
>>> Precisely my point.  Lochnagar is a small device yet it's required to
>>> submit hundreds of lines of Regmap tables.  Imagine what that would
>>> look like for a large device.
>>
>> There's no *requirement* to provide the data even if you're using the
>> cache (and the cache support is entirely optional), there's just costs
>> to not providing it in terms of what features you can get from the
>> regmap API and the performance of the system.  Not every device is going
>> to be bothered by those costs, many devices don't provide all of the
>> data they could.
> 
> So what do we do in the case where, due to the size of the device, the
> amount of lines required by these tables go from crazy to grotesque?
> 
>> I'm not clear to me that Lochnagar will particularly benefit from
>> providing the cache defaults but it sounds like you've raised concerns
>> about other devices which would, and it seems clear that the readability
>> information is very useful for this device if there's registers that
>> it's unsafe to try to read from.
> 
> Any reduction in lines would be a good thing.  Charles, could you
> please define what specific benefits you gain from providing providing
> the pre-cache data please?  With a particular emphasis on whether the
> trade-off is justified.
> 
>>>>> Even if it is absolutely critical that you have to supply these to
>>>>> Regmap up-front, instead of on first use/read, why can't you just
>>>>> supply the oddball non-zero ones?
>>
>> That doesn't work, we need to know both if the register has a default
>> value and what that value is - there's no great value in only supplying
>> the defaults for registers with non-zero values.
> 
> All registers have a default value.  Why can't we assume that if a
> register is writable and a default value was omitted then the default
> is zero?
> 
> Ah wait!  As I was typing the above, I just had a thought.  We don't
> actually provide a list of writable registers do we?  Only a the
> ability to verify if one is writable (presumably) before a write.
> 
> That's frustrating!
> 
>>>> If you aren't happy with the regmap subsystem you could send some
>>>> patches to change it to what you would be happy with (and patch the ~1300
>>>> drivers that use it)
>>
>>> That may well happen.  This is the pre-patch discussion.
>>
>>> Apologies that it had to happen on your submission, but this is that
>>> alerted me to the issue(s).
>>
>> The regmap cache API has been this way since it was introduced in 2011
>> FWIW, we did add range based support later which is purely data driven.
> 
> Utilising range support here would certainly help IMHO.
> 
>>>> Like any kernel subsystem it has an API that we have to obey to be able to
>>>> use it.
>>
>>> Again, this isn't about Lochnagar.
>>
>> I think from the perspective of Richard and Charles who are just trying
>> to get their driver merged this is something of an abstract distinction.
>> If the driver were merged and this discussion were happening separately
>> their perspective would most likely be different.
> 
> Charles has already mentioned that he'd take a look at the current
> *use* (not changing the API, but the way in which Lochnagar
> *uses/consumes* it).  Actions which would be most welcomed.
> 
>>>>> The API is obscene and needs a re-work IMHO.
>>
>>>>> I really hope we do not really have to list every register, but *if we
>>>>> absolutely must*, let's do it once:
>>
>>>>>     REG_ADDRESS, WRV, INITIAL_VALUE
>>
>> There is the option to specify range based access tables instead of a
>> function, for a lot of devices this is a nice, easy way to specify
>> things that makes more sense so we support it.  We don't combine the
>> tables because they're range based and if there is 100% overlap you can
>> always just point at the same table.
>>
>> We allow the functions partly because it lets people handle weird cases
>> but also because it turned out when I looked at this that a lot of the
>> time the compiler output for switch statements was pretty efficient with
>> sparse register maps and it makes the code incredibly simple, much
>> simpler than trying to parse access tables into a more efficient data
>> structure and IIRC more compact too.  It's possible that those tradeoffs
>> have changed since but at the time there was a class of devices where it
>> wasn't clear that putting more effort in would result in substantially
>> better outcomes.
>>
>>>> To re-iterate, regmap is a standard kernel subsystem. It's not owned by Cirrus,
>>>> so it's not our responsibility if you don't like it. Mark Brown is the maintainer.
>>
>>> Sounds very much like you are saying, "it's not Cirrus' fault,
>>> therefore it is not my problem"?  Which is a terrible attitude.
>>
>> I think from the perspective of Charles and Richard this is sounding an
>> awful lot like you want them (or someone else) to rewrite a very widely
>> used kernel API before they can get their driver merged.  To me that
>> would be a completely disproportionate amount of effort for their goal
>> but unfortunately people do get asked to do things like that so it's not
>> an unreasonable fear for them to have.
> 
> I would see that as an unreasonable request.
> 
> To be clear, that is *not* what I am asking.
> 
>>> Remember that the Linux kernel is an open community.  Anyone should be
>>> free to discuss any relevant issue.  If we decide to take this
>>> off-line, which is likely, then so be it.  In the mean time, as a
>>> contributor to this community project, it's absolutely your
>>> responsibly to help discuss and potentially solve wider issues than
>>> just your lines of code.
>>
>> It's also a community of people with differing amounts of ability to
>> contribute, due to things like time, energy and so on.  Not everyone can
>> work on everything they want to, let alone other things people ask them
>> to look at.
> 
> I'm not asking for code submission.  Merely contributing to this
> discussion, or simply allowing it to happen on the back of one of
> their submission is enough.
> 
> Denouncing all responsibility and a lack of care is not okay.
> 
>>>> As above, if one subsystem owner doesn't like another subsystem then those
>>>> subsystem owners should talk to each other and sort something out. It shouldn't
>>>> block patches that are just trying to use the subsystem as it currently exists
>>>> in the kernel.
>>
>>> Again, no one is blocking this patch.
>>
>>> This driver was submitted for review/discussion.  We are discussing.
>>
>> I kind of said this above but just to be clear this driver seems to me
>> like an idiomatic user of the regmap API as it is today.  My guess is
>> that we could probably loose the defaults tables and not suffer too much
>> but equally well they don't hurt from a regmap point of view.
> 
> Perhaps Charles could elaborate on whether this is possible or not?
> 
>> Reviwed-by: Mark Brown <broonie@kernel.org>
> 
> Thanks.
> 

If we're going to need to change regmap to get drivers that use regmap accepted into
MFD (at least without crippling them), can Lee or Mark please create a separate discussion
thread for that, and include the major contributors/users of regmap so Lee can air his
objections and proposals to a more appropriate group of people and we can get feedback
from the right people, and hopefully come to some sort of decision.
Mark Brown Oct. 29, 2018, 12:57 p.m. UTC | #19
On Mon, Oct 29, 2018 at 11:04:39AM +0000, Lee Jones wrote:
> On Fri, 26 Oct 2018, Mark Brown wrote:

> > There's no *requirement* to provide the data even if you're using the
> > cache (and the cache support is entirely optional), there's just costs
> > to not providing it in terms of what features you can get from the
> > regmap API and the performance of the system.  Not every device is going
> > to be bothered by those costs, many devices don't provide all of the
> > data they could.

> So what do we do in the case where, due to the size of the device, the
> amount of lines required by these tables go from crazy to grotesque?

My recommendation when the tables get in the way of the rest of the
driver is to move them into a separate file that contains only tables,
these get big but they're sitting in a separate file that only contains
big data tables so they're fairly simple to look at (and generate or
whatever) and people working on the active code don't need to look at
them.

> > That doesn't work, we need to know both if the register has a default
> > value and what that value is - there's no great value in only supplying
> > the defaults for registers with non-zero values.

> All registers have a default value.  Why can't we assume that if a
> register is writable and a default value was omitted then the default
> is zero?

There are volatile registers which can't be part of the cache as the
hardware might change the state, and we have things like device
identification registers which are fixed but which we want to read from
the device.  This second set of registers can usually be modelled quite
happily as volatile registers though, we don't tend to read them often.

There's also registers where the user just didn't tell us what's going
on, either through oversight or because there's some code that touches
undocumented test registers at runtime for quirk reasons using a read,
modify write cycle.  We could try to insist that the device drivers
always provide a default or mark things as volatile but that's likely to
be an uphill struggle and more error prone.

> Ah wait!  As I was typing the above, I just had a thought.  We don't
> actually provide a list of writable registers do we?  Only a the
> ability to verify if one is writable (presumably) before a write.

> That's frustrating!

You can provide the writability information either with the function or
with data.  The tables code doesn't currently scale very well to large,
sparse register maps but that could in theory be optimized.  Few devices
bother providing distinct writability information as it's not often an
issue.
Charles Keepax Nov. 1, 2018, 10:28 a.m. UTC | #20
On Mon, Oct 29, 2018 at 11:04:39AM +0000, Lee Jones wrote:
> On Fri, 26 Oct 2018, Mark Brown wrote:
> > On Fri, Oct 26, 2018 at 09:00:51AM +0100, Lee Jones wrote:
> > > On Thu, 25 Oct 2018, Richard Fitzgerald wrote:

I have re-ordered some of the quotes here for the benefit
of clarity. I will start with the Lochnagar focused bits
and then cover some of the more general regmap discussion.
Apologies for the wall of text towards the end but it seemed
wise to explore some of the why for parts of the current regmap
implementation and some of the implications for changing it.

> > I think from the perspective of Richard and Charles who are just trying
> > to get their driver merged this is something of an abstract distinction.
> > If the driver were merged and this discussion were happening separately
> > their perspective would most likely be different.
>
> Charles has already mentioned that he'd take a look at the current
> *use* (not changing the API, but the way in which Lochnagar
> *uses/consumes* it).  Actions which would be most welcomed.
>
> > I kind of said this above but just to be clear this driver seems to me
> > like an idiomatic user of the regmap API as it is today.  My guess is
> > that we could probably loose the defaults tables and not suffer too much
> > but equally well they don't hurt from a regmap point of view.
>
> Perhaps Charles could elaborate on whether this is possible or not?

So on this front I have had a look through and we can drop the
defaults tables for Lochnagar, although as I shall cover later
Lochnagar is the exceptional case with respect to our normal
devices.

> Utilising range support here would certainly help IMHO.
>

I am rather hesitant to switch to the range API. Whilst it is
significantly more clear in certain situations, such as say
mapping out the memory for a DSP, where there exist large
amorphis blobs of identical function registers. I am really
not convinced it really suits something like the register map
that controls Lochnagar. You have an intertwinned mix of
various purpose registers, each with a clearly defined name
and potentially with quite fine grained properties.

Certainly when working with the driver I want to be able to
fairly quickly see what registers are marked as by name. The
callbacks are very simple and I don't need to look up where
register are in the regmap to be able check their attributes.
But perhaps I have just got too used to seeing these callbacks,
things do have a way of becoming normal after being exposed to
them for a while.

What I will try for the next spin is to try to use as much:

  case REG1...REG2:

As I can without totally sacrificing grepability/clarity and
hopefully that will get us to a compromise we can all live
with at least for now. Lochnagar is a fairly small part so it
feels like this should be doable.

> > > > +       ret = devm_of_platform_populate(dev);
> > > > +       if (ret < 0) {
> > > > +               dev_err(dev, "Failed to populate child
> > > > nodes:
> > > > %d\n", ret);
> > > > +               return ret;
> > > > +       }
> > >
> > > Please do not mix OF and MFD device registration
> > > strategies.
> > >
> > > Pick one and register all devices through your chosen
> > > method.
> >
> > Hmmm we use this to do things like register some fixed
> > regulators
> > and clocks that don't need any control but do need to be
> > associated 
> > with the device. I could do that through the MFD although it
> > is in 
> > direct conflict with the feedback on the clock patches I
> > received
> > to move the fixed clocks into devicetree rather than
> > registering
> > them manually (see v2 of the patch chain).
>
> The I suggest moving everything to DT.

So pulling this out from earlier discussions in this thread,
it seems I can happily move all the child device registration
into device tree. I will also try this for the next version of
the patch, unless anyone wants to object? But it does change
the DT binding quite a lot as the individual sub drivers now
each require their own node rather than one single unified
Lochnagar node.

> > > Precisely my point.  Lochnagar is a small device yet it's required to
> > > submit hundreds of lines of Regmap tables.  Imagine what that would
> > > look like for a large device.
> >
> > There's no *requirement* to provide the data even if you're using the
> > cache (and the cache support is entirely optional), there's just costs
> > to not providing it in terms of what features you can get from the
> > regmap API and the performance of the system.  Not every device is going
> > to be bothered by those costs, many devices don't provide all of the
> > data they could.
>
> So what do we do in the case where, due to the size of the device, the
> amount of lines required by these tables go from crazy to grotesque?
>

Ultimately, I guess I have always just viewed it as just data
tables. Its a lot of lines of source but its not complicated,
and complexity has really always been the thing I try to avoid.

> > > > > Even if it is absolutely critical that you have to supply these to
> > > > > Regmap up-front, instead of on first use/read, why can't you just
> > > > > supply the oddball non-zero ones?
> > 
> > That doesn't work, we need to know both if the register has a default
> > value and what that value is - there's no great value in only supplying
> > the defaults for registers with non-zero values.
> 
> All registers have a default value.  Why can't we assume that if a
> register is writable and a default value was omitted then the default
> is zero?

Defaults basically serve two purposes in regmap:

 1) Initialise the register cache.

    There are basically three ways you could handle this
    (at least that I can think of) and regmap supports all
    three. Obviously each with their own pros and cons:

     1.1) Table of default values
          + Fast boot time
          - Uses some additional memory
     1.2) Read all the registers from the device on boot
          + Uses less memory
          - Potentially very slow boot time
     1.3) Only read values as you touch registers
          + Uses less memory
          + Usually no significant impact on boot time
          - Can't do read or read/write/modify operations on
            previously untouched registers whilst chip is off

    1.3 does probably make sense here for Lochnagar since we
    don't currently power things down. However, it is worth
    noting that such an approach is extremely challenging for many
    devices. For example the CODECs generally have all sorts of
    actual user-facing interface that needs to be accessible
    regardless of if the CODEC is powered up or down and powering it
    up to access registers would end up being either horrific on
    power consumption and/or horrific in terms of code complexity.

    1.1 and 1.2 are basically a straight trade off. Generally
    for our devices we talking loads of registers and
    potentially connected over I2C. Our customers care deeply
    about device boot time, Android has specs for such things
    and often it is tight for a system to make those specs.
    Conversly, generally the products we integrate into have
    a fairly large amount of memory. As such this is a no
    brainer of a choice for most of our devices.

 2) Determine what registers should be synchronised on a cache
    sync.

    A cache sync is usually done when pulling a device out of
    low power mode to reapply the currently desired register
    settings. As discussed in 1) we don't currently do cache
    syncs on Lochnagar, but this would affect most of our
    parts. Again I can see three approaches to synchronising
    the cache:

     2.1) Sync out registers that arn't at their default value
          + Only syncs register that are actually needed
          - Requires a table of defaults to work
     2.2) Store a per register dirty flag in the cache
          + No up front table required in the driver
          + Potentially less memory as only a single bit required
            per register, although realising that saving might be
            very hard on some cache types
          - May sync registers that arn't required
     2.3) Sync all registers from the cache
          + No memory usage
          - Potentially large penalty for cache sync

    Regmap has options to do 2.3, however for most of our
    devices that would be totally unacceptable. Our parts have
    a lot of registers most of which are cacheable and for
    power reasons cache syncs happen as the audio path is being
    brought up. Again Android has targets here and they are
    in low 10's of millseconds so bus accesses really do matter.

    2.1 is the normal proceedure in regmap, although this
    is defined on a per cache implementation basis, with the
    core providing 2.1 as a default.

    Again it's a bit of a trade off between 2.1 and 2.2, but
    given 1 pretty much requires us to know the defaults
    anyway, 2.1 will in general make the most sense, at least
    for Cirrus parts.

So I would conclude, certainly for most Cirrus devices, we
do need to know the defaults at least for the vast majority
of registers. I guess the next question would be could we
generate some of the defaults table to reduce the table size
in the driver. I would agree with you that the only sensible
approach to reducing the defaults table size I can see would be
to not have to specify defaults for registers with a default
of zero. As an example to set expectations cs47l90, probably
the largest CODEC we have made, has 1357 defaults of which
693 are zero so ~50%.

The issue really boils down to how do we tell the difference
between a register that has no default, and one that has a default
of zero? There are a few problems I can foresee on this front:

 1) There are occasions where people use a readable,
    non-volatile register with no default for things like
    device ID or revision. The idea being it can be read once
    which will populate it into the cache then it never needs
    to be read from the hardware again. Especially useful if
    this information might be required whilst the hardware
    is powered down.

    We could potentially update such drivers to mark the
    register as volatile and then store the value explicitly
    in the drivers data structures?

 2) There are occasions where a readable, writeable,
    non-volatile register cannot be given a fixed default.
    Usually this will be registers that are configured by
    firmware or hardware during boot but then have control
    passed to the kernel.

    For example a couple of registers on Lochnagar will be
    configured by the board controller itself depending on
    the CODEC that is attached, mostly regulators that are
    enabled during boot being set to appropriate voltages to
    avoid hardware damage. To handle these we don't give them
    defaults which forces a read from the device but then after
    that we can use the cache.

    For this one would probably have to add a new register
    property (in addition to the existing readable, writeable,
    volatile, and precious) for no default. Which would require
    an additional callback. Although I guess that would cover 1
    as well, and normally there would be very few registers in
    this catagory.

 3) Rather nervous there are other issues I am not currently
    thinking of this is a widely used API and I am mostly
    familiar only with the style of devices I work with.

    We could potentially add an assume zero defaults flag,
    that would at least then only apply the change to devices
    that opt in?

One other related thing that rests on my mind is that creating
the defaults table is going to be a little intensive. I guess
it is not so bad if using the ranges API, so perhaps we would
need to tie it in with that. Although it is still a little
fiddly as you need to work out overlaps between the ranges for
different properties to be more efficient than just checking
each register.  For the callback based systems though you
would have to check each register and for example coming
back to cs47l90, the highest register address is 0x3FFE7C
which means you would need to call over 4 million callbacks
probably multiple times whilst constructing your defaults table.

> Ah wait!  As I was typing the above, I just had a thought.  We don't
> actually provide a list of writable registers do we?  Only a the
> ability to verify if one is writable (presumably) before a write.

You can mark registers as writable. Its just that most drivers
don't bother to define the writable registers, there isn't
presently a great reason to do so and in that case the core
defaults to all register addresses being writable. I guess if
we added the automatic zero defaults you might well want to
start adding this callback though.

> > > > > The API is obscene and needs a re-work IMHO.
> > 
> > > > > I really hope we do not really have to list every register, but *if we
> > > > > absolutely must*, let's do it once:
> > 
> > > > >    REG_ADDRESS, WRV, INITIAL_VALUE
> > 
> > There is the option to specify range based access tables instead of a
> > function, for a lot of devices this is a nice, easy way to specify
> > things that makes more sense so we support it.  We don't combine the
> > tables because they're range based and if there is 100% overlap you can
> > always just point at the same table.
> > 
> > We allow the functions partly because it lets people handle weird cases
> > but also because it turned out when I looked at this that a lot of the
> > time the compiler output for switch statements was pretty efficient with
> > sparse register maps and it makes the code incredibly simple, much
> > simpler than trying to parse access tables into a more efficient data
> > structure and IIRC more compact too.  It's possible that those tradeoffs
> > have changed since but at the time there was a class of devices where it
> > wasn't clear that putting more effort in would result in substantially
> > better outcomes.
> > 

Exactly this, the regmap core makes a lot of readable/volatile
checks and the callbacks with switch statements are a very
runtime efficient way to implement those, in addition to the
code clarity.

I guess historically we have tried to be verbose as it
really made things very simple for us. We have scripts that
can generate the defaults and callbacks from outputs of the
hardware design giving us a very high degree of confidence in
them. Range based approaches always result in a bit of manual
work, unless we just literally pulled sequential ranges but that
results in code that is really not friendly when developing
the driver. But maybe that is the point we need to budge on
from the Cirrus side.

I think this really the crux of my concerns here about updating
the regmap API are that it feels like almost any path we take
from here results in worse runtime performance and probably
equal or worse memory usage. In return we gain a reduction of
some, whilst admittedly large, very simple data tables from
the driver. Which is a trade it feels hard to get invested in.

Anyway, well done to anyone who made it this far. I will
keep pondering on the issue, hopefully we can fudge things
for Lochnagar. But maybe we can come up with some longer term
compromise that devolves more of the regmap stuff through the
driver, hopefully reducing both the central data tables and the
large register include files both of which seem to be constant
pain points with review of parts. Although I am not sure how
much we can do for Arizona/Madera at this point but the parts
after that change the regmap quite dramatically so will need
a new driver so perhaps that is the point to look at things.

Thanks,
Charles
Richard Fitzgerald Nov. 1, 2018, 11:40 a.m. UTC | #21
On 01/11/18 10:28, Charles Keepax wrote:
> On Mon, Oct 29, 2018 at 11:04:39AM +0000, Lee Jones wrote:
>> On Fri, 26 Oct 2018, Mark Brown wrote:
>>> On Fri, Oct 26, 2018 at 09:00:51AM +0100, Lee Jones wrote:
>>>> On Thu, 25 Oct 2018, Richard Fitzgerald wrote:
> 
> I have re-ordered some of the quotes here for the benefit
> of clarity. I will start with the Lochnagar focused bits
> and then cover some of the more general regmap discussion.
> Apologies for the wall of text towards the end but it seemed
> wise to explore some of the why for parts of the current regmap
> implementation and some of the implications for changing it.
> 
>>> I think from the perspective of Richard and Charles who are just trying
>>> to get their driver merged this is something of an abstract distinction.
>>> If the driver were merged and this discussion were happening separately
>>> their perspective would most likely be different.
>>
>> Charles has already mentioned that he'd take a look at the current
>> *use* (not changing the API, but the way in which Lochnagar
>> *uses/consumes* it).  Actions which would be most welcomed.
>>
>>> I kind of said this above but just to be clear this driver seems to me
>>> like an idiomatic user of the regmap API as it is today.  My guess is
>>> that we could probably loose the defaults tables and not suffer too much
>>> but equally well they don't hurt from a regmap point of view.
>>
>> Perhaps Charles could elaborate on whether this is possible or not?
> 
> So on this front I have had a look through and we can drop the
> defaults tables for Lochnagar, although as I shall cover later
> Lochnagar is the exceptional case with respect to our normal
> devices.
> 
>> Utilising range support here would certainly help IMHO.
>>
> 
> I am rather hesitant to switch to the range API. Whilst it is
> significantly more clear in certain situations, such as say
> mapping out the memory for a DSP, where there exist large
> amorphis blobs of identical function registers. I am really
> not convinced it really suits something like the register map
> that controls Lochnagar. You have an intertwinned mix of
> various purpose registers, each with a clearly defined name
> and potentially with quite fine grained properties.
> 
> Certainly when working with the driver I want to be able to
> fairly quickly see what registers are marked as by name. The
> callbacks are very simple and I don't need to look up where
> register are in the regmap to be able check their attributes.
> But perhaps I have just got too used to seeing these callbacks,
> things do have a way of becoming normal after being exposed to
> them for a while.
> 
> What I will try for the next spin is to try to use as much:
> 
>    case REG1...REG2:
> 
> As I can without totally sacrificing grepability/clarity and
> hopefully that will get us to a compromise we can all live
> with at least for now. Lochnagar is a fairly small part so it
> feels like this should be doable.
> 
>>>>> +       ret = devm_of_platform_populate(dev);
>>>>> +       if (ret < 0) {
>>>>> +               dev_err(dev, "Failed to populate child
>>>>> nodes:
>>>>> %d\n", ret);
>>>>> +               return ret;
>>>>> +       }
>>>>
>>>> Please do not mix OF and MFD device registration
>>>> strategies.
>>>>
>>>> Pick one and register all devices through your chosen
>>>> method.
>>>
>>> Hmmm we use this to do things like register some fixed
>>> regulators
>>> and clocks that don't need any control but do need to be
>>> associated
>>> with the device. I could do that through the MFD although it
>>> is in
>>> direct conflict with the feedback on the clock patches I
>>> received
>>> to move the fixed clocks into devicetree rather than
>>> registering
>>> them manually (see v2 of the patch chain).
>>
>> The I suggest moving everything to DT.
> 
> So pulling this out from earlier discussions in this thread,
> it seems I can happily move all the child device registration
> into device tree. I will also try this for the next version of
> the patch, unless anyone wants to object? But it does change
> the DT binding quite a lot as the individual sub drivers now
> each require their own node rather than one single unified
> Lochnagar node.
> 

We went through this discussion with the Madera MFD patches. I had
originally implemented it using DT to register the child drivers and
it was nice in some ways each driver having its own node. But Mark
and Rob didn't like it so I went back to non-DT child registration with
all sharing the parent MFD node. It would be nice if we could stick to
one way of doing it so that Cirrus drivers don't flip-flop between
different styles of DT binding.

With the Madera MFD, since it now uses non-DT registration of children
if there was a reason we need to be able to register DT-defined children
(and there are potential uses like adding platform-specific virtual
regulators that are treated as a child) the bindings are now fixed so we
would end up having a mixed non-DT and DT registration.

<snip>
Mark Brown Nov. 1, 2018, 12:01 p.m. UTC | #22
On Thu, Nov 01, 2018 at 10:28:28AM +0000, Charles Keepax wrote:

>      1.2) Read all the registers from the device on boot
>           + Uses less memory
>           - Potentially very slow boot time
>      1.3) Only read values as you touch registers
>           + Uses less memory
>           + Usually no significant impact on boot time
>           - Can't do read or read/write/modify operations on
>             previously untouched registers whilst chip is off

Both of these options also require that you be able to reset the device
which isn't possible with all devices and systems.

> I think this really the crux of my concerns here about updating
> the regmap API are that it feels like almost any path we take
> from here results in worse runtime performance and probably
> equal or worse memory usage. In return we gain a reduction of
> some, whilst admittedly large, very simple data tables from
> the driver. Which is a trade it feels hard to get invested in.

Right, I don't see an urgent problem either - if the tables were
complicated to think about it'd be a bit different but they aren't
particularly and like you say there's tradeoffs both at runtime and
at development time for trying to make the source code smaller.

Equally if there are useful things we can add to the API for some use
cases I'm all for that so long as they don't get in the way of other
users, most of the extensions in the API have come from someone having
problems and coming up with a fix that meets their needs.
Mark Brown Nov. 1, 2018, 12:04 p.m. UTC | #23
On Thu, Nov 01, 2018 at 11:40:01AM +0000, Richard Fitzgerald wrote:
> On 01/11/18 10:28, Charles Keepax wrote:

> > So pulling this out from earlier discussions in this thread,
> > it seems I can happily move all the child device registration
> > into device tree. I will also try this for the next version of
> > the patch, unless anyone wants to object? But it does change
> > the DT binding quite a lot as the individual sub drivers now
> > each require their own node rather than one single unified
> > Lochnagar node.

> We went through this discussion with the Madera MFD patches. I had
> originally implemented it using DT to register the child drivers and
> it was nice in some ways each driver having its own node. But Mark
> and Rob didn't like it so I went back to non-DT child registration with
> all sharing the parent MFD node. It would be nice if we could stick to
> one way of doing it so that Cirrus drivers don't flip-flop between
> different styles of DT binding.

The basic concern I have is encoding the current Linux idea of how to
split the subfunctions up into drivers into an ABI - the clocks in CODEC
drivers is the obvious example, they might want to be in the clock API
in future.  If there's a very direct mapping onto individual hardware
blocks that worries me a lot less since it's more obviously reflecting
how the hardware is designed.
Richard Fitzgerald Nov. 1, 2018, 2:17 p.m. UTC | #24
On 01/11/18 10:28, Charles Keepax wrote:
> On Mon, Oct 29, 2018 at 11:04:39AM +0000, Lee Jones wrote:
>> On Fri, 26 Oct 2018, Mark Brown wrote:
>>> On Fri, Oct 26, 2018 at 09:00:51AM +0100, Lee Jones wrote:
>>>> On Thu, 25 Oct 2018, Richard Fitzgerald wrote:
> 

<snip>

>>>> Precisely my point.  Lochnagar is a small device yet it's required to
>>>> submit hundreds of lines of Regmap tables.  Imagine what that would
>>>> look like for a large device.
>>>
>>> There's no *requirement* to provide the data even if you're using the
>>> cache (and the cache support is entirely optional), there's just costs
>>> to not providing it in terms of what features you can get from the
>>> regmap API and the performance of the system.  Not every device is going
>>> to be bothered by those costs, many devices don't provide all of the
>>> data they could.
>>
>> So what do we do in the case where, due to the size of the device, the
>> amount of lines required by these tables go from crazy to grotesque?
>>
> 
> Ultimately, I guess I have always just viewed it as just data
> tables. Its a lot of lines of source but its not complicated,
> and complexity has really always been the thing I try to avoid.
>

Again my question is why does it matter how many lines of source the tables
take? Unlike actual code, reducing the number of lines in a table doesn't
mean its more efficient. The tables aren't even particularly large when
compiled, a few kilobytes for CS47L90, which the users of that codec don't
care about compared to speed.

I can give a size comparison based on the Madera patches. When built as
modules for 32-bit ARM, the pinctrl driver for the (very simple) GPIOs on
those codecs builds to 22kB. All the regmap tables for the CS47L90
(the largest codec), including the defaults and all the readable/volatile
functions, builds to 23kB, and the smaller (but still quite big) CS47L35
to 16kB. So we're talking <= a trivial pinctrl driver even for a big
complex device like a CS47L90 with >1350 registers. For another comparison
the bulk of the CS47L90 object is the audio driver, which is ~2.2MB. So
the regmap tables are < 0.1%.

>>>>>> Even if it is absolutely critical that you have to supply these to
>>>>>> Regmap up-front, instead of on first use/read, why can't you just
>>>>>> supply the oddball non-zero ones?
>>>
>>> That doesn't work, we need to know both if the register has a default
>>> value and what that value is - there's no great value in only supplying
>>> the defaults for registers with non-zero values.
>>
>> All registers have a default value.  Why can't we assume that if a
>> register is writable and a default value was omitted then the default
>> is zero?
> 
> Defaults basically serve two purposes in regmap:
> 
>   1) Initialise the register cache.
> 
>      There are basically three ways you could handle this
>      (at least that I can think of) and regmap supports all
>      three. Obviously each with their own pros and cons:
> 
>       1.1) Table of default values
>            + Fast boot time
>            - Uses some additional memory
>       1.2) Read all the registers from the device on boot
>            + Uses less memory
>            - Potentially very slow boot time
>       1.3) Only read values as you touch registers
>            + Uses less memory
>            + Usually no significant impact on boot time
>            - Can't do read or read/write/modify operations on
>              previously untouched registers whilst chip is off
> 
>      1.3 does probably make sense here for Lochnagar since we
>      don't currently power things down. However, it is worth
>      noting that such an approach is extremely challenging for many
>      devices. For example the CODECs generally have all sorts of
>      actual user-facing interface that needs to be accessible
>      regardless of if the CODEC is powered up or down and powering it
>      up to access registers would end up being either horrific on
>      power consumption and/or horrific in terms of code complexity.
> 
>      1.1 and 1.2 are basically a straight trade off. Generally
>      for our devices we talking loads of registers and
>      potentially connected over I2C. Our customers care deeply
>      about device boot time, Android has specs for such things
>      and often it is tight for a system to make those specs.
>      Conversly, generally the products we integrate into have
>      a fairly large amount of memory. As such this is a no
>      brainer of a choice for most of our devices.
> 
>   2) Determine what registers should be synchronised on a cache
>      sync.
> 
>      A cache sync is usually done when pulling a device out of
>      low power mode to reapply the currently desired register
>      settings. As discussed in 1) we don't currently do cache
>      syncs on Lochnagar, but this would affect most of our
>      parts. Again I can see three approaches to synchronising
>      the cache:
> 
>       2.1) Sync out registers that arn't at their default value
>            + Only syncs register that are actually needed
>            - Requires a table of defaults to work
>       2.2) Store a per register dirty flag in the cache
>            + No up front table required in the driver
>            + Potentially less memory as only a single bit required
>              per register, although realising that saving might be
>              very hard on some cache types
>            - May sync registers that arn't required
>       2.3) Sync all registers from the cache
>            + No memory usage
>            - Potentially large penalty for cache sync
> 
>      Regmap has options to do 2.3, however for most of our
>      devices that would be totally unacceptable. Our parts have
>      a lot of registers most of which are cacheable and for
>      power reasons cache syncs happen as the audio path is being
>      brought up. Again Android has targets here and they are
>      in low 10's of millseconds so bus accesses really do matter.
> 
>      2.1 is the normal proceedure in regmap, although this
>      is defined on a per cache implementation basis, with the
>      core providing 2.1 as a default.
> 
>      Again it's a bit of a trade off between 2.1 and 2.2, but
>      given 1 pretty much requires us to know the defaults
>      anyway, 2.1 will in general make the most sense, at least
>      for Cirrus parts.
> 
> So I would conclude, certainly for most Cirrus devices, we
> do need to know the defaults at least for the vast majority
> of registers. I guess the next question would be could we
> generate some of the defaults table to reduce the table size
> in the driver. I would agree with you that the only sensible
> approach to reducing the defaults table size I can see would be
> to not have to specify defaults for registers with a default
> of zero. As an example to set expectations cs47l90, probably
> the largest CODEC we have made, has 1357 defaults of which
> 693 are zero so ~50%.
> 
> The issue really boils down to how do we tell the difference
> between a register that has no default, and one that has a default
> of zero? There are a few problems I can foresee on this front:
> 
>   1) There are occasions where people use a readable,
>      non-volatile register with no default for things like
>      device ID or revision. The idea being it can be read once
>      which will populate it into the cache then it never needs
>      to be read from the hardware again. Especially useful if
>      this information might be required whilst the hardware
>      is powered down.
> 
>      We could potentially update such drivers to mark the
>      register as volatile and then store the value explicitly
>      in the drivers data structures?
> 
>   2) There are occasions where a readable, writeable,
>      non-volatile register cannot be given a fixed default.
>      Usually this will be registers that are configured by
>      firmware or hardware during boot but then have control
>      passed to the kernel.
> 
>      For example a couple of registers on Lochnagar will be
>      configured by the board controller itself depending on
>      the CODEC that is attached, mostly regulators that are
>      enabled during boot being set to appropriate voltages to
>      avoid hardware damage. To handle these we don't give them
>      defaults which forces a read from the device but then after
>      that we can use the cache.
> 
>      For this one would probably have to add a new register
>      property (in addition to the existing readable, writeable,
>      volatile, and precious) for no default. Which would require
>      an additional callback. Although I guess that would cover 1
>      as well, and normally there would be very few registers in
>      this catagory.
> 
>   3) Rather nervous there are other issues I am not currently
>      thinking of this is a widely used API and I am mostly
>      familiar only with the style of devices I work with.
> 
>      We could potentially add an assume zero defaults flag,
>      that would at least then only apply the change to devices
>      that opt in?
> 
> One other related thing that rests on my mind is that creating
> the defaults table is going to be a little intensive. I guess
> it is not so bad if using the ranges API, so perhaps we would
> need to tie it in with that. Although it is still a little
> fiddly as you need to work out overlaps between the ranges for
> different properties to be more efficient than just checking
> each register.  For the callback based systems though you
> would have to check each register and for example coming
> back to cs47l90, the highest register address is 0x3FFE7C
> which means you would need to call over 4 million callbacks
> probably multiple times whilst constructing your defaults table.
>

Part of the point of the cache is that you can read and write non-volatile
register values without powering up the hardware. For that to work for
registers with zero defaults you have 3 options:

1) Add extra conditionals into the regmap cache code so that if a register
is accessed and a default hasn't been declared, check (using some other
configuration table or function) whether this is one that must be populated
from the hardware or can be assumed to be zero.
   + would work
   - it's more overhead in the cache code paths and the caching code is
     already adding code path overhead. It's not safe to assume "well we
     only need to do this once so it's ok", what if a time-critical block
     of code accesses a large sequence of registers that all invoke this
     extra code path?

2) Power-up the hardware during boot and touch all the registers with
zero values.
   + ensures the cache is pre-populated with all defaults so no deferred
     overhead that could happen at an inconvenient time as (1) would.
   - adds boot time overhead.
   - if the registers are sparse (as on Cirrus codecs) this can't be a
     simple loop, it would need a table and we were trying to avoid
     tabulating registers that have a zero default

3) Regmap initializes to zero all cache entries where the register default
wasn't given and the register is not in the list "non-volatile registers that
must be populated from the real hardware value"
   + as (2) it avoids the potential of a large deferred cache initialization
     at an inconvenient time.
   - a missing address in the defaults table could be
        i.   a register with a zero default,
        ii.  a register that must be initialized from the hardware,
        iii. an unused address.
     The only way regmap could decide would be to check the readable/writable
     definitions and a new table of "non-volatile registers that must be
     populated from the real hardware value" to decide whether the address
     is a register or not. For a very large sparse register map there could
     be a huge number of addresses that have to be checked. We can get some
     idea how long this might take, regmap's debugfs builds a table this way
     and for the largest Cirrus codec (not upstream yet) we've seen this take
     up to 2 minutes (on an ARM CPU). While that's a maximum it gives some
     idea - even if we added only 10 seconds to boot time that's 10 seconds
     to long for most users of our codecs.

4) Regmap pre-populates the entire cache with zeros and then fills in any
non-zero defaults from the provided defaults table.
    + simpler to implement in regmap than (3)
    - more overhead than (3), you have to fill the whole cache so you're
      invoking the full 2 minute overhead that I mentioned in (3).

In practice (1) is feasible and could be acceptable for some devices
(but those are quite likely also devices where the current defaults table
isn't large anyway). It would have to be an option flag because it could
break the behaviour of existing regmap clients. But the potential for a
deferred overhead popping up at bad times might not always be acceptable.
One particular bad place is syncing the cache around a suspend/resume to
know the default values to eliminate useless writes so effectively (1) is
turning into (3). As a suspend/resume can be subject to tight OS limits
on time to setup a use-case it's not something we want to be bloating,
even if it's only on the first invocation.

In fact if there was anything we really wanted to contribute to regmap it
would be things that makes it faster. There don't seem to be any options
for shortening the data tables that don't also make regmap slower in some
way. I suppose if someone could make regmap faster we'd then have some
extra leeway to put in things that add overhead but then it's a shame to
have gained some speed and then take it away.

<snip>
diff mbox series

Patch

diff --git a/MAINTAINERS b/MAINTAINERS
index d9e6d86488df4..f1f3494ac5cd8 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -3557,6 +3557,19 @@  L:	netdev@vger.kernel.org
 S:	Maintained
 F:	drivers/net/ethernet/cirrus/ep93xx_eth.c
 
+CIRRUS LOGIC LOCHNAGAR DRIVER
+M:	Charles Keepax <ckeepax@opensource.cirrus.com>
+M:	Richard Fitzgerald <rf@opensource.cirrus.com>
+L:	patches@opensource.cirrus.com
+S:	Supported
+F:	drivers/clk/clk-lochnagar.c
+F:	drivers/mfd/lochnagar-i2c.c
+F:	drivers/pinctrl/cirrus/pinctrl-lochnagar.c
+F:	drivers/regulator/lochnagar-regulator.c
+F:	include/dt-bindings/clk/lochnagar.h
+F:	include/dt-bindings/pinctrl/lochnagar.h
+F:	include/linux/mfd/lochnagar*
+
 CISCO FCOE HBA DRIVER
 M:	Satish Kharat <satishkh@cisco.com>
 M:	Sesidhar Baddela <sebaddel@cisco.com>
diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
index f3a5f8d027906..dc64151373714 100644
--- a/drivers/mfd/Kconfig
+++ b/drivers/mfd/Kconfig
@@ -1676,6 +1676,14 @@  config MFD_VX855
 	  VIA VX855/VX875 south bridge. You will need to enable the vx855_spi
 	  and/or vx855_gpio drivers for this to do anything useful.
 
+config MFD_LOCHNAGAR
+	bool "Cirrus Logic Lochnagar Audio Development Board"
+	select MFD_CORE
+	select REGMAP_I2C
+	depends on I2C=y && OF
+	help
+	  Support for Cirrus Logic Lochnagar audio development board.
+
 config MFD_ARIZONA
 	select REGMAP
 	select REGMAP_IRQ
diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
index 5856a9489cbd8..f16773cb887ff 100644
--- a/drivers/mfd/Makefile
+++ b/drivers/mfd/Makefile
@@ -37,6 +37,8 @@  obj-$(CONFIG_MFD_T7L66XB)	+= t7l66xb.o tmio_core.o
 obj-$(CONFIG_MFD_TC6387XB)	+= tc6387xb.o tmio_core.o
 obj-$(CONFIG_MFD_TC6393XB)	+= tc6393xb.o tmio_core.o
 
+obj-$(CONFIG_MFD_LOCHNAGAR)	+= lochnagar-i2c.o
+
 obj-$(CONFIG_MFD_ARIZONA)	+= arizona-core.o
 obj-$(CONFIG_MFD_ARIZONA)	+= arizona-irq.o
 obj-$(CONFIG_MFD_ARIZONA_I2C)	+= arizona-i2c.o
diff --git a/drivers/mfd/lochnagar-i2c.c b/drivers/mfd/lochnagar-i2c.c
new file mode 100644
index 0000000000000..7ac7af9e3130b
--- /dev/null
+++ b/drivers/mfd/lochnagar-i2c.c
@@ -0,0 +1,732 @@ 
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Lochnagar I2C bus interface
+ *
+ * Copyright (c) 2012-2018 Cirrus Logic, Inc. and
+ *                         Cirrus Logic International Semiconductor Ltd.
+ *
+ * Author: Charles Keepax <ckeepax@opensource.cirrus.com>
+ */
+
+#include <linux/delay.h>
+#include <linux/device.h>
+#include <linux/err.h>
+#include <linux/gpio/consumer.h>
+#include <linux/i2c.h>
+#include <linux/mfd/core.h>
+#include <linux/mutex.h>
+#include <linux/of.h>
+#include <linux/of_platform.h>
+#include <linux/regmap.h>
+
+#include <linux/mfd/lochnagar.h>
+
+#define LOCHNAGAR_BOOT_RETRIES		10
+#define LOCHNAGAR_BOOT_DELAY_MS		350
+
+#define LOCHNAGAR_CONFIG_POLL_US	10000
+
+static bool lochnagar1_readable_register(struct device *dev, unsigned int reg)
+{
+	switch (reg) {
+	case LOCHNAGAR_SOFTWARE_RESET:
+	case LOCHNAGAR_FIRMWARE_ID1:
+	case LOCHNAGAR_FIRMWARE_ID2:
+	case LOCHNAGAR1_CDC_AIF1_SEL:
+	case LOCHNAGAR1_CDC_AIF2_SEL:
+	case LOCHNAGAR1_CDC_AIF3_SEL:
+	case LOCHNAGAR1_CDC_MCLK1_SEL:
+	case LOCHNAGAR1_CDC_MCLK2_SEL:
+	case LOCHNAGAR1_CDC_AIF_CTRL1:
+	case LOCHNAGAR1_CDC_AIF_CTRL2:
+	case LOCHNAGAR1_EXT_AIF_CTRL:
+	case LOCHNAGAR1_DSP_AIF1_SEL:
+	case LOCHNAGAR1_DSP_AIF2_SEL:
+	case LOCHNAGAR1_DSP_CLKIN_SEL:
+	case LOCHNAGAR1_DSP_AIF:
+	case LOCHNAGAR1_GF_AIF1:
+	case LOCHNAGAR1_GF_AIF2:
+	case LOCHNAGAR1_PSIA_AIF:
+	case LOCHNAGAR1_PSIA1_SEL:
+	case LOCHNAGAR1_PSIA2_SEL:
+	case LOCHNAGAR1_SPDIF_AIF_SEL:
+	case LOCHNAGAR1_GF_AIF3_SEL:
+	case LOCHNAGAR1_GF_AIF4_SEL:
+	case LOCHNAGAR1_GF_CLKOUT1_SEL:
+	case LOCHNAGAR1_GF_AIF1_SEL:
+	case LOCHNAGAR1_GF_AIF2_SEL:
+	case LOCHNAGAR1_GF_GPIO2:
+	case LOCHNAGAR1_GF_GPIO3:
+	case LOCHNAGAR1_GF_GPIO7:
+	case LOCHNAGAR1_RST:
+	case LOCHNAGAR1_LED1:
+	case LOCHNAGAR1_LED2:
+	case LOCHNAGAR1_I2C_CTRL:
+		return true;
+	default:
+		return false;
+	}
+}
+
+static const struct reg_default lochnagar1_reg_defaults[] = {
+	{ LOCHNAGAR1_CDC_AIF1_SEL,    0x00 },
+	{ LOCHNAGAR1_CDC_AIF2_SEL,    0x00 },
+	{ LOCHNAGAR1_CDC_AIF3_SEL,    0x00 },
+	{ LOCHNAGAR1_CDC_MCLK1_SEL,   0x00 },
+	{ LOCHNAGAR1_CDC_MCLK2_SEL,   0x00 },
+	{ LOCHNAGAR1_CDC_AIF_CTRL1,   0x00 },
+	{ LOCHNAGAR1_CDC_AIF_CTRL2,   0x00 },
+	{ LOCHNAGAR1_EXT_AIF_CTRL,    0x00 },
+	{ LOCHNAGAR1_DSP_AIF1_SEL,    0x00 },
+	{ LOCHNAGAR1_DSP_AIF2_SEL,    0x00 },
+	{ LOCHNAGAR1_DSP_CLKIN_SEL,   0x01 },
+	{ LOCHNAGAR1_DSP_AIF,         0x08 },
+	{ LOCHNAGAR1_GF_AIF1,         0x00 },
+	{ LOCHNAGAR1_GF_AIF2,         0x00 },
+	{ LOCHNAGAR1_PSIA_AIF,        0x00 },
+	{ LOCHNAGAR1_PSIA1_SEL,       0x00 },
+	{ LOCHNAGAR1_PSIA2_SEL,       0x00 },
+	{ LOCHNAGAR1_SPDIF_AIF_SEL,   0x00 },
+	{ LOCHNAGAR1_GF_AIF3_SEL,     0x00 },
+	{ LOCHNAGAR1_GF_AIF4_SEL,     0x00 },
+	{ LOCHNAGAR1_GF_CLKOUT1_SEL,  0x00 },
+	{ LOCHNAGAR1_GF_AIF1_SEL,     0x00 },
+	{ LOCHNAGAR1_GF_AIF2_SEL,     0x00 },
+	{ LOCHNAGAR1_GF_GPIO2,        0x00 },
+	{ LOCHNAGAR1_GF_GPIO3,        0x00 },
+	{ LOCHNAGAR1_GF_GPIO7,        0x00 },
+	{ LOCHNAGAR1_RST,             0x00 },
+	{ LOCHNAGAR1_LED1,            0x00 },
+	{ LOCHNAGAR1_LED2,            0x00 },
+	{ LOCHNAGAR1_I2C_CTRL,        0x01 },
+};
+
+static const struct regmap_config lochnagar1_i2c_regmap = {
+	.reg_bits = 8,
+	.val_bits = 8,
+	.reg_format_endian = REGMAP_ENDIAN_BIG,
+	.val_format_endian = REGMAP_ENDIAN_BIG,
+
+	.max_register = 0x50,
+	.readable_reg = lochnagar1_readable_register,
+
+	.use_single_rw = true,
+
+	.cache_type = REGCACHE_RBTREE,
+
+	.reg_defaults = lochnagar1_reg_defaults,
+	.num_reg_defaults = ARRAY_SIZE(lochnagar1_reg_defaults),
+};
+
+static const struct reg_sequence lochnagar1_patch[] = {
+	{ 0x40, 0x0083 },
+	{ 0x46, 0x0001 },
+	{ 0x47, 0x0018 },
+	{ 0x50, 0x0000 },
+};
+
+static struct mfd_cell lochnagar1_devs[] = {
+	{
+		.name = "lochnagar-pinctrl"
+	},
+	{
+		.name = "lochnagar-clk"
+	},
+};
+
+static bool lochnagar2_readable_register(struct device *dev, unsigned int reg)
+{
+	switch (reg) {
+	case LOCHNAGAR_SOFTWARE_RESET:
+	case LOCHNAGAR_FIRMWARE_ID1:
+	case LOCHNAGAR_FIRMWARE_ID2:
+	case LOCHNAGAR2_CDC_AIF1_CTRL:
+	case LOCHNAGAR2_CDC_AIF2_CTRL:
+	case LOCHNAGAR2_CDC_AIF3_CTRL:
+	case LOCHNAGAR2_DSP_AIF1_CTRL:
+	case LOCHNAGAR2_DSP_AIF2_CTRL:
+	case LOCHNAGAR2_PSIA1_CTRL:
+	case LOCHNAGAR2_PSIA2_CTRL:
+	case LOCHNAGAR2_GF_AIF3_CTRL:
+	case LOCHNAGAR2_GF_AIF4_CTRL:
+	case LOCHNAGAR2_GF_AIF1_CTRL:
+	case LOCHNAGAR2_GF_AIF2_CTRL:
+	case LOCHNAGAR2_SPDIF_AIF_CTRL:
+	case LOCHNAGAR2_USB_AIF1_CTRL:
+	case LOCHNAGAR2_USB_AIF2_CTRL:
+	case LOCHNAGAR2_ADAT_AIF_CTRL:
+	case LOCHNAGAR2_CDC_MCLK1_CTRL:
+	case LOCHNAGAR2_CDC_MCLK2_CTRL:
+	case LOCHNAGAR2_DSP_CLKIN_CTRL:
+	case LOCHNAGAR2_PSIA1_MCLK_CTRL:
+	case LOCHNAGAR2_PSIA2_MCLK_CTRL:
+	case LOCHNAGAR2_SPDIF_MCLK_CTRL:
+	case LOCHNAGAR2_GF_CLKOUT1_CTRL:
+	case LOCHNAGAR2_GF_CLKOUT2_CTRL:
+	case LOCHNAGAR2_ADAT_MCLK_CTRL:
+	case LOCHNAGAR2_SOUNDCARD_MCLK_CTRL:
+	case LOCHNAGAR2_GPIO_FPGA_GPIO1:
+	case LOCHNAGAR2_GPIO_FPGA_GPIO2:
+	case LOCHNAGAR2_GPIO_FPGA_GPIO3:
+	case LOCHNAGAR2_GPIO_FPGA_GPIO4:
+	case LOCHNAGAR2_GPIO_FPGA_GPIO5:
+	case LOCHNAGAR2_GPIO_FPGA_GPIO6:
+	case LOCHNAGAR2_GPIO_CDC_GPIO1:
+	case LOCHNAGAR2_GPIO_CDC_GPIO2:
+	case LOCHNAGAR2_GPIO_CDC_GPIO3:
+	case LOCHNAGAR2_GPIO_CDC_GPIO4:
+	case LOCHNAGAR2_GPIO_CDC_GPIO5:
+	case LOCHNAGAR2_GPIO_CDC_GPIO6:
+	case LOCHNAGAR2_GPIO_CDC_GPIO7:
+	case LOCHNAGAR2_GPIO_CDC_GPIO8:
+	case LOCHNAGAR2_GPIO_DSP_GPIO1:
+	case LOCHNAGAR2_GPIO_DSP_GPIO2:
+	case LOCHNAGAR2_GPIO_DSP_GPIO3:
+	case LOCHNAGAR2_GPIO_DSP_GPIO4:
+	case LOCHNAGAR2_GPIO_DSP_GPIO5:
+	case LOCHNAGAR2_GPIO_DSP_GPIO6:
+	case LOCHNAGAR2_GPIO_GF_GPIO2:
+	case LOCHNAGAR2_GPIO_GF_GPIO3:
+	case LOCHNAGAR2_GPIO_GF_GPIO7:
+	case LOCHNAGAR2_GPIO_CDC_AIF1_BCLK:
+	case LOCHNAGAR2_GPIO_CDC_AIF1_RXDAT:
+	case LOCHNAGAR2_GPIO_CDC_AIF1_LRCLK:
+	case LOCHNAGAR2_GPIO_CDC_AIF1_TXDAT:
+	case LOCHNAGAR2_GPIO_CDC_AIF2_BCLK:
+	case LOCHNAGAR2_GPIO_CDC_AIF2_RXDAT:
+	case LOCHNAGAR2_GPIO_CDC_AIF2_LRCLK:
+	case LOCHNAGAR2_GPIO_CDC_AIF2_TXDAT:
+	case LOCHNAGAR2_GPIO_CDC_AIF3_BCLK:
+	case LOCHNAGAR2_GPIO_CDC_AIF3_RXDAT:
+	case LOCHNAGAR2_GPIO_CDC_AIF3_LRCLK:
+	case LOCHNAGAR2_GPIO_CDC_AIF3_TXDAT:
+	case LOCHNAGAR2_GPIO_DSP_AIF1_BCLK:
+	case LOCHNAGAR2_GPIO_DSP_AIF1_RXDAT:
+	case LOCHNAGAR2_GPIO_DSP_AIF1_LRCLK:
+	case LOCHNAGAR2_GPIO_DSP_AIF1_TXDAT:
+	case LOCHNAGAR2_GPIO_DSP_AIF2_BCLK:
+	case LOCHNAGAR2_GPIO_DSP_AIF2_RXDAT:
+	case LOCHNAGAR2_GPIO_DSP_AIF2_LRCLK:
+	case LOCHNAGAR2_GPIO_DSP_AIF2_TXDAT:
+	case LOCHNAGAR2_GPIO_PSIA1_BCLK:
+	case LOCHNAGAR2_GPIO_PSIA1_RXDAT:
+	case LOCHNAGAR2_GPIO_PSIA1_LRCLK:
+	case LOCHNAGAR2_GPIO_PSIA1_TXDAT:
+	case LOCHNAGAR2_GPIO_PSIA2_BCLK:
+	case LOCHNAGAR2_GPIO_PSIA2_RXDAT:
+	case LOCHNAGAR2_GPIO_PSIA2_LRCLK:
+	case LOCHNAGAR2_GPIO_PSIA2_TXDAT:
+	case LOCHNAGAR2_GPIO_GF_AIF3_BCLK:
+	case LOCHNAGAR2_GPIO_GF_AIF3_RXDAT:
+	case LOCHNAGAR2_GPIO_GF_AIF3_LRCLK:
+	case LOCHNAGAR2_GPIO_GF_AIF3_TXDAT:
+	case LOCHNAGAR2_GPIO_GF_AIF4_BCLK:
+	case LOCHNAGAR2_GPIO_GF_AIF4_RXDAT:
+	case LOCHNAGAR2_GPIO_GF_AIF4_LRCLK:
+	case LOCHNAGAR2_GPIO_GF_AIF4_TXDAT:
+	case LOCHNAGAR2_GPIO_GF_AIF1_BCLK:
+	case LOCHNAGAR2_GPIO_GF_AIF1_RXDAT:
+	case LOCHNAGAR2_GPIO_GF_AIF1_LRCLK:
+	case LOCHNAGAR2_GPIO_GF_AIF1_TXDAT:
+	case LOCHNAGAR2_GPIO_GF_AIF2_BCLK:
+	case LOCHNAGAR2_GPIO_GF_AIF2_RXDAT:
+	case LOCHNAGAR2_GPIO_GF_AIF2_LRCLK:
+	case LOCHNAGAR2_GPIO_GF_AIF2_TXDAT:
+	case LOCHNAGAR2_GPIO_DSP_UART1_RX:
+	case LOCHNAGAR2_GPIO_DSP_UART1_TX:
+	case LOCHNAGAR2_GPIO_DSP_UART2_RX:
+	case LOCHNAGAR2_GPIO_DSP_UART2_TX:
+	case LOCHNAGAR2_GPIO_GF_UART2_RX:
+	case LOCHNAGAR2_GPIO_GF_UART2_TX:
+	case LOCHNAGAR2_GPIO_USB_UART_RX:
+	case LOCHNAGAR2_GPIO_CDC_PDMCLK1:
+	case LOCHNAGAR2_GPIO_CDC_PDMDAT1:
+	case LOCHNAGAR2_GPIO_CDC_PDMCLK2:
+	case LOCHNAGAR2_GPIO_CDC_PDMDAT2:
+	case LOCHNAGAR2_GPIO_CDC_DMICCLK1:
+	case LOCHNAGAR2_GPIO_CDC_DMICDAT1:
+	case LOCHNAGAR2_GPIO_CDC_DMICCLK2:
+	case LOCHNAGAR2_GPIO_CDC_DMICDAT2:
+	case LOCHNAGAR2_GPIO_CDC_DMICCLK3:
+	case LOCHNAGAR2_GPIO_CDC_DMICDAT3:
+	case LOCHNAGAR2_GPIO_CDC_DMICCLK4:
+	case LOCHNAGAR2_GPIO_CDC_DMICDAT4:
+	case LOCHNAGAR2_GPIO_DSP_DMICCLK1:
+	case LOCHNAGAR2_GPIO_DSP_DMICDAT1:
+	case LOCHNAGAR2_GPIO_DSP_DMICCLK2:
+	case LOCHNAGAR2_GPIO_DSP_DMICDAT2:
+	case LOCHNAGAR2_GPIO_I2C2_SCL:
+	case LOCHNAGAR2_GPIO_I2C2_SDA:
+	case LOCHNAGAR2_GPIO_I2C3_SCL:
+	case LOCHNAGAR2_GPIO_I2C3_SDA:
+	case LOCHNAGAR2_GPIO_I2C4_SCL:
+	case LOCHNAGAR2_GPIO_I2C4_SDA:
+	case LOCHNAGAR2_GPIO_DSP_STANDBY:
+	case LOCHNAGAR2_GPIO_CDC_MCLK1:
+	case LOCHNAGAR2_GPIO_CDC_MCLK2:
+	case LOCHNAGAR2_GPIO_DSP_CLKIN:
+	case LOCHNAGAR2_GPIO_PSIA1_MCLK:
+	case LOCHNAGAR2_GPIO_PSIA2_MCLK:
+	case LOCHNAGAR2_GPIO_GF_GPIO1:
+	case LOCHNAGAR2_GPIO_GF_GPIO5:
+	case LOCHNAGAR2_GPIO_DSP_GPIO20:
+	case LOCHNAGAR2_GPIO_CHANNEL1:
+	case LOCHNAGAR2_GPIO_CHANNEL2:
+	case LOCHNAGAR2_GPIO_CHANNEL3:
+	case LOCHNAGAR2_GPIO_CHANNEL4:
+	case LOCHNAGAR2_GPIO_CHANNEL5:
+	case LOCHNAGAR2_GPIO_CHANNEL6:
+	case LOCHNAGAR2_GPIO_CHANNEL7:
+	case LOCHNAGAR2_GPIO_CHANNEL8:
+	case LOCHNAGAR2_GPIO_CHANNEL9:
+	case LOCHNAGAR2_GPIO_CHANNEL10:
+	case LOCHNAGAR2_GPIO_CHANNEL11:
+	case LOCHNAGAR2_GPIO_CHANNEL12:
+	case LOCHNAGAR2_GPIO_CHANNEL13:
+	case LOCHNAGAR2_GPIO_CHANNEL14:
+	case LOCHNAGAR2_GPIO_CHANNEL15:
+	case LOCHNAGAR2_GPIO_CHANNEL16:
+	case LOCHNAGAR2_MINICARD_RESETS:
+	case LOCHNAGAR2_ANALOGUE_PATH_CTRL1:
+	case LOCHNAGAR2_ANALOGUE_PATH_CTRL2:
+	case LOCHNAGAR2_COMMS_CTRL4:
+	case LOCHNAGAR2_SPDIF_CTRL:
+	case LOCHNAGAR2_POWER_CTRL:
+	case LOCHNAGAR2_MICVDD_CTRL1:
+	case LOCHNAGAR2_MICVDD_CTRL2:
+	case LOCHNAGAR2_VDDCORE_CDC_CTRL1:
+	case LOCHNAGAR2_VDDCORE_CDC_CTRL2:
+	case LOCHNAGAR2_SOUNDCARD_AIF_CTRL:
+		return true;
+	default:
+		return false;
+	}
+}
+
+static bool lochnagar2_volatile_register(struct device *dev, unsigned int reg)
+{
+	switch (reg) {
+	case LOCHNAGAR2_GPIO_CHANNEL1:
+	case LOCHNAGAR2_GPIO_CHANNEL2:
+	case LOCHNAGAR2_GPIO_CHANNEL3:
+	case LOCHNAGAR2_GPIO_CHANNEL4:
+	case LOCHNAGAR2_GPIO_CHANNEL5:
+	case LOCHNAGAR2_GPIO_CHANNEL6:
+	case LOCHNAGAR2_GPIO_CHANNEL7:
+	case LOCHNAGAR2_GPIO_CHANNEL8:
+	case LOCHNAGAR2_GPIO_CHANNEL9:
+	case LOCHNAGAR2_GPIO_CHANNEL10:
+	case LOCHNAGAR2_GPIO_CHANNEL11:
+	case LOCHNAGAR2_GPIO_CHANNEL12:
+	case LOCHNAGAR2_GPIO_CHANNEL13:
+	case LOCHNAGAR2_GPIO_CHANNEL14:
+	case LOCHNAGAR2_GPIO_CHANNEL15:
+	case LOCHNAGAR2_GPIO_CHANNEL16:
+	case LOCHNAGAR2_ANALOGUE_PATH_CTRL1:
+		return true;
+	default:
+		return false;
+	}
+}
+
+static const struct reg_default lochnagar2_reg_defaults[] = {
+	{ LOCHNAGAR2_CDC_AIF1_CTRL,         0x0000 },
+	{ LOCHNAGAR2_CDC_AIF2_CTRL,         0x0000 },
+	{ LOCHNAGAR2_CDC_AIF3_CTRL,         0x0000 },
+	{ LOCHNAGAR2_DSP_AIF1_CTRL,         0x0000 },
+	{ LOCHNAGAR2_DSP_AIF2_CTRL,         0x0000 },
+	{ LOCHNAGAR2_PSIA1_CTRL,            0x0000 },
+	{ LOCHNAGAR2_PSIA2_CTRL,            0x0000 },
+	{ LOCHNAGAR2_GF_AIF3_CTRL,          0x0000 },
+	{ LOCHNAGAR2_GF_AIF4_CTRL,          0x0000 },
+	{ LOCHNAGAR2_GF_AIF1_CTRL,          0x0000 },
+	{ LOCHNAGAR2_GF_AIF2_CTRL,          0x0000 },
+	{ LOCHNAGAR2_SPDIF_AIF_CTRL,        0x0000 },
+	{ LOCHNAGAR2_USB_AIF1_CTRL,         0x0000 },
+	{ LOCHNAGAR2_USB_AIF2_CTRL,         0x0000 },
+	{ LOCHNAGAR2_ADAT_AIF_CTRL,         0x0000 },
+	{ LOCHNAGAR2_CDC_MCLK1_CTRL,        0x0000 },
+	{ LOCHNAGAR2_CDC_MCLK2_CTRL,        0x0000 },
+	{ LOCHNAGAR2_DSP_CLKIN_CTRL,        0x0000 },
+	{ LOCHNAGAR2_PSIA1_MCLK_CTRL,       0x0000 },
+	{ LOCHNAGAR2_PSIA2_MCLK_CTRL,       0x0000 },
+	{ LOCHNAGAR2_SPDIF_MCLK_CTRL,       0x0000 },
+	{ LOCHNAGAR2_GF_CLKOUT1_CTRL,       0x0000 },
+	{ LOCHNAGAR2_GF_CLKOUT2_CTRL,       0x0000 },
+	{ LOCHNAGAR2_ADAT_MCLK_CTRL,        0x0000 },
+	{ LOCHNAGAR2_SOUNDCARD_MCLK_CTRL,   0x0000 },
+	{ LOCHNAGAR2_GPIO_FPGA_GPIO1,       0x0000 },
+	{ LOCHNAGAR2_GPIO_FPGA_GPIO2,       0x0000 },
+	{ LOCHNAGAR2_GPIO_FPGA_GPIO3,       0x0000 },
+	{ LOCHNAGAR2_GPIO_FPGA_GPIO4,       0x0000 },
+	{ LOCHNAGAR2_GPIO_FPGA_GPIO5,       0x0000 },
+	{ LOCHNAGAR2_GPIO_FPGA_GPIO6,       0x0000 },
+	{ LOCHNAGAR2_GPIO_CDC_GPIO1,        0x0000 },
+	{ LOCHNAGAR2_GPIO_CDC_GPIO2,        0x0000 },
+	{ LOCHNAGAR2_GPIO_CDC_GPIO3,        0x0000 },
+	{ LOCHNAGAR2_GPIO_CDC_GPIO4,        0x0000 },
+	{ LOCHNAGAR2_GPIO_CDC_GPIO5,        0x0000 },
+	{ LOCHNAGAR2_GPIO_CDC_GPIO6,        0x0000 },
+	{ LOCHNAGAR2_GPIO_CDC_GPIO7,        0x0000 },
+	{ LOCHNAGAR2_GPIO_CDC_GPIO8,        0x0000 },
+	{ LOCHNAGAR2_GPIO_DSP_GPIO1,        0x0000 },
+	{ LOCHNAGAR2_GPIO_DSP_GPIO2,        0x0000 },
+	{ LOCHNAGAR2_GPIO_DSP_GPIO3,        0x0000 },
+	{ LOCHNAGAR2_GPIO_DSP_GPIO4,        0x0000 },
+	{ LOCHNAGAR2_GPIO_DSP_GPIO5,        0x0000 },
+	{ LOCHNAGAR2_GPIO_DSP_GPIO6,        0x0000 },
+	{ LOCHNAGAR2_GPIO_GF_GPIO2,         0x0000 },
+	{ LOCHNAGAR2_GPIO_GF_GPIO3,         0x0000 },
+	{ LOCHNAGAR2_GPIO_GF_GPIO7,         0x0000 },
+	{ LOCHNAGAR2_GPIO_CDC_AIF1_BCLK,    0x0000 },
+	{ LOCHNAGAR2_GPIO_CDC_AIF1_RXDAT,   0x0000 },
+	{ LOCHNAGAR2_GPIO_CDC_AIF1_LRCLK,   0x0000 },
+	{ LOCHNAGAR2_GPIO_CDC_AIF1_TXDAT,   0x0000 },
+	{ LOCHNAGAR2_GPIO_CDC_AIF2_BCLK,    0x0000 },
+	{ LOCHNAGAR2_GPIO_CDC_AIF2_RXDAT,   0x0000 },
+	{ LOCHNAGAR2_GPIO_CDC_AIF2_LRCLK,   0x0000 },
+	{ LOCHNAGAR2_GPIO_CDC_AIF2_TXDAT,   0x0000 },
+	{ LOCHNAGAR2_GPIO_CDC_AIF3_BCLK,    0x0000 },
+	{ LOCHNAGAR2_GPIO_CDC_AIF3_RXDAT,   0x0000 },
+	{ LOCHNAGAR2_GPIO_CDC_AIF3_LRCLK,   0x0000 },
+	{ LOCHNAGAR2_GPIO_CDC_AIF3_TXDAT,   0x0000 },
+	{ LOCHNAGAR2_GPIO_DSP_AIF1_BCLK,    0x0000 },
+	{ LOCHNAGAR2_GPIO_DSP_AIF1_RXDAT,   0x0000 },
+	{ LOCHNAGAR2_GPIO_DSP_AIF1_LRCLK,   0x0000 },
+	{ LOCHNAGAR2_GPIO_DSP_AIF1_TXDAT,   0x0000 },
+	{ LOCHNAGAR2_GPIO_DSP_AIF2_BCLK,    0x0000 },
+	{ LOCHNAGAR2_GPIO_DSP_AIF2_RXDAT,   0x0000 },
+	{ LOCHNAGAR2_GPIO_DSP_AIF2_LRCLK,   0x0000 },
+	{ LOCHNAGAR2_GPIO_DSP_AIF2_TXDAT,   0x0000 },
+	{ LOCHNAGAR2_GPIO_PSIA1_BCLK,       0x0000 },
+	{ LOCHNAGAR2_GPIO_PSIA1_RXDAT,      0x0000 },
+	{ LOCHNAGAR2_GPIO_PSIA1_LRCLK,      0x0000 },
+	{ LOCHNAGAR2_GPIO_PSIA1_TXDAT,      0x0000 },
+	{ LOCHNAGAR2_GPIO_PSIA2_BCLK,       0x0000 },
+	{ LOCHNAGAR2_GPIO_PSIA2_RXDAT,      0x0000 },
+	{ LOCHNAGAR2_GPIO_PSIA2_LRCLK,      0x0000 },
+	{ LOCHNAGAR2_GPIO_PSIA2_TXDAT,      0x0000 },
+	{ LOCHNAGAR2_GPIO_GF_AIF3_BCLK,     0x0000 },
+	{ LOCHNAGAR2_GPIO_GF_AIF3_RXDAT,    0x0000 },
+	{ LOCHNAGAR2_GPIO_GF_AIF3_LRCLK,    0x0000 },
+	{ LOCHNAGAR2_GPIO_GF_AIF3_TXDAT,    0x0000 },
+	{ LOCHNAGAR2_GPIO_GF_AIF4_BCLK,     0x0000 },
+	{ LOCHNAGAR2_GPIO_GF_AIF4_RXDAT,    0x0000 },
+	{ LOCHNAGAR2_GPIO_GF_AIF4_LRCLK,    0x0000 },
+	{ LOCHNAGAR2_GPIO_GF_AIF4_TXDAT,    0x0000 },
+	{ LOCHNAGAR2_GPIO_GF_AIF1_BCLK,     0x0000 },
+	{ LOCHNAGAR2_GPIO_GF_AIF1_RXDAT,    0x0000 },
+	{ LOCHNAGAR2_GPIO_GF_AIF1_LRCLK,    0x0000 },
+	{ LOCHNAGAR2_GPIO_GF_AIF1_TXDAT,    0x0000 },
+	{ LOCHNAGAR2_GPIO_GF_AIF2_BCLK,     0x0000 },
+	{ LOCHNAGAR2_GPIO_GF_AIF2_RXDAT,    0x0000 },
+	{ LOCHNAGAR2_GPIO_GF_AIF2_LRCLK,    0x0000 },
+	{ LOCHNAGAR2_GPIO_GF_AIF2_TXDAT,    0x0000 },
+	{ LOCHNAGAR2_GPIO_DSP_UART1_RX,     0x0000 },
+	{ LOCHNAGAR2_GPIO_DSP_UART1_TX,     0x0000 },
+	{ LOCHNAGAR2_GPIO_DSP_UART2_RX,     0x0000 },
+	{ LOCHNAGAR2_GPIO_DSP_UART2_TX,     0x0000 },
+	{ LOCHNAGAR2_GPIO_GF_UART2_RX,      0x0000 },
+	{ LOCHNAGAR2_GPIO_GF_UART2_TX,      0x0000 },
+	{ LOCHNAGAR2_GPIO_USB_UART_RX,      0x0000 },
+	{ LOCHNAGAR2_GPIO_CDC_PDMCLK1,      0x0000 },
+	{ LOCHNAGAR2_GPIO_CDC_PDMDAT1,      0x0000 },
+	{ LOCHNAGAR2_GPIO_CDC_PDMCLK2,      0x0000 },
+	{ LOCHNAGAR2_GPIO_CDC_PDMDAT2,      0x0000 },
+	{ LOCHNAGAR2_GPIO_CDC_DMICCLK1,     0x0000 },
+	{ LOCHNAGAR2_GPIO_CDC_DMICDAT1,     0x0000 },
+	{ LOCHNAGAR2_GPIO_CDC_DMICCLK2,     0x0000 },
+	{ LOCHNAGAR2_GPIO_CDC_DMICDAT2,     0x0000 },
+	{ LOCHNAGAR2_GPIO_CDC_DMICCLK3,     0x0000 },
+	{ LOCHNAGAR2_GPIO_CDC_DMICDAT3,     0x0000 },
+	{ LOCHNAGAR2_GPIO_CDC_DMICCLK4,     0x0000 },
+	{ LOCHNAGAR2_GPIO_CDC_DMICDAT4,     0x0000 },
+	{ LOCHNAGAR2_GPIO_DSP_DMICCLK1,     0x0000 },
+	{ LOCHNAGAR2_GPIO_DSP_DMICDAT1,     0x0000 },
+	{ LOCHNAGAR2_GPIO_DSP_DMICCLK2,     0x0000 },
+	{ LOCHNAGAR2_GPIO_DSP_DMICDAT2,     0x0000 },
+	{ LOCHNAGAR2_GPIO_I2C2_SCL,         0x0000 },
+	{ LOCHNAGAR2_GPIO_I2C2_SDA,         0x0000 },
+	{ LOCHNAGAR2_GPIO_I2C3_SCL,         0x0000 },
+	{ LOCHNAGAR2_GPIO_I2C3_SDA,         0x0000 },
+	{ LOCHNAGAR2_GPIO_I2C4_SCL,         0x0000 },
+	{ LOCHNAGAR2_GPIO_I2C4_SDA,         0x0000 },
+	{ LOCHNAGAR2_GPIO_DSP_STANDBY,      0x0000 },
+	{ LOCHNAGAR2_GPIO_CDC_MCLK1,        0x0000 },
+	{ LOCHNAGAR2_GPIO_CDC_MCLK2,        0x0000 },
+	{ LOCHNAGAR2_GPIO_DSP_CLKIN,        0x0000 },
+	{ LOCHNAGAR2_GPIO_PSIA1_MCLK,       0x0000 },
+	{ LOCHNAGAR2_GPIO_PSIA2_MCLK,       0x0000 },
+	{ LOCHNAGAR2_GPIO_GF_GPIO1,         0x0000 },
+	{ LOCHNAGAR2_GPIO_GF_GPIO5,         0x0000 },
+	{ LOCHNAGAR2_GPIO_DSP_GPIO20,       0x0000 },
+	{ LOCHNAGAR2_MINICARD_RESETS,       0x0000 },
+	{ LOCHNAGAR2_ANALOGUE_PATH_CTRL2,   0x0000 },
+	{ LOCHNAGAR2_COMMS_CTRL4,           0x0001 },
+	{ LOCHNAGAR2_SPDIF_CTRL,            0x0008 },
+	{ LOCHNAGAR2_POWER_CTRL,            0x0001 },
+	{ LOCHNAGAR2_SOUNDCARD_AIF_CTRL,    0x0000 },
+};
+
+static const struct regmap_config lochnagar2_i2c_regmap = {
+	.reg_bits = 16,
+	.val_bits = 16,
+	.reg_format_endian = REGMAP_ENDIAN_BIG,
+	.val_format_endian = REGMAP_ENDIAN_BIG,
+
+	.max_register = 0x1F1F,
+	.readable_reg = lochnagar2_readable_register,
+	.volatile_reg = lochnagar2_volatile_register,
+
+	.cache_type = REGCACHE_RBTREE,
+
+	.reg_defaults = lochnagar2_reg_defaults,
+	.num_reg_defaults = ARRAY_SIZE(lochnagar2_reg_defaults),
+};
+
+static const struct reg_sequence lochnagar2_patch[] = {
+	{ 0x00EE, 0x0000 },
+	{ 0x00F0, 0x0001 },
+};
+
+static struct mfd_cell lochnagar2_devs[] = {
+	{
+		.name = "lochnagar-pinctrl"
+	},
+	{
+		.name = "lochnagar-clk"
+	},
+	{
+		.name = "lochnagar-regulator"
+	},
+	{
+		.name = "lochnagar2-sound-card",
+	},
+};
+
+static struct lochnagar_config {
+	int id;
+	const char * const name;
+	enum lochnagar_type type;
+	const struct regmap_config *regmap;
+	struct mfd_cell *devs;
+	int ndevs;
+	const struct reg_sequence *patch;
+	int npatch;
+} lochnagar_configs[] = {
+	{
+		.id = 0x50,
+		.name = "lochnagar1",
+		.type = LOCHNAGAR1,
+		.regmap = &lochnagar1_i2c_regmap,
+		.devs = lochnagar1_devs,
+		.ndevs = ARRAY_SIZE(lochnagar1_devs),
+		.patch = lochnagar1_patch,
+		.npatch = ARRAY_SIZE(lochnagar1_patch),
+	},
+	{
+		.id = 0xCB58,
+		.name = "lochnagar2",
+		.type = LOCHNAGAR2,
+		.regmap = &lochnagar2_i2c_regmap,
+		.devs = lochnagar2_devs,
+		.ndevs = ARRAY_SIZE(lochnagar2_devs),
+		.patch = lochnagar2_patch,
+		.npatch = ARRAY_SIZE(lochnagar2_patch),
+	},
+};
+
+static const struct of_device_id lochnagar_of_match[] = {
+	{ .compatible = "cirrus,lochnagar1", .data = &lochnagar_configs[0] },
+	{ .compatible = "cirrus,lochnagar2", .data = &lochnagar_configs[1] },
+	{},
+};
+
+static int lochnagar_wait_for_boot(struct regmap *regmap, unsigned int *id)
+{
+	int i, ret;
+
+	for (i = 0; i < LOCHNAGAR_BOOT_RETRIES; ++i) {
+		msleep(LOCHNAGAR_BOOT_DELAY_MS);
+
+		ret = regmap_read(regmap, LOCHNAGAR_SOFTWARE_RESET, id);
+		if (!ret)
+			return ret;
+	}
+
+	return -ETIMEDOUT;
+}
+
+int lochnagar_update_config(struct lochnagar *lochnagar)
+{
+	struct regmap *regmap = lochnagar->regmap;
+	unsigned int done = LOCHNAGAR2_ANALOGUE_PATH_UPDATE_STS_MASK;
+	int timeout_ms = LOCHNAGAR_BOOT_DELAY_MS * LOCHNAGAR_BOOT_RETRIES;
+	unsigned int val = 0;
+	int ret;
+
+	switch (lochnagar->type) {
+	case LOCHNAGAR1:
+		return 0;
+	case LOCHNAGAR2:
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	ret = regmap_write(regmap, LOCHNAGAR2_ANALOGUE_PATH_CTRL1, 0);
+	if (ret < 0)
+		return ret;
+
+	ret = regmap_write(regmap, LOCHNAGAR2_ANALOGUE_PATH_CTRL1,
+			   LOCHNAGAR2_ANALOGUE_PATH_UPDATE_MASK);
+	if (ret < 0)
+		return ret;
+
+	ret = regmap_read_poll_timeout(regmap,
+				       LOCHNAGAR2_ANALOGUE_PATH_CTRL1, val,
+				       (val & done), LOCHNAGAR_CONFIG_POLL_US,
+				       timeout_ms * 1000);
+	if (ret < 0)
+		return ret;
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(lochnagar_update_config);
+
+static int lochnagar_i2c_probe(struct i2c_client *i2c)
+{
+	struct device *dev = &i2c->dev;
+	const struct lochnagar_config *config = NULL;
+	const struct of_device_id *of_id;
+	struct lochnagar *lochnagar;
+	unsigned int val;
+	struct gpio_desc *reset, *present;
+	unsigned int firmwareid;
+	int devid, rev;
+	int ret;
+
+	lochnagar = devm_kzalloc(dev, sizeof(*lochnagar), GFP_KERNEL);
+	if (!lochnagar)
+		return -ENOMEM;
+
+	of_id = of_match_device(lochnagar_of_match, dev);
+	if (!of_id)
+		return -EINVAL;
+
+	config = of_id->data;
+
+	lochnagar->dev = dev;
+	mutex_init(&lochnagar->analogue_config_lock);
+
+	dev_set_drvdata(dev, lochnagar);
+
+	reset = devm_gpiod_get(dev, "reset", GPIOD_OUT_LOW);
+	if (IS_ERR(reset)) {
+		ret = PTR_ERR(reset);
+		dev_err(dev, "Failed to get reset GPIO: %d\n", ret);
+		return ret;
+	}
+
+	present = devm_gpiod_get_optional(dev, "present", GPIOD_OUT_HIGH);
+	if (IS_ERR(present)) {
+		ret = PTR_ERR(present);
+		dev_err(dev, "Failed to get present GPIO: %d\n", ret);
+		return ret;
+	}
+
+	msleep(20);
+
+	/* Bring Lochnagar out of reset */
+	gpiod_set_value_cansleep(reset, 1);
+
+	/* Identify Lochnagar */
+	lochnagar->type = config->type;
+	lochnagar->regmap = devm_regmap_init_i2c(i2c, config->regmap);
+	if (IS_ERR(lochnagar->regmap)) {
+		ret = PTR_ERR(lochnagar->regmap);
+		dev_err(dev, "Failed to allocate register map: %d\n", ret);
+		return ret;
+	}
+
+	/* Wait for Lochnagar to boot */
+	ret = lochnagar_wait_for_boot(lochnagar->regmap, &val);
+	if (ret < 0) {
+		dev_err(dev, "Failed to read device ID: %d\n", ret);
+		return ret;
+	}
+
+	devid = val & LOCHNAGAR_DEVICE_ID_MASK;
+	rev = val & LOCHNAGAR_REV_ID_MASK;
+
+	if (devid != config->id) {
+		dev_err(dev,
+			"ID does not match %s (expected 0x%x got 0x%x)\n",
+			config->name, config->id, devid);
+		return -ENODEV;
+	}
+
+	/* Identify firmware */
+	ret = regmap_read(lochnagar->regmap, LOCHNAGAR_FIRMWARE_ID1, &val);
+	if (ret < 0) {
+		dev_err(dev, "Failed to read firmware id 1: %d\n", ret);
+		return ret;
+	}
+
+	firmwareid = val;
+
+	ret = regmap_read(lochnagar->regmap, LOCHNAGAR_FIRMWARE_ID2, &val);
+	if (ret < 0) {
+		dev_err(dev, "Failed to read firmware id 2: %d\n", ret);
+		return ret;
+	}
+
+	firmwareid |= (val << config->regmap->val_bits);
+
+	dev_info(dev, "Found %s (0x%x) revision %d firmware 0x%.6x\n",
+		 config->name, devid, rev + 1, firmwareid);
+
+	ret = regmap_register_patch(lochnagar->regmap, config->patch,
+				    config->npatch);
+	if (ret < 0) {
+		dev_err(dev, "Failed to register patch: %d\n", ret);
+		return ret;
+	}
+
+	ret = devm_mfd_add_devices(dev, PLATFORM_DEVID_NONE, config->devs,
+				   config->ndevs, NULL, 0, NULL);
+	if (ret != 0) {
+		dev_err(dev, "Failed to add subdevices: %d\n", ret);
+		return ret;
+	}
+
+	ret = devm_of_platform_populate(dev);
+	if (ret < 0) {
+		dev_err(dev, "Failed to populate child nodes: %d\n", ret);
+		return ret;
+	}
+
+	return ret;
+}
+
+static struct i2c_driver lochnagar_i2c_driver = {
+	.driver = {
+		.name = "lochnagar",
+		.of_match_table = of_match_ptr(lochnagar_of_match),
+		.suppress_bind_attrs = true,
+	},
+
+	.probe_new = lochnagar_i2c_probe,
+};
+
+static int __init lochnagar_i2c_init(void)
+{
+	int ret;
+
+	ret = i2c_add_driver(&lochnagar_i2c_driver);
+	if (ret != 0)
+		pr_err("Failed to register Lochnagar driver: %d\n", ret);
+
+	return ret;
+}
+subsys_initcall(lochnagar_i2c_init);
diff --git a/include/linux/mfd/lochnagar.h b/include/linux/mfd/lochnagar.h
new file mode 100644
index 0000000000000..be485d6714f7c
--- /dev/null
+++ b/include/linux/mfd/lochnagar.h
@@ -0,0 +1,48 @@ 
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Lochnagar internals
+ *
+ * Copyright (c) 2013-2018 Cirrus Logic, Inc. and
+ *                         Cirrus Logic International Semiconductor Ltd.
+ *
+ * Author: Charles Keepax <ckeepax@opensource.cirrus.com>
+ */
+
+#ifndef CIRRUS_LOCHNAGAR_H
+#define CIRRUS_LOCHNAGAR_H
+
+#include "lochnagar1_regs.h"
+#include "lochnagar2_regs.h"
+
+struct device;
+struct regmap;
+struct mutex;
+
+enum lochnagar_type {
+	LOCHNAGAR1,
+	LOCHNAGAR2,
+};
+
+struct lochnagar {
+	enum lochnagar_type type;
+	struct device *dev;
+	struct regmap *regmap;
+
+	/* Lock to protect updates to the analogue configuration */
+	struct mutex analogue_config_lock;
+};
+
+/* Register Addresses */
+#define LOCHNAGAR_SOFTWARE_RESET                             0x00
+#define LOCHNAGAR_FIRMWARE_ID1                               0x01
+#define LOCHNAGAR_FIRMWARE_ID2                               0x02
+
+/* (0x0000)  Software Reset */
+#define LOCHNAGAR_DEVICE_ID_MASK                           0xFFFC
+#define LOCHNAGAR_DEVICE_ID_SHIFT                               2
+#define LOCHNAGAR_REV_ID_MASK                              0x0003
+#define LOCHNAGAR_REV_ID_SHIFT                                  0
+
+int lochnagar_update_config(struct lochnagar *lochnagar);
+
+#endif
diff --git a/include/linux/mfd/lochnagar1_regs.h b/include/linux/mfd/lochnagar1_regs.h
new file mode 100644
index 0000000000000..114b846245d96
--- /dev/null
+++ b/include/linux/mfd/lochnagar1_regs.h
@@ -0,0 +1,157 @@ 
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Lochnagar1 register definitions
+ *
+ * Copyright (c) 2017-2018 Cirrus Logic, Inc. and
+ *                         Cirrus Logic International Semiconductor Ltd.
+ *
+ * Author: Charles Keepax <ckeepax@opensource.cirrus.com>
+ */
+
+#ifndef LOCHNAGAR1_REGISTERS_H
+#define LOCHNAGAR1_REGISTERS_H
+
+/* Register Addresses */
+#define LOCHNAGAR1_CDC_AIF1_SEL                       0x0008
+#define LOCHNAGAR1_CDC_AIF2_SEL                       0x0009
+#define LOCHNAGAR1_CDC_AIF3_SEL                       0x000A
+#define LOCHNAGAR1_CDC_MCLK1_SEL                      0x000B
+#define LOCHNAGAR1_CDC_MCLK2_SEL                      0x000C
+#define LOCHNAGAR1_CDC_AIF_CTRL1                      0x000D
+#define LOCHNAGAR1_CDC_AIF_CTRL2                      0x000E
+#define LOCHNAGAR1_EXT_AIF_CTRL                       0x000F
+#define LOCHNAGAR1_DSP_AIF1_SEL                       0x0010
+#define LOCHNAGAR1_DSP_AIF2_SEL                       0x0011
+#define LOCHNAGAR1_DSP_CLKIN_SEL                      0x0012
+#define LOCHNAGAR1_DSP_AIF                            0x0013
+#define LOCHNAGAR1_GF_AIF1                            0x0014
+#define LOCHNAGAR1_GF_AIF2                            0x0015
+#define LOCHNAGAR1_PSIA_AIF                           0x0016
+#define LOCHNAGAR1_PSIA1_SEL                          0x0017
+#define LOCHNAGAR1_PSIA2_SEL                          0x0018
+#define LOCHNAGAR1_SPDIF_AIF_SEL                      0x0019
+#define LOCHNAGAR1_GF_AIF3_SEL                        0x001C
+#define LOCHNAGAR1_GF_AIF4_SEL                        0x001D
+#define LOCHNAGAR1_GF_CLKOUT1_SEL                     0x001E
+#define LOCHNAGAR1_GF_AIF1_SEL                        0x001F
+#define LOCHNAGAR1_GF_AIF2_SEL                        0x0020
+#define LOCHNAGAR1_GF_GPIO2                           0x0026
+#define LOCHNAGAR1_GF_GPIO3                           0x0027
+#define LOCHNAGAR1_GF_GPIO7                           0x0028
+#define LOCHNAGAR1_RST                                0x0029
+#define LOCHNAGAR1_LED1                               0x002A
+#define LOCHNAGAR1_LED2                               0x002B
+#define LOCHNAGAR1_I2C_CTRL                           0x0046
+
+/*
+ * (0x0008 - 0x000C, 0x0010 - 0x0012, 0x0017 - 0x0020)
+ * CDC_AIF1_SEL - GF_AIF2_SEL
+ */
+#define LOCHNAGAR1_SRC_MASK                             0xFF
+#define LOCHNAGAR1_SRC_SHIFT                               0
+
+/* (0x000D)  CDC_AIF_CTRL1 */
+#define LOCHNAGAR1_CDC_AIF2_LRCLK_DIR_MASK              0x40
+#define LOCHNAGAR1_CDC_AIF2_LRCLK_DIR_SHIFT                6
+#define LOCHNAGAR1_CDC_AIF2_BCLK_DIR_MASK               0x20
+#define LOCHNAGAR1_CDC_AIF2_BCLK_DIR_SHIFT                 5
+#define LOCHNAGAR1_CDC_AIF2_ENA_MASK                    0x10
+#define LOCHNAGAR1_CDC_AIF2_ENA_SHIFT                      4
+#define LOCHNAGAR1_CDC_AIF1_LRCLK_DIR_MASK              0x04
+#define LOCHNAGAR1_CDC_AIF1_LRCLK_DIR_SHIFT                2
+#define LOCHNAGAR1_CDC_AIF1_BCLK_DIR_MASK               0x02
+#define LOCHNAGAR1_CDC_AIF1_BCLK_DIR_SHIFT                 1
+#define LOCHNAGAR1_CDC_AIF1_ENA_MASK                    0x01
+#define LOCHNAGAR1_CDC_AIF1_ENA_SHIFT                      0
+
+/* (0x000E)  CDC_AIF_CTRL2 */
+#define LOCHNAGAR1_CDC_AIF3_LRCLK_DIR_MASK              0x40
+#define LOCHNAGAR1_CDC_AIF3_LRCLK_DIR_SHIFT                6
+#define LOCHNAGAR1_CDC_AIF3_BCLK_DIR_MASK               0x20
+#define LOCHNAGAR1_CDC_AIF3_BCLK_DIR_SHIFT                 5
+#define LOCHNAGAR1_CDC_AIF3_ENA_MASK                    0x10
+#define LOCHNAGAR1_CDC_AIF3_ENA_SHIFT                      4
+#define LOCHNAGAR1_CDC_MCLK1_ENA_MASK                   0x02
+#define LOCHNAGAR1_CDC_MCLK1_ENA_SHIFT                     1
+#define LOCHNAGAR1_CDC_MCLK2_ENA_MASK                   0x01
+#define LOCHNAGAR1_CDC_MCLK2_ENA_SHIFT                     0
+
+/* (0x000F)  EXT_AIF_CTRL */
+#define LOCHNAGAR1_SPDIF_AIF_LRCLK_DIR_MASK             0x20
+#define LOCHNAGAR1_SPDIF_AIF_LRCLK_DIR_SHIFT               5
+#define LOCHNAGAR1_SPDIF_AIF_BCLK_DIR_MASK              0x10
+#define LOCHNAGAR1_SPDIF_AIF_BCLK_DIR_SHIFT                4
+#define LOCHNAGAR1_SPDIF_AIF_ENA_MASK                   0x08
+#define LOCHNAGAR1_SPDIF_AIF_ENA_SHIFT                     3
+
+/* (0x0013)  DSP_AIF */
+#define LOCHNAGAR1_DSP_AIF2_LRCLK_DIR_MASK              0x40
+#define LOCHNAGAR1_DSP_AIF2_LRCLK_DIR_SHIFT                6
+#define LOCHNAGAR1_DSP_AIF2_BCLK_DIR_MASK               0x20
+#define LOCHNAGAR1_DSP_AIF2_BCLK_DIR_SHIFT                 5
+#define LOCHNAGAR1_DSP_AIF2_ENA_MASK                    0x10
+#define LOCHNAGAR1_DSP_AIF2_ENA_SHIFT                      4
+#define LOCHNAGAR1_DSP_CLKIN_ENA_MASK                   0x08
+#define LOCHNAGAR1_DSP_CLKIN_ENA_SHIFT                     3
+#define LOCHNAGAR1_DSP_AIF1_LRCLK_DIR_MASK              0x04
+#define LOCHNAGAR1_DSP_AIF1_LRCLK_DIR_SHIFT                2
+#define LOCHNAGAR1_DSP_AIF1_BCLK_DIR_MASK               0x02
+#define LOCHNAGAR1_DSP_AIF1_BCLK_DIR_SHIFT                 1
+#define LOCHNAGAR1_DSP_AIF1_ENA_MASK                    0x01
+#define LOCHNAGAR1_DSP_AIF1_ENA_SHIFT                      0
+
+/* (0x0014)  GF_AIF1 */
+#define LOCHNAGAR1_GF_CLKOUT1_ENA_MASK                  0x40
+#define LOCHNAGAR1_GF_CLKOUT1_ENA_SHIFT                    6
+#define LOCHNAGAR1_GF_AIF3_LRCLK_DIR_MASK               0x20
+#define LOCHNAGAR1_GF_AIF3_LRCLK_DIR_SHIFT                 5
+#define LOCHNAGAR1_GF_AIF3_BCLK_DIR_MASK                0x10
+#define LOCHNAGAR1_GF_AIF3_BCLK_DIR_SHIFT                  4
+#define LOCHNAGAR1_GF_AIF3_ENA_MASK                     0x08
+#define LOCHNAGAR1_GF_AIF3_ENA_SHIFT                       3
+#define LOCHNAGAR1_GF_AIF1_LRCLK_DIR_MASK               0x04
+#define LOCHNAGAR1_GF_AIF1_LRCLK_DIR_SHIFT                 2
+#define LOCHNAGAR1_GF_AIF1_BCLK_DIR_MASK                0x02
+#define LOCHNAGAR1_GF_AIF1_BCLK_DIR_SHIFT                  1
+#define LOCHNAGAR1_GF_AIF1_ENA_MASK                     0x01
+#define LOCHNAGAR1_GF_AIF1_ENA_SHIFT                       0
+
+/* (0x0015)  GF_AIF2 */
+#define LOCHNAGAR1_GF_AIF4_LRCLK_DIR_MASK               0x20
+#define LOCHNAGAR1_GF_AIF4_LRCLK_DIR_SHIFT                 5
+#define LOCHNAGAR1_GF_AIF4_BCLK_DIR_MASK                0x10
+#define LOCHNAGAR1_GF_AIF4_BCLK_DIR_SHIFT                  4
+#define LOCHNAGAR1_GF_AIF4_ENA_MASK                     0x08
+#define LOCHNAGAR1_GF_AIF4_ENA_SHIFT                       3
+#define LOCHNAGAR1_GF_AIF2_LRCLK_DIR_MASK               0x04
+#define LOCHNAGAR1_GF_AIF2_LRCLK_DIR_SHIFT                 2
+#define LOCHNAGAR1_GF_AIF2_BCLK_DIR_MASK                0x02
+#define LOCHNAGAR1_GF_AIF2_BCLK_DIR_SHIFT                  1
+#define LOCHNAGAR1_GF_AIF2_ENA_MASK                     0x01
+#define LOCHNAGAR1_GF_AIF2_ENA_SHIFT                       0
+
+/* (0x0016)  PSIA_AIF */
+#define LOCHNAGAR1_PSIA2_LRCLK_DIR_MASK                 0x40
+#define LOCHNAGAR1_PSIA2_LRCLK_DIR_SHIFT                   6
+#define LOCHNAGAR1_PSIA2_BCLK_DIR_MASK                  0x20
+#define LOCHNAGAR1_PSIA2_BCLK_DIR_SHIFT                    5
+#define LOCHNAGAR1_PSIA2_ENA_MASK                       0x10
+#define LOCHNAGAR1_PSIA2_ENA_SHIFT                         4
+#define LOCHNAGAR1_PSIA1_LRCLK_DIR_MASK                 0x04
+#define LOCHNAGAR1_PSIA1_LRCLK_DIR_SHIFT                   2
+#define LOCHNAGAR1_PSIA1_BCLK_DIR_MASK                  0x02
+#define LOCHNAGAR1_PSIA1_BCLK_DIR_SHIFT                    1
+#define LOCHNAGAR1_PSIA1_ENA_MASK                       0x01
+#define LOCHNAGAR1_PSIA1_ENA_SHIFT                         0
+
+/* (0x0029)  RST */
+#define LOCHNAGAR1_DSP_RESET_MASK                       0x02
+#define LOCHNAGAR1_DSP_RESET_SHIFT                         1
+#define LOCHNAGAR1_CDC_RESET_MASK                       0x01
+#define LOCHNAGAR1_CDC_RESET_SHIFT                         0
+
+/* (0x0046)  I2C_CTRL */
+#define LOCHNAGAR1_CDC_CIF_MODE_MASK                    0x01
+#define LOCHNAGAR1_CDC_CIF_MODE_SHIFT                      0
+
+#endif
diff --git a/include/linux/mfd/lochnagar2_regs.h b/include/linux/mfd/lochnagar2_regs.h
new file mode 100644
index 0000000000000..2c6340c3006b7
--- /dev/null
+++ b/include/linux/mfd/lochnagar2_regs.h
@@ -0,0 +1,253 @@ 
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Lochnagar2 register definitions
+ *
+ * Copyright (c) 2017-2018 Cirrus Logic, Inc. and
+ *                         Cirrus Logic International Semiconductor Ltd.
+ *
+ * Author: Charles Keepax <ckeepax@opensource.cirrus.com>
+ */
+
+#ifndef LOCHNAGAR2_REGISTERS_H
+#define LOCHNAGAR2_REGISTERS_H
+
+/* Register Addresses */
+#define LOCHNAGAR2_CDC_AIF1_CTRL                      0x000D
+#define LOCHNAGAR2_CDC_AIF2_CTRL                      0x000E
+#define LOCHNAGAR2_CDC_AIF3_CTRL                      0x000F
+#define LOCHNAGAR2_DSP_AIF1_CTRL                      0x0010
+#define LOCHNAGAR2_DSP_AIF2_CTRL                      0x0011
+#define LOCHNAGAR2_PSIA1_CTRL                         0x0012
+#define LOCHNAGAR2_PSIA2_CTRL                         0x0013
+#define LOCHNAGAR2_GF_AIF3_CTRL                       0x0014
+#define LOCHNAGAR2_GF_AIF4_CTRL                       0x0015
+#define LOCHNAGAR2_GF_AIF1_CTRL                       0x0016
+#define LOCHNAGAR2_GF_AIF2_CTRL                       0x0017
+#define LOCHNAGAR2_SPDIF_AIF_CTRL                     0x0018
+#define LOCHNAGAR2_USB_AIF1_CTRL                      0x0019
+#define LOCHNAGAR2_USB_AIF2_CTRL                      0x001A
+#define LOCHNAGAR2_ADAT_AIF_CTRL                      0x001B
+#define LOCHNAGAR2_CDC_MCLK1_CTRL                     0x001E
+#define LOCHNAGAR2_CDC_MCLK2_CTRL                     0x001F
+#define LOCHNAGAR2_DSP_CLKIN_CTRL                     0x0020
+#define LOCHNAGAR2_PSIA1_MCLK_CTRL                    0x0021
+#define LOCHNAGAR2_PSIA2_MCLK_CTRL                    0x0022
+#define LOCHNAGAR2_SPDIF_MCLK_CTRL                    0x0023
+#define LOCHNAGAR2_GF_CLKOUT1_CTRL                    0x0024
+#define LOCHNAGAR2_GF_CLKOUT2_CTRL                    0x0025
+#define LOCHNAGAR2_ADAT_MCLK_CTRL                     0x0026
+#define LOCHNAGAR2_SOUNDCARD_MCLK_CTRL                0x0027
+#define LOCHNAGAR2_GPIO_FPGA_GPIO1                    0x0031
+#define LOCHNAGAR2_GPIO_FPGA_GPIO2                    0x0032
+#define LOCHNAGAR2_GPIO_FPGA_GPIO3                    0x0033
+#define LOCHNAGAR2_GPIO_FPGA_GPIO4                    0x0034
+#define LOCHNAGAR2_GPIO_FPGA_GPIO5                    0x0035
+#define LOCHNAGAR2_GPIO_FPGA_GPIO6                    0x0036
+#define LOCHNAGAR2_GPIO_CDC_GPIO1                     0x0037
+#define LOCHNAGAR2_GPIO_CDC_GPIO2                     0x0038
+#define LOCHNAGAR2_GPIO_CDC_GPIO3                     0x0039
+#define LOCHNAGAR2_GPIO_CDC_GPIO4                     0x003A
+#define LOCHNAGAR2_GPIO_CDC_GPIO5                     0x003B
+#define LOCHNAGAR2_GPIO_CDC_GPIO6                     0x003C
+#define LOCHNAGAR2_GPIO_CDC_GPIO7                     0x003D
+#define LOCHNAGAR2_GPIO_CDC_GPIO8                     0x003E
+#define LOCHNAGAR2_GPIO_DSP_GPIO1                     0x003F
+#define LOCHNAGAR2_GPIO_DSP_GPIO2                     0x0040
+#define LOCHNAGAR2_GPIO_DSP_GPIO3                     0x0041
+#define LOCHNAGAR2_GPIO_DSP_GPIO4                     0x0042
+#define LOCHNAGAR2_GPIO_DSP_GPIO5                     0x0043
+#define LOCHNAGAR2_GPIO_DSP_GPIO6                     0x0044
+#define LOCHNAGAR2_GPIO_GF_GPIO2                      0x0045
+#define LOCHNAGAR2_GPIO_GF_GPIO3                      0x0046
+#define LOCHNAGAR2_GPIO_GF_GPIO7                      0x0047
+#define LOCHNAGAR2_GPIO_CDC_AIF1_BCLK                 0x0048
+#define LOCHNAGAR2_GPIO_CDC_AIF1_RXDAT                0x0049
+#define LOCHNAGAR2_GPIO_CDC_AIF1_LRCLK                0x004A
+#define LOCHNAGAR2_GPIO_CDC_AIF1_TXDAT                0x004B
+#define LOCHNAGAR2_GPIO_CDC_AIF2_BCLK                 0x004C
+#define LOCHNAGAR2_GPIO_CDC_AIF2_RXDAT                0x004D
+#define LOCHNAGAR2_GPIO_CDC_AIF2_LRCLK                0x004E
+#define LOCHNAGAR2_GPIO_CDC_AIF2_TXDAT                0x004F
+#define LOCHNAGAR2_GPIO_CDC_AIF3_BCLK                 0x0050
+#define LOCHNAGAR2_GPIO_CDC_AIF3_RXDAT                0x0051
+#define LOCHNAGAR2_GPIO_CDC_AIF3_LRCLK                0x0052
+#define LOCHNAGAR2_GPIO_CDC_AIF3_TXDAT                0x0053
+#define LOCHNAGAR2_GPIO_DSP_AIF1_BCLK                 0x0054
+#define LOCHNAGAR2_GPIO_DSP_AIF1_RXDAT                0x0055
+#define LOCHNAGAR2_GPIO_DSP_AIF1_LRCLK                0x0056
+#define LOCHNAGAR2_GPIO_DSP_AIF1_TXDAT                0x0057
+#define LOCHNAGAR2_GPIO_DSP_AIF2_BCLK                 0x0058
+#define LOCHNAGAR2_GPIO_DSP_AIF2_RXDAT                0x0059
+#define LOCHNAGAR2_GPIO_DSP_AIF2_LRCLK                0x005A
+#define LOCHNAGAR2_GPIO_DSP_AIF2_TXDAT                0x005B
+#define LOCHNAGAR2_GPIO_PSIA1_BCLK                    0x005C
+#define LOCHNAGAR2_GPIO_PSIA1_RXDAT                   0x005D
+#define LOCHNAGAR2_GPIO_PSIA1_LRCLK                   0x005E
+#define LOCHNAGAR2_GPIO_PSIA1_TXDAT                   0x005F
+#define LOCHNAGAR2_GPIO_PSIA2_BCLK                    0x0060
+#define LOCHNAGAR2_GPIO_PSIA2_RXDAT                   0x0061
+#define LOCHNAGAR2_GPIO_PSIA2_LRCLK                   0x0062
+#define LOCHNAGAR2_GPIO_PSIA2_TXDAT                   0x0063
+#define LOCHNAGAR2_GPIO_GF_AIF3_BCLK                  0x0064
+#define LOCHNAGAR2_GPIO_GF_AIF3_RXDAT                 0x0065
+#define LOCHNAGAR2_GPIO_GF_AIF3_LRCLK                 0x0066
+#define LOCHNAGAR2_GPIO_GF_AIF3_TXDAT                 0x0067
+#define LOCHNAGAR2_GPIO_GF_AIF4_BCLK                  0x0068
+#define LOCHNAGAR2_GPIO_GF_AIF4_RXDAT                 0x0069
+#define LOCHNAGAR2_GPIO_GF_AIF4_LRCLK                 0x006A
+#define LOCHNAGAR2_GPIO_GF_AIF4_TXDAT                 0x006B
+#define LOCHNAGAR2_GPIO_GF_AIF1_BCLK                  0x006C
+#define LOCHNAGAR2_GPIO_GF_AIF1_RXDAT                 0x006D
+#define LOCHNAGAR2_GPIO_GF_AIF1_LRCLK                 0x006E
+#define LOCHNAGAR2_GPIO_GF_AIF1_TXDAT                 0x006F
+#define LOCHNAGAR2_GPIO_GF_AIF2_BCLK                  0x0070
+#define LOCHNAGAR2_GPIO_GF_AIF2_RXDAT                 0x0071
+#define LOCHNAGAR2_GPIO_GF_AIF2_LRCLK                 0x0072
+#define LOCHNAGAR2_GPIO_GF_AIF2_TXDAT                 0x0073
+#define LOCHNAGAR2_GPIO_DSP_UART1_RX                  0x0074
+#define LOCHNAGAR2_GPIO_DSP_UART1_TX                  0x0075
+#define LOCHNAGAR2_GPIO_DSP_UART2_RX                  0x0076
+#define LOCHNAGAR2_GPIO_DSP_UART2_TX                  0x0077
+#define LOCHNAGAR2_GPIO_GF_UART2_RX                   0x0078
+#define LOCHNAGAR2_GPIO_GF_UART2_TX                   0x0079
+#define LOCHNAGAR2_GPIO_USB_UART_RX                   0x007A
+#define LOCHNAGAR2_GPIO_CDC_PDMCLK1                   0x007C
+#define LOCHNAGAR2_GPIO_CDC_PDMDAT1                   0x007D
+#define LOCHNAGAR2_GPIO_CDC_PDMCLK2                   0x007E
+#define LOCHNAGAR2_GPIO_CDC_PDMDAT2                   0x007F
+#define LOCHNAGAR2_GPIO_CDC_DMICCLK1                  0x0080
+#define LOCHNAGAR2_GPIO_CDC_DMICDAT1                  0x0081
+#define LOCHNAGAR2_GPIO_CDC_DMICCLK2                  0x0082
+#define LOCHNAGAR2_GPIO_CDC_DMICDAT2                  0x0083
+#define LOCHNAGAR2_GPIO_CDC_DMICCLK3                  0x0084
+#define LOCHNAGAR2_GPIO_CDC_DMICDAT3                  0x0085
+#define LOCHNAGAR2_GPIO_CDC_DMICCLK4                  0x0086
+#define LOCHNAGAR2_GPIO_CDC_DMICDAT4                  0x0087
+#define LOCHNAGAR2_GPIO_DSP_DMICCLK1                  0x0088
+#define LOCHNAGAR2_GPIO_DSP_DMICDAT1                  0x0089
+#define LOCHNAGAR2_GPIO_DSP_DMICCLK2                  0x008A
+#define LOCHNAGAR2_GPIO_DSP_DMICDAT2                  0x008B
+#define LOCHNAGAR2_GPIO_I2C2_SCL                      0x008C
+#define LOCHNAGAR2_GPIO_I2C2_SDA                      0x008D
+#define LOCHNAGAR2_GPIO_I2C3_SCL                      0x008E
+#define LOCHNAGAR2_GPIO_I2C3_SDA                      0x008F
+#define LOCHNAGAR2_GPIO_I2C4_SCL                      0x0090
+#define LOCHNAGAR2_GPIO_I2C4_SDA                      0x0091
+#define LOCHNAGAR2_GPIO_DSP_STANDBY                   0x0092
+#define LOCHNAGAR2_GPIO_CDC_MCLK1                     0x0093
+#define LOCHNAGAR2_GPIO_CDC_MCLK2                     0x0094
+#define LOCHNAGAR2_GPIO_DSP_CLKIN                     0x0095
+#define LOCHNAGAR2_GPIO_PSIA1_MCLK                    0x0096
+#define LOCHNAGAR2_GPIO_PSIA2_MCLK                    0x0097
+#define LOCHNAGAR2_GPIO_GF_GPIO1                      0x0098
+#define LOCHNAGAR2_GPIO_GF_GPIO5                      0x0099
+#define LOCHNAGAR2_GPIO_DSP_GPIO20                    0x009A
+#define LOCHNAGAR2_GPIO_CHANNEL1                      0x00B9
+#define LOCHNAGAR2_GPIO_CHANNEL2                      0x00BA
+#define LOCHNAGAR2_GPIO_CHANNEL3                      0x00BB
+#define LOCHNAGAR2_GPIO_CHANNEL4                      0x00BC
+#define LOCHNAGAR2_GPIO_CHANNEL5                      0x00BD
+#define LOCHNAGAR2_GPIO_CHANNEL6                      0x00BE
+#define LOCHNAGAR2_GPIO_CHANNEL7                      0x00BF
+#define LOCHNAGAR2_GPIO_CHANNEL8                      0x00C0
+#define LOCHNAGAR2_GPIO_CHANNEL9                      0x00C1
+#define LOCHNAGAR2_GPIO_CHANNEL10                     0x00C2
+#define LOCHNAGAR2_GPIO_CHANNEL11                     0x00C3
+#define LOCHNAGAR2_GPIO_CHANNEL12                     0x00C4
+#define LOCHNAGAR2_GPIO_CHANNEL13                     0x00C5
+#define LOCHNAGAR2_GPIO_CHANNEL14                     0x00C6
+#define LOCHNAGAR2_GPIO_CHANNEL15                     0x00C7
+#define LOCHNAGAR2_GPIO_CHANNEL16                     0x00C8
+#define LOCHNAGAR2_MINICARD_RESETS                    0x00DF
+#define LOCHNAGAR2_ANALOGUE_PATH_CTRL1                0x00E3
+#define LOCHNAGAR2_ANALOGUE_PATH_CTRL2                0x00E4
+#define LOCHNAGAR2_COMMS_CTRL4                        0x00F0
+#define LOCHNAGAR2_SPDIF_CTRL                         0x00FE
+#define LOCHNAGAR2_POWER_CTRL                         0x0116
+#define LOCHNAGAR2_MICVDD_CTRL1                       0x0119
+#define LOCHNAGAR2_MICVDD_CTRL2                       0x011B
+#define LOCHNAGAR2_VDDCORE_CDC_CTRL1                  0x011E
+#define LOCHNAGAR2_VDDCORE_CDC_CTRL2                  0x0120
+#define LOCHNAGAR2_SOUNDCARD_AIF_CTRL                 0x0180
+
+/* (0x000D-0x001B, 0x0180)  CDC_AIF1_CTRL - SOUNCARD_AIF_CTRL */
+#define LOCHNAGAR2_AIF_ENA_MASK                       0x8000
+#define LOCHNAGAR2_AIF_ENA_SHIFT                          15
+#define LOCHNAGAR2_AIF_LRCLK_DIR_MASK                 0x4000
+#define LOCHNAGAR2_AIF_LRCLK_DIR_SHIFT                    14
+#define LOCHNAGAR2_AIF_BCLK_DIR_MASK                  0x2000
+#define LOCHNAGAR2_AIF_BCLK_DIR_SHIFT                     13
+#define LOCHNAGAR2_AIF_SRC_MASK                       0x00FF
+#define LOCHNAGAR2_AIF_SRC_SHIFT                           0
+
+/* (0x001E - 0x0027)  CDC_MCLK1_CTRL - SOUNDCARD_MCLK_CTRL */
+#define LOCHNAGAR2_CLK_ENA_MASK                       0x8000
+#define LOCHNAGAR2_CLK_ENA_SHIFT                          15
+#define LOCHNAGAR2_CLK_DIR_MASK                       0x4000
+#define LOCHNAGAR2_CLK_DIR_SHIFT                          14
+#define LOCHNAGAR2_CLK_SRC_MASK                       0x00FF
+#define LOCHNAGAR2_CLK_SRC_SHIFT                           0
+
+/* (0x0031 - 0x009A)  GPIO_FPGA_GPIO1 - GPIO_DSP_GPIO20 */
+#define LOCHNAGAR2_GPIO_SRC_MASK                      0x00FF
+#define LOCHNAGAR2_GPIO_SRC_SHIFT                          0
+
+/* (0x00B9 - 0x00C8)  GPIO_CHANNEL1 - GPIO_CHANNEL16 */
+#define LOCHNAGAR2_GPIO_CHANNEL_STS_MASK              0x8000
+#define LOCHNAGAR2_GPIO_CHANNEL_STS_SHIFT                 15
+#define LOCHNAGAR2_GPIO_CHANNEL_SRC_MASK              0x00FF
+#define LOCHNAGAR2_GPIO_CHANNEL_SRC_SHIFT                  0
+
+/* (0x00DF)  MINICARD_RESETS */
+#define LOCHNAGAR2_DSP_RESET_MASK                     0x0002
+#define LOCHNAGAR2_DSP_RESET_SHIFT                         1
+#define LOCHNAGAR2_CDC_RESET_MASK                     0x0001
+#define LOCHNAGAR2_CDC_RESET_SHIFT                         0
+
+/* (0x00E3)  ANALOGUE_PATH_CTRL1 */
+#define LOCHNAGAR2_ANALOGUE_PATH_UPDATE_MASK          0x8000
+#define LOCHNAGAR2_ANALOGUE_PATH_UPDATE_SHIFT             15
+#define LOCHNAGAR2_ANALOGUE_PATH_UPDATE_STS_MASK      0x4000
+#define LOCHNAGAR2_ANALOGUE_PATH_UPDATE_STS_SHIFT         14
+
+/* (0x00E4)  ANALOGUE_PATH_CTRL2 */
+#define LOCHNAGAR2_P2_INPUT_BIAS_ENA_MASK             0x0080
+#define LOCHNAGAR2_P2_INPUT_BIAS_ENA_SHIFT                 7
+#define LOCHNAGAR2_P1_INPUT_BIAS_ENA_MASK             0x0040
+#define LOCHNAGAR2_P1_INPUT_BIAS_ENA_SHIFT                 6
+#define LOCHNAGAR2_P2_MICBIAS_SRC_MASK                0x0038
+#define LOCHNAGAR2_P2_MICBIAS_SRC_SHIFT                    3
+#define LOCHNAGAR2_P1_MICBIAS_SRC_MASK                0x0007
+#define LOCHNAGAR2_P1_MICBIAS_SRC_SHIFT                    0
+
+/* (0x00F0)  COMMS_CTRL4 */
+#define LOCHNAGAR2_CDC_CIF1MODE_MASK                  0x0001
+#define LOCHNAGAR2_CDC_CIF1MODE_SHIFT                      0
+
+/* (0x00FE)  SPDIF_CTRL */
+#define LOCHNAGAR2_SPDIF_HWMODE_MASK                  0x0008
+#define LOCHNAGAR2_SPDIF_HWMODE_SHIFT                      3
+#define LOCHNAGAR2_SPDIF_RESET_MASK                   0x0001
+#define LOCHNAGAR2_SPDIF_RESET_SHIFT                       0
+
+/* (0x0116)  POWER_CTRL */
+#define LOCHNAGAR2_PWR_ENA_MASK                       0x0001
+#define LOCHNAGAR2_PWR_ENA_SHIFT                           0
+
+/* (0x0119)  MICVDD_CTRL1 */
+#define LOCHNAGAR2_MICVDD_REG_ENA_MASK                0x8000
+#define LOCHNAGAR2_MICVDD_REG_ENA_SHIFT                   15
+
+/* (0x011B)  MICVDD_CTRL2 */
+#define LOCHNAGAR2_MICVDD_VSEL_MASK                   0x001F
+#define LOCHNAGAR2_MICVDD_VSEL_SHIFT                       0
+
+/* (0x011E)  VDDCORE_CDC_CTRL1 */
+#define LOCHNAGAR2_VDDCORE_CDC_REG_ENA_MASK           0x8000
+#define LOCHNAGAR2_VDDCORE_CDC_REG_ENA_SHIFT              15
+
+/* (0x0120)  VDDCORE_CDC_CTRL2 */
+#define LOCHNAGAR2_VDDCORE_CDC_VSEL_MASK              0x007F
+#define LOCHNAGAR2_VDDCORE_CDC_VSEL_SHIFT                  0
+
+#endif