All of lore.kernel.org
 help / color / mirror / Atom feed
* [U-Boot] [PATCH] w1: fix build without CONFIG_W1_EEPROM
@ 2018-10-22 16:51 Martin Fuzzey
  2018-10-23  7:07 ` Eugen.Hristev at microchip.com
  0 siblings, 1 reply; 7+ messages in thread
From: Martin Fuzzey @ 2018-10-22 16:51 UTC (permalink / raw)
  To: u-boot

Building with CONFIG_W1 and CONFIG_CMD_W1 but without CONFIG_W1_EEPROM
fails with
	drivers/w1/w1-uclass.c:104: undefined reference to `w1_eeprom_register_new_device'
	cmd/w1.c:93: undefined reference to `w1_eeprom_read_buf'

Fix this.

Signed-off-by: Martin Fuzzey <martin.fuzzey@flowbird.group>
---
 cmd/w1.c               | 9 ++++++++-
 drivers/w1/w1-uclass.c | 2 ++
 2 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/cmd/w1.c b/cmd/w1.c
index 9c95fcf..9765bd6 100644
--- a/cmd/w1.c
+++ b/cmd/w1.c
@@ -41,6 +41,7 @@ static int w1_bus(void)
 	return CMD_RET_SUCCESS;
 }
 
+#ifdef CONFIG_W1_EEPROM
 static int w1_read(int argc, char *const argv[])
 {
 	int bus_n = 0, dev_n = 0, offset = 0, len = 512;
@@ -102,6 +103,7 @@ static int w1_read(int argc, char *const argv[])
 
 	return CMD_RET_SUCCESS;
 }
+#endif
 
 int do_w1(cmd_tbl_t *cmdtp, int flag, int argc, char *const argv[])
 {
@@ -111,8 +113,10 @@ int do_w1(cmd_tbl_t *cmdtp, int flag, int argc, char *const argv[])
 	if (!strcmp(argv[1], "bus"))
 		return w1_bus();
 
+#ifdef CONFIG_W1_EEPROM
 	if (!strcmp(argv[1], "read"))
 		return w1_read(argc, argv);
+#endif
 
 	return CMD_RET_SUCCESS;
 }
@@ -120,7 +124,10 @@ int do_w1(cmd_tbl_t *cmdtp, int flag, int argc, char *const argv[])
 U_BOOT_CMD(w1, 6, 0, do_w1,
 	   "onewire interface utility commands",
 	   "bus - show onewire bus info (all)\n"
+#ifdef CONFIG_W1_EEPROM
 	   "w1 read [<bus> [<dev> [offset [length]]]]"
 	   "    - read from onewire device 'dev' on onewire bus 'bus'"
 	   " starting from offset 'offset' and length 'length'\n"
-	   "      defaults: bus 0, dev 0, offset 0, length 512 bytes.");
+	   "      defaults: bus 0, dev 0, offset 0, length 512 bytes."
+#endif
+	   );
diff --git a/drivers/w1/w1-uclass.c b/drivers/w1/w1-uclass.c
index cb41b68..5544b19 100644
--- a/drivers/w1/w1-uclass.c
+++ b/drivers/w1/w1-uclass.c
@@ -100,8 +100,10 @@ static int w1_enumerate(struct udevice *bus)
 			debug("%s: Detected new device 0x%llx (family 0x%x)\n",
 			      bus->name, rn, (u8)(rn & 0xff));
 
+#ifdef CONFIG_W1_EEPROM
 			/* attempt to register as w1-eeprom device */
 			w1_eeprom_register_new_device(rn);
+#endif
 		}
 	}
 
-- 
1.9.1

^ permalink raw reply related	[flat|nested] 7+ messages in thread

* [U-Boot] [PATCH] w1: fix build without CONFIG_W1_EEPROM
  2018-10-22 16:51 [U-Boot] [PATCH] w1: fix build without CONFIG_W1_EEPROM Martin Fuzzey
@ 2018-10-23  7:07 ` Eugen.Hristev at microchip.com
  2018-10-23  8:31   ` Martin Fuzzey
  0 siblings, 1 reply; 7+ messages in thread
From: Eugen.Hristev at microchip.com @ 2018-10-23  7:07 UTC (permalink / raw)
  To: u-boot



On 22.10.2018 19:51, Martin Fuzzey wrote:
> Building with CONFIG_W1 and CONFIG_CMD_W1 but without CONFIG_W1_EEPROM
> fails with
> 	drivers/w1/w1-uclass.c:104: undefined reference to `w1_eeprom_register_new_device'
> 	cmd/w1.c:93: undefined reference to `w1_eeprom_read_buf'
> 
> Fix this.

I would prefer if you let the w1 read command to be accessible 
regardless if CONFIG_W1_EEPROM is defined or not. Hence have only the w1 
eeprom reads under the ifdef...
The w1_read checks for devices anyway and for the bus, so it should 
print invalid bus/device if nothing is present there.
Any opinion on this ?

Thanks,
Eugen
> 
> Signed-off-by: Martin Fuzzey <martin.fuzzey@flowbird.group>
> ---
>   cmd/w1.c               | 9 ++++++++-
>   drivers/w1/w1-uclass.c | 2 ++
>   2 files changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/cmd/w1.c b/cmd/w1.c
> index 9c95fcf..9765bd6 100644
> --- a/cmd/w1.c
> +++ b/cmd/w1.c
> @@ -41,6 +41,7 @@ static int w1_bus(void)
>   	return CMD_RET_SUCCESS;
>   }
>   
> +#ifdef CONFIG_W1_EEPROM
>   static int w1_read(int argc, char *const argv[])
>   {
>   	int bus_n = 0, dev_n = 0, offset = 0, len = 512;
> @@ -102,6 +103,7 @@ static int w1_read(int argc, char *const argv[])
>   
>   	return CMD_RET_SUCCESS;
>   }
> +#endif
>   
>   int do_w1(cmd_tbl_t *cmdtp, int flag, int argc, char *const argv[])
>   {
> @@ -111,8 +113,10 @@ int do_w1(cmd_tbl_t *cmdtp, int flag, int argc, char *const argv[])
>   	if (!strcmp(argv[1], "bus"))
>   		return w1_bus();
>   
> +#ifdef CONFIG_W1_EEPROM
>   	if (!strcmp(argv[1], "read"))
>   		return w1_read(argc, argv);
> +#endif
>   
>   	return CMD_RET_SUCCESS;
>   }
> @@ -120,7 +124,10 @@ int do_w1(cmd_tbl_t *cmdtp, int flag, int argc, char *const argv[])
>   U_BOOT_CMD(w1, 6, 0, do_w1,
>   	   "onewire interface utility commands",
>   	   "bus - show onewire bus info (all)\n"
> +#ifdef CONFIG_W1_EEPROM
>   	   "w1 read [<bus> [<dev> [offset [length]]]]"
>   	   "    - read from onewire device 'dev' on onewire bus 'bus'"
>   	   " starting from offset 'offset' and length 'length'\n"
> -	   "      defaults: bus 0, dev 0, offset 0, length 512 bytes.");
> +	   "      defaults: bus 0, dev 0, offset 0, length 512 bytes."
> +#endif
> +	   );
> diff --git a/drivers/w1/w1-uclass.c b/drivers/w1/w1-uclass.c
> index cb41b68..5544b19 100644
> --- a/drivers/w1/w1-uclass.c
> +++ b/drivers/w1/w1-uclass.c
> @@ -100,8 +100,10 @@ static int w1_enumerate(struct udevice *bus)
>   			debug("%s: Detected new device 0x%llx (family 0x%x)\n",
>   			      bus->name, rn, (u8)(rn & 0xff));
>   
> +#ifdef CONFIG_W1_EEPROM
>   			/* attempt to register as w1-eeprom device */
>   			w1_eeprom_register_new_device(rn);
> +#endif
>   		}
>   	}
>   
> 

^ permalink raw reply	[flat|nested] 7+ messages in thread

* [U-Boot] [PATCH] w1: fix build without CONFIG_W1_EEPROM
  2018-10-23  7:07 ` Eugen.Hristev at microchip.com
