RE: [2/3][PATCH][v2] TDM Framework

[Aggrwal Poonam-B10812]

> -----Original Message-----
> From: Linuxppc-dev [mailto:linuxppc-dev-
> bounces+poonam.aggrwal=freescale.com@xxxxxxxxxxxxxxxx] On Behalf Of Greg
> KH
> Sent: Friday, July 27, 2012 11:30 PM
> To: Singh Sandeep-B37400
> Cc: devel@xxxxxxxxxxxxxxxxxxxx; linuxppc-dev@xxxxxxxxxxxxxxxx; linux-arm-
> kernel@xxxxxxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx
> Subject: Re: [2/3][PATCH][v2] TDM Framework
> 
> On Fri, Jul 27, 2012 at 07:35:38PM +0530, sandeep@xxxxxxxxxxxxx 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-dev@xxxxxxxxxxxxxxxx
> 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 majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/