Re: [PATCH v4 2/2] i2c: Add Qualcomm CCI I2C driver

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi

Sorry to enter the discussion this late.

I have been using this driver with release/qcomlt-4.14 on a db820c and
if I try to use the i2c bus before camss is running I get a Timeout
error and the bus is unusable after that. I have sent a patch that
fixes this issue, in case you want to merge it/add it before this
driver goes upstream.

Is there a way to use the i2c bus without using the camera?

Cheers

On Tue, Aug 28, 2018 at 5:39 AM Vinod <vkoul@xxxxxxxxxx> wrote:
>
> On 27-08-18, 12:19, Stephen Boyd wrote:
> > Quoting Vinod (2018-08-24 03:16:35)
> > > On 24-08-18, 00:30, Stephen Boyd wrote:
> > > > Quoting Vinod Koul (2018-08-19 23:39:53)
>
> > > > > +static int cci_i2c_write(struct cci *cci, u16 master,
> > > > > +                        u16 addr, u8 *buf, u16 len)
> > > > > +{
> > > > > +       u8 queue = QUEUE_0;
> > > > > +       u8 load[12] = { 0 };
> > > > > +       int i = 0, j, ret;
> > > > > +       u32 val;
> > > > > +
> > > > > +       /*
> > > > > +        * Call validate queue to make sure queue is empty before starting.
> > > > > +        * This is to avoid overflow / underflow of queue.
> > > > > +        */
> > > > > +       ret = cci_validate_queue(cci, master, queue);
> > > > > +       if (ret < 0)
> > > > > +               return ret;
> > > > > +
> > > > > +       val = CCI_I2C_SET_PARAM | (addr & 0x7f) << 4;
> > > > > +       writel(val, cci->base + CCI_I2C_Mm_Qn_LOAD_DATA(master, queue));
> > > > > +
> > > > > +       load[i++] = CCI_I2C_WRITE | len << 4;
> > > >
> > > > Can 'len' really be 16 bits? Because this assignment truncates that very
> > > > quickly.
> > >
> > > Yes, this is passed from i2c_msg->len which is 16 bytes. But here it
> > > doesn't support so it is truncated here
> > >
> > > >
> > > > > +
> > > > > +       for (j = 0; j < len; j++)
> > > >
> > > > Well I guess len can be at most '11', so maybe the type should be u8
> > > > instead?
> > >
> > > I can use a local variable and cast to it, but am not sure that helps!
> >
> > Maybe just a comment indicating that max len is 11?
>
> Sure, will add
>
> > > > > +               return PTR_ERR(cci->base);
> > > > > +       }
> > > > > +
> > > > > +       /* Interrupt */
> > > > > +
> > > > > +       ret = platform_get_irq(pdev, 0);
> > > > > +       if (ret < 0) {
> > > > > +               dev_err(dev, "missing IRQ: %d\n", ret);
> > > > > +               return ret;
> > > > > +       }
> > > > > +       cci->irq = ret;
> > > > > +
> > > > > +       ret = devm_request_irq(dev, cci->irq, cci_isr,
> > > > > +                              IRQF_TRIGGER_RISING, dev_name(dev), cci);
> > > > > +       if (ret < 0) {
> > > > > +               dev_err(dev, "request_irq failed, ret: %d\n", ret);
> > > > > +               return ret;
> > > > > +       }
> > > > > +
> > > > > +       disable_irq(cci->irq);
> > > >
> > > > Why? Is the irq always triggering or something?
> > >
> > > I supposed Todor didn't want to enable IRQ until everything is set.
> > > I could move this block before adding i2c adapter.
> >
> > But does it actually matter? The interrupt is "enabled" from request to
> > this function call so it's still possible to trigger. I'd just remove
> > the irq disabling unless it actually matters.
>
> Agreed, will do that
>
> > > > > +static const struct cci_data cci_8916_data = {
> > > > > +       .num_masters = 1,
> > > > > +       .queue_size = { 64, 16 },
> > > > > +       .quirks = {
> > > > > +               .max_write_len = 10,
> > > > > +               .max_read_len = 12,
> > > > > +       },
> > > > > +       .res = {
> > > > > +               .clock = {
> > > > > +                       "camss_top_ahb",
> > > > > +                       "cci_ahb",
> > > > > +                       "camss_ahb",
> > > > > +                       "cci"
> > > >
> > > > I guess this is another design where you just want to "get all the clks"
> > > > and not care about what they are?
> > >
> > > Yes that is how this seems to be
> >
> > Ok. I'm going to merge the "get all the clks" API soon. Maybe you can
> > use that here.
>
> Sure, once merged I will send a patch for this driver, which I guess
> would be after next merge window..
>
> --
> ~Vinod



-- 
Ricardo Ribalda



[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux