mfd: cs42l43: Correct SoundWire port list
Commit Message
Two ports are missing from the port list. Also the attempt to list
them by function is rather misguided, there is nothing in the hardware
that fixes a particular port to one function. Fix the missing ports
and change to just listing them by the DPx naming scheme that is used
throughout the rest of the driver.
Fixes: ace6d1448138 ("mfd: cs42l43: Add support for cs42l43 core driver")
Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com>
---
drivers/mfd/cs42l43-sdw.c | 25 ++++++++++++-------------
1 file changed, 12 insertions(+), 13 deletions(-)
Comments
On Mon, 13 Nov 2023, Charles Keepax wrote:
> Two ports are missing from the port list. Also the attempt to list
> them by function is rather misguided, there is nothing in the hardware
> that fixes a particular port to one function. Fix the missing ports
> and change to just listing them by the DPx naming scheme that is used
> throughout the rest of the driver.
>
> Fixes: ace6d1448138 ("mfd: cs42l43: Add support for cs42l43 core driver")
> Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com>
> ---
> drivers/mfd/cs42l43-sdw.c | 25 ++++++++++++-------------
> 1 file changed, 12 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/mfd/cs42l43-sdw.c b/drivers/mfd/cs42l43-sdw.c
> index 7392b3d2e6b96..6d67020396fb5 100644
> --- a/drivers/mfd/cs42l43-sdw.c
> +++ b/drivers/mfd/cs42l43-sdw.c
> @@ -18,11 +18,13 @@
> #include "cs42l43.h"
>
> enum cs42l43_sdw_ports {
> - CS42L43_DMIC_DEC_ASP_PORT = 1,
> - CS42L43_SPK_TX_PORT,
> - CS42L43_SPDIF_HP_PORT,
> - CS42L43_SPK_RX_PORT,
> - CS42L43_ASP_PORT,
> + CS42L43_DP1 = 1,
> + CS42L43_DP2,
> + CS42L43_DP3,
> + CS42L43_DP4,
> + CS42L43_DP5,
> + CS42L43_DP6,
> + CS42L43_DP7,
> };
This is starting to look a little redundant now.
I'm not sure if this is better, but it sure takes up less room:
#define CS42L43_SDW_PORT(port) port
> static const struct regmap_config cs42l43_sdw_regmap = {
> @@ -55,9 +57,9 @@ static int cs42l43_read_prop(struct sdw_slave *sdw)
> prop->use_domain_irq = true;
> prop->paging_support = true;
> prop->wake_capable = true;
> - prop->source_ports = BIT(CS42L43_DMIC_DEC_ASP_PORT) | BIT(CS42L43_SPK_TX_PORT);
> - prop->sink_ports = BIT(CS42L43_SPDIF_HP_PORT) |
> - BIT(CS42L43_SPK_RX_PORT) | BIT(CS42L43_ASP_PORT);
> + prop->source_ports = BIT(CS42L43_DP1) | BIT(CS42L43_DP2) |
> + BIT(CS42L43_DP3) | BIT(CS42L43_DP4);
> + prop->sink_ports = BIT(CS42L43_DP5) | BIT(CS42L43_DP6) | BIT(CS42L43_DP7);
> prop->quirks = SDW_SLAVE_QUIRKS_INVALID_INITIAL_PARITY;
> prop->scp_int1_mask = SDW_SCP_INT1_BUS_CLASH | SDW_SCP_INT1_PARITY |
> SDW_SCP_INT1_IMPL_DEF;
> @@ -78,11 +80,8 @@ static int cs42l43_read_prop(struct sdw_slave *sdw)
> dpn[i].max_word = 24;
> i++;
> }
> - /*
> - * All ports are 2 channels max, except the first one,
> - * CS42L43_DMIC_DEC_ASP_PORT.
> - */
> - dpn[CS42L43_DMIC_DEC_ASP_PORT].max_ch = 4;
> + /* All ports are 2 channels max, except the first one. */
> + dpn[CS42L43_DP1].max_ch = 4;
>
> nval = hweight32(prop->sink_ports);
> prop->sink_dpn_prop = devm_kcalloc(dev, nval, sizeof(*prop->sink_dpn_prop),
> --
> 2.39.2
>
On Thu, Nov 23, 2023 at 11:30:55AM +0000, Lee Jones wrote:
> On Mon, 13 Nov 2023, Charles Keepax wrote:
> > enum cs42l43_sdw_ports {
> > - CS42L43_DMIC_DEC_ASP_PORT = 1,
> > - CS42L43_SPK_TX_PORT,
> > - CS42L43_SPDIF_HP_PORT,
> > - CS42L43_SPK_RX_PORT,
> > - CS42L43_ASP_PORT,
> > + CS42L43_DP1 = 1,
> > + CS42L43_DP2,
> > + CS42L43_DP3,
> > + CS42L43_DP4,
> > + CS42L43_DP5,
> > + CS42L43_DP6,
> > + CS42L43_DP7,
> > };
>
> This is starting to look a little redundant now.
>
> I'm not sure if this is better, but it sure takes up less room:
>
> #define CS42L43_SDW_PORT(port) port
>
Sure, I think that is better. Will respin the patch. I mean I
guess I could also just drop the enum/define entirely and just
use the port numbers. But will assume you prefer the define
unless I hear otherwise.
Thanks,
Charles
On Thu, 23 Nov 2023, Charles Keepax wrote:
> On Thu, Nov 23, 2023 at 11:30:55AM +0000, Lee Jones wrote:
> > On Mon, 13 Nov 2023, Charles Keepax wrote:
> > > enum cs42l43_sdw_ports {
> > > - CS42L43_DMIC_DEC_ASP_PORT = 1,
> > > - CS42L43_SPK_TX_PORT,
> > > - CS42L43_SPDIF_HP_PORT,
> > > - CS42L43_SPK_RX_PORT,
> > > - CS42L43_ASP_PORT,
> > > + CS42L43_DP1 = 1,
> > > + CS42L43_DP2,
> > > + CS42L43_DP3,
> > > + CS42L43_DP4,
> > > + CS42L43_DP5,
> > > + CS42L43_DP6,
> > > + CS42L43_DP7,
> > > };
> >
> > This is starting to look a little redundant now.
> >
> > I'm not sure if this is better, but it sure takes up less room:
> >
> > #define CS42L43_SDW_PORT(port) port
> >
>
> Sure, I think that is better. Will respin the patch. I mean I
> guess I could also just drop the enum/define entirely and just
> use the port numbers. But will assume you prefer the define
> unless I hear otherwise.
Yes, I like magic numbers even less. Thanks.
@@ -18,11 +18,13 @@
#include "cs42l43.h"
enum cs42l43_sdw_ports {
- CS42L43_DMIC_DEC_ASP_PORT = 1,
- CS42L43_SPK_TX_PORT,
- CS42L43_SPDIF_HP_PORT,
- CS42L43_SPK_RX_PORT,
- CS42L43_ASP_PORT,
+ CS42L43_DP1 = 1,
+ CS42L43_DP2,
+ CS42L43_DP3,
+ CS42L43_DP4,
+ CS42L43_DP5,
+ CS42L43_DP6,
+ CS42L43_DP7,
};
static const struct regmap_config cs42l43_sdw_regmap = {
@@ -55,9 +57,9 @@ static int cs42l43_read_prop(struct sdw_slave *sdw)
prop->use_domain_irq = true;
prop->paging_support = true;
prop->wake_capable = true;
- prop->source_ports = BIT(CS42L43_DMIC_DEC_ASP_PORT) | BIT(CS42L43_SPK_TX_PORT);
- prop->sink_ports = BIT(CS42L43_SPDIF_HP_PORT) |
- BIT(CS42L43_SPK_RX_PORT) | BIT(CS42L43_ASP_PORT);
+ prop->source_ports = BIT(CS42L43_DP1) | BIT(CS42L43_DP2) |
+ BIT(CS42L43_DP3) | BIT(CS42L43_DP4);
+ prop->sink_ports = BIT(CS42L43_DP5) | BIT(CS42L43_DP6) | BIT(CS42L43_DP7);
prop->quirks = SDW_SLAVE_QUIRKS_INVALID_INITIAL_PARITY;
prop->scp_int1_mask = SDW_SCP_INT1_BUS_CLASH | SDW_SCP_INT1_PARITY |
SDW_SCP_INT1_IMPL_DEF;
@@ -78,11 +80,8 @@ static int cs42l43_read_prop(struct sdw_slave *sdw)
dpn[i].max_word = 24;
i++;
}
- /*
- * All ports are 2 channels max, except the first one,
- * CS42L43_DMIC_DEC_ASP_PORT.
- */
- dpn[CS42L43_DMIC_DEC_ASP_PORT].max_ch = 4;
+ /* All ports are 2 channels max, except the first one. */
+ dpn[CS42L43_DP1].max_ch = 4;
nval = hweight32(prop->sink_ports);
prop->sink_dpn_prop = devm_kcalloc(dev, nval, sizeof(*prop->sink_dpn_prop),