RE: [PATCH v4 0/2] ASoC: da7219: Convert driver to use generic FW functions
On 21 June 2016 23:30, Rafael J. Wysocki wrote: > > This patch set converts the da7219 codec driver to use device/fwnode > > functions > > to access properties, instead of the DT only of_* functions, allowing ACPI > > to be used as well. > > > > The DT bindings for da7219 have a device node for the main codec properties, > > and a named child node (da7219_aad), which contains all of the accessory > > detection related properties for the device. ACPI also supports this kind of > > FW hierarchy (data only sub-nodes), but some support in the kernel needs to > > be > > added to take make use of this in driver code. > > > > The first patch adds functions to allow searching for a named child node of > > a > > device, for both DT and ACPI, and the second patch updates the codec driver > > to > > use the standard device/fwnode calls, including this new function. > > > > These changes are based on the v4.7-rc4 kernel. > > > > Changes in v4: > > - Rebase to v4.7-rc4 > > - Use strcmp() in acpi_data_node_match() for matching ACPI data nodes. > > > > Changes in v3: > > - Use of_node_cmp() in device_get_named_child_node() to match DT node. > > > > Changes in v2: > > - Rebase to v4.7-rc1 > > - Small updates to codec patch based on previous reviewer comments > > > > Adam Thomson (2): > >device property: Add function to search for named child of device > >ASoC: da7219: Convert driver to use generic device/fwnode functions > > > > drivers/base/property.c | 28 > > include/acpi/acpi_bus.h | 7 +++ > > include/linux/acpi.h | 6 +++ > > include/linux/of.h| 14 +++--- > > include/linux/property.h | 3 ++ > > sound/soc/codecs/da7219-aad.c | 103 > > + > - > > sound/soc/codecs/da7219.c | 34 +++--- > > 7 files changed, 119 insertions(+), 76 deletions(-) > > > > -- > > 1.9.3 > > > I'm fine with the first patch and the second one carries a couple of > ACKs already, so do you want me to apply them both? Thanks Rafael. I think we still need Mark's Ack for the ASoC codec changes though before we can proceed.
RE: [RESEND PATCH v3 1/2] device property: Add function to search for named child of device
21 June 2016 12:42, Rafael J. Wysocki wrote:
> > > +static inline bool acpi_data_node_match(struct fwnode_handle *fwnode,
> > > + const char *name)
> > > +{
> > > + return is_acpi_data_node(fwnode) ?
> > > + (!strcasecmp(to_acpi_data_node(fwnode)->name, name)) : false;
> > > +}
> >
> > Looks fine to me.
> >
> > One question - is it expected that matching ACPI data nodes is always
> > case insensitive?
>
> That would not be a correct expectation in theory, although I don't think it
> really matters in practice.
From my reading of the Hierarchical Data Extension and ACPI Spec, I thought
that was the case (section 19.3.1 ASL Names - ASL names are not case-sensitive
and will be converted to upper case). Am I misreading the documents/missing
something else?
RE: [RESEND PATCH v3 1/2] device property: Add function to search for named child of device
On 20 June 2016 12:39, Adam Thomson wrote:
> For device nodes in both DT and ACPI, it possible to have named
> child nodes which contain properties (an existing example being
> gpio-leds). This adds a function to find a named child node for
> a device which can be used by drivers for property retrieval.
>
> For DT data node name matching, of_node_cmp() and similar functions
> are made available outside of CONFIG_OF block so the new function
> can reference these for DT and non-DT builds.
>
> For ACPI data node name matching, a helper function is also added
> which returns false if CONFIG_ACPI is not set, otherwise it
> performs a string comparison on the data node name. This avoids
> using the acpi_data_node struct for non CONFIG_ACPI builds,
> which would otherwise cause a build failure.
>
> Signed-off-by: Adam Thomson
> Tested-by: Sathyanarayana Nujella
> Acked-by: Rob Herring
> ---
>
> Changes in v3:
> - Move of_*_cmp() functions in of.h outside of CONFIG_OF block so they are
>available for non-DT builds
> - In device_get_named_child_node(), use of_node_cmp() helper macro instead of
>strcasecmp() (node names not alway case insensitive, depending on
> platform).
>
> Changes in v2:
> - Rebase to v4.7-rc1
>
> drivers/base/property.c | 28
> include/acpi/acpi_bus.h | 7 +++
> include/linux/acpi.h | 6 ++
> include/linux/of.h | 14 +++---
> include/linux/property.h | 3 +++
> 5 files changed, 51 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/base/property.c b/drivers/base/property.c
> index f38c21d..43a36d6 100644
> --- a/drivers/base/property.c
> +++ b/drivers/base/property.c
> @@ -888,6 +888,34 @@ struct fwnode_handle *device_get_next_child_node(struct
> device *dev,
> EXPORT_SYMBOL_GPL(device_get_next_child_node);
>
> /**
> + * device_get_named_child_node - Return first matching named child node
> handle
> + * @dev: Device to find the named child node for.
> + * @childname: String to match child node name against.
> + */
> +struct fwnode_handle *device_get_named_child_node(struct device *dev,
> + const char *childname)
> +{
> + struct fwnode_handle *child;
> +
> + /*
> + * Find first matching named child node of this device.
> + * For ACPI this will be a data only sub-node.
> + */
> + device_for_each_child_node(dev, child) {
> + if (is_of_node(child)) {
> + if (!of_node_cmp(to_of_node(child)->name, childname))
> + return child;
> + } else if (is_acpi_data_node(child)) {
> + if (acpi_data_node_match(child, childname))
> + return child;
> + }
> + }
> +
> + return NULL;
> +}
> +EXPORT_SYMBOL_GPL(device_get_named_child_node);
> +
> +/**
> * fwnode_handle_put - Drop reference to a device node
> * @fwnode: Pointer to the device node to drop the reference to.
> *
> diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h
> index 788c6c3..993bdd0 100644
> --- a/include/acpi/acpi_bus.h
> +++ b/include/acpi/acpi_bus.h
> @@ -420,6 +420,13 @@ static inline struct acpi_data_node
> *to_acpi_data_node(struct fwnode_handle *fwn
> container_of(fwnode, struct acpi_data_node, fwnode) : NULL;
> }
>
> +static inline bool acpi_data_node_match(struct fwnode_handle *fwnode,
> + const char *name)
> +{
> + return is_acpi_data_node(fwnode) ?
> + (!strcasecmp(to_acpi_data_node(fwnode)->name, name)) : false;
> +}
> +
> static inline struct fwnode_handle *acpi_fwnode_handle(struct acpi_device
> *adev)
> {
> return &adev->fwnode;
> diff --git a/include/linux/acpi.h b/include/linux/acpi.h
> index 288fac5..03039c4 100644
> --- a/include/linux/acpi.h
> +++ b/include/linux/acpi.h
> @@ -568,6 +568,12 @@ static inline struct acpi_data_node
> *to_acpi_data_node(struct fwnode_handle *fwn
> return NULL;
> }
>
> +static inline bool acpi_data_node_match(struct fwnode_handle *fwnode,
> + const char *name)
> +{
> + return false;
> +}
> +
> static inline struct fwnode_handle *acpi_fwnode_handle(struct acpi_device
> *adev)
> {
> return NULL;
> diff --git a/include/linux/of.h b/include/linux/of.h
> index 74eb28c..310e32f 100644
> --- a/include/linux/of.h
> +++ b/include/linux/of.h
> @@ -238,13 +238,6 @@ static inline unsigned long of_read_ulong(const __be32
> *cell, int size)
> #define OF_ROOT_NODE_SIZE_CELLS_DEFAULT 1
> #endif
>
> -/* Default string compare functions, Allow arch asm/prom.h to override */
> -#if !defined(of_compat_cmp)
> -#define of_compat_cmp(s1, s2, l) strcasecmp((s1), (s2))
> -#define of_prop_cmp(s1, s2) strcmp((s1), (s2))
> -#define of_node_cmp(s1, s2) strcasecmp((s1), (s2))
> -#endif
> -
> #define OF_IS_DYNAMIC(x) test_bit(OF_DYNAMIC, &x->_flags)
> #define
RE: [PATCH v5] ASoC: rockchip: Add machine driver for RK3399 GRU Boards
On 16 June 2016 02:15, Xing Zheng wrote: > >> + /* Enable Headset and 4 Buttons Jack detection */ > >> + ret = snd_soc_card_jack_new(rtd->card, "Headset Jack", > >> + SND_JACK_HEADSET | > > Should this also include SND_JACK_LINEOUT? da7219 differentiates > > between the two so not including lineout means, for example, we can > > miss jack insertion/removal events. > > > Hi Adam, > > Could you please confirm Steve says that about SND_JACK_LINEOUT? > > Thanks. Hi Xing Zheng, Yes, as Steve mentioned DA7219 does support SND_JACK_LINEOUT so we should really include this here.
RE: [RESEND PATCH v2 1/2] device property: Add function to search for named child of device
On 13 June 2016 20:33, Frank Rowand wrote: > > DT node names are case insensitive. The of.h header does provide a helper > > macro > > which is equivalent to this, but that macro is part of the '#ifdef > > CONFIG_OF' > > block. If I were to use it then it would cause non-DT builds to fail. I > > opted > > for strcasecmp() directly as I didn't think for just this one scenario it > > made > > sense to reorganise the of.h header with regards to the helper macros. Of > > course > > if there are other opinions on this then am happy to listen. > > DT node names are not always case insensitive. Please us of_node_cmp(). > > -Frank Ok, fair enough. I'll have to move those definitions in the of.h header out of the CONFIG_OF block then.
RE: [RESEND PATCH v2 1/2] device property: Add function to search for named child of device
On 13 June 2016 09:47, Mark Brown wrote: > > That's strange. I'm not a subscriber to that mailing list, but I assume that > > shouldn't matter here? Strangely though the only mailing list these seem to > > have > > made it to is the ALSA one. :( Will see if I can find out why as I've not > > seen this problem before. > > You have a very large number of people on copy, a lot of the lists have > restrictions on the number of recipients. Thanks Mark. Good to know for future mails.
RE: [PATCH 0/3] ASoC: da7219: Add ACPI initialisation support to driver
On 10 June 2016 11:16, Adam Thomson wrote: > This patch set updates the driver to use generic device property & fwnode > related functions to read in either DT and ACPI data for driver > initialisation. > > Changes are based on v4.6-rc6 Linux Kernel. > > Adam Thomson (3): > ASoC: da7219: Convert driver to use generic device/fwnode functions > ASoC: da7219: Add ACPI parsing support > ASoC: da7219: Add initial ACPI id for device > > sound/soc/codecs/da7219-aad.c | 96 +++- > --- > sound/soc/codecs/da7219.c | 35 ++-- > 2 files changed, 91 insertions(+), 40 deletions(-) > > -- > 1.9.3 PLEASE IGNORE THIS PATCH SET MAIL. Apologies, finger trouble from my end :(
RE: [RESEND PATCH v2 1/2] device property: Add function to search for named child of device
On 10 June 2016 00:11, Rafael J. Wysocki wrote:
> For some reason that didn't make it into the linux-acpi list, or at
> least I can't see it there.
That's strange. I'm not a subscriber to that mailing list, but I assume that
shouldn't matter here? Strangely though the only mailing list these seem to have
made it to is the ALSA one. :( Will see if I can find out why as I've not
seen this problem before.
> > + * device_get_named_child_node - Return first matching named child node
> handle
> > + * @dev: Device to find the named child node for.
> > + * @childname: String to match child node name against.
> > + */
> > +struct fwnode_handle *device_get_named_child_node(struct device *dev,
> > + const char *childname)
> > +{
> > + struct fwnode_handle *child;
> > +
> > + /*
> > +* Find first matching named child node of this device.
> > +* For ACPI this will be a data only sub-node.
> > +*/
> > + device_for_each_child_node(dev, child) {
> > + if (is_of_node(child)) {
> > + if (!strcasecmp(to_of_node(child)->name, childname))
>
> Why do you use strcasecmp() here?
DT node names are case insensitive. The of.h header does provide a helper macro
which is equivalent to this, but that macro is part of the '#ifdef CONFIG_OF'
block. If I were to use it then it would cause non-DT builds to fail. I opted
for strcasecmp() directly as I didn't think for just this one scenario it made
sense to reorganise the of.h header with regards to the helper macros. Of course
if there are other opinions on this then am happy to listen.
> > +static inline bool acpi_data_node_match(struct fwnode_handle *fwnode,
> > + const char *name)
> > +{
> > + return is_acpi_data_node(fwnode) ?
> > + (!strcasecmp(to_acpi_data_node(fwnode)->name, name)) : false;
> > +}
>
> Is there any particular reason to introduce this function instead of
> doing the test in device_get_named_child_node() directly?
Again this is a build related design option (I mention it in the patch
description). In a non-DT build there is no access to the acpi_data_node struct
(returned by to_acpi_data_node() call) so if we call this in that scenario then
the build will fail. I could have added some #ifdefs to the
device_get_named_child_node() function directly, but that would have been a bit
messy I think. To me it made more sense to have this helper function which can
be called regardless of build type, and for non-ACPI builds its definition
always returns false.
RE: [PATCH 1/3] ASoC: da7219: Convert driver to use generic device/fwnode functions
On May 06, 2016, 17:06, Mark Brown wrote:
> No, not really - your DT is fairly unusual in how it's done and the lack
> of ACPI helpers is not a good sign on that side. The _DSD things are
> really only supposed to work for simple properties on devices.
It's unusual in that there's a child node ("da7219_aad") of the device node, to
encapsulate AAD specific information. The properties inside though are data only
and simple. Actually, as I read it this is exactly the kind of thing that the
'Hierarchical Data Extension UUID For _DSD' is for.
> > The intention was to just match against DT or ACPI and nothing else, so that
> > didn't feel generic enough to be pushed into the fwnode framework. However
> > I will take another look.
>
> That's currently the entire set of things that fwnode supports so...
I believe there's the FWNODE_PDATA type as well so 3 things, although I assume
that this is to be used longer term instead of the old fashioned platform
data mechanism, for built-in properties. Right now though I don't see much
actual usage of this.
RE: [PATCH 2/3] ASoC: da7219: Add ACPI parsing support
On May 06, 2016, 13:39, Mark Brown wrote:
> > @@ -27,7 +28,6 @@
> > #include "da7219.h"
> > #include "da7219-aad.h"
> >
> > -
> > /*
> > * Detection control
> > */
>
> Random whitespace change.
Fair point. Will sort it.
>
> > static struct fwnode_handle *da7219_aad_of_named_fwhandle(struct device
> *dev,
> > const char *name)
> > {
> > @@ -551,6 +571,9 @@ static struct fwnode_handle
> *da7219_aad_of_named_fwhandle(struct device *dev,
> > of_node = to_of_node(child);
> > if (of_node_cmp(of_node->name, name) == 0)
> > return child;
> > + } else if (is_acpi_data_node(child)) {
> > + if (da7219_aad_of_acpi_node_matched(child, name))
> > + return child;
> > }
> > }
> >
>
> This seems messy. It is a function with a DT specific name that's
> matching ACPI stuff and the fwnode API isn't hiding anything for us
> which suggests this isn't something that's expected to work
> transparently. At least the naming needs to be corrected, and if this
> *is* supposed to be something we do in ACPI I'd expect the handling to
> be pushed into the fwnode API rather than open coded in a driver - at
> the minute I'm unsure if this is messy because it's a bad idea to do
> this at all or if it's just the naming and so on.
ACPI allows for data only child nodes, like DT, so I do believe this is a valid
case. Also, this kind of functionality is already used in a similar-ish manner
in
the leds-gpio code. I agree that maybe the name should be changed though as it's
misleading. Will also look at the fwnode API and see if it makes sense to move
this there.
>
> > - /* Handle any DT/platform data */
> > - if ((codec->dev->of_node) && (da7219->pdata))
> > + /* Handle any DT/ACPI/platform data */
> > + if (((codec->dev->of_node) || is_acpi_node(codec->dev->fwnode)) &&
> > + (da7219->pdata))
> > da7219->pdata->aad_pdata = da7219_aad_of_to_pdata(codec);
> >
> > da7219_aad_handle_pdata(codec);
>
> Surely we should be able to check if there's firmware data without
> enumerating every possible firmware type?
There doesn't seem to be a unified check for this. Also, Given these are the
only two types the driver expects and supports right now, I don't see a problem
being explicit here in the checking.
RE: [PATCH 1/3] ASoC: da7219: Convert driver to use generic device/fwnode functions
On May 06, 2016, 13:27, Mark Brown wrote:
> > This change converts the driver from using the of_* functions to using
> > the device_* and fwnode_* functions for accssing DT related data.
> > This is in preparation for updates to support ACPI based initialisation.
>
> Is this *really* sensible? DT idioms don't always match up with ACPI
> idioms well and this isn't a trivial DT binding.
For what we're doing here, both DT and ACPI match up well, and so what I've
implemented works on both sides. There are other examples of this already in the
Linux kernel, so I don't think it's anything particularly new.
>
> > +static struct fwnode_handle *da7219_aad_of_named_fwhandle(struct device
> *dev,
> > + const char *name)
> > +{
> > + struct fwnode_handle *child;
> > + struct device_node *of_node;
> > +
> > + /* Find first matching child node */
> > + device_for_each_child_node(dev, child) {
> > + if (is_of_node(child)) {
> > + of_node = to_of_node(child);
> > + if (of_node_cmp(of_node->name, name) == 0)
> > + return child;
> > + }
> > + }
> > +
> > + return NULL;
> > +}
>
> There's nothing device specific about this, it should go in generic
> code.
The intention was to just match against DT or ACPI and nothing else, so that
didn't feel generic enough to be pushed into the fwnode framework. However
I will take another look.
RE: linux-next: build failure after merge of the sound-asoc tree
On May 06, 2016, 13:03, Mark Brown wrote: > > This patch was the 3rd in of a set of 3, and the header you added below was > > added as part of the 2nd patch of that set, hence why it is missing here. I > > assume the first 2 patches are still being reviewed by Mark. > > Yes. The ID patch should have been the first one here, put the simple > stuff first. In my mind it makes sense to only add this after support to use it is in place in the driver, hence the ordering.
RE: linux-next: build failure after merge of the sound-asoc tree
On May 06, 2016, 01:58, Stephen Rothwell wrote:
> Hi all,
>
> After merging the sound-asoc tree, today's linux-next build (x86_64
> allmodconfig) failed like this:
>
> sound/soc/codecs/da7219.c:1964:23: error: implicit declaration of function
> 'ACPI_PTR' [-Werror=implicit-function-declaration]
>.acpi_match_table = ACPI_PTR(da7219_acpi_match),
>^
> sound/soc/codecs/da7219.c:1964:23: error: initializer element is not constant
> sound/soc/codecs/da7219.c:1964:23: note: (near initialization for
> 'da7219_i2c_driver.driver.acpi_match_table')
>
> Caused by commit
>
> 5181365f5312 ("ASoC: da7219: Add initial ACPI id for device")
>
> I added this patch for today:
This patch was the 3rd in of a set of 3, and the header you added below was
added as part of the 2nd patch of that set, hence why it is missing here. I
assume the first 2 patches are still being reviewed by Mark.
RE: [PATCH] mfd: Allow i2c modular drivers to build with I2C=m
On January 21, 2016 00:50, Axel Lin wrote: > These drivers can be built as module, so make them depend on I2C rather > than I2C=y. > > Signed-off-by: Axel Lin For Dialog devices: Acked-by: Adam Thomson
RE: [PATCH 3/6] ASoC: da7219: Update REFERENCES reg default, in-line with HW
On December 23, 2015 00:10, Mark Brown wrote: > On Tue, Dec 22, 2015 at 06:27:53PM +, Adam Thomson wrote: > > In current AB silicon, BIAS_EN field is enabled by default in the > > REFERENCES register, so the regmap default value should reflect > > this. > > This is the sort of thing where a register patch would normally be used > - if you put in a register patch for the older silicon then the driver > can correct for the register default automatically. Thanks. Yes, am aware of that. Were the older silicon still in use then I'd have taken that route, but didn't seem necessary in this instance.
RE: [PATCH] ASoC: da7218: Enable mic level detection reporting to user-space
On December 03, 2015 11:37, Takashi Iwai wrote: > > > > This patch adds support to the codec driver to handle mic level > > > > detect related IRQs, and report these to user-space using a uevent > > > > variable. > > > > > > Is the uevent the best way for this? > > > > > > > > > thanks, > > > > > > Takashi > > > > Well originally I was using an input device mechanism to report to > > user-space, > > albeit using KEY_VOICECOMMAND, which Mark mentioned wasn't correct for this > > scenario, which was fair enough. He also mentioned that there had been a > > general > > push back on using input devices so that code was dropped. See thread below: > > > > https://lkml.org/lkml/2015/11/10/347 > > > > uevent seemed like the simplest solution to report this event, and allow > > user-space to act based on it. As mentioned before though, I am open to > > suggestions if there's a better way. > > I'm not strongly opposing to uevent as long as it's well considered. > But then please describe how it's notified properly and why it was > chosen. The patch changelog has enough free room to write more > details, fortunately. Tell more your story for Christmas :) > > In general, you can use an ALSA control notification for this kind of > event. But it also depends on the user-space side. With alsa-lib, > it's pretty easy to use, while with tinyalsa that lacks of the notifier > handling, you'd need to implement more code there. > > For a plug/unplug type event, we have a generic jack layer, but this > doesn't seem fitting perfectly, either. For ALSA control notification, this implies there's a control associated, but for this scenario that's really not necessary. You just want to know when the noise level captured at the mic has gone above a certain threshold and then act accordingly (process audio capture stream). There's no real need I can see for a control to read from. As for the jack layer, I agree that it doesn't really fit. Ok, will update the patch commit message to add more details on this, including references to Santa Claus, reindeer with shining noses, and other festive symbols. :)
RE: [PATCH] ASoC: da7218: Enable mic level detection reporting to user-space
On December 03, 2015 10:56, Takashi Iwai wrote:
> > This patch adds support to the codec driver to handle mic level
> > detect related IRQs, and report these to user-space using a uevent
> > variable.
>
> Is the uevent the best way for this?
>
>
> thanks,
>
> Takashi
Well originally I was using an input device mechanism to report to user-space,
albeit using KEY_VOICECOMMAND, which Mark mentioned wasn't correct for this
scenario, which was fair enough. He also mentioned that there had been a general
push back on using input devices so that code was dropped. See thread below:
https://lkml.org/lkml/2015/11/10/347
uevent seemed like the simplest solution to report this event, and allow
user-space to act based on it. As mentioned before though, I am open to
suggestions if there's a better way.
> >
> > Signed-off-by: Adam Thomson
> > ---
> > sound/soc/codecs/da7218.c | 19 ++-
> > 1 file changed, 14 insertions(+), 5 deletions(-)
> >
> > diff --git a/sound/soc/codecs/da7218.c b/sound/soc/codecs/da7218.c
> > index 4fee7ae..752ed04 100644
> > --- a/sound/soc/codecs/da7218.c
> > +++ b/sound/soc/codecs/da7218.c
> > @@ -2202,6 +2202,16 @@ int da7218_hpldet(struct snd_soc_codec *codec,
> struct snd_soc_jack *jack)
> > }
> > EXPORT_SYMBOL_GPL(da7218_hpldet);
> >
> > +static void da7218_micldet_irq(struct snd_soc_codec *codec)
> > +{
> > + char *envp[] = {
> > + "EVENT=MIC_LEVEL_DETECT",
> > + NULL,
> > + };
> > +
> > + kobject_uevent_env(&codec->dev->kobj, KOBJ_CHANGE, envp);
> > +}
> > +
> > static void da7218_hpldet_irq(struct snd_soc_codec *codec)
> > {
> > struct da7218_priv *da7218 = snd_soc_codec_get_drvdata(codec);
> > @@ -2232,6 +2242,10 @@ static irqreturn_t da7218_irq_thread(int irq, void
> *data)
> > if (!status)
> > return IRQ_NONE;
> >
> > + /* Mic level detect */
> > + if (status & DA7218_LVL_DET_EVENT_MASK)
> > + da7218_micldet_irq(codec);
> > +
> > /* HP detect */
> > if (status & DA7218_HPLDET_JACK_EVENT_MASK)
> > da7218_hpldet_irq(codec);
> > @@ -2936,11 +2950,6 @@ static int da7218_probe(struct snd_soc_codec *codec)
> > }
> >
> > if (da7218->irq) {
> > - /* Mask off mic level events, currently not handled */
> > - snd_soc_update_bits(codec, DA7218_EVENT_MASK,
> > - DA7218_LVL_DET_EVENT_MSK_MASK,
> > - DA7218_LVL_DET_EVENT_MSK_MASK);
> > -
> > ret = devm_request_threaded_irq(codec->dev, da7218->irq, NULL,
> > da7218_irq_thread,
> > IRQF_TRIGGER_LOW |
> IRQF_ONESHOT,
> > --
> > 1.9.3
> >
> >
RE: [PATCH v3 1/2] ASoC: da7218: Add bindings documentation for DA7218 audio codec
On November 20, 2015 15:54, Rob Herring wrote: > > +- dlg,micbias1-lvl-millivolt : Voltage (mV) for Mic Bias 1 > > + [<1200>, <1600>, <1800>, <2000>, <2200>, <2400>, <2600>, <2800>, > <3000>] > > +- dlg,micbias2-lvl-millivolt : Voltage (mV) for Mic Bias 2 > > + [<1200>, <1600>, <1800>, <2000>, <2200>, <2400>, <2600>, <2800>, > <3000>] > > +- dlg,mic1-amp-in-sel : Mic1 input source type > > + ["diff", "se_p", "se_n"] > > +- dlg,mic2-amp-in-sel : Mic2 input source type > > + ["diff", "se_p", "se_n"] > > +- dlg,dmic1-data-sel : DMIC1 channel select based on clock edge. > > + ["lrise_rfall", "lfall_rrise"] > > +- dlg,dmic1-samplephase : When to sample audio from DMIC1. > > + ["on_clkedge", "between_clkedge"] > > +- dlg,dmic1-clkrate-hertz : DMic1 clock frequency (Hz). > > -hz > > Documenting the unit suffixes is on my todo list... So is the general rule to use abbreviated suffixes, apart from millivolts, microvolts, etc, or are there some other rules that I should be aware of? I don't want to have to keep re-submitting patches in the future because I'm not following a rule I have no way of knowing about? > > + [<150>, <300>] > > +- dlg,dmic2-data-sel : DMic2 channel select based on clock edge. > > + ["lrise_rfall", "lfall_rrise"] > > +- dlg,dmic2-samplephase : When to sample audio from DMic2. > > + ["on_clkedge", "between_clkedge"] > > +- dlg,dmic2-clkrate-hertz : DMic2 clock frequency (Hz). > > -hz > > > + [<150>, <300>] > > +- dlg,hp-diff-single-supply : Boolean flag, use single supply for HP > > + (DA7217 only) > > + > > +== > > + > > +Optional Child node - 'da7218_hpldet' (DA7218 only): > > + > > +Optional properties: > > +- dlg,jack-rate-microsecond : Time between jack detect measurements (us) > > -us > > Completely consistent, isn't it. Why we did -microvolt I don't know. I would guess this is due to the all lower-case rule, as 'uv' doesn't look right when indicating microvolts. Maybe the lower-case rule should not apply for abbreviations like this so we can use 'mV', 'uV', etc...? In theory then it could be 'Hz' as well, if we're being absolutely correct.
RE: [PATCH v2 1/2] ASoC: da7218: Add bindings documentation for DA7218 audio codec
On November 17, 2015 17:54, Rob Herring wrote:
> >> > +- dlg,micbias1-lvl : Voltage (mV) for Mic Bias 1
> >> > + [<1200>, <1600>, <1800>, <2000>, <2200>, <2400>, <2600>, <2800>,
> >> <3000>]
> >> > +- dlg,micbias2-lvl : Voltage (mV) for Mic Bias 2
> >> > + [<1200>, <1600>, <1800>, <2000>, <2200>, <2400>, <2600>, <2800>,
> >> <3000>]
> >>
> >> Units please (-microvolt).
> >
> > I refer back to our previous discussion
> > (https://lkml.org/lkml/2015/10/8/661).
> > This doesn't add anything and makes the binding name unnecessarily long.
> > Why is
> > this being enforced? Whoever uses the binding will have to look at the
> > documentation to understand which values are valid anyway, so this seems
> > like
> > cruft.
>
> It is simply standard, best practice for new bindings. Certainly there
> are examples that don't follow this, but they are either old or
> escaped review.
>
> Drop the 'lvl' part if you are so concerned about length.
If this is a standard, then maybe this should be explicitly documented and
pushed to all other maintainers so it isn't escaping review? Otherwise you're
left with a disparity which suggests it isn't standard at all. Personally I
stand by that for device specific bindings this shouldn't apply as it gains
nothing except achieving an overly long binding, or you end up cutting down the
descriptive part of the name to accommodate units at the end.
N�r��yb�X��ǧv�^�){.n�+{zX����ܨ}���Ơz�&j:+v���zZ+��+zf���h���~i���z� �w���?�&�)ߢf��^jǫy�m��@A�a���
0��h���i
RE: [PATCH v2 1/2] ASoC: da7218: Add bindings documentation for DA7218 audio codec
On November 11, 2015 20:20, Rob Herring wrote: > > +- dlg,micbias1-lvl : Voltage (mV) for Mic Bias 1 > > + [<1200>, <1600>, <1800>, <2000>, <2200>, <2400>, <2600>, <2800>, > <3000>] > > +- dlg,micbias2-lvl : Voltage (mV) for Mic Bias 2 > > + [<1200>, <1600>, <1800>, <2000>, <2200>, <2400>, <2600>, <2800>, > <3000>] > > Units please (-microvolt). I refer back to our previous discussion (https://lkml.org/lkml/2015/10/8/661). This doesn't add anything and makes the binding name unnecessarily long. Why is this being enforced? Whoever uses the binding will have to look at the documentation to understand which values are valid anyway, so this seems like cruft. > > +Optional properties: > > +- dlg,jack-rate : Time between jack detect measurements (us) > > + [<5>, <10>, <20>, <40>, <80>, <160>, <320>, <640>] > > Units ditto.
RE: [PATCH 2/2] ASoC: codecs: Add da7218 codec driver
On November 10, 2015 15:45, Mark Brown wrote: > > It's to detect the noise level on a mic and raise an event if the captured > > sound is above a specific threshold level. Apologies if that wasn't clear. > > > In the driver code I'm using KEY_VOICECOMMAND, and simulating a press and > > release of this key, to indicate to user-space. This seemed like the obvious > > choice for this feature to me, although I'd happily get your opinion on > > this. > > That seems like a particularly unfortunate choice given that > VOICECOMMAND is used in the standard Google headset mapping (see > ts3a227e for an example, that's a device specifically aimed at providing > accessory detection in Chromebooks). There's also been some pushback > against using the input devices due to the difficulty in enabling apps > to access input devices - ALSA controls were preferred instead but > that's less helpful for tinyalsa. Perhaps that can be added relatively > easily, or a uevent or something. > I chose VOICECOMMAND as I thought this kind of feature might offer the same kind of use as the physical button, but if this only for Google headset use then fair enough. > Not sure what the best way forward here is, the other implementations of > this that I'm aware of do more of the detection in offload and present > streams of detected audio to userspace via normal capture. > Yes, this is far more simplistic, and any voice processing or capture is not handled by the codec. It just an indication of above threshold noise level at the mic. For the implementations you know of, how are those events indicated to user-space? > I would at least suggest moving this into a separate patch and doing > the integration separately. Are you happy for me to leave the actual controls for this feature in, without the user-space reporting side? Otherwise it's a pain to strip that out, and then re-instate later. The event can be masked off until the user-space reporting is added in a subsequent patch.
RE: [PATCH 2/2] ASoC: codecs: Add da7218 codec driver
On November 10, 2015 14:15, Mark Brown wrote: > > > The general userspace expectation is that the detection is always active > > > and consistent rather than varying at runtime - runtime variability > > > might be a bit surprising for it, and even then variability in what is > > > detected based on other settings is a bit surprising. If the hardware > > > is that limited I guess it's about all that can be done but I'm still > > > not clear what the use cases are for configuring the levels (as opposed > > > ot the routing). > > > How about the example of always on voice in Android, which can be enabled > > and > > disabled, depending on user settings, and routing will vary depending on > > which > > mic is in use at the time? For the levelling is it not plausible that a user > > could configure the level based on their current environment. You have > > moderately loud background noise, then your threshold would want to be > > higher, but in a quiet environment the likelihood is you would want to lower > > that threshold? > > So this *isn't* a normal mic detection feature? What's the userspace > interface for reporting then? By mic detection you thought this was to detect if a mic was present or not? It's to detect the noise level on a mic and raise an event if the captured sound is above a specific threshold level. Apologies if that wasn't clear. In the driver code I'm using KEY_VOICECOMMAND, and simulating a press and release of this key, to indicate to user-space. This seemed like the obvious choice for this feature to me, although I'd happily get your opinion on this.
RE: [PATCH 2/2] ASoC: codecs: Add da7218 codec driver
On November 9, 2015 14:02, Mark Brown wrote: > > > What I'm trying to figure out here is if this depends on the audio > > > routing at runtime or if it's got dedicated configuration? > > > This feature is available for any/all mics connected. Which mics are enabled > > is a runtime configuration of routing, so to me it makes sense also that we > > can > > configure which channel triggers an event, based on our scenario at that > > time. > > The general userspace expectation is that the detection is always active > and consistent rather than varying at runtime - runtime variability > might be a bit surprising for it, and even then variability in what is > detected based on other settings is a bit surprising. If the hardware > is that limited I guess it's about all that can be done but I'm still > not clear what the use cases are for configuring the levels (as opposed > ot the routing). How about the example of always on voice in Android, which can be enabled and disabled, depending on user settings, and routing will vary depending on which mic is in use at the time? For the levelling is it not plausible that a user could configure the level based on their current environment. You have moderately loud background noise, then your threshold would want to be higher, but in a quiet environment the likelihood is you would want to lower that threshold?
RE: [PATCH 2/2] ASoC: codecs: Add da7218 codec driver
On November 8, 2015 10:34, Mark Brown wrote:
> > > Hang on, is this just recording a DC value with the ADC and then looking
> > > at that?
>
> > The RMS of the Mic signal is taken and compared to the trigger level set. If
> > it's above that level then an IRQ is raised.
>
> What I'm trying to figure out here is if this depends on the audio
> routing at runtime or if it's got dedicated configuration?
This feature is available for any/all mics connected. Which mics are enabled
is a runtime configuration of routing, so to me it makes sense also that we can
configure which channel triggers an event, based on our scenario at that time.
N�r��yb�X��ǧv�^�){.n�+{zX����ܨ}���Ơz�&j:+v���zZ+��+zf���h���~i���z� �w���?�&�)ߢf��^jǫy�m��@A�a���
0��h���i
RE: [PATCH 2/2] ASoC: codecs: Add da7218 codec driver
On November 6, 2015 11:55, Mark Brown wrote: > > > > I can envisage in a system you may want to choose which capture > > > > channels can > > > > trigger level detection (if any), and this may change depending on the > > > > use-case > > > > at the time, so having it as a control makes sense to me. > > > > What is a "capture channel" here? > > > Input filters 1L/R and 2L/R, which are fed from either Mic1(ADC1) or > > DMic1L/R > > and Mic2(ADC2) or DMic2L/R. > > Hang on, is this just recording a DC value with the ADC and then looking > at that? The RMS of the Mic signal is taken and compared to the trigger level set. If it's above that level then an IRQ is raised.
RE: [PATCH 2/2] ASoC: codecs: Add da7218 codec driver
On November 6, 2015 11:22, Mark Brown wrote:
> > > > +static int da7218_mic_lvl_det_sw_put(struct snd_kcontrol *kcontrol,
> > > > +struct snd_ctl_elem_value
> > > > *ucontrol)
> > > > +{
>
> > > Why is this a user visible control?
>
> > I can envisage in a system you may want to choose which capture channels can
> > trigger level detection (if any), and this may change depending on the
> > use-case
> > at the time, so having it as a control makes sense to me.
>
> What is a "capture channel" here?
Input filters 1L/R and 2L/R, which are fed from either Mic1(ADC1) or DMic1L/R
and Mic2(ADC2) or DMic2L/R.
RE: [PATCH 2/2] ASoC: codecs: Add da7218 codec driver
On November 5, 2015 15:28, Mark Brown wrote:
> > +/* ALC */
> > +static void da7218_alc_calib(struct snd_soc_codec *codec)
> > +{
> > + struct da7218_priv *da7218 = snd_soc_codec_get_drvdata(codec);
> > + u8 calib_ctrl;
> > + int i = 0;
> > + bool calibrated = false;
> > +
> > + /* Bypass cache so it saves current settings */
> > + regcache_cache_bypass(da7218->regmap, true);
>
> What ensures that nothing else is running at the same time this is?
Is a fair point. Originally I was saving the state of registers then
re-instating them at the end, which worked fine, but then was trying to be
clever and tidy things up by bypassing the cache instead. Will revert back to
the previous method.
>
> > +static int da7218_mic_lvl_det_sw_put(struct snd_kcontrol *kcontrol,
> > +struct snd_ctl_elem_value *ucontrol)
> > +{
>
> Why is this a user visible control?
I can envisage in a system you may want to choose which capture channels can
trigger level detection (if any), and this may change depending on the use-case
at the time, so having it as a control makes sense to me.
>
> > + /* Default all mixers off */
> > + snd_soc_write(codec, DA7218_DROUTING_OUTDAI_1L, 0);
> > + snd_soc_write(codec, DA7218_DROUTING_OUTDAI_1R, 0);
> > + snd_soc_write(codec, DA7218_DROUTING_OUTDAI_2L, 0);
> > + snd_soc_write(codec, DA7218_DROUTING_OUTDAI_2R, 0);
> > + snd_soc_write(codec, DA7218_DROUTING_OUTFILT_1L, 0);
> > + snd_soc_write(codec, DA7218_DROUTING_OUTFILT_1R, 0);
> > + snd_soc_write(codec, DA7218_DROUTING_ST_OUTFILT_1L, 0);
> > + snd_soc_write(codec, DA7218_DROUTING_ST_OUTFILT_1R, 0);
>
> We generally just use the device defaults, why change them?
I figured it made more sense to have the device start with audio routes disabled
but I can remove this as it's really not essential.
RE: [PATCH 1/2] ASoC: da7218: Add bindings documentation for DA7218 audio codec
On November 05, 2015 14:59, Mark Brown wrote:
> > +- dlg,ldo-lvl : Required internal LDO voltage (mV) level
> > + [<1050>, <1100>, <1200>, <1400>]
>
> Why would this ever be anything other than the minimum voltage, and
> might we not want to vary it at runtime?
Normally you are correct and you'll want minimum voltage for the digital engine
but there may be the possibility for certain platform scenarios that to meet
timings a higher voltage is required. Would prefer to leave this in in case it
is required in the future. As a side, the default setting is the lowest voltage,
if this binding is not provided.
>
> > +- dlg,biquad-cfg : List of data & address pairs to configure BiQuad filters
> > + [ < {data1} {addr1} {data2} {addr2} ... >; ]
> > +- dlg,st-biquad-cfg : List of data & address pairs to configure Sidetone
> > + BiQuad filters
> > + [ < {data1} {addr1} {data2} {addr2} ... >; ]
>
> These look like DSP coefficients which I would therefore expect to be
> configurable at runtime via a binary control rather than specified in
> the DT - why are they in the DT?
The expectation was these would be set once and left for a platform, which is
why I added them to the DT. However there's no reason they couldn't be moved
to binary control. Having looked at other codecs, I assume SND_SOC_BYTES* would
be the preferred method for setting this kind of data?
N�r��yb�X��ǧv�^�){.n�+{zX����ܨ}���Ơz�&j:+v���zZ+��+zf���h���~i���z� �w���?�&�)ߢf��^jǫy�m��@A�a���
0��h���i
RE: [PATCH v2 3/3] ASoC: da7213: Add bindings documentation for codec driver
On October 08, 2015 14:16, Rob Herring wrote: > >> > +- dlg,micbias1-lvl : Voltage (mV) for Mic Bias 1 > >> > + [<1600>, <2200>, <2500>, <3000>] > >> > +- dlg,micbias2-lvl : Voltage (mV) for Mic Bias 2 > >> > + [<1600>, <2200>, <2500>, <3000>] > >> > >> Please append the units (-microvolt). > > > > Given that a user needs to read the bindings document to understand what is > > available, and what they're for, this seems a little unnecessary. > > You may think so, but it is standard practice. I can find a number of situations in the kernel, where this just isn't followed, for device specific bindings. What percentage have to follow this for it to be 'standard'? Is this documented somewhere so it's clear for those writing drivers which need to use DT bindings? I had a quick look and couldn't find anything on this. > >> > >> > +- dlg,dmic-clkrate : DMIC clock frequency (MHz). > >> > + [<150>, <300>] > >> > >> So 1.5GHz or 3GHz? > >> > >> Add units (-hz). > > > > Agreed, the description of MHz is misleading so will update to Hz. Thanks. > > Again though, I don't see what the suffix gives you, and would prefer to > > leave > > the binding as is. > > It tells someone reading the dts what the units are without having to > find the documentation and helps prevent people using properties with > differing units. For a lot of device specific bindings, you will need to read the associated documentation, and possibly the datasheet, to understand what it's for, and what values are valid. I don't see this suffix really saving you any time. For standard frameworks ok, but here it seems overkill.
RE: [PATCH v2 3/3] ASoC: da7213: Add bindings documentation for codec driver
On October 07, 2015 17:22, Rob Herring wrote:
> > +- dlg,micbias1-lvl : Voltage (mV) for Mic Bias 1
> > + [<1600>, <2200>, <2500>, <3000>]
> > +- dlg,micbias2-lvl : Voltage (mV) for Mic Bias 2
> > + [<1600>, <2200>, <2500>, <3000>]
>
> Please append the units (-microvolt).
Given that a user needs to read the bindings document to understand what is
available, and what they're for, this seems a little unnecessary.
>
> > +- dlg,dmic-data-sel : DMIC channel select based on clock edge.
> > + ["lrise_rfall", "lfall_rrise"]
> > +- dlg,dmic-samplephase : When to sample audio from DMIC.
> > + ["on_clkedge", "between_clkedge"]
>
> How about boolean for these two.
Wanted these to be explicit, hence not choosing boolean. Would prefer to keep
them as is.
>
> > +- dlg,dmic-clkrate : DMIC clock frequency (MHz).
> > + [<150>, <300>]
>
> So 1.5GHz or 3GHz?
>
> Add units (-hz).
Agreed, the description of MHz is misleading so will update to Hz. Thanks.
Again though, I don't see what the suffix gives you, and would prefer to leave
the binding as is.
N�r��yb�X��ǧv�^�){.n�+{zX����ܨ}���Ơz�&j:+v���zZ+��+zf���h���~i���z� �w���?�&�)ߢf��^jǫy�m��@A�a���
0��h���i
RE: [PATCH v2 0/3] ASoC: da7213: Add DT support for codec
On October 07, 2015 15:11, Mark Brown wrote: > > Adam Thomson (3): > > ASoC: da7213: Add DT support to codec driver > > ASoC: da7213: Add support to handle mclk data provided to driver > > ASoC: da7213: Add bindings documentation for codec driver > > Please remember to send the DT binding before the code as is normal, > this helps review the code as we know in advance what the code is > supposed to do. Ok, understood. Will make sure in the future I order them this way. Thanks.
RE: [PATCH 1/2] ASoC: da7213: Add DT support to codec driver
On October 05, 2015 16:56, Mark Brown wrote:
> On Mon, Oct 05, 2015 at 04:40:18PM +0100, Adam Thomson wrote:
>
> > This patch adds support for DT bindings in the codec driver.
> > As part of this support, the mclk data can now be provided and
> > used to control the mclk during codec operation.
>
> Please split out the new MCLK configuration into a separate patch which
> describes what that configuration is, it's much easier to review that
> way. Otherwise there's a couple of minor issues here but nothing too
> critical.
Ok, no problem. Will do.
>
> > case SND_SOC_BIAS_STANDBY:
> > if (snd_soc_codec_get_bias_level(codec) == SND_SOC_BIAS_OFF) {
> > + /* MCLK */
> > + clk_prepare_enable(da7213->mclk);
> > +
>
> This doesn't check the return value.
Yep, will update.
>
> > .driver = {
> > .name = "da7213",
> > + .of_match_table = da7213_of_match,
>
> Please use of_match_ptr() here.
Ok, didn't know about that. Will update.
RE: [PATCH v5 0/6] Add support for DA9150 Fuel-Gauge
On September 22, 2015 18:11, Lee Jones wrote: > On Tue, 22 Sep 2015, Sebastian Reichel wrote: > > On Fri, Sep 11, 2015 at 11:05:21AM +0100, Adam Thomson wrote: > > > This patch set adds support for the Dialog DA9150 Fuel-Gauge. > > > > > > [...] > > > > > > Adam Thomson (6): > > > mfd: da9150: Add support for Fuel-Gauge > > > mfd: da9150: Update DT bindings for Fuel-Gauge support > > > power: Add support for DA9150 Fuel-Gauge > > > power: da9150: Add DT bindings documentation for Fuel-Gauge > > > mfd: da9150: Use relative paths in DT bindings document > > > mfd: da9150: Use DEFINE_RES_IRQ_NAMED() help macro for IRQ resource > > > > > > Documentation/devicetree/bindings/mfd/da9150.txt | 33 +- > > > .../devicetree/bindings/power/da9150-fg.txt| 23 + > > > drivers/mfd/da9150-core.c | 191 +-- > > > drivers/power/Kconfig | 10 + > > > drivers/power/Makefile | 1 + > > > drivers/power/da9150-fg.c | 578 > > > + > > > include/linux/mfd/da9150/core.h| 19 +- > > > 7 files changed, 810 insertions(+), 45 deletions(-) > > > create mode 100644 Documentation/devicetree/bindings/power/da9150-fg.txt > > > create mode 100644 drivers/power/da9150-fg.c > > > > I suggest, that you take the whole patchset via the mfd tree. > > That's fine, but I'm still missing an Ack for patch 4. Hi Lee, I believe all patches are now acked. Is there anything else needed to help this along? Thanks Adam
RE: [PATCH 7/8] mfd: da9052: Simplify function return logic
On September 29, 2015 12:26, Javier Martinez Canillas wrote: > The invoked functions already return zero on success or a negative > errno code so there is no need to open code the logic in the caller. > > Signed-off-by: Javier Martinez Canillas > --- Acked-by: Adam Thomson
RE: [PATCH 6/8] mfd: da903x: Simplify function return logic
On September 29, 2015 12:26, Javier Martinez Canillas wrote: > The invoked function already returns zero on success or a negative > errno code so there is no need to open code the logic in the caller. > > Signed-off-by: Javier Martinez Canillas > --- Acked-by: Adam Thomson
RE: [PATCH 1/3] ASoC: codecs: Add da7219 codec driver
On September 21, 2015 18:11, Mark Brown wrote: > > In a system scenario the likelihood of that happening is small as you > > cannot use the mic or headphones until they've been inserted. The system is > > only likely to act after the jack insertion events have occurred. However, > > it > > This really isn't an OK approach here, you're making a whole bunch of > assumptions about how the system is implemented that aren't robust and > will lead to loss of audio if things go wrong which is a pretty serious > consequence. Are you *sure* there's going to be a quick enough response > to cover all jack inserts and remove (especially under load), or that > userspace even bothers paying attention given that there's no other > input and output devices? You make a fair point, and I'd much rather it was bullet proof. > > would be better to prevent any chance of concurrent access. The problem is > > how > > best to lock the Kcontrols whilst the test procedure in progress. At the > > moment > > the only way I can see is to add explicit control set() functions which > > would > > use a lock that can also be controlled by the HP test code. Does this make > > sense > > to you or do you know of a simpler method? Obviously dapm has function to > > lock > > as required. > > Yes, you're going to have to do something like that if you want to do > this - you'll also need to lock the reads since otherwise userspace will > see the intermediate control states. Ok, will look at adding set() and get() functions for the controls affected. > > > For the cache resync idea, in terms of code, it will look cleaner, but you > > are > > talking about 4 to 5 times the number of I2C accesses to the device, to > > restore > > configuration. Does that not seem like a bit too much overhead? > > There's regcache_sync_region(). That's fine if the registers are clumped closely together. Will have a look and see if it works out cleaner. Thanks. > > > Why are we scheduling work - we're already in thread context? > > > hptest will take some time to complete (over 100ms), and in that time it's > > plausible that a user could remove the jack. If we deal with this in the IRQ > > thread, we won't be aware of jack removal during the process, and will send > > a > > report regardless, which will almost definitely be incorrect, and > > unnecessary. > > By spawning off work, it allows the removal to be dealt with if the hptest > > work > > procedure is currently in process, and then we can avoid sending incorrect > > jack > > reports at the end. > > OK, please document this then. Ok, will add comments to clarify. > > > > > + /* Un-drive headphones/lineout */ > > > > + snd_soc_update_bits(codec, DA7219_HP_R_CTRL, > > > > + DA7219_HP_R_AMP_OE_MASK, 0); > > > > + snd_soc_update_bits(codec, DA7219_HP_L_CTRL, > > > > + DA7219_HP_L_AMP_OE_MASK, 0); > > > > This looks like DAPM? > > > The control of driving the headphones or making them high impedance is > > handled > > in the AAD code because we cannot have the headphones driven before a jack > > is > > inserted as it will affect the pole detection. Adding it to DAPM seemed > > like it > > would cause more problems in terms of controlling when it would and > > wouldn't be > > enabled. > > IIRC you had some DAPM updates in adjacent code? Yes, there's a disable_pin() call for micbias. However that will not disable the pin indefinitely which is what I would require for this. I need to know that there's no chance of the pin being enabled prior to jack insertion. > > > > + default: > > > > + return DA7219_AAD_JACK_INS_DEB_20MS; > > > > This isn't an error? > > > Opted for HW default in case of invalid values provided. Maybe a dev_warn() > > would be useful though to indicate this is the case? > > Yes - the user has explicitly tried to set something and the driver is > ignoring it. Agreed. Will update accordingly. > > > > > + ret = regmap_bulk_read(da7219->regmap, reg, &val, sizeof(val)); > > > > + if (ret) > > > > + return ret; > > > > > + ucontrol->value.integer.value[0] = le16_to_cpu(val); > > > > This is *weird*. We do a bulk read for a single register using an API > > > that returns CPU endian data then make it CPU endian (without any > > > annotations on variables...). Why not use regmap_read()? Why swap? > > > Why not use raw I/O? > > > The device is 8-bit register access only, and the value spans two registers, > > hence why this is done here. The register defined for the Kcontrol is the > > first > > in the sequence of two registers (first lower byte, second upper byte). > > Thought > > this was cleaner than having two separate controls to configure upper and > > lower bytes. > > Again some documentation would help, and also using raw reads rather > than bulk rea
RE: [PATCH 1/3] ASoC: codecs: Add da7219 codec driver
On September 19, 2015 18:44, Mark Brown wrote:
> > + do {
> > + statusa = snd_soc_read(codec, DA7219_ACCDET_STATUS_A);
> > + if (statusa & DA7219_MICBIAS_UP_STS_MASK)
> > + micbias_up = true;
> > + } while (!micbias_up);
>
> This could go into an inifinite loop.
>
Only if the HW is unresponsive. However, it's probably better to add a timeout
and some debug in the unlikely event that does happen.
> > +static void da7219_aad_hptest_work(struct work_struct *work)
> > +{
> > + struct da7219_aad_priv *da7219_aad =
> > + container_of(work, struct da7219_aad_priv, hptest_work);
> > + struct snd_soc_codec *codec = da7219_aad->codec;
> > + struct da7219_priv *da7219 = snd_soc_codec_get_drvdata(codec);
> > +
> > + u8 tonegen_cfg1, tonegen_cfg2, tonegen_onper;
> > + u16 tonegen_freq1, tonegen_freq_hptest;
> > + u8 hpl_gain, hpr_gain, dacl_gain, dacr_gain, dac_filters1, dac_filters4;
> > + u8 dac_filters5, cp_ctrl, routing_dac, dacl_ctrl, dacr_ctrl;
> > + u8 mixoutl_sel, mixoutr_sel, st_outfilt_1l, st_outfilt_1r;
> > + u8 mixoutl_ctrl, mixoutr_ctrl, hpl_ctrl, hpr_ctrl, accdet_cfg8;
> > + int report = 0;
> > +
> > + /* Save current settings */
>
> This is obviously a massive reconfiguration of the device. I'm not
> seeing anything here which prevents userspace coming in and change the
> configuration while we're in this function, that would obviously have
> serious issues.
>
> I'm also wondering if this might be more elegantly implemented by going
> into cache bypass mode, doing the test and then using a cache resync to
> restore the initial configuration. That will at least avoid issues with
> updates adding a new register but not modifying it.
In a system scenario the likelihood of that happening is small as you
cannot use the mic or headphones until they've been inserted. The system is
only likely to act after the jack insertion events have occurred. However, it
would be better to prevent any chance of concurrent access. The problem is how
best to lock the Kcontrols whilst the test procedure in progress. At the moment
the only way I can see is to add explicit control set() functions which would
use a lock that can also be controlled by the HP test code. Does this make sense
to you or do you know of a simpler method? Obviously dapm has function to lock
as required.
For the cache resync idea, in terms of code, it will look cleaner, but you are
talking about 4 to 5 times the number of I2C accesses to the device, to restore
configuration. Does that not seem like a bit too much overhead?
> > + if (statusa & DA7219_JACK_TYPE_STS_MASK) {
> > + report |= SND_JACK_HEADSET;
> > + mask |= SND_JACK_HEADSET |
> SND_JACK_LINEOUT;
> > + schedule_work(&da7219_aad->btn_det_work);
> > + } else {
> > + schedule_work(&da7219_aad->hptest_work);
> > + }
>
> Why are we scheduling work - we're already in thread context?
hptest will take some time to complete (over 100ms), and in that time it's
plausible that a user could remove the jack. If we deal with this in the IRQ
thread, we won't be aware of jack removal during the process, and will send a
report regardless, which will almost definitely be incorrect, and unnecessary.
By spawning off work, it allows the removal to be dealt with if the hptest work
procedure is currently in process, and then we can avoid sending incorrect jack
reports at the end.
For button detection, for certain mics it's required to pulse the micbias to a
higher voltage, for a defined period of time, to enable the mic. Again this
period is likely to take maybe 100ms, depending on the mic, so it made far more
sense to me to do this outside of the IRQ thread. However, if you have a better
proposal for this then am happy to take it on board.
> > + /* Un-drive headphones/lineout */
> > + snd_soc_update_bits(codec, DA7219_HP_R_CTRL,
> > + DA7219_HP_R_AMP_OE_MASK, 0);
> > + snd_soc_update_bits(codec, DA7219_HP_L_CTRL,
> > + DA7219_HP_L_AMP_OE_MASK, 0);
>
> This looks like DAPM?
The control of driving the headphones or making them high impedance is handled
in the AAD code because we cannot have the headphones driven before a jack is
inserted as it will affect the pole detection. Adding it to DAPM seemed like it
would cause more problems in terms of controlling when it would and wouldn't be
enabled.
> > +static enum da7219_aad_jack_ins_deb da7219_aad_of_jack_ins_deb(u32 val)
> > +{
> > + switch (val) {
> > + case 5:
> > + return DA7219_AAD_JACK_INS_DEB_5MS;
> > + case 10:
> > + return DA7219_AAD_JACK_INS_DEB_10MS;
> > + case 20:
> > + return DA7219_AAD_JACK_INS_DEB_20MS;
> > + case 50:
> > + return D
RE: [PATCH 2/3] ASoC: da7219: Add bindings documentation for DA7219 audio codec
On September 19, 2015 18:10, Mark Brown wrote:
> > +- dlg,io-lvl : Expected voltage level range for digital IO
> > + ["2.5V_3.6V", "1.2V_2.8V"]
>
> If the driver needs to read or set the voltage a supply is at it should
> do that via the regulator API.
This would just be a read for the driver. However it's a fair point, so I'll
look to add passing of the regulator information for VDDIO, so i can set this
based on read voltage.
>
> > +- dlg,cp-mchange : Charge pump voltage tracking mode
> > + ["largest_vol", "dac_vol", "sig_mag"]
> > +- dlg,cp-vol-thresh : Charge pump volume threshold value (6-bit value)
> > + [ 0 - 0x3F ]
>
> Why are these in the device tree rather than runtime parameters?
>
From previous internal discussions, these seemed to be fire and forget
parameters, hence their inclusion in the DT binding, rather than as controls.
Personally didn't see either needing runtime updates.
> > +Child node - 'da7219_aad':
> > +
> > +Required properties:
> > +- interrupt-parent : Specifies the phandle of the interrupt controller to
> > which
> > + the IRQs from DA7219 AAD block are delivered to.
> > +- interrupts : IRQ line info for DA7219 AAD block.
> > + (See
> > Documentation/devicetree/bindings/interrupt-controller/interrupts.txt for
> > + further information relating to interrupt properties)
>
> Why is this not specified at the device level (the device does not
> appear to support other interrupts)?
Given the way that the driver code was structured, and that the IRQ is only used
for accessory detection, I added it to the child node. The other option would
be to flatten out bindings, and remove the child node. Felt like keeping the
accessory detect items separate though was a sensible approach. What is your
feeling on this?
N�r��yb�X��ǧv�^�){.n�+{zX����ܨ}���Ơz�&j:+v���zZ+��+zf���h���~i���z� �w���?�&�)ߢf��^jǫy�m��@A�a���
0��h���i
RE: [PATCH 02/12] ASoC: da9055: Fix module autoload for OF platform driver
On September 03, 2015 11:56, Luis de Bethencourt wrote: > This platform driver has a OF device ID table but the OF module > alias information is not created so module autoloading won't work. > > Signed-off-by: Luis de Bethencourt > --- Acked-by: Adam Thomson
RE: [PATCH v4 3/6] power: Add support for DA9150 Fuel-Gauge
On August 04, 2015 17:17, Adam Thomson wrote:
> This adds power supply driver support for the Fuel-Gauge part of
> the DA9150 combined Charger and Fuel-Gauge device.
>
> Signed-off-by: Adam Thomson
> ---
Hi Sebastian,
Just wondered when you might be able to review the power-supply parts of this
patch? Lee has Acked the MFD patches, so it's just these left.
Thanks
Adam
N�r��yb�X��ǧv�^�){.n�+{zX����ܨ}���Ơz�&j:+v���zZ+��+zf���h���~i���z� �w���?�&�)ߢf��^jǫy�m��@A�a���
0��h���i
RE: [PATCH V3] Watchdog: Fix parent of watchdog_devices
On August 19, 2015 04:28, Pratyush Anand wrote: > diff --git a/drivers/watchdog/da9052_wdt.c b/drivers/watchdog/da9052_wdt.c > index 2e9589652e1e..67e67977bd29 100644 > --- a/drivers/watchdog/da9052_wdt.c > +++ b/drivers/watchdog/da9052_wdt.c > @@ -195,6 +195,7 @@ static int da9052_wdt_probe(struct platform_device *pdev) > da9052_wdt->timeout = DA9052_DEF_TIMEOUT; > da9052_wdt->info = &da9052_wdt_info; > da9052_wdt->ops = &da9052_wdt_ops; > + da9052_wdt->parent = &pdev->dev; > watchdog_set_drvdata(da9052_wdt, driver_data); > > kref_init(&driver_data->kref); > diff --git a/drivers/watchdog/da9055_wdt.c b/drivers/watchdog/da9055_wdt.c > index 495089d8dbfe..04d1430d93d2 100644 > --- a/drivers/watchdog/da9055_wdt.c > +++ b/drivers/watchdog/da9055_wdt.c > @@ -161,6 +161,7 @@ static int da9055_wdt_probe(struct platform_device *pdev) > da9055_wdt->timeout = DA9055_DEF_TIMEOUT; > da9055_wdt->info = &da9055_wdt_info; > da9055_wdt->ops = &da9055_wdt_ops; > + da9055_wdt->parent = &pdev->dev; > watchdog_set_nowayout(da9055_wdt, nowayout); > watchdog_set_drvdata(da9055_wdt, driver_data); > > diff --git a/drivers/watchdog/da9062_wdt.c b/drivers/watchdog/da9062_wdt.c > index b3a870ce85be..7386111220d5 100644 > --- a/drivers/watchdog/da9062_wdt.c > +++ b/drivers/watchdog/da9062_wdt.c > @@ -210,6 +210,7 @@ static int da9062_wdt_probe(struct platform_device *pdev) > wdt->wdtdev.max_timeout = DA9062_WDT_MAX_TIMEOUT; > wdt->wdtdev.timeout = DA9062_WDG_DEFAULT_TIMEOUT; > wdt->wdtdev.status = WATCHDOG_NOWAYOUT_INIT_STATUS; > + wdt->wdtdev.parent = &pdev->dev; > > watchdog_set_drvdata(&wdt->wdtdev, wdt); > dev_set_drvdata(&pdev->dev, wdt); > diff --git a/drivers/watchdog/da9063_wdt.c b/drivers/watchdog/da9063_wdt.c > index e2fe2ebdebd4..6bf130bd863d 100644 > --- a/drivers/watchdog/da9063_wdt.c > +++ b/drivers/watchdog/da9063_wdt.c > @@ -175,6 +175,7 @@ static int da9063_wdt_probe(struct platform_device *pdev) > wdt->wdtdev.min_timeout = DA9063_WDT_MIN_TIMEOUT; > wdt->wdtdev.max_timeout = DA9063_WDT_MAX_TIMEOUT; > wdt->wdtdev.timeout = DA9063_WDG_TIMEOUT; > + wdt->wdtdev.parent = &pdev->dev; > > wdt->wdtdev.status = WATCHDOG_NOWAYOUT_INIT_STATUS; > Acked-by: Adam Thomson
RE: [PATCH v4 1/6] mfd: da9150: Add support for Fuel-Gauge
On August 10, 2015 14:54, Lee Jones wrote: > On Tue, 04 Aug 2015, Adam Thomson wrote: > > > Signed-off-by: Adam Thomson > > --- > > > > Changes in v4: > > - Update compatible string of fuel-gauge to "dlg, da9150-fuel-gauge". Also > >made similar change to driver name to keep things consistent. > > > > Changes in v3: > > - Use DEFINE_RES_IRQ_NAMED() helper for defining FG IRQ resource. > > - Unwanted new line & comments removed. > > - Remove gotos which can be replaced with straight forward return calls. > > - Add enum for indexing MFD cells list, which is used to assign pdata for > > FG > >sub-device. > > - Remove use of function pointers for QIF related read/write functions as > >currently only I2C supported, so call I2C functions directly. > > - Fold fg.h contents into core.h. > > Assuming these have all been taken care of: > > Acked-by: Lee Jones > Thanks for your time on this patch set.
RE: [PATCH v3 3/6] power: Add support for DA9150 Fuel-Gauge
On August 03, 2015 18:26, Sebastian Reichel wrote:
> > > > +MODULE_LICENSE("GPL");
> > >
> > > MODULE_LICENSE("GPL v2");
> >
> > If I do this then I need to update the file license disclaimer at the top,
> > as
> > right now they match and are correct as far as I can tell. Is this change
> > needed
> > and if so is this the intention for other drivers as well, just so I'm
> > clear?
>
> The header states GPL v2+.
>
> -- Sebastian
Yes, and this matches the "GPL" string as I understand it. The following is
taken from "include/linux/module.h":
---
/*
* The following license idents are currently accepted as indicating free
* software modules
*
* "GPL" [GNU Public License v2 or later]
* "GPL v2"[GNU Public License v2]
RE: [PATCH v3 4/6] power: da9150: Add DT bindings documentation for Fuel-Gauge
On July 24, 2015 23:01, Sebastian Reichel wrote:
> Hi,
>
> On Tue, Jul 07, 2015 at 05:34:21PM +0100, Adam Thomson wrote:
> > diff --git a/Documentation/devicetree/bindings/power/da9150-fg.txt
> b/Documentation/devicetree/bindings/power/da9150-fg.txt
> > new file mode 100644
> > index 000..c3c76eb
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/power/da9150-fg.txt
> > @@ -0,0 +1,23 @@
> > +Dialog Semiconductor DA9150 Fuel-Gauge Power Supply bindings
> > +
> > +Required properties:
> > +- compatible: "dlg,da9150-fg" for DA9150 Fuel-Gauge Power Supply
> ^
>
> s/dlg,da9150-fg/dlg,da9150-fuel-gauge/g
>
Ok, can do.
> > +Optional properties:
> > +- dlg,update-interval: Interval time (milliseconds) between battery level
> > checks.
> > +- dlg,warn-soc-level: Battery discharge level (%) where warning event
> > raised.
> > + [1 - 100]
> > +- dlg,crit-soc-level: Battery discharge level (%) where critical event
> > raised.
> > + This value should be lower than the warning level.
> > + [1 - 100]
> > +
> > +
> > +Example:
> > +
> > + da9150-fg {
>
> Make this just "fuel-gauge".
No problem.
>
> > + compatible = "dlg,da9150-fg";
> ^
>
> here too ;)
Consider it done. :)
>
> > +
> > + dlg,update-interval = <1>;
> > + dlg,warn-soc-level = /bits/ 8 <15>;
> > + dlg,crit-soc-level = /bits/ 8 <5>;
> > + };
>
> -- Sebastian
N�r��yb�X��ǧv�^�){.n�+{zX����ܨ}���Ơz�&j:+v���zZ+��+zf���h���~i���z� �w���?�&�)ߢf��^jǫy�m��@A�a���
0��h���i
RE: [PATCH v3 3/6] power: Add support for DA9150 Fuel-Gauge
On July 25, 2015 18:27, Sebastian Reichel wrote:
> Hi Adam,
>
> The driver looks mostly fine. I have a few comments, though:
>
> On Tue, Jul 07, 2015 at 05:34:19PM +0100, Adam Thomson wrote:
> > Signed-off-by: Adam Thomson
>
> Please add a short description to the commit message.
>
Ok, I can add something. Didn't feel like it needed to be too descriptive, but
can make it a little more verbose.
> > [...]
> >
> > +config FG_DA9150
>
> Please name the config entry BATTERY_DA9150 to be consistent with
> the other fuel gauges.
>
Ok no problem.
> > [...]
> >
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
>
> I wonder if the "depends CHARGER_DA9150" in Kconfig should be
> "depends MFD_DA9150" instead.
>
I did think about this, but it felt like it was a bit pointless having the
fuel-gauge driver without the charger driver, as they're intrinsically tied in
HW. I guess if you don't want the charging reporting but still wanted fuel-gauge
information then maybe. I don't mind making the change you mention as there
should be no functional impact.
> > [...]
> > +
> > +/* Power Supply attributes */
> > +static int da9150_fg_capacity(struct da9150_fg *fg,
> > + union power_supply_propval *val)
> > +{
> > + da9150_fg_read_sync_start(fg);
> > + val->intval = da9150_fg_read_attr(fg, DA9150_QIF_SOC_PCT,
> > + DA9150_QIF_SOC_PCT_SIZE);
> > + da9150_fg_read_sync_end(fg);
>
> Create a wrapper function for this. Reading a single value
> with sync guards is done multiple times.
>
> da9150_fg_read_attr_sync(fg, ...) {
> da9150_fg_read_sync_start(fg);
> da9150_fg_read_attr(fg, ...);
> da9150_fg_read_sync_end(fg);
> }
>
Yep, fair point. Can add something for the single attribute accesses.
> > + if (val->intval > 100)
> > + val->intval = 100;
> > +
> > + return 0;
> > +}
> > +
> >
> > [...]
> > + DA9150_QIF_E_FG_STATUS_SIZE);
> > +
> > + /* Handle warning/critical threhold events */
> > + if ((DA9150_FG_IRQ_LOW_SOC_MASK | DA9150_FG_IRQ_HIGH_SOC_MASK)
> &
> > + e_fg_status)
>
> Please make this (e_fg_status & CONSTANT_MASK).
>
Fine, not a problem.
> > + da9150_fg_soc_event_config(fg);
> > +
> > + /* Clear any FG IRQs */
> > + da9150_fg_write_attr(fg, DA9150_QIF_E_FG_STATUS,
> > +DA9150_QIF_E_FG_STATUS_SIZE, e_fg_status);
> > +
> > + return IRQ_HANDLED;
> > +}
> > +
> >
> > [...]
> >
> > +static int da9150_fg_probe(struct platform_device *pdev)
> > +{
> > + struct device *dev = &pdev->dev;
> > + struct da9150 *da9150 = dev_get_drvdata(dev->parent);
> > + struct da9150_fg_pdata *fg_pdata = dev_get_platdata(dev);
> > + struct da9150_fg *fg;
> > + int ver, irq, ret;
> > +
> > + fg = devm_kzalloc(dev, sizeof(*fg), GFP_KERNEL);
> > + if (fg == NULL)
> > + return -ENOMEM;
> > +
> > + platform_set_drvdata(pdev, fg);
> > + fg->da9150 = da9150;
> > + fg->dev = dev;
> > +
> > + mutex_init(&fg->io_lock);
> > +
> > + /* Enable QIF */
> > + da9150_set_bits(da9150, DA9150_CORE2WIRE_CTRL_A,
> DA9150_FG_QIF_EN_MASK,
> > + DA9150_FG_QIF_EN_MASK);
> > +
> > + fg->battery = power_supply_register(dev, &fg_desc, NULL);
> > + if (IS_ERR(fg->battery)) {
> > + ret = PTR_ERR(fg->battery);
> > + return ret;
> > + }
>
> Please use devm_power_supply_register(...) to simplify the driver.
>
> > + ver = da9150_fg_read_attr(fg, DA9150_QIF_FW_MAIN_VER,
> > + DA9150_QIF_FW_MAIN_VER_SIZE);
> > + dev_info(dev, "Version: 0x%x\n", ver);
> > +
> > + /* Handle DT data if provided */
> > + if (dev->of_node) {
> > + fg_pdata = da9150_fg_dt_pdata(dev);
> > + dev->platform_data = fg_pdata;
> > + }
> > +
> > + /* Handle any pdata provided */
> > + if (fg_pdata) {
> > + fg->interval = fg_pdata->update_interval;
> > +
> > + if (fg_pdata->warn_soc_lvl > 100)
> > + dev_warn(dev, "Invalid SOC warning level provided,
> Ignoring");
> > + else
> > + fg->warn_soc = fg_pdata->warn_soc_lvl;
> > +
> > + if ((fg_pdata->crit_soc_lvl > 100) ||
> > + (fg_pdata->crit_soc_lvl >= fg_pdata->warn_soc_lvl))
> > + dev_warn(dev, "Invalid SOC critical level provided,
> Ignoring");
> > + else
> > + fg->crit_soc = fg_pdata->crit_soc_lvl;
> > +
> > +
> > + }
> > +
> > + /* Configure initial SOC level events */
> > + da9150_fg_soc_event_config(fg);
> > +
> > + /*
> > +* If an interval period has been provided then setup repeating
> > +* work for reporting data updates.
> > +*/
> > + if (fg->interval) {
> > + INIT_DELAYED_WORK(&fg->work, da9150_fg_work);
> > +
RE: [PATCH v2 3/4] power: Add support for DA9150 Fuel-Gauge
On July 3, 2015 16:22, Lee Jones wrote:
> > +/*
> > + * Function template to provide battery temperature. Should provide
> > + * 0.1 degrees C resolution return values.
> > + */
> > +typedef int (*da9150_read_temp_t)(void *context);
> > +
> > +/* Register temp callback function */
> > +void da9150_fg_register_temp_cb(struct power_supply *psy,
> da9150_read_temp_t cb,
> > + void *cb_context);
> > +
> > #endif /* __DA9150_FG_H */
>
> I still don't get why you think pointers are better than just calling
> the function directly. Can the *fn() ever point to different functions?
Here, the intention is to cover the scenario where a battery has no internal
thermistor, and cannot provide temperature readings to the DA9150 device. In
that scenario I've allowed for the option of providing an external function
which can give the temperature reading instead as DA9150 will not be able to
provide a correct reading in that scenario. This would be platform dependent and
such a platform using a battery not employing an NTC in their battery can
register its own call-back function to provide battery temperature instead.
So, in answer to your question, yes.
N�r��yb�X��ǧv�^�){.n�+{zX����ܨ}���Ơz�&j:+v���zZ+��+zf���h���~i���z� �w���?�&�)ߢf��^jǫy�m��@A�a���
0��h���i
RE: [PATCH v2 2/4] mfd: da9150: Update DT bindings for Fuel-Gauge support
On July 3, 2015 16:19, Lee Jones wrote:
> > @@ -22,6 +23,7 @@ Required properties:
> > Sub-devices:
> > - da9150-gpadc: See Documentation/devicetree/bindings/iio/adc/da9150-
> gpadc.txt
> > - da9150-charger: See Documentation/devicetree/bindings/power/da9150-
> charger.txt
> > +- da9150-fg: See Documentation/devicetree/bindings/power/da9150-fg.txt
>
> Can you send a subsequent patch doing:
>
> s-Documentation/devicetree/bindings/-../-
Yep, can do.
> >
> >
>
> Superfluous '\n' here.
Will remove.
>
> > Example:
> > @@ -40,4 +42,8 @@ Example:
> > da9150-charger {
> > ...
> > };
> > +
> > + da9150-fg {
> > + ...
> > + };
>
> Perhaps a little content wouldn't go amiss here for quick reference.
>
> Saves dipping in and out of the other binding files.
>
> This patch is fine though:
> Acked-by: Lee Jones
>
Thanks for the ack. Will add a little info in the example.
N�r��yb�X��ǧv�^�){.n�+{zX����ܨ}���Ơz�&j:+v���zZ+��+zf���h���~i���z� �w���?�&�)ߢf��^jǫy�m��@A�a���
0��h���i
RE: [PATCH v2 1/4] mfd: da9150: Add support for Fuel-Gauge
On July 3, 2015 16:16, Lee Jones wrote:
>
> Changelog v1 => v2?
Is described in the cover letter, but can move that information to the
individual
patches in the future, if that helps.
> >
> > +static struct resource da9150_fg_resources[] = {
> > + {
> > + .name = "FG",
> > + .start = DA9150_IRQ_FG,
> > + .end = DA9150_IRQ_FG,
> > + .flags = IORESOURCE_IRQ,
>
> Can you use the DEFINE_RES_* helpers?
Hadn't spotted those before. Will use those instead.
> > - if (!da9150)
> > - return -ENOMEM;
> > + if (!da9150) {
> > + ret = -ENOMEM;
> > + goto fail;
>
> Don't do this. 'fail' doesn't do anything, just return here.
Ok. I was opting for choosing one consistent method of handling failure, rather
than a combination of gotos and immediate returns, but will change.
>
> > + }
> >
> > da9150->dev = &client->dev;
> > da9150->irq = client->irq;
> > @@ -332,19 +444,46 @@ static int da9150_probe(struct i2c_client *client,
> > ret = PTR_ERR(da9150->regmap);
> > dev_err(da9150->dev, "Failed to allocate register map: %d\n",
> > ret);
> > - return ret;
> > + goto fail;
>
> It was better before.
Ok
>
> > + }
> > +
> > + /* Setup secondary I2C interface for QIF access */
> > + qif_addr = da9150_reg_read(da9150, DA9150_CORE2WIRE_CTRL_A);
> > + qif_addr = (qif_addr & DA9150_CORE_BASE_ADDR_MASK) >> 1;
> > + qif_addr |= DA9150_QIF_I2C_ADDR_LSB;
> > + da9150->core_qif = i2c_new_dummy(client->adapter, qif_addr);
> > + if (!da9150->core_qif) {
> > + dev_err(da9150->dev, "Failed to attach QIF client\n");
> > + ret = -ENODEV;
> > + goto fail;
>
> Same.
Ok
>
> > }
> >
> > - da9150->irq_base = pdata ? pdata->irq_base : -1;
> > + i2c_set_clientdata(da9150->core_qif, da9150);
> > + da9150->read_dev = (read_dev_t) da9150_i2c_read_device;
> > + da9150->write_dev = (write_dev_t) da9150_i2c_write_device;
>
> Is it possible for read_dev to be set to anything other than
> da9150_i2c_read_device?
Right now there is no SPI support in the driver so no. Given the comment further
down, I'll remove these function pointers for now, and directly call the I2C
related functions.
>
> > + if (pdata) {
> > + da9150->irq_base = pdata->irq_base;
> > +
> > + da9150_devs[2].platform_data = pdata->fg_pdata;
> > + da9150_devs[2].pdata_size = sizeof(struct da9150_fg_pdata);
>
> This is fragile.
>
> If another device gets inserted above the FG, this will break.
>
It is unlikely, but you are right. Will fix this.
> > diff --git a/include/linux/mfd/da9150/core.h
> > b/include/linux/mfd/da9150/core.h
> > index 76e6689..98ecfea 100644
> > --- a/include/linux/mfd/da9150/core.h
> > +++ b/include/linux/mfd/da9150/core.h
> > @@ -15,9 +15,11 @@
> > #define __DA9150_CORE_H
> >
> > #include
> > +#include
> > #include
> > #include
> >
> > +
>
> Please remove this.
Yep.
>
> > /* I2C address paging */
> > #define DA9150_REG_PAGE_SHIFT 8
> > #define DA9150_REG_PAGE_MASK 0xFF
> > @@ -46,23 +48,40 @@
> > #define DA9150_IRQ_GPADC 19
> > #define DA9150_IRQ_WKUP20
> >
> > +/* Platform Data */
>
> No need for this comment. The pdata in the name give it away.
Fine.
>
> > +struct da9150_fg_pdata;
>
> Why can't you just put the struct definition in here?
>
Was a design choice as I was providing a header for FG to expose the temp
reading callback function and figured it made sense to keep other FG related
items in there as well. Can fold them all into the core.h though if that's the
preferred method.
> > struct da9150_pdata {
> > int irq_base;
> > + struct da9150_fg_pdata *fg_pdata;
> > };
> >
> > +/* I/O function typedefs for raw access */
> > +typedef int (*read_dev_t)(void *client, u8 addr, int count, u8 *buf);
> > +typedef int (*write_dev_t)(void *client, u8 addr, int count, const u8
> > *buf);
>
> No need to typedef, just pop them directly into the struct. Although
> I'm not convinced that need them at all really.
Will remove use of this for now, as without SPI support, they don't add
anything.
>
> > struct da9150 {
> > struct device *dev;
> > struct regmap *regmap;
> > + struct i2c_client *core_qif;
> > +
> > + read_dev_t read_dev;
> > + write_dev_t write_dev;
> > +
> > struct regmap_irq_chip_data *regmap_irq_data;
> > int irq;
> > int irq_base;
>
> Why are there 2 irq_bases? That could get pretty confusing.
>
> Why is it being stored?
>
> Same with irq.
Not 2 IRQ bases. 'irq' is the chip IRQ. Same as is done in a number of other
MFD drivers. Technically though they don't need storing within the private data,
and was done as a convenience. I can add a follow up patch to remove this, if
it's wanted though.
>
> > };
> >
> > /* Device I/O */
> > +void da9150_read_qif(struct da9150 *da9150, u8 addr, int count, u8 *buf);
> > +
RE: [PATCH 1/4] mfd: da9150: Add support for Fuel-Gauge
On June 22, 2015 17:48, Lee Jones wrote: > > > The pedant in me noticed that this function is actually added in 3/4. So > > > this chunk might be moved to 3/4, if you like to entertain pedantry like > > > that, that is. (But see my remark on 3/4 too.) > > > > This is true, but as the header is part of MFD, I included it as part of > > that > > patch as I thought this made more sense. I guess Lee will comment as to > > whether > > that was correct or not. :) > > Glanced over this (as I still have 240 unread emails to attend to). > If the question is whether to submit the prototype at the same time as > the associated function, the answer is yes. Hope you've managed to trawl through the e-mails. The question was whether or not the function and prototype should be submitted as part of the same individual patch file. Both are submitted as part of the patch set but the prototype resides in the MFD patch, and the function in the power (fuel-gauge) patch. Personally, this seems fine to me but I'm not a maintainer.
RE: [PATCH 1/4] mfd: da9150: Add support for Fuel-Gauge
On June 19, 2015 17:48, Paul Bolle wrote: > On Thu, 2015-06-18 at 17:06 +0100, Adam Thomson wrote: > > --- /dev/null > > +++ b/include/linux/mfd/da9150/fg.h > > > +/* > > + * Function template to provide battery temperature. Should provide > > + * 0.1 degrees C resolution return values. > > + */ > > +typedef int (*da9150_read_temp_t)(void *context); > > + > > +/* Register temp callback function */ > > +void da9150_fg_register_temp_cb(struct power_supply *psy, > da9150_read_temp_t cb, > > + void *cb_context); > > The pedant in me noticed that this function is actually added in 3/4. So > this chunk might be moved to 3/4, if you like to entertain pedantry like > that, that is. (But see my remark on 3/4 too.) This is true, but as the header is part of MFD, I included it as part of that patch as I thought this made more sense. I guess Lee will comment as to whether that was correct or not. :)
RE: [PATCH 3/4] power: Add support for DA9150 Fuel-Gauge
On June 19, 2015 17:52, Paul Bolle wrote:
> On Thu, 2015-06-18 at 17:06 +0100, Adam Thomson wrote:
> > --- /dev/null
> > +++ b/drivers/power/da9150-fg.c
>
> > +/* Register external function to give battery temperature */
> > +void da9150_fg_register_temp_cb(struct power_supply *psy,
> da9150_read_temp_t cb,
> > + void *cb_context)
> > +{
> > + struct da9150_fg *fg = dev_get_drvdata(psy->dev.parent);
> > +
> > + fg->read_bat_temp = cb;
> > + fg->bat_temp_context = cb_context;
> > +}
> > +EXPORT_SYMBOL_GPL(da9150_fg_register_temp_cb);
>
> This series doesn't add a user of this export. Actually, it doesn't even
> add a local caller of this function. Is a patch that adds a user of this
> function queued somewhere?
The device relies on the battery providing temperature through a dedicated
pin, but that isn't always the case. I added this function as a means to handle
the scenarios where the battery being used didn't provide such a feature so
this allows for an alternative method of giving the battery temperature. At
present there is no patch lined up to use this.
N�r��yb�X��ǧv�^�){.n�+{zX�>Wi�axPj�m -�+��d�_
RE: [PATCH v7 5/7] power: Add support for DA9150 Charger
On February 25, 2015 12:11, Jonathan Cameron wrote:
> > +static int da9150_charger_remove(struct platform_device *pdev)
> > +{
> > + struct da9150_charger *charger = platform_get_drvdata(pdev);
> > + int irq;
> > +
> > + /* Make sure IRQs are released before unregistering power supplies */
> > + irq = platform_get_irq_byname(pdev, "CHG_VBUS");
> > + free_irq(irq, charger);
> I'd use your unregister_irq function here as well. Better than open
> coding the same thing again. (more obviously correct as well as the
> error condition unwinding above then looks just like we have here).
Fair point. Will update that when I start on the additional modules I need to
submit. Thanks.
N�r��yb�X��ǧv�^�){.n�+{zX����ܨ}���Ơz�&j:+v���zZ+��+zf���h���~i���z� �w���?�&�)ߢf��^jǫy�m��@A�a���
0��h���i
RE: [PATCH v7 1/7] mfd: Add support for DA9150 combined charger & fuel-gauge device
On February 25, 2015 20:08, Sebastian Reichel wrote: > Hi, > > On Wed, Feb 18, 2015 at 02:08:24PM +, Adam Thomson wrote: > > DA9150 is a combined Charger and Fuel-Gauge IC, with additional > > GPIO and GPADC functionality. > > > > Signed-off-by: Adam Thomson > > Acked-by: Lee Jones > > This patch is already in 4.0-rc1 making things easier for 4.1 :) Excellent. Didn't realise it had already made it in. Thanks.
RE: [PATCH v6 0/7] Add initial support for DA9150 Charger & Fuel-Gauge IC
On February 17, 2015 18:04, Sebastian Reichel wrote:
> Hi,
>
> On Mon, Feb 16, 2015 at 09:45:59PM +, Opensource [Adam Thomson] wrote:
> > Just a quick one to see what the status is on this patch set? Just
> > interested as to when it'll be pulled in.
>
> I plan to pull it into for-next once -rc1 has been tagged (probably
> about a week). Feel free to send a new version with the style fixes.
Ok thanks for the update. Will see if I can get the updates out before then.
Thanks
Adam
N�r��yb�X��ǧv�^�){.n�+{zX����ܨ}���Ơz�&j:+v���zZ+��+zf���h���~i���z� �w���?�&�)ߢf��^jǫy�m��@A�a���
0��h���i
RE: [PATCH v6 0/7] Add initial support for DA9150 Charger & Fuel-Gauge IC
On January 22, 2015 07:58, Lee Jones wrote: > On Wed, 21 Jan 2015, Jonathan Cameron wrote: > > > > > > > On 21 January 2015 16:52:50 GMT+00:00, Sebastian Reichel > wrote: > > >Hi, > > > > > >On Wed, Jan 21, 2015 at 03:46:25PM +, Adam Thomson wrote: > > >> This patch set adds initial support for the Dialog DA9150 Integrated > > >Charger & > > >> Fuel-Gauge IC. The device also provides GPIO and GPADC functionality. > > >> > > >> In this patch set the following is provided: > > >> - MFD Core support and DT bindings documentation. > > >> - IIO GPADC support and DT bindings documentation. > > >> - Power Supply Charger support and DT bindings documentation. > > >> - Update to MAINTAINERS file to add DA9150 files to Dialog support > > >list. > > >> > > >> To keep patch submission from being too large, support for GPIO and > > >Fuel-Gauge > > >> will come after initial support patches are accepted. > > >> > > >> This patch set is baselined against the v3.19-rc5 kernel version. > > >> > > >> Changes in v6: > > >> - For GPADC driver, Use lower case extended names for IIO GPADC > > >channels, and > > >> remove extended names for GPIO related channels. > > >> - For charger driver, revert to no devm IRQ request/free functions as > > >ordering > > >> is important, and probe failure scenario was not being correctly > > >covered. A > > >> helper function for free IRQs has been added to make code cleaner. > > > > > >Probably a devm_ variant of power_supply_register should be created, > > >but for now this is ok for me. > > > > > >Jonathan, Lee: I assume you are ok with the whole patchset going > > >through the power supply subsystem? > > > > > Fine by me if either power supply or MFD. > > I am, but there is a lot of MFD code here, so I'd like to take it in > as well. I'd like a tagged pull-request from Sebastian. Hi Sebastien, Just a quick one to see what the status is on this patch set? Just interested as to when it'll be pulled in. Thanks Adam
RE: [PATCH v6 5/7] power: Add support for DA9150 Charger
On January 21, 2015 16:15, Varka Bhadram wrote:
> On Wednesday 21 January 2015 09:16 PM, Adam Thomson wrote:
> > This patch adds support for DA9150 Charger & Fuel-Gauge IC Charger.
> >
> > Signed-off-by: Adam Thomson
> > ---
>
> (...)
>
> > +static int da9150_charger_register_irq(struct platform_device *pdev,
> > + irq_handler_t handler,
> > + const char *irq_name)
> > +{
> > + struct device *dev = &pdev->dev;
> > + struct da9150_charger *charger = platform_get_drvdata(pdev);
> > + int irq, ret;
> > +
> > + irq = platform_get_irq_byname(pdev, irq_name);
> > + if (irq < 0) {
> > + dev_err(dev, "Failed to get IRQ CHG_STATUS: %d\n", irq);
> > + return irq;
> > + }
> > +
> > + ret = request_threaded_irq(irq, NULL, handler, IRQF_ONESHOT, irq_name,
> > + charger);
>
> Why don you use devm_* API..?
As mentioned in previous discussions, the order of tidy up is important so
devm_* functions cannot be used here.
>
> > + if (ret)
> > + dev_err(dev, "Failed to request IRQ %d: %d\n", irq, ret);
> > +
> > + return ret;
> > +}
> > +
> > +static void da9150_charger_unregister_irq(struct platform_device *pdev,
> > + const char *irq_name)
> > +{
> > + struct device *dev = &pdev->dev;
> > + struct da9150_charger *charger = platform_get_drvdata(pdev);
> > + int irq;
> > +
> > + irq = platform_get_irq_byname(pdev, irq_name);
> > + if (irq < 0) {
> > + dev_err(dev, "Failed to get IRQ CHG_STATUS: %d\n", irq);
> > + return;
> > + }
> > +
> > + free_irq(irq, charger);
> > +}
> > +
> > +static int da9150_charger_probe(struct platform_device *pdev)
> > +{
> > + struct device *dev = &pdev->dev;
> > + struct da9150 *da9150 = dev_get_drvdata(dev->parent);
> > + struct da9150_charger *charger;
> > + struct power_supply *usb, *battery;
> > + u8 reg;
> > + int ret;
> > +
> > + charger = devm_kzalloc(dev, sizeof(struct da9150_charger), GFP_KERNEL);
> > + if (charger == NULL)
> > + return -ENOMEM;
>
> sizeof(struct da9150_charger) can be replaced with sizeof(*charger)...
>
> *!* operator can be used in comparison with NULL...
>
Yes, but functionally no different. Can maybe change this around later, but
would like to avoid changing current patch set right now.
RE: [PATCH v6 3/7] iio: Add support for DA9150 GPADC
On January 21, 2015 16:07, Varka Bhadram wrote:
> On Wednesday 21 January 2015 09:16 PM, Adam Thomson wrote:
> > This patch adds support for DA9150 Charger & Fuel-Gauge IC GPADC.
> >
> > Signed-off-by: Adam Thomson
> > Reviewed-by: Hartmut Knaack
> > Acked-by: Jonathan Cameron
>
> (...)
>
> > +
> > +static int da9150_gpadc_probe(struct platform_device *pdev)
> > +{
> > + struct device *dev = &pdev->dev;
> > + struct da9150 *da9150 = dev_get_drvdata(dev->parent);
> > + struct da9150_gpadc *gpadc;
> > + struct iio_dev *indio_dev;
> > + int irq, ret;
> > +
> > + indio_dev = devm_iio_device_alloc(&pdev->dev, sizeof(*gpadc));
>
> You can directly use *dev* instead of *&pdev->dev*
Yes agreed. Can follow up with change for this as it's not urgent and would
rather leave current patch set as is.
N�r��yb�X��ǧv�^�){.n�+{zX����ܨ}���Ơz�&j:+v���zZ+��+zf���h���~i���z� �w���?�&�)ߢf��^jǫy�m��@A�a���
0��h���i
RE: [PATCH v5 3/7] iio: Add support for DA9150 GPADC
On January 20, 2015 20:49, Jonathan Cameron wrote:
> > +#define DA9150_GPADC_CHANNEL_PROCESSED(_id, _hw_id, _type,
> >> _ext_name)
> \
> > + DA9150_GPADC_CHANNEL(_id, _hw_id, _type,
> > \
> > +BIT(IIO_CHAN_INFO_PROCESSED), _ext_name)
> > +
> > +/* Supported channels */
> > +static const struct iio_chan_spec da9150_gpadc_channels[] = {
> > + DA9150_GPADC_CHANNEL_PROCESSED(GPIOA, GPIOA_6V, IIO_VOLTAGE,
> "GPIOA"),
> > + DA9150_GPADC_CHANNEL_PROCESSED(GPIOB, GPIOB_6V, IIO_VOLTAGE,
> "GPIOB"),
> > + DA9150_GPADC_CHANNEL_PROCESSED(GPIOC, GPIOC_6V, IIO_VOLTAGE,
> "GPIOC"),
> > + DA9150_GPADC_CHANNEL_PROCESSED(GPIOD, GPIOD_6V, IIO_VOLTAGE,
> "GPIOD"),
> I'm not sure some of these really deserve extended names. Those are
> usually
> reserved for naming strange internal adc channels etc. These first 4 are
> presumably just for input pins? Those should just be channels 0..3
> On another note, unless you want really weird sysfs attribute names, the
> extended names want to be lowercase.
>
> >>>
> >>> I'd prefer to keep the names because the input pins are muxed with GPIOs
> >>> of the
> >>> chip, so thought it sensible to show that this is the case. Am happy to
> >>> change
> >>> to lower-case to follow convention.
> >> Hmm. It's a bit of an oddity as the point of the naming
> >> is about the uses, not which pins they are on. If we exposed the
> >> 'datasheet_name' parameter directly rather than just using it internally
> >> I'd suggest relying on that - but clearly you want it to be apparent
> >> in the interface. Whether that is useful is the question I'd raise
> >> here (and is the reason datasheet_name is not exposed.
> >>
> >> The obvious question is does userspace care? Answer is probably not.
> >>
> >> It cares what is being measured but this is about what pins it is
> >> on and doesn't provide any information on what is connected to them.
> >>
> >
> > Surely it helps when using sysfs to access whatever is connected to one of
> > those pins if we label the pin with something meaningful? If say you have a
> > device connected to GPIC of the charger IC, it's easier to work out which
> > ADC
> > channel you need to access through sysfs if the naming is as I have now,
> > rather
> > than some arbitrary number which doesn't necessarily tally to the channel
> > in the
> > datasheet. You'd then need to look at the code and work out which channel
> number
> > GPIOC actually was. Or am I just missing something here? :)
> Not really for the vast majority of users. They tend not to have a detailed
> board layout in front of them. It's more interesting if you know 'what' they
> are measuring (hence we do use these names when that is true - such as
> internal voltage measurements).
>
> The numbers almost never tally with the datasheet, but then datasheet
> numbering
> has a habit of being inconsistent as well!
Can't say I agree that this won't be useful/informative to many users, but don't
want to make this a sticking point so will update.
RE: [PATCH v5 1/7] mfd: Add support for DA9150 combined charger & fuel-gauge device
On January 19, 2015 11:34, Lee Jones wrote:
> On Mon, 22 Dec 2014, Adam Thomson wrote:
>
> > DA9150 is a combined Charger and Fuel-Gauge IC, with additional
> > GPIO and GPADC functionality.
> >
> > Signed-off-by: Adam Thomson
> > ---
> > drivers/mfd/Kconfig | 12 +
> > drivers/mfd/Makefile |2 +-
> > drivers/mfd/da9150-core.c| 413
> > include/linux/mfd/da9150/core.h | 68 ++
> > include/linux/mfd/da9150/registers.h | 1155
> ++
> > 5 files changed, 1649 insertions(+), 1 deletion(-)
> > create mode 100644 drivers/mfd/da9150-core.c
> > create mode 100644 include/linux/mfd/da9150/core.h
> > create mode 100644 include/linux/mfd/da9150/registers.h
> >
> > diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> > index 2e6b731..56e80d2 100644
> > --- a/drivers/mfd/Kconfig
> > +++ b/drivers/mfd/Kconfig
> > @@ -195,6 +195,18 @@ config MFD_DA9063
> > Additional drivers must be enabled in order to use the functionality
> > of the device.
> >
> > +config MFD_DA9150
> > + tristate "Dialog Semiconductor DA9150 Charger Fuel-Gauge chip"
> > + depends on I2C=y
> > + select MFD_CORE
> > + select REGMAP_I2C
> > + select REGMAP_IRQ
> > + help
> > + This adds support for the DA9150 integrated charger and fuel-gauge
> > + chip. This driver provides common support for accessing the device.
> > + Additional drivers must be enabled in order to use the specific
> > + features of the device.
> > +
> > config MFD_DLN2
> > tristate "Diolan DLN2 support"
> > select MFD_CORE
> > diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> > index 53467e2..f06f4c6 100644
> > --- a/drivers/mfd/Makefile
> > +++ b/drivers/mfd/Makefile
> > @@ -113,7 +113,7 @@ obj-$(CONFIG_MFD_DA9055)+= da9055.o
> >
> > da9063-objs:= da9063-core.o da9063-irq.o
> > da9063-i2c.o
> > obj-$(CONFIG_MFD_DA9063) += da9063.o
> > -
> > +obj-$(CONFIG_MFD_DA9150) += da9150-core.o
> > obj-$(CONFIG_MFD_MAX14577) += max14577.o
> > obj-$(CONFIG_MFD_MAX77686) += max77686.o
> > obj-$(CONFIG_MFD_MAX77693) += max77693.o
> > diff --git a/drivers/mfd/da9150-core.c b/drivers/mfd/da9150-core.c
> > new file mode 100644
> > index 000..4d757b9
> > --- /dev/null
> > +++ b/drivers/mfd/da9150-core.c
> > @@ -0,0 +1,413 @@
> > +/*
> > + * DA9150 Core MFD Driver
> > + *
> > + * Copyright (c) 2014 Dialog Semiconductor
> > + *
> > + * Author: Adam Thomson
> > + *
> > + * This program is free software; you can redistribute it and/or modify it
> > + * under the terms of the GNU General Public License as published by the
> > + * Free Software Foundation; either version 2 of the License, or (at your
> > + * option) any later version.
> > + */
>
> [...]
>
> > +u8 da9150_reg_read(struct da9150 *da9150, u16 reg)
> > +{
> > + int val, ret;
> > +
> > + ret = regmap_read(da9150->regmap, reg, &val);
> > + if (ret)
> > + dev_err(da9150->dev, "Failed to read from reg 0x%x: %d\n",
> > + reg, ret);
> > +
> > + return (u8) val;
> > +}
> > +EXPORT_SYMBOL_GPL(da9150_reg_read);
> > +
> > +void da9150_reg_write(struct da9150 *da9150, u16 reg, u8 val)
> > +{
> > + int ret;
> > +
> > + ret = regmap_write(da9150->regmap, reg, val);
> > + if (ret)
> > + dev_err(da9150->dev, "Failed to write to reg 0x%x: %d\n",
> > + reg, ret);
> > +}
> > +EXPORT_SYMBOL_GPL(da9150_reg_write);
> > +
> > +void da9150_set_bits(struct da9150 *da9150, u16 reg, u8 mask, u8 val)
> > +{
> > + int ret;
> > +
> > + ret = regmap_update_bits(da9150->regmap, reg, mask, val);
> > + if (ret)
> > + dev_err(da9150->dev, "Failed to set bits in reg 0x%x: %d\n",
> > + reg, ret);
> > +}
> > +EXPORT_SYMBOL_GPL(da9150_set_bits);
> > +
> > +void da9150_bulk_read(struct da9150 *da9150, u16 reg, int count, u8 *buf)
> > +{
> > + int ret;
> > +
> > + ret = regmap_bulk_read(da9150->regmap, reg, buf, count);
> > + if (ret)
> > + dev_err(da9150->dev, "Failed to bulk read from reg 0x%x: %d\n",
> > + reg, ret);
> > +}
> > +EXPORT_SYMBOL_GPL(da9150_bulk_read);
> > +
> > +void da9150_bulk_write(struct da9150 *da9150, u16 reg, int count, const u8
> *buf)
> > +{
> > + int ret;
> > +
> > + ret = regmap_raw_write(da9150->regmap, reg, buf, count);
> > + if (ret)
> > + dev_err(da9150->dev, "Failed to bulk write to reg 0x%x %d\n",
> > + reg, ret);
> > +}
> > +EXPORT_SYMBOL_GPL(da9150_bulk_write);
>
> I've never been a fan of this type of aggregation. Can you explain to
> me what the point of them is?
I refer you back to our previous discussion some time back:
https://lkml.org/lkml/2014/9/9/284
'The reason for these is because future patches to add additional functionality
will introduce I2C access functions which do not use regmap and access the
device via a separate I2C address for this
RE: [PATCH v5 3/7] iio: Add support for DA9150 GPADC
On January 10, 2015 22:19, Jonathan Cameron wrote:
> On 07/01/15 16:03, Opensource [Adam Thomson] wrote:
> > On January 4, 2015 17:22, Jonathan Cameron wrote:
> >
> >> On 22/12/14 16:51, Adam Thomson wrote:
> >>> This patch adds support for DA9150 Charger & Fuel-Gauge IC GPADC.
> >>>
> >>> Signed-off-by: Adam Thomson
> >> One last query from me.
> >>
> >> Using the extended channel names in IIO is only really appropriate when
> >> they don't correspond to simple pins on the side of the chip. For those
> >> just drop the extname bit. Some of the channels you have here, definitely
> >> need them though.
> >>
> >> Drop those first 4 or convince me otherwise and add
> >> Acked-by: Jonathan Cameron
> >
> > Have added responses below. If the comments are accepted then I'll respin
> > and add you're 'Acked-by'. Is that ok?
> >
> Not accepted as yet :)
Yeah, that's why I added the 'if'. :)
> >>> +#define DA9150_GPADC_CHANNEL_PROCESSED(_id, _hw_id, _type,
> _ext_name)
> >>\
> >>> + DA9150_GPADC_CHANNEL(_id, _hw_id, _type,\
> >>> + BIT(IIO_CHAN_INFO_PROCESSED), _ext_name)
> >>> +
> >>> +/* Supported channels */
> >>> +static const struct iio_chan_spec da9150_gpadc_channels[] = {
> >>> + DA9150_GPADC_CHANNEL_PROCESSED(GPIOA, GPIOA_6V, IIO_VOLTAGE,
> >> "GPIOA"),
> >>> + DA9150_GPADC_CHANNEL_PROCESSED(GPIOB, GPIOB_6V, IIO_VOLTAGE,
> >> "GPIOB"),
> >>> + DA9150_GPADC_CHANNEL_PROCESSED(GPIOC, GPIOC_6V, IIO_VOLTAGE,
> >> "GPIOC"),
> >>> + DA9150_GPADC_CHANNEL_PROCESSED(GPIOD, GPIOD_6V, IIO_VOLTAGE,
> >> "GPIOD"),
> >> I'm not sure some of these really deserve extended names. Those are
> >> usually
> >> reserved for naming strange internal adc channels etc. These first 4 are
> >> presumably just for input pins? Those should just be channels 0..3
> >> On another note, unless you want really weird sysfs attribute names, the
> >> extended names want to be lowercase.
> >>
> >
> > I'd prefer to keep the names because the input pins are muxed with GPIOs of
> > the
> > chip, so thought it sensible to show that this is the case. Am happy to
> > change
> > to lower-case to follow convention.
> Hmm. It's a bit of an oddity as the point of the naming
> is about the uses, not which pins they are on. If we exposed the
> 'datasheet_name' parameter directly rather than just using it internally
> I'd suggest relying on that - but clearly you want it to be apparent
> in the interface. Whether that is useful is the question I'd raise
> here (and is the reason datasheet_name is not exposed.
>
> The obvious question is does userspace care? Answer is probably not.
>
> It cares what is being measured but this is about what pins it is
> on and doesn't provide any information on what is connected to them.
>
Surely it helps when using sysfs to access whatever is connected to one of
those pins if we label the pin with something meaningful? If say you have a
device connected to GPIC of the charger IC, it's easier to work out which ADC
channel you need to access through sysfs if the naming is as I have now, rather
than some arbitrary number which doesn't necessarily tally to the channel in the
datasheet. You'd then need to look at the code and work out which channel number
GPIOC actually was. Or am I just missing something here? :)
>
> >
> >>> + DA9150_GPADC_CHANNEL_PROCESSED(IBUS, IBUS_SENSE, IIO_CURRENT,
> >> "IBUS"),
> >>> + DA9150_GPADC_CHANNEL_PROCESSED(VBUS, VBUS_DIV_, IIO_VOLTAGE,
> >> "VBUS"),
> >>> + DA9150_GPADC_CHANNEL_RAW(ID, ID, IIO_VOLTAGE, "ID"),
> >> You hae an identifier voltage? That's certainly unusual but if so - fair
> >> enough
> >> and it defintely needs the extname!
> >
> > Thanks for pointing that out. Having checked again, this is not needed and
> > can
> > be dispensed with.
> >
> >>> + DA9150_GPADC_CHANNEL_PROCESSED(VSYS, VSYS, IIO_VOLTAGE, "VSYS"),
> >>> + DA9150_GPADC_CHANNEL_SCALED(VBAT, VBAT, IIO_VOLTAGE, "VBAT"),
> >>> + DA9150_GPADC_CHANNEL_RAW(TBAT, TBAT, IIO_VOLTAGE, "TBAT"),
> >>> + DA9150_GPADC_CHANNEL_SCALED(TJUNC_CORE, TJUNC_CORE,
> >> IIO_TEMP,
> >>> + &q
RE: [PATCH v5 5/7] power: Add support for DA9150 Charger
On January 4, 2015 17:26, Jonathan Cameron wrote:
> On 22/12/14 16:51, Adam Thomson wrote:
> > This patch adds support for DA9150 Charger & Fuel-Gauge IC Charger.
> >
> > Signed-off-by: Adam Thomson
> The IIO bits look fine, but your use of devm_free * doesn't...
>
> As a side note, looks like we could benefit from some array versions
> of the IIO channel getting and release functions. Would save a fair bit
> of boiler plate.
Yes, I guess when more drivers come along which need multiple channels then
something like you suggest would make sense to help reduce code duplication.
Also devm_* functions could be good too.
>
> Jonathan
> > ---
> > drivers/power/Kconfig | 12 +
> > drivers/power/Makefile | 1 +
> > drivers/power/da9150-charger.c | 666
> +
> > 3 files changed, 679 insertions(+)
> > create mode 100644 drivers/power/da9150-charger.c
> >
> > diff --git a/drivers/power/Kconfig b/drivers/power/Kconfig
> > index 0108c2a..eb79a7a 100644
> > --- a/drivers/power/Kconfig
> > +++ b/drivers/power/Kconfig
> > @@ -192,6 +192,18 @@ config BATTERY_DA9052
> > Say Y here to enable support for batteries charger integrated into
> > DA9052 PMIC.
> >
> > +config CHARGER_DA9150
> > + tristate "Dialog Semiconductor DA9150 Charger support"
> > + depends on MFD_DA9150
> > + depends on DA9150_GPADC
> > + depends on IIO
> > + help
> > + Say Y here to enable support for charger unit of the DA9150
> > + Integrated Charger & Fuel-Gauge IC.
> > +
> > + This driver can also be built as a module. If so, the module will be
> > + called da9150-charger.
> > +
> > config BATTERY_MAX17040
> > tristate "Maxim MAX17040 Fuel Gauge"
> > depends on I2C
> > diff --git a/drivers/power/Makefile b/drivers/power/Makefile
> > index dfa8942..0c1896d 100644
> > --- a/drivers/power/Makefile
> > +++ b/drivers/power/Makefile
> > @@ -31,6 +31,7 @@ obj-$(CONFIG_BATTERY_SBS) += sbs-battery.o
> > obj-$(CONFIG_BATTERY_BQ27x00) += bq27x00_battery.o
> > obj-$(CONFIG_BATTERY_DA9030) += da9030_battery.o
> > obj-$(CONFIG_BATTERY_DA9052) += da9052-battery.o
> > +obj-$(CONFIG_CHARGER_DA9150) += da9150-charger.o
> > obj-$(CONFIG_BATTERY_MAX17040) += max17040_battery.o
> > obj-$(CONFIG_BATTERY_MAX17042) += max17042_battery.o
> > obj-$(CONFIG_BATTERY_Z2) += z2_battery.o
> > diff --git a/drivers/power/da9150-charger.c b/drivers/power/da9150-charger.c
> > new file mode 100644
> > index 000..9b1826d
> > --- /dev/null
> > +++ b/drivers/power/da9150-charger.c
> > @@ -0,0 +1,666 @@
> > +/*
> > + * DA9150 Charger Driver
> > + *
> > + * Copyright (c) 2014 Dialog Semiconductor
> > + *
> > + * Author: Adam Thomson
> > + *
> > + * This program is free software; you can redistribute it and/or modify it
> > + * under the terms of the GNU General Public License as published by the
> > + * Free Software Foundation; either version 2 of the License, or (at your
> > + * option) any later version.
> > + */
> > +
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +
> > +/* Private data */
> > +struct da9150_charger {
> > + struct da9150 *da9150;
> > + struct device *dev;
> > +
> > + struct power_supply usb;
> > + struct power_supply battery;
> > + struct power_supply *supply_online;
> > +
> > + struct usb_phy *usb_phy;
> > + struct notifier_block otg_nb;
> > + struct work_struct otg_work;
> > + unsigned long usb_event;
> > +
> > + struct iio_channel *ibus_chan;
> > + struct iio_channel *vbus_chan;
> > + struct iio_channel *tjunc_chan;
> > + struct iio_channel *vbat_chan;
> > +};
> > +
> > +static inline int da9150_charger_supply_online(struct da9150_charger
> > *charger,
> > + struct power_supply *psy,
> > + union power_supply_propval *val)
> > +{
> > + val->intval = (psy == charger->supply_online) ? 1 : 0;
> > +
> > + return 0;
> > +}
> > +
> > +/* Charger Properties */
> > +static int da9150_charger_vbus_voltage_now(struct da9150_charger *charger,
> > + union power_supply_propval *val)
> > +{
> > + int v_val, ret;
> > +
> > + /* Read processed value - mV units */
> > + ret = iio_read_channel_processed(charger->vbus_chan, &v_val);
> > + if (ret < 0)
> > + return ret;
> > +
> > + /* Convert voltage to expected uV units */
> > + val->intval = v_val * 1000;
> > +
> > + return 0;
> > +}
> > +
> > +static int da9150_charger_ibus_current_avg(struct da9150_charger *charger,
> > + union power_supply_propval *val)
> > +{
> > + int i_val, ret;
> > +
> > + /* Read processed value - mA units */
> > + ret = iio_read_channel_processed(charger->ibus_chan, &i_val);
> > + if (ret
RE: [PATCH v5 3/7] iio: Add support for DA9150 GPADC
On January 4, 2015 17:22, Jonathan Cameron wrote:
> On 22/12/14 16:51, Adam Thomson wrote:
> > This patch adds support for DA9150 Charger & Fuel-Gauge IC GPADC.
> >
> > Signed-off-by: Adam Thomson
> One last query from me.
>
> Using the extended channel names in IIO is only really appropriate when
> they don't correspond to simple pins on the side of the chip. For those
> just drop the extname bit. Some of the channels you have here, definitely
> need them though.
>
> Drop those first 4 or convince me otherwise and add
> Acked-by: Jonathan Cameron
Have added responses below. If the comments are accepted then I'll respin
and add you're 'Acked-by'. Is that ok?
> > ---
> > drivers/iio/adc/Kconfig| 9 +
> > drivers/iio/adc/Makefile | 1 +
> > drivers/iio/adc/da9150-gpadc.c | 409
> +
> > 3 files changed, 419 insertions(+)
> > create mode 100644 drivers/iio/adc/da9150-gpadc.c
> >
> > diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> > index 0f79e47..6e00b81 100644
> > --- a/drivers/iio/adc/Kconfig
> > +++ b/drivers/iio/adc/Kconfig
> > @@ -135,6 +135,15 @@ config AXP288_ADC
> > device. Depending on platform configuration, this general purpose ADC
> > can
> > be used for sampling sensors such as thermal resistors.
> >
> > +config DA9150_GPADC
> > + tristate "Dialog DA9150 GPADC driver support"
> > + depends on MFD_DA9150
> > + help
> > + Say yes here to build support for Dialog DA9150 GPADC.
> > +
> > + This driver can also be built as a module. If chosen, the module name
> > + will be da9150-gpadc.
> > +
> > config EXYNOS_ADC
> > tristate "Exynos ADC driver support"
> > depends on ARCH_EXYNOS || ARCH_S3C24XX || ARCH_S3C64XX || (OF &&
> COMPILE_TEST)
> > diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
> > index 701fdb7..d56ee9b 100644
> > --- a/drivers/iio/adc/Makefile
> > +++ b/drivers/iio/adc/Makefile
> > @@ -15,6 +15,7 @@ obj-$(CONFIG_AD7887) += ad7887.o
> > obj-$(CONFIG_AD799X) += ad799x.o
> > obj-$(CONFIG_AT91_ADC) += at91_adc.o
> > obj-$(CONFIG_AXP288_ADC) += axp288_adc.o
> > +obj-$(CONFIG_DA9150_GPADC) += da9150-gpadc.o
> > obj-$(CONFIG_EXYNOS_ADC) += exynos_adc.o
> > obj-$(CONFIG_LP8788_ADC) += lp8788_adc.o
> > obj-$(CONFIG_MAX1027) += max1027.o
> > diff --git a/drivers/iio/adc/da9150-gpadc.c b/drivers/iio/adc/da9150-gpadc.c
> > new file mode 100644
> > index 000..3f7bc81
> > --- /dev/null
> > +++ b/drivers/iio/adc/da9150-gpadc.c
> > @@ -0,0 +1,409 @@
> > +/*
> > + * DA9150 GPADC Driver
> > + *
> > + * Copyright (c) 2014 Dialog Semiconductor
> > + *
> > + * Author: Adam Thomson
> > + *
> > + * This program is free software; you can redistribute it and/or modify it
> > + * under the terms of the GNU General Public License as published by the
> > + * Free Software Foundation; either version 2 of the License, or (at your
> > + * option) any later version.
> > + */
> > +
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +
> > +/* Channels */
> > +enum da9150_gpadc_hw_channel {
> > + DA9150_GPADC_HW_CHAN_GPIOA_2V = 0,
> > + DA9150_GPADC_HW_CHAN_GPIOA_2V_,
> > + DA9150_GPADC_HW_CHAN_GPIOB_2V,
> > + DA9150_GPADC_HW_CHAN_GPIOB_2V_,
> > + DA9150_GPADC_HW_CHAN_GPIOC_2V,
> > + DA9150_GPADC_HW_CHAN_GPIOC_2V_,
> > + DA9150_GPADC_HW_CHAN_GPIOD_2V,
> > + DA9150_GPADC_HW_CHAN_GPIOD_2V_,
> > + DA9150_GPADC_HW_CHAN_IBUS_SENSE,
> > + DA9150_GPADC_HW_CHAN_IBUS_SENSE_,
> > + DA9150_GPADC_HW_CHAN_VBUS_DIV,
> > + DA9150_GPADC_HW_CHAN_VBUS_DIV_,
> > + DA9150_GPADC_HW_CHAN_ID,
> > + DA9150_GPADC_HW_CHAN_ID_,
> > + DA9150_GPADC_HW_CHAN_VSYS,
> > + DA9150_GPADC_HW_CHAN_VSYS_,
> > + DA9150_GPADC_HW_CHAN_GPIOA_6V,
> > + DA9150_GPADC_HW_CHAN_GPIOA_6V_,
> > + DA9150_GPADC_HW_CHAN_GPIOB_6V,
> > + DA9150_GPADC_HW_CHAN_GPIOB_6V_,
> > + DA9150_GPADC_HW_CHAN_GPIOC_6V,
> > + DA9150_GPADC_HW_CHAN_GPIOC_6V_,
> > + DA9150_GPADC_HW_CHAN_GPIOD_6V,
> > + DA9150_GPADC_HW_CHAN_GPIOD_6V_,
> > + DA9150_GPADC_HW_CHAN_VBAT,
> > + DA9150_GPADC_HW_CHAN_VBAT_,
> > + DA9150_GPADC_HW_CHAN_TBAT,
> > + DA9150_GPADC_HW_CHAN_TBAT_,
> > + DA9150_GPADC_HW_CHAN_TJUNC_CORE,
> > + DA9150_GPADC_HW_CHAN_TJUNC_CORE_,
> > + DA9150_GPADC_HW_CHAN_TJUNC_OVP,
> > + DA9150_GPADC_HW_CHAN_TJUNC_OVP_,
> > +};
> > +
> > +enum da9150_gpadc_channel {
> > + DA9150_GPADC_CHAN_GPIOA = 0,
> > + DA9150_GPADC_CHAN_GPIOB,
> > + DA9150_GPADC_CHAN_GPIOC,
> > + DA9150_GPADC_CHAN_GPIOD,
> > + DA9150_GPADC_CHAN_IBUS,
> > + DA9150_GPADC_CHAN_VBUS,
> > + DA9150_GPADC_CHAN_ID,
> > + DA9150_GPADC_CHAN_VSYS,
> > + DA9150_GPADC_CHAN_VBAT,
> > + DA9150_GPADC_CHAN_TBAT,
> > + DA9150_GPADC_CHAN_TJUNC_CORE,
> > + DA9150_GPADC_CHAN_TJUNC_OVP,
> > +};
> > +
> > +/* Private data */
> > +struct da9150_gpadc {
> > + struct da9150 *da9
RE: [PATCH v4 8/8] iio: Add ABI documentation for input current readings
On December 12, 2014 11:46, Jonathan Cameron wrote: > On 25/11/14 18:25, Adam Thomson wrote: > > Add information on in_current related readings. > > > > Signed-off-by: Adam Thomson > This one stands on it's own to applied (with some fuzz and tiny > bit of rearranging) to the togreg branch of iio.git - initially pushed > out as testing for the autobuilders to play with it. > > Thanks, > > Jonathan Ok great. Thanks.
RE: [PATCH v4 3/8] iio: Add support for DA9150 GPADC
On December 04, 2014 23:02, Hartmut Knaack wrote:
> > + indio_dev = devm_iio_device_alloc(&pdev->dev,
> > + sizeof(struct da9150_gpadc));
> BTW: If you use sizeof(*gpadc) instead (like it is usually done as well),
> it will perfectly fit in one line.
I preferred to be explicit that the item is a struct, but really not that fussed
either way. Also have been asked previously in a separate driver to make the
same mod you mention so will update accordingly.
> > + ret = devm_request_threaded_irq(dev, irq, NULL, da9150_gpadc_irq,
> > + IRQF_ONESHOT, "GPADC", gpadc);
> > + if (ret) {
> > + dev_err(dev, "Failed to request IRQ %d: %d\n", irq, ret);
> > + goto iio_map_unreg;
> You need to return ret here, no jump to error out path.
Yes, thanks. Annoyed I missed that in the re-factor. Will fix it.
RE: [PATCH 3/8] iio: Add support for DA9150 GPADC
On October 11, 2014 23:48, Hartmut Knaack wrote:
> Hi,
> I have put a few comments inline.
>
> Adam Thomson schrieb am 23.09.2014 12:53:
> > This patch adds support for DA9150 Charger & Fuel-Gauge IC GPADC.
> >
> > Signed-off-by: Adam Thomson
> > ---
> > drivers/iio/adc/Kconfig| 9 +
> > drivers/iio/adc/Makefile | 1 +
> > drivers/iio/adc/da9150-gpadc.c | 406
> +
> > 3 files changed, 416 insertions(+)
> > create mode 100644 drivers/iio/adc/da9150-gpadc.c
> >
> > diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> > index 11b048a..8041347 100644
> > --- a/drivers/iio/adc/Kconfig
> > +++ b/drivers/iio/adc/Kconfig
> > @@ -127,6 +127,15 @@ config AT91_ADC
> > help
> > Say yes here to build support for Atmel AT91 ADC.
> >
> > +config DA9150_GPADC
> > + tristate "Dialog DA9150 GPADC driver support"
> > + depends on MFD_DA9150
> > + help
> > + Say yes here to build support for Dialog DA9150 GPADC.
> > +
> > + This driver can also be built as a module. If chosen, the module name
> > + will be da9150-gpadc.
> > +
> > config EXYNOS_ADC
> > tristate "Exynos ADC driver support"
> > depends on ARCH_EXYNOS || (OF && COMPILE_TEST)
> > diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
> > index ad81b51..48413d2 100644
> > --- a/drivers/iio/adc/Makefile
> > +++ b/drivers/iio/adc/Makefile
> > @@ -14,6 +14,7 @@ obj-$(CONFIG_AD7793) += ad7793.o
> > obj-$(CONFIG_AD7887) += ad7887.o
> > obj-$(CONFIG_AD799X) += ad799x.o
> > obj-$(CONFIG_AT91_ADC) += at91_adc.o
> > +obj-$(CONFIG_DA9150_GPADC) += da9150-gpadc.o
> > obj-$(CONFIG_EXYNOS_ADC) += exynos_adc.o
> > obj-$(CONFIG_LP8788_ADC) += lp8788_adc.o
> > obj-$(CONFIG_MAX1027) += max1027.o
> > diff --git a/drivers/iio/adc/da9150-gpadc.c b/drivers/iio/adc/da9150-gpadc.c
> > new file mode 100644
> > index 000..2b83ee0
> > --- /dev/null
> > +++ b/drivers/iio/adc/da9150-gpadc.c
> > @@ -0,0 +1,406 @@
> > +/*
> > + * DA9150 GPADC Driver
> > + *
> > + * Copyright (c) 2014 Dialog Semiconductor
> > + *
> > + * Author: Adam Thomson
> > + *
> > + * This program is free software; you can redistribute it and/or modify it
> > + * under the terms of the GNU General Public License as published by the
> > + * Free Software Foundation; either version 2 of the License, or (at your
> > + * option) any later version.
> > + */
> > +
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +
> > +/* Channels */
> > +enum da9150_gpadc_hw_channel {
> > + DA9150_GPADC_HW_CHAN_GPIOA_2V = 0,
> > + DA9150_GPADC_HW_CHAN_GPIOA_2V_,
> > + DA9150_GPADC_HW_CHAN_GPIOB_2V,
> > + DA9150_GPADC_HW_CHAN_GPIOB_2V_,
> > + DA9150_GPADC_HW_CHAN_GPIOC_2V,
> > + DA9150_GPADC_HW_CHAN_GPIOC_2V_,
> > + DA9150_GPADC_HW_CHAN_GPIOD_2V,
> > + DA9150_GPADC_HW_CHAN_GPIOD_2V_,
> > + DA9150_GPADC_HW_CHAN_IBUS_SENSE,
> > + DA9150_GPADC_HW_CHAN_IBUS_SENSE_,
> > + DA9150_GPADC_HW_CHAN_VBUS_DIV,
> > + DA9150_GPADC_HW_CHAN_VBUS_DIV_,
> > + DA9150_GPADC_HW_CHAN_ID,
> > + DA9150_GPADC_HW_CHAN_ID_,
> > + DA9150_GPADC_HW_CHAN_VSYS,
> > + DA9150_GPADC_HW_CHAN_VSYS_,
> > + DA9150_GPADC_HW_CHAN_GPIOA_6V,
> > + DA9150_GPADC_HW_CHAN_GPIOA_6V_,
> > + DA9150_GPADC_HW_CHAN_GPIOB_6V,
> > + DA9150_GPADC_HW_CHAN_GPIOB_6V_,
> > + DA9150_GPADC_HW_CHAN_GPIOC_6V,
> > + DA9150_GPADC_HW_CHAN_GPIOC_6V_,
> > + DA9150_GPADC_HW_CHAN_GPIOD_6V,
> > + DA9150_GPADC_HW_CHAN_GPIOD_6V_,
> > + DA9150_GPADC_HW_CHAN_VBAT,
> > + DA9150_GPADC_HW_CHAN_VBAT_,
> > + DA9150_GPADC_HW_CHAN_TBAT,
> > + DA9150_GPADC_HW_CHAN_TBAT_,
> > + DA9150_GPADC_HW_CHAN_TJUNC_CORE,
> > + DA9150_GPADC_HW_CHAN_TJUNC_CORE_,
> > + DA9150_GPADC_HW_CHAN_TJUNC_OVP,
> > + DA9150_GPADC_HW_CHAN_TJUNC_OVP_,
> > +};
> > +
> > +enum da9150_gpadc_channel {
> > + DA9150_GPADC_CHAN_GPIOA = 0,
> > + DA9150_GPADC_CHAN_GPIOB,
> > + DA9150_GPADC_CHAN_GPIOC,
> > + DA9150_GPADC_CHAN_GPIOD,
> > + DA9150_GPADC_CHAN_IBUS,
> > + DA9150_GPADC_CHAN_VBUS,
> > + DA9150_GPADC_CHAN_ID,
> > + DA9150_GPADC_CHAN_VSYS,
> > + DA9150_GPADC_CHAN_VBAT,
> > + DA9150_GPADC_CHAN_TBAT,
> > + DA9150_GPADC_CHAN_TJUNC_CORE,
> > + DA9150_GPADC_CHAN_TJUNC_OVP,
> > +};
> > +
> > +/* Private data */
> > +struct da9150_gpadc {
> > + struct da9150 *da9150;
> > + struct device *dev;
> > +
> > + struct mutex lock;
> > + struct completion complete;
> > +};
> > +
> > +
> > +static irqreturn_t da9150_gpadc_irq(int irq, void *data)
> > +{
> > +
> > + struct da9150_gpadc *gpadc = data;
> > +
> > + complete(&gpadc->complete);
> > +
> > + return IRQ_HANDLED;
> > +}
> > +
> > +static int da9150_gpadc_read_adc(struct da9150_gpadc *gpadc, int hw_chan)
> > +{
> > + u8 result_regs[2];
> > + int result;
> > +
> > + mutex_lock(&gpadc->lock);
> > +
> > + /* Set channel & enable measurement */
RE: [PATCH 3/8] iio: Add support for DA9150 GPADC
On October 7, 2014 20:37, Jonathan Cameron wrote:
> On October 7, 2014 3:55:55 PM GMT+01:00, "Opensource [Adam Thomson]"
> wrote:
> >On September 27, 2014 11:50, Jonathan Cameron wrote:
> >
> >> On 23/09/14 11:53, Adam Thomson wrote:
> >> > This patch adds support for DA9150 Charger & Fuel-Gauge IC GPADC.
> >
> >> > +
> >> > +static inline int da9150_gpadc_gpio_6v_voltage_now(int raw_val)
> >> > +{
> >> > +/* Convert to mV */
> >> > +return (6 * ((raw_val * 1000) + 500)) / 1024;
> >> These could all be expressed as raw values with offsets
> >> and scales (and that would be preferred).
> >> E.g. This one has offset 50 and scale 6000/1024 or even
> >> better use IIO_VAL_FRACTIONAL_LOG2 for scale with val1 = 6000
> >> and val2 = (log_2 1024) = 10.
> >>
> >
> >What you've suggested isn't correct. The problem here is that the
> >offset is
> >added first to the raw ADC reading, without factoring the ADC value
> >accordingly
> >to match the factor of the offset. If we take the original equation
> >provided for
> >this channel of the ADC, the offset is actually 0.5 which should be
> >added to the
> >raw ADC value. This doesn't fit into the implementation in the kernel
> >as we
> >can't use floating point. If we multiply the offset but not the raw ADC
> >value,
> >then add them before applying the scale factor, then the result is
> >wrong at the
> >end. Basically you need a scale for the raw ADC value to match the
> >offset scale
> >so you can achieve the correct results, which is what my calculation
> >does.
> >But that seems impossible with the current raw|offset|scale method.
> Oops got that wrong. The fixed point maths to fix the in kernel interface
> isn't exactly
> difficult but indeed it does not handle this currently.
I did have a quick look when I had a spare moment, and I guess you could do
something like having an offset scale/factor which can be applied to the raw
reading, prior to adding the offset. May be an option but I think this would
also have to be exposed to user-space as I believe the same problem would reside
there as well.
> >
> >> > +ret = iio_map_array_register(indio_dev,
> >da9150_gpadc_default_maps);
> >> > +if (ret) {
> >> > +dev_err(dev, "Failed to register IIO maps: %d\n", ret);
> >> > +return ret;
> >> > +}
> >> I'd suggest doing the devm_request_thread_irq before the
> >iio_map_array
> >> stuff. This is purely to avoid the order during remove not being
> >> obviously correct as it isn't the reverse of during probe.
> >
> >Ok, should still work ok that way so can update.
> >
> >> > +static int da9150_gpadc_remove(struct platform_device *pdev)
> >> > +{
> >> > +struct iio_dev *indio_dev = platform_get_drvdata(pdev);
> >> > +
> >> > +iio_map_array_unregister(indio_dev);
> >> Twice in one day. I'm definitely thinking we should add a
> >> devm version of iio_map_array_register...
> >
> >I assume you mean here that iio_device_unregister() should come first?
> >Will
> >update.
> Nope just that such a new function might be useful.
:) Ok fair enough.
>
> --
> Sent from my Android phone with K-9 Mail. Please excuse my brevity.
RE: [PATCH 3/8] iio: Add support for DA9150 GPADC
On September 27, 2014 11:50, Jonathan Cameron wrote:
> On 23/09/14 11:53, Adam Thomson wrote:
> > This patch adds support for DA9150 Charger & Fuel-Gauge IC GPADC.
> > +
> > +static inline int da9150_gpadc_gpio_6v_voltage_now(int raw_val)
> > +{
> > + /* Convert to mV */
> > + return (6 * ((raw_val * 1000) + 500)) / 1024;
> These could all be expressed as raw values with offsets
> and scales (and that would be preferred).
> E.g. This one has offset 50 and scale 6000/1024 or even
> better use IIO_VAL_FRACTIONAL_LOG2 for scale with val1 = 6000
> and val2 = (log_2 1024) = 10.
>
What you've suggested isn't correct. The problem here is that the offset is
added first to the raw ADC reading, without factoring the ADC value accordingly
to match the factor of the offset. If we take the original equation provided for
this channel of the ADC, the offset is actually 0.5 which should be added to the
raw ADC value. This doesn't fit into the implementation in the kernel as we
can't use floating point. If we multiply the offset but not the raw ADC value,
then add them before applying the scale factor, then the result is wrong at the
end. Basically you need a scale for the raw ADC value to match the offset scale
so you can achieve the correct results, which is what my calculation does.
But that seems impossible with the current raw|offset|scale method.
> > + ret = iio_map_array_register(indio_dev, da9150_gpadc_default_maps);
> > + if (ret) {
> > + dev_err(dev, "Failed to register IIO maps: %d\n", ret);
> > + return ret;
> > + }
> I'd suggest doing the devm_request_thread_irq before the iio_map_array
> stuff. This is purely to avoid the order during remove not being
> obviously correct as it isn't the reverse of during probe.
Ok, should still work ok that way so can update.
> > +static int da9150_gpadc_remove(struct platform_device *pdev)
> > +{
> > + struct iio_dev *indio_dev = platform_get_drvdata(pdev);
> > +
> > + iio_map_array_unregister(indio_dev);
> Twice in one day. I'm definitely thinking we should add a
> devm version of iio_map_array_register...
I assume you mean here that iio_device_unregister() should come first? Will
update.
RE: [PATCH 2/8] mfd: da9150: Add DT binding documentation for core
On September 27, 2014 11:37, Jonathan Cameron wrote:
> On 23/09/14 11:53, Adam Thomson wrote:
> > Signed-off-by: Adam Thomson
> Obviously this really wants a review from one of the device tree guys, but I
> have a few
> bits based on what Mark has recently said in other reviews ;)
> > ---
> > Documentation/devicetree/bindings/mfd/da9150.txt | 41
>
> > 1 file changed, 41 insertions(+)
> > create mode 100644 Documentation/devicetree/bindings/mfd/da9150.txt
> >
> > diff --git a/Documentation/devicetree/bindings/mfd/da9150.txt
> b/Documentation/devicetree/bindings/mfd/da9150.txt
> > new file mode 100644
> > index 000..d7de150
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/mfd/da9150.txt
> > @@ -0,0 +1,41 @@
> > +Dialog Semiconductor DA9150 Combined Charger/Fuel-Gauge MFD bindings
> > +
> > +DA9150 consists of a group of sub-devices (I2C Only):
> What does I2C only add to the description?
Nothing really. Will remove.
> > +
> > +Device Description
> > +-- ---
> > +da9150-gpadc : IIO - GPADC
> Given usual aversion to anything driver specific in the device tree
> description
> you probably
> just want to describe what they do rather than what subsystem provides the
> driver.
Ok, can update accordingly.
>
> > +da9150-charger : Power Supply (Charger)
> > +
> > +==
> > +
> > +Required properties:
> > +- compatible : Should be "dlg,da9150"
> > +- reg: Specifies the I2C slave address
> > +- interrupt-parent: Specifies the phandle of the interrupt controller to
> > which
> > + the IRQs from da9150 are delivered to.
> > +- interrupts: IRQ line info for da9150 chip.
> Cross refer to the standard interrupts doc for these...
>
Ok, can do that.
> > +- interrupt-controller: da9150 has internal IRQs (own IRQ domain).
> > +
> > +Sub-devices:
> > +- da9150-gpadc: See Documentation/devicetree/bindings/iio/adc/da9150-
> gpadc.txt
> > +- da9150-charger: See Documentation/devicetree/bindings/power/da9150-
> charger.txt
> > +
> > +
> > +Example:
> > +
> > + charger_fg: da9150@58 {
> > + compatible = "dlg,da9150";
> > + reg = <0x58>;
> > + interrupt-parent = <&gpio6>;
> > + interrupts = <11 IRQ_TYPE_LEVEL_LOW>;
> > + interrupt-controller;
> > +
> > + gpadc: da9150-gpadc {
> > + ...
> > + };
> > +
> > + da9150-charger {
> > + ...
> > + };
> > + };
> > --
> > 1.9.3
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> > the body of a message to majord...@vger.kernel.org
> > More majordomo info at http://vger.kernel.org/majordomo-info.html
> >
N�r��yb�X��ǧv�^�){.n�+{zX����ܨ}���Ơz�&j:+v���zZ+��+zf���h���~i���z� �w���?�&�)ߢf��^jǫy�m��@A�a���
0��h���i
RE: [PATCH v2 1/7] mfd: Add support for DA9150 combined charger & fuel-gauge device
On September 15, 2014 23:39, Lee Jones wrote:
> On Wed, 10 Sep 2014, Opensource [Adam Thomson] wrote:
> > On September 10, 2014 10:50, Lee Jones wrote:
> > > On Tue, 09 Sep 2014, Opensource [Adam Thomson] wrote:
> > >
> > > > On August 28, 2014 17:36, Lee Jones wrote:
> > > >
> > > > Thanks for the feedback. As a general comment a couple of the items
> > > > you've
> > > > identified relate to future updates (additional functionality being
> > > > added).
> > > > I already have code in place for this but have stripped out a couple of
> > > > the
> > > > drivers just to reduce the churn and size of patch submission. These
> > > > will be
> > > > added once these patches have been accepted.
> > > >
> > > > Where this is the case, I have added notes in-line against the relevant
> > > > comments you made.
> > > >
> > > > > On Thu, 28 Aug 2014, Adam Thomson wrote:
> > > > >
> > > > > > DA9150 is a combined Charger and Fuel-Gauge IC, with additional
> > > > > > GPIO and GPADC functionality.
> > > > > >
> > > > > > Signed-off-by: Adam Thomson
>
> > > > > > ---
> > > > > > drivers/mfd/Kconfig | 12 +
> > > > > > drivers/mfd/Makefile |2 +
> > > > > > drivers/mfd/da9150-core.c| 332 ++
> > > > > > drivers/mfd/da9150-i2c.c | 176 ++
>
> [...]
>
> > > > > > +/* Helper functions for sub-devices to request/free IRQs */
> > > > > > +int da9150_register_irq(struct platform_device *pdev, void *dev_id,
> > > > > > + irq_handler_t handler, const char *name)
> > > > > > +{
> > > > > > + int irq, ret;
> > > > > > +
> > > > > > + irq = platform_get_irq_byname(pdev, name);
> > > > > > + if (irq < 0)
> > > > > > + return irq;
> > > > > > +
> > > > > > + ret = devm_request_threaded_irq(&pdev->dev, irq, NULL, handler,
> > > > > > + IRQF_ONESHOT, name, dev_id);
> > > > > > + if (ret)
> > > > > > + dev_err(&pdev->dev, "Failed to request IRQ %d: %d\n",
> > > > > > irq,
> ret);
> > > > > > +
> > > > > > + return ret;
> > > > > > +}
> > > > > > +EXPORT_SYMBOL_GPL(da9150_register_irq);
> > > > >
> > > > > Why do they need help? What problem does adding these layers solve?
> > > >
> > > > Means I don't have to keep adding print error lines everywhere else if
> > > > this
> > > > function takes care of it. Thought that would be cleaner.
> > >
> > > You only need to request each IRQ once. It's just more abstraction
> > > for the sake of it. I would prefer if you removed them.
> >
> > Yes, but in the charger driver for example, there are 4 IRQs to request. If
> > I don't use this approach the IRQ requesting becomes bloated, hence why I
> > went
> > for a common function like this. Thought generally the intention was to cut
> > down on repeated code?
>
> If you're worried about bloat in .probe() it's okay to define a new
> function within the charger driver; however, creating a call-back into
> the MFD driver like this I think it over-kill for 4 requests.
I could do something just in the charger, but why not have something which can
be used for all sub-devices? There is also an IRQ used in the IIO ADC driver and
there will be another in the later fuel-gauge driver too. Doesn't make sense to
me not to do in the MFD code when that will provide a simple common call for all
sub-devices. What is your concern with adding something like this, just so I'm
clear?
>
> > > > > > +void da9150_release_irq(struct platform_device *pdev, void *dev_id,
> > > > > > + const char *name)
> > > > > > +{
> > > > > > + int irq;
> > > > > > +
> > > > > > + irq = platform_get_irq_byname(pdev, name);
> > > > > > + if (irq < 0)
> > > > > > + return;
> > > > > > +
> > > > > > + devm_free_irq(&pdev->dev, irq, dev_
RE: [PATCH v2 1/7] mfd: Add support for DA9150 combined charger & fuel-gauge device
On September 10, 2014 10:50, Lee Jones wrote: > On Tue, 09 Sep 2014, Opensource [Adam Thomson] wrote: > > > On August 28, 2014 17:36, Lee Jones wrote: > > > > Thanks for the feedback. As a general comment a couple of the items you've > > identified relate to future updates (additional functionality being added). > > I already have code in place for this but have stripped out a couple of the > > drivers just to reduce the churn and size of patch submission. These will be > > added once these patches have been accepted. > > > > Where this is the case, I have added notes in-line against the relevant > > comments you made. > > > > > On Thu, 28 Aug 2014, Adam Thomson wrote: > > > > > > > DA9150 is a combined Charger and Fuel-Gauge IC, with additional > > > > GPIO and GPADC functionality. > > > > > > > > Signed-off-by: Adam Thomson > > > > --- > > > > drivers/mfd/Kconfig | 12 + > > > > drivers/mfd/Makefile |2 + > > > > drivers/mfd/da9150-core.c| 332 ++ > > > > drivers/mfd/da9150-i2c.c | 176 ++ > > > > > > Do you also have another, say SPI version? > > > > No, not yet, but this is something that we may add later as the device does > > support SPI. > > I'm not sure we want to split up the files like this for an 'if we > decide to produce an SPI variant in the future'. If you do, then you > can split the files up. Until then, stick everything in -core. Right. Can't say I agree here, but will refactor. > > > > > > include/linux/mfd/da9150/core.h | 80 +++ > > > > include/linux/mfd/da9150/pdata.h | 21 + > > > > include/linux/mfd/da9150/registers.h | 1153 > > > ++ > > > > 7 files changed, 1776 insertions(+) > > > > create mode 100644 drivers/mfd/da9150-core.c > > > > create mode 100644 drivers/mfd/da9150-i2c.c > > > > create mode 100644 include/linux/mfd/da9150/core.h > > > > create mode 100644 include/linux/mfd/da9150/pdata.h > > > > create mode 100644 include/linux/mfd/da9150/registers.h > > > > > > > > diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig > > > > index de5abf2..76adb2c 100644 > > > > --- a/drivers/mfd/Kconfig > > > > +++ b/drivers/mfd/Kconfig > > > > @@ -183,6 +183,18 @@ config MFD_DA9063 > > > > Additional drivers must be enabled in order to use the > > > > functionality > > > > of the device. > > > > > > > > +config MFD_DA9150 > > > > + bool "Dialog Semiconductor DA9150 Charger Fuel-Gauge chip" > > > > > > Why can't this be built as a module? > > > > No reason. Will change it. > > > > > > > > > + depends on I2C=y > > > > + select MFD_CORE > > > > + select REGMAP_I2C > > > > + select REGMAP_IRQ > > > > + help > > > > + This adds support for the DA9150 integrated charger and > > > > fuel-gauge > > > > + chip. This driver provides common support for accessing the > > > > device. > > > > + Additional drivers must be enabled in order to use the > > > > specific > > > > + features of the device. > > > > + > > > > config MFD_MC13XXX > > > > tristate > > > > depends on (SPI_MASTER || I2C) > > > > diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile > > > > index f001487..098dfa1 100644 > > > > --- a/drivers/mfd/Makefile > > > > +++ b/drivers/mfd/Makefile > > > > @@ -114,6 +114,8 @@ obj-$(CONFIG_MFD_DA9055)+= da9055.o > > > > da9063-objs:= da9063-core.o da9063-irq.o da9063- > i2c.o > > > > obj-$(CONFIG_MFD_DA9063) += da9063.o > > > > > > > > +obj-$(CONFIG_MFD_DA9150) += da9150-core.o da9150-i2c.o > > > > + > > > > > > Do the other drivers smell? Please butt up against them. > > > > > > I'm not entirely sure why there are so many '\n's in the Makefile! > > > > Okey dokey. Will change. > > > > > > > > > obj-$(CONFIG_MFD_MAX14577) += max14577.o > > > > obj-$(CONFIG_MFD_MAX77686) += max77686.o > > >
RE: [PATCH v2 1/7] mfd: Add support for DA9150 combined charger & fuel-gauge device
On August 28, 2014 17:36, Lee Jones wrote:
Thanks for the feedback. As a general comment a couple of the items you've
identified relate to future updates (additional functionality being added).
I already have code in place for this but have stripped out a couple of the
drivers just to reduce the churn and size of patch submission. These will be
added once these patches have been accepted.
Where this is the case, I have added notes in-line against the relevant
comments you made.
> On Thu, 28 Aug 2014, Adam Thomson wrote:
>
> > DA9150 is a combined Charger and Fuel-Gauge IC, with additional
> > GPIO and GPADC functionality.
> >
> > Signed-off-by: Adam Thomson
> > ---
> > drivers/mfd/Kconfig | 12 +
> > drivers/mfd/Makefile |2 +
> > drivers/mfd/da9150-core.c| 332 ++
> > drivers/mfd/da9150-i2c.c | 176 ++
>
> Do you also have another, say SPI version?
No, not yet, but this is something that we may add later as the device does
support SPI.
>
> > include/linux/mfd/da9150/core.h | 80 +++
> > include/linux/mfd/da9150/pdata.h | 21 +
> > include/linux/mfd/da9150/registers.h | 1153
> ++
> > 7 files changed, 1776 insertions(+)
> > create mode 100644 drivers/mfd/da9150-core.c
> > create mode 100644 drivers/mfd/da9150-i2c.c
> > create mode 100644 include/linux/mfd/da9150/core.h
> > create mode 100644 include/linux/mfd/da9150/pdata.h
> > create mode 100644 include/linux/mfd/da9150/registers.h
> >
> > diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> > index de5abf2..76adb2c 100644
> > --- a/drivers/mfd/Kconfig
> > +++ b/drivers/mfd/Kconfig
> > @@ -183,6 +183,18 @@ config MFD_DA9063
> > Additional drivers must be enabled in order to use the functionality
> > of the device.
> >
> > +config MFD_DA9150
> > + bool "Dialog Semiconductor DA9150 Charger Fuel-Gauge chip"
>
> Why can't this be built as a module?
No reason. Will change it.
>
> > + depends on I2C=y
> > + select MFD_CORE
> > + select REGMAP_I2C
> > + select REGMAP_IRQ
> > + help
> > + This adds support for the DA9150 integrated charger and fuel-gauge
> > + chip. This driver provides common support for accessing the device.
> > + Additional drivers must be enabled in order to use the specific
> > + features of the device.
> > +
> > config MFD_MC13XXX
> > tristate
> > depends on (SPI_MASTER || I2C)
> > diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> > index f001487..098dfa1 100644
> > --- a/drivers/mfd/Makefile
> > +++ b/drivers/mfd/Makefile
> > @@ -114,6 +114,8 @@ obj-$(CONFIG_MFD_DA9055)+= da9055.o
> > da9063-objs:= da9063-core.o da9063-irq.o
> > da9063-i2c.o
> > obj-$(CONFIG_MFD_DA9063) += da9063.o
> >
> > +obj-$(CONFIG_MFD_DA9150) += da9150-core.o da9150-i2c.o
> > +
>
> Do the other drivers smell? Please butt up against them.
>
> I'm not entirely sure why there are so many '\n's in the Makefile!
Okey dokey. Will change.
>
> > obj-$(CONFIG_MFD_MAX14577) += max14577.o
> > obj-$(CONFIG_MFD_MAX77686) += max77686.o
> > obj-$(CONFIG_MFD_MAX77693) += max77693.o
> > diff --git a/drivers/mfd/da9150-core.c b/drivers/mfd/da9150-core.c
> > new file mode 100644
> > index 000..029a30b
> > --- /dev/null
> > +++ b/drivers/mfd/da9150-core.c
> > @@ -0,0 +1,332 @@
> > +/*
> > + * DA9150 Core MFD Driver
> > + *
> > + * Copyright (c) 2014 Dialog Semiconductor
> > + *
> > + * Author: Adam Thomson
> > + *
> > + * This program is free software; you can redistribute it and/or modify it
> > + * under the terms of the GNU General Public License as published by the
> > + * Free Software Foundation; either version 2 of the License, or (at your
> > + * option) any later version.
> > + */
> > +
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +
>
> No real need for this '\n'.
I can change this, but my reason was to separate common kernel includes from
device specific ones, for readability.
>
> > +#include
> > +#include
> > +#include
> > +
> > +u8 da9150_reg_read(struct da9150 *da9150, u16 reg)
> > +{
> > + int val, ret;
> > +
> > + ret = regmap_read(da9150->regmap, reg, &val);
> > + if (ret < 0)
>
> What if ret > 0? Is that a good thing? :)
>
> Just 'if (ret)'.
Fine, will change.
>
> > + dev_err(da9150->dev, "Failed to read from reg 0x%x: %d\n",
> > + reg, ret);
> > +
> > + return (u8) val;
> > +}
> > +EXPORT_SYMBOL_GPL(da9150_reg_read);
>
> Not sure I like this abstraction stuff. I could understand if there
> were locking involved, but there isn't. You don't appear to check for
> errors in the subordinate drivers either, rather you just plough on
> ahead. Not sure that's a good idea either.
>
> Anyone have a second opinion?
The reason for these is because future patches to add additional functionality
will introdu
RE: [PATCH v2 1/7] mfd: Add support for DA9150 combined charger & fuel-gauge device
On August 28, 2014 12:47, Varka Bhadram wrote:
> On 08/28/2014 04:18 PM, Adam Thomson wrote:
>
> (...)
>
> > +static int da9150_probe(struct i2c_client *client,
> > + const struct i2c_device_id *id)
> > +{
> > + struct da9150 *da9150;
> > + int ret;
> > +
> > + da9150 = devm_kzalloc(&client->dev, sizeof(struct da9150), GFP_KERNEL);
> > + if (da9150 == NULL)
> > + return -ENOMEM;
>
> da9150 = devm_kzalloc(&client->dev, sizeof(*da9150), GFP_KERNEL);
> if (!da9150)
> return -ENOMEM;
Ok, no real difference, but can change it.
>
> > + da9150->dev = &client->dev;
> > + da9150->irq = client->irq;
> > + i2c_set_clientdata(client, da9150);
> > + dev_set_drvdata(da9150->dev, da9150);
> > +
> > + da9150->regmap = devm_regmap_init_i2c(client, &da9150_regmap_config);
> > + if (IS_ERR(da9150->regmap)) {
> > + ret = PTR_ERR(da9150->regmap);
> > + dev_err(da9150->dev, "Failed to allocate register map: %d\n",
> > + ret);
> > + return ret;
> > + }
> > +
> > + return da9150_device_init(da9150);
> > +}
> > +
> > +static int da9150_remove(struct i2c_client *client)
> > +{
> > + struct da9150 *da9150 = i2c_get_clientdata(client);
> > +
> > + da9150_device_exit(da9150);
> > +
> > + return 0;
> > +}
> > +
> > +static void da9150_shutdown(struct i2c_client *client)
> > +{
> > + struct da9150 *da9150 = i2c_get_clientdata(client);
> > +
> > + da9150_device_shutdown(da9150);
> > +}
> > +
> > +static const struct i2c_device_id da9150_i2c_id[] = {
> > + { "da9150", 0 },
> > + { }
> > +};
> > +MODULE_DEVICE_TABLE(i2c, da9150_i2c_id);
> > +
> > +static const struct of_device_id da9150_of_match[] = {
> > + { .compatible = "dlg,da9150", },
> > + { }
> > +};
> > +
>
> missed MODULE_DEVICE_TABLE(of, ...) ?
Ok, will add that in.
>
> > +static struct i2c_driver da9150_driver = {
> > + .driver = {
> > + .name = "da9150",
> > + .owner = THIS_MODULE,
>
> No need to update this field...
Ok, will remove .owner setting.
>
> > + .of_match_table = of_match_ptr(da9150_of_match),
> > + },
> > + .probe = da9150_probe,
> > + .remove = da9150_remove,
> > + .shutdown = da9150_shutdown,
> > + .id_table = da9150_i2c_id,
> > +};
> > +
> > +module_i2c_driver(da9150_driver);
> > +
> > +MODULE_DESCRIPTION("I2C Driver for DA9150");
> > +MODULE_AUTHOR("Adam Thomson
> > +MODULE_LICENSE("GPL");
> >
> --
> Regards,
> Varka Bhadram.
RE: [PATCH] mfd: da9052: Avoid setting read_flag_mask for da9052-i2c driver
On August 28, 2014 14:38, Adam Thomson wrote:
> On August 28, 2014 12:21, Lee Jones wrote:
>
> > Steve, Adam,
> >
> > > Current code init regmap with &da9052_regmap_config for both da9052-spi
> > > and
> > > da9052-i2c drivers. da9052-spi sets the read_flag_mask.
> > > The same setting may be applied for da9052-i2c if da9052-spi driver is
> > > loaded
> > > first because they actually use the same regmap_config setting.
> > > Fix this issue by using a local variable for regmap_config in da9052-spi
> > > driver,
> > > so the settings in spi driver won't impact the settings in i2c driver.
> > > Also makes da9052_regmap_config const to avoid similar issue.
> > >
> > > Signed-off-by: Axel Lin
> > > ---
> > > Hi Adam and Steve,
> > > Any chance to test this patch?
> >
> > ** Look here **
>
> Lee, Axel,
>
> This was on my to-do list but hadn't yet responded. Will try and find some
> time
> in the next week to see if we can verify this.
Hi Axel, Lee,
I don't think I'm going to have time to test this soon. In terms of the change
it looks fine to me, although I don't think you can run both SPI and I2C at the
same time for this driver (I tried a quick test to instantiate both and saw
issues with regulator initialisation for the second instance). Anyway, here's
my ack for the change.
Acked-by: Adam Thomson
N�r��yb�X��ǧv�^�){.n�+{zX����ܨ}���Ơz�&j:+v���zZ+��+zf���h���~i���z� �w���?�&�)ߢf��^jǫy�m��@A�a���
0��h���i
RE: [PATCH] mfd: da9052: Avoid setting read_flag_mask for da9052-i2c driver
On August 28, 2014 12:21, Lee Jones wrote:
> Steve, Adam,
>
> > Current code init regmap with &da9052_regmap_config for both da9052-spi and
> > da9052-i2c drivers. da9052-spi sets the read_flag_mask.
> > The same setting may be applied for da9052-i2c if da9052-spi driver is
> > loaded
> > first because they actually use the same regmap_config setting.
> > Fix this issue by using a local variable for regmap_config in da9052-spi
> > driver,
> > so the settings in spi driver won't impact the settings in i2c driver.
> > Also makes da9052_regmap_config const to avoid similar issue.
> >
> > Signed-off-by: Axel Lin
> > ---
> > Hi Adam and Steve,
> > Any chance to test this patch?
>
> ** Look here **
Lee, Axel,
This was on my to-do list but hadn't yet responded. Will try and find some time
in the next week to see if we can verify this.
N�r��yb�X��ǧv�^�){.n�+{zX����ܨ}���Ơz�&j:+v���zZ+��+zf���h���~i���z� �w���?�&�)ߢf��^jǫy�m��@A�a���
0��h���i
RE: [PATCH] iio: core: Propagate error codes from OF layer to client drivers
On August 26, 2014 14:48, Ivan T. Ivanov wrote:
> On Tue, 2014-08-26 at 06:25 -0700, Guenter Roeck wrote:
> > On 08/26/2014 12:51 AM, Ivan T. Ivanov wrote:
> > > Hi,
> > >
> > [ ... ]
> > >
> > > Another, less intrusive, solution will be if we revert last patch and
> > > explicitly
> > > check for EPROBE_DEFER on of_ by_name() return. How this sounds?
> > >
> > How is that different to the just accepted patch ?
>
> You mean this one[1]. I have prepared fix last Friday and forget to
> check again before posting it, sorry. Please ignore my patch.
>
> Thanks,
> Ivan
>
> [1] iio:inkern: fix overwritten -EPROBE_DEFER in of_iio_channel_get_by_name
>
Apologies on my part for fixing one problem and introducing another. Didn't see
that in my testing, and missed that potential return value when examining the
code. At the time It looked like the parent function would only expect NULL
pointers for failure, especially given the non CONFIG_OF definitions of the
functions. Should've delved further. :(
N�r��yb�X��ǧv�^�){.n�+{zX����ܨ}���Ơz�&j:+v���zZ+��+zf���h���~i���z� �w���?�&�)ߢf��^jǫy�m��@A�a���
0��h���i
RE: [PATCH 6/8] power: Add support for DA9150 Charger
On June 16, 2014 14:28, Lee Jones wrote:
> > +#ifndef _DA9150_CHARGER_H
> > +#define _DA9150_CHARGER_H
>
> Two '_'s are normally preferred.
Ok, can change it accordingly.
>
> > +#include
> > +#include
>
> What are you using this for?
Nothing. Will remove them. Thanks.
> > +#include
> > +
> > +
>
> Extra '\n' here.
Yes, just separate includes from the rest of the file. Find it more readable.
If it's an issue though I can remove it.
> > + struct power_supply ac;
> > + struct power_supply usb;
> > + struct power_supply battery;
>
> Do you want these (or pointers to these) here?
>
> How much space do they take up?
Yes these are required in that structure. This is the common approach for
power_supply drivers. The structures, having had a very quick look, are around
maybe 70 bytes each.
N�r��yb�X��ǧv�^�){.n�+{zX����ܨ}���Ơz�&j:+v���zZ+��+zf���h���~i���z� �w���?�&�)ߢf��^jǫy�m��@A�a���
0��h���i
RE: [PATCH 4/8] iio: Add support for DA9150 GPADC
On June 15, 2014 21:19, Jonathon Cameron wrote: > Hi Adam > > Reasonably clean code, but the _ channels stuff doesn't comply with the ABI > and is rather confusing. To be honest I did debate this in my head for a while. The reason I went with the current approach was to make the driver channel layout match that of the datasheet for the device. Sounds like it wasn't the correct direction though. > If it really does make sense to allow reading these channels at different > ranges (presumably to enhance the effective adc resolution) then we need > to control this via existing ABIs. Controlling it via scale with a > range attribute if required (read only but changes with whatever the > scale attribute it set to). Yes, with smaller range then you get a better degree of accuracy. Ok, will have a look at this and see if I can improve things based on your comments. > Often, for slow parts at least it makes little sense to have provide > software support for variable input ranges. > > Looking at what is covered, is it the case that only one option will make > sense for a given hardware setup? (cover the required range and nothing more). > > If so, perhaps this wants to go into the device tree or similar? Possibly. Having read your comments further on though, and given that it seems reasonable to use the range/scale attribute to choose the resolution, I'd prefer to opt for that approach (seems more flexible). > Please get rid of excess white space and comments that just tell you > something obvious about the code following them. Preferably I'd like to keep it this way as I think it makes the code easier to read, but if you're dead against it then I'll remove them. > Linear in the coeeficients, so fine for raw output with a scale and offset. Ok will have a look at re-working this as per your comment. > Ah, so here is your reasoning behind the 'interesting' underscore channels. > This is just doing different range checks on the channel? If so allow one > at a time and provide a writable scale info_mask element to control which > is used.. > > Looks like there are only thes two channels doing this, so shouldn't be > too hard to support it via more conventional means. The GPIO, VBUS and VSYS related channels provide different ranges. I guess they should all follow the same approach for implementing this alternate range. So I will assume that for the 'repeat' channels where the same raw value is provided on both the _ and non _ channels only one channel should be provided. > Don't use the |=, just = and you can't avoid assigning reg above. > Actually, it's not complex enough that you couldn't just do it directly into > the write function and skip this local variable. Ok fine. Will update to tidy up. > Unlock first, then check for error. Yep. Makes sense. Will update. > The mixture of having defs here for the shift. > > + /* MSBs - 8 bits */ > > + result |= result_regs[0] << 2; > and not here is inconsistent. I honestly don't mind which way, but > just use one style. > > You could just use an endian conversion and shift the combined 16bit result Fair point. Will make this cleaner. > It very rarely makes sense to provide both raw and processed interfaces. > If the transform is nasty and non linear then userspace won't have the info > to do anything with it. If the transform is linear, then provide scale > and offset and let userspace perform the computation. The charger module uses certain channels for its readings, and to me it makes more sense for that calculation to be done by the GPADC. However having looked at the inkern.c framework code it looks like if you request a processed channel and it's doesn't provide that feature, then the code drops to linear scaling if possible to provide the processed value. I take it this is the preferred approach then? Just thought it was more consistent to see the calculation using dedicated functions (processed channel approach). > Get rid of this comment and any other ones that don't add any > actual information. Also this is single line comment, please > check the comment syntax. As per previous comment on this topic. > Why a blank line here. Accidental. Will remove it. > The device register call is the one that exposes userspace interfaces. As > a general rule it wants to be the last thing in probe as everything > else should be setup first. Ok, that's something I missed. Will make the change. > > +#include > This isn't used in the header, so should be included in the c file, not here. Yes, is something I meant to remove during development increments. Consider it gone. > > +#include > Having moved the struct definition into the c file > this header include will not want to be here. I personally would prefer a separate header for the definitions as I believe this is tidier. Have never really liked struct definitions in c files. I could move this header to the same directory as the source file, if that is preferred. As it stands this head
RE: [PATCH 1/8] mfd: Add support for DA9150 combined charger & fuel-gauge device
On Sun, Jun 15, 2014 at 20:49, Jonathan Cameron wrote: > Hi Adam, > > Some general comments inline. > > It's been a while since I've looked at any particularly similar parts, > but it seems to me that a lot of indirection gets added here that > if anything makes the codes slightly harder to follow... > > Feel free to disagree with me though! Will do :) > To my mind all these wrappers add nothing significant so you might as well > just call da9150->read_dev etc directly. > > Also, what are the read_qif and write_qif for? They don't seem to be used > anywhere. read_qif and write_qif are for the Fuel-Gauge functionality of the chip. The associated driver will be submitted after acceptance of initial driver code, and will make use of these functions. The wrappers automatically choose the correct client to use (QIF uses a different slave address to the main chip one). Means the child drivers only need to pass through the da9150 struct and the rest is dealt with underneath. > The only real reason I can see for these wrappers is because you want > to hide the struct da9150 contents from the children of the mfd. As you > aren't doing that, you might as well drop these in favour of direct > calls to regmap_read and friends. As I have a need to pass through the main da9150 struct point for the aforementioned wrappers, it seemed cleaner and more consistent to have wrappers for these as well, which did the job of regmap access. Means all HW access uses the same kind of approach, and all sub-devices just need a point to the main da9150 struct to be able to use the functions. > I'll continue my tirade against obvious comments. Wrong format and > adds nothing to what is here as init and exit functions are clearly > doing what their name suggests (it's one of my pet hates ;) I agree the comment doesn't add much in terms of description but for me it breaks up the code to make it easier to follow. However if I get an overwhelming hatred for this I can change it. Also, I know the rule regarding single/multiple line comments but here again I feel it helps separate the code and makes it easier to read. > As a general good practice point, I'd rather that the driver supported > more than one instance of the chip.. Hence you'd take a copy of da9150_devs > to use here. I guess it is relatively unlikely with one of these, but > you never know ;) Have followed the general methods for MFD here, and a number of drivers take the same approach. Also, I think it would be undesirable to have multiple charger chips of the same type in one platform. I agree generally it's best to support multiple instances, but here I don't think we should. > Why does this need it's own file? Does the DA9150 support any other > interfaces? Yes, the DA9150 also has a SPI interface. At present the plan is to just add I2C support for now, but in the future we may add SPI support, so have written the code with this in mind. > Why the indirection? The da9150 only supports i2c as far as I can see. As per my last comment. > I'd roll this into one line and not bother with the local variable... Fair enough but I think this keeps the code cleaner, and to me it makes sense for the actual logic to be in core file as that's interface agnostic. > Drop comments on things that are self-evident. Also these are one > line comments so should be using the single line comment syntax. As per my previous comment I think it just helps to break up the code and makes it more readable. Will change it though if the general consensus is to remove it.
RE: [PATCH 4/8] iio: Add support for DA9150 GPADC
On June 11, 2014 17:35, Jonathon Cameron wrote: > Hi Adam. Silly question but what are the _ channels? > > I have put in a perhaps optimistic data sheet request via the website, > throwing in > your name. Hope you don't mind! > > Jonathan Hi Jonathon. Some of the _channels are a different representation of the reading (e.g. different scale - one would be 0-6V range, and one would be 0-21V for example). Others provide identical readings to the non _channel but have left those in to keep the channel numbering sane and easy to reference. No problem regarding the request.
RE: [PATCH 8/8] DT: Add vendor prefix for Dialog Semiconductor Ltd.
> On Wed, Jun 11, 2014 at 13:43, Rob Herring wrote:
>
> dlg is the correct one and seems to be the most widely used, so we
> should go with that and mark the others deprecated.
>
> diasemi is documented for the da9210, but not used anywhere.
>
> There appears to be 2 different da9053 bindings. I'd guess the dialog
> variant used on 2 i.MX boards is not used. Other i.MX boards use the
> dlg version. I'm not sure what is going on with da9055. Please sort
> these out.
>
The intention is to update the other Dialog drivers after we've agreed this
prefix. With regards to da9055, what's the issue here as I believe that uses
the 'dlg' prefix already?
N�r��yb�X��ǧv�^�){.n�+{zX����ܨ}���Ơz�&j:+v���zZ+��+zf���h���~i���z� �w���?�&�)ߢf��^jǫy�m��@A�a���
0��h���i
RE: [PATCH Resend 2/2] mfd: da9055: Add DT binding documentation for PMIC
On Tue, 05 Mar 2014 02:40:38 +, Lee Jones wrote:
> > Signed-off-by: Adam Thomson
> > Acked-by: Mark Brown
>
> As this has Mark's ack, I'd be happy to apply the patch if you correct
> a couple of nits.
Yep. No problem.
> > +Example:
> > +
> > + pmic: da9055-pmic@5a {
> > + compatible = "dlg,da9055-pmic";
> > + reg = <0x5a>;
> > + interrupt-parent = <&intc>;
> > + interrupts = <5 0x8>;
>
> I'd prefer if you used the #defines in:
> dt-bindings/interrupt-controller/irq.h
Fine. Will do that.
> > + buck2: BUCK2 {
> > + regulator-min-microvolt = <925000>;
> > + regulator-max-microvolt = <250>;
> > + };
>
> New line here.
Will remove it.
RE: [PATCH v2 2/8] ASoC: da9055: Add DT support for CODEC
On Fri, Feb 07, 2014 at 04:27:17PM +, Mark Brown wrote:
> On Fri, Feb 07, 2014 at 04:25:04PM +, Mark Brown wrote:
> > On Thu, Feb 06, 2014 at 06:03:09PM +, Adam Thomson wrote:
> > > Signed-off-by: Adam Thomson
> >
> > This doesn't apply against my current for-next or v3.14-rc1, can you
> > please check and resend?
>
> Ugh, sorry - it'll be due to patch 1 of course which I'd skipped due to
> the MFD bit. If you need to resend it's probably easier all round to
> just send the MFD and ASoC bits of that separately since there's no
> direct dependency.
Ok, no problem. Will see what Lee comes back and go from there.
N�r��yb�X��ǧv�^�){.n�+{zX����ܨ}���Ơz�&j:+v���zZ+��+zf���h���~i���z� �w���?�&�)ߢf��^jǫy�m��@A�a���
0��h���i
RE: [PATCH v2 3/8] mfd: da9055: Add DT support for PMIC
On Fri, Feb 07, 2014 at 11:03:18AM +, Lee Jones wrote: > > Can you break this up please. > > Bindings should be separate to the binding document. Right, fine. Figured as they were directly related they should be in one patch but will separate them. > Entangling the i2c_device_id table with the of_device_id table sounds > a bit too wacky for my liking. Where do you even use it? > You're right. Is not used so can remove it.
RE: [PATCH 0/8] da9055: Driver initialisation fixes, add DT support
On Thu, Feb 06, 2014 at 13:23:47PM +, Guenter Roeck wrote:
> > What I could've done is use platform_get_irq_byname() and avoided using
> > regmap_irq_get_virq() as I would already have the correct VIRQ to pass to
> > request_threaded_irq(), but I figured that using regmap_irq_get_virq() made
> > more sense at the time, and was unable to use both.
> >
>
> I may be missing something, but I think the problem may be that you are
> doing two mappings instead of just one. I don't think you need to call
> regmap_irq_get_virq() at all.
Yes, you're correct. The issue was already there in the code and I was
attempting to fix it. When I made the change I figured using only
regmap_irq_get_virq() was the way to go, but seems like I chose the wrong
option. Will make the changes (remove regmap_irq_get_virq()), test on both DT
and non-DT platforms, and then will re-submit the patches.
N�r��yb�X��ǧv�^�){.n�+{zX����ܨ}���Ơz�&j:+v���zZ+��+zf���h���~i���z� �w���?�&�)ߢf��^jǫy�m��@A�a���
0��h���i
RE: [PATCH 4/8] regulator: da9055: Add DT support
On Thu, Feb 06, 2014 at 12:04:52PM +, Mark Brown wrote: > > Used another driver as an example for this, but if there's a better method > > then I'm happy to use it. Will have a look. > > That's probably an older driver - the code was factored out at some > point but lots of drivers don't get much love. Yep, fair enough. Will sort it. > What's happening here is that the MFD framework has done the lookup for > you when passing the interrupt resource through - you should just use > platform_get_irq_byname() and save a little code in the driver. If it's > behaving differently on DT and non-DT systems it seems better to figure > out why and then make it consistent. Is what I summised but I obviously chose the wrong direction for my fix. Will use platform_get_irq_byname() solely, and will make sure all works on both DT and non-DT setups. If not I'll dig further.
RE: [PATCH 0/8] da9055: Driver initialisation fixes, add DT support
On Thu, Feb 06, 2014 at 00:54:11AM +, Guenter Roeck wrote: > Adam, > > You don't really explain what the problem actually is. Can you elaborate ? Sorry, yes. For the conflicting device Ids, both the PMIC and the CODEC used the same I2C Id string, which meant if you tried to intiate both together on the same bus, then the second would fail. For the removal of platform_get_irq_byname(), the reason for this was that it was conflicting with regmap_irq_get_virq() when the IRQ value returned from platform_get_irq_byname() was being passed to regmap_irq_get_virq(). The result for the code was that it would try to request a threaded IRQ using an invalid IRQ number (have also described this further in patch 0004 mail thread, https://lkml.org/lkml/2014/2/6/126). > > Also, I have been using platform_get_irq() to get the interrupt resource > in mfd client drivers and similar situations. Wouldn't this work here as well > if you don't want to use platform_get_irq_byname() ? What I could've done is use platform_get_irq_byname() and avoided using regmap_irq_get_virq() as I would already have the correct VIRQ to pass to request_threaded_irq(), but I figured that using regmap_irq_get_virq() made more sense at the time, and was unable to use both.
RE: [PATCH 4/8] regulator: da9055: Add DT support
On Wed, Feb 05, 2014 at 18:37:21PM +, Mark Brown wrote:
> On Wed, Feb 05, 2014 at 05:48:35PM +, Adam Thomson wrote:
>
> > +#ifdef CONFIG_OF
> > +#include
> > +#include
> > +#endif /* CONFIG_OF */
>
> Don't do ifdefs for includes like this, it's not worth it.
Fine. Seen examples of both in the kernel, but happy to remove it.
>
> > + for_each_child_of_node(nproot, np) {
> > + if (!of_node_cmp(np->name,
> > +regulator->info->reg_desc.name)) {
> > + config->init_data = of_get_regulator_init_data(
> > + &pdev->dev, np);
> > + config->of_node = np;
> > + break;
> > + }
> > + }
>
> I think you're looking for of_regulator_match() here.
Used another driver as an example for this, but if there's a better method
then I'm happy to use it. Will have a look.
>
> > if (pdata && pdata->regulators)
> > config.init_data = pdata->regulators[pdev->id];
> > + else {
> > + ret = da9055_regulator_dt_init(pdev, regulator, &config);
> > + if (ret < 0)
> > + return ret;
> > + }
>
> Coding style, both sides of the if should have braces if one does.
Fine. Will update.
>
> > /* Only LDO 5 and 6 has got the over current interrupt */
> > if (pdev->id == DA9055_ID_LDO5 || pdev->id == DA9055_ID_LDO6) {
> > - irq = platform_get_irq_byname(pdev, "REGULATOR");
> > - irq = regmap_irq_get_virq(da9055->irq_data, irq);
> > + irq = regmap_irq_get_virq(da9055->irq_data,
> > + DA9055_IRQ_REGULATOR);
>
> This seems like a bit of a step backwards - what happened in the MFD
> (and why didn't it update the users to avoid breaking bisection)?
I tested this on target, when doing tests for devicetree. What was happening was
that platform_get_irq_byname() was returning the VIRQ number already (368 in one
test case where onkey was being probed) rather than the local IRQ number for the
device (the resource information seemed to have been updated with the VIRQ
number instead of the local IRQ number). So when that was passed to
regmap_irq_get_virq() it would then return an incorrect IRQ number (0 in the
same scenario, when I enabled DEBUG in irqdomain.c, I would see the message
"error: hwirq 0x170 is too large for da9055_irq"). That incorrect irq was then
being passed to devm_request_threaded_irq() which subsequently failed. This is
why I made the change. Is it preferrable to use platform_get_irq_byname()
instead of regmap_irq_get_virq() as using them both doesn't seem to work, unless
I'm missing something fundamental here.
