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

drivers: sensor: Added support for htu21d sensor #3

Closed
wants to merge 4 commits into from

Conversation

arun-mani-tech
Copy link

Added driver support for htu21d sensor

Signed-off-by: Arunmani Alagarsamy arunmani@linumiz.com

Copy link
Member

@parthitce parthitce left a comment

Choose a reason for hiding this comment

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

Few tech stuff and various alignment + nit picks all around.

drivers/sensor/htu21d/Kconfig Outdated Show resolved Hide resolved
drivers/sensor/htu21d/htu21d.c Outdated Show resolved Hide resolved
drivers/sensor/htu21d/htu21d.c Outdated Show resolved Hide resolved
drivers/sensor/htu21d/htu21d.c Outdated Show resolved Hide resolved
drivers/sensor/htu21d/htu21d.c Outdated Show resolved Hide resolved
drivers/sensor/htu21d/htu21d.c Outdated Show resolved Hide resolved
drivers/sensor/htu21d/htu21d.c Outdated Show resolved Hide resolved
drivers/sensor/htu21d/htu21d.c Outdated Show resolved Hide resolved
drivers/sensor/htu21d/htu21d.h Outdated Show resolved Hide resolved
samples/sensor/htu21d/src/main.c Outdated Show resolved Hide resolved
Copy link

@ssekar15 ssekar15 left a comment

Choose a reason for hiding this comment

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

please check

drivers/sensor/htu21d/htu21d.c Outdated Show resolved Hide resolved
drivers/sensor/htu21d/htu21d.c Show resolved Hide resolved
drivers/sensor/htu21d/htu21d.c Outdated Show resolved Hide resolved
drivers/sensor/htu21d/htu21d.c Outdated Show resolved Hide resolved
drivers/sensor/htu21d/htu21d.c Outdated Show resolved Hide resolved
drivers/sensor/htu21d/htu21d.c Outdated Show resolved Hide resolved
drivers/sensor/htu21d/htu21d.c Outdated Show resolved Hide resolved
drivers/sensor/htu21d/htu21d.c Outdated Show resolved Hide resolved
drivers/sensor/htu21d/htu21d.c Outdated Show resolved Hide resolved
drivers/sensor/htu21d/Kconfig Show resolved Hide resolved
drivers/sensor/htu21d/htu21d.c Outdated Show resolved Hide resolved
drivers/sensor/htu21d/htu21d.c Outdated Show resolved Hide resolved
drivers/sensor/htu21d/htu21d.c Outdated Show resolved Hide resolved
drivers/sensor/htu21d/htu21d.c Outdated Show resolved Hide resolved
drivers/sensor/htu21d/htu21d.c Outdated Show resolved Hide resolved
drivers/sensor/htu21d/htu21d.c Outdated Show resolved Hide resolved
drivers/sensor/htu21d/htu21d.c Outdated Show resolved Hide resolved
drivers/sensor/htu21d/htu21d.c Outdated Show resolved Hide resolved
@parthitce
Copy link
Member

@arun-mani-tech https://github.com/linumiz/zephyr/tree/htu21d_pr is synced with main now. You have to rebase your branch https://github.com/linumiz/zephyr/tree/htu21d with htu21d_pr to keep only your changes.

Also, have you already fixed all the remarks and commects?

Copy link

@ssekar15 ssekar15 left a comment

Choose a reason for hiding this comment

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

please fix the review comments..

drivers/sensor/htu21d/Kconfig Show resolved Hide resolved
drivers/sensor/htu21d/htu21d.c Outdated Show resolved Hide resolved
drivers/sensor/htu21d/htu21d.c Outdated Show resolved Hide resolved
drivers/sensor/htu21d/htu21d.c Outdated Show resolved Hide resolved
drivers/sensor/htu21d/htu21d.c Show resolved Hide resolved
drivers/sensor/htu21d/htu21d.c Outdated Show resolved Hide resolved
drivers/sensor/htu21d/htu21d.c Outdated Show resolved Hide resolved
drivers/sensor/htu21d/htu21d.c Outdated Show resolved Hide resolved
drivers/sensor/htu21d/htu21d.h Outdated Show resolved Hide resolved
samples/sensor/htu21d/src/main.c Outdated Show resolved Hide resolved
@ssekar15
Copy link

