AW: [PATCH 2/5] spi: implement companion-spi driver

[Jonas Mark (BT-FIR/ENG1)]

Hello Andy,

Thank you very much for your feedback.

> > +       /*TODO: support mutiple packets in one write in future*/
> 
> Hmm... Either fix this, or remove comment?

Agreed, we will manage ideas for future changes at a different place.

> > +       if (copy_from_user(p.data, buf, sizeof(p)) == 0) {
> > +               if (is_can_type(&p))
> > +                       return -EINVAL;
> > +       } else {
> > +               dev_info(parent, "copy from user not succeed in one call\n");
> 
> Shouldn't it return immediately?

Yes, it should. Will be changed.

> 
> > +       }
> 
> > +
> > +       error = qm_io_txq_in(&priv->pm.qm, buf, count, &copied);
> 
> ...what the point to call if we got garbage from user space?

Will be changed with the code above.

> > +       if (!error) {
> 
> The usual pattern is to check for errors first.

Understood, will be changed.

> > +               wake_up_interruptible(&priv->wait);
> > +               priv->pm.stats.io_tx++;
> > +               return copied;
> > +       } else {
> > +               priv->pm.stats.io_tx_overflows++;
> > +       }
> > +       return error;
> > +}
> 
> > +       ... qm_io_rxq_out(&priv->pm.qm, buf, count, &copied);
> 
> > +       ... pm_can_data_tx(&priv->pm, port, prio, cf);
> 
> Oh, oh, oh.
> 
> These namespaces are too generic, moreover pm is kinda occupied by
> power management. You bring a lot of confusion here, I think.

We agree and we started thinking about better names. We will send a proposal.

> > +       err = pm_can_get_txq_status(pm, port);
> > +       if (!err) {
> 
> if (err)
>  return err;

Yes, will be changed.

> > +       }
> > +       return err;
> 
> 
> > +       int                        ret, value;
> > +
> > +       ret = sscanf(buf, "%d", &value);
> > +       if (ret != 1) {
> 
> > +       }
> 
> You have to be consistent in your code.
> 
> I've already noticed
> 
> err
> error
> 
> and now
> 
> ret
> 
> Choose one and stick with it.

Yes, will be changed.

> Also check your entire patch series' code for consistency.

We will have a look.

> > +static DEVICE_ATTR(dump_packets, S_IRUGO | S_IWUSR,
> > +                   show_dump_packets, store_dump_packets);
> 
> We have helpers, like DEVICE_ATTR_RW().

Will be changed.

> > +               ret  = snprintf(buf + pos, PAGE_SIZE - pos,
> > +                               "\ntx: %u, rx: %u, err: %u\n\n",
> > +                               total,
> > +                               priv->pm.stats.can_rx_overflows[i],
> > +                               priv->pm.stats.can_err_overflows[i]);
> 
> Please, avoid leading '\n'.

We think we will stick to the existing. Otherweise we would have to
insert another pair of sprint() and pos += ret. Is it worth that?

> 
> > +       gpio_set_value(priv->cs_gpios, priv->cs_gpios_assert);
> 
> > +       gpio_set_value(priv->cs_gpios, !priv->cs_gpios_assert);
> 
> Can you switch to GPIO descriptor API?

Yes, we are working on it.

> > +       unsigned int count = READY_POLL_US / READY_POLL_US_GRAN;
> > +       while (count--) {
> 
> For counting like this better to have
> do {
> } while (--count);
> 
> The rationale, reader at first glance will know that the loop will
> iterate at least once.

Agreed, will be changed.

> > +               if (slave_is_not_busy(priv))
> > +                       return 0;
> > +
> 
> > +               udelay(READY_POLL_US_GRAN);
> 
> Should it be atomic?
> Can it use read_poll_* type of helpers instead?

Yes, it shall be atomic because we need to reduce the latency at
detecting the de-assertion of the busy signal. We accept that this will
cost CPU time.

In case the Companion itself is very busy and does not reply quickly we
are have second polling loop below which sleeps longer and is non-atomic.

> Above comments applicable to entire code you have.

We will look at it.

> > +static void companion_spi_cpu_to_be32(char *buf)
> > +{
> > +       u32 *buf32 = (u32*)buf;
> > +       int  i;
> > +
> > +       for (i = 0; i < (BCP_PACKET_SIZE / sizeof(u32)); i++, buf32++)
> > +               *buf32 = cpu_to_be32(*buf32);
> > +}
> 
> This entire function should be replaced by one of the helpers from
> include/linux/byteorder/generic.h
> 
> I guess cpu_to_be32_array() is a right one.

Correct. We are testing the driver against 4.14 and that function is not
available there. It will be changed later.

> > +static void companion_spi_be32_to_cpu(char *buf)
> > +{
> > +       u32 *buf32 = (u32*)buf;
> > +       int  i;
> > +
> > +       for (i = 0; i < (BCP_PACKET_SIZE / sizeof(u32)); i++, buf32++)
> > +               *buf32 = be32_to_cpu(*buf32);
> > +}
> 
> Ditto.
> 
> Recommendation: check your code against existing helpers.

Yes, every kernel release brings new helpers. We will have to learn how
to keep track of the additions.

> > +               p = (const struct companion_packet*)transfer->tx_buf;
> 
> > +       companion_spi_cpu_to_be32((char*)transfer->tx_buf);
> 
> If tx_buf is type of void * all these castings are redundant.

The type is const void*. So the first cast is superfluous, the second
is not.

> Also looking at the function, did you consider to use whatever SPI
> core provides, like CS handling, messages handling and so on?

SPI CS has to be done manually in our case because we need to wait
until the Companion signals that it is ready for the transfer.

Do you have concrete proposals regarding messages handling?

> > +static int companion_spi_thread(void *data)
> > +{
> > +       struct companion_spi_priv *priv = data;
> > +       struct companion_packet    tx_packet;
> > +       struct companion_packet    rx_packet;
> > +       struct spi_message         message;
> > +       struct spi_transfer        transfer;
> > +
> > +       memset(&transfer, 0, sizeof(transfer));
> > +       transfer.tx_buf        = tx_packet.data;
> > +       transfer.rx_buf        = rx_packet.data;
> > +       transfer.len           = sizeof(struct companion_packet);
> 
> > +       transfer.cs_change     = 0;
> 
> Redundant.

Yes, it is redundant. We want to explicitly show here that the CS
handling is done manually.

> > +       for (;;) {
> 
> Infinite loops are evil in most of the cases.
> I see here
> 
> do {
> } while (kthread_should_stop());

Yes, will be fixed.

> > +static const struct of_device_id companion_spi_of_match[] = {
> > +       { .compatible = DRIVER_NAME, .data = NULL, },
> > +       { /* sentinel */ },
> 
> terminators better without commas.

Yes, will be fixed.

> > +               .owner          = THIS_MODULE,
> 
> This is redundant, macro you are using below sets that.

Will be fixed.

> Something wrong with indentation in this file.

Yes, we will need to work on the indentation. We will do it in a
following round.

> > +#define CHECK_SIZE(x) BUILD_BUG_ON(sizeof(struct companion_packet) != \
> > +                                   sizeof(x))
> 
> Better to split like
>  _SIZE(x) \
> BUILD_BUG_ON()

Will be fixed.

> > +void pm_init(struct companion_protocol_manager *pm)
> 
> Unfortunately, horrible name for the function.
> Namespace is kinda occupied, name itself way too generic.

Yes, we will send a proposal.

> > +       if (ctrl & CAN_CTRLMODE_LISTENONLY)
> > +               p.mode = BCP_CAN_MODE_LISTEN;
> > +       else
> > +               return -EOPNOTSUPP;
> 
> if (!(cond))
>  return -ERRNO;

Will be fixed.

> > + * This program is free software; you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License version 2 as
> > + * published by the Free Software Foundation.
> 
> Do you still need this text?

Do you mean the SPDX is enough? Then, yes, we can remove it.

> > + * TODO: add more statistics fields and export to sysfs
> 
> Do it or remove the comment?
> 
> > + * TODO: re-think the data structure for handle CAN response
> 
> Ditto.

We will handle future improvement ideas in a different place.

> > +/**
> > + * BCP status field definitions
> > + */
> > +#define BCP_STATUS_SUCCESS 0x00u
> > +#define BCP_STATUS_UNKNOWN 0x01u
> > +#define BCP_STATUS_OTHER   0x02u
> 
> BIT() ?

No, these are fixed numbers, not bit fields.

> > +struct companion_packet {
> > +       __u8 data[BCP_PACKET_SIZE];
> > +};
> 
> Is it going from / to user space? Otherwise why __ kind of type?

Will be fixed.

> > +#define CAN_MAX_IN_A_ROW 8
> > +
> > +
> > +
> 
> Too many blank lines.

Yes. Will be fixed.

> > +int companion_do_can_err(struct device           *parent,
> > +                         u8                       port,
> > +                         struct can_berr_counter *bec,
> > +                         u8                      *state,
> > +                         u8                      *code);
> 
> + blank line here.

Will be fixed.

> 
> > +#define COMPANION_CAN_STATE_WARNING 0x01u
> > +#define COMPANION_CAN_STATE_PASSIVE 0x02u
> > +#define COMPANION_CAN_STATE_BUS_OFF 0x04u
> > +#define COMPANION_CAN_ERROR_STUFF   0x01u
> > +#define COMPANION_CAN_ERROR_FORM    0x02u
> > +#define COMPANION_CAN_ERROR_ACK     0x04u
> > +#define COMPANION_CAN_ERROR_BIT1    0x08u
> > +#define COMPANION_CAN_ERROR_BIT0    0x10u
> > +#define COMPANION_CAN_ERROR_CRC     0x20u
> > +#define COMPANION_CAN_ERROR_RXOV    0x80u
> 
> BIT() ?

Yes, this is a bit field. Will be fixed.

Greetings,
Mark

Building Technologies, Panel Software Fire (BT-FIR/ENG1) 
Bosch Sicherheitssysteme GmbH | Postfach 11 11 | 85626 Grasbrunn | GERMANY | www.boschsecurity.com

Sitz: Stuttgart, Registergericht: Amtsgericht Stuttgart HRB 23118 
Aufsichtsratsvorsitzender: Stefan Hartung; GeschÃftsfÃhrung: Gert van Iperen, Andreas Bartz, Thomas Quante, Bernhard Schuster