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

Mehdi Djait
2023-03-21T15:40:16Z

Hello everyone,

On Mon, Mar 20, 2023 at 12:24:47PM +0000, Jonathan Cameron wrote:
> On Mon, 20 Mar 2023 09:17:54 +0200
> Matti Vaittinen <mazziesaccount@gmail.com> wrote:
>
> > 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@gmail.com> 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.
>
> Ah. Maybe that issue only occurred for SPI - I'd assumed it was more general.
>
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/?id=5fa6863ba692
>
> >
> > > 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.
>
> We already have other kionix drivers that don't use these defines.
> This is less of an issue if they are very local - so pushed down to the C file,
> but I still don't like the implication that they extend to a broad range of
> devices.
>
> > 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.
>
> I think this already runs into this problem or at least sets the driver
> up to hit it very soon. The reality is that these definitions are shared
> by the 2 parts supported so far. 3rd part comes along and I'd be willing
> to place a bet that at least one of these definitions doesn't apply.
> So we end up with a mess converting it back to a specific name.
>
> I've gone down this path many times before and it very rarely works out.
>
> > 3) I definitely like the idea of using exact model number prefix for
> > 'stuff' which is intended to work only on one exact .
>
> When you have 2 devices it is easy to separate the 'generic' from the
> 'specific'. That breaks when you have 3. If we are sure there won't be
> a 3rd device supported by this driver then fair enough...
>
> >
> > 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.
>
> Given they tend to be accessed via a function pointer, even functions
> are only set up the once. For these I'm fine with a nasty listing
> type approach with multiple part names in the function defintion.
> That doesn't scale great either as lots of parts get added but it at least
> calls out which function covers which parts.
>
> >
> > 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... :/
>
> If we have truely universal defines - sometimes this happens for WHO AM I
> registers for example as they are the same over all devices from a manufacturer
> (more or less anyway) then the broad forms are fine. Otherwise it just tends
> to end up as a mess if lots of parts added.

I will remove the generic defines.

I also really liked the idea of seperating the IC specific stuff from the
generic ones. Better play it safe here, I can also see it becoming a mess in
the long run.

> >
> > >>
> > >> +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 ;)
>
> This one wasn't a particularly important bit of feedback. I'm fine with the
> table, though seems slightly less readable to my eyes.

My reasoning behind it: when supporting multiple devices, having a single
array of chip_info and one single EXPORT_SYMBOL is more concise and
readable.

>
> >
> > >> 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).
>
> Meh. This stuff happens and at the end of the day it's a magic string
> that userspace can match against. No userspace knows all of them anyway
> so most likely it's just provided in a 'selection' box for a user or encoded
> in a custom script / config file. So not hugely important for it to have
> the simplest possible form.
>
> >
> > > It's ABI so we are stuck with it, but avoid repeating that issue
> > > for new devices. >

I will change the name in the chip_info of kx022a back to "kx022-accel"

I am already using "kx132" as name for the newly supported device

> >
> > >>
> > >> +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 ;)

Yes, my goal is to support more devices and I want to make as easy as
possible to extend this driver :)

>
> Not adding a lot, but you are going to end up with adding one line
> to an enum in the header for each new device, vs one
> extern line. So I'm not sure it saves anything either.
>

Using a separate instance of the chip_info structure for each chip means
also an extra symbol export

> >
> > >> -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.

no strong reason, I will move it back to the .c file

--
Kind Regards
Mehdi Djait