Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18;
MIME-Version: 1.0
References: <20220509064928.2352796-1-Qing-wu.Li@leica-geosystems.com.cn> <20220509064928.2352796-4-Qing-wu.Li@leica-geosystems.com.cn>
In-Reply-To: <20220509064928.2352796-4-Qing-wu.Li@leica-geosystems.com.cn>
From:   Andy Shevchenko <andy.shevchenko@gmail.com>
Date:   Mon, 9 May 2022 11:23:13 +0200
Message-ID: <CAHp75VcAvRkM84XzYtE7T=Zwoa0Ee=OfCbOV-KbsDELAsy+ZiA@mail.gmail.com>
Subject: Re: [PATCH V4 3/5] iio: accel: sca3300: modified to support multi chips
To:     LI Qingwu <Qing-wu.Li@leica-geosystems.com.cn>
Cc:     jic23@kernel.org, lars@metafoo.de, robh+dt@kernel.org,
        tomas.melin@vaisala.com, devicetree@vger.kernel.org,
        linux-kernel@vger.kernel.org, linux-iio@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Mon, May 9, 2022 at 8:49 AM LI Qingwu
<Qing-wu.Li@leica-geosystems.com.cn> wrote:
>
> The driver supports sca3300 only.
> There are some other similar chips, for instance, SCL3300.
> Prepare the way for multiple chips and additional channels.
> Modify the driver to read the device id.
> Add the tables for the corresponding id to support multiple chips.
> Add prepares for the addition of extra channels.
> Add prepares for handling the operation modes for multiple chips.

Fancy style. Utilize the entire width of the line, i.e. ~72 characters.

...

> +struct sca3300_chip_info {
> +       const struct iio_chan_spec *channels;
> +       u8 num_channels;
> +       const unsigned long *scan_masks;
> +       const int (*accel_scale)[2];
> +       const int *accel_scale_map;
> +       u8 num_accel_scales;
> +       const int *freq_table;
> +       const int *freq_map;
> +       u8 num_freqs;
> +       const int *avail_modes_table;
> +       u8 num_avail_modes;
> +       const char *name;
> +       u8 chip_id;

I recommend to shuffle it in a way that u8 members go closer to each
other if possible.

Like:

       const unsigned long *scan_masks;
       const struct iio_chan_spec *channels;
       u8 num_channels;
       u8 num_accel_scales;
       const int (*accel_scale)[2];
       const int *accel_scale_map;

It will save a few bytes per object instance due to padding reduction.

The idea about grouping all of them also can be done despite the
comment given previously by someone.

> +};

...

> +       for (i = 0; i < sca_data->chip->num_avail_modes; i++) {
> +               if (sca_data->chip->avail_modes_table[i] == reg_val) {
> +                       *index = i;
> +                       return 0;
> +               }
> +       }
> +
> +       return -EINVAL;

Can be modified to use standard pattern (return errors first), but
it's up to maintainers, because the latter requires an additional
check after the loop.

...

> +       for (i = 0; i < chip->num_avail_modes; i++) {
> +               if (val == chip->freq_table[chip->freq_map[i]]) {
> +                       if (chip->accel_scale[chip->accel_scale_map[index]] ==
> +                           chip->accel_scale[chip->accel_scale_map[i]])
> +                               return sca3300_set_op_mode(data, i);

The
  if (a) {
    if (b) {
      ...
    }
  }

can be replaced by

  if (a && b) {
    ...
  }

> +               }
> +       }
> +
> +       return -EINVAL;

As per above.

-- 
With Best Regards,
Andy Shevchenko