Hi Carlos,
Looks good, just few little comments.
On Thu, Aug 29, 2024 at 06:54:44PM GMT, carlos.song@xxxxxxx wrote:
> From: Carlos Song <carlos.song@xxxxxxx>
>
> Add target mode support for LPI2C.
Can you please be a bit more descriptive? What is this mode
doing, how it works, what are you adding, etc. etc. etc.
> Signed-off-by: Carlos Song <carlos.song@xxxxxxx>
> ---
> Change for V2:
> - remove unused variable ‘lpi2c_imx’ in lpi2c_suspend_noirq.
this was part of a 5 patches series. Is that series superseded?
(please, next time when you send a series with more than one
patch, include a cover letter).
...
> +#define SCR_SEN BIT(0)
> +#define SCR_RST BIT(1)
> +#define SCR_FILTEN BIT(4)
> +#define SCR_RTF BIT(8)
> +#define SCR_RRF BIT(9)
> +#define SCFGR1_RXSTALL BIT(1)
> +#define SCFGR1_TXDSTALL BIT(2)
> +#define SCFGR2_FILTSDA_SHIFT 24
> +#define SCFGR2_FILTSCL_SHIFT 16
> +#define SCFGR2_CLKHOLD(x) (x)
> +#define SCFGR2_FILTSDA(x) ((x) << SCFGR2_FILTSDA_SHIFT)
> +#define SCFGR2_FILTSCL(x) ((x) << SCFGR2_FILTSCL_SHIFT)
> +#define SSR_TDF BIT(0)
> +#define SSR_RDF BIT(1)
> +#define SSR_AVF BIT(2)
> +#define SSR_TAF BIT(3)
> +#define SSR_RSF BIT(8)
> +#define SSR_SDF BIT(9)
> +#define SSR_BEF BIT(10)
> +#define SSR_FEF BIT(11)
> +#define SSR_SBF BIT(24)
> +#define SSR_BBF BIT(25)
> +#define SSR_CLEAR_BITS (SSR_RSF | SSR_SDF | SSR_BEF | SSR_FEF)
> +#define SIER_TDIE BIT(0)
> +#define SIER_RDIE BIT(1)
> +#define SIER_AVIE BIT(2)
> +#define SIER_TAIE BIT(3)
> +#define SIER_RSIE BIT(8)
> +#define SIER_SDIE BIT(9)
> +#define SIER_BEIE BIT(10)
> +#define SIER_FEIE BIT(11)
> +#define SIER_AM0F BIT(12)
> +#define SASR_READ_REQ 0x1
> +#define SLAVE_INT_FLAG (SIER_TDIE | SIER_RDIE | SIER_AVIE | \
> + SIER_SDIE | SIER_BEIE)
I'm not happy about the alignment here, can we have the columns
well aligned, please?
> +
> #define I2C_CLK_RATIO 2
> #define CHUNK_DATA 256
...
> + if (sier_filter & SSR_SDF) {
> + /* STOP */
> + i2c_slave_event(lpi2c_imx->target, I2C_SLAVE_STOP, &value);
> + }
nit: no need for brackets here.
...
> +static irqreturn_t lpi2c_imx_isr(int irq, void *dev_id)
> +{
> + struct lpi2c_imx_struct *lpi2c_imx = dev_id;
> + u32 ssr, sier_filter;
> + unsigned int scr;
why is ssr unsigned int and not u32?
Can we define ssr, sier_filter and scr in the innermost section?
i.e. inside the "if () { ... }"
> +
> + if (lpi2c_imx->target) {
> + scr = readl(lpi2c_imx->base + LPI2C_SCR);
> + ssr = readl(lpi2c_imx->base + LPI2C_SSR);
> + sier_filter = ssr & readl(lpi2c_imx->base + LPI2C_SIER);
> + if ((scr & SCR_SEN) && sier_filter)
> + return lpi2c_imx_target_isr(lpi2c_imx, ssr, sier_filter);
> + else
> + return lpi2c_imx_master_isr(lpi2c_imx);
> + } else {
> + return lpi2c_imx_master_isr(lpi2c_imx);
> + }
this can be simplified a bit:
if (...) {
scr = ...
ssr = ...
sier_filter = ...
/* a nice comment */
if (...)
return lpi2c_imx_target_isr(...)
}
return lpi2c_imx_master_isr(lpi2c_imx);
Not a binding comment. Your choice.
> +}
> +
> +static void lpi2c_imx_target_init(struct lpi2c_imx_struct *lpi2c_imx)
> +{
> + int temp;
u32?
Thanks,
Andi
