Re: [PATCH] regulator: s2m,s5m: constify regulator_ops structures

2015-12-23 Thread Mark Brown
On Wed, Dec 23, 2015 at 08:20:43AM +0100, Julia Lawall wrote:
> On Wed, 23 Dec 2015, Mark Brown wrote:

> > On Sat, Dec 19, 2015 at 03:58:00PM +0100, Julia Lawall wrote:
> > > The regulator_ops structures are never modified, so declare them as const.

> > > Done with the help of Coccinelle.

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

> It works fine for me on today's linux-next (80c75a0f1d).  What goes wrong?

It just doesn't apply against my tree.  Presumably there's a change
somewhere else in -next (probably the MFD tree).


signature.asc
Description: PGP signature


Re: [PATCH] regulator: s2m,s5m: constify regulator_ops structures

2015-12-22 Thread Mark Brown
On Sat, Dec 19, 2015 at 03:58:00PM +0100, Julia Lawall wrote:
> The regulator_ops structures are never modified, so declare them as const.
> 
> Done with the help of Coccinelle.

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


signature.asc
Description: PGP signature


Re: [PATCH 2/3] dt-bindings: regulator/mfd: Reorganize S5M8767 bindings

2015-12-04 Thread Mark Brown
On Fri, Dec 04, 2015 at 10:10:04AM +0900, Krzysztof Kozlowski wrote:
> The regulator/s5m8767-regulator.txt duplicates some of the information
> about bindings with old mfd/s2mps11.txt. Now common part exists entirely
> in mfd/samsung,sec-core.txt so:

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


signature.asc
Description: PGP signature


Re: [PATCH 1/3] dt-bindings: regulator/clock/mfd: Reorganize S2MPS-family bindings

2015-12-04 Thread Mark Brown
On Fri, Dec 04, 2015 at 10:10:03AM +0900, Krzysztof Kozlowski wrote:
> Bindings for Samsung S2M and S5M family PMICs are in mess. They are
> spread over different files and subdirectories in a non-consistent way.
> The devices and respective drivers for them share a lot in common so
> everything could be organized in a more readable way.

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


signature.asc
Description: PGP signature


Re: [PATCH 3/3] dt-bindings: regulator/mfd: Reorganize S2MPA01 bindings

2015-12-04 Thread Mark Brown
On Fri, Dec 04, 2015 at 10:10:05AM +0900, Krzysztof Kozlowski wrote:
> The mfd/s2mpa01.txt duplicates some of the information about bindings
> with old mfd/s2mps11.txt. Now common part exists entirely in
> mfd/samsung,sec-core.txt so:

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

>  - add company prefix to file name (regulator/samsung,s2mpa01.txt),

I'm not 100% convinced about these prefixes BTW, the duplication isn't
usually an issue within a subsystem.  They don't do any harm either
though.


signature.asc
Description: PGP signature


Re: [PATCH 00/10] ARM: s3c64xx multiplatform

2015-11-25 Thread Mark Brown
On Wed, Nov 25, 2015 at 05:06:45PM +0100, Arnd Bergmann wrote:
> I've posted the series before and we got a few people to test it
> the last time, though I think the touchscreen portion is still
> untested.
> 
> I'd really like to get this merged for 4.5 and would put it into
> a next/multiplatform branch unless there are any objections
> or concerns.

Is this a series with interdependencies or just a set of separate fixes?


signature.asc
Description: PGP signature


Re: [RESEND PATCH v5 2/3] regulator: s2mps11: add support for S2MPS15 regulators

2015-11-22 Thread Mark Brown
On Fri, Nov 20, 2015 at 04:07:52PM +0530, Alim Akhtar wrote:
> From: Thomas Abraham <thomas...@samsung.com>
> 
> The S2MPS15 PMIC is similar in functionality to S2MPS11/14 PMIC. It contains
> 27 LDO and 10 Buck regulators and allows programming these regulators via a
> I2C interface. This patch adds initial support for LDO/Buck regulators of
> S2MPS15 PMIC.

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


signature.asc
Description: PGP signature


Re: [PATCH v3 3/3] ASoC: samsung: pass filter function as pointer

2015-11-18 Thread Mark Brown
On Wed, Nov 18, 2015 at 03:26:41PM +0100, Arnd Bergmann wrote:
> As we are now passing the filter data as pointers to the drivers,
> we can take the final step and also pass the filter function the
> same way. I'm keeping this change separate, as there it's less
> obvious that this is a net win.

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


signature.asc
Description: PGP signature


Re: [PATCH v2] mfd: sec-core: Rename MFD and regulator names differently

2015-11-04 Thread Mark Brown
On Mon, Nov 02, 2015 at 09:36:09AM +0530, Alim Akhtar wrote:
> Currently S2MPSXX multifunction device is named as *-pmic,
> and these MFDs also supports regulator as a one of its MFD cell which
> has the same name, because current name is confusing and we want to
> sort it out.

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


signature.asc
Description: PGP signature


Re: [PATCH v3 5/5] drivers/rtc/rtc-s5m.c: add support for S2MPS15 RTC

2015-10-27 Thread Mark Brown
On Wed, Oct 28, 2015 at 10:29:56AM +0900, Krzysztof Kozlowski wrote:

> If that's true, then don't add new compatibles, new names etc. Re-use.
> No new code needed, no changes needed. Keep it simple.

Well, it depends - it can be useful to get the information about it
being a different part into DT so that if in future we realise that
there is some difference (perhaps a bug workaround even if the IP is
intended to be the same).  Though in the case of a MFD that information
can be obtained from the MFD for the device.


signature.asc
Description: PGP signature


Re: [PATCH v2] spi: bitbang: Replace spinlock by mutex

2015-08-31 Thread Mark Brown
On Mon, Aug 31, 2015 at 03:54:13PM +0800, Nicolas Boichat wrote:
> On Mon, Aug 17, 2015 at 11:52 AM, Nicolas Boichat  
> wrote:

> Any update on this patch? It still applies on for-next but happy to
> resend if necessary (patchwork:
> https://patchwork.kernel.org/patch/7023091/).

Please don't send content free pings and please allow a reasonable
amount of time for review.


signature.asc
Description: Digital signature


Re: [PATCH 1/3] spi: bitbang: Replace spinlock by mutex when calling chipselect

2015-08-14 Thread Mark Brown
On Wed, Aug 05, 2015 at 06:24:05PM +0800, Nicolas Boichat wrote:

 Anyway, the safer way to fix this would be to keep the
 prepare/unprepare functions, busy variable, and just protect it with a
 mutex instead of a spinlock...

OK, that seems reasonable.


signature.asc
Description: Digital signature


Re: [PATCH 3/3] spi: s3c24xx: Convert spinlock to mutex

2015-08-04 Thread Mark Brown
On Tue, Aug 04, 2015 at 02:09:58PM +0800, Nicolas Boichat wrote:
 bitbang-lock is now a mutex: replace spinlock function calls
 by mutex functions.

This would need to be combined with the first patch, individual patches
shouldn't break the build.


signature.asc
Description: Digital signature


Re: [PATCH 1/3] spi: bitbang: Replace spinlock by mutex when calling chipselect

2015-08-04 Thread Mark Brown
On Tue, Aug 04, 2015 at 02:09:56PM +0800, Nicolas Boichat wrote:
 Enabling CONFIG_DEBUG_ATOMIC_SLEEP in kernel configuration, we get
 this warning in spi_gpio_setup:
 [1.177747] BUG: sleeping function called from invalid context at 
 drivers/gpio/gpiolib.c:1431
 [1.190182] in_atomic(): 1, irqs_disabled(): 128, pid: 1, name: swapper/0
 [1.196922] 3 locks held by swapper/0/1:

Please don't include entire stack traces in commit logs, they're
enormous and overwhelm the actual content (like here where the trace is
much bigger than the actual commit message).  If you feel the
information from the trace adds something please present *edited*
highlights instead.

 Actually, I'm not sure if I understand the existing code: why are we not
 waiting for busy to go down to 0, then call chipselect, instead of not calling
 it at all if the bus happens to be busy when we setup the device? With the
 current approach, it would be easy to just use an unconditional mutex_lock.

We shouldn't be blocking waiting for the bus to become free, that's not
how the interface works.

 Also, is it harmful to deactivate the newly setup device in spi_bitbang_setup,
 even if the bus is busy with another device? chipselect should be independent
 for each device (or is it not?). So I'm not clear why we need any locking at
 all...

If you assert chip select on one device while another device is still
being interacted with then the new device will see the traffic for the
old device and become confused.

 Anyway, this patch series does not change the existing behaviour, applies on
 top of broonie-sound/for-next, and, along with the 2 follow-up patches, was
 compile-tested on x86-64/arm (allyesconfig) and ppc44x (defconfig+SPI driver),
 and runtime-tested on an arm platform.

I'm not seeing enough analysis in the commit log of what's being locked
and why - I don't fully understand what the busy stuff is for either
(not that I've looked at the code in detail just now) but I think that's
going to be the key thing here.


signature.asc
Description: Digital signature


Re: [RFC PATCH 2/2] dt: spi: s3c64xx: add compatible to controller-data

2015-07-30 Thread Mark Brown
On Thu, Jul 30, 2015 at 10:24:37AM +0200, Michal Suchanek wrote:

 So my suggestion is to

 1) search of ofpart subnode in mtd node. If present read address and
 reg from it and search partitions as subnodes of the ofpart node. In
 this case unknown nodes can cause error.

 2) failing that issue a warning and try to parse ofpart partitions
 from the mtd node itself. In this case unknown nodes cannot cause
 error for compatibility with other drivers including the already
 exisitn s3c64xx controller-data node.

 The parser code can be the same for both cases and only operate on
 different node with a flag to reject unknown subnodes or not.

That seems reasonable to me.


signature.asc
Description: Digital signature


Re: [RFC PATCH 2/2] dt: spi: s3c64xx: add compatible to controller-data

2015-07-30 Thread Mark Brown
On Thu, Jul 30, 2015 at 09:50:51AM +0200, Boris Brezillon wrote:

 Since you have to patch your DTs anyway, how about putting your
 partitions in a subnode and patch the ofpart code to parse this subnode
 if it is present (see the following patch).

This is the best idea, yes - if we're changing the DT for the system
anyway then making a sane binding and using that seems better than
trying to mitigate problems with the old bindings.


signature.asc
Description: Digital signature


Re: [RFC PATCH 2/2] dt: spi: s3c64xx: add compatible to controller-data

