[U-Boot] [PATCH v2 1/2] misc: uclass: Add enable/disable function

[U-Boot] [PATCH v2 1/2] misc: uclass: Add enable/disable function

[Mario Six]

Add generic enable/disable function to the misc uclass.

Signed-off-by: Mario Six <mario.six@gdsys.cc>
---

v1 -> v2:
* Merged the two functions into one function
* Explained the semantics of enabling/disabling more throughly

---
 drivers/misc/misc-uclass.c | 10 ++++++++++
 include/misc.h             | 25 +++++++++++++++++++++++++
 2 files changed, 35 insertions(+)

diff --git a/drivers/misc/misc-uclass.c b/drivers/misc/misc-uclass.c
index d9eea3dac5..0e3e0e8bf7 100644
--- a/drivers/misc/misc-uclass.c
+++ b/drivers/misc/misc-uclass.c
@@ -56,6 +56,16 @@ int misc_call(struct udevice *dev, int msgid, void *tx_msg, int tx_size,
 	return ops->call(dev, msgid, tx_msg, tx_size, rx_msg, rx_size);
 }
 
+int misc_set_enabled(struct udevice *dev, bool val)
+{
+	const struct misc_ops *ops = device_get_ops(dev);
+
+	if (!ops->set_enabled)
+		return -ENOSYS;
+
+	return ops->set_enabled(dev, val);
+}
+
 UCLASS_DRIVER(misc) = {
 	.id		= UCLASS_MISC,
 	.name		= "misc",
diff --git a/include/misc.h b/include/misc.h
index 03ef55cdc8..04a4b65155 100644
--- a/include/misc.h
+++ b/include/misc.h
@@ -58,6 +58,22 @@ int misc_ioctl(struct udevice *dev, unsigned long request, void *buf);
 int misc_call(struct udevice *dev, int msgid, void *tx_msg, int tx_size,
 	      void *rx_msg, int rx_size);
 
+/*
+ * Enable or disable a device.
+ *
+ * The semantics of "disable" and "enable" should be understood here as
+ * activating or deactivating the device's primary function, hence a "disabled"
+ * device should be dormant, but still answer to commands and queries.
+ *
+ * A probed device may start in a disabled or enabled state, depending on the
+ * driver and hardware.
+ *
+ * @dev: the device to enable or disable.
+ * @val: the flag that tells the driver to either enable or disable the device.
+ * @return: 0 if OK, -ve on error
+ */
+int misc_set_enabled(struct udevice *dev, bool val);
+
 /*
  * struct misc_ops - Driver model Misc operations
  *
@@ -109,6 +125,15 @@ struct misc_ops {
 	 */
 	int (*call)(struct udevice *dev, int msgid, void *tx_msg, int tx_size,
 		    void *rx_msg, int rx_size);
+	/*
+	 * Enable or disable a device, optional.
+	 *
+	 * @dev: the device to enable.
+	 * @val: the flag that tells the driver to either enable or disable the
+	 *	 device.
+	 * @return: 0 if OK, -ve on error
+	 */
+	int (*set_enabled)(struct udevice *dev, bool val);
 };
 
 #endif	/* _MISC_H_ */
-- 
2.16.1

[U-Boot] [PATCH v2 2/2] misc: Add gdsys_ioep driver

[Mario Six]

Add driver for the IHS IO endpoint on IHS FPGAs.

Signed-off-by: Mario Six <mario.six@gdsys.cc>
---

v1 -> v2:
* Switched to regmap usage (instead of fpgamap)

---
 drivers/misc/Kconfig      |   5 ++
 drivers/misc/Makefile     |   1 +
 drivers/misc/gdsys_ioep.c | 150 ++++++++++++++++++++++++++++++++++++++++++++++
 drivers/misc/gdsys_ioep.h |  73 ++++++++++++++++++++++
 4 files changed, 229 insertions(+)
 create mode 100644 drivers/misc/gdsys_ioep.c
 create mode 100644 drivers/misc/gdsys_ioep.h

diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
index d774569cbc..db688f440d 100644
--- a/drivers/misc/Kconfig
+++ b/drivers/misc/Kconfig
@@ -264,4 +264,9 @@ config SYS_I2C_EEPROM_ADDR_OVERFLOW
 endif
 
 
