Hi Vitaly, > -----Original Message----- > From: Vitaly Wool [mailto:vitalywool@xxxxxxxxx] > Sent: den 17 december 2010 13:03 > To: Par-Gunnar HJALMDAHL > Cc: Pavan Savoy; Alan Cox; Arnd Bergmann; Samuel Ortiz; Marcel > Holtmann; linux-kernel@xxxxxxxxxxxxxxx; linux- > bluetooth@xxxxxxxxxxxxxxx; Lukasz Rymanowski; Linus WALLEIJ; Par-Gunnar > Hjalmdahl > Subject: Re: [PATCH 08/11] Bluetooth: Add support for CG2900 UART > > Hi Par, > > so on the top level: this is yet another H4 implementation plus > channel-based packet routing, right? > > Could you please also elaborate > Yes, the low-level basis is similar to e.g. hci_h4.c, where we register to N_HCI line discipline and then use the first byte to separate between different channels. While hci_h4.c supports only the standardized Bluetooth H4 channels, the cg2900_uart targets also the CG2900 specific H4 channels. > More comments on the code are inlined. > > > +#define MAIN_DEV (uart_info->dev) > > What is that for? > This is just a simplification when using for example dev_err(). It's just to shorten the text (not much in this case but there are other files where it looks better). No big problem to fix if you want that. > > + * cg2900_uart_suspend() - Called by Linux PM to put the device in a > low power mode. > > + * @pdev: Pointer to platform device. > > + * @state: New state. > > + * > > + * In UART case, CG2900 driver does nothing on suspend. > > + * > > + * Returns: > > + * 0 - Success. > > + */ > > +static int cg2900_uart_suspend(struct platform_device *pdev, > pm_message_t state) > > +{ > > + struct uart_info *uart_info = dev_get_drvdata(&pdev->dev); > > + > > + if (uart_info->sleep_state == CHIP_POWERED_DOWN) > > + return 0; > > + > > + if (uart_info->sleep_state != CHIP_ASLEEP) > > + return -EBUSY; > > + > > + dev_dbg(MAIN_DEV, "New sleep_state: CHIP_SUSPENDED\n"); > > + uart_info->sleep_state = CHIP_SUSPENDED; > > + return 0; > > +} > > I don't think this is safe wrt work queue. What if it gets scheduled > when drivers are suspended? > > Thanks, > Vitaly I'm not 100% sure what you mean since I don't think sleep_state should ever be in CHIP_ASLEEP if we have a work ongoing or in the queue, but I will ask our low power expert to look into this. /P-G -- To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