@ 2018-10-23  8:31   ` Martin Fuzzey
  2018-10-23  8:40     ` Eugen.Hristev at microchip.com
  0 siblings, 1 reply; 7+ messages in thread
From: Martin Fuzzey @ 2018-10-23  8:31 UTC (permalink / raw)
  To: u-boot

On 23/10/18 09:07, Eugen.Hristev at microchip.com wrote:
>
> On 22.10.2018 19:51, Martin Fuzzey wrote:
>> Building with CONFIG_W1 and CONFIG_CMD_W1 but without CONFIG_W1_EEPROM
>> fails with
>> 	drivers/w1/w1-uclass.c:104: undefined reference to `w1_eeprom_register_new_device'
>> 	cmd/w1.c:93: undefined reference to `w1_eeprom_read_buf'
>>
>> Fix this.
> I would prefer if you let the w1 read command to be accessible
> regardless if CONFIG_W1_EEPROM is defined or not. Hence have only the w1
> eeprom reads under the ifdef...
> The w1_read checks for devices anyway and for the bus, so it should
> print invalid bus/device if nothing is present there.
> Any opinion on this ?

I don't really have a strong opinion on this.

Completely removing non implemented commands seems to be a common thing 
to do in u-boot (cmd/i2c.c for instance) presumably to keep the image 
size as small as possible.
But for the one wire case the code space saving is likely to be small 
and, currently at least, there is little point buiding without 
CONFIG_W1_EEPROM, not sure if that will change some day - of course 
there are other types of one wire devices like various sensors but they 
are probably of less interest in the context of a bootloader.

