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@xxxxxxxxxxx>
>>>>>> ---
>>>>>> 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
