Mpu6050 device improvement and cdev usage #33

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

mpu6050/mpu6050_data.h

+	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

mpu6050/mpu6050_data.h

+	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

mpu6050/mpu6050_cdev.c

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

mpu6050/mpu6050_cdev.c

+	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

mpu6050/mpu6050_cdev.h

+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

mpu6050/mpu6050_main.c

+	}
+
+	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

mpu6050/mpu6050_main.c

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

mpu6050/mpu6050_main.c

+		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

mpu6050/mpu6050_main.c

+	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

mpu6050/mpu6050_main.c

+/* 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 an1kh and AleksandrBulyshchenko 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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Mpu6050 device improvement and cdev usage #33

Mpu6050 device improvement and cdev usage #33

sergiiromantsov commented Nov 19, 2017 •

edited

Loading

an1kh commented Nov 21, 2017

sergiiromantsov commented Nov 22, 2017

an1kh left a comment

sergiiromantsov commented Dec 12, 2017

an1kh commented Dec 13, 2017

AleksandrBulyshchenko left a comment

AleksandrBulyshchenko Dec 13, 2017

AleksandrBulyshchenko Dec 13, 2017

sergiiromantsov Jan 1, 2018

AleksandrBulyshchenko Dec 13, 2017

sergiiromantsov Jan 1, 2018

AleksandrBulyshchenko Dec 13, 2017

sergiiromantsov Jan 1, 2018

AleksandrBulyshchenko Dec 13, 2017

sergiiromantsov Jan 1, 2018

AleksandrBulyshchenko Dec 13, 2017

sergiiromantsov Jan 1, 2018

AleksandrBulyshchenko Dec 13, 2017

sergiiromantsov Jan 1, 2018

AleksandrBulyshchenko Dec 13, 2017

sergiiromantsov Jan 1, 2018

AleksandrBulyshchenko Dec 13, 2017

sergiiromantsov Jan 1, 2018

AleksandrBulyshchenko Dec 13, 2017

sergiiromantsov Jan 1, 2018

AleksandrBulyshchenko Jan 10, 2018

AleksandrBulyshchenko left a comment

Mpu6050 device improvement and cdev usage #33

Are you sure you want to change the base?

Mpu6050 device improvement and cdev usage #33

Conversation

sergiiromantsov commented Nov 19, 2017 • edited Loading

an1kh commented Nov 21, 2017

sergiiromantsov commented Nov 22, 2017

an1kh left a comment

Choose a reason for hiding this comment

sergiiromantsov commented Dec 12, 2017

an1kh commented Dec 13, 2017

AleksandrBulyshchenko left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AleksandrBulyshchenko left a comment

Choose a reason for hiding this comment

sergiiromantsov commented Nov 19, 2017 •

edited

Loading