Re: [PATCH v2 2/2] Input: isa1200 - new driver for Imagis ISA1200

[Svyatoslav Ryhel]

чт, 30 квіт. 2026 р. о 12:22 Linus Walleij <linusw@xxxxxxxxxx> пише:
>
> Hi Svyatoslav,
>
> I just saw this thing:
>
> On Tue, Apr 28, 2026 at 1:43 PM Svyatoslav Ryhel <clamor95@xxxxxxxxx> wrote:
>
> > +/* HCTRL5 controls the PWM high duty cycle of internal channel */
> > +#define ISA1200_HCTRL5                 0x35
> (...)
> > +struct isa1200_config {
> (...)
> > +       u32 duty;
> > +};
> > +       /* Duty cycle */
> > +       regmap_write(isa->map, ISA1200_HCTRL5, config->period >> 1);
> (...)
> > +       if (isa->clk)
> > +               regmap_write(isa->map, ISA1200_HCTRL5, config->duty);
> (..)
> > +       if (isa->clk) {
> > +               ret = device_property_read_u32(dev, "imagis,period-ns",
> > +                                              &config->period);
> > +               if (ret)
> > +                       return dev_err_probe(dev, ret,
> > +                                            "failed to get period\n");
> > +
> > +               config->period /= ISA1200_HCTRL6_PERIOD_SCALE;
> > +               config->duty = config->period >> 1;
> > +       }
> > +
> > +       if (isa->pwm) {
> > +               struct pwm_state state;
> > +
> > +               pwm_init_state(isa->pwm, &state);
> > +
> > +               if (!state.period)
> > +                       return dev_err_probe(dev, -EINVAL,
> > +                                            "PWM period cannot be zero\n");
> > +
> > +               config->freq = div64_u64(NANO, state.period * config->clkdiv);
> > +               config->duty = state.period >> 1;
>
> So you don't need the same code writing
> regmap_write(isa->map, ISA1200_HCTRL5, config->period >> 1);
> above, just use confg->duty which contains config->period >> 1
> (...)

No this is intended. It seems that isa1200 uses period >> 1 (aka 1/2
of period for some type of pre-enable). Setting duty as 1/2 period be
default is a safe fallback but it will not trigger haptics.

> > +       device_property_read_u32(dev, "imagis,duty-cycle-ns", &config->duty);
>
> I'm not sure about this, it works for me when I set this to 100
> resulting in a register write of 100 to ISA1200_HCTRL5. But are
> we sure that this register is expressed in nanoseconds? I think
> some calculation is needed here (and probably another value
> in my device tree).
>

I do are that this most likely is somehow mapped, but I have no clue
how to transfer this into actual nanoseconds. That is very
unfortunate.

In you original code
/*
* This is done in the vendor tree with the commment
* "Duty 0x64 == nForce 90", and no force feedback happens
* unless we do this.
*/
if (isa->clk)
regmap_write(isa->map, ISA1200_HCTRL5, 0x64);

0x64 is actually some conversion of duty cycle, you got this accurately.

> Yours,
> Linus Walleij