Daniel Baluta schrieb am 29.06.2015 um 09:52:
> On Mon, Jun 29, 2015 at 2:07 AM, Hartmut Knaack <knaack.h@xxxxxx> wrote:
>> Daniel Baluta schrieb am 24.04.2015 um 17:58:
>>> Minimal implementation for MMC35240 3-axis magnetometer
>>> sensor. It provides processed readings and possiblity to change
>>> the sampling frequency.
>>>
>>
>> Hi Daniel,
>> please find a few issues inline, which you could address in a next
>> rework patch set. I would have sent a patch for the critical stuff,
>> but was obviously too stupid to find a data sheet :-(
>
> Well, there is no public datasheet. We are discussing with the vendor
> to make it public.
>
<...>
>>> +static int mmc35240_hw_set(struct mmc35240_data *data, bool set)
>>> +{
>>> + int ret;
>>> + u8 coil_bit;
>>> +
>>> + /*
>>> + * Recharge the capacitor at VCAP pin, requested to be issued
>>> + * before a SET/RESET command.
>>> + */
>>> + ret = regmap_update_bits(data->regmap, MMC35240_REG_CTRL0,
>>> + MMC35240_CTRL0_REFILL_BIT,
>>> + MMC35240_CTRL0_REFILL_BIT);
>>> + if (ret < 0)
>>> + return ret;
>>> + usleep_range(MMC35240_WAIT_CHARGE_PUMP, MMC35240_WAIT_CHARGE_PUMP + 1);
>>> +
>>> + if (set)
>>> + coil_bit = MMC35240_CTRL0_SET_BIT;
>>> + else
>>> + coil_bit = MMC35240_CTRL0_RESET_BIT;
>>> +
>>> + return regmap_update_bits(data->regmap, MMC35240_REG_CTRL0,
>>> + MMC35240_CTRL0_REFILL_BIT,
>>> + coil_bit);
>>
>> coil_bit is in any case outside the mask of MMC35240_CTRL0_REFILL_BIT.
>> Not sure about the whole intention, meaning in which state
>> MMC35240_CTRL0_REFILL_BIT is supposed to be (set) when either
>> MMC35240_CTRL0_SET_BIT or MMC35240_CTRL0_RESET_BIT is written.
>
> Yes, this is a bug. We have a patch prepared for it. Will send once Jonathan is
> ready to accept bugfixes. Together with this:
>
> http://marc.info/?l=linux-iio&m=143489464403101&w=2
>
Sending it out earlier gives people more time to review (or may allow more people
to review).
>
>>
>>> +}
<...>
>>> +
>>> +static int mmc35240_take_measurement(struct mmc35240_data *data)
>>> +{
>>> + int ret, tries = 100;
>>> + unsigned int reg_status;
>>> +
>>> + ret = regmap_write(data->regmap, MMC35240_REG_CTRL0,
>>> + MMC35240_CTRL0_TM_BIT);
>>> + if (ret < 0)
>>> + return ret;
>>> +
>>> + while (tries-- > 0) {
>>> + ret = regmap_read(data->regmap, MMC35240_REG_STATUS,
>>> + ®_status);
>>> + if (ret < 0)
>>> + return ret;
>>> + if (reg_status & MMC35240_STATUS_MEAS_DONE_BIT)
>>> + break;
>>
>> I would actually return 0 here, as measurement was successful. That
>> would mean that getting outside the loop is the error case and would
>> make the check obsolete.
>
> You are right, this could also work. Anyhow, I think code is easier to
> understand as it is. The check for (tries < 0) makes it very clear, that the
> data was not ready.
>
> Getting outside the loop in the error case is harder to understand at a first
> glance.
>
I can not really agree. The mission is accomplished at the break, so better
take the shortest way out (return 0 usually reflects that). Still going through
a check that won't trigger in this case just adds bloat without any benefit.
It's not a bug, so I don't feel too strong to fix it myself (still too much
reviews to be done). Sorry for annoying with such issues, spending my childhood
with slow and low memory 8 bit machines just left a mark ;-)
>>
>>> + msleep(20);
>>> + }
>>> +
>>> + if (tries < 0) {
>>> + dev_err(&data->client->dev, "data not ready\n");
>>> + return -EIO;
>>
>> Doesn't this qualify more for a timeout error, rather than I/O?
>
> Looking at the IIO drivers, most of them return EIO in such case.
> (grep for EIO in drivers/iio/light for example)
>
I don't feel too strong about this. I just regard I/O errors as indication
that communication with the device went wrong. But when getting here, it
always told to be busy.
> <snip>
>
>>> + case IIO_CHAN_INFO_SAMP_FREQ:
>>> + mutex_lock(&data->mutex);
>>> + ret = regmap_read(data->regmap, MMC35240_REG_CTRL1, ®);
>>> + mutex_unlock(&data->mutex);
>>> + if (ret < 0)
>>> + return ret;
>>> +
>>> + i = (reg & MMC35240_CTRL1_BW_MASK) >> MMC35240_CTRL1_BW_SHIFT;
>>> + if (i < 0 || i > ARRAY_SIZE(mmc35240_samp_freq))
>>
>> I would drop i and keep using reg. Has the nice side effect that you don't
>> need to check for negative values.
>
> Ok.
>
>
> <snip>
>
>>> +
>>> +static bool mmc35240_is_volatile_reg(struct device *dev, unsigned int reg)
>>> +{
>>> + switch (reg) {
>>> + case MMC35240_REG_CTRL0:
>>> + case MMC35240_REG_CTRL1:
>>
>> I would regard MMC35240_REG_ID as non-volatile, as well. Or is it because it
>> is just accessed once?
>
> Correct, we access it only once.
>
> Hartmut, thanks a lot for your reviews!
>
> thanks,
> Daniel.
> --
> 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
>
--
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
