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
