| Message ID | 20180920141321.15251-1-songqiang1304521@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | iio: magnetometer: hmc5843: Clean up redundant code. | expand |
Hi Song,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on iio/togreg]
[also build test ERROR on v4.19-rc4 next-20180920]
[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/Song-Qiang/iio-magnetometer-hmc5843-Clean-up-redundant-code/20180921-091239
base: https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git togreg
config: x86_64-randconfig-u0-09211331 (attached as .config)
compiler: gcc-5 (Debian 5.5.0-3) 5.4.1 20171010
reproduce:
# save the attached .config to linux build tree
make ARCH=x86_64
All errors (new ones prefixed by >>):
>> ERROR: "hmc5843_volatile_table" [drivers/iio/magnetometer/hmc5843_i2c.ko] undefined!
>> ERROR: "hmc5843_readable_table" [drivers/iio/magnetometer/hmc5843_i2c.ko] undefined!
>> ERROR: "hmc5843_writable_table" [drivers/iio/magnetometer/hmc5843_i2c.ko] undefined!
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
Hi Song, On Fri, Sep 21, 2018 at 04:10:16PM +0800, kbuild test robot wrote: > Hi Song, > > Thank you for the patch! Yet something to improve: > > [auto build test ERROR on iio/togreg] > [also build test ERROR on v4.19-rc4 next-20180920] > [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/Song-Qiang/iio-magnetometer-hmc5843-Clean-up-redundant-code/20180921-091239 > base: https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git togreg > config: x86_64-randconfig-u0-09211331 (attached as .config) > compiler: gcc-5 (Debian 5.5.0-3) 5.4.1 20171010 > reproduce: > # save the attached .config to linux build tree > make ARCH=x86_64 > > All errors (new ones prefixed by >>): > > >> ERROR: "hmc5843_volatile_table" [drivers/iio/magnetometer/hmc5843_i2c.ko] undefined! > >> ERROR: "hmc5843_readable_table" [drivers/iio/magnetometer/hmc5843_i2c.ko] undefined! > >> ERROR: "hmc5843_writable_table" [drivers/iio/magnetometer/hmc5843_i2c.ko] undefined! You would need to export these above symbols using EXPORT_SYMBOL() to be used by i2c/spi modules. But on the other hand, exporting too many symbols is a bad idea since it is only used for this driver and not at any other place in IIO. So, in my opinion drop this patch and leave the code as-is. https://lkml.org/lkml/2018/7/16/566 --> worth reading Thanks
On Fri, Sep 21, 2018 at 11:56:16PM +0530, Himanshu Jha wrote: > Hi Song, > > On Fri, Sep 21, 2018 at 04:10:16PM +0800, kbuild test robot wrote: > > Hi Song, > > > > Thank you for the patch! Yet something to improve: > > > > [auto build test ERROR on iio/togreg] > > [also build test ERROR on v4.19-rc4 next-20180920] > > [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/Song-Qiang/iio-magnetometer-hmc5843-Clean-up-redundant-code/20180921-091239 > > base: https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git togreg > > config: x86_64-randconfig-u0-09211331 (attached as .config) > > compiler: gcc-5 (Debian 5.5.0-3) 5.4.1 20171010 > > reproduce: > > # save the attached .config to linux build tree > > make ARCH=x86_64 > > > > All errors (new ones prefixed by >>): > > > > >> ERROR: "hmc5843_volatile_table" [drivers/iio/magnetometer/hmc5843_i2c.ko] undefined! > > >> ERROR: "hmc5843_readable_table" [drivers/iio/magnetometer/hmc5843_i2c.ko] undefined! > > >> ERROR: "hmc5843_writable_table" [drivers/iio/magnetometer/hmc5843_i2c.ko] undefined! > > You would need to export these above symbols using EXPORT_SYMBOL() > to be used by i2c/spi modules. > > But on the other hand, exporting too many symbols is a bad idea since > it is only used for this driver and not at any other place in IIO. > So, in my opinion drop this patch and leave the code as-is. > > https://lkml.org/lkml/2018/7/16/566 --> worth reading > > > Thanks > -- > Himanshu Jha > Undergraduate Student > Department of Electronics & Communication > Guru Tegh Bahadur Institute of Technology Hi Himanshu, You're right, that's exactly what I was missing! I saw the link you mentioned above and I also think that's a very good idea to limit the scope of symbols. But I don't know when this work can be applied to the kernel, as it seems like a not little change for the build infrastructure. I think this maybe a common problem for some drivers. Divers for bmc150 in drivers/iio/accel/bmc-150-accel-core.c did the same exporting stuff as I was prefered. So I think even if either exporting or duplicating is not good enough, one must be choosed for now. I think this is a topic that I have some ideas but not experienced enough to say what should we do is better. I would like to hear Jonathan's ideas about this. If this patched shouldn't be applied, then maybe bmc150 should be patched. yours, Song Qiang
On Sat, 22 Sep 2018 18:04:19 +0800 Song Qiang <songqiang1304521@gmail.com> wrote: > On Fri, Sep 21, 2018 at 11:56:16PM +0530, Himanshu Jha wrote: > > Hi Song, > > > > On Fri, Sep 21, 2018 at 04:10:16PM +0800, kbuild test robot wrote: > > > Hi Song, > > > > > > Thank you for the patch! Yet something to improve: > > > > > > [auto build test ERROR on iio/togreg] > > > [also build test ERROR on v4.19-rc4 next-20180920] > > > [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/Song-Qiang/iio-magnetometer-hmc5843-Clean-up-redundant-code/20180921-091239 > > > base: https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git togreg > > > config: x86_64-randconfig-u0-09211331 (attached as .config) > > > compiler: gcc-5 (Debian 5.5.0-3) 5.4.1 20171010 > > > reproduce: > > > # save the attached .config to linux build tree > > > make ARCH=x86_64 > > > > > > All errors (new ones prefixed by >>): > > > > > > >> ERROR: "hmc5843_volatile_table" [drivers/iio/magnetometer/hmc5843_i2c.ko] undefined! > > > >> ERROR: "hmc5843_readable_table" [drivers/iio/magnetometer/hmc5843_i2c.ko] undefined! > > > >> ERROR: "hmc5843_writable_table" [drivers/iio/magnetometer/hmc5843_i2c.ko] undefined! > > > > You would need to export these above symbols using EXPORT_SYMBOL() > > to be used by i2c/spi modules. > > > > But on the other hand, exporting too many symbols is a bad idea since > > it is only used for this driver and not at any other place in IIO. > > So, in my opinion drop this patch and leave the code as-is. > > > > https://lkml.org/lkml/2018/7/16/566 --> worth reading > > > > > > Thanks > > -- > > Himanshu Jha > > Undergraduate Student > > Department of Electronics & Communication > > Guru Tegh Bahadur Institute of Technology > > Hi Himanshu, > > You're right, that's exactly what I was missing! > I saw the link you mentioned above and I also think that's a very good > idea to limit the scope of symbols. But I don't know when this work can > be applied to the kernel, as it seems like a not little change for the > build infrastructure. > I think this maybe a common problem for some drivers. > Divers for bmc150 in drivers/iio/accel/bmc-150-accel-core.c did the same > exporting stuff as I was prefered. So I think even if either exporting or > duplicating is not good enough, one must be choosed for now. > > I think this is a topic that I have some ideas but not experienced > enough to say what should we do is better. I would like to hear Jonathan's > ideas about this. If this patched shouldn't be applied, then maybe bmc150 > should be patched. As you say it's not a particularly clear advantage either way and will depend on the size of the repetition. So I'm afraid it's in the category of insisting on one option or the other is just overly fussy. Al, for existing cases it's also not worth the noise that changing to one or other option creates. Whilst the noise effect on things like backporting fixes is minor for patches like this, it isn't zero so there has to be a clear advantage in whatever is changed. Hence I won't be taking this one, or any patches going the other way for existing drivers. Thanks, Jonathan > > yours, > Song Qiang >
diff --git a/drivers/iio/magnetometer/hmc5843.h b/drivers/iio/magnetometer/hmc5843.h index 76a5d7484d8d..0ab400f27c97 100644 --- a/drivers/iio/magnetometer/hmc5843.h +++ b/drivers/iio/magnetometer/hmc5843.h @@ -46,6 +46,10 @@ struct hmc5843_data { __be16 buffer[8]; }; +extern const struct regmap_access_table hmc5843_readable_table; +extern const struct regmap_access_table hmc5843_writable_table; +extern const struct regmap_access_table hmc5843_volatile_table; + int hmc5843_common_probe(struct device *dev, struct regmap *regmap, enum hmc5843_ids id, const char *name); int hmc5843_common_remove(struct device *dev); diff --git a/drivers/iio/magnetometer/hmc5843_core.c b/drivers/iio/magnetometer/hmc5843_core.c index ada142fb7aa3..a18e6d89f42a 100644 --- a/drivers/iio/magnetometer/hmc5843_core.c +++ b/drivers/iio/magnetometer/hmc5843_core.c @@ -92,6 +92,34 @@ static const char *const hmc5843_meas_conf_modes[] = {"normal", "positivebias", static const char *const hmc5983_meas_conf_modes[] = {"normal", "positivebias", "negativebias", "disabled"}; + +static const struct regmap_range hmc5843_readable_ranges[] = { + regmap_reg_range(0, HMC5843_ID_END), +}; + +const struct regmap_access_table hmc5843_readable_table = { + .yes_ranges = hmc5843_readable_ranges, + .n_yes_ranges = ARRAY_SIZE(hmc5843_readable_ranges), +}; + +static const struct regmap_range hmc5843_writable_ranges[] = { + regmap_reg_range(0, HMC5843_MODE_REG), +}; + +const struct regmap_access_table hmc5843_writable_table = { + .yes_ranges = hmc5843_writable_ranges, + .n_yes_ranges = ARRAY_SIZE(hmc5843_writable_ranges), +}; + +static const struct regmap_range hmc5843_volatile_ranges[] = { + regmap_reg_range(HMC5843_DATA_OUT_MSB_REGS, HMC5843_STATUS_REG), +}; + +const struct regmap_access_table hmc5843_volatile_table = { + .yes_ranges = hmc5843_volatile_ranges, + .n_yes_ranges = ARRAY_SIZE(hmc5843_volatile_ranges), +}; + /* Scaling factors: 10000000/Gain */ static const int hmc5843_regval_to_nanoscale[] = { 6173, 7692, 10309, 12821, 18868, 21739, 25641, 35714 diff --git a/drivers/iio/magnetometer/hmc5843_i2c.c b/drivers/iio/magnetometer/hmc5843_i2c.c index 3de7f4426ac4..a2c45a23e3c9 100644 --- a/drivers/iio/magnetometer/hmc5843_i2c.c +++ b/drivers/iio/magnetometer/hmc5843_i2c.c @@ -17,33 +17,6 @@ #include "hmc5843.h" -static const struct regmap_range hmc5843_readable_ranges[] = { - regmap_reg_range(0, HMC5843_ID_END), -}; - -static const struct regmap_access_table hmc5843_readable_table = { - .yes_ranges = hmc5843_readable_ranges, - .n_yes_ranges = ARRAY_SIZE(hmc5843_readable_ranges), -}; - -static const struct regmap_range hmc5843_writable_ranges[] = { - regmap_reg_range(0, HMC5843_MODE_REG), -}; - -static const struct regmap_access_table hmc5843_writable_table = { - .yes_ranges = hmc5843_writable_ranges, - .n_yes_ranges = ARRAY_SIZE(hmc5843_writable_ranges), -}; - -static const struct regmap_range hmc5843_volatile_ranges[] = { - regmap_reg_range(HMC5843_DATA_OUT_MSB_REGS, HMC5843_STATUS_REG), -}; - -static const struct regmap_access_table hmc5843_volatile_table = { - .yes_ranges = hmc5843_volatile_ranges, - .n_yes_ranges = ARRAY_SIZE(hmc5843_volatile_ranges), -}; - static const struct regmap_config hmc5843_i2c_regmap_config = { .reg_bits = 8, .val_bits = 8, diff --git a/drivers/iio/magnetometer/hmc5843_spi.c b/drivers/iio/magnetometer/hmc5843_spi.c index 535f03a70d63..19541f5bb38f 100644 --- a/drivers/iio/magnetometer/hmc5843_spi.c +++ b/drivers/iio/magnetometer/hmc5843_spi.c @@ -14,33 +14,6 @@ #include "hmc5843.h" -static const struct regmap_range hmc5843_readable_ranges[] = { - regmap_reg_range(0, HMC5843_ID_END), -}; - -static const struct regmap_access_table hmc5843_readable_table = { - .yes_ranges = hmc5843_readable_ranges, - .n_yes_ranges = ARRAY_SIZE(hmc5843_readable_ranges), -}; - -static const struct regmap_range hmc5843_writable_ranges[] = { - regmap_reg_range(0, HMC5843_MODE_REG), -}; - -static const struct regmap_access_table hmc5843_writable_table = { - .yes_ranges = hmc5843_writable_ranges, - .n_yes_ranges = ARRAY_SIZE(hmc5843_writable_ranges), -}; - -static const struct regmap_range hmc5843_volatile_ranges[] = { - regmap_reg_range(HMC5843_DATA_OUT_MSB_REGS, HMC5843_STATUS_REG), -}; - -static const struct regmap_access_table hmc5843_volatile_table = { - .yes_ranges = hmc5843_volatile_ranges, - .n_yes_ranges = ARRAY_SIZE(hmc5843_volatile_ranges), -}; - static const struct regmap_config hmc5843_spi_regmap_config = { .reg_bits = 8, .val_bits = 8,
Regmap tables are both defined in hmc5843_spi.c and hmc5843_i2c.c, while the only difference between these two set of tables is SPI need a read mask, which is set only in 'regmap_config' structrue. This patch moves the other structrues into hmc5843_core.c to reduce redundance. Signed-off-by: Song Qiang <songqiang1304521@gmail.com> --- drivers/iio/magnetometer/hmc5843.h | 4 ++++ drivers/iio/magnetometer/hmc5843_core.c | 28 +++++++++++++++++++++++++ drivers/iio/magnetometer/hmc5843_i2c.c | 27 ------------------------ drivers/iio/magnetometer/hmc5843_spi.c | 27 ------------------------ 4 files changed, 32 insertions(+), 54 deletions(-)