From: Jonathan Cameron <jic23@kernel.org>
To: David Lechner <dlechner@baylibre.com>
Cc: linux-iio@vger.kernel.org, devicetree@vger.kernel.org,
linux-staging@lists.linux.dev, linux-kernel@vger.kernel.org,
"Rob Herring" <robh+dt@kernel.org>,
"Krzysztof Kozlowski" <krzysztof.kozlowski+dt@linaro.org>,
"Conor Dooley" <conor+dt@kernel.org>,
"Michael Hennerich" <Michael.Hennerich@analog.com>,
"Nuno Sá" <nuno.sa@analog.com>,
"Axel Haslam" <ahaslam@baylibre.com>,
"Philip Molloy" <pmolloy@baylibre.com>
Subject: Re: [PATCH v2 09/19] staging: iio: resolver: ad2s1210: use regmap for config registers
Date: Sun, 24 Sep 2023 18:59:02 +0100 [thread overview]
Message-ID: <20230924185902.579a444b@jic23-huawei> (raw)
In-Reply-To: <20230921144400.62380-10-dlechner@baylibre.com>
On Thu, 21 Sep 2023 09:43:50 -0500
David Lechner <dlechner@baylibre.com> 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@baylibre.com>
> -/* 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;
> }
next prev parent reply other threads:[~2023-09-24 17:59 UTC|newest]
Thread overview: 40+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-09-21 14:43 [PATCH v2 00/19] iio: resolver: move ad2s1210 out of staging David Lechner
2023-09-21 14:43 ` [PATCH v2 01/19] dt-bindings: iio: resolver: add devicetree bindings for ad2s1210 David Lechner
2023-09-22 22:44 ` Rob Herring
2023-09-24 16:57 ` Jonathan Cameron
2023-09-25 15:54 ` David Lechner
2023-09-25 17:47 ` David Lechner
2023-09-21 14:43 ` [PATCH v2 02/19] staging: iio: Documentation: document IIO resolver AD2S1210 sysfs attributes David Lechner
2023-09-24 17:15 ` Jonathan Cameron
2023-09-21 14:43 ` [PATCH v2 03/19] staging: iio: resolver: ad2s1210: fix ad2s1210_show_fault David Lechner
2023-09-24 17:17 ` Jonathan Cameron
2023-09-21 14:43 ` [PATCH v2 04/19] staging: iio: resolver: ad2s1210: fix not restoring sample gpio in channel read David Lechner
2023-09-24 17:19 ` Jonathan Cameron
2023-09-21 14:43 ` [PATCH v2 05/19] staging: iio: resolver: ad2s1210: fix probe David Lechner
2023-09-24 17:25 ` Jonathan Cameron
2023-09-21 14:43 ` [PATCH v2 06/19] staging: iio: resolver: ad2s1210: always use 16-bit value for raw read David Lechner
2023-09-24 17:31 ` Jonathan Cameron
2023-09-21 14:43 ` [PATCH v2 07/19] staging: iio: resolver: ad2s1210: implement IIO_CHAN_INFO_SCALE David Lechner
2023-09-24 17:38 ` Jonathan Cameron
2023-09-21 14:43 ` [PATCH v2 08/19] staging: iio: resolver: ad2s1210: use devicetree to get fclkin David Lechner
2023-09-24 17:43 ` Jonathan Cameron
2023-09-21 14:43 ` [PATCH v2 09/19] staging: iio: resolver: ad2s1210: use regmap for config registers David Lechner
2023-09-24 17:59 ` Jonathan Cameron [this message]
2023-09-25 16:37 ` David Lechner
2023-09-21 14:43 ` [PATCH v2 10/19] staging: iio: resolver: ad2s1210: add debugfs reg access David Lechner
2023-09-21 14:43 ` [PATCH v2 11/19] staging: iio: resolver: ad2s1210: remove config attribute David Lechner
2023-09-21 14:43 ` [PATCH v2 12/19] staging: iio: resolver: ad2s1210: rework gpios David Lechner
2023-09-21 14:43 ` [PATCH v2 13/19] staging: iio: resolver: ad2s1210: implement hysteresis as channel attr David Lechner
2023-09-24 18:05 ` Jonathan Cameron
2023-09-21 14:43 ` [PATCH v2 14/19] staging: iio: resolver: ad2s1210: refactor setting excitation frequency David Lechner
2023-09-24 18:08 ` Jonathan Cameron
2023-09-21 14:43 ` [PATCH v2 15/19] staging: iio: resolver: ad2s1210: read excitation frequency from control register David Lechner
2023-09-21 14:43 ` [PATCH v2 16/19] staging: iio: resolver: ad2s1210: rename fexcit attribute David Lechner
2023-09-21 14:43 ` [PATCH v2 17/19] staging: iio: resolver: ad2s1210: convert resolution to devicetree property David Lechner
2023-09-24 18:10 ` Jonathan Cameron
2023-09-21 14:43 ` [PATCH v2 18/19] staging: iio: resolver: ad2s1210: add phase_lock_range attributes David Lechner
2023-09-24 18:12 ` Jonathan Cameron
2023-09-21 14:44 ` [PATCH v2 19/19] staging: iio: resolver: ad2s1210: add triggered buffer support David Lechner
2023-09-24 18:17 ` Jonathan Cameron
2023-09-25 16:50 ` David Lechner
2023-09-24 16:51 ` [PATCH v2 00/19] iio: resolver: move ad2s1210 out of staging Jonathan Cameron
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20230924185902.579a444b@jic23-huawei \
--to=jic23@kernel.org \
--cc=Michael.Hennerich@analog.com \
--cc=ahaslam@baylibre.com \
--cc=conor+dt@kernel.org \
--cc=devicetree@vger.kernel.org \
--cc=dlechner@baylibre.com \
--cc=krzysztof.kozlowski+dt@linaro.org \
--cc=linux-iio@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-staging@lists.linux.dev \
--cc=nuno.sa@analog.com \
--cc=pmolloy@baylibre.com \
--cc=robh+dt@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).