[PATCH 1/3] dt-bindings: Add Immersion Corporation prefix

[PATCH 1/3] dt-bindings: Add Immersion Corporation prefix

[Linus Walleij]

Immersion Corporation makes haptic feedback circuits such as
the ISA1200.

Cc: phone-devel@vger.kernel.org
Cc: devicetree@vger.kernel.org
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
 Documentation/devicetree/bindings/vendor-prefixes.yaml | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Documentation/devicetree/bindings/vendor-prefixes.yaml b/Documentation/devicetree/bindings/vendor-prefixes.yaml
index 294093d45a23..182771cc913d 100644
--- a/Documentation/devicetree/bindings/vendor-prefixes.yaml
+++ b/Documentation/devicetree/bindings/vendor-prefixes.yaml
@@ -549,6 +549,8 @@ patternProperties:
     description: Imagination Technologies Ltd.
   "^imi,.*":
     description: Integrated Micro-Electronics Inc.
+  "^immersion,.*":
+    description: Immersion Corporation
   "^incircuit,.*":
     description: In-Circuit GmbH
   "^inet-tek,.*":
-- 
2.35.1

[PATCH 2/3] dt-bindings: input: Add bindings for Immersion ISA1200

[Linus Walleij]

This adds device tree bindings for the Immersion ISA1200
haptic feedback unit.

Cc: phone-devel@vger.kernel.org
Cc: devicetree@vger.kernel.org
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
 .../bindings/input/immersion,isa1200.yaml     | 74 +++++++++++++++++++
 1 file changed, 74 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/input/immersion,isa1200.yaml

diff --git a/Documentation/devicetree/bindings/input/immersion,isa1200.yaml b/Documentation/devicetree/bindings/input/immersion,isa1200.yaml
new file mode 100644
index 000000000000..e6bbefce74a8
--- /dev/null
+++ b/Documentation/devicetree/bindings/input/immersion,isa1200.yaml
@@ -0,0 +1,74 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/input/immersion,isa1200.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Immersion ISA1200 Haptic Feedback Unit
+
+maintainers:
+  - Linus Walleij <linus.walleij@linaro.org>
+
+description:
+  The Immersion ISA1200 is a haptic feedback chip using two motors of
+  LRA or ERM type. It can generate a PWM signal to control the motors from
+  a fixed input clock, or it can amplify and modify an existing PWM
+  input. It is communicating with the host system using I2C.
+
+properties:
+  compatible:
+    description: One compatible per product using this chip. Each product
+      need deliberate custom values for things such as LRA resonance
+      frequency and these are not stored in the device tree, rather we
+      let the operating system look up the appropriate parameters from a
+      table.
+    enum:
+      - immersion,isa1200-janice
+      - immersion,isa1200-gavini
+
+  reg:
+    description: I2C address for the ISA1200
+
+  hen-gpios:
+    description: GPIO line that drives the HEN (Motor Hardware Enable) pin
+      on the chip.
+    maxItems: 1
+
+  len-gpios:
+    description: GPIO line that drives the LEN (LDO Enable) pin on the chip.
+    maxItems: 1
+
+  clocks:
+    description: Clock that drives the chip if using the chip to generate a
+      PWM from a clock. Either clocks or pwms must be defined.
+    maxItems: 1
+
+  pwms:
+    description: PWM that drives the chip if using an external PWM generator.
+      Either pwms or clocks must be defined.
+    maxItems: 1
+
+required:
+  - compatible
+  - reg
+  - hen-gpios
+  - len-gpios
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/gpio/gpio.h>
+
+    i2c {
+      #address-cells = <1>;
+      #size-cells = <0>;
+
+      haptic@49 {
+        compatible = "immersion,isa1200-janice";
+        reg = <0x49>;
+        hen-gpios = <&gpio6 2 GPIO_ACTIVE_HIGH>;
+        len-gpios = <&gpio6 3 GPIO_ACTIVE_HIGH>;
+        clocks = <&clkout_clk>;
+      };
+    };
-- 
2.35.1

Re: [PATCH 1/3] dt-bindings: Add Immersion Corporation prefix

[Rob Herring]

On Wed, 16 Mar 2022 00:35:26 +0100, Linus Walleij wrote:
> Immersion Corporation makes haptic feedback circuits such as
> the ISA1200.
> 
> Cc: phone-devel@vger.kernel.org
> Cc: devicetree@vger.kernel.org
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
>  Documentation/devicetree/bindings/vendor-prefixes.yaml | 2 ++
>  1 file changed, 2 insertions(+)
> 

Acked-by: Rob Herring <robh@kernel.org>

Re: [PATCH 2/3] dt-bindings: input: Add bindings for Immersion ISA1200

[Rob Herring]

On Wed, Mar 16, 2022 at 12:35:27AM +0100, Linus Walleij wrote:
> This adds device tree bindings for the Immersion ISA1200
> haptic feedback unit.
> 
> Cc: phone-devel@vger.kernel.org
> Cc: devicetree@vger.kernel.org
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
>  .../bindings/input/immersion,isa1200.yaml     | 74 +++++++++++++++++++
>  1 file changed, 74 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/input/immersion,isa1200.yaml
> 
> diff --git a/Documentation/devicetree/bindings/input/immersion,isa1200.yaml b/Documentation/devicetree/bindings/input/immersion,isa1200.yaml
> new file mode 100644
> index 000000000000..e6bbefce74a8
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/input/immersion,isa1200.yaml
> @@ -0,0 +1,74 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/input/immersion,isa1200.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Immersion ISA1200 Haptic Feedback Unit
> +
> +maintainers:
> +  - Linus Walleij <linus.walleij@linaro.org>
> +
> +description:
> +  The Immersion ISA1200 is a haptic feedback chip using two motors of
> +  LRA or ERM type. It can generate a PWM signal to control the motors from
> +  a fixed input clock, or it can amplify and modify an existing PWM
> +  input. It is communicating with the host system using I2C.
> +
> +properties:
> +  compatible:
> +    description: One compatible per product using this chip. Each product
> +      need deliberate custom values for things such as LRA resonance
> +      frequency and these are not stored in the device tree, rather we
> +      let the operating system look up the appropriate parameters from a
> +      table.
> +    enum:
> +      - immersion,isa1200-janice
> +      - immersion,isa1200-gavini

Same device, different boards. I think I would put necessary properties 
in the DT.

> +
> +  reg:
> +    description: I2C address for the ISA1200
> +
> +  hen-gpios:
> +    description: GPIO line that drives the HEN (Motor Hardware Enable) pin
> +      on the chip.
> +    maxItems: 1
> +
> +  len-gpios:
> +    description: GPIO line that drives the LEN (LDO Enable) pin on the chip.
> +    maxItems: 1
> +
> +  clocks:
> +    description: Clock that drives the chip if using the chip to generate a
> +      PWM from a clock. Either clocks or pwms must be defined.
> +    maxItems: 1
> +
> +  pwms:
> +    description: PWM that drives the chip if using an external PWM generator.
> +      Either pwms or clocks must be defined.

That can be expressed as:

oneOf:
 - required: [ clocks ]
 - required: [ pwms ]

(or 'anyOf' if you want to allow both)

> +    maxItems: 1
> +
> +required:
> +  - compatible
> +  - reg
> +  - hen-gpios
> +  - len-gpios
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/gpio/gpio.h>
> +
> +    i2c {
> +      #address-cells = <1>;
> +      #size-cells = <0>;
> +
> +      haptic@49 {
> +        compatible = "immersion,isa1200-janice";
> +        reg = <0x49>;
> +        hen-gpios = <&gpio6 2 GPIO_ACTIVE_HIGH>;
> +        len-gpios = <&gpio6 3 GPIO_ACTIVE_HIGH>;
> +        clocks = <&clkout_clk>;
> +      };
> +    };
> -- 
> 2.35.1
> 
> 

Re: [PATCH 2/3] dt-bindings: input: Add bindings for Immersion ISA1200

[Linus Walleij]

On Mon, Mar 21, 2022 at 8:10 PM Rob Herring <robh@kernel.org> wrote:

> > +properties:
> > +  compatible:
> > +    description: One compatible per product using this chip. Each product
> > +      need deliberate custom values for things such as LRA resonance
> > +      frequency and these are not stored in the device tree, rather we
> > +      let the operating system look up the appropriate parameters from a
> > +      table.
> > +    enum:
> > +      - immersion,isa1200-janice
> > +      - immersion,isa1200-gavini
>
> Same device, different boards. I think I would put necessary properties
> in the DT.

That will be all of these (from the driver):

+struct isa1200_config {
+       u8 ldo_voltage;
+       bool pwm_in;
+       bool erm;
+       u8 clkdiv;
+       u8 plldiv;
+       u8 freq;
+       u8 duty;
+       u8 period;
+};

Example:

+/* Configuration for Janice, Samsung Galaxy S Advance GT-I9070 */
+static const struct isa1200_config isa1200_janice = {
+       .ldo_voltage = ISA1200_LDO_VOLTAGE_30V,
+       .pwm_in = false,
+       .clkdiv = ISA1200_HCTRL0_DIV_256,
+       .plldiv = 2,
+       .freq = 0,
+       .duty = 0x3b,
+       .period = 0x77,
+};

This is derived from the compatible rather than individual properties
or extra regulator and/or clock abstractions in line with:
Documentation/devicetree/bindings/display/panel/ilitek,ili9322.yaml

Which was originally looking like so:
https://lore.kernel.org/dri-devel/20170813114448.20179-2-linus.walleij@linaro.org/

To which you replied:
https://lore.kernel.org/dri-devel/20170817204424.e2wdkmyp4vyx2qj3@rob-hp-laptop/

"Normally, we the physical panel is described which would imply all these
settings. Are there lots of panels with this controller that would
justify all these settings?"

In that case there was one (1)

In this case there are two (2) products that I know of. It does not have the
relationship between panel and panel controller products though, but...
it's not very different.

I don't think this chip was used a lot, I really tried to find other instances.
But they could exist of course.

If you are certain you want all of these in the device tree instead, I can
fix it of course, then I also want to know if e.g. regulator framework should
be employed for the (internal) LDO and clock framework for the internal
PLL divider so I don't have to rewrite the whole thing again.

Yours,
Linus Walleij

Re: [PATCH 2/3] dt-bindings: input: Add bindings for Immersion ISA1200

[Rob Herring]

On Wed, Mar 23, 2022 at 03:57:44PM +0100, Linus Walleij wrote:
> On Mon, Mar 21, 2022 at 8:10 PM Rob Herring <robh@kernel.org> wrote:
> 
> > > +properties:
> > > +  compatible:
> > > +    description: One compatible per product using this chip. Each product
> > > +      need deliberate custom values for things such as LRA resonance
> > > +      frequency and these are not stored in the device tree, rather we
> > > +      let the operating system look up the appropriate parameters from a
> > > +      table.
> > > +    enum:
> > > +      - immersion,isa1200-janice
> > > +      - immersion,isa1200-gavini
> >
> > Same device, different boards. I think I would put necessary properties
> > in the DT.
> 
> That will be all of these (from the driver):
> 
> +struct isa1200_config {
> +       u8 ldo_voltage;
> +       bool pwm_in;
> +       bool erm;
> +       u8 clkdiv;
> +       u8 plldiv;
> +       u8 freq;
> +       u8 duty;
> +       u8 period;
> +};

Could be all, but in your 2 cases some of these values are the same.

> 
> Example:
> 
> +/* Configuration for Janice, Samsung Galaxy S Advance GT-I9070 */
> +static const struct isa1200_config isa1200_janice = {
> +       .ldo_voltage = ISA1200_LDO_VOLTAGE_30V,
> +       .pwm_in = false,
> +       .clkdiv = ISA1200_HCTRL0_DIV_256,
> +       .plldiv = 2,
> +       .freq = 0,
> +       .duty = 0x3b,
> +       .period = 0x77,
> +};
> 
> This is derived from the compatible rather than individual properties
> or extra regulator and/or clock abstractions in line with:
> Documentation/devicetree/bindings/display/panel/ilitek,ili9322.yaml
> 
> Which was originally looking like so:
> https://lore.kernel.org/dri-devel/20170813114448.20179-2-linus.walleij@linaro.org/
> 
> To which you replied:
> https://lore.kernel.org/dri-devel/20170817204424.e2wdkmyp4vyx2qj3@rob-hp-laptop/
> 
> "Normally, we the physical panel is described which would imply all these
> settings. Are there lots of panels with this controller that would
> justify all these settings?"

I reserve the right to contradict myself. :)

Seriously, it's always a judgement call.

> 
> In that case there was one (1)
> 
> In this case there are two (2) products that I know of. It does not have the
> relationship between panel and panel controller products though, but...
> it's not very different.
> 
> I don't think this chip was used a lot, I really tried to find other instances.
> But they could exist of course.

Okay, if you want to leave it like this, I'm fine with that.

Rob