Re: [PATCH 4/4] regmap: sdw: add support for SoundWire 1.2 MBQ

On 25-08-20, 12:16, Pierre-Louis Bossart wrote:

The SoundWire 1.1 specification only allowed for reads and writes of bytes. The SoundWire 1.2 specification adds a new capability to transfer "Multi-Byte Quantities" (MBQ) across the bus. The transfers still happens one-byte-at-a-time, but the update is atomic.

For example when writing a 16-bit volume, the first byte transferred is only taken into account when the second byte is successfully transferred.

The mechanism is symmetrical for read and writes:

• On a read, the address of the last byte to be read is modified by

setting the MBQ bit

• On a write, the address of all but the last byte to be written are

modified by setting the MBQ bit. The address for the last byte relies on the MBQ bit being cleared.

The current definitions for MBQ-based controls in the SDCA draft standard are limited to 16 bits for volumes, so for now this is the only supported format. An update will be provided if and when support for 24-bit and 32-bit values is specified by the SDCA standard.

One possible objection is that this code could have been handled with regmap-sdw.c. However this is a new spec addition not handled by every SoundWire 1.1 and non-SDCA device, so there's no reason to load code that will never be used.

Also in practice it's extremely unlikely that CONFIG_REGMAP would not be selected with CONFIG_REGMAP_MBQ selected. However there's no functional dependency between the two modules so they can be selected separately.

Is there a reason for a new module for mbq writes, cant we do this as part of sdw module? Driver can invoke either regmap_init_sdw() or regmap_init_sdw_mbq()?

+++ b/drivers/base/regmap/regmap-sdw-mbq.c @@ -0,0 +1,102 @@ +// SPDX-License-Identifier: GPL-2.0 +// Copyright(c) 2020 Intel Corporation.

+#include <linux/device.h> +#include <linux/errno.h> +#include <linux/mod_devicetable.h>

Curious why do you need this header?

+#include <linux/module.h> +#include <linux/regmap.h> +#include <linux/soundwire/sdw.h> +#include <linux/soundwire/sdw_registers.h> +#include "internal.h"

+static int regmap_sdw_mbq_write(void *context, unsigned int reg, unsigned int val) +{

• struct device *dev = context;
• struct sdw_slave *slave = dev_to_sdw_dev(dev);
• int ret;
• ret = sdw_write(slave, SDW_SDCA_MBQ_CTL(reg), (val >> 8) & 0xff);
• if (ret < 0)

• return ret;
  

• return sdw_write(slave, reg, val & 0xff);

+}

+static int regmap_sdw_mbq_read(void *context, unsigned int reg, unsigned int *val) +{

• struct device *dev = context;
• struct sdw_slave *slave = dev_to_sdw_dev(dev);
• int read0;
• int read1;
• read0 = sdw_read(slave, reg);
• if (read0 < 0)

• return read0;
  

• read1 = sdw_read(slave, SDW_SDCA_MBQ_CTL(reg));
• if (read1 < 0)

• return read1;
  

• *val = (read1 << 8) | read0;
• return 0;

+}

+static struct regmap_bus regmap_sdw_mbq = {

• .reg_read = regmap_sdw_mbq_read,
• .reg_write = regmap_sdw_mbq_write,
• .reg_format_endian_default = REGMAP_ENDIAN_LITTLE,
• .val_format_endian_default = REGMAP_ENDIAN_LITTLE,

+};

+static int regmap_sdw_mbq_config_check(const struct regmap_config *config) +{

• /* MBQ-based controls are only 16-bits for now */
• if (config->val_bits != 16)

• return -EOPNOTSUPP;
  

• /* Registers are 32 bits wide */
• if (config->reg_bits != 32)

• return -EOPNOTSUPP;
  

• if (config->pad_bits != 0)

• return -EOPNOTSUPP;
  

• return 0;

+}

+struct regmap *__regmap_init_sdw_mbq(struct sdw_slave *sdw,

• 		     const struct regmap_config *config,
  

• 		     struct lock_class_key *lock_key,
  

• 		     const char *lock_name)
  

+{

• int ret;
• ret = regmap_sdw_mbq_config_check(config);
• if (ret)

• return ERR_PTR(ret);
  

• return __regmap_init(&sdw->dev, &regmap_sdw_mbq,

• 	&sdw->dev, config, lock_key, lock_name);
  

+} +EXPORT_SYMBOL_GPL(__regmap_init_sdw_mbq);

+struct regmap *__devm_regmap_init_sdw_mbq(struct sdw_slave *sdw,

• 			  const struct regmap_config *config,
  

• 			  struct lock_class_key *lock_key,
  

• 			  const char *lock_name)
  

+{

• int ret;
• ret = regmap_sdw_mbq_config_check(config);
• if (ret)

• return ERR_PTR(ret);
  

• return __devm_regmap_init(&sdw->dev, &regmap_sdw_mbq,

• 	&sdw->dev, config, lock_key, lock_name);
  

+} +EXPORT_SYMBOL_GPL(__devm_regmap_init_sdw_mbq);

+MODULE_DESCRIPTION("Regmap SoundWire Module");

This is same of sdw module, pls make this one a bit different.