* [PATCH 1/7] eeprom: at24: don't explicitely include header files which are implicitely included
2017-11-30 6:40 [PATCH 0/7] eeprom: at24: series with smaller improvements Heiner Kallweit
@ 2017-11-30 6:48 ` Heiner Kallweit
2017-11-30 15:56 ` Peter Rosin
2017-11-30 6:49 ` [PATCH 2/7] eeprom: at24: consider that SERIAL and MAC flags imply read-only Heiner Kallweit
` (5 subsequent siblings)
6 siblings, 1 reply; 16+ messages in thread
From: Heiner Kallweit @ 2017-11-30 6:48 UTC (permalink / raw)
To: Bartosz Golaszewski; +Cc: linux-i2c@vger.kernel.org
Don't explicitely include header files which are implicitely included.
Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
drivers/misc/eeprom/at24.c | 5 -----
1 file changed, 5 deletions(-)
diff --git a/drivers/misc/eeprom/at24.c b/drivers/misc/eeprom/at24.c
index cd87f1b21..c75bb9b45 100644
--- a/drivers/misc/eeprom/at24.c
+++ b/drivers/misc/eeprom/at24.c
@@ -10,15 +10,10 @@
* (at your option) any later version.
*/
#include <linux/kernel.h>
-#include <linux/init.h>
#include <linux/module.h>
#include <linux/of_device.h>
#include <linux/slab.h>
#include <linux/delay.h>
-#include <linux/mutex.h>
-#include <linux/mod_devicetable.h>
-#include <linux/log2.h>
-#include <linux/bitops.h>
#include <linux/jiffies.h>
#include <linux/property.h>
#include <linux/acpi.h>
--
2.15.0
^ permalink raw reply related [flat|nested] 16+ messages in thread* Re: [PATCH 1/7] eeprom: at24: don't explicitely include header files which are implicitely included
2017-11-30 6:48 ` [PATCH 1/7] eeprom: at24: don't explicitely include header files which are implicitely included Heiner Kallweit
@ 2017-11-30 15:56 ` Peter Rosin
2017-11-30 19:32 ` Heiner Kallweit
0 siblings, 1 reply; 16+ messages in thread
From: Peter Rosin @ 2017-11-30 15:56 UTC (permalink / raw)
To: Heiner Kallweit, Bartosz Golaszewski; +Cc: linux-i2c@vger.kernel.org
On 2017-11-30 07:48, Heiner Kallweit wrote:
> Don't explicitely include header files which are implicitely included.
That's backwards. It hinders future header rearrangement. Things that
are used directly (e.g. the BIT macro) should have its respective
include specified (bitops.h in that case).
It's not a competition for the shortest file, the goal is easy
maintenance.
Cheers,
Peter
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> ---
> drivers/misc/eeprom/at24.c | 5 -----
> 1 file changed, 5 deletions(-)
>
> diff --git a/drivers/misc/eeprom/at24.c b/drivers/misc/eeprom/at24.c
> index cd87f1b21..c75bb9b45 100644
> --- a/drivers/misc/eeprom/at24.c
> +++ b/drivers/misc/eeprom/at24.c
> @@ -10,15 +10,10 @@
> * (at your option) any later version.
> */
> #include <linux/kernel.h>
> -#include <linux/init.h>
> #include <linux/module.h>
> #include <linux/of_device.h>
> #include <linux/slab.h>
> #include <linux/delay.h>
> -#include <linux/mutex.h>
> -#include <linux/mod_devicetable.h>
> -#include <linux/log2.h>
> -#include <linux/bitops.h>
> #include <linux/jiffies.h>
> #include <linux/property.h>
> #include <linux/acpi.h>
>
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 1/7] eeprom: at24: don't explicitely include header files which are implicitely included
2017-11-30 15:56 ` Peter Rosin
@ 2017-11-30 19:32 ` Heiner Kallweit
2017-12-01 10:09 ` Bartosz Golaszewski
0 siblings, 1 reply; 16+ messages in thread
From: Heiner Kallweit @ 2017-11-30 19:32 UTC (permalink / raw)
To: Peter Rosin, Bartosz Golaszewski; +Cc: linux-i2c@vger.kernel.org
Am 30.11.2017 um 16:56 schrieb Peter Rosin:
> On 2017-11-30 07:48, Heiner Kallweit wrote:
>> Don't explicitely include header files which are implicitely included.
>
> That's backwards. It hinders future header rearrangement. Things that
> are used directly (e.g. the BIT macro) should have its respective
> include specified (bitops.h in that case).
>
> It's not a competition for the shortest file, the goal is easy
> maintenance.
>
Understood ..
@Bartosz: then you can ignore this one.
Rgds, Heiner
> Cheers,
> Peter
>
>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
>> ---
>> drivers/misc/eeprom/at24.c | 5 -----
>> 1 file changed, 5 deletions(-)
>>
>> diff --git a/drivers/misc/eeprom/at24.c b/drivers/misc/eeprom/at24.c
>> index cd87f1b21..c75bb9b45 100644
>> --- a/drivers/misc/eeprom/at24.c
>> +++ b/drivers/misc/eeprom/at24.c
>> @@ -10,15 +10,10 @@
>> * (at your option) any later version.
>> */
>> #include <linux/kernel.h>
>> -#include <linux/init.h>
>> #include <linux/module.h>
>> #include <linux/of_device.h>
>> #include <linux/slab.h>
>> #include <linux/delay.h>
>> -#include <linux/mutex.h>
>> -#include <linux/mod_devicetable.h>
>> -#include <linux/log2.h>
>> -#include <linux/bitops.h>
>> #include <linux/jiffies.h>
>> #include <linux/property.h>
>> #include <linux/acpi.h>
>>
>
>
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 1/7] eeprom: at24: don't explicitely include header files which are implicitely included
2017-11-30 19:32 ` Heiner Kallweit
@ 2017-12-01 10:09 ` Bartosz Golaszewski
0 siblings, 0 replies; 16+ messages in thread
From: Bartosz Golaszewski @ 2017-12-01 10:09 UTC (permalink / raw)
To: Heiner Kallweit; +Cc: Peter Rosin, linux-i2c@vger.kernel.org
2017-11-30 20:32 GMT+01:00 Heiner Kallweit <hkallweit1@gmail.com>:
> Am 30.11.2017 um 16:56 schrieb Peter Rosin:
>> On 2017-11-30 07:48, Heiner Kallweit wrote:
>>> Don't explicitely include header files which are implicitely included.
>>
>> That's backwards. It hinders future header rearrangement. Things that
>> are used directly (e.g. the BIT macro) should have its respective
>> include specified (bitops.h in that case).
>>
>> It's not a competition for the shortest file, the goal is easy
>> maintenance.
>>
> Understood ..
>
> @Bartosz: then you can ignore this one.
>
> Rgds, Heiner
>
>
>> Cheers,
>> Peter
>>
>>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
>>> ---
>>> drivers/misc/eeprom/at24.c | 5 -----
>>> 1 file changed, 5 deletions(-)
>>>
>>> diff --git a/drivers/misc/eeprom/at24.c b/drivers/misc/eeprom/at24.c
>>> index cd87f1b21..c75bb9b45 100644
>>> --- a/drivers/misc/eeprom/at24.c
>>> +++ b/drivers/misc/eeprom/at24.c
>>> @@ -10,15 +10,10 @@
>>> * (at your option) any later version.
>>> */
>>> #include <linux/kernel.h>
>>> -#include <linux/init.h>
>>> #include <linux/module.h>
>>> #include <linux/of_device.h>
>>> #include <linux/slab.h>
>>> #include <linux/delay.h>
>>> -#include <linux/mutex.h>
>>> -#include <linux/mod_devicetable.h>
>>> -#include <linux/log2.h>
>>> -#include <linux/bitops.h>
>>> #include <linux/jiffies.h>
>>> #include <linux/property.h>
>>> #include <linux/acpi.h>
>>>
>>
>>
>
Yep, whenever we use anything from some header, we want to have it
included directly. Otherwise any rearrangement would cause us pain.
Thanks,
Bartosz
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH 2/7] eeprom: at24: consider that SERIAL and MAC flags imply read-only
2017-11-30 6:40 [PATCH 0/7] eeprom: at24: series with smaller improvements Heiner Kallweit
2017-11-30 6:48 ` [PATCH 1/7] eeprom: at24: don't explicitely include header files which are implicitely included Heiner Kallweit
@ 2017-11-30 6:49 ` Heiner Kallweit
2017-12-01 10:10 ` Bartosz Golaszewski
2017-11-30 6:49 ` [PATCH 3/7] eeprom: at24: simplify probe a little by replacing &client->dev Heiner Kallweit
` (4 subsequent siblings)
6 siblings, 1 reply; 16+ messages in thread
From: Heiner Kallweit @ 2017-11-30 6:49 UTC (permalink / raw)
To: Bartosz Golaszewski; +Cc: linux-i2c@vger.kernel.org
Flags AT24_FLAG_SERIAL and AT24_FLAG_MAC imply read-only.
Therefore handle this in the code instead of specifying
AT24_FLAG_READONLY in the config data in these cases.
Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
drivers/misc/eeprom/at24.c | 31 +++++++++++++------------------
1 file changed, 13 insertions(+), 18 deletions(-)
diff --git a/drivers/misc/eeprom/at24.c b/drivers/misc/eeprom/at24.c
index c75bb9b45..90fefd1cf 100644
--- a/drivers/misc/eeprom/at24.c
+++ b/drivers/misc/eeprom/at24.c
@@ -131,38 +131,29 @@ static const struct i2c_device_id at24_ids[] = {
{ "24c00", AT24_DEVICE_MAGIC(128 / 8, AT24_FLAG_TAKE8ADDR) },
/* old variants can't be handled with this generic entry! */
{ "24c01", AT24_DEVICE_MAGIC(1024 / 8, 0) },
- { "24cs01", AT24_DEVICE_MAGIC(16,
- AT24_FLAG_SERIAL | AT24_FLAG_READONLY) },
+ { "24cs01", AT24_DEVICE_MAGIC(16, AT24_FLAG_SERIAL) },
{ "24c02", AT24_DEVICE_MAGIC(2048 / 8, 0) },
- { "24cs02", AT24_DEVICE_MAGIC(16,
- AT24_FLAG_SERIAL | AT24_FLAG_READONLY) },
- { "24mac402", AT24_DEVICE_MAGIC(48 / 8,
- AT24_FLAG_MAC | AT24_FLAG_READONLY) },
- { "24mac602", AT24_DEVICE_MAGIC(64 / 8,
- AT24_FLAG_MAC | AT24_FLAG_READONLY) },
+ { "24cs02", AT24_DEVICE_MAGIC(16, AT24_FLAG_SERIAL) },
+ { "24mac402", AT24_DEVICE_MAGIC(48 / 8, AT24_FLAG_MAC) },
+ { "24mac602", AT24_DEVICE_MAGIC(64 / 8, AT24_FLAG_MAC) },
/* spd is a 24c02 in memory DIMMs */
{ "spd", AT24_DEVICE_MAGIC(2048 / 8,
AT24_FLAG_READONLY | AT24_FLAG_IRUGO) },
{ "24c04", AT24_DEVICE_MAGIC(4096 / 8, 0) },
- { "24cs04", AT24_DEVICE_MAGIC(16,
- AT24_FLAG_SERIAL | AT24_FLAG_READONLY) },
+ { "24cs04", AT24_DEVICE_MAGIC(16, AT24_FLAG_SERIAL) },
/* 24rf08 quirk is handled at i2c-core */
{ "24c08", AT24_DEVICE_MAGIC(8192 / 8, 0) },
- { "24cs08", AT24_DEVICE_MAGIC(16,
- AT24_FLAG_SERIAL | AT24_FLAG_READONLY) },
+ { "24cs08", AT24_DEVICE_MAGIC(16, AT24_FLAG_SERIAL) },
{ "24c16", AT24_DEVICE_MAGIC(16384 / 8, 0) },
- { "24cs16", AT24_DEVICE_MAGIC(16,
- AT24_FLAG_SERIAL | AT24_FLAG_READONLY) },
+ { "24cs16", AT24_DEVICE_MAGIC(16, AT24_FLAG_SERIAL) },
{ "24c32", AT24_DEVICE_MAGIC(32768 / 8, AT24_FLAG_ADDR16) },
{ "24cs32", AT24_DEVICE_MAGIC(16,
AT24_FLAG_ADDR16 |
- AT24_FLAG_SERIAL |
- AT24_FLAG_READONLY) },
+ AT24_FLAG_SERIAL) },
{ "24c64", AT24_DEVICE_MAGIC(65536 / 8, AT24_FLAG_ADDR16) },
{ "24cs64", AT24_DEVICE_MAGIC(16,
AT24_FLAG_ADDR16 |
- AT24_FLAG_SERIAL |
- AT24_FLAG_READONLY) },
+ AT24_FLAG_SERIAL) },
{ "24c128", AT24_DEVICE_MAGIC(131072 / 8, AT24_FLAG_ADDR16) },
{ "24c256", AT24_DEVICE_MAGIC(262144 / 8, AT24_FLAG_ADDR16) },
{ "24c512", AT24_DEVICE_MAGIC(524288 / 8, AT24_FLAG_ADDR16) },
@@ -556,6 +547,10 @@ static int at24_probe(struct i2c_client *client, const struct i2c_device_id *id)
chip.context = NULL;
}
+ /* both flags imply read-only */
+ if (chip.flags & AT24_FLAG_SERIAL || chip.flags & AT24_FLAG_MAC)
+ chip.flags |= AT24_FLAG_READONLY;
+
if (!is_power_of_2(chip.byte_len))
dev_warn(&client->dev,
"byte_len looks suspicious (no power of 2)!\n");
--
2.15.0
^ permalink raw reply related [flat|nested] 16+ messages in thread* Re: [PATCH 2/7] eeprom: at24: consider that SERIAL and MAC flags imply read-only
2017-11-30 6:49 ` [PATCH 2/7] eeprom: at24: consider that SERIAL and MAC flags imply read-only Heiner Kallweit
@ 2017-12-01 10:10 ` Bartosz Golaszewski
2017-12-02 22:00 ` Heiner Kallweit
0 siblings, 1 reply; 16+ messages in thread
From: Bartosz Golaszewski @ 2017-12-01 10:10 UTC (permalink / raw)
To: Heiner Kallweit; +Cc: linux-i2c@vger.kernel.org
2017-11-30 7:49 GMT+01:00 Heiner Kallweit <hkallweit1@gmail.com>:
> Flags AT24_FLAG_SERIAL and AT24_FLAG_MAC imply read-only.
> Therefore handle this in the code instead of specifying
> AT24_FLAG_READONLY in the config data in these cases.
>
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> ---
> drivers/misc/eeprom/at24.c | 31 +++++++++++++------------------
> 1 file changed, 13 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/misc/eeprom/at24.c b/drivers/misc/eeprom/at24.c
> index c75bb9b45..90fefd1cf 100644
> --- a/drivers/misc/eeprom/at24.c
> +++ b/drivers/misc/eeprom/at24.c
> @@ -131,38 +131,29 @@ static const struct i2c_device_id at24_ids[] = {
> { "24c00", AT24_DEVICE_MAGIC(128 / 8, AT24_FLAG_TAKE8ADDR) },
> /* old variants can't be handled with this generic entry! */
> { "24c01", AT24_DEVICE_MAGIC(1024 / 8, 0) },
> - { "24cs01", AT24_DEVICE_MAGIC(16,
> - AT24_FLAG_SERIAL | AT24_FLAG_READONLY) },
> + { "24cs01", AT24_DEVICE_MAGIC(16, AT24_FLAG_SERIAL) },
> { "24c02", AT24_DEVICE_MAGIC(2048 / 8, 0) },
> - { "24cs02", AT24_DEVICE_MAGIC(16,
> - AT24_FLAG_SERIAL | AT24_FLAG_READONLY) },
> - { "24mac402", AT24_DEVICE_MAGIC(48 / 8,
> - AT24_FLAG_MAC | AT24_FLAG_READONLY) },
> - { "24mac602", AT24_DEVICE_MAGIC(64 / 8,
> - AT24_FLAG_MAC | AT24_FLAG_READONLY) },
> + { "24cs02", AT24_DEVICE_MAGIC(16, AT24_FLAG_SERIAL) },
> + { "24mac402", AT24_DEVICE_MAGIC(48 / 8, AT24_FLAG_MAC) },
> + { "24mac602", AT24_DEVICE_MAGIC(64 / 8, AT24_FLAG_MAC) },
> /* spd is a 24c02 in memory DIMMs */
> { "spd", AT24_DEVICE_MAGIC(2048 / 8,
> AT24_FLAG_READONLY | AT24_FLAG_IRUGO) },
> { "24c04", AT24_DEVICE_MAGIC(4096 / 8, 0) },
> - { "24cs04", AT24_DEVICE_MAGIC(16,
> - AT24_FLAG_SERIAL | AT24_FLAG_READONLY) },
> + { "24cs04", AT24_DEVICE_MAGIC(16, AT24_FLAG_SERIAL) },
> /* 24rf08 quirk is handled at i2c-core */
> { "24c08", AT24_DEVICE_MAGIC(8192 / 8, 0) },
> - { "24cs08", AT24_DEVICE_MAGIC(16,
> - AT24_FLAG_SERIAL | AT24_FLAG_READONLY) },
> + { "24cs08", AT24_DEVICE_MAGIC(16, AT24_FLAG_SERIAL) },
> { "24c16", AT24_DEVICE_MAGIC(16384 / 8, 0) },
> - { "24cs16", AT24_DEVICE_MAGIC(16,
> - AT24_FLAG_SERIAL | AT24_FLAG_READONLY) },
> + { "24cs16", AT24_DEVICE_MAGIC(16, AT24_FLAG_SERIAL) },
> { "24c32", AT24_DEVICE_MAGIC(32768 / 8, AT24_FLAG_ADDR16) },
> { "24cs32", AT24_DEVICE_MAGIC(16,
> AT24_FLAG_ADDR16 |
> - AT24_FLAG_SERIAL |
> - AT24_FLAG_READONLY) },
> + AT24_FLAG_SERIAL) },
> { "24c64", AT24_DEVICE_MAGIC(65536 / 8, AT24_FLAG_ADDR16) },
> { "24cs64", AT24_DEVICE_MAGIC(16,
> AT24_FLAG_ADDR16 |
> - AT24_FLAG_SERIAL |
> - AT24_FLAG_READONLY) },
> + AT24_FLAG_SERIAL) },
> { "24c128", AT24_DEVICE_MAGIC(131072 / 8, AT24_FLAG_ADDR16) },
> { "24c256", AT24_DEVICE_MAGIC(262144 / 8, AT24_FLAG_ADDR16) },
> { "24c512", AT24_DEVICE_MAGIC(524288 / 8, AT24_FLAG_ADDR16) },
> @@ -556,6 +547,10 @@ static int at24_probe(struct i2c_client *client, const struct i2c_device_id *id)
> chip.context = NULL;
> }
>
> + /* both flags imply read-only */
> + if (chip.flags & AT24_FLAG_SERIAL || chip.flags & AT24_FLAG_MAC)
> + chip.flags |= AT24_FLAG_READONLY;
> +
> if (!is_power_of_2(chip.byte_len))
> dev_warn(&client->dev,
> "byte_len looks suspicious (no power of 2)!\n");
> --
> 2.15.0
>
>
Just as with the header patch: this doesn't save us any code other
than visual. We want the definition to scream that this chip is
read-only. Nacked.
Thanks,
Bartosz
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH 2/7] eeprom: at24: consider that SERIAL and MAC flags imply read-only
2017-12-01 10:10 ` Bartosz Golaszewski
@ 2017-12-02 22:00 ` Heiner Kallweit
2017-12-03 21:20 ` Bartosz Golaszewski
0 siblings, 1 reply; 16+ messages in thread
From: Heiner Kallweit @ 2017-12-02 22:00 UTC (permalink / raw)
To: Bartosz Golaszewski; +Cc: linux-i2c@vger.kernel.org
Am 01.12.2017 um 11:10 schrieb Bartosz Golaszewski:
> 2017-11-30 7:49 GMT+01:00 Heiner Kallweit <hkallweit1@gmail.com>:
>> Flags AT24_FLAG_SERIAL and AT24_FLAG_MAC imply read-only.
>> Therefore handle this in the code instead of specifying
>> AT24_FLAG_READONLY in the config data in these cases.
>>
>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
>> ---
>> drivers/misc/eeprom/at24.c | 31 +++++++++++++------------------
>> 1 file changed, 13 insertions(+), 18 deletions(-)
>>
>> diff --git a/drivers/misc/eeprom/at24.c b/drivers/misc/eeprom/at24.c
>> index c75bb9b45..90fefd1cf 100644
>> --- a/drivers/misc/eeprom/at24.c
>> +++ b/drivers/misc/eeprom/at24.c
>> @@ -131,38 +131,29 @@ static const struct i2c_device_id at24_ids[] = {
>> { "24c00", AT24_DEVICE_MAGIC(128 / 8, AT24_FLAG_TAKE8ADDR) },
>> /* old variants can't be handled with this generic entry! */
>> { "24c01", AT24_DEVICE_MAGIC(1024 / 8, 0) },
>> - { "24cs01", AT24_DEVICE_MAGIC(16,
>> - AT24_FLAG_SERIAL | AT24_FLAG_READONLY) },
>> + { "24cs01", AT24_DEVICE_MAGIC(16, AT24_FLAG_SERIAL) },
>> { "24c02", AT24_DEVICE_MAGIC(2048 / 8, 0) },
>> - { "24cs02", AT24_DEVICE_MAGIC(16,
>> - AT24_FLAG_SERIAL | AT24_FLAG_READONLY) },
>> - { "24mac402", AT24_DEVICE_MAGIC(48 / 8,
>> - AT24_FLAG_MAC | AT24_FLAG_READONLY) },
>> - { "24mac602", AT24_DEVICE_MAGIC(64 / 8,
>> - AT24_FLAG_MAC | AT24_FLAG_READONLY) },
>> + { "24cs02", AT24_DEVICE_MAGIC(16, AT24_FLAG_SERIAL) },
>> + { "24mac402", AT24_DEVICE_MAGIC(48 / 8, AT24_FLAG_MAC) },
>> + { "24mac602", AT24_DEVICE_MAGIC(64 / 8, AT24_FLAG_MAC) },
>> /* spd is a 24c02 in memory DIMMs */
>> { "spd", AT24_DEVICE_MAGIC(2048 / 8,
>> AT24_FLAG_READONLY | AT24_FLAG_IRUGO) },
>> { "24c04", AT24_DEVICE_MAGIC(4096 / 8, 0) },
>> - { "24cs04", AT24_DEVICE_MAGIC(16,
>> - AT24_FLAG_SERIAL | AT24_FLAG_READONLY) },
>> + { "24cs04", AT24_DEVICE_MAGIC(16, AT24_FLAG_SERIAL) },
>> /* 24rf08 quirk is handled at i2c-core */
>> { "24c08", AT24_DEVICE_MAGIC(8192 / 8, 0) },
>> - { "24cs08", AT24_DEVICE_MAGIC(16,
>> - AT24_FLAG_SERIAL | AT24_FLAG_READONLY) },
>> + { "24cs08", AT24_DEVICE_MAGIC(16, AT24_FLAG_SERIAL) },
>> { "24c16", AT24_DEVICE_MAGIC(16384 / 8, 0) },
>> - { "24cs16", AT24_DEVICE_MAGIC(16,
>> - AT24_FLAG_SERIAL | AT24_FLAG_READONLY) },
>> + { "24cs16", AT24_DEVICE_MAGIC(16, AT24_FLAG_SERIAL) },
>> { "24c32", AT24_DEVICE_MAGIC(32768 / 8, AT24_FLAG_ADDR16) },
>> { "24cs32", AT24_DEVICE_MAGIC(16,
>> AT24_FLAG_ADDR16 |
>> - AT24_FLAG_SERIAL |
>> - AT24_FLAG_READONLY) },
>> + AT24_FLAG_SERIAL) },
>> { "24c64", AT24_DEVICE_MAGIC(65536 / 8, AT24_FLAG_ADDR16) },
>> { "24cs64", AT24_DEVICE_MAGIC(16,
>> AT24_FLAG_ADDR16 |
>> - AT24_FLAG_SERIAL |
>> - AT24_FLAG_READONLY) },
>> + AT24_FLAG_SERIAL) },
>> { "24c128", AT24_DEVICE_MAGIC(131072 / 8, AT24_FLAG_ADDR16) },
>> { "24c256", AT24_DEVICE_MAGIC(262144 / 8, AT24_FLAG_ADDR16) },
>> { "24c512", AT24_DEVICE_MAGIC(524288 / 8, AT24_FLAG_ADDR16) },
>> @@ -556,6 +547,10 @@ static int at24_probe(struct i2c_client *client, const struct i2c_device_id *id)
>> chip.context = NULL;
>> }
>>
>> + /* both flags imply read-only */
>> + if (chip.flags & AT24_FLAG_SERIAL || chip.flags & AT24_FLAG_MAC)
>> + chip.flags |= AT24_FLAG_READONLY;
>> +
>> if (!is_power_of_2(chip.byte_len))
>> dev_warn(&client->dev,
>> "byte_len looks suspicious (no power of 2)!\n");
>> --
>> 2.15.0
>>
>>
>
> Just as with the header patch: this doesn't save us any code other
> than visual. We want the definition to scream that this chip is
> read-only. Nacked.
>
I see your point. My concern is that so far we may have platform-data-
configured devices setting flag SERIAL or MAC and not READONLY and we
have no control over such (IMO) misconfigurations.
So my primary question would be whether we agree on SERIAL and MAC
implying READONLY. If yes then I think we should reflect this
relationship in the code.
Instead of silently setting flag READONLY we could also reject such
invalid flag configurations. Or print at least a message/warning
when setting READONLY.
Rgds, Heiner
> Thanks,
> Bartosz
>
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH 2/7] eeprom: at24: consider that SERIAL and MAC flags imply read-only
2017-12-02 22:00 ` Heiner Kallweit
@ 2017-12-03 21:20 ` Bartosz Golaszewski
0 siblings, 0 replies; 16+ messages in thread
From: Bartosz Golaszewski @ 2017-12-03 21:20 UTC (permalink / raw)
To: Heiner Kallweit; +Cc: linux-i2c@vger.kernel.org
2017-12-02 23:00 GMT+01:00 Heiner Kallweit <hkallweit1@gmail.com>:
> Am 01.12.2017 um 11:10 schrieb Bartosz Golaszewski:
>> 2017-11-30 7:49 GMT+01:00 Heiner Kallweit <hkallweit1@gmail.com>:
>>> Flags AT24_FLAG_SERIAL and AT24_FLAG_MAC imply read-only.
>>> Therefore handle this in the code instead of specifying
>>> AT24_FLAG_READONLY in the config data in these cases.
>>>
>>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
>>> ---
>>> drivers/misc/eeprom/at24.c | 31 +++++++++++++------------------
>>> 1 file changed, 13 insertions(+), 18 deletions(-)
>>>
>>> diff --git a/drivers/misc/eeprom/at24.c b/drivers/misc/eeprom/at24.c
>>> index c75bb9b45..90fefd1cf 100644
>>> --- a/drivers/misc/eeprom/at24.c
>>> +++ b/drivers/misc/eeprom/at24.c
>>> @@ -131,38 +131,29 @@ static const struct i2c_device_id at24_ids[] = {
>>> { "24c00", AT24_DEVICE_MAGIC(128 / 8, AT24_FLAG_TAKE8ADDR) },
>>> /* old variants can't be handled with this generic entry! */
>>> { "24c01", AT24_DEVICE_MAGIC(1024 / 8, 0) },
>>> - { "24cs01", AT24_DEVICE_MAGIC(16,
>>> - AT24_FLAG_SERIAL | AT24_FLAG_READONLY) },
>>> + { "24cs01", AT24_DEVICE_MAGIC(16, AT24_FLAG_SERIAL) },
>>> { "24c02", AT24_DEVICE_MAGIC(2048 / 8, 0) },
>>> - { "24cs02", AT24_DEVICE_MAGIC(16,
>>> - AT24_FLAG_SERIAL | AT24_FLAG_READONLY) },
>>> - { "24mac402", AT24_DEVICE_MAGIC(48 / 8,
>>> - AT24_FLAG_MAC | AT24_FLAG_READONLY) },
>>> - { "24mac602", AT24_DEVICE_MAGIC(64 / 8,
>>> - AT24_FLAG_MAC | AT24_FLAG_READONLY) },
>>> + { "24cs02", AT24_DEVICE_MAGIC(16, AT24_FLAG_SERIAL) },
>>> + { "24mac402", AT24_DEVICE_MAGIC(48 / 8, AT24_FLAG_MAC) },
>>> + { "24mac602", AT24_DEVICE_MAGIC(64 / 8, AT24_FLAG_MAC) },
>>> /* spd is a 24c02 in memory DIMMs */
>>> { "spd", AT24_DEVICE_MAGIC(2048 / 8,
>>> AT24_FLAG_READONLY | AT24_FLAG_IRUGO) },
>>> { "24c04", AT24_DEVICE_MAGIC(4096 / 8, 0) },
>>> - { "24cs04", AT24_DEVICE_MAGIC(16,
>>> - AT24_FLAG_SERIAL | AT24_FLAG_READONLY) },
>>> + { "24cs04", AT24_DEVICE_MAGIC(16, AT24_FLAG_SERIAL) },
>>> /* 24rf08 quirk is handled at i2c-core */
>>> { "24c08", AT24_DEVICE_MAGIC(8192 / 8, 0) },
>>> - { "24cs08", AT24_DEVICE_MAGIC(16,
>>> - AT24_FLAG_SERIAL | AT24_FLAG_READONLY) },
>>> + { "24cs08", AT24_DEVICE_MAGIC(16, AT24_FLAG_SERIAL) },
>>> { "24c16", AT24_DEVICE_MAGIC(16384 / 8, 0) },
>>> - { "24cs16", AT24_DEVICE_MAGIC(16,
>>> - AT24_FLAG_SERIAL | AT24_FLAG_READONLY) },
>>> + { "24cs16", AT24_DEVICE_MAGIC(16, AT24_FLAG_SERIAL) },
>>> { "24c32", AT24_DEVICE_MAGIC(32768 / 8, AT24_FLAG_ADDR16) },
>>> { "24cs32", AT24_DEVICE_MAGIC(16,
>>> AT24_FLAG_ADDR16 |
>>> - AT24_FLAG_SERIAL |
>>> - AT24_FLAG_READONLY) },
>>> + AT24_FLAG_SERIAL) },
>>> { "24c64", AT24_DEVICE_MAGIC(65536 / 8, AT24_FLAG_ADDR16) },
>>> { "24cs64", AT24_DEVICE_MAGIC(16,
>>> AT24_FLAG_ADDR16 |
>>> - AT24_FLAG_SERIAL |
>>> - AT24_FLAG_READONLY) },
>>> + AT24_FLAG_SERIAL) },
>>> { "24c128", AT24_DEVICE_MAGIC(131072 / 8, AT24_FLAG_ADDR16) },
>>> { "24c256", AT24_DEVICE_MAGIC(262144 / 8, AT24_FLAG_ADDR16) },
>>> { "24c512", AT24_DEVICE_MAGIC(524288 / 8, AT24_FLAG_ADDR16) },
>>> @@ -556,6 +547,10 @@ static int at24_probe(struct i2c_client *client, const struct i2c_device_id *id)
>>> chip.context = NULL;
>>> }
>>>
>>> + /* both flags imply read-only */
>>> + if (chip.flags & AT24_FLAG_SERIAL || chip.flags & AT24_FLAG_MAC)
>>> + chip.flags |= AT24_FLAG_READONLY;
>>> +
>>> if (!is_power_of_2(chip.byte_len))
>>> dev_warn(&client->dev,
>>> "byte_len looks suspicious (no power of 2)!\n");
>>> --
>>> 2.15.0
>>>
>>>
>>
>> Just as with the header patch: this doesn't save us any code other
>> than visual. We want the definition to scream that this chip is
>> read-only. Nacked.
>>
> I see your point. My concern is that so far we may have platform-data-
> configured devices setting flag SERIAL or MAC and not READONLY and we
> have no control over such (IMO) misconfigurations.
>
> So my primary question would be whether we agree on SERIAL and MAC
> implying READONLY. If yes then I think we should reflect this
> relationship in the code.
> Instead of silently setting flag READONLY we could also reject such
> invalid flag configurations. Or print at least a message/warning
> when setting READONLY.
>
> Rgds, Heiner
>
>> Thanks,
>> Bartosz
>>
>
OK, this makes more sense, let's check if READONLY is set for MAC and
CS and if not: let's set it ourselves while also printing a warning.
Thanks,
Bartosz
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH 3/7] eeprom: at24: simplify probe a little by replacing &client->dev
2017-11-30 6:40 [PATCH 0/7] eeprom: at24: series with smaller improvements Heiner Kallweit
2017-11-30 6:48 ` [PATCH 1/7] eeprom: at24: don't explicitely include header files which are implicitely included Heiner Kallweit
2017-11-30 6:49 ` [PATCH 2/7] eeprom: at24: consider that SERIAL and MAC flags imply read-only Heiner Kallweit
@ 2017-11-30 6:49 ` Heiner Kallweit
2017-12-01 10:14 ` Bartosz Golaszewski
2017-11-30 6:49 ` [PATCH 4/7] eeprom: at24: simplify functions at24_read/write a little Heiner Kallweit
` (3 subsequent siblings)
6 siblings, 1 reply; 16+ messages in thread
From: Heiner Kallweit @ 2017-11-30 6:49 UTC (permalink / raw)
To: Bartosz Golaszewski; +Cc: linux-i2c@vger.kernel.org
We have lots of places in probe where &client->dev is used, replace it
with a variable to simplify code a little.
In addition remove redundant check for client->dev.of_node when using
of_match_device, this function can deal with a NULL of_node.
Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
drivers/misc/eeprom/at24.c | 48 +++++++++++++++++++++-------------------------
1 file changed, 22 insertions(+), 26 deletions(-)
diff --git a/drivers/misc/eeprom/at24.c b/drivers/misc/eeprom/at24.c
index 90fefd1cf..d56be71f1 100644
--- a/drivers/misc/eeprom/at24.c
+++ b/drivers/misc/eeprom/at24.c
@@ -511,26 +511,25 @@ static int at24_probe(struct i2c_client *client, const struct i2c_device_id *id)
int err;
unsigned i, num_addresses;
const struct regmap_config *config;
+ struct device *dev = &client->dev;
u8 test_byte;
- if (client->dev.platform_data) {
- chip = *(struct at24_platform_data *)client->dev.platform_data;
+ if (dev->platform_data) {
+ chip = *(struct at24_platform_data *)dev->platform_data;
} else {
/*
* The I2C core allows OF nodes compatibles to match against the
* I2C device ID table as a fallback, so check not only if an OF
* node is present but also if it matches an OF device ID entry.
*/
- if (client->dev.of_node &&
- of_match_device(at24_of_match, &client->dev)) {
- magic = (kernel_ulong_t)
- of_device_get_match_data(&client->dev);
+ if (of_match_device(at24_of_match, dev)) {
+ magic = (kernel_ulong_t) of_device_get_match_data(dev);
} else if (id) {
magic = id->driver_data;
} else {
const struct acpi_device_id *aid;
- aid = acpi_match_device(at24_acpi_ids, &client->dev);
+ aid = acpi_match_device(at24_acpi_ids, dev);
if (aid)
magic = aid->driver_data;
}
@@ -541,7 +540,7 @@ static int at24_probe(struct i2c_client *client, const struct i2c_device_id *id)
magic >>= AT24_SIZE_BYTELEN;
chip.flags = magic & AT24_BITMASK(AT24_SIZE_FLAGS);
- at24_get_pdata(&client->dev, &chip);
+ at24_get_pdata(dev, &chip);
chip.setup = NULL;
chip.context = NULL;
@@ -552,15 +551,13 @@ static int at24_probe(struct i2c_client *client, const struct i2c_device_id *id)
chip.flags |= AT24_FLAG_READONLY;
if (!is_power_of_2(chip.byte_len))
- dev_warn(&client->dev,
- "byte_len looks suspicious (no power of 2)!\n");
+ dev_warn(dev, "byte_len looks suspicious (no power of 2)!\n");
if (!chip.page_size) {
- dev_err(&client->dev, "page_size must not be 0!\n");
+ dev_err(dev, "page_size must not be 0!\n");
return -EINVAL;
}
if (!is_power_of_2(chip.page_size))
- dev_warn(&client->dev,
- "page_size looks suspicious (no power of 2)!\n");
+ dev_warn(dev, "page_size looks suspicious (no power of 2)!\n");
/*
* REVISIT: the size of the EUI-48 byte array is 6 in at24mac402, while
@@ -588,7 +585,7 @@ static int at24_probe(struct i2c_client *client, const struct i2c_device_id *id)
else
config = ®map_config_8;
- at24 = devm_kzalloc(&client->dev, sizeof(struct at24_data) +
+ at24 = devm_kzalloc(dev, sizeof(struct at24_data) +
num_addresses * sizeof(struct at24_client), GFP_KERNEL);
if (!at24)
return -ENOMEM;
@@ -604,8 +601,7 @@ static int at24_probe(struct i2c_client *client, const struct i2c_device_id *id)
return PTR_ERR(at24->client[0].regmap);
if ((chip.flags & AT24_FLAG_SERIAL) && (chip.flags & AT24_FLAG_MAC)) {
- dev_err(&client->dev,
- "invalid device data - cannot have both AT24_FLAG_SERIAL & AT24_FLAG_MAC.");
+ dev_err(dev, "invalid device data - cannot have both AT24_FLAG_SERIAL & AT24_FLAG_MAC.");
return -EINVAL;
}
@@ -622,8 +618,8 @@ static int at24_probe(struct i2c_client *client, const struct i2c_device_id *id)
at24->client[i].client = i2c_new_dummy(client->adapter,
client->addr + i);
if (!at24->client[i].client) {
- dev_err(&client->dev, "address 0x%02x unavailable\n",
- client->addr + i);
+ dev_err(dev, "address 0x%02x unavailable\n",
+ client->addr + i);
err = -EADDRINUSE;
goto err_clients;
}
@@ -638,27 +634,27 @@ static int at24_probe(struct i2c_client *client, const struct i2c_device_id *id)
i2c_set_clientdata(client, at24);
/* enable runtime pm */
- pm_runtime_set_active(&client->dev);
- pm_runtime_enable(&client->dev);
+ pm_runtime_set_active(dev);
+ pm_runtime_enable(dev);
/*
* Perform a one-byte test read to verify that the
* chip is functional.
*/
err = at24_read(at24, 0, &test_byte, 1);
- pm_runtime_idle(&client->dev);
+ pm_runtime_idle(dev);
if (err) {
err = -ENODEV;
goto err_clients;
}
- at24->nvmem_config.name = dev_name(&client->dev);
- at24->nvmem_config.dev = &client->dev;
+ at24->nvmem_config.name = dev_name(dev);
+ at24->nvmem_config.dev = dev;
at24->nvmem_config.read_only = !writable;
at24->nvmem_config.root_only = true;
at24->nvmem_config.owner = THIS_MODULE;
at24->nvmem_config.compat = true;
- at24->nvmem_config.base_dev = &client->dev;
+ at24->nvmem_config.base_dev = dev;
at24->nvmem_config.reg_read = at24_read;
at24->nvmem_config.reg_write = at24_write;
at24->nvmem_config.priv = at24;
@@ -673,7 +669,7 @@ static int at24_probe(struct i2c_client *client, const struct i2c_device_id *id)
goto err_clients;
}
- dev_info(&client->dev, "%u byte %s EEPROM, %s, %u bytes/write\n",
+ dev_info(dev, "%u byte %s EEPROM, %s, %u bytes/write\n",
chip.byte_len, client->name,
writable ? "writable" : "read-only", at24->write_max);
@@ -688,7 +684,7 @@ static int at24_probe(struct i2c_client *client, const struct i2c_device_id *id)
if (at24->client[i].client)
i2c_unregister_device(at24->client[i].client);
- pm_runtime_disable(&client->dev);
+ pm_runtime_disable(dev);
return err;
}
--
2.15.0
^ permalink raw reply related [flat|nested] 16+ messages in thread* Re: [PATCH 3/7] eeprom: at24: simplify probe a little by replacing &client->dev
2017-11-30 6:49 ` [PATCH 3/7] eeprom: at24: simplify probe a little by replacing &client->dev Heiner Kallweit
@ 2017-12-01 10:14 ` Bartosz Golaszewski
0 siblings, 0 replies; 16+ messages in thread
From: Bartosz Golaszewski @ 2017-12-01 10:14 UTC (permalink / raw)
To: Heiner Kallweit; +Cc: linux-i2c@vger.kernel.org
2017-11-30 7:49 GMT+01:00 Heiner Kallweit <hkallweit1@gmail.com>:
> We have lots of places in probe where &client->dev is used, replace it
> with a variable to simplify code a little.
>
> In addition remove redundant check for client->dev.of_node when using
> of_match_device, this function can deal with a NULL of_node.
This belongs in a separate patch. Please split it for v2.
>
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> ---
> drivers/misc/eeprom/at24.c | 48 +++++++++++++++++++++-------------------------
> 1 file changed, 22 insertions(+), 26 deletions(-)
>
> diff --git a/drivers/misc/eeprom/at24.c b/drivers/misc/eeprom/at24.c
> index 90fefd1cf..d56be71f1 100644
> --- a/drivers/misc/eeprom/at24.c
> +++ b/drivers/misc/eeprom/at24.c
> @@ -511,26 +511,25 @@ static int at24_probe(struct i2c_client *client, const struct i2c_device_id *id)
> int err;
> unsigned i, num_addresses;
> const struct regmap_config *config;
> + struct device *dev = &client->dev;
> u8 test_byte;
>
> - if (client->dev.platform_data) {
> - chip = *(struct at24_platform_data *)client->dev.platform_data;
> + if (dev->platform_data) {
> + chip = *(struct at24_platform_data *)dev->platform_data;
> } else {
> /*
> * The I2C core allows OF nodes compatibles to match against the
> * I2C device ID table as a fallback, so check not only if an OF
> * node is present but also if it matches an OF device ID entry.
> */
> - if (client->dev.of_node &&
> - of_match_device(at24_of_match, &client->dev)) {
> - magic = (kernel_ulong_t)
> - of_device_get_match_data(&client->dev);
> + if (of_match_device(at24_of_match, dev)) {
> + magic = (kernel_ulong_t) of_device_get_match_data(dev);
> } else if (id) {
> magic = id->driver_data;
> } else {
> const struct acpi_device_id *aid;
>
> - aid = acpi_match_device(at24_acpi_ids, &client->dev);
> + aid = acpi_match_device(at24_acpi_ids, dev);
> if (aid)
> magic = aid->driver_data;
> }
> @@ -541,7 +540,7 @@ static int at24_probe(struct i2c_client *client, const struct i2c_device_id *id)
> magic >>= AT24_SIZE_BYTELEN;
> chip.flags = magic & AT24_BITMASK(AT24_SIZE_FLAGS);
>
> - at24_get_pdata(&client->dev, &chip);
> + at24_get_pdata(dev, &chip);
>
> chip.setup = NULL;
> chip.context = NULL;
> @@ -552,15 +551,13 @@ static int at24_probe(struct i2c_client *client, const struct i2c_device_id *id)
> chip.flags |= AT24_FLAG_READONLY;
>
> if (!is_power_of_2(chip.byte_len))
> - dev_warn(&client->dev,
> - "byte_len looks suspicious (no power of 2)!\n");
> + dev_warn(dev, "byte_len looks suspicious (no power of 2)!\n");
> if (!chip.page_size) {
> - dev_err(&client->dev, "page_size must not be 0!\n");
> + dev_err(dev, "page_size must not be 0!\n");
> return -EINVAL;
> }
> if (!is_power_of_2(chip.page_size))
> - dev_warn(&client->dev,
> - "page_size looks suspicious (no power of 2)!\n");
> + dev_warn(dev, "page_size looks suspicious (no power of 2)!\n");
>
> /*
> * REVISIT: the size of the EUI-48 byte array is 6 in at24mac402, while
> @@ -588,7 +585,7 @@ static int at24_probe(struct i2c_client *client, const struct i2c_device_id *id)
> else
> config = ®map_config_8;
>
> - at24 = devm_kzalloc(&client->dev, sizeof(struct at24_data) +
> + at24 = devm_kzalloc(dev, sizeof(struct at24_data) +
> num_addresses * sizeof(struct at24_client), GFP_KERNEL);
> if (!at24)
> return -ENOMEM;
> @@ -604,8 +601,7 @@ static int at24_probe(struct i2c_client *client, const struct i2c_device_id *id)
> return PTR_ERR(at24->client[0].regmap);
>
> if ((chip.flags & AT24_FLAG_SERIAL) && (chip.flags & AT24_FLAG_MAC)) {
> - dev_err(&client->dev,
> - "invalid device data - cannot have both AT24_FLAG_SERIAL & AT24_FLAG_MAC.");
> + dev_err(dev, "invalid device data - cannot have both AT24_FLAG_SERIAL & AT24_FLAG_MAC.");
> return -EINVAL;
> }
>
> @@ -622,8 +618,8 @@ static int at24_probe(struct i2c_client *client, const struct i2c_device_id *id)
> at24->client[i].client = i2c_new_dummy(client->adapter,
> client->addr + i);
> if (!at24->client[i].client) {
> - dev_err(&client->dev, "address 0x%02x unavailable\n",
> - client->addr + i);
> + dev_err(dev, "address 0x%02x unavailable\n",
> + client->addr + i);
> err = -EADDRINUSE;
> goto err_clients;
> }
> @@ -638,27 +634,27 @@ static int at24_probe(struct i2c_client *client, const struct i2c_device_id *id)
> i2c_set_clientdata(client, at24);
>
> /* enable runtime pm */
> - pm_runtime_set_active(&client->dev);
> - pm_runtime_enable(&client->dev);
> + pm_runtime_set_active(dev);
> + pm_runtime_enable(dev);
>
> /*
> * Perform a one-byte test read to verify that the
> * chip is functional.
> */
> err = at24_read(at24, 0, &test_byte, 1);
> - pm_runtime_idle(&client->dev);
> + pm_runtime_idle(dev);
> if (err) {
> err = -ENODEV;
> goto err_clients;
> }
>
> - at24->nvmem_config.name = dev_name(&client->dev);
> - at24->nvmem_config.dev = &client->dev;
> + at24->nvmem_config.name = dev_name(dev);
> + at24->nvmem_config.dev = dev;
> at24->nvmem_config.read_only = !writable;
> at24->nvmem_config.root_only = true;
> at24->nvmem_config.owner = THIS_MODULE;
> at24->nvmem_config.compat = true;
> - at24->nvmem_config.base_dev = &client->dev;
> + at24->nvmem_config.base_dev = dev;
> at24->nvmem_config.reg_read = at24_read;
> at24->nvmem_config.reg_write = at24_write;
> at24->nvmem_config.priv = at24;
> @@ -673,7 +669,7 @@ static int at24_probe(struct i2c_client *client, const struct i2c_device_id *id)
> goto err_clients;
> }
>
> - dev_info(&client->dev, "%u byte %s EEPROM, %s, %u bytes/write\n",
> + dev_info(dev, "%u byte %s EEPROM, %s, %u bytes/write\n",
> chip.byte_len, client->name,
> writable ? "writable" : "read-only", at24->write_max);
>
> @@ -688,7 +684,7 @@ static int at24_probe(struct i2c_client *client, const struct i2c_device_id *id)
> if (at24->client[i].client)
> i2c_unregister_device(at24->client[i].client);
>
> - pm_runtime_disable(&client->dev);
> + pm_runtime_disable(dev);
>
> return err;
> }
> --
> 2.15.0
>
>
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH 4/7] eeprom: at24: simplify functions at24_read/write a little
2017-11-30 6:40 [PATCH 0/7] eeprom: at24: series with smaller improvements Heiner Kallweit
` (2 preceding siblings ...)
2017-11-30 6:49 ` [PATCH 3/7] eeprom: at24: simplify probe a little by replacing &client->dev Heiner Kallweit
@ 2017-11-30 6:49 ` Heiner Kallweit
2017-12-01 10:14 ` Bartosz Golaszewski
2017-11-30 6:49 ` [PATCH 5/7] eeprom: at24: zero-initialize variable chip in probe Heiner Kallweit
` (2 subsequent siblings)
6 siblings, 1 reply; 16+ messages in thread
From: Heiner Kallweit @ 2017-11-30 6:49 UTC (permalink / raw)
To: Bartosz Golaszewski; +Cc: linux-i2c@vger.kernel.org
Simplify functions at24_read/write a little.
Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
drivers/misc/eeprom/at24.c | 40 ++++++++++++++++------------------------
1 file changed, 16 insertions(+), 24 deletions(-)
diff --git a/drivers/misc/eeprom/at24.c b/drivers/misc/eeprom/at24.c
index d56be71f1..837f1d88c 100644
--- a/drivers/misc/eeprom/at24.c
+++ b/drivers/misc/eeprom/at24.c
@@ -374,24 +374,20 @@ static int at24_read(void *priv, unsigned int off, void *val, size_t count)
mutex_lock(&at24->lock);
while (count) {
- int status;
+ ret = at24_regmap_read(at24, buf, off, count);
+ if (ret < 0)
+ goto out;
- status = at24_regmap_read(at24, buf, off, count);
- if (status < 0) {
- mutex_unlock(&at24->lock);
- pm_runtime_put(&client->dev);
- return status;
- }
- buf += status;
- off += status;
- count -= status;
+ buf += ret;
+ off += ret;
+ count -= ret;
}
-
+out:
mutex_unlock(&at24->lock);
pm_runtime_put(&client->dev);
- return 0;
+ return ret < 0 ? ret : 0;
}
static int at24_write(void *priv, unsigned int off, void *val, size_t count)
@@ -424,24 +420,20 @@ static int at24_write(void *priv, unsigned int off, void *val, size_t count)
mutex_lock(&at24->lock);
while (count) {
- int status;
+ ret = at24_regmap_write(at24, buf, off, count);
+ if (ret < 0)
+ goto out;
- status = at24_regmap_write(at24, buf, off, count);
- if (status < 0) {
- mutex_unlock(&at24->lock);
- pm_runtime_put(&client->dev);
- return status;
- }
- buf += status;
- off += status;
- count -= status;
+ buf += ret;
+ off += ret;
+ count -= ret;
}
-
+out:
mutex_unlock(&at24->lock);
pm_runtime_put(&client->dev);
- return 0;
+ return ret < 0 ? ret : 0;
}
static void at24_get_pdata(struct device *dev, struct at24_platform_data *chip)
--
2.15.0
^ permalink raw reply related [flat|nested] 16+ messages in thread* Re: [PATCH 4/7] eeprom: at24: simplify functions at24_read/write a little
2017-11-30 6:49 ` [PATCH 4/7] eeprom: at24: simplify functions at24_read/write a little Heiner Kallweit
@ 2017-12-01 10:14 ` Bartosz Golaszewski
0 siblings, 0 replies; 16+ messages in thread
From: Bartosz Golaszewski @ 2017-12-01 10:14 UTC (permalink / raw)
To: Heiner Kallweit; +Cc: linux-i2c@vger.kernel.org
2017-11-30 7:49 GMT+01:00 Heiner Kallweit <hkallweit1@gmail.com>:
> Simplify functions at24_read/write a little.
>
Please be more specific in your commit message - it's not obvious at
first glance what you're actually simplifying.
Thanks,
Bartosz
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> ---
> drivers/misc/eeprom/at24.c | 40 ++++++++++++++++------------------------
> 1 file changed, 16 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/misc/eeprom/at24.c b/drivers/misc/eeprom/at24.c
> index d56be71f1..837f1d88c 100644
> --- a/drivers/misc/eeprom/at24.c
> +++ b/drivers/misc/eeprom/at24.c
> @@ -374,24 +374,20 @@ static int at24_read(void *priv, unsigned int off, void *val, size_t count)
> mutex_lock(&at24->lock);
>
> while (count) {
> - int status;
> + ret = at24_regmap_read(at24, buf, off, count);
> + if (ret < 0)
> + goto out;
>
> - status = at24_regmap_read(at24, buf, off, count);
> - if (status < 0) {
> - mutex_unlock(&at24->lock);
> - pm_runtime_put(&client->dev);
> - return status;
> - }
> - buf += status;
> - off += status;
> - count -= status;
> + buf += ret;
> + off += ret;
> + count -= ret;
> }
> -
> +out:
> mutex_unlock(&at24->lock);
>
> pm_runtime_put(&client->dev);
>
> - return 0;
> + return ret < 0 ? ret : 0;
> }
>
> static int at24_write(void *priv, unsigned int off, void *val, size_t count)
> @@ -424,24 +420,20 @@ static int at24_write(void *priv, unsigned int off, void *val, size_t count)
> mutex_lock(&at24->lock);
>
> while (count) {
> - int status;
> + ret = at24_regmap_write(at24, buf, off, count);
> + if (ret < 0)
> + goto out;
>
> - status = at24_regmap_write(at24, buf, off, count);
> - if (status < 0) {
> - mutex_unlock(&at24->lock);
> - pm_runtime_put(&client->dev);
> - return status;
> - }
> - buf += status;
> - off += status;
> - count -= status;
> + buf += ret;
> + off += ret;
> + count -= ret;
> }
> -
> +out:
> mutex_unlock(&at24->lock);
>
> pm_runtime_put(&client->dev);
>
> - return 0;
> + return ret < 0 ? ret : 0;
> }
>
> static void at24_get_pdata(struct device *dev, struct at24_platform_data *chip)
> --
> 2.15.0
>
>
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH 5/7] eeprom: at24: zero-initialize variable chip in probe
2017-11-30 6:40 [PATCH 0/7] eeprom: at24: series with smaller improvements Heiner Kallweit
` (3 preceding siblings ...)
2017-11-30 6:49 ` [PATCH 4/7] eeprom: at24: simplify functions at24_read/write a little Heiner Kallweit
@ 2017-11-30 6:49 ` Heiner Kallweit
2017-11-30 6:49 ` [PATCH 6/7] eeprom: at24: don't check chip.byte_len for power of two Heiner Kallweit
2017-11-30 6:49 ` [PATCH 7/7] eeprom: at24: don't check page_size for read-only chips and reorder checks Heiner Kallweit
6 siblings, 0 replies; 16+ messages in thread
From: Heiner Kallweit @ 2017-11-30 6:49 UTC (permalink / raw)
To: Bartosz Golaszewski; +Cc: linux-i2c@vger.kernel.org
Zero-initialize variable chip to be on the safe side. This also
allows ro remove some explicit NULL initializations.
Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
drivers/misc/eeprom/at24.c | 5 +----
1 file changed, 1 insertion(+), 4 deletions(-)
diff --git a/drivers/misc/eeprom/at24.c b/drivers/misc/eeprom/at24.c
index 837f1d88c..8fef6d5a8 100644
--- a/drivers/misc/eeprom/at24.c
+++ b/drivers/misc/eeprom/at24.c
@@ -496,7 +496,7 @@ static const struct regmap_config regmap_config_16 = {
static int at24_probe(struct i2c_client *client, const struct i2c_device_id *id)
{
- struct at24_platform_data chip;
+ struct at24_platform_data chip = {};
kernel_ulong_t magic = 0;
bool writable;
struct at24_data *at24;
@@ -533,9 +533,6 @@ static int at24_probe(struct i2c_client *client, const struct i2c_device_id *id)
chip.flags = magic & AT24_BITMASK(AT24_SIZE_FLAGS);
at24_get_pdata(dev, &chip);
-
- chip.setup = NULL;
- chip.context = NULL;
}
/* both flags imply read-only */
--
2.15.0
^ permalink raw reply related [flat|nested] 16+ messages in thread* [PATCH 6/7] eeprom: at24: don't check chip.byte_len for power of two
2017-11-30 6:40 [PATCH 0/7] eeprom: at24: series with smaller improvements Heiner Kallweit
` (4 preceding siblings ...)
2017-11-30 6:49 ` [PATCH 5/7] eeprom: at24: zero-initialize variable chip in probe Heiner Kallweit
@ 2017-11-30 6:49 ` Heiner Kallweit
2017-11-30 6:49 ` [PATCH 7/7] eeprom: at24: don't check page_size for read-only chips and reorder checks Heiner Kallweit
6 siblings, 0 replies; 16+ messages in thread
From: Heiner Kallweit @ 2017-11-30 6:49 UTC (permalink / raw)
To: Bartosz Golaszewski; +Cc: linux-i2c@vger.kernel.org
When using AT24_FLAG_SERIAL and AT24_FLAG_MAC we expose just parts of
the chip. These parts can have arbitrary size (e.g. a 6 byte MAC),
so remove the check for power of two.
Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
drivers/misc/eeprom/at24.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/drivers/misc/eeprom/at24.c b/drivers/misc/eeprom/at24.c
index 8fef6d5a8..74d2347a1 100644
--- a/drivers/misc/eeprom/at24.c
+++ b/drivers/misc/eeprom/at24.c
@@ -539,8 +539,6 @@ static int at24_probe(struct i2c_client *client, const struct i2c_device_id *id)
if (chip.flags & AT24_FLAG_SERIAL || chip.flags & AT24_FLAG_MAC)
chip.flags |= AT24_FLAG_READONLY;
- if (!is_power_of_2(chip.byte_len))
- dev_warn(dev, "byte_len looks suspicious (no power of 2)!\n");
if (!chip.page_size) {
dev_err(dev, "page_size must not be 0!\n");
return -EINVAL;
--
2.15.0
^ permalink raw reply related [flat|nested] 16+ messages in thread* [PATCH 7/7] eeprom: at24: don't check page_size for read-only chips and reorder checks
2017-11-30 6:40 [PATCH 0/7] eeprom: at24: series with smaller improvements Heiner Kallweit
` (5 preceding siblings ...)
2017-11-30 6:49 ` [PATCH 6/7] eeprom: at24: don't check chip.byte_len for power of two Heiner Kallweit
@ 2017-11-30 6:49 ` Heiner Kallweit
6 siblings, 0 replies; 16+ messages in thread
From: Heiner Kallweit @ 2017-11-30 6:49 UTC (permalink / raw)
To: Bartosz Golaszewski; +Cc: linux-i2c@vger.kernel.org
Currently we check for a proper page_size value also for read-only chips
where this value isn't used. Therefore remove these checks for read-only
chips.
In addition reorder checks to do sanity checking first.
Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
drivers/misc/eeprom/at24.c | 33 +++++++++++++++++----------------
1 file changed, 17 insertions(+), 16 deletions(-)
diff --git a/drivers/misc/eeprom/at24.c b/drivers/misc/eeprom/at24.c
index 74d2347a1..2348da953 100644
--- a/drivers/misc/eeprom/at24.c
+++ b/drivers/misc/eeprom/at24.c
@@ -535,16 +535,28 @@ static int at24_probe(struct i2c_client *client, const struct i2c_device_id *id)
at24_get_pdata(dev, &chip);
}
+ if ((chip.flags & AT24_FLAG_SERIAL) && (chip.flags & AT24_FLAG_MAC)) {
+ dev_err(dev, "invalid device data - cannot have both AT24_FLAG_SERIAL & AT24_FLAG_MAC.");
+ return -EINVAL;
+ }
+
/* both flags imply read-only */
if (chip.flags & AT24_FLAG_SERIAL || chip.flags & AT24_FLAG_MAC)
chip.flags |= AT24_FLAG_READONLY;
- if (!chip.page_size) {
- dev_err(dev, "page_size must not be 0!\n");
- return -EINVAL;
+ writable = !(chip.flags & AT24_FLAG_READONLY);
+ if (writable) {
+ if (!chip.page_size) {
+ dev_err(dev, "page_size must not be 0!\n");
+ return -EINVAL;
+ }
+ if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C) &&
+ !i2c_check_functionality(client->adapter,
+ I2C_FUNC_SMBUS_WRITE_I2C_BLOCK))
+ chip.page_size = 1;
+ if (!is_power_of_2(chip.page_size))
+ dev_warn(dev, "page_size looks suspicious (no power of 2)!\n");
}
- if (!is_power_of_2(chip.page_size))
- dev_warn(dev, "page_size looks suspicious (no power of 2)!\n");
/*
* REVISIT: the size of the EUI-48 byte array is 6 in at24mac402, while
@@ -556,11 +568,6 @@ static int at24_probe(struct i2c_client *client, const struct i2c_device_id *id)
if (chip.flags & AT24_FLAG_MAC && chip.byte_len == 4)
chip.byte_len = 6;
- if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C) &&
- !i2c_check_functionality(client->adapter,
- I2C_FUNC_SMBUS_WRITE_I2C_BLOCK))
- chip.page_size = 1;
-
if (chip.flags & AT24_FLAG_TAKE8ADDR)
num_addresses = 8;
else
@@ -587,12 +594,6 @@ static int at24_probe(struct i2c_client *client, const struct i2c_device_id *id)
if (IS_ERR(at24->client[0].regmap))
return PTR_ERR(at24->client[0].regmap);
- if ((chip.flags & AT24_FLAG_SERIAL) && (chip.flags & AT24_FLAG_MAC)) {
- dev_err(dev, "invalid device data - cannot have both AT24_FLAG_SERIAL & AT24_FLAG_MAC.");
- return -EINVAL;
- }
-
- writable = !(chip.flags & AT24_FLAG_READONLY);
if (writable) {
at24->write_max = min_t(unsigned int, chip.page_size, io_limit);
if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C) &&
--
2.15.0
^ permalink raw reply related [flat|nested] 16+ messages in thread