Re: [spi-devel-general] [PATCH 1/2] spi/spi_s3c64xx: Make probe morerobust against missing board config

[Jassi Brar]

On Sat, Aug 21, 2010 at 7:08 PM, Mark Brown
<broonie@xxxxxxxxxxxxxxxxxxxxxxxxxxx> wrote:
> On Sat, Aug 21, 2010 at 10:45:56AM +0900, Jassi Brar wrote:
>> On Sat, Aug 21, 2010 at 1:17 AM, Mark Brown
>> <broonie@xxxxxxxxxxxxxxxxxxxxxxxxxxx> wrote:
>> > The S3C64xx SPI driver requires the machine to call s3c64xx_spi_set_info()
>> > to select a few options, including the clock to use for the SPI controller.
>> > If this is not done then a NULL will be passed as the clock name for
>> > clk_get(), causing an obscure crash. Guard against this and other missing
>> > configuration by validating that the clock name has been filled in in
>> > the platform data that ets passed in.
>
>> Â Â ÂThe movement of sci assignment and check doesn't make any
>> difference because
>> Â Â Â we already check for presence of platform_data and DMA-Tx,Rx and
>> IO base is
>> Â Â Â set irrespective of calling s3c64xx_spi_set_info()
>
> While it does check for those things for at least the 6410 they're all
> unconditionally set up by dev-spi.c so the tests all pass and we make it
> down into to the clk_get() which then falls over horribly.
Those parameters are SoC specific and hence will be always available to
platform devices.
Clock selection and num_cs(number of slaves attached to the SPI bus) are
machine specific and hence responsibility of the machine developer to
set appropriately via s3c64xx_spi_set_info()


>> Â Â Also, I think !sci->num_cs might be an even better check because
>> the samsung clock
>> Â Âapi might be changed (IIRC Ben was already working it out) to make
>> it redundant
>> Â Âto pass clock name strings to clk_get. If that is the case, we might end up
>> Â Âadding another foolproof check like !sci->num_cs
>
> The problem with num_cs is that it gets interpreted by a custom function
> provided by the board driver so we really can't say anything about what
> it does.
num_cs is passed on as such to the SPI core to master->num_chipselect
which is strictly checked for before a master is created.

> ÂIf the clock API gets enhanced then we can always cope with
> things then, but given the pace of development there I'd expect that
> we'd need to continue checking for a while.
yes, so do i think.

> TBH I'm a bit surprised that the driver has to do custom stuff to
> support gpiolib chip selects - my first thought when I saw that stuff
> was that it seemed like something that lots of SPI controllers would be
> able to share but I didn't look at the overall SPI code for long enough
> to figure out what was going on there.
we have common spi bitbanging driver that takes only four gpio pins
and work like a charm.
In order to enable users use multiple CS per master, in spi_s3c64xx.c
the single CS feature provided by the controller is deliberately left unused
and the driver accepts a gpio per slave and manually controls it. Though
the callbacks and arguments are designed so that a simple gpio number
and gpio_set can be assigned by the machine code.

Anyways, I think we can keep the patch as it is, if it's already
applied in Grant's
tree.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/