Add support for Cirrus Logic CS47L35/L85/L90/L91 codecs

This patch adds a header file of register definitions for Cirrus Logic "Madera" class codecs. These codecs are all based off a common set of hardware IP so have a common register map (with a few minor device-to-device variations).

The registers.h file is tool-generated directly from the hardware design but has been manually stripped down to reduce size (full register map is >44000 lines). All names are kept the same as datasheet names so that they can be cross-referenced between source and datasheet without confusion.

The register map layout is kept fully-defined rather than factored into macros and/or block-indexing code. The major reasons for this are:

- #1 is that it makes the source highly greppable, which is important. "What does the driver do with register bits XYZ" or "Where does it use register bits XYZ" are commonly types of questions. These can be quickly answered by a grep. Squashing definitions into generator macros or block- indexing code is a way of defeating grep.

- most of the register definitions are used in tables, so a constant value is required. Using generator macros make the table definition clunky and obscure.

- the code is clearer when it's there in the source exactly what register and field it is using

- it is easier to diff the register map of a new (unsupported) codec against what is already supported and merge in differences

- it makes the register map available in source for maintenance/debugging instead of having to refer back to the datasheet for a register map

Quoted 24 lines · Richard Fitzgerald

This patch adds a header file of register definitions for Cirrus Logic "Madera" class codecs. These codecs are all based off a common set of hardware IP so have a common register map (with a few minor device-to-device variations).

The registers.h file is tool-generated directly from the hardware design but has been manually stripped down to reduce size (full register map is >44000 lines). All names are kept the same as datasheet names so that they can be cross-referenced between source and datasheet without confusion.

The register map layout is kept fully-defined rather than factored into macros and/or block-indexing code. The major reasons for this are:

- #1 is that it makes the source highly greppable, which is important. "What does the driver do with register bits XYZ" or "Where does it use register bits XYZ" are commonly types of questions. These can be quickly answered by a grep. Squashing definitions into generator macros or block- indexing code is a way of defeating grep.

- most of the register definitions are used in tables, so a constant value is required. Using generator macros make the table definition clunky and obscure.

Quoted 8 lines

- the code is clearer when it's there in the source exactly what register and field it is using

- it is easier to diff the register map of a new (unsupported) codec against what is already supported and merge in differences

- it makes the register map available in source for maintenance/debugging instead of having to refer back to the datasheet for a register map

...

include/linux/mfd/madera/registers.h line 3913

Quoted 3 lines

include/linux/mfd/madera/registers.h new file +2 -0

+#define MADERA_OTP_HPDET_GRADIENT_1X_MASK          0x0000FF00
 
+#define MADERA_OTP_HPDET_GRADIENT_0X_MASK          0x000000FF

GENMASK() for all masks?

drivers/mfd/madera-core.c line 1

Quoted 4 lines · Richard Fitzgerald

drivers/mfd/madera-core.c new file +1 -0

@@ -0,0 +1,602 @@
+// SPDX-License-Identifier: GPL-2.0

As you use SPDX line here....

drivers/mfd/madera-core.c line 9

Quoted 8 lines

drivers/mfd/madera-core.c new file +8 -0

+/*
+ * Core MFD support for Cirrus Logic Madera codecs
+ *
+ * Copyright (C) 2015-2018 Cirrus Logic
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by the
+ * Free Software Foundation; version 2.

You can remove this paragraph.

Same applies to the other places of this series.

This adds the generic core support for Cirrus Logic "Madera" class codecs. These are complex audio codec SoCs with a variety of digital and analogue I/O, onboard audio processing and DSPs, and other features.

These codecs are all based off a common set of hardware IP so can be supported by a core of common code (with a few minor device-to-device variations).

+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by the
+ * Free Software Foundation; version 2.

+ dev_set_drvdata(madera->dev, madera);

+ if (dev_get_platdata(madera->dev))

+ * @reset: GPIO controlling /RESET (0 = none)

Quoted 7 lines · Richard Fitzgerald

This adds the generic core support for Cirrus Logic "Madera" class codecs. These are complex audio codec SoCs with a variety of digital and analogue I/O, onboard audio processing and DSPs, and other features.

These codecs are all based off a common set of hardware IP so can be supported by a core of common code (with a few minor device-to-device variations).

Quoted 4 lines

+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by the
+ * Free Software Foundation; version 2.

This is redundant.

drivers/mfd/madera-core.c line 188

Quoted 3 lines

drivers/mfd/madera-core.c new file +3 -0

+static void madera_enable_hard_reset(struct madera *madera)
+{
+       if (madera->reset_gpio)

if (!...)
return

drivers/mfd/madera-core.c line 194

Quoted 6 lines

drivers/mfd/madera-core.c new file +6 -0

+               gpiod_set_value_cansleep(madera->reset_gpio, 0);
+}
+
+static void madera_disable_hard_reset(struct madera *madera)
+{
+       if (madera->reset_gpio) {

Ditto.

drivers/mfd/madera-core.c line 200

Quoted 7 lines

drivers/mfd/madera-core.c new file +6 -0

+               gpiod_set_value_cansleep(madera->reset_gpio, 1);
+               usleep_range(1000, 2000);
+       }
+}
+
 
+#ifdef CONFIG_PM

__maybe_unused

drivers/mfd/madera-core.c line 264

Quoted 5 lines

drivers/mfd/madera-core.c new file +5 -0

+const struct dev_pm_ops madera_pm_ops = {
+       SET_RUNTIME_PM_OPS(madera_runtime_suspend,
+                          madera_runtime_resume,
+                          NULL)
+};

There is a macro helper for this I believe.

Quoted 7 lines

drivers/mfd/madera-core.c new file +6 -0

+const struct of_device_id madera_of_match[] = {
+       { .compatible = "cirrus,cs47l35", .data = (void *)CS47L35 },
+       { .compatible = "cirrus,cs47l85", .data = (void *)CS47L85 },
+       { .compatible = "cirrus,cs47l90", .data = (void *)CS47L90 },
+       { .compatible = "cirrus,cs47l91", .data = (void *)CS47L91 },
+       { .compatible = "cirrus,wm1840", .data = (void *)WM1840 },

Quoted 1 line

+ {},

No comma.

drivers/mfd/madera-core.c line 289

Quoted 9 lines

drivers/mfd/madera-core.c new file +7 -0

+};
 
 
+               ret = devm_gpio_request_one(madera->dev,
+                                           madera->pdata.reset,
+                                           GPIOF_DIR_OUT | GPIOF_INIT_LOW,
+                                           "madera reset");
+               if (!ret)
+                       madera->reset_gpio = gpio_to_desc(madera->pdata.reset);

Why? What's wrong with descriptors?

Quoted 1 line

+ dev_set_drvdata(madera->dev, madera);

...

Quoted 1 line

+ if (dev_get_platdata(madera->dev))

What this dance for?

drivers/mfd/madera-core.c line 552

Quoted 3 lines

drivers/mfd/madera-core.c new file +3 -0

+       ret = mfd_add_devices(madera->dev, PLATFORM_DEVID_NONE,
+                             mfd_devs, n_devs,
+                             NULL, 0, NULL);

devm_?

drivers/mfd/madera-i2c.c

Quoted 12 lines

drivers/mfd/madera-i2c.c new file +11 -0

+       if (i2c->dev.of_node) {
+               of_id = of_match_device(madera_of_match, &i2c->dev);
+               if (of_id)
+                       type = (unsigned long)of_id->data;
+       } else {
+               type = id->driver_data;
+       }
 
+       if (spi->dev.of_node) {
+               of_id = of_match_device(madera_of_match, &spi->dev);
+               if (of_id)
+                       type = (unsigned long)of_id->data;

There is a helper to get match data.

Quoted 3 lines

drivers/mfd/madera-i2c.c new file +3 -0

+       } else {
+               type = id->driver_data;
+       }

include/linux/mfd/madera/pdata.h line 27

Quoted 2 lines

include/linux/mfd/madera/pdata.h new file +2 -0

+struct madera_irqchip_pdata;
+struct madera_codec_pdata;

Why do you need platform data in new code?

Quoted 1 line

+ * @reset: GPIO controlling /RESET (0 = none)

Shouldn't be descriptor?

Quoted 11 lines · Andy Shevchenko

drivers/mfd/madera-core.c line 188

Quoted 3 lines · Richard Fitzgerald

drivers/mfd/madera-core.c new file +3 -0

+static void madera_enable_hard_reset(struct madera *madera)
+{
+       if (madera->reset_gpio)

if (!...)
return

Could do but why bother? For such a trivial function, in my opinion

static void madera_enable_hard_reset(struct madera *madera)

{
        if (madera->reset_gpio)
                gpiod_set_value_cansleep(madera->reset_gpio, 0);
}

is simpler and more readable than

static void madera_enable_hard_reset(struct madera *madera)

{
        if (!madera->reset_gpio)
                return;

        gpiod_set_value_cansleep(madera->reset_gpio, 0);
}

Quoted 11 lines

drivers/mfd/madera-core.c line 194

Quoted 7 lines

drivers/mfd/madera-core.c new file +6 -0

+               gpiod_set_value_cansleep(madera->reset_gpio, 0);
+}
+
+static void madera_disable_hard_reset(struct madera *madera)
+{
+       if (madera->reset_gpio) {

Ditto.

As above, yes it would work the other way but I think for such a simple implementation the way I have written it is more readable.

Quoted 13 lines

Quoted 11 lines

drivers/mfd/madera-core.c new file +10 -0

+               gpiod_set_value_cansleep(madera->reset_gpio, 1);
+               usleep_range(1000, 2000);
+       }
+}
 
+const struct of_device_id madera_of_match[] = {
+       { .compatible = "cirrus,cs47l35", .data = (void *)CS47L35 },
+       { .compatible = "cirrus,cs47l85", .data = (void *)CS47L85 },
+       { .compatible = "cirrus,cs47l90", .data = (void *)CS47L90 },
+       { .compatible = "cirrus,cs47l91", .data = (void *)CS47L91 },
+       { .compatible = "cirrus,wm1840", .data = (void *)WM1840 },

Quoted 5 lines

Quoted 1 line

+ {},

No comma.

Seems to be personal preference. Both ways are used in the kernel and we've always used this style so I'll leave it to Lee to decide.

Quoted 13 lines

Quoted 11 lines

drivers/mfd/madera-core.c new file +4 -0

+};
 
+               ret = devm_gpio_request_one(madera->dev,
+                                           madera->pdata.reset,
+                                           GPIOF_DIR_OUT |

GPIOF_INIT_LOW,

drivers/mfd/madera-core.c new file +3 -0

+                                           "madera reset");
+               if (!ret)
+                       madera->reset_gpio =

gpio_to_desc(madera->pdata.reset);

Quoted 1 line

Why? What's wrong with descriptors?

This is what I mean by code going stale when it's acked but then never gets merged. Some time ago there was a reason (which I forget).

Quoted 6 lines

Review

Quoted 2 lines

(quoted diff) +2 -0

+       dev_set_drvdata(madera->dev, madera);
+       if (dev_get_platdata(madera->dev))

What this dance for?

Are you perhaps thinking the second line is dev_get_drvdata()? dev_get_platdata() gets a pointer to any pdata, so not related to dev_set_drvdata().

Quoted 7 lines

drivers/mfd/madera-core.c line 552

Quoted 3 lines

drivers/mfd/madera-core.c new file +3 -0

+       ret = mfd_add_devices(madera->dev, PLATFORM_DEVID_NONE,
+                             mfd_devs, n_devs,
+                             NULL, 0, NULL);

devm_?

I can try it and see. It's scary because we can depend on our children but maybe devm_mfd_add_devices() is safe.

Quoted 4 lines

include/linux/mfd/madera/pdata.h line 27

Quoted 2 lines

include/linux/mfd/madera/pdata.h new file +2 -0

+struct madera_irqchip_pdata;
+struct madera_codec_pdata;

Why do you need platform data in new code?

Answered in a comment in another patch. We care about allowing people to use our chips with systems that don't use devicetree/acpi. There are also many out-of-tree systems.

Quoted 28 lines · Richard Fitzgerald

Quoted 11 lines · Andy Shevchenko

drivers/mfd/madera-core.c line 188

Quoted 3 lines · Richard Fitzgerald

drivers/mfd/madera-core.c new file +3 -0

+static void madera_enable_hard_reset(struct madera *madera)
+{
+       if (madera->reset_gpio)

if (!...)
return

Could do but why bother? For such a trivial function, in my opinion

static void madera_enable_hard_reset(struct madera *madera)

{
         if (madera->reset_gpio)
                 gpiod_set_value_cansleep(madera->reset_gpio, 0);
}

is simpler and more readable than

static void madera_enable_hard_reset(struct madera *madera)

{
         if (!madera->reset_gpio)
                 return;

Quoted 2 lines

         gpiod_set_value_cansleep(madera->reset_gpio, 0);
}

The rationale is that if someone wants to add more code you will not need to take care of deeper indentation and potentially split lines.

Quoted 12 lines

Quoted 11 lines

drivers/mfd/madera-core.c line 194

Quoted 7 lines

drivers/mfd/madera-core.c new file +6 -0

+               gpiod_set_value_cansleep(madera->reset_gpio, 0);
+}
+
+static void madera_disable_hard_reset(struct madera *madera)
+{
+       if (madera->reset_gpio) {

Ditto.

Quoted 2 lines

As above, yes it would work the other way but I think for such a simple implementation the way I have written it is more readable.

I have different opinion, but yes. It's more matter of taste with rationale above (perhaps never happen to this code).

Quoted 19 lines

Quoted 18 lines

Quoted 4 lines

drivers/mfd/madera-core.c new file +4 -0

+               gpiod_set_value_cansleep(madera->reset_gpio, 1);
+               usleep_range(1000, 2000);
+       }
+}

drivers/mfd/madera-core.c line 273

Quoted 10 lines

drivers/mfd/madera-core.c new file +7 -0

 
+const struct of_device_id madera_of_match[] = {
+       { .compatible = "cirrus,cs47l35", .data = (void *)CS47L35 },
+       { .compatible = "cirrus,cs47l85", .data = (void *)CS47L85 },
+       { .compatible = "cirrus,cs47l90", .data = (void *)CS47L90 },
+       { .compatible = "cirrus,cs47l91", .data = (void *)CS47L91 },
+       { .compatible = "cirrus,wm1840", .data = (void *)WM1840 },
 
 
+       {},

No comma.

Quoted 2 lines

Seems to be personal preference. Both ways are used in the kernel and we've always used this style so I'll leave it to Lee to decide.

This is not.
The rationale is pretty obvious, terminator must terminate. With cheap price (no comma), we just prevent some potential weird cases (bad patch application for example, or not very careful contributor) where entry goes after. Compiler will fail.

Quoted 13 lines

Quoted 13 lines

Quoted 11 lines

drivers/mfd/madera-core.c new file +4 -0

+};
 
+               ret = devm_gpio_request_one(madera->dev,
+                                           madera->pdata.reset,
+                                           GPIOF_DIR_OUT |

GPIOF_INIT_LOW,

drivers/mfd/madera-core.c new file +3 -0

+                                           "madera reset");
+               if (!ret)
+                       madera->reset_gpio =

gpio_to_desc(madera->pdata.reset);

Quoted 4 lines

Quoted 1 line

Why? What's wrong with descriptors?

This is what I mean by code going stale when it's acked but then never gets merged. Some time ago there was a reason (which I forget).

So, can we switch to descriptors?

Quoted 10 lines

Quoted 6 lines

Review

Quoted 2 lines

(quoted diff) +2 -0

+       dev_set_drvdata(madera->dev, madera);
+       if (dev_get_platdata(madera->dev))

What this dance for?

Are you perhaps thinking the second line is dev_get_drvdata()? dev_get_platdata() gets a pointer to any pdata, so not related to dev_set_drvdata().

Indeed.

Quoted 10 lines

Quoted 7 lines

drivers/mfd/madera-core.c line 552

Quoted 3 lines

drivers/mfd/madera-core.c new file +3 -0

+       ret = mfd_add_devices(madera->dev, PLATFORM_DEVID_NONE,
+                             mfd_devs, n_devs,
+                             NULL, 0, NULL);

devm_?

I can try it and see. It's scary because we can depend on our children but maybe devm_mfd_add_devices() is safe.

It will fail in the same way. It does nothing more, than keeping a pointer to release function and its data.

Quoted 4 lines

Quoted 4 lines

include/linux/mfd/madera/pdata.h line 27

Quoted 2 lines

include/linux/mfd/madera/pdata.h new file +2 -0

+struct madera_irqchip_pdata;
+struct madera_codec_pdata;

Why do you need platform data in new code?

Quoted 3 lines

Answered in a comment in another patch. We care about allowing people to use our chips with systems that don't use devicetree/acpi. There are also many out-of-tree systems.

a) we don't care about out of tree much;

b) there are other means to provide date w/o using platform data:
- unified device property API (including built-in device properties)
- bunch of lookup tables GPIO, regulator, PWM, etc
- fwnode graph for more complex cases with device dependencies

This is the binding description of the pinctrl driver for Cirrus Logic Madera codecs. The binding uses the generic pinctrl binding so the main purpose here is to describe the device-specific names for groups and functions.

Signed-off-by Richard Fitzgerald <[email protected]>

Acked-by Rob Herring <[email protected]>

These codecs have a variable number of I/O lines each of which is individually selectable to a wide range of possible functions.

The functionality is slightly different from the traditional muxed GPIO since most of the functions can be mapped to any pin (and even the same function to multiple pins). Most pins have a dedicated "alternate" function that is only available on that pin. The alternate functions are usually a group of signals, though it is not always necessary to enable the full group, depending on the alternate function and how it is to be used. The mapping between alternate functions and GPIO pins varies between codecs depending on the number of alternate functions and available pins.

Signed-off-by Richard Fitzgerald <[email protected]>

Reviewed-by Linus Walleij <[email protected]>

---
There are some minor changes since LinusW last reviewed this patch but as they are trivial I have carried forward Linus's Reviewed-by:
- SPDX license headers
- can now build it as a module
- avoided a minor checkpatch warning about an unnecessary else {} in madera_get_group_name()

This adds support for the GPIOs on Cirrus Logic Madera class codecs. Any pins not used for special functions (see the pinctrl driver) can be used as general single-bit input or output lines. The number of available GPIOs varies between codecs.

Note that this is part of a composite MFD for these codecs and can only be used with the corresponding MFD and other child drivers on those silicon. The GPIO block on these codecs does not exist indepedently of the rest of the MFD.

Quoted 9 lines · Richard Fitzgerald

This adds support for the GPIOs on Cirrus Logic Madera class codecs. Any pins not used for special functions (see the pinctrl driver) can be used as general single-bit input or output lines. The number of available GPIOs varies between codecs.

Note that this is part of a composite MFD for these codecs and can only be used with the corresponding MFD and other child drivers on those silicon. The GPIO block on these codecs does not exist indepedently of the rest of the MFD.

drivers/gpio/gpio-madera.c line 24

Quoted 4 lines

drivers/gpio/gpio-madera.c new file +4 -0

+struct madera_gpio {
+       struct madera *madera;
+       struct gpio_chip gpio_chip;
+};

Why do you need this? I suppose one embeds or refers to the other.