Re: [PATCH] dt-bindings: input: Add the Goodix GTx5 series Touchscreen devicetree bindings

[Wang Yafei <wangyafei-PiwChjOeyQzQT0dZR+AlfA@public.gmane.org>]

On Mon, Jun 26, 2017 at 09:16:40AM -0500, Rob Herring wrote:
> On Mon, Jun 26, 2017 at 8:22 AM, Wang Yafei <wangyafei-PiwChjOeyQzQT0dZR+AlfA@public.gmane.org> wrote:
> > Hi Rob,
> >
> > Thank you for the review.
> >
> > On Fri, Jun 23, 2017 at 03:37:06PM -0500, Rob Herring wrote:
> >> On Mon, Jun 19, 2017 at 06:40:32PM +0800, Wang Yafei wrote:
> >> > Signed-off-by: Wang Yafei <wangyafei-PiwChjOeyQzQT0dZR+AlfA@public.gmane.org>
> >> > ---
> >> >  .../bindings/input/touchscreen/goodix-gtx5.txt     | 75 ++++++++++++++++++++++
> >> >  1 file changed, 75 insertions(+)
> >> >  create mode 100755 Documentation/devicetree/bindings/input/touchscreen/goodix-gtx5.txt
> 
> [...]
> 
> >> > +- touchscreen-key-map: keycode value map  /*KEY_HOMEPAGE, KEY_BACK*/
> >>
> >> This hardly seems sufficient to map screen areas to keys. There's been
> >> some discussion about doing that in the past.
> >
> > Sorry haven't notice that before, I will change it to the flowing
> > property format:
> > linux,keycodes = <KEY_HOMEPAGE>, <KEY_MENU>, <KEY_BACK>;
> 
> That's better, but not really what I meant. There's still some
> assumption about what region each key is located in. Is every design 3
> keys? How do you know what region of the touchscreen is a given key?
> iOW, what are the valid ranges of coordinates for each key?
> 
> 
> >> > +           compatible = "goodix,gtx5";
> >> > +           reg = <0x14>;
> >> > +           interrupt-parent = <&msm_gpio>;
> >> > +           interrupts = <13 0x2800>;
> >> > +           vtouch-supply = <&pm8916_l15>;
> >> > +           reset-gpios = <&msm_gpio 12 0x0>;
> >> > +           irq-gpios = <&msm_gpio 13 0x2800>;
> >> > +           irq-flags = <1>; /* 1:trigger rising, 2:trigger falling;*/
> >> > +           touchscreen-max-id = <10>;
> >> > +           touchscreen-size-x = <400>;
> >> > +           touchscreen-size-y = <400>;
> >> > +           touchscreen-max-w = <400>;
> >> > +           touchscreen-max-pressure = <255>;
> >> > +           touchscreen-swapped-x-y;
> >> > +           touchscreen-key-map = <172 158>; /*KEY_HOMEPAGE, KEY_BACK*/
> >> > +           sensor0 {
> >>
> >> How many sensors can there be? That needs to be documented.
> >>
> >> Why do you need a sub-node?
> >
> > sensor is an internal call, we use senesorX to represent the touchpanel
> > is produced by which factory. A panel factory will have it corresponding
> > config data to get the best touch results. It will delete the sub-node
> > and change to the following format:
> >         panel0,normal-cfg = [...]  /* panelX,cfg : X is no more than 10*/
> 
> panel0 is not a vendor and normal doesn't tell us anything. How about
> just "goodix,panel-cfg-X"?

Can I use "goodix,normal-cfg-X" rather than "goodix,panel-cfg-X"? The
string "normal-cfg" is meaningful to driver.

> Rob
> --
> To unsubscribe from this list: send the line "unsubscribe linux-input" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html