Re: [PATCH v2 6/7] platform: x86: Add intel_skl_int3472 driver

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

On Wed, Jan 20, 2021 at 06:21:41AM +0200, Laurent Pinchart wrote:
> On Tue, Jan 19, 2021 at 07:51:14PM +0200, Andy Shevchenko wrote:
> > On Tue, Jan 19, 2021 at 06:48:15PM +0200, Laurent Pinchart wrote:
> > > On Tue, Jan 19, 2021 at 01:08:37PM +0200, Andy Shevchenko wrote:
> > > > On Tue, Jan 19, 2021 at 10:40:42AM +0000, Daniel Scally wrote:
> > > > > On 19/01/2021 09:24, Andy Shevchenko wrote:
> > > > > >>>>> +static struct i2c_driver int3472_tps68470 = {
> > > > > >>>>> +	.driver = {
> > > > > >>>>> +		.name = "int3472-tps68470",
> > > > > >>>>> +		.acpi_match_table = int3472_device_id,
> > > > > >>>>> +	},
> > > > > >>>>> +	.probe_new = skl_int3472_tps68470_probe,
> > > > > >>>>> +};
> > > > > >>> I'm not sure we want to have like this. If I'm not mistaken the I²C driver can
> > > > > >>> be separated without ACPI IDs (just having I²C IDs) and you may instantiate it
> > > > > >>> via i2c_new_client_device() or i2c_acpi_new_device() whichever suits better...
> > > > > >> Sorry, I'm a bit confused by this. The i2c device is already
> > > > > >> present...we just want the driver to bind to them, so what role do those
> > > > > >> functions have there?
> > > > > > What I meant is something like
> > > > > >
> > > > > >  *_i2c.c
> > > > > > 	real I²C driver for the TPS chip, but solely with I²C ID table, no ACPI
> > > > > > 	involved (and it sounds like it should be mfd/tps one, in which you
> > > > > > 	just cut out ACPI IDs and convert to pure I²C one, that what I had
> > > > > > 	suggested in the first place)
> > > > > 
> > > > > Ahh; sorry - i misunderstood what you meant there. I understand now I
> > > > > think, but there is one complication; the ACPI subsystem already creates
> > > > > a client for that i2c adapter and address; i2c_new_client_device()
> > > > > includes a check to see whether that adapter / address combination has
> > > > > an i2c device already.  So we would have to have the platform driver
> > > > > with ACPI ID first find the existing i2c_client and unregister it before
> > > > > registering the new one...the existing clients have a name matching the
> > > > > ACPI device instance name (e.g i2c-INT3472:00) which we can't use as an
> > > > > i2c_device_id of course.
> > > > 
> > > > See how INT33FE is being handled. Hint: drivers/acpi/scan.c:~1600
> > > > 
> > > > static const struct acpi_device_id i2c_multi_instantiate_ids[] = {
> > > > 	{"BSG1160", },
> > > > 	{"BSG2150", },
> > > > 	{"INT33FE", },
> > > > 	{"INT3515", },
> > > > 	{}
> > > > };
> > > > 
> > > > So, we quirklist it here and instantiate manually from platform driver (new
> > > > coming one).
> > > 
> > > This is documented as used for devices that have multiple I2cSerialBus
> > > resources. That's not the case for the INT3472 as far as I can tell. I
> > > don't think we should abuse this mechanism.
> > 
> > This is quite a similar case to that one. Let's avoid yak shaving, right?
> 
> Exactly my point, that's why I think this patch is good overall, I don't
> think it requires a complete rewrite.

The approach in the series is to reinvent the MFD driver which I against of.
I don;t think we need to kill it there and reborn in a new form and dragging
code from there to here to there.

On top of that the approach with a quirk driver in the middle seems to me
cleaner than using different paths how the two drivers are being initialized.
In the proposed approach there will be one making decision point and easy to
understand what's going on.

The bad example of two making decision points is acpi_lpss.c vs. individual
drivers (however in that case it have different ID's, i.e. ACPI vs. PCI),

> > > Don't forget that the TPS68470 I2C driver needs to be ACPI-aware, as it
> > > has to register an OpRegion for ACPI-based Chrome OS devices. On other
> > > platforms (including DT platforms), it should only register regulators,
> > > clocks and GPIOs. Given the differences between those platforms, I don't
> > > think a TPS68470 driver that would fake being unaware of being probed
> > > through ACPI would be a good idea. We can always refactor the code later
> > > when we'll have a non-ACPI based platform using the TPS68470, without
> > > such a platform there's no way we can test the I2C driver without ACPI
> > > anyway.
> > 
> > Are you agree that MFD approach should stay? How then we can manage to have an
> > MFD driver cohabit with our new driver? I proposed a clean solution which will
> > handle all possible cases via quirk driver. Having two drivers enumerated by
> > different scenarios is a call for troubles (we have already with one of that
> > sensors).
> 
> I think we should solve this problem when it will arise. Solving
> problems with complex architectures without a platform to test the code
> on is a pretty sure way to get the architecture design wrong. Let's get
> this merged, it's an improvement compared to the current situation, and
> then let's improve it further on top when we'll need to support more use
> cases.

But this is problem already here right now. The submitted code is to support
a new platform that needs a quirk and treats INT3472 differently. The usual
way is to refactor the existing solution to make them both to have a best
compromise.

> > And there is no "faking" anything, it's rather gating it depending on the
> > platform.

-- 
With Best Regards,
Andy Shevchenko