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

[Rob Herring <robh@kernel.org>]

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