Re: [PATCH] max3100 driver

[chri]

On Sat, Sep 20, 2008 at 10:24 AM, Andrew Morton
<akpm@xxxxxxxxxxxxxxxxxxxx> wrote:

>> +#define MAX3100_MAJOR 204
>
> Allocating a new major is a Big Deal.  It involves getting the major
> registered by contacting device@xxxxxxxxxxx
>
> It's better to dynamically allocate it - let udev handle it.
>

The MAX3100 is used in just small embedded systems where there is
usually no udev. I looked at other serial drivers but I haven't seen
none that uses dynamic allocation (they are share just 2 major
numbers). I followed the guide in devices.txt and asked for an
allocation of 4 minor numbers in the "Low-density serial ports".

>> +#include <linux/freezer.h>
>
> Gad. Are all those includes needed?
>

cleaned

>> +
>> +struct max3100_port_s {
>
> `struct max3100_port' is sufficient, and would be more typical.
>

done

>> +     struct uart_port port;
>> +     struct spi_device *spi;
>> +
>> +     int cts:1;              /* last CTS received for flow ctrl */
>> +     int tx_empty:1;         /* last TX empty bit */
>
> These two bits will share a word and hence locking is needed to prevent
> modifications to one from trashing modifications to the other on SMP.
>
> That's OK, but it would be best to document that locking right here, and
> to check that it is adhered to.
>

I reverted to using word aligned data.

>> +     /* and it's timer */
>
> s/it's/its/
>

done. I just found the useful flyspell-prog-mode so I will drastically
reduce English errors in the patches!

>> +             parity = parity ^ ((c>>i) & 1);
>
> hrmpf.  Don't we have a library function for that?
>
> Perhaps you could use hweight8(c)&1.
>

done

>> +     return parity;
>> +}
>> +
>> +static int max3100_check_parity(struct max3100_port_s *s, u16 c)
>> +{
>> +     return max3100_do_parity(s, c) ==
>> +       ((c >> (s->parity & MAX3100_7BIT ? 7 : 8)) & 1);
>
> <checks his C precedence table>
>
> OK...

done. Anyway I think that if precedence is unclear to me, it may be
unclear to others too; so I prefer to use a parenthesis too much than
one too few.


>> +     *c &=  ~MAX3100_PT;
>
> s/  / /
>

done

>
>> +}
>> +
>> +static void max3100_work(struct work_struct *w);
>> +
>> +static void max3100_dowork(struct max3100_port_s *s)
>> +{
>> +     if (!s->force_end_work && !work_pending(&s->work)) {
>> +             PREPARE_WORK(&s->work, max3100_work);
>
> This is a strange place to run PREPARE_WORK().  Normally it would be
> done during driver/device initialsiation, and I think that can be done
> here.
>

done, I checked the meaning of PREPARE_WORK in the header file.

>> +
>> +     etx = htons(tx);
>> +     spi_message_init(&message);
>> +     memset(&tran, 0, sizeof(tran));
>> +     tran.tx_buf = &etx;
>> +     tran.rx_buf = &erx;
>> +     tran.len = 2;
>
> could do
>
>        struct spi_transfer tran = {
>                .tx_buf = &etx,
>                .rx_buf = &erx
>                .len = 2,
>        };
>
> and the compiler will zero out the rest.  Minor point.
>

done

>> +
>> +     dev_dbg(&s->spi->dev, "%s\n", __func__);
>> +
>> +     /* may not be truely up-to-date */
>
> s/truely/truly/
>

done, next time flypsell-prog-mode will help me :-)

>> +
>> +     sprintf(b, "max3100-%d", s->minor);
>
> hm.  This will go nasty if s->minor > 9.  I'd suggest that b[] be made
> larger.
>

I limit the number of MAX3100s to 4, anyway I enlarged the buffer by
one so to be sure.

>> +     if (request_irq(s->irq, max3100_irq, irq_type, "max3100", s) < 0) {
>> +             dev_warn(&s->spi->dev, "cannot allocate irq %d\n", s->irq);
>> +             s->irq = 0;
>
> Shouldn't we tear down that workqueue before returning?
>

done

>> +static void max3100_release_port(struct uart_port *port)
>> +{
>> +     struct max3100_port_s *s = (struct max3100_port_s *)port;
>
> container_of()
>

done


Sorry again for not replying in firs time.


-- 
Christian Pellegrin, see http://www.evolware.org/chri/
"Real Programmers don't play tennis, or any other sport which requires
you to change clothes. Mountain climbing is OK, and Real Programmers
wear their climbing boots to work in case a mountain should suddenly
spring up in the middle of the computer room."
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/