Re: Let's talk about boot loaders
On Fri, May 06, 2016 at 02:03:42PM -0400, Jon Masters wrote: > On 05/06/2016 01:10 PM, Mark Brown wrote: > > On Fri, May 06, 2016 at 12:20:40PM -0400, Jon Masters wrote: > > > But is it really worth trying after so long of the right thing not > > > happening? If anyone really cared about making general purpose distros > > > boot > > > on embedded boards, efforts to compel standards would have happened years > > > ago. To do it right, we would need to have a couple of vendors involved > > > who > > > could compel vendors to comply. > Note: by standards above, I specifically mean "separate platform flash" in > addition to all of the other associated things. Actually, to do it right you Oh, right. In that case this is all irrelevant anyway, there's no need to worry about magic areas of the disk and distros can just do whatever. > > Distros care and currently do ship on such systems - Debian stable lists > > a bunch of boards (something like 20 IIRC) as actively tested for > > example. The board and SoC vendors are to an extent irrelevant here, > I get your point, but for separate flash and getting vendors to ship > firmware, they very much need to be involved. Today, we can't agree as an > industry who is on the hook for this. With an enterprise hat on, I get to Personally I'm not convinced it's particularly worth worrying about - there's enough sensible ways to build systems where the separate storage for bootloaders just isn't solving problems people have that such things are going to be around for a while and inevitably people will end up wanting to run distros on them so at least the community distros need to work with them. > compel vendors to do the only sane thing (in my opinion) which is "thou > shalt ship EFI, on flash that we don't touch". And those who screwed up and > put EFI parameters on hidden disk partitions, or thought EFI variables were > a place to store MAC and platform parameters are slowly found and forced to > comply with the way the industry works. But on embedded, spending a few > cents to do the "right" thing is something that isn't going to happen unless > everyone mandates and pushes for it. It's not just cents, it's also things like board area, manufacturing process, usage models and for some use cases customers who want full control over the software stack. For some kinds of system like the enterprise market what you're describing is absolutely the right way to go but there's other segments where it either isn't solving problems people have or is currently actively worse so there's no compelling reason to adopt. But this is a bit off topic... signature.asc Description: PGP signature ___ linaro-dev mailing list linaro-dev@lists.linaro.org https://lists.linaro.org/mailman/listinfo/linaro-dev
Re: Let's talk about boot loaders
On Fri, May 06, 2016 at 12:20:40PM -0400, Jon Masters wrote: > But is it really worth trying after so long of the right thing not > happening? If anyone really cared about making general purpose distros boot > on embedded boards, efforts to compel standards would have happened years > ago. To do it right, we would need to have a couple of vendors involved who > could compel vendors to comply. Distros care and currently do ship on such systems - Debian stable lists a bunch of boards (something like 20 IIRC) as actively tested for example. The board and SoC vendors are to an extent irrelevant here, when people do this they often don't use any of the software the board vendors provide and just use things like upstream u-boot. Coming up with something that covers all the cases with minimal code for the distros will make life easier for them, if board vendors want to pick it up as well in what they're shipping (and ideally in how they spec their media usage) that's great but we're already winning even if all the board vendors totally ignore it. signature.asc Description: PGP signature ___ linaro-dev mailing list linaro-dev@lists.linaro.org https://lists.linaro.org/mailman/listinfo/linaro-dev
Re: Let's talk about boot loaders
On Thu, May 05, 2016 at 12:44:57PM -0700, Brendan Conoboy wrote: > All of the best practices people here are talking about appear to be geared > toward a frictionless connection to the ARM Linux ecosystem. That's > something many software focused Linaro participants care about, but is that > something manufacturers care about? Usually I only hear about saving > pennies leading to profits at scale being a priority. So if we can talk up > gaining scale by following the practices, there's a better chance members > will listen. The obvious angle on that is that if it's easier to pick up and just use work from the ecosystem then that's less special integration work that's needed per system reducing costs at the system vendor level. signature.asc Description: PGP signature ___ linaro-dev mailing list linaro-dev@lists.linaro.org https://lists.linaro.org/mailman/listinfo/linaro-dev
Re: Let's talk about boot loaders
On Thu, May 05, 2016 at 07:47:41PM +0100, Martin Stadtler wrote: > Specifically for the 96boards, the spec is a recommended view, but its not > meant to be constraining, however it does allow one to then show a best > practice, that others can adopt. That's where the RPB comes in to play, > again to demonstrate and not restrict. Note that this is also going to the cross-distro list, we're probably getting a bit off topic with Linaro/96boards internal discussions like this. signature.asc Description: PGP signature ___ linaro-dev mailing list linaro-dev@lists.linaro.org https://lists.linaro.org/mailman/listinfo/linaro-dev
Re: Let's talk about boot loaders
On Thu, May 05, 2016 at 06:03:40PM +0100, Grant Likely wrote:
> On Thu, May 5, 2016 at 5:53 PM, Mark Brown <broo...@kernel.org> wrote:
> > I think there's one other slightly different angle on this which we
> > should address at the same time, creating fresh boot media for a device
> > ("I just unpacked my board and want to write a system image to a SD
> > card"). If we can come up with a standard way of describing the
> > requirements of boards then we could provide a shared database of this
> > information that tools could use. This might also be useful for the
> > less helpful requirements where it's hard to figure out what's going on
> > from the media itself.
> Are you talking about for boards that don't have on-board media? For
> the boards with eMMC, I think the normal use-cases should always be to
> boot from eMMC (regardless of where the OS is installed). Booting from
> an SD shouldn't require anything special. The only time to boot
> firmware from SD is when upgrading the platform firmware, or doing
> firmware development.
> For systems without on-board storage, yes I agree it would be good to
> have a standard way of describing the layout because the images will
> always need to be tailored for the board.
Yes, boards that don't have on board media are the big issue here -
they're typically the lowest cost and hence quite widely used. The same
infrastructure might also help deal with some of the oddballs that have
unhelpful on board storage but that's much less important if it's even
useful at all.
signature.asc
Description: PGP signature
___
linaro-dev mailing list
linaro-dev@lists.linaro.org
https://lists.linaro.org/mailman/listinfo/linaro-dev
Re: Let's talk about boot loaders
On Thu, May 05, 2016 at 04:21:59PM +0100, Grant Likely wrote:
> I think we have everything we need to work around the location of the
> FW boot image without breaking the UEFI spec. The biggest problem is
> making sure partitioning tools don't go stomping over required
> firmware data and rendering systems unbootable. I *think* we can solve
> that problem by extending the MBR definition to block out a required
> region and then work around that. Tools can generically see the
> special region in the MBR and work around it accordingly.
> So, let me try to itemize the use cases:
I think there's one other slightly different angle on this which we
should address at the same time, creating fresh boot media for a device
("I just unpacked my board and want to write a system image to a SD
card"). If we can come up with a standard way of describing the
requirements of boards then we could provide a shared database of this
information that tools could use. This might also be useful for the
less helpful requirements where it's hard to figure out what's going on
from the media itself.
signature.asc
Description: PGP signature
___
linaro-dev mailing list
linaro-dev@lists.linaro.org
https://lists.linaro.org/mailman/listinfo/linaro-dev
Re: Let's talk about boot loaders
On Thu, May 05, 2016 at 09:01:05PM +0530, Amit Kucheria wrote: > On Thu, May 5, 2016 at 5:15 PM, Marcin Juszkiewicz > > Solution for existing SoCs is usually adding 1MB of SPI flash during design > > phase of device and store boot loader(s) there. But it is so expensive > > someone would say when it is in 10-30 cents range... > > Even 96boards CE specification totally ignored that fact while it could be a > > way of showing how to make popular board. Instead it became > > yet-another-board-to-laugh (EE spec did not improve much). > > Is there a way to get it improved? At least for new designs? > Yes! I've added this suggestion to a list of suggestions for evolution > of the 96boards spec. We already went round the houses repeatedly on that one :( signature.asc Description: PGP signature ___ linaro-dev mailing list linaro-dev@lists.linaro.org https://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH] configs: android: Enable SELinux related configs
On 17 June 2014 12:18, Vishal Bhoj vishal.b...@linaro.org wrote: On 17 June 2014 16:41, Fathi Boudra fathi.bou...@linaro.org wrote: Vishal, 1. against which tree should it be applied? I have tested it against TC2 with the LSK tree. I was not sure if the patches directly go to LSK. I thought it should first go into config fragment tree. I don't know what the config fragment tree is but the LSK doesn't use it and you've not CCed - I guess you should send this to Andrey too for inclusion in linux-linaro? I've added him. 2. do you want it enabled by default for all android builds? Moving ahead SELinux is mandatory for Android so it needs to be enabled across all the platforms we care for. Linux-linaro and LSK are validated on VExpress platform so I am enabling the userspace bits for VExpress to start with. OK, I'll a change to the LSK which enables these options - I can't see any reason to restrict them to Android so I won't just apply this as-is. Please include information like the above in the changelogs for patches, this is just generally good practice and will help people figure out why the change is being made and if it is sensible to apply it. ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH] configs: android: Enable SELinux related configs
On 17 June 2014 13:16, Jon Medhurst (Tixy) t...@linaro.org wrote: On Tue, 2014-06-17 at 12:53 +0100, Mark Brown wrote: I've added him. You didn't seem to have, unless this more of the Linaro's lists's default behaviour of dropping people from CC who are subscribed? Looks like it. It's really broken, it'd be good if we could get it fixed :/ ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: LSK getting started
On 16 September 2013 11:48, Riku Voipio riku.voi...@linaro.org wrote: Do we have a plan for adding gator to LSK now? I have a request to support gator on LSK based image, and I'd prefer not to add the module from outside the kernel. No, there's a card in process but it's not been approved yet. ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: LSK getting started
On Thu, Aug 08, 2013 at 05:20:46PM +0400, Andrey Konovalov wrote: On 08/05/2013 11:34 PM, Mark Brown wrote: These appear to be upstream anyway in one form or another. KBuild: Allow scripts/* to be cross compiled What's the upstreaming status on this? Good question. There are no plans on upstreaming it atm Any great reason why not? It doesn't seem particularly controversial or anything and definitely seems useful. signature.asc Description: Digital signature ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: LSK getting started
On Thu, Aug 08, 2013 at 05:50:57PM +0400, Andrey Konovalov wrote: On 08/08/2013 05:30 PM, Mark Brown wrote: Any great reason why not? It doesn't seem particularly controversial or anything and definitely seems useful. Just the patch author is not with Linaro any more. So I am not very clear who would submit the patch. Hrm, OK. I'll try to take a look myself - from a brief Google it looks like nobody tried at all which is a shame. We should really be pushing people to upstream things so stuff like this doesn't happen. signature.asc Description: Digital signature ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: LSK getting started
On Tue, Aug 06, 2013 at 09:12:44PM +0800, Andy Green wrote: On 6 August 2013 20:47, Mark Brown broo...@kernel.org wrote: Please submit things normally - attachments are non-standard and difficult to work with (both from the point of view of applying and from the point of view of workflow) and if you don't mention them they're not always terribly visible either. I didn't actually notice there was anything here first time around... I wonder why Google has an attachment button. Attachments are obviously useful but as you should be aware given your kernel experience they're not part of the normal kernel development workflow - patches in the body of the e-mail (or git) are the normal way of handling things for projects that don't use a tool like gerrit, all the tooling and so on is based around that. There's good, solid reasoning behind the way the workflow is set up. Like I say if you're going to do something unusual you should at least mention it but it's best avoided unless it's solving a problem. 4) warning-elimination: ata: ata_hpa_resize default assignment Issue is upstream but I can't reproduce original compiler warning If the compiler figures it out we can probably drop this then. If it is still needed then it should be being submitted upstream. Yes it's strange though I did not have a stroke and start editing code randomly, this was generating an error in the recent past. Most likely someone fixed the warning some other way since you wrote the patch. Hm you know the dynamic of people submitting things for your critique is not the only conversational mode that's possible, has that crossed your mind? Sorry about that. It would be really helpful if you could pay attention to the workflow stuff; I think a bunch of what you're seeing here is me getting grumpy due to the difficulty in working with the mail. signature.asc Description: Digital signature ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: LSK getting started
On 5 August 2013 10:11, Jon Medhurst (Tixy) t...@linaro.org wrote: On Mon, 2013-08-05 at 16:43 +0800, Andy Green wrote: 5) Gator bits don't seem to be in there, presumably that's something ARM would like to see in there (it appears in llct) Yes, and I believe someone was raising a card to get it added, and I assume the 'stable' kernel will need updating for each new release of DS-5 as people will want the latest version. This is the first I've heard of this, could whoever this is please talk to me about it? ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: LSK getting started
On 5 August 2013 10:44, Jon Medhurst (Tixy) t...@linaro.org wrote: On Mon, 2013-08-05 at 17:13 +0800, Andy Green wrote: The whole list is good things to have I just wonder how ongoing updates will be handled for backport. For example at some point Tweaks to the MCPM code which aren't upstream. will go upstream and probably be a bit different by then, someone should revert (it may not be that clean since other patches may have meddled) the old one in lsk and replace with the upstream patches. Mark's watching out for that, or you are for the trees you merged that went into LSK, or what's the plan? Plan? There's no plan that I know of. As was mentioned on linaro-kernel the plan is that you should be sending me incremental updates as needed. I'm not monitoring everything that goes upstream, especially in this case where I didn't get a breakdown of what went in there or topic branches for the vexpress enablement so I've very little visibility of what's there. ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: LSK getting started
On 5 August 2013 03:45, Andy Green andy.gr...@linaro.org wrote: 1) There seems to be two choices, linux-linaro-lsk and linux-linaro-lsk-android. I chose the android one, I assume it has the same androidization series on top that linux-linaro-core-tracking used at 3.10? Are there any other differences? There are some patches to improve the performance of the interactive scheduler in there as well. Currently we didn't take John's branch in order to make it easier to track the Google stuff while we're preparing for release, that will get filtered in sometime this week. There may be other stuff lurking in linux-linaro that I'm not aware of, everything is supposed to be individually selected for backport. 3) In our LT tree we patch mainline to remove all warnings coming with our defconfig. Then if we see any warnings coming, we know it's our fault and we need to go fix it. Are you interested in taking a similar approach? We will take suitably non-invasive warning fixes and obviously any actual bug fixes that are fixed in the upstream LTS but we won't actively go looking for warnings in anything that's not built for testing of LSK ourselves. There is no commitment to making things in the underlying kernel warning free. 4) Maybe this is too much thinking ahead, but shouldn't these lsk branches be versioned, like linux-linaro-lsk-3.10? Otherwise when the next lsk version is announced there'll be a problem. This is what I inherited, we'd certainly start versioning things when there's more than one LSK around but having a this is the default version pointer does seem useful. I was intending to add versioned branches as part of the official release (which should be 13.08 now Greg's announced v3.10 as a LTS). ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: LSK getting started
On 5 August 2013 11:00, Jon Medhurst (Tixy) t...@linaro.org wrote: As was mentioned on linaro-kernel the plan is that you should be sending me incremental updates as needed. But who decides what's needed? If what is in 3.10 works, why backport a different version? And I hadn't planned on spending any time on backporting new versions or fixes. For things that are out of tree the advertised policy is that we should be tracking the upstream submissions as far as possible in order to avoid having a special version of the code in the LSK. There's two broad reasons for keeping the backport in sync with the upstream version (if there is an upstream version). One is that if there are changes being made upstream they are hopefully being made for some reason and often those reasons will also apply to the backported version, bug fixes being the most obvious example here. The other is that if we track what's going on development wise then it reduces the effort required when we do need to do the backporting of the bug fixes or ask people working on the upstream code for help resolving issues. This is the flip side of the problem that frequently exists with people doing development on their production releases and never syncing them with upstream, the benefits go both ways here. ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: LSK getting started
On Mon, Aug 05, 2013 at 06:42:33PM +0800, Andy Green wrote: On 5 August 2013 18:16, Mark Brown broo...@linaro.org wrote: There may be other stuff lurking in linux-linaro that I'm not aware of, everything is supposed to be individually selected for backport. Literally linux-linaro I'm not sure is useful for anything, the tree LTs are basing on is linux-linaro-core-tracking. It's composed by rebase and then merges, so it's easy to see what's in there from a quick git log. That doesn't help with understanding why any of it is there or what it's for. Sounds reasonable attached is our current patch for your consideration. There's a permanent #warning about an unwind tables TODO this also knocks out the others are actual problems. Please split this up into proper patches, and of course you should be submitting any fixes upstream if they're still present - if you're doing this sort of warning cleanup work it's going to be useful upstream too. I had a quick glance through and: - Things like just assigning a value to variables at declaration are worrying since that just shuts up the flow analysis warnings even if they're actually pointing out a genuine missed code path. - The regmap change isn't something that I've seen upstream... - For printf() warnings consider changing the printf() format specifier to be accurate rather than casting. signature.asc Description: Digital signature ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: LSK getting started
On Mon, Aug 05, 2013 at 07:37:10PM +0800, Andy Green wrote: On 5 August 2013 18:59, Mark Brown broo...@kernel.org wrote: - The regmap change isn't something that I've seen upstream... If you mean where did the original come from I mean I haven't seen that warning that I'm aware of. commit 5a08d15602987bbdff3407d7645f95b7a70f1a6f Author: Stephen Warren swar...@nvidia.com Date: Wed Mar 20 17:02:02 2013 -0600 regmap: don't corrupt work buffer in _regmap_raw_write() Note that the above change is part of v3.10... signature.asc Description: Digital signature ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: LSK getting started
On Mon, Aug 05, 2013 at 11:23:19PM +0400, Andrey Konovalov wrote: # Misc fixes which don't belong to any particular topic: ynk/llct-v3.10-misc-fixes Add cross-build support to tools/lib/lk library perf tools: make perf to build in 3.9 kernel tree again ARM: crypto: sha1-armv4-large.S: fix SP handling These appear to be upstream anyway in one form or another. KBuild: Allow scripts/* to be cross compiled What's the upstreaming status on this? signature.asc Description: Digital signature ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [ANNOUNCE] linux-linaro kernel schedule / llct age
On Wed, Jun 05, 2013 at 12:05:30AM +0400, Andrey Konovalov wrote: Another point to mention, is the proposal to merge the board enablement topics first, and the generic features next (the LSK case). This would assume the generic topics to enable their features for all the linaro supported boards, or adding separate topic to enable the features for particular boards. This also breaks the current 2-level llct/ll model. I think if we were doing this (though I do agree with Andy that it's not the ordering I'd expect) we'd want to keep the branches doing the enabling separate - that way people can pick up the generic branches for use in their projects without worrying about dependencies on boards or whatever that they don't care about. signature.asc Description: Digital signature ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [ANNOUNCE] linux-linaro kernel schedule for 13.05 published
On Fri, May 03, 2013 at 09:47:53AM +0100, Jon Medhurst (Tixy) wrote: On Thu, 2013-05-02 at 22:40 +0400, Andrey Konovalov wrote: v3.9 release based linux-linaro-core-tracking (llct) rebuild has been published, the tag is llct-20130502.0 . The 13.05 linux-linaro release will be v3.10-rc3 based, I think you may have miscalculated. Isn't the merge window 2 weeks long? In which case, as 3.9 was released this past Monday, we shouldn't expect -rc1 until May 13th and -rc3 will be in June. Yes, the merge window is about two weeks long. Approximately, depending on what Linus feels like. signature.asc Description: Digital signature ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: Suggestion
On Mon, Apr 29, 2013 at 09:28:16AM -0400, Christopher Covington wrote: I'm familiar with a submit- or merge-time option for this. It's not clear to me from your reply whether you're referring to this or an upload- or push-time option. Oh, on initial push? I've not seen that but it does strike me that it would be somewhat useful in avoiding frustration from things not merging later on - at least you know there was a chance that the change could've been merged. signature.asc Description: Digital signature ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: Suggestion
On Wed, Apr 17, 2013 at 09:14:54AM -0400, Christopher Covington wrote: On 04/17/2013 06:29 AM, Jon Medhurst (Tixy) wrote: And with gerrit the patch author needs to get an account enabled with the project, produce a git commit against the current tip, I can't recall ever seeing an upload refused because it wasn't against the latest commit. What's the error message you get? Or is it possible that you might be misremembering such an incident? This is a configuration option within gerrit - you can set a repository or branch up to be fast forward only. This is frequently done for kernels as it's relatively easy for a change in one part of the code base to interact with changes in another part (especially board changes interacting with the drivers they instantiate) so the theory is that this will force all the verification to be done with exactly the tree that ends up in gerrit. Obviously there's a tradeoff here with the rebases. signature.asc Description: Digital signature ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH 2/2] ARM: s3c64xx: cpuidle - use timekeeping wrapper
On Mon, May 14, 2012 at 04:06:17PM +0200, Daniel Lezcano wrote: The timekeeping is computed from the cpuidle core if we set the .en_core_tk_irqen flag. Let's use it and remove the duplicated code. Tested-by: Mark Brown broo...@opensource.wolfsonmicro.com signature.asc Description: Digital signature ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH 1/2] ARM: s3c64xx: cpuidle - declare the states with the new api
On Mon, May 14, 2012 at 04:06:16PM +0200, Daniel Lezcano wrote: The states are now part of the cpuidle_driver structure, so we can declare the states in this structure directly. That saves us an extra variable declaration and a memcpy. Tested-by: Mark Brown broo...@opensource.wolfsonmicro.com signature.asc Description: Digital signature ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH 0/2] ARM: S3C64xx: cpuidle cleanups
On Mon, May 14, 2012 at 10:52:46AM +0200, Heiko St??bner wrote: Am Montag, 14. Mai 2012, 01:51:00 schrieb Daniel Lezcano: On 05/09/2012 04:08 PM, Daniel Lezcano wrote: Are these patches ok for inclusion ? you might want to include the maintainer Kukjin Kim kgene@samsung.com and the samsung list linux-samsung-...@vger.kernel.org into your recipients Also the original author (me). ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH 1/2] mfd: Add Freescale's PMIC MC34708 support
On Fri, Apr 20, 2012 at 12:38:40AM +0800, Ying-Chun Liu (PaulLiu) wrote:
+Sub-nodes:
+- regulators : Contain the regulator nodes. The MC34708 regulators are
+ bound using their names as listed below for enabling.
+
+mc34708__sw1a: regulator SW1A
+mc34708__sw1b: regulator SW1B
There's no point in including the chip name in the properties - the
device has already been bound at the device level, this is just noise
at this level.
+ int ret;
+ int i;
+
+ memset(buf, 0, 3);
+ for (i = 0; i PMIC_I2C_RETRY_TIMES; i++) {
+ ret = i2c_smbus_read_i2c_block_data(client, offset, 3, buf);
The I2C layer already has a retry mechanism, and obviously if I2C is
failing at all the board generally has serious problems.
In general I'm not 100% sure why you're not using the regmap API here -
it looks like the 24 bit I/O is just a block I/O. Alternatively you
could use regmap for the register I/O and then open code the 24 bit
access if they really are different. This would let you
make much more use of framework support.
+ return mc34708_reg_write(mc_pmic, offmask, mask | irqbit);
+}
+EXPORT_SYMBOL(mc34708_irq_mask);
You shouldn't be open coding stuff like this, you should be implementing
it using genirq. This again gives you better framework support.
+static const struct i2c_device_id *i2c_match_id(const struct i2c_device_id
*id,
+ const struct i2c_client *client)
+{
+ while (id-name[0]) {
+ if (strcmp(client-name, id-name) == 0)
+ return id;
+ id++;
+ }
+
+ return NULL;
+}
+
+static const struct i2c_device_id *i2c_get_device_id(const struct i2c_client
+ *idev)
+{
+ const struct i2c_driver *idrv = to_i2c_driver(idev-dev.driver);
+
+ return i2c_match_id(idrv-id_table, idev);
+}
This stuff should be added as generic I2C helpers if it's useful.
+ if (pdata pdata-flags MC34708_USE_REGULATOR) {
+ struct mc34708_regulator_platform_data regulator_pdata = {
+ .num_regulators = pdata-num_regulators,
+ .regulators = pdata-regulators,
+ };
+
+ mc34708_add_subdevice_pdata(mc_pmic, %s-regulator,
+ regulator_pdata,
+ sizeof(regulator_pdata));
+ } else if (of_find_node_by_name(np, regulators)) {
+ mc34708_add_subdevice(mc_pmic, %s-regulator);
+ }
This shouldn't be conditional, the regulators are always physically
present and even if they're not actively managed we can look at their
setup.
signature.asc
Description: Digital signature
___
linaro-dev mailing list
linaro-dev@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH 2/2] regulator: Add Freescale's MC34708 regulators
On Fri, Apr 20, 2012 at 12:38:41AM +0800, Ying-Chun Liu (PaulLiu) wrote:
+static const int mc34708_sw1A[] = {
+ 65, 662500, 675000, 687500, 70, 712500,
Replace these by direct calculations, using tables is both less
efficient and less clear.
+ mc34708_lock(priv-mc34708);
+ ret = mc34708_reg_rmw(priv-mc34708, mc34708_regulators[id].reg,
+ mc34708_regulators[id].enable_bit,
+ mc34708_regulators[id].enable_bit);
+ mc34708_unlock(priv-mc34708);
Having to open code this locking in every single driver is a bit
painful; just have the default register I/O operations do the locking
and introduce additional unlocked versions if needed.
All this stuff could be factored out if you were using regmap.
+EXPORT_SYMBOL_GPL(mc34708_regulator_list_voltage);
No, this stuff should only be accessed via the ops. Why are you doing
this?
+int
+mc34708_get_best_voltage_index(struct regulator_dev *rdev,
+int min_uV, int max_uV)
+{
You're reimplementing core functionality here, or it'd be even better to
use calculations.
+static int mc34708_regulator_get_voltage(struct regulator_dev *rdev)
+{
Why is this not get_voltage_sel?
+static struct regulator_ops mc34708_regulator_ops = {
+ .enable = mc34708_regulator_enable,
+ .disable = mc34708_regulator_disable,
+ .is_enabled = mc34708_regulator_is_enabled,
+ .list_voltage = mc34708_regulator_list_voltage,
+ .set_voltage = mc34708_regulator_set_voltage,
+ .get_voltage = mc34708_regulator_get_voltage,
+};
+EXPORT_SYMBOL_GPL(mc34708_regulator_ops);
No. What are you doing this for?
+int
+mc34708_fixed_regulator_set_voltage(struct regulator_dev *rdev, int min_uV,
+ int max_uV, unsigned *selector)
This function makes no sense...
+int mc34708_sw_regulator_is_enabled(struct regulator_dev *rdev)
+{
+ return 1;
+}
Why are you doing this - this function is redundant.
+ ret = mc34708_reg_rmw(mc34708, MC34708_SW12OP,
+ MC34708_SW12OP_SW1AMODE_M |
+ MC34708_SW12OP_SW2MODE_M,
+ MC34708_SW12OP_SW1AMODE_VALUE |
+ MC34708_SW12OP_SW2MODE_VALUE);
+ if (ret)
+ goto err_free;
+
+ ret = mc34708_reg_rmw(mc34708, MC34708_SW345OP,
+ MC34708_SW345OP_SW3MODE_M |
+ MC34708_SW345OP_SW4AMODE_M |
+ MC34708_SW345OP_SW4BMODE_M |
+ MC34708_SW345OP_SW5MODE_M,
+ MC34708_SW345OP_SW3MODE_VALUE |
+ MC34708_SW345OP_SW4AMODE_VALUE |
+ MC34708_SW345OP_SW4BMODE_VALUE |
+ MC34708_SW345OP_SW5MODE_VALUE);
+ if (ret)
+ goto err_free;
If this needs to be done unconditionally shouldn't it be being donei in
the MFD core driver?
signature.asc
Description: Digital signature
___
linaro-dev mailing list
linaro-dev@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH 00/13] common clk framework misc fixes
On Wed, Apr 11, 2012 at 06:02:38PM -0700, Mike Turquette wrote: This series collects many of the fixes posted for the recently merged common clock framework as well as some general clean-up. Most of the code classifies as a clean-up moreso than a bug fix; hopefully this is not a problem since the common clk framework is new code pulled for 3.4. Can you please CC people on your cover letters as well as on the individual patches? ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH v2 1/2] mfd: da9052: add device-tree support for i2c driver
On Fri, Apr 13, 2012 at 09:37:40PM +0800, Ying-Chun Liu (PaulLiu) wrote:
+ regulators {
+ buck0 {
+ regulator-name = DA9052_BUCK_CORE;
+ regulator-min-microvolt = 50;
+ regulator-max-microvolt = 2075000;
+ };
This is more of a nit but these names look like they aren't terribly
idiomatic and could just be omitted - normally the name is used to tie
the name to the supply on the schematic so the above might be VCORE or
something. If no name is provided then whatever the driver provides
will be used.
signature.asc
Description: Digital signature
___
linaro-dev mailing list
linaro-dev@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH v2 2/2] regulator: da9052: add device tree support
On Fri, Apr 13, 2012 at 09:37:41PM +0800, Ying-Chun Liu (PaulLiu) wrote: From: Ying-Chun Liu (PaulLiu) paul@linaro.org This patch adds device tree support for dialog regulators Applied, thanks. It'd be good to correct the documentation in patch 1 but there should be no code dependency on the core changes so it seems safe for them to go in separately. signature.asc Description: Digital signature ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH 1/2] mfd: da9052: add device-tree support for i2c driver
On Thu, Apr 12, 2012 at 11:39:41PM +0800, Ying-Chun Liu (PaulLiu) wrote:
+- compatible : Should be dialog,da9052, dialog,da9053-aa,
+ dialog,da9053-ab, or dialog,da9053-bb
This is generally the stock ticker symbol so DLG for Dialog.
+Sub-nodes:
+- regulators : Contain the regulator nodes. The DA9052 regulators are
+ sorted as following:
This seems poor from a usability point of view, the user shouldn't have
to care about the order in which they list things in their device tree
and they should be able to only list the regulators they use.
+buck_core : regulator BUCK_CORE (id: 0 )
Having to care about numeric IDs also seems bad for usability.
+#ifdef CONFIG_OF
+ if (!id) {
+ int i;
+ struct device_node *np = client-dev.of_node;
+
+ for (i = 0;
+ i sizeof(dialog_dt_ids)/sizeof(dialog_dt_ids[0]);
+ i++)
+ if (of_device_is_compatible(np,
+ dialog_dt_ids[i].compatible))
+ id = da9052_i2c_id[i];
+ }
+#endif
Ick, this doesn't look terribly nice - are you sure this is idiomatic?
The bound for the iteration also looks a lot like ARRAY_SIZE().
signature.asc
Description: Digital signature
___
linaro-dev mailing list
linaro-dev@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH 2/2] regulator: da9052: add device tree support
On Thu, Apr 12, 2012 at 11:39:42PM +0800, Ying-Chun Liu (PaulLiu) wrote:
+#ifdef CONFIG_OF
+ struct device_node *nproot = da9052-dev-of_node;
+ struct device_node *np;
+ int c;
+
+ if (!nproot) {
+ ret = -ENODEV;
+ goto err;
+ }
+
+ nproot = of_find_node_by_name(nproot, regulators);
+ if (!nproot) {
+ ret = -ENODEV;
+ goto err;
+ }
+
+ c = 0;
+ for (np = of_get_next_child(nproot, NULL);
+ np != NULL;
+ np = of_get_next_child(nproot, np)) {
+ if (c == pdev-id) {
+ initdata = of_get_regulator_init_data(
+ pdev-dev, np);
+ break;
+ }
+ c++;
+ }
This is really quite unclear but it looks like this is relying on the
order of regulators in the OF table to match things. As I said in my
reply to the first patch this is really poor for usability and it's
also making the code here more obscure - we should be looking for the
regulator nodes by name.
signature.asc
Description: Digital signature
___
linaro-dev mailing list
linaro-dev@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH] Regulator: anatop-regulator: patching to device-tree property reg.
On Tue, Mar 27, 2012 at 03:54:01PM +0800, Ying-Chun Liu (PaulLiu) wrote: From: Ying-Chun Liu (PaulLiu) paul@linaro.org Change reg to anatop-reg-offset due to there is a warning of handling no size field in reg. This patch also adds the missing device-tree binding documentation. Applied, thanks, but please do split patches. signature.asc Description: Digital signature ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH v7 1/3] Documentation: common clk API
On Wed, Mar 21, 2012 at 11:38:58AM -0700, Saravana Kannan wrote: So it would be interesting to know more about why you (or anyone else) perceive that the Kconfig changes would be harmful. But the enthusiasm of the clock driver developers doesn't necessarily translate to users of the clock APIs (other driver devs). My worry with marking it as experimental in Kconfig and to a certain extent in the documentation is that it will discourage the driver devs from switching to the new APIs. If no one is using the new APIs, then platforms can't switch to the common clock framework These aren't new APIs, the clock API has been around since forever. For driver authors working on anything that isn't platform specific the issue has been that it's not implemented at all on the overwhelming majority of architectures and those that do all have their own random implementations with their own random quirks and with no ability for anything except the platform to even try to do incredibly basic stuff like register a new clock. Simply having something, anything, in place even if it's going to churn is a massive step forward here for people working with clocks. signature.asc Description: Digital signature ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH v7 1/3] Documentation: common clk API
On Wed, Mar 21, 2012 at 01:04:22PM -0700, Saravana Kannan wrote: Sure, prepare/unprepare are already there in the .h file. But they are stubs and have no impact till we move to the common clock framework or platforms move to them with their own implementation (certainly not happening in upstream, so let's leave that part out of this discussion). So. IMO, for all practical purposes, common clk fwk forces the move to these new APIs and hence IMO forces the new APIs. Sure, if you want to look at it from that point of view - anything wanting to run on a platform which uses the generic API needs to use them, but there's no blocker on the user from this (it can convert with or without the platform it runs on) - but it's hardly a tough sell. signature.asc Description: Digital signature ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH v10] mfd: Add anatop mfd driver
On Fri, Mar 16, 2012 at 04:16:56PM +0800, Ying-Chun Liu (PaulLiu) wrote: From: Ying-Chun Liu (PaulLiu) paul@linaro.org Anatop is a mfd chip embedded in Freescale i.MX6Q SoC. Anatop provides regulators and thermal. This driver handles the address space and the operation of the mfd device. Reviwed-by: Mark Brown broo...@opensource.wolfsonmicro.com signature.asc Description: Digital signature ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH v9] mfd: Add anatop mfd driver
On Thu, Mar 15, 2012 at 09:07:29AM +, Arnd Bergmann wrote: Very broadly speaking, I wonder whether we could use the regmap infrastructure for these things in the future, but I would first need to understand whether that is actually in the scope of regmap. It seems that you just need a subset of what regmap provides, so it could work, but it might not actually be better than what you have now. Mark, can you comment on that? There were some other mutterings about using regmap for memory mapped devices, mostly from the point of view of building framework features like this on top of it. regmap currently makes some assumptions that the I/O is going to be slow so approximately any amount of CPU time can usefully be spent on avoiding I/O but we can probably do something about that. It also uses mutexes to lock I/O which might not be the most sensible thing for memory mapped devices, but again that's solveable. Right now there's no memory mapping support but there's no reason that can't be added. In short, it does seem sensible to want to have some support for this for devices that use appropriate idioms. signature.asc Description: Digital signature ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH v13] Regulator: Add Anatop regulator driver
On Wed, Mar 14, 2012 at 10:29:12AM +0800, Ying-Chun Liu (PaulLiu) wrote: From: Ying-Chun Liu (PaulLiu) paul@linaro.org Anatop is an integrated regulator inside i.MX6 SoC. There are 3 digital regulators which controls PU, CORE (ARM), and SOC. And 3 analog regulators which controls 1P1, 2P5, 3P0 (USB). This patch adds the Anatop regulator driver. Applied, thanks. signature.asc Description: Digital signature ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH v12] Regulator: Add Anatop regulator driver
On Sat, Mar 10, 2012 at 11:13:08AM +0800, Ying-Chun Liu (PaulLiu) wrote: From: Ying-Chun Liu (PaulLiu) paul@linaro.org Anatop is an integrated regulator inside i.MX6 SoC. There are 3 digital regulators which controls PU, CORE (ARM), and SOC. And 3 analog regulators which controls 1P1, 2P5, 3P0 (USB). This patch adds the Anatop regulator driver. This looks OK but doesn't apply against current regulator code due to sorting in Kconfig and Makefile, you should always submit against current development versions of the subsystem. Please rebase and resend, this will also give you an opportunity to build test against the current subsystem :) signature.asc Description: Digital signature ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH v10] Regulator: Add Anatop regulator driver
On Fri, Mar 09, 2012 at 10:58:34AM +0100, Jean-Christophe PLAGNIOL-VILLARD wrote: I've modify the patch based on your review. However, the last one cannot be made because regulator_unregister is void return. so we have a issue here regulator_unregister MUST return an error conde The error handling on remove is totally irrelevant to merging this driver. If you want to work on changing this in the core you're more than welcome to spend your time on it, I'm really not sure it's really worth the time or effort for the systems we currently have, if someone actually has a system where it becomes relevant then they can work on it. signature.asc Description: Digital signature ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH v11] Regulator: Add Anatop regulator driver
On Fri, Mar 09, 2012 at 03:57:09PM +0800, Ying-Chun Liu (PaulLiu) wrote: From: Ying-Chun Liu (PaulLiu) paul@linaro.org Anatop is an integrated regulator inside i.MX6 SoC. There are 3 digital regulators which controls PU, CORE (ARM), and SOC. And 3 analog regulators which controls 1P1, 2P5, 3P0 (USB). This patch adds the Anatop regulator driver. This looks good apart from the issue Axel noted. signature.asc Description: Digital signature ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH v9] Regulator: Add Anatop regulator driver
On Wed, Mar 07, 2012 at 02:22:25PM +0100, Jean-Christophe PLAGNIOL-VILLARD wrote: +- compatible: Must be fsl,anatop-regulator +- vol-bit-shift: Bit shift for the register +- vol-bit-width: Number of bits used in the register +- min-bit-val: Minimum value of this register +- min-voltage: Minimum voltage of this regulator +- max-voltage: Maximum voltage of this regulator specific properites need to be prefix by the vendor This really doesn't seem at all sane for a device which is already vendor specific, it's just noise in the bindings. signature.asc Description: Digital signature ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH v9] Regulator: Add Anatop regulator driver
On Wed, Mar 07, 2012 at 04:36:22PM +0100, Jean-Christophe PLAGNIOL-VILLARD wrote: This really doesn't seem at all sane for a device which is already vendor specific, it's just noise in the bindings. No it's ...? Here is a good example as we have regulator generic binding vendor specific bindig It's not vendor specific, it's device specific and people are doing it even for devices with no generic bindings at all which is particularly silly. Device specific prefixes probably make sense, but vendor specific ones are just noise. signature.asc Description: Digital signature ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH v6 2/2] Regulator: Add Anatop regulator driver
On Sun, Mar 04, 2012 at 02:51:48PM +0800, Shawn Guo wrote: + sreg = devm_kzalloc(dev, sizeof(struct anatop_regulator), GFP_KERNEL); + if (!sreg) + return -EINVAL; + rdesc = devm_kzalloc(dev, sizeof(struct regulator_desc), GFP_KERNEL); + if (!rdesc) + return -EINVAL; Would something like the following be better? sreg = devm_kzalloc(dev, sizeof(*sreg) + sizeof(*rdesc), GFP_KERNEL); if (!sreg) return -ENOMEM; rdesc = (struct regulator_desc *)(sreg + 1); No, that sort of pointer arithmetic would be much worse - it's harder to read and more likely to break. However, embedding the regulator_desc in sreg would achieve the same result without the legibility issues. signature.asc Description: Digital signature ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [linux-pm] [PATCH 2/4] hwmon: exynos4: Move thermal sensor driver to driver/mfd directory
On Sat, Mar 03, 2012 at 04:36:05PM +0530, Amit Daniel Kachhap wrote: This movement is needed because the hwmon entries and corresponding sysfs interface is a duplicate of utilities already provided by driver/thermal/thermal_sys.c. The goal is to place it in mfd folder and add necessary calls to get the temperature information. --- a/Documentation/hwmon/exynos4_tmu +++ /dev/null Moving this seems to be a failure, the device is exposing a hwmon interface even if you've moved the code to mfd (though it doesn't actually look like a multi-function device at all as far as I can see - usually a MFD would have a bunch of unrelated functionality while this has one function used by two subsystems). If anything it looks like the ADC driver ought to be moved into IIO with either generic or Exynos specific function drivers layered on top of it in hwmon and thermal making use of the values that are read. ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH v7 1/2] mfd: Add anatop mfd driver
On Sun, Mar 04, 2012 at 01:39:12AM +0800, Ying-Chun Liu (PaulLiu) wrote: From: Ying-Chun Liu (PaulLiu) paul@linaro.org Anatop is a mfd chip embedded in Freescale i.MX6Q SoC. Anatop provides regulators and thermal. This driver handles the address space and the operation of the mfd device. Please stop sending new patches as followups to old versions, it tends not to do the right thing in mail software. signature.asc Description: Digital signature ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH v5 2/2] Regulator: Add Anatop regulator driver
On Thu, Mar 01, 2012 at 05:10:52PM +0800, Ying-Chun Liu (PaulLiu) wrote:
+ if (IS_ERR(rdev)) {
+ dev_err(pdev-dev, failed to register %s\n,
+ rdesc-name);
+ kfree(rdesc-name);
+ return PTR_ERR(rdev);
+ }
+
+ return 0;
+}
+
+int anatop_regulator_remove(struct platform_device *pdev)
+{
+ struct regulator_dev *rdev = platform_get_drvdata(pdev);
+ regulator_unregister(rdev);
+ return 0;
Looks mostly good but this leaks rdesc-name which was allocated with
kstrdup() in the probe() function.
signature.asc
Description: Digital signature
___
linaro-dev mailing list
linaro-dev@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH v5 1/2] mfd: Add anatop mfd driver
On Thu, Mar 01, 2012 at 05:10:51PM +0800, Ying-Chun Liu (PaulLiu) wrote: + spin_lock(adata-reglock); + val = readl(adata-ioreg + addr); + spin_unlock(adata-reglock); Do you really need to take a lock for a single read operation from a memory mapped register? I'd expect this to be atomic in itself. You need to lock on read/modify/write cycles to make sure that you don't get a read/read/modify/modify/write/write and misplace one of the modifies but that's not an issue for an isolated read. signature.asc Description: Digital signature ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH 0/4] twl-regulator DT adaptation and updates to add new regulators
On Tue, Feb 28, 2012 at 03:09:09PM +0530, Rajendra Nayak wrote: Hi Mark, Here is a consolidated series which adds DT support for twl regulator driver and adds support for VDD1/2/3 regulator and support for fixed LDO V1V8 and V2V1. The patches are based on -next and tested on omap3 beagle and omap4 panda boards. Applied all, thanks. Please do try to use subject lines consistent with the subsystem. signature.asc Description: Digital signature ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH v3 0/2] Device tree support for TWL regulators
On Tue, Feb 28, 2012 at 11:11:48AM +0530, Rajendra Nayak wrote: changes have no dependencies with any other DT series. I will repost all of Tero/Peter and my changes (to add DT support to the driver) as one single series and drop the dts file updates, which I guess can go via Tony/OMAP tree. Yes, that sounds like a good plan - the DTS changes are largely orthogonal to the code changes and don't need to go via the same path (this is true in general, the DTSs are pretty horrible for merge issues). signature.asc Description: Digital signature ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH v3 0/2] Device tree support for TWL regulators
On Mon, Feb 27, 2012 at 06:01:20PM +0530, Rajendra Nayak wrote: Depending on what order Mark happens to pull them in, I am fine re-sending adding support for the 2 twl6030 fixed regulators. Please can you guys come up with a single unified series for this stuff - I'll hold off on applying anything to allow you to do this. signature.asc Description: Digital signature ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH v3 0/2] Device tree support for TWL regulators
On Mon, Feb 27, 2012 at 02:52:05PM +0100, Cousson, Benoit wrote: On 2/27/2012 2:41 PM, Mark Brown wrote: On Mon, Feb 27, 2012 at 06:01:20PM +0530, Rajendra Nayak wrote: Please can you guys come up with a single unified series for this stuff - I'll hold off on applying anything to allow you to do this. The issue is that the initial TWL regulator series from Rajendra will depend on the twl core DT support I have that depends on the irq_domain series from Grant... Really? That's the first I've heard of any such dependency here. Is it a build time dependency or is it just something that's required to make the code actually spring into life? It looks like it's the latter but you're saying there's an actual dependency. There's also more than that, there's also at least Tero submitting some other stuff separately (and his stuff won't play with DT...) and I think Peter also. It really fees like there's a bunch of people working on different things without talking to each other here. So I guess, it will be easy for us to split the regulator patches from the DTS ones to have at least the driver changes merged by you. Then Tony might be able to pull all the DTS in one series and thus avoid the various merge conflict that will happen since most OMAP drivers are hacking the same DTS files. Does that make sense? Or do you think it will be even worst separating the patches? Unless there's a compile time dependency I don't think we need to refactor anything here, what I'm seeing at the minute looks OK from a merge point of view except that there appears to be a lack of coordination between the various serieses. signature.asc Description: Digital signature ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH v3 0/2] Device tree support for TWL regulators
On Mon, Feb 27, 2012 at 03:21:26PM +0100, Cousson, Benoit wrote: Mmm, it is written in Rajendra's changelog: -2- All common regulator nodes for twl4030 and twl6030 are now defined in the twl4030.dtsi and twl6030.dtsi instead of Oh, it's buried at the end of a rather verbose inter-patch changelog, that's not exactly visible :( The good point is that a missing or broken DTS will indeed not break the build, it will just not boot the platform properly. But that still an important dependency to me. If adding device tree support breaks existing platforms something is going wrong, while you're pulling things together device tree might not work until all the support makes it in but the old non-DT code should continue to function. There's also more than that, there's also at least Tero submitting some other stuff separately (and his stuff won't play with DT...) and I think Peter also. It really fees like there's a bunch of people working on different things without talking to each other here. That's the problem with MFD devices that are doing everything from audio to power including the coffee... Well, in this case everyone's only working on the regulator stuff, or at least only overlapping on that. The twl drivers aren't helping here - the structure is *very* non-standard and seems to make updates more fiddly than they should be as you're not just adding stuff to tables. Every update seems to need explicit code and per-change data adding which is making life harder. If things looked more standard from a code point of view it'd probably reduce the pain a lot. signature.asc Description: Digital signature ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH v3 1/2] regulator: twl: adapt twl-regulator driver to dt
On Thu, Feb 23, 2012 at 05:05:53PM +0530, Rajendra Nayak wrote: Modify the twl regulator driver to extract the regulator_init_data from device tree when passed, instead of getting it through platform_data structures (on non-DT builds) This doesn't apply to current -next, I expect because of the SMPS regulator changes which went in. Please always submit patches against -next. There's also a patch from Tero adding support for some additional regulators which is floating around and would conflict with this. Could you guys please coordinate with each other on this? signature.asc Description: Digital signature ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH v4 2/2] Regulator: Add Anatop regulator driver
On Fri, Feb 10, 2012 at 10:36:38PM -0800, Shawn Guo wrote: On Thu, Feb 09, 2012 at 04:51:26AM +0800, Ying-Chun Liu (PaulLiu) wrote: + rval = of_get_property(np, min-voltage, NULL); + if (rval) + sreg-rdata-min_voltage = be32_to_cpu(*rval); + rval = of_get_property(np, max-voltage, NULL); + if (rval) + sreg-rdata-max_voltage = be32_to_cpu(*rval); We need a sensible binding document to understand those. But at least, shouldn't min-voltage and max-voltage be retrieved as the common regulator binding documented in Documentation/devicetree/bindings/regulator/regulator.txt? Normally this would be a bad idea as the set of voltages that can safely be used on a given board might differ from those which are supported by the device. However in this case you might be OK as this is all internal to the SoC and so presumably won't vary from board to board. signature.asc Description: Digital signature ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH v4 2/2] Regulator: Add Anatop regulator driver
On Thu, Feb 09, 2012 at 04:51:26AM +0800, Ying-Chun Liu (PaulLiu) wrote:
Overall this is looking pretty good, a few issues but relatively minor.
+ if (uv anatop_reg-rdata-min_voltage
+ || uv anatop_reg-rdata-max_voltage) {
+ if (max_uV anatop_reg-rdata-min_voltage)
+ uv = anatop_reg-rdata-min_voltage;
+ else
+ return -EINVAL;
+ }
This looks buggy (and is quite hard to follow). The check for the
voltage being above the minimum voltage is fine but surely if the
voltage is too high then the first if statement will be true and max_uV
will be greater than the minimum voltage so the second if will be true.
This will cause us to choose the minimum voltage that the regulator can
do which is less than the minimum voltage requested.
You probably shouldn't be checking for the upper end of the range at all
here...
+ if (anatop_reg-rdata-control_reg) {
+ sel = (uv - anatop_reg-rdata-min_voltage) / 25000;
+ val = anatop_reg-rdata-min_bit_val + sel;
+ *selector = sel;
+ pr_debug(%s: calculated val %d\n, __func__, val);
...instead check that selector is in range here.
+ anatop_reg-rdata-mfd-write(anatop_reg-rdata-mfd,
+ anatop_reg-rdata-control_reg,
+ anatop_reg-rdata-vol_bit_shift,
+ anatop_reg-rdata-vol_bit_size,
+ val);
Just have a write() function in the MFD.
+ memset(rdesc, 0, sizeof(struct regulator_desc));
+ rdesc-name = kstrdup(of_get_property(np, regulator-name, NULL),
+ GFP_KERNEL);
You should add binding documentation for the device since it's OF based.
+ sreg-rdata-name = kstrdup(rdesc-name, GFP_KERNEL);
Can't you just point to rdesc-name?
+ if (IS_ERR(rdev)) {
+ dev_err(pdev-dev, failed to register %s\n,
+ rdesc-name);
+ return PTR_ERR(rdev);
+ }
All your kstrdup()s need to be unwound in error and free cases.
+int anatop_regulator_init(void)
+{
+ return platform_driver_register(anatop_regulator);
+}
+
+void anatop_regulator_exit(void)
+{
+ platform_driver_unregister(anatop_regulator);
+}
+
+postcore_initcall(anatop_regulator_init);
+module_exit(anatop_regulator_exit);
These should be adjacent to the functions.
+MODULE_AUTHOR(Freescale Semiconductor, Inc.);
Either omit this or put one or more humans as the author.
+struct anatop_regulator {
+ struct regulator_desc *regulator;
+ struct regulator_init_data *initdata;
+ struct anatop_regulator_data *rdata;
+};
May as well just merge the regulator data in here - it's not ever used
except with a 1:1 relationship between them. Could also directly
embed the desc and init_data, then you just have one allocation for the
data rather than a series to error check.
+int anatop_register_regulator(
+ struct anatop_regulator *reg_data, int reg,
+ struct regulator_init_data *initdata);
This looks like it's not defined any more so could be removed and since
you only appear to support OF the entire header could be merged into the
driver.
signature.asc
Description: Digital signature
___
linaro-dev mailing list
linaro-dev@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: Linaro Audio development ideas for 12.02 and beyond
On Thu, Jan 26, 2012 at 02:26:06PM -0600, Kurt Taylor wrote: Is this complete? Absolutely not. This is meant to be a place to capture and refine ideas before creating cards and/or blueprints for them. In other words, this should compliment the existing work and backlog already in LP. Looks good. I'm not sure of the need for a PulseAudio sink for TinyALSA - it's all the same thing under the hood, the reasons that people choose TinyALSA over the regular ALSA library is mainly the licensing on the regular one and PulseAudio has all the same licensing issues. For the compressed audio passthrough it'd be good to see people working on making this stuff work with the compressed API that Intel recently got merged into ALSA, though that's a larger goal and should probably be listed separately. ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH V2 1/4] cpufreq: add arm soc generic cpufreq driver
On Fri, Dec 16, 2011 at 06:30:59PM +0800, Richard Zhao wrote:
+ if (higher cpu_reg)
+ regulator_set_voltage(cpu_reg,
+ cpu_volts[index], cpu_volts[index]);
+
+ ret = clk_set_rate(cpu_clk, freq);
+ if (ret != 0) {
+ printk(KERN_DEBUG cannot set CPU clock rate\n);
+ return ret;
+ }
+
+ if (!higher cpu_reg)
+ regulator_set_voltage(cpu_reg,
+ cpu_volts[index], cpu_volts[index]);
This appears to reintroduce the setting of an exact voltage which I'm
sure was fixed in previous versions of the patch.
+static struct cpufreq_driver arm_cpufreq_driver = {
+ .flags = CPUFREQ_STICKY,
+ .verify = arm_verify_speed,
+ .target = arm_set_target,
+ .get = arm_get_speed,
+ .init = arm_cpufreq_init,
+ .exit = arm_cpufreq_exit,
+ .name = arm,
+};
This code doesn't actually look terribly ARM specific...
+ printk(KERN_INFO ARM SoC generic CPU frequency driver\n);
Do we need this?
___
linaro-dev mailing list
linaro-dev@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH V2 1/4] cpufreq: add arm soc generic cpufreq driver
On Wed, Jan 18, 2012 at 11:39:50AM +, Mark Brown wrote: This appears to reintroduce the setting of an exact voltage which I'm sure was fixed in previous versions of the patch. Erk, sorry - it looks like the device tree list has quite a bit of lag in moderation and sent out some old patches which I didn't notice. All this has been fixed in subsequent verisons. ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH] mx53_loco: add DA9053 PMIC support
On Tue, Jan 17, 2012 at 01:10:53AM +0800, Ying-Chun Liu (PaulLiu) wrote:
+#define DA9052_LDO1_VOLT_UPPER 1800
+#define DA9052_LDO1_VOLT_LOWER 600
+#define DA9052_LDO1_VOLT_STEP50
This is almost certainly wrong - you should rarely if ever need the
voltage step in a consumer or machine driver.
+#define DA9052_LDO(max, min, rname, suspend_mv) \
+{\
+ .constraints = {\
+ .name = (rname), \
+ .max_uV = (max) * 1000,\
+ .min_uV = (min) * 1000,\
+ .valid_ops_mask = REGULATOR_CHANGE_VOLTAGE\
+ |REGULATOR_CHANGE_STATUS | REGULATOR_CHANGE_MODE,\
+ .valid_modes_mask = REGULATOR_MODE_NORMAL,\
This looks *very* odd - apart from anything else you're setting
REGULATOR_CHANGE_MODE but only permitting one mode. You're also
allowing things to be changed but not providing any consumers which
means nothing can ever change any of them.
It looks awfully like you've just copied the supported feature set of
the PMIC rather than configured things for your board.
+static struct regulator_init_data da9052_regulators_init[] = {
+ /* BUCKS */
+ DA9052_LDO(DA9052_BUCK_CORE_PRO_VOLT_UPPER,
+DA9052_BUCK_CORE_PRO_VOLT_LOWER, DA9052_BUCK_CORE, 850),
If you're going to specify names they should be names which are
meaningful for your board - usually the name used to identify the
relevant rail or rails in the schematic.
+#define MX53_LOCO_DA9052_IRQ (6*32 + 11) /* GPIO7_11 */
+
+static int __init loco_da9052_init(struct da9052 *da9052)
+{
+ /* Configuring for DA9052 interrupt servce */
+ /* s3c_gpio_setpull(DA9052_IRQ_PIN, S3C_GPIO_PULL_UP); */
+
I apprecate that my code can be inspirational at times but you probably
don't want to cut'n'paste this bit. :)
+ /* Set interrupt as LOW LEVEL interrupt source */
+ irq_set_irq_type(gpio_to_irq(MX53_LOCO_DA9052_IRQ),
+ IRQF_TRIGGER_LOW);
Why is this needed? The PMIC driver should do the right thing when it
requests the interrupt...
@@ -273,6 +397,10 @@ static struct i2c_board_info mx53loco_i2c_devices[] = {
{
I2C_BOARD_INFO(mma8450, 0x1C),
},
+ {
+ I2C_BOARD_INFO(da9053-aa, 0x90 1),
+ .platform_data = da9052_plat,
+ },
You're mixing different ways of numbering I2C devices here.
___
linaro-dev mailing list
linaro-dev@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH v4 4/7] cpufreq: add clk-reg cpufreq driver
On Tue, Jan 03, 2012 at 01:47:09PM +, Russell King - ARM Linux wrote: On Tue, Jan 03, 2012 at 09:25:30PM +0800, Richard Zhao wrote: In latest v6 version, I get clk transition latency from dt property, and get regulator transition latency from regulator API. Could you please help review other arm common changes in v6 version? You didn't get my point: how do you specify a clock transition latency for a clock with a PLL when the data sheets don't tell you what that is, and they instead give you a bit to poll? I'd rather suspect you'll find there are actually specs for these things but they're hidden in the electrical characteristics which don't tend to go into the published datasheets (though ). Not useful if we don't actually see them though. Do you: (a) make up some number and hope that it's representative (b) not specify any transition latency These are the traditional approaches, pioneered by essentially every existing cpufreq driver (well, make up is a bit harsh - usually the numbers are measured, though possibly only on a limited set of systems). (c) think about the problem _now_ and define what it means for a clock without a transition latency. This would be nice, in both the clock and cpufreq code (the cpufreq code is pretty limited here in that it assumes an equal transition latency for all transitions which isn't the case usually). You can generally do something useful with measurement, probably we can arrange to measure the times we're seeing on the actua system or something. ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH V5 4/7] cpufreq: add clk-reg cpufreq driver
On Wed, Dec 28, 2011 at 11:31:29AM +0800, Richard Zhao wrote:
On Wed, Dec 28, 2011 at 11:14:10AM +0800, Richard Zhao wrote:
+ if (cpu_reg) {
+ ret = regulator_is_supported_voltage(cpu_reg,
+ cpu_volts[i * 2], cpu_volts[i * 2 + 1]);
Is there any reason you didn't export symbol regulator_is_supported_voltage?
and also it don't have !REGULATOR dummy implementation.
regulator_set_voltage_time and some other functions don't have dummy one
either.
You can't usefully work with voltages without knowing what the actual
voltages are - the only sensible stubs we could provide would return
errors but then any driver using the stubs would probably fail to do
whatever it was doing. With enable and disable we can sensibly stub
things out with an always on regulator.
___
linaro-dev mailing list
linaro-dev@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH V5 4/7] cpufreq: add clk-reg cpufreq driver
On Wed, Dec 28, 2011 at 08:05:20PM +0800, Richard Zhao wrote: Looks like the problem with your mail client is that it's wrapping at exactly 80 characters which is too little - you need to leave space for being quoted. On Wed, Dec 28, 2011 at 11:42:37AM +, Mark Brown wrote: You can't usefully work with voltages without knowing what the actual voltages are - the only sensible stubs we could provide would return errors but then any driver using the stubs would probably fail to do whatever it was doing. With enable and disable we can sensibly stub things out with an always on regulator. Sorry, I can not get your point here. Let me describe the problem I met: - regulator_is_supported_voltage is not exported. when I build clk-reg-cpufreq as kernel module, there's a link error. This is an oversight, I've just fixed it. - I saw linux/regulator/consumer.h has some dummy functions if !REGULATOR. I tried to make clk-reg-cpufreq driver work even !REGULATOR. I think that's why the dummy functions are there. If regulator_get return NULL, it'll avoid calling other regulator functions. But regulator_is_supported_voltage and regulator_set_voltage_time don't have such dummy ones. Undefined functions. I can only repeat what I wrote above explaining why no stubs are provided. ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH V5 4/7] cpufreq: add clk-reg cpufreq driver
On Wed, Dec 28, 2011 at 08:40:56PM +0800, Richard Zhao wrote: On Wed, Dec 28, 2011 at 12:14:04PM +, Mark Brown wrote: On Wed, Dec 28, 2011 at 08:05:20PM +0800, Richard Zhao wrote: Looks like the problem with your mail client is that it's wrapping at exactly 80 characters which is too little - you need to leave space for being quoted. I'm using vim. any suggestion for auto-wrapping? :set tw=72 It'd also be useful to leave blank lines You can't usefully work with voltages without knowing what the actual voltages are if people don't enable REGULATOR, the stubs (if you mean the dummy func) are only be used to pass build. The code is optimized out by compiler. No, this is not the case. All the functions have return values which still need to be handled gracefully by drivers using those functions. - the only sensible stubs we could provide would return errors but then any driver using the stubs would probably fail to do whatever it was doing. In this case, If regulator_get return NULL, I won't call other regulator functions at runtime. That should be OK for your use case but it might not be sensible for other cases. Any stubs for this provided by the core need to work well for any user. - I saw linux/regulator/consumer.h has some dummy functions if !REGULATOR. I tried to make clk-reg-cpufreq driver work even !REGULATOR. I think that's why the dummy functions are there. If regulator_get return NULL, it'll avoid calling other regulator functions. But regulator_is_supported_voltage and regulator_set_voltage_time don't have such dummy ones. Undefined functions. I can only repeat what I wrote above explaining why no stubs are provided. One word. You mean I have to always depends on REGULATOR config, right? Yes. ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH V5 4/7] cpufreq: add clk-reg cpufreq driver
On Wed, Dec 28, 2011 at 09:06:20PM +0800, Shawn Guo wrote: On Wed, Dec 28, 2011 at 12:47:40PM +, Mark Brown wrote: One word. You mean I have to always depends on REGULATOR config, right? Yes. I do not care too much. But it puts the driver on an interesting position, that is it can work without a regulator driver backing the cpu voltage but it has to enable CONFIG_REGULATOR. Well, the other option is ifdefs in your driver. ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH v4 4/7] cpufreq: add clk-reg cpufreq driver
On Tue, Dec 27, 2011 at 09:51:10AM +0800, Richard Zhao wrote: On Mon, Dec 26, 2011 at 02:22:34PM +, Mark Brown wrote: Fix your mailer to word wrap properly please. If you mean last mail I sent, I didn't see anything wrong. I use mutt. It's wrapping at a bit more than 80 columns a lot of the time. So what's your suggestion? We can not set transition_latency to set random number. As I've repeatedly said I think you should define it to be the latency for the SoC only, not for the regulators. Sometimes, regulators are in SoC too. To avoid confusion, I'll use below: clk-trans-latency = 61036; Makes sense. ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCHv2] Regulator: Add Anatop regulator driver
On Tue, Dec 27, 2011 at 06:06:27PM +0800, Ying-Chun Liu (PaulLiu) wrote: (2011年12月22日 19:33), Mark Brown wrote: +#include linux/platform_device.h +#include linux/regulator/machine.h Why does your regulator driver need this? That suggests a layering violation. Sorry, I'm not sure what does this mean. But if I want to access regulator_constraints, shouldn't I include this header file? If your regulator driver wants to access regulator_constraints it is doing something wrong, that should never be required. ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH v4 4/7] cpufreq: add clk-reg cpufreq driver
On Sat, Dec 24, 2011 at 11:52:29PM +0800, Richard Zhao wrote: On Sat, Dec 24, 2011 at 01:42:29PM +, Mark Brown wrote: On Sat, Dec 24, 2011 at 09:28:33PM +0800, Richard Zhao wrote: If you think regulator thansition latency is board specific, then the board dts can overrite it. We should just query this information from the regulator subsystem (there's hooks but currently nothing implements them). The regulators can define their own bindings if they need to read it from device tree, most of them should be able to do this as a function of knowing about the device. None of this is specific to cpufreq so cpufreq shouldn't have to define its own support for this. I'd like to query the latency by call clk and regulator APIs. but as you said both of them have not implemented it yet. I think, for now, we can use the The *call* is there in the regulator subsystem, it's just that none of the drivers back it up with an actual implementation yet. Which turns out to be a good thing as cpufreq can't currently understand variable latencies and the governors don't deal well with non-trivial latencies anyway. property to get the total latency. Once I can get it at runtime, I'll remove it. So the definition of trans-latency is just the same as cpufreq transition_latency, people get less confused. What do you think? The problem with device tree is that once you've defined a binding you're stuck with it, it's very hard to change - witness all the magic number based stuff with the interrupt bindings for example ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH v4 4/7] cpufreq: add clk-reg cpufreq driver
On Mon, Dec 26, 2011 at 09:44:52PM +0800, Richard Zhao wrote: On Mon, Dec 26, 2011 at 11:10:30AM +, Mark Brown wrote: Fix your mailer to word wrap properly please. The *call* is there in the regulator subsystem, it's just that none of the drivers back it up with an actual implementation yet. Which turns out to be a good thing as cpufreq can't currently understand variable latencies and the governors don't deal well with non-trivial latencies anyway. but clk API don't have such calls. and many SoCs only adjust clk frequencies, using one single voltage. I've not suggested doing this in the clock API, only for the regulator. For the clocks it's less clear that it's useful as you don't have the bulk operations and it's much rarer to need them. The problem with device tree is that once you've defined a binding you're stuck with it, it's very hard to change - witness all the magic number based stuff with the interrupt bindings for example So what's your suggestion? We can not set transition_latency to set random number. As I've repeatedly said I think you should define it to be the latency for the SoC only, not for the regulators. ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH v4 4/7] cpufreq: add clk-reg cpufreq driver
On Sat, Dec 24, 2011 at 04:55:42PM +0800, Richard Zhao wrote: On Fri, Dec 23, 2011 at 01:18:51PM +, Mark Brown wrote: +- trans-latency : transition_latency, in unit of ns. trans-latency should really say what latency is being measured (the CPU core only or the whole operation). dts only descibe hw info. so the transition latency is for hw. - trans-latency : transition latency of HW, in unit of ns. The issue is that you need to clarify which hardware this is - it's the clocks only, not the time for regulators. ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCHv2] Regulator: Add Anatop regulator driver
On Thu, Dec 22, 2011 at 11:33:38AM +, Mark Brown wrote:
On Wed, Dec 21, 2011 at 05:03:31PM +0800, Ying-Chun Liu (PaulLiu) wrote:
+ if (anatop_reg-rdata-control_reg) {
+ val = anatop_reg-rdata-min_bit_val +
+ (uv - reg-constraints-min_uV) / 25000;
You're not checking that the resulting voltage matches the constraints
or updating selector.
Also on re-reading this looks *very* broken - you're using the
constraints value in your set_voltage() operation. The runtime
constraints a system has should have *no* impact on the way you ask for
a specific voltage from the regulator.
___
linaro-dev mailing list
linaro-dev@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH v4 4/7] cpufreq: add clk-reg cpufreq driver
On Sat, Dec 24, 2011 at 09:28:33PM +0800, Richard Zhao wrote: On Sat, Dec 24, 2011 at 12:24:11PM +, Mark Brown wrote: - trans-latency : transition latency of cpu freq and related regulator, in unit of ns. Does it look better? I think it shouldn't include the regulator part of things. If you think regulator thansition latency is board specific, then the board dts can overrite it. We should just query this information from the regulator subsystem (there's hooks but currently nothing implements them). The regulators can define their own bindings if they need to read it from device tree, most of them should be able to do this as a function of knowing about the device. None of this is specific to cpufreq so cpufreq shouldn't have to define its own support for this. ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH v4 4/7] cpufreq: add clk-reg cpufreq driver
On Thu, Dec 22, 2011 at 03:09:10PM +0800, Richard Zhao wrote: The driver get cpu operation point table from device tree cpu0 node, and adjusts operating points using clk and regulator APIs. Reviewed-by: Mark Brown broo...@opensource.wolfsonmicro.com but one nit: +Required properties in /cpus/cpu@0: +- cpu-freqs : cpu frequency points it support, in unit of Hz. +- cpu-volts : cpu voltages required by the frequency point at the same index, + in unit of uV. +- trans-latency : transition_latency, in unit of ns. trans-latency should really say what latency is being measured (the CPU core only or the whole operation). ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH] regulator: use usleep_range() instead of mdelay()/udelay()
On Wed, Dec 21, 2011 at 11:04:53AM +0400, Dmitry Antipov wrote:
From 00753f3d48c4b6c45c1778c3e37bc9949ed79e77 Mon Sep 17 00:00:00 2001
From: Dmitry Antipov dmitry.anti...@linaro.org
Date: Wed, 21 Dec 2011 11:01:42 +0400
Subject: [PATCH] regulator: use usleep_range() instead of mdelay()/udelay()
Follow the instructions in Documentation/SubmittingPatches.
- if (delay = 1000) {
- mdelay(delay / 1000);
- udelay(delay % 1000);
- } else if (delay) {
- udelay(delay);
- }
+ usleep_range(delay, delay + 1000);
These two aren't obviously equivalent in several respects and you
haven't provided any explanation for what you're trying to accomplish.
___
linaro-dev mailing list
linaro-dev@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCHv2] Regulator: Add Anatop regulator driver
On Wed, Dec 21, 2011 at 05:03:31PM +0800, Ying-Chun Liu (PaulLiu) wrote:
From: Ying-Chun Liu (PaulLiu) paul@linaro.org
Anatop regulator driver is used by i.MX6 SoC. The regulator provides
controlling the voltage of PU, CORE, SOC, and some devices. This patch
adds the Anatop regulator driver.
It's still not at all clear to me what an Anatop regulator is or why
everything is being done as platform data.
+config REGULATOR_ANATOP
+ tristate ANATOP LDO regulators
+ depends on SOC_IMX6
+ default y if SOC_IMX6
This isn't great, it might be on your reference design but people are
going to change that.
+#include linux/platform_device.h
+#include linux/regulator/machine.h
Why does your regulator driver need this? That suggests a layering
violation.
+static int anatop_set_voltage(struct regulator_dev *reg, int min_uV,
+ int max_uV, unsigned *selector)
+{
+ struct anatop_regulator *anatop_reg = rdev_get_drvdata(reg);
+ u32 val, rega;
+ int uv;
+
+ uv = max_uV;
This looks wrong, you should be aiming to get as close as possible to
the minimum not the maximum.
+ if (anatop_reg-rdata-control_reg) {
+ val = anatop_reg-rdata-min_bit_val +
+ (uv - reg-constraints-min_uV) / 25000;
You're not checking that the resulting voltage matches the constraints
or updating selector.
+ } else {
+ pr_debug(Regulator not supported.\n);
Why are you logging an error as a debug message? You should also use
dev_ logging.
+static int anatop_get_voltage(struct regulator_dev *reg)
+{
Implement this as get_voltage_sel()
+static int anatop_enable(struct regulator_dev *reg)
+{
+ return 0;
+}
+
+static int anatop_disable(struct regulator_dev *reg)
+{
+ return 0;
+}
+
+static int anatop_is_enabled(struct regulator_dev *reg)
+{
+ return 1;
+}
The regulator clearly doesn't have enable or disable support at all, it
shouldn't have these operations.
+static struct regulator_ops anatop_rops = {
+ .set_voltage= anatop_set_voltage,
+ .get_voltage= anatop_get_voltage,
You should implement list_voltage() as well.
+static struct regulator_desc anatop_reg_desc[] = {
+ {
+ .name = vddpu,
+ .id = ANATOP_VDDPU,
+ .ops = anatop_rops,
+ .irq = 0,
No need to set zero fields. It's also *very* odd to see a table
explicitly listing regulators by name in a driver that doesn't know
which registers it's working with.
+int anatop_regulator_probe(struct platform_device *pdev)
+{
+ struct regulator_desc *rdesc;
+ struct regulator_dev *rdev;
+ struct anatop_regulator *sreg;
+ struct regulator_init_data *initdata;
+
+ sreg = platform_get_drvdata(pdev);
+ initdata = pdev-dev.platform_data;
+ sreg-cur_current = 0;
+ sreg-next_current = 0;
+ sreg-cur_voltage = 0;
You're trying to read the driver data but you haven't set any. This is
going to crash.
+ init_waitqueue_head(sreg-wait_q);
This waitqueue appears to never be referenced.
+ if (pdev-id ANATOP_SUPPLY_NUM) {
+ rdesc = kzalloc(sizeof(struct regulator_desc), GFP_KERNEL);
devm_kzalloc() and check the return value.
+ memcpy(rdesc, anatop_reg_desc[ANATOP_SUPPLY_NUM],
+ sizeof(struct regulator_desc));
+ rdesc-name = kstrdup(sreg-rdata-name, GFP_KERNEL);
This looks really confused, you've got some regulators embedded into the
driver and some with things passed in as platform data. Either approach
should be fine but mixing both complicates things needlessly.
+ } else
+ rdesc = anatop_reg_desc[pdev-id];
Use braces on both sides of the if.
+ pr_debug(probing regulator %s %s %d\n,
+ sreg-rdata-name,
+ rdesc-name,
+ pdev-id);
A lot of this logging looks like it's just replicating logging from the
core.
+ /* register regulator */
+ rdev = regulator_register(rdesc, pdev-dev,
+ initdata, sreg);
The driver isn't doing anything to for example map the register bank
it's using.
+int anatop_register_regulator(
+ struct anatop_regulator *reg_data, int reg,
+ struct regulator_init_data *initdata)
+{
Eew, no. Just register the platform device normally.
+ int mode;
+ int cur_voltage;
+ int cur_current;
+ int next_current;
These appear to be unused and are rather obscure.
+struct anatop_regulator_data {
+ char name[80];
const char *.
+ char *parent_name;
What's this?
___
linaro-dev mailing list
linaro-dev@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH V3 4/7] cpufreq: add generic cpufreq driver
On Wed, Dec 21, 2011 at 09:43:34AM +, Arnd Bergmann wrote: On Wednesday 21 December 2011, Richard Zhao wrote: Mark, cpu node is not a struct device, sys_device instead. I can not find regulator via device/dt node. Can I still use the string to get regulator after converting to DT? I believe Kay and Greg have the plan to unify class and bus in sysfs, which implies turning sys_device into a derived class of device instead of kobject. If that understanding is correct, we might as well do that now so we can attach a device_node to a sys_device. Kay, does this make sense? I'm noy Kay but I think even if it's not what we end uo doing internally it's a sensible design for the device tree representation. ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH V3 4/7] cpufreq: add generic cpufreq driver
On Wed, Dec 21, 2011 at 12:44:57PM +0100, Kay Sievers wrote: We will convert all classes to buses over time time, and have a single type of device and a single type of subsystem. Are there any conversions that have been done already that I can look at for reference? ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH V3 4/7] cpufreq: add generic cpufreq driver
On Wed, Dec 21, 2011 at 10:19:11PM +0800, Richard Zhao wrote: Even cpu node is device, I still need to find a way to get it. I think it's better have another patch to fix the regulator dt binding in cpu node. I'll not include it in this patch series. I'd expect this to be easy if we can find any device tree data for the CPU at all? It's just another piece of data no different to the clock rates or whatever. ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH V2 1/4] cpufreq: add arm soc generic cpufreq driver
On Fri, Dec 16, 2011 at 06:30:59PM +0800, Richard Zhao wrote:
+
+ if (higher cpu_reg)
+ regulator_set_voltage(cpu_reg,
+ cpu_volts[index], cpu_volts[index]);
This is really bad, you're only supporting the configuration of a
specific voltage which doesn't reflect what hardware does (things will
be specified as a range of voltages) and will needlessly cause
interoperability problems between chips and regulators if the regulator
can't hit the *exact* voltage requested. There's a good solid reason
why the regulator API specifies everything in terms of voltage ranges.
+ ret = clk_set_rate(cpu_clk, freq);
+ if (ret != 0) {
+ printk(KERN_DEBUG cannot set CPU clock rate\n);
+ return ret;
+ }
This error checking is really random - you're ignoring some errors and
logging the errors you do detect as debug messages which seems odd.
Similar issues apply throughough.
+ if (cpu_volts) {
+ cpu_reg = regulator_get(NULL, cpu);
+ if (IS_ERR(cpu_reg)) {
+ printk(KERN_WARNING
+ cpufreq: regulator cpu get failed.\n);
+ cpu_reg = NULL;
+ }
+ }
You should log what the error was. You're also not doing anything to
check that the voltage ranges required by the frequencies are actually
supported on this system, the driver should double check this so that
governors don't sit there trying to set voltages that are impossible on
a given board.
___
linaro-dev mailing list
linaro-dev@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH V3 4/7] cpufreq: add generic cpufreq driver
On Mon, Dec 19, 2011 at 11:21:40AM +0800, Richard Zhao wrote:
It support single core and multi-core ARM SoCs. But currently it assume
all cores share the same frequency and voltage.
My comments on the previous version of the patch still apply:
- The voltage ranges being set need to be specified as ranges.
- Frequencies that can't be supported due to limitations of the
available supplies shouldn't be exposed to users.
- The driver needs to handle errors.
+Required properties in /cpus/cpu@0:
+- compatible : generic-cpufreq
+- cpu-freqs : cpu frequency points it support
+- cpu-volts : cpu voltages required by the frequency point at the same index
+- trans-latency : transition_latency
You need to define units for all of these, and for the transition
latency you need to be clear about what's being measured (it looks like
the CPU time only, not any voltage ramping).
You also need to define how the core supplies get looked up.
+ /* Manual states, that PLL stabilizes in two CLK32 periods */
+ policy-cpuinfo.transition_latency = trans_latency;
I guess this comment is a cut'n'paste error.
+
+ ret = cpufreq_frequency_table_cpuinfo(policy, freq_table);
+
+ if (ret 0) {
+ pr_err(%s: invalid frequency table for cpu %d\n,
+__func__, policy-cpu);
You should define pr_fmt to always include __func__ in the messages
rather than open coding - ensures consistency and is less noisy in the
code.
+ pr_info(Generic CPU frequency driver\n);
This seems noisy...
___
linaro-dev mailing list
linaro-dev@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH V3 4/7] cpufreq: add generic cpufreq driver
On Wed, Dec 21, 2011 at 07:27:03AM +0800, Richard Zhao wrote: On Tue, Dec 20, 2011 at 02:59:04PM +, Mark Brown wrote: My comments on the previous version of the patch still apply: - The voltage ranges being set need to be specified as ranges. cpu normally need strict voltages. and only proved operating opints are allowed to set in dts. If the voltage changes slightly especially for high frequency, it's easy to cause unstable. Clearly there will be limits which get more and more restrictive as the frequencies get higher but there will always be at least some play in the numbers as one must at a minimum specify tolerance ranges, and at lower frequencies the ranges specified will typically get compartively large. Note also that not all hardware specifies things in terms of a fixed set of operating points, sometimes only the minimum voltage specification is varied with frequency or sometimes you see maximum and minimum stepping independently. Further note that if all hardware really does have as tight a set of requirements as you suggest then the regulator support in the driver needs to be non-optional otherwise a board without software regulator control might drop the frequency without also dropping the voltage. - Frequencies that can't be supported due to limitations of the available supplies shouldn't be exposed to users. As I said, only proved operation points are allowed. This statement appears to be unrelated to the comment you're replying to. You also need to define how the core supplies get looked up. It's pure software. platform uses this driver have to define cpu consumer. You still need to define this in the binding. + pr_info(Generic CPU frequency driver\n); This seems noisy... Why? Do you think only errors and warnings can print out? Yes. ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH V3 4/7] cpufreq: add generic cpufreq driver
On Wed, Dec 21, 2011 at 09:20:46AM +0800, Richard Zhao wrote: On Tue, Dec 20, 2011 at 11:48:45PM +, Mark Brown wrote: Note also that not all hardware specifies things in terms of a fixed set of operating points, sometimes only the minimum voltage specification is varied with frequency or sometimes you see maximum and minimum stepping independently. cpus that don't use freq table is out of scope of this driver. That's not the point - the point is that you may do something like specify a defined set of frequencies and just drop the minimum supported voltage without altering the maximum supported voltage as the frequency gets lower. Further note that if all hardware really does have as tight a set of requirements as you suggest then the regulator support in the driver needs to be non-optional otherwise a board without software regulator control might drop the frequency without also dropping the voltage. It's ok to only adjuct freq without changes voltage. You can find many arm soc cpufreq drivers don't change voltage. If dts specify voltage but don't have such regulator. I'll assume it always runs on the initial volatage (highest for most cases). My point exactly; such devices are examples of the policy outlined above where only the minimum voltage changes with frequency and the maximum voltage is constant. The cpufreq driver would lower the supported voltage when possible but wouldn't actually care if the voltage changes. - Frequencies that can't be supported due to limitations of the available supplies shouldn't be exposed to users. As I said, only proved operation points are allowed. This statement appears to be unrelated to the comment you're replying to. I meant the exact voltage can always successfull set. Anyway, I'll add This is just not the case. Regulators don't offer a continuous range of voltages, they offer steps of varying sizes which may miss setting some voltages, and board designers may choose not to support some of the voltage range. regulator_set_voltage return value checking. While this is important the driver should also be enumerating the supported voltages at probe time and eliminating unsupportable settings so that governors aren't constantly trying to set things that can never be supported. The s3c64xx cpufreq driver provides an implementation of this. Maybe I can remove it. But I'd probably add freq table dump. It's more important. Agree? That seems like something the core should be doing if ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH V3 4/7] cpufreq: add generic cpufreq driver
On Wed, Dec 21, 2011 at 10:24:53AM +0800, Richard Zhao wrote: On Wed, Dec 21, 2011 at 01:32:21AM +, Mark Brown wrote: That's not the point - the point is that you may do something like specify a defined set of frequencies and just drop the minimum supported voltage without altering the maximum supported voltage as the frequency gets lower. What do you mean by drop? Drop is a synonym for lower in this context. cpu-volts = /* min max */ 110 120 /* 1G HZ */ 100 120 /* 800M HZ */ 90 120 /* 600M HZ */ ; The above sample dts could meet your point? Yes. While this is important the driver should also be enumerating the supported voltages at probe time and eliminating unsupportable settings so that governors aren't constantly trying to set things that can never be supported. The s3c64xx cpufreq driver provides an implementation of this. I'll use regulator_is_supported_voltage pre-check the cpu-volts. For clock, conditions ((clk_round_rate(cpu-freq)/1000) * 1000 == cpu-freq) seems too strict. Because cpu clock is SoC dependent, I'll not add the check. The issue that was there for is that there are multiple runtime detectable variant clocking configurations which couldn't be switched between so the driver needs to select only the rates that can be reached from the current configuration. I'd imagine that might be an issue elsewhere. ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH v2 1/3] regulator: twl: adapt twl-regulator driver to dt
On Tue, Dec 13, 2011 at 03:49:33PM +0530, Rajendra Nayak wrote: I'm OK with this but would prefer that OMAP or TWL people were OK with it too. If you do need to respin: +For twl4030 regulators/LDO's ' should *not* be used for plurals except when omitting a duplicated s introduced by one (grammar nit but it really bugs me). ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [Patch] Regulator: Replace kzalloc with devm_kzalloc and if-else with a switch-case for da9052-regulator
On Thu, Dec 15, 2011 at 06:59:53PM +0530, Ashish Jangam wrote: Reported-by: Mark Brown broo...@opensource.wolfsonmicro.com Signed-off-by: David Dajun Chen dc...@diasemi.com Signed-off-by: Ashish Jangam ashish.jan...@kpitcummins.com Applied, but this really should have been sent as two separate patches as it's two unrelated changes which don't overlap with each other. ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: linux-next not booting on snowball
On Wed, Dec 14, 2011 at 09:24:33AM +0100, Linus Walleij wrote: The above remaps and reads from some random ROM page to get the ASIC ID is actually not screwing things up. Right now. The ASIC ID reads are also done by Samsung platforms which boot fine - it's not strictly good but it happens to work (and has done for some considerable time). ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH 01/06] MFD: DA9052/53 MFD core module v10
On Mon, Dec 12, 2011 at 08:06:56PM +0530, Ashish Jangam wrote: The DA9052/53 is a highly integrated PMIC subsystem with supply domain flexibility to support wide range of high performance application. Applied, thanks. ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH 03/06] MFD: DA9052/53 MFD core module add SPI support v2
On Mon, Dec 12, 2011 at 08:37:41PM +0530, Ashish Jangam wrote: This patch add SPI support for DA9052/53 MFD core module. Applied, thanks. ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [Patch 06/06] Regulator: DA9052/53 Regulator support v5
On Fri, Dec 09, 2011 at 07:48:20PM +0530, Ashish Jangam wrote:
The Dialog PMIC has below featured regulators:-
DA9052-BC - 4 DVS Buck converters 0.5V - 3.6V upto 1Amp.
DA9053-AA/BX - 4 DVS Buck converters 0.5V - 2.5V upto 3Amp.
DA9052/53 - 10 Programmable LDO's High PSSR, 1% accuracy.
Applied but there are some small issues - please send incremental
patches fixing these.
+ if (chip_id == DA9052) {
+ for (i = 0; i ARRAY_SIZE(da9052_regulator_info); i++) {
+ info = da9052_regulator_info[i];
+ if (info-reg_desc.id == id)
+ return info;
+ }
+ } else {
+ for (i = 0; i ARRAY_SIZE(da9053_regulator_info); i++) {
+ info = da9053_regulator_info[i];
+ if (info-reg_desc.id == id)
+ return info;
+ }
+ }
This would be better written as a switch statement.
+ regulator = kzalloc(sizeof(struct da9052_regulator), GFP_KERNEL);
+ if (!regulator)
+ return -ENOMEM;
You should use devm_kzalloc().
___
linaro-dev mailing list
linaro-dev@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH 01/06] MFD: DA9052/53 MFD core module v10
On Mon, Dec 12, 2011 at 08:06:56PM +0530, Ashish Jangam wrote: The DA9052/53 is a highly integrated PMIC subsystem with supply domain flexibility to support wide range of high performance application. It provides voltage regulators, GPIO controller, Touch Screen, RTC, Battery control and other functionality. This patch is functionally tested on Samsung SMDKV6410. Reviewed-by: Mark Brown broo...@opensource.wolfsonmicro.com Looking good now! Samuel, this uses regmap-irq so either I can carry this or you can merge: git://git.kernel.org/pub/scm/linux/kernel/git/broonie/regmap.git topic/irq into your tree to get the changes that the driver depends on. ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH 01/06] MFD: DA9052/53 MFD core module v9
On Fri, Dec 09, 2011 at 07:45:44PM +0530, Ashish Jangam wrote:
The DA9052/53 is a highly integrated PMIC subsystem with supply domain
flexibility to support wide range of high performance application.
This looks pretty good now. There's a few issues below but with those
corrected the driver looks good.
Reviewed-by: Mark Brown broo...@opensource.wolfsonmicro.com
+int da9052_add_regulator_devices(struct da9052 *da9052)
+{
+ struct platform_device *pdev;
+ int i, ret;
+
+ for (i = 0; i DA9052_MAX_REGULATORS; i++) {
+ pdev = platform_device_alloc(da9052-regulator, i);
+ if (!pdev)
+ return -ENOMEM;
+
+ pdev-dev.parent = da9052-dev;
+ ret = platform_device_add(pdev);
+ if (ret) {
+ platform_device_put(pdev);
+ return ret;
+ }
+ }
Same comment as ever about these - just make them normal MFD children.
+ if (!pdata !pdata-irq_base)
+ da9052-irq_base = -1;
The above should be || not - you want to set irq_base to -1 if you
don't have one provided.
+static inline int da9052_reg_update(struct da9052 *da9052, unsigned char reg,
+ unsigned char bit_mask,
+ unsigned char reg_val)
+{
+ if (reg DA9052_MAX_REG_CNT) {
+ dev_err(da9052-dev, invalid reg %x\n, reg);
+ return -EINVAL;
+ }
regmap will do this for you if you tell it max_register, this will also
get you the ability to inspect the device registers via debugfs which
can be very helpful. I'd also encourage you to enable cache support, it
should make system performance better.
___
linaro-dev mailing list
linaro-dev@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH 01/06] MFD: DA9052/53 MFD core module v9
On Fri, Dec 09, 2011 at 07:45:44PM +0530, Ashish Jangam wrote: The DA9052/53 is a highly integrated PMIC subsystem with supply domain flexibility to support wide range of high performance application. Oh, actually I see you didn't Samuel, you must always CC maintainers on patches otherwise they're likely to not see and apply them. Samuel, as this uses a newly introduced regmap feature do you want me to apply these patches? I'm also happy for you to merge git://git.kernel.org/pub/scm/linux/kernel/git/broonie/regmap.git topic/irq into your tree (though obviously that'd prevent you rebasing). ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH 02/06] MFD: DA9052/53 MFD core module v9 Added ADC support
On Fri, Dec 09, 2011 at 07:46:06PM +0530, Ashish Jangam wrote: + req = kzalloc(sizeof(*req), GFP_KERNEL); + if (!req) + return -ENOMEM; + init_completion(req-done); + req-input = channel; + + if (channel DA9052_ADC_VBBAT) + return -EINVAL; This will leak the request. + list_del(req-list); +err: + mutex_unlock(da9052-auxadc_lock); + return ret; +} +EXPORT_SYMBOL_GPL(da9052_adc_manual_read); In fact is req freed at all? ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH 03/06] MFD: DA9052/53 MFD core module v9 Added SPI support
On Fri, Dec 09, 2011 at 07:46:33PM +0530, Ashish Jangam wrote: The DA9052/53 is a highly integrated PMIC subsystem with supply domain flexibility to support wide range of high performance application. Reviwed-by: Mark Brown broo...@opensource.wolfsonmicro.com Looks good, though again you should've CCed Samuel. One small thing: struct device *dev; + struct spi_device *spi_dev; struct i2c_client *i2c_client; You con't actually use this (or the i2c_client) any more so could just drop them. ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH] Regulator: Add Anatop regulator driver
On Wed, Dec 07, 2011 at 09:53:18PM +0800, Ying-Chun Liu (PaulLiu) wrote: From: Ying-Chun Liu (PaulLiu) paul@linaro.org Anatop regulator driver is used by i.MX6 SoC. This patch adds the Anatop regulator driver. This changelog isn't terribly verbose but looking at the code what you've actually got here is a driver that is simply an indirection layer for the regulator API and no explanation as to why you're doing this. My first thought is that anything using this driver should just be a regulator driver directly. ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH v5 1/4] regulator: helper routine to extract regulator_init_data
On Mon, Dec 05, 2011 at 02:40:50PM +0530, Thomas Abraham wrote: On 4 December 2011 21:24, Mark Brown If the regulator isn't software managed then always_on covers this - the regulator core will enable any always_on regulators that haven't been enabled already. Thanks for the hint. I was trying to deal with a regulator that was not software managed but also required the voltage level to be set to certain level. That was possible with 'apply_uV' constraint in non-dt case. Anyway, I have modified the code to manage the regulator and this works fine in dt case as well without the 'apply_uV' constraint. With the regulator device tree bindings if the regulator is configured to run a single voltage the bindings will set apply_uV unconditionally so there's no need for a separate constraint. ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH v5 1/4] regulator: helper routine to extract regulator_init_data
On Mon, Dec 05, 2011 at 04:14:40PM +0530, Thomas Abraham wrote: On 5 December 2011 16:04, Mark Brown With the regulator device tree bindings if the regulator is configured to run a single voltage the bindings will set apply_uV unconditionally so there's no need for a separate constraint. Sorry if I have missed this, but I could not find 'apply_uV' being set as you described in the v5 of the regulator-dt patchset. Yes, looks like Rajendra missed that - just fixed it. ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH 01/11] MFD: DA9052/53 MFD core module v8
On Mon, Dec 05, 2011 at 12:16:24PM +0530, ashishj3 wrote:
--- a/drivers/base/regmap/regmap-irq.c
+++ b/drivers/base/regmap/regmap-irq.c
@@ -164,7 +164,6 @@ static irqreturn_t regmap_irq_thread(int irq, void *d)
* irq: The IRQ the device uses to signal interrupts
* irq_flags: The IRQF_ flags to use for the primary interrupt.
* chip: Configuration for the interrupt controller.
- * data: Runtime data structure for the controller, allocated on success
Uh, that's not good...
- irq_base = irq_alloc_descs(irq_base, 0, chip-num_irqs, 0);
- if (irq_base 0) {
+ *irq_base = irq_alloc_descs(*irq_base, 0, chip-num_irqs, 0);
+ if (*irq_base 0) {
This isn't adding an accessory, this is passing irq_base by reference.
Seems like a lot of pointers, really. I'll send a patch.
___
linaro-dev mailing list
linaro-dev@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-dev
