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@xxxxxxxxxx> 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@xxxxxxxxxx/
>
> 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
