Re: [PATCH v2] input/misc: new ADXL345/346 driver

[Mike Frysinger <vapier.adi-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>]

On Fri, Oct 16, 2009 at 03:22, Dmitry Torokhov wrote:
> On Fri, Oct 16, 2009 at 02:57:15AM -0400, Mike Frysinger wrote:
>> On Fri, Oct 16, 2009 at 01:39, Mike Frysinger wrote:
>> > On Fri, Oct 16, 2009 at 00:36, Dmitry Torokhov wrote:
>> >> On Wed, Oct 14, 2009 at 06:54:40AM -0400, Mike Frysinger wrote:
>> >>> +struct device;
>> >>> +struct adxl34x;
>> >>> +typedef int (adxl34x_read_t) (struct device *, unsigned char);
>> >>> +typedef int (adxl34x_read_block_t) (struct device *, unsigned char, int, unsigned char *);
>> >>> +typedef int (adxl34x_write_t) (struct device *, unsigned char, unsigned char);
>> >>> +
>> >>> +void adxl34x_disable(struct adxl34x *ac);
>> >>> +void adxl34x_enable(struct adxl34x *ac);
>> >>> +int adxl34x_probe(struct adxl34x **pac, struct device *dev, u16 bus_type,
>> >>> +     int irq, int fifo_delay_default, adxl34x_read_t read,
>> >>> +     adxl34x_read_block_t read_block, adxl34x_write_t write);
>> >>
>> >> Too many arguments... I think creating "struct adxl34x_ops" is called
>> >> for.
>> >
>> > guess i should do the same with the ad714x driver ?
>>
>> although looking at it, it'd only combine 4 args into 1 (so there'd be
>> 5 instead of 8).  i'd still have to pass the rest in as they're
>> instance-specific.  guess you still want the change though ?
>
> Yes, please. I still think it would be cleaner this way.

i converted the bus pieces to a struct then:
+struct adxl34x_bus_ops {
+	u16 bustype;
+	adxl34x_read_t *read;
+	adxl34x_read_block_t *read_block;
+	adxl34x_write_t *write;
+};

i'll leave the rest of your feedback to be addressed by Michael since
he has actual hardware to test with (and he wrote/understands the
driver)
-mike
_______________________________________________
Uclinux-dist-devel mailing list
Uclinux-dist-devel@blackfin.uclinux.org
https://blackfin.uclinux.org/mailman/listinfo/uclinux-dist-devel