From: David Cohen <david.a.cohen@intel.com>
To: Grant Likely <grant.likely@secretlab.ca>
Cc: linux-kernel@vger.kernel.org, alan@linux.intel.com
Subject: Re: [PATCH 1/3] gpio-langwell: cleanup driver
Date: Thu, 20 Dec 2012 13:35:43 -0800 [thread overview]
Message-ID: <50D384AF.40002@intel.com> (raw)
In-Reply-To: <20121220011812.98B003E0AD7@localhost>
On 12/19/2012 05:18 PM, Grant Likely wrote:
> On Tue, 18 Dec 2012 17:52:11 -0800, David Cohen <david.a.cohen@intel.com> wrote:
>> This patch cleans up cosmetic issues, remove useless functions and add
>> to_lnw_priv() macro to replace many usages of container_of().
>>
>> Change-Id: I70a8fadd20a42493271d91633739bdddff19c8d8
>> Signed-off-by: David Cohen <david.a.cohen@intel.com>
> Hi David. Comments below...
Hi Grant,
Thanks for comments. Let me go through them.
>
>> ---
>> drivers/gpio/gpio-langwell.c | 64 ++++++++++++++----------------------------
>> 1 file changed, 21 insertions(+), 43 deletions(-)
>>
>> diff --git a/drivers/gpio/gpio-langwell.c b/drivers/gpio/gpio-langwell.c
>> index 202a992..8220c04 100644
>> --- a/drivers/gpio/gpio-langwell.c
>> +++ b/drivers/gpio/gpio-langwell.c
>> @@ -71,10 +71,12 @@ struct lnw_gpio {
>> struct irq_domain *domain;
>> };
>>
>> +#define to_lnw_priv(chip) container_of(chip, struct lnw_gpio, chip)
>> +
>> static void __iomem *gpio_reg(struct gpio_chip *chip, unsigned offset,
>> enum GPIO_REG reg_type)
>> {
>> - struct lnw_gpio *lnw = container_of(chip, struct lnw_gpio, chip);
>> + struct lnw_gpio *lnw = to_lnw_priv(chip);
>> unsigned nreg = chip->ngpio / 32;
>> u8 reg = offset / 32;
>> void __iomem *ptr;
>> @@ -86,7 +88,7 @@ static void __iomem *gpio_reg(struct gpio_chip *chip, unsigned offset,
>> static void __iomem *gpio_reg_2bit(struct gpio_chip *chip, unsigned offset,
>> enum GPIO_REG reg_type)
>> {
>> - struct lnw_gpio *lnw = container_of(chip, struct lnw_gpio, chip);
>> + struct lnw_gpio *lnw = to_lnw_priv(chip);
>> unsigned nreg = chip->ngpio / 32;
>> u8 reg = offset / 16;
>> void __iomem *ptr;
>> @@ -130,7 +132,7 @@ static void lnw_gpio_set(struct gpio_chip *chip, unsigned offset, int value)
>>
>> static int lnw_gpio_direction_input(struct gpio_chip *chip, unsigned offset)
>> {
>> - struct lnw_gpio *lnw = container_of(chip, struct lnw_gpio, chip);
>> + struct lnw_gpio *lnw = to_lnw_priv(chip);
>> void __iomem *gpdr = gpio_reg(chip, offset, GPDR);
>> u32 value;
>> unsigned long flags;
>> @@ -153,7 +155,7 @@ static int lnw_gpio_direction_input(struct gpio_chip *chip, unsigned offset)
>> static int lnw_gpio_direction_output(struct gpio_chip *chip,
>> unsigned offset, int value)
>> {
>> - struct lnw_gpio *lnw = container_of(chip, struct lnw_gpio, chip);
>> + struct lnw_gpio *lnw = to_lnw_priv(chip);
>> void __iomem *gpdr = gpio_reg(chip, offset, GPDR);
>> unsigned long flags;
>>
>> @@ -176,7 +178,7 @@ static int lnw_gpio_direction_output(struct gpio_chip *chip,
>>
>> static int lnw_gpio_to_irq(struct gpio_chip *chip, unsigned offset)
>> {
>> - struct lnw_gpio *lnw = container_of(chip, struct lnw_gpio, chip);
>> + struct lnw_gpio *lnw = to_lnw_priv(chip);
>> return irq_create_mapping(lnw->domain, offset);
>> }
> Nice cleanup above.
>
>>
>> @@ -215,19 +217,14 @@ static int lnw_irq_type(struct irq_data *d, unsigned type)
>> return 0;
>> }
>>
>> -static void lnw_irq_unmask(struct irq_data *d)
>> -{
>> -}
>> -
>> -static void lnw_irq_mask(struct irq_data *d)
>> -{
>> -}
>> +static void lnw_irq_noop(struct irq_data *d) {}
> Umm, this looks entirely wrong. There needs to be either mask/unmask or
> enable/disable ops for irq_chips. Yes I see that this patch is just
> consolidating two empty functions, but they are two empty functions that
> appear to be completely wrong.
I see your point. The solution does not belong to a clean up patch,
so I'll just remove it from here.
>
>>
>> static struct irq_chip lnw_irqchip = {
>> .name = "LNW-GPIO",
>> - .irq_mask = lnw_irq_mask,
>> - .irq_unmask = lnw_irq_unmask,
>> + .irq_mask = lnw_irq_noop,
>> + .irq_unmask = lnw_irq_noop,
>> .irq_set_type = lnw_irq_type,
>> + .irq_ack = lnw_irq_noop,
>> };
>>
>> static DEFINE_PCI_DEVICE_TABLE(lnw_gpio_ids) = { /* pin number */
>> @@ -299,17 +296,6 @@ static const struct irq_domain_ops lnw_gpio_irq_ops = {
>> .xlate = irq_domain_xlate_twocell,
>> };
>>
>> -#ifdef CONFIG_PM
>> -static int lnw_gpio_runtime_resume(struct device *dev)
>> -{
>> - return 0;
>> -}
>> -
>> -static int lnw_gpio_runtime_suspend(struct device *dev)
>> -{
>> - return 0;
>> -}
>> -
>> static int lnw_gpio_runtime_idle(struct device *dev)
>> {
>> int err = pm_schedule_suspend(dev, 500);
>> @@ -320,16 +306,8 @@ static int lnw_gpio_runtime_idle(struct device *dev)
>> return -EBUSY;
>> }
>>
>> -#else
>> -#define lnw_gpio_runtime_suspend NULL
>> -#define lnw_gpio_runtime_resume NULL
>> -#define lnw_gpio_runtime_idle NULL
>> -#endif
>> -
>> static const struct dev_pm_ops lnw_gpio_pm_ops = {
>> - .runtime_suspend = lnw_gpio_runtime_suspend,
>> - .runtime_resume = lnw_gpio_runtime_resume,
>> - .runtime_idle = lnw_gpio_runtime_idle,
>> + SET_RUNTIME_PM_OPS(NULL, NULL, lnw_gpio_runtime_idle)
>> };
> Also good.
>
>>
>> static int __devinit lnw_gpio_probe(struct pci_dev *pdev,
>> @@ -349,7 +327,7 @@ static int __devinit lnw_gpio_probe(struct pci_dev *pdev,
>> retval = pci_request_regions(pdev, "langwell_gpio");
>> if (retval) {
>> dev_err(&pdev->dev, "error requesting resources\n");
>> - goto err2;
>> + goto err;
> Renaming the labels just increases the noise in the diff. I prefer to
> see labels in the form "err-what-i-just-tried-to-do:" so they don't need
> to get reshuffled every time the code logic changes.
>
> There is no good reason for this change. Please drop it.
Maybe instead of drop it I'd prefer to fix the labels. They
are wrong currently.
Br, David
>
> g.
> .
>
next prev parent reply other threads:[~2012-12-20 21:35 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2012-12-19 1:52 [PATCH 0/3] gpio-langwell updates David Cohen
2012-12-19 1:52 ` [PATCH 1/3] gpio-langwell: cleanup driver David Cohen
2012-12-20 1:18 ` Grant Likely
2012-12-20 21:35 ` David Cohen [this message]
2012-12-20 21:56 ` Grant Likely
2012-12-19 1:52 ` [PATCH 2/3] gpio-langwell: update pci device table David Cohen
2012-12-20 1:20 ` Grant Likely
2012-12-19 1:52 ` [PATCH 3/3] gpio-langwell: implement irq shutdown interface David Cohen
2012-12-20 1:26 ` Grant Likely
2012-12-20 21:37 ` David Cohen
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=50D384AF.40002@intel.com \
--to=david.a.cohen@intel.com \
--cc=alan@linux.intel.com \
--cc=grant.likely@secretlab.ca \
--cc=linux-kernel@vger.kernel.org \
/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
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).