Re: [PATCH v2 1/7] mfd: Add support for DA9150 combined charger & fuel-gauge device

[Lee Jones <lee.jones@linaro.org>]

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 <Adam.Thomson.Opensource@diasemi.com>
> > > > > ---
> > > > >  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.

> > > > > +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_id);
> > > > > +}
> > > > > +EXPORT_SYMBOL_GPL(da9150_release_irq);
> > > >
> > > > Do you ever release the IRQ and not unbind the driver?
> > > >
> > > > Are there ordering issues at play here?
> > > >
> > > > If not, there's no need to conduct a manual free.
> > >
> > > In the charger driver, in the remove function there is a need I believe to
> > > free the IRQs before other items are cleared up (e.g. power_supply classes),
> > > so this is why I have added this in here.
> > 
> > Can you handle this separately in the Charger driver then please?
> > 
> > [...]
> 
> If I have to remove the IRQ register function, then yes, otherwise it makes more
> sense to have the pair of functions in the MFD core I would say.

I would prefer you to remove the call-back please.

> > > > > +	if (pdata)
> > > > > +		da9150->irq_base = pdata->irq_base;
> > > > > +	else
> > > > > +		da9150->irq_base = -1;
> > > >
> > > > pdata ? pdata->irq_base : -1;
> > >
> > > This is left this way as later updates to add additional functionality will
> > > require addtional work to be done with the platform data. Seemed pointless
> > > changing it here just to change it back later.
> > 
> > You're not changing anything, as this is the introduction of the code.
> > I can't tell you how many times I've heard "I will change it later",
> > or "doing it this way will support subsequent submissions", then
> > received no more patches.  It's okay to do it nicely now and expand
> > it back out in the new patches.
> > 
> > [...]
> 
> It appears that way to you but I have to modify my code for sumbission as the
> local code I have covers all functionality. Am having to refactor again and
> again just to suit this initial submission, and then I have to revert it back
> again when submitting the last couple of drivers. Time consuming, and quite
> frustrating when the intention was to make the whole process easier. Anyway,
> will update for now and revert in subsequent patches.

I sincerely hope the refactorings won't add too much effort, but it's
difficult to have one rule for the masses and different ones for
others.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog