Re: [PATCH] Input: qt1050 - add Microchip AT42QT1050 support

[Marco Felsch <m.felsch@pengutronix.de>]

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