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

board: Google Twinkie V2 #54286

Merged

Conversation

ualbertagreen
Copy link
Contributor

@ualbertagreen ualbertagreen commented Jan 31, 2023

This is a new board for the google Twinkie V2 tool.

Signed-off-by: Jason Yuan [email protected]

@@ -0,0 +1,195 @@
/*
* Copyright 2022 The Chromium OS Authors
Copy link
Collaborator

Choose a reason for hiding this comment

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

Copyright 2023 The ChromiumOS Authors - notice the year and no space in "ChromiumOS"

(Also elsewhere in this commit)

# Enable MPU
CONFIG_ARM_MPU=y

#CONFIG_USB_DEVICE_VID=18d1
Copy link
Collaborator

Choose a reason for hiding this comment

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

why is this commented out?

@@ -0,0 +1,22 @@
# Private config options for eSPI sample app

# Copyright (c) 2019 Intel Corporation
Copy link
Collaborator

Choose a reason for hiding this comment

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

Intel?

/* Constants matching USB3 and USB PD definitions */
#define CRC32_INITIAL 0xFFFFFFFF

/* Pre-computed values for polynom 0x04C11DB7 */
Copy link
Collaborator

Choose a reason for hiding this comment

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

How can one re-compute these values?

}

return crc;
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Zephyr already provides a CRC32 implementation: https://docs.zephyrproject.org/apidoc/latest/group__crc.html

Why does this need duplicated?

