linux-kernel - Re: [PATCH V5 2/2] mfd: stmpe: Update DT support in stmpe driver

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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <20121206101131.GP2718@gmail.com>
Date:	Thu, 6 Dec 2012 10:11:31 +0000
From:	Lee Jones <lee.jones@...aro.org>
To:	Viresh Kumar <viresh.kumar@...aro.org>
Cc:	Grant Likely <grant.likely@...retlab.ca>,
	Samuel Ortiz <sameo@...ux.intel.com>,
	rabin.vincent@...ricsson.com, shiraz.hashim@...com,
	devicetree-discuss@...ts.ozlabs.org, linux-kernel@...r.kernel.org,
	spear-devel@...t.st.com, linus.walleij@...aro.org,
	Vipul Kumar Samar <vipulkumar.samar@...com>
Subject: Re: [PATCH V5 2/2] mfd: stmpe: Update DT support in stmpe driver

On Thu, 06 Dec 2012, Viresh Kumar wrote:

> On 6 December 2012 15:20, Lee Jones <lee.jones@...aro.org> wrote:
> >> > But regardless, it is the responsiblity of the probe function to go and
> >> > look if of_driver_match_device() matches against anything if it cares
> >> > about the of_match_table entries (for instance, if there is extra data
> >> > attached).
> >>
> >> Ok, so filling .data field in of_device_id[] is not required for our case as
> >> we aren't doing anything special in our drivers.
> >
> > This is exactly my point, and the reason I bought it up in the
> > first place. Normally when you specify an ID table and populate
> > the .data attribute, you parse for it in the code and then cast
> > it back to some kind of useful data. However, you're not doing
> > that, which is precisely why I wondered if the table was
> > necessary at all. In all my testing, the DT portion worked and
> > the correct STMPE chip was identified without it.
> 
> Probably Vipul (Author of this patch), copied it from existing i2c/spi
> clients, which have also added this blindly :)
> 
> > So, are you adding the table for good reason, or because you
> > think it's the right thing to do?
> 
> I would be keeping the table as that's the right thing to do. 

So then I'm back to my original question, why?

What is it used for? What difference does it make?

I could understand if the .data attribute was used in the driver
to make vital decisions based on STMPE version, but it's not. So
why are we burdening the driver with unused code that's not
required? Other than to get your mainlined patch-count up of
course? This has an air of "placing header files in alphabetical
order" about it. ;)

> By chance
> our non-DT and DT tables had a difference of "st," only in the name
> of instances and so it worked without tables. Otherwise it couldn't
> have worked.
> 
> Over that, i am looking to bring the "stmpe,id" binding back again (unless
> you have a better option), as device name is not coming from DT currently,
> which we discussed earlier.

Or you could not put unnecessary bindings into the Device Tree
by putting two and two together and realise that using the table
is the correct thing to do instead. This actually gives reason
to you previous patch, but should probably be fixed-up into it
so it has some proper meaning/purpose. ;)

-- 
Lee Jones
Linaro ST-Ericsson Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
--
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/