Re: [PATCH v2 1/3] regmap: add Intel SPI Slave to AVMM Bus Bridge support

[Xu Yilun <yilun.xu@intel.com>]

On Wed, Jul 22, 2020 at 12:32:02PM +0100, Mark Brown wrote:
> On Tue, Jul 21, 2020 at 01:11:31AM +0800, Xu Yilun wrote:
> > On Fri, Jul 17, 2020 at 07:15:08PM +0100, Mark Brown wrote:
> 
> > > there looks to be a lot of abstraction layers simply within the software
> > > here which make things hard to follow.  At the very least there doesn't
> 
> > We need to follow the 3 layers protocol for register accessing. On SPI
> > slave side, spi-avmm HW finishes the whole protocol encoding/decoding
> > work. But on host side, no dedicated IP block is designed. So host need
> > to handle all the protocol work by SW. This introduces the complexity of
> > the driver.
> 
> > We don't introduce any extra software layers, just follows the
> > definition of 3 layers in HW spec. But I know reviewing the detail of the
> > protocol is tedious. Maybe I should put more comments about what part of
> > SPEC should be referenced for some piece of code. Hope this makes the
> > review work easier.
> 
> You don't need to reflect all the layers that the system has in the
> software, if some of the layers are always used together with no
> possibility of replacing them then it can make sense to collapse them in
> software.  That did seem to be the case here.

OK. I understand your point.

> 
> > > > +config REGMAP_SPI_AVMM
> > > > +	tristate
> > > > +	depends on SPI
> 
> > > Note that for selected symbols dependencies don't take effect.
> 
> > I see. So should I change something here? I see the same case for other
> > regmap options.
> 
> It's fine, just be aware that you still need to have appropriate
> dependencies and selects in the user.

OK. Thanks for the reminding.

> 
> > > > +/* The input is a trans stream in trans_buf, format a phy stream in phy_buf. */
> > > > +static void pkt_phy_tx_prepare(char *trans_buf, unsigned int trans_len,
> > > > +			       char *phy_buf, unsigned int *phy_len)
> > > > +{
> > > > +	unsigned int i;
> > > > +	char *b, *p;
> > > > +
> > > > +	b = trans_buf;
> > > > +	p = phy_buf;
> 
> > > I'm not seeing any bounds checking on the size of an operation in this
> > > function and I think I'd expect some, similarly for a lot of the other
> > > I/O operations.
> 
> > I have caculated and allocated 2 buffers that are large enough to
> > contain any possible data stream for regmap_bus.max_raw_read/write. See
> > the definition of TRANS_BUF_SIZE & PHY_BUF_SIZE.
> 
> > So maybe we don't have to check the length?
> 
> This isn't at all clear looking at the code, perhaps it would be clearer
> with fewer layers of abstraction but at the minute it's a lot of effort
> to confirm if it's safe.

OK. I'll add the check.

> 
> > > > +	if (spi->bits_per_word != 8 && spi->bits_per_word != 32)
> > > > +		return ERR_PTR(-EINVAL);
> 
> > > Most controllers support configurable bits per word (and modes) - you
> > > shouldn't be rejecting things based on whatever the default happens to
> > > be.
> 
> > I'm not sure if it is good that the spi_avmm_regmap_init changes the
> > configuration of spi devices silently.
> 
> That's the expected usage.
> 
> > In my current implementation, the spi device driver should be aware and
> > choose the right spi setup before trying to init the regmap. Would it be
> > too hard for other drivers to use it?
> 
> It'd be duplicated effort in all the users, if the only way to use the
> bus is with this configuration then simply saying that they're using the
> bus should be enough.

OK. I could add the spi_setup on init.