lkml.org 
[lkml]   [2008]   [Jul]   [15]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRe: [PATCH] Add dynamic MMC-over-SPI-GPIO driver
Date
On Monday 14 July 2008 22:54:41 Andrew Morton wrote:
> > +static int gpiommc_probe(struct platform_device *pdev)
> > +{
> > + static int instance;
> > + struct gpiommc_device *d = platform_get_drvdata(pdev);
> > + struct spi_gpio_platform_data pdata;
> > + int err = -ENOMEM;
> > +
> > + d->spi_pdev = platform_device_alloc("spi-gpio", instance++);
> > + if (!d->spi_pdev)
> > + goto out;
>
> I guess that incrementing `instance' even if the allocation failed is
> somewhat wrong.

Well, I guess it doesn't matter much. The number is pretty random anyway.

> > +static struct gpiommc_device *gpiommc_alloc(struct platform_device *pdev,
> > + const char *name,
> > + const struct gpiommc_pins *pins,
> > + u8 mode)
> > +{
> > + struct gpiommc_device *d;
> > +
> > + d = kmalloc(sizeof(*d), GFP_KERNEL);
> > + if (!d)
> > + return NULL;
> > +
> > + strcpy(d->name, name);
>
> No check for overruns?

The caller checks the length, but it is a good idea to double-check here.
Good catch.

> > + memcpy(&d->pins, pins, sizeof(d->pins));
>
> If this had used the typesafe
>
> d->pins = *pins;
>
> I wouldn't have needed to run all around the place working out if
> overflow/underflow checks were needed here.

Yeah well, can use this.

> > +static ssize_t gpiommc_add_store(struct device_driver *drv,
> > + const char *buf, size_t count)
> > +{
> > + int res, err;
> > + char name[GPIOMMC_MAX_NAMELEN + 1];
> > + struct gpiommc_pins pins;
> > + unsigned int mode;
> > +
> > + res = sscanf(buf, "%" GPIOMMC_MAX_NAMELEN_STR "s %u,%u,%u,%u %u",
> > + name, &pins.gpio_di, &pins.gpio_do,
> > + &pins.gpio_clk, &pins.gpio_cs, &mode);
>
> What's going on here? So new kernel/userspace ABI.

The whole point of the module is to create a new userspace interface for
creating the device. The module does just glue several modules together
and create an actual device.

> Not documented in
> changelog, not documented in code comments, not documented in
> Documentation/ABI. This forces reviewers to reverse-engineer the
> interface design from the implementation and then attempt to review
> that design. Reviewers not happy!

Yeah well, as I said, I will do docs later. I didn't have any time
to write documentation, yet.
by. ;)

> > +static ssize_t gpiommc_remove_show(struct device_driver *drv,
> > + char *buf)
> > +{
> > + return snprintf(buf, PAGE_SIZE, "write device-name to remove the device\n");
> > +}
>
> Now that is one weird way in which to document the interface! What a
> waste of kernel text :(

Yeah, well. Better than nothing ;)
As I already said in the original patch announcement. This is by no way the
final version of the patch. Docs will be moved to Documentation/

--
Greetings Michael.


\
 
 \ /
  Last update: 2008-07-15 15:01    [W:0.100 / U:0.288 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site