On Tue, Sep 19, 2023 at 7:09 PM Conor Dooley [off-list ref] wrote:

On Tue, Sep 19, 2023 at 05:31:29PM +0800, yang tylor wrote:

quoted

Hi Conor,

quoted

quoted

Additional optional arguments:
ic-det-delay-ms and ic-resume-delay-ms are using to solve runtime
conditions.

quoted

Runtime conditions? Aren't thєse properties of the panel & therefore
fixed? If they were runtime conditions, then setting them statically in
your DT is not going to work, right?

Because each platform's display driver ready time is different. TP part
need to avoid this timing by measuring the waveform of LCD reset pin
low period and TP probe timing. For example, if LCD rst pin low from
timestamp 100 to 800, TP driver probe at 600. TP probe will fail. Then
user should set ic-det-delay-ms bigger than 200, to avoid LCD rst low
timing. As you can see, the timing needs to be measured at runtime to
decide how long it should be. Then, if the condition is not changed, the
value could keep the same.

That sounds to me like something you would test once for a given
platform and then the values are static. If you are actually changing it
at *runtime*, how is doing it through DT suitable? Does your firmware do
the tests & then set the values in DT dynamically?

Yes, you are right. I'll change the description.

quoted

quoted

It looks like you deleted all of the properties from the previous
submission of these changes. I don't really understand that, it kinda
feels just like appeasement, as you must have needed those properties
to do the firmware loading etc. How are you filling the gap those
properties have left, when you still only have a single compatible
string in thㄟs binding? Is there a way to do runtime detection of which
chip you're dealing with that you are now using?

After reviewing, I found the properties could go to IC driver settings :
"himax,heatmap_16bits" because it depends on IC's ability;

How do you detect the IC's abilities?

The driver code has a part of IC detect process, and each IC has its own
driver code to define its abilities. This part moves to that position.

quoted

Some
could remove and use default values: "himax,fw_size",
"himax,boot_time_fw_upgrade". "himax,fw_size" has a default value in
IC settings, and likely won't change in this IC.

Okay.

quoted

The behavior of "himax,boot_time_fw_upgrade" seems not stable and
should be removed. "himax,fw_in_flash", I use the kernel config for
user to select.

That seems like a bad idea, we want to be able to build one kernel that
works for all hardware at the same time.

I see, so I should take that back?
I'll explain more about it.

quoted

"himax,pid" could be remove and use default firmware name
"himax_i2chid.bin" to load. It was added because users may desire to
choose a special name like "himax_i2chid_{pid}.bin" instead of the default
one.
It also could be replaced with newly added "himax",id-gpios" which is still
experimental.

Also, pleae don't top post, but instead reply in-line with my comments,
as I have done here.

Ok.

quoted

Btw, I encounter an error of patch [2/2], which says:
BOUNCE linux-input@vger.kernel.org: Message too long (>100000 chars)
and the patch didn't appear at patchwork.kernel.org. What should I do to
deal with this problem?

No idea. Maybe try to split it into multiple patches?
The other option is to also cc patches@lists.linux.dev as that has some
higher capacities, but that's not going to be a silver bullet.

Thanks for the reply. I'll try multiple commits to reduce the size.

Thanks,
Tylor