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

[Vinod <vkoul@kernel.org>]

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