| Message ID | 20220704075032.383700-4-biju.das.jz@bp.renesas.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Add support for RZ/N1 SJA1000 CAN controller | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Guessed tree name to be net-next |
| netdev/fixes_present | success | Fixes tag not required for -next series |
| netdev/subject_prefix | success | Link |
| netdev/cover_letter | success | Series has a cover letter |
| netdev/patch_count | success | Link |
| netdev/header_inline | success | No static functions without inline keyword in header files |
| netdev/build_32bit | fail | Errors and warnings before: 5 this patch: 5 |
| netdev/cc_maintainers | success | CCed 12 of 12 maintainers |
| netdev/build_clang | success | Errors and warnings before: 0 this patch: 0 |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Signed-off-by tag matches author and committer |
| netdev/check_selftest | success | No net selftest shell script |
| netdev/verify_fixes | success | No Fixes tag |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 0 this patch: 0 |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 34 lines checked |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/source_inline | success | Was 0 now: 0 |
Hi Biju, I gave a quick look to your series. Nothing odd to me, I just have a single nitpick. On Mon. 4 juil. 2022 at 16:51, Biju Das <biju.das.jz@bp.renesas.com> wrote: > As per Chapter 6.5.16 of the RZ/N1 Peripheral Manual, The SJA1000 > CAN controller does not support Clock Divider Register compared to > the reference Philips SJA1000 device. > > This patch adds a device quirk to handle this difference. > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> > --- > v2->v3: > * No Change > v1->v2: > * Updated commit description > * Removed the quirk macro SJA1000_NO_HW_LOOPBACK_QUIRK > * Added prefix SJA1000_QUIRK_* for quirk macro. > --- > drivers/net/can/sja1000/sja1000.c | 13 ++++++++----- > drivers/net/can/sja1000/sja1000.h | 3 ++- > 2 files changed, 10 insertions(+), 6 deletions(-) > > diff --git a/drivers/net/can/sja1000/sja1000.c b/drivers/net/can/sja1000/sja1000.c > index 2e7638f98cf1..e9c55f5aa3c3 100644 > --- a/drivers/net/can/sja1000/sja1000.c > +++ b/drivers/net/can/sja1000/sja1000.c > @@ -183,8 +183,9 @@ static void chipset_init(struct net_device *dev) > { > struct sja1000_priv *priv = netdev_priv(dev); > > - /* set clock divider and output control register */ > - priv->write_reg(priv, SJA1000_CDR, priv->cdr | CDR_PELICAN); > + if (!(priv->flags & SJA1000_QUIRK_NO_CDR_REG)) > + /* set clock divider and output control register */ > + priv->write_reg(priv, SJA1000_CDR, priv->cdr | CDR_PELICAN); > > /* set acceptance filter (accept all) */ > priv->write_reg(priv, SJA1000_ACCC0, 0x00); > @@ -208,9 +209,11 @@ static void sja1000_start(struct net_device *dev) > if (priv->can.state != CAN_STATE_STOPPED) > set_reset_mode(dev); > > - /* Initialize chip if uninitialized at this stage */ > - if (!(priv->read_reg(priv, SJA1000_CDR) & CDR_PELICAN)) > - chipset_init(dev); > + if (!(priv->flags & SJA1000_QUIRK_NO_CDR_REG)) { > + /* Initialize chip if uninitialized at this stage */ > + if (!(priv->read_reg(priv, SJA1000_CDR) & CDR_PELICAN)) You can combine the two if in one: | /* Initialize chip if uninitialized at this stage */ | if (!(priv->flags & SJA1000_QUIRK_NO_CDR_REG || | priv->read_reg(priv, SJA1000_CDR) & CDR_PELICAN)) | chipset_init(dev); > + chipset_init(dev); > + } > > /* Clear error counters and error code capture */ > priv->write_reg(priv, SJA1000_TXERR, 0x0); > diff --git a/drivers/net/can/sja1000/sja1000.h b/drivers/net/can/sja1000/sja1000.h > index 9d46398f8154..7f736f1df547 100644 > --- a/drivers/net/can/sja1000/sja1000.h > +++ b/drivers/net/can/sja1000/sja1000.h > @@ -145,7 +145,8 @@ > /* > * Flags for sja1000priv.flags > */ > -#define SJA1000_CUSTOM_IRQ_HANDLER 0x1 > +#define SJA1000_CUSTOM_IRQ_HANDLER BIT(0) > +#define SJA1000_QUIRK_NO_CDR_REG BIT(1) > > /* > * SJA1000 private data structure
Hi Vincent, Thanks for the feedback. > Subject: Re: [PATCH v3 3/6] can: sja1000: Add Quirk for RZ/N1 SJA1000 CAN > controller > > Hi Biju, > > I gave a quick look to your series. Nothing odd to me, I just have a > single nitpick. > > On Mon. 4 juil. 2022 at 16:51, Biju Das <biju.das.jz@bp.renesas.com> > wrote: > > As per Chapter 6.5.16 of the RZ/N1 Peripheral Manual, The SJA1000 CAN > > controller does not support Clock Divider Register compared to the > > reference Philips SJA1000 device. > > > > This patch adds a device quirk to handle this difference. > > > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> > > --- > > v2->v3: > > * No Change > > v1->v2: > > * Updated commit description > > * Removed the quirk macro SJA1000_NO_HW_LOOPBACK_QUIRK > > * Added prefix SJA1000_QUIRK_* for quirk macro. > > --- > > drivers/net/can/sja1000/sja1000.c | 13 ++++++++----- > > drivers/net/can/sja1000/sja1000.h | 3 ++- > > 2 files changed, 10 insertions(+), 6 deletions(-) > > > > diff --git a/drivers/net/can/sja1000/sja1000.c > > b/drivers/net/can/sja1000/sja1000.c > > index 2e7638f98cf1..e9c55f5aa3c3 100644 > > --- a/drivers/net/can/sja1000/sja1000.c > > +++ b/drivers/net/can/sja1000/sja1000.c > > @@ -183,8 +183,9 @@ static void chipset_init(struct net_device *dev) > > { > > struct sja1000_priv *priv = netdev_priv(dev); > > > > - /* set clock divider and output control register */ > > - priv->write_reg(priv, SJA1000_CDR, priv->cdr | CDR_PELICAN); > > + if (!(priv->flags & SJA1000_QUIRK_NO_CDR_REG)) > > + /* set clock divider and output control register */ > > + priv->write_reg(priv, SJA1000_CDR, priv->cdr | > > + CDR_PELICAN); > > > > /* set acceptance filter (accept all) */ > > priv->write_reg(priv, SJA1000_ACCC0, 0x00); @@ -208,9 +209,11 > > @@ static void sja1000_start(struct net_device *dev) > > if (priv->can.state != CAN_STATE_STOPPED) > > set_reset_mode(dev); > > > > - /* Initialize chip if uninitialized at this stage */ > > - if (!(priv->read_reg(priv, SJA1000_CDR) & CDR_PELICAN)) > > - chipset_init(dev); > > + if (!(priv->flags & SJA1000_QUIRK_NO_CDR_REG)) { > > + /* Initialize chip if uninitialized at this stage */ > > + if (!(priv->read_reg(priv, SJA1000_CDR) & > > + CDR_PELICAN)) > > You can combine the two if in one: > > | /* Initialize chip if uninitialized at this stage */ > | if (!(priv->flags & SJA1000_QUIRK_NO_CDR_REG || > | priv->read_reg(priv, SJA1000_CDR) & CDR_PELICAN)) > | chipset_init(dev); > > > + chipset_init(dev); > > + } OK, will fix this in next version. Cheers, Biju > > > > /* Clear error counters and error code capture */ > > priv->write_reg(priv, SJA1000_TXERR, 0x0); diff --git > > a/drivers/net/can/sja1000/sja1000.h > > b/drivers/net/can/sja1000/sja1000.h > > index 9d46398f8154..7f736f1df547 100644 > > --- a/drivers/net/can/sja1000/sja1000.h > > +++ b/drivers/net/can/sja1000/sja1000.h > > @@ -145,7 +145,8 @@ > > /* > > * Flags for sja1000priv.flags > > */ > > -#define SJA1000_CUSTOM_IRQ_HANDLER 0x1 > > +#define SJA1000_CUSTOM_IRQ_HANDLER BIT(0) > > +#define SJA1000_QUIRK_NO_CDR_REG BIT(1) > > > > /* > > * SJA1000 private data structure
diff --git a/drivers/net/can/sja1000/sja1000.c b/drivers/net/can/sja1000/sja1000.c index 2e7638f98cf1..e9c55f5aa3c3 100644 --- a/drivers/net/can/sja1000/sja1000.c +++ b/drivers/net/can/sja1000/sja1000.c @@ -183,8 +183,9 @@ static void chipset_init(struct net_device *dev) { struct sja1000_priv *priv = netdev_priv(dev); - /* set clock divider and output control register */ - priv->write_reg(priv, SJA1000_CDR, priv->cdr | CDR_PELICAN); + if (!(priv->flags & SJA1000_QUIRK_NO_CDR_REG)) + /* set clock divider and output control register */ + priv->write_reg(priv, SJA1000_CDR, priv->cdr | CDR_PELICAN); /* set acceptance filter (accept all) */ priv->write_reg(priv, SJA1000_ACCC0, 0x00); @@ -208,9 +209,11 @@ static void sja1000_start(struct net_device *dev) if (priv->can.state != CAN_STATE_STOPPED) set_reset_mode(dev); - /* Initialize chip if uninitialized at this stage */ - if (!(priv->read_reg(priv, SJA1000_CDR) & CDR_PELICAN)) - chipset_init(dev); + if (!(priv->flags & SJA1000_QUIRK_NO_CDR_REG)) { + /* Initialize chip if uninitialized at this stage */ + if (!(priv->read_reg(priv, SJA1000_CDR) & CDR_PELICAN)) + chipset_init(dev); + } /* Clear error counters and error code capture */ priv->write_reg(priv, SJA1000_TXERR, 0x0); diff --git a/drivers/net/can/sja1000/sja1000.h b/drivers/net/can/sja1000/sja1000.h index 9d46398f8154..7f736f1df547 100644 --- a/drivers/net/can/sja1000/sja1000.h +++ b/drivers/net/can/sja1000/sja1000.h @@ -145,7 +145,8 @@ /* * Flags for sja1000priv.flags */ -#define SJA1000_CUSTOM_IRQ_HANDLER 0x1 +#define SJA1000_CUSTOM_IRQ_HANDLER BIT(0) +#define SJA1000_QUIRK_NO_CDR_REG BIT(1) /* * SJA1000 private data structure
As per Chapter 6.5.16 of the RZ/N1 Peripheral Manual, The SJA1000 CAN controller does not support Clock Divider Register compared to the reference Philips SJA1000 device. This patch adds a device quirk to handle this difference. Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> --- v2->v3: * No Change v1->v2: * Updated commit description * Removed the quirk macro SJA1000_NO_HW_LOOPBACK_QUIRK * Added prefix SJA1000_QUIRK_* for quirk macro. --- drivers/net/can/sja1000/sja1000.c | 13 ++++++++----- drivers/net/can/sja1000/sja1000.h | 3 ++- 2 files changed, 10 insertions(+), 6 deletions(-)