| Message ID | 20181108101405.14062-1-ckeepax@opensource.cirrus.com |
|---|---|
| Headers | show |
| Series | Lochnagar Driver | expand |
On Thu, Nov 08, 2018 at 10:14:01AM +0000, Charles Keepax wrote: > +static const struct of_device_id lochnagar_of_match[] = { > + { .compatible = "cirrus,lochnagar-regulator" }, > + {}, > +}; This is obviously just dumping the Linux driver model into the DT, the regulators are clearly different. If you want to explicitly list the regulators in DT it'd be better to reflect the hardware and enumerate the regulators individually - you can use one driver for all the MICVDDs and a separate one for VDDCORE for example.
On Thu, Nov 08, 2018 at 12:16:29PM +0000, Mark Brown wrote: > On Thu, Nov 08, 2018 at 10:14:01AM +0000, Charles Keepax wrote: > > > +static const struct of_device_id lochnagar_of_match[] = { > > + { .compatible = "cirrus,lochnagar-regulator" }, > > + {}, > > +}; > > This is obviously just dumping the Linux driver model into the DT, the > regulators are clearly different. If you want to explicitly list the > regulators in DT it'd be better to reflect the hardware and enumerate > the regulators individually - you can use one driver for all the MICVDDs > and a separate one for VDDCORE for example. Agreed, would be relatively doable for regulators, might be a bit verbose for the clock, but I really don't see how to handle pinctrl. I guess I will wait and see what the other maintainers think but personally I would really be inclined to keep the binding the same as the last version if we can. Thanks, Charles
On Thu, 08 Nov 2018, Charles Keepax wrote: > 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.cirrus.com> > --- > > Changes since v3: > - Bind in child drivers through device tree rather than using mfd_add_devices > - Remove defaults arrays for regmap > - Use a lot of ... in the regmap readable/volatile callbacks > - Explicitly include register headers, rather than relying on lochnagar.h > - Add some kernel doc > - Some minor cosmetic fixups > - Add a lockdep_assert in lochnagar_update_config > > Thanks, > Charles > > MAINTAINERS | 17 ++ > drivers/mfd/Kconfig | 8 + > drivers/mfd/Makefile | 2 + > drivers/mfd/lochnagar-i2c.c | 394 ++++++++++++++++++++++++++++++++++++ > include/linux/mfd/lochnagar.h | 55 +++++ > include/linux/mfd/lochnagar1_regs.h | 157 ++++++++++++++ > include/linux/mfd/lochnagar2_regs.h | 253 +++++++++++++++++++++++ > 7 files changed, 886 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 f4855974f3250..0398c8752e610 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -3567,6 +3567,23 @@ 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* > +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* > +F: Documentation/devicetree/bindings/mfd/cirrus,lochnagar.txt > +F: Documentation/devicetree/bindings/clock/cirrus,lochnagar.txt > +F: Documentation/devicetree/bindings/pinctrl/cirrus,lochnagar.txt > +F: Documentation/devicetree/bindings/regulator/cirrus,lochnagar.txt > + > 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 8c5dfdce4326c..51de2db3f6537 100644 > --- a/drivers/mfd/Kconfig > +++ b/drivers/mfd/Kconfig > @@ -1685,6 +1685,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 12980a4ad4608..93284316d5307 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..133159d7646ed > --- /dev/null > +++ b/drivers/mfd/lochnagar-i2c.c > @@ -0,0 +1,394 @@ > +// 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/lockdep.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> > +#include <linux/mfd/lochnagar1_regs.h> > +#include <linux/mfd/lochnagar2_regs.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...LOCHNAGAR_FIRMWARE_ID2: > + case LOCHNAGAR1_CDC_AIF1_SEL...LOCHNAGAR1_CDC_AIF3_SEL: > + case LOCHNAGAR1_CDC_MCLK1_SEL...LOCHNAGAR1_CDC_MCLK2_SEL: > + case LOCHNAGAR1_CDC_AIF_CTRL1...LOCHNAGAR1_CDC_AIF_CTRL2: > + case LOCHNAGAR1_EXT_AIF_CTRL: > + case LOCHNAGAR1_DSP_AIF1_SEL...LOCHNAGAR1_DSP_AIF2_SEL: > + case LOCHNAGAR1_DSP_CLKIN_SEL: > + case LOCHNAGAR1_DSP_AIF: > + case LOCHNAGAR1_GF_AIF1...LOCHNAGAR1_GF_AIF2: > + case LOCHNAGAR1_PSIA_AIF: > + case LOCHNAGAR1_PSIA1_SEL...LOCHNAGAR1_PSIA2_SEL: > + case LOCHNAGAR1_SPDIF_AIF_SEL: > + case LOCHNAGAR1_GF_AIF3_SEL...LOCHNAGAR1_GF_AIF4_SEL: > + case LOCHNAGAR1_GF_CLKOUT1_SEL: > + case LOCHNAGAR1_GF_AIF1_SEL...LOCHNAGAR1_GF_AIF2_SEL: > + case LOCHNAGAR1_GF_GPIO2...LOCHNAGAR1_GF_GPIO7: > + case LOCHNAGAR1_RST: > + case LOCHNAGAR1_LED1...LOCHNAGAR1_LED2: > + case LOCHNAGAR1_I2C_CTRL: > + return true; > + default: > + return false; > + } > +} Niiiiiice! Driver is much better for me to swallow like this. For my own reference: Acked-for-MFD-by: Lee Jones <lee.jones@linaro.org>
On Thu, Nov 8, 2018 at 11:14 AM Charles Keepax <ckeepax@opensource.cirrus.com> wrote: > 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. > > Lochnagar provides many pins which can generally be used for an > audio function such as an AIF or a PDM interface, but also as > GPIOs. > > Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com> This v4 version looks very good to me! Reviewed-by: Linus Walleij <linus.walleij@linaro.org> Nitpicks (fix or not, my review tag stands): > drivers/pinctrl/cirrus/pinctrl-lochnagar.c | 1145 ++++++++++++++++++++++++++++ > drivers/pinctrl/cirrus/pinctrl-lochnagar.h | 112 +++ Why don't you just include that header file in the C file. 112 lines +/- on a 1145 line file doesn't matter. > + priv->gpio_chip = template_chip; I would just skip the template and just assign struct members directly. Bonus if you set ->label to something instance-unique as well. Yours, Linus Walleij
On Thu, Nov 15, 2018 at 11:54:48AM +0100, Linus Walleij wrote: > On Thu, Nov 8, 2018 at 11:14 AM Charles Keepax > <ckeepax@opensource.cirrus.com> wrote: > > > 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. > > > > Lochnagar provides many pins which can generally be used for an > > audio function such as an AIF or a PDM interface, but also as > > GPIOs. > > > > Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com> > > This v4 version looks very good to me! > Reviewed-by: Linus Walleij <linus.walleij@linaro.org> > > Nitpicks (fix or not, my review tag stands): > > > drivers/pinctrl/cirrus/pinctrl-lochnagar.c | 1145 ++++++++++++++++++++++++++++ > > drivers/pinctrl/cirrus/pinctrl-lochnagar.h | 112 +++ > > Why don't you just include that header file in the C file. > 112 lines +/- on a 1145 line file doesn't matter. > > > + priv->gpio_chip = template_chip; > > I would just skip the template and just assign struct members > directly. > > Bonus if you set ->label to something instance-unique as well. > No objection to any of those need to respin for Mark's comments on the regulators anyway, so will wrap these in and add your reviewed by. Thanks, Charles