Re: [PATCH] misc/pca9552: Add qom set and get

[Cédric Le Goater]

On 18/11/2019 18:48, Philippe Mathieu-Daudé wrote:
> Odd... I only received Cédric answer, not Joel patch.
> 
> On 11/18/19 11:00 AM, Cédric Le Goater wrote:
>> On 18/11/2019 07:17, Joel Stanley wrote:
>>> Following the pattern of the work recently done with the ASPEED GPIO
>>> model, this adds support for inspecting and modifying the PCA9552 LEDs
>>> from the monitor.
>>>
>>>   (qemu) qom-set  /machine/unattached/device[17] led0 on
>>>   (qemu) qom-get  /machine/unattached/device[17] led0
>>>   "on"
>>>
>>>   (qemu) qom-set  /machine/unattached/device[17] led0 off
>>>   (qemu) qom-get  /machine/unattached/device[17] led0
>>>   "off"
>>>
>>>   (qemu) qom-set  /machine/unattached/device[17] led0 pwm0
>>>   (qemu) qom-get  /machine/unattached/device[17] led0
>>>   "pwm0"
>>>
>>>   (qemu) qom-set  /machine/unattached/device[17] led0 pwm1
>>>   (qemu) qom-get  /machine/unattached/device[17] led0
>>>   "pwm1"
>>
>> It would be nice to revive the QOM get patchset from David.
>>
>>     http://patchwork.ozlabs.org/patch/666458/
>>
>> Did we reach some consensus ?
>>
>>> Signed-off-by: Joel Stanley <address@hidden>
>>
>> Some comments below.
>>
>>> ---
>>> The qom device in mainline qemu is a different path. Using the monitor
>>> examine `info qom-tree /machine/unattached/` to discover it.
>>>
>>> This can be tested with a Witherspoon image.
>>>
>>> $ wget 
>>> https://openpower.xyz/job/openbmc-build/distro=ubuntu,label=builder,target=witherspoon/lastSuccessfulBuild/artifact/deploy/images/witherspoon/obmc-phosphor-image-witherspoon.ubi.mtd
>>>
>>> $ qemu-system-arm -M witherspoon-bmc -serial pty -monitor pty -nographic \
>>>   -drive file=obmc-phosphor-image-witherspoon.ubi.mtd,format=raw,if=mtd
>>> char device redirected to /dev/pts/5 (label compat_monitor0)
>>> char device redirected to /dev/pts/10 (label serial0)
>>>
>>> $ screen /dev/pts/5
>>> QEMU 4.1.91 monitor - type 'help' for more information
>>> (qemu) qom-get  /machine/unattached/device[17] led0
>>> "off"
>>>
>>> $ screen /dev/pts/19
>>> root@witherspoon:~# cd /sys/class/gpio/
>>> root@witherspoon:/sys/class/gpio# echo 248 > export
>>> root@witherspoon:/sys/class/gpio# cat gpio248/value
>>> 0
>>>
>>> (qemu) qom-set  /machine/unattached/device[17] led0 on
>>>
>>> root@witherspoon:/sys/class/gpio# echo out > gpio248/direction
>>> root@witherspoon:/sys/class/gpio# cat gpio248/value
>>> 1
>>>
>>> (qemu) qom-get  /machine/unattached/device[17] led0
>>> "on"
>>>
>>> (qemu) qom-set  /machine/unattached/device[17] led0 off
>>> (qemu) qom-get  /machine/unattached/device[17] led0
>>> "off"
>>>
>>> root@witherspoon:/sys/class/gpio# cat gpio248/value
>>> 0
> 
> I am assuming this is 5.0 material.
> 
> This is a good starting point, and is useful.
> 
> However this would be useful for other boards/devices too.
> 
> What about defining a InterfaceInfo[] for external GPIOs?

The code below is very specific to pca9552.

What kind of interface do you have in mind more precisely ? 
which handlers ?

Thanks

C.

