| Message ID | 20240131-x13s-touchscreen-v4-1-39c0f9925d3c@quicinc.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | arm64: dts: qcom: sc8280xp-x13s: Enable touchscreen | expand |
On Wed, Jan 31, 2024 at 07:07:26PM -0800, Bjorn Andersson wrote: > Some I2C HID devices has a reset pin and requires that some specified > time elapses after this reset pin is deasserted, before communication > with the device is attempted. > > The Linux implementation is looking for these in the "reset-gpios" and > "post-reset-deassert-delay-ms" properties already, so use these property > names. > > Reviewed-by: Johan Hovold <johan+linaro@kernel.org> > Acked-by: Rob Herring <robh@kernel.org> > Signed-off-by: Bjorn Andersson <quic_bjorande@quicinc.com> > --- > Documentation/devicetree/bindings/input/hid-over-i2c.yaml | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/Documentation/devicetree/bindings/input/hid-over-i2c.yaml b/Documentation/devicetree/bindings/input/hid-over-i2c.yaml > index 138caad96a29..f07ff4cb3d26 100644 > --- a/Documentation/devicetree/bindings/input/hid-over-i2c.yaml > +++ b/Documentation/devicetree/bindings/input/hid-over-i2c.yaml > @@ -50,6 +50,12 @@ properties: > description: Time required by the device after enabling its regulators > or powering it on, before it is ready for communication. > > + post-reset-deassert-delay-ms: > + description: Time required by the device after reset has been deasserted, > + before it is ready for communication. I know that Rob reluctantly acked this, but re-reading the commit message for the commit that added support for the reset gpio to the driver, and added a comment about this not having been added to the devicetree binding, it becomes obvious that the latter was done on purpose and that we probably should not be adding the 'post-reset-deassert-delay-ms' property after all: For now the new "post-reset-deassert-delay-ms" property is only used on x86/ACPI (non devicetree) devs. IOW it is not used in actual devicetree files and the same goes for the reset GPIO. The devicetree-bindings maintainers have requested properties like these to not be added to the devicetree-bindings, so the new property + GPIO are deliberately not added to the existing devicetree-bindings. 2be404486c05 ("HID: i2c-hid-of: Add reset GPIO support to i2c-hid-of") So perhaps we should just do this properly and add a new compatible property for X13s touchscreen which can be used to determine these delays (e.g. for cases where some default values are insufficient). Johan
On Thu, Feb 01, 2024 at 11:05:47AM +0100, Johan Hovold wrote: > On Wed, Jan 31, 2024 at 07:07:26PM -0800, Bjorn Andersson wrote: > > Some I2C HID devices has a reset pin and requires that some specified > > time elapses after this reset pin is deasserted, before communication > > with the device is attempted. > > > > The Linux implementation is looking for these in the "reset-gpios" and > > "post-reset-deassert-delay-ms" properties already, so use these property > > names. > > > > Reviewed-by: Johan Hovold <johan+linaro@kernel.org> > > Acked-by: Rob Herring <robh@kernel.org> > > Signed-off-by: Bjorn Andersson <quic_bjorande@quicinc.com> > > --- > > Documentation/devicetree/bindings/input/hid-over-i2c.yaml | 6 ++++++ > > 1 file changed, 6 insertions(+) > > > > diff --git a/Documentation/devicetree/bindings/input/hid-over-i2c.yaml b/Documentation/devicetree/bindings/input/hid-over-i2c.yaml > > index 138caad96a29..f07ff4cb3d26 100644 > > --- a/Documentation/devicetree/bindings/input/hid-over-i2c.yaml > > +++ b/Documentation/devicetree/bindings/input/hid-over-i2c.yaml > > @@ -50,6 +50,12 @@ properties: > > description: Time required by the device after enabling its regulators > > or powering it on, before it is ready for communication. > > > > + post-reset-deassert-delay-ms: > > + description: Time required by the device after reset has been deasserted, > > + before it is ready for communication. > > I know that Rob reluctantly acked this, but re-reading the commit > message for the commit that added support for the reset gpio to the > driver, and added a comment about this not having been added to the > devicetree binding, it becomes obvious that the latter was done on > purpose and that we probably should not be adding the > 'post-reset-deassert-delay-ms' property after all: > > For now the new "post-reset-deassert-delay-ms" property is only > used on x86/ACPI (non devicetree) devs. IOW it is not used in > actual devicetree files and the same goes for the reset GPIO. > The devicetree-bindings maintainers have requested properties > like these to not be added to the devicetree-bindings, so the > new property + GPIO are deliberately not added to the existing > devicetree-bindings. > > 2be404486c05 ("HID: i2c-hid-of: Add reset GPIO support to i2c-hid-of") > > So perhaps we should just do this properly and add a new compatible > property for X13s touchscreen which can be used to determine these > delays (e.g. for cases where some default values are insufficient). > So we should add a new binding, with a device-specific compatible and add a reset-gpios only for that (and not the generic hid-over-i2c binding), and then in the i2c-hid driver encode the two delays? I can try to rewrite these patches, if you can provide me with a compatible. Regards, Bjorn
On Thu, Feb 01, 2024 at 04:08:05PM -0600, Bjorn Andersson wrote: > On Thu, Feb 01, 2024 at 11:05:47AM +0100, Johan Hovold wrote: > > On Wed, Jan 31, 2024 at 07:07:26PM -0800, Bjorn Andersson wrote: > > > + post-reset-deassert-delay-ms: > > > + description: Time required by the device after reset has been deasserted, > > > + before it is ready for communication. > > > > I know that Rob reluctantly acked this, but re-reading the commit > > message for the commit that added support for the reset gpio to the > > driver, and added a comment about this not having been added to the > > devicetree binding, it becomes obvious that the latter was done on > > purpose and that we probably should not be adding the > > 'post-reset-deassert-delay-ms' property after all: > > > > For now the new "post-reset-deassert-delay-ms" property is only > > used on x86/ACPI (non devicetree) devs. IOW it is not used in > > actual devicetree files and the same goes for the reset GPIO. > > The devicetree-bindings maintainers have requested properties > > like these to not be added to the devicetree-bindings, so the > > new property + GPIO are deliberately not added to the existing > > devicetree-bindings. > > > > 2be404486c05 ("HID: i2c-hid-of: Add reset GPIO support to i2c-hid-of") > > > > So perhaps we should just do this properly and add a new compatible > > property for X13s touchscreen which can be used to determine these > > delays (e.g. for cases where some default values are insufficient). > > So we should add a new binding, with a device-specific compatible and > add a reset-gpios only for that (and not the generic hid-over-i2c > binding), and then in the i2c-hid driver encode the two delays? Right. > I can try to rewrite these patches, if you can provide me with a > compatible. My X13s doesn't have a touchscreen, but the ACPI tables says "ELAN901C". You can look at the current binding and work with the HID and DT maintainers to come up with something appropriate. There is one device-specific compatible in the DT schema currently: wacom,w9013 so something like elan,<product> where <product> is a name for the device with product id 0x901c (or you use the HID product id directly somehow). Johan
diff --git a/Documentation/devicetree/bindings/input/hid-over-i2c.yaml b/Documentation/devicetree/bindings/input/hid-over-i2c.yaml index 138caad96a29..f07ff4cb3d26 100644 --- a/Documentation/devicetree/bindings/input/hid-over-i2c.yaml +++ b/Documentation/devicetree/bindings/input/hid-over-i2c.yaml @@ -50,6 +50,12 @@ properties: description: Time required by the device after enabling its regulators or powering it on, before it is ready for communication. + post-reset-deassert-delay-ms: + description: Time required by the device after reset has been deasserted, + before it is ready for communication. + + reset-gpios: true + touchscreen-inverted-x: true touchscreen-inverted-y: true