+config GDSYS_IOEP
+	bool "Enable gdsys IOEP driver"
+	depends on MISC
+	help
+	  Support gdsys FPGA's IO endpoint driver.
 endmenu
diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
index e8d598cd47..0aa1e3fa4b 100644
--- a/drivers/misc/Makefile
+++ b/drivers/misc/Makefile
@@ -54,3 +54,4 @@ obj-$(CONFIG_QFW) += qfw.o
 obj-$(CONFIG_ROCKCHIP_EFUSE) += rockchip-efuse.o
 obj-$(CONFIG_STM32_RCC) += stm32_rcc.o
 obj-$(CONFIG_SYS_DPAA_QBMAN) += fsl_portals.o
+obj-$(CONFIG_GDSYS_IOEP) += gdsys_ioep.o
diff --git a/drivers/misc/gdsys_ioep.c b/drivers/misc/gdsys_ioep.c
new file mode 100644
index 0000000000..552c1872c8
--- /dev/null
+++ b/drivers/misc/gdsys_ioep.c
@@ -0,0 +1,150 @@
+/*
+ * (C) Copyright 2017
+ * Mario Six,  Guntermann & Drunck GmbH, mario.six at gdsys.cc
+ *
+ * based on the cmd_ioloop driver/command, which is
+ *
+ * (C) Copyright 2014
+ * Dirk Eibach, Guntermann & Drunck GmbH, dirk.eibach at gdsys.cc
+ *
+ * SPDX-License-Identifier:	GPL-2.0+
+ */
+
+#include <common.h>
+#include <dm.h>
+#include <misc.h>
+#include <regmap.h>
+
+#include "gdsys_ioep.h"
+
+struct gdsys_ioep_priv {
+	struct regmap *map;
+};
+
+enum last_spec {
+	READ_DATA_IS_LAST,
+	READ_DATA_IS_NOT_LAST,
+};
+
+int gdsys_ioep_set_receive(struct udevice *dev, bool val)
+{
+	struct gdsys_ioep_priv *priv = dev_get_priv(dev);
+	u16 state;
+
+	if (val)
+		state = CTRL_PROC_RECEIVE_ENABLE;
+	else
+		state = ~CTRL_PROC_RECEIVE_ENABLE;
+
+	gdsys_ioep_set(priv->map, tx_control, state);
+
+	if (val) {
+		/* Set device address to dummy 1*/
+		gdsys_ioep_set(priv->map, device_address, 1);
+	}
+
+	return 0;
+}
+
+int gdsys_ioep_send(struct udevice *dev, int offset, const void *buf, int size)
+{
+	struct gdsys_ioep_priv *priv = dev_get_priv(dev);
+	int k;
+	u16 *p = (u16 *)buf;
+
+	for (k = 0; k < size; ++k)
+		gdsys_ioep_set(priv->map, transmit_data, *(p++));
+
+	gdsys_ioep_set(priv->map, tx_control, CTRL_PROC_RECEIVE_ENABLE |
+					      CTRL_FLUSH_TRANSMIT_BUFFER);
+
+	return 0;
+}
+
+int receive_byte_buffer(struct udevice *dev, uint len, u16 *buffer, enum last_spec last_spec)
+{
+	struct gdsys_ioep_priv *priv = dev_get_priv(dev);
+	int k;
+	int res = -EIO;
+
+	for (k = 0; k < len; ++k) {
+		u16 rx_tx_status;
+
+		gdsys_ioep_get(priv->map, receive_data, buffer++);
+
+		gdsys_ioep_get(priv->map, rx_tx_status, &rx_tx_status);
+		if (k == (len - 1) && (last_spec == READ_DATA_IS_NOT_LAST ||
+				       rx_tx_status & STATE_RX_DATA_LAST))
+			res = 0;
+	}
+
+	return res;
+}
+
+int gdsys_ioep_receive(struct udevice *dev, int offset, void *buf, int size)
+{
+	int res1, res2;
+	struct io_generic_packet header;
+	u16 *p = (u16 *)buf;
+	const int header_words = sizeof(struct io_generic_packet) / 2;
+	uint len;
+
+	res1 = receive_byte_buffer(dev, header_words, p, READ_DATA_IS_NOT_LAST);
+	memcpy(&header, p, 2 * header_words);
+	p += header_words;
+
+	len = (header.packet_length + 1) / 2;
+
+	if (!res1)
+		res2 = receive_byte_buffer(dev, len, p, READ_DATA_IS_LAST);
+
+	return res1 ? res1 : res2;
+}
+
+int gdsys_ioep_get_and_reset_status(struct udevice *dev, int msgid,
+				    void *tx_msg, int tx_size, void *rx_msg,
+				    int rx_size)
+{
+	struct gdsys_ioep_priv *priv = dev_get_priv(dev);
+	const u16 mask = STATE_RX_DIST_ERR | STATE_RX_LENGTH_ERR |
+			 STATE_RX_FRAME_CTR_ERR | STATE_RX_FCS_ERR |
+			 STATE_RX_PACKET_DROPPED | STATE_TX_ERR;
+	u16 *status = rx_msg;
+
+	gdsys_ioep_get(priv->map, rx_tx_status, status);
+
+	gdsys_ioep_set(priv->map, rx_tx_status, *status);
+
+	return (*status & mask) ? 1 : 0;
+}
+
+static const struct misc_ops gdsys_ioep_ops = {
+	.set_enabled = gdsys_ioep_set_receive,
+	.write = gdsys_ioep_send,
+	.read = gdsys_ioep_receive,
+	.call = gdsys_ioep_get_and_reset_status,
+};
+
+int gdsys_ioep_probe(struct udevice *dev)
+{
+	struct gdsys_ioep_priv *priv = dev_get_priv(dev);
+
+	regmap_init_mem(dev, &priv->map);
+
+	return 0;
+}
+
+static const struct udevice_id gdsys_ioep_ids[] = {
+	{ .compatible = "gdsys,io-endpoint" },
+	{ }
+};
+
+U_BOOT_DRIVER(gdsys_ioep) = {
+	.name           = "gdsys_ioep",
+	.id             = UCLASS_MISC,
+	.ops		= &gdsys_ioep_ops,
+	.flags		= DM_UC_FLAG_SEQ_ALIAS,
+	.of_match       = gdsys_ioep_ids,
+	.probe          = gdsys_ioep_probe,
+	.priv_auto_alloc_size = sizeof(struct gdsys_ioep_priv),
+};
diff --git a/drivers/misc/gdsys_ioep.h b/drivers/misc/gdsys_ioep.h
new file mode 100644
index 0000000000..cccf580091
--- /dev/null
+++ b/drivers/misc/gdsys_ioep.h
@@ -0,0 +1,73 @@
+/*
+ * (C) Copyright 2018
+ * Mario Six, Guntermann & Drunck GmbH, mario.six at gdsys.cc
+ *
+ * SPDX-License-Identifier:	GPL-2.0+
+ */
+
+#ifndef __GDSYS_IOEP_H_
+#define __GDSYS_IOEP_H_
+
+/**
+ * struct io_generic_packet - header structure for GDSYS IOEP packets
+ *
+ * @target_address:     Target protocol address of the packet.
+ * @source_address:     Source protocol address of the packet.
+ * @packet_type:        Packet type.
+ * @bc:                 Block counter (filled in by FPGA).
+ * @packet_length:      Length of the packet's payload bytes.
+ */
+struct io_generic_packet {
+        u16 target_address;
+        u16 source_address;
+        u8 packet_type;
+        u8 bc;
+        u16 packet_length;
+} __attribute__((__packed__));
+
+struct gdsys_ioep_regs {
+	u16 transmit_data;
+	u16 tx_control;
+	u16 receive_data;
+	u16 rx_tx_status;
+	u16 device_address;
+	u16 target_address;
+	u16 int_enable;
+};
+
+#define gdsys_ioep_set(map, member, val) \
+	regmap_set(map, struct gdsys_ioep_regs, member, val)
+
+#define gdsys_ioep_get(map, member, valp) \
+	regmap_get(map, struct gdsys_ioep_regs, member, valp)
+enum {
+	STATE_TX_PACKET_BUILDING = BIT(0),
+	STATE_TX_TRANSMITTING = BIT(1),
+	STATE_TX_BUFFER_FULL = BIT(2),
+	STATE_TX_ERR = BIT(3),
+	STATE_RECEIVE_TIMEOUT = BIT(4),
+	STATE_PROC_RX_STORE_TIMEOUT = BIT(5),
+	STATE_PROC_RX_RECEIVE_TIMEOUT = BIT(6),
+	STATE_RX_DIST_ERR = BIT(7),
+	STATE_RX_LENGTH_ERR = BIT(8),
+	STATE_RX_FRAME_CTR_ERR = BIT(9),
+	STATE_RX_FCS_ERR = BIT(10),
+	STATE_RX_PACKET_DROPPED = BIT(11),
+	STATE_RX_DATA_LAST = BIT(12),
+	STATE_RX_DATA_FIRST = BIT(13),
+	STATE_RX_DATA_AVAILABLE = BIT(15),
+};
+
+enum {
+	CTRL_PROC_RECEIVE_ENABLE = BIT(12),
+	CTRL_FLUSH_TRANSMIT_BUFFER = BIT(15),
+};
+
+enum {
+	IRQ_CPU_TRANSMITBUFFER_FREE_STATUS = BIT(5),
+	IRQ_CPU_PACKET_TRANSMITTED_EVENT = BIT(6),
+	IRQ_NEW_CPU_PACKET_RECEIVED_EVENT = BIT(7),
+	IRQ_CPU_RECEIVE_DATA_AVAILABLE_STATUS = BIT(8),
+};
+
+#endif /* __GDSYS_IOEP_H_ */
-- 
2.16.1