> Shouldn't we use the qemu_irq type for that then?
> 
>>>
>>> Signed-off-by: Joel Stanley <address@hidden>
>>> ---
>>>   hw/misc/pca9552.c | 91 +++++++++++++++++++++++++++++++++++++++++++++++
>>>   1 file changed, 91 insertions(+)
>>>
>>> diff --git a/hw/misc/pca9552.c b/hw/misc/pca9552.c
>>> index 73be28d9369c..0362aac8c862 100644
>>> --- a/hw/misc/pca9552.c
>>> +++ b/hw/misc/pca9552.c
>>> @@ -15,12 +15,16 @@
>>>   #include "hw/misc/pca9552.h"
>>>   #include "hw/misc/pca9552_regs.h"
>>>   #include "migration/vmstate.h"
>>> +#include "qapi/error.h"
>>> +#include "qapi/visitor.h"
>>>     #define PCA9552_LED_ON   0x0
>>>   #define PCA9552_LED_OFF  0x1
>>>   #define PCA9552_LED_PWM0 0x2
>>>   #define PCA9552_LED_PWM1 0x3
>>>   +static const char *led_state[] = {"on", "off", "pwm0", "pwm1"};
> 
> BTW "pwm0/pwm1" as "state" is confuse, but I understand this is how the 
> PCA9552 datasheet describes it.
> 
>>> +
>>>   static uint8_t pca9552_pin_get_config(PCA9552State *s, int pin)
>>>   {
>>>       uint8_t reg   = PCA9552_LS0 + (pin / 4);
>>> @@ -169,6 +173,84 @@ static int pca9552_event(I2CSlave *i2c, enum i2c_event 
>>> event)
>>>       return 0;
>>>   }
>>>   +static void pca9552_get_led(Object *obj, Visitor *v, const char *name,
>>> +                            void *opaque, Error **errp)
>>> +{
>>> +    PCA9552State *s = PCA9552(obj);
>>> +    int led, rc, reg;
>>> +    char *str;
>>> +    uint8_t state;
>>> +
>>> +    rc = sscanf(name, "led%2d", &led);
>>> +    if (rc != 1) {
>>> +        error_setg(errp, "%s: error reading %s", __func__, name);
>>> +        return;
>>> +    }
>>> +    if (led < 0 || led > s->nr_leds) {
>>> +        error_setg(errp, "%s invalid led %s", __func__, name);
>>> +        return;
>>> +    }
>>> +    /*
>>> +     * Get the LSx register as the qom interface should expose the device
>>> +     * state, not the modeled 'input line' behaviour which would come from
>>> +     * reading the INPUTx reg
>>> +     */
>>> +    reg = PCA9552_LS0 + led / 4;
>>> +    state = (pca9552_read(s, reg) >> (led % 8)) & 0x3;
>>
>> Could we add accessors to extract the register fields and to clarify
>> the layout ?
>>
>>> +    str = g_strdup(led_state[state]);
>>> +    visit_type_str(v, name, &str, errp);
>>> +}
>>> +
>>> +/*
>>> + * Return an LED selector register value based on an existing one, with
>>> + * the appropriate 2-bit state value set for the given LED number (0-3).
>>> + */
>>> +static inline uint8_t pca955x_ledsel(uint8_t oldval, int led_num, int 
>>> state)
>>> +{
>>> +        return (oldval & (~(0x3 << (led_num << 1)))) |
>>> +                ((state & 0x3) << (led_num << 1));
>>> +}
>>> +
>>> +static void pca9552_set_led(Object *obj, Visitor *v, const char *name,
>>> +                            void *opaque, Error **errp)
>>> +{
>>> +    PCA9552State *s = PCA9552(obj);
>>> +    Error *local_err = NULL;
>>> +    int led, rc, reg, val;
>>> +    uint8_t state;
>>> +    char *state_str;
>>> +
>>> +    visit_type_str(v, name, &state_str, &local_err);
>>> +    if (local_err) {
>>> +        error_propagate(errp, local_err);
>>> +        return;
>>> +    }
>>> +    rc = sscanf(name, "led%2d", &led);
>>> +    if (rc != 1) {
>>> +        error_setg(errp, "%s: error reading %s", __func__, name);
>>> +        return;
>>> +    }
>>> +    if (led < 0 || led > s->nr_leds) {
>>> +        error_setg(errp, "%s invalid led %s", __func__, name);
>>> +        return;
>>> +    }
>>> +
>>> +    for (state = 0; state < ARRAY_SIZE(led_state); state++) {
>>> +        if (!strcmp(state_str, led_state[state])) {
>>> +            break;
>>> +        }
>>> +    }
>>> +    if (state >= ARRAY_SIZE(led_state)) {
>>> +        error_setg(errp, "%s invalid led state %s", __func__, state_str);
>>> +        return;
>>> +    }
>>> +
>>> +    reg = PCA9552_LS0 + led / 4;
>>> +    val = pca9552_read(s, reg);
>>> +    val = pca955x_ledsel(val, led % 4, state);
>>> +    pca9552_write(s, reg, val);
>>> +}
>>> +
>>>   static const VMStateDescription pca9552_vmstate = {
>>>       .name = "PCA9552",
>>>       .version_id = 0,
>>> @@ -204,6 +286,7 @@ static void pca9552_reset(DeviceState *dev)
>>>   static void pca9552_initfn(Object *obj)
>>>   {
>>>       PCA9552State *s = PCA9552(obj);
>>> +    int led;
>>>         /* If support for the other PCA955X devices are implemented, these
>>>        * constant values might be part of class structure describing the
>>> @@ -211,6 +294,14 @@ static void pca9552_initfn(Object *obj)
>>>        */
>>>       s->max_reg = PCA9552_LS3;
>>>       s->nr_leds = 16;
>>> +
>>> +    for (led = 0; led < s->nr_leds; led++) {
>>> +        char *name;
>>> +
>>> +        name = g_strdup_printf("led%d", led);
>>> +        object_property_add(obj, name, "bool", pca9552_get_led, 
>>> pca9552_set_led,
>>> +                            NULL, NULL, NULL);
>>
>> It misses a :
>>
>>     g_free(name)
>>
>> C.
>>
>>> +    }
>>>   }
>>>     static void pca9552_class_init(ObjectClass *klass, void *data)
>>>
>>
>>
>