[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH] misc/pca9552: Add qom set and get
From: |
Cédric Le Goater |
Subject: |
Re: [PATCH] misc/pca9552: Add qom set and get |
Date: |
Tue, 19 Nov 2019 17:27:39 +0100 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.1.1 |
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)
>>>
>>
>>
>