Re: [PATCH v4 3/7] input: keyboard: Add support for Azoteq IQS620A/621/622/624/625

["dmitry.torokhov@gmail.com" <dmitry.torokhov@gmail.com>]

On Sun, Jan 19, 2020 at 10:40:31PM +0000, Jeff LaBundy wrote:
> On Fri, Jan 17, 2020 at 01:33:30PM -0800, dmitry.torokhov@gmail.com wrote:
> > On Fri, Jan 17, 2020 at 02:35:46AM +0000, Jeff LaBundy wrote:
> > > +
> > > +	ret = device_property_read_u32_array(&pdev->dev, "linux,keycodes",
> > > +					     iqs62x_keys->keycode,
> > > +					     iqs62x_keys->keycodemax);
> > > +	if (ret) {
> > > +		dev_err(&pdev->dev, "Failed to read keycodes: %d\n", ret);
> > > +		return ret;
> > > +	}
> > 
> > I wonder why you can't simply use
> > 
> > 	error = device_property_read_u32_array(&pdev->dev, "linux,keycodes",
> > 						iqs62x_keys->keycode,
> > 						IQS62X_NUM_KEYS);
> > 
> > Are you concerned with someone trying to set up keys that are not
> > actually exposed later via EVOCSKEYCODES and that is why you are
> > limiting keycodemax?
> 
> When I try this, I find that device_property_read_u32_array returns -EOVERFLOW
> for arrays with fewer than IQS62X_NUM_KEYS elements. To avoid forcing users to
> pad the array all the way out to IQS62X_NUM_KEYS in the case of simple channel
> assignments (like those in the example bindings), keycodemax must be passed to
> device_property_read_u32_array which means it must be limited before-hand. The
> same method seems to be used in other drivers as well (e.g. mpr121_touchkey).

Ah, indeed, ignore me here please.

...

> > > +MODULE_AUTHOR("Jeff LaBundy <jeff@labundy.com>");
> > > +MODULE_DESCRIPTION("Azoteq IQS620A/621/622/624/625 Keys and Switches");
> > > +MODULE_LICENSE("GPL");
> > > +MODULE_ALIAS("platform:" IQS62X_DRV_NAME_KEYS);
> > 
> > Otherwise
> > 
> > Acked-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> > 
> > I suppose it will be merged through MFD?
> 
> That's the plan; Lee confirmed this would be OK once the series is ready. Just
> as a heads up, I expect minor changes to this and other patches as iqs62x.h is
> hardened (e.g. "iqs62x->map" --> "iqs62x->regmap"). I assume you're OK with me
> keeping your Acked-by unless there are major changes, but let me know if you'd
> prefer I didn't.

Yes, please keep Acked-by unless there is significant rework.

Thanks.

-- 
Dmitry