Hi Greg KH,
I have added my answer below.
Please find '<MASA>'.
Thanks, Ohtake
----- Original Message -----
From: "Greg KH" <gre...@suse.de>
To: "Development for the MeeGo Project (discussion list)" <meego-dev@meego.com>
Cc: "Khor, Andrew Chih Howe" <andrew.chih.howe.k...@intel.com>
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.
Thus, phub driver should be integrated as character device.
>
> > 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.
>
> > + 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.
>
> > +/**
> > + * 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.
>
> > +/* 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.
>
> Also, why post this here and not on the linux-kernel mailing list?
<MASA>This patch have already accepted by linux kernel upstream.
>
> thanks,
>
> greg k-h
> _______________________________________________
> MeeGo-dev mailing list
> MeeGo-dev@meego.com
> http://lists.meego.com/listinfo/meego-dev
>
_______________________________________________
MeeGo-dev mailing list
MeeGo-dev@meego.com
http://lists.meego.com/listinfo/meego-dev
