| Message ID | 20180911150011.31964-4-afonsobordado@az8.co (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v4,1/5] iio: gyro: add support for fxas21002c | expand |
Hi Afonso,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on iio/togreg]
[also build test ERROR on v4.19-rc3 next-20180912]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Afonso-Bordado/iio-gyro-add-support-for-fxas21002c/20180912-084443
base: https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git togreg
config: i386-allmodconfig (attached as .config)
compiler: gcc-7 (Debian 7.3.0-1) 7.3.0
reproduce:
# save the attached .config to linux build tree
make ARCH=i386
All errors (new ones prefixed by >>):
>> ERROR: "__divdi3" [drivers/iio/gyro/fxas21002c.ko] undefined!
>> ERROR: "__udivdi3" [drivers/iio/gyro/fxas21002c.ko] undefined!
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
Hello Afonso, On Wed, Sep 12, 2018 at 05:26:01PM +0800, kbuild test robot wrote: > Hi Afonso, > > Thank you for the patch! Yet something to improve: > > [auto build test ERROR on iio/togreg] > [also build test ERROR on v4.19-rc3 next-20180912] > [if your patch is applied to the wrong git tree, please drop us a note to help improve the system] > > url: https://github.com/0day-ci/linux/commits/Afonso-Bordado/iio-gyro-add-support-for-fxas21002c/20180912-084443 > base: https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git togreg > config: i386-allmodconfig (attached as .config) > compiler: gcc-7 (Debian 7.3.0-1) 7.3.0 > reproduce: > # save the attached .config to linux build tree > make ARCH=i386 > > All errors (new ones prefixed by >>): > > >> ERROR: "__divdi3" [drivers/iio/gyro/fxas21002c.ko] undefined! > >> ERROR: "__udivdi3" [drivers/iio/gyro/fxas21002c.ko] undefined! Hmm. This is nasty error that had hit me back and occurs when you do 64 bit arithmetic in your code and assume it will also build for 32 bit environment(i386). https://lists.01.org/pipermail/kbuild-all/2018-July/050481.html But looking at the code seems like there is no such 64 bit division which is why 0-day didn't inform you the exact line of error unlike my case in above link. And I suspect it may be originating from your code snippet: #define FXAS21002C_SCALE(scale) (IIO_DEGREE_TO_RAD(62500U >> (scale))) and looking at the implementation: include/linux/iio/iio.h /** * IIO_DEGREE_TO_RAD() - Convert degree to rad * @deg: A value in degree * * Returns the given value converted from degree to rad */ #define IIO_DEGREE_TO_RAD(deg) (((deg) * 314159ULL + 9000000ULL) / 18000000ULL) This '/' operator might be the culprit! Just for checking that the error, remove the macro declaration `FXAS21002C_SCALE` plus its usage and re-cross compile using `make ARCH=i386`. In my case I used the `div64_s64` function handles builds for both 32/64 arch accordingly. Thanks
Hi, Thanks for your help with this. > And I suspect it may be originating from your code snippet: > > #define FXAS21002C_SCALE(scale) (IIO_DEGREE_TO_RAD(62500U >> > (scale))) > > and looking at the implementation: > > include/linux/iio/iio.h > /** > * IIO_DEGREE_TO_RAD() - Convert degree to rad > * @deg: A value in degree > * > * Returns the given value converted from degree to rad > */ > #define IIO_DEGREE_TO_RAD(deg) (((deg) * 314159ULL + 9000000ULL) / > 18000000ULL) > > This '/' operator might be the culprit! > > Just for checking that the error, remove the macro declaration > `FXAS21002C_SCALE` > plus its usage and re-cross compile using `make ARCH=i386`. > > In my case I used the `div64_s64` function handles builds for both > 32/64 > arch accordingly. Yes, this is indeed the culprit. If `div64_s64` works the same way, I wonder if the best option is to change the macro definition. I can provide a patch for this along with changing the rest of the definitions. However i would like some confirmation before starting this.
On Fri, Sep 14, 2018 at 04:26:44PM +0100, Afonso Bordado wrote: > Hi, > > Thanks for your help with this. > > > And I suspect it may be originating from your code snippet: > > > > #define FXAS21002C_SCALE(scale) (IIO_DEGREE_TO_RAD(62500U >> > > (scale))) > > > > and looking at the implementation: > > > > include/linux/iio/iio.h > > /** > > * IIO_DEGREE_TO_RAD() - Convert degree to rad > > * @deg: A value in degree > > * > > * Returns the given value converted from degree to rad > > */ > > #define IIO_DEGREE_TO_RAD(deg) (((deg) * 314159ULL + 9000000ULL) / > > 18000000ULL) > > > > This '/' operator might be the culprit! > > > > Just for checking that the error, remove the macro declaration > > `FXAS21002C_SCALE` > > plus its usage and re-cross compile using `make ARCH=i386`. > > > > In my case I used the `div64_s64` function handles builds for both > > 32/64 > > arch accordingly. > > Yes, this is indeed the culprit. If `div64_s64` works the same way, I > wonder if the best option is to change the macro definition. "....works the same way" ? Let us assume that the problem arises due to the 64 bit division, in which gcc places the __divdi3() runtime function to promote the "freestanding" environment implementation. And then linking fails due to unavailability of definitions/declarations of the aforementioned function. With `div64_s64` usgae the linker binds the definition present at lib/div64.c and the build completes successfully whether building for 32/64 bit environment. But then why didn't this error showed up in the past, in the rest of the drivers ? I see its wide usage in IIO without bug reports: himanshu@himanshu-Vostro-3559:~/linux-next$ git grep -w "IIO_DEGREE_TO_RAD" drivers/iio/ | wc -l 34 And that concludes, that there is some problem within your code! In the meantime, you can try to look the disassembly of the function where this macro is actually used and search for __divdi3/__udivdi3 function referenced in the plt. I might be wrong though... Wait a while for the experts to join in!
On Fri, 14 Sep 2018 23:00:58 +0530 Himanshu Jha <himanshujha199640@gmail.com> wrote: > On Fri, Sep 14, 2018 at 04:26:44PM +0100, Afonso Bordado wrote: > > Hi, > > > > Thanks for your help with this. > > > > > And I suspect it may be originating from your code snippet: > > > > > > #define FXAS21002C_SCALE(scale) (IIO_DEGREE_TO_RAD(62500U >> > > > (scale))) > > > > > > and looking at the implementation: > > > > > > include/linux/iio/iio.h > > > /** > > > * IIO_DEGREE_TO_RAD() - Convert degree to rad > > > * @deg: A value in degree > > > * > > > * Returns the given value converted from degree to rad > > > */ > > > #define IIO_DEGREE_TO_RAD(deg) (((deg) * 314159ULL + 9000000ULL) / > > > 18000000ULL) > > > > > > This '/' operator might be the culprit! > > > > > > Just for checking that the error, remove the macro declaration > > > `FXAS21002C_SCALE` > > > plus its usage and re-cross compile using `make ARCH=i386`. > > > > > > In my case I used the `div64_s64` function handles builds for both > > > 32/64 > > > arch accordingly. > > > > Yes, this is indeed the culprit. If `div64_s64` works the same way, I > > wonder if the best option is to change the macro definition. > > "....works the same way" ? > > Let us assume that the problem arises due to the 64 bit division, in > which gcc places the __divdi3() runtime function to promote the > "freestanding" environment implementation. And then linking fails > due to unavailability of definitions/declarations of the aforementioned > function. > > With `div64_s64` usgae the linker binds the definition present at lib/div64.c > and the build completes successfully whether building for 32/64 bit > environment. > > But then why didn't this error showed up in the past, in the rest > of the drivers ? > > I see its wide usage in IIO without bug reports: > > himanshu@himanshu-Vostro-3559:~/linux-next$ git grep -w "IIO_DEGREE_TO_RAD" drivers/iio/ | wc -l > 34 > > And that concludes, that there is some problem within your code! > > In the meantime, you can try to look the disassembly of the function > where this macro is actually used and search for __divdi3/__udivdi3 > function referenced in the plt. > > I might be wrong though... > > Wait a while for the experts to join in! > The reason it's not usually a problem is because it is a compile time constant for all the other drivers and GCC is more than happy to do it on 32 bit platforms. Now whilst it looks like you are doing something that needs to be dynamic there are actually only a few possible values so this is something we 'want' to do at compile time rather than runtime. Just add a look up table for those 4 values instead of computing the conversion every time. Thanks, Jonathan
diff --git a/drivers/iio/gyro/fxas21002c.c b/drivers/iio/gyro/fxas21002c.c index e2ce0622944b..ea9934b91789 100644 --- a/drivers/iio/gyro/fxas21002c.c +++ b/drivers/iio/gyro/fxas21002c.c @@ -7,7 +7,6 @@ * IIO driver for FXAS21002C (7-bit I2C slave address 0x20 or 0x21). * Datasheet: https://www.nxp.com/docs/en/data-sheet/FXAS21002.pdf * TODO: - * ODR / Scale Support * Scale Boost Mode * Power management * GPIO Reset @@ -66,12 +65,14 @@ #define FXAS21002C_REG_CTRL_REG2 0x14 #define FXAS21002C_REG_CTRL_REG3 0x15 -#define FXAS21002C_DEFAULT_ODR_HZ 800 +#define FXAS21002C_TEMP_SCALE 1000 -/* 0.0625 deg/s */ -#define FXAS21002C_DEFAULT_SENSITIVITY IIO_DEGREE_TO_RAD(62500) -#define FXAS21002C_TEMP_SCALE 1000 +#define FXAS21002C_SCALE(scale) (IIO_DEGREE_TO_RAD(62500 >> (scale))) + +#define FXAS21002C_SAMPLE_FREQ(odr) (800 >> (odr)) +#define FXAS21002C_SAMPLE_FREQ_MICRO(odr) ( \ + ((odr) == FXAS21002C_ODR_12_5) ? 500000 : 0) enum { ID_FXAS21002C, @@ -89,6 +90,25 @@ struct fxas21002c_data { struct regmap *regmap; }; +enum fxas21002c_scale { + FXAS21002C_SCALE_62MDPS, + FXAS21002C_SCALE_31MDPS, + FXAS21002C_SCALE_15MDPS, + FXAS21002C_SCALE_7MDPS, + __FXAS21002C_SCALE_MAX, +}; + +enum fxas21002c_odr { + FXAS21002C_ODR_800, + FXAS21002C_ODR_400, + FXAS21002C_ODR_200, + FXAS21002C_ODR_100, + FXAS21002C_ODR_50, + FXAS21002C_ODR_25, + FXAS21002C_ODR_12_5, + __FXAS21002C_ODR_MAX, +}; + static const struct regmap_range fxas21002c_writable_ranges[] = { regmap_reg_range(FXAS21002C_REG_F_SETUP, FXAS21002C_REG_F_SETUP), regmap_reg_range(FXAS21002C_REG_CTRL_REG0, FXAS21002C_REG_RT_CFG), @@ -261,6 +281,49 @@ static int fxas21002c_read_oneshot(struct fxas21002c_data *data, } } +static int fxas21002c_scale_read(struct fxas21002c_data *data, int *val, + int *val2) +{ + int ret; + unsigned int raw; + + ret = regmap_read(data->regmap, FXAS21002C_REG_CTRL_REG0, &raw); + if (ret) + return ret; + + raw &= FXAS21002C_SCALE_MASK; + + *val = 0; + *val2 = FXAS21002C_SCALE(raw); + + return IIO_VAL_INT_PLUS_MICRO; +} + +static int fxas21002c_odr_read(struct fxas21002c_data *data, int *val, + int *val2) +{ + int ret; + unsigned int raw; + + ret = regmap_read(data->regmap, FXAS21002C_REG_CTRL_REG1, &raw); + if (ret) + return ret; + + raw = (raw & FXAS21002C_ODR_MASK) >> FXAS21002C_ODR_SHIFT; + + /* + * We don't use this mode but according to the datasheet its + * also a 12.5Hz + */ + if (raw == 7) + raw = FXAS21002C_ODR_12_5; + + *val = FXAS21002C_SAMPLE_FREQ(raw); + *val2 = FXAS21002C_SAMPLE_FREQ_MICRO(raw); + + return IIO_VAL_INT_PLUS_MICRO; +} + static int fxas21002c_read_raw(struct iio_dev *indio_dev, struct iio_chan_spec const *chan, int *val, int *val2, long mask) @@ -273,10 +336,7 @@ static int fxas21002c_read_raw(struct iio_dev *indio_dev, case IIO_CHAN_INFO_SCALE: switch (chan->type) { case IIO_ANGL_VEL: - *val = 0; - *val2 = FXAS21002C_DEFAULT_SENSITIVITY; - - return IIO_VAL_INT_PLUS_MICRO; + return fxas21002c_scale_read(data, val, val2); case IIO_TEMP: *val = FXAS21002C_TEMP_SCALE; @@ -288,16 +348,75 @@ static int fxas21002c_read_raw(struct iio_dev *indio_dev, if (chan->type != IIO_ANGL_VEL) return -EINVAL; - *val = FXAS21002C_DEFAULT_ODR_HZ; - - return IIO_VAL_INT; + return fxas21002c_odr_read(data, val, val2); } return -EINVAL; } + +static int fxas21002c_write_raw(struct iio_dev *indio_dev, + struct iio_chan_spec const *chan, int val, + int val2, long mask) +{ + struct fxas21002c_data *data = iio_priv(indio_dev); + int ret = -EINVAL; + int i; + + switch (mask) { + case IIO_CHAN_INFO_SAMP_FREQ: + for (i = 0; i < __FXAS21002C_ODR_MAX; i++) { + if (FXAS21002C_SAMPLE_FREQ(i) == val && + FXAS21002C_SAMPLE_FREQ_MICRO(i) == val2) + break; + } + + if (i == __FXAS21002C_ODR_MAX) + break; + + return regmap_update_bits(data->regmap, + FXAS21002C_REG_CTRL_REG1, + FXAS21002C_ODR_MASK, + i << FXAS21002C_ODR_SHIFT); + case IIO_CHAN_INFO_SCALE: + for (i = 0; i < __FXAS21002C_SCALE_MAX; i++) { + if (val == 0 && FXAS21002C_SCALE(i) == val2) + break; + } + + if (i == __FXAS21002C_SCALE_MAX) + break; + + return regmap_update_bits(data->regmap, + FXAS21002C_REG_CTRL_REG0, + FXAS21002C_SCALE_MASK, i); + } + + return ret; +} + +static IIO_CONST_ATTR(anglevel_scale_available, + "0.001090831 " /* 62.5 mdps */ + "0.000545415 " /* 31.25 mdps */ + "0.000272708 " /* 15.625 mdps */ + "0.000136354"); /* 7.8125 mdps */ + +static IIO_CONST_ATTR_SAMP_FREQ_AVAIL("800 400 200 100 50 25 12.5"); + +static struct attribute *fxas21002c_attributes[] = { + &iio_const_attr_anglevel_scale_available.dev_attr.attr, + &iio_const_attr_sampling_frequency_available.dev_attr.attr, + NULL +}; + +static const struct attribute_group fxas21002c_attribute_group = { + .attrs = fxas21002c_attributes, +}; + static const struct iio_info fxas21002c_info = { .read_raw = fxas21002c_read_raw, + .write_raw = fxas21002c_write_raw, + .attrs = &fxas21002c_attribute_group, }; static int fxas21002c_probe(struct i2c_client *client,
This patch adds support for reading/writing ODR/Scale We don't support the scale boost modes. Signed-off-by: Afonso Bordado <afonsobordado@az8.co> --- drivers/iio/gyro/fxas21002c.c | 143 +++++++++++++++++++++++++++++++--- 1 file changed, 131 insertions(+), 12 deletions(-)