Mpu6050 device improvement and cdev usage by sergiiromantsov · Pull Request #33 · Kernel-GL-HRK/gl-kernel-training-2017

sergiiromantsov · 2017-11-19T18:37:52Z

Properly rebased pull-request #29 (up to commit d2f5920).
Improved mpu6050 implementation.
Added storage for 10 latest reads from mpu6050.
Added cdevs to print data in one-line and ten-lines format.
Added timer to read data each 1000 msecs.

an1kh · 2017-11-21T12:45:39Z

I have a question: what was the reason for using sysfs kobject?

Also have several warnings about format:

avoid using characters like minuses in commit messages, they may be treated as diff info.
change the author completely or add a second line, don't modify the original one.
avoid using c++ comments (use c-style /* */), gcc-style struct declarations (use c99: .xyz = 123; ).

sergiiromantsov · 2017-11-22T17:31:05Z

<<< I have a question: what was the reason for using sysfs kobject?
To be allowed to use sysfs_create_files
<<< avoid using characters like minuses in commit messages, they may be treated as diff info.
ok

an1kh

Please fix checkpatch errors (and warnings if possible). e.g:

../../linux/scripts/checkpatch.pl -f *.c
../../linux/scripts/checkpatch.pl -f *.h

Code itself is OK. Please fix format issues and I will have no objections.

sergiiromantsov · 2017-12-12T14:40:10Z

CS is done
Another PRs will be able to fix after 25 December

an1kh · 2017-12-13T08:23:04Z

Thank you! Please merge

AleksandrBulyshchenko

I've made shallow review and haven't tested the solution.

Generally I like the work, which you've done. 👍
The driver looks good despite of unneeded complexity in some parts.

I've commented inline some issues.

And here are some more general notes:

Implementation of mpu6050_data module looks like example of overengineering for this case:
- If you want to create generic collection, it shouldn't use mpu6050-specific types;
- If its the only purpose to store 10 samples from the mpu6050 - it might be much simpler;
Please use conventional form of tagging commit subjects (mpu6050: ... in this case);
Also commit messages may be more descriptive;
And don't forget SOB line;
7c095fa ("[Lesson 6] mpu6050 review fix") is empty commit;

IMO at least commit messages and some obvious issues should be fixed before merging.

AleksandrBulyshchenko · 2017-12-13T12:41:00Z

+	struct mpu6050_data_list *element_iter_current;
+	struct mpu6050_data_list list;
+};
+


Please, never ever do like that.
Functions (except inline) mustn’t be defined in header files!

AleksandrBulyshchenko · 2017-12-13T13:07:57Z

+	INDEX_GYRO_Z,
+	INDEX_TEMPERATURE,
+	INDEX_COUNT,
+	/* Another level of data*/


Please avoid mixing indexes of different arrays within the same enum.
The following are not for the data - they are for extra_data and enum should be named accordingly.

AleksandrBulyshchenko · 2017-12-13T13:17:50Z

