Re: [PATCH] regulator: s2m,s5m: constify regulator_ops structures
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
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 3/3] dt-bindings: regulator/mfd: Reorganize S2MPA01 bindings
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 > - 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 2/3] dt-bindings: regulator/mfd: Reorganize S5M8767 bindings
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 signature.asc Description: PGP signature
Re: [PATCH 1/3] dt-bindings: regulator/clock/mfd: Reorganize S2MPS-family bindings
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 signature.asc Description: PGP signature
Re: [PATCH 00/10] ARM: s3c64xx multiplatform
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
On Fri, Nov 20, 2015 at 04:07:52PM +0530, Alim Akhtar wrote: > From: Thomas Abraham > > 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 signature.asc Description: PGP signature
Re: [PATCH v3 3/3] ASoC: samsung: pass filter function as pointer
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
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 signature.asc Description: PGP signature
Re: [PATCH v3 5/5] drivers/rtc/rtc-s5m.c: add support for S2MPS15 RTC
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
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
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 1/3] spi: bitbang: Replace spinlock by mutex when calling chipselect
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: [PATCH 3/3] spi: s3c24xx: Convert spinlock to mutex
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: [RFC PATCH 2/2] dt: spi: s3c64xx: add compatible to controller-data
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
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
On Wed, Jul 29, 2015 at 08:21:34PM +0200, Michal Suchanek wrote: > On 29 July 2015 at 19:16, Mark Brown 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
On Wed, Jul 29, 2015 at 06:19:24PM +0200, Michal Suchanek wrote: > On 29 July 2015 at 16:00, Mark Brown 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
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.
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
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 RESEND] regulator: s2mps11: Fix GPIO suspend enable shift wrapping bug
On Wed, Jun 24, 2015 at 07:48:43PM +0900, Krzysztof Kozlowski wrote: > From: Krzysztof Kozlowski > > Status of enabling suspend mode for regulator was stored in bitmap-like > long integer. Applied, thanks. signature.asc Description: Digital signature
Re: [PATCH] regulator: s2mps11: Fix GPIO suspend enable shift wrapping bug
On Wed, Jun 24, 2015 at 01:02:28PM +0900, Krzysztof Kozlowski wrote: > 2015-05-28 23:44 GMT+09:00 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. > 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 10/11] spi: add more debug prints in s3c64xx
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
On Thu, Jun 04, 2015 at 11:33:37AM +0200, Michal Suchanek wrote: > On 4 June 2015 at 11:16, Mark Brown 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
On Thu, Jun 04, 2015 at 11:30:15AM +0200, Geert Uytterhoeven wrote: > On Thu, Jun 4, 2015 at 11:16 AM, Mark Brown wrote: > >> --- a/drivers/spi/spi-s3c64xx.c > >> +++ b/drivers/spi/spi-s3c64xx.c > >> @@ -18,6 +18,7 @@ > >> #include > >> #include > >> #include > >> +#include > > 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 10/11] spi: add more debug prints in s3c64xx
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
> #include
> #include
> +#include
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] regulator: s2mps11: Fix GPIO suspend enable shift wrapping bug
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
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
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()
On Thu, Apr 16, 2015 at 08:18:02PM +0800, weiyj...@163.com wrote: > From: Wei Yongjun > > 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
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 Reviwed-by: Mark Brown This probably wants to go to stable as well. signature.asc Description: Digital signature
Re: [PATCH 05/10] ARM: s3c64xx: enable sparse IRQ support
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
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
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 */ > .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
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
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 signature.asc Description: Digital signature
Re: [PATCH 00/10] ARM: s3c64xx multiplatform, help needed
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
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
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
On Thu, Feb 19, 2015 at 09:44:08AM -0800, Doug Anderson wrote: > On Thu, Feb 19, 2015 at 1:44 AM, Mark Brown 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
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
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 14/15] ARM: dts: Switch Odroid X2/U2 to simple-audio-card
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: [RESEND PATCH V3 13/15] ARM: dts: Exynos4 and Odroid X2/U3 sound device nodes update
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: [PATCH 0/4] ASoC: samsung: clean up references to outdated Kconfig symbols
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 1/4] ASoC: samsung: Remove goni or aquila with the WM8994
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: [PATCH 0/4] ASoC: samsung: clean up references to outdated Kconfig symbols
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 4/4] ASoC: samsung: remove outdated Kconfig references
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 3/4] ASoC: samsung: MACH_SMDKC100 isn't supported anymore
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 2/4] ASoC: samsung: Remove PCM support for WM8580 on SMDK
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: [RESEND PATCH V3 15/15] ARM: dts: Fix I2S1, I2S2 compatible for exynos4 SoCs
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
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
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
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
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: [PATCH] spi: pl08x: do not select S3C64XX_PL080
On Wed, Jan 28, 2015 at 02:27:06PM +0100, Arnd Bergmann wrote: > The pl08x driver originally selected S3C64XX_PL080 to avoid having > the legacy Samsung DMA interfaces. Those are now gone, so the > select is no longer needed, but it now causes problems when > CONFIG_DMA_ENGINE is disabled: Applied, thanks. signature.asc Description: Digital signature
Re: [alsa-devel] [PATCH v4 1/3] ASoC: samsung: Add machine driver for Trats2
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: [alsa-devel] [PATCH v4 2/3] ASoC: samsung: Document Trats2 audio subsystem bindings
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
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
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: [RFC 2/3] regmap: Use the enhancement of i2c API to address circular dependency problem
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
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: [RFC 2/3] regmap: Use the enhancement of i2c API to address circular dependency problem
On Fri, Jan 16, 2015 at 06:36:14PM +0100, Paul Osmialowski wrote:
> On Fri, 16 Jan 2015, Mark Brown wrote:
> >I don't know what this means, sorry. I'm also very worried about the
> >fact that this is being discussed purely in terms of I2C - why would
> >this not affect other buses?
> I tried to open some gate for further extension to any bus that is used for
> regmap communications. Currently it goes down to regmap-i2c.c since I
> enhanced i2c API for this. Anyone who feels it is useful or saves oneself
> from locking troubles can voluntarily adapt other regmap-i2c.* places (as
> needed?).
> My whole point is that I proposed a way to solve nasty deadlock which is
> better to fix than just leave as it is. I got a feeling that situation I
> adressed here may occur others too, so I proposed this extension that allows
> future adaptations. I don't expect it to be accepted easily (i.e. I'm new
> here and have mixed feelins about proposing changes that go so far),
> therefore I prepared other solution for this particular deadlock that occurs
> on this particular device.
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,
I2C is a bus that has some properties which you're saying needs some
changes, what are those properties and those changes?
> >>+ void (*reg_unprepare_sync_io)(void *context);
> >The first question here is why this only affects synchronous I/O or
> >alternatively why these operations have _sync in the name if they aren't
> >for synchronous I/O.
> IMHO this whole idea is against asynchronous I/O.
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.
> >>+ if (bus) {
> >>+ map->reg_prepare_sync_io = regmap_bus_prepare_sync_io;
> >>+ map->reg_unprepare_sync_io = regmap_bus_unprepare_sync_io;
> >>+ }
> >Why are we using these indirections instead of assigning the operation
> >directly? They...
> I followed the pattern used throughout this file.
Not in this pattern where the caller needs to check too.
signature.asc
Description: Digital signature
Re: [RFC 3/3] i2c: s3c2410: Adopt i2c-s3c2410 driver for new enhancement of i2c API
On Fri, Jan 16, 2015 at 03:39:54PM +0100, Paul Osmialowski wrote:
> This adopts i2c-s3c2410 driver for new enhancement of i2c API that
> exposes preparation and unpreparation stages of i2c transfer.
This doesn't seem to have any dependency on the previous patch at all...
it probably does want a better commit log, probably the subject should
say what new enhancement of the API is being adopted. ("Use
prepare/unprepare transfer" for example.)
signature.asc
Description: Digital signature
Re: [RFC 2/3] regmap: Use the enhancement of i2c API to address circular dependency problem
On Fri, Jan 16, 2015 at 03:39:53PM +0100, Paul Osmialowski wrote:
> This uses the enhancement of i2c API in order to address following problem
> caused by circular lock dependency:
Please don't just dump enormous backtraces into commit messages as
explanations, explain in words what the problem you are trying to
address is. If the backtrace is longer than the commit message things
probably aren't working well, and similarly if the first thing the
reader sees is several screenfuls of backtrace that's not really aiding
understanding.
This is all particularly important with something like locking where a
clear understanding of the rules and assumptions being made is very
important to ensuring correctness, it's easy to just paper over a
specific problem and miss a bigger problem or introduce new ones.
> Apparently regulator and clock try to acquire lock which is protecting the
> same regmap. Communication over i2c requires clock to be started. Both things
> require access to the same regmap in order to complete.
> To solve this, i2c clock should be started before attempting operation on
> regulator (which requires locked regmap).
It sounds awfully like something is not doing the right thing with
preparing clocks somewhere along the line but since there's no
analysis it's hard to tell (I don't propose to spend time trawling
backtraces for something I don't know).
Please also use blank lines between paragraphs like all the other commit
messages, it makes things easier to read.
> Exposing preparation and unpreparation stage of i2c transfer serves this
> purpose.
I don't know what this means, sorry. I'm also very worried about the
fact that this is being discussed purely in terms of I2C - why would
this not affect other buses?
> Note that this change does not require modifications in other places.
>
> Signed-off-by: Paul Osmialowski
> ---
> drivers/base/regmap/internal.h | 2 +
> drivers/base/regmap/regmap-i2c.c | 18 +++
> drivers/base/regmap/regmap.c | 107
> ++-
> include/linux/regmap.h | 7 +++
> 4 files changed, 133 insertions(+), 1 deletion(-)
Modification may not be required in other places but this is an
*enormous* change in the code which I can't really review.
> + int (*reg_prepare_sync_io)(void *context);
> + void (*reg_unprepare_sync_io)(void *context);
The first question here is why this only affects synchronous I/O or
alternatively why these operations have _sync in the name if they aren't
for synchronous I/O.
> + if (bus) {
> + map->reg_prepare_sync_io = regmap_bus_prepare_sync_io;
> + map->reg_unprepare_sync_io = regmap_bus_unprepare_sync_io;
> + }
Why are we using these indirections instead of assigning the operation
directly? They...
> +static int regmap_bus_prepare_sync_io(void *context)
> +{
> + struct regmap *map = context;
> +
> + if (map->bus->prepare_sync_io)
> + return map->bus->prepare_sync_io(map->bus_context);
> +
> + return 0;
> +}
...seem to simply check for the operation which appears redundant
especially given that the caller...
> +static void regmap_unprepare_sync_io(struct regmap *map)
> +{
> + void *context = _regmap_map_get_context(map);
> +
> + if (map->reg_unprepare_sync_io)
> + map->reg_unprepare_sync_io(context);
> +}
...does essentially the same check again on every call anyway.
> @@ -1491,12 +1536,18 @@ int regmap_write(struct regmap *map, unsigned int
> reg, unsigned int val)
> if (reg % map->reg_stride)
> return -EINVAL;
>
> + ret = regmap_prepare_sync_io(map);
> + if (ret)
> + return ret;
> +
> map->lock(map->lock_arg);
So what are the rules for calling this operation and how are they
different to those for locking the map? It looks like they might be the
same in which case it seems better to combine them rather than having
to update every single caller and remember why they're always being
worked with in tandem.
signature.asc
Description: Digital signature
Re: [PATCH V3 13/15] ARM: dts: Exynos4 and Odroid X2/U3 sound device nodes update
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 V3 00/15] ASoC: samsung: Add clk provider for I2S internal clocks
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 v5 0/5] regulator: Allow parsing custom DT properties with simplified DT parse
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 00/16] ASoC: samsung: Add clk provider for I2S internal clocks
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: [PATCH v2 16/16] ARM: dts: Fix I2S1, I2S2 compatible for exynos4 SoCs
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 > --- This should probably go to stable as a bug fix too. signature.asc Description: Digital signature
Re: [PATCH v2 02/16] ASoC: samsung: i2s: samsung_i2s_get_driver_data() cleanup
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 01/16] ASoC: samsung: i2s: Remove unused gpios field from struct i2s
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()
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: [alsa-devel] [PATCH 2/3] ASoC: samsung: Document Trats2 audio subsystem bindings
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
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: [PATCH] iommu/exynos: Fix arm64 allmodconfig build
On Mon, Dec 15, 2014 at 03:38:04PM +, Will Deacon wrote: > On Mon, Dec 15, 2014 at 03:35:29PM +0000, 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
[PATCH] iommu/exynos: Fix arm64 allmodconfig build
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 --- 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
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
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 --- 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: [RFC 01/15] drivers/base: add track framework
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: [RFC 02/15] drivers/base: add restrack framework
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: [alsa-devel] [PATCH/RFC 12/14] ASoC: samsung: i2s: Add clock provider for the I2S internal clocks
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: [PATCH/RFC 11/14] ASoC: samsung: odroidx2: Handle I2S CDCLK clock conditionally
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: [PATCH/RFC 04/14] ASoC: samsung: i2s: Request memory region in driver probe()
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: [RFC 02/15] drivers/base: add restrack framework
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: [RFC 01/15] drivers/base: add track framework
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 04/15] regulator: add restrack support
On Thu, Dec 11, 2014 at 11:49:54AM +0100, Andrzej Hajda wrote: > On 12/10/2014 05:07 PM, Mark Brown wrote: > > 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... > Looking at this patch makes no sense without looking at cover letter > or at the patch adding restrack framework. In short adding restrack support > to regulators will allow to: I did look at the cover letter and glance at the first couple of patches but I still can't tell what this is really intended to do. There's a lot of implementation detail but not a lot of high level overview so it's not very clear to me how exactly this solves problems or how it is different to the component framework, it sounds very similar. I'd expect someone reading the change in the regulator API to have at least some idea how this fits in with the rest of the API and how to use it, and probably more importantly I'd expect to be able to understand why this is DT only. signature.asc Description: Digital signature
Re: [RFC 04/15] regulator: add restrack support
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
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
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
---
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 4/7] regulator: Use ena_gpio supplied with generic regulator bindings
On Fri, Nov 28, 2014 at 03:14:04PM +0100, Krzysztof Kozlowski wrote: > On pią, 2014-11-28 at 11:38 +0000, 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 2/7] regulator: dt-bindings: Document the ena-gpios property
On Fri, Nov 28, 2014 at 12:54:27PM +0100, Krzysztof Kozlowski wrote: > On pią, 2014-11-28 at 11:21 +0000, 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
On Fri, Nov 28, 2014 at 11:30:55AM +0100, Krzysztof Kozlowski wrote: > On czw, 2014-11-27 at 18:43 +0000, 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
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 3/7] regulator: of: Parse ena-gpios property from DTS
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 v4 4/7] regulator: Use ena_gpio supplied with generic regulator bindings
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 2/7] regulator: dt-bindings: Document the ena-gpios property
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 1/7] mfd: max77686/802: Remove support for board files
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 1/3] regulator: max77802: Document binding for regulator operating modes
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
