Closed mtvec closed 1 year ago
Nice. Does this driver work nicely on FP3?
There was a firmware upload procedure that was never triggered on my device, I guess because the FW is stored in non-volatile memory. I'm not sure if devices are shipped with FW stored. If not, this might be a issue for people that never booted into Android;
afaik downstream does load the firmware and update it, but assuming you booted some recent-ish downstream (Android) at some point it's probably fine. It can come later if required.
The driver used fb_register_client to get notifications about framebuffer blanking events. I removed this because it was never triggered on my device and no other touchscreen drivers seem to use it;
Downstream addition, I think on Android more the kernel driver is responsible for powering off the touchscreen when the screen is off, which is a bit backwards but whatever. Shouldn't be needed on mainline.
There was some code that tries to detect if a USB cable is present (I guess the goal was to see if the device is charging) and if so, write some magic commands to the controller. I removed this since it's unclear what these commands were supposed to do;
I imagine it could help with ghost touches due to some electrical interference while charging that it doesn't need when it's not plugged in.
There was a function that "entered safe mode" at some point during probe. Again, I have no idea what this meant, and everything seems to work without it, so I removed this; Some other initialization configurations that don't seem to have any effect were removed. Rare comments talked about things like enabling "continuous burst mode", "AHB address auto +4", "RawOut", and "DSRAM func".
dunno
Also you need to make good and descriptive commits out of the current commits, see https://git-rebase.io/ and https://www.kernel.org/doc/html/latest/process/submitting-patches.html All your patches also need a signed-off-by line, as also described in the second link.
Nice. Does this driver work nicely on FP3?
I've tested single touch directly by interacting with applications in xfce and that seems to work fine. For multi touch, I've used evtest
and afaict, that also looks fine. Are there any better ways I could test it?
Also you need to make good and descriptive commits out of the current commits, see https://git-rebase.io/ and https://www.kernel.org/doc/html/latest/process/submitting-patches.html All your patches also need a signed-off-by line, as also described in the second link.
Thanks for the links! I will look into it and rebase asap. Who would need to be in the signed-off-by line?
I took a first shot a fixing-up the commits @z3ntu.
Edit: also fixed all checkpatch.pl
warnings (except the missing signed-off-by field).
You should be in the signed-off so Signed-off-by: Job Noorman <job@noorman.info>
I'll look in trying this driver on FP4 soon.
I would still like to have some feedback on the regulator stuff. Should I just add the default regulator enabling/disabling code that all touchscreen drivers have? I'm a bit hesitant about this since everything works without doing that so I don't really know how to test that code.
The original driver does use regulators. However, all that code was guarded by an unused config option called CONFIG_HMX_DB
which KConfig
describes as "This enables support for HIMAX driver test over Dragon Board". So it seems only used for some internal test setup.
I don't think you need explicit regulator handling. This two-in-one (display + touchscreen in one module) also doesn't make it easier to differentiate in devicetree, and it seems at least on FP3 and FP4 the power is on whenever the display is on at least, so I wouldn't worry about it here. If some other device needs it at some point it can be added later.
I really like how you made ~500 lines out of the downstream 18729 lines :joy: (wc -l drivers/input/touchscreen/hxchipset/*
on FP4)
Also not sure if "himax.c" is a good name as there's other touchscreens by himax that are not compatible. Checking downstream kconfig these chips use similar-enough protocol but probably use some adjustments:
So I'd recommend either hx83xxx.c or just hx83112.c which is common naming also, just because it's the first variant supported, and other variants can still be added later to the same file.
I really like how you made ~500 lines out of the downstream 18729 lines joy (
wc -l drivers/input/touchscreen/hxchipset/*
on FP4)
It was "only" 11k on the FP3 iirc but yeah, that felt good :)
So I'd recommend either hx83xxx.c or just hx83112.c which is common naming also, just because it's the first variant supported, and other variants can still be added later to the same file.
Good point! I was also wondering if we should add a "ts" suffix (so hx83112_ts.c
) to avoid conflicts with the panel driver.
I've just tried this on FP4. First failed to probe because I had reset polarity wrong (apparently downstream changed it at some point like I recommended you)
[ 0.278801] Himax-TS 0-0048: i2c write failed: -6
Now it probes
[ 0.302144] input: Himax Touchscreen as /devices/platform/i2c-muic/i2c-0/0-0048/input/input0
but I don't get any interrupts, so no touch events either. Maybe a problem on my side, not sure right now
/ # cat /proc/interrupts | grep "msmgpio 22"
117: 0 0 0 0 0 0 0 0 msmgpio 22 Edge hx83112a
This is the dts node I used with downstream btw:
himax_ts@48 {
compatible = "himax,hxcommon";
reg = <0x48>;
interrupt-parent = <&tlmm>;
interrupts = <22 0x2>;
pinctrl-names = "pmx_ts_active","pmx_ts_suspend",
"pmx_ts_release";
pinctrl-0 = <&ts_active &ts_rst_active>;
pinctrl-1 = <&ts_int_suspend &ts_reset_suspend>;
pinctrl-2 = <&pmx_ts_release>;
himax,panel-coords = <0 1080 0 2340>;
himax,display-coords = <0 1080 0 2340>;
himax,irq-gpio = <&tlmm 22 0x00>;
himax,rst-gpio = <&tlmm 21 0x00>;
report_type = <1>;
};
Nothing special, but different interrupt type, and report_type = <1> whatever that means
Nothing special, but different interrupt type
Could it then be caused by hardcoding the interrupt type in the driver? That should be fixed now.
and report_type = <1> whatever that means
This was called report_type
in the android driver and only used for debugging purposes afaict.
Thanks a lot for your extensive review! Most comment should be fixed now. The main thing I still have to do is look at regmap
.
I think there are two main issues still open:
hx83112.c
or hx83112_ts.c
to avoid conflicts with the panel driver? In any case, I suppose it's best to also change the himax_
prefix of all function/type names?hx83112a
controller. If we can't make that work, it's maybe best to just remove if from compatible
?Renaming: yes sounds good. I guess _ts suffix is also good if other touchscreen drivers also do something similar. That way we can really avoid ambiguity with the panel.
For FP4 here's the log from downstream driver:
And we do indeed get interrupts there as expected
/ # cat /proc/interrupts | grep "gpio 22"
117: 284 0 0 0 0 0 0 0 msmgpio 22 Edge hxcommon
I'll continue looking into it, I'd really like to have this working!
The reason I thought this driver might work for hx83112a
is that the downstream driver refers to it and, afaict, no (relevant) different choices were made in the driver based on which model was detected.
But I just took a look at the downstream FP4 driver and it looks quite different, at least in terms of code structure. So now I started wondering if the FP3 driver was ever supposed to work on FP4.
Could it maybe make sense to try and get the downstream FP3 driver working on FP4? If we can't get that to work, I guess there's no hope getting this one to work.
FP3 touchscreen driver on FP4 first attempt: doesn't work, no interrupts
This kinda looks like an interesting difference, right?
FP4 downstream:
[ 1.168183] [HXTP] himax_mcu_touch_information:HX_RX_NUM =36,HX_TX_NUM =16
[ 1.168190] [HXTP] himax_mcu_touch_information:HX_MAX_PT=10,HX_XY_REVERSE =0
[ 1.168195] [HXTP] himax_mcu_touch_information:HX_Y_RES=2340,HX_X_RES =1080
[ 1.168201] [HXTP] himax_mcu_touch_information:HX_INT_IS_EDGE =1,HX_PEN_FUNC = 0
[ 1.168207] [HXTP] himax_report_data_init:rawdata_fsz = 10,HX_MAX_PT:10,hx_raw_cnt_max:2
[ 1.168213] [HXTP] himax_report_data_init:hx_raw_cnt_rmd:2,g_hx_rawdata_size:66,touch_info_size:56
FP3 downstream driver on FP4:
[ 0.751403] [HXTP] himax_touch_information:HX_RX_NUM =30,HX_TX_NUM =18,HX_MAX_PT=10
[ 0.751410] [HXTP] himax_touch_information:HX_XY_REVERSE =1,HX_Y_RES =2160,HX_X_RES=1080
[ 0.751417] [HXTP] himax_touch_information:HX_INT_IS_EDGE =0
Do you think this might have any effect?
As far as I understood, HX_RX_NUM
and HX_TX_NUM
only had an impact on debugging code (e.g., the /proc
fs).
The difference in HX_INT_IS_EDGE
could be relevant though:
if(ic_data->HX_INT_IS_EDGE)
ret = request_threaded_irq(client->irq, NULL, himax_ts_thread,IRQF_TRIGGER_FALLING | IRQF_ONESHOT, client->name,
else
ret = request_threaded_irq(client->irq, NULL, himax_ts_thread,IRQF_TRIGGER_LOW | IRQF_ONESHOT, client->name, ts);
So you could try to set FIX_HX_INT_IS_EDGE
to true
on line 180 of himax_common.h
to make it correspond with the FP4 driver.
By the way, here's the log of the downstream driver on FP3
[ 1.254911] [HXTP] Enter Himax common touch panel driver init 83112-B [ 1.267152] [HXTP] Exit Himax common touch panel driver init 83112-B [ 1.410973] [HXTP] DT-himax_parse_dt:panel-coords = 0, 1080, 0, 2160 [ 1.413857] [HXTP] DT-himax_parse_dt:display-coords = (1080, 2160) [ 1.413885] [HXTP] DT:gpio_3v3_en value is not valid [ 1.426447] [HXTP] DT:gpio_irq=435, gpio_rst=434, gpio_3v3_en=-2 [ 1.426452] [HXTP] DT:protocol_type=1 [ 1.431639] [HXTP] DT-No vk info in DT [ 1.486209] [HXTP] himax_enter_safe_mode: Check enter_save_mode data[0]=C [ 1.518610] [HXTP] himax_ic_package_check:Read driver IC ID = 83,11,2B [ 1.536297] [HXTP] Himax IC package 83112_in [ 1.536300] [HXTP] Enter himax_sense_on [ 1.550005] [HXTP] himax_ic_reset,status: loadconfig=0,int_off=0 [ 1.555913] [HXTP] himax_pin_reset: Now reset the Touch chip. [ 1.589151] [HXTP] reload OK! [ 1.597402] [HXTP] himax_read_FW_ver : data[0]=0x4B,data[1]=0x91,data[2]=0x54,data[3]=0x47 [ 1.605056] [HXTP] reload_status=1 [ 1.642707] [HXTP] himax_sense_off: Check enter_save_mode data[0]=C [ 1.671209] [HXTP] PANEL_VER : 6 [ 1.671245] [HXTP] FW_VER : 209 [ 1.673559] [HXTP] IC_ID : 52 [ 1.679101] [HXTP] TOUCH_VER : 15 [ 1.679686] [HXTP] DISPLAY_VER : 2 [ 1.686510] [HXTP] CID_VER : 804 [ 1.749257] [HXTP] himax_hw_check_CRC: tmp_data[3]=0, tmp_data[2]=0, tmp_data[1]=0, tmp_data[0]=0 [ 1.807689] [HXTP] himax_loadSensorConfig: initialization complete [ 1.807731] [HXTP] himax_power_on_init: [ 1.812794] [HXTP] himax_touch_information:HX_RX_NUM =36,HX_TX_NUM =18,HX_MAX_PT=10 [ 1.816513] [HXTP] himax_touch_information:HX_XY_REVERSE =1,HX_Y_RES =2160,HX_X_RES=1080 [ 1.824542] [HXTP] himax_touch_information:HX_INT_IS_EDGE =0 [ 1.848275] [HXTP] Enter himax_sense_on [ 1.850582] [HXTP] himax_ic_reset,status: loadconfig=0,int_off=0 [ 1.851289] [HXTP] himax_pin_reset: Now reset the Touch chip. [ 1.883700] [HXTP] calcDataSize: coord_data_size: 40, area_data_size:12, raw_data_frame_size:67, raw_data_nframes:1 [ 1.883724] [HXTP] himax_chip_common_probe: calcDataSize complete [ 1.892978] [HXTP] himax_chip_common_probe: Use Protocol Type B [ 1.899230] [HXTP] input_set_abs_params: mix_x 0, max_x 1080, min_y 0, max_y 2160 [ 1.913435] [HXTP] himax_report_data_init: rawdata_frame_size = 11 [ 1.913440] [HXTP] himax_report_data_init: ic_data->HX_MAX_PT:10,hx_raw_cnt_max:2,hx_raw_cnt_rmd:2,g_hx_rawdata_size:67,hx_touch_data->touch_info_size:56 [ 1.925135] [HXTP] himax_int_register_trigger level trigger low [ 1.946502] [HXTP] himax_ts_register_interrupt: irq enabled at qpio: 94 [ 17.119413] [HXTP] himax_fb_register in [ 31.713140] [HXTP] [HX_HW_RESET_ACTIVATE]:himax_checksum_cal: Back from reset, ready to serve. [ 31.720563] [HXTP] [HIMAX TP MSG] checksum fail : check_sum_cal: 0x1B6 [ 31.731326] [HXTP] S1@721, 908 [ 32.182620] [HXTP] E1@256, 1580
I think I did select TRIGGER_FALLING on devicetree :shrug: But need to debug some more :)
I really like how you made ~500 lines out of the downstream 18729 lines 😂 (
wc -l drivers/input/touchscreen/hxchipset/*
on FP4)Also not sure if "himax.c" is a good name as there's other touchscreens by himax that are not compatible. Checking downstream kconfig these chips use similar-enough protocol but probably use some adjustments:
- HX852xH
- HX852xG
- HX83192
- HX83191
- HX83113
- HX83112
- HX83111
- HX83106
- HX83103
- HX83102
So I'd recommend either hx83xxx.c or just hx83112.c which is common naming also, just because it's the first variant supported, and other variants can still be added later to the same file.
Hello,
I am looking for a Linux driver for Himax HX83192.
But I couldn't come to a conclusion.
Could you please help with this issue?
Best regards.
@cebele Assuming you have a board with this screen, can you get a driver from your vendor? While this (rewritten) driver might work on HX83192 it's quite possible it needs some adjustments to work there.
@mtvec If you're happy with the driver, could you send an initial version upstream to the correct mailing lists? If you need any help with this, please let me know!
I tested myself on 5.18.3 kernel and it works really well! Functionally I don't have any complaints right now :)
@z3ntu Yes, I can submit it.
Did you manage to get it to work on the FP4 or did you test it on the FP3? Just to know if I should remove the FP4 touchscreen from the compatible
list.
Probably good to remove references to hx83112a for now, I didn't manage to get it to work yet. Also as I've written somewhere before I'd rename the driver from just "himax" (https://github.com/msm8953-mainline/linux/pull/43#issuecomment-1190314605)
So I'd recommend either hx83xxx.c or just hx83112.c which is common naming also, just because it's the first variant supported, and other variants can still be added later to the same file.
Also rename kconfig symbol
@mtvec ?
Sorry @z3ntu but I've been a bit overwhelmed with work lately. I'll try to take a look at this this week.
@z3ntu I removed all references to hx8311a and renamed the files so I think I'm ready to send it upstream. Some quick questions:
Reviewed-by
tag. Is this ok?linux-input
but I was wondering where I should send the others. If I should send those to other lists, do I make one patch set and send it to all lists or should I make separate patch sets?Another question: afaict, not enough patches from this repository have been upstreamed to have a bootable kernel on FP3. So I wonder how I'm supposed to test this driver on the linux-input tree. It seems wrong to propose patches to a tree on which they cannot be tested. Am I missing something?
I added you in a Reviewed-by tag. Is this ok?
Not really... I'm not really a qualified reviewer for input patches, people on the list will do it for you.
I can give you a Tested-by
as email reply though it the sent version works well for me :wink:
I will send the driver patch to linux-input but I was wondering where I should send the others. If I should send those to other lists, do I make one patch set and send it to all lists or should I make separate patch sets?
https://www.kernel.org/doc/html/latest/process/submitting-patches.html - in short, use ./scripts/get_maintainers.pl
for getting list of recipients
Another question: afaict, not enough patches from this repository have been upstreamed to have a bootable kernel on FP3.
Pure mainline should boot on FP3, although without many features supported here incl. display.
So I wonder how I'm supposed to test this driver on the linux-input tree. It seems wrong to propose patches to a tree on which they cannot be tested. Am I missing something?
If you base it on linux-next/master or torvalds/master it also in general shouldn't be a problem btw. But even if it needed some extra dts patches here (or more) it shouldn't be a problem because the driver itself works correctly.
I cherry picked the commits on 6.0-wip for #53
We can close this, its already in the 6.0.10 branch.
This is a simple touchscreen driver for the Himax hx83112b controller used in the FP3. I suspect it will also work for the hx83112a controller (used in the FP4?) but I have not been able to test this.
There are no datasheets available for these devices, everything is based on the Android driver which had been initially ported to mainline by @z3ntu.
My approach was to gradually chip away at the (huge) Android driver, removing everything that seemed unnecessary. Some things were obvious (e.g., a
/proc
interface for debugging) but for other things, I could use some feedback:fb_register_client
to get notifications about framebuffer blanking events. I removed this because it was never triggered on my device and no other touchscreen drivers seem to use it;probe
. Again, I have no idea what this meant, and everything seems to work without it, so I removed this;Another thing I'm not quite sure about is how to handle regulators. The original DTS defined two, and all touchscreen drivers seem to enable some, but everything works without defining any. Since regulators can be shared between multiple devices, they might have already been enabled by another device. So should this driver explicitly manage them? If so, how can I find out which ones are actually needed?