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

[Mark Brown <broonie@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 3150 bytes --]

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.

> > > +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.

> > > +/* 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.

> > > +	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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]