meas_vbus_c(&out);
shell_print(shell, "current of vbus: %i", out);
}
else {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Take a look at the style guide ... else must be cuddled. clang-format can fix this automatically for you.


smf_set_initial(SMF_CTX(&view_obj), &view_states[SNOOP0]);

while(1) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

see the style guide, space is always required after while.

clang-format can fix this automatically for you.

samples/boards/google_pda/src/view.c Outdated Show resolved Hide resolved
ret = gpio_pin_configure_dt(&led2, GPIO_OUTPUT_ACTIVE);
if (ret < 0) {
return ret;
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

same here, this should be a loop

@fabiobaltieri
Copy link
Member

fabiobaltieri commented Feb 1, 2023

Hey @ualbertagreen, I don't really think that the application code belong to the sample/ directory here in the upstream repo. That's really meant for code demonstrating a specific functionality or API. The board should be fine though, how about you keep this PR for adding the board and move the application in a separate repository under the Google org? There's an example on how to setup a standalone application:

https://github.com/zephyrproject-rtos/example-application/

also check out https://zmk.dev/docs/development/setup, their setup has shallow clone and module filtering which you may find useful: https://github.com/zmkfirmware/zmk/blob/main/app/west.yml

@ualbertagreen ualbertagreen force-pushed the twinkie_v2-no_share_isr-2 branch 2 times, most recently from 662c73b to 7f98968 Compare February 1, 2023 00:33
@ualbertagreen ualbertagreen changed the title Firmware for Google Twinkie V2. board: Google Twinkie V2 Feb 1, 2023
@ualbertagreen ualbertagreen force-pushed the twinkie_v2-no_share_isr-2 branch 5 times, most recently from 6c0ebfd to 726b291 Compare February 1, 2023 21:21
boards/arm/google_twinkie_v2/Kconfig.board Outdated Show resolved Hide resolved
boards/arm/google_twinkie_v2/Kconfig.defconfig Outdated Show resolved Hide resolved
boards/arm/google_twinkie_v2/Kconfig.board Outdated Show resolved Hide resolved
boards/arm/google_twinkie_v2/board.cmake Outdated Show resolved Hide resolved
boards/arm/google_twinkie_v2/google_twinkie_v2.dts Outdated Show resolved Hide resolved
boards/arm/google_twinkie_v2/google_twinkie_v2.dts Outdated Show resolved Hide resolved
boards/arm/google_twinkie_v2/google_twinkie_v2.dts Outdated Show resolved Hide resolved
boards/arm/google_twinkie_v2/google_twinkie_v2.dts Outdated Show resolved Hide resolved
boards/arm/google_twinkie_v2/google_twinkie_v2.dts Outdated Show resolved Hide resolved
boards/arm/google_twinkie_v2/google_twinkie_v2.yaml Outdated Show resolved Hide resolved
@ualbertagreen ualbertagreen force-pushed the twinkie_v2-no_share_isr-2 branch 2 times, most recently from 17da79c to d31b74e Compare February 7, 2023 23:01
@zephyrbot zephyrbot added the area: Devicetree Binding PR modifies or adds a Device Tree binding label Feb 7, 2023
@ualbertagreen ualbertagreen force-pushed the twinkie_v2-no_share_isr-2 branch 2 times, most recently from 5907627 to d1dec09 Compare February 8, 2023 00:14
@ualbertagreen ualbertagreen force-pushed the twinkie_v2-no_share_isr-2 branch 3 times, most recently from 71119ac to 43f7b4a Compare February 23, 2023 03:44
Copy link
Member

@fabiobaltieri fabiobaltieri left a comment

Choose a reason for hiding this comment

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

Well I guess we are waiting for #54634 for the shut commit, should defo not be here.

In the meantime, the board should get some basic documentation as well, just realized it's missing, could you add some? (look for other boards as reference)

@ualbertagreen
Copy link
Contributor Author

Well I guess we are waiting for #54634 for the shut commit, should defo not be here.

In the meantime, the board should get some basic documentation as well, just realized it's missing, could you add some? (look for other boards as reference)

@fabiobaltieri @galak I've moved #54634 in here as a stacked PR to avoid potential confusion. They are still kept as separate commits with separate commit messages so everything's compartmentalized.

I'll also add another commit stacked on top of this PR with a small app in samples/ that simply reads the voltage/current sensors and toggles the LED to show an example of how the sensor binding and board devicetree files are to be used. That way we can keep the full Twinkie app as a separate repository. Does that sound good with everyone?

@ualbertagreen ualbertagreen force-pushed the twinkie_v2-no_share_isr-2 branch 3 times, most recently from 9557d88 to 65cf6e8 Compare June 27, 2023 21:07
@ualbertagreen
Copy link
Contributor Author

Hi @fabiobaltieri @galak @jackrosenthal, the sensor PR has finally been merged. Could you take a look again at this PR. Just a reminder since it's been a while, this is the PR for the Twinkie V2 board and a sample code demonstrating its ability to snoop the usb-c lines. Thanks.

Copy link
Member

@fabiobaltieri fabiobaltieri left a comment

Choose a reason for hiding this comment

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

Thanks for your patience, just few nitpicks, mostly cosmetic.

boards/arm/google_twinkie_v2/board.cmake Show resolved Hide resolved
boards/arm/google_twinkie_v2/doc/index.rst Outdated Show resolved Hide resolved
boards/arm/google_twinkie_v2/google_twinkie_v2.dts Outdated Show resolved Hide resolved
boards/arm/google_twinkie_v2/google_twinkie_v2.dts Outdated Show resolved Hide resolved
samples/boards/google_twinkie_v2_pda/src/view.c Outdated Show resolved Hide resolved
@fabiobaltieri fabiobaltieri dismissed galak’s stale review July 3, 2023 10:42

Dropping Kumar blocker since the DT changes are gone.

@ualbertagreen ualbertagreen force-pushed the twinkie_v2-no_share_isr-2 branch 2 times, most recently from fb10b29 to 0d74da1 Compare July 6, 2023 23:04
@fabiobaltieri
Copy link
Member

Heya, I think you accidentally pushed some changes of the first commit into the second one.

@ualbertagreen ualbertagreen force-pushed the twinkie_v2-no_share_isr-2 branch 2 times, most recently from 1a85ad1 to de15ad6 Compare July 7, 2023 20:41
@ualbertagreen
Copy link
Contributor Author

Heya, I think you accidentally pushed some changes of the first commit into the second one.

Sorry about that, it should be fixed now.

@fabiobaltieri
Copy link
Member

Heya, I think you accidentally pushed some changes of the first commit into the second one.

Sorry about that, it should be fixed now.

Yup no prob, happens to me all the time as well. There's still a couple of unresolved comments, all nitpicky, doule blank lines, odd property ordering etc, could you have a look at those? Happy with it after those.

fabiobaltieri
fabiobaltieri previously approved these changes Jul 10, 2023
jackrosenthal
jackrosenthal previously approved these changes Jul 18, 2023
boards/arm/google_twinkie_v2/google_twinkie_v2.yaml Outdated Show resolved Hide resolved
This is a new board for the google Twinkie V2 tool.

Signed-off-by: Jason Yuan <[email protected]>
sample firmware for Twinkie V2 that toggles the LED based on charging
status.

Signed-off-by: Jason Yuan <[email protected]>
@carlescufi carlescufi merged commit ad555d0 into zephyrproject-rtos:main Jul 25, 2023
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: ADC Analog-to-Digital Converter (ADC) area: Devicetree Binding PR modifies or adds a Device Tree binding
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants