Re: [PATCH] can: janz-ican3: add support for CAL/CANopen firmware

[Marc Kleine-Budde <mkl@pengutronix.de>]

[-- Attachment #1: Type: text/plain, Size: 2915 bytes --]

On 05/02/2015 06:40 PM, Ira W. Snyder wrote:
> On Fri, May 01, 2015 at 10:21:57PM +0200, Marc Kleine-Budde wrote:
>> On 05/01/2015 08:31 PM, Andreas Gröger wrote:
>>> in our department we are using some older Janz ICAN3-modules in our
>>> dekstop pcs. There we have slightly different carrier boards than the
>>> janz-cmodio supported in the kernel sources, called CAN-PCI2 with two
>>> submodules. But the pci configuration regions are identical. So
>>> extending the supported pci devices to the corresponding device ids
>>> is sufficient to get the drivers working. Great.
>>>
>>> I have two minor issues:
>>> * The old ICAN3-modules with firmware 1.28 need more then 250ms for
>>> the restart after reset. I've increased the timeout to 500ms.
>>
>> Should be no problem, as it's a timeout.
>>
>>> * The janz_ican3 module uses the raw can services of the
>>> Janz-firmware, this means firmware must be ICANOS/2. Our
>>> ICAN3-modules are equipped with CAL/CANopen-firmware, so I must use
>>> the appropriate commands for the layer management services.
>>> My proposal: the driver detects the firmware after module reset and
>>> selects the commands matching the firmware. This affects the bus
>>> on/off-command (ican3_set_bus_state) and the configuration of the
>>> bittiming (ican3_set_bittiming). For better diagnostics the detected
>>
>> Is it possible to move the set_bittiming for both firmwares into the
>> ican3_set_bus_state() function? For various reasons I don't like the
>> fact that the bitrate is written into the hardware while the interface
>> is down.
>>
>>> firmware string is presented as sysfs attribute (fwinfo).
>>
>> Please document the new sysfs entry, see
>> Documentation/ABI/testing/sysfs-platform-at91 as an example. Please be
>> so kind and document the already existing termination attribute, too.
>>
>>> Currently my system works great. Are there any possibilities to get
>>> my changes into the linux kernel? Here are my changes based on a
>>> linux-3.19.6 vanilla kernel. Is this the right place for my request,
>>> is there any maintainer I can ask? I would be grateful for any help.
>>
>> Ira, can you have a look at the changes and give your Acked-by?
>>
> 
> Hi Andreas, Hi Marc,
> 
> The changes look great to me. I no longer work for Caltech, and so I
> don't have access to the hardware to test the changes. They are very
> self contained, and so are unlikely to cause any problems.
> 
> I have one more nitpick in addition to Marc's, which I added inline
> below.

Good catch.

> Great job on your first patch!

ACK.

regards,
Marc

-- 
Pengutronix e.K.                  | Marc Kleine-Budde           |
Industrial Linux Solutions        | Phone: +49-231-2826-924     |
Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 801 bytes --]