O
> +/*
> + * This array represents the Battery Pack thermistor
> + * temarature and corresponding ADC value limits
> + */
> +static int
const
(as I pointed out in the very very first submission response)
> +
> +/* SFI table entries */
> +struct msic_batt_sfi_prop {
> + unsigned char sign[5];
> + unsigned int length;
> + unsigned char revision;
> + unsigned char checksum;
> + unsigned char oem_id[7];
> + unsigned char oem_tid[9];
> + struct battery_id batt_id;
> + unsigned short int voltage_max;
> + unsigned int capacity;
> + unsigned char battery_type;
> + char safe_temp_low_lim;
> + char safe_temp_up_lim;
> + unsigned short int safe_vol_low_lim;
> + unsigned short int safe_vol_up_lim;
> + unsigned short int chrg_cur_lim;
> + char chrg_term_lim;
> + unsigned short int term_cur;
> + char temp_mon_ranges;
> + struct temperature_monitoring_range temp_mon_range[4];
> + unsigned int sram_addr;
> +};
And you've still not explained this - as requested in the very first
submission response
> +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
> + uint8_t data;
> +
> + ret = intel_scu_ipc_ioread8(addr, &data);
> + if (ret) {
> + dev_warn(msic_dev, "%s:ipc read failed\n", __func__);
> + return ret;
> + }
> +
> + if (reset)
> + data &= (~val);
Brackets not needed btw
>
> +static unsigned int mdf_cal_avg(unsigned int avg)
> +{
> + unsigned int charge_now = 0;
> +
> + charge_now = msic_read_coloumb_ctr();
More pointless 0 assignments. They just help hide bugs later on so
please don't do that.
> +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
?
> +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;
> + address[1] = MSIC_BATT_CHR_CHRCTRL_ADDR;
> + address[2] = MSIC_BATT_CHR_CHRSTWDT_ADDR;
> +
> + data[0] = WDTWRITE_UNLOCK_VALUE; /* unlock WDT write */
> + data[1] = CHRCNTL_CHRG_DISABLE; /* Disable Charging */
> + data[2] = CHR_WDT_DISABLE; /* Disable WDT Timer */
Why initialise it to zero then fill it in the hard way - why not just
make it static const and pre-initialised ?
> +
> + /*
> + * Charger connect handler delayed work also modifies the
> + * MSIC charger parameter registers.To avoid concurrent
> + * read writes to same set of registers locking applied
> + */
> + mutex_lock(&mbi->ipc_rw_lock);
> + for (i = 1; i < 3; i++) {
> + retval = intel_scu_ipc_iowrite8(address[0], data[0]);
> + 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
> + /*
> + * 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 ?
> +
> + 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.
> +static int msic_charger_callback(void *arg, int event, struct
> otg_bc_cap *cap) +{
> + struct msic_power_module_info *mbi =
> + (struct msic_power_module_info *)arg;
> + int ret = 0;
> +
> + dev_info(msic_dev, "%s\n", __func__);
Just what we all want the log full of !
> + /* 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);
> + /* CHRINT Register */
> + irq_qinfo.qlist[idx].regs[2] = readb(mbi->msic_regs_iomap + 2);
> + /* CHRINT1 Register */
> + irq_qinfo.qlist[idx].regs[3] = readb(mbi->msic_regs_iomap + 3);
> +
> + /* 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 ?
> +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 ?
> + 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)
> +/**
> + * 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 ?
> + 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
> +static void init_charger_props(struct msic_power_module_info *mbi)
> +{
> + dev_info(msic_dev, "init charg props\n");
More random dev_info bits
> + 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;
> + address[2] = MSIC_BATT_CHR_CHRTTIME_ADDR;
> + address[3] = MSIC_BATT_CHR_SPCHARGER_ADDR;
> + address[4] = MSIC_BATT_CHR_CHRSTWDT_ADDR;
Same comment on initialisers as before
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 ?
Alan
_______________________________________________
Meego-kernel mailing list
[email protected]
http://lists.meego.com/listinfo/meego-kernel
