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.
I don't care much if you call it a driver bug or a dtb bug if the above
happens. For sure the error checking that Oleksij introduces isn't
mandatory, but still it improves the driver.
How can you implement an improved error check?
You need the maximal range to check it, the hardware register
definition in the iMX25 has 2bits - with reserved bits above it.
You could reasonably guess that they have left space for up to 4 bits
to select the chip select. In any case even if you somewhat arbitrarily
limit the chip select to 2bits you still are not giving an error
for the hardware with a 1bit selection. Of if some devices do have
4bits for chip select and you set the range limit to 16 chip selects
then you won't produce an error for the case Oleksij presents above.
Regards
Greg
--
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
