Re: [PATCH v5 1/5] iio: adc: ad7192: Use standard attribute

[David Lechner]

On Sat, Apr 13, 2024 at 10:12 AM Alisa-Dariana Roman
<alisadariana@xxxxxxxxx> wrote:
>
> Replace custom attribute filter_low_pass_3db_frequency_available with
> standard attribute.
>
> Store the available values in ad7192_state struct.
>
> The function that used to compute those values replaced by
> ad7192_update_filter_freq_avail().
>
> Function ad7192_show_filter_avail() is no longer needed.
>
> Note that the initial available values are hardcoded.
>
> Signed-off-by: Alisa-Dariana Roman <alisa.roman@xxxxxxxxxx>
> ---

With the question below addressed:

Reviewed-by: David Lechner <dlechner@xxxxxxxxxxxx>

>  drivers/iio/adc/ad7192.c | 67 ++++++++++++++++++----------------------
>  1 file changed, 30 insertions(+), 37 deletions(-)
>
> diff --git a/drivers/iio/adc/ad7192.c b/drivers/iio/adc/ad7192.c
> index 7bcc7e2aa2a2..ac737221beae 100644
> --- a/drivers/iio/adc/ad7192.c
> +++ b/drivers/iio/adc/ad7192.c
> @@ -190,6 +190,7 @@ struct ad7192_state {
>         u32                             mode;
>         u32                             conf;
>         u32                             scale_avail[8][2];
> +       u32                             filter_freq_avail[4][2];
>         u32                             oversampling_ratio_avail[4];
>         u8                              gpocon;
>         u8                              clock_sel;
> @@ -473,6 +474,16 @@ static int ad7192_setup(struct iio_dev *indio_dev, struct device *dev)
>         st->oversampling_ratio_avail[2] = 8;
>         st->oversampling_ratio_avail[3] = 16;
>
> +       st->filter_freq_avail[0][0] = 600;
> +       st->filter_freq_avail[1][0] = 800;
> +       st->filter_freq_avail[2][0] = 2300;
> +       st->filter_freq_avail[3][0] = 2720;
> +
> +       st->filter_freq_avail[0][1] = 1000;
> +       st->filter_freq_avail[1][1] = 1000;
> +       st->filter_freq_avail[2][1] = 1000;
> +       st->filter_freq_avail[3][1] = 1000;
> +
>         return 0;
>  }
>
> @@ -586,48 +597,24 @@ static int ad7192_get_f_adc(struct ad7192_state *st)
>                                  f_order * FIELD_GET(AD7192_MODE_RATE_MASK, st->mode));
>  }
>
> -static void ad7192_get_available_filter_freq(struct ad7192_state *st,
> -                                                   int *freq)
> +static void ad7192_update_filter_freq_avail(struct ad7192_state *st)
>  {
>         unsigned int fadc;
>
>         /* Formulas for filter at page 25 of the datasheet */
>         fadc = ad7192_compute_f_adc(st, false, true);
> -       freq[0] = DIV_ROUND_CLOSEST(fadc * 240, 1024);
> +       st->filter_freq_avail[0][0] = DIV_ROUND_CLOSEST(fadc * 240, 1024);
>
>         fadc = ad7192_compute_f_adc(st, true, true);
> -       freq[1] = DIV_ROUND_CLOSEST(fadc * 240, 1024);
> +       st->filter_freq_avail[1][0] = DIV_ROUND_CLOSEST(fadc * 240, 1024);
>
>         fadc = ad7192_compute_f_adc(st, false, false);
> -       freq[2] = DIV_ROUND_CLOSEST(fadc * 230, 1024);
> +       st->filter_freq_avail[2][0] = DIV_ROUND_CLOSEST(fadc * 230, 1024);
>
>         fadc = ad7192_compute_f_adc(st, true, false);
> -       freq[3] = DIV_ROUND_CLOSEST(fadc * 272, 1024);
> +       st->filter_freq_avail[3][0] = DIV_ROUND_CLOSEST(fadc * 272, 1024);
>  }
>
> -static ssize_t ad7192_show_filter_avail(struct device *dev,
> -                                       struct device_attribute *attr,
> -                                       char *buf)
> -{
> -       struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> -       struct ad7192_state *st = iio_priv(indio_dev);
> -       unsigned int freq_avail[4], i;
> -       size_t len = 0;
> -
> -       ad7192_get_available_filter_freq(st, freq_avail);
> -
> -       for (i = 0; i < ARRAY_SIZE(freq_avail); i++)
> -               len += sysfs_emit_at(buf, len, "%d.%03d ", freq_avail[i] / 1000,
> -                                    freq_avail[i] % 1000);
> -
> -       buf[len - 1] = '\n';
> -
> -       return len;
> -}
> -
> -static IIO_DEVICE_ATTR(filter_low_pass_3db_frequency_available,
> -                      0444, ad7192_show_filter_avail, NULL, 0);
> -
>  static IIO_DEVICE_ATTR(bridge_switch_en, 0644,
>                        ad7192_show_bridge_switch, ad7192_set,
>                        AD7192_REG_GPOCON);
> @@ -637,7 +624,6 @@ static IIO_DEVICE_ATTR(ac_excitation_en, 0644,
>                        AD7192_REG_CONF);
>
>  static struct attribute *ad7192_attributes[] = {
> -       &iio_dev_attr_filter_low_pass_3db_frequency_available.dev_attr.attr,
>         &iio_dev_attr_bridge_switch_en.dev_attr.attr,
>         NULL
>  };
> @@ -647,7 +633,6 @@ static const struct attribute_group ad7192_attribute_group = {
>  };
>
>  static struct attribute *ad7195_attributes[] = {
> -       &iio_dev_attr_filter_low_pass_3db_frequency_available.dev_attr.attr,
>         &iio_dev_attr_bridge_switch_en.dev_attr.attr,
>         &iio_dev_attr_ac_excitation_en.dev_attr.attr,
>         NULL
> @@ -665,17 +650,15 @@ static unsigned int ad7192_get_temp_scale(bool unipolar)
>  static int ad7192_set_3db_filter_freq(struct ad7192_state *st,
>                                       int val, int val2)
>  {
> -       int freq_avail[4], i, ret, freq;
> +       int i, ret, freq;
>         unsigned int diff_new, diff_old;
>         int idx = 0;
>
>         diff_old = U32_MAX;
>         freq = val * 1000 + val2;
>
> -       ad7192_get_available_filter_freq(st, freq_avail);
> -
> -       for (i = 0; i < ARRAY_SIZE(freq_avail); i++) {
> -               diff_new = abs(freq - freq_avail[i]);
> +       for (i = 0; i < ARRAY_SIZE(st->filter_freq_avail); i++) {
> +               diff_new = abs(freq - st->filter_freq_avail[i][0]);
>                 if (diff_new < diff_old) {
>                         diff_old = diff_new;
>                         idx = i;
> @@ -826,6 +809,7 @@ static int ad7192_write_raw(struct iio_dev *indio_dev,
>                 st->mode &= ~AD7192_MODE_RATE_MASK;
>                 st->mode |= FIELD_PREP(AD7192_MODE_RATE_MASK, div);
>                 ad_sd_write_reg(&st->sd, AD7192_REG_MODE, 3, st->mode);
> +               ad7192_update_filter_freq_avail(st);
>                 break;
>         case IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY:
>                 ret = ad7192_set_3db_filter_freq(st, val, val2 / 1000);
> @@ -846,6 +830,7 @@ static int ad7192_write_raw(struct iio_dev *indio_dev,
>                                 break;
>                         }
>                 mutex_unlock(&st->lock);
> +               ad7192_update_filter_freq_avail(st);

Does this need to go inside of the mutex guard to avoid potential race
conditions?

>                 break;
>         default:
>                 ret = -EINVAL;
> @@ -888,6 +873,12 @@ static int ad7192_read_avail(struct iio_dev *indio_dev,
>                 /* Values are stored in a 2D matrix  */
>                 *length = ARRAY_SIZE(st->scale_avail) * 2;
>
> +               return IIO_AVAIL_LIST;
> +       case IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY:
> +               *vals = (int *)st->filter_freq_avail;
> +               *type = IIO_VAL_FRACTIONAL;
> +               *length = ARRAY_SIZE(st->filter_freq_avail) * 2;
> +
>                 return IIO_AVAIL_LIST;
>         case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
>                 *vals = (int *)st->oversampling_ratio_avail;
> @@ -956,7 +947,9 @@ static const struct iio_info ad7195_info = {
>                         BIT(IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY) | \
>                         (_mask_all), \
>                 .info_mask_shared_by_type_available = (_mask_type_av), \
> -               .info_mask_shared_by_all_available = (_mask_all_av), \
> +               .info_mask_shared_by_all_available = \
> +                       BIT(IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY) | \
> +                       (_mask_all_av), \
>                 .ext_info = (_ext_info), \
>                 .scan_index = (_si), \
>                 .scan_type = { \
> --
> 2.34.1
>