All of lore.kernel.org
 help / color / mirror / Atom feed
From: Chanwoo Choi <cwchoi00@gmail.com>
To: Stephan Gerhold <stephan@gerhold.net>
Cc: Chanwoo Choi <cw00.choi@samsung.com>,
	MyungJoo Ham <myungjoo.ham@samsung.com>,
	Rob Herring <robh+dt@kernel.org>,
	devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
	Nikita Travkin <nikita@trvn.ru>,
	~postmarketos/upstreaming@lists.sr.ht
Subject: Re: [PATCH v3 2/3] extcon: sm5502: Refactor driver to use chip-specific struct
Date: Thu, 3 Jun 2021 00:30:45 +0900	[thread overview]
Message-ID: <83b00ca8-aa70-6c4b-5f9f-eebf571ee621@gmail.com> (raw)
In-Reply-To: <YLeh2Q1q3643iFkZ@gerhold.net>

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.

> 
> Thanks!
> Stephan
> 


-- 
Best Regards,
Samsung Electronics
Chanwoo Choi

  reply	other threads:[~2021-06-02 15:31 UTC|newest]

Thread overview: 11+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-06-01 20:00 [PATCH v3 0/3] extcon: sm5502: Add support for SM5504 Stephan Gerhold
2021-06-01 20:00 ` [PATCH v3 1/3] dt-bindings: extcon: sm5502: Document siliconmitus,sm5504-muic Stephan Gerhold
2021-06-02 19:37   ` Rob Herring
2021-06-01 20:00 ` [PATCH v3 2/3] extcon: sm5502: Refactor driver to use chip-specific struct Stephan Gerhold
2021-06-02 15:13   ` Chanwoo Choi
2021-06-02 15:20     ` Stephan Gerhold
2021-06-02 15:30       ` Chanwoo Choi [this message]
2021-06-02 15:35         ` Chanwoo Choi
2021-06-02 15:42           ` Stephan Gerhold
2021-06-03  3:05             ` Chanwoo Choi
2021-06-01 20:00 ` [PATCH v3 3/3] extcon: sm5502: Add support for SM5504 Stephan Gerhold

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=83b00ca8-aa70-6c4b-5f9f-eebf571ee621@gmail.com \
    --to=cwchoi00@gmail.com \
    --cc=cw00.choi@samsung.com \
    --cc=devicetree@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=myungjoo.ham@samsung.com \
    --cc=nikita@trvn.ru \
    --cc=robh+dt@kernel.org \
    --cc=stephan@gerhold.net \
    --cc=~postmarketos/upstreaming@lists.sr.ht \
    --subject='Re: [PATCH v3 2/3] extcon: sm5502: Refactor driver to use chip-specific struct' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.