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

[Laurent Pinchart]

Hi Andy,

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.

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

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

-- 
Regards,

Laurent Pinchart