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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAGD2q_bkTpvXiomWb_yerNjQfMVKOctYgBqF_RBSo_jYqyyyxw@mail.gmail.com>
Date:   Fri, 22 Sep 2023 15:56:25 +0800
From:   yang tylor <tylor_yang@...ax.corp-partner.google.com>
To:     Conor Dooley <conor@...nel.org>
Cc:     dmitry.torokhov@...il.com, robh+dt@...nel.org,
        krzysztof.kozlowski+dt@...aro.org, conor+dt@...nel.org,
        jikos@...nel.org, benjamin.tissoires@...hat.com,
        linux-input@...r.kernel.org, devicetree@...r.kernel.org,
        linux-kernel@...r.kernel.org,
        poyuan_chang@...ax.corp-partner.google.com, hbarnor@...omium.org,
        "jingyliang@...omium.org" <jingyliang@...omium.org>
Subject: Re: [PATCH V2 1/2] dt-bindings: input: Introduce Himax HID-over-SPI device

On Tue, Sep 19, 2023 at 7:09 PM Conor Dooley <conor@...nel.org> 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@...r.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@...ts.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