Re: [PATCH v3 1/2] i2c: Add driver for Cadence I2C controller

[SÃren Brinkmann]

On Tue, 2014-04-01 at 02:50AM +0200, Wolfram Sang wrote:
> On Tue, Mar 11, 2014 at 09:50:12AM -0700, Soren Brinkmann wrote:
> > Add a driver for the Cadence I2C controller. This controller is for
> > example found in Xilinx Zynq.
> > 
> > Signed-off-by: Soren Brinkmann <soren.brinkmann@xxxxxxxxxx>
> 
> ...
> 
> > +static irqreturn_t cdns_i2c_isr(int irq, void *ptr)
> > +{
> > +	unsigned int isr_status, avail_bytes;
> > +	unsigned int bytes_to_recv, bytes_to_send;
> > +	struct cdns_i2c *id = ptr;
> > +	/* Signal completion only after everything is updated */
> > +	int done_flag = 0;
> > +
> > +	isr_status = cdns_i2c_readreg(CDNS_I2C_ISR_OFFSET);
> > +
> > +	/* Handling nack and arbitration lost interrupt */
> > +	if (isr_status & (CDNS_I2C_IXR_NACK | CDNS_I2C_IXR_ARB_LOST))
> > +		done_flag = 1;
> > +
> > +	/* Handling Data interrupt */
> > +	if ((isr_status & CDNS_I2C_IXR_DATA) &&
> > +			(id->recv_count >= CDNS_I2C_DATA_INTR_DEPTH)) {
> > +		/* Always read data interrupt threshold bytes */
> > +		bytes_to_recv = CDNS_I2C_DATA_INTR_DEPTH;
> > +		id->recv_count = id->recv_count - CDNS_I2C_DATA_INTR_DEPTH;
> 
> Use '-='
> 
> ...
> 
> > +static int cdns_i2c_process_msg(struct cdns_i2c *id, struct i2c_msg *msg,
> > +		struct i2c_adapter *adap)
> > +{
> > +	int ret;
> > +	u32 reg;
> > +	bool retry = false;
> > +	unsigned retries = adap->retries;
> > +
> > +	id->p_msg = msg;
> > +	do {
> > +		id->err_status = 0;
> > +		init_completion(&id->xfer_done);
> 
> reinit_completion. And add init_completion in probe.
> 
> > +
> > +		/* Check for the TEN Bit mode on each msg */
> > +		reg = cdns_i2c_readreg(CDNS_I2C_CR_OFFSET);
> > +		if (msg->flags & I2C_M_TEN) {
> > +			if (reg & CDNS_I2C_CR_NEA)
> > +				cdns_i2c_writereg(reg & ~CDNS_I2C_CR_NEA,
> > +						CDNS_I2C_CR_OFFSET);
> > +		} else {
> > +			if (!(reg & CDNS_I2C_CR_NEA))
> > +				cdns_i2c_writereg(reg | CDNS_I2C_CR_NEA,
> > +						CDNS_I2C_CR_OFFSET);
> > +		}
> > +
> > +		/* Check for the R/W flag on each msg */
> > +		if (msg->flags & I2C_M_RD)
> > +			cdns_i2c_mrecv(id);
> > +		else
> > +			cdns_i2c_msend(id);
> > +
> > +		/* Wait for the signal of completion */
> > +		ret = wait_for_completion_timeout(&id->xfer_done, HZ);
> > +		if (!ret) {
> > +			cdns_i2c_master_reset(adap);
> > +			dev_err(id->adap.dev.parent,
> > +					"timeout waiting on completion\n");
> > +			return -ETIMEDOUT;
> > +		}
> > +
> > +		cdns_i2c_writereg(CDNS_I2C_IXR_ALL_INTR_MASK,
> > +				  CDNS_I2C_IDR_OFFSET);
> > +
> > +		/* If it is bus arbitration error, try again */
> > +		if (id->err_status & CDNS_I2C_IXR_ARB_LOST) {
> 
> Just return -EAGAIN here...
> 
> > +			dev_dbg(id->adap.dev.parent,
> > +				 "Lost ownership on bus, trying again\n");
> > +			if (retries--) {
> > +				mdelay(2);
> > +				retry = true;
> > +			} else {
> > +				dev_err(id->adap.dev.parent,
> > +					 "Retries completed, exit\n");
> > +				return -EREMOTEIO;
> > +			}
> > +		}
> 
> ... and you can skip this block since the core will do the retries.
> 
> > +	} while (retry);
> > +
> > +	return 0;
> > +}
> > +
> > +/**
> > + * cdns_i2c_master_xfer - The main i2c transfer function
> > + * @adap:	pointer to the i2c adapter driver instance
> > + * @msgs:	pointer to the i2c message structure
> > + * @num:	the number of messages to transfer
> > + *
> > + * Return: number of msgs processed on success, negative error otherwise
> > + *
> > + * This function waits for the bus idle condition and updates the timeout if
> > + * modified by user. Then initiates the send/recv activity based on the
> > + * transfer message received.
> > + */
> > +static int cdns_i2c_master_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs,
> > +				int num)
> > +{
> > +	struct cdns_i2c *id = adap->algo_data;
> > +	unsigned long timeout;
> > +	int ret, count;
> > +	u32 reg;
> > +
> > +	/* Waiting for bus-ready. If bus not ready, it returns after timeout */
> > +	timeout = jiffies + CDNS_I2C_TIMEOUT;
> > +	while (cdns_i2c_readreg(CDNS_I2C_SR_OFFSET) & CDNS_I2C_SR_BA) {
> > +		if (time_after(jiffies, timeout)) {
> > +			dev_warn(id->adap.dev.parent,
> > +					"timedout waiting for bus ready\n");
> > +			cdns_i2c_master_reset(adap);
> > +			return -ETIMEDOUT;
> > +		}
> > +		schedule_timeout(1);
> > +	}
> > +
> > +	/* The bus is free. Set the new timeout value if updated */
> > +	if (id->adap.timeout != id->cur_timeout) {
> > +		cdns_i2c_writereg(id->adap.timeout & CDNS_I2C_TIME_OUT_TO_MASK,
> > +					CDNS_I2C_TIME_OUT_OFFSET);
> > +		id->cur_timeout = id->adap.timeout;
> > +	}
> > +
> > +	/*
> > +	 * Set the flag to one when multiple messages are to be
> > +	 * processed with a repeated start.
> > +	 */
> > +	if (num > 1) {
> > +		id->bus_hold_flag = 1;
> > +		reg = cdns_i2c_readreg(CDNS_I2C_CR_OFFSET);
> > +		reg |= CDNS_I2C_CR_HOLD;
> > +		cdns_i2c_writereg(reg, CDNS_I2C_CR_OFFSET);
> > +	} else {
> > +		id->bus_hold_flag = 0;
> > +	}
> > +
> > +	/* Process the msg one by one */
> > +	for (count = 0; count < num; count++, msgs++) {
> > +		if (count == (num - 1))
> > +			id->bus_hold_flag = 0;
> > +
> > +		ret = cdns_i2c_process_msg(id, msgs, adap);
> > +		if (ret)
> > +			return ret;
> > +
> > +		/* Report the other error interrupts to application as EIO */
> > +		if (id->err_status & 0xE4) {
> 
> Please replace this magic hex value with something readable.
> 
> > +			cdns_i2c_master_reset(adap);
> > +			return -EIO;
> > +		}
> > +	}
> > +
> > +	return num;
> > +}
> > +
> 
> ...
> 
> > +/**
> > + * cdns_i2c_probe - Platform registration call
> > + * @pdev:	Handle to the platform device structure
> > + *
> > + * Return: 0 on success, negative error otherwise
> > + *
> > + * This function does all the memory allocation and registration for the i2c
> > + * device. User can modify the address mode to 10 bit address mode using the
> > + * ioctl call with option I2C_TENBIT.
> > + */
> > +static int cdns_i2c_probe(struct platform_device *pdev)
> > +{
> > +	struct resource *r_mem;
> > +	struct cdns_i2c *id;
> > +	int ret;
> > +
> > +	id = devm_kzalloc(&pdev->dev, sizeof(*id), GFP_KERNEL);
> > +	if (!id)
> > +		return -ENOMEM;
> > +
> > +	platform_set_drvdata(pdev, id);
> > +
> > +	r_mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > +	id->membase = devm_ioremap_resource(&pdev->dev, r_mem);
> > +	if (IS_ERR(id->membase))
> > +		return PTR_ERR(id->membase);
> > +
> > +	id->irq = platform_get_irq(pdev, 0);
> > +
> > +	id->adap.nr = pdev->id;
> 
> Drop this line and use i2c_add_adapter() later to let the core handle
> the numbering. This will also take aliases into account.
> 
> > +	id->adap.dev.of_node = pdev->dev.of_node;
> > +	id->adap.algo = &cdns_i2c_algo;
> > +	id->adap.timeout = 0x1F;	/* Default timeout value */
> 
> This value is in jiffies, so you probably want to use msecs_to_jiffies
> or alike.
> 
> > +	id->adap.retries = 3;		/* Default retry value. */
> > +	id->adap.algo_data = id;
> > +	id->adap.dev.parent = &pdev->dev;
> > +	snprintf(id->adap.name, sizeof(id->adap.name),
> > +		 "Cadence I2C at %08lx", (unsigned long)r_mem->start);
> > +
> > +	id->cur_timeout = id->adap.timeout;
> > +	id->clk = devm_clk_get(&pdev->dev, NULL);
> > +	if (IS_ERR(id->clk)) {
> > +		dev_err(&pdev->dev, "input clock not found.\n");
> > +		return PTR_ERR(id->clk);
> > +	}
> > +	ret = clk_prepare_enable(id->clk);
> > +	if (ret) {
> > +		dev_err(&pdev->dev, "Unable to enable clock.\n");
> > +		return ret;
> > +	}
> > +	id->clk_rate_change_nb.notifier_call = cdns_i2c_clk_notifier_cb;
> > +	if (clk_notifier_register(id->clk, &id->clk_rate_change_nb))
> > +		dev_warn(&pdev->dev, "Unable to register clock notifier.\n");
> > +	id->input_clk = clk_get_rate(id->clk);
> > +
> > +	ret = of_property_read_u32(pdev->dev.of_node, "clock-frequency",
> > +			&id->i2c_clk);
> > +	if (ret || (id->i2c_clk > CDNS_I2C_SPEED_MAX))
> > +		id->i2c_clk = CDNS_I2C_SPEED_MAX;
> > +
> > +	cdns_i2c_writereg(0xE, CDNS_I2C_CR_OFFSET);
> 
> Remove magic value. Please check the driver for more, in case I missed
> some.

Thanks for reviewing. I'll work on the next iteration.

	SÃren


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/