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

[Opensource [Adam Thomson]]

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
> <Adam.Thomson.Opensource@xxxxxxxxxxx>
> > > > > > ---
> > > > > >  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_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.

Right.

> 
> > > > > > +	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.

I do understand that, and that's fair enough. Is just frustrating when you're
trying to do a proper job. Anyway, am sure I'll live. :)

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