| Message ID | 20220803131132.19630-1-ddrokosov@sberdevices.ru |
|---|---|
| Headers | show |
| Series | iio: accel: add MSA311 accelerometer driver | expand |
On Thu, Aug 04, 2022 at 08:30:12PM +0200, Andy Shevchenko wrote: > > > > > + dev_err(dev, "cannot %s register %u from debugfs (%d)\n", > > > > > + readval ? "read" : "write", reg, err); > > > > > > > > You may consider taking [1] as a precursor here and use str_read_write(). > > > > > > > > [1]: https://lore.kernel.org/linux-i2c/20220703154232.55549-1-andriy.shevchenko@linux.intel.com/ > > > > > > Oh, really... Thank you for suggestion! > > > > I have taken it closer, and it's really helpful and nice, but looks like > > it's not merged to linux-next. > > Please advise how I can use it in the driver. Should I provide > > "Depends-On:" tag as I did for my HZ units patchset? > > No, just take that patch into your series. Do you mean include your patch to this reply-thread through the message-id linking? If my understanding is correct, maybe also it's better to include HZ units series to the current series? I mean patch series on below link: https://lore.kernel.org/linux-iio/20220801143811.14817-1-ddrokosov@sberdevices.ru/
On Thu, Aug 04, 2022 at 08:28:28PM +0200, Andy Shevchenko wrote: > > > > +static const struct { > > > > + int val; > > > > + int val2; > > > > +} msa311_fs_table[] = { > > > > + {0, 9580}, {0, 19160}, {0, 38320}, {0, 76641} > > > > +}; > > > > > > Besides that this struct is defined not only once in the file, this is > > > very well NIH struct s32_fract. Why not use the latter? > > > Good point, but looks like this struct is not so popular in the other > > kernel drivers: > > It's simply new, it is not about popularity. I would put it as it's > not _yet_ so popular. Okay, no problem. I've already reworked it to s32_fract locally. > > ... > > > grep "s32_fract" -r -l . | wc -l > > 3 > > Hint: `git grep` much much faster on Git repositories (it goes via > index and not working copy) and see > `git grep -lw s32_fract` > Thank you for the hint. Actually I'm using ripgrep for the kernel fuzzysearching, it's faster than git grep. Here I gave an example based on grep command, because it's general shell command for the searching substrings I suppose. > ... > > > > > + .cache_type = REGCACHE_RBTREE, > > > > > > Tree hash is good for sparse data, do you really have it sparse (a lot > > > of big gaps in between)? > > > > I suppose so. MSA311 regmap has ~6 gaps. > > Yes and how long is each gap in comparison to the overall space? The longest gap is 0xb bytes. > > ... > > > > > + for (fs = 0; fs < ARRAY_SIZE(msa311_fs_table); ++fs) > > > > > > fs++ will work as well. > > > > I would prefer ++fs if you don't mind :) > > Why? It's a non-standard pattern, and needs an explanation. >
On Fri, Aug 5, 2022 at 4:04 PM Dmitry Rokosov <DDRokosov@sberdevices.ru> wrote: > > On Thu, Aug 04, 2022 at 08:30:12PM +0200, Andy Shevchenko wrote: > > > > > > + dev_err(dev, "cannot %s register %u from debugfs (%d)\n", > > > > > > + readval ? "read" : "write", reg, err); > > > > > > > > > > You may consider taking [1] as a precursor here and use str_read_write(). > > > > > > > > > > [1]: https://lore.kernel.org/linux-i2c/20220703154232.55549-1-andriy.shevchenko@linux.intel.com/ > > > > > > > > Oh, really... Thank you for suggestion! > > > > > > I have taken it closer, and it's really helpful and nice, but looks like > > > it's not merged to linux-next. > > > Please advise how I can use it in the driver. Should I provide > > > "Depends-On:" tag as I did for my HZ units patchset? > > > > No, just take that patch into your series. > > Do you mean include your patch to this reply-thread through the > message-id linking? No, just take it as a part of your series. Ah, I wrote almost the same thing above... The idea is you rebase your tree interactively and put a breakpoint to the beginning of your series, then you take a link and run `b4 am -s ...` (-s is important) followed by `git am ...` (b4 will show you the command you need to run). Then you continue your rebasing. Now, when you send a new version of the series, take one more patch into it by changing depth from 3 (the number of patches in your series) to 4 (+ my patch). Generally speaking the HZ series is something different. It's a series which can't be simply taken because it might touch the different subsystem(s). Luckily for you the "different subsystem(s)" is the same subsystem you are taking these patches with. Hence it might not be a problem to attach it as well into a chain. Speaking of lib/ code almost any maintainer can take it via their trees. So taking a _single_ patch against lib/ is fine in most cases.
On Fri, Aug 05, 2022 at 06:03:12PM +0200, Andy Shevchenko wrote: > On Fri, Aug 5, 2022 at 4:04 PM Dmitry Rokosov <DDRokosov@sberdevices.ru> wrote: > > > > On Thu, Aug 04, 2022 at 08:30:12PM +0200, Andy Shevchenko wrote: > > > > > > > + dev_err(dev, "cannot %s register %u from debugfs (%d)\n", > > > > > > > + readval ? "read" : "write", reg, err); > > > > > > > > > > > > You may consider taking [1] as a precursor here and use str_read_write(). > > > > > > > > > > > > [1]: https://lore.kernel.org/linux-i2c/20220703154232.55549-1-andriy.shevchenko@linux.intel.com/ > > > > > > > > > > Oh, really... Thank you for suggestion! > > > > > > > > I have taken it closer, and it's really helpful and nice, but looks like > > > > it's not merged to linux-next. > > > > Please advise how I can use it in the driver. Should I provide > > > > "Depends-On:" tag as I did for my HZ units patchset? > > > > > > No, just take that patch into your series. > > > > Do you mean include your patch to this reply-thread through the > > message-id linking? > > No, just take it as a part of your series. Ah, I wrote almost the same > thing above... > > The idea is you rebase your tree interactively and put a breakpoint to > the beginning of your series, then you take a link and run `b4 am -s > ...` (-s is important) followed by `git am ...` (b4 will show you the > command you need to run). Then you continue your rebasing. Now, when > you send a new version of the series, take one more patch into it by > changing depth from 3 (the number of patches in your series) to 4 (+ > my patch). > > Generally speaking the HZ series is something different. It's a series > which can't be simply taken because it might touch the different > subsystem(s). Luckily for you the "different subsystem(s)" is the same > subsystem you are taking these patches with. Hence it might not be a > problem to attach it as well into a chain. > > Speaking of lib/ code almost any maintainer can take it via their > trees. So taking a _single_ patch against lib/ is fine in most cases. Oh, got it. Thanks a lot for detailed explanation. I'll attach both of them: my HZ units series and your str_read_write() patchset.
On Fri, Aug 5, 2022 at 6:21 PM Dmitry Rokosov <DDRokosov@sberdevices.ru> wrote: > On Fri, Aug 05, 2022 at 06:03:12PM +0200, Andy Shevchenko wrote: ... > Oh, got it. Thanks a lot for detailed explanation. I'll attach both of > them: my HZ units series and your str_read_write() patchset. s/patchset/single patch/ The I2C patches mustn't be included (they are actually rejected, that's why you haven't seen the first patch in the Linux Next).
> > [...] > > > > + return dev_err_probe(dev, err, "cannot enable push-pull int\n"); > > > > interrupt > > It will be more ugly due 80 symbols restriction. These days we let that stretch a little for cases like these. > > > > + indio_dev->modes = 0; /* setup buffered mode later */ > > > > Why explicit assignment to 0? Doesn't kzalloc() do it for you? > > kzalloc() will do it for me, of course. Previously, I initialized modes to > INDIO_DIRECT_MODE to just provide default value for that. Jonathan > suggested to replace it with 0. I did? I wonder what I was smoking that day. Should be set to INDIO_DIRECT_MODE as you had it previously. (From what I recall it will work either way but we have in the past had core code that checked this and may do again in the future so drivers should still be setting it to specify they provide sysfs interfaces to directly read the channels). > I can remove this line at all, no problem. > I just thought, it's more readable.
On Tue, 9 Aug 2022 09:47:54 +0000 Dmitry Rokosov <DDRokosov@sberdevices.ru> wrote: > Hello Jonathan, > > On Sat, Aug 06, 2022 at 04:32:04PM +0100, Jonathan Cameron wrote: > > [...] > > > > +/** > > > + * struct msa311_priv - MSA311 internal private state > > > + * @regs: Underlying I2C bus adapter used to abstract slave > > > + * register accesses > > > + * @fields: Abstract objects for each registers fields access > > > + * @dev: Device handler associated with appropriate bus client > > > + * @lock: Protects msa311 device state between setup and data access routines > > > + * (power transitions, samp_freq/scale tune, retrieving axes data, etc) > > > + * @new_data_trig: Optional NEW_DATA interrupt driven trigger used > > > + * to notify external consumers a new sample is ready > > > + * @vdd: Optional external voltage regulator for the device power supply > > > + */ > > > +struct msa311_priv { > > > + struct regmap *regs; > > > + struct regmap_field *fields[F_MAX_FIELDS]; > > > + > > > + struct device *dev; > > > + struct mutex lock; /* state guard */ > > > > Shouldn't need this comment given documentation above that provides > > more information. > > Without this comment checkpatch.pl raises a warning about uncommented > lock definition. > I agree with you, above comment is redundant, but is it okay to ignore > such warnings before sending the patch? > > I'm talking about below checkpatch condition: > ===== > # check for spinlock_t definitions without a comment. > if ($line =~ /^.\s*(struct\s+mutex|spinlock_t)\s+\S+;/ || > $line =~ /^.\s*(DEFINE_MUTEX)\s*\(/) { > my $which = $1; > if (!ctx_has_comment($first_line, $linenr)) { > CHK("UNCOMMENTED_DEFINITION", > "$1 definition without comment\n" . $herecurr); > } > } > ===== Hmm. I guess checkpatch is more stupid than I thought on this. Definitely fine to ignore that shortcoming of checkpatch. > > > > > > + > > > + struct iio_trigger *new_data_trig; > > > + struct regulator *vdd; > > > +}; > > > > > > > > > > +static irqreturn_t msa311_irq_thread(int irq, void *p) > > > +{ > > > + struct msa311_priv *msa311 = iio_priv(p); > > > + unsigned int new_data_int_enabled; > > > + struct device *dev = msa311->dev; > > > + int err; > > > + > > > + mutex_lock(&msa311->lock); > > > > > + > > > + /* > > > + * We do not check NEW_DATA int status, because of based on > > > + * specification it's cleared automatically after a fixed time. > > > + * So just check that is enabled by driver logic. > > > > That is going to be very problematic if we can have this and events coming > > through the same interrupt pin. Not harmful for now though given you are > > only supporting NEW_DATA for now. Just something to watch out for. > > > > Actually, I have run some experiments with NEW_DATA status bits. And > looks like we can't determince actual status of NEW_DATA virtual > interrupt when physical IRQ is raised. I will back to this problem when > begin Motion Events feature implementation. > > [...] > > > > + err = devm_pm_runtime_enable(dev); > > > + if (err) > > > + return err; > > > + > > > + pm_runtime_get_noresume(dev); > > > + pm_runtime_set_autosuspend_delay(dev, MSA311_PWR_SLEEP_DELAY_MS); > > > + pm_runtime_use_autosuspend(dev); > > > + > > > + err = msa311_chip_init(msa311); > > > + if (err) > > > + return err; > > > + > > > + indio_dev->modes = 0; /* setup buffered mode later */ > > > > As per other branch, I led you astray here it seems. > > > > Sorry, I've made a mistake. Comment about INDIO_DIRECT_MODE was left > by Andy here: > > https://lore.kernel.org/linux-iio/CAHp75Vc0+ckNnm2tzLMPrjeFRjwoj3zy0C4koNShFRG3kP8b6w@mail.gmail.com/ > > [...] No problem. That's an odd historical corner case. I liked having clear values for 'currentmode' (now in iio_opaque) and those matching the bits set in available modes. So even if we didn't set it we'd end up with a reserved bit which would add extra confusion. The direct mode is currently just used as a placeholder for 'not a buffered mode', rather than the state variable has never been set. Jonathan >
MSA311 is a tri-axial, low-g accelerometer with I2C digital output for sensitivity consumer applications. It has dynamic user-selectable full scales range of +-2g/+-4g/+-8g/+-16g and allows acceleration measurements with output data rates from 1Hz to 1000Hz. Spec: https://cdn-shop.adafruit.com/product-files/5309/MSA311-V1.1-ENG.pdf This driver supports following MSA311 features: - IIO interface - Different power modes: NORMAL and SUSPEND (using pm_runtime) - ODR (Output Data Rate) selection - Scale and samp_freq selection - IIO triggered buffer, IIO reg access - NEW_DATA interrupt + trigger Below features to be done: - Motion Events: ACTIVE, TAP, ORIENT, FREEFALL - Low Power mode Also this patchset has new vendor prefix for MEMSensing Microsystems and MSA311 dt-binding schema. You can test msa311 driver using libiio and gnuplot following below instructions: $ # Create hrtimer trigger object $ mkdir /sys/kernel/config/iio/triggers/hrtimer/iio_hrtimer_trigger $ # Read 4K samples using msa311-new-data trigger (irq) and $ # buffer with depth equals to 64 samples and rotate device a little bit $ iio_readdev -u "local:" -b 64 -s 4096 -t msa311-new-data -T 0 \ $ msa311 > /tmp/msa311.dat $ # Or using hrtimer trigger instead of msa311-new-data trigger $ iio_readdev -u "local:" -b 64 -s 4096 -t iio_hrtimer_trigger -T 0 \ $ msa311 > /data/local/tmp/msa311.dat $ cat <<EOF >> msa311_data.gnu set title "MSA311 Accel Data" set key below set xdata time set format x "%H:%M\n%.4S" set xlabel "timestamp" set autoscale y plot 'msa311.dat' binary endian=little \ format='%int16%int16%int16%uint16%uint64' using \ (\$5/1000000000):(int(\$1)/16) title "acc_x" \ with lines,\\ 'msa311.dat' binary endian=little \ format='%int16%int16%int16%uint16%uint64' using \ (\$5/1000000000):(int(\$2)/16) title "acc_y" \ with lines,\\ 'msa311.dat' binary endian=little \ format='%int16%int16%int16%uint16%uint64' using \ (\$5/1000000000):(int(\$3)/16) title "acc_z" with lines EOF $ gnuplot --persist msa311_data.gnu Depends-On: https://lore.kernel.org/linux-iio/20220801143811.14817-1-ddrokosov@sberdevices.ru/ Changes: * v3->v4: - totally reworked pm_runtime flow based on Jonathan suggestions - replaced temporary field variable with tmp pointer to field in the MSA311_GENMASK macro helper - removed i2c pointer from MSA311 private context, retrieved it from msa311 object, if anything - added struct device *dev reference to MSA311 private context for easier msa311->dev translation - moved regmap pointer to the beginning of MSA311 private context to save some instructions - refactored 'if' conditions to be positive and shorter - moved msa311_check_partid() and msa311_soft_reset() to separate routines and call them before powerup IP logic during probe() execution - used str_enable_disable() string helper as Andy suggested - used msa311_pwr_modes const char * array to translate power modes to strings - reworked hz->ms translation, used MICROHZ_PER_HZ from the following review: https://lore.kernel.org/linux-iio/20220801143811.14817-1-ddrokosov@sberdevices.ru/ - moved dev_dbg() log about MSA311 compatible chip finding under partid check - refactored stack variables definitions based on "longer lines first" thumb - used 0 instead of INDIO_DIRECT_MODE before iio buffer setup - moved i2c->irq check to msa311_setup_interrupts() - removed dev_dbg() prints from ->resume() and ->suspend() callbacks - removed "description" fields from "interrupts" and i2c "reg" YAML schema nodes - implemented simple power supply for MSA311 (vdd-supply) - reworked shared_by_all info mask to shared_by_type for MSA311 accel channels - tagged datasheet URL link in the commit message - made mutex-based critical section shorter inside odr and fs loop as Jonathan suggested - fixed wording in the commit messages and comments a little bit, refactored some indentations - replaced blank lines between register offset definitions with short comments * v2->v3: - removed MSA311_TIMESTAMP_CHANNEL() macro, used IIO_CHAN_SOFT_TIMESTAMP directly - do not call dev_err_probe() inside functions, which is used not only from probe() path - simplified error handling a little bit - used iio_device_claim_direct_mode() and iio_device_release_direct_mode() to lock attributes when buffer mode is enabled - prohibited sampling frequency changing during buffer usage because otherwise sometimes MSA311 returns outliers when frequency values grow up in the read operation moment - allowed scale value changing when buffer mode is enabled - removed IRQF_TRIGGER_RISING irq flag from irg registration because it's provided from device tree directly - do not switch off autosuspend from powerdown() devm callback, because it's already done from pm_runtime_disable() during devm pm_runtime actions - provided more information why we need force suspend state for MSA311 in the powerdown flow - reworked comments stuff: removed obvious extra comments, provided more details in the complex driver code places * v1->v2: - memsensing vendor prefix was moved to right place by alphabetical order - LOW mode mention was deleted, because LOW mode isn't supported in the current driver version - reworked some enums with gaps to defines - reworked register names as Jonathan mentioned in the v1 - do not use regmap_field API for entire registers - deleted all extra comments - supported info_mask_*_avail bitmaps instead of explicit IIO attrs definitions, implemented read_avail() callback for samp_freq and scale values - msa311 mutex is still used to protect msa311 power transitions, samp_freq/scale tune and axes data handling; described this lock more informative - ask new_data interruption status from appropriate register, do not hold atomic variable for that - optimized reads of axes data by I2C using regmap_bulk API - use dev_err_probe() instead of dev_err() for all probe() code paths - from now all I2C bus communication failures are interpreted as errors - described wait_from_next() semantic better - deleted all unneeded pm wrappers - interpreter all axes data as __le16 type and adjust them to right format (endianness + sign) for raw() flow only - redesigned msa311_fs_table[] to 2D matrix (it's more comfortable format for read_avail() callback) - align and initialize msa311 buffer before pushing properly - use pm_runtime resume and suspend from buffer preenable/postdisable, deleted them from trigger set_state - supported multiple trigger usage (tested with external hrtimer trigger and internal new_data trigger) - moved all irq related stuff to msa311_setup_interrupts() routine - implemented msa311_powerdown() devm release action - reworked initialization of pm_runtime msa311 flow, use autosuspend logic - purged driver remove() callback, because of devm release logic runs all deinit stuff fully - fixed dts bindings problems - changed irq type in the dt-binding description, because interrupt type for msa311 should have the same type as i2c irq, for example using the gpio_intc it's IRQ_TYPE_EDGE_RISING usually. Otherwise we may lose irq map on the second and further insmod attempts Dmitry Rokosov (3): dt-bindings: vendor-prefixes: add MEMSensing Microsystems Co., Ltd. iio: add MEMSensing MSA311 3-axis accelerometer driver dt-bindings: iio: accel: add dt-binding schema for msa311 accel driver .../bindings/iio/accel/memsensing,msa311.yaml | 52 + .../devicetree/bindings/vendor-prefixes.yaml | 2 + MAINTAINERS | 7 + drivers/iio/accel/Kconfig | 13 + drivers/iio/accel/Makefile | 2 + drivers/iio/accel/msa311.c | 1326 +++++++++++++++++ 6 files changed, 1402 insertions(+) create mode 100644 Documentation/devicetree/bindings/iio/accel/memsensing,msa311.yaml create mode 100644 drivers/iio/accel/msa311.c