From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752453AbbETIRP (ORCPT ); Wed, 20 May 2015 04:17:15 -0400 Received: from mail-pd0-f180.google.com ([209.85.192.180]:35518 "EHLO mail-pd0-f180.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750794AbbETIRK (ORCPT ); Wed, 20 May 2015 04:17:10 -0400 Date: Wed, 20 May 2015 13:46:52 +0530 From: Sudip Mukherjee To: Jean Delvare Cc: Greg KH , Dan Carpenter , One Thousand Gnomes , linux-kernel@vger.kernel.org Subject: Re: [PATCH v5 WIP 4/5] i2c-parport: use new parport device model Message-ID: <20150520081651.GA2997@sudip-PC> References: <1430907377-17147-1-git-send-email-sudipm.mukherjee@gmail.com> <1430907377-17147-4-git-send-email-sudipm.mukherjee@gmail.com> <20150520095724.286c21ef@endymion.delvare> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20150520095724.286c21ef@endymion.delvare> User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Wed, May 20, 2015 at 09:57:24AM +0200, Jean Delvare wrote: > Hi Sudip, > > On Wed, 6 May 2015 15:46:16 +0530, Sudip Mukherjee wrote: > > +static struct pardev_cb i2c_parport_cb = { > > + .flags = PARPORT_FLAG_EXCL, > > + .irq_func = i2c_parport_irq, > > +}; > > There's no reason for this variable to be global. It is only needed > temporarily at attach time if I understand correctly, so it should be > local to function i2c_parport_attach(). yes, this is only like a placeholder of the function pointers. will make it local in the patch. (I guess WIP is over) > > > + > > +static int i2c_parport_probe(struct pardevice *par_dev) > > +{ > > + if (strcmp(par_dev->name, "i2c-parport")) > > + return -ENODEV; > > + return 0; > > +} > > I'm wondering, is there any reason why this can't be automated by the > driver core part of the code? Most drivers will simply compare > drv->name with par_dev->name, which could be done in function > parport_probe() when no custom probe function is provided. yes, it can be done. I will add that in the patch. > > Now I see that you use the existence of the probe callback to decide > that the driver implements the device driver model. I suppose you could > use match_port instead, except that for some reason the paride driver > doesn't implement one. Maybe it should, or maybe you can check of the > presence of either to decide that this is a device model driver. as Dan suggested I am checking on devmodel now, instead of the existence of probe, so what you are suggestiong now can definitely be done with very small change. > > > + > > static struct parport_driver i2c_parport_driver = { > > .name = "i2c-parport", > > - .attach = i2c_parport_attach, > > + .match_port = i2c_parport_attach, > > .detach = i2c_parport_detach, > > + .probe = i2c_parport_probe, > > }; > > > > /* ----- Module loading, unloading and information ------------------------ */ > > Tested OK on my ADM1032 evaluation board. > > Tested-by: Jean Delvare Thanks again. regards sudip > > -- > Jean Delvare > SUSE L3 Support