[U-Boot] [PATCH v2 1/2] misc: uclass: Add enable/disable function

[Michal Simek]

On 27.4.2018 14:52, Mario Six wrote:
> Add generic enable/disable function to the misc uclass.
> 
> Signed-off-by: Mario Six <mario.six@gdsys.cc>
> ---
> 
> v1 -> v2:
> * Merged the two functions into one function
> * Explained the semantics of enabling/disabling more throughly
> 
> ---
>  drivers/misc/misc-uclass.c | 10 ++++++++++
>  include/misc.h             | 25 +++++++++++++++++++++++++
>  2 files changed, 35 insertions(+)
> 
> diff --git a/drivers/misc/misc-uclass.c b/drivers/misc/misc-uclass.c
> index d9eea3dac5..0e3e0e8bf7 100644
> --- a/drivers/misc/misc-uclass.c
> +++ b/drivers/misc/misc-uclass.c
> @@ -56,6 +56,16 @@ int misc_call(struct udevice *dev, int msgid, void *tx_msg, int tx_size,
>  	return ops->call(dev, msgid, tx_msg, tx_size, rx_msg, rx_size);
>  }
>  
> +int misc_set_enabled(struct udevice *dev, bool val)
> +{
> +	const struct misc_ops *ops = device_get_ops(dev);
> +
> +	if (!ops->set_enabled)
> +		return -ENOSYS;
> +
> +	return ops->set_enabled(dev, val);
> +}
> +
>  UCLASS_DRIVER(misc) = {
>  	.id		= UCLASS_MISC,
>  	.name		= "misc",
> diff --git a/include/misc.h b/include/misc.h
> index 03ef55cdc8..04a4b65155 100644
> --- a/include/misc.h
> +++ b/include/misc.h
> @@ -58,6 +58,22 @@ int misc_ioctl(struct udevice *dev, unsigned long request, void *buf);
>  int misc_call(struct udevice *dev, int msgid, void *tx_msg, int tx_size,
>  	      void *rx_msg, int rx_size);
>  
> +/*
> + * Enable or disable a device.
> + *
> + * The semantics of "disable" and "enable" should be understood here as
> + * activating or deactivating the device's primary function, hence a "disabled"
> + * device should be dormant, but still answer to commands and queries.
> + *
> + * A probed device may start in a disabled or enabled state, depending on the
> + * driver and hardware.
> + *
> + * @dev: the device to enable or disable.
> + * @val: the flag that tells the driver to either enable or disable the device.
> + * @return: 0 if OK, -ve on error
> + */


Nit: comment above suggest that you want to use kernel-doc format but
unfortunately it is not. It is easy to convert it to that.

I know that this the same style as is done in the whole file but there
is no reason to follow incorrect style if you know how to do it right
and ./script/kernel-doc helps you with validation.


> +int misc_set_enabled(struct udevice *dev, bool val);
> +
>  /*
>   * struct misc_ops - Driver model Misc operations
>   *
> @@ -109,6 +125,15 @@ struct misc_ops {
>  	 */
>  	int (*call)(struct udevice *dev, int msgid, void *tx_msg, int tx_size,
>  		    void *rx_msg, int rx_size);
> +	/*
> +	 * Enable or disable a device, optional.
> +	 *
> +	 * @dev: the device to enable.
> +	 * @val: the flag that tells the driver to either enable or disable the
> +	 *	 device.
> +	 * @return: 0 if OK, -ve on error
> +	 */

The same here.

> +	int (*set_enabled)(struct udevice *dev, bool val);
>  };
>  
>  #endif	/* _MISC_H_ */
> 