run checkpatch and solve the compiler warnings

Arunmani Alagarsamy added 4 commits October 18, 2022 11:25
Add properties for htu21d driver

Signed-off-by: Arunmani Alagarsamy <arunmani@linumiz.com>
Add driver support for htu21d sensor

Signed-off-by: Arunmani Alagarsamy <arunmani@linumiz.com>
Add sample application for htu21d driver

Signed-off-by: Arunmani Alagarsamy <arunmani@linumiz.com>
Update CODEOWNERS for HTU21D

Signed-off-by: Arunmani Alagarsamy <arunmani@linumiz.com>
@ssekar15
Copy link

ACK, please create upstream PR

DineshDK03 pushed a commit that referenced this pull request Dec 8, 2022
This patch reworks how fragments are handled in the net_buf
infrastructure.

In particular, it removes the union around the node and frags members in
the main net_buf structure. This is done so that both can be used at the
same time, at a cost of 4 bytes per net_buf instance.
This implies that the layout of net_buf instances changes whenever being
inserted into a queue (fifo or lifo) or a linked list (slist).

Until now, this is what happened when enqueueing a net_buf with frags in
a queue or linked list:

1.1 Before enqueueing:

 +--------+      +--------+      +--------+
 |#1  node|\     |#2  node|\     |#3  node|\
 |        | \    |        | \    |        | \
 | frags  |------| frags  |------| frags  |------NULL
 +--------+      +--------+      +--------+

net_buf #1 has 2 fragments, net_bufs #2 and #3. Both the node and frags
pointers (they are the same, since they are unioned) point to the next
fragment.

1.2 After enqueueing:

 +--------+      +--------+      +--------+      +--------+      +--------+
 |q/slist |------|#1  node|------|#2  node|------|#3  node|------|q/slist |
 |node    |      | *flag  | /    | *flag  | /    |        | /    |node    |
 |        |      | frags  |/     | frags  |/     | frags  |/     |        |
 +--------+      +--------+      +--------+      +--------+      +--------+

When enqueing a net_buf (in this case #1) that contains fragments, the
current net_buf implementation actually enqueues all the fragments (in
this case #2 and #3) as actual queue/slist items, since node and frags
are one and the same in memory. This makes the enqueuing operation
expensive and it makes it impossible to atomically dequeue. The `*flag`
notation here means that the `flags` member has been set to
`NET_BUF_FRAGS` in order to be able to reconstruct the frags pointers
when dequeuing.

After this patch, the layout changes considerably:

2.1 Before enqueueing:

 +--------+       +--------+       +--------+
 |#1  node|--NULL |#2  node|--NULL |#3  node|--NULL
 |        |       |        |       |        |
 | frags  |-------| frags  |-------| frags  |------NULL
 +--------+       +--------+       +--------+

This is very similar to 1.1, except that now node and frags are
different pointers, so node is just set to NULL.

2.2 After enqueueing:

 +--------+       +--------+       +--------+
 |q/slist |-------|#1  node|-------|q/slist |
 |node    |       |        |       |node    |
 |        |       | frags  |       |        |
 +--------+       +--------+       +--------+
                      |            +--------+       +--------+
                      |            |#2  node|--NULL |#3  node|--NULL
                      |            |        |       |        |
                      +------------| frags  |-------| frags  |------NULL
                                   +--------+       +--------+

When enqueuing net_buf #1, now we only enqueue that very item, instead
of enqueing the frags as well, since now node and frags are separate
pointers. This simplifies the operation and makes it atomic.

Resolves zephyrproject-rtos#52718.

Signed-off-by: Carles Cufi <carles.cufi@nordicsemi.no>
@parthitce
Copy link
Member

Merged with different PR in mainline zephyr. Closing it.

@parthitce parthitce closed this Jan 4, 2023
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.

None yet

3 participants