On 18-10-18 11:23, Dmitry Torokhov wrote:
> On Thu, Oct 18, 2018 at 10:13:08AM +0200, Marco Felsch wrote:
> > Hi Dmitry,
> > On 18-10-17 17:39, Dmitry Torokhov wrote:
> > > On Thu, Oct 18, 2018 at 01:31:53AM +0200, Marco Felsch wrote:
> > > > On 18-10-15 20:44, Dmitry Torokhov wrote:
> > > > > On Mon, Sep 24, 2018 at 05:13:30PM +0200, Marco Felsch wrote:
> > > > > > +static int qt1050_disable_key(struct regmap *map, int number)
> > > > > > +{
> > > > > > + unsigned int reg;
> > > > > > +
> > > > > > + switch (number) {
> > > > > > + case 0:
> > > > > > + reg = QT1050_DI_AKS_0;
> > > > > > + break;
> > > > > > + case 1:
> > > > > > + reg = QT1050_DI_AKS_1;
> > > > > > + break;
> > > > > > + case 2:
> > > > > > + reg = QT1050_DI_AKS_2;
> > > > > > + break;
> > > > > > + case 3:
> > > > > > + reg = QT1050_DI_AKS_3;
> > > > > > + break;
> > > > > > + case 4:
> > > > > > + reg = QT1050_DI_AKS_4;
> > > > > > + break;
> > >
> > > I wonder if there is any value in doing
> > >
> > > reg = QT1050_DI_AKS_0 + i + (i > 2);
> > >
> > > and similarly for other registers.
> >
> > Yes there is a gap in the register map and your approach is smart, but I
> > find my less error prone (i.e. it should be (i > 1)) and easier to read
> > it. Anyway I can change this if you find it better.
>
> No, I was just wondering if we could avoid bunch of "switch"es in the
> code. Maybe have static const array of structures for various registers
> and offsets which you index by key number?
Yes, that is a better approach. I will convert the driver to it, thanks
for your suggestion.
>
> ...
>
> > > > > > + if (IS_ENABLED(CONFIG_OF)) {
> > > > > > + err = qt1050_parse_dt(ts);
> > > > > > + if (err) {
> > > > > > + dev_err(dev, "Failed to parse dt: %d\n", err);
> > > > > > + return err;
> > > > > > + }
> > > > > > + } else {
> > > > > > + /* init each key with a valid code */
> > > > > > + for (i = 0; i < QT1050_MAX_KEYS; i++)
> > > > > > + ts->keys[i].keycode = KEY_1 + i;
> > > > > > + }
> > > > >
> > > > > I'd rather we used generic device properties (i.e.
> > > > > device_property_read_xxx() instead of of_property_read_xxx()) and did
> > > > > not provide this fallback.
> > > >
> > > > I'm with you, but I wanted to use the of_property_read_variable_*()
> > > > helpers, since all properties can distinguish in their array size.
> > > > Sure I can add a helper to reimplement that localy using the
> > > > device_property_read_xxx() functions. IMHO this will be a later on
> > > > feature, if the acpi guys needs this features too. Is that okay?
> > >
> > > Well, that is an argument for adding proper
> > > device_property_read_variable_*(). However, after lookign at the binding
> > > again, I wonder if you should not describe this as sub-nodes:
> >
> > A device_property_read_variable_*() would be nice.
>
> Then please add it ;)
>
> >
> > >
> > > touchkeys@41 {
> > > ...
> > > up@0 {
> > > reg = <0>;
> > > linux,code = <KEY_UP>;
> > > average-samples = <64>;
> > > pre-scaling = <16>;
> > > pressure-threshold = <...>;
> > > };
> > >
> > > right@1 {
> > > reg = <1>;
> > > linux,code = <KEY_RIGHT>;
> > > average-samples = <64>;
> > > pre-scaling = <8>;
> > > pressure-threshold = <2>;
> > > };
> > > ...
> > > };
> > >
> > > and then use device_for_each_child_node() to parse it.
> >
> > That's a good approach, thanks. I wanted to be similar with the other
> > input bindings, which uses arrays for linux,code and scalar values for
> > other properties. Since the qt1050 can configure each pad differently
> > I used only arrays. Is it good to change the layout only for one deivce?
> > Maybe it would better to implement the device_property_read_variable_*()
> > helper.
>
> We do have similar binding in input, namely
> Documentation/devicetree/bindings/input/gpio-keys.txt
> I think if keys form a simple array and not allow individual
> configuration (noise, number of samples, etc), then weshoudl use
> linux,keycodes binding, and if keys are individually controllable you
> might want to use the sub-node approach.
Yes, I got your point.
Thanks fpr your feedback I will integrate it and send a v2.
Regards,
Marco
