Re: [PATCH v4 3/8] hw/misc/pca9552: Use the PCA9552_PIN_COUNT definition

[Cédric Le Goater]

On 6/22/20 10:37 AM, Philippe Mathieu-Daudé wrote:
> On 6/22/20 8:25 AM, Cédric Le Goater wrote:
>> On 6/21/20 12:58 AM, Philippe Mathieu-Daudé wrote:
>>> The current code models the PCA9552, but there are comments
>>> saying the code could be easily adapted for the rest of the
>>> PCA955x family.
>>> Since we assume we have at most 16 pins (for the PCA9552),
>>> add a definition and check the instance doesn't use more than
>>> this number. This makes the code a bit safer in case other
>>> PCA955x devices are added.
>>>
>>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>>
>> I would introduce a PCA9552Class and move nr_leds under the class.
> 
> A bit excessive (for now) for the hobbyist time I can dedicate
> to this, but I'll try (also renaming nr_leds -> nr_pins).


It's fine with me if you don't have time.  

Reviewed-by: Cédric Le Goater <clg@kaod.org>


> 
>>
>> C. 
>>
>>
>>> ---
>>>  hw/misc/pca9552.c | 12 ++++++++++++
>>>  1 file changed, 12 insertions(+)
>>>
>>> diff --git a/hw/misc/pca9552.c b/hw/misc/pca9552.c
>>> index cfefb8fce8..b97fc2893c 100644
>>> --- a/hw/misc/pca9552.c
>>> +++ b/hw/misc/pca9552.c
>>> @@ -303,6 +303,17 @@ static void pca9552_initfn(Object *obj)
>>>      }
>>>  }
>>>  
>>> +static void pca9552_realize(DeviceState *dev, Error **errp)
>>> +{
>>> +    PCA9552State *s = PCA9552(dev);
>>> +
>>> +    if (s->nr_leds > PCA9552_PIN_COUNT) {
>>> +        error_setg(errp, "%s invalid led count %u (max: %u)",
>>> +                   __func__, s->nr_leds, PCA9552_PIN_COUNT);
>>> +        return;
>>> +    }
>>> +}
>>> +
>>>  static void pca9552_class_init(ObjectClass *klass, void *data)
>>>  {
>>>      DeviceClass *dc = DEVICE_CLASS(klass);
>>> @@ -311,6 +322,7 @@ static void pca9552_class_init(ObjectClass *klass, void 
>>> *data)
>>>      k->event = pca9552_event;
>>>      k->recv = pca9552_recv;
>>>      k->send = pca9552_send;
>>> +    dc->realize = pca9552_realize;
>>>      dc->reset = pca9552_reset;
>>>      dc->vmsd = &pca9552_vmstate;
>>>  }
>>>
>>
>>