M

[U-Boot] [PATCH v2 1/2] misc: uclass: Add enable/disable function

[Simon Glass]

Hi Mario,

On 27 April 2018 at 06:52, Mario Six <mario.six@gdsys.cc> wrote:
> Add generic enable/disable function to the misc uclass.
>
> Signed-off-by: Mario Six <mario.six@gdsys.cc>
> ---
>
> v1 -> v2:
> * Merged the two functions into one function
> * Explained the semantics of enabling/disabling more throughly
>
> ---
>  drivers/misc/misc-uclass.c | 10 ++++++++++
>  include/misc.h             | 25 +++++++++++++++++++++++++
>  2 files changed, 35 insertions(+)
>
> diff --git a/drivers/misc/misc-uclass.c b/drivers/misc/misc-uclass.c
> index d9eea3dac5..0e3e0e8bf7 100644
> --- a/drivers/misc/misc-uclass.c
> +++ b/drivers/misc/misc-uclass.c
> @@ -56,6 +56,16 @@ int misc_call(struct udevice *dev, int msgid, void *tx_msg, int tx_size,
>         return ops->call(dev, msgid, tx_msg, tx_size, rx_msg, rx_size);
>  }
>
> +int misc_set_enabled(struct udevice *dev, bool val)
> +{
> +       const struct misc_ops *ops = device_get_ops(dev);
> +
> +       if (!ops->set_enabled)
> +               return -ENOSYS;
> +
> +       return ops->set_enabled(dev, val);
> +}
> +
>  UCLASS_DRIVER(misc) = {
>         .id             = UCLASS_MISC,
>         .name           = "misc",
> diff --git a/include/misc.h b/include/misc.h
> index 03ef55cdc8..04a4b65155 100644
> --- a/include/misc.h
> +++ b/include/misc.h
> @@ -58,6 +58,22 @@ int misc_ioctl(struct udevice *dev, unsigned long request, void *buf);
>  int misc_call(struct udevice *dev, int msgid, void *tx_msg, int tx_size,
>               void *rx_msg, int rx_size);
>
> +/*
> + * Enable or disable a device.
> + *
> + * The semantics of "disable" and "enable" should be understood here as
> + * activating or deactivating the device's primary function, hence a "disabled"
> + * device should be dormant, but still answer to commands and queries.
> + *
> + * A probed device may start in a disabled or enabled state, depending on the
> + * driver and hardware.

This makes be wonder whether we should have this as a concept
understood by the DM core. But perhaps not until we have more use
cases. So far I know of only display that needs this.

> + *
> + * @dev: the device to enable or disable.
> + * @val: the flag that tells the driver to either enable or disable the device.
> + * @return: 0 if OK, -ve on error
> + */
> +int misc_set_enabled(struct udevice *dev, bool val);
> +
>  /*
>   * struct misc_ops - Driver model Misc operations
>   *
> @@ -109,6 +125,15 @@ struct misc_ops {
>          */
>         int (*call)(struct udevice *dev, int msgid, void *tx_msg, int tx_size,
>                     void *rx_msg, int rx_size);
> +       /*
> +        * Enable or disable a device, optional.
> +        *
> +        * @dev: the device to enable.
> +        * @val: the flag that tells the driver to either enable or disable the
> +        *       device.
> +        * @return: 0 if OK, -ve on error

How about returning the old state (0 or 1)?

> +        */
> +       int (*set_enabled)(struct udevice *dev, bool val);
>  };
>
>  #endif /* _MISC_H_ */
> --
> 2.16.1
>

