On 23.09.2024 07:53:24, Hal Feng wrote:
> > > +static inline u8 ccan_read_reg_8bit(const struct ccan_priv *priv,
> > > + enum ccan_reg reg)
> > > +{
> > > + u8 reg_down;
> > > + union val {
> > > + u8 val_8[4];
> > > + u32 val_32;
> > > + } val;
> > > +
> > > + reg_down = ALIGN_DOWN(reg, 4);
> > > + val.val_32 = ccan_read_reg(priv, reg_down);
> > > + return val.val_8[reg - reg_down];
> >
> > There is an ioread8(). Is it invalid to do a byte read for this hardware? If so, it is
> > probably worth a comment.
>
> The hardware has been initially developed as peripheral component for 8 bit systems
> and therefore control and status registers defined as 8 bit groups. Nevertheless
> the hardware is designed as a 32 bit component finally. It prefers 32-bit read/write
> interfaces. I will add a comment later.
As mentioned in my v1 review, you are doing proper u32 accesses.
> > > +static int ccan_bittime_configuration(struct net_device *ndev) {
> > > + struct ccan_priv *priv = netdev_priv(ndev);
> > > + struct can_bittiming *bt = &priv->can.bittiming;
> > > + struct can_bittiming *dbt = &priv->can.data_bittiming;
> > > + u32 bittiming, data_bittiming;
> > > + u8 reset_test;
> > > +
> > > + reset_test = ccan_read_reg_8bit(priv, CCAN_CFG_STAT);
> > > +
> > > + if (!(reset_test & CCAN_RST_MASK)) {
> > > + netdev_alert(ndev, "Not in reset mode, cannot set bit
> > timing\n");
> > > + return -EPERM;
> > > + }
> >
> >
> > You don't see nedev_alert() used very often. If this is fatal then netdev_err().
> >
> > Also, EPERM? man 3 errno say:
> >
> > EPERM Operation not permitted (POSIX.1-2001).
> >
> > Why is this a permission issue?
>
> Will use netdev_err() and return -EWOULDBLOCK instead.
You have a dedicated function to put the IP core into reset
"ccan_set_reset_mode()". If you don't trust you IP core or it needs some
time, add a poll to that function and return an error.
Then there's no need on ccan_bittime_configuration() to check for reset
mode.
Marc
--
Pengutronix e.K. | Marc Kleine-Budde |
Embedded Linux | https://www.pengutronix.de |
Vertretung Nürnberg | Phone: +49-5121-206917-129 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-9 |
Attachment:
signature.asc
Description: PGP signature