Let's wait a bit and see what Maxime or anyone else has to say about this.

Regards,

Martin

^ permalink raw reply	[flat|nested] 7+ messages in thread

* [U-Boot] [PATCH] w1: fix build without CONFIG_W1_EEPROM
  2018-10-23  8:31   ` Martin Fuzzey
@ 2018-10-23  8:40     ` Eugen.Hristev at microchip.com
  2018-10-23 14:09       ` Martin Fuzzey
  0 siblings, 1 reply; 7+ messages in thread
From: Eugen.Hristev at microchip.com @ 2018-10-23  8:40 UTC (permalink / raw)
  To: u-boot



On 23.10.2018 11:31, Martin Fuzzey wrote:
> On 23/10/18 09:07, Eugen.Hristev at microchip.com wrote:
>>
>> On 22.10.2018 19:51, Martin Fuzzey wrote:
>>> Building with CONFIG_W1 and CONFIG_CMD_W1 but without CONFIG_W1_EEPROM
>>> fails with
>>>     drivers/w1/w1-uclass.c:104: undefined reference to 
>>> `w1_eeprom_register_new_device'
>>>     cmd/w1.c:93: undefined reference to `w1_eeprom_read_buf'
>>>
>>> Fix this.
>> I would prefer if you let the w1 read command to be accessible
>> regardless if CONFIG_W1_EEPROM is defined or not. Hence have only the w1
>> eeprom reads under the ifdef...
>> The w1_read checks for devices anyway and for the bus, so it should
>> print invalid bus/device if nothing is present there.
>> Any opinion on this ?
> 
> I don't really have a strong opinion on this.
> 
> Completely removing non implemented commands seems to be a common thing 
> to do in u-boot (cmd/i2c.c for instance) presumably to keep the image 
> size as small as possible.
> But for the one wire case the code space saving is likely to be small 
> and, currently at least, there is little point buiding without 
> CONFIG_W1_EEPROM, not sure if that will change some day - of course 
> there are other types of one wire devices like various sensors but they 
> are probably of less interest in the context of a bootloader.
> 
> Let's wait a bit and see what Maxime or anyone else has to say about this.

I tried as much as possible to decouple the W1 bus from the W1 EEPROM 
memories. It is possible that we will have a different framework for 
EEPROMs that will include both 1wire and i2c eeproms, and then the 
interfacing would be pretty easy to change to.

That's why I am thinking that w1 bus read should not be much affected if 
the 1w EEPROMs are unknown to U-boot

> 
> Regards,
> 
> Martin
> 
> 

^ permalink raw reply	[flat|nested] 7+ messages in thread

* [U-Boot] [PATCH] w1: fix build without CONFIG_W1_EEPROM
  2018-10-23  8:40     ` Eugen.Hristev at microchip.com
