Re: [PATCH 2/2] mfd: palmas add variant and OTP detection

[Ian Lartey <ian@slimlogic.co.uk>]

On 22/02/13 10:30, Laxman Dewangan wrote:
> On Friday 22 February 2013 06:22 AM, Ian Lartey wrote:
>> From: Graeme Gregory <gg@slimlogic.co.uk>
>>
>> Read the chip varient and the OTP information from the chip and display
>> this on probe to aid in debugging of issues.
>>
>
>> +    /* Read varient info from the device */
>> +    slave = PALMAS_BASE_TO_SLAVE(PALMAS_ID_BASE);
>> +    addr = PALMAS_BASE_TO_REG(PALMAS_ID_BASE, PALMAS_PRODUCT_ID_LSB);
>> +    ret = regmap_read(palmas->regmap[slave], addr, &reg);
>> +    if (ret < 0) {
>> +        dev_err(palmas->dev, "Unable to read ID err: %d\n", ret);
>> +        goto err;
>> +    }
>> +
>
> Can you please use the api palmas_* for reading register which I added
> recently. This will reduce the calc of slave and addr and done in single
> call?
> Same of following code also.

Will do.
>
>
> Also can we move all these new code to one function where we read id and
> diaplay. To keep probe() smaller.

Yes, I'll do that too.
>
>
>> +    palmas->id = reg;
>> +
>> +    slave = PALMAS_BASE_TO_SLAVE(PALMAS_ID_BASE);
>> +    addr = PALMAS_BASE_TO_REG(PALMAS_ID_BASE, PALMAS_PRODUCT_ID_MSB);
>> +    ret = regmap_read(palmas->regmap[slave], addr, &reg);
>> +    if (ret < 0) {
>> +        dev_err(palmas->dev, "Unable to read ID err: %d\n", ret);
>> +        goto err;
>> +    }
>> +
>> +    palmas->id |= reg << 8;
>
> Is it possible to change variable name id to product_id and then update
> to have more meaningful name?
> We can add for vendor_id also if require.

The rename of id to product_id is fine, if this what you mean by
"update to have a more meaningful name" then I agree.
Though if you wish for a additional change, I am not sure what the 
additional change is.

>
>> +
>> +    dev_info(palmas->dev, "Product ID %x\n", palmas->id);
>> +
>> +    slave = PALMAS_BASE_TO_SLAVE(PALMAS_DESIGNREV_BASE);
>> +    addr = PALMAS_BASE_TO_REG(PALMAS_DESIGNREV_BASE, PALMAS_DESIGNREV);
>> +    ret = regmap_read(palmas->regmap[slave], addr, &reg);
>> +    if (ret < 0) {
>> +        dev_err(palmas->dev, "Unable to read DESIGNREV err: %d\n", ret);
>> +        goto err;
>> +    }
>> +
>> +    palmas->designrev = reg & PALMAS_DESIGNREV_DESIGNREV_MASK;
>> +
>> +    dev_info(palmas->dev, "Product Design Rev %x\n", palmas->designrev);
>> +
>> +    slave = PALMAS_BASE_TO_SLAVE(PALMAS_PMU_CONTROL_BASE);
>> +    addr = PALMAS_BASE_TO_REG(PALMAS_PMU_CONTROL_BASE,
>> PALMAS_SW_REVISION);
>> +    ret = regmap_read(palmas->regmap[slave], addr, &reg);
>> +    if (ret < 0) {
>> +        dev_err(palmas->dev, "Unable to read SW_REVISION err: %d\n",
>> +                ret);
>> +        goto err;
>> +    }
>> +
>> +    palmas->sw_revision = reg;
>> +
>> +    dev_info(palmas->dev, "Product SW Rev %x\n", palmas->sw_revision);
>> +
>
>
> Here designrev also says the ES version and sw revision says the OTP SW
> revision.
> TI has make the design revision as ES1.0, ES2.0, ES2.1, ES2.2 like this
> and other technical names are 0xA0, 0xB0, 0xB1, 0xB2 etc.
>
> Probably I will add this in follow on patch becasue we have some errata
> which we need to implement based on the version number.

OK, I'll not make this change in favour of your later patch.
>
>
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/