On Fri, Feb 04, 2022 at 03:59:36PM +0530, Krishna Yarlagadda wrote:
> + /* Process individual transfer list */
> + list_for_each_entry(xfer, &msg->transfers, transfer_list) {
> + if (transfer_phase == CMD_TRANSFER) {
> + } else if (transfer_phase == ADDR_TRANSFER) {
> + } else {
Looks like you're writing a switch statement here...
> + /* X1 SDR mode */
> + cmd_config = tegra_qspi_cmd_config(false, 0,
> + xfer->len);
> + cmd_value = *((const u8 *)(xfer->tx_buf));
> +
> + len = xfer->len;
> + /* X1 SDR mode */
> + addr_config = tegra_qspi_addr_config(false, 0,
> + xfer->len);
> + address_value = *((const u32 *)(xfer->tx_buf));
> + /* Program Command, Address value in register */
> + tegra_qspi_writel(tqspi, cmd_value, QSPI_CMB_SEQ_CMD);
> + tegra_qspi_writel(tqspi, address_value,
> + QSPI_CMB_SEQ_ADDR);
> + /* Program Command and Address config in register */
> + tegra_qspi_writel(tqspi, cmd_config,
> + QSPI_CMB_SEQ_CMD_CFG);
> + tegra_qspi_writel(tqspi, addr_config,
> + QSPI_CMB_SEQ_ADDR_CFG);
It looks like the command and address have to be specific lengths? If
that's the case then
> + if (cdata->is_cmb_xfer && transfer_count == 3)
> + ret = tegra_qspi_combined_seq_xfer(tqspi, msg);
> + else
> + ret = tegra_qspi_non_combined_seq_xfer(tqspi, msg);
This check needs to be more specific. But like I said in reply to the
binding patch I don't see why we can't just pattern match on the data
without requiring a property here, we'd need to check that the message
is suitable no matter what.
> -----Original Message-----
> From: Mark Brown <[email protected]>
> Sent: 04 February 2022 19:39
> To: Krishna Yarlagadda <[email protected]>
> Cc: [email protected]; Jonathan Hunter <[email protected]>;
> [email protected]; [email protected]; Sowjanya Komatineni
> <[email protected]>; Laxman Dewangan <[email protected]>;
> [email protected]; [email protected]; linux-
> [email protected]; [email protected]
> Subject: Re: [PATCH 6/6] spi: tegra210-quad: combined sequence mode
>
> On Fri, Feb 04, 2022 at 03:59:36PM +0530, Krishna Yarlagadda wrote:
>
> > + /* Process individual transfer list */
> > + list_for_each_entry(xfer, &msg->transfers, transfer_list) {
> > + if (transfer_phase == CMD_TRANSFER) {
>
> > + } else if (transfer_phase == ADDR_TRANSFER) {
>
> > + } else {
>
> Looks like you're writing a switch statement here...
Yes. This can be switch statement.
>
> > + /* X1 SDR mode */
> > + cmd_config = tegra_qspi_cmd_config(false, 0,
> > + xfer->len);
> > + cmd_value = *((const u8 *)(xfer->tx_buf));
> > +
> > + len = xfer->len;
>
> > + /* X1 SDR mode */
> > + addr_config = tegra_qspi_addr_config(false, 0,
> > + xfer->len);
> > + address_value = *((const u32 *)(xfer->tx_buf));
>
> > + /* Program Command, Address value in register */
> > + tegra_qspi_writel(tqspi, cmd_value,
> QSPI_CMB_SEQ_CMD);
> > + tegra_qspi_writel(tqspi, address_value,
> > + QSPI_CMB_SEQ_ADDR);
> > + /* Program Command and Address config in register
> */
> > + tegra_qspi_writel(tqspi, cmd_config,
> > + QSPI_CMB_SEQ_CMD_CFG);
> > + tegra_qspi_writel(tqspi, addr_config,
> > + QSPI_CMB_SEQ_ADDR_CFG);
>
> It looks like the command and address have to be specific lengths? If that's the
> case then
Cmd and address are configurable to a limit. Will add min and max check.
>
> > + if (cdata->is_cmb_xfer && transfer_count == 3)
> > + ret = tegra_qspi_combined_seq_xfer(tqspi, msg);
> > + else
> > + ret = tegra_qspi_non_combined_seq_xfer(tqspi, msg);
>
> This check needs to be more specific. But like I said in reply to the binding
> patch I don't see why we can't just pattern match on the data without requiring
> a property here, we'd need to check that the message is suitable no matter
> what.
There is no real-world use case we encountered so far preventing us stick to pattern.
But this was to avoid any corner case where there could be 3 different transfers sent in single msg.
On Mon, Feb 07, 2022 at 02:54:00PM +0000, Krishna Yarlagadda wrote:
> > > + if (cdata->is_cmb_xfer && transfer_count == 3)
> > > + ret = tegra_qspi_combined_seq_xfer(tqspi, msg);
> > > + else
> > > + ret = tegra_qspi_non_combined_seq_xfer(tqspi, msg);
> > This check needs to be more specific. But like I said in reply to the binding
> > patch I don't see why we can't just pattern match on the data without requiring
> > a property here, we'd need to check that the message is suitable no matter
> > what.
> There is no real-world use case we encountered so far preventing us stick to pattern.
> But this was to avoid any corner case where there could be 3 different transfers sent in single msg.
So you'll remove the property and just pattern match on the message?
Please fix your mail client to word wrap within paragraphs at something
substantially less than 80 columns. Doing this makes your messages much
easier to read and reply to.