2015-07-29 Thread Mark Brown
On Wed, Jul 29, 2015 at 06:19:24PM +0200, Michal Suchanek wrote:
 On 29 July 2015 at 16:00, Mark Brown broo...@kernel.org wrote:

  I can't tell from this commit message what the issue you're trying to
  fix is, sorry.  Nodes without compatible strings are entirely normal and
  don't need compatible strings.  It sounds like a bug in whatever other
  driver is becoming confused.

 The driver that gets confused is ofpart.

 The two-line patch to allow it to just ignore controller-data has been
 rejected on the basis that s3c64xx should use a compatible string
 because ofpart monopolizes all nodes without compatible which are
 children of a mtd device. Devicetrees containing such nodes that are
 not partitions are presumably invalid and should be rejected when
 ofpart is compiled into the kernel.

That seems like an extremely limited binding, the normal thing here
would be to create a specifically named node to contain the collection
of subnodes like many PMICs do for their regulators.  As a fix I'd
suggest just silently ignoring nodes it can't understand, or printing a
warning if that's a serious issue.

  + if (!of_get_property(data_np, compatible, NULL) ||
  + strcmp(of_get_property(data_np, compatible, NULL),
  +samsung,s3c-controller-data))
  + dev_err(spi-dev, child node 'controller-data' does not 
  have correct compatible\n);

  This will break all existing users which is not acceptable for
  mainline, we need to preserve compatibility with existing device trees.

 It will not break anything. It will just spam dmesg.

I'm confused - if all this change does is to spam dmesg then what's the
point?


signature.asc
Description: Digital signature


Re: [RFC PATCH 2/2] dt: spi: s3c64xx: add compatible to controller-data

2015-07-29 Thread Mark Brown
On Wed, Jul 29, 2015 at 08:21:34PM +0200, Michal Suchanek wrote:
 On 29 July 2015 at 19:16, Mark Brown broo...@kernel.org wrote:

  It will not break anything. It will just spam dmesg.

  I'm confused - if all this change does is to spam dmesg then what's the
  point?

 Presumably when your SPI NOR flash fails to probe this message will be
 just above and you will look into the binding doc and add the
 compatible.

If you're looking to add a warning message when the flash device fails
to probe then add that in the flash driver when it detects an error,
this will cause needless noise for everyone else using this controller
purely to work around the broken binding.

And like I say compatible really seems like it isn't an appropriate
property here.


signature.asc
Description: Digital signature


Re: [RFC PATCH 2/2] dt: spi: s3c64xx: add compatible to controller-data

2015-07-29 Thread Mark Brown
On Wed, Jul 29, 2015 at 12:19:57PM +0200, Michal Suchanek wrote:

Please use subject lines matching the style for the subsytsem so people
can spot that the patch is in some way relevant.

 The controller-data subnode has no compatible. This can lead to other
 drivers getting confused by it. Add a compatible to make devicetreee
 unambiguous.

I can't tell from this commit message what the issue you're trying to
fix is, sorry.  Nodes without compatible strings are entirely normal and
don't need compatible strings.  It sounds like a bug in whatever other
driver is becoming confused.

 + if (!of_get_property(data_np, compatible, NULL) ||
 + strcmp(of_get_property(data_np, compatible, NULL),
 +samsung,s3c-controller-data))
 + dev_err(spi-dev, child node 'controller-data' does not have 
 correct compatible\n);

This will break all existing users which is not acceptable for
mainline, we need to preserve compatibility with existing device trees.


signature.asc
Description: Digital signature


Re: [PATCH v2 2/2] spi: s3c64xx: add more debug prints.

2015-07-28 Thread Mark Brown
On Tue, Jul 28, 2015 at 09:37:03AM -, Michal Suchanek wrote:
 The SPI transfers can mysteriously fail so add more debug prints about
 SPI parameters set by the driver.
 
 The hardware specific SPI driver is the only place where the programmed
 SPI parameters are known so there is no other reasonable place for these
 prints.

For timing things there should already be enough information in the
trace points with better resolution.


signature.asc
Description: Digital signature


Re: [PATCH v2 1/4] mfd: max77686: Don't suggest in binding to use a deprecated property

2015-07-27 Thread Mark Brown
On Mon, Jul 27, 2015 at 12:28:07PM +0200, Javier Martinez Canillas wrote:
 On 07/20/2015 12:12 PM, Javier Martinez Canillas wrote:

  This PMIC uses a single I2C address for all the regulators and these are
  controlled by writing to different I2C register addresses. So the regulator
  nodes don't have a reg property in this case.

  By looking at other regulators bindings, besides the generic regulator.txt
  and fixed-regulator.txt DT bindings, there are only 5 (out of 40) that use
  the node-name@unit-address convention mentioned in the ePAPR document.

  AFAICT all these are for regulators that are actually in different addresses
  but I could be wrong so let's see what Mark says.

 Any opinions on this?

I just don't care, this is just syntactic noise which has no practical
meaning as far as I can tell.


signature.asc
Description: Digital signature


Re: [PATCH] regulator: s2mps11: Fix GPIO suspend enable shift wrapping bug

2015-06-24 Thread Mark Brown
On Wed, Jun 24, 2015 at 01:02:28PM +0900, Krzysztof Kozlowski wrote:
 2015-05-28 23:44 GMT+09:00 Mark Brown broo...@kernel.org:
  On Wed, May 27, 2015 at 12:22:08PM +0900, Krzysztof Kozlowski wrote:
  Status of enabling suspend mode for regulator was stored in bitmap-like
  long integer.

  Applied, thanks.

 Mark, what happened with this patch? I can't find it in your tree.

I don't know.  If it's not there it's not there and you'll need to
resend, sorry.  There were some problems with error handling for bad
networks when pushing things but it looks like this was after I fixed
those.


signature.asc
Description: Digital signature


Re: [PATCH RESEND] regulator: s2mps11: Fix GPIO suspend enable shift wrapping bug

2015-06-24 Thread Mark Brown
On Wed, Jun 24, 2015 at 07:48:43PM +0900, Krzysztof Kozlowski wrote:
 From: Krzysztof Kozlowski k.kozlow...@samsung.com
 
 Status of enabling suspend mode for regulator was stored in bitmap-like
 long integer.

Applied, thanks.


signature.asc
Description: Digital signature


Re: [PATCH 10/11] spi: add more debug prints in s3c64xx

2015-06-04 Thread Mark Brown
On Thu, Jun 04, 2015 at 11:33:37AM +0200, Michal Suchanek wrote:
 On 4 June 2015 at 11:16, Mark Brown broo...@kernel.org wrote:

  Also for this patch (which just adds some trace) there isn't any clear
  reason for it to be sent as part of the series at all, it doesn't help
  deliver the functionality and doesn't depend on the rest of the series.

 I used this patch to add missing information to dmesg output so I
 could diagnose the SPI failures so I think it is relevant.

The fact that you used this to debug problems is not relevant to any fix
you might have developed for those problems; the fix can be explained
without needing to know how you got there.  Similarly the debugging is
hopefully useful in general without needing to know which particular
problem prompted it's creation.

 To print a clock name you AFAICT need this header. I think this is an
 abstraction problem in the clock framework. Fixing the abstraction
 problem with clock framework would only grow the list of recipients
 which is already so long you complain about it.

This is a bit of a warning sign that the series isn't very focused.
It's also just not good practice, sending patches with obvious problems
(especially obvious problems that aren't clearly flagged up as something
temporary) will frustrate reviewers and can often lead to other issues
being obscured.


signature.asc
Description: Digital signature


Re: [PATCH 10/11] spi: add more debug prints in s3c64xx

2015-06-04 Thread Mark Brown
On Thu, Jun 04, 2015 at 12:52:19PM +0200, Michal Suchanek wrote:

 There is no code interdependency with the other patches so if it's
 preferred I can send patches like this separately.

Yes, that's better practice.


signature.asc
Description: Digital signature


Re: [PATCH 10/11] spi: add more debug prints in s3c64xx

2015-06-04 Thread Mark Brown
On Wed, Jun 03, 2015 at 09:26:42PM -, Michal Suchanek wrote:
 The SPI NOR transfers mysteriously fail so add more debug prints about
 SPI transactions.

Please try to only send patches to relevant people - the list of
recipients for this is so large that it only barely fits on a single
screen in my mail client.

Also for this patch (which just adds some trace) there isn't any clear
reason for it to be sent as part of the series at all, it doesn't help
deliver the functionality and doesn't depend on the rest of the series.

 --- a/drivers/spi/spi-s3c64xx.c
 +++ b/drivers/spi/spi-s3c64xx.c
 @@ -18,6 +18,7 @@
  #include linux/interrupt.h
  #include linux/delay.h
  #include linux/clk.h
 +#include linux/clk-provider.h

Whatever you're doing here this indicates that there's a very big
abstraction problem going on.

 + pr_debug(%s %s %s waiting for %ims transferring %zubytes@%iHz,
 +  __func__, sdd-pdev ? dev_name(sdd-pdev-dev) : NULL,
 +  dev_name(sdd-master-dev),
 +  ms, xfer-len, sdd-cur_speed);

I'd say dev_dbg() but more generally this is just tracing things that
seem to be already covered by the trace points already present in the
core, the same goes for most of the rest of it.  If there's things
missing from the existing trace it seems better to add to it.


signature.asc
Description: Digital signature


Re: [PATCH 10/11] spi: add more debug prints in s3c64xx

2015-06-04 Thread Mark Brown
On Thu, Jun 04, 2015 at 11:30:15AM +0200, Geert Uytterhoeven wrote:
 On Thu, Jun 4, 2015 at 11:16 AM, Mark Brown broo...@kernel.org wrote:
  --- a/drivers/spi/spi-s3c64xx.c
  +++ b/drivers/spi/spi-s3c64xx.c
  @@ -18,6 +18,7 @@
   #include linux/interrupt.h
   #include linux/delay.h
   #include linux/clk.h
  +#include linux/clk-provider.h

  Whatever you're doing here this indicates that there's a very big
  abstraction problem going on.

 I guess it's needed for __clk_get_name(), which can be avoided by using
 the %pC printf format specifier instead.

Right, or by adding an externally usable API - the point is that using
internal functions in a client driver is a big red flag.


signature.asc
Description: Digital signature


Re: [PATCH] regulator: s2mps11: Fix GPIO suspend enable shift wrapping bug

2015-05-28 Thread Mark Brown
On Wed, May 27, 2015 at 12:22:08PM +0900, Krzysztof Kozlowski wrote:
 Status of enabling suspend mode for regulator was stored in bitmap-like
 long integer.

Applied, thanks.


signature.asc
Description: Digital signature


Re: [RFC PATCH 1/4] spi: spidev: Add Google SPI flash compatible string

2015-05-20 Thread Mark Brown
On Wed, May 20, 2015 at 01:21:53PM +0200, Javier Martinez Canillas wrote:

 The ChromeOS user-space just uses flashrom to send a raw stream of bytes
 via spidev to the SPI NOR flash chip. There is drivers/mtd/spi-nor/spi-nor.c
 but AFAIU there are some limitations when interfacing the flash through
 the MTD layer, for example there isn't a way to set the SPI flash write
 protection through MTD. But I'll do some investigation before re-spinning
 the patches.

