RE: [RFC PATCH 1/2] spi: Add multiple CS support for a single SPI device

[Mahapatra, Amit Kumar]

Hello Mark,

> -----Original Message-----
> From: Mark Brown <broonie@xxxxxxxxxx>
> Sent: Thursday, June 23, 2022 5:37 PM
> To: Mahapatra, Amit Kumar <amit.kumar-mahapatra@xxxxxxx>
> Cc: Amit Kumar Mahapatra <amit.kumar-mahapatra@xxxxxxxxxx>;
> p.yadav@xxxxxx; miquel.raynal@xxxxxxxxxxx; richard@xxxxxx;
> vigneshr@xxxxxx; git@xxxxxxxxxx; michal.simek@xxxxxxxxxx; linux-
> spi@xxxxxxxxxxxxxxx; linux-arm-kernel@xxxxxxxxxxxxxxxxxxx; linux-
> kernel@xxxxxxxxxxxxxxx; michael@xxxxxxxx; linux-mtd@xxxxxxxxxxxxxxxxxxx;
> git (AMD-Xilinx) <git@xxxxxxx>
> Subject: Re: [RFC PATCH 1/2] spi: Add multiple CS support for a single SPI
> device
> 
> On Thu, Jun 23, 2022 at 11:39:19AM +0000, Mahapatra, Amit Kumar wrote:
> 
> > > >  	/* Mode (clock phase/polarity/etc.) */
> > > >  	if (of_property_read_bool(nc, "spi-cpha"))
> 
> > > This is changing the DT binding but doesn't have any updates to the
> > > binding document.  The binding code also doesn't validate that we
> > > don't have too many chip selects.
> 
> > The following updates are done in the binding documents for adding
> > multiple CS support:
> > In jedec,spi-nor.yaml file " maxItems " of the "reg" DT property has
> > been updated to accommodate two CS per SPI device.
> 
> This is a change to a binding for a specific driver, this is changing the SPI
> core.
> 
> > > I'm also not seeing anything here that checks that the driver
> > > supports multiple chip selects - it seems like something that's
> > > going to cause issues and we should probably have something to handle
> that situation.
> 
> > In my approach the chip select member (chip_select) of the spi_device
> > structure is changed to an array (chip_select[2]). This array is used
> > to store the CS values coming from the "reg" DT property.
> > In case of multiple chip selects  spi->chip_slect[0] will hold CS0
> > value &
> > spi->chip_select[1] wil hold CS1 value.
> > In case of single chip select the spi->chip_select[0] will hold the chip select
> value.
> 
> That doesn't address the issue, the issue is checking that the driver can
> support multiple chip selects.

To address this issue, in spi core we will check the number of items 
in the "reg" property of the flash node(which is nothing but the 
number of chip selects) against the "num-cs" property of the spi 
controller(which is total number of chip selects supported by the 
controller). If the number of items mentioned in the "reg" property 
is greater than "num-cs" value then we error out.

For eg.,

rc = of_property_read_variable_u32_array(nc, "reg", &cs[0], 1, 
						SPI_CS_CNT_MAX);
if(rc > ctlr->num_chipselect) {
	dev_err(&ctlr->dev, "%pOF has invalid 'reg' property (%d)\n", 
							nc, rc);
	return -EINVAL;
}

Please let us know if you have any other approach in mind.

Regards,
Amit