Re: [PATCHv3 1/2] spi: imx: fix use of native chip-selects with devicetree

["Uwe Kleine-König" <u.kleine-koenig-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>]

Hello Greg,

On Thu, Aug 10, 2017 at 10:24:09PM +1000, Greg Ungerer wrote:
> On 10/08/17 21:49, Uwe Kleine-König wrote:
> > On Thu, Aug 10, 2017 at 09:35:15PM +1000, Greg Ungerer wrote:
> > > On 10/08/17 19:35, Vladimir Zapolskiy wrote:
> > > > Errors in DTB (or platform data) may confuse a driver and lead to runtime
> > > > misbehaviour. You describe an error in a board DTB, which is definitely
> > > > better to handle in the SPI driver, but I don't think it is strictly
> > > > mandatory to do it, because DTB errors are supposed to be fixed in DTB.
> > > > 
> > > > May be one day a formal check of DTBs against Documentation/devicetree
> > > > descriptions will be added and such DTB errors could be captured on DTB
> > > > compilation stage.
> > > 
> > > I completely agree with Vladmir here. Since "cs-gpios" defines the
> > > number of chip selects, as per the code you point out, it is the range
> > > limit. So if a DTB defines it wrongly then you can expect some things
> > > not to work right. The spi code quite rightly relies on the DTB
> > > definitions to be correct for proper operation.
> > > 
> > > 
> > > > > And in this case:
> > > > > cs-gpios = <&gpio1 0 0>, <&gpio1 1 0>, <&gpio1 2 0>, <&gpio1 3 0>, <0>;
> > > > > 
> > > > > we should produce a 3 bit value b100 which will be shifted left and
> > > > > "or"-ed with other ctrl bits.
> > > 
> > > So the register settings will be wrong and the device will not work.
> > > You can't really expect any other behavior from an incorrect DTB.
> > 
> > I think nobody expects that a wrong dtb is good enough to make
> > everything work. Maybe you can argue what should happen in a driver if
> > it gets a wrong dtb. It can make the kernel oops, just silently not work
> > or issue an error message; probably there are more options.
> > 
> > The current state is that a broken dtb makes the spi-imx driver write to
> > a unrelated register bit when asked to set a chip select signal. Oleksij
> > tries to improve that and make the driver error out instead. It makes it
> > easier for users to report problems or find the culprit themselves. I
> > like that.
> 
> But the check if the chip select is valid is based on information
> that comes from the DTB. In the example of the wrong cs-gpios list
> the DTB is saying it has more chip selects that the actual hardware
> device does. How can you possibly protect against that?
> 
> The current code seems to do its best to range check the chip select,
> based on what the DTB says this hardware has.

If I understood correctly the current state is the following:

The spi-imx driver supports GPIOs as CS and a "native" mode where the
hardware drives the CS line. If you write:

	cs-gpios = <&gpio2 0 0>, <0>, <&gpio1 17 0>

That means: Use gpio2.0 as CS 0, native CS as CS 1 and gpio1.17 as CS 2.

So what the driver can check here: Does the dtb request a native CS at a
position that is not supported by the hardware. Of course even if there
are only 4 hardware CS the driver can still be used with 7 spi devices
given that CS 4 .. 6 are using a GPIO as CS.

Oleksij: please correct me if I'm wrong.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html