Use the BITS_TO_BYTES() macro instead of dividing by 8 to convert bits
to bytes.
This makes it more obvious what unit conversion is taking place.
In one instance, we also avoid the temporary assignment to a variable
as it was confusing that reg_size was being used with two different
units (bits and bytes).
scan_type is factored out to reduce line wrapping.
Signed-off-by: David Lechner <dlechner@baylibre.com>
---
drivers/iio/adc/ad_sigma_delta.c | 15 ++++++++-------
1 file changed, 8 insertions(+), 7 deletions(-)
diff --git a/drivers/iio/adc/ad_sigma_delta.c b/drivers/iio/adc/ad_sigma_delta.c
index 5362157966d89cbf0e602716aaaf0b78f3921b11..64ed8aeb71f7c6ca19fff0438fa5ccce0c1d8f8f 100644
--- a/drivers/iio/adc/ad_sigma_delta.c
+++ b/drivers/iio/adc/ad_sigma_delta.c
@@ -7,6 +7,7 @@
*/
#include <linux/align.h>
+#include <linux/bitops.h>
#include <linux/device.h>
#include <linux/err.h>
#include <linux/interrupt.h>
@@ -190,7 +191,7 @@ int ad_sd_reset(struct ad_sigma_delta *sigma_delta)
u8 *buf;
int ret;
- size = DIV_ROUND_UP(reset_length, 8);
+ size = BITS_TO_BYTES(reset_length);
buf = kcalloc(size, sizeof(*buf), GFP_KERNEL);
if (!buf)
return -ENOMEM;
@@ -419,7 +420,7 @@ int ad_sigma_delta_single_conversion(struct iio_dev *indio_dev,
data_reg = AD_SD_REG_DATA;
ret = ad_sd_read_reg(sigma_delta, data_reg,
- DIV_ROUND_UP(chan->scan_type.realbits + chan->scan_type.shift, 8),
+ BITS_TO_BYTES(chan->scan_type.realbits + chan->scan_type.shift),
&raw_sample);
out:
@@ -453,6 +454,7 @@ EXPORT_SYMBOL_NS_GPL(ad_sigma_delta_single_conversion, "IIO_AD_SIGMA_DELTA");
static int ad_sd_buffer_postenable(struct iio_dev *indio_dev)
{
struct ad_sigma_delta *sigma_delta = iio_device_get_drvdata(indio_dev);
+ const struct iio_scan_type *scan_type = &indio_dev->channels[0].scan_type;
unsigned int i, slot, samples_buf_size;
unsigned int channel;
u8 *samples_buf;
@@ -488,7 +490,7 @@ static int ad_sd_buffer_postenable(struct iio_dev *indio_dev)
return ret;
}
- samples_buf_size = ALIGN(slot * indio_dev->channels[0].scan_type.storagebits / 8, 8);
+ samples_buf_size = ALIGN(slot * BITS_TO_BYTES(scan_type->storagebits), 8);
samples_buf_size += sizeof(s64);
samples_buf = devm_krealloc(&sigma_delta->spi->dev, sigma_delta->samples_buf,
samples_buf_size, GFP_KERNEL);
@@ -543,6 +545,7 @@ static irqreturn_t ad_sd_trigger_handler(int irq, void *p)
{
struct iio_poll_func *pf = p;
struct iio_dev *indio_dev = pf->indio_dev;
+ const struct iio_scan_type *scan_type = &indio_dev->channels[0].scan_type;
struct ad_sigma_delta *sigma_delta = iio_device_get_drvdata(indio_dev);
u8 *data = sigma_delta->rx_buf;
unsigned int transfer_size;
@@ -552,9 +555,7 @@ static irqreturn_t ad_sd_trigger_handler(int irq, void *p)
unsigned int reg_size;
unsigned int data_reg;
- reg_size = indio_dev->channels[0].scan_type.realbits +
- indio_dev->channels[0].scan_type.shift;
- reg_size = DIV_ROUND_UP(reg_size, 8);
+ reg_size = BITS_TO_BYTES(scan_type->realbits + scan_type->shift);
if (sigma_delta->info->data_reg != 0)
data_reg = sigma_delta->info->data_reg;
@@ -616,7 +617,7 @@ static irqreturn_t ad_sd_trigger_handler(int irq, void *p)
}
}
- sample_size = indio_dev->channels[0].scan_type.storagebits / 8;
+ sample_size = BITS_TO_BYTES(scan_type->storagebits);
sample_pos = sample_size * sigma_delta->current_slot;
memcpy(&sigma_delta->samples_buf[sample_pos], data, sample_size);
sigma_delta->current_slot++;
--
2.43.0On Fri, 27 Jun 2025 18:40:00 -0500 David Lechner <dlechner@baylibre.com> wrote: > Use the BITS_TO_BYTES() macro instead of dividing by 8 to convert bits > to bytes. > > This makes it more obvious what unit conversion is taking place. > > In one instance, we also avoid the temporary assignment to a variable > as it was confusing that reg_size was being used with two different > units (bits and bytes). > > scan_type is factored out to reduce line wrapping. > > Signed-off-by: David Lechner <dlechner@baylibre.com> > > - samples_buf_size = ALIGN(slot * indio_dev->channels[0].scan_type.storagebits / 8, 8); > + samples_buf_size = ALIGN(slot * BITS_TO_BYTES(scan_type->storagebits), 8); Ah. You do it here. Fair enough and no problem wrt to patch 1 then. > samples_buf_size += sizeof(s64); > samples_buf = devm_krealloc(&sigma_delta->spi->dev, sigma_delta->samples_buf, > samples_buf_size, GFP_KERNEL);
On Sat, Jun 28, 2025 at 03:56:43PM +0100, Jonathan Cameron wrote: > On Fri, 27 Jun 2025 18:40:00 -0500 > David Lechner <dlechner@baylibre.com> wrote: ... > > - samples_buf_size = ALIGN(slot * indio_dev->channels[0].scan_type.storagebits / 8, 8); > > + samples_buf_size = ALIGN(slot * BITS_TO_BYTES(scan_type->storagebits), 8); > > Ah. You do it here. Fair enough and no problem wrt to patch 1 then. Hmm... Should the second 8 be something like sizeof(unsigned long lone) for semantic distinguishing with 8-bit bytes? -- With Best Regards, Andy Shevchenko
On 6/30/25 3:59 AM, Andy Shevchenko wrote: > On Sat, Jun 28, 2025 at 03:56:43PM +0100, Jonathan Cameron wrote: >> On Fri, 27 Jun 2025 18:40:00 -0500 >> David Lechner <dlechner@baylibre.com> wrote: > > ... > >>> - samples_buf_size = ALIGN(slot * indio_dev->channels[0].scan_type.storagebits / 8, 8); >>> + samples_buf_size = ALIGN(slot * BITS_TO_BYTES(scan_type->storagebits), 8); >> >> Ah. You do it here. Fair enough and no problem wrt to patch 1 then. > > Hmm... Should the second 8 be something like sizeof(unsigned long lone) for > semantic distinguishing with 8-bit bytes? > Yeah, I considered to use sizeof(s64) to match the next line, but it it seems like a separate change, so in the end I decided against doing it in this patch and it seems too small of a thing for a separate patch.
On Mon, Jun 30, 2025 at 08:33:59AM -0500, David Lechner wrote: > On 6/30/25 3:59 AM, Andy Shevchenko wrote: > > On Sat, Jun 28, 2025 at 03:56:43PM +0100, Jonathan Cameron wrote: > >> On Fri, 27 Jun 2025 18:40:00 -0500 > >> David Lechner <dlechner@baylibre.com> wrote: ... > >>> - samples_buf_size = ALIGN(slot * indio_dev->channels[0].scan_type.storagebits / 8, 8); > >>> + samples_buf_size = ALIGN(slot * BITS_TO_BYTES(scan_type->storagebits), 8); > >> > >> Ah. You do it here. Fair enough and no problem wrt to patch 1 then. > > > > Hmm... Should the second 8 be something like sizeof(unsigned long lone) for > > semantic distinguishing with 8-bit bytes? > > Yeah, I considered to use sizeof(s64) to match the next line, but it > it seems like a separate change, so in the end I decided against doing > it in this patch and it seems too small of a thing for a separate patch. The problem in not the size of the change, the problem is that semantically those 8:s are _different_ and code readability will be much better if we make them so explicitly. -- With Best Regards, Andy Shevchenko
On Tue, 1 Jul 2025 16:47:02 +0300 Andy Shevchenko <andriy.shevchenko@intel.com> wrote: > On Mon, Jun 30, 2025 at 08:33:59AM -0500, David Lechner wrote: > > On 6/30/25 3:59 AM, Andy Shevchenko wrote: > > > On Sat, Jun 28, 2025 at 03:56:43PM +0100, Jonathan Cameron wrote: > > >> On Fri, 27 Jun 2025 18:40:00 -0500 > > >> David Lechner <dlechner@baylibre.com> wrote: > > ... > > > >>> - samples_buf_size = ALIGN(slot * indio_dev->channels[0].scan_type.storagebits / 8, 8); > > >>> + samples_buf_size = ALIGN(slot * BITS_TO_BYTES(scan_type->storagebits), 8); > > >> > > >> Ah. You do it here. Fair enough and no problem wrt to patch 1 then. > > > > > > Hmm... Should the second 8 be something like sizeof(unsigned long lone) for > > > semantic distinguishing with 8-bit bytes? > > > > Yeah, I considered to use sizeof(s64) to match the next line, but it > > it seems like a separate change, so in the end I decided against doing > > it in this patch and it seems too small of a thing for a separate patch. > > The problem in not the size of the change, the problem is that semantically > those 8:s are _different_ and code readability will be much better if we make > them so explicitly. Agreed. It's not so bad once we are down to just one magic 8 (ball :) but definitely makes sense to give them both explicit meaning. A tiny follow up patch, or rolling it in here with a comment in the patch description would both be fine. Jonathan >
© 2016 - 2026 Red Hat, Inc.