Re: [PATCH RESEND v2 1/4] mfd: intel_soc_pmic: Core driver
On Wed, May 28, 2014 at 08:55:11AM +0800, Zhu, Lejun wrote:
> Oh I see. Sorry I missed your point. So you are saying "int
> intel_soc_pmic_readb(int reg)" is bad, but if I have:
> int intel_soc_pmic_readb(struct intel_soc_pmic *pmic, int reg)
> {
> int ret;
> unsigned int val;
>
> ret = regmap_read(pmic->regmap, reg, );
> if (!ret)
> ret = val;
>
> return ret;
> }
> And have the caller (device or core) look up and pass *pmic in, this
> will be OK?
Yes, that's the more common pattern - normally the caller will need
*pmic for some other purpose anyway so it saves an additional regmap
lookup if that's desired.
signature.asc
Description: Digital signature
Re: [PATCH RESEND v2 1/4] mfd: intel_soc_pmic: Core driver
On Wed, May 28, 2014 at 08:55:11AM +0800, Zhu, Lejun wrote:
Oh I see. Sorry I missed your point. So you are saying int
intel_soc_pmic_readb(int reg) is bad, but if I have:
int intel_soc_pmic_readb(struct intel_soc_pmic *pmic, int reg)
{
int ret;
unsigned int val;
ret = regmap_read(pmic-regmap, reg, val);
if (!ret)
ret = val;
return ret;
}
And have the caller (device or core) look up and pass *pmic in, this
will be OK?
Yes, that's the more common pattern - normally the caller will need
*pmic for some other purpose anyway so it saves an additional regmap
lookup if that's desired.
signature.asc
Description: Digital signature
Re: [PATCH RESEND v2 1/4] mfd: intel_soc_pmic: Core driver
On 5/27/2014 7:20 PM, Mark Brown wrote:
> On Tue, May 27, 2014 at 08:48:58AM +0800, Zhu, Lejun wrote:
>> On 5/26/2014 10:51 PM, Mark Brown wrote:
>
We created these names to hide the implementation of how read/write is
done from other platform specific patches interacting with this driver.
So when we change the implementation, e.g. from I2C read/write to
regmap, we don't have to touch all these patches.
>
>>> This sort of HAL is frowned upon in the upstream kernel.
>
>> We want to do what other MFD drivers' been doing, and make it easier for
>> the callers. A couple of similar examples are intel_msic_reg_read() and
>> lp3943_read_byte(). We want to do the same with intel_soc_pmic_readb(),
>> and I don't think it's too odd.
>
> The odd and problematic bit is the global variable part of things -
> these wrappers are usually just doing lookup of the underlying I/O
> handle in the struct for the device and can be implemented as static
> inlines in the header.
>
Oh I see. Sorry I missed your point. So you are saying "int
intel_soc_pmic_readb(int reg)" is bad, but if I have:
int intel_soc_pmic_readb(struct intel_soc_pmic *pmic, int reg)
{
int ret;
unsigned int val;
ret = regmap_read(pmic->regmap, reg, );
if (!ret)
ret = val;
return ret;
}
And have the caller (device or core) look up and pass *pmic in, this
will be OK?
Best Regards
Lejun
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH RESEND v2 1/4] mfd: intel_soc_pmic: Core driver
On Tue, May 27, 2014 at 08:48:58AM +0800, Zhu, Lejun wrote: > On 5/26/2014 10:51 PM, Mark Brown wrote: > >> We created these names to hide the implementation of how read/write is > >> done from other platform specific patches interacting with this driver. > >> So when we change the implementation, e.g. from I2C read/write to > >> regmap, we don't have to touch all these patches. > > This sort of HAL is frowned upon in the upstream kernel. > We want to do what other MFD drivers' been doing, and make it easier for > the callers. A couple of similar examples are intel_msic_reg_read() and > lp3943_read_byte(). We want to do the same with intel_soc_pmic_readb(), > and I don't think it's too odd. The odd and problematic bit is the global variable part of things - these wrappers are usually just doing lookup of the underlying I/O handle in the struct for the device and can be implemented as static inlines in the header. signature.asc Description: Digital signature
Re: [PATCH RESEND v2 1/4] mfd: intel_soc_pmic: Core driver
On Tue, May 27, 2014 at 08:48:58AM +0800, Zhu, Lejun wrote: On 5/26/2014 10:51 PM, Mark Brown wrote: We created these names to hide the implementation of how read/write is done from other platform specific patches interacting with this driver. So when we change the implementation, e.g. from I2C read/write to regmap, we don't have to touch all these patches. This sort of HAL is frowned upon in the upstream kernel. We want to do what other MFD drivers' been doing, and make it easier for the callers. A couple of similar examples are intel_msic_reg_read() and lp3943_read_byte(). We want to do the same with intel_soc_pmic_readb(), and I don't think it's too odd. The odd and problematic bit is the global variable part of things - these wrappers are usually just doing lookup of the underlying I/O handle in the struct for the device and can be implemented as static inlines in the header. signature.asc Description: Digital signature
Re: [PATCH RESEND v2 1/4] mfd: intel_soc_pmic: Core driver
On 5/27/2014 7:20 PM, Mark Brown wrote:
On Tue, May 27, 2014 at 08:48:58AM +0800, Zhu, Lejun wrote:
On 5/26/2014 10:51 PM, Mark Brown wrote:
We created these names to hide the implementation of how read/write is
done from other platform specific patches interacting with this driver.
So when we change the implementation, e.g. from I2C read/write to
regmap, we don't have to touch all these patches.
This sort of HAL is frowned upon in the upstream kernel.
We want to do what other MFD drivers' been doing, and make it easier for
the callers. A couple of similar examples are intel_msic_reg_read() and
lp3943_read_byte(). We want to do the same with intel_soc_pmic_readb(),
and I don't think it's too odd.
The odd and problematic bit is the global variable part of things -
these wrappers are usually just doing lookup of the underlying I/O
handle in the struct for the device and can be implemented as static
inlines in the header.
Oh I see. Sorry I missed your point. So you are saying int
intel_soc_pmic_readb(int reg) is bad, but if I have:
int intel_soc_pmic_readb(struct intel_soc_pmic *pmic, int reg)
{
int ret;
unsigned int val;
ret = regmap_read(pmic-regmap, reg, val);
if (!ret)
ret = val;
return ret;
}
And have the caller (device or core) look up and pass *pmic in, this
will be OK?
Best Regards
Lejun
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH RESEND v2 1/4] mfd: intel_soc_pmic: Core driver
On 5/26/2014 10:51 PM, Mark Brown wrote: > On Mon, May 26, 2014 at 02:01:11PM +0800, Zhu, Lejun wrote: >> On 5/24/2014 1:49 AM, Mark Brown wrote: > >>> There should also be no need to add extra locking around regmap calls, >>> the regmap API has locking as standard. > >> Actually it also protects the pmic variable, so it won't be set to NULL >> while there's ongoing read/write. > > Righ, but there is no clear need for the pmic variable to exist in the > first place. > >>> It's also not clear why this API exists at all, surely all the >>> interaction with the device happens from the core or function drivers >>> for the device which ought to be able to get a direct reference to the >>> regmap anyway and only be instantiated when one is present. > >> We created these names to hide the implementation of how read/write is >> done from other platform specific patches interacting with this driver. >> So when we change the implementation, e.g. from I2C read/write to >> regmap, we don't have to touch all these patches. > > This sort of HAL is frowned upon in the upstream kernel. We want to do what other MFD drivers' been doing, and make it easier for the callers. A couple of similar examples are intel_msic_reg_read() and lp3943_read_byte(). We want to do the same with intel_soc_pmic_readb(), and I don't think it's too odd. Best Regards Lejun -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH RESEND v2 1/4] mfd: intel_soc_pmic: Core driver
On Mon, May 26, 2014 at 02:01:11PM +0800, Zhu, Lejun wrote: > On 5/24/2014 1:49 AM, Mark Brown wrote: > > There should also be no need to add extra locking around regmap calls, > > the regmap API has locking as standard. > Actually it also protects the pmic variable, so it won't be set to NULL > while there's ongoing read/write. Righ, but there is no clear need for the pmic variable to exist in the first place. > > It's also not clear why this API exists at all, surely all the > > interaction with the device happens from the core or function drivers > > for the device which ought to be able to get a direct reference to the > > regmap anyway and only be instantiated when one is present. > We created these names to hide the implementation of how read/write is > done from other platform specific patches interacting with this driver. > So when we change the implementation, e.g. from I2C read/write to > regmap, we don't have to touch all these patches. This sort of HAL is frowned upon in the upstream kernel. signature.asc Description: Digital signature
Re: [PATCH RESEND v2 1/4] mfd: intel_soc_pmic: Core driver
On 5/24/2014 1:49 AM, Mark Brown wrote:
> On Fri, May 23, 2014 at 08:40:26AM +0800, Zhu, Lejun wrote:
>
>> +struct device *intel_soc_pmic_dev(void)
>> +{
>> +return pmic->dev;
>> +}
>> +EXPORT_SYMBOL(intel_soc_pmic_dev);
>
> Why do you need to take a global reference to this?
It was used by the GPIO driver to get the parent device. The latest
patch use dev.parent instead, so the whole function can be removed.
>> +/*
>> + * Read from a PMIC register
>> + */
>> +int intel_soc_pmic_readb(int reg)
>> +{
>> +int ret;
>> +unsigned int val;
>> +
>> +mutex_lock(_lock);
>> +
>> +if (!pmic) {
>> +ret = -EIO;
>> +} else {
>> +ret = regmap_read(pmic->regmap, reg, );
>> +if (!ret)
>> +ret = val;
>> +}
>> +
>> +mutex_unlock(_lock);
>> +
>> +return ret;
>> +}
>> +EXPORT_SYMBOL(intel_soc_pmic_readb);
>
> This is wrapping the EXPORT_SYMBOL_GPL() regmap API with an
> EXPORT_SYMBOL() API. Don't do that, if you really do need these
> wrappers make them EXPORT_SYMBOL_GPL().
I'll change them to use EXPORT_SYMBOL_GPL().
> There should also be no need to add extra locking around regmap calls,
> the regmap API has locking as standard.
Actually it also protects the pmic variable, so it won't be set to NULL
while there's ongoing read/write.
> It's also not clear why this API exists at all, surely all the
> interaction with the device happens from the core or function drivers
> for the device which ought to be able to get a direct reference to the
> regmap anyway and only be instantiated when one is present.
We created these names to hide the implementation of how read/write is
done from other platform specific patches interacting with this driver.
So when we change the implementation, e.g. from I2C read/write to
regmap, we don't have to touch all these patches.
>> +/*
>> + * Set platform_data of the child devices. Needs to be called before
>> + * the MFD device is added.
>> + */
>> +int intel_soc_pmic_set_pdata(const char *name, void *data, int len)
(...)
>> +EXPORT_SYMBOL(intel_soc_pmic_set_pdata);
>
> What is going on here, why aren't the normal ways of getting data to the
> devices (such as reading the platform data of the parent device) OK?
For this PMIC (Crystal Cove) it is not used. So I'll remove it.
>> +static void __pmic_regmap_flush(void)
>> +{
>> +if (cache_write_pending)
>> +WARN_ON(regmap_write(pmic->regmap, cache_offset,
>> + cache_write_val));
>
>> +static int pmic_regmap_load_from_hw(struct intel_pmic_regmap *map)
>> +{
>> +int ret;
>
> This all appears to be an open coded cache layer - there is already
> cache support in regmap, just reuse that.
>
>> +static irqreturn_t pmic_irq_isr(int irq, void *data)
>> +{
>> +return IRQ_WAKE_THREAD;
>> +}
>
> Just register the IRQ as IRQF_ONESHOT and only provide the threaded
> handler.
I'll fix it.
>> +static irqreturn_t pmic_irq_thread(int irq, void *data)
>> +{
>> +int i;
>> +
>> +mutex_lock(>irq_lock);
>> +
>> +for (i = 0; i < pmic->irq_num; i++) {
>> +if (test_bit(PMIC_IRQ_MASK_BIT(i), _IRQ_MASK(pmic, i)))
>> +continue;
>> +
>> +if (pmic_regmap_read(>irq_regmap[i].status)) {
>> +pmic_regmap_write(>irq_regmap[i].ack, 1);
>> +handle_nested_irq(pmic->irq_base + i);
>> +}
>> +}
>
> This looks like you should be using regmap-irq, or at least like there's
> some small additions needed to make it usable.
I'll check if I can convert to regmap-irq. If it works, I won't need
this and the cache layer.
>> +if (gpio_is_valid(pmic->pmic_int_gpio)) {
>> +ret = gpio_request_one(pmic->pmic_int_gpio,
>> + GPIOF_DIR_IN, "PMIC Interupt");
>> +if (ret) {
>> +dev_err(pmic->dev, "Request PMIC_INT gpio error\n");
>> +goto out_free_desc;
>> +}
>> +
>> +pmic->irq = gpio_to_irq(pmic->pmic_int_gpio);
>> +}
>
> There should be no need to do this, simply requesting the interrupt
> should be sufficient to ensure the pin is in the correct mode. If this
> isn't the case the interrupt controller driver probably needs an update,
> there's some support for helping with this in the GPIO framework IIRC.
I'll remove this.
> You're also not using managed (devm) allocations for anything here.
Best Regards
Lejun
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH RESEND v2 1/4] mfd: intel_soc_pmic: Core driver
On 5/24/2014 1:49 AM, Mark Brown wrote:
On Fri, May 23, 2014 at 08:40:26AM +0800, Zhu, Lejun wrote:
+struct device *intel_soc_pmic_dev(void)
+{
+return pmic-dev;
+}
+EXPORT_SYMBOL(intel_soc_pmic_dev);
Why do you need to take a global reference to this?
It was used by the GPIO driver to get the parent device. The latest
patch use dev.parent instead, so the whole function can be removed.
+/*
+ * Read from a PMIC register
+ */
+int intel_soc_pmic_readb(int reg)
+{
+int ret;
+unsigned int val;
+
+mutex_lock(pmic_lock);
+
+if (!pmic) {
+ret = -EIO;
+} else {
+ret = regmap_read(pmic-regmap, reg, val);
+if (!ret)
+ret = val;
+}
+
+mutex_unlock(pmic_lock);
+
+return ret;
+}
+EXPORT_SYMBOL(intel_soc_pmic_readb);
This is wrapping the EXPORT_SYMBOL_GPL() regmap API with an
EXPORT_SYMBOL() API. Don't do that, if you really do need these
wrappers make them EXPORT_SYMBOL_GPL().
I'll change them to use EXPORT_SYMBOL_GPL().
There should also be no need to add extra locking around regmap calls,
the regmap API has locking as standard.
Actually it also protects the pmic variable, so it won't be set to NULL
while there's ongoing read/write.
It's also not clear why this API exists at all, surely all the
interaction with the device happens from the core or function drivers
for the device which ought to be able to get a direct reference to the
regmap anyway and only be instantiated when one is present.
We created these names to hide the implementation of how read/write is
done from other platform specific patches interacting with this driver.
So when we change the implementation, e.g. from I2C read/write to
regmap, we don't have to touch all these patches.
+/*
+ * Set platform_data of the child devices. Needs to be called before
+ * the MFD device is added.
+ */
+int intel_soc_pmic_set_pdata(const char *name, void *data, int len)
(...)
+EXPORT_SYMBOL(intel_soc_pmic_set_pdata);
What is going on here, why aren't the normal ways of getting data to the
devices (such as reading the platform data of the parent device) OK?
For this PMIC (Crystal Cove) it is not used. So I'll remove it.
+static void __pmic_regmap_flush(void)
+{
+if (cache_write_pending)
+WARN_ON(regmap_write(pmic-regmap, cache_offset,
+ cache_write_val));
+static int pmic_regmap_load_from_hw(struct intel_pmic_regmap *map)
+{
+int ret;
This all appears to be an open coded cache layer - there is already
cache support in regmap, just reuse that.
+static irqreturn_t pmic_irq_isr(int irq, void *data)
+{
+return IRQ_WAKE_THREAD;
+}
Just register the IRQ as IRQF_ONESHOT and only provide the threaded
handler.
I'll fix it.
+static irqreturn_t pmic_irq_thread(int irq, void *data)
+{
+int i;
+
+mutex_lock(pmic-irq_lock);
+
+for (i = 0; i pmic-irq_num; i++) {
+if (test_bit(PMIC_IRQ_MASK_BIT(i), PMIC_IRQ_MASK(pmic, i)))
+continue;
+
+if (pmic_regmap_read(pmic-irq_regmap[i].status)) {
+pmic_regmap_write(pmic-irq_regmap[i].ack, 1);
+handle_nested_irq(pmic-irq_base + i);
+}
+}
This looks like you should be using regmap-irq, or at least like there's
some small additions needed to make it usable.
I'll check if I can convert to regmap-irq. If it works, I won't need
this and the cache layer.
+if (gpio_is_valid(pmic-pmic_int_gpio)) {
+ret = gpio_request_one(pmic-pmic_int_gpio,
+ GPIOF_DIR_IN, PMIC Interupt);
+if (ret) {
+dev_err(pmic-dev, Request PMIC_INT gpio error\n);
+goto out_free_desc;
+}
+
+pmic-irq = gpio_to_irq(pmic-pmic_int_gpio);
+}
There should be no need to do this, simply requesting the interrupt
should be sufficient to ensure the pin is in the correct mode. If this
isn't the case the interrupt controller driver probably needs an update,
there's some support for helping with this in the GPIO framework IIRC.
I'll remove this.
You're also not using managed (devm) allocations for anything here.
Best Regards
Lejun
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH RESEND v2 1/4] mfd: intel_soc_pmic: Core driver
On Mon, May 26, 2014 at 02:01:11PM +0800, Zhu, Lejun wrote: On 5/24/2014 1:49 AM, Mark Brown wrote: There should also be no need to add extra locking around regmap calls, the regmap API has locking as standard. Actually it also protects the pmic variable, so it won't be set to NULL while there's ongoing read/write. Righ, but there is no clear need for the pmic variable to exist in the first place. It's also not clear why this API exists at all, surely all the interaction with the device happens from the core or function drivers for the device which ought to be able to get a direct reference to the regmap anyway and only be instantiated when one is present. We created these names to hide the implementation of how read/write is done from other platform specific patches interacting with this driver. So when we change the implementation, e.g. from I2C read/write to regmap, we don't have to touch all these patches. This sort of HAL is frowned upon in the upstream kernel. signature.asc Description: Digital signature
Re: [PATCH RESEND v2 1/4] mfd: intel_soc_pmic: Core driver
On 5/26/2014 10:51 PM, Mark Brown wrote: On Mon, May 26, 2014 at 02:01:11PM +0800, Zhu, Lejun wrote: On 5/24/2014 1:49 AM, Mark Brown wrote: There should also be no need to add extra locking around regmap calls, the regmap API has locking as standard. Actually it also protects the pmic variable, so it won't be set to NULL while there's ongoing read/write. Righ, but there is no clear need for the pmic variable to exist in the first place. It's also not clear why this API exists at all, surely all the interaction with the device happens from the core or function drivers for the device which ought to be able to get a direct reference to the regmap anyway and only be instantiated when one is present. We created these names to hide the implementation of how read/write is done from other platform specific patches interacting with this driver. So when we change the implementation, e.g. from I2C read/write to regmap, we don't have to touch all these patches. This sort of HAL is frowned upon in the upstream kernel. We want to do what other MFD drivers' been doing, and make it easier for the callers. A couple of similar examples are intel_msic_reg_read() and lp3943_read_byte(). We want to do the same with intel_soc_pmic_readb(), and I don't think it's too odd. Best Regards Lejun -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH RESEND v2 1/4] mfd: intel_soc_pmic: Core driver
On Fri, May 23, 2014 at 08:40:26AM +0800, Zhu, Lejun wrote:
> +struct device *intel_soc_pmic_dev(void)
> +{
> + return pmic->dev;
> +}
> +EXPORT_SYMBOL(intel_soc_pmic_dev);
Why do you need to take a global reference to this?
> +/*
> + * Read from a PMIC register
> + */
> +int intel_soc_pmic_readb(int reg)
> +{
> + int ret;
> + unsigned int val;
> +
> + mutex_lock(_lock);
> +
> + if (!pmic) {
> + ret = -EIO;
> + } else {
> + ret = regmap_read(pmic->regmap, reg, );
> + if (!ret)
> + ret = val;
> + }
> +
> + mutex_unlock(_lock);
> +
> + return ret;
> +}
> +EXPORT_SYMBOL(intel_soc_pmic_readb);
This is wrapping the EXPORT_SYMBOL_GPL() regmap API with an
EXPORT_SYMBOL() API. Don't do that, if you really do need these
wrappers make them EXPORT_SYMBOL_GPL().
There should also be no need to add extra locking around regmap calls,
the regmap API has locking as standard.
It's also not clear why this API exists at all, surely all the
interaction with the device happens from the core or function drivers
for the device which ought to be able to get a direct reference to the
regmap anyway and only be instantiated when one is present.
> +/*
> + * Set platform_data of the child devices. Needs to be called before
> + * the MFD device is added.
> + */
> +int intel_soc_pmic_set_pdata(const char *name, void *data, int len)
> +{
> + struct cell_dev_pdata *pdata;
> +
> + pdata = kzalloc(sizeof(*pdata), GFP_KERNEL);
> + if (!pdata) {
> + pr_err("intel_pmic: can't set pdata!\n");
> + return -ENOMEM;
> + }
> +
> + pdata->name = name;
> + pdata->data = data;
> + pdata->len = len;
> +
> + list_add_tail(>list, _list);
> +
> + return 0;
> +}
> +EXPORT_SYMBOL(intel_soc_pmic_set_pdata);
What is going on here, why aren't the normal ways of getting data to the
devices (such as reading the platform data of the parent device) OK?
> +static void __pmic_regmap_flush(void)
> +{
> + if (cache_write_pending)
> + WARN_ON(regmap_write(pmic->regmap, cache_offset,
> + cache_write_val));
> +static int pmic_regmap_load_from_hw(struct intel_pmic_regmap *map)
> +{
> + int ret;
This all appears to be an open coded cache layer - there is already
cache support in regmap, just reuse that.
> +static irqreturn_t pmic_irq_isr(int irq, void *data)
> +{
> + return IRQ_WAKE_THREAD;
> +}
Just register the IRQ as IRQF_ONESHOT and only provide the threaded
handler.
> +static irqreturn_t pmic_irq_thread(int irq, void *data)
> +{
> + int i;
> +
> + mutex_lock(>irq_lock);
> +
> + for (i = 0; i < pmic->irq_num; i++) {
> + if (test_bit(PMIC_IRQ_MASK_BIT(i), _IRQ_MASK(pmic, i)))
> + continue;
> +
> + if (pmic_regmap_read(>irq_regmap[i].status)) {
> + pmic_regmap_write(>irq_regmap[i].ack, 1);
> + handle_nested_irq(pmic->irq_base + i);
> + }
> + }
This looks like you should be using regmap-irq, or at least like there's
some small additions needed to make it usable.
> + if (gpio_is_valid(pmic->pmic_int_gpio)) {
> + ret = gpio_request_one(pmic->pmic_int_gpio,
> +GPIOF_DIR_IN, "PMIC Interupt");
> + if (ret) {
> + dev_err(pmic->dev, "Request PMIC_INT gpio error\n");
> + goto out_free_desc;
> + }
> +
> + pmic->irq = gpio_to_irq(pmic->pmic_int_gpio);
> + }
There should be no need to do this, simply requesting the interrupt
should be sufficient to ensure the pin is in the correct mode. If this
isn't the case the interrupt controller driver probably needs an update,
there's some support for helping with this in the GPIO framework IIRC.
You're also not using managed (devm) allocations for anything here.
signature.asc
Description: Digital signature
Re: [PATCH RESEND v2 1/4] mfd: intel_soc_pmic: Core driver
On Fri, May 23, 2014 at 08:40:26AM +0800, Zhu, Lejun wrote:
+struct device *intel_soc_pmic_dev(void)
+{
+ return pmic-dev;
+}
+EXPORT_SYMBOL(intel_soc_pmic_dev);
Why do you need to take a global reference to this?
+/*
+ * Read from a PMIC register
+ */
+int intel_soc_pmic_readb(int reg)
+{
+ int ret;
+ unsigned int val;
+
+ mutex_lock(pmic_lock);
+
+ if (!pmic) {
+ ret = -EIO;
+ } else {
+ ret = regmap_read(pmic-regmap, reg, val);
+ if (!ret)
+ ret = val;
+ }
+
+ mutex_unlock(pmic_lock);
+
+ return ret;
+}
+EXPORT_SYMBOL(intel_soc_pmic_readb);
This is wrapping the EXPORT_SYMBOL_GPL() regmap API with an
EXPORT_SYMBOL() API. Don't do that, if you really do need these
wrappers make them EXPORT_SYMBOL_GPL().
There should also be no need to add extra locking around regmap calls,
the regmap API has locking as standard.
It's also not clear why this API exists at all, surely all the
interaction with the device happens from the core or function drivers
for the device which ought to be able to get a direct reference to the
regmap anyway and only be instantiated when one is present.
+/*
+ * Set platform_data of the child devices. Needs to be called before
+ * the MFD device is added.
+ */
+int intel_soc_pmic_set_pdata(const char *name, void *data, int len)
+{
+ struct cell_dev_pdata *pdata;
+
+ pdata = kzalloc(sizeof(*pdata), GFP_KERNEL);
+ if (!pdata) {
+ pr_err(intel_pmic: can't set pdata!\n);
+ return -ENOMEM;
+ }
+
+ pdata-name = name;
+ pdata-data = data;
+ pdata-len = len;
+
+ list_add_tail(pdata-list, pdata_list);
+
+ return 0;
+}
+EXPORT_SYMBOL(intel_soc_pmic_set_pdata);
What is going on here, why aren't the normal ways of getting data to the
devices (such as reading the platform data of the parent device) OK?
+static void __pmic_regmap_flush(void)
+{
+ if (cache_write_pending)
+ WARN_ON(regmap_write(pmic-regmap, cache_offset,
+ cache_write_val));
+static int pmic_regmap_load_from_hw(struct intel_pmic_regmap *map)
+{
+ int ret;
This all appears to be an open coded cache layer - there is already
cache support in regmap, just reuse that.
+static irqreturn_t pmic_irq_isr(int irq, void *data)
+{
+ return IRQ_WAKE_THREAD;
+}
Just register the IRQ as IRQF_ONESHOT and only provide the threaded
handler.
+static irqreturn_t pmic_irq_thread(int irq, void *data)
+{
+ int i;
+
+ mutex_lock(pmic-irq_lock);
+
+ for (i = 0; i pmic-irq_num; i++) {
+ if (test_bit(PMIC_IRQ_MASK_BIT(i), PMIC_IRQ_MASK(pmic, i)))
+ continue;
+
+ if (pmic_regmap_read(pmic-irq_regmap[i].status)) {
+ pmic_regmap_write(pmic-irq_regmap[i].ack, 1);
+ handle_nested_irq(pmic-irq_base + i);
+ }
+ }
This looks like you should be using regmap-irq, or at least like there's
some small additions needed to make it usable.
+ if (gpio_is_valid(pmic-pmic_int_gpio)) {
+ ret = gpio_request_one(pmic-pmic_int_gpio,
+GPIOF_DIR_IN, PMIC Interupt);
+ if (ret) {
+ dev_err(pmic-dev, Request PMIC_INT gpio error\n);
+ goto out_free_desc;
+ }
+
+ pmic-irq = gpio_to_irq(pmic-pmic_int_gpio);
+ }
There should be no need to do this, simply requesting the interrupt
should be sufficient to ensure the pin is in the correct mode. If this
isn't the case the interrupt controller driver probably needs an update,
there's some support for helping with this in the GPIO framework IIRC.
You're also not using managed (devm) allocations for anything here.
signature.asc
Description: Digital signature
[PATCH RESEND v2 1/4] mfd: intel_soc_pmic: Core driver
This patch provides the common code for the intel_soc_pmic MFD driver,
such as read/write register and set up IRQ.
v2:
- Use regmap instead of our own callbacks for read/write.
- Add one missing EXPORT_SYMBOL.
- Remove some duplicate code and put them into pmic_regmap_load_from_hw.
Signed-off-by: Yang, Bin
Signed-off-by: Zhu, Lejun
---
drivers/mfd/intel_soc_pmic_core.c | 521 +
drivers/mfd/intel_soc_pmic_core.h | 83 ++
include/linux/mfd/intel_soc_pmic.h | 29 +++
3 files changed, 633 insertions(+)
create mode 100644 drivers/mfd/intel_soc_pmic_core.c
create mode 100644 drivers/mfd/intel_soc_pmic_core.h
create mode 100644 include/linux/mfd/intel_soc_pmic.h
diff --git a/drivers/mfd/intel_soc_pmic_core.c
b/drivers/mfd/intel_soc_pmic_core.c
new file mode 100644
index 000..76d366e
--- /dev/null
+++ b/drivers/mfd/intel_soc_pmic_core.c
@@ -0,0 +1,521 @@
+/*
+ * intel_soc_pmic_core.c - Intel SoC PMIC Core Functions
+ *
+ * Copyright (C) 2013, 2014 Intel Corporation. All rights reserved.
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License version
+ * 2 as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * Author: Yang, Bin
+ */
+
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include "intel_soc_pmic_core.h"
+
+struct cell_dev_pdata {
+ struct list_headlist;
+ const char *name;
+ void*data;
+ int len;
+};
+static LIST_HEAD(pdata_list);
+
+static DEFINE_MUTEX(pmic_lock);/* protect the following variables */
+static struct intel_soc_pmic *pmic;
+static int cache_offset = -1;
+static unsigned int cache_read_val;
+static unsigned int cache_write_val;
+static int cache_write_pending;
+static int cache_flags;
+
+struct device *intel_soc_pmic_dev(void)
+{
+ return pmic->dev;
+}
+EXPORT_SYMBOL(intel_soc_pmic_dev);
+
+/*
+ * Read from a PMIC register
+ */
+int intel_soc_pmic_readb(int reg)
+{
+ int ret;
+ unsigned int val;
+
+ mutex_lock(_lock);
+
+ if (!pmic) {
+ ret = -EIO;
+ } else {
+ ret = regmap_read(pmic->regmap, reg, );
+ if (!ret)
+ ret = val;
+ }
+
+ mutex_unlock(_lock);
+
+ return ret;
+}
+EXPORT_SYMBOL(intel_soc_pmic_readb);
+
+/*
+ * Write to a PMIC register
+ */
+int intel_soc_pmic_writeb(int reg, u8 val)
+{
+ int ret;
+
+ mutex_lock(_lock);
+
+ if (!pmic)
+ ret = -EIO;
+ else
+ ret = regmap_write(pmic->regmap, reg, val);
+
+ mutex_unlock(_lock);
+
+ return ret;
+}
+EXPORT_SYMBOL(intel_soc_pmic_writeb);
+
+/*
+ * Set 1 bit in a PMIC register
+ */
+int intel_soc_pmic_setb(int reg, u8 mask)
+{
+ int ret;
+
+ mutex_lock(_lock);
+
+ if (!pmic)
+ ret = -EIO;
+ else
+ ret = regmap_update_bits(pmic->regmap, reg, mask, mask);
+
+ mutex_unlock(_lock);
+
+ return ret;
+}
+EXPORT_SYMBOL(intel_soc_pmic_setb);
+
+/*
+ * Clear 1 bit in a PMIC register
+ */
+int intel_soc_pmic_clearb(int reg, u8 mask)
+{
+ int ret;
+
+ mutex_lock(_lock);
+
+ if (!pmic)
+ ret = -EIO;
+ else
+ ret = regmap_update_bits(pmic->regmap, reg, mask, 0);
+
+ mutex_unlock(_lock);
+
+ return ret;
+}
+EXPORT_SYMBOL(intel_soc_pmic_clearb);
+
+/*
+ * Set and clear multiple bits of a PMIC register
+ */
+int intel_soc_pmic_update(int reg, u8 val, u8 mask)
+{
+ int ret;
+
+ mutex_lock(_lock);
+
+ if (!pmic)
+ ret = -EIO;
+ else
+ ret = regmap_update_bits(pmic->regmap, reg, mask, val);
+
+ mutex_unlock(_lock);
+
+ return ret;
+}
+EXPORT_SYMBOL(intel_soc_pmic_update);
+
+/*
+ * Set platform_data of the child devices. Needs to be called before
+ * the MFD device is added.
+ */
+int intel_soc_pmic_set_pdata(const char *name, void *data, int len)
+{
+ struct cell_dev_pdata *pdata;
+
+ pdata = kzalloc(sizeof(*pdata), GFP_KERNEL);
+ if (!pdata) {
+ pr_err("intel_pmic: can't set pdata!\n");
+ return -ENOMEM;
+ }
+
+ pdata->name = name;
+ pdata->data = data;
+ pdata->len = len;
+
+ list_add_tail(>list, _list);
+
+ return 0;
+}
+EXPORT_SYMBOL(intel_soc_pmic_set_pdata);
+
+static void __pmic_regmap_flush(void)
+{
+ if (cache_write_pending)
+ WARN_ON(regmap_write(pmic->regmap, cache_offset,
+
[PATCH RESEND v2 1/4] mfd: intel_soc_pmic: Core driver
This patch provides the common code for the intel_soc_pmic MFD driver,
such as read/write register and set up IRQ.
v2:
- Use regmap instead of our own callbacks for read/write.
- Add one missing EXPORT_SYMBOL.
- Remove some duplicate code and put them into pmic_regmap_load_from_hw.
Signed-off-by: Yang, Bin bin.y...@intel.com
Signed-off-by: Zhu, Lejun lejun@linux.intel.com
---
drivers/mfd/intel_soc_pmic_core.c | 521 +
drivers/mfd/intel_soc_pmic_core.h | 83 ++
include/linux/mfd/intel_soc_pmic.h | 29 +++
3 files changed, 633 insertions(+)
create mode 100644 drivers/mfd/intel_soc_pmic_core.c
create mode 100644 drivers/mfd/intel_soc_pmic_core.h
create mode 100644 include/linux/mfd/intel_soc_pmic.h
diff --git a/drivers/mfd/intel_soc_pmic_core.c
b/drivers/mfd/intel_soc_pmic_core.c
new file mode 100644
index 000..76d366e
--- /dev/null
+++ b/drivers/mfd/intel_soc_pmic_core.c
@@ -0,0 +1,521 @@
+/*
+ * intel_soc_pmic_core.c - Intel SoC PMIC Core Functions
+ *
+ * Copyright (C) 2013, 2014 Intel Corporation. All rights reserved.
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License version
+ * 2 as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * Author: Yang, Bin bin.y...@intel.com
+ */
+
+#include linux/kernel.h
+#include linux/module.h
+#include linux/delay.h
+#include linux/mutex.h
+#include linux/mfd/core.h
+#include linux/err.h
+#include linux/irq.h
+#include linux/interrupt.h
+#include linux/workqueue.h
+#include linux/acpi.h
+#include linux/version.h
+#include linux/gpio.h
+#include linux/regmap.h
+#include linux/mfd/intel_soc_pmic.h
+#include intel_soc_pmic_core.h
+
+struct cell_dev_pdata {
+ struct list_headlist;
+ const char *name;
+ void*data;
+ int len;
+};
+static LIST_HEAD(pdata_list);
+
+static DEFINE_MUTEX(pmic_lock);/* protect the following variables */
+static struct intel_soc_pmic *pmic;
+static int cache_offset = -1;
+static unsigned int cache_read_val;
+static unsigned int cache_write_val;
+static int cache_write_pending;
+static int cache_flags;
+
+struct device *intel_soc_pmic_dev(void)
+{
+ return pmic-dev;
+}
+EXPORT_SYMBOL(intel_soc_pmic_dev);
+
+/*
+ * Read from a PMIC register
+ */
+int intel_soc_pmic_readb(int reg)
+{
+ int ret;
+ unsigned int val;
+
+ mutex_lock(pmic_lock);
+
+ if (!pmic) {
+ ret = -EIO;
+ } else {
+ ret = regmap_read(pmic-regmap, reg, val);
+ if (!ret)
+ ret = val;
+ }
+
+ mutex_unlock(pmic_lock);
+
+ return ret;
+}
+EXPORT_SYMBOL(intel_soc_pmic_readb);
+
+/*
+ * Write to a PMIC register
+ */
+int intel_soc_pmic_writeb(int reg, u8 val)
+{
+ int ret;
+
+ mutex_lock(pmic_lock);
+
+ if (!pmic)
+ ret = -EIO;
+ else
+ ret = regmap_write(pmic-regmap, reg, val);
+
+ mutex_unlock(pmic_lock);
+
+ return ret;
+}
+EXPORT_SYMBOL(intel_soc_pmic_writeb);
+
+/*
+ * Set 1 bit in a PMIC register
+ */
+int intel_soc_pmic_setb(int reg, u8 mask)
+{
+ int ret;
+
+ mutex_lock(pmic_lock);
+
+ if (!pmic)
+ ret = -EIO;
+ else
+ ret = regmap_update_bits(pmic-regmap, reg, mask, mask);
+
+ mutex_unlock(pmic_lock);
+
+ return ret;
+}
+EXPORT_SYMBOL(intel_soc_pmic_setb);
+
+/*
+ * Clear 1 bit in a PMIC register
+ */
+int intel_soc_pmic_clearb(int reg, u8 mask)
+{
+ int ret;
+
+ mutex_lock(pmic_lock);
+
+ if (!pmic)
+ ret = -EIO;
+ else
+ ret = regmap_update_bits(pmic-regmap, reg, mask, 0);
+
+ mutex_unlock(pmic_lock);
+
+ return ret;
+}
+EXPORT_SYMBOL(intel_soc_pmic_clearb);
+
+/*
+ * Set and clear multiple bits of a PMIC register
+ */
+int intel_soc_pmic_update(int reg, u8 val, u8 mask)
+{
+ int ret;
+
+ mutex_lock(pmic_lock);
+
+ if (!pmic)
+ ret = -EIO;
+ else
+ ret = regmap_update_bits(pmic-regmap, reg, mask, val);
+
+ mutex_unlock(pmic_lock);
+
+ return ret;
+}
+EXPORT_SYMBOL(intel_soc_pmic_update);
+
+/*
+ * Set platform_data of the child devices. Needs to be called before
+ * the MFD device is added.
+ */
+int intel_soc_pmic_set_pdata(const char *name, void *data, int len)
+{
+ struct cell_dev_pdata *pdata;
+
+ pdata = kzalloc(sizeof(*pdata), GFP_KERNEL);
+ if (!pdata) {
+ pr_err(intel_pmic: can't set pdata!\n);
+ return -ENOMEM;
+ }
+
+ pdata-name = name;
+ pdata-data = data;
+
