Re: [PATCH v3 2/3] extcon: sm5502: Refactor driver to use chip-specific struct

[Chanwoo Choi <cw00.choi@samsung.com>]

On 6/3/21 12:42 AM, Stephan Gerhold wrote:
> On Thu, Jun 03, 2021 at 12:35:58AM +0900, Chanwoo Choi wrote:
>> On 21. 6. 3. 오전 12:30, Chanwoo Choi wrote:
>>> On 21. 6. 3. 오전 12:20, Stephan Gerhold wrote:
>>>> On Thu, Jun 03, 2021 at 12:13:18AM +0900, Chanwoo Choi wrote:
>>>>> On 21. 6. 2. 오전 5:00, Stephan Gerhold wrote:
>>>>>> Prepare for supporting SM5504 in the extcon-sm5502 driver by replacing
>>>>>> enum sm5504_types with a struct sm5504_type that stores the
>>>>>> chip-specific
>>>>>> definitions. This struct can then be defined separately for SM5504
>>>>>> without having to add if (type == TYPE_SM5504) everywhere in the code.
>>>>>>
>>>>>> Signed-off-by: Stephan Gerhold <stephan@gerhold.net>
>>>>>> ---
>>>>>> Changes in v3: New patch to simplify diff on next patch
>>>>>> ---
>>>>>>    drivers/extcon/extcon-sm5502.c | 64
>>>>>> +++++++++++++++++++++-------------
>>>>>>    drivers/extcon/extcon-sm5502.h |  4 ---
>>>>>>    2 files changed, 40 insertions(+), 28 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/extcon/extcon-sm5502.c
>>>>>> b/drivers/extcon/extcon-sm5502.c
>>>>>> index 9f40bb9f1f81..951f6ca4c479 100644
>>>>>> --- a/drivers/extcon/extcon-sm5502.c
>>>>>> +++ b/drivers/extcon/extcon-sm5502.c
>>>>>> @@ -40,17 +40,13 @@ struct sm5502_muic_info {
>>>>>>        struct i2c_client *i2c;
>>>>>>        struct regmap *regmap;
>>>>>> +    const struct sm5502_type *type;
>>>>>>        struct regmap_irq_chip_data *irq_data;
>>>>>> -    struct muic_irq *muic_irqs;
>>>>>> -    unsigned int num_muic_irqs;
>>>>>>        int irq;
>>>>>>        bool irq_attach;
>>>>>>        bool irq_detach;
>>>>>>        struct work_struct irq_work;
>>>>>> -    struct reg_data *reg_data;
>>>>>> -    unsigned int num_reg_data;
>>>>>> -
>>>>>>        struct mutex mutex;
>>>>>>        /*
>>>>>> @@ -62,6 +58,17 @@ struct sm5502_muic_info {
>>>>>>        struct delayed_work wq_detcable;
>>>>>>    };
>>>>>> +struct sm5502_type {
>>>>>> +    struct muic_irq *muic_irqs;
>>>>>> +    unsigned int num_muic_irqs;
>>>>>> +    const struct regmap_irq_chip *irq_chip;
>>>>>> +
>>>>>> +    struct reg_data *reg_data;
>>>>>> +    unsigned int num_reg_data;
>>>>>> +
>>>>>> +    int (*parse_irq)(struct sm5502_muic_info *info, int irq_type);
>>>>>> +};
>>>>>> +
>>>>>>    /* Default value of SM5502 register to bring up MUIC device. */
>>>>>>    static struct reg_data sm5502_reg_data[] = {
>>>>>>        {
>>>>>> @@ -502,11 +509,11 @@ static irqreturn_t
>>>>>> sm5502_muic_irq_handler(int irq, void *data)
>>>>>>        struct sm5502_muic_info *info = data;
>>>>>>        int i, irq_type = -1, ret;
>>>>>> -    for (i = 0; i < info->num_muic_irqs; i++)
>>>>>> -        if (irq == info->muic_irqs[i].virq)
>>>>>> -            irq_type = info->muic_irqs[i].irq;
>>>>>> +    for (i = 0; i < info->type->num_muic_irqs; i++)
>>>>>> +        if (irq == info->type->muic_irqs[i].virq)
>>>>>> +            irq_type = info->type->muic_irqs[i].irq;
>>>>>> -    ret = sm5502_parse_irq(info, irq_type);
>>>>>> +    ret = info->type->parse_irq(info, irq_type);
>>>>>
>>>>> Looks good to me. But there is only one comment.
>>>>> Need to check the 'parse_irq' as following:
>>>>>
>>>>> If you agree this suggestion, I'll apply with following changes
>>>>> by myself:
>>>>>
>>>>>     if (!info->type->parse_irq) {
>>>>>         dev_err(info->dev, "failed to handle irq due to parse_irq\n",
>>>>>         return IRQ_NONE;
>>>>>     }
>>>>>
>>>>>
>>>>
>>>> This condition should be impossible, since .parse_irq is set for both
>>>> SM5502 and SM5504:
>>>>
>>>> static const struct sm5502_type sm5502_data = {
>>>>     /* ... */
>>>>     .parse_irq = sm5502_parse_irq,
>>>> };
>>>>
>>>> static const struct sm5502_type sm5504_data = {
>>>>     /* ... */
>>>>     .parse_irq = sm5504_parse_irq,
>>>> };
>>>>
>>>> Which failure case are you trying to handle with that if statement?
>>>
>>> There is not failure case of this patchset. But, this refactoring
>>> suggestion has the potential problem without checking whether mandatory
>>> function pointer is NULL or not. When adding new chip by using this
>>> driver, the author might have the human error without parse_irq
>>> initialization even if the mandatory.
>>>
>>
>> Instead, it is better to check whether parser_irq is NULL or not
>> on probe function in order to reduce the unnecessary repetitive checking.
>>
> 
> Thanks for the explanation. This suggestion sounds better to me.
> (Although I consider it unlikely that someone would forget to define
>  .parse_irq when adding a new chip...)
> 
> Feel free to add something like the below when applying.
> Or let me know if I should re-send with this change:

Please resend them. Thanks.

> 
> diff --git a/drivers/extcon/extcon-sm5502.c b/drivers/extcon/extcon-sm5502.c
> index af44c1e2f368..93da2d8379b1 100644
> --- a/drivers/extcon/extcon-sm5502.c
> +++ b/drivers/extcon/extcon-sm5502.c
> @@ -694,6 +694,10 @@ static int sm5022_muic_i2c_probe(struct i2c_client *i2c)
>  	info->type = device_get_match_data(info->dev);
>  	if (!info->type)
>  		return -EINVAL;
> +	if (!info->type->parse_irq) {
> +		dev_err(info->dev, "parse_irq missing in struct sm5502_type\n");
> +		return -EINVAL;
> +	}
>  
>  	mutex_init(&info->mutex);
>  
> 
> Thanks for your review!
> Stephan
> 
> 


-- 
Best Regards,
Chanwoo Choi
Samsung Electronics