Re: [PATCH v3 6/7] mtd: m25p80: add a read function to read page by page
--- On Thu, 9/30/10, Mingkai Hu wrote: > From: Mingkai Hu > Subject: [PATCH v3 6/7] mtd: m25p80: add a read function to read page by page NAK. We went over this before. The bug is in your SPI master controller driver, and the fix there involves mapping large reads into multiple smaller reads. (Example, 128K read as two 64K reads instead of one of 128K. It's *NEVER* appropriate to commit to patching all upper level drivers in order to work around bugs in lower level ones. The set of such upper level drivers that may need bugfixing is quite large, most will never be used with your buggy controller driver, and all such patches will need testing (but the test resources are probably not available). Whatever SPI controller driver you're working with is clearly buggy ... but not unfixably so. DO NOT head down the path of requiring every SPI device driver to include workarounds for this odd little SPI master driver bug. - Dave ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH] of_mmc_spi: add card detect irq support
Since I don't do OpenFirmware, let's hear from Grant on this one. ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [spi-devel-general] [PATCH v2 4/6] mtd: m25p80: add a read function to read page by page
--- On Tue, 8/10/10, Grant Likely wrote: > This one bothers me, but I can't put my > finger on it. The flag feels > like a controller specific hack. That's because it *IS* ... Not clear what a good fix would look like. But in general, SPI master controllers are responsible for returning all bytes requested, instead of (as with this one) a subset. Suggesting the real issue is a buggy SPI master controller and/or driver. Can't it issue multiple reads to collect all the data requested?? - Dave ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [spi-devel-general] [PATCH v2 3/6] mtd: m25p80: add support to parse the SPI flash's partitions
--- On Mon, 8/9/10, Grant Likely wrote: > > + nr_parts = > of_mtd_parse_partitions(&spi->dev, np, &parts); Let's keep OF-specific logic out of drivers like this one ... intended to work without OF. NAK on adding dependencies like OF to drivers and other infrastructure that starts generic. ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH RFC] usb gadget: introduce usb_gadget_probe_driver
> - The bind functions are only called at init time, so there > is no need > to save a pointer to it. Right. Let's retain the space-saving behaviors by keeping init-only code in init sections. - Dave ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH 1/9 v1.01] Add Synopsys DesignWare HS USB OTG Controller driver.
> > and make the OTG functionality > > key on the generic OTG symbol, not a DW-specific one. > > > > > Use "drivers/usb/otg/otg.c and include/linux/usb/otg.h"? Maybe; CONFIG_USB_OTG specifically, though (or whatever that generic symbol is) ... ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH 1/9 v1.01] Add Synopsys DesignWare HS USB OTG Controller driver.
Please remove all the changes not related to this Synopsis IP ... and make the OTG functionality key on the generic OTG symbol, not a DW-specific one. ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH 1/9] Add Synopsys DesignWare HS USB OTG Controller driver.
Good -- MUSB won't be the only one. ;) Could you mention a few Linux-enabled chips which include this controller? > arch/powerpc/boot/dts/kilauea.dts | 15 + Also, please provide a clean patch that only includes the driver, and split PPC hooks into a separate patch. ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH 2/4] of/gpio: Add support for two-stage registration for the of_gpio_chips
On Tuesday 26 January 2010, Anton Vorontsov wrote: > > > Why have two options, instead of just the first/simpler one?? > > Because I2C/SPI drivers allocate (and register) gpio_chip structures > by themselves, so the first option is a no-go. You should be mentioning such issues in the patch comments. ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH 1/4] gpiolib: Introduce chip addition/removal notifier
On Tuesday 26 January 2010, Anton Vorontsov wrote: > > Just > > inline the little two blocking_notifier_call_chain() calls directly, > > making this a *LOT* simpler. > > I'd rather stay with gpio_call_chain() helper, it makes the code > a little bit prettier, IMO. Compare this: The one without the wrapper is IMO more clear, since it doesn't obfuscate anything. Fewer lines of code, too. :) Pretty is a good attribute ... but is a distant third to clarity. ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH 4/4] powerpc/mcu_mpc8349emitx: Remove OF GPIO handling stuff
On Monday 25 January 2010, Anton Vorontsov wrote: > With the new OF GPIO infrastructure it's much easier to handle I2C > GPIO controllers, i.e. now drivers don't have to deal with the > OF-specific bits. Good, that's how it should have been done in the first place. :) Of course, there's still the question of how to get driver-specific configuration data into the driver... ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH 1/4] gpiolib: Introduce chip addition/removal notifier
On Monday 25 January 2010, Anton Vorontsov wrote: > > +config GPIOLIB_NOTIFIER > + bool > + help > + This symbol is selected by subsystems that need to handle GPIO > + chips addition and removal. E.g., this is used for the > + OpenFirmware bindings. > + I'm no huge fan of notifiers, but I suppose they have their place. However ... I don't see a lot of win to making this optional. Just inline the little two blocking_notifier_call_chain() calls directly, making this a *LOT* simpler. - Dave ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH 2/4] of/gpio: Add support for two-stage registration for the of_gpio_chips
On Monday 25 January 2010, Anton Vorontsov wrote: > With this patch there are two ways to register OF GPIO controllers: > > 1. Allocating the of_gpio_chip structure and passing the > &of_gc->gc pointer to the gpiochip_add. (Can use container_of > to convert the gpio_chip to the of_gpio_chip.) > > 2. Allocating and registering the gpio_chip structure separately > from the of_gpio_chip. (Since two allocations are separate, > container_of won't work.) > > As time goes by we'll kill the first option. Why have two options, instead of just the first/simpler one?? ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH v2 0/8] spi_mpc8xxx: Add support for DMA transfers
On Tuesday 18 August 2009, Anton Vorontsov wrote: > - Fix build issues in fsl_qe_udc; > - Some minor cosmetic changes in "Add support for QE DMA mode and > CPM1/CPM2 chips" patch. Hmm ... the first four of these are pure PPC stuff and thus not appropriate to send as SPI patches; but the second four depend on them. So I'll just say Acked-by: David Brownell and ask you to merge via the PPC tree. (And hope that you verified these are bisectable...) ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH] rtc: Set wakeup capability for I2C and SPI RTC drivers
NAK; see details below On Thursday 27 August 2009, Anton Vorontsov wrote: > RTC core won't allow wakeup alarms to be set if RTC devices' parent > (i.e. i2c_client or spi_device) isn't wakeup capable. Quite rightly so ... being wakeup-capable is config-specific. > For I2C devices there is I2C_CLIENT_WAKE flag exists that we can pass > via board info, and if set, I2C core will initialize wakeup capability. > For SPI devices there is no such flag at all. So why not add it for SPI? If it's an issue, it's not unique to RTC devices. > I believe that it's not platform code responsibility to allow or > disallow wakeups, instead, drivers themselves should set the capability > if a device can trigger wakeups. Drivers can't generally know if that's possible though. They need to be told that it is, by platform code. Most devices can't issue wakeup events. > That's what drivers/base/power/sysfs.c says: > > * It is the responsibility of device drivers to enable (or disable) > * wakeup signaling as part of changing device power states, respecting > * the policy choices provided through the driver model. > > I2C and SPI RTC devices send wakeup events via interrupt lines, so we > should set the wakeup capability if IRQ is routed. Re-read the quoted sentence. You're saying that policy choices should be hard-wired into the driver; contrary to that quote. (In this case the choice is one that platform code must report, and which the hardware designer made: if the device can issue wakeup events.) > Ideally we should also check irq for wakeup capability before setting > device's capability, i.e. > > if (can_irq_wake(irq)) > device_set_wakeup_capable(&client->dev, 1); > > But there is no can_irq_wake() call exist, and it is not that trivial > to implement it for all interrupts controllers and complex/cascaded > setups. That is why platform code should device_init_wakeup() and drivers should check device_can_wakeup(dev) ... > drivers/base/power/sysfs.c also covers these cases: > > * Devices may not be able to generate wakeup events from all power > * states. Also, the events may be ignored in some configurations; > * for example, they might need help from other devices that aren't > * active > > So there is no guarantee that wakeup will actually work, Yes there is ... it's only **exceptional** cases where it can't work. Your patch would make it routine that those flags be unreliable; pretty nasty. ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH 2/6] mtd: m25p80: Convert to device table matching
On Thursday 30 July 2009, Anton Vorontsov wrote:
> This patch converts the m25p80 driver so that now it uses .id_table
> for device matching, making it properly detect devices on OpenFirmware
> platforms (prior to this patch the driver misdetected non-JEDEC chips,
> seeing all chips as "m25p80").
I suspect "detect" is a misnomer there. It only "detects" JEDEC chips.
All others got explicit declarations ... so if there's misbehavior for
other chips, it's because those declarations were poorly handled. Maybe
they were not properly flagged as non-JDEC
> Also, now jedec_probe() only does jedec probing, nothing else. If it
> is not able to detect a chip, NULL is returned and the driver fall
> backs to the information specified by the platform (platform_data, or
> exact ID).
I'd rather keep the warning, so there's a clue about what's really
going on: JEDEC chip found, but its ID is not handled.
> Signed-off-by: Anton Vorontsov
> ---
> drivers/mtd/devices/m25p80.c | 146
> +++---
> 1 files changed, 80 insertions(+), 66 deletions(-)
>
> diff --git a/drivers/mtd/devices/m25p80.c b/drivers/mtd/devices/m25p80.c
> index 10ed195..0d74b38 100644
> --- a/drivers/mtd/devices/m25p80.c
> +++ b/drivers/mtd/devices/m25p80.c
> ... deletia ...
> @@ -481,74 +480,83 @@ struct flash_info {
> #define SECT_4K 0x01/* OPCODE_BE_4K works uniformly
> */
> };
>
> +#define INFO(_jedec_id, _ext_id, _sector_size, _n_sectors, _flags) \
> + ((kernel_ulong_t)&(struct flash_info) { \
> + .jedec_id = (_jedec_id),\
> + .ext_id = (_ext_id),\
> + .sector_size = (_sector_size), \
> + .n_sectors = (_n_sectors), \
> + .flags = (_flags), \
> + })
Anonymous inlined structures ... kind of ugly, but I can
understand why you might not want to declare and name a
few dozen single-use structures.
>
> /* NOTE: double check command sets and memory organization when you add
> * more flash chips. This current list focusses on newer chips, which
> * have been converging on command sets which including JEDEC ID.
> */
> -static struct flash_info __devinitdata m25p_data [] = {
> -
> +static const struct spi_device_id m25p_ids[] = {
> /* Atmel -- some are (confusingly) marketed as "DataFlash" */
> - { "at25fs010", 0x1f6601, 0, 32 * 1024, 4, SECT_4K, },
> - { "at25fs040", 0x1f6604, 0, 64 * 1024, 8, SECT_4K, },
> + { "at25fs010", INFO(0x1f6601, 0, 32 * 1024, 4, SECT_4K) },
> + { "at25fs040", INFO(0x1f6604, 0, 64 * 1024, 8, SECT_4K) },
>
> ... deletia ...
>
> @@ -596,6 +602,7 @@ static struct flash_info *__devinit jedec_probe(struct
> spi_device *spi)
> */
> static int __devinit m25p_probe(struct spi_device *spi)
> {
> + const struct spi_device_id *id;
> struct flash_platform_data *data;
> struct m25p *flash;
> struct flash_info *info;
> @@ -608,32 +615,38 @@ static int __devinit m25p_probe(struct spi_device *spi)
>*/
> data = spi->dev.platform_data;
> if (data && data->type) {
At this point I wonder why you're not changing the probe sequence
more. Get "id" and then "id" here. If it's for "m25p80" assume
it's an old-style board init and do the current logic. Else just
verify "info".
There's a new error case of course: new-style but data->type
doesn't match id->name.
> - for (i = 0, info = m25p_data;
> - i < ARRAY_SIZE(m25p_data);
> - i++, info++) {
> - if (strcmp(data->type, info->name) == 0)
> - break;
> + for (i = 0; i < ARRAY_SIZE(m25p_ids) - 1; i++) {
> + id = &m25p_ids[i];
> + info = (void *)m25p_ids[i].driver_data;
> + if (strcmp(data->type, id->name))
> + continue;
> + break;
> }
>
> /* unrecognized chip? */
> - if (i == ARRAY_SIZE(m25p_data)) {
> + if (i == ARRAY_SIZE(m25p_ids) - 1) {
Better: "if (info == NULL) ..." You've got all the pointers
in hand; don't use indices.
> DEBUG(MTD_DEBUG_LEVEL0, "%s: unrecognized id %s\n",
> dev_name(&spi->dev), data->type);
> info = NULL;
>
> /* recognized; is that chip really what's there? */
> } else if (info->jedec_id) {
> - struct flash_info *chip = jedec_probe(spi);
> + id = jedec_probe(spi);
>
> - if (!chip || chip != info) {
> + if
Re: [PATCH 1/7] spi: Add support for device table matching
On Wednesday 29 July 2009, Ben Dooks wrote:
> > struct spi_driver {
> > + const struct spi_device_id *id_table;
> > + int (*probe_id)(struct spi_device *spi,
> > + const struct spi_device_id *id);
>
> how about leaving it at just probe and have either a call or a field
> in the device that you can look at to see if this was a new style of
> call?
>
> > int (*probe)(struct spi_device *spi);
For the record, if this is going to happen I think the
appropriate long-term solution is to have probe() take
the device_id just as it does with other busses.
Of course that involves changing *every* SPI driver...
and I'd rather not do that quite yet.
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH 0/7] Device table matching for SPI subsystem
On Wednesday 29 July 2009, Anton Vorontsov wrote: > platform_data is overkill for m25p80 chips, the > driver only needs to know exact chip model, and that's what device > tables are for. To be fair, the platform_data also supports partitioning and labeling e.g. for cmdlinepart ... though I'd tend to agree that most SPI flash chips are kind of small (so they're mostly just one smallish partition). ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [spi-devel-general] [PATCH -mm v4][POWERPC] mpc8xxx : allow SPI without cs.
On Friday 19 June 2009, Rini van Zetten wrote: > This patch adds the possibility to have a spi device without a cs. Note that there's now the SPI_NO_CS bit in spi_device.mode to describe this situation ... so no "-EEXIST" hackery should ever tempt anyone again. ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: Subject: [PATCH v8] spi: Add PPC4xx SPI driver
On Friday 26 June 2009, Steven A. Falco wrote: > + > + /* > + * If there are no chip selects at all, or if this is the special > + * case of a non-existent (dummy) chip select, do nothing. > + */ > + > + if (!hw->master->num_chipselect || hw->gpios[cs] == -EEXIST) > + return; > + I'm going to send this in, but please send a followup patch making all this "non-existent (dummy) chip select" stuff use the SPI_NO_CS flag. > + /* > +* A count of zero implies a single SPI device without any > chip-select. > +* Note that of_gpio_count counts all gpios assigned to this spi > master. > +* This includes both "null" gpio's and real ones. > +*/ ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: Subject: [PATCH v7] spi: Add PPC4xx SPI driver
On Thursday 25 June 2009, Steven A. Falco wrote:
> + if (spi->mode & ~MODEBITS) {
> + dev_dbg(&spi->dev, "setup: unsupported mode bits %x\n",
> + spi->mode & ~MODEBITS);
> + return -EINVAL;
> + }
This wasn't tested against 2.6.30-rc1 was it?
See the recent fixup patch I sent. There's a spi_master->modebits
mask that should have been initialized, and which eliminates the
need for tests like that ...
> + dev_dbg(&spi->dev, "%s: mode %d, %u bpw, %d hz\n",
> + __func__, spi->mode, spi->bits_per_word,
> + spi->max_speed_hz);
> +
... also the SPI core now provides a *standard* format debug
message for that stuff. It also handles one more thing, which
I expect to see fixed in a v8 of this patch ... :)
- Dave
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [Question] m25p80 driver versus spi clock rate
On Wednesday 24 June 2009, Steven A. Falco wrote: > Your changes to bitbang_work look good. You tested? > I'm not clear on why you first set do_setup = -1 but later > use do_setup = 1. Perhaps they should both be "1". Other than that, > > Acked-by: Steven A. Falco The "-1" is for the init path, "1" for per-transfer overrides; this way it avoids some extra calls to set up the bits/speed. ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [Question] m25p80 driver versus spi clock rate
On Tuesday 23 June 2009, Steven A. Falco wrote:
> m25p80 spi0.0: invalid bits-per-word (0)
>
> This message comes from spi_ppc4xx_setupxfer. I believe your patch
> is doing what you intended (i.e. forcing an initial call to
> spi_ppc4xx_setupxfer), but it exposes an OF / SPI linkage problem.
>
> Namely, of_register_spi_devices does not support a bits-per-word
> property, so bits-per-word is zero.
Bits-per-word == 0 must be interpreted as == 8.
Simple bug in the ppc4xx code. It currently rejects
values other than 8.
Speaking of spi_ppc4xx issues ... I still have an oldish
copy in my review queue, it needs something like the
appended patch. (Plus something to accept bpw == 0.)
Is there a newer version?
- Dave
--- a/drivers/spi/spi_ppc4xx.c
+++ b/drivers/spi/spi_ppc4xx.c
@@ -61,9 +61,6 @@
/* RxD ready */
#define SPI_PPC4XX_SR_RBR (0x80 >> 7)
-/* the spi->mode bits understood by this driver: */
-#define MODEBITS (SPI_CPHA | SPI_CPOL | SPI_CS_HIGH | SPI_LSB_FIRST)
-
/* clock settings (SCP and CI) for various SPI modes */
#define SPI_CLK_MODE0 SPI_PPC4XX_MODE_SCP
#define SPI_CLK_MODE1 0
@@ -198,9 +195,6 @@ static int spi_ppc4xx_setup(struct spi_d
struct spi_ppc4xx_cs *cs = spi->controller_state;
int init = 0;
- if (!spi->bits_per_word)
- spi->bits_per_word = 8;
-
if (spi->bits_per_word != 8) {
dev_err(&spi->dev, "invalid bits-per-word (%d)\n",
spi->bits_per_word);
@@ -212,12 +206,6 @@ static int spi_ppc4xx_setup(struct spi_d
return -EINVAL;
}
- if (spi->mode & ~MODEBITS) {
- dev_dbg(&spi->dev, "setup: unsupported mode bits %x\n",
- spi->mode & ~MODEBITS);
- return -EINVAL;
- }
-
if (cs == NULL) {
cs = kzalloc(sizeof *cs, GFP_KERNEL);
if (!cs)
@@ -268,10 +256,6 @@ static int spi_ppc4xx_setup(struct spi_d
}
}
- dev_dbg(&spi->dev, "%s: mode %d, %u bpw, %d hz\n",
- __func__, spi->mode, spi->bits_per_word,
- spi->max_speed_hz);
-
return 0;
}
@@ -442,6 +426,9 @@ static int __init spi_ppc4xx_of_probe(st
}
}
+ /* the spi->mode bits understood by this driver: */
+ master->modebits = SPI_CPHA | SPI_CPOL | SPI_CS_HIGH | SPI_LSB_FIRST;
+
/* Setup the state for the bitbang driver */
bbp = &hw->bitbang;
bbp->master = hw->master;
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [BUILD FAILURE 02/04] Next June 04:PPC64 randconfig [drivers/usb/host/ohci-hcd.o]
On Friday 05 June 2009, Subrata Modak wrote: > Correct, it fixes the issue. However, since few changes might have gone > to the Kconfig, the patch does not apply cleanly. Below is the patch, just > a retake of the earlier one, but on the latest code. And it got mangled a bit along the way. Plus, the original one goofed up Kconfig dependency displays ... both issues fixed in this version, against current mainline GIT. If someone can verify all four PPC/OF/OHCI configs build on on PPC64, I'm OK with it. - Dave == CUT HERE From: Arnd Bergmann Subject: fix build failure for PPC64 randconfig [usb/ohci] We could just make the USB_OHCI_HCD_PPC_OF option implicit and selected only if at least one of USB_OHCI_HCD_PPC_OF_BE and USB_OHCI_HCD_PPC_OF_LE are set. [ dbrown...@users.sourceforge.net: fix patch manglation and dependencies ] Signed-off-by: Arnd Bergmann Resent-by: Subrata Modak Signed-off-by: David Brownell --- drivers/usb/host/Kconfig | 29 +++-- 1 file changed, 15 insertions(+), 14 deletions(-) --- a/drivers/usb/host/Kconfig +++ b/drivers/usb/host/Kconfig @@ -180,26 +180,27 @@ config USB_OHCI_HCD_PPC_SOC Enables support for the USB controller on the MPC52xx or STB03xxx processor chip. If unsure, say Y. -config USB_OHCI_HCD_PPC_OF - bool "OHCI support for PPC USB controller on OF platform bus" - depends on USB_OHCI_HCD && PPC_OF - default y - ---help--- - Enables support for the USB controller PowerPC present on the - OpenFirmware platform bus. - config USB_OHCI_HCD_PPC_OF_BE - bool "Support big endian HC" - depends on USB_OHCI_HCD_PPC_OF - default y + bool "OHCI support for OF platform bus (big endian)" + depends on USB_OHCI_HCD && PPC_OF select USB_OHCI_BIG_ENDIAN_DESC select USB_OHCI_BIG_ENDIAN_MMIO + ---help--- + Enables support for big-endian USB controllers present on the + OpenFirmware platform bus. config USB_OHCI_HCD_PPC_OF_LE - bool "Support little endian HC" - depends on USB_OHCI_HCD_PPC_OF - default n + bool "OHCI support for OF platform bus (little endian)" + depends on USB_OHCI_HCD && PPC_OF select USB_OHCI_LITTLE_ENDIAN + ---help--- + Enables support for little-endian USB controllers present on the + OpenFirmware platform bus. + +config USB_OHCI_HCD_PPC_OF + bool + depends on USB_OHCI_HCD && PPC_OF + default USB_OHCI_HCD_PPC_OF_BE || USB_OHCI_HCD_PPC_OF_LE config USB_OHCI_HCD_PCI bool "OHCI support for PCI-bus USB controllers" ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: general SPI question (WAS: [PATCH v6] spi: Add PPC4xx SPI driver)
On Thursday 23 April 2009, Arnav Das wrote: > i am a newbie Lesson #1: make sure your Subject: lines match the message topic (I did a partial repair) and don't post to the wrong list (e.g. PPC lists for OMAP questions). > and am doing a project on beagle board(running > omap3530). i am interfacing an adc(ads7886) to the beagleboard via > spi. need to know how the spi works Read the ads7886 data sheet, in this case. It uses a subset of SPI ... no input commands, just a 12-bit sample returned each time chipselect pulses. > and how a module can access the > spi registers of the omap. if someones already made an adc driver can > they mail me? Use drivers/spi/omap2_mcspi.c on OMAP3 boards, at least to start with. If you want streaming conversions you may want to use different modes than are currently supported by that driver. - Dave ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH v6] spi: Add PPC4xx SPI driver
On Thursday 08 January 2009, Stefan Roese wrote:
> This adds a SPI driver for the SPI controller found in the IBM/AMCC
> 4xx PowerPC's.
> +/*
> + * The PPC4xx SPI controller has no FIFO so each sent/received byte will
> + * generate an interrupt to the CPU. This can cause high CPU utilization.
> + * This driver allows platforms to reduce the interrupt load on the CPU
> + * during SPI transfers by setting max_speed_hz via the device tree.
Note that an alternate strategy is to use polling instead of IRQs,
at least when the data rate is high enough that the IRQ processing
is also slowing down the data transfers.
> +/* bits in mode register - bit 0 ist MSb */
> +/* data latched on leading edge of clock, else trailing edge */
> +#define SPI_PPC4XX_MODE_SCP (0x80 >> 3)
Or in short, SCP == CPHA.
> +/* port enabled */
> +#define SPI_PPC4XX_MODE_SPE (0x80 >> 4)
> +/* MSB first, else LSB first */
> +#define SPI_PPC4XX_MODE_RD (0x80 >> 5)
> +/* clock invert - idle clock = 1, active clock = 0; else reversed */
> +#define SPI_PPC4XX_MODE_CI (0x80 >> 6)
Or in short, CI == CPOL.
> +/* loopback enable */
> +#define SPI_PPC4XX_MODE_IL (0x80 >> 7)
> +/* bits in control register */
> +/* starts a transfer when set */
> +#define SPI_PPC4XX_CR_STR(0x80 >> 7)
> +/* bits in status register */
> +/* port is busy with a transfer */
> +#define SPI_PPC4XX_SR_BSY(0x80 >> 6)
> +/* RxD ready */
> +#define SPI_PPC4XX_SR_RBR(0x80 >> 7)
> +
> +/* the spi->mode bits understood by this driver: */
> +#define MODEBITS (SPI_CPHA | SPI_CPOL | SPI_CS_HIGH | SPI_LSB_FIRST)
> +
> +/* clock settings (SCP and CI) for various SPI modes */
> +#define SPI_CLK_MODE0SPI_PPC4XX_MODE_SCP
> +#define SPI_CLK_MODE10
> +#define SPI_CLK_MODE2SPI_PPC4XX_MODE_CI
> +#define SPI_CLK_MODE3(SPI_PPC4XX_MODE_SCP | SPI_PPC4XX_MODE_CI)
Color me puzzled then. Either the definitions of MODE0 and MODE1
are swapped here ... or the comments above are wrong, and SCP should
describe "falling" vs "rising" instead of "leading" vs "trailing".
> +static int spi_ppc4xx_setupxfer(struct spi_device *spi, struct spi_transfer
> *t)
> +{
> + struct ppc4xx_spi *hw = spi_master_get_devdata(spi->master);
> + struct spi_ppc4xx_cs *cs = spi->controller_state;
> + int scr;
> + u8 cdm = 0;
> + u32 speed;
> + u8 bits_per_word;
> +
> + bits_per_word = (t) ? t->bits_per_word : spi->bits_per_word;
> + speed = (t) ? t->speed_hz : spi->max_speed_hz;
> +
> + ...
> +
> + if (!speed || (speed > spi->max_speed_hz)) {
This is wrong. Typical case is that t->speed_hz is zero,
meaning "use spi->max_speed_hz" ... you treat that as an error.
> + dev_err(&spi->dev, "invalid speed_hz (%d)\n", speed);
> + return -EINVAL;
> + }
> +
> + ...
> +}
> +
> +static int spi_ppc4xx_setup(struct spi_device *spi)
> +{
> + int ret;
> + struct spi_ppc4xx_cs *cs = spi->controller_state;
> + int init = 0;
This "init" thing is still wrong.
Consider: board gets set up with one device. Later,
*WHILE BUSY TRANSFERRING DATA TO/FROM THAT DEVICE*
some other device gets instantiated. Then ...
> + if (cs == NULL) {
> + cs = kzalloc(sizeof *cs, GFP_KERNEL);
> + if (!cs)
> + return -ENOMEM;
> + spi->controller_state = cs;
> +
> + /*
> + * First time called, so let's init the SPI controller
> + * at the end of this function
> + */
> + init = 1;
"init" becomes true here, although the controller has
already been initialized. If it needs to be set up,
do it in probe() before registering the spi_master.
> + }
> +
> + ...
> +
> + /*
> + * New configuration (mode, speed etc) will be written to the
> + * controller in spi_ppc4xx_setupxfer(). Only call
> + * spi_ppc4xx_setupxfer() directly upon first initialization.
> + */
> + if (init) {
> + ret = spi_ppc4xx_setupxfer(spi, NULL);
... so blam, you clobber the settings currently being used for
the active transfer. So this would be a bug.
If not ... this driver deserves a comment on exactly how
unusual this driver is. Specifically, that all spi_device
children must be set up *in advance* so that standard calls
like spi_new_device() don't work with it; and why.
I don't think I see anything in this driver which would
prevent that from working, though. Sure you've got to
have a list of chipselect GPIOs in advance, but that's
distinct from being unable to use spi_new_device().
> + ...
> +}
> +
> +static void spi_ppc4xx_chipsel(struct spi_device *spi, int value)
> +{
> + struct ppc4xx_spi *hw = spi_master_get_devdata(spi->master);
> + unsigned int cspol = spi->mode & SPI_CS_HIGH ? 1 : 0;
> + unsigned int cs = spi->chip_select;
> +
> + if (!hw->num_gpios)
> + return;
Hmm, num_gpios ... can never be zero, right? You always
set it up
Re: [PATCH v6] spi: Add PPC4xx SPI driver
On Wednesday 22 April 2009, Stefan Roese wrote: > On Wednesday 22 April 2009, David Brownell wrote: > > On Thursday 08 January 2009, Stefan Roese wrote: > > > This adds a SPI driver for the SPI controller found in the IBM/AMCC > > > 4xx PowerPC's. > > > > Note that given some patches now in the mm tree, this needs > > something like the appended fixup. Some common code has now > > moved into the spi core. > > So would you accept the driver for 2.6.31 merge with your suggested change > below? If yes, I'll generate a 7th version. Hold on until I send the rest of my review comments; almost done. Sorry for the delay. Yes, I think a 2.6.31 merge should be doable. - Dave > > Thanks. > > Best regards, > Stefan > > ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH v6] spi: Add PPC4xx SPI driver
On Thursday 08 January 2009, Stefan Roese wrote:
> This adds a SPI driver for the SPI controller found in the IBM/AMCC
> 4xx PowerPC's.
Note that given some patches now in the mm tree, this needs
something like the appended fixup. Some common code has now
moved into the spi core.
- Dave
---
drivers/spi/spi_ppc4xx.c | 19 +++
1 file changed, 3 insertions(+), 16 deletions(-)
--- a/drivers/spi/spi_ppc4xx.c
+++ b/drivers/spi/spi_ppc4xx.c
@@ -61,9 +61,6 @@
/* RxD ready */
#define SPI_PPC4XX_SR_RBR (0x80 >> 7)
-/* the spi->mode bits understood by this driver: */
-#define MODEBITS (SPI_CPHA | SPI_CPOL | SPI_CS_HIGH | SPI_LSB_FIRST)
-
/* clock settings (SCP and CI) for various SPI modes */
#define SPI_CLK_MODE0 SPI_PPC4XX_MODE_SCP
#define SPI_CLK_MODE1 0
@@ -198,9 +195,6 @@ static int spi_ppc4xx_setup(struct spi_d
struct spi_ppc4xx_cs *cs = spi->controller_state;
int init = 0;
- if (!spi->bits_per_word)
- spi->bits_per_word = 8;
-
if (spi->bits_per_word != 8) {
dev_err(&spi->dev, "invalid bits-per-word (%d)\n",
spi->bits_per_word);
@@ -212,12 +206,6 @@ static int spi_ppc4xx_setup(struct spi_d
return -EINVAL;
}
- if (spi->mode & ~MODEBITS) {
- dev_dbg(&spi->dev, "setup: unsupported mode bits %x\n",
- spi->mode & ~MODEBITS);
- return -EINVAL;
- }
-
if (cs == NULL) {
cs = kzalloc(sizeof *cs, GFP_KERNEL);
if (!cs)
@@ -268,10 +256,6 @@ static int spi_ppc4xx_setup(struct spi_d
}
}
- dev_dbg(&spi->dev, "%s: mode %d, %u bpw, %d hz\n",
- __func__, spi->mode, spi->bits_per_word,
- spi->max_speed_hz);
-
return 0;
}
@@ -442,6 +426,9 @@ static int __init spi_ppc4xx_of_probe(st
}
}
+ /* the spi->mode bits understood by this driver: */
+ master->modebits = SPI_CPHA | SPI_CPOL | SPI_CS_HIGH | SPI_LSB_FIRST;
+
/* Setup the state for the bitbang driver */
bbp = &hw->bitbang;
bbp->master = hw->master;
___
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [BUILD FAILURE 11/12] Next April 21 : PPC64 randconfig [drivers/usb/host/ohci-hcd.o]
On Tuesday 21 April 2009, Randy Dunlap wrote: > > > Since its feasible to say 'n' to both we get the compile error. How do > > we enforce having at least one set? > > Looks like using "choice" without "optional" would do it. > See Documentation/kbuild/kconfig-language.txt and various examples > in Kconfig* files. That won't quite work ... "at least one" includes "two" (i.e. a PCI card in little-endian, a native controller in big-endian). Real-world systems need such configs, or so I'm told, and that's why their supported. Is there maybe a way to force Kconfig to just reject such illegal configs -- neither option set -- rather than trying some how to fix it? Or maybe ... if neither one is set, have the header force both on, and issue a warning. ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [BUILD FAILURE 11/12] Next April 21 : PPC64 randconfig [drivers/usb/host/ohci-hcd.o]
On Tuesday 21 April 2009, Subrata Modak wrote: > Observing this for the first time: > > CC drivers/usb/host/ohci-hcd.o > In file included from drivers/usb/host/ohci-hcd.c:1060: > drivers/usb/host/ohci-ppc-of.c:242:2: error: #error "No endianess Hmm, scripts/get_maintainer.pl doesn't report the PPC folk who maintain that file and its kbuild infrastructure. Can we have some PPC folk look at (and fix) this? > selected for ppc-of-ohci" > make[3]: *** [drivers/usb/host/ohci-hcd.o] Error 1 > make[2]: *** [drivers/usb/host] Error 2 > make[1]: *** [drivers/usb] Error 2 > make: *** [drivers] Error 2 > --- > > Regards-- > Subrata > > ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH] Xilinx: SPI: driver not releasing memory
On Friday 06 March 2009, Grant Likely wrote:
> David,
>
> Are you okay with this patch and okay with it going in via Ben's
> powerpc tree? Ben wants to ensure that changes outside arch/powerpc/
> are properly acked before going into his tree.
Sure, no problem.
> Thanks,
> g.
>
> On Sat, Feb 28, 2009 at 9:09 PM, Grant Likely
> wrote:
> > On Fri, Feb 27, 2009 at 4:54 PM, John Linn wrote:
> >> The driver was not releasing memory when it was removed or
> >> when there was a failure during probe. This fixes it.
> >>
> >> Signed-off-by: John Linn
> >
> > Looks good.
> >
> > Acked-by: Grant Likely
Acked-by: David Brownell
> > I'll pick this up into my -next branch and ask Ben to pull it in the
> > next week or so.
>
> > ---
> > This is an incremental patch to the patch (updated driver
> > for device tree) that is in the next branch.
> > ---
> > drivers/spi/xilinx_spi.c | 9 +++--
> > 1 files changed, 7 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/spi/xilinx_spi.c b/drivers/spi/xilinx_spi.c
> > index fe7e5f3..494d3f7 100644
> > --- a/drivers/spi/xilinx_spi.c
> > +++ b/drivers/spi/xilinx_spi.c
> > @@ -354,7 +354,7 @@ static int __init xilinx_spi_of_probe(struct of_device
> > *ofdev,
> > if (xspi->regs == NULL) {
> > rc = -ENOMEM;
> > dev_warn(&ofdev->dev, "ioremap failure\n");
> > - goto put_master;
> > + goto release_mem;
> > }
> > xspi->irq = r_irq->start;
> >
> > @@ -365,7 +365,7 @@ static int __init xilinx_spi_of_probe(struct of_device
> > *ofdev,
> > prop = of_get_property(ofdev->node, "xlnx,num-ss-bits", &len);
> > if (!prop || len < sizeof(*prop)) {
> > dev_warn(&ofdev->dev, "no 'xlnx,num-ss-bits' property\n");
> > - goto put_master;
> > + goto unmap_io;
> > }
> > master->num_chipselect = *prop;
> >
> > @@ -397,6 +397,8 @@ free_irq:
> > free_irq(xspi->irq, xspi);
> > unmap_io:
> > iounmap(xspi->regs);
> > +release_mem:
> > + release_mem_region(r_mem->start, resource_size(r_mem));
> > put_master:
> > spi_master_put(master);
> > return rc;
> > @@ -406,6 +408,7 @@ static int __devexit xilinx_spi_remove(struct of_device
> > *ofdev)
> > {
> > struct xilinx_spi *xspi;
> > struct spi_master *master;
> > + struct resource r_mem;
> >
> > master = platform_get_drvdata(ofdev);
> > xspi = spi_master_get_devdata(master);
> > @@ -413,6 +416,8 @@ static int __devexit xilinx_spi_remove(struct of_device
> > *ofdev)
> > spi_bitbang_stop(&xspi->bitbang);
> > free_irq(xspi->irq, xspi);
> > iounmap(xspi->regs);
> > + if (!of_address_to_resource(ofdev->node, 0, &r_mem))
> > + release_mem_region(r_mem.start, resource_size(&r_mem));
> > dev_set_drvdata(&ofdev->dev, 0);
> > spi_master_put(xspi->bitbang.master);
> >
> > --
> > 1.5.3.4
> >
> >
> >
> > This email and any attachments are intended for the sole use of the named
> > recipient(s) and contain(s) confidential information that may be
> > proprietary, privileged or copyrighted under applicable law. If you are not
> > the intended recipient, do not read, copy, or forward this email message or
> > any attachments. Delete this email message and any attachments immediately.
> >
> >
> >
>
>
>
> --
> Grant Likely, B.Sc., P.Eng.
> Secret Lab Technologies Ltd.
>
>
___
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH v5] spi: Add PPC4xx SPI driver
Also:
> +static struct of_platform_driver spi_ppc4xx_of_driver = {
> + .owner = THIS_MODULE,
> + .name = DRIVER_NAME,
I'd hope the PPC folk eliminate this duplication soonish.
Those fields are obvious duplicates of the driver model
fields...
> + .match_table = spi_ppc4xx_of_match,
> + .probe = spi_ppc4xx_of_probe,
> + .remove = __exit_p(spi_ppc4xx_of_remove),
> + .driver = {
> + .name = DRIVER_NAME,
> + .owner = THIS_MODULE,
> + },
> +};
___
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH v5] spi: Add PPC4xx SPI driver
On Tuesday 25 November 2008, Stefan Roese wrote:
> Changes in v5:
> - Don't call setupxfer() from setup() so that the baudrate etc
> won't get changed while another transfer is active, as suggested
> by David Brownell.
Better, but this still doesn't seem quite right:
> +static int spi_ppc4xx_setupxfer(struct spi_device *spi, struct spi_transfer
> *t)
> +{
> + struct ppc4xx_spi *hw = spi_master_get_devdata(spi->master);
> + struct spi_ppc4xx_cs *cs = spi->controller_state;
> + unsigned char cdm = 0;
> + int scr;
> + u8 bpw;
> +
> + /* Write new configration */
> + out_8(&hw->regs->mode, cs->mode);
> +
> + /*
> + * Allow platform reduce the interrupt load on the CPU during SPI
> + * transfers. We do not target maximum performance, but rather allow
> + * platform to limit SPI bus frequency and interrupt rate.
That comment doesn't seem even vaguely related to the
code it allegedly refers to ... nothing in this routine
affects the IRQ load.
> + */
> + bpw = t ? t->bits_per_word : spi->bits_per_word;
> + cs->speed_hz = t ? min(t->speed_hz, spi->max_speed_hz) :
> + spi->max_speed_hz;
> +
> + if (bpw != 8) {
> + dev_err(&spi->dev, "invalid bits-per-word (%d)\n", bpw);
> + return -EINVAL;
> + }
> +
> + if (cs->speed_hz == 0) {
> + dev_err(&spi->dev, "invalid speed_hz (must be non-zero)\n");
> + return -EINVAL;
> + }
> +
> + /* set the clock */
> + /* opb_freq was already divided by 4 */
> + scr = (hw->opb_freq / cs->speed_hz) - 1;
> +
> + if (scr > 0)
> + cdm = min(scr, 0xff);
> +
> + dev_dbg(&spi->dev, "setting pre-scaler to %d (hz %d)\n", cdm,
> + cs->speed_hz);
> +
> + if (in_8(&hw->regs->cdm) != cdm)
> + out_8(&hw->regs->cdm, cdm);
> +
> + spin_lock(&hw->bitbang.lock);
> + if (!hw->bitbang.busy) {
> + hw->bitbang.chipselect(spi, BITBANG_CS_INACTIVE);
> + /* need to ndelay here? */
> + }
> + spin_unlock(&hw->bitbang.lock);
> +
> + return 0;
> +}
> +
> +static int spi_ppc4xx_setup(struct spi_device *spi)
> +{
> + int ret;
> + struct spi_ppc4xx_cs *cs = spi->controller_state;
> + int init = 0;
> +
> + if (!spi->bits_per_word)
> + spi->bits_per_word = 8;
Given the above restrictions, it'd be better to
if (spi->bits_per_word != 8)
return -EINVAL;
On the general policy of reporting such errors as near as
practical to the place they appear ... otherwise it gets
hard to track them down, since the faults get reported a
long time later, well after the point drivers expect to see
such reports. Likewise with spi->max_speed_hz.
> +
> + if (spi->mode & ~MODEBITS) {
> + dev_dbg(&spi->dev, "setup: unsupported mode bits %x\n",
> + spi->mode & ~MODEBITS);
> + return -EINVAL;
> + }
> +
> + if (cs == NULL) {
> + cs = kzalloc(sizeof *cs, GFP_KERNEL);
> + if (!cs)
> + return -ENOMEM;
> + spi->controller_state = cs;
> +
> + /*
> + * First time called, so let's init the SPI controller
> + * at the end of this function
> + */
> + init = 1;
> + }
> +
> + /*
> + * We set all bits of the SPI0_MODE register, so,
> + * no need to read-modify-write
> + */
> + cs->mode = SPI_PPC4XX_MODE_SPE;
> +
> + switch (spi->mode & (SPI_CPHA | SPI_CPOL)) {
> + case SPI_MODE_0:
> + cs->mode |= SPI_CLK_MODE0;
> + break;
> + case SPI_MODE_1:
> + cs->mode |= SPI_CLK_MODE1;
> + break;
> + case SPI_MODE_2:
> + cs->mode |= SPI_CLK_MODE2;
> + break;
> + case SPI_MODE_3:
> + cs->mode |= SPI_CLK_MODE3;
> + break;
> + }
> +
> + if (spi->mode & SPI_LSB_FIRST) {
> + /* this assumes that bit 7 is the LSb! */
> + cs->mode |= SPI_PPC4XX_MODE_RD;
The Linux bit numbering convention is that BIT(0) is the LSB,
so that comment is nonsensical. BIT(7) will always bee the
MSB of an 8-bit byte.
If the issue is a PPC convention that BIT(0) is the MSB,
then please adjust comments accordingly. Or better, just
strike the comment ... bit numbering is irrelevan
Re: [PATCH/RFC] Add Alternative Log Buffer Support for printk Messages
No comment from me on $SUBJECT beyond "it seems plausible", but ... On Tuesday 25 November 2008, David VomLehn wrote: > The important point, though, is that device tree is the only > thing approaching a standard on any non-x86-based platform for passing > structured information from the bootloader to the kernel. The command > line is just not sufficient for this. Me, I'll be happier if I don't have to try using that device tree. Having board-specific code in the kernel is a more complete solution, and makes it a lot easier to cope with all the hardware goofage. Recall that the *original* notion behind OpenBoot (now "OpenFirmware") was to have tables for the stuff that was table-friendly, and call out to FORTH code (possibly not just at boot time) for the rest. (Given the choice of FORTH vs ACPI bytecodes, I'd go for FORTH; but the better option is "neither".) Right now I see an awful lot of work going into trying to force lots of stuff into table format. Even when it's the sort of one-off or board-specific quirkery that was an original motivation for having FORTH escapes (tasks that were not table-friendly). - Dave ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [spi-devel-general] [PATCH v4] spi: Add PPC4xx SPI driver
On Friday 31 October 2008, Stefan Roese wrote: > + dev_dbg(&spi->dev, "%s: mode %d, %u bpw, %d hz\n", > + __FUNCTION__, spi->mode, spi->bits_per_word, > + spi->max_speed_hz); Oh, and checkpatch.pl would warn about __FUNCTION__ vs __func__ ... ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [spi-devel-general] [PATCH v4] spi: Add PPC4xx SPI driver
On Friday 31 October 2008, Stefan Roese wrote:
> +static int spi_ppc4xx_setupxfer(struct spi_device *spi, struct spi_transfer
> *t)
> +{
> ...
>
> + if (in_8(&hw->regs->cdm) != cdm)
> + out_8(&hw->regs->cdm, cdm);
... writes to hardware, updating SPI the clock rate ...
> +static int spi_ppc4xx_setup(struct spi_device *spi)
> +{
> ...
> +
> + ret = spi_ppc4xx_setupxfer(spi, NULL);
... hence this also writes to the hardware.
Except ... the setup() method may be called for one SPI device
in the middle of a transfer for another device. This might for
example cause the clock rate to speed up so it's too fast, thus
corrupting a transfer for that other device. So you should NOT
be calling setupxfer() here.
> + /* write new configration */
> + out_8(&hw->regs->mode, mode);
Or here: changing from a device using MSB-first mode 3 to
one using LSB-first mode 1. This could also corrupt a transfer.
It might be better to have the setup() validate its inputs
and store them in the spi->controller_state, and then have
the setup_transfer() pull values from there ... but not make
setup() call setupxfer(). The best model would be that each
chipselect has its own set of speed/mode registers; if the
hardware doesn't work that way, then it can be emulated.
> +static int __init spi_ppc4xx_init(void)
> +{
> + return of_register_platform_driver(&spi_ppc4xx_of_driver);
> +}
> +
> +static void __exit spi_ppc4xx_exit(void)
> +{
> + of_unregister_platform_driver(&spi_ppc4xx_of_driver);
> +}
> +
> +module_init(spi_ppc4xx_init);
> +module_exit(spi_ppc4xx_exit);
I prefer to see module_{init,exit} annotations snugged up next
to the functions to which they apply ... just like EXPORT_SYMBOL
annotations, and for the same reasons.
- Dave
___
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [spi-devel-general] [PATCH v3] spi: Add PPC4xx SPI driver
On Thursday 30 October 2008, Jason Hanna wrote: > Also, any pointers to sample/test code incorporating a spi protocol > driver would be incredibly helpful. Look at Documentation/spi/*.c ... for user mode code hooking up through "spidev". > I'm very new to device driver > programming and don't really know what I'm doing yet. I seem to be > getting the spi_ppc4xx and spi_bitbang modules loaded, but am unsure > how to verify proper initialization and what next steps I need to > follow in order to develop and associate a protocol driver. Documentation/spi/spi-summary should help. And sticking a 'scope on the outputs (as you said you plan) is a decent place to start, if you don't have something to talk to yet. - dave ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH 2/6 v3] usb/fsl_qe_udc: Fix recursive locking bug in ch9getstatus()
On Tuesday 18 November 2008, Anton Vorontsov wrote:
> The call chain is this:
>
> qe_udc_irq() <- grabs the udc->lock spinlock
> rx_irq()
> qe_ep0_rx()
> ep0_setup_handle()
> setup_received_handle()
> ch9getstatus()
> qe_ep_queue() <- tries to grab the udc->lock again
>
> It seems unsafe to temporarily drop the lock in the ch9getstatus(),
> so to fix that bug the lock-less __qe_ep_queue() function
> implemented and used by the ch9getstatus().
>
> Signed-off-by: Anton Vorontsov <[EMAIL PROTECTED]>
Acked-by: David Brownell <[EMAIL PROTECTED]>
... noting that this *also* fixes a locking bug in qe_ep_queue(),
where it modified the queue head without holding the spinlock.
(So for example an IRQ in the middle of that update could end
up updating the incompletely updated list head ... oopsie!)
And I'd have made the __qe_ep_queue() routine take a qe_ep
and qe_req, but that's just a minor issue.
> ---
>
> On Tue, Nov 18, 2008 at 02:13:30PM -0800, David Brownell wrote:
> > On Tuesday 18 November 2008, Anton Vorontsov wrote:
> > > + spin_lock_irqsave(&udc->lock, flags);
> > > + ret = __qe_ep_queue(_ep, _req, gfp_flags);
> > > + spin_unlock_irqrestore(&udc->lock, flags);
> >
> > Why are you passing "gfp_flags"? Especially without
> > checking ... GFP_KERNEL will be illegal, for example.
>
> Ugh, the gfp_flags aren't used at all. How about that
> patch?
>
> drivers/usb/gadget/fsl_qe_udc.c | 26 ++
> 1 files changed, 18 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/usb/gadget/fsl_qe_udc.c b/drivers/usb/gadget/fsl_qe_udc.c
> index 60b9279..3587970 100644
> --- a/drivers/usb/gadget/fsl_qe_udc.c
> +++ b/drivers/usb/gadget/fsl_qe_udc.c
> @@ -1681,14 +1681,11 @@ static void qe_free_request(struct usb_ep *_ep,
> struct usb_request *_req)
> kfree(req);
> }
>
> -/* queues (submits) an I/O request to an endpoint */
> -static int qe_ep_queue(struct usb_ep *_ep, struct usb_request *_req,
> - gfp_t gfp_flags)
> +static int __qe_ep_queue(struct usb_ep *_ep, struct usb_request *_req)
> {
> struct qe_ep *ep = container_of(_ep, struct qe_ep, ep);
> struct qe_req *req = container_of(_req, struct qe_req, req);
> struct qe_udc *udc;
> - unsigned long flags;
> int reval;
>
> udc = ep->udc;
> @@ -1732,7 +1729,7 @@ static int qe_ep_queue(struct usb_ep *_ep, struct
> usb_request *_req,
> list_add_tail(&req->queue, &ep->queue);
> dev_vdbg(udc->dev, "gadget have request in %s! %d\n",
> ep->name, req->req.length);
> - spin_lock_irqsave(&udc->lock, flags);
> +
> /* push the request to device */
> if (ep_is_in(ep))
> reval = ep_req_send(ep, req);
> @@ -1748,11 +1745,24 @@ static int qe_ep_queue(struct usb_ep *_ep, struct
> usb_request *_req,
> if (ep->dir == USB_DIR_OUT)
> reval = ep_req_receive(ep, req);
>
> - spin_unlock_irqrestore(&udc->lock, flags);
> -
> return 0;
> }
>
> +/* queues (submits) an I/O request to an endpoint */
> +static int qe_ep_queue(struct usb_ep *_ep, struct usb_request *_req,
> +gfp_t gfp_flags)
> +{
> + struct qe_ep *ep = container_of(_ep, struct qe_ep, ep);
> + struct qe_udc *udc = ep->udc;
> + unsigned long flags;
> + int ret;
> +
> + spin_lock_irqsave(&udc->lock, flags);
> + ret = __qe_ep_queue(_ep, _req);
> + spin_unlock_irqrestore(&udc->lock, flags);
> + return ret;
> +}
> +
> /* dequeues (cancels, unlinks) an I/O request from an endpoint */
> static int qe_ep_dequeue(struct usb_ep *_ep, struct usb_request *_req)
> {
> @@ -2008,7 +2018,7 @@ static void ch9getstatus(struct qe_udc *udc, u8
> request_type, u16 value,
> udc->ep0_dir = USB_DIR_IN;
>
> /* data phase */
> - status = qe_ep_queue(&ep->ep, &req->req, GFP_ATOMIC);
> + status = __qe_ep_queue(&ep->ep, &req->req);
>
> if (status == 0)
> return;
> --
> 1.5.6.3
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-usb" in
> the body of a message to [EMAIL PROTECTED]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
>
___
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH 2/6 v2] usb/fsl_qe_udc: Fix recursive locking bug in ch9getstatus()
On Tuesday 18 November 2008, Anton Vorontsov wrote: > + spin_lock_irqsave(&udc->lock, flags); > + ret = __qe_ep_queue(_ep, _req, gfp_flags); > + spin_unlock_irqrestore(&udc->lock, flags); Why are you passing "gfp_flags"? Especially without checking ... GFP_KERNEL will be illegal, for example. ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH 2/6] usb/fsl_qe_udc: Fix recursive locking bug in ch9getstatus()
On Tuesday 11 November 2008, Anton Vorontsov wrote: > - spin_lock_irqsave(&udc->lock, flags); > + if (lock) > + spin_lock_irqsave(lock, flags); Ugly ugly ugly. Conditional locking is error prone ... don't. Couldn't you just have the usb_ep_queue() method wrap lock calls around a common routine called by that and the status reporting code? Or just have the status reporting code stuff the two bytes directly into the FIFO? (Which is what a lot of other drivers do.) - Dave ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH 1/6] usb/fsl_qe_udc: Fix oops on QE UDC probe failure
On Tuesday 11 November 2008, Anton Vorontsov wrote:
> In case of probing errors the driver kfrees the udc_controller, but it
> doesn't set the pointer to NULL.
>
> When usb_gadget_register_driver is called, it checks for udc_controller
> != NULL, the check passes and the driver accesses nonexistent memory.
> Fix this by setting udc_controller to NULL in case of errors.
>
> While at it, also implement irq_of_parse_and_map()'s failure and cleanup
> cases.
>
> Signed-off-by: Anton Vorontsov <[EMAIL PROTECTED]>
Acked-by: David Brownell <[EMAIL PROTECTED]>
I seem to detect a lot of bugfix activity here, which
tends to reflect usage ... good! :)
> ---
> drivers/usb/gadget/fsl_qe_udc.c |9 -
> 1 files changed, 8 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/usb/gadget/fsl_qe_udc.c b/drivers/usb/gadget/fsl_qe_udc.c
> index 94c38e4..60b9279 100644
> --- a/drivers/usb/gadget/fsl_qe_udc.c
> +++ b/drivers/usb/gadget/fsl_qe_udc.c
> @@ -2601,6 +2601,10 @@ static int __devinit qe_udc_probe(struct of_device
> *ofdev,
> (unsigned long)udc_controller);
> /* request irq and disable DR */
> udc_controller->usb_irq = irq_of_parse_and_map(np, 0);
> + if (!udc_controller->usb_irq) {
> + ret = -EINVAL;
> + goto err_noirq;
> + }
>
> ret = request_irq(udc_controller->usb_irq, qe_udc_irq, 0,
> driver_name, udc_controller);
> @@ -2622,6 +2626,8 @@ static int __devinit qe_udc_probe(struct of_device
> *ofdev,
> err6:
> free_irq(udc_controller->usb_irq, udc_controller);
> err5:
> + irq_dispose_mapping(udc_controller->usb_irq);
> +err_noirq:
> if (udc_controller->nullmap) {
> dma_unmap_single(udc_controller->gadget.dev.parent,
> udc_controller->nullp, 256,
> @@ -2645,7 +2651,7 @@ err2:
> iounmap(udc_controller->usb_regs);
> err1:
> kfree(udc_controller);
> -
> + udc_controller = NULL;
> return ret;
> }
>
> @@ -2707,6 +2713,7 @@ static int __devexit qe_udc_remove(struct of_device
> *ofdev)
> kfree(ep->txframe);
>
> free_irq(udc_controller->usb_irq, udc_controller);
> + irq_dispose_mapping(udc_controller->usb_irq);
>
> tasklet_kill(&udc_controller->rx_tasklet);
>
> --
> 1.5.6.3
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-usb" in
> the body of a message to [EMAIL PROTECTED]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
>
___
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH 6/6] usb/fsl_qe_udc: Fix stalled TX requests bug
On Tuesday 11 November 2008, Anton Vorontsov wrote: > While disabling an endpoint the driver nuking any pending requests, > thus completing them with -ESHUTDOWN status. But the driver doesn't > clear the tx_req, which means that a next TX request (after > ep_enable), might get stalled, since the driver won't queue the new > reqests. > > This fixes a bug I'm observing with ethernet gadget while playing > with ifconfig usb0 up/down (the up/down sequence disables and > enables `in' and `out' endpoints). > > Signed-off-by: Anton Vorontsov <[EMAIL PROTECTED]> Acked-by: David Brownell <[EMAIL PROTECTED]> > --- > drivers/usb/gadget/fsl_qe_udc.c |1 + > 1 files changed, 1 insertions(+), 0 deletions(-) > > diff --git a/drivers/usb/gadget/fsl_qe_udc.c b/drivers/usb/gadget/fsl_qe_udc.c > index cb47337..37c8575 100644 > --- a/drivers/usb/gadget/fsl_qe_udc.c > +++ b/drivers/usb/gadget/fsl_qe_udc.c > @@ -1622,6 +1622,7 @@ static int qe_ep_disable(struct usb_ep *_ep) > nuke(ep, -ESHUTDOWN); > ep->desc = NULL; > ep->stopped = 1; > + ep->tx_req = NULL; > qe_ep_reset(udc, ep->epnum); > spin_unlock_irqrestore(&udc->lock, flags); > > -- > 1.5.6.3 > -- > To unsubscribe from this list: send the line "unsubscribe linux-usb" in > the body of a message to [EMAIL PROTECTED] > More majordomo info at http://vger.kernel.org/majordomo-info.html > > ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH 5/6] usb/fsl_qe_udc: Fix muram corruption by disabled endpoints
On Tuesday 11 November 2008, Anton Vorontsov wrote: > Before freeing an endpoint's muram memory, we should stop all activity > of the endpoint, otherwise the QE UDC controller might do nasty things > with the muram memory that isn't belong to that endpoint anymore. > > The qe_ep_reset() effectively flushes the hardware fifos, finishes all > late transaction and thus prevents the corruption. > > Signed-off-by: Anton Vorontsov <[EMAIL PROTECTED]> Acked-by: David Brownell <[EMAIL PROTECTED]> > --- > drivers/usb/gadget/fsl_qe_udc.c |1 + > 1 files changed, 1 insertions(+), 0 deletions(-) > > diff --git a/drivers/usb/gadget/fsl_qe_udc.c b/drivers/usb/gadget/fsl_qe_udc.c > index 7bb5f36..cb47337 100644 > --- a/drivers/usb/gadget/fsl_qe_udc.c > +++ b/drivers/usb/gadget/fsl_qe_udc.c > @@ -1622,6 +1622,7 @@ static int qe_ep_disable(struct usb_ep *_ep) > nuke(ep, -ESHUTDOWN); > ep->desc = NULL; > ep->stopped = 1; > + qe_ep_reset(udc, ep->epnum); > spin_unlock_irqrestore(&udc->lock, flags); > > cpm_muram_free(cpm_muram_offset(ep->rxbase)); > -- > 1.5.6.3 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-usb" in > the body of a message to [EMAIL PROTECTED] > More majordomo info at http://vger.kernel.org/majordomo-info.html > > ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH 4/6] usb/fsl_qe_udc: Fix disconnects reporting during bus reset
On Tuesday 11 November 2008, Anton Vorontsov wrote:
> Freescale QE UDC controllers can't report the "port change" states,
> so the only way to handle disconnects is to process bus reset
> interrupts. The bus reset can take some time, that is, few irqs.
> Gadgets may print the disconnection events, and this causes few
> repetitive messages in the kernel log.
>
> This patch fixes the issue by using the usb_state machine, if the
> usb controller has been already reset, just quit the reset irq
> early.
>
> Signed-off-by: Anton Vorontsov <[EMAIL PROTECTED]>
Acked-by: David Brownell <[EMAIL PROTECTED]>
Note that the "port change" you reference is more typically
packaged as "VBUS detect" ... often handled with a GPIO.
> ---
> drivers/usb/gadget/fsl_qe_udc.c |3 +++
> 1 files changed, 3 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/usb/gadget/fsl_qe_udc.c b/drivers/usb/gadget/fsl_qe_udc.c
> index 1789f65..7bb5f36 100644
> --- a/drivers/usb/gadget/fsl_qe_udc.c
> +++ b/drivers/usb/gadget/fsl_qe_udc.c
> @@ -2161,6 +2161,9 @@ static int reset_irq(struct qe_udc *udc)
> {
> unsigned char i;
>
> + if (udc->usb_state == USB_STATE_DEFAULT)
> + return 0;
> +
> qe_usb_disable();
> out_8(&udc->usb_regs->usb_usadr, 0);
>
> --
> 1.5.6.3
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-usb" in
> the body of a message to [EMAIL PROTECTED]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
>
___
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH v2] usb/fsl_usb2_udc: Report disconnect before unbinding
On Thursday 13 November 2008, Anton Vorontsov wrote: > Gadgets disable endpoints in their disconnect callbacks, so > we must call disconnect before unbinding. > > The patch fixes following badness: > > [EMAIL PROTECTED]:~# insmod fsl_usb2_udc.ko > Freescale High-Speed USB SOC Device Controller driver (Apr 20, 2007) > [EMAIL PROTECTED]:~# insmod g_ether.ko > g_ether gadget: using random self ethernet address > g_ether gadget: using random host ethernet address > usb0: MAC 26:07:ba:c0:44:33 > usb0: HOST MAC 96:81:0c:05:4d:e3 > g_ether gadget: Ethernet Gadget, version: Memorial Day 2008 > g_ether gadget: g_ether ready > fsl-usb2-udc: bind to driver g_ether > g_ether gadget: high speed config #1: CDC Ethernet (ECM) > [EMAIL PROTECTED]:~# rmmod g_ether.ko > [ cut here ] > Badness at drivers/usb/gadget/composite.c:871 > [...] > NIP [e10c3454] composite_unbind+0x24/0x15c [g_ether] > LR [e10aa454] usb_gadget_unregister_driver+0x13c/0x164 [fsl_usb2_udc] > Call Trace: > [df145e80] [ff94] 0xff94 (unreliable) > [df145eb0] [e10aa454] usb_gadget_unregister_driver+0x13c/0x164 [fsl_usb2_udc] > [df145ed0] [e10c4c40] usb_composite_unregister+0x3c/0x4c [g_ether] > [df145ee0] [c006bcc0] sys_delete_module+0x130/0x19c > [df145f40] [c00142d8] ret_from_syscall+0x0/0x38 > [...] > unregistered gadget driver 'g_ether' > > Signed-off-by: Anton Vorontsov <[EMAIL PROTECTED]> Acked-by: David Brownell <[EMAIL PROTECTED]> > --- > > v2: > - Comment update per Alan Stern review. > > drivers/usb/gadget/fsl_usb2_udc.c |3 +++ > 1 files changed, 3 insertions(+), 0 deletions(-) > > diff --git a/drivers/usb/gadget/fsl_usb2_udc.c > b/drivers/usb/gadget/fsl_usb2_udc.c > index 091bb55..f3c6703 100644 > --- a/drivers/usb/gadget/fsl_usb2_udc.c > +++ b/drivers/usb/gadget/fsl_usb2_udc.c > @@ -1836,6 +1836,9 @@ int usb_gadget_unregister_driver(struct > usb_gadget_driver *driver) > nuke(loop_ep, -ESHUTDOWN); > spin_unlock_irqrestore(&udc_controller->lock, flags); > > + /* report disconnect; the controller is already quiesced */ > + driver->disconnect(&udc_controller->gadget); > + > /* unbind gadget and unhook driver. */ > driver->unbind(&udc_controller->gadget); > udc_controller->gadget.dev.driver = NULL; > -- > 1.5.6.3 > -- > To unsubscribe from this list: send the line "unsubscribe linux-usb" in > the body of a message to [EMAIL PROTECTED] > More majordomo info at http://vger.kernel.org/majordomo-info.html > > ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH v2] usb/fsl_qe_udc: Report disconnect before unbinding
On Thursday 13 November 2008, Anton Vorontsov wrote: > Gadgets disable endpoints in their disconnect callbacks, so > we must call disconnect before unbinding. This also fixes > muram memory leak, since we free muram in the qe_ep_disable(). > > But mainly the patch fixes following badness: > > [EMAIL PROTECTED]:~# insmod fsl_qe_udc.ko > fsl_qe_udc: Freescale QE/CPM USB Device Controller driver, 1.0 > fsl_qe_udc e01006c0.usb: QE USB controller initialized as device > [EMAIL PROTECTED]:~# insmod g_ether.ko > g_ether gadget: using random self ethernet address > g_ether gadget: using random host ethernet address > usb0: MAC be:2d:3c:fa:be:f0 > usb0: HOST MAC 62:b8:6a:df:38:66 > g_ether gadget: Ethernet Gadget, version: Memorial Day 2008 > g_ether gadget: g_ether ready > fsl_qe_udc e01006c0.usb: fsl_qe_udc bind to driver g_ether > g_ether gadget: high speed config #1: CDC Ethernet (ECM) > [EMAIL PROTECTED]:~# rmmod g_ether.ko > [ cut here ] > Badness at drivers/usb/gadget/composite.c:871 > [...] > NIP [d10c1374] composite_unbind+0x24/0x15c [g_ether] > LR [d10a82f4] usb_gadget_unregister_driver+0x128/0x168 [fsl_qe_udc] > Call Trace: > [cfb93e80] [cfb1f3a0] 0xcfb1f3a0 (unreliable) > [cfb93eb0] [d10a82f4] usb_gadget_unregister_driver+0x128/0x168 [fsl_qe_udc] > [cfb93ed0] [d10c2a3c] usb_composite_unregister+0x3c/0x4c [g_ether] > [cfb93ee0] [c006bde0] sys_delete_module+0x130/0x19c > [cfb93f40] [c00142d8] ret_from_syscall+0x0/0x38 > [...] > fsl_qe_udc e01006c0.usb: unregistered gadget driver 'g_ether' > > Signed-off-by: Anton Vorontsov <[EMAIL PROTECTED]> Acked-by: David Brownell <[EMAIL PROTECTED]> > --- > > v2: > - Comment update per Alan Stern review. > > replaces > usb.current/usb-fsl_qe_udc-report-disconnect-before-unbinding-fixing-oopses.patch > > drivers/usb/gadget/fsl_qe_udc.c |3 +++ > 1 files changed, 3 insertions(+), 0 deletions(-) > > diff --git a/drivers/usb/gadget/fsl_qe_udc.c b/drivers/usb/gadget/fsl_qe_udc.c > index 37c8575..3d6c956 100644 > --- a/drivers/usb/gadget/fsl_qe_udc.c > +++ b/drivers/usb/gadget/fsl_qe_udc.c > @@ -2382,6 +2382,9 @@ int usb_gadget_unregister_driver(struct > usb_gadget_driver *driver) > nuke(loop_ep, -ESHUTDOWN); > spin_unlock_irqrestore(&udc_controller->lock, flags); > > + /* report disconnect; the controller is already quiesced */ > + driver->disconnect(&udc_controller->gadget); > + > /* unbind gadget and unhook driver. */ > driver->unbind(&udc_controller->gadget); > udc_controller->gadget.dev.driver = NULL; > -- > 1.5.6.3 > -- > To unsubscribe from this list: send the line "unsubscribe linux-usb" in > the body of a message to [EMAIL PROTECTED] > More majordomo info at http://vger.kernel.org/majordomo-info.html > > ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [RFC PATCH] rtc: add rtc_systohc for ntp use
On Tuesday 11 November 2008, David Woodhouse wrote: > I believe you were also concerned that some device wouldn't want the > behaviour given by the existing sync_cmos_clock() function and workqueue > stuff in kernel/ntp.c, where we update the clock precisely half-way > through the second? That's a problem, yes. I've never heard of any RTC that wants such delays ... except for the PC's "CMOS" RTC. There was some discussion about how to expose that knowledge to userspace too. The "hwclock" utility always imposes that delay, and it shouldn't (except when talking to a PC RTC). > We should probably rip that code out of ntp.c (or just disable it ifdef > CONFIG_RTC_CLASS), and provide our own version of notify_cmos_timer(). My suggestion for in-kernel code is that "struct rtc_device" just grow a field like "unsigned settime_delay_msec" which would never be initialized except by rtc-cmos (which uses 500) ... and the NTP sync code would use that value. For out-of-kernel use, that value could be extracted with an ioctl(), and used similarly. > The workqueue stuff to trigger at half-past the second could be kept as > a library function which most RTC devices would use, but they'd have the > option to use their own instead. I thought the workqueue stuff was primarily there to make sure that RTC was always updated in task context -- so it can grab the relevant mutex -- and the half-second delay was legacy PC code ... - Dave ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [RFC PATCH] rtc: add rtc_systohc for ntp use
Yeah, we should have one of these. :) On Monday 10 November 2008, Alessandro Zummo wrote: > @@ -30,7 +30,7 @@ config RTC_HCTOSYS > unnecessary fsck runs at boot time, and to network better. > > config RTC_HCTOSYS_DEVICE > - string "RTC used to set the system time" > + string "RTC used to set the system time on startup and resume" > depends on RTC_HCTOSYS = y > default "rtc0" > help > @@ -52,6 +52,23 @@ config RTC_HCTOSYS_DEVICE > sleep states. Do not specify an RTC here unless it stays powered > during all this system's supported sleep states. > > +config RTC_SYSTOHC > + bool "Set RTC from system time in NTP sync mode" > + depends on RTC_CLASS = y > + default y > + help > + If you say yes here, the system time (wall clock) will be written > + to the hardware clock every 11 minutes, if the kernel is in NTP > + mode and your platforms supports it. > + > +config RTC_SYSTOHC_DEVICE > + string "RTC used to save the system time in NTP sync mode" Why not just use RTC_HCTOSYS_DEVICE for this? I have a hard time sseeing any other value make sense ... assuming there's only one RTC involved. A better default might be to update all the RTCs on the system. I'm thinking of my trusty test-case here: rtc0 is highly functional (including wake from RTC alarm) but not battery backed, while rtc1 is battery backed but only tracks time. NTP really needs to update both of them ... rtc0 since that's what's used most of the time, and also rtc1 since that's what actually *stores* the time during power off cycles. > +static int rtc_systohc(struct rtc_time *tm) I think "static" will lose, especially since ... > +EXPORT_SYMBOL(rtc_systohc); ... you want other code to call it. ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH 4/7] gpiolib: implement dev_gpiochip_{add,remove} calls
On Wednesday 22 October 2008, Anton Vorontsov wrote: > --- a/drivers/gpio/pcf857x.c > +++ b/drivers/gpio/pcf857x.c > @@ -187,7 +187,7 @@ static int pcf857x_probe(struct i2c_client *client, > struct pcf857x *gpio; > int status; > > - pdata = client->dev.platform_data; > + pdata = pcf857x_get_pdata(client); > if (!pdata) > return -ENODEV; > I suppose that can work. Regardless, some OF-specific code will need to map device tree state into a generic format that's fully decoupled from OF. (And there's something to be said to having that mapping sit in the same directory as the driver needing it.) ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH 4/7] gpiolib: implement dev_gpiochip_{add,remove} calls
On Wednesday 22 October 2008, Benjamin Herrenschmidt wrote: > On Wed, 2008-10-22 at 14:04 -0700, David Brownell wrote: > > > So if we register the board infos after > > > the controller registered, then nobody will probe the board infos. > > > > See above. If you're doing it right, there's no problem. > > That is, scan the OF tables early. Just like PNP tables > > get scanned early, for example. > > It's still pretty yucky in that case to scan the device-tree to convert > it into some kind of fugly board info ... I'd rather have the end > drivers that actually use those GPIOs scan the device-tree directly. Keep in mind that these problems are not specific to GPIOs. And, very important!!, most of the drivers run without OF... Pretty much any little device that needs board-specific customization has the same class of problems: drivers will need a variety of parameters that may are often not sharable with other devices, with idiosyncratic usage. And they hook up to other drivers in arbitrary ways. When PCs have such issues, ACPI hides them from Linux. If those parameters -- potentially including callbacks that escape to FORTH -- are stored in the OF device tree, so be it. But "fugly" sounds like part of that problem domain, so it's no surprise that it maps onto the solution space too... Specifically with respect to GPIOs ... what do you mean by "end driver" though? I previously pointed at one example (Davinci EVM) where one bank of GPIOs is used by about six different drivers ... none of which have any reason to know they're using a pcf8574a vs any other kind of GPIO. Is the "end driver" the IDE/CF driver? The USB OTG driver? The driver sitting the next layer above of one of those? *Some* of the drivers need to touch the GPIOs. Others don't. > But then, I'm not a believer in generic drivers for things > like GPIOs, i2c devices, etc.. :-) I kind of like being able to re-use code myself. ;) It's a win to have *one* pcf8574/5 driver that can be reused -- with a bit of care configuring it into the system -- instead of having every board contribute yet another board-specific hack in drivers/i2c/chips ... And I think such stuff can be done even with OF. - Dave ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH 4/7] gpiolib: implement dev_gpiochip_{add,remove} calls
On Wednesday 22 October 2008, Anton Vorontsov wrote: > > > > So have it live in the __init text section... > > Won't work, unfortunately. I2C devices are created by the > i2c controllers, via drivers/of_i2c.c of_register_i2c_devices(). And I'm pointing out a way to have the normal I2C core code flow do that creation. OF shouldn't need to be so much of a special case. > There is a good reason to do so, the code needs to know > controller's OF node to walk down and register the child nodes > (devices). See drivers/i2c/busses/i2c-mpc.c -- it calls > of_register_i2c_devices() at the end of the probe(). I don't get it. (But then, so much of the OF support seems needlessly convoluted to me ... on top of seeming to be insufficient for configuring board-specific details.) There's an OF device tree, distinct from the Linux driver model tree. Why should there be any obstacle to accessing records from that tree before the relevant driver model nodes have been created? Remember that the various board_info structs get registered before the driver model nodes for which they are templates. Just translate the OF tree data to those templates(*), then register them. I understand that it's currently structured differetnly than that ... consulting the OF tree "late" not early. But that's still newish, and from what I've heard so far it doesn't seem like the best structure either... nothing seems to plug in smoothly. - Dave (*) The role of the board_info structs is very similar to the role of OF device attributes. As is the role of the platform_data ... except that's more specific to the chip involved (and its driver), and expects any callbacks to be in C code not FORTH. ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH 4/7] gpiolib: implement dev_gpiochip_{add,remove} calls
On Wednesday 22 October 2008, Anton Vorontsov wrote: > > > > > So if we register the board infos after > > > the controller registered, then nobody will probe the board infos. > > > > See above. If you're doing it right, there's no problem. > > That is, scan the OF tables early. Just like PNP tables > > get scanned early, for example. > > Heh. If we don't want to be able to make the OF-parsing code > be a module then there is no problem at all. I can use the bus > notifiers. And it is most straightforward solution then. > > But I quite dislike to bloat the kernel image with > maybe-never-used-on-this-board code. So have it live in the __init text section. If you're building a kernel with support for several boards, you know it's necessarily going to be larger than it would be if only one board were supported. But you can shrink kernel size by judicious use of __init sections.. > My aim was to make the > OF-parsing part be a module too. Because in the long run we > need the OF-parsing stuff for _every_ driver that needs > platform data. It's quite expensive to have it always built-in, > don't you think? If it's discarded early, after translating the data from OF format into what the drivers need, there will be no RAM footprint. - Dave ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH 4/7] gpiolib: implement dev_gpiochip_{add,remove} calls
On Wednesday 22 October 2008, Anton Vorontsov wrote: > > > > The board info has another problem though. We can't remove it, thus > > > we can't implement module_exit() for the 'OF glue'. That's not a problem. Why would you want to remove it? > > And try to solve this problem... maybe then things will begin to > > move forward. > > There is another problem: board infos are scanned at the controller > registration time only. Right. Such board description data should be made available early in boot. As a rule: before arch_initcall() finishes, so that subsys_initcall() code can use the associated GPIOs. (It's fairly well acknowledged that init dependency handling has a lot of problems. Until that's fixed ... for GPIOs, the general advice is to make sure everything is available by subsys_initcall time, so the subsystems which rely on GPIOs to initialize -- power switches, resets, etc -- can initialize. That can require i2c adapter drivers to use subsys_initcall, for example.) > So if we register the board infos after > the controller registered, then nobody will probe the board infos. See above. If you're doing it right, there's no problem. That is, scan the OF tables early. Just like PNP tables get scanned early, for example. - Dave ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH 4/7] gpiolib: implement dev_gpiochip_{add,remove} calls
On Tuesday 21 October 2008, Benjamin Herrenschmidt wrote: > The notifier can be registered before the devices, though it's a little > bit fishy and fragile. > > Easier I suppose to just have OF specific hooks in the bus code. Like what I suggested: "chip-aware OF glue drivers". The relevant bus code being the "of_platform_bus_type" infrastructure. Example: instead of Anton's patch #6 modifying the existing pca953x driver, an of_pca953x driver that knows how to poke around in the OF device attributes to (a) create the pca953x_platform_data, (b) call i2c_register_board_info() to make that available later, and then finally (c) vanish, since it's not needed any longer. Better that than either the $SUBJECT patch, or modifying gpiolib to grow OF-specific hooks ... hooks that can at best solve *one* of the problems: which GPIO numbers to use with this chip. The platform data does solve other problems(*) like: (i) how to initialize the polarity inversion register, (ii) arranging to set up other devices only after their GPIOs are ready, (iii) initializing things that device drivers won't always know about, or which may need to be set up before such drivers are available. - Dave (*) A trivial example of (ii) would be LEDs driven by those GPIOs. A less trivial example: see arch/arm/mach-davinci/board-evm.c in current GIT. There are three pcf8574 I2C expanders used for various things ... LEDs, audio PLL, device power supplies, reset lines for external devices, more. ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: Bug in "genirq: record trigger type"
On Monday 20 October 2008, Benjamin Herrenschmidt wrote: > This one is obviously broken and breaks booting on a whole bunch of > machines (including powermac's and thus my G5, it's never good when my > own machine breaks !). > > Nice to see 3 SOB's and one Ack and nobody caught the obvious bug :-) As you saw, that one's fixed. Chris' patch unfortunately didn't get integrated right away. I'm a bit more curious about another potential issue though ... as described in the patch comment: - Make set_irq_type() usage match request_irq() usage: * IRQ_TYPE_NONE should be a NOP; succeed, so irq_chip methods won't have to handle that case any more (many do it wrong). It might be a bit more accurate to say irq_chip.set_type() methods are *inconsistent* in handling IRQ_TYPE_NONE. Previously the set_irq_type() method would pass that down to irq_chip code. I had observed two behaviors, but I thought I observed a third one in some of the PowerPC code: (1) ignore it ... matching request_irq() usage (2) return an error ... nasty (3) assign some irq_chip-specific trigger mode That third behavior might cause a bit of trouble, but I think it was only used during platform init. Someone more attuned to PowerPC might want to check ... - Dave ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH 4/7] gpiolib: implement dev_gpiochip_{add,remove} calls
On Friday 17 October 2008, Anton Vorontsov wrote:
> On Fri, Oct 17, 2008 at 01:24:42PM -0700, David Brownell wrote:
> > On Thursday 16 October 2008, Anton Vorontsov wrote:
> > > +/*
> > > + * Platforms can define their own __dev_ versions to glue gpio_chips
> > > with the
> > > + * architecture-specific code.
> > > + */
> > > +#ifndef __dev_gpiochip_add
> > > +#define __dev_gpiochip_add __dev_gpiochip_add
> > > +static inline int __dev_gpiochip_add(struct device *dev,
> > > + struct gpio_chip *chip)
> > > +{
> > > + chip->dev = dev;
> > > + return gpiochip_add(chip);
> > > +}
> > > +#endif /* __dev_gpiochip_add */
> >
> > This is pretty ugly, especially the implication that *EVERY* gpio_chip
> > provider needs modification to use these calls.
>
> Anyway most of them need some modifications to work with OF...
The changes I saw were just to cope with not having
the system-specific platform_data provided: don't
fail if that pointer is NULL, and arrange for dynamic
allocation of some GPIO numbers.
With OpenFirmware, presumably the implication is that
the relevant data is in the OF device tree...
I think that it *barely* makes sense to allow the chips
to bind to drivers without platform data when there's
not even OF in the environment. ONLY in the case where
the GPIOs are exported through sysfs, in fact, since
otherwise there's no way for other system components
to know those GPIOs even exist!! And even that seems
pretty marginal to me...
> > Surely it would be a lot simpler to just add platform-specific hooks
> > to gpiochip_{add,remove}(), [...]
>
> We have printk and dev_printk. kzalloc and devm_kzalloc (though I
> aware that devm_ are different than just dev_). So I thought that
> dev_gpiochip_* would be logical order of things...
Those aren't platform hook mechanisms though, and
there's no need to modify every driver to use them
in order to work *at all* on OpenFirmware systems.
> If you don't like it, I can readily implement hooks for
> gpiochip_{add,remove}().
It seems a better way to a clean solution, IMO. For
example, the OF hook for adding a gpio_chip might
know that it's got to stuff chip->base with a number
other than "-1" (say, "42") since that was stored in
some property of the device's OF shadow, and other
devices have properties associating them with GPIO
numbers derived from that (3rd gpio on that chip,
42 + 3 == 45) and so forth.
That said ... there's a LOT of configuration that
doesn't seem to me like it can be generic. Pullups,
pulldowns, default values, polarity inversion,
what devices depend on those GPIOs being available
before they can come up (GPIO leds and power switches
come quickly to mind), all kinds of chip-specific
details, and more.
Did you look at providing chip-aware OF glue drivers
for this stuff? Doing stuff like just turn the OF
device properties into the right platform_data, and
maybe runing FORTH bytecodes to do other configuration
magic needed...
- Dave
___
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH 5/7] of/gpio: implement of_dev_gpiochip_{add, remove} calls
On Thursday 16 October 2008, Anton Vorontsov wrote: > + if (of_gc->chip) > + return of_gc->chip; > + return &of_gc->gc; presumably there's a reason not to of_gc->chip = &of_gc->gc; when this gets set up, so this can always be a simple return of_gc->chip; (and inlined)? Needlessly complex... ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH 4/7] gpiolib: implement dev_gpiochip_{add,remove} calls
On Thursday 16 October 2008, Anton Vorontsov wrote:
> +/*
> + * Platforms can define their own __dev_ versions to glue gpio_chips with the
> + * architecture-specific code.
> + */
> +#ifndef __dev_gpiochip_add
> +#define __dev_gpiochip_add __dev_gpiochip_add
> +static inline int __dev_gpiochip_add(struct device *dev,
> + struct gpio_chip *chip)
> +{
> + chip->dev = dev;
> + return gpiochip_add(chip);
> +}
> +#endif /* __dev_gpiochip_add */
This is pretty ugly, especially the implication that *EVERY* gpio_chip
provider needs modification to use these calls.
Surely it would be a lot simpler to just add platform-specific hooks
to gpiochip_{add,remove}(), so that no providers need to be changed??
> +#ifndef __dev_gpiochip_remove
> +#define __dev_gpiochip_remove __dev_gpiochip_remove
> +static inline int __dev_gpiochip_remove(struct device *dev,
> + struct gpio_chip *chip)
> +{
> + return gpiochip_remove(chip);
> +}
> +#endif /* __dev_gpiochip_remove */
___
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH] Modify sysfs gpio export so that "value" displays as 0 or 1
On Friday 10 October 2008, Steven A. Falco wrote: > gpiolib can export GPIOs to userspace via sysfs. This patch modifies > the gpio_value_show() so that any non-zero value is explicitly printed > as "1", rather than whatever numerical value the lower-level driver returns. > > Signed-off-by: Steve Falco I just forwarded this to Andrew (cc LKML) with my signoff ... sorry I forgot to add you to the CC. :( > --- > > diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c > index 8d29405..36bf72b 100644 > --- a/drivers/gpio/gpiolib.c > +++ b/drivers/gpio/gpiolib.c > @@ -237,7 +237,7 @@ static ssize_t gpio_value_show(struct device *dev, > if (!test_bit(FLAG_EXPORT, &desc->flags)) > status = -EIO; > else > - status = sprintf(buf, "%d\n", gpio_get_value_cansleep(gpio)); > + status = sprintf(buf, "%d\n", !!gpio_get_value_cansleep(gpio)); > > mutex_unlock(&sysfs_lock); > return status; > > ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH v6] PPC440EPx gpio driver
On Thursday 09 October 2008, Anton Vorontsov wrote: > > > I've incorporated the other changes, with one exception. I want > > ppc4xx_gpio_get() to return 0 or 1 (rather than Anton's comment that any > > non-zero value is ok), because when you use the new "export feature" in > > sysfs, you see the raw value returned from ppc4xx_gpio_get(). So, without > > the !! in the return statement, you would see a strange value, like 32768 > > instead of 1: > > > > # cd gpio208 > > # cat value > > 32768 > > > > So, I think it is worth sanitizing the return value here. > > I think that nonzero == high assumption is also ok for the userspace. Actually, I was thinking that userspace might be happier not having to deal with that ... so that's worth changing in the sysfs glue. I'd sign off on a patch fixing that. I seem to recall seeing an explicit policy saying sysfs exports booleans as 0/1, at some point, though I can't find it just now. - Dave ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH 2/4] powerpc/qe: new call to revert a gpio to a dedicated function
On Wednesday 24 September 2008, Anton Vorontsov wrote: > > Anyway, just want to thank you for your time and persistence on this > matter, you're forcing others' people brains to *work*. And since you > rejected this approach too, I have no other option but to implement > something else... something better. ;-) I think you have enough pieces in place to get that "something better" _very_ quickly. Or I'd feel worse about abusing those poor little grey cells. ;) ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH] usb: add Freescale QE/CPM USB peripheral controllerdriver
On Wednesday 24 September 2008, Joakim Tjernlund wrote: > > > I see. If one wants to connect with CDC to Windows, what drivers are > > > there for Windows that works well with Linux? > > > > I believe MCCI has some. It also has drivers for a CDC subset, > > pretty much the same one Linux has used forever except wrapped > > with a few extra descriptors to make it practical to identify > > that "SAFE" (!) subset without needing vendor and product IDs. > > Thanks, had a look and it seems like I have to buy these drives. I was > hoping to find some free ones, do you know of such free drivers? No, sorry. Remember MSFT is somewhat rabidly anti-Free... However, we'd be quite happy to have someone more attuned to MS-Windows sort out the remaining issues in the Linux support for RNDIS. ;) - Dave ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH 1/4] gpiolib: make gpio_to_chip() public
On Wednesday 24 September 2008, Anton Vorontsov wrote:
> We'll need this function to write platform-specific hooks to deal
> with pin's dedicated functions. Quite obviously this will work only
> for the platforms with 1-to-1 GPIO to PIN mapping.
>
> This is stopgap solution till we think out and implement a proper
> api (pinlib?).
>
> p.s. This patch actually exports gpio_desc and places gpio_to_chip
> into the asm-generic/gpio.h as `static inline'. This is needed
> to not cause function calls for this trivial translation.
>
> Also, the patch does not export FLAG_*s... the names are too
> generic, and nobody is using them outside of gpiolib.c.
For the record: NAK, still. The concept has problems,
there is no "need" for this. I sketched a cleaner way
to address the issues of the QE USB driver; I'm sure it
would only take an hour or two to code, using what's
already present.
And if I were to approve something like this it would
be a lot simpler, not exposing internals, and with
appropriate kerneldoc. Simpler such as
struct gpio_chip *gpio_to_gpiochip(unsigned gpio)
{
return gpio_to_chip(gpio);
}
EXPORT_SYMBOL_NOTREALLY(gpio_to_gpiochip);
with a declaration in a header. It's not like THIS
version would be performance-critical (unlike the one
inside gpiolib).
- Dave
___
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH] usb: add Freescale QE/CPM USB peripheral controllerdriver
On Tuesday 02 September 2008, Joakim Tjernlund wrote: > > > Noted: AFAIK, RNDIS gadget in Linux doesn't interoperate with windows > > well enough to be production level. Use at your own risk. > > I see. If one wants to connect with CDC to Windows, what drivers are > there for Windows that works well with Linux? I believe MCCI has some. It also has drivers for a CDC subset, pretty much the same one Linux has used forever except wrapped with a few extra descriptors to make it practical to identify that "SAFE" (!) subset without needing vendor and product IDs. - Dave ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH] usb: add Freescale QE/CPM USB peripheral controllerdriver
On Tuesday 02 September 2008, Li Yang-R58472 wrote: > > > Does RNDIS work too? If not, is it possible to add or doesn't > > the HW support it? > > RNDIS is a gadget(protocol) level thing. I believe it can work with > this driver although not tested myself. It should, so long as the QE hardware doesn't try to manage too many USB protocol details. Examples would be caring at all about "config change events" -- SET_CONFIGURATION picking between one of N configurations, and SET_INTERFACE picking between altsettings of a given interface. > Noted: AFAIK, RNDIS gadget in Linux doesn't interoperate with windows > well enough to be production level. Use at your own risk. Something in the past few releases broke so it may not be operating much at all, for that matter... However, it's a fair point that the interoperation requirements for RNDIS are so badly defined that non-Microsoft code has a very hard time being robust. Somewhere in the linux-usb archives is someone's fairly careful summary of a dozen extremely rude ways the MSFT RNDIS stack misbehaves. It'll do things like deciding to stop forwarding packets to certain applications for a while ... then continuing after a few minutes as if nothing was wrong. Blue screens are a possibility. Best to wait a few minutes between plugging and unplugging, while the races between the USB and NET stacks sort themselves out. - Dave ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH] usb: add Freescale QE/CPM USB peripheral controller driver
On Thursday 28 August 2008, Arnd Bergmann wrote:
> If the gadget hardware drivers were registering the device with a
> gadget_bus_type, you could still enforce the "only one protocol"
> rule by binding every protocol to every device in that bus type.
And you'd have to rewrite all the gadget drivers ("protocol")
to work with multiple upstream ports.
That gets messy with e.g. the Ethernet links ... each would
need to be configured with unique ethernet address pairs.
Likewise with serial numbers.
I've learned to just accept complaints in this area as sort
of a price for existing. It's all complaints, no patches.
So obviously the complaints don't have any requirements
backing them. ;)
___
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH] usb: add Freescale QE/CPM USB peripheral controller driver
On Thursday 28 August 2008, Arnd Bergmann wrote: > > +/*- > > + Gadget driver register and unregister. > > + > > --*/ > > +int usb_gadget_register_driver(struct usb_gadget_driver *driver) > > +EXPORT_SYMBOL(usb_gadget_register_driver); > > + > > +int usb_gadget_unregister_driver(struct usb_gadget_driver *driver) > > +EXPORT_SYMBOL(usb_gadget_unregister_driver); > > Not addressing this driver in particular, but the USB gadget layer in > general: This is a horrible interface, since every gadget driver exports > the same symbols, Bad terminology. Gadget drivers are what sit on TOP of peripheral controller drivers ... only peripheral controller drivers touch the actual hardware registers. They export an abstract "gadget" interface. Gadget drivers are what talk *to* that abstract interface. > you can never build a kernel that includes more than > one gadget driver. Even if the drivers are all built as modules, simply > loading one of them prevents loading another one. That's never been a particular requirement. Systems won't get USB branding if they have more than one USB peripheral (upstream) port. Supporting more than one type of controller hardware is at best a pretty esoteric configuration. If you really want to see such stuff ... -ENOPATCH. :) - Dave ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH 0/3] Patches to support QE USB Host Controller
On Thursday 14 August 2008, Laurent Pinchart wrote:
>
> > David, could you bear with gpio_to_chip() exported function, just as
> > a stopgap for a proper api?
>
> I need gpio_to_chip() (or another 'proper API') as well for RTS/CTS
> based flow control in the CPM/CPM2 UART driver.
I'l still say "proper". This should be straightforward; along
the lines of
struct qe_pin {
struct ... *qe_ports; /* includes gpio_chip */
unsigned offset;
};
And instead of having the driver look up a "gpio" for such
non-GPIO usage, have it call something that sets up a qe_pin.
All that infrastructure exists already...
Then drivers can use calls which mux the pin into its "normal"
mode (QE function of some kind), or into its "gpio" mode.
The gpio number would be gpio_chip->base + offset, and the
gpio_chip is visible -- in a fully typesafe manner! -- from
the qe_ports structure.
No type-unsafe interfaces. No confusion between roles of a
given pin. No hidden assumption there's only one kind of GPIO
(backed by QE ports). And ... no need to change any core
structural assumptions of the GPIO framework.
- Dave
___
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH 3/3] USB: driver for Freescale QUICC Engine USB Host Controller
On Wednesday 24 September 2008, Sergei Shtylyov wrote: > > >> ... then the root hub emulation is completely pointless. > >> > > > > It isn't. We always should emulate the root hub. The root hub > > is part and parcel of any USB Host. Even the one-port one. > > Hm, maybe that's what USB core thinks (because UHCI/OHCI/EHCI all > have it) but e.g. MUSB doesn't have the root hub registers... Only the OHCI registers have bit positions matching what the USB spec says for hub status bits. Everything else, including musb_hdrc, has the relevant status encoded in other bits. > I looked at the core and figured that USB core seems to use the root hub > interface for port PM, etc. and expects it to bee present, so it seems > unavoidable indeed... :-/ Or more fundamentally: for enumeration. "Unavoidable" is correct. ;) ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH 2/4] powerpc/qe: new call to revert a gpio to a dedicated function
On Tuesday 23 September 2008, Anton Vorontsov wrote:
> qe_gpio_set_dedicated() is a platform specific function, which is used
> to revert a pin to a dedicated function. Caller should have already
> obtained the gpio via gpio_request().
Note the missing sibling function: putting the pin back into
GPIO mode!! You seem to assume that some of the GPIO calls
will be performing that pinmux function. But those calls are
explicitly defined as NOT incorporating any pinmux tasks.
Also note your nonportable assumption that GPIOs and pins are
the same concept ... they aren't.
You'd be better off calling something other than of_get_gpio()
for those three pins in of_fhci_probe() ... call something
that returns a "qe_pin" structure (e.g. wrapping an instance of
the misnamed qe_gpio_chip plus an offset) which holds the
pinmux primitives you need. Like this one to put the pin into
its normal mode.
When I look at patch 4 of this series I observe that only
two pins are true GPIOs: the optional POWER and SPEED pins.
(External transceiver support?)
> +int qe_gpio_set_dedicated(unsigned int gpio)
> +{
> + struct gpio_chip *gc = gpio_to_chip(gpio);
So the caller must already have requested it, yes -- that's a
needed for any stable mapping between GPIO and controller inside
the GPIO library.
For the record, this single call seems to be the entire reason
motivating that rather ugly patch #1. (Ugly for more than just
the confusion between pin, which is what you need, and GPIO.
There's no need to export those internal data structures.)
And in turn, the reason to want this call is so that you can
have io_port_generate_reset() generate a short reset on the
single downstream USB port. ("Short" meaning "45 msec below
USB spec requirements for root hub resets" ...)
And to top it off ... that driver does gpio_request(), runs
those pins as GPIOs for virtually no time, and then uses
them as "dedicated functions" the rest of the time (after
the reset completes)!!
Which highlights the fact that these pins are fundamentally
NOT used as GPIOs. They're function pins that need brief
detours as GPIOs because, it seems, those functions only
support differential signaling (USB J and K states) instead
of the full set of USB states. (It's not quite clear from
the driver. Are the pins expected to be using a 3-wire
external transciever hookup? 4-wire? 6-wire?)
But there are other requirements for this no-kerneldoc call:
> + struct of_mm_gpio_chip *mm_gc = to_of_mm_gpio_chip(gc);
... it must be part of an of_mm_gpio_chip (in ,
which might seem less odd to me if I read its supporting code).
Can you first ensure that it *is* an of_mm_gpio_chip
instance? When it isn't, this code will oops rudely.
> + struct qe_gpio_chip *qe_gc = to_qe_gpio_chip(mm_gc);
... it must be the qe_gpio_chip flavor (defined only in this
very file, arch/powerpc/sysdev/qe_lib/gpio.c); IMO the code
would be cleaner if you just did
qe_gc = container_of(gpio_to_chip(gpio),
struct qe_gpio_chip, mm_gc.of_gc.gc);
and had just one pointer (not three!) for all these purposes.
(And cleaner still if it didn't require whacking the GPIO
framework out of shape to have a hope of working.)
But again: you're trusting, with no evident basis for that
trust, that it's a qe_gpio_chip instance. Oops if it isn't.
Much better for these calls to take e.g. a "qe_pin" parameter,
struct pointer or whatever ... not a GPIO number.
> + struct qe_pio_regs __iomem *regs = mm_gc->regs;
> + struct qe_pio_regs *sregs = &qe_gc->saved_regs;
> + u8 pin = gpio - gc->base;
> + u32 mask1 = 1 << (QE_PIO_PINS - (pin + 1));
> + u32 mask2 = 0x3 << (QE_PIO_PINS - (pin % (QE_PIO_PINS / 2) + 1) * 2);
> + bool second_reg = pin > (QE_PIO_PINS / 2) - 1;
> ...
___
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH 2/4] powerpc/qe: new call to revert a gpio to a dedicated function
On Wednesday 24 September 2008, Anton Vorontsov wrote: > > what do you mean by dedicated function.. be a bit clearer in the commit > > log. > > This term is from the QE spec, I didn't invent anything. ;-) > > "Each pin in the I/O ports can be configured as a general-purpose > I/O signal or as a dedicated peripheral interface signal. ...many > dedicated peripheral functions are multiplexed onto the ports." Which, to me, highlights the point I've made previously: the right abstraction for you to work with is "pin", not GPIO. You need to switch a QE "pin" from one of its roles to the other. Evil things, like oopsing, will happen if you try to use this call on a GPIO that's not backed by a QE pin. ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH 3/4] USB: Protect hcd.h from multiple inclusions
On Tuesday 23 September 2008, Anton Vorontsov wrote: > This will let us use this header in other header files. > Will be needed for the FHCI USB Host driver. > > Signed-off-by: Anton Vorontsov <[EMAIL PROTECTED]> This seems innocuous. I'm content with Greg merging it into his tree for 2.6.28-rc0 merge ... Acked-by: David Brownell <[EMAIL PROTECTED]> > --- > drivers/usb/core/hcd.h |4 > 1 files changed, 4 insertions(+), 0 deletions(-) > > diff --git a/drivers/usb/core/hcd.h b/drivers/usb/core/hcd.h > index e710ce0..66b64d7 100644 > --- a/drivers/usb/core/hcd.h > +++ b/drivers/usb/core/hcd.h > @@ -16,6 +16,8 @@ > * Inc., 675 Mass Ave, Cambridge, MA 02139, USA. > */ > > +#ifndef __USB_CORE_HCD_H > +#define __USB_CORE_HCD_H > > #ifdef __KERNEL__ > > @@ -483,3 +485,5 @@ static inline void usbmon_urb_complete(struct usb_bus > *bus, struct urb *urb, > extern struct rw_semaphore ehci_cf_port_reset_rwsem; > > #endif /* __KERNEL__ */ > + > +#endif /* __USB_CORE_HCD_H */ > -- > 1.5.6.3 > > ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [spi-devel-general] [PATCH 1/4] [SPI] [POWERPC] spi_mpc83xx: handles Freescale MPC8610 as well
On Friday 16 May 2008, Scott Wood wrote: > On Fri, May 16, 2008 at 08:50:52PM +0400, Anton Vorontsov wrote: > > config SPI_MPC83xx > > tristate "Freescale MPC83xx/QUICC Engine SPI controller" > > - depends on SPI_MASTER && (PPC_83xx || QUICC_ENGINE) && EXPERIMENTAL > > + depends on SPI_MASTER && (PPC_83xx || QUICC_ENGINE || PPC_86xx) && > > EXPERIMENTAL > > How about "depends on SPI_MASTER && FSL_SOC && EXPERIMENTAL"? > > Plus, we should change the option text to something like "MPC8xxx SPI > controller". Is there an approved-by-powerpc-folk version of this patch yet? (Noting that SPI_MASTER is now implicit ...) ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH v3 0/4 REPOST] OF infrastructure for SPI devices
On Friday 25 July 2008, Grant Likely wrote: > I don't know what to do with these patches. I'd really like to see them > in .27, and now that akpm has cleared his queue, the prerequisite patch > has been merged so they are ready to go in. However, even though there > has been favourable reception on the SPI and linuxppc lists for these > changes I don't have any acks from anybody yet. > > David, can you please reply on if you are okay with these patches > getting merged? If so, do you want me to merge them via my tree, or > do you want to pick them up? OK ... to recap, #1 and #3 are OF-specific, I'll be agnostic. If other OF folk think it's OK, great. Some cleanup was needed for #2, but it was basically ok ... I'd like to see the final version, and if it goes via an OF push that's OK by me. (Though I'd like to see that change make it into 2.6.27 regardless, so let me know.) And #4 isn't quite cooked yet. ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH v3 4/4] powerpc/mpc5200: Add mpc5200-spi (non-PSC) device driver
On Friday 25 July 2008, Grant Likely wrote:
> From: Grant Likely <[EMAIL PROTECTED]>
>
> Adds support for the dedicated SPI device on the Freescale MPC5200(b)
> SoC.
It'd be a bit more clear if you said dedicated SPI "controller";
"device" sounds like it's a "struct spi_device".
Capsule summary: fault handling needs work. (Details below.)
> Signed-off-by: Grant Likely <[EMAIL PROTECTED]>
> ---
>
> drivers/spi/Kconfig |8 +
> drivers/spi/Makefile|1
> drivers/spi/mpc52xx_spi.c | 595
> +++
> include/linux/spi/mpc52xx_spi.h | 10 +
> 4 files changed, 614 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig
> index 2303521..68e4a4a 100644
> --- a/drivers/spi/Kconfig
> +++ b/drivers/spi/Kconfig
> @@ -116,6 +116,14 @@ config SPI_LM70_LLP
> which interfaces to an LM70 temperature sensor using
> a parallel port.
>
> +config SPI_MPC52xx
> + tristate "Freescale MPC52xx SPI (non-PSC) controller support"
> + depends on PPC_MPC52xx && SPI
> + select SPI_MASTER_OF
> + help
> + This drivers supports the MPC52xx SPI controller in master SPI
> + mode.
> +
> config SPI_MPC52xx_PSC
> tristate "Freescale MPC52xx PSC SPI controller"
> depends on PPC_MPC52xx && EXPERIMENTAL
> diff --git a/drivers/spi/Makefile b/drivers/spi/Makefile
> index 7fca043..340b878 100644
> --- a/drivers/spi/Makefile
> +++ b/drivers/spi/Makefile
> @@ -22,6 +22,7 @@ obj-$(CONFIG_SPI_PXA2XX)+= pxa2xx_spi.o
> obj-$(CONFIG_SPI_OMAP_UWIRE) += omap_uwire.o
> obj-$(CONFIG_SPI_OMAP24XX) += omap2_mcspi.o
> obj-$(CONFIG_SPI_MPC52xx_PSC)+= mpc52xx_psc_spi.o
> +obj-$(CONFIG_SPI_MPC52xx)+= mpc52xx_spi.o
> obj-$(CONFIG_SPI_MPC83xx)+= spi_mpc83xx.o
> obj-$(CONFIG_SPI_S3C24XX_GPIO) += spi_s3c24xx_gpio.o
> obj-$(CONFIG_SPI_S3C24XX)+= spi_s3c24xx.o
> diff --git a/drivers/spi/mpc52xx_spi.c b/drivers/spi/mpc52xx_spi.c
> new file mode 100644
> index 000..453690f
> --- /dev/null
> +++ b/drivers/spi/mpc52xx_spi.c
> @@ -0,0 +1,595 @@
> +/*
> + * MPC52xx SPI master driver.
> + * Copyright (C) 2008 Secret Lab Technologies Ltd.
> + *
> + * This is the driver for the MPC5200's dedicated SPI device (not for a
> + * PSC in SPI mode)
> + */
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +MODULE_AUTHOR("Grant Likely <[EMAIL PROTECTED]>");
> +MODULE_DESCRIPTION("MPC52xx SPI (non-PSC) Driver");
> +MODULE_LICENSE("GPL");
> +
> +/* Register offsets */
> +#define SPI_CTRL10x00
> +#define SPI_CTRL1_SPIE (1 << 7)
> +#define SPI_CTRL1_SPE(1 << 6)
> +#define SPI_CTRL1_MSTR (1 << 4)
> +#define SPI_CTRL1_CPOL (1 << 3)
> +#define SPI_CTRL1_CPHA (1 << 2)
> +#define SPI_CTRL1_SSOE (1 << 1)
> +#define SPI_CTRL1_LSBFE (1 << 0)
> +
> +#define SPI_CTRL20x01
> +#define SPI_BRR 0x04
> +
> +#define SPI_STATUS 0x05
> +#define SPI_STATUS_SPIF (1 << 7)
> +#define SPI_STATUS_WCOL (1 << 6)
> +#define SPI_STATUS_MODF (1 << 4)
> +
> +#define SPI_DATA 0x09
> +#define SPI_PORTDATA 0x0d
> +#define SPI_DATADIR 0x10
> +
> +/* FSM state return values */
> +#define FSM_STOP 0
> +#define FSM_POLL 1
> +#define FSM_CONTINUE 2
> +
> +/* Driver internal data */
> +struct mpc52xx_spi {
> + struct spi_master *master;
> + u32 sysclk;
> + void __iomem *regs;
> + int irq0; /* MODF irq */
> + int irq1; /* SPIF irq */
> + int ipb_freq;
> +
> + /* Statistics */
> + int msg_count;
> + int wcol_count;
> + int wcol_ticks;
> + u32 wcol_tx_timestamp;
> + int modf_count;
> + int byte_count;
> +
> + /* Hooks for platform modification of behaviour */
> + void (*premessage)(struct spi_message *m, void *context);
> + void *premessage_context;
> +
> + struct list_head queue; /* queue of pending messages */
> + spinlock_t lock;
> + struct work_struct work;
> +
> +
> + /* Details of current transfer (length, and buffer pointers) */
> + struct spi_message *message;/* current message */
> + struct spi_transfer *transfer; /* current transfer */
> + int (*state)(int irq, struct mpc52xx_spi *ms, u8 status, u8 data);
> + int len;
> + int timestamp;
> + u8 *rx_buf;
> + const u8 *tx_buf;
> + int cs_change;
> +};
> +
> +/*
> + * CS control function
> + */
> +static void mpc52xx_spi_chipsel(struct mpc52xx_spi *ms, int value)
> +{
> + if (value)
> + writeb(0, ms->regs + SPI_PORTDATA); /* Assert SS pin */
> + else
> + writeb(0x08, ms->regs + SPI_PORTDATA); /* Deassert SS pin */
> +}
> +
> +/*
> + * Start a new transfe
Re: [PATCH] powerpc/mpc5200: Fix locking on mpc52xx_spi driver
Applying patch mpc52xx-spi-fix0.patch
patching file drivers/spi/mpc52xx_spi.c
Hunk #1 FAILED at 149.
Hunk #2 succeeded at 154 (offset -7 lines).
Hunk #3 succeeded at 311 (offset -7 lines).
1 out of 3 hunks FAILED -- rejects in file drivers/spi/mpc52xx_spi.c
Patch mpc52xx-spi-fix0.patch does not apply (enforce with -f)
... looks like chunk #1 was against something other than
what you posted, and was partly reversed ..
> +static irqreturn_t mpc52xx_spi_irq(int irq, void *_ms)
> +{
> + struct mpc52xx_spi *ms = _ms;
> + spin_lock(&ms->lock);
Blank line after variable declaration blocks please ... and
also, since you're not using IRQF_DISABLED when you request
this IRQ, you've got locking trouble here. Either specify
IRQF_DISABLED (my preference) or use spin_lock_irqsave().
> + mpc52xx_spi_fsm_process(irq, ms);
> + spin_unlock(&ms->lock);
> return IRQ_HANDLED;
> }
___
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH v3 2/4] spi: split up spi_new_device() to allow two stage registration.
On Friday 25 July 2008, Grant Likely wrote:
> From: Grant Likely <[EMAIL PROTECTED]>
>
> spi_new_device() allocates and registers an spi device all in one swoop.
> If the driver needs to add extra data to the spi_device before it is
> registered, then this causes problems.
Mention an example please ... you have dev->archdata in mind, yes?
> This patch splits the allocation and registration portions of code out
> of spi_new_device() and creates three new functions; spi_alloc_device(),
> spi_register_device(), and spi_device_release().
Comment needs fixing: *TWO* new functions, spi_device_release() was
not needed ...
> spi_new_device() is
> modified to use the new functions for allocation and registration.
> None of the existing users of spi_new_device() should be affected by
> this change.
>
> Drivers using the new API can forego the use of an spi_board_info
Strike "an". :)
> structure to describe the device layout and populate data into the
> spi_device structure directly.
>
> This change is in preparation for adding an OF device tree parser to
> generate spi_devices based on data in the device tree.
>
> Signed-off-by: Grant Likely <[EMAIL PROTECTED]>
Given the comment fixes noted above and below:
Acked-by: David Brownell <[EMAIL PROTECTED]>
Thanks.
> ---
>
> drivers/spi/spi.c | 139
> ---
> include/linux/spi/spi.h | 10 +++
> 2 files changed, 105 insertions(+), 44 deletions(-)
>
> diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
> index ecca4a6..2077ef5 100644
> --- a/drivers/spi/spi.c
> +++ b/drivers/spi/spi.c
> @@ -178,6 +178,96 @@ struct boardinfo {
> static LIST_HEAD(board_list);
> static DEFINE_MUTEX(board_lock);
>
> +/**
> + * spi_alloc_device - Allocate a new SPI device
> + * @master: Controller to which device is connected
> + * Context: can sleep
> + *
> + * Allows a driver to allocate and initialize and spi_device without
"a" spi_device
> + * registering it immediately. This allows a driver to directly
> + * fill the spi_device with device parameters before calling
> + * spi_add_device() on it.
> + *
> + * Caller is responsible to call spi_add_device() on the returned
> + * spi_device structure to add it to the SPI master. If the caller
> + * needs to discard the spi_device without adding it, then it should
> + * call spi_dev_put() on it.
> + *
> + * Returns a pointer to the new device, or NULL.
> + */
> +struct spi_device *spi_alloc_device(struct spi_master *master)
> +{
> + struct spi_device *spi;
> + struct device *dev = master->dev.parent;
> +
> + if (!spi_master_get(master))
> + return NULL;
> +
> + spi = kzalloc(sizeof *spi, GFP_KERNEL);
> + if (!spi) {
> + dev_err(dev, "cannot alloc spi_device\n");
> + spi_master_put(master);
> + return NULL;
> + }
> +
> + spi->master = master;
> + spi->dev.parent = dev;
> + spi->dev.bus = &spi_bus_type;
> + spi->dev.release = spidev_release;
> + device_initialize(&spi->dev);
> + return spi;
> +}
> +EXPORT_SYMBOL_GPL(spi_alloc_device);
> +
> +/**
> + * spi_add_device - Add an spi_device allocated with spi_alloc_device
Strike "an".
> + * @spi: spi_device to register
> + *
> + * Companion function to spi_alloc_device. Devices allocated with
> + * spi_alloc_device can be added onto the spi bus with this function.
> + *
> + * Returns 0 on success; non-zero on failure
> + */
> +int spi_add_device(struct spi_device *spi)
> +{
> + struct device *dev = spi->master->dev.parent;
> + int status;
> +
> + /* Chipselects are numbered 0..max; validate. */
> + if (spi->chip_select >= spi->master->num_chipselect) {
> + dev_err(dev, "cs%d > max %d\n",
Nit; "cs%d >= max %d" ... no need to carry this minor bug forward.
> + spi->chip_select,
> + spi->master->num_chipselect);
> + return -EINVAL;
> + }
> +
> + /* Set the bus ID string */
> + snprintf(spi->dev.bus_id, sizeof spi->dev.bus_id,
> + "%s.%u", spi->master->dev.bus_id,
> + spi->chip_select);
> +
> + /* drivers may modify this initial i/o setup */
> + status = spi->master->setup(spi);
Hmm, I just noticed this *pre-existing* bug: if there's already
a device with this chipselect (so the device_add will fail, later),
its configuration will be trashed here. That&
Re: latest tree build failure -- cpm uart & gpio
On Tuesday 22 July 2008, Anton Vorontsov wrote: > On Tue, Jul 22, 2008 at 08:54:16AM -0500, Kumar Gala wrote: > > can someone look at the following compile failure in linus's tree. I'm > > guessing part of this has to do with Alan's tty changes (and might > > already be addressed?). > > > > include/asm-generic/gpio.h:131: error: implicit declaration of function > > 'gpio_get_value' > > include/asm-generic/gpio.h:137: error: implicit declaration of function > > 'gpio_set_value' > > I think this patch should help: > > [OF] of_gpio: should use new header > http://patchwork.ozlabs.org/linuxppc/patch?id=18750 Is that confirmed as fixing this, vs just being the Right Thing To Do? > David, can you please Ack it? See Benjamin's mail: > http://ozlabs.org/pipermail/linuxppc-dev/2008-July/060109.html If you like ... but this is an OF-specific change, making it conform with the interface spec, so I wouldn't expect this to need more approvals than it's already got. Acked-by: David Brownell <[EMAIL PROTECTED]> > > -- > Anton Vorontsov > email: [EMAIL PROTECTED] > irc://irc.freenode.net/bd2 > ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH 2/4] spi: split up spi_new_device() to allow two stage registration.
On Tuesday 17 June 2008, Grant Likely wrote: > >>> This patch splits the allocation and registration portions of code out > >>> of spi_new_device() and creates three new functions; spi_alloc_device(), > >>> spi_register_device(), and spi_device_release(). > >> > >> I have no problem with the first two, but why the last? > >> > >> If the devices are always allocated by spi_alloc_device() as > >> they should be -- probably through an intermediary -- the > >> only public function necessary for that cleanup should be > >> the existing spi_dev_put(). > > > > Ah, okay. I'm still a bit fuzzy on the device model conventions. > > I'll remove that then. > > I've dug into this some more. spi_alloc_device only allocates the > memory. It doesn't call device_initialize() to initialize the kref. Well, the driver model idiom is initialize() then add(), with register() calls combining the two. An alloc() is just a bit outside those core idioms ... But one alloc() example is platform_device_alloc(), which does the device_initialize() call ... followed by platform_device_add(). The spi_new_device() call does a bunch of stuff beyond a register(), but it also calls device_register(). > All of that behaviour is handled within device_register(). Therefore > if a driver uses spi_alloc_device() and then if a later part of the > initialization fails before spi_register_device() is called, then the > alloc'd memory needs to be freed, but spi_dev_put() won't work because > the kobj isn't set up so I need another function to handle freeing it > in on a failure path. I see ... > Should I switch things around to do device_initialize() in the alloc > function Yes. > and call device_add() instead of device_register() in the > spi_register_device() function? You should also rename it to spi_add_device(), since register() calls always do the initialize() rather than having it done for them in advance. People rely on those names supporting that pattern (as they should). > Is that sufficient to make put_device() work? Looks like it to me. Calling device_initialize() will do a kobject_init(), which is documented as requiring a kobject_put() to clean up ... that's all put_device() will ever do. - Dave ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH 2/4] spi: split up spi_new_device() to allow two stage registration.
On Friday 23 May 2008, Grant Likely wrote: > Question: spi_alloc_device() (and the original code) does a > spi_master_get() on the spi_master device. Doesn't spi_master_put() > need to be called when the device is discarded? spi_dev_put() doesn't > do that explicitly; is it an implicit operation after a device has > been deregistered from the spi_master? Depends whether or not the add() has been done to hook things into the driver model tree, as I recall. The add() presumes things are properly refcounted. When you make a driver model tree node vanish, its associated refcounts get updated too. - Dave ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [i2c] [PATCH] Convert i2c-mpc from a platform driver to an of_platform one
On Sunday 29 June 2008, Jean Delvare wrote: > > > After the i2c adapter registers itself, of_register_i2c_devices() is called > > which walks through the child nodes of the i2c adapter node in the device > > tree. Each child node is an i2c device, and it immediately get > > registered with the adapter. Because this ensures that i2c device > > registration always happens after adapter registration, and since the > > pointer to the i2c_adapter is known, then i2c_new_device() can be used > > directly without ever needing to know the bus number. > > Ah, OK. If you use i2c_new_device() then it's alright. Right. Conceptually the way that the i2c core uses "numbered" adapters and registered board_info could be viewed as a way to let platforms avoid tracking that stuff themselves. Since the of_* framework is already tracking that, there's no big win in trying to have i2c-core track that too, on its behalf. - Dave ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [RFC] OpenFirmware bindings for the MMC-over-SPI driver
On Friday 23 May 2008, Anton Vorontsov wrote: > > This is second attempt to write the OpenFirmware bindings for the > MMC-over-SPI (and SPI bindings in general). Summary: an OF-specific wrapper around the mmc_spi platform code. I think a wrapper to encapsulate all the OF-specific knowledge makes much sense here. The only thing that looks odd to me about this is that the wrapper is a spi_device rather than an of_device. To me it makes more sense to just have an of_device setting up the right spi_device. (Though maybe I missed some discussion about why that can't work.) Example: drivers/usb/host/sl811_cs.c does that for PCMCIA card that just wraps an sl811 chip that's more often used standalone. The PCMCIA descriptors being analagous to OF device table entries. - Dave ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH 3/4] spi: Add OF binding support for SPI busses
On Saturday 24 May 2008, Grant Likely wrote: > > Isn't the same true for drivers/of/gpio.c or drivers/of/of_i2c.c, as well? > > I would argue 'yes!' ... all the more reason to have the SPI glue go there too, matching the ACPI/PCI precedent as well as those others! ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH 3/4] spi: Add OF binding support for SPI busses
On Saturday 24 May 2008, Grant Likely wrote: > >>> > +++ b/drivers/spi/spi_of.c > >>> > >>> I think better placement for this is drivers/of, no? > >> > >> Yes please. > > > > Okay, I wasn't sure. Will do. > > I'm having second thoughts about this. I think this code is more SPI > centric than it is OF centric. ie. it is usable by all spi masters in > an OF enabled system, but it is not usable by all OF devices in an SPI > enabled system. It's not usable by *any* SPI master on a non-OF system though. So in that sense it's far more about OF setup than it is about SPI. > Or, in other words; it adds OF support to SPI, not > the other way around. I think drivers/spi is the right place for this > to live. I'd still rather see such translations in the OF-specific part of the source tree. Like drivers/acpi/pci_*.c code, this has more to do with the firmware interface than with bus (SPI) interface. Arguments could be made both ways here, but for the moment it makes more sense to me to keep this type of platform glue (be it OF, ACPI, arch-specific setup code, or whatever) together in the source tree and apart from the bus-specific code. Where do the proposed patches gluing OF to I2C live, or has that been settled yet? - Dave ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [spi-devel-general] [PATCH 3/4] spi: Add OF binding support for SPI busses
On Friday 23 May 2008, Grant Likely wrote: > Very good point. Okay, so we cannot assume any correlation between > the number of CS lines and the number of child nodes to the SPI bus. That wasn't what I was implying ... all the devices hooked up to a given chipselect should be viewed as a single (albeit composite) child node. Now, the driver for that child node may want to expose lots of substructure. But that's no different from any other complex device, whose protocol happens to embed some notion of component addressing. It's just that in the case I mentioned, that addressing is a bit more externally visible than it is in some other cases. - Dave ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [spi-devel-general] [PATCH 3/4] spi: Add OF binding support for SPI busses
On Wednesday 21 May 2008, Grant Likely wrote:
> > spi-controller {
> > #address-cells = 2;
> > #size-cells = 0;
> > [EMAIL PROTECTED],f000 { reg = < 0 f000 >; } // CS 0, SPI address f000
> > [EMAIL PROTECTED],f000 { reg = < 1 f000 >; } // CS 1, SPI address f000
> > [EMAIL PROTECTED],ff00 { reg = < 1 ff00 >; } // CS 1, SPI address ff00
> > }
>
> For SPI the CS # *is* the address. :-)
>
> Unlike I2C, SPI doesn't impose any protocol on the data. It is all
> anonymous data out, anonymous data in, a clock and a chip select.
Very true ... but then there are SPI chips which embed addressing.
I have in mind the mcp23s08 (and mcp23s17) GPIO expanders, which
support up to four chips wired in parallel on a given chipselect.
The devices are distinguished by how two address pins are wired;
and two bits in the command byte must match them. (I think they
just recycled an I2C design into the SPI world.)
- Dave
___
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: SD-MMC over SPI on PSC6
On Thursday 22 May 2008, Fabio Tosetto wrote: > <*> MMC host test driver Unwise unless you really want to trash every MMC or SD card you insert ... You might consider not crossposting to such a large proportion of the free world, by the way. ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH 3/4] spi: Add OF binding support for SPI busses
On Wednesday 21 May 2008, Anton Vorontsov wrote: > > +++ b/drivers/spi/spi_of.c > > I think better placement for this is drivers/of, no? Yes please. ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH 3/4] spi: Add OF binding support for SPI busses
On Friday 16 May 2008, Grant Likely wrote: > In my mind; platform_data and the device tree are all about the same > thing: representation. In other words, how to describe the > configuration of the hardware independent of the driver itself. Platform_data isn't what I'd call independent of drivers. The reason the data is there in the first place is that the driver needs it ... and chose not to hard-wire it. > One of the things I find rather interesting is just how frequently > drivers using platform data structures have a big block of code which > simply copy pdata fields into identically named fields in the device > private data... ... because platform data was designed as a partial template for that driver, letting it do that. (Sometimes without even doing scale conversions.) As drivers grow functionally, they sometimes end up needing more platform data fields, to expose data that previously didn't matter. Whether that data can usefully be stored in flash (or ROM) and handed out through the bootloader is something of a manufacturing issue. - Dave ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH 2/4] spi: split up spi_new_device() to allow two stage registration.
On Friday 16 May 2008, Grant Likely wrote: > > This patch splits the allocation and registration portions of code out > of spi_new_device() and creates three new functions; spi_alloc_device(), > spi_register_device(), and spi_device_release(). I have no problem with the first two, but why the last? If the devices are always allocated by spi_alloc_device() as they should be -- probably through an intermediary -- the only public function necessary for that cleanup should be the existing spi_dev_put(). - Dave ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [RFC] GPIO-based flow control in the cpm_uart driver
On Tuesday 15 April 2008, Laurent Pinchart wrote: > Or maybe some kind of gpio_set_option() with flags specific to the > controller ? This could be used to enable open-drain outputs or internal > pull-ups for instance. That presumes that the pin config is associated with the GPIO controller, rather than being an independent modules... In the cases that a platform has such an association, there should be no problem just using a platform-specific code to do the relevant magic. - Dave ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [RFC] GPIO-based flow control in the cpm_uart driver
On Tuesday 15 April 2008, Laurent Pinchart wrote: > I'm implementing flow control and modem control lines support in the > cpm_uart driver. > > The implementation is based on the GPIO lib. Modem control lines are > described in the device tree as GPIO resources and accessed through the OF > GPIO bindings. The I/O ports have to be initialized as GPIOs in the > platform-specific code. I don't follow the "have to be" ... why couldn't the platform setup code know that if a given UART is being used with hardware handshaking, that means a particular pin mux config should be used? Are there maybe some on-chip routing options, so that for example RTS2 could come out on any of three different balls? (In which case that setup code could just be told *which* config to use...) In this case I'm asking specifically about the normal all-works-ok situation ... no errata or similar complications I mention below. > An option would be to export gpio_to_chip from drivers/gpio/gpiolib.c > and use cpm1/2_set_pin in the cpm_uart driver. Help me to understand this a bit. UARTs are a fair example of places where I've seen such pin reconfiguration be useful, but it's never seemed to be generalizable except possibly as callbacks to SOC-specific code (which don't imply updating generic programming interfaces). I've seen examples where the hardware flow control normally works, so that board setup sets up those pins in "hardware flow control" mode when it's told the board has them wired up that way ... but then, some chip revisions have errata forcing the driver to use a particular port's handshake pins as GPIOs in some cases. (Obviously troublesome except at low data rates, so one hopes the board designers just avoid using those UARTs in that mode.) I've also seen examples where the UART clock needs to be disabled in deeper sleep states, but the system still wants the UART to be a wake event source ... by having either the START bit, or maybe BREAK (it gives a longer latch period), kick in a gpio-based pin change IRQ on the UARTn.RX line. Now in *both* of those examples I've seen before, the solution could not be generic. When the pin was used as GPIO, a particular pinmux configuration was needed. (Bitfield X of register Y has value Z ... or maybe a couple registers needed to change.) And when used as a UART signal, a different one was used. (Same setup. Maybe value Z became value W. Or again, maybe a few registers needed changing.) And for different chips in the family, sometimes even different revisions of one chip, the W/X/Y/Z/etc values differed even for UART1. For other UARTs, the same was true. So given that ... how would knowing the GPIO chip help, when I'd still need to find out the W/X/Y/Z/etc values? And if I've got a way to convey W/X/Y/Z/etc -- canonical example being a callback to the relevant SOC-specific code -- why shouldn't that obviate the need for any scheme to look up the gpio chip? - Dave ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [patch 2.6.25-git] spi_mpc83xx much improved driver
On Wednesday 30 April 2008, Andrew Morton wrote: > > + spin_lock_irq(&mpc83xx_spi->lock); > > irq-safe. > > > ... > > > > + spin_lock(&mpc83xx_spi->lock); > > not irq-safe. > > Deliberate? That latter one is inside an #if 0/#endif block, so it won't matter. The #if 0 block bothered me. Maybe Joakim can remove it. (By the way, I'd feel safer about this patch if there were an ack or two from more PPC folk...) ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
[patch 2.6.25-git] spi_mpc83xx much improved driver
From: Joakim Tjernlund <[EMAIL PROTECTED]>
Date: Fri, 11 Apr 2008 18:57:21 +0200
Subject: [PATCH] Much improved mpc83xx SPI driver.
The current driver may cause glitches on SPI CLK line since one must
disable the SPI controller before changing any HW settings. Fix this
by implementing a local spi_transfer function that won't change speed
and/or word size while CS is active.
While doing that heavy lifting a few other issues were addressed too:
- Make word size 16 and 32 work too.
- Honor bits_per_word and speed_hz in spi transaction.
- Optimize the common path.
This also stops using the "bitbang" framework (except for a few
constants).
Signed-off-by: Joakim Tjernlund <[EMAIL PROTECTED]>
[ Roel Kluin <[EMAIL PROTECTED]>: "irq" needs to be signed ]
Signed-off-by: David Brownell <[EMAIL PROTECTED]>
---
drivers/spi/Kconfig |1
drivers/spi/spi_mpc83xx.c | 409 +++---
2 files changed, 281 insertions(+), 129 deletions(-)
--- at91.orig/drivers/spi/Kconfig 2008-04-30 15:06:30.0 -0700
+++ at91/drivers/spi/Kconfig2008-04-30 15:06:34.0 -0700
@@ -126,7 +126,6 @@ config SPI_MPC52xx_PSC
config SPI_MPC83xx
tristate "Freescale MPC83xx/QUICC Engine SPI controller"
depends on SPI_MASTER && (PPC_83xx || QUICC_ENGINE) && EXPERIMENTAL
- select SPI_BITBANG
help
This enables using the Freescale MPC83xx and QUICC Engine SPI
controllers in master mode.
--- at91.orig/drivers/spi/spi_mpc83xx.c 2008-04-30 14:58:33.0 -0700
+++ at91/drivers/spi/spi_mpc83xx.c 2008-04-30 15:35:51.0 -0700
@@ -49,6 +49,7 @@ struct mpc83xx_spi_reg {
#defineSPMODE_LEN(x) ((x) << 20)
#defineSPMODE_PM(x)((x) << 16)
#defineSPMODE_OP (1 << 14)
+#defineSPMODE_CG(x)((x) << 7)
/*
* Default for SPI Mode:
@@ -67,10 +68,6 @@ struct mpc83xx_spi_reg {
/* SPI Controller driver's private data. */
struct mpc83xx_spi {
- /* bitbang has to be first */
- struct spi_bitbang bitbang;
- struct completion done;
-
struct mpc83xx_spi_reg __iomem *base;
/* rx & tx bufs from the spi_transfer */
@@ -82,7 +79,7 @@ struct mpc83xx_spi {
u32(*get_tx) (struct mpc83xx_spi *);
unsigned int count;
- u32 irq;
+ int irq;
unsigned nsecs; /* (clock cycle time)/2 */
@@ -94,6 +91,25 @@ struct mpc83xx_spi {
void (*activate_cs) (u8 cs, u8 polarity);
void (*deactivate_cs) (u8 cs, u8 polarity);
+
+ u8 busy;
+
+ struct workqueue_struct *workqueue;
+ struct work_struct work;
+
+ struct list_head queue;
+ spinlock_t lock;
+
+ struct completion done;
+};
+
+struct spi_mpc83xx_cs {
+ /* functions to deal with different sized buffers */
+ void (*get_rx) (u32 rx_data, struct mpc83xx_spi *);
+ u32 (*get_tx) (struct mpc83xx_spi *);
+ u32 rx_shift; /* RX data reg shift when in qe mode */
+ u32 tx_shift; /* TX data reg shift when in qe mode */
+ u32 hw_mode;/* Holds HW mode register settings */
};
static inline void mpc83xx_spi_write_reg(__be32 __iomem * reg, u32 val)
@@ -137,6 +153,7 @@ static void mpc83xx_spi_chipselect(struc
{
struct mpc83xx_spi *mpc83xx_spi;
u8 pol = spi->mode & SPI_CS_HIGH ? 1 : 0;
+ struct spi_mpc83xx_cs *cs = spi->controller_state;
mpc83xx_spi = spi_master_get_devdata(spi->master);
@@ -147,50 +164,26 @@ static void mpc83xx_spi_chipselect(struc
if (value == BITBANG_CS_ACTIVE) {
u32 regval = mpc83xx_spi_read_reg(&mpc83xx_spi->base->mode);
- u32 len = spi->bits_per_word;
- u8 pm;
-
- if (len == 32)
- len = 0;
- else
- len = len - 1;
- /* mask out bits we are going to set */
- regval &= ~(SPMODE_CP_BEGIN_EDGECLK | SPMODE_CI_INACTIVEHIGH
- | SPMODE_LEN(0xF) | SPMODE_DIV16
- | SPMODE_PM(0xF) | SPMODE_REV | SPMODE_LOOP);
-
- if (spi->mode & SPI_CPHA)
- regval |= SPMODE_CP_BEGIN_EDGECLK;
- if (spi->mode & SPI_CPOL)
- regval |= SPMODE_CI_INACTIVEHIGH;
- if (!(spi->mode & SPI_LSB_FIRST))
- regval |= SPMODE_REV;
- if (spi->mode & SPI_LOOP)
- regval |= SPMODE_LOOP;
-
- regval |= SPMODE_LEN(len);
-
- if ((mpc83xx_spi->spibrg / spi->max_speed_hz) >= 64) {
- pm = mpc83xx_spi->spibrg / (spi->max_speed_hz * 64) - 1;
-
Re: [spi-devel-general] [PATCH] spi_mpc83xx: test below 0 on unsigned irq in mpc83xx_spi_probe()
On Wednesday 23 April 2008, Kumar Gala wrote: > > On Apr 23, 2008, at 3:19 PM, Roel Kluin wrote: > > mpc83xx_spi->irq is unsigned, so the test fails > > > > Signed-off-by: Roel Kluin <[EMAIL PROTECTED]> Any reason to not just make mpc83xx_spi->irq be "int", following normal practice everywhere? If not, I'll roll that into the patch from Joakim Tjernlund... - Dave ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH 4/5] [POWERPC] QE: implement support for the GPIO LIB API
On Monday 21 April 2008, Anton Vorontsov wrote:
> On Mon, Apr 21, 2008 at 01:01:12PM -0700, David Brownell wrote:
> > The way other platforms do this is to hav SOC-specific
> > init code, and have board-specific initcalls call the
> > relevant SOC-specific setup.
>
> I don't know about other platforms other than PowerPC and some ARM,
> of course. For example, PXA boards don't call any SOC specific code.
> Instead, looking into reworked PXA code, I can see that there are
> separate arch_initcalls() for the pxa25x, pxa27x, and pxa3xx SOCs.
Exactly: SOC-specific initcalls.
> > Among other things that facilitates kernels that handle
> > multiple SOCs (if they're closely-enough related). That
> > may not be used by many distros (handhelds.org being at
> > least a partial exception), but it certainly helps cut
> > the number of configurations that need build-testing.
>
> Is this about QE_GPIO being user-selectable?
No. It's about "kernels that handle multiple SOCs". Like
for example pxa{25,27,3x}x ... or omap{16xx,17xx,5912}.
Consider several ways to support a family of SOCs.
- One way supports only one board at a time. Testing
builds after arch level changes means rebuilding kernels
for each board ... quite possibly several dozen.
- Another supports several boards, but only if they use
the same SOC. Testing builds here takes fewer kernels;
one per SOC; better, but still routinely shortchanged.
- Yet another way supports any number of boards, using
a variety of mostly-compatible SOCs. This allows test
builds that support entire SOC families (e.g. OMAP1,
or OMAP2/OMAP3, or PXA XScale, etc).
Test builds are a quality control thing. If they're done
regularly, it's less likely you'll push code upstream
that can't build in some configuration.
> And well, I'm not objecting for placing qe gpio code under arch/, too.
> I'll resend this patch once again reverting its placement to arch/.
OK, good. Then I don't really have to be involved with that.
- Dave
___
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH 4/5] [POWERPC] QE: implement support for the GPIO LIB API
On Monday 21 April 2008, Anton Vorontsov wrote: > From: J. Random Hacker > Subject: [POWERPC] cleanup board initialization code > > This patch removes vast amount of machine_arch_initcall()s that were > used to solely initialize some hardware, like this: > > qe_add_gpio_chips(); > fsl_gtm_init(); > fsl_spi_init(); > ... > > So, instead of calling this stuff from the board files, we implement > own arch_initcalls for these functions. The way other platforms do this is to hav SOC-specific init code, and have board-specific initcalls call the relevant SOC-specific setup. Among other things that facilitates kernels that handle multiple SOCs (if they're closely-enough related). That may not be used by many distros (handhelds.org being at least a partial exception), but it certainly helps cut the number of configurations that need build-testing. - Dave ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH 4/5] [POWERPC] QE: implement support for the GPIO LIB API
On Friday 18 April 2008, Anton Vorontsov wrote: > On Thu, Apr 17, 2008 at 09:21:40PM -0500, Kumar Gala wrote: > > On Apr 17, 2008, at 5:41 PM, Anton Vorontsov wrote: > >> > >> No problem. Would you prefer this to go under drivers/gpio/ ? > > > > Yes that would be better. We actively worked on pull drivers out of > > arch/ppc back in the day. I'm not sure I see the problem here. This is precisely the kind of *non-driver* code that normally (i.e. for all other Linux platforms I've looked at) *belongs* in the arch code ... what's the objection to doing it the same way other platforms do? And for that matter, why make it optional, via Kconfig, instead of always configuring it in, along with the GPIO interrupt support? A quick glance at one of the MPC83xx chips suggested that QE port IRQ capabilities would be very chip specific, so I'd expect that GPIO IRQ support wouldn't naturally move out of the arch tree. - Dave > Hi David, > > Do we need your Ack to go this through powerpc tree, and if so, could > you provide one? > > Thanks. > > Documentation/powerpc/booting-without-of.txt | 34 --- > arch/powerpc/platforms/Kconfig | 2 + > drivers/gpio/Kconfig | 9 ++ > drivers/gpio/Makefile | 1 + > drivers/gpio/qe.c | 147 > ++ > 5 files changed, 180 insertions(+), 13 deletions(-) > create mode 100644 drivers/gpio/qe.c ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH] Freescale QUICC Engine USB Host Controller
On Thursday 03 April 2008, Timur Tabi wrote: > > Well, there is Linux CLK API (somewhat similar to GPIO API), but PowerPC > > doesn't use it yet. > > Yeah, I noticed that too. I'll add it to my to-do list, but I suspect that > someone else will get around to it before I do. Note that there's some work afoot to provide a generic implementation framework for the clk_*() calls. It was last posted on the ARM list ... I'd hope it would make it easier for at least some PowerPC platforms to support that interface. - Dave ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH RFC 4/7] [GPIO] Let drivers link if they support GPIO API as an addition
On Monday 10 December 2007, Anton Vorontsov wrote: > On Mon, Dec 10, 2007 at 02:55:24PM -0800, David Brownell wrote: > > The point of CONFIG_GENERIC_GPIO is to be *the* conditional used to > > tell whether that programming interface is available ... starting > > from "#include ", and including all gpio_*() calls. > > > > So my first reaction is to not like this patch. It changes semantics > > in an incompatible way. And AFAICT, needlessly so. > > Why incompatible? gpio-aware drivers will get -ENOSYS on gpio_request, > thus they will not do anything wrong. GPIO-only drivers could still > depend on GENERIC_GPIO, and their behaviour will not change. If you still want this, I think a better approach would be: http://marc.info/?l=linux-kernel&m=120295461410848&w=2 That is, #include and have *that* do the relevant switch, based on GENERIC_GPIO. No semantic changes at all, if one discounts the implicit switch to (important for platforms that don't *have* any header), which won't affect any existing code. So your NAND code could use that, and work equally well on SOC variants that have generic GPIOs and those that don't. Comments? - Dave ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
