Re: [PATCH v5 1/2] i2c: Add Imagination Technologies I2C SCB driver

[Ezequiel Garcia <ezequiel.garcia-1AXoQHu6uovQT0dZR+AlfA@public.gmane.org>]

On 11/10/2014 05:53 PM, Andrew Bresticker wrote:
> Hi Ezequiel,
> 
> On Mon, Nov 10, 2014 at 11:30 AM, Ezequiel Garcia
> <ezequiel.garcia-1AXoQHu6uovQT0dZR+AlfA@public.gmane.org> wrote:
>> From: James Hogan <james.hogan-1AXoQHu6uovQT0dZR+AlfA@public.gmane.org>
>>
>> Add support for the IMG I2C Serial Control Bus (SCB) found on the
>> Pistachio and TZ1090 SoCs.
>>
>> Signed-off-by: James Hogan <james.hogan-1AXoQHu6uovQT0dZR+AlfA@public.gmane.org>
>> [Ezequiel: code cleaning and rebasing]
>> Signed-off-by: Ezequiel Garcia <ezequiel.garcia-1AXoQHu6uovQT0dZR+AlfA@public.gmane.org>
> 
> A few minor comments below, otherwise:
> 
> Reviewed-by: Andrew Bresticker <abrestic-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
> 
>> diff --git a/drivers/i2c/busses/i2c-img-scb.c b/drivers/i2c/busses/i2c-img-scb.c
[..]
>> +
>> +               wait_for_completion_timeout(&i2c->msg_complete, IMG_I2C_TOUT);
> 
> ... and here?
> 

Right, missed these two.

>> +               del_timer_sync(&i2c->check_timer);
>> +               if (i2c->msg_status)
>> +                       break;
>> +       }
>> +
>> +       clk_disable_unprepare(i2c->scb_clk);
>> +
>> +       return i2c->msg_status ? i2c->msg_status : num;
>> +}
> 
>> +static int img_i2c_init(struct img_i2c *i2c)
>> +{
>> +       unsigned int clk_khz, bitrate_khz, clk_period, tckh, tckl, tsdh;
>> +       unsigned int i, ret, data, prescale, inc, int_bitrate;
>> +       unsigned int filt, filt_disable, filt_bypass;
>> +       struct img_i2c_timings timing;
>> +       u32 rev;
>> +
>> +       ret = clk_prepare_enable(i2c->scb_clk);
>> +       if (ret)
>> +               return ret;
>> +
>> +       rev = img_i2c_readl(i2c, SCB_CORE_REV_REG);
>> +       if ((rev & 0x00ffffff) < 0x00020200) {
>> +               dev_info(i2c->adap.dev.parent,
>> +                        "Unknown hardware revision (%d.%d.%d.%d)\n",
>> +                        (rev >> 24) & 0xff, (rev >> 16) & 0xff,
>> +                        (rev >> 8) & 0xff, rev & 0xff);
>> +               clk_disable_unprepare(i2c->scb_clk);
>> +               return -EINVAL;
>> +       }
>> +
>> +       if (rev == REL_SOC_IP_SCB_2_2_1)
>> +               i2c->need_wr_rd_fence = true;
>> +
>> +       bitrate_khz = i2c->bitrate / 1000;
>> +       clk_khz = clk_get_rate(i2c->scb_clk) / 1000;
>> +
>> +       /* Determine what mode we're in from the bitrate */
>> +       timing = timings[0];
>> +       for (i = 0; i < ARRAY_SIZE(timings); i++) {
>> +               if (i2c->bitrate <= timings[i].max_bitrate) {
>> +                       timing = timings[i];
>> +                       break;
>> +               }
>> +       }
>> +
>> +       /* Find the prescale that would give us that inc (approx delay = 0) */
>> +       prescale = SCB_OPT_INC * clk_khz / (256 * 16 * bitrate_khz);
>> +       prescale = clamp_t(unsigned int, prescale, 1, 8);
>> +       clk_khz /= prescale;
>> +
>> +       /* Setup the clock increment value */
>> +       inc = ((256 * 16 * bitrate_khz) /
>> +              (clk_khz - (16 * bitrate_khz * (clk_khz / 1000) *
>> +                          i2c->busdelay) / 10000));
>> +
>> +       /* Setup the filter clock value */
>> +       filt_bypass = 0;
>> +       filt_disable = 0;
>> +       filt = 0;
>> +       if (clk_khz < 20000) {
>> +               filt_disable = SCB_FILT_DISABLE;
>> +       } else if (clk_khz < 40000) {
>> +               filt_bypass = SCB_FILT_BYPASS;
>> +       } else {
>> +               /* Calculate filter clock */
>> +               filt = ((640000) / ((clk_khz / 1000) * (250 - i2c->busdelay)));
>> +               if ((640000) % ((clk_khz / 1000) * (250 - i2c->busdelay))) {
>> +                       /* Scale up */
>> +                       inc++;
>> +               }
>> +               if (filt > SCB_FILT_INC_MASK)
>> +                       filt = SCB_FILT_INC_MASK;
>> +       }
>> +       data = filt_disable | filt_bypass |
>> +              ((filt & SCB_FILT_INC_MASK) << SCB_FILT_INC_SHIFT) |
>> +              ((inc & SCB_INC_MASK) << SCB_INC_SHIFT) |
>> +              (prescale - 1);
>> +       img_i2c_writel(i2c, SCB_CLK_SET_REG, data);
> 
> Do filt_bypass/filt_disable really need to be separate variables?
> Can't we just use filt in all three cases?
> 

Hm... how about this?

        /* Setup the filter clock value */
        if (clk_khz < 20000) {
                filt = SCB_FILT_DISABLE;
        } else if (clk_khz < 40000) {
                filt = SCB_FILT_BYPASS;
        } else {
                /* Calculate filter clock */
                filt = ((640000) / ((clk_khz / 1000) * (250 - i2c->busdelay)));
                if ((640000) % ((clk_khz / 1000) * (250 - i2c->busdelay))) {
                        /* Scale up */
                        inc++;
                }
                if (filt > SCB_FILT_INC_MASK)
                        filt = SCB_FILT_INC_MASK;

                filt = ((filt & SCB_FILT_INC_MASK) << SCB_FILT_INC_SHIFT) |
                       ((inc & SCB_INC_MASK) << SCB_INC_SHIFT) |
                       (prescale - 1);
        }
        img_i2c_writel(i2c, SCB_CLK_SET_REG, filt);


>> +#ifdef CONFIG_PM_SLEEP
>> +static int img_i2c_suspend(struct device *dev)
>> +{
>> +       struct img_i2c *i2c = dev_get_drvdata(dev);
>> +
>> +       img_i2c_switch_mode(i2c, MODE_SUSPEND);
>> +
>> +       clk_disable(i2c->sys_clk);
> 
> Why not unprepare as well?
> 

Hm.. seems I got confused here, and thought we couldn't sleep.

Thanks for the review!
-- 
Ezequiel