--- Begin Message ---
- To: Christophe Leroy <christophe.leroy@xxxxxxxxxx>
- Subject: Re: [PATCH v2 2/7] drivers: misc: Add support for the Lantiq PEF2256 framer
- From: Herve Codina <herve.codina@xxxxxxxxxxx>
- Date: Fri, 17 Mar 2023 17:24:41 +0100
- Cc: Rob Herring <robh+dt@xxxxxxxxxx>, Krzysztof Kozlowski <krzysztof.kozlowski+dt@xxxxxxxxxx>, Liam Girdwood <lgirdwood@xxxxxxxxx>, Mark Brown <broonie@xxxxxxxxxx>, Derek Kiernan <derek.kiernan@xxxxxxxxxx>, Dragan Cvetic <dragan.cvetic@xxxxxxxxxx>, Arnd Bergmann <arnd@xxxxxxxx>, Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>, Takashi Iwai <tiwai@xxxxxxxx>, "linux-kernel@xxxxxxxxxxxxxxx" <linux-kernel@xxxxxxxxxxxxxxx>, "devicetree@xxxxxxxxxxxxxxx" <devicetree@xxxxxxxxxxxxxxx>, "alsa-devel@xxxxxxxxxxxxxxxx" <alsa-devel@xxxxxxxxxxxxxxxx>, Thomas Petazzoni <thomas.petazzoni@xxxxxxxxxxx>
- In-reply-to: <43b7e386-5b85-3d51-5431-4a46b2779729@csgroup.eu>
- Organization: Bootlin
- References: <20230316122741.577663-1-herve.codina@bootlin.com> <20230316122741.577663-3-herve.codina@bootlin.com> <43b7e386-5b85-3d51-5431-4a46b2779729@csgroup.eu>
Hi Christophe, On Thu, 16 Mar 2023 14:00:42 +0000 Christophe Leroy <christophe.leroy@xxxxxxxxxx> wrote: [...] > > +#define PEF2256_PC6 0x86 /* Port Configuration 6 */ > > +#define PEF2256_GCM(_n) (0x92 + _n - 1) /* Global Counter Mode n=1..8 */ > > Should be (0x92 + (_n) - 1) to avoid any risk. Yes definitively. Will be updated in v3. > > > +#define PEF2256_GCM1 0x92 /* Global Counter Mode 1 */ [...] > > +#define PEF2256_GIS_ISR(_n) (1<<(n)) > > _n or n ? Should be (1 << (_n)) Will be updated in v3. > > > +#define PEF2256_WID 0xEC /* Wafer Identification Register */ [...] > > +static inline void pef2256_clrbits8(struct pef2256 *pef2256, int offset, u8 clr) > > +{ > > + u8 v; > > + > > + v = pef2256_read8(pef2256, offset); > > + v &= ~clr; > > + pef2256_write8(pef2256, offset, v); > > +} > > Not sure it is worth the number of lines. > > Would liekly be as good with just: > > pef2256_write8(pef2256, offset, pef2256_read8(pef2256, offset) & ~clr); > > Same for all. Will be updated in v3. > > > Also, it shouldn't need to be flagged 'inline' as it is defined in the C > file it is used. GCC will decide by itself if it is worth inlining. In the kernel register accessor helpers are traditionally inline. # git grep 'static .* u32 .*read.*(' drivers/*.c | grep inline | wc -l 432 # git grep 'static .* u32 .*read.*(' drivers/*.c | grep -v inline | wc -l 1 Sure, my git grep is not accurate but it gives ideas of the quantities. I prefer to keep the inline for the register accessor helpers. > > > + > > +static inline void pef2256_setbits8(struct pef2256 *pef2256, int offset, u8 set) > > +{ > > + u8 v; > > + > > + v = pef2256_read8(pef2256, offset); > > + v |= set; > > + pef2256_write8(pef2256, offset, v); > > +} > > + > > + > > +static inline void pef2256_clrsetbits8(struct pef2256 *pef2256, int offset, u8 clr, u8 set) > > +{ > > + u8 v; > > + > > + v = pef2256_read8(pef2256, offset); > > + v &= ~clr; > > + v |= set; > > + pef2256_write8(pef2256, offset, v); > > +} > > + > > +static enum pef2256_version pef2256_get_version(struct pef2256 *pef2256) > > +{ > > + enum pef2256_version version; > > + const char *version_txt; > > + u8 vstr, wid; > > + > > + vstr = pef2256_read8(pef2256, PEF2256_VSTR); > > + wid = pef2256_read8(pef2256, PEF2256_WID); > > + > > + switch (vstr) { > > + case PEF2256_VSTR_VERSION_12: > > + if ((wid & PEF2256_12_WID_MASK) == PEF2256_12_WID_VERSION_12) { > > + version_txt = "1.2"; > > + version = PEF2256_VERSION_1_2; > > + goto end; > > + } > > + break; > > + case PEF2256_VSTR_VERSION_2x: > > + switch (wid & PEF2256_2X_WID_MASK) { > > + case PEF2256_2X_WID_VERSION_21: > > + version_txt = "2.1 (2.x)"; > > + version = PEF2256_VERSION_2_1; > > + goto end; > > + case PEF2256_2X_WID_VERSION_22: > > + version_txt = "2.2 (2.x)"; > > + version = PEF2256_VERSION_2_2; > > + goto end; > > + default: > > + break; > > + } > > + break; > > + case PEF2256_VSTR_VERSION_21: > > + version_txt = "2.1"; > > + version = PEF2256_VERSION_2_1; > > + goto end; > > + default: > > + break; > > A default doing nothing is not needed unless the switch handles a enum > and you have not covered all possible values. > > My preference would be that you use the default to set: > version = PEF2256_VERSION_UNKNOWN; > > then use an if (version == PEF2256_VERSION_UNKNOWN) / else below for the > dev_err/dev_info. The function will be reworked in v3 > > > + } > > + > > + dev_err(pef2256->dev, "Unknown version (0x%02x, 0x%02x)\n", vstr, wid); > > + return PEF2256_VERSION_UNKNOWN; > > + > > +end: > > + dev_info(pef2256->dev, "Version %s detected\n", version_txt); > > + return version; > > +} > > + > > +static int pef2256_12_setup_gcm(struct pef2256 *pef2256) > > +{ > > + static const u8 gcm_1544000[6] = {0xF0, 0x51, 0x00, 0x80, 0x00, 0x15}; > > + static const u8 gcm_2048000[6] = {0x00, 0x58, 0xD2, 0xC2, 0x00, 0x10}; > > + static const u8 gcm_8192000[6] = {0x00, 0x58, 0xD2, 0xC2, 0x03, 0x10}; > > + static const u8 gcm_10000000[6] = {0x90, 0x51, 0x81, 0x8F, 0x04, 0x10}; > > + static const u8 gcm_12352000[6] = {0xF0, 0x51, 0x00, 0x80, 0x07, 0x15}; > > + static const u8 gcm_16384000[6] = {0x00, 0x58, 0xD2, 0xC2, 0x07, 0x10}; > > + unsigned long mclk_rate; > > + const u8 *gcm; > > + int i; > > + > > + mclk_rate = clk_get_rate(pef2256->mclk); > > + switch (mclk_rate) { > > + case 1544000: > > + gcm = gcm_1544000; > > + break; > > + case 2048000: > > + gcm = gcm_2048000; > > + break; > > + case 8192000: > > + gcm = gcm_8192000; > > + break; > > + case 10000000: > > + gcm = gcm_10000000; > > + break; > > + case 12352000: > > + gcm = gcm_12352000; > > + break; > > + case 16384000: > > + gcm = gcm_16384000; > > + break; > > + default: > > + dev_err(pef2256->dev, "Unsupported v1.2 MCLK rate %lu\n", mclk_rate); > > + return -EINVAL; > > + } > > + for (i = 0; i < 6; i++) > > + pef2256_write8(pef2256, PEF2256_GCM(i+1), gcm[i]); > > + > > + return 0; > > +} > > + > > +static int pef2256_2x_setup_gcm(struct pef2256 *pef2256) > > +{ > > + static const u8 gcm_1544000[8] = {0x00, 0x15, 0x00, 0x08, 0x00, 0x3F, 0x9C, 0xDF}; > > + static const u8 gcm_2048000[8] = {0x00, 0x18, 0xFB, 0x0B, 0x00, 0x2F, 0xDB, 0xDF}; > > + static const u8 gcm_8192000[8] = {0x00, 0x18, 0xFB, 0x0B, 0x00, 0x0B, 0xDB, 0xDF}; > > + static const u8 gcm_10000000[8] = {0x40, 0x1B, 0x3D, 0x0A, 0x00, 0x07, 0xC9, 0xDC}; > > + static const u8 gcm_12352000[8] = {0x00, 0x19, 0x00, 0x08, 0x01, 0x0A, 0x98, 0xDA}; > > + static const u8 gcm_16384000[8] = {0x00, 0x18, 0xFB, 0x0B, 0x01, 0x0B, 0xDB, 0xDF}; > > + unsigned long mclk_rate; > > + const u8 *gcm; > > + int i; > > + > > + mclk_rate = clk_get_rate(pef2256->mclk); > > + switch (mclk_rate) { > > + case 1544000: > > + gcm = gcm_1544000; > > + break; > > + case 2048000: > > + gcm = gcm_2048000; > > + break; > > + case 8192000: > > + gcm = gcm_8192000; > > + break; > > + case 10000000: > > + gcm = gcm_10000000; > > + break; > > + case 12352000: > > + gcm = gcm_12352000; > > + break; > > + case 16384000: > > + gcm = gcm_16384000; > > + break; > > + default: > > + dev_err(pef2256->dev, "Unsupported v2.x MCLK rate %lu\n", mclk_rate); > > + return -EINVAL; > > + } > > + for (i = 0; i < 8; i++) > > + pef2256_write8(pef2256, PEF2256_GCM(i+1), gcm[i]); > > + > > + return 0; > > +} > > This function and the previous one look very similar, can we merge them ? Yes, will be merged in v3. > > > + > > +static int pef2256_setup_gcm(struct pef2256 *pef2256) > > +{ > > + return (pef2256->version == PEF2256_VERSION_1_2) ? > > + pef2256_12_setup_gcm(pef2256) : pef2256_2x_setup_gcm(pef2256); > > +} > > + > > +static int pef2256_setup_e1(struct pef2256 *pef2256) > > +{ > > + u8 fmr1, fmr2, sic1; > > + int ret; > > + > > + /* Basic setup, Master clocking mode (GCM8..1) */ > > + ret = pef2256_setup_gcm(pef2256); > > + if (ret) > > + return ret; > > + > > + /* RCLK output : DPLL clock, DCO-X enabled, DCO-X internal ref clock */ > > + pef2256_write8(pef2256, PEF2256_CMR1, 0x00); > > + > > + /* > > + * SCLKR selected, SCLKX selected, > > + * receive synchro pulse sourced by SYPR, > > + * transmit synchro pulse sourced by SYPX > > + */ > > + pef2256_write8(pef2256, PEF2256_CMR2, 0x00); > > + > > + /* NRZ coding, no alarm simulation */ > > + pef2256_write8(pef2256, PEF2256_FMR0, 0x00); > > + > > + /* > > + * E1, frame format, 2 Mbit/s system data rate, no AIS > > + * transmission to remote end or system interface, payload loop > > + * off, transmit remote alarm on > > + */ > > + fmr1 = 0x00; > > + fmr2 = PEF2256_FMR2_AXRA; > > + switch (pef2256->frame_type) { > > + case PEF2256_FRAME_E1_DOUBLEFRAME: > > + fmr2 |= PEF2256_FMR2_RFS_DOUBLEFRAME; > > + break; > > + case PEF2256_FRAME_E1_CRC4_MULTIFRAME: > > + fmr1 |= PEF2256_FMR1_XFS; > > + fmr2 |= PEF2256_FMR2_RFS_CRC4_MULTIFRAME; > > + break; > > + case PEF2256_FRAME_E1_AUTO_MULTIFRAME: > > + fmr1 |= PEF2256_FMR1_XFS; > > + fmr2 |= PEF2256_FMR2_RFS_AUTO_MULTIFRAME; > > + break; > > + default: > > + dev_err(pef2256->dev, "Unsupported frame type %d\n", pef2256->frame_type); > > + return -EINVAL; > > + } > > + pef2256_write8(pef2256, PEF2256_FMR1, fmr1); > > + pef2256_write8(pef2256, PEF2256_FMR2, fmr2); > > + > > + /* E1 default for the receive slicer threshold */ > > + pef2256_write8(pef2256, PEF2256_LIM2, PEF2256_LIM2_SLT_THR50); > > + if (!pef2256->is_subordinate) { > > + /* SEC input, active high */ > > + pef2256_write8(pef2256, PEF2256_GPC1, PEF2256_GPC1_CSFP_SEC_IN_HIGH); > > + } else { > > + /* Loop-timed */ > > + pef2256_setbits8(pef2256, PEF2256_LIM2, PEF2256_LIM2_ELT); > > + /* FSC output, active high */ > > + pef2256_write8(pef2256, PEF2256_GPC1, PEF2256_GPC1_CSFP_FSC_OUT_HIGH); > > + } > > + > > + /* internal second timer, power on */ > > + pef2256_write8(pef2256, PEF2256_GCR, 0x00); > > + > > + /* > > + * slave mode, local loop off, mode short-haul > > + * In v2.x, bit3 is a forced 1 bit in the datasheet -> Need to be set. > > + */ > > + if (pef2256->version == PEF2256_VERSION_1_2) > > + pef2256_write8(pef2256, PEF2256_LIM0, 0x00); > > + else > > + pef2256_write8(pef2256, PEF2256_LIM0, PEF2256_2X_LIM0_BIT3); > > + > > + /* analog interface selected, remote loop off */ > > + pef2256_write8(pef2256, PEF2256_LIM1, 0x00); > > + > > + /* > > + * SCLKR, SCLKX, RCLK configured to inputs, > > + * XFMS active low, CLK1 and CLK2 pin configuration > > + */ > > + pef2256_write8(pef2256, PEF2256_PC5, 0x00); > > + pef2256_write8(pef2256, PEF2256_PC6, 0x00); > > + > > + /* > > + * 2.048 MHz system clocking rate, receive buffer 2 frames, transmit > > + * buffer bypass, data sampled and transmitted on the falling edge of > > + * SCLKR/X, automatic freeze signaling, data is active in the first > > + * channel phase > > + */ > > + pef2256_write8(pef2256, PEF2256_SIC1, 0x00); > > + pef2256_write8(pef2256, PEF2256_SIC2, 0x00); > > + pef2256_write8(pef2256, PEF2256_SIC3, 0x00); > > + > > + /* channel loop-back and single frame mode are disabled */ > > + pef2256_write8(pef2256, PEF2256_LOOP, 0x00); > > + > > + /* all bits of the transmitted service word are cleared */ > > + pef2256_write8(pef2256, PEF2256_XSW, PEF2256_XSW_XY(0x1F)); > > + /* CAS disabled and clear spare bit values */ > > + pef2256_write8(pef2256, PEF2256_XSP, 0x00); > > + > > + /* no transparent mode active */ > > + pef2256_write8(pef2256, PEF2256_TSWM, 0x00); > > + > > + /* > > + * the transmit clock offset is cleared > > + * the transmit time slot offset is cleared > > + */ > > + pef2256_write8(pef2256, PEF2256_XC0, 0x00); > > + > > + /* Keep default value for the transmit offset */ > > + pef2256_write8(pef2256, PEF2256_XC1, 0x9C); > > + > > + /* > > + * transmit pulse mask, default value from datasheet > > + * transmitter in tristate mode > > + */ > > + if (pef2256->version == PEF2256_VERSION_1_2) { > > + pef2256_write8(pef2256, PEF2256_XPM0, 0x7B); > > + pef2256_write8(pef2256, PEF2256_XPM1, 0x03); > > + pef2256_write8(pef2256, PEF2256_XPM2, PEF2256_XPM2_XLT | 0x00); > > + } else { > > + pef2256_write8(pef2256, PEF2256_XPM0, 0x9C); > > + pef2256_write8(pef2256, PEF2256_XPM1, 0x03); > > + pef2256_write8(pef2256, PEF2256_XPM2, PEF2256_XPM2_XLT | 0x00); > > + } > > Only first line seem different, could XPM1 and XPM2 be outside the if/else ? Sure. Will be updated in v3. > > > + > > + /* "master" mode */ > > + if (!pef2256->is_subordinate) > > + pef2256_setbits8(pef2256, PEF2256_LIM0, PEF2256_LIM0_MAS); > > + > > + /* transmit line in normal operation */ > > + pef2256_clrbits8(pef2256, PEF2256_XPM2, PEF2256_XPM2_XLT); > > + > > + if (pef2256->version == PEF2256_VERSION_1_2) { > > + /* receive input threshold = 0,21V */ > > + pef2256_clrsetbits8(pef2256, PEF2256_LIM1, PEF2256_12_LIM1_RIL_MASK, > > + PEF2256_12_LIM1_RIL_210); > > + } else { > > + /* receive input threshold = 0,21V */ > > Same comment twice, could be before the 'if' and remove the { } then ? Will be updated in v3. > > > + pef2256_clrsetbits8(pef2256, PEF2256_LIM1, PEF2256_2X_LIM1_RIL_MASK, > > + PEF2256_2X_LIM1_RIL_210); > > + } > > + /* transmit line coding = HDB3 */ > > + pef2256_clrsetbits8(pef2256, PEF2256_FMR0, PEF2256_FMR0_XC_MASK, > > + PEF2256_FMR0_XC_HDB3); > > Wouldn't that fit in a single line with the new recommended 100 char max > line length ? I thing it would be more readable as a single line. Will be updated in v3 if it fits in 100 chars. Same for the line right below related to the receive part. > > > + > > + /* receive line coding = HDB3 */ > > + pef2256_clrsetbits8(pef2256, PEF2256_FMR0, PEF2256_FMR0_RC_MASK, > > + PEF2256_FMR0_RC_HDB3); > > + > > + /* detection of LOS alarm = 176 pulses (ie (10 + 1) * 16) */ > > + pef2256_write8(pef2256, PEF2256_PCD, 10); > > + > > + /* recovery of LOS alarm = 22 pulses (ie 21 + 1) */ > > + pef2256_write8(pef2256, PEF2256_PCR, 21); > > + > > + /* DCO-X center frequency enabled */ > > + pef2256_setbits8(pef2256, PEF2256_CMR2, PEF2256_CMR2_DCOXC); > > + > > + if (pef2256->is_subordinate) { > > + /* select RCLK source = 2M */ > > + pef2256_clrsetbits8(pef2256, PEF2256_CMR1, PEF2256_CMR1_RS_MASK, > > + PEF2256_CMR1_RS_DCOR_2048); > > + /* disable switching from RCLK to SYNC */ > > + pef2256_setbits8(pef2256, PEF2256_CMR1, PEF2256_CMR1_DCS); > > + } > > I'd move the comments into a single one before the 'if', the two > comments are related. I will change to a single line comment but not before the 'if'. The comment is related to the subordinate case and not global for 'all' cases. > > > + > > + if (pef2256->version != PEF2256_VERSION_1_2) { > > + /* during inactive channel phase switch RDO/RSIG into tri-state */ > > + pef2256_setbits8(pef2256, PEF2256_SIC3, PEF2256_SIC3_RTRI); > > + } > > I'd put the comment before the 'if' and remove the { } Same reason to keep as it. The comment is specific to the if content. > > > + > > + if (!pef2256->is_tx_falling_edge) { > > + /* rising edge sync pulse transmit */ > > This comment doesn't seem to match the condition (rising versus falling). if not falling, it is rising. I will update in v3 to invert the condition and improve the comment by mentioning transmit and receive in each cases. > > > + pef2256_clrsetbits8(pef2256, PEF2256_SIC3, > > + PEF2256_SIC3_RESR, PEF2256_SIC3_RESX); > > + } else { > > + /* rising edge sync pulse receive */ > > This comment doesn't seem to match the condition (receive versus tx). > > > + pef2256_clrsetbits8(pef2256, PEF2256_SIC3, > > + PEF2256_SIC3_RESX, PEF2256_SIC3_RESR); > > + } > > + > > + /* transmit offset counter (XCO10..0) = 4 */ > > + pef2256_write8(pef2256, PEF2256_XC0, 0); > > + pef2256_write8(pef2256, PEF2256_XC1, 4); > > + /* receive offset counter (RCO10..0) = 4 */ > > + pef2256_write8(pef2256, PEF2256_RC0, 0); > > + pef2256_write8(pef2256, PEF2256_RC1, 4); > > + > > + /* system clock rate */ > > + switch (pef2256->sysclk_rate) { > > + case 2048000: > > + sic1 = PEF2256_SIC1_SSC_2048; > > + break; > > + case 4096000: > > + sic1 = PEF2256_SIC1_SSC_4096; > > + break; > > + case 8192000: > > + sic1 = PEF2256_SIC1_SSC_8192; > > + break; > > + case 16384000: > > + sic1 = PEF2256_SIC1_SSC_16384; > > + break; > > + default: > > + dev_err(pef2256->dev, "Unsupported sysclk rate %u\n", pef2256->sysclk_rate); > > + return -EINVAL; > > + } > > + pef2256_clrsetbits8(pef2256, PEF2256_SIC1, PEF2256_SIC1_SSC_MASK, sic1); > > + > > + /* data clock rate */ > > + switch (pef2256->data_rate) { > > + case 2048000: > > + fmr1 = PEF2256_FMR1_SSD_2048; > > + sic1 = PEF2256_SIC1_SSD_2048; > > + break; > > + case 4096000: > > + fmr1 = PEF2256_FMR1_SSD_4096; > > + sic1 = PEF2256_SIC1_SSD_4096; > > + break; > > + case 8192000: > > + fmr1 = PEF2256_FMR1_SSD_8192; > > + sic1 = PEF2256_SIC1_SSD_8192; > > + break; > > + case 16384000: > > + fmr1 = PEF2256_FMR1_SSD_16384; > > + sic1 = PEF2256_SIC1_SSD_16384; > > + break; > > + default: > > + dev_err(pef2256->dev, "Unsupported data rate %u\n", pef2256->data_rate); > > + return -EINVAL; > > + } > > + pef2256_clrsetbits8(pef2256, PEF2256_FMR1, PEF2256_FMR1_SSD_MASK, fmr1); > > + pef2256_clrsetbits8(pef2256, PEF2256_SIC1, PEF2256_SIC1_SSD_MASK, sic1); > > + > > + /* channel phase */ > > + pef2256_clrsetbits8(pef2256, PEF2256_SIC2, PEF2256_SIC2_SICS_MASK, > > + PEF2256_SIC2_SICS(pef2256->channel_phase)); > > + > > + if (pef2256->is_subordinate) { > > + /* transmit buffer size = 2 frames */ > > + pef2256_clrsetbits8(pef2256, PEF2256_SIC1, PEF2256_SIC1_XBS_MASK, > > + PEF2256_SIC1_XBS_2FRAMES); > > + /* transmit transparent mode */ > > + pef2256_setbits8(pef2256, PEF2256_XSW, PEF2256_XSW_XTM); > > + } > > Could this block get regrouped with the RX block depending on > is_subordinate ? Yes, Will be done in v3. > > > + > > + /* error counter latched every 1s */ > > + pef2256_setbits8(pef2256, PEF2256_FMR1, PEF2256_FMR1_ECM); > > + /* error counter mode COFA */ > > + pef2256_setbits8(pef2256, PEF2256_GCR, PEF2256_GCR_ECMC); > > + /* errors in service words have no influence */ > > + pef2256_setbits8(pef2256, PEF2256_RC0, PEF2256_RC0_SWD); > > + /* 4 consecutive incorrect FAS causes loss of sync */ > > + pef2256_setbits8(pef2256, PEF2256_RC0, PEF2256_RC0_ASY4); > > + /* Si-Bit, Spare bit For International, FAS word */ > > + pef2256_setbits8(pef2256, PEF2256_XSW, PEF2256_XSW_XSIS); > > + pef2256_setbits8(pef2256, PEF2256_XSP, PEF2256_XSP_XSIF); > > + > > + /* port RCLK is output */ > > + pef2256_setbits8(pef2256, PEF2256_PC5, PEF2256_PC5_CRP); > > + /* status changed interrupt at both up and down */ > > + pef2256_setbits8(pef2256, PEF2256_GCR, PEF2256_GCR_SCI); > > + > > + /* Clear any ISR2 pending interrupts and unmask needed interrupts */ > > + pef2256_read8(pef2256, PEF2256_ISR2); > > + pef2256_clrbits8(pef2256, PEF2256_IMR2, PEF2256_INT2_LOS | PEF2256_INT2_AIS); > > + > > + /* reset lines */ > > + pef2256_write8(pef2256, PEF2256_CMDR, PEF2256_CMDR_RRES | PEF2256_CMDR_XRES); > > + return 0; > > +} > > + > > +static int pef2256_setup(struct pef2256 *pef2256) > > +{ > > + if (!pef2256->is_e1) { > > + dev_err(pef2256->dev, "Only E1 line is currently supported\n"); > > + return -EOPNOTSUPP; > > + } > > + > > + return pef2256_setup_e1(pef2256); > > In order to use future addition of other modes, I'd do: > > if (!pef2256->is_e1) > return pef2256_setup_e1(pef2256); > > dev_err(pef2256->dev, "Only E1 line is currently supported\n"); > return -EOPNOTSUPP; > Will be changed in v3 using your proposition (with 'if' condition fixed). > > +} > > + > > + > > + > > +static void pef2256_isr_default_handler(struct pef2256 *pef2256, u8 nbr, u8 isr) > > +{ > > + dev_warn(pef2256->dev, "ISR%u: 0x%02x not handled\n", nbr, isr); > > +} > > + > > +static bool pef2256_is_carrier_on(struct pef2256 *pef2256) > > +{ > > + u8 frs0; > > + > > + frs0 = pef2256_read8(pef2256, PEF2256_FRS0); > > + return !(frs0 & (PEF2256_FRS0_LOS | PEF2256_FRS0_AIS)); > > +} > > + > > +static void pef2256_isr2_handler(struct pef2256 *pef2256, u8 nbr, u8 isr) > > +{ > > + bool is_changed = false; > > + unsigned long flags; > > + bool is_carrier_on; > > + > > + if (isr & (PEF2256_INT2_LOS | PEF2256_INT2_AIS)) { > > + > > + spin_lock_irqsave(&pef2256->lock, flags); > > + is_carrier_on = pef2256_is_carrier_on(pef2256); > > + if (is_carrier_on != pef2256->is_carrier_on) { > > + pef2256->is_carrier_on = is_carrier_on; > > + is_changed = true; > > + } > > + spin_unlock_irqrestore(&pef2256->lock, flags); > > Do we really need spin_locks for that ? > If atomicity is really an issue, may we use atomic operations instead ? Indeed, I can use atomic ops here. Will be changed in v3. > > > + > > + if (is_changed) > > + atomic_notifier_call_chain(&pef2256->event_notifier_list, > > + PEF2256_EVENT_CARRIER, NULL); > > + } > > +} > > + Thanks for the review. Regards, Hervé -- Hervé Codina, Bootlin Embedded Linux and Kernel engineering https://bootlin.com
--- End Message ---
