From: "Mahapatra, Amit Kumar" <amit.kumar-mahapatra@amd.com>
To: Mark Brown <broonie@kernel.org>,
Amit Kumar Mahapatra <amit.kumar-mahapatra@xilinx.com>
Cc: "p.yadav@ti.com" <p.yadav@ti.com>,
"miquel.raynal@bootlin.com" <miquel.raynal@bootlin.com>,
"richard@nod.at" <richard@nod.at>,
"vigneshr@ti.com" <vigneshr@ti.com>,
"git@xilinx.com" <git@xilinx.com>,
"michal.simek@xilinx.com" <michal.simek@xilinx.com>,
"linux-spi@vger.kernel.org" <linux-spi@vger.kernel.org>,
"linux-arm-kernel@lists.infradead.org"
<linux-arm-kernel@lists.infradead.org>,
"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
"michael@walle.cc" <michael@walle.cc>,
"linux-mtd@lists.infradead.org" <linux-mtd@lists.infradead.org>,
"Mahapatra, Amit Kumar" <amit.kumar-mahapatra@amd.com>,
"Mahapatra, Amit Kumar" <amit.kumar-mahapatra@amd.com>,
"git (AMD-Xilinx)" <git@amd.com>
Subject: RE: [RFC PATCH 1/2] spi: Add multiple CS support for a single SPI device
Date: Thu, 23 Jun 2022 11:39:19 +0000 [thread overview]
Message-ID: <DM6PR12MB2809F6C7D80B60556218D627DCB59@DM6PR12MB2809.namprd12.prod.outlook.com> (raw)
In-Reply-To: <YqHfccvhy7e5Bc6m@sirena.org.uk>
Hello Mark,
> -----Original Message-----
> From: linux-arm-kernel <linux-arm-kernel-bounces@lists.infradead.org> On
> Behalf Of Mark Brown
> Sent: Thursday, June 9, 2022 5:24 PM
> To: Amit Kumar Mahapatra <amit.kumar-mahapatra@xilinx.com>
> Cc: p.yadav@ti.com; miquel.raynal@bootlin.com; richard@nod.at;
> vigneshr@ti.com; git@xilinx.com; michal.simek@xilinx.com; linux-
> spi@vger.kernel.org; linux-arm-kernel@lists.infradead.org; linux-
> kernel@vger.kernel.org; michael@walle.cc; linux-mtd@lists.infradead.org
> Subject: Re: [RFC PATCH 1/2] spi: Add multiple CS support for a single SPI
> device
>
> On Mon, Jun 06, 2022 at 04:56:06PM +0530, Amit Kumar Mahapatra wrote:
>
> > ---
> > drivers/spi/spi-zynqmp-gqspi.c | 30 ++++++++++++++++++++++++++----
> > drivers/spi/spi.c | 10 +++++++---
> > include/linux/spi/spi.h | 10 +++++++++-
> > 3 files changed, 42 insertions(+), 8 deletions(-)
>
> Please split the core and driver support into separate patches, they are
> separate things.
Ok, I will split the patches.
>
> > --- a/drivers/spi/spi.c
> > +++ b/drivers/spi/spi.c
> > @@ -2082,6 +2082,8 @@ static int of_spi_parse_dt(struct spi_controller
> > *ctlr, struct spi_device *spi, {
> > u32 value;
> > int rc;
> > + u32 cs[SPI_CS_CNT_MAX];
> > + u8 idx;
> >
> > /* 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.
https://github.com/torvalds/linux/blob/de5c208d533a46a074eb46ea17f672cc005a7269/Documentation/devicetree/bindings/mtd/jedec%2Cspi-nor.yaml#L49
An example of a flash node with two CS has been added in spi-controller.yaml
https://github.com/torvalds/linux/blob/de5c208d533a46a074eb46ea17f672cc005a7269/Documentation/devicetree/bindings/spi/spi-controller.yaml#L141
>
> > + /* Bit mask of the chipselect(s) that the driver
> > + * need to use form the chipselect array.
> > + */
> > + u8 cs_index_mask : 2;
>
> Why make this a bitfield?
https://github.com/torvalds/linux/blob/de5c208d533a46a074eb46ea17f672cc005a7269/Documentation/devicetree/bindings/mtd/jedec%2Cspi-nor.yaml#L49
As per the DT bindings we are supporting max 2 chip selects per SPI device
that is the reason I had taken it as an bitfield of 2. But now I think that in
future when the number of chip selects per device would increase i.e.,
more than 2, then we have to again increase the bitfield allocation for
accommodating the increase in the number of chip selects per SPI device,
So I think it's better to drop the bitfield for now and use cs_index_mask
as an u8
>
> 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.
As per the current implementation all the drivers fetch their chip select value form
spi->chip_select, but now the driver code needs to be modified to fetch the value
from spi->chip_select[0] instead and by this approach we do not need to check if the
driver supports single or multiple CS.
Hope I addressed all your concerns and please let us know what you think.
Regards,
Amit
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/
next prev parent reply other threads:[~2022-06-23 11:40 UTC|newest]
Thread overview: 15+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-06-06 11:26 [RFC PATCH 0/2] spi: Add support for stacked/parallel memories Amit Kumar Mahapatra
2022-06-06 11:26 ` [RFC PATCH 1/2] spi: Add multiple CS support for a single SPI device Amit Kumar Mahapatra
2022-06-09 11:54 ` Mark Brown
2022-06-23 11:39 ` Mahapatra, Amit Kumar [this message]
2022-06-23 12:06 ` Mark Brown
2022-07-15 15:35 ` Mahapatra, Amit Kumar
2022-07-15 15:54 ` Mark Brown
2022-07-19 13:21 ` Mahapatra, Amit Kumar
2022-07-19 17:53 ` Mark Brown
2022-07-27 13:02 ` Mahapatra, Amit Kumar
2022-07-11 12:47 ` Michal Simek
2022-07-11 14:52 ` Mark Brown
2022-07-15 15:36 ` Mahapatra, Amit Kumar
2022-07-15 16:03 ` Mark Brown
2022-06-06 11:26 ` [RFC PATCH 2/2] mtd: spi-nor: Add support for stacked/parallel memories Amit Kumar Mahapatra
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=DM6PR12MB2809F6C7D80B60556218D627DCB59@DM6PR12MB2809.namprd12.prod.outlook.com \
--to=amit.kumar-mahapatra@amd.com \
--cc=amit.kumar-mahapatra@xilinx.com \
--cc=broonie@kernel.org \
--cc=git@amd.com \
--cc=git@xilinx.com \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mtd@lists.infradead.org \
--cc=linux-spi@vger.kernel.org \
--cc=michael@walle.cc \
--cc=michal.simek@xilinx.com \
--cc=miquel.raynal@bootlin.com \
--cc=p.yadav@ti.com \
--cc=richard@nod.at \
--cc=vigneshr@ti.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).