Re: [PATCH v5 0/5] Add memory mapped read support for ti-qspi

[Mark Brown]

On Tue, Jan 05, 2016 at 10:50:42AM +0530, Vignesh R wrote:
> Hi Brian,
> 
> On 12/11/2015 09:39 AM, Vignesh R wrote:
> > Changes since v4:
> > Use syscon to access system control module register in ti-qspi driver.
> > 
> 
> Gentle ping...
> Are you ok with MTD side changes of this patch series?

Please don't send content free pings and please allow a reasonable time
for review (we've just had the Christmas vacation...).


signature.asc
Description: PGP signature

Re: [PATCH 4/6] regulator: lp872x: Add enable GPIO pin support

[Mark Brown]

On Thu, Dec 31, 2015 at 10:59:06PM +0100, Paul Kocialkowski wrote:

> I understand, thanks for pointing this out. Well, for my use case, there
> is no use in disabling the chip at any point as it powers the external
> mmc.

Presumably someone might decide not to use the MMC in some case (perhaps
only mounting it when explicitly needed in order to save power for
example, or the MMC subsystem might figure out a way to power down an
idle MMC block device).

> Would you agree to have the enable pin handled directly (and by that, I
> mean enabled once, when requested, as I first suggested in the patchset)
> in the driver then?

That's probably fine, or do it via runtime PM (the framework is fairly
simple to use, I'll probably go add support in the core for it in the
next day or two as this seems like a sensible use case).  I can't
remember if this device is a MFD or not and I'm just on my way out the
door.


signature.asc
Description: PGP signature

Re: [PATCH 4/6] regulator: lp872x: Add enable GPIO pin support

[Mark Brown]

On Wed, Dec 30, 2015 at 07:37:19PM +0100, Paul Kocialkowski wrote:
> Le mercredi 30 décembre 2015 à 16:33 +0000, Mark Brown a écrit :
> > On Wed, Dec 30, 2015 at 09:35:21AM +0100, Paul Kocialkowski wrote:

> > > In my opinion, it would be more elegant to adapt the core regulator
> > > framework to first enable the GPIO and then call the regulator enable
> > > ops callback instead of handling the GPIO in the driver.

> > Why would we want to actively manage both things at runtime?  It's more
> > work, what do we gain from it?

> Well, I figured that it would be best to disable the EN pin when we're
> not using any of the regulators, since that allows the chip to enter
> standby mode (and thus consume less power).

This doesn't sound like it's anything to do with the regulators, that's
a chip wide power management function which should be implemented via
runtime PM if there's any value in implementing it at all (if the device
is a primary PMIC normally this would be handled by the CPU core when it
enters low power state without any software).  It's not something we
should be considering on a per regulator basis since it's at the chip
level and on a per regulator basis it's not doing anything useful for
the reasons above.

> It also doesn't hurt regulators that only use a GPIO for enable.

It causes problems for any device with an optional GPIO,  it means that
we end up mantaining both GPIO and register which as I've said a couple
of times now defeats the point of having the GPIO.


signature.asc
Description: PGP signature

Re: [PATCH 4/6] regulator: lp872x: Add enable GPIO pin support

[Mark Brown]

On Wed, Dec 30, 2015 at 09:35:21AM +0100, Paul Kocialkowski wrote:

> In my opinion, it would be more elegant to adapt the core regulator
> framework to first enable the GPIO and then call the regulator enable
> ops callback instead of handling the GPIO in the driver.

Why would we want to actively manage both things at runtime?  It's more
work, what do we gain from it?


signature.asc
Description: PGP signature

Re: [PATCH 4/6] regulator: lp872x: Add enable GPIO pin support

[Mark Brown]

On Thu, Dec 24, 2015 at 07:12:53PM +0100, Paul Kocialkowski wrote:
> Le mercredi 23 décembre 2015 à 11:56 +0000, Mark Brown a écrit :

> > This isn't really adding support for the enable GPIO as the changelog
> > suggests, it's requesting but not managing the GPIO.  Since there is
> > core support for manging enable GPIOs this seems especially silly,
> > please tell the core about the GPIO and then it will work at runtime
> > too.

> It looks like the core bindings for GPIO can only be used instead of the
> rdev->desc->ops->enable callback, not jointly, which doesn't fit my use
> case, where both the GPIO and register write have to be used to enable
> regulators.

> I think it would be worth making it possible to use both in core, since
> that situation is probably shared with other regulators. I suggest the
> following diff (that would be split into a separate patch in v2 of this
> series):

No, that's broken - the whole point with using a GPIO for enable control
on a lot of devices is that it is much faster than doing a register
write.  What I would expect to happen in this case is that when
initialsing the GPIO we set the register to enabled and then only manage
the GPIO at runtime.


signature.asc
Description: PGP signature

Re: [PATCH 3/6] regulator: lp872x: Remove warning about invalid DVS GPIO

[Mark Brown]

On Wed, Dec 23, 2015 at 11:58:36AM +0100, Paul Kocialkowski wrote:
> Some devices don't hook the DVS pin to a GPIO but to ground or VCC.
> In those cases, it is not a problem to have no DVS GPIO.

I would expect the driver at least needs to know how the pins or
strapped, or otherwise have configuration for ignoring the input on the
pins.  This just deletes the warnings, it doesn't have any handling for
this case that I can see.


signature.asc
Description: PGP signature

Re: [PATCH 4/6] regulator: lp872x: Add enable GPIO pin support

[Mark Brown]

On Wed, Dec 23, 2015 at 11:58:37AM +0100, Paul Kocialkowski wrote:

> + gpio = lp->pdata->enable_gpio;
> + if (!gpio_is_valid(gpio))
> + return 0;
> +
> + /* Always set enable GPIO high. */
> + ret = devm_gpio_request_one(lp->dev, gpio, GPIOF_OUT_INIT_HIGH, "LP872X 
> EN");
> + if (ret) {
> + dev_err(lp->dev, "gpio request err: %d\n", ret);
> + return ret;
> + }

This isn't really adding support for the enable GPIO as the changelog
suggests, it's requesting but not managing the GPIO.  Since there is
core support for manging enable GPIOs this seems especially silly,
please tell the core about the GPIO and then it will work at runtime
too.


signature.asc
Description: PGP signature

Re: [PATCH 3/6] regulator: lp872x: Remove warning about invalid DVS GPIO

[Mark Brown]

On Wed, Dec 23, 2015 at 12:50:20PM +0100, Paul Kocialkowski wrote:
> Le mercredi 23 décembre 2015 à 11:41 +0000, Mark Brown a écrit :

> > I would expect the driver at least needs to know how the pins or
> > strapped, or otherwise have configuration for ignoring the input on the
> > pins.  This just deletes the warnings, it doesn't have any handling for
> > this case that I can see.

> When the DVS GPIO is invalid, lp872x_init_dvs jumps to the
> set_default_dvs_mode label, which instructs the chip not to use the DVS
> pin at all and do it all in software instead (by clearing the
> LP8720_EXT_DVS_M bit in the LP872X_GENERAL_CFG register).

> That is reflected later in the code, when setting the bucks (the DVS pin
> only applies to the bucks) by checking for the LP8720_EXT_DVS_M bit on
> the LP872X_GENERAL_CFG register (in lp872x_select_buck_vout_addr) to
> decide whether to use software or hardware DVS selection.

> As far as I understood, everything goes well when the GPIO is invalid.

So please put this analysis in the changelog so that the changelog
accurately reflects what the change is doing.


signature.asc
Description: PGP signature

Re: [PATCH v4 1/5] spi: introduce accelerated read support for spi flash devices

[Mark Brown]

On Mon, Nov 30, 2015 at 10:45:11AM +0530, Vignesh R wrote:
> In addition to providing direct access to SPI bus, some spi controller
> hardwares (like ti-qspi) provide special port (like memory mapped port)
> that are optimized to improve SPI flash read performance.

I'm reasonably OK with this from the SPI side but I'd really like to see
people working on MTD say that this makes sense.


signature.asc
Description: PGP signature

Re: [PATCH v2 1/5] spi: introduce mmap read support for spi flash devices

[Mark Brown]

On Thu, Nov 05, 2015 at 05:59:36PM +0530, Vignesh R wrote:
> On 11/04/2015 08:09 PM, Mark Brown wrote:

> > It's a bit worrying that this doesn't sync with the message queue except
> > via the mutex: this means that we might be out of order with respect to
> > any asynchronous transfers that are happening on the device.  I'm not
> > sure that this is a practical problem, though there is some risk of
> > unfair scheduling that would have to be under extreme load and it might
> > make sense to prioritise reads anyway.

> Since mmap interface is used only by mtd flash drivers and since almost
> all mtd flash devices use synchronous transfers (spi_sync()), IMO, there
> wont be out of order problem wrt mtd flashes.
> But mmap read might delay transfers queued for non mtd flash devices. It
> is difficult to wait for all transfers already queued to be pumped out
> by __spi_pump_messages() and then do the mmap transfer. Do you have any
> thoughts on how to sync with message queue?

Like I say I think it probably doesn't matter.  The main issue would be
with other devices sharing the bus with a flash chip.


signature.asc
Description: PGP signature

Re: [PATCH v2 2/5] spi: spi-ti-qspi: add mmap mode read support

[Mark Brown]

On Tue, Nov 03, 2015 at 03:36:11PM +0530, Vignesh R wrote:

> + ti_qspi_enable_memory_map(spi);
> + ti_qspi_setup_mmap_read(spi, read_opcode, addr_width,
> + dummy_bytes);
> + memcpy_fromio(buf, qspi->mmap_base + from, len);
> + *retlen = len;
> + ti_qspi_disable_memory_map(spi);

We'll be constantly enabling and disabling memory mapping with this.
I'm not sure that's a meaningful cost given that it doesn't actually
remap anything but rather just switches hardware modes, we can always
optimise it later if it is.


signature.asc
Description: PGP signature

Re: [PATCH v2 1/5] spi: introduce mmap read support for spi flash devices

[Mark Brown]

On Tue, Nov 03, 2015 at 03:36:10PM +0530, Vignesh R wrote:

> + }
> + mutex_lock(>mmap_lock_mutex);
> + ret = master->spi_mtd_mmap_read(spi, from, len, retlen, buf,
> + read_opcode, addr_width,
> + dummy_bytes);
> + mutex_unlock(>mmap_lock_mutex);
> + if (master->auto_runtime_pm)
> + pm_runtime_put(master->dev.parent);

It's a bit worrying that this doesn't sync with the message queue except
via the mutex: this means that we might be out of order with respect to
any asynchronous transfers that are happening on the device.  I'm not
sure that this is a practical problem, though there is some risk of
unfair scheduling that would have to be under extreme load and it might
make sense to prioritise reads anyway.


signature.asc
Description: PGP signature

Re: [PATCH] regulator: tps65217: remove tps65217.dtsi file

[Mark Brown]

On Mon, Oct 26, 2015 at 10:13:55AM +0100, Heiko Schocher wrote:
> remove tps65217.dtsi and adapt all boards, which
> used it.

Acked-by: Mark Brown <broo...@kernel.org>

but really this is a DTS change so I'd expect it to go via arm-soc
rather than me.


signature.asc
Description: PGP signature

Re: [PATCH RFC] ASoC: simple-card: Update clocks binding for simple-card DAI subnodes

[Mark Brown]

On Mon, Sep 28, 2015 at 09:49:35PM +0300, Jyri Sarha wrote:
> On 09/19/15 21:42, Mark Brown wrote:

> >What's the use case again?  Can we address this by converting the
> >relevant drivers to the clock API (or improving their clock API
> >support)?

