ssd1306 device driver example by DevyatovAndrey · Pull Request #37 · Kernel-GL-HRK/gl-kernel-training-2017

DevyatovAndrey · 2017-11-20T15:07:02Z

This adds step by step example of writing kernel device driver for OLED LCD display based on ssd1306 controller.

AleksandrBulyshchenko

@DevyatovAndrey,
Thank you for sharing the driver.

But could you rebase the branch onto this repo tree?
Because currently it's impossible to diff separate commits via github.

And also if possible -please fix checkpatch issues (at least for the ssd1306, but it make sense for userapp_analog_clock as well):

${BBB_KERNEL}/scripts/checkpatch.pl --file ${TRAINING_REPO}/ssd1306/*.[ch] --terse

AleksandrBulyshchenko · 2017-11-20T17:31:10Z

+obj-m := ssd1306.o
+
+KDIR ?= ${HOME}/BBB/bb_kernel/
+#KDIR ?= ~/BBB/KERNEL/


Please avoid hardcodding any local path.
And unify expected env. vars with existent mpu6050 driver (see its makefile)

AleksandrBulyshchenko · 2017-11-20T17:37:38Z

@@ -0,0 +1,41 @@
+#!/bin/bash -e
+
+export CROSS_COMPILE=${HOME}/BBB/gcc-linaro-5.3.1-2016.05-x86_64_arm-linux-gnueabihf/bin/arm-linux-gnueabihf-


This hardcodded export will override anything set in user's environment.
Please avoid committing your paths.

AleksandrBulyshchenko · 2017-11-20T17:44:38Z

@@ -0,0 +1,415 @@
+/*


Could you use the same baseline version of am335x-bone-common.dtsi as mpu6050 uses? (see ee49cc6)

AleksandrBulyshchenko · 2017-11-20T17:57:59Z

+
+clean:
+	rm fb_user
+


This makefile has very odd view.
Does it work at all?

I believe you meant something like the following

CROSS_COMPILE ?= arm-linux-gnueabihf- CC = $(CROSS_COMPILE)gcc CFLAGS = -Wall .PHONY: all clean all: analog_clock analog_clock: analog_clock.c $(CC) $(CFLAGS) $^ -o $@ clean: rm analog_clock

AleksandrBulyshchenko · 2017-11-20T17:59:29Z

@@ -0,0 +1,11 @@
+#!/bin/bash -e
+
+export CROSS_COMPILE=${HOME}/BBB/gcc-linaro-arm-linux-gnueabihf-4.9-2014.09_linux/bin/arm-linux-gnueabihf-


Please avoid using hardcoded absolute paths - use just filename and add its location to $PATH

AleksandrBulyshchenko · 2017-11-20T18:01:38Z

@@ -0,0 +1,11 @@
+#!/bin/bash -e


Why do you have shell script and makefile same time.
I believe something one is enough (preferably Makefile if you don't have to use some complex shell scripting)

It's just to have an unified way to build and deploy this application

So which one is correct?
Now they use different options.

Either make should be called from the shell script and everything have to be defined in the makefile (as it done for ssd1306);

or one of them should be removed to avoid confusion.

DevyatovAndrey · 2017-11-21T12:07:14Z

@AleksandrBulyshchenko,

The branch is rebased onto this repo tree.
Fixed checkpatch issues
Removed unnesessary export of enviroment variabless

AleksandrBulyshchenko

@DevyatovAndrey,
Thank you for the fixes.

Here some more comments:

It's better to remove mistakenly added build artefacts right in that commit (8e7c145).
What about unifying am335x-bone-common.dtsi base with mpu6050? (will require editing of f243df6)
If possible, please provide more descriptive commit messages.

AleksandrBulyshchenko · 2017-11-21T13:30:04Z

-    u8      data[LCD_WIDTH * LCD_HEIGHT / 8];
-
+	u8      data[LCD_WIDTH * LCD_HEIGHT / 8];
 } ssd1306_data_array;


ssd1306.c:90: WARNING: do not add new typedefs

AleksandrBulyshchenko · 2017-11-21T13:31:32Z

+
+	if (unlikely(ret < 0)) {
 		printk(KERN_ERR "i2c_master_send() has returned ERROR %d\n", ret);
 		return ret;


Better to use pr_err()

AleksandrBulyshchenko · 2017-11-21T13:55:10Z

@@ -4,15 +4,15 @@ export CROSS_COMPILE=${HOME}/BBB/gcc-linaro-5.3.1-2016.05-x86_64_arm-linux-gnuea



I don't think this envsetup.sh should be published at all.
It will be different for everybody (and don't have to be at all)

How to set up enviroment variables needed for these examples if there is no "envsetup.sh"?

The idea of using environment variables - is exactly to allow to use own environment, which is different for every user (otherwise they should be defined in build scripts instead of passing from environment).
So storing environment settings in VCS doesn't make much sense, because they either will be changed by users and will create a noise in git status or won't be used.

AleksandrBulyshchenko · 2017-11-21T13:56:58Z

@@ -0,0 +1,11 @@
+#!/bin/bash -e


So which one is correct?
Now they use different options.

Either make should be called from the shell script and everything have to be defined in the makefile (as it done for ssd1306);

or one of them should be removed to avoid confusion.

AleksandrBulyshchenko · 2017-11-21T14:18:02Z

+                echo "configure kernel"
+                make ARCH=arm config
+                ;;
+


redundant space line

AleksandrBulyshchenko · 2017-11-21T14:19:36Z

+PWD := $(shell pwd)
+
+default:
+		  $(MAKE) ARCH=arm  -C $(KDIR) M=$(PWD) modules


Please unify indentation - use single tab

added READMEs

DevyatovAndrey · 2017-12-14T12:42:39Z

I've updated this PR:

removed mistakenly added build artefacts
unified am335x-bone-common.dtsi base with mpu6050

DevyatovAndrey changed the title ~~Andrii.deviatov~~ ssd1306 device driver example Nov 20, 2017

AleksandrBulyshchenko requested changes Nov 20, 2017

View reviewed changes

AleksandrBulyshchenko added the RFC (example) label Nov 20, 2017

DevyatovAndrey closed this Nov 21, 2017

DevyatovAndrey force-pushed the Andrii.Deviatov branch from 2fbf462 to 79f1a87 Compare November 21, 2017 09:10

DevyatovAndrey reopened this Nov 21, 2017

AleksandrBulyshchenko reviewed Nov 21, 2017

View reviewed changes

joy4eg mentioned this pull request Dec 10, 2017

ssd1306: add driver with optimizations #48

Closed

AleksandrBulyshchenko assigned DevyatovAndrey Dec 12, 2017

DevyatovAndrey force-pushed the Andrii.Deviatov branch from 32ee952 to 6d1bb82 Compare December 14, 2017 12:35

DevyatovAndrey added 12 commits December 14, 2017 14:41

ssd1306: initial commit - I2C driver skeleton

96a4232

ssd1306: added auxilary files - Makefile, build script and devicetree

0810ea3

ssd1306: added sysfs entries

5890e2e

ssd1306: added I2C communications to LCD

83b3990

ssd1306: added graphics primitives and test

f6bd465

ssd1306: added i2c1 node

e433e2b

ssd1306: added framebuffer support

db035ec

ssd1306: LCD update speedup

25855b8

ssd1306: added delayed workqueue for screen updating

4d3b43d

analog_clock: userspace application added

813a22f

ssd1306: fixed checkpatch errors/warnings;

148b340

added READMEs

ssd1306: fixed build scripts and Makefiles

665c191

DevyatovAndrey force-pushed the Andrii.Deviatov branch from 6d1bb82 to 665c191 Compare December 14, 2017 12:41

		@@ -0,0 +1,41 @@
		#!/bin/bash -e

		export CROSS_COMPILE=${HOME}/BBB/gcc-linaro-5.3.1-2016.05-x86_64_arm-linux-gnueabihf/bin/arm-linux-gnueabihf-

		@@ -0,0 +1,11 @@
		#!/bin/bash -e

		export CROSS_COMPILE=${HOME}/BBB/gcc-linaro-arm-linux-gnueabihf-4.9-2014.09_linux/bin/arm-linux-gnueabihf-

		@@ -4,15 +4,15 @@ export CROSS_COMPILE=${HOME}/BBB/gcc-linaro-5.3.1-2016.05-x86_64_arm-linux-gnuea

Uh oh!

Conversation

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

Uh oh!

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!

DevyatovAndrey commented Nov 21, 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!

DevyatovAndrey commented Dec 14, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

DevyatovAndrey commented Nov 20, 2017 •

edited

Loading