On Fri, Dec 04, 2009 at 08:06:12PM +0900, jassi brar wrote:
> On Fri, Dec 4, 2009 at 7:06 AM, Ben Dooks <ben-linux@xxxxxxxxx> wrote:
> > On Thu, Nov 26, 2009 at 03:48:17PM +0900, Jassi Brar wrote:
> >> We need a way to pass controller specific information to the
> >> SPI device driver. For that purpose a new header is made.
> >>
> >> Signed-off-by: Jassi Brar <jassi.brar@xxxxxxxxxxx>
> >> ---
> >> arch/arm/plat-s3c64xx/include/plat/spi.h | 68 ++++++++++++++++++++++++++++++
> >> 1 files changed, 68 insertions(+), 0 deletions(-)
> >> create mode 100644 arch/arm/plat-s3c64xx/include/plat/spi.h
> >>
> >> diff --git a/arch/arm/plat-s3c64xx/include/plat/spi.h b/arch/arm/plat-s3c64xx/include/plat/spi.h
> >> new file mode 100644
> >> index 0000000..d65ddfd
> >> --- /dev/null
> >> +++ b/arch/arm/plat-s3c64xx/include/plat/spi.h
> >
> > let's not have all these called spi.h, it will make life more difficult
> > when trying to find which spi.h we are searching for in our platform
> > support.
> We can call it s3c64xx-spi.h but won't that be kinda redundant as it's
> in plat-s3c64xx ?
If it ever gets moved, then there's your first problem case.
The second, is that you look at the top of the driver and see <plat/spi.h>
and then go 'find . -type f -name spi.h' and see how many results you get
for that. Giving it a more descriptive name makes it easier to find the
right header without having to work out what is being included.
> >> @@ -0,0 +1,68 @@
> >> +/* linux/arch/arm/plat-s3c64xx/include/plat/spi.h
> >> + *
> >> + * Copyright (C) 2009 Samsung Electronics Ltd.
> >> + * Jaswinder Singh <jassi.brar@xxxxxxxxxxx>
> >> + *
> >> + * This program is free software; you can redistribute it and/or modify
> >> + * it under the terms of the GNU General Public License version 2 as
> >> + * published by the Free Software Foundation.
> >> + */
> >> +
> >> +#ifndef __S3C64XX_PLAT_SPI_H
> >> +#define __S3C64XX_PLAT_SPI_H __FILE__
> >> +
> >> +#define S3C64XX_SPI_SRCCLK_PCLK 0
> >> +#define S3C64XX_SPI_SRCCLK_SPIBUS 1
> >> +#define S3C64XX_SPI_SRCCLK_48M 2
> >> +
> >> +#define BUSNUM(b) (b)
> >> +
> >> +/**
> >> + * struct s3c64xx_spi_csinfo - ChipSelect description
> >> + * @fb_delay: Slave specific feedback delay.
> >> + * @set_level: CS line control.
> >> + */
> >> +struct s3c64xx_spi_csinfo {
> >> + u8 fb_delay;
> >> + void (*set_level)(int lvl);
> >> +};
> >
> > I think set_level should be called 'set_cs' to make it clearer what is
> > being done here.
> Well, in the driver we instantiate the structure pointer as 'cs', so all
> the calls look like "cs->set_level" so I think that should be ok,
> as it's quite obvious its all about cs(ChipSelect).
>
> >> +/**
> >> + * struct s3c64xx_spi_cntrlr_info - SPI Controller defining structure
> >> + * @src_clk_nr: Clock source index for the CLK_CFG[SPI_CLKSEL] field.
> >> + * @src_clk_name: Platform name of the corresponding clock.
> >> + * @src_clk: Pointer to the source clock.
> >> + * @num_cs: Number of CS this controller emulates.
> >> + * @cs: Array describing each CS.
> >> + * @cfg_gpio: Configure pins for this SPI controller.
> >> + * @fifo_lvl_mask: All tx fifo_lvl fields start at offset-6
> >> + * @rx_lvl_offset: Depends on tx fifo_lvl field and bus number
> >> + * @high_speed: If the controller supports HIGH_SPEED_EN bit
> >> + */
> >> +struct s3c64xx_spi_cntrlr_info {
> >
> > how about not bothering with the _cntrlr_ here and just call it
> > s3c64xx_spi_info instead?
> Sure.
>
> >> + int src_clk_nr;
> >> + char *src_clk_name;
> >> + struct clk *src_clk;
> >
> > do not pass 'struct clk *' in via platform data.
> Since this is not initialized in platform code: just a pointer
> made available to the driver. So, yes, this can be made a
> member of s3c64xx_spi_driver_data rather.
>
> >> + int num_cs;
> >> + struct s3c64xx_spi_csinfo *cs;
> >> +
> >> + int (*cfg_gpio)(struct platform_device *pdev);
> >> +
> >> + /* Following two fields are for future compatibility */
> >> + int fifo_lvl_mask;
> >> + int rx_lvl_offset;
> >> + int high_speed;
> >> +};
> >
> > I was wondering if a single 'set_cs' callback here would be in order,
> > given each spi device can already hold a chip-select number for use
> > with such callbacks, so:
> >
> > void (*set_cs)(struct s3c64xx_spi_cntrlr_info *us, struct spi_device *sel, int to);
> In that case the machine code wud have to map the chipselect number to
> appropriate function/switch-case. Switch-case maybe ok, but calling some
> function to toggle CS might result in bigger lags between CS and appearance
> of clock on the bus.
The point is that we should already have a pointer to the spi device
being initialised, and this can have a machine-set field in it specifying
the chipselect. If it is all gpio, then this simply could be the
number of the gpio involved.
I don't see that this is going to save a lot of code time, wheras it is
adding to the complexity of the platform data.
> >> +/**
> >> + * s3c64xx_spi_set_info - SPI Controller configure callback by the board
> >> + * initialization code.
> >> + * @cntrlr: SPI controller number the configuration is for.
> >> + * @src_clk_nr: Clock the SPI controller is to use to generate SPI clocks.
> >> + * @cs: Pointer to the array of CS descriptions.
> >> + * @num_cs: Number of elements in the 'cs' array.
> >> + */
> >> +extern void s3c64xx_spi_set_info(int cntrlr, int src_clk_nr, int num_cs);
> >> +
> >> +#endif /* __S3C64XX_PLAT_SPI_H */
> >> --
> >> 1.6.2.5
> >>
> >>
> >> _______________________________________________
> >> linux-arm-kernel mailing list
> >> linux-arm-kernel@xxxxxxxxxxxxxxxxxxx
> >> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> >
> > --
> > --
> > Ben
> >
> > Q: What's a light-year?
> > A: One-third less calories than a regular year.
> >
> >
> > _______________________________________________
> > linux-arm-kernel mailing list
> > linux-arm-kernel@xxxxxxxxxxxxxxxxxxx
> > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> >
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@xxxxxxxxxxxxxxxxxxx
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
--
--
Ben
Q: What's a light-year?
A: One-third less calories than a regular year.
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
