Re: [PATCH 2/3] i2c-piix4: separate registration and probing code
Hi Andrew,
On Wed, 13 Jun 2012 12:59:08 -0400, Andrew Armenia wrote:
> Some chipsets have multiple sets of SMBus registers each controlling a
> separate SMBus. Supporting these chipsets properly will require registering
> multiple I2C adapters for one piix4.
>
> The code to initialize and register the i2c_adapter structure has been
> separated from piix4_probe and allows registration of a piix4 adapter
> given its base address. Note that the i2c_adapter and i2c_piix4_adapdata
> structures are now dynamically allocated.
>
> Signed-off-by: Andrew Armenia
> ---
> drivers/i2c/busses/i2c-piix4.c | 113
> ++--
> 1 file changed, 73 insertions(+), 40 deletions(-)
> (...)
Applied, with the minor changes below:
> -static int piix4_transaction(unsigned short piix4_smba)
> +static int piix4_transaction(struct i2c_adapter *piix4_adapter)
> {
> int temp;
> int result = 0;
> int timeout = 0;
>
> - dev_dbg(&piix4_adapter.dev, "Transaction (pre): CNT=%02x, CMD=%02x, "
> + struct i2c_piix4_adapdata *adapdata;
> + unsigned short piix4_smba;
> +
> + adapdata = i2c_get_adapdata(piix4_adapter);
> + piix4_smba = adapdata->smba;
It is customary to declare and assign all at once in this case:
struct i2c_piix4_adapdata *adapdata = i2c_get_adapdata(piix4_adapter);
unsigned short piix4_smba = adapdata->smba;
This makes the code more compact and more readable too. I've fixed it
and everywhere else.
> (...)
> +static int __devinit piix4_add_adapter(struct pci_dev *dev,
> + unsigned short smba,
> + struct i2c_adapter **padap)
> +{
> + struct i2c_adapter *adap;
> + struct i2c_piix4_adapdata *adapdata;
> +
> + int retval;
> +
> + adap = kzalloc(sizeof(*adap), GFP_KERNEL);
> + if (NULL == adap)
There's no point in inverting comparisons this way. Compilers would let
you know if you had it wrong, they do for at least 10 years now.
--
Jean Delvare
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 09/14] i2c: Add Device Tree support to the Nomadik I2C driver
On 14/06/12 19:46, Mark Brown wrote: On Thu, Jun 14, 2012 at 07:36:36PM +0100, Mark Brown wrote: You're not understanding Linus' point. The compatible string isn't useful here because properties like the maximum clock rate of the bus depend on the board design, not the silicon. The controller may be perfectly happy to run at a given rate but other devices on the bus or the electrical engineering of the PCB itself may restrict this further. Sorry, I read the next revision and see this was actually resolved OK. Yes, I just went ahead created the bindings anyway. I figured it would be neater (if no more functional) to keep all the variations in DT. Especially if we had devices which only varied by one or two settings, which would still require a complete new struct using the previous method. -- Lee Jones Linaro ST-Ericsson Landing Team Lead M: +44 77 88 633 515 Linaro.org │ Open source software for ARM SoCs Follow Linaro: Facebook | Twitter | Blog -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 09/14] i2c: Add Device Tree support to the Nomadik I2C driver
On 14/06/12 19:36, Mark Brown wrote: On Wed, Jun 13, 2012 at 01:28:17PM +0100, Lee Jones wrote: Device Tree. However, we have just as much control by keeping them in separate structs in the C file and selecting the right one using the compatible sting. You're not understanding Linus' point. The compatible string isn't useful here because properties like the maximum clock rate of the bus depend on the board design, not the silicon. The controller may be perfectly happy to run at a given rate but other devices on the bus or the electrical engineering of the PCB itself may restrict this further. And you're not understanding mine. ;) You can have multiple compatible strings for a single driver. Which one you reference from the Device Tree will dictate which group of settings are used, including variation of clock rates. -- Lee Jones Linaro ST-Ericsson Landing Team Lead M: +44 77 88 633 515 Linaro.org │ Open source software for ARM SoCs Follow Linaro: Facebook | Twitter | Blog -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 09/14] i2c: Add Device Tree support to the Nomadik I2C driver
On Thu, Jun 14, 2012 at 07:36:36PM +0100, Mark Brown wrote: > You're not understanding Linus' point. The compatible string isn't > useful here because properties like the maximum clock rate of the bus > depend on the board design, not the silicon. The controller may be > perfectly happy to run at a given rate but other devices on the bus or > the electrical engineering of the PCB itself may restrict this further. Sorry, I read the next revision and see this was actually resolved OK. -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 09/14] i2c: Add Device Tree support to the Nomadik I2C driver
On Wed, Jun 13, 2012 at 01:28:17PM +0100, Lee Jones wrote: > Device Tree. However, we have just as much control by keeping them > in separate structs in the C file and selecting the right one using > the compatible sting. You're not understanding Linus' point. The compatible string isn't useful here because properties like the maximum clock rate of the bus depend on the board design, not the silicon. The controller may be perfectly happy to run at a given rate but other devices on the bus or the electrical engineering of the PCB itself may restrict this further. -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V3 2/3] i2c-nomadik: turn the platform driver to an amba driver
On Thu, Jun 14, 2012 at 10:39 AM, Lee Jones wrote: > On 14/06/12 09:17, Alessandro Rubini wrote: >>> >>> You change only one half of ux500 here: the part where the device >>> gets defined statically, but not not the definition in the >>> device-tree. >> >> Yes, I'm aware. That's why I added Lee Jones as Cc:, on Linusw's >> suggestion. Lee told me to go ahead and he'll fix the DT stuff. > > I did? Yes :-) > I have already DT:ed this driver. There are 3 patches on the MLs currently. > >> [PATCH 1/3] i2c: Add Device Tree support to the Nomadik I2C driver >> [PATCH 2/3] ARM: ux500: Add i2c configurations to the Device Tree for >> DB8500 based devices >> [PATCH 3/3] Documentation: Device Tree binding information for i2c-nomadik >> driver > > I'm more than happy for you to base your patches on these and make the > necessary changes. Off hand, I think the only changes you'll need to make is > the probing from DT itself. Depends on whether this patch or yours go in first I guess. Which is up to Wolfram. What I know is that Alessandros patches are ready to merge IMO. > So something like: > >- compatible = "stericsson,db8500-i2c", > "st,nomadik-i2c"; >+ compatible = "st,nomadik-i2c", "arm,primecell"; Yep that should do it. Which brings me to the fear of what happens the day we have deployed device trees out there and just cannot change things like this easily :-/ Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/3] Documentation: Device Tree binding information for i2c-nomadik driver
On Wed, Jun 13, 2012 at 6:08 PM, Lee Jones wrote: > This document describes each of the non-standard Device Tree bindings > used in the Nomadik I2C driver. > > Signed-off-by: Lee Jones Acked-by: Linus Walleij Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/3] ARM: ux500: Add i2c configurations to the Device Tree for DB8500 based devices
On Wed, Jun 13, 2012 at 6:08 PM, Lee Jones wrote: > From: Lee Jones > Date: Wed, 13 Jun 2012 16:21:14 +0100 > Subject: [PATCH 2/3] ARM: ux500: Add i2c configurations to the Device Tree > for DB8500 based devices > > For correct operation of the i2c-nomadik driver, it requires some > information surrounding clock-frequencies, delay times and thresholds. > Here we apply those configurations to be used when a platform is > booted via the device Tree. > > Signed-off-by: Lee Jones Splendid, Acked-by: Linus Walleij Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/3] i2c: Add Device Tree support to the Nomadik I2C driver
On Wed, Jun 13, 2012 at 6:07 PM, Lee Jones wrote:
> Here we apply the bindings required for successful Device Tree
> probing of the i2c-nomadik driver. We also apply a fall-back
> configuration in case either one is not provided, or a required
> element is missing from the one supplied.
>
> Cc: linux-i2c@vger.kernel.org
> Signed-off-by: Lee Jones
This looks more like I'd expect it, good job! :-D
> +static int __devinit
> +nmk_i2c_of_probe(struct device_node *np, struct nmk_i2c_controller *pdata)
> +{
> + of_property_read_u32(np, "clock-frequency", (u32*)&pdata->clk_freq);
> + if (!pdata->clk_freq) {
> + pr_warn("%s: Clock frequency not found\n", np->full_name);
> + return -EINVAL;
> + }
> +
> + of_property_read_u32(np, "stericsson,slsu", (u32*)&pdata->slsu);
> + if (!pdata->slsu) {
> + pr_warn("%s: Data line delay not found\n", np->full_name);
> + return -EINVAL;
> + }
> +
> + of_property_read_u32(np, "stericsson,tft", (u32*)&pdata->tft);
> + if (!pdata->tft) {
> + pr_warn("%s: Tx FIFO threshold not found\n", np->full_name);
> + return -EINVAL;
> + }
> +
> + of_property_read_u32(np, "stericsson,rft", (u32*)&pdata->rft);
> + if (!pdata->rft) {
> + pr_warn("%s: Rx FIFO threshold not found\n", np->full_name);
> + return -EINVAL;
> + }
> +
> + of_property_read_u32(np, "stericsson,timeout", (u32*)&pdata->timeout);
> + if (!pdata->timeout) {
> + pr_warn("%s: Timeout not found\n", np->full_name);
> + return -EINVAL;
> + }
> +
> + if (of_get_property(np, "stericsson,i2c_freq_mode_fast", NULL))
> + pdata->sm = I2C_FREQ_MODE_FAST;
> + else
> + pdata->sm = I2C_FREQ_MODE_STANDARD;
> +
> + return 0;
> +}
Will this compile fine if CONFIG_OF is not selected?
> /* fetch the controller configuration from machine */
> dev->cfg.clk_freq = pdata->clk_freq;
> - dev->cfg.slsu = pdata->slsu;
> - dev->cfg.tft = pdata->tft;
> - dev->cfg.rft = pdata->rft;
> - dev->cfg.sm = pdata->sm;
> -
> - i2c_set_adapdata(adap, dev);
> + dev->cfg.slsu = pdata->slsu;
> + dev->cfg.tft = pdata->tft;
> + dev->cfg.rft = pdata->rft;
> + dev->cfg.sm = pdata->sm; i2c_set_adapdata(adap, dev);
This looks like an unrelated whitespace fix, but OK...
Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V3 1/4] i2c: tegra: make sure register writes completes
> probably doesn't impact performance. It'd be more self-documenting if > this readback was limited to the specific registers where it was needed > though. I'd think this is easier to maintain in case code gets rewritten/shuffled around. -- Pengutronix e.K. | Wolfram Sang| Industrial Linux Solutions | http://www.pengutronix.de/ | signature.asc Description: Digital signature
Re: [PATCH V3 1/4] i2c: tegra: make sure register writes completes
On 06/14/2012 06:35 AM, Laxman Dewangan wrote:
> On Wednesday 13 June 2012 09:25 PM, Stephen Warren wrote:
>> On 06/13/2012 04:12 AM, Laxman Dewangan wrote:
>> @@ -165,6 +165,10 @@ static void i2c_writel(struct tegra_i2c_dev
>> *i2c_dev, u32 val,
>>> unsigned long reg)
>>> {
>>> writel(val, i2c_dev->base + tegra_i2c_reg_addr(i2c_dev, reg));
>>> +
>>> +/* Read back register to make sure that register writes
>>> completed */
>>> +if (reg != I2C_TX_FIFO)
>>> +readl(i2c_dev->base + tegra_i2c_reg_addr(i2c_dev, reg));
>> I guess that's fine, but it sure does seem rather heavy-weight. Don't
>> you only need to do the readback if you just wrote to the IRQ status or
>> mask registers, rather than if you wrote to /any/ register other than
>> the FIFO?
>
> That's what my second patch but based on your earlier review comment, I
> did for every register.
Well, just for the record, in that comment you refer to, I was talking
about the location in the code where the readback should be implemented,
not the set of registers that the readback should happen for. But, it
probably doesn't impact performance. It'd be more self-documenting if
this readback was limited to the specific registers where it was needed
though.
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/4] i2c: i2c-ocores: Use reg-shift property
On Fri, Jun 08, 2012 at 05:56:51PM +0530, Jayachandran C. wrote:
> On Fri, Jun 08, 2012 at 01:41:36PM +0200, Peter Korsgaard wrote:
> > > "J" == Jayachandran C writes:
> >
> > J> From: Ganesan Ramalingam
> > J> Deprecate 'regstep' property and use the standard 'reg-shift' property
> > J> for register offset shifts. 'regstep' will still be supported as an
> > J> optional property, but will give a warning when used.
> >
> > ..
> >
> > J> struct ocores_i2c_platform_data {
> > J> - u32 regstep; /* distance between registers */
> > J> - u32 clock_khz; /* input clock in kHz */
> > J> - u8 num_devices; /* number of devices in the devices list */
> > J> + u32 reg_shift; /* register offset shift value */
> > J> + u32 clock_khz; /* input clock in kHz */
> > J> + u8 num_devices; /* number of devices in the devices
> > list */
> > J> struct i2c_board_info const *devices; /* devices connected to
> > the bus */
> > J> };
> >
> > Why not just keep this change to the dt bindings, instead of risking
> > breaking stuff for platform drivers as well? There's no conceptual
> > reason why reg_shift is any better than regstep.
>
> This is to keep the names and meanings of platform property and DT
> property same. Having two ways (setting regstep in platform code or
> setting 'reg-shift' in DT) of specifying the same parameter is not
> a nice.
>
> There is only one user of this API in the whole kernel tree, which
> is fixed as part of the patchset.
>
> Also we make sure that we do not break existing DTBs by still accepting
> 'regstep' property.
Any further comments on this patchset? If the changes are fine, an Acked-by
would be really aeppreciated.
Thanks,
JC.
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V3 1/4] i2c: tegra: make sure register writes completes
On Wednesday 13 June 2012 09:25 PM, Stephen Warren wrote:
On 06/13/2012 04:12 AM, Laxman Dewangan wrote:
@@ -165,6 +165,10 @@ static void i2c_writel(struct tegra_i2c_dev
*i2c_dev, u32 val,
unsigned long reg)
{
writel(val, i2c_dev->base + tegra_i2c_reg_addr(i2c_dev, reg));
+
+ /* Read back register to make sure that register writes completed */
+ if (reg != I2C_TX_FIFO)
+ readl(i2c_dev->base + tegra_i2c_reg_addr(i2c_dev, reg));
I guess that's fine, but it sure does seem rather heavy-weight. Don't
you only need to do the readback if you just wrote to the IRQ status or
mask registers, rather than if you wrote to /any/ register other than
the FIFO?
That's what my second patch but based on your earlier review comment, I
did for every register.
I think it will not matter much as we dont write all register with every
transaction, only during initialization.
Then for each transfer we write manly on Tx fifo and interrupt
mask/status register and hence not too much overweight.
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/9] ARM: Kirkwood: Add interrupt controller support for DT boards
On Thursday 14 June 2012, Andrew Lunn wrote: > > I think if you compute the number of interrupts in the domain from the > > number of > > registers that are described in the device tree, call orion_irq_init() > > based on those registers, and use irq domains for the gpio stuff as Rob > > suggested, > > this init_irq function can become completely generic to all orion platforms. > > I'm reworking the GPIO stuff at the moment, moving it to use IRQ > domains for its interrupts. That code will be generic to all Orion > platforms. I'm modeling the code on the AT91 GPIO handler, since that > has a similar structure, one hardware interrupt for a number of GPIO > lines, which is in software demultiplexed and triggers secondary level > interrupts. The major difference is that AT91 has one hardware > interrupt for a GPIO chip with 32 lines, where as Orion has 4 hardware > interrupt lines, so maybe four interrupt domains, for one GPIO chip > with 32 lines. My guess is that you're still better off with a single interrupt domain for 32 lines, or even for all the gpio lines, but I'm sure you can find a solution that works best for you. > Non-GPIO interrupts are more of a problem. Underneath it uses the > generic-chip interrupt code. The patchset to add _domain versions of > these calls is stalled. So i think for the moment, we need to stay > with the domain bolted on top, until generic-chip gets domain > support. I would also expect that generic-chip also gets a common DT > binding which any SoC can use. I suggested that before, and the comments I got back then were that we should treat the generic irq chip more like a library and keep the device tree binding at a higher level. If I understood things right, that means we would just have one plat-orion (or mach-mvebu later) irq chip abstraction that handles the DT binding and the irq domains itself but uses the generic irq chip code to implement the actual irq handling. > My aim at the moment is to get something which works well enough that > we can add DT support to other drivers. Without interrupts, we are not > going to get very far. We can later rip it out what i create now and > replace it once generic-chip becomes domain and DT aware, and there > should be no effect on other drivers. Right. I just want to ensure that we don't need to change the bindings in incompatible ways. Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 06/17] i2c: omap: improve 1p153 errata handling
Hi,
On Thu, Jun 14, 2012 at 12:17:46PM +0100, Russell King - ARM Linux wrote:
> On Thu, Jun 14, 2012 at 01:20:39PM +0300, Felipe Balbi wrote:
> > -static int errata_omap3_1p153(struct omap_i2c_dev *dev, u16 *stat, int
> > *err)
> > +static int errata_omap3_1p153(struct omap_i2c_dev *dev)
> > {
> > - unsigned long timeout = 1;
> > + unsigned long timeout = 1;
> > + u16 stat;
>
> Eww, no, not more of this "lets add tabs to align auto variable
> declarations". This is detrimental when you add another variable whose
> type is longer than the current indentation - you have to reformat the
> entire block.
>
> It's really not worth it. Stick to just using one space, like the rest
> of the kernel code. Like the code which Linus writes. And thereby help
> to avoid future "pointless churn" whinges.
fair enough, no need to get so over the top like that, it's just a tab.
Will fix it.
--
balbi
signature.asc
Description: Digital signature
Re: [PATCH 09/17] i2c: omap: ack IRQ in parts
On Thu, Jun 14, 2012 at 12:20:17PM +0100, Russell King - ARM Linux wrote: > On Thu, Jun 14, 2012 at 01:20:42PM +0300, Felipe Balbi wrote: > > According to flow diagrams on OMAP TRMs, > > we should ACK the IRQ as they happen. > > You don't explain that you're adding a new dev_err() statement into the > driver for a missing acknowledge. > > What if you're probing for a device - can this cause spam to the kernel > console? What if you have protocol mangling enabled with the "ignore > lack of ack bit set" ? indeed... maybe dev_vdbg() would fit better, or just don't add anything. -- balbi signature.asc Description: Digital signature
Re: [PATCH 09/17] i2c: omap: ack IRQ in parts
On Thu, Jun 14, 2012 at 04:41:45PM +0530, Shilimkar, Santosh wrote:
> On Thu, Jun 14, 2012 at 3:50 PM, Felipe Balbi wrote:
> > According to flow diagrams on OMAP TRMs,
> > we should ACK the IRQ as they happen.
> >
> > Signed-off-by: Felipe Balbi
> > ---
> > drivers/i2c/busses/i2c-omap.c | 29 +
> > 1 file changed, 17 insertions(+), 12 deletions(-)
> >
> > diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
> > index f978b14..c00ba7d 100644
> > --- a/drivers/i2c/busses/i2c-omap.c
> > +++ b/drivers/i2c/busses/i2c-omap.c
> > @@ -894,21 +894,20 @@ omap_i2c_isr(int this_irq, void *dev_id)
> > }
> >
> > complete:
> > - /*
> > - * Ack the stat in one go, but [R/X]DR and [R/X]RDY should
> > be
> > - * acked after the data operation is complete.
> > - * Ref: TRM SWPU114Q Figure 18-31
> > - */
> > - omap_i2c_write_reg(dev, OMAP_I2C_STAT_REG, stat &
> > - ~(OMAP_I2C_STAT_RRDY | OMAP_I2C_STAT_RDR |
> > - OMAP_I2C_STAT_XRDY | OMAP_I2C_STAT_XDR));
> > -
> > - if (stat & OMAP_I2C_STAT_NACK)
> > + if (stat & OMAP_I2C_STAT_NACK) {
> > + dev_err(dev->dev, "No Acknowledge\n");
> > err |= OMAP_I2C_STAT_NACK;
> > + omap_i2c_ack_stat(dev, OMAP_I2C_STAT_NACK);
> > + omap_i2c_complete_cmd(dev, err);
> > + return IRQ_HANDLED;
> > + }
> >
> Do you think making I2C IRQ ack + complete as one small static inline function
> can save clean the code further. I see it has been used in multiple paths.
done in a later patch
--
balbi
signature.asc
Description: Digital signature
Re: [PATCH 04/17] i2c: omap: simplify omap_i2c_ack_stat()
Hi,
On Thu, Jun 14, 2012 at 12:13:33PM +0100, Russell King - ARM Linux wrote:
> On Thu, Jun 14, 2012 at 01:20:37PM +0300, Felipe Balbi wrote:
> > stat & BIT(1) is the same as BIT(1), so let's
> > simplify things a bit by removing "stat &" from
> > all omap_i2c_ack_stat() calls.
>
> This doesn't feel right, and the explanation is definitely wrong.
>
> "stat & BIT(1)" is not the same as "BIT(1)" _unless_ you're saying that
> stat always has BIT(1) already set. Can you guarantee that in this code?
> If so, how?
>
> What happens if you read the status register, and it has bit 1 clear.
> immediately after the read, the status register bit 1 becomes set, and
> then you write bit 1 set (because you've dropped the stat & BIT(1) from
> the code.)
>
> Is it not going to acknowledge that bit-1-set but because you haven't
> read it, you're going to miss that event?
>
> This feels like a buggy change to me.
I fail to see that situation would happen with this driver. See what it
does (extremely simplified):
if (stat & NACK) {
...
omap_i2c_ack_stat(dev, stat & NACK);
}
if (stat & RDR) {
...
omap_i2c_ack_stat(dev, stat & RDR);
}
and so on. The tricky place is only WRT errata handling, for example:
if (*stat & (NACK | AL)) {
omap_i2c_ack_stat(dev, *stat & (XRDY | XDR));
...
}
but in this case, the errata says we must clear XRDY and XDR if that
errata triggers, so if they just got enabled or not, it doesn't matter.
Another tricky place is RDR | RRDY (likewise for XDR | XRDY):
if (stat & (RDR | RRDY)) {
...
omap_i2c_ack_stat(dev, stat & (RDR | RRDY));
}
again here there will be no issues because those IRQs never fire
simultaneously and one will only after after we have handled the
previous, that's because the same FIFO is used anyway and we won't shift
data into FIFO until we tell the IP "hey, I'm done with the FIFO, you
can shift more data". Right ?
--
balbi
signature.asc
Description: Digital signature
Re: [PATCH 15/17] i2c: omap: always return IRQ_HANDLED
On Thu, Jun 14, 2012 at 4:53 PM, Russell King - ARM Linux wrote: > On Thu, Jun 14, 2012 at 04:48:56PM +0530, Shilimkar, Santosh wrote: >> On Thu, Jun 14, 2012 at 3:50 PM, Felipe Balbi wrote: >> > otherwise we could get our IRQ line disabled due >> > to many spurious IRQs. >> > >> > Signed-off-by: Felipe Balbi >> > --- >> > drivers/i2c/busses/i2c-omap.c | 2 +- >> > 1 file changed, 1 insertion(+), 1 deletion(-) >> > >> > diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c >> > index fc5b8bc..5b78a73 100644 >> > --- a/drivers/i2c/busses/i2c-omap.c >> > +++ b/drivers/i2c/busses/i2c-omap.c >> > @@ -1015,7 +1015,7 @@ omap_i2c_isr(int this_irq, void *dev_id) >> > } >> > } while (stat); >> > >> > - return count ? IRQ_HANDLED : IRQ_NONE; >> > + return IRQ_HANDLED; >> >> no sure if this is correct. if you have IRQ flood and instead of _actually_ >> handling it, if you return handled, you still have interrupt pending, right? > > The point of returning IRQ_NONE is to indicate to the interrupt layer that > the interrupt you received was not processed by any interrupt handler, and > therefore to provide a way of preventing the system being brought to a halt > though a stuck interrupt line. > > So, if you do process an interrupt, you should always return IRQ_HANDLED > even if you couldn't complete its processing (eg, because you've serviced > it 100 times.) That make sense. Thanks for explanation Russell. Regards santosh -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 00/17] Big OMAP I2C Cleanup
On Thu, Jun 14, 2012 at 3:50 PM, Felipe Balbi wrote: > Hi guys, > > I have dropped a few patches from the series and also > tested every single patch on my pandaboard. I2C messages > are still working fine with panda after each patch. > > There's still lots of work to be done on the i2c-omap.c > driver but it's now easier to read, IMO. > Great cleanup. Have reviewed the entire series based on limited knowledge i have about the OMAP i2c driver. Apart from those comments, the series looks good to me. We may to take similar dig at MMC and SPI driver one day :-p Regards santosh -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 15/17] i2c: omap: always return IRQ_HANDLED
On Thu, Jun 14, 2012 at 04:48:56PM +0530, Shilimkar, Santosh wrote: > On Thu, Jun 14, 2012 at 3:50 PM, Felipe Balbi wrote: > > otherwise we could get our IRQ line disabled due > > to many spurious IRQs. > > > > Signed-off-by: Felipe Balbi > > --- > > drivers/i2c/busses/i2c-omap.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c > > index fc5b8bc..5b78a73 100644 > > --- a/drivers/i2c/busses/i2c-omap.c > > +++ b/drivers/i2c/busses/i2c-omap.c > > @@ -1015,7 +1015,7 @@ omap_i2c_isr(int this_irq, void *dev_id) > > } > > } while (stat); > > > > - return count ? IRQ_HANDLED : IRQ_NONE; > > + return IRQ_HANDLED; > > no sure if this is correct. if you have IRQ flood and instead of _actually_ > handling it, if you return handled, you still have interrupt pending, right? The point of returning IRQ_NONE is to indicate to the interrupt layer that the interrupt you received was not processed by any interrupt handler, and therefore to provide a way of preventing the system being brought to a halt though a stuck interrupt line. So, if you do process an interrupt, you should always return IRQ_HANDLED even if you couldn't complete its processing (eg, because you've serviced it 100 times.) -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 09/17] i2c: omap: ack IRQ in parts
On Thu, Jun 14, 2012 at 01:20:42PM +0300, Felipe Balbi wrote:
> According to flow diagrams on OMAP TRMs,
> we should ACK the IRQ as they happen.
You don't explain that you're adding a new dev_err() statement into the
driver for a missing acknowledge.
What if you're probing for a device - can this cause spam to the kernel
console? What if you have protocol mangling enabled with the "ignore
lack of ack bit set" ?
>
> Signed-off-by: Felipe Balbi
> ---
> drivers/i2c/busses/i2c-omap.c | 29 +
> 1 file changed, 17 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
> index f978b14..c00ba7d 100644
> --- a/drivers/i2c/busses/i2c-omap.c
> +++ b/drivers/i2c/busses/i2c-omap.c
> @@ -894,21 +894,20 @@ omap_i2c_isr(int this_irq, void *dev_id)
> }
>
> complete:
> - /*
> - * Ack the stat in one go, but [R/X]DR and [R/X]RDY should be
> - * acked after the data operation is complete.
> - * Ref: TRM SWPU114Q Figure 18-31
> - */
> - omap_i2c_write_reg(dev, OMAP_I2C_STAT_REG, stat &
> - ~(OMAP_I2C_STAT_RRDY | OMAP_I2C_STAT_RDR |
> - OMAP_I2C_STAT_XRDY | OMAP_I2C_STAT_XDR));
> -
> - if (stat & OMAP_I2C_STAT_NACK)
> + if (stat & OMAP_I2C_STAT_NACK) {
> + dev_err(dev->dev, "No Acknowledge\n");
> err |= OMAP_I2C_STAT_NACK;
> + omap_i2c_ack_stat(dev, OMAP_I2C_STAT_NACK);
> + omap_i2c_complete_cmd(dev, err);
> + return IRQ_HANDLED;
> + }
>
> if (stat & OMAP_I2C_STAT_AL) {
> dev_err(dev->dev, "Arbitration lost\n");
> err |= OMAP_I2C_STAT_AL;
> + omap_i2c_ack_stat(dev, OMAP_I2C_STAT_NACK);
> + omap_i2c_complete_cmd(dev, err);
> + return IRQ_HANDLED;
> }
>
> /*
> @@ -989,12 +988,18 @@ complete:
>
> if (stat & OMAP_I2C_STAT_ROVR) {
> dev_err(dev->dev, "Receive overrun\n");
> - dev->cmd_err |= OMAP_I2C_STAT_ROVR;
> + err |= OMAP_I2C_STAT_ROVR;
> + omap_i2c_ack_stat(dev, OMAP_I2C_STAT_ROVR);
> + omap_i2c_complete_cmd(dev, err);
> + return IRQ_HANDLED;
> }
>
> if (stat & OMAP_I2C_STAT_XUDF) {
> dev_err(dev->dev, "Transmit underflow\n");
> - dev->cmd_err |= OMAP_I2C_STAT_XUDF;
> + err |= OMAP_I2C_STAT_XUDF;
> + omap_i2c_ack_stat(dev, OMAP_I2C_STAT_XUDF);
> + omap_i2c_complete_cmd(dev, err);
> + return IRQ_HANDLED;
> }
> } while (stat);
>
> --
> 1.7.10.4
>
>
> ___
> linux-arm-kernel mailing list
> linux-arm-ker...@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 15/17] i2c: omap: always return IRQ_HANDLED
On Thu, Jun 14, 2012 at 3:50 PM, Felipe Balbi wrote:
> otherwise we could get our IRQ line disabled due
> to many spurious IRQs.
>
> Signed-off-by: Felipe Balbi
> ---
> drivers/i2c/busses/i2c-omap.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
> index fc5b8bc..5b78a73 100644
> --- a/drivers/i2c/busses/i2c-omap.c
> +++ b/drivers/i2c/busses/i2c-omap.c
> @@ -1015,7 +1015,7 @@ omap_i2c_isr(int this_irq, void *dev_id)
> }
> } while (stat);
>
> - return count ? IRQ_HANDLED : IRQ_NONE;
> + return IRQ_HANDLED;
no sure if this is correct. if you have IRQ flood and instead of _actually_
handling it, if you return handled, you still have interrupt pending, right?
if (count++ == 100) {
dev_warn(dev->dev, "Too much work in one IRQ\n");
break;
}
ofcourse we do get warning message already, so as such the change
should be fine.
Just want to understand the change bit more.
Regards
Santosh
Regards
santosh
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 06/17] i2c: omap: improve 1p153 errata handling
On Thu, Jun 14, 2012 at 01:20:39PM +0300, Felipe Balbi wrote:
> -static int errata_omap3_1p153(struct omap_i2c_dev *dev, u16 *stat, int *err)
> +static int errata_omap3_1p153(struct omap_i2c_dev *dev)
> {
> - unsigned long timeout = 1;
> + unsigned long timeout = 1;
> + u16 stat;
Eww, no, not more of this "lets add tabs to align auto variable
declarations". This is detrimental when you add another variable whose
type is longer than the current indentation - you have to reformat the
entire block.
It's really not worth it. Stick to just using one space, like the rest
of the kernel code. Like the code which Linus writes. And thereby help
to avoid future "pointless churn" whinges.
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 04/17] i2c: omap: simplify omap_i2c_ack_stat()
On Thu, Jun 14, 2012 at 01:20:37PM +0300, Felipe Balbi wrote: > stat & BIT(1) is the same as BIT(1), so let's > simplify things a bit by removing "stat &" from > all omap_i2c_ack_stat() calls. This doesn't feel right, and the explanation is definitely wrong. "stat & BIT(1)" is not the same as "BIT(1)" _unless_ you're saying that stat always has BIT(1) already set. Can you guarantee that in this code? If so, how? What happens if you read the status register, and it has bit 1 clear. immediately after the read, the status register bit 1 becomes set, and then you write bit 1 set (because you've dropped the stat & BIT(1) from the code.) Is it not going to acknowledge that bit-1-set but because you haven't read it, you're going to miss that event? This feels like a buggy change to me. -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 09/17] i2c: omap: ack IRQ in parts
On Thu, Jun 14, 2012 at 3:50 PM, Felipe Balbi wrote:
> According to flow diagrams on OMAP TRMs,
> we should ACK the IRQ as they happen.
>
> Signed-off-by: Felipe Balbi
> ---
> drivers/i2c/busses/i2c-omap.c | 29 +
> 1 file changed, 17 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
> index f978b14..c00ba7d 100644
> --- a/drivers/i2c/busses/i2c-omap.c
> +++ b/drivers/i2c/busses/i2c-omap.c
> @@ -894,21 +894,20 @@ omap_i2c_isr(int this_irq, void *dev_id)
> }
>
> complete:
> - /*
> - * Ack the stat in one go, but [R/X]DR and [R/X]RDY should be
> - * acked after the data operation is complete.
> - * Ref: TRM SWPU114Q Figure 18-31
> - */
> - omap_i2c_write_reg(dev, OMAP_I2C_STAT_REG, stat &
> - ~(OMAP_I2C_STAT_RRDY | OMAP_I2C_STAT_RDR |
> - OMAP_I2C_STAT_XRDY | OMAP_I2C_STAT_XDR));
> -
> - if (stat & OMAP_I2C_STAT_NACK)
> + if (stat & OMAP_I2C_STAT_NACK) {
> + dev_err(dev->dev, "No Acknowledge\n");
> err |= OMAP_I2C_STAT_NACK;
> + omap_i2c_ack_stat(dev, OMAP_I2C_STAT_NACK);
> + omap_i2c_complete_cmd(dev, err);
> + return IRQ_HANDLED;
> + }
>
Do you think making I2C IRQ ack + complete as one small static inline function
can save clean the code further. I see it has been used in multiple paths.
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 08/17] i2c: omap: switch over to do {} while loop
On Thu, Jun 14, 2012 at 04:33:39PM +0530, Shilimkar, Santosh wrote: > On Thu, Jun 14, 2012 at 3:50 PM, Felipe Balbi wrote: > > this will make sure that we execute at least once. > > No functional changes otherwise. > > > > Signed-off-by: Felipe Balbi > > --- > Executing at least once instead of never is > still a functional change :-) there's a check for spurious interrupts. The idea was mainly to initialise stat and bits correctly at the beginning of the handler. Besides that "otherwise" is telling you that: "except this, there are no other functional changes" ;) -- balbi signature.asc Description: Digital signature
Re: [PATCH 08/17] i2c: omap: switch over to do {} while loop
On Thu, Jun 14, 2012 at 3:50 PM, Felipe Balbi wrote: > this will make sure that we execute at least once. > No functional changes otherwise. > > Signed-off-by: Felipe Balbi > --- Executing at least once instead of never is still a functional change :-) Regards Santosh -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 06/17] i2c: omap: improve 1p153 errata handling
On Thu, Jun 14, 2012 at 3:50 PM, Felipe Balbi wrote:
> Make it not depend on ISR's local variables
> in order to make it easier to re-factor the
> transmit data loop.
>
> Signed-off-by: Felipe Balbi
> ---
> drivers/i2c/busses/i2c-omap.c | 47
> +
> 1 file changed, 34 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
> index 0661ca1..52861c2 100644
> --- a/drivers/i2c/busses/i2c-omap.c
> +++ b/drivers/i2c/busses/i2c-omap.c
> @@ -768,21 +768,24 @@ omap_i2c_omap1_isr(int this_irq, void *dev_id)
> * data to DATA_REG. Otherwise some data bytes can be lost while transferring
> * them from the memory to the I2C interface.
> */
> -static int errata_omap3_1p153(struct omap_i2c_dev *dev, u16 *stat, int *err)
> +static int errata_omap3_1p153(struct omap_i2c_dev *dev)
> {
> - unsigned long timeout = 1;
> + unsigned long timeout = 1;
Not related to this patch but this 1 timeout magic
needs to be fixed as well.
The patch looks fine.
Regards
santosh
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 5/9] ARM: kirkwood: use devicetree for orion-spi
On Thursday 14 June 2012, Andrew Lunn wrote: > We either have auxdata and clean clock code, or no auxdata and messy > clock code with lots of aliases. > > The auxdata is also not limited to the name of the clocks. It allows > me to diff the kernel log for a DT boot and a none DT boot of the same > box and see they are identical. I've found a few typo's that way, and > not been hindered by noise of the devices changing name is useful. > > Once we have the clock tree described in DT, then i think it makes > sense to remove these auxdata entries. > Ok, fair enough. Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 04/17] i2c: omap: simplify omap_i2c_ack_stat()
On Thu, Jun 14, 2012 at 3:50 PM, Felipe Balbi wrote: > stat & BIT(1) is the same as BIT(1), so let's > simplify things a bit by removing "stat &" from > all omap_i2c_ack_stat() calls. > > Signed-off-by: Felipe Balbi > --- Indeed. Looks good to me. Reviewed-by : Santosh Shilimkar -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 03/17] i2c: omap: add blank lines
On Thu, Jun 14, 2012 at 3:50 PM, Felipe Balbi wrote: > trivial patch to aid readability. No functional > changes. > > Signed-off-by: Felipe Balbi > --- Not sure if it is needed but may be spliting the series into clean up and functional update like logic changes etc may be better for merge as well as testing. Regards santosh -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 02/17] i2c: omap: decrease indentation level on data handling
On Thu, Jun 14, 2012 at 04:11:14PM +0530, Shilimkar, Santosh wrote:
> On Thu, Jun 14, 2012 at 3:50 PM, Felipe Balbi wrote:
> > trivial patch, no functional changes.
> >
> > Signed-off-by: Felipe Balbi
> > ---
> > drivers/i2c/busses/i2c-omap.c | 63
> > -
> > 1 file changed, 31 insertions(+), 32 deletions(-)
> >
> > diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
> > index 9b532cd..39d5583 100644
> > --- a/drivers/i2c/busses/i2c-omap.c
> > +++ b/drivers/i2c/busses/i2c-omap.c
> > @@ -856,22 +856,7 @@ complete:
> > >> 8) & 0x3F;
> > }
> > while (num_bytes--) {
> > - w = omap_i2c_read_reg(dev,
> > OMAP_I2C_DATA_REG);
> > - if (dev->buf_len) {
> > - *dev->buf++ = w;
> > - dev->buf_len--;
> > - /*
> > - * Data reg in 2430, omap3 and
> > - * omap4 is 8 bit wide
> > - */
> > - if (dev->flags &
> > -
> > OMAP_I2C_FLAG_16BIT_DATA_REG) {
> > - if (dev->buf_len) {
> > - *dev->buf++ = w >>
> > 8;
> > - dev->buf_len--;
> > - }
> > - }
> > - } else {
> > + if (!dev->buf_len) {
> > if (stat & OMAP_I2C_STAT_RRDY)
> > dev_err(dev->dev,
> > "RRDY IRQ while no
> > data"
> > @@ -882,6 +867,21 @@ complete:
> > "
> > requested\n");
> > break;
> > }
> > +
> > + w = omap_i2c_read_reg(dev,
> > OMAP_I2C_DATA_REG);
> > + *dev->buf++ = w;
> > + dev->buf_len--;
> > + /*
> > + * Data reg in 2430, omap3 and
> > + * omap4 is 8 bit wide
> > + */
> > + if (dev->flags &
> > +
> > OMAP_I2C_FLAG_16BIT_DATA_REG) {
> > + if (dev->buf_len) {
> > + *dev->buf++ = w >> 8;
> > + dev->buf_len--;
> > + }
> > + }
> > }
> > omap_i2c_ack_stat(dev,
> > stat & (OMAP_I2C_STAT_RRDY |
> > OMAP_I2C_STAT_RDR));
> > @@ -898,22 +898,7 @@ complete:
> > & 0x3F;
> > }
> > while (num_bytes--) {
> > - w = 0;
> > - if (dev->buf_len) {
> > - w = *dev->buf++;
> > - dev->buf_len--;
> > - /*
> > - * Data reg in 2430, omap3 and
> > - * omap4 is 8 bit wide
> > - */
> > - if (dev->flags &
> > -
> > OMAP_I2C_FLAG_16BIT_DATA_REG) {
> > - if (dev->buf_len) {
> > - w |= *dev->buf++ <<
> > 8;
> > - dev->buf_len--;
> > - }
> > - }
> > - } else {
> > + if (!dev->buf_len) {
> > if (stat & OMAP_I2C_STAT_XRDY)
> > dev_err(dev->dev,
> > "XRDY IRQ while no "
> > @@ -925,6 +910,20 @@ complete:
> > break;
> > }
> >
> > + w = *dev->buf++;
> > + dev->buf_len--;
> > + /*
> >
Re: [PATCH 02/17] i2c: omap: decrease indentation level on data handling
On Thu, Jun 14, 2012 at 3:50 PM, Felipe Balbi wrote:
> trivial patch, no functional changes.
>
> Signed-off-by: Felipe Balbi
> ---
> drivers/i2c/busses/i2c-omap.c | 63
> -
> 1 file changed, 31 insertions(+), 32 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
> index 9b532cd..39d5583 100644
> --- a/drivers/i2c/busses/i2c-omap.c
> +++ b/drivers/i2c/busses/i2c-omap.c
> @@ -856,22 +856,7 @@ complete:
> >> 8) & 0x3F;
> }
> while (num_bytes--) {
> - w = omap_i2c_read_reg(dev, OMAP_I2C_DATA_REG);
> - if (dev->buf_len) {
> - *dev->buf++ = w;
> - dev->buf_len--;
> - /*
> - * Data reg in 2430, omap3 and
> - * omap4 is 8 bit wide
> - */
> - if (dev->flags &
> -
> OMAP_I2C_FLAG_16BIT_DATA_REG) {
> - if (dev->buf_len) {
> - *dev->buf++ = w >> 8;
> - dev->buf_len--;
> - }
> - }
> - } else {
> + if (!dev->buf_len) {
> if (stat & OMAP_I2C_STAT_RRDY)
> dev_err(dev->dev,
> "RRDY IRQ while no
> data"
> @@ -882,6 +867,21 @@ complete:
> "
> requested\n");
> break;
> }
> +
> + w = omap_i2c_read_reg(dev, OMAP_I2C_DATA_REG);
> + *dev->buf++ = w;
> + dev->buf_len--;
> + /*
> + * Data reg in 2430, omap3 and
> + * omap4 is 8 bit wide
> + */
> + if (dev->flags &
> + OMAP_I2C_FLAG_16BIT_DATA_REG)
> {
> + if (dev->buf_len) {
> + *dev->buf++ = w >> 8;
> + dev->buf_len--;
> + }
> + }
> }
> omap_i2c_ack_stat(dev,
> stat & (OMAP_I2C_STAT_RRDY |
> OMAP_I2C_STAT_RDR));
> @@ -898,22 +898,7 @@ complete:
> & 0x3F;
> }
> while (num_bytes--) {
> - w = 0;
> - if (dev->buf_len) {
> - w = *dev->buf++;
> - dev->buf_len--;
> - /*
> - * Data reg in 2430, omap3 and
> - * omap4 is 8 bit wide
> - */
> - if (dev->flags &
> -
> OMAP_I2C_FLAG_16BIT_DATA_REG) {
> - if (dev->buf_len) {
> - w |= *dev->buf++ << 8;
> - dev->buf_len--;
> - }
> - }
> - } else {
> + if (!dev->buf_len) {
> if (stat & OMAP_I2C_STAT_XRDY)
> dev_err(dev->dev,
> "XRDY IRQ while no "
> @@ -925,6 +910,20 @@ complete:
> break;
> }
>
> + w = *dev->buf++;
> + dev->buf_len--;
> + /*
> + * Data reg in 2430, omap3 and
> + * omap4 is 8 bit wide
> + */
> + if (dev->flags &
> + OMAP_I2C_FLAG_16BIT_DAT
Re: [PATCH 01/17] i2c: omap: simplify num_bytes handling
On Thu, Jun 14, 2012 at 3:50 PM, Felipe Balbi wrote: > trivial patch, no functional changes > > Signed-off-by: Felipe Balbi > --- Looks good. Reviewed-by : Santosh Shilimkar -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 17/17] i2c: omap: resize fifos before each message
This patch will try to avoid the usage of
draining feature by reconfiguring the FIFO
the start condition of each message based
on the message's size.
By doing that, we will be better utilizing
the FIFO when doing big transfers.
While at that also drop the now uneeded
check for dev->buf_len as we always know
the amount of data to be transmitted.
Signed-off-by: Felipe Balbi
---
drivers/i2c/busses/i2c-omap.c | 83 +
1 file changed, 51 insertions(+), 32 deletions(-)
diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
index 6e75d03..759fdbd 100644
--- a/drivers/i2c/busses/i2c-omap.c
+++ b/drivers/i2c/busses/i2c-omap.c
@@ -193,6 +193,7 @@ struct omap_i2c_dev {
u8 *regs;
size_t buf_len;
struct i2c_adapter adapter;
+ u8 threshold;
u8 fifo_size; /* use as flag and value
* fifo_size==0 implies no fifo
* if set, should be trsh+1
@@ -459,13 +460,6 @@ static int omap_i2c_init(struct omap_i2c_dev *dev)
omap_i2c_write_reg(dev, OMAP_I2C_SCLL_REG, scll);
omap_i2c_write_reg(dev, OMAP_I2C_SCLH_REG, sclh);
- if (dev->fifo_size) {
- /* Note: setup required fifo size - 1. RTRSH and XTRSH */
- buf = (dev->fifo_size - 1) << 8 | OMAP_I2C_BUF_RXFIF_CLR |
- (dev->fifo_size - 1) | OMAP_I2C_BUF_TXFIF_CLR;
- omap_i2c_write_reg(dev, OMAP_I2C_BUF_REG, buf);
- }
-
/* Take the I2C module out of reset: */
omap_i2c_write_reg(dev, OMAP_I2C_CON_REG, OMAP_I2C_CON_EN);
@@ -508,6 +502,45 @@ static int omap_i2c_wait_for_bb(struct omap_i2c_dev *dev)
return 0;
}
+static void omap_i2c_resize_fifo(struct omap_i2c_dev *dev, u8 size, bool is_rx)
+{
+ u16 buf;
+
+ if (dev->flags & OMAP_I2C_FLAG_NO_FIFO)
+ return;
+
+ /*
+* Set up notification threshold based on message size. We're doing
+* this to try and avoid draining feature as much as possible. Whenever
+* we have big messages to transfer (bigger than our total fifo size)
+* then we might use draining feature to transfer the remaining bytes.
+*/
+
+ dev->threshold = clamp(size, (u8) 1, dev->fifo_size);
+
+ buf = omap_i2c_read_reg(dev, OMAP_I2C_BUF_REG);
+
+ if (is_rx) {
+ /* Clear RX Threshold */
+ buf &= ~(0x3f << 8);
+ buf |= ((dev->threshold - 1) << 8) | OMAP_I2C_BUF_RXFIF_CLR;
+ } else {
+ /* Clear TX Threshold */
+ buf &= ~0x3f;
+ buf |= (dev->threshold - 1) | OMAP_I2C_BUF_TXFIF_CLR;
+ }
+
+ omap_i2c_write_reg(dev, OMAP_I2C_BUF_REG, buf);
+
+ if (dev->rev < OMAP_I2C_REV_ON_3530_4430)
+ dev->b_hw = 1; /* Enable hardware fixes */
+
+ /* calculate wakeup latency constraint for MPU */
+ if (dev->set_mpu_wkup_lat != NULL)
+ dev->latency = (100 * dev->threshold) /
+ (1000 * dev->speed / 8);
+}
+
/*
* Low level master read/write transaction.
*/
@@ -524,6 +557,9 @@ static int omap_i2c_xfer_msg(struct i2c_adapter *adap,
if (msg->len == 0)
return -EINVAL;
+ dev->receiver = !!(msg->flags & I2C_M_RD);
+ omap_i2c_resize_fifo(dev, msg->len, dev->receiver);
+
omap_i2c_write_reg(dev, OMAP_I2C_SA_REG, msg->addr);
/* REVISIT: Could the STB bit of I2C_CON be used with probing? */
@@ -539,7 +575,6 @@ static int omap_i2c_xfer_msg(struct i2c_adapter *adap,
init_completion(&dev->cmd_complete);
dev->cmd_err = 0;
- dev->receiver = !!(msg->flags & I2C_M_RD);
w = OMAP_I2C_CON_EN | OMAP_I2C_CON_MST | OMAP_I2C_CON_STT;
@@ -803,12 +838,6 @@ static void omap_i2c_receive_data(struct omap_i2c_dev
*dev, u8 num_bytes,
u16 w;
while (num_bytes--) {
- if (!dev->buf_len) {
- dev_err(dev->dev, "%s without data",
- is_rdr ? "RDR" : "RRDY");
- break;
- }
-
w = omap_i2c_read_reg(dev, OMAP_I2C_DATA_REG);
*dev->buf++ = w;
dev->buf_len--;
@@ -818,10 +847,8 @@ static void omap_i2c_receive_data(struct omap_i2c_dev
*dev, u8 num_bytes,
* omap4 is 8 bit wide
*/
if (dev->flags & OMAP_I2C_FLAG_16BIT_DATA_REG) {
- if (dev->buf_len) {
- *dev->buf++ = w >> 8;
- dev->buf_len--;
- }
+ *dev->buf++ = w >> 8;
+ dev->buf_len--;
}
}
}
@@ -832,12 +859,6 @@ static int omap_i2c
[PATCH 03/17] i2c: omap: add blank lines
trivial patch to aid readability. No functional
changes.
Signed-off-by: Felipe Balbi
---
drivers/i2c/busses/i2c-omap.c |5 +
1 file changed, 5 insertions(+)
diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
index 39d5583..07ae391 100644
--- a/drivers/i2c/busses/i2c-omap.c
+++ b/drivers/i2c/busses/i2c-omap.c
@@ -829,6 +829,7 @@ complete:
dev_err(dev->dev, "Arbitration lost\n");
err |= OMAP_I2C_STAT_AL;
}
+
/*
* ProDB0017052: Clear ARDY bit twice
*/
@@ -841,6 +842,7 @@ complete:
omap_i2c_complete_cmd(dev, err);
return IRQ_HANDLED;
}
+
if (stat & (OMAP_I2C_STAT_RRDY | OMAP_I2C_STAT_RDR)) {
u8 num_bytes = 1;
@@ -887,6 +889,7 @@ complete:
stat & (OMAP_I2C_STAT_RRDY |
OMAP_I2C_STAT_RDR));
continue;
}
+
if (stat & (OMAP_I2C_STAT_XRDY | OMAP_I2C_STAT_XDR)) {
u8 num_bytes = 1;
if (dev->fifo_size) {
@@ -934,10 +937,12 @@ complete:
stat & (OMAP_I2C_STAT_XRDY |
OMAP_I2C_STAT_XDR));
continue;
}
+
if (stat & OMAP_I2C_STAT_ROVR) {
dev_err(dev->dev, "Receive overrun\n");
dev->cmd_err |= OMAP_I2C_STAT_ROVR;
}
+
if (stat & OMAP_I2C_STAT_XUDF) {
dev_err(dev->dev, "Transmit underflow\n");
dev->cmd_err |= OMAP_I2C_STAT_XUDF;
--
1.7.10.4
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 00/17] Big OMAP I2C Cleanup
Hi guys,
I have dropped a few patches from the series and also
tested every single patch on my pandaboard. I2C messages
are still working fine with panda after each patch.
There's still lots of work to be done on the i2c-omap.c
driver but it's now easier to read, IMO.
Felipe Balbi (17):
i2c: omap: simplify num_bytes handling
i2c: omap: decrease indentation level on data handling
i2c: omap: add blank lines
i2c: omap: simplify omap_i2c_ack_stat()
i2c: omap: split out [XR]DR and [XR]RDY
i2c: omap: improve 1p153 errata handling
i2c: omap: re-factor receive/transmit data loop
i2c: omap: switch over to do {} while loop
i2c: omap: ack IRQ in parts
i2c: omap: get rid of the "complete" label
i2c: omap: switch to devm_* API
i2c: omap: switch to platform_get_irq()
i2c: omap: bus: add a receiver flag
i2c: omap: simplify errata check
i2c: omap: always return IRQ_HANDLED
i2c: omap: simplify IRQ exit path
i2c: omap: resize fifos before each message
drivers/i2c/busses/i2c-omap.c | 388 +++--
1 file changed, 222 insertions(+), 166 deletions(-)
--
1.7.10.4
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 02/17] i2c: omap: decrease indentation level on data handling
trivial patch, no functional changes.
Signed-off-by: Felipe Balbi
---
drivers/i2c/busses/i2c-omap.c | 63 -
1 file changed, 31 insertions(+), 32 deletions(-)
diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
index 9b532cd..39d5583 100644
--- a/drivers/i2c/busses/i2c-omap.c
+++ b/drivers/i2c/busses/i2c-omap.c
@@ -856,22 +856,7 @@ complete:
>> 8) & 0x3F;
}
while (num_bytes--) {
- w = omap_i2c_read_reg(dev, OMAP_I2C_DATA_REG);
- if (dev->buf_len) {
- *dev->buf++ = w;
- dev->buf_len--;
- /*
-* Data reg in 2430, omap3 and
-* omap4 is 8 bit wide
-*/
- if (dev->flags &
-OMAP_I2C_FLAG_16BIT_DATA_REG) {
- if (dev->buf_len) {
- *dev->buf++ = w >> 8;
- dev->buf_len--;
- }
- }
- } else {
+ if (!dev->buf_len) {
if (stat & OMAP_I2C_STAT_RRDY)
dev_err(dev->dev,
"RRDY IRQ while no data"
@@ -882,6 +867,21 @@ complete:
" requested\n");
break;
}
+
+ w = omap_i2c_read_reg(dev, OMAP_I2C_DATA_REG);
+ *dev->buf++ = w;
+ dev->buf_len--;
+ /*
+* Data reg in 2430, omap3 and
+* omap4 is 8 bit wide
+*/
+ if (dev->flags &
+ OMAP_I2C_FLAG_16BIT_DATA_REG) {
+ if (dev->buf_len) {
+ *dev->buf++ = w >> 8;
+ dev->buf_len--;
+ }
+ }
}
omap_i2c_ack_stat(dev,
stat & (OMAP_I2C_STAT_RRDY |
OMAP_I2C_STAT_RDR));
@@ -898,22 +898,7 @@ complete:
& 0x3F;
}
while (num_bytes--) {
- w = 0;
- if (dev->buf_len) {
- w = *dev->buf++;
- dev->buf_len--;
- /*
-* Data reg in 2430, omap3 and
-* omap4 is 8 bit wide
-*/
- if (dev->flags &
-OMAP_I2C_FLAG_16BIT_DATA_REG) {
- if (dev->buf_len) {
- w |= *dev->buf++ << 8;
- dev->buf_len--;
- }
- }
- } else {
+ if (!dev->buf_len) {
if (stat & OMAP_I2C_STAT_XRDY)
dev_err(dev->dev,
"XRDY IRQ while no "
@@ -925,6 +910,20 @@ complete:
break;
}
+ w = *dev->buf++;
+ dev->buf_len--;
+ /*
+* Data reg in 2430, omap3 and
+* omap4 is 8 bit wide
+*/
+ if (dev->flags &
+ OMAP_I2C_FLAG_16BIT_DATA_REG) {
+ if (dev->buf_len) {
+ w |= *dev->buf++ << 8;
+ dev->buf_len--;
+
[PATCH 01/17] i2c: omap: simplify num_bytes handling
trivial patch, no functional changes
Signed-off-by: Felipe Balbi
---
drivers/i2c/busses/i2c-omap.c |6 ++
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
index 801df60..9b532cd 100644
--- a/drivers/i2c/busses/i2c-omap.c
+++ b/drivers/i2c/busses/i2c-omap.c
@@ -855,8 +855,7 @@ complete:
OMAP_I2C_BUFSTAT_REG)
>> 8) & 0x3F;
}
- while (num_bytes) {
- num_bytes--;
+ while (num_bytes--) {
w = omap_i2c_read_reg(dev, OMAP_I2C_DATA_REG);
if (dev->buf_len) {
*dev->buf++ = w;
@@ -898,8 +897,7 @@ complete:
OMAP_I2C_BUFSTAT_REG)
& 0x3F;
}
- while (num_bytes) {
- num_bytes--;
+ while (num_bytes--) {
w = 0;
if (dev->buf_len) {
w = *dev->buf++;
--
1.7.10.4
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 05/17] i2c: omap: split out [XR]DR and [XR]RDY
While they do pretty much the same thing, there
are a few peculiarities. Specially WRT erratas,
it's best to split those out and re-factor the
read/write loop to another function which both
cases call.
This last part will be done on another patch.
While at that, also avoid an unncessary register
read since dev->fifo_len will always contain the
correct amount of data to be transferred.
Signed-off-by: Felipe Balbi
---
drivers/i2c/busses/i2c-omap.c | 126 ++---
1 file changed, 92 insertions(+), 34 deletions(-)
diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
index fa9ddb6..0661ca1 100644
--- a/drivers/i2c/busses/i2c-omap.c
+++ b/drivers/i2c/busses/i2c-omap.c
@@ -844,36 +844,62 @@ complete:
return IRQ_HANDLED;
}
- if (stat & (OMAP_I2C_STAT_RRDY | OMAP_I2C_STAT_RDR)) {
+ if (stat & OMAP_I2C_STAT_RDR) {
u8 num_bytes = 1;
+ if (dev->fifo_size)
+ num_bytes = dev->fifo_size;
+
+ while (num_bytes--) {
+ if (!dev->buf_len) {
+ dev_err(dev->dev,
+ "RDR IRQ while no data"
+ " requested\n");
+ break;
+ }
+
+ w = omap_i2c_read_reg(dev, OMAP_I2C_DATA_REG);
+ *dev->buf++ = w;
+ dev->buf_len--;
+
+ /*
+* Data reg in 2430, omap3 and
+* omap4 is 8 bit wide
+*/
+ if (dev->flags &
+ OMAP_I2C_FLAG_16BIT_DATA_REG) {
+ if (dev->buf_len) {
+ *dev->buf++ = w >> 8;
+ dev->buf_len--;
+ }
+ }
+ }
+
if (dev->errata & I2C_OMAP_ERRATA_I207)
i2c_omap_errata_i207(dev, stat);
- if (dev->fifo_size) {
- if (stat & OMAP_I2C_STAT_RRDY)
- num_bytes = dev->fifo_size;
- else/* read RXSTAT on RDR interrupt */
- num_bytes = (omap_i2c_read_reg(dev,
- OMAP_I2C_BUFSTAT_REG)
- >> 8) & 0x3F;
- }
+ omap_i2c_ack_stat(dev, OMAP_I2C_STAT_RDR);
+ continue;
+ }
+
+ if (stat & OMAP_I2C_STAT_RRDY) {
+ u8 num_bytes = 1;
+
+ if (dev->fifo_size)
+ num_bytes = dev->fifo_size;
+
while (num_bytes--) {
if (!dev->buf_len) {
- if (stat & OMAP_I2C_STAT_RRDY)
- dev_err(dev->dev,
+ dev_err(dev->dev,
"RRDY IRQ while no data"
- " requested\n");
- if (stat & OMAP_I2C_STAT_RDR)
- dev_err(dev->dev,
- "RDR IRQ while no data"
- " requested\n");
+ " requested\n");
break;
}
w = omap_i2c_read_reg(dev, OMAP_I2C_DATA_REG);
*dev->buf++ = w;
dev->buf_len--;
+
/*
* Data reg in 2430, omap3 and
* omap4 is 8 bit wide
@@ -886,36 +912,68 @@ complete:
}
}
}
- omap_i2c_ack_stat(dev, (OMAP_I2C_STAT_RRDY |
- OMAP_I2C_STAT_RDR));
+
+ omap_i2c_ack_stat(dev, OMAP_I2C_STAT_RRDY);
continue;
}
- if (stat & (OMAP_I2C_STAT_XRDY | OMAP_I2C_STAT_XDR)) {
+ if (stat & OMAP_I2C_STAT_XDR) {
[PATCH 07/17] i2c: omap: re-factor receive/transmit data loop
re-factor the common parts to a separate function,
so that code is easier to read and understand.
No functional changes.
Signed-off-by: Felipe Balbi
---
drivers/i2c/busses/i2c-omap.c | 208 +
1 file changed, 84 insertions(+), 124 deletions(-)
diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
index 52861c2..381bb36 100644
--- a/drivers/i2c/busses/i2c-omap.c
+++ b/drivers/i2c/busses/i2c-omap.c
@@ -795,12 +795,81 @@ static int errata_omap3_1p153(struct omap_i2c_dev *dev)
return 0;
}
+static void omap_i2c_receive_data(struct omap_i2c_dev *dev, u8 num_bytes,
+ bool is_rdr)
+{
+ u16 w;
+
+ while (num_bytes--) {
+ if (!dev->buf_len) {
+ dev_err(dev->dev, "%s without data",
+ is_rdr ? "RDR" : "RRDY");
+ break;
+ }
+
+ w = omap_i2c_read_reg(dev, OMAP_I2C_DATA_REG);
+ *dev->buf++ = w;
+ dev->buf_len--;
+
+ /*
+* Data reg in 2430, omap3 and
+* omap4 is 8 bit wide
+*/
+ if (dev->flags & OMAP_I2C_FLAG_16BIT_DATA_REG) {
+ if (dev->buf_len) {
+ *dev->buf++ = w >> 8;
+ dev->buf_len--;
+ }
+ }
+ }
+}
+
+static int omap_i2c_transmit_data(struct omap_i2c_dev *dev, u8 num_bytes,
+ bool is_xdr)
+{
+ u16 w;
+
+ while (num_bytes--) {
+ if (!dev->buf_len) {
+ dev_err(dev->dev, "%s without data",
+ is_xdr ? "XDR" : "XRDY");
+ break;
+ }
+
+ w = *dev->buf++;
+ dev->buf_len--;
+
+ /*
+* Data reg in 2430, omap3 and
+* omap4 is 8 bit wide
+*/
+ if (dev->flags & OMAP_I2C_FLAG_16BIT_DATA_REG) {
+ if (dev->buf_len) {
+ w |= *dev->buf++ << 8;
+ dev->buf_len--;
+ }
+ }
+
+ if (dev->errata & I2C_OMAP3_1P153) {
+ int ret;
+
+ ret = errata_omap3_1p153(dev);
+ if (ret < 0)
+ return ret;
+ }
+
+ omap_i2c_write_reg(dev, OMAP_I2C_DATA_REG, w);
+ }
+
+ return 0;
+}
+
static irqreturn_t
omap_i2c_isr(int this_irq, void *dev_id)
{
struct omap_i2c_dev *dev = dev_id;
u16 bits;
- u16 stat, w;
+ u16 stat;
int err, count = 0;
if (pm_runtime_suspended(dev->dev))
@@ -853,30 +922,7 @@ complete:
if (dev->fifo_size)
num_bytes = dev->fifo_size;
- while (num_bytes--) {
- if (!dev->buf_len) {
- dev_err(dev->dev,
- "RDR IRQ while no data"
- " requested\n");
- break;
- }
-
- w = omap_i2c_read_reg(dev, OMAP_I2C_DATA_REG);
- *dev->buf++ = w;
- dev->buf_len--;
-
- /*
-* Data reg in 2430, omap3 and
-* omap4 is 8 bit wide
-*/
- if (dev->flags &
- OMAP_I2C_FLAG_16BIT_DATA_REG) {
- if (dev->buf_len) {
- *dev->buf++ = w >> 8;
- dev->buf_len--;
- }
- }
- }
+ omap_i2c_receive_data(dev, num_bytes, true);
if (dev->errata & I2C_OMAP_ERRATA_I207)
i2c_omap_errata_i207(dev, stat);
@@ -891,78 +937,23 @@ complete:
if (dev->fifo_size)
num_bytes = dev->fifo_size;
- while (num_bytes--) {
- if (!dev->buf_len) {
- dev_err(dev->dev,
- "RRDY IRQ while no data"
- " requested\n");
- break;
- }
-
- w = omap_i2c_read_reg(dev, O
[PATCH 10/17] i2c: omap: get rid of the "complete" label
we can ack stat and complete the command from
the errata handling itself.
Signed-off-by: Felipe Balbi
---
drivers/i2c/busses/i2c-omap.c | 13 -
1 file changed, 8 insertions(+), 5 deletions(-)
diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
index c00ba7d..c20c45f 100644
--- a/drivers/i2c/busses/i2c-omap.c
+++ b/drivers/i2c/busses/i2c-omap.c
@@ -893,7 +893,6 @@ omap_i2c_isr(int this_irq, void *dev_id)
return IRQ_HANDLED;
}
-complete:
if (stat & OMAP_I2C_STAT_NACK) {
dev_err(dev->dev, "No Acknowledge\n");
err |= OMAP_I2C_STAT_NACK;
@@ -958,10 +957,12 @@ complete:
num_bytes = dev->fifo_size;
ret = omap_i2c_transmit_data(dev, num_bytes, true);
- stat = omap_i2c_read_reg(dev, OMAP_I2C_STAT_REG);
if (ret < 0) {
err |= OMAP_I2C_STAT_XUDF;
- goto complete;
+ omap_i2c_ack_stat(dev, OMAP_I2C_STAT_XUDF |
+ OMAP_I2C_STAT_XDR);
+ omap_i2c_complete_cmd(dev, err);
+ return IRQ_HANDLED;
}
omap_i2c_ack_stat(dev, OMAP_I2C_STAT_XDR);
@@ -976,10 +977,12 @@ complete:
num_bytes = dev->fifo_size;
ret = omap_i2c_transmit_data(dev, num_bytes, false);
- stat = omap_i2c_read_reg(dev, OMAP_I2C_STAT_REG);
if (ret < 0) {
err |= OMAP_I2C_STAT_XUDF;
- goto complete;
+ omap_i2c_ack_stat(dev, OMAP_I2C_STAT_XUDF |
+ OMAP_I2C_STAT_XRDY);
+ omap_i2c_complete_cmd(dev, err);
+ return IRQ_HANDLED;
}
omap_i2c_ack_stat(dev, OMAP_I2C_STAT_XRDY);
--
1.7.10.4
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 12/17] i2c: omap: switch to platform_get_irq()
that's a nice helper from drivers core which
will give us the exact IRQ number, instead
of a pointer to an IRQ resource.
Signed-off-by: Felipe Balbi
---
drivers/i2c/busses/i2c-omap.c | 11 ++-
1 file changed, 6 insertions(+), 5 deletions(-)
diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
index d805270..43d3289 100644
--- a/drivers/i2c/busses/i2c-omap.c
+++ b/drivers/i2c/busses/i2c-omap.c
@@ -1045,11 +1045,12 @@ omap_i2c_probe(struct platform_device *pdev)
{
struct omap_i2c_dev *dev;
struct i2c_adapter *adap;
- struct resource *mem, *irq;
+ struct resource *mem;
struct omap_i2c_bus_platform_data *pdata = pdev->dev.platform_data;
struct device_node *node = pdev->dev.of_node;
const struct of_device_id *match;
irq_handler_t isr;
+ int irq;
int r;
/* NOTE: driver uses the static register mapping */
@@ -1059,10 +1060,10 @@ omap_i2c_probe(struct platform_device *pdev)
return -ENODEV;
}
- irq = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
- if (!irq) {
+ irq = platform_get_irq(pdev, 0);
+ if (irq < 0) {
dev_err(&pdev->dev, "no irq resource?\n");
- return -ENODEV;
+ return irq;
}
dev = devm_kzalloc(&pdev->dev, sizeof(struct omap_i2c_dev), GFP_KERNEL);
@@ -1090,7 +1091,7 @@ omap_i2c_probe(struct platform_device *pdev)
}
dev->dev = &pdev->dev;
- dev->irq = irq->start;
+ dev->irq = irq;
dev->base = devm_request_and_ioremap(&pdev->dev, mem);
if (!dev->base) {
dev_err(&pdev->dev, "ioremap failed\n");
--
1.7.10.4
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 15/17] i2c: omap: always return IRQ_HANDLED
otherwise we could get our IRQ line disabled due
to many spurious IRQs.
Signed-off-by: Felipe Balbi
---
drivers/i2c/busses/i2c-omap.c |2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
index fc5b8bc..5b78a73 100644
--- a/drivers/i2c/busses/i2c-omap.c
+++ b/drivers/i2c/busses/i2c-omap.c
@@ -1015,7 +1015,7 @@ omap_i2c_isr(int this_irq, void *dev_id)
}
} while (stat);
- return count ? IRQ_HANDLED : IRQ_NONE;
+ return IRQ_HANDLED;
}
static const struct i2c_algorithm omap_i2c_algo = {
--
1.7.10.4
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 14/17] i2c: omap: simplify errata check
omap_i2c_dev is allocated with kzalloc(), so we need not initialize b_hw to zero. Signed-off-by: Felipe Balbi --- drivers/i2c/busses/i2c-omap.c |4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c index fad5f6f..fc5b8bc 100644 --- a/drivers/i2c/busses/i2c-omap.c +++ b/drivers/i2c/busses/i2c-omap.c @@ -1139,9 +1139,7 @@ omap_i2c_probe(struct platform_device *pdev) dev->fifo_size = (dev->fifo_size / 2); - if (dev->rev >= OMAP_I2C_REV_ON_3530_4430) - dev->b_hw = 0; /* Disable hardware fixes */ - else + if (dev->rev < OMAP_I2C_REV_ON_3530_4430) dev->b_hw = 1; /* Enable hardware fixes */ /* calculate wakeup latency constraint for MPU */ -- 1.7.10.4 -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 16/17] i2c: omap: simplify IRQ exit path
instead of having multiple return points, use
a goto statement to make that clearer.
Signed-off-by: Felipe Balbi
---
drivers/i2c/busses/i2c-omap.c | 33 -
1 file changed, 12 insertions(+), 21 deletions(-)
diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
index 5b78a73..6e75d03 100644
--- a/drivers/i2c/busses/i2c-omap.c
+++ b/drivers/i2c/busses/i2c-omap.c
@@ -889,33 +889,27 @@ omap_i2c_isr(int this_irq, void *dev_id)
stat &= ~(OMAP_I2C_STAT_RDR | OMAP_I2C_STAT_RRDY);
}
- if (!stat) {
- /* my work here is done */
- omap_i2c_complete_cmd(dev, err);
- return IRQ_HANDLED;
- }
+ if (!stat)
+ goto out;
dev_dbg(dev->dev, "IRQ (ISR = 0x%04x)\n", stat);
if (count++ == 100) {
dev_warn(dev->dev, "Too much work in one IRQ\n");
- omap_i2c_complete_cmd(dev, err);
- return IRQ_HANDLED;
+ goto out;
}
if (stat & OMAP_I2C_STAT_NACK) {
dev_err(dev->dev, "No Acknowledge\n");
err |= OMAP_I2C_STAT_NACK;
omap_i2c_ack_stat(dev, OMAP_I2C_STAT_NACK);
- omap_i2c_complete_cmd(dev, err);
- return IRQ_HANDLED;
+ goto out;
}
if (stat & OMAP_I2C_STAT_AL) {
dev_err(dev->dev, "Arbitration lost\n");
err |= OMAP_I2C_STAT_AL;
omap_i2c_ack_stat(dev, OMAP_I2C_STAT_NACK);
- omap_i2c_complete_cmd(dev, err);
- return IRQ_HANDLED;
+ goto out;
}
/*
@@ -928,8 +922,7 @@ omap_i2c_isr(int this_irq, void *dev_id)
OMAP_I2C_STAT_XRDY |
OMAP_I2C_STAT_XDR |
OMAP_I2C_STAT_ARDY));
- omap_i2c_complete_cmd(dev, err);
- return IRQ_HANDLED;
+ goto out;
}
if (stat & OMAP_I2C_STAT_RDR) {
@@ -970,8 +963,7 @@ omap_i2c_isr(int this_irq, void *dev_id)
err |= OMAP_I2C_STAT_XUDF;
omap_i2c_ack_stat(dev, OMAP_I2C_STAT_XUDF |
OMAP_I2C_STAT_XDR);
- omap_i2c_complete_cmd(dev, err);
- return IRQ_HANDLED;
+ goto out;
}
omap_i2c_ack_stat(dev, OMAP_I2C_STAT_XDR);
@@ -990,8 +982,7 @@ omap_i2c_isr(int this_irq, void *dev_id)
err |= OMAP_I2C_STAT_XUDF;
omap_i2c_ack_stat(dev, OMAP_I2C_STAT_XUDF |
OMAP_I2C_STAT_XRDY);
- omap_i2c_complete_cmd(dev, err);
- return IRQ_HANDLED;
+ goto out;
}
omap_i2c_ack_stat(dev, OMAP_I2C_STAT_XRDY);
@@ -1002,19 +993,19 @@ omap_i2c_isr(int this_irq, void *dev_id)
dev_err(dev->dev, "Receive overrun\n");
err |= OMAP_I2C_STAT_ROVR;
omap_i2c_ack_stat(dev, OMAP_I2C_STAT_ROVR);
- omap_i2c_complete_cmd(dev, err);
- return IRQ_HANDLED;
+ goto out;
}
if (stat & OMAP_I2C_STAT_XUDF) {
dev_err(dev->dev, "Transmit underflow\n");
err |= OMAP_I2C_STAT_XUDF;
omap_i2c_ack_stat(dev, OMAP_I2C_STAT_XUDF);
- omap_i2c_complete_cmd(dev, err);
- return IRQ_HANDLED;
+ goto out;
}
} while (stat);
+out:
+ omap_i2c_complete_cmd(dev, err);
return IRQ_HANDLED;
}
--
1.7.10.4
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 13/17] i2c: omap: bus: add a receiver flag
that way we can ignore TX IRQs while in receiver
mode and ignore RX IRQs while in transmitter mode.
Signed-off-by: Felipe Balbi
---
drivers/i2c/busses/i2c-omap.c |9 +
1 file changed, 9 insertions(+)
diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
index 43d3289..fad5f6f 100644
--- a/drivers/i2c/busses/i2c-omap.c
+++ b/drivers/i2c/busses/i2c-omap.c
@@ -199,6 +199,7 @@ struct omap_i2c_dev {
*/
u8 rev;
unsignedb_hw:1; /* bad h/w fixes */
+ unsignedreceiver:1; /* true when we're in receiver
mode */
u16 iestate;/* Saved interrupt register */
u16 pscstate;
u16 scllstate;
@@ -538,6 +539,7 @@ static int omap_i2c_xfer_msg(struct i2c_adapter *adap,
init_completion(&dev->cmd_complete);
dev->cmd_err = 0;
+ dev->receiver = !!(msg->flags & I2C_M_RD);
w = OMAP_I2C_CON_EN | OMAP_I2C_CON_MST | OMAP_I2C_CON_STT;
@@ -880,6 +882,13 @@ omap_i2c_isr(int this_irq, void *dev_id)
stat = omap_i2c_read_reg(dev, OMAP_I2C_STAT_REG);
stat &= bits;
+ /* If we're in receiver mode, ignore XDR/XRDY */
+ if (dev->receiver) {
+ stat &= ~(OMAP_I2C_STAT_XDR | OMAP_I2C_STAT_XRDY);
+ } else {
+ stat &= ~(OMAP_I2C_STAT_RDR | OMAP_I2C_STAT_RRDY);
+ }
+
if (!stat) {
/* my work here is done */
omap_i2c_complete_cmd(dev, err);
--
1.7.10.4
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 11/17] i2c: omap: switch to devm_* API
that helps deleting some boiler plate code
and lets driver-core manage our resources
for us.
Signed-off-by: Felipe Balbi
---
drivers/i2c/busses/i2c-omap.c | 43 +++--
1 file changed, 11 insertions(+), 32 deletions(-)
diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
index c20c45f..d805270 100644
--- a/drivers/i2c/busses/i2c-omap.c
+++ b/drivers/i2c/busses/i2c-omap.c
@@ -1045,7 +1045,7 @@ omap_i2c_probe(struct platform_device *pdev)
{
struct omap_i2c_dev *dev;
struct i2c_adapter *adap;
- struct resource *mem, *irq, *ioarea;
+ struct resource *mem, *irq;
struct omap_i2c_bus_platform_data *pdata = pdev->dev.platform_data;
struct device_node *node = pdev->dev.of_node;
const struct of_device_id *match;
@@ -1058,23 +1058,17 @@ omap_i2c_probe(struct platform_device *pdev)
dev_err(&pdev->dev, "no mem resource?\n");
return -ENODEV;
}
+
irq = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
if (!irq) {
dev_err(&pdev->dev, "no irq resource?\n");
return -ENODEV;
}
- ioarea = request_mem_region(mem->start, resource_size(mem),
- pdev->name);
- if (!ioarea) {
- dev_err(&pdev->dev, "I2C region already claimed\n");
- return -EBUSY;
- }
-
- dev = kzalloc(sizeof(struct omap_i2c_dev), GFP_KERNEL);
+ dev = devm_kzalloc(&pdev->dev, sizeof(struct omap_i2c_dev), GFP_KERNEL);
if (!dev) {
- r = -ENOMEM;
- goto err_release_region;
+ dev_err(&pdev->dev, "not enough memory\n");
+ return -ENOMEM;
}
match = of_match_device(of_match_ptr(omap_i2c_of_match), &pdev->dev);
@@ -1097,10 +1091,10 @@ omap_i2c_probe(struct platform_device *pdev)
dev->dev = &pdev->dev;
dev->irq = irq->start;
- dev->base = ioremap(mem->start, resource_size(mem));
+ dev->base = devm_request_and_ioremap(&pdev->dev, mem);
if (!dev->base) {
- r = -ENOMEM;
- goto err_free_mem;
+ dev_err(&pdev->dev, "ioremap failed\n");
+ return -ENOMEM;
}
platform_set_drvdata(pdev, dev);
@@ -1151,7 +1145,7 @@ omap_i2c_probe(struct platform_device *pdev)
isr = (dev->rev < OMAP_I2C_OMAP1_REV_2) ? omap_i2c_omap1_isr :
omap_i2c_isr;
- r = request_irq(dev->irq, isr, 0, pdev->name, dev);
+ r = devm_request_irq(&pdev->dev, dev->irq, isr, 0, pdev->name, dev);
if (r) {
dev_err(dev->dev, "failure requesting irq %i\n", dev->irq);
@@ -1177,24 +1171,16 @@ omap_i2c_probe(struct platform_device *pdev)
r = i2c_add_numbered_adapter(adap);
if (r) {
dev_err(dev->dev, "failure adding adapter\n");
- goto err_free_irq;
+ goto err_unuse_clocks;
}
of_i2c_register_devices(adap);
return 0;
-err_free_irq:
- free_irq(dev->irq, dev);
err_unuse_clocks:
omap_i2c_write_reg(dev, OMAP_I2C_CON_REG, 0);
pm_runtime_put(dev->dev);
- iounmap(dev->base);
-err_free_mem:
- platform_set_drvdata(pdev, NULL);
- kfree(dev);
-err_release_region:
- release_mem_region(mem->start, resource_size(mem));
return r;
}
@@ -1203,17 +1189,10 @@ static int
omap_i2c_remove(struct platform_device *pdev)
{
struct omap_i2c_dev *dev = platform_get_drvdata(pdev);
- struct resource *mem;
- platform_set_drvdata(pdev, NULL);
-
- free_irq(dev->irq, dev);
i2c_del_adapter(&dev->adapter);
omap_i2c_write_reg(dev, OMAP_I2C_CON_REG, 0);
- iounmap(dev->base);
- kfree(dev);
- mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
- release_mem_region(mem->start, resource_size(mem));
+
return 0;
}
--
1.7.10.4
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 09/17] i2c: omap: ack IRQ in parts
According to flow diagrams on OMAP TRMs,
we should ACK the IRQ as they happen.
Signed-off-by: Felipe Balbi
---
drivers/i2c/busses/i2c-omap.c | 29 +
1 file changed, 17 insertions(+), 12 deletions(-)
diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
index f978b14..c00ba7d 100644
--- a/drivers/i2c/busses/i2c-omap.c
+++ b/drivers/i2c/busses/i2c-omap.c
@@ -894,21 +894,20 @@ omap_i2c_isr(int this_irq, void *dev_id)
}
complete:
- /*
-* Ack the stat in one go, but [R/X]DR and [R/X]RDY should be
-* acked after the data operation is complete.
-* Ref: TRM SWPU114Q Figure 18-31
-*/
- omap_i2c_write_reg(dev, OMAP_I2C_STAT_REG, stat &
- ~(OMAP_I2C_STAT_RRDY | OMAP_I2C_STAT_RDR |
- OMAP_I2C_STAT_XRDY | OMAP_I2C_STAT_XDR));
-
- if (stat & OMAP_I2C_STAT_NACK)
+ if (stat & OMAP_I2C_STAT_NACK) {
+ dev_err(dev->dev, "No Acknowledge\n");
err |= OMAP_I2C_STAT_NACK;
+ omap_i2c_ack_stat(dev, OMAP_I2C_STAT_NACK);
+ omap_i2c_complete_cmd(dev, err);
+ return IRQ_HANDLED;
+ }
if (stat & OMAP_I2C_STAT_AL) {
dev_err(dev->dev, "Arbitration lost\n");
err |= OMAP_I2C_STAT_AL;
+ omap_i2c_ack_stat(dev, OMAP_I2C_STAT_NACK);
+ omap_i2c_complete_cmd(dev, err);
+ return IRQ_HANDLED;
}
/*
@@ -989,12 +988,18 @@ complete:
if (stat & OMAP_I2C_STAT_ROVR) {
dev_err(dev->dev, "Receive overrun\n");
- dev->cmd_err |= OMAP_I2C_STAT_ROVR;
+ err |= OMAP_I2C_STAT_ROVR;
+ omap_i2c_ack_stat(dev, OMAP_I2C_STAT_ROVR);
+ omap_i2c_complete_cmd(dev, err);
+ return IRQ_HANDLED;
}
if (stat & OMAP_I2C_STAT_XUDF) {
dev_err(dev->dev, "Transmit underflow\n");
- dev->cmd_err |= OMAP_I2C_STAT_XUDF;
+ err |= OMAP_I2C_STAT_XUDF;
+ omap_i2c_ack_stat(dev, OMAP_I2C_STAT_XUDF);
+ omap_i2c_complete_cmd(dev, err);
+ return IRQ_HANDLED;
}
} while (stat);
--
1.7.10.4
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 08/17] i2c: omap: switch over to do {} while loop
this will make sure that we execute at least once.
No functional changes otherwise.
Signed-off-by: Felipe Balbi
---
drivers/i2c/busses/i2c-omap.c | 21 +++--
1 file changed, 15 insertions(+), 6 deletions(-)
diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
index 381bb36..f978b14 100644
--- a/drivers/i2c/busses/i2c-omap.c
+++ b/drivers/i2c/busses/i2c-omap.c
@@ -870,20 +870,29 @@ omap_i2c_isr(int this_irq, void *dev_id)
struct omap_i2c_dev *dev = dev_id;
u16 bits;
u16 stat;
- int err, count = 0;
+ int err = 0, count = 0;
if (pm_runtime_suspended(dev->dev))
return IRQ_NONE;
- bits = omap_i2c_read_reg(dev, OMAP_I2C_IE_REG);
- while ((stat = (omap_i2c_read_reg(dev, OMAP_I2C_STAT_REG))) & bits) {
+ do {
+ bits = omap_i2c_read_reg(dev, OMAP_I2C_IE_REG);
+ stat = omap_i2c_read_reg(dev, OMAP_I2C_STAT_REG);
+ stat &= bits;
+
+ if (!stat) {
+ /* my work here is done */
+ omap_i2c_complete_cmd(dev, err);
+ return IRQ_HANDLED;
+ }
+
dev_dbg(dev->dev, "IRQ (ISR = 0x%04x)\n", stat);
if (count++ == 100) {
dev_warn(dev->dev, "Too much work in one IRQ\n");
- break;
+ omap_i2c_complete_cmd(dev, err);
+ return IRQ_HANDLED;
}
- err = 0;
complete:
/*
* Ack the stat in one go, but [R/X]DR and [R/X]RDY should be
@@ -987,7 +996,7 @@ complete:
dev_err(dev->dev, "Transmit underflow\n");
dev->cmd_err |= OMAP_I2C_STAT_XUDF;
}
- }
+ } while (stat);
return count ? IRQ_HANDLED : IRQ_NONE;
}
--
1.7.10.4
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 04/17] i2c: omap: simplify omap_i2c_ack_stat()
stat & BIT(1) is the same as BIT(1), so let's
simplify things a bit by removing "stat &" from
all omap_i2c_ack_stat() calls.
Signed-off-by: Felipe Balbi
---
drivers/i2c/busses/i2c-omap.c | 21 +++--
1 file changed, 11 insertions(+), 10 deletions(-)
diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
index 07ae391..fa9ddb6 100644
--- a/drivers/i2c/busses/i2c-omap.c
+++ b/drivers/i2c/busses/i2c-omap.c
@@ -774,8 +774,8 @@ static int errata_omap3_1p153(struct omap_i2c_dev *dev, u16
*stat, int *err)
while (--timeout && !(*stat & OMAP_I2C_STAT_XUDF)) {
if (*stat & (OMAP_I2C_STAT_NACK | OMAP_I2C_STAT_AL)) {
- omap_i2c_ack_stat(dev, *stat & (OMAP_I2C_STAT_XRDY |
- OMAP_I2C_STAT_XDR));
+ omap_i2c_ack_stat(dev, (OMAP_I2C_STAT_XRDY |
+ OMAP_I2C_STAT_XDR));
*err |= OMAP_I2C_STAT_XUDF;
return -ETIMEDOUT;
}
@@ -835,10 +835,11 @@ complete:
*/
if (stat & (OMAP_I2C_STAT_ARDY | OMAP_I2C_STAT_NACK |
OMAP_I2C_STAT_AL)) {
- omap_i2c_ack_stat(dev, stat &
- (OMAP_I2C_STAT_RRDY | OMAP_I2C_STAT_RDR |
- OMAP_I2C_STAT_XRDY | OMAP_I2C_STAT_XDR |
- OMAP_I2C_STAT_ARDY));
+ omap_i2c_ack_stat(dev, (OMAP_I2C_STAT_RRDY |
+ OMAP_I2C_STAT_RDR |
+ OMAP_I2C_STAT_XRDY |
+ OMAP_I2C_STAT_XDR |
+ OMAP_I2C_STAT_ARDY));
omap_i2c_complete_cmd(dev, err);
return IRQ_HANDLED;
}
@@ -885,8 +886,8 @@ complete:
}
}
}
- omap_i2c_ack_stat(dev,
- stat & (OMAP_I2C_STAT_RRDY |
OMAP_I2C_STAT_RDR));
+ omap_i2c_ack_stat(dev, (OMAP_I2C_STAT_RRDY |
+ OMAP_I2C_STAT_RDR));
continue;
}
@@ -933,8 +934,8 @@ complete:
omap_i2c_write_reg(dev, OMAP_I2C_DATA_REG, w);
}
- omap_i2c_ack_stat(dev,
- stat & (OMAP_I2C_STAT_XRDY |
OMAP_I2C_STAT_XDR));
+ omap_i2c_ack_stat(dev, (OMAP_I2C_STAT_XRDY |
+ OMAP_I2C_STAT_XDR));
continue;
}
--
1.7.10.4
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 06/17] i2c: omap: improve 1p153 errata handling
Make it not depend on ISR's local variables
in order to make it easier to re-factor the
transmit data loop.
Signed-off-by: Felipe Balbi
---
drivers/i2c/busses/i2c-omap.c | 47 +
1 file changed, 34 insertions(+), 13 deletions(-)
diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
index 0661ca1..52861c2 100644
--- a/drivers/i2c/busses/i2c-omap.c
+++ b/drivers/i2c/busses/i2c-omap.c
@@ -768,21 +768,24 @@ omap_i2c_omap1_isr(int this_irq, void *dev_id)
* data to DATA_REG. Otherwise some data bytes can be lost while transferring
* them from the memory to the I2C interface.
*/
-static int errata_omap3_1p153(struct omap_i2c_dev *dev, u16 *stat, int *err)
+static int errata_omap3_1p153(struct omap_i2c_dev *dev)
{
- unsigned long timeout = 1;
+ unsigned long timeout = 1;
+ u16 stat;
- while (--timeout && !(*stat & OMAP_I2C_STAT_XUDF)) {
- if (*stat & (OMAP_I2C_STAT_NACK | OMAP_I2C_STAT_AL)) {
+ do {
+ stat = omap_i2c_read_reg(dev, OMAP_I2C_STAT_REG);
+ if (stat & OMAP_I2C_STAT_XUDF)
+ break;
+
+ if (stat & (OMAP_I2C_STAT_NACK | OMAP_I2C_STAT_AL)) {
omap_i2c_ack_stat(dev, (OMAP_I2C_STAT_XRDY |
OMAP_I2C_STAT_XDR));
- *err |= OMAP_I2C_STAT_XUDF;
return -ETIMEDOUT;
}
cpu_relax();
- *stat = omap_i2c_read_reg(dev, OMAP_I2C_STAT_REG);
- }
+ } while (--timeout);
if (!timeout) {
dev_err(dev->dev, "timeout waiting on XUDF bit\n");
@@ -946,9 +949,18 @@ complete:
}
}
- if ((dev->errata & I2C_OMAP3_1P153) &&
- errata_omap3_1p153(dev, &stat, &err))
- goto complete;
+ if (dev->errata & I2C_OMAP3_1P153) {
+ int ret;
+
+ ret = errata_omap3_1p153(dev);
+ stat = omap_i2c_read_reg(dev,
+ OMAP_I2C_STAT_REG);
+
+ if (ret < 0) {
+ err |= OMAP_I2C_STAT_XUDF;
+ goto complete;
+ }
+ }
omap_i2c_write_reg(dev, OMAP_I2C_DATA_REG, w);
}
@@ -986,9 +998,18 @@ complete:
}
}
- if ((dev->errata & I2C_OMAP3_1P153) &&
- errata_omap3_1p153(dev, &stat, &err))
- goto complete;
+ if (dev->errata & I2C_OMAP3_1P153) {
+ int ret;
+
+ ret = errata_omap3_1p153(dev);
+ stat = omap_i2c_read_reg(dev,
+ OMAP_I2C_STAT_REG);
+
+ if (ret < 0) {
+ err |= OMAP_I2C_STAT_XUDF;
+ goto complete;
+ }
+ }
omap_i2c_write_reg(dev, OMAP_I2C_DATA_REG, w);
}
--
1.7.10.4
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 5/9] ARM: kirkwood: use devicetree for orion-spi
On Wed, Jun 13, 2012 at 09:10:30PM +, Arnd Bergmann wrote:
> On Sunday 10 June 2012, Andrew Lunn wrote:
> > @@ -26,6 +26,11 @@ static struct of_device_id kirkwood_dt_match_table[]
> > __initdata = {
> > { }
> > };
> >
> > +struct of_dev_auxdata kirkwood_auxdata_lookup[] __initdata = {
> > + OF_DEV_AUXDATA("marvell,orion-spi", 0xf1010600, "orion_spi.0",
> > NULL),
> > + {},
> > +};
> > +
> > static void __init kirkwood_dt_init(void)
> > {
>
> How about instead adding the clkdev for "f1010600.spi" so you don't need the
> auxdata?
We either have auxdata and clean clock code, or no auxdata and messy
clock code with lots of aliases.
The auxdata is also not limited to the name of the clocks. It allows
me to diff the kernel log for a DT boot and a none DT boot of the same
box and see they are identical. I've found a few typo's that way, and
not been hindered by noise of the devices changing name is useful.
Once we have the clock tree described in DT, then i think it makes
sense to remove these auxdata entries.
Andrew
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/9] ARM: Kirkwood: Add interrupt controller support for DT boards
On Thu, Jun 14, 2012 at 08:12:34AM +, Arnd Bergmann wrote:
> On Sunday 10 June 2012, Andrew Lunn wrote:
> > +static int __init kirkwood_add_irq_domain(struct device_node *np,
> > + struct device_node
> > *interrupt_parent)
> > +{
> > + kirkwood_init_irq();
> > + irq_domain_add_legacy(np, 64, 0, 0, &irq_domain_simple_ops, NULL);
> > + return 0;
> > +}
> > +
> > +static const struct of_device_id kirkwood_irq_match[] = {
> > + { .compatible = "marvell,orion-intc",
> > + .data = kirkwood_add_irq_domain, },
> > + {},
> > +};
> > +
> > +static void __init kirkwood_dt_init_irq(void)
> > +{
> > + of_irq_init(kirkwood_irq_match);
> > +}
> > +
>
> I think if you compute the number of interrupts in the domain from the number
> of
> registers that are described in the device tree, call orion_irq_init()
> based on those registers, and use irq domains for the gpio stuff as Rob
> suggested,
> this init_irq function can become completely generic to all orion platforms.
I'm reworking the GPIO stuff at the moment, moving it to use IRQ
domains for its interrupts. That code will be generic to all Orion
platforms. I'm modeling the code on the AT91 GPIO handler, since that
has a similar structure, one hardware interrupt for a number of GPIO
lines, which is in software demultiplexed and triggers secondary level
interrupts. The major difference is that AT91 has one hardware
interrupt for a GPIO chip with 32 lines, where as Orion has 4 hardware
interrupt lines, so maybe four interrupt domains, for one GPIO chip
with 32 lines.
Non-GPIO interrupts are more of a problem. Underneath it uses the
generic-chip interrupt code. The patchset to add _domain versions of
these calls is stalled. So i think for the moment, we need to stay
with the domain bolted on top, until generic-chip gets domain
support. I would also expect that generic-chip also gets a common DT
binding which any SoC can use.
My aim at the moment is to get something which works well enough that
we can add DT support to other drivers. Without interrupts, we are not
going to get very far. We can later rip it out what i create now and
replace it once generic-chip becomes domain and DT aware, and there
should be no effect on other drivers.
Andrew
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V3 2/3] i2c-nomadik: turn the platform driver to an amba driver
On 14/06/12 09:17, Alessandro Rubini wrote:
You change only one half of ux500 here: the part where the device
gets defined statically, but not not the definition in the
device-tree.
Yes, I'm aware. That's why I added Lee Jones as Cc:, on Linusw's
suggestion. Lee told me to go ahead and he'll fix the DT stuff.
I did?
I have already DT:ed this driver. There are 3 patches on the MLs currently.
[PATCH 1/3] i2c: Add Device Tree support to the Nomadik I2C driver
[PATCH 2/3] ARM: ux500: Add i2c configurations to the Device Tree for DB8500
based devices
[PATCH 3/3] Documentation: Device Tree binding information for i2c-nomadik
driver
I'm more than happy for you to base your patches on these and make the
necessary changes. Off hand, I think the only changes you'll need to
make is the probing from DT itself.
So something like:
--- a/arch/arm/boot/dts/db8500.dtsi
+++ b/arch/arm/boot/dts/db8500.dtsi
@@ -449,7 +449,7 @@
};
i2c@80004000 {
- compatible = "stericsson,db8500-i2c", "st,nomadik-i2c";
+ compatible = "st,nomadik-i2c", "arm,primecell";
reg = <0x80004000 0x1000>;
interrupts = <0 21 0x4>;
#address-cells = <1>;
... for each of the controllers.
--
Lee Jones
Linaro ST-Ericsson Landing Team Lead
M: +44 77 88 633 515
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V3 2/3] i2c-nomadik: turn the platform driver to an amba driver
> You change only one half of ux500 here: the part where the device > gets defined statically, but not not the definition in the > device-tree. Yes, I'm aware. That's why I added Lee Jones as Cc:, on Linusw's suggestion. Lee told me to go ahead and he'll fix the DT stuff. Lee? thanks /alessandro -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/9] ARM: Kirkwood: Add interrupt controller support for DT boards
On Sunday 10 June 2012, Andrew Lunn wrote:
> +static int __init kirkwood_add_irq_domain(struct device_node *np,
> + struct device_node
> *interrupt_parent)
> +{
> + kirkwood_init_irq();
> + irq_domain_add_legacy(np, 64, 0, 0, &irq_domain_simple_ops, NULL);
> + return 0;
> +}
> +
> +static const struct of_device_id kirkwood_irq_match[] = {
> + { .compatible = "marvell,orion-intc",
> + .data = kirkwood_add_irq_domain, },
> + {},
> +};
> +
> +static void __init kirkwood_dt_init_irq(void)
> +{
> + of_irq_init(kirkwood_irq_match);
> +}
> +
I think if you compute the number of interrupts in the domain from the number of
registers that are described in the device tree, call orion_irq_init()
based on those registers, and use irq domains for the gpio stuff as Rob
suggested,
this init_irq function can become completely generic to all orion platforms.
That is what I tried to do with an earlier patch, which was flawed for other
reasons.
Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V3 2/3] i2c-nomadik: turn the platform driver to an amba driver
On Monday 11 June 2012, Alessandro Rubini wrote: > The i2c-nomadik gateware is really a PrimeCell APB device. By hosting > the driver under the amba bus we can access it more easily, for > example using the generic pci-amba driver. The patch also fixes the > mach-ux500 users, so they register an amba device instead than a > platform device. > > Signed-off-by: Alessandro Rubini > Acked-by: Giancarlo Asnaghi > Tested-by: Linus Walleij > --- > arch/arm/mach-ux500/devices-common.h | 22 + > drivers/i2c/busses/i2c-nomadik.c | 142 + > 2 files changed, 78 insertions(+), 86 deletions(-) You change only one half of ux500 here: the part where the device gets defined statically, but not not the definition in the device-tree. I think all you need to do is mark the device compatible with primecell and add an ID if necessary, but we should definitely make sure that that path still works, because the one you patch is going away. Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 00/16] Enable SPARSE_IRQ support for imx
On Thu, Jun 14, 2012 at 01:59:31PM +0800, Shawn Guo wrote: > It seems that the lack of SPARSE_IRQ support becomes the last blocker > for imx being built with multi-platform. The series is to enable > SPARSE_IRQ for imx by having all the irqchips allocate their irq_descs. > Along with the change, a legacy irqdomain is added for each of these > irqchips (except ipu_irq) to help the mapping between hardware irq and > Linux irq number, which is required by DT boot but also benefits non-DT. > > Based on v3.5-rc2. Boot tested on imx3, imx5 and imx6, and compile > tested with imx_v4_v5_defconfig. I gave it a test on i.MX1 and i.MX27, so Tested-by: Sascha Hauer Also, nice move ;) Acked-by: Sascha Hauer Sascha > > Shawn Guo (16): > ARM: imx: eliminate macro IMX_GPIO_TO_IRQ() > ARM: imx: eliminate macro IOMUX_TO_IRQ() > ARM: imx: eliminate macro IRQ_GPIOx() > gpio/mxc: move irq_domain_add_legacy call into gpio driver > ARM: imx: move irq_domain_add_legacy call into tzic driver > ARM: imx: move irq_domain_add_legacy call into avic driver > dma: ipu: remove the use of ipu_platform_data > ARM: imx: leave irq_base of wm8350_platform_data uninitialized > ARM: imx: pass gpio than irq number into mxc_expio_init > ARM: imx: add a legacy irqdomain for 3ds_debugboard > ARM: imx: add a legacy irqdomain for mx31ads > i2c: imx: remove unneeded mach/irqs.h inclusion > ARM: imx: remove unneeded mach/irq.h inclusion > tty: serial: imx: remove the use of MXC_INTERNAL_IRQS > ARM: fiq: save FIQ_START by passing absolute fiq number > ARM: imx: enable SPARSE_IRQ for imx platform > > arch/arm/Kconfig|1 + > arch/arm/kernel/fiq.c |4 +- > arch/arm/mach-imx/devices-imx31.h |4 +- > arch/arm/mach-imx/devices-imx35.h |4 +- > arch/arm/mach-imx/eukrea_mbimx27-baseboard.c|3 +- > arch/arm/mach-imx/eukrea_mbimxsd35-baseboard.c |6 +- > arch/arm/mach-imx/imx27-dt.c| 28 --- > arch/arm/mach-imx/imx51-dt.c| 27 --- > arch/arm/mach-imx/imx53-dt.c| 27 --- > arch/arm/mach-imx/mach-apf9328.c|7 +- > arch/arm/mach-imx/mach-armadillo5x0.c | 18 +- > arch/arm/mach-imx/mach-cpuimx27.c | 12 +- > arch/arm/mach-imx/mach-cpuimx35.c |3 +- > arch/arm/mach-imx/mach-cpuimx51sd.c |3 +- > arch/arm/mach-imx/mach-imx27_visstrim_m10.c |9 +- > arch/arm/mach-imx/mach-imx6q.c | 14 -- > arch/arm/mach-imx/mach-kzm_arm11_01.c | 20 ++- > arch/arm/mach-imx/mach-mx1ads.c |1 - > arch/arm/mach-imx/mach-mx21ads.c| 16 ++- > arch/arm/mach-imx/mach-mx27_3ds.c |7 +- > arch/arm/mach-imx/mach-mx27ads.c| 12 +- > arch/arm/mach-imx/mach-mx31_3ds.c | 18 +-- > arch/arm/mach-imx/mach-mx31ads.c| 63 --- > arch/arm/mach-imx/mach-mx31lilly.c | 10 +- > arch/arm/mach-imx/mach-mx31lite.c | 11 +- > arch/arm/mach-imx/mach-mx31moboard.c| 10 +- > arch/arm/mach-imx/mach-mx35_3ds.c | 18 +-- > arch/arm/mach-imx/mach-mx51_3ds.c |3 +- > arch/arm/mach-imx/mach-mx53_ard.c |5 +- > arch/arm/mach-imx/mach-mxt_td60.c |6 +- > arch/arm/mach-imx/mach-pca100.c |5 +- > arch/arm/mach-imx/mach-pcm037.c | 24 ++-- > arch/arm/mach-imx/mach-pcm038.c |4 +- > arch/arm/mach-imx/mach-pcm043.c |6 +- > arch/arm/mach-imx/mach-qong.c | 10 +- > arch/arm/mach-imx/mach-scb9328.c|7 +- > arch/arm/mach-imx/mach-vpr200.c | 10 +- > arch/arm/mach-imx/mm-imx1.c |1 - > arch/arm/mach-imx/mm-imx21.c|1 - > arch/arm/mach-imx/mm-imx25.c|1 - > arch/arm/mach-imx/mm-imx27.c|1 - > arch/arm/mach-imx/mm-imx3.c |1 - > arch/arm/mach-imx/mx31lilly-db.c| 11 +- > arch/arm/mach-imx/mx31lite-db.c |5 +- > arch/arm/mach-imx/mx51_efika.c |3 +- > arch/arm/mach-imx/pcm970-baseboard.c| 13 +- > arch/arm/mach-rpc/include/mach/irqs.h | 12 +- > arch/arm/plat-mxc/3ds_debugboard.c | 50 +++--- > arch/arm/plat-mxc/avic.c| 26 ++- > arch/arm/plat-mxc/devices/platform-ipu-core.c |5 +- > arch/arm/plat-mxc/include/mach/3ds_debugboard.h |2 +- > arch/arm/plat-mxc/include/mach/devices-common.h |4 +- > arch/arm/plat-mxc/include/mach/hardware.h |2 - > arch/arm/plat-mxc/include/mach/iomux-mx3.h |3 - > arch/arm/plat-mxc/include/mach/iomux-v1.h |7 - > arch/arm/p
Re: [PATCH 12/16] i2c: imx: remove unneeded mach/irqs.h inclusion
On Thu, Jun 14, 2012 at 01:59:43PM +0800, Shawn Guo wrote: > Remove unneeded mach/irq.h inclusion from i2c-imx driver. > > Signed-off-by: Shawn Guo > Cc: linux-i2c@vger.kernel.org > Cc: Wolfram Sang Acked-by: Wolfram Sang Thanks! -- Pengutronix e.K. | Wolfram Sang| Industrial Linux Solutions | http://www.pengutronix.de/ | signature.asc Description: Digital signature
Re: [PATCH 1/2] i2c: stu300: use clk_prepare/unprepare
On Wed, Jun 13, 2012 at 8:22 AM, Shubhrajyoti Datta wrote: > wrote: >> >> Make sure we prepare/unprepare the clock for the ST U300 >> I2C driver as is required by the clk API especially if you >> use common clock. > > Not a comment rather a doubt. > > The transfer is still using enable and disable? > Can you help me understand the difference. Do you mean you want me to explain the difference between enable/disable and prepare/unprepare? >> drivers/i2c/busses/i2c-stu300.c | 4 +++- >> 1 file changed, 3 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/i2c/busses/i2c-stu300.c >> b/drivers/i2c/busses/i2c-stu300.c >> index 4d44af1..79b7851 100644 >> --- a/drivers/i2c/busses/i2c-stu300.c >> +++ b/drivers/i2c/busses/i2c-stu300.c >> @@ -924,7 +924,7 @@ stu300_probe(struct platform_device *pdev) >> >> dev->speed = scl_frequency; >> >> - clk_enable(dev->clk); >> + clk_prepare_enable(dev->clk); >> ret = stu300_init_hw(dev); >> clk_disable(dev->clk); >> >> @@ -960,6 +960,7 @@ stu300_probe(struct platform_device *pdev) >> >> err_add_adapter: >> err_init_hw: >> + clk_unprepare(dev->clk); > > So this also fixes an earlier lack of disable also? No, there was no lack of disable earlier. If you look just a few lines up in the patch there is a disable immediately after the stu300_init_hw() call. Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
