Hi Alan,
Thanks for the comments.
My apologies, unfortunately I missed your first round of review comments.
Please find my responses below.
>> + * temarature and corresponding ADC value limits
>> + */
>> +static int
>
> const
>
> (as I pointed out in the very very first submission response)
Agreed, will fix it
>> +
>> +/* SFI table entries */
>> +struct msic_batt_sfi_prop {
>> + unsigned char sign[5];
>> + unsigned int length;
>
> And you've still not explained this - as requested in the very first
> submission response
This structure is to hold the SFI table entries which would be exposed by
Firmware.
>> +static int ipc_read_modify_reg(uint16_t addr, uint8_t val, int reset)
>> +{
>> + int ret = 0;
>
> Why set it to 0 when it is then set in the first call - yo are just
> hiding potential future errors
Agreed, will fix it
>> + }
>> +
>> + if (reset)
>> + data &= (~val);
>
> Brackets not needed btw
Agreed, will fix it
>> +static unsigned int mdf_cal_avg(unsigned int avg)
>> +{
>> + unsigned int charge_now = 0;
>
> More pointless 0 assignments. They just help hide bugs later on so
> please don't do that.
Agreed, will fix it
> +static void msic_log_exception_event(enum msic_event event)
> +{
> + switch (event) {
> + case MSIC_EVENT_BATTOCP_EXCPT:
> + printk(KERN_WARNING "msic-battery: over battery charge "
> + " current condition detected\n");
>
> dev_ - ?? you use the device for the default case so why not the others
Agreed, will change to dev_warn
>> +static int msic_batt_stop_charging(struct msic_power_module_info
>> *mbi) +{
>> + int retval, i;
>> + uint16_t address[3] = {0};
>> + unsigned char data[3] = {0};
>> +
>> + address[0] = MSIC_BATT_CHR_WDTWRITE_ADDR;
>>
>> + data[0] = WDTWRITE_UNLOCK_VALUE; /* unlock WDT write */
>
> Why initialise it to zero then fill it in the hard way - why not just
> make it static const and pre-initialised ?
Agreed, will fix it
>> + if (retval) {
>> + dev_warn(&mbi->pdev->dev, "%s(): ipc msic "
>> + "write failed\n", __func__);
>
> As I said the first time please don't split text like that - it makes
> it much harder to then search for
Agreed, will fix it
>> + /*
>> + * Charger disconnect handler also modifies the
>> + * MSIC charger parameter registers.To avoid concurrent
>> + * read writes to same set of registers locking applied
>> + */
>
> Would it make sense to have a routine that is given a table of values
> to write and just goes off and does it ?
disconnect handler is called from msic_chrg_callback function which in turn
called from USB OTG in Interrupt context.
>> +
>> + mutex_lock(&mbi->usb_chrg_lock);
>> + mbi->usb_chrg_props.charger_health = POWER_SUPPLY_HEALTH_GOOD;
>> + mbi->usb_chrg_props.charger_present = MSIC_USB_CHARGER_PRESENT;
>> + memcpy(mbi->usb_chrg_props.charger_model, "msic",
>
> What keeps this all in sync - you've got lots of locks but nothing I
> see that ensures that a parallel pair of events do anything more than
> keep a few writes together - so what stops the updates from being
> inconsistent.
Usb charger properties can be modified either from interrupt handler bottom half
Or from disconnect handler and sysfs read calls also read these properties
To keep all these events in sync I am using the lock.
>> +
>> + dev_info(msic_dev, "%s\n", __func__);
>
> Just what we all want the log full of !
Agreed, will fix it
>> + /* Copy Interrupt registers locally */
>> + /* PERSRCINT Register */
>> + irq_qinfo.qlist[idx].regs[0] = readb(mbi->msic_regs_iomap);
>> + /* PERSRCINT1 Register */
>> + irq_qinfo.qlist[idx].regs[1] = readb(mbi->msic_regs_iomap + 1);
>> + /* Add the Interrupt regs to Queue */
>> + list_add_tail(&irq_qinfo.qlist[idx].list, &mbi->irq_head);
>> + irq_qinfo.idx++;
>> + spin_unlock_irq(&mbi->irq_lock);
>
> As I asked the first time - can you not use a single readl for this and
> a threaded IRQ ?
Agreed, I will replace the readb's will single readl.
>> + tmp = list_first_entry(&mbi->irq_head, struct irq_list, list);
>> + /* We are only interested in Charger Interrupts */
>> + data[0] = tmp->regs[2];
>> + data[1] = tmp->regs[3];
>> + list_del(&tmp->list);
>> + irq_qinfo.idx--;
>> + spin_unlock(&mbi->irq_lock);
>
> So why store the rest, and given how it is used why not use a kfifo (as
> I asked originally)
Currently I am not handling PWRSRC Interrupts but I may handle
in the future.
I may use IPC read/write calls in the bottom half worker thread
And if I use threaded IRQ do I still need to maintain queue or kfifo?
>> +static void msic_battery_handle_intrpt(struct work_struct *work)
>> +{
>> + int err;
>> + uint32_t batt_charge_val = 0;
>> + unsigned char data[2] = {0};
>
> More unneed initialisers ?
Agreed, will fix it
>> +/**
>> + * sfi_table_populate - Simple Firmware Interface table Populate
>> + * @sfi_table: Simple Firmware Interface table structure
>> + *
>> + * SFI table has entries for the temperature limits
>> + * which is populated in a local structure
>
> Why is this here - surely it belongs in the firmware ?
Agree, currently firmware is support is not there will modify or remove when
Firmware support comes in.
>> + address = MSIC_BATT_CHR_SPWRSRCINT_ADDR;
>> + /* read specific to determine the status */
>> + retval = intel_scu_ipc_ioread8(address, &data);
>
> You killed most of them but you still seem to have gratuitious
> assignments and variables for address left over
Agree, will fix it
>> +static void init_charger_props(struct msic_power_module_info *mbi)
>> +{
>> + dev_info(msic_dev, "init charg props\n");
>
> More random dev_info bits
Agree will fix it
>> + uint16_t address[NR_ARR_ELM_MAX] = {0};
>> + unsigned char data[NR_ARR_ELM_MAX] = {0};
>> +
>> + address[0] = MSIC_BATT_CHR_PWRSRCLMT_ADDR;
>> + address[1] = MSIC_BATT_CHR_CHRCVOLTAGE_ADDR;
>
> Same comment on initialisers as before
Agree will fix it.
> We are heading in the right direction but there is still stuff here
> that was pointed out on the original review. I also don't understand
> how your locking is expected to keep the system as a whole consistent.
> In fact I suspect you want a single lock for the lot instead ?
I am using total five locks in this driver.
Mutex batt_lock to protect battery property variables like status,heath etc..
Mutex usb_chrg_lock to protect usb charger property variables like status,heath
etc..
Battery and charger properties can be read or modified independently
so separate lock for each.
Mutex ipc_rw_lock to have controlled access to HW registers wherever necessary.
Spinlock irq_lock to protect irq queue for queuing and de-queuing the
interrupts.
Spinlock batt_event to protect batt_event and charging_mode variables against
concurrent access
Please let me know you comments.
Thanks,
Ram
_______________________________________________
Meego-kernel mailing list
[email protected]
http://lists.meego.com/listinfo/meego-kernel
