linux-kernel - Re: [RESEND PATCH v4 0/8] i2c: Relax mandatory I2C ID table passing

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Thu, 24 Sep 2015 19:32:34 +0200
From:	Javier Martinez Canillas <javier@....samsung.com>
To:	Lee Jones <lee.jones@...aro.org>
Cc:	Kieran Bingham <kieranbingham@...il.com>,
	Wolfram Sang <wsa@...-dreams.de>,
	Samuel Ortiz <sameo@...ux.intel.com>,
	linux-i2c@...r.kernel.org, linux-kernel@...r.kernel.org,
	grant.likely@...aro.org
Subject: Re: [RESEND PATCH v4 0/8] i2c: Relax mandatory I2C ID table passing

Hello Lee,

On 09/24/2015 06:58 PM, Lee Jones wrote:

[snip]

>>
>>> Drivers will know if they either only supply an I2C or OF table, so
>>> they will know which call to use in order to obtain their
>>
>> Yes but that is not true for drivers that support both OF and legacy board
>> files. For those drivers, there will be a lot of boiler plate code duplicated
>> that would look something like:
>>
>>      unsigned long data;
>>      struct of_device_id *match;
>>      struct i2c_devicd_id *id;
>>
>>      if (i2c->dev.of_node) {
>>             match = i2c_of_match_device(of_match_table, i2c);
>> 	    if (!match)
>> 	           return -EINVAL;
>>
>>             data = (unsigned long)match->data;
>>      } else {
>>             id = i2c_match_id(id_table, i2c);
>> 	    if (!id)
>> 	           return -EINVAL;
>>
>>             data = id->driver_data;
>>      }
>>
>> While it would be nice to have something like:
>>
>>     data = i2c_get_data(i2c);
>>
>> and let the core handle which table should be looked up depending on
>> which mechanism was used to register the i2c device (legacy or OF).
> 
> I'm fine with a new API for this stuff.  I'm even happy to go ahead
> and code it up, but it's important to note that this is work which
> should be based on this set and not a blocker for this set to be
> accepted.
>

I didn't mean this should be a blocker and yes can be done as a follow up.
 
>>> .driver_data|.data. attributes.  We can generify the call if you think
>>> that makes things easier, but I don't see a need for it ATM.
>>>
>>
>> As I explained above, it will make easier for drivers but I raised the
>> point to discuss if the table data should be looked up by the driver
>> or if the core should get it and pass to the probe() function as it is
>> made right now for the I2C device ID table. i.e:
>>
>> static int foo_i2c_probe(struct i2c_client *i2c, const void *data)
>>
>> If the correct approach is the former, then this series is the right
>> direction and as you said a generic match function can be added later.
>>
>> But if the correct approach is the latter, then this series is not
>> the right direction and a different approach is needed. I don't have
>> a strong opinion but wanted to mention that we have two options here.
> 
> The correct approach is the former.  One of the aims of this set was
> to bring the I2C .probe() call-back more into line with the majority
> of the other .probe() calls in the kernel i.e. with only a single
> parameter.  I'm really not a fan of passing some random void pointer
> in.  Using a look-up call to fetch ACPI/OF/I2C/etc data is the current
> norm and is a very viable option.
>

Ok, as I said I don't have a strong opinion and you are right that this
set will make I2C to be more aligned with other subsystems (i.e: SPI that
the I2C implementation is very similar to).

> Wolfram, please (finally :D) take this set.
>

Indeed :)

Best regards,
-- 
Javier Martinez Canillas
Open Source Group
Samsung Research America
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/