Messages in this thread | | | Date | Wed, 27 Jul 2022 18:40:37 +0800 | Subject | Re: [PATCH v2 1/1] iio: humidity: hdc100x: add manufacturer and device ID cehck | From | Potin Lai <> |
| |
On 7/27/22 18:00, Andy Shevchenko wrote: > On Wed, Jul 27, 2022 at 8:46 AM Potin Lai <potin.lai.pt@gmail.com> wrote: >> Add manufacturer and device ID checking during probe, and skip the >> checking if chip model not supported. >> >> Supported: >> - HDC1000 >> - HDC1010 >> - HDC1050 >> - HDC1080 >> >> Not supported: >> - HDC1008 > Thanks for an update, my comments below. > > ... > >> + const struct of_device_id *match; > Don't you have a compiler warning? Always compile your code with `make W=1` > > ...
You are correct, I will remove this unused variable.
>> + data = device_get_match_data(&client->dev); >> + if (data) { > This check is redundant. Too much protective programming. Just oblige > that matched ID has to always have an associated data. Is it guaranteed that device_get_match_data will not return NULL? I find some examples in other drivers, all of them have a check on returned data.
Will it be more appropriate if I move device_get_match_data to probe function, and return EINVAL when we get a NULL pointer from device_get_match_data?
>> + if (!data->support_mfr_check) >> + return true; >> + } > ... > >> - .probe = hdc100x_probe, >> + .probe_new = hdc100x_probe, > Make this a separate patch before the presented one. > got it, will move this into a separate patch in next version.
thanks, Potin
| |