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