@ 2018-10-23 14:09       ` Martin Fuzzey
  2018-10-23 22:31         ` Lukasz Majewski
  0 siblings, 1 reply; 7+ messages in thread
From: Martin Fuzzey @ 2018-10-23 14:09 UTC (permalink / raw)
  To: u-boot

On 23/10/18 10:40, Eugen.Hristev at microchip.com wrote:
>
> I tried as much as possible to decouple the W1 bus from the W1 EEPROM
> memories. It is possible that we will have a different framework for
> EEPROMs that will include both 1wire and i2c eeproms, and then the
> interfacing would be pretty easy to change to.
>
> That's why I am thinking that w1 bus read should not be much affected if
> the 1w EEPROMs are unknown to U-boot
>
Yes sure that's great.

Somewhat diverting this thread....

I noticed that currently "w1 read" displays the data in hex with "%x", 
which means it prints just one character if in range 00-0f.
And as there is no seperator the output isn't currently useful.

That's a trivial fix of course but it got me thinking about what do we 
want to "w1 read" to do?

A lot of other storage reading commands use "read" to mean "read to 
memory" and take a destination RAM address parameter.
Eg mmc read, sf read, ...

There are counter examples though like "mii read" or "pmic read" which 
just print the output but they don't read aribitary
user defined data like an eeprom.

So I was wondering if it would be better to modify "w1 read" to mean 
"read to memory" and add a new "w1 dump" command
to do what "w1 read" currently does (as it is much more convenient for 
quickly checking the eeprom contents than
having to use md).

The type of use case I'm thinking of for "read to memory" is if someone 
wants to put a device tree blob on an eeprom.

What do you think?

Regards,

Martin



>> Regards,
>>
>> Martin
>>
>>

^ permalink raw reply	[flat|nested] 7+ messages in thread

* [U-Boot] [PATCH] w1: fix build without CONFIG_W1_EEPROM
  2018-10-23 14:09       ` Martin Fuzzey
@ 2018-10-23 22:31         ` Lukasz Majewski
  2018-10-24 10:08           ` Eugen.Hristev at microchip.com
  0 siblings, 1 reply; 7+ messages in thread
From: Lukasz Majewski @ 2018-10-23 22:31 UTC (permalink / raw)
  To: u-boot

Hi Martin, Eugen,

> On 23/10/18 10:40, Eugen.Hristev at microchip.com wrote:
> >
> > I tried as much as possible to decouple the W1 bus from the W1
> > EEPROM memories. It is possible that we will have a different
> > framework for EEPROMs that will include both 1wire and i2c eeproms,
> > and then the interfacing would be pretty easy to change to.
> >
> > That's why I am thinking that w1 bus read should not be much
> > affected if the 1w EEPROMs are unknown to U-boot
> >  
> Yes sure that's great.
> 
> Somewhat diverting this thread....
> 
> I noticed that currently "w1 read" displays the data in hex with
> "%x", which means it prints just one character if in range 00-0f.
> And as there is no seperator the output isn't currently useful.
> 
> That's a trivial fix of course but it got me thinking about what do
> we want to "w1 read" to do?
> 
> A lot of other storage reading commands use "read" to mean "read to 
> memory" and take a destination RAM address parameter.
> Eg mmc read, sf read, ...
> 
> There are counter examples though like "mii read" or "pmic read"
> which just print the output but they don't read aribitary
> user defined data like an eeprom.
> 
> So I was wondering if it would be better to modify "w1 read" to mean 
> "read to memory" and add a new "w1 dump" command
> to do what "w1 read" currently does (as it is much more convenient
> for quickly checking the eeprom contents than
> having to use md).
> 
> The type of use case I'm thinking of for "read to memory" is if
> someone wants to put a device tree blob on an eeprom.
> 

If I may add my 2cents.

I also would like to see the w1 command to load read data to memory (to
be consistent with e.g. load), so we could use md.b, mw.b, etc.

> What do you think?
> 
> Regards,
> 
> Martin
> 
> 
> 
> >> Regards,
> >>
> >> Martin
> >>
> >>  
> 
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> https://lists.denx.de/listinfo/u-boot




Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 488 bytes
Desc: OpenPGP digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20181024/beb37de8/attachment.sig>

^ permalink raw reply	[flat|nested] 7+ messages in thread

* [U-Boot] [PATCH] w1: fix build without CONFIG_W1_EEPROM
  2018-10-23 22:31         ` Lukasz Majewski
