Re: [PATCH 00/34] omap drivers going upstream
On Mon, Sep 01, 2008 at 03:22:23AM +0200, andrzej zaborowski wrote:
> 2008/9/1 Felipe Balbi <[EMAIL PROTECTED]>:
> > On Sat, Aug 30, 2008 at 08:16:00PM +0300, Felipe Balbi wrote:
> >> From: Felipe Balbi <[EMAIL PROTECTED]>
> >>
> >> The following drivers are going upstream for integration.
> >> They have been sitting on linux-omap for quite a while just
> >> increasing the diff against mainline and probability of
> >> merge conflicts.
> >
> > Just one comment to this. I had to left bluetooth driver out of the
> > series because it's using omap2_block/allow_sleep in the driver code.
> > That should be fixed and the set_clock function should come via
> > platform_data to the driver.
> >
> > 53 static void hci_h4p_set_clk(struct hci_h4p_info *info, int *clock, int
> > enable)
> > 54 {
> > 55 unsigned long flags;
> > 56
> > 57 spin_lock_irqsave(&info->clocks_lock, flags);
> > 58 if (enable && !*clock) {
> > 59 NBT_DBG_POWER("Enabling %p\n", clock);
> > 60 clk_enable(info->uart_fclk);
> > 61 #ifdef CONFIG_ARCH_OMAP2
> > 62 if (cpu_is_omap24xx()) {
> > 63 clk_enable(info->uart_iclk);
> > 64 omap2_block_sleep();
> > 65 }
> > 66 #endif
> > 67 }
> > 68 if (!enable && *clock) {
> > 69 NBT_DBG_POWER("Disabling %p\n", clock);
> > 70 clk_disable(info->uart_fclk);
> > 71 #ifdef CONFIG_ARCH_OMAP2
> > 72 if (cpu_is_omap24xx()) {
> > 73 clk_disable(info->uart_iclk);
> > 74 omap2_allow_sleep();
> > 75 }
> > 76 #endif
> > 77 }
> > 78
> > 79 *clock = enable;
> > 80 spin_unlock_irqrestore(&info->clocks_lock, flags);
> > 81 }
> >
> > That driver is full of arch specific code and should be cleaned up ASAP.
> >
> > A few things I could get by briefly looking at the driver (actualy only
> > drivers/bluetooth/hci_h4p/core.c):
>
> There's also a curious issue in hci_h4p_interrupt I hit recently but
> after looking at the rest of the driver thought it was beating a dead
> horse..., but just in case it isn't:
> the driver assumes the OMAP UART, but then it uses UART_IIR_ID which
> is only valid for standard UARTs, causing OMAP-specific Rx errors to
> be ignored silently. Turns out that on my N810 there are actually Rx
> errors reported during firmware upload, but I didn't find a better way
> to handle them than to ignore them:
>
> --- a/drivers/bluetooth/hci_h4p/core.c
> +++ b/drivers/bluetooth/hci_h4p/core.c
> @@ -482,7 +491,14 @@ static irqreturn_t hci_h4p_interrupt(int irq, void *data)
>
> NBT_DBG("In interrupt handler iir 0x%.2x\n", iir);
>
> - iir &= UART_IIR_ID;
> + iir &= 0x1e; /* OMAP UART has wider INT than UART_IIR_ID */
> +
> + /*
> +* Often Rx errors are reported but reading the receive buffer
> +* gives the correct data, so treat it as an Rx interrupt.
> +*/
> + if (iir == 0xc)
> + iir = 0x4;
>
> if (iir == UART_IIR_MSI) {
> msr = hci_h4p_inb(info, UART_MSR);
>
> The tsc210x drivers should be upstreamable with the exception of ALSA
> code which needs to be converted to ASoC. Maruk Vasut found a leak in
> one error path, but I can't charge the device that has the tsc2102
> that I used for testing.
>
> I have some improvements to drivers/net/irda/omap-ir.c (clean-up and
> removing OMAP16xx specific bits to support OMAP1) but again, have no
> charger for the device.
Let's put Ville on the loop as he might be interested in it. Ville, any
comments ?
--
balbi
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 00/34] omap drivers going upstream
2008/8/30 Felipe Balbi <[EMAIL PROTECTED]>: > Add omap touchscreen driver This is just a tsc2101 touchscreen + pin multiplexing, I'm not really sure it should be upstream (but don't drop it without someone who knows commenting). I think someone even experimented with using a tsc2101 driver on the Hx board, but I may be wrong. Regards -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 00/34] omap drivers going upstream
2008/9/1 Felipe Balbi <[EMAIL PROTECTED]>:
> On Sat, Aug 30, 2008 at 08:16:00PM +0300, Felipe Balbi wrote:
>> From: Felipe Balbi <[EMAIL PROTECTED]>
>>
>> The following drivers are going upstream for integration.
>> They have been sitting on linux-omap for quite a while just
>> increasing the diff against mainline and probability of
>> merge conflicts.
>
> Just one comment to this. I had to left bluetooth driver out of the
> series because it's using omap2_block/allow_sleep in the driver code.
> That should be fixed and the set_clock function should come via
> platform_data to the driver.
>
> 53 static void hci_h4p_set_clk(struct hci_h4p_info *info, int *clock, int
> enable)
> 54 {
> 55 unsigned long flags;
> 56
> 57 spin_lock_irqsave(&info->clocks_lock, flags);
> 58 if (enable && !*clock) {
> 59 NBT_DBG_POWER("Enabling %p\n", clock);
> 60 clk_enable(info->uart_fclk);
> 61 #ifdef CONFIG_ARCH_OMAP2
> 62 if (cpu_is_omap24xx()) {
> 63 clk_enable(info->uart_iclk);
> 64 omap2_block_sleep();
> 65 }
> 66 #endif
> 67 }
> 68 if (!enable && *clock) {
> 69 NBT_DBG_POWER("Disabling %p\n", clock);
> 70 clk_disable(info->uart_fclk);
> 71 #ifdef CONFIG_ARCH_OMAP2
> 72 if (cpu_is_omap24xx()) {
> 73 clk_disable(info->uart_iclk);
> 74 omap2_allow_sleep();
> 75 }
> 76 #endif
> 77 }
> 78
> 79 *clock = enable;
> 80 spin_unlock_irqrestore(&info->clocks_lock, flags);
> 81 }
>
> That driver is full of arch specific code and should be cleaned up ASAP.
>
> A few things I could get by briefly looking at the driver (actualy only
> drivers/bluetooth/hci_h4p/core.c):
There's also a curious issue in hci_h4p_interrupt I hit recently but
after looking at the rest of the driver thought it was beating a dead
horse..., but just in case it isn't:
the driver assumes the OMAP UART, but then it uses UART_IIR_ID which
is only valid for standard UARTs, causing OMAP-specific Rx errors to
be ignored silently. Turns out that on my N810 there are actually Rx
errors reported during firmware upload, but I didn't find a better way
to handle them than to ignore them:
--- a/drivers/bluetooth/hci_h4p/core.c
+++ b/drivers/bluetooth/hci_h4p/core.c
@@ -482,7 +491,14 @@ static irqreturn_t hci_h4p_interrupt(int irq, void *data)
NBT_DBG("In interrupt handler iir 0x%.2x\n", iir);
- iir &= UART_IIR_ID;
+ iir &= 0x1e; /* OMAP UART has wider INT than UART_IIR_ID */
+
+ /*
+* Often Rx errors are reported but reading the receive buffer
+* gives the correct data, so treat it as an Rx interrupt.
+*/
+ if (iir == 0xc)
+ iir = 0x4;
if (iir == UART_IIR_MSI) {
msr = hci_h4p_inb(info, UART_MSR);
The tsc210x drivers should be upstreamable with the exception of ALSA
code which needs to be converted to ASoC. Maruk Vasut found a leak in
one error path, but I can't charge the device that has the tsc2102
that I used for testing.
I have some improvements to drivers/net/irda/omap-ir.c (clean-up and
removing OMAP16xx specific bits to support OMAP1) but again, have no
charger for the device.
Thanks
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 00/34] omap drivers going upstream
On Mon, Sep 01, 2008 at 01:37:37AM +0300, Felipe Balbi wrote: > On Sat, Aug 30, 2008 at 08:16:00PM +0300, Felipe Balbi wrote: > > From: Felipe Balbi <[EMAIL PROTECTED]> > > > > The following drivers are going upstream for integration. > > They have been sitting on linux-omap for quite a while just > > increasing the diff against mainline and probability of > > merge conflicts. > > Just one comment to this. I had to left bluetooth driver out of the typo, 'I had to leave' -- balbi -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 00/34] omap drivers going upstream
On Sat, Aug 30, 2008 at 08:16:00PM +0300, Felipe Balbi wrote:
> From: Felipe Balbi <[EMAIL PROTECTED]>
>
> The following drivers are going upstream for integration.
> They have been sitting on linux-omap for quite a while just
> increasing the diff against mainline and probability of
> merge conflicts.
Just one comment to this. I had to left bluetooth driver out of the
series because it's using omap2_block/allow_sleep in the driver code.
That should be fixed and the set_clock function should come via
platform_data to the driver.
53 static void hci_h4p_set_clk(struct hci_h4p_info *info, int *clock, int
enable)
54 {
55 unsigned long flags;
56
57 spin_lock_irqsave(&info->clocks_lock, flags);
58 if (enable && !*clock) {
59 NBT_DBG_POWER("Enabling %p\n", clock);
60 clk_enable(info->uart_fclk);
61 #ifdef CONFIG_ARCH_OMAP2
62 if (cpu_is_omap24xx()) {
63 clk_enable(info->uart_iclk);
64 omap2_block_sleep();
65 }
66 #endif
67 }
68 if (!enable && *clock) {
69 NBT_DBG_POWER("Disabling %p\n", clock);
70 clk_disable(info->uart_fclk);
71 #ifdef CONFIG_ARCH_OMAP2
72 if (cpu_is_omap24xx()) {
73 clk_disable(info->uart_iclk);
74 omap2_allow_sleep();
75 }
76 #endif
77 }
78
79 *clock = enable;
80 spin_unlock_irqrestore(&info->clocks_lock, flags);
81 }
That driver is full of arch specific code and should be cleaned up ASAP.
A few things I could get by briefly looking at the driver (actualy only
drivers/bluetooth/hci_h4p/core.c):
* set_clock should come via platform_data
* gpio handling should use gpiolib instead of omap-specific ones
* irq should come via platform_data to avoid 'if (cpu_is_xxx())'
* unnecessary casts should be removed
* one line ifs do not need {}
* there's a memory leak on probe if no platform_data is found
* use of io_p2v() should be avoided on drivers, refer to Russel King's
patch
* OMAP_GPIO_IRQ() should not be used on the driver.
--
balbi
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
