Re: [Qemu-devel] [PATCH] hw/misc: Add code to emulate PFUZE3000 PMIC

[Andrey Smirnov <andrew.smirnov@gmail.com>]

On Fri, Dec 15, 2017 at 6:56 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 14 December 2017 at 15:19, Andrey Smirnov <andrew.smirnov@gmail.com> wrote:
>> Add trivial code to emulate PFUZE3000 PMIC.
>>
>> Cc: qemu-devel@nongnu.org
>> Cc: qemu-arm@nongnu.orgn
>> Cc: yurovsky@gmail.com
>> Cc: Peter Maydell <peter.maydell@linaro.org>
>> Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
>> ---
>>
>> Integrating this into a build system via "obj-y" might not be the best
>> way. Does this code need a dedicated CONFIG_ symbol?
>
> Yes, it ought to have a CONFIG_something symbol and be enabled
> via the whatever.mak for whatever guest architecture needs this
> device.
>
> Is there a board which needs this device? We usually
> only add devices together with whatever's using them.
>

It's a pretty popular PMIC used on majority on i.MX reference designs,
but I am not sure how many of those boards truly need it in QEMU. I
ended up having to implement this code for a custom i.MX7 board that
used one of PFUZE3000's output as a power supply for USB. I am not
sure if I'm ever going to submit patches for that mystery board
upstream. Looking at imx6qdl-sabresd.dtsi, in Linux source tree I
think this emulation code would also be needed for USB emulation on
i.MX6 SabreSD board, but I haven't verified it in practice.

Is this enough of a case to justify the patch's inclusion, or should I
go back and find a board QEMU supports that actually needs this
(either answer is perfectly fine with me)?

>> diff --git a/hw/misc/pfuze3000.c b/hw/misc/pfuze3000.c
>> new file mode 100644
>> index 0000000000..f414b7c0ba
>> --- /dev/null
>> +++ b/hw/misc/pfuze3000.c
>> @@ -0,0 +1,212 @@
>> +/*
>> + *
>> + * Copyright (c) 2017, Impinj, Inc.
>> + *
>> + * Author: Andrey Smirnov <andrew.smirnov@gmail.com>
>> + *
>> + * This program is free software; you can redistribute it and/or
>> + * modify it under the terms of the GNU General Public License as
>> + * published by the Free Software Foundation; either version 2 or
>> + * (at your option) version 3 of the License.
>
> The .h file is "v2 or later", but the .c file is "v2 or v3".
> Is that an intentional difference? Generally we go with "v2 or later".
>

Nope, just me not paying attention. Will change in v2.

>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> + * GNU General Public License for more details.
>> + *
>> + * You should have received a copy of the GNU General Public License along
>> + * with this program; if not, see <http://www.gnu.org/licenses/>.
>> + */
>
>> +static void pfuze3000_reset(DeviceState *ds)
>> +{
>> +    PFuze3000State *s = PFUZE3000(ds);
>> +
>> +    s->reg = PFUZE100_INVAL;
>
> This function needs to reset all the device state
> (all the fields that the guest can modify).
>

Good point, will change in v2.

Thanks,
Andrey Smirnov