@ 2018-10-24 10:08           ` Eugen.Hristev at microchip.com
  0 siblings, 0 replies; 7+ messages in thread
From: Eugen.Hristev at microchip.com @ 2018-10-24 10:08 UTC (permalink / raw)
  To: u-boot



On 24.10.2018 01:31, Lukasz Majewski wrote:
> Hi Martin, Eugen,
> 
>> On 23/10/18 10:40, Eugen.Hristev at microchip.com wrote:
>>>
>>> I tried as much as possible to decouple the W1 bus from the W1
>>> EEPROM memories. It is possible that we will have a different
>>> framework for EEPROMs that will include both 1wire and i2c eeproms,
>>> and then the interfacing would be pretty easy to change to.
>>>
>>> That's why I am thinking that w1 bus read should not be much
>>> affected if the 1w EEPROMs are unknown to U-boot
>>>   
>> Yes sure that's great.
>>
>> Somewhat diverting this thread....
>>
>> I noticed that currently "w1 read" displays the data in hex with
>> "%x", which means it prints just one character if in range 00-0f.
>> And as there is no seperator the output isn't currently useful.
>>
>> That's a trivial fix of course but it got me thinking about what do
>> we want to "w1 read" to do?
>>
>> A lot of other storage reading commands use "read" to mean "read to
>> memory" and take a destination RAM address parameter.
>> Eg mmc read, sf read, ...
>>
>> There are counter examples though like "mii read" or "pmic read"
>> which just print the output but they don't read aribitary
>> user defined data like an eeprom.
>>
>> So I was wondering if it would be better to modify "w1 read" to mean
>> "read to memory" and add a new "w1 dump" command
>> to do what "w1 read" currently does (as it is much more convenient
>> for quickly checking the eeprom contents than
>> having to use md).
>>
>> The type of use case I'm thinking of for "read to memory" is if
>> someone wants to put a device tree blob on an eeprom.
>>
> 
> If I may add my 2cents.
> 
> I also would like to see the w1 command to load read data to memory (to
> be consistent with e.g. load), so we could use md.b, mw.b, etc.
> 
>> What do you think?

I believe it's a good thing to have them as you said. The w1 support 
which I made was "initial" and "minimal" . I am happy to see it extended.

Eugen

>>
>> Regards,
>>
>> Martin
>>
>>
>>
>>>> Regards,
>>>>
>>>> Martin
>>>>
>>>>   
>>
>> _______________________________________________
>> U-Boot mailing list
>> U-Boot at lists.denx.de
>> https://lists.denx.de/listinfo/u-boot
> 
> 
> 
> 
> Best regards,
> 
> Lukasz Majewski
> 
> --
> 
> DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
> Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
> 

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2018-10-24 10:08 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-10-22 16:51 [U-Boot] [PATCH] w1: fix build without CONFIG_W1_EEPROM Martin Fuzzey
2018-10-23  7:07 ` Eugen.Hristev at microchip.com
2018-10-23  8:31   ` Martin Fuzzey
2018-10-23  8:40     ` Eugen.Hristev at microchip.com
2018-10-23 14:09       ` Martin Fuzzey
2018-10-23 22:31         ` Lukasz Majewski
2018-10-24 10:08           ` Eugen.Hristev at microchip.com

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.