From: Johan Hovold <johan@kernel.org> To: Laurentiu Palcu <laurentiu.palcu@intel.com> Cc: Mark Brown <broonie@kernel.org>, Samuel Ortiz <sameo@linux.intel.com>, Lee Jones <lee.jones@linaro.org>, Johan Havold <johan@kernel.org>, Octavian Purdila <octavian.purdila@intel.com>, linux-spi@vger.kernel.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH v4 1/2] spi: add support for DLN-2 USB-SPI adapter Date: Wed, 19 Nov 2014 13:21:11 +0100 [thread overview] Message-ID: <20141119122111.GE7857@localhost> (raw) In-Reply-To: <1416394031-2663-1-git-send-email-laurentiu.palcu@intel.com> 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
WARNING: multiple messages have this Message-ID (diff)
From: Johan Hovold <johan-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> To: Laurentiu Palcu <laurentiu.palcu-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org> Cc: Mark Brown <broonie-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>, Samuel Ortiz <sameo-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>, Lee Jones <lee.jones-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>, Johan Havold <johan-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>, Octavian Purdila <octavian.purdila-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>, linux-spi-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org Subject: Re: [PATCH v4 1/2] spi: add support for DLN-2 USB-SPI adapter Date: Wed, 19 Nov 2014 13:21:11 +0100 [thread overview] Message-ID: <20141119122111.GE7857@localhost> (raw) In-Reply-To: <1416394031-2663-1-git-send-email-laurentiu.palcu-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org> 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-spi" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html
next prev parent reply other threads:[~2014-11-19 12:21 UTC|newest] Thread overview: 48+ messages / expand[flat|nested] mbox.gz Atom feed top 2014-11-18 12:09 [PATCH v3 0/2] Add SPI support for Diolan DLN2 Laurentiu Palcu 2014-11-18 12:09 ` Laurentiu Palcu 2014-11-18 12:09 ` [PATCH v3 1/2] spi: add support for DLN-2 USB-SPI adapter Laurentiu Palcu 2014-11-18 17:06 ` Johan Hovold 2014-11-18 17:06 ` Johan Hovold 2014-11-19 9:53 ` Laurentiu Palcu 2014-11-19 10:09 ` Johan Hovold 2014-11-19 10:09 ` Johan Hovold 2014-11-19 10:47 ` [PATCH v4 " Laurentiu Palcu 2014-11-19 10:47 ` Laurentiu Palcu 2014-11-19 12:21 ` Johan Hovold [this message] 2014-11-19 12:21 ` Johan Hovold 2014-11-19 13:07 ` [PATCH v5 " Laurentiu Palcu 2014-11-19 13:07 ` Laurentiu Palcu 2014-11-19 13:51 ` Johan Hovold 2014-11-19 13:51 ` Johan Hovold 2014-11-19 14:12 ` Laurentiu Palcu 2014-11-19 14:12 ` Laurentiu Palcu 2014-11-26 10:09 ` [PATCH v6 " Laurentiu Palcu 2014-11-26 18:25 ` Mark Brown 2014-11-26 18:25 ` Mark Brown 2014-11-27 10:30 ` Laurentiu Palcu 2014-11-27 10:30 ` Laurentiu Palcu 2014-11-28 13:09 ` [PATCH v7 " Laurentiu Palcu 2014-11-28 14:10 ` Mark Brown 2014-11-28 14:57 ` Laurentiu Palcu 2014-11-28 14:57 ` Laurentiu Palcu 2014-11-28 15:14 ` Mark Brown 2014-11-28 15:14 ` Mark Brown 2014-11-28 15:31 ` Laurentiu Palcu 2014-11-28 15:31 ` Laurentiu Palcu 2014-11-28 16:01 ` Mark Brown 2014-11-28 18:13 ` Laurentiu Palcu 2014-11-28 18:13 ` Laurentiu Palcu 2014-11-28 18:35 ` Mark Brown 2014-11-28 18:35 ` Mark Brown 2014-11-28 19:27 ` Laurentiu Palcu 2014-11-28 19:27 ` Laurentiu Palcu 2014-11-28 19:46 ` Mark Brown 2014-11-28 19:46 ` Mark Brown 2014-11-28 19:21 ` Octavian Purdila 2014-11-28 19:21 ` Octavian Purdila 2014-12-05 18:34 ` Mark Brown 2014-12-05 18:34 ` Mark Brown 2014-11-18 12:09 ` [PATCH v3 2/2] mfd: dln2: add support for USB-SPI module Laurentiu Palcu 2014-11-18 12:09 ` Laurentiu Palcu 2014-11-19 16:29 ` Lee Jones 2014-11-19 16:29 ` Lee Jones
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20141119122111.GE7857@localhost \ --to=johan@kernel.org \ --cc=broonie@kernel.org \ --cc=laurentiu.palcu@intel.com \ --cc=lee.jones@linaro.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-spi@vger.kernel.org \ --cc=octavian.purdila@intel.com \ --cc=sameo@linux.intel.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.