> Sorry, I forgot to reply this earlier. The reason why we need this is the
> way McASP driver uses and provides clocks for different purposes. The most
> pressing need is to be able to select if we want to use some external clock
> pin as an input for McASP clock divider that produces the i2s bit-clock or
> if we want to use McASP's internal clock source.

> There are several other things this binding would allow us, and others with
> flexible i2s HW, to do. Some TI codecs would also benefit from a flexible
> way of describing the used clock configuration, but Peter know that part
> better.

> I tried to make the binding as flexible and generic as possible. But I do
> not currently see any immediate need for more than one set_sysclk() call per
> dai. I just did not see any reason to not allow it either.

This explains why you want to do this but what about the clock API
portion of the question - it would be good to move the ASoC clocking
more into the clock API, this would help integration with wider clock
trees.


signature.asc
Description: Digital signature

Re: [PATCH RFC v4 3/8] ASoC: hdmi-codec: Add hdmi-codec for external HDMI-encoders

[Mark Brown]

On Mon, Sep 21, 2015 at 04:41:20PM +0300, Jyri Sarha wrote:
> On 09/21/15 12:31, Russell King - ARM Linux wrote:

> >The device may accept 32 bit I2S, but it would have to be truncated to
> >24 bit before transmitting it to the sink.  This should be mentioned
> >somewhere.

> There is ".sig_bits = 24" in hdmi_i2s_dai, but I can add an explicit comment
> about it.

> We needed 32bit format in practice until Peter got 24_LE properly working
> with McASP. I just thought the may still be some platforms out there that
> can not produce 24bit i2s samples for some reason, but work just fine with
> 32bit samples. Those platforms would be limited to less than 24bit precision
> without 32bit formats. But as said, it is not critical to us any more and I
> can drop the 32bit formats as well.

Yes, this is currently a bit messy - we should really be able to allow
S24_LE and S32 to interoperate better without drivers having to know
about it since physically they are essentially compatible in memory.
I'm pretty much OK with it for now since even with that framework
support it should be harmless.


signature.asc
Description: Digital signature

Re: [PATCH RFC v4 2/8] ALSA: pcm: add IEC958 channel status helper for hw_params

[Mark Brown]

On Fri, Sep 18, 2015 at 02:06:39PM +0300, Jyri Sarha wrote:
> Add IEC958 channel status helper that gets the audio properties from
> snd_pcm_hw_params instead of snd_pcm_runtime. This is needed to
> produce the channel status bits already in audio stream configuration
> phase.
> 
> Signed-off-by: Jyri Sarha 
> ---
>  include/sound/pcm_iec958.h |  2 ++
>  sound/core/pcm_iec958.c| 52 
> +++---

This is core ALSA code, you need to CC Takashi (added him, not cutting
context).

