-
Notifications
You must be signed in to change notification settings - Fork 8k
drivers: input: Add input driver support on RSK-RX130 #96559
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
base: main
Are you sure you want to change the base?
drivers: input: Add input driver support on RSK-RX130 #96559
Conversation
The following west manifest projects have changed revision in this Pull Request:
✅ All manifest checks OK Note: This message is automatically posted and updated by the Manifest GitHub Action. |
d4c5731
to
4dd7324
Compare
4dd7324
to
04a6047
Compare
04a6047
to
7fccb78
Compare
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.
LGTM
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.
Look good to me, too.
} | ||
|
||
if (data->work_phase == SCANNING) { | ||
if (p_arg->event == CTSU_EVENT_SCAN_COMPLETE) { |
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.
swap the argument, unindent the normal case and return on this one
if (p_arg->event != CTSU_EVENT_SCAN_COMPLETE) {
return;
}
k_work_submit(&data->data_process_work);
You could probably also merge the two conditions? return if `data->work_phase == SCANNING && p_arg->event ~= CTSU_EVENT_SCAN_COMPLETE`
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's done, thank you
CONTAINER_OF(work, struct renesas_rx_ctsu_data, data_process_work); | ||
const struct device *dev = data->dev; | ||
const struct renesas_rx_ctsu_config *config = dev->config; | ||
fsp_err_t ret = FSP_SUCCESS; |
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.
drop the initializer, you literally set this in the next line
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.
Dropped it
int button_position = 0; | ||
|
||
while (changed_buttons != 0) { | ||
if (changed_buttons & 0x1) { |
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.
BIT(0)
instead of 0x01? up to you, no strong feelings
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.
yah, I have updated, thanks
int index = config->button_position_index[button_position]; | ||
|
||
input_report_key(dev, config->buttons[index].event, | ||
data->curr_buttons_data & (1 << button_position), true, |
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.
BIT(button_position)
instead of 1 << button_position
} | ||
} | ||
|
||
void timer_callback(struct k_timer *timer) |
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.
static
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.
thanks, I have updated
} | ||
} | ||
|
||
void button2_task(void) |
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.
static
#endif /* CONFIG_INPUT_RENESAS_RX_QE_TOUCH_CFG */ | ||
} | ||
|
||
void button1_task(void) |
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.
static
#define DEBOUNCE_TIMES 3 | ||
#define DEBOUNCE_INTERVAL_MS (500 / DEBOUNCE_TIMES) |
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.
unused? drop
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.
thanks, dropped it
leds_on_off[(current_main_led + i) % 4] = | ||
((led_state >> i) & 0x1) ? LED_ON : LED_OFF; |
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.
why not just have a
if (led_state & BIT(i)) {
gpio_pin_set_dt(...)
} else {
gpio_pin_set_dt(...)
}
current logic seems unnecessarily complicated, two cycles, the global variable...
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.
Updated the code, thanks
#define QE_TOUCH_DEFINE_H | ||
|
||
/* Macro definitions */ | ||
#define QE_TOUCH_VERSION (0x0420) |
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.
drop the parenthesis from the literals here, they are only needed on expressions, no point adding them everywhere
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.
dropped the parenthesis
hey, few coding style things, sorry for the delay reviewing this, ooo at the moment |
013669e
7fccb78
to
013669e
Compare
k_sem_give(&data->tune_scan_end); | ||
} | ||
|
||
if (data->work_phase == SCANNING) { |
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.
can you swap this and make it an early return
as well? May even do a single if
with the one below
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.
I updated this, could you please take a look?
#ifndef CONFIG_INPUT_RENESAS_RX_QE_TOUCH_CFG | ||
#define CTSU_HAL_CONFIG_DEFINE(idx) \ | ||
.ctsu_cfg = \ | ||
{ \ |
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.
nit: bring this to the line above? that's the normal formatting, I usually let it go on macros because I know clang-format does not respect that but you have it right on the field below so I presume you are manually indenting, may as well
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.
Actually, I did try moving it to the line above as in the field below, but clang-format automatically reformatted it to the current layout
Add support for Capatitive Touch Sensing Unit driver for RX MCUs Signed-off-by: Minh Tang <[email protected]>
Add support CTSU device tree node for Renesas RX130 Signed-off-by: Minh Tang <[email protected]>
Add Pinctrl, status okay for CTSU node on RSK-RX130 Signed-off-by: Minh Tang <[email protected]>
Add sample for CTSU input driver using QE config files on RSK-RX130-512KB Signed-off-by: Minh Tang <[email protected]>
013669e
to
da088e5
Compare
|
@quytranpzz hey, sorry this needs a rebase now |
Add Input subsys support on Renesas RX CTSU HWIP