Re: [PATCH 2/3] iio: accel: kionix-kx022a: Add chip_info structure

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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! ~~




[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux