| Message ID | 20181018114915.GA30276@x220.localdomain (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2,1/1] staging: iio: adc: ad7280a: use crc8.h API to build crc table | expand |
On 10/18/2018 01:49 PM, Slawomir Stepien wrote: > The custom build function ad7280_crc8_build_table is not needed. The > crc8_populate_msb function from linux/crc8.h will build the same crc > table. > > Signed-off-by: Slawomir Stepien <sst@poczta.fm> This looks good, thanks. Acked-by: Lars-Peter Clausen <lars@metafoo.de> I supposed the code could be improved a bit further in a follow up patch by only having one global table since it will be the same for all instances. > --- > Since v1: > * Fix typo in commit message. > --- > drivers/staging/iio/adc/ad7280a.c | 24 +++--------------------- > 1 file changed, 3 insertions(+), 21 deletions(-) > > diff --git a/drivers/staging/iio/adc/ad7280a.c b/drivers/staging/iio/adc/ad7280a.c > index b736275c10f5..c701f07853fa 100644 > --- a/drivers/staging/iio/adc/ad7280a.c > +++ b/drivers/staging/iio/adc/ad7280a.c > @@ -6,6 +6,7 @@ > * Licensed under the GPL-2. > */ > > +#include <linux/crc8.h> > #include <linux/device.h> > #include <linux/kernel.h> > #include <linux/slab.h> > @@ -121,8 +122,6 @@ static unsigned int ad7280a_devaddr(unsigned int addr) > * P(x) = x^8 + x^5 + x^3 + x^2 + x^1 + x^0 = 0b100101111 => 0x2F > */ > #define POLYNOM 0x2F > -#define POLYNOM_ORDER 8 > -#define HIGHBIT (1 << (POLYNOM_ORDER - 1)) > > struct ad7280_state { > struct spi_device *spi; > @@ -131,7 +130,7 @@ struct ad7280_state { > int slave_num; > int scan_cnt; > int readback_delay_us; > - unsigned char crc_tab[256]; > + unsigned char crc_tab[CRC8_TABLE_SIZE]; > unsigned char ctrl_hb; > unsigned char ctrl_lb; > unsigned char cell_threshhigh; > @@ -144,23 +143,6 @@ struct ad7280_state { > __be32 buf[2] ____cacheline_aligned; > }; > > -static void ad7280_crc8_build_table(unsigned char *crc_tab) > -{ > - unsigned char bit, crc; > - int cnt, i; > - > - for (cnt = 0; cnt < 256; cnt++) { > - crc = cnt; > - for (i = 0; i < 8; i++) { > - bit = crc & HIGHBIT; > - crc <<= 1; > - if (bit) > - crc ^= POLYNOM; > - } > - crc_tab[cnt] = crc; > - } > -} > - > static unsigned char ad7280_calc_crc8(unsigned char *crc_tab, unsigned int val) > { > unsigned char crc; > @@ -857,7 +839,7 @@ static int ad7280_probe(struct spi_device *spi) > if (!pdata) > pdata = &ad7793_default_pdata; > > - ad7280_crc8_build_table(st->crc_tab); > + crc8_populate_msb(st->crc_tab, POLYNOM); > > st->spi->max_speed_hz = AD7280A_MAX_SPI_CLK_HZ; > st->spi->mode = SPI_MODE_1; >
On paź 18, 2018 19:31, Lars-Peter Clausen wrote: > On 10/18/2018 01:49 PM, Slawomir Stepien wrote: > > The custom build function ad7280_crc8_build_table is not needed. The > > crc8_populate_msb function from linux/crc8.h will build the same crc > > table. > > > > Signed-off-by: Slawomir Stepien <sst@poczta.fm> > > This looks good, thanks. > > Acked-by: Lars-Peter Clausen <lars@metafoo.de> Thank you. > I supposed the code could be improved a bit further in a follow up patch by > only having one global table since it will be the same for all instances. Yes that's a good idea. How should I proceed? v3 or a separate commit based on the changes in this commit? I think separate commit will be better, so I can make a good commit message...or maybe put it all in a patch series with your ack added to this change? Sorry I don't know the rules in such cases.
On 10/18/2018 08:06 PM, Slawomir Stepien wrote: > On paź 18, 2018 19:31, Lars-Peter Clausen wrote: >> On 10/18/2018 01:49 PM, Slawomir Stepien wrote: >>> The custom build function ad7280_crc8_build_table is not needed. The >>> crc8_populate_msb function from linux/crc8.h will build the same crc >>> table. >>> >>> Signed-off-by: Slawomir Stepien <sst@poczta.fm> >> >> This looks good, thanks. >> >> Acked-by: Lars-Peter Clausen <lars@metafoo.de> > > Thank you. > >> I supposed the code could be improved a bit further in a follow up patch by >> only having one global table since it will be the same for all instances. > > Yes that's a good idea. > How should I proceed? v3 or a separate commit based on the changes in this > commit? I think separate commit will be better, so I can make a good commit > message...or maybe put it all in a patch series with your ack added to this > change? Sorry I don't know the rules in such cases. I think Jonathan will pick this patch up soon. I'd wait until the patch has been applied and then send your next patch with further improvements to the driver.
On paź 18, 2018 13:49, Slawomir Stepien wrote: > The custom build function ad7280_crc8_build_table is not needed. The > crc8_populate_msb function from linux/crc8.h will build the same crc > table. > > Signed-off-by: Slawomir Stepien <sst@poczta.fm> > --- > Since v1: > * Fix typo in commit message. > --- > drivers/staging/iio/adc/ad7280a.c | 24 +++--------------------- > 1 file changed, 3 insertions(+), 21 deletions(-) > > diff --git a/drivers/staging/iio/adc/ad7280a.c b/drivers/staging/iio/adc/ad7280a.c > index b736275c10f5..c701f07853fa 100644 > --- a/drivers/staging/iio/adc/ad7280a.c > +++ b/drivers/staging/iio/adc/ad7280a.c > @@ -6,6 +6,7 @@ > * Licensed under the GPL-2. > */ > > +#include <linux/crc8.h> > #include <linux/device.h> > #include <linux/kernel.h> > #include <linux/slab.h> > @@ -121,8 +122,6 @@ static unsigned int ad7280a_devaddr(unsigned int addr) > * P(x) = x^8 + x^5 + x^3 + x^2 + x^1 + x^0 = 0b100101111 => 0x2F > */ > #define POLYNOM 0x2F > -#define POLYNOM_ORDER 8 > -#define HIGHBIT (1 << (POLYNOM_ORDER - 1)) > > struct ad7280_state { > struct spi_device *spi; > @@ -131,7 +130,7 @@ struct ad7280_state { > int slave_num; > int scan_cnt; > int readback_delay_us; > - unsigned char crc_tab[256]; > + unsigned char crc_tab[CRC8_TABLE_SIZE]; > unsigned char ctrl_hb; > unsigned char ctrl_lb; > unsigned char cell_threshhigh; > @@ -144,23 +143,6 @@ struct ad7280_state { > __be32 buf[2] ____cacheline_aligned; > }; > > -static void ad7280_crc8_build_table(unsigned char *crc_tab) > -{ > - unsigned char bit, crc; > - int cnt, i; > - > - for (cnt = 0; cnt < 256; cnt++) { > - crc = cnt; > - for (i = 0; i < 8; i++) { > - bit = crc & HIGHBIT; > - crc <<= 1; > - if (bit) > - crc ^= POLYNOM; > - } > - crc_tab[cnt] = crc; > - } > -} > - > static unsigned char ad7280_calc_crc8(unsigned char *crc_tab, unsigned int val) > { > unsigned char crc; > @@ -857,7 +839,7 @@ static int ad7280_probe(struct spi_device *spi) > if (!pdata) > pdata = &ad7793_default_pdata; > > - ad7280_crc8_build_table(st->crc_tab); > + crc8_populate_msb(st->crc_tab, POLYNOM); Ups...this function requires CONFIG_CRC8. I need to make a v3 with Kbuild change. > st->spi->max_speed_hz = AD7280A_MAX_SPI_CLK_HZ; > st->spi->mode = SPI_MODE_1;
diff --git a/drivers/staging/iio/adc/ad7280a.c b/drivers/staging/iio/adc/ad7280a.c index b736275c10f5..c701f07853fa 100644 --- a/drivers/staging/iio/adc/ad7280a.c +++ b/drivers/staging/iio/adc/ad7280a.c @@ -6,6 +6,7 @@ * Licensed under the GPL-2. */ +#include <linux/crc8.h> #include <linux/device.h> #include <linux/kernel.h> #include <linux/slab.h> @@ -121,8 +122,6 @@ static unsigned int ad7280a_devaddr(unsigned int addr) * P(x) = x^8 + x^5 + x^3 + x^2 + x^1 + x^0 = 0b100101111 => 0x2F */ #define POLYNOM 0x2F -#define POLYNOM_ORDER 8 -#define HIGHBIT (1 << (POLYNOM_ORDER - 1)) struct ad7280_state { struct spi_device *spi; @@ -131,7 +130,7 @@ struct ad7280_state { int slave_num; int scan_cnt; int readback_delay_us; - unsigned char crc_tab[256]; + unsigned char crc_tab[CRC8_TABLE_SIZE]; unsigned char ctrl_hb; unsigned char ctrl_lb; unsigned char cell_threshhigh; @@ -144,23 +143,6 @@ struct ad7280_state { __be32 buf[2] ____cacheline_aligned; }; -static void ad7280_crc8_build_table(unsigned char *crc_tab) -{ - unsigned char bit, crc; - int cnt, i; - - for (cnt = 0; cnt < 256; cnt++) { - crc = cnt; - for (i = 0; i < 8; i++) { - bit = crc & HIGHBIT; - crc <<= 1; - if (bit) - crc ^= POLYNOM; - } - crc_tab[cnt] = crc; - } -} - static unsigned char ad7280_calc_crc8(unsigned char *crc_tab, unsigned int val) { unsigned char crc; @@ -857,7 +839,7 @@ static int ad7280_probe(struct spi_device *spi) if (!pdata) pdata = &ad7793_default_pdata; - ad7280_crc8_build_table(st->crc_tab); + crc8_populate_msb(st->crc_tab, POLYNOM); st->spi->max_speed_hz = AD7280A_MAX_SPI_CLK_HZ; st->spi->mode = SPI_MODE_1;
The custom build function ad7280_crc8_build_table is not needed. The crc8_populate_msb function from linux/crc8.h will build the same crc table. Signed-off-by: Slawomir Stepien <sst@poczta.fm> --- Since v1: * Fix typo in commit message. --- drivers/staging/iio/adc/ad7280a.c | 24 +++--------------------- 1 file changed, 3 insertions(+), 21 deletions(-)