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, ®map_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, ®map_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.
--
~Vinod
