Re: [PATCH v2 2/2] IIO: st_accel_i2c.c: Use probe_new() instead of probe()

[Javier Martinez Canillas]

On Wed, Jul 4, 2018 at 1:46 PM, Nikolaus Voss
<nikolaus.voss@xxxxxxxxxxxxxxxxxxxxx> wrote:

[snip]

>>>
>>> Ok, in my opinion it is an elegant way of not bloating the driver when no
>>> explicit handling (e.g. reading DT properties) is needed. Just adding an
>>> of_device_id doesn't change any driver functionality then but only maps
>>> to
>>> an already existing i2c_table_id.
>>>
>>
>> I disagree, in the case of OF for example a compatible string is
>> composed of both a vendor a device, the complete tuple is what should
>> be matched.
>>
>> But with the fallback, only the device portion would be used so both
>> <foo,bar> and <baz,bar> will match against the i2c device with id
>> "bar". It may or may not be correct but the vendor portion is very
>> important to disambiguate.
>
>
> Disambiguation via DT implies there is already a name collision in i2c
> modalias name space, so adding an of_id would work around this collision for

There wouldn't be a name collision between OF modaliases in that case
since the vendor would be different (of:N*T*Cfoo,bar and
of:N*T*Cbaz,bar). So it wouldn't be a problem for DT-only drivers.

> DT enumeration. I2c_board_info driver binding would still be broken. The
> right fix would be to remove the name collision.
>

Yes, for legacy drivers using board files it would be an issue. But
that's unrelated to what I'm saying. What I don't think is correct is
to ignore the vendor prefix since that's part of the compatible string
semantics.

In general, I think that there should be consistency between the
firmware interface used to register the device, how the module alias
uevent is reported to auto-load a driver and how the driver is matched
with the device. So drivers supporting DT should have a n OF table
(and export it with MODULE_DEVICE_TABLE), drivers supporting ACPI
should have a ACPI table and so on.

But this discussion isn't really related to your patch. I think is
correct but just said that (b) wasn't a justification to leave the I2C
table, points (a) and (c) are though. I won't really be convinced that
the fallback is the correct thing to do or even a good idea.

[snip]

>>>
>>> It could have been a null pointer and device driver binding (and
>>> auto-loading) done just via driver.name.
>>>
>>
>> I'm not sure I understood this comment.
>
>
> What I was trying to say is that if the i2c_device_id table information was
> not needed (i.d. only one single id), even the old probe() could be used
> without defining an i2c_device_id table, the i2c_device_id* argument to
> probe() being a nullptr.
>

I see, yes that would work too. I didn't authored the .probe_new
callback so I don't know if other options were discussed. I also can't
remember if the I2C core attempted to probe even without an I2C device
ID table, I thought it didn't but I can be wrong.

> Niko

Best regards,
Javier