OK, that's a bit concerning - perhaps what's needed here is to extend
MTD and so on (if only to find some way for it to coexist with spidev
for the time being)?

 BTW, the other rohm,dh2228fv compatible string in spidev added by commit
 8fad805bdc52 (spi: spidev: Add Rohm DH2228FV DAC compatible string), also
 does not have a documented DT binding so that should be fixed as well.

wv :)


signature.asc
Description: Digital signature


Re: [RFC PATCH 1/4] spi: spidev: Add Google SPI flash compatible string

2015-05-20 Thread Mark Brown
On Tue, May 19, 2015 at 03:34:11PM +0200, Javier Martinez Canillas wrote:
 Google Chromebooks have a SPI flash that is used to store firmware and
 different system parameters and data (i.e: Google Binary Block flags).

 ---
  drivers/spi/spidev.c | 1 +
  1 file changed, 1 insertion(+)

This is adding a binding with no documentation, documentation is
mandatory for all bindings.


signature.asc
Description: Digital signature


Re: [PATCH] ASoC: samsung: s3c24xx-i2s: Fix return value check in s3c24xx_iis_dev_probe()

2015-04-17 Thread Mark Brown
On Thu, Apr 16, 2015 at 08:18:02PM +0800, weiyj...@163.com wrote:
 From: Wei Yongjun yongjun_...@trendmicro.com.cn
 
 In case of error, the function devm_ioremap_resource() returns
 ERR_PTR() and never returns NULL. The NULL test in the return
 value check should be replaced with IS_ERR().

Applied, thanks.


signature.asc
Description: Digital signature


Re: [PATCH] ARM: S3C64XX: Use fixed IRQ bases to avoid conflicts on Cragganmore

2015-03-22 Thread Mark Brown
On Sun, Mar 22, 2015 at 10:40:41AM +, Charles Keepax wrote:
 There are two PMICs on Cragganmore, currently one dynamically assign
 its IRQ base and the other uses a fixed base. It is possible for the
 statically assigned PMIC to fail if its IRQ is taken by the dynamically
 assigned one. Fix this by statically assigning both the IRQ bases.
 
 Signed-off-by: Charles Keepax ckee...@opensource.wolfsonmicro.com

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

This probably wants to go to stable as well.


signature.asc
Description: Digital signature


Re: [PATCH 05/10] ARM: s3c64xx: enable sparse IRQ support

2015-03-21 Thread Mark Brown
On Sat, Mar 21, 2015 at 04:38:21PM +, Charles Keepax wrote:

 So adding a bit of an offset into the PMIC with the fixed IRQ
 base fixes the problem. Although I am not sure if it would be
 better to move both PMICs onto a fixed IRQ base so there is no
 chance of the clashing. Any thoughts?

Yes, that sounds like the safest thing.


signature.asc
Description: Digital signature


Re: [PATCH] ASoC: samsung: Enable SND_SIMPLE_CARD for Odroid X2/U3

2015-03-18 Thread Mark Brown
On Wed, Mar 18, 2015 at 11:52:19AM +0100, Sylwester Nawrocki wrote:
 Odroid X2/U3 sound support can now be specified in device tree using
 the simple card binding, make SND_SOC_ODROIDX2 select SND_SIMPLE_CARD
 to ensure there are always required drivers in place.

No, this isn't an actual dependency so it shouldn't be in Kconfig.


signature.asc
Description: Digital signature


Re: [PATCH 05/10] ARM: s3c64xx: enable sparse IRQ support

2015-03-17 Thread Mark Brown
On Sun, Mar 08, 2015 at 10:42:59PM +0100, Arnd Bergmann wrote:
 On Friday 06 March 2015 17:43:16 Charles Keepax wrote:

  This one appears to cause some problems with the IRQs on
  Cragganmore, I need to look into it a bit more but it looks like
  one of the PMICs can't allocate its IRQs:

  [0.961455] wm831x 1-0034: WM8311 revision C
  [0.965066] wm831x 1-0034: Failed to allocate IRQs: -17

  And the CODEC can't request its IRQ:

  [4.252735] arizona spi0.1: WM5102 revision C
  [4.269763] arizona spi0.1: Failed to request primary IRQ 263: -22

  Hopefully I can look into this a little more next week.

 My interpretation is that I mistakenly set the .nr_irqs value for
 craggamore to include all irqs that the board has, while the
 wm831x tries to allocate the irq descriptors itself and fails if
 they are already allocated. If that is the only problem here, it
 would get fixed by this change:

 diff --git a/arch/arm/mach-s3c64xx/mach-crag6410.c 
 b/arch/arm/mach-s3c64xx/mach-crag6410.c
 index f395a5617142..cbe353a5450e 100644
 --- a/arch/arm/mach-s3c64xx/mach-crag6410.c
 +++ b/arch/arm/mach-s3c64xx/mach-crag6410.c
 @@ -855,7 +855,7 @@ static void __init crag6410_machine_init(void)
  MACHINE_START(WLF_CRAGG_6410, Wolfson Cragganmore 6410)
   /* Maintainer: Mark Brown broo...@opensource.wolfsonmicro.com */
   .atag_offset= 0x100,
 - .nr_irqs= S3C64XX_NR_IRQS + 160,
 + .nr_irqs= S3C64XX_NR_IRQS,
   .init_irq   = s3c6410_init_irq,
   .map_io = crag6410_map_io,
   .init_machine   = crag6410_machine_init,

 The samsung-gpio driver does not allocate irq descriptors for itself
 though, otherwise we could make the S3C64XX_NR_IRQS number smaller.

That's not the only thing, I'm still seeing an issue even with the
change above.  The VICs are also complaining about preallocated
descriptors, but they just assume the descriptors were preallocated and
carry on happily.  I'd also expect this to be affecting both wm831xs but
it's only affecting the WM8311 on the base board, not the primary
WM8312, so this seems to be a red herring.

I rather suspect that the issue is at least partly that the interrupt
numbering is off - the CODEC has 24 interrupts allocated to it in
crag6410.h which is nowhere near enough and the PMICs have 32 each
rather than the 58 they need.  This is broken for the existing code too
so we should get a fix to make that right in before anything else.  A
trivial change to fix that doesn't seem to have helped though but I
probably just miscounted or something.


signature.asc
Description: Digital signature


Re: [PATCH 05/10] ARM: s3c64xx: enable sparse IRQ support

2015-03-08 Thread Mark Brown
On Fri, Mar 06, 2015 at 05:43:16PM +, Charles Keepax wrote:

 Cragganmore, I need to look into it a bit more but it looks like
 one of the PMICs can't allocate its IRQs:

 [0.961455] wm831x 1-0034: WM8311 revision C
 [0.965066] wm831x 1-0034: Failed to allocate IRQs: -17

 And the CODEC can't request its IRQ:

 [4.252735] arizona spi0.1: WM5102 revision C
 [4.269763] arizona spi0.1: Failed to request primary IRQ 263: -22

 Hopefully I can look into this a little more next week.

The second is a knock on from the first - the CODEC interrupt line is
connected to the PMIC so since the PMIC can't allocate its IRQ range the
CODEC can't request it.


signature.asc
Description: Digital signature


Re: [PATCH 02/10] ASoC: samsung/smartq: use dynamic registration

2015-03-03 Thread Mark Brown
On Mon, Mar 02, 2015 at 01:35:55PM +0100, Arnd Bergmann wrote:
 As a prerequisite for moving s3c64xx into multiplatform configurations,
 we need to change the smartq audio driver to stop using hardcoded
 gpio numbers from the header file, and instead pass the gpio data
 through platform_data.

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


signature.asc
Description: Digital signature


Re: [PATCH 00/10] ARM: s3c64xx multiplatform, help needed

2015-03-02 Thread Mark Brown
On Mon, Mar 02, 2015 at 01:35:53PM +0100, Arnd Bergmann wrote:

 Does anyone still have access to the hardware? I'm particularly
 interested in seeing this patch set get tested on smartq
 and on smdk6410, which have the majority of the hardware.

I and a bunch of other people still have Cragganmore, the ex-Wolfson
(added Charles here) people may also be able to locate a smdk6410.
may also be a


signature.asc
Description: Digital signature


Re: [PATCH] drivers: spi: fix compiler warning in spi-s3c64xx

2015-02-23 Thread Mark Brown
On Mon, Feb 23, 2015 at 12:30:46PM +, Andre Przywara wrote:
 The Exynos 7 arm64 support now allows the S3C64xx SPI driver to be
 compiled into an ARM64 kernel, so the cast from the [rt]x_dmach int
 variable to a void* in this driver now triggers a warning.
 Add a long cast to silence the compiler.

Applied, thanks.


signature.asc
Description: Digital signature


Re: [PATCH v2] ASoC: Samsung: add missing I2C/SPI dependencies

2015-02-21 Thread Mark Brown
On Wed, Feb 18, 2015 at 09:35:08PM +0100, Arnd Bergmann wrote:
 A few sound drivers for the samsung platforms are missing dependencies
 on I2C or SPI, which can lead to build errors like

Applied, thanks.


signature.asc
Description: Digital signature


Re: [PATCH 5/6] ASoC: samsung: Extend Snow driver to support max98089

2015-02-20 Thread Mark Brown
On Thu, Feb 19, 2015 at 09:44:08AM -0800, Doug Anderson wrote:
 On Thu, Feb 19, 2015 at 1:44 AM, Mark Brown broo...@kernel.org wrote:

 I think what you're suggesting is that here we should add a new
 compatible string google,snow-audio instead of adding
 google,snow-audio-max98089 here.  Then the sound node in the spring
 DTS would look like:

   compatible = google,snow-audio-max98089, google,snow-audio;

Yes.


signature.asc
Description: Digital signature


Re: [PATCH 5/6] ASoC: samsung: Extend Snow driver to support max98089

2015-02-19 Thread Mark Brown
On Wed, Feb 18, 2015 at 07:25:58PM +0100, Andreas Färber wrote:

  static const struct of_device_id snow_of_match[] = {
 + { .compatible = google,snow-audio-max98089, },
   { .compatible = google,snow-audio-max98090, },
   { .compatible = google,snow-audio-max98091, },
   { .compatible = google,snow-audio-max98095, },

Since we completely ignore the CODEC in the property might it not be
better to just add a plain old snow-audio compatible and bind to that,
that way we don't need these driver updates?  Just the snow bit should
be enough to know it's one of this class of machines.


signature.asc
Description: Digital signature


Re: [PATCH 0/4] ASoC: samsung: clean up references to outdated Kconfig symbols

2015-02-13 Thread Mark Brown
On Wed, Feb 11, 2015 at 10:21:38AM +0100, Paul Bolle wrote:

 TL;DR: what exactly should I resend and when?

This is disappointing; you're someone who's been contributing for quite
some time now, I'd really expect you to be faimilar with the process of
submitting patches and able to identify answers to your questions.

  What's posted doesn't apply against -next.

 That's because of trivial conflicts with commit a59aa180ea56 (ASoC:
 samsung: Replace depends on REGMAP_I2C with depends on I2C). That
 commit landed in next-20150127 (ie, eight days after I sent this short
 series). I guess I resolved these conflicts in the local version of
 these patches and promptly forgot about them, as they were so trivial.