>  2 files changed, 37 insertions(+), 17 deletions(-)
> 
> diff --git a/include/sound/pcm_iec958.h b/include/sound/pcm_iec958.h
> index 0eed397..36f023a 100644
> --- a/include/sound/pcm_iec958.h
> +++ b/include/sound/pcm_iec958.h
> @@ -6,4 +6,6 @@
>  int snd_pcm_create_iec958_consumer(struct snd_pcm_runtime *runtime, u8 *cs,
>   size_t len);
>  
> +int snd_pcm_create_iec958_consumer_hw_params(struct snd_pcm_hw_params 
> *params,
> +  u8 *cs, size_t len);
>  #endif
> diff --git a/sound/core/pcm_iec958.c b/sound/core/pcm_iec958.c
> index 36b2d7a..c9f8b66 100644
> --- a/sound/core/pcm_iec958.c
> +++ b/sound/core/pcm_iec958.c
> @@ -9,30 +9,18 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  #include 
>  
> -/**
> - * snd_pcm_create_iec958_consumer - create consumer format IEC958 channel 
> status
> - * @runtime: pcm runtime structure with ->rate filled in
> - * @cs: channel status buffer, at least four bytes
> - * @len: length of channel status buffer
> - *
> - * Create the consumer format channel status data in @cs of maximum size
> - * @len corresponding to the parameters of the PCM runtime @runtime.
> - *
> - * Drivers may wish to tweak the contents of the buffer after creation.
> - *
> - * Returns: length of buffer, or negative error code if something failed.
> - */
> -int snd_pcm_create_iec958_consumer(struct snd_pcm_runtime *runtime, u8 *cs,
> - size_t len)
> +static int create_iec958_consumer(uint rate, uint sample_width,
> +   u8 *cs, size_t len)
>  {
>   unsigned int fs, ws;
>  
>   if (len < 4)
>   return -EINVAL;
>  
> - switch (runtime->rate) {
> + switch (rate) {
>   case 32000:
>   fs = IEC958_AES3_CON_FS_32000;
>   break;
> @@ -59,7 +47,7 @@ int snd_pcm_create_iec958_consumer(struct snd_pcm_runtime 
> *runtime, u8 *cs,
>   }
>  
>   if (len > 4) {
> - switch (snd_pcm_format_width(runtime->format)) {
> + switch (sample_width) {
>   case 16:
>   ws = IEC958_AES4_CON_WORDLEN_20_16;
>   break;
> @@ -92,4 +80,34 @@ int snd_pcm_create_iec958_consumer(struct snd_pcm_runtime 
> *runtime, u8 *cs,
>  
>   return len;
>  }
> +
> +/**
> + * snd_pcm_create_iec958_consumer - create consumer format IEC958 channel 
> status
> + * @runtime: pcm runtime structure with ->rate filled in
> + * @cs: channel status buffer, at least four bytes
> + * @len: length of channel status buffer
> + *
> + * Create the consumer format channel status data in @cs of maximum size
> + * @len corresponding to the parameters of the PCM runtime @runtime.
> + *
> + * Drivers may wish to tweak the contents of the buffer after creation.
> + *
> + * Returns: length of buffer, or negative error code if something failed.
> + */
> +int snd_pcm_create_iec958_consumer(struct snd_pcm_runtime *runtime, u8 *cs,
> + size_t len)
> +{
> + return create_iec958_consumer(runtime->rate,
> +   snd_pcm_format_width(runtime->format),
> +   cs, len);
> +}
>  EXPORT_SYMBOL(snd_pcm_create_iec958_consumer);
> +
> +
> +int snd_pcm_create_iec958_consumer_hw_params(struct snd_pcm_hw_params 
> *params,
> +  u8 *cs, size_t len)
> +{
> + return create_iec958_consumer(params_rate(params), params_width(params),
> +   cs, len);
> +}
> +EXPORT_SYMBOL(snd_pcm_create_iec958_consumer_hw_params);
> -- 
> 1.9.1
> 
> 


signature.asc
Description: Digital signature

Re: [PATCH RFC] ASoC: simple-card: Update clocks binding for simple-card DAI subnodes

[Mark Brown]

On Fri, Sep 11, 2015 at 04:18:02PM +0300, Jyri Sarha wrote:

> The updated binding provides a way to set clock-ID and direction
> parameters for DAI drivers set_sysclk() call back.

> I proposed something similar about a year ago, but Mark rejected that
> at the time. This RFC is to start that discussion again. This time
> before I do any code changes.

What's the use case again?  Can we address this by converting the
relevant drivers to the clock API (or improving their clock API
support)?


signature.asc
Description: Digital signature

Re: [PATCH RFC v4 3/8] ASoC: hdmi-codec: Add hdmi-codec for external HDMI-encoders

[Mark Brown]

On Fri, Sep 18, 2015 at 02:06:40PM +0300, Jyri Sarha wrote:

> +#define SPDIF_FORMATS(SNDRV_PCM_FMTBIT_S16_LE | 
> SNDRV_PCM_FMTBIT_S16_BE |\
> +  SNDRV_PCM_FMTBIT_S20_3LE | SNDRV_PCM_FMTBIT_S20_3BE |\
> +  SNDRV_PCM_FMTBIT_S24_3LE | SNDRV_PCM_FMTBIT_S24_3BE |\
> +  SNDRV_PCM_FMTBIT_S24_LE | SNDRV_PCM_FMTBIT_S24_BE)
> +
> +#define I2S_FORMATS  (SNDRV_PCM_FMTBIT_S16_LE | SNDRV_PCM_FMTBIT_S16_BE |\
> +  SNDRV_PCM_FMTBIT_S18_3LE | SNDRV_PCM_FMTBIT_S18_3BE |\
> +  SNDRV_PCM_FMTBIT_S20_3LE | SNDRV_PCM_FMTBIT_S20_3BE |\
> +  SNDRV_PCM_FMTBIT_S24_3LE | SNDRV_PCM_FMTBIT_S24_3BE |\
> +  SNDRV_PCM_FMTBIT_S24_LE | SNDRV_PCM_FMTBIT_S24_BE |\
> +  SNDRV_PCM_FMTBIT_S32_LE | SNDRV_PCM_FMTBIT_S32_BE)

I'm not sure how abstracted this I2S and S/PDIF DAI business is - it
doesn't feel like something that's going to be a property of all HDMI
devices, and the specific sets of formats above cause me to raise a bit
of an eyebrow.  Should this be an interface where the HDMI chip
registers multiple interfaces if it has them instead of automatically
getting this split as is?

We can probably skip more specific constraint stuff I guess on the basis
that things tend to support the full range of HDMI functionality, or at
least that can be added later?

Otherwise this looks mostly OK on a first pass, thanks for working on
it.


signature.asc
Description: Digital signature

Re: [PATCH] ASoC: soc-core: Fix sparse warning in be32_to_cpup() call

[Mark Brown]

On Thu, Sep 17, 2015 at 11:02:57AM +0300, Jyri Sarha wrote:

>   val /= sizeof(u32);
>   for (i = 0; i < val; i++)
> - if (be32_to_cpup(_slot_mask[i]))
> + if (be32_to_cpup((__be32 *)_slot_mask[i]))
>   *mask |= (1 << i);
>  

There was no changelog and this is setting off alarm bells since the
cast just smashes warnings - are you sure we're not missing some other
annotations and that a cast is the best thing here?


signature.asc
Description: Digital signature

Re: [PATCH 1/5] spi: introduce mmap read support for spi flash devices

[Mark Brown]

On Wed, Sep 16, 2015 at 03:38:09PM +0530, Vignesh R wrote:

> But, I didn't get how to integrate with existing message queue. Memory
> mapped read by-passes message queue of SPI core. Could you please
> explain a bit more on integrating with message queue? Did you mean
> locking the existing message queue when memory mapped read is being
> requested?

Yes, and also making sure that we don't everything gets processed in
order so memory mapped requests come after any commands needed to set
them up and don't starve other work.


signature.asc
Description: Digital signature

Re: [PATCH 1/5] spi: introduce mmap read support for spi flash devices

[Mark Brown]

On Wed, Sep 16, 2015 at 06:16:55PM +0530, Jagan Teki wrote:
> On 15 September 2015 at 00:05, Mark Brown <broo...@kernel.org> wrote:

> > There seem to be a reasonable number of SPI controllers out there which
> > have as an extension the ability to do memory mapped reads but are
> > otherwise perfectly normal SPI controllers and which rely on that for
> > everything except reads.

> This is true, but exposing direct spi-nor arguments or features like
> read_opcode, addr_width or dummy_byte to spi layer isn't fine? because
> for spi layer there is a spi to flash transition mtd m25p80.c is there
> to handle is it?

The m25p80 driver is a generic, SPI level piece of code not a driver for
specific controller hardware.  The controller hardware drivers live in
the SPI subsystem.  In order to support this memory mapped feature we
need the controller drivers to expose an interface for controlling it.


signature.asc
Description: Digital signature

Re: [PATCH RFC 4/5] ASoC: davinci-mcasp: Get rid of bclk_lrclk_ratio in private data

[Mark Brown]

On Thu, Sep 10, 2015 at 10:16:30AM +0300, Jyri Sarha wrote:
> On 09/09/15 21:27, Jyri Sarha wrote:

> >+dev_warn(mcasp->dev,
> >+ "%s(): BCLK/LRCLK %d is not divisible by %d 
> >tdm slots",
> >+ div, mcasp->tdm_slots);
> 
> Oops, there is a __func__ missing here.

The patch is basically fine but please send a new version with this
fixed!


signature.asc
Description: Digital signature

Re: [PATCH 1/5] spi: introduce mmap read support for spi flash devices

[Mark Brown]

On Fri, Sep 04, 2015 at 04:55:33PM +0530, Jagan Teki wrote:
> On 4 September 2015 at 13:59, Vignesh R  wrote:

> > + * @spi_mtd_mmap_read: some spi-controller hardwares provide memory
> > + * mapped interface to communicate with mtd flashes.
> > + * For this, spi  controller needs to know flash
> > + * memory settings like read command to use, dummy
> > + * bytes and address width. Once these settings are
> > + * populated in hardware registers, any read
> > + * accesses to flash's memory map region(as defined
> > + * by SoC) through memcpy or mem-to-mem DMA copy
> > + * will be handled by controller hardware. The
> > + * hardware will automatically generate spi signals
> > + * required to read data from flash and present it
> > + * to CPU or DMA. SPI master drivers can use this
> > + * callback to implement memory mapped read
> > + * interface. Flash driver (like m25p80) requests
> > + * memory mapped read via this method. The interface
> > + * should  only be used mtd flashes and cannot be
> > + * used with other spi devices.

This comment is *way* too verbose - probably you just need up to the
"Once" here.

> > +   int (*spi_mtd_mmap_read)(struct  spi_device *spi,
> > +loff_t from, size_t len, size_t *retlen,
> > +u_char *buf, u8 read_opcode,
> > +u8 addr_width, u8 dummy_bytes);

> This looks un-manageable to know spi core or master knows what are the
> command or opcode used by spi-nor flash and also I think mmap support
> seems to be flash related or any justification this is spi bus
> master/controller feature?

There seem to be a reasonable number of SPI controllers out there which
have as an extension the ability to do memory mapped reads but are
otherwise perfectly normal SPI controllers and which rely on that for
everything except reads.


signature.asc
Description: Digital signature

Re: [PATCH 2/5] spi: spi-ti-qspi: add mmap mode read support

[Mark Brown]

On Fri, Sep 04, 2015 at 01:59:59PM +0530, Vignesh R wrote:

> +static int ti_qspi_spi_mtd_mmap_read(struct  spi_device *spi,
> +  loff_t from, size_t len,
> +  size_t *retlen, u_char *buf,
> +  u8 read_opcode, u8 addr_width,
> +  u8 dummy_bytes)
> +{
> + struct ti_qspi *qspi = spi_master_get_devdata(spi->master);
> + int ret = 0;
> +
> + spi_bus_lock(qspi->master);

I suspect I'm going to see the answer to this in another patch but the
fact that we're having to take this lock in a driver when it's an op the
core should be calling.

> + ret = pm_runtime_get_sync(qspi->dev);
> + if (ret < 0) {
> + dev_err(qspi->dev, "pm_runtime_get_sync() failed\n");
> + return ret;
> + }

This would be better outside the lock, there's no need to have the lock
before we power on and this fixes the fact that you don't release the
lock here.

> + memcpy(buf, (__force void *)(qspi->mmap_base + from), len);

The fact that you're having to cast here should be a warning that
there's someting wrong here.  I think you're looking for
memcpy_fromio().

> @@ -479,6 +576,7 @@ static int ti_qspi_probe(struct platform_device *pdev)
>   master->setup = ti_qspi_setup;
>   master->auto_runtime_pm = true;
>   master->transfer_one_message = ti_qspi_start_transfer_one;
> + master->spi_mtd_mmap_read = ti_qspi_spi_mtd_mmap_read;
>   master->dev.of_node = pdev->dev.of_node;
>   master->bits_per_word_mask = SPI_BPW_MASK(32) | SPI_BPW_MASK(16) |
>SPI_BPW_MASK(8);

Don't we need to map a resource somewhere?


signature.asc
Description: Digital signature

Re: [PATCH 3/5] mtd: devices: m25p80: add support for mmap read request

[Mark Brown]

On Fri, Sep 04, 2015 at 02:00:00PM +0530, Vignesh R wrote:

> + if (spi->master->spi_mtd_mmap_read) {
> + return  spi->master->spi_mtd_mmap_read(spi, from, len,
> +retlen, buf,
> +nor->read_opcode,
> +nor->addr_width,
> +dummy);
> + }

We should be calling some API provided by the SPI core here, not peering
directly into the ops struture.


signature.asc
Description: Digital signature

Re: [PATCH 1/5] spi: introduce mmap read support for spi flash devices

[Mark Brown]

On Fri, Sep 04, 2015 at 01:59:58PM +0530, Vignesh R wrote:
> In addition to providing direct access to SPI bus, some spi controller
> hardwares (like ti-qspi) provide special memory mapped port
> to accesses SPI flash devices in order to increase read performance.
> This means the controller can automatically send the SPI signals
> required to read data from the SPI flash device.

Sorry, also meant to say here: as I kind of indicated in response to the
flash patch I'd expect to see the SPI core know something about this and
export an API for this which is integrated with things like the existing
message queue.


signature.asc
Description: Digital signature

Re: [PATCH v3 1/2] regulator: pbias: program pbias register offset in pbias driver

[Mark Brown]

On Fri, Sep 04, 2015 at 05:30:24PM +0530, Kishon Vijay Abraham I wrote:
> Add separate compatible strings for every platform and populate the
> pbias register offset in the driver data.
> This helps avoid depending on the dt for pbias register offset.

If there are any changes from the already applied patch for this please
send an incremental patch.  Please don't resend already applied patches.


signature.asc
Description: Digital signature

Re: [PATCH 1/6] regulator: pbias: program pbias register offset in pbias driver

[Mark Brown]

On Wed, Sep 02, 2015 at 04:17:28PM +0530, Kishon Vijay Abraham I wrote:
> Add separate compatible strings for every platform and populate the
> pbias register offset in the driver data.
> This helps avoid depending on the dt for pbias register offset.

Can you respin with Tony's comment please and I'll apply?


signature.asc
Description: Digital signature

Re: [PATCH 1/2] regulator: pbias: use untranslated address to program pbias regulator

[Mark Brown]

On Tue, Sep 01, 2015 at 11:56:06AM -0700, Tony Lindgren wrote:
> * Mark Brown <broo...@kernel.org> [150901 11:40]:

> > That'd work.  The other thing I was thinking we could do is to get
> > syscon to treat any excessively large address that gets passed in that
> > looks like an absolute address appropriately.

> Hmm wouldn't that get messy for 64-bit :) How about something
> like:

Meh, it'd probably be fine - I was thinking falling back to trying to
substract the base address if the passed address was out of bounds.
It's not super elegant though, something that directly does the right
thing would be better.

> unsigned long syscon_regmap_get_offset(struct regmap *syscon,
>   void __iomem *base)

> Not sure if that's something that some drivers would start to
> misuse with read/writel though.. Presumably not if the driver
> already is using syscon.

It should be really easy to spot abuse I'd hope.


signature.asc
Description: Digital signature

Re: [PATCH 1/2] regulator: pbias: use untranslated address to program pbias regulator

[Mark Brown]

On Tue, Sep 01, 2015 at 07:17:21AM -0700, Tony Lindgren wrote:

> Why don't you just make reg unused for pbias_regulator since
> we already use regmap for this driver?

> Then in the pbias driver just define the register offset from
> the syscon base? You may need to set it based on the compatible
> value, but it's not like it's going to change for SoC.

> If we eventually add some API to calculate reg base offset
> from the syscon base it's easy to update the driver to use
> that.

That'd work.  The other thing I was thinking we could do is to get
syscon to treat any excessively large address that gets passed in that
looks like an absolute address appropriately.


signature.asc
Description: Digital signature

Re: [PATCH 1/2] regulator: pbias: use untranslated address to program pbias regulator

[Mark Brown]

On Mon, Aug 31, 2015 at 04:14:07PM +0530, Kishon Vijay Abraham I wrote:
> On Tuesday 25 August 2015 07:20 PM, Mark Brown wrote:
> > On Tue, Aug 25, 2015 at 01:03:04PM +0300, Grygorii Strashko wrote:
> >> On 08/19/2015 09:11 PM, Mark Brown wrote:

> >>> So substract this address from the start of the resource to get the
> >>> offset?  Or provide a wrapper function in the resource code which does
> >>> that.  
> > 
> >> I'd be very appreciated if you have and can share any thought on
> >> How can we get this absolute base address to substract?
> > 
> > Ask the syscon device for its resource?  Or have it provide an absolute
> 
> Even if we get the absolute address of syscon, we have to do the
> subtraction only for the newer dtbs (previous dtbs already have only the
> offset). Do you recommend adding a new property to differentiate between
> older dtbs and newer dtbs? Any other suggestions here?

Hang on.  This is the first I've heard of any DTs not just having
absolute addresses.  How does any of this work - has it ever worked, and
is the situation completely understood?  My original concern with this
was that I coudn't understand the commit log and your original response
seemed to indicate that we always have the absolute address :(  Perhaps
this is something to do with the brief mention of having moved the DT
node for some reason?

We at least need some sort of coherent explanation of the problem and a
comprehensible fix to go with it.  Right now it seems like things are
just being moved about to hide problems without either of these things
which seems like it makes the code less clear and more fragile.

> > address based interface for that matter?

> Syscon doesn't directly expose any API's to write to it's register.
> Rather it uses regmap APIs to read/write to it's register. I'm not sure
> if it's possible to add regmap APIs to write to a register with absolute
> address. Any hints?

Yes, I'm aware that it is regmap based!  What I am suggesting is that if
people are making DTs like yours with devices that are children of the
syscon then presumably it might be useful for it to allow client drivers
to pass absolute addresses in so that it can translate for them.


signature.asc
Description: Digital signature

Re: [PATCH] spi: omap2-mcspi: add runtime PM to set_cs()

[Mark Brown]

On Sun, Aug 30, 2015 at 11:44:45AM -0500, Michael Welling wrote:

> The patch is currently sitting in linux-next.

> Not sure why it wasn't merged with 4.2.0-rc8.

You didn't indicate that it was a bug fix for Linus rather than a fix
for recent development :(


signature.asc
Description: Digital signature

Re: [PATCH] spi: omap2-mcspi: add runtime PM to set_cs()

[Mark Brown]

On Mon, Aug 31, 2015 at 08:46:46AM -0500, Michael Welling wrote:
> On Mon, Aug 31, 2015 at 09:53:55AM +0100, Mark Brown wrote:
> > On Sun, Aug 30, 2015 at 11:44:45AM -0500, Michael Welling wrote:

> > > The patch is currently sitting in linux-next.

> > > Not sure why it wasn't merged with 4.2.0-rc8.

> > You didn't indicate that it was a bug fix for Linus rather than a fix
> > for recent development :(

Ah, actually it did get applied as a fix - it's just that I didn't send
a pull request before v4.3 got released.  Looking at what's there I
wasn't comfortable with the volume of fixes that arrived and never got
round to picking out those that were most urgent.  Sorry, these things
do happen from time to time I'm afraid especially when I'm travelling,
if something is urgent it's good to verify around -rc6 or so.

> Sorry, I did not know that it was my responsibility.

> How do I indicate this for future reference?

> The patch that Sebastian sent said the following:

> "
> Michael also tested the patch, but have not explicitly written an
> Tested-By, so you may want to wait for feedback from him. The patch
> should be sent for 4.2-rc, which introduced the regression.
> "

That's not in the changelog which is all I have after the patch is
applied (and what I was looking at since I just pulled the commit up by
ID).  If something is in Linus' tree it's often helpful to say
"...introduced in v4.2-rc1" or similar in the changelog.  Though in this
case it wasn't the issue.


signature.asc
Description: Digital signature

Re: [PATCH 0/2] OMAPDSS: Couple of HDMI audio fixes

[Mark Brown]

On Wed, Aug 26, 2015 at 04:11:38PM +0300, Jyri Sarha wrote:

 The two fixes are totally independent and the video and audio side
 patches applied separately trough their own trees.

In general if patches aren't related to each other either through code
overlap or through content it's better to just send them as individual
changes, that avoids any potential for confusion.


signature.asc
Description: Digital signature

Re: [PATCH 1/2] regulator: pbias: use untranslated address to program pbias regulator

[Mark Brown]

On Tue, Aug 25, 2015 at 01:03:04PM +0300, Grygorii Strashko wrote:
 On 08/19/2015 09:11 PM, Mark Brown wrote:

  So substract this address from the start of the resource to get the
  offset?  Or provide a wrapper function in the resource code which does
  that.  

 I'd be very appreciated if you have and can share any thought on
 How can we get this absolute base address to substract?

Ask the syscon device for its resource?  Or have it provide an absolute
address based interface for that matter?


signature.asc
Description: Digital signature

Re: [PATCH 1/2] regulator: pbias: use untranslated address to program pbias regulator

[Mark Brown]

On Thu, Aug 20, 2015 at 11:21:06AM +0530, Kishon Vijay Abraham I wrote:
 On Wednesday 19 August 2015 11:41 PM, Mark Brown wrote:
  On Tue, Aug 18, 2015 at 11:23:54AM +0530, Kishon Vijay Abraham I wrote:

Please fix your mail client to word wrap within paragraphs.

  platform_get_resource can be used if we need the absolute address but here 
  we
  need only the offset.

  So substract this address from the start of the resource to get the

 That would mean from the offset (provided in dt) get the absolute address and
 then again from the absolute address get the offset.

Sure, that's how the Linux APIs work right now and why I'm suggesting
you might want a wrapper.

  offset?  Or provide a wrapper function in the resource code which does

 Why not use 'of_get_address' which does the same thing? Moreover it's not a
 resource we are dealing with here. It's a resource only for the syscon driver.

This is how the OF description you've done is intended to be parsed and
hence is interpreted by the generic code, it's just a detail of the
syscon implementation that you need to translate it into an offset.

  that.  What you're saying above is pretty much this happens to work
  but my concern is that the solution that happens to work isn't really
  what we want to do.

 Not just makes this work, this is also the most reasonable solution available 
 IMHO.

In general moving to a lower level inteface is not usually a step
forward.  As far as I can tell the driver already has all the
information it needs from the more generic APIs, it's just not
interpreting it in the way that the APIs it's trying to use wants.  It
just needs to fix the way it interprets the data it's passing through.

 The most ideal way would have been to use something like what Grygorii
 mentioned to use syscon = scm_conf 0xe00 and then use the phandle to get 
 the
 offset. But then with this we'll be breaking older dtbs.

There is no need to break older DTs, that would clearly be a bad idea.


signature.asc
Description: Digital signature

Re: [PATCH] spi: ti-qspi: use 128 bit transfer mode for writing to flash

[Mark Brown]

On Thu, Aug 20, 2015 at 04:00:59PM +0530, Vignesh R wrote:

 - writeb(*txbuf, qspi-base + QSPI_SPI_DATA_REG);
 + if (count = QSPI_WLEN_MAX_BYTES) {
 + u32 *txp = (u32 *)txbuf;
 +
 + data = cpu_to_be32(*txp++);
 + writel(data, qspi-base +
 +QSPI_SPI_DATA_REG_3);
 + data = cpu_to_be32(*txp++);
 + writel(data, qspi-base +
 +QSPI_SPI_DATA_REG_2);
 + data = cpu_to_be32(*txp++);
 + writel(data, qspi-base +
 +QSPI_SPI_DATA_REG_1);
 + data = cpu_to_be32(*txp++);
 + writel(data, qspi-base +
 +QSPI_SPI_DATA_REG);
 + xfer_len = QSPI_WLEN_MAX_BYTES;
 + cmd |= QSPI_WLEN(QSPI_WLEN_MAX_BITS);
 + } else {
 + writeb(*txbuf, qspi-base + QSPI_SPI_DATA_REG);
 + cmd = qspi-cmd | QSPI_WR_SNGL;
 + xfer_len = wlen;
 + cmd |= QSPI_WLEN(wlen);
 + }

It's a bit sad that this isn't able to do a Duff's device type thing and
only kicks in for the full 128 bit FIFO size, it looks like it could do
any number of words.


signature.asc
Description: Digital signature

Re: [PATCH 1/2] regulator: pbias: use untranslated address to program pbias regulator

[Mark Brown]

On Tue, Aug 18, 2015 at 11:23:54AM +0530, Kishon Vijay Abraham I wrote:
 On Friday 14 August 2015 11:30 PM, Mark Brown wrote:
  On Mon, Jul 27, 2015 at 04:54:09PM +0530, Kishon Vijay Abraham I wrote:

  is moved as a child node of syscon, vsel_reg and enable_reg has the
  absolute address because of the address translation that happens while
  creating device from device tree node.
  So avoid using platform_get_resource and use of_get_address in order to
  get only the offset (untranslated address) and populate these in
  vsel_reg and enable_reg.

  This sounds like we're going in the wrong direction, we're moving from a
  more generic API to a firmware specific one.  Why is this a good fix?

 platform_get_resource can be used if we need the absolute address but here we
 need only the offset.

So substract this address from the start of the resource to get the
offset?  Or provide a wrapper function in the resource code which does
that.  What you're saying above is pretty much this happens to work
but my concern is that the solution that happens to work isn't really
what we want to do.


signature.asc
Description: Digital signature

Re: [PATCH RFC v3 3/7] ASoC: hdmi-codec: Add hdmi-codec for external HDMI-encoders

[Mark Brown]

On Mon, Aug 17, 2015 at 10:07:55AM +0300, Jyri Sarha wrote:
 On 08/14/15 19:18, Mark Brown wrote:
 On Fri, Aug 14, 2015 at 12:30:41PM +0300, Jyri Sarha wrote:

 +   /* Called when ASoC starts an audio stream setup. The call
 +* provides an audio abort callback for stoping an ongoing
 +* stream if the HDMI audio becomes unavailable.
 +* Optional */
 +   int (*audio_startup)(struct device *dev,
 +void (*abort_cb)(struct device *dev));

 I'm a bit confused about what is going to use abort_cb() and why they
 wouldn't just call shutdown instead?

 audio_shutdown() is for ASoC side to tell video side that audio playback has
 stopped.

 The abort_cb() is for video side to inform ASoC that current audio stream
 can not continue anymore and it should be aborted. The similar mechanism is
 currently in use in sound/soc/omap/omap-hdmi-audio.c.

Someone reading the code needs to be able to understand this.


signature.asc
Description: Digital signature

Re: [PATCH RFC v3 2/7] ASoC: hdmi: Remove obsolete dummy HDMI codec

[Mark Brown]

On Mon, Aug 17, 2015 at 10:00:07AM +0300, Jyri Sarha wrote:
 On 08/14/15 19:10, Mark Brown wrote:

 Don't you mean omap-hdmi-audio, that is implemented in
 sound/soc/omap/omap-hdmi-audio.c ?

 That driver is bit different. It implements ASoC card and uses generic dummy
 codec. The hdmi-audio-codec has not been used for OMAP HDMI audio for
 several releases already.

Possibly, yes, or I was on an old tree by mistake.  It's definitely not
used now like you say.


signature.asc
Description: Digital signature

Re: [PATCH RFC v2 3/7] ASoC: hdmi-codec: Add hdmi-codec for external HDMI-encoders

[Mark Brown]

On Tue, May 26, 2015 at 09:59:07PM +0300, Jyri Sarha wrote:

 +
 + mutex_lock(hcp-current_stream_lock);
 + if (hcp-current_stream  hcp-current_stream-runtime 
 + snd_pcm_running(hcp-current_stream)) {
 + dev_info(dev, HDMI audio playback aborted\n);

Does this really need to be dev_info()?

 + if (hcp-hcd.ops-get_eld) {
 + hcp-eld = hcp-hcd.ops-get_eld(hcp-hcd.dev);
 +
 + /* Call snd_pcm_hw_constraint_eld here */
 + }

...

 + dev_dbg(dai-dev, %s()\n, __func__);
 +
 + mutex_lock(hcp-current_stream_lock);
 + BUG_ON(hcp-current_stream != substream);
 + hcp-current_stream = NULL;
 + mutex_unlock(hcp-current_stream_lock);
 +
 + hcp-hcd.ops-audio_shutdown(hcp-hcd.dev);

Shouldn't the callback be in or before the lock?  Otherwise we could
potentially race with starting a new stream.


signature.asc
Description: Digital signature

Re: [PATCH RFC v3 2/7] ASoC: hdmi: Remove obsolete dummy HDMI codec

[Mark Brown]

On Fri, Aug 14, 2015 at 12:30:40PM +0300, Jyri Sarha wrote:
 The hdmi stub codec has not been used since refactoring of OMAP HDMI
 audio support.

grep tells me that the OMAP HDMI4 and HDMI5 drivers are still
registering this device in -next... 


signature.asc
Description: Digital signature

Re: [PATCH RFC v3 3/7] ASoC: hdmi-codec: Add hdmi-codec for external HDMI-encoders

[Mark Brown]

On Fri, Aug 14, 2015 at 12:30:41PM +0300, Jyri Sarha wrote:

 +struct hdmi_codec_ops {
 + /* For runtime clock configuration from ASoC machine driver.
 +  * A direct forward from set_sysclk in struct snd_soc_dai_ops.
 +  * Optional */
 + int (*set_clk)(struct device *dev, int clk_id, int freq);

I'd be much happier if we were using the clock API as the external
interface here, it's where we want to be internally too and it's going
to be easier to not introduce any external dependencies on the ASoC
internal stuff.

 + /* Called when ASoC starts an audio stream setup. The call
 +  * provides an audio abort callback for stoping an ongoing
 +  * stream if the HDMI audio becomes unavailable.
 +  * Optional */
 + int (*audio_startup)(struct device *dev,
 +  void (*abort_cb)(struct device *dev));

I'm a bit confused about what is going to use abort_cb() and why they
wouldn't just call shutdown instead?

 +/* HDMI codec initalization data */
 +struct hdmi_codec_pdata {
 + struct device *dev; /* The HDMI encoder registering the codec */

Shouldn't this just be dev-parent?

 +enum {
 + DAI_ID_I2C = 0,
 + DAI_ID_SPDIF,
 +};

I2C?  :P


signature.asc
Description: Digital signature

Re: [PATCH 1/2] regulator: pbias: use untranslated address to program pbias regulator

[Mark Brown]

On Mon, Jul 27, 2015 at 04:54:09PM +0530, Kishon Vijay Abraham I wrote:

 vsel_reg and enable_reg of the pbias regulator descriptor should actually
 have the offset from syscon. However after the pbias device tree node

I'm having a hard time understanding this statement, sorry.  What makes
you say that they shouild actually have the offset from syscon?  What
is the problem that this is supposed to fix?

 is moved as a child node of syscon, vsel_reg and enable_reg has the
 absolute address because of the address translation that happens while
 creating device from device tree node.
 So avoid using platform_get_resource and use of_get_address in order to
 get only the offset (untranslated address) and populate these in
 vsel_reg and enable_reg.

This sounds like we're going in the wrong direction, we're moving from a
more generic API to a firmware specific one.  Why is this a good fix?


signature.asc
Description: Digital signature

Re: [RFC PATCH 1/5] spi: introduce flag for memory mapped read

[Mark Brown]

On Thu, Aug 06, 2015 at 11:22:25AM +0100, Russell King - ARM Linux wrote:

 If that's the case, then maybe you should consider whether using the SPI
 bus infrastructure is really the best way forward.  Would it make more
 sense instead to adopt a different software structure, something more
 high-level like:

  +---+
  |  m25p80 high-level driver |
  +--++
  |   SPI m25p80 driver  ||
  +--+|
  |  SPI layer   |  Special driver|
  +--+|
  |SPI bus driver||
  +--++
  | SPI hardware |  Special hardware  |
  +--++

Yes, that's what's been talked about before - for some of these devices
they're sufficiently flash specialist that we just don't bother exposing
a SPI interface at all though AIUI they could be persuaded to do it.  It
isn't entirely clear that we want exactly that split, if the devices are
reasonable SPI controllers we will want to handle the case where they
have flash and non-flash devices on the same bus.  For that there is
going to be some generalisable work possible for managing switching
between memory mapped and SPI modes where those are mutually exclusive,
especially if the switch between them isn't free.


signature.asc
Description: Digital signature

Re: [RFC PATCH 1/5] spi: introduce flag for memory mapped read

[Mark Brown]

On Thu, Aug 06, 2015 at 12:01:37PM +0200, Michal Suchanek wrote:

 However, I am familiar m25p80.c and as I understand it the controller
 is basically supposed to implement m25p80.c in hardware when this flag
 is set.

But what in concrete terms is that supposed to mean?  It's currently
just an essentially undocumented flag on a message rather than something
operating at the level of a flash chip.  That's pretty much where
Russell's comments come from.

 If I was using m25p80.c to talk to anything but an actual flash chip
 it would get me quite worried.

Sure, but at the end of the day it's just emitting standard SPI messages
which don't know anything about flash.  If those messages are a sensible
interface here then why bother with the flag, we can just pattern match
on the format of the message.  If that doesn't work then probably this
isn't a great interface and a separate, application specific interface
makes more sense.


signature.asc
Description: Digital signature

Re: next-20150806 build: 2 failures 52 warnings (next-20150806)

[Mark Brown]

On Thu, Aug 06, 2015 at 12:00:47PM +0100, Build bot for Mark Brown wrote:

Today's linux-next fails to build an ARM allmodconfig with:

 ../drivers/iommu/omap-iommu-debug.c:138:2: error: void value not ignored as 
 it ought to be

caused by a combination of 1ba2f20ac (fs/seq_file: convert int
seq_vprint/seq_printf/etc... returns to void) which made seq_printf()
return void and e203db293863fa15 (iommu/omap: Fix debug_read_tlb() to
use seq_printf()) which has been in -next for a little while now.


signature.asc
Description: Digital signature

Re: [RFC PATCH 1/5] spi: introduce flag for memory mapped read

[Mark Brown]

On Thu, Aug 06, 2015 at 02:51:29PM +0100, Russell King - ARM Linux wrote:

 The M25P80 driver just appends additional bytes to the message to
 achieve this:
 
 struct m25p *flash = nor-priv;
 unsigned int dummy = nor-read_dummy;
 
 /* convert the dummy cycles to the number of bytes */
 dummy /= 8;
 
 flash-command[0] = nor-read_opcode;
 m25p_addr2cmd(nor, from, flash-command);
 
 t[0].tx_buf = flash-command;
 t[0].len = m25p_cmdsz(nor) + dummy;
 spi_message_add_tail(t[0], m);

 The reason that the number of dummy bytes can't be detected is because
 it's all hidden in the first transaction as the total number of bytes to
 be transmitted - and the dummy bytes are uninitialised, so you can't
 make any assumptions what value they are.  There is no way for the SPI
 driver to know whether these dummy bytes are dummy bytes or whether they
 have an effect on the targetted device.

We *could* (as you suggest below) indicate dummy transfers by having a
separate transfer which omits the transmit buffers though I'd expect
that normally that is going to be a small performance hit if interpreted
directly so we need to think what to do there.  We do get other devices
sending dummy bytes, it's sometimes a requirement for high speed
register access to give settling time for the device, so other things
would get some milage from it.

 What may make more sense from the SPI point of view is to communicate to
 all SPI drivers how many dummy bytes are to be transferred.  I'm not fully
 up on SPI, but maybe something like this:

   t[0].tx_buf = flash-command;
   t[0].len = m25p_cmdsz(nor);
   spi_message_add_tail(t[0], m);
   t[1].tx_buf = dummy_buffer;
   t[1].len = dummy;
   t[1].dummy = 1;
   spi_message_add_tail(t[1], m);

 This way, we're describing the transfer to the SPI core, and explicitly
 indicating that there are some dummy bytes.  The SPI driver can then
 tell that these are dummy bytes, and if the SPI message consists of:

That'd work as well, my first thought would to use NULL as a dummy
buffer pointer and let the core substitute in data for the drivers.  We
currently insist on having at least one buffer but that's fixable.

 This would not be a hack to the SPI code: we're describing to the SPI
 code what we want to achieve in terms of the activity on the bus, and
 providing that level of description then allows the SPI driver to make
 informed decisions on whether it can handle the transfer using some
 non-standard feature.

Yup.


signature.asc
Description: Digital signature

Re: [RFC PATCH 1/5] spi: introduce flag for memory mapped read

[Mark Brown]

On Thu, Aug 06, 2015 at 05:55:23PM +0530, Vignesh R wrote:

 1.Write to flash config register via config port to switch to QUAD MODE
 (or any mode that flash supports).
 2. Populate QSPI_SPI_SETUP_REGx with flash read command, number of
 address bytes to use and dummy bytes required.

These things being constant for a given flash?

 3. Switch to memory mapped port by writing to QSPI_SPI_SWITCH_REG.
 4. Now, its possible to perform read from 0x5C00 to 0x5FFF using
 memcpy. The qspi controller hardware will communicate over SPI bus and
 get the data. This data is directly sent to RAM via SoC's interconnect.

Presumably if it's done via memcpy() it won't go direct to RAM but
rather be bounced through the CPU which is a bit interesting for
performance (it might help, but it does mean that there's a core stalled
waiting for the flash which might not be the best use of resources if
there's other things it can be getting on with).  With DMA it'd be a
direct to memory transfer though.

 Advantages of memory mapped port are: improved read performance,
 MEM_TO_MEM DMA support can be added (ti-qspi hardware as such does not
 provide DMA events).

DMA would definitely help here.

 I just need to know whether the user that requested the transfer is
 m25p80 driver. If yes, ti-qspi driver can take advantage of memory
 mapped interface, else just use config port to access SPI bus directly.

You don't *really* care if it's that specific user so much as that it's
that particular pattern.

 Writing separate driver based on spi-nor framework to interface with
 m25p80 is not an option because, I would lose the ability to interface
 with non-flash devices.

You could, however, expose an explicit flash mapping interface for this
functionality.  That does seem like it's going to be an awful lot easier 
and help with keeping things like DMA support out of the driver and in
the core (which would be useful if there's other controllers with the
same functionality, I seem to recall that there are).

 The spi_message that is received in transfer_one_message() is too
 generic to imply the slave device that is on the other side of the wire.
 IMO, the read command does not imply that the slave is m25p80 flash
 (besides the read opcodes vary across vendors of m25p80 and across modes).

Again, it doesn't matter if it's actually a read command only that it's
got a compatible format on the bus.


signature.asc
Description: Digital signature

Re: [RFC PATCH 1/5] spi: introduce flag for memory mapped read

[Mark Brown]

On Wed, Aug 05, 2015 at 02:56:09PM +0200, Michal Suchanek wrote:
 On 5 August 2015 at 14:44, Mark Brown broo...@kernel.org wrote:
  On Wed, Aug 05, 2015 at 02:40:01PM +0200, Michal Suchanek wrote:

  I don't think sending 03 or other random byte as the first byte of a
  SPI transfer can be used as reliable detection that we are talking to
  a SPI flash memory.

  Why care - if something is physically in the same format as a flash read
  command how would a device be able to tell that it wasn't actually a
  flash read command?  The signals sent on the bus are going to be
  identical anyway.

 Not only must the command be the same but also the response must be tha same.

What difference would that make?  The caller is sending a single SPI
operation and this is a user visible thing...

 The flash chip responds by sending arbitrary amount of data. Given
 that transfer_one gets only the part that sends the read command and
 the part to do the actual read may or may not follow this is getting a
 bit hairy. Add in dummy bytes due to fast-read lag and page write
 wrap-around and you get something that you definitely do not want
 unless you are really sure that there is a flash memory on the other
 end of the wire.

So if you're doing this you may have a good reason to implement
transfer_one_message() instead.  Or perhaps implement it in the core and
provide operations to do the map and unmap.  And of course if this sort
of requirement exists that's an obvious thing that must be documented
in the interfaces but isn't.

We need a lot more thought about the interface here, the lack of any
explanation of what the interface is supposed to be and the fact that
all questions about it are being answered in terms of describing the
specific system are both a bit worrying.


signature.asc
Description: Digital signature

Re: [RFC PATCH 1/5] spi: introduce flag for memory mapped read

[Mark Brown]

On Thu, Aug 06, 2015 at 01:42:32PM +0200, Michal Suchanek wrote:
 On 6 August 2015 at 13:23, Mark Brown broo...@kernel.org wrote:

  Sure, but at the end of the day it's just emitting standard SPI messages
  which don't know anything about flash.  If those messages are a sensible
  interface here then why bother with the flag, we can just pattern match
  on the format of the message.  If that doesn't work then probably this
  isn't a great interface and a separate, application specific interface
  makes more sense.

 The messages are sensible interface for communicating with a device
 that interprets a particular part of the mesasge as address and
 another particular part of the message as command and sends same
 amount of junk before reply as the flash chip would. If your device

That's just a statement that it's possible to do things this way, it's
not clear to me that it's an explanation as to why it is sensible to do
so - the obvious thing to do there would be to specify the flash
operation as a flash operation rather than reverse engineer the flash
operation from the wire format.

 happens to send reply immediately part of it is trashed. If it happens
 to interpret address differently the data ends up in random part of
 your memory. So no, that is not something you can autodetect.

If this stuff doesn't appear in the spi_message in some observable form
then I'd expect that any existing flash support is broken since a SPI
controller that doesn't have any acceleration is just going to do what
the message says.

 At the end of the day you have valid SPI messages but the m25p80 layer
 adds interpretation to those messages which may not always give
 correct result.

Which comes back to the thing I keep saying about needing some sort of
information about what the flag means and the questions about why this
is a good interface...

 On the other hand, if you ever get to m25p80 or spi-nor you can assume
 any message you send goes to a flash chip and insist that the
 controller uses the flash-specific interface.

There's an awful lot of flashes out there connected to controllers that
don't implement any sort of acceleration for flash, I'm not convinced
that your assumption there is valid.

 If there is possibility of connecting different kind of devices to
 multiple chipselects on the same master then you probably want to
 select this option per message or per slave.

We definitely need to account for that.


signature.asc
Description: Digital signature

Re: [RFC PATCH 1/5] spi: introduce flag for memory mapped read

[Mark Brown]

On Tue, Aug 04, 2015 at 11:29:52PM +0530, R, Vignesh wrote:
 On 8/4/2015 9:21 PM, Mark Brown wrote:
  On Mon, Aug 03, 2015 at 10:27:19AM +0530, Vignesh R wrote:

  I still can't tell from the above what this interface is supposed to do.
  It sounds like the use of memory mapped mode is supposed to be
  transparent to users, it should just affect how the controller interacts
  with the hardware, but if that's the case why do we need to expose it to
  users at all?  Shouldn't the driver just use memory mapped mode if it's
  faster?

 2. SFI_MM_IF(SPI memory mapped interface): The SFI_MM_IF block only
 allows reading and writing to an SPI flash device only. Used to speed up
 flash reads. It _cannot_ be used to communicate with non flash devices.
 Now, the spi_message that ti-qspi receives in transfer_one() callback
 can be from mtd device(in which case SFI_MM_IF can be used) or from any
 other non flash SPI device (in which case SFI_MM_IF must not be used
 instead SPI_CORE is to be used) but there is no way(is there?) to
 distinguish where spi_message is from. Therefore I introduced flag
 (use_mmap_mode) to struct spi_message. mtd driver will set flag to true,
 this helps the ti-qspi driver to determine that the user is flash device
 and thus can do read via SFI_MM_IF. If this flag is not set then the
 user is assumed to be non flash SPI driver and will use SPI_CORE block
 to communicate.

So if you're trying to do this you need to document it adequately so
that other people can understand what it is supposed to do and how to
use and implement it.  People can't really tell how the interface is
supposed to work based on what was in the patch and the above isn't
really helping.  For example, how does this change or restrict what the
contents of the spi_message are?

 On the whole, I just need a way to determine that the user is a flash
 device in order to switch to memory mapped interface.

As far as I can tell you want to set a per spi_message flag saying that
the message is a flash read command?  If that's what this is trying to
do then why do you need to set the flag at all?  If the message is in a
clearly defined format and it's more efficient to use this mmap mode
then surely the driver can just recognise that the format is approprate
and switch into mmap mode without being explicitly told - I'm not clear
what the flag adds here.


signature.asc
Description: Digital signature

Re: [RFC PATCH 1/5] spi: introduce flag for memory mapped read

[Mark Brown]

On Wed, Aug 05, 2015 at 02:40:01PM +0200, Michal Suchanek wrote:
 On 5 August 2015 at 13:50, Mark Brown broo...@kernel.org wrote:

  As far as I can tell you want to set a per spi_message flag saying that
  the message is a flash read command?  If that's what this is trying to
  do then why do you need to set the flag at all?  If the message is in a
  clearly defined format and it's more efficient to use this mmap mode
  then surely the driver can just recognise that the format is approprate
  and switch into mmap mode without being explicitly told - I'm not clear
  what the flag adds here.

 ehm, the read command is just one byte.

 I don't think sending 03 or other random byte as the first byte of a
 SPI transfer can be used as reliable detection that we are talking to
 a SPI flash memory.

Why care - if something is physically in the same format as a flash read
command how would a device be able to tell that it wasn't actually a
flash read command?  The signals sent on the bus are going to be
identical anyway.


signature.asc
Description: Digital signature

Re: [RFC PATCH 1/5] spi: introduce flag for memory mapped read

[Mark Brown]

On Mon, Aug 03, 2015 at 10:27:19AM +0530, Vignesh R wrote:

 @use_mmap_mode: Some SPI controller chips are optimized for interacting
 with serial flash memories. These chips have memory mapped interface,
 through which entire serial flash memory slave can be read/written as if
 though they are physical memories (like RAM). Using this interface,
 flash can be accessed using memcpy() function and the spi controller
 hardware will take care of communicating with serial flash over SPI.
 Setting this flag will indicate the SPI controller driver that the
 spi_message is from mtd layer to read from/write to flash. The SPI
 master driver can then appropriately switch the controller to memory
 mapped interface to read from/write to flash, based on this flag (See
 drivers/spi/spi-ti-qspi.c for example).
 NOTE: If the SPI controller chip lacks memory mapped interface, then the
 driver will ignore this flag and use normal SPI protocol to read
 from/write to flash. Communication with non-flash SPI devices is not
 possible using the memory mapped interface.

I still can't tell from the above what this interface is supposed to do.
It sounds like the use of memory mapped mode is supposed to be
transparent to users, it should just affect how the controller interacts
with the hardware, but if that's the case why do we need to expose it to
users at all?  Shouldn't the driver just use memory mapped mode if it's
faster?


signature.asc
Description: Digital signature

Re: [RFC PATCH 4/5] ARM: dts: DRA7: Add memory map region entries for qspi

[Mark Brown]

On Mon, Aug 03, 2015 at 10:32:09AM +0530, Vignesh R wrote:
 On 07/31/2015 11:49 PM, Mark Brown wrote:

  -  reg = 0x4790 0x100;
  +  reg = 0x4790 0x100,
  +0x3000 0x3ff;
  +  reg-names = qspi_base, qspi_mmap;

  The DT binding document for the controller needs to be extended to
  document this change in the binding.

 DT bindings are already documented at
 Documentation/devicetree/bindings/spi/ti_qspi.txt. Did you mean to add
 this node as an example in that file?

No, I mean you're changing the binding to add this new memory region -
that new region needs to be added to the documentation.


signature.asc
Description: Digital signature

Re: [RFC PATCH 1/5] spi: introduce flag for memory mapped read

[Mark Brown]

On Tue, Jul 28, 2015 at 02:11:12PM +0530, Vignesh R wrote:

 Introduce use_mmap_read field in spi_message struct. This can be set by
 mtd devices (m25p80) to indicate to spi-master (ti-qspi) to perform
 memory mapped read. This helps to distinguish whether the spi-message is
 from mtd layer(hence mmap read is possible) or by other spi devices.

Based on this description and...

 + * @use_mmap_mode: Indicate to spi master to perform memory mapped
 + *   read if possible.

...the internal documentation I unable to tell what is meant by perform
a memory mapped read, at least to the extent where it is visible
outside of the driver.  This means we can't really use this as a generic
API since other people won't be able to tell what it does.


signature.asc
Description: Digital signature

Re: [RFC PATCH 4/5] ARM: dts: DRA7: Add memory map region entries for qspi

[Mark Brown]

On Tue, Jul 28, 2015 at 02:11:15PM +0530, Vignesh R wrote:
 Add qspi memory mapped region entries for DRA7xx based SoCs.
 
 Signed-off-by: Vignesh R vigne...@ti.com

   qspi: qspi@4790 {
   compatible = ti,am4372-qspi;
 - reg = 0x4790 0x100;
 + reg = 0x4790 0x100,
 +   0x3000 0x3ff;
 + reg-names = qspi_base, qspi_mmap;

The DT binding document for the controller needs to be extended to
document this change in the binding.


signature.asc
Description: Digital signature

Re: [PATCH] spi: omap2-mcspi: add runtime PM to set_cs()

[Mark Brown]

On Wed, Jul 22, 2015 at 08:46:09PM +0200, Sebastian Reichel wrote:
 Since commit ddcad7e9068eb omap2_mcspi_set_cs() is called without
 runtime power management requested. Thus the below kernel oops may be
 generated if a device is accessed after the runtime power management
 timeout. This patch fixes the problem by requesting runtime power
 management in omap2_mcspi_set_cs().
 
 [   13.933959] Unhandled fault: external abort on non-linefetch (0x1028) at 
 0xfa09802c
 [   13.940490] pgd = cfb38000
 [   13.946594] [fa09802c] *pgd=48011452(bad)
 [   13.952758] Internal error: : 1028 [#1] PREEMPT ARM
 [   13.958862] Modules linked in: tsc2005(+) omap_sham twl4030_wdt omap_wdt
 [   13.965332] CPU: 0 PID: 183 Comm: modprobe Not tainted 4.2.0-rc1+ #363
 [   13.971801] Hardware name: Nokia RX-51 board
 [   13.978302] task: cf572300 ti: cb1f2000 task.ti: cb1f2000
 [   13.984924] PC is at omap2_mcspi_set_cs+0x44/0x4c
 [   13.991485] LR is at spi_set_cs+0x5c/0x60

Please don't paste entire backtraces into commit messages, they are very
large and almost entirely noise (for example in this case the entire
explanation is in the commit message itself).  If you feel a backtrace
helps clarify things then please present an *edited* highlight of the
relevant sections.
present


signature.asc
Description: Digital signature

Re: [PATCH 1/2] V4 regmap: Use reg_sequence for multi_reg_write / register_patch

[Mark Brown]

On Tue, Jul 14, 2015 at 03:45:51PM +0100, Nariman Poushin wrote:

Please submit patches in the format covered in SubmittingPatches,
version information goes inside the [].

 Add support for writing sequences of registers / patches with specified
 delays (in microseconds). Logically separates the functionality using
 sequences of register writes from the functions that take register
 defaults, as adding a delay field on the reg_defaults can increase
 memory usage substantially.

This change doesn't do what the above changelog says.  It introduces a
new struct reg_sequence and updates the multi write and patch APIs to
use that but it doesn't implement any delay functionality.  Please
resend with a clearer changelog that describes why the struct is being
split out from the reg_defaults struct and makes it clear that this is
just a rename.  It's probably best to also defer the addition of the
delay field until the second patch where this function is actually
implemented.

 +/**
 + * Register / Value pairs for sequences of writes, incorporating an optional

Register/value.

 + * delay in microseconds.
 + *
 + * @reg: Register address.
 + * @def: Register default value.
 + * @delay_us: Delay in microseconds
 + */
 +
 +struct reg_sequence {

No blank line between the kerneldoc and the struct (as is the style for
other kernel code).


signature.asc
Description: Digital signature

Re: [alsa-devel] [PATCH 1/2] V4 regmap: Use reg_sequence for multi_reg_write / register_patch

[Mark Brown]

On Thu, Jul 16, 2015 at 04:45:25PM +0100, Nariman Poushin wrote:

 I will resend with a cover letter explaining the change from the previous
 patch set if that is the right thing to do.

No, that's fine.  If you want to fix something like that just reply to
the cover letter with the extra information.


signature.asc
Description: Digital signature

Re: [PATCH 2/2 V5] regmap: Apply optional delay in multi_reg_write/register_patch

[Mark Brown]

On Thu, Jul 16, 2015 at 04:36:22PM +0100, Nariman Poushin wrote:

 +
 + if (regs[i].delay_us)
 + udelay(regs[i].delay_us);

This is a bit funky.  While Takashi is correct that we could be running
in a spinlock equally this will mean that we could end up with some
really long busy waits.  It feels like we should at least make an effort
to complain about that, print a warning or something.


signature.asc
Description: Digital signature

Re: next-20150629 build: 1 failures 74 warnings (next-20150629)

[Mark Brown]

On Mon, Jun 29, 2015 at 11:36:17AM +0100, Build bot for Mark Brown wrote:

Today's -next fails to build an ARM allmodconfig due to:

 ../drivers/watchdog/omap_wdt.c:288:18: error: 'omap_wdt' undeclared (first 
 use in this function)

which is introduced by watchdog: omap_wdt: early_enable module
parameter which adds the above reference to a nonexistant variable.
Presumably the patch was generated against something sufficiently old to
lack watchdog: omap: put struct watchdog_device into driver data which
appears to be where it was removed.


signature.asc
Description: Digital signature

Re: [PATCH v3] ASoC: davinci-mcasp: Choose PCM driver based on configured DMA controller

[Mark Brown]

On Tue, Jun 02, 2015 at 11:09:34PM +0300, Jyri Sarha wrote:
 Find the configured DMA controller by asking for a DMA channel in the
 probe phase and releasing it right after. The controller device can be
 found via the dma_chan struct and the controller is recognized from
 the compatible property of its device node. The patch assumes EDMA if
 there is no device node.

Applied, thanks.


signature.asc
Description: Digital signature

Re: next-20150604 build: 2 failures 37 warnings (next-20150604)

[Mark Brown]

On Fri, Jun 05, 2015 at 07:49:00PM +0900, Greg Kroah-Hartman wrote:
 On Thu, Jun 04, 2015 at 10:20:26AM -0700, Tony Lindgren wrote:

  Greg, there's now commit 9809889c708e in tty-linus and commit
  9e91597f2423 in tty-next.

 Yes, I can't go back and remove the one in tty-next, I'm guessing when
 they are merged there is an issue.  I'll look into that after Linus
 pulls in my tty-linus tree.

Yeah, git isn't always spectacularly good at resolving add/add if
there's context changes between two things in a merge - it sometimes
ends up double adding things.


signature.asc
Description: Digital signature

Re: next-20150604 build: 2 failures 37 warnings (next-20150604)

[Mark Brown]

On Thu, Jun 04, 2015 at 10:20:26AM -0700, Tony Lindgren wrote:
 * Tony Lindgren t...@atomide.com [150604 10:11]:

  Sounds like I'm using an old commit in one of my pending branches,
  will check immediately.

 Greg, there's now commit 9809889c708e in tty-linus and commit
 9e91597f2423 in tty-next.

Thanks for investigating!


signature.asc
Description: Digital signature

Re: next-20150604 build: 2 failures 37 warnings (next-20150604)

[Mark Brown]

On Thu, Jun 04, 2015 at 05:47:05PM +0100, Build bot for Mark Brown wrote:

   arm-allmodconfig
 ../drivers/tty/serial/8250/8250_omap.c:586:20: error: redefinition of 
 'omap8250_irq'

Current -next fails to build an ARM allmodconfig (and possibly other
things) due to 9e91597f24234 and 9809889c708eb (serial: 8250_omap:
provide complete custom startup  shutdown callbacks) which appear to be
the same commit that's been applied twice somehow.  Not sure what's
going on there...


signature.asc
Description: Digital signature

Re: [PATCH RFC v2 2/7] ASoC: hdmi: Remove obsolete dummy HDMI codec

[Mark Brown]

On Tue, May 26, 2015 at 09:59:06PM +0300, Jyri Sarha wrote:

 -#ifdef CONFIG_OF
 -static const struct of_device_id hdmi_audio_codec_ids[] = {
 - { .compatible = linux,hdmi-audio, },
 - { }
 -};
 -MODULE_DEVICE_TABLE(of, hdmi_audio_codec_ids);
 -#endif

There is actually a binding document, you should remove that too.


signature.asc
Description: Digital signature

Re: [PATCH RFC v2 1/7] ASoC: core: If component doesn't have of_node use parent's node instead

[Mark Brown]

On Tue, May 26, 2015 at 09:59:05PM +0300, Jyri Sarha wrote:
 If an ASoC component device does not have a device tree node, use its
 parent's node instead, when looking for a matching DAI based on a
 device tree reference.

Applied, thanks.


signature.asc
Description: Digital signature

Re: [PATCH] ASoC: davinci-mcasp: Choose PCM driver based on configured DMA controller

[Mark Brown]

On Tue, Jun 02, 2015 at 01:31:28PM +0300, Peter Ujfalusi wrote:
 On 06/02/2015 11:58 AM, Jyri Sarha wrote:

  +enum {
  +   MCASP_EDMA = 1,

 Why start from 1?

It's a fairly common style to ensure that zero initialised structures
don't have a default value.  Don't know if that's the intent here or
not.


signature.asc
Description: Digital signature

Re: [PATCH 11/13] spi: omap2-mcspi: Support for deferred probing when requesting DMA channels

[Mark Brown]

On Tue, May 26, 2015 at 04:26:06PM +0300, Peter Ujfalusi wrote:
 Switch to use ma_request_slave_channel_compat_reason() to request the DMA
 channels. Only fall back to pio mode if the error code returned is not
 -EPROBE_DEFER, otherwise return from the probe with the -EPROBE_DEFER.

Acked-by: Mark Brown broo...@kernel.org


signature.asc
Description: Digital signature

Re: [PATCH 11/13] spi: omap2-mcspi: Support for deferred probing when requesting DMA channels

[Mark Brown]

On Wed, May 27, 2015 at 02:15:12PM +0300, Peter Ujfalusi wrote:

 I have put the maintainers of the relevant subsystems as CC in the commit
 message and sent the series to all of the mailing lists. This series was
 touching 7 subsystems and I thought not spamming every maintainer with all the
 mails might be better.

You need to at least include people on the cover letter, otherwise
they'll have no idea what's going on.


signature.asc
Description: Digital signature

Re: [PATCH 13/13] ASoC: omap-pcm: Switch to use dma_request_slave_channel_compat_reason()

[Mark Brown]

On Tue, May 26, 2015 at 04:26:08PM +0300, Peter Ujfalusi wrote:
 dmaengine provides a wrapper function to handle DT and non DT boots when
 requesting DMA channel. Use that instead of checking for of_node in the
 platform driver.

Acked-by: Mark Brown broo...@kernel.org


signature.asc
Description: Digital signature

Re: [PATCH 11/13] spi: omap2-mcspi: Support for deferred probing when requesting DMA channels

[Mark Brown]

On Tue, May 26, 2015 at 04:26:06PM +0300, Peter Ujfalusi wrote:

 Switch to use ma_request_slave_channel_compat_reason() to request the DMA
 channels. Only fall back to pio mode if the error code returned is not
 -EPROBE_DEFER, otherwise return from the probe with the -EPROBE_DEFER.

I've got two patches from a patch series here with no cover letter...
I'm guessing there's no interdependencies or anything?  Please always
ensure that when sending a patch series everyone getting the patches can
tell what the series as a whole looks like (if there's no dependencies
consider posting as individual patches rather than a series).


signature.asc
Description: Digital signature

Re: [PATCH 0/4] spi: omap2-mcspi: Fixes for recent updates

[Mark Brown]

On Sat, May 23, 2015 at 09:13:41PM -0500, Michael Welling wrote:
 The recent update of the OMAP2 McSPI driver left some unresolved issues.
 These patches should take care them and again allow for GPIO chip selects
 and native chip selects.

Applied all, thanks.


signature.asc
Description: Digital signature

Re: [PATCH] spi: omap2-mcspi: Fix native cs with new set_cs

[Mark Brown]

On Thu, May 21, 2015 at 06:48:33PM -0500, Michael Welling wrote:

 So after reverting this patch I found there is a issue in that it is difficult
 to determine when a transfer is complete to properly drive the chipselect from
 within the transfer_one function.

Is unprepare_message() a suitable place here?  I've got a feeling the
answer is no...

 Then I figured that we could handle the case when GPIOs are being used with
 some conditional calls to omap2_mcspi_set_cs in the omap2_mcspi_work_one
 function.
 
 Near the beginning of the function I added:
   if (gpio_is_valid(spi-cs_gpio))
   omap2_mcspi_set_cs(spi, 0);
 
 Near the end of the function I added:
   if (gpio_is_valid(spi-cs_gpio))
   omap2_mcspi_set_cs(spi, 1);
 
 This makes GPIO chip select support work while leaving the native working
 as previous.
 
 Is this solution acceptible?

I think that's probably OK as well, it's not ideal though (and risky if
the chip select is routed somewhere...).  

 In the process of reviewing the changes I found a few other things that
 should be changed as well.

Please send fixes for these as separate patches (ideally without any
dependency on your new work so we can send them to Linus as fixes).

 Here you will see a delay that is already handled by the core spi driver:
 https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/tree/drivers/spi/spi-omap2-mcspi.c#n1166

I can't actually see that since I have no internet access right now!


signature.asc
Description: Digital signature

Re: [PATCH] spi: omap2-mcspi: Fix native cs with new set_cs

[Mark Brown]

On Thu, May 21, 2015 at 04:04:11PM -0500, Michael Welling wrote:

 Do you want to revert the patch and apply a new one or should I provide a
 patch that reverts the changes and fixes it all in one?

Can you please send me separate revert and re-add patches, that's
probably going to be easier to review.


signature.asc
Description: Digital signature

Re: [PATCH] spi: omap2-mcspi: Fix native cs with new set_cs

[Mark Brown]

On Wed, May 20, 2015 at 09:07:09PM -0500, Michael Welling wrote:

 My guess is that the set_cs needs to be called even when toggling as GPIO.

 How should I handle this?

It shouldn't be part of a set_cs() operation but rather part of the main
transfer operation.


signature.asc
Description: Digital signature

Re: [PATCH] ASoC: omap: fix up SND_OMAP_SOC_OMAP_ABE_TWL6040 dependency, again

[Mark Brown]

On Tue, May 19, 2015 at 02:47:32PM +0200, Arnd Bergmann wrote:
 I tried to fix this before and submitted a working patch, but after
 some discussion we came up with what seemed to be a nicer solution,
 resulting in commit 3d4cf65e2d (ASoC: omap: fix up
 SND_OMAP_SOC_OMAP_ABE_TWL6040 dependency). Unfortunately, that
 version was incomplete, and we still get this build error:

Applied, thanks.


signature.asc
Description: Digital signature

Re: [PATCH] ASoC: rx51: use flags argument of devm_gpiod_get to set direction

[Mark Brown]

On Tue, May 19, 2015 at 09:48:08AM +0200, Uwe Kleine-König wrote:
 Since 39b2bbe3d715 (gpio: add flags argument to gpiod_get*() functions)
 which appeared in v3.17-rc1, the gpiod_get* functions take an additional
 parameter that allows to specify direction and initial value for output.

Applied, thanks.


signature.asc
Description: Digital signature

Re: [PATCH] ASoC: rx51: use flags argument of devm_gpiod_get to set direction

[Mark Brown]

On Tue, May 19, 2015 at 11:16:50AM +0300, Jarkko Nikula wrote:

 I don't think Fixes tag is justified. Otherwise than that

Yes, I don't see any bug fixing here - am I missing something?


signature.asc
Description: Digital signature

Re: [PATCH 5/5] ASoC: omap-hdmi-audio: Fix invalid combination of DM_INH and CA

[Mark Brown]

On Wed, Apr 22, 2015 at 04:23:01PM +0300, Jyri Sarha wrote:
 From: Misael Lopez Cruz misael.lo...@ti.com
 
 DM_INH = 1 (stereo downmix prohibited) and CA = 0x00 (Channel
 Allocation: FR, FL) is an invalid combination according to the
 HDMI Compliance Test 7.31 Audio InfoFrame.

Acked-by: Mark Brown broo...@kernel.org


signature.asc
Description: Digital signature

Re: [PATCH 4/5] ASoC: omap-hdmi-audio: Force channel allocation only for OMAP4

[Mark Brown]

On Wed, Apr 22, 2015 at 04:23:00PM +0300, Jyri Sarha wrote:
 From: Misael Lopez Cruz misael.lo...@ti.com
 
 There is a constraint in the OMAP4 HDMI IP that requires to use
 the 8-channel code when transmitting more than two channels.

Acked-by: Mark Brown broo...@kernel.org


signature.asc
Description: Digital signature

Re: LDP: next-20150402: twl4030 regression?

[Mark Brown]

On Mon, Apr 06, 2015 at 08:21:49AM -0500, Nishanth Menon wrote:

 https://github.com/nmenon/kernel-test-logs/blob/next-20150401/omap2plus_defconfig/ldp.txt
 looks clean, but

 https://github.com/nmenon/kernel-test-logs/blob/next-20150402/omap2plus_defconfig/ldp.txt#L488
 seems to have picked up a regression in the regulator driver.

Please provide actual bug reports if you're trying to report something -
at least a description of the problem you're seeing and some attempt at
analysis.  For example say what the test was, and if there's logs paste
the relevant section into the e-mail.  Just randomly pasting a couple of
web links in and saying there's a problem isn't helpful.

Please also CC people responsible for relevant drivers and systems.


signature.asc
Description: Digital signature

Re: LDP: next-20150402: twl4030 regression?

[Mark Brown]

On Mon, Apr 06, 2015 at 02:58:29PM +0100, Russell King - ARM Linux wrote:
 On Mon, Apr 06, 2015 at 08:53:36AM -0500, Nishanth Menon wrote:

   at least a description of the problem you're seeing and some attempt at

  Test was a simple boot test. There seems to be a lockdep reported at the
  very least in the log provided (see
  https://github.com/nmenon/kernel-test-logs/blob/next-20150402/omap2plus_defconfig/ldp.txt#L488
  ).

 I think what Mark is trying to say is to include a fuller description of
 the problem, and don't expect people to fire up their web browser to get
 a basic overview of what the problem is.

Yes, indeed.  I hadn't actually opened the links, I might've got round
to it later on.

 My guess is that the problem _appears_ to be that someone's added a call
 to debug_check_no_locks_held() into schedule_hrtimeout_range_clock()
 without considering what this means.

 What it means is that you can't now use usleep_range() from within any
 driver probe function - which is absolutely absurd.

I can't think of any regulator side changes which might be relevant in
that period.  It's possible that there might be something in the MFD I
guess.


signature.asc
Description: Digital signature

Re: [PATCH] ASoC: davinci-mcasp: Index ruledata in drvdata with substream-stream

[Mark Brown]

On Fri, Mar 27, 2015 at 11:47:51AM +0200, Jyri Sarha wrote:
 The serializer direction definitions runs from 1 to 2, which does not
 suite the purpose. The substream-stream is perfect for the purpose
 and should have been used from the beginning.

Applied, thanks.


signature.asc
Description: Digital signature

Re: [PATCH] ASoC: davinci-evm: drop un-necessary remove function

[Mark Brown]

On Thu, Mar 26, 2015 at 03:38:25PM +0200, Jyri Sarha wrote:
 From: Manish Badarkhe manis...@ti.com
 
 As davinci card gets registered using 'devm_' api
 there is no need to unregister the card in 'remove'
 function.
 Hence drop the 'remove' function.

Not only that but the current remove function creates a double free.
Applied, thanks.


signature.asc
Description: Digital signature

Re: [PATCH v3] ASoC: davinci-mcasp: Set rule constraints if implicit BCLK divider is used

[Mark Brown]

On Fri, Mar 20, 2015 at 01:31:08PM +0200, Jyri Sarha wrote:
 Set rule constraints to allow only combinations of sample-rate,
 sample-format, and channels counts that can be played/captured with
 reasonable sample-rate accuracy.

Applied, thanks.


signature.asc
Description: Digital signature

Re: Patch to parameterize DMA_MIN_BYTES for omap2-mcspi

[Mark Brown]

On Fri, Mar 20, 2015 at 03:10:04PM +0200, Peter Ujfalusi wrote:
 On 03/19/2015 08:51 PM, Mark Brown wrote:

  You probably need both - there's often a hard limit where the FIFO size
  in the hardware becomes a limit for DMA as well as a soft limit on top
  of that for performance reasons.

 The FIFO is only going to be enabled when the DMA is used for transfer so we
 should have some lower limit for the PIO/DMA threshold. The FIFO in McSPI is a

Right, that's pretty much what I'm trying to say.

 tricky one anyways, since it has only one FIFO but several channels and the
 FIFO can be enabled for only one channel, if it is enabled for more channels
 it is not going to be used by either channel.

Not sure I follow this - how does this whole multiple channels thing
work for SPI exactly?


signature.asc
Description: Digital signature

Re: Patches to bind the SGTL5000 chip to AM33XX McASP

[Mark Brown]

On Thu, Mar 19, 2015 at 01:48:08PM -0400, Greg Knight wrote:

 Is there a straightforward way we could enable the clock on-demand -
 when we're communicating with the codec/mixer or actually using the
 audio subsystem - while keeping it disabled when not in use? We don't
 expect to be using the audio subsystem frequently.

This is what the clock API is there for - the driver can enable the
clock whenver it needs it and disable it when it doesn't.


signature.asc
Description: Digital signature

Re: [alsa-devel] Patches to bind the SGTL5000 chip to AM33XX McASP

[Mark Brown]

On Thu, Mar 19, 2015 at 08:06:15PM +0200, Nikolay Dimitrov wrote:

 Correct. And this is a big issue. As far as I know, the kernel drivers
 control separately the clock domains, and separately i2c devices, so
 the basic expectation on the kernel side is that there's no connection
 between these 2. In this specific case, because of the SGTL5000's

Not really - it's simply that it's the responsibility of the driver for
the device with the dependency to ensure that the dependency is
satisfied.  There are no general rules about what the requirements of
devices are.

 2. Add hacks in the DAPM widgets that add control to the codec's
 reference clocks. While this seems the preferred route to many, the
 general feeling is that such approach is not very welcome in upstream.

There is no need for any hacks, we already know when register accesses
are happening so we can easily add clock enables there (by moving the
code from the MMIO layer to the generic layer, this is the first device
with this unusual requirement).

 3. Add explicit support in the kernel's audio subsystem for
 dependencies between i2c devices and clocks, maybe via DAPM clock
 widget or something like this. Provided that the DAPM graphs are
 defined properly, this will work out-of-the-box for all use cases,
 without the hacks (until we see even more twisted cases).

There's nothing audio specific about the idea of a device needing a
clock for register access, doing something at the audio level is the
wrong abstraction layer.


signature.asc
Description: Digital signature

Re: Patch to parameterize DMA_MIN_BYTES for omap2-mcspi

[Mark Brown]

On Thu, Mar 19, 2015 at 01:05:01AM -0400, Greg Knight wrote:

 What is the process for getting this upstreamed?

Please submit the patch following the process in
Documentation/SubmittingPatches.


signature.asc
Description: Digital signature

Re: [PATCH v2] ASoC: davinci-mcasp: Set rule constraints if implicit BCLK divider is used

[Mark Brown]

On Thu, Mar 19, 2015 at 10:51:09AM +0200, Jyri Sarha wrote:
 Set rule constraints to allow only combinations of sample-rate,
 sample-bits, and channels that can be played/captured with reasonable
 sample-rate accuracy.

This doesn't apply against current code, please check and resend.


signature.asc
Description: Digital signature

Re: Patches to bind the SGTL5000 chip to AM33XX McASP

[Mark Brown]

On Thu, Mar 19, 2015 at 01:07:39AM -0400, Greg Knight wrote:

 These patches are based off of 3.14.31-ti-r49. What is the process for
 getting these merged?

Please submit patches using the process in SubmittingPatches, you need
to work against current development versions of the kernel rather than
an old vendor kernel.


signature.asc
Description: Digital signature

Re: Patch to parameterize DMA_MIN_BYTES for omap2-mcspi

[Mark Brown]

On Thu, Mar 19, 2015 at 09:16:31AM -0400, Greg Knight wrote:
 Will refer to that documentation and update the SPI docs before resubmitting.

Please don't top post on kernel lists, reply in line (deleting any
unneeded context) so people have context for what you are talking about.
This makes discussions much easier to follow.


signature.asc
Description: Digital signature

Re: [PATCH 02/35 linux-next] regulator: constify of_device_id array

[Mark Brown]

On Tue, Mar 17, 2015 at 06:26:17PM +0100, Fabian Frederick wrote:
  On 17 March 2015 at 18:19 Mark Brown broo...@kernel.org wrote:

  Well, another thing to do if there's no dependencies is to just send
  each patch separately - there's no real relationship between the
  patches.  This also avoids the issue.

 Such trivial updates are generally applied by David or Greg
 after receiving some ack. That's the reason for the patchset :)

It's pretty disappointing if that's the case, though it might be
something connected with the submission style - for most of the kernel
patches would normally be handled directly by the maintainer.


signature.asc
Description: Digital signature

Re: Patch to parameterize DMA_MIN_BYTES for omap2-mcspi

[Mark Brown]

On Thu, Mar 19, 2015 at 01:28:00PM -0400, Greg Knight wrote:

 Changing DMA_MIN_BYTES to, say, dma_min_time_ms sounds reasonable to
 me. I don't know how to compute it completely accurately as some SPI

You probably need both - there's often a hard limit where the FIFO size
in the hardware becomes a limit for DMA as well as a soft limit on top
of that for performance reasons.

 implementations I've seen seem to like to inject little delays between
 bytes for some reason, but a reasonable enough estimate should just be
 spi_transfer_time_ms = (bits * 1000) / spi_clock_speed.

 I would like to have the ability to configure it without a kernel
 recompile, though. Would a kernel param (module_param) be acceptable for
 this application?

Exposing it as a sysfs or debugfs file might make more sense if you were
going to do this.  Is that required after merge though?  It seems like
this is something where we've perhaps been tuning the wrong parameter
(transfer size instead of runtime), or need to investigate dynamic
tuning.


signature.asc
Description: Digital signature

Re: [PATCH 02/35 linux-next] regulator: constify of_device_id array

[Mark Brown]

On Tue, Mar 17, 2015 at 06:10:12PM +0100, Fabian Frederick wrote:

 Thanks Mark, I used a --cc-cmd script by Joe Perches with
 git send-email which uses --nom for cover-letter.

 This limits recipients to mailing lists: 37 instead of 123 in this case.
 I guess it's better to reduce the noise :)

Well, another thing to do if there's no dependencies is to just send
each patch separately - there's no real relationship between the
patches.  This also avoids the issue.


signature.asc
Description: Digital signature

Re: [PATCH 02/35 linux-next] regulator: constify of_device_id array

[Mark Brown]

On Mon, Mar 16, 2015 at 08:17:08PM +0100, Fabian Frederick wrote:
 of_device_id is always used as const.
 (See driver.of_match_table and open firmware functions)

Applied, thanks.  Please remember to always include people in the cover
letter for patch serieses so they know what's going on with regard to
dependencies and so on.


signature.asc
Description: Digital signature

Re: [PATCH 0/2] regulator: TPS659038: Fixes for REGEN2 and REGEN3

[Mark Brown]

On Tue, Mar 17, 2015 at 03:56:03PM +0530, Keerthy wrote:
 The series fixes couple of issues in the driver w.r.t TPS659038 PMICs.
 1) Correcting the REGEN2_CTRL address.
 2) REGEN3 is not present hence adding a check not to register for TPS659038.

Applied both, thanks.  

   regulator: palmas: Correct TPS659038 register definition for REGEN2
   regulator: Palmas: Add has_regen3 check for TPS659038

Please try to keep the style of the subject lines consistent...


signature.asc
Description: Digital signature

Re: [PATCH] regulator: tps65910: Add missing #include linux/of.h

[Mark Brown]

On Sun, Mar 15, 2015 at 02:03:50PM +0100, Geert Uytterhoeven wrote:
 drivers/regulator/tps65910-regulator.c: In function 
 ‘tps65910_parse_dt_reg_data’:
 drivers/regulator/tps65910-regulator.c:1018: error: implicit declaration of 
 function ‘of_get_child_by_name’

Applied, thanks.


signature.asc
Description: Digital signature