Messages in this thread | | | Date | Tue, 8 Jan 2019 10:12:44 -0800 | From | Moritz Fischer <> | Subject | Re: [PATCH v2] platform/chrome: Add cros_ec_readmem() helpers for I2C/SPI based ECs |
| |
Hi,
On Tue, Jan 08, 2019 at 04:00:58PM +0100, Enric Balletbo i Serra wrote: > Hi, > > On 8/1/19 15:34, Guenter Roeck wrote: > > On Tue, Jan 8, 2019 at 6:28 AM Enric Balletbo i Serra > > <enric.balletbo@collabora.com> wrote: > >> > >> Hi Moritz, > >> > >> Many thanks for the patch,I've one concern on this, and I'd be also interested > >> on Benson and Guenter opinion ... > >> > >> On 8/1/19 4:56, Moritz Fischer wrote: > >>> From: Moritz Fischer <mdf@kernel.org> > >>> > >>> Add cros_ec_readmem() based helpers for I2C/SPI based ECs.
That sentence also should probably get reworked on my end :) > >>> Unlike the LPC based ECs the I2C/SPI based ones cannot just directly > >>> read the mapped region. > >>> > >>> This is useful for things like accessing fan speeds or temperatures. > >>> > >>> Signed-off-by: Moritz Fischer <mdf@kernel.org> > >>> --- > >>> > >>> Hi all, > >>> > >>> > >>> I had submitted this back in May 2017 [1] and then somewhat forgot > >>> about it. So here's a new version :) > >>> > >>> As to why is this required? we're using this in some of our devices > >>> [2] that run a modified firmware based on Chromium-EC. > >>> I've been carrying the patch downstream in our tree for a while and > >>> it would be nice if we can merge this upstream so I can stop rebasing > >>> this :) > >>> > >> > >> Right, if we merge this you'll probably reduce your downstream patches but > >> actually, if I am not wrong, there is no user for this. There isn't an upstream > >> driver that uses the new functions so we will end up having upstream dead code. > >> IMO if you want to have this upstream you should also upstream the drivers that > >> use that code. Makes sense?
Sure, see below. > >> > > > > He has a hwmon driver which I think uses it. > > Nice, sorry Moritz because the hwmon driver was not on my radar, I missed it. > The patch needs some rework I guess so would be nice have all together in the > same patch series.
My fault, sorry, the reason there is that we have a bunch of versions of this driver internally (some of them thermal drivers, some of the hwmon drivers). All of them are open source and in our meta-ettus yocto layer, just need cleanup on our end first.
> > > Question would rather be > > if that is an acceptable use or if it exposes EC internals, ie non-API > > data. Comments, anyone ? > > > > Yes, that's the question. We have similar command for devices that use the lpc > bus. On my side I'll take a deeper look. More Comments ?
Yeah basically one comment from my side: The cros_ec_readmem only exposes a bunch of 'virtual' registers on I2C/SPI based ECs so it's not like you're leaking non-API data, but merely brings functionality for I2C/SPI based ECs to parity :)
Thanks for considering, I'll take another look at the Kbuild issue
Moritz
| |