Hi Sjur,
On Wed, Sep 19, 2012 at 1:08 PM, Sjur BRENDELAND
<sjur.brandel...@stericsson.com> wrote:
>> > include/linux/modem_shm/ste_modem.h | 71 ++++++
>>
>> Why did you decide to create a separate folder for this header ? if
>> it's STE specific, maybe use an 'ste' prefix for it too ?
>
> There has been some attempt to upstream a shm driver for another modem
> vendor as well, see https://lkml.org/lkml/2012/8/27/15.
>
> This driver used .../driver/modem_shm, and
> .../include/linux/modem_shm/. I feel that this driver belongs in the same
> family. This other driver did not include any vendor prefix though.
>
> What about .../include/linux/modem_shm/ste/modem.h or maybe just
> .../include/linux/modem_shm/modem.h?
Do your driver and Arun's share any code ? More importantly - is there
any common functionality to consolidate ?
If it's just the name "modem_shm" that is common, I'm not sure there's
merit in having a common folder ?
But if you do, why not use ste_modem_shm ? It looks like Arun comes
from STE too :)
>> > + /*
>> > + * STE-modem requires the firmware to be located
>> > + * at the start of the shared memory region. So we need to
>> > + * reserve space for firmware at the start.
>> > + * This cannot be done in the function sproc_load_segments because
>> > + * then dma_alloc_coherent is already called by Core and the
>> > + * start of the share memory area would already have been occupied.
>> > + */
>> > + if (!sproc->fw_addr) {
>> > + sproc->fw_addr = dma_alloc_coherent(rproc->dev.parent, fw-
>> >size,
>>
>> This doesn't look good: this function should just find an offset
>> within the firmware and return it, and not do any memory allocations.
>>
>> I understand the reason why you do that, ...
>
> I am afraid I *must* put the TOC at the start of memory. There is no way
> around this.
Sure that's fine. Let's just do it the right way :)
> But I can pre-allocate space for firmware and just bail out if
> it is not enough room. This is a much simpler approach.
>
>> but I think we had a nice
>> generic solution which shouldn't be too hard to implement (i.e. let
>> remoteproc maintain dedicated, purpose-specific, memory pools).
>> Moreover, if we implement it into the core, others could use it too.
>> Any chance you can look into it ? Ludovic started spinning some code
>> internally but was probably sucked away for other tasks meanwhile.
>
> I propose we pre-allocate some memory for now
Care to elaborate what do you mean exactly ? or just send a patch :)
>> > +static int __init sproc_init(void)
>> > +{
>> > + return platform_driver_register(&sproc_driver.drv);
>> > +}
>> > +module_init(sproc_init);
>> > +
>> > +static void __exit sproc_exit(void)
>> > +{
>> > + platform_driver_unregister(&sproc_driver.drv);
>> > +}
>> > +module_exit(sproc_exit);
>>
>> Replace boilerplate code with module_platform_driver ?
>
> I tried, but the macros cannot handle the sproc_driver.drv as argument.
I see, but I wonder why did you have to create the ste_modem_driver
struct in the first place ? How does the modem code access
sproc_kick_callback ?
Can you instead just ditch ste_modem_driver and provide
sproc_kick_callback in kick_subscribe or something ?
I'm asking not because using module_platform_driver is so important,
but because this may suggest something more importantly may need to be
fixed here.
Thanks,
Ohad.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
