linux-kernel - Re: [PATCH v2 1/3] iio: Add driver for Infineon DPS310

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]

Message-Id: <cf108058-dd50-4651-6c97-a5c7e82ef085@linux.ibm.com>
Date:   Wed, 15 May 2019 13:46:46 -0500
From:   Eddie James <eajames@...ux.ibm.com>
To:     Jonathan Cameron <jic23@...nel.org>
Cc:     linux-iio@...r.kernel.org, linux-kernel@...r.kernel.org,
        joel@....id.au, pmeerw@...erw.net, lars@...afoo.de, knaack.h@....de
Subject: Re: [PATCH v2 1/3] iio: Add driver for Infineon DPS310


On 5/11/19 4:22 AM, Jonathan Cameron wrote:
> On Wed,  8 May 2019 14:35:26 -0500
> Eddie James <eajames@...ux.ibm.com> wrote:
>
>> From: Joel Stanley <joel@....id.au>
>>
>> The DPS310 is a temperature and pressure sensor. It can be accessed over
>> i2c and SPI.
>>
>> Signed-off-by: Eddie James <eajames@...ux.ibm.com>
> Hi Eddie,
>
> Ideally we'll get a sign off form Joel as well on this.
>
> A few comments inline.
>
> I 'think' this is probably fine without any locking to prevent simultaneous reads
> and /or writes to the registers because the few functions that do multiple reads
> and writes look fine.  Please do take another look at that though to confirm there
> are no corner cases.
>
> Otherwise there is a race in the remove path that needs fixing.
> Various minor bits and bobs inline.
>
> thanks,
>
> Jonathan
>
>
>
>> ---
>>   MAINTAINERS                   |   6 +
>>   drivers/iio/pressure/Kconfig  |  10 +
>>   drivers/iio/pressure/Makefile |   1 +
>>   drivers/iio/pressure/dps310.c | 429 ++++++++++++++++++++++++++++++++++++++++++
>>   4 files changed, 446 insertions(+)
>>   create mode 100644 drivers/iio/pressure/dps310.c
>>

>> +};
>> +MODULE_DEVICE_TABLE(i2c, dps310_id);
>> +
>> +static const unsigned short normal_i2c[] = {
>> +	0x77, 0x76, I2C_CLIENT_END
>> +};
>> +
>> +static struct i2c_driver dps310_driver = {
>> +	.driver = {
>> +		.name = "dps310",
>> +	},
>> +	.probe = dps310_probe,
>> +	.remove = dps310_remove,
>> +	.address_list = normal_i2c,
> I'm fairly sure the address list is only used along with the detection
> infrastructure.  As such it doesn't actually provide any value unless
> you have a detect callback.  Please remove.
>
> I would like to see a DT and/or ACPI binding though as that is the
> means most people will use to find the device.


Somehow the device is already present in the witherspoon device tree 
where it's currently being used, so I don't have anything to add. 
arch/arm/boot/dts/aspeed-bmc-opp-witherspoon.dts

Thanks,

Eddie


>
>> +	.id_table = dps310_id,
>> +};
>> +module_i2c_driver(dps310_driver);
>> +
>> +MODULE_AUTHOR("Joel Stanley <joel@....id.au>");
>> +MODULE_DESCRIPTION("Infineon DPS310 pressure and temperature sensor");
>> +MODULE_LICENSE("GPL v2");