* [PATCH 3/5] SPI S3C64XX: Header for passing platform data @ 2009-11-26 6:48 Jassi Brar 2009-12-03 22:06 ` Ben Dooks 0 siblings, 1 reply; 7+ messages in thread From: Jassi Brar @ 2009-11-26 6:48 UTC (permalink / raw) To: linux-arm-kernel 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@samsung.com> --- 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 @@ -0,0 +1,68 @@ +/* linux/arch/arm/plat-s3c64xx/include/plat/spi.h + * + * Copyright (C) 2009 Samsung Electronics Ltd. + * Jaswinder Singh <jassi.brar@samsung.com> + * + * 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); +}; + +/** + * 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 { + int src_clk_nr; + char *src_clk_name; + struct clk *src_clk; + + 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; +}; + +/** + * 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 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH 3/5] SPI S3C64XX: Header for passing platform data 2009-11-26 6:48 [PATCH 3/5] SPI S3C64XX: Header for passing platform data Jassi Brar @ 2009-12-03 22:06 ` Ben Dooks 2009-12-04 11:06 ` jassi brar 0 siblings, 1 reply; 7+ messages in thread From: Ben Dooks @ 2009-12-03 22:06 UTC (permalink / raw) To: linux-arm-kernel 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@samsung.com> > --- > 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. > @@ -0,0 +1,68 @@ > +/* linux/arch/arm/plat-s3c64xx/include/plat/spi.h > + * > + * Copyright (C) 2009 Samsung Electronics Ltd. > + * Jaswinder Singh <jassi.brar@samsung.com> > + * > + * 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. > +/** > + * 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? > + int src_clk_nr; > + char *src_clk_name; > + struct clk *src_clk; do not pass 'struct clk *' in via platform data. > + 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); > +/** > + * 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 at lists.infradead.org > 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. ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH 3/5] SPI S3C64XX: Header for passing platform data 2009-12-03 22:06 ` Ben Dooks @ 2009-12-04 11:06 ` jassi brar 2009-12-07 14:41 ` Ben Dooks 0 siblings, 1 reply; 7+ messages in thread From: jassi brar @ 2009-12-04 11:06 UTC (permalink / raw) To: linux-arm-kernel On Fri, Dec 4, 2009 at 7:06 AM, Ben Dooks <ben-linux@fluff.org> 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@samsung.com> >> --- >> ?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 ? >> @@ -0,0 +1,68 @@ >> +/* linux/arch/arm/plat-s3c64xx/include/plat/spi.h >> + * >> + * Copyright (C) 2009 Samsung Electronics Ltd. >> + * ? Jaswinder Singh <jassi.brar@samsung.com> >> + * >> + * 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. >> +/** >> + * 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 at lists.infradead.org >> 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 at lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel > ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH 3/5] SPI S3C64XX: Header for passing platform data 2009-12-04 11:06 ` jassi brar @ 2009-12-07 14:41 ` Ben Dooks 2009-12-09 1:50 ` jassi brar 0 siblings, 1 reply; 7+ messages in thread From: Ben Dooks @ 2009-12-07 14:41 UTC (permalink / raw) To: linux-arm-kernel 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@fluff.org> 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@samsung.com> > >> --- > >> ?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@samsung.com> > >> + * > >> + * 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 at lists.infradead.org > >> 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 at lists.infradead.org > > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel > > > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel at lists.infradead.org > 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. ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH 3/5] SPI S3C64XX: Header for passing platform data 2009-12-07 14:41 ` Ben Dooks @ 2009-12-09 1:50 ` jassi brar 2009-12-09 12:16 ` Mark Brown 0 siblings, 1 reply; 7+ messages in thread From: jassi brar @ 2009-12-09 1:50 UTC (permalink / raw) To: linux-arm-kernel On Mon, Dec 7, 2009 at 11:41 PM, Ben Dooks <ben-linux@fluff.org> wrote: > 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@fluff.org> 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@samsung.com> >> >> --- >> >> ?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. Ok. Also I think, for reuse with newer SoCs, we need to divide this into two parts First - SPI controller specific part, which is common to all newer SoCs. Maybe this could go into plat-samsung/include with the name s3c64xx-spi.h? Second - SPI source clocks specific, which SoCs differ by name and number. Maybe this could go into plat-<soc>/include with the name <soc>-spi.h Please share your opinion. >> >> @@ -0,0 +1,68 @@ >> >> +/* linux/arch/arm/plat-s3c64xx/include/plat/spi.h >> >> + * >> >> + * Copyright (C) 2009 Samsung Electronics Ltd. >> >> + * ? Jaswinder Singh <jassi.brar@samsung.com> >> >> + * >> >> + * 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. Just for clarification:- the current implementation does make use of spi_device->controller_data which, as u suggest, is set by machine code to the pointer to a structure(since there is another CS specific parameter- fb_delay) s3c64xx_spi_csinfo We must have a callback function member, rather than void*, because not every CS maybe simple GPIO manipulation. The driver simply call the pointer to a function implemented in machine code. I am unable to decide how to implement what u suggest. If u think u understand the situation well and still stand by ur idea, please let me know, I will modify the driver as you will explain. > 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. Yes, that shudn't be a concern. >> >> +/** >> >> + * 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH 3/5] SPI S3C64XX: Header for passing platform data 2009-12-09 1:50 ` jassi brar @ 2009-12-09 12:16 ` Mark Brown 2009-12-09 12:31 ` jassi brar 0 siblings, 1 reply; 7+ messages in thread From: Mark Brown @ 2009-12-09 12:16 UTC (permalink / raw) To: linux-arm-kernel On Wed, Dec 09, 2009 at 10:50:16AM +0900, jassi brar wrote: > Second - SPI source clocks specific, which SoCs differ by name and number. > Maybe this could go into plat-<soc>/include with the name > <soc>-spi.h With device based lookup (which Ben mentioned he was working on) this mapping should get handled entirely within the CPU clock tree definition and be invisible to the driver. ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH 3/5] SPI S3C64XX: Header for passing platform data 2009-12-09 12:16 ` Mark Brown @ 2009-12-09 12:31 ` jassi brar 0 siblings, 0 replies; 7+ messages in thread From: jassi brar @ 2009-12-09 12:31 UTC (permalink / raw) To: linux-arm-kernel On Wed, Dec 9, 2009 at 9:16 PM, Mark Brown <broonie@opensource.wolfsonmicro.com> wrote: > On Wed, Dec 09, 2009 at 10:50:16AM +0900, jassi brar wrote: > >> ? Second - SPI source clocks specific, which SoCs differ by name and number. >> ? ? ? ? ? Maybe this could go into plat-<soc>/include with the name >> ? ? ? ? ? <soc>-spi.h > > With device based lookup (which Ben mentioned he was working on) this > mapping should get handled entirely within the CPU clock tree > definition and be invisible to the driver. I meant the following part as SoC specific include:- #define S3C64XX_SPI_SRCCLK_PCLK 0 #define S3C64XX_SPI_SRCCLK_SPIBUS 1 #define S3C64XX_SPI_SRCCLK_48M 2 #define BUSNUM(b) (b) the rest can be shared across SoCs in common include ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2009-12-09 12:31 UTC | newest] Thread overview: 7+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2009-11-26 6:48 [PATCH 3/5] SPI S3C64XX: Header for passing platform data Jassi Brar 2009-12-03 22:06 ` Ben Dooks 2009-12-04 11:06 ` jassi brar 2009-12-07 14:41 ` Ben Dooks 2009-12-09 1:50 ` jassi brar 2009-12-09 12:16 ` Mark Brown 2009-12-09 12:31 ` jassi brar
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).