lkml.org 
[lkml]   [2021]   [Jan]   [19]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH v2 6/7] platform: x86: Add intel_skl_int3472 driver
    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?

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

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

    --
    With Best Regards,
    Andy Shevchenko


    \
     
     \ /
      Last update: 2021-01-19 22:41    [W:3.073 / U:0.456 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site