On Wed, Nov 19, 2014 at 12:47:11PM +0200, Laurentiu Palcu wrote:
> + /* no need to protect this buffer because all SPI communication is
> + * serialized by the SPI core
> + */
> + void *buf;
Preferred comment style is
/*
* ...
*/
Capitalisation and punctuation is also nice.
Why not mention where you use it and why (i.e. to avoid having those
large messages on the stack?
> +static int dln2_spi_get_cs_num(struct dln2_spi *dln2, u16 *cs_num)
> +{
> + int ret;
> + struct {
> + u8 port;
> + } tx;
> + struct {
> + __le16 cs_count;
> + } rx;
> + unsigned rx_len = sizeof(rx);
> +
> + if (!cs_num)
> + return -EINVAL;
Why did you add this check since v1? You only call this function from
probe with a non-null argument.
> +
> + tx.port = dln2->port;
> + ret = dln2_transfer(dln2->pdev, DLN2_SPI_GET_SS_COUNT, &tx, sizeof(tx),
> + &rx, &rx_len);
> + if (ret < 0)
> + return ret;
> + if (rx_len < sizeof(rx))
> + return -EPROTO;
> +
> + *cs_num = le16_to_cpu(rx.cs_count);
> +
> + dev_dbg(&dln2->pdev->dev, "cs_num = %d\n", *cs_num);
> +
> + return 0;
> +}
> +
> +static int dln2_spi_get_speed(struct dln2_spi *dln2, u16 cmd, u32 *freq)
> +{
> + int ret;
> + struct {
> + u8 port;
> + } tx;
> + struct {
> + __le32 speed;
> + } rx;
> + unsigned rx_len = sizeof(rx);
> +
> + if (!freq)
> + return -EINVAL;
Same here. You only call this function indirectly from probe with a
non-null argument.
> +
> + tx.port = dln2->port;
> +
> + ret = dln2_transfer(dln2->pdev, cmd, &tx, sizeof(tx), &rx, &rx_len);
> + if (ret < 0)
> + return ret;
> + if (rx_len < sizeof(rx))
> + return -EPROTO;
> +
> + *freq = le32_to_cpu(rx.speed);
> +
> + return 0;
> +}
> +
> +/*
> + * Get bus min/max frequencies.
> + */
> +static int dln2_spi_get_speed_range(struct dln2_spi *dln2, u32 *fmin, u32
> *fmax)
> +{
> + int ret;
> +
> + if (!fmin || !fmax)
> + return -EINVAL;
Not needed either.
> +
> + ret = dln2_spi_get_speed(dln2, DLN2_SPI_GET_MIN_FREQUENCY,
> + fmin);
No need to break the line.
> + if (ret < 0)
> + return ret;
> +
> + ret = dln2_spi_get_speed(dln2, DLN2_SPI_GET_MAX_FREQUENCY,
> + fmax);
Ditto.
> + if (ret < 0)
> + return ret;
> +
> + dev_dbg(&dln2->pdev->dev, "freq_min = %d, freq_max = %d\n",
> + *fmin, *fmax);
> +
> + return 0;
> +}
> +static int dln2_spi_get_supported_frame_sizes(struct dln2_spi *dln2,
> + u32 *bpw_mask)
> +{
> + int ret;
> + struct {
> + u8 port;
> + } tx;
> + struct {
> + u8 count;
> + u8 frame_sizes[36];
> + } *rx = dln2->buf;
> + unsigned rx_len = sizeof(*rx);
> + int i;
> +
> + if (!bpw_mask)
> + return -EINVAL;
Will never be NULL either.
> +
> + tx.port = dln2->port;
> +
> + ret = dln2_transfer(dln2->pdev, DLN2_SPI_GET_SUPPORTED_FRAME_SIZES,
> + &tx, sizeof(tx), rx, &rx_len);
> + if (ret < 0)
> + return ret;
> + if (rx_len != sizeof(*rx))
> + return -EPROTO;
rx_len will never be larger than the requested transfer size, so stick
to less-than comparison as you do everywhere else.
Johan
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
