On Fri, May 20, 2016 at 02:53:04AM +0000, Rich Felker wrote: > Signed-off-by: Rich Felker > --- > My previous post of the patch series accidentally omitted omitted > Cc'ing of subsystem maintainers for the necessary clocksource, > irqchip, and spi drivers. Please ack if this looks ok because I want > to get it merged as part of the arch/sh pull request for 4.7. This is *extremely* late for a first posting of a driver for v4.7 (you missed the list as well as the maintainers). > +static void jcore_spi_chipsel(struct spi_device *spi, bool value) > +{ > + struct jcore_spi *hw = spi_master_get_devdata(spi->master); > + > + pr_debug("%s: CS=%d\n", __func__, value); dev_dbg() > + > + hw->csReg = ( JCORE_SPI_CTRL_ACS | JCORE_SPI_CTRL_CCS | JCORE_SPI_CTRL_DCS ) > + ^ (!value << 2*spi->chip_select); Why the xor here and not an or? The coding style is also weird, a mix of extra spaces around the () and missing ones around *. I'm finding the intent of the code confusing here. > +static int jcore_spi_txrx(struct spi_master *master, struct spi_device *spi, struct spi_transfer *t) Coding style, please keep lines under 80 columns unless there's a good reason. > +#if !USE_MESSAGE_MODE > + spi_finalize_current_transfer(master); > +#endif I'm not sure what the if is about but it doesn't belong upstream, you shouldn't be open coding bits of the framework. > + /* register our spi controller */ > + err = spi_register_master(master); devm_ > +static int jcore_spi_remove(struct platform_device *dev) > +{ > + struct jcore_spi *hw = platform_get_drvdata(dev); > + struct spi_master *master = hw->master; > + > + platform_set_drvdata(dev, NULL); > + spi_master_put(master); > + return 0; > +} This can be removed entirely. > +static const struct of_device_id jcore_spi_of_match[] = { > + { .compatible = "jcore,spi2" }, > + {}, > +}; This is adding a DT binding with no binding document. All new DT bindings need to be documented. > + .owner = THIS_MODULE, > + .pm = NULL, No need to set either of these.