Hi and thank you for your review.
On 25/07/14 06:11, Varka Bhadram wrote:
On 07/24/2014 09:52 PM, Martin Fuzzey wrote:
This driver provides two MTD devices, one for the configuration data
(FPGA bitstream) and another for the general purpose user flash.
Both behave as normal MTD devices bit the configuration one has
some extra sysfs entries to synchronize updating.
Signed-off-by: Martin Fuzzey <mfuzzey@xxxxxxxxxxx>
+#include <linux/delay.h>
+#include <linux/slab.h>
+#include <linux/module.h>
+#include <linux/mtd/mtd.h>
+#include <linux/mutex.h>
+#include <linux/of.h>
Includes in alphabetical order...
Ok done for V2
+
+static void machxo2_lock(struct machxo2 *machxo2)
+{
+ if (!mutex_trylock(&machxo2->lock)) {
+ dev_dbg(machxo2->dev, "wait for %s from %pf\n",
+ __func__, __builtin_return_address(0));
+
dev_dbg(machxo2->dev, "wait for %s from %pf\n",
__func__, __builtin_return_address(0));
Sorry, not understanding here.
You want the second line to be LESS intented??
Coding style (chapter 2) says:
Descendants are always substantially shorter than the parent and
are placed substantially to the right
Idem for the others
+static int machxo2_transceive(
+ struct machxo2 *machxo2,
+ const void *send_buf, unsigned send_len,
+ void *recv_buf, unsigned recv_len)
static int machxo2_transceive(struct machxo2 *machxo2,
const void *send_buf, unsigned send_len,
void *recv_buf, unsigned recv_len)
Ok done for V2
+static const struct spi_device_id machxo2_spi_device_id[] = {
+ {
+ .name = "machxo2",
+ }, {
+ /* sentinel */
+ }
+};
+MODULE_DEVICE_TABLE(spi, machxo2_spi_device_id);
+
+static const struct of_device_id machxo2_dt_ids[] = {
+ { .compatible = "lattice,machxo2"},
+ { /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, machxo2_dt_ids);
+
move the device ids after probe()/remove()...
Ok done for V2
+
+static int machxo2_spi_transceive(
+ struct device *dev,
+ const void *send_buf, unsigned send_len,
+ void *recv_buf, unsigned recv_len)
proper alignment...
static int machxo2_spi_transceive(struct device *dev,
const void *send_buf, unsigned send_len,
void *recv_buf, unsigned recv_len)
Ok done for V2
+static struct spi_driver machxo2_spi_driver = {
+ .id_table = machxo2_spi_device_id,
+ .driver = {
+ .name = "machxo2",
+ .owner = THIS_MODULE,
we can drop owner field...
Ok done for V2
+ .of_match_table = machxo2_dt_ids,
+ },
+ .probe = machxo2_spi_probe,
+ .remove = machxo2_spi_remove,
+};
+
+
+static int __init machxo2_init(void)
+{
+ return spi_register_driver(&machxo2_spi_driver);
+}
+subsys_initcall(machxo2_init);
+
+static void __exit machxo2_exit(void)
+{
+ spi_unregister_driver(&machxo2_spi_driver);
+}
+module_exit(machxo2_exit);
+
module_spi_driver()...?
Yes, done for V2
+struct machxo2;
+struct machxo2_busops {
+ int (*transceive)(
+ struct device *dev,
+ const void *send_buf, unsigned send_len,
+ void *recv_buf, unsigned recv_len);
+};
+
proper indentation...
int (*transceive)(struct device *dev,
const void *send_buf, unsigned send_len,
void *recv_buf, unsigned recv_len);
Ok, done for V2
+struct machxo2 *machxo2_create(struct device *dev,
+ struct machxo2_busops *busops);
+
struct machxo2 *machxo2_create(struct device *dev,
struct machxo2_busops *busops);
Same again - is it just too much indenting?
I don't see anything wrong here
This patch has coding style problems.. run checkpatch on this patch..
I did, before submission (with the 3.6.16-rc6 checkpatch).
The only thing it complained about was one Kconfig which I couldn't see
a way to improve
WARNING: please write a paragraph that describes the config symbol fully
#202: FILE: drivers/mtd/devices/Kconfig:148:
+config MTD_MACHXO2_SPI
total: 0 errors, 1 warnings, 1248 lines checked
/tmp/fpga-machxo2 has style problems, please review.
If any of these errors are false positives, please report
them to the maintainer, see CHECKPATCH in MAINTAINERS.
Regards,
Martin
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
