Marc Dietrich wrote at Wednesday, August 17, 2011 1:01 PM:
> the patch below adds slave mode to tegra's i2c controller. I tried to make it as
> small as possible. It is needed to allow the nvec driver to deligate the
> initialization to the i2c-tegra driver as part of its cleanup. Patch is tested
> with our version of nvec. Patch is against arm-soc + boards-3.2. Please review
> and consider it for merge.
Forgive my lack of familiarity with the Tegra HW, but is the slave
controller completely autonomous? I'm curious what data slave transactions
access, since there's no actual code here to respond to transactions
directed at the slave address and provide data in response.
Or put another way, is this patch really adding slave mode support, or
is it just providing a way to program the slave controller's I2C address?
Some comments below.
> diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c
...
> -struct tegra_i2c_dev {
> - struct device *dev;
> - struct i2c_adapter adapter;
...
> -};
Nothing got added that needed that struct definition in a header file;
was there meant to be a new file included in the patch that implements
the slave driver (and makefile and perhaps Kconfig changes?)
> + int slave_addr = i2c_dev->is_slave ? 0xfc : i2c_dev->slave_addr;
Isn't that backwards; don't you want to hard-code the slave address as 0xfc only
when the controller isn't intended to be used in slave mode?
> i2c_dev->bus_clk_rate = 100000; /* default clock rate */
> if (pdata) {
> i2c_dev->bus_clk_rate = pdata->bus_clk_rate;
> + i2c_dev->is_slave = pdata->is_slave;
> + i2c_dev->slave_addr = pdata->slave_addr;
Maybe store a pointer to pdata itself rather than having to copy this
manually; it'll be one less thing to worry about when adding fields to
pdata.
...
> - ret = request_irq(i2c_dev->irq, tegra_i2c_isr, 0, pdev->name, i2c_dev);
> - if (ret) {
> - dev_err(&pdev->dev, "Failed to request irq %i\n", i2c_dev->irq);
> - goto err_free;
> + if (!i2c_dev->is_slave) {
> + ret = request_irq(i2c_dev->irq, tegra_i2c_isr, 0, pdev->name, i2c_dev);
> + if (ret) {
> + dev_err(&pdev->dev, "Failed to request irq %i\n", i2c_dev->irq);
> + goto err_free;
> + }
> }
This appears to make master and slave mode mutually exclusive. Doesn't
the HW allow use of both controllers on the same bus?
--
nvpublic
��.n��������+%������w��{.n�����{��-��)�� jg��� �����ݢj����G���� ���j:+v���w�m������w�������h�����٥
