Re: [PATCH v2] platform/chrome: Add cros_ec_readmem() helpers for I2C/SPI based ECs

[Moritz Fischer]

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@xxxxxxxxxxxxx> 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@xxxxxxxxxx>
> >>>
> >>> 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@xxxxxxxxxx>
> >>> ---
> >>>
> >>> 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