lkml.org 
[lkml]   [2017]   [Feb]   [9]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
Date
SubjectRe: [PATCH v2] eeprom/at24: use device_property_*() functions instead of of_get_property()
> Still couple of comments.
>
>> @@ -19,7 +19,6 @@
>> #include <linux/log2.h>
>> #include <linux/bitops.h>
>> #include <linux/jiffies.h>
>
>> -#include <linux/of.h>
>
> I suppose
> + #include <linux/property.h>
> ?

It looks like <linux/property.h> is included via <linux/acpi.h>
outside of any "#ifdef CONFIG_ACPI" check.
But Documentation/process/submit-checklist.rst indicates:

1) If you use a facility then #include the file that defines/declares
that facility. Don't depend on other header files pulling in ones
that you use.

So, I'll add it.
As an aside, a quick scan of the drivers/ folder shows that fewer than
half of the files that use functions from that header actually include
it.

>> +static void at24_get_pdata(struct device *dev,
>> + struct at24_platform_data *chip)
>
> Now it fits one line.
Sure does. Exactly 80 columns.

>> + u32 val;
>> +
>> + if (device_property_present(dev, "read-only"))
>> + chip->flags |= AT24_FLAG_READONLY;
>> +
>> + if (device_property_read_u32(dev, "pagesize", &val) == 0) {
>
> ' == 0' looks awkward.
>
> And I think
>
> ret = x();
> if (ret)
> ...
> else
> ...
>
> pattern would look slightly better.

There are 51 C files that use a temporary variable as you suggest with
"device_property_read" functions.
There are 7 C files that use the "if (!device_property_read" style.
It looks like at25.c was the only one that uses the '== 0' construct.
Guess I picked the wrong one to copy.
So, sure, I'll change that.

>> + chip->page_size = val;
>> + } else {
>> + /*
>> + * This is slow, but we can't know all eeproms, so we better
>> + * play safe. Specifying custom eeprom-types via platform_data
>> + * is recommended anyhow.
>> + */
>> + chip.page_size = 1;
>
> --
> With Best Regards,
> Andy Shevchenko

\
 
 \ /
  Last update: 2017-02-09 19:41    [W:0.150 / U:0.160 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site