[PATCH] spi: mt7621: Move SPI driver out of staging
Stefan Roese
sr at denx.de
Fri Mar 22 14:02:54 UTC 2019
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 at marvell.com>
>> + * 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
More information about the devel
mailing list