linux-kernel - Re: [PATCH 3/3] max7359_keypad: implement DT bindings

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20150518170203.GD19708@dtor-ws>
Date:	Mon, 18 May 2015 10:02:03 -0700
From:	Dmitry Torokhov <dmitry.torokhov@...il.com>
To:	Grant Likely <grant.likely@...aro.org>, robh+dt@...nel.org,
	Evgeniy Dushistov <dushistov@...l.ru>
Cc:	devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
	linux-input@...r.kernel.org
Subject: Re: [PATCH 3/3] max7359_keypad: implement DT bindings

On Sun, May 17, 2015 at 07:55:02PM +0300, Evgeniy Dushistov wrote:
> On Fri, May 15, 2015 at 11:00:02PM +0200, Dmitry Torokhov wrote:
> > On Thu, May 14, 2015 at 05:38:03AM +0300, Evgeniy Dushistov wrote:
> > > +		maxim,debounce_reg = /bits/ 8 <0x5F>;
> > > +		maxim,ports_reg = /bits/ 8 <0xFF>;
> > 
> > Specifying exact size for properties is quite uncommon; I think the
> > usual recommendation is is to use the "standard" u32 and validate the
> > range in parser function.
> > 
> 
> Using of u8 has advantages, it is possible on compilation stage
> (dts->dtb) check that you enter right value, but
> because of DT validation tools are not part of mainline,
> I replace u8 with u32 as you suggest, see patch bellow.

If you get DT folks to OK using /bits/ - that's fine. Grant, Rob, any
guidance here?

> 
> > > +		MATRIX_KEY(0, 7, KEY_RESERVED)
> > > +		...
> > 
> > 
> > Indent one more level? Also, maybe fill with something other than
> > KEY_RESERVED?
> > 
> 
> Fixed, see patch bellow.
> 
> > 
> > 
>  
> > >  	dev_dbg(&client->dev, "keys FIFO is 0x%02x\n", ret);
> > > +	if (!keymap_data) {
> > > +		error = max7359_parse_dt(&client->dev, &init_state);
> > > +		if (error) {
> > > +			dev_err(&client->dev, "platform data null, and no DT data\n");
> > > +			return error;
> > 
> > Both debounce and ports values are optional and we'll fail building
> > keymap if there are no platform data nor device tree data, so I would
> > drop this check and the stub for max7359_parse_dt() as well - we have
> > stubs for property parsing anyway.
> > 
> 
> Because of u32/u8 check at now max7359_parse_dt can return not 0.
> 
> > -- 
> > Dmitry
> > 
> 
> Thanks, for review. 
> 
> Please add me to CC, because of I'm not the part
> of any mailing list (too huge traffic for me).

Umm, if you want to be replied (or CCed) to, why do you make sure that
replies do not go to you by default??? Why do you set "Mail-Followup-To"
to exclude yourself?

> Mail-Followup-To: Dmitry Torokhov <dmitry.torokhov@...il.com>,
> 	devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
> 	linux-input@...r.kernel.org

Thanks.

-- 
Dmitry
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/