On 27.06.22 04:18, Edward O'Callaghan wrote:
> On Sun, 26 Jun 2022 at 10:10, Nico Huber <nico.h@gmx.de> wrote:
>
>> Hi Anastasia,
>>
>
> Probably not suitable to laser Anastasia in particular. Peter owns the i2c
> set of drivers.

What's 'laser'? Peter didn't ask, Anastasia did.

>> I've seen you put some questions about the programmer tickets[1-4] on
>> the meeting agenda. I hope I can provide some answers already. Looking
>> at the tickets, I see there are already lists of things to do. But you
>> ask what is left to do. So somewhere seems to be a misunderstanding.
>>
>
> Maybe a good idea to clarify within the ticket the specific in order to
> keep the owner of the ticket engaged with the issue and not have the
> dialogue fragmented across tracker and mailing list. The issues here do not
> seem to be fundamentally design discussion, rather just nits and
> clarifications.

Sounds good, please do.

>>
>> * raiden_debug_spi: Seems ok. I think this one was really only missing
>>   a manpage entry. Now that it has some context, I guess an independent
>>   developer could actually review it now. ^^
>>
>> * lspcon_i2c_spi & realtek_mst_i2c_spi: There's a misunderstanding I
>>   guess. The tickets say "Document ... programmer". But they list actual
>>   problems. My idea when I wrote the long lists of issues was not to
>>   document issues. But, hopefully, to get them addressed, in code.
>>
>
> Comment on the bug to re-focus it to what you want? I believe Anastasia
> opened these bugs to capture your initial thoughts in the new issue tracker
> and not completely understand the full scope from the get go from a bullet
> point list.
>

Well, if you don't find the time necessary to look into it deep enough
to understand the requirements of flashrom, maybe you can find another
developer at Google who can? Maybe somebody with more experience who
gets their quicker. Or just get somebody else paid?

>>   We could also document the issues, e.g. by cluttering the code with
>>   FIXME comments. This might at least slow the spreading of such code
>>   down. But I would have to ask, why have these drivers on the master
>>   branch? In one of the tickets it even says the driver is not used
>>   in production. Does that mean it's abandoned?
>>
>
> No they are not abandoned, this has been answered a number of times now so
> I am unclear why the looping of this idea keeps happening? Drivers have an
> owner, Peter, these are used for debug and are of interest to the wider
> community for people who want to access this hardware.

I don't care what role people play during their dayjob. It's not my job
to keep track of these things. Also what exactly does it include being
an owner? I don't know that. Does it include keeping the code tidy? If
so, what's the role of the person who tells owners how to do their job
and who is that for Peter?

There's a reason why I don't like this "that's the responsible person"
pointing. In my experience with coreboot, it turned out that they are
never paid to do it anyway, because they are already busy with some-
thing else. If you want them to do something after the code is merged,
you need to pay them to work proactively on it and make it a top prio-
rity. Otherwise, the community might pick up the work, or hate you be-
cause they are already too upset.

>
> I think the OWNERS file discussed could help remind who to contact and make
> the bug owner for a specific issue.
>
>
>>
>>   Like I mentioned elsewhere, many of the issues could be mitigated by
>>   going through the board_enabled infrastructure. This was also men-
>>   tioned in the review of the mediatek_i2c_spi driver. All these I2C
>>   drivers could share a similar design, AFAICT.
>>
>
> I think this was already answered before. board_enable was designed around
> PCIe and similar, right now in its current API form it is insufficiently
> extensible to trivially allow for i2c<->board pairing. Agree this
> definitely needs core flashrom development work however does not seem like
> a driver author burden but rather a collective make flashrom have more
> extensible and robust core API missing.
>
> We should open some bugs here to try and drive towards a better
> board_enable future certainly!

Why we? You bypassed the process (development guidelines clearly say to
discuss things first to avoid frustration). So not we but you should.
And we shouldn't have to wait for you (which we already did for years
by now). In my experience with you, Edward, you procrastinate no matter
what. It's really years already! Please wake up.

>>   One of the manpage entries also revealed a new problem: I think it
>>   implies that after running `flashrom -p realtek_mst_i2c_spi:bus=0 -E`
>>   the device would be broken after a reset. That's unacceptable for
>>   flashrom. Another sign that it should go through the infrastructure
>>   of the internal programmer.
>>
>
> Seems like a bug for the bug tracker and assign the driver owner to take
> care of it?

Well, if you don't want to fix it now, please go ahead.

>>
>>   Documentation for the hardware interface would also be very welcome.
>>   Basically, everything that one needs to understand the code (and can't
>>   be derived from flashrom sources) should be publicly documented.
>>   I think we should make this a rule for vendor additions.
>>
>
> This was discussed at length.

No, you talked at length.

> Once again, documentation may not even exist
> at all in the first place.

But you can write it.

> This fundamentally assumes the vendor provides
> such details at all.

I guess you mean the silicon vendor, in that case: Not at all.
Details can be figured out. Reverse engineering, testing how the
hardware behaves, ...

> Agree, we should document better however one can only
> document what one actually knows for sure.

So you are saying the authors of your drivers knew nothing at all?

> Free software support for
> hardware has a long history of this problem,

Why does it only bother this project now then? I somewhat hate
saying it, but it seems you need to hear it: We had much less of
such problems before Google came.

> it is unreasonable to point
> the finger to have this paradise world where silicon vendors are so
> forthcoming, in practice we live in a very mixed world where the developer
> works things out and does their best to document what they know at the time.

So where is the documentation? Or are you saying the best they can
do is nothing? I'm confused.

Also, why blame silicon vendors for everything? They don't want your
code in the flashrom repo, you want it. I feel like there's a deep
misunderstanding. Maybe it's this: When I say "vendor", I mean the
company that designs products, puts their name and brands on it,
produces the software for them and makes money with them, e.g. Google.
Maybe I just used the wrong word.

>
> Although, a bug to document how the driver seems to operate in a
> documentation/ directory has been discussed so I think we could get that
> moving.
>
>
>>
>> * mediatek_i2c_spi: What I have to say about this, I did on Gerrit.
>>   What happened is clearly against our development guidelines. It's
>>   a copy of another driver, but we don't have the time to look into
>>   it so let the community take care of it? WTF?
>>
>
> Seldom have I seen someone developing hardware support for the first time
> not copy something similar and use that as a starting point.

Not similar, it's for the same hardware / IP block IIRC. Did you
forget already?

> What precisely
> is against development guidelines? If you can be more concrete and not say
> "WTF" it could be more helpful.

This is the part that I had in mind:

  We try to reuse as much code as possible and create new files only if
  absolutely needed, so if you find a function somewhere in the tree
  which already does what you want (even if it is for a totally
  different chip), please use it. See also Command set secrets.

The part about discussing first applies too of course. I don't think
there is much in our coding guidelines that doesn't disagree.

Nico
>> [1] https://ticket.coreboot.org/issues/358
>> [2] https://ticket.coreboot.org/issues/360
>> [3] https://ticket.coreboot.org/issues/361
>> [4] https://ticket.coreboot.org/issues/376