linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Rich Felker <dalias@libc.org>
To: Mark Brown <broonie@kernel.org>
Cc: devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
	linux-spi@vger.kernel.org, linux-sh@vger.kernel.org,
	Rob Herring <robh+dt@kernel.org>,
	Mark Rutland <mark.rutland@arm.com>
Subject: Re: [PATCH v4 2/2] spi: add driver for J-Core SPI controller
Date: Thu, 28 Jul 2016 15:40:45 -0400	[thread overview]
Message-ID: <20160728194045.GK15995@brightrain.aerifal.cx> (raw)
In-Reply-To: <20160728191153.GL11806@sirena.org.uk>

On Thu, Jul 28, 2016 at 08:11:53PM +0100, Mark Brown wrote:
> On Sun, Apr 03, 2016 at 05:12:45AM +0000, Rich Felker wrote:
> 
> > +config SPI_JCORE
> > +	tristate "J-Core SPI Master"
> > +	depends on OF
> > +
> 
> An architecture or SoC dependency with || COMPILE_TEST would be useful
> for avoiding cluttering Kconfig for other users.  Though as this is in a
> FPGA it's perhaps likely people will pick this up for other FPGAs so
> perhaps a comment to that effect if it seems likely.

Unlike some of the other SoC hardware (interrupt controller) that's
more closely tied to the SH cpu trap behavior, the SPI master seems
like something that would be nice and easy to reuse elsewhere. I don't
feel strongly about it either way though; I can add the arch dep if
you want.

> > +static void jcore_spi_baudrate(struct jcore_spi *hw, int speed)
> > +{
> > +	if (speed == hw->speed_hz) return;
> > +	hw->speed_hz = speed;
> > +	if (speed >= hw->clock_freq/2)
> 
> Coding style, spaces around /.

OK.

> > +	return count<len ? -EREMOTEIO : 0;
> 
> Again here, and please also write this as a normal if statement so it's
> easier to read.

OK.

> > +	/* The SPI clock rate controlled via a configurable clock divider
> > +	 * which is applied to the reference clock. A 50 MHz reference is
> > +	 * most suitable for obtaining standard SPI clock rates, but some
> > +	 * designs may have a different reference clock, and the DT must
> > +	 * make the driver aware so that it can properly program the
> > +	 * requested rate. If omitted, 50 MHz is assumed. */
> > +	clock_freq = 50000000;
> > +	of_property_read_u32(node, "clock-frequency", &clock_freq);
> > +	hw->clock_freq = clock_freq;
> 
> Why are you not using the clock API for this?  Just require a clock and
> use clk_get_rate() to find out what rate it is.

I thought about that but I'm not familiar with it. I can try to figure
it out quickly and test that approach; don't see any reason it
shouldn't work. Would you insist on having full support for
enabling/disabling the clk when it's in use, or would you be happy
with treating it as a fixed clock that's always-on for now and
possibly extending it with more functionality later if there's ever
hardware where that's relevant/helpful?

> > +	pdev->dev.dma_mask = 0;
> 
> Why are we doing this?  There's no DMA code...

There will be later, but I can remove this for now if it's not needed.

> > +	dev_info(&pdev->dev, "base %p, noirq\n", hw->base);
> 
> This is just adding noise to the boot, just remove it - it's useful to
> log information we get from the silicon but things like the base address
> don't really add anything and end up cluttering (and slowing) the boot
> when everything does it.

OK.

Rich

  reply	other threads:[~2016-07-28 19:40 UTC|newest]

Thread overview: 15+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2016-07-28  6:11 [PATCH v4 0/2] J-Core SPI controller support Rich Felker
2016-04-03  5:12 ` [PATCH v4 2/2] spi: add driver for J-Core SPI controller Rich Felker
2016-07-28 19:11   ` Mark Brown
2016-07-28 19:40     ` Rich Felker [this message]
2016-07-28 19:51       ` Mark Brown
2016-07-30  3:34         ` Rich Felker
2016-08-01 18:12           ` Mark Brown
2016-08-02  1:45             ` Rich Felker
2016-08-02 18:11               ` Rob Landley
2016-08-04 13:05           ` Geert Uytterhoeven
2016-08-04 17:03             ` Rich Felker
2016-05-17 23:19 ` [PATCH v4 1/2] of: add J-Core SPI master bindings Rich Felker
2016-07-29 20:58   ` Rob Herring
2016-08-02 22:00     ` Rich Felker
2016-08-02 22:41       ` Rob Herring

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=20160728194045.GK15995@brightrain.aerifal.cx \
    --to=dalias@libc.org \
    --cc=broonie@kernel.org \
    --cc=devicetree@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-sh@vger.kernel.org \
    --cc=linux-spi@vger.kernel.org \
    --cc=mark.rutland@arm.com \
    --cc=robh+dt@kernel.org \
    /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: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).