Re: [PATCH V2] i2c: imx-lpi2c: add target mode support

[Carlos Song]

> 
> 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