Re: [PATCH v2 2/7] drivers: misc: Add support for the Lantiq PEF2256 framer

From: Herve Codina
Date: Fri Mar 17 2023 - 12:25:00 EST

Next message: Álvaro Fernández Rojas: "Re: [PATCH] net: dsa: tag_brcm: legacy: fix daisy-chained switches"
Previous message: syzbot: "[syzbot] [ntfs?] KASAN: use-after-free Read in ntfs_read_folio"
In reply to: Christophe Leroy: "Re: [PATCH v2 2/7] drivers: misc: Add support for the Lantiq PEF2256 framer"
Next in thread: Herve Codina: "[PATCH v2 4/7] MAINTAINERS: Add the Lantiq PEF2256 driver entry"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

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

Next message: Álvaro Fernández Rojas: "Re: [PATCH] net: dsa: tag_brcm: legacy: fix daisy-chained switches"
Previous message: syzbot: "[syzbot] [ntfs?] KASAN: use-after-free Read in ntfs_read_folio"
In reply to: Christophe Leroy: "Re: [PATCH v2 2/7] drivers: misc: Add support for the Lantiq PEF2256 framer"
Next in thread: Herve Codina: "[PATCH v2 4/7] MAINTAINERS: Add the Lantiq PEF2256 driver entry"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]