| 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 |
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?
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
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 >
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.
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
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.
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
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
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. >
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 >
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.
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.
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.
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.
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>
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.
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. >
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.
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.
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
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>
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.
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.
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 --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