Re: [PATCH 00/16] Add support for Lattice MachXO2 programming via I2C

From: Xu Yilun <yilun.xu@intel.com>
To: Ivan Bornyakov <i.bornyakov@metrotek.ru>
Cc: Sascha Hauer <sha@pengutronix.de>,
	j.zink@pengutronix.de, devicetree@vger.kernel.org,
	linux-fpga@vger.kernel.org, robh+dt@kernel.org, mdf@kernel.org,
	kernel@pengutronix.de, hao.wu@intel.com
Subject: Re: [PATCH 00/16] Add support for Lattice MachXO2 programming via I2C
Date: Fri, 26 Aug 2022 23:26:44 +0800	[thread overview]
Message-ID: <YwjmNAmfMSSTrbkS@yilunxu-OptiPlex-7050> (raw)
In-Reply-To: <20220826091912.xunn3cv3gwhktjib@h-e2.ddg>

On 2022-08-26 at 12:19:12 +0300, Ivan Bornyakov wrote:
> On Fri, Aug 26, 2022 at 12:00:44PM +0300, Ivan Bornyakov wrote:
> > On Fri, Aug 26, 2022 at 10:25:39AM +0200, Sascha Hauer wrote:
> > > Hi Ivan,
> > > 
> > > On Thu, Aug 25, 2022 at 06:25:14PM +0300, Ivan Bornyakov wrote:
> > > > Hi, Johannes!
> > > > 
> > > > I just came across your patches. Surprisingly, our work interferes.
> > > > 
> > > > I recently posted patch-series for configuring ECP5 and was asked to make
> > > > generalized sysCONFIG driver with support for both ECP5 and MachXO2, which
> > > > I did. Sadly I don't have hardware with MachXO2, but you clearly do :)
> > > > 
> > > > Please, take a look at
> > > > 
> > > > https://lore.kernel.org/linux-fpga/20220825112433.14583-1-i.bornyakov@metrotek.ru/
> > > > 
> > > > and please help test MachXO2 variant. When we pull this off, you may add I2C
> > > > interface on top.
> > > 
> > > You are adding a new driver for something we already have a driver for
> > > in the tree.
> > 
> > My intent was to add new driver and drop old one once the new driver is
> > proven to be working.
> > 
> > > The final goal should be that we only have a single driver
> > > for sysconfig based FPGAs in the tree.
> > 
> > It is.
> > 
> > > Johannes' series is a step in
> > > that direction: He cleans up the existing driver and starts abstracting
> > > out common sysconfig functions so that they can be used by both the I2C
> > > and SPI interface. He just told me that the abstraction is likely not
> > > enough to integrate ECP5 support right away, one reason being that the
> > > machxo2 has a flash whereas the ECP5 does not.
> > > 
> > > Unless you can explain why the existing driver is broken beyond repair
> > > I think we should rather incrementally improve the existing driver
> > > instead of adding a new one with a conflicting compatible.
> > 
> > Yeah, conflicting compatible was my oversight.
> > 
> 
> Wait! They are not conflicting :) The new one is "lattice,machxo2-fpga-mgr",
> the old one is "lattice,machxo2-slave-spi"
> 
> > > So despite you were in the room earlier I think you should rather base
> > > your work on Johannes' series.
> > 
> > We on par here. You guys didn't join ongoing work, I didn't rework
> > existing driver.

I didn't look into the code yet, so maybe some misunderstanding.

I'd rather we pay more attention on the code design for the FULL feature
of this sysCONFIG interface, rather than worrying about whose patches should
go first.

So just review patches for each other and collabrate for a well design,
then your features can be accepted faster.

Thanks,
Yilun

> > 
> > > 
> > > Just my 2 cents, maybe one of the maintainers has a few words on it.
> > > 
> > > Sascha
> > > 
> > > 
> > > -- 
> > > Pengutronix e.K.                           |                             |
> > > Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
> > > 31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
> > > Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |
> 