[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 at chromium.org> wrote:
> Hi Mario,
>
> On 27 April 2018 at 06:52, Mario Six <mario.six at gdsys.cc> wrote:
>> Add generic enable/disable function to the misc uclass.
>>
>> Signed-off-by: Mario Six <mario.six at 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