For something like this I'm just not even going to look at what's
required to fix up by hand, sorry.

  As ever submit against the tree listed in MAINTAINERS

 git://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-next

 Which is not entirely obvious, since this series only touches files
 below sound/soc/samsung/. That directory is mentioned in MAINTAINERS
 under ARM/SAMSUNG EXYNOS ARM ARCHITECTURES and under SAMSUNG AUDIO
 (ASoC) DRIVERS. (Why does that directory need two entries?) Neither
 entry mention a repository.

Of course neither of those entries list me either yet you managed to
figure out that you should send the patch to me.  The same process that
helps you identify that things should be submitted to the subsystem
maintainers should allow you to identify where they're likely to try to
apply changes.  Or simply looking at how other similar patches get
merged.

 And now I wonder what I should resend: the three patches 2/4, 3/4, and
 4/4 with the trivial conflicts resolved or the entire series? And when
 do you prefer I resend?

Again this is something I'd really expect you to be familiar with.  As
ever if there are changes you would like to see integrated into the
kernel then please submit them using the normal patch submission
process.


signature.asc
Description: Digital signature


Re: [RESEND PATCH V3 13/15] ARM: dts: Exynos4 and Odroid X2/U3 sound device nodes update

2015-02-08 Thread Mark Brown
On Tue, Feb 03, 2015 at 03:06:20PM +0100, Sylwester Nawrocki wrote:
 Clock related properties are added to the Exynos4 I2S device nodes
 so they can be referred to as clock providers. Missing i2s_opclk1
 clock is added to the I2S0 node and clock properties are added
 to the MAX98090 codec node to allow it to control/read frequency
 of the MCLK clock directly.

Applied, thanks.


signature.asc
Description: Digital signature


Re: [RESEND PATCH V3 14/15] ARM: dts: Switch Odroid X2/U2 to simple-audio-card

2015-02-08 Thread Mark Brown
On Tue, Feb 03, 2015 at 03:06:21PM +0100, Sylwester Nawrocki wrote:
 Now when the CDCLK I2S output clock can be handled through the clock
 API the Odroid X2/U3 can be switched to the simple-audio-card DT binding.

Applied, thanks.


signature.asc
Description: Digital signature


Re: [PATCH 0/4] ASoC: samsung: clean up references to outdated Kconfig symbols

2015-02-08 Thread Mark Brown
On Sat, Feb 07, 2015 at 09:38:04AM +0100, Paul Bolle wrote:

 Patches 2/4, 3/4, and 4/4 didn't apply. Locally I carry them in a tree
 that constantly rebases on top of linux-next. So locally I carry these
 patches on top of next-20150204.

What's posted doesn't apply against -next.

 What tree would you like me to rebase this onto?

As ever submit against the tree listed in MAINTAINERS

   git://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-next


signature.asc
Description: Digital signature


Re: [PATCH 4/4] ASoC: samsung: remove outdated Kconfig references

2015-02-07 Thread Mark Brown
On Mon, Jan 19, 2015 at 11:43:02AM +0100, Paul Bolle wrote:
 The Kconfig symbols MACH_SMDKC110 and MACH_SMDKV210 were removed in
 v3.17. But the Kconfig entries for SND_SOC_SAMSUNG_SMDK_WM8580 and
 SND_SOC_SAMSUNG_SMDK_WM9713 still reference them.

This doesn't apply against current code.


signature.asc
Description: Digital signature


Re: [PATCH 0/4] ASoC: samsung: clean up references to outdated Kconfig symbols

2015-02-07 Thread Mark Brown
On Mon, Jan 19, 2015 at 11:41:07AM +0100, Paul Bolle wrote:
 A few series of ARM cleanups regarding S5PC100 and S5PV210 were included
 in v3.17. These series removed these five Kconfig symbols:
 MACH_AQUILA
 MACH_GONI
 MACH_SMDKC100
 MACH_SMDKC110
 MACH_SMDKV210
 
 This series, done on top of next-20150119, removes the remaining

If you are posting a patch series please ensure that all the patches in
the series are threaded in reply to the cover letter (or first patch if
there isn't one) so that mail clients can see to display them as a
series.


signature.asc
Description: Digital signature


Re: [PATCH 2/4] ASoC: samsung: Remove PCM support for WM8580 on SMDK

2015-02-07 Thread Mark Brown
On Mon, Jan 19, 2015 at 11:42:07AM +0100, Paul Bolle wrote:
 Commit 28c8331d386a (ARM: S5PV210: Remove support for board files)
 removed the Kconfig symbols MACH_SMDKC110 and MACH_SMDKV210. As a result
 the dependencies of SND_SOC_SMDK_WM8580_PCM can never be met. So remove
 the unbuildable SoC PCM Audio support for WM8580 on SMDK.

This doesn't apply against current code.


signature.asc
Description: Digital signature


Re: [PATCH 3/4] ASoC: samsung: MACH_SMDKC100 isn't supported anymore

2015-02-07 Thread Mark Brown
On Mon, Jan 19, 2015 at 11:42:45AM +0100, Paul Bolle wrote:
 Commit b8529ec1c1b0 (ARM: S5PC100: no more support S5PC100 SoC)
 removed Kconfig symbol MACH_SMDKC100. But there are still two references
 to that symbol left. These are now pointless. Remove them too.

This doesn't apply against current code.


signature.asc
Description: Digital signature


Re: [PATCH 1/4] ASoC: samsung: Remove goni or aquila with the WM8994

2015-02-07 Thread Mark Brown
On Mon, Jan 19, 2015 at 11:41:41AM +0100, Paul Bolle wrote:
 Commit 28c8331d386a (ARM: S5PV210: Remove support for board files)
 removed the Kconfig symbols MACH_GONI and MACH_AQUILA. As a result the
 dependencies of SND_SOC_GONI_AQUILA_WM8994 can never be met. So remove
 the unbuildable SoC I2S Audio support for AQUILA/GONI - WM8994.

Applied, thanks.


signature.asc
Description: Digital signature


Re: [RESEND PATCH V3 15/15] ARM: dts: Fix I2S1, I2S2 compatible for exynos4 SoCs

2015-02-06 Thread Mark Brown
On Fri, Feb 06, 2015 at 12:30:45PM +0100, Sylwester Nawrocki wrote:

 Thank you for merging it.  I hope you picked up the previous 2 as well,
 I couldn't see them in your tree.

I probably will if I don't hear anything but since they don't need to go
as a bug fix I can leave Kukjin a bit longer to say if he's OK with
that.


signature.asc
Description: Digital signature


Re: [RESEND PATCH V3 15/15] ARM: dts: Fix I2S1, I2S2 compatible for exynos4 SoCs

2015-02-05 Thread Mark Brown
On Tue, Feb 03, 2015 at 03:06:22PM +0100, Sylwester Nawrocki wrote:
 I2S1, I2S2 on Exynos4 SoC series have limited functionality compared
 to I2S0, samsung,s3c6410-i2s compatible should be used for them.

I've applied this even though I really shouldn't in order to get it in.
Since it's a bug fix for stable it should've been near the head of any
series it's part of not the very last patch in the series, and since
it's not really related to the rest of the series except in that it
relates to the same driver it should probably have been sent by itself
rather than mixed in with everything else.


signature.asc
Description: Digital signature


Re: [PATCH V3 13/15] ARM: dts: Exynos4 and Odroid X2/U3 sound device nodes update

2015-02-03 Thread Mark Brown
On Tue, Feb 03, 2015 at 03:05:36PM +0100, Sylwester Nawrocki wrote:
 On 03/02/15 14:11, Mark Brown wrote:

  OK, I can apply them if people want but I'd need a resend - I discarded
  them since they'd normally go via the arch tree.

 I will resend the last 3 patches then. There also shouldn't be any issues
 if 13, 14 are only merged through ASoC tree and patch 15 through Samsung
 tree.

OK.  Kukjin, are you OK with me applying some or all of these?


signature.asc
Description: Digital signature


Re: [PATCH V3 13/15] ARM: dts: Exynos4 and Odroid X2/U3 sound device nodes update

2015-02-03 Thread Mark Brown
On Tue, Feb 03, 2015 at 12:04:16PM +0100, Sylwester Nawrocki wrote:

  Sorry, I should've said - I applied the ASoC patches, not these.

  Shall I take 13 to 15 DT patches in Samsung tree?

 Patches 13, 14 use macro definitions which are added in patch which is
 already in Mark's sound tree (ASoC: samsung: i2s: Add clk provider DT
 binding documentation) [1]. We would need to consider that to avoid
 build breaks.

OK, I can apply them if people want but I'd need a resend - I discarded
them since they'd normally go via the arch tree.


signature.asc
Description: Digital signature


Re: [PATCH] ASoC: Samsung: add missing I2C/SPI dependencies

2015-01-29 Thread Mark Brown
On Wed, Jan 28, 2015 at 10:28:55PM +0100, Arnd Bergmann wrote:
 The SND_SOC_ARNDALE_RT5631_ALC5631 selects the rt5631 codec
 that requires I2C to work, so we get a build error if I2C
 is disabled:

You rather buried the lead about there being other drivers in this
changelog, makes the code a bit surprising when you find it.

  config SND_SOC_SPEYSIDE
   tristate Audio support for Wolfson Speyside
 - depends on SND_SOC_SAMSUNG  MACH_WLF_CRAGG_6410
 + depends on SND_SOC_SAMSUNG  MACH_WLF_CRAGG_6410  SPI_MASTER
   select SND_SAMSUNG_I2S
   select SND_SOC_WM8996
   select SND_SOC_WM9081

Why only add a dependency on SPI here?


signature.asc
Description: Digital signature


Re: [alsa-devel] [PATCH v4 1/3] ASoC: samsung: Add machine driver for Trats2

