?? 2012??07??31?? 19:23, Arnd Bergmann д??:
>
> You posted the sdmmc host driver and the pci card reader driver.
> I assume that the USB card reader and the memstick host
> will also get posted at some point. Do you have a timeframe
> for those?
I will submit my memstick host driver around two months later, and
submit USB part at the end of this year.
>
> The hardware seems to also support xd/smartmedia. Do you have
> plans to add those? I think we have some code in drivers/mfd
> that acts as an abstraction layer for these, given that the
> host has to do the wear leveling there too.
Yes, xD is still in our plan. But because the user population of xD/SM
is so small, we put the priority of writing xD host driver in a relevant
low level.
Maybe we will submit this driver in the next year.
> As usual for most things getting added to drivers/misc, I'm skeptical
> about it actually fitting in here. Normally I'd put such a multiplexer
> into drivers/mfd. Given that you actually need your own bus, rather
> than just a single host with multiple endpoints, drivers/bus/realtek/cr
> might be best.
We do need a bus driver. We support multi models of card at the same
time, so we need a way to bind all of the host drivers. And in the
internal of our card reader, we have only one DMA engine and one ring
buffer to handle massive data, so we also need a way to protect the
critical area between different card hosts. Bus driver is convenient to
handle this situation. Another way to fulfill is to call every register
function of different host (like mmc, memstick) in sequence in card
reader driver, whether pci-based or usb-based, if not using bus driver.
I prefer the prior scheme, which is more flexible and less redundant code.
Using bus driver:
sdmmc memstick
\ /
\ /
\ /
rtsx bus driver
/ \
/ \
/ \
/ \
rtsx pci rtsx usb
Not using bus driver:
sdmmc-pci memstick-pci
\ /
\ /
\ /
\ /
rtsx pci
sdmmc-usb memstick-usb
\ /
\ /
\ /
\ /
rtsx usb
And you said we should put our own bus driver in drivers/bus/realtek/cr,
but where is drivers/bus? Or can I just put this bus driver and our
pci/usb card reader driver into drivers/mfd?
>> +udev rules
>> +----------
>> +
>> +In order to modprobe Realtek SD/MMC interface driver automatically, the following rule
>> +should be added to the udev rules file:
>> +
>> +SUBSYSTEM=="rtsx_cr", ENV{RTSX_CARD_TYPE}=="SD", RUN+="/sbin/modprobe -bv rtsx_sdmmc"
> This should not be necessary if you put the right module alias into the driver itself.
>
> You should generally use EXPORT_SYMBOL_GPL for new symbols unless there is
> a strong reason not to (and then you should explain that reason).
Got it, I will modify it.
>> +
>> +void rtsx_queue_work(struct work_struct *work)
>> +{
>> + queue_work(workqueue, work);
>> +}
>> +EXPORT_SYMBOL(rtsx_queue_work);
>> +
>> +void rtsx_queue_delayed_work(struct delayed_work *dwork, unsigned long delay)
>> +{
>> + queue_delayed_work(workqueue, dwork, delay);
>> +}
>> +EXPORT_SYMBOL(rtsx_queue_delayed_work);
> I would not bother with this, just move the workqueue into the client driver
> itself, which may result in a few duplicate lines but less code overall.
>
>> +int rtsx_register_driver(struct rtsx_driver *drv)
>> +{
>> + drv->driver.bus = &rtsx_bus_type;
>> +
>> + return driver_register(&drv->driver);
>> +}
>> +EXPORT_SYMBOL(rtsx_register_driver);
>> +
>> +void rtsx_unregister_driver(struct rtsx_driver *drv)
>> +{
>> + driver_unregister(&drv->driver);
>> +}
>> +EXPORT_SYMBOL(rtsx_unregister_driver);
> Same thing here: There will only be very few drivers for this bus, so it's
> easier to call the driver_register function from the drivers. You need to export
> the rtsx_bus_type in that case though, but that is done for most buses.
I will consider this.
> With this level of abstraction in your own driver, I wonder if it wouldn't be
> easier to have completely separate mmc drivers for each of the two host options
> (pci and usb). Apparently you have almost no shared code in the MMC driver
> /or/ in the bus driver.
If we support MMC only, writing separate drivers for pci and usb is more
proper and clear. But we try to support mmc and memstick, and maybe xD
later, it seems that having a bus driver is more convenient.
>
> From this, I think it would be easier to kill off your own bus driver entirely
> and move the rtsx_pci.c driver into drivers/mfd, and then merge your
> pci/sdmmc.c file with rtsx_sdmmc.c.
>
>> +#define wait_timeout_x(task_state, msecs) \
>> +do { \
>> + set_current_state((task_state)); \
>> + schedule_timeout(msecs_to_jiffies(msecs)); \
>> +} while (0)
>> +
>> +#define wait_timeout(msecs) wait_timeout_x(TASK_INTERRUPTIBLE, (msecs))
> This is the same as msleep_interruptible, right? Note that with interuptible
> sleep, you should always check if you got interrupted and return -ERESTARTSYS
> to user space.
Thank you for your explanation. It seems that I should call msleep
instead of msleep_interruptible.
Best regards,
wwang
On Wednesday 01 August 2012, wwang wrote:
> ?? 2012??07??31?? 19:23, Arnd Bergmann д??:
> >
> > You posted the sdmmc host driver and the pci card reader driver.
> > I assume that the USB card reader and the memstick host
> > will also get posted at some point. Do you have a timeframe
> > for those?
>
> I will submit my memstick host driver around two months later, and
> submit USB part at the end of this year.
ok.
> > The hardware seems to also support xd/smartmedia. Do you have
> > plans to add those? I think we have some code in drivers/mfd
> > that acts as an abstraction layer for these, given that the
> > host has to do the wear leveling there too.
>
> Yes, xD is still in our plan. But because the user population of xD/SM
> is so small, we put the priority of writing xD host driver in a relevant
> low level.
> Maybe we will submit this driver in the next year.
Ok. When you get to that, I think you should use the code
from drivers/mtd/nand/sm_common.c, but it's better to confirm
that with the MTD maintainers first.
> > As usual for most things getting added to drivers/misc, I'm skeptical
> > about it actually fitting in here. Normally I'd put such a multiplexer
> > into drivers/mfd. Given that you actually need your own bus, rather
> > than just a single host with multiple endpoints, drivers/bus/realtek/cr
> > might be best.
> We do need a bus driver. We support multi models of card at the same
> time, so we need a way to bind all of the host drivers. And in the
> internal of our card reader, we have only one DMA engine and one ring
> buffer to handle massive data, so we also need a way to protect the
> critical area between different card hosts. Bus driver is convenient to
> handle this situation. Another way to fulfill is to call every register
> function of different host (like mmc, memstick) in sequence in card
> reader driver, whether pci-based or usb-based, if not using bus driver.
> I prefer the prior scheme, which is more flexible and less redundant code.
I understand where you are coming from, but IMHO a bus driver would
make more sense if the bus was a low-level abstraction that allows you
to add new high-level drivers (memstick, smartmedia, ...) without
having to modify the low-level drivers, which I believe is not possible
with your current code.
> Using bus driver:
>
> sdmmc memstick
> \ /
> \ /
> \ /
> rtsx bus driver
> / \
> / \
> / \
> / \
> rtsx pci rtsx usb
In reality, what you have seems to be actually more like
sdmmc memstick
\ /
\ /
\ /
rtsx bus driver
/ \
/ \
/ \
/ \
rtsx-pci rtsx-usb
/ \ / \
pci-mmc \ usb-mmc \
pci-memstick usb-memstick
> Not using bus driver:
>
> sdmmc-pci memstick-pci
> \ /
> \ /
> \ /
> \ /
> rtsx pci
>
> sdmmc-usb memstick-usb
> \ /
> \ /
> \ /
> \ /
> rtsx usb
>
> And you said we should put our own bus driver in drivers/bus/realtek/cr,
> but where is drivers/bus? Or can I just put this bus driver and our
> pci/usb card reader driver into drivers/mfd?
The best driver abstractions have the most specific code as a top-level
module that calls into more generic code.
What I would suggest you do is to have the code that is common between
the USB and PCI drivers in a loadable module that both of the other
modules call into:
sdmmc-pci sdmmc-usb memstick-pci memstick-usb
\ \ / \ / \ / |
| \ / \ / \ / |
| \ / \___ / \ / |
\ sdmmc-common ___|____/ memstick-common |
\ | / | | /
\____|______ / |____________ _____|___/
| \ / \ / |
| pci-common usb-common |
\ \ / /
\ \ / /
\_____________ \ /____________/
\rtsx-common/
You can skip a few of these if they are not needed, but in principle
you should get the idea. In this example, the pci-common and the
usb-common drivers would each be MFD driver that export a bunch
of slave devices. All the mmc specific code that you currently
have in the pci driver would then go all the way to the top into
the sdmmc-pci driver, and anything that is shared goes into one
of the lower modules.
Arnd
?? 2012??08??01?? 22:31, Arnd Bergmann д??:
>
> I understand where you are coming from, but IMHO a bus driver would
> make more sense if the bus was a low-level abstraction that allows you
> to add new high-level drivers (memstick, smartmedia, ...) without
> having to modify the low-level drivers, which I believe is not possible
> with your current code.
>
> In reality, what you have seems to be actually more like
>
> sdmmc memstick
> \ /
> \ /
> \ /
> rtsx bus driver
> / \
> / \
> / \
> / \
> rtsx-pci rtsx-usb
> / \ / \
> pci-mmc \ usb-mmc \
> pci-memstick usb-memstick
>
> The best driver abstractions have the most specific code as a top-level
> module that calls into more generic code.
>
> What I would suggest you do is to have the code that is common between
> the USB and PCI drivers in a loadable module that both of the other
> modules call into:
>
>
> sdmmc-pci sdmmc-usb memstick-pci memstick-usb
> \ \ / \ / \ / |
> | \ / \ / \ / |
> | \ / \___ / \ / |
> \ sdmmc-common ___|____/ memstick-common |
> \ | / | | /
> \____|______ / |____________ _____|___/
> | \ / \ / |
> | pci-common usb-common |
> \ \ / /
> \ \ / /
> \_____________ \ /____________/
> \rtsx-common/
>
>
> You can skip a few of these if they are not needed, but in principle
> you should get the idea. In this example, the pci-common and the
> usb-common drivers would each be MFD driver that export a bunch
> of slave devices. All the mmc specific code that you currently
> have in the pci driver would then go all the way to the top into
> the sdmmc-pci driver, and anything that is shared goes into one
> of the lower modules.
>
> Arnd
Hi Arnd:
I got your ideas. Bus driver depending on other modules is indeed a bad
style.
In our situation, just take pci device for example, pci-common is the
place to detect card plugged or unplugged, so pci-common is required to
call and probe sdmmc-pci or memstick-pci. If not using bus driver, I
need to fulfill my own mechanism, like callback functions and other
internal data structures, to achieve this goal. But bus driver can
easily finish this job. I still prefer to retain bus driver, but detach
the adapter part from the original bus driver. So the bus driver will
not depend on other modules any more, and don't need any further
modification if adding new high-level drivers. I will also move all the
mmc specific code to sdmmc-pci driver. pci-common only contains the
generic code related to pci operations. Just like the below diagram:
sdmmc-pci sdmmc-usb memstick-pci memstick-usb
\ \ / \ / \ / |
| \ / \ / \ / |
| \ / \___ / \ | |
\ \ ____/ ___|____/ \_______| |
\ | / | | /
\____|______ / |____________ _____|___/
| \ / \ / |
| pci-common usb-common |
\ \ / /
\ \ / /
\_____________ \ /___________/
\rtsx-slot-bus/
And I plan to push sdmmc-pci and sdmmc-usb to drivers/mmc/host, push
memstick-pci and memstick-usb to drivers/memstick/host, and push
pci-common, usb-common and rtsx-slot-bus to drivers/mfd/realtek_cr.
I would like to receive your suggestions. Thank you.
Best regards,
wwang
On Friday 03 August 2012, wwang wrote:
> I got your ideas. Bus driver depending on other modules is indeed a bad
> style.
>
> In our situation, just take pci device for example, pci-common is the
> place to detect card plugged or unplugged, so pci-common is required to
> call and probe sdmmc-pci or memstick-pci. If not using bus driver, I
> need to fulfill my own mechanism, like callback functions and other
> internal data structures, to achieve this goal.
The MFD layer provides some helpers to create "platform" devices for the
child nodes. You can attach all the data you need for those in
the "platform_data" field, including callback pointers if necessary.
> But bus driver can
> easily finish this job. I still prefer to retain bus driver, but detach
> the adapter part from the original bus driver. So the bus driver will
> not depend on other modules any more, and don't need any further
> modification if adding new high-level drivers. I will also move all the
> mmc specific code to sdmmc-pci driver. pci-common only contains the
> generic code related to pci operations. Just like the below diagram:
>
> sdmmc-pci sdmmc-usb memstick-pci memstick-usb
> \ \ / \ / \ / |
> | \ / \ / \ / |
> | \ / \___ / \ | |
> \ \ ____/ ___|____/ \_______| |
> \ | / | | /
> \____|______ / |____________ _____|___/
> | \ / \ / |
> | pci-common usb-common |
> \ \ / /
> \ \ / /
> \_____________ \ /___________/
> \rtsx-slot-bus/
>
>
> And I plan to push sdmmc-pci and sdmmc-usb to drivers/mmc/host, push
> memstick-pci and memstick-usb to drivers/memstick/host, and push
> pci-common, usb-common and rtsx-slot-bus to drivers/mfd/realtek_cr.
>
> I would like to receive your suggestions. Thank you.
This looks ok, yes. I still don't see the value in having your own
bus_type though, and I believe using a platform device will save
you a significant chunk of code while achieving the same.
In the diagram above, the pci-common and the usb-common modules
each know exactly what their child devices are, so there is little
value creating an device-agnostic abstraction layer beneath it.
Having a module for common stuff in the place of your "rtsx-slot-bus"
is ok to handle stuff like your "ring buffer" that would be needed
by all four devices on the top. But when you introduce infrastructure
like your own bus_type, you should always consider whether that
infrastructure actually does something that is at the same time
common to all of your hardware and different from everything else.
I suspect you will find that your bus_type provide something
specific to realtek card readers and that you can just as well use
the platform bus.
Arnd
On Wed, 2012-08-01 at 14:31 +0000, Arnd Bergmann wrote:
> On Wednesday 01 August 2012, wwang wrote:
> > 于 2012年07月31日 19:23, Arnd Bergmann 写道:
> > >
> > > You posted the sdmmc host driver and the pci card reader driver.
> > > I assume that the USB card reader and the memstick host
> > > will also get posted at some point. Do you have a timeframe
> > > for those?
> >
> > I will submit my memstick host driver around two months later, and
> > submit USB part at the end of this year.
>
> ok.
>
> > > The hardware seems to also support xd/smartmedia. Do you have
> > > plans to add those? I think we have some code in drivers/mfd
> > > that acts as an abstraction layer for these, given that the
> > > host has to do the wear leveling there too.
> >
> > Yes, xD is still in our plan. But because the user population of xD/SM
> > is so small, we put the priority of writing xD host driver in a relevant
> > low level.
> > Maybe we will submit this driver in the next year.
>
> Ok. When you get to that, I think you should use the code
> from drivers/mtd/nand/sm_common.c, but it's better to confirm
> that with the MTD maintainers first.
Just note that this file doesn't have much stuff as it turned out
that xD card readers and cards are just raw NAND.
Except that large xD cards, still use small block size, and somewhat
different IDs, thus I placed them in this file.
You just need to write a NAND driver exposing raw flash, and my sm_ftl
will take care of wear leveling and everything else.
If any questions, bug-reports, just notes, I glad to hear about this.
Best regards,
Maxim Levitsky