Re: [PATCH 2/2 v1] Input: cy8ctma140 - add driver

[Dmitry Torokhov <dmitry.torokhov@gmail.com>]

On Sun, Mar 15, 2020 at 05:12:29PM +0100, Linus Walleij wrote:
> Hi Dmitry,
> 
> thanks for the quick review!
> 
> I fixed most stuff, just one minor comment:
> 
> On Tue, Mar 10, 2020 at 6:08 PM Dmitry Torokhov
> <dmitry.torokhov@gmail.com> wrote:
> 
> > > +     /* According to datasheet this should be in the 2.7-3.6 V range */
> > > +     ret = regulator_set_voltage(ts->vcpin, 2700000, 3600000);
> > > +     if (ret) {
> > > +             dev_err(dev, "failed to set VCPIN voltage\n");
> > > +             return ret;
> > > +     }
> >
> > Shouldn't this already be in DT? We typically do not configure voltage
> > on various rail unless in very specific circumstances.
> 
> Certainly the DT defines the voltage range on the regulator
> on the producer side.
> 
> This is the consumer range, and DT has no facility to put
> restrictions on the consumer voltage window.
> 
> I think it is pretty natural to do in the code.

That means we will have essentially a boilerplate code in many many
drivers.

If we indeed want to do this (although I am not sure if practically this
makes much sense - nobody creates a rail delivering 24 volts by default
and saying "oh well, when driver loads it will request us to lower the
voltage down to 1.5V that components attached to the rail require") can
we consider adding consumer side constraints to the DT so that
regulator_get() can set the voltage right there and driver does not have
to? I am just trying to limit the amount of custom code in the drivers.

> 
> The typical usecase is when two components share the
> same line, and in this case two voltage consumer inputs
> *may* be connected to the same producer regulator.
> 
> The regulator framework will the infer that the producer
> can produce a voltage that fulfil the constraints on all the
> consumers.
> 
> The fact that few devices issue regulator_set_voltage()
> is a combination of "good enough" and general sloppiness,
> I think it should reflect the operation voltages of the component
> and stop people from shooting themselves in the foot.
> 
> But I CC Mark on this and see what he says. (I might be wrong.)

Thanks.

-- 
Dmitry