Re: [PATCH] spi: xilinx: Add DT support for selecting transfer word width

[Geert Uytterhoeven <geert@xxxxxxxxxxxxxx>]

Hi Alvaro,

On Tue, Oct 22, 2019 at 12:38 PM Alvaro Gamez Machado
<alvaro.gamez@xxxxxxxxxx> wrote:
> On Tue, Oct 22, 2019 at 11:20:07AM +0100, Mark Brown wrote:
> > On Tue, Oct 22, 2019 at 11:00:07AM +0200, Alvaro Gamez Machado wrote:
> >
> > >             of_property_read_u32(pdev->dev.of_node, "xlnx,num-ss-bits",
> > >                                       &num_cs);
> > > +           ret = of_property_read_u32(pdev->dev.of_node,
> > > +                                      "xlnx,num-transfer-bits",
> > > +                                      &bits_per_word);
> > > +           if (ret)
> > > +                   bits_per_word = 8;
> >
> > Any new DT property needs documentation adding but in any case this
>
> Oh, ok. If this patch is of any interest, then I should provide changes to
> the appropriate file under Documentation/, is that right?
>
> I was preparing and testing a second patch to add "spi-bits-per-word"
> property to children of the SPI driver, similar to the spi-max-frequency
> property and others alike, to fully support AXI Quad SPI core with different
> word widths. This also modifies the DT, so I guess it'd be better to send a
> single patch that comprises all these changes alongside the DT
> documentation. Would that be correct procedure?

Is this "spi-bits-per-word" a property of the SPI slave device?
If yes, it may be better to hardcode it in the SPI slave device driver,
as it is fixed for that type of SPI slave.

If not, perhaps I'm misunderstanding the purpose.

> > shouldn't be set in DT, it should be set by the client driver - it's
> > going to be a fixed property of the device, not something that varies
> > per system.
>
> But this is in fact something that changes per system. When instantiating an
> AXI Quad SPI core in a HDL design that's exactly one of the options you can
> provide. In fact, in the board I'm working with right now, I'm instantiating
> two SPI cores, one of them with 8 bits per word but the other one requires
> 32 bits per word, as the devices it's going to talk to have this
> requirement.

So you're instantiating two variants of the same "xlnx,axi-quad-spi-1.00.a"
controller, with different options?
While you could add an "xlnx,foo" DT property for that, an alternative could
be to introduce a new compatible value.
It all depends on how many options there are when instantiating such a
controller.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds