Re: [2/3][PATCH][v2] TDM Framework
On Wed, Aug 01, 2012 at 05:37:38AM -0700, Greg KH wrote: > On Wed, Aug 01, 2012 at 12:13:19PM +, Singh Sandeep-B37400 wrote: > > But running a complete voice stack itself is beyond the scope of Freescale. > > So vendors integrate their solutions with FSL solution. > And sorry, I was thinking you had kernel drivers that attached to this > framework, not userspace programs. Actually, what is the user/kernel > interface for this framework, I seem to have missed that entirely. You > will have to document that quite well, and run it by the linux-api > mailing list. This does also sound like we ought to be playing nicer with ALSA rather than just providing a binary only pipe... -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [2/3][PATCH][v2] TDM Framework
On Wed, Aug 01, 2012 at 12:13:19PM +, Singh Sandeep-B37400 wrote: > > On Mon, Jul 30, 2012 at 09:50:57AM +, Singh Sandeep-B37400 wrote: > > > 1. You should send some kernel mode TDM clients. Without those the > > framework > > >is pretty useless. > > > [Sandeep] We do have a test client but not good enough to be pushed in > > > open source, should we add it to documentation?? > > > > Then how do you know if the framework is "correct" and good enough for > > real clients? We don't add frameworks, or apis, to the kernel without > > users, so you will have to come up with some users before we can accept > > it. > We can only say that this framework is available in FSL BSPs and being used > by VoIP companies. > But running a complete voice stack itself is beyond the scope of Freescale. > So vendors integrate their solutions with FSL solution. > To test the framework we have a small application in our BSP (this is a very > basic test client) which tests the TDM driver and the SLIC interface from > voice transfer perspective. > We can get this added in the Linux codebase in some test directory. What > could be a good place for this? tools/ is a good place for that. And sorry, I was thinking you had kernel drivers that attached to this framework, not userspace programs. Actually, what is the user/kernel interface for this framework, I seem to have missed that entirely. You will have to document that quite well, and run it by the linux-api mailing list. thanks, greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [2/3][PATCH][v2] TDM Framework
> -Original Message- > From: Greg KH [mailto:g...@kroah.com] > Sent: Monday, July 30, 2012 9:32 PM > To: Singh Sandeep-B37400 > Cc: Francois Romieu; de...@driverdev.osuosl.org; linuxppc- > d...@lists.ozlabs.org; ga...@kernel.crashing.org; linux-arm- > ker...@lists.infradead.org; linux-kernel@vger.kernel.org > Subject: Re: [2/3][PATCH][v2] TDM Framework > > On Mon, Jul 30, 2012 at 09:50:57AM +, Singh Sandeep-B37400 wrote: > > 1. You should send some kernel mode TDM clients. Without those the > framework > >is pretty useless. > > [Sandeep] We do have a test client but not good enough to be pushed in > > open source, should we add it to documentation?? > > Then how do you know if the framework is "correct" and good enough for > real clients? We don't add frameworks, or apis, to the kernel without > users, so you will have to come up with some users before we can accept > it. We can only say that this framework is available in FSL BSPs and being used by VoIP companies. But running a complete voice stack itself is beyond the scope of Freescale. So vendors integrate their solutions with FSL solution. To test the framework we have a small application in our BSP (this is a very basic test client) which tests the TDM driver and the SLIC interface from voice transfer perspective. We can get this added in the Linux codebase in some test directory. What could be a good place for this? Regards Sandeep -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [2/3][PATCH][v2] TDM Framework
-Original Message- From: Mark Brown [mailto:broo...@opensource.wolfsonmicro.com] Sent: Monday, July 30, 2012 9:16 PM To: Francois Romieu Cc: Singh Sandeep-B37400; de...@driverdev.osuosl.org; linuxppc-...@lists.ozlabs.org; ga...@kernel.crashing.org; linux-arm-ker...@lists.infradead.org; linux-kernel@vger.kernel.org Subject: Re: [2/3][PATCH][v2] TDM Framework On Fri, Jul 27, 2012 at 05:25:42PM +0200, Francois Romieu wrote: > 2. It would probably make sense to Cc: netdev and serial. There may be >some kernel client network integration from the start. Plus audio, quite a few of the buses mentioned as examples of use cases for the hardware are audio ones. [Sandeep] Ok -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [2/3][PATCH][v2] TDM Framework
-Original Message- From: John Stoffel [mailto:j...@stoffel.org] Sent: Monday, July 30, 2012 7:40 PM To: Singh Sandeep-B37400 Cc: John Stoffel; linuxppc-...@lists.ozlabs.org; linux-arm-ker...@lists.infradead.org; ga...@kernel.crashing.org; linux-kernel@vger.kernel.org; de...@driverdev.osuosl.org Subject: RE: [2/3][PATCH][v2] TDM Framework >>>>> "Singh" == Singh Sandeep-B37400 writes: Singh> -Original Message- Singh> From: John Stoffel [mailto:j...@stoffel.org] Singh> Sent: 27 July 2012 19:42 Singh> To: Singh Sandeep-B37400 Singh> Cc: linuxppc-...@lists.ozlabs.org; Singh> linux-arm-ker...@lists.infradead.org; ga...@kernel.crashing.org; Singh> linux-kernel@vger.kernel.org; de...@driverdev.osuosl.org Singh> Subject: Re: [2/3][PATCH][v2] TDM Framework >> From: Sandeep Singh TDM Framework is an >> attempt to provide a platform independent layer which can offer a >> standard interface for TDM access to different client modules. Singh> Please don't use TLAs (Three Letter Acronyms) like TDM without explaining the clearly and up front. It makes it hard for anyone else who doens't know your code to look it over without having to spend lots of time poking around to figure it out from either context or somewhere else. Singh> [Sandeep] Patch for documentation for TDM is present in this Singh> patch set, which explains TDM in detail. Should we do this in Singh> commit message too?? Link too documentation patch: Singh> http://patchwork.ozlabs.org/patch/173680/ You should put the expansion of TDM into the initial commit message, and also into the Kconfig text, so that someone configuring the kernel has a clue what you're talking about. [Sandeep] Thanks for suggestion. Will take care. Try to approach this as a brandnew user who doesn't have your knowledge of the software. Write for the un-initiated if at all possible. John -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [2/3][PATCH][v2] TDM Framework
On Mon, Jul 30, 2012 at 09:10:48AM +, Aggrwal Poonam-B10812 wrote:
>
>
> > -Original Message-
> > From: Linuxppc-dev [mailto:linuxppc-dev-
> > bounces+poonam.aggrwal=freescale@lists.ozlabs.org] On Behalf Of Greg
> > KH
> > Sent: Friday, July 27, 2012 11:30 PM
> > To: Singh Sandeep-B37400
> > Cc: de...@driverdev.osuosl.org; linuxppc-...@lists.ozlabs.org; linux-arm-
> > ker...@lists.infradead.org; linux-kernel@vger.kernel.org
> > Subject: Re: [2/3][PATCH][v2] TDM Framework
> >
> > On Fri, Jul 27, 2012 at 07:35:38PM +0530, sand...@freescale.com wrote:
> > > +/* Data structures required for sysfs */ static struct tdm_sysfs attr
> > > += {
> > > + .attr.name = "use_latest_data",
> > > + .attr.mode = 0664,
> > > + .cmd_type = TDM_LATEST_DATA,
> > > +};
> >
> > What is this for?
> This knob is to control the behavior of the TDM framework with respect
> to handling the received TDM frames.
How will userspace know how to use this? Who will use this?
> The framework layer receives the TDM frames from the TDM adapter
> driver, de-interleaves the data and sends to respective client
> modules.
Why would userspace care about this then?
> In the case when the TDM client module has not consumed the data and
> emptied the Buffer, this flag decides whether to discard the
> un-fetched data, or discard the latest received data.
Again, why let userspace make this decision? How will it know to do
this or not?
Don't add options for things that don't need options.
> > > +int tdm_sysfs_init(void)
> > > +{
> > > + struct kobject *tdm_kobj;
> > > + int err = 1;
> > > + tdm_kobj = kzalloc(sizeof(*tdm_kobj), GFP_KERNEL);
> > > + if (tdm_kobj) {
> > > + kobject_init(tdm_kobj, &tdm_type);
> > > + if (kobject_add(tdm_kobj, NULL, "%s", "tdm")) {
> > > + pr_err("tdm: Sysfs creation failed\n");
> > > + kobject_put(tdm_kobj);
> > > + err = -EINVAL;
> > > + goto out;
> > > + }
> > > + } else {
> > > + pr_err("tdm: Unable to allocate tdm_kobj\n");
> > > + err = -ENOMEM;
> > > + goto out;
> > > + }
> > > +
> > > +out:
> > > + return err;
> > > +}
> >
> > You just leaked memory, what are you trying to do here?
> >
> > And why are you using "raw" kobjects? That's a sure sign that something
> > is really wrong.
> Will refer the documentation. Not very experienced on this stuff. Thanks for
> the comment.
> >
> > Your code doesn't look like it is tied into the driver model at all, why
> > not? What are you trying to do here?
> This is a framework layer, not associated to a particular device.
Not true, you have a parent pointer already, so you are hooked up to the
device tree.
> TDM adapter drivers will register to this framework.
Then you had better be part of the kernel driver model.
> We got this comment in internal freescale review list also.
Why did you ignore that feedback and make us ask the same thing?
> > Also, when creating new sysfs entries, like you are attempting to do here
> > (unsuccessfully I might add), you must create Documentation/ABI/ files as
> > well.
> Ok.
> >
> > And, to top it all off, you do realize you are asking us to do code
> > review in the middle of the merge window, when we are all busy doing
> > other things?
> Apologize for asking a review in the merge window time frame.
> Are there any guidelines when to send something for a review? We will
> be careful next time.
Anytime not in the merge window is usually good, also the week before
the merge window is usually busy as well.
greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [2/3][PATCH][v2] TDM Framework
On Mon, Jul 30, 2012 at 09:50:57AM +, Singh Sandeep-B37400 wrote: > 1. You should send some kernel mode TDM clients. Without those the framework >is pretty useless. > [Sandeep] We do have a test client but not good enough to be pushed in > open source, should we add it to documentation?? Then how do you know if the framework is "correct" and good enough for real clients? We don't add frameworks, or apis, to the kernel without users, so you will have to come up with some users before we can accept it. greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [2/3][PATCH][v2] TDM Framework
On Fri, Jul 27, 2012 at 05:25:42PM +0200, Francois Romieu wrote: > 2. It would probably make sense to Cc: netdev and serial. There may be >some kernel client network integration from the start. Plus audio, quite a few of the buses mentioned as examples of use cases for the hardware are audio ones. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [2/3][PATCH][v2] TDM Framework
>>>>> "Singh" == Singh Sandeep-B37400 writes: Singh> -Original Message- Singh> From: John Stoffel [mailto:j...@stoffel.org] Singh> Sent: 27 July 2012 19:42 Singh> To: Singh Sandeep-B37400 Singh> Cc: linuxppc-...@lists.ozlabs.org; linux-arm-ker...@lists.infradead.org; ga...@kernel.crashing.org; linux-kernel@vger.kernel.org; de...@driverdev.osuosl.org Singh> Subject: Re: [2/3][PATCH][v2] TDM Framework >> From: Sandeep Singh TDM Framework is an >> attempt to provide a platform independent layer which can offer a >> standard interface for TDM access to different client modules. Singh> Please don't use TLAs (Three Letter Acronyms) like TDM without explaining the clearly and up front. It makes it hard for anyone else who doens't know your code to look it over without having to spend lots of time poking around to figure it out from either context or somewhere else. Singh> [Sandeep] Patch for documentation for TDM is present in this Singh> patch set, which explains TDM in detail. Should we do this in Singh> commit message too?? Link too documentation patch: Singh> http://patchwork.ozlabs.org/patch/173680/ You should put the expansion of TDM into the initial commit message, and also into the Kconfig text, so that someone configuring the kernel has a clue what you're talking about. Try to approach this as a brandnew user who doesn't have your knowledge of the software. Write for the un-initiated if at all possible. John -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [2/3][PATCH][v2] TDM Framework
Thanks for your comments.
Please find my response inline.
Regards,
Sandeep
-Original Message-
From: Russell King - ARM Linux [mailto:li...@arm.linux.org.uk]
Sent: Friday, July 27, 2012 8:22 PM
To: Singh Sandeep-B37400
Cc: linuxppc-...@lists.ozlabs.org; linux-arm-ker...@lists.infradead.org;
de...@driverdev.osuosl.org; ga...@kernel.crashing.org;
linux-kernel@vger.kernel.org
Subject: Re: [2/3][PATCH][v2] TDM Framework
On Fri, Jul 27, 2012 at 07:35:38PM +0530, sand...@freescale.com wrote:
> +static DEFINE_MUTEX(tdm_core_lock);
> +static DEFINE_IDR(tdm_adapter_idr);
> +/* List of TDM adapters registered with TDM framework */
> +LIST_HEAD(adapter_list);
> +
> +/* List of TDM clients registered with TDM framework */
> +LIST_HEAD(driver_list);
These two are far too generic to be public. Have you checked your code with
sparse? I think not.
[Sandeep] Will changes the name to be more appropriate. Right, I haven't
checked with sparse.
> +
> +/*
> + * In case the previous data is not fetched by the client driver, the
> + * de-interleaving function will discard the old data and rewrite
> +the
> + * new data
> + */
> +
> +static int use_latest_tdm_data = 1;
> +
> +/* Data structures required for sysfs */ static struct tdm_sysfs attr
> += {
> + .attr.name = "use_latest_data",
> + .attr.mode = 0664,
> + .cmd_type = TDM_LATEST_DATA,
> +};
> +
> +static struct attribute *tdm_attr[] = {
> + &attr.attr,
> + NULL
> +};
> +
> +const struct sysfs_ops tdm_ops = {
> + .show = tdm_show_sysfs,
> + .store = tdm_store_sysfs,
> +};
Again, lack of static.
[Sandeep] Ok
> +
> +static struct kobj_type tdm_type = {
> + .sysfs_ops = &tdm_ops,
> + .default_attrs = tdm_attr,
> +};
> +
> +/* tries to match client driver with the adapter */ static int
> +tdm_device_match(struct tdm_driver *driver, struct tdm_adapter *adap)
> +{
> + /* match on an id table if there is one */
> + if (driver->id_table && driver->id_table->name[0]) {
> + if (!(strcmp(driver->id_table->name, adap->name)))
> + return (int)driver->id_table;
Casting a pointer to 'int' is not a good thing to do. Please fix this.
[Sandeep] Will fix this.
> + }
> + return 0;
> +}
> +
> +static int tdm_attach_driver_adap(struct tdm_driver *driver,
> + struct tdm_adapter *adap)
> +{
> + int ret = 0;
> + /* if driver is already attached to any other adapter, return*/
> + if (driver->adapter && (driver->adapter != adap))
Additional parens not required.
[Sandeep] Ok
> + return 0;
> +
> + driver->adapter = adap;
> +
> + if (driver->attach_adapter) {
> + ret = driver->attach_adapter(adap);
> + if (ret < 0) {
> + pr_err("tdm: attach_adapter failed for driver [%s]"
> + "err:%d\n", driver->name, ret);
> + return ret;
> + }
> + }
> + adap->drv_count++;
> +
> + if (!adap->tasklet_conf) {
> + tdm_sysfs_init();
> + tasklet_init(&adap->tdm_data_tasklet, tdm_data_tasklet_fn,
> + (unsigned long)adap);
Why not init this tasklet when the struct tdm_adapter is first created?
Why do you need to wait, and then have state tracking for this?
[Sandeep] Ok, will take care
> + adap->tasklet_conf = 1;
> + }
> +
> + return ret;
> +}
> +
> +/* Detach client driver and adapter */ static int
> +tdm_detach_driver_adap(struct tdm_driver *driver,
> + struct tdm_adapter *adap)
> +{
> + int res = 0;
> +
> + if (!driver->adapter || (driver->adapter != adap))
Additional parens not required.
[Sandeep] Ok.
> + return 0;
> +
> + adap->drv_count--;
> +
> + /* If no more driver is registed with the adapter*/
> + if (!adap->drv_count && adap->tasklet_conf) {
> + tasklet_disable(&adap->tdm_data_tasklet);
> + tasklet_kill(&adap->tdm_data_tasklet);
> + adap->tasklet_conf = 0;
> + }
> +
> + if (driver->detach_adapter) {
> + if (driver->detach_adapter(adap))
> + pr_err("tdm: detach_adapter failed for driver [%s]\n",
> + driver->name);
> + }
> +
> + driver->adapter = NULL;
> + return res;
> +}
> +
> +/* TDM adapter Registration/De-registration with TDM framework */
> +
> +static int tdm_reg
RE: [2/3][PATCH][v2] TDM Framework
Thanks for your comments. Please find replies inline. Regards, Sandeep -Original Message- From: Francois Romieu [mailto:rom...@fr.zoreil.com] Sent: 27 July 2012 20:56 To: Singh Sandeep-B37400 Cc: linuxppc-...@lists.ozlabs.org; linux-arm-ker...@lists.infradead.org; ga...@kernel.crashing.org; linux-kernel@vger.kernel.org; de...@driverdev.osuosl.org Subject: Re: [2/3][PATCH][v2] TDM Framework sand...@freescale.com : [...] > The main functions of this Framework are: > - provides interface to TDM clients to access TDM functionalities. > - provides standard interface for TDM drivers to hook with the framework. > - handles various data handling stuff and buffer management. > > In future this Framework will be extended to provide Interface for Line > control devices also. For example SLIC, E1/T1 Framers etc. > > Presently the framework supports only Single Port channelised mode. > Also the configurability options are limited which will be extended later on. > Only kernel mode TDM clients are supported currently. Support for User mode > clients will be added later. 1. You should send some kernel mode TDM clients. Without those the framework is pretty useless. [Sandeep] We do have a test client but not good enough to be pushed in open source, should we add it to documentation?? 2. It would probably make sense to Cc: netdev and serial. There may be some kernel client network integration from the start. [Sandeep] Ok. 3. Where is the userspace configuration interface ? [Sandeep] TDM framework right now supports only kernel mode clients. It has been tested with the client module that I mentioned above. Both the framework and test client are a part of Freescale BSP. [...] > Based on: git://git.am.freescale.net/gitolite/mirrors/galak-powerpc.git [Sandeep] Please try below mentioned link. The above one is Freescale's internal mirror of: git://git.kernel.org/pub/scm/linux/kernel/git/galak/powerpc.git $ git clone git://git.am.freescale.net/gitolite/mirrors/galak-powerpc.git Cloning into 'galak-powerpc'... fatal: Unable to look up git.am.freescale.net (port 9418) (No address associated with hostname) -- Ueimor -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [2/3][PATCH][v2] TDM Framework
-Original Message- From: John Stoffel [mailto:j...@stoffel.org] Sent: 27 July 2012 19:42 To: Singh Sandeep-B37400 Cc: linuxppc-...@lists.ozlabs.org; linux-arm-ker...@lists.infradead.org; ga...@kernel.crashing.org; linux-kernel@vger.kernel.org; de...@driverdev.osuosl.org Subject: Re: [2/3][PATCH][v2] TDM Framework > From: Sandeep Singh TDM Framework is an > attempt to provide a platform independent layer which can offer a > standard interface for TDM access to different client modules. Please don't use TLAs (Three Letter Acronyms) like TDM without explaining the clearly and up front. It makes it hard for anyone else who doens't know your code to look it over without having to spend lots of time poking around to figure it out from either context or somewhere else. [Sandeep] Patch for documentation for TDM is present in this patch set, which explains TDM in detail. Should we do this in commit message too?? Link too documentation patch: http://patchwork.ozlabs.org/patch/173680/ John -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [2/3][PATCH][v2] TDM Framework
> -Original Message-
> From: Linuxppc-dev [mailto:linuxppc-dev-
> bounces+poonam.aggrwal=freescale@lists.ozlabs.org] On Behalf Of Greg
> KH
> Sent: Friday, July 27, 2012 11:42 PM
> To: Singh Sandeep-B37400
> Cc: de...@driverdev.osuosl.org; linuxppc-...@lists.ozlabs.org; linux-arm-
> ker...@lists.infradead.org; linux-kernel@vger.kernel.org
> Subject: Re: [2/3][PATCH][v2] TDM Framework
>
> On Fri, Jul 27, 2012 at 07:35:38PM +0530, sand...@freescale.com wrote:
> > +static struct kobj_type tdm_type = {
> > + .sysfs_ops = &tdm_ops,
> > + .default_attrs = tdm_attr,
> > +};
>
> Ah, also, as per the documentation in the kernel (go look, it's there), I
> now get to publicly mock you for ignoring the error messages that the
> kernel is giving you when you try to shut down your code path.
>
> Well, to be fair, you are leaking memory like a sieve, so I doubt you
> ever saw those error messages because you never cleaned up after
> yourself, so perhaps I can forgive you, but your users can't, sorry.
> They like to rely on the fact that Linux is a reliable operating system,
> don't cause them to doubt that.
>
> Please fix this code, it's horribly broken. Read
> Documentation/kobject.txt for why. That file was written for a reason,
> and not just because we like writing documentation (hint, we hate to...)
To be honest we are not sysfs experts. Thanks for pointing to the documentation.
We will rework the stuff.
Regards
Poonam
>
> Ugh,
>
> greg k-h
> ___
> Linuxppc-dev mailing list
> linuxppc-...@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/linuxppc-dev
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
RE: [2/3][PATCH][v2] TDM Framework
> -Original Message-
> From: Linuxppc-dev [mailto:linuxppc-dev-
> bounces+poonam.aggrwal=freescale@lists.ozlabs.org] On Behalf Of Greg
> KH
> Sent: Friday, July 27, 2012 11:30 PM
> To: Singh Sandeep-B37400
> Cc: de...@driverdev.osuosl.org; linuxppc-...@lists.ozlabs.org; linux-arm-
> ker...@lists.infradead.org; linux-kernel@vger.kernel.org
> Subject: Re: [2/3][PATCH][v2] TDM Framework
>
> On Fri, Jul 27, 2012 at 07:35:38PM +0530, sand...@freescale.com wrote:
> > +/* Data structures required for sysfs */ static struct tdm_sysfs attr
> > += {
> > + .attr.name = "use_latest_data",
> > + .attr.mode = 0664,
> > + .cmd_type = TDM_LATEST_DATA,
> > +};
>
> What is this for?
This knob is to control the behavior of the TDM framework with respect to
handling the received TDM frames.
The framework layer receives the TDM frames from the TDM adapter driver,
de-interleaves the data and sends to respective client modules.
In the case when the TDM client module has not consumed the data and emptied
the Buffer, this flag decides whether to discard the un-fetched data, or
discard the latest received data.
>
> > +int tdm_sysfs_init(void)
> > +{
> > + struct kobject *tdm_kobj;
> > + int err = 1;
> > + tdm_kobj = kzalloc(sizeof(*tdm_kobj), GFP_KERNEL);
> > + if (tdm_kobj) {
> > + kobject_init(tdm_kobj, &tdm_type);
> > + if (kobject_add(tdm_kobj, NULL, "%s", "tdm")) {
> > + pr_err("tdm: Sysfs creation failed\n");
> > + kobject_put(tdm_kobj);
> > + err = -EINVAL;
> > + goto out;
> > + }
> > + } else {
> > + pr_err("tdm: Unable to allocate tdm_kobj\n");
> > + err = -ENOMEM;
> > + goto out;
> > + }
> > +
> > +out:
> > + return err;
> > +}
>
> You just leaked memory, what are you trying to do here?
>
> And why are you using "raw" kobjects? That's a sure sign that something
> is really wrong.
Will refer the documentation. Not very experienced on this stuff. Thanks for
the comment.
>
> Your code doesn't look like it is tied into the driver model at all, why
> not? What are you trying to do here?
This is a framework layer, not associated to a particular device. TDM adapter
drivers will register to this framework.
We got this comment in internal freescale review list also.
>
> Also, when creating new sysfs entries, like you are attempting to do here
> (unsuccessfully I might add), you must create Documentation/ABI/ files as
> well.
Ok.
>
> And, to top it all off, you do realize you are asking us to do code
> review in the middle of the merge window, when we are all busy doing
> other things?
Apologize for asking a review in the merge window time frame.
Are there any guidelines when to send something for a review? We will be
careful next time.
Regards
Poonam
>
> greg k-h
> ___
> Linuxppc-dev mailing list
> linuxppc-...@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/linuxppc-dev
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [2/3][PATCH][v2] TDM Framework
On Fri, Jul 27, 2012 at 07:35:38PM +0530, sand...@freescale.com wrote:
> +static struct kobj_type tdm_type = {
> + .sysfs_ops = &tdm_ops,
> + .default_attrs = tdm_attr,
> +};
Ah, also, as per the documentation in the kernel (go look, it's there),
I now get to publicly mock you for ignoring the error messages that
the kernel is giving you when you try to shut down your code path.
Well, to be fair, you are leaking memory like a sieve, so I doubt you
ever saw those error messages because you never cleaned up after
yourself, so perhaps I can forgive you, but your users can't, sorry.
They like to rely on the fact that Linux is a reliable operating system,
don't cause them to doubt that.
Please fix this code, it's horribly broken. Read
Documentation/kobject.txt for why. That file was written for a reason,
and not just because we like writing documentation (hint, we hate to...)
Ugh,
greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [2/3][PATCH][v2] TDM Framework
On Fri, Jul 27, 2012 at 07:35:38PM +0530, sand...@freescale.com wrote:
> +/* Data structures required for sysfs */
> +static struct tdm_sysfs attr = {
> + .attr.name = "use_latest_data",
> + .attr.mode = 0664,
> + .cmd_type = TDM_LATEST_DATA,
> +};
What is this for?
> +int tdm_sysfs_init(void)
> +{
> + struct kobject *tdm_kobj;
> + int err = 1;
> + tdm_kobj = kzalloc(sizeof(*tdm_kobj), GFP_KERNEL);
> + if (tdm_kobj) {
> + kobject_init(tdm_kobj, &tdm_type);
> + if (kobject_add(tdm_kobj, NULL, "%s", "tdm")) {
> + pr_err("tdm: Sysfs creation failed\n");
> + kobject_put(tdm_kobj);
> + err = -EINVAL;
> + goto out;
> + }
> + } else {
> + pr_err("tdm: Unable to allocate tdm_kobj\n");
> + err = -ENOMEM;
> + goto out;
> + }
> +
> +out:
> + return err;
> +}
You just leaked memory, what are you trying to do here?
And why are you using "raw" kobjects? That's a sure sign that something
is really wrong.
Your code doesn't look like it is tied into the driver model at all, why
not? What are you trying to do here?
Also, when creating new sysfs entries, like you are attempting to do
here (unsuccessfully I might add), you must create Documentation/ABI/
files as well.
And, to top it all off, you do realize you are asking us to do code
review in the middle of the merge window, when we are all busy doing
other things?
greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [2/3][PATCH][v2] TDM Framework
sand...@freescale.com : [...] > The main functions of this Framework are: > - provides interface to TDM clients to access TDM functionalities. > - provides standard interface for TDM drivers to hook with the framework. > - handles various data handling stuff and buffer management. > > In future this Framework will be extended to provide Interface for Line > control devices also. For example SLIC, E1/T1 Framers etc. > > Presently the framework supports only Single Port channelised mode. > Also the configurability options are limited which will be extended later on. > Only kernel mode TDM clients are supported currently. Support for User mode > clients will be added later. 1. You should send some kernel mode TDM clients. Without those the framework is pretty useless. 2. It would probably make sense to Cc: netdev and serial. There may be some kernel client network integration from the start. 3. Where is the userspace configuration interface ? [...] > Based on: git://git.am.freescale.net/gitolite/mirrors/galak-powerpc.git $ git clone git://git.am.freescale.net/gitolite/mirrors/galak-powerpc.git Cloning into 'galak-powerpc'... fatal: Unable to look up git.am.freescale.net (port 9418) (No address associated with hostname) -- Ueimor -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [2/3][PATCH][v2] TDM Framework
On Fri, Jul 27, 2012 at 07:35:38PM +0530, sand...@freescale.com wrote:
> +static DEFINE_MUTEX(tdm_core_lock);
> +static DEFINE_IDR(tdm_adapter_idr);
> +/* List of TDM adapters registered with TDM framework */
> +LIST_HEAD(adapter_list);
> +
> +/* List of TDM clients registered with TDM framework */
> +LIST_HEAD(driver_list);
These two are far too generic to be public. Have you checked your code
with sparse? I think not.
> +
> +/*
> + * In case the previous data is not fetched by the client driver, the
> + * de-interleaving function will discard the old data and rewrite the
> + * new data
> + */
> +
> +static int use_latest_tdm_data = 1;
> +
> +/* Data structures required for sysfs */
> +static struct tdm_sysfs attr = {
> + .attr.name = "use_latest_data",
> + .attr.mode = 0664,
> + .cmd_type = TDM_LATEST_DATA,
> +};
> +
> +static struct attribute *tdm_attr[] = {
> + &attr.attr,
> + NULL
> +};
> +
> +const struct sysfs_ops tdm_ops = {
> + .show = tdm_show_sysfs,
> + .store = tdm_store_sysfs,
> +};
Again, lack of static.
> +
> +static struct kobj_type tdm_type = {
> + .sysfs_ops = &tdm_ops,
> + .default_attrs = tdm_attr,
> +};
> +
> +/* tries to match client driver with the adapter */
> +static int tdm_device_match(struct tdm_driver *driver, struct tdm_adapter
> *adap)
> +{
> + /* match on an id table if there is one */
> + if (driver->id_table && driver->id_table->name[0]) {
> + if (!(strcmp(driver->id_table->name, adap->name)))
> + return (int)driver->id_table;
Casting a pointer to 'int' is not a good thing to do. Please fix this.
> + }
> + return 0;
> +}
> +
> +static int tdm_attach_driver_adap(struct tdm_driver *driver,
> + struct tdm_adapter *adap)
> +{
> + int ret = 0;
> + /* if driver is already attached to any other adapter, return*/
> + if (driver->adapter && (driver->adapter != adap))
Additional parens not required.
> + return 0;
> +
> + driver->adapter = adap;
> +
> + if (driver->attach_adapter) {
> + ret = driver->attach_adapter(adap);
> + if (ret < 0) {
> + pr_err("tdm: attach_adapter failed for driver [%s]"
> + "err:%d\n", driver->name, ret);
> + return ret;
> + }
> + }
> + adap->drv_count++;
> +
> + if (!adap->tasklet_conf) {
> + tdm_sysfs_init();
> + tasklet_init(&adap->tdm_data_tasklet, tdm_data_tasklet_fn,
> + (unsigned long)adap);
Why not init this tasklet when the struct tdm_adapter is first created?
Why do you need to wait, and then have state tracking for this?
> + adap->tasklet_conf = 1;
> + }
> +
> + return ret;
> +}
> +
> +/* Detach client driver and adapter */
> +static int tdm_detach_driver_adap(struct tdm_driver *driver,
> + struct tdm_adapter *adap)
> +{
> + int res = 0;
> +
> + if (!driver->adapter || (driver->adapter != adap))
Additional parens not required.
> + return 0;
> +
> + adap->drv_count--;
> +
> + /* If no more driver is registed with the adapter*/
> + if (!adap->drv_count && adap->tasklet_conf) {
> + tasklet_disable(&adap->tdm_data_tasklet);
> + tasklet_kill(&adap->tdm_data_tasklet);
> + adap->tasklet_conf = 0;
> + }
> +
> + if (driver->detach_adapter) {
> + if (driver->detach_adapter(adap))
> + pr_err("tdm: detach_adapter failed for driver [%s]\n",
> + driver->name);
> + }
> +
> + driver->adapter = NULL;
> + return res;
> +}
> +
> +/* TDM adapter Registration/De-registration with TDM framework */
> +
> +static int tdm_register_adapter(struct tdm_adapter *adap)
> +{
> + int res = 0;
> + struct tdm_driver *driver, *next;
> +
> + mutex_init(&adap->adap_lock);
> + INIT_LIST_HEAD(&adap->myports);
> + spin_lock_init(&adap->portlist_lock);
> +
> + adap->drv_count = 0;
> + adap->tasklet_conf = 0;
> +
> + list_add_tail(&adap->list, &adapter_list);
What protects this list?
> +
> + /* initialization of driver by framework in default configuration */
> + init_config_adapter(adap);
> +
> + /* Notify drivers */
> + pr_info("adapter [%s] registered\n", adap->name);
> + mutex_lock(&tdm_core_lock);
> + list_for_each_entry_safe(driver, next, &driver_list, list) {
> + if (tdm_device_match(driver, adap)) {
> + res = tdm_attach_driver_adap(driver, adap);
> + if (res == 0) {
> + pr_info("tdm: Driver(ID=%d) is "
> + "attached with Adapter %s(ID"
> + " = %d)\n", driver->id,
> + adap->name, adap->id);
> + } e
Re: [2/3][PATCH][v2] TDM Framework
> From: Sandeep Singh > TDM Framework is an attempt to provide a platform independent layer which can > offer a standard interface for TDM access to different client modules. Please don't use TLAs (Three Letter Acronyms) like TDM without explaining the clearly and up front. It makes it hard for anyone else who doens't know your code to look it over without having to spend lots of time poking around to figure it out from either context or somewhere else. John -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