Regards,
Simon

[U-Boot] [PATCH v2 1/2] misc: uclass: Add enable/disable function

[Mario Six]

Hi Simon,

On Thu, May 3, 2018 at 9:02 PM, Simon Glass <sjg@chromium.org> wrote:
> Hi Mario,
>
> On 27 April 2018 at 06:52, Mario Six <mario.six@gdsys.cc> wrote:
>> Add generic enable/disable function to the misc uclass.
>>
>> Signed-off-by: Mario Six <mario.six@gdsys.cc>
>> ---
>>
>> v1 -> v2:
>> * Merged the two functions into one function
>> * Explained the semantics of enabling/disabling more throughly
>>
>> ---
>>  drivers/misc/misc-uclass.c | 10 ++++++++++
>>  include/misc.h             | 25 +++++++++++++++++++++++++
>>  2 files changed, 35 insertions(+)
>>
>> diff --git a/drivers/misc/misc-uclass.c b/drivers/misc/misc-uclass.c
>> index d9eea3dac5..0e3e0e8bf7 100644
>> --- a/drivers/misc/misc-uclass.c
>> +++ b/drivers/misc/misc-uclass.c
>> @@ -56,6 +56,16 @@ int misc_call(struct udevice *dev, int msgid, void *tx_msg, int tx_size,
>>         return ops->call(dev, msgid, tx_msg, tx_size, rx_msg, rx_size);
>>  }
>>
>> +int misc_set_enabled(struct udevice *dev, bool val)
>> +{
>> +       const struct misc_ops *ops = device_get_ops(dev);
>> +
>> +       if (!ops->set_enabled)
>> +               return -ENOSYS;
>> +
>> +       return ops->set_enabled(dev, val);
>> +}
>> +
>>  UCLASS_DRIVER(misc) = {
>>         .id             = UCLASS_MISC,
>>         .name           = "misc",
>> diff --git a/include/misc.h b/include/misc.h
>> index 03ef55cdc8..04a4b65155 100644
>> --- a/include/misc.h
>> +++ b/include/misc.h
>> @@ -58,6 +58,22 @@ int misc_ioctl(struct udevice *dev, unsigned long request, void *buf);
>>  int misc_call(struct udevice *dev, int msgid, void *tx_msg, int tx_size,
>>               void *rx_msg, int rx_size);
>>
>> +/*
>> + * Enable or disable a device.
>> + *
>> + * The semantics of "disable" and "enable" should be understood here as
>> + * activating or deactivating the device's primary function, hence a "disabled"
>> + * device should be dormant, but still answer to commands and queries.
>> + *
>> + * A probed device may start in a disabled or enabled state, depending on the
>> + * driver and hardware.
>
> This makes be wonder whether we should have this as a concept
> understood by the DM core. But perhaps not until we have more use
> cases. So far I know of only display that needs this.
>

The concept seems useful in general, true.

>> + *
>> + * @dev: the device to enable or disable.
>> + * @val: the flag that tells the driver to either enable or disable the device.
>> + * @return: 0 if OK, -ve on error
>> + */
>> +int misc_set_enabled(struct udevice *dev, bool val);
>> +
>>  /*
>>   * struct misc_ops - Driver model Misc operations
>>   *
>> @@ -109,6 +125,15 @@ struct misc_ops {
>>          */
>>         int (*call)(struct udevice *dev, int msgid, void *tx_msg, int tx_size,
>>                     void *rx_msg, int rx_size);
>> +       /*
>> +        * Enable or disable a device, optional.
>> +        *
>> +        * @dev: the device to enable.
>> +        * @val: the flag that tells the driver to either enable or disable the
>> +        *       device.
>> +        * @return: 0 if OK, -ve on error
>
> How about returning the old state (0 or 1)?
>

Good idea. Wouldn't it be good to have a distinct output parameter (e.g. bool
*old_state) though, so that we can still check whether the execution failed for
some reason?

>> +        */
>> +       int (*set_enabled)(struct udevice *dev, bool val);
>>  };
>>
>>  #endif /* _MISC_H_ */
>> --
>> 2.16.1
>>
>
> Regards,
> Simon
>

