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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]


On Mon, Jul 30, 2012 at 09:10:48AM +0000, Aggrwal Poonam-B10812 wrote:
> 
> 
> > -----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.

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 majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


[Other Archives]     [Linux Kernel Newbies]     [Linux Driver Development]     [Linux Kbuild]     [Fedora Kernel]     [Linux Kernel Testers]     [Linux SH]     [Linux Omap]     [Linux Tape]     [Linux Input]     [Linux Kernel Janitors]     [Linux Kernel Packagers]     [Linux Doc]     [Linux Man Pages]     [Linux API]     [Linux Memory Management]     [Linux Modules]     [Linux Standards]     [Kernel Announce]     [Netdev]     [Git]     [Linux PCI]     Linux CAN Development     [Linux I2C]     [Linux RDMA]     [Linux NUMA]     [Netfilter]     [Netfilter Devel]     [SELinux]     [Bugtraq]     [FIO]     [Linux Perf Users]     [Linux Serial]     [Linux PPP]     [Linux ISDN]     [Linux Next]     [Kernel Stable Commits]     [Linux Tip Commits]     [Kernel MM Commits]     [Linux Security Module]     [AutoFS]     [Filesystem Development]     [Ext3 Filesystem]     [Linux bcache]     [Ext4 Filesystem]     [Linux BTRFS]     [Linux CEPH Filesystem]     [Linux XFS]     [XFS]     [Linux NFS]     [Linux CIFS]     [Ecryptfs]     [Linux NILFS]     [Linux Cachefs]     [Reiser FS]     [Initramfs]     [Linux FB Devel]     [Linux OpenGL]     [DRI Devel]     [Fastboot]     [Linux RT Users]     [Linux RT Stable]     [eCos]     [Corosync]     [Linux Clusters]     [LVS Devel]     [Hot Plug]     [Linux Virtualization]     [KVM]     [KVM PPC]     [KVM ia64]     [Linux Containers]     [Linux Hexagon]     [Linux Cgroups]     [Util Linux]     [Wireless]     [Linux Bluetooth]     [Bluez Devel]     [Ethernet Bridging]     [Embedded Linux]     [Barebox]     [Linux MMC]     [Linux IIO]     [Sparse]     [Smatch]     [Linux Arch]     [x86 Platform Driver]     [Linux ACPI]     [Linux IBM ACPI]     [LM Sensors]     [CPU Freq]     [Linux Power Management]     [Linmodems]     [Linux DCCP]     [Linux SCTP]     [ALSA Devel]     [Linux USB]     [Linux PA RISC]     [Linux Samsung SOC]     [MIPS Linux]     [IBM S/390 Linux]     [ARM Linux]     [ARM Kernel]     [ARM MSM]     [Tegra Devel]     [Sparc Linux]     [Linux Security]     [Linux Sound]     [Linux Media]     [Video 4 Linux]     [Linux IRDA Users]     [Linux for the blind]     [Linux RAID]     [Linux ATA RAID]     [Device Mapper]     [Linux SCSI]     [SCSI Target Devel]     [Linux SCSI Target Infrastructure]     [Linux IDE]     [Linux SMP]     [Linux AXP]     [Linux Alpha]     [Linux M68K]     [Linux ia64]     [Linux 8086]     [Linux x86_64]     [Linux Config]     [Linux Apps]     [Linux MSDOS]     [Linux X.25]     [Linux Crypto]     [DM Crypt]     [Linux Trace Users]     [Linux Btrace]     [Linux Watchdog]     [Utrace Devel]     [Linux C Programming]     [Linux Assembly]     [Dash]     [DWARVES]     [Hail Devel]     [Linux Kernel Debugger]     [Linux gcc]     [Gcc Help]     [X.Org]     [Wine]

Add to Google Powered by Linux

[Older Kernel Discussion]     [Yosemite National Park Forum]     [Large Format Photos]     [Gimp]     [Yosemite Photos]     [Stuff]