Hi Mehdi and Jonathan,
Just my take on couple of comments from Jonathan :) I still have my own
review to do though...
On 3/19/23 18:20, Jonathan Cameron wrote:
On Fri, 17 Mar 2023 00:48:36 +0100
Mehdi Djait <mehdi.djait.k@xxxxxxxxx> wrote:
Refactor the kx022a driver implementation to make it more
generic and extensible.
Add the chip_info structure will to the driver's private
data to hold all the device specific infos.
Move the enum, struct and constants definitions to the header
file.
You also introduce an i2c_device_id table
Without that I think module autoloading will be broken anyway so that's
definitely a good thing to add.
I am pretty sure the autoloading worked for OF-systems. But yes, adding
the i2c_device_id is probably a good idea. Thanks.
A few comments inline. Mostly around reducing the name changes.
Wild cards (or simply shorted 'generic' prefixes like KX_)
almost always bite back in the long run. Hence we generally just try
to name things after the first device that they were relevant to.
I must say I disagree on this with you Jonathan. I know wildcards tend
to get confusing - but I still like the idea of showing which of the
definitions are IC specific and which ones are generic or at least used
by more than one variant - especially as long as we only have two
supported ICs. I definitely like the macro naming added by Mehdi. This
approach has been very helpful for me for example in the BD718x7
(BD71837/BD71847/BD71850) PMIC driver. My take on this is:
1) I like the generic KX_define.
2) I would not try adding wildcards like KX_X22 - to denote support for
122 and 022 - while not supporting 132 - in my experience - that won't
scale.
3) I definitely like the idea of using exact model number prefix for
'stuff' which is intended to work only on one exact model.
Regarding the 3) - I am not so strict on how the register/mask defines
are handled - I _like_ the 1) 2) 3) approach above - but mask/register
defines tend to get set (correctly) once and not required to be looked
up after this. But. When the 'stuff' is functions - this gets very
useful as one is very often required to see which functions are executed
on which IC variant. Same goes to structs.
So, if we manage to convince Jonathan about the naming, then I like what
yoo had here! I would hovever do it in two steps. I would at first do
renaming patch where the generic defines were renamed - without any
functional changes - and only then add the kx132 stuff in a subsequent
patch. That would simplify seeing which changes are just renaming and
which are functional ones.
But here, I must go with the wind - if subsystem maintainer says the
code should not have naming like this - then I have no say over it... :/
+static const struct i2c_device_id kx022a_i2c_id[] = {
+ { .name = "kx022a", .driver_data = (kernel_ulong_t)&kx_chip_info[KX022A] },
If there are a small set and we aren't ever going to index the chip_info structure
we might be better off not bothering with the enum and instead using a separate
instance of the structure for each chip.
I kind of like also the table added by Mehdi. I admit I was at first
just thinking that we should have a pointer to the struct here without
any tables - but... After I took a peek in the kionix-kx022a.c - I kind
of liked the table and not exporting the struct names. So, I don't have
a strong opinion on this.
I think it's worth noting that this driver could (maybe easily enough)
be extended to support also a few other kionix accelerometers. Maybe, if
we don't scare Mehdi away, we will see a few other variants supported as
well ;)
data->regmap = regmap;
data->dev = dev;
data->irq = irq;
- data->odr_ns = KX022A_DEFAULT_PERIOD_NS;
+ data->odr_ns = KX_DEFAULT_PERIOD_NS;
mutex_init(&data->mutex);
- idev->channels = kx022a_channels;
- idev->num_channels = ARRAY_SIZE(kx022a_channels);
- idev->name = "kx022-accel";
Ah. Missed this naming in original driver review. We only normally
postfix with accel in devices that have multiple sensors with separate
drivers. Don't think that is true of the kx022a.
Ouch. I am not 100% sure but may be you didn't miss it. It may be I just
missed fixing this because your comment here sounds somewhat familiar to
me! (Or then you commented on suffix in driver-name).
It's ABI so we are stuck with it, but avoid repeating that issue
for new devices. >
+enum kx022a_device_type {
+ KX022A,
+};
As below. I'd avoid using the enum unless needed.
That can make sense where a driver supports lots of devices but I don't think
it does here.
Well, I know it is usually not too clever to be prepared for the future
stuff too well. But - I don't think the enum and table are adding much
of complexity? I am saying this as I think this driver could be extended
to support also kx022 (without the A), kx023, kx122. I've also seen some
references to model kx022A-120B (but I have no idea what's the story
there or if that IC is publicly available). Maybe Mehdi would like to
extend this driver further after the KX132 is done ;)
-int kx022a_probe_internal(struct device *dev);
-extern const struct regmap_config kx022a_regmap;
+struct kx022a_chip_info {
+ const char *name;
+ enum kx022a_device_type type;
+ const struct regmap_config *regmap_config;
+ const struct iio_chan_spec *channels;
+ unsigned int num_channels;
+ unsigned int fifo_length;
+ u8 who;
Some of these are no immediately obvious so either rename the
field so it is more obvious what it is, or add comments.
I would vote for adding a comment :) I like the who. Both the band and
this member here :) Data-sheet has register named as "who_am_i" - so I
don't think this name is too obfuscating - and what matters to me - it
is short yet meaningful.
+ u8 id;
+ u8 cntl;
+ u8 cntl2;
+ u8 odcntl;
+ u8 buf_cntl1;
+ u8 buf_cntl2;
+ u8 buf_clear;
+ u8 buf_status1;
+ u16 buf_smp_lvl_mask;
+ u8 buf_read;
+ u8 inc1;
+ u8 inc4;
+ u8 inc5;
+ u8 inc6;
+ u8 xout_l;
+};
+
+struct kx022a_data {
Why move this to the header? Unless there is a strong reason
I'd prefer this to stay down in the .c file.
So would I. It's definitely nice to be able to see the struct in the
same file where the code referencing it is.
Yours,
-- Matti
--
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland
~~ When things go utterly wrong vim users can always type :help! ~~
