>
> 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.
>
Hi, Andi
Thank you for your suggestion! I will enrich it as much as possible.
> > 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?
>
Oh... so sorry about this, it was my mistake.
Before my true intention is only to fix it using V2 patch not replace the series.
Other patches are used to fix other problems. No change in other patches
So I don't send out them. Now I know this is completely wrong.
I need to send all patches in one serials with a cover letter even if other
patches have not changed. I should update all patches status at the same time
using change log, it will be more clear. Next I will follow this rule.
> (please, next time when you send a series with more than one patch, include a
> cover letter).
>
Yes. I will do this in the future. Thank you.
One thing bother me, for other patches in this series, I plan to separate this series patches
and send them out in different series(maybe it looks like I dropped this patches series, later I will use other series).
So for this patch, can you accept that I only send this V3 patch fix without other patches?(Later other patches will send out
in other series separately, one patches series for fixing one issue).
> ...
>
> > +#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
