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

[yang tylor]

On Thu, Oct 12, 2023 at 11:24 PM Conor Dooley <conor@xxxxxxxxxx> wrote:
>
> On Thu, Oct 12, 2023 at 10:30:03AM +0800, yang tylor wrote:
> > On Tue, Oct 10, 2023 at 1:52 AM Conor Dooley <conor@xxxxxxxxxx> wrote:
> > >
> > > On Mon, Oct 02, 2023 at 06:44:41PM +0800, yang tylor wrote:
> > > > On Fri, Sep 29, 2023 at 12:56 AM Conor Dooley <conor@xxxxxxxxxx> wrote:
> > > > >
> > > > > On Thu, Sep 28, 2023 at 10:12:41AM +0800, yang tylor wrote:
> > > > > > On Tue, Sep 26, 2023 at 8:53 PM Conor Dooley <conor@xxxxxxxxxx> wrote:
> > > > > > > On Tue, Sep 26, 2023 at 05:52:39PM +0800, yang tylor wrote:
> > > > > > > > On Tue, Sep 26, 2023 at 5:02 PM Conor Dooley <conor@xxxxxxxxxx> wrote:
> > > > > > > > > On Mon, Sep 25, 2023 at 06:16:29PM +0800, yang tylor wrote:
> > > > > > > > > > On Mon, Sep 25, 2023 at 4:41 PM Conor Dooley <conor.dooley@xxxxxxxxxxxxx> wrote:
> > > > > > > > > > We have a default prefix firmware name(like himax_xxxx.bin) in the driver code.
> > > > > > > > >
> > > > > > > > > How do you intend generating the name of the firmware file? I assume the
> > > > > > > > > same firmware doesn't work on every IC, so you'll need to pick a
> > > > > > > > > different one depending on the compatible?
> > > > > > > > >
> > > > > > > > If considering a firmware library line-up for all the incoming panels
> > > > > > > > of this driver.
> > > > > > > > We would use PID as part of the file name. Because all the support panels would
> > > > > > > > have a unique PID associated. Which will make the firmware name like
> > > > > > > > himax_xxx_{$PID}.bin. The problem is, we need to know PID before firmware load
> > > > > > > > at no flash condition. Thus PID information is required in dts when
> > > > > > > > no-flash-flag
> > > > > > > > is specified.
> > > > > > >
> > > > > > > Firstly, where does the "xxx" come from?
> > > > > > > And you're making it sound more like having firmware-name is suitable
> > > > > > > for this use case, given you need to determine the name of the file to
> > > > > > > use based on something that is hardware specific but is not
> > > > > > > dynamically detectable.
> > > > > > Current driver patch uses a prefix name "himax_i2chid" which comes
> > > > > > from the previous project
> > > > > >  and seems not suitable for this condition, so I use "xxx" and plan to
> > > > > > replace it in the next version.
> > > > > > For finding firmware, I think both solutions are reasonable.
> > > > > > - provide firmware name directly: implies no-flash and use user
> > > > > > specified firmware, no PID info.
> > > > > > - provide no-flash-flag and PID info: loading firmware from organized
> > > > > > names with PID info.
> > > > > > I prefer the 2nd solution, but it needs more properties in dts. 1st
> > > > > > has less properties and more
> > > > > > intuitive.
> > > > > >
> > > > > > I don't know which one is more acceptable by the community, as you
> > > > > > know I'm a newbie here.
> > > > >
> > > > > To be honest, I am not all that sure either! Does the panel id have
> > > > > value in its own right, or is that only used to determine the firmware
> > > > > filename?
> > > > Currently, PID stands for Panel/Project ID and is used for determining
> > > > the firmware filename only. We haven't come up with any new attribute that
> > > > may attach to it. The differences between panels are handled in firmware
> > > > dedicated to its PID.
> > > >
> > > > > Also, if it does have value in its own right, rather than a "pid",
> > > > > should the panel be a child node of this hid device with its own
> > > > > compatible?
> > > > It may need a child node if we find it necessary to add attributes to each PID.
> > > > But currently we have no idea about it.
> > >
> > > To be honest, it seems to me like you are using "PID" in place of a
> > > compatible for the panel, since it needs to be provided via DT anyway.
> >
> > Hmm... So the more formal way is?
> > If I add a sub-note inside this spi-device block, such as "panel" and
> > add PID inside.
> > Will it be more appropriate?
> > ...
> > spi {
> > ...
> >     hx_spi@0 {
> > ...
> >         panel {
> >             himax,pid = ...
>
> And this now looks exactly like compatible = "vendor,part" now, no?

I think it's not the same, I thought "compatible" is used to target
from the driver side.
For finding other information inside the block. But I just store PID
information in this
one, not used for targeting but getting infos from it.

Thanks,
Tylor