[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 at gdsys.cc> wrote:
> 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?

I think just returning an int would work:

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

Regards,
Simon