2015-01-27 Thread Mark Brown
On Tue, Jan 27, 2015 at 06:09:39PM +0100, Sylwester Nawrocki wrote:
 On 23/01/15 06:03, Inha Song wrote:

  +   priv-clk_mclk =  of_clk_get_by_name(codec_node, MCLK1);
  +   if (IS_ERR(priv-clk_mclk)) {
  +   dev_err(pdev-dev, Failed to get mclk clock\n);
  +   of_node_put(codec_node);
  +   return PTR_ERR(priv-clk_mclk);
  +   }

 Wouldn't it also work if we added clock handling into the wm8994 codec
 driver instead? Not sure if it is correct to retrieve the codec's clock
 in the machine driver like this. Or perhaps the MCLK1 (SoC CLKOUT) clock
 should be added to the sound DT node and handled only by the machine
 driver, together with the other (MCLK2) clock?

That's definitely where we should end up but there are practical issues
with that approach since it involves coordination with all the machine
drivers using the device.


signature.asc
Description: Digital signature


Re: [RFC 2/3] regmap: Use the enhancement of i2c API to address circular dependency problem

2015-01-27 Thread Mark Brown
On Tue, Jan 20, 2015 at 12:14:31PM +0100, Paul Osmialowski wrote:
 On Mon, 19 Jan 2015, Mark Brown wrote:

 OK, so that's what should go in the changelog (along with an explanation
 of why this preparation is required at all) - but I still don't see the
 async bit of this I'm afraid.

 I don't think preparation stage should be exposed for asynchronous transfer.
 Due to its nature, it shouldn't cause circular lock dependency as we can
 observe for synchronous io. Since i2c does not support async transfer anyway
 (so (map-async  map-bus-async_write) will be always false for i2c
 transfers), let's use spi as an example.

Again I come back to explaining this out of the context of this
particular issue in terms of something that's comprehensible at the
regmap level.

 With spi we have curious situation where both sync and async are handled by
 the same __spi_async() function, though for sync transfer
 wait_for_completion() is called soon after __spi_async() in order to ensure
 that transfer is completed.

That's not actually that strange really, in general synchronous
operation can always be implemented as a simple wrapper around
asynchronous operation.

 Actual transfer is handled by spi_transfer_one_message() called from
 spi_pump_messages(). Note that spi_pump_message() before it calls
 spi_transfer_one_message() also calls prepare_message() callback (if
 provided):

We are *massively* into implementation details here, if we're relying on
these things then they could change at any time (and in fact what you
say above is partly specific and is not true even in the default case
for current code).  Think in terms of abstractions and locking
guarantees rather than the details of the current code.


signature.asc
Description: Digital signature


Re: [alsa-devel] [PATCH v4 2/3] ASoC: samsung: Document Trats2 audio subsystem bindings

2015-01-27 Thread Mark Brown
On Fri, Jan 23, 2015 at 02:03:29PM +0900, Inha Song wrote:

 + - samsung,audio-routing : A list of the connections between audio
 +   components. each entry is a pair of strings, the first being the
 +   connection's sink, the second being the connection's source

The list of valid components should be specified here - reference the
CODEC bindings for anything on the CODEC but things on the board need to
be enumerated.


signature.asc
Description: Digital signature


Re: [alsa-devel] [PATCH v4 1/3] ASoC: samsung: Add machine driver for Trats2

2015-01-27 Thread Mark Brown
On Fri, Jan 23, 2015 at 02:03:28PM +0900, Inha Song wrote:
 This patch add the sound machine driver for Trats2 board.
 The codec operate in master mode.

This looks like (and mostly should be) a DTified copy of the littlemill
driver.  The major differences are the fact that this lacks jack
detection, the management of the clock and the fact that instead of
picking a fixed clock frequency this tries to adjust based on
hw_params().  Ideally we'd be able to merge these drivers so we're
keeping up with best practice.

The reason we pick a fixed frequency for littlemill is that the device
supports analogue bypass paths which would be broken otherwise which
will apply here also I imagine.

 +config SND_SOC_SAMSUNG_TRATS2_WM1811
 + tristate SoC I2S Audio support for WM1811 on Tizen Trats2 board
 + depends on SND_SOC_SAMSUNG
 + select SND_SAMSUNG_I2S
 + select MFD_WM8994

For things outside the audio subsystem it's more common to use a depends
instead of a select - the selects are there because the drivers only
work with a machine driver and if we go outside the subsystem then since
selects aren't recursive we end up breaking elsewhere.  For example here
we'll end up not selecting MFD_CORE.


signature.asc
Description: Digital signature


Re: [RFC 2/3] regmap: Use the enhancement of i2c API to address circular dependency problem

2015-01-19 Thread Mark Brown
On Mon, Jan 19, 2015 at 10:31:22AM +0100, Paul Osmialowski wrote:
 On Fri, 16 Jan 2015, Mark Brown wrote:

 What I'm saying is that I want to understand this change from a point of
 view that isn't tied to I2C - at the regmap level what is this doing,

 From the regmap point of view, it allows its functions to have a chance to
 prepare transfer medium for (synchronous) transfer (no matter what bus
 handles it) before it actually start to happen (then unprepare it when it's
 done) and crucially before any lock is obtained in functions like
 regmap_write(), regmap_read() or regmap_update_bits.

OK, so that's what should go in the changelog (along with an explanation
of why this preparation is required at all) - but I still don't see the
async bit of this I'm afraid.

 Maybe adding a pair of callbacks (map-reg_write_sync_prepared(),
 map-reg_read_sync_prepared()) would make situation clearer.

No, I don't think so - it'd just complicate the callers.

 I2C is a bus that has some properties which you're saying needs some
 changes, what are those properties and those changes?

 I'm not saying I2C as a bus requires changes. What I'm saying is that I2C
 API can be extended to allow more detailed control on what happens with the
 transfer.

My point here is that your explanation is in terms of I2C specifics and
not really at a generic regmap level.

 Can you be more specific please?  If something needs preparing it seems
 like it'd need preparing over an async transaction just as much as over
 a synchronous one.

 Even with those preparation and unpreparation stages, this transfer is still
 synchronous. For example, it starts when regmap_read() starts and ends when
 regmap_read() ends. Nothing is queued or deferred. Namely, when
 max_gen_clk_unprepare() function calls regmap_update_bits() it expects that
 when regmap_update_bits() returned, no outstanding transfer are happening
 nor waiting to proceed. Everything must be completed before returning to
 max_gen_clk_unprepare().

That doesn't address my question - all you're saying is that in a
synchronous call path things are synchronous which is fine but obviously
regmap supports async I/O too.

 Not in this pattern where the caller needs to check too.

 I don't persist on that. Apparently, you're the author of this file, though
 regmap_init() function was later expanded by other guys. They never assigned
 bus callback function pointers directly to map operation callbacks. It is
 possible to replace 'map-reg_prepare_sync_io = regmap_bus_prepare_sync_io'
 with 'map-reg_prepare_sync_io = map-bus-prepare_sync_io' - this will
 compile and this will work properly. But IMHO it wouldn't match with what
 the others did.

If you look at the other callbacks they're doing other things beyond
simply forwarding the functions on.  That's the problem here, the
functions just add a layer of indirection and nothing else.


signature.asc
Description: Digital signature


Re: [RFC 1/3] i2c: Enhancement of i2c API to address circular lock dependency problem

2015-01-18 Thread Mark Brown
On Sun, Jan 18, 2015 at 11:54:56AM +0100, Krzysztof Kozlowski wrote:
 W dniu 18.01.2015 o 07:30, Tomasz Figa pisze:

 So, the question is, do we actually have hardware that _really_
 requires _actual_ preparation or all the clk_prepare_enable()s in I2C
 drivers (at least in i2c-s3c2410) are just to simplify the code?

 I completely forgot that you already thought about this deadlock in 2014. I
 think we can try the no-prepare way for i2c-s3c2410. However this would be
 only workaround for specific chip. Other buses (like SPI) would require
 similar changes.

Right, and it's every single driver which would need an update too which
is a bit icky and sad.  On the other hand a more detailed fix is going
to involve trying to make the clock API locking more fine grained which
isn't fun.

 I wondered why this was not observed (at least not observed by me with
 lockdep) on Gear 2 (Rinato) board. This is quite similar case: the S2MPS14
 PMIC provides regulators and 32kHz clocks. I think it is exactly the same
 pattern as for max77686... but somehow lockdep never reported that deadlock
 there.

Mostly the clocks on PMICs never get changed at runtime which helps a
lot here.


signature.asc
Description: Digital signature


Re: [PATCH V3 00/15] ASoC: samsung: Add clk provider for I2S internal clocks

2015-01-14 Thread Mark Brown
On Wed, Jan 14, 2015 at 07:42:27PM +0100, Sylwester Nawrocki wrote:
 This series is an attempt to resolve the CDCLK clock gating issue on Odroid
 X2/U3 as reported by Daniel Drake [1], by exposing the CDCLK gate clock
 (and the two other clocks) through clk API. The upside is we can switch
 Odroid X2/U3 to the simple-card, once the CDCLK clock is taken care of by
 the clk core and DT.

Applied all, thanks.


signature.asc
Description: Digital signature


Re: [PATCH V3 13/15] ARM: dts: Exynos4 and Odroid X2/U3 sound device nodes update

2015-01-14 Thread Mark Brown
On Wed, Jan 14, 2015 at 07:42:40PM +0100, Sylwester Nawrocki wrote:
 Clock related properties are added to the Exynos4 I2S device nodes
 so they can be referred to as clock providers. Missing i2s_opclk1
 clock is added to the I2S0 node and clock properties are added
 to the MAX98090 codec node to allow it to control/read frequency
 of the MCLK clock directly.

Sorry, I should've said - I applied the ASoC patches, not these.


signature.asc
Description: Digital signature


Re: [PATCH v5 0/5] regulator: Allow parsing custom DT properties with simplified DT parse

2015-01-08 Thread Mark Brown
On Mon, Jan 05, 2015 at 12:48:40PM +0100, Krzysztof Kozlowski wrote:
 Hi,
 
 
 The patchset adds:
 1. a way of parsing custom DT properties by the driver when simplified
DT parsing method is used,
 2. GPIO enable control to the max77686 driver.

Applied 1-4, thanks.


signature.asc
Description: Digital signature


Re: [PATCH v2 16/16] ARM: dts: Fix I2S1, I2S2 compatible for exynos4 SoCs

2015-01-06 Thread Mark Brown
On Fri, Dec 19, 2014 at 02:55:36PM +0100, Sylwester Nawrocki wrote:
 I2S1, I2S2 on Exynos4 SoC series have limited functionality compared
 to I2S0, samsung,s3c6410-i2s compatible should be used for them.
 
 Signed-off-by: Sylwester Nawrocki s.nawro...@samsung.com
 ---

This should probably go to stable as a bug fix too.


signature.asc
Description: Digital signature


Re: [PATCH v2 01/16] ASoC: samsung: i2s: Remove unused gpios field from struct i2s

