Re: [PATCH v5 2/3] i2c: at91: split driver into core and master file
From: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Date: 2019-02-25 10:34:50
Also in:
linux-i2c, lkml
On Fri, Feb 22, 2019 at 10:25:21AM +0100, Ludovic Desroches wrote:
From: Juergen Fitschen <redacted> The single file i2c-at91.c has been split into core code (i2c-at91-core.c) and master mode specific code (i2c-at91-master.c). This should enhance maintainability and reduce ifdeffery for slave mode related code. The code itself hasn't been touched. Shared functions only had to be made non-static. Furthermore, includes have been cleaned up.
Since it's a split of existing code, consider my comments as a way to improve it afterwards.
+static struct at91_twi_pdata *at91_twi_get_driver_data(
+ struct platform_device *pdev)
+{
+ if (pdev->dev.of_node) {
+ const struct of_device_id *match;
+ match = of_match_node(atmel_twi_dt_ids, pdev->dev.of_node);
+ if (!match)
+ return NULL;
+ return (struct at91_twi_pdata *)match->data;
+ }
+ return (struct at91_twi_pdata *) platform_get_device_id(pdev)->driver_data;One may do the following instead: struct at91_twi_pdata *data; data = device_get_match_data(&pdev->dev); if (data) return data; return ... And if you ever will switch old platform to somelike unified interface for device properties, I would recommend to use device_property_* instead of of_property_* and so on.
+}
+#include <linux/clk.h> +#include <linux/completion.h> +#include <linux/dma-mapping.h> +#include <linux/dmaengine.h> +#include <linux/i2c.h> +#include <linux/platform_data/dma-atmel.h> +#include <linux/platform_device.h>
Are all those anyhow in use in this header file?
+#define AT91_I2C_TIMEOUT msecs_to_jiffies(100) /* transfer timeout */
Wouldn't be better to use _MS here and call actual conversion whenever it's needed?
+#define AT91_I2C_DMA_THRESHOLD 8 /* enable DMA if transfer size is bigger than this threshold */
+#define AUTOSUSPEND_TIMEOUT 2000
_MS ?
+#define AT91_TWI_SR 0x0020 /* Status Register */ +#define AT91_TWI_TXCOMP BIT(0) /* Transmission Complete */ +#define AT91_TWI_RXRDY BIT(1) /* Receive Holding Register Ready */ +#define AT91_TWI_TXRDY BIT(2) /* Transmit Holding Register Ready */ +#define AT91_TWI_OVRE BIT(6) /* Overrun Error */ +#define AT91_TWI_UNRE BIT(7) /* Underrun Error */ +#define AT91_TWI_NACK BIT(8) /* Not Acknowledged */ +#define AT91_TWI_LOCK BIT(23) /* TWI Lock due to Frame Errors */ + +#define AT91_TWI_INT_MASK \ + (AT91_TWI_TXCOMP | AT91_TWI_RXRDY | AT91_TWI_TXRDY | AT91_TWI_NACK) + +#define AT91_TWI_IER 0x0024 /* Interrupt Enable Register */ +#define AT91_TWI_IDR 0x0028 /* Interrupt Disable Register */ +#define AT91_TWI_IMR 0x002c /* Interrupt Mask Register */ +#define AT91_TWI_RHR 0x0030 /* Receive Holding Register */ +#define AT91_TWI_THR 0x0034 /* Transmit Holding Register */ + +#define AT91_TWI_ACR 0x0040 /* Alternative Command Register */ +#define AT91_TWI_ACR_DATAL(len) ((len) & 0xff) +#define AT91_TWI_ACR_DIR BIT(8) + +#define AT91_TWI_FMR 0x0050 /* FIFO Mode Register */ +#define AT91_TWI_FMR_TXRDYM(mode) (((mode) & 0x3) << 0)
+#define AT91_TWI_FMR_TXRDYM_MASK (0x3 << 0)
GENMASK() ?
+#define AT91_TWI_FMR_RXRDYM(mode) (((mode) & 0x3) << 4)
+#define AT91_TWI_FMR_RXRDYM_MASK (0x3 << 4)
Ditto. -- With Best Regards, Andy Shevchenko _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel