Re: [PATCH v6 4/6] mfd: ahc1ec0: Add support for Advantech embedded controller

[Lee Jones]

On Wed, 24 Mar 2021, Campion.Kang wrote:

> Thanks a lot, please see below descriptions.
> 
> >-----Original Message-----
> >From: Lee Jones <lee.jones@xxxxxxxxxx>
> >Sent: Friday, March 19, 2021 10:15 PM
> >To: Campion.Kang <Campion.Kang@xxxxxxxxxxxxxxxx>
> >Cc: chia-lin.kao@xxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx;
> >jdelvare@xxxxxxxx; linux-hwmon@xxxxxxxxxxxxxxx;
> >linux-kernel@xxxxxxxxxxxxxxx; linux-watchdog@xxxxxxxxxxxxxxx;
> >linux@xxxxxxxxxxxx; robh+dt@xxxxxxxxxx; wim@xxxxxxxxxxxxxxxxxx;
> >Campion.Kang@xxxxxxxxx
> >Subject: Re: [PATCH v6 4/6] mfd: ahc1ec0: Add support for Advantech
> >embedded controller
> >
> >On Fri, 19 Mar 2021, Campion Kang wrote:
> >
> >>
> >> Please check [Campion] text in below as my reply.
> >
> >This is a mess.  Please setup your mailer to quote correctly.
> >
> >> Sorry, due to the mail was rejected by vger.kernel.org as SPAM before
> >> so I reply the mail late and had some test email before.
> >>
> >> -----------------------------------------------------------------------------------------
> >> Date:   Tue, 9 Mar 2021 16:07:55 +0000
> >> From:   Lee Jones <lee.jones@xxxxxxxxxx>
> >
> >[...]
> >
> >> > +enum {
> >> > +	ADVEC_SUBDEV_BRIGHTNESS = 0,
> >> > +	ADVEC_SUBDEV_EEPROM,
> >> > +	ADVEC_SUBDEV_GPIO,
> >> > +	ADVEC_SUBDEV_HWMON,
> >> > +	ADVEC_SUBDEV_LED,
> >> > +	ADVEC_SUBDEV_WDT,
> >> > +	ADVEC_SUBDEV_MAX,
> >> > +};
> >>
> >> Are these arbitrary?
> >> [Campion] cannot arbitrary there, due to it is a index to identify its number
> >of sub device.
> >
> >I'm asking what this is dictated by.
> >
> >Are these ID/index values written into H/W?
> >
> 
> These index written in BIOS ACPI table. 

Please consider renaming to make this clear.

  ADVEC_ACPI_ID_{DEVICE}

Add a comment to express the importance of the order too.

[...]

> >> > +int write_gpio_dir(struct adv_ec_platform_data *adv_ec_data, unsigned
> >char PinNumber,
> >> > +		unsigned char value)
> >> > +{
> >> > +}
> >>
> >> All of the GPIO functions above should move into drivers/gpio.
> >>
> >> [Campion] Due to it has a flow need to cowork with EC chip and firmware, it
> >cannot be interrupt
> >>           by other functions. So it needs to keep in here.
> >
> >Please provide more details.
> 
> These gpio functions are common used for gpio to adjust the gpio direction and access its status. It has a complete lower process with EC that cannot be interrupted likes others. The flow is:
> 0.mutex lock to get the EC access
> 1.it needs wait IBF (Input Buffer Full) to be clear and then can send the command
> 2.Send read command to EC command port
> 3.wait IBF clear that means command is got by EC
> 4.send read address to EC data port
> 5.wait OBF (Output Buffer Full) data ready
> 6.get data from EC data port
> 7.mutex unlock
> So it cannot be interrupted as other EC lower access due to they use the same mutex to lock the flow. 

That's fine.  You can share a lock with the GPIO driver.

> >>           But vger.kernel.org returned the mail to mail as spam mail.
> >>           I will modity it as the following, is it OK?
> >>           examples:
> >>             - |
> >>           #include <dt-bindings/mfd/ahc1ec0-dt.h>
> >>           ahc1ec0 {
> >>                   compatible = "advantech,ahc1ec0";
> >>
> >>                   advantech,hwmon-profile =
> ><AHC1EC0_HWMON_PRO_UNO2271G>;
> >>                   advantech,watchdog = true;
> >
> >Shouldn't the watchdog be it's own sub-node?
> 
> it doesnot need so far. 

IMO, it's more clear than having a property.

[...]

> [Campion.Kang] So far, some registers or settings are have default value. They can be adjusted by runtime. 
> Is this ahc1ec0.yaml OK?

Rob has the final say on that.

-- 
Lee Jones [李琼斯]
Senior Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog