| Message ID | 1371024438-16631-3-git-send-email-maxime.ripard@free-electrons.com |
|---|---|
| State | Superseded |
| Headers | show |
On Wed, Jun 12, 2013 at 10:07:11AM +0200, Maxime Ripard wrote: > The Allwinner i2c controller uses the same logic as the Marvell one, but > with slightly different register offsets. > > Introduce a structure that will be passed by either the pdata or > associated to the compatible strings, and that holds the various > registers that might be needed. Hi Maxime I don't think this change is bisectable. It assumes the platform data passes the registers, which is not true until the next patch. Please can you re-arrange the order. Add the extra information to the platform data, and then make use of it, not the other way around. Thanks Andrew > > Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com> > --- > drivers/i2c/busses/i2c-mv64xxx.c | 81 +++++++++++++++++++++------------------- > include/linux/mv643xx_i2c.h | 27 ++++++++++++-- > 2 files changed, 66 insertions(+), 42 deletions(-) > > diff --git a/drivers/i2c/busses/i2c-mv64xxx.c b/drivers/i2c/busses/i2c-mv64xxx.c > index d70a2fda..f9e076e 100644 > --- a/drivers/i2c/busses/i2c-mv64xxx.c > +++ b/drivers/i2c/busses/i2c-mv64xxx.c > @@ -19,20 +19,12 @@ > #include <linux/platform_device.h> > #include <linux/io.h> > #include <linux/of.h> > +#include <linux/of_device.h> > #include <linux/of_irq.h> > #include <linux/of_i2c.h> > #include <linux/clk.h> > #include <linux/err.h> > > -/* Register defines */ > -#define MV64XXX_I2C_REG_SLAVE_ADDR 0x00 > -#define MV64XXX_I2C_REG_DATA 0x04 > -#define MV64XXX_I2C_REG_CONTROL 0x08 > -#define MV64XXX_I2C_REG_STATUS 0x0c > -#define MV64XXX_I2C_REG_BAUD 0x0c > -#define MV64XXX_I2C_REG_EXT_SLAVE_ADDR 0x10 > -#define MV64XXX_I2C_REG_SOFT_RESET 0x1c > - > #define MV64XXX_I2C_ADDR_ADDR(val) ((val & 0x7f) << 1) > #define MV64XXX_I2C_BAUD_DIV_N(val) (val & 0x7) > #define MV64XXX_I2C_BAUD_DIV_M(val) ((val & 0xf) << 3) > @@ -98,6 +90,7 @@ struct mv64xxx_i2c_data { > u32 aborting; > u32 cntl_bits; > void __iomem *reg_base; > + struct mv64xxx_i2c_regs *regs; > u32 addr1; > u32 addr2; > u32 bytes_left; > @@ -154,13 +147,13 @@ mv64xxx_i2c_prepare_for_io(struct mv64xxx_i2c_data *drv_data, > static void > mv64xxx_i2c_hw_init(struct mv64xxx_i2c_data *drv_data) > { > - writel(0, drv_data->reg_base + MV64XXX_I2C_REG_SOFT_RESET); > + writel(0, drv_data->reg_base + drv_data->regs->soft_reset); > writel(MV64XXX_I2C_BAUD_DIV_M(drv_data->freq_m) | MV64XXX_I2C_BAUD_DIV_N(drv_data->freq_n), > - drv_data->reg_base + MV64XXX_I2C_REG_BAUD); > - writel(0, drv_data->reg_base + MV64XXX_I2C_REG_SLAVE_ADDR); > - writel(0, drv_data->reg_base + MV64XXX_I2C_REG_EXT_SLAVE_ADDR); > + drv_data->reg_base + drv_data->regs->clock); > + writel(0, drv_data->reg_base + drv_data->regs->addr); > + writel(0, drv_data->reg_base + drv_data->regs->ext_addr); > writel(MV64XXX_I2C_REG_CONTROL_TWSIEN | MV64XXX_I2C_REG_CONTROL_STOP, > - drv_data->reg_base + MV64XXX_I2C_REG_CONTROL); > + drv_data->reg_base + drv_data->regs->control); > drv_data->state = MV64XXX_I2C_STATE_IDLE; > } > > @@ -282,7 +275,7 @@ mv64xxx_i2c_do_action(struct mv64xxx_i2c_data *drv_data) > > drv_data->cntl_bits |= MV64XXX_I2C_REG_CONTROL_START; > writel(drv_data->cntl_bits, > - drv_data->reg_base + MV64XXX_I2C_REG_CONTROL); > + drv_data->reg_base + drv_data->regs->control); > > drv_data->msgs++; > drv_data->num_msgs--; > @@ -300,48 +293,48 @@ mv64xxx_i2c_do_action(struct mv64xxx_i2c_data *drv_data) > > case MV64XXX_I2C_ACTION_CONTINUE: > writel(drv_data->cntl_bits, > - drv_data->reg_base + MV64XXX_I2C_REG_CONTROL); > + drv_data->reg_base + drv_data->regs->control); > break; > > case MV64XXX_I2C_ACTION_SEND_START: > writel(drv_data->cntl_bits | MV64XXX_I2C_REG_CONTROL_START, > - drv_data->reg_base + MV64XXX_I2C_REG_CONTROL); > + drv_data->reg_base + drv_data->regs->control); > break; > > case MV64XXX_I2C_ACTION_SEND_ADDR_1: > writel(drv_data->addr1, > - drv_data->reg_base + MV64XXX_I2C_REG_DATA); > + drv_data->reg_base + drv_data->regs->data); > writel(drv_data->cntl_bits, > - drv_data->reg_base + MV64XXX_I2C_REG_CONTROL); > + drv_data->reg_base + drv_data->regs->control); > break; > > case MV64XXX_I2C_ACTION_SEND_ADDR_2: > writel(drv_data->addr2, > - drv_data->reg_base + MV64XXX_I2C_REG_DATA); > + drv_data->reg_base + drv_data->regs->data); > writel(drv_data->cntl_bits, > - drv_data->reg_base + MV64XXX_I2C_REG_CONTROL); > + drv_data->reg_base + drv_data->regs->control); > break; > > case MV64XXX_I2C_ACTION_SEND_DATA: > writel(drv_data->msg->buf[drv_data->byte_posn++], > - drv_data->reg_base + MV64XXX_I2C_REG_DATA); > + drv_data->reg_base + drv_data->regs->data); > writel(drv_data->cntl_bits, > - drv_data->reg_base + MV64XXX_I2C_REG_CONTROL); > + drv_data->reg_base + drv_data->regs->control); > break; > > case MV64XXX_I2C_ACTION_RCV_DATA: > drv_data->msg->buf[drv_data->byte_posn++] = > - readl(drv_data->reg_base + MV64XXX_I2C_REG_DATA); > + readl(drv_data->reg_base + drv_data->regs->data); > writel(drv_data->cntl_bits, > - drv_data->reg_base + MV64XXX_I2C_REG_CONTROL); > + drv_data->reg_base + drv_data->regs->control); > break; > > case MV64XXX_I2C_ACTION_RCV_DATA_STOP: > drv_data->msg->buf[drv_data->byte_posn++] = > - readl(drv_data->reg_base + MV64XXX_I2C_REG_DATA); > + readl(drv_data->reg_base + drv_data->regs->data); > drv_data->cntl_bits &= ~MV64XXX_I2C_REG_CONTROL_INTEN; > writel(drv_data->cntl_bits | MV64XXX_I2C_REG_CONTROL_STOP, > - drv_data->reg_base + MV64XXX_I2C_REG_CONTROL); > + drv_data->reg_base + drv_data->regs->control); > drv_data->block = 0; > wake_up(&drv_data->waitq); > break; > @@ -356,7 +349,7 @@ mv64xxx_i2c_do_action(struct mv64xxx_i2c_data *drv_data) > case MV64XXX_I2C_ACTION_SEND_STOP: > drv_data->cntl_bits &= ~MV64XXX_I2C_REG_CONTROL_INTEN; > writel(drv_data->cntl_bits | MV64XXX_I2C_REG_CONTROL_STOP, > - drv_data->reg_base + MV64XXX_I2C_REG_CONTROL); > + drv_data->reg_base + drv_data->regs->control); > drv_data->block = 0; > wake_up(&drv_data->waitq); > break; > @@ -372,9 +365,9 @@ mv64xxx_i2c_intr(int irq, void *dev_id) > irqreturn_t rc = IRQ_NONE; > > spin_lock_irqsave(&drv_data->lock, flags); > - while (readl(drv_data->reg_base + MV64XXX_I2C_REG_CONTROL) & > + while (readl(drv_data->reg_base + drv_data->regs->control) & > MV64XXX_I2C_REG_CONTROL_IFLG) { > - status = readl(drv_data->reg_base + MV64XXX_I2C_REG_STATUS); > + status = readl(drv_data->reg_base + drv_data->regs->status); > mv64xxx_i2c_fsm(drv_data, status); > mv64xxx_i2c_do_action(drv_data); > rc = IRQ_HANDLED; > @@ -496,6 +489,12 @@ static const struct i2c_algorithm mv64xxx_i2c_algo = { > ***************************************************************************** > */ > #ifdef CONFIG_OF > +static const struct of_device_id mv64xxx_i2c_of_match_table[] = { > + { .compatible = "marvell,mv64xxx-i2c", .data = &mv64xxx_i2c_regs_mv64xxx}, > + {} > +}; > +MODULE_DEVICE_TABLE(of, mv64xxx_i2c_of_match_table); > + > static int > mv64xxx_calc_freq(const int tclk, const int n, const int m) > { > @@ -528,8 +527,10 @@ mv64xxx_find_baud_factors(const u32 req_freq, const u32 tclk, u32 *best_n, > > static int > mv64xxx_of_config(struct mv64xxx_i2c_data *drv_data, > - struct device_node *np) > + struct device *dev) > { > + const struct of_device_id *device; > + struct device_node *np = dev->of_node; > u32 bus_freq, tclk; > int rc = 0; > > @@ -558,6 +559,13 @@ mv64xxx_of_config(struct mv64xxx_i2c_data *drv_data, > * So hard code the value to 1 second. > */ > drv_data->adapter.timeout = HZ; > + > + device = of_match_device(mv64xxx_i2c_of_match_table, dev); > + if (!device) > + return -ENODEV; > + > + drv_data->regs = (struct mv64xxx_i2c_regs *)device->data; > + > out: > return rc; > #endif > @@ -565,7 +573,7 @@ out: > #else /* CONFIG_OF */ > static int > mv64xxx_of_config(struct mv64xxx_i2c_data *drv_data, > - struct device_node *np) > + struct device *dev) > { > return -ENODEV; > } > @@ -611,8 +619,9 @@ mv64xxx_i2c_probe(struct platform_device *pd) > drv_data->freq_n = pdata->freq_n; > drv_data->irq = platform_get_irq(pd, 0); > drv_data->adapter.timeout = msecs_to_jiffies(pdata->timeout); > + drv_data->regs = pdata->regs; > } else if (pd->dev.of_node) { > - rc = mv64xxx_of_config(drv_data, pd->dev.of_node); > + rc = mv64xxx_of_config(drv_data, &pd->dev); > if (rc) > goto exit_clk; > } > @@ -680,12 +689,6 @@ mv64xxx_i2c_remove(struct platform_device *dev) > return 0; > } > > -static const struct of_device_id mv64xxx_i2c_of_match_table[] = { > - { .compatible = "marvell,mv64xxx-i2c", }, > - {} > -}; > -MODULE_DEVICE_TABLE(of, mv64xxx_i2c_of_match_table); > - > static struct platform_driver mv64xxx_i2c_driver = { > .probe = mv64xxx_i2c_probe, > .remove = mv64xxx_i2c_remove, > diff --git a/include/linux/mv643xx_i2c.h b/include/linux/mv643xx_i2c.h > index 5db5152..9304c94 100644 > --- a/include/linux/mv643xx_i2c.h > +++ b/include/linux/mv643xx_i2c.h > @@ -12,11 +12,32 @@ > > #define MV64XXX_I2C_CTLR_NAME "mv64xxx_i2c" > > +struct mv64xxx_i2c_regs { > + u8 addr; > + u8 ext_addr; > + u8 data; > + u8 control; > + u8 status; > + u8 clock; > + u8 soft_reset; > +}; > + > +struct mv64xxx_i2c_regs mv64xxx_i2c_regs_mv64xxx = { > + .addr = 0x00, > + .ext_addr = 0x10, > + .data = 0x04, > + .control = 0x08, > + .status = 0x0c, > + .clock = 0x0c, > + .soft_reset = 0x1c, > +}; > + > /* i2c Platform Device, Driver Data */ > struct mv64xxx_i2c_pdata { > - u32 freq_m; > - u32 freq_n; > - u32 timeout; /* In milliseconds */ > + u32 freq_m; > + u32 freq_n; > + struct mv64xxx_i2c_regs *regs; > + u32 timeout; /* In milliseconds */ > }; > > #endif /*_MV64XXX_I2C_H_*/ > -- > 1.8.3 > -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Andrew, On Wed, Jun 12, 2013 at 12:54:31PM +0200, Andrew Lunn wrote: > On Wed, Jun 12, 2013 at 10:07:11AM +0200, Maxime Ripard wrote: > > The Allwinner i2c controller uses the same logic as the Marvell one, but > > with slightly different register offsets. > > > > Introduce a structure that will be passed by either the pdata or > > associated to the compatible strings, and that holds the various > > registers that might be needed. > > Hi Maxime > > I don't think this change is bisectable. It assumes the platform data > passes the registers, which is not true until the next patch. > > Please can you re-arrange the order. Add the extra information to the > platform data, and then make use of it, not the other way around. Thanks to Tomasz comments, the next patch will be removed, and the whole regs passing in the pdata as well, so it will become bisectable. Thanks, Maxime
On Wed, Jun 12, 2013 at 10:07:11AM +0200, Maxime Ripard wrote: > The Allwinner i2c controller uses the same logic as the Marvell one, but > with slightly different register offsets. > > Introduce a structure that will be passed by either the pdata or > associated to the compatible strings, and that holds the various > registers that might be needed. I don't like this change. It introduces further indirection where it's not really necessary, and it's also using platform data to specify this which is in the opposite direction to what's required for moving towards DT. > @@ -154,13 +147,13 @@ mv64xxx_i2c_prepare_for_io(struct mv64xxx_i2c_data *drv_data, > static void > mv64xxx_i2c_hw_init(struct mv64xxx_i2c_data *drv_data) > { > - writel(0, drv_data->reg_base + MV64XXX_I2C_REG_SOFT_RESET); > + writel(0, drv_data->reg_base + drv_data->regs->soft_reset); It'd be much better to copy the offsets themselves in drv_data. You're only talking about 7 bytes here, so there's no worry about bloating the drv_data structure. > @@ -611,8 +619,9 @@ mv64xxx_i2c_probe(struct platform_device *pd) > drv_data->freq_n = pdata->freq_n; > drv_data->irq = platform_get_irq(pd, 0); > drv_data->adapter.timeout = msecs_to_jiffies(pdata->timeout); > + drv_data->regs = pdata->regs; > } else if (pd->dev.of_node) { > - rc = mv64xxx_of_config(drv_data, pd->dev.of_node); > + rc = mv64xxx_of_config(drv_data, &pd->dev); I'd suggest making the default register offsets be the drivers existing offsets, and allowing it to be overriden. That nicely sorts out the next comment below, and also gets rid of it in platform data. Moreover, if you're going to re-use this driver, you should do it via a different "compatible" name in DT, which the driver can then use to identify the different register set layout. > +struct mv64xxx_i2c_regs mv64xxx_i2c_regs_mv64xxx = { > + .addr = 0x00, > + .ext_addr = 0x10, > + .data = 0x04, > + .control = 0x08, > + .status = 0x0c, > + .clock = 0x0c, > + .soft_reset = 0x1c, > +}; No, this means every file which includes this header ends up defining this structure, which is globally visiable, and therefore its a recipe for link failures. -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Russel, On Wed, Jun 12, 2013 at 02:57:35PM +0100, Russell King - ARM Linux wrote: > On Wed, Jun 12, 2013 at 10:07:11AM +0200, Maxime Ripard wrote: > > The Allwinner i2c controller uses the same logic as the Marvell one, but > > with slightly different register offsets. > > > > Introduce a structure that will be passed by either the pdata or > > associated to the compatible strings, and that holds the various > > registers that might be needed. > > I don't like this change. It introduces further indirection where it's > not really necessary, and it's also using platform data to specify this > which is in the opposite direction to what's required for moving towards > DT. Well, some users of this aren't converted to DT, hence why I made the changes to the platform_data. > > @@ -154,13 +147,13 @@ mv64xxx_i2c_prepare_for_io(struct mv64xxx_i2c_data *drv_data, > > static void > > mv64xxx_i2c_hw_init(struct mv64xxx_i2c_data *drv_data) > > { > > - writel(0, drv_data->reg_base + MV64XXX_I2C_REG_SOFT_RESET); > > + writel(0, drv_data->reg_base + drv_data->regs->soft_reset); > > It'd be much better to copy the offsets themselves in drv_data. You're > only talking about 7 bytes here, so there's no worry about bloating the > drv_data structure. It was more about keeping things separated. Moreover, the probe function gets smaller, since you have only a pointer to pass on, instead of assigning those 7 bytes. And since Gregory Clement is working on adding more registers, I believe it makes more sense to have things separate. > > > @@ -611,8 +619,9 @@ mv64xxx_i2c_probe(struct platform_device *pd) > > drv_data->freq_n = pdata->freq_n; > > drv_data->irq = platform_get_irq(pd, 0); > > drv_data->adapter.timeout = msecs_to_jiffies(pdata->timeout); > > + drv_data->regs = pdata->regs; > > } else if (pd->dev.of_node) { > > - rc = mv64xxx_of_config(drv_data, pd->dev.of_node); > > + rc = mv64xxx_of_config(drv_data, &pd->dev); > > I'd suggest making the default register offsets be the drivers existing > offsets, and allowing it to be overriden. That nicely sorts out the > next comment below, and also gets rid of it in platform data. Moreover, > if you're going to re-use this driver, you should do it via a different > "compatible" name in DT, which the driver can then use to identify the > different register set layout. The logic here will change quite a bit in the next iteration thanks to the comments I received. I'm now using a platform_device_id structure to match the name of the driver just like what was done with the DT in that patchset. This also removes the need to add the regs field to the platform data and ... > > > +struct mv64xxx_i2c_regs mv64xxx_i2c_regs_mv64xxx = { > > + .addr = 0x00, > > + .ext_addr = 0x10, > > + .data = 0x04, > > + .control = 0x08, > > + .status = 0x0c, > > + .clock = 0x0c, > > + .soft_reset = 0x1c, > > +}; > > No, this means every file which includes this header ends up defining > this structure, which is globally visiable, and therefore its a recipe > for link failures. solves this as well. Maxime
On Wed, Jun 12, 2013 at 04:44:47PM +0200, Maxime Ripard wrote: > Hi Russel, > > On Wed, Jun 12, 2013 at 02:57:35PM +0100, Russell King - ARM Linux wrote: > > It'd be much better to copy the offsets themselves in drv_data. You're > > only talking about 7 bytes here, so there's no worry about bloating the > > drv_data structure. > > It was more about keeping things separated. Moreover, the probe > function gets smaller, since you have only a pointer to pass on, instead > of assigning those 7 bytes. struct driver_data { struct mv64xxx_i2c_regs reg_offsets; }; struct driver_data *drv_data; memcpy(drv_data->reg_offsets, reg_offsets, sizeof(drv_data->reg_offsets)); No need to write it each member as a separate assignment. -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 06/12/13 16:44, Maxime Ripard wrote: > Hi Russel, > > On Wed, Jun 12, 2013 at 02:57:35PM +0100, Russell King - ARM Linux wrote: >> On Wed, Jun 12, 2013 at 10:07:11AM +0200, Maxime Ripard wrote: >>> The Allwinner i2c controller uses the same logic as the Marvell one, but >>> with slightly different register offsets. >>> >>> Introduce a structure that will be passed by either the pdata or >>> associated to the compatible strings, and that holds the various >>> registers that might be needed. >> >> I don't like this change. It introduces further indirection where it's >> not really necessary, and it's also using platform data to specify this >> which is in the opposite direction to what's required for moving towards >> DT. > > Well, some users of this aren't converted to DT, hence why I made the > changes to the platform_data. Actually, this is not quite true. Yes of course, there are still users of non-DT Marvell SoCs and it is still in the progress of full-DT. But also ppc is using DT, except that they parse it and put in in platform_data. Reasonable since back then, there was no global DT API available. IMHO for the time in between (i.e. now) check for pdev->dev.of_node and !pdev->dev.platform_data will allow you to distinguish all users perfectly: - non-DT has platform_data set only - ppc DT has of_node and platform_data set - pure DT has of_node set only This will allow you to limit your register offset modifications to Allwinner exclusively and for pure DT (if that is what you want for Allwinner). Checkout mv643xx_eth in net-next where the above discrimination strategy was chosen. [...] >> I'd suggest making the default register offsets be the drivers existing >> offsets, and allowing it to be overriden. That nicely sorts out the >> next comment below, and also gets rid of it in platform data. Moreover, >> if you're going to re-use this driver, you should do it via a different >> "compatible" name in DT, which the driver can then use to identify the >> different register set layout. > > The logic here will change quite a bit in the next iteration thanks to > the comments I received. > > I'm now using a platform_device_id structure to match the name of the > driver just like what was done with the DT in that patchset. This also > removes the need to add the regs field to the platform data and ... Also here, if Allwinner is pure DT, you can call some mv643xx_i2c_of_probe() for pure DT only with the above discrimination. Sebastian -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Jun 12, 2013 at 03:51:39PM +0100, Russell King - ARM Linux wrote: > On Wed, Jun 12, 2013 at 04:44:47PM +0200, Maxime Ripard wrote: > > Hi Russel, > > > > On Wed, Jun 12, 2013 at 02:57:35PM +0100, Russell King - ARM Linux wrote: > > > It'd be much better to copy the offsets themselves in drv_data. You're > > > only talking about 7 bytes here, so there's no worry about bloating the > > > drv_data structure. > > > > It was more about keeping things separated. Moreover, the probe > > function gets smaller, since you have only a pointer to pass on, instead > > of assigning those 7 bytes. > > struct driver_data { > struct mv64xxx_i2c_regs reg_offsets; > }; > > struct driver_data *drv_data; > > memcpy(drv_data->reg_offsets, reg_offsets, sizeof(drv_data->reg_offsets)); > > No need to write it each member as a separate assignment. Ah right. I previously understood that you wanted a single variable for each register in the driver data structure. I'll do like you suggested. Maxime
Hi Sebastian, On Wed, Jun 12, 2013 at 05:03:12PM +0200, Sebastian Hesselbarth wrote: > On 06/12/13 16:44, Maxime Ripard wrote: > >Hi Russel, > > > >On Wed, Jun 12, 2013 at 02:57:35PM +0100, Russell King - ARM Linux wrote: > >>On Wed, Jun 12, 2013 at 10:07:11AM +0200, Maxime Ripard wrote: > >>>The Allwinner i2c controller uses the same logic as the Marvell one, but > >>>with slightly different register offsets. > >>> > >>>Introduce a structure that will be passed by either the pdata or > >>>associated to the compatible strings, and that holds the various > >>>registers that might be needed. > >> > >>I don't like this change. It introduces further indirection where it's > >>not really necessary, and it's also using platform data to specify this > >>which is in the opposite direction to what's required for moving towards > >>DT. > > > >Well, some users of this aren't converted to DT, hence why I made the > >changes to the platform_data. > > Actually, this is not quite true. Yes of course, there are still users > of non-DT Marvell SoCs and it is still in the progress of full-DT. But > also ppc is using DT, except that they parse it and put in in > platform_data. Reasonable since back then, there was no global DT API > available. Ah, I see, thanks for the insight. I was here referring more specifically to Orion that seems to be still stuck with !DT at the moment, at least partially. > IMHO for the time in between (i.e. now) check for pdev->dev.of_node > and !pdev->dev.platform_data will allow you to distinguish all users > perfectly: > > - non-DT has platform_data set only > - ppc DT has of_node and platform_data set > - pure DT has of_node set only > > This will allow you to limit your register offset modifications to > Allwinner exclusively and for pure DT (if that is what you want for > Allwinner). > > Checkout mv643xx_eth in net-next where the above discrimination > strategy was chosen. > > [...] > >>I'd suggest making the default register offsets be the drivers existing > >>offsets, and allowing it to be overriden. That nicely sorts out the > >>next comment below, and also gets rid of it in platform data. Moreover, > >>if you're going to re-use this driver, you should do it via a different > >>"compatible" name in DT, which the driver can then use to identify the > >>different register set layout. > > > >The logic here will change quite a bit in the next iteration thanks to > >the comments I received. > > > >I'm now using a platform_device_id structure to match the name of the > >driver just like what was done with the DT in that patchset. This also > >removes the need to add the regs field to the platform data and ... > > Also here, if Allwinner is pure DT, you can call some > mv643xx_i2c_of_probe() for pure DT only with the above discrimination. Unless I'm missing something, isn't it what's already in place here? We have: if (pdata) { /* Fill in the driver data structure from pdata */ } else if (pd->dev.of_node) { /* Fill in the driver data structure from dt */ } else { return -EFAIL; } I guess that should cover all the cases you mentionned, even the PPC one, right? Now, the question about what content do we find in these platform_data is actually a different one. This patch passed the regs offset as a member of those. We all agreed that it was not the most elegant solution (and like you mentionned, I will never use this pdata structure anyway for the Allwinner stuff). I guess we could just take the marvell offsets when using pdata, and use different register offsets based on the compatibles when loading from dt?
diff --git a/drivers/i2c/busses/i2c-mv64xxx.c b/drivers/i2c/busses/i2c-mv64xxx.c index d70a2fda..f9e076e 100644 --- a/drivers/i2c/busses/i2c-mv64xxx.c +++ b/drivers/i2c/busses/i2c-mv64xxx.c @@ -19,20 +19,12 @@ #include <linux/platform_device.h> #include <linux/io.h> #include <linux/of.h> +#include <linux/of_device.h> #include <linux/of_irq.h> #include <linux/of_i2c.h> #include <linux/clk.h> #include <linux/err.h> -/* Register defines */ -#define MV64XXX_I2C_REG_SLAVE_ADDR 0x00 -#define MV64XXX_I2C_REG_DATA 0x04 -#define MV64XXX_I2C_REG_CONTROL 0x08 -#define MV64XXX_I2C_REG_STATUS 0x0c -#define MV64XXX_I2C_REG_BAUD 0x0c -#define MV64XXX_I2C_REG_EXT_SLAVE_ADDR 0x10 -#define MV64XXX_I2C_REG_SOFT_RESET 0x1c - #define MV64XXX_I2C_ADDR_ADDR(val) ((val & 0x7f) << 1) #define MV64XXX_I2C_BAUD_DIV_N(val) (val & 0x7) #define MV64XXX_I2C_BAUD_DIV_M(val) ((val & 0xf) << 3) @@ -98,6 +90,7 @@ struct mv64xxx_i2c_data { u32 aborting; u32 cntl_bits; void __iomem *reg_base; + struct mv64xxx_i2c_regs *regs; u32 addr1; u32 addr2; u32 bytes_left; @@ -154,13 +147,13 @@ mv64xxx_i2c_prepare_for_io(struct mv64xxx_i2c_data *drv_data, static void mv64xxx_i2c_hw_init(struct mv64xxx_i2c_data *drv_data) { - writel(0, drv_data->reg_base + MV64XXX_I2C_REG_SOFT_RESET); + writel(0, drv_data->reg_base + drv_data->regs->soft_reset); writel(MV64XXX_I2C_BAUD_DIV_M(drv_data->freq_m) | MV64XXX_I2C_BAUD_DIV_N(drv_data->freq_n), - drv_data->reg_base + MV64XXX_I2C_REG_BAUD); - writel(0, drv_data->reg_base + MV64XXX_I2C_REG_SLAVE_ADDR); - writel(0, drv_data->reg_base + MV64XXX_I2C_REG_EXT_SLAVE_ADDR); + drv_data->reg_base + drv_data->regs->clock); + writel(0, drv_data->reg_base + drv_data->regs->addr); + writel(0, drv_data->reg_base + drv_data->regs->ext_addr); writel(MV64XXX_I2C_REG_CONTROL_TWSIEN | MV64XXX_I2C_REG_CONTROL_STOP, - drv_data->reg_base + MV64XXX_I2C_REG_CONTROL); + drv_data->reg_base + drv_data->regs->control); drv_data->state = MV64XXX_I2C_STATE_IDLE; } @@ -282,7 +275,7 @@ mv64xxx_i2c_do_action(struct mv64xxx_i2c_data *drv_data) drv_data->cntl_bits |= MV64XXX_I2C_REG_CONTROL_START; writel(drv_data->cntl_bits, - drv_data->reg_base + MV64XXX_I2C_REG_CONTROL); + drv_data->reg_base + drv_data->regs->control); drv_data->msgs++; drv_data->num_msgs--; @@ -300,48 +293,48 @@ mv64xxx_i2c_do_action(struct mv64xxx_i2c_data *drv_data) case MV64XXX_I2C_ACTION_CONTINUE: writel(drv_data->cntl_bits, - drv_data->reg_base + MV64XXX_I2C_REG_CONTROL); + drv_data->reg_base + drv_data->regs->control); break; case MV64XXX_I2C_ACTION_SEND_START: writel(drv_data->cntl_bits | MV64XXX_I2C_REG_CONTROL_START, - drv_data->reg_base + MV64XXX_I2C_REG_CONTROL); + drv_data->reg_base + drv_data->regs->control); break; case MV64XXX_I2C_ACTION_SEND_ADDR_1: writel(drv_data->addr1, - drv_data->reg_base + MV64XXX_I2C_REG_DATA); + drv_data->reg_base + drv_data->regs->data); writel(drv_data->cntl_bits, - drv_data->reg_base + MV64XXX_I2C_REG_CONTROL); + drv_data->reg_base + drv_data->regs->control); break; case MV64XXX_I2C_ACTION_SEND_ADDR_2: writel(drv_data->addr2, - drv_data->reg_base + MV64XXX_I2C_REG_DATA); + drv_data->reg_base + drv_data->regs->data); writel(drv_data->cntl_bits, - drv_data->reg_base + MV64XXX_I2C_REG_CONTROL); + drv_data->reg_base + drv_data->regs->control); break; case MV64XXX_I2C_ACTION_SEND_DATA: writel(drv_data->msg->buf[drv_data->byte_posn++], - drv_data->reg_base + MV64XXX_I2C_REG_DATA); + drv_data->reg_base + drv_data->regs->data); writel(drv_data->cntl_bits, - drv_data->reg_base + MV64XXX_I2C_REG_CONTROL); + drv_data->reg_base + drv_data->regs->control); break; case MV64XXX_I2C_ACTION_RCV_DATA: drv_data->msg->buf[drv_data->byte_posn++] = - readl(drv_data->reg_base + MV64XXX_I2C_REG_DATA); + readl(drv_data->reg_base + drv_data->regs->data); writel(drv_data->cntl_bits, - drv_data->reg_base + MV64XXX_I2C_REG_CONTROL); + drv_data->reg_base + drv_data->regs->control); break; case MV64XXX_I2C_ACTION_RCV_DATA_STOP: drv_data->msg->buf[drv_data->byte_posn++] = - readl(drv_data->reg_base + MV64XXX_I2C_REG_DATA); + readl(drv_data->reg_base + drv_data->regs->data); drv_data->cntl_bits &= ~MV64XXX_I2C_REG_CONTROL_INTEN; writel(drv_data->cntl_bits | MV64XXX_I2C_REG_CONTROL_STOP, - drv_data->reg_base + MV64XXX_I2C_REG_CONTROL); + drv_data->reg_base + drv_data->regs->control); drv_data->block = 0; wake_up(&drv_data->waitq); break; @@ -356,7 +349,7 @@ mv64xxx_i2c_do_action(struct mv64xxx_i2c_data *drv_data) case MV64XXX_I2C_ACTION_SEND_STOP: drv_data->cntl_bits &= ~MV64XXX_I2C_REG_CONTROL_INTEN; writel(drv_data->cntl_bits | MV64XXX_I2C_REG_CONTROL_STOP, - drv_data->reg_base + MV64XXX_I2C_REG_CONTROL); + drv_data->reg_base + drv_data->regs->control); drv_data->block = 0; wake_up(&drv_data->waitq); break; @@ -372,9 +365,9 @@ mv64xxx_i2c_intr(int irq, void *dev_id) irqreturn_t rc = IRQ_NONE; spin_lock_irqsave(&drv_data->lock, flags); - while (readl(drv_data->reg_base + MV64XXX_I2C_REG_CONTROL) & + while (readl(drv_data->reg_base + drv_data->regs->control) & MV64XXX_I2C_REG_CONTROL_IFLG) { - status = readl(drv_data->reg_base + MV64XXX_I2C_REG_STATUS); + status = readl(drv_data->reg_base + drv_data->regs->status); mv64xxx_i2c_fsm(drv_data, status); mv64xxx_i2c_do_action(drv_data); rc = IRQ_HANDLED; @@ -496,6 +489,12 @@ static const struct i2c_algorithm mv64xxx_i2c_algo = { ***************************************************************************** */ #ifdef CONFIG_OF +static const struct of_device_id mv64xxx_i2c_of_match_table[] = { + { .compatible = "marvell,mv64xxx-i2c", .data = &mv64xxx_i2c_regs_mv64xxx}, + {} +}; +MODULE_DEVICE_TABLE(of, mv64xxx_i2c_of_match_table); + static int mv64xxx_calc_freq(const int tclk, const int n, const int m) { @@ -528,8 +527,10 @@ mv64xxx_find_baud_factors(const u32 req_freq, const u32 tclk, u32 *best_n, static int mv64xxx_of_config(struct mv64xxx_i2c_data *drv_data, - struct device_node *np) + struct device *dev) { + const struct of_device_id *device; + struct device_node *np = dev->of_node; u32 bus_freq, tclk; int rc = 0; @@ -558,6 +559,13 @@ mv64xxx_of_config(struct mv64xxx_i2c_data *drv_data, * So hard code the value to 1 second. */ drv_data->adapter.timeout = HZ; + + device = of_match_device(mv64xxx_i2c_of_match_table, dev); + if (!device) + return -ENODEV; + + drv_data->regs = (struct mv64xxx_i2c_regs *)device->data; + out: return rc; #endif @@ -565,7 +573,7 @@ out: #else /* CONFIG_OF */ static int mv64xxx_of_config(struct mv64xxx_i2c_data *drv_data, - struct device_node *np) + struct device *dev) { return -ENODEV; } @@ -611,8 +619,9 @@ mv64xxx_i2c_probe(struct platform_device *pd) drv_data->freq_n = pdata->freq_n; drv_data->irq = platform_get_irq(pd, 0); drv_data->adapter.timeout = msecs_to_jiffies(pdata->timeout); + drv_data->regs = pdata->regs; } else if (pd->dev.of_node) { - rc = mv64xxx_of_config(drv_data, pd->dev.of_node); + rc = mv64xxx_of_config(drv_data, &pd->dev); if (rc) goto exit_clk; } @@ -680,12 +689,6 @@ mv64xxx_i2c_remove(struct platform_device *dev) return 0; } -static const struct of_device_id mv64xxx_i2c_of_match_table[] = { - { .compatible = "marvell,mv64xxx-i2c", }, - {} -}; -MODULE_DEVICE_TABLE(of, mv64xxx_i2c_of_match_table); - static struct platform_driver mv64xxx_i2c_driver = { .probe = mv64xxx_i2c_probe, .remove = mv64xxx_i2c_remove, diff --git a/include/linux/mv643xx_i2c.h b/include/linux/mv643xx_i2c.h index 5db5152..9304c94 100644 --- a/include/linux/mv643xx_i2c.h +++ b/include/linux/mv643xx_i2c.h @@ -12,11 +12,32 @@ #define MV64XXX_I2C_CTLR_NAME "mv64xxx_i2c" +struct mv64xxx_i2c_regs { + u8 addr; + u8 ext_addr; + u8 data; + u8 control; + u8 status; + u8 clock; + u8 soft_reset; +}; + +struct mv64xxx_i2c_regs mv64xxx_i2c_regs_mv64xxx = { + .addr = 0x00, + .ext_addr = 0x10, + .data = 0x04, + .control = 0x08, + .status = 0x0c, + .clock = 0x0c, + .soft_reset = 0x1c, +}; + /* i2c Platform Device, Driver Data */ struct mv64xxx_i2c_pdata { - u32 freq_m; - u32 freq_n; - u32 timeout; /* In milliseconds */ + u32 freq_m; + u32 freq_n; + struct mv64xxx_i2c_regs *regs; + u32 timeout; /* In milliseconds */ }; #endif /*_MV64XXX_I2C_H_*/
The Allwinner i2c controller uses the same logic as the Marvell one, but with slightly different register offsets. Introduce a structure that will be passed by either the pdata or associated to the compatible strings, and that holds the various registers that might be needed. Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com> --- drivers/i2c/busses/i2c-mv64xxx.c | 81 +++++++++++++++++++++------------------- include/linux/mv643xx_i2c.h | 27 ++++++++++++-- 2 files changed, 66 insertions(+), 42 deletions(-)