| Message ID | 8f5f58c9bf0f4006fabd01b5564af071d20f2a2d.1659909060.git.jahau@rocketmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add support for magnetometer Yamaha YAS537 | expand |
Hi Jakob,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on v5.19]
[also build test ERROR on next-20220805]
[cannot apply to jic23-iio/togreg linus/master]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Jakob-Hauser/iio-magnetometer-yas530-Change-data-type-of-hard_offsets-to-signed/20220808-080209
base: 3d7cb6b04c3f3115719235cc6866b10326de34cd
config: hexagon-randconfig-r045-20220807 (https://download.01.org/0day-ci/archive/20220808/202208081346.EWHUWCSa-lkp@intel.com/config)
compiler: clang version 16.0.0 (https://github.com/llvm/llvm-project 5f1c7e2cc5a3c07cbc2412e851a7283c1841f520)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/2e5a660a127b0fa7ca71e3e30356dc2254ec13eb
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Jakob-Hauser/iio-magnetometer-yas530-Change-data-type-of-hard_offsets-to-signed/20220808-080209
git checkout 2e5a660a127b0fa7ca71e3e30356dc2254ec13eb
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=hexagon SHELL=/bin/bash drivers/iio/magnetometer/
If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <lkp@intel.com>
All errors (new ones prefixed by >>):
>> drivers/iio/magnetometer/yamaha-yas530.c:933:19: error: initializer element is not a compile-time constant
.product_name = yas5xx_product_name[yas530],
^~~~~~~~~~~~~~~~~~~~~~~~~~~
1 error generated.
vim +933 drivers/iio/magnetometer/yamaha-yas530.c
929
930 static const struct yas5xx_chip_info yas5xx_chip_info_tbl[] = {
931 [yas530] = {
932 .devid = YAS530_DEVICE_ID,
> 933 .product_name = yas5xx_product_name[yas530],
934 .version_name = yas5xx_version_names[yas530],
935 },
936 [yas532] = {
937 .devid = YAS532_DEVICE_ID,
938 .product_name = yas5xx_product_name[yas532],
939 .version_name = yas5xx_version_names[yas532],
940 },
941 [yas533] = {
942 .devid = YAS532_DEVICE_ID,
943 .product_name = yas5xx_product_name[yas533],
944 .version_name = yas5xx_version_names[yas533],
945 },
946 };
947
On Mon, Aug 8, 2022 at 7:40 AM kernel test robot <lkp@intel.com> wrote: ... > All errors (new ones prefixed by >>): > > >> drivers/iio/magnetometer/yamaha-yas530.c:933:19: error: initializer element is not a compile-time constant > .product_name = yas5xx_product_name[yas530], > ^~~~~~~~~~~~~~~~~~~~~~~~~~~ > 1 error generated. What?! The yas530 is a part of the enum, how come that compiler can't see this? Looks like a Clang bug. > 930 static const struct yas5xx_chip_info yas5xx_chip_info_tbl[] = { > 931 [yas530] = { > 932 .devid = YAS530_DEVICE_ID, > > 933 .product_name = yas5xx_product_name[yas530], > 934 .version_name = yas5xx_version_names[yas530], > 935 }, > 936 [yas532] = { > 937 .devid = YAS532_DEVICE_ID, > 938 .product_name = yas5xx_product_name[yas532], > 939 .version_name = yas5xx_version_names[yas532], > 940 }, > 941 [yas533] = { > 942 .devid = YAS532_DEVICE_ID, > 943 .product_name = yas5xx_product_name[yas533], > 944 .version_name = yas5xx_version_names[yas533], > 945 }, > 946 };
On Mon, Aug 8, 2022 at 1:07 AM Jakob Hauser <jahau@rocketmail.com> wrote: > > This commit introduces the "chip_info" structure approach for better variant > handling. Read "Submitting Patches" in the chapter which mentions "This patch" pattern and fix the above accordingly. > The variant to be used is now chosen by the Device Tree (enum "chip_ids"), > not by the chip ID in the register. However, there is a check to make sure > they match (using integer "id_check"). ... > +enum chip_ids { > + yas530, > + yas532, > + yas533, > +}; So, it's an error from Clang... Workaround can be simply to use set of #define:s instead. ... > + if (id_check != yas5xx->chip_info->devid) { > + switch (yas5xx->chip_info->devid) { You can make these kind of lines shorter by introducing a temporary variable: struct ... *ci = yaas5xx->chip_info;
+Cc: clang people On Mon, Aug 8, 2022 at 1:18 PM Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > On Mon, Aug 8, 2022 at 7:40 AM kernel test robot <lkp@intel.com> wrote: > > ... > > > All errors (new ones prefixed by >>): > > > > >> drivers/iio/magnetometer/yamaha-yas530.c:933:19: error: initializer element is not a compile-time constant > > .product_name = yas5xx_product_name[yas530], > > ^~~~~~~~~~~~~~~~~~~~~~~~~~~ > > 1 error generated. > > What?! > > The yas530 is a part of the enum, how come that compiler can't see > this? Looks like a Clang bug. > > > 930 static const struct yas5xx_chip_info yas5xx_chip_info_tbl[] = { > > 931 [yas530] = { > > 932 .devid = YAS530_DEVICE_ID, > > > 933 .product_name = yas5xx_product_name[yas530], > > 934 .version_name = yas5xx_version_names[yas530], > > 935 }, > > 936 [yas532] = { > > 937 .devid = YAS532_DEVICE_ID, > > 938 .product_name = yas5xx_product_name[yas532], > > 939 .version_name = yas5xx_version_names[yas532], > > 940 }, > > 941 [yas533] = { > > 942 .devid = YAS532_DEVICE_ID, > > 943 .product_name = yas5xx_product_name[yas533], > > 944 .version_name = yas5xx_version_names[yas533], > > 945 }, > > 946 };
On Mon, Aug 8, 2022 at 1:07 AM Jakob Hauser <jahau@rocketmail.com> wrote: ... > + yas5xx->chip = id->driver_data; > + yas5xx->chip_info = &yas5xx_chip_info_tbl[yas5xx->chip]; I don't see how ->chip is being used, I would expect it is the part of chip_info, if it's really needed. That said, please make it directly a pointer, so the above becomes: ... ->chip_info = (const struct ...)id->driver_data; ... > + {"yas530", yas530 }, > + {"yas532", yas532 }, > + {"yas533", yas533 }, Read above and here: yas53... ==> (kernel_ulong_t)&...[yas53...]
Hi Andy, On Mon, Aug 08, 2022 at 01:18:06PM, +0200, Andy Shevchenko wrote: > On Mon, Aug 8, 2022 at 7:40 AM kernel test robot <lkp@intel.com> wrote: > > ... > > > All errors (new ones prefixed by >>): > > > > >> drivers/iio/magnetometer/yamaha-yas530.c:933:19: error: initializer element is not a compile-time constant > > .product_name = yas5xx_product_name[yas530], > > ^~~~~~~~~~~~~~~~~~~~~~~~~~~ > > 1 error generated. > > What?! > > The yas530 is a part of the enum, how come that compiler can't see > this? Looks like a Clang bug. That is not what clang is complaining about here, you'll see the same error even if you used '0', '1', or '2' here: drivers/iio/magnetometer/yamaha-yas530.c:933:19: error: initializer element is not a compile-time constant .product_name = yas5xx_product_name[0], ^~~~~~~~~~~~~~~~~~~~~~ 1 error generated. It is complaining that the initializer element ('yas5xx_product_name[yas530]', rather than just 'yas530') is not constant, which is a true complaint if I am reading C11 standard 6.6.7 correctly. GCC 8+ has chosen to accept const structures as constant expressions in designated initializers, which it is allowed to do per 6.6.10. Nick did have a patch to try and match this behavior in clang but the work that was requested doesn't seem to be trivial so it was never finalized: https://reviews.llvm.org/D76096 You'll see the same error with GCC 7: drivers/iio/magnetometer/yamaha-yas530.c:933:19: error: initializer element is not constant .product_name = yas5xx_product_name[yas530], ^~~~~~~~~~~~~~~~~~~ drivers/iio/magnetometer/yamaha-yas530.c:933:19: note: (near initialization for ‘yas5xx_chip_info_tbl[0].product_name’) drivers/iio/magnetometer/yamaha-yas530.c:938:19: error: initializer element is not constant .product_name = yas5xx_product_name[yas532], ^~~~~~~~~~~~~~~~~~~ drivers/iio/magnetometer/yamaha-yas530.c:938:19: note: (near initialization for ‘yas5xx_chip_info_tbl[1].product_name’) drivers/iio/magnetometer/yamaha-yas530.c:943:19: error: initializer element is not constant .product_name = yas5xx_product_name[yas533], ^~~~~~~~~~~~~~~~~~~ drivers/iio/magnetometer/yamaha-yas530.c:943:19: note: (near initialization for ‘yas5xx_chip_info_tbl[2].product_name’) Cheers, Nathan > > 930 static const struct yas5xx_chip_info yas5xx_chip_info_tbl[] = { > > 931 [yas530] = { > > 932 .devid = YAS530_DEVICE_ID, > > > 933 .product_name = yas5xx_product_name[yas530], > > 934 .version_name = yas5xx_version_names[yas530], > > 935 }, > > 936 [yas532] = { > > 937 .devid = YAS532_DEVICE_ID, > > 938 .product_name = yas5xx_product_name[yas532], > > 939 .version_name = yas5xx_version_names[yas532], > > 940 }, > > 941 [yas533] = { > > 942 .devid = YAS532_DEVICE_ID, > > 943 .product_name = yas5xx_product_name[yas533], > > 944 .version_name = yas5xx_version_names[yas533], > > 945 }, > > 946 }; > > -- > With Best Regards, > Andy Shevchenko >
On Mon, Aug 8, 2022 at 5:59 PM Nathan Chancellor <nathan@kernel.org> wrote: > On Mon, Aug 08, 2022 at 01:18:06PM, +0200, Andy Shevchenko wrote: > > On Mon, Aug 8, 2022 at 7:40 AM kernel test robot <lkp@intel.com> wrote: > > > > ... > > > > > All errors (new ones prefixed by >>): > > > > > > >> drivers/iio/magnetometer/yamaha-yas530.c:933:19: error: initializer element is not a compile-time constant > > > .product_name = yas5xx_product_name[yas530], > > > ^~~~~~~~~~~~~~~~~~~~~~~~~~~ > > > 1 error generated. > > > > What?! > > > > The yas530 is a part of the enum, how come that compiler can't see > > this? Looks like a Clang bug. > > That is not what clang is complaining about here, you'll see the same > error even if you used '0', '1', or '2' here: > > drivers/iio/magnetometer/yamaha-yas530.c:933:19: error: initializer element is not a compile-time constant > .product_name = yas5xx_product_name[0], > ^~~~~~~~~~~~~~~~~~~~~~ > 1 error generated. > > It is complaining that the initializer element > ('yas5xx_product_name[yas530]', rather than just 'yas530') is not > constant, which is a true complaint if I am reading C11 standard 6.6.7 > correctly. > > GCC 8+ has chosen to accept const structures as constant expressions in > designated initializers, which it is allowed to do per 6.6.10. Nick did > have a patch to try and match this behavior in clang but the work that > was requested doesn't seem to be trivial so it was never finalized: > https://reviews.llvm.org/D76096 > > You'll see the same error with GCC 7: > > drivers/iio/magnetometer/yamaha-yas530.c:933:19: error: initializer element is not constant > .product_name = yas5xx_product_name[yas530], > ^~~~~~~~~~~~~~~~~~~ > drivers/iio/magnetometer/yamaha-yas530.c:933:19: note: (near initialization for ‘yas5xx_chip_info_tbl[0].product_name’) > drivers/iio/magnetometer/yamaha-yas530.c:938:19: error: initializer element is not constant > .product_name = yas5xx_product_name[yas532], > ^~~~~~~~~~~~~~~~~~~ > drivers/iio/magnetometer/yamaha-yas530.c:938:19: note: (near initialization for ‘yas5xx_chip_info_tbl[1].product_name’) > drivers/iio/magnetometer/yamaha-yas530.c:943:19: error: initializer element is not constant > .product_name = yas5xx_product_name[yas533], > ^~~~~~~~~~~~~~~~~~~ > drivers/iio/magnetometer/yamaha-yas530.c:943:19: note: (near initialization for ‘yas5xx_chip_info_tbl[2].product_name’) > > > 930 static const struct yas5xx_chip_info yas5xx_chip_info_tbl[] = { > > > 931 [yas530] = { > > > 932 .devid = YAS530_DEVICE_ID, > > > > 933 .product_name = yas5xx_product_name[yas530], > > > 934 .version_name = yas5xx_version_names[yas530], Would then .product_name = "YAS530 MS-3E", .version_names = { "A", "B" }, work? Jakob, note 's' in the field name as well. > > > 935 }, > > > 936 [yas532] = { > > > 937 .devid = YAS532_DEVICE_ID, > > > 938 .product_name = yas5xx_product_name[yas532], > > > 939 .version_name = yas5xx_version_names[yas532], > > > 940 }, > > > 941 [yas533] = { > > > 942 .devid = YAS532_DEVICE_ID, > > > 943 .product_name = yas5xx_product_name[yas533], > > > 944 .version_name = yas5xx_version_names[yas533], > > > 945 }, > > > 946 };
On Mon, Aug 08, 2022 at 08:04:20PM +0200, Andy Shevchenko wrote: > On Mon, Aug 8, 2022 at 5:59 PM Nathan Chancellor <nathan@kernel.org> wrote: > > On Mon, Aug 08, 2022 at 01:18:06PM, +0200, Andy Shevchenko wrote: > > > On Mon, Aug 8, 2022 at 7:40 AM kernel test robot <lkp@intel.com> wrote: > > > > > > ... > > > > > > > All errors (new ones prefixed by >>): > > > > > > > > >> drivers/iio/magnetometer/yamaha-yas530.c:933:19: error: initializer element is not a compile-time constant > > > > .product_name = yas5xx_product_name[yas530], > > > > ^~~~~~~~~~~~~~~~~~~~~~~~~~~ > > > > 1 error generated. > > > > > > What?! > > > > > > The yas530 is a part of the enum, how come that compiler can't see > > > this? Looks like a Clang bug. > > > > That is not what clang is complaining about here, you'll see the same > > error even if you used '0', '1', or '2' here: > > > > drivers/iio/magnetometer/yamaha-yas530.c:933:19: error: initializer element is not a compile-time constant > > .product_name = yas5xx_product_name[0], > > ^~~~~~~~~~~~~~~~~~~~~~ > > 1 error generated. > > > > It is complaining that the initializer element > > ('yas5xx_product_name[yas530]', rather than just 'yas530') is not > > constant, which is a true complaint if I am reading C11 standard 6.6.7 > > correctly. > > > > GCC 8+ has chosen to accept const structures as constant expressions in > > designated initializers, which it is allowed to do per 6.6.10. Nick did > > have a patch to try and match this behavior in clang but the work that > > was requested doesn't seem to be trivial so it was never finalized: > > https://reviews.llvm.org/D76096 > > > > You'll see the same error with GCC 7: > > > > drivers/iio/magnetometer/yamaha-yas530.c:933:19: error: initializer element is not constant > > .product_name = yas5xx_product_name[yas530], > > ^~~~~~~~~~~~~~~~~~~ > > drivers/iio/magnetometer/yamaha-yas530.c:933:19: note: (near initialization for ‘yas5xx_chip_info_tbl[0].product_name’) > > drivers/iio/magnetometer/yamaha-yas530.c:938:19: error: initializer element is not constant > > .product_name = yas5xx_product_name[yas532], > > ^~~~~~~~~~~~~~~~~~~ > > drivers/iio/magnetometer/yamaha-yas530.c:938:19: note: (near initialization for ‘yas5xx_chip_info_tbl[1].product_name’) > > drivers/iio/magnetometer/yamaha-yas530.c:943:19: error: initializer element is not constant > > .product_name = yas5xx_product_name[yas533], > > ^~~~~~~~~~~~~~~~~~~ > > drivers/iio/magnetometer/yamaha-yas530.c:943:19: note: (near initialization for ‘yas5xx_chip_info_tbl[2].product_name’) > > > > > 930 static const struct yas5xx_chip_info yas5xx_chip_info_tbl[] = { > > > > 931 [yas530] = { > > > > 932 .devid = YAS530_DEVICE_ID, > > > > > 933 .product_name = yas5xx_product_name[yas530], > > > > 934 .version_name = yas5xx_version_names[yas530], > > Would then > > .product_name = "YAS530 MS-3E", > .version_names = { "A", "B" }, > > work? I haven't tested it but there is no reason that shouldn't work. > Jakob, note 's' in the field name as well. > > > > > 935 }, > > > > 936 [yas532] = { > > > > 937 .devid = YAS532_DEVICE_ID, > > > > 938 .product_name = yas5xx_product_name[yas532], > > > > 939 .version_name = yas5xx_version_names[yas532], > > > > 940 }, > > > > 941 [yas533] = { > > > > 942 .devid = YAS532_DEVICE_ID, > > > > 943 .product_name = yas5xx_product_name[yas533], > > > > 944 .version_name = yas5xx_version_names[yas533], > > > > 945 }, > > > > 946 }; > > -- > With Best Regards, > Andy Shevchenko
Hi Andy, On 08.08.22 20:04, Andy Shevchenko wrote: > On Mon, Aug 8, 2022 at 5:59 PM Nathan Chancellor <nathan@kernel.org> wrote: >> On Mon, Aug 08, 2022 at 01:18:06PM, +0200, Andy Shevchenko wrote: >>> On Mon, Aug 8, 2022 at 7:40 AM kernel test robot <lkp@intel.com> wrote: >>> >>> ... >>> >>>> All errors (new ones prefixed by >>): >>>> >>>>>> drivers/iio/magnetometer/yamaha-yas530.c:933:19: error: initializer element is not a compile-time constant >>>> .product_name = yas5xx_product_name[yas530], >>>> ^~~~~~~~~~~~~~~~~~~~~~~~~~~ >>>> 1 error generated. >>> >>> What?! >>> >>> The yas530 is a part of the enum, how come that compiler can't see >>> this? Looks like a Clang bug. >> >> That is not what clang is complaining about here, you'll see the same >> error even if you used '0', '1', or '2' here: >> >> drivers/iio/magnetometer/yamaha-yas530.c:933:19: error: initializer element is not a compile-time constant >> .product_name = yas5xx_product_name[0], >> ^~~~~~~~~~~~~~~~~~~~~~ >> 1 error generated. >> >> It is complaining that the initializer element >> ('yas5xx_product_name[yas530]', rather than just 'yas530') is not >> constant, which is a true complaint if I am reading C11 standard 6.6.7 >> correctly. >> >> GCC 8+ has chosen to accept const structures as constant expressions in >> designated initializers, which it is allowed to do per 6.6.10. Nick did >> have a patch to try and match this behavior in clang but the work that >> was requested doesn't seem to be trivial so it was never finalized: >> https://reviews.llvm.org/D76096 >> >> You'll see the same error with GCC 7: >> >> drivers/iio/magnetometer/yamaha-yas530.c:933:19: error: initializer element is not constant >> .product_name = yas5xx_product_name[yas530], >> ^~~~~~~~~~~~~~~~~~~ >> drivers/iio/magnetometer/yamaha-yas530.c:933:19: note: (near initialization for ‘yas5xx_chip_info_tbl[0].product_name’) >> drivers/iio/magnetometer/yamaha-yas530.c:938:19: error: initializer element is not constant >> .product_name = yas5xx_product_name[yas532], >> ^~~~~~~~~~~~~~~~~~~ >> drivers/iio/magnetometer/yamaha-yas530.c:938:19: note: (near initialization for ‘yas5xx_chip_info_tbl[1].product_name’) >> drivers/iio/magnetometer/yamaha-yas530.c:943:19: error: initializer element is not constant >> .product_name = yas5xx_product_name[yas533], >> ^~~~~~~~~~~~~~~~~~~ >> drivers/iio/magnetometer/yamaha-yas530.c:943:19: note: (near initialization for ‘yas5xx_chip_info_tbl[2].product_name’) > >>>> 930 static const struct yas5xx_chip_info yas5xx_chip_info_tbl[] = { >>>> 931 [yas530] = { >>>> 932 .devid = YAS530_DEVICE_ID, >>>> > 933 .product_name = yas5xx_product_name[yas530], >>>> 934 .version_name = yas5xx_version_names[yas530], > > Would then > > .product_name = "YAS530 MS-3E", > .version_names = { "A", "B" }, > > work? > > Jakob, note 's' in the field name as well. Thanks for clarifying. I'll change it that way. >>>> 935 }, >>>> 936 [yas532] = { >>>> 937 .devid = YAS532_DEVICE_ID, >>>> 938 .product_name = yas5xx_product_name[yas532], >>>> 939 .version_name = yas5xx_version_names[yas532], >>>> 940 }, >>>> 941 [yas533] = { >>>> 942 .devid = YAS532_DEVICE_ID, >>>> 943 .product_name = yas5xx_product_name[yas533], >>>> 944 .version_name = yas5xx_version_names[yas533], >>>> 945 }, >>>> 946 }; > Kind regards, Jakob
Hi Andy, On 08.08.22 13:22, Andy Shevchenko wrote: > On Mon, Aug 8, 2022 at 1:07 AM Jakob Hauser <jahau@rocketmail.com> wrote: >> >> This commit introduces the "chip_info" structure approach for better variant >> handling. > > Read "Submitting Patches" in the chapter which mentions "This patch" > pattern and fix the above accordingly. I'll change the sentence to imperative mood. >> The variant to be used is now chosen by the Device Tree (enum "chip_ids"), >> not by the chip ID in the register. However, there is a check to make sure >> they match (using integer "id_check"). ... >> + if (id_check != yas5xx->chip_info->devid) { > >> + switch (yas5xx->chip_info->devid) { > > You can make these kind of lines shorter by introducing a temporary variable: > > struct ... *ci = yaas5xx->chip_info; > Everywhere? OK. There will be many changes, this also affects the following patches. I hope "ci" for chip_info and "c" for calibration is not too confusing. Though I guess it's ok. Kind regards, Jakob
Hi Andy, On 08.08.22 13:32, Andy Shevchenko wrote: > On Mon, Aug 8, 2022 at 1:07 AM Jakob Hauser <jahau@rocketmail.com> wrote: > > .. > >> + yas5xx->chip = id->driver_data; >> + yas5xx->chip_info = &yas5xx_chip_info_tbl[yas5xx->chip]; > > I don't see how ->chip is being used, I would expect it is the part of > chip_info, if it's really needed. That said, please make it directly a > pointer, so the above becomes: > > ... ->chip_info = (const struct ...)id->driver_data; > > .. > >> + {"yas530", yas530 }, >> + {"yas532", yas532 }, >> + {"yas533", yas533 }, > > Read above and here: > > yas53... ==> (kernel_ulong_t)&...[yas53...] > Generally on this part, I'm quite confused about the ... enum chip_ids { yas530, yas532, yas533, }; ... at the beginning of the driver. In my naive beginners approach I think that I have to initialize this enum. I did this by: struct yas5xx { ... enum chip_ids chip; ... }; ... yas5xx->chip = id->driver_data; The i2c_device_id at the end of the driver initially only contained the fist part, looking like this: static const struct i2c_device_id yas5xx_id[] = { {"yas530", }, {"yas532", }, {"yas533", }, {} }; This first part is "char name[I2C_NAME_SIZE]" according to [1]. I didn't manage to initialize the enum with "id->name"... yas5xx->chip = id->name; ... this resulted in a compiler error stating "incompatible types when assigning to type 'enum chip_ids' from type 'const char *'". This made me introduce the second part of i2c_device_id... static const struct i2c_device_id yas5xx_id[] = { {"yas530", yas530 }, {"yas532", yas532 }, {"yas533", yas533 }, {} }; ... which is "kernel_ulong_t driver_data;". Initializing the enum by "id->driver_data" did work: yas5xx->chip = id->driver_data; [1] https://github.com/torvalds/linux/blob/v5.19/include/linux/mod_devicetable.h#L465 -------------------- I think in other drivers I've seen enums not being initialized. I don't understand how this works. Unfortunately I can't recall specific examples. -------------------- I now had a try with following changes... struct yas5xx { struct device *dev; - enum chip_ids chip; const struct yas5xx_chip_info *chip_info; ... }; ... - yas5xx->chip = id->driver_data; - yas5xx->chip_info = &yas5xx_chip_info_tbl[yas5xx->chip]; + yas5xx->chip_info = &yas5xx_chip_info_tbl[id->driver_data]; ... or summarized as: enum chip_ids { yas530, yas532, yas533, }; ... struct yas5xx { ... }; ... yas5xx->chip_info = &yas5xx_chip_info_tbl[id->driver_data]; ... static const struct i2c_device_id yas5xx_id[] = { {"yas530", yas530 }, {"yas532", yas532 }, {"yas533", yas533 }, {} }; This seems to work. Therefore I would it implement it that way. I hope it works reliably, as I don't see a connection between the enum and the chip_info. -------------------- What I still can't manage is getting rid of the "id->driver_data" part. When trying the above with "id->name"... yas5xx->chip_info = &yas5xx_chip_info_tbl[id->name]; ... the compiler complains about "array subscript is not an integer". When trying to add quotation marks to the enum chip_ids content, the compiler complains about this by "expected identifier before string constant". Kind regards, Jakob
diff --git a/drivers/iio/magnetometer/yamaha-yas530.c b/drivers/iio/magnetometer/yamaha-yas530.c index bf0aa64ac1a2..ecc2b61a5c4f 100644 --- a/drivers/iio/magnetometer/yamaha-yas530.c +++ b/drivers/iio/magnetometer/yamaha-yas530.c @@ -96,6 +96,24 @@ /* Turn off device regulators etc after 5 seconds of inactivity */ #define YAS5XX_AUTOSUSPEND_DELAY_MS 5000 +enum chip_ids { + yas530, + yas532, + yas533, +}; + +static const char * const yas5xx_product_name[] = { + "YAS530 MS-3E", + "YAS532 MS-3R", + "YAS533 MS-3F", +}; + +static const char * const yas5xx_version_names[][2] = { + [yas530] = { "A", "B" }, + [yas532] = { "AB", "AC" }, + [yas533] = { "AB", "AC" }, +}; + struct yas5xx_calibration { /* Linearization calibration x, y1, y2 */ s32 r[3]; @@ -110,12 +128,26 @@ struct yas5xx_calibration { u8 dck; }; +struct yas5xx; + +/** + * struct yas5xx_chip_info - device-specific data and function pointers + * @devid: device ID number + * @product_name: product name of the YAS variant + * @version_name: version letter or naming + */ +struct yas5xx_chip_info { + unsigned int devid; + const char *product_name; + const char * const *version_name; +}; + /** * struct yas5xx - state container for the YAS5xx driver * @dev: parent device pointer - * @devid: device ID number + * @chip: enumeration of the device variant + * @chip_info: device-specific data * @version: device version - * @name: device name * @calibration: calibration settings from the OTP storage * @hard_offsets: offsets for each axis measured with initcoil actuated * @orientation: mounting matrix, flipped axis etc @@ -129,9 +161,9 @@ struct yas5xx_calibration { */ struct yas5xx { struct device *dev; - unsigned int devid; + enum chip_ids chip; + const struct yas5xx_chip_info *chip_info; unsigned int version; - char name[16]; struct yas5xx_calibration calibration; s8 hard_offsets[3]; struct iio_mount_matrix orientation; @@ -222,7 +254,7 @@ static int yas530_measure(struct yas5xx *yas5xx, u16 *t, u16 *x, u16 *y1, u16 *y mutex_unlock(&yas5xx->lock); - switch (yas5xx->devid) { + switch (yas5xx->chip_info->devid) { case YAS530_DEVICE_ID: /* * The t value is 9 bits in big endian format @@ -276,7 +308,7 @@ static s32 yas530_linearize(struct yas5xx *yas5xx, u16 val, int axis) s32 coef; /* Select coefficients */ - switch (yas5xx->devid) { + switch (yas5xx->chip_info->devid) { case YAS530_DEVICE_ID: if (yas5xx->version == YAS530_VERSION_A) coef = YAS530_VERSION_A_COEF; @@ -336,7 +368,7 @@ static int yas530_get_measure(struct yas5xx *yas5xx, s32 *to, s32 *xo, s32 *yo, sy2 = yas530_linearize(yas5xx, y2, 2); /* Set the temperature reference value (unit: counts) */ - switch (yas5xx->devid) { + switch (yas5xx->chip_info->devid) { case YAS530_DEVICE_ID: t_ref = YAS530_20DEGREES; break; @@ -349,7 +381,7 @@ static int yas530_get_measure(struct yas5xx *yas5xx, s32 *to, s32 *xo, s32 *yo, } /* Temperature compensation for x, y1, y2 respectively */ - if (yas5xx->devid == YAS532_DEVICE_ID && + if (yas5xx->chip_info->devid == YAS532_DEVICE_ID && yas5xx->version == YAS532_VERSION_AC) { /* * YAS532 version AC uses the temperature deviation as a @@ -384,7 +416,7 @@ static int yas530_get_measure(struct yas5xx *yas5xx, s32 *to, s32 *xo, s32 *yo, sz = -sy1 - sy2; /* Process temperature readout */ - switch (yas5xx->devid) { + switch (yas5xx->chip_info->devid) { case YAS530_DEVICE_ID: /* * Raw temperature value t is the number of counts starting @@ -473,7 +505,7 @@ static int yas5xx_read_raw(struct iio_dev *indio_dev, } return IIO_VAL_INT; case IIO_CHAN_INFO_SCALE: - switch (yas5xx->devid) { + switch (yas5xx->chip_info->devid) { case YAS530_DEVICE_ID: /* * Raw values of YAS530 are in picotesla. Divide by @@ -814,7 +846,7 @@ static int yas530_measure_offsets(struct yas5xx *yas5xx) return ret; /* When the initcoil is active this should be around the center */ - switch (yas5xx->devid) { + switch (yas5xx->chip_info->devid) { case YAS530_DEVICE_ID: center = YAS530_DATA_CENTER; break; @@ -895,12 +927,31 @@ static int yas530_power_on(struct yas5xx *yas5xx) return regmap_write(yas5xx->map, YAS530_MEASURE_INTERVAL, 0); } +static const struct yas5xx_chip_info yas5xx_chip_info_tbl[] = { + [yas530] = { + .devid = YAS530_DEVICE_ID, + .product_name = yas5xx_product_name[yas530], + .version_name = yas5xx_version_names[yas530], + }, + [yas532] = { + .devid = YAS532_DEVICE_ID, + .product_name = yas5xx_product_name[yas532], + .version_name = yas5xx_version_names[yas532], + }, + [yas533] = { + .devid = YAS532_DEVICE_ID, + .product_name = yas5xx_product_name[yas533], + .version_name = yas5xx_version_names[yas533], + }, +}; + static int yas5xx_probe(struct i2c_client *i2c, const struct i2c_device_id *id) { struct iio_dev *indio_dev; struct device *dev = &i2c->dev; struct yas5xx *yas5xx; + int id_check; int ret; indio_dev = devm_iio_device_alloc(dev, sizeof(*yas5xx)); @@ -947,33 +998,41 @@ static int yas5xx_probe(struct i2c_client *i2c, goto assert_reset; } - ret = regmap_read(yas5xx->map, YAS5XX_DEVICE_ID, &yas5xx->devid); + yas5xx->chip = id->driver_data; + yas5xx->chip_info = &yas5xx_chip_info_tbl[yas5xx->chip]; + + ret = regmap_read(yas5xx->map, YAS5XX_DEVICE_ID, &id_check); if (ret) goto assert_reset; - switch (yas5xx->devid) { + if (id_check != yas5xx->chip_info->devid) { + ret = dev_err_probe(dev, -ENODEV, + "device ID %02x doesn't match %s\n", + id_check, id->name); + goto assert_reset; + } + + switch (yas5xx->chip_info->devid) { case YAS530_DEVICE_ID: ret = yas530_get_calibration_data(yas5xx); if (ret) goto assert_reset; - dev_info(dev, "detected YAS530 MS-3E %s", - yas5xx->version ? "B" : "A"); - strncpy(yas5xx->name, "yas530", sizeof(yas5xx->name)); break; case YAS532_DEVICE_ID: ret = yas532_get_calibration_data(yas5xx); if (ret) goto assert_reset; - dev_info(dev, "detected YAS532/YAS533 MS-3R/F %s", - yas5xx->version ? "AC" : "AB"); - strncpy(yas5xx->name, "yas532", sizeof(yas5xx->name)); break; default: ret = -ENODEV; - dev_err(dev, "unhandled device ID %02x\n", yas5xx->devid); + dev_err(dev, "unhandled device ID %02x\n", + yas5xx->chip_info->devid); goto assert_reset; } + dev_info(dev, "detected %s %s\n", yas5xx->chip_info->product_name, + yas5xx->chip_info->version_name[yas5xx->version]); + yas530_dump_calibration(yas5xx); ret = yas530_power_on(yas5xx); if (ret) @@ -985,7 +1044,7 @@ static int yas5xx_probe(struct i2c_client *i2c, indio_dev->info = &yas5xx_info; indio_dev->available_scan_masks = yas5xx_scan_masks; indio_dev->modes = INDIO_DIRECT_MODE; - indio_dev->name = yas5xx->name; + indio_dev->name = id->name; indio_dev->channels = yas5xx_channels; indio_dev->num_channels = ARRAY_SIZE(yas5xx_channels); @@ -1100,9 +1159,9 @@ static const struct dev_pm_ops yas5xx_dev_pm_ops = { }; static const struct i2c_device_id yas5xx_id[] = { - {"yas530", }, - {"yas532", }, - {"yas533", }, + {"yas530", yas530 }, + {"yas532", yas532 }, + {"yas533", yas533 }, {} }; MODULE_DEVICE_TABLE(i2c, yas5xx_id);
This commit introduces the "chip_info" structure approach for better variant handling. The variant to be used is now chosen by the Device Tree (enum "chip_ids"), not by the chip ID in the register. However, there is a check to make sure they match (using integer "id_check"). Signed-off-by: Jakob Hauser <jahau@rocketmail.com> --- drivers/iio/magnetometer/yamaha-yas530.c | 107 ++++++++++++++++++----- 1 file changed, 83 insertions(+), 24 deletions(-)