Best regards,
Mario

[U-Boot] [PATCH v2 1/2] misc: uclass: Add enable/disable function

[Simon Glass]

Hi Mario,

On 4 May 2018 at 03:01, Mario Six <mario.six@gdsys.cc> wrote:
> Hi Simon,
>
> On Thu, May 3, 2018 at 9:02 PM, Simon Glass <sjg@chromium.org> wrote:
>> Hi Mario,
>>
>> On 27 April 2018 at 06:52, Mario Six <mario.six@gdsys.cc> wrote:
>>> Add generic enable/disable function to the misc uclass.
>>>
>>> Signed-off-by: Mario Six <mario.six@gdsys.cc>
>>> ---
>>>
>>> v1 -> v2:
>>> * Merged the two functions into one function
>>> * Explained the semantics of enabling/disabling more throughly
>>>
>>> ---
>>>  drivers/misc/misc-uclass.c | 10 ++++++++++
>>>  include/misc.h             | 25 +++++++++++++++++++++++++
>>>  2 files changed, 35 insertions(+)
>>>
>>> diff --git a/drivers/misc/misc-uclass.c b/drivers/misc/misc-uclass.c
>>> index d9eea3dac5..0e3e0e8bf7 100644
>>> --- a/drivers/misc/misc-uclass.c
>>> +++ b/drivers/misc/misc-uclass.c
>>> @@ -56,6 +56,16 @@ int misc_call(struct udevice *dev, int msgid, void
*tx_msg, int tx_size,
>>>         return ops->call(dev, msgid, tx_msg, tx_size, rx_msg, rx_size);
>>>  }
>>>
>>> +int misc_set_enabled(struct udevice *dev, bool val)
>>> +{
>>> +       const struct misc_ops *ops = device_get_ops(dev);
>>> +
>>> +       if (!ops->set_enabled)
>>> +               return -ENOSYS;
>>> +
>>> +       return ops->set_enabled(dev, val);
>>> +}
>>> +
>>>  UCLASS_DRIVER(misc) = {
>>>         .id             = UCLASS_MISC,
>>>         .name           = "misc",
>>> diff --git a/include/misc.h b/include/misc.h
>>> index 03ef55cdc8..04a4b65155 100644
>>> --- a/include/misc.h
>>> +++ b/include/misc.h
>>> @@ -58,6 +58,22 @@ int misc_ioctl(struct udevice *dev, unsigned long
request, void *buf);
>>>  int misc_call(struct udevice *dev, int msgid, void *tx_msg, int
tx_size,
>>>               void *rx_msg, int rx_size);
>>>
>>> +/*
>>> + * Enable or disable a device.
>>> + *
>>> + * The semantics of "disable" and "enable" should be understood here as
>>> + * activating or deactivating the device's primary function, hence a
"disabled"
>>> + * device should be dormant, but still answer to commands and queries.
>>> + *
>>> + * A probed device may start in a disabled or enabled state, depending
on the
>>> + * driver and hardware.
>>
>> This makes be wonder whether we should have this as a concept
>> understood by the DM core. But perhaps not until we have more use
>> cases. So far I know of only display that needs this.
>>
>
> The concept seems useful in general, true.
>
>>> + *
>>> + * @dev: the device to enable or disable.
>>> + * @val: the flag that tells the driver to either enable or disable
the device.
>>> + * @return: 0 if OK, -ve on error
>>> + */
>>> +int misc_set_enabled(struct udevice *dev, bool val);
>>> +
>>>  /*
>>>   * struct misc_ops - Driver model Misc operations
>>>   *
>>> @@ -109,6 +125,15 @@ struct misc_ops {
>>>          */
>>>         int (*call)(struct udevice *dev, int msgid, void *tx_msg, int
tx_size,
>>>                     void *rx_msg, int rx_size);
>>> +       /*
>>> +        * Enable or disable a device, optional.
>>> +        *
>>> +        * @dev: the device to enable.
>>> +        * @val: the flag that tells the driver to either enable or
disable the
>>> +        *       device.
>>> +        * @return: 0 if OK, -ve on error
>>
>> How about returning the old state (0 or 1)?
>>
>
> Good idea. Wouldn't it be good to have a distinct output parameter (e.g.
bool
> *old_state) though, so that we can still check whether the execution
failed for
> some reason?

I think just returning an int would work:

< 0 : error
0 success, old value was false
1 success, old value was try

Regards,
Simon