+
+struct cdevs_holder *get_cdevs(void)
+{
+	static struct cdevs_holder g_cdevs_holder;


Why the driver's static data defined as local variable?
It may have some sense for future compatibility if you're going to expand the driver to support several instances.
But AFAIK this is not the case.
Right now it makes driver understanding and debugging harder with no benefit.

AleksandrBulyshchenko · 2017-12-13T13:27:17Z

+	if (result < 0)
+		goto error1;
+	cdevs->cdev_full.major = MAJOR(cdevs->cdev_full.dev_no);
+	cdevs->cdev_line.read_all = false;


Looks like copy-paste error is here.

AleksandrBulyshchenko · 2017-12-13T13:46:12Z

+struct cdev_instance {
+	unsigned int major;
+	dev_t dev_no;
+	dev_t read_all;


The type of read_all seems to be wrong.

AleksandrBulyshchenko · 2017-12-13T15:23:25Z

+	}
+
+	g_reading_thread = kthread_run(reading_thread, NULL, "mpu6050_read_th");
+	if (!g_reading_thread) {


Wrong error handling - kthread_run will return negative code in case of errors.
IS_ERR macro should be used for verification.

AleksandrBulyshchenko · 2017-12-13T15:28:39Z

+
+	if (g_reading_thread) {
+		complete(&g_read_comletion);
+		kthread_stop(g_reading_thread);


It makes sense to set should_stop flag by kthread_stop before waking up the thread by complete.

AleksandrBulyshchenko · 2017-12-13T15:31:16Z

+		kthread_stop(g_reading_thread);
+	}
+	result = del_timer_sync(&g_read_timer);
+	pr_info("mpu6050: timer deleted %d\n", result);


Logically timer should be stopped before deleting the thread.

AleksandrBulyshchenko · 2017-12-13T15:33:32Z

+	NULL,
+};
+
+/* Gets a number of attributes + 1 (null-terminated) */


In current implementation there's no need in attributes_count() function.
So why not to simplify code a bit?

AleksandrBulyshchenko · 2017-12-13T15:51:20Z

+/* functionality */
+static size_t get_attribute_index(struct kobj_attribute const *attribute);
+
+static int mpu6050_read_data(bool debug)


This is definitely wrong way of controlling debug printout.
It should be done on preprocessing stage.
Consider defining macro wrapper over dev_info, which will be expanded to nothing if debug mode not defined.

That is over-control. It used to allow do not print for any type: or debug, or release

Renaming the argument to print_out doesn't change anything.
The value provided to it is hardcodded, so there's no sense to keep it as parameter - define should be used instead.

Instead of sysfs class used sysfs kobject. Files-data created at once. Reading of data is done by one function. Controlled resources on initialization failure.

Added functionality to hold 10 last reads of data from mpu6050 device.

Data from device mpu6050 will be read not often than 1 time per second.

Fixed misprint in storing timestamp: it was possibility to store diff of timestamps instead of real value.

Added charetcer-device that allows to read mpu6050-device data in one-line format.

Implementation from header-files (mpu6050_cdev.h, mpu6050_data.h) is moved to source-files.

File with initialization-functions wasn't compiled.

Avoided usage of c++-style comments. Added author.

Added support to dump latest 10 reads from mpu6050 by charecter device.

Structure of data and access to them are incapsulated to allow exclusive access and synchronization.

Added synchronization for data reading/writing.

Timer is added to read data each 1000 msecs.

Code correction according to checkpatch.pl

Cleaned code: avoided unused functions. Added support to read to user buffer with requested size. Corrected misprints for variable types. Divided indexes for different data. Improved module unitialization.

AleksandrBulyshchenko

:+1

sergiiromantsov mentioned this pull request Nov 19, 2017

[Only one cdev]Mpu6050 device #29

Closed

sergiiromantsov added the ready for review label Nov 27, 2017

sergiiromantsov requested review from AleksandrBulyshchenko and an1kh November 27, 2017 18:17

an1kh requested changes Dec 12, 2017

View reviewed changes

an1kh approved these changes Dec 13, 2017

View reviewed changes

AleksandrBulyshchenko reviewed Dec 13, 2017

View reviewed changes

AleksandrBulyshchenko added reviewed and removed ready for review labels Dec 13, 2017

AleksandrBulyshchenko assigned sergiiromantsov Dec 13, 2017

sergiiromantsov added 14 commits January 2, 2018 20:13

[Lesson 6] mpu6050: improved driver implementation

6ba0b70

Instead of sysfs class used sysfs kobject. Files-data created at once. Reading of data is done by one function. Controlled resources on initialization failure.

[Lesson 6] mpu6050: added data storage.

b0ee4c4

Added functionality to hold 10 last reads of data from mpu6050 device.

[Lesson 6] mpu6050: improved device-data reading.

b1d5e73

Data from device mpu6050 will be read not often than 1 time per second.

[Lesson 6] mpu6050: misprint in timestamp

3c872ce

Fixed misprint in storing timestamp: it was possibility to store diff of timestamps instead of real value.

[Lesson 6] mpu6050: added one-line charecter-device

e28e105

Added charetcer-device that allows to read mpu6050-device data in one-line format.

[Lesson 6] mpu6050: implementation moved from h-files

d747102

Implementation from header-files (mpu6050_cdev.h, mpu6050_data.h) is moved to source-files.

[Lesson 6] mpu6050: restored module loadability

ff9c9b7

File with initialization-functions wasn't compiled.

[Lesson 6] mpu6050: review fixes

9b15c7f

Avoided usage of c++-style comments. Added author.

[Lesson 6] mpu6050: cdev to dump last 10 reads added.

b59194d

Added support to dump latest 10 reads from mpu6050 by charecter device.

[Lesson 6] mpu6050: data-structure is incapsulated.

38fb790

Structure of data and access to them are incapsulated to allow exclusive access and synchronization.

[Lesson 6] mpu6050: data access synchronization.

f43c2ac

Added synchronization for data reading/writing.

[Lesson 6] mpu6050: added timer to read data

f48bd18

Timer is added to read data each 1000 msecs.

[Lesson 6] mpu6050: codestyle fixes.

69d4102

Code correction according to checkpatch.pl

[Lesson 6] mpu6050: review fixes

9661f28

Cleaned code: avoided unused functions. Added support to read to user buffer with requested size. Corrected misprints for variable types. Divided indexes for different data. Improved module unitialization.

sergiiromantsov force-pushed the mpu6050_rsa_clean branch from f8d5c26 to 9661f28 Compare January 2, 2018 19:08

sergiiromantsov added the ready for review label Jan 6, 2018

sergiiromantsov removed the reviewed label Jan 6, 2018

sergiiromantsov assigned AleksandrBulyshchenko and unassigned sergiiromantsov Jan 6, 2018

AleksandrBulyshchenko approved these changes Jan 10, 2018

View reviewed changes

AleksandrBulyshchenko assigned sergiiromantsov and unassigned AleksandrBulyshchenko Jan 10, 2018

AleksandrBulyshchenko added reviewed and removed ready for review labels Jan 10, 2018

Uh oh!

Conversation

sergiiromantsov commented Nov 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

an1kh commented Nov 21, 2017

Uh oh!

sergiiromantsov commented Nov 22, 2017

Uh oh!

an1kh left a comment

Choose a reason for hiding this comment

Uh oh!

sergiiromantsov commented Dec 12, 2017

Uh oh!

an1kh commented Dec 13, 2017

Uh oh!

AleksandrBulyshchenko left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AleksandrBulyshchenko left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

sergiiromantsov commented Nov 19, 2017 •

edited

Loading