2015-01-06 Thread Mark Brown
On Fri, Dec 19, 2014 at 02:55:21PM +0100, Sylwester Nawrocki wrote:
 The 'gpios' field in 'struct i2s' is now unused, this change
 seems to be missing in commit 0429ffeff460c4302bd1520e6
 (ASoC: samsung: Remove obsolete GPIO based DT pinmuxing).

Applied, thanks.


signature.asc
Description: Digital signature


Re: [PATCH v2 06/16] ASoC: samsung: i2s: Move clk enable to the platform driver probe()

2015-01-06 Thread Mark Brown
On Fri, Dec 19, 2014 at 02:55:26PM +0100, Sylwester Nawrocki wrote:
 Gating the I2S bus clock in the driver's runtime PM callbacks has
 currently really no effect since the clock is being enabled
 in the DAI's probe() and thus is permanently turned on. Now we just
 move the enable to the platform driver's probe(), which doesn't
 change the situation much. It will allow us to register a clock
 provider already in samsung_i2s_probe() function.

That doesn't sound quite right - the normal idiom for this stuff is to
enable on probe, mark the device as active then if runtime PM is enabled
it can idle the device and turn off the clock.  That way if runtime PM
is disabled things continue to run.  I've not checked to see if this is
actually happening correctly all the way through but that's what's
supposed to happen and means that enabling in the probe should still
result in working clock management.

It will as things currently stand be broken for the dual DAI case so
this is a fix for that (the dual DAI case will double enable if both
links are in use) but the analysis isn't quite correct.


signature.asc
Description: Digital signature


Re: [PATCH v2 02/16] ASoC: samsung: i2s: samsung_i2s_get_driver_data() cleanup

2015-01-06 Thread Mark Brown
On Fri, Dec 19, 2014 at 02:55:22PM +0100, Sylwester Nawrocki wrote:
 Tidy up the samsung_i2s_get_driver_data() function by using
 IS_ENABLE() instead of #ifdef and add missing braces for
 the 'else' part. Also ensure we are not dereferencing NULL
 'match' pointer.

This doesn't apply against current code, can you please check and
resend?


signature.asc
Description: Digital signature


Re: [PATCH v2 00/16] ASoC: samsung: Add clk provider for I2S internal clocks

2015-01-06 Thread Mark Brown
On Fri, Dec 19, 2014 at 02:55:20PM +0100, Sylwester Nawrocki wrote:
 This series is an attempt to resolve the CDCLK clock gating issue on Odroid
 X2/U3 as reported by Daniel Drake [1], by exposing the CDCLK gate clock 
 (and the two other clocks) through clk API. 

This all looks basically fine apart from the comments I made but since I
couldn't apply patch 2 I've not been able to to test anything.


signature.asc
Description: Digital signature


Re: [alsa-devel] [PATCH 2/3] ASoC: samsung: Document Trats2 audio subsystem bindings

2015-01-05 Thread Mark Brown
On Mon, Jan 05, 2015 at 08:25:16PM +0900, Inha Song wrote:

 + - clocks : Reference to the codec master clock
 + - clock-names : The clock should be named mclk

This should be done in the CODEC driver, not in the machine driver - the
CODEC always needs the clock, it's not something specific to this
machine.  Even if we decide that for Linux the best thing to do is to
manage the clock in the machine driver it should be described in the DT
as being attached to the CODEC.


signature.asc
Description: Digital signature


Re: [PATCH 27/28] ASoC: samsung: drop owner assignment from platform_drivers

2014-12-22 Thread Mark Brown
On Sun, Dec 21, 2014 at 10:14:48PM +0100, Wolfram Sang wrote:
 This platform_driver does not need to set an owner, it will be populated by 
 the
 driver core.

Applied, thanks.


signature.asc
Description: Digital signature


Re: [RFC 02/15] drivers/base: add restrack framework

2014-12-15 Thread Mark Brown
On Mon, Dec 15, 2014 at 09:28:41AM +0100, Andrzej Hajda wrote:
 On 12/12/2014 05:52 PM, Mark Brown wrote:
  On Wed, Dec 10, 2014 at 04:48:20PM +0100, Andrzej Hajda wrote:

  I don't know about anyone else but I'm having a hard time reading the
  restrack name, it looks like a misspelling of restack to me.

 Any alternative names?

Well, even just res_track would help.

 I will move the code for provider matching to frameworks,
 so it will be easy to add just dev_info after every failed attempt
 of getting resource, including deferring. This is the simplest solution
 and it should be similar in verbosity to deferred probing.

 Maybe other solution is to provide debug_fs (or device) attribute showing
 restrack status per device.

I think both are useful - it's often helpful to have a listing of what
resources have actually been registered, for example to help spot typos.


signature.asc
Description: Digital signature


Re: [RFC 01/15] drivers/base: add track framework

2014-12-15 Thread Mark Brown
On Sat, Dec 13, 2014 at 12:12:19AM +0100, AH wrote:
 Mark Brown wrote on 12.12.2014 17:36:
 On Wed, Dec 10, 2014 at 04:48:19PM +0100, Andrzej Hajda wrote:

 +   kfree(ptask);
 +
 +   if (empty)
 +   break;
 +
 +   track_process_task(track, task);

 ...we then go and do some other stuff, including processing that task,
 without the lock or or any other means I can see of excluding other
 users before going round and removing the task.  This seems to leave us
 vulnerable to double execution.

 No, if you look at track_add_task function you will see that the queue is
 processed only if it is initially empty, otherwise the task is only added to
 the queue, so it will be processed after processing earlier tasks.
 So the rule is that if someone add task to the queue it checks if the queue
 is empty, in such case it process all tasks from the queue until
 the queue becomes empty, even the tasks added by other processed.
 This way all tasks are serialized.

This is all pretty fiddly and seems fragile - if nothing else the code
seems undercommented since the above is only going to be apparent with
following through multiple functions and we're relying on both owner and
list emptiness with more than one place where a task can get processed.

 I'm also unclear what is supposed to happen if adding a notification
 races with removing the thing being watched.

 The sequence should be always as follows:
 1. create thing, then call track_up(thing).
 ...
 2. call track_down(thing) then remove thing.

 If we put 1 into probe and 2 into remove callback of the driver it will be
 safe - we are synchronised by device_lock. But if, for some reason, we want
 to create object after probe we should do own synchronization or just put
 device_lock around 1. The same applies if we want to remove
 object earlier. This is the comment above about. I will expand it to more
 verbose explanation.

You can't rely on the device lock here since this isn't tied to kobjects
or anything at all - it's a freestanding interface someone could pick up
and use in another context.  Besides, that isn't really my concern - my
concern is what happens if something asks to wait for 


signature.asc
Description: Digital signature


Re: [PATCH] iommu/exynos: Fix arm64 allmodconfig build

2014-12-15 Thread Mark Brown
On Mon, Dec 15, 2014 at 02:10:37PM +0100, Krzysztof Kozłowski wrote:

 Hi Mark,

 Few days ago I posted similar patch:
 https://lkml.org/lkml/2014/12/5/268
 but no one have picked it up.

 Anyway the fix of yours seems fine to me.

I don't mind either way, it just seemed to be easier to report the bug
with a patch.  If Jeorg is busy perhaps it can go via the arm64 tree,
the issue is triggered by the addition of the platform there?


signature.asc
Description: Digital signature


[PATCH] iommu/exynos: Fix arm64 allmodconfig build

2014-12-15 Thread Mark Brown
The Exynos IOMMU driver uses the ARM specific dmac_flush_range() and
outer_flush_range() functions. This breaks the build on arm64 allmodconfig
in -next since support has been merged for some Exynos ARMv8 SoCs. Add a
dependency on ARM to keep things building until either the driver has the
ARM dependencies removed or the ARMv8 architecture code implements these
ARM specific APIs.

Signed-off-by: Mark Brown broo...@kernel.org
---

Resending to the arm-soc people since the addition of the Exynos
platform for ARMv8 went via them, Krzysztof also sent a fix for this
earlier but it there's been no response.

 drivers/iommu/Kconfig | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/iommu/Kconfig b/drivers/iommu/Kconfig
index 01e8bfae569b..325188eef1c1 100644
--- a/drivers/iommu/Kconfig
+++ b/drivers/iommu/Kconfig
@@ -187,7 +187,7 @@ config TEGRA_IOMMU_SMMU
 
 config EXYNOS_IOMMU
bool Exynos IOMMU Support
-   depends on ARCH_EXYNOS
+   depends on ARCH_EXYNOS  ARM
select IOMMU_API
select ARM_DMA_USE_IOMMU
help
-- 
2.1.3

--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH] iommu/exynos: Fix arm64 allmodconfig build

2014-12-15 Thread Mark Brown
On Mon, Dec 15, 2014 at 03:38:04PM +, Will Deacon wrote:
 On Mon, Dec 15, 2014 at 03:35:29PM +, Mark Brown wrote:

  I don't mind either way, it just seemed to be easier to report the bug
  with a patch.  If Jeorg is busy perhaps it can go via the arm64 tree,
  the issue is triggered by the addition of the platform there?

 Can I pass the buck to arm-soc, as they're handling arm64 platform code?

 It seems sensible to merge the fix via the same tree that introduced the
 breakage.

Sure, just resending to them...


signature.asc
Description: Digital signature


Re: [RFC 01/15] drivers/base: add track framework

2014-12-12 Thread Mark Brown
On Wed, Dec 10, 2014 at 04:48:19PM +0100, Andrzej Hajda wrote:
 track is a generic framework for safe tracking presence of any kernel objects
 having an id. There are no special requirements about type of object or its
 id. Id shall be unique.

This is pretty confusing, when it talks about kernel objects that
sounds like it means struct kobject but in fact there's no connection
with that or any of the other kernel object stuff.  Perhaps it makes
sense but it should be addressed.

 Typical usage of the framework by consumer looks as follow:
 1. Consumer registers notifier callback for objects with given id.

This is also a custom thing not connected with the notifier mechanism
implemented by struct notifier_block.  Again this should probably be
addressed, even if it's just used internally it seems like there should
be some opportunity for code reuse here.

 + case track_task_up:
 + node = track_get_node(track, task-type, task-id, true);
 +
 + node-up = true;
 + node-data = task-data;
 + list_for_each_entry_safe(itb, node-itb_next, node-itb_head,
 +  list)
 + itb-callback(itb, node-data, true);
 + return;
 + case track_task_down:

