MBED OS 3 to 5 changes added for SPI #2866

pradeep-gr · 2016-09-29T13:01:19Z

Notes:

Pull requests will not be accepted until the submitter has agreed to the contributer agreement.
This is just a template, so feel free to use/remove the unnecessary things

Description

A few sentences describing the overall goals of the pull request's commits.

Status

READY/IN DEVELOPMENT/HOLD

Migrations

If this PR changes any APIs or behaviors, give a short description of what API users should do when this PR is merged.

YES | NO

Related PRs

List related PRs against other branches:

branch	PR
other_pr_production	link
other_pr_master	link

Todos

Tests
Documentation

Deploy notes

Notes regarding the deployment of this PR. These should note any
required changes in the build environment, tools, compilers, etc.

Steps to test or reproduce

Outline the steps to test or reproduce the PR here.

bridadan

Thanks for the PR @pradeep-gr!

I made a few comments about the styling, would you mind taking a look? Here's our style guide that I usually refer to:
https://github.com/ARMmbed/mbed-os/blob/master/docs/COMMITTERS.md#c-coding-rules--coding-guidelines

Thanks!

bridadan · 2016-09-30T20:47:58Z

hal/targets/hal/TARGET_ONSEMI/TARGET_NCS36510/ncs36510_spi.c

@@ -75,29 +75,40 @@ void fSpiInit(spi_t *obj, PinName mosi, PinName miso, PinName sclk, PinName ssel
        CLOCK_ENABLE(CLOCK_SPI2);		/* Enable clock */
    }

+	CLOCK_ENABLE(CLOCK_CROSSB);


Could you please fix the indent here?

bridadan · 2016-09-30T20:48:08Z

hal/targets/hal/TARGET_ONSEMI/TARGET_NCS36510/ncs36510_spi.c

-    padRegOffset->PADIO2.BITS.POWER = 1; /* miso: Drive strength */
-    CLOCK_DISABLE(CLOCK_PAD);
+	/* Pad settings */
+	CLOCK_ENABLE(CLOCK_PAD);


Could you please fix the indent here?

bridadan · 2016-09-30T20:48:19Z

hal/targets/hal/TARGET_ONSEMI/TARGET_NCS36510/ncs36510_spi.c

+	}
+	CLOCK_DISABLE(CLOCK_PAD);
+	CLOCK_DISABLE(CLOCK_GPIO);
+	CLOCK_DISABLE(CLOCK_CROSSB);


Could you please fix the indent here?

bridadan · 2016-09-30T20:49:16Z

hal/targets/hal/TARGET_ONSEMI/TARGET_NCS36510/spi_api.c

