Re: [PATCH RESEND v2 1/4] mfd: intel_soc_pmic: Core driver

[Mark Brown <broonie@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 3603 bytes --]

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.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]