On Tue, Aug 03, 2010 at 07:48:48PM +0900, Masayuki Ohtake wrote: > Hi Greg KH, > > I have added my answer below. > Please find '<MASA>'. > > Thanks, Ohtake > > ----- Original Message ----- > From: "Greg KH" <[email protected]> > To: "Development for the MeeGo Project (discussion list)" > <[email protected]> > Cc: "Khor, Andrew Chih Howe" <[email protected]> > Sent: Tuesday, August 03, 2010 12:57 PM > Subject: Re: [MeeGo-dev] [MeeGo-Dev][PATCH] Topcliff: Update PCH_PHUB driver > to 2.6.35 > > > > On Tue, Aug 03, 2010 at 11:34:32AM +0800, Wang, Qi wrote: > > > Packet hub driver of Topcliff PCH > > > > > > Topcliff PCH is the platform controller hub that is going to be used in > > > Intel's upcoming general embedded platform. All IO peripherals in > > > Topcliff PCH are actually devices sitting on AMBA bus. Packet hub is > > > a special converter device in Topcliff PCH that translate AMBA > > > transactions > > > to PCI Express transactions and vice versa. Thus packet hub helps present > > > all IO peripherals in Topcliff PCH as PCIE devices to IA system. > > > Topcliff PCH has MAC address and Option ROM data. > > > These data are in SROM which is connected to PCIE bus. > > > Packet hub driver of Topcliff PCH can access MAC address and Option ROM > > > data in > > > SROM. > > > The driver creates a character device /dev/pch_phub. That device file > > > supports the following operations: > > > > > > read() :Read Option ROM data of SROM > > > write():Write Option ROM data of SROM > > > > Shouldn't that just be done with a standard sysfs binary file instead of > > a character device node? > <MASA>This driver also sets Phub hw configuration.
It does? From userspace? I didn't see that code, care to point it out? > Thus, phub driver should be integrated as character device. I don't understand why this needs to be a character device still. What is wrong with using sysfs? > > > ioctl():Read/Write MAC address of SROM > > > > What is the MAC address for? Why would you want to change it? Again, a > > sysfs file instead? > <MASA> The MAC address is used by GbE. Then why not use the network apis to set this like all other kernel drivers do? > > > > > + dev_dbg(&pdev->dev, "%s : " > > > > You forgot to terminate the line with a \n character. > <MASA> The above debug message is one sentence. Thus, we didn't add \n > character. Then you need to fix it as you just messed up the system log :) > > > +/** > > > + * pch_phub_write_gbe_mac_addr() - Write MAC address > > > + * @offset_address: Gigabit Ethernet MAC address offset value. > > > + * @data: Gigabit Ethernet MAC address value. > > > + */ > > > > Ah, it's a network MAC address. Then why not just allow the "standard" > > interface that Linux provides for changing the MAC address of a network > > device, and not worry about the MAC address here in the hub controller > > at all? > <MASA> Please refer above answer. I still do not understand. Please explain further. > > > +/* SROM ACCESS Macro */ > > > +#define PCH_WORD_ADDR_MASK (~((1 << 2) - 1)) > > > + > > > +/* Registers address offset */ > > > +#define PCH_PHUB_ID_REG 0x0000 > > > +#define PCH_PHUB_QUEUE_PRI_VAL_REG 0x0004 > > > +#define PCH_PHUB_RC_QUEUE_MAXSIZE_REG 0x0008 > > > +#define PCH_PHUB_BRI_QUEUE_MAXSIZE_REG 0x000C > > > +#define PCH_PHUB_COMP_RESP_TIMEOUT_REG 0x0010 > > > +#define PCH_PHUB_BUS_SLAVE_CONTROL_REG 0x0014 > > > +#define PCH_PHUB_DEADLOCK_AVOID_TYPE_REG 0x0018 > > > +#define PCH_PHUB_INTPIN_REG_WPERMIT_REG0 0x0020 > > > +#define PCH_PHUB_INTPIN_REG_WPERMIT_REG1 0x0024 > > > +#define PCH_PHUB_INTPIN_REG_WPERMIT_REG2 0x0028 > > > +#define PCH_PHUB_INTPIN_REG_WPERMIT_REG3 0x002C > > > +#define PCH_PHUB_INT_REDUCE_CONTROL_REG_BASE 0x0040 > > > +#define CLKCFG_REG_OFFSET 0x500 > > > + > > > +#define PCH_PHUB_OROM_SIZE 15360 > > > > All of these do not need to be in the .h file, right? Even if you were > > to keep the ioctl/read/write mess. > > > > Please just move this to sysfs and not use a character device. > <MASA> Please refer above answer. Again, you aren't using these from userspace, right? If you are, then they need to be individual sysfs files, not one big blob that you need to read/write from individual offsets. Please don't do that, it's not portable or supportable, or easily understood :) > > Also, why post this here and not on the linux-kernel mailing list? > <MASA>This patch have already accepted by linux kernel upstream. Great, fixes will be easier then :) Where was it posted and reviewed, I must have missed it. thanks, greg k-h _______________________________________________ MeeGo-dev mailing list [email protected] http://lists.meego.com/listinfo/meego-dev