@@ -59,24 +59,28 @@ void spi_free(spi_t *obj)

 void spi_format(spi_t *obj, int bits, int mode, int slave)
 {
-    if(slave) {
+    if(slave) 
+	{


Could you please revert this change to the bracket? It does not fit with our style guide: https://github.com/ARMmbed/mbed-os/blob/master/docs/COMMITTERS.md#c-coding-rules--coding-guidelines

Brian,
As per your comment
"if(slave){"
should be changed to
"if(slave)
{"

Can you confirm this. Because as per coding guidelines
"if(slave){" is correct

should be

if (condition) { do_sth; }

bridadan · 2016-09-30T20:49:30Z

hal/targets/hal/TARGET_ONSEMI/TARGET_NCS36510/spi_api.c

-    } else {
+    } 
+	else 
+	{


Could you please revert this change to the bracket? It does not fit with our style guide: https://github.com/ARMmbed/mbed-os/blob/master/docs/COMMITTERS.md#c-coding-rules--coding-guidelines

bridadan · 2016-09-30T20:49:37Z

hal/targets/hal/TARGET_ONSEMI/TARGET_NCS36510/spi_api.c

 }

 void spi_frequency(spi_t *obj, int hz)
 {
    /* If the frequency is outside the allowable range, set it to the max */
-    if(hz > SPI_FREQ_MAX) {
+    if(hz > SPI_FREQ_MAX) 
+	{


Could you please revert this change to the bracket? It does not fit with our style guide: https://github.com/ARMmbed/mbed-os/blob/master/docs/COMMITTERS.md#c-coding-rules--coding-guidelines

maclobdell · 2016-10-04T14:33:36Z

@bridadan the CI log doesn't give me much of a clue why it failed. Can you check?

bridadan · 2016-10-04T14:38:18Z

@maclobdell I think the CI that's currently complaining is due to issues in Oulu at the moment from the restructure PR that went in.

@pradeep-gr The restructure PR also affected this PR. Could you please rebase?

pradeep-gr · 2016-10-05T12:15:07Z

Rebased fork and pull request is updated. Please let me know everything is fine with rebase & PR.
Can you please let me know this PR can be merged.

pradeep-gr · 2016-10-07T08:35:19Z

Any update on the PR?

0xc0170 · 2016-10-07T13:12:42Z

@bridadan Requested changes were done.

@pradeep-gr Thanks for running astyle. Be aware that running it on the files that come outside of mbed, like drivers. It's not recommended to run astyle on those, as it makes difficult future integration (diff noise whitespaces, indentation, etc). I have noticed this on the files like targets/TARGET_ONSEMI/TARGET_NCS36510/pmu_map.h.

0xc0170

Besides changing some external files (probably it was an intention and all is good as it is), LGTM

0xc0170 · 2016-10-07T13:14:15Z

/morph test

bridadan

Thanks for the changes!

mbed-bot · 2016-10-07T15:58:56Z

Result: SUCCESS

Your command has finished executing! Here's what you wrote!

/morph test

Output

mbed Build Number: 1076

All builds and test passed!

sg- · 2016-10-07T16:37:28Z

@pradeep-gr There was a consolidation of the cmsis files to make switching versions easier. This caused a conflict. Can you resolve?

pradeep-gr · 2016-10-12T09:30:25Z

Re-based to update folder name and location from cmsis to device.

sg- · 2016-10-12T13:14:51Z

/morph test

mbed-bot · 2016-10-12T14:26:35Z

Result: SUCCESS

Your command has finished executing! Here's what you wrote!

/morph test

Output

mbed Build Number: 0

All builds and test passed!

pradeep-gr · 2016-10-12T15:25:58Z

Commit 22c50d3 also contains OS 5 changes for low power timer.

bridadan suggested changes Sep 30, 2016

View reviewed changes

bridadan added the needs: work label Sep 30, 2016

pradeep-gr force-pushed the master branch from 24798e6 to cb26642 Compare October 5, 2016 12:05

0xc0170 approved these changes Oct 7, 2016

View reviewed changes

0xc0170 added needs: CI and removed needs: work labels Oct 7, 2016

bridadan approved these changes Oct 7, 2016

View reviewed changes

pradeep-gr force-pushed the master branch 2 times, most recently from 798633c to 3bd8a5b Compare October 12, 2016 09:21

cmsis to device changes

22c50d3

pradeep-gr force-pushed the master branch from 3bd8a5b to 22c50d3 Compare October 12, 2016 09:26

Low power timer fine tuned for smaller duration

324dbaf

sg- added ready for merge and removed needs: CI labels Oct 12, 2016

sg- merged commit ffe05f7 into ARMmbed:master Oct 12, 2016

sg- added release-version: 5.2.0-rc3 and removed ready for merge labels Oct 12, 2016

pradeep-gr mentioned this pull request Oct 18, 2016

NCS36510 - lp timeout failure #3038

Closed

MBED OS 3 to 5 changes added for SPI #2866

MBED OS 3 to 5 changes added for SPI #2866

Uh oh!

Conversation

pradeep-gr commented Sep 29, 2016

Description

Status

Migrations

Related PRs

Todos

Deploy notes

Steps to test or reproduce

Uh oh!

bridadan 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!

pradeep-gr Oct 3, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

maclobdell commented Oct 4, 2016

Uh oh!

bridadan commented Oct 4, 2016

Uh oh!

pradeep-gr commented Oct 5, 2016

Uh oh!

pradeep-gr commented Oct 7, 2016

Uh oh!

0xc0170 commented Oct 7, 2016

Uh oh!

0xc0170 left a comment

Choose a reason for hiding this comment

Uh oh!

0xc0170 commented Oct 7, 2016

Uh oh!

bridadan left a comment

Choose a reason for hiding this comment

Uh oh!

mbed-bot commented Oct 7, 2016

Output

Uh oh!

sg- commented Oct 7, 2016

Uh oh!

pradeep-gr commented Oct 12, 2016

Uh oh!

sg- commented Oct 12, 2016

Uh oh!

mbed-bot commented Oct 12, 2016

Output

Uh oh!

pradeep-gr commented Oct 12, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

pradeep-gr Oct 3, 2016 •

edited

Loading

pradeep-gr commented Oct 12, 2016 •

edited

Loading