Re: [PATCH 07/10] spi: sh-msiof: Add support for R-Car H2 and M2

[Magnus Damm <magnus.damm@xxxxxxxxx>]

On Thu, Feb 20, 2014 at 11:43 PM, Geert Uytterhoeven
<geert@xxxxxxxxxxxxxx> wrote:
> From: Geert Uytterhoeven <geert+renesas@xxxxxxxxxxxxxx>
>
> Add support for the MSIOF variant in the R-Car H2 (r8a7790) and M2
> (r8a7791) SoCs.
>
> Binding documentation:
>   - Add future-proof "renesas,msiof-<soctype>" compatible values,
>   - Add example bindings.
>
> Implementation:
>   - MSIOF on R-Car H2 and M2 requires the transmission of dummy data if
>     data is being received only (cfr. "Set SICTR.TSCKE to 1" and "Write
>     dummy transmission data to SITFDR" in paragraph "Transmit and Receive
>     Procedures" of the Hardware User's Manual).
>   - As RX depends on TX, MSIOF on R-Car H2 and M2 also lacks the RSCR
>     register (Receive Clock Select Register), and some bits in the RMDR1
>     (Receive Mode Register 1) and TMDR2 (Transmit Mode Register 2)
>     registers.
>   - Use the recently introduced SPI_MASTER_MUST_TX flag to enable support
>     for dummy transmission in the SPI core, and to differentiate from other
>     MSIOF implementations in code paths that need this.
>   - New DT compatible values ("renesas,msiof-r8a7790" and
>     "renesas,msiof-r8a7791") are added, as well as new platform device
>     names ("spi_r8a7790_msiof" and "spi_r8a7791_msiof").
>   - Hardware features are indicated using a new struct sh_msiof_chipdata,
>     which is used for both DT and legacy binding. For now this contains the
>     SPI master flags only.
>
> This is loosely based on a set of patches from Takashi Yoshii
> <takasi-y@xxxxxxxxxxxxx>.
>
> Signed-off-by: Geert Uytterhoeven <geert+renesas@xxxxxxxxxxxxxx>
> Cc: Takashi Yoshii <takasi-y@xxxxxxxxxxxxx>
> Cc: devicetree@xxxxxxxxxxxxxxx
> ---
>  Documentation/devicetree/bindings/spi/sh-msiof.txt |   21 +++++++-
>  drivers/spi/spi-sh-msiof.c                         |   57 ++++++++++++++++----
>  2 files changed, 66 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/spi/spi-sh-msiof.c b/drivers/spi/spi-sh-msiof.c
> index 92515c1ececa..31624fb4997d 100644
> --- a/drivers/spi/spi-sh-msiof.c
> +++ b/drivers/spi/spi-sh-msiof.c
> @@ -659,6 +671,23 @@ static u32 sh_msiof_spi_txrx_word(struct spi_device *spi, unsigned nsecs,
>  }
>
>  #ifdef CONFIG_OF
> +static const struct sh_msiof_chipdata sh_data = {
> +       .master_flags = 0,
> +};
> +
> +static const struct sh_msiof_chipdata r8a779x_data = {
> +       .master_flags = SPI_MASTER_MUST_TX,
> +};
> +
> +static const struct of_device_id sh_msiof_match[] = {
> +       { .compatible = "renesas,sh-msiof",        .data = &sh_data },
> +       { .compatible = "renesas,sh-mobile-msiof", .data = &sh_data },
> +       { .compatible = "renesas,msiof-r8a7790",   .data = &r8a779x_data },
> +       { .compatible = "renesas,msiof-r8a7791",   .data = &r8a779x_data },
> +       {},
> +};
> +MODULE_DEVICE_TABLE(of, sh_msiof_match);

Hi Geert,

Thanks for your patches. They all look good in general I think.

On thing stuck out a bit with the bindings. I can see that you specify
both fifo size and use the SoC suffix for the r8a7790 and r8a7791
bindings. Isn't that a bit of redundant information there, if we know
that the SoC is r8a7790 or r8a7791 then can't we simply put that
information in r8a779x_data above and perhaps keep the binding
simpler? Perhaps same thing applies to other properties as well?

Cheers,

/ magnus
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html