Date:	Mon, 26 May 2014 14:01:11 +0800
From:	"Zhu, Lejun" <lejun.zhu@...ux.intel.com>
To:	Mark Brown <broonie@...nel.org>
CC:	lee.jones@...aro.org, sameo@...ux.intel.com,
	linux-kernel@...r.kernel.org, jacob.jun.pan@...ux.intel.com,
	bin.yang@...el.com
Subject: 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 majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists