Re: [PATCH V2 1/2] dt-bindings: input: Introduce Himax HID-over-SPI device

[yang tylor]

On Tue, Sep 19, 2023 at 7:09 PM Conor Dooley <conor@xxxxxxxxxx> wrote:
>
> On Tue, Sep 19, 2023 at 05:31:29PM +0800, yang tylor wrote:
> > Hi Conor,
> >
> > > > Additional optional arguments:
> > > > ic-det-delay-ms and ic-resume-delay-ms are using to solve runtime
> > > > conditions.
> >
> > > 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.

> >
> > > 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.

> > 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.
>
> > 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.

> > "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.

> > Btw, I encounter an error of patch [2/2], which says:
> > BOUNCE linux-input@xxxxxxxxxxxxxxx: 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@xxxxxxxxxxxxxxx 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