On Tue, 2018-07-17 at 13:48 +0200, Alexandre Belloni wrote:
> The Microsemi Ocelot I2C controller is a designware IP. It also has a
> second set of registers to allow tweaking SDA hold time and spike
> filtering.
Thanks for information you provided. See my comments below.
> struct dw_i2c_dev {
> struct device *dev;
> void __iomem *base;
> + void __iomem *base_ext;
Maybe simple "ext"? Up to you.
> +#define MSCC_ICPU_CFG_TWI_DELAY 0x0
> +#define MSCC_ICPU_CFG_TWI_DELAY_ENABLE BIT(0)
> +#define MSCC_ICPU_CFG_TWI_SPIKE_FILTER 0x4
> +
> +static int mscc_twi_set_sda_hold_time(struct dw_i2c_dev *dev)
> +{
> + writel((dev->sda_hold_time << 1) |
> MSCC_ICPU_CFG_TWI_DELAY_ENABLE,
> + dev->base_ext + MSCC_ICPU_CFG_TWI_DELAY);
> +
> + return 0;
> +}
(1)
>
> + if (of_device_is_compatible(pdev->dev.of_node, "mscc,ocelot-
> i2c")) {
> + mem = platform_get_resource(pdev, IORESOURCE_MEM, 1);
> + dev->base_ext = devm_ioremap_resource(&pdev->dev,
> mem);
> + if (!IS_ERR(dev->base_ext))
> + dev->set_sda_hold_time =
> mscc_twi_set_sda_hold_time;
> + }
(2)
> static const struct of_device_id dw_i2c_of_match[] = {
> { .compatible = "snps,designware-i2c", },
> + { .compatible = "mscc,ocelot-i2c", },
> {},
> };
(3)
I would rather place them in analogue how we do for ACPI, i.e.
--- 8< --- 8< ---
...
#define MODEL_MSCC_OCELOT 0x00000200
#define MODEL_MASK 0x00000f00
...
#ifdef CONFIG_OF
-->(1)
int dw_i2c_of_configure(pdev)
{
...
-->(2) (perhaps we don't care if it goes without condition, or move it
below in the corresponding case?
...
switch(dev->flags & MODEL_MASK) {
case MODEL_MSCC_OCELOT:
...
break;
default:
break;
}
return 0;
}
-->(3)
...
{ .compatible = "mscc,ocelot-i2c", .data = (void *)MODEL_MSCC_OCELOT },
...
#else
static inline int dw_i2c_of_configure(pdev) { return -ENODEV; }
#endif
...
->probe():
...
/* REPLACE THIS in dw_i2c_acpi_configure() by below in ->probe():
* id = acpi_match_device(pdev->dev.driver->acpi_match_table, &pdev-
>dev);
* if (id && id->driver_data)
* dev->flags |= (u32)id->driver_data;
*/
dev->flags |= (u32)device_get_match_data(&pdev->dev);
if (&pdev->dev.of_node)
dw_i2c_of_configure(pdev);
...
--- 8< --- 8< ---
What do you think?
--
Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>
Intel Finland Oy
