Skip to content

Conversation

@galak
Copy link
Contributor

@galak galak commented Sep 25, 2019

Update driver code and board files to use new GPIO configuration flags
such as GPIO_ACTIVE_LOW. Also add implementation of new port_* driver
API as well as gpio_pin_interrupt_configure function.

Signed-off-by: Kumar Gala [email protected]

@pabigot
Copy link
Contributor

pabigot commented Oct 1, 2019

AFAICT this platform exists only as a basis for qemu_cortex_m3, which says:

.. note::
   This board configuration makes no claims about its suitability for use
   with an actual ti_lm3s6965 hardware system, or any other hardware system.

Why are we keeping this?

Copy link
Member

@mnkp mnkp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The gpio_stellaris_configure function should also be updated.

@galak galak force-pushed the gpio-new-stellaris branch from 988014d to 1165a77 Compare October 17, 2019 16:24
@galak galak requested a review from pabigot as a code owner October 17, 2019 16:24
@galak galak requested a review from mnkp October 17, 2019 16:24
@mnkp
Copy link
Member

mnkp commented Oct 17, 2019

Seems like this PR was rebased but no changes to the driver code were applied.

@galak galak force-pushed the gpio-new-stellaris branch from 1165a77 to c671478 Compare October 17, 2019 17:48
@galak
Copy link
Contributor Author

galak commented Oct 17, 2019

Seems like this PR was rebased but no changes to the driver code were applied.

should be fixed now.

@mnkp
Copy link
Member

mnkp commented Oct 17, 2019

It seems like it's again the same version, no changes.

@galak
Copy link
Contributor Author

galak commented Oct 18, 2019

Line 194 should be changed. That was the only review comment that was made.

@mnkp
Copy link
Member

mnkp commented Oct 18, 2019

I've opened the PR in another browser, just in case, still the line 194 is written as

	sys_write32(GPIO_RW_MASK_ADDR(base, GPIO_DATA_OFFSET, value), mask);

which is the original. I believe it should be written as

	sys_write32(value, GPIO_RW_MASK_ADDR(base, GPIO_DATA_OFFSET, mask));

since

void sys_write32(u32_t data, mem_addr_t addr);

Apart from line 194 also gpio_stellaris_configure function should be updated.

@galak galak force-pushed the gpio-new-stellaris branch from c671478 to fbe2dc8 Compare October 18, 2019 21:34
@galak
Copy link
Contributor Author

galak commented Oct 18, 2019

I've opened the PR in another browser, just in case, still the line 194 is written as

	sys_write32(GPIO_RW_MASK_ADDR(base, GPIO_DATA_OFFSET, value), mask);

which is the original. I believe it should be written as

	sys_write32(value, GPIO_RW_MASK_ADDR(base, GPIO_DATA_OFFSET, mask));

since

void sys_write32(u32_t data, mem_addr_t addr);

Apart from line 194 also gpio_stellaris_configure function should be updated.

Gotcha, sorry bit a little while since I worked on the driver. Should now hopefully be fixed correctly.

@mnkp
Copy link
Member

mnkp commented Oct 18, 2019

It worked. However, we also need to update gpio_stellaris_configure function. It contains old interrupt configuration code and is using deprecated masks, like GPIO_DIR_IN.

@galak galak force-pushed the gpio-new-stellaris branch from fbe2dc8 to 67ba5de Compare October 21, 2019 14:10
@galak
Copy link
Contributor Author

galak commented Oct 21, 2019

It worked. However, we also need to update gpio_stellaris_configure function. It contains old interrupt configuration code and is using deprecated masks, like GPIO_DIR_IN.

Should now be updated.

@galak galak force-pushed the gpio-new-stellaris branch from 67ba5de to 8b3f4d9 Compare October 21, 2019 15:25
@galak galak force-pushed the gpio-new-stellaris branch from 8b3f4d9 to e02eccc Compare October 21, 2019 20:04
@galak galak requested a review from mnkp October 21, 2019 20:04
@zephyrbot
Copy link

zephyrbot commented Oct 21, 2019

All checks are passing now.

Tip: The bot edits this comment instead of posting a new one, so you can check the comment's history to see earlier messages.

@galak
Copy link
Contributor Author

galak commented Oct 22, 2019

Not sure I follow where in the code we should handle 'disconnected' mode.

Was thinking we'd have:

if (flags & INPUT)
...
else if (flags & OUTPUT)
...
else
/* disconnect */

@galak galak force-pushed the gpio-new-stellaris branch from e02eccc to 104c5c7 Compare October 22, 2019 15:03
Copy link
Member

@mnkp mnkp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was thinking we'd have:

if (flags & INPUT)
...
else if (flags & OUTPUT)
...
else
/* disconnect */

This version of the code looks good with the only remark that we should test first for GPIO_OUTPUT flag and then have } else if (flags & INPUT) {. If we swap the branches the code should also work if user passes both flags simultaneously (GPIO_OUTPUT | GPIO_INPUT). The pin will be configured as an output, but the input direction should also work as long as the pin is in push-pull mode.

@galak galak force-pushed the gpio-new-stellaris branch from 104c5c7 to 1ad401b Compare October 22, 2019 22:53
@galak galak requested a review from mnkp October 22, 2019 22:54
Update driver code and board files to use new GPIO configuration flags
such as GPIO_ACTIVE_LOW. Also add implementation of new port_* driver
API as well as gpio_pin_interrupt_configure function.

Signed-off-by: Kumar Gala <[email protected]>
@galak galak force-pushed the gpio-new-stellaris branch from ba7aa7d to f0e00e7 Compare October 25, 2019 23:10
@galak galak merged commit 41c0217 into zephyrproject-rtos:topic-gpio Oct 26, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants