On Tue, 10 Dec 2024 10:46:44 +0000
Guillaume Stols <gstols@xxxxxxxxxxxx> wrote:
> Since the register are always the same, whatever bus is used, moving the
> software functions into the main file avoids the code to be duplicated
> in both SPI and parallel version of the driver.
>
> Signed-off-by: Guillaume Stols <gstols@xxxxxxxxxxxx>
Hi Guillaume,
Patch content is fine, but I'd ideally like you to take the opportunity to
tidy up some of the really inconsistent indentation in the code you are moving.
> int ad7616_write_scale_sw(struct iio_dev *indio_dev, int ch, int val)
> {
> struct ad7606_state *st = iio_priv(indio_dev);
> + unsigned int ch_addr, mode, ch_index;
>
> - st->sw_mode_en = st->bops->sw_mode_config &&
> - device_property_present(st->dev, "adi,sw-mode");
> - if (!st->sw_mode_en)
> - return 0;
> + /*
> + * Ad7616 has 16 channels divided in group A and group B.
> + * The range of channels from A are stored in registers with address 4
> + * while channels from B are stored in register with address 6.
> + * The last bit from channels determines if it is from group A or B
> + * because the order of channels in iio is 0A, 0B, 1A, 1B...
> + */
> + ch_index = ch >> 1;
> +
> + ch_addr = AD7616_RANGE_CH_ADDR(ch_index);
> +
> + if ((ch & 0x1) == 0) /* channel A */
> + ch_addr += AD7616_RANGE_CH_A_ADDR_OFF;
> + else /* channel B */
> + ch_addr += AD7616_RANGE_CH_B_ADDR_OFF;
> +
> + /* 0b01 for 2.5v, 0b10 for 5v and 0b11 for 10v */
> + mode = AD7616_RANGE_CH_MODE(ch_index, ((val + 1) & 0b11));
>
> - indio_dev->info = &ad7606_info_sw_mode;
> + return ad7606_write_mask(st, ch_addr, AD7616_RANGE_CH_MSK(ch_index),
> + mode);
Another odd indent to fix.
> +}
> +
> +static int ad7616_write_os_sw(struct iio_dev *indio_dev, int val)
> +{
> + struct ad7606_state *st = iio_priv(indio_dev);
> +
> + return ad7606_write_mask(st, AD7616_CONFIGURATION_REGISTER,
> + AD7616_OS_MASK, val << 2);
Trivial: Odd indentation choice.
> +}
> +
> +static int ad7606_write_scale_sw(struct iio_dev *indio_dev, int ch, int val)
> +{
> + struct ad7606_state *st = iio_priv(indio_dev);
> +
> + return ad7606_write_mask(st,
> + AD7606_RANGE_CH_ADDR(ch),
In general maybe aim for more consistent indentation choice. I'd pull this time up
on the line above.
> + AD7606_RANGE_CH_MSK(ch),
> + AD7606_RANGE_CH_MODE(ch, val));
> +}
> +
> +static int ad7606_write_os_sw(struct iio_dev *indio_dev, int val)
> +{
> + struct ad7606_state *st = iio_priv(indio_dev);
> +
> + return st->bops->reg_write(st, AD7606_OS_MODE, val);
> +}
> +
> +static int ad7616_sw_mode_setup(struct iio_dev *indio_dev)
> +{
> + struct ad7606_state *st = iio_priv(indio_dev);
> + int ret;
> +
> + /*
> + * Scale can be configured individually for each channel
> + * in software mode.
> + */
> +
> + st->write_scale = ad7616_write_scale_sw;
> + st->write_os = &ad7616_write_os_sw;
> +
> + ret = st->bops->sw_mode_config(indio_dev);
> + if (ret)
> + return ret;
> +
> + /* Activate Burst mode and SEQEN MODE */
> + return ad7606_write_mask(st,
> + AD7616_CONFIGURATION_REGISTER,
Whilst moving the code, feel free to tidy up the indent for inconsistent
cases. Align this lot just after the opening bracket etc.
> + AD7616_BURST_MODE | AD7616_SEQEN_MODE,
> + AD7616_BURST_MODE | AD7616_SEQEN_MODE);
> +}
Thanks,
Jonathan