I'm not sure the up and down naming is the most obvious naming for
users.  It's obviously inspired by semaphores but it's not entirely
obvious that this is going to make things clear and meaningful for
someone trying to understand the interface.

 +static int track_process_queue(struct tracker *track)
 +{
 + struct track_task *task, *ptask = NULL;
 + unsigned long flags;
 + bool empty;
 +
 + /* Queue non-emptiness is used as a sentinel to prevent processing
 +  * by multiple threads, so we cannot delete entry from the queue
 +  * until it is processed.
 +  */
 + while (true) {
 + spin_lock_irqsave(track-queue_lock, flags);
 +
 + if (ptask)
 + list_del(ptask-list);
 + task = list_first_entry(track-queue,
 + struct track_task, list);
 +
 + empty = list_empty(track-queue);
 + if (empty)
 + complete_all(track-queue_empty);
 +
 + spin_unlock_irqrestore(track-queue_lock, flags);

Here we get a task from the head of the list and drop the lock, leaving
the task on the list...

 + kfree(ptask);
 +
 + if (empty)
 + break;
 +
 + track_process_task(track, task);

...we then go and do some other stuff, including processing that task,
without the lock or or any other means I can see of excluding other
users before going round and removing the task.  This seems to leave us
vulnerable to double execution.  I *think* this is supposed to be
handled by your comment Provider should take care of calling
notifications synchronously in proper order in the changelog but that's
a bit obscure, it's not specific about what the requirements are (or
what the limits are supposed to be on the notification callbacks).

I'm also unclear what is supposed to happen if adding a notification
races with removing the thing being watched.


signature.asc
Description: Digital signature


Re: [RFC 02/15] drivers/base: add restrack framework

2014-12-12 Thread Mark Brown
On Wed, Dec 10, 2014 at 04:48:20PM +0100, Andrzej Hajda wrote:
 restrack framework allows tracking presence of resources with dynamic life
 time. Typical example of such resources are all resources provided by device

I don't know about anyone else but I'm having a hard time reading the
restrack name, it looks like a misspelling of restack to me.

At a high level my biggest questions here are the relationship between
this and the component code and usability.  The usability concern I have
is that I see no diagnostics or trace here at all.  This means that if a
user looks at their system, sees that the device model claims the driver
for a device bound to the device but can't see any sign of the device
doing anything they don't have any way of telling why that is other than
to look in the driver code, see what resources it was trying to depend
on and then go back to the running system to try to understand which of
those resources hasn't appeared.

 +int restrack_up(unsigned long type, const void *id, void *data)

 +int restrack_down(unsigned long type, const void *id, void *data)

Again I'm not sure that the up and down naming is meaningful in the
context of this interface.

 +static void restrack_itb_cb(struct track_block *itb, void *data, bool on)
 +{

itb_cb?


signature.asc
Description: Digital signature


Re: [PATCH/RFC 04/14] ASoC: samsung: i2s: Request memory region in driver probe()

2014-12-12 Thread Mark Brown
On Thu, Dec 11, 2014 at 06:45:42PM +0100, Sylwester Nawrocki wrote:
 The memory mapped registers region is common for both DAIs so request
 it in the I2S platform device driver's probe for the platform device
 corresponding to the primary DAI, rather than in the ASoC DAI's probe
 callback. While at it switch to devm_ioremap_resource(). This also
 drops the hard coded (0x100) register region size in the driver.

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


signature.asc
Description: Digital signature


Re: [PATCH/RFC 11/14] ASoC: samsung: odroidx2: Handle I2S CDCLK clock conditionally

2014-12-12 Thread Mark Brown
On Thu, Dec 11, 2014 at 06:45:49PM +0100, Sylwester Nawrocki wrote:
 If the codec control it's master clock provider by the I2S module
 we should not be touching it with set_sysclk() calls.
 So skip the set_sysclk() call in the machine driver if clocks
 property is found in the codec device node.

Please clarify in the changelog here that this is a transition measure
to support old DTs, it's not clear why we might want to do things for
the I2S controller by fishing around in the CODEC DT otherwise.


signature.asc
Description: Digital signature


Re: [alsa-devel] [PATCH/RFC 12/14] ASoC: samsung: i2s: Add clock provider for the I2S internal clocks

2014-12-12 Thread Mark Brown
On Thu, Dec 11, 2014 at 06:45:50PM +0100, Sylwester Nawrocki wrote:

 +Optional Properties:
 
  - samsung,idma-addr: Internal DMA register base address of the audio
sub system(used in secondary sound source).
  - pinctrl-0: Should specify pin control groups used for this controller.
  - pinctrl-names: Should contain only one value - default.
 +- #clock-cells: should be 1, this property must be present if the I2S device
 +  is a clock provider in terms of the common clock bindings, described in
 +  ../clock/clock-bindings.txt.
 +- clock-output-names: from the common clock bindings, names of the CDCLK
 +  I2S output clocks, suggested values are i2s_cdclk0, i2s_cdclk1,
 +  i2s_cdclk3 for the I2S0, I2S1, I2S2 devices recpectively.
 +

Why not make this mandatory going forwards?  You can add a note saying
that older DTs may not have it.

 +The assignment of indices for the I2Sx clock provider is as follows:
 + 0 - the CDCLK (CODECLKO) gate clock,
 + 1 - the RCLK prescaler divider clock (corresponding to the IISPSR register),
 + 2 - the RCLKSRC mux clock (corresponding to RCLKSRC bit in register IISMOD).

Why use the indicies here, they only seem to add obscurity?

 + clk_put(rclksrc);
 + }
 + /*

Missing blank line.

 +  * Register the mux and div clocks only if both source clocks
 +  * are available, i.e. for the I2S0 instance and versions with
 +  * QUIRK_NO_MUXPSR quirk not set.
 +  */

Why not proceed even if one is missing?  This repeats in words the if
statement but doesn't explain why we're doing this.

 + ret = of_clk_add_provider(dev-of_node, of_clk_src_onecell_get,
 +   i2s-clk_data);
 + if (ret  0) {
 + dev_err(dev, failed to add clock provider\n);

Best practice is to print the error code.


signature.asc
Description: Digital signature


Re: [RFC 04/15] regulator: add restrack support

2014-12-10 Thread Mark Brown
On Wed, Dec 10, 2014 at 04:48:22PM +0100, Andrzej Hajda wrote:
 Regulators supports various methods of lookup.
 The patch adds restrack support only to DT based regulators.

Why, what does this mean and how might one use it?  I've not looked at
the code since I don't know what it's supposed to accomplish...

One very high level thing is that anything that only works for DT only
seems to be a non-starter, the API should be hiding details of the
firmware interface.


signature.asc
Description: Digital signature


Re: [PATCH] ASoC: samsung: i2s: Add missing assignment of variant_regs

2014-12-08 Thread Mark Brown
On Mon, Dec 08, 2014 at 06:45:54PM +0100, Sylwester Nawrocki wrote:

 Add assignment of the variant_regs field which is missing in commit
 a5a56871f804edac93a53b5e871c0e9818fb9033 (ASoC: samsung: add support
 for exynos7 I2S controller). Without this attempting to probe the
 secondary DAI fails with an error like:

Applied, thanks.  Broader testing of changes before sending them
upstream would be good...


signature.asc
Description: Digital signature


[PATCH] spi/s3c64xx: Remove redundant runtime PM management

2014-12-05 Thread Mark Brown
The device already asks the core to hold a runtime PM reference while it
is active so it is redundant to open code that in the driver itself.

Signed-off-by: Mark Brown broo...@kernel.org
---
 drivers/spi/spi-s3c64xx.c | 9 -
 1 file changed, 9 deletions(-)

diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c
index 9c616091429d..197bcf093174 100644
--- a/drivers/spi/spi-s3c64xx.c
+++ b/drivers/spi/spi-s3c64xx.c
@@ -346,16 +346,8 @@ static int s3c64xx_spi_prepare_transfer(struct spi_master 
*spi)
spi-dma_tx = sdd-tx_dma.ch;
}
 
-   ret = pm_runtime_get_sync(sdd-pdev-dev);
-   if (ret  0) {
-   dev_err(dev, Failed to enable device: %d\n, ret);
-   goto out_tx;
-   }
-
return 0;
 
-out_tx:
-   dma_release_channel(sdd-tx_dma.ch);
 out_rx:
dma_release_channel(sdd-rx_dma.ch);
 out:
@@ -372,7 +364,6 @@ static int s3c64xx_spi_unprepare_transfer(struct spi_master 
*spi)
dma_release_channel(sdd-tx_dma.ch);
}
 
-   pm_runtime_put(sdd-pdev-dev);
return 0;
 }
 
-- 
2.1.3

--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH v4 2/7] regulator: dt-bindings: Document the ena-gpios property

2014-11-28 Thread Mark Brown
On Fri, Nov 28, 2014 at 10:09:44AM +0100, Krzysztof Kozlowski wrote:

 I understand your concerns here however I didn't want to overengineer
 this. Is the same GPIO (on more complex PMICs) used in different
 contexts? Like enable control and something more in the same time?

Yes, and it's often reprogrammable at runtime.

 Something like:
  struct regulator_desc desc {
   .name   = LDO1
   .of_match   = of_match_ptr(LDO1),
   .regulators_node = of_match_ptr(voltage-regulators),
   .ops= max77686_ldo_ops,
 + .of_ena_gpio= of_match_ptr(ena-gpios),
  ...
  }

Yes, and note that this also means existing bindings can use the core
code.


signature.asc
Description: Digital signature


Re: [PATCH v4 4/7] regulator: Use ena_gpio supplied with generic regulator bindings

2014-11-28 Thread Mark Brown
On Fri, Nov 28, 2014 at 11:30:55AM +0100, Krzysztof Kozlowski wrote:
 On czw, 2014-11-27 at 18:43 +, Mark Brown wrote:

  Why do we need some special magic operation for GPIO based enables
  that's separate to any other enable operation?  This seems really
  confusing, if the constraint setting doesn't work somehow for GPIO based
  enables we should fix that.  Though since this operation takes no
  parameters it's hard to see how it's supposed to apply constraints
  unless it reparses them which doesn't seem like a good idea...

 The regulator driver no longer parses GPIO control from DTS. So somehow
 it should be notified that regulator core parsed this and GPIO should be
 enabled.

 That is the purpose of ops-set_ena_gpio() call.

This sort of thing is a sign that we're not saving much by moving the
parsing to the core and perhaps there's more flexiblity here...  It's
also not something that should be in the constraints handling, it's not
something that constrains the regulator.

   +static int regulator_ena_gpio_setup(struct regulator_dev *rdev,
   + const struct regulator_config *config,
   + const struct regulator_init_data *init_data)

  Why is setting up the GPIO different to requesting it, especially given
  that we have an existing function called _request() which still exists?

 Maybe the name was not a best choice. The setup calls request.

 My patchset here tried to retain the compatibility with
 config.ena_gpio way so the core would accept GPIOs passed in one of
 two ways:
 1. old: config.ena_gpio,
 2. new: parsed by core from DTS.

 The request function previously worked only on config.ena_gpio and I
 changed it here to accept any GPIO. The setup uses one of GPIO methods
 (old or new) and calls request with appropriate GPIO.

We need to support both methods since not all the world is DT.  What I
can't tell is how this code achieves these objectives - it seems to be
an awfully big patch if that's all it's supposed to be doing, I'd expect
just a conditional where we try the two methods in turn.  It may be that
there's a good reason for all this but that needs to be made clear.


signature.asc
Description: Digital signature


Re: [PATCH v4 2/7] regulator: dt-bindings: Document the ena-gpios property

2014-11-28 Thread Mark Brown
On Fri, Nov 28, 2014 at 12:54:27PM +0100, Krzysztof Kozlowski wrote:
 On pią, 2014-11-28 at 11:21 +, Mark Brown wrote:
  On Fri, Nov 28, 2014 at 10:09:44AM +0100, Krzysztof Kozlowski wrote:

   I understand your concerns here however I didn't want to overengineer
   this. Is the same GPIO (on more complex PMICs) used in different
   contexts? Like enable control and something more in the same time?

  Yes, and it's often reprogrammable at runtime.

 I have doubts if generalized code could support such configuration...

That's pretty much my point.


signature.asc
Description: Digital signature


Re: [PATCH v4 4/7] regulator: Use ena_gpio supplied with generic regulator bindings

2014-11-28 Thread Mark Brown
On Fri, Nov 28, 2014 at 03:14:04PM +0100, Krzysztof Kozlowski wrote:
 On pią, 2014-11-28 at 11:38 +, Mark Brown wrote:

  This sort of thing is a sign that we're not saving much by moving the
  parsing to the core and perhaps there's more flexiblity here... 

 The driver receive callbacks (or exposes other kind of interface) for
 other core-generalized code. Recent example is parsing regulator mode
 (added by Javier) and .of_map_mode() callback.

Right, but that's actually doing some device specific translation and
successfully factoring out the bulk of the code - the fact that it's
taking parameters and returning data is a good sign.  This is a callback
placed randomly away from any other related code (adding to the
confusion - it's not integrated into the rest of the flow around this at
all) without a clear purpose.

 I thought how to do this without this additional set_ena_gpio() call.
 One way would be to extend the regulator modes (FAST/IDLE/STANDBY/ and
 GPIO) but this would look somehow unnatural.

Yes, that's absolutely hideous.


signature.asc
Description: Digital signature


Re: [PATCH v4 1/7] mfd: max77686/802: Remove support for board files

2014-11-27 Thread Mark Brown
On Thu, Nov 27, 2014 at 12:20:47PM +0100, Krzysztof Kozlowski wrote:
 The driver is used only on Exynos based boards with DTS support.
 After removal of board file support from max77686 and max77802 regulator
 drivers, the MFD driver can be converted to DTS-only version. This
 simplifies a little the code:

Why is this a dependency for new features in the regulator core?


signature.asc
Description: Digital signature


Re: [PATCH v4 2/7] regulator: dt-bindings: Document the ena-gpios property

2014-11-27 Thread Mark Brown
On Thu, Nov 27, 2014 at 12:20:48PM +0100, Krzysztof Kozlowski wrote:

 +- ena-gpios: GPIO to use for enable control. Actual implementation depends
 +  on regulator driver. The bindings documentation for given driver describes
 +  which regulator actually supports it.
 +- ena-gpio-open-drain: GPIO is open drain type.

I'm relly not a big fan of adding a fixed name property here with no
override capability, it means that the naming won't reflect the specific
regulator design so closely and in practice for many of the PMICs the
GPIO control can do rather more than just control enables and supports
reprogramming.  The latter case where we've got a signal which can
sometimes be simply and enable but sometimes more makes it especially
worrying to have the property always be there, it's something that might
work in some configurations but could easily be broken if we try to
exploit more advanced functionality (things also triggering other
configuration changes at the same time).

Factoring out the code is good but it seems better to have it be
something which drivers can control, for example by having them
explicitly specify a property name to use or perhaps a flag to enable
the default name.

We also need an invert option.


signature.asc
Description: Digital signature


Re: [PATCH v4 4/7] regulator: Use ena_gpio supplied with generic regulator bindings

2014-11-27 Thread Mark Brown
On Thu, Nov 27, 2014 at 12:20:50PM +0100, Krzysztof Kozlowski wrote:
 Use ena_gpio from regulator constraints (filled by parsing generic
 bindings) to initialize the GPIO enable control. Support also the old
 way: ena_gpio supplied in regulator_config structure.
 
 This also adds a new set_ena_gpio() callback in regulator_ops structure
 which driver may provide to actually enable the GPIO control in
 hardware.

This seems really confused like it's trying to work around some other
problem - this all feels like it's at the wrong abstraction level.  As
far as I can tell this is trying to fix bugs in the previous patch and
do some other refactorings (the also add this other random op bit
especially) but I'm really not clear what the goal is.

Please try to think if the code you're writing makes sense at the big
picture level rather than just band aiding specific problems you see.
It's also a good idea to keep random code motion separate from
functional changes since it makes it much easier to follow what each is
supposed to do.

 @@ -1044,6 +1045,14 @@ static int set_machine_constraints(struct 
 regulator_dev *rdev,
   }
   }
  
 + if (rdev-constraints-use_ena_gpio  ops-set_ena_gpio) {
 + ret = ops-set_ena_gpio(rdev);
 + if (ret  0) {
 + rdev_err(rdev, failed to set enable GPIO control\n);
 + goto out;
 + }
 + }

Why do we need some special magic operation for GPIO based enables
that's separate to any other enable operation?  This seems really
confusing, if the constraint setting doesn't work somehow for GPIO based
enables we should fix that.  Though since this operation takes no
parameters it's hard to see how it's supposed to apply constraints
unless it reparses them which doesn't seem like a good idea...

  static int regulator_ena_gpio_request(struct regulator_dev *rdev,
 - const struct regulator_config *config)

 - ret = gpio_request_one(config-ena_gpio,
 - GPIOF_DIR_OUT | config-ena_gpio_flags,
 + ret = gpio_request_one(gpio, GPIOF_DIR_OUT | gpio_flags,
   rdev_get_name(rdev));

 +/*
 + * Request GPIO for enable control from regulator_config
 + * or init_data-constraints.
 + */
 +static int regulator_ena_gpio_setup(struct regulator_dev *rdev,
 + const struct regulator_config *config,
 + const struct regulator_init_data *init_data)

Why is setting up the GPIO different to requesting it, especially given
that we have an existing function called _request() which still exists?


signature.asc
Description: Digital signature


Re: [PATCH v4 3/7] regulator: of: Parse ena-gpios property from DTS

2014-11-27 Thread Mark Brown
On Thu, Nov 27, 2014 at 12:20:49PM +0100, Krzysztof Kozlowski wrote:

 + constraints-ena_gpio = of_get_named_gpio_flags(np, ena-gpios, 0,
 + gpio_flags);
 + if (gpio_is_valid(constraints-ena_gpio)) {

No, this isn't sensible - in what way would an enable control GPIO be a
constraint?  The whole reason we have separate constraint and config
structures is that these are different things.  Keep the GPIO setup in
the configuration.


signature.asc
Description: Digital signature


Re: [PATCH] ASoC: Samsung: Add arndale_rt5631 machine driver and binding

2014-11-26 Thread Mark Brown
On Wed, Nov 26, 2014 at 02:53:04PM +0530, Krishna Mohan Dani wrote:
 Adding machine driver to instantiate I2S based realtek's ALC5631
 sound card on Arndale board.

Applied, thanks.


signature.asc
Description: Digital signature


Re: [PATCH] ASoC: rt5631: Fixing compilation warning when DT is disabled

2014-11-26 Thread Mark Brown
On Wed, Nov 26, 2014 at 02:26:07PM +0530, D Krishna Mohan wrote:

 Following your suggestion I have sent a patch
 (187024b36c635bd454c1b1587b58c9439d3a46ad on your git, branch: rt5631 )
 using ifdef which you have already applied.
 Since there are more suggestion asking for second (__maybe_unused) method, I
 have sent another patch for which below is the link.

 You may be applying the second patch on the already applied first patch. so
 I request you to apply only second patch in place of first and abandon first
 patch.

Once something is applied in git you should always send further patches
as incrmental updates to that, this is much easier to manage and avoids
any potential confusion to other people looking at the tree.


signature.asc
Description: Digital signature


Re: [PATCH v6 0/5] regulator: of: Add initial and suspend modes support

2014-11-26 Thread Mark Brown
On Mon, Nov 10, 2014 at 02:43:50PM +0100, Javier Martinez Canillas wrote:
 Hello Mark,
 
 This is the sixth version of the series that adds regulator initial
 and suspend operating modes support. It relies on the existing work
 that added suspend states bindings. The opmodes are parsed by the
 regulator core and drivers should only define a translation function
 to map between hardware specific to standard modes.

Applied all, thanks.


signature.asc
Description: Digital signature


Re: [PATCH] spi: s3c64xx: add support for exynos7 SPI controller

2014-11-26 Thread Mark Brown
On Thu, Nov 06, 2014 at 03:21:49PM +0530, Padmavathi Venna wrote:
 Exynos7 SPI controller supports only the auto Selection of
 CS toggle mode and Exynos7 SoC includes six SPI controllers.
 Add support for these changes in Exynos7 SPI controller driver.

Applied, thanks.  It does seem like these controllers are getting more
restricted in functionality which seems a bit of a shame.


signature.asc
Description: Digital signature


Re: [PATCH 2/3] regulator: max77802: Fill regulator modes translation callback

2014-11-26 Thread Mark Brown
On Tue, Nov 11, 2014 at 01:04:44PM +0100, Javier Martinez Canillas wrote:
 The max77802 PMIC regulators output can be configured in one of two
 modes: Output ON (normal) and Output ON in Low Power Mode. Some of

Applied, thanks.


signature.asc
Description: Digital signature


Re: [PATCH 1/3] regulator: max77802: Document binding for regulator operating modes

2014-11-26 Thread Mark Brown
On Tue, Nov 11, 2014 at 01:04:43PM +0100, Javier Martinez Canillas wrote:
 Some regulators from the max77802 PMIC support to be configured in one
 of two operating mode: Output ON (normal) and Output On Low Power Mode.

Applied, thanks.


signature.asc
Description: Digital signature


Re: [PATCH] ASoC: rt5631: Fixing compilation warning when DT is disabled

2014-11-24 Thread Mark Brown
On Mon, Nov 24, 2014 at 12:48:31PM +0530, D Krishna Mohan wrote:
 Attaching the patch with changes suggested by Uwe. Though there is another
 patch which I submitted, but I leave it to Mark Brown as to which patch to
 pick.

Please send patches in the format covered in SubmittingPatches, the
tools can't apply this.


signature.asc
Description: Digital signature


  1   2   3   4   5   6   7   8   9   10   >