Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add common hardware alocator for nRF5x TWI and SPI instances #4491

Closed
wants to merge 4 commits into from

Conversation

nvlsianpu
Copy link
Contributor

Description

Add Support for sharing resources between SPI and TWI instancess.
This fix the issue #4357: NRF52 doesn't support simultaneous use of I2C and SPI.
Now this resources are reserved by each of hal driver using shared_hw module.

Status

READY

@nvlsianpu
Copy link
Contributor Author

cc @pan-, @anangl

@0xc0170
Copy link
Contributor

0xc0170 commented Jun 12, 2017

Please resolve the conflict

@pan- @anangl Please review

Copy link
Contributor

@0xc0170 0xc0170 left a comment

Choose a reason for hiding this comment

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

just minor style comments

@@ -87,6 +89,11 @@ typedef struct {
nrf_drv_spis_t slave;
} sdk_driver_instances_t;

typedef struct {
uint8_t hw_idx;
Copy link
Contributor

Choose a reason for hiding this comment

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

2 spaces here?

@@ -219,6 +239,18 @@ static nrf_drv_spis_event_handler_t const m_slave_event_handlers[SPIS_COUNT] = {
#endif
};

static sdk_driver_instances_t const * get_driver_pattern(uint8_t hw_idx) {
Copy link
Contributor

Choose a reason for hiding this comment

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

new line {

int8_t idx = instance_hw_idx_get(HW_RESOURCE_SPI);

if (idx < 0)
{
Copy link
Contributor

Choose a reason for hiding this comment

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

if () { style fix

@theotherjimmy
Copy link
Contributor

@0xc0170 Could you rereview?

Copy link
Contributor

@0xc0170 0xc0170 left a comment

Choose a reason for hiding this comment

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

How was this tested?

Intention is to have different policies for peripheral types with shares the same resources.
Thanks to that, maximum numbers of peripheral instances will be available.
*/
static const uint8_t m_serch_policy[] = {SERCH_ASCCENDING, SERCH_DESCCENDING};
Copy link
Contributor

Choose a reason for hiding this comment

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

typo - ascending /descending?

static uint8_t m_reservation = 0;


int8_t instance_hw_idx_get(hw_resource_type_t peripheral_type) {
Copy link
Contributor

Choose a reason for hiding this comment

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

{ newline

{
MBED_ASSERT(idx < SHARED_INSTANCES_CNT);

m_reservation &= (1 << idx);
Copy link
Contributor

Choose a reason for hiding this comment

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

shouldnt you negate the number to return it to 0 ? As the above is setting it to 0?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes sure!

@0xc0170
Copy link
Contributor

0xc0170 commented Jun 21, 2017

@pan- want to review this one?

@NeilMacMullen
Copy link
Contributor

Please see my comments at #4357 This change appears to be a serious regression. Even when assigning a single I2C bus (no SPI), "i2c->write(address, NULL, 0)" is returning 0 for any value of address.

@MarceloSalazar
Copy link

@nvlsianpu could you please let us know whether you are able to reproduce the problems?
This is quite urgent and we'd appreciate a prompt fix. We are happy to help with testing. Thanks.

@nvlsianpu
Copy link
Contributor Author

As this has serious bug - after discussion we provide fix in quite different way.

@nvlsianpu nvlsianpu closed this Jun 23, 2017
@sg- sg- removed the needs: work label Jun 23, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants