On 21.03.19 15:25, Mark Brown wrote:
On Thu, Mar 14, 2019 at 12:52:12PM +0100, Stefan Roese wrote:
This looks pretty good, a few trivial issues below but nothing major I
think.
+config SPI_MT7621
+ tristate "MediaTek MT7621 SPI Controller"
+ depends on RALINK
+ help
+ This selects a driver for the MediaTek MT7621 SPI Controller.
+
I'm not seeing any build time dependency on this, please add an ||
COMPILE_TEST to help with build testing.
Okay, will add in v2.
@@ -0,0 +1,422 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * spi-mt7621.c -- MediaTek MT7621 SPI controller driver
+ *
Please make the entire comment a C++ one to
Will change in v2.
+ * Some parts are based on spi-orion.c:
+ * Author: Shadi Ammouri <shadi@xxxxxxxxxxx>
+ * Copyright (C) 2007-2008 Marvell Ltd.
That driver dates from 2008 AFAICT, and had some changes after then?
The spi-orion driver? I didn't check. That's what in the header of
this driver since quite some time. And I didn't want to change any
copyright notices.
BTW: spi-orion still has the same copyright message.
+static void mt7621_spi_set_cs(struct spi_device *spi, int enable)
+{
+ struct mt7621_spi *rs = spidev_to_mt7621_spi(spi);
+ int cs = spi->chip_select;
+ u32 polar = 0;
+
+ mt7621_spi_reset(rs);
+ if (enable)
+ polar = BIT(cs);
+ mt7621_spi_write(rs, MT7621_SPI_POLAR, polar);
+}
Will doing a reset mess up the rate configuration and stuff that's done
in _prepare(), or is _reset() perhaps not an entirely accurate name?
You are correct. The function name is not really matching its
implementation. Will change in v2.
+ list_for_each_entry(t, &m->transfers, transfer_list)
+ if (t->speed_hz < speed)
+ speed = t->speed_hz;
This should be in the core if it's needed, it's going to be the same for
all drivers.
I'm not sure here, other drivers might set the speed individually for
each "spi_transfer" entry. In this driver its set only once: before
beginning to transfer all entries. So should I really move this into
the core and set all speed_hz values to the minimum one of the list?
+ master->setup = mt7621_spi_setup;
+ master->transfer_one_message = mt7621_spi_transfer_one_message;
+ master->bits_per_word_mask = SPI_BPW_MASK(8);
+ master->dev.of_node = pdev->dev.of_node;
+ master->num_chipselect = 2;
The driver should set SPI_CONTROLLER_HALF_DUPLEX in flags.
Will update in v2.
+ clk_disable(rs->clk);
This needs to be clk_disable_unprepare() to ensure the clock is
unprepared as well as disabled.
Okay, will change in v2.
+ spi_unregister_master(master);
Just use devm_spi_register_controller()? The _master() name is legacy
and devm_ would make life easier.
Yes, will change in v2.
Thanks for the review.
Thanks,
Stefan
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
