Hi Michael,
>
> /* Todo: test this */
> int max1363_single_channel_from_ring(long mask, struct max1363_state *st)
> {
> unsigned long numvals;
> int count = 0, ret;
> u8 *ring_data;
> if (!(st->current_mode->modemask & mask)) {
> ret = -EBUSY;
> goto error_ret;
> }
> numvals = hweight_long(st->current_mode->modemask);
>
> ring_data = kmalloc(numvals*2, GFP_KERNEL);
> if (ring_data == NULL) {
> ret = -ENOMEM;
> goto error_ret;
> }
> ret = st->indio_dev->ring->access.read_last(st->indio_dev->ring,
> ring_data);
> if (ret)
> goto error_free_ring_data;
> /* Need a count of channels prior to this one */
> mask >>= 1;
> while (mask) {
> if (mask && st->current_mode->modemask)
> count++;
> mask >>= 1;
> }
> if (st->chip_info->bits != 8)
> return ((int)(ring_data[count*2 + 0] & 0x0F) << 8)
> + (int)(ring_data[count*2 + 1]);
> else
> return ring_data[count];
>
> error_free_ring_data:
> kfree(ring_data);
> error_ret:
> return ret;
> }
>
> 1) Where does ring_data get freed?
Good point. i really should have looked at this more carefully.
I guess at somepoint I took that dynamic and clean forgot to change the exit path
appropriately.
> 2) if (mask && st->current_mode->modemask)? Shouldn't this be a Bitwise AND & ?
Yes.
>
>
> static int __devinit max1363_probe(struct i2c_client *client,
> const struct i2c_device_id *id)
> {
>
> [--snip--]
>
> /* Find the chip model specific data */
> for (i = 0; i < ARRAY_SIZE(max1363_chip_info_tbl); i++)
> if (!strcmp(max1363_chip_info_tbl[i].name, id->name)) {
> st->chip_info = &max1363_chip_info_tbl[i];
> break;
> };
>
> Isn't this what id->driver_data is for?
>
> st->chip_info = &max1363_chip_info_tbl[id->driver_data];
>
> In case you want to make this bullet prove - use distinct offsets.
Also a good point. Ouch that piece of stupidity has been
there a very long time. Might be worth squashing the name field
in that structure as well. Can easily get that from the id table.
> See example below.
>
> /* max1363 and max1368 tested - rest from data sheet */
> static const struct max1363_chip_info max1363_chip_info_tbl[] = {
> - {
> + [max1361] = {
> .name = "max1361",
> .num_inputs = 4,
> .bits = 10,
> .int_vref_mv = 2048,
> .monitor_mode = 1,
> .mode_list = max1363_mode_list,
> .num_modes = ARRAY_SIZE(max1363_mode_list),
> .default_mode = s0to3,
> .dev_attrs = &max1363_dev_attr_group,
> .scan_attrs = &max1363_scan_el_group,
> }, {
>
I'll put together a patch to fix these sometime in the next few days.
Thanks for taking a look at this driver. I fear it has previously sneaked
through as the least read of all the drivers (by merit of being complex
and rather dull!)
Jonathan
--
To unsubscribe from this list: send the line "unsubscribe linux-iio" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
