On 10/17/2016 05:21 AM, Eva Rachel Retuya wrote:
> Introduce defines for shifting and mask under the config register for
> better readability.
>
> Signed-off-by: Eva Rachel Retuya <eraretuya@xxxxxxxxx>
Hi,
Thanks for the patch. Comments inline.
> ---
> drivers/staging/iio/cdc/ad7746.c | 26 ++++++++++++++++----------
> 1 file changed, 16 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/staging/iio/cdc/ad7746.c b/drivers/staging/iio/cdc/ad7746.c
> index f41251c..751f0d1 100644
> --- a/drivers/staging/iio/cdc/ad7746.c
> +++ b/drivers/staging/iio/cdc/ad7746.c
> @@ -70,8 +70,12 @@
> #define AD7746_EXCSETUP_EXCLVL(x) (((x) & 0x3) << 0)
>
> /* Config Register Bit Designations (AD7746_REG_CFG) */
> -#define AD7746_CONF_VTFS(x) ((x) << 6)
> -#define AD7746_CONF_CAPFS(x) ((x) << 3)
> +#define AD7746_CONF_VTFS_SHIFT 6
> +#define AD7746_CONF_CAPFS_SHIFT 3
> +#define AD7746_CONF_VTFS_MASK 0x3
> +#define AD7746_CONF_CAPFS_MASK 0x7
> +#define AD7746_CONF_VTFS(x) ((x) << AD7746_CONF_VTFS_SHIFT)
> +#define AD7746_CONF_CAPFS(x) ((x) << AD7746_CONF_CAPFS_SHIFT)
> #define AD7746_CONF_MODE_IDLE (0 << 0)
> #define AD7746_CONF_MODE_CONT_CONV (1 << 0)
> #define AD7746_CONF_MODE_SINGLE_CONV (2 << 0)
> @@ -224,8 +228,8 @@ static int ad7746_select_channel(struct iio_dev *indio_dev,
> case IIO_CAPACITANCE:
> cap_setup = (chan->address & 0xFF) | AD7746_CAPSETUP_CAPEN;
> vt_setup = chip->vt_setup & ~AD7746_VTSETUP_VTEN;
> - delay = ad7746_cap_filter_rate_table[(chip->config >> 3) &
> - 0x7][1];
> + delay = ad7746_cap_filter_rate_table[(chip->config &
> + AD7746_CONF_CAPFS_MASK) >> AD7746_CONF_CAPFS_SHIFT][1];
This flips the order in which the shift and the mask are applied, that will
break things. But maybe we should change things so that the mask includes
the offset and use the GENMASK() macro to create them. This would also allow
to simplify
chip->config &= ~AD7746_CONF_CAPFS(AD7746_CONF_CAPFS_MASK);
to
chip->config &= ~AD7746_CONF_CAPFS_MASK;
I'd also introduce a helper variable to move the index calculation outside
of the array brackets. It is a bit convoluted the way it is right now.
Same comment for the similar changes done below.
>
> if (chip->capdac_set != chan->channel) {
> ret = i2c_smbus_write_byte_data(chip->client,
> @@ -246,8 +250,8 @@ static int ad7746_select_channel(struct iio_dev *indio_dev,
> case IIO_TEMP:
> vt_setup = (chan->address & 0xFF) | AD7746_VTSETUP_VTEN;
> cap_setup = chip->cap_setup & ~AD7746_CAPSETUP_CAPEN;
> - delay = ad7746_cap_filter_rate_table[(chip->config >> 6) &
> - 0x3][1];
> + delay = ad7746_cap_filter_rate_table[(chip->config &
> + AD7746_CONF_VTFS_MASK) >> AD7746_CONF_VTFS_SHIFT][1];
> break;
> default:
> return -EINVAL;
[...]
> @@ -636,12 +640,14 @@ static int ad7746_read_raw(struct iio_dev *indio_dev,
> switch (chan->type) {
> case IIO_CAPACITANCE:
> *val = ad7746_cap_filter_rate_table[
> - (chip->config >> 3) & 0x7][0];
> + (chip->config & AD7746_CONF_CAPFS_MASK) >>
> + AD7746_CONF_CAPFS_SHIFT][0];
> ret = IIO_VAL_INT;
> break;
> case IIO_VOLTAGE:
> *val = ad7746_vt_filter_rate_table[
> - (chip->config >> 6) & 0x3][0];
> + (chip->config & AD7746_CONF_VTFS_MASK) >>
> + AD7746_CONF_VTFS_SHIFT][0];
> break;
> default:
> ret = -EINVAL;
>
--
To unsubscribe from this list: send the line "unsubscribe linux-iio" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
