On Wed, Mar 23, 2022 at 03:57:44PM +0100, Linus Walleij wrote:

On Mon, Mar 21, 2022 at 8:10 PM Rob Herring [off-list ref] wrote:

quoted

quoted

+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/ (local)

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

"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