On Thu, 21 Sep 2023 09:43:50 -0500
David Lechner <dlechner@xxxxxxxxxxxx> wrote:
> This makes use of the regmap API to read and write the configuration
> registers. This simplifies code quite a bit and makes it safer
> (previously, it was easy to write a bad value to the config registers
> which causes the chip to lock up and need to be reset).
>
I'd like a bit more description in here -mostly because I have no idea
what the original code was doing.
What were the MSB writes that followed main config writes for and why
can we get rid of them>
Also, using regmap for only some accesses is a bit unusual. Add some
text here to justify that decision.
Jonathan
> Signed-off-by: David Lechner <dlechner@xxxxxxxxxxxx>
> -/* write 1 bytes (address or data) to the chip */
> -static int ad2s1210_config_write(struct ad2s1210_state *st, u8 data)
> +/*
> + * Writes the given data to the given register address.
> + *
> + * If the mode is configurable, the device will first be placed in
> + * configuration mode.
> + */
> +static int ad2s1210_regmap_reg_write(void *context, unsigned int reg,
> + unsigned int val)
> {
> - int ret;
> + struct ad2s1210_state *st = context;
> + struct spi_transfer xfers[] = {
> + {
> + .len = 1,
> + .rx_buf = &st->rx[0],
> + .tx_buf = &st->tx[0],
> + .cs_change = 1,
> + }, {
> + .len = 1,
> + .rx_buf = &st->rx[1],
> + .tx_buf = &st->tx[1],
> + },
> + };
> +
> + /* values can only be 7 bits, the MSB indicates an address */
> + if (val & ~0x7F)
> + return -EINVAL;
> +
> + st->tx[0] = reg;
> + st->tx[1] = val;
>
> ad2s1210_set_mode(MOD_CONFIG, st);
> - st->tx[0] = data;
> - ret = spi_write(st->sdev, st->tx, 1);
> - if (ret < 0)
> - return ret;
>
> - return 0;
> + return spi_sync_transfer(st->sdev, xfers, ARRAY_SIZE(xfers));
> }
>
> -/* read value from one of the registers */
> -static int ad2s1210_config_read(struct ad2s1210_state *st,
> - unsigned char address)
> +/*
> + * Reads value from one of the registers.
> + *
> + * If the mode is configurable, the device will first be placed in
> + * configuration mode.
> + */
> +static int ad2s1210_regmap_reg_read(void *context, unsigned int reg,
> + unsigned int *val)
> {
> + struct ad2s1210_state *st = context;
> struct spi_transfer xfers[] = {
> {
> .len = 1,
> @@ -146,22 +176,34 @@ static int ad2s1210_config_read(struct ad2s1210_state *st,
> .tx_buf = &st->tx[1],
> },
> };
> - int ret = 0;
> + int ret;
>
> ad2s1210_set_mode(MOD_CONFIG, st);
> - st->tx[0] = address | AD2S1210_MSB_IS_HIGH;
> + st->tx[0] = reg;
> + /* Must be valid register address here otherwise this could write data.
> + * It doesn't matter which one.
> + */
> st->tx[1] = AD2S1210_REG_FAULT;
> - ret = spi_sync_transfer(st->sdev, xfers, 2);
> +
> + ret = spi_sync_transfer(st->sdev, xfers, ARRAY_SIZE(xfers));
> if (ret < 0)
> return ret;
>
> - return st->rx[1];
> + /* If the D7 bit is set on any read/write register, it indicates a
IIO comments are
/*
* If ...
> + * parity error. The fault register is read-only and the D7 bit means
> + * something else there.
> + */
> + if (reg != AD2S1210_REG_FAULT && st->rx[1] & AD2S1210_ADDRESS_DATA)
> + return -EBADMSG;
> +
> + *val = st->rx[1];
> +
> + return 0;
> }
>
> static ssize_t ad2s1210_store_control(struct device *dev,
> @@ -264,25 +297,13 @@ static ssize_t ad2s1210_store_control(struct device *dev,
> return -EINVAL;
>
> mutex_lock(&st->lock);
> - ret = ad2s1210_config_write(st, AD2S1210_REG_CONTROL);
> - if (ret < 0)
> - goto error_ret;
> - data = udata & AD2S1210_MSB_IS_LOW;
> - ret = ad2s1210_config_write(st, data);
> + data = udata & ~AD2S1210_ADDRESS_DATA;
> + ret = regmap_write(st->regmap, AD2S1210_REG_CONTROL, data);
> if (ret < 0)
> goto error_ret;
>
> - ret = ad2s1210_config_read(st, AD2S1210_REG_CONTROL);
> - if (ret < 0)
> - goto error_ret;
> - if (ret & AD2S1210_MSB_IS_HIGH) {
> - ret = -EIO;
> - dev_err(dev,
> - "ad2s1210: write control register fail\n");
> - goto error_ret;
> - }
> st->resolution =
> - ad2s1210_resolution_value[data & AD2S1210_SET_RESOLUTION];
> + ad2s1210_resolution_value[data & AD2S1210_SET_RES];
> ad2s1210_set_resolution_pin(st);
> ret = len;
> st->hysteresis = !!(data & AD2S1210_ENABLE_HYSTERESIS);
> @@ -315,30 +336,17 @@ static ssize_t ad2s1210_store_resolution(struct device *dev,
> dev_err(dev, "ad2s1210: resolution out of range\n");
> return -EINVAL;
> }
> +
> + data = (udata - 10) >> 1;
> +
> mutex_lock(&st->lock);
> - ret = ad2s1210_config_read(st, AD2S1210_REG_CONTROL);
> - if (ret < 0)
> - goto error_ret;
> - data = ret;
> - data &= ~AD2S1210_SET_RESOLUTION;
> - data |= (udata - 10) >> 1;
> - ret = ad2s1210_config_write(st, AD2S1210_REG_CONTROL);
> - if (ret < 0)
> - goto error_ret;
> - ret = ad2s1210_config_write(st, data & AD2S1210_MSB_IS_LOW);
> + ret = regmap_update_bits(st->regmap, AD2S1210_REG_CONTROL,
> + AD2S1210_SET_RES, data);
> if (ret < 0)
> goto error_ret;
> - ret = ad2s1210_config_read(st, AD2S1210_REG_CONTROL);
> - if (ret < 0)
> - goto error_ret;
> - data = ret;
> - if (data & AD2S1210_MSB_IS_HIGH) {
> - ret = -EIO;
> - dev_err(dev, "ad2s1210: setting resolution fail\n");
> - goto error_ret;
> - }
> +
> st->resolution =
> - ad2s1210_resolution_value[data & AD2S1210_SET_RESOLUTION];
> + ad2s1210_resolution_value[data & AD2S1210_SET_RES];
> ad2s1210_set_resolution_pin(st);
> ret = len;
> error_ret:
> @@ -351,13 +359,14 @@ static ssize_t ad2s1210_show_fault(struct device *dev,
> struct device_attribute *attr, char *buf)
> {
> struct ad2s1210_state *st = iio_priv(dev_to_iio_dev(dev));
> + unsigned int value;
> int ret;
>
> mutex_lock(&st->lock);
> - ret = ad2s1210_config_read(st, AD2S1210_REG_FAULT);
> + ret = regmap_read(st->regmap, AD2S1210_REG_FAULT, &value);
> mutex_unlock(&st->lock);
>
> - return (ret < 0) ? ret : sprintf(buf, "0x%02x\n", ret);
> + return ret < 0 ? ret : sprintf(buf, "0x%02x\n", value);
You added the brackets in earlier patch. I've dropped them now
from my tree which will make this not quite apply.
> }
>
...
>
> static ssize_t ad2s1210_store_reg(struct device *dev,
> @@ -409,14 +420,11 @@ static ssize_t ad2s1210_store_reg(struct device *dev,
> struct iio_dev_attr *iattr = to_iio_dev_attr(attr);
>
> ret = kstrtou8(buf, 10, &data);
> - if (ret)
> + if (ret < 0)
> return -EINVAL;
> +
Unrelated. Also unnecessary as it doesn't return postive values.
> mutex_lock(&st->lock);
> - ret = ad2s1210_config_write(st, iattr->address);
> - if (ret < 0)
> - goto error_ret;
> - ret = ad2s1210_config_write(st, data & AD2S1210_MSB_IS_LOW);
> -error_ret:
> + ret = regmap_write(st->regmap, iattr->address, data);
> mutex_unlock(&st->lock);
> return ret < 0 ? ret : len;
> }
> @@ -583,23 +591,12 @@ static int ad2s1210_initial(struct ad2s1210_state *st)
> mutex_lock(&st->lock);
> ad2s1210_set_resolution_pin(st);
>
> - ret = ad2s1210_config_write(st, AD2S1210_REG_CONTROL);
> - if (ret < 0)
> - goto error_ret;
> - data = AD2S1210_DEF_CONTROL & ~(AD2S1210_SET_RESOLUTION);
> + data = AD2S1210_DEF_CONTROL & ~AD2S1210_SET_RES;
Somewhat unrelated and you may sort it later, but I'd like
to see DEF_CONTROL broken out and FIELD_PREP() used for all the fields.
Seems crazy to use a value, then drop some bits then fill them in with
something else.
> data |= (st->resolution - 10) >> 1;
> - ret = ad2s1210_config_write(st, data);
> - if (ret < 0)
> - goto error_ret;
> - ret = ad2s1210_config_read(st, AD2S1210_REG_CONTROL);
> + ret = regmap_write(st->regmap, AD2S1210_REG_CONTROL, data);
> if (ret < 0)
> goto error_ret;
>
> - if (ret & AD2S1210_MSB_IS_HIGH) {
I guess this was meant to be a sanity check on the chip responding.
> - ret = -EIO;
> - goto error_ret;
> - }
> -
> ret = ad2s1210_update_frequency_control_word(st);
> if (ret < 0)
> goto error_ret;
> @@ -652,6 +649,52 @@ static int ad2s1210_setup_gpios(struct ad2s1210_state *st)
> return 0;
> }
