-
Notifications
You must be signed in to change notification settings - Fork 6.8k
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
ev11l78a: add en_sink as a custom binding. #61740
Conversation
boards/arm/ev11l78a/ev11l78a.dts
Outdated
|
||
// The following are not LEDs on the board, but rather | ||
// testpoints. On the other hand, they are "fit the part." | ||
ind_1_5a: led_1 { | ||
gpios = <&porta 1 GPIO_ACTIVE_HIGH>; | ||
label = "1.5A_IND"; | ||
}; | ||
|
||
ind_3_0a: led_2 { | ||
gpios = <&porta 15 GPIO_ACTIVE_HIGH>; | ||
label = "3.0A_IND"; | ||
}; | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if not LEDs, why under gpio-leds
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seemed the easiest way to expose these with an actual label — but I'm not convinced myself. What other options are there?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
likely https://github.com/zephyrproject-rtos/zephyr/tree/main/samples/basic/custom_dts_binding, but if not used upstream, you should likely keep these downstream.
enable_sink: enable_sink { | ||
compatible = "microchip,ev11l78a-ensink"; | ||
gpios = <&porta 22 GPIO_ACTIVE_HIGH>; | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks like a regulator-fixed
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Almost but not quite? It's technically not fixed, because it's controlled by the TCPC, but as far as I can tell there's no precedent for a TCPC controlled regulator.
But I guess it would work by not passing min/max microvolt, hmm.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so if controlled by TCPC it should be a property there, something like:
tcpc {
enable-sink-gpios = <...>;
};
then TCPC code would use the GPIO API. Or, if using regulators:
enable_sink: enable-sink {
compatible = "regulator-fixed";
enable-gpios = <...>;
};
...
tcpc {
...
sink-supply = <&enable_sink>;
};
Then TCPC would use the regulators API (main advantage is that it is reference counted, useful if supply is shared).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So this is a bit more complicated — the voltage is controlled by a TCPC (for which I'm writing the driver, slowly), but the output of the board is controlled by a MOSFET controlled by this GPIO.
So maybe a fixed regulator would be the best fit? While the indicators could be ignored (although I really would have preferred labeling them, they are part of the board after all, even if they're just test points) I believe this GPIO needs to be exposed as it is clearly part of the eval part of the board.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It depends on how it is used, if by multiple clients then regulator is a good fit, is by a single client, a GPIO does the job. A typical case for GPIO usage is e.g. on sensors that have a shutdown pin. I don't know this case in detail to suggest one way or the other.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To be honest, this is almost the literal case from the documentation you linked earlier as it's a GPIO connected to a specific MOSFET…
Leaving it as a generic GPIO doesn't seem right though — it's most certainly no longer "General-purpose" once it's wired to the MOSFET on the board, and it has a very specific function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let me move the important change (memory and flash size) to #61744 that is more generic, and I'll follow up here about the regulator/sink enable option.
boards/arm/ev11l78a/ev11l78a.dts
Outdated
|
||
// The following are not LEDs on the board, but rather | ||
// testpoints. On the other hand, they are "fit the part." | ||
ind_1_5a: led_1 { | ||
gpios = <&porta 1 GPIO_ACTIVE_HIGH>; | ||
label = "1.5A_IND"; | ||
}; | ||
|
||
ind_3_0a: led_2 { | ||
gpios = <&porta 15 GPIO_ACTIVE_HIGH>; | ||
label = "3.0A_IND"; | ||
}; | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seemed the easiest way to expose these with an actual label — but I'm not convinced myself. What other options are there?
enable_sink: enable_sink { | ||
compatible = "microchip,ev11l78a-ensink"; | ||
gpios = <&porta 22 GPIO_ACTIVE_HIGH>; | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Almost but not quite? It's technically not fixed, because it's controlled by the TCPC, but as far as I can tell there's no precedent for a TCPC controlled regulator.
But I guess it would work by not passing min/max microvolt, hmm.
beb5bcf
to
1c92c40
Compare
You need rebase : ) |
Ping |
The ATSAMD20E16 used in this board only has 8KiB SRAM, so the defaults are not really suitable for anything close to useful, particularly the sink sample that most closely resembles the original demo for the board. This should also allow more tests not to be skipped for this board as otherwise they overflow RAM usage. Signed-off-by: Diego Elio Pettenò <[email protected]>
This configures the DAC in the same way as it is configured for the arduino_zero and other similar boards. It also includes the board in the DAC tests. Signed-off-by: Diego Elio Pettenò <[email protected]>
This exposes the EN_SINK signal as a custom binding, since it is ore to the business logic of the board. EN_SINK is a GPIO line connected to a MOSFET that provides power on the screw terminal of the board, and should be enabled after USB-PD negotiation. Signed-off-by: Diego Elio Pettenò <[email protected]>
Hi @Flameeyes , Is there anything pending from your side on this? |
This pull request has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this pull request will automatically be closed in 14 days. Note, that you can always re-open a closed pull request at any time. |
ev11l78a: add en_sink as a custom binding.
This exposes the EN_SINK signal as a custom binding, since it is ore to
the business logic of the board.
EN_SINK is a GPIO line connected to a MOSFET that provides power on the
screw terminal of the board, and should be enabled after USB-PD
negotiation.
Signed-off-by: Diego Elio Pettenò [email protected]
Stack created with Sapling. Best reviewed with ReviewStack.