Re: [PATCH v2 3/4] pwm: lpss: Do not export board infos for different PWM types

[Thierry Reding <thierry.reding@gmail.com>]

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

On Wed, Jan 18, 2017 at 03:01:38PM +0200, Mika Westerberg wrote:
> On Wed, Jan 18, 2017 at 12:11:09PM +0100, Thierry Reding wrote:
> > On Mon, Jan 02, 2017 at 11:16:46AM +0200, Andy Shevchenko wrote:
> > > From: Mika Westerberg <mika.westerberg@linux.intel.com>
> > > 
> > > The PWM LPSS probe drivers just pass a pointer to the exported board info
> > > structures to pwm_lpss_probe() based on device PCI or ACPI ID. Since the
> > > core driver knows everything else except mapping between device ID and the
> > > type, just pass the type with pwm_lpss_probe() and stop exporting the board
> > > info structures.
> > > 
> > > Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> > > ---
> > >  drivers/pwm/pwm-lpss-pci.c      | 20 ++++++++---------
> > >  drivers/pwm/pwm-lpss-platform.c | 10 ++++-----
> > >  drivers/pwm/pwm-lpss.c          | 49 ++++++++++++++++++++++-------------------
> > >  drivers/pwm/pwm-lpss.h          | 14 +++++-------
> > >  4 files changed, 44 insertions(+), 49 deletions(-)
> > 
> > Is there anything in particular that you think will need this change? It
> > looks to me more like churn than anything else. Moving away from the per
> > device struct to describe the particular instance seems to me like
> > removing flexibility that we might want at some point rather than any
> > real gain.
> 
> It simplifies the probe drivers for one. Since the core driver already
> handles details of the specific SoC family, I don't think we need the
> flexibility to be able to pass arbitrary platform data anyway.
> 
> No strong feelings, though. I'm fine either way :)

The current driver uses a strange inversion of the abstraction layer.
For one we have a "board info" structure that is supposed to describe
the variants of the hardware that exist. That data is in the core
driver, for reasons that I no longer remember, and then the PCI and
ACPI drivers reference those info structures depending on the type of
hardware they bind to. And worse, we now have to export symbols to
the PCI and ACPI drivers to make use of them.

I think this is the wrong way around. The core would ideally be unaware
of any particular variants and use only the struct pwm_lpss_boardinfo.
It would then be up to the ACPI and PCI drivers to provide the variants
they need.

Perhaps the only reason why the board info structures are in the core
driver is because the same variant exists as PCI and ACPI devices, so
putting them in the core removes potential duplication.

What I'm saying is that its wrong to have board specific bits in the
core driver. Duplicating the board info isn't a very attractive
alternative either, though, so it